Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/23149 )
Change subject: [webserver] Add support for requiring TLS 1.3 ...................................................................... Patch Set 3: (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 f Yea, that's a good point, should be done in this patch. 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 Done http://gerrit.cloudera.org:8080/#/c/23149/2/src/kudu/server/webserver-test.cc@526 PS2, Line 526: curl_.set_tls_max_version(TlsVersion::TLSv1_2); > Why to set the minimum version here at all? Wouldn't it be enough to limit Done 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 Done http://gerrit.cloudera.org:8080/#/c/23149/2/src/kudu/server/webserver-test.cc@532 PS2, Line 532: curl_.set_tls_min_version(TlsVersion::TLSv1_3); > Ditto: why to set the minimum version? I would expect it working as expect Done 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: "--webserver_private_key_file"; : return false; : } : > 1. Instead of hard-coding the tokens, consider using the corresponding con Those are in an anonymous namespace, so I think this is cleaner. Added case-insensitive checking though. 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_tls_min_version(TlsVersion tls_min_version) { : tls_min_version_ = tls_min_version; : } : > Consider separating this into two different methods: one for setting the mi Done 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 Done http://gerrit.cloudera.org:8080/#/c/23149/2/src/kudu/util/curl_util.cc@237 PS2, Line 237: ) { > Once setting min and max TLS versions are separated into different setter f Done http://gerrit.cloudera.org:8080/#/c/23149/2/src/kudu/util/curl_util.cc@253 PS2, Line 253: > Is there a way to be more specific on min or max version it was about? Als Done http://gerrit.cloudera.org:8080/#/c/23149/2/src/kudu/util/curl_util.cc@253 PS2, Line 253: > I'd expect Status::ConfigurationError() here instead of Status::RuntimeErro Done http://gerrit.cloudera.org:8080/#/c/23149/2/src/kudu/util/curl_util.cc@274 PS2, Line 274: OT_OK(curl_easy_setopt(curl_, CURLOPT_SSLVER > Ditto Done 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: /squeasel.c > I'm not sure this is the correct way to tell incompatibility/absence-of-sup Done -- 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: 3 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Comment-Date: Thu, 10 Jul 2025 06:39:27 +0000 Gerrit-HasComments: Yes
