Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/17802 )
Change subject: IMPALA-10876: Support to download JWKS from given URL ...................................................................... Patch Set 5: (8 comments) Thanks for working on this, this is making a lot of sense to me. I really appreciate switching over to Kudu's EasyCurl wrapper. http://gerrit.cloudera.org:8080/#/c/17802/5/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/17802/5/be/src/rpc/authentication.cc@169 PS5, Line 169: DEFINE_int32(update_jwks_frequency_ms, 5000, : "(Advanced) The time in milliseconds to wait between downloading JWKS from the " : "specified URL."); Key rotation is usually not expected to happen in a matter of seconds, so I think we can use a longer interval here, like 60 seconds. >From a units perspective, do we expect anyone to set this smaller than one >second or need the extra precision of milliseconds? We might be just as happy >with seconds precision. Nit: From a naming perspective, I think I would rearrange the words to be "jwks_update_frequency_ms" http://gerrit.cloudera.org:8080/#/c/17802/5/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/17802/5/be/src/service/impala-server.cc@2918 PS5, Line 2918: if (TestInfo::is_test()) sleep(1); Nit: What is the purpose of this sleep? I'm not necessarily against it, given it is test-only, but I'm just curious what it is solving. http://gerrit.cloudera.org:8080/#/c/17802/5/be/src/util/jwt-util.h File be/src/util/jwt-util.h: http://gerrit.cloudera.org:8080/#/c/17802/5/be/src/util/jwt-util.h@70 PS5, Line 70: static Status Verify( : const JWTDecodedToken* decoded_token, std::shared_ptr<const JWKSSnapshot> jwks); Do we call this static Verify() from anywhere other than the other Verify()? If not, then I would just fold the logic into Verify(). http://gerrit.cloudera.org:8080/#/c/17802/5/be/src/util/jwt-util.cc File be/src/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/17802/5/be/src/util/jwt-util.cc@554 PS5, Line 554: curl.set_timeout(kudu::MonoDelta::FromMilliseconds(2000)); We might want to make this configurable. I'm not sure what a good value is. It seems like most requests will complete within 2 seconds, and given that we are trying periodically, we are likely to successfully get the latest JWKS. We might consider bumping this, depending on what value we choose for the update frequency. http://gerrit.cloudera.org:8080/#/c/17802/5/be/src/util/jwt-util.cc@665 PS5, Line 665: if (!TestInfo::is_be_test()) { : RETURN_IF_ERROR(Thread::Create("impala-server", "JWKS-mgr", : &JWKSMgr::UpdateJWKSThread, this, &update_jwks_thread_)); : } Just for reference, one way to make a thread work in a backend test is to provide some logic to shut it down at the end. For example, the file handle cache has a Promise that tells the thread to shut down: https://github.com/apache/impala/blob/master/be/src/runtime/io/handle-cache.h#L217 The destructor sets that promise and waits for the thread to shut down. https://github.com/apache/impala/blob/master/be/src/runtime/io/handle-cache.inline.h#L80-L83 The thread's loop is actually waiting on the promise with a timeout, which lets it immediately exit if the promise gets set. https://github.com/apache/impala/blob/master/be/src/runtime/io/handle-cache.inline.h#L211 In this case, we may not be testing the URL as part of the backend test, so skipping the thread is ok. http://gerrit.cloudera.org:8080/#/c/17802/5/be/src/util/jwt-util.cc@678 PS5, Line 678: SleepForMs(FLAGS_update_jwks_frequency_ms); Nit: add a check in JWKSMgr::Init() for this to be positive. http://gerrit.cloudera.org:8080/#/c/17802/5/be/src/util/jwt-util.cc@773 PS5, Line 773: JWKSSnapshotPtr jwks = GetJWKS(); : // Skip to signature validation if there is no public key. : if (!jwks->IsEmpty()) status = Verify(decoded_token, jwks); In the case where we don't specify a JWKS file or url, I think jwks_mgr_ is null. In the null case, we should skip verification like we did before (and we wouldn't get to the GetJWKS() call). The other case here is where the JWKS is configured but the JWKS file does not have any keys (i.e. IsEmpty()). From a security point of view, I think it is safer to have that mean that everything gets rejected (i.e. goes through verification and has no matching valid key). The downside to that is if this every happens by accident, then that is a complete outage, but that seems unlikely. http://gerrit.cloudera.org:8080/#/c/17802/5/fe/src/test/java/org/apache/impala/customcluster/JwtWebserverTest.java File fe/src/test/java/org/apache/impala/customcluster/JwtWebserverTest.java: http://gerrit.cloudera.org:8080/#/c/17802/5/fe/src/test/java/org/apache/impala/customcluster/JwtWebserverTest.java@55 PS5, Line 55: CustomClusterRunner.StartImpalaCluster(); Is this so that the test leaves a cluster running with the default configuration? -- To view, visit http://gerrit.cloudera.org:8080/17802 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic6ac8cf0010c13db30219776d1d275709bf211df Gerrit-Change-Number: 17802 Gerrit-PatchSet: 5 Gerrit-Owner: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Thu, 23 Sep 2021 21:15:44 +0000 Gerrit-HasComments: Yes
