diff --git a/pkg/console/controllers/oidcsetup/oidcsetup.go b/pkg/console/controllers/oidcsetup/oidcsetup.go index ef0b506a60..36affd1e53 100644 --- a/pkg/console/controllers/oidcsetup/oidcsetup.go +++ b/pkg/console/controllers/oidcsetup/oidcsetup.go @@ -2,7 +2,15 @@ package oidcsetup import ( "context" + "crypto/tls" + "crypto/x509" + "encoding/json" "fmt" + "io" + "mime" + "net/http" + "net/url" + "strings" "time" configv1client "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1" @@ -206,12 +214,17 @@ func (c *oidcSetupController) syncAuthTypeOIDC(ctx context.Context, authnConfig return err } + var caBundle []byte if caCMName := oidcProvider.Issuer.CertificateAuthority.Name; len(caCMName) > 0 { caCM, err := c.configConfigMapLister.ConfigMaps(api.OpenShiftConfigNamespace).Get(caCMName) if err != nil { return fmt.Errorf("failed to get the CA configMap %q configured for the OIDC provider %q: %w", caCMName, oidcProvider.Name, err) } + if data, ok := caCM.Data["ca-bundle.crt"]; ok { + caBundle = []byte(data) + } + _, _, err = resourceapply.SyncPartialConfigMap(ctx, c.configMapClient, recorder, @@ -224,6 +237,11 @@ func (c *oidcSetupController) syncAuthTypeOIDC(ctx context.Context, authnConfig } } + if err := validateOIDCIssuer(ctx, oidcProvider.Issuer.URL, oidcProvider.Issuer.DiscoveryURL, caBundle); err != nil { + c.authStatusHandler.DegradedNotAvailable("OIDCIssuerURLInvalid", err.Error()) + return err + } + if valid, msg, err := c.checkClientConfigStatus(authnConfig, clientSecret); err != nil { c.authStatusHandler.Degraded("DeploymentOIDCConfig", err.Error()) return err @@ -298,3 +316,115 @@ func (c *oidcSetupController) handleManaged() (bool, error) { return false, fmt.Errorf("console is in an unknown state: %v", managementState) } } + +// validateOIDCIssuer checks that the OIDC issuer URL is well-formed and that +// the OIDC discovery endpoint is reachable. It returns an error describing the +// problem when the URL is invalid or the discovery probe fails. +func validateOIDCIssuer(ctx context.Context, issuerURL string, discoveryURLOverride string, caBundle []byte) error { + if len(issuerURL) == 0 { + return fmt.Errorf("issuer URL is empty") + } + + parsed, err := url.Parse(issuerURL) + if err != nil { + return fmt.Errorf("issuer URL is malformed: %w", err) + } + + if !strings.EqualFold(parsed.Scheme, "https") { + return fmt.Errorf("issuer URL must use HTTPS scheme, got %q", parsed.Scheme) + } + + if len(parsed.Host) == 0 { + return fmt.Errorf("issuer URL has no host") + } + + if parsed.RawQuery != "" { + return fmt.Errorf("issuer URL must not contain a query component") + } + + if parsed.Fragment != "" { + return fmt.Errorf("issuer URL must not contain a fragment component") + } + + // Probe the OIDC discovery endpoint. When the API specifies an explicit + // discoveryURL (ExternalOIDCWithUpstreamParity feature gate), use it + // instead of deriving the standard path from the issuer URL. + discoveryURL := discoveryURLOverride + if len(discoveryURL) == 0 { + discoveryURL = strings.TrimRight(issuerURL, "/") + "/.well-known/openid-configuration" + } + + tlsConfig := &tls.Config{ + MinVersion: tls.VersionTLS12, + } + if len(caBundle) > 0 { + pool := x509.NewCertPool() + if !pool.AppendCertsFromPEM(caBundle) { + return fmt.Errorf("failed to parse CA bundle for OIDC issuer") + } + tlsConfig.RootCAs = pool + } + + transport := &http.Transport{ + TLSClientConfig: tlsConfig, + Proxy: http.ProxyFromEnvironment, + } + client := &http.Client{ + Transport: transport, + Timeout: 10 * time.Second, + } + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, discoveryURL, nil) + if err != nil { + return fmt.Errorf("failed to create OIDC discovery request: %w", err) + } + + resp, err := client.Do(req) + if err != nil { + return fmt.Errorf("OIDC issuer URL %q is not reachable: %w", issuerURL, err) + } + defer func() { + if closeErr := resp.Body.Close(); closeErr != nil { + klog.V(4).Infof("failed to close OIDC discovery response body: %v", closeErr) + } + }() + + if resp.StatusCode != http.StatusOK { + return fmt.Errorf("OIDC discovery endpoint returned HTTP %d for issuer %q", resp.StatusCode, issuerURL) + } + + // Per OpenID Connect Discovery 1.0 ยง4.2, the response MUST use + // application/json. Require the header so we fail fast on + // misconfigured providers instead of falling through to JSON parsing. + contentType := resp.Header.Get("Content-Type") + if contentType == "" { + return fmt.Errorf("OIDC discovery endpoint returned no Content-Type header for issuer %q", issuerURL) + } + mediaType, _, err := mime.ParseMediaType(contentType) + if err != nil || mediaType != "application/json" { + return fmt.Errorf("OIDC discovery endpoint returned non-JSON content type %q for issuer %q", contentType, issuerURL) + } + + // Decode and validate the discovery document + body, err := io.ReadAll(io.LimitReader(resp.Body, 1<<20)) // 1 MiB limit + if err != nil { + return fmt.Errorf("failed to read OIDC discovery response: %w", err) + } + + var discovery struct { + Issuer string `json:"issuer"` + } + if err := json.Unmarshal(body, &discovery); err != nil { + return fmt.Errorf("OIDC discovery response is not valid JSON: %w", err) + } + + // Per OpenID Connect Discovery 1.0, the issuer value in the discovery + // document MUST exactly match the issuer URL used to retrieve it. + normalizedIssuer := strings.TrimRight(issuerURL, "/") + normalizedDiscovery := strings.TrimRight(discovery.Issuer, "/") + if normalizedDiscovery != normalizedIssuer { + return fmt.Errorf("OIDC discovery issuer %q does not match configured issuer %q", discovery.Issuer, issuerURL) + } + + return nil +} diff --git a/pkg/console/controllers/oidcsetup/oidcsetup_test.go b/pkg/console/controllers/oidcsetup/oidcsetup_test.go new file mode 100644 index 0000000000..5d3f307643 --- /dev/null +++ b/pkg/console/controllers/oidcsetup/oidcsetup_test.go @@ -0,0 +1,301 @@ +package oidcsetup + +import ( + "context" + "encoding/pem" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// writeJSON writes a JSON response and reports errors through t. +func writeJSON(t *testing.T, w http.ResponseWriter, format string, args ...interface{}) { + t.Helper() + w.Header().Set("Content-Type", "application/json") + if _, err := fmt.Fprintf(w, format, args...); err != nil { + t.Errorf("failed to write response: %v", err) + } +} + +// certPEM returns the PEM-encoded certificate of a TLS test server. +func certPEM(s *httptest.Server) []byte { + cert := s.TLS.Certificates[0] + return pem.EncodeToMemory(&pem.Block{ + Type: "CERTIFICATE", + Bytes: cert.Certificate[0], + }) +} + +func TestValidateOIDCIssuer(t *testing.T) { + // Create a TLS test server that echoes back its own URL as the issuer + var validServer *httptest.Server + validServer = httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/.well-known/openid-configuration" || r.URL.Path == "/custom-discovery" { + writeJSON(t, w, `{"issuer": %q}`, validServer.URL) + return + } + http.NotFound(w, r) + })) + defer validServer.Close() + + validServerCAPEM := certPEM(validServer) + + // Server that returns 404 for discovery + notFoundServer := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.NotFound(w, r) + })) + defer notFoundServer.Close() + notFoundServerCAPEM := certPEM(notFoundServer) + + // Server that returns 500 + errorServer := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "internal error", http.StatusInternalServerError) + })) + defer errorServer.Close() + errorServerCAPEM := certPEM(errorServer) + + // Server that returns HTML instead of JSON + htmlServer := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/html") + if _, err := fmt.Fprint(w, "not json"); err != nil { + t.Errorf("failed to write response: %v", err) + } + })) + defer htmlServer.Close() + htmlServerCAPEM := certPEM(htmlServer) + + // Server that returns JSON with a mismatched issuer + mismatchServer := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + writeJSON(t, w, `{"issuer": "https://wrong-issuer.example.com"}`) + })) + defer mismatchServer.Close() + mismatchServerCAPEM := certPEM(mismatchServer) + + // Server that returns invalid JSON + invalidJSONServer := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + writeJSON(t, w, `{invalid json}`) + })) + defer invalidJSONServer.Close() + invalidJSONServerCAPEM := certPEM(invalidJSONServer) + + // Server that returns valid JSON but without a Content-Type header + var noContentTypeServer *httptest.Server + noContentTypeServer = httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Remove auto-detected Content-Type before writing the response. + // Setting the map entry to nil prevents Go from sniffing the type + // while ensuring no Content-Type header is sent on the wire. + w.Header()["Content-Type"] = nil + w.WriteHeader(http.StatusOK) + if _, err := fmt.Fprintf(w, `{"issuer": %q}`, noContentTypeServer.URL); err != nil { + t.Errorf("failed to write response: %v", err) + } + })) + defer noContentTypeServer.Close() + noContentTypeServerCAPEM := certPEM(noContentTypeServer) + + // Server that only serves discovery at a custom path (not .well-known), + // simulating a provider that requires discoveryURL override. + var customDiscoveryServer *httptest.Server + customDiscoveryServer = httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/oidc/.well-known/openid-configuration" { + writeJSON(t, w, `{"issuer": %q}`, customDiscoveryServer.URL) + return + } + http.NotFound(w, r) + })) + defer customDiscoveryServer.Close() + customDiscoveryServerCAPEM := certPEM(customDiscoveryServer) + + tests := []struct { + name string + issuerURL string + discoveryURL string + caBundle []byte + wantErr bool + errSubstr string + }{ + { + name: "empty URL", + issuerURL: "", + wantErr: true, + errSubstr: "issuer URL is empty", + }, + { + name: "non-HTTPS scheme", + issuerURL: "http://example.com", + wantErr: true, + errSubstr: "must use HTTPS", + }, + { + name: "missing host", + issuerURL: "https://", + wantErr: true, + errSubstr: "has no host", + }, + { + name: "malformed URL", + issuerURL: "://bad-url", + wantErr: true, + errSubstr: "malformed", + }, + { + name: "valid OIDC discovery", + issuerURL: validServer.URL, + caBundle: validServerCAPEM, + wantErr: false, + }, + { + name: "valid OIDC discovery with trailing slash", + issuerURL: validServer.URL + "/", + caBundle: validServerCAPEM, + wantErr: false, + }, + { + name: "discovery returns 404", + issuerURL: notFoundServer.URL, + caBundle: notFoundServerCAPEM, + wantErr: true, + errSubstr: "HTTP 404", + }, + { + name: "discovery returns 500", + issuerURL: errorServer.URL, + caBundle: errorServerCAPEM, + wantErr: true, + errSubstr: "HTTP 500", + }, + { + name: "unreachable host", + issuerURL: "https://192.0.2.1:1", + wantErr: true, + errSubstr: "not reachable", + }, + { + name: "custom CA bundle succeeds", + issuerURL: validServer.URL, + caBundle: validServerCAPEM, + wantErr: false, + }, + { + name: "missing CA bundle for self-signed cert fails", + issuerURL: validServer.URL, + caBundle: nil, + wantErr: true, + errSubstr: "not reachable", + }, + { + name: "invalid CA bundle PEM", + issuerURL: validServer.URL, + caBundle: []byte("not-a-valid-pem"), + wantErr: true, + errSubstr: "failed to parse CA bundle", + }, + { + name: "URL with query component", + issuerURL: "https://example.com?foo=bar", + wantErr: true, + errSubstr: "must not contain a query", + }, + { + name: "URL with fragment component", + issuerURL: "https://example.com#frag", + wantErr: true, + errSubstr: "must not contain a fragment", + }, + { + name: "discovery returns non-JSON content type", + issuerURL: htmlServer.URL, + caBundle: htmlServerCAPEM, + wantErr: true, + errSubstr: "non-JSON content type", + }, + { + name: "discovery issuer mismatch", + issuerURL: mismatchServer.URL, + caBundle: mismatchServerCAPEM, + wantErr: true, + errSubstr: "does not match configured issuer", + }, + { + name: "discovery returns invalid JSON", + issuerURL: invalidJSONServer.URL, + caBundle: invalidJSONServerCAPEM, + wantErr: true, + errSubstr: "not valid JSON", + }, + { + name: "discovery returns no Content-Type header", + issuerURL: noContentTypeServer.URL, + caBundle: noContentTypeServerCAPEM, + wantErr: true, + errSubstr: "no Content-Type header", + }, + { + name: "valid with custom discoveryURL", + issuerURL: validServer.URL, + discoveryURL: validServer.URL + "/custom-discovery", + caBundle: validServerCAPEM, + wantErr: false, + }, + { + name: "custom discoveryURL overrides default path", + issuerURL: customDiscoveryServer.URL, + discoveryURL: customDiscoveryServer.URL + "/oidc/.well-known/openid-configuration", + caBundle: customDiscoveryServerCAPEM, + wantErr: false, + }, + { + name: "discoveryURL not set falls back to default path which 404s", + issuerURL: customDiscoveryServer.URL, + caBundle: customDiscoveryServerCAPEM, + wantErr: true, + errSubstr: "HTTP 404", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + err := validateOIDCIssuer(ctx, tt.issuerURL, tt.discoveryURL, tt.caBundle) + + if tt.wantErr { + if err == nil { + t.Fatalf("expected error containing %q, got nil", tt.errSubstr) + } + if tt.errSubstr != "" && !strings.Contains(err.Error(), tt.errSubstr) { + t.Fatalf("expected error containing %q, got: %v", tt.errSubstr, err) + } + } else { + if err != nil { + t.Fatalf("expected no error, got: %v", err) + } + } + }) + } +} + +// TestValidateOIDCIssuerTLSConfig exercises validateOIDCIssuer with TLS +// servers to verify that the production TLS configuration (MinVersion 1.2, +// custom CA bundle) works end-to-end. +func TestValidateOIDCIssuerTLSConfig(t *testing.T) { + var server *httptest.Server + server = httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + writeJSON(t, w, `{"issuer": %q}`, server.URL) + })) + defer server.Close() + + caBundle := certPEM(server) + ctx := context.Background() + + // TLS 1.2+ server with correct CA should succeed + if err := validateOIDCIssuer(ctx, server.URL, "", caBundle); err != nil { + t.Fatalf("expected success with correct CA bundle, got: %v", err) + } + + // Without CA bundle, TLS verification should fail + if err := validateOIDCIssuer(ctx, server.URL, "", nil); err == nil { + t.Fatal("expected TLS verification error without CA bundle, got nil") + } +} diff --git a/pkg/console/status/auth_status.go b/pkg/console/status/auth_status.go index cd0fea2310..65d877806f 100644 --- a/pkg/console/status/auth_status.go +++ b/pkg/console/status/auth_status.go @@ -49,6 +49,16 @@ func (c *AuthStatusHandler) Degraded(reason, message string) { c.setCondition(conditionTypeDegraded, metav1.ConditionTrue, reason, message, now) } +// DegradedNotAvailable sets the Degraded condition to True, and both Available +// and Progressing to False. Use this when the configuration is fundamentally +// broken and the component cannot function (e.g. invalid OIDC issuer URL). +func (c *AuthStatusHandler) DegradedNotAvailable(reason, message string) { + now := metav1.Now() + c.setCondition(conditionTypeAvailable, metav1.ConditionFalse, reason, message, now) + c.setCondition(conditionTypeProgressing, metav1.ConditionFalse, reason, message, now) + c.setCondition(conditionTypeDegraded, metav1.ConditionTrue, reason, message, now) +} + // Progressing sets the Progressing condition to True and Degraded to False func (c *AuthStatusHandler) Progressing(reason, message string) { now := metav1.Now()