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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
