OCPBUGS-66898: Implement mTLS authentication and authorization for CVO metrics endpoint#1271
OCPBUGS-66898: Implement mTLS authentication and authorization for CVO metrics endpoint#1271DavidHurta wants to merge 12 commits intoopenshift:mainfrom
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughSwitched metrics auth from bearer tokens to TLS client certificates with CN-based authorization; introduced dynamic serving-cert and client-CA controllers; added public MetricsOptions and updated RunMetrics/createHttpServer signatures; updated ServiceMonitor, go.mod, tests, startup flags, and call sites. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/test ? |
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 |
DavidHurta
force-pushed
the
metrics-mtls
branch
from
b0454ca to
123ba67
Compare
|
/test all |
|
@coderabbitai review |
|
/hold Developing and testing. |
openshift-ci
bot
added
the
do-not-merge/hold
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/cvo/metrics.go`:
- Around line 206-209: The HTTP handler in metrics.go is returning the rejected
common name in the response body (commonName vs metricsAllowedClientCommonName),
which can leak internal names; change the http.Error call to return a generic
message like "access denied" or "unauthorized" while keeping the detailed
klog.V(4).Infof that logs the actual commonName for internal debugging; ensure
status remains http.StatusForbidden and do not include commonName in any
client-facing response (leave klog line unchanged or adjust verbosity if
needed).
🧹 Nitpick comments (1)
pkg/cvo/metrics.go (1)
197-215: Hardcoded authorized CN limits extensibility.The
metricsAllowedClientCommonNameconstant is defined inline within theauthorizefunction. While this is acceptable for the current use case (only Prometheus scraping), consider extracting it to a package-level constant or making it configurable viaMetricsOptionsif future requirements may need additional authorized clients.
DavidHurta
force-pushed
the
metrics-mtls
branch
from
267e8cc to
5f02932
Compare
|
/retest |
DavidHurta
force-pushed
the
metrics-mtls
branch
from
5f02932 to
5685780
Compare
|
/unhold |
|
/test okd-scos-images |
openshift-ci
bot
removed
the
do-not-merge/hold
hongkailiu
left a comment
hongkailiu
left a comment
There was a problem hiding this comment.
Only a question about a0b08df.
Others are just NITs and remarks.
|
/lgtm |
…updates This commit's goal is to prepare the existing code for mTLS support. In OpenShift, core operators SHOULD require authentication, and they SHOULD support TLS client certificate authentication [1]. They also SHOULD support local authorization and SHOULD allow the well-known metrics scraping identity [1]. To achieve this, an operator must be able to verify a client's certificate. To do this, the certificate can be verified using the certificate authority (CA) bundle located in a ConfigMap in the kube-system namespace [2]. This would entail an implementation of a new controller to watch the ConfigMap for changes. To avoid such implementation to avoid potential bugs and future maintenance, my goal is to utilize the `k8s.io/apiserver/pkg/server/dynamiccertificates` package for this goal as the package provides a functionality for this specific use case. While doing so, we can also rework the existing, a bit complex, implementation and utilize the package for existing use cases as well to simplify the logic and use an existing, well-tested library. [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 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
In OpenShift, core operators SHOULD require authentication and they SHOULD support TLS client certificate authentication [1]. They also SHOULD support local authorization and SHOULD allow the well-known metrics scraping identity [1]. To achieve this, an operator must be able to verify a client's certificate. To do this, the certificate can be verified using the certificate authority (CA) bundle located at the client-ca-file key of the kube-system/extension-apiserver-authentication ConfigMap [2]. Guarantee failed connections when the config from the GetConfigForClient method is nil to ensure connections are only using the TLS config from the serving cert controller. [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 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…arer token The paths are specified at [1]. [1]: https://github.com/openshift/enhancements/blob/master/CONVENTIONS.md
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>
The `o.HyperShift` option is not available in older release branches. In newer branches, the option can be utilized.
In HyperShift, the CVO currently needs to have disabled both authorization and authentication. Ensure the aspects are disabled so as not break HyperShift. However, in the future, the authentication will be enabled using mTLS and a mounted CA bundle file. Thus, authentication needs to be configurable. Authorization needs to be configurable as well because HyperShift allows a custom monitoring stack to scrape hosted control plane components. In the future in HyperShift, authentication of the metrics endpoint of the CVO will be enforced; however, the authorization will be disabled. This commit prepares the code for these changes.
This is done to provide HTTP return values in failures to comply with the origin test suite. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
DavidHurta
force-pushed
the
metrics-mtls
branch
from
5685780 to
62f88dd
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DavidHurta, hongkailiu 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 |
|
@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. |
6 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: