Wenzhe Zhou 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: (6 comments) 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@158 PS20, Line 158: v = values[0]; enclose this line with { } to follow coding style. http://gerrit.cloudera.org:8080/#/c/21382/20/be/src/util/jwt-util.cc@258 PS20, Line 258: GetString() Line 251 allow different json type, here assume it's string. Please keep consistent. http://gerrit.cloudera.org:8080/#/c/21382/20/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java File fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java: http://gerrit.cloudera.org:8080/#/c/21382/20/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@110 PS20, Line 110: JWKS_FILE_NAME This function currently use fixed json file. You may add new input parameter when using jwks_x5c_rs256.json http://gerrit.cloudera.org:8080/#/c/21382/20/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@575 PS20, Line 575: impaladJwtArgs, "", statestoreWebserverArgs, 0) add a new parameter to pass jwks_x5c_rs256.json http://gerrit.cloudera.org:8080/#/c/21382/20/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@600 PS20, Line 600: // Update JWKS in the root directory of Web server. : createTempJWKSInWebServerRootDir("jwks_x5c_rs256.json"); : // Sleep long enough for coordinator to update JWKS from Web server. : Thread.sleep(3000); : // Authenticate fails due JWT verification failure since the RS256 public key cannot : // be found in the JWKS. : transport.setCustomHeaders(headers); : try { : openResp = client.OpenSession(openReq); : } catch (Exception e) { : verifyJwtAuthMetrics(3, 1); : assertEquals(e.getMessage(), "HTTP Response code: 401"); : } This function block seems copying from testJwtAuthWithJwksHttpUrl(). In that function, we add a negative test case by changing json file with unexpected contents. Why do you callcreateTempJWKSInWebServerRootDir() with"jwks_x5c_rs256.json"? http://gerrit.cloudera.org:8080/#/c/21382/20/testdata/jwt/jwks_x5c_rs256.json File testdata/jwt/jwks_x5c_rs256.json: http://gerrit.cloudera.org:8080/#/c/21382/20/testdata/jwt/jwks_x5c_rs256.json@10 PS20, Line 10: "MIIE2jCCAsICAQ missing double quote in the end of line -- 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: Sat, 11 May 2024 00:46:22 +0000 Gerrit-HasComments: Yes
