gaurav singh 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 19: (12 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: sha512WithRSAEncryption => rs512 > In the existing code, we support algorithms PS256, PS384, and PS512 but do I printed the complete list of signatures but I could not locate the PS algo class. Here's the list for reference. Digest: id-rsassa-pkcs1-v1_5-with-sha3-512 Digest: sha512WithRSAEncryption Digest: sha384WithRSAEncryption Digest: sha224WithRSAEncryption Digest: RSA-SHA512/224 Digest: sha512-224 Digest: md5WithRSAEncryption Digest: sm3 Digest: sha512 Digest: sha384 Digest: sha224 Digest: md4 Digest: blake2b512 Digest: RSA-SHA256 Digest: RSA-SHA224 Digest: ssl3-sha1 Digest: id-rsassa-pkcs1-v1_5-with-sha3-256 Digest: ripemd160 Digest: ssl3-md5 Digest: sha512-224WithRSAEncryption Digest: sha256 Digest: sha512-256 Digest: RSA-SM3 Digest: shake128 Digest: RSA-RIPEMD160 Digest: whirlpool Digest: id-rsassa-pkcs1-v1_5-with-sha3-384 Digest: RSA-SHA3-512 Digest: rmd160 Digest: RSA-SHA1 Digest: md5-sha1 Digest: RSA-SHA512/256 Digest: md4WithRSAEncryption Digest: sm3WithRSAEncryption Digest: blake2s256 Digest: sha3-256 Digest: RSA-MD5 Digest: RSA-SHA512 Digest: shake256 Digest: ripemd Digest: RSA-SHA1-2 Digest: RSA-SHA384 Digest: RSA-SHA3-256 Digest: sha512-256WithRSAEncryption Digest: sha3-224 Digest: id-rsassa-pkcs1-v1_5-with-sha3-224 Digest: sha3-512 Digest: sha3-384 Digest: md5 Digest: ripemd160WithRSA Digest: sha256WithRSAEncryption Digest: RSA-SHA3-224 Digest: sha1 Digest: sha1WithRSAEncryption Digest: RSA-SHA3-384 Digest: RSA-MD4 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 = > Some common code between these new tests. Please refactor it out into a se Created a static function: CreateJwtX5cToken() with the common code. http://gerrit.cloudera.org:8080/#/c/21382/14/be/src/util/jwt-util-test.cc@1264 PS14, Line 1264: << " Actual error: " << status.GetDetail(); > Use "kid_x5c" variable instead of string literal. 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. > Nit: unnecessary Removed. http://gerrit.cloudera.org:8080/#/c/21382/14/be/src/util/jwt-util-test.cc@1300 PS14, Line 1300: status = jwt_helper.Verify(decoded_token.get()); > Use "kid_x5c" variable instead of string literal. Done http://gerrit.cloudera.org:8080/#/c/21382/14/be/src/util/jwt-util-test.cc@1304 PS14, Line 1304: } // namespace impala > Nit: unnecessary Removed. 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 > Nit: it's cleaner to not initialize the array members at all. Then, down i Good point. Thanks. 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 fie True, but I think we should generic pick any other array claims as well for future work or changes in the RFC spec. 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, the Since the x5c certificate mapping has limited values(4) and also we dont have coverage for all RSA types(ps256, ps384, ps512 etc), I think we should prioritize "alg" over the derived value from certificate ? Also, the certificate validity should be the liability of the ID provider (aws/azure etc). Thoughts ? http://gerrit.cloudera.org:8080/#/c/21382/14/be/src/util/jwt-util.cc@390 PS14, Line 390: algorithm = "rs256"; > Shouldn't need to declare sslbuf, just directly construct sigalg from OBJ_n Done http://gerrit.cloudera.org:8080/#/c/21382/14/be/src/util/jwt-util.cc@393 PS14, Line 393: } else if (sigalg == "sha512WithRSAEncryption") { > We can drop the parsing of this algorithm since it's not handled in the if Done http://gerrit.cloudera.org:8080/#/c/21382/18/be/src/util/jwt-util.cc File be/src/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/21382/18/be/src/util/jwt-util.cc@157 PS18, Line 157: if (!values[0].empty()) > Need an "else" clause for this statement. Currently, if the statement eval Moved the string k, v, values[MAX_X5C_CERTIFICATES] inside the for loop for better context and scope. -- 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: 19 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: Fri, 10 May 2024 21:15:48 +0000 Gerrit-HasComments: Yes
