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

Cookies set by oauthOidc -filters are occasionally too big #1089

Closed
josafat opened this issue Jun 12, 2019 · 12 comments
Closed

Cookies set by oauthOidc -filters are occasionally too big #1089

josafat opened this issue Jun 12, 2019 · 12 comments
Assignees
Labels
bugfix Bug fixes and patches

Comments

@josafat
Copy link

josafat commented Jun 12, 2019

Describe the bug
oauthOidc* -filters like oauthOidcAnyClaims set a cookie from the client. The cookie size reaches easily the maximum size that the browsers accept (4096 bytes on Chrome?) causing the cookie to be dropped. With a basic OIDC setup e.g. with Keycloak, the size of the cookie easily reaches 5100bytes.

To Reproduce

  1. Any route with oauthOidcAnyClaims -filter, which passes OIDC auth.
test:
      PathSubtree("/test/")
      -> oauthOidcAnyClaims("https://test.net/auth/realms/testrealm", "test", "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxxx", "https://test.net/test/", "email profile", "name email")
      -> "http://test:9912";

Callback request sent after successful authentication to oidcp

Request URL: https://test.net/test/?state=e30123f4c2164...(truncated)...d791826ebce&session_state=614e2f07-8c31-...(truncated)...-d215d8569e70
Request Method: GET
Status Code: 307 

Response headers:

content-length: 0
date: Thu, 06 Jun 2019 09:14:34 GMT
location: /test
server: Skipper
set-cookie: skipperOauthOidcec3dfd82=d6a93c3bc69cae1201503...(truncated 5.75KB of data)...1a1c0b72ddd32577ed2b443d990891558930364288da0835; Path=/; HttpOnly; MaxAge=3600; Domain=test.net
status: 307

Expected behavior
The cookie(s) fit to browsers. E.g. the big ones are split into smaller cookies and merged when read. As an example keycloak-gatekeeper, which acts in a similar role from OIDC point of view splits the big cookies into smaller ones https://github.com/keycloak/keycloak-gatekeeper/blob/master/cookies.go

Observed behavior
Cookies are rejected by Chrome, Safari due to too big size.

@szuecs
Copy link
Member

szuecs commented Jun 12, 2019

@josafat thanks for the detailed explanation!

@arjunrn do you want to work on a fix?

@szuecs szuecs added the bugfix Bug fixes and patches label Jun 12, 2019
@arjunrn
Copy link
Contributor

arjunrn commented Jun 16, 2019

@szuecs yes I will assign the issue to myself

@arjunrn arjunrn self-assigned this Jun 16, 2019
@universam1
Copy link
Contributor

Looks like we are hitting the same issue

@szuecs
Copy link
Member

szuecs commented Jan 24, 2020

It would be nice to have a test bed here, such that we can fix it more easy.

@universam1 Do you want to work on a fix for the oidc filter? Not sure if splitting the cookie or if gzip the cookie content could help. Split would be a fix that clearly works. What do you think?

@universam1
Copy link
Contributor

Chrome says "Malformed Response Cookies: This set-cookie had invalid syntax"

Example truncated:

Set-Cookie: skipperOauthOidcb70dad48=98c8e57ca6...................adf43cd78a8e22893b4; Path=/; Secure; HttpOnly; MaxAge=3600; Domain=o.....org

@universam1
Copy link
Contributor

universam1 commented Jan 24, 2020

Made a test for my idP token, they are 14.4k plain and 5.5k using simple gzip compression. Guess the idea to gzip @szuecs is an interesting one. Nevertheless, splitting must happen as well.
If you like @szuecs I could work on an implementation of that

@szuecs szuecs assigned universam1 and unassigned arjunrn Jan 24, 2020
@szuecs
Copy link
Member

szuecs commented Jan 24, 2020

I am happy to review your pr or if you need guidance let us know!

@universam1
Copy link
Contributor

Ok sounds good- is there any policy or suggestions from your side that would be valuable to know for an acceptable implementation?

@szuecs
Copy link
Member

szuecs commented Jan 24, 2020

Normal Go style, tests that show the problem and that your fix work. Simple is better. :)
Sign off your commits is mandatory.

I guess, you will add a function, that checks cookie content size, gzip if too big, split into chunks of cookieMaxSize - len(cookieConfig) and append all the cookies and have a counter in the name. And to check the cookie you combine them again in another function. And add tests for both functions.
I hope this helps.

universam1 pushed a commit to universam1/skipper that referenced this issue Jan 28, 2020
This solves a cookie size overflow issue zalando#1089 where at some point the cookie is not accepted by the browser due to its size

* adding a `chunkCookie` function that splits an existing cookie based on limits defined at `cookieMaxSize`
* the value of `cookieMaxSize` is taken by well-known projects and verified with Chrome
* the approach is `non-deterministic` as the signature of cookies might change over time
* mergerCookies return a single (virtual) cookie, ordered by the suffix

Signed-off-by: Samuel Lang <gh@lang-sam.de>
@universam1
Copy link
Contributor

@szuecs Please see my first part of the proposed solution in #1289 . I like to split the chunking and the compressing into two PR as they are not directly tied and it will allow you possibly an easier review.

aryszka pushed a commit that referenced this issue Jan 28, 2020
* adding OIDC Cookie chunk and merge functions

This solves a cookie size overflow issue #1089 where at some point the cookie is not accepted by the browser due to its size

* adding a `chunkCookie` function that splits an existing cookie based on limits defined at `cookieMaxSize`
* the value of `cookieMaxSize` is taken by well-known projects and verified with Chrome
* the approach is `non-deterministic` as the signature of cookies might change over time
* mergerCookies return a single (virtual) cookie, ordered by the suffix

Signed-off-by: Samuel Lang <gh@lang-sam.de>

* implementing cookie chunks for OIDC

* leveraging the `chunkCookie` and at the response `mergerCookie` to split the data into valid chunks
* switching serialization to Base64 instead of Base16 since it reduces the transaction data by 33% and is considered common practice. It does not influence other cookies of Skipper

Signed-off-by: Samuel Lang <gh@lang-sam.de>
@szuecs
Copy link
Member

szuecs commented Jan 28, 2020

Thanks @universam1 your PR triggered the release: https://github.com/zalando/skipper/releases/tag/v0.11.30

@szuecs szuecs closed this as completed Jan 28, 2020
universam1 added a commit to universam1/skipper that referenced this issue Jan 31, 2020
This is the second part follow up of zalando#1089 to properly deal with large JWT token and cookies.
Tests showed that compression on the cookies pays off from >200 bytes payload which is granted.

Leveraging `sync.Pools` that provide safe parallelization and by reusing the writer also less memory footprint, benchmarks included.

`Deflate` compression is chosen after tests with similar algorithms, providing the best balance of simple implementation, speed and ratio.

Signed-off-by: Samuel Lang <gh@lang-sam.de>
universam1 added a commit to universam1/skipper that referenced this issue Jan 31, 2020
This is the second part follow up of zalando#1089 to properly deal with large JWT token and cookies.
Tests showed that compression on the cookies pays off from >200 bytes payload which is granted.

Leveraging `sync.Pools` that provide safe parallelization and by reusing the writer also less memory footprint, benchmarks included.

`Deflate` compression is chosen after tests with similar algorithms, providing the best balance of simple implementation, speed and ratio.

Signed-off-by: Samuel Lang <gh@lang-sam.de>
@universam1
Copy link
Contributor

universam1 commented Jan 31, 2020

@szuecs Please see the second part #1293 to properly deal with large JWT token and cookies by compressing the cookie. Your idea is a great one - it actually not only solves our issues with ALB limits but also improves the overall latency on the deployments quite a bit!

universam1 added a commit to universam1/skipper that referenced this issue Feb 2, 2020
This is the second part follow up of zalando#1089 to properly deal with large JWT token and cookies.
Tests showed that compression on the cookies pays off from >200 bytes payload which is granted.

Leveraging `sync.Pools` that provide safe parallelization and by reusing the writer also less memory footprint, benchmarks included.

`Deflate` compression is chosen after tests with similar algorithms, providing the best balance of simple implementation, speed and ratio.

Signed-off-by: Samuel Lang <gh@lang-sam.de>
universam1 added a commit to universam1/skipper that referenced this issue Feb 3, 2020
This is the second part follow up of zalando#1089 to properly deal with large JWT token and cookies.
Tests showed that compression on the cookies pays off from >200 bytes payload which is granted.

Leveraging `sync.Pools` that provide safe parallelization and by reusing the writer also less memory footprint, benchmarks included.

`Deflate` compression is chosen after tests with similar algorithms, providing the best balance of simple implementation, speed and ratio.

Signed-off-by: Samuel Lang <gh@lang-sam.de>
aryszka pushed a commit that referenced this issue Feb 4, 2020
This is the second part follow up of #1089 to properly deal with large JWT token and cookies.
Tests showed that compression on the cookies pays off from >200 bytes payload which is granted.

Leveraging `sync.Pools` that provide safe parallelization and by reusing the writer also less memory footprint, benchmarks included.

`Deflate` compression is chosen after tests with similar algorithms, providing the best balance of simple implementation, speed and ratio.

Signed-off-by: Samuel Lang <gh@lang-sam.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Bug fixes and patches
Projects
None yet
Development

No branches or pull requests

4 participants