pkg/cvo/metrics: Authorize using CN verification

In OpenShift, core operators SHOULD support local authorization and
SHOULD allow the well-known metrics scraping identity
(system:serviceaccount:openshift-monitoring:prometheus-k8s) to access
the /metrics endpoint. They MAY support delegated authorization check
via SubjectAccessReviews. [1]

The well-known metrics scraping identity's client certificate is issued
for the system:serviceaccount:openshift-monitoring:prometheus-k8s
Common Name (CN) and signed by the kubernetes.io/kube-apiserver-client
signer. [2]

Thus, the commit utilizes this fact to check the client's certificate
for this specific CN value. This is also done by the hardcodedauthorizer
package utilized by other OpenShift operators for the metrics
endpoint [3].

We could utilize the existing bearer token authorization as a fallback.
However, I would like to minimize the attack surface. Especially for
security things that we are implementing and testing, rather than
importing from well-established modules.

The commit implements a user information extraction from a
certificate to minimize the needed dependencies.

[1]: https://github.com/openshift/enhancements/blob/master/CONVENTIONS.md#metrics
[2]: https://rhobs-handbook.netlify.app/products/openshiftmonitoring/collecting_metrics.md/#exposing-metrics-for-prometheus
[3]: https://pkg.go.dev/github.com/openshift/library-go/pkg/authorization/hardcodedauthorizer

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

Author: DavidHurta

pkg/cvo/metrics.go

