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 <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
