Todd Lipcon has posted comments on this change. Change subject: [security] add channel binding to krpc ......................................................................
Patch Set 2: (19 comments) http://gerrit.cloudera.org:8080/#/c/5884/2//COMMIT_MSG Commit Message: Line 7: [security] add channel binding to krpc can you add a little bit to rpc.md about it? http://gerrit.cloudera.org:8080/#/c/5884/2/src/kudu/rpc/client_negotiation.cc File src/kudu/rpc/client_negotiation.cc: Line 500: // Send the channel bindings to the client. think this is misplaced Line 503: RETURN_NOT_OK(tls_socket->GetRemoteCert(&cert)); I think a _PREPEND here and below on 506 would be useful Line 508: if (response.channel_bindings().empty()) { is .has_channel_bindings() more appropriate? PS2, Line 512: string actual_channel_bindings; : RETURN_NOT_OK(SaslDecode(response.channel_bindings(), &actual_channel_bindings)); "actual" doesn't sound right to me. Maybe "channel_bindings_from_server"? or "received_channel_bindings" or something? Line 516: // TODO(dan): should we do a more drastic warning? This indicates an active MITM. yea I think a LOG(WARNING) is probably worth it (if we can include the remote peer addr/port that would be nice too) http://gerrit.cloudera.org:8080/#/c/5884/2/src/kudu/rpc/client_negotiation.h File src/kudu/rpc/client_negotiation.h: Line 167: Status HandleSaslSuccess(const NegotiatePB& response) WARN_UNUSED_RESULT; nit: doc http://gerrit.cloudera.org:8080/#/c/5884/2/src/kudu/rpc/rpc_header.proto File src/kudu/rpc/rpc_header.proto: Line 126: optional bytes channel_bindings = 6 [(REDACT) = true]; hrm, does this need to be redacted? what would someone gain by seeing it? (I think the fact that it's just integrity-protected means that it's OK for this to be over an untrusted channel or in a log) http://gerrit.cloudera.org:8080/#/c/5884/2/src/kudu/rpc/sasl_common.cc File src/kudu/rpc/sasl_common.cc: Line 372: sasl_security_properties_t sec_props = {}; does this guarantee zero-initialization? or is a memset(0) a good idea here? (not sure if there are any other fields) http://gerrit.cloudera.org:8080/#/c/5884/2/src/kudu/rpc/server_negotiation.cc File src/kudu/rpc/server_negotiation.cc: Line 550: // TODO(dan): this should actually contain the hash of the cert per RFC 5929. isn't that what the GetServerEndPointChannelbindings is doing? http://gerrit.cloudera.org:8080/#/c/5884/2/src/kudu/security/cert.cc File src/kudu/security/cert.cc: Line 63: // Find the signature type of the certificate. This corresponds to the digest curious if we're really getting much out of following the RFC here vs just always using SHA512. Seems a fair bit of complexity and unknowns Line 65: int signature_nid = X509_get_signature_nid(data_.get()); > error: use of undeclared identifier 'X509_get_signature_nid' [clang-diagnos ruh roh PS2, Line 73: is typo Line 98: ToBIO(bio.get(), DataFormat::DER, data_.get()); doesn't this return Status? PS2, Line 104: append assign? http://gerrit.cloudera.org:8080/#/c/5884/2/src/kudu/security/cert.h File src/kudu/security/cert.h: Line 45: void AdoptAndIncrementRawData(X509* data); hrm, maybe AdoptAndAddRefRawData() or something? it's not really incrementing the data. http://gerrit.cloudera.org:8080/#/c/5884/2/src/kudu/security/tls_socket.cc File src/kudu/security/tls_socket.cc: Line 30: template<> struct SslTypeTraits<X509> { hrm, can we move this to a common header? eg openssl_util-inl.h or something if we don't want it in openssl_util.h? or a new ssl_types.h? http://gerrit.cloudera.org:8080/#/c/5884/2/src/kudu/security/tls_socket.h File src/kudu/security/tls_socket.h: Line 20: #include <boost/optional/optional_fwd.hpp> hm, used? http://gerrit.cloudera.org:8080/#/c/5884/2/src/kudu/util/status.cc File src/kudu/util/status.cc: Line 25: CHECK(code != kOk); I think this should be DCHECK -- To view, visit http://gerrit.cloudera.org:8080/5884 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id73fceebfcb47c881c30f6904cfd6fc6d80f50b8 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
