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 13: (36 comments) http://gerrit.cloudera.org:8080/#/c/24448/13//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24448/13//COMMIT_MSG@13 PS13, Line 13: Address follow-up review feedback by adding Nit: no need to mention addressing review feedback unless its feedback from a prior change. 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, > Done. Marked --jwt_token_auth as deprecated and updated the help text to di Done 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, > Done. Marked --jwt_allow_without_tls as deprecated and updated the help tex Done http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/rpc/authentication.cc@796 PS12, Line 796: const AuthenticationHash& hash, const string& token) { > Done. Moved common bearer-token decode/verify/username logic into OAuthServ Done http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/rpc/authentication.cc@807 PS12, Line 807: connection_context->return_headers.push_back( > Done. Updated RPC logging to use generic bearer-token validation errors ins Done 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 > Done for the generic wording. I kept logs metadata-light (no JWKS/kid) to a Done http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/rpc/authentication.cc@833 PS12, Line 833: } > Done. Updated this RPC error path to the same generic bearer-token validati Done http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/rpc/authentication.cc@843 PS12, Line 843: if (spnego_status.ok()) { > Done. Updated cookie generation to always pass HTTP_AUTH_MECH_OAUTH. Done http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/rpc/authentication.cc@190 PS13, Line 190: DEFINE_bool(jwt_validate_signature, true, This flag is dangerous and doesn't make a lot of sense anymore now that multiple oauth servers can be specified. Delete it entirely. The JwtHttpTest#testJwtAuthNotVerifySig test will need to be updated since that is the only place where this flag is set to false. http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/rpc/authentication.cc@237 PS13, Line 237: DEFINE_bool(oauth_jwt_validate_signature, true, This flag is dangerous and doesn't make a lot of sense anymore now that multiple oauth servers can be specified. Delete it entirely. http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/rpc/authentication.cc@802 PS13, Line 802: LOG(ERROR) << "Error validating bearer token in Authorization header received from: " Let's also include the username in this error message if the username is not empty. 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) { > Done. Removed the temporary boolean and inlined the condition directly. Done http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/jwt-util.h File be/src/util/jwt-util.h: http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/jwt-util.h@100 PS13, Line 100: std::shared_ptr<JWKSMgr> jwks_mgr_; This is fine to leave as a std::unique_ptr since jwks_mgr_ is not shared outside of this class. 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) { > Done. Removed fallback usage of those legacy flags in JWKSMgr::Init() and n Done 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 > Done. Defaulted JWTHelper ctor/dtor in the header and removed explicit ctor Done http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/jwt-util.cc File be/src/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/jwt-util.cc@988 PS13, Line 988: if (jwks_mgr_ == nullptr) return Status::OK(); Add DCHECK(jwks_mgr_ != nullptr) to ensure that Init() was already called. http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/jwt-util.cc@991 PS13, Line 991: if (jwks->IsEmpty()) { Should this be an error condition if the jwks is empty? Possibly the init code guards against this situation happening? http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/jwt-util.cc@995 PS13, Line 995: if (!decoded_token->decoded_jwt_.has_key_id()) return Status::OK(); The "alg" token header parameter is required. First check if the jwks has any keys with an algorithm that matches the algorithm of the decoded jwt. http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/oauth-server-config.h File be/src/util/oauth-server-config.h: http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/oauth-server-config.h@38 PS13, Line 38: bool validate_token_signature = true; Delete this struct member. Token signatures must always be validated. 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; > Done. Added validate_token_signature to OAuthServerConfig, parse support fo Done 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 { > Done. No need to check FLAGS_jwks_file_path or FLAGS_jwks_url. http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/oauth-server-config.cc@61 PS12, Line 61: bool IsLegacyJwtConfigFlagSpecified() { > Done. No need to check FLAGS_oauth_jwks_file_path or FLAGS_oauth_jwks_url. 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); > Done. Updated OAuthServersManagerTest.VerifyFailsWhenOnlyWrongJwksConfigure Done http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/oauth-servers-manager.h File be/src/util/oauth-servers-manager.h: http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/oauth-servers-manager.h@56 PS13, Line 56: /// Extracts the username using the username claim from the server at 'server_idx'. : Status GetUsername(const JWTHelper::JWTDecodedToken* decoded_token, size_t server_idx, : std::string* username_out) const; Move this function declaration down into the private section. 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; > Done. Updated API contract comment and behavior to preserve matched_server_ Done http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/oauth-servers-manager.h@66 PS12, Line 66: > Done. Renamed internal member from impl_ to jwt_helpers_. 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@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 > Done. Added key preselection via JWTHelper::CanVerify() (kid/algorithm-fami Done http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/oauth-servers-manager.cc File be/src/util/oauth-servers-manager.cc: http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/oauth-servers-manager.cc@59 PS13, Line 59: RETURN_IF_ERROR(GetUsername(decoded_token.get(), matched_server_idx, username_out)); Let's get the username first so that output function parameter will be set if verification fails. http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/oauth-servers-manager.cc@69 PS13, Line 69: DCHECK(!jwt_helpers_->empty()); Put the two jwt_helpers_ DCHECKS next to each other. http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/oauth-servers-manager.cc@75 PS13, Line 75: if (!verifier.validate_token_signature) { This condition will authorize every token if any of the verifies have validate_token_signature set to false. Instead, the token signature validation should only be skipped after the jwt_helper that would have verified the token has been located. In other words, delete this entire "if" code black and skip the call to verifier.jwt_helper.Verify if verifier.validate_token_signature is false. 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; > Done. Removed JWT/OAuth mode branching from webserver bearer auth and switc Done http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/webserver.cc@1169 PS12, Line 1169: return false; > Done. Updated webserver logs to generic bearer-token validation error wordi Done http://gerrit.cloudera.org:8080/#/c/24448/12/be/src/util/webserver.cc@1180 PS12, Line 1180: return Status::Expected("No Authorization header provided."); : } : > Done for generic wording. I intentionally left out JWKS/kid details to avoi Done 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; > Done. Updated this webserver error path to the same generic bearer-token va Done http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/webserver.cc File be/src/util/webserver.cc: http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/webserver.cc@1160 PS13, Line 1160: (void)connection; Delete this line. If possible, remove the struct sq_connection* connection function parameter too. http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/webserver.cc@1166 PS13, Line 1166: LOG(ERROR) << "Error validating bearer token in Authorization header from " Need to also set the "WWW-Authenticate" http response header per https://datatracker.ietf.org/doc/html/rfc6750#section-3. -- 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: Tue, 14 Jul 2026 19:05:44 +0000 Gerrit-HasComments: Yes
