You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2017/04/03 20:43:26 UTC

[kudu-CR] [security] fixed shortened TSK validity interval 

Alexey Serbin has uploaded a new change for review.

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

Change subject: [security] fixed shortened TSK validity interval
......................................................................

[security] fixed shortened TSK validity interval

The TSK validity interval should be (authn_token_validity_interval +
tsk_propagation_interval + tsk_rotation_interval), which is
(auth_token_validity_interval + 2 * tsk_rotation_interval) since the
propagation interval is set equal to the rotation interval now.

Prior to this fix, as spotted by Dan, the TSK validity interval was
missing the rotation interval delta, which could lead to situations when
a valid authn token could not be verified due to already expired TSK.

Change-Id: I84f9789276c4b48a3ba5274393fe30c8bf3ea85d
---
M src/kudu/security/token-test.cc
M src/kudu/security/token_signer.cc
M src/kudu/security/token_signer.h
3 files changed, 31 insertions(+), 12 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/6536/1
-- 
To view, visit http://gerrit.cloudera.org:8080/6536
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I84f9789276c4b48a3ba5274393fe30c8bf3ea85d
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>

[kudu-CR] [security] fixed shortened TSK validity interval

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged.

Change subject: [security] fixed shortened TSK validity interval
......................................................................


[security] fixed shortened TSK validity interval

The TSK validity interval should be (authn_token_validity_interval +
tsk_propagation_interval + tsk_rotation_interval), which is
(auth_token_validity_interval + 2 * tsk_rotation_interval) since the
propagation interval is set equal to the rotation interval now.

Prior to this fix, as spotted by Dan, the TSK validity interval was
missing the rotation interval delta, which could lead to situations when
a valid authn token could not be verified due to already expired TSK.

Added an integration test to cover the fixed issue and exercise token
verification during and past token and its TSK lifecycle.

Change-Id: I84f9789276c4b48a3ba5274393fe30c8bf3ea85d
Reviewed-on: http://gerrit.cloudera.org:8080/6536
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert <da...@apache.org>
---
M src/kudu/integration-tests/token_signer-itest.cc
M src/kudu/security/token-test.cc
M src/kudu/security/token_signer.cc
M src/kudu/security/token_signer.h
4 files changed, 190 insertions(+), 39 deletions(-)

Approvals:
  Dan Burkert: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I84f9789276c4b48a3ba5274393fe30c8bf3ea85d
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] [security] fixed shortened TSK validity interval

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: [security] fixed shortened TSK validity interval
......................................................................

[security] fixed shortened TSK validity interval

The TSK validity interval should be (authn_token_validity_interval +
tsk_propagation_interval + tsk_rotation_interval), which is
(auth_token_validity_interval + 2 * tsk_rotation_interval) since the
propagation interval is set equal to the rotation interval now.

Prior to this fix, as spotted by Dan, the TSK validity interval was
missing the rotation interval delta, which could lead to situations when
a valid authn token could not be verified due to already expired TSK.

Added an integration test to cover the fixed issue and exercise token
verification during and past token and its TSK lifecycle.

Change-Id: I84f9789276c4b48a3ba5274393fe30c8bf3ea85d
---
M src/kudu/integration-tests/token_signer-itest.cc
M src/kudu/security/token-test.cc
M src/kudu/security/token_signer.cc
M src/kudu/security/token_signer.h
4 files changed, 190 insertions(+), 39 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/6536/2
-- 
To view, visit http://gerrit.cloudera.org:8080/6536
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I84f9789276c4b48a3ba5274393fe30c8bf3ea85d
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] [security] fixed shortened TSK validity interval

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: [security] fixed shortened TSK validity interval
......................................................................

[security] fixed shortened TSK validity interval

The TSK validity interval should be (authn_token_validity_interval +
tsk_propagation_interval + tsk_rotation_interval), which is
(auth_token_validity_interval + 2 * tsk_rotation_interval) since the
propagation interval is set equal to the rotation interval now.

Prior to this fix, as spotted by Dan, the TSK validity interval was
missing the rotation interval delta, which could lead to situations when
a valid authn token could not be verified due to already expired TSK.

