UPSTREAM: <carry>: NE-2744: Remove hardcoded TLS ciphers and set PQC …

[davidesalerno]

…curve preferences

The setTLSDefaults function hardcoded a MaxVersion of TLS 1.3 and a list of ECDHE-only cipher suites (with duplicate entries). This prevented Go's crypto/tls from negotiating post-quantum key exchange (ML-KEM) and bypassed OpenShift's dynamic TLS security profiles.

Remove the hardcoded MaxVersion and CipherSuites to let Go's standard library manage cipher negotiation with its secure defaults. Set explicit CurvePreferences matching the OpenShift Intermediate TLS profile (X25519MLKEM768, X25519, P-256, P-384) to align with the TLSGroupPreferences feature gate introduced in openshift/api#2583.

1. Why is this pull request needed and what does it do?

OpenShift's PQC (Post-Quantum Cryptography) readiness requires all core network components to adopt dynamic TLS policies. An audit (NE-2744) found that CoreDNS's shared TLS library (plugin/pkg/tls/tls.go) hardcodes a MaxVersion of TLS 1.3 and 9 ECDHE-only cipher suites (3 of which are duplicates). These hardcoded values bypass OpenShift's centralized TLS security profiles and prevent negotiation of ML-KEM post-quantum key exchange - even though Go 1.24+ supports it natively.

2. Which issues (if any) are related?

This PR is related to NE-2744

3. Which documentation changes (if any) need to be made?

None. This change modifies internal TLS defaults with no impact on user-facing configuration or APIs.

4. Does this introduce a backward incompatible change or deprecation?

No, this does not introduce a backward incompatible change or deprecation.

Summary by CodeRabbit

• Bug Fixes
  • Updated default TLS configuration to use an explicit, ordered set of curve preferences (including X25519MLKEM768, X25519, P256, and P384).
  • Refined default TLS settings to better align with modern, secure defaults while preserving the configured TLS version bounds.
• Tests
  • Added a unit test covering the expected TLS default values to prevent regressions in cipher suites, versions, and curve preferences.

[openshift-ci-robot]

@davidesalerno: This pull request references NE-2744 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 story to target the "5.0.0" version, but no target version was set.

Details

In response to this:

…curve preferences

The setTLSDefaults function hardcoded a MaxVersion of TLS 1.3 and a list of ECDHE-only cipher suites (with duplicate entries). This prevented Go's crypto/tls from negotiating post-quantum key exchange (ML-KEM) and bypassed OpenShift's dynamic TLS security profiles.

Remove the hardcoded MaxVersion and CipherSuites to let Go's standard library manage cipher negotiation with its secure defaults. Set explicit CurvePreferences matching the OpenShift Intermediate TLS profile (X25519MLKEM768, X25519, P-256, P-384) to align with the TLSGroupPreferences feature gate introduced in openshift/api#2583.

1. Why is this pull request needed and what does it do?

OpenShift's PQC (Post-Quantum Cryptography) readiness requires all core network components to adopt dynamic TLS policies. An audit (NE-2744) found that CoreDNS's shared TLS library (plugin/pkg/tls/tls.go) hardcodes a MaxVersion of TLS 1.3 and 9 ECDHE-only cipher suites (3 of which are duplicates). These hardcoded values bypass OpenShift's centralized TLS security profiles and prevent negotiation of ML-KEM post-quantum key exchange - even though Go 1.24+ supports it natively.

2. Which issues (if any) are related?

This PR is related to NE-2744

3. Which documentation changes (if any) need to be made?

None. This change modifies internal TLS defaults with no impact on user-facing configuration or APIs.

4. Does this introduce a backward incompatible change or deprecation?

No, this does not introduce a backward incompatible change or deprecation.

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]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 82757332-e86e-4b7e-90a4-ddd93403e819

📥 Commits

Reviewing files that changed from the base of the PR and between 1c46d3b and 83e30aa.

📒 Files selected for processing (2)

• plugin/pkg/tls/tls.go
• plugin/pkg/tls/tls_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• plugin/pkg/tls/tls.go

---

Walkthrough

setTLSDefaults now configures an ordered TLS curve preference list, and TestSetTLSDefaults verifies the resulting TLS config values from NewTLSConfig, including version bounds, cipher suite count, and curve ordering.

