Allow to control Same-Site attribute for OIDC state cookie

[sberyozkin]

Fixes #52607

Currently only the session cookie can have its same-site attribute controlled, the session cookie has its lax by default.
As per the links from the issue, originally, I started with both the session and state cookies sharing the same Same-site values that broke several applications once it is set to Strict, when the session-cookie: strict, state-cookie: lax typically works.

Other than that, in #52607, we found a workaround for the authentication to work by controlling the state cookie same site status at the Vert.x HTTP level - but it does require disabling the mullti-tab auth. Additionally the reporter discovered that for same site none cookies this attribute has to be set when the cookie is removed

So having a dedicated property to control it for the state cookie works best.

[github-actions]

🙈 The PR is closed and the preview is expired.

[michalvavrik]

I'd be much more comfortable to make a breaking change and make strict default. I don't believe every user reads all our config properties and documentation (I never did, though I am not user per say). We would add it to the migration guide and users are supposed to read migration guides, so it would be fine.

But as far as I am concerned, this change is positive.

[sberyozkin]

@michalvavrik hi Michal, as I said in the description and as shown in the linked old issue, state cookie in a strict mode broke applications.
This is how I started but with the state cookie associated with redirects, it is something that users have to evaluate and set. We recommend to do it in the oidc expanded docs

[michalvavrik]

@michalvavrik hi Michal, as I said in the description and as shown in the linked old issue, state cookie in a strict mode broke applications.

That is fine, because it only broke "some" applications and if users set it to lax from strict, that is conscious decision. Breaking changes happen and this one would be for good, if we say we are "secure first".

This is how I started but with the state cookie associated with redirects, it is something that users have to evaluate and set. We recommend to do it in the oidc expanded docs

Here, we are already going in a circle, I explained my opinion on that in my initial comment, so I suggest to just leave it as you introduced here as lax and I'll keep my opinion.

[sberyozkin]

@michalvavrik

you introduced here as lax and I'll keep my opinion.

It is already lax by default on main. Of course, but we know the users had the applications broken, so I'm not sure what your point is to be honest

[sberyozkin]

we say we are "secure first".

This is a fair point, but this is certainly a bigger concern than the same site attribute. And again, it is not about users making a decision in those linked cases whether to stay on lax or strict for the state cookie - the application just does not work with state cookie in a strict same site mode

[sberyozkin]

@michalvavrik Hey, so first of all, I cleaned up the removeCookie, at the moment it just checks if the cookie name has a session or state cookie root, for now it is probably simplest. Let me know what you think.

Your point about the security first is worth discussing further in the bigger context, I have probably a 5 year old issue to enable the authentication by default when the security capability is detected. I guess the key point is that users have a choice, toggle it off or keep it on, but in the case I linked to - they don't and that changes the picture completely. Anyway, I propose we discuss it further without delaying it for too long.

[sberyozkin]

@qkxy, hi, is there a chance you can test this PR and verify both the state and session cookies with the same site=none are removed by the browser ?

Thanks

[qkxy]

@qkxy, hi, is there a chance you can test this PR and verify both the state and session cookies with the same site=none are removed by the browser ?

Sure, I will get back to you with the result.

[michalvavrik]

It is already lax by default on main.

Hmm, I have tried to find out just from reading code, so it is my bad I didn't mention it is actually set somewhere. I thought it was simply unset, which is not the same as when it is set explicitly (AKA "default lax" is not the same as "lax" explicitly set and firefox behaves differently depending on the version). I think we don't understand each other though - I think this change is fine. I just questioned if we can do better (strict).

Your point about the security first is worth discussing further in the bigger context, I have probably a 5 year old issue to enable the authentication by default when the security capability is detected. I guess the key point is that users have a choice, toggle it off or keep it on, but in the case I linked to - they don't and that changes the picture completely. Anyway, I propose we discuss it further without delaying it for too long.

I know about that issue. That and you saying "the security first" from time to time (in other words) made me to raise this. Yeah, I like the idea of secure profile that will harden defaults. We should go back to it.

[michalvavrik]

LGTM, thanks

[qkxy]

@qkxy, hi, is there a chance you can test this PR and verify both the state and session cookies with the same site=none are removed by the browser ?

Sure, I will get back to you with the result.

I built my app using this PR version of Quarkus, and the q_auth cookie is created and removed with SameSite=None and the browser didn’t block it.

[sberyozkin]

Thanks @qkxy

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit d883340.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

[sberyozkin]

@michalvavrik Just FYI, GitHub OAuth2 sets Same-site lax too by default

[michalvavrik]

@michalvavrik Just FYI, GitHub OAuth2 sets Same-site lax too by default

Ok