@@ -7,20 +7,16 @@ import (
 	"fmt"
 	"net"
 	"net/http"
-	"strings"
 	"time"
 
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promhttp"
-	authenticationv1 "k8s.io/api/authentication/v1"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apiserver/pkg/server/dynamiccertificates"
 	"k8s.io/client-go/kubernetes"
-	authenticationclientsetv1 "k8s.io/client-go/kubernetes/typed/authentication/v1"
 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/klog/v2"
@@ -127,7 +123,7 @@ type asyncResult struct {
 	error error
 }
 
-func createHttpServer(ctx context.Context, client *authenticationclientsetv1.AuthenticationV1Client, disableAuth bool) *http.Server {
+func createHttpServer(disableAuth bool) *http.Server {
 	if disableAuth {
 		handler := http.NewServeMux()
 		handler.Handle("/metrics", promhttp.Handler())
@@ -137,7 +133,7 @@ func createHttpServer(ctx context.Context, client *authenticationclientsetv1.Aut
 		return server
 	}
 
-	auth := authHandler{downstream: promhttp.Handler(), ctx: ctx, client: client.TokenReviews()}
+	auth := authHandler{downstream: promhttp.Handler()}
 	handler := http.NewServeMux()
 	handler.Handle("/metrics", &auth)
 	server := &http.Server{
@@ -146,62 +142,32 @@ func createHttpServer(ctx context.Context, client *authenticationclientsetv1.Aut
 	return server
 }
 
-type tokenReviewInterface interface {
-	Create(ctx context.Context, tokenReview *authenticationv1.TokenReview, opts metav1.CreateOptions) (*authenticationv1.TokenReview, error)
-}
-
 type authHandler struct {
 	downstream http.Handler
-	ctx        context.Context
-	client     tokenReviewInterface
-}
-
-func (a *authHandler) authorize(token string) (bool, error) {
-	tr := &authenticationv1.TokenReview{
-		Spec: authenticationv1.TokenReviewSpec{
-			Token: token,
-		},
-	}
-	result, err := a.client.Create(a.ctx, tr, metav1.CreateOptions{})
-	if err != nil {
-		return false, fmt.Errorf("failed to check token: %w", err)
-	}
-	isAuthenticated := result.Status.Authenticated
-	isPrometheus := result.Status.User.Username == "system:serviceaccount:openshift-monitoring:prometheus-k8s"
-	if !isAuthenticated {
-		klog.V(4).Info("The token cannot be authenticated.")
-	} else if !isPrometheus {
-		klog.V(4).Infof("Access the metrics from the unexpected user %s is denied.", result.Status.User.Username)
-	}
-	return isAuthenticated && isPrometheus, nil
 }
 
 func (a *authHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
-	authHeader := r.Header.Get("Authorization")
-	if authHeader == "" {
-		http.Error(w, "failed to get the Authorization header", http.StatusUnauthorized)
-		return
-	}
-	token := strings.TrimPrefix(authHeader, "Bearer ")
-	if token == "" {
-		http.Error(w, "empty Bearer token", http.StatusUnauthorized)
-		return
-	}
-	if token == authHeader {
-		http.Error(w, "failed to get the Bearer token", http.StatusUnauthorized)
+	if r.TLS == nil || len(r.TLS.PeerCertificates) == 0 {
+		klog.V(4).Info("Client certificate required but not provided")
+		http.Error(w, "client certificate required", http.StatusUnauthorized)
 		return
 	}
 
-	authorized, err := a.authorize(token)
-	if err != nil {
-		klog.Warningf("Failed to authorize token: %v", err)
-		http.Error(w, "failed to authorize due to an internal error", http.StatusInternalServerError)
-		return
-	}
-	if !authorized {
-		http.Error(w, "failed to authorize", http.StatusUnauthorized)
+	// metricsAllowedClientCommonName is the Common Name (CN) of the client certificate
+	// that is authorized to access the metrics endpoint. This corresponds to the
+	// well-known Prometheus service account in OpenShift monitoring.
+	// See: https://github.com/openshift/enhancements/blob/master/CONVENTIONS.md#metrics
+	metricsAllowedClientCommonName := "system:serviceaccount:openshift-monitoring:prometheus-k8s"
+
+	// The first element is the leaf certificate that the connection is verified against
+	commonName := r.TLS.PeerCertificates[0].Subject.CommonName
+	if commonName != metricsAllowedClientCommonName {
+		klog.V(4).Infof("Access denied for common name: %s", commonName)
+		http.Error(w, fmt.Sprintf("unauthorized common name: %s", commonName), http.StatusForbidden)
 		return
 	}
+
+	klog.V(5).Infof("Access granted for common name: %s", commonName)
 	a.downstream.ServeHTTP(w, r)
 }
 
@@ -288,11 +254,6 @@ func RunMetrics(runContext context.Context, shutdownContext context.Context, lis
 		return fmt.Errorf("failed to initialize client CA controller: %w", err)
 	}
 
-	client, err := authenticationclientsetv1.NewForConfig(restConfig)
-	if err != nil {
-		return fmt.Errorf("failed to create config: %w", err)
-	}
-
 	// Start the client CA controller to begin watching the ConfigMap
 	resultChannelCount++
 	go func() {
@@ -325,7 +286,7 @@ func RunMetrics(runContext context.Context, shutdownContext context.Context, lis
 		resultChannel <- asyncResult{name: "serving certification controller"}
 	}()
 
-	server := createHttpServer(metricsContext, client, disableMetricsAuth)
+	server := createHttpServer(disableMetricsAuth)
 	tlsConfig := crypto.SecureTLSConfig(&tls.Config{
 		GetConfigForClient: func(clientHello *tls.ClientHelloInfo) (*tls.Config, error) {
 			config, err := servingCertController.GetConfigForClient(clientHello)

pkg/cvo/metrics_test.go

@@ -1,10 +1,11 @@
 package cvo
 
 import (
-	"context"
+	"crypto/tls"
+	"crypto/x509"
+	"crypto/x509/pkix"
 	"errors"
 	"fmt"
-	"io"
 	"net/http"
 	"net/http/httptest"
 	"sort"
@@ -16,7 +17,6 @@ import (
 	"github.com/google/go-cmp/cmp"
 	"github.com/prometheus/client_golang/prometheus"
 	dto "github.com/prometheus/client_model/go"
-	authenticationv1 "k8s.io/api/authentication/v1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/tools/record"
@@ -1019,27 +1019,6 @@ func metricParts(t *testing.T, metric prometheus.Metric, labels ...string) strin
 	return strings.Join(parts, " ")
 }
 
-type fakeClient struct {
-}
-
-func (c *fakeClient) Create(_ context.Context, tokenReview *authenticationv1.TokenReview, _ metav1.CreateOptions) (*authenticationv1.TokenReview, error) {
-	if tokenReview != nil {
-		ret := tokenReview.DeepCopy()
-		if tokenReview.Spec.Token == "good" {
-			ret.Status.Authenticated = true
-			ret.Status.User.Username = "system:serviceaccount:openshift-monitoring:prometheus-k8s"
-		}
-		if tokenReview.Spec.Token == "authenticated" {
-			ret.Status.Authenticated = true
-		}
-		if tokenReview.Spec.Token == "error" {
-			return nil, errors.New("fake error")
-		}
-		return ret, nil
-	}
-	return nil, errors.New("nil input")
-}
-
 type okHandler struct {
 }
 
@@ -1051,112 +1030,62 @@ func Test_authHandler(t *testing.T) {
 	tests := []struct {
 		name               string
 		handler            *authHandler
-		method             string
-		body               io.Reader
-		headerKey          string
-		headerValue        string
+		clientCN           string
+		provideCert        bool
 		expectedStatusCode int
 		expectedBody       string
 	}{
 		{
-			name: "good",
+			name: "allowed CN - prometheus-k8s",
 			handler: &authHandler{
-				ctx:        context.TODO(),
 				downstream: &okHandler{},
-				client:     &fakeClient{},
 			},
-			method:             "GET",
-			headerKey:          "Authorization",
-			headerValue:        "Bearer good",
+			clientCN:           "system:serviceaccount:openshift-monitoring:prometheus-k8s",
+			provideCert:        true,
 			expectedStatusCode: http.StatusOK,
 			expectedBody:       "ok",
 		},
 		{
-			name: "empty bearer token",
-			handler: &authHandler{
-				ctx:        context.TODO(),
-				downstream: &okHandler{},
-				client:     &fakeClient{},
-			},
-			method:             "GET",
-			headerKey:          "Authorization",
-			headerValue:        "Bearer ",
-			expectedStatusCode: 401,
-			expectedBody:       "empty Bearer token\n",
-		},
-		{
-			name: "authenticated",
-			handler: &authHandler{
-				ctx:        context.TODO(),
-				downstream: &okHandler{},
-				client:     &fakeClient{},
-			},
-			method:             "GET",
-			headerKey:          "Authorization",
-			headerValue:        "Bearer authenticated",
-			expectedStatusCode: 401,
-			expectedBody:       "failed to authorize\n",
-		},
-		{
-			name: "bad",
+			name: "unauthorized CN",
 			handler: &authHandler{
-				ctx:        context.TODO(),
 				downstream: &okHandler{},
-				client:     &fakeClient{},
 			},
-			method:             "GET",
-			headerKey:          "Authorization",
-			headerValue:        "Bearer bad",
-			expectedStatusCode: 401,
-			expectedBody:       "failed to authorize\n",
+			clientCN:           "system:serviceaccount:default:unauthorized",
+			provideCert:        true,
+			expectedStatusCode: http.StatusForbidden,
+			expectedBody:       "unauthorized common name: system:serviceaccount:default:unauthorized\n",
 		},
 		{
-			name: "failed to get the Authorization header",
+			name: "no client certificate",
 			handler: &authHandler{
-				ctx:        context.TODO(),
 				downstream: &okHandler{},
-				client:     &fakeClient{},
 			},
-			method:             "GET",
-			expectedStatusCode: 401,
-			expectedBody:       "failed to get the Authorization header\n",
-		},
-		{
-			name: "failed to get the Bearer token",
-			handler: &authHandler{
-				ctx:        context.TODO(),
-				downstream: &okHandler{},
-				client:     &fakeClient{},
-			},
-			method:             "GET",
-			headerKey:          "Authorization",
-			headerValue:        "xxx bad",
-			expectedStatusCode: 401,
-			expectedBody:       "failed to get the Bearer token\n",
-		},
-		{
-			name: "error",
-			handler: &authHandler{
-				ctx:        context.TODO(),
-				downstream: &okHandler{},
-				client:     &fakeClient{},
-			},
-			method:             "GET",
-			headerKey:          "Authorization",
-			headerValue:        "Bearer error",
-			expectedStatusCode: 500,
-			expectedBody:       "failed to authorize due to an internal error\n",
+			provideCert:        false,
+			expectedStatusCode: http.StatusUnauthorized,
+			expectedBody:       "client certificate required\n",
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			rr := httptest.NewRecorder()
 
-			req, err := http.NewRequest(tt.method, "url-not-important", tt.body)
+			req, err := http.NewRequest("GET", "url-not-important", nil)
 			if err != nil {
 				t.Fatal(err)
 			}
-			req.Header.Set(tt.headerKey, tt.headerValue)
+
+			// Mock TLS connection state with client certificate
+			if tt.provideCert {
+				req.TLS = &tls.ConnectionState{
+					PeerCertificates: []*x509.Certificate{
+						{
+							Subject: pkix.Name{
+								CommonName: tt.clientCN,
+							},
+						},
+					},
+				}
+			}
 
 			tt.handler.ServeHTTP(rr, req)
 			if diff := cmp.Diff(tt.expectedStatusCode, rr.Code); diff != "" {