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/03/09 18:54:05 UTC

[kudu-CR] [security] avoid sparse seq numbers in TokenSigner

Alexey Serbin has uploaded a new change for review.

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

Change subject: [security] avoid sparse seq numbers in TokenSigner
......................................................................

[security] avoid sparse seq numbers in TokenSigner

Changed the way how TokenSigner assigns sequence numbers.  The new way
of assigning sequence numbers allows to avoid sparse sequence numbers
for the CheckNeedKey()-try-to-store-key-AddKey() sequence if
the 'try-to-store-key' part fails.

Added unit test for that and some other scenarios as well.

Change-Id: Ib84f3d4f2596b3e75d1e0132f1e3362812d5799e
---
M src/kudu/security/token-test.cc
M src/kudu/security/token_signer.cc
M src/kudu/security/token_signer.h
M src/kudu/security/token_signing_key.h
4 files changed, 154 insertions(+), 13 deletions(-)


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

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

[kudu-CR] [security] avoid sparse seq numbers in TokenSigner

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

Alexey Serbin has posted comments on this change.

Change subject: [security] avoid sparse seq numbers in TokenSigner
......................................................................

Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/6329/1/src/kudu/security/token-test.cc
File src/kudu/security/token-test.cc:

Line 117: TEST_F(TokenTest, TestTokenSignerAddKeyAfterImport) {
> As mentioned on slack, a test for the failure case in the commit message wo
Good catch!

Done: a new test added.

PS1, Line 146: valie
> valid
Done

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

PS1, Line 205: It's not a problem to call this method multiple times but call the AddKey()
: // method only once, effectively discarding all the generated keys except for
: // the key passed to the AddKey() call as a parameter. In other words,
: // it's possible and not a problem to have 'holes' in the key sequence
: // numbers. Other components working with verification of the signed tokens
: // should take that into account.
> Should this be updated? I think the following sequence would now be proble
Right, this needs to be updated.

Yep, the former sequence should fail during the second AddKey() call. The latter sequence should work with no issues.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib84f3d4f2596b3e75d1e0132f1e3362812d5799e
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: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes