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 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/23149/4/src/kudu/server/webserver-test.cc File src/kudu/server/webserver-test.cc: http://gerrit.cloudera.org:8080/#/c/23149/4/src/kudu/server/webserver-test.cc@524 PS4, Line 524: TEST_F(Tls13WebserverTest, TestTlsMinVersion) { Since you added support of setting of TLSv1.3 ciphers into the embedded webserver since PS2, does it make sense to add corresponding test scenarios into this test to cover the newly added functionality? I guess ideally those should be in the squeasel's codebase, but adding them here would be much better than not having those at all. http://gerrit.cloudera.org:8080/#/c/23149/4/thirdparty/patches/squeasel-tls-min-version.patch File thirdparty/patches/squeasel-tls-min-version.patch: PS4: nit: maybe, consider renaming this patch to match what it has become in PS4 It's just a nit, so feel free to ignore if that's too cumbersome. http://gerrit.cloudera.org:8080/#/c/23149/4/thirdparty/patches/squeasel-tls-min-version.patch@58 PS4, Line 58: + if (ctx->config[SSL_CIPHERS] != NULL || ctx->config[SSL_CIPHERSUITES] != NULL) { : + if (ctx->config[SSL_CIPHERS] != NULL) { : + if (SSL_CTX_set_cipher_list(ctx->ssl_ctx, ctx->config[SSL_CIPHERS]) != 1) { : + cry(fc(ctx), "SSL_CTX_set_cipher_list: error setting ciphers (%s): %s", : + ctx->config[SSL_CIPHERS], ssl_error()); : + return 0; : + } : + } : + if (ctx->config[SSL_CIPHERSUITES] != NULL) { : + if (SSL_CTX_set_ciphersuites(ctx->ssl_ctx, ctx->config[SSL_CIPHERSUITES]) != 1) { : + cry(fc(ctx), "SSL_CTX_set_ciphersuites: error setting ciphers (%s): %s", : + ctx->config[SSL_CIPHERSUITES], ssl_error()); : + return 0; : + } IIRC, there were some funny details on the proper ordering of these calls for earlier versions of OpenSSL 1.1.1: it seems the order of these should be reversed to allow for spotting an incorrect/empty list of TLSv1.2 ciphers and in some other peculiar situations. Please take a look at https://github.com/apache/kudu/blob/88eea89cc2e8f019c5f80e4ba7eb2947dc646f64/src/kudu/security/tls_context.cc#L258-L286 At least, it makes sense to verify this order works as expected for the use cases we expect to have here. -- 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: 4 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: Mon, 14 Jul 2025 18:45:34 +0000 Gerrit-HasComments: Yes
