Dan Burkert 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 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 Line 97: RETURN_NOT_OK(verifier_->ImportKeys({ public_key_pb })); Is this the only place this is called? Could consider either batch importing the keys, or just changing it to take a single key. Line 107: tsk_deque_.clear(); Does this need to check that the imported keys are more recent than the current keys in the deque? Line 168: const auto* key = tsk_deque_.front().get(); I'm confused by this algorithm. I would have expected the criteria about whether a new key is needed to only depend on the last key in the queue. - If there is no second key in the queue, then always get a new key - then, I would check the condition you have below, but based on the last key's expire_time. This would have the effect of double rotating the initial key, but I think the header comment is saying we should probably do that anyway. I *think* the current algorithm would rotate much more aggressively than necessary, but I haven't tested that hypothesis. Line 176: if (key->expire_time() - key_validity_seconds_ + key_rotation_seconds_ <= For clarity, consider changing this to auto remaining_period = key->expire_time() - now; if (remaining_period <= key_validity_seconds_ - key_rotation_seconds_) { .... } I found that a little easier to reason through. PS14, Line 177: now) this wrap is hurting legibility. PS14, Line 209: tsk_deque_.emplace_back(); : tsk.swap(tsk_deque_.back()); shouldn't this just be tsk_deque_.emplace_back(std::move(tsk)) Line 225: bool rotated = false; I think this would be simpler if you initially set has_rotated to false, then only change it in the 1 success path. Line 234: if (key->expire_time() - key_validity_seconds_ + 2 * key_rotation_seconds_ <= Same as above, consider auto remaining_period = key->expire_time() - now; if (remaining_period <= key_validity_seconds_ - 2 * key_rotation_seconds_) { .... } PS14, Line 235: WallTime_Now() Was the WallTime_Now() done outside the lock for perf reasons above? If so consider doing that here as well. Line 235: WallTime_Now()) { Same, confusing wrap. 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(...) 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 PS14, Line 92: TSK considered TSK is considered PS14, Line 183: possible to : // examine the key process the key as needed nit Line 198: Status CheckNeedKey(std::unique_ptr<TokenSigningPrivateKey>* tsk) const; WARN_UNUSED_RESULT here and below. Line 222: const TokenVerifier& verifier() { return *verifier_; } I think this should be a const method. -- 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
