Anubhav Jindal has posted comments on this change. ( http://gerrit.cloudera.org:8080/24472 )
Change subject: IMPALA-12232: Validate JWT aud/iss claims ...................................................................... Patch Set 5: (10 comments) Done http://gerrit.cloudera.org:8080/#/c/24472/4/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/24472/4/be/src/rpc/authentication.cc@221 PS4, Line 221: // If set, Impala allows JWT authentication on unsecure channel. : // JWT is only secure when used with TLS. But in some deployment scenarios, TLS is handled : // by proxy so that it does not show up as TLS to Impala. : DEFINE_bool_hidden(jwt_allow_without_tls, false, : "When this configuration is set to true, Impala allows JWT authentication on " : "unsecure channel. This shou > Let's not add new flags that are automatically deprecated. Done. I removed the newly introduced deprecated flags --jwt_audience_claims and --jwt_issuer_claims, and removed all related parsing/migration wiring. http://gerrit.cloudera.org:8080/#/c/24472/4/be/src/rpc/authentication.cc@276 PS4, Line 276: "TLS is handled by proxy."); : // End OAuth : : DEFINE_bool(enable_group_filter_check_for_authenticated_kerberos_user, false, : "If this configuration is set to true, Impala checks the provided " : "LDAP group filter, if any, with the au > Let's not add new flags that are automatically deprecated. Done. I removed the newly introduced deprecated flags --oauth_jwt_audience_claims and --oauth_jwt_issuer_claims, and removed all related parsing/migration wiring. http://gerrit.cloudera.org:8080/#/c/24472/4/be/src/rpc/authentication.cc@316 PS4, Line 316: *matched_server_idx_out = 0; > Wrap all the placeholders with single quotes to make it clearer when values Done. Updated JWT/OAuth auth log context formatting to quote all placeholders. http://gerrit.cloudera.org:8080/#/c/24472/4/be/src/rpc/authentication.cc@919 PS4, Line 919: LOG(INFO) << "JWT token authentication succeeded: " : << JwtLogContext(client_ip, decoded_token, username); > Don't extract the username twice. Done. In JWT auth flow, username is now extracted once and reused for claim-failure logging and success assignment. http://gerrit.cloudera.org:8080/#/c/24472/4/be/src/rpc/authentication.cc@999 PS4, Line 999: } : > Don't extract the username twice. Done. In OAuth auth flow, username is now extracted once and reused for claim-failure logging and success assignment. http://gerrit.cloudera.org:8080/#/c/24472/4/be/src/util/jwt-util.h File be/src/util/jwt-util.h: http://gerrit.cloudera.org:8080/#/c/24472/4/be/src/util/jwt-util.h@88 PS4, Line 88: /// Returns the JSON value of a payload claim, or an empty string if the claim is : /// absent. : static std::string GetPayloadClaim( : const JWTDecodedToken* decoded_token, const std::string& claim_name); > Is there anything specific to logging in this function? If not, let's name Done. Renamed GetPayloadClaimForLog() to GetPayloadClaim() and updated callers. http://gerrit.cloudera.org:8080/#/c/24472/4/be/src/util/jwt-util.h@95 PS4, Line 95: > Is there anything specific to logging in this function? If not, let's name Done. Renamed GetKeyIdForLog() to GetKeyId() and updated callers. http://gerrit.cloudera.org:8080/#/c/24472/4/be/src/util/jwt-util.cc File be/src/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/24472/4/be/src/util/jwt-util.cc@70 PS4, Line 70: } > TL;DR -- remove '=' as an allowed character. Done. Removed '=' from ValidateTokenFormat() allowed characters while keeping '.' as the JWT segment separator. Added unit coverage for rejecting a token containing = http://gerrit.cloudera.org:8080/#/c/24472/4/be/src/util/jwt-util.cc@992 PS4, Line 992: DCHECK(decoded_token != nullptr); > Per the JWT RFC, "iss" must be a string while "aud" can be either a string Done. Claim verification now allows array values for aud only, iss is restricted to string values. Added a unit test that verifies array-form iss is rejected. http://gerrit.cloudera.org:8080/#/c/24472/4/be/src/util/jwt-util.cc@992 PS4, Line 992: DCHECK(decoded_token != nullptr); > Nit: I would prefer if the "aud" and "iss" magic strings were pulled out as Done. Introduced constants for aud and iss and switched claim verification to use them. -- To view, visit http://gerrit.cloudera.org:8080/24472 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0a00b126359f2bc7e2f73d894cebc2b9014c7375 Gerrit-Change-Number: 24472 Gerrit-PatchSet: 5 Gerrit-Owner: Anubhav Jindal <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Anubhav Jindal <[email protected]> Gerrit-Reviewer: Gokul Kolady <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Comment-Date: Wed, 08 Jul 2026 21:33:58 +0000 Gerrit-HasComments: Yes
