[WIP] OCPBUGS-85705: Add client certificate authentication for HCP metrics#3864
Open
rashmigottipati wants to merge 2 commits into
Open
Conversation
…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 <rgottipa@redhat.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
This PR updates OLM’s metrics server to support dual authentication on the /metrics endpoint—accepting either a verified client certificate (HCP Prometheus scrape path) or falling back to delegated bearer-token auth via controller-runtime (standalone OCP path).
Changes:
- Add a dual-auth
/metricshandler that prefers client certificate authentication and otherwise performs bearer token authentication. - Add new unit tests intended to cover the dual-auth behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| pkg/lib/server/server.go | Implements dual auth for /metrics by checking TLS client certs first, then delegating to controller-runtime auth for bearer tokens. |
| pkg/lib/server/dual_auth_test.go | Adds tests intended to validate client-cert, bearer-token, and unauthenticated request handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+141
to
+151
| // 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 | ||
| } |
Comment on lines
+161
to
+169
| // 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) |
Comment on lines
+38
to
+42
| req.TLS = &tls.ConnectionState{ | ||
| PeerCertificates: []*x509.Certificate{ | ||
| {}, // Just need a non-nil cert for the test | ||
| }, | ||
| } |
Comment on lines
+126
to
+130
| req.TLS = &tls.ConnectionState{ | ||
| PeerCertificates: []*x509.Certificate{ | ||
| {}, // Just need a non-nil cert for the test | ||
| }, | ||
| } |
Comment on lines
+16
to
+34
| // 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")) | ||
| }) |
Signed-off-by: Rashmi Gottipati <rgottipa@redhat.com>
Comment on lines
+145
to
+149
| bearerTokenHandler, 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") | ||
|
|
Comment on lines
+16
to
+19
| // 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.VerifiedChains) > 0 { |
rashmigottipati
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of the change:
Implement dual authentication support for OLM metrics endpoints in pkg/lib/server/server.go:
r.TLS.PeerCertificates-and if present, the TLS layer already verified it via--client-caflag, accept as authenticated and serve metricsfilters.WithAuthenticationAndAuthorization()which validates tokens viaTokenReview APIClient certificate is checked first as an optimization to avoid unnecessary TokenReview API calls when cert is already validated.
Motivation for the change:
In HCP deployments, OLM metrics endpoints return 401 Unauthorized errors when Prometheus scrapes them.
The root cause: HCP Prometheus authenticates using client certificates (configured via tlsConfig.cert/keySecret in ServiceMonitors), but OLM's existing metrics authentication only supported bearer token validation. The controller-runtime filter
filters.WithAuthenticationAndAuthorization()only hasTokenAccessReviewClientconfigured, with noClientCertificateCAContentProvider, so it cannot authenticate client certificates.This breaks metrics collection in HCP.Reviewer Checklist
/doc[FLAKE]are truly flaky and have an issue