Sailesh Mukil has posted comments on this change. Change subject: WIP: rpc: Initiate TLS connection upgrade following SASL negotiation ......................................................................
Patch Set 1: (3 comments) This looks good to me. I just have a few comments. I was under the impression that previously, it was decided that the SASL negotiation would need to be encrypted as well. Would that be the case when we have the server flag that forces TLS? Also, the downgrade attack seems very easy to do. Is there any way to deal with this when someone sets the future server side "force TLS" flag to off? http://gerrit.cloudera.org:8080/#/c/5484/1/src/kudu/rpc/negotiation.cc File src/kudu/rpc/negotiation.cc: PS1, Line 189: if (ContainsKey(conn->sasl_client().server_features(), TLS)) { Don't we also need to check if the client itself supports those features? If the remote server supports it, but the client doesn't, it will DCHECK in conn->InitTls(). PS1, Line 210: if (ContainsKey(conn->sasl_server().client_features(), TLS)) { Same here for the server. http://gerrit.cloudera.org:8080/#/c/5484/1/src/kudu/rpc/sasl_helper.h File src/kudu/rpc/sasl_helper.h: PS1, Line 102: // Check if TLS is supported. : Status IsTlsEnabled(); This doesn't seem to be implemented. -- To view, visit http://gerrit.cloudera.org:8080/5484 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If44a71186eb2cdeebaf46cc372596f3ee6b47ac0 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
