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 <[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
