WINC-1988: TLS profile adherence - #4340
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. 🚫 Excluded labels (none allowed) (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe operator reads the cluster Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (17 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
Bot
added
the
approved
mansikulkarni96
changed the title
|
@mansikulkarni96: This pull request references WINC-1988 which is a valid jira issue. 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-robot
added
the
jira/valid-reference
|
@code-rabbit review |
|
@code-rabbit full review |
|
/test aws-e2e-operator |
|
/test lint |
|
@mansikulkarni96: 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. |
jrvaldes
left a comment
jrvaldes
left a comment
There was a problem hiding this comment.
@mansikulkarni96 thanks for working on this, mostly LGTM.
PTAL at the comments.
|
|
||
| // Convert TLS profile to config function | ||
| tlsConfigFn, unsupportedCiphers := tlspkg.NewTLSConfigFromProfile(tlsProfile) | ||
| if len(unsupportedCiphers) > 0 { |
There was a problem hiding this comment.
should this log a WARN instead?
There was a problem hiding this comment.
There is no log level for WARN with logr, do you expect this to be less visible than info?
| } | ||
|
|
||
| setupLog.Info("TLS configuration loaded", | ||
| "minVersion", tlsProfile.MinTLSVersion, |
There was a problem hiding this comment.
windows-exporter may need min_version and cipher_suites on it TLS configuration
as it serve metrics over HTTPS
There was a problem hiding this comment.
not a blocker, consider a separate PR/jira for this.
| // cache type. | ||
| mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{ | ||
| Scheme: scheme, | ||
| Metrics: metricsserver.Options{ |
There was a problem hiding this comment.
the proposal only sets MinVersion and CipherSuites. It does not set NextProtos. Without this, the metrics server falls back to Go's default
ALPN behavior (which is implementation-defined per Go version).
The doc reference explicitly states:
▎ Set NextProtos / ALPN yourself — the cluster profile does not set it.
There was a problem hiding this comment.
controller-runtime's metrics server already sets NextProtos: []string{"h2"} in server.go:279 before calling our TLSOpts function. Since NewTLSConfigFromProfile only sets MinVersion and CipherSuites, the
existing NextProtos is preserved. No additional setting needed on our side.
There was a problem hiding this comment.
There was a major issue identified with http2 (rapid reset CVE) a few years ago. The mitigation most teams pursued was to disable http2 entirely.
See openshift/controller-runtime-common#21 (just merged)
There was a problem hiding this comment.
can I resolve this comment?
@mansikulkarni96 do not resolve/close comments in PRs where you are the author, just reply with your feedback, and leave the resolution of the comment to the owner of the comment.
cc @openshift/openshift-team-windows-containers
There was a problem hiding this comment.
@jrvaldes rest of the comments resolved were addressed and pending resolution by you. Do you want me to unresolve all of them for you to take a look?
| SecureServing: true, | ||
| FilterProvider: filters.WithAuthenticationAndAuthorization, | ||
| TLSOpts: []func(*tls.Config){tlsConfigFn}, // Apply cluster TLS profile | ||
| }, |
There was a problem hiding this comment.
curve preferences PR merged, consider setting CurvePreferences to include X25519MLKEM768 for post-quantum key exchange
There was a problem hiding this comment.
only the enhancement merged, implementation is pending https://github.com/openshift/controller-runtime-common/pull/22/changes
There was a problem hiding this comment.
Add TLS strict obedience for PQC readiness
consider re-wording the PR description, the one thing that actually matters for post-quantum readiness is the hybrid KEM group X25519MLKEM768 and isn't applied in this change.
the MinVersion and CipherSuites don't get PQC on their own.
I'd rather see the description say plainly "lays the groundwork, curve preferences to follow once controller-runtime-common#22 lands" and get a Jira follow-up filed and linked now while the context's fresh
dc4efd2 to
41196ff
Compare
41196ff to
41253d6
Compare
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/operator/main.go`:
- Line 148: Update the TLS setup around NewTLSConfigFromProfile so the returned
tls.Config applies the profile’s CurvePreferences, including X25519MLKEM768 when
supported by the repository’s Go version. Handle FIPS mode by excluding
unsupported or disallowed groups while preserving the profile’s MinVersion and
CipherSuites and existing unsupported-cipher behavior.
In `@pkg/cluster/config.go`:
- Around line 138-154: Add adjacent table-driven tests covering the config
initialization path around APIServer retrieval and TLS setup: assert successful
storage of the resolved TLS profile and TLS adherence policy, require an error
when APIServers().Get fails, and require an error for an invalid custom profile
from tlspkg.GetTLSProfileSpec. Use testify/assert and require consistently, and
keep the tests focused on the APIServer TLS contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 8196c4ee-2f0b-4d00-a9cb-6bedc8de9483
⛔ Files ignored due to path filters (12)
go.sumis excluded by!**/*.sum,!go.sumvendor/github.com/openshift/controller-runtime-common/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/controller-runtime-common/pkg/tls/controller.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/controller-runtime-common/pkg/tls/tls.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/library-go/pkg/crypto/OWNERSis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/library-go/pkg/crypto/cert_config.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/library-go/pkg/crypto/crypto.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/library-go/pkg/crypto/keygen.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/library-go/pkg/crypto/options.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/library-go/pkg/crypto/rotation.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/library-go/pkg/crypto/tls_adherence.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (5)
bundle/manifests/windows-machine-config-operator.clusterserviceversion.yamlcmd/operator/main.goconfig/rbac/role.yamlgo.modpkg/cluster/config.go
| "adherencePolicy", tlsAdherence) | ||
|
|
||
| // Convert TLS profile to config function | ||
| tlsConfigFn, unsupportedCiphers := tlspkg.NewTLSConfigFromProfile(tlsProfile) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
Apply TLS profile groups before claiming strict profile adherence.
NewTLSConfigFromProfile sets MinVersion and CipherSuites, but it does not set CurvePreferences. The current OpenShift TLS profile includes allowed groups, including the PQC hybrid group X25519MLKEM768. The metrics server can therefore retain Go default groups instead of the cluster profile. Update the helper dependency or configure supported curve preferences, with FIPS handling. (github.com)
For the Go version declared by this repository, verify which crypto/tls CurveID values support X25519MLKEM768 and how FIPS mode affects its availability.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/operator/main.go` at line 148, Update the TLS setup around
NewTLSConfigFromProfile so the returned tls.Config applies the profile’s
CurvePreferences, including X25519MLKEM768 when supported by the repository’s Go
version. Handle FIPS mode by excluding unsupported or disallowed groups while
preserving the profile’s MinVersion and CipherSuites and existing
unsupported-cipher behavior.
Source: MCP tools
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo="openshift/windows-machine-config-operator"
pr="4340"
echo "== Relevant PR review comments =="
gh api --paginate "repos/${repo}/pulls/${pr}/comments?per_page=100" \
--jq '.[] | select(.id == 3708467856 or .id == 3609640829) | {id, user: .user.login, path, line, body, html_url}'
echo
echo "== Go version and pinned controller-runtime-common dependency =="
rg -n -C 2 '(^go |controller-runtime-common)' go.mod
echo
echo "== TLS helper source availability and relevant API references =="
fd -a 'tls\.go$' . | while IFS= read -r f; do
if rg -q 'NewTLSConfigFromProfile|SecurityProfileWatcher' "$f"; then
echo "--- $f"
rg -n -C 4 'NewTLSConfigFromProfile|SecurityProfileWatcher|CurvePreferences' "$f"
fi
done
echo
echo "== TLS profile use in current PR =="
rg -n -C 4 'NewTLSConfigFromProfile|TLSProfileSpec|TLSAdherencePolicy|CurvePreferences' \
cmd/operator/main.go pkg/cluster/config.go 2>/dev/null || trueLength of output: 10974
🏁 Script executed:
#!/bin/bash
set -euo pipefail
helper_url="https://raw.githubusercontent.com/openshift/controller-runtime-common/64ee174f5e2ebc630fbb554dd114d7a7a878693f/pkg/tls/tls.go"
helper_file="$(mktemp)"
curl --fail --silent --show-error --location "$helper_url" -o "$helper_file"
echo "== Pinned controller-runtime-common TLS helper =="
rg -n -C 8 'NewTLSConfigFromProfile|ShouldHonorClusterTLSProfile|CurvePreferences|TLSAdherence|FIPS' "$helper_file"
echo
echo "== TLS types and adherence-policy helpers in declared OpenShift API module cache =="
module_dir="$(go env GOPATH 2>/dev/null)/pkg/mod/github.com/openshift/api@v0.0.0-20260729135743-72624b98ff3d"
if [ -d "$module_dir" ]; then
rg -n -C 5 'type TLSProfileSpec|type TLSProfile|TLSAdherencePolicy|ShouldHonorClusterTLSProfile|CurvePreferences' \
"$module_dir/config" "$module_dir/config/v1" 2>/dev/null || true
else
echo "OpenShift API module source is unavailable in the sandbox."
fiLength of output: 4428
@jrvaldes, reviewed. The two comments are related, but they are not duplicates.
The linked discussion identifies the required TLSAdherencePolicy gate. cmd/operator/main.go currently applies tlsConfigFn unconditionally.
This finding identifies incomplete profile application after enforcement is enabled. The pinned NewTLSConfigFromProfile explicitly does not set tls.Config.CurvePreferences:
// TODO: add curve preferences from profile once https://github.com/openshift/api/pull/2583 merges.
// tlsConf.CurvePreferences <<<<<< profile.CurvesTherefore, the implementation needs both controls:
- Apply the TLS profile only when
ShouldHonorClusterTLSProfile(tlsAdherence)requires it. - When it applies the profile, configure supported curve preferences, including the permitted PQC group where available and allowed by FIPS.
I will keep this finding open because the gate does not address the missing TLS group configuration.
🧬 Code Graph Analysis Results
pkg/cluster/config.go:51-62, 97-103, 106-156, 185-194, 360-362
Defines the cluster.Config interface and its TLS, network, validation, and proxy-related methods. NewConfig retrieves cluster network, platform, and APIServer TLS configuration, returning wrapped errors. Validate validates Kubernetes version and network configuration.
pkg/nodeconfig/payload/payload.go:310-320, 366-380
PopulateSHAMap reads and parses /payload/sha256sum; PopulateNetworkConfScript generates the network configuration script, records its SHA256 checksum, creates the target archive, and returns wrapped file-creation errors.
pkg/nodeconfig/payload/payload.go:21-84
Defines the payload file-path constants used by checkIfRequiredFilesExist in cmd/operator/main.go.
pkg/logconfig/logconfig.go:64-72, 78-103
ValidateLogConfig validates log rotation environment variables and returns aggregated errors; LogConfig logs configured values when present.
pkg/certificatesigningrequests_controller.go:58-74, 134-152
Creates the CSR reconciler using manager dependencies and cluster service CIDR, then configures its controller watch predicates for pending CSRs.
You are interacting with an AI system.
There was a problem hiding this comment.
This will be addressed in a follow-up when controller-runtime-common#22 merges
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| // Fetch TLS configuration from APIServer | ||
| apiServer, err := oclient.ConfigV1().APIServers().Get(ctx, "cluster", meta.GetOptions{}) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error getting APIServer configuration: %w", err) | ||
| } | ||
| tlsProfileSpec, err := tlspkg.GetTLSProfileSpec(apiServer.Spec.TLSSecurityProfile) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error getting TLS profile from APIServer: %w", err) | ||
| } | ||
|
|
||
| return &config{ | ||
| oclient: oclient, | ||
| operatorClient: operatorClient, | ||
| network: network, | ||
| platform: platformStatus.Type, | ||
| oclient: oclient, | ||
| operatorClient: operatorClient, | ||
| network: network, | ||
| platform: platformStatus.Type, | ||
| tlsProfileSpec: tlsProfileSpec, | ||
| tlsAdherencePolicy: apiServer.Spec.TLSAdherence, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Add table-driven tests for the APIServer TLS contract.
Test successful profile and policy storage. Test APIServer retrieval failure. Test invalid custom profile handling. This startup path now controls manager TLS behavior.
As per path instructions, “Add or update adjacent table-driven tests using testify/assert and require.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/cluster/config.go` around lines 138 - 154, Add adjacent table-driven
tests covering the config initialization path around APIServer retrieval and TLS
setup: assert successful storage of the resolved TLS profile and TLS adherence
policy, require an error when APIServers().Get fails, and require an error for
an invalid custom profile from tlspkg.GetTLSProfileSpec. Use testify/assert and
require consistently, and keep the tests focused on the APIServer TLS contract.
Source: Path instructions
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '=== Relevant Go files ==='
fd -t f -e go . pkg/cluster | sort
printf '%s\n' '=== Production file outline ==='
ast-grep outline pkg/cluster/config.go --items all --type function,interface,struct --match 'NewConfig|Config|config' || true
printf '%s\n' '=== Relevant test symbols and assertions ==='
rg -n -C 4 -g '*_test.go' \
'NewConfig\s*\(|APIServers\(\)\.Get\s*\(|TLSProfileSpec\s*\(|TLSAdherencePolicy\s*\(|GetTLSProfileSpec\s*\(' \
pkg/cluster || true
printf '%s\n' '=== Current PR diff for cluster config and tests ==='
git diff --no-ext-diff --unified=60 'origin/main...HEAD' -- pkg/cluster/config.go pkg/cluster/*_test.go 2>/dev/null || \
git diff --no-ext-diff --unified=60 -- pkg/cluster/config.go pkg/cluster/*_test.goLength of output: 1814
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '=== Test file outline ==='
ast-grep outline pkg/cluster/config_test.go --items all --type function --view expanded
printf '%s\n' '=== Test file content ==='
cat -n pkg/cluster/config_test.go
printf '%s\n' '=== Repository state ==='
git status --short
printf '%s\n' '=== HEAD metadata ==='
git show -s --format='%H%n%s%n%D' HEAD
printf '%s\n' '=== Changed paths relative to HEAD parent ==='
git diff-tree --no-commit-id --name-status -r HEADLength of output: 12324
@jrvaldes, I reviewed the current test coverage.
The finding remains valid. pkg/cluster/config_test.go has no tests for the NewConfig APIServer TLS path.
Please add table-driven tests that cover:
- successful storage of
TLSProfileSpec()andTLSAdherencePolicy(); - an error from
APIServers().Get; - an invalid custom TLS profile returned to
tlspkg.GetTLSProfileSpec.
This path configures the metrics server TLS behavior at operator startup.
You are interacting with an AI system.
There was a problem hiding this comment.
updated added tests.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
updated, added tests
jrvaldes
left a comment
jrvaldes
left a comment
There was a problem hiding this comment.
/lgtm
@mansikulkarni96 thanks for working on this. LGTM so far.
Did you considered added a e2e test to check that the metrics server actually negotiates with the expected MinVersion/CipherSuites, or that the pod actually restarts when the APIServer CR changes?
|
@rphillips is this something you can help reviewing from the Staff Eng standpoint? |
|
/remove-approve for Staff Eng sign-off |
|
/remove-approve |
|
/approve cancel |
41253d6 to
aafc48c
Compare
|
New changes are detected. LGTM label has been removed. |
|
|
||
| // Get TLS configuration from cluster config (fetched from APIServer during NewConfig) | ||
| tlsProfile := clusterConfig.TLSProfileSpec() | ||
| tlsAdherence := clusterConfig.TLSAdherencePolicy() |
There was a problem hiding this comment.
@mansikulkarni96 are you planning squash this commit?
|
@mansikulkarni96 please post a comment with |
26c2ce9 to
c1b0600
Compare
|
/remove-approve |
openshift-ci
Bot
removed
the
approved
Enforce the cluster-wide TLS security profile on the WMCO metrics server, gated on the TLS adherence policy. WMCO was not previously honoring the cluster TLS profile, so enforcement only applies when the adherence policy is StrictAllComponents, per the centralized TLS config enhancement. Changes: - Fetch TLS profile and adherence policy from apiserver.config.openshift.io/cluster - Gate enforcement using library-go's ShouldHonorClusterTLSProfile - Disable HTTP/2 (HTTP/1.1 only) as defense-in-depth against CVE-2023-44487 - Watch for TLS profile and adherence policy changes, restart on change - Add controller-runtime-common to hack/update_submodules.sh CurvePreferences with X25519MLKEM768 for post-quantum key exchange will be added in a follow-up once openshift/controller-runtime-common#22 lands.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
c1b0600 to
a2c3cb3
Compare
4 participants
Add TLS strict obedience for PQC readiness
Fetch TLS configuration from cluster APIServer and apply to metrics server.
Watch for TLS profile changes and restart pod to reload config.
Uses SecurityProfileWatcher from controller-runtime-common following the pattern from machine-api-operator. Satisfies OpenShift 5.0 TLS.
Strict Obedience requirement for centralized TLS management.
Summary by CodeRabbit
New Features
Bug Fixes