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 17: (6 comments) 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: connection_context->return_headers.push_back( : OAuthServersManager::BearerAuthFailureHeader(status)); : return false; : } : connection_context->username = username; > Nit: would be nice to have this header generation handled in OAuthServersMa Done. I moved WWW-Authenticate bearer failure header construction into OAuthServersManager::BearerAuthFailureHeader() and now both RPC and webserver auth paths use this shared helper. 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: JWKSSnapshotPtr jwks = GetJWKS(); > Wrap this call with UNLIKELY and add a check for the string length: Done. CanVerify() now uses UNLIKELY and checks both empty and short algorithm values before prefix parsing. 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: config.verify_server_cert = FLAGS_jwks_verify_server_certificate; > This line will not throw an error when compiled in release mode. Done. I replaced the DCHECK_OK path by making BuildLegacyJwtServerConfig() return Status and propagating SetJwksSource() errors with RETURN_IF_ERROR. http://gerrit.cloudera.org:8080/#/c/24448/16/be/src/util/oauth-server-config.cc@182 PS16, Line 182: if (!jwks_file_path.empty() && !jwks_url.empty()) jwks_url.clear(); > This line will not throw an error when compiled in release mode. Done. I applied the same fix for legacy OAuth config: BuildLegacyOAuthServerConfig() now returns Status and propagates SetJwksSource() failures instead of relying on DCHECK. 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: Status OAuthServersManager::AuthenticateBearerToken(const string& token, : string* username_out, size_t* matched_server_idx_out) const { : DCHECK(jwt_helpers_); : DCHECK(username_out != nullptr); : username_out->clear(); : JWTHelper::UniqueJWTDecodedToken decoded_token; : RETURN_IF_ERROR(JWTHelper::Decode(token, decoded_token)); : > This code is not very efficient. FindMatchingServer() is called below from Done. I removed the pre-verification FindMatchingServer()/username prefetch path from AuthenticateBearerToken(). http://gerrit.cloudera.org:8080/#/c/24448/16/be/src/util/oauth-servers-manager.cc@75 PS16, Line 75: DCHECK(!jwt_helpers_->empty()); > I realized today that Verify() can only return the username when either the Done. Username is now only extracted on successful verification in Verify(), we no longer attempt username extraction on failed auth. -- 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: 17 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: Tue, 04 Aug 2026 22:12:26 +0000 Gerrit-HasComments: Yes
