Skip to content
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

Use token endpoint URL as OIDC JWT authentication audience #21319

Merged
merged 1 commit into from
Nov 10, 2021

Conversation

sberyozkin
Copy link
Member

@sberyozkin sberyozkin commented Nov 9, 2021

Fixes #21310.

This PR is about improving (one may say about bug fixing of) the way OIDC JWT client authentication code sets the audience.

The client JWT authentication is specified here: https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication

Quarkus supports and tests all the options there. The only problem, which has been caught by #21310 is that the audience claim value is not a recommended token endpoint URL (per the spec text: The Audience SHOULD be the URL of the Authorization Server's Token Endpoint.).

Currently it is the value of either quarkus.oidc.auth-server-url or quarkus.oidc-client.auth-server-url, so for example, in case of Keycloak, it would be http://localhost:8180/auth/realms/quarkus vs the recommended http://localhost:8180/auth/realms/quarkus/protocol/openid-connect/token (note the extra /protocol/openid-connect/token path).

Given that the OIDC spec does recommend using the token path and the token path is known to OIDC, suggesting as I did to fix #21310 with

quarkus.oidc-client.auth-server-url=https://dev.api.service.nhs.uk/oauth2
quarkus.oidc-client.discovery-enabled=false # token path MUST be set
quarkus.oidc-client.token-path=/token

#and now
quarkus.oidc-client.jwt.credentials.jwt.audience=https://dev.api.service.nhs.uk/oauth2/token

is a poor deal for the users - here this is just redundant - but worse is the case where the auto-discovery works and the OIDC provider is as strict as the one used in #21310 - despite the endpoint URLs being discovered, one would still have to type the token endpoint URL as the audience which may not be even easy to find.

So this PR just improves the code a tiny bit only to use the token endpoint URL as opposed to the base URL of the provider.
We have 4 tests with Keycloak (3 in the oidc client module, one in he oidc code flow tests) - all of them pass, but, just in case (which I doubt will happen) I've still added an audience property so that one can refer to the base URL and updated one of the OidcClient tests (note Keycloak does not like audience values with the trailing path separators - so I added the code to strip it).

It is hard to add the tests confirming the new audience property is effective - since the token does not reach Quarkus at all and goes directly into Keycloak, while trying to wiremock it would not be very useful - it is the behavior of Keycloak which is important to verify. So I can only confirm that the new property is effective - this is why I had to add the code to strip the path segment :-), otherwise Keycloak returns a failure.

@sberyozkin
Copy link
Member Author

Proposing for a backport - I'm not 100% sure it can qualify as a bug fix, so Guillaume, please remove the label if you prefer.

@sberyozkin
Copy link
Member Author

Thanks @gastaldi, I'll wait till tomorrow just in case for Pedro to comment - though it is a safe fix IMHO

@sberyozkin sberyozkin merged commit 888daed into quarkusio:main Nov 10, 2021
@sberyozkin sberyozkin deleted the oidc_client_jwt_aud branch November 10, 2021 09:31
@gsmet gsmet added this to the 2.6 - main milestone Nov 10, 2021
@liamor
Copy link

liamor commented Nov 11, 2021

Thanks @sberyozkin . I just want to confirm that this does work for my initial use case.

@gsmet gsmet modified the milestones: 2.6 - main, 2.4.2.Final Nov 11, 2021
@gsmet gsmet changed the title Use token endoint URL as OIDC JWT authentication audience Use token endpoint URL as OIDC JWT authentication audience Nov 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OIDC client needs extra flexibility
4 participants