Sailesh Mukil has posted comments on this change. Change subject: KuduRPC integration with OpenSSL ......................................................................
Patch Set 2: (83 comments) Thanks Dan, Alexey and Todd. I've made the changes. Sorry for the delay. http://gerrit.cloudera.org:8080/#/c/4789/2//COMMIT_MSG Commit Message: Line 34: - Make SSL methods (SSLv23, TLS1, etc.) configurable and OpenSSL > yea, I think it's very important that we disallow SSLv2/v3. Isn't that just Yes, but configurable in the sense TLSv1 vs TLSv2, etc. if the users have a preference. http://gerrit.cloudera.org:8080/#/c/4789/1/CMakeLists.txt File CMakeLists.txt: PS1, Line 836: STATIC_LIB > I think we want SHARED_LIB here, so we aren't statically linking. Oops, yes, changed it. http://gerrit.cloudera.org:8080/#/c/4789/2/CMakeLists.txt File CMakeLists.txt: PS2, Line 834: include_directories(${OPENSSL_INCLUDE_DIR}) > Why not to add this only when it's relevant? I.e. in util CMakeFiles.txt Done PS2, Line 835: ADD_THIRDPARTY_LIB(openssl_ssl : STATIC_LIB "${OPENSSL_SSL}") : ADD_THIRDPARTY_LIB(openssl_crypto : STATIC_LIB "${OPENSSL_CRYPTO}") > Most likely, we don't want this to be third-party, we will use system libra Done http://gerrit.cloudera.org:8080/#/c/4789/1/cmake_modules/FindOpenSSL.cmake File cmake_modules/FindOpenSSL.cmake: Line 25: set(_OPENSSL_SEARCH_DIR) > Is there a reason not use the built in FindOpenSSL in Cmake? Ah, I'm used to a older version of cmake. Didn't realize there is a built in one now. Removed this file. http://gerrit.cloudera.org:8080/#/c/4789/2/cmake_modules/FindOpenSSL.cmake File cmake_modules/FindOpenSSL.cmake: > Why is it needed? Aren't that installed/share/cmake-3.6/Modules/FindOpenSS Yes, I'm used to an older version of cmake, I didn't realize that the new version had inbuilt support. Removed this. http://gerrit.cloudera.org:8080/#/c/4789/1/src/kudu/rpc/connection.cc File src/kudu/rpc/connection.cc: PS1, Line 71: NULL > prefer using nullptr here and below. Done http://gerrit.cloudera.org:8080/#/c/4789/2/src/kudu/rpc/connection.cc File src/kudu/rpc/connection.cc: PS2, Line 74: bool ssl_enabled = reactor_thread_->reactor()->messenger()->ssl_enabled(); : if (ssl_enabled) { : socket_.reset(reactor_thread_->reactor()->messenger()->ssl_factory()->CreateSocket( : socket, direction_ == SERVER)); : } else { : socket_.reset(new Socket(socket)); : } : > rather than doing this work in the ctor, does it make sense to do this at t Yes, that makes more sense. I've done that now. http://gerrit.cloudera.org:8080/#/c/4789/1/src/kudu/rpc/connection.h File src/kudu/rpc/connection.h: PS1, Line 144: Socket *socket() > Keep the asterisk with the type: Done http://gerrit.cloudera.org:8080/#/c/4789/2/src/kudu/rpc/connection.h File src/kudu/rpc/connection.h: PS2, Line 232: gscoped_ptr > Consider using std::unique_ptr instead. Done http://gerrit.cloudera.org:8080/#/c/4789/1/src/kudu/rpc/messenger.cc File src/kudu/rpc/messenger.cc: Line 60: DEFINE_string(ssl_server_certificate, "", "Path to the SSL certificate to be used."); > Mark these experimental, for example: Done http://gerrit.cloudera.org:8080/#/c/4789/2/src/kudu/rpc/messenger.cc File src/kudu/rpc/messenger.cc: PS2, Line 59: : DEFINE_string(ssl_server_certificate, "", "Path to the SSL certificate to be used."); : DEFINE_string(ssl_private_key, "", : "Path to the private key to be used to complement the public key present in " : "--ssl_server_certificate"); : DEFINE_string(ssl_certificate_authority, "", : > can we tag these as experimental for now? I don't think we want to sign up Done Line 167: if (ssl_enabled()) { > how about checking if (ssl_factory_) since even if ssl is enabled, maybe it Done http://gerrit.cloudera.org:8080/#/c/4789/1/src/kudu/rpc/messenger.h File src/kudu/rpc/messenger.h: Line 188: bool ssl_enabled() { return ssl_enabled_; } > const Done PS1, Line 189: > whitespace Done http://gerrit.cloudera.org:8080/#/c/4789/2/src/kudu/rpc/messenger.h File src/kudu/rpc/messenger.h: Line 220: bool ssl_enabled_; > maybe I'm being pedantic, but shouldn't all of these variables and classes We could, but the library is still called OpenSSL and all the exposed APIs are SSL_*(). I honestly don't mind it either way. If you think it should be changed, I can go ahead and do that. http://gerrit.cloudera.org:8080/#/c/4789/1/src/kudu/rpc/rpc-test-base.h File src/kudu/rpc/rpc-test-base.h: Line 345: int n_reactors = 1, bool enable_ssl = false) { > Put the new arg on it's own line, and please fix up the indent while you ar Done http://gerrit.cloudera.org:8080/#/c/4789/2/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: Line 52: class TestRpc : public RpcTestBase, public ::testing::WithParamInterface<bool> { > just a thought: if you made the RpcTestBase extend WithParamInterface, woul It would be a lot harder to control for 2 reasons: 1) As you said, other tests depend on it. 2) Some tests here use the StartFakeServer() which does not use the RPC code path, so we cannot have those tests pass with SSL. So I've purposely not added them as parameterized tests. PS2, Line 55: parametrized > nit: parameterized Done PS2, Line 56: ,Te > nit: missing space Done http://gerrit.cloudera.org:8080/#/c/4789/2/src/kudu/rpc/sasl_client.h File src/kudu/rpc/sasl_client.h: PS2, Line 70: Required for some mechanisms. > this bit of the comment seems out of place Done http://gerrit.cloudera.org:8080/#/c/4789/1/src/kudu/rpc/sasl_rpc-test.cc File src/kudu/rpc/sasl_rpc-test.cc: PS1, Line 53: NULL > prefer nullptr here and below Done http://gerrit.cloudera.org:8080/#/c/4789/2/src/kudu/rpc/sasl_rpc-test.cc File src/kudu/rpc/sasl_rpc-test.cc: PS2, Line 53: NUL > nit: prefer nullptr here and elsewhere Done http://gerrit.cloudera.org:8080/#/c/4789/1/src/kudu/rpc/sasl_server.h File src/kudu/rpc/sasl_server.h: Line 48: // Does not take ownership of the socket indicated by the fd. > Update this comment. Done PS1, Line 69: > whitespace Done http://gerrit.cloudera.org:8080/#/c/4789/2/src/kudu/rpc/sasl_server.h File src/kudu/rpc/sasl_server.h: Line 69: // Set the underlying socket to be used. > nit: trailing whitesapce Done http://gerrit.cloudera.org:8080/#/c/4789/2/src/kudu/testutil/server-cert.pem File src/kudu/testutil/server-cert.pem: > Consider having these in code of the test and writing into a file under uni Done. Please see comment below too. > agreed. Otherwise you'll need to at least add this to the RAT exclude list, Done. The only issue is that this gets written to a file once for every test, since each test directory is a different path. http://gerrit.cloudera.org:8080/#/c/4789/2/src/kudu/testutil/server-key.pem File src/kudu/testutil/server-key.pem: > Instead of adding this file, consider having this as a string in correspond Done http://gerrit.cloudera.org:8080/#/c/4789/2/src/kudu/util/CMakeLists.txt File src/kudu/util/CMakeLists.txt: PS2, Line 205: openssl_crypto > ${OPENSSL_CRYPTO_LIBRARY} Removed this and added to L223 PS2, Line 206: openssl_ssl > ${OPENSSL_SSL_LIBRARY} Removed this and added to L223 http://gerrit.cloudera.org:8080/#/c/4789/1/src/kudu/util/net/ssl_factory.cc File src/kudu/util/net/ssl_factory.cc: PS1, Line 69: NULL > nullptr here and elsewhere. Done http://gerrit.cloudera.org:8080/#/c/4789/2/src/kudu/util/net/ssl_factory.cc File src/kudu/util/net/ssl_factory.cc: PS2, Line 29: Mutex* > Consider using std::mutex instead. Also, what is the reason of allocating Yes, as Todd mentioned, these will be alive for the lifetime of the process, so it would be better to have them in the heap. PS2, Line 29: Mutex* > I disagree with the std::mutex thing - we don't typically use those, and in Done PS2, Line 42: return pthread_self(); > This is not portable: pthread_t can be any type (like a structure). Instea Went with Todd's suggestion. PS2, Line 42: return pthread_self(); > I think we have some wrapper in Thread which is a portable way of getting a I used the Thread::UniqueThreadId() call PS2, Line 52: new Mutex > Where is the corresponding delete part? These are purposefully leaked. There is no SSL teardown code as it needs to be alive for the lifetime of the process. Line 55: // Callbacks used by OpenSSL required in a multi-threaded setting. > Are these going to play nice if you're also using OpenSSL from the squeasel Looks like you've already put out a patch for this from the squeasel side, so no changes here. PS2, Line 57: CRYPTO_set_id_callback > From 1.0.0, CRYPTO_set_id_callback() is replaced/deprecated by CRYPTO_THREA Done PS2, Line 60: GoogleOnceType > Consider using std::flag_once from the standard library. Done PS2, Line 63: GoogleOnceInit > Consider using std::call_once() from the standard library. Done Line 66: Status SSLFactory::Init() { > CHECK(!ctx_) here to make sure we don't double-init and leak Done Line 67: // TODO(sailesh): Make method configurable. > not sure it needs to be configurable. Perhaps we should be doing something Done PS2, Line 74: SSL_VERIFY_FAIL_IF_NO_PEER_CERT > we don't expect the client to have a cert in the Kudu use case, so we'll ne The only way I see to do this is to introduce another flag. Do you think there is any other way? PS2, Line 92: NetworkError > To me this does not look like a network error: it's close either to NotFoun Done PS2, Line 116: if (!ssl_socket) > this condition is unnecessary since 'new' will always return non-NULL Done 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 20: #include <openssl/err.h> : #include <openssl/ssl.h> : #include <string> : #include <sys/uio.h> > nit: consider re-ordering these as prescribed by google style guide: https: Done Line 34: // Setup the OpenSSL library. > specify when this must be called and how many times. I initially wanted to follow the pattern that the destructor destroys whatever is setup by the constructor. But we don't shutdown the SSL lib. Teardown() currently cleans up what is set up by Init(). If we use a constructor-destructor, the destructor will destroy what Init() sets up. But I don't think it matters that much to introduce 2 new functions. So I changed it to how you suggested. And I've added Alexy's suggestion of using a custom deleter for ctx_, so the destructor is now empty. Line 38: public: > please add a ctor and dtor (and define them in the .cc file) even if they'r Done PS2, Line 42: // Teardown any context set up by this class. : void Teardown(); > do you expect that this will block? Given it doesn't return any error, why Same explanation as above, but changed it. And it shouldn't block. PS2, Line 56: bool is_server); > we usually prefer enums to bools, eg: Yes I initially wanted to go that way, but that would mean exposing Connection::Direction to this class and SSLSocket which seems a little messy to me. Or I could expose it as a util/net/. What do you think? PS2, Line 60: SSL_CTX > Consider using unique_ptr with custom deleter. Done PS2, Line 62: SSL error queue > please document that this is a thread-local concept. I've added ERR_clear_error before every SSL_* call now. I also changed to using ERR_get_error() instead. PS2, Line 63: ssp > typo Done Line 64: static std::string GetLastError(int errno_copy) { > seems better to define this in the .cc file in an anonymous namespace, sinc This function is also used by SSLSocket, which is why I've made SSLSocket a friend class. And it's a static function too. Line 67: if (error_code != 0) { > I think this would be clearer to invert this condition so the shorter thing Done PS2, Line 72: "SSL error " + error_code; > that's not what you want. I think you mean Substitute("SSL error $0", error Done http://gerrit.cloudera.org:8080/#/c/4789/2/src/kudu/util/net/ssl_socket.cc File src/kudu/util/net/ssl_socket.cc: Line 19: #include <openssl/err.h> > nit: please add an empty line between these: Done PS2, Line 29: ssl_ = ssl; : SSL_set_fd(ssl_, fd); : is_server_ = is_server; > use an initializer list for the assignments of ssl_ and is_server_ Done Line 37: std::string GetX509CommonName(X509* cert, int* cn_size) { > this function and a few below are only used within this compilation unit, s Done PS2, Line 38: std::string() > here and below, you can just return ""; Done Line 53: *cn_size = ASN1_STRING_to_UTF8(&utf8_cn, cn_asn1); > The example on https://wiki.openssl.org/index.php/Hostname_validation shows Done PS2, Line 54: if (*cn_size < 0) return std::string() > Is it necessary to release memory allocated by cn_asn1 if returning from he No, that's an internal pointer so it shouldn't be freed. Line 56: std::string common_name(reinterpret_cast<char*>(utf8_cn)); > why not pass *cn_size as the second argument to this constructor, and then Done Line 63: // Return 'true' if successful and 'false' otherwise. > I think it's worth adding a comment here that references https://wiki.opens Done Line 64: bool VerifyHost(const std::string& hostname, const std::string& subject_name, int size) { > would be nice to have some specific unit tests for this one, since it's got Added a test in net-util_test Line 68: // RFC-6125 Section 6.4.3, item 1: Only the complete leftmost label may have a wildcard. > wondering if this would be easier to read doing something like: Done Line 73: int i = 0, j = 0; > why not define these inside the for loop? Done PS2, Line 76: while (j < hostname_size && hostname[j] != '.') { : ++j; : } > is this greedy match always right? the RFC 6125 seems to indicate that you Done PS2, Line 105: X509* cert > Consider using unique_ptr with custom deleter to avoid memory leaks in case Done PS2, Line 116: return > Is it necessary to release cert before returning? Yes. Forgot to do that. Done. PS2, Line 122: return > Is it necessary to release cert before returning? Done Line 125: peer_addr.LookupHostname(&peer_hostname); > need to check result (eg if reverse DNS fails) Done PS2, Line 129: return > Is it necessary to release cert before returning? Done Line 136: if (ssl_ == NULL) return Status::NetworkError("SSLSocket::Write(): SSL context unavailable"); > this can probably just be a CHECK, since it would be programmer error. (sam Done Line 155: for (int i = 0; i < iov_len; ++i) { > We typically set O_NODELAY on our sockets, so each of the underlying Write Added a Socket::SetTcpCork() helper function and used that. PS2, Line 157: int32_t bytes_written = SSL_write(ssl_, iov[i].iov_base, frame_size); : if (bytes_written <= 0) { : if (SSL_get_error(ssl_, bytes_written) == SSL_ERROR_WANT_WRITE) { : // Socket not ready to write yet. : *nwritten = 0; : return Status::OK(); : } : return Status::NetworkError("SSL_write", SSLFactory::GetLastError(errno)); : } : total_written += bytes_written; > can you refactor out this common code with Write above? or just call throug Done Line 196: if (ret == 0) ret = SSL_shutdown(ssl_); > this doesn't seem right, since this seems like it's a blocking call. Given Both the calls do not block since the underlying socket is still a non blocking socket. The second shutdown is to receive a notification from the peer that the shutdown was successful for the peer. But I think we can do away with that since we're going to close the socket anyway. Done. PS2, Line 207: Socket::Close() > this is calling it again which seems wrong Done http://gerrit.cloudera.org:8080/#/c/4789/2/src/kudu/util/net/ssl_socket.h File src/kudu/util/net/ssl_socket.h: PS2, Line 20: #include <openssl/ssl.h> : #include <sys/uio.h> : #include <string> > nit: consider re-ordering these as prescribed by google style guide: https: Done PS2, Line 34: explicit > Is it really needed here? Nope, that was a mistake. Removed it. PS2, Line 40: Status Write(const uint8_t *buf, int32_t amt, int32_t *nwritten); : : Status Writev(const struct ::iovec *iov, int iov_len, int32_t *nwritten); : : Status Recv(uint8_t *buf, int32_t amt, int32_t *nread); > these should all have the 'override' keyword on them Done PS2, Line 49: Inherits > better to say 'Owned'. Inherits sounds like it's something coming from a su 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: 2 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
