You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Andrew Wong (Code Review)" <ge...@cloudera.org> on 2022/05/02 23:58:17 UTC
[kudu-CR] jwt: add test for fetching JWKS via URL
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18468
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
jwt: add test for fetching JWKS via URL
Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
3 files changed, 75 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/18468/1
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has uploaded a new patch set (#12) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
jwt: add test for fetching JWKS via URL
Co-authored-by: Zoltan Chovan <zc...@cloudera.com>
Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
3 files changed, 193 insertions(+), 126 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/18468/12
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 12
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has uploaded a new patch set (#22) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
jwt: add test for fetching JWKS via URL
Co-authored-by: Zoltan Chovan <zc...@cloudera.com>
Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/jwt-util-internal.h
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
4 files changed, 257 insertions(+), 198 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/18468/22
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 22
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Patch Set 19:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@193
PS16, Line 193: return Stat
> Yep, that's right
Done
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@307
PS16, Line 307: if (algorithm == "hs256") {
: jwt_pub_key.reset(new HS256JWTPublicKey(algorithm, it_k->second));
: } else if (algorithm == "hs384") {
: jwt_pub_key.reset(new HS384JWTPublicKey(algorithm, it_k->second));
: } else if (algorithm == "hs512") {
: jwt_pub_key.reset(new HS512JWTPublicKey(algorithm, it_k->second));
: } else {
: return Status::InvalidArgument(Substitute("Invalid 'alg' property value: '$0'", algorithm));
: }
: } catch (const std::exception& e) {
: r
> nit for here and elsewhere with allocating JWTPublicKey: since using unique
Done
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@611
PS16, Line 611: // TODO support CurlAuthType
> We should add a flag to turn on verification of the peer's certificate in f
@Alexey, yes this makes sense, I agree we should make it possible to enable peer cert verification. Thanks for the attack vector explanations! As both you and @Wenzhe has mentioned, I'll create a new change to add the flag to do this.
http://gerrit.cloudera.org:8080/#/c/18468/18/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18468/18/src/kudu/util/jwt-util.cc@620
PS18, Line 620: // Append '\0' so t
> We can move line #617 (*is_changed = false) to the beginning of the functio
Done
http://gerrit.cloudera.org:8080/#/c/18468/18/src/kudu/util/jwt-util.cc@739
PS18, Line 739: SetJWKSSnapshot(new_jwks);
: if (new_jwks->IsEmpty()) {
:
> Add a warning log message?
yes it's a leftover, but I do agree with Wenzhe, we should add a warning here
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 19
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Sat, 19 Nov 2022 17:57:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Patch Set 13:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/18468/13/src/kudu/util/jwt-util-internal.h
File src/kudu/util/jwt-util-internal.h:
http://gerrit.cloudera.org:8080/#/c/18468/13/src/kudu/util/jwt-util-internal.h@244
PS13, Line 244: JWKSSnapshot
If JWKS is loaded from remote URL, JWKSSnapshot could be updated. We should define a destructor function, which clean hs_key_map_, rsa_pub_key_map_ and ec_pub_key_map_.
http://gerrit.cloudera.org:8080/#/c/18468/13/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18468/13/src/kudu/util/jwt-util.cc@202
PS13, Line 202: RETURN_NOT_OK(HSJWTPublicKeyBuilder::CreateJWKPublicKey(kv_map, &jwt_pub_key));
: jwks_->AddHSKey(key_id, jwt_pub_key);
> I took a closer look at the code, and I'm not sure I understand what's are
Thanks to catch memory leak issue. Agree to change to unique pointer to avoid memory leak.
There is another memory leak related to saved public keys in jwks_. We should define a destructor for JWKSSnapshot to clean three maps.
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 13
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Sat, 12 Nov 2022 00:17:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has uploaded a new patch set (#16) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
jwt: add test for fetching JWKS via URL
Co-authored-by: Zoltan Chovan <zc...@cloudera.com>
Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/jwt-util-internal.h
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
4 files changed, 223 insertions(+), 149 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/18468/16
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 16
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Patch Set 17:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18468/17/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18468/17/src/kudu/util/jwt-util.cc@22
PS17, Line 22: #include <cerrno>
nit: these should be grouped with <cstring> et al
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 17
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 18 Nov 2022 13:03:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Patch Set 2:
(3 comments)
LINT and IWYU builds are failing, please make sure they pass. Probably, that something related to the parent patch.
http://gerrit.cloudera.org:8080/#/c/18468/2/src/kudu/util/jwt-util-test.cc
File src/kudu/util/jwt-util-test.cc:
http://gerrit.cloudera.org:8080/#/c/18468/2/src/kudu/util/jwt-util-test.cc@1173
PS2, Line 1173: "127.0.0.1"
nit: instead of this, I'd expect setting opts.bind_interface to 127.0.0.1 and then calling addrs[0].ToString()
http://gerrit.cloudera.org:8080/#/c/18468/2/src/kudu/util/jwt-util-test.cc@1201
PS2, Line 1201: picojson
What's picojson?
http://gerrit.cloudera.org:8080/#/c/18468/2/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18468/2/src/kudu/util/jwt-util.cc@696
PS2, Line 696: // This Get() will time out until shutdown, when the promise is set.
: bool timed_out;
: shut_down_promise_.Get(timeout_millis, &timed_out);
: if (!timed_out) break;
I'd suggest get rid of the Get() method which has bogus signature and simply use the Wait(const MonoDelta&) here instead. That should allow to get rid of ASAN warning reported by the pre-commit build.
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 19 Oct 2022 19:54:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has uploaded a new patch set (#6) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
jwt: add test for fetching JWKS via URL
Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
3 files changed, 78 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/18468/6
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has uploaded a new patch set (#11) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
jwt: add test for fetching JWKS via URL
Co-authored-by: Zoltan Chovan <zc...@cloudera.com>
Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
3 files changed, 195 insertions(+), 127 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/18468/11
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 11
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has uploaded a new patch set (#18) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
jwt: add test for fetching JWKS via URL
Co-authored-by: Zoltan Chovan <zc...@cloudera.com>
Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/jwt-util-internal.h
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
4 files changed, 255 insertions(+), 178 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/18468/18
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 18
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Patch Set 18:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util-test.cc
File src/kudu/util/jwt-util-test.cc:
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util-test.cc@410
PS16, Line 410: void TempTestDataFile::Delete() {
: if (deleted_) return;
: deleted_ = true;
: if (remove(name_.c_str()) != 0) {
: std::cout << "Error deleting temp file; " << strerror(errno) << std::endl;
: abort();
: }
: }
> I'm not sure this is needed since the file created with NewTempWritableFile
This function is copied from Impala, which did not use NewTempWritableFile() to create the file. It's not needed.
http://gerrit.cloudera.org:8080/#/c/18468/18/src/kudu/util/jwt-util-test.cc
File src/kudu/util/jwt-util-test.cc:
http://gerrit.cloudera.org:8080/#/c/18468/18/src/kudu/util/jwt-util-test.cc@410
PS18, Line 410: void TempTestDataFile::Delete() {
: if (deleted_) return;
: deleted_ = true;
: if (remove(name_.c_str()) != 0) {
: std::cout << "Error deleting temp file; " << strerror(errno) << std::endl;
: abort();
: }
: }
> I guess this isn't needed once switched to proper usage of NewTempWritableF
Right, we can also remove destructor ~TempTestDataFile()
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@611
PS16, Line 611: RETURN_NOT_OK_PREPEND(curl.F
> I'd suggest to address this in a separate changelist: the context of this p
We should add a flag to turn on verification of the peer's certificate in following patch.
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@615
PS16, Line 615: jwks_checksum_ =
> I guess we can discuss that with Wenzhe and clarify using docs and other in
We should return "is_changed" as true if the server doesn't have any keys in its response so that the caller will call SetJWKSSnapshot() with an empty JWKSSnapshot.
http://gerrit.cloudera.org:8080/#/c/18468/18/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18468/18/src/kudu/util/jwt-util.cc@620
PS18, Line 620: *is_changed = true;
> It seems I missed to point at this earlier, but ideally should be moved to
We can move line #617 (*is_changed = false) to the beginning of the function, and move #620 to #643 before calling "return Status::OK()".
With this change, *is_changed return true for dst.size() as 0. The caller will call SetJWKSSnapshot() with empty JWKS.
http://gerrit.cloudera.org:8080/#/c/18468/18/src/kudu/util/jwt-util.cc@739
PS18, Line 739: if (new_jwks->IsEmpty()) {
: // new_jwks->
: }
> nit: is it some left-over from the editing? If not, maybe add a TODO inste
Add a warning log message?
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 18
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Sat, 19 Nov 2022 00:07:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has uploaded a new patch set (#15) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
jwt: add test for fetching JWKS via URL
Co-authored-by: Zoltan Chovan <zc...@cloudera.com>
Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/jwt-util-internal.h
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
4 files changed, 220 insertions(+), 148 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/18468/15
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 15
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Patch Set 18:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util-internal.h
File src/kudu/util/jwt-util-internal.h:
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util-internal.h@246
PS15, Line 246: public:
> As per Alexey suggested, I've marked this class as final.
My concern is that three maps in this class is not explicitly cleared. Should we add a destructor function to clear the maps?
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 18
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Sat, 19 Nov 2022 00:31:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Patch Set 18: Code-Review+1
(7 comments)
Looks good enough, just some nits left.
I'd suggest to address the rest of those non-nit issues in a separate changelist: as I understand, the original scope of this patch is just to add an extra test.
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util-test.cc
File src/kudu/util/jwt-util-test.cc:
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util-test.cc@397
PS16, Line 397: NewTempWritableFile(opts, &name_[0], &created_filename, &tmp_file);
> So the TempTestDataFile class should have a std::unique_ptr<WritableFile> t
Right: I'd think it worked as that. And once the instance of this file is destroyed, the temporary file is automatically deleted when the destructor for the tmp_file_ member is called.
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util-test.cc@1225
PS16, Line 1225:
> JWKSMockServer will be replaced in a followup patch with the MiniOIDC compo
I'm totally fine with addressing that in its own patch, especially if some of the components are going to be replaced in future patches, sure.
I'm advocating for more test coverage not because of some whim, but because of the following:
* being more explicit how those components behave: it helps us better understand what's our expectations for this new JWT functionality
* make sure those 'negative' scenarios are handled as expected: nothing crashes, no undefined behavior is manifested
* be able to spot regressions in the future
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@193
PS16, Line 193: return Stat
> since the FindOrNull returns the value already from the map, this line beco
Yep, that's right
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@611
PS16, Line 611: RETURN_NOT_OK_PREPEND(curl.F
> We can add a flag for this, but I'm not sure I understand the attack vector
I'd suggest to address this in a separate changelist: the context of this patch has become too bloated already.
As for possible attack vectors.
If somebody gives us wrong public keys, then all valid tokens suddenly start failing upon an attempt to verify them. That's a vector for DOS attacks.
If a client is given a JWT generated by a malicious actor and signed with their private key, once they are able to substitute authentic public keys with their own, suddenly their tokens are valid and the system will allow those clients to authenticate. That gives the malicious party a way to start using the system, allowing non-authorized access.
Does it make sense or those concerns are not valid becaues I'm missing something?
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@615
PS16, Line 615: jwks_checksum_ =
> I haven't really found anything about this in example implementations. Howe
I guess we can discuss that with Wenzhe and clarify using docs and other information available so far. I'd suggest to address this in a separate changelist since the scope of this review item is already pretty bloated (it started with just adding an extra test).
http://gerrit.cloudera.org:8080/#/c/18468/18/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18468/18/src/kudu/util/jwt-util.cc@620
PS18, Line 620: *is_changed = true;
It seems I missed to point at this earlier, but ideally should be moved to just precede 'return Status::OK()' at 643. Nothing is actually changed before the newly downloaded content is successfully parsed at line 641, right?
http://gerrit.cloudera.org:8080/#/c/18468/18/src/kudu/util/jwt-util.cc@739
PS18, Line 739: if (new_jwks->IsEmpty()) {
: // new_jwks->
: }
nit: is it some left-over from the editing? If not, maybe add a TODO instead with explicit description what's to address here?
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 18
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 18 Nov 2022 19:12:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
jwt: add test for fetching JWKS via URL
Co-authored-by: Zoltan Chovan <zc...@cloudera.com>
Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Reviewed-on: http://gerrit.cloudera.org:8080/18468
Tested-by: Alexey Serbin <al...@apache.org>
Reviewed-by: Alexey Serbin <al...@apache.org>
Reviewed-by: Wenzhe Zhou <wz...@cloudera.com>
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/jwt-util-internal.h
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
4 files changed, 257 insertions(+), 198 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved; Verified
Wenzhe Zhou: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 23
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util-test.cc
File src/kudu/util/jwt-util-test.cc:
http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util-test.cc@1220
PS7, Line 1220:
Could you add a new test case for which JWKS on Web server is changed and verify new snapshot of JWKS is created by downloading new JWKS?
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Mon, 07 Nov 2022 22:40:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Patch Set 13:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/18468/2/src/kudu/util/jwt-util-test.cc
File src/kudu/util/jwt-util-test.cc:
http://gerrit.cloudera.org:8080/#/c/18468/2/src/kudu/util/jwt-util-test.cc@1173
PS2, Line 1173:
> It seems this one was missed.
Ack
http://gerrit.cloudera.org:8080/#/c/18468/5/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18468/5/src/kudu/util/jwt-util.cc@84
PS5, Line 84: return true;
: }
:
: DEFINE_validator(jwks_update_frequency_s, &ValidateBiggerThanZero);
: DEFINE_validator(jwks_pulling_timeout_s, &ValidateBiggerThanZero);
:
: namespace kudu {
> nit: instead of duplicating code, maybe introduce a method and use it for b
I already fixed this, you might have been looking at an earlier changeset.
http://gerrit.cloudera.org:8080/#/c/18468/5/src/kudu/util/jwt-util.cc@209
PS5, Line 209: JWTPublicKey* jwt_pub_key;
: RETURN_NOT_OK(ECJWTPublicKeyBuilder::CreateJWKPublicKe
> here and below: why not to use RETURN_NOT_OK() instead? The result status
Done
http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util.cc@267
PS7, Line 267: try {
: // Call jwt-cpp API to verify token's signature.
: verifier_.verify(decoded_jwt);
: } catch (const jwt::error::rsa_exception& e) {
: return Status::NotAuthorized(Substitute("RSA error: $0", e.what()));
: } catch (const jwt::error::token_verification_exception& e) {
: return Status::NotAuthorized(
: Substitute("Verification failed, error: $0", e.what()));
: } catch (const std::exception& e) {
: return Status::NotAuthorized(
: Substitute("Verification failed, error: $0", e.what()));
: }
: return Status::OK();
: }
> nit: assigning status and continuing further hints that there is some non-t
Done
http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util.cc@526
PS7, Line 526: BN_bin2bn(reinterpret_cast<const unsigned char*>(ascii_x.c_str()),
: static_cast<int>(ascii_x.size()),
: nullptr),
> nit: could use "c_unique_ptr<BIGNUM> x { BN_bin2bn(...), &BN_free }" here i
Done
http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util.cc@534
PS7, Line 534: nullptr),
> nit: could use c_unique_ptr<EC_KEY> ecKey { EC_KEY_new_by_curve_name(eccgrp
Done
http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util.cc@538
PS7, Line 538: security::c_unique_ptr<EC_KEY> ecKey { EC_KEY_new_by_curve_name(eccgrp), &EC_KEY_free };
: EC_KEY_set_asn1_flag(e
> Is these two lines result in the same as
yepp, changed it
http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util.cc@540
PS7, Line 540: if (EC_KEY_set_public_key_a
> nit: could use 'auto bio = ssl_make_unique(BIO_new(BIO_s_mem()))' and get r
Done
http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util.cc@542
PS7, Line 542: unsigned char desc[1024] = { 0 };
: auto bio = security::ssl_make_unique(BIO_new
> Per https://www.openssl.org/docs/man1.1.1/man3/BIO_read.html I cannot see t
Ack
http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util.cc@902
PS7, Line 902: return Status::NotAuthorized(
: Substitute("Claim '$0' was not present", jwt_custom_claim_username));
: }
:
: username = decoded_jwt.get_payload_claim(jwt_custom_claim_username).to_json().to_str();
:
: if (username.empty()) {
: return Status::NotAuthorized(
: Substitute("Claim '$0' is empty", jwt_custom_claim_username));
: }
:
: } catch (const std::runtime_error& e) {
: return Status::NotAuthorized(
: Substitute("Claim '$0' was not present, error: $1", jwt_custom_claim_username,
:
> nit: this could be a bit more readable
Done
http://gerrit.cloudera.org:8080/#/c/18468/11/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18468/11/src/kudu/util/jwt-util.cc@30
PS11, Line 30: #include <openssl/ssl.h>
: #include <openssl
> nit: move these two lines to line #23 to keep alphabet order
Done
http://gerrit.cloudera.org:8080/#/c/18468/11/src/kudu/util/jwt-util.cc@405
PS11, Line 405: nullptr)
> nit: alignment
Done
http://gerrit.cloudera.org:8080/#/c/18468/11/src/kudu/util/jwt-util.cc@717
PS11, Line 717: JWKSMgr::UpdateJWKSThread() {
: std::shared_ptr<JWKSSna
> rebase with parent patch
Done
http://gerrit.cloudera.org:8080/#/c/18468/11/src/kudu/util/jwt-util.cc@872
PS11, Line 872: return Status::NotA
> return status;
Done
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 13
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 11 Nov 2022 19:47:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has uploaded a new patch set (#8) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
jwt: add test for fetching JWKS via URL
Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
3 files changed, 163 insertions(+), 116 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/18468/8
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 8
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has uploaded a new patch set (#21) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
jwt: add test for fetching JWKS via URL
Co-authored-by: Zoltan Chovan <zc...@cloudera.com>
Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/jwt-util-internal.h
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
4 files changed, 257 insertions(+), 198 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/18468/21
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 21
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Patch Set 22:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util-internal.h
File src/kudu/util/jwt-util-internal.h:
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util-internal.h@246
PS15, Line 246: public:
> My concern is that three maps in this class is not explicitly cleared. Shou
Why would we want to clear those three maps explicitly? Isn't it enough that those are cleared upon destruction of the object they are encapsulated into?
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 22
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Mon, 21 Nov 2022 20:10:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Patch Set 15:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util-internal.h
File src/kudu/util/jwt-util-internal.h:
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util-internal.h@28
PS15, Line 28: #include <memory>
> nit: move this into the group of the standard C++ headers above
Done
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util-internal.h@48
PS15, Line 48:
> nit: the indent is off (should be 2 spaces from the beginning of the line)
Done
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util-internal.h@247
PS15, Line 247:
> nit: the indent is off
Done
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util-internal.h@292
PS15, Line 292: &
> remove &
Done
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util.cc@56
PS15, Line 56: #include <memory>
> don't need this line since it's already included in jwt-util.h
Done
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util.cc@202
PS15, Line 202: std::unique_ptr<JWTPublicKey> jwt_pub_key;
: RETURN_NOT_OK(HSJWTPublicKeyBuilder::CreateJWKPublicKey(kv_map, jwt_pub_key));
: jwks_->AddHSKey(key_id, jwt_pub_key);
> I thought it would be something like below:
Done
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util.cc@285
PS15, Line 285:
> nit: 4 spaces
Done
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util.cc@329
PS15, Line 329:
> nit: 4 spaces
Done
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 15
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Thu, 17 Nov 2022 17:27:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Patch Set 15:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util-internal.h
File src/kudu/util/jwt-util-internal.h:
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util-internal.h@28
PS15, Line 28: #include <memory>
nit: move this into the group of the standard C++ headers above
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util-internal.h@48
PS15, Line 48:
nit: the indent is off (should be 2 spaces from the beginning of the line)
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util-internal.h@209
PS15, Line 209: std::unique_ptr<JWTPublicKey>&
style (here and for all other methods): for output parameters, use pointers, not references.
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util-internal.h@247
PS15, Line 247:
nit: the indent is off
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util.cc@202
PS15, Line 202: std::unique_ptr<JWTPublicKey> jwt_pub_key;
: RETURN_NOT_OK(HSJWTPublicKeyBuilder::CreateJWKPublicKey(kv_map, jwt_pub_key));
: jwks_->AddHSKey(key_id, jwt_pub_key);
I thought it would be something like below:
unique_ptr<JWTPublicKey> jwt_pub_key;
RETURN_NOT_OK(HSJWTPublicKeyBuilder::CreateJWKPublicKey(kv_map, &jwt_pub_key));
jwks_->AddHSKey(key_id, std::move(jwt_pub_key));
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 15
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Thu, 17 Nov 2022 03:46:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Patch Set 16:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util-internal.h
File src/kudu/util/jwt-util-internal.h:
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util-internal.h@246
PS15, Line 246: public:
> Add destructor function
Even the public keys in JWTPublicKeyMap are unique pointer, we still need to clean the maps in destructor.
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 16
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Thu, 17 Nov 2022 19:01:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Patch Set 16:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util-internal.h
File src/kudu/util/jwt-util-internal.h:
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util-internal.h@246
PS15, Line 246: public:
> Even the public keys in JWTPublicKeyMap are unique pointer, we still need t
Wenzhe, is your concern about adding virtual destructor in case if this class is used as a base in the future?
That's a good point, and as alternative it's possible to declare this class as final (Kudu uses C++17, so it's possible to use final classes).
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util-test.cc
File src/kudu/util/jwt-util-test.cc:
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util-test.cc@397
PS16, Line 397: NewTempWritableFile(opts, &name_[0], &created_filename, &tmp_file);
The underlying tmp_file will be deleted automatically when 'tmp_file' goes out of scope, i.e. once the constructor is done. I guess it's necessary to make std::unique_ptr<WritableFile> a member of this class to avoid this to happen?
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util-test.cc@410
PS16, Line 410: void TempTestDataFile::Delete() {
: if (deleted_) return;
: deleted_ = true;
: if (remove(name_.c_str()) != 0) {
: std::cout << "Error deleting temp file; " << strerror(errno) << std::endl;
: abort();
: }
: }
I'm not sure this is needed since the file created with NewTempWritableFile isn't supposed to be here once its unique_ptr wrapper goes out of scope.
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util-test.cc@1225
PS16, Line 1225:
Could we add coverage for negative scenarios as well?
* server is not yet started upon calling JWTHelper::Init()
* server is started with different algorithm than the created JWT token
* server was running upon calling JWTHelper::Init(), but shutdown when calling JWTHelper::Verify() on a token
* server is started with different algorithm than the created token, but later on the algorithm is updated to match the algorithm (or the corresponding algorithm is added)
* server was started with necessary algorithm, but later on it was restarted with an empty set of keys: what happens when attempting to verify a token that was valid some time ago when the server supplied the key matching the token, but now no key are supplied by the server?
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@182
PS16, Line 182: if (kv_map.find(key) == kv_map.end()) {
: kv_map.insert(make_pair(key, value));
: } else {
: LOG(WARNING) << "Duplicate property of JWK: " << key;
: }
nit: to avoid double lookups, this could be
if (!EmplaceIfNotPresent(&kv_map, key, value)) {
LOG(WARNING) << "Duplicate property of JWK: " << key;
}
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@189
PS16, Line 189: kv_map.find
nit: we strive to use wrappers from map-util.h for things like this, so consider updating this piece to use the following pattern
const auto* kty = FindOrNull(kv_map, "kty");
if (!kty) {
return Status::InvalidArgument("'kty' property is required");
}
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@193
PS16, Line 193: string key_id
nit: could this be 'const auto& key_id' to avoid copying?
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@201
PS16, Line 201: std::unique_ptr
nit for here and below: you could add 'using std::unique_ptr;' to file file and drop the 'std::' namespace prefix
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@307
PS16, Line 307: JWTPublicKey* jwt_pub_key = nullptr;
: try {
: if (algorithm == "hs256") {
: jwt_pub_key = new HS256JWTPublicKey(algorithm, it_k->second);
: } else if (algorithm == "hs384") {
: jwt_pub_key = new HS384JWTPublicKey(algorithm, it_k->second);
: } else if (algorithm == "hs512") {
: jwt_pub_key = new HS512JWTPublicKey(algorithm, it_k->second);
: } else {
: return Status::InvalidArgument(Substitute("Invalid 'alg' property value: '$0'", algorithm));
: }
nit for here and elsewhere with allocating JWTPublicKey: since using unique_ptr, it would make sense to update this to be more leak-proof (there isn't a leak possibility right now, but it might suddenly appear if this code modified later):
unique_ptr<JWTPublicKey> jwt_pub_key;
...
if (...) {
jwt_pub_key.reset(new HS256JWTPublicKey(...));
} else if (...) {
jwt_pub_key.reset(new ...);
}
*pub_key_out = std::move(jwt_pub_key);
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@611
PS16, Line 611: curl.set_verify_peer(false);
That looks pretty weird since it opens all this JWT stuff to various MITM attacks. Should we at least add a flag to be able to turn verification of the peer's certificate on and off, and by default it should be on, while for testing purposes we can turn off the verification of the server's certificate?
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@615
PS16, Line 615: if (dst.size() > 0)
What if the server doesn't have any keys in its response? Should the already known keys be removed in that case?
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@618
PS16, Line 618: if (jwks_checksum_ == cur_jwks_checksum) return Status::OK();
Should the code set '*is_changed = false;' here before returning from this method? Otherwise, the caller might be reading uninitialized variable passed to 'is_changed' as an output parameter.
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@707
PS16, Line 707: impala-server
Why is this set to "impala-server"? Maybe, set this to "JWT" or something like that?
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 16
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 18 Nov 2022 02:07:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has uploaded a new patch set (#20) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
jwt: add test for fetching JWKS via URL
Co-authored-by: Zoltan Chovan <zc...@cloudera.com>
Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/jwt-util-internal.h
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
4 files changed, 259 insertions(+), 195 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/18468/20
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 20
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 11
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Patch Set 13:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18468/13/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18468/13/src/kudu/util/jwt-util.cc@201
PS13, Line 201: jwt_pub_key
could we initialize this pointer as nullptr to avoid warning?
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 13
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 11 Nov 2022 19:53:53 +0000
Gerrit-HasComments: Yes
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Patch Set 13:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/18468/13/src/kudu/util/jwt-util-test.cc
File src/kudu/util/jwt-util-test.cc:
http://gerrit.cloudera.org:8080/#/c/18468/13/src/kudu/util/jwt-util-test.cc@1194
PS13, Line 1194: Sockaddr addr_;
nit: it seems this is unused, as pointed by Wenzhe in one of earlier revisions
http://gerrit.cloudera.org:8080/#/c/18468/13/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18468/13/src/kudu/util/jwt-util.cc@202
PS13, Line 202: RETURN_NOT_OK(HSJWTPublicKeyBuilder::CreateJWKPublicKey(kv_map, &jwt_pub_key));
: jwks_->AddHSKey(key_id, jwt_pub_key);
I took a closer look at the code, and I'm not sure I understand what's are the rules of ownership for the new keys allocated by various CreateJWKPublicKey() methods.
One issue I see that those raw pointers are put into corresponding maps by jwks_->AddHSKey() and methods alike. However, if the key is already there, nothing is done (except for logging a warning) and the 'jwt_pub_key' is then leaked.
With that, does it make sense to change the signature of CreateJWKPublicKey() methods and AddXxxPublicKey() methods to operate with unique pointers instead of raw pointers to avoid memory leaks in such situations?
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 13
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 11 Nov 2022 21:21:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has uploaded a new patch set (#9) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
jwt: add test for fetching JWKS via URL
Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
3 files changed, 194 insertions(+), 127 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/18468/9
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 9
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Patch Set 11: Verified+1
unrelated java test failure (TSAN build)
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 11
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 11 Nov 2022 19:06:20 +0000
Gerrit-HasComments: No
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Patch Set 2:
> Uploaded patch set 8.
Many JWT tests failed, and LINT and IWYU builds are failing as well.
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Nov 2022 20:02:05 +0000
Gerrit-HasComments: No
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Patch Set 15:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util-internal.h
File src/kudu/util/jwt-util-internal.h:
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util-internal.h@246
PS15, Line 246:
Add destructor function
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util-internal.h@292
PS15, Line 292: &
remove &
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util.cc@56
PS15, Line 56: #include <memory>
don't need this line since it's already included in jwt-util.h
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util.cc@285
PS15, Line 285:
nit: 4 spaces
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util.cc@329
PS15, Line 329:
nit: 4 spaces
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 15
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Thu, 17 Nov 2022 05:59:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Patch Set 14:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/18468/13/src/kudu/util/jwt-util-test.cc
File src/kudu/util/jwt-util-test.cc:
http://gerrit.cloudera.org:8080/#/c/18468/13/src/kudu/util/jwt-util-test.cc@1194
PS13, Line 1194:
> nit: it seems this is unused, as pointed by Wenzhe in one of earlier revisi
Ack
http://gerrit.cloudera.org:8080/#/c/18468/13/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18468/13/src/kudu/util/jwt-util.cc@202
PS13, Line 202: std::unique_ptr<JWTPublicKey> jwt_pub_key;
: RETURN_NOT_OK(HSJWTPublicKeyBuilder::
> Thanks to catch memory leak issue. Agree to change to unique pointer to avo
I've updated the signatures of the AddXXKey methods to pass unique_ptrs. As for creating a destructor, it's not necessary, since the key maps are JWTPublicKeyMaps, which is:
typedef std::unordered_map<std::string, std::unique_ptr<JWTPublicKey>> JWTPublicKeyMap;
so the cleanup should be automatic.
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 14
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 16 Nov 2022 19:28:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Patch Set 22: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 22
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Mon, 21 Nov 2022 21:52:13 +0000
Gerrit-HasComments: No
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Patch Set 11:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/18468/11/src/kudu/util/jwt-util-test.cc
File src/kudu/util/jwt-util-test.cc:
http://gerrit.cloudera.org:8080/#/c/18468/11/src/kudu/util/jwt-util-test.cc@1194
PS11, Line 1194: Sockaddr addr_
not used
http://gerrit.cloudera.org:8080/#/c/18468/11/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18468/11/src/kudu/util/jwt-util.cc@30
PS11, Line 30: #include <cstdint>
: #include <cstdio>
nit: move these two lines to line #23 to keep alphabet order
http://gerrit.cloudera.org:8080/#/c/18468/11/src/kudu/util/jwt-util.cc@405
PS11, Line 405: &BN_free
nit: alignment
http://gerrit.cloudera.org:8080/#/c/18468/11/src/kudu/util/jwt-util.cc@717
PS11, Line 717: bool timed_out = shut_down_promise_.WaitFor(timeout);
: if (timed_out) break;
rebase with parent patch
http://gerrit.cloudera.org:8080/#/c/18468/11/src/kudu/util/jwt-util.cc@872
PS11, Line 872: RETURN_NOT_OK(status)
return status;
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 11
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 11 Nov 2022 19:42:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has uploaded a new patch set (#7) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
jwt: add test for fetching JWKS via URL
Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
3 files changed, 83 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/18468/7
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Patch Set 7:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/18468/2/src/kudu/util/jwt-util-test.cc
File src/kudu/util/jwt-util-test.cc:
http://gerrit.cloudera.org:8080/#/c/18468/2/src/kudu/util/jwt-util-test.cc@1173
PS2, Line 1173:
> Done
It seems this one was missed.
http://gerrit.cloudera.org:8080/#/c/18468/2/src/kudu/util/jwt-util-test.cc@1201
PS2, Line 1201:
> picojson is the JSON library that the jwt-cpp library is using to handle JS
Thanks for the clarification.
http://gerrit.cloudera.org:8080/#/c/18468/5/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18468/5/src/kudu/util/jwt-util.cc@84
PS5, Line 84: }
:
: DEFINE_validator(jwks_update_frequency_s, &ValidateBiggerThanZero);
: DEFINE_validator(jwks_pulling_timeout_s, &ValidateBiggerThanZero);
:
: namespace kudu {
:
nit: instead of duplicating code, maybe introduce a method and use it for both validators?
http://gerrit.cloudera.org:8080/#/c/18468/5/src/kudu/util/jwt-util.cc@209
PS5, Line 209: JWTPublicKey* jwt_pub_key;
: status = ECJWTPublicKeyBuilder::CreateJWKPublicKey(kv_
here and below: why not to use RETURN_NOT_OK() instead? The result status isn't used below expect for just 'return status' anyways.
http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util.cc@267
PS7, Line 267: Status status;
: try {
: // Call jwt-cpp API to verify token's signature.
: verifier_.verify(decoded_jwt);
: } catch (const jwt::error::rsa_exception& e) {
: status = Status::NotAuthorized(Substitute("RSA error: $0", e.what()));
: } catch (const jwt::error::token_verification_exception& e) {
: status = Status::NotAuthorized(
: Substitute("Verification failed, error: $0", e.what()));
: } catch (const std::exception& e) {
: status = Status::NotAuthorized(
: Substitute("Verification failed, error: $0", e.what()));
: }
: return status;
nit: assigning status and continuing further hints that there is some non-trivial activity before calling 'return status' but de-facto there is nothing.
Maybe, change this pattern here and elsewhere in this file to be simply
try {
...
} catch (...) {
return Status::NotAuthorized(...);
} catch (...) {
return Status::NotAuthorized(...);
}
return Status::OK();
?
http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util.cc@381
PS7, Line 381: status = Status::NotAuthorized(Substitute("RSA error: $0", e.what()));
: RETURN_NOT_OK(status);
It seems that eventually became an oxymoron:
RETURN_NOT_OK(status) will always return right away due to the assignment at the line above.
Maybe, change these two lines here and below to
return Status::NotAuthorized(...);
?
http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util.cc@526
PS7, Line 526: BIGNUM* x = BN_bin2bn(reinterpret_cast<const unsigned char*>(ascii_x.c_str()),
: static_cast<int>(ascii_x.size()),
: nullptr);
nit: could use "c_unique_ptr<BIGNUM> x { BN_bin2bn(...), &BN_free }" here instead and get rid of BN_free() at line 554
http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util.cc@534
PS7, Line 534: EC_KEY* ecKey = EC_KEY_new_by_curve_name(eccgrp);
nit: could use c_unique_ptr<EC_KEY> ecKey { EC_KEY_new_by_curve_name(eccgrp), &EC_KEY_free } instead and get rid of EC_KEY_free(ecKey) at line 553.
http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util.cc@538
PS7, Line 538: unsigned char desc[1024];
: memset(desc, 0, 1024);
Is these two lines result in the same as
unsigned char desc[1024] = { 0 };
?
http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util.cc@540
PS7, Line 540: bio = BIO_new(BIO_s_mem());
nit: could use 'auto bio = ssl_make_unique(BIO_new(BIO_s_mem()))' and get rid of line 552.
http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util.cc@542
PS7, Line 542: if (BIO_read(bio, desc, 1024) > 0) {
: pub_key = reinterpret_cast<char*>(desc);
Per https://www.openssl.org/docs/man1.1.1/man3/BIO_read.html I cannot see that the result of BIO_read() is always NULL-terminated, so if there is 1024 or more bytes to read in the 'bio', I suspect there might be a buffer overrun when doing "pub_key = reinterpret_cast<char*>(desc);"
I'd suggest reading one less byte than the size of the array, assuming the array is zeroed.
http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util.cc@902
PS7, Line 902: // Get value of custom claim 'username' from the token payload.
: if (decoded_jwt.has_payload_claim(jwt_custom_claim_username)) {
: // Assume the claim data type of 'username' is string.
: username.assign(
: decoded_jwt.get_payload_claim(jwt_custom_claim_username)
: .to_json()
: .to_str());
: if (username.empty()) {
: status = Status::NotAuthorized(
: Substitute("Claim '$0' is empty", jwt_custom_claim_username));
: }
: } else {
: status = Status::NotAuthorized(
: Substitute("Claim '$0' was not present", jwt_custom_claim_username));
: }
nit: this could be a bit more readable
if (!decoded_jwt.has_payload_claim(jwt_custom_claim_username)) {
return Status::NotAuthorized(...);
}
username = decoded_jwt.get_payload_claim(jwt_custom_claim_username).to_json().to_str();
if (username.empty()) {
return Status::NotAuthorized();
}
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Nov 2022 03:20:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has uploaded a new patch set (#19) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
jwt: add test for fetching JWKS via URL
Co-authored-by: Zoltan Chovan <zc...@cloudera.com>
Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/jwt-util-internal.h
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
4 files changed, 257 insertions(+), 179 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/18468/19
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 19
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Patch Set 22: Verified+1
unrelated test failure in kudu.tests.test_scanner.TestScanner.test_varchar_pred
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 22
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Mon, 21 Nov 2022 21:48:06 +0000
Gerrit-HasComments: No
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Patch Set 22: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 22
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Mon, 21 Nov 2022 21:57:31 +0000
Gerrit-HasComments: No
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Patch Set 16: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 16
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Thu, 17 Nov 2022 18:06:33 +0000
Gerrit-HasComments: No
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Patch Set 18:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util-test.cc
File src/kudu/util/jwt-util-test.cc:
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util-test.cc@397
PS16, Line 397: NewTempWritableFile(opts, &name_[0], &created_filename, &tmp_file);
> Right: I'd think it worked as that. And once the instance of this file is
s/... instance of this file .../... instance of this class .../
http://gerrit.cloudera.org:8080/#/c/18468/18/src/kudu/util/jwt-util-test.cc
File src/kudu/util/jwt-util-test.cc:
http://gerrit.cloudera.org:8080/#/c/18468/18/src/kudu/util/jwt-util-test.cc@410
PS18, Line 410: void TempTestDataFile::Delete() {
: if (deleted_) return;
: deleted_ = true;
: if (remove(name_.c_str()) != 0) {
: std::cout << "Error deleting temp file; " << strerror(errno) << std::endl;
: abort();
: }
: }
I guess this isn't needed once switched to proper usage of NewTempWritableFile.
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 18
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 18 Nov 2022 19:31:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has uploaded a new patch set (#14) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
jwt: add test for fetching JWKS via URL
Co-authored-by: Zoltan Chovan <zc...@cloudera.com>
Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/jwt-util-internal.h
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
4 files changed, 220 insertions(+), 148 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/18468/14
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 14
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Patch Set 18:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util-internal.h
File src/kudu/util/jwt-util-internal.h:
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util-internal.h@246
PS15, Line 246: public:
> Wenzhe, is your concern about adding virtual destructor in case if this cla
As per Alexey suggested, I've marked this class as final.
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util-test.cc
File src/kudu/util/jwt-util-test.cc:
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util-test.cc@397
PS16, Line 397: NewTempWritableFile(opts, &name_[0], &created_filename, &tmp_file);
> The underlying tmp_file will be deleted automatically when 'tmp_file' goes
So the TempTestDataFile class should have a std::unique_ptr<WritableFile> tmp_file_ member?
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util-test.cc@1225
PS16, Line 1225:
> Could we add coverage for negative scenarios as well?
JWKSMockServer will be replaced in a followup patch with the MiniOIDC component. I think it might be better to cover these scenarios (some are already covered) there. What do you think?
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@182
PS16, Line 182: RETURN_NOT_OK(ReadKeyProperty(key, json_key, &value, /*required*/ false));
: if (!EmplaceIfNotPresent(&kv_map, key, value)) {
: LOG(WARNING) << "Duplicate property of JWK: " << key;
: }
: }
> nit: to avoid double lookups, this could be
Done
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@189
PS16, Line 189: return Sta
> nit: we strive to use wrappers from map-util.h for things like this, so con
Done
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@193
PS16, Line 193: return Stat
> nit: could this be 'const auto& key_id' to avoid copying?
since the FindOrNull returns the value already from the map, this line becomes unneccesary
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@201
PS16, Line 201: jwks_->AddHSKey
> nit for here and below: you could add 'using std::unique_ptr;' to file file
Done
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@611
PS16, Line 611: RETURN_NOT_OK_PREPEND(curl.F
> That looks pretty weird since it opens all this JWT stuff to various MITM a
We can add a flag for this, but I'm not sure I understand the attack vector. The information in the JWKS is basically public keys for the generated tokens for verification.
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@615
PS16, Line 615: jwks_checksum_ =
> What if the server doesn't have any keys in its response? Should the alrea
I haven't really found anything about this in example implementations. However the comment in the header states the following:
// Download JWKS JSON file from the given URL, then load the public keys if the
// checksum of JWKS object is changed. If no keys were given in the URL, the internal
// maps will be empty.
So I think the known keys should be removed. However, currently the ParseKeys() method returns with an invalid argument status when there are no keys in the newly fetched jwks object, see line 154 of this file.
Should I update the behaviour to match the comments?
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@618
PS16, Line 618: return Status::OK();
> Should the code set '*is_changed = false;' here before returning from this
Done
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@707
PS16, Line 707: eriodically c
> Why is this set to "impala-server"? Maybe, set this to "JWT" or something
Done
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 18
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 18 Nov 2022 17:25:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/18468/2/src/kudu/util/jwt-util-test.cc
File src/kudu/util/jwt-util-test.cc:
http://gerrit.cloudera.org:8080/#/c/18468/2/src/kudu/util/jwt-util-test.cc@1173
PS2, Line 1173: "127.0.0.1"
> nit: instead of this, I'd expect setting opts.bind_interface to 127.0.0.1 a
Done
http://gerrit.cloudera.org:8080/#/c/18468/2/src/kudu/util/jwt-util-test.cc@1201
PS2, Line 1201: picojson
> What's picojson?
picojson is the JSON library that the jwt-cpp library is using to handle JSON data
http://gerrit.cloudera.org:8080/#/c/18468/2/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18468/2/src/kudu/util/jwt-util.cc@696
PS2, Line 696: shut_down_promise_.Set(true);
: if (jwks_update_thread_ != nullptr) jwks_update_thread_->Join();
: }
:
> I'd suggest get rid of the Get() method which has bogus signature and simpl
Done
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Mon, 07 Nov 2022 11:00:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has uploaded a new patch set (#3) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
jwt: add test for fetching JWKS via URL
Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
3 files changed, 72 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/18468/3
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has uploaded a new patch set (#5) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
jwt: add test for fetching JWKS via URL
Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
3 files changed, 78 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/18468/5
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has uploaded a new patch set (#4) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
jwt: add test for fetching JWKS via URL
Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
3 files changed, 74 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/18468/4
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has uploaded a new patch set (#10) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
jwt: add test for fetching JWKS via URL
Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
3 files changed, 195 insertions(+), 127 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/18468/10
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 10
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has uploaded a new patch set (#13) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
jwt: add test for fetching JWKS via URL
Co-authored-by: Zoltan Chovan <zc...@cloudera.com>
Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
3 files changed, 194 insertions(+), 126 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/18468/13
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 13
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Patch Set 13:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18468/11/src/kudu/util/jwt-util-test.cc
File src/kudu/util/jwt-util-test.cc:
http://gerrit.cloudera.org:8080/#/c/18468/11/src/kudu/util/jwt-util-test.cc@1194
PS11, Line 1194: Sockaddr addr_
> not used
Should we keep this variable?
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 13
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 11 Nov 2022 19:56:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Patch Set 20:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util-test.cc
File src/kudu/util/jwt-util-test.cc:
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util-test.cc@397
PS16, Line 397: e(Env::Default(), contents, created_filename);
> s/... instance of this file .../... instance of this class .../
Done
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util-test.cc@410
PS16, Line 410: JWTHelper jwt_helper;
: Status status = jwt_helper.Init(jwks_file.Filename(), true);
: EXPECT_OK(status);
: JWKSSnapshotPtr jwks = jwt_helper.GetJWKS();
: ASSERT_FALSE(jwks->IsEmpty());
: ASSERT_EQ(2, jwks->GetRSAPublicKeyNum());
:
:
> This function is copied from Impala, which did not use NewTempWritableFile(
Done
http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util-test.cc@1225
PS16, Line 1225:
> I'm totally fine with addressing that in its own patch, especially if some
I absolutely agree with adding the mentioned test scenarios, but also agree that this commit is not the best place for it. I'll make sure they are included in a following patch.
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 20
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Sat, 19 Nov 2022 18:59:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has uploaded a new patch set (#17) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
jwt: add test for fetching JWKS via URL
Co-authored-by: Zoltan Chovan <zc...@cloudera.com>
Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/jwt-util-internal.h
M src/kudu/util/jwt-util-test.cc
M src/kudu/util/jwt-util.cc
4 files changed, 223 insertions(+), 149 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/18468/17
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 17
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 22
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] jwt: add test for fetching JWKS via URL
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL
......................................................................
Patch Set 22:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util-internal.h
File src/kudu/util/jwt-util-internal.h:
http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util-internal.h@246
PS15, Line 246: public:
> Why would we want to clear those three maps explicitly? Isn't it enough th
Right, all standard containers in c++ follow RAII and there's no need to explicitly clear().
--
To view, visit http://gerrit.cloudera.org:8080/18468
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8
Gerrit-Change-Number: 18468
Gerrit-PatchSet: 22
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Mon, 21 Nov 2022 20:59:20 +0000
Gerrit-HasComments: Yes