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.

Sign up for GitHub

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

Jump to bottom

Support dropping skipperOauthOidc cookie #3459

Open
askorczyk-jw opened this issue Mar 28, 2025 · 4 comments · May be fixed by #3465
Open

Support dropping skipperOauthOidc cookie #3459

askorczyk-jw opened this issue Mar 28, 2025 · 4 comments · May be fixed by #3465
Assignees
@AndreasSko

Comments

@askorczyk-jw
Copy link

Is your feature request related to a problem? Please describe.
Some of our applications can't easily handle the size of the header forwarded by Skipper. This is mainly due to the skipperOauthOidc* cookies, which the application doesn't even use. We were thinking of just dropping the skipperOauthOidc* cookies when forwarding the request.

Describe the solution you would like
We thought of two different approaches we could take:

  1. Either we could adjust the existing oauthOidc* filters to drop the OIDC cookies before forwarding it. This could be hidden behind a feature flag and turned off by default
  2. Another more generic solution could be to add functionality to the dropRequestCookie filter to drop based on a regex. This could be turned off by default, but enabled by an optional parameter. Something like dropRequestCookie("skipperOauthOidc.*", "true").

Describe alternatives you've considered (optional)
We could probably get it to work with a custom Lua script as well, but it seems a bit fiddly.

Would you like to work on it?
Yes
@askorczyk-jw
Copy link
Author

cc @universam1

@szuecs
Copy link
Member

szuecs commented Mar 28, 2025

Hmm @AlexanderYastrebov why do we even forward cookies to the backend?
We could drop request session cookies in the filters.

@AlexanderYastrebov
Copy link
Member

Hmm @AlexanderYastrebov why do we even forward cookies to the backend? We could drop request session cookies in the filters.

Right, the cookie is an encrypted blob intended only for Skipper so it should be safe just to drop it after authenticating.

We do this for oauthGrant()

skipper/filters/auth/grantcookie.go

Lines 93 to 100 in 6b448b1

// extractCookie removes and returns the OAuth Grant token cookie from a HTTP request.
// The function supports multiple cookies with the same name and returns
// the best match (the one that decodes properly).
// The client may send multiple cookies if a parent domain has set a
// cookie of the same name.
// The grant token cookie is extracted so it does not get exposed to untrusted downstream
// services.
func (ce *EncryptedCookieEncoder) extractCookie(request *http.Request) (*cookie, error) {

but apparently not for OIDC.
This also seems to fit #3455

@AlexanderYastrebov AlexanderYastrebov mentioned this issue Mar 24, 2025
Open
@AndreasSko AndreasSko linked a pull request Mar 31, 2025 that will close this issue
Open
@AndreasSko
Copy link
Contributor

Thank you for the very quick feedback, @szuecs @AlexanderYastrebov ! 🙌
I created a small PR that drops the cookie here: #3465 If you have the chance, could you review it for me?
(Sorry, I opened this issue with my wrong account 🤦‍♂️)

AndreasSko added a commit to o11n/skipper that referenced this issue Apr 7, 2025
@AndreasSko
Drop skipperOauthOidc cookies in oauthOidc filter
8efd40f 
The `skipperOauthOidc` cookies are only intended for Skipper, so they
don't need to be forwarded to the application.

With this PR we simply drop those cookies before forwarding the request.
This will reduce the overall header size of requests.
The implementation is inspired by the
[`oauthGrant` filter](https://github.com/zalando/skipper/blob/6b448b1fe90cc113e365be8fba7cd6d122ad7a6d/filters/auth/grantcookie.go#L93-L100)

Closes zalando#3459

Signed-off-by: Andreas Skorczyk <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@AndreasSko AndreasSko

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Drop skipperOauthOidc cookies in oauthOidc filter o11n/skipper
4 participants
@szuecs @AlexanderYastrebov @AndreasSko @askorczyk-jw