Anubhav Jindal has posted comments on this change. ( http://gerrit.cloudera.org:8080/24448 )
Change subject: IMPALA-14799: Add oauth_servers support and tests ...................................................................... Patch Set 16: (8 comments) Done http://gerrit.cloudera.org:8080/#/c/24448/14/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/24448/14/be/src/rpc/authentication.cc@790 PS14, Line 790: LOG(ERROR) << "Error validating bearer token in Authorization header received from: " > Nit: this variable declaration is not really necessary since its only used I inlined that expression into the log statement and removed the extra variable. http://gerrit.cloudera.org:8080/#/c/24448/14/be/src/util/jwt-util.cc File be/src/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/24448/14/be/src/util/jwt-util.cc@852 PS14, Line 852: void JWTHelper::TokenDeleter::operator()(JWTHelper::JWTDecodedToken* token) const { : if (token != nullptr) delete token; : }; : > Nit: since there is no code here, remove these lines and set the constructo I defaulted the ctor/dtor/move members directly in jwt-util.h and removed the out-of-line defaults from jwt-util.cc http://gerrit.cloudera.org:8080/#/c/24448/14/be/src/util/jwt-util.cc@1006 PS14, Line 1006: > Nit: add the `const` keyword to this variable declaration. Done. Added const to the variable declaration http://gerrit.cloudera.org:8080/#/c/24448/14/be/src/util/jwt-util.cc@1009 PS14, Line 1009: key_map = jwks->GetAllHSKeys(); : } else if (prefix == RS_ALGORITHM_PREFIX || prefix == PS_ALGORITHM_PREFIX) { : key_map = jwks->GetAllRSAPublicKeys(); : } else if (prefix == ES_ALGORITHM_PREFIX) { : key_map = jwks->GetAllECPu > Even though existing code uses magic strings for the algorithm prefixes, pl Done. Replaced the literal prefixes with named constants for algorithm-family checks. http://gerrit.cloudera.org:8080/#/c/24448/14/be/src/util/oauth-servers-manager.h File be/src/util/oauth-servers-manager.h: http://gerrit.cloudera.org:8080/#/c/24448/14/be/src/util/oauth-servers-manager.h@53 PS14, Line 53: /// failure, 'matched_server_idx_out' is not modified. > This function is only called from within OAuthServersManager. Move it to t Done. Moved Verify() to the private section of OAuthServersManager http://gerrit.cloudera.org:8080/#/c/24448/14/be/src/util/oauth-servers-manager.cc File be/src/util/oauth-servers-manager.cc: http://gerrit.cloudera.org:8080/#/c/24448/14/be/src/util/oauth-servers-manager.cc@51 PS14, Line 51: Status OAuthServersManager::AuthenticateBearerToken(const string& token, > There is duplicate code in this function and OAuthServersManager::Verify. Done. I extracted the shared server-match logic into FindMatchingServer() and reused it from both paths. http://gerrit.cloudera.org:8080/#/c/24448/14/be/src/util/oauth-servers-manager.cc@55 PS14, Line 55: JWTHelper::UniqueJWTDecodedToken decoded_token; > Is this line necessary? Done. Removed the unnecessary line as part of the shared-logic cleanup in AuthenticateBearerToken() http://gerrit.cloudera.org:8080/#/c/24448/14/be/src/util/webserver.cc File be/src/util/webserver.cc: http://gerrit.cloudera.org:8080/#/c/24448/14/be/src/util/webserver.cc@753 PS14, Line 753: // Random value from cookie that we'll also use as a csrf_token to implement the : // "Double Submit Cookie" and custom header (X-Requested-By) patterns for preventing : // cross-site request forgery (CSRF). : std::string cookie_rand_value; : // With JWTs we can skip CSRF protection because browsers won't send "Authorization: : // Bearer" headers automatically. : bo > What is the reason for this code? The WWW-Authenticate header is only adde Done. Removed that dead header-removal block from the success path in webserver auth flow. -- To view, visit http://gerrit.cloudera.org:8080/24448 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib29ff36600406ba59c10f29d79cc632020f4a3f7 Gerrit-Change-Number: 24448 Gerrit-PatchSet: 16 Gerrit-Owner: Anubhav Jindal <[email protected]> Gerrit-Reviewer: Anubhav Jindal <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Comment-Date: Wed, 29 Jul 2026 21:27:40 +0000 Gerrit-HasComments: Yes
