Dan Burkert has posted comments on this change. Change subject: [security] Add per-connection nonce for Kerberos replay resistance ......................................................................
Patch Set 7: (10 comments) http://gerrit.cloudera.org:8080/#/c/6137/7//COMMIT_MSG Commit Message: PS7, Line 9: suceptible > seems to be a typo: susceptible ? Done http://gerrit.cloudera.org:8080/#/c/6137/7/docs/design-docs/rpc.md File docs/design-docs/rpc.md: PS7, Line 540: tls > nit: TLS ? tls-server-end-point is the name defined by the RFC. http://gerrit.cloudera.org:8080/#/c/6137/7/src/kudu/rpc/sasl_common.cc File src/kudu/rpc/sasl_common.cc: Line 351: Status SaslEncode(sasl_conn_t* conn, const std::string& plaintext, std::string* encoded) { > It seems you have just moved this method here, but while you are at it, con Done PS7, Line 353: const char* out; : unsigned out_len; > Consider making these local for the scope of the while() loop below. Done PS7, Line 358: plaintext.size() > nit: maybe, create a constant for the scope of this method so it would not I think this should be hoisted by the compiler, since plaintext is const. PS7, Line 374: const char* out; : unsigned out_len; > Ditto: consider making there local for the while() loop below. Done Line 378: // have to call decode multiple times if our input is larger than this max. > nit: does it make sense to call something like The plaintext is usually shorter, since there are length delimiters and hash overhead. We're only using these functions for very short tokens, so I'm not sure it matters anyway. http://gerrit.cloudera.org:8080/#/c/6137/7/src/kudu/rpc/server_negotiation.cc File src/kudu/rpc/server_negotiation.cc: PS7, Line 788: CHECK_OK(s); > DCHECK_OK(s) ? Otherwise, why to have that Done http://gerrit.cloudera.org:8080/#/c/6137/7/src/kudu/security/crypto-test.cc File src/kudu/security/crypto-test.cc: PS7, Line 246: ASSERT_EQ > nit here and below: the 'expected' parameter comes first in {ASSERT,EXPECT} Done PS7, Line 247: string(kNonceSize, '\0') > nit: consider making this a constant (or event static constant) for this te I don't think it's possible to have a static std::string. -- To view, visit http://gerrit.cloudera.org:8080/6137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If0fb433896963be5e81d349ebf3a044a458e6627 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
