[kudu-CR] KUDU-2319 follower masters should be able to verify authn tokens

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9373 )

Change subject: KUDU-2319 follower masters should be able to verify authn tokens
..


Patch Set 12: Verified+1

Unrelated flake in DeleteTableITest.TestNoDeleteTombstonedTablets


--
To view, visit http://gerrit.cloudera.org:8080/9373
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
Gerrit-Change-Number: 9373
Gerrit-PatchSet: 12
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 02 Mar 2018 20:22:32 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2319 follower masters should be able to verify authn tokens

[Alexey Serbin (Code Review)]

Alexey Serbin has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9373 )

Change subject: KUDU-2319 follower masters should be able to verify authn tokens
..

KUDU-2319 follower masters should be able to verify authn tokens

This patch addresses the issue that follower masters could not accept
authn tokens to authenticate clients because they didn't have keys for
token signature verification.

Added a couple of tests:
  * SecurityMasterAuthTest::FollowerTokenVerificationKeys
  * ConnectToFollowerMasterTest::AuthnTokenVerifierHaveKeys

The former is more a unit test; the latter is an integration test.

Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
Reviewed-on: http://gerrit.cloudera.org:8080/9373
Reviewed-by: Dan Burkert 
Tested-by: Alexey Serbin 
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/security-itest.cc
R src/kudu/integration-tests/security-master-auth-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
5 files changed, 233 insertions(+), 24 deletions(-)

Approvals:
  Dan Burkert: Looks good to me, approved
  Alexey Serbin: Verified

--
To view, visit http://gerrit.cloudera.org:8080/9373
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
Gerrit-Change-Number: 9373
Gerrit-PatchSet: 13
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2319 follower masters should be able to verify authn tokens

[Alexey Serbin (Code Review)]

Alexey Serbin has removed Kudu Jenkins from this change.  ( 
http://gerrit.cloudera.org:8080/9373 )

Change subject: KUDU-2319 follower masters should be able to verify authn tokens
..


Removed reviewer Kudu Jenkins with the following votes:

* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/9373
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
Gerrit-Change-Number: 9373
Gerrit-PatchSet: 12
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2319 follower masters should be able to verify authn tokens

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9373 )

Change subject: KUDU-2319 follower masters should be able to verify authn tokens
..


Patch Set 12: Code-Review+2

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9373/11/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/9373/11/src/kudu/master/catalog_manager.cc@1034
PS11, Line 1034:   return s;
   :   }
> I updated this code to be easier to follow.
looks great


http://gerrit.cloudera.org:8080/#/c/9373/11/src/kudu/master/catalog_manager.cc@1042
PS11, Line 1042:
   :
   :   LOG_WITH_PREFIX(INFO) << kDescription
   : <<
> The return status from this methods affects whether the PrepareFollower() m
oh I see, thanks for the explanation.



--
To view, visit http://gerrit.cloudera.org:8080/9373
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
Gerrit-Change-Number: 9373
Gerrit-PatchSet: 12
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 02 Mar 2018 19:54:50 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2319 follower masters should be able to verify authn tokens

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9373 )

Change subject: KUDU-2319 follower masters should be able to verify authn tokens
..


