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 3: (19 comments) Addressed comments. http://gerrit.cloudera.org:8080/#/c/19503/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19503/1//COMMIT_MSG@12 PS1, Line 12: I > nit: one extra space Done http://gerrit.cloudera.org:8080/#/c/19503/1//COMMIT_MSG@13 PS1, Line 13: HTTPS > nit: HTTPS Done http://gerrit.cloudera.org:8080/#/c/19503/1//COMMIT_MSG@24 PS1, Line 24: T > nit: one extra space Done http://gerrit.cloudera.org:8080/#/c/19503/1//COMMIT_MSG@46 PS1, Line 46: PEM > nit: PEM Done http://gerrit.cloudera.org:8080/#/c/19503/1//COMMIT_MSG@46 PS1, Line 46: e bundle tha > Should those be CA certificates or non-CA certs (e.g., exact TLS server cer It's valid to trust both CA and non-CA certificates. All the curl lib needs to do is establish a chain of certificates between the leaf cert presented by the server and a certificate it trusts. If curl trusts the certificate presented by the server, then a chain of trust has been established. http://gerrit.cloudera.org:8080/#/c/19503/1//COMMIT_MSG@48 PS1, Line 48: > add a Testing section Done 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: certificates > Are these should be CA certificates or non-CA certs are also accepted? It can be either. I updated the comment to reflect this. http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/kudu/util/curl_util.h@75 PS1, Line 75: CA ce > nit: HTTPS Done 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? I copied this code from line 92-93. I'm very new to Impala, so I don't have a good understanding of the difference between CHECK_EQ on the return code or CURL_RETURN_NOT_OK. What are the differences? 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_verify_serve > nit: 'jwks_insecure_tls' sounds a bit vague to me: it might be authenticati I modeled this flag after the curl command line utility, but I like your suggestion better. I'm going to leave out the "tls" part because that is implied when working with server certs. http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/rpc/authentication.cc@1418 PS1, Line 1418: return Status( > Check jwks_ca_certificate is not empty if jwks_insecure_tls is set as false If jwks_insecure_tls is set to false and jwks_ca_certificate is empty, then Impala will use the system trust store. Most of the time (including CDP production), the system trust store will be enough. 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: of certs > nit: what exactly this 'trust' covers? Is this just to verify authenticity I modified this comment a little bit in an attempt to clarify. It's only used when retrieving the JWKS. We do need to be clear since there are multiple places throughout Impala where a PEM certificate bundle could be used as a trust store. 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 I based my changes off the pattern that was already established. Looking at the code, JWTHelper is a static singleton that gets instantiated during application startup: https://github.com/apache/impala/blob/feb4a76ed4cb5b688143eb21370f78ec93133c56/be/src/util/jwt-util.cc#L739 At this point in my C++ experience, I don't feel comfortable taking on a refactor of this nature. http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/util/jwt-util.h@63 PS1, Line 63: Init > ditto see previous comment http://gerrit.cloudera.org:8080/#/c/19503/1/be/src/util/jwt-util.h@64 PS1, Line 64: bool is_local_file > do we still need this variable? Yeah. If is_local_file is true, then the JWKS is read from the local filesystem instead of from a URL. 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: > for ? ha ha, yes. http://gerrit.cloudera.org:8080/#/c/19503/1/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@397 PS1, Line 397: > It's better to give a certificate which does not match the certificate retu Good point. Really we need to test both cases: 1. cert trust chain cannot be established but the cert CN and server hostname matches 2. cert trust chain is established but the cert CN and server hostname do not match I added test cases for each situation. http://gerrit.cloudera.org:8080/#/c/19503/1/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@420 PS1, Line 420: > nit: extra spaces Done http://gerrit.cloudera.org:8080/#/c/19503/1/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@440 PS1, Line 440: JWKS server is n > Is this certificate also has the CA capability? If not, I'm a bit surprise The expected use case in the wild would be a trusted root CA. Then, the server would return its leaf cert (where the server hostname matches the cert CN/SAN) and any intermediates necessary to established a trust chain back to the root. Wenzhe made a comment up on line 397 that prompted me to rethink the test cases. I realized I combined two test cases into one, and I need to split them apart. Once I do that, this concern will be addressed. -- 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: 3 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 02:37:55 +0000 Gerrit-HasComments: Yes
