From 9432b4df381e95766e98e4d89939d1fa7622c78f Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Tue, 27 Feb 2024 17:11:18 -0500 Subject: [PATCH] Prevent conflicts between service account and jwt issuers Signed-off-by: Monis Khan Kubernetes-commit: 05e1eff7933a440595f4bea322b54054d3c1b153 --- pkg/apis/apiserver/validation/validation.go | 29 ++++++----- .../apiserver/validation/validation_test.go | 51 +++++++++++++++--- plugin/pkg/authenticator/token/oidc/oidc.go | 4 +- .../pkg/authenticator/token/oidc/oidc_test.go | 52 +++++++++++++++++++ 4 files changed, 115 insertions(+), 21 deletions(-) diff --git a/pkg/apis/apiserver/validation/validation.go b/pkg/apis/apiserver/validation/validation.go index 74513ff38..2f54a7e5b 100644 --- a/pkg/apis/apiserver/validation/validation.go +++ b/pkg/apis/apiserver/validation/validation.go @@ -41,7 +41,7 @@ import ( ) // ValidateAuthenticationConfiguration validates a given AuthenticationConfiguration. -func ValidateAuthenticationConfiguration(c *api.AuthenticationConfiguration) field.ErrorList { +func ValidateAuthenticationConfiguration(c *api.AuthenticationConfiguration, disallowedIssuers []string) field.ErrorList { root := field.NewPath("jwt") var allErrs field.ErrorList @@ -69,7 +69,7 @@ func ValidateAuthenticationConfiguration(c *api.AuthenticationConfiguration) fie // check and add validation for duplicate issuers. for i, a := range c.JWT { fldPath := root.Index(i) - _, errs := validateJWTAuthenticator(a, fldPath, utilfeature.DefaultFeatureGate.Enabled(features.StructuredAuthenticationConfiguration)) + _, errs := validateJWTAuthenticator(a, fldPath, sets.New(disallowedIssuers...), utilfeature.DefaultFeatureGate.Enabled(features.StructuredAuthenticationConfiguration)) allErrs = append(allErrs, errs...) } @@ -79,17 +79,17 @@ func ValidateAuthenticationConfiguration(c *api.AuthenticationConfiguration) fie // CompileAndValidateJWTAuthenticator validates a given JWTAuthenticator and returns a CELMapper with the compiled // CEL expressions for claim mappings and validation rules. // This is exported for use in oidc package. -func CompileAndValidateJWTAuthenticator(authenticator api.JWTAuthenticator) (authenticationcel.CELMapper, field.ErrorList) { - return validateJWTAuthenticator(authenticator, nil, utilfeature.DefaultFeatureGate.Enabled(features.StructuredAuthenticationConfiguration)) +func CompileAndValidateJWTAuthenticator(authenticator api.JWTAuthenticator, disallowedIssuers []string) (authenticationcel.CELMapper, field.ErrorList) { + return validateJWTAuthenticator(authenticator, nil, sets.New(disallowedIssuers...), utilfeature.DefaultFeatureGate.Enabled(features.StructuredAuthenticationConfiguration)) } -func validateJWTAuthenticator(authenticator api.JWTAuthenticator, fldPath *field.Path, structuredAuthnFeatureEnabled bool) (authenticationcel.CELMapper, field.ErrorList) { +func validateJWTAuthenticator(authenticator api.JWTAuthenticator, fldPath *field.Path, disallowedIssuers sets.Set[string], structuredAuthnFeatureEnabled bool) (authenticationcel.CELMapper, field.ErrorList) { var allErrs field.ErrorList compiler := authenticationcel.NewCompiler(environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion())) mapper := &authenticationcel.CELMapper{} - allErrs = append(allErrs, validateIssuer(authenticator.Issuer, fldPath.Child("issuer"))...) + allErrs = append(allErrs, validateIssuer(authenticator.Issuer, disallowedIssuers, fldPath.Child("issuer"))...) allErrs = append(allErrs, validateClaimValidationRules(compiler, mapper, authenticator.ClaimValidationRules, fldPath.Child("claimValidationRules"), structuredAuthnFeatureEnabled)...) allErrs = append(allErrs, validateClaimMappings(compiler, mapper, authenticator.ClaimMappings, fldPath.Child("claimMappings"), structuredAuthnFeatureEnabled)...) allErrs = append(allErrs, validateUserValidationRules(compiler, mapper, authenticator.UserValidationRules, fldPath.Child("userValidationRules"), structuredAuthnFeatureEnabled)...) @@ -97,10 +97,10 @@ func validateJWTAuthenticator(authenticator api.JWTAuthenticator, fldPath *field return *mapper, allErrs } -func validateIssuer(issuer api.Issuer, fldPath *field.Path) field.ErrorList { +func validateIssuer(issuer api.Issuer, disallowedIssuers sets.Set[string], fldPath *field.Path) field.ErrorList { var allErrs field.ErrorList - allErrs = append(allErrs, validateIssuerURL(issuer.URL, fldPath.Child("url"))...) + allErrs = append(allErrs, validateIssuerURL(issuer.URL, disallowedIssuers, fldPath.Child("url"))...) allErrs = append(allErrs, validateIssuerDiscoveryURL(issuer.URL, issuer.DiscoveryURL, fldPath.Child("discoveryURL"))...) allErrs = append(allErrs, validateAudiences(issuer.Audiences, issuer.AudienceMatchPolicy, fldPath.Child("audiences"), fldPath.Child("audienceMatchPolicy"))...) allErrs = append(allErrs, validateCertificateAuthority(issuer.CertificateAuthority, fldPath.Child("certificateAuthority"))...) @@ -108,12 +108,12 @@ func validateIssuer(issuer api.Issuer, fldPath *field.Path) field.ErrorList { return allErrs } -func validateIssuerURL(issuerURL string, fldPath *field.Path) field.ErrorList { +func validateIssuerURL(issuerURL string, disallowedIssuers sets.Set[string], fldPath *field.Path) field.ErrorList { if len(issuerURL) == 0 { return field.ErrorList{field.Required(fldPath, "URL is required")} } - return validateURL(issuerURL, fldPath) + return validateURL(issuerURL, disallowedIssuers, fldPath) } func validateIssuerDiscoveryURL(issuerURL, issuerDiscoveryURL string, fldPath *field.Path) field.ErrorList { @@ -127,13 +127,18 @@ func validateIssuerDiscoveryURL(issuerURL, issuerDiscoveryURL string, fldPath *f allErrs = append(allErrs, field.Invalid(fldPath, issuerDiscoveryURL, "discoveryURL must be different from URL")) } - allErrs = append(allErrs, validateURL(issuerDiscoveryURL, fldPath)...) + // issuerDiscoveryURL is not an issuer URL and does not need to validated against any set of disallowed issuers + allErrs = append(allErrs, validateURL(issuerDiscoveryURL, nil, fldPath)...) return allErrs } -func validateURL(issuerURL string, fldPath *field.Path) field.ErrorList { +func validateURL(issuerURL string, disallowedIssuers sets.Set[string], fldPath *field.Path) field.ErrorList { var allErrs field.ErrorList + if disallowedIssuers.Has(issuerURL) { + allErrs = append(allErrs, field.Invalid(fldPath, issuerURL, fmt.Sprintf("URL must not overlap with disallowed issuers: %s", sets.List(disallowedIssuers)))) + } + u, err := url.Parse(issuerURL) if err != nil { allErrs = append(allErrs, field.Invalid(fldPath, issuerURL, err.Error())) diff --git a/pkg/apis/apiserver/validation/validation_test.go b/pkg/apis/apiserver/validation/validation_test.go index 551d2cc20..92d205c5a 100644 --- a/pkg/apis/apiserver/validation/validation_test.go +++ b/pkg/apis/apiserver/validation/validation_test.go @@ -50,9 +50,10 @@ func TestValidateAuthenticationConfiguration(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StructuredAuthenticationConfiguration, true)() testCases := []struct { - name string - in *api.AuthenticationConfiguration - want string + name string + in *api.AuthenticationConfiguration + disallowedIssuers []string + want string }{ { name: "jwt authenticator is empty", @@ -174,6 +175,33 @@ func TestValidateAuthenticationConfiguration(t *testing.T) { }, want: `jwt[0].userValidationRules[1].expression: Duplicate value: "user.username == 'foo'"`, }, + { + name: "valid authentication configuration with disallowed issuer", + in: &api.AuthenticationConfiguration{ + JWT: []api.JWTAuthenticator{ + { + Issuer: api.Issuer{ + URL: "https://issuer-url", + Audiences: []string{"audience"}, + }, + ClaimValidationRules: []api.ClaimValidationRule{ + { + Claim: "foo", + RequiredValue: "bar", + }, + }, + ClaimMappings: api.ClaimMappings{ + Username: api.PrefixedClaimOrExpression{ + Claim: "sub", + Prefix: pointer.String("prefix"), + }, + }, + }, + }, + }, + disallowedIssuers: []string{"a", "b", "https://issuer-url", "c"}, + want: `jwt[0].issuer.url: Invalid value: "https://issuer-url": URL must not overlap with disallowed issuers: [a b c https://issuer-url]`, + }, { name: "valid authentication configuration", in: &api.AuthenticationConfiguration{ @@ -204,7 +232,7 @@ func TestValidateAuthenticationConfiguration(t *testing.T) { for _, tt := range testCases { t.Run(tt.name, func(t *testing.T) { - got := ValidateAuthenticationConfiguration(tt.in).ToAggregate() + got := ValidateAuthenticationConfiguration(tt.in, tt.disallowedIssuers).ToAggregate() if d := cmp.Diff(tt.want, errString(got)); d != "" { t.Fatalf("AuthenticationConfiguration validation mismatch (-want +got):\n%s", d) } @@ -216,9 +244,10 @@ func TestValidateIssuerURL(t *testing.T) { fldPath := field.NewPath("issuer", "url") testCases := []struct { - name string - in string - want string + name string + in string + disallowedIssuers sets.Set[string] + want string }{ { name: "url is empty", @@ -250,6 +279,12 @@ func TestValidateIssuerURL(t *testing.T) { in: "https://issuer-url#fragment", want: `issuer.url: Invalid value: "https://issuer-url#fragment": URL must not contain a fragment`, }, + { + name: "valid url that is disallowed", + in: "https://issuer-url", + disallowedIssuers: sets.New("https://issuer-url"), + want: `issuer.url: Invalid value: "https://issuer-url": URL must not overlap with disallowed issuers: [https://issuer-url]`, + }, { name: "valid url", in: "https://issuer-url", @@ -259,7 +294,7 @@ func TestValidateIssuerURL(t *testing.T) { for _, tt := range testCases { t.Run(tt.name, func(t *testing.T) { - got := validateIssuerURL(tt.in, fldPath).ToAggregate() + got := validateIssuerURL(tt.in, tt.disallowedIssuers, fldPath).ToAggregate() if d := cmp.Diff(tt.want, errString(got)); d != "" { t.Fatalf("URL validation mismatch (-want +got):\n%s", d) } diff --git a/plugin/pkg/authenticator/token/oidc/oidc.go b/plugin/pkg/authenticator/token/oidc/oidc.go index b0b633e82..77a72aaeb 100644 --- a/plugin/pkg/authenticator/token/oidc/oidc.go +++ b/plugin/pkg/authenticator/token/oidc/oidc.go @@ -94,6 +94,8 @@ type Options struct { // https://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation SupportedSigningAlgs []string + DisallowedIssuers []string + // now is used for testing. It defaults to time.Now. now func() time.Time } @@ -227,7 +229,7 @@ var allowedSigningAlgs = map[string]bool{ } func New(opts Options) (authenticator.Token, error) { - celMapper, fieldErr := apiservervalidation.CompileAndValidateJWTAuthenticator(opts.JWTAuthenticator) + celMapper, fieldErr := apiservervalidation.CompileAndValidateJWTAuthenticator(opts.JWTAuthenticator, opts.DisallowedIssuers) if err := fieldErr.ToAggregate(); err != nil { return nil, err } diff --git a/plugin/pkg/authenticator/token/oidc/oidc_test.go b/plugin/pkg/authenticator/token/oidc/oidc_test.go index f8bf78ed2..05276d932 100644 --- a/plugin/pkg/authenticator/token/oidc/oidc_test.go +++ b/plugin/pkg/authenticator/token/oidc/oidc_test.go @@ -2772,6 +2772,58 @@ func TestToken(t *testing.T) { }`, valid.Unix()), wantInitErr: `claimMappings.extra[2].key: Duplicate value: "example.org/foo"`, }, + { + name: "disallowed issuer via configured value", + options: Options{ + JWTAuthenticator: apiserver.JWTAuthenticator{ + Issuer: apiserver.Issuer{ + URL: "https://auth.example.com", + Audiences: []string{"my-client"}, + }, + ClaimMappings: apiserver.ClaimMappings{ + Username: apiserver.PrefixedClaimOrExpression{ + Expression: "claims.username", + }, + Groups: apiserver.PrefixedClaimOrExpression{ + Expression: "claims.groups", + }, + UID: apiserver.ClaimOrExpression{ + Expression: "claims.uid", + }, + Extra: []apiserver.ExtraMapping{ + { + Key: "example.org/foo", + ValueExpression: "claims.foo", + }, + { + Key: "example.org/bar", + ValueExpression: "claims.bar", + }, + }, + }, + }, + DisallowedIssuers: []string{"https://auth.example.com"}, + now: func() time.Time { return now }, + }, + signingKey: loadRSAPrivKey(t, "testdata/rsa_1.pem", jose.RS256), + pubKeys: []*jose.JSONWebKey{ + loadRSAKey(t, "testdata/rsa_1.pem", jose.RS256), + }, + claims: fmt.Sprintf(`{ + "iss": "https://auth.example.com", + "aud": "my-client", + "username": "jane", + "groups": ["team1", "team2"], + "exp": %d, + "uid": "1234", + "foo": "bar", + "bar": [ + "baz", + "qux" + ] + }`, valid.Unix()), + wantInitErr: `issuer.url: Invalid value: "https://auth.example.com": URL must not overlap with disallowed issuers: [https://auth.example.com]`, + }, { name: "extra claim mapping, empty string value for key", options: Options{