Patch Set 11:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9373/11/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/9373/11/src/kudu/master/catalog_manager.cc@1034
PS11, Line 1034: } else {
   : if (!key
> this can be un-nested one level with an 'else if', which I think will make
I updated this code to be easier to follow.


http://gerrit.cloudera.org:8080/#/c/9373/11/src/kudu/master/catalog_manager.cc@1042
PS11, Line 1042: ) in this case would lead to
   :   // waiting till next cycle of TSK rotation. Returning 
non-OK status will
   :   // make the periodic task of 
CatalogManagerBgTasks::Run() to retry
   :   // fetching the keys.
> I'm not seeing how the return status from this method is effecting CatalogM
The return status from this methods affects whether the PrepareFollower() 
method updates its output parameter 'last_tspk_run'.  Yes, the 
CatalogManagerBgTasks::Run() runs with its own interval, but 
PrepareFollowerTokenVerifier() will not be called until enough time has passed 
to expect a new TSK to appear.



--
To view, visit http://gerrit.cloudera.org:8080/9373
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
Gerrit-Change-Number: 9373
Gerrit-PatchSet: 11
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 02 Mar 2018 19:39:27 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2319 follower masters should be able to verify authn tokens

[Alexey Serbin (Code Review)]

Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Hao Hao,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/9373

to look at the new patch set (#12).

Change subject: KUDU-2319 follower masters should be able to verify authn tokens
..

KUDU-2319 follower masters should be able to verify authn tokens

This patch addresses the issue that follower masters could not accept
authn tokens to authenticate clients because they didn't have keys for
token signature verification.

Added a couple of tests:
  * SecurityMasterAuthTest::FollowerTokenVerificationKeys
  * ConnectToFollowerMasterTest::AuthnTokenVerifierHaveKeys

The former is more a unit test; the latter is an integration test.

Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/security-itest.cc
R src/kudu/integration-tests/security-master-auth-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
5 files changed, 233 insertions(+), 24 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/9373/12
--
To view, visit http://gerrit.cloudera.org:8080/9373
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
Gerrit-Change-Number: 9373
Gerrit-PatchSet: 12
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2319 follower masters should be able to verify authn tokens

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9373 )

Change subject: KUDU-2319 follower masters should be able to verify authn tokens
..


Patch Set 11:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9373/11/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/9373/11/src/kudu/master/catalog_manager.cc@1034
PS11, Line 1034: } else {
   : if (!key
this can be un-nested one level with an 'else if', which I think will make it 
easier to follow as well.


http://gerrit.cloudera.org:8080/#/c/9373/11/src/kudu/master/catalog_manager.cc@1042
PS11, Line 1042: ) in this case would lead to
   :   // waiting till next cycle of TSK rotation. Returning 
non-OK status will
   :   // make the periodic task of 
CatalogManagerBgTasks::Run() to retry
   :   // fetching the keys.
I'm not seeing how the return status from this method is effecting 
CatalogManagerBgTasks::Run() in this way.  From what I can see it's simply 
logging a warning if it's a non-OK success status.  Either way it appears to go 
back through the loop 1 second later (line 569)?



--
To view, visit http://gerrit.cloudera.org:8080/9373
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
Gerrit-Change-Number: 9373
Gerrit-PatchSet: 11
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 02 Mar 2018 17:52:06 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2319 follower masters should be able to verify authn tokens

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9373 )

Change subject: KUDU-2319 follower masters should be able to verify authn tokens
..


Patch Set 11:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9373/11/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/9373/11/src/kudu/master/catalog_manager.cc@1046
PS11, Line 1046:   s = Status::NotFound("no TSK found in the system table");
> Talked to Hao offline.  As I understood, Hao's concern was that the origina
Thanks Alexey. After our offline conversations, it looks good to me now,



--
To view, visit http://gerrit.cloudera.org:8080/9373
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
Gerrit-Change-Number: 9373
Gerrit-PatchSet: 11
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 28 Feb 2018 01:44:07 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2319 follower masters should be able to verify authn tokens

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9373 )

Change subject: KUDU-2319 follower masters should be able to verify authn tokens
..


Patch Set 11: Code-Review+1


--
To view, visit http://gerrit.cloudera.org:8080/9373
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
Gerrit-Change-Number: 9373
Gerrit-PatchSet: 11
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 28 Feb 2018 01:44:21 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2319 follower masters should be able to verify authn tokens

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9373 )

Change subject: KUDU-2319 follower masters should be able to verify authn tokens
..


Patch Set 11:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9373/11/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/9373/11/src/kudu/master/catalog_manager.cc@1046
PS11, Line 1046:   s = Status::NotFound("no TSK found in the system table");
> Also, this condition can only happen on the very first start of a freshly d
Talked to Hao offline.  As I understood, Hao's concern was that the original 
patch (i.e. PS1) didn't contain this addition.

I found this condition might happen when running the new test many times -- it 
happened in a tiny fraction of all runs, but it was caught by the newly added 
test anyway.  So, in that regard I think we don't need a new test to cover that 
since the newly added test catches that condition (not in every run, though).



--
To view, visit http://gerrit.cloudera.org:8080/9373
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
Gerrit-Change-Number: 9373
Gerrit-PatchSet: 11
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 28 Feb 2018 01:21:44 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2319 follower masters should be able to verify authn tokens

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9373 )

