Jason Fehr 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: (14 comments) 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: " > I inlined that expression into the log statement and removed the extra vari Done http://gerrit.cloudera.org:8080/#/c/24448/16/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/24448/16/be/src/rpc/authentication.cc@794 PS16, Line 794: const string error_message = : status.GetDetail().empty() ? status.msg().msg() : status.GetDetail(); : connection_context->return_headers.push_back( : Substitute("WWW-Authenticate: Bearer error=\"invalid_token\",\ : error_description=\"$0 \"", error_message)); Nit: would be nice to have this header generation handled in OAuthServersManager so this logic is contained in one place instead of being duplicated in be/src/util/webserver.cc. 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; : }; : > I defaulted the ctor/dtor/move members directly in jwt-util.h and removed t Done http://gerrit.cloudera.org:8080/#/c/24448/14/be/src/util/jwt-util.cc@1006 PS14, Line 1006: > Done. Added const to the variable declaration Done 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 > Done. Replaced the literal prefixes with named constants for algorithm-fami Done http://gerrit.cloudera.org:8080/#/c/24448/16/be/src/util/jwt-util.cc File be/src/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/24448/16/be/src/util/jwt-util.cc@1001 PS16, Line 1001: if (algorithm.empty()) { Wrap this call with UNLIKELY and add a check for the string length: if (UNLIKELY(algorithm.empty() || algorithm.length() < 2)) { http://gerrit.cloudera.org:8080/#/c/24448/16/be/src/util/oauth-server-config.cc File be/src/util/oauth-server-config.cc: http://gerrit.cloudera.org:8080/#/c/24448/16/be/src/util/oauth-server-config.cc@168 PS16, Line 168: DCHECK_OK(SetJwksSource(jwks_file_path, jwks_url, &config)); This line will not throw an error when compiled in release mode. http://gerrit.cloudera.org:8080/#/c/24448/16/be/src/util/oauth-server-config.cc@182 PS16, Line 182: DCHECK_OK(SetJwksSource(jwks_file_path, jwks_url, &config)); This line will not throw an error when compiled in release mode. 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. > Done. Moved Verify() to the private section of OAuthServersManager Done 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, > Done. I extracted the shared server-match logic into FindMatchingServer() a Done http://gerrit.cloudera.org:8080/#/c/24448/14/be/src/util/oauth-servers-manager.cc@55 PS14, Line 55: JWTHelper::UniqueJWTDecodedToken decoded_token; > Done. Removed the unnecessary line as part of the shared-logic cleanup in A Done http://gerrit.cloudera.org:8080/#/c/24448/16/be/src/util/oauth-servers-manager.cc File be/src/util/oauth-servers-manager.cc: http://gerrit.cloudera.org:8080/#/c/24448/16/be/src/util/oauth-servers-manager.cc@58 PS16, Line 58: size_t username_server_idx = size(); : RETURN_IF_ERROR(FindMatchingServer(decoded_token.get(), 0, &username_server_idx)); : if (username_server_idx < size()) { : Status username_status = : GetUsername(decoded_token.get(), username_server_idx, username_out); : if (!username_status.ok()) username_out->clear(); : } : This code is not very efficient. FindMatchingServer() is called below from the Verify() function, and, if the decoded_token does not specify a key id (which is common), then FindMatchingServer() returns the first OAuth server that has a key that supports the same algorithm as the decoded_token. Remove this code and update Verify() to return the username. http://gerrit.cloudera.org:8080/#/c/24448/16/be/src/util/oauth-servers-manager.cc@75 PS16, Line 75: Status OAuthServersManager::Verify(const JWTHelper::JWTDecodedToken* decoded_token, I realized today that Verify() can only return the username when either the token verification succeeds or the decoded_token has a key id that exactly matches a key id from an OAuth server's JWKS. The reason is those are the only two cases when we can be absolutely sure the correct username claim is read. It's not as important as I originally thought to return the user name when token verification fails. Let's skip implementing the second case (decoded_token key id matches a JWKS key id) and instead ensure a message is logged somewhere (not necessarily here) containing the client IP on failed auth. I see log messages in be/src/rpc/authentication.cc and be/src/util/webserver.cc, so no new log messages should be needed (unless I am misunderstanding the code). 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 > Done. Removed that dead header-removal block from the success path in webse Done -- 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: Mon, 03 Aug 2026 23:08:37 +0000 Gerrit-HasComments: Yes
