Skip to content

CORENET-7497: Honor cluster TLS profile - #135

Open
alebedev87 wants to merge 2 commits into
openshift:mainfrom
alebedev87:tls-consistency
Open

alebedev87 wants to merge 2 commits into
openshift:mainfrom
alebedev87:tls-consistency

Conversation

@alebedev87

@alebedev87 alebedev87 commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

This makes the operator honor the cluster-wide TLS security profile (apiservers.config.openshift.io/cluster) on its own TLS servers (metrics and webhook), instead of always serving with the controller-runtime defaults.

At startup the operator reads apiservers/cluster and, when tlsAdherence is Strict, translates the cluster tlsSecurityProfile into tls.Config options — minimum TLS version, cipher suites, and the group/curve preferences (CurvePreferences) — which are appended to the manager's TLSOpts. When tlsAdherence is Legacy (or the APIServer/config API is unavailable, e.g. on non-OpenShift clusters), no options are applied and the servers keep their existing defaults, so the change degrades gracefully.

A watch on the profile is also registered: if the tlsSecurityProfile or tlsAdherence changes at runtime, the operator cancels the manager context and exits, letting the Deployment restart the pod so the new profile takes effect. main is restructured to share a single cancelable context and client between the profile lookup, the watch, and mgr.Start.

Honoring the profile's group/curve preferences requires the newer controller-runtime-common, whose NewTLSConfigFromProfile helper sets CurvePreferences. Adopting that version transitively bumps k8s.io/* to 0.36 and sigs.k8s.io/controller-runtime to 0.24.

🤖 Generated with Claude Code

Test on 4.22

# before featuregate was enabled
artifacts-before$ ag api_server_tls_config_compliance -A4 results.json 
104:          "api_server_tls_config_compliance": {
105-            "configured_profile": "Intermediate",
106-            "version": true,
107-            "ciphers": false
108-          },

# after Strict TLS adherence was set in APIServer
artifacts-after$ ag api_server_tls_config_compliance -A4 results.json 
66:          "api_server_tls_config_compliance": {
67-            "configured_profile": "Intermediate",
68-            "version": true,
69-            "ciphers": true
70-          },

# after Legacy TLS adherence was set in APIServer
artifacts-legacy$ ag api_server_tls_config_compliance -A4 results.json 
72:          "api_server_tls_config_compliance": {
73-            "configured_profile": "Intermediate",
74-            "version": true,
75-            "ciphers": false
76-          },

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Sep 15, 2026
@openshift-ci-robot

openshift-ci-robot commented Sep 15, 2026

Copy link
Copy Markdown

@alebedev87: This pull request references CORENET-7497 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "5.1.0" version, but no target version was set.

Details

In response to this:

This makes the operator honor the cluster-wide TLS security profile (apiservers.config.openshift.io/cluster) on its own TLS servers (metrics and webhook), instead of always serving with the controller-runtime defaults.

At startup the operator reads apiservers/cluster and, when tlsAdherence is Strict, translates the cluster tlsSecurityProfile into tls.Config options — minimum TLS version, cipher suites, and the group/curve preferences (CurvePreferences) — which are appended to the manager's TLSOpts. When tlsAdherence is Legacy (or the APIServer/config API is unavailable, e.g. on non-OpenShift clusters), no options are applied and the servers keep their existing defaults, so the change degrades gracefully.

A watch on the profile is also registered: if the tlsSecurityProfile or tlsAdherence changes at runtime, the operator cancels the manager context and exits, letting the Deployment restart the pod so the new profile takes effect. main is restructured to share a single cancelable context and client between the profile lookup, the watch, and mgr.Start.

Honoring the profile's group/curve preferences requires the newer controller-runtime-common, whose NewTLSConfigFromProfile helper sets CurvePreferences. Adopting that version transitively bumps k8s.io/* to 0.36 and sigs.k8s.io/controller-runtime to 0.24 (from 0.34/0.22).

🤖 Generated with Claude Code

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.

@coderabbitai

