diff --git a/pkg/controllers/externaloidc/externaloidc_controller.go b/pkg/controllers/externaloidc/externaloidc_controller.go index c1128d36a..0ba09e31f 100644 --- a/pkg/controllers/externaloidc/externaloidc_controller.go +++ b/pkg/controllers/externaloidc/externaloidc_controller.go @@ -24,7 +24,6 @@ import ( "k8s.io/apimachinery/pkg/api/errors" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/util/sets" apiserverv1beta1 "k8s.io/apiserver/pkg/apis/apiserver/v1beta1" corev1ac "k8s.io/client-go/applyconfigurations/core/v1" corev1client "k8s.io/client-go/kubernetes/typed/core/v1" @@ -124,9 +123,8 @@ func (c *externalOIDCController) sync(ctx context.Context, syncCtx factory.SyncC return nil } - errList := validateAuthenticationConfiguration(*authConfig) - if len(errList) > 0 { - return fmt.Errorf("auth config validation failed: %v", errList) + if err := validateAuthenticationConfiguration(*authConfig); err != nil { + return fmt.Errorf("auth config validation failed: %v", err) } cm := corev1ac.ConfigMap(targetAuthConfigCMName, managedNamespace).WithData(map[string]string{authConfigDataKey: authConfigJSON}) @@ -219,114 +217,36 @@ func (c *externalOIDCController) generateAuthConfig(auth configv1.Authentication return &authConfig, nil } -// validateAuthenticationConfiguration runs as many validations as possible -// on the AuthenticationConfiguration in order to catch validation errors early in the process -// instead of waiting for the KAS pods to roll out and consume the configuration -func validateAuthenticationConfiguration(auth apiserverv1beta1.AuthenticationConfiguration) (errs []error) { - // TODO currently validations from k8s.io/apiserver/pkg/apis/apiserver/validation cannot be used here - // since they aren't defined for the beta type; once the feature goes out of beta, we should replace - // this func with the upstream validations (but keep CA cert validation) - - if len(auth.JWT) == 0 { - errs = append(errs, fmt.Errorf("no JWT issuers defined")) - } - +// validateAuthenticationConfiguration performs validations that are not done at the server-side, +// including validation that the provided CA cert (or system CAs if not specified) can be used for +// TLS cert verification +func validateAuthenticationConfiguration(auth apiserverv1beta1.AuthenticationConfiguration) error { for _, jwt := range auth.JWT { - var issuerURL *url.URL - issuerURLValid := true - if len(jwt.Issuer.URL) == 0 { - errs = append(errs, fmt.Errorf("issuer URL must not be empty")) - issuerURLValid = false - } else { - var err error - issuerURL, err = url.Parse(jwt.Issuer.URL) - if err != nil { - errs = append(errs, err) - issuerURLValid = false - } else { - if issuerURL.Scheme != "https" { - errs = append(errs, fmt.Errorf("issuer URL must use HTTPS")) - issuerURLValid = false - } - if issuerURL.User != nil { - errs = append(errs, fmt.Errorf("URL must not contain a username or password")) - issuerURLValid = false - } - if len(issuerURL.RawQuery) > 0 { - errs = append(errs, fmt.Errorf("URL must not contain a query")) - issuerURLValid = false - } - if len(issuerURL.Fragment) > 0 { - errs = append(errs, fmt.Errorf("URL must not contain a fragment")) - issuerURLValid = false - } - } - } - - if len(jwt.Issuer.Audiences) == 0 { - errs = append(errs, fmt.Errorf("at least one audience must be defined")) - } - seenAudiences := sets.NewString() - for i, aud := range jwt.Issuer.Audiences { - if len(aud) == 0 { - errs = append(errs, fmt.Errorf("audience must not be empty (at index %d)", i)) - } else if seenAudiences.Has(aud) { - errs = append(errs, fmt.Errorf("duplicate audience: %s", aud)) - } - - seenAudiences.Insert(aud) - } - var caCertPool *x509.CertPool - caCertPoolValid := true + var err error if len(jwt.Issuer.CertificateAuthority) > 0 { - var err error caCertPool, err = cert.NewPoolFromBytes([]byte(jwt.Issuer.CertificateAuthority)) if err != nil { - caCertPoolValid = false - errs = append(errs, fmt.Errorf("issuer CA is invalid: %v", err)) + return fmt.Errorf("issuer CA is invalid: %v", err) } } - if issuerURL != nil && issuerURLValid && caCertPoolValid { - // make sure we can access the issuer with the given cert pool (system CAs used if pool is empty) - if err := validateCACert(*issuerURL, caCertPool); err != nil { - certMessage := "using the specified CA cert" - if caCertPool == nil { - certMessage = "using the system CAs" - } - errs = append(errs, fmt.Errorf("could not validate IDP URL %s: %v", certMessage, err)) + // make sure we can access the issuer with the given cert pool (system CAs used if pool is empty) + if err := validateCACert(jwt.Issuer.URL, caCertPool); err != nil { + certMessage := "using the specified CA cert" + if caCertPool == nil { + certMessage = "using the system CAs" } - } - - seenClaims := sets.NewString() - for i, rule := range jwt.ClaimValidationRules { - if len(rule.Claim) == 0 { - errs = append(errs, fmt.Errorf("claim must not be empty for claim validation rule at index %d", i)) - } else if seenClaims.Has(rule.Claim) { - errs = append(errs, fmt.Errorf("duplicate claim validation rule: %s", rule.Claim)) - } - - seenClaims.Insert(rule.Claim) - } - - if len(jwt.ClaimMappings.Username.Claim) == 0 { - errs = append(errs, fmt.Errorf("username claim must not be empty")) - } else if jwt.ClaimMappings.Username.Prefix == nil { - errs = append(errs, fmt.Errorf("username prefix must not be nil when claim is set")) - } - - if len(jwt.ClaimMappings.Groups.Claim) > 0 && jwt.ClaimMappings.Groups.Prefix == nil { - errs = append(errs, fmt.Errorf("group prefix must not be nil when claim is set")) + return fmt.Errorf("could not validate IDP URL %s: %v", certMessage, err) } } - return + return nil } // validateCACert makes a request to the provider's well-known endpoint using the // specified CA cert pool to validate that the certs in the pool match the host -func validateCACert(hostURL url.URL, caCertPool *x509.CertPool) error { +func validateCACert(hostURL string, caCertPool *x509.CertPool) error { client := &http.Client{ Transport: &http.Transport{ TLSClientConfig: &tls.Config{RootCAs: caCertPool}, @@ -340,7 +260,7 @@ func validateCACert(hostURL url.URL, caCertPool *x509.CertPool) error { Timeout: 5 * time.Second, } - wellKnown := strings.TrimSuffix(hostURL.String(), "/") + oidcDiscoveryEndpointPath + wellKnown := strings.TrimSuffix(hostURL, "/") + oidcDiscoveryEndpointPath req, err := http.NewRequest(http.MethodGet, wellKnown, nil) if err != nil { return fmt.Errorf("could not create well-known HTTP request: %v", err) diff --git a/pkg/controllers/externaloidc/externaloidc_controller_test.go b/pkg/controllers/externaloidc/externaloidc_controller_test.go index 6bd6f7c9e..fea385b1e 100644 --- a/pkg/controllers/externaloidc/externaloidc_controller_test.go +++ b/pkg/controllers/externaloidc/externaloidc_controller_test.go @@ -17,7 +17,6 @@ import ( "net" "net/http" "net/http/httptest" - "net/url" "strings" "testing" "time" @@ -608,7 +607,7 @@ func TestExternalOIDCController_validateAuthenticationConfiguration(t *testing.T { name: "empty config", authConfig: apiserverv1beta1.AuthenticationConfiguration{}, - expectError: true, + expectError: false, }, { name: "issuer with empty URL", @@ -628,67 +627,6 @@ func TestExternalOIDCController_validateAuthenticationConfiguration(t *testing.T }), expectError: true, }, - { - name: "issuer with user in URL", - authConfig: *authConfigWithUpdates(baseAuthConfig, []func(authConfig *apiserverv1beta1.AuthenticationConfiguration){ - func(authConfig *apiserverv1beta1.AuthenticationConfiguration) { - authConfig.JWT[0].Issuer.URL = "https://username:password@secure.com" - }, - }), - expectError: true, - }, - { - name: "issuer with query in URL", - authConfig: *authConfigWithUpdates(baseAuthConfig, []func(authConfig *apiserverv1beta1.AuthenticationConfiguration){ - func(authConfig *apiserverv1beta1.AuthenticationConfiguration) { - authConfig.JWT[0].Issuer.URL = "https://secure.com?query=true" - }, - }), - expectError: true, - }, - { - name: "issuer with fragment in URL", - authConfig: *authConfigWithUpdates(baseAuthConfig, []func(authConfig *apiserverv1beta1.AuthenticationConfiguration){ - func(authConfig *apiserverv1beta1.AuthenticationConfiguration) { - authConfig.JWT[0].Issuer.URL = "https://secure.com/index.html#fragment" - }, - }), - expectError: true, - }, - { - name: "issuer without audiences", - authConfig: *authConfigWithUpdates(baseAuthConfig, []func(authConfig *apiserverv1beta1.AuthenticationConfiguration){ - func(authConfig *apiserverv1beta1.AuthenticationConfiguration) { - authConfig.JWT[0].Issuer.Audiences = []string{} - }, - }), - expectError: true, - }, - { - name: "issuer with empty audience", - authConfig: *authConfigWithUpdates(baseAuthConfig, []func(authConfig *apiserverv1beta1.AuthenticationConfiguration){ - func(authConfig *apiserverv1beta1.AuthenticationConfiguration) { - authConfig.JWT[0].Issuer.Audiences = []string{ - "aud1", - "", - } - }, - }), - expectError: true, - }, - { - name: "issuer with duplicate audience", - authConfig: *authConfigWithUpdates(baseAuthConfig, []func(authConfig *apiserverv1beta1.AuthenticationConfiguration){ - func(authConfig *apiserverv1beta1.AuthenticationConfiguration) { - authConfig.JWT[0].Issuer.Audiences = []string{ - "aud1", - "aud2", - "aud1", - } - }, - }), - expectError: true, - }, { name: "issuer with invalid CA", authConfig: *authConfigWithUpdates(baseAuthConfig, []func(authConfig *apiserverv1beta1.AuthenticationConfiguration){ @@ -698,70 +636,6 @@ func TestExternalOIDCController_validateAuthenticationConfiguration(t *testing.T }), expectError: true, }, - { - name: "issuer with empty claim validation rule", - authConfig: *authConfigWithUpdates(baseAuthConfig, []func(authConfig *apiserverv1beta1.AuthenticationConfiguration){ - func(authConfig *apiserverv1beta1.AuthenticationConfiguration) { - authConfig.JWT[0].ClaimValidationRules = []apiserverv1beta1.ClaimValidationRule{ - { - Claim: "", - }, - } - }, - }), - expectError: true, - }, - { - name: "issuer with duplicate claim validation rule", - authConfig: *authConfigWithUpdates(baseAuthConfig, []func(authConfig *apiserverv1beta1.AuthenticationConfiguration){ - func(authConfig *apiserverv1beta1.AuthenticationConfiguration) { - authConfig.JWT[0].ClaimValidationRules = []apiserverv1beta1.ClaimValidationRule{ - { - Claim: "claim1", - RequiredValue: "val", - }, - { - Claim: "claim2", - RequiredValue: "val", - }, - { - Claim: "claim1", - RequiredValue: "val", - }, - } - }, - }), - expectError: true, - }, - { - name: "issuer with empty username claim", - authConfig: *authConfigWithUpdates(baseAuthConfig, []func(authConfig *apiserverv1beta1.AuthenticationConfiguration){ - func(authConfig *apiserverv1beta1.AuthenticationConfiguration) { - authConfig.JWT[0].ClaimMappings.Username.Claim = "" - }, - }), - expectError: true, - }, - { - name: "issuer with empty username prefix while claim exists", - authConfig: *authConfigWithUpdates(baseAuthConfig, []func(authConfig *apiserverv1beta1.AuthenticationConfiguration){ - func(authConfig *apiserverv1beta1.AuthenticationConfiguration) { - authConfig.JWT[0].ClaimMappings.Username.Claim = "claim" - authConfig.JWT[0].ClaimMappings.Username.Prefix = nil - }, - }), - expectError: true, - }, - { - name: "issuer with empty groups prefix while claim exists", - authConfig: *authConfigWithUpdates(baseAuthConfig, []func(authConfig *apiserverv1beta1.AuthenticationConfiguration){ - func(authConfig *apiserverv1beta1.AuthenticationConfiguration) { - authConfig.JWT[0].ClaimMappings.Groups.Claim = "claim" - authConfig.JWT[0].ClaimMappings.Groups.Prefix = nil - }, - }), - expectError: true, - }, { name: "valid auth config", authConfig: *authConfigWithUpdates(baseAuthConfig, []func(authConfig *apiserverv1beta1.AuthenticationConfiguration){ @@ -773,13 +647,13 @@ func TestExternalOIDCController_validateAuthenticationConfiguration(t *testing.T }, } { t.Run(tt.name, func(t *testing.T) { - errs := validateAuthenticationConfiguration(tt.authConfig) - if tt.expectError && len(errs) == 0 { + err := validateAuthenticationConfiguration(tt.authConfig) + if tt.expectError && err == nil { t.Errorf("expected error but didn't get any") } - if !tt.expectError && len(errs) > 0 { - t.Errorf("did not expect any error but got: %v", errs) + if !tt.expectError && err != nil { + t.Errorf("did not expect any error but got: %v", err) } }) @@ -796,20 +670,16 @@ func TestExternalOIDCController_validateCACert(t *testing.T) { } defer testServer.Close() testServer.StartTLS() - serverURL, err := url.Parse(testServer.URL) - if err != nil { - t.Fatalf("could not parse test server URL: %v", err) - } t.Run("nil CA cert to use system CAs", func(t *testing.T) { - err := validateCACert(*serverURL, nil) + err := validateCACert(testServer.URL, nil) if err == nil { t.Errorf("did not get an error while expecting one") } }) t.Run("valid CA cert", func(t *testing.T) { - err := validateCACert(*serverURL, certPool) + err := validateCACert(testServer.URL, certPool) if err != nil { t.Errorf("got error while not expecting one: %v", err) } @@ -822,32 +692,28 @@ func TestExternalOIDCController_validateCACert(t *testing.T) { } certPool := x509.NewCertPool() certPool.AddCert(anotherCACert) - err = validateCACert(*serverURL, certPool) + err = validateCACert(testServer.URL, certPool) if err == nil { t.Errorf("did not get an error while expecting one") } }) t.Run("unknown URL", func(t *testing.T) { - u, err := url.Parse("https://does-not-exist.com") - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - err = validateCACert(*u, certPool) + err = validateCACert("https://does-not-exist.com", certPool) if err == nil { t.Errorf("did not get an error while expecting one") } }) t.Run("nil cert pool", func(t *testing.T) { - err := validateCACert(*serverURL, nil) + err := validateCACert(testServer.URL, nil) if err == nil { t.Errorf("did not get an error while expecting one") } }) t.Run("empty cert pool", func(t *testing.T) { - err := validateCACert(*serverURL, x509.NewCertPool()) + err := validateCACert(testServer.URL, x509.NewCertPool()) if err == nil { t.Errorf("did not get an error while expecting one") } @@ -865,12 +731,8 @@ func TestExternalOIDCController_validateCACert(t *testing.T) { } defer testServer.Close() testServer.StartTLS() - serverURL, err := url.Parse(testServer.URL) - if err != nil { - t.Fatalf("could not parse test server URL: %v", err) - } - err = validateCACert(*serverURL, certPool) + err = validateCACert(testServer.URL, certPool) if err == nil { t.Errorf("did not get an error while expecting one") } @@ -887,12 +749,8 @@ func TestExternalOIDCController_validateCACert(t *testing.T) { } defer testServer.Close() testServer.StartTLS() - serverURL, err := url.Parse(testServer.URL) - if err != nil { - t.Fatalf("could not parse test server URL: %v", err) - } - err = validateCACert(*serverURL, certPool) + err = validateCACert(testServer.URL, certPool) if err == nil { t.Errorf("did not get an error while expecting one") }