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 12: (16 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: > Please don't add the "Co-authored-by" trailer since Impala uses "Assisted-b Done 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@796 PS10, Line 796: bool OAuthTokenAuth(ThriftServer::ConnectionContext* connection_context, > Now that all JWT/OAuth configs (including legacy configs) are in one place Done. Removed JWTTokenAuth() and now both JWT and OAuth HTTP auth paths use OAuthTokenAuth(..., use_jwt_auth) http://gerrit.cloudera.org:8080/#/c/24448/10/be/src/rpc/authentication.cc@824 PS10, Line 824: error_description=\"$0 \"", status.GetDetail())); : return false; : } : } : : string username; : status = oauth_servers_mgr->GetUsername( : > This block of code contains an abstraction leak that is not the best approa Done. Auth now consistently goes through oauth_servers_mgr. For JWT/OAuth, verification is conditional on the corresponding signature-validation flag, while decode + username extraction continue through manager-backed config. http://gerrit.cloudera.org:8080/#/c/24448/10/be/src/rpc/authentication.cc@874 PS10, Line 874: if (*is_complete) { : if (username.empty()) { : spnego_status = kudu::Status::RuntimeError( : "SPNEGO indicated complete, but got empty principal"); : // Crash in debug builds, but fall through to treating as an error in release. : LOG(DFATAL) << "Got no authenticated principal for SPNEGO-authenticated " : << " connection from " : > Same comment as line 831. Addressed with the same fix as above. 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_oauth_token_auth; > Need to: Done. Startup now initializes oauth_servers_mgr when either JWT or OAuth token auth is enabled, without the extra pre-check. 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; : : /// Opaque types for storing the JWT decode > Why were these added? They shouldn't be necesary. Kept only what is needed for safe ownership/lifetime. The current JWTHelper declarations are intentionally minimal for non-copyable behavior and correct destruction with forward-declared internals. 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: std::shared_ptr<JWKSSnapshot> new_jwks; : int64_t timeout_millis = jwks_update_f > I believe the DCHECKS in JWKSMgr::Init take care of this validation which m Done. Removed the redundant DCHECK in JWKSMgr::UpdateJWKSThread() and rely on initialization-time validation. 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: : /// Builds the full list of OAuth server configs from --oauth_servers plus any legacy : /// jwks_* / oauth_* startup flags. : Sta > This function should be deleted since it will never return `false` because Done. Removed the redundant HasJwksSource() exposure; source validation is enforced during config construction. 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 b Done. Added VerifyFailsWhenNoServersConfigured() coverage. 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: }; > This should be a std::unique_ptr<OAuthServersImpl> since the impl_ class me Done. impl_ is now std::unique_ptr<OAuthServersImpl> 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: impl_ = std::make_unique<OAuthServersImpl>(); > This InitWithConfigs function is unnecessary since it is only called once. Done. Inlined InitWithConfigs() logic into Init() and removed the extra method. http://gerrit.cloudera.org:8080/#/c/24448/10/be/src/util/oauth-servers-manager.cc@64 PS10, Line 64: } : return last_error; : } > Is this check already performed elsewhere? Yes. The validation is already enforced during config parsing/construction and init flow, redundant check removed. 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@1158 PS10, Line 1158: bool Webserver::OAuthTokenAuth(const std::string& token, > Now that all JWT/OAuth configs (including legacy configs) are in one place Done. Removed Webserver::JWTTokenAuth() and now both JWT/OAuth paths use Webserver::OAuthTokenAuth(..., use_jwt_auth) http://gerrit.cloudera.org:8080/#/c/24448/10/be/src/util/webserver.cc@1181 PS10, Line 1181: << " token in Authorization header, " : << "Error: " << status; : return false; : } : } : : string username; : s > Same comment as https://gerrit.cloudera.org/c/24448/10/be/src/rpc/authentic Addressed with the same manager-centric auth flow update. http://gerrit.cloudera.org:8080/#/c/24448/10/be/src/util/webserver.cc@1221 PS10, Line 1221: request_info->remote_user = strdup(username.c_str()); : return Status::OK(); : } : : return Status::Expected("Failed to authenticate to LDAP."); : } : : voi > Same comment as https://gerrit.cloudera.org/c/24448/10/be/src/rpc/authentic Addressed with the same manager-centric auth flow update. 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: impalad_args=JWT_IMPALAD_ARGS, > Add a similar test that sets the legacy oauth_ flags. Done. Added legacy OAuth flags custom-cluster test coverage. -- 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: 12 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: Wed, 24 Jun 2026 23:15:28 +0000 Gerrit-HasComments: Yes
