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 17: (13 comments) 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; > Done. I moved WWW-Authenticate bearer failure header construction into OAut 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: JWKSSnapshotPtr jwks = GetJWKS(); > Done. CanVerify() now uses UNLIKELY and checks both empty and short algorit Done http://gerrit.cloudera.org:8080/#/c/24448/17/be/src/util/jwt-util.cc File be/src/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/24448/17/be/src/util/jwt-util.cc@1025 PS17, Line 1025: if (!decoded_token->decoded_jwt_.has_key_id()) { Please add a comment here explaining that if the decoded_jwt_ does not have a key id specified, then we cannot know exactly which JWTHelper to use, and this JWTHelper is a candidate for validating the JWT since it has at least one key with the same algorithm as the JWT. http://gerrit.cloudera.org:8080/#/c/24448/17/be/src/util/jwt-util.cc@1030 PS17, Line 1030: const JWTPublicKey* pub_key = nullptr; Please add a comment here explaining that the decoded_jwt_ contains a specific key id, and this JWTHelper can validate the JWT if its JWKS contains a key with the same id. 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; > Done. I replaced the DCHECK_OK path by making BuildLegacyJwtServerConfig() Done 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(); > Done. I applied the same fix for legacy OAuth config: BuildLegacyOAuthServe Done http://gerrit.cloudera.org:8080/#/c/24448/17/be/src/util/oauth-servers-manager.h File be/src/util/oauth-servers-manager.h: http://gerrit.cloudera.org:8080/#/c/24448/17/be/src/util/oauth-servers-manager.h@38 PS17, Line 38: size_t* matched_server_idx_out = nullptr The matched_server_idx_out parameter is only used in test code. While test-only code in main code is not necessarily bad, it is a design smell. Please look into eliminating the function parameter both here and on the Verify() function. 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)); : > Done. I removed the pre-verification FindMatchingServer()/username prefetch Done http://gerrit.cloudera.org:8080/#/c/24448/16/be/src/util/oauth-servers-manager.cc@75 PS16, Line 75: DCHECK(!jwt_helpers_->empty()); > Done. Username is now only extracted on successful verification in Verify() Done http://gerrit.cloudera.org:8080/#/c/24448/17/be/src/util/oauth-servers-manager.cc File be/src/util/oauth-servers-manager.cc: http://gerrit.cloudera.org:8080/#/c/24448/17/be/src/util/oauth-servers-manager.cc@30 PS17, Line 30: string OAuthServersManager::BearerAuthFailureHeader(const Status& status) { : const string error_message = : status.GetDetail().empty() ? status.msg().msg() : status.GetDetail(); : return Substitute("WWW-Authenticate: Bearer error=\"invalid_token\"," : "error_description=\"$0 \"", error_message); : } Please add a test to oauth-servers-manager-test.cc for this function. http://gerrit.cloudera.org:8080/#/c/24448/17/be/src/util/oauth-servers-manager.cc@81 PS17, Line 81: ;) { : Since line 84 handles the case where `matched_server_idx == size()`, I think this should be: `next_idx < jwt_helpers_->size()` http://gerrit.cloudera.org:8080/#/c/24448/17/be/src/util/oauth-servers-manager.cc@108 PS17, Line 108: i < size() Should be i < jwt_helpers_->size() http://gerrit.cloudera.org:8080/#/c/24448/17/be/src/util/oauth-servers-manager.cc@125 PS17, Line 125: if (server_idx >= size()) { : return Status(Substitute("Invalid OAuth server index $0", server_idx)); : } I don't think this condition will ever be true? If it can be true, would a DCHECK be sufficient? If this condition can be true, then it should be modified to something like the following and have a ctest test added for it in oauth-servers-manager-test.cc: if (UNLIKELY(server_idx >= jwt_helpers_->size())) { -- 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: Mon, 10 Aug 2026 19:07:00 +0000 Gerrit-HasComments: Yes
