Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/17663 )
Change subject: IMPALA-10489 part2: Support RSASSA-PSS and EC Algorithms for JWT ...................................................................... Patch Set 1: (2 comments) Thank you for this follow-on change. I only have minor nits. http://gerrit.cloudera.org:8080/#/c/17663/1/be/src/util/jwt-util-test.cc File be/src/util/jwt-util-test.cc: http://gerrit.cloudera.org:8080/#/c/17663/1/be/src/util/jwt-util-test.cc@634 PS1, Line 634: TempTestDataFile jwks_file(Substitute(jwks_rsa_file_format, kid_1, "PS384", : rsa_pub_key_jwk_n, rsa_pub_key_jwk_e, kid_2, "PS384", rsa_invalid_pub_key_jwk_n, : rsa_pub_key_jwk_e)); I'm seeing that PS256/PS384/PS512 use the same RSA public key to initialize the JWKS file. What are the rules around that? Would we get any testing value from having different keys for each size? http://gerrit.cloudera.org:8080/#/c/17663/1/be/src/util/jwt-util.cc File be/src/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/17663/1/be/src/util/jwt-util.cc@313 PS1, Line 313: if (algorithm.compare("rs256") == 0) { : jwt_pub_key = new RS256JWTPublicKey(algorithm, pub_key); : } else if (algorithm.compare("rs384") == 0) { : jwt_pub_key = new RS384JWTPublicKey(algorithm, pub_key); : } else if (algorithm.compare("rs512") == 0) { : jwt_pub_key = new RS512JWTPublicKey(algorithm, pub_key); : } else if (algorithm.compare("ps256") == 0) { : jwt_pub_key = new PS256JWTPublicKey(algorithm, pub_key); : } else if (algorithm.compare("ps384") == 0) { : jwt_pub_key = new PS384JWTPublicKey(algorithm, pub_key); : } else if (algorithm.compare("ps512") == 0) { : jwt_pub_key = new PS512JWTPublicKey(algorithm, pub_key); Nit: Very minor thing, but the C++ std library implements the "==" relational operator for strings. For exact matching, you can do "str1 == str2" directly. (compare() could still be useful for substring matching.) https://www.cplusplus.com/reference/string/string/operators/ You also have the option of using a switch statement like: switch (algorithm) { case "rs256": etc etc break; ... default: return Status(etc); } I'm ok with either, but I would prefer one of these as opposed to compare() across these files. (But this is minor and I'm open to disagreement.) -- To view, visit http://gerrit.cloudera.org:8080/17663 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib4dc30c51f503b609dd311ce4387080abc5a0832 Gerrit-Change-Number: 17663 Gerrit-PatchSet: 1 Gerrit-Owner: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Comment-Date: Tue, 13 Jul 2021 23:27:42 +0000 Gerrit-HasComments: Yes
