Skip to content

Commit

Permalink
fix: OAuth2 iss claim verification in JWT/OIDC authenticators when …
Browse files Browse the repository at this point in the history 
…used with `metadata_endpoint` (#1658)

---
Co-authored-by: Martin Koppehel <martin@mko.dev>
Co-authored-by: Dimitrij Drus <dadrus@gmx.de>
  • Loading branch information
@dadrus
dadrus authored Jul 25, 2024
1 parent 8e4d74b commit 5d38e68
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 54 deletions.
10 changes: 5 additions & 5 deletions internal/rules/mechanisms/authenticators/jwt_authenticator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Expand Down Expand Up @@ -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},
})

Expand Down
10 changes: 5 additions & 5 deletions internal/rules/mechanisms/authenticators/oauth2_introspection_authenticator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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),
Expand Down Expand Up @@ -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},
})

Expand Down
20 changes: 8 additions & 12 deletions ...l/rules/mechanisms/oauth2/expectations.go → ...al/rules/mechanisms/oauth2/expectation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,37 +38,33 @@ 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)
}

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)
}

return nil
}

func (e *Expectation) AssertAudience(audience []string) error {
func (e Expectation) AssertAudience(audience []string) error {
if len(e.Audiences) == 0 {
return nil
}
Expand All @@ -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()
Expand All @@ -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
Expand All @@ -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) }
75 changes: 43 additions & 32 deletions ...es/mechanisms/oauth2/expectations_test.go → ...les/mechanisms/oauth2/expectation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -524,51 +524,42 @@ 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)
},
},
{
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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
}

0 comments on commit 5d38e68

Please sign in to comment.