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
The cipher order matters, as it is used during TLS handshake to determine the prefered cipher. Don't sort lexicographically.
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. |
WalkthroughTLS injection logic in ConfigMap handling now captures and logs the detected RNode kind separately for ChangesTLS Injection Logic & Cipher Suite Ordering
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
@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. |
| } | ||
|
|
||
| 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.
| "ECDHE-ECDSA-AES128-GCM-SHA256", | ||
| } | ||
|
|
||
| // Alternate cipher suite |
There was a problem hiding this comment.
Can this change be made on the original lines? To simplify the diff.
There was a problem hiding this comment.
I changed the order because having related suites together felt more readable to me (in particular to notice same ciphers with different IDs), but if you feel otherwise I can keep the old order and reword the comments ?
| } | ||
|
|
||
| defaultCipherSuites = getDefaultCipherSuitesSorted() | ||
| // Default cipher suite returned bu library-go |
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 |
| klog.V(4).Infof("ConfigMap %s/%s: observed minTLSVersion=%v, cipherSuites=%v", | ||
| cm.Namespace, cm.Name, minTLSLog, cipherSuitesLog) | ||
|
|
||
| // Process each data entry that contains GenericOperatorConfig |
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?
4 participants
Cipher order is meaningful, don't sort alphabetically.
Summary by CodeRabbit
Bug Fixes
Tests