Jason Fehr 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 7: (14 comments) addressed additional comments, thanks to all the reviewers who took time to review! 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. Done http://gerrit.cloudera.org:8080/#/c/19503/6//COMMIT_MSG@29 PS6, Line 29: with any claims > this flag is already renamed Done http://gerrit.cloudera.org:8080/#/c/19503/6//COMMIT_MSG@38 PS6, Line 38: verify that cert. Thus, any cases where the JWKS was > Maybe a more descriptive name would be useful? Just insecure_tls seems very Updated to match the new impala startup flag. 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 Done http://gerrit.cloudera.org:8080/#/c/19503/6/be/src/kudu/util/curl_util.cc@116 PS6, Line 116: // across calls. > Nit: add period Done 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," Done 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 Done 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@55 PS6, Line 55: tring SERVER_KEY > name constant variables with all upper case characters Deleted since they are not used. http://gerrit.cloudera.org:8080/#/c/19503/6/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@414 PS6, Line 414: + "problem: unable to get local issuer certificate", jwksHttpUrl); > Nit: move to where it is used, like Done http://gerrit.cloudera.org:8080/#/c/19503/6/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@465 PS6, Line 465: // check in the impalad logs that the server startup failed for the expected reason > Move to where used, as above? Done http://gerrit.cloudera.org:8080/#/c/19503/6/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@479 PS6, Line 479: String certDir = setupServerAndRootCerts("testJwtAuthWithTrustedJwksHttpsUrl", > Can we say Yes, leveraging a couple of hamcrest matchers to figure out if the log lines contains an item that contains the expected string within that item. 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@80 PS6, Line 80: private final KeyPair rootCaKp_; : private final KeyPair leafKp_; : private final X509Certificate rootCert_; : private final X509Certificate leafCert_ > follow the coding style to name the member variables with last character as Done http://gerrit.cloudera.org:8080/#/c/19503/6/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@103 PS6, Line 103: public String rootCertAsPemString() throws CertificateEncodingException, IOException { > Maybe delete these lines, they don't really add much Done http://gerrit.cloudera.org:8080/#/c/19503/6/fe/src/test/java/org/apache/impala/testutil/X509CertChain.java@179 PS6, Line 179: gen.setSubject(cn); > Maybe comment the meaning of these dates Done -- 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: 7 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 22:49:34 +0000 Gerrit-HasComments: Yes
