Sailesh Mukil has posted comments on this change. Change subject: KuduRPC integration with OpenSSL ......................................................................
Patch Set 6: (18 comments) http://gerrit.cloudera.org:8080/#/c/4789/6/LICENSE.txt File LICENSE.txt: PS6, Line 512: "This product includes software developed by the OpenSSL Project : for use in the OpenSSL Toolkit (http://www.openssl.org/)" > I think this text needs to go in NOTICE.txt Done PS6, Line 529: This product includes cryptographic software written by Eric Young : (e...@cryptsoft.com). This product includes software written by Tim : Hudson (t...@cryptsoft.com). > and potentially this as well in NOTICE.txt Done http://gerrit.cloudera.org:8080/#/c/4789/6/src/kudu/rpc/connection.h File src/kudu/rpc/connection.h: Line 41: #include "kudu/util/net/ssl_socket.h" > this include isn't necessary Done http://gerrit.cloudera.org:8080/#/c/4789/6/src/kudu/rpc/rpc-test-base.h File src/kudu/rpc/rpc-test-base.h: Line 321: Status CreateSSLServerCert(const std::string& file_path) { > should be marked inline Done Line 321: Status CreateSSLServerCert(const std::string& file_path) { > warning: function 'CreateSSLServerCert' defined in a header file; function Done Line 355: Status CreateSSLPrivateKey(const std::string& file_path) { > warning: function 'CreateSSLPrivateKey' defined in a header file; function Done Line 355: Status CreateSSLPrivateKey(const std::string& file_path) { > same Done http://gerrit.cloudera.org:8080/#/c/4789/6/src/kudu/rpc/sasl_server.h File src/kudu/rpc/sasl_server.h: PS6, Line 69: // Set the underlying socket to be used. : // Must be called before Init(). : void set_socket(Socket* socket) { sock_ = socket; } > no longer used Done http://gerrit.cloudera.org:8080/#/c/4789/6/src/kudu/util/net/ssl_factory.cc File src/kudu/util/net/ssl_factory.cc: Line 66: static std::once_flag ssl_once; > this can be a local static variable down inside the SSLFactory ctor Done PS6, Line 83: NetworkError > RuntimeError Done PS6, Line 96: errno > I just grepped the openssl source and I don't see it explicitly setting err Yes, the OpenSSL manuals claim that if a call returns with an error and the SSL error queue is empty, then the errno would most likely be set, presumably by underlying syscalls. I've added code to reset errno before every use now. http://gerrit.cloudera.org:8080/#/c/4789/6/src/kudu/util/net/ssl_factory.h File src/kudu/util/net/ssl_factory.h: Line 64: static std::string GetLastError(int errno_copy) { > still think this definition should move to the .cc file Done http://gerrit.cloudera.org:8080/#/c/4789/6/src/kudu/util/net/ssl_socket.cc File src/kudu/util/net/ssl_socket.cc: Line 47: CHECK_NOTNULL(ssl_); > should be CHECK(ssl_) to avoid an unused result warning (same below in a fe Done Line 55: if (ret <= 0) return Status::NetworkError(SSLFactory::GetLastError(errno)); > same comment about errno Done Line 79: RETURN_NOT_OK(peer_addr.LookupHostname(&peer_hostname)); > what happens if the remote address has no reverse DNS associated? does this If the reverse lookup fails, the IP address will be returned as a string. Although it doesn't explicitly state that in the manpage, I just did a test to confirm that. http://man7.org/linux/man-pages/man3/getnameinfo.3.html It will not return a bad status when the lookup fails unless we set the NI_NAMEREQD flag which we do not set. Line 89: return Status::NetworkError("Handshake failed:", SSLFactory::GetLastError(errno)); > same errno question Done http://gerrit.cloudera.org:8080/#/c/4789/6/src/kudu/util/x509_check_host.h File src/kudu/util/x509_check_host.h: Line 12: #ifndef X509_CHECK_HOST_H > is this going to be a problem on machines that have OpenSSL 1.1? maybe we s I've disabled compilation of these files when the OpenSSL version is 1.0.2 or above: https://gerrit.cloudera.org/#/c/4789/6/src/kudu/util/CMakeLists.txt Line 45: int X509_check_host(X509 *x, const char *chk, size_t chklen, > maybe we should prefix this name as well with kudu_x509_check_host or put i Please refer above comment. If you still think it's necessary to add the KUDU_ prefixes, I'll do so. -- 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: 6 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
