aspnet / AspNetKatana

Microsoft's OWIN implementation, the Katana project
Apache License 2.0
963 stars 332 forks source link

SameSite=None is always set on OpenIdConnect nonce cookie regardless if request is insecure #386

Closed lyubomirr closed 9 months ago

lyubomirr commented 3 years ago

Recently, I've upgraded the Microosft.Owin.Security.OpenIdConnect package in order to accomodate the new samesite changes. The problem I have is that the nonce cookie SameSite mode is always set to None, even on http. This makes the browser ignore the cookie. Can you elaborate why the implementation is like that? Is it possible for insecure requests to set the SameSite mode Lax for example, or export an option in the OpenIdConnectAuthenticationOptions to choose the mode, or maybe even a delegate which dynamically choses your SameSite mode? Im open to contribute if needed.

Tratcher commented 3 years ago

SameSite=None is required for almost all OIDC flows, which means https & secure are now required as well. An OIDC sign-in should have been using HTTPS in the past to prevent cookie highjacking, but now the browsers are forcing you to use https.

lyubomirr commented 3 years ago

In the particular use case i'm using the implicit flow and I wanted to be able to use it also under http, because you can use the system in your own server in your own local network and didn't want to force https. But if its not possible, then I will try just to override it myself.

psteniusubi commented 3 years ago

I run into this issue when hosting my oidc client web app on localhost using plain http not https. I guess this use case is quite common while developing a web app and launching the app directly from visual studio.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/SameSite

None requires the Secure attribute in latest browser versions

OpenIdConnectAuthenticationHandler already checks for Request.IsSecure when setting the Secure flag. The handler should also check Request.IsSecure when setting the SameSite=None flag.

I'd like to suggest a simple fix. Replace

SameSite = SameSiteMode.None

with

SameSite = Request.IsSecure ? SameSiteMode.None : default

In the following two locations

https://github.com/aspnet/AspNetKatana/blob/7aa465d56878b43409a603cf5384e7614cc35e19/src/Microsoft.Owin.Security.OpenIdConnect/OpenidConnectAuthenticationHandler.cs#L640

https://github.com/aspnet/AspNetKatana/blob/7aa465d56878b43409a603cf5384e7614cc35e19/src/Microsoft.Owin.Security.OpenIdConnect/OpenidConnectAuthenticationHandler.cs#L671

lyubomirr commented 3 years ago

@psteniusubi That's my suggestion too.

Tratcher commented 3 years ago

@psteniusubi SameSite=None is required for the default OIDC flows. Disabling it isn't going to work. For local testing you either need to use https or to disable the same site restrictions in Chrome via chrome://flags/.

Local https testing is quite easy in IIS Express.

psteniusubi commented 3 years ago

There are valid use cases that do work with the default SameSite=Lax policy for the nonce cookie.

For example OIDC authorization code flow using top-level redirects and http get method will work with SameSite=Lax policy for all cookies.

SameSite=Lax policy is not sufficient when there is an embedded resource such as iframe or a top-level redirect with http post method.

All of this is only partially relevant. Currently the browser rejects the nonce cookie because the combination of cookie flags are invalid when web app is hosted on http and not https. This invalid combination of flags is the error I'm asking to fix.

Tratcher commented 3 years ago

Sorry, I should have prefaced this discussion with a few points:

Your best option at the moment is to customize the behavior through the OpenIdConnectAuthenticationOptions.CookieManager extensibility hook, that lets you adjust cookie parameters before they're written out.

psteniusubi commented 3 years ago

Ok, thank you. That's good to know.

nbevans commented 9 months ago

Have you read about the CHIPS (partitioned cookies) stuff that is coming in Chrome early 2024? I think this is going to break SameSite=None again.

Tratcher commented 9 months ago

Comments on closed issues are not tracked, please open a new issue with the details for your scenario.