Todd Lipcon has posted comments on this change. Change subject: KuduRPC integration with OpenSSL ......................................................................
Patch Set 6: (16 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 PS6, Line 529: This product includes cryptographic software written by Eric Young : ([email protected]). This product includes software written by Tim : Hudson ([email protected]). and potentially this as well in NOTICE.txt I pinged [email protected] to try to confirm this. 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 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) { > warning: function 'CreateSSLServerCert' defined in a header file; function should be marked inline Line 355: Status CreateSSLPrivateKey(const std::string& file_path) { > warning: function 'CreateSSLPrivateKey' defined in a header file; function same 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 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 PS6, Line 83: NetworkError RuntimeError PS6, Line 96: errno I just grepped the openssl source and I don't see it explicitly setting errno anywhere. Are you expectinng that it would be setting it by virtue of calling some other system call underneath? Probably safest if so to also reset errno = 0 before making any of these calls, if we're going to inspect errno afterwards. 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 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 few places) Line 55: if (ret <= 0) return Status::NetworkError(SSLFactory::GetLastError(errno)); same comment about errno Line 79: RETURN_NOT_OK(peer_addr.LookupHostname(&peer_hostname)); what happens if the remote address has no reverse DNS associated? does this end up with an empty string in peer_hostname when instead we should probably be using the dotted decimal IP address? or does it return a bad Status? Line 89: return Status::NetworkError("Handshake failed:", SSLFactory::GetLastError(errno)); same errno question 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 should be prefixing all of these defines with KUDU_? 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 it in a namespace kudu? Otherwise on a machine that actually has the OpenSSL impl we might fail to build -- 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 <[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
