impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson (Code Review)" <>
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:

File be/src/rpc/

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.
File be/src/rpc/thrift-server.h:

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

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
File be/src/service/

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
To unsubscribe, visit

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

View raw message