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

Docs: OpenID Connect authorization code flow mechanism #37422

Closed
fedinskiy opened this issue Nov 30, 2023 · 8 comments · Fixed by #37689
Closed

Docs: OpenID Connect authorization code flow mechanism #37422

fedinskiy opened this issue Nov 30, 2023 · 8 comments · Fixed by #37689
Assignees
Labels
Milestone

Comments

@fedinskiy
Copy link
Contributor

Describe the bug

I identified several issues in OpenID Connect authorization code flow mechanism for protecting web applications guide[1]

  1. The guide advises[2] to use property quarkus.oidc.authentication.error-path=/error for failures during user identification. When I use recommended solution (keycloak), I am not redirected back to the app on failure, but just get a message Invalid username or password.

  2. An example in the token section[3] contains method getReservationfromRemoteEndpoint without any description. If it is supposed to be user-defined method, then a) it should be mentioned b) an implementation example would be helpful, given that the method should retrieve data from a token, which looks like a long line of random ASCII.

  3. If user info[4] is requested (via quarkus.oidc.authentication.user-info-required=true) this is often (even for a very small payloads[5]) leads to a very big cookies, which break some browsers (eg Firefox). I suppose, we should advise to add quarkus.oidc.token-state-manager.split-tokens=true for this case. That option is described in error messages in devmode and in another section[6], but we need at least to mention it.

  4. I am not sure, if this is our problem, or Keycloak's, but method io.quarkus.oidc.UserInfo.getFirstName() returns null for user info, retireved from keycloak[5]. In the UI of Keycloak(version 22.0.1) the field is called "First Name", so that is unexpected.

  5. Token state manager[6] section recommends to use value id-refresh-token for quarkus.oidc.token-state-manager.strategy property. When I put this in application.properties, I get an exception: Cannot convert id-refresh-token to enum class io.quarkus.oidc.OidcTenantConfig$TokenStateManager$Strategy. Options id-token and keep-all-tokens doesn't have this problem.

[1] https://quarkus.io/version/main/guides/security-oidc-code-flow-authentication
[2] https://quarkus.io/version/main/guides/security-oidc-code-flow-authentication#customizing-the-authentication-error-response
[3] https://quarkus.io/version/main/guides/security-oidc-code-flow-authentication#access_id_and_access_tokens
[4] https://quarkus.io/version/main/guides/security-oidc-code-flow-authentication#user-info
[5] {"sub":"eb4123a3-b722-4798-9af5-8957f823657a","email_verified":true,"name":"Alice Longbottom","preferred_username":"alice","given_name":"Alice","family_name":"Longbottom","email":"[email protected]"}
[6] https://quarkus.io/version/main/guides/security-oidc-code-flow-authentication#token-state-manager

Expected behavior

No response

Actual behavior

No response

How to Reproduce?

No response

Output of uname -a or ver

No response

Output of java -version

No response

Quarkus version or git rev

No response

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

@fedinskiy fedinskiy added the kind/bug Something isn't working label Nov 30, 2023
@fedinskiy
Copy link
Contributor Author

area/documentation , area/oidc

Copy link

quarkus-bot bot commented Nov 30, 2023

/cc @MichalMaler (documentation), @ebullient (documentation), @inoxx03 (documentation), @michelle-purcell (documentation), @pedroigor (bearer-token,oidc), @rolfedh (documentation), @sberyozkin (bearer-token,oidc), @sheilamjones (documentation), @sunayna15 (documentation)

@sberyozkin
Copy link
Member

@fedinskiy Thanks for going through this doc,

An example in the token section[3] contains method getReservationfromRemoteEndpoint without any description. If it is supposed to be user-defined method, then a) it should be mentioned b) an implementation example would be helpful, given that the method should retrieve data from a token, which looks like a long line of random ASCII.

I can add some comments/clarifications, but we can't have some implementation there as it would imply some token propagation etc.

I am not sure, if this is our problem, or Keycloak's, but method io.quarkus.oidc.UserInfo.getFirstName() returns null for user info, retireved from keycloak[5]. In the UI of Keycloak(version 22.0.1) the field is called "First Name", so that is unexpected.

UserInfo.getFirstName() is not tied specifically to Keycloak but to a standard first_name claim in the token, AFAIK, KC does not include it, at least by default, but with Keycloak UserInfo.getPreferredName() would always work

If user info[4] is requested (via quarkus.oidc.authentication.user-info-required=true) this is often (even for a very small payloads[5]) leads to a very big cookies, which break some browsers (eg Firefox). I suppose, we should advise to add quarkus.oidc.token-state-manager.split-tokens=true for this case.

