diff --git a/pkg/auth/awssts/role_mapper.go b/pkg/auth/awssts/role_mapper.go index 0fbad5ef80..4223d0c24c 100644 --- a/pkg/auth/awssts/role_mapper.go +++ b/pkg/auth/awssts/role_mapper.go @@ -92,10 +92,14 @@ type compiledMapping struct { // Claim-based mappings bind claim_value and role_claim_key as variables so that // user-supplied values are never interpolated into CEL expression strings, // eliminating CEL injection by design. Matcher-based mappings only need claims. -func (cm *compiledMapping) evalContext(claims map[string]any, roleClaim string) map[string]any { +func (cm *compiledMapping) evalContext( + claims map[string]any, + normalizedClaims map[string]any, + roleClaim string, +) map[string]any { if cm.claimValue != "" { return map[string]any{ - "claims": claims, + "claims": normalizedClaims, "claim_value": cm.claimValue, "role_claim_key": roleClaim, } @@ -103,6 +107,45 @@ func (cm *compiledMapping) evalContext(claims map[string]any, roleClaim string) return map[string]any{"claims": claims} } +// normalizeRoleClaim returns a copy of claims whose role claim value is +// normalized to a list so the claim binding expression +// `claim_value in claims[role_claim_key]` performs exact element membership +// regardless of how the IdP serializes the claim: a single string is wrapped +// into a one-element list, and a list passes through unchanged. +// +// The documented contract (config.go) treats the role claim as a value list, so +// any other shape is an unsupported deviation and fails closed: without this +// guard, a string-typed claim made CEL `in` raise a "no such overload" error +// that SelectRole swallowed as a non-match (silently granting FallbackRoleArn), +// and an object-typed claim made `in` test map-key membership (spuriously +// matching when the configured value was a key). +func normalizeRoleClaim(claims map[string]any, roleClaim string) (map[string]any, error) { + v, ok := claims[roleClaim] + if !ok { + // A missing role claim is a normal "no match", not an error: index the + // expression against an empty list so it evaluates false and SelectRole + // falls back as it always did. + return cloneClaimsWithRoleClaim(claims, roleClaim, []any{}), nil + } + switch t := v.(type) { + case string: + return cloneClaimsWithRoleClaim(claims, roleClaim, []any{t}), nil + case []any, []string: + return claims, nil + default: + return nil, fmt.Errorf("role claim %q has unsupported value type %T (want string or list of strings)", roleClaim, v) + } +} + +func cloneClaimsWithRoleClaim(claims map[string]any, roleClaim string, value any) map[string]any { + clone := make(map[string]any, len(claims)+1) + for key, claim := range claims { + clone[key] = claim + } + clone[roleClaim] = value + return clone +} + // RoleMapper handles mapping JWT claims to IAM roles with priority-based selection. // It uses CEL expressions for flexible claim matching. type RoleMapper struct { @@ -169,21 +212,54 @@ func NewRoleMapper(cfg *Config) (*RoleMapper, error) { func (rm *RoleMapper) SelectRole(claims map[string]any) (string, error) { // If no role mappings configured, use default role if len(rm.mappings) == 0 { - if rm.config.FallbackRoleArn == "" { - return "", ErrMissingRoleConfig - } - return rm.config.FallbackRoleArn, nil + return rm.fallbackRole(ErrMissingRoleConfig) } // Find all matching mappings roleClaim := rm.config.GetRoleClaim() + normalizedClaims := claims + var normalizationErr error + for _, mapping := range rm.mappings { + if mapping.claimValue != "" { + normalizedClaims, normalizationErr = normalizeRoleClaim(claims, roleClaim) + break + } + } var matches []compiledMapping + var claimMappingErr error for _, mapping := range rm.mappings { - match, err := mapping.expr.EvaluateBool(mapping.evalContext(claims, roleClaim)) + if mapping.claimValue != "" && normalizationErr != nil { + // Keep evaluating other mappings: a valid matcher mapping may have + // already matched. If none do, fail closed below rather than granting + // the fallback role for an unsupported claim shape. + slog.Warn("role claim has unsupported shape, failing closed", + "role_arn", mapping.roleArn, "error", normalizationErr) + if claimMappingErr == nil { + claimMappingErr = normalizationErr + } + continue + } + + ctx := mapping.evalContext(claims, normalizedClaims, roleClaim) + match, err := mapping.expr.EvaluateBool(ctx) if err != nil { + if mapping.claimValue != "" { + // Claim-based mappings are normalized before evaluation, so an + // error here is unexpected. Keep evaluating other mappings; if no + // valid mapping matches, fail closed below instead of falling back. + slog.Warn("claim-based role mapping evaluation failed, failing closed", + "role_arn", mapping.roleArn, "error", err) + if claimMappingErr == nil { + claimMappingErr = err + } + continue + } + // Matcher expressions are admin-authored; keep the historical + // skip-and-fall-back behavior but surface the failure at Warn so + // operators can see it. //nolint:gosec // G706: role ARN is from server configuration - slog.Debug("CEL expression evaluation failed, skipping mapping", + slog.Warn("CEL expression evaluation failed, skipping mapping", "role_arn", mapping.roleArn, "error", err) continue } @@ -193,12 +269,15 @@ func (rm *RoleMapper) SelectRole(claims map[string]any) (string, error) { } } + // A malformed claim-based mapping must not fall through to the fallback + // role, but a valid mapping match always takes precedence over that error. + if len(matches) == 0 && claimMappingErr != nil { + return "", fmt.Errorf("%w: %w", ErrNoRoleMapping, claimMappingErr) + } + // If no matches, fall back to default role if len(matches) == 0 { - if rm.config.FallbackRoleArn == "" { - return "", fmt.Errorf("%w: no mapping matched for the provided claims", ErrNoRoleMapping) - } - return rm.config.FallbackRoleArn, nil + return rm.fallbackRole(fmt.Errorf("%w: no mapping matched for the provided claims", ErrNoRoleMapping)) } // Sort by priority (lower number = higher priority). @@ -212,6 +291,16 @@ func (rm *RoleMapper) SelectRole(claims map[string]any) (string, error) { return matches[0].roleArn, nil } +// fallbackRole returns the configured fallback role, or missingErr when none +// is configured. Callers provide their context-specific error to preserve the +// distinction between missing configuration and unmatched mappings. +func (rm *RoleMapper) fallbackRole(missingErr error) (string, error) { + if rm.config.FallbackRoleArn == "" { + return "", missingErr + } + return rm.config.FallbackRoleArn, nil +} + // ValidateConfig validates the AWS STS configuration structure. // It checks that required fields are present, ARNs are well-formed, // and session duration is within bounds. diff --git a/pkg/auth/awssts/role_mapper_test.go b/pkg/auth/awssts/role_mapper_test.go index eda5ecc9da..3bade467f7 100644 --- a/pkg/auth/awssts/role_mapper_test.go +++ b/pkg/auth/awssts/role_mapper_test.go @@ -674,3 +674,143 @@ func TestRoleMapper_Concurrency(t *testing.T) { } } } + +// TestRoleMapper_SelectRole_StringRoleClaim verifies that a string-typed role +// claim matches the mapping exactly (same as a single-element list), and that a +// string merely containing the configured claim value does not match. Previously +// string claims made CEL `in` raise "no such overload", the error was swallowed, +// and the user silently got FallbackRoleArn. +func TestRoleMapper_SelectRole_StringRoleClaim(t *testing.T) { + t.Parallel() + + cfg := &awssts.Config{ + Region: "us-east-1", + RoleClaim: "groups", + FallbackRoleArn: "arn:aws:iam::123456789012:role/DefaultRole", + RoleMappings: []awssts.RoleMapping{ + {Claim: "admins", RoleArn: "arn:aws:iam::123456789012:role/AdminRole", Priority: intPtr(1)}, + }, + } + + rm, err := awssts.NewRoleMapper(cfg) + require.NoError(t, err) + + tests := []struct { + name string + claims map[string]any + expected string + }{ + { + name: "string claim exactly equal matches", + claims: map[string]any{"sub": "user1", "groups": "admins"}, + expected: "arn:aws:iam::123456789012:role/AdminRole", + }, + { + name: "string claim containing claim value does not match", + claims: map[string]any{"sub": "user2", "groups": "superadmins"}, + expected: "arn:aws:iam::123456789012:role/DefaultRole", + }, + { + name: "string claim with suffix does not match", + claims: map[string]any{"sub": "user3", "groups": "admins-readonly"}, + expected: "arn:aws:iam::123456789012:role/DefaultRole", + }, + { + name: "list claim exact element still matches", + claims: map[string]any{"sub": "user4", "groups": []any{"users", "admins"}}, + expected: "arn:aws:iam::123456789012:role/AdminRole", + }, + { + name: "missing claim still falls back", + claims: map[string]any{"sub": "user5"}, + expected: "arn:aws:iam::123456789012:role/DefaultRole", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + role, err := rm.SelectRole(tt.claims) + require.NoError(t, err) + assert.Equal(t, tt.expected, role) + }) + } +} + +// TestRoleMapper_SelectRole_UnsupportedRoleClaimShape verifies that a role claim +// whose value is neither a string nor a list (e.g. an object or a number) fails +// closed with an error instead of silently granting a role. An object-typed claim +// would otherwise be matched by CEL `in` as a map-key membership test, and other +// types made CEL evaluation error while SelectRole swallowed the error. +func TestRoleMapper_SelectRole_UnsupportedRoleClaimShape(t *testing.T) { + t.Parallel() + + cfg := &awssts.Config{ + Region: "us-east-1", + RoleClaim: "groups", + RoleMappings: []awssts.RoleMapping{ + {Claim: "admins", RoleArn: "arn:aws:iam::123456789012:role/AdminRole", Priority: intPtr(1)}, + }, + } + + rm, err := awssts.NewRoleMapper(cfg) + require.NoError(t, err) + + tests := []struct { + name string + claims map[string]any + }{ + { + name: "object-typed role claim fails closed", + claims: map[string]any{"sub": "user1", "groups": map[string]any{"admins": map[string]any{}}}, + }, + { + name: "numeric role claim fails closed", + claims: map[string]any{"sub": "user2", "groups": 7}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + role, err := rm.SelectRole(tt.claims) + require.ErrorIs(t, err, awssts.ErrNoRoleMapping) + assert.Empty(t, role) + }) + } +} + +func TestRoleMapper_SelectRole_MatcherMatchWinsOverInvalidRoleClaim(t *testing.T) { + t.Parallel() + + matcherPriority := 1 + claimPriority := 2 + matcherRole := "arn:aws:iam::123456789012:role/MatcherRole" + cfg := &awssts.Config{ + Region: "us-east-1", + FallbackRoleArn: "arn:aws:iam::123456789012:role/FallbackRole", + RoleClaim: "groups", + RoleMappings: []awssts.RoleMapping{ + { + RoleArn: matcherRole, + Matcher: "claims.sub == 'admin1'", + Priority: &matcherPriority, + }, + { + RoleArn: "arn:aws:iam::123456789012:role/ClaimRole", + Claim: "admins", + Priority: &claimPriority, + }, + }, + } + + rm, err := awssts.NewRoleMapper(cfg) + require.NoError(t, err) + + role, err := rm.SelectRole(map[string]any{ + "sub": "admin1", + "groups": map[string]any{"admins": true}, + }) + require.NoError(t, err) + assert.Equal(t, matcherRole, role) +}