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

Allow token verification with user info when no introspection endpoint is available #29715

Conversation

michalvavrik
Copy link
Member

@michalvavrik michalvavrik commented Dec 6, 2022

closes: #20911

Makes it possible to use user-info endpoint with providers that does not provide introspection endpoint for Bearer token indirect verification, so that providers like GitHub may be used with service endpoints. I tested it with real GitHub application (not mock).

@quarkus-bot

This comment has been minimized.

@sberyozkin
Copy link
Member

Thanks Michal @michalvavrik for giving it a try - it does look quite good, I've left a good number of comments, but nothing major really, for example if it is a token in JWT format then the provider must be able to verify this token in a standard way, by providing the verification keys or having an introspection endpoint.

Re the bearer token vs access token, the bearer tokens are always the access tokens, that is true, but if we have Code Flow then we get ID token (always JWT) and access token - which can be opaque as with Google/Azure/etc, so it would be more flexible to have access token in the name

Cheers

@michalvavrik michalvavrik force-pushed the feature/oidc-service-app-user-info-token-verification branch from afab697 to 34d9ada Compare December 7, 2022 12:35
@michalvavrik
Copy link
Member Author

Thank you @sberyozkin , I've learned a lot from your comments and adjusted PR accordingly.

@michalvavrik michalvavrik changed the title Allow token verification with user info when no introspection endpoint Allow token verification with user info when no introspection endpoint is available Dec 7, 2022
@quarkus-bot

This comment has been minimized.

@michalvavrik michalvavrik force-pushed the feature/oidc-service-app-user-info-token-verification branch 2 times, most recently from bd43468 to a9470dc Compare December 7, 2022 19:03
@quarkus-bot

This comment has been minimized.

@michalvavrik
Copy link
Member Author

michalvavrik commented Dec 7, 2022

Win/IT test failures are interesting, best guess is that it's down to the test order which makes them flaky. I'll investigate tonight or tomorrow when I find a time. Update: I think I found it, let see what CI says.

@michalvavrik michalvavrik force-pushed the feature/oidc-service-app-user-info-token-verification branch from a9470dc to 48e0ea8 Compare December 7, 2022 22:53
@quarkus-bot

This comment has been minimized.

@sberyozkin
Copy link
Member

Hi @michalvavrik I've left another 3 comments - I know it can be time consuming. This is how I work with OIDC issues myself unless they are really trivial, I go in multiple iterations to prepare a single PR, do it once, then it is always the case of being in real doubt if I did it right, I keep renaming things, etc. It is a very important piece of Quarkus security so we just need to be as precise as possible.
Thanks

@michalvavrik michalvavrik force-pushed the feature/oidc-service-app-user-info-token-verification branch from 48e0ea8 to cb095d4 Compare December 8, 2022 20:14
@michalvavrik
Copy link
Member Author

Np @sberyozkin , let's do as many iterations as necessary. I think PR looks better after changes and I appreciate your help.

@quarkus-bot
Copy link

quarkus-bot bot commented Dec 8, 2022

Failing Jobs - Building cb095d4

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
✔️ JVM Tests - JDK 17
JVM Tests - JDK 17 MacOS M1 Set up runner ⚠️ Check → Logs Raw logs
✔️ JVM Tests - JDK 18

Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

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

Thanks @michalvavrik, nice work.

I've been thinking, OK, what happens now if someone steals the token and posts it and UserInfo is returned and hence the access is allowed.... But then I've realized the same risk exists with any bearer token authentication, if the token is stolen.
This is why the use of HTTPS, MTLS is needed, other measures like limiting the number of times the token can be used, do not give it 1 month lifetime, etc that can help.
This also made me think we'd need to consider to have an optional request token cache, similarly to what we have with UserInfo/TokenIntrospection, where the users can enforce for example a single (limited - number of times, age) token use only - something that OIDC providers should also be doing - I'll open an enhancement request to discuss it further

@sberyozkin
Copy link
Member

And which is why we have made it a bit harder to configure in this particular case, not only require users to say the userinfo is needed but also to approve the indirect token verification with an extra flag

@sberyozkin
Copy link
Member

@pedroigor Hey Pedro, I'm intending to merge early next week, LGTM, but if you have any comments let us know please.
To summarize, we'd like to be able to handle GitHub/etc tokens as bearer tokens which can help with some tooling integration for ex, with GitHub providing only an indirect verification via its user info endpoint. Recall, we already handle such tokens basically the same way in OIDC code flows, where we verify the synthesized ID token indirectly by insisting on the UserInfo acquisition.

@sberyozkin
Copy link
Member

For the future work: #29777

@sberyozkin sberyozkin merged commit 7c6bb9b into quarkusio:main Dec 14, 2022
@quarkus-bot quarkus-bot bot added this to the 2.16 - main milestone Dec 14, 2022
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Dec 14, 2022
@michalvavrik michalvavrik deleted the feature/oidc-service-app-user-info-token-verification branch December 14, 2022 15:37
michalvavrik added a commit to michalvavrik/quarkus that referenced this pull request Dec 16, 2022
follow up to quarkusio#29715

I think previous condition was typo and it worked as by default `allow-opaque-token-introspection` is set to true (which is going to change in the future). We should only allow empty introspection result if that's a way how user info verification says "verification has been successful".
ebullient pushed a commit to maxandersen/quarkus that referenced this pull request Jan 24, 2023
follow up to quarkusio#29715

I think previous condition was typo and it worked as by default `allow-opaque-token-introspection` is set to true (which is going to change in the future). We should only allow empty introspection result if that's a way how user info verification says "verification has been successful".
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 service applications should be able to accept GitHub tokens
2 participants