config/v1: TLSSecurityProfile: align with Mozilla v5.7 guidelines for Go by removing unsupported go ciphers#2697
Conversation
|
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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request updates TLS security profile documentation and example cipher lists in Go source (config/v1 types and generated swagger) and multiple CRD YAMLs. It updates the Mozilla TLS guideline reference from v5.0 to v5.7, clarifies that profile cipher lists are the configuration's "ciphersuites" followed by Go-specific "ciphers", documents that TLS 1.3 cipher suites are not configurable and are always enabled, and removes several DHE- and certain AES/SHA384-related cipher entries from Old and Intermediate profile examples. No exported/public type or function signatures were changed. 🚥 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)
No actionable comments were generated in the recent review. 🎉 Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented 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}
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-Default.crd.yaml
Outdated
Show resolved
Hide resolved
![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.
|
/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. |
|
/pipeline required |
|
Scheduling tests matching the |
|
/test e2e-azure |
|
PR-Agent: could not fine a component named |
|
/hold for review |
openshift-ci
bot
added
the
do-not-merge/hold
sanchezl
left a comment
sanchezl
left a comment
There was a problem hiding this comment.
Your end result is the Mozilla SSL 5.7 profile (the "go" specific version). Please delete all the commented out suites.
|
/hold cancel removing the hold as my change requests are all Godoc. I can follow up with a another PR if this one is merged without the updates. |
openshift-ci
bot
removed
do-not-merge/hold
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/v1/types_tlssecurityprofile.go (1)
174-188:⚠️ Potential issue | 🟠 MajorDuplicate documentation: old ciphers field comment not removed.
Lines 174-179 contain the old documentation (with DES-CBC3-SHA example) while lines 180-188 contain the new documentation (with ECDHE-RSA-AES128-GCM-SHA256 example). The old block should be removed.
🐛 Proposed fix
-// ciphers is used to specify the cipher algorithms that are negotiated -// during the TLS handshake. Operators may remove entries their operands -// do not support. For example, to use DES-CBC3-SHA (yaml): -// -// ciphers: -// - DES-CBC3-SHA // ciphers is used to specify the cipher algorithms that are negotiated // during the TLS handshake. Operators may remove entries their operands // do not support. For example, to use only ECDHE-RSA-AES128-GCM-SHA256 (yaml):
🤖 Fix all issues with AI agents
In `@config/v1/types_tlssecurityprofile.go`:
- Around line 222-229: The top-of-file comment in types_tlssecurityprofile.go
incorrectly references "version 5.0" and contains an incomplete fragment
("prepended for"); remove the outdated 5.0 lines and the incomplete phrase and
replace the block with a single clear sentence stating that the profiles are
based on "version 5.7 of the Mozilla Server Side TLS configuration guidelines"
(see https://ssl-config.mozilla.org/guidelines/5.7.json) and keep the following
sentence about Ciphers slices being the configuration's "ciphersuites" followed
by the Go-specific "ciphers" from the guidelines so consumers of
TLSecurityProfile and the Ciphers slice comment are unambiguous.
- Around line 10-15: Remove the stale/contradictory lines referencing "version
5.0" and the incomplete phrase and replace the comment block so it consistently
documents that the profiles are based on Mozilla Server Side TLS guidelines
version 5.7 (include the https://ssl-config.mozilla.org/guidelines/5.7.json
link) and note that the cipher lists are the configuration's "ciphersuites"
followed by the Go-specific "ciphers"; update the comment near the
TLSSecurityProfile type in config/v1/types_tlssecurityprofile.go accordingly so
only the 5.7 reference and the complete explanation remain.
damdo
changed the title
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/verified by ci runs |
openshift-ci-robot
added
the
verified
|
@gangwgr: This PR has been marked as verified by 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. |
|
/test e2e-aws-ovn-hypershift |
|
PR-Agent: could not fine a component named |
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
damdo
force-pushed
the
config-tlssecurityprofile-remove-go-incompatible-ciphers
branch
from
de3d7a6 to
164ef8e
Compare
|
/pipeline required |
|
Scheduling tests matching the |
|
/lgtm |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/retest |
|
/lgtm |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: joelanford, JoelSpeed, sanchezl 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 |
|
/retest |
1 similar comment
|
/retest |
|
There is a known issue with the serial jobs: https://redhat-internal.slack.com/archives/CBN38N3MW/p1770883379901109 And the azure failure is unrelated: https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_api/2697/pull-ci-openshift-api-master-e2e-azure/2021844746799943680 as it passed on a previous job run |
|
/test e2e-azure |
|
PR-Agent: could not fine a component named |
|
/retest |
|
openshift/ci-tools#4943 has merged /retest |
|
@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. |
6 participants
Following up from the dicussion in:
https://redhat-internal.slack.com/archives/C098FU5MRAB/p1770309657097269
We are aligning with the Mozilla v5.7 security guidelines for Go.
The profiles are now based on version 5.7 of the Mozilla Server Side TLS
configuration guidelines. The cipher lists consist the configuration's
"ciphersuites" followed by the Go-specific "ciphers" from the guidelines.
See: https://ssl-config.mozilla.org/guidelines/5.7.json
This effectively removes the DHE and CBC mode ciphers groups as they are not supported due to Go's crypto/tls limitations.
golang/go#7758
golang/go#26652
--
openshift/library-go counterpart: openshift/library-go#2119