Todd Lipcon has posted comments on this change. Change subject: KuduRPC integration with OpenSSL ......................................................................
Patch Set 4: (24 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()); > What happens if the socket_ does not actually contains an object of SSLSock we actually have a template called down_cast<> that does that verification and CHECK 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 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? Here I think you're saying it's enabled only of all are non-empty, which I guess is reasonable too, but then it would be good to give some kind of bad status error if the user misconfigured and forgot one of them, rather than silently not using SSL (!!) 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 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 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* Line 356: static const std::string test_private_key = R"( same 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 "multiple wildcards" and "wildcard in the second component" Line 163: ASSERT_FALSE(VerifyHost(hostname, subject_name)); how about another test for empty hostname, just to be safe? 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 it a no-op for now is fine, and add a TODO to use TCP_NOPUSH on osx if we ever care about perf 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 here Line 119: CHECK_NOTNULL(ctx_.get()); this will give an unused result warning. You can either change this to CHECK(ctx_); or below do: SSL_new(CHECK_NOTNULL(ctx_.get()) PS4, Line 125: SSLSocket* ssl_socket = new SSLSocket(socket_fd, ssl, is_server); : return ssl_socket; no need for intermediate local variable http://gerrit.cloudera.org:8080/#/c/4789/2/src/kudu/util/net/ssl_factory.h File src/kudu/util/net/ssl_factory.h: PS2, Line 56: certificate. > Yes I initially wanted to go that way, but that would mean exposing Connect I think I'd add an inner enum inside this class which just happens to have the same fields Line 64: SSLSocket* CreateSocket(int socket_fd, bool is_server); > This function is also used by SSLSocket, which is why I've made SSLSocket a OK, I still think it makes sense to move the definition to the .cc (even if it's kept as a private static function) 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 39: OpenSSLInit > Is this forward declaration? why not just use std::call_once in SSLFactory::SSLFactory to make sure it's initialized, without callers having to worry about it? Actually, it appears you are already doing that, and this 'OpenSSLInit' function isn't implemented anywhere Line 63: // a server socket. probably worth commenting also whether the returned SSLSocket 'owns' socket_fd now (i.e will it get closed when the SSLSocket is destructed?) Line 74: std::string reason; I don't think this local variable is very useful anymore (see below) Line 78: reason = error_reason; why not just 'return error_reason;' here? Line 80: reason = strings::Substitute("SSL error $0", error_code); and 'return strings::Substitute(...)' 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 Line 70: return MatchPattern(hostname, subject_name); I'm not sure this is matching the RFC exactly: . If the wildcard character is the only character of the left-most label in the presented identifier, the client SHOULD NOT compare against anything but the left-most label of the reference identifier (e.g., *.example.com would match foo.example.com but not bar.foo.example.com or example.com). also, the RFC says the host should be canonicalized to lower case first. Maybe we should be pulling in some other library's implementation of this, since it's fairly important for security? https://github.com/google/boringssl/blob/master/crypto/x509v3/v3_utl.c for example Line 111: std::string common_name = GetX509CommonName(cert.get()); The RFCs I've been reading seem to indicate we should not just be checking the X509 common name, but actually prefer the SubjectAltNames first. http://bxr.su/OpenBSD/lib/libtls/tls_verify.c is another source we could borrow from here. Or https://cs.chromium.org/chromium/src/net/cert/x509_certificate.cc?q=RFC+6125&sq=package:chromium&dr=C which has code that's more in our style. 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 it's only visible for testing, add that to a comment. -- 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
