Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13672 )
Change subject: IMPALA-8584: Add cookie support to the HTTP HS2 server ...................................................................... Patch Set 6: (17 comments) http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.h File be/src/rpc/cookie-util.h: http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.h@29 PS5, Line 29: bool AuthenticateCookie(ThriftServer::ConnectionContext* connection_context, > I think hash could be passed in as a const AuthenticationHash& in both thes Done http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.h@34 PS5, Line 34: std::s > std::string Done http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc File be/src/rpc/cookie-util.cc: http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@38 PS5, Line 38: in cooki > timestamp Done http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@49 PS5, Line 49: > It'd be more consistent to invert this condition and do a goto. Done http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@71 PS5, Line 71: > signature Done http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@81 PS5, Line 81: goto error; > I'd probably prefer using StringParser::StringToInt() for the better error Done http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@96 PS5, Line 96: } > Maybe should be WARNING or INFO since it doesn't indicate the service itsel Done http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@104 PS5, Line 104: > We generally don't like rand() because it's not a particularly good quality Yeah, I guess my thinking was that it doesn't really matter what RNG we use here, since being able to guess what number might come next doesn't get you anything and the generated numbers are returned in plain text anyways. The important thing is that we're using a strong RNG to generate the key used for calculating the signature, which we are. Of course, happy to change this if you have a suggestion of a better one to use. http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@104 PS5, Line 104: lifetime. > Should this be MonotonicMillis() instead? Since we only use this for compar Done http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@105 PS5, Line 105: > Can we use a constant here instead of the magic number? Done http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@112 PS5, Line 112: << ": " << error_str; > Is this array size bounded? I think it would be safer to statically initial Yes, the size is bounded, since hash_len is a constant. I rearranged things a bit to make this more obvious. http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@117 PS5, Line 117: string cookie_value = > We're just pointing to constants, so could use a "const char*" and avoid al Done http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/transport/THttpServer.h File be/src/transport/THttpServer.h: http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/transport/THttpServer.h@32 PS5, Line 32: attempts > "attempts" here and below Done http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/util/openssl-util.h File be/src/util/openssl-util.h: http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/util/openssl-util.h@99 PS5, Line 99: s > Use a named constant? Done http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/util/openssl-util.cc File be/src/util/openssl-util.cc: http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/util/openssl-util.cc@167 PS5, Line 167: Status AuthenticationHash::Compute(const uint8_t* data, int64_t len, uint8_t* out) const { > Looks like this methods could be const Done http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/util/openssl-util.cc@169 PS5, Line 169: uint8_t* result = > I think we might need to clean for and clean errors after the HMAC call. Ha Done http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/util/openssl-util.cc@174 PS5, Line 174: DCHECK_EQ(out_len, HashLen()); > Looks like this methods could be const Done -- To view, visit http://gerrit.cloudera.org:8080/13672 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4 Gerrit-Change-Number: 13672 Gerrit-PatchSet: 6 Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Thu, 22 Aug 2019 20:40:07 +0000 Gerrit-HasComments: Yes
