Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/19503 )
Change subject: IMPALA-11922 Verify JWKS URL server TLS certificate by default. ...................................................................... Patch Set 6: (11 comments) Mostly comments about comments http://gerrit.cloudera.org:8080/#/c/19503/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19503/6//COMMIT_MSG@9 PS6, Line 9: **** BREAKING CHANGE **** Maybe a summary of what is breaking would be useful here. http://gerrit.cloudera.org:8080/#/c/19503/6//COMMIT_MSG@38 PS6, Line 38: 2. The new flag jwks_insecure_tls controls whether or not Maybe a more descriptive name would be useful? Just insecure_tls seems very general. jwks_allow_unverified_certificate http://gerrit.cloudera.org:8080/#/c/19503/6/be/src/kudu/util/curl_util.cc File be/src/kudu/util/curl_util.cc: http://gerrit.cloudera.org:8080/#/c/19503/6/be/src/kudu/util/curl_util.cc@89 PS6, Line 89: // Ensure the curl error buffer is large enough Nit: add period http://gerrit.cloudera.org:8080/#/c/19503/6/be/src/kudu/util/curl_util.cc@116 PS6, Line 116: // across calls Nit: add period http://gerrit.cloudera.org:8080/#/c/19503/6/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/19503/6/be/src/rpc/authentication.cc@174 PS6, Line 174: "trust chain can be established for it and if the certificate has a common name or " Nit: "it," http://gerrit.cloudera.org:8080/#/c/19503/6/be/src/rpc/authentication.cc@175 PS6, Line 175: "SAN that matches the server's hostname. This should only be set to false for " Nit: double space http://gerrit.cloudera.org:8080/#/c/19503/6/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/19503/6/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@414 PS6, Line 414: List<String> logLines; Nit: move to where it is used, like List<String> logLines = Files.readAllLines(logDir.resolve("impalad.ERROR")); http://gerrit.cloudera.org:8080/#/c/19503/6/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@465 PS6, Line 465: List<String> logLines; Move to where used, as above? http://gerrit.cloudera.org:8080/#/c/19503/6/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@479 PS6, Line 479: for (Iterator<String> i = logLines.iterator(); i.hasNext();) { Can we say assertTrue(..., logLines.contains(expectedErrString)) ? http://gerrit.cloudera.org:8080/#/c/19503/6/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java File fe/src/test/java/org/apache/impala/testutil/X509CertChain.java: http://gerrit.cloudera.org:8080/#/c/19503/6/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@103 PS6, Line 103: * @return String Maybe delete these lines, they don't really add much http://gerrit.cloudera.org:8080/#/c/19503/6/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@179 PS6, Line 179: gen.setEndDate(new Time(new Date(System.currentTimeMillis() + 60 * 60 * 1000))); Maybe comment the meaning of these dates -- To view, visit http://gerrit.cloudera.org:8080/19503 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5f1e887fae39b5fb82fa9a40352e4b507b7d8d35 Gerrit-Change-Number: 19503 Gerrit-PatchSet: 6 Gerrit-Owner: Jason Fehr <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Tue, 21 Feb 2023 19:02:55 +0000 Gerrit-HasComments: Yes
