Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/23149 )
Change subject: [webserver] Add support for requiring TLS 1.3 ...................................................................... Patch Set 2: (13 comments) http://gerrit.cloudera.org:8080/#/c/23149/2//COMMIT_MSG Commit Message: PS2: Are you planning to have a separate patch for customizing TLSv1.3 ciphers for Squeasel or it's already addressed by other contributors? AFAIK, OpenSSL uses different API call to set those: that's SSL_CTX_set_ciphersuites(), not SSL_CTX_set_cipher_list(). http://gerrit.cloudera.org:8080/#/c/23149/2/src/kudu/server/webserver-test.cc File src/kudu/server/webserver-test.cc: http://gerrit.cloudera.org:8080/#/c/23149/2/src/kudu/server/webserver-test.cc@524 PS2, Line 524: TEST_F(Tls13WebserverTest, TestTlsMinVersion) { Since you added a setter for maximum TLS version, I'd expect to see a test scenario that verifies the max setting as well, but in this scenario I'd expect to see only customization of the minimum TLS version. http://gerrit.cloudera.org:8080/#/c/23149/2/src/kudu/server/webserver-test.cc@526 PS2, Line 526: curl_.set_min_max_tls_version(MinMaxTlsVersion::TLSv1_2, MinMaxTlsVersion::TLSv1_2); Why to set the minimum version here at all? Wouldn't it be enough to limit the maximum TLS version only? If it's really necessary, please add in-line comment to explain. http://gerrit.cloudera.org:8080/#/c/23149/2/src/kudu/server/webserver-test.cc@529 PS2, Line 529: ASSERT_TRUE(s.IsNetworkError()); nit: if this assertion ever triggers, it's valuable to have the information on the actual error type, so this is better be something like ASSERT_TRUE(s.IsNetworkError()) << s.ToString(); http://gerrit.cloudera.org:8080/#/c/23149/2/src/kudu/server/webserver-test.cc@532 PS2, Line 532: curl_.set_min_max_tls_version(MinMaxTlsVersion::TLSv1_3, MinMaxTlsVersion::TLSv1_3); Ditto: why to set the minimum version? I would expect it working as expected if only setting the maximum TLS version. Please add a comment to explain. http://gerrit.cloudera.org:8080/#/c/23149/2/src/kudu/server/webserver_options.cc File src/kudu/server/webserver_options.cc: http://gerrit.cloudera.org:8080/#/c/23149/2/src/kudu/server/webserver_options.cc@151 PS2, Line 151: return ver == "TLSv1" || : ver == "TLSv1.1" || : ver == "TLSv1.2" || : ver == "TLSv1.3"; 1. Instead of hard-coding the tokens, consider using the corresponding constants from tls_context.{h,cc} 2. Use case-insensitive comparison for the values of the flag. If necessary, you can normalize the values to be upper or lower case for the internal use, but user-facing interface should be more forgiving and user-friendly. http://gerrit.cloudera.org:8080/#/c/23149/2/src/kudu/util/curl_util.h File src/kudu/util/curl_util.h: http://gerrit.cloudera.org:8080/#/c/23149/2/src/kudu/util/curl_util.h@120 PS2, Line 120: void set_min_max_tls_version(MinMaxTlsVersion tls_min_version, MinMaxTlsVersion tls_max_version) { : tls_min_version_ = tls_min_version; : tls_max_version_ = tls_max_version; : } Consider separating this into two different methods: one for setting the min version, another for setting the max version. FWIW, the use case of setting the maximum TLS version isn't popular, so it's hard to think of it being used in real life scenarios. http://gerrit.cloudera.org:8080/#/c/23149/2/src/kudu/util/curl_util.cc File src/kudu/util/curl_util.cc: http://gerrit.cloudera.org:8080/#/c/23149/2/src/kudu/util/curl_util.cc@141 PS2, Line 141: nit: remove the stray empty line http://gerrit.cloudera.org:8080/#/c/23149/2/src/kudu/util/curl_util.cc@237 PS2, Line 237: MinMaxTlsVersion::NONE Once setting min and max TLS versions are separated into different setter functions, MinMaxTlsVersion::NONE is naturally maps into CURL_SSLVERSION_DEFAULT and CURL_SSLVERSION_MAX_DEFAULT correspondingly. With that, the API becomes more consistent, so it's be possibleto re-use a EasyCurl handle to set CURL_SSLVERSION_DEFAULT/CURL_SSLVERSION_MAX_DEFAULT even if there was any prior customization for the TLS version on the handle. http://gerrit.cloudera.org:8080/#/c/23149/2/src/kudu/util/curl_util.cc@253 PS2, Line 253: Status::RuntimeError I'd expect Status::ConfigurationError() here instead of Status::RuntimeError(). If there is a particular reason to use RuntimeError() here, please add a small text blurb to explain why. http://gerrit.cloudera.org:8080/#/c/23149/2/src/kudu/util/curl_util.cc@253 PS2, Line 253: Invalid TLS version Is there a way to be more specific on min or max version it was about? Also, I'd think that outputting the actual value passed to the function would help in troubleshooting. http://gerrit.cloudera.org:8080/#/c/23149/2/src/kudu/util/curl_util.cc@274 PS2, Line 274: Status::RuntimeError("Invalid TLS version"); Ditto http://gerrit.cloudera.org:8080/#/c/23149/2/thirdparty/patches/squeasel-tls-min-version.patch File thirdparty/patches/squeasel-tls-min-version.patch: http://gerrit.cloudera.org:8080/#/c/23149/2/thirdparty/patches/squeasel-tls-min-version.patch@10 PS2, Line 10: if (SSLeay() < OPENSSL_MIN_VERSION_WITH_TLS_1_1) I'm not sure this is the correct way to tell incompatibility/absence-of-support for TLSv1.3. Instead, I'd expect to see a comparison with version 1.1.1 where TLSv1.3 support has been added, not with 1.1.0 where TLSv1.3 isn't present yet. And another concern which is a bit out of scope, but just in case: in newer version of the OpenSSL library, the SSLeay() macro might not be available (unless OPENSSL_API_COMPAT is set to version matching 1.1.0) and OpenSSL_version_num() [1] should be used instead, but I guess it's a separate item since SSLeay() is used elsewhere in squeasel.c [1] https://docs.openssl.org/1.1.1/man3/OPENSSL_VERSION_NUMBER/ -- To view, visit http://gerrit.cloudera.org:8080/23149 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba21d62e22962c782ff8013d805b31ff058d9245 Gerrit-Change-Number: 23149 Gerrit-PatchSet: 2 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Comment-Date: Wed, 09 Jul 2025 23:51:23 +0000 Gerrit-HasComments: Yes
