-
Notifications
You must be signed in to change notification settings - Fork 81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WFLY-17143] Preview - Add the ability to specify that the OIDC Authentication Request should include request and request_uri parameters #532
Conversation
1b7f98a
to
c4629b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @PrarthonaPaul! I've started adding a few comments here.
|
||
== Overview | ||
|
||
OpenID Connect is an authentication mechanism that builds on OAuth 2.0 and allows a user to login to a web application using credentials established by an OpenID provider. Authentication requests sent using OpenID Connect can be signed and optionally encrypted. This can be achieved using 2 auth request parameters: `request` and `request_uri`, which are both optional. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should mention in the overview that the elytron-oidc-client
subsystem doesn't currently include the ability to specify the request
or request_uri
in the authentication request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed!
=== Related Issues | ||
|
||
* https://issues.redhat.com/browse/EAP7-1974[EAP7-1974] | ||
* [INSERT ELY ISSUE] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to create the corresponding ELY issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added!
|
||
=== Hard Requirements | ||
|
||
* Two new attributes named `request` and `request-uri` will be added to the `secure-deployment` resource under the `elytron-oidc-client` subsystem, which will be used to specify either the `request` or the `request_uri`, which are both JWTs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What will be the type of the new attributes? (i.e., what will their allowed values be, what will they default to, etc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed!
|
||
* Two new attributes named `request` and `request-uri` will be added to the `secure-deployment` resource under the `elytron-oidc-client` subsystem, which will be used to specify either the `request` or the `request_uri`, which are both JWTs. | ||
|
||
* According to the https://issues.redhat.com/browse/EAP7-1974[OIDC specs], only one of the two parameters can be specified at a time. If `request` is added, then `request_uri` MUST NOT be used in the same request. However, the user can choose to specify neither. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the wrong link here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
||
* The feature must deal with cases where the OpenId Provider does not support request parameters by recognizing `request_not_supported` error and dealing with it accordingly. | ||
|
||
* It should be possible to specify that these parameters should be included in the Authentication Request via deployment configuration using the `oidc.json` file inside the `WEB-INF` directory of the web application and `elytron-oidc-client` subsystem configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What will the new attributes in oidc.json look like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
* | ||
|
||
== Community Documentation | ||
Documentation for the new scope option will be added to https://github.com/wildfly/wildfly/blob/main/docs/src/main/asciidoc/_admin-guide/subsystem-configuration/Elytron_OIDC_Client.adoc[Elytron OpenID Connect Client Subsystem Configuration]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated! Thank you for the reviews!
876b195
to
14b9503
Compare
|
||
=== Hard Requirements | ||
|
||
* A new attribute named `request-object-required` will be added to the `secure-deployment` resource under the `elytron-oidc-client` subsystem. The value for this attribute will be a String with 3 acceptable values: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A new attribute named
request-object-required
will be added to thesecure-deployment
resource under theelytron-oidc-client
subsystem.
Why just to secure-deployment
? Why not also to provider
, realm
, and/or secure-server
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @OndrejKotek,
Thank you for your review. Adding this attribute to secure-server
makes sense and I can add that to the analysis doc. However, I am not sure if this attribute relates to realm
or provider_url
since request parameters encrypt query parameters. Additionally, you cannot configure client-id
using realm
or provider_url
, which is needed for the auth request. So, they would have to configure secure-server
or secure-deployment
regardless. However, if you see other benefits to having request-object-required
added to realm
and provider_url
, I am happy to add it there!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUIC, provider
and realm
resources are just containers for settings that can be referenced in secure-deployment
or secure-server
resources. It makes sense to me to allow to define request-object-required
also in such resources (at higher/more global configuration levels) like the other attributes. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that makes sense! This way if we have two secure-deployment
s using the same realm
or provider
, then we don't need to set request-object-required
multiple times. And if we want to have multiple secure-deployment
or secure-server
s using the same realm but one with request_uri
and the other with plaintext query, then they can declare it under secure-server
directly.
Ill update the docs and implement it accordingly on the subsystem.
Thank you!
14b9503
to
efee31f
Compare
|
||
* The `request-object-required` attribute will specify if either `request` or the `request_uri` would be added to the auth request. This will be an optional attribute. The default value for this attribute will be set to `none`, indicating that neither request parameters are used and the query parameters are sent through the url. | ||
|
||
** For multiple `secure-server` or `secure-deployment`s using the same `realm` or `provider`, the `request-object-required` can be specified at the higher level resource. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`secure-deployment`s
This formatting does not work, there needs to a space before the s
. I suggest to use something like `secure-deployment` resources
instead.
The same in the next point/sentence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed!
c082d6b
to
43c47d1
Compare
|
||
=== Hard Requirements | ||
|
||
* A new attributes named `request-object-required`, `request-signing-algorithm`, `request-encrypt-algorithm` and `request-encrypt-enc` will be added to the `secure-deployment`, `secure-server`, `realm` and `provider` resources under the `elytron-oidc-client` subsystem. The value for the `request-object-required` attribute will be a String with 3 acceptable values: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of request-object-required
, I'm wondering if authentication-request-format
might be a better name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds better. I can change it to this. Thanks for the suggestion!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
=== Hard Requirements | ||
|
||
* A new attributes named `request-object-required`, `request-signing-algorithm`, `request-encrypt-algorithm` and `request-encrypt-enc` will be added to the `secure-deployment`, `secure-server`, `realm` and `provider` resources under the `elytron-oidc-client` subsystem. The value for the `request-object-required` attribute will be a String with 3 acceptable values: | ||
** none |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of none
, maybe we can call it oauth2
(for the standard OAuth2 request format)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
|
||
** The values for `request-signing-algorithm` attribute are of type `String`. Default value for this attribute would be `none`, specifying that the request would be sent as a plaintext. The default value of `request-signing-algorithm` is `none`. The signing algorithm specified must be one of the signing algorithms accepted by the OpenID provider. | ||
|
||
* The request object may also be encrypted. To specify that the request parameter will be encrypted, the user needs to specify the relevant algorithms using `request-encrypt-algorithm` and `request-encrypt-enc`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just looking at https://openid.net/specs/openid-connect-core-1_0.html#EncryptedRequestObject.
It mentions "If the Authorization Server has advertised JWE encryption algorithms in the request_object_encryption_alg_values_supported and request_object_encryption_enc_values_supported elements of its Discovery document [OpenID.Discovery], or has supplied encryption algorithms by other means, these are used by the Client to encrypt the JWT."
To keep things simple, instead of making the encryption algorithm configurable, we could just rely on discovery, i.e., we can use one of the algorithms discovered from request_object_encryption_alg_values_supported
and request_object_encryption_enc_values_supported
.
(Note that the discovery code is here: https://github.com/wildfly-security/wildfly-elytron/blob/2.x/http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcClientConfiguration.java#L215-L253)
@OndrejKotek What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know if Keycloak supports request object encryption? (Was just taking a look at the response from http://localhost:8080/auth/realms/WildFly/.well-known/openid-configuration and I do see request_object_signing_alg_values_supported
but I don't seem to see request_object_encryption_alg_values_supported
or request_object_encryption_enc_values_supported
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your comment.
I have added to the discovery code to extract openid information, such as algorithms that are supported for signing and encrypting. There are multiple algorithms for all three supported by keycloak.
For example here are the algorithms that keycloak supports for encrypting:
This is why I allowed the user to specify the algorithms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And yes, Keycloak does support request object encryption (please see image above). We can do a check to see if the algorithm specified by the user is one of the ones supported and throw an error/exception otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarifying that, if Keycloak provides the ability to configure that, then it's fine to have corresponding attributes in the subsystem / JSON configuration.
How about renaming to request-object-encryption-algorithm
and request-object-content-encryption-algorithm
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
|
||
* The Request Object MAY be signed or unsigned (plaintext) and can be specified by value or by reference. The user can specify the algorithm used to sign the JWT request using the `request-signing-algorithm` attribute which will also be added to the resources specified above. | ||
|
||
** The values for `request-signing-algorithm` attribute are of type `String`. Default value for this attribute would be `none`, specifying that the request would be sent as a plaintext. The default value of `request-signing-algorithm` is `none`. The signing algorithm specified must be one of the signing algorithms accepted by the OpenID provider. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could call this request-object-signing-algorithm
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated! I have also added a point about configuring the keystore information using existing keystore file related attributes.
Thank you for the comments :)
43c47d1
to
9491f5b
Compare
|
||
=== Hard Requirements | ||
|
||
* A new attributes named `authentication-request-format`, `request-object-signing-algorithm`, `request-object-encryption-algorithm` and `request-object-content-encryption-algorithm` will be added to the `secure-deployment`, `secure-server`, `realm` and `provider` resources under the `elytron-oidc-client` subsystem. The value for the `authentication-request-format` attribute will be a String with 3 acceptable values: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor, s/A new attributes/New attributes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
* A new attributes named `authentication-request-format`, `request-object-signing-algorithm`, `request-object-encryption-algorithm` and `request-object-content-encryption-algorithm` will be added to the `secure-deployment`, `secure-server`, `realm` and `provider` resources under the `elytron-oidc-client` subsystem. The value for the `authentication-request-format` attribute will be a String with 3 acceptable values: | ||
** oauth2: Indicating that the query parameters will be sent as usual following the oauth2 syntax. | ||
** request: Indicating that the query will be sent as a jwt by value. | ||
** request_uri: Indicating that the query will be sent as a reference to the jwt. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be consistent with the format of the other attribute names, I think we could use request-uri
instead of request_uri
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, since this is a value and not an attribute name, we will keep it as request_uri
to be consistent with the specifications.
** request: Indicating that the query will be sent as a jwt by value. | ||
** request_uri: Indicating that the query will be sent as a reference to the jwt. | ||
|
||
* The `authentication-request-format` attribute will specify if either `request` or the `request_uri` would be added to the auth request. This will be an optional attribute. The default value for this attribute will be set to `none`, indicating that neither request parameters are used and the query parameters are sent through the url. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/would be added/should be added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default value will be oauth2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
|
||
** For multiple `secure-server` or `secure-deployment` resources using the same `realm` or `provider`, the `authentication-request-format` can be specified at the higher level resource. | ||
|
||
** For multiple `secure-server` or `secure-deployment` resources using the same higher level resource, if the Request Object type is not the same, then the authentication-request-format` attribute can be set accordingly for the individual lower level resources. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
authentication-request-format is missing the beginning `
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
|
||
** For multiple `secure-server` or `secure-deployment` resources using the same higher level resource, if the Request Object type is not the same, then the authentication-request-format` attribute can be set accordingly for the individual lower level resources. | ||
|
||
* According to the https://openid.net/specs/openid-connect-core-1_0.html#JWTRequests[OIDC specs], only one of the two parameters can be specified at a time. If `request` is added, then `request_uri` MUST NOT be used in the same request. However, the user can choose to specify neither by setting `authentication-request-format` to `none`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/none/oauth2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
Thank you for the comments!
I have added more information about the key configurations and how request_uri value is obtained.
/subsystem=elytron-oidc-client/secure-deployment:add(client_id=myclient, provider-url="http://localhost:8090/", authentication-request-format="request_uri") | ||
``` | ||
|
||
* For signed and encrypted request objects, we must need to use a private key, whose corresponding public key and certificate must be communicated with the OpenID provider. The public key is communicated to the OpenID provider using the admin console of the OpenID provider (i.e. Keycloak). And the same keypair is added to the deployment settings using existing attributes `client-keystore`, `client-keystore-password`, `client-key-password`, which are available under the `secure-deployment`, `secure-server`, `realm` and provider resources. Additionally, `client-key-alias` and `client-keystore-type` attributes will be added to these resources for additional information about the keystore. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm, I think client-key-alias
and client-keystore-type
are part of the JSON configuration already, right? So here, they just need to be added to the subsystem configuration, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the comments. I will address these changes.
Looking at the schema for wildfly, it seems that elytron-oidc-client.secure-deployment.credential.client-keystore-type
and etc. already exists. This is used to specify the client credentials, such as the client secret using a keystore. But we need to use it for the secure-deployment
, secure-server
, realm
and provider
. So, we will be adding it one level higher as well.
9491f5b
to
d4d2f91
Compare
|
||
* According to the OpenID docs, if the same parameter exists both in the Request Object and the OAuth Authorization Request parameters, the parameter in the Request Object is used. | ||
|
||
* The feature must deal with cases where the OpenId Provider does not support request parameters by recognizing `request_not_supported` error and dealing with it accordingly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by recognizing
request_not_supportederror and dealing with it accordingly.
what will be the approach for this situation? Will some error be thrown that says that request parameters are not supported and only oauth2 format is supported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now I have coded it so that if either request or request_uri is not supported, then we send it as usual.
But this may not be as secure. So, in that case, if it makes more sense to, I can change it to throw a runtime error saying that request/request_uri is not supported by the OpenID provider
Which one do you think would be more appropriate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PrarthonaPaul I think we should log this and then document that the request and the request_uri methods are used only if available with the OpenID provider, if not available then the warning message will be logged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is an excellent idea! I will add that to the code.
Thank you!
* The Request Object MAY be signed or unsigned (plaintext) and can be specified by value or by reference. The user can specify the algorithm used to sign the JWT request using the `request-object-signing-algorithm` attribute which will also be added to the resources specified above. This algorithm must be one of the Request object signature algorithms supported by the | ||
OpenID provider. | ||
|
||
** The values for `request-object-signing-algorithm` attribute are of type `String`. Default value for this attribute would be `none`, specifying that the request would be sent as a plaintext.In order to sign the jwt using an algorithm other than `none`, the user must specify the KeyPair used. This can be done using the `client-keystore-file`, `client-keystore-password`, `client-key-password`, `client-key-alias` and `client-keystore-type` attributes. The algorithm for the KeyPair must match the algorithm used to sign the JWT. The keystore should be on type `JKS` and can be generated using `keytool` on the linux terminal. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PrarthonaPaul Just a minor, s/JKS/PKCS12
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yes. I will change it. Should we allow both JKS and PKCS since keycloak allows both?
I tested both manually and they both work. (PKCS12 is a bit picky when it comes to EC algorithm)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
d4d2f91
to
a8de81d
Compare
|
||
This RFE will add an attribute to the `elytron-oidc-client` subsystem called `authentication-request-format`, which can allow the user to specify if they will use either of the request parameters. Additionally, we will be adding supporting attributes called `request-object-signing-algorithm`, `request-object-encryption-algorithm` and `request-object-content-encryption-algorithm`, which will be used to specify if and how the request parameters will be signed and/or encrypted. All of these attributes will be added to the `secure-server`, `secure-deployment`, `provider` and `realm` resources, which the user can configure using either the `elytron-oidc-client` subsystem, or using a json file in deployment configuration. | ||
|
||
We will also be adding some keystore attributes, such as, `client-keystore-file`, `client-keystore-password`, `client-key-password`, `client-key-alias` and `client-keystore-type` to the resources mentioned above. These attributes are currently available under the `credential` attribute. However, when they are configured outside of the `credentials` attribute, they will be used to sign Request Objects. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"to the resources mentioned above"
Just looking at the current schema and it's currently possible to configure client-keystore-password
, client-keystore
, etc. for realm
, provider
, secure-deployment
, and secure-server
resources to specify the client keystore configuration to be used for HTTPS connections. Will the attributes being added conflict with these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could conflict if both OIDC and HTTPS are configured at the same time.
Would there be a case where the user would want to configure OIDC with HTTPS?
If so, then I can use two new attributes called oidc-client-keystore
and oidc-client-keystore-password
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's possible that a user would want their request to be signed and also want to use HTTPS for communication with the OpenID provider. I think it would be good to use attribute names that explain what the attribute is for (e.g., something like request-object-signing-keystore-file
perhaps).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
I will add those two attributes and edit the proposal.
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Note that this applies to all the signing keystore related attributes mentioned above, i.e.,
client-keystore-file
, client-keystore-password
, client-key-password
, client-key-alias
and client-keystore-type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These attribute have been renamed to request-object-signing-keystore-file
, request-object-signing-keystore-password
, request-object-signing-key-password
, request-object-signing-key-alias
and request-object-signing-keystore-type
|
||
** The values for the `request-object-encryption-algorithm` and the `request-object-content-encryption-algorithm` attributes are of type `String`. The default value for the attributes are undefined, which specifies that the request object will not be encrypted. | ||
|
||
** The request object will only be encrypted if both `request-object-encryption-algorithm` and `request-object-content-encryption-algorithm` are specified. The values for these attributes must be part of the encryption algorithms and content encryption methods supported by the OpenID provider. These values can also be found by sending a GET request to the discovery url or by looking at the client configurations on the admin console of the OpenID provider. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to mention the names of the attributes in the result of the GET request to the discovery url that contain the allowed values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the output of this would be a bit long.
Do you think it would be better to point to https://github.com/wildfly-security/wildfly-elytron/blob/2.x/http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcClientConfiguration.java#L212-L238 and https://github.com/wildfly-security/wildfly-elytron/blob/2.x/http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcProviderMetadata.java for more information?
Alternatively, we could give some examples of what metadata is obtained through this GET request and link either the elytron source code links I added above or this keycloak doc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could just mention the specific ones that contain the info we're interested in (I think you had these in a previous version of the proposal), e.g., request_object_signing_alg_values_supported
, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
I have made this change to include the attributes relevant to Request Objects.
|
||
** The request object will only be encrypted if both `request-object-encryption-algorithm` and `request-object-content-encryption-algorithm` are specified. The values for these attributes must be part of the encryption algorithms and content encryption methods supported by the OpenID provider. These values can also be found by sending a GET request to the discovery url or by looking at the client configurations on the admin console of the OpenID provider. | ||
|
||
** The JWT must be signed first and then encrypted to make a nested JWT and the key used to encrypt the JWT must be the realm public key shared by the OpenID provided. The key algorithm must be the same as the encrypting algorithm. Signing and encrypting algorithms can be of different types (i. e. ES-256 for signing and RSA-OAEP for encrypting is allowed). The realm key for encrypting the JWT can be obtained by sending a GET request to the `jwksUri` obtained from the `discoveryUrl`(http://localhost:8080/realms/myrealm/.well-known/openid-configuration). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/OpenID provided/OpenID provider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to add some links to Keycloak issues/PRs/docs where the request object encryption is described, e.g.,
https://issues.redhat.com/browse/KEYCLOAK-18630
keycloak/keycloak#8243
keycloak/keycloak#8261
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have some sample output that shows the result of sending a GET request to the jwksUri
endpoint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added the links and a sample output with some of the longer values truncated for readability.
cc17b5c
to
8201c5c
Compare
8201c5c
to
536579b
Compare
536579b
to
0717791
Compare
|
||
** The certificate for the KeyPair used to sign the JWT must be shared with the OpenID provider either by importing it from the keystore, or by uploading a jwksUrl. | ||
|
||
** In order to be able to upload client keys on the admin console of the OpenID Provider, the `Client Authentication` toggle must be turned on. As a result, when configuring the client in deployment or subsystem settings, the `public-client` attribute must be set to `false` and client credentials must be specified as well using the `credentials` attribute. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is specifically about the Keycloak provider right? (since this mentions the Client Authentication
toggle).
|
||
This RFE will add an attribute to the `elytron-oidc-client` subsystem called `authentication-request-format`, which can allow the user to specify if they will use either of the request parameters. Additionally, we will be adding supporting attributes called `request-object-signing-algorithm`, `request-object-encryption-algorithm` and `request-object-content-encryption-algorithm`, which will be used to specify if and how the request parameters will be signed and/or encrypted. All of these attributes will be added to the `secure-server`, `secure-deployment`, `provider` and `realm` resources, which the user can configure using either the `elytron-oidc-client` subsystem, or using a json file in deployment configuration. | ||
|
||
We will also be adding some keystore attributes, such as, `request-object-signing-keystore-file`, `request-object-signing-keystore-password`, `request-object-signing-key-password`, `request-object-signing-key-alias` and `request-object-signing-keystore-type` to the resources mentioned above. There are similar attributes that are currently available under the `credential` attribute. However, when they are configured outside of the `credentials` attribute, they will be used to sign Request Objects. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fjuma I see this is re-using a pattern already in use in this subsystem but is there any reason we are not making use of the key-store capability so we can rely on the Elytron subsystem handling all of the different types of KeyStore?
Or possibly something using a credential reference / credential source type pattern?
Maybe this could be some kind of follow up consideration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this is re-using a pattern that was already present elsewhere in the subsystem.
We could switch to making use of the key-store
capability instead to simplify things.
We had previously talked about introducing that elsewhere in the subsystem too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fjuma Should we move that to a Jira as a follow up consideration so it does not block this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
=== Related Issues | ||
|
||
* https://issues.redhat.com/browse/EAP7-1974[EAP7-1974] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove the EAP7 reference as we are going for community stability level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove the EAP7 reference and I think we should have a follow up to consider the key-store reference but adding my approval.
0717791
to
29d2cac
Compare
* Tests will be added for both subsystem and deployment configurations. | ||
* Tests may be added to ensure that server configuration fails when the stability level is not specified appropriately. | ||
|
||
== Community Documentation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just minor, I think the Release Note Content section is missing here (see https://github.com/wildfly/wildfly-proposals/blob/main/design-doc-template.adoc).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
I have added the section, but not sure what to add.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a short summary about what the new feature provides (this is used when creating the blog post about the WildFly release).
|
||
** In order to be able to upload client keys on the admin console of the OpenID Provider, the `Client Authentication` toggle must be turned on. As a result, when configuring the client in deployment or subsystem settings, the `public-client` attribute must be set to `false` and client credentials must be specified as well using the `credentials` attribute. | ||
|
||
* The request object may also be encrypted. To specify that the Request Object will be encrypted, the user needs to specify the relevant algorithms using `request-object-encryption-algorithm` and `request-object-content-encryption-algorithm`. Encryption is done using https://openid.net/specs/draft-jones-json-web-encryption-02.html#[JWE]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be updated based on the discussion on the Elytron PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made this update and squashed the commits.
Thanks @fjuma !
@PrarthonaPaul Please squash the commits when you get a chance. Thanks! |
b1d74f5
to
cd8849e
Compare
…ntication Request should include request and request_uri parameters
cd8849e
to
e2097a3
Compare
Issue: https://issues.redhat.com/browse/WFLY-17143
related issue: https://issues.redhat.com/browse/ELY-2584
Relevant github discussions: