-
Notifications
You must be signed in to change notification settings - Fork 7.7k
Make sure that there is single audience allowed by default in JWT tok… #38830
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
Conversation
…ens sent to client authentication closes keycloak#38819 Signed-off-by: mposolda <mposolda@gmail.com> Co-authored-by: Thomas Darimont <thomas.darimont@googlemail.com> Co-authored-by: mposolda <mposolda@gmail.com>
Thanks @mposolda to take this over. I think the essence of the revised audience was not to just use exactly one value, but to use the issuer url as is indicated by the spec.
...while gracefully ignoring the I think this PR still allows the usage of non-issuer URLs according to this method: https://github.com/keycloak/keycloak/pull/38830/files#diff-6d5e969bcfdea82e54294def66d68f1b19188d943919cdcdb426cb752373e9a4R279 I think to be compliant with the spec, we must enforce that the issuer URL is used as the audience if |
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 @mposolda! LGTM!
@thomasdarimont Yes, you are right. The small problem with enforcing issuer URL is, that it is backwards incompatible change in the OIDC specification. As latest stable (non-draft) version of OIDC specification [1] still mentions that "The Audience SHOULD be the URL of the Authorization Server's Token Endpoint." . Hence people relying on the latest stable OIDC version will be forced to use backwards compatibility option, which is not ideal and also will not add any security benefit. For fixing the actual security issue, the most important is the single audience. Some more context in the comment [2] [1] https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication |
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 understand the rationale to remain compatible with the /current/ OIDC core spec. However, this will not suffice to be compatible with FAPI 2.0 Security Profile finale version.
See: https://openid.net/specs/fapi-security-profile-2_0-final.html#section-5.3.2.1-2.8
I propose to add an additional note to the release notes that we'll use the issuer URL in a future release for the audience for JWT client assertions to be compatible with OIDC Core 1.0 Errata 3 / FAPI 2.0 Security Profile.
Btw. the latest OIDC Core version already shows the changes Errata 3 ("released" in January) - but still in draft https://openid.bitbucket.io/connect/openid-connect-core-1_0.html
whereas the URL https://openid.net/specs/openid-connect-core-1_0.html still shows Errata 2.
However, I'm good with the change in general, but we need to do a follow up change to use the issuer in the not too distant future :)
LGTM.
@thomasdarimont Thanks for the feedback! +1 that we will need to follow and enforce using the "issuer" . I think we can use your original issue #38751 for tracking this? If you agree, we can maybe just slightly update the description of the issue with the reference to this issue #38819 . WDYT? I think we will need to do that once the OIDC specification is updated to errata 3. |
Regarding documentation, I've added recommendation to the upgrading guide, that people are recommended to use the "issuer URL" . Let's address the rest as a follow-up once OIDC specification is updated. |
…ens sent to client authentication
closes #38819