impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sailesh Mukil (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5696: Enable cipher configuration when using TLS / Thrift
Date Fri, 04 Aug 2017 21:25:04 GMT
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 <henry@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message