Alexey Serbin has posted comments on this change. Change subject: [security] tailored TokenSigner for system catalog ......................................................................
Patch Set 14: (18 comments) http://gerrit.cloudera.org:8080/#/c/5930/14/src/kudu/master/master.cc File src/kudu/master/master.cc: PS14, Line 59: n > capitalize here and below I thought we have switched to the style with lower-case letters for description of the flags. All right, I'll will return that back. http://gerrit.cloudera.org:8080/#/c/5930/14/src/kudu/security/token_signer.cc File src/kudu/security/token_signer.cc: PS14, Line 71: & > ampersand with type woops, I thought it has been fixed already. Line 97: RETURN_NOT_OK(verifier_->ImportKeys({ public_key_pb })); > Is this the only place this is called? Could consider either batch importi I don't think it's worth adding a method just for this case. Batching keys import would make the code more complex: it would be necessary to partially duplicate the logic and make a second pass over the input keys. Line 107: tsk_deque_.clear(); > Does this need to check that the imported keys are more recent than the cur Good catch. Yes, I think it's worth to be a little bit more defensive here. Line 168: const auto* key = tsk_deque_.front().get(); > I'm confused by this algorithm. I would have expected the criteria about w Try to think of that the following way: * It's not worth creating a new key before reaching 'computed' active phase of the current key (that's about the second step of the algorithm you describe). This also addresses cases when previously stored key generated with shorter rotation interval is loaded into a master instance running with longer rotation interval. * a new key is not created if there are 2 keys in the queue already. * once there are 2 keys in the queue and CheckNeedKey() returns false, a call to TryRotate() will eventually pop the inactive key out of the queue, setting the next key as the active one. * the CheckNeedKey()/AddKey() + TryRotate() sequence is being called periodically, much often than the rotation interval. To me, this simplifies this algorithm so it's not necessary to look into any other key besides the very first in the keys queue. BTW, if looking at the expiration time of not the very first key in the queue, but the last one, it's also necessary to set the expiration time of the next key based not on WallTime_Now(), but on the expire time of the last key as well. Also, I don't think the 'active phase start' criterion is applicable to any other key except for the current one. I don't think current algorithm rotates more aggressively than it's necessary. It _would_ if it were allowing to create a new key when there are 2 keys in the queue. However, if you feel strong about this, we can try to converge on something which seems acceptable to you. Line 176: if (key->expire_time() - key_validity_seconds_ + key_rotation_seconds_ <= > For clarity, consider changing this to Interesting. To me it's much easier to look at that in the absolute time scale, not intervals. The diagram above is for illustration of that relation. The code you propose is much harder to me to comprehend and ensure it's correct. :) Would it help if I add the parenthesis to separate key's creation time: if ((key->expire_time() - key_validity_seconds_) + key_rotation_seconds_ <= now) { ... } Or: const auto key_creation_time = key->expire_time() - key_validity_seconds_; if (key_creation_time + key_rotation_seconds_ <= now) { ... } ? PS14, Line 177: now) > this wrap is hurting legibility. Done PS14, Line 209: tsk_deque_.emplace_back(); : tsk.swap(tsk_deque_.back()); > shouldn't this just be yep, that would be easier to read. Line 225: bool rotated = false; > I think this would be simpler if you initially set has_rotated to false, th Done Line 234: if (key->expire_time() - key_validity_seconds_ + 2 * key_rotation_seconds_ <= > Same as above, consider The same -- to me it's much easier to compare points in the absolute time scale. PS14, Line 235: WallTime_Now() > Was the WallTime_Now() done outside the lock for perf reasons above? If so That time WallTime_Now() was called outside the lock for consistency -- it was necessary to supply that value in more than one place. Why is that perfomance-wise beneficial to call WallTime_Now() outside of the lock? Line 235: WallTime_Now()) { > Same, confusing wrap. Done PS14, Line 253: unique_ptr<TokenSigningPrivateKey> new_tsk( : new TokenSigningPrivateKey(key_seq_num, key_expiration, std::move(key))); : tsk->swap(new_tsk); > *tsk = new TokenSigningPrivateKey(...) replaced with tsk.reset(new ...) http://gerrit.cloudera.org:8080/#/c/5930/14/src/kudu/security/token_signer.h File src/kudu/security/token_signer.h: PS14, Line 44: aggregated > 'included' may be clearer than 'aggregated' here and below Done PS14, Line 92: TSK considered > TSK is considered Done PS14, Line 183: possible to : // examine the key process the key as needed > nit Done Line 198: Status CheckNeedKey(std::unique_ptr<TokenSigningPrivateKey>* tsk) const; > WARN_UNUSED_RESULT here and below. Done Line 222: const TokenVerifier& verifier() { return *verifier_; } > I think this should be a const method. sure -- 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: 14 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: 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
