From 84852ff56f952b4c3daab920d119d24c2e6a3476 Mon Sep 17 00:00:00 2001 From: Anish Ramasekar Date: Wed, 7 Feb 2024 01:41:52 +0000 Subject: [PATCH] Add `DiscoveryURL` to AuthenticationConfiguration Signed-off-by: Anish Ramasekar --- .../apiserver/pkg/apis/apiserver/types.go | 38 +++++++- .../pkg/apis/apiserver/v1alpha1/types.go | 34 ++++++- .../v1alpha1/zz_generated.conversion.go | 31 ++++++- .../v1alpha1/zz_generated.deepcopy.go | 5 ++ .../apis/apiserver/validation/validation.go | 29 ++++-- .../apiserver/validation/validation_test.go | 89 ++++++++++++++++++- 6 files changed, 212 insertions(+), 14 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/apis/apiserver/types.go b/staging/src/k8s.io/apiserver/pkg/apis/apiserver/types.go index 9153dfaf79a..a31b8753693 100644 --- a/staging/src/k8s.io/apiserver/pkg/apis/apiserver/types.go +++ b/staging/src/k8s.io/apiserver/pkg/apis/apiserver/types.go @@ -175,9 +175,43 @@ type JWTAuthenticator struct { UserValidationRules []UserValidationRule } -// Issuer provides the configuration for a external provider specific settings. +// Issuer provides the configuration for an external provider's specific settings. type Issuer struct { - URL string + // url points to the issuer URL in a format https://url or https://url/path. + // This must match the "iss" claim in the presented JWT, and the issuer returned from discovery. + // Same value as the --oidc-issuer-url flag. + // Discovery information is fetched from "{url}/.well-known/openid-configuration" unless overridden by discoveryURL. + // Required to be unique across all JWT authenticators. + // Note that egress selection configuration is not used for this network connection. + // +required + URL string + // discoveryURL, if specified, overrides the URL used to fetch discovery + // information instead of using "{url}/.well-known/openid-configuration". + // The exact value specified is used, so "/.well-known/openid-configuration" + // must be included in discoveryURL if needed. + // + // The "issuer" field in the fetched discovery information must match the "issuer.url" field + // in the AuthenticationConfiguration and will be used to validate the "iss" claim in the presented JWT. + // This is for scenarios where the well-known and jwks endpoints are hosted at a different + // location than the issuer (such as locally in the cluster). + // + // Example: + // A discovery url that is exposed using kubernetes service 'oidc' in namespace 'oidc-namespace' + // and discovery information is available at '/.well-known/openid-configuration'. + // discoveryURL: "https://oidc.oidc-namespace/.well-known/openid-configuration" + // certificateAuthority is used to verify the TLS connection and the hostname on the leaf certificate + // must be set to 'oidc.oidc-namespace'. + // + // curl https://oidc.oidc-namespace/.well-known/openid-configuration (.discoveryURL field) + // { + // issuer: "https://oidc.example.com" (.url field) + // } + // + // discoveryURL must be different from url. + // Required to be unique across all JWT authenticators. + // Note that egress selection configuration is not used for this network connection. + // +optional + DiscoveryURL string CertificateAuthority string Audiences []string AudienceMatchPolicy AudienceMatchPolicyType diff --git a/staging/src/k8s.io/apiserver/pkg/apis/apiserver/v1alpha1/types.go b/staging/src/k8s.io/apiserver/pkg/apis/apiserver/v1alpha1/types.go index be9a67ef588..840c6f1ec25 100644 --- a/staging/src/k8s.io/apiserver/pkg/apis/apiserver/v1alpha1/types.go +++ b/staging/src/k8s.io/apiserver/pkg/apis/apiserver/v1alpha1/types.go @@ -209,17 +209,45 @@ type JWTAuthenticator struct { UserValidationRules []UserValidationRule `json:"userValidationRules,omitempty"` } -// Issuer provides the configuration for a external provider specific settings. +// Issuer provides the configuration for an external provider's specific settings. type Issuer struct { // url points to the issuer URL in a format https://url or https://url/path. // This must match the "iss" claim in the presented JWT, and the issuer returned from discovery. // Same value as the --oidc-issuer-url flag. - // Used to fetch discovery information unless overridden by discoveryURL. - // Required to be unique. + // Discovery information is fetched from "{url}/.well-known/openid-configuration" unless overridden by discoveryURL. + // Required to be unique across all JWT authenticators. // Note that egress selection configuration is not used for this network connection. // +required URL string `json:"url"` + // discoveryURL, if specified, overrides the URL used to fetch discovery + // information instead of using "{url}/.well-known/openid-configuration". + // The exact value specified is used, so "/.well-known/openid-configuration" + // must be included in discoveryURL if needed. + // + // The "issuer" field in the fetched discovery information must match the "issuer.url" field + // in the AuthenticationConfiguration and will be used to validate the "iss" claim in the presented JWT. + // This is for scenarios where the well-known and jwks endpoints are hosted at a different + // location than the issuer (such as locally in the cluster). + // + // Example: + // A discovery url that is exposed using kubernetes service 'oidc' in namespace 'oidc-namespace' + // and discovery information is available at '/.well-known/openid-configuration'. + // discoveryURL: "https://oidc.oidc-namespace/.well-known/openid-configuration" + // certificateAuthority is used to verify the TLS connection and the hostname on the leaf certificate + // must be set to 'oidc.oidc-namespace'. + // + // curl https://oidc.oidc-namespace/.well-known/openid-configuration (.discoveryURL field) + // { + // issuer: "https://oidc.example.com" (.url field) + // } + // + // discoveryURL must be different from url. + // Required to be unique across all JWT authenticators. + // Note that egress selection configuration is not used for this network connection. + // +optional + DiscoveryURL *string `json:"discoveryURL,omitempty"` + // certificateAuthority contains PEM-encoded certificate authority certificates // used to validate the connection when fetching discovery information. // If unset, the system verifier is used. diff --git a/staging/src/k8s.io/apiserver/pkg/apis/apiserver/v1alpha1/zz_generated.conversion.go b/staging/src/k8s.io/apiserver/pkg/apis/apiserver/v1alpha1/zz_generated.conversion.go index 815df06aa9f..9ee1ef8a4b5 100644 --- a/staging/src/k8s.io/apiserver/pkg/apis/apiserver/v1alpha1/zz_generated.conversion.go +++ b/staging/src/k8s.io/apiserver/pkg/apis/apiserver/v1alpha1/zz_generated.conversion.go @@ -24,6 +24,7 @@ package v1alpha1 import ( unsafe "unsafe" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" conversion "k8s.io/apimachinery/pkg/conversion" runtime "k8s.io/apimachinery/pkg/runtime" apiserver "k8s.io/apiserver/pkg/apis/apiserver" @@ -324,7 +325,17 @@ func Convert_apiserver_AdmissionPluginConfiguration_To_v1alpha1_AdmissionPluginC } func autoConvert_v1alpha1_AuthenticationConfiguration_To_apiserver_AuthenticationConfiguration(in *AuthenticationConfiguration, out *apiserver.AuthenticationConfiguration, s conversion.Scope) error { - out.JWT = *(*[]apiserver.JWTAuthenticator)(unsafe.Pointer(&in.JWT)) + if in.JWT != nil { + in, out := &in.JWT, &out.JWT + *out = make([]apiserver.JWTAuthenticator, len(*in)) + for i := range *in { + if err := Convert_v1alpha1_JWTAuthenticator_To_apiserver_JWTAuthenticator(&(*in)[i], &(*out)[i], s); err != nil { + return err + } + } + } else { + out.JWT = nil + } return nil } @@ -334,7 +345,17 @@ func Convert_v1alpha1_AuthenticationConfiguration_To_apiserver_AuthenticationCon } func autoConvert_apiserver_AuthenticationConfiguration_To_v1alpha1_AuthenticationConfiguration(in *apiserver.AuthenticationConfiguration, out *AuthenticationConfiguration, s conversion.Scope) error { - out.JWT = *(*[]JWTAuthenticator)(unsafe.Pointer(&in.JWT)) + if in.JWT != nil { + in, out := &in.JWT, &out.JWT + *out = make([]JWTAuthenticator, len(*in)) + for i := range *in { + if err := Convert_apiserver_JWTAuthenticator_To_v1alpha1_JWTAuthenticator(&(*in)[i], &(*out)[i], s); err != nil { + return err + } + } + } else { + out.JWT = nil + } return nil } @@ -580,6 +601,9 @@ func Convert_apiserver_ExtraMapping_To_v1alpha1_ExtraMapping(in *apiserver.Extra func autoConvert_v1alpha1_Issuer_To_apiserver_Issuer(in *Issuer, out *apiserver.Issuer, s conversion.Scope) error { out.URL = in.URL + if err := v1.Convert_Pointer_string_To_string(&in.DiscoveryURL, &out.DiscoveryURL, s); err != nil { + return err + } out.CertificateAuthority = in.CertificateAuthority out.Audiences = *(*[]string)(unsafe.Pointer(&in.Audiences)) out.AudienceMatchPolicy = apiserver.AudienceMatchPolicyType(in.AudienceMatchPolicy) @@ -593,6 +617,9 @@ func Convert_v1alpha1_Issuer_To_apiserver_Issuer(in *Issuer, out *apiserver.Issu func autoConvert_apiserver_Issuer_To_v1alpha1_Issuer(in *apiserver.Issuer, out *Issuer, s conversion.Scope) error { out.URL = in.URL + if err := v1.Convert_string_To_Pointer_string(&in.DiscoveryURL, &out.DiscoveryURL, s); err != nil { + return err + } out.CertificateAuthority = in.CertificateAuthority out.Audiences = *(*[]string)(unsafe.Pointer(&in.Audiences)) out.AudienceMatchPolicy = AudienceMatchPolicyType(in.AudienceMatchPolicy) diff --git a/staging/src/k8s.io/apiserver/pkg/apis/apiserver/v1alpha1/zz_generated.deepcopy.go b/staging/src/k8s.io/apiserver/pkg/apis/apiserver/v1alpha1/zz_generated.deepcopy.go index 932af612707..e618178bfec 100644 --- a/staging/src/k8s.io/apiserver/pkg/apis/apiserver/v1alpha1/zz_generated.deepcopy.go +++ b/staging/src/k8s.io/apiserver/pkg/apis/apiserver/v1alpha1/zz_generated.deepcopy.go @@ -308,6 +308,11 @@ func (in *ExtraMapping) DeepCopy() *ExtraMapping { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Issuer) DeepCopyInto(out *Issuer) { *out = *in + if in.DiscoveryURL != nil { + in, out := &in.DiscoveryURL, &out.DiscoveryURL + *out = new(string) + **out = **in + } if in.Audiences != nil { in, out := &in.Audiences, &out.Audiences *out = make([]string, len(*in)) diff --git a/staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go b/staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go index ee75cd042f3..74513ff3836 100644 --- a/staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go +++ b/staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go @@ -100,21 +100,40 @@ func validateJWTAuthenticator(authenticator api.JWTAuthenticator, fldPath *field func validateIssuer(issuer api.Issuer, fldPath *field.Path) field.ErrorList { var allErrs field.ErrorList - allErrs = append(allErrs, validateURL(issuer.URL, fldPath.Child("url"))...) + allErrs = append(allErrs, validateIssuerURL(issuer.URL, 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"))...) return allErrs } -func validateURL(issuerURL string, fldPath *field.Path) field.ErrorList { +func validateIssuerURL(issuerURL string, fldPath *field.Path) field.ErrorList { + if len(issuerURL) == 0 { + return field.ErrorList{field.Required(fldPath, "URL is required")} + } + + return validateURL(issuerURL, fldPath) +} + +func validateIssuerDiscoveryURL(issuerURL, issuerDiscoveryURL string, fldPath *field.Path) field.ErrorList { var allErrs field.ErrorList - if len(issuerURL) == 0 { - allErrs = append(allErrs, field.Required(fldPath, "URL is required")) - return allErrs + if len(issuerDiscoveryURL) == 0 { + return nil } + if len(issuerURL) > 0 && strings.TrimRight(issuerURL, "/") == strings.TrimRight(issuerDiscoveryURL, "/") { + allErrs = append(allErrs, field.Invalid(fldPath, issuerDiscoveryURL, "discoveryURL must be different from URL")) + } + + allErrs = append(allErrs, validateURL(issuerDiscoveryURL, fldPath)...) + return allErrs +} + +func validateURL(issuerURL string, fldPath *field.Path) field.ErrorList { + var allErrs field.ErrorList + u, err := url.Parse(issuerURL) if err != nil { allErrs = append(allErrs, field.Invalid(fldPath, issuerURL, err.Error())) diff --git a/staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation_test.go b/staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation_test.go index 54cf78e4ae6..551d2cc203d 100644 --- a/staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation_test.go +++ b/staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation_test.go @@ -212,7 +212,7 @@ func TestValidateAuthenticationConfiguration(t *testing.T) { } } -func TestValidateURL(t *testing.T) { +func TestValidateIssuerURL(t *testing.T) { fldPath := field.NewPath("issuer", "url") testCases := []struct { @@ -259,7 +259,92 @@ func TestValidateURL(t *testing.T) { for _, tt := range testCases { t.Run(tt.name, func(t *testing.T) { - got := validateURL(tt.in, fldPath).ToAggregate() + got := validateIssuerURL(tt.in, fldPath).ToAggregate() + if d := cmp.Diff(tt.want, errString(got)); d != "" { + t.Fatalf("URL validation mismatch (-want +got):\n%s", d) + } + }) + } +} + +func TestValidateIssuerDiscoveryURL(t *testing.T) { + fldPath := field.NewPath("issuer", "discoveryURL") + + testCases := []struct { + name string + in string + issuerURL string + want string + }{ + { + name: "url is empty", + in: "", + want: "", + }, + { + name: "url parse error", + in: "https://oidc.oidc-namespace.svc:invalid-port", + want: `issuer.discoveryURL: Invalid value: "https://oidc.oidc-namespace.svc:invalid-port": parse "https://oidc.oidc-namespace.svc:invalid-port": invalid port ":invalid-port" after host`, + }, + { + name: "url is not https", + in: "http://oidc.oidc-namespace.svc", + want: `issuer.discoveryURL: Invalid value: "http://oidc.oidc-namespace.svc": URL scheme must be https`, + }, + { + name: "url user info is not allowed", + in: "https://user:pass@oidc.oidc-namespace.svc", + want: `issuer.discoveryURL: Invalid value: "https://user:pass@oidc.oidc-namespace.svc": URL must not contain a username or password`, + }, + { + name: "url raw query is not allowed", + in: "https://oidc.oidc-namespace.svc?query", + want: `issuer.discoveryURL: Invalid value: "https://oidc.oidc-namespace.svc?query": URL must not contain a query`, + }, + { + name: "url fragment is not allowed", + in: "https://oidc.oidc-namespace.svc#fragment", + want: `issuer.discoveryURL: Invalid value: "https://oidc.oidc-namespace.svc#fragment": URL must not contain a fragment`, + }, + { + name: "valid url", + in: "https://oidc.oidc-namespace.svc", + want: "", + }, + { + name: "valid url with path", + in: "https://oidc.oidc-namespace.svc/path", + want: "", + }, + { + name: "discovery url same as issuer url", + issuerURL: "https://issuer-url", + in: "https://issuer-url", + want: `issuer.discoveryURL: Invalid value: "https://issuer-url": discoveryURL must be different from URL`, + }, + { + name: "discovery url same as issuer url, with trailing slash", + issuerURL: "https://issuer-url", + in: "https://issuer-url/", + want: `issuer.discoveryURL: Invalid value: "https://issuer-url/": discoveryURL must be different from URL`, + }, + { + name: "discovery url same as issuer url, with multiple trailing slashes", + issuerURL: "https://issuer-url", + in: "https://issuer-url///", + want: `issuer.discoveryURL: Invalid value: "https://issuer-url///": discoveryURL must be different from URL`, + }, + { + name: "discovery url same as issuer url, issuer url with trailing slash", + issuerURL: "https://issuer-url/", + in: "https://issuer-url", + want: `issuer.discoveryURL: Invalid value: "https://issuer-url": discoveryURL must be different from URL`, + }, + } + + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + got := validateIssuerDiscoveryURL(tt.issuerURL, tt.in, fldPath).ToAggregate() if d := cmp.Diff(tt.want, errString(got)); d != "" { t.Fatalf("URL validation mismatch (-want +got):\n%s", d) }