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 13: (24 comments) Done http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/rpc/authentication.cc@184 PS12, Line 184: DEFINE_bool(jwt_token_auth, false, > This flag also needs to be deprecated. Done. Marked --jwt_token_auth as deprecated and updated the help text to direct users to --oauth_token_auth with --oauth_servers http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/rpc/authentication.cc@224 PS12, Line 224: DEFINE_bool_hidden(jwt_allow_without_tls, false, > This flag needs to be marked deprecated too. Done. Marked --jwt_allow_without_tls as deprecated and updated the help text to direct users to --oauth_allow_without_tls http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/rpc/authentication.cc@796 PS12, Line 796: const AuthenticationHash& hash, const string& token) { > This function contains a lot of code in common with Webserver::OAuthTokenAu Done. Moved common bearer-token decode/verify/username logic into OAuthServersManager::AuthenticateBearerToken() and now both RPC and webserver paths call this shared function. http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/rpc/authentication.cc@797 PS12, Line 797: = > This function should not need to know whether it's handling JWT or OAuth. Done. Removed JWT/OAuth mode branching from the RPC bearer-token auth function and switched to a single unified bearer-token flow http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/rpc/authentication.cc@807 PS12, Line 807: connection_context->return_headers.push_back( > Make this message more generic by having it say something like, "Error deco Done. Updated RPC logging to use generic bearer-token validation errors instead of token-type-specific wording. http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/rpc/authentication.cc@819 PS12, Line 819: } : : // Performs a step of SPNEGO auth for the > Same comment as line 807. Possibly also include the token's JWKS and key i Done for the generic wording. I kept logs metadata-light (no JWKS/kid) to avoid leaking token/key identifiers in error paths. http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/rpc/authentication.cc@833 PS12, Line 833: } > Same comment as line 807. Done. Updated this RPC error path to the same generic bearer-token validation wording. http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/rpc/authentication.cc@843 PS12, Line 843: if (spnego_status.ok()) { > The call to GenerateCookie() should always pass HTTP_AUTH_MECH_OAUTH as the Done. Updated cookie generation to always pass HTTP_AUTH_MECH_OAUTH. http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/service/impala-server.cc@3237 PS12, Line 3237: if (FLAGS_jwt_token_auth || FLAGS_oauth_token_auth) { > Unnecessary variable declaration. Done. Removed the temporary boolean and inlined the condition directly. http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/jwt-util.cc File be/src/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/jwt-util.cc@765 PS12, Line 765: jwks_pull_timeout_secs_ = jwks_pull_timeout_secs; : jwks_update_frequency_secs_ = jwks_update_frequency_secs; : DCHECK_GT(jwks_pull_timeout_secs_, 0); : DCHECK(is_local_file || jwks_update_frequency_secs_ > 0); : std::shared_ptr<JWKSSnapshot> new_jwks = std::make_shared<JWKSSnapshot>(); : if (is_local_file) { > Now that legacy JWT and OAuth flags are managed by OAuthServersManager, the Done. Removed fallback usage of those legacy flags in JWKSMgr::Init() and now use timeout/frequency values supplied by manager-driven config. http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/jwt-util.cc@852 PS12, Line 852: Status JWTHelper::Init(const std::string& jwks_file_path) { : return Init(jwks_file_path, fals > Set the ctor/dtor to the default implementation in the header file and dele Done. Defaulted JWTHelper ctor/dtor in the header and removed explicit ctor/dtor definitions from the .cc file. http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/oauth-server-config.h File be/src/util/oauth-server-config.h: http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/oauth-server-config.h@40 PS12, Line 40: int32_t jwks_update_frequency_secs = DEFAULT_JWKS_UPDATE_FREQUENCY_SECS; > I realize now that OAuthServerConfig needs to have a "validateTokenSignatur Done. Added validate_token_signature to OAuthServerConfig, parse support for validateTokenSignature in --oauth_servers, and mapping from legacy signature-validation flags http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/oauth-server-config.cc File be/src/util/oauth-server-config.cc: http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/oauth-server-config.cc@57 PS12, Line 57: namespace impala { > Need to check jwt_token_auth flag only. Done. http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/oauth-server-config.cc@61 PS12, Line 61: bool IsLegacyJwtConfigFlagSpecified() { > Need to check oauth_token_auth flag only. Done. http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/oauth-servers-manager-test.cc File be/src/util/oauth-servers-manager-test.cc: http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/oauth-servers-manager-test.cc@145 PS12, Line 145: size_t matched_server_idx = static_cast<size_t>(-1); : Status status = manager.Verify(decoded_token.get(), &matched_server_idx); > Initialize matched_server_idx to -1 and then check that matched_server_idx Done. Updated OAuthServersManagerTest.VerifyFailsWhenOnlyWrongJwksConfigured to initialize with -1 and assert that it remains unchanged on failure. http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/oauth-servers-manager.h File be/src/util/oauth-servers-manager.h: http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/oauth-servers-manager.h@54 PS12, Line 54: size_t* matched_server_idx_out) const; > Please also note that on failure, the value of matched_server_idx_out is no Done. Updated API contract comment and behavior to preserve matched_server_idx_out on verify failure. http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/oauth-servers-manager.h@66 PS12, Line 66: > Nit: The variable name impl_ is not very descriptive. Please rename it to Done. Renamed internal member from impl_ to jwt_helpers_. http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/oauth-servers-manager.cc File be/src/util/oauth-servers-manager.cc: http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/oauth-servers-manager.cc@40 PS12, Line 40: OAuthServerVerifier verifier; > Why the switch to using a std::unique_ptr for the JWTHelper instance? The Done. Switched manager storage to hold JWTHelper directly (no std::unique_ptr<JWTHelper> wrapper) http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/oauth-servers-manager.cc@45 PS12, Line 45: verifier.username_claim = std::move(config.username_claim); > There is no need to keep the OAuthServerConfig object around once the JWTHe Done. Removed persistent storage of full OAuthServerConfig from manager internals. http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/oauth-servers-manager.cc@57 PS12, Line 57: size_t matched_server_idx = 0; : RETURN_IF_ERROR(Verify(decoded_token.get(), &matched_server_idx)); : RETURN_IF_ERROR(GetUsername(decoded_token.get(), matched_server_idx, username_out)); : if (matched_server_idx_out != nullptr) *matched_server_idx_out = matched_server_idx; : return Status::OK(); : } : : Sta > Verifying decoded_token is a cryptographical operation and thus could poten Done. Added key preselection via JWTHelper::CanVerify() (kid/algorithm-family check) before signature verification, so cryptographic verify is only attempted on matching helper candidates. http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/webserver.cc File be/src/util/webserver.cc: http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/webserver.cc@1160 PS12, Line 1160: (void)connection; > This function should not need to know whether it's handling JWT or OAuth. Done. Removed JWT/OAuth mode branching from webserver bearer auth and switched to the same manager-backed bearer-token flow used in RPC http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/webserver.cc@1169 PS12, Line 1169: return false; > Make this message more generic by having it say something like, "Error deco Done. Updated webserver logs to generic bearer-token validation error wording http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/webserver.cc@1180 PS12, Line 1180: return Status::Expected("No Authorization header provided."); : } : > Same comment as line 1169. Possibly also include the token's JWKS and key Done for generic wording. I intentionally left out JWKS/kid details to avoid logging token key identifiers. http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/webserver.cc@1191 PS12, Line 1191: << GetRemoteAddress(request_info).ToString() << " Error: " << status; : return status; > Same comment as line 1169. Done. Updated this webserver error path to the same generic bearer-token validation wording. -- 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: 13 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, 06 Jul 2026 21:56:52 +0000 Gerrit-HasComments: Yes
