config/v1: TLSSecurityProfile: Remove go's unsupported ciphers#2697
config/v1: TLSSecurityProfile: Remove go's unsupported ciphers#2697damdo wants to merge 2 commits intoopenshift:masterfrom
Conversation
Following up from the dicussion in https://redhat-internal.slack.com/archives/C098FU5MRAB/p1770309657097269 We are removing DHE and CBC mode ciphers groups as they are not supported due to Go's crypto/tls limitations. Added notes and context for removed ciphers. Context: golang/go#7758 golang/go#26652
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Hello @damdo! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/XL
📝 WalkthroughWalkthroughThis pull request modifies TLS security profile configurations across multiple Go source files and Kubernetes CRD YAML files. The changes remove several DHE (Diffie-Hellman Ephemeral) and CBC-mode cipher entries from Old and Intermediate TLS profile definitions. The removed ciphers include DHE-RSA-AES128-GCM-SHA256, DHE-RSA-AES256-GCM-SHA384, DHE-RSA-CHACHA20-POLY1305, DHE-RSA-AES128-SHA256, DHE-RSA-AES256-SHA256, and various CBC-based ciphers. Explanatory comments are added throughout to document that these ciphers are not supported by Go's crypto/tls library. No new exported APIs or type declarations are introduced. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Review Summary by QodoRemove Go-unsupported DHE and CBC ciphers from TLS security profiles
WalkthroughsDescription• Remove unsupported DHE and CBC ciphers from TLS profiles • Add explanatory comments referencing Go crypto/tls limitations • Update documentation with rationale for removed ciphers • Synchronize changes across all generated CRD manifests Diagramflowchart LR
A["TLS Security Profiles"] -->|Remove DHE ciphers| B["Go crypto/tls Issue #7758"]
A -->|Remove CBC ciphers| C["Go crypto/tls Issue #26652"]
B --> D["Updated Old Profile"]
C --> D
B --> E["Updated Intermediate Profile"]
C --> E
D --> F["Generated Manifests Updated"]
E --> F
File Changes1. config/v1/types_tlssecurityprofile.go
|
Code Review by Qodo
1. Old field missing optional docs
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| // +optional | ||
| // +nullable | ||
| Old *OldTLSProfile `json:"old,omitempty"` |
There was a problem hiding this comment.
1. old field missing optional docs 📘 Rule violation ✓ Correctness
• The Old *OldTLSProfile field has +optional and +nullable markers, but its comment only lists cipher details and does not explain what happens when the field is omitted or explicitly set to null. • This makes the API behavior unclear to consumers and violates the requirement that field comments document the meaning/behavior of kubebuilder optionality markers.
Agent Prompt
## Issue description
`TLSSecurityProfile` fields (e.g., `Old`) include kubebuilder optionality markers (`+optional`, `+nullable`) but their comments do not document what omitted or null values mean.
## Issue Context
PR Compliance ID 11 requires that any field with optionality/validation markers has comments explaining those markers’ meaning and behavior so the API is self-documenting.
## Fix Focus Areas
- config/v1/types_tlssecurityprofile.go[6-128]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // Ciphers not in this profile even though listed | ||
| // in the Mozilla Server Side TLS configuration guidelines: | ||
| // Go's crypto/tls does not support DHE ciphers (see https://github.com/golang/go/issues/7758) | ||
| - DHE-RSA-AES128-GCM-SHA256 | ||
| - DHE-RSA-AES256-GCM-SHA384 |
There was a problem hiding this comment.
2. Invalid yaml doc snippets 🐞 Bug ✓ Correctness
• Multiple CRD/OpenAPI descriptions contain YAML configuration examples that now include // ... lines; // is not valid YAML comment syntax. • Users/operators copying these examples into real YAML manifests will hit YAML parse errors (or end up with unintended config). • Because these are generated artifacts, the issue propagates widely (config, operator, machineconfiguration, payload manifests, OpenAPI).
Agent Prompt
### Issue description
CRD/OpenAPI `description` fields embed YAML examples that now contain `//` lines. These are not valid YAML comments, so users copying the example into manifests will get YAML parse errors.
### Issue Context
The problematic text is generated from Go doc comments in `config/v1/types_tlssecurityprofile.go` and then propagated to CRD manifests and OpenAPI/Swagger output.
### Fix Focus Areas
- config/v1/types_tlssecurityprofile.go[22-98]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@config/v1/types_tlssecurityprofile.go`:
- Around line 53-64: Update the overstated comment that reads "Go's crypto/tls
does not support CBC mode ciphers" to specifically say that Go's crypto/tls
lacks support for the CBC SHA-256/384 suites (e.g., AES256-SHA256,
ECDHE-*-AES256-SHA384) so it only excludes those CBC SHA-256/384 ciphers rather
than all CBC-mode ciphers; apply this same clarified wording to both comment
occurrences in the TLS profile comment block (the blocks listing the excluded
CBC SHA-256/384 suites alongside the DHE exclusions) so the comment matches the
actual excluded cipher list.
| // // Ciphers not in this profile even though listed | ||
| // // in the Mozilla Server Side TLS configuration guidelines: | ||
| // // Go's crypto/tls does not support DHE ciphers (see https://github.com/golang/go/issues/7758) | ||
| // - DHE-RSA-AES128-GCM-SHA256 | ||
| // - DHE-RSA-AES256-GCM-SHA384 | ||
| // - DHE-RSA-CHACHA20-POLY1305 | ||
| // - DHE-RSA-AES128-SHA256 | ||
| // - DHE-RSA-AES256-SHA256 | ||
| // // Go's crypto/tls does not support CBC mode ciphers (see https://github.com/golang/go/issues/26652) | ||
| // - ECDHE-ECDSA-AES256-SHA384 | ||
| // - ECDHE-RSA-AES256-SHA384 | ||
| // - AES256-SHA256 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the file
find . -name "types_tlssecurityprofile.go" -type fRepository: openshift/api
Length of output: 96
🏁 Script executed:
# Once we find it, let's look at the specific lines mentioned
# Lines 53-64 (Old profile) and 252-255 (also applies to)
# We'll get broader context to understand the profile structure
cat -n config/v1/types_tlssecurityprofile.go | sed -n '1,100p'Repository: openshift/api
Length of output: 4697
🌐 Web query:
Go crypto/tls CBC cipher support all versions limitations
💡 Result:
Go crypto/tls CBC cipher suite support (by TLS version) and key limitations
1) TLS 1.3: No CBC at all
TLS 1.3 cipher suites in Go are AEAD-only (AES-GCM, ChaCha20-Poly1305) and cannot be configured via Config.CipherSuites. So CBC suites are never negotiated in TLS 1.3. [1], [2]
2) TLS 1.0–1.2: CBC is implemented, but with important caveats
Go implements multiple AES-CBC suites for TLS 1.0–1.2, including (examples): TLS_RSA_WITH_AES_128_CBC_SHA, TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, etc. [3]
However:
- CBC + SHA-256 (“CBC_SHA256”) suites are considered insecure in Go (no Lucky13 countermeasures) and are disabled by default. They appear under
tls.InsecureCipherSuites(). [3], [2] - CBC + SHA-1 suites still exist (and are not in
InsecureCipherSuites()), but CBC as a family is historically attack-prone (e.g., Lucky13-style side channels). Go’s mitigations are described as only partial/complex, and CBC suites are intended mainly for compatibility fallback. [2]
3) “Default enabled” vs “implemented” changes across Go versions
Even when CBC suites are implemented, whether they’re in the default set can change across Go releases:
- Go’s docs note that defaults change over time; e.g. RSA key-exchange suites removed from defaults in Go 1.22 (re-add via
GODEBUG=tlsrsakex=1) and 3DES removed from defaults in Go 1.23 (re-add viaGODEBUG=tls3des=1). (3DES is CBC-mode and is also classified insecure.) [1], [3]
4) Configuring CBC in Go: what you can and can’t do
Config.CipherSuitesonly affects TLS 1.0–1.2; it does nothing for TLS 1.3. [1]- To “force” CBC you generally must also ensure you’re not negotiating TLS 1.3 (e.g.,
MaxVersion: tls.VersionTLS12), otherwise TLS 1.3 will avoid CBC entirely. [1]
5) Extra restriction: FIPS mode
In Go’s FIPS 140-3 mode, non-approved cipher suites/versions are “silently ignored” or rejected; this can further limit (or eliminate) CBC availability depending on the selected module and Go version. [1]
Sources: Go crypto/tls docs (Go 1.25.2) [1], Go blog on cipher suite ordering / CBC & Lucky13 notes [2], Go crypto/tls cipher suite source (lists secure vs insecure suites and supported versions) [3].
🏁 Script executed:
cat -n config/v1/types_tlssecurityprofile.go | sed -n '240,280p'Repository: openshift/api
Length of output: 1776
🏁 Script executed:
# Also check if this pattern exists in intermediate profile comments
cat -n config/v1/types_tlssecurityprofile.go | sed -n '85,100p'Repository: openshift/api
Length of output: 815
Clarify CBC support wording in both Old profile locations.
The Old profile still lists CBC SHA1 suites (AES128-SHA, AES256-SHA, DES-CBC3-SHA), so the comment "Go's crypto/tls does not support CBC mode ciphers" is overstated. The actual limitation applies only to the CBC SHA-256/384 suites listed below. Narrow the wording to match the specific ciphers excluded.
✏️ Suggested wording
- // // Go's crypto/tls does not support CBC mode ciphers (see https://github.com/golang/go/issues/26652)
+ // // Go's crypto/tls does not support these CBC mode SHA-256/384 ciphers (see https://github.com/golang/go/issues/26652)
- // Go's crypto/tls does not support CBC mode ciphers (see https://github.com/golang/go/issues/26652)
+ // Go's crypto/tls does not support these CBC mode SHA-256/384 ciphers (see https://github.com/golang/go/issues/26652)Also applies to: 252-255
🤖 Prompt for AI Agents
In `@config/v1/types_tlssecurityprofile.go` around lines 53 - 64, Update the
overstated comment that reads "Go's crypto/tls does not support CBC mode
ciphers" to specifically say that Go's crypto/tls lacks support for the CBC
SHA-256/384 suites (e.g., AES256-SHA256, ECDHE-*-AES256-SHA384) so it only
excludes those CBC SHA-256/384 ciphers rather than all CBC-mode ciphers; apply
this same clarified wording to both comment occurrences in the TLS profile
comment block (the blocks listing the excluded CBC SHA-256/384 suites alongside
the DHE exclusions) so the comment matches the actual excluded cipher list.
|
/approve |
openshift-ci
bot
added
the
approved
|
/lgtm Thanks @damdo ! |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: joelanford, JoelSpeed 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 |
|
/pipeline required |
|
Scheduling tests matching the |
|
/test e2e-azure |
|
PR-Agent: could not fine a component named |
|
@damdo: 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. |
4 participants
Following up from the dicussion in:
https://redhat-internal.slack.com/archives/C098FU5MRAB/p1770309657097269
we are removing DHE and CBC mode ciphers groups as they are not supported due to Go's crypto/tls limitations.
Added notes and context for removed ciphers
golang/go#7758
golang/go#26652
--
openshift/library-go counterpart: openshift/library-go#2119