OCPBUGS-66898: Implement mTLS authentication and authorization for CVO metrics endpoint #1271
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
WalkthroughSwitched metrics authentication from bearer-token to TLS client certificates with CN-based authorization, introduced dynamic certificate controllers and a MetricsOptions API, removed fsnotify-based reloads and token-review code, updated ServiceMonitor and go.mod, and adjusted tests and startup wiring. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🪛 ast-grep (0.40.5)pkg/cvo/metrics.go[warning] 297-297: MinVersion (missing-ssl-minversion-go) [warning] 325-328: MinVersion (missing-ssl-minversion-go) 🔇 Additional comments (5)
Comment |
|
/test ? |
|
@DavidHurta: The following commands are available to trigger required jobs: The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DavidHurta The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/test e2e-agnostic-ovn |
1 similar comment
|
/test e2e-agnostic-ovn |
DavidHurta
force-pushed
the
metrics-mtls
branch
from
cb7083d to
e7705e2
Compare
|
/test all |
DavidHurta
force-pushed
the
metrics-mtls
branch
from
e6cc3ae to
920ad71
Compare
|
/test all |
|
Note: PromeCIeus and the |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cvo/metrics.go (1)
201-299: Harden the listener TLS config: ensure MinVersion/ciphers apply even ifGetConfigForClientreturns nil.The base TLS config is secured via
crypto.SecureTLSConfig, but the listener uses a freshtls.Configliteral containing onlyGetConfigForClient. If the callback returns nil or fails, the listener operates with default TLS settings lacking explicit cipher suite and minimum version constraints. Wrap the listener config withcrypto.SecureTLSConfigto guarantee hardened settings at the listener level.- tlsConfig := &tls.Config{GetConfigForClient: servingCertController.GetConfigForClient} + tlsConfig := crypto.SecureTLSConfig(&tls.Config{GetConfigForClient: servingCertController.GetConfigForClient}) go startListening(server, tlsConfig, listenAddress, resultChannel)Also verify the process has RBAC to read
kube-system/extension-apiserver-authenticationand thatclient-ca-fileis the intended trust root for Prometheus' client cert chain in all target environments.
🧹 Nitpick comments (3)
pkg/cvo/metrics_test.go (1)
4-6: CN-based TLS auth tests are aligned with the new implementation; consider usinghttptest.NewRequestfor a fully-formed request.
Currenthttp.NewRequest("GET", "url-not-important", nil)is probably fine for this handler, buthttptest.NewRequest("GET", "https://example.invalid/metrics", nil)is more idiomatic and avoids edge cases if the handler later reads URL/Host.Also applies to: 1030-1088
pkg/cvo/metrics.go (2)
126-143: RenamedisableAuthtodisableAuthorizationto avoid confusion (authn vs authz).
Right nowcreateHttpServer(disableAuth bool)is called withmetricsOptions.DisableAuthorization, but the parameter name reads like “disable authentication”.-func createHttpServer(disableAuth bool) *http.Server { - if disableAuth { +func createHttpServer(disableAuthorization bool) *http.Server { + if disableAuthorization { handler := http.NewServeMux() handler.Handle("/metrics", promhttp.Handler()) server := &http.Server{ Handler: handler, } return server }
149-166: CN authorization is very strict; confirm the allowed identity set is complete for all supported deployments.
Hardcoding onlysystem:serviceaccount:openshift-monitoring:prometheus-k8smay be correct, but it will break scraping if the client cert CN differs in hosted control planes / alternate Prometheus setups. Consider allowing a small allowlist (still static) to reduce fragility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (69)
go.sumis excluded by!**/*.sumvendor/github.com/fsnotify/fsnotify/.cirrus.ymlis excluded by!vendor/**,!**/vendor/**vendor/github.com/fsnotify/fsnotify/.gitignoreis excluded by!vendor/**,!**/vendor/**vendor/github.com/fsnotify/fsnotify/.mailmapis excluded by!vendor/**,!**/vendor/**vendor/github.com/fsnotify/fsnotify/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/fsnotify/fsnotify/CONTRIBUTING.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/fsnotify/fsnotify/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/fsnotify/fsnotify/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/fsnotify/fsnotify/backend_fen.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/fsnotify/fsnotify/backend_inotify.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/fsnotify/fsnotify/backend_kqueue.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/fsnotify/fsnotify/backend_other.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/fsnotify/fsnotify/backend_windows.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/fsnotify/fsnotify/fsnotify.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/fsnotify/fsnotify/internal/darwin.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/fsnotify/fsnotify/internal/debug_darwin.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/fsnotify/fsnotify/internal/debug_dragonfly.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/fsnotify/fsnotify/internal/debug_freebsd.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/fsnotify/fsnotify/internal/debug_kqueue.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/fsnotify/fsnotify/internal/debug_linux.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/fsnotify/fsnotify/internal/debug_netbsd.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/fsnotify/fsnotify/internal/debug_openbsd.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/fsnotify/fsnotify/internal/debug_solaris.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/fsnotify/fsnotify/internal/debug_windows.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/fsnotify/fsnotify/internal/freebsd.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/fsnotify/fsnotify/internal/internal.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/fsnotify/fsnotify/internal/unix.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/fsnotify/fsnotify/internal/unix2.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/fsnotify/fsnotify/internal/windows.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/fsnotify/fsnotify/shared.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/fsnotify/fsnotify/staticcheck.confis excluded by!vendor/**,!**/vendor/**vendor/github.com/fsnotify/fsnotify/system_bsd.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/fsnotify/fsnotify/system_darwin.gois excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/fsnotify.v1/.editorconfigis excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/fsnotify.v1/.gitignoreis excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/fsnotify.v1/.travis.ymlis excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/fsnotify.v1/AUTHORSis excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/fsnotify.v1/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/fsnotify.v1/CONTRIBUTING.mdis excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/fsnotify.v1/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/fsnotify.v1/README.mdis excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/fsnotify.v1/fen.gois excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/fsnotify.v1/fsnotify.gois excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/fsnotify.v1/inotify.gois excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/fsnotify.v1/inotify_poller.gois excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/fsnotify.v1/kqueue.gois excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/fsnotify.v1/open_mode_bsd.gois excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/fsnotify.v1/open_mode_darwin.gois excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/fsnotify.v1/windows.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apiserver/pkg/server/dynamiccertificates/cert_key.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apiserver/pkg/server/dynamiccertificates/client_ca.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apiserver/pkg/server/dynamiccertificates/configmap_cafile_content.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apiserver/pkg/server/dynamiccertificates/dynamic_cafile_content.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apiserver/pkg/server/dynamiccertificates/dynamic_serving_content.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apiserver/pkg/server/dynamiccertificates/dynamic_sni_content.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apiserver/pkg/server/dynamiccertificates/interfaces.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apiserver/pkg/server/dynamiccertificates/named_certificates.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apiserver/pkg/server/dynamiccertificates/static_content.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apiserver/pkg/server/dynamiccertificates/tlsconfig.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apiserver/pkg/server/dynamiccertificates/union_content.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/apiserver/pkg/server/dynamiccertificates/util.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/client-go/tools/events/OWNERSis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/client-go/tools/events/doc.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/client-go/tools/events/event_broadcaster.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/client-go/tools/events/event_recorder.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/client-go/tools/events/fake.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/client-go/tools/events/helper.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/client-go/tools/events/interfaces.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (5)
go.mod(2 hunks)install/0000_90_cluster-version-operator_02_servicemonitor.yaml(1 hunks)pkg/cvo/metrics.go(5 hunks)pkg/cvo/metrics_test.go(2 hunks)pkg/start/start.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
install/0000_90_cluster-version-operator_02_servicemonitor.yamlpkg/cvo/metrics_test.gopkg/start/start.gopkg/cvo/metrics.gogo.mod
🧬 Code graph analysis (1)
pkg/start/start.go (1)
pkg/cvo/metrics.go (2)
MetricsOptions(196-199)RunMetrics(208-299)
🪛 ast-grep (0.40.0)
pkg/cvo/metrics.go
[warning] 245-245: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
[warning] 247-247: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{ClientAuth: tls.RequireAndVerifyClientCert}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
[warning] 273-273: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{GetConfigForClient: servingCertController.GetConfigForClient}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🔇 Additional comments (5)
install/0000_90_cluster-version-operator_02_servicemonitor.yaml (1)
14-21: Verify Prometheus mountsmetrics-client-certsat the referenced file paths (otherwise scraping will fail).
ServiceMonitor now expects client cert/key at/etc/prometheus/secrets/metrics-client-certs/{tls.crt,tls.key}; ensure the cluster-monitoring-managed Prometheus pod has that secret mounted and populated in all target topologies (self-managed + hosted).go.mod (1)
29-29: Dependency changes look consistent with dynamiccertificates adoption; please verify module graph is tidy and conflict-free.
Run ago mod tidy-equivalent in CI and ensure no remaining imports ofgopkg.in/fsnotify.v1, and nok8s.io/*version skew introduced by the new directk8s.io/apiserverrequirement.Also applies to: 42-42
pkg/start/start.go (1)
353-358: Double-check HyperShift semantics: this disables both client-cert auth and CN authorization.
Wiring too.HyperShiftis clean, but please confirm the hosted-control-plane scraping model really expects unauthenticated metrics over TLS (vs. mTLS with different client identity).pkg/cvo/metrics.go (2)
18-20: Good move switching todynamiccertificates; verifyk8s.io/apiserverintegration matches the vendored K8s minor version.
This import-level change is appropriate for hot-reloading certs/CA; just ensure nok8s.io/*skew and thatdynamiccertificatesAPIs used here match v0.34.1 expectations.
196-199:MetricsOptionsis a clean API shape for callers.
|
/test e2e-hypershift |
DavidHurta
force-pushed
the
metrics-mtls
branch
from
920ad71 to
ff6d96e
Compare
|
/test all |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/cvo/metrics.go (1)
149-166: Certificate-based authorization correctly implemented.The CN validation logic is sound: it checks for the presence of peer certificates and validates that the Common Name matches the expected Prometheus service account. The HTTP status codes (401 for missing certificate, 403 for unauthorized CN) are appropriate.
Note: The CN
"system:serviceaccount:openshift-monitoring:prometheus-k8s"is hardcoded. Consider whether this should be configurable viaMetricsOptionsif other clients might need access in the future.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
install/0000_90_cluster-version-operator_02_servicemonitor.yaml(1 hunks)pkg/cvo/metrics.go(5 hunks)pkg/cvo/metrics_test.go(2 hunks)pkg/start/start.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/cvo/metrics_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
install/0000_90_cluster-version-operator_02_servicemonitor.yamlpkg/start/start.gopkg/cvo/metrics.go
🧬 Code graph analysis (1)
pkg/start/start.go (1)
pkg/cvo/metrics.go (2)
MetricsOptions(196-199)RunMetrics(208-312)
🪛 ast-grep (0.40.0)
pkg/cvo/metrics.go
[warning] 256-256: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{ClientAuth: clientAuth}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
[warning] 283-286: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
ClientAuth: clientAuth,
GetConfigForClient: servingCertController.GetConfigForClient,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🔇 Additional comments (10)
install/0000_90_cluster-version-operator_02_servicemonitor.yaml (1)
14-21: LGTM! TLS client certificate authentication correctly configured.The ServiceMonitor now uses TLS client certificates (
certFileandkeyFile) for authentication instead of bearer tokens, aligning with the mTLS implementation inpkg/cvo/metrics.gothat validates the client certificate's CN.pkg/cvo/metrics.go (8)
18-19: LGTM! Imports correctly updated for certificate-based approach.The new imports for
dynamiccertificatesandkubernetesclient replace the previous token-based authentication dependencies and support the dynamic certificate loading implementation.
126-143: LGTM! Simplified server creation aligns with TLS-based authentication.The function signature correctly removes the authentication client dependency since authentication is now handled at the TLS layer via client certificates, with authorization delegated to the
authHandler.
196-199: LGTM! Clean options struct for metrics configuration.The
MetricsOptionsstruct provides a clear, type-safe way to configure authentication and authorization behavior for the metrics server.
208-223: LGTM! Serving certificate controller correctly initialized.The serving certificate controller is properly set up to watch and automatically reload the server's TLS certificate and key files. The
RunOnceinitialization before starting the background watcher ensures the files are loaded before the server starts.
225-249: LGTM! Client CA controller correctly set up when authentication is enabled.The conditional creation and initialization of the client CA controller is appropriate—it's only needed when client certificate authentication is enabled. The controller watches the
extension-apiserver-authenticationConfigMap inkube-systemfor CA changes, which aligns with Kubernetes' standard approach for extension API server authentication.
276-288: LGTM! Server and TLS configuration properly wired.The HTTP server creation respects the
DisableAuthorizationsetting, and the TLS configuration correctly usesGetConfigForClientto enable dynamic certificate updates. TheClientAuthsetting in the final config ensures proper fallback behavior if the callback fails.
290-311: LGTM! Server lifecycle properly managed with graceful shutdown.The implementation correctly handles the run/shutdown contexts, waits for server termination, performs graceful shutdown on context cancellation, and properly collects results from the server goroutine. Error handling and logging are appropriate.
252-267: This concern is not valid.crypto.SecureTLSConfigfromgithub.com/openshift/library-go/pkg/cryptoautomatically setsMinVersionto the cluster's default TLS version (viaDefaultTLSVersion()) whenMinVersionis not explicitly set in the passedtls.Config. The code in metrics.go correctly relies on this behavior and does not need explicitMinVersionconfiguration.Likely an incorrect or invalid review comment.
pkg/start/start.go (1)
353-357: LGTM! MetricsOptions correctly configured for HyperShift mode.The creation of the
MetricsOptionsstruct with bothDisableAuthenticationandDisableAuthorizationset too.HyperShiftis appropriate. This ensures that in HyperShift deployments, both authentication and authorization are disabled together, while in standard deployments, both are enabled.
|
/retest |
1 similar comment
|
/retest |
|
/test e2e-hypershift-conformance |
|
/test all |
DavidHurta
force-pushed
the
metrics-mtls
branch
from
1db20de to
b0454ca
Compare
|
/test all |
openshift-ci-robot
added
jira/valid-bug
|
@DavidHurta: This pull request references Jira Issue OCPBUGS-66898, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
TestingStandalone OpenShiftPrometheus can scrape using mTLSClient needs to provide its certificateThe CN is verified correctlyThe metrics-server Pod has its client certificate signed by the same issuer as does Prometheus. Let's try to scrape the CVO using the metric-server certs. I am using Checking the CVO logs: HyperShiftPromeCleus can be used in combination with the Given that the OCP monitoring is able to scrape hosted CVO, it is safe to assume that a custom monitoring stack can as well. The hosted CVO does not even have access to the required CA bundle. Prometheus output using PromeCIeus from a The E2E functionality requires a follow-up with the HyperShift team regarding the E2E testing to not potentially introduce a regression with this PR. |
|
/hold I am checking with the HyperShift team. However, the PR is ready for a review. |
openshift-ci
bot
added
the
do-not-merge/hold
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/test e2e-agnostic-ovn-upgrade-into-change |
|
@DavidHurta: This pull request references Jira Issue OCPBUGS-66898, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
pkg/cvo/metrics.go
Outdated
| cn := verifiedChains[0][0].Subject.CommonName | ||
| if cn != metricsAllowedClientCN { | ||
| klog.V(4).Infof("Access denied for CN: %s", cn) | ||
| return errors.New("unauthorized CN") |
There was a problem hiding this comment.
nit: I don't mind as much for logs, but for error messages, which might eventually reach users, can we avoid accronyms with unauthorized common name or similar instead of using CN?
Also, not clear to me why we'd log the rejected CN in the previous Infof but not include it in the error message here. The client has the certificate, and they can go look at their CN there, but if we include the CN in the error message, maybe that saves them a bit of work?
There was a problem hiding this comment.
Let me double-check where exactly does the returned error bubble up to.
There was a problem hiding this comment.
The error value bubbles up to the server's logger, that is, the value is logged in CVO logs (see a net/http line). This can be verified by verifying the CVO's logs. We currently use the default server's logger.
2026/01/08 17:08:57 http: TLS handshake error from 10.0.110.147:42700: unauthorized CN
The user observers only a TLS alert message. See a crypto/tls code section:
if c.config.VerifyPeerCertificate != nil && !echRejected {
if err := c.config.VerifyPeerCertificate(certificates, c.verifiedChains); err != nil {
c.sendAlert(alertBadCertificate)
return err
}
}This is unfortunate to some degree, as there is a specific access_denied TLS alert message that I would probably want to return instead of bad_certificate for failed authorizations.
There was a problem hiding this comment.
I am starting to be open to reverting the 290ee8b commit.
There was a problem hiding this comment.
I could highlight the security aspects in more detail in the code comments.
The optimization was not my main goal with the commit. It was rather to move the authorization into the TLS layer and to make the mechanism's nature more explicit. The optimization was a nice plus.
However, the trade-off is more difficult debugging and technically wrong return codes to clients. This is an internal service, so I don't presume to cause it too many disruptions. However, I am inclined into reverting the commit to keep things correct.
There was a problem hiding this comment.
Back to your original question.
I don't mind as much for logs, but for error messages, which might eventually reach users, can we avoid accronyms with unauthorized common name or similar instead of using CN?
Yes, definitely.
Also, not clear to me why we'd log the rejected CN in the previous Infof but not include it in the error message here. The client has the certificate, and they can go look at their CN there, but if we include the CN in the error message, maybe that saves them a bit of work?
Given we have found out that the client does not get to see these logs, we can delete the klog.V(4).Infof("Access denied for CN: %s", cn) line entirely, and we can include the CN in error itself.
pkg/cvo/metrics.go
Outdated
| // they change on disk using dynamiccertificates. | ||
| func RunMetrics(runContext context.Context, shutdownContext context.Context, listenAddress, certFile, keyFile string, restConfig *rest.Config, metricsOptions MetricsOptions) error { | ||
| if listenAddress == "" { | ||
| return errors.New("TLS configuration is required to serve metrics") |
| origKeyChecksum, err := checksumFile(keyFile) | ||
| if err != nil { | ||
| return fmt.Errorf("Failed to initialize key file checksum: %w", err) | ||
| if err := servingContentController.RunOnce(metricsContext); err != nil { |
There was a problem hiding this comment.
I suspect that the RunOnce approach is intended to catch issues early and allow this function to hard fail, before launching the Run method. But there's more going on in Run than in RunOnce, and having no way to get information back out of the Run goroutine to tell you how it's going seems like it could be problematic. What if we have some kind of issue that would need a container restart to recover? But 🤷, I guess if I was particularly motivated, I could try to push for a clearer feedback mechanism in the upstream library. And ClusterVersionOperatorDown gives us a chance at calling in an admin to manually recover things if something in the metrics-serving pipe breaks. So not a blocker, just something that I'm grumbling about, in the hopes that it might motivate someone else to chase upstream about moving to an API with better feeback mechanisms ;)
There was a problem hiding this comment.
Worth noting for transparency that the dynamiccertificates.NewDynamicServingCertificateController does allow to consume an event recorder. It would not fix the CVO, but it could potentially help debug some issues.
There was a problem hiding this comment.
It is not currently set up in the PR.
There was a problem hiding this comment.
This was originally done to keep the scope down, and it eventually ended up in the PR. Also, the related docs, which reference rhobs/observailbity-operator changes that initially omitted the logging due to:
// Disabling events for now because the controller generates
// invalid events when used with DynamicServingContentFromFiles.
However, I see that the project's contributors added the logging a year ago!
I don't see a valid point in defending to not bring the logging with this PR other than "let's ship it, life is short 
"
pkg/cvo/metrics.go
Outdated
| } | ||
|
|
||
| clientAuth := tls.NoClientCert | ||
| if !metricsOptions.DisableAuthentication { |
There was a problem hiding this comment.
nit: this seems like it would be slightly simpler if you shifted the clientAuth := tls.NoClientCert default up to the var clientCA ... level above, and merged the clientAuth = tls.RequireAndVerifyClientCert line into the previous !metricsOptions.DisableAuthentication block, instead of having two of those blocks in such close proximity.
Same for baseTlSConfig too, I think collecting all the insecure defaults in one place, and then overriding all of them in the secure section. Or maybe for the cheap ones we can initialize to the secure settings, like:
clientAuth := tls.RequireAndVerifyClientCert
baseTlSConfig := crypto.SecureTLSConfig(&tls.Config{ClientAuth: clientAuth})
baseTlSConfig.VerifyPeerCertificate = verifyPeerCertificate
var clientCA dynamiccertificates.CAContentProvider
var clientCAController *dynamiccertificates.ConfigMapCAController
if metricsOptions.DisableAuthentication { // weakening
clientAuth = tls.NoClientCert
baseTlSConfig.VerifyPeerCertificate = someDefault
} else { // expensive strengthening
kubeClient, err := kubernetes.NewForConfig(restConfig)
...
}
|
|
||
| resultChannelCount++ | ||
| go func() { | ||
| servingCertController.Run(1, metricsContext.Done()) |
There was a problem hiding this comment.
Wondering about that workers=1 argument, 😂:
doesn't matter what workers say, only start one.
pkg/cvo/metrics.go
Outdated
| continue | ||
| } | ||
| case <-metricsContext.Done(): | ||
| klog.Infof("Clean shutdown requested: %v", metricsContext.Err()) |
There was a problem hiding this comment.
the klog line will include metrics.go, but maybe mention metrics here, just to be extra clear? Clean metrics shutdown requested: ...?
| if loopError == nil { | ||
| loopError = shutdownError | ||
| } else if shutdownError != nil { // log the error we are discarding | ||
| klog.Errorf("Failed to gracefully shut down metrics server: %v", shutdownError) |
There was a problem hiding this comment.
I think you want to keep the logging down here, instead of shifting it to log unconditionally, otherwise you'll double up on the loopError message (which is getting logged below at the Infof level, and then again (presumably at the Errorf level) in whatever is at the top of the bubbled-back return loopError pipeline.
There was a problem hiding this comment.
This change deserved its own commit.
It seems to be possible for the previous logic:
shutdown = true
shutdownError := server.Shutdown(shutdownContext)
if loopError == nil {
loopError = shutdownError
} else if shutdownError != nil { // log the error we are discarding
klog.Errorf("Failed to gracefully shut down metrics server: %v", shutdownError)
}to not print an error from the http server not shutting down correctly. This was not previously possible as only one go routine existed.
Imagine a scenario:
loopErroris nilmetricsContextis cancelled- http server shuts down incorrectly; the
shutdownErrorholds the error - not printed immediately; the value gets saved to
loopError loopErrormay get overridden while other go routines are shutting down- The
Graceful shutdown complete for metrics server: %soutputs the last value saved inloopError - the
shutdownErrorgets forgotten
This does not resolve your comment regarding the even triple same logs. Maybe it's worth revisiting the purpose of the loopError and my usage of it, as the logic seems to be a bit too tricky at first glance. My proposed changes should log every failure and return the last loop's error. Without the change, it is still possible to even triple log failures (handleServerResult, the Graceful shutdown complete for metrics server, and the parent function), but we can also forget the http server's failures all together.
This reverts commit 290ee8b. The used library does not allow to modify the returned TLS error message to return `access_denied` on authorization failures. The logic would proceed to return `bad_certificate` on authorization failures. Revert to HTTP authorization to be able to return a user-friendly return values, which are correct.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
@DavidHurta: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Testing for 9ad1d4eStandalone OpenShiftPrometheus can scrape using mTLSClient needs to provide its certificateThe CN is verified correctlyThe metrics-server Pod has its client certificate signed by the same issuer as does Prometheus. Let's try to scrape the CVO using the metric-server certs. I am using CA is verified during the TLS handshakeUsing different certs from the metrics-server Pod: Checking the CVO logs:The logs need a higher verbosity level. Switching to TechPreviewNoUpgrade feature set, switching to Trace log level, restarting the CVO, and running: HyperShiftFor information, see a previous comment. Prometheus output using PromeCIeus from a 
|
|
/test e2e-agnostic-ovn |
3 participants
This pull request proposes to migrate the CVO metrics endpoint from bearer token authentication to mutual TLS (mTLS) to comply with OpenShift metrics conventions.
Key changes:
k8s.io/apiserver/pkg/server/dynamiccertificatespackage to simplify logicBreaking Changes: