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 all 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
8 changes: 8 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,12 @@
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

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

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

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

View check run for this annotation

Codecov / codecov/patch

internal/gatewayapi/securitypolicy.go#L500-L501 

Added lines #L500 - L501 were not covered by tests
suffix := fmt.Sprintf("%X", h.Sum32())

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

Expand Down
1 change: 1 addition & 0 deletions internal/gatewayapi/testdata/securitypolicy-with-oidc-invalid-issuer.in.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ securityPolicies:
metadata:
namespace: default
name: policy-non-exist-secretRef
uid: b8284d0f-de82-4c65-b204-96a0d3f258a1
spec:
targetRef:
group: gateway.networking.k8s.io
Expand Down
1 change: 1 addition & 0 deletions internal/gatewayapi/testdata/securitypolicy-with-oidc-invalid-issuer.out.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ securityPolicies:
creationTimestamp: null
name: policy-non-exist-secretRef
namespace: default
uid: b8284d0f-de82-4c65-b204-96a0d3f258a1
spec:
oidc:
clientID: client1.apps.foo.bar.com
Expand Down
2 changes: 2 additions & 0 deletions internal/gatewayapi/testdata/securitypolicy-with-oidc-invalid-secretref.in.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ securityPolicies:
metadata:
namespace: default
name: policy-non-exist-secretRef
uid: b8284d0f-de82-4c65-b204-96a0d3f258a1
spec:
targetRef:
group: gateway.networking.k8s.io
Expand All @@ -81,6 +82,7 @@ securityPolicies:
metadata:
namespace: default
name: policy-no-referenceGrant
uid: 08335a80-83ba-4592-888f-6ac0bba44ce4
spec:
targetRef:
group: gateway.networking.k8s.io
Expand Down
2 changes: 2 additions & 0 deletions internal/gatewayapi/testdata/securitypolicy-with-oidc-invalid-secretref.out.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ securityPolicies:
creationTimestamp: null
name: policy-non-exist-secretRef
namespace: default
uid: b8284d0f-de82-4c65-b204-96a0d3f258a1
spec:
oidc:
clientID: client1.apps.googleusercontent.com
Expand Down Expand Up @@ -200,6 +201,7 @@ securityPolicies:
creationTimestamp: null
name: policy-no-referenceGrant
namespace: default
uid: 08335a80-83ba-4592-888f-6ac0bba44ce4
spec:
oidc:
clientID: client1.apps.googleusercontent.com
Expand Down
2 changes: 2 additions & 0 deletions internal/gatewayapi/testdata/securitypolicy-with-oidc.in.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ securityPolicies:
metadata:
namespace: envoy-gateway
name: policy-for-gateway-discover-endpoints # This policy should attach httproute-2
uid: b8284d0f-de82-4c65-b204-96a0d3f258a1
spec:
targetRef:
group: gateway.networking.k8s.io
Expand All @@ -99,6 +100,7 @@ securityPolicies:
metadata:
namespace: default
name: policy-for-http-route # This policy should attach httproute-1
uid: 08335a80-83ba-4592-888f-6ac0bba44ce4
spec:
targetRef:
group: gateway.networking.k8s.io
Expand Down
4 changes: 4 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 @@ -139,6 +139,7 @@ securityPolicies:
creationTimestamp: null
name: policy-for-http-route
namespace: default
uid: 08335a80-83ba-4592-888f-6ac0bba44ce4
spec:
oidc:
clientID: client2.oauth.foo.com
Expand Down Expand Up @@ -174,6 +175,7 @@ securityPolicies:
creationTimestamp: null
name: policy-for-gateway-discover-endpoints
namespace: envoy-gateway
uid: b8284d0f-de82-4c65-b204-96a0d3f258a1
spec:
oidc:
clientID: client1.apps.googleusercontent.com
Expand Down Expand Up @@ -230,6 +232,7 @@ xdsIR:
oidc:
clientID: client2.oauth.foo.com
clientSecret: Y2xpZW50MTpzZWNyZXQK
cookieSuffix: 5F93C2E4
logoutPath: /foo/logout
provider:
authorizationEndpoint: https://oauth.foo.com/oauth2/v2/auth
Expand Down Expand Up @@ -261,6 +264,7 @@ xdsIR:
oidc:
clientID: client1.apps.googleusercontent.com
clientSecret: Y2xpZW50MTpzZWNyZXQK
cookieSuffix: B0A1B740
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: 5F93C2E4
- 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: B0A1B740
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-5F93C2E4
idToken: IdToken-5F93C2E4
oauthExpires: OauthExpires-5F93C2E4
oauthHmac: OauthHMAC-5F93C2E4
refreshToken: RefreshToken-5F93C2E4
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-B0A1B740
idToken: IdToken-B0A1B740
oauthExpires: OauthExpires-B0A1B740
oauthHmac: OauthHMAC-B0A1B740
refreshToken: RefreshToken-B0A1B740
hmacSecret:
name: second-route/oauth2/hmac_secret
sdsConfig:
Expand Down