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

feature: add ability to match any audience via '*' #6132

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+19 −3
Open

feature: add ability to match any audience via '*' #6132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 4 additions & 2 deletions flyteadmin/auth/authzserver/claims_verifier.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,17 @@ import (
func verifyClaims(expectedAudience sets.String, claimsRaw map[string]interface{}) (interfaces.IdentityContext, error) {
claims := jwtx.ParseMapStringInterfaceClaims(claimsRaw)

audience := ""
foundAudIndex := -1
for audIndex, aud := range claims.Audience {
if expectedAudience.Has(aud) {
foundAudIndex = audIndex
audience = aud
break
}
}

if foundAudIndex < 0 {
if foundAudIndex < 0 && !expectedAudience.Has("*") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider handling empty audience lists

Consider checking for empty audience list before checking for wildcard match. If claims.Audience is empty and expectedAudience has *, we may want to allow that case.

Code suggestion
Check the AI-generated fix before applying
Suggested change
if foundAudIndex < 0 && !expectedAudience.Has("*") {
if foundAudIndex < 0 {
if len(claims.Audience) == 0 && expectedAudience.Has("*") {
return auth.NewIdentityContext("", claims.Subject, clientID, claims.IssuedAt, scopes, userInfo, claimsRaw)
} else if !expectedAudience.Has("*") {

Code Review Run #34b05e

Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged

Copy link
Author

Choose a reason for hiding this comment

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

This case is covered and there is a test demonstrating this behavior.

return nil, fmt.Errorf("invalid audience [%v], wanted [%v]", claims, expectedAudience)
}

Expand Down Expand Up @@ -71,5 +73,5 @@ func verifyClaims(expectedAudience sets.String, claimsRaw map[string]interface{}
scopes.Insert(auth.ScopeAll)
}

return auth.NewIdentityContext(claims.Audience[foundAudIndex], claims.Subject, clientID, claims.IssuedAt, scopes, userInfo, claimsRaw)
return auth.NewIdentityContext(audience, claims.Subject, clientID, claims.IssuedAt, scopes, userInfo, claimsRaw)
}
14 changes: 14 additions & 0 deletions flyteadmin/auth/authzserver/claims_verifier_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,20 @@ func Test_verifyClaims(t *testing.T) {
assert.Error(t, err)
})

t.Run("ExpectedAudience == '*' allows any aud", func(t *testing.T) {
identityCtx, err := verifyClaims(sets.NewString("*"), map[string]interface{}{
"aud": []string{"https://myserver"},
})
assert.NoError(t, err)
assert.Equal(t, "", identityCtx.Audience())
})

t.Run("ExpectedAudience == '*' allows missing aud", func(t *testing.T) {
identityCtx, err := verifyClaims(sets.NewString("*"), map[string]interface{}{})
assert.NoError(t, err)
assert.Equal(t, "", identityCtx.Audience())
})

t.Run("All filled", func(t *testing.T) {
identityCtx, err := verifyClaims(sets.NewString("https://myserver"), map[string]interface{}{
"aud": []string{"https://myserver"},
Expand Down
2 changes: 1 addition & 1 deletion flyteadmin/auth/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ type ExternalAuthorizationServer struct {
// BaseURL should be the base url of the authorization server that you are trying to hit. With Okta for instance, it will look something like https://company.okta.com/oauth2/abcdef123456789/
// If not provided, the OpenID.BaseURL will be assumed instead.
BaseURL config.URL `json:"baseUrl" pflag:",This should be the base url of the authorization server that you are trying to hit. With Okta for instance, it will look something like https://company.okta.com/oauth2/abcdef123456789/"`
AllowedAudience []string `json:"allowedAudience" pflag:",Optional: A list of allowed audiences. If not provided, the audience is expected to be the public Uri of the service."`
AllowedAudience []string `json:"allowedAudience" pflag:",Optional: A list of allowed audiences. If not provided, the audience is expected to be the public Uri of the service. '*' can be used to allow any audience or missing aud claim."`
MetadataEndpointURL config.URL `json:"metadataUrl" pflag:",Optional: If the server doesn't support /.well-known/oauth-authorization-server, you can set a custom metadata url here.'"`
// HTTPProxyURL allows operators to access external OAuth2 servers using an external HTTP Proxy
HTTPProxyURL config.URL `json:"httpProxyURL" pflag:",OPTIONAL: HTTP Proxy to be used for OAuth requests."`
Expand Down