coderabbitai Bot commented Sep 15, 2026

Copy link
Copy Markdown

Caution

CodeRabbit couldn't post its review summary.

Error details
Validation Failed: {"resource":"IssueComment","code":"unprocessable","field":"data","message":"Body is too long (maximum is 65536 characters)"} - https://docs.github.com/rest/issues/comments#create-an-issue-comment

@openshift-ci

openshift-ci Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alebedev87

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 15, 2026
@coderabbitai

coderabbitai Bot commented Sep 15, 2026

Copy link
Copy Markdown

Important

Review skipped

We couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting @coderabbitai full review.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: f44dedf5-9700-486a-bc7e-9d83f802382c

📥 Commits

Reviewing files that changed from the base of the PR and between b066ae2 and abe04a2.

📒 Files selected for processing (3)
  • api/v1beta1/bgpcloudconfiguration_types.go
  • api/v1beta1/bgprouting_types.go
  • api/v1beta1/groupversion_info.go

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


📝 Walkthrough

Walkthrough

The operator now loads the OpenShift cluster TLS profile and applies its TLS options to metrics and webhook servers. It watches profile changes and triggers the configured restart callback. Startup uses a shared cancellable context and Kubernetes configuration. CSV declarations and RBAC rules enable TLS profile support and APIServer access. API scheme registration and Go dependencies were updated.

Change Details
TLS profiles Added loading, option construction, watching, and tests.
Startup Registered the OpenShift API scheme and applied profile options.
Permissions Added APIServer read access and enabled TLS profile declarations.
API registration Updated project resource scheme registration.
Dependencies Updated direct and indirect modules.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to abe04

No actionable issue remains in the reviewed changes; the focused test was blocked by unavailable local test infrastructure rather than a code failure.


Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (2 errors, 1 warning)

