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
RATIS-2064. Bump Netty to 4.1.109 #50
Conversation
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.
+1 the change looks good.
"TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384", | ||
"TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256", | ||
"TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", | ||
"TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384", | ||
"TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA", | ||
"TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA", | ||
"TLS_RSA_WITH_AES_128_GCM_SHA256", | ||
"TLS_RSA_WITH_AES_128_CBC_SHA", | ||
"SSL_RSA_WITH_3DES_EDE_CBC_SHA")); | ||
|
||
// "RSA" in this case refers to the key exchange algorithm, | ||
// "SHA" refers to the message digest algorithm to provide integrity | ||
// "NULL" is the encryption algorithm, to disable encryption. | ||
// TODO: support NULL cipher from tcnative | ||
private final List<String> tlsCipherSuitesNoEncryption = Collections.singletonList("TLS_RSA_WITH_AES_128_GCM_SHA256"); | ||
"TLS_RSA_WITH_AES_256_CBC_SHA", | ||
"TLS_AES_128_GCM_SHA256", | ||
"TLS_AES_256_GCM_SHA384", | ||
"TLS_AES_128_GCM_SHA256", | ||
"TLS_AES_256_GCM_SHA384", | ||
"TLS_AES_128_GCM_SHA256", | ||
"TLS_AES_256_GCM_SHA384", | ||
"TLS_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.
@adoroszlai , How did you find out this list?
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.
I took the list from this log message emitted by Netty during the test:
[main] DEBUG ssl.OpenSsl (OpenSsl.java:<clinit>(402)) - Default cipher suites (OpenSSL): [TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA, TLS_RSA_WITH_AES_128_GCM_SHA256, TLS_RSA_WITH_AES_128_CBC_SHA, TLS_RSA_WITH_AES_256_CBC_SHA, TLS_AES_128_GCM_SHA256, TLS_AES_256_GCM_SHA384, TLS_AES_128_GCM_SHA256, TLS_AES_256_GCM_SHA384, TLS_AES_128_GCM_SHA256, TLS_AES_256_GCM_SHA384, TLS_CHACHA20_POLY1305_SHA256]
I don't know why the old list (particularly the NoEncryption
one) wouldn't work. The list of default ciphers is the same for both versions.
The following mapping is no longer present with new Netty:
[main] DEBUG ssl.CipherSuiteConverter (CipherSuiteConverter.java:cacheFromOpenSsl(352)) - Cipher suite mapping: TLS_RSA_WITH_3DES_EDE_CBC_SHA => DES-CBC3-SHA
[main] DEBUG ssl.CipherSuiteConverter (CipherSuiteConverter.java:cacheFromOpenSsl(353)) - Cipher suite mapping: SSL_RSA_WITH_3DES_EDE_CBC_SHA => DES-CBC3-SHA
so I first removed that from the original list, but that would not fix the problem.
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.
+1 the changes look good to me. Thanks @adoroszlai for bumping Netty and tweak the list.
Thanks @szetszwo, @SzyWilliam for the review. |
What changes were proposed in this pull request?
Bump Netty to latest (4.1.109 currently).
I had to tweak cipher list in tests to make
GrpcSslTest
to pass. It was failing withStacklessSSLHandshakeException: Connection closed while SSL/TLS handshake was in progress
due to one of the changes in Netty 4.1.108 related to SSL.https://issues.apache.org/jira/browse/RATIS-2064
How was this patch tested?
CI:
https://github.com/adoroszlai/ratis-thirdparty/actions/runs/8877229601