Process OIDC code query param only if the state cookie exists

[sberyozkin]

Fixes #22195 (and next it will be straightforward to resolve both #22189 and #22029).

So Steph reported the issue with the custom code parameter triggering the code flow completion process and he was not the first one, I looked at the earlier issue but decided to avoid fixing it back then because of the ambiguity - what to do if the state cookie is missing but the code is not - is it a faulty redirect or just an initial custom request.

However, after seeing #22195, and then also #22189, I thought it was not cool to ask users to replace the custom code and also error query parameters. And especially after seeing #22029, where the post request parameters should not be even checked if it is not an OIDC redirect, I thought it was the right time to address the problem.

So here is what this PR does:

• Makes sure the code is checked only if the state cookie exists and valid, confirming it came from the OIDC provider. it really makes sense to make it conditional on the presence of this cookie before deciding to start processing this code (and as I said, the same would apply to the error and also the form_post response mode processing)
• Right now on main, if no state cookie is found but code is then 401 is returned to prevent the redirect loops (i.e, the user successfully logged in, redirect to Quarkus, the state cookie is lost due to the wrong configuration - cookie domain or path is not set correctly, without 401 but with a 302 challenge the user goes back to the provider which already has the user session, send the user back to Quarkus, and the loop starts). I had to introduce a simple property to retain this behavior of reporting 401 if no state cookie is available but code is.
  ** Steph - I did look at your proposal of checking Referer which is a good idea and I started coding - but unfortunately it does not work in all the cases , it is not guaranteed to match the actual authorization provider URL with the various port, http vs https translations, I just did not feel it would be a reliable solution. I also think that controlling it with the property is more flexible as some users will want 401, some users - the redirect again (see below why)
• So if one needs to support custom code (or error) query parameters during the initial auth request then the only thing they should do is to set quarkus.oidc.authentication.redirect-without-state-cookie=true. In this case, if the state cookie is absent, the usual challenge with a redirect to OIDC will occur. The important thing is, it does not compromise on anything really, the only reason 401 is returned by default when no state cookie is present but the code is is to avoid a redirect loop in case of the bad configuration. So enabling this property will only have an extra requirement to make sure the configuration is correct to avoid losing the state cookie - and even it is not correct - what will happen is that if the state cookie is missing then instead of 401 the browser will fail with the redirect loop - unless the loop is prevented with requiring the provider to always challenge even the already authenticated users.

IMHO this is the best compromise we can do to let users supply custom code and soon error query parameters - but also by making the code processing conditional on the availability of the state cookie is a better approach in any case

[sberyozkin]

@FroMage @pedroigor After a walk I decided I was may be exaggerating the problem related to what to return in case of a missing state cookie but with the code missing.

I think the Steph's idea of checking the referer is in fact the simplest as opposed to adding a property. If it ever happens that the referer does not match the discover authorization uri then the worst what will happen is that the challenge with a redirect will start again as opposed to 401, and the main point here is that this can only happen in a faulty configuration situation...

So, I'll update the PR a bit - and the important part is - the current Quarkus OIDC behavior won't change a bit but will only become better prepared for dealing with the next related issues :-). And the property option will still be open...

Note I've updated some of the existing tests for them to pass, these are the cases where the test disables the auto redirect from the quarkus challenge, checks something and then calls Quarkus with the auto-redirect re-enabled - here the state cookie returned with the 1st call persists between these 2 calls which is causing a failure - so I just clean the cookies in between these 2 calls

[sberyozkin]

@FroMage @pedroigor

This is now better IMHO, in fact I actually start seeing easily now what is going in CodeAuthenticationMechanism :-)

It is all easy to understand now

1. if the session token is the - reauthenticate
2. if the state cookie is there - complete the exchange and start a new session
3. if neither cookie is there - return a challenge
4. in the challenge - if it is detected it is a redirect from Keycloak/etc - 401 (as per the current expectations) - else - redirect to the provider. Note I had to do this detection in the challenge method because it is there where the OIDC provider metadata are visible for us to compare it against the Referer

I've also fixed one more incorrect test which was exposed by the refactoring - the idea there was to simulate the code exchange failure by returning wrong redirect uri on the return leg - by returning different configurations - but this test was in fact failing much earlier - because by returning different tenant ids every time the state cookie could not be in fact detected, so I cleaned the corresponding setup...

So IMHO it is looking better now

[pedroigor]

I'm also not 100% sure how reliable is this. Is it really needed?

[sberyozkin]

@pedroigor Good point, I've been contemplating above about it, it is the best effort at trying to figure out how to fail the request best if the state cookie is missing.

On the main it is an immediate 401. But on the main it is also the problem that if the code is detected then it is assumed it is the redirect from KC - and only then then the state cookie is checked, but with the error and the Apple's form_post response mode to be supported next it just does not work - especially with form_post - we can't start checking the post data without asserting the state cookie exists, so this refactoring is making these checks dependent on the availability of the state cookie.

So now, if the state cookie is missing and 1) If no code is there - we fail indirectly by challenging the user with a redirect to KC - this is how it works on main 2) if code is there - we check Referer and if proved it comes from KC only then do we stop everything with 401 - this is new to this PR, instead of just failing with 401

So if the Referer check fails then instead of 401 the user will be redirected to KC to re-authenticate. And as I said above, we can follow up with adding the property configuring what to do if the state cookie is missing and the code (error) (query or form post) present - do 401 or redirect to KC. In fact I started with the property first but then thought it was unnecessary yet.

Does it clarify it ?

We have a test expecting 401 in case of a missing state cookie and this code properly gives 401 as expected.

[pedroigor]

I see. I'm still trying to figure out if this has some security implication because the header can be easily forged into the request. But don't think there is and it should improve UX.

Makes sense?

[sberyozkin]

@pedroigor Hi Pedro,

I'm still trying to figure out if this has some security implication because the header can be easily forged into the request. But don't think there is and it should improve UX. Makes sense?

I'm not sure I follow, can you clarify please ?

This header is only checked in the getChallenge method. We don't do any positive decision based on its match - we only check it to decide how to fail if state cookie is not available but 'code' is.

If it matches the Keycloak authorization URL then it is 401 (meaning the authentication has failed, no retries), if it does not match it - it is a new unauthenticated request, the user goes to Keycloak/etc. This referer won't be used to redirect the user.

We can totally drop this check, but that will mean it will always be only a redirect to Keycloak if the state cookie is absent but the code is there; the referer check gives us an option to react to this case in a more nuanced way.
It is really about this use case, how to fail when the code is available but the state cookie is not.

I'm not sure, may be I will just drop this Referer check. We have a test where a state cookie is missing but the code is, and 401 is expected. Without this check I'll have to tweak the test to expect 302 to Keycloak in this case though.

[sberyozkin]

@FroMage It will work fine, I promise :-).

I'll need to wait for Pedro @pedroigor to finalize if he is OK with the best effort check for the referer

[sberyozkin]

@FroMage @pedroigor Yet again I've started experimenting with the idea of checking the redirect path (though only instead of checking the referer), when deciding if the challenge should end the flow with 401 (scenario: state cookie is lost, code is there), and stopped - because what prevents a user sending an initial unauthenticated request with the custom code to the URI which matches the one the OIDC Provider will also use to redirect the user to ? All in all, starting with checking the redirect path does not work.
I think it is totally safe just to check the referrer and fail with 401 in case of a match, to avoid the redirect loops when the state cookie was lost after the redirect.

[sberyozkin]

Hi Pedro @pedroigor - let me merge it now, I can't go without this PR as a rebase will be needed for the pkce draft, and other issues @FroMage has reported can't be fixed without it.

So the only thing the referrer check is done for is to try to respond with 401 if the state cookie is lost after the user has been redirected back to Quarkus - without this check, in such a case, it will be a redirect loop for the user and the browser failing with the error and trace which will be even worse than 401 error :-) - this is exactly why we have a test for this case - to prove that the redirect loop can be avoided. I agree this check is not guaranteed to produce a match - which is fine - as far as Quarkus is concerned - it will challenge the user to authenticate then. It is the rare/edge case related to the cookie path/domain misconfiguration which can loose a state cookie, we had a few such cases in the early days, before we ended up setting a default cookie path to /.