Skip to content
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

How to implement the double cookie policy for the SameSite policy changes #17518

Closed
drauch opened this issue Dec 2, 2019 · 25 comments
Closed
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer

Comments

@drauch
Copy link

drauch commented Dec 2, 2019

In contrast to your recommendation described here and here we want to go with Google's recommended solution: namely setting two cookies (if you want to know why, see details below).

Question: How would one implement the 2-cookie-solution using the Cookie and OpenIDConnect middlewares?
(we're using ASP.NET Core 2.1.x and .NET Framework 4.7.2)

Best regards,
D.R.

Details: major browser vendors and authentication providers (e.g., Google or Auth0) opt for the double cookie solution. Mostly because: your recommended solution of browser sniffing doesn't cover all the scenarios (see Auth0 blog article for more details on this).

@drauch drauch changed the title How to implement the double cookie policy for the SameSite changes using ASP.NET Core? How to implement the double cookie policy for the SameSite policy changes Dec 2, 2019
@javiercn javiercn added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Dec 2, 2019
@blowdart
Copy link
Contributor

blowdart commented Dec 2, 2019

You would fork the cookie middleware, and have it drop two cookies, then check on the return for the presence of one or both.

You may think that you could add two cookie middlewares, and set the Same Site setting to Lax on one and -1 on the other so the attribute isn't written, but you can still only have one middleware as the default, so that approach won't work.

@Tratcher
Copy link
Member

Tratcher commented Dec 2, 2019

Double issuing cookies is going to be at least as complicated as browser sniffing. @blowdart is right, you're going to have to fork every component that issues a cookie and customize it. You'll have to be extra careful around deleting cookies, ensuring you always delete both even if you only receive one.

@drauch
Copy link
Author

drauch commented Dec 2, 2019

@blowdart: CookieAuthenticationHandler seems to be a good boy and always uses Options.CookieManager to retrieve/append cookies. So it should be enough to set a different CookieManager which handles the duplication issue? Do you see problems with this approach?

@Tratcher: we are only talking about the authentication cookie, all other cookies can have the Lax or even Strict policy in our application.

@blowdart
Copy link
Contributor

blowdart commented Dec 2, 2019

We think it's possible, you'd just have to make decisions on which cookie wins. It's not something we've tried though.

@drauch
Copy link
Author

drauch commented Dec 2, 2019

OK, we'll give it a try and will report back. Thanks so far!

@Tratcher
Copy link
Member

Tratcher commented Dec 2, 2019

Note OpenIDConnect uses temporary nonce and correlation cookies that are highly impacted by the SameSite transition. These default to SameSite=None which will have issues on older browsers.

Two places you're going to struggle with for CookieAuth:

  • Size: Auth cookies are already fairly large, double issuing them is going to risk hitting browser or server size limits.
  • Delete: When cookies get large the ChunkingCookieManager splits them up into numbered chunks. Properly deleting them requires knowing how many chunks there are. If a request doesn't send you an auth cookie for SameSite reasons, you won't know how many chunks it has if you need to delete it.

@drauch
Copy link
Author

drauch commented Dec 2, 2019

Note OpenIDConnect uses temporary nonce and correlation cookies that are highly impacted by the SameSite transition. These default to SameSite=None which will have issues on older browsers.

You're right. Unfortunately the OpenIDConnect middleware does not use a cookie manager or some other replacable component to obtain/set cookies. Do you think it's possible to replace the Request.Cookies and Response.Cookies objects to implement that duplication behavior?

Size: [...]

We've not tested this, however, as Google is recommending to do it that way we don't think we run into much trouble due to size limits.

Delete: [...]

Is the ChunkingCookieManager magically in place somehow? If the OpenIDConnect middleware doesn't use a cookie manager at all, and the Cookies middleware uses our replaced cookie manager -> is there still cookie chunking going on in some way?

Best regards,
D.R.

@Tratcher
Copy link
Member

Tratcher commented Dec 2, 2019

Is the ChunkingCookieManager magically in place somehow?

It's the default. https://github.com/aspnet/AspNetCore/blob/629b444a91f7980f1d071164affceb7e1e16f321/src/Security/Authentication/Cookies/src/PostConfigureCookieAuthenticationOptions.cs#L43

If the OpenIDConnect middleware doesn't use a cookie manager at all, and the Cookies middleware uses our replaced cookie manager -> is there still cookie chunking going on in some way?

The OIDC temp cookies are small and don't need chunking. Only the final auth cookie is large and that's managed by CookieAuth.

We've not tested this, however, as Google is recommending to do it that way we don't think we run into much trouble due to size limits.

Take that guidance with a grain of salt, that came from the Chrome team, they didn't actually implement it that guidance anywhere. You're almost guaranteed to break Safari with large cookies.

Do you think it's possible to replace the Request.Cookies and Response.Cookies objects to implement that duplication behavior?

The CookiePolicyMiddleware middleware does this, but only for response cookies. CookiePolicyMiddleware can't do double cookies, but you could model a solution off that. Request cookies could be intercepted in a similar way via the IRequestCookiesFeature.

Again, you're not saving yourself any effort with this approach. If anything it will be harder and more error prone. Yes user agents are annoying to deal with, but that's a problem fairly easily addressed by monitoring your app telemetry. The community has already put together a good starting list.

@drauch
Copy link
Author

drauch commented Dec 2, 2019

First: thank you for the short reply times, really appreciate it!

Again, you're not saving yourself any effort with this approach.

We've realized that already, still, there is this problem with the user agent sniffing that it is brittle by design. What works today may (and will be!) broken tomorrow. We think it's better to invest some more upfront design/coding into the matter and have no problems with our customers later on in the wild.

The community has already put together a good starting list.

Can you give me a pointer to the issue where this is happening? I'll have a look tomorrow. If we are running into broken-tomorrow-problems with the cookie approach too, we might decide to not go the double-cookie path after all.

Best regards,
D.R.

@Tratcher
Copy link
Member

Tratcher commented Dec 2, 2019

What works today may (and will be!) broken tomorrow.

Perhaps, but the fix is usually easy as well.

Can you give me a pointer to the issue where this is happening?

We're summarizing it in a new doc here:
https://github.com/aspnet/AspNetCore.Docs/pull/15618/files#diff-5508379853be41560030cbe1228ce324R85-R120

@huysentruitw
Copy link
Contributor

@Tratcher is it ok to take that sample code from the doc and create a drop-in NuGet package for it?

@Tratcher
Copy link
Member

Tratcher commented Dec 4, 2019

@huysentruitw Only if you want to maintain it, it will likely need additions. That's why we didn't embedded it in the framework.

@analogrelay
Copy link
Contributor

Triage: We don't intend to support double-cookies at this time. There are a number of problems with implementing it and it would significant disrupt the ecosystem. Our recommendation here is to use the browser sniffing mechanism.

@clement911
Copy link

We have implemented a solution that combines both the policy solution and the double cookie solution to avoid sniffing the agent.
When the user first interacts with our site, we issue two dummy cookies, one with SameSite.None and one with SameSite.Unspecified.
In future requests, our cookie policy checks if the one with Unspecified is in the request but the one with None is not. If so, it means the browser doesn't implement the None option.
It seems to work fine for us, although I'm pretty sure this won't work for all situations.

private void CheckSameSite(HttpContext ctx, CookieOptions options, string cookieName)
        {
            if (cookieName.In(TEST_COOKIE_SAMESITE_UNSPECIFIED, TEST_COOKIE_SAMESITE_NONE))
                return;

            if (options.SameSite != SameSiteMode.None)
                return;

            var cookies = ctx.Request.Cookies;
            bool browserDoesntRecognizeSameSiteNone = cookies.ContainsKey(TEST_COOKIE_SAMESITE_UNSPECIFIED) &&
                                                      !cookies.ContainsKey(TEST_COOKIE_SAMESITE_NONE);

            if (browserDoesntRecognizeSameSiteNone)
                options.SameSite = SameSiteMode.Unspecified;
        }

@drauch
Copy link
Author

drauch commented Dec 10, 2019

That sounds like a very good idea. How do you add this extra request to the pipeline before the authentication redirect is performed?

@clement911
Copy link

Yes, we issue these two cookies if they are not present, before even authenticating the user.

@Tratcher
Copy link
Member

Tratcher commented Dec 10, 2019

In future requests, our cookie policy checks if the one with Unspecified is in the request but the one with None is not. If so, it means the browser doesn't implement the None option.

This might work under some conditions, but I expect it will usually be too late. E.g. You have to know this information when you first issue a cookie, but most of the time that is on a normal SameSite-safe request so you get both of your test cookies. By the time the SameSite protections kick in, it's already too late to change the cookie that matters. E.g. in a remote login flow you'd do this check during the initial challenge which is a SameSite request, issue the temp auth cookie normally, but when you returned from the 3rd party then both the test cookie and the temp cookie would fail.

@clement911
Copy link

That's a good point.
The reason it works for us is because we are in a third party context (iFrame) when we issue the authentication cookie.

@drauch
Copy link
Author

drauch commented Dec 10, 2019

A colleague of mine had the following idea today: in a middlware before OpenID Connect we catch all POST requests from our OpenID Connect authentication provider (those requests have no cookies attached), and render a page which immediately posts the same values via AJAX back to the server (cookies are now attached, are they?!). After receiving the response (including the actual authentication cookie) we redirect to the actual redirect URL. Could this work?

@brockallen
Copy link

You mean something like this, but for POST instead of just GET?

https://brockallen.com/2019/01/11/same-site-cookies-asp-net-core-and-external-authentication-providers/

@huysentruitw
Copy link
Contributor

@drauch sounds like a clever idea. The middleware could check on endpoint, POST and the lack of the nonce cookie. Only need to prevent not to get into an endless loop, right?

@drauch
Copy link
Author

drauch commented Dec 10, 2019

@brockallen:

You mean something like this, but for POST instead of just GET?

Yes. Have you tested it with POST as well?

BTW: Are you sure that in step 3 the original nonce cookie for the external provider (which is set in response to step 1) is included in step 3? I think it already fails in step 3 because of this, not in step 4.

@drauch
Copy link
Author

drauch commented Dec 10, 2019

@huysentruitw : I'm not sure what your sentence is asking of me, sorry :-) Could you rephrase it?

@huysentruitw
Copy link
Contributor

@drauch I'm just thinking that an implementation like that could easily get into an endless loop of POST-ing the same thing back (f.e. when the nonce cookie has already expired for whatever reason).

@drauch
Copy link
Author

drauch commented Dec 10, 2019

@huysentruitw : Ah, now I understand. Yes, you would need to mitigate such situations, if the POST comes from within the page and the response to it would be a 302 to the authentication provider, instead fail with 403.

@dotnet dotnet locked as resolved and limited conversation to collaborators Jan 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer
Projects
None yet
Development

No branches or pull requests

8 participants