Added an integration test to cover the fixed issue and exercise token
verification during and past token and its TSK lifecycle.

Change-Id: I84f9789276c4b48a3ba5274393fe30c8bf3ea85d
---
M src/kudu/integration-tests/token_signer-itest.cc
M src/kudu/security/token-test.cc
M src/kudu/security/token_signer.cc
M src/kudu/security/token_signer.h
4 files changed, 190 insertions(+), 39 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/6536/3
-- 
To view, visit http://gerrit.cloudera.org:8080/6536
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I84f9789276c4b48a3ba5274393fe30c8bf3ea85d
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] [security] fixed shortened TSK validity interval

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: [security] fixed shortened TSK validity interval
......................................................................


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I84f9789276c4b48a3ba5274393fe30c8bf3ea85d
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] [security] fixed shortened TSK validity interval

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: [security] fixed shortened TSK validity interval
......................................................................


Patch Set 1:

(2 comments)

> (2 comments)
 > 
 > Do we have a (slow) test case that goes through a full cycle of
 > keys after issuing a token, and making sure it can't be verified? 
 > I think that would have caught this issue.

We have a unit test which verifies that the system sees TSK as expired after its expiration interval (TokenTest::TestIsCurrentKeyValid), but it does not cover the full cycle of TSK and authn token signed by the TSK.  I'll that as an integration test.

http://gerrit.cloudera.org:8080/#/c/6536/1/src/kudu/security/token_signer.h
File src/kudu/security/token_signer.h:

Line 143: // Day          1    2    3    4    5    6    7
> It may be possible to reuse the diagram above by explaining that the TSK va
Yep, I think it makes sense, especially if we de-facto already introduced the inactivity interval which corresponds to the maxium possible auth token validity interval.  I'll update this correspondingly.


Line 193:   //   key_validity = key_rotation + authn_token_validity.
> Does this need to be updated as well?
Good catch!  Sure, this needs to be updated as well.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I84f9789276c4b48a3ba5274393fe30c8bf3ea85d
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [security] fixed shortened TSK validity interval

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: [security] fixed shortened TSK validity interval
......................................................................


Patch Set 1:

(2 comments)

Do we have a (slow) test case that goes through a full cycle of keys after issuing a token, and making sure it can't be verified?  I think that would have caught this issue.

http://gerrit.cloudera.org:8080/#/c/6536/1/src/kudu/security/token_signer.h
File src/kudu/security/token_signer.h:

Line 143: // Day          1    2    3    4    5    6    7
It may be possible to reuse the diagram above by explaining that the TSK validity must be as long as the 'inactivity interval', which I think make intuitive sense.


Line 193:   //   key_validity = key_rotation + authn_token_validity.
Does this need to be updated as well?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I84f9789276c4b48a3ba5274393fe30c8bf3ea85d
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [security] fixed shortened TSK validity interval

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: [security] fixed shortened TSK validity interval
......................................................................


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/6536/2/src/kudu/integration-tests/token_signer-itest.cc
File src/kudu/integration-tests/token_signer-itest.cc:

Line 253:     }, MonoDelta::FromMilliseconds(5 * FLAGS_heartbeat_interval_ms));
> warning: either cast from 'int' to 'int64_t' (aka 'long') is ineffective, o
Done


PS2, Line 259: very
> typo
Done


PS2, Line 315: int
> should this be equal_to<bool>?
Thanks! Sure -- it should be equal_to<bool>.  It seems I played with counters first, and then realized a boolean flag is enough.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I84f9789276c4b48a3ba5274393fe30c8bf3ea85d
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [security] fixed shortened TSK validity interval

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: [security] fixed shortened TSK validity interval
......................................................................


Patch Set 2:

(2 comments)

LGTM, just a few nits

http://gerrit.cloudera.org:8080/#/c/6536/2/src/kudu/integration-tests/token_signer-itest.cc
File src/kudu/integration-tests/token_signer-itest.cc:

PS2, Line 259: very
typo


PS2, Line 315: int
should this be equal_to<bool>?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I84f9789276c4b48a3ba5274393fe30c8bf3ea85d
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes