Andrew Wong 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 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/17189/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17189/2//COMMIT_MSG@33 PS2, Line 33: requires first : to squeasel nit: "requires pushing an update to squeasel"? http://gerrit.cloudera.org:8080/#/c/17189/2/src/kudu/rpc/messenger.cc File src/kudu/rpc/messenger.cc: http://gerrit.cloudera.org:8080/#/c/17189/2/src/kudu/rpc/messenger.cc@148 PS2, Line 148: std::string nit: can drop the std::s here and elsewhere http://gerrit.cloudera.org:8080/#/c/17189/2/src/kudu/rpc/server_negotiation.cc File src/kudu/rpc/server_negotiation.cc: http://gerrit.cloudera.org:8080/#/c/17189/2/src/kudu/rpc/server_negotiation.cc@610 PS2, Line 610: // Check that the handshake step didn't produce an error. It also propagates : // any non-OK status. : RETURN_NOT_OK(s); Wouldn't this return early if 's' is Incomplete in the needs_extra_step case, even if we've already handled the needs_extra_step above? Should we reset 's' after L607? http://gerrit.cloudera.org:8080/#/c/17189/2/src/kudu/security/tls_context.h File src/kudu/security/tls_context.h: http://gerrit.cloudera.org:8080/#/c/17189/2/src/kudu/security/tls_context.h@181 PS2, Line 181: // The cipher suite preferences to use for TLS-secured RPC connections. Uses the OpenSSL : // cipher preference list format. See man (1) ciphers for more information. nit: I suppose it's implied from the below comment, but maybe also mention here that these are pre-1.3 ciphers only? http://gerrit.cloudera.org:8080/#/c/17189/2/src/kudu/security/tls_context.cc File src/kudu/security/tls_context.cc: http://gerrit.cloudera.org:8080/#/c/17189/2/src/kudu/security/tls_context.cc@117 PS2, Line 117: constexpr const char* const kTLSv1_1 = "TLSv1.1"; > kKTlSv11 instead of kTLSv1_1 ? No, thank you :) +11 :) http://gerrit.cloudera.org:8080/#/c/17189/2/src/kudu/security/tls_context.cc@241 PS2, Line 241: nit: add space http://gerrit.cloudera.org:8080/#/c/17189/2/src/kudu/security/tls_handshake-test.cc File src/kudu/security/tls_handshake-test.cc: http://gerrit.cloudera.org:8080/#/c/17189/2/src/kudu/security/tls_handshake-test.cc@327 PS2, Line 327: server.set_verification_mode(security::TlsVerificationMode::VERIFY_NONE); nit: mind adding a comment whether/why this is important for this test? http://gerrit.cloudera.org:8080/#/c/17189/2/src/kudu/security/tls_handshake-test.cc@358 PS2, Line 358: verfy nit: very http://gerrit.cloudera.org:8080/#/c/17189/2/src/kudu/security/tls_handshake-test.cc@375 PS2, Line 375: nit: extra space -- 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: 2 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: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 22 Mar 2021 22:13:02 +0000 Gerrit-HasComments: Yes
