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 tokens to TLS client certificates with CN-based authorization, added dynamic serving-certificate and client-CA controllers, introduced a public MetricsOptions API and updated RunMetrics/createHttpServer signatures; updated ServiceMonitor, go.mod, tests, and startup flag wiring. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing touches
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. |
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 |
openshift-merge-robot
removed
the
needs-rebase
…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]. [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>
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.
The previous approach of checking the client certificate on every HTTP request was redundant. The certificate remains constant for the entire session duration, which can span days with TLS session tickets and resumptions. Optimize authorization by verifying the CN once during the TLS handshake instead. This moves the authorization from the application onto the TLS layer. This can result in harder debugging due to the TLS layer failing without providing detailed information. For example: ``` curl: (56) OpenSSL SSL_read: error:0A000412:SSL routines::sslv3 alert bad certificate, errno 0 command terminated with exit code 56 ``` However, it also makes the current authorization mechanism more explicit. The previous mechanism could infer that every HTTP request was being authorized, but that was not the case in reality. Some alternatives may be: - Disable session tickets and the resumption support to force more frequent authorizations. - Verify the client's certificate in the application layer, but add useful logic, such as checking for the certificate expiration date per every HTTP request. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
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
force-pushed
the
metrics-mtls
branch
from
78d5349 to
64e28ff
Compare
|
/test ci/prow/unit A flake? |
|
@DavidHurta: The specified target(s) for 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. |
|
/test unit |
|
@DavidHurta: all tests passed! 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. |
|
/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 |
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: