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

Add suffix for oauth cookies #2664

Merged
merged 4 commits into from
Feb 22, 2024
Merged

Add suffix for oauth cookies #2664

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eeea5c7
add suffix for oauth cookies
zhaohuabing Feb 20, 2024
ac86fbc
use hash as suffix
zhaohuabing Feb 21, 2024
b84c92e
Merge branch 'main' into oauth-cookie-names
zhaohuabing Feb 21, 2024
822d5dd
generate hash with policy UID
zhaohuabing Feb 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions internal/gatewayapi/securitypolicy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
"encoding/json"
"errors"
"fmt"
"hash/fnv"
"net/http"
"net/netip"
"net/url"
Expand Down Expand Up @@ -494,6 +495,17 @@
logoutPath = *oidc.LogoutPath
}

nsName := types.NamespacedName{
Namespace: policy.GetNamespace(),
Name: policy.GetName(),
}
zhaohuabing marked this conversation as resolved.
Show resolved Hide resolved
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

gateway/internal/provider/utils/utils.go

Line 39 in 4c79ef9

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 •
edited
Loading

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 •
edited
Loading

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.

#2681

_, err = h.Write([]byte(nsName.String()))
if err != nil {
return nil, fmt.Errorf("error generating oauth cookie suffix: %w", err)
}

Check warning on line 506 in internal/gatewayapi/securitypolicy.go

View check run for this annotation

Codecov / codecov/patch

internal/gatewayapi/securitypolicy.go#L505-L506 

Added lines #L505 - L506 were not covered by tests
suffix := strconv.Itoa(int(h.Sum32()))
zhaohuabing marked this conversation as resolved.
Show resolved Hide resolved

return &ir.OIDC{
Provider: *provider,
ClientID: oidc.ClientID,
Expand All @@ -502,6 +514,7 @@
RedirectURL: redirectURL,
RedirectPath: redirectPath,
LogoutPath: logoutPath,
CookieSuffix: suffix,
}, nil
}

Expand Down
2 changes: 2 additions & 0 deletions internal/gatewayapi/testdata/securitypolicy-with-oidc.out.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,7 @@ xdsIR:
oidc:
clientID: client2.oauth.foo.com
clientSecret: Y2xpZW50MTpzZWNyZXQK
cookieSuffix: "1667669650"
logoutPath: /foo/logout
provider:
authorizationEndpoint: https://oauth.foo.com/oauth2/v2/auth
Expand Down Expand Up @@ -261,6 +262,7 @@ xdsIR:
oidc:
clientID: client1.apps.googleusercontent.com
clientSecret: Y2xpZW50MTpzZWNyZXQK
cookieSuffix: "2003913538"
logoutPath: /bar/logout
provider:
authorizationEndpoint: https://accounts.google.com/o/oauth2/v2/auth
Expand Down
6 changes: 6 additions & 0 deletions internal/ir/xds.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -521,6 +521,12 @@ type OIDC struct {

// The path to log a user out, clearing their credential cookies.
LogoutPath string `json:"logoutPath,omitempty"`

// CookieSuffix will be added to the name of the cookies set by the oauth filter.
// Adding a suffix avoids multiple oauth filters from overwriting each other's cookies.
// These cookies are set by the oauth filter, including: BearerToken,
// OauthHMAC, OauthExpires, IdToken, and RefreshToken.
CookieSuffix string `json:"cookieSuffix,omitempty"`
}

type OIDCProvider struct {
Expand Down
7 changes: 7 additions & 0 deletions internal/xds/translator/oidc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,13 @@ func oauth2Config(route *ir.HTTPRoute) (*oauth2v3.OAuth2, error) {
SdsConfig: makeConfigSource(),
},
},
CookieNames: &oauth2v3.OAuth2Credentials_CookieNames{
BearerToken: fmt.Sprintf("BearerToken-%s", route.OIDC.CookieSuffix),
OauthHmac: fmt.Sprintf("OauthHMAC-%s", route.OIDC.CookieSuffix),
OauthExpires: fmt.Sprintf("OauthExpires-%s", route.OIDC.CookieSuffix),
IdToken: fmt.Sprintf("IdToken-%s", route.OIDC.CookieSuffix),
RefreshToken: fmt.Sprintf("RefreshToken-%s", route.OIDC.CookieSuffix),
},
},
// every OIDC provider supports basic auth
AuthType: oauth2v3.OAuth2Config_BASIC_AUTH,
Expand Down
2 changes: 2 additions & 0 deletions internal/xds/translator/testdata/in/xds-ir/oidc.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ http:
redirectURL: "https://www.example.com/foo/oauth2/callback"
redirectPath: "/foo/oauth2/callback"
logoutPath: "/foo/logout"
cookieSuffix: "1667669650"
- name: "second-route"
hostname: "*"
pathMatch:
Expand All @@ -54,3 +55,4 @@ http:
redirectURL: "https://www.example.com/bar/oauth2/callback"
redirectPath: "/bar/oauth2/callback"
logoutPath: "/bar/logout"
cookieSuffix: "2003913538"
12 changes: 12 additions & 0 deletions internal/xds/translator/testdata/out/xds-ir/oidc.listeners.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,12 @@
authorizationEndpoint: https://oauth.foo.com/oauth2/v2/auth
credentials:
clientId: client.oauth.foo.com
cookieNames:
bearerToken: BearerToken-1667669650
idToken: IdToken-1667669650
oauthExpires: OauthExpires-1667669650
oauthHmac: OauthHMAC-1667669650
refreshToken: RefreshToken-1667669650
hmacSecret:
name: first-route/oauth2/hmac_secret
sdsConfig:
Expand Down Expand Up @@ -62,6 +68,12 @@
authorizationEndpoint: https://oauth.bar.com/oauth2/v2/auth
credentials:
clientId: client.oauth.bar.com
cookieNames:
bearerToken: BearerToken-2003913538
idToken: IdToken-2003913538
oauthExpires: OauthExpires-2003913538
oauthHmac: OauthHMAC-2003913538
refreshToken: RefreshToken-2003913538
hmacSecret:
name: second-route/oauth2/hmac_secret
sdsConfig:
Expand Down
Loading