Dan Burkert has posted comments on this change. Change subject: [security] tailored TokenSigner for system catalog ......................................................................
Patch Set 7: (3 comments) I have some high level questions about the direction of TokenSigner and AuthnTokenManager. Why is the TokenSigner holding all of the previous TSK public keys? I wouldn't expect a class called 'TokenSigner' to need anything except a single private key and the current sequence number. I realize that something in the master will need to track of the TSK public keys, but that's true of the tserver as well (which ostensibly won't have a TokenSigner), so I don't see why it should be in the TokenSigner. Right now AuthnTokenManager is just a thin wrapper around TokenSigner. Is it pulling its weight? Are we planning to add more responsibilities to AuthnTokenManager in the future which would make it more than just a wrapper? http://gerrit.cloudera.org:8080/#/c/5930/7/src/kudu/security/token-test.cc File src/kudu/security/token-test.cc: Line 278: // The there should be 2 public keys exported. remove 'The' http://gerrit.cloudera.org:8080/#/c/5930/7/src/kudu/security/token_signer.cc File src/kudu/security/token_signer.cc: Line 43: // TODO(PKI): add flag tags duplicate TODO http://gerrit.cloudera.org:8080/#/c/5930/7/src/kudu/security/token_signer.h File src/kudu/security/token_signer.h: PS7, Line 91: CheckGenerateNewSigningKey Should this be GenerateSigningKey? -- To view, visit http://gerrit.cloudera.org:8080/5930 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie2417e2ccba6a1114db366b2f642f95362bf479c Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