Change subject: KUDU-2319 follower masters should be able to verify authn tokens
..


Patch Set 11:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9373/11/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/9373/11/src/kudu/master/catalog_manager.cc@1046
PS11, Line 1046:   s = Status::NotFound("no TSK found in the system table");
> I'm not sure what to test for in this case.
Also, this condition can only happen on the very first start of a freshly 
deployed multi-master Kudu cluster.  When restarting a master daemon, at least 
one TSK should be around.



--
To view, visit http://gerrit.cloudera.org:8080/9373
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
Gerrit-Change-Number: 9373
Gerrit-PatchSet: 11
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 27 Feb 2018 16:11:56 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2319 follower masters should be able to verify authn tokens

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9373 )

Change subject: KUDU-2319 follower masters should be able to verify authn tokens
..


Patch Set 11:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9373/11/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/9373/11/src/kudu/master/catalog_manager.cc@1046
PS11, Line 1046:   s = Status::NotFound("no TSK found in the system table");
> Can we add a test to cover this scenario?
I'm not sure what to test for in this case.

This state is intermittent and will not persist for a long time, because the 
leader master will eventually generate at least one TSK.



--
To view, visit http://gerrit.cloudera.org:8080/9373
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
Gerrit-Change-Number: 9373
Gerrit-PatchSet: 11
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 27 Feb 2018 16:05:11 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2319 follower masters should be able to verify authn tokens

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9373 )

Change subject: KUDU-2319 follower masters should be able to verify authn tokens
..


Patch Set 11:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9373/11/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/9373/11/src/kudu/master/catalog_manager.cc@1046
PS11, Line 1046:   s = Status::NotFound("no TSK found in the system table");
Can we add a test to cover this scenario?



--
To view, visit http://gerrit.cloudera.org:8080/9373
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
Gerrit-Change-Number: 9373
Gerrit-PatchSet: 11
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 27 Feb 2018 05:00:15 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2319 follower masters should be able to verify authn tokens

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9373 )

Change subject: KUDU-2319 follower masters should be able to verify authn tokens
..


Patch Set 10:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9373/6/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/9373/6/src/kudu/master/catalog_manager.cc@1050
PS6, Line 1050: MonoDelta::FromSeconds(FLAGS_tsk_rotation_seconds);
  :   const auto now = MonoTime::Now();
> Seems like these first two clauses can be removed if you just initialize 'l
Huh, it turned out that initializing last_tspk_run with MonoTime::Min() was not 
a good idea, not at all.  The catch is that MonoTime::Now() is not based on 
wall clock, it's just some monotonic kernel clock.

I waste a lot of time trying to understand why the test passes on my machines 
but fails at dist-test machines.



--
To view, visit http://gerrit.cloudera.org:8080/9373
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
Gerrit-Change-Number: 9373
Gerrit-PatchSet: 10
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Sat, 24 Feb 2018 06:05:08 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2319 follower masters should be able to verify authn tokens

[Alexey Serbin (Code Review)]

Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Hao Hao,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/9373

