quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.73k stars 2.67k forks source link

OIDC: Constant 401 instead of new challenge when token expired #33200

Closed Felk closed 1 year ago

Felk commented 1 year ago

Describe the bug

Hello there, I have an application that is secured with a separate keycloak instance and a quarkus OIDC config that looks something like this:

quarkus.oidc.auth-server-url=https://mykeycloak.example.com/realms/myrealm
quarkus.oidc.client-id=my-client-id
quarkus.oidc.credentials.secret=12345678-abcd-dcba-4321-87654321
quarkus.oidc.application-type=HYBRID
quarkus.oidc.token-state-manager.split-tokens=true
# quarkus uses http if accessed through a reverse proxy, because it detects being accessed
# through http (by the reverse proxy), but we want the redirect uri to be https.
%prod.quarkus.oidc.authentication.force-redirect-https-scheme=true

We have a Single Page Application and want to implement the following use case:

the problem

Starting with Quarkus 3.0, this is where some problems arise. This is the problematic behaviour I am experiencing:

workaround

Set quarkus.oidc.authentication.redirect-path to some dedicated path that is used by successful oidc redirects only. The path must then not be navigated to by any other means other than a successful redirect from the OIDC provider, because it will result in a 401 error.

In this configuration, setting quarkus.oidc.authentication.restore-path-after-redirect to true is probably also desirable.

expected behaviour

I did not expect my application to be stuck in 401 errors. There are probably many ways to fix this. If I had to spitball:

/cc @sberyozkin because you implemented https://github.com/quarkusio/quarkus/pull/31079, which introduced the State parameter can not be empty or multi-valued if the state cookie is present error I see in the debug logs.

Thanks in advance for taking a look! And sorry if I misunderstood the way OIDC works and this is just a layer 8 problem.

Quarkus version or git rev

3.0.2.Final

quarkus-bot[bot] commented 1 year ago

/cc @pedroigor (oidc), @sberyozkin (oidc)

sberyozkin commented 1 year ago

Hi @Felk

What I'm not sure about yet is why we have a case here where a state cookie is available but no state query parameter is present ?

Set quarkus.oidc.authentication.redirect-path to some dedicated path that is used by successful oidc redirects only.

This is a typical setup I guess.

The path must then not be navigated to by any other means other than a successful redirect from the OIDC provider, because it will result in a 401 error.

I don't think so, if your first request goes there, you will be redirected

sberyozkin commented 1 year ago

@Felk 401 is returned if the state cookie is available on the configured (default) redirect path - but no state parameter is available. So this is correct I think...

sberyozkin commented 1 year ago

@Felk We'll tune it for you if that would be possible, np, I just need to have a clearer picture of this case

Felk commented 1 year ago

Hey, @sberyozkin thanks for the quick response!

The path must then not be navigated to by any other means other than a successful redirect from the OIDC provider, because it will result in a 401 error.

I don't think so, if your first request goes there, you will be redirected

Yes, that's what I'm experiencing as well in the "happy" case. Unfortunately because I'm dealing with a SPA the first request comes from some fetch call from the frontend. The challenge does not get completed there. Now because I did not configure a dedicated redirect URI before, trying to open a popup to trigger a second challenge where the user can actually enter some credentials fails, because each following request will 401 until the state cookie gets deleted. I haven't found a way to "repair" the session after this has happend besides deleting the cookie and refreshing the page so I get a fresh challenge.

What I'm not sure about yet is why we have a case here where a state cookie is available but no state query parameter is present ?

The state cookie is just set for the whole domain it looks like.

401 is returned if the state cookie is available on the configured (default) redirect path - but no state parameter is available. So this is correct I think...

I agree. It sounds correct in principle. But in practice it's a bit wonky since I believe it's possible that this check also triggers on pages that are not exclusively an oauth redirect page. If no explicit redirect is configured, every page appears to be stuck on 401s.

sberyozkin commented 1 year ago

@Felk np, OK I see, I got it, with your last comment.

If no explicit redirect is configured, every page appears to be stuck on 401s.

Right - this is a fairly niche case, where the redirect URI is the same as the current request URI, unless you have the OIDC provider itself configured with a wildcard redirect, it won't really work anywhere. It is useful for testing, Dev Services work, but not really in production, from the security point's of view, the workaround you mentioned would be in fact the best solution where you designate some specific virtual redirect path and then restore the original request path if it is what is needed in your case.

From one of your proposals:

Force users to configure a quarkus.oidc.authentication.redirect-path so only that path 401s by default, not the entire application

This is pretty much it. The reason the entire app space is giving you 401 is that you have configured your OIDC provider to allow any redirect URIs, right ?

and another proposal:

Always automatically start new challenges instead of aborting with 401 errors when the application is of type web-app or hybrid

It is simplest but if the state parameter is missing on the redirect path, something strange is going on from an OIDC perspective, the state parameter must be returned...

I wonder, if we can just avoid generating unique state cookie name if allowMultipleAuthorizationCodeFlows=false, will that work for you if you do prefer to stay with the widcard redirect setup (internal network etc) ? The only reason the state cookie name is unique now is for the multi-tab authentication to work, but if it is explicitly disabled, we don't have to gen the unique names

sberyozkin commented 1 year ago

@Felk I've reread this issue's description. It does feel to me that for your case, configuring a specific redirect URI is the right thing to do. It will work perfectly for all the providers you may want to integrate with - without configuring it it may only work with providers which will allow you to set up a wildcard redirect - and such support can be revoked by these providers at any time really. I believe it does not work with Keycloak out of the box now either.

You won't get 401 when you point to such a path during the initial request and a pop-up window can continue triggering the authentication by accessing some other static resource for ex.

If you do not configure the redirect path, then the redirect path is equal to the current request URI, so this is why you are seeing 401 when the pop-up window opens a request. The check which causes 401 was not in Quarkus 2.x - we did not support the multi-tab authentication back then. The interesting thing is that we actually do it now, your case is a case of the multi-tab authentication. The only difference - you have a wildcard redirect URI support.

The wildcard URI support is handy for devmode and devservices, example, for the OIDC web-app, I do mvn quarkus:dev, point the browser to localhost:8080/testendpoint, I get redirected to Keycloak (launched internally and set up to support a wildcard redirect), authenticate, and get returned to localhost:8080/testendpoint, zero config.

In production, it is not recommended. The problem is, if we fail at the startup, it will upset users doing demos. And it can't work anyway against any provider unless users go to Keycloak or other provider and enable a wildcard redirect in the admin dashboard there.

Given all of it, I think we can close this issue. Here we essentially have a multi-tab authentication in action, with the configuration to be set as per your described workaround.

Let me know what you think please Thanks

Felk commented 1 year ago

Hey @sberyozkin, thanks for your detailed analysis! I agree with your verdict that configuring a dedicated redirect URI is not only a workaround but the proper solution for my case.

The problem is, if we fail at the startup, it will upset users doing demos.

Yeah you are right, maintaining the zero-config usecase is more important. I did not think about that.

I think we can close this issue

Yep, I think so too. Thanks you for your time!

sberyozkin commented 1 year ago

Thanks @Felk