HdonCr/Allow multiple audiences #427
Conversation
|
Hi! Looks like we have two PRs now for the same issue :) I will take some time after the holidays with @mfridman to see which direction we will follow. |
| @@ -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 | |||
There was a problem hiding this comment.
I wonder whether we could merge this with the expectedAud above
There was a problem hiding this comment.
See my comment here: https://github.com/golang-jwt/jwt/pull/427/files#r1948157329
| // | ||
| // 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.
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 verifyAudience in this as well - or rather merge the two since one is just a special case of the other.
There was a problem hiding this comment.
I agree. I would propose to remove the validation for single audiences, since the verifyAudiences already supports validating a single audience by passing a slice with a single element.
I have removed the expectedAud field and verifyAudiences functions and everything related (including tests) to merge verifying one or multiple audiences. Additionally, I have added deduplication for auds and cmps and added documentation.
Note that this is a breaking change.
| for _, a := range aud { | ||
|
|
||
| // Perform constant time comparison | ||
| result := subtle.ConstantTimeCompare([]byte(a), []byte(cmp)) != 0 |
There was a problem hiding this comment.
quite a large duplicate code fragment, this is essentially what the existing verifyAudience does. why not use it?
There was a problem hiding this comment.
See my comment here: https://github.com/golang-jwt/jwt/pull/427/files#r1948157329
| for _, cmp := range cmps { | ||
|
|
||
| // Perform constant time comparison | ||
| result := subtle.ConstantTimeCompare([]byte(a), []byte(cmp)) != 0 |
There was a problem hiding this comment.
See my comment here: https://github.com/golang-jwt/jwt/pull/427/files#r1948157329
|
@oxisto, I just made some minor changes. Will look into the bigger changes soon (possibly merging |
| var stringClaims string | ||
|
|
||
| // If matchAllAuds is true, check if all the cmps matches any of the aud | ||
| if matchAllAuds { |
There was a problem hiding this comment.
@oxisto, I am not sure whether this verifyAudiences function is constante time safe. Could you clarify how relevant that is? If so, I can add testing and implement this if not.
| // the wrong type, an ErrTokenUnverifiable error will be returned. | ||
| func (v *Validator) verifyAudiences(claims Claims, cmps []string, required bool, matchAllAuds bool) error { | ||
|
|
||
| // Get the audience claim(s) from the token |
There was a problem hiding this comment.
@oxisto, regarding your first comment here: https://github.com/golang-jwt/jwt/pull/427/files#r1917080944
I agree that this function is quite complex. I think this is the result of the option to match all auds. Deduplicating both situations is difficult, making the function very long due to two separate flows.
Additionally, the situation in which at least one cmp must match an aud is relatively simple. However, all cmps matching all auds is more difficult to implement.
I have added documentation in an attempt to make the function more legible. If required, I can look into simplifying it further.
Allow for multiple expected audiences as requested in issue 342.