Minor clarification, it does not break the browser :-), but it will just cause re-authentication. I think there is a guideline somewhere to split in such case, but I repeat it here. It is probably the right time to also optimize the way session cookie is encrypted to save on the overall cookie size

Cannot convert id-refresh-token

Yes, a small doc typo, good catch:
https://github.com/quarkusio/quarkus/blob/main/integration-tests/oidc-code-flow/src/main/resources/application.properties#L149

@fedinskiy
Copy link
Contributor Author

@sberyozkin regarding "first name" claim in Keycloak: keycloak UI calls this field "First name", standard claim is called "first_name", but keycloak token contains it as given_name. Sounds like very weird behavior to me, should we create an issue for Keycloak?

@fedinskiy
Copy link
Contributor Author

A couple more things:

  1. Section mapping token claims[1] mentions microprofile_jwt scope, while keycloak documentation[2] calls it microprofile-jwt (with dash, not underscore)
  2. Example on "how to set up keycloak and Quarkus, so user admin will have roles admin and user, but user alice will have only user " would be highly appreciated, either in this guide, or in tutorial[3]. Separate examples on getting groups from Keycloak, storing groups locally for known Keycloak users and mapping Keycloak groups to app groups will be even better.

[1] https://quarkus.io/guides/security-oidc-code-flow-authentication#token-claims-roles
[2] https://www.keycloak.org/docs/latest/server_admin/#protocol
[3] https://quarkus.io/version/main/guides/security-oidc-code-flow-authentication-tutorial

@sberyozkin sberyozkin self-assigned this Dec 12, 2023
@sberyozkin
Copy link
Member

@fedinskiy

regarding "first name" claim in Keycloak: keycloak UI calls this field "First name", standard claim is called "first_name", but keycloak token contains it as given_name. Sounds like very weird behavior to me, should we create an issue for Keycloak?

Sure, may be as an enhancement request, such that users can choose which claim to use in the Keycloak dashboard, they won't be able just to change given_name to first_name to avoid breaking the user code

@sberyozkin
Copy link
Member

sberyozkin commented Dec 12, 2023

@fedinskiy

Example on "how to set up keycloak and Quarkus, so user admin will have roles admin and user, but user alice will have only user " would be highly appreciated, either in this guide, or in tutorial[3]. Separate examples on getting groups from Keycloak, storing groups locally for known Keycloak users and mapping Keycloak groups to app groups will be even better.

I don't mind giving more information how to configure things in Keycloak, but it can't be done in isolation, just about alice and bob, the whole process of creating clients, realms, and many other properties will then have to be described, essentially we'd have to get to something what is provided for Auth0: https://quarkus.io/guides/security-oidc-auth0-tutorial, where a step by step guide how to do this and that in the Auth0 dashboard is provided, etc. It definitely should be done for Keycloak too, but I'd rather have a dedicated more advanced tutorial, or perhaps, indeed the existing tutorial can be reworked, but please open a dedicated enhancement request, the concept doc the issue is addressing has already been reviewed by writers, so we just need to fix the issues you have reported here, without trying to significantly rewrite it as it would require another round of the Doc team reviewes, etc

@sberyozkin
Copy link
Member

sberyozkin commented Dec 12, 2023

@fedinskiy

The guide advises[2] to use property quarkus.oidc.authentication.error-path=/error for failures during user identification. When I use recommended solution (keycloak), I am not redirected back to the app on failure, but just get a message Invalid username or password.

I've tried to clarify that it may happen when for example, an invalid scope is set in the redirect URI, there is a test for this case in Quarkus. I suppose, as far as invalid user authentication is concerned, may be Keycloak will redirect back with an error after 3 attempts or may not, while other providers mat do it immediately, but in any case, I've tried to shift focus for the failed authentication

An example in the token section[3] contains method getReservationfromRemoteEndpoint without any description. If it is supposed to be user-defined method, then a) it should be mentioned b) an implementation example would be helpful, given that the method should retrieve data from a token, which looks like a long line of random ASCII.

This is just some example user function which passes the access token somewhere else, it is not meant to show how to parse the token, the access token can be binary. I added a comment there suggesting passing the token as a RestClient Authorization Bearer scheme value.

If user info[4] is requested (via quarkus.oidc.authentication.user-info-required=true) this is often (even for a very small payloads[5]) leads to a very big cookies, which break some browsers (eg Firefox).

How did you reproduce it ? I've just tried security-openid-connect-web-authentication-quickstart and I don't see any warnings being logged where it says the session cookie is too large

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

Successfully merging a pull request may close this issue.

3 participants