-
Notifications
You must be signed in to change notification settings - Fork 38.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: switch to tls cipher suite in stdlib #105064
refactor: switch to tls cipher suite in stdlib #105064
Conversation
@knight42: Adding the "do-not-merge/release-note-label-needed" label and removing any existing "release-note-none" label because there is a "kind/deprecation" label on the PR. 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/test-infra repository. |
|
||
for _, suite := range tls.InsecureCipherSuites() { | ||
insecureCiphers[suite.Name] = suite.ID | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that we're using these, we can drop the TestConstantMaps
unit test that ensured we stayed in sync with the stdlib
ciphers[suite.Name] = suite.ID | ||
} | ||
// keep legacy names for backward compatibility | ||
ciphers["TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305"] = tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't match the previous mapping, right?
"TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305": tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://cs.opensource.google/go/go/+/refs/tags/go1.17.1:src/crypto/tls/cipher_suites.go;l=687
tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305
is actually the same as tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
, tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305
is retained for backwards compability.
} | ||
// keep legacy names for backward compatibility | ||
ciphers["TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305"] = tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 | ||
ciphers["TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305"] = tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't match the previous mapping, right?
"TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305": tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305
is also the same as tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add the _114 mappings to the unit test to verify this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
lgtm, go ahead and squash commits |
Signed-off-by: Jian Zeng <zengjian.zj@bytedance.com>
2bebdf2
to
2fbbd38
Compare
@liggitt Hi! The commits have been squashed now, could I get a |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: knight42, liggitt The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Jian Zeng zengjian.zj@bytedance.com
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #88547
xref #105063
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: