impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5743: Support TLS version configuration for Thrift servers
Date Thu, 10 Aug 2017 19:38:19 GMT
Henry Robinson has posted comments on this change.

Change subject: IMPALA-5743: Support TLS version configuration for Thrift servers
......................................................................


Patch Set 1:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/7606/1/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:

PS1, Line 257: TEST(SslTest, StringToProtocol) {
> Please add a brief description explaining what this test does, especially b
Not yet, but I'll run tests on CentOS 6 once the toolchain changes have been published.


PS1, Line 341: // AES256 is v1.2+ only.
> Do we know if thrift bubbles up sensible errors for cipher-SSL version inco
Do you mean if the cipher is incompatible with the TLS version requested? In that case yes
- it says that no matching cipher is found, which is not perfect but a good start.


http://gerrit.cloudera.org:8080/#/c/7606/1/be/src/rpc/thrift-server.h
File be/src/rpc/thrift-server.h:

PS1, Line 31: #include "rpc/auth-provider.h"
> We can change this to a forward declare right?
Done


PS1, Line 32: #include "util/metrics.h"
> Same as above
This one is a bit harder because of the templated definitions that are trickier to forward-declare.


Line 165:   /// is used only for password-protected .PEM files.
> Should have caught this in the other review, but please add a comment for t
Done


http://gerrit.cloudera.org:8080/#/c/7606/1/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

PS1, Line 181: Supported versions are "
             : #if OPENSSL_VERSION_NUMBER >= 0x1000100L
             :     "TLSv1.0, TLSv1.1 and TLSv1.2");
             : #else
             :     "TLSv1.0");
             : #endif
> Should we also mention what the strings representing these different versio
These are the strings that represent those versions :)


-- 
To view, visit http://gerrit.cloudera.org:8080/7606
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4c68a6c9658ddbfbe8025f2021fd5ed7a9dec5a5
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message