Skip to content

NE-2744: UPSTREAM: 8227: fix(tls): use Go TLS defaults#194

Open
aswinsuryan wants to merge 1 commit into
openshift:mainfrom
aswinsuryan:aswinsuryan/NE-2744-upstream-tls-defaults
Open

NE-2744: UPSTREAM: 8227: fix(tls): use Go TLS defaults#194
aswinsuryan wants to merge 1 commit into
openshift:mainfrom
aswinsuryan:aswinsuryan/NE-2744-upstream-tls-defaults

Conversation

@aswinsuryan

@aswinsuryan aswinsuryan commented Jul 7, 2026

Copy link
Copy Markdown

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

Cherry-pick of coredns#8227 which removes hardcoded TLS cipher
suites and MaxVersion from setTLSDefaults, delegating to Go crypto/tls
defaults. This enables post-quantum key exchange (ML-KEM/X25519MLKEM768)
supported natively since Go 1.24.

2. Which issues (if any) are related?

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

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

Summary by CodeRabbit

  • Documentation

    • Clarified TLS guidance across several plugin docs, noting that the minimum TLS version is TLS 1.2 and other TLS settings follow Go defaults.
  • Bug Fixes

    • Updated TLS configuration behavior to use the standard minimum TLS version without forcing additional version, cipher suite, or key exchange limits.

Remove fixed TLS 1.2 cipher suite list and maximum TLS version so
crypto/tls can use its maintained defaults. Keep TLS 1.2 as the
minimum supported version.

Document the shared TLS default behavior for plugins that expose TLS
configuration and add coverage to ensure CoreDNS leaves Go-managed
TLS fields unset.

Signed-off-by: Ville Vesilehto <ville@vesilehto.fi>
@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 7, 2026
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 7, 2026
@openshift-ci

openshift-ci Bot commented Jul 7, 2026

Copy link
Copy Markdown

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci-robot

openshift-ci-robot commented Jul 7, 2026

Copy link
Copy Markdown

@aswinsuryan: 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:

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

Cherry-pick of coredns#8227 which removes hardcoded TLS cipher
suites and MaxVersion from setTLSDefaults, delegating to Go crypto/tls
defaults. This enables post-quantum key exchange (ML-KEM/X25519MLKEM768)
supported natively since Go 1.24.

2. Which issues (if any) are related?

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

4. Does this 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

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown

Walkthrough

Modifies TLS configuration defaults by removing explicit MaxVersion and CipherSuites restrictions from setTLSDefaults, keeping only MinVersion TLS1.2, and relying on Go crypto/tls defaults otherwise. Adds nosec annotations, updates tests with default-value assertions, and documents the change across plugin READMEs.

Changes

TLS defaults relaxation

Layer / File(s) Summary
Core TLS default logic
plugin/pkg/tls/tls.go
setTLSDefaults now sets only MinVersion; NewTLSConfig and NewTLSClientConfig use expanded struct literals with #nosec G402 comments.
Test coverage for defaults
plugin/pkg/tls/tls_test.go
Adds assertTLSDefaults helper and invokes it across config-creation tests to verify MinVersion/MaxVersion/CipherSuites/CurvePreferences values.
Documentation updates
plugin/etcd/README.md, plugin/forward/README.md, plugin/grpc/README.md, plugin/tls/README.md
Documents that CoreDNS enforces minimum TLS 1.2 while other TLS parameters follow Go crypto/tls defaults; adjusts grpc "Bugs" section wording.

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

🚥 Pre-merge checks | ✅ 15
✅ Passed checks (15 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 No Ginkgo titles were added; the changed Go tests use static Test*/t.Run names and no dynamic values.
Test Structure And Quality ✅ Passed Not applicable: the PR only changes plain Go unit tests, not Ginkgo, and no cluster/timeouts/cleanup patterns are present.
Microshift Test Compatibility ✅ Passed No new Ginkgo e2e tests were added; plugin/pkg/tls/tls_test.go uses standard Go unit tests only, with no MicroShift-sensitive APIs.
Single Node Openshift (Sno) Test Compatibility ✅ Passed No new Ginkgo e2e tests were added; the only test changes are standard Go unit tests in plugin/pkg/tls with no multi-node assumptions.
Topology-Aware Scheduling Compatibility ✅ Passed Only TLS config/tests and README docs changed; no deployment manifests, controllers, pod specs, affinities, selectors, or topology logic were added.
Ote Binary Stdout Contract ✅ Passed Touched Go files contain only library/test functions; no main/init/TestMain/suite setup or stdout logging calls were added.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed No new Ginkgo/e2e tests were added; the PR only changes docs and unit tests, and I found no IPv4-only or external connectivity assumptions.
No-Weak-Crypto ✅ Passed Touched TLS code only sets TLS1.2 minimum and leaves Go defaults; no weak-crypto primitives, custom crypto, or secret comparisons were added.
Container-Privileges ✅ Passed No container/K8s manifests were changed, and the modified files contain none of the flagged privilege settings.
No-Sensitive-Data-In-Logs ✅ Passed No new logging or sensitive-data exposure was added; the touched code only changes TLS defaults and test assertions, with no logger usage in modified files.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: TLS defaults now rely on Go's crypto/tls defaults.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

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

@aswinsuryan aswinsuryan marked this pull request as ready for review July 8, 2026 01:21
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 8, 2026
@openshift-ci openshift-ci Bot requested review from candita and frobware July 8, 2026 01:21
@davidesalerno

Copy link
Copy Markdown

/retest-required

@openshift-ci

openshift-ci Bot commented Jul 8, 2026

Copy link
Copy Markdown

@aswinsuryan: 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.

@gcs278

gcs278 commented Jul 8, 2026

Copy link
Copy Markdown

/assign @davidesalerno

@davidesalerno

Copy link
Copy Markdown

/approve
/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2026
@openshift-ci

openshift-ci Bot commented Jul 8, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidesalerno

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 Jul 8, 2026
@candita

candita commented Jul 8, 2026

Copy link
Copy Markdown

/jira refresh

@openshift-ci-robot

openshift-ci-robot commented Jul 8, 2026

Copy link
Copy Markdown

@candita: This pull request references NE-2744 which is a valid jira issue.

Details

In response to this:

/jira refresh

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.

@aswinsuryan

Copy link
Copy Markdown
Author

/hold

@openshift-ci openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 9, 2026
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. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants