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:12 UTC
[kudu-CR] util: pull jwt code from Impala
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18467
Change subject: util: pull jwt code from Impala
......................................................................
util: pull jwt code from Impala
Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/jwt-util-internal.h
A src/kudu/util/jwt-util-test.cc
A src/kudu/util/jwt-util.cc
A src/kudu/util/jwt-util.h
M src/kudu/util/promise.h
6 files changed, 2,541 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/18467/1
--
To view, visit http://gerrit.cloudera.org:8080/18467
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
Gerrit-Change-Number: 18467
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
[kudu-CR] util: pull jwt code from Impala
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/18467 )
Change subject: util: pull jwt code from Impala
......................................................................
util: pull jwt code from Impala
Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/jwt-util-internal.h
A src/kudu/util/jwt-util-test.cc
A src/kudu/util/jwt-util.cc
A src/kudu/util/jwt-util.h
M src/kudu/util/promise.h
6 files changed, 2,528 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/18467/3
--
To view, visit http://gerrit.cloudera.org:8080/18467
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
Gerrit-Change-Number: 18467
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] util: pull jwt code from Impala
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/18467 )
Change subject: util: pull jwt code from Impala
......................................................................
util: pull jwt code from Impala
Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/jwt-util-internal.h
A src/kudu/util/jwt-util-test.cc
A src/kudu/util/jwt-util.cc
A src/kudu/util/jwt-util.h
M src/kudu/util/promise.h
6 files changed, 2,567 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/18467/6
--
To view, visit http://gerrit.cloudera.org:8080/18467
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
Gerrit-Change-Number: 18467
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] util: pull jwt code from Impala
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 )
Change subject: util: pull jwt code from Impala
......................................................................
Patch Set 8:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@272
PS2, Line 272: otAut
> Did you miss to address this comment or there is some other reason behind?
This is a bit weird, I think it's okay if we move these to constant, but they are not used anywhere else. At the end these only indicate which struct to pass to the verifier's allow_algorithm method as a parameter. Unfortunately the allow_algorithm's parameter is a struct (jwt::algorithm::*) from the jwt-cpp thirdparty library. E.g https://github.com/Thalhammer/jwt-cpp/blob/master/include/jwt-cpp/jwt.h#L971
So we can leave these as they are single use, or move them to constants, alternatively we can re-work the constructors for the public key variants, but that might take some serious refactoring. What would you recommend?
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@688
PS2, Line 688: if (jwks_update_thread_ != nullptr) jwks_update_thread_->Join()
> Did you find the reason behind having this as a warning instead of an error
Sorry I haven't replied to this. The only reason why this may be okay as a warning is that the JWKS information might be changing over time, if for some reason, the file the URI is pointing at becomes empty, it'll still be downloaded, but no keys will be verified successfully (due to no public keys in the jwks to do so). Since the UpdateJWKSThread is still alive, if the jwks file/endpoint is fixed, then the verification will be successful again.
http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc@84
PS7, Line 84: DEFINE_validator(jwks_pulling_timeout_s, &ValidateBiggerThanZero);
:
: using std::string;
: using strings::Substitute;
:
: namespace kudu {
:
> nit: instead of duplicating the code, consider introducing a function and t
Done
http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc@474
PS7, Line 474: // ECDSA using P-521 and SHA-512 (OBJ_txt2nid("secp521r1")).
: eccgrp = NID_secp521r1;
: } else {
: return Status::NotSupported(Substitute("Unsupported alg: '$0'", algorithm));
: }
: }
:
: auto it_x = kv_map.find("x");
: auto it_y = kv_map.find("y");
> Since there isn't any return statement involved in three if() scopes below,
Ack
http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc@734
PS7, Line 734: << status.ToString();
> nit: this could be moved out of the cycle and saved to be reused in this cy
Done
http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc@804
PS7, Line 804: } catch (const std::invalid_argument& e) {
: status = Status::NotAuthorized(
: Substitute("Token is not in correct format, error: $0", e.what()));
: } catch (const std::runtime_error& e) {
: status = Status::NotAuthorized(
: Substitute("Base64 decoding failed or invalid json, error: $0", e.what()));
: }
> nit: could you structured binding here instead
Done
http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/promise.h
File src/kudu/util/promise.h:
http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/promise.h@64
PS7, Line 64: null
> nit: since you are making changes here, maybe change NULL to nullptr as wel
Done
--
To view, visit http://gerrit.cloudera.org:8080/18467
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
Gerrit-Change-Number: 18467
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Mon, 07 Nov 2022 10:00:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] util: pull jwt code from Impala
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 )
Change subject: util: pull jwt code from Impala
......................................................................
Patch Set 12: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/18467
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
Gerrit-Change-Number: 18467
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 09 Nov 2022 18:56:32 +0000
Gerrit-HasComments: No
[kudu-CR] util: pull jwt code from Impala
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 )
Change subject: util: pull jwt code from Impala
......................................................................
Patch Set 4:
(21 comments)
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-internal.h
File src/kudu/util/jwt-util-internal.h:
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-internal.h@49
PS2, Line 49:
> missed std::move()?
Done
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-internal.h@49
PS2, Line 49:
> missed std::move()?
Done
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-internal.h@79
PS2, Line 79:
> style nit for here and below: per Kudu code style, the indent is wrong (see
Done
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-test.cc
File src/kudu/util/jwt-util-test.cc:
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-test.cc@370
PS2, Line 370: std::unique_ptr<WritableFile
> IIRC, in Kudu we do have env::NewTempWritableFile() for this.
Done
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-test.cc@372
PS2, Line 372: WritableFileOptions opts;
> Throughout this code:
Ack
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-test.cc@542
PS2, Line 542: .
> What is this for?
Done
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-test.cc@1141
PS2, Line 1141:
:
> Here and elsewhere: we do have ASSERT_STR_CONTAINS() for this in Kudu.
Done
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.h
File src/kudu/util/jwt-util.h:
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.h@71
PS2, Line 71: const JWTDecodedToken* decoded_token
> In Kudu we do pass input parameters as constant references, not constant po
As discussed, we'll keep the signatures for now.
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@20
PS2, Line 20: #include <cstddef>
> nit: use C++ style includes instead (i.e. this should be <cstring>)
Done
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@90
PS2, Line 90: tatus Parse(const Document& rules_doc) {
> This can be a static method.
Done
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@118
PS2, Line 118: ret
> nit: remove 'else' for readability
Done
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@341
PS2, Line 341: return
> Why not just return the status from here?
Done
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@346
PS2, Line 346: return Status::InvalidArgument
> There is RETURN_NOT_OK() for this
Done
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@358
PS2, Line 358: 2JWTPublicKey(algorith
> Use C++ casts instead
Done
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@372
PS2, Line 372: status = Status::NotAuthorized(
: Substitute("Failed to initial
> We do have safer ssl_make_unique() for these in Kudu.
Done
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@616
PS2, Line 616: status = Status::InvalidArgument("root element must be a JSON Object");
: } else if (!jwks_doc.HasMember("keys")) {
: status = Status::InvalidArgument("keys is required");
: } else {
: // Load and initialize public keys.
: JWKSetParser jwks_parser(
> In Kudu we have FindPtrOrNull() for this.
Done
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@663
PS2, Line 663:
: //
: // JWKSMgr member functions.
: //
:
: JWKSMgr::~JWKSMgr() {
: shut_down_promise_.Set(true);
: if (jwks_update_thread_ != nullptr) jwks_update_thread_->Join();
: }
:
> This is a wrong place to check -- that should be done in flag validators in
Done
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@793
PS2, Line 793: rn status;
> nit: create a reference decoded_token->decoded_jwt_ and use it here and bel
Done
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/promise.h
File src/kudu/util/promise.h:
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/promise.h@51
PS2, Line 51: bool IsSet() const {
> I think the overall semantics of this method is somewhat bogus due to the s
Done
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/promise.h@53
PS2, Line 53: }
:
> It seems 'return' is misplaced here?
Done
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/promise.h@56
PS2, Line 56: //
> It seems 'return' is missed here?
Done
--
To view, visit http://gerrit.cloudera.org:8080/18467
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
Gerrit-Change-Number: 18467
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Thu, 03 Nov 2022 14:38:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] util: pull jwt code from Impala
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 )
Change subject: util: pull jwt code from Impala
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@663
PS2, Line 663: if (FLAGS_jwks_update_frequency_s <= 0) {
: LOG(WARNING) << "Invalid value for flag jwks_update_frequency_s: "
: << FLAGS_jwks_update_frequency_s << ", use default value 60.";
: FLAGS_jwks_update_frequency_s = 60;
: }
: if (FLAGS_jwks_pulling_timeout_s <= 0) {
: LOG(WARNING) << "Invalid value for flag jwks_pulling_timeout_s: "
: << FLAGS_jwks_pulling_timeout_s << ", use default value 10.";
: FLAGS_jwks_pulling_timeout_s = 10;
: }
This is a wrong place to check -- that should be done in flag validators instead.
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@688
PS2, Line 688: if (new_jwks->IsEmpty()) LOG(WARNING) << "JWKS file is empty.";
That's a bit funny to see as a warning since in other places it's considered as an error, e.g. in JWTHelper::Verify() method.
--
To view, visit http://gerrit.cloudera.org:8080/18467
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
Gerrit-Change-Number: 18467
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: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 19 Oct 2022 20:06:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] util: pull jwt code from Impala
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/18467 )
Change subject: util: pull jwt code from Impala
......................................................................
util: pull jwt code from Impala
Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/jwt-util-internal.h
A src/kudu/util/jwt-util-test.cc
A src/kudu/util/jwt-util.cc
A src/kudu/util/jwt-util.h
M src/kudu/util/promise.h
6 files changed, 2,567 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/18467/5
--
To view, visit http://gerrit.cloudera.org:8080/18467
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
Gerrit-Change-Number: 18467
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] util: pull jwt code from Impala
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/18467 )
Change subject: util: pull jwt code from Impala
......................................................................
util: pull jwt code from Impala
Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/jwt-util-internal.h
A src/kudu/util/jwt-util-test.cc
A src/kudu/util/jwt-util.cc
A src/kudu/util/jwt-util.h
M src/kudu/util/promise.h
6 files changed, 2,558 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/18467/8
--
To view, visit http://gerrit.cloudera.org:8080/18467
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
Gerrit-Change-Number: 18467
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] util: pull jwt code from Impala
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 )
Change subject: util: pull jwt code from Impala
......................................................................
Patch Set 11:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/18467/10/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18467/10/src/kudu/util/jwt-util.cc@718
PS10, Line 718: the file which U
> nit: the file which the URI
Done
http://gerrit.cloudera.org:8080/#/c/18467/10/src/kudu/util/jwt-util.cc@728
PS10, Line 728: const MonoDelta &timeout = MonoDelta::FromSeconds(FLAGS_jwks_update_frequency_s);
:
> nit: this could be just
Done
http://gerrit.cloudera.org:8080/#/c/18467/10/src/kudu/util/jwt-util.cc@732
PS10, Line 732: // Shutdown has happened, stop updating JWKS.
> Good point. I'd vote for
Done
--
To view, visit http://gerrit.cloudera.org:8080/18467
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
Gerrit-Change-Number: 18467
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 09 Nov 2022 16:20:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] util: pull jwt code from Impala
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/18467 )
Change subject: util: pull jwt code from Impala
......................................................................
util: pull jwt code from Impala
Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/jwt-util-internal.h
A src/kudu/util/jwt-util-test.cc
A src/kudu/util/jwt-util.cc
A src/kudu/util/jwt-util.h
M src/kudu/util/promise.h
6 files changed, 2,564 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/18467/10
--
To view, visit http://gerrit.cloudera.org:8080/18467
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
Gerrit-Change-Number: 18467
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] util: pull jwt code from Impala
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 )
Change subject: util: pull jwt code from Impala
......................................................................
Patch Set 11: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/18467
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
Gerrit-Change-Number: 18467
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 09 Nov 2022 17:07:38 +0000
Gerrit-HasComments: No
[kudu-CR] util: pull jwt code from Impala
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 )
Change subject: util: pull jwt code from Impala
......................................................................
Patch Set 10:
(2 comments)
Overall looks good to me. Only two minor comments.
http://gerrit.cloudera.org:8080/#/c/18467/10/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18467/10/src/kudu/util/jwt-util.cc@718
PS10, Line 718: the file the URI
nit: the file which the URI
http://gerrit.cloudera.org:8080/#/c/18467/10/src/kudu/util/jwt-util.cc@732
PS10, Line 732: bool timed_out = shut_down_promise_.WaitFor(timeout);
WaitFor() returns nullptr if the timeout elapses so timed_out should be true if WaitFor() returns nullptr. We should break the loop if it's NOT timed out.
bool timed_out = (shut_down_promise_.WaitFor(timeout) == nullptr).
if (!timed_out) break;
or
if (shut_down_promise_.WaitFor(timeout) != nullptr) break;
--
To view, visit http://gerrit.cloudera.org:8080/18467
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
Gerrit-Change-Number: 18467
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Nov 2022 19:34:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] util: pull jwt code from Impala
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/18467 )
Change subject: util: pull jwt code from Impala
......................................................................
util: pull jwt code from Impala
Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/jwt-util-internal.h
A src/kudu/util/jwt-util-test.cc
A src/kudu/util/jwt-util.cc
A src/kudu/util/jwt-util.h
M src/kudu/util/promise.h
6 files changed, 2,528 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/18467/4
--
To view, visit http://gerrit.cloudera.org:8080/18467
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
Gerrit-Change-Number: 18467
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] util: pull jwt code from Impala
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/18467 )
Change subject: util: pull jwt code from Impala
......................................................................
util: pull jwt code from Impala
Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/jwt-util-internal.h
A src/kudu/util/jwt-util-test.cc
A src/kudu/util/jwt-util.cc
A src/kudu/util/jwt-util.h
M src/kudu/util/promise.h
6 files changed, 2,558 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/18467/9
--
To view, visit http://gerrit.cloudera.org:8080/18467
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
Gerrit-Change-Number: 18467
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] util: pull jwt code from Impala
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/18467 )
Change subject: util: pull jwt code from Impala
......................................................................
util: pull jwt code from Impala
Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/jwt-util-internal.h
A src/kudu/util/jwt-util-test.cc
A src/kudu/util/jwt-util.cc
A src/kudu/util/jwt-util.h
M src/kudu/util/promise.h
6 files changed, 2,562 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/18467/12
--
To view, visit http://gerrit.cloudera.org:8080/18467
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
Gerrit-Change-Number: 18467
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] util: pull jwt code from Impala
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 )
Change subject: util: pull jwt code from Impala
......................................................................
Patch Set 7: Code-Review+1
(7 comments)
Looks better now: a few nits
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@272
PS2, Line 272:
> Should these strings be constants and used as such elsewhere in the code?
Did you miss to address this comment or there is some other reason behind?
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@688
PS2, Line 688: return FindPointeeOrNull(ec_pub_key_map_, kid);
> That's a bit funny to see as a warning since in other places it's considere
Did you find the reason behind having this as a warning instead of an error? Would be great adding a comment on that since it's treated as error in other part of the JWT code.
http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc@84
PS7, Line 84: [](const char* name, const int32_t val) {
: if (val <= 0) {
: LOG(ERROR) << Substitute("Invalid value for $0 flag: $1", name, val);
: return false;
: }
: return true;
: });
nit: instead of duplicating the code, consider introducing a function and then refer it in both validators
http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc@474
PS7, Line 474: if (algorithm == "es256") {
: // ECDSA using P-256 and SHA-256 (OBJ_txt2nid("prime256v1")).
: eccgrp = NID_X9_62_prime256v1;
: }
: if (algorithm == "es384") {
: // ECDSA using P-384 and SHA-384 (OBJ_txt2nid("secp384r1")).
: eccgrp = NID_secp384r1;
: }
: if (algorithm == "es512") {
Since there isn't any return statement involved in three if() scopes below, keeping the original 'else if' would make the code more efficient?
http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc@734
PS7, Line 734: MonoDelta::FromMilliseconds(timeout_millis)
nit: this could be moved out of the cycle and saved to be reused in this cycle over and over again.
http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc@804
PS7, Line 804: for (auto& claim : decoded_token_out.get()->decoded_jwt_.get_header_claims()) {
: msg << claim.first << "=" << claim.second.to_json().serialize() << ";";
: }
: msg << " JWT token payload: ";
: for (auto& claim : decoded_token_out.get()->decoded_jwt_.get_payload_claims()) {
: msg << claim.first << "=" << claim.second.to_json().serialize() << ";";
: }
nit: could you structured binding here instead
for (auto& [key, val] : ...) {
msg << key << ...;
}
https://en.cppreference.com/w/cpp/language/structured_binding
http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/promise.h
File src/kudu/util/promise.h:
http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/promise.h@64
PS7, Line 64: NULL
nit: since you are making changes here, maybe change NULL to nullptr as well
--
To view, visit http://gerrit.cloudera.org:8080/18467
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
Gerrit-Change-Number: 18467
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Sun, 06 Nov 2022 16:56:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] util: pull jwt code from Impala
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/18467 )
Change subject: util: pull jwt code from Impala
......................................................................
util: pull jwt code from Impala
Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/jwt-util-internal.h
A src/kudu/util/jwt-util-test.cc
A src/kudu/util/jwt-util.cc
A src/kudu/util/jwt-util.h
M src/kudu/util/promise.h
6 files changed, 2,566 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/18467/11
--
To view, visit http://gerrit.cloudera.org:8080/18467
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
Gerrit-Change-Number: 18467
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] util: pull jwt code from Impala
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 )
Change subject: util: pull jwt code from Impala
......................................................................
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc@734
PS7, Line 734: << status.ToString();
> Done
define a variable for MonoDelta::FromMilliseconds(timeout_millis) out of the loop. But shut_down_promise_.WaitFor() should be left in the loop.
--
To view, visit http://gerrit.cloudera.org:8080/18467
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
Gerrit-Change-Number: 18467
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Mon, 07 Nov 2022 21:33:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] util: pull jwt code from Impala
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 )
Change subject: util: pull jwt code from Impala
......................................................................
Patch Set 9: Code-Review+1
(3 comments)
Overall looks good to me, but please address Wenzhe's comment athttps://gerrit.cloudera.org/#/c/18467/7/src/kudu/util/jwt-util.cc@734
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@272
PS2, Line 272: otAut
> This is a bit weird, I think it's okay if we move these to constant, but th
Yep -- I agree: that can be taken care of separately, if needed. So, I'm OK with leaving this as-is for now.
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@688
PS2, Line 688: if (jwks_update_thread_ != nullptr) jwks_update_thread_->Join()
> Sorry I haven't replied to this. The only reason why this may be okay as a
Thank you for the analysis. It would be great if you could add a blurb to put that information as a code's comment.
http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc@734
PS7, Line 734: << status.ToString();
> define a variable for MonoDelta::FromMilliseconds(timeout_millis) out of th
Yup, that's what I was trying to recommend as well, but failed to articulate properly. Thanks for the clarification, Wenzhe!
--
To view, visit http://gerrit.cloudera.org:8080/18467
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
Gerrit-Change-Number: 18467
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Nov 2022 02:22:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] util: pull jwt code from Impala
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 )
Change subject: util: pull jwt code from Impala
......................................................................
Patch Set 9:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@272
PS2, Line 272: otAut
> Yep -- I agree: that can be taken care of separately, if needed. So, I'm O
Ack
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@688
PS2, Line 688: if (jwks_update_thread_ != nullptr) jwks_update_thread_->Join()
> Thank you for the analysis. It would be great if you could add a blurb to
Done
http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc@734
PS7, Line 734: << status.ToString();
> Yup, that's what I was trying to recommend as well, but failed to articulat
Done
--
To view, visit http://gerrit.cloudera.org:8080/18467
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
Gerrit-Change-Number: 18467
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Nov 2022 08:28:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] util: pull jwt code from Impala
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/18467 )
Change subject: util: pull jwt code from Impala
......................................................................
util: pull jwt code from Impala
Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/jwt-util-internal.h
A src/kudu/util/jwt-util-test.cc
A src/kudu/util/jwt-util.cc
A src/kudu/util/jwt-util.h
M src/kudu/util/promise.h
6 files changed, 2,567 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/18467/7
--
To view, visit http://gerrit.cloudera.org:8080/18467
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
Gerrit-Change-Number: 18467
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] util: pull jwt code from Impala
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 )
Change subject: util: pull jwt code from Impala
......................................................................
Patch Set 12: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/18467
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
Gerrit-Change-Number: 18467
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 09 Nov 2022 19:06:48 +0000
Gerrit-HasComments: No
[kudu-CR] util: pull jwt code from Impala
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 )
Change subject: util: pull jwt code from Impala
......................................................................
Patch Set 2:
(24 comments)
Both LINT and IWYU fail -- please make sure those pass.
From my assessment, this code is of not high enough quality to meet standards we use for Kudu. I'd suggest to try improving that right away, but we can discuss other options.
Also, did you check recent updates on this functionality in the Impala repo?
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-internal.h
File src/kudu/util/jwt-util-internal.h:
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-internal.h@49
PS2, Line 49: algorithm
missed std::move()?
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-internal.h@49
PS2, Line 49: pub_key
missed std::move()?
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-internal.h@65
PS2, Line 65: const std::string algorithm_;
This looks a bit lame to use arbitrary string representation for an enumerated entity.
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-internal.h@78
PS2, Line 78: std::string algorithm,
here and below: I'm not sure I understand why to have this 'algorithm' as a parameter when it's already clear this is a key for a particular algorithm (here it's HS256 algorithm)? Am I missing something?
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-internal.h@79
PS2, Line 79:
style nit for here and below: per Kudu code style, the indent is wrong (see https://google.github.io/styleguide/cppguide.html#Constructor_Initializer_Lists)
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-test.cc
File src/kudu/util/jwt-util-test.cc:
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-test.cc@35
PS2, Line 35: std::string rsa_priv_key_pem = R"(-----BEGIN PRIVATE KEY-----
Should all these be constant strings?
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-test.cc@370
PS2, Line 370: int fd = mkstemp(&name_[0]);
IIRC, in Kudu we do have env::NewTempWritableFile() for this.
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-test.cc@372
PS2, Line 372: std::cout << "Error creating temp file; " << strerror(errno) << std::endl;
Throughout this code:
* errors are usually written into std:cerr, not std::cout
* before doing anything that might change errno (like memory allocation or other activity in outputting to streams) it's necessary to store errno into a temporary variable, and only then work with it.
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-test.cc@542
PS2, Line 542: EXPECT_OK(status);
What is this for?
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-test.cc@1141
PS2, Line 1141: ASSERT_TRUE(status.ToString().find("Verification failed, error: token expired")
: != std::string::npos)
Here and elsewhere: we do have ASSERT_STR_CONTAINS() for this in Kudu.
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.h
File src/kudu/util/jwt-util.h:
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.h@71
PS2, Line 71: const JWTDecodedToken* decoded_token
In Kudu we do pass input parameters as constant references, not constant pointers.
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@20
PS2, Line 20: #include <string.h>
nit: use C++ style includes instead (i.e. this should be <cstring>)
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@90
PS2, Line 90: tring NameOfTypeOfJsonValue(const Value& value) {
This can be a static method.
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@118
PS2, Line 118: else
nit: remove 'else' for readability
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@272
PS2, Line 272: hs256
Should these strings be constants and used as such elsewhere in the code?
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@341
PS2, Line 341: status
Why not just return the status from here?
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@346
PS2, Line 346: if (!status.ok()) return status;
There is RETURN_NOT_OK() for this
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@358
PS2, Line 358: (const unsigned char*)
Use C++ casts instead
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@372
PS2, Line 372: BIO* bio = BIO_new(BIO_s_mem());
: PEM_write_bio_RSA_PUBKEY(bio, rsa);
We do have safer ssl_make_unique() for these in Kudu.
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@616
PS2, Line 616: auto find_it = hs_key_map_.find(kid);
: if (find_it == hs_key_map_.end()) {
: // Could not find key for the given key ID.
: return nullptr;
: }
: return find_it->second.get();
In Kudu we have FindPtrOrNull() for this.
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@793
PS2, Line 793: decoded_token->decoded_jwt_
nit: create a reference decoded_token->decoded_jwt_ and use it here and below
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/promise.h
File src/kudu/util/promise.h:
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/promise.h@51
PS2, Line 51: const T& Get(int64_t timeout_millis, bool* timed_out) const
I think the overall semantics of this method is somewhat bogus due to the signature that returns a constant reference to the underlying 'val_' object (i.e. the future object).
In case of timeout, the returned value wouldn't make much sense and might cause issues if accessed by the callee.
Why WaitFor(const MonoDelta&) isn't enough?
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/promise.h@53
PS2, Line 53: return val_;
: *timed_out = false;
It seems 'return' is misplaced here?
http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/promise.h@56
PS2, Line 56: *timed_out = true;
It seems 'return' is missed here?
--
To view, visit http://gerrit.cloudera.org:8080/18467
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
Gerrit-Change-Number: 18467
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: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 19 Oct 2022 19:36:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] util: pull jwt code from Impala
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 )
Change subject: util: pull jwt code from Impala
......................................................................
Patch Set 10:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/18467/10/src/kudu/util/jwt-util.cc
File src/kudu/util/jwt-util.cc:
http://gerrit.cloudera.org:8080/#/c/18467/10/src/kudu/util/jwt-util.cc@728
PS10, Line 728: int64_t timeout_millis = static_cast<int64_t>(FLAGS_jwks_update_frequency_s) * 1000;
: const MonoDelta &timeout = MonoDelta::FromMilliseconds(timeout_millis);
nit: this could be just
const MonoDelta timeout = MonoDelta::FromSeconds(FLAGS_jwks_update_frequency_s);
http://gerrit.cloudera.org:8080/#/c/18467/10/src/kudu/util/jwt-util.cc@732
PS10, Line 732: bool timed_out = shut_down_promise_.WaitFor(timeout);
> WaitFor() returns nullptr if the timeout elapses so timed_out should be tru
Good point. I'd vote for
if (shut_down_promise_.WaitFor(timeout) != nullptr) {
// Shutdown has happened, stop updating JWKS.
break;
}
since it doesn't introduce useless variables.
Also, adding a comment helps to increase readability.
--
To view, visit http://gerrit.cloudera.org:8080/18467
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
Gerrit-Change-Number: 18467
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Nov 2022 19:59:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] util: pull jwt code from Impala
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 )
Change subject: util: pull jwt code from Impala
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18467/11/src/kudu/util/promise.h
File src/kudu/util/promise.h:
http://gerrit.cloudera.org:8080/#/c/18467/11/src/kudu/util/promise.h@51
PS11, Line 51: bool IsSet() const {
: return latch_.count() == 0;
: }
Is it needed as of PS11? I could not find its usage, but I might be missing something.
--
To view, visit http://gerrit.cloudera.org:8080/18467
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
Gerrit-Change-Number: 18467
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 09 Nov 2022 18:50:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] util: pull jwt code from Impala
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18467 )
Change subject: util: pull jwt code from Impala
......................................................................
util: pull jwt code from Impala
Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
Reviewed-on: http://gerrit.cloudera.org:8080/18467
Reviewed-by: Alexey Serbin <al...@apache.org>
Reviewed-by: Wenzhe Zhou <wz...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/jwt-util-internal.h
A src/kudu/util/jwt-util-test.cc
A src/kudu/util/jwt-util.cc
A src/kudu/util/jwt-util.h
M src/kudu/util/promise.h
6 files changed, 2,562 insertions(+), 2 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved
Wenzhe Zhou: Looks good to me, but someone else must approve
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/18467
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b
Gerrit-Change-Number: 18467
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: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>