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 10: (47 comments) http://gerrit.cloudera.org:8080/#/c/24448/10//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24448/10//COMMIT_MSG@37 PS10, Line 37: Co-authored-by: Cursor <[email protected]> Please don't add the "Co-authored-by" trailer since Impala uses "Assisted-by". http://gerrit.cloudera.org:8080/#/c/24448/10/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/24448/10/be/src/rpc/authentication.cc@824 PS10, Line 824: if (FLAGS_jwt_validate_signature) { : status = oauth_servers_mgr->GetUsername( : decoded_token.get(), matched_server_idx, &username); : } else { : DCHECK(!FLAGS_jwt_custom_claim_username.empty()); : status = JWTHelper::GetCustomClaimUsername( : decoded_token.get(), FLAGS_jwt_custom_claim_username, username); : } This block of code contains an abstraction leak that is not the best approach. It would be best to have the oauth_server_mgr take care of everything. I understand why it was written the way it was though. Let's do this -- if FLAGS_jwt_validate_signature is true, then skip the call to oauth_servers_mgr->Verify() but perform every other check. This means that every incoming JWT will need a corresponding OAuthServerConfig instance defining a JWK for that token. This approach breaks backwards compatibility, but that is ok with the Impala 5 release. http://gerrit.cloudera.org:8080/#/c/24448/10/be/src/rpc/authentication.cc@874 PS10, Line 874: if (FLAGS_oauth_jwt_validate_signature) { : status = oauth_servers_mgr->GetUsername( : decoded_token.get(), matched_server_idx, &username); : } else { : DCHECK(!FLAGS_oauth_jwt_custom_claim_username.empty()); : status = JWTHelper::GetCustomClaimUsername( : decoded_token.get(), FLAGS_oauth_jwt_custom_claim_username, username); : } Same comment as line 831. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/runtime/exec-env.h File be/src/runtime/exec-env.h: http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/runtime/exec-env.h@110 PS8, Line 110: ~ExecEnv(); > Done. I renamed and relocated the getter as oauth_servers_mgr() alongside t Done http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/runtime/exec-env.h@299 PS8, Line 299: > Done. ExecEnv::oauth_servers_mgr_ is now a std::shared_ptr<OAuthServersMana Done http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/runtime/exec-env.cc@578 PS8, Line 578: > Done. I moved oauth_servers_mgr_ initialization into the ExecEnv constructo Done http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/service/impala-server.cc@3240 PS8, Line 3240: RETURN_IF_ERROR(ExecEnv::GetInstance()->oauth_servers_mgr()->Init()); : } : : // Initialize the client servers. > Done. I moved config construction into OAuthServersManager::Init() and now Done http://gerrit.cloudera.org:8080/#/c/24448/10/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/24448/10/be/src/service/impala-server.cc@3237 PS10, Line 3237: const bool require_oauth_servers = (FLAGS_jwt_token_auth && FLAGS_jwt_validate_signature) Need to: * also check FLAGS_oauth_token_auth * skip the check of OAuthServerConfig http://gerrit.cloudera.org:8080/#/c/24448/10/be/src/util/jwt-util.h File be/src/util/jwt-util.h: http://gerrit.cloudera.org:8080/#/c/24448/10/be/src/util/jwt-util.h@40 PS10, Line 40: JWTHelper(); : ~JWTHelper(); : JWTHelper(const JWTHelper&) = delete; : JWTHelper& operator=(const JWTHelper&) = delete; : JWTHelper(JWTHelper&&) noexcept; : JWTHelper& operator=(JWTHelper&&) noexcept; Why were these added? They shouldn't be necesary. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/jwt-util.cc File be/src/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/jwt-util.cc@771 PS8, Line 771: 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) { : > Done. I removed the warning/default fallback path and now rely on validated Done http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/jwt-util.cc@785 PS8, Line 785: if (!status.ok()) { : LOG(ERROR) << "Failed to load JWKS: " << status; : return status; : } : D > Done. I removed the warning/default fallback for update frequency and use r Done http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/jwt-util.cc@812 PS8, Line 812: new_jwks = std::make_shared<JWKSSnapshot>(); : bool is_changed = false; : Status status = : new_jwks->LoadKeysFromUrl(jwks_uri_, jwks_verify_server_certificate_, : jwks_ca_certificate_, current_jwks_checksum_, &is_changed, : jwks_pull_timeout_secs_); : if (!status.ok()) { > Done. JWKSMgr::Init() now resolves these values once and UpdateJWKSThread() Done http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/jwt-util.cc@869 PS8, Line 869: jwks_mgr_.reset(new JWKSMgr()); : RETURN_IF_ERROR(jwks_mgr_->Init(jwks_uri, jwks_verify_server_certificate, : jwks_ca_certificate, is_local_file, jwks_pull_timeout_secs, : jwks_update_frequency_secs)); : if (!initialized_) initialized_ = true; : return Status::OK(); : } : > Done. I removed CreateJWTHelper() / DestroyJWTHelper() and switched manager Done http://gerrit.cloudera.org:8080/#/c/24448/10/be/src/util/jwt-util.cc File be/src/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/24448/10/be/src/util/jwt-util.cc@803 PS10, Line 803: DCHECK_GT(jwks_update_frequency_secs_, 0); : DCHECK_GT(jwks_pull_timeout_secs_, 0); I believe the DCHECKS in JWKSMgr::Init take care of this validation which means these DCHECKS are duplicates that can be removed. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-server-config-test.cc File be/src/util/oauth-server-config-test.cc: http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-server-config-test.cc@164 PS8, Line 164: FLAGS_jwks_url = "https://example.com/jwks.json";; > Done. Added BuildOAuthServerConfigsCombinesJsonLegacyJwtAndLegacyOAuth. Done http://gerrit.cloudera.org:8080/#/c/24448/10/be/src/util/oauth-server-config.h File be/src/util/oauth-server-config.h: http://gerrit.cloudera.org:8080/#/c/24448/10/be/src/util/oauth-server-config.h@42 PS10, Line 42: /// Returns true if this server has a JWKS source configured. : bool HasJwksSource() const { : return !jwks_uri.empty(); : } This function should be deleted since it will never return `false` because the SetJwksSource() function in oauth-server-config.cc requires either the JWKS uri or path to be set. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-server-config.h File be/src/util/oauth-server-config.h: http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-server-config.h@17 PS8, Line 17: : #pragma once : > Done. Converted oauth-server-config.h to #pragma once. Done http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-server-config.h@37 PS8, Line 37: bool is_local_jwks = false; : int32_t jwks_pull_tim > Done. Replaced separate jwks_file_path / jwks_url with jwks_uri plus is_loc Done http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-server-config.h@35 PS8, Line 35: bool verify_server_cert = true; : std::string jwks_uri; : bool is_local_jwks = false; : int32_t jwks_pull_timeout_secs = DEFAULT_JWKS_PULL_TIMEOUT_SECS; : int32_t jwks_update_frequency_secs = DEFAULT_JWKS_UPDATE_FREQUENCY_SECS; : std::string username_claim = "username"; : > I did not convert OAuthServerConfig to fully constructor-only immutable fie Please convert OAuthServerConfig to constructor-only immutable fields. It's better to do it right the first time. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-server-config.h@48 PS8, Line 48: /// Builds the full list of OAuth server configs from --oauth_servers plus any legacy : /// jwks_* / oauth_* startup flags. : Status BuildOAuthServerConfigs(std::vector<OAuthServerConfig>* configs > Done. Legacy file-vs-url preference behavior is now contained in legacy bui Done http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-server-config.h@53 PS8, Line 53: : : > Done. I removed ParseOAuthServersJson() from the header and updated tests t Done http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-server-config.h@62 PS8, Line 62: > Done. Deprecated-flag warning helper path is now kept internal (anonymous n Done http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-server-config.cc File be/src/util/oauth-server-config.cc: http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-server-config.cc@31 PS8, Line 31: DECLARE_string(jwks_file_path); > Done. Removed redundant DECLARE_string(oauth_servers) Done http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager-test.cc File be/src/util/oauth-servers-manager-test.cc: http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager-test.cc@120 PS8, Line 120: google::FlagSaver flag_saver; > Done. Added VerifyFailsWhenOnlyWrongJwksConfigured. Done http://gerrit.cloudera.org:8080/#/c/24448/10/be/src/util/oauth-servers-manager-test.cc File be/src/util/oauth-servers-manager-test.cc: http://gerrit.cloudera.org:8080/#/c/24448/10/be/src/util/oauth-servers-manager-test.cc@55 PS10, Line 55: The VerifyFailsWhenNoServersConfigured() test from patchset 8 should also be included in this file. http://gerrit.cloudera.org:8080/#/c/24448/10/be/src/util/oauth-servers-manager.h File be/src/util/oauth-servers-manager.h: http://gerrit.cloudera.org:8080/#/c/24448/10/be/src/util/oauth-servers-manager.h@67 PS10, Line 67: std::shared_ptr<OAuthServersImpl> impl_; This should be a std::unique_ptr<OAuthServersImpl> since the impl_ class member variable is never directly returned. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.h File be/src/util/oauth-servers-manager.h: http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.h@17 PS8, Line 17: : #pragma once : > Done. Converted oauth-servers-manager.h to #pragma once. Done http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.h@40 PS8, Line 40: > Done. Removed require_jwks parameter from manager init and adjusted tests t Done http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.h@41 PS8, Line 41: } : : size_t size() const { : DCHECK(impl_); : return impl_->size(); > Done. Inlined one-line methods (empty(), size(), config()) in the header. Done http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.h@53 PS8, Line 53: /// Ver > Done. Converted GetUsername() from static helper to class member method. Done http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.cc File be/src/util/oauth-servers-manager.cc: http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.cc@35 PS8, Line 35: > Done. Manager now stores JWTHelper objects, not pointers. Done http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.cc@32 PS8, Line 32: RETURN_IF_ERROR(BuildOAuthServerConfigs(&configs)); : return InitWithConfigs(std::move(configs)); : } : : Status OAuthServersManager::InitWithConfigs(vector<OAuthServerConfig>&& configs) { : impl_ = std::make_shared<OAuthServersImpl>(); : impl_->reserve(configs.size()); : > Done (adapted with comment 18). I removed the extra struct and now use a pr Done http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.cc@41 PS8, Line 41: for (OAuthServerConfig& config : configs) { : if (!config.HasJwksSource()) con > Done (using shared ownership from related comments). I removed the construc Done http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.cc@50 PS8, Line 50: impl_->emplace_back(std::move(config), std::move(jwt_helper)); > Done. Removed manual reset/destructor cleanup path. Done http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.cc@55 PS8, Line 55: } > Done. Added DCHECK(impl_) checks in manager accessors/paths. Done http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.cc@59 PS8, Line 59: Status OAuthServersManager::Verify(const JWTHelper::JWTDecodedToken* decoded_token, > Done. Added corresponding DCHECK(impl_). Done http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.cc@63 PS8, Line 63: DCHECK(matched_server_idx_out != nullptr); > Done. Added corresponding DCHECK(impl_). Done http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.cc@88 PS8, Line 88: ername_cla > Done. Removed duplicate validation path and rely on HasJwksSource() flow. Done http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.cc@93 PS8, Line 93: > Done. Removed the test-only sleep from manager initialization. Done http://gerrit.cloudera.org:8080/#/c/24448/10/be/src/util/oauth-servers-manager.cc File be/src/util/oauth-servers-manager.cc: http://gerrit.cloudera.org:8080/#/c/24448/10/be/src/util/oauth-servers-manager.cc@36 PS10, Line 36: Status OAuthServersManager::InitWithConfigs(vector<OAuthServerConfig>&& configs) { This InitWithConfigs function is unnecessary since it is only called once. Move its code into OAuthServersManager::Init and remove the function. http://gerrit.cloudera.org:8080/#/c/24448/10/be/src/util/oauth-servers-manager.cc@64 PS10, Line 64: if (impl_->empty()) { : return Status("No OAuth servers are configured for token verification"); : } Is this check already performed elsewhere? http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/webserver.cc File be/src/util/webserver.cc: http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/webserver.cc@1167 PS8, Line 1167: > Done. Updated Webserver auth paths to use std::shared_ptr<OAuthServersManag Done http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/webserver.cc@1177 PS8, Line 1177: } : } > Done. Removed redundant matched_server_idx reset/condition in JWT web auth Done http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/webserver.cc@1219 PS8, Line 1219: : string username; > Done. Removed redundant matched_server_idx reset/condition in OAuth web aut Done http://gerrit.cloudera.org:8080/#/c/24448/10/be/src/util/webserver.cc File be/src/util/webserver.cc: http://gerrit.cloudera.org:8080/#/c/24448/10/be/src/util/webserver.cc@1181 PS10, Line 1181: if (FLAGS_jwt_validate_signature) { : status = oauth_servers_mgr->GetUsername(decoded_token.get(), matched_server_idx, : &username); : } else { : DCHECK(!FLAGS_jwt_custom_claim_username.empty()); : status = JWTHelper::GetCustomClaimUsername( : decoded_token.get(), FLAGS_jwt_custom_claim_username, username); : } Same comment as https://gerrit.cloudera.org/c/24448/10/be/src/rpc/authentication.cc#831. http://gerrit.cloudera.org:8080/#/c/24448/10/be/src/util/webserver.cc@1221 PS10, Line 1221: if (FLAGS_oauth_jwt_validate_signature) { : status = oauth_servers_mgr->GetUsername(decoded_token.get(), matched_server_idx, : &username); : } else { : DCHECK(!FLAGS_oauth_jwt_custom_claim_username.empty()); : status = JWTHelper::GetCustomClaimUsername( : decoded_token.get(), FLAGS_oauth_jwt_custom_claim_username, username); : } Same comment as https://gerrit.cloudera.org/c/24448/10/be/src/rpc/authentication.cc#831. http://gerrit.cloudera.org:8080/#/c/24448/10/tests/custom_cluster/test_shell_oauth_servers_auth.py File tests/custom_cluster/test_shell_oauth_servers_auth.py: http://gerrit.cloudera.org:8080/#/c/24448/10/tests/custom_cluster/test_shell_oauth_servers_auth.py@62 PS10, Line 62: def test_jwt_auth_with_oauth_servers(self, vector): Add a similar test that sets the legacy oauth_ flags. -- 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: 10 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: Thu, 18 Jun 2026 22:10:25 +0000 Gerrit-HasComments: Yes
