[KEYCLOAK-18723] - Configurable constraints for request object encryption by pedroigor · Pull Request #8261 · keycloak/keycloak

pedroigor · 2021-07-13T11:25:22Z

No description provided.

pedroigor · 2021-07-13T16:34:20Z

@tnorimat Could you check if it makes sense for you?

tnorimat · 2021-07-13T20:30:11Z

@pedroigor Yes, it follows the item 1 of https://openbanking-brasil.github.io/specs-seguranca/open-banking-brasil-financial-api-1_ID1.html#section-5.2.2 .

tnorimat

@pedroigor I have a few comments.

Request object encryption's "alg" and "enc"
Unlike ID token encryption, It seems that only "alg" can be specified by client's setting(request_object_encryption_alg). How about supporting "enc" setting(request_object_encryption_enc)?
Server Metadata
It might be better to add server metadata "request_object_encryption_alg_values_supported". As mentioned just above, if "enc" setting(request_object_encryption_enc) is supported, it might be better to add also "request_object_encryption_enc_values_supported".

pedroigor · 2021-07-13T21:54:01Z

@pedroigor I have a few comments.

Request object encryption's "alg" and "enc"
Unlike ID token encryption, It seems that only "alg" can be specified by client's setting(request_object_encryption_alg). How about supporting "enc" setting(request_object_encryption_enc)?

Makes sense. I can update this.

Server Metadata
It might be better to add server metadata "request_object_encryption_alg_values_supported". As mentioned just above, if "enc" setting(request_object_encryption_enc) is supported, it might be better to add also "request_object_encryption_enc_values_supported".

Both are already supported as per #8243. See

keycloak/services/src/main/java/org/keycloak/protocol/oidc/OIDCWellKnownProvider.java

Line 134 in 1baab67

    
           config.setRequestObjectEncryptionAlgValuesSupported(getSupportedEncryptionAlgorithms());

.

tnorimat · 2021-07-14T00:44:31Z

@pedroigor Yes, I forgot it.

mposolda

+1 for the Takashi's concern about alg and enc .

Also I've added one inline comment.

Finally, There is this condition in both FAPI and OpenBanking Brasil FAPI about the restrictions for the used alg and enc :
https://openid.net/specs/openid-financial-api-part-2-1_0.html#encryption-algorithm-considerations
https://openbanking-brasil.github.io/specs-seguranca/open-banking-brasil-financial-api-1_ID1.html#section-6.1.1

I think that it will be nice to have executor (similar to SecureSigningAlgorithmExecutor), which will enforce this for new clients and check the algorithms for the existing clients. But that is probably something to consider in separate PR?

mposolda · 2021-07-19T17:36:33Z

        }

