Alexey Serbin has posted comments on this change. Change subject: KuduRPC integration with OpenSSL ......................................................................
Patch Set 4: (4 comments) Some more comments. http://gerrit.cloudera.org:8080/#/c/4789/4/src/kudu/util/net/socket.cc File src/kudu/util/net/socket.cc: Line 174: if (setsockopt(fd_, IPPROTO_TCP, TCP_CORK, &flag, sizeof(flag)) == -1) { > we probably need to ifdef this out on OSX, right OSX users? I think making Correct, there is no TCP_CORK on OS X. There are at least a couple of options how to deal with it in the code: 1. use #if defined(TCP_NODELAY) ... #endif approach 2. use #if defined(__APPLE__) ... #endif You can check how the __APPLE__ macro is used in the Kudu's code for the examples of the latter. http://gerrit.cloudera.org:8080/#/c/4789/4/src/kudu/util/net/ssl_factory.cc File src/kudu/util/net/ssl_factory.cc: Line 56: ssl_mutexes.push_back(new Mutex()); > I think to avoid LSAN errors you might need to use ScopedLeakCheckDisabler First of all, why to leak those at all? I.e. why not to release those on teardown? http://gerrit.cloudera.org:8080/#/c/4789/4/src/kudu/util/net/ssl_socket.cc File src/kudu/util/net/ssl_socket.cc: Line 111: std::string common_name = GetX509CommonName(cert.get()); > The RFCs I've been reading seem to indicate we should not just be checking Yep, please check X509_VERIFY_PARAM_set1_host instead: https://www.openssl.org/docs/man1.0.2/crypto/X509_VERIFY_PARAM_set1_host.html or at least X509_check_host (but it's best to use X509_VERIFY_PARAM_xxx instead) https://www.openssl.org/docs/man1.0.2/crypto/X509_check_host.html PS4, Line 116: // Get the peer's hostname : Sockaddr peer_addr; : if (!GetPeerAddress(&peer_addr).ok()) { : return Status::NetworkError("Handshake failed: Could not retreive peer address"); : } : std::string peer_hostname; : RETURN_NOT_OK(peer_addr.LookupHostname(&peer_hostname)); : : // Verify if the hostname and the CommonName match. : if (!VerifyHost(peer_hostname, common_name)) { : return Status::NetworkError("Handshake failed: Could not verify host with certificate"); : } Like Todd already mentioned above, there might be IP address or IP network in the subject alternative name; combinations of those, etc. Why not to use standard OpenSSL methods? https://www.openssl.org/docs/man1.0.2/crypto/X509_VERIFY_PARAM_set1_host.html The underlying methods, like X509_check_host(), etc. already implemented to perform those checks in accordance with RFC: https://www.openssl.org/docs/man1.0.2/crypto/X509_check_host.html -- To view, visit http://gerrit.cloudera.org:8080/4789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I27167faa4e6a78e59b46093055b16682c93af0ea Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
