diff --git a/pkg/console/controllers/oidcsetup/oidcsetup.go b/pkg/console/controllers/oidcsetup/oidcsetup.go index ef0b506a60..6e41b24b82 100644 --- a/pkg/console/controllers/oidcsetup/oidcsetup.go +++ b/pkg/console/controllers/oidcsetup/oidcsetup.go @@ -2,7 +2,16 @@ package oidcsetup import ( "context" + "crypto/sha256" + "crypto/tls" + "crypto/x509" + "encoding/json" + "errors" "fmt" + "io" + "mime" + "net/http" + "strings" "time" configv1client "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1" @@ -35,6 +44,42 @@ import ( utilsub "github.com/openshift/console-operator/pkg/console/subresource/util" ) +// reachabilityError wraps errors caused by transient network issues (DNS, +// TLS handshake, timeouts, server errors) as opposed to permanent +// configuration mistakes. The caller uses this to choose Degraded (retry) +// over DegradedNotAvailable (component unavailable). +type reachabilityError struct { + err error +} + +func (e *reachabilityError) Error() string { return e.err.Error() } +func (e *reachabilityError) Unwrap() error { return e.err } + +const ( + // issuerCacheSuccessTTL is the maximum age of a successful issuer + // validation before a fresh probe is required. This ensures that + // later-invalid discovery responses are detected without a + // configuration change or controller restart. + issuerCacheSuccessTTL = 5 * time.Minute + + // issuerCacheFailureTTL is the maximum age of a failed issuer + // validation. A shorter TTL allows transient failures (DNS, + // network) to clear without waiting for the full success interval. + issuerCacheFailureTTL = 1 * time.Minute +) + +// issuerValidationCache caches the result of the last OIDC issuer probe +// so that the synchronous HTTP call is not repeated on every sync when +// the issuer configuration has not changed. Results expire after a TTL +// so that recovered issuers and later-invalid responses are detected. +type issuerValidationCache struct { + issuerURL string + discoveryURL string + caBundleHash [32]byte + err error + validatedAt time.Time +} + // oidcSetupController: // // writes: @@ -64,7 +109,8 @@ type oidcSetupController struct { externalOIDCFeatureEnabled bool - authStatusHandler *status.AuthStatusHandler + authStatusHandler *status.AuthStatusHandler + lastIssuerValidation *issuerValidationCache } func NewOIDCSetupController( @@ -206,12 +252,19 @@ 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) } + data, ok := caCM.Data["ca-bundle.crt"] + if !ok { + return fmt.Errorf("CA configMap %q for OIDC provider %q does not contain the expected %q key", caCMName, oidcProvider.Name, "ca-bundle.crt") + } + caBundle = []byte(data) + _, _, err = resourceapply.SyncPartialConfigMap(ctx, c.configMapClient, recorder, @@ -224,6 +277,19 @@ func (c *oidcSetupController) syncAuthTypeOIDC(ctx context.Context, authnConfig } } + if err := c.validateOIDCIssuerCached(ctx, oidcProvider.Issuer.URL, oidcProvider.Issuer.DiscoveryURL, caBundle); err != nil { + var reachErr *reachabilityError + if errors.As(err, &reachErr) { + // Transient reachability failures use Degraded so the operator + // retries on the next sync; Available is left unchanged because + // operator-pod reachability does not reflect end-user reachability. + c.authStatusHandler.Degraded("OIDCIssuerNotReachable", err.Error()) + } else { + 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 +364,139 @@ func (c *oidcSetupController) handleManaged() (bool, error) { return false, fmt.Errorf("console is in an unknown state: %v", managementState) } } + +// validateOIDCIssuerCached wraps validateOIDCIssuer with a cache keyed on the +// issuer configuration (URL, discovery URL, CA bundle). The probe is only +// executed when the configuration changes, avoiding a synchronous HTTP request +// on every sync triggered by unrelated informer events. +func (c *oidcSetupController) validateOIDCIssuerCached(ctx context.Context, issuerURL, discoveryURLOverride string, caBundle []byte) error { + bundleHash := sha256.Sum256(caBundle) + if c.lastIssuerValidation != nil && + c.lastIssuerValidation.issuerURL == issuerURL && + c.lastIssuerValidation.discoveryURL == discoveryURLOverride && + c.lastIssuerValidation.caBundleHash == bundleHash { + ttl := issuerCacheSuccessTTL + if c.lastIssuerValidation.err != nil { + ttl = issuerCacheFailureTTL + } + if time.Since(c.lastIssuerValidation.validatedAt) < ttl { + return c.lastIssuerValidation.err + } + } + + err := validateOIDCIssuer(ctx, issuerURL, discoveryURLOverride, caBundle) + c.lastIssuerValidation = &issuerValidationCache{ + issuerURL: issuerURL, + discoveryURL: discoveryURLOverride, + caBundleHash: bundleHash, + err: err, + validatedAt: time.Now(), + } + return err +} + +// validateOIDCIssuer probes the OIDC discovery endpoint and validates the +// response. It returns a *reachabilityError for transient network/HTTP +// failures and a plain error for permanent configuration problems. +// +// Structural URL checks (scheme, host, query, fragment) are omitted because +// the API server's XValidation rules on issuerURL already enforce them before +// the config is persisted. +func validateOIDCIssuer(ctx context.Context, issuerURL string, discoveryURLOverride string, caBundle []byte) error { + if len(issuerURL) == 0 { + return fmt.Errorf("issuer URL is empty") + } + + // 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, + } + defer transport.CloseIdleConnections() + + 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 &reachabilityError{err: 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 { + msg := fmt.Errorf("OIDC discovery endpoint returned HTTP %d for issuer %q", resp.StatusCode, issuerURL) + // Server errors (5xx) are transient — the endpoint may recover. + // Client errors (4xx, e.g. 404) indicate invalid configuration + // because the discovery document does not exist at the configured + // location per OpenID Connect Discovery 1.0. + if resp.StatusCode >= 500 { + return &reachabilityError{err: msg} + } + return msg + } + + // 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 &reachabilityError{err: 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 match the configured issuer URL. Trailing slashes are + // normalized because the CRD itself normalizes them on write. + 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..80a88c491f --- /dev/null +++ b/pkg/console/controllers/oidcsetup/oidcsetup_test.go @@ -0,0 +1,297 @@ +package oidcsetup + +import ( + "context" + "encoding/pem" + "errors" + "fmt" + "net" + "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) + + // Start a local listener and close it immediately to obtain an address + // that always refuses connections. Using a loopback address avoids + // HTTPS_PROXY interference (Go exempts loopback from proxy) which + // would make the "unreachable host" assertion flaky with 192.0.2.1. + unreachableLn, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("failed to create listener for unreachable-host test: %v", err) + } + unreachableAddr := unreachableLn.Addr().String() + unreachableLn.Close() + + tests := []struct { + name string + issuerURL string + discoveryURL string + caBundle []byte + wantErr bool + wantReachErr bool // true when the error should be a *reachabilityError + errSubstr string + }{ + { + name: "empty URL", + issuerURL: "", + wantErr: true, + errSubstr: "issuer URL is empty", + }, + { + 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, + wantReachErr: true, + errSubstr: "HTTP 500", + }, + { + name: "unreachable host", + issuerURL: "https://" + unreachableAddr, + wantErr: true, + wantReachErr: 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, + wantReachErr: 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: "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) + } + + var reachErr *reachabilityError + isReachErr := errors.As(err, &reachErr) + if tt.wantReachErr && !isReachErr { + t.Errorf("expected *reachabilityError, got %T", err) + } + if !tt.wantReachErr && isReachErr { + t.Errorf("expected permanent error, got *reachabilityError") + } + } 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()