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

fix: Add custom sessionstore with cookie-splitting functionality #117

Open
wants to merge 2 commits into
base: master
from

Conversation

@Croissong
Copy link

commented Mar 8, 2019

This PR extends the gorilla/sessions.CookieStore with the feature of automatically splitting the session cookie into multiple smaller ones if it is > 4kb.

Despite the efforts in #85 we still faced the issue of securecookie: the value is too long because some id_tokens were growing larger than 4kb (in our case from azure AD auth).
This solution (and implementation) is largely inspired by oauth2_proxy and keycloak-gatekeeper as they do the same thing.

I hope the comments explain most things - I also wrote some tests ;).
However, I'm not using go too often so feel free to comment on any changes you'd like me to make.
PS: thank you for maintaining the project, it helps us a lot.

@lucasslima

This comment has been minimized.

Copy link

commented Apr 26, 2019

Can confirm that this patch works, is currently running on one of my clusters. We also have a problem with the tokens being too large.

Important note for users trying to apply it, make sure your ingress controller also has a expanded proxy-buffer-size configuration, otherwise you'll hit a 502 with the following error on the Ingress Controller:

2019/04/26 20:02:59 [error] 503#503: *9515930 upstream sent too big header while reading response header from upstream, client: 172.18.176.0, server: .., request: "GET /callback?code=z4x2bjar5u62qtozhtpo45dfw&state=j7kGI5Xi8tCI4DYWNA04RcW5NLCTSaGHy6%2FJgsKQQtM%3D HTTP/2.0", upstream: "http://172.18.128.27:8080/callback?code=z4x2bjar5u62qtozhtpo45dfw&state=j7kGI5Xi8tCI4DYWNA04RcW5NLCTSaGHy6%2FJgsKQQtM%3D", host: "...", referrer: ".."

You can add the configuration on the gangway Ingress resource with the annotation nginx.ingress.kubernetes.io/proxy-buffer-size: "20k", assuming an Nginx Ingress Controller.

@Croissong

This comment has been minimized.

Copy link
Author

commented May 14, 2019

Rebased onto master.
Would be awesome if you could have a look.

@nickperry

This comment has been minimized.

Copy link

commented May 28, 2019

Considering the additional testing of this fix in #130, would it be possible to get this PR merged now?

@nickperry
Copy link

left a comment

LGTM

@figaw

This comment has been minimized.

Copy link

commented Jul 15, 2019

Do you have an update on this? Thank you very much.

@devopstales

This comment has been minimized.

Copy link

commented Jul 22, 2019

Any update on this?

@benceszikora

This comment has been minimized.

Copy link

commented Jul 22, 2019

We are running on our internal cluster with this fix applied and it works perfectly. It would be nice to see this merged!

@SEJeff

This comment has been minimized.

Copy link

commented Jul 22, 2019

I can confirm this PR fixes Azure Active Directory auth for us.

Maybe cc: @jpweber, @alexbrand, or @stevesloka could take a look?

@jdconti

This comment has been minimized.

Copy link

commented Jul 22, 2019

I can also confirm this PR address our issues... thanks!

@pszelestey
Copy link

left a comment

I also tested this fix. It solves our problem when a user has a high number of AD groups. Please merge this PR. Thank you!

@splushii

This comment has been minimized.

Copy link

commented Aug 19, 2019

I tested this fix which initially solved our problem with users having a lot of groups, but encountered it again (with a user having even more groups) which led me to bump cookie.MaxLength from 16384 to 81920. Other than that it works as a charm!

fix: Add custom sessionstore with cookie-splitting functionality
Signed-off-by: Jan Möller <jan.moeller0@gmail.com>

@Croissong Croissong force-pushed the Croissong:fix/split_large_cookies branch from 6ffdbd3 to ae266a8 Aug 19, 2019

@Croissong

This comment has been minimized.

Copy link
Author

commented Aug 19, 2019

@splushii You're right, i was a bit too conservative here. Bumped it to 81920 as well.

(make) Rm unused check; Update staticcheck with unused flag
Unused functionality has been integrated into staticcheck

Signed-off-by: Jan Möller <jan.moeller0@gmail.com>

@Croissong Croissong force-pushed the Croissong:fix/split_large_cookies branch from 220da84 to a1a811c Aug 19, 2019

@johnharris85

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

LGTM, but maybe we should also add a comment to the effect of #117 (comment) in the docs as part of this PR as well?

@alexbrand
Copy link
Collaborator

left a comment

Overall LGTM. I agree with @johnharris85 on the note re: configuring ingress. Would be great to get that in the documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.