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

Update OIDC client and token propagation to support a jwt-bearer token grant #29130

Merged
merged 1 commit into from Nov 9, 2022

Conversation

sberyozkin
Copy link
Member

@sberyozkin sberyozkin commented Nov 8, 2022

Fixes #18649.

This PR looks quite massive but in fact the changes are trivial, the bulk of the changes are related to adding tests to both extensions/oidc-token-propagation/deployment and extensions/oidc-token-propagation-reactive/deployment. Here is a summary:

  • OidcClientConfig has a jwtbearer grant type added which is a shortcut for urn:ietf:params:oauth:grant-type:jwt-bearer
  • RestClient Classic Client AccessTokenRequestFilter in oidc-token-propagation can already use OidcClient to exchange the token using the token exchange grant - OidcClient manages it all, AccessTokenRequestFilter only supplies the current token to OidcClient using a property set by default to subject_token which is relevant in the context of the exchange token grant. The mechanics are exactly the same if jwtbearer has to be used to exchange the token but assertion, instead of subject_token, has to be used to supply the current token to OidcClient - so AccessTokenRequestFilter has been updated to deduce this property name based on the configured grant
  • Keycloak does not support jwtbearer so we need use Wiremock to test, I updated the test oidc-server to have a jwtbearer stub
  • Added test to extensions/oidc-token-propagation/deployment: Test client acquires a token for alice and posts it as a bearer token to FrontendResource which delegates further to ProtectedResource, at this point AccessTokenRequestFilter requests a token exchange, Wiremock stub is setup to exchange it for a token issued to bob - and ProtectedResource return the user name from the new token, test confirms that the original token user name was alice but now it is bob
  • Updated oidc-token-propagation-reactive to support the token exchange exactly the same way it can be done for oidc-token-propagation, AccessTokenRequestReactiveFilter will suspend/resume the request thread if it needs to exchange the token; test is identical as well

@gastaldi @geoand Please have a look when you get a chance, as I said, the actual main source changes are fairly simple, the complexity is within the tests only. It is RFE.

I'll also follow up with another PR to let uses register the reactive token propagation filter with the annotation shortcut, similarly to how Georgios did it for the reactive OidcClientFilter

Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, added a small suggestion about the enum constant

* 'urn:ietf:params:oauth:grant-type:jwt-bearer' grant requiring an OIDC client authentication as well as
* at least an 'assertion' parameter which must be passed to OidcClient at the token request time.
*/
JWTBEARER("urn:ietf:params:oauth:grant-type:jwt-bearer"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
JWTBEARER("urn:ietf:params:oauth:grant-type:jwt-bearer"),
JWT_BEARER("urn:ietf:params:oauth:grant-type:jwt-bearer"),

Copy link
Member Author

@sberyozkin sberyozkin Nov 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @gastaldi One thing I've been following with these grant names is introducing the shortcuts containing -, _, for example, client_credentials -> client, urn:ietf:params:oauth:grant-type:token-exchange -> exchange, etc. One thing it avoids is hard to catch issues with the extra parameters which I vaguely recall we had when client was named as client_credentials, where the user typed extra-params.client-credentials.a=b.
I've been thinking though earlier to have even a shorter shortcut, jwt -> urn:ietf:params:oauth:grant-type:jwt-bearer - perhaps we can try this one ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, yes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for keeping pushing, just noticed it :-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, jwt is much simpler, thanks for suggesting a grant name change in the first place as well

Quarkus Documentation automation moved this from To do to Reviewer approved Nov 8, 2022
@sberyozkin sberyozkin force-pushed the oidc_client_jwt_bearer branch 2 times, most recently from 23b67b1 to 1fb0ecb Compare November 8, 2022 17:34
@sberyozkin sberyozkin merged commit 6ba114c into quarkusio:main Nov 9, 2022
Quarkus Documentation automation moved this from Reviewer approved to Done Nov 9, 2022
@quarkus-bot quarkus-bot bot added this to the 2.15 - main milestone Nov 9, 2022
@sberyozkin sberyozkin deleted the oidc_client_jwt_bearer branch November 9, 2022 10:19
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Nov 9, 2022
@geoand
Copy link
Contributor

geoand commented Nov 9, 2022

👍🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Update OidcClient to support JWT bearer grant and Token propagation filter to use it
3 participants