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

Drop skipperOauthOidc cookies in oauthOidc filter #3465

Merged
merged 1 commit into from
Apr 14, 2025

Conversation

AndreasSko
Copy link
Contributor

@AndreasSko AndreasSko commented Mar 31, 2025

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

Closes #3459

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request removes the skipperOauthOidc cookies from requests before they are forwarded to reduce header size. Key changes include:

  • Adding a new test assertion to ensure the cookie is dropped.
  • Modifying the request processing in the oidc filter to remove cookies with a name matching the skipperOauthOidc pattern.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
filters/auth/oidc_test.go Added assertion to verify that the skipperOauthOidc cookie is dropped.
filters/auth/oidc.go Updated cookie processing to drop cookies with names starting with the skipperOauthOidc prefix.
Comments suppressed due to low confidence (2)

filters/auth/oidc.go:795

  • [nitpick] Consider aligning the cookie name reference (f.cookiename) with the naming used in tests (oauthOidcCookieName) to ensure consistency across the codebase.
if strings.HasPrefix(cookie.Name, f.cookiename) {

filters/auth/oidc_test.go:867

  • Verify that the oauthOidcCookieName used in the test matches the cookie name pattern applied in the oidc filter for consistency.
assert.NotContains(t, string(requestDump), oauthOidcCookieName, oauthOidcCookieName+" cookie should be dropped")

@AlexanderYastrebov
Copy link
Member

AlexanderYastrebov commented Apr 1, 2025

@AndreasSko FYI, I am testing what Copilot review is capable of so I've requested a review from it.

@AndreasSko AndreasSko force-pushed the del_oidc_cookie branch 2 times, most recently from a284aec to 08560c3 Compare April 2, 2025 08:15
@AlexanderYastrebov AlexanderYastrebov added the major moderate risk, for example new API, small filter changes that have no risk like refactoring or logs label Apr 2, 2025
@AndreasSko
Copy link
Contributor Author

@AlexanderYastrebov thank you for the review! I adjusted the code 🙂
In the process I noticed a bug in the insecureCookieJar, which I fixed as well (1889ea3).

@AlexanderYastrebov
Copy link
Member

This feels somehow related to #1965

@AndreasSko
Copy link
Contributor Author

This feels somehow related to #1965

Yeah, true. Would you like me to adjust anything in the test?

@AlexanderYastrebov
Copy link
Member

👍

@AlexanderYastrebov
Copy link
Member

Thank you for improving Skipper.

@AlexanderYastrebov
Copy link
Member

@AndreasSko Could you please squash and rebase on top of master and make proper commit message. PR description and title should match those of commit message - the reason is that we release each merge to master so commit message becomes release notes.

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]>
@AndreasSko
Copy link
Contributor Author

Sure! Squashed and adjusted the commit message to match the PR description 🙂

@AlexanderYastrebov
Copy link
Member

👍

@AndreasSko
Copy link
Contributor Author

@AlexanderYastrebov @MustafaSaber any update on this one? We are very eager to use it soon 😁😊

@MustafaSaber
Copy link
Member

👍

@MustafaSaber MustafaSaber merged commit 1c7e275 into zalando:master Apr 14, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major moderate risk, for example new API, small filter changes that have no risk like refactoring or logs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support dropping skipperOauthOidc cookie
3 participants