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: plumb JWT into mini cluster
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18475
Change subject: jwt: plumb JWT into mini cluster
......................................................................
jwt: plumb JWT into mini cluster
This patch adds options to ExternalMiniCluster to start a MiniOidc
alongside the calling process.
Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
---
M src/kudu/integration-tests/security-itest.cc
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/util/CMakeLists.txt
A src/kudu/util/jwt_test_certs.cc
M src/kudu/util/jwt_test_certs.h
7 files changed, 450 insertions(+), 303 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/18475/1
--
To view, visit http://gerrit.cloudera.org:8080/18475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
Gerrit-Change-Number: 18475
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
[kudu-CR] jwt: plumb JWT into mini cluster
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18475 )
Change subject: jwt: plumb JWT into mini cluster
......................................................................
jwt: plumb JWT into mini cluster
This patch adds options to ExternalMiniCluster to start a MiniOidc
alongside the calling process.
Co-authored-by: Andrew Wong <aw...@apache.org>
Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
Reviewed-on: http://gerrit.cloudera.org:8080/18475
Tested-by: Kudu Jenkins
Reviewed-by: Wenzhe Zhou <wz...@cloudera.com>
Reviewed-by: Alexey Serbin <al...@apache.org>
---
M src/kudu/integration-tests/security-itest.cc
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/rpc/negotiation.cc
M src/kudu/server/server_base.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/jwt-util.cc
A src/kudu/util/jwt_test_certs.cc
M src/kudu/util/jwt_test_certs.h
M src/kudu/util/mini_oidc.cc
11 files changed, 471 insertions(+), 314 deletions(-)
Approvals:
Kudu Jenkins: Verified
Wenzhe Zhou: Looks good to me, but someone else must approve
Alexey Serbin: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/18475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
Gerrit-Change-Number: 18475
Gerrit-PatchSet: 15
Gerrit-Owner: Andrew Wong <an...@gmail.com>
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: plumb JWT into mini cluster
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18475 )
Change subject: jwt: plumb JWT into mini cluster
......................................................................
Patch Set 12:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc@515
PS11, Line 515: TEST_F(SecurityITest, TestJwtMiniCluster) {
> Could you also add a test to cover handling of an expired JWT token?
that testcase is added in the following patch
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc@526
PS11, Line 526:
> nit: duplicated as line #517
Done
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc@534
PS11, Line 534: *pb.mutable_jwt() = std::move(jw
> nit: could this be written to match line 535?
Done
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc@535
PS11, Line 535:
> nit: could use std::move() here?
Done
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc@538
PS11, Line 538: for (auto i = 0; i < cluster_
> Is this really necessary given that the jwt field has just been set at line
Done
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc@560
PS11, Line 560:
> In addition to verifying non-OK result, does it make sense to check for par
The error status returned here is a RuntimeError, I don't think that would provide more context, also we're parsing the error message in the next line to make sure it fails in the way we expect it.
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc@563
PS11, Line 563: shared_ptr<KuduClient> client;
> Could you add a sub-case to check how it works when no JWT is provided with
Done
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/mini-cluster/external_mini_cluster.cc@85
PS11, Line 85: #include "kudu/util/test_util.h
> nit: put this line in front of line #75 to keep alphabet order
Done
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/server/server_base.cc@716
PS11, Line 716: }
> If we call Init() on JWT verifier here, why to call Init() on JWT verifier
Done
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/util/jwt_test_certs.h
File src/kudu/util/jwt_test_certs.h:
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/util/jwt_test_certs.h@21
PS11, Line 21: // The
> Here and below: should all these be 'extern const' since they are not modif
Done
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/util/jwt_test_certs.h@22
PS11, Line 22: to-jwk tool
> nit: using 'const char[]' or 'const char* const' instead of std::string for
Done
--
To view, visit http://gerrit.cloudera.org:8080/18475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
Gerrit-Change-Number: 18475
Gerrit-PatchSet: 12
Gerrit-Owner: Andrew Wong <an...@gmail.com>
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, 28 Mar 2023 14:18:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] jwt: plumb JWT into mini cluster
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has uploaded a new patch set (#2) to the change originally created by Andrew Wong. ( http://gerrit.cloudera.org:8080/18475 )
Change subject: jwt: plumb JWT into mini cluster
......................................................................
jwt: plumb JWT into mini cluster
This patch adds options to ExternalMiniCluster to start a MiniOidc
alongside the calling process.
Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
---
M src/kudu/integration-tests/security-itest.cc
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/messenger.h
M src/kudu/util/CMakeLists.txt
A src/kudu/util/jwt_test_certs.cc
M src/kudu/util/jwt_test_certs.h
9 files changed, 452 insertions(+), 305 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/18475/2
--
To view, visit http://gerrit.cloudera.org:8080/18475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
Gerrit-Change-Number: 18475
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] jwt: plumb JWT into mini cluster
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/18475 )
Change subject: jwt: plumb JWT into mini cluster
......................................................................
jwt: plumb JWT into mini cluster
This patch adds options to ExternalMiniCluster to start a MiniOidc
alongside the calling process.
Co-authored-by: Andrew Wong <aw...@apache.org>
Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
---
M src/kudu/integration-tests/security-itest.cc
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/rpc/negotiation.cc
M src/kudu/server/server_base.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/jwt_test_certs.cc
M src/kudu/util/jwt_test_certs.h
9 files changed, 524 insertions(+), 305 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/18475/4
--
To view, visit http://gerrit.cloudera.org:8080/18475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
Gerrit-Change-Number: 18475
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: plumb JWT into mini cluster
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/18475 )
Change subject: jwt: plumb JWT into mini cluster
......................................................................
jwt: plumb JWT into mini cluster
This patch adds options to ExternalMiniCluster to start a MiniOidc
alongside the calling process.
Co-authored-by: Andrew Wong <aw...@apache.org>
Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
---
M src/kudu/integration-tests/security-itest.cc
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/rpc/negotiation.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/jwt_test_certs.cc
M src/kudu/util/jwt_test_certs.h
8 files changed, 524 insertions(+), 307 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/18475/5
--
To view, visit http://gerrit.cloudera.org:8080/18475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
Gerrit-Change-Number: 18475
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: plumb JWT into mini cluster
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/18475 )
Change subject: jwt: plumb JWT into mini cluster
......................................................................
jwt: plumb JWT into mini cluster
This patch adds options to ExternalMiniCluster to start a MiniOidc
alongside the calling process.
Co-authored-by: Andrew Wong <aw...@apache.org>
Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
---
M src/kudu/integration-tests/security-itest.cc
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/rpc/negotiation.cc
M src/kudu/server/server_base.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/jwt-util.cc
A src/kudu/util/jwt_test_certs.cc
M src/kudu/util/jwt_test_certs.h
M src/kudu/util/mini_oidc.cc
11 files changed, 466 insertions(+), 311 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/18475/8
--
To view, visit http://gerrit.cloudera.org:8080/18475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
Gerrit-Change-Number: 18475
Gerrit-PatchSet: 8
Gerrit-Owner: Andrew Wong <an...@gmail.com>
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: plumb JWT into mini cluster
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18475 )
Change subject: jwt: plumb JWT into mini cluster
......................................................................
Patch Set 13:
(1 comment)
> Patch Set 13:
>
> One comment in external_mini_cluster.cc, line 285 is not fixed. Other looks good.
thanks, I missed that
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/mini-cluster/external_mini_cluster.cc@286
PS11, Line 286: ),
> Could use std::move() to avoid incrementing reference counter for jwt_verif
Done
--
To view, visit http://gerrit.cloudera.org:8080/18475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
Gerrit-Change-Number: 18475
Gerrit-PatchSet: 13
Gerrit-Owner: Andrew Wong <an...@gmail.com>
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, 28 Mar 2023 17:40:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] jwt: plumb JWT into mini cluster
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/18475 )
Change subject: jwt: plumb JWT into mini cluster
......................................................................
jwt: plumb JWT into mini cluster
This patch adds options to ExternalMiniCluster to start a MiniOidc
alongside the calling process.
Co-authored-by: Andrew Wong <aw...@apache.org>
Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
---
M src/kudu/integration-tests/security-itest.cc
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/rpc/negotiation.cc
M src/kudu/server/server_base.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/jwt-util.cc
A src/kudu/util/jwt_test_certs.cc
M src/kudu/util/jwt_test_certs.h
M src/kudu/util/mini_oidc.cc
11 files changed, 474 insertions(+), 314 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/18475/12
--
To view, visit http://gerrit.cloudera.org:8080/18475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
Gerrit-Change-Number: 18475
Gerrit-PatchSet: 12
Gerrit-Owner: Andrew Wong <an...@gmail.com>
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: plumb JWT into mini cluster
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/18475 )
Change subject: jwt: plumb JWT into mini cluster
......................................................................
jwt: plumb JWT into mini cluster
This patch adds options to ExternalMiniCluster to start a MiniOidc
alongside the calling process.
Co-authored-by: Andrew Wong <aw...@apache.org>
Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
---
M src/kudu/integration-tests/security-itest.cc
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/rpc/negotiation.cc
M src/kudu/server/server_base.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/jwt-util.cc
A src/kudu/util/jwt_test_certs.cc
M src/kudu/util/jwt_test_certs.h
M src/kudu/util/mini_oidc.cc
11 files changed, 471 insertions(+), 314 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/18475/13
--
To view, visit http://gerrit.cloudera.org:8080/18475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
Gerrit-Change-Number: 18475
Gerrit-PatchSet: 13
Gerrit-Owner: Andrew Wong <an...@gmail.com>
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: plumb JWT into mini cluster
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18475 )
Change subject: jwt: plumb JWT into mini cluster
......................................................................
Patch Set 14: Code-Review+2
(3 comments)
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc@515
PS11, Line 515: TEST_F(SecurityITest, TestJwtMiniCluster) {
> that testcase is added in the following patch
Ah, I missed that -- thanks for the clarification.
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc@560
PS11, Line 560:
> The error status returned here is a RuntimeError, I don't think that would
That's right, but from the other side printing OK in case when s == Status::OK() doesn't provide more context either.
Anyways, that sounds good enough to me.
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc@563
PS11, Line 563: shared_ptr<KuduClient> client;
> Done
Thanks!
--
To view, visit http://gerrit.cloudera.org:8080/18475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
Gerrit-Change-Number: 18475
Gerrit-PatchSet: 14
Gerrit-Owner: Andrew Wong <an...@gmail.com>
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, 28 Mar 2023 19:04:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] jwt: plumb JWT into mini cluster
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/18475 )
Change subject: jwt: plumb JWT into mini cluster
......................................................................
jwt: plumb JWT into mini cluster
This patch adds options to ExternalMiniCluster to start a MiniOidc
alongside the calling process.
Co-authored-by: Andrew Wong <aw...@apache.org>
Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
---
M src/kudu/integration-tests/security-itest.cc
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/rpc/negotiation.cc
M src/kudu/server/server_base.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/jwt-util.cc
A src/kudu/util/jwt_test_certs.cc
M src/kudu/util/jwt_test_certs.h
M src/kudu/util/mini_oidc.cc
11 files changed, 464 insertions(+), 310 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/18475/10
--
To view, visit http://gerrit.cloudera.org:8080/18475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
Gerrit-Change-Number: 18475
Gerrit-PatchSet: 10
Gerrit-Owner: Andrew Wong <an...@gmail.com>
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: plumb JWT into mini cluster
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/18475 )
Change subject: jwt: plumb JWT into mini cluster
......................................................................
Patch Set 13:
One comment in external_mini_cluster.cc, line 285 is not fixed. Other looks good.
--
To view, visit http://gerrit.cloudera.org:8080/18475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
Gerrit-Change-Number: 18475
Gerrit-PatchSet: 13
Gerrit-Owner: Andrew Wong <an...@gmail.com>
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, 28 Mar 2023 17:12:31 +0000
Gerrit-HasComments: No
[kudu-CR] jwt: plumb JWT into mini cluster
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/18475 )
Change subject: jwt: plumb JWT into mini cluster
......................................................................
Patch Set 14: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/18475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
Gerrit-Change-Number: 18475
Gerrit-PatchSet: 14
Gerrit-Owner: Andrew Wong <an...@gmail.com>
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, 28 Mar 2023 18:28:39 +0000
Gerrit-HasComments: No
[kudu-CR] jwt: plumb JWT into mini cluster
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/18475 )
Change subject: jwt: plumb JWT into mini cluster
......................................................................
jwt: plumb JWT into mini cluster
This patch adds options to ExternalMiniCluster to start a MiniOidc
alongside the calling process.
Co-authored-by: Andrew Wong <aw...@apache.org>
Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
---
M src/kudu/integration-tests/security-itest.cc
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/rpc/negotiation.cc
M src/kudu/server/server_base.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/jwt-util.cc
A src/kudu/util/jwt_test_certs.cc
M src/kudu/util/jwt_test_certs.h
M src/kudu/util/mini_oidc.cc
11 files changed, 471 insertions(+), 314 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/18475/14
--
To view, visit http://gerrit.cloudera.org:8080/18475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
Gerrit-Change-Number: 18475
Gerrit-PatchSet: 14
Gerrit-Owner: Andrew Wong <an...@gmail.com>
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: plumb JWT into mini cluster
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/18475 )
Change subject: jwt: plumb JWT into mini cluster
......................................................................
Patch Set 11:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc@526
PS11, Line 526: cluster_opts_.num_tablet_servers = 0;
nit: duplicated as line #517
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/mini-cluster/external_mini_cluster.cc@85
PS11, Line 85: #include "kudu/util/jwt-util.h"
nit: put this line in front of line #75 to keep alphabet order
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/mini-cluster/external_mini_cluster.cc@147
PS11, Line 147: ,
nit: move comma to the end of line #145
--
To view, visit http://gerrit.cloudera.org:8080/18475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
Gerrit-Change-Number: 18475
Gerrit-PatchSet: 11
Gerrit-Owner: Andrew Wong <an...@gmail.com>
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, 27 Mar 2023 20:46:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] jwt: plumb JWT into mini cluster
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18475 )
Change subject: jwt: plumb JWT into mini cluster
......................................................................
Patch Set 11:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc@515
PS11, Line 515: TEST_F(SecurityITest, TestJwtMiniCluster) {
Could you also add a test to cover handling of an expired JWT token?
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc@534
PS11, Line 534: jwt.set_jwt_data(encoded_token);
nit: could this be written to match line 535?
*jwt.mutable_jwt_data() = std::move(encoded_token);
Alternatively, since encoded_token isn't used anywhere else, could it be just
*jwt.mutable_jwt_data() = MiniOidc::CreateJwt(account_id, kSubject, true);
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc@535
PS11, Line 535: jwt
nit: could use std::move() here?
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc@538
PS11, Line 538: CHECK_EQ(true, pb.has_jwt());
Is this really necessary given that the jwt field has just been set at line 535?
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc@560
PS11, Line 560: ASSERT_FALSE(s.ok()) << s.ToString();
In addition to verifying non-OK result, does it make sense to check for particular type of error here?
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/integration-tests/security-itest.cc@563
PS11, Line 563: }
Could you add a sub-case to check how it works when no JWT is provided with serialized AuthenticationCredentialsPB imported as authn creds?
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/mini-cluster/external_mini_cluster.cc@286
PS11, Line 286: jwt_verifier
Could use std::move() to avoid incrementing reference counter for jwt_verifier?
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/server/server_base.cc@716
PS11, Line 716: RETURN_NOT_OK_PREPEND(jwt_verifier->Init(), "Failed to init JWT verifier");
If we call Init() on JWT verifier here, why to call Init() on JWT verifier second time in MessengerBuilder::Build() then?
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/util/jwt_test_certs.h
File src/kudu/util/jwt_test_certs.h:
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/util/jwt_test_certs.h@21
PS11, Line 21: extern
Here and below: should all these be 'extern const' since they are not modified once instantiated?
http://gerrit.cloudera.org:8080/#/c/18475/11/src/kudu/util/jwt_test_certs.h@22
PS11, Line 22: std::string
nit: using 'const char[]' or 'const char* const' instead of std::string for such constants might make the data section of the executable a bit lighter (de facto all such constants in Kudu code are char[] or char*, not std::string).
--
To view, visit http://gerrit.cloudera.org:8080/18475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
Gerrit-Change-Number: 18475
Gerrit-PatchSet: 11
Gerrit-Owner: Andrew Wong <an...@gmail.com>
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, 27 Mar 2023 22:26:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] jwt: plumb JWT into mini cluster
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/18475 )
Change subject: jwt: plumb JWT into mini cluster
......................................................................
jwt: plumb JWT into mini cluster
This patch adds options to ExternalMiniCluster to start a MiniOidc
alongside the calling process.
Co-authored-by: Andrew Wong <aw...@apache.org>
Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
---
M src/kudu/integration-tests/security-itest.cc
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/rpc/negotiation.cc
M src/kudu/server/server_base.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/jwt-util.cc
A src/kudu/util/jwt_test_certs.cc
M src/kudu/util/jwt_test_certs.h
M src/kudu/util/mini_oidc.cc
11 files changed, 466 insertions(+), 311 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/18475/7
--
To view, visit http://gerrit.cloudera.org:8080/18475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
Gerrit-Change-Number: 18475
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Wong <an...@gmail.com>
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: plumb JWT into mini cluster
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/18475 )
Change subject: jwt: plumb JWT into mini cluster
......................................................................
jwt: plumb JWT into mini cluster
This patch adds options to ExternalMiniCluster to start a MiniOidc
alongside the calling process.
Co-authored-by: Andrew Wong <aw...@apache.org>
Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
---
M src/kudu/integration-tests/security-itest.cc
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/rpc/negotiation.cc
M src/kudu/server/server_base.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/jwt-util.cc
A src/kudu/util/jwt_test_certs.cc
M src/kudu/util/jwt_test_certs.h
M src/kudu/util/mini_oidc.cc
11 files changed, 471 insertions(+), 311 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/18475/6
--
To view, visit http://gerrit.cloudera.org:8080/18475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
Gerrit-Change-Number: 18475
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong <an...@gmail.com>
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: plumb JWT into mini cluster
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/18475 )
Change subject: jwt: plumb JWT into mini cluster
......................................................................
jwt: plumb JWT into mini cluster
This patch adds options to ExternalMiniCluster to start a MiniOidc
alongside the calling process.
Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
---
M src/kudu/integration-tests/security-itest.cc
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/util/CMakeLists.txt
A src/kudu/util/jwt_test_certs.cc
M src/kudu/util/jwt_test_certs.h
7 files changed, 513 insertions(+), 303 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/18475/3
--
To view, visit http://gerrit.cloudera.org:8080/18475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653
Gerrit-Change-Number: 18475
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>