to look at the new patch set (#11).

Change subject: KUDU-2319 follower masters should be able to verify authn tokens
..

KUDU-2319 follower masters should be able to verify authn tokens

This patch addresses the issue that follower masters could not accept
authn tokens to authenticate clients because they didn't have keys for
token signature verification.

Added a couple of tests:
  * SecurityMasterAuthTest::FollowerTokenVerificationKeys
  * ConnectToFollowerMasterTest::AuthnTokenVerifierHaveKeys

The former is more a unit test; the latter is an integration test.

Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/security-itest.cc
R src/kudu/integration-tests/security-master-auth-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
5 files changed, 234 insertions(+), 24 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/9373/11
--
To view, visit http://gerrit.cloudera.org:8080/9373
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
Gerrit-Change-Number: 9373
Gerrit-PatchSet: 11
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2319 follower masters should be able to verify authn tokens

[Alexey Serbin (Code Review)]

Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Hao Hao,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/9373

to look at the new patch set (#10).

Change subject: KUDU-2319 follower masters should be able to verify authn tokens
..

KUDU-2319 follower masters should be able to verify authn tokens

This patch addresses the issue that follower masters could not accept
authn tokens to authenticate clients because they didn't have keys for
token signature verification.

Added a couple of tests:
  * SecurityMasterAuthTest::FollowerTokenVerificationKeys
  * ConnectToFollowerMasterTest::AuthnTokenVerifierHaveKeys

The former is more a unit test; the latter is an integration test.

Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/security-itest.cc
R src/kudu/integration-tests/security-master-auth-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
5 files changed, 223 insertions(+), 23 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/9373/10
--
To view, visit http://gerrit.cloudera.org:8080/9373
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
Gerrit-Change-Number: 9373
Gerrit-PatchSet: 10
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2319 follower masters should be able to verify authn tokens

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9373 )

Change subject: KUDU-2319 follower masters should be able to verify authn tokens
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9373/6/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/9373/6/src/kudu/master/catalog_manager.cc@1040
PS6, Line 1040:   static const auto kTskRotationInterval =
> I agree it's better to be certain here, especially if you feel strong about
I think the resource below is more credible than any stack-overflow articles:

http://en.cppreference.com/w/cpp/language/storage_duration#Static_local_variables

And it's closer to what I saw in the standard.



--
To view, visit http://gerrit.cloudera.org:8080/9373
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
Gerrit-Change-Number: 9373
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 23 Feb 2018 23:34:10 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2319 follower masters should be able to verify authn tokens

[Alexey Serbin (Code Review)]

Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Hao Hao,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/9373

to look at the new patch set (#9).

Change subject: KUDU-2319 follower masters should be able to verify authn tokens
..

KUDU-2319 follower masters should be able to verify authn tokens

This patch addresses the issue that follower masters could not accept
authn tokens to authenticate clients because they didn't have keys for
token signature verification.

Added a couple of tests:
  * SecurityMasterAuthTest::FollowerTokenVerificationKeys
  * ConnectToFollowerMasterTest::AuthnTokenVerifierHaveKeys

The former is more a unit test; the latter is an integration test.

Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/security-itest.cc
R src/kudu/integration-tests/security-master-auth-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
5 files changed, 225 insertions(+), 23 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/9373/9
--
To view, visit http://gerrit.cloudera.org:8080/9373
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
Gerrit-Change-Number: 9373
Gerrit-PatchSet: 9
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2319 follower masters should be able to verify authn tokens

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9373 )

Change subject: KUDU-2319 follower masters should be able to verify authn tokens
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9373/6/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/9373/6/src/kudu/master/catalog_manager.cc@1040
PS6, Line 1040:   static const auto kTskRotationInterval =
> According to https://stackoverflow.com/questions/55510/when-do-function-lev
I agree it's better to be certain here, especially if you feel strong about 
that.  But I'm sure it would work as needed, though.

But just for fun, I dig a bit more.

And actually, that's true not only for non-POD types:


#include 

using namespace std;

static int X = 100;

void foo() {
  const static int x = X;
  cout << x << endl;
}

int main() {
   X = 200;
   foo();

   return 0;
}



But it might depend on the whether X in the same compilation unit, though.

In addtioon, MonoDelta is _not_ a POD type because it's not a trivial type:

http://en.cppreference.com/w/cpp/language/default_constructor#Trivial_default_constructor

Just for fun I decided to check std::is_pod(), and it confirmed MonoDelta 
not a POD:


TEST_F(SecurityITest, IsMonoDeltaPod) {
  ASSERT_FALSE(std::is_pod::value);
}


==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from SecurityITest
[ RUN  ] SecurityITest.IsMonoDeltaPod
[   OK ] SecurityITest.IsMonoDeltaPod (2 ms)
[--] 1 test from SecurityITest (2 ms total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (2 ms total)
[  PASSED  ] 1 test.



--
To view, visit http://gerrit.cloudera.org:8080/9373
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
Gerrit-Change-Number: 9373
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 23 Feb 2018 23:09:15 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2319 follower masters should be able to verify authn tokens

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9373 )

Change subject: KUDU-2319 follower masters should be able to verify authn tokens
..


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9373/6/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/9373/6/src/kudu/master/catalog_manager.cc@1040
PS6, Line 1040: Status CatalogManager::PrepareFollower(MonoTime* last_tspk_run) 
{
> I don't think it's fishy -- this is initialized first time the function is
According to 
https://stackoverflow.com/questions/55510/when-do-function-level-static-variables-get-allocated-initialized
 this is only true of non-POD types, and it seems plausible that MonoDelta is a 
POD type (I haven't checked).  Given that this is a tricky corner of the 
language spec, and the overhead of just making it a normal local is so low, I'd 
suggest not making it static.



--
To view, visit http://gerrit.cloudera.org:8080/9373
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
Gerrit-Change-Number: 9373
Gerrit-PatchSet: 8
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 23 Feb 2018 22:18:24 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2319 follower masters should be able to verify authn tokens

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9373 )

Change subject: KUDU-2319 follower masters should be able to verify authn tokens
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9373/6/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/9373/6/src/kudu/master/catalog_manager.cc@1032
PS6, Line 1032: LOG_WITH_PREFIX(INFO) << kDescription << ": success";
> Done
I did that but that didn't work very well here since keys are being passed 
around using the moving semantics.  I think it should be enough to have that 
VLOG(2) in CatalogManager::LoadTspkEntries().



--
To view, visit http://gerrit.cloudera.org:8080/9373
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
Gerrit-Change-Number: 9373
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 23 Feb 2018 22:10:23 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2319 follower masters should be able to verify authn tokens

[Alexey Serbin (Code Review)]

Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Hao Hao,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/9373

to look at the new patch set (#8).

Change subject: KUDU-2319 follower masters should be able to verify authn tokens
..

KUDU-2319 follower masters should be able to verify authn tokens

This patch addresses the issue that follower masters could not accept
authn tokens to authenticate clients because they didn't have keys for
token signature verification.

Added a couple of tests:
  * SecurityMasterAuthTest::FollowerTokenVerificationKeys
  * ConnectToFollowerMasterTest::AuthnTokenVerifierHaveKeys

The former is more a unit test; the latter is an integration test.

Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/security-itest.cc
R src/kudu/integration-tests/security-master-auth-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
5 files changed, 221 insertions(+), 23 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/9373/8
--
To view, visit http://gerrit.cloudera.org:8080/9373
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
Gerrit-Change-Number: 9373
Gerrit-PatchSet: 8
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2319 follower masters should be able to verify authn tokens

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9373 )

Change subject: KUDU-2319 follower masters should be able to verify authn tokens
..


Patch Set 5:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/9373/6/src/kudu/integration-tests/security-master-auth-itest.cc
File src/kudu/integration-tests/security-master-auth-itest.cc:

http://gerrit.cloudera.org:8080/#/c/9373/6/src/kudu/integration-tests/security-master-auth-itest.cc@119
PS6, Line 119:   ASSERT_LE(0, verifier.GetMaxKnownKeySequenceNumber());
> Shouldn't this be ASSERT_LT?
In current implementation, valid TSK sequence numbers start with 0.  E.g., 
below an excerpt from master's log:

I0222 19:56:02.962090 139954 catalog_manager.cc:3847] T 
 P 0139908237bb4734833947c72c0faff3: Generated 
new TSK 0


http://gerrit.cloudera.org:8080/#/c/9373/6/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/9373/6/src/kudu/master/catalog_manager.cc@1032
PS6, Line 1032: LOG_WITH_PREFIX(INFO) << kDescription << ": success";
> Consider adding the last key ID to this message.
Done


http://gerrit.cloudera.org:8080/#/c/9373/6/src/kudu/master/catalog_manager.cc@1040
PS6, Line 1040:   static const auto kTskRotationInterval =
> 'static const' seems a little fishy here - does this pick up the non-defaul
I don't think it's fishy -- this is initialized first time the function is 
called.  That's how static variables work in the function scope, and as far as 
I remember that's in the C++ standard.

By the time this function is called, the flags should have been already 
initialized with appropriate override of FLAGS_tsk_rotation_seconds, so yes, it 
picks up the non-default value set via the flag.


http://gerrit.cloudera.org:8080/#/c/9373/6/src/kudu/master/catalog_manager.cc@1050
PS6, Line 1050: 
master_->messenger()->token_verifier().GetMaxKnownKeySequenceNumber() < 0 ||
  :   !last_tspk_run->Initialized()
> Seems like these first two clauses can be removed if you just initialize 'l
That's a good observation -- yep, last_tspk_run is updated only on successful 
run.


http://gerrit.cloudera.org:8080/#/c/9373/6/src/kudu/master/catalog_manager.cc@3875
PS6, Line 3875: LOG_WITH_PREFIX(INFO) << "Read public part of TSK: " << 
key.key_seq_num();
> This seems like it's going to be pretty chatty, maybe vlog?  We're already
Good idea!



--
To view, visit http://gerrit.cloudera.org:8080/9373
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
Gerrit-Change-Number: 9373
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 23 Feb 2018 21:49:10 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2319 follower masters should be able to verify authn tokens

[Alexey Serbin (Code Review)]

Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Hao Hao,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/9373

to look at the new patch set (#7).

Change subject: KUDU-2319 follower masters should be able to verify authn tokens
..

KUDU-2319 follower masters should be able to verify authn tokens

This patch addresses the issue that follower masters could not accept
authn tokens to authenticate clients because they didn't have keys for
token signature verification.

Added a couple of tests:
  * SecurityMasterAuthTest::FollowerTokenVerificationKeys
  * ConnectToFollowerMasterTest::AuthnTokenVerifierHaveKeys

The former is more a unit test; the latter is an integration test.

Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/security-itest.cc
R src/kudu/integration-tests/security-master-auth-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
5 files changed, 223 insertions(+), 23 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/9373/7
--
To view, visit http://gerrit.cloudera.org:8080/9373
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
Gerrit-Change-Number: 9373
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2319 follower masters should be able to verify authn tokens

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9373 )

Change subject: KUDU-2319 follower masters should be able to verify authn tokens
..


Patch Set 6:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/9373/6/src/kudu/integration-tests/security-master-auth-itest.cc
File src/kudu/integration-tests/security-master-auth-itest.cc:

http://gerrit.cloudera.org:8080/#/c/9373/6/src/kudu/integration-tests/security-master-auth-itest.cc@119
PS6, Line 119:   ASSERT_LE(0, verifier.GetMaxKnownKeySequenceNumber());
Shouldn't this be ASSERT_LT?


http://gerrit.cloudera.org:8080/#/c/9373/6/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/9373/6/src/kudu/master/catalog_manager.cc@1032
PS6, Line 1032: LOG_WITH_PREFIX(INFO) << kDescription << ": success";
Consider adding the last key ID to this message.


http://gerrit.cloudera.org:8080/#/c/9373/6/src/kudu/master/catalog_manager.cc@1040
PS6, Line 1040:   static const auto kTskRotationInterval =
'static const' seems a little fishy here - does this pick up the non-default 
value if it's set via a flag?  I don't see how it would.  Seem better to just 
have it be a normal local variable, the overhead can't be very high.


http://gerrit.cloudera.org:8080/#/c/9373/6/src/kudu/master/catalog_manager.cc@1050
PS6, Line 1050: 
master_->messenger()->token_verifier().GetMaxKnownKeySequenceNumber() < 0 ||
  :   !last_tspk_run->Initialized()
Seems like these first two clauses can be removed if you just initialize 
'last_tspk_run' to 0, e.g. the unix epoch.


http://gerrit.cloudera.org:8080/#/c/9373/6/src/kudu/master/catalog_manager.cc@3875
PS6, Line 3875: LOG_WITH_PREFIX(INFO) << "Read public part of TSK: " << 
key.key_seq_num();
This seems like it's going to be pretty chatty, maybe vlog?  We're already INFO 
logging when the entire process is done, which seems good enough to me.



--
To view, visit http://gerrit.cloudera.org:8080/9373
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
Gerrit-Change-Number: 9373
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 23 Feb 2018 18:42:52 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2319 follower masters should be able to verify authn tokens

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9373 )

