You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Henry Robinson (Code Review)" <ge...@cloudera.org> on 2017/07/27 22:51:48 UTC
[Impala-ASF-CR] IMPALA-5696: Enable cipher configuration when using TLS / Thrift
Henry Robinson has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7524
Change subject: IMPALA-5696: Enable cipher configuration when using TLS / Thrift
......................................................................
IMPALA-5696: Enable cipher configuration when using TLS / Thrift
The 'cipher suite' is a description of the set of algorithms used by SSL
and TLS to execute key exchange, encryption, message authentication, and
random number generation functions. SSL implementations allows the
cipher suite to be configured so that ciphers may be removed from the
whitelist if they are shown to be weak.
* Add a flag --ssl_cipher_list which controls cipher selection for both
thrift servers and clients. Default is blank, which means use all
available cipher suites.
* Add ThriftServerBuilder to simplify construction of
ThriftServers (whose constructors were otherwise getting very long).
Testing: new tests added to thrift-server-test. Test cases added follow:
* A client cannot connect to a server which does not have any ciphers in
common with it.
* If ciphers are identical on clients and servers, that ssl connections
can be made.
* Bad cipher strings lead to errors on both client and server.
Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
---
M be/src/benchmarks/network-perf-benchmark.cc
M be/src/catalog/catalogd-main.cc
M be/src/rpc/thrift-client.cc
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/runtime/data-stream-test.cc
M be/src/service/impala-server.cc
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestored-main.cc
M be/src/testutil/in-process-servers.cc
A be/src/testutil/scoped-flag-setter.h
M be/src/util/webserver-test.cc
13 files changed, 399 insertions(+), 144 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/24/7524/1
--
To view, visit http://gerrit.cloudera.org:8080/7524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
[Impala-ASF-CR] IMPALA-5696: Enable cipher configuration when using TLS / Thrift
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5696: Enable cipher configuration when using TLS / Thrift
......................................................................
Patch Set 4: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/995/
--
To view, visit http://gerrit.cloudera.org:8080/7524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5696: Enable cipher configuration when using TLS / Thrift
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5696: Enable cipher configuration when using TLS / Thrift
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1000/
--
To view, visit http://gerrit.cloudera.org:8080/7524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5696: Enable cipher configuration when using TLS / Thrift
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins, Matthew Jacobs, Sailesh Mukil,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7524
to look at the new patch set (#6).
Change subject: IMPALA-5696: Enable cipher configuration when using TLS / Thrift
......................................................................
IMPALA-5696: Enable cipher configuration when using TLS / Thrift
The 'cipher suite' is a description of the set of algorithms used by SSL
and TLS to execute key exchange, encryption, message authentication, and
random number generation functions. SSL implementations allow the cipher
suite to be configured so that ciphers may be removed from the whitelist
if they are shown to be weak.
* Add a flag --ssl_cipher_list which controls cipher selection for both
thrift servers and clients. Default is blank, which means use all
available cipher suites.
* Add ThriftServerBuilder to simplify construction of
ThriftServers (whose constructors were otherwise getting very long).
Testing: new tests added to thrift-server-test. Test cases added follow:
* A client cannot connect to a server which does not have any ciphers in
common with it.
* If ciphers are identical on clients and servers, that ssl connections
can be made.
* Bad cipher strings lead to errors on both client and server.
Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
---
M be/src/benchmarks/network-perf-benchmark.cc
M be/src/catalog/catalogd-main.cc
M be/src/rpc/thrift-client.cc
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/runtime/data-stream-test.cc
M be/src/service/impala-server.cc
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestored-main.cc
M be/src/testutil/in-process-servers.cc
A be/src/testutil/scoped-flag-setter.h
M be/src/util/webserver-test.cc
13 files changed, 440 insertions(+), 143 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/24/7524/6
--
To view, visit http://gerrit.cloudera.org:8080/7524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-5696: Enable cipher configuration when using TLS / Thrift
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has uploaded a new patch set (#2).
Change subject: IMPALA-5696: Enable cipher configuration when using TLS / Thrift
......................................................................
IMPALA-5696: Enable cipher configuration when using TLS / Thrift
The 'cipher suite' is a description of the set of algorithms used by SSL
and TLS to execute key exchange, encryption, message authentication, and
random number generation functions. SSL implementations allow the cipher
suite to be configured so that ciphers may be removed from the whitelist
if they are shown to be weak.
* Add a flag --ssl_cipher_list which controls cipher selection for both
thrift servers and clients. Default is blank, which means use all
available cipher suites.
* Add ThriftServerBuilder to simplify construction of
ThriftServers (whose constructors were otherwise getting very long).
Testing: new tests added to thrift-server-test. Test cases added follow:
* A client cannot connect to a server which does not have any ciphers in
common with it.
* If ciphers are identical on clients and servers, that ssl connections
can be made.
* Bad cipher strings lead to errors on both client and server.
Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
---
M be/src/benchmarks/network-perf-benchmark.cc
M be/src/catalog/catalogd-main.cc
M be/src/rpc/thrift-client.cc
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/runtime/data-stream-test.cc
M be/src/service/impala-server.cc
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestored-main.cc
M be/src/testutil/in-process-servers.cc
A be/src/testutil/scoped-flag-setter.h
M be/src/util/webserver-test.cc
13 files changed, 426 insertions(+), 144 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/24/7524/2
--
To view, visit http://gerrit.cloudera.org:8080/7524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-5696: Enable cipher configuration when using TLS / Thrift
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5696: Enable cipher configuration when using TLS / Thrift
......................................................................
Patch Set 3: Code-Review+2
(1 comment)
Makes sense. I like the refactoring; we should probably be using the builder pattern in a few more cases where we have absurdly large constructors.
http://gerrit.cloudera.org:8080/#/c/7524/3/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
PS3, Line 173: TODO: For 3.0
This should be tracked by a JIRA otherwise we might forget
--
To view, visit http://gerrit.cloudera.org:8080/7524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5696: Enable cipher configuration when using TLS / Thrift
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5696: Enable cipher configuration when using TLS / Thrift
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1004/
--
To view, visit http://gerrit.cloudera.org:8080/7524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5696: Enable cipher configuration when using TLS / Thrift
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-5696: Enable cipher configuration when using TLS / Thrift
......................................................................
IMPALA-5696: Enable cipher configuration when using TLS / Thrift
The 'cipher suite' is a description of the set of algorithms used by SSL
and TLS to execute key exchange, encryption, message authentication, and
random number generation functions. SSL implementations allow the cipher
suite to be configured so that ciphers may be removed from the whitelist
if they are shown to be weak.
* Add a flag --ssl_cipher_list which controls cipher selection for both
thrift servers and clients. Default is blank, which means use all
available cipher suites.
* Add ThriftServerBuilder to simplify construction of
ThriftServers (whose constructors were otherwise getting very long).
Testing: new tests added to thrift-server-test. Test cases added follow:
* A client cannot connect to a server which does not have any ciphers in
common with it.
* If ciphers are identical on clients and servers, that ssl connections
can be made.
* Bad cipher strings lead to errors on both client and server.
Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
Reviewed-on: http://gerrit.cloudera.org:8080/7524
Reviewed-by: Henry Robinson <he...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/benchmarks/network-perf-benchmark.cc
M be/src/catalog/catalogd-main.cc
M be/src/rpc/thrift-client.cc
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/runtime/data-stream-test.cc
M be/src/service/impala-server.cc
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestored-main.cc
M be/src/testutil/in-process-servers.cc
A be/src/testutil/scoped-flag-setter.h
M be/src/util/webserver-test.cc
13 files changed, 440 insertions(+), 143 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Henry Robinson: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/7524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-5696: Enable cipher configuration when using TLS / Thrift
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5696: Enable cipher configuration when using TLS / Thrift
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/998/
--
To view, visit http://gerrit.cloudera.org:8080/7524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5696: Enable cipher configuration when using TLS / Thrift
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-5696: Enable cipher configuration when using TLS / Thrift
......................................................................
Patch Set 5: Code-Review+2
Fix some clang-tidy warnings in the tests.
--
To view, visit http://gerrit.cloudera.org:8080/7524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5696: Enable cipher configuration when using TLS / Thrift
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-5696: Enable cipher configuration when using TLS / Thrift
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/7524/1/be/src/rpc/thrift-server.h
File be/src/rpc/thrift-server.h:
PS1, Line 254: /// Sets the auth provider for this server. Default is the system global auth provider.
: ThriftServerBuilder& auth_provider(AuthProvider* provider) {
: auth_provider_ = provider;
> The idea is that the c'tors take mandatory parameters. Doing it this way me
I was alluding to the caller explicitly setting what options they want. But I guess we can argue for it either way, so I'm fine leaving it the way it is too.
http://gerrit.cloudera.org:8080/#/c/7524/1/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
PS1, Line 1947: be_builder.metrics(exec_env->metrics())
> Hm, why? That introduces an extra statement, and is kind of against the poi
I was thinking about it from a readability point of view, but this is fine too.
http://gerrit.cloudera.org:8080/#/c/7524/2/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
PS2, Line 172: ssl_cipher_list
I'm wondering if we should default to "intermediate" compatibility:
Ciphersuites: ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:DHE-RSA-AES128-GCM-SHA256:DHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA256:ECDHE-ECDSA-AES128-SHA:ECDHE-RSA-AES256-SHA384:ECDHE-RSA-AES128-SHA:ECDHE-ECDSA-AES256-SHA384:ECDHE-ECDSA-AES256-SHA:ECDHE-RSA-AES256-SHA:DHE-RSA-AES128-SHA256:DHE-RSA-AES128-SHA:DHE-RSA-AES256-SHA256:DHE-RSA-AES256-SHA:ECDHE-ECDSA-DES-CBC3-SHA:ECDHE-RSA-DES-CBC3-SHA:EDH-RSA-DES-CBC3-SHA:AES128-GCM-SHA256:AES256-GCM-SHA384:AES128-SHA256:AES256-SHA256:AES128-SHA:AES256-SHA:DES-CBC3-SHA:!DSS
This is the recommended yet conservative set of ciphers.
From https://wiki.mozilla.org/Security/Server_Side_TLS
Older ciphers are not recommended, and if anyone wants to use any of them, they will have to specifically add them to this list.
Risk: Might break existing clients after they upgrade.
Reward: Better default security.
What do you think?
http://gerrit.cloudera.org:8080/#/c/7524/1/be/src/statestore/statestore-subscriber.cc
File be/src/statestore/statestore-subscriber.cc:
PS1, Line 203: ThriftServer* server;
: RETURN_IF_ERROR(builder.Build(&server));
: heartbeat_server_.reset(server);
> Not sure this is warranted (you could argue the same thing about 'builder')
Yes, but 'server' will be an invalid pointer after the reset(). But it's fine to leave it as it is.
--
To view, visit http://gerrit.cloudera.org:8080/7524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5696: Enable cipher configuration when using TLS / Thrift
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5696: Enable cipher configuration when using TLS / Thrift
......................................................................
Patch Set 6: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/7524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5696: Enable cipher configuration when using TLS / Thrift
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5696: Enable cipher configuration when using TLS / Thrift
......................................................................
Patch Set 5:
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1000/
--
To view, visit http://gerrit.cloudera.org:8080/7524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5696: Enable cipher configuration when using TLS / Thrift
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-5696: Enable cipher configuration when using TLS / Thrift
......................................................................
Patch Set 3: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/7524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5696: Enable cipher configuration when using TLS / Thrift
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-5696: Enable cipher configuration when using TLS / Thrift
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7524/2/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
PS2, Line 172: ssl_cipher_list
> I'm wondering if we should default to "intermediate" compatibility:
It's a good idea, but I'm really keen not to break any existing clients. I'll leave a comment for 3.0!
--
To view, visit http://gerrit.cloudera.org:8080/7524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5696: Enable cipher configuration when using TLS / Thrift
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins, Matthew Jacobs, Sailesh Mukil,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7524
to look at the new patch set (#5).
Change subject: IMPALA-5696: Enable cipher configuration when using TLS / Thrift
......................................................................
IMPALA-5696: Enable cipher configuration when using TLS / Thrift
The 'cipher suite' is a description of the set of algorithms used by SSL
and TLS to execute key exchange, encryption, message authentication, and
random number generation functions. SSL implementations allow the cipher
suite to be configured so that ciphers may be removed from the whitelist
if they are shown to be weak.
* Add a flag --ssl_cipher_list which controls cipher selection for both
thrift servers and clients. Default is blank, which means use all
available cipher suites.
* Add ThriftServerBuilder to simplify construction of
ThriftServers (whose constructors were otherwise getting very long).
Testing: new tests added to thrift-server-test. Test cases added follow:
* A client cannot connect to a server which does not have any ciphers in
common with it.
* If ciphers are identical on clients and servers, that ssl connections
can be made.
* Bad cipher strings lead to errors on both client and server.
Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
---
M be/src/benchmarks/network-perf-benchmark.cc
M be/src/catalog/catalogd-main.cc
M be/src/rpc/thrift-client.cc
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/runtime/data-stream-test.cc
M be/src/service/impala-server.cc
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestored-main.cc
M be/src/testutil/in-process-servers.cc
A be/src/testutil/scoped-flag-setter.h
M be/src/util/webserver-test.cc
13 files changed, 430 insertions(+), 142 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/24/7524/5
--
To view, visit http://gerrit.cloudera.org:8080/7524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-5696: Enable cipher configuration when using TLS / Thrift
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has uploaded a new patch set (#3).
Change subject: IMPALA-5696: Enable cipher configuration when using TLS / Thrift
......................................................................
IMPALA-5696: Enable cipher configuration when using TLS / Thrift
The 'cipher suite' is a description of the set of algorithms used by SSL
and TLS to execute key exchange, encryption, message authentication, and
random number generation functions. SSL implementations allow the cipher
suite to be configured so that ciphers may be removed from the whitelist
if they are shown to be weak.
* Add a flag --ssl_cipher_list which controls cipher selection for both
thrift servers and clients. Default is blank, which means use all
available cipher suites.
* Add ThriftServerBuilder to simplify construction of
ThriftServers (whose constructors were otherwise getting very long).
Testing: new tests added to thrift-server-test. Test cases added follow:
* A client cannot connect to a server which does not have any ciphers in
common with it.
* If ciphers are identical on clients and servers, that ssl connections
can be made.
* Bad cipher strings lead to errors on both client and server.
Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
---
M be/src/benchmarks/network-perf-benchmark.cc
M be/src/catalog/catalogd-main.cc
M be/src/rpc/thrift-client.cc
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/runtime/data-stream-test.cc
M be/src/service/impala-server.cc
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestored-main.cc
M be/src/testutil/in-process-servers.cc
A be/src/testutil/scoped-flag-setter.h
M be/src/util/webserver-test.cc
13 files changed, 427 insertions(+), 142 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/24/7524/3
--
To view, visit http://gerrit.cloudera.org:8080/7524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-5696: Enable cipher configuration when using TLS / Thrift
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-5696: Enable cipher configuration when using TLS / Thrift
......................................................................
Patch Set 1:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/7524/1/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:
Line 276:
> I would add one more test case where the server is started with a list of c
Done
http://gerrit.cloudera.org:8080/#/c/7524/1/be/src/rpc/thrift-server.h
File be/src/rpc/thrift-server.h:
PS1, Line 166: friend class ThriftServerBuilder;
> nit: Move just below private: as it's easier to find, plus that's the proto
Done
PS1, Line 247: // Returns true if, per the process configuration flags, server<->server communications
: // should use SSL.
: bool EnableInternalSslConnections();
> nit: Move below ThriftServerBuilder class or above ThriftServer class.
Done
PS1, Line 254: ThriftServerBuilder(const std::string& name,
: const boost::shared_ptr<apache::thrift::TProcessor>& processor, int port)
: : name_(name), processor_(processor), port_(port) {}
> Builder constructors usually just take the name from what I've seen in the
The idea is that the c'tors take mandatory parameters. Doing it this way means there's no way to construct a server without a name or processor or port, which means you don't need to check the error of whether they're set or not.
I'm not sure what you mean by "options are being set at the caller side vs passing members into a constructor" - can you clarify?
PS1, Line 259: auth_provider
> Since this is a builder class, I'd prefer if all these functions were prepe
I prefer not, unless you feel strongly - the 'set_' is redundant with the context of it being a builder.
Line 311: Status Build(ThriftServer** server) {
> Thorough error checking needs to be done on the builder side before creatin
Any use of the server will trigger a failure if it's misconfigured (and I think duplicating error checking here probably doesn't make sense for that reason).
http://gerrit.cloudera.org:8080/#/c/7524/1/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
PS1, Line 1947: be_builder.metrics(exec_env->metrics())
> Set metrics just after constructor initialization in L1940 and just call be
Hm, why? That introduces an extra statement, and is kind of against the point of a fluent interface (where you can chain things together).
http://gerrit.cloudera.org:8080/#/c/7524/1/be/src/statestore/statestore-subscriber.cc
File be/src/statestore/statestore-subscriber.cc:
PS1, Line 203: ThriftServer* server;
: RETURN_IF_ERROR(builder.Build(&server));
: heartbeat_server_.reset(server);
> Surround with curly braces so that 'server' goes out of scope when it's not
Not sure this is warranted (you could argue the same thing about 'builder'). It's usually ok for local variables to extend their scope to the end of a method.
--
To view, visit http://gerrit.cloudera.org:8080/7524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5696: Enable cipher configuration when using TLS / Thrift
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5696: Enable cipher configuration when using TLS / Thrift
......................................................................
Patch Set 5: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/998/
--
To view, visit http://gerrit.cloudera.org:8080/7524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5696: Enable cipher configuration when using TLS / Thrift
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5696: Enable cipher configuration when using TLS / Thrift
......................................................................
Patch Set 6:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1011/
--
To view, visit http://gerrit.cloudera.org:8080/7524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5696: Enable cipher configuration when using TLS / Thrift
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5696: Enable cipher configuration when using TLS / Thrift
......................................................................
Patch Set 4:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/995/
--
To view, visit http://gerrit.cloudera.org:8080/7524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5696: Enable cipher configuration when using TLS / Thrift
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5696: Enable cipher configuration when using TLS / Thrift
......................................................................
Patch Set 6:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1005/
--
To view, visit http://gerrit.cloudera.org:8080/7524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5696: Enable cipher configuration when using TLS / Thrift
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-5696: Enable cipher configuration when using TLS / Thrift
......................................................................
Patch Set 1:
(12 comments)
A lot of the comments are just about refactoring. Feel free to disagree with them if you think it's better the way it is.
http://gerrit.cloudera.org:8080/#/c/7524/1//COMMIT_MSG
Commit Message:
PS1, Line 11: allows
nit: allow
http://gerrit.cloudera.org:8080/#/c/7524/1/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:
Line 276:
I would add one more test case where the server is started with a list of ciphers and the client is started with a different list of ciphers such that there is an overlap of a few ciphers; and test that things work as expected.
Eg:
Server has 3DES and RSA
Client has AES256 and RSA
Things should work because RSA is common between them?
(If the above is a valid example)
http://gerrit.cloudera.org:8080/#/c/7524/1/be/src/rpc/thrift-server.h
File be/src/rpc/thrift-server.h:
PS1, Line 166: friend class ThriftServerBuilder;
nit: Move just below private: as it's easier to find, plus that's the protocol usually followed to declare a friend.
PS1, Line 247: // Returns true if, per the process configuration flags, server<->server communications
: // should use SSL.
: bool EnableInternalSslConnections();
nit: Move below ThriftServerBuilder class or above ThriftServer class.
Good to have the builder and target class next to each other for readability.
PS1, Line 254: ThriftServerBuilder(const std::string& name,
: const boost::shared_ptr<apache::thrift::TProcessor>& processor, int port)
: : name_(name), processor_(processor), port_(port) {}
Builder constructors usually just take the name from what I've seen in the past.
Everything else would ideally be set as an option.
Plus, that looks easier to read by knowing what options are being set at the caller side vs. passing members into a constructor.
The Build() function can return an error if the required parameters are not set.
PS1, Line 259: auth_provider
Since this is a builder class, I'd prefer if all these functions were prepended with "set_"
Eg:
set_auth_provider(), etc.
Line 311: Status Build(ThriftServer** server) {
Thorough error checking needs to be done on the builder side before creating the target class object.
Eg: if (port_ == 0)
This is if you decide to change the constructor as I suggested above.
http://gerrit.cloudera.org:8080/#/c/7524/1/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
PS1, Line 1947: be_builder.metrics(exec_env->metrics())
Set metrics just after constructor initialization in L1940 and just call be_builder.Build() here.
PS1, Line 1974: builder.auth_provider(AuthManager::GetInstance()->GetExternalAuthProvider())
: .metrics(exec_env->metrics())
: .thread_pool(FLAGS_fe_service_threads)
Same here, set all options just after constructor init and just call builder.Build() here.
PS1, Line 2000: builder.auth_provider(AuthManager::GetInstance()->GetExternalAuthProvider())
: .metrics(exec_env->metrics())
: .thread_pool(FLAGS_fe_service_threads)
: .Build(hs2_server));
Same as above
http://gerrit.cloudera.org:8080/#/c/7524/1/be/src/statestore/statestore-subscriber.cc
File be/src/statestore/statestore-subscriber.cc:
PS1, Line 203: ThriftServer* server;
: RETURN_IF_ERROR(builder.Build(&server));
: heartbeat_server_.reset(server);
Surround with curly braces so that 'server' goes out of scope when it's not needed?
http://gerrit.cloudera.org:8080/#/c/7524/1/be/src/statestore/statestored-main.cc
File be/src/statestore/statestored-main.cc:
PS1, Line 89: metrics(metrics.get())
move to L83.
Also follow this pattern everywhere else if you agree.
--
To view, visit http://gerrit.cloudera.org:8080/7524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5696: Enable cipher configuration when using TLS / Thrift
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-5696: Enable cipher configuration when using TLS / Thrift
......................................................................
Patch Set 6: Code-Review+2
Forgot to add the couple of lines of code to enable this in the BE and FE servers :/ Done now, carry +2.
--
To view, visit http://gerrit.cloudera.org:8080/7524
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No