You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org> on 2021/08/21 00:37:01 UTC
[Impala-ASF-CR] IMPALA-10876: Support to download JWKS from given URL
Wenzhe Zhou has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17802
Change subject: IMPALA-10876: Support to download JWKS from given URL
......................................................................
IMPALA-10876: Support to download JWKS from given URL
This patch added supporting to download JWKS from a given URL.
We call libcurl's APIs to download file from the given URL. curl was
added to native-toolchain. This patch modified bootstrap_toolchain.py
and makefiles to integrate libcurl.
Added end-end JWT authentication test cases with JWKS specified as
HTTP/HTTPS URL.
Testing:
- Passed core run.
Change-Id: Ic6ac8cf0010c13db30219776d1d275709bf211df
---
M CMakeLists.txt
M be/CMakeLists.txt
M be/src/rpc/authentication.cc
M be/src/service/impala-server.cc
M be/src/util/jwt-util-internal.h
M be/src/util/jwt-util.cc
M be/src/util/jwt-util.h
M bin/bootstrap_toolchain.py
M bin/impala-config.sh
M bin/rat_exclude_files.txt
A cmake_modules/FindCurl.cmake
M fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java
M fe/src/test/java/org/apache/impala/customcluster/JwtWebserverTest.java
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
M fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java
R www/jwt/jwks_rs256.json
16 files changed, 357 insertions(+), 36 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/17802/2
--
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: newchange
Gerrit-Change-Id: Ic6ac8cf0010c13db30219776d1d275709bf211df
Gerrit-Change-Number: 17802
Gerrit-PatchSet: 2
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
[Impala-ASF-CR] IMPALA-10876: Support to download JWKS from given URL
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/17802 )
Change subject: IMPALA-10876: Support to download JWKS from given URL
......................................................................
IMPALA-10876: Support to download JWKS from given URL
This patch added supporting to download JWKS from a given URL.
We call libcurl's APIs to download file from the given URL. curl was
added to native-toolchain. This patch modified bootstrap_toolchain.py
and makefiles to integrate libcurl.
Added end-end JWT authentication test cases with JWKS specified as
HTTP/HTTPS URL.
Testing:
- Passed core run.
Change-Id: Ic6ac8cf0010c13db30219776d1d275709bf211df
---
M CMakeLists.txt
M be/CMakeLists.txt
M be/src/rpc/authentication.cc
M be/src/service/impala-server.cc
M be/src/util/jwt-util-internal.h
M be/src/util/jwt-util.cc
M be/src/util/jwt-util.h
M bin/bootstrap_toolchain.py
M bin/impala-config.sh
M bin/rat_exclude_files.txt
A cmake_modules/FindCurl.cmake
M fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java
M fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java
M fe/src/test/java/org/apache/impala/customcluster/JwtWebserverTest.java
M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
M fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java
R www/jwt/jwks_rs256.json
17 files changed, 396 insertions(+), 39 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/17802/3
--
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: newpatchset
Gerrit-Change-Id: Ic6ac8cf0010c13db30219776d1d275709bf211df
Gerrit-Change-Number: 17802
Gerrit-PatchSet: 3
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-10876: Support to download JWKS from given URL
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/9479/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 4
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Sep 2021 22:19:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10876: Support to download JWKS from given URL
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
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 7: Code-Review+2
This looks good to me
--
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: 7
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 27 Sep 2021 22:03:30 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10876: Support to download JWKS from given URL
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/17802 )
Change subject: IMPALA-10876: Support to download JWKS from given URL
......................................................................
IMPALA-10876: Support to download JWKS from given URL
This patch added functionality to download JWKS from a given URL and
support key rotation by periodically checking the JWKS URL for updates.
We use Kudu's EasyCurl wrapper to download file from the given URL.
curl was added to native-toolchain. This patch modified makefiles
and bootstrap_toolchain.py to integrate libcurl and libkudu_curl_util.
Added end-end JWT authentication test cases with JWKS specified as
HTTP/HTTPS URL.
Testing:
- Passed core run, including new test cases.
Change-Id: Ic6ac8cf0010c13db30219776d1d275709bf211df
---
M CMakeLists.txt
M be/CMakeLists.txt
M be/src/kudu/util/CMakeLists.txt
M be/src/rpc/authentication.cc
M be/src/service/impala-server.cc
M be/src/util/jwt-util-internal.h
M be/src/util/jwt-util-test.cc
M be/src/util/jwt-util.cc
M be/src/util/jwt-util.h
M bin/bootstrap_toolchain.py
M bin/impala-config.sh
M bin/rat_exclude_files.txt
A cmake_modules/FindCurl.cmake
M fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java
M fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java
M fe/src/test/java/org/apache/impala/customcluster/JwtWebserverTest.java
A testdata/jwt/jwks_es256.json
17 files changed, 604 insertions(+), 140 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/17802/7
--
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: newpatchset
Gerrit-Change-Id: Ic6ac8cf0010c13db30219776d1d275709bf211df
Gerrit-Change-Number: 17802
Gerrit-PatchSet: 7
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[Impala-ASF-CR] IMPALA-10876: Support to download JWKS from given URL
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/9451/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 3
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 Sep 2021 20:56:12 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10876: Support to download JWKS from given URL
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
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 4:
(1 comment)
Thanks for the changes, I'm working my way through. I had one initial comment.
http://gerrit.cloudera.org:8080/#/c/17802/4/be/src/kudu/util/curl_util.h
File be/src/kudu/util/curl_util.h:
http://gerrit.cloudera.org:8080/#/c/17802/4/be/src/kudu/util/curl_util.h@17
PS4, Line 17:
: #pragma once
:
: #include <cstddef>
: #include <string>
: #include <utility>
: #include <vector>
:
: #include "kudu/gutil/macros.h"
: #include "kudu/util/monotime.h"
: #include "kudu/util/status.h"
:
: typedef void CURL;
:
: namespace kudu {
:
: class faststring;
:
: enum class CurlAuthType {
: NONE,
: BASIC,
: DIGEST,
: SPNEGO,
: };
:
: // Simple wrapper around curl's "easy" interface, allowing the user to
: // fetch web pages into memory using a blocking API.
: //
: // This is not thread-safe.
: class EasyCurl {
: public:
: EasyCurl();
: ~EasyCurl();
:
: // Fetch the given URL into the provided buffer.
: // Any existing data in the buffer is replaced.
: // The optional param 'headers' holds additional headers.
: // e.g. {"Accept-Encoding: gzip"}
: Status FetchURL(const std::string& url,
: faststring* dst,
: const std::vector<std::string>& headers = {});
:
: // Issue an HTTP POST to the given URL with the given data.
: // Returns results in 'dst' as above.
: // The optional param 'headers' holds additional headers.
: // e.g. {"Accept-Encoding: gzip"}
: Status PostToURL(const std::string& url,
: const std::string& post_data,
: faststring* dst,
: const std::vector<std::string>& headers = {});
:
: // Set whether to verify the server's SSL certificate in the case of an HTTPS
: // connection.
: void set_verify_peer(bool verify)
Can we split out the change to be/src/kudu into its own commit? That commit can list out which Kudu JIRAs are being incorporated.
It will be a bit of logistic annoyance, but that will keep it clear what we are doing to be/src/kudu.
--
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: 4
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Sep 2021 22:22:00 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10876: Support to download JWKS from given URL
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 8: Verified+1
--
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: 8
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 28 Sep 2021 04:45:22 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10876: Support to download JWKS from given URL
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 8:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/7494/ DRY_RUN=false
--
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: 8
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 27 Sep 2021 22:34:20 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10876: Support to download JWKS from given URL
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/9497/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 6
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 24 Sep 2021 03:20:23 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10876: Support to download JWKS from given URL
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
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 6: Code-Review+2
(1 comment)
This change is looking good to me. I have one nit, but this looks ready to go otherwise.
http://gerrit.cloudera.org:8080/#/c/17802/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/17802/6/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@89
PS6, Line 89: String srcJwksFilename =
: new File(System.getenv("IMPALA_HOME"), "testdata/jwt/jwks_rs256.json").getPath();
: String dstJwksFilename =
: new File(System.getenv("IMPALA_HOME"), "www/jwks_test.json").getPath();
: List<String> command =
: Lists.newArrayList(Arrays.asList("cp", srcJwksFilename, dstJwksFilename));
: RunShellCommand.Run(command.toArray(new String[0]), /*shouldSucceed*/ true, "", "");
Nit: It would be nice to use native Java methods to avoid dropping down to shell. It looks like Java's java.nio.file.Files has the methods we need:
https://docs.oracle.com/javase/8/docs/api/java/nio/file/Files.html
This comment applies here and the other copy and remove below.
TestRequestPoolService.java uses a guava equivalent to do this, but I think we would prefer the regular Java library rather than using guava:
https://github.com/apache/impala/blob/master/fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java#L50
https://github.com/apache/impala/blob/master/fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java#L100
--
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: 6
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 27 Sep 2021 17:28:01 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10876: Support to download JWKS from given URL
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
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 <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 23 Sep 2021 21:15:44 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10876: Support to download JWKS from given URL
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/17802 )
Change subject: IMPALA-10876: Support to download JWKS from given URL
......................................................................
IMPALA-10876: Support to download JWKS from given URL
This patch added functionality to download JWKS from a given URL and
support key rotation by periodically checking the JWKS URL for updates.
We use Kudu's EasyCurl wrapper to download file from the given URL.
curl was added to native-toolchain. This patch modified makefiles
and bootstrap_toolchain.py to integrate libcurl and libkudu_curl_util.
Added end-end JWT authentication test cases with JWKS specified as
HTTP/HTTPS URL.
Testing:
- Passed core run, including new test cases.
Change-Id: Ic6ac8cf0010c13db30219776d1d275709bf211df
---
M CMakeLists.txt
M be/CMakeLists.txt
M be/src/kudu/util/CMakeLists.txt
M be/src/rpc/authentication.cc
M be/src/service/impala-server.cc
M be/src/util/jwt-util-internal.h
M be/src/util/jwt-util-test.cc
M be/src/util/jwt-util.cc
M be/src/util/jwt-util.h
M bin/bootstrap_toolchain.py
M bin/impala-config.sh
M bin/rat_exclude_files.txt
A cmake_modules/FindCurl.cmake
M fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java
M fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java
M fe/src/test/java/org/apache/impala/customcluster/JwtWebserverTest.java
A testdata/jwt/jwks_es256.json
17 files changed, 604 insertions(+), 140 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/17802/6
--
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: newpatchset
Gerrit-Change-Id: Ic6ac8cf0010c13db30219776d1d275709bf211df
Gerrit-Change-Number: 17802
Gerrit-PatchSet: 6
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[Impala-ASF-CR] IMPALA-10876: Support to download JWKS from given URL
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/9490/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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 <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 23 Sep 2021 00:45:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10876: Support to download JWKS from given URL
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou 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 3:
(3 comments)
Add a working thread to periodically check the JWKS URL for updates.
http://gerrit.cloudera.org:8080/#/c/17802/3/be/src/util/jwt-util.cc
File be/src/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/17802/3/be/src/util/jwt-util.cc@585
PS3, Line 585: curl_global_init(CURL_GLOBAL_DEFAULT);
:
: // Initialize curl session.
: curl_handle = curl_easy_init();
: if (curl_handle == nullptr) {
: status = Status("Error to initialize curl session");
: goto cleanup;
: }
: curl_easy_setopt(curl_handle, CURLOPT_URL, jwks_url.c_str());
: curl_easy_setopt(curl_handle, CURLOPT_WRITEFUNCTION, CurlWriteDownloadBufferCallback);
: curl_easy_setopt(curl_handle, CURLOPT_WRITEDATA, (void*)&chunk);
: curl_easy_setopt(curl_handle, CURLOPT_USERAGENT, "libcurl-agent/1.0");
> It's worth looking at Kudu's EasyCurl wrapper to see if it would work for u
Will change code to use Kudu's EasyCurl wrapper. The source code of Kudu's EasyCurl were updated in Kudu's upstream repo. Need to upgrade the corresponding code from Kudu.
http://gerrit.cloudera.org:8080/#/c/17802/3/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/17802/3/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@227
PS3, Line 227: String webserverArgs = "--webserver_port=25010";
> It would be good to make it very clear that the statestore is serving up th
Will change variable names as suggested to make code more readable.
http://gerrit.cloudera.org:8080/#/c/17802/3/www/jwt/jwks_rs256.json
File www/jwt/jwks_rs256.json:
http://gerrit.cloudera.org:8080/#/c/17802/3/www/jwt/jwks_rs256.json@1
PS3, Line 1:
: {
: "keys": [
: { "use": "sig", "kty": "RSA", "kid": "public:c424b67b-fe28-45d7-b015-f79da50b5b21", "alg": "RS256", "n": "uGbXWiK3dQTyCbX5xdE4yCuYp0AF2d15Qq1JSXT_lx8CEcXb9RbDddl8jGDv-spi5qPa8qEHiK7FwV2KpRE983wGPnYsAm9BxLFb4YrLYcDFOIGULuk2FtrPS512Qea1bXASuvYXEpQNpGbnTGVsWXI9C-yjHztqyL2h8P6mlThPY9E9ue2fCqdgixfTFIF9Dm4SLHbphUS2iw7w1JgT69s7of9-I9l5lsJ9cozf1rxrXX4V1u_SotUuNB3Fp8oB4C1fLBEhSlMcUJirz1E8AziMCxS-VrRPDM-zfvpIJg3JljAh3PJHDiLu902v9w-Iplu1WyoB2aPfitxEhRN0Yw", "e": "AQAB" },
: { "use": "sig", "kty": "RSA", "kid": "public:9b9d0b47-b9ed-4ba6-9180-52fc5b161a3a", "alg": "RS256", "n": "xzYuc22QSst_dS7geYYK5l5kLxU0tayNdixkEQ17ix-CUcUbKIsnyftZxaCYT46rQtXgCaYRdJcbB3hmyrOavkhTpX79xJZnQmfuamMbZBqitvscxW9zRR9tBUL6vdi_0rpoUwPMEh8-Bw7CgYR0FK0DhWYBNDfe9HKcyZEv3max8Cdq18htxjEsdYO0iwzhtKRXomBWTdhD5ykd_fACVTr4-KEY-IeLvubHVmLUhbE5NgWXxrRpGasDqzKhCTmsa2Ysf712rl57SlH0Wz_Mr3F7aM9YpErzeYLrl0GhQr9BVJxOvXcVd4kmY-XkiCcrkyS1cnghnllh-LCwQu1sYw", "e": "AQAB" }
: ]
: }
> Everything in the www/ directory gets shipped as part of the docker image b
Move the JWKS file back to directory testdata/jwt, and copy the JWKS files temporarily to root directory of Web server when running test and delete it at the end of test.
--
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: 3
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Sep 2021 21:46:02 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10876: Support to download JWKS from given URL
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17802 )
Change subject: IMPALA-10876: Support to download JWKS from given URL
......................................................................
IMPALA-10876: Support to download JWKS from given URL
This patch added functionality to download JWKS from a given URL and
support key rotation by periodically checking the JWKS URL for updates.
We use Kudu's EasyCurl wrapper to download file from the given URL.
curl was added to native-toolchain. This patch modified makefiles
and bootstrap_toolchain.py to integrate libcurl and libkudu_curl_util.
Added end-end JWT authentication test cases with JWKS specified as
HTTP/HTTPS URL.
Testing:
- Passed core run, including new test cases.
Change-Id: Ic6ac8cf0010c13db30219776d1d275709bf211df
Reviewed-on: http://gerrit.cloudera.org:8080/17802
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M CMakeLists.txt
M be/CMakeLists.txt
M be/src/kudu/util/CMakeLists.txt
M be/src/rpc/authentication.cc
M be/src/service/impala-server.cc
M be/src/util/jwt-util-internal.h
M be/src/util/jwt-util-test.cc
M be/src/util/jwt-util.cc
M be/src/util/jwt-util.h
M bin/bootstrap_toolchain.py
M bin/impala-config.sh
M bin/rat_exclude_files.txt
A cmake_modules/FindCurl.cmake
M fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java
M fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java
M fe/src/test/java/org/apache/impala/customcluster/JwtWebserverTest.java
A testdata/jwt/jwks_es256.json
17 files changed, 604 insertions(+), 140 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
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: merged
Gerrit-Change-Id: Ic6ac8cf0010c13db30219776d1d275709bf211df
Gerrit-Change-Number: 17802
Gerrit-PatchSet: 9
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[Impala-ASF-CR] IMPALA-10876: Support to download JWKS from given URL
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 7:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/9507/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 7
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 27 Sep 2021 19:36:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10876: Support to download JWKS from given URL
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/9341/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 2
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Sat, 21 Aug 2021 01:00:38 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10876: Support to download JWKS from given URL
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/17802 )
Change subject: IMPALA-10876: Support to download JWKS from given URL
......................................................................
IMPALA-10876: Support to download JWKS from given URL
This patch added functionality to download JWKS from a given URL and
support key rotation by periodically checking the JWKS URL for updates.
We use Kudu's EasyCurl wrapper to download file from the given URL.
curl was added to native-toolchain. This patch modified makefiles
and bootstrap_toolchain.py to integrate libcurl and libkudu_curl_util.
Upgraded source code for Kudu's EasyCurl wrapper (curl_util.cc and
curl_util.h in kudu/util) from the top of Kudu upstream (commit hash:
7a4061e87e).
Added end-end JWT authentication test cases with JWKS specified as
HTTP/HTTPS URL.
Testing:
- Passed core run, including new test cases.
Change-Id: Ic6ac8cf0010c13db30219776d1d275709bf211df
---
M CMakeLists.txt
M be/CMakeLists.txt
M be/src/kudu/util/CMakeLists.txt
M be/src/kudu/util/curl_util-test.cc
M be/src/kudu/util/curl_util.cc
M be/src/kudu/util/curl_util.h
M be/src/rpc/authentication.cc
M be/src/service/impala-server.cc
M be/src/util/jwt-util-internal.h
M be/src/util/jwt-util-test.cc
M be/src/util/jwt-util.cc
M be/src/util/jwt-util.h
M bin/bootstrap_toolchain.py
M bin/impala-config.sh
M bin/rat_exclude_files.txt
A cmake_modules/FindCurl.cmake
M fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java
M fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java
M fe/src/test/java/org/apache/impala/customcluster/JwtWebserverTest.java
A testdata/jwt/jwks_es256.json
20 files changed, 725 insertions(+), 192 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/17802/4
--
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: newpatchset
Gerrit-Change-Id: Ic6ac8cf0010c13db30219776d1d275709bf211df
Gerrit-Change-Number: 17802
Gerrit-PatchSet: 4
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[Impala-ASF-CR] IMPALA-10876: Support to download JWKS from given URL
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 8: Code-Review+2
--
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: 8
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 27 Sep 2021 22:34:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10876: Support to download JWKS from given URL
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
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 3:
I'm just digging into reviewing this. Did we have a plan to periodically check the JWKS URL for updates? That would provide support for key rotation. This change is useful on its own, but we may want that functionality.
--
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: 3
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Comment-Date: Wed, 15 Sep 2021 20:31:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10876: Support to download JWKS from given URL
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
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 3:
(3 comments)
Initial round of comments
http://gerrit.cloudera.org:8080/#/c/17802/3/be/src/util/jwt-util.cc
File be/src/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/17802/3/be/src/util/jwt-util.cc@585
PS3, Line 585: curl_global_init(CURL_GLOBAL_DEFAULT);
:
: // Initialize curl session.
: curl_handle = curl_easy_init();
: if (curl_handle == nullptr) {
: status = Status("Error to initialize curl session");
: goto cleanup;
: }
: curl_easy_setopt(curl_handle, CURLOPT_URL, jwks_url.c_str());
: curl_easy_setopt(curl_handle, CURLOPT_WRITEFUNCTION, CurlWriteDownloadBufferCallback);
: curl_easy_setopt(curl_handle, CURLOPT_WRITEDATA, (void*)&chunk);
: curl_easy_setopt(curl_handle, CURLOPT_USERAGENT, "libcurl-agent/1.0");
It's worth looking at Kudu's EasyCurl wrapper to see if it would work for us:
https://github.com/apache/impala/blob/master/be/src/kudu/util/curl_util.h
If it does what we need, it would be less code for us to maintain.
http://gerrit.cloudera.org:8080/#/c/17802/3/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/17802/3/fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java@227
PS3, Line 227: String webserverArgs = "--webserver_port=25010";
It would be good to make it very clear that the statestore is serving up the JWKS.
One way is to be very explicit with the variable names here. For "webserverArgs", we could use "statestoreWebserverArgs". For jwksHttpUrl, we could use "statestoreWebserverJwksUrl" or something like that. For "jwtAargs", we could use "impaladJwtArgs".
http://gerrit.cloudera.org:8080/#/c/17802/3/www/jwt/jwks_rs256.json
File www/jwt/jwks_rs256.json:
http://gerrit.cloudera.org:8080/#/c/17802/3/www/jwt/jwks_rs256.json@1
PS3, Line 1:
: {
: "keys": [
: { "use": "sig", "kty": "RSA", "kid": "public:c424b67b-fe28-45d7-b015-f79da50b5b21", "alg": "RS256", "n": "uGbXWiK3dQTyCbX5xdE4yCuYp0AF2d15Qq1JSXT_lx8CEcXb9RbDddl8jGDv-spi5qPa8qEHiK7FwV2KpRE983wGPnYsAm9BxLFb4YrLYcDFOIGULuk2FtrPS512Qea1bXASuvYXEpQNpGbnTGVsWXI9C-yjHztqyL2h8P6mlThPY9E9ue2fCqdgixfTFIF9Dm4SLHbphUS2iw7w1JgT69s7of9-I9l5lsJ9cozf1rxrXX4V1u_SotUuNB3Fp8oB4C1fLBEhSlMcUJirz1E8AziMCxS-VrRPDM-zfvpIJg3JljAh3PJHDiLu902v9w-Iplu1WyoB2aPfitxEhRN0Yw", "e": "AQAB" },
: { "use": "sig", "kty": "RSA", "kid": "public:9b9d0b47-b9ed-4ba6-9180-52fc5b161a3a", "alg": "RS256", "n": "xzYuc22QSst_dS7geYYK5l5kLxU0tayNdixkEQ17ix-CUcUbKIsnyftZxaCYT46rQtXgCaYRdJcbB3hmyrOavkhTpX79xJZnQmfuamMbZBqitvscxW9zRR9tBUL6vdi_0rpoUwPMEh8-Bw7CgYR0FK0DhWYBNDfe9HKcyZEv3max8Cdq18htxjEsdYO0iwzhtKRXomBWTdhD5ykd_fACVTr4-KEY-IeLvubHVmLUhbE5NgWXxrRpGasDqzKhCTmsa2Ysf712rl57SlH0Wz_Mr3F7aM9YpErzeYLrl0GhQr9BVJxOvXcVd4kmY-XkiCcrkyS1cnghnllh-LCwQu1sYw", "e": "AQAB" }
: ]
: }
Everything in the www/ directory gets shipped as part of the docker image build and is likely to be shipped by other packaging systems. We don't want to ship this file.
I think it would be better for the test that needs it to copy it in temporarily and delete it at the end. Then this file can live in some other location.
--
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: 3
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Sep 2021 23:25:18 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10876: Support to download JWKS from given URL
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/17802 )
Change subject: IMPALA-10876: Support to download JWKS from given URL
......................................................................
IMPALA-10876: Support to download JWKS from given URL
This patch added functionality to download JWKS from a given URL and
support key rotation by periodically checking the JWKS URL for updates.
We use Kudu's EasyCurl wrapper to download file from the given URL.
curl was added to native-toolchain. This patch modified makefiles
and bootstrap_toolchain.py to integrate libcurl and libkudu_curl_util.
Added end-end JWT authentication test cases with JWKS specified as
HTTP/HTTPS URL.
Testing:
- Passed core run, including new test cases.
Change-Id: Ic6ac8cf0010c13db30219776d1d275709bf211df
---
M CMakeLists.txt
M be/CMakeLists.txt
M be/src/kudu/util/CMakeLists.txt
M be/src/rpc/authentication.cc
M be/src/service/impala-server.cc
M be/src/util/jwt-util-internal.h
M be/src/util/jwt-util-test.cc
M be/src/util/jwt-util.cc
M be/src/util/jwt-util.h
M bin/bootstrap_toolchain.py
M bin/impala-config.sh
M bin/rat_exclude_files.txt
A cmake_modules/FindCurl.cmake
M fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java
M fe/src/test/java/org/apache/impala/customcluster/JwtHttpTest.java
M fe/src/test/java/org/apache/impala/customcluster/JwtWebserverTest.java
A testdata/jwt/jwks_es256.json
17 files changed, 570 insertions(+), 140 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/17802/5
--
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: newpatchset
Gerrit-Change-Id: Ic6ac8cf0010c13db30219776d1d275709bf211df
Gerrit-Change-Number: 17802
Gerrit-PatchSet: 5
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>