Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/21382 )
Change subject: IMPALA-12559: Support x5c Parameter in JSON Web Keys ...................................................................... Patch Set 6: (11 comments) http://gerrit.cloudera.org:8080/#/c/21382/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21382/6//COMMIT_MSG@12 PS6, Line 12: supports a single x5c certificate What's the reason to support single x5c certificate? http://gerrit.cloudera.org:8080/#/c/21382/6//COMMIT_MSG@22 PS6, Line 22: verify jwt with x5c certificate. Could you add test case in fe/test/java/org/apache/impala/customcluster/JwtHttpTest.java? http://gerrit.cloudera.org:8080/#/c/21382/6/be/src/util/jwt-util.cc File be/src/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/21382/6/be/src/util/jwt-util.cc@51 PS6, Line 51: x5c certificate Is there hard limit for number of x5s certificate in RFC? http://gerrit.cloudera.org:8080/#/c/21382/6/be/src/util/jwt-util.cc@150 PS6, Line 150: "Array" nit: define a constant http://gerrit.cloudera.org:8080/#/c/21382/6/be/src/util/jwt-util.cc@200 PS6, Line 200: is_array this variable is unused http://gerrit.cloudera.org:8080/#/c/21382/6/be/src/util/jwt-util.cc@209 PS6, Line 209: nit: extra indent spaces http://gerrit.cloudera.org:8080/#/c/21382/6/be/src/util/jwt-util.cc@226 PS6, Line 226: nit: extra indent spaces http://gerrit.cloudera.org:8080/#/c/21382/6/be/src/util/jwt-util.cc@245 PS6, Line 245: A true return value function return Status, not boolean value http://gerrit.cloudera.org:8080/#/c/21382/6/be/src/util/jwt-util.cc@256 PS6, Line 256: for (size_t i = 0; i < json_value.Size() && i < MAX_X5C_CERTIFICATES; i++) { \ : value[i] = json_value[i].GetString(); \ : } nit: indent http://gerrit.cloudera.org:8080/#/c/21382/6/be/src/util/jwt-util.cc@323 PS6, Line 323: auto it_x5c = kv_map.find("x5c"); since 'x5c' has priority over 'k', could we check 'x5c' before checking 'k'? http://gerrit.cloudera.org:8080/#/c/21382/6/be/src/util/jwt-util.cc@385 PS6, Line 385: auto it_x5c = kv_map.find("x5c"); : if (it_x5c != kv_map.end()) : pub_key_str = jwt::helper::convert_base64_der_to_pem(it_x5c->second); : else : pub_key_str = pub_key; since 'x5c' has priority over 'n' and 'e', could we check 'x5c' first? same comments for other types of KeyBuilder. -- 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: 6 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: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Comment-Date: Wed, 01 May 2024 22:36:38 +0000 Gerrit-HasComments: Yes