Change subject: KUDU-2319 follower masters should be able to verify authn tokens
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9373/5/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:

http://gerrit.cloudera.org:8080/#/c/9373/5/src/kudu/integration-tests/security-itest.cc@408
PS5, Line 408:   { "required", "required", },
> You mean client 'wouldn't' instead? :)
Yep, that seems to be a typo :)

While negotiating connections, client never sends authn tokens to 
non-authenticated serves.  When encryption is disabled, the client cannot 
authenticate servers.



--
To view, visit http://gerrit.cloudera.org:8080/9373
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
Gerrit-Change-Number: 9373
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 23 Feb 2018 17:01:47 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2319 follower masters should be able to verify authn tokens

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9373 )

Change subject: KUDU-2319 follower masters should be able to verify authn tokens
..


Patch Set 5:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/9373/5/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:

http://gerrit.cloudera.org:8080/#/c/9373/5/src/kudu/integration-tests/security-itest.cc@408
PS5, Line 408:   { "required", "required", },
> Do we need to test the case encryption is disabled?
It does not make sense to test when encryption is disabled, because in that 
case client would send authn token even if it had it.


http://gerrit.cloudera.org:8080/#/c/9373/5/src/kudu/integration-tests/security-itest.cc@467
PS5, Line 467:   ASSERT_EVENTUALLY([&] {
> Can you add a comment what is this block of trying to test?
Sure, will do.


http://gerrit.cloudera.org:8080/#/c/9373/5/src/kudu/integration-tests/security-itest.cc@497
PS5, Line 497: ASSERT_OK(client->ListTables());
> Can we somehow ensure that client is not getting any exceptions from the fo
I don't think we need it here: the ASSERT_EVENTUALLY above verifies that the 
client is able to authenticate against both masters using its authn token.

Here we are just verifying that client is able to connect and perform basic 
operations once it's using correct set of masters' endpoints.



--
To view, visit http://gerrit.cloudera.org:8080/9373
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
Gerrit-Change-Number: 9373
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 23 Feb 2018 07:34:06 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2319 follower masters should be able to verify authn tokens

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9373 )

Change subject: KUDU-2319 follower masters should be able to verify authn tokens
..


Patch Set 5:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/9373/5/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:

http://gerrit.cloudera.org:8080/#/c/9373/5/src/kudu/integration-tests/security-itest.cc@408
PS5, Line 408:   { "required", "required", },
Do we need to test the case encryption is disabled?


http://gerrit.cloudera.org:8080/#/c/9373/5/src/kudu/integration-tests/security-itest.cc@467
PS5, Line 467:   ASSERT_EVENTUALLY([&] {
Can you add a comment what is this block of trying to test?


http://gerrit.cloudera.org:8080/#/c/9373/5/src/kudu/integration-tests/security-itest.cc@497
PS5, Line 497: ASSERT_OK(client->ListTables());
Can we somehow ensure that client is not getting any exceptions from the 
followers here?



--
To view, visit http://gerrit.cloudera.org:8080/9373
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
Gerrit-Change-Number: 9373
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 23 Feb 2018 07:24:47 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2319 follower masters should be able to verify authn tokens

[Alexey Serbin (Code Review)]

Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Hao Hao,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/9373

to look at the new patch set (#5).

Change subject: KUDU-2319 follower masters should be able to verify authn tokens
..

KUDU-2319 follower masters should be able to verify authn tokens

This patch addresses the issue that follower masters could not accept
authn tokens to authenticate clients because they didn't have keys for
token signature verification.

Added a couple of tests:
  * FollowerMastersHaveTokenVerificationKeys
  * ConnectToFollowerMasterTest::AuthnTokenVerifierHaveKeys

The former is more a unit test; the latter is an integration test.

Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/security-itest.cc
R src/kudu/integration-tests/security-master-auth-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
5 files changed, 217 insertions(+), 23 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/9373/5
--
To view, visit http://gerrit.cloudera.org:8080/9373
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
Gerrit-Change-Number: 9373
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2319 follower masters should be able to verify authn tokens

[Alexey Serbin (Code Review)]

Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Hao Hao,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/9373

to look at the new patch set (#4).

Change subject: KUDU-2319 follower masters should be able to verify authn tokens
..

KUDU-2319 follower masters should be able to verify authn tokens

This patch addresses the issue that follower masters could not accept
authn tokens to authenticate clients because they didn't have keys for
token signature verification.

Added a couple of tests:
  * FollowerMastersHaveTokenVerificationKeys
  * ConnectToFollowerMasterTest::AuthnTokenVerifierHaveKeys

The former is more a unit test; the latter is an integration test.

Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
---
M src/kudu/integration-tests/security-itest.cc
M src/kudu/integration-tests/security-master-certificates-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
4 files changed, 216 insertions(+), 20 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/9373/4
--
To view, visit http://gerrit.cloudera.org:8080/9373
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7
Gerrit-Change-Number: 9373
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot