Cookie overflow fix

[sporkmonger]

Problem

Azure AD (#118) produces massive 3kb tokens. Storing them directly inside cookies produces around 5kb of cookie after compression (#95) and encryption. Browsers typically limit sites to only 4096 bytes (or in some cases, very slightly less) per cookie, with about 110 bytes or so being used up by the cookie name and various attributes. So in practice, you can only store about 3986ish bytes per cookie, which the Azure AD provider goes way past.

Solution

Since we don't want to maintain server-side state, I've implemented a cookie store that automatically spans the data being stored across multiple cookies when needed, using a byte length prefix in the first cookie. It uses ~ as the delimiter between the cookie length prefix and the spanned cookie value since it's not used by any base64 encoding and also not escaped in URLs, HTTP headers, or HTML forms. The prefix is simply a base64 encoded binary serialization of the integer byte length.

Notes

The prefix length isn't signed or encrypted and could be altered by a malicious client, however altering it should't do anything beneficial. It should just result in an error (all error conditions covered in test cases).

[sporkmonger]

Tests appear to be more brittle than I'd have suspected. Might need to revisit the byte length prefix encoding design.

[sporkmonger]

Test failures were because the cookie max size shifted and test cases were expressed in terms of that constant.

[sporkmonger]

One option I'm considering is this:

commit 54b933473376bf0b5bdf4fb3a0a81a4fa7383da5
Author: Bob Aman <boba@remitly.com>
Date:   Wed Jan 30 10:43:45 2019 -0800

    Clear legacy cookie on signout

diff --git a/internal/pkg/sessions/cookie_store.go b/internal/pkg/sessions/cookie_store.go
index 0fc9f44..93b8544 100644
--- a/internal/pkg/sessions/cookie_store.go
+++ b/internal/pkg/sessions/cookie_store.go
@@ -257,6 +257,8 @@ func (s *CookieStore) GetCSRF(req *http.Request) (*http.Cookie, error) {
 
 // ClearSession clears the session cookie from a request
 func (s *CookieStore) ClearSession(rw http.ResponseWriter, req *http.Request) {
+       // Clear the old cookies
+       http.SetCookie(rw, s.makeCookie(req, s.Name, "", time.Hour*-1, time.Now()))
        setCookies(rw, s.makeSessionCookies(req, "", time.Hour*-1, time.Now()))
 }

I don't know if it's critical or not for anyone with an existing install to clear out existing cookies. The cookies would eventually expire anyway, so explicit clears may not be strictly necessary, but we did run into some issues in our testing w/ existing cookies, so I'm throwing it out there as an option.

[sporkmonger]

This should be ready to review. Let me know if you'd like me to clear the old cookies or not.

[shrayolacrayon]

One high level comment - I think I'd rather we separate out the overflow cookie store than change the behavior of the existing CookieStore. This approach would work by creating an implementation of the SessionCookie interface that has the overflow behavior, which then could be configured to be used for the Azure AD Provider. Let me know what you think about that!

[shrayolacrayon]

See comment above for requested changes!

[sporkmonger]

I like the idea, it feels a bit cleaner in some respects, but I worry there are practical concerns.

Were you thinking that we'd swap cookie store implementations depending on provider somehow?

Setup code for cookie store is here:

authenticator, err := auth.NewAuthenticator(opts, emailValidator, auth.AssignProvider(opts), auth.SetCookieStore(opts), auth.AssignStatsdClient(opts))
if err != nil {
	logger.Error(err, "error creating new Authenticator")
	os.Exit(1)
}

Presumably auth.SetCookieStore would need to get provider-specific logic to make that work, which seems non-ideal. Maybe auth.AssignProvider could have side-effects on the opts struct, but that feels even worse to me.

The alternative solution of putting it in the hands of the person deploying SSO by making it a config switch increases documentation burden. It's already pretty rough to get a provider set up and working as intended, so I'm a little nervous about adding to the pile of things you need to consider when setting up.

One possibility might be to make it a config switch but just have it be the default, so you only have a documentation burden if you need to turn it off for some reason?

[shrayolacrayon]

What provider specific information, other than the name of the provider do you think would be necessary?
Since we already set the provider name in the config, we could feasibly switch on the provider name to assign the appropriate store for the appropriate provider.

[sporkmonger]

I suppose that's true. If it could be managed entirely by the provider switch (switch o.Provider) in options.go, that might be OK.

[sylr]

Hi!

Could we move forward with this ? It's blocking the Azure Ad implementation.

Regards.

[sporkmonger]

While yes, it's blocking Azure AD, as I mentioned in that PR, I don't think cookie splitting in practice is great. Some part of that is simply that the failure scenario is pretty opaque to the user. Browsers just drop the cookie on the floor, which sends them into an authentication loop. Debugging can be a challenge too because, if you don't have checkboxes for preserving logs set, the error message telling you your cookie was too big only happens on a page behind a redirect, so you never see the error in the console unless you're actively looking specifically for it. Meanwhile browsers measure cookie limits in really surprising ways, and I've had to repeatedly reduce the max bytes-per-cookie limit for weird edge cases we keep hitting. It's super painful to deal with and I'd rather just use Redis for sessions.

[sporkmonger]

We've found that running with cookie splitting tends to result in a lot of tribal "knowledge" of "did you try clearing your cookies?" whenever someone reports something isn't working. I don't believe this is a good long term end state for things to land in.

[sylr]

@sporkmonger I totally agree but unfortunately, unless someone starts to implement #158, that's the only solution we currently have.

[sporkmonger]

We've been running this in production for some time and I think I can confidently say that this approach is a bad idea. I'm going to close this PR now that #158 is underway.