+        Boolean encryptionRequired = Optional.ofNullable(configuration.isEncryptionRequired()).orElse(Boolean.FALSE);
+        if (encryptionRequired && session.getAttribute(AuthzEndpointRequestParser.AUTHZ_REQUEST_OBJECT_ENCRYPTED) == null) {


@pedroigor It seems to me that with this flag encryptionRequired set to true, all clients in the realm (which will pass the client policy conditions) will be required to use encrypted request objects. Which I am not sure if it is desired behaviour.

IMO it will be good to stick with the same like for request object signatures. Which means that it would be enforced per the client if request object is encrypted just in case that request.object.encryption.alg is used for particular client. And this is already done in this PR (in AuthzEndpointRequestObjectParser).

In shortcut, my vote is to remove flag encryptionRequired from SecureRequestObjectExecutor. WDYT?

As you mentioned, clients that match the policy conditions.

I've opened a discussion on keycloak-dev about this topic in order to understand whether a setting should be at the client or at the client policy.

I agree with @tnorimat that moving configuration to policies is the best to do. And whether encryption is required looks like a good example.

As per signature, I guess it was done before client policies support. Right?

I'm not sure, for me either way works. Let me know if the above makes sense if not I can change to do the same as signature.

Yes, we discussed this already some time ago during work on client policies and during designing client policies. And I also agree that it will be nice to move many of client switches and keep them at the client policy level. This will allow some flexibility (EG. enforce secure ID Token signature algorithm just in case that scope=fapi is requested, which is possible due the fact that client policy condition will be evaluated just with the scope=fapi and the particular executor for enforce algorithm will be also called just at that time etc).

However one thing is, that some configurations are enforced by OIDC specification itself at the client level (like the request_object_encryption_alg and request_object_encryption_enc) and hence for those switches we need to stick them at the client level exactly as you did in this PR.

I am personally still not sure if the particular switch encryptionRequired is needed on the executor as we don't have similar thing for the signature, but I don't see this as a blocker in this PR as it is false by default anyway. so I am approving this. Thanks for the updates!

pedroigor · 2021-07-19T18:45:55Z

@tnorimat Added support for content encryption settings.

tnorimat · 2021-07-19T21:09:29Z

According to https://openid.net/specs/openid-connect-registration-1_0.html#ClientMetadata, it might be better to be

public static final String REQUEST_OBJECT_ENCRYPTION_ENC = "request.object.encryption.enc";

WDYT?

tnorimat · 2021-07-19T21:18:01Z

If https://github.com/keycloak/keycloak/pull/8261/files#r672633565 is agreed, it might be better to be getRequestObjectEncryptionEnc .

tnorimat · 2021-07-19T21:18:13Z

If https://github.com/keycloak/keycloak/pull/8261/files#r672633565 is agreed, it might be better to be setRequestObjectEncryptionEnc .

tnorimat · 2021-07-19T21:20:43Z

If https://github.com/keycloak/keycloak/pull/8261/files#r672633565 is agreed, it might be better to be 'request.object.encryption.enc.alg' .

Also to be requestObjectEncryptionEnc

tnorimat · 2021-07-19T21:22:09Z

If https://github.com/keycloak/keycloak/pull/8261/files#r672633565 is agreed, it might be better to be changeRequestObjectEncryptionEnc .

tnorimat · 2021-07-19T21:22:27Z

If https://github.com/keycloak/keycloak/pull/8261/files#r672633565 is agreed, it might be better to be 'request.object.encryption.enc' .

tnorimat · 2021-07-19T21:22:43Z

The same just above.

tnorimat · 2021-07-19T21:23:38Z

If https://github.com/keycloak/keycloak/pull/8261/files#r672633565 is agreed, it might be better to be

<label class="col-md-2 control-label" for="requestObjectEncryptionEnc">{{:: 'request-object-encryption-enc' | translate}}</label>

tnorimat · 2021-07-19T21:24:43Z

If https://github.com/keycloak/keycloak/pull/8261/files#r672633565 is agreed, it might be better to be

<select class="form-control" id="requestObjectEncryptionEnc" ng-change="changeRequestObjectEncryptionEnc()" ng-model="requestObjectEncryptionEnc">

tnorimat · 2021-07-19T21:25:16Z

If https://github.com/keycloak/keycloak/pull/8261/files#r672633565 is agreed, it might be better to be 'request-object-encryption-enc.tooltip'

tnorimat · 2021-07-19T21:26:16Z

The same just above.

…tion

pedroigor · 2021-07-19T21:40:06Z

@tnorimat The name works for me too. Changed.

tnorimat

@pedroigor I've confirmed your changes. Thank you.

pedroigor added area/oidc Indicates an issue on OIDC area kind/enhancement Categorizes a PR related to an enhancement labels Jul 13, 2021

pedroigor requested a review from mposolda July 13, 2021 12:26

tnorimat reviewed Jul 13, 2021

View reviewed changes

mposolda requested changes Jul 19, 2021

View reviewed changes

pedroigor force-pushed the KEYCLOAK-18723 branch 3 times, most recently from 89c9587 to 1734c8a Compare July 19, 2021 18:44

tnorimat reviewed Jul 19, 2021

View reviewed changes

[KEYCLOAK-18723] - Configurable constraints for request object encryp…

86be626

…tion

pedroigor force-pushed the KEYCLOAK-18723 branch from 1734c8a to 86be626 Compare July 19, 2021 21:38

tnorimat reviewed Jul 19, 2021

View reviewed changes

mposolda approved these changes Jul 20, 2021

View reviewed changes

mposolda merged commit 396a78b into keycloak:master Jul 20, 2021

pedroigor deleted the KEYCLOAK-18723 branch August 3, 2021 12:19

fjuma mentioned this pull request Oct 30, 2023

[WFLY-17143] Preview - Add the ability to specify that the OIDC Authentication Request should include request and request_uri parameters wildfly/wildfly-proposals#532

Merged

Uh oh!

Conversation

pedroigor commented Jul 13, 2021

Uh oh!

pedroigor commented Jul 13, 2021

Uh oh!

tnorimat commented Jul 13, 2021

Uh oh!

tnorimat left a comment

Choose a reason for hiding this comment

Uh oh!

pedroigor commented Jul 13, 2021

Uh oh!

tnorimat commented Jul 14, 2021

Uh oh!

mposolda left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedroigor Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedroigor commented Jul 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedroigor commented Jul 19, 2021

Uh oh!

tnorimat left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pedroigor Jul 19, 2021 •

edited

Loading