From 5d38e685afe7592521d2fe6baa0f0b8a73615b88 Mon Sep 17 00:00:00 2001 From: Dimitrij Drus Date: Thu, 25 Jul 2024 19:07:55 +0200 Subject: [PATCH] fix: OAuth2 `iss` claim verification in JWT/OIDC authenticators when used with `metadata_endpoint` (#1658) --- Co-authored-by: Martin Koppehel Co-authored-by: Dimitrij Drus --- .../authenticators/jwt_authenticator.go | 10 +-- .../oauth2_introspection_authenticator.go | 10 +-- .../{expectations.go => expectation.go} | 20 ++--- ...pectations_test.go => expectation_test.go} | 75 +++++++++++-------- 4 files changed, 61 insertions(+), 54 deletions(-) rename internal/rules/mechanisms/oauth2/{expectations.go => expectation.go} (85%) rename internal/rules/mechanisms/oauth2/{expectations_test.go => expectation_test.go} (91%) diff --git a/internal/rules/mechanisms/authenticators/jwt_authenticator.go b/internal/rules/mechanisms/authenticators/jwt_authenticator.go index d8f6fd817..c8eaedd6f 100644 --- a/internal/rules/mechanisms/authenticators/jwt_authenticator.go +++ b/internal/rules/mechanisms/authenticators/jwt_authenticator.go @@ -208,9 +208,9 @@ func (a *jwtAuthenticator) WithConfig(config map[string]any) (Authenticator, err } type Config struct { - Assertions *oauth2.Expectation `mapstructure:"assertions" validate:"-"` - CacheTTL *time.Duration `mapstructure:"cache_ttl"` - AllowFallbackOnError *bool `mapstructure:"allow_fallback_on_error"` + Assertions oauth2.Expectation `mapstructure:"assertions" validate:"-"` + CacheTTL *time.Duration `mapstructure:"cache_ttl"` + AllowFallbackOnError *bool `mapstructure:"allow_fallback_on_error"` } var conf Config @@ -221,7 +221,7 @@ func (a *jwtAuthenticator) WithConfig(config map[string]any) (Authenticator, err return &jwtAuthenticator{ id: a.id, r: a.r, - a: conf.Assertions.Merge(&a.a), + a: conf.Assertions.Merge(a.a), ttl: x.IfThenElse(conf.CacheTTL != nil, conf.CacheTTL, a.ttl), sf: a.sf, ads: a.ads, @@ -314,7 +314,7 @@ func (a *jwtAuthenticator) verifyToken(ctx heimdall.Context, token *jwt.JSONWebT } // configured assertions take precedence over those available in the metadata - assertions := a.a.Merge(&oauth2.Expectation{ + assertions := a.a.Merge(oauth2.Expectation{ TrustedIssuers: []string{metadata.Issuer}, }) diff --git a/internal/rules/mechanisms/authenticators/oauth2_introspection_authenticator.go b/internal/rules/mechanisms/authenticators/oauth2_introspection_authenticator.go index b38bd907a..b4dc43b12 100644 --- a/internal/rules/mechanisms/authenticators/oauth2_introspection_authenticator.go +++ b/internal/rules/mechanisms/authenticators/oauth2_introspection_authenticator.go @@ -192,9 +192,9 @@ func (a *oauth2IntrospectionAuthenticator) WithConfig(rawConfig map[string]any) } type Config struct { - Assertions *oauth2.Expectation `mapstructure:"assertions"` - CacheTTL *time.Duration `mapstructure:"cache_ttl"` - AllowFallbackOnError *bool `mapstructure:"allow_fallback_on_error"` + Assertions oauth2.Expectation `mapstructure:"assertions"` + CacheTTL *time.Duration `mapstructure:"cache_ttl"` + AllowFallbackOnError *bool `mapstructure:"allow_fallback_on_error"` } var conf Config @@ -205,7 +205,7 @@ func (a *oauth2IntrospectionAuthenticator) WithConfig(rawConfig map[string]any) return &oauth2IntrospectionAuthenticator{ id: a.id, r: a.r, - a: conf.Assertions.Merge(&a.a), + a: conf.Assertions.Merge(a.a), sf: a.sf, ads: a.ads, ttl: x.IfThenElse(conf.CacheTTL != nil, conf.CacheTTL, a.ttl), @@ -299,7 +299,7 @@ func (a *oauth2IntrospectionAuthenticator) getSubjectInformation(ctx heimdall.Co } // configured assertions take precedence over those available in the metadata - assertions := a.a.Merge(&oauth2.Expectation{ + assertions := a.a.Merge(oauth2.Expectation{ TrustedIssuers: []string{metadata.Issuer}, }) diff --git a/internal/rules/mechanisms/oauth2/expectations.go b/internal/rules/mechanisms/oauth2/expectation.go similarity index 85% rename from internal/rules/mechanisms/oauth2/expectations.go rename to internal/rules/mechanisms/oauth2/expectation.go index 72310398e..ed3f49b8b 100644 --- a/internal/rules/mechanisms/oauth2/expectations.go +++ b/internal/rules/mechanisms/oauth2/expectation.go @@ -38,21 +38,17 @@ type Expectation struct { ValidityLeeway time.Duration `mapstructure:"validity_leeway"` } -func (e *Expectation) Merge(other *Expectation) Expectation { - if e == nil { - return *other - } - +func (e Expectation) Merge(other Expectation) Expectation { e.TrustedIssuers = x.IfThenElse(len(e.TrustedIssuers) != 0, e.TrustedIssuers, other.TrustedIssuers) e.ScopesMatcher = x.IfThenElse(e.ScopesMatcher != nil, e.ScopesMatcher, other.ScopesMatcher) e.Audiences = x.IfThenElse(len(e.Audiences) != 0, e.Audiences, other.Audiences) e.AllowedAlgorithms = x.IfThenElse(len(e.AllowedAlgorithms) != 0, e.AllowedAlgorithms, other.AllowedAlgorithms) e.ValidityLeeway = x.IfThenElse(e.ValidityLeeway != 0, e.ValidityLeeway, other.ValidityLeeway) - return *e + return e } -func (e *Expectation) AssertAlgorithm(alg string) error { +func (e Expectation) AssertAlgorithm(alg string) error { if !slices.Contains(e.AllowedAlgorithms, alg) { return errorchain.NewWithMessagef(ErrAssertion, "algorithm %s is not allowed", alg) } @@ -60,7 +56,7 @@ func (e *Expectation) AssertAlgorithm(alg string) error { return nil } -func (e *Expectation) AssertIssuer(issuer string) error { +func (e Expectation) AssertIssuer(issuer string) error { if !slices.Contains(e.TrustedIssuers, issuer) { return errorchain.NewWithMessagef(ErrAssertion, "issuer %s is not trusted", issuer) } @@ -68,7 +64,7 @@ func (e *Expectation) AssertIssuer(issuer string) error { return nil } -func (e *Expectation) AssertAudience(audience []string) error { +func (e Expectation) AssertAudience(audience []string) error { if len(e.Audiences) == 0 { return nil } @@ -80,7 +76,7 @@ func (e *Expectation) AssertAudience(audience []string) error { return nil } -func (e *Expectation) AssertValidity(notBefore, notAfter time.Time) error { +func (e Expectation) AssertValidity(notBefore, notAfter time.Time) error { leeway := int64(x.IfThenElse(e.ValidityLeeway != 0, e.ValidityLeeway, defaultLeeway).Seconds()) now := time.Now().Unix() nbf := notBefore.Unix() @@ -97,7 +93,7 @@ func (e *Expectation) AssertValidity(notBefore, notAfter time.Time) error { return nil } -func (e *Expectation) AssertIssuanceTime(issuedAt time.Time) error { +func (e Expectation) AssertIssuanceTime(issuedAt time.Time) error { leeway := x.IfThenElse(e.ValidityLeeway != 0, e.ValidityLeeway, defaultLeeway) // IssuedAt is optional but cannot be in the future. This is not required by the RFC, but @@ -109,4 +105,4 @@ func (e *Expectation) AssertIssuanceTime(issuedAt time.Time) error { return nil } -func (e *Expectation) AssertScopes(scopes []string) error { return e.ScopesMatcher.Match(scopes) } +func (e Expectation) AssertScopes(scopes []string) error { return e.ScopesMatcher.Match(scopes) } diff --git a/internal/rules/mechanisms/oauth2/expectations_test.go b/internal/rules/mechanisms/oauth2/expectation_test.go similarity index 91% rename from internal/rules/mechanisms/oauth2/expectations_test.go rename to internal/rules/mechanisms/oauth2/expectation_test.go index f007ca7a6..2650c38ec 100644 --- a/internal/rules/mechanisms/oauth2/expectations_test.go +++ b/internal/rules/mechanisms/oauth2/expectation_test.go @@ -524,35 +524,26 @@ func TestExpectationAssertScopes(t *testing.T) { } } -func TestMergeExpectations(t *testing.T) { +func TestExpectationMerge(t *testing.T) { t.Parallel() for _, tc := range []struct { uc string - target *Expectation - source *Expectation - assert func(t *testing.T, merged *Expectation, source *Expectation, target *Expectation) + target Expectation + source Expectation + assert func(t *testing.T, merged Expectation, source Expectation, target Expectation) }{ - { - uc: "with nil target", - source: &Expectation{}, - assert: func(t *testing.T, merged *Expectation, source *Expectation, _ *Expectation) { - t.Helper() - - require.Equal(t, source, merged) - }, - }, { uc: "with empty target", - source: &Expectation{ + source: Expectation{ ScopesMatcher: ExactScopeStrategyMatcher{}, Audiences: []string{"foo"}, TrustedIssuers: []string{"bar"}, AllowedAlgorithms: []string{"RS512"}, ValidityLeeway: 10 * time.Second, }, - target: &Expectation{}, - assert: func(t *testing.T, merged *Expectation, source *Expectation, _ *Expectation) { + target: Expectation{}, + assert: func(t *testing.T, merged Expectation, source Expectation, _ Expectation) { t.Helper() require.Equal(t, source, merged) @@ -560,15 +551,15 @@ func TestMergeExpectations(t *testing.T) { }, { uc: "with target having only scopes configured", - source: &Expectation{ + source: Expectation{ ScopesMatcher: ExactScopeStrategyMatcher{}, Audiences: []string{"foo"}, TrustedIssuers: []string{"bar"}, AllowedAlgorithms: []string{"RS512"}, ValidityLeeway: 10 * time.Second, }, - target: &Expectation{ScopesMatcher: HierarchicScopeStrategyMatcher{}}, - assert: func(t *testing.T, merged *Expectation, source *Expectation, target *Expectation) { + target: Expectation{ScopesMatcher: HierarchicScopeStrategyMatcher{}}, + assert: func(t *testing.T, merged Expectation, source Expectation, target Expectation) { t.Helper() assert.NotEqual(t, source, merged) @@ -582,18 +573,18 @@ func TestMergeExpectations(t *testing.T) { }, { uc: "with target having scopes and audience configured", - source: &Expectation{ + source: Expectation{ ScopesMatcher: ExactScopeStrategyMatcher{}, Audiences: []string{"foo"}, TrustedIssuers: []string{"bar"}, AllowedAlgorithms: []string{"RS512"}, ValidityLeeway: 10 * time.Second, }, - target: &Expectation{ + target: Expectation{ ScopesMatcher: HierarchicScopeStrategyMatcher{}, Audiences: []string{"baz"}, }, - assert: func(t *testing.T, merged *Expectation, source *Expectation, target *Expectation) { + assert: func(t *testing.T, merged Expectation, source Expectation, target Expectation) { t.Helper() assert.NotEqual(t, source, merged) @@ -608,19 +599,19 @@ func TestMergeExpectations(t *testing.T) { }, { uc: "with target having scopes, audience and trusted issuers configured", - source: &Expectation{ + source: Expectation{ ScopesMatcher: ExactScopeStrategyMatcher{}, Audiences: []string{"foo"}, TrustedIssuers: []string{"bar"}, AllowedAlgorithms: []string{"RS512"}, ValidityLeeway: 10 * time.Second, }, - target: &Expectation{ + target: Expectation{ ScopesMatcher: HierarchicScopeStrategyMatcher{}, Audiences: []string{"baz"}, TrustedIssuers: []string{"zab"}, }, - assert: func(t *testing.T, merged *Expectation, source *Expectation, target *Expectation) { + assert: func(t *testing.T, merged Expectation, source Expectation, target Expectation) { t.Helper() assert.NotEqual(t, source, merged) @@ -636,20 +627,20 @@ func TestMergeExpectations(t *testing.T) { }, { uc: "with target having scopes, audience, trusted issuers and allowed algorithms configured", - source: &Expectation{ + source: Expectation{ ScopesMatcher: ExactScopeStrategyMatcher{}, Audiences: []string{"foo"}, TrustedIssuers: []string{"bar"}, AllowedAlgorithms: []string{"RS512"}, ValidityLeeway: 10 * time.Second, }, - target: &Expectation{ + target: Expectation{ ScopesMatcher: HierarchicScopeStrategyMatcher{}, Audiences: []string{"baz"}, TrustedIssuers: []string{"zab"}, AllowedAlgorithms: []string{"BAR128"}, }, - assert: func(t *testing.T, merged *Expectation, source *Expectation, target *Expectation) { + assert: func(t *testing.T, merged Expectation, source Expectation, target Expectation) { t.Helper() assert.NotEqual(t, source, merged) @@ -666,21 +657,21 @@ func TestMergeExpectations(t *testing.T) { }, { uc: "with target having everything reconfigured", - source: &Expectation{ + source: Expectation{ ScopesMatcher: ExactScopeStrategyMatcher{}, Audiences: []string{"foo"}, TrustedIssuers: []string{"bar"}, AllowedAlgorithms: []string{"RS512"}, ValidityLeeway: 10 * time.Second, }, - target: &Expectation{ + target: Expectation{ ScopesMatcher: HierarchicScopeStrategyMatcher{}, Audiences: []string{"baz"}, TrustedIssuers: []string{"zab"}, AllowedAlgorithms: []string{"BAR128"}, ValidityLeeway: 20 * time.Minute, }, - assert: func(t *testing.T, merged *Expectation, source *Expectation, target *Expectation) { + assert: func(t *testing.T, merged Expectation, source Expectation, target Expectation) { t.Helper() assert.NotEqual(t, source, merged) @@ -702,7 +693,27 @@ func TestMergeExpectations(t *testing.T) { exp := tc.target.Merge(tc.source) // THEN - tc.assert(t, &exp, tc.source, tc.target) + tc.assert(t, exp, tc.source, tc.target) }) } } + +func TestExpectationMergeIdempotency(t *testing.T) { + t.Parallel() + + exp := Expectation{} + + exp.Merge(Expectation{ + ScopesMatcher: ExactScopeStrategyMatcher{}, + Audiences: []string{"foo"}, + TrustedIssuers: []string{"bar"}, + AllowedAlgorithms: []string{"RS512"}, + ValidityLeeway: 10 * time.Second, + }) + + assert.Nil(t, exp.ScopesMatcher) + assert.Empty(t, exp.Audiences) + assert.Empty(t, exp.TrustedIssuers) + assert.Empty(t, exp.AllowedAlgorithms) + assert.Equal(t, 0*time.Second, exp.ValidityLeeway) +}