Skip to content
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

Backport #14500 to 3.4 #14601

Merged
merged 1 commit into from
Oct 27, 2022
Merged

Backport #14500 to 3.4 #14601

merged 1 commit into from
Oct 27, 2022

Conversation

dusk125
Copy link
Contributor

@dusk125 dusk125 commented Oct 19, 2022

server/etcdmain: add configurable cipher list to gRPC proxy listener

…oxy listener

Signed-off-by: Allen Ray <alray@redhat.com>
@ahrtr
Copy link
Member

ahrtr commented Oct 19, 2022

Backporting #14308

Comment on lines +35 to +40
switch s {
case "TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305":
return tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256, true
case "TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305":
return tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256, true
}
Copy link
Member

@ahrtr ahrtr Oct 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Golang 1.16.15 already includes these two cipher suites, why do you process them separately?

FYI. https://github.com/golang/go/blob/go1.16.15/src/crypto/tls/cipher_suites.go#L71-L72

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be a unneeded copy from Sam's original PR; I don't have a specific reason to have this. I would be happy to remove it, however it does align this version with the future versions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the future versions

Do you mean future golang version? All golang future version already includes these two cipher suites. So we need to remove them from main and release-3.5 as well.

@ptabor could you share the thought why did you add this (process these two cipher suite separately) in the first place?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double checked the go library src, it includes both TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 and TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256, but we (etcd) also supports TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305 and TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305. Let keep it as it's for now.

Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thank you @dusk125

@ahrtr ahrtr merged commit ce1630f into etcd-io:release-3.4 Oct 27, 2022
@dusk125 dusk125 deleted the release-3.4 branch October 27, 2022 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants