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

sso-proxy: avoid oversized cookies #95

Merged
merged 3 commits into from
Apr 3, 2019

Conversation

sporkmonger
Copy link
Contributor

Problem

Some potential providers (e.g. Azure AD) use very large tokens. Once you apply encryption to them, they exceed 4096 bytes, which causes the browser to reject them. This PR gives a little more breathing room for larger tokens.

Solution

This gzips the marshalled json before encrypting it. I couldn't think of an obvious scenario where introducing compression would leak information (via cookie length) that would compromise security since the contents of the cookie are already largely known to the client, and this doesn't aid in forging an attacker controlled cookie.

Notes

I was getting cookies in the ballpark of 4400 bytes before this PR, and they're running around 2900 bytes with the same payload now. Chrome (and presumably most other browsers) enforces a limit of 4096 bytes. May be worth also triggering a server side error if we exceed that, since it took me 3 days of debugging to figure out that was happening.

luisdavim
luisdavim previously approved these changes Oct 21, 2018
@shrayolacrayon
Copy link

Thanks for opening this PR @sporkmonger. Could you add some tests and also add the server-side error for exceeded payloads in this PR if it's not too much?
Also a note that deploying this change on our side will also affect existing cookies that are not zipped, so we will need to deploy/validate this to ensure that there are no UX changes (there should not be since we have redirects in place) before merging.

@sporkmonger
Copy link
Contributor Author

Sure, not a problem. Might take a couple days since I've got a bunch of stuff in the air right now.

@danbf
Copy link
Contributor

danbf commented Oct 30, 2018

@shrayolacrayon
Copy link

@sporkmonger I have a potential solution for supporting multiple cookies in a session to store the session state but it depends on #43 being completed. I have this prioritized to work on this week, so I'll try to push up a PR as soon as I can.

After both sso-proxy and sso-auth use a SessionStore for saving and loading sessions, we could create a new cookie store that would be a distributed sessions store that implements the SessionStore interface and can be configured to be used instead of CookieStore for providers that need it.

@mreiferson mreiferson changed the title Avoid oversized cookies sso-proxy: avoid oversized cookies Nov 26, 2018
@mreiferson mreiferson added the enhancement New feature or request label Nov 26, 2018
@sporkmonger
Copy link
Contributor Author

@shrayolacrayon now that #43 / #137 are done, could you elaborate a bit on the multi-cookie idea?

@sporkmonger
Copy link
Contributor Author

sporkmonger commented Jan 19, 2019

I've made significant progress on a cookie-spanning PR, but I'm going to implement it separately from this one. ☝️ I talked to @shrayolacrayon to confirm no issue w/ plans.

@sporkmonger sporkmonger mentioned this pull request Jan 29, 2019
@sporkmonger
Copy link
Contributor Author

This PR does not interfere with #150. We're running a branch that has both.

@shrayolacrayon
Copy link

@sporkmonger, I approved this but it looks like there might be a CI failure, probably from updating the stale branch. I can merge this in once that's resolved!

@sporkmonger
Copy link
Contributor Author

OK, I'll take a look.

@shrayolacrayon shrayolacrayon merged commit 6ea74f2 into buzzfeed:master Apr 3, 2019
@sporkmonger sporkmonger deleted the compress-cookie branch April 23, 2019 02:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants