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>