Check name Status Explanation Resolution
No-Weak-Crypto ❌ Error The PR introduces a runtime path that can enable weak TLS algorithms. internal/tls/profile.go calls NewTLSConfigFromProfile for profiles that ShouldHonorClusterTLSProfile accepts, and `cmd/main.… Filter prohibited cipher suites before creating or applying tls.Config.CipherSuites. Reject or skip 3DES/DES, RC4, and SHA-1-based suites, including weak suites supplied through the Old or Custom cluster profiles. Add tests that exercise …
No-Sensitive-Data-In-Logs ❌ Error The new TLS-profile fetch can log raw Kubernetes client errors. GetProfileInfo wraps client.Get errors with %w in internal/tls/profile.go:53-59, and cmd/main.go:110-113 passes that error to … Sanitize Kubernetes client and watcher errors before logging. Do not pass raw REST or transport errors to setupLog.Error or return them to a controller-runtime logger. Log only a fixed failure message and safe error classification, or red…
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 6 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (12 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed PASS. The pull request adds one test file, internal/tls/profile_test.go. Its four t.Run names are fixed string literals: they contain no generated identifiers, timestamps, node or namespace names,…
Test Structure And Quality ✅ Passed PASS: The pull request adds only internal/tls/profile_test.go as non-vendored test code. It uses Go's testing package and t.Run, not Ginkgo. Each subtest covers one table-driven profile case. Th…
Microshift Test Compatibility ✅ Passed The pull request adds only one test file outside vendor: internal/tls/profile_test.go. It is a standard Go testing unit test with TestGetProfileInfo and t.Run; it does not add Ginkgo It, `De…
Single Node Openshift (Sno) Test Compatibility ✅ Passed PASS. The reviewed range adds only internal/tls/profile_test.go as a test file. It uses Go's testing package with TestGetProfileInfo and t.Run; it does not add Ginkgo It, Describe, `Contex…
Topology-Aware Scheduling Compatibility ✅ Passed The pull request does not introduce topology-dependent scheduling constraints. The non-vendor diff changes TLS profile handling, API scheme registration, RBAC, and CSV TLS metadata. It adds no require…
Ote Binary Stdout Contract ✅ Passed No reviewed change writes to process stdout. cmd/main.go adds only setupLog.Info/Error calls and uses controller-runtime zap, whose base and head defaults set DestWriter to os.Stderr. `interna…
Ipv6 And Disconnected Network Test Compatibility ✅ Passed The pull request adds only internal/tls/profile_test.go as a project test. It uses Go testing.T and t.Run, not Ginkgo e2e constructs. The test uses a controller-runtime fake client and contains …
Container-Privileges ✅ Passed PASS. The pull request changes only TLS feature annotations and RBAC rules for config.openshift.io/apiservers; it does not add privileged: true, hostPID, hostNetwork, hostIPC, SYS_ADMIN, o…
Description check ✅ Passed The description clearly explains how the operator applies and watches the cluster TLS profile for metrics and webhook servers.
Title check ✅ Passed The title accurately and concisely identifies the main change: honoring the cluster TLS profile.
Full details: No-Weak-Crypto

Explanation

The PR introduces a runtime path that can enable weak TLS algorithms. internal/tls/profile.go calls NewTLSConfigFromProfile for profiles that ShouldHonorClusterTLSProfile accepts, and cmd/main.go appends the resulting option to the metrics and webhook TLS options. The newly vendored OpenShift Old profile contains DES-CBC3-SHA and SHA-1-based *-SHA suites. The vendored crypto mapper converts DES-CBC3-SHA to TLS_RSA_WITH_3DES_EDE_CBC_SHA, and the TLS helper assigns the resulting list to tls.Config.CipherSuites. Therefore, a cluster using the Old profile with strict adherence can make the operator serve 3DES and SHA-1 suites. This behavior was not present in the base code, which only applied the existing HTTP/2 option.

Resolution

Filter prohibited cipher suites before creating or applying tls.Config.CipherSuites. Reject or skip 3DES/DES, RC4, and SHA-1-based suites, including weak suites supplied through the Old or Custom cluster profiles. Add tests that exercise the Old profile and a Custom profile and verify that no prohibited suite reaches tls.Config.CipherSuites.

Full details: No-Sensitive-Data-In-Logs

Explanation

The new TLS-profile fetch can log raw Kubernetes client errors. GetProfileInfo wraps client.Get errors with %w in internal/tls/profile.go:53-59, and cmd/main.go:110-113 passes that error to setupLog.Error. Kubernetes REST transport errors can include the request URL and API-server hostname. The new watcher also returns wrapped client errors, which controller-runtime logs through its generic Reconciler error path. This introduces a path that may expose an internal hostname in logs.

Resolution

Sanitize Kubernetes client and watcher errors before logging. Do not pass raw REST or transport errors to setupLog.Error or return them to a controller-runtime logger. Log only a fixed failure message and safe error classification, or redact URL hosts and credentials from all wrapped errors, including watcher reconcile failures.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@alebedev87

Copy link
Copy Markdown
Contributor Author

/test e2e-aws-operator

Watch `apiservers/cluster` and apply `tlsSecurityProfile`, including groups,
when `tlsAdherence` is `Strict`. Honoring the profile's group/curve
preferences requires the newer `controller-runtime-common`, which in turn
bumps k8s to `0.36` and controller-runtime to `0.24`. Mark the CSV as
tls-profiles capable.

Co-Authored-By: Cursor
Co-Authored-By: Claude
The controller-runtime `0.24` bump marks `sigs.k8s.io/controller-runtime/pkg/scheme.Builder`
as deprecated, which `staticcheck` (`SA1019`) flags in the `lint` CI job. The
deprecation asks api packages to depend only on `k8s.io/apimachinery`.

Replace `scheme.Builder` with apimachinery's `runtime.SchemeBuilder` and pass
`GroupVersion` explicitly at registration time via `AddKnownTypes` in each
`_types.go`, keeping the `metav1.AddToGroupVersion` call that the
controller-runtime builder performed implicitly.

Co-Authored-By: Claude
@openshift-ci

openshift-ci Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

@alebedev87: all tests passed!

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants