From 0d46b1e899af779288cbab73d52842ccbb58293a Mon Sep 17 00:00:00 2001 From: Rashmi Gottipati Date: Mon, 29 Jun 2026 13:00:37 -0400 Subject: [PATCH 1/2] Fix OCPBUGS-85705: Add client certificate authentication support for HCP metrics In HCP, Prometheus uses client certificates (tlsConfig.cert/keySecret) instead of bearer tokens. OLM only supported bearer token auth via TokenReview API, causing 401 errors. This adds dual authentication: - Client cert path: Check r.TLS.PeerCertificates (TLS already verified it) - Bearer token path: Validate via existing TokenReview (standalone OCP) Client cert checked first to avoid unnecessary API calls when cert present. Testing: Unit tests pass, verified on HCP cluster (targets now UP). Backwards compatible: standalone OCP bearer tokens unchanged. Fixes: https://issues.redhat.com/browse/OCPBUGS-85705 Signed-off-by: Rashmi Gottipati --- pkg/lib/server/dual_auth_test.go | 138 +++++++++++++++++++++++++++++++ pkg/lib/server/server.go | 90 +++++++++++++------- 2 files changed, 198 insertions(+), 30 deletions(-) create mode 100644 pkg/lib/server/dual_auth_test.go diff --git a/pkg/lib/server/dual_auth_test.go b/pkg/lib/server/dual_auth_test.go new file mode 100644 index 0000000000..07ae8d3d50 --- /dev/null +++ b/pkg/lib/server/dual_auth_test.go @@ -0,0 +1,138 @@ +package server + +import ( + "crypto/tls" + "crypto/x509" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestMetricsHandler_ClientCertAuthentication tests that requests with +// client certificates are accepted (HCP use case) +func TestMetricsHandler_ClientCertAuthentication(t *testing.T) { + // Create a test handler that mimics our dual auth logic + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Check for client cert (HCP path) + if r.TLS != nil && len(r.TLS.PeerCertificates) > 0 { + w.WriteHeader(http.StatusOK) + w.Write([]byte("# metrics")) + return + } + + // No client cert and no bearer token + if r.Header.Get("Authorization") == "" { + http.Error(w, "Unauthorized", http.StatusUnauthorized) + return + } + + // Bearer token path would validate here + w.WriteHeader(http.StatusOK) + w.Write([]byte("# metrics")) + }) + + // Test 1: Request WITH client certificate succeeds + req := httptest.NewRequest("GET", "/metrics", nil) + req.TLS = &tls.ConnectionState{ + PeerCertificates: []*x509.Certificate{ + {}, // Just need a non-nil cert for the test + }, + } + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusOK, rec.Code, "Request with client cert should succeed") + assert.Contains(t, rec.Body.String(), "metrics", "Should return metrics") +} + +// TestMetricsHandler_BearerTokenPath tests that requests without client certs +// but with Authorization header go to bearer token validation path +func TestMetricsHandler_BearerTokenPath(t *testing.T) { + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // No client cert + if r.TLS == nil || len(r.TLS.PeerCertificates) == 0 { + // Check for bearer token + if r.Header.Get("Authorization") != "" { + // In real code, this validates the token + // For test, just accept it + w.WriteHeader(http.StatusOK) + w.Write([]byte("# metrics")) + return + } + } + + http.Error(w, "Unauthorized", http.StatusUnauthorized) + }) + + // Test: Request with Authorization header (no client cert) + req := httptest.NewRequest("GET", "/metrics", nil) + req.Header.Set("Authorization", "Bearer test-token") + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusOK, rec.Code, "Request with bearer token should reach validation path") +} + +// TestMetricsHandler_NoAuthentication tests that requests without +// client cert or bearer token are rejected +func TestMetricsHandler_NoAuthentication(t *testing.T) { + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // No client cert and no bearer token + if (r.TLS == nil || len(r.TLS.PeerCertificates) == 0) && r.Header.Get("Authorization") == "" { + http.Error(w, "Unauthorized", http.StatusUnauthorized) + return + } + + w.WriteHeader(http.StatusOK) + }) + + // Test: Request without any authentication + req := httptest.NewRequest("GET", "/metrics", nil) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusUnauthorized, rec.Code, "Request without auth should be rejected") + assert.Contains(t, rec.Body.String(), "Unauthorized", "Should return unauthorized error") +} + +// TestMetricsHandler_ClientCertTakesPrecedence tests that when both +// client cert and bearer token are present, client cert is used +func TestMetricsHandler_ClientCertTakesPrecedence(t *testing.T) { + clientCertUsed := false + bearerTokenUsed := false + + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Check client cert first (HCP) + if r.TLS != nil && len(r.TLS.PeerCertificates) > 0 { + clientCertUsed = true + w.WriteHeader(http.StatusOK) + return + } + + // Check bearer token (standalone OCP) + if r.Header.Get("Authorization") != "" { + bearerTokenUsed = true + w.WriteHeader(http.StatusOK) + return + } + + http.Error(w, "Unauthorized", http.StatusUnauthorized) + }) + + // Test: Request with BOTH client cert and bearer token + req := httptest.NewRequest("GET", "/metrics", nil) + req.TLS = &tls.ConnectionState{ + PeerCertificates: []*x509.Certificate{ + {}, // Just need a non-nil cert for the test + }, + } + req.Header.Set("Authorization", "Bearer test-token") + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusOK, rec.Code, "Request should succeed") + assert.True(t, clientCertUsed, "Client cert should be checked first") + assert.False(t, bearerTokenUsed, "Bearer token should NOT be checked when client cert is present") +} diff --git a/pkg/lib/server/server.go b/pkg/lib/server/server.go index f56622ec52..7311246156 100644 --- a/pkg/lib/server/server.go +++ b/pkg/lib/server/server.go @@ -127,36 +127,66 @@ func (sc serverConfig) getListenAndServeFunc() (func() error, error) { // Set up authenticated metrics endpoint if kubeConfig is provided if sc.kubeConfig != nil && tlsEnabled { - sc.logger.Info("Setting up authenticated metrics endpoint") - // Create HTTP client with proper TLS configuration from kubeConfig - // This is necessary for TokenReview/SubjectAccessReview API calls to verify API server certificates - httpClient, err := rest.HTTPClientFor(sc.kubeConfig) - if err != nil { - return nil, fmt.Errorf("failed to create http client for authentication: %w", err) - } - // Create authentication filter using controller-runtime - filter, err := filters.WithAuthenticationAndAuthorization(sc.kubeConfig, httpClient) - if err != nil { - return nil, fmt.Errorf("failed to create authentication filter: %w", err) - } - // Create authenticated metrics handler - logger := log.FromContext(context.Background()) - authenticatedMetricsHandler, err := filter(logger, promhttp.Handler()) - if err != nil { - return nil, fmt.Errorf("failed to wrap metrics handler with authentication: %w", err) - } - // Add request logging for debugging if debug mode is enabled - if sc.debug { - debugAuthHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - sc.logger.Infof("Metrics request from %s, Auth header present: %v, User-Agent: %s", - r.RemoteAddr, r.Header.Get("Authorization") != "", r.Header.Get("User-Agent")) - authenticatedMetricsHandler.ServeHTTP(w, r) - }) - mux.Handle("/metrics", debugAuthHandler) - } else { - mux.Handle("/metrics", authenticatedMetricsHandler) - } - sc.logger.Info("Metrics endpoint configured with authentication and authorization") + sc.logger.Info("Setting up authenticated metrics endpoint with client cert and bearer token support") + + // Create metrics handler that supports BOTH: + // 1. Bearer token auth (for standalone OCP where Prometheus uses bearer tokens) + // 2. Client cert auth (for HCP where Prometheus uses client certificates) + // + // HCP ServiceMonitors configure client certificates (tlsConfig.cert/keySecret) + // while standalone OCP uses bearerTokenFile. The TLS layer (--client-ca) + // already verifies client certs; we just need to accept them as authenticated. + + metricsHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Check if client presented a verified certificate (HCP) + if r.TLS != nil && len(r.TLS.PeerCertificates) > 0 { + // Client cert was verified by TLS layer via --client-ca + // Accept it as authenticated and serve metrics + if sc.debug { + sc.logger.Infof("Metrics request authenticated via client certificate from %s, CN: %s", + r.RemoteAddr, r.TLS.PeerCertificates[0].Subject.CommonName) + } + promhttp.Handler().ServeHTTP(w, r) + return + } + + // No client cert - try bearer token auth (standalone OCP) + authHeader := r.Header.Get("Authorization") + if authHeader == "" { + sc.logger.Warnf("Metrics request rejected: no client cert and no bearer token from %s", r.RemoteAddr) + http.Error(w, "Unauthorized: client certificate or bearer token required", http.StatusUnauthorized) + return + } + + // Validate bearer token via controller-runtime + httpClient, err := rest.HTTPClientFor(sc.kubeConfig) + if err != nil { + sc.logger.WithError(err).Error("Failed to create HTTP client for bearer token auth") + http.Error(w, "Internal Server Error", http.StatusInternalServerError) + return + } + + filter, err := filters.WithAuthenticationAndAuthorization(sc.kubeConfig, httpClient) + if err != nil { + sc.logger.WithError(err).Error("Failed to create auth filter for bearer token") + http.Error(w, "Internal Server Error", http.StatusInternalServerError) + return + } + + logger := log.FromContext(r.Context()) + authenticatedHandler, err := filter(logger, promhttp.Handler()) + if err != nil { + sc.logger.WithError(err).Error("Failed to wrap metrics handler") + http.Error(w, "Internal Server Error", http.StatusInternalServerError) + return + } + + // Let controller-runtime's filter handle bearer token validation + authenticatedHandler.ServeHTTP(w, r) + }) + + mux.Handle("/metrics", metricsHandler) + sc.logger.Info("Metrics endpoint configured with dual authentication (client cert + bearer token)") } else { // Fallback to unprotected metrics (for development/testing) mux.Handle("/metrics", promhttp.Handler()) From 8eb640e7f83ddc6a30f21b4b96ab014ace1fa53b Mon Sep 17 00:00:00 2001 From: Rashmi Gottipati Date: Thu, 9 Jul 2026 15:45:39 -0400 Subject: [PATCH 2/2] address review feedback Signed-off-by: Rashmi Gottipati --- pkg/lib/server/dual_auth_test.go | 18 ++++++++----- pkg/lib/server/server.go | 45 ++++++++++++++------------------ 2 files changed, 32 insertions(+), 31 deletions(-) diff --git a/pkg/lib/server/dual_auth_test.go b/pkg/lib/server/dual_auth_test.go index 07ae8d3d50..ede9a3c3db 100644 --- a/pkg/lib/server/dual_auth_test.go +++ b/pkg/lib/server/dual_auth_test.go @@ -16,7 +16,7 @@ func TestMetricsHandler_ClientCertAuthentication(t *testing.T) { // Create a test handler that mimics our dual auth logic handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { // Check for client cert (HCP path) - if r.TLS != nil && len(r.TLS.PeerCertificates) > 0 { + if r.TLS != nil && len(r.TLS.VerifiedChains) > 0 { w.WriteHeader(http.StatusOK) w.Write([]byte("# metrics")) return @@ -37,7 +37,10 @@ func TestMetricsHandler_ClientCertAuthentication(t *testing.T) { req := httptest.NewRequest("GET", "/metrics", nil) req.TLS = &tls.ConnectionState{ PeerCertificates: []*x509.Certificate{ - {}, // Just need a non-nil cert for the test + &x509.Certificate{}, // Just need a non-nil cert for the test + }, + VerifiedChains: [][]*x509.Certificate{ + {&x509.Certificate{}}, // Verified chain indicates cert was validated }, } rec := httptest.NewRecorder() @@ -52,7 +55,7 @@ func TestMetricsHandler_ClientCertAuthentication(t *testing.T) { func TestMetricsHandler_BearerTokenPath(t *testing.T) { handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { // No client cert - if r.TLS == nil || len(r.TLS.PeerCertificates) == 0 { + if r.TLS == nil || len(r.TLS.VerifiedChains) == 0 { // Check for bearer token if r.Header.Get("Authorization") != "" { // In real code, this validates the token @@ -80,7 +83,7 @@ func TestMetricsHandler_BearerTokenPath(t *testing.T) { func TestMetricsHandler_NoAuthentication(t *testing.T) { handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { // No client cert and no bearer token - if (r.TLS == nil || len(r.TLS.PeerCertificates) == 0) && r.Header.Get("Authorization") == "" { + if (r.TLS == nil || len(r.TLS.VerifiedChains) == 0) && r.Header.Get("Authorization") == "" { http.Error(w, "Unauthorized", http.StatusUnauthorized) return } @@ -105,7 +108,7 @@ func TestMetricsHandler_ClientCertTakesPrecedence(t *testing.T) { handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { // Check client cert first (HCP) - if r.TLS != nil && len(r.TLS.PeerCertificates) > 0 { + if r.TLS != nil && len(r.TLS.VerifiedChains) > 0 { clientCertUsed = true w.WriteHeader(http.StatusOK) return @@ -125,7 +128,10 @@ func TestMetricsHandler_ClientCertTakesPrecedence(t *testing.T) { req := httptest.NewRequest("GET", "/metrics", nil) req.TLS = &tls.ConnectionState{ PeerCertificates: []*x509.Certificate{ - {}, // Just need a non-nil cert for the test + &x509.Certificate{}, // Just need a non-nil cert for the test + }, + VerifiedChains: [][]*x509.Certificate{ + {&x509.Certificate{}}, // Verified chain indicates cert was validated }, } req.Header.Set("Authorization", "Bearer test-token") diff --git a/pkg/lib/server/server.go b/pkg/lib/server/server.go index 7311246156..f9b8bff04e 100644 --- a/pkg/lib/server/server.go +++ b/pkg/lib/server/server.go @@ -129,6 +129,24 @@ func (sc serverConfig) getListenAndServeFunc() (func() error, error) { if sc.kubeConfig != nil && tlsEnabled { sc.logger.Info("Setting up authenticated metrics endpoint with client cert and bearer token support") + // Create HTTP client and auth filter once for bearer token validation + // These are reused across requests to avoid expensive allocations and TLS handshakes + httpClient, err := rest.HTTPClientFor(sc.kubeConfig) + if err != nil { + return nil, fmt.Errorf("failed to create http client for authentication: %w", err) + } + + filter, err := filters.WithAuthenticationAndAuthorization(sc.kubeConfig, httpClient) + if err != nil { + return nil, fmt.Errorf("failed to create authentication filter: %w", err) + } + + logger := log.FromContext(context.Background()) + bearerTokenHandler, err := filter(logger, promhttp.Handler()) + if err != nil { + return nil, fmt.Errorf("failed to wrap metrics handler with authentication: %w", err) + } + // Create metrics handler that supports BOTH: // 1. Bearer token auth (for standalone OCP where Prometheus uses bearer tokens) // 2. Client cert auth (for HCP where Prometheus uses client certificates) @@ -139,7 +157,7 @@ func (sc serverConfig) getListenAndServeFunc() (func() error, error) { metricsHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { // Check if client presented a verified certificate (HCP) - if r.TLS != nil && len(r.TLS.PeerCertificates) > 0 { + if r.TLS != nil && len(r.TLS.VerifiedChains) > 0 { // Client cert was verified by TLS layer via --client-ca // Accept it as authenticated and serve metrics if sc.debug { @@ -158,31 +176,8 @@ func (sc serverConfig) getListenAndServeFunc() (func() error, error) { return } - // Validate bearer token via controller-runtime - httpClient, err := rest.HTTPClientFor(sc.kubeConfig) - if err != nil { - sc.logger.WithError(err).Error("Failed to create HTTP client for bearer token auth") - http.Error(w, "Internal Server Error", http.StatusInternalServerError) - return - } - - filter, err := filters.WithAuthenticationAndAuthorization(sc.kubeConfig, httpClient) - if err != nil { - sc.logger.WithError(err).Error("Failed to create auth filter for bearer token") - http.Error(w, "Internal Server Error", http.StatusInternalServerError) - return - } - - logger := log.FromContext(r.Context()) - authenticatedHandler, err := filter(logger, promhttp.Handler()) - if err != nil { - sc.logger.WithError(err).Error("Failed to wrap metrics handler") - http.Error(w, "Internal Server Error", http.StatusInternalServerError) - return - } - // Let controller-runtime's filter handle bearer token validation - authenticatedHandler.ServeHTTP(w, r) + bearerTokenHandler.ServeHTTP(w, r) }) mux.Handle("/metrics", metricsHandler)