Sailesh Mukil has posted comments on this change. Change subject: KuduRPC integration with OpenSSL ......................................................................
Patch Set 4: (34 comments) http://gerrit.cloudera.org:8080/#/c/4789/4/src/kudu/rpc/connection.cc File src/kudu/rpc/connection.cc: PS4, Line 626: static_cast<SSLSocket*>(socket_.get()); > we actually have a template called down_cast<> that does that verification Done PS4, Line 626: static_cast<SSLSocket*>(socket_.get()); > What happens if the socket_ does not actually contains an object of SSLSock I used the down_cast<>. Done. http://gerrit.cloudera.org:8080/#/c/4789/4/src/kudu/rpc/messenger.cc File src/kudu/rpc/messenger.cc: Line 172: ssl_factory_.reset(); > if you're just resetting, you don't need the if() anymore Done Line 280: ssl_enabled_ = !(FLAGS_rpc_ssl_server_certificate.empty() || FLAGS_rpc_ssl_private_key.empty() > I think this boolean is wrong. Shouldn't this enabled if _any_ is non-empty Ah I was going for the latter, i.e. unless all are configured, SSL will be disabled. But I guess the contract you're proposing makes more sense. If they forget to configure one or more of them, this should fail while trying to load the certificates. I've changed it to that. Done. http://gerrit.cloudera.org:8080/#/c/4789/4/src/kudu/rpc/messenger.h File src/kudu/rpc/messenger.h: Line 188: const bool ssl_enabled() { return ssl_enabled_; } > I think you meant: bool ssl_enabled() const Yup. Done. http://gerrit.cloudera.org:8080/#/c/4789/4/src/kudu/rpc/reactor.cc File src/kudu/rpc/reactor.cc: Line 347: // Register the new connection in our map. > hrm, this comment now seems misplaced Done http://gerrit.cloudera.org:8080/#/c/4789/4/src/kudu/rpc/rpc-test-base.h File src/kudu/rpc/rpc-test-base.h: Line 322: static const std::string test_server_cert = R"( > should be const char* Done Line 356: static const std::string test_private_key = R"( > same Done http://gerrit.cloudera.org:8080/#/c/4789/4/src/kudu/util/net/net_util-test.cc File src/kudu/util/net/net_util-test.cc: Line 159: subject_name = "*oo.ba*.com"; > how about also testing "foo.*.com' here? (here you are testing both "multip Done Line 163: ASSERT_FALSE(VerifyHost(hostname, subject_name)); > how about another test for empty hostname, just to be safe? Done 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 Done 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 Done Line 56: ssl_mutexes.push_back(new Mutex()); > First of all, why to leak those at all? I.e. why not to release those on t We don't shutdown the OpenSSL library, so in the theoretical case of 2 Messenger objects being created, the second object will use-after-free the Mutex objects since they would have been freed with the shutdown of the first Messenger object and also since DoSSLInit() is called only once. PS4, Line 66: [](SSL_CTX* x) { SSL_CTX_free(x); } > Nit: if ctx_ were defined as unique_ptr<SSL_CTX, std::function<void(SSL_CTX Done Line 119: CHECK_NOTNULL(ctx_.get()); > this will give an unused result warning. You can either change this to CHEC Done PS4, Line 125: SSLSocket* ssl_socket = new SSLSocket(socket_fd, ssl, is_server); : return ssl_socket; > no need for intermediate local variable Done http://gerrit.cloudera.org:8080/#/c/4789/4/src/kudu/util/net/ssl_factory.h File src/kudu/util/net/ssl_factory.h: PS4, Line 17: #ifndef KUDU_UTIL_NET_SSL_FACTORY_H : #define KUDU_UTIL_NET_SSL_FACTORY_H > nit: consider using Do you think it's necessary for ssl_socket.h too? PS4, Line 37: function > nit: function --> method I've removed this function now. PS4, Line 39: OpenSSLInit > Is this forward declaration? This function was removed and I forgot to get rid of the declaration. Done. PS4, Line 39: OpenSSLInit > why not just use std::call_once in SSLFactory::SSLFactory to make sure it's Oops, sorry. Yes I was supposed to remove this declaration. Done. PS4, Line 51: void Teardown(); > As far as I know, the OpenSSL library contains some statics which cannot be Sorry, I got rid of this function too and forgot to remove the declaration. We decided to not go with tearing down the OpenSSL library. PS4, Line 57: > nit: an extra space Done Line 63: // a server socket. > probably worth commenting also whether the returned SSLSocket 'owns' socket Yes, I've added that as a comment now. PS4, Line 64: SSLSocket* > Consider returning unique_ptr<SSLSocket> instead: the signature with smart Done Line 74: std::string reason; > I don't think this local variable is very useful anymore (see below) Ah, forgot to get rid of it with a refactor. Done. Line 78: reason = error_reason; > why not just 'return error_reason;' here? Done Line 80: reason = strings::Substitute("SSL error $0", error_code); > and 'return strings::Substitute(...)' Done http://gerrit.cloudera.org:8080/#/c/4789/4/src/kudu/util/net/ssl_socket.cc File src/kudu/util/net/ssl_socket.cc: PS4, Line 60: size > there's no 'size' parameter anymore Done Line 70: return MatchPattern(hostname, subject_name); > I'm not sure this is matching the RFC exactly: Yes you're right. I've ported the X509_check_host() function from a later version of OpenSSL and included the correct licensing. I had to tweak the code slightly to get it to build and work well. The code is in src/kudu/util/x509_check_host.cc/h I'm not sure if there are some guidelines to follow while porting code, so please do let me know if I need to change anything. I've also gotten rid of the GetX509CommonName and VerifyHost functions. Line 111: std::string common_name = GetX509CommonName(cert.get()); > Yep, please check X509_VERIFY_PARAM_set1_host instead: Done Line 111: std::string common_name = GetX509CommonName(cert.get()); > The RFCs I've been reading seem to indicate we should not just be checking I didn't want to include SANs in the initial patch at first as I thought it'll be harder to review. In any case now, it's taken care of by the ported code. As we spoke, Chromium has a lot of util baggage which would be a lot to port over, so I went with porting from a later version of OpenSSL. 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 Done 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"); : } > Sailesh pointed to the fact that those X509_VERIFY_PARAM_set1_xxx() methods Thanks Alexey. Please see comment above. Also, the code is in src/kudu/util. I didn't put it under security because we link openssl only to util_libs now. If you think it'll be better to have it under src/kudu/security, I can change that. I only ported x509_check_host() as it's a lot less code to port than X509_VERIFY_PARAM_set1_host(). http://gerrit.cloudera.org:8080/#/c/4789/4/src/kudu/util/net/ssl_socket.h File src/kudu/util/net/ssl_socket.h: Line 29: bool VerifyHost(const std::string& hostname, const std::string& subject_name); > this should be namespaced at least inside of kudu, and have some docs. If i Done -- 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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
