Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/24472 )
Change subject: IMPALA-12232: Validate JWT aud/iss claims ...................................................................... Patch Set 4: (11 comments) 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: DEFINE_string(jwt_audience_claims, "", : "(DEPRECATED) Comma-separated list of allowed values for the JWT 'aud' claim. Use " : "--oauth_servers instead."); : DEFINE_string(jwt_issuer_claims, "", : "(DEPRECATED) Comma-separated list of allowed values for the JWT 'iss' claim. Use " : "--oauth_servers instead."); Let's not add new flags that are automatically deprecated. http://gerrit.cloudera.org:8080/#/c/24472/4/be/src/rpc/authentication.cc@276 PS4, Line 276: DEFINE_string(oauth_jwt_audience_claims, "", : "(DEPRECATED) Comma-separated list of allowed values for the OAuth token 'aud' " : "claim. Use --oauth_servers instead."); : DEFINE_string(oauth_jwt_issuer_claims, "", : "(DEPRECATED) Comma-separated list of allowed values for the OAuth token 'iss' " : "claim. Use --oauth_servers instead."); Let's not add new flags that are automatically deprecated. http://gerrit.cloudera.org:8080/#/c/24472/4/be/src/rpc/authentication.cc@316 PS4, Line 316: return Substitute("client ip: $0, username: $1, audience: $2, issuer: $3, kid: $4", Wrap all the placeholders with single quotes to make it clearer when values are missing. http://gerrit.cloudera.org:8080/#/c/24472/4/be/src/rpc/authentication.cc@919 PS4, Line 919: status = ExtractAuthenticatedUsername(oauth_servers_mgr, decoded_token, : matched_server_idx, FLAGS_jwt_custom_claim_username, &username); Don't extract the username twice. http://gerrit.cloudera.org:8080/#/c/24472/4/be/src/rpc/authentication.cc@999 PS4, Line 999: status = ExtractAuthenticatedUsername(oauth_servers_mgr, decoded_token, : matched_server_idx, FLAGS_oauth_jwt_custom_claim_username, &username); Don't extract the username twice. 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 for audit logging, or an empty string if : /// the claim is absent. : static std::string GetPayloadClaimForLog( : const JWTDecodedToken* decoded_token, const std::string& claim_name); Is there anything specific to logging in this function? If not, let's name it "GetPayloadClaim". http://gerrit.cloudera.org:8080/#/c/24472/4/be/src/util/jwt-util.h@95 PS4, Line 95: static std::string GetKeyIdForLog(const JWTDecodedToken* decoded_token); Is there anything specific to logging in this function? If not, let's name it "GetKeyId". 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: || c == '.'; TL;DR -- remove '=' as an allowed character. This list of allowed characters does not quite match the RFCs. The JWT RFC says each portion of a token is Base64-URL encoded (https://datatracker.ietf.org/doc/html/rfc7519#:~:text=A%20JWT%20is%20represented%20as%20a%20sequence%20of%20URL%2Dsafe%20parts%20separated%20by%0A%20%20%20period%20(%27.%27)%20characters.%20%20Each%20part%20contains%20a%20base64url%2Dencoded%0A%20%20%20value). That RFC points the JWS RFC which says that JWTs have the trailing equals sign(s) removed (https://datatracker.ietf.org/doc/html/rfc7515#:~:text=JWS)%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20May%202015-,Base64url%20Encoding,-Base64%20encoding%20using) and that the Base64-URL allowed characters are defined at https://datatracker.ietf.org/doc/html/rfc4648#section-5. The dot does have to be allowed as well since that is the separator charter used in a JWT. http://gerrit.cloudera.org:8080/#/c/24472/4/be/src/util/jwt-util.cc@992 PS4, Line 992: RETURN_IF_ERROR(VerifyClaim(decoded_token->decoded_jwt_, "iss", issuer_claims)); Nit: I would prefer if the "aud" and "iss" magic strings were pulled out as constants. http://gerrit.cloudera.org:8080/#/c/24472/4/be/src/util/jwt-util.cc@992 PS4, Line 992: RETURN_IF_ERROR(VerifyClaim(decoded_token->decoded_jwt_, "iss", issuer_claims)); Per the JWT RFC, "iss" must be a string while "aud" can be either a string or an array (https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.1). Don't allow "iss" to be an array. http://gerrit.cloudera.org:8080/#/c/24472/4/be/src/util/oauth-server-config-test.cc File be/src/util/oauth-server-config-test.cc: http://gerrit.cloudera.org:8080/#/c/24472/4/be/src/util/oauth-server-config-test.cc@47 PS4, Line 47: "issuerClaims": ["auth0"] Need to check audienceClaims too. -- 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: 4 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, 24 Jun 2026 18:34:46 +0000 Gerrit-HasComments: Yes
