OCPBUGS-77256: Implement mTLS authentication and authorization for CVO metrics endpoint#1326
Open
DavidHurta wants to merge 12 commits intoopenshift:release-4.21from
Open
OCPBUGS-77256: Implement mTLS authentication and authorization for CVO metrics endpoint#1326DavidHurta wants to merge 12 commits intoopenshift:release-4.21from
DavidHurta wants to merge 12 commits intoopenshift:release-4.21from
Conversation
(cherry picked from commit f77bc1e)
…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> (cherry picked from commit 2a432a6)
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> (cherry picked from commit 622e335)
…arer token The paths are specified at [1]. [1]: https://github.com/openshift/enhancements/blob/master/CONVENTIONS.md (cherry picked from commit 81f3bb5)
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> (cherry picked from commit 18495ae)
The `o.HyperShift` option is not available in older release branches. In newer branches, the option can be utilized. (cherry picked from commit 554fea0)
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. (cherry picked from commit 3519037)
(cherry picked from commit 2f736b5)
(cherry picked from commit 3cd9e22)
(cherry picked from commit e47ae18)
(cherry picked from commit b68827c)
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> (cherry picked from commit 62f88dd)
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 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:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[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
Contributor
Author
|
/jira cherrypick OCPBUGS-66898 |
Contributor
|
@DavidHurta: Jira Issue OCPBUGS-66898 has been cloned as Jira Issue OCPBUGS-77256. Will retitle bug to link to clone. 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. |
openshift-ci
bot
changed the title
openshift-ci-robot
added
jira/severity-moderate
Contributor
|
@DavidHurta: This pull request references Jira Issue OCPBUGS-77256, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
DavidHurta
changed the title
Contributor
Author
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
Contributor
Author
|
/retest |
Contributor
|
@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. |
Member
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
Contributor
|
@wking: This pull request references Jira Issue OCPBUGS-77256, which is valid. The bug has been moved to the POST state. 7 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. |
Contributor
|
/jira refresh |
Contributor
|
@jiajliu: This pull request references Jira Issue OCPBUGS-77256, which is valid. 7 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. |
|
Test Scenario's: Prometheus should not scrape with bearer token. Client key is required with CA bundle |
|
Test Scenario: Verify CVO endpoint cluster-version-operator.openshift-cluster-version.svc_9099 is working well with new mTLS authentication. used code from OTP#28753 |
|
/verified by @dis016 |
openshift-ci-robot
added
the
verified
Contributor
|
@dis016: This PR has been marked as verified by 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A manual backport of #1271 due to conflicts in the
pkg/cvo/metrics.gofile caused by #1299 when applying thepkg/cvo/metrics: Utilize dynamiccertificates package for certificate updatescommit.