Todd Lipcon has posted comments on this change. Change subject: TLS-negotiation [8/n]: TLS negotiation ......................................................................
Patch Set 5: (10 comments) http://gerrit.cloudera.org:8080/#/c/5762/5//COMMIT_MSG Commit Message: Line 9: This commit adds TLS negotiation to the KRPC protocol, and removes the can you document this in rpc.md? http://gerrit.cloudera.org:8080/#/c/5762/5/src/kudu/rpc/client_negotiation.cc File src/kudu/rpc/client_negotiation.cc: Line 338: // TODO(dan): should this be done without copying the response token? I suppose it could but I don't see it has a potential perf issue (compared to the cost of waiting for the round trip, what's the extra increment of a refcount?) PS5, Line 345: InvalidArgument would prefer NetworkError or something here (same below in a few places) http://gerrit.cloudera.org:8080/#/c/5762/5/src/kudu/rpc/client_negotiation.h File src/kudu/rpc/client_negotiation.h: PS5, Line 93: borrowed borrowed makes it sound like it's temporarily taken exclusive ownership of, whereas really it just references it. http://gerrit.cloudera.org:8080/#/c/5762/5/src/kudu/rpc/constants.cc File src/kudu/rpc/constants.cc: Line 28: set<RpcFeatureFlag> kSupportedServerRpcFeatureFlags = { APPLICATION_FEATURE_FLAGS }; Add a comment why TLS isn't listed on the server too? http://gerrit.cloudera.org:8080/#/c/5762/5/src/kudu/rpc/messenger.cc File src/kudu/rpc/messenger.cc: PS5, Line 305: tls_context_.reset(new security::TlsContext()); : RETURN_NOT_OK(tls_context_->Init()); I think it's worth a comment saying something like "enable TLS unconditionally for use by clients" http://gerrit.cloudera.org:8080/#/c/5762/5/src/kudu/rpc/messenger.h File src/kudu/rpc/messenger.h: Line 232: bool server_tls_enabled_; now that we support negotiation, I'm guessing we'll soon need the 'RequireTLS()' type call on the client side as well to prevent downgrades (eg in Impala's use case they want to blanket on/off it, and we'll need it too) http://gerrit.cloudera.org:8080/#/c/5762/5/src/kudu/rpc/server_negotiation.cc File src/kudu/rpc/server_negotiation.cc: Line 135: if (tls_context_ && ContainsKey(client_features_, TLS)) { also worth a TODO here about allowing the server to specify TLS-required? PS5, Line 365: InvalidArgument same as elsewhere http://gerrit.cloudera.org:8080/#/c/5762/5/src/kudu/rpc/server_negotiation.h File src/kudu/rpc/server_negotiation.h: Line 95: // Allow TLS to be used on the connection. 'tls_context' is borrowed, and must same comment as the equivalent in client_negotation -- To view, visit http://gerrit.cloudera.org:8080/5762 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4f7d36ed8ecf6067a3043344557f45ebd3cdcf9d Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[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
