Alexey Serbin 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 1: (13 comments) http://gerrit.cloudera.org:8080/#/c/19503/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19503/1//COMMIT_MSG@13 PS1, Line 13: https nit: HTTPS http://gerrit.cloudera.org:8080/#/c/19503/1//COMMIT_MSG@46 PS1, Line 46: pem nit: PEM http://gerrit.cloudera.org:8080/#/c/19503/1//COMMIT_MSG@46 PS1, Line 46: certificates Should those be CA certificates or non-CA certs (e.g., exact TLS server certificates without CA capability) are also accepted? http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/kudu/util/curl_util.h File be/src/kudu/util/curl_util.h: http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/kudu/util/curl_util.h@74 PS1, Line 74: pem nit: PEM http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/kudu/util/curl_util.h@74 PS1, Line 74: certificates Are these should be CA certificates or non-CA certs are also accepted? http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/kudu/util/curl_util.h@75 PS1, Line 75: https nit: HTTPS http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/kudu/util/curl_util.cc File be/src/kudu/util/curl_util.cc: http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/kudu/util/curl_util.cc@122 PS1, Line 122: CHECK_EQ Does it make sense to switch to using CURL_RETURN_NOT_OK() here instead? http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/rpc/authentication.cc@171 PS1, Line 171: jwks_insecure_tls nit: 'jwks_insecure_tls' sounds a bit vague to me: it might be authentication-only TLS channel, not verifying certs on either of the sides, using weak ciphers for the handshake, using weak ciphers to encrypt the data sent over the established channel, etc. Maybe, something like 'jwks_verify_server_tls_cert' or similar would be more descriptive of what this flag is actually for? http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/util/jwt-util-internal.h File be/src/util/jwt-util-internal.h: http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/util/jwt-util-internal.h@374 PS1, Line 374: to trust nit: what exactly this 'trust' covers? Is this just to verify authenticity of the JWKS server's TLS certificate or the certificates in the bundle are used for something else as well? http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/util/jwt-util.h File be/src/util/jwt-util.h: http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/util/jwt-util.h@59 PS1, Line 59: const std::string& jwks_file_path Does it make sense to make this a parameter of one of the constructors for this class and have just one Init() method with the signature Status Init(); ? http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/util/jwt-util.h@63 PS1, Line 63: Init ditto http://gerrit.cloudera.org:8080/#/c/19503/1/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/1/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@83 PS1, Line 83: the for ? http://gerrit.cloudera.org:8080/#/c/19503/1/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@440 PS1, Line 440: webserverTLSCert Is this certificate also has the CA capability? If not, I'm a bit surprised a non-CA certificate is accepted here. Overall, is it possible to pass here not the server's certificate as is, but the CA certificate that the server's cert is signed with? I guess that would be the expected use case in the wild, no? -- 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: 1 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: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Thu, 16 Feb 2023 03:06:52 +0000 Gerrit-HasComments: Yes
