-
Notifications
You must be signed in to change notification settings - Fork 361
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
HdonCr/Allow multiple audiences #427
base: main
Are you sure you want to change the base?
Changes from 3 commits
8f964e5
ca00a1f
ab835f5
f1c5576
ca1d34c
e107f51
8e711bc
de939e0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,6 +55,13 @@ type Validator struct { | |
// string will disable aud checking. | ||
expectedAud string | ||
|
||
//expectedAuds contains the audiences this token expects. Supplying an empty | ||
// []string will disable auds checking. | ||
expectedAuds []string | ||
|
||
// expectedAudsMatchAll specifies whether all expected audiences must match all auds from claim | ||
expectedAudsMatchAll bool | ||
hdonCr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// expectedIss contains the issuer this token expects. Supplying an empty | ||
// string will disable iss checking. | ||
expectedIss string | ||
|
@@ -126,6 +133,13 @@ func (v *Validator) Validate(claims Claims) error { | |
} | ||
} | ||
|
||
// If we have expected audiences, we also require the audiences claim | ||
if len(v.expectedAuds) != 0 { | ||
hdonCr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if err := v.verifyAudiences(claims, v.expectedAuds, true, v.expectedAudsMatchAll); err != nil { | ||
errs = append(errs, err) | ||
} | ||
} | ||
|
||
// If we have an expected issuer, we also require the issuer claim | ||
if v.expectedIss != "" { | ||
if err = v.verifyIssuer(claims, v.expectedIss, true); err != nil { | ||
|
@@ -255,6 +269,102 @@ func (v *Validator) verifyAudience(claims Claims, cmp string, required bool) err | |
return errorIfFalse(result, ErrTokenInvalidAudience) | ||
} | ||
|
||
// verifyAudiences compares the aud claim against cmps. | ||
// If matchAllAuds is true, all cmps must match a aud. | ||
// If matchAllAuds is false, at least one cmp must match a aud. | ||
// | ||
// If matchAllAuds is true and aud length does not match cmps length, an ErrTokenInvalidAudience error will be returned. | ||
// Note that this does not account for any duplicate aud or cmps | ||
// | ||
// If aud is not set or an empty list, it will succeed if the claim is not required, | ||
// otherwise ErrTokenRequiredClaimMissing will be returned. | ||
// | ||
// Additionally, if any error occurs while retrieving the claim, e.g., when its | ||
// the wrong type, an ErrTokenUnverifiable error will be returned. | ||
func (v *Validator) verifyAudiences(claims Claims, cmps []string, required bool, matchAllAuds bool) error { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function seems to be very complex, I wonder if we could streamline this implementation a little bit. Furthermore, it should be possible to include the previous implementation of |
||
|
||
aud, err := claims.GetAudience() | ||
if err != nil { | ||
return err | ||
} | ||
|
||
if len(aud) == 0 { | ||
return errorIfRequired(required, "aud") | ||
} | ||
|
||
var stringClaims string | ||
|
||
// If matchAllAuds is true, check if all the cmps matches any of the aud | ||
if matchAllAuds { | ||
|
||
// cmps and aud length should match if matchAllAuds is true | ||
// Note that this does not account for possible duplicates | ||
if len(cmps) != len(aud) { | ||
return errorIfFalse(false, ErrTokenInvalidAudience) | ||
} | ||
|
||
// Check all cmps values | ||
for _, cmp := range cmps { | ||
matchFound := false | ||
for _, a := range aud { | ||
|
||
// Perform constant time comparison | ||
result := subtle.ConstantTimeCompare([]byte(a), []byte(cmp)) != 0 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. quite a large duplicate code fragment, this is essentially what the existing |
||
|
||
stringClaims = stringClaims + a | ||
|
||
// If a match is found, set matchFound to true and break out of inner aud loop and continue to next cmp | ||
if result { | ||
matchFound = true | ||
break | ||
} | ||
} | ||
|
||
// If no match was found for the current cmp, return a ErrTokenInvalidAudience error | ||
if !matchFound { | ||
return ErrTokenInvalidAudience | ||
} | ||
} | ||
|
||
} else { | ||
// if matchAllAuds is false, check if any of the cmps matches any of the aud | ||
|
||
matchFound := false | ||
|
||
// Label to break out of both loops if a match is found | ||
outer: | ||
|
||
// Check all aud values | ||
for _, a := range aud { | ||
for _, cmp := range cmps { | ||
|
||
// Perform constant time comparison | ||
result := subtle.ConstantTimeCompare([]byte(a), []byte(cmp)) != 0 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see above |
||
|
||
stringClaims = stringClaims + a | ||
|
||
// If a match is found, break out of both loops and finish comparison | ||
if result { | ||
matchFound = true | ||
break outer | ||
} | ||
} | ||
} | ||
|
||
// If no match was found for any cmp, return an error | ||
if !matchFound { | ||
return errorIfFalse(false, ErrTokenInvalidAudience) | ||
} | ||
} | ||
|
||
// case where "" is sent in one or many aud claims | ||
if stringClaims == "" { | ||
return errorIfRequired(required, "aud") | ||
} | ||
|
||
return nil | ||
} | ||
|
||
// verifyIssuer compares the iss claim in claims against cmp. | ||
// | ||
// If iss is not set, it will succeed if the claim is not required, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether we could merge this with the
expectedAud
above