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 12: (37 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: > Done 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, > Done. Removed JWTTokenAuth() and now both JWT and OAuth HTTP auth paths use Done 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( : > Done. Auth now consistently goes through oauth_servers_mgr. For JWT/OAuth, Done 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 " : > Addressed with the same fix as above. 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. 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. http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/rpc/authentication.cc@796 PS12, Line 796: bool OAuthTokenAuth(ThriftServer::ConnectionContext* connection_context, This function contains a lot of code in common with Webserver::OAuthTokenAuth(). Move the common code into a single function. http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/rpc/authentication.cc@797 PS12, Line 797: bool use_jwt_auth This function should not need to know whether it's handling JWT or OAuth. http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/rpc/authentication.cc@807 PS12, Line 807: LOG(ERROR) << "Error decoding " << token_kind << " token received from: " Make this message more generic by having it say something like, "Error decoding bearer token in Authorization header". http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/rpc/authentication.cc@819 PS12, Line 819: LOG(ERROR) << "Error verifying " << token_kind << " token received from: " : << TNetworkAddressToString(connection_context->network_address) : << " Error: " << status; Same comment as line 807. Possibly also include the token's JWKS and key id in this log message. http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/rpc/authentication.cc@833 PS12, Line 833: LOG(ERROR) << "Error extracting username from " << token_kind Same comment as line 807. http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/rpc/authentication.cc@843 PS12, Line 843: Substitute("Set-Cookie: $0", GenerateCookie(username, hash, auth_mech))); The call to GenerateCookie() should always pass HTTP_AUTH_MECH_OAUTH as the auth mech. 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; > Done. Startup now initializes oauth_servers_mgr when either JWT or OAuth to Done 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: const bool require_oauth_servers = FLAGS_jwt_token_auth || FLAGS_oauth_token_auth; Unnecessary variable declaration. 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 > Kept only what is needed for safe ownership/lifetime. The current JWTHelper 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: std::shared_ptr<JWKSSnapshot> new_jwks; : int64_t timeout_millis = jwks_update_f > Done. Removed the redundant DCHECK in JWKSMgr::UpdateJWKSThread() and rely Done 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 >= 0 : ? jwks_pull_timeout_secs : : FLAGS_jwks_pulling_timeout_s; : jwks_update_frequency_secs_ = jwks_update_frequency_secs >= 0 : ? jwks_update_frequency_secs : : FLAGS_jwks_update_frequency_s; Now that legacy JWT and OAuth flags are managed by OAuthServersManager, there is no need to check the jwks_pulling_timeout_s and jwks_update_frequency_secs flags. http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/jwt-util.cc@852 PS12, Line 852: JWTHelper::JWTHelper() = default; : JWTHelper::~JWTHelper() = default; Set the ctor/dtor to the default implementation in the header file and delete these lines. 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: std::string username_claim = "username"; I realize now that OAuthServerConfig needs to have a "validateTokenSignature" struct member. The Jira has been updated to reflect the addition of this new field. 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: return !FLAGS_jwks_file_path.empty() || !FLAGS_jwks_url.empty(); Need to check jwt_token_auth flag only. http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/oauth-server-config.cc@61 PS12, Line 61: return !FLAGS_oauth_jwks_file_path.empty() || !FLAGS_oauth_jwks_url.empty(); Need to check oauth_token_auth flag only. 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: > Done. Added VerifyFailsWhenNoServersConfigured() coverage. 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 = 0; : Status status = manager.Verify(decoded_token.get(), &matched_server_idx); Initialize matched_server_idx to -1 and then check that matched_server_idx is still set to -1 after the call to manager.Verify(). 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: /// index of the server that verified the token in 'matched_server_idx_out'. Please also note that on failure, the value of matched_server_idx_out is not modified. http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/oauth-servers-manager.h@66 PS12, Line 66: impl_ Nit: The variable name impl_ is not very descriptive. Please rename it to something plural like jwt_helpers_. 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: }; > Done. impl_ is now std::unique_ptr<OAuthServersImpl> 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: impl_ = std::make_unique<OAuthServersImpl>(); > Done. Inlined InitWithConfigs() logic into Init() and removed the extra met Done http://gerrit.cloudera.org:8080/#/c/24448/10/be/src/util/oauth-servers-manager.cc@64 PS10, Line 64: } : return last_error; : } > Yes. The validation is already enforced during config parsing/construction Done 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: auto jwt_helper = std::make_unique<JWTHelper>(); Why the switch to using a std::unique_ptr for the JWTHelper instance? The simpler implementation that used the JWTHelper instance directly should be sufficient. http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/oauth-servers-manager.cc@45 PS12, Line 45: impl_->emplace_back(std::move(config), std::move(jwt_helper)); There is no need to keep the OAuthServerConfig object around once the JWTHelper is constructed. http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/oauth-servers-manager.cc@57 PS12, Line 57: for (size_t i = 0; i < impl_->size(); ++i) { : Status status = impl_->at(i).second->Verify(decoded_token); : if (status.ok()) { : *matched_server_idx_out = i; : return Status::OK(); : } : last_error = status; : } Verifying decoded_token is a cryptographical operation and thus could potentially be heavy. Instead of verifying decoded_token against every JWTHelper, first locate the JWTHelper with the public key that matches decoded_token's public key/key type and verify against that. 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, > Done. Removed Webserver::JWTTokenAuth() and now both JWT/OAuth paths use We Done 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: bool use_jwt_auth) { This function should not need to know whether it's handling JWT or OAuth. http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/webserver.cc@1169 PS12, Line 1169: LOG(ERROR) << "Error decoding " << token_kind << " token in Authorization header, " Make this message more generic by having it say something like, "Error decoding bearer token in Authorization header". http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/webserver.cc@1180 PS12, Line 1180: LOG(ERROR) << "Error verifying " << token_kind : << " token in Authorization header, " : << "Error: " << status; Same comment as line 1169. Possibly also include the token's JWKS and key id in this log message. http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/webserver.cc@1191 PS12, Line 1191: LOG(ERROR) << "Cannot retrieve username from " << token_kind : << " token in Authorization header, " Same comment as line 1169. 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, > Done. Added legacy OAuth flags custom-cluster test coverage. Done -- 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: Fri, 26 Jun 2026 20:28:47 +0000 Gerrit-HasComments: Yes
