-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Recurring redirects in OIDC against Azure AD #20363
Comments
/cc @pedroigor, @sberyozkin |
Hi @artkonr Hmm... A few Azure users have confirmed it was working for them and I thought we have fixed all the redirect loops problems. The last message explains the problem, the state cookie set during the initial redirect is not visible when the user is redirected back to Quarkus so it is impossible to confirm Quarkus initiated it - I'm surprised it is happening since the cookie path is set to |
Hi @sberyozkin, Thanks for the info. Odd bit here is that in my case, the setup is probably the simplest one, no overlapping roots and the app itself runs on localhost only. Besides, I got a feeling that this last statement in the log is actually an evidence of Chrome breaking the redirect loop, as if denying the state cookie is part of the recursion control. I also tried setting You mentioned other users reporting similar behavior, could you share the issue details, please? Maybe there could be something I could try as well. Interesting though, I tried with another tenant on Azure and the behavior was slightly different: the redirects went on until Azure returned "error signing in" page. |
@artkonr Have a look please at https://github.com/quarkusio/quarkus/issues?q=is%3Aissue+is%3Aclosed+azure, a few of them are there.
Unlikely IMHO, the browser is redirecting the user back to Quarkus which fails the authentication - the problem the exception it throws is not recognized as the authentication completion failure but a failure requiring a challenge so Quarkus again sends a redirect to the OIDC provider for the user to authenticate - Azure has already authenticated this user, redirects back to Quarkus, etc. I'll look at fixing the problem with the repeated redirect attempts on the Quarkus end - but the underlying problem is about the cookie not being available. Can you give me a favor please and create a test user/account for me to try ? You can ping me privately at https://quarkusio.zulipchat.com/ with the test account connection details or please type them here if it would be a very short term test account. |
@sberyozkin, gotcha, just pinged you in Zulip. |
OK, I have it.
Theoretically, splitting ID token in parts and saving in separate cookies could solve the issue for the cases when the token is too big. This should IMHO be considered a valid case, because JWT can contain arbitrary amount of data and may vary between tenants even for one IDP. For the record, OIDC web-app example is perfectly valid, because the realm settings for the Keycloak produce a token only ~2700 chars long. |
Hi @artkonr Nice work, thanks for this investigation. We have an option to customize the way the session data is represented, https://quarkus.io/guides/security-openid-connect-web-authentication#tokenstatemanager. In meantime we'll need to think about offering more options. One idea is to ship a database manager which will store the tokens in a database, but it should not be a default. @pedroigor has proposed an option the other day, which, as far as I recall, was about keeping the few most relevant ID token claims only, I'll explore it Thanks |
@artkonr By the way, I may've misread your conclusion. It looks like you refer to the I think that if you don't expect to propagate the access token to the downstream service or use to request Can you try it please ? I'm nearly sure now it will fix it for you. |
Oh, sure, I'll give it a shot, haven't thought of that, thank you! |
@sberyozkin, I tried a different token management strategy as you proposed, worked like a charm that, thank you very much! I guess I just didn't see the token value delimiter in the cookie value, did not connect it to the code flow I saw. I'm curious, though, if this has any performance implications, would not this force the app to always re-request the access token? |
@artkonr Thanks for verifying the alt strategy works.
No, in the code flow ID token is a primary one, the access token is not used by So unless 1)-3) requirements have to be met it is not needed and avoiding storing it by default would even make things a bit faster (fewer bytes to carry in the cookie value :-) ). But there will be cases where 1)-3) have to be met and the default cookie value is > 4096. What I'll do I'll resolve this issue with a PR which will add a warning when the cookie is > 4096 so it can be much easier to trace it :-), and will open a new enhancement request to optimize ID tokens |
Oh, OK, it's good that you mentioned the downstream service case, which is effectively the case I will be pursuing: I need Azure AD access to query MS Graph API, which is authenticated with an access token, so it looks like changing of the strategy won't cut it. Guess I'll have to look into overriding I guess a warning would indeed be a good thing to have at least for the time being. Thank you for your assistance! UPD: actually I feel a bit stupid, because the case of having a large cookie is mentioned on the OIDC doc page :D |
@artkonr I see, you can use https://quarkus.io/guides/security-openid-connect-client#token-propagation for propagating the access token acquired via the code flow. Oh, I forgot, setting only So I won't even have to try to optimize the token - as it won't work if the logout is requested and an id token hint has to be provided or if the endpoint code needs to access the claims which were deemed redundant - can be brittle. Please let me know if |
@sberyozkin , I tried it and it worked immediately, I needed to tweak the claim path to make the entire thing work, do I'm all set now. Thank you so much for the help! |
@artkonr Np at all, glad it worked. Thanks for highlighting how hard it can be to trace the problem related to the cookie size, at least one user did investigate it before, which is why a token state manager was introduced but I did not guess back then to add a warning along the way... |
Describe the bug
Hey team,
I am trying to enable OIDC security for my web app (
authorization_code
flow) with Azure AD being the identity provider. The problem is that endpoint declaration like below fails with recurring redirects:The error flow goes as follows:
401
.The logs aren't too helpful and look like this:
It seems to me like something goes wrong with how
redirect_uri
parameter is passed and treated by Azure. The value of the parameter seems to be valid and does not change between redirects, though.Expected behavior
Once I log in on the IDP page, the browser redirects me to a URI set in the
redirect_uri
parameter, which should match the endpoint I hit.Actual behavior
Once I log in on the IDP page I get redirected to the endpoint I hit.
How to Reproduce?
To reproduce:
1. Create a simple quarkus app with Resteasy and OIDC
The only thing that matters is that the app's controller class should have an RBAC-protected endpoint.
The contents of
application.properties
:2. Create an App in Azure, create the roles and permissions.
In Azure Portal:
CLIENT_ID
, "Directory ID" toTENANT_ID
placeholders inapplication.properties
above.CLIENT_SECRET
placeholder.3. Enter the name of the created role into
@RolesAllowed
on the protected endpoint.4. Hit the endpoint.
Output of
uname -a
orver
Darwin C8049 20.5.0 Darwin Kernel Version 20.5.0: Sat May 8 05:10:33 PDT 2021; root:xnu-7195.121.3~9/RELEASE_X86_64 x86_64
Output of
java -version
openjdk version "16.0.1" 2021-04-20 OpenJDK Runtime Environment (build 16.0.1+9-24) OpenJDK 64-Bit Server VM (build 16.0.1+9-24, mixed mode, sharing)
GraalVM version (if different from Java)
not used
Quarkus version or git rev
2.2.3.Final
Build tool (ie. output of
mvnw --version
orgradlew --version
)Apache Maven 3.8.1 (05c21c65bdfed0f71a2f2ada8b84da59348c4c5d)
Additional information
No response
The text was updated successfully, but these errors were encountered: