Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/17189 )
Change subject: KUDU-2871 support TLSv1.3 in Kudu RPC (C++ part) ...................................................................... Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/17189/5/src/kudu/security/tls_context.cc File src/kudu/security/tls_context.cc: http://gerrit.cloudera.org:8080/#/c/17189/5/src/kudu/security/tls_context.cc@259 PS5, Line 259: #if OPENSSL_VERSION_NUMBER >= 0x10101000L : // Set TLSv1.3 ciphers. : OPENSSL_RET_NOT_OK( : SSL_CTX_set_ciphersuites(ctx, tls_1_3_ciphers_.c_str()), : Substitute("failed to set TLSv1.3 ciphers: $0", tls_1_3_ciphers_)); : #endif I have one drive-by comment that I just noticed: I think we want the SSL_CTX_set_ciphersuites() call to come before the SSL_CTX_set_cipher_list() call. This revolves around whether the code will throw an error here if no ciphers are configured. SSL_CTX_set_ciphersuites() will throw an error if no ciphers are configured, but on some versions of OpenSSL (like 1.1.1 used on Ubuntu 18), it is including TLS 1.3 ciphersuites in the list. (This behavior is fixed in more recent OpenSSL.) So, if tls_ciphers_ is "invalid_cipher" and tls_1_3_ciphers_ is the empty string (i.e. no TLS 1.3 ciphers), SSL_CTX_set_cipher_list() won't fail, because the TLS 1.3 ciphersuites default to having multiple ciphers available. The SSL_CTX_set_ciphersuites() call doesn't actually check whether there are zero ciphers configured overall. So, it also wouldn't fail. Reversing the calls, SSL_CTX_set_ciphersuites() with an empty string is valid and then the SSL_CTX_set_cipher_list() call will fail if there are truly no ciphers available (and it sees the accurate TLS 1.3 list). -- To view, visit http://gerrit.cloudera.org:8080/17189 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia92a4d102c3c8cff76101e71ff71d24a9d78b672 Gerrit-Change-Number: 17189 Gerrit-PatchSet: 5 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 26 Mar 2021 18:19:07 +0000 Gerrit-HasComments: Yes
