search

vmware-archive / gangway

An application that can be used to easily enable authentication flows via OIDC for a kubernetes cluster.
Apache License 2.0
410 stars 113 forks source link

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

Closed croissong closed 5 years ago

croissong commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

nickperry commented 5 years ago

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

figaw commented 5 years ago

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

devopstales commented 5 years ago

Any update on this?

benceszikora commented 5 years ago

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

SEJeff commented 5 years ago

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

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

jdconti commented 5 years ago

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

splushii commented 5 years ago

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!

croissong commented 5 years ago

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

johnharris85 commented 5 years ago

LGTM, but maybe we should also add a comment to the effect of https://github.com/heptiolabs/gangway/pull/117#issuecomment-487193918 in the docs as part of this PR as well?

jenting commented 5 years ago

Could this PR be merged?

croissong commented 5 years ago

Sorry, i didn't find time yet to update the docs :(

pszelestey commented 5 years ago

Any update on this PR? Would you be so kind to merge it? Thank you in advance!

alexbrand commented 5 years ago

We can tackle the docs in a follow-up issue/PR.