Changes

TLS Defaults Update

Layer / File(s)	Summary
Defaults configuration plugin/pkg/tls/tls.go	setTLSDefaults sets CurvePreferences to X25519MLKEM768, X25519, P256, and P384 in order.
Defaults test plugin/pkg/tls/tls_test.go	Adds crypto/tls and TestSetTLSDefaults, which asserts MinVersion, MaxVersion, CipherSuites length, and the exact CurvePreferences returned by NewTLSConfig.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 14 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (14 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately reflects the main change: removing hardcoded TLS ciphers and enabling PQC-oriented defaults.
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	The added test is a static Go unit test name (TestSetTLSDefaults); no Ginkgo titles or dynamic values appear in the changed test file.
Test Structure And Quality	✅ Passed	The added test is a simple stdlib unit test with isolated setup via t.TempDir, meaningful messages, and no cluster/timeouts/Ginkgo concerns.
Microshift Test Compatibility	✅ Passed	Only standard unit tests were added in plugin/pkg/tls; no Ginkgo e2e tests or MicroShift-unsupported OpenShift APIs/features are referenced.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	This PR adds only a standard Go unit test (TestSetTLSDefaults) in plugin/pkg/tls/tls_test.go, not Ginkgo e2e tests. The check applies only to Ginkgo e2e tests, so it is not applicable.
Topology-Aware Scheduling Compatibility	✅ Passed	Only plugin/pkg/tls TLS-default code and tests changed; no manifests, controllers, replicas, affinity, selectors, or topology-aware scheduling logic were introduced.
Ote Binary Stdout Contract	✅ Passed	No process-level stdout writes were added; the diff only changes TLS defaults and a normal unit test, with no main/init/TestMain/BeforeSuite hooks.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	Only a standard Go unit test was added; no Ginkgo e2e tests, IPv4-only assumptions, or external connectivity requirements were found.
No-Weak-Crypto	✅ Passed	Touched TLS code uses modern TLS ciphers/curves only; no MD5/SHA1/DES/RC4/3DES/Blowfish/ECB, custom crypto, or unsafe secret comparisons were added.
Container-Privileges	✅ Passed	Only plugin/pkg/tls/tls.go and tls_test.go changed; no container/K8s manifests or privilege flags (privileged, hostPID/network/IPC, SYS_ADMIN, allowPrivilegeEscalation) were found.
No-Sensitive-Data-In-Logs	✅ Passed	No new logging was added in plugin/pkg/tls; only TLS defaults and tests changed, and error returns don’t expose sensitive data.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign thealisyed for approval. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[candita]

IIuc, this is only an UPSTREAM: <carry> if it exists upstream first.

[candita]

MaxVersion should not have any impact on this. Please keep it in.

[davidesalerno]

I removed MaxVersion since the crypto/tls documentation states that when MaxVersion is zero, Go uses the highest version it supports (currently TLS 1.3).

Removing it was unnecessary for the PQC goal and even if it It would allow us to avoid having to update anything should Go's maximum supported version become 1.4 at same time it could introduced risk for no benefit.

I'm restoring it in the next push.

[candita]

We may still need the cipher suites if TLS v1.2 is being used. Why remove them entirely?

[davidesalerno]

You're right, since MinVersion is still TLS 1.2 we need those cipher suites for TLS 1.2 negotiation. I probably over-scoped the change: the only thing needed for ML-KEM support is adding CurvePreferences. I've restored the CipherSuites (and also removed 3 duplicate entries that were copy-paste artifacts) list and the updated diff now only adds CurvePreferences on top of the existing defaults.

[davidesalerno]

IIuc, this is only an UPSTREAM: <carry> if it exists upstream first.

Looking at the repo history, UPSTREAM: is used for all downstream-only patches that persist across rebases (e.g., the ocp_dnsnameresolver plugin, vendor tracking, go-version bumps). This change fits that pattern - since it's an OpenShift-specific TLS default we'll want to carry forward.

Happy to change it if I misunderstood the commit message conventions and it is better to use a different convention though.

[openshift-ci]

@davidesalerno: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws-ovn	83e30aa	link	true	/test e2e-aws-ovn

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.