From 41d4477d855dfd218c7056e413f97e6df1aa976b Mon Sep 17 00:00:00 2001 From: platex-rehor-bot Date: Mon, 31 Aug 2026 19:22:45 +0000 Subject: [PATCH 1/4] fix(oidcsetup): validate OIDC issuer URL and set Degraded when invalid OCPBUGS-114898 When an invalid OIDC issuer URL is configured, the console operator now validates the URL format and probes the OIDC discovery endpoint before checking deployment status. Invalid or unreachable issuer URLs cause Degraded=True and Available=False with reason OIDCIssuerURLInvalid, instead of silently staying Progressing=True indefinitely. Changes: - Add DegradedNotAvailable() method to AuthStatusHandler that sets Degraded=True, Available=False, Progressing=False - Add validateOIDCIssuer() that checks URL format (HTTPS, has host) and probes .well-known/openid-configuration with 10s timeout, custom CA bundle support, and proxy env var support - Wire validation into syncAuthTypeOIDC after CA configmap sync and before deployment availability check - Add comprehensive table-driven unit tests covering URL validation, discovery endpoint responses, TLS/CA handling, and unreachable hosts Co-Authored-By: Claude Opus 4.6 --- .../controllers/oidcsetup/oidcsetup.go | 75 +++++++ .../controllers/oidcsetup/oidcsetup_test.go | 192 ++++++++++++++++++ pkg/console/status/auth_status.go | 10 + 3 files changed, 277 insertions(+) create mode 100644 pkg/console/controllers/oidcsetup/oidcsetup_test.go diff --git a/pkg/console/controllers/oidcsetup/oidcsetup.go b/pkg/console/controllers/oidcsetup/oidcsetup.go index ef0b506a60..7ecb4ec9ed 100644 --- a/pkg/console/controllers/oidcsetup/oidcsetup.go +++ b/pkg/console/controllers/oidcsetup/oidcsetup.go @@ -2,7 +2,12 @@ package oidcsetup import ( "context" + "crypto/tls" + "crypto/x509" "fmt" + "net/http" + "net/url" + "strings" "time" configv1client "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1" @@ -206,12 +211,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 +234,11 @@ func (c *oidcSetupController) syncAuthTypeOIDC(ctx context.Context, authnConfig } } + if err := validateOIDCIssuer(ctx, oidcProvider.Issuer.URL, caBundle); err != nil { + c.authStatusHandler.DegradedNotAvailable("OIDCIssuerURLInvalid", err.Error()) + return nil + } + if valid, msg, err := c.checkClientConfigStatus(authnConfig, clientSecret); err != nil { c.authStatusHandler.Degraded("DeploymentOIDCConfig", err.Error()) return err @@ -298,3 +313,63 @@ 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, 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: %v", 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") + } + + // Probe the OIDC discovery endpoint + discoveryURL := strings.TrimRight(issuerURL, "/") + "/.well-known/openid-configuration" + + tlsConfig := &tls.Config{} + 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: %v", err) + } + + resp, err := client.Do(req) + if err != nil { + return fmt.Errorf("OIDC issuer URL %q is not reachable: %v", issuerURL, err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return fmt.Errorf("OIDC discovery endpoint returned HTTP %d for issuer %q", resp.StatusCode, 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..bc9d5dd425 --- /dev/null +++ b/pkg/console/controllers/oidcsetup/oidcsetup_test.go @@ -0,0 +1,192 @@ +package oidcsetup + +import ( + "context" + "crypto/tls" + "crypto/x509" + "encoding/pem" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// 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 serves a valid OIDC discovery response + validServer := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/.well-known/openid-configuration" { + w.Header().Set("Content-Type", "application/json") + fmt.Fprintf(w, `{"issuer": "%s"}`, "https://valid-issuer") + 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) + + tests := []struct { + name string + issuerURL 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", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + err := validateOIDCIssuer(ctx, tt.issuerURL, 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 verifies that TLS configuration +// with a custom CA bundle works correctly end-to-end. +func TestValidateOIDCIssuerTLSConfig(t *testing.T) { + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `{"issuer": "https://test"}`) + })) + defer server.Close() + + caBundle := certPEM(server) + + // Verify we can reach the server with the correct CA + pool := x509.NewCertPool() + if !pool.AppendCertsFromPEM(caBundle) { + t.Fatal("failed to add server cert to pool") + } + + client := &http.Client{ + Transport: &http.Transport{ + TLSClientConfig: &tls.Config{ + RootCAs: pool, + }, + }, + } + + resp, err := client.Get(server.URL + "/.well-known/openid-configuration") + if err != nil { + t.Fatalf("failed to reach test server: %v", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + t.Fatalf("expected 200, got %d", resp.StatusCode) + } +} 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() From f777d2155c802ee1aee1d6fe0e9f59ad59f1a583 Mon Sep 17 00:00:00 2001 From: platex-rehor-bot Date: Mon, 31 Aug 2026 19:31:27 +0000 Subject: [PATCH 2/4] fix(oidcsetup): validate OIDC discovery response and harden URL checks OCPBUGS-114898 Address review feedback: reject query/fragment in issuer URL per OIDC Discovery spec, validate discovery JSON response (content-type, issuer match), set TLS MinVersion, use %w for error wrapping, handle all returned errors in tests. --- .../controllers/oidcsetup/oidcsetup.go | 57 ++++++++++- .../controllers/oidcsetup/oidcsetup_test.go | 96 +++++++++++++++++-- 2 files changed, 138 insertions(+), 15 deletions(-) diff --git a/pkg/console/controllers/oidcsetup/oidcsetup.go b/pkg/console/controllers/oidcsetup/oidcsetup.go index 7ecb4ec9ed..d3bdc5a090 100644 --- a/pkg/console/controllers/oidcsetup/oidcsetup.go +++ b/pkg/console/controllers/oidcsetup/oidcsetup.go @@ -4,7 +4,10 @@ import ( "context" "crypto/tls" "crypto/x509" + "encoding/json" "fmt" + "io" + "mime" "net/http" "net/url" "strings" @@ -324,7 +327,7 @@ func validateOIDCIssuer(ctx context.Context, issuerURL string, caBundle []byte) parsed, err := url.Parse(issuerURL) if err != nil { - return fmt.Errorf("issuer URL is malformed: %v", err) + return fmt.Errorf("issuer URL is malformed: %w", err) } if !strings.EqualFold(parsed.Scheme, "https") { @@ -335,10 +338,20 @@ func validateOIDCIssuer(ctx context.Context, issuerURL string, caBundle []byte) 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 discoveryURL := strings.TrimRight(issuerURL, "/") + "/.well-known/openid-configuration" - tlsConfig := &tls.Config{} + tlsConfig := &tls.Config{ + MinVersion: tls.VersionTLS12, + } if len(caBundle) > 0 { pool := x509.NewCertPool() if !pool.AppendCertsFromPEM(caBundle) { @@ -358,18 +371,52 @@ func validateOIDCIssuer(ctx context.Context, issuerURL string, caBundle []byte) req, err := http.NewRequestWithContext(ctx, http.MethodGet, discoveryURL, nil) if err != nil { - return fmt.Errorf("failed to create OIDC discovery request: %v", err) + 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: %v", issuerURL, err) + return fmt.Errorf("OIDC issuer URL %q is not reachable: %w", issuerURL, err) } - defer resp.Body.Close() + 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) } + // Validate content type is application/json + contentType := resp.Header.Get("Content-Type") + if contentType != "" { + 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 index bc9d5dd425..0c60644655 100644 --- a/pkg/console/controllers/oidcsetup/oidcsetup_test.go +++ b/pkg/console/controllers/oidcsetup/oidcsetup_test.go @@ -12,6 +12,15 @@ import ( "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] @@ -22,11 +31,11 @@ func certPEM(s *httptest.Server) []byte { } func TestValidateOIDCIssuer(t *testing.T) { - // Create a TLS test server that serves a valid OIDC discovery response - validServer := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // 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" { - w.Header().Set("Content-Type", "application/json") - fmt.Fprintf(w, `{"issuer": "%s"}`, "https://valid-issuer") + writeJSON(t, w, `{"issuer": %q}`, validServer.URL) return } http.NotFound(w, r) @@ -49,6 +58,30 @@ func TestValidateOIDCIssuer(t *testing.T) { 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) + tests := []struct { name string issuerURL string @@ -132,6 +165,39 @@ func TestValidateOIDCIssuer(t *testing.T) { 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", + }, } for _, tt := range tests { @@ -158,9 +224,9 @@ func TestValidateOIDCIssuer(t *testing.T) { // TestValidateOIDCIssuerTLSConfig verifies that TLS configuration // with a custom CA bundle works correctly end-to-end. func TestValidateOIDCIssuerTLSConfig(t *testing.T) { - server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - fmt.Fprint(w, `{"issuer": "https://test"}`) + 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() @@ -175,16 +241,26 @@ func TestValidateOIDCIssuerTLSConfig(t *testing.T) { client := &http.Client{ Transport: &http.Transport{ TLSClientConfig: &tls.Config{ - RootCAs: pool, + MinVersion: tls.VersionTLS12, + RootCAs: pool, }, }, } - resp, err := client.Get(server.URL + "/.well-known/openid-configuration") + req, err := http.NewRequest(http.MethodGet, server.URL+"/.well-known/openid-configuration", nil) + if err != nil { + t.Fatalf("failed to create request: %v", err) + } + + resp, err := client.Do(req) if err != nil { t.Fatalf("failed to reach test server: %v", err) } - defer resp.Body.Close() + defer func() { + if closeErr := resp.Body.Close(); closeErr != nil { + t.Errorf("failed to close response body: %v", closeErr) + } + }() if resp.StatusCode != http.StatusOK { t.Fatalf("expected 200, got %d", resp.StatusCode) From 8f8b521bee1d25f3b6f0f348b99992037f1c6999 Mon Sep 17 00:00:00 2001 From: platex-rehor-bot Date: Fri, 11 Sep 2026 13:00:12 +0000 Subject: [PATCH 3/4] OCPBUGS-114898: surface issuer validation in operator conditions and support discoveryURL Address review feedback from jhadvig: 1. Return the validation error from validateOIDCIssuer so that sync() feeds it into HandleProgressingOrDegraded, setting the OIDCClientConfigDegraded operator condition. Previously returning nil cleared the operator-level condition, hiding the failure. 2. Accept an optional discoveryURL override (from the ExternalOIDCWithUpstreamParity feature gate) instead of always deriving the discovery endpoint from the issuer URL. Configs that set spec.oidcProviders[].issuer.discoveryURL now validate correctly instead of being falsely marked OIDCIssuerURLInvalid. Adds regression tests for both discoveryURL override and fallback. Co-Authored-By: Claude Opus 4.6 --- .../controllers/oidcsetup/oidcsetup.go | 15 ++++-- .../controllers/oidcsetup/oidcsetup_test.go | 49 ++++++++++++++++--- 2 files changed, 52 insertions(+), 12 deletions(-) diff --git a/pkg/console/controllers/oidcsetup/oidcsetup.go b/pkg/console/controllers/oidcsetup/oidcsetup.go index d3bdc5a090..aa62c9ae21 100644 --- a/pkg/console/controllers/oidcsetup/oidcsetup.go +++ b/pkg/console/controllers/oidcsetup/oidcsetup.go @@ -237,9 +237,9 @@ func (c *oidcSetupController) syncAuthTypeOIDC(ctx context.Context, authnConfig } } - if err := validateOIDCIssuer(ctx, oidcProvider.Issuer.URL, caBundle); err != nil { + if err := validateOIDCIssuer(ctx, oidcProvider.Issuer.URL, oidcProvider.Issuer.DiscoveryURL, caBundle); err != nil { c.authStatusHandler.DegradedNotAvailable("OIDCIssuerURLInvalid", err.Error()) - return nil + return err } if valid, msg, err := c.checkClientConfigStatus(authnConfig, clientSecret); err != nil { @@ -320,7 +320,7 @@ func (c *oidcSetupController) handleManaged() (bool, error) { // 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, caBundle []byte) error { +func validateOIDCIssuer(ctx context.Context, issuerURL string, discoveryURLOverride string, caBundle []byte) error { if len(issuerURL) == 0 { return fmt.Errorf("issuer URL is empty") } @@ -346,8 +346,13 @@ func validateOIDCIssuer(ctx context.Context, issuerURL string, caBundle []byte) return fmt.Errorf("issuer URL must not contain a fragment component") } - // Probe the OIDC discovery endpoint - discoveryURL := strings.TrimRight(issuerURL, "/") + "/.well-known/openid-configuration" + // 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, diff --git a/pkg/console/controllers/oidcsetup/oidcsetup_test.go b/pkg/console/controllers/oidcsetup/oidcsetup_test.go index 0c60644655..b4b319b1d0 100644 --- a/pkg/console/controllers/oidcsetup/oidcsetup_test.go +++ b/pkg/console/controllers/oidcsetup/oidcsetup_test.go @@ -34,7 +34,7 @@ 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" { + if r.URL.Path == "/.well-known/openid-configuration" || r.URL.Path == "/custom-discovery" { writeJSON(t, w, `{"issuer": %q}`, validServer.URL) return } @@ -82,12 +82,26 @@ func TestValidateOIDCIssuer(t *testing.T) { defer invalidJSONServer.Close() invalidJSONServerCAPEM := certPEM(invalidJSONServer) + // 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 - caBundle []byte - wantErr bool - errSubstr string + name string + issuerURL string + discoveryURL string + caBundle []byte + wantErr bool + errSubstr string }{ { name: "empty URL", @@ -198,12 +212,33 @@ func TestValidateOIDCIssuer(t *testing.T) { wantErr: true, errSubstr: "not valid JSON", }, + { + 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.caBundle) + err := validateOIDCIssuer(ctx, tt.issuerURL, tt.discoveryURL, tt.caBundle) if tt.wantErr { if err == nil { From ed55f56655c1691bc7117d2315d2f3902782640d Mon Sep 17 00:00:00 2001 From: platex-rehor-bot Date: Fri, 11 Sep 2026 13:21:24 +0000 Subject: [PATCH 4/4] OCPBUGS-114898: require Content-Type header and refactor TLS test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address CodeRabbit review feedback: - Require Content-Type: application/json on OIDC discovery responses per OpenID Connect Discovery 1.0 §4.2, rejecting responses with no Content-Type header instead of falling through to JSON parsing. - Refactor TestValidateOIDCIssuerTLSConfig to exercise the production validateOIDCIssuer code path instead of using an independent HTTP client, which also resolves the noctx lint finding (NewRequest → NewRequestWithContext). - Add test case for missing Content-Type header. Co-Authored-By: Claude Opus 4.6 --- .../controllers/oidcsetup/oidcsetup.go | 15 +++-- .../controllers/oidcsetup/oidcsetup_test.go | 66 +++++++++---------- 2 files changed, 41 insertions(+), 40 deletions(-) diff --git a/pkg/console/controllers/oidcsetup/oidcsetup.go b/pkg/console/controllers/oidcsetup/oidcsetup.go index aa62c9ae21..36affd1e53 100644 --- a/pkg/console/controllers/oidcsetup/oidcsetup.go +++ b/pkg/console/controllers/oidcsetup/oidcsetup.go @@ -393,13 +393,16 @@ func validateOIDCIssuer(ctx context.Context, issuerURL string, discoveryURLOverr return fmt.Errorf("OIDC discovery endpoint returned HTTP %d for issuer %q", resp.StatusCode, issuerURL) } - // Validate content type is application/json + // 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 != "" { - 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) - } + 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 diff --git a/pkg/console/controllers/oidcsetup/oidcsetup_test.go b/pkg/console/controllers/oidcsetup/oidcsetup_test.go index b4b319b1d0..5d3f307643 100644 --- a/pkg/console/controllers/oidcsetup/oidcsetup_test.go +++ b/pkg/console/controllers/oidcsetup/oidcsetup_test.go @@ -2,8 +2,6 @@ package oidcsetup import ( "context" - "crypto/tls" - "crypto/x509" "encoding/pem" "fmt" "net/http" @@ -82,6 +80,21 @@ func TestValidateOIDCIssuer(t *testing.T) { 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 @@ -212,6 +225,13 @@ func TestValidateOIDCIssuer(t *testing.T) { 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, @@ -256,8 +276,9 @@ func TestValidateOIDCIssuer(t *testing.T) { } } -// TestValidateOIDCIssuerTLSConfig verifies that TLS configuration -// with a custom CA bundle works correctly end-to-end. +// 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) { @@ -266,38 +287,15 @@ func TestValidateOIDCIssuerTLSConfig(t *testing.T) { defer server.Close() caBundle := certPEM(server) + ctx := context.Background() - // Verify we can reach the server with the correct CA - pool := x509.NewCertPool() - if !pool.AppendCertsFromPEM(caBundle) { - t.Fatal("failed to add server cert to pool") + // 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) } - client := &http.Client{ - Transport: &http.Transport{ - TLSClientConfig: &tls.Config{ - MinVersion: tls.VersionTLS12, - RootCAs: pool, - }, - }, - } - - req, err := http.NewRequest(http.MethodGet, server.URL+"/.well-known/openid-configuration", nil) - if err != nil { - t.Fatalf("failed to create request: %v", err) - } - - resp, err := client.Do(req) - if err != nil { - t.Fatalf("failed to reach test server: %v", err) - } - defer func() { - if closeErr := resp.Body.Close(); closeErr != nil { - t.Errorf("failed to close response body: %v", closeErr) - } - }() - - if resp.StatusCode != http.StatusOK { - t.Fatalf("expected 200, got %d", resp.StatusCode) + // 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") } }