Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/21382 )
Change subject: IMPALA-12559: Support x5c Parameter for RSA JSON Web Keys ...................................................................... Patch Set 20: (15 comments) http://gerrit.cloudera.org:8080/#/c/21382/14//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21382/14//COMMIT_MSG@26 PS14, Line 26: > I printed the complete list of signatures but I could not locate the PS alg Done http://gerrit.cloudera.org:8080/#/c/21382/14/be/src/util/jwt-util-test.cc File be/src/util/jwt-util-test.cc: http://gerrit.cloudera.org:8080/#/c/21382/14/be/src/util/jwt-util-test.cc@1245 PS14, Line 1245: auto token = > Created a static function: CreateJwtX5cToken() with the common code. Done http://gerrit.cloudera.org:8080/#/c/21382/14/be/src/util/jwt-util-test.cc@1268 PS14, Line 1268: // Verify JWT token with x5c certificate. > Removed. Done http://gerrit.cloudera.org:8080/#/c/21382/14/be/src/util/jwt-util-test.cc@1304 PS14, Line 1304: } // namespace impala > Removed. Done http://gerrit.cloudera.org:8080/#/c/21382/20/be/src/util/jwt-util-test.cc File be/src/util/jwt-util-test.cc: http://gerrit.cloudera.org:8080/#/c/21382/20/be/src/util/jwt-util-test.cc@322 PS20, Line 322: kid_x5c I think the value of this constant still needs to be "internal-gateway-jwt.api.sc.net" since that is the value at https://gerrit.cloudera.org/c/21382/20/testdata/jwt/jwks_x5c_rs256.json#5 http://gerrit.cloudera.org:8080/#/c/21382/20/be/src/util/jwt-util-test.cc@460 PS20, Line 460: "kid_x5c" This code is using the string "kid_x5c" but should be using the variable kid_x5c (no double quotes). http://gerrit.cloudera.org:8080/#/c/21382/20/be/src/util/jwt-util-test.cc@1267 PS20, Line 1267: TEST(JwtUtilTest, VerifyJwtTokenWithx5cCertificate) { These two functions only really differ in the jwks_file creation. Please move all the duplicate lines into the static helper function. http://gerrit.cloudera.org:8080/#/c/21382/14/be/src/util/jwt-util.cc File be/src/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/21382/14/be/src/util/jwt-util.cc@148 PS14, Line 148: _key > Good point. Thanks. Done http://gerrit.cloudera.org:8080/#/c/21382/14/be/src/util/jwt-util.cc@153 PS14, Line 153: if (NameOfTypeOfJsonValue(json_value) == ARRAY_TYPE) { > True, but I think we should generic pick any other array claims as well for Done http://gerrit.cloudera.org:8080/#/c/21382/14/be/src/util/jwt-util.cc@375 PS14, Line 375: const char *data = pub_key_str.c_str(); > Since the x5c certificate mapping has limited values(4) and also we dont ha Sounds good. http://gerrit.cloudera.org:8080/#/c/21382/14/be/src/util/jwt-util.cc@390 PS14, Line 390: algorithm = "rs256"; > Done Done http://gerrit.cloudera.org:8080/#/c/21382/14/be/src/util/jwt-util.cc@393 PS14, Line 393: } else if (sigalg == "sha512WithRSAEncryption") { > Done Done http://gerrit.cloudera.org:8080/#/c/21382/20/be/src/util/jwt-util.cc File be/src/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/21382/20/be/src/util/jwt-util.cc@56 PS20, Line 56: #define MAX_X5C_CERTIFICATES 1 Nit: per the style guide, macros are discouraged. Use a constant here instead. http://gerrit.cloudera.org:8080/#/c/21382/20/be/src/util/jwt-util.cc@157 PS20, Line 157: if (!values[0].empty()) This statement needs an "else" clause. http://gerrit.cloudera.org:8080/#/c/21382/20/be/src/util/jwt-util.cc@395 PS20, Line 395: } If the certificate algorithm is not supported, then the users will get an error message that alg must be non-empty. An "else" clause here would be good to return a not supported error instead. -- To view, visit http://gerrit.cloudera.org:8080/21382 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I70be6f9f54190544aa005b2644e2ed8db6f6bb74 Gerrit-Change-Number: 21382 Gerrit-PatchSet: 20 Gerrit-Owner: gaurav singh <gsi...@cloudera.com> Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com> Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Reviewer: gaurav singh <gsi...@cloudera.com> Gerrit-Comment-Date: Fri, 10 May 2024 21:57:48 +0000 Gerrit-HasComments: Yes
