CNTRLPLANE-3429: Respect configured cipher order for inject-tls#1386
CNTRLPLANE-3429: Respect configured cipher order for inject-tls#1386vincentdephily wants to merge 2 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@vincentdephily: This pull request explicitly references no 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughCore TLS injection records each parsed RNode kind (operator or controller) and uses it for kind-specific handling; observed cipher-suite order is preserved; tests and helpers were refactored to assert order-preserving TLS injection. ChangesTLS Injection Logic & Cipher Suite Ordering
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vincentdephily 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 |
|
/retest |
| } | ||
|
|
||
| func getDefaultCipherSuitesSorted() []string { | ||
| cipherSuites := crypto.CipherSuitesToNamesOrDie(crypto.DefaultCiphers()) |
There was a problem hiding this comment.
Why not just drop the sort.Strings statement?
There was a problem hiding this comment.
Because crypto.DefaultCiphers() and configv1.TLSProfiles[crypto.DefaultTLSProfileType] return the same ciphers but in a different order. The new TLS1.3 ciphers have a higher precedence in os library-go than in golang crypto.
Ideally I would have taken the cipher suite by calling apiserver.ObserveTLSSecurityProfile(), but the context args made that akward. So instead I extracted the core logic for the 'default case', and added a comment. If the apiserver code drifts, we'll have to update this test.
There was a problem hiding this comment.
The new TLS1.3 ciphers have a higher precedence in os library-go than in golang crypto.
That does not play well with the cipher ordering either. Both config/v1/types_tlssecurityprofile.go and DefaultCiphers() need to respect the same ordering then. Maybe noone has though of it before?
vincentdephily
changed the title
|
@vincentdephily: This pull request references CNTRLPLANE-3429 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 only the "5.0.0" version, but multiple target versions were set. 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. |
|
/cc |
There was a problem hiding this comment.
nit:
Thank you for the fix! Could we also update the various existing comments referencing GenericOperatorConfig in the file to something more generic, perhaps? There are a few of them.
| return nil, err | ||
| } else if ciphersFound { | ||
| // Sort cipher suites for consistent ordering | ||
| sort.Strings(cipherSuites) |
There was a problem hiding this comment.
This is very interesting.
Have you reached out to the relevant centralized-tls-profile folks about whether the API description should be updated?
Currently, the relevant APIServer API is described as [1]:
// TLSProfileSpec is the desired behavior of a TLSSecurityProfile.
type TLSProfileSpec struct {
// ciphers is used to specify the cipher algorithms that are negotiated
// during the TLS handshake. Operators may remove entries that their operands
// do not support. For example, to use only ECDHE-RSA-AES128-GCM-SHA256 (yaml):
//
// ciphers:
// - ECDHE-RSA-AES128-GCM-SHA256
//
// TLS 1.3 cipher suites (e.g. TLS_AES_128_GCM_SHA256) are not configurable
// and are always enabled when TLS 1.3 is negotiated.
// +listType=atomic
Ciphers []string `json:"ciphers"`No mention of preferred ordering. Maybe there should be?
Ideally, the majority of components use library functions, which hopefully respect the ordering. Hopefully. However, at a minimum, some components will not respect this.
Also, the tls-scanner compliance check does not seem to check the ordering [2] as well. Maybe it should?
There was a problem hiding this comment.
Just a note: this will be less a problem from TLS v1.3 given the list of ciphers will be ignored/rejected.
There was a problem hiding this comment.
Having investigated this properly, it turns out that Go actually does ignore the configured cipher order (and cipher list, with TLS1.3). This is Go-specific behaviour (and new since 1.17), so I don't feel too bad for having assumed that order mattered 😅
I'll update this PR with clarifying comments.
There's a future-proofing/least-surprise argument to be made for keeping the user's order, but I think that the config-reconcilation-minimizing argument wins out, so I'll put the sort back in place unless you have other opinions.
There was a problem hiding this comment.
Nice exploration. This is quite surprising.
Yeah, dropping down a comment makes sense here.
There was a problem hiding this comment.
Yes, a nice exploration indeed 🙇
I think that the config-reconcilation-minimizing argument wins out
Me as well 👍
* Fix assumptions about NodeKind in comments * Log the actual NodeKind during tls injection * Other minor tweaks
vincentdephily
force-pushed
the
vdp-respect-cipher-order
branch
from
4efff84 to
7e9dbd4
Compare
![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
🤖 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 `@lib/resourcebuilder/core_test.go`:
- Around line 108-112: The "reversed" test data isn't reversed: update
testCipherSuitesYmlReversed so its elements are the opposite order of
testCipherSuitesYml (i.e., swap the two entries) so the test at lines ~445-456
actually exercises order preservation; locate the variable
testCipherSuitesYmlReversed in core_test.go and change its element order to be
the reverse of testCipherSuitesYml.
🪄 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: Enterprise
Run ID: 0193b92c-a844-42b7-9b4e-8a41326679ff
📒 Files selected for processing (3)
lib/resourcebuilder/core.golib/resourcebuilder/core_test.golib/resourcebuilder/resourcebuilder_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/resourcebuilder/core.go
Although Go TLS ignores ciphersuites order, and has some complicated rules about ciphersuites content, other implementations (and perhaps future Go implementations) have different bahaviors, so we shouldn't optimize for the current implementation too closely. Also, ciphersuites opinions should be have a single source of truth, and be implemented at the APIServer level. Applying changes at the CluserVersionOperator level makes the config flow harer to follow. * Don't sort ciphersuites, and document the rationale * Add testscases and remove redundant ones * Clarify test labels and testdata name/comments * Reorder testcases and data to be more thematic * Simplify test helpers
vincentdephily
force-pushed
the
vdp-respect-cipher-order
branch
from
7e9dbd4 to
f8e2063
Compare
|
I've identified that unittest race condition with @ricardomaraschini but I'll treat it in another PR, just retrying here. /retest |
|
@vincentdephily: 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. |
|
PR updated: I've addressed the comment nitpicks, added/removed/tweaked some testcases, refactored the test code... And kept the "no sorting" behavior despite my earlier opinion about minimizing config reconciliation. The rationale is in the commit message and code comments, but the main arguments are that we don't want the CVO to inject a different config than what the operator can get via the APIServer, and that we don't want to make an assumption that might not be valid for some operators. I've spent a lot of time trying to get the best of both worlds by keeping the order when injecting, but only injecting when it makes a semantic difference (eg ignoring order, old-style names, etc). But the deeper I dug, the more opinionated quirks I found in Go and library-go. Too many opportunities for bitrot and false negatives, so I deleted those attempts and kept things simple, stupid. |
4 participants
Cipher order is meaningful, don't sort alphabetically.
Summary by CodeRabbit
Bug Fixes
Tests