You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Sailesh Mukil (Code Review)" <ge...@cloudera.org> on 2016/10/21 22:49:47 UTC
[kudu-CR] KuduRPC integration with OpenSSL
Sailesh Mukil has uploaded a new change for review.
http://gerrit.cloudera.org:8080/4789
Change subject: KuduRPC integration with OpenSSL
......................................................................
KuduRPC integration with OpenSSL
This patch adds SSL support for the RPC layer in Kudu. It uses the
OpenSSL library for this purpose.
This is acheived by subclassing 'Socket' to add 'SSLSocket' which
calls into the OpenSSL library for auth/read/write/shutdown.
SSL is enabled only at a 'Connection' object level, i.e. the
AcceptorPool still only works with regular 'Socket's, and the reactor
threads also use a regular 'Socket' on a new outgoing call.
The first point of any SSL activity happens in the context of the
negotiation pool, where the SSL handshake happens before the SASL
handshake. On a successful handshake, further communication is
encrypted on that 'Connection'.
A 'SSLFactory' is created at the 'Messenger' level. This factory is
in-charge of creating 'SSLSocket' objects when necessary and also
keeps a track of the certificates, keys and the SSL context that is
shared among all the sockets of that 'Messenger'.
Added a parameter to the rpc-test to additionally run the tests with
SSL enabled.
Future TODOs:
- Enable authorization of SANs (SubjectAlternativeNames)
- Make SSL methods (SSLv23, TLS1, etc.) configurable and OpenSSL
version aware. (Choosing APIs based on supported versions)
- Allow loading keys as strings vs files. (Need to use different APIs)
Change-Id: I27167faa4e6a78e59b46093055b16682c93af0ea
---
M CMakeLists.txt
A cmake_modules/FindOpenSSL.cmake
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_client.cc
M src/kudu/rpc/sasl_client.h
M src/kudu/rpc/sasl_rpc-test.cc
M src/kudu/rpc/sasl_server.cc
M src/kudu/rpc/sasl_server.h
A src/kudu/testutil/server-cert.pem
A src/kudu/testutil/server-key.pem
M src/kudu/util/CMakeLists.txt
M src/kudu/util/net/socket.h
A src/kudu/util/net/ssl_factory.cc
A src/kudu/util/net/ssl_factory.h
A src/kudu/util/net/ssl_socket.cc
A src/kudu/util/net/ssl_socket.h
22 files changed, 775 insertions(+), 110 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/4789/1
--
To view, visit http://gerrit.cloudera.org:8080/4789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I27167faa4e6a78e59b46093055b16682c93af0ea
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4789
to look at the new patch set (#3).
Change subject: KuduRPC integration with OpenSSL
......................................................................
KuduRPC integration with OpenSSL
This patch adds SSL support for the RPC layer in Kudu. It uses the
OpenSSL library for this purpose.
This is acheived by subclassing 'Socket' to add 'SSLSocket' which
calls into the OpenSSL library for auth/read/write/shutdown.
SSL is enabled only at a 'Connection' object level, i.e. the
AcceptorPool still only works with regular 'Socket's, and the reactor
threads also use a regular 'Socket' on a new outgoing call.
The first point of any SSL activity happens in the context of the
negotiation pool, where the SSL handshake happens before the SASL
handshake. On a successful handshake, further communication is
encrypted on that 'Connection'.
A 'SSLFactory' is created at the 'Messenger' level. This factory is
in-charge of creating 'SSLSocket' objects when necessary and also
keeps a track of the certificates, keys and the SSL context that is
shared among all the sockets of that 'Messenger'.
Added a parameter to the rpc-test to additionally run the tests with
SSL enabled.
Future TODOs:
- Enable authorization of SANs (SubjectAlternativeNames)
- Make SSL methods (SSLv23, TLS1, etc.) configurable and OpenSSL
version aware. (Choosing APIs based on supported versions)
- Allow loading keys as strings vs files. (Need to use different APIs)
Change-Id: I27167faa4e6a78e59b46093055b16682c93af0ea
---
M CMakeLists.txt
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_client.cc
M src/kudu/rpc/sasl_client.h
M src/kudu/rpc/sasl_rpc-test.cc
M src/kudu/rpc/sasl_server.cc
M src/kudu/rpc/sasl_server.h
M src/kudu/util/CMakeLists.txt
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
A src/kudu/util/net/ssl_factory.cc
A src/kudu/util/net/ssl_factory.h
A src/kudu/util/net/ssl_socket.cc
A src/kudu/util/net/ssl_socket.h
22 files changed, 806 insertions(+), 115 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/4789/3
--
To view, visit http://gerrit.cloudera.org:8080/4789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I27167faa4e6a78e59b46093055b16682c93af0ea
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: KuduRPC integration with OpenSSL
......................................................................
Patch Set 3:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/4789/3/src/kudu/rpc/reactor.cc
File src/kudu/rpc/reactor.cc:
Line 51: #include "kudu/util/status.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done
http://gerrit.cloudera.org:8080/#/c/4789/3/src/kudu/rpc/rpc-test-base.h
File src/kudu/rpc/rpc-test-base.h:
Line 321: Status CreateSSLServerCert(std::string file_path) {
> warning: the parameter 'file_path' is copied for each invocation but only u
Done
Line 355: Status CreateSSLPrivateKey(std::string file_path) {
> warning: the parameter 'file_path' is copied for each invocation but only u
Done
http://gerrit.cloudera.org:8080/#/c/4789/3/src/kudu/util/net/socket.h
File src/kudu/util/net/socket.h:
Line 125: Status BlockingWrite(const uint8_t *buf, size_t buflen, size_t *num_written,
> warning: function 'kudu::Socket::BlockingWrite' has a definition with diffe
Done
http://gerrit.cloudera.org:8080/#/c/4789/3/src/kudu/util/net/ssl_factory.cc
File src/kudu/util/net/ssl_factory.cc:
Line 78: if (ctx_.get() == nullptr) {
> warning: redundant get() call on smart pointer [google-readability-redundan
Done
http://gerrit.cloudera.org:8080/#/c/4789/3/src/kudu/util/net/ssl_socket.cc
File src/kudu/util/net/ssl_socket.cc:
Line 62: bool VerifyHost(const std::string& hostname, const std::string& subject_name, int size) {
> warning: parameter 'size' is unused [misc-unused-parameters]
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: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4789
to look at the new patch set (#6).
Change subject: KuduRPC integration with OpenSSL
......................................................................
KuduRPC integration with OpenSSL
This patch adds SSL support for the RPC layer in Kudu. It uses the
OpenSSL library for this purpose.
This is acheived by subclassing 'Socket' to add 'SSLSocket' which
calls into the OpenSSL library for auth/read/write/shutdown.
SSL is enabled only at a 'Connection' object level, i.e. the
AcceptorPool still only works with regular 'Socket's, and the reactor
threads also use a regular 'Socket' on a new outgoing call.
The first point of any SSL activity happens in the context of the
negotiation pool, where the SSL handshake happens before the SASL
handshake. On a successful handshake, further communication is
encrypted on that 'Connection'.
A 'SSLFactory' is created at the 'Messenger' level. This factory is
in-charge of creating 'SSLSocket' objects when necessary and also
keeps a track of the certificates, keys and the SSL context that is
shared among all the sockets of that 'Messenger'.
Added a parameter to the rpc-test to additionally run the tests with
SSL enabled.
Future TODOs:
- Make SSL methods (SSLv23, TLS1, etc.) configurable and OpenSSL
version aware. (Choosing APIs based on supported versions)
- Allow loading keys as strings vs files. (Need to use different APIs)
Change-Id: I27167faa4e6a78e59b46093055b16682c93af0ea
---
M CMakeLists.txt
M LICENSE.txt
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_client.cc
M src/kudu/rpc/sasl_client.h
M src/kudu/rpc/sasl_rpc-test.cc
M src/kudu/rpc/sasl_server.cc
M src/kudu/rpc/sasl_server.h
M src/kudu/util/CMakeLists.txt
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
A src/kudu/util/net/ssl_factory.cc
A src/kudu/util/net/ssl_factory.h
A src/kudu/util/net/ssl_socket.cc
A src/kudu/util/net/ssl_socket.h
A src/kudu/util/x509_check_host.cc
A src/kudu/util/x509_check_host.h
25 files changed, 1,291 insertions(+), 118 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/4789/6
--
To view, visit http://gerrit.cloudera.org:8080/4789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I27167faa4e6a78e59b46093055b16682c93af0ea
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: KuduRPC integration with OpenSSL
......................................................................
Patch Set 2:
(8 comments)
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
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 libraries.
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/FindOpenSSL.cmake is not enough?
Most probably, we will use system crypto and ssl libraries, linking with them dynamically.
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.
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}
PS2, Line 206: openssl_ssl
${OPENSSL_SSL_LIBRARY}
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 92: NetworkError
To me this does not look like a network error: it's close either to NotFound, Corrupted, or RunTimeError.
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 60: SSL_CTX
Consider using unique_ptr with custom deleter.
--
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 <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KuduRPC integration with OpenSSL
......................................................................
Patch Set 2:
(2 comments)
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
I disagree with the std::mutex thing - we don't typically use those, and instead use our own. But agreed that pointers are strange... I guess this is because these are purposefully leaked mutexes and we don't want non-POD values in static storage. It would be worth a comment to that effect.
PS2, Line 42: return pthread_self();
> This is not portable: pthread_t can be any type (like a structure). Instea
I think we have some wrapper in Thread which is a portable way of getting a unique tid. I don't think the hasher approach guarantees any uniqueness.
--
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 <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KuduRPC integration with OpenSSL
......................................................................
Patch Set 6:
btw, you can edit build-support/lint.sh to exclude the x509_check_host.cc file from lint checking.
--
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 <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KuduRPC integration with OpenSSL
......................................................................
Patch Set 7: Code-Review+1 Verified-1
OK, this looks good to me. I'm guessing it will need a rebase, and we're waiting on the legal-discuss@ thread to determine if we need to file the crypto thing with the government before committing.
--
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: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has uploaded a new patch set (#2).
Change subject: KuduRPC integration with OpenSSL
......................................................................
KuduRPC integration with OpenSSL
This patch adds SSL support for the RPC layer in Kudu. It uses the
OpenSSL library for this purpose.
This is acheived by subclassing 'Socket' to add 'SSLSocket' which
calls into the OpenSSL library for auth/read/write/shutdown.
SSL is enabled only at a 'Connection' object level, i.e. the
AcceptorPool still only works with regular 'Socket's, and the reactor
threads also use a regular 'Socket' on a new outgoing call.
The first point of any SSL activity happens in the context of the
negotiation pool, where the SSL handshake happens before the SASL
handshake. On a successful handshake, further communication is
encrypted on that 'Connection'.
A 'SSLFactory' is created at the 'Messenger' level. This factory is
in-charge of creating 'SSLSocket' objects when necessary and also
keeps a track of the certificates, keys and the SSL context that is
shared among all the sockets of that 'Messenger'.
Added a parameter to the rpc-test to additionally run the tests with
SSL enabled.
Future TODOs:
- Enable authorization of SANs (SubjectAlternativeNames)
- Make SSL methods (SSLv23, TLS1, etc.) configurable and OpenSSL
version aware. (Choosing APIs based on supported versions)
- Allow loading keys as strings vs files. (Need to use different APIs)
Change-Id: I27167faa4e6a78e59b46093055b16682c93af0ea
---
M CMakeLists.txt
A cmake_modules/FindOpenSSL.cmake
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_client.cc
M src/kudu/rpc/sasl_client.h
M src/kudu/rpc/sasl_rpc-test.cc
M src/kudu/rpc/sasl_server.cc
M src/kudu/rpc/sasl_server.h
A src/kudu/testutil/server-cert.pem
A src/kudu/testutil/server-key.pem
M src/kudu/util/CMakeLists.txt
M src/kudu/util/net/socket.h
A src/kudu/util/net/ssl_factory.cc
A src/kudu/util/net/ssl_factory.h
A src/kudu/util/net/ssl_socket.cc
A src/kudu/util/net/ssl_socket.h
22 files changed, 775 insertions(+), 110 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/4789/2
--
To view, visit http://gerrit.cloudera.org:8080/4789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I27167faa4e6a78e59b46093055b16682c93af0ea
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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
: (eay@cryptsoft.com). This product includes software written by Tim
: Hudson (tjh@cryptsoft.com).
and potentially this as well in NOTICE.txt
I pinged legal-discuss@apache.org 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 <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: KuduRPC integration with OpenSSL
......................................................................
Patch Set 8: Code-Review+2
--
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: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: KuduRPC integration with OpenSSL
......................................................................
Patch Set 4:
(4 comments)
Some more comments.
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
Correct, there is no TCP_CORK on OS X. There are at least a couple of options how to deal with it in the code:
1. use #if defined(TCP_NODELAY) ... #endif approach
2. use #if defined(__APPLE__) ... #endif
You can check how the __APPLE__ macro is used in the Kudu's code for the examples of the latter.
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
First of all, why to leak those at all? I.e. why not to release those on teardown?
http://gerrit.cloudera.org:8080/#/c/4789/4/src/kudu/util/net/ssl_socket.cc
File src/kudu/util/net/ssl_socket.cc:
Line 111: std::string common_name = GetX509CommonName(cert.get());
> The RFCs I've been reading seem to indicate we should not just be checking
Yep, please check X509_VERIFY_PARAM_set1_host instead:
https://www.openssl.org/docs/man1.0.2/crypto/X509_VERIFY_PARAM_set1_host.html
or at least X509_check_host (but it's best to use X509_VERIFY_PARAM_xxx instead)
https://www.openssl.org/docs/man1.0.2/crypto/X509_check_host.html
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 in the subject alternative name; combinations of those, etc.
Why not to use standard OpenSSL methods?
https://www.openssl.org/docs/man1.0.2/crypto/X509_VERIFY_PARAM_set1_host.html
The underlying methods, like X509_check_host(), etc. already implemented to perform those checks in accordance with RFC:
https://www.openssl.org/docs/man1.0.2/crypto/X509_check_host.html
--
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 <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KuduRPC integration with OpenSSL
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4789/7/LICENSE.txt
File LICENSE.txt:
Line 533: ================================================================================
Jim Wright on legal-discuss@ pointed out that OpenSSL actually has two licenses applied to it (see https://www.openssl.org/source/license.html which has the 'Original SSLeay license' text). Is there a reason you only inserted the first one or was that an oversight?
--
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: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4789
to look at the new patch set (#7).
Change subject: KuduRPC integration with OpenSSL
......................................................................
KuduRPC integration with OpenSSL
This patch adds SSL support for the RPC layer in Kudu. It uses the
OpenSSL library for this purpose.
This is acheived by subclassing 'Socket' to add 'SSLSocket' which
calls into the OpenSSL library for auth/read/write/shutdown.
SSL is enabled only at a 'Connection' object level, i.e. the
AcceptorPool still only works with regular 'Socket's, and the reactor
threads also use a regular 'Socket' on a new outgoing call.
The first point of any SSL activity happens in the context of the
negotiation pool, where the SSL handshake happens before the SASL
handshake. On a successful handshake, further communication is
encrypted on that 'Connection'.
A 'SSLFactory' is created at the 'Messenger' level. This factory is
in-charge of creating 'SSLSocket' objects when necessary and also
keeps a track of the certificates, keys and the SSL context that is
shared among all the sockets of that 'Messenger'.
The x509_check_host() function is ported from OpenSSL-1.1.0b which we
use when the version of OpenSSL detected is less that 1.0.2. It is
added in util/x509_check_host.cc/h
Added a parameter to the rpc-test to additionally run the tests with
SSL enabled.
Future TODOs:
- Make SSL methods (SSLv23, TLS1, etc.) configurable and OpenSSL
version aware. (Choosing APIs based on supported versions)
- Allow loading keys as strings vs files. (Need to use different APIs)
Change-Id: I27167faa4e6a78e59b46093055b16682c93af0ea
---
M CMakeLists.txt
M LICENSE.txt
M NOTICE.txt
M build-support/lint.sh
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_client.cc
M src/kudu/rpc/sasl_client.h
M src/kudu/rpc/sasl_rpc-test.cc
M src/kudu/rpc/sasl_server.cc
M src/kudu/rpc/sasl_server.h
M src/kudu/util/CMakeLists.txt
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
A src/kudu/util/net/ssl_factory.cc
A src/kudu/util/net/ssl_factory.h
A src/kudu/util/net/ssl_socket.cc
A src/kudu/util/net/ssl_socket.h
A src/kudu/util/x509_check_host.cc
A src/kudu/util/x509_check_host.h
27 files changed, 1,301 insertions(+), 120 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/4789/7
--
To view, visit http://gerrit.cloudera.org:8080/4789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I27167faa4e6a78e59b46093055b16682c93af0ea
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: KuduRPC integration with OpenSSL
......................................................................
Patch Set 7: Code-Review+1
(8 comments)
LGTM, just a couple of nits and a questions regarding TLS version: I guess disabling everything below TLS v1.2 is the way to go.
http://gerrit.cloudera.org:8080/#/c/4789/7//COMMIT_MSG
Commit Message:
PS7, Line 12: acheived
nit: typo; acheived --> achieved
PS7, Line 30: t
nit: typo; that --> than
PS7, Line 37: - Make SSL methods (SSLv23, TLS1, etc.) configurable and OpenSSL
: version aware. (Choosing APIs based on supported versions)
I'm not aware of compatibility requirements, but probably it would be enough to enable only TLS v1.2, disabling all prior versions. TLS v1.2 was published 8 years ago.
http://gerrit.cloudera.org:8080/#/c/4789/7/LICENSE.txt
File LICENSE.txt:
PS7, Line 489:
nit: an extra space at the EOL.
http://gerrit.cloudera.org:8080/#/c/4789/7/src/kudu/util/net/ssl_factory.cc
File src/kudu/util/net/ssl_factory.cc:
PS7, Line 76: #if OPENSSL_VERSION_NUMBER >= 0x10100000L
: ctx_.reset(SSL_CTX_new(TLS_method()));
: #else
: ctx_.reset(SSL_CTX_new(SSLv23_method()));
I might miss something, but TLSv1_2_method() is available starting OpenSSL v1.0.1. If I'm not mistaken, the oldest version we are about to deal with is OpenSSL v1.0.1e (CentOS/RH 6.6). That said, consider using TLSv1_2_method() and dropping the if/else.
The spec for TLS v1.2 was released in 2008 :)
PS7, Line 85: SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3
Consider adding SSL_OP_NO_TLSv1 and SSL_OP_NO_TLSv1_1, if we are about to use TLSv1_2 anyway both from the client and the server side.
http://gerrit.cloudera.org:8080/#/c/4789/7/src/kudu/util/net/ssl_factory.h
File src/kudu/util/net/ssl_factory.h:
PS7, Line 19: include <openssl/err.h>
: #include <openssl/ssl.h>
: #include <sys/uio.h>
nit: consider removing these and providing forward declarations for SSL_CTX
http://gerrit.cloudera.org:8080/#/c/4789/7/src/kudu/util/x509_check_host.h
File src/kudu/util/x509_check_host.h:
Probably, we will also need X509_check_ip and X509_check_ip_ascii, but those can be added separately.
--
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: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: KuduRPC integration with OpenSSL
......................................................................
Patch Set 4:
(8 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 SSLSocket type?
Consider using dynamic_cast instead and verify that dynamic_cast returns non-null pointer. If it returns null, return an error.
http://gerrit.cloudera.org:8080/#/c/4789/4/src/kudu/util/net/ssl_factory.cc
File src/kudu/util/net/ssl_factory.cc:
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*)>>, here it would be possible to write just
ctx_(nullptr, SSL_CTX_free)
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
#pragma once
instead
PS4, Line 37: function
nit: function --> method
PS4, Line 39: OpenSSLInit
Is this forward declaration?
PS4, Line 51: void Teardown();
As far as I know, the OpenSSL library contains some statics which cannot be unset during tear-down, making it non-reentrant. Given that, do you think it makes sense to make this method public?
Also, I could not find the implementation of this method in the corresponding .cc file.
PS4, Line 57:
nit: an extra space
PS4, Line 64: SSLSocket*
Consider returning unique_ptr<SSLSocket> instead: the signature with smart pointer for return type makes users of the API less leak-prone. Besides, it explicitly expresses memory ownership for the returned pointers, so no convention/comments are needed.
--
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 <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: KuduRPC integration with OpenSSL
......................................................................
Patch Set 1:
(18 comments)
http://gerrit.cloudera.org:8080/#/c/4789/1/src/kudu/rpc/rpc-test-base.h
File src/kudu/rpc/rpc-test-base.h:
Line 347: // TODO: Figure out a way to properly populate this.
> warning: missing username/bug in TODO [google-readability-todo]
Done
http://gerrit.cloudera.org:8080/#/c/4789/1/src/kudu/util/net/ssl_factory.cc
File src/kudu/util/net/ssl_factory.cc:
Line 32: static void CryptoLockingCallback(int mode, int n, const char*, int) {
> warning: all parameters should be named in a function [readability-named-pa
Done
Line 41: static unsigned long CryptoThreadIDCallback() {
> warning: consider replacing 'unsigned long' with 'uint64' [google-runtime-i
Done
Line 67: // TODO: Make method configurable.
> warning: missing username/bug in TODO [google-readability-todo]
Done
Line 120: }
> warning: namespace 'kudu' not terminated with a closing comment [google-rea
Done
http://gerrit.cloudera.org:8080/#/c/4789/1/src/kudu/util/net/ssl_factory.h
File src/kudu/util/net/ssl_factory.h:
Line 20: #include <sys/uio.h>
> warning: #includes are not sorted properly [llvm-include-order]
Done
Line 46: Status LoadCertificate(std::string& certificate_path);
> warning: non-const reference parameter 'certificate_path', make it const or
Done
Line 49: Status LoadPrivateKey(std::string& key_path);
> warning: non-const reference parameter 'key_path', make it const or use a p
Done
Line 52: Status LoadCertificateAuthority(std::string& certificate_path);
> warning: non-const reference parameter 'certificate_path', make it const or
Done
Line 75: } else {
> warning: don't use else after return [readability-else-after-return]
Done
Line 83: }
> warning: namespace 'kudu' not terminated with a closing comment [google-rea
Done
http://gerrit.cloudera.org:8080/#/c/4789/1/src/kudu/util/net/ssl_socket.cc
File src/kudu/util/net/ssl_socket.cc:
Line 22: #include <openssl/err.h>
> warning: #includes are not sorted properly [llvm-include-order]
Done
Line 55: std::string common_name((char*)utf8_cn);
> warning: C-style casts are discouraged; use reinterpret_cast [google-readab
Done
Line 63: bool VerifyHost(std::string& hostname, std::string& subject_name, int size) {
> warning: non-const reference parameter 'subject_name', make it const or use
Done
Line 63: bool VerifyHost(std::string& hostname, std::string& subject_name, int size) {
> warning: non-const reference parameter 'hostname', make it const or use a p
Done
Line 65: size_t leftmost_size = subject_name.find_first_of(".");
> warning: 'find_first_of' called with a string literal consisting of a singl
Done
Line 68: size_t wildcard_pos = subject_name.find_first_of("*", leftmost_size);
> warning: 'find_first_of' called with a string literal consisting of a singl
Done
Line 210: }
> warning: namespace 'kudu' not terminated with a closing comment [google-rea
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: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KuduRPC integration with OpenSSL
......................................................................
Patch Set 8: Code-Review+2
--
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: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: KuduRPC integration with OpenSSL
......................................................................
Patch Set 5:
(2 comments)
Should I convert all the C-style casts in the ported code to C++ style casts?
http://gerrit.cloudera.org:8080/#/c/4789/5/src/kudu/util/net/ssl_factory.h
File src/kudu/util/net/ssl_factory.h:
Line 70: } else {
> warning: don't use else after return [readability-else-after-return]
Done
http://gerrit.cloudera.org:8080/#/c/4789/5/src/kudu/util/net/ssl_socket.cc
File src/kudu/util/net/ssl_socket.cc:
Line 87: } else if (match < 0) {
> warning: don't use else after return [readability-else-after-return]
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: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4789
to look at the new patch set (#5).
Change subject: KuduRPC integration with OpenSSL
......................................................................
KuduRPC integration with OpenSSL
This patch adds SSL support for the RPC layer in Kudu. It uses the
OpenSSL library for this purpose.
This is acheived by subclassing 'Socket' to add 'SSLSocket' which
calls into the OpenSSL library for auth/read/write/shutdown.
SSL is enabled only at a 'Connection' object level, i.e. the
AcceptorPool still only works with regular 'Socket's, and the reactor
threads also use a regular 'Socket' on a new outgoing call.
The first point of any SSL activity happens in the context of the
negotiation pool, where the SSL handshake happens before the SASL
handshake. On a successful handshake, further communication is
encrypted on that 'Connection'.
A 'SSLFactory' is created at the 'Messenger' level. This factory is
in-charge of creating 'SSLSocket' objects when necessary and also
keeps a track of the certificates, keys and the SSL context that is
shared among all the sockets of that 'Messenger'.
Added a parameter to the rpc-test to additionally run the tests with
SSL enabled.
Future TODOs:
- Make SSL methods (SSLv23, TLS1, etc.) configurable and OpenSSL
version aware. (Choosing APIs based on supported versions)
- Allow loading keys as strings vs files. (Need to use different APIs)
Change-Id: I27167faa4e6a78e59b46093055b16682c93af0ea
---
M CMakeLists.txt
M LICENSE.txt
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_client.cc
M src/kudu/rpc/sasl_client.h
M src/kudu/rpc/sasl_rpc-test.cc
M src/kudu/rpc/sasl_server.cc
M src/kudu/rpc/sasl_server.h
M src/kudu/util/CMakeLists.txt
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
A src/kudu/util/net/ssl_factory.cc
A src/kudu/util/net/ssl_factory.h
A src/kudu/util/net/ssl_socket.cc
A src/kudu/util/net/ssl_socket.h
A src/kudu/util/x509_check_host.cc
A src/kudu/util/x509_check_host.h
25 files changed, 1,296 insertions(+), 118 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/4789/5
--
To view, visit http://gerrit.cloudera.org:8080/4789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I27167faa4e6a78e59b46093055b16682c93af0ea
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4789
to look at the new patch set (#4).
Change subject: KuduRPC integration with OpenSSL
......................................................................
KuduRPC integration with OpenSSL
This patch adds SSL support for the RPC layer in Kudu. It uses the
OpenSSL library for this purpose.
This is acheived by subclassing 'Socket' to add 'SSLSocket' which
calls into the OpenSSL library for auth/read/write/shutdown.
SSL is enabled only at a 'Connection' object level, i.e. the
AcceptorPool still only works with regular 'Socket's, and the reactor
threads also use a regular 'Socket' on a new outgoing call.
The first point of any SSL activity happens in the context of the
negotiation pool, where the SSL handshake happens before the SASL
handshake. On a successful handshake, further communication is
encrypted on that 'Connection'.
A 'SSLFactory' is created at the 'Messenger' level. This factory is
in-charge of creating 'SSLSocket' objects when necessary and also
keeps a track of the certificates, keys and the SSL context that is
shared among all the sockets of that 'Messenger'.
Added a parameter to the rpc-test to additionally run the tests with
SSL enabled.
Future TODOs:
- Enable authorization of SANs (SubjectAlternativeNames)
- Make SSL methods (SSLv23, TLS1, etc.) configurable and OpenSSL
version aware. (Choosing APIs based on supported versions)
- Allow loading keys as strings vs files. (Need to use different APIs)
Change-Id: I27167faa4e6a78e59b46093055b16682c93af0ea
---
M CMakeLists.txt
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_client.cc
M src/kudu/rpc/sasl_client.h
M src/kudu/rpc/sasl_rpc-test.cc
M src/kudu/rpc/sasl_server.cc
M src/kudu/rpc/sasl_server.h
M src/kudu/util/CMakeLists.txt
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
A src/kudu/util/net/ssl_factory.cc
A src/kudu/util/net/ssl_factory.h
A src/kudu/util/net/ssl_socket.cc
A src/kudu/util/net/ssl_socket.h
22 files changed, 813 insertions(+), 118 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/4789/4
--
To view, visit http://gerrit.cloudera.org:8080/4789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I27167faa4e6a78e59b46093055b16682c93af0ea
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
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 <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
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
: (eay@cryptsoft.com). This product includes software written by Tim
: Hudson (tjh@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 <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KuduRPC integration with OpenSSL
......................................................................
Patch Set 2:
(44 comments)
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 a matter of one call: SSL_CTX_set_options(ctx->ssl_ctx, SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3);
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 the point just before you create the connection, and then pass in the constructed Socket to the Connection ctor? seems like looser coupling (you don't need to call back into the messenger to find if SSL is enabled), and then you don't need this two-step initialization of the sasl server/client
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 for maintaining them until we get a little farther into actually operating this, etc, and I think the settings may end up needing to be passed in from elsewhere. For example, the client CA definitely needs to be exposed as an API, not a gflag, since gflags aren't exposed to clients, and because different clients in the same process may need to choose different CAs. On the server side, we expect that we'll be using a cert generated on the fly, in which case we also wouldn't want these as gflags.
Also, the descriptions and names should more specifically reference RPC. Otherwise I think most users would read these and think it's talking about HTTPS
Line 167: if (ssl_enabled()) {
how about checking if (ssl_factory_) since even if ssl is enabled, maybe it didn't actually get constructed
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 be called 'TLS' rather than 'SSL' since we'll probably disable SSLv2 and SSLv3?
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, would that avoid having to propagate the flag in all the superclass method calls? or is that more of a mess because of other tests depending on it?
PS2, Line 55: parametrized
nit: parameterized
PS2, Line 56: ,Te
nit: missing space
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
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
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
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
agreed. Otherwise you'll need to at least add this to the RAT exclude list, and to the list of files to always ship to slaves in the build-support/dist_test.py script
http://gerrit.cloudera.org:8080/#/c/4789/2/src/kudu/util/net/ssl_factory.cc
File src/kudu/util/net/ssl_factory.cc:
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 library? It also sets the locking and id callbacks. I'm afraid it's likely that both initializations will run, and there will be some mutexes locked using one set of callbacks, and then attempt to unlock (a different mutex) using the other set. Or, switching thread IDs, etc.
Do we need some option to tell squeasel "SSL is already initialized, don't do your own init"?
Line 66: Status SSLFactory::Init() {
CHECK(!ctx_) here to make sure we don't double-init and leak
Line 67: // TODO(sailesh): Make method configurable.
not sure it needs to be configurable. Perhaps we should be doing something like checking:
#if OPENSSL_VERSION_NUMBER >= 0x10100000L
SSL_CTX_new(TLS_method())
#else
SSL_CTX_new(SSLv23_method())
#endif
since it seems like SSLv23_method has been depreacted in 1.1.0
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 need some flexibility here to set it on a per-server basis.
PS2, Line 116: if (!ssl_socket)
this condition is unnecessary since 'new' will always return non-NULL
http://gerrit.cloudera.org:8080/#/c/4789/2/src/kudu/util/net/ssl_factory.h
File src/kudu/util/net/ssl_factory.h:
Line 34: // Setup the OpenSSL library.
specify when this must be called and how many times.
Why not keep this internal and call it from the first SSLFactory ctor, considering it doesn't return Status?
Line 38: public:
please add a ctor and dtor (and define them in the .cc file) even if they're empty.
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 not just do this work in the dtor?
PS2, Line 56: bool is_server);
we usually prefer enums to bools, eg:
enum Direction {
CLIENT, SERVER
};
just to make call sites more obvious
PS2, Line 62: SSL error queue
please document that this is a thread-local concept.
Also, note that this doesn't actually modify the error queue since it's using peek. Is that the behavior we want?
http://stackoverflow.com/questions/18179128/how-to-manage-the-error-queue-in-openssl-ssl-get-error-and-err-get-error also seems to suggest that if you ever plan to call SSLGetError, you should probably be using ERR_clear_error before every call.
PS2, Line 63: ssp
typo
Line 64: static std::string GetLastError(int errno_copy) {
seems better to define this in the .cc file in an anonymous namespace, since it's private anyway
Line 67: if (error_code != 0) {
I think this would be clearer to invert this condition so the shorter thing is indented:
if (error_code == 0) return ErrnoToString(errno_copy)
PS2, Line 72: "SSL error " + error_code;
that's not what you want. I think you mean Substitute("SSL error $0", error_code).
(otherwise this is incrementing the char* pointer)
http://gerrit.cloudera.org:8080/#/c/4789/2/src/kudu/util/net/ssl_socket.cc
File src/kudu/util/net/ssl_socket.cc:
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_
Line 37: std::string GetX509CommonName(X509* cert, int* cn_size) {
this function and a few below are only used within this compilation unit, so they should be put in an anonymous namespace
PS2, Line 38: std::string()
here and below, you can just return "";
Line 53: *cn_size = ASN1_STRING_to_UTF8(&utf8_cn, cn_asn1);
The example on https://wiki.openssl.org/index.php/Hostname_validation shows them checking that there are no embedded \0s in the resulting string. That's probably a good idea.
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 you don't need the out-param of GetX509CommonName?
Line 63: // Return 'true' if successful and 'false' otherwise.
I think it's worth adding a comment here that references https://wiki.openssl.org/index.php/Hostname_validation
I was surprised when reading the code that you had to implement this yourself, but seems necessary since we need to support openssl 1.0.1 on el6
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 a few edge cases
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:
vector<StringPiece> components = strings::Split(subject_name, '*');
for (int i = 1; i < components.size(); i++) {
if (components[i].find('*') != npos) ...
}
(I always find substr stuff hard to reason about and prone to off-by-one errors)
Line 73: int i = 0, j = 0;
why not define these inside the for loop?
PS2, Line 76: while (j < hostname_size && hostname[j] != '.') {
: ++j;
: }
is this greedy match always right? the RFC 6125 seems to indicate that you should be able to match things like "*baz.example.com" and I'm not sure this algorithm works.
Maybe using MatchPattern would be better?
Line 125: peer_addr.LookupHostname(&peer_hostname);
need to check result (eg if reverse DNS fails)
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. (same below in a few places)
Line 155: for (int i = 0; i < iov_len; ++i) {
We typically set O_NODELAY on our sockets, so each of the underlying Write calls is going to cause a separate TCP packet.
I think we should do something whereby we set TCP_CORK before the loop, and unset it at the end, so that we still end up consolidating packets for vectored writes.
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 through to Write even?
Line 196: if (ret == 0) ret = SSL_shutdown(ssl_);
this doesn't seem right, since this seems like it's a blocking call. Given we plan to shut down the socket anyway, isn't a unidirectional one enough?
PS2, Line 207: Socket::Close()
this is calling it again which seems wrong
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 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
PS2, Line 49: Inherits
better to say 'Owned'. Inherits sounds like it's something coming from a superclass
--
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 <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
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 <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: KuduRPC integration with OpenSSL
......................................................................
Patch Set 4:
(1 comment)
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 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
Sailesh pointed to the fact that those X509_VERIFY_PARAM_set1_xxx() methods are not available on CentOS 6.6 since it uses OpenSSL version 1.0.1<something>, while the method mentioned appeared in OpenSSL starting version 1.0.2.
What do you guys think about bringing in that X509_VERIFY_PARAM and related code from OpenSSL v1.0.2 into the Kudu sources and compiling/linking them in into libkudu_security when OPENSSL_VERSION_NUMBER shows it's 1.0.1 or an earlier version?
--
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 <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: KuduRPC integration with OpenSSL
......................................................................
Patch Set 1:
(2 comments)
This patch is a simple basic version of KuduRPC over SSL. There is some room for future work, and I think they should be done as separate patches once the general structure is decided upon.
There are a couple of obvious flaws in this patch, and it would be great if I could get some help with those. I've added them as comments.
http://gerrit.cloudera.org:8080/#/c/4789/1/src/kudu/rpc/rpc-test-base.h
File src/kudu/rpc/rpc-test-base.h:
Line 347: // TODO: Figure out a way to properly populate this.
This is obviously wrong.
Since there are no environment variables where I can source the path from, I'm not sure how to point the tests to the correct relative path.
The auto gerrit tests will fail because of this.
http://gerrit.cloudera.org:8080/#/c/4789/1/src/kudu/util/net/ssl_socket.cc
File src/kudu/util/net/ssl_socket.cc:
Line 149: Status SSLSocket::Writev(const struct ::iovec *iov, int iov_len,
Since there's no real writev() type support for SSL, this function breaks up the frames put together by SendBuffer() and sends them individually. So the buffering and un-buffering seem like a waste here.
I'm not sure what a good solution is unless we subclass OutboundTransfer for the sake of SSL too, which seems a bit messy.
--
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: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Todd Lipcon, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4789
to look at the new patch set (#8).
Change subject: KuduRPC integration with OpenSSL
......................................................................
KuduRPC integration with OpenSSL
This patch adds SSL support for the RPC layer in Kudu. It uses the
OpenSSL library for this purpose.
This is achieved by subclassing 'Socket' to add 'SSLSocket' which
calls into the OpenSSL library for auth/read/write/shutdown.
SSL is enabled only at a 'Connection' object level, i.e. the
AcceptorPool still only works with regular 'Socket's, and the reactor
threads also use a regular 'Socket' on a new outgoing call.
The first point of any SSL activity happens in the context of the
negotiation pool, where the SSL handshake happens before the SASL
handshake. On a successful handshake, further communication is
encrypted on that 'Connection'.
A 'SSLFactory' is created at the 'Messenger' level. This factory is
in-charge of creating 'SSLSocket' objects when necessary and also
keeps a track of the certificates, keys and the SSL context that is
shared among all the sockets of that 'Messenger'.
The x509_check_host() function is ported from OpenSSL-1.1.0b which we
use when the version of OpenSSL detected is less than 1.0.2. It is
added in util/x509_check_host.cc/h
Added a parameter to the rpc-test to additionally run the tests with
SSL enabled.
Future TODOs:
- Allow loading keys as strings vs files. (Need to use different APIs)
- Consider porting x509_check_ip and x509_check_ip_ascii.
Change-Id: I27167faa4e6a78e59b46093055b16682c93af0ea
---
M CMakeLists.txt
M LICENSE.txt
M NOTICE.txt
M build-support/lint.sh
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_client.cc
M src/kudu/rpc/sasl_client.h
M src/kudu/rpc/sasl_rpc-test.cc
M src/kudu/rpc/sasl_server.cc
M src/kudu/rpc/sasl_server.h
M src/kudu/util/CMakeLists.txt
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
A src/kudu/util/net/ssl_factory.cc
A src/kudu/util/net/ssl_factory.h
A src/kudu/util/net/ssl_socket.cc
A src/kudu/util/net/ssl_socket.h
A src/kudu/util/x509_check_host.cc
A src/kudu/util/x509_check_host.h
27 files changed, 1,373 insertions(+), 122 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/4789/8
--
To view, visit http://gerrit.cloudera.org:8080/4789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I27167faa4e6a78e59b46093055b16682c93af0ea
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: KuduRPC integration with OpenSSL
......................................................................
Patch Set 8: Code-Review+1
(11 comments)
Carry +1.
http://gerrit.cloudera.org:8080/#/c/4789/7//COMMIT_MSG
Commit Message:
PS7, Line 12: achieved
> nit: typo; acheived --> achieved
Done
PS7, Line 30: n
> nit: typo; that --> than
Done
PS7, Line 37: - Allow loading keys as strings vs files. (Need to use different APIs)
: - Consider porting x509_check_ip and x509_check_ip_ascii.
> I'm not aware of compatibility requirements, but probably it would be enoug
Done
http://gerrit.cloudera.org:8080/#/c/4789/7/LICENSE.txt
File LICENSE.txt:
PS7, Line 489:
> nit: an extra space at the EOL.
Done
Line 533: STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> Jim Wright on legal-discuss@ pointed out that OpenSSL actually has two lice
My bad, I thought SSLeay was deprecated and so the license for that wasn't required. I added it now.
http://gerrit.cloudera.org:8080/#/c/4789/7/src/kudu/util/net/ssl_factory.cc
File src/kudu/util/net/ssl_factory.cc:
PS7, Line 76: Status SSLFactory::Init() {
: CHECK(!ctx_.get());
: ctx_.reset(SSL_CTX_new(TLSv1_2_method()));
: if (ctx_ == nullptr) {
> I might miss something, but TLSv1_2_method() is available starting OpenSSL
Yes, that makes sense. I've changed it.
PS7, Line 85:
> Consider adding SSL_OP_NO_TLSv1 and SSL_OP_NO_TLSv1_1, if we are about to u
Done
http://gerrit.cloudera.org:8080/#/c/4789/7/src/kudu/util/net/ssl_factory.h
File src/kudu/util/net/ssl_factory.h:
PS7, Line 19: include <string>
: #include <memory>
:
> nit: consider removing these and providing forward declarations for SSL_CTX
Done. Did the same for 'SSL' in ssl_socket.h
http://gerrit.cloudera.org:8080/#/c/4789/7/src/kudu/util/net/ssl_socket.cc
File src/kudu/util/net/ssl_socket.cc:
PS7, Line 77:
> retrieve
Done
PS7, Line 86:
> nullptr
Done
http://gerrit.cloudera.org:8080/#/c/4789/7/src/kudu/util/x509_check_host.h
File src/kudu/util/x509_check_host.h:
> Probably, we will also need X509_check_ip and X509_check_ip_ascii, but thos
Agreed. We can do that as a follow on patch. Added it as a future TODO in the commit message.
--
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: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: KuduRPC integration with OpenSSL
......................................................................
KuduRPC integration with OpenSSL
This patch adds SSL support for the RPC layer in Kudu. It uses the
OpenSSL library for this purpose.
This is achieved by subclassing 'Socket' to add 'SSLSocket' which
calls into the OpenSSL library for auth/read/write/shutdown.
SSL is enabled only at a 'Connection' object level, i.e. the
AcceptorPool still only works with regular 'Socket's, and the reactor
threads also use a regular 'Socket' on a new outgoing call.
The first point of any SSL activity happens in the context of the
negotiation pool, where the SSL handshake happens before the SASL
handshake. On a successful handshake, further communication is
encrypted on that 'Connection'.
A 'SSLFactory' is created at the 'Messenger' level. This factory is
in-charge of creating 'SSLSocket' objects when necessary and also
keeps a track of the certificates, keys and the SSL context that is
shared among all the sockets of that 'Messenger'.
The x509_check_host() function is ported from OpenSSL-1.1.0b which we
use when the version of OpenSSL detected is less than 1.0.2. It is
added in util/x509_check_host.cc/h
Added a parameter to the rpc-test to additionally run the tests with
SSL enabled.
Future TODOs:
- Allow loading keys as strings vs files. (Need to use different APIs)
- Consider porting x509_check_ip and x509_check_ip_ascii.
Change-Id: I27167faa4e6a78e59b46093055b16682c93af0ea
Reviewed-on: http://gerrit.cloudera.org:8080/4789
Reviewed-by: Sailesh Mukil <sa...@cloudera.com>
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M CMakeLists.txt
M LICENSE.txt
M NOTICE.txt
M build-support/lint.sh
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/sasl_client.cc
M src/kudu/rpc/sasl_client.h
M src/kudu/rpc/sasl_rpc-test.cc
M src/kudu/rpc/sasl_server.cc
M src/kudu/rpc/sasl_server.h
M src/kudu/util/CMakeLists.txt
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
A src/kudu/util/net/ssl_factory.cc
A src/kudu/util/net/ssl_factory.h
A src/kudu/util/net/ssl_socket.cc
A src/kudu/util/net/ssl_socket.h
A src/kudu/util/x509_check_host.cc
A src/kudu/util/x509_check_host.h
27 files changed, 1,373 insertions(+), 122 deletions(-)
Approvals:
Sailesh Mukil: Looks good to me, but someone else must approve
Todd Lipcon: Looks good to me, approved
Alexey Serbin: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/4789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I27167faa4e6a78e59b46093055b16682c93af0ea
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: KuduRPC integration with OpenSSL
......................................................................
Patch Set 2:
(17 comments)
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 unit test directory.
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 corresponding test class/file and writing it into the unit test directory during run-time.
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 them dynamically, i.e. why storing pointers instead of objects themselves?
PS2, Line 42: return pthread_self();
This is not portable: pthread_t can be any type (like a structure). Instead, consider using something like:
std::hash<std::thread::id> hasher;
return hasher(std::this_thread::get_id());
PS2, Line 52: new Mutex
Where is the corresponding delete part?
PS2, Line 57: CRYPTO_set_id_callback
From 1.0.0, CRYPTO_set_id_callback() is replaced/deprecated by CRYPTO_THREADID_set_callback.
PS2, Line 60: GoogleOnceType
Consider using std::flag_once from the standard library.
PS2, Line 63: GoogleOnceInit
Consider using std::call_once() from the standard library.
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://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
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:
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
PS2, Line 54: if (*cn_size < 0) return std::string()
Is it necessary to release memory allocated by cn_asn1 if returning from here?
PS2, Line 105: X509* cert
Consider using unique_ptr with custom deleter to avoid memory leaks in case of returning from this method prior to reaching line 131
PS2, Line 116: return
Is it necessary to release cert before returning?
PS2, Line 122: return
Is it necessary to release cert before returning?
PS2, Line 129: return
Is it necessary to release cert before returning?
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://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
PS2, Line 34: explicit
Is it really needed here?
--
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 <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KuduRPC integration with OpenSSL
......................................................................
Patch Set 7: Code-Review+1
(2 comments)
LGTM, just a few small nits.
http://gerrit.cloudera.org:8080/#/c/4789/7/src/kudu/util/net/ssl_socket.cc
File src/kudu/util/net/ssl_socket.cc:
PS7, Line 77: retreive
retrieve
PS7, Line 86: NULL
nullptr
--
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: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KuduRPC integration with OpenSSL
......................................................................
Patch Set 1:
(13 comments)
This is great. Did a quick first pass mostly looking at style, I'll take a harder look at the openssl usage soon.
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.
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?
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.
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:
Socket* socket()
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:
TAG_FLAG(ssl_server_certificate, experimental);
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
PS1, Line 189:
whitespace
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 are at it.
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
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.
PS1, Line 69:
whitespace
http://gerrit.cloudera.org:8080/#/c/4789/1/src/kudu/util/net/ssl_factory.cc
File src/kudu/util/net/ssl_factory.cc:
Line 42: return pthread_self();
This likely won't work on OS X. Use UniqueThreadId() in thread.h instead.
PS1, Line 69: NULL
nullptr here and elsewhere.
--
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: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KuduRPC integration with OpenSSL
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KuduRPC integration with OpenSSL
......................................................................
Patch Set 2:
As a general note, I'm pretty sure this is going to bump the minimum version requirement on Centos 6 to something like 6.6. We should update the installation requirement docs accordingly.
--
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 <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No