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 14: (15 comments) Done 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: Remove deprecated JWT/OAuth signature-toggl > Nit: no need to mention addressing review feedback unless its feedback from 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: "(DEPRECATED) File path of the pre-installed JSON Web Key Set (JWKS) for JWT " > This flag is dangerous and doesn't make a lot of sense anymore now that mul Done. I removed --jwt_validate_signature entirely and updated JwtHttpTest so it no longer uses that flag. http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/rpc/authentication.cc@237 PS13, Line 237: // Enables retrieving the OAuth JWKS from the specified URL without verifying the > This flag is dangerous and doesn't make a lot of sense anymore now that mul Done. I removed --oauth_jwt_validate_signature entirely. http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/rpc/authentication.cc@802 PS13, Line 802: } > Let's also include the username in this error message if the username is no Done. The error log now appends username when one was extracted. 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::unique_ptr<JWKSMgr> jwks_mgr_; > This is fine to leave as a std::unique_ptr since jwks_mgr_ is not shared ou Done. jwks_mgr_ is now std::unique_ptr<JWKSMgr> again. 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: Status JWTHelper::CanVerify( > Add DCHECK(jwks_mgr_ != nullptr) to ensure that Init() was already called. 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: DCHECK(decoded_token != nullptr); > Should this be an error condition if the jwks is empty? Possibly the init I kept this as a non-error in CanVerify(). An empty JWKS is treated as “cannot verify with this helper” so multi-server matching can continue, the overall verify path still returns a hard failure (Verification failed, no matching key) if no helper can verify. http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/jwt-util.cc@995 PS13, Line 995: > The "alg" token header parameter is required. First check if the jwks has Done. CanVerify() now requires a non-empty alg, validates algorithm family first, and only considers helpers with matching JWKS key families. 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: int32_t jwks_pull_timeout_secs = DEFAULT_JWKS_PULL_TIMEOUT_SECS; > Delete this struct member. Token signatures must always be validated. Done. Removed validate_token_signature from OAuthServerConfig and removed all related parsing/mapping logic. 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: private: : struct OAuthServerVerifier { : JWTHelper jwt_helper; > Move this function declaration down into the private section. 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: bool found_username_server = false; > Let's get the username first so that output function parameter will be set Done http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/oauth-servers-manager.cc@69 PS13, Line 69: } > Put the two jwt_helpers_ DCHECKS next to each other. Done. Grouped DCHECK(jwt_helpers_) and DCHECK(!jwt_helpers_->empty()) together. http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/oauth-servers-manager.cc@75 PS13, Line 75: > This condition will authorize every token if any of the verifies have valid Done. I removed that bypass path entirely by removing validate_token_signature support and requiring signature verification for matched helpers. 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: } > Delete this line. If possible, remove the struct sq_connection* connection Done. Removed the unused line and removed the sq_connection* parameter from Webserver::OAuthTokenAuth(). http://gerrit.cloudera.org:8080/#/c/24448/13/be/src/util/webserver.cc@1166 PS13, Line 1166: DCHECK(response_headers != nullptr); > Need to also set the "WWW-Authenticate" http response header per https://da Done. Added WWW-Authenticate: Bearer error="invalid_token", error_description="..." on bearer-token auth failures in webserver auth flow. -- 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: 14 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, 15 Jul 2026 23:33:38 +0000 Gerrit-HasComments: Yes
