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

Add suffix for oauth cookies #2664

Merged
merged 4 commits into from
Feb 22, 2024

Conversation

zhaohuabing
Copy link
Member

@zhaohuabing zhaohuabing commented Feb 20, 2024

This PR adds a suffix for oauth cookies to prevent multiple oauth filters from overwriting each other's cookies.

The suffix is the hashed UID of the SecurityPolicy that contains the OIDC configuration.

For example:

                  bearerToken: BearerToken-5F93C2E4
                  idToken: IdToken-5F93C2E4
                  oauthExpires: OauthExpires-5F93C2E4
                  oauthHmac: OauthHMAC-5F93C2E4
                  refreshToken: RefreshToken-5F93C2E4

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
@zhaohuabing zhaohuabing requested a review from a team as a code owner February 20, 2024 13:15
@zhaohuabing zhaohuabing requested a review from arkodg February 20, 2024 13:15
Copy link

codecov bot commented Feb 20, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (7ab3da6) 63.36% compared to head (822d5dd) 63.37%.
Report is 1 commits behind head on main.

Files Patch % Lines
internal/gatewayapi/securitypolicy.go 57.14% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2664      +/-   ##
==========================================
+ Coverage   63.36%   63.37%   +0.01%     
==========================================
  Files         119      119              
  Lines       19098    19112      +14     
==========================================
+ Hits        12102    12113      +11     
- Misses       6196     6198       +2     
- Partials      800      801       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zhaohuabing zhaohuabing enabled auto-merge (squash) February 21, 2024 03:02
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
internal/gatewayapi/securitypolicy.go Outdated Show resolved Hide resolved
internal/gatewayapi/securitypolicy.go Outdated Show resolved Hide resolved
@zhaohuabing zhaohuabing force-pushed the oauth-cookie-names branch 4 times, most recently from e83b3bf to 0496bbd Compare February 22, 2024 02:21
Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
@@ -494,6 +495,12 @@ func (t *Translator) buildOIDC(
logoutPath = *oidc.LogoutPath
}

h := fnv.New32a()
Copy link
Contributor

Choose a reason for hiding this comment

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

can we reuse

func HashString(str string) string {
?

Copy link
Contributor

Choose a reason for hiding this comment

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

HashString uses SHA256, which is theoretically slower than FNV-32, and generates a 256-bit result.

I don't think it matters that much for this purpose that the hash being used is slower, or crypto-secure, but using the full 256 bits in the cookie suffix would result in 64 characters instead of the current 8. I think that would be a little long for a cookie name.

Using only 32 bit from the result would theoretically be OK for this purpose, but I would prefer to use the FNV-32 hash function here so that the entire hash result would still be used.

Copy link
Member Author

@zhaohuabing zhaohuabing Feb 22, 2024

Choose a reason for hiding this comment

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

It's just 5 lines and I don't like importing provider/utils package in gateway :-)
WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

move it into a more common package, e.g. internal/util

Copy link
Contributor

Choose a reason for hiding this comment

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

non blocking comment - less code, lesser bugs, lesser vulnerabilities, if this hash function is better, lets replace it with the one in provider/utils, as @zirain suggests, if provider/utils is the wrong home, lets move it to utils

Copy link
Member Author

@zhaohuabing zhaohuabing Feb 23, 2024

Choose a reason for hiding this comment

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

Will do in a follow-up PR because changing the location of the provider/utils impacts many places.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@liorokman liorokman left a comment

Choose a reason for hiding this comment

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

LGTM

@zhaohuabing zhaohuabing merged commit fc7d6bc into envoyproxy:main Feb 22, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants