Tim Armstrong 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 5: (16 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 these methods if the methods on AuthenticationHash were const qualified. http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.h@34 PS5, Line 34: string std::string 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: timstamp timestamp http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@49 PS5, Line 49: if (TryStripPrefixString(cookie_header, StrCat(COOKIE_NAME, "="), &cookie)) { It'd be more consistent to invert this condition and do a goto. http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@71 PS5, Line 71: signnature signature http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@81 PS5, Line 81: long create_time = stol(cookie_value_split[1]); I'd probably prefer using StringParser::StringToInt() for the better error handling. Non-blocking comment since the timestamp is generated by us and we'd hit an error anyway if it returned 0. http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@96 PS5, Line 96: LOG(ERROR) << "Invalid cookie provided: " << cookie_header Maybe should be WARNING or INFO since it doesn't indicate the service itself malfunctioning. http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@104 PS5, Line 104: rand We generally don't like rand() because it's not a particularly good quality RNG. I need to think a bit more about whether it's appropriate here. http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@104 PS5, Line 104: UnixMillis Should this be MonotonicMillis() instead? Since we only use this for comparing against time of this server. http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@105 PS5, Line 105: 32 Can we use a constant here instead of the magic number? http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@112 PS5, Line 112: char base64_signature[base64_len + 1]; Is this array size bounded? I think it would be safer to statically initialise it to the known max size instead of allocating a variable-length array on the stack. http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/rpc/cookie-util.cc@117 PS5, Line 117: string secure_flag = ";Secure"; We're just pointing to constants, so could use a "const char*" and avoid allocating the string. 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: ettempts "attempts" here and below 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: 32 Use a named constant? 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: bool AuthenticationHash::Compute(const uint8_t* data, int64_t len, uint8_t* out) { Looks like this methods could be const http://gerrit.cloudera.org:8080/#/c/13672/5/be/src/util/openssl-util.cc@174 PS5, Line 174: bool AuthenticationHash::Verify( Looks like this methods could be const -- 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: 5 Gerrit-Owner: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Fri, 16 Aug 2019 01:21:17 +0000 Gerrit-HasComments: Yes
