From 5eb04402be526f32862c80f48b34b123b08046de Mon Sep 17 00:00:00 2001 From: Christian Banse Date: Mon, 10 Feb 2025 01:45:22 +0100 Subject: [PATCH 1/2] Implementing validation of multiple audiences This PR implements a validation option to validate multiple audiences. It takes ideas from #426 and #427, but implements a sleaker and cleaner design. Fixes #342 --- map_claims_test.go | 61 +++++++++++++++++++++++++--------------------- parser_option.go | 25 ++++++++++++++++--- validator.go | 39 ++++++++++++++++++++++------- validator_test.go | 26 +++++++++++--------- 4 files changed, 98 insertions(+), 53 deletions(-) diff --git a/map_claims_test.go b/map_claims_test.go index 034173d2..d78b30ec 100644 --- a/map_claims_test.go +++ b/map_claims_test.go @@ -10,56 +10,61 @@ func TestVerifyAud(t *testing.T) { var nilListInterface []interface{} var intListInterface interface{} = []int{1, 2, 3} type test struct { - Name string - MapClaims MapClaims - Expected bool - Comparison string - Required bool + Name string + MapClaims MapClaims + Expected bool + Comparison []string + MatchAllAud bool + Required bool } tests := []test{ // Matching Claim in aud // Required = true - {Name: "String Aud matching required", MapClaims: MapClaims{"aud": "example.com"}, Expected: true, Required: true, Comparison: "example.com"}, - {Name: "[]String Aud with match required", MapClaims: MapClaims{"aud": []string{"example.com", "example.example.com"}}, Expected: true, Required: true, Comparison: "example.com"}, + {Name: "String Aud matching required", MapClaims: MapClaims{"aud": "example.com"}, Expected: true, Required: true, Comparison: []string{"example.com"}}, + {Name: "[]String Aud with match required", MapClaims: MapClaims{"aud": []string{"example.com", "example.example.com"}}, Expected: true, Required: true, Comparison: []string{"example.com"}}, + {Name: "[]String Aud with []match any required", MapClaims: MapClaims{"aud": []string{"example.com", "example.example.com"}}, Expected: true, Required: true, Comparison: []string{"example.com", "auth.example.com"}}, + {Name: "[]String Aud with []match all required", MapClaims: MapClaims{"aud": []string{"example.com", "example.example.com"}}, Expected: true, Required: true, Comparison: []string{"example.com", "example.example.com"}, MatchAllAud: true}, // Required = false - {Name: "String Aud with match not required", MapClaims: MapClaims{"aud": "example.com"}, Expected: true, Required: false, Comparison: "example.com"}, - {Name: "Empty String Aud with match not required", MapClaims: MapClaims{}, Expected: true, Required: false, Comparison: "example.com"}, - {Name: "Empty String Aud with match not required", MapClaims: MapClaims{"aud": ""}, Expected: true, Required: false, Comparison: "example.com"}, - {Name: "Nil String Aud with match not required", MapClaims: MapClaims{"aud": nil}, Expected: true, Required: false, Comparison: "example.com"}, + {Name: "String Aud with match not required", MapClaims: MapClaims{"aud": "example.com"}, Expected: true, Required: false, Comparison: []string{"example.com"}}, + {Name: "Empty String Aud with match not required", MapClaims: MapClaims{}, Expected: true, Required: false, Comparison: []string{"example.com"}}, + {Name: "Empty String Aud with match not required", MapClaims: MapClaims{"aud": ""}, Expected: true, Required: false, Comparison: []string{"example.com"}}, + {Name: "Nil String Aud with match not required", MapClaims: MapClaims{"aud": nil}, Expected: true, Required: false, Comparison: []string{"example.com"}}, - {Name: "[]String Aud with match not required", MapClaims: MapClaims{"aud": []string{"example.com", "example.example.com"}}, Expected: true, Required: false, Comparison: "example.com"}, - {Name: "Empty []String Aud with match not required", MapClaims: MapClaims{"aud": []string{}}, Expected: true, Required: false, Comparison: "example.com"}, + {Name: "[]String Aud with match not required", MapClaims: MapClaims{"aud": []string{"example.com", "example.example.com"}}, Expected: true, Required: false, Comparison: []string{"example.com"}}, + {Name: "Empty []String Aud with match not required", MapClaims: MapClaims{"aud": []string{}}, Expected: true, Required: false, Comparison: []string{"example.com"}}, // Non-Matching Claim in aud // Required = true - {Name: "String Aud without match required", MapClaims: MapClaims{"aud": "not.example.com"}, Expected: false, Required: true, Comparison: "example.com"}, - {Name: "Empty String Aud without match required", MapClaims: MapClaims{"aud": ""}, Expected: false, Required: true, Comparison: "example.com"}, - {Name: "[]String Aud without match required", MapClaims: MapClaims{"aud": []string{"not.example.com", "example.example.com"}}, Expected: false, Required: true, Comparison: "example.com"}, - {Name: "Empty []String Aud without match required", MapClaims: MapClaims{"aud": []string{""}}, Expected: false, Required: true, Comparison: "example.com"}, - {Name: "String Aud without match not required", MapClaims: MapClaims{"aud": "not.example.com"}, Expected: false, Required: true, Comparison: "example.com"}, - {Name: "Empty String Aud without match not required", MapClaims: MapClaims{"aud": ""}, Expected: false, Required: true, Comparison: "example.com"}, - {Name: "[]String Aud without match not required", MapClaims: MapClaims{"aud": []string{"not.example.com", "example.example.com"}}, Expected: false, Required: true, Comparison: "example.com"}, + {Name: "String Aud without match required", MapClaims: MapClaims{"aud": "not.example.com"}, Expected: false, Required: true, Comparison: []string{"example.com"}}, + {Name: "Empty String Aud without match required", MapClaims: MapClaims{"aud": ""}, Expected: false, Required: true, Comparison: []string{"example.com"}}, + {Name: "[]String Aud without match required", MapClaims: MapClaims{"aud": []string{"not.example.com", "example.example.com"}}, Expected: false, Required: true, Comparison: []string{"example.com"}}, + {Name: "Empty []String Aud without match required", MapClaims: MapClaims{"aud": []string{""}}, Expected: false, Required: true, Comparison: []string{"example.com"}}, + {Name: "String Aud without match not required", MapClaims: MapClaims{"aud": "not.example.com"}, Expected: false, Required: true, Comparison: []string{"example.com"}}, + {Name: "Empty String Aud without match not required", MapClaims: MapClaims{"aud": ""}, Expected: false, Required: true, Comparison: []string{"example.com"}}, + {Name: "[]String Aud without match not required", MapClaims: MapClaims{"aud": []string{"not.example.com", "example.example.com"}}, Expected: false, Required: true, Comparison: []string{"example.com"}}, // Required = false - {Name: "Empty []String Aud without match required", MapClaims: MapClaims{"aud": []string{""}}, Expected: true, Required: false, Comparison: "example.com"}, + {Name: "Empty []String Aud without match required", MapClaims: MapClaims{"aud": []string{""}}, Expected: true, Required: false, Comparison: []string{"example.com"}}, // []interface{} - {Name: "Empty []interface{} Aud without match required", MapClaims: MapClaims{"aud": nilListInterface}, Expected: true, Required: false, Comparison: "example.com"}, - {Name: "[]interface{} Aud with match required", MapClaims: MapClaims{"aud": []interface{}{"a", "foo", "example.com"}}, Expected: true, Required: true, Comparison: "example.com"}, - {Name: "[]interface{} Aud with match but invalid types", MapClaims: MapClaims{"aud": []interface{}{"a", 5, "example.com"}}, Expected: false, Required: true, Comparison: "example.com"}, - {Name: "[]interface{} Aud int with match required", MapClaims: MapClaims{"aud": intListInterface}, Expected: false, Required: true, Comparison: "example.com"}, + {Name: "Empty []interface{} Aud without match required", MapClaims: MapClaims{"aud": nilListInterface}, Expected: true, Required: false, Comparison: []string{"example.com"}}, + {Name: "[]interface{} Aud with match required", MapClaims: MapClaims{"aud": []interface{}{"a", "foo", "example.com"}}, Expected: true, Required: true, Comparison: []string{"example.com"}}, + {Name: "[]interface{} Aud with match but invalid types", MapClaims: MapClaims{"aud": []interface{}{"a", 5, "example.com"}}, Expected: false, Required: true, Comparison: []string{"example.com"}}, + {Name: "[]interface{} Aud int with match required", MapClaims: MapClaims{"aud": intListInterface}, Expected: false, Required: true, Comparison: []string{"example.com"}}, // interface{} - {Name: "Empty interface{} Aud without match not required", MapClaims: MapClaims{"aud": nilInterface}, Expected: true, Required: false, Comparison: "example.com"}, + {Name: "Empty interface{} Aud without match not required", MapClaims: MapClaims{"aud": nilInterface}, Expected: true, Required: false, Comparison: []string{"example.com"}}, } for _, test := range tests { t.Run(test.Name, func(t *testing.T) { var opts []ParserOption - if test.Required { - opts = append(opts, WithAudience(test.Comparison)) + if test.Required && test.MatchAllAud { + opts = append(opts, WithAllAudiences(test.Comparison...)) + } else if test.Required { + opts = append(opts, WithAudience(test.Comparison...)) } validator := NewValidator(opts...) diff --git a/parser_option.go b/parser_option.go index 88a780fb..43157355 100644 --- a/parser_option.go +++ b/parser_option.go @@ -66,20 +66,37 @@ func WithExpirationRequired() ParserOption { } } -// WithAudience configures the validator to require the specified audience in -// the `aud` claim. Validation will fail if the audience is not listed in the -// token or the `aud` claim is missing. +// WithAudience configures the validator to require any of the specified +// audiences in the `aud` claim. Validation will fail if the audience is not +// listed in the token or the `aud` claim is missing. // // NOTE: While the `aud` claim is OPTIONAL in a JWT, the handling of it is // application-specific. Since this validation API is helping developers in // writing secure application, we decided to REQUIRE the existence of the claim, // if an audience is expected. -func WithAudience(aud string) ParserOption { +func WithAudience(aud ...string) ParserOption { return func(p *Parser) { p.validator.expectedAud = aud } } +// WithAllAudiences configures the validator to require all the specified +// audiences in the `aud` claim. Validation will fail if the specified audiences +// are not listed in the token or the `aud` claim is missing. Duplicates within +// the list are de-duplicated since internally, we use a map to look up the +// audiences. +// +// NOTE: While the `aud` claim is OPTIONAL in a JWT, the handling of it is +// application-specific. Since this validation API is helping developers in +// writing secure application, we decided to REQUIRE the existence of the claim, +// if an audience is expected. +func WithAllAudiences(aud ...string) ParserOption { + return func(p *Parser) { + p.validator.expectedAud = aud + p.validator.expectAllAud = true + } +} + // WithIssuer configures the validator to require the specified issuer in the // `iss` claim. Validation will fail if a different issuer is specified in the // token or the `iss` claim is missing. diff --git a/validator.go b/validator.go index 008ecd87..1686e67e 100644 --- a/validator.go +++ b/validator.go @@ -1,7 +1,6 @@ package jwt import ( - "crypto/subtle" "fmt" "time" ) @@ -52,8 +51,12 @@ type Validator struct { verifyIat bool // expectedAud contains the audience this token expects. Supplying an empty - // string will disable aud checking. - expectedAud string + // slice will disable aud checking. + expectedAud []string + + // expectAllAud specifies whether all expected audiences must be present in + // the token. If false, only one of the expected audiences must be present. + expectAllAud bool // expectedIss contains the issuer this token expects. Supplying an empty // string will disable iss checking. @@ -120,8 +123,8 @@ func (v *Validator) Validate(claims Claims) error { } // If we have an expected audience, we also require the audience claim - if v.expectedAud != "" { - if err = v.verifyAudience(claims, v.expectedAud, true); err != nil { + if len(v.expectedAud) > 0 { + if err = v.verifyAudience(claims, v.expectedAud, v.expectAllAud, true); err != nil { errs = append(errs, err) } } @@ -226,7 +229,7 @@ func (v *Validator) verifyNotBefore(claims Claims, cmp time.Time, required bool) // // 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) verifyAudience(claims Claims, cmp string, required bool) error { +func (v *Validator) verifyAudience(claims Claims, cmp []string, expectAllAud bool, required bool) error { aud, err := claims.GetAudience() if err != nil { return err @@ -237,16 +240,34 @@ func (v *Validator) verifyAudience(claims Claims, cmp string, required bool) err } // use a var here to keep constant time compare when looping over a number of claims - result := false + matching := make(map[string]bool, 0) + + // build a matching hashmap out of the expected aud + for _, expected := range cmp { + matching[expected] = false + } + // compare the expected aud with the actual aud in a constant time manner by looping over all actual values var stringClaims string for _, a := range aud { - if subtle.ConstantTimeCompare([]byte(a), []byte(cmp)) != 0 { - result = true + _, ok := matching[a] + if ok { + matching[a] = true } + stringClaims = stringClaims + a } + // check if all expected auds are present + result := true + for _, match := range matching { + if !expectAllAud && match { + break + } else if !match { + result = false + } + } + // case where "" is sent in one or many aud claims if stringClaims == "" { return errorIfRequired(required, "aud") diff --git a/validator_test.go b/validator_test.go index 08a6bd71..df08a539 100644 --- a/validator_test.go +++ b/validator_test.go @@ -22,12 +22,13 @@ func (m MyCustomClaims) Validate() error { func Test_Validator_Validate(t *testing.T) { type fields struct { - leeway time.Duration - timeFunc func() time.Time - verifyIat bool - expectedAud string - expectedIss string - expectedSub string + leeway time.Duration + timeFunc func() time.Time + verifyIat bool + expectedAud []string + expectAllAud bool + expectedIss string + expectedSub string } type args struct { claims Claims @@ -72,12 +73,13 @@ func Test_Validator_Validate(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { v := &Validator{ - leeway: tt.fields.leeway, - timeFunc: tt.fields.timeFunc, - verifyIat: tt.fields.verifyIat, - expectedAud: tt.fields.expectedAud, - expectedIss: tt.fields.expectedIss, - expectedSub: tt.fields.expectedSub, + leeway: tt.fields.leeway, + timeFunc: tt.fields.timeFunc, + verifyIat: tt.fields.verifyIat, + expectedAud: tt.fields.expectedAud, + expectAllAud: tt.fields.expectAllAud, + expectedIss: tt.fields.expectedIss, + expectedSub: tt.fields.expectedSub, } if err := v.Validate(tt.args.claims); (err != nil) && !errors.Is(err, tt.wantErr) { t.Errorf("validator.Validate() error = %v, wantErr %v", err, tt.wantErr) From 289a4b12233fe586f952542f6f8bca5361ed6c10 Mon Sep 17 00:00:00 2001 From: Christian Banse Date: Mon, 10 Feb 2025 01:52:46 +0100 Subject: [PATCH 2/2] Copy loop variable --- validator.go | 1 + 1 file changed, 1 insertion(+) diff --git a/validator.go b/validator.go index 1686e67e..184c833c 100644 --- a/validator.go +++ b/validator.go @@ -250,6 +250,7 @@ func (v *Validator) verifyAudience(claims Claims, cmp []string, expectAllAud boo // compare the expected aud with the actual aud in a constant time manner by looping over all actual values var stringClaims string for _, a := range aud { + a := a _, ok := matching[a] if ok { matching[a] = true