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 16: (12 comments) Overall looks very good! A few things to review. 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: sha512WithRSAEncryption => rs512 In the existing code, we support algorithms PS256, PS384, and PS512 but do not support RS224. We should not add RS224 given that it's weaker than supported today. Can we also add support for the three missing algorithms? https://github.com/apache/impala/blob/fdb87a755a2ac581b3144f1506b6e901ec8d2da2/be/src/util/jwt-util.cc#L323-L333 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: TEST(JwtUtilTest, VerifyJwtTokenWithx5cCertificate) { Some common code between these new tests. Please refactor it out into a separate function. http://gerrit.cloudera.org:8080/#/c/21382/14/be/src/util/jwt-util-test.cc@1264 PS14, Line 1264: .set_key_id("internal-gateway-jwt.api.sc.net") Use "kid_x5c" variable instead of string literal. http://gerrit.cloudera.org:8080/#/c/21382/14/be/src/util/jwt-util-test.cc@1268 PS14, Line 1268: .set_payload_claim("sample", jwt::claim(std::string{"test"})) Nit: unnecessary http://gerrit.cloudera.org:8080/#/c/21382/14/be/src/util/jwt-util-test.cc@1300 PS14, Line 1300: .set_key_id("internal-gateway-jwt.api.sc.net") Use "kid_x5c" variable instead of string literal. http://gerrit.cloudera.org:8080/#/c/21382/14/be/src/util/jwt-util-test.cc@1304 PS14, Line 1304: .set_payload_claim("sample", jwt::claim(std::string{"test"})) Nit: unnecessary 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: "\0" Nit: it's cleaner to not initialize the array members at all. Then, down in line 157, check values[0].empty() http://gerrit.cloudera.org:8080/#/c/21382/14/be/src/util/jwt-util.cc@153 PS14, Line 153: if (NameOfTypeOfJsonValue(json_value) == ARRAY_TYPE) { This condition needs to check that the key is "x5c" otherwise any array field will be picked up here. http://gerrit.cloudera.org:8080/#/c/21382/14/be/src/util/jwt-util.cc@248 PS14, Line 248: #define EXTRACT_ARRAY_VALUE(json_type, cpp_type) \ Even though this is following the existing pattern (which I'm generally in favor of doing), macros are highly discouraged in the C++ style guide. Thus, this is an opportunity to upgrade to a more supportable pattern. Please either make ValidateTypeAndExtractArrayValue its own function or else move this code into the ReadKeyArrayProperty function. 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(); According to the RFC, if the same data is represented in multiple ways, then the data must be the same: https://datatracker.ietf.org/doc/html/rfc7517#section-4.6 We should read the algorithm from the certificate if the "x5c" field is present and ignore the "alg" field. If the certificate does not have a signature, then error since the certificate is not valid. http://gerrit.cloudera.org:8080/#/c/21382/14/be/src/util/jwt-util.cc@390 PS14, Line 390: if (sigalg == "sha224WithRSAEncryption") { Shouldn't need to declare sslbuf, just directly construct sigalg from OBJ_nid2ln(pkey_nid) http://gerrit.cloudera.org:8080/#/c/21382/14/be/src/util/jwt-util.cc@393 PS14, Line 393: algorithm = "rs256"; We can drop the parsing of this algorithm since it's not handled in the if statement starting on line 419. Also, we do not want to add support for algorithms weaker than rs256. -- 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: 16 Gerrit-Owner: gaurav singh <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: gaurav singh <[email protected]> Gerrit-Comment-Date: Thu, 09 May 2024 19:34:39 +0000 Gerrit-HasComments: Yes
