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 10: (33 comments) 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(); > Nit: The original names GetJWTHelperInstance() and GetOAuthHelperInstance() Done. I renamed and relocated the getter as oauth_servers_mgr() alongside the other getters and changed the return type to std::shared_ptr<OAuthServersManager> http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/runtime/exec-env.h@299 PS8, Line 299: > Make this a std::shared_ptr to avoid use-after-free issues. Done. ExecEnv::oauth_servers_mgr_ is now a std::shared_ptr<OAuthServersManager> 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: > oauth_servers_mgr_ should be initialized as part of the constructor definit Done. I moved oauth_servers_mgr_ initialization into the ExecEnv constructor initializer list and removed the allocation from ExecEnv::Init() 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. > This coding structure is sub-optimal since it requires initializing a throw Done. I moved config construction into OAuthServersManager::Init() and now call ExecEnv::GetInstance()->oauth_servers_mgr()->Init() directly from ImpalaServer::Start(). Configs are moved into manager storage. 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) { : > The flag validators for FLAGS_jwks_pulling_timeout_s take care of ensuring Done. I removed the warning/default fallback path and now rely on validated values; the code path now uses resolved values with DCHECKs. 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 > The flag validators for FLAGS_jwks_update_frequency_s take care of ensuring Done. I removed the warning/default fallback for update frequency and use resolved validated values directly. 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()) { > The logic in JWKSMgr::Init should take care of ensuring jwks_update_frequen Done. JWKSMgr::Init() now resolves these values once and UpdateJWKSThread() uses those resolved members with DCHECKs. 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(); : } : > These functions are not needed. Done. I removed CreateJWTHelper() / DestroyJWTHelper() and switched manager storage/ownership accordingly. 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";; > Please add another test that combines all three sets of flags (oauth_server Done. Added BuildOAuthServerConfigsCombinesJsonLegacyJwtAndLegacyOAuth. 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 : > Use the newer #pragma once directive instead of macro guards. Done. Converted oauth-server-config.h to #pragma once. 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 > Instead of having separate fields for jwks file path and url, please consid Done. Replaced separate jwks_file_path / jwks_url with jwks_uri plus is_local_jwks. 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"; : > Since this is security related, I would prefer for all these fields to be d I did not convert OAuthServerConfig to fully constructor-only immutable fields in this patch because it would require a larger parser/build refactor across legacy migration and tests. I can do that in a follow-up if you want this tightened further. 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 > Since defining both the JWKS file path and url is legacy behavior, let's ke Done. Legacy file-vs-url preference behavior is now contained in legacy builders; the struct no longer carries both fields. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-server-config.h@53 PS8, Line 53: : : > It's a design smell if a function is exposed only for testing purposes. C+ Done. I removed ParseOAuthServersJson() from the header and updated tests to validate through BuildOAuthServerConfigs(). http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-server-config.h@62 PS8, Line 62: > I don't think this function needs to be in the global namespace but rather Done. Deprecated-flag warning helper path is now kept internal (anonymous namespace scope). 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); > Remove, no need to DECLARE_ in the same file where the flag is defined. Done. Removed redundant DECLARE_string(oauth_servers) 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; > Please add a test that asserts token validation fails when only the testdat Done. Added VerifyFailsWhenOnlyWrongJwksConfigured. 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 : > Use the newer #pragma once directive instead of macro guards. Done. Converted oauth-servers-manager.h to #pragma once. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.h@40 PS8, Line 40: > Remove this function parameter since it is `true` in all the main code wher Done. Removed require_jwks parameter from manager init and adjusted tests to use real config-driven init. 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(); > Nit: these simple one-line functions can have their function bodies placed Done. Inlined one-line methods (empty(), size(), config()) in the header. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.h@53 PS8, Line 53: /// Ver > Why is this function declared static instead of being a class member functi Done. Converted GetUsername() from static helper to class member method. 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: > No need to use JWTHelper*, use JWTHelper instead. Done. Manager now stores JWTHelper objects, not pointers. 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()); : > This struct is not necessary. Instead, in the private section of the OAuth Done (adapted with comment 18). I removed the extra struct and now use a private alias in the header: std::vector<std::pair<OAuthServerConfig, JWTHelper>>. 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 > Delete this constructor implementation, and, in the Init() function, do thi Done (using shared ownership from related comments). I removed the constructor-based init path and initialize impl_ inside manager init flow. 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)); > No need to reset impl_, it will be automatically reset when it goes out of Done. Removed manual reset/destructor cleanup path. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.cc@55 PS8, Line 55: } > Add DCHECK(impl_) to ensure that Init() has been called. Done. Added DCHECK(impl_) checks in manager accessors/paths. 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, > Add DCHECK(impl_) to ensure that Init() has been called. Done. Added corresponding DCHECK(impl_). 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); > Add DCHECK(impl_) to ensure that Init() has been called. Done. Added corresponding DCHECK(impl_). http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.cc@88 PS8, Line 88: ername_cla > This function duplicates the validation performed in HasJwksSource(). Done. Removed duplicate validation path and rely on HasJwksSource() flow. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/oauth-servers-manager.cc@93 PS8, Line 93: > Is this sleep necessary? If so, add a comment explaining why or remove the Done. Removed the test-only sleep from manager initialization. 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: > Needs to be changed to std::shared_ptr<OAuthServersManager> Done. Updated Webserver auth paths to use std::shared_ptr<OAuthServersManager> from ExecEnv. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/webserver.cc@1177 PS8, Line 1177: } : } > Useless condition, matched_server_idx will already be 0 here. Done. Removed redundant matched_server_idx reset/condition in JWT web auth path. http://gerrit.cloudera.org:8080/#/c/24448/8/be/src/util/webserver.cc@1219 PS8, Line 1219: : string username; > Useless condition, matched_server_idx will already be 0 here. Done. Removed redundant matched_server_idx reset/condition in OAuth web auth path. -- 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 09:36:26 +0000 Gerrit-HasComments: Yes
