Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1968053002:
Certificate Transparency: Notify STH Observers of known STHs (Closed)
|
Created:
4 years, 7 months ago by Eran Messeri Modified:
4 years, 7 months ago Reviewers:
Ryan Sleevi CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCertificate Transparency: Notify STH Observers of known STHs
When new STHObservers are registered, provide them with an STH from
each log.
BUG=611021
Committed: https://crrev.com/e2c861428cf7096b1d5317f4c033d55180618d2a
Cr-Commit-Position: refs/heads/master@{#394385}
Patch Set 1 #
Total comments: 22
Patch Set 2 : Addressing review comments. #
Total comments: 10
Patch Set 3 : Addressing review comments #
Total comments: 11
Patch Set 4 : Copying STHs locally #
Total comments: 8
Patch Set 5 : Test changed not to rely on fixed tree size #
Total comments: 8
Patch Set 6 : Further unit test improvements #
Total comments: 6
Patch Set 7 : Final operator, test changes #
Messages
Total messages: 23 (6 generated)
Description was changed from ========== Certificate Transparency: Notify STH Observers of known STHs When new STHObservers are registered, provide them with an STH from each log. BUG=611021 ========== to ========== Certificate Transparency: Notify STH Observers of known STHs When new STHObservers are registered, provide them with an STH from each log. BUG=611021 ==========
eranm@chromium.org changed reviewers: + rsleevi@chromium.org
Ryan, because no good deed goes unpunished, please review the fix to the problem with the STHDistributor you've noticed the other day.
https://codereview.chromium.org/1968053002/diff/1/net/cert/sth_distributor.cc File net/cert/sth_distributor.cc (right): https://codereview.chromium.org/1968053002/diff/1/net/cert/sth_distributor.cc... net/cert/sth_distributor.cc:51: for (const SignedTreeHead& sth : observed_sths_) const auto& is fine here. https://codereview.chromium.org/1968053002/diff/1/net/cert/sth_distributor.h File net/cert/sth_distributor.h (right): https://codereview.chromium.org/1968053002/diff/1/net/cert/sth_distributor.h#... net/cert/sth_distributor.h:25: } // namespace internal DESIGN: You don't need to expose this in a public class. https://codereview.chromium.org/1968053002/diff/1/net/cert/sth_distributor.h#... net/cert/sth_distributor.h:52: std::set<SignedTreeHead, internal::STHLogIDComparator> observed_sths_; DESIGN: An std::set<> is pretty inefficient for your problem case, as it forces a lot of dynamic allocation (for the red-black tree), forces the design issue you see above (the internal), and doesn't substantially gain benefits. We're talking a value of N < 10. A linear scan over a vector is perfectly fine
https://codereview.chromium.org/1968053002/diff/1/net/cert/sth_distributor_un... File net/cert/sth_distributor_unittest.cc (right): https://codereview.chromium.org/1968053002/diff/1/net/cert/sth_distributor_un... net/cert/sth_distributor_unittest.cc:21: const char kAnotherLogName[] = "another log"; This doesn't need to be file-level, does it? https://codereview.chromium.org/1968053002/diff/1/net/cert/sth_distributor_un... net/cert/sth_distributor_unittest.cc:25: "\x98\x04\xf9\x1b\xdf\xb8\xe3\x77\xcd\x0e\xc8\x0d\xdc\x10"; The first two variables don't need to be file-level, do they? https://codereview.chromium.org/1968053002/diff/1/net/cert/sth_distributor_un... net/cert/sth_distributor_unittest.cc:28: class StoringSTHObserver : public net::ct::STHObserver { Document https://codereview.chromium.org/1968053002/diff/1/net/cert/sth_distributor_un... net/cert/sth_distributor_unittest.cc:28: class StoringSTHObserver : public net::ct::STHObserver { IWYU: STHObserver https://codereview.chromium.org/1968053002/diff/1/net/cert/sth_distributor_un... net/cert/sth_distributor_unittest.cc:34: std::map<std::string, net::ct::SignedTreeHead> sths; IWYU: map https://codereview.chromium.org/1968053002/diff/1/net/cert/sth_distributor_un... net/cert/sth_distributor_unittest.cc:42: distributor_.reset(new STHDistributor()); There's no need to do this in SetUp versus the constructor, is there? Is there a reason the STHDistributor isn't just a member? https://codereview.chromium.org/1968053002/diff/1/net/cert/sth_distributor_un... net/cert/sth_distributor_unittest.cc:53: second_sth.log_id = std::string(kAnotherLogName); Doesn't need to be a constant, does it? https://codereview.chromium.org/1968053002/diff/1/net/cert/sth_distributor_un... net/cert/sth_distributor_unittest.cc:62: EXPECT_EQ(1u, observer.sths.count(kAnotherLogName)); Document what you're testing, and why. You should be clear about what the expectations are trying to test.
https://codereview.chromium.org/1968053002/diff/1/net/cert/sth_distributor.cc File net/cert/sth_distributor.cc (right): https://codereview.chromium.org/1968053002/diff/1/net/cert/sth_distributor.cc... net/cert/sth_distributor.cc:51: for (const SignedTreeHead& sth : observed_sths_) On 2016/05/12 19:51:28, Ryan Sleevi wrote: > const auto& is fine here. Done. https://codereview.chromium.org/1968053002/diff/1/net/cert/sth_distributor.h File net/cert/sth_distributor.h (right): https://codereview.chromium.org/1968053002/diff/1/net/cert/sth_distributor.h#... net/cert/sth_distributor.h:25: } // namespace internal On 2016/05/12 19:51:28, Ryan Sleevi wrote: > DESIGN: You don't need to expose this in a public class. Done. https://codereview.chromium.org/1968053002/diff/1/net/cert/sth_distributor.h#... net/cert/sth_distributor.h:52: std::set<SignedTreeHead, internal::STHLogIDComparator> observed_sths_; On 2016/05/12 19:51:28, Ryan Sleevi wrote: > DESIGN: An std::set<> is pretty inefficient for your problem case, as it forces > a lot of dynamic allocation (for the red-black tree), forces the design issue > you see above (the internal), and doesn't substantially gain benefits. > > We're talking a value of N < 10. A linear scan over a vector is perfectly fine Done. https://codereview.chromium.org/1968053002/diff/1/net/cert/sth_distributor_un... File net/cert/sth_distributor_unittest.cc (right): https://codereview.chromium.org/1968053002/diff/1/net/cert/sth_distributor_un... net/cert/sth_distributor_unittest.cc:21: const char kAnotherLogName[] = "another log"; On 2016/05/12 19:55:06, Ryan Sleevi wrote: > This doesn't need to be file-level, does it? Done. https://codereview.chromium.org/1968053002/diff/1/net/cert/sth_distributor_un... net/cert/sth_distributor_unittest.cc:25: "\x98\x04\xf9\x1b\xdf\xb8\xe3\x77\xcd\x0e\xc8\x0d\xdc\x10"; On 2016/05/12 19:55:06, Ryan Sleevi wrote: > The first two variables don't need to be file-level, do they? Done. https://codereview.chromium.org/1968053002/diff/1/net/cert/sth_distributor_un... net/cert/sth_distributor_unittest.cc:28: class StoringSTHObserver : public net::ct::STHObserver { On 2016/05/12 19:55:06, Ryan Sleevi wrote: > IWYU: STHObserver Done. https://codereview.chromium.org/1968053002/diff/1/net/cert/sth_distributor_un... net/cert/sth_distributor_unittest.cc:28: class StoringSTHObserver : public net::ct::STHObserver { On 2016/05/12 19:55:06, Ryan Sleevi wrote: > Document Done. https://codereview.chromium.org/1968053002/diff/1/net/cert/sth_distributor_un... net/cert/sth_distributor_unittest.cc:34: std::map<std::string, net::ct::SignedTreeHead> sths; On 2016/05/12 19:55:06, Ryan Sleevi wrote: > IWYU: map Done. https://codereview.chromium.org/1968053002/diff/1/net/cert/sth_distributor_un... net/cert/sth_distributor_unittest.cc:42: distributor_.reset(new STHDistributor()); On 2016/05/12 19:55:06, Ryan Sleevi wrote: > There's no need to do this in SetUp versus the constructor, is there? Is there a > reason the STHDistributor isn't just a member? Done. https://codereview.chromium.org/1968053002/diff/1/net/cert/sth_distributor_un... net/cert/sth_distributor_unittest.cc:53: second_sth.log_id = std::string(kAnotherLogName); On 2016/05/12 19:55:06, Ryan Sleevi wrote: > Doesn't need to be a constant, does it? Done. https://codereview.chromium.org/1968053002/diff/1/net/cert/sth_distributor_un... net/cert/sth_distributor_unittest.cc:62: EXPECT_EQ(1u, observer.sths.count(kAnotherLogName)); On 2016/05/12 19:55:06, Ryan Sleevi wrote: > Document what you're testing, and why. You should be clear about what the > expectations are trying to test. Done.
https://codereview.chromium.org/1968053002/diff/20001/net/cert/sth_distributo... File net/cert/sth_distributor.cc (right): https://codereview.chromium.org/1968053002/diff/20001/net/cert/sth_distributo... net/cert/sth_distributor.cc:24: // map of STHs where there's only one STH for each log. Documentation isn't correct because this isn't a map https://codereview.chromium.org/1968053002/diff/20001/net/cert/sth_distributo... net/cert/sth_distributor.cc:44: STHLogIDComparator(sth.log_id)); auto it = std::find_if(observed_sths_.begin(), observed_sths_.end(), [&sth](const SignedTreeHead& other) { return sth.log_id == other.log_id }); https://codereview.chromium.org/1968053002/diff/20001/net/cert/sth_distributo... net/cert/sth_distributor.cc:55: } Why do you do this for loop? It seems entirely redundant with 46-49? Also, I feel like I've pointed out a few times before, but please read https://chromium-cpp.appspot.com/ on Range-based for loops, and see the rule of thumb. https://codereview.chromium.org/1968053002/diff/20001/net/cert/sth_distributor.h File net/cert/sth_distributor.h (right): https://codereview.chromium.org/1968053002/diff/20001/net/cert/sth_distributo... net/cert/sth_distributor.h:32: // STHReporter implementation [NOT FOR THIS CL] I'm not sure how or why I missed this when you originally landed it, but now this whole interface seems weird. That is, STHReporter's interface fundamentally suggests that it handles notifying all STHObservers. Of course, STHReporter isn't documented, so it's not clear if or why this is *not* the case (e.g. can there only be one RegisterObserver call?). Put differently, if the contract is "STHReporter says there can only be one RegisterObserver call", then this is NOT an STHReporter. If "STHReporter says there can be many RegisterObserver calls", then *this* class (the STHDistributor) seems redundant. Further, given that this class is-a STHObserver, the whole abstraction thing seems overwrought - whomever is taking an STHObserver wired up to the STHDistributor here, aren't they an STHReporter? If not, what value is STHReporter serving? My concern here is that we have unnecessary interfaces that are non-obvious. I apologize for letting this design land, I think I was a bit exhausted after the original review, and I think we need to think carefully about this class hierarchy, because it already seems like we have unnecessary abstractions here. https://codereview.chromium.org/1968053002/diff/20001/net/cert/sth_distributo... net/cert/sth_distributor.h:44: std::vector<SignedTreeHead> observed_sths_; Why doesn't this need to be included or forward declared? You fixed the vector issue, but now it seems like you're relying on SignedTreeHead to be carried over from the STHObserver, and that's not necessarily true. That is, previously, the only use of SignedTreeHead was as part of implementing the STHObserver interface; now, you've got more usages of it (outside of the method signature), and you should either IWYU or forward declare, as appropriate.
https://codereview.chromium.org/1968053002/diff/20001/net/cert/sth_distributo... File net/cert/sth_distributor.cc (right): https://codereview.chromium.org/1968053002/diff/20001/net/cert/sth_distributo... net/cert/sth_distributor.cc:24: // map of STHs where there's only one STH for each log. On 2016/05/13 15:41:12, Ryan Sleevi wrote: > Documentation isn't correct because this isn't a map Removed the struct so the documentation is no longer there. https://codereview.chromium.org/1968053002/diff/20001/net/cert/sth_distributo... net/cert/sth_distributor.cc:44: STHLogIDComparator(sth.log_id)); On 2016/05/13 15:41:12, Ryan Sleevi wrote: > auto it = std::find_if(observed_sths_.begin(), observed_sths_.end(), > [&sth](const SignedTreeHead& other) { return sth.log_id == other.log_id }); Thanks, I wasn't aware lambdas were allowed in Chromium code. https://codereview.chromium.org/1968053002/diff/20001/net/cert/sth_distributo... net/cert/sth_distributor.cc:55: } On 2016/05/13 15:41:12, Ryan Sleevi wrote: > Why do you do this for loop? It seems entirely redundant with 46-49? > > Also, I feel like I've pointed out a few times before, but please read > https://chromium-cpp.appspot.com/ on Range-based for loops, and see the rule of > thumb. My bad - leftover code. Removed. You have pointed out chromium-cpp resource about Range-based for loops in the past. Removed. https://codereview.chromium.org/1968053002/diff/20001/net/cert/sth_distributor.h File net/cert/sth_distributor.h (right): https://codereview.chromium.org/1968053002/diff/20001/net/cert/sth_distributo... net/cert/sth_distributor.h:32: // STHReporter implementation On 2016/05/13 15:41:12, Ryan Sleevi wrote: > [NOT FOR THIS CL] > > I'm not sure how or why I missed this when you originally landed it, but now > this whole interface seems weird. > > That is, STHReporter's interface fundamentally suggests that it handles > notifying all STHObservers. Of course, STHReporter isn't documented, so it's not > clear if or why this is *not* the case (e.g. can there only be one > RegisterObserver call?). > > Put differently, if the contract is "STHReporter says there can only be one > RegisterObserver call", then this is NOT an STHReporter. If "STHReporter says > there can be many RegisterObserver calls", then *this* class (the > STHDistributor) seems redundant. > > Further, given that this class is-a STHObserver, the whole abstraction thing > seems overwrought - whomever is taking an STHObserver wired up to the > STHDistributor here, aren't they an STHReporter? If not, what value is > STHReporter serving? > > My concern here is that we have unnecessary interfaces that are non-obvious. I > apologize for letting this design land, I think I was a bit exhausted after the > original review, and I think we need to think carefully about this class > hierarchy, because it already seems like we have unnecessary abstractions here. Acknowledged - it seems possible to remove the STHReporter interface. The original reason for the creation of the STHReporter interface is role separation, so it's clear that no code which deals with STHObserver registration/unregirstration can publish new STHs to the STHDistributor and the code that publishes new STHs can only do that, not manipulate the STHObservers registered. If that is not a valid approach in Chromium I'll remove the STHReporter interface and leave only the STHDistributor. https://codereview.chromium.org/1968053002/diff/20001/net/cert/sth_distributo... net/cert/sth_distributor.h:44: std::vector<SignedTreeHead> observed_sths_; On 2016/05/13 15:41:12, Ryan Sleevi wrote: > Why doesn't this need to be included or forward declared? > > You fixed the vector issue, but now it seems like you're relying on > SignedTreeHead to be carried over from the STHObserver, and that's not > necessarily true. > > That is, previously, the only use of SignedTreeHead was as part of implementing > the STHObserver interface; now, you've got more usages of it (outside of the > method signature), and you should either IWYU or forward declare, as > appropriate. Done - as you point out I relied on the STHObserver include defining SignedTreeHead, so I've included signed_tree_head.h.
https://codereview.chromium.org/1968053002/diff/40001/net/cert/sth_distributo... File net/cert/sth_distributor.cc (right): https://codereview.chromium.org/1968053002/diff/40001/net/cert/sth_distributo... net/cert/sth_distributor.cc:52: observer->NewSTHObserved(sth); DESIGN/DANGER/SAFETY: It seems possible that |observer->NewSTHObserved()| may end up calling STHDistributor::NewSTHObserved, which would invalidate the iterator being used here. Nothing about the API contract prevents this, especially if end its up propagating through a number of dependent classes. // Make a local copy, because notifying the |observer| of a // new STH may result in this class being notified of a // (different) new STH, thus invalidating the iterator. std::vector<SignedTreeHeads> local_sths(observed_sths_); for (const auto& sth : local_sths) observer->NewSTHObserved(sth); That's because the is-a contract doesn't provide any guarantees against this reentrancy. If you wanted to violate the is-a rule and add that condition, then I'd suggest something like base::AutoReset auto_reset(®istering_observer_, true); for (const auto& sth : observed_sths_) observer->NewSTHObserved(sth); And in NewSTHObserved: DCHECK(!registering_observer_); However, imposing that constraint (on when RegisterObserver can be called) seems to violate the is-a rule, which is why I'm much more keen to explicitly handle it. But that's more memory intensive, so *shrug* https://codereview.chromium.org/1968053002/diff/40001/net/cert/sth_distributor.h File net/cert/sth_distributor.h (right): https://codereview.chromium.org/1968053002/diff/40001/net/cert/sth_distributo... net/cert/sth_distributor.h:45: std::vector<SignedTreeHead> observed_sths_; FWIW you can still forward declare this (because the ctor/dtor is out of line) https://codereview.chromium.org/1968053002/diff/40001/net/cert/sth_distributo... File net/cert/sth_distributor_unittest.cc (right): https://codereview.chromium.org/1968053002/diff/40001/net/cert/sth_distributo... net/cert/sth_distributor_unittest.cc:23: class StoringSTHObserver : public net::ct::STHObserver { These helper classes should be in an unnamed namespace to avoid ODR violations (Tests don't, but helpers should be) https://codereview.chromium.org/1968053002/diff/40001/net/cert/sth_distributo... net/cert/sth_distributor_unittest.cc:29: std::map<std::string, net::ct::SignedTreeHead> sths; You're in net::ct:: - remove all the net::ct:: here https://codereview.chromium.org/1968053002/diff/40001/net/cert/sth_distributo... net/cert/sth_distributor_unittest.cc:71: "\x98\x04\xf9\x1b\xdf\xb8\xe3\x77\xcd\x0e\xc8\x0d\xdc\x10"; uint8_t for the new code?
Thanks, PTAL. https://codereview.chromium.org/1968053002/diff/40001/net/cert/sth_distributo... File net/cert/sth_distributor.cc (right): https://codereview.chromium.org/1968053002/diff/40001/net/cert/sth_distributo... net/cert/sth_distributor.cc:52: observer->NewSTHObserved(sth); On 2016/05/16 19:19:01, Ryan Sleevi wrote: > DESIGN/DANGER/SAFETY: It seems possible that |observer->NewSTHObserved()| may > end up calling STHDistributor::NewSTHObserved, which would invalidate the > iterator being used here. > > Nothing about the API contract prevents this, especially if end its up > propagating through a number of dependent classes. > > // Make a local copy, because notifying the |observer| of a > // new STH may result in this class being notified of a > // (different) new STH, thus invalidating the iterator. > std::vector<SignedTreeHeads> local_sths(observed_sths_); > for (const auto& sth : local_sths) > observer->NewSTHObserved(sth); > > That's because the is-a contract doesn't provide any guarantees against this > reentrancy. If you wanted to violate the is-a rule and add that condition, then > I'd suggest something like > > base::AutoReset auto_reset(®istering_observer_, true); > for (const auto& sth : observed_sths_) > observer->NewSTHObserved(sth); > > > And in NewSTHObserved: > DCHECK(!registering_observer_); > > > However, imposing that constraint (on when RegisterObserver can be called) seems > to violate the is-a rule, which is why I'm much more keen to explicitly handle > it. But that's more memory intensive, so *shrug* Good point - switched to your first suggestion of making a local copy. I'd like to point out that since observer registration is a rare event (start-up / new profile creation) memory consumption during the process may not be such a major concern. https://codereview.chromium.org/1968053002/diff/40001/net/cert/sth_distributor.h File net/cert/sth_distributor.h (right): https://codereview.chromium.org/1968053002/diff/40001/net/cert/sth_distributo... net/cert/sth_distributor.h:45: std::vector<SignedTreeHead> observed_sths_; On 2016/05/16 19:19:01, Ryan Sleevi wrote: > FWIW you can still forward declare this (because the ctor/dtor is out of line) Done. https://codereview.chromium.org/1968053002/diff/40001/net/cert/sth_distributo... File net/cert/sth_distributor_unittest.cc (right): https://codereview.chromium.org/1968053002/diff/40001/net/cert/sth_distributo... net/cert/sth_distributor_unittest.cc:23: class StoringSTHObserver : public net::ct::STHObserver { On 2016/05/16 19:19:02, Ryan Sleevi wrote: > These helper classes should be in an unnamed namespace to avoid ODR violations > (Tests don't, but helpers should be) Done. As you referred to multiple helper classes, I assume you refer to STHDistributorTest as well - I've put everything in an unnamed namespace, as that seems what some of the other tests are doing. https://codereview.chromium.org/1968053002/diff/40001/net/cert/sth_distributo... net/cert/sth_distributor_unittest.cc:29: std::map<std::string, net::ct::SignedTreeHead> sths; On 2016/05/16 19:19:02, Ryan Sleevi wrote: > You're in net::ct:: - remove all the net::ct:: here Done. https://codereview.chromium.org/1968053002/diff/40001/net/cert/sth_distributo... net/cert/sth_distributor_unittest.cc:71: "\x98\x04\xf9\x1b\xdf\xb8\xe3\x77\xcd\x0e\xc8\x0d\xdc\x10"; On 2016/05/16 19:19:01, Ryan Sleevi wrote: > uint8_t for the new code? Done.
https://codereview.chromium.org/1968053002/diff/40001/net/cert/sth_distributo... File net/cert/sth_distributor_unittest.cc (right): https://codereview.chromium.org/1968053002/diff/40001/net/cert/sth_distributo... net/cert/sth_distributor_unittest.cc:23: class StoringSTHObserver : public net::ct::STHObserver { On 2016/05/17 09:51:03, Eran Messeri wrote: > On 2016/05/16 19:19:02, Ryan Sleevi wrote: > > These helper classes should be in an unnamed namespace to avoid ODR violations > > (Tests don't, but helpers should be) > > Done. As you referred to multiple helper classes, I assume you refer to > STHDistributorTest as well - I've put everything in an unnamed namespace, as > that seems what some of the other tests are doing. Right, both this and the STHDistributorTest. The individual TEST_F (line 46 on) don't need to be. https://codereview.chromium.org/1968053002/diff/60001/net/cert/sth_distributor.h File net/cert/sth_distributor.h (right): https://codereview.chromium.org/1968053002/diff/60001/net/cert/sth_distributo... net/cert/sth_distributor.h:37: // per log). Grammar: Registers |observer| for new STH notifications. On registration, the |observer| will be notified of the latest STH for each log that the STHDistributor has observed. https://codereview.chromium.org/1968053002/diff/60001/net/cert/sth_distributo... net/cert/sth_distributor.h:41: // that were previously registered via RegisterObserver; s/ / / (single space) https://codereview.chromium.org/1968053002/diff/60001/net/cert/sth_distributo... net/cert/sth_distributor.h:41: // that were previously registered via RegisterObserver; Style: // Unregisters |observer|, which must have been previously registered via RegisterObserver(). https://codereview.chromium.org/1968053002/diff/60001/net/cert/sth_distributo... File net/cert/sth_distributor_unittest.cc (right): https://codereview.chromium.org/1968053002/diff/60001/net/cert/sth_distributo... net/cert/sth_distributor_unittest.cc:84: sample_sth_.tree_size = 23u; DESIGN: You should set a value for .tree_size before this test, otherwise this test is tightly coupled to GetSampleSignedTreeHead, and I don't see any API contract that it's setting tree_size e.g. sample_sth.tree_size = 22u; NewSTHObserved(...) // Increase the tree size, but not the timestamp, and ensure that the newer STH is processed. sample_sth.tree_size = 23u; NewSTHObserved(...) Seems you should also write separate tests for the other conditions - that is, you have [same timestamp+new tree size], [newer timestamp+same tree size], [older timestamp + same tree size], [older timestamp + larger tree size - e.g. the log is naughty], [older timestamp + smaller tree size] This suggests that it may be useful to factor into a test that covers a set of test data permutations - the old/new treesize and the old/new timestamp - and goes through each to make sure that the appropriate thing happens. Alternatively, if this logic is not part of the STHDistributor, then making sure that in all cases it just takes the 'latest' (last observed) STH is also fine. But as it stands, changing the tree size and doing the test is unclear about the value, and seems to imply a semantic relationship when there is none - either we test that relationship, or we make it explicit that the relationship is irrelevant (by doing the 'worst thing possible' - that violates all the preconditions - and seeing that it still updates to that STH)
Addressed all comments - PTAL. https://codereview.chromium.org/1968053002/diff/60001/net/cert/sth_distributor.h File net/cert/sth_distributor.h (right): https://codereview.chromium.org/1968053002/diff/60001/net/cert/sth_distributo... net/cert/sth_distributor.h:37: // per log). On 2016/05/17 16:52:51, Ryan Sleevi wrote: > Grammar: > Registers |observer| for new STH notifications. On registration, the |observer| > will be notified of the > latest STH for each log that the STHDistributor has observed. Done. https://codereview.chromium.org/1968053002/diff/60001/net/cert/sth_distributo... net/cert/sth_distributor.h:41: // that were previously registered via RegisterObserver; On 2016/05/17 16:52:51, Ryan Sleevi wrote: > Style: > // Unregisters |observer|, which must have been previously registered via > RegisterObserver(). Done. https://codereview.chromium.org/1968053002/diff/60001/net/cert/sth_distributo... net/cert/sth_distributor.h:41: // that were previously registered via RegisterObserver; On 2016/05/17 16:52:51, Ryan Sleevi wrote: > s/ / / (single space) Done. https://codereview.chromium.org/1968053002/diff/60001/net/cert/sth_distributo... File net/cert/sth_distributor_unittest.cc (right): https://codereview.chromium.org/1968053002/diff/60001/net/cert/sth_distributo... net/cert/sth_distributor_unittest.cc:84: sample_sth_.tree_size = 23u; On 2016/05/17 16:52:51, Ryan Sleevi wrote: > DESIGN: You should set a value for .tree_size before this test, otherwise this > test is tightly coupled to GetSampleSignedTreeHead, and I don't see any API > contract that it's setting tree_size > > e.g. > > sample_sth.tree_size = 22u; > NewSTHObserved(...) > // Increase the tree size, but not the timestamp, and ensure that the newer STH > is processed. > sample_sth.tree_size = 23u; > NewSTHObserved(...) > > Seems you should also write separate tests for the other conditions - that is, > you have [same timestamp+new tree size], [newer timestamp+same tree size], > [older timestamp + same tree size], [older timestamp + larger tree size - e.g. > the log is naughty], [older timestamp + smaller tree size] > > This suggests that it may be useful to factor into a test that covers a set of > test data permutations - the old/new treesize and the old/new timestamp - and > goes through each to make sure that the appropriate thing happens. > > > Alternatively, if this logic is not part of the STHDistributor, then making sure > that in all cases it just takes the 'latest' (last observed) STH is also fine. > But as it stands, changing the tree size and doing the test is unclear about the > value, and seems to imply a semantic relationship when there is none - either we > test that relationship, or we make it explicit that the relationship is > irrelevant (by doing the 'worst thing possible' - that violates all the > preconditions - and seeing that it still updates to that STH) What I've done is stored the value of sample_sth_.tree_size, increased it by one after calling NewSTHObserved once and checked (in EXPECT_EQ) that the tree size of the observed STH is the original sample_sth_.tree_size + 1. As the STHDistributor does not have any logic for dropping updates of smaller / older STHs there's no benefit in testing it here.
https://codereview.chromium.org/1968053002/diff/80001/net/cert/sth_distributo... File net/cert/sth_distributor_unittest.cc (right): https://codereview.chromium.org/1968053002/diff/80001/net/cert/sth_distributo... net/cert/sth_distributor_unittest.cc:48: TEST_F(STHDistributorTest, NotifiesOfExistingSTHs) { Test style - I haven't pushed as hard on this with the CTVerifier tests, but I regret it, because it actually was rather confusing. I'm much more inclined to focus on making sure there's clear documentation in the test, beyond just the name, that tries to capture the *why*, and not just the *what* (that the name captures) // Test that registering a new observer, after some set of STHs have been observed, will notify the newly // added observer of the most recent STHs. TEST_F(...) https://codereview.chromium.org/1968053002/diff/80001/net/cert/sth_distributo... net/cert/sth_distributor_unittest.cc:65: TEST_F(STHDistributorTest, LogsUMAForPilotSTH) { // Test that histograms are properly recorded for [blah blah] of the Google Pilot Log, as they're // used for [blah blah]. https://codereview.chromium.org/1968053002/diff/80001/net/cert/sth_distributo... net/cert/sth_distributor_unittest.cc:82: TEST_F(STHDistributorTest, UpdatesObservedSTHData) { Test that... blah https://codereview.chromium.org/1968053002/diff/80001/net/cert/sth_distributo... net/cert/sth_distributor_unittest.cc:91: EXPECT_EQ(orig_tree_size + 1, observer.sths[GetTestPublicKeyId()].tree_size); This is even less clear than what you originally had. Why store orig_tree_size? Why not just EXPECT_EQ(sample_sth_.tree_size, observer.sths[...].tree_size) There's also the concern that perhaps I didn't make clearer, which is that, as this function reads, it's "magic". It's not documented sufficiently to explain what you're testing, why you're changing the value, or how it accomplishes the result. It's definitely been something I've encouraged more of - clearer documentation not just in the test name, but how the test accomplishes its goal. Consider something like: // Observe an initial STH StoringSTHObserver observer; distributor_.RegisterObserver(&observer); distributor.NewSTHObserved(sample_sth_); EXPECT_EQ(1u, observer.sths.size()); EXPECT_EQ(sample_sth_, observer.sths[GetTestPublicKeyId()]); // Observe a new STH. "new" simply means that it is a more recently observed SignedTreeHead // for the given log ID, not necessarily that it's newer chronologically (the timestamp) or the log // state (the tree size). // To make sure the more recently observed SignedTreeHead is returned, just modify some fields. SignedTreeHead new_sth = sample_sth; new_sth.tree_size++; new_sth.timestamp -= base::TimeDelta::FromSeconds(3); distributor_.NewSTHObserved(new_sth); // The STH should have been broadcast to existing observers. EXPECT_EQ(1u, observer.sths.size()); EXPECT_NE(sample_sth_, observer.sths[GetTestPublicKeyId()]); EXPECT_EQ(new_sth, observer.sths[GetTestPublicKeyId()]); // Registering a new observer should only receive the most recently observed STH StoringSTHObserver new_observer; distributor_.RegisterObserver(&new_observer); EXPECT_EQ(1u, new_observer.sths.size()); EXPECT_NE(sample_sth_, new_observer.sths[GetTestPublicKeyId()]); EXPECT_EQ(new_sth, new_observer.sths[GetTestPublicKeyId()]); This tries to make it clear what the conditions you are testing. It tries to actually test the condition - don't just test the tree_size, make sure that no other fields were mutated. It tries to cover both scenarios relevant for testing here. Does this make it clearer? I probably have bugs in the above, but it tries to document clearly what's going on.
Thanks for the quick follow-up, PTAL. https://codereview.chromium.org/1968053002/diff/80001/net/cert/sth_distributo... File net/cert/sth_distributor_unittest.cc (right): https://codereview.chromium.org/1968053002/diff/80001/net/cert/sth_distributo... net/cert/sth_distributor_unittest.cc:48: TEST_F(STHDistributorTest, NotifiesOfExistingSTHs) { On 2016/05/17 17:57:17, Ryan Sleevi wrote: > Test style - I haven't pushed as hard on this with the CTVerifier tests, but I > regret it, because it actually was rather confusing. > > I'm much more inclined to focus on making sure there's clear documentation in > the test, beyond just the name, that tries to capture the *why*, and not just > the *what* (that the name captures) > > // Test that registering a new observer, after some set of STHs have been > observed, will notify the newly > // added observer of the most recent STHs. > TEST_F(...) Done. https://codereview.chromium.org/1968053002/diff/80001/net/cert/sth_distributo... net/cert/sth_distributor_unittest.cc:65: TEST_F(STHDistributorTest, LogsUMAForPilotSTH) { On 2016/05/17 17:57:17, Ryan Sleevi wrote: > // Test that histograms are properly recorded for [blah blah] of the Google > Pilot Log, as they're > // used for [blah blah]. Done. https://codereview.chromium.org/1968053002/diff/80001/net/cert/sth_distributo... net/cert/sth_distributor_unittest.cc:82: TEST_F(STHDistributorTest, UpdatesObservedSTHData) { On 2016/05/17 17:57:17, Ryan Sleevi wrote: > Test that... blah Done. https://codereview.chromium.org/1968053002/diff/80001/net/cert/sth_distributo... net/cert/sth_distributor_unittest.cc:91: EXPECT_EQ(orig_tree_size + 1, observer.sths[GetTestPublicKeyId()].tree_size); On 2016/05/17 17:57:17, Ryan Sleevi wrote: > This is even less clear than what you originally had. Why store orig_tree_size? > Why not just > > EXPECT_EQ(sample_sth_.tree_size, observer.sths[...].tree_size) > > There's also the concern that perhaps I didn't make clearer, which is that, as > this function reads, it's "magic". It's not documented sufficiently to explain > what you're testing, why you're changing the value, or how it accomplishes the > result. > > It's definitely been something I've encouraged more of - clearer documentation > not just in the test name, but how the test accomplishes its goal. > > Consider something like: > > // Observe an initial STH > StoringSTHObserver observer; > distributor_.RegisterObserver(&observer); > > distributor.NewSTHObserved(sample_sth_); > > EXPECT_EQ(1u, observer.sths.size()); > EXPECT_EQ(sample_sth_, observer.sths[GetTestPublicKeyId()]); > > // Observe a new STH. "new" simply means that it is a more recently observed > SignedTreeHead > // for the given log ID, not necessarily that it's newer chronologically (the > timestamp) or the log > // state (the tree size). > // To make sure the more recently observed SignedTreeHead is returned, just > modify some fields. > SignedTreeHead new_sth = sample_sth; > new_sth.tree_size++; > new_sth.timestamp -= base::TimeDelta::FromSeconds(3); > > distributor_.NewSTHObserved(new_sth); > > // The STH should have been broadcast to existing observers. > EXPECT_EQ(1u, observer.sths.size()); > EXPECT_NE(sample_sth_, observer.sths[GetTestPublicKeyId()]); > EXPECT_EQ(new_sth, observer.sths[GetTestPublicKeyId()]); > > // Registering a new observer should only receive the most recently observed STH > StoringSTHObserver new_observer; > distributor_.RegisterObserver(&new_observer); > EXPECT_EQ(1u, new_observer.sths.size()); > EXPECT_NE(sample_sth_, new_observer.sths[GetTestPublicKeyId()]); > EXPECT_EQ(new_sth, new_observer.sths[GetTestPublicKeyId()]); > > > > This tries to make it clear what the conditions you are testing. It tries to > actually test the condition - don't just test the tree_size, make sure that no > other fields were mutated. It tries to cover both scenarios relevant for testing > here. Does this make it clearer? I probably have bugs in the above, but it tries > to document clearly what's going on. Done - adapted your suggestion, thanks for providing such a clear test case. Note I had to implement operator== and operator!= for SignedTreeHead for this to work.
LGTM % nits https://codereview.chromium.org/1968053002/diff/100001/net/cert/signed_tree_h... File net/cert/signed_tree_head.cc (right): https://codereview.chromium.org/1968053002/diff/100001/net/cert/signed_tree_h... net/cert/signed_tree_head.cc:50: lhs.tree_size == rhs.tree_size && lhs.log_id == rhs.log_id && fwiw, you could use return std::tie(lhs.version, lhs.timestamp, lhs.tree_size, lhs.log_id) == std::tie(rhs.version, rhs.timestamp, rhs.tree_size, rhs.log_id) && memcmp(...) == 0 && lhs.signature.SignatureParamsMatch(...) && lhs.signature.signature_data == ... If you opt-not to (fine), or do, drop the extra parens around the memcmp and lhs.signature.SignatureParametersMatch https://codereview.chromium.org/1968053002/diff/100001/net/cert/signed_tree_h... net/cert/signed_tree_head.cc:60: return !operator==(lhs, rhs); !(lhs == rhs) No need to directly invoke operator== here (we only do that for the member-methods, which are discouraged by https://google.github.io/styleguide/cppguide.html#Operator_Overloading anyways) https://codereview.chromium.org/1968053002/diff/100001/net/cert/sth_distribut... File net/cert/sth_distributor_unittest.cc (right): https://codereview.chromium.org/1968053002/diff/100001/net/cert/sth_distribut... net/cert/sth_distributor_unittest.cc:121: // STH grammar: STH. (end with a period/full-stop)
Thanks, applied all your suggestions, submitting now. https://codereview.chromium.org/1968053002/diff/100001/net/cert/signed_tree_h... File net/cert/signed_tree_head.cc (right): https://codereview.chromium.org/1968053002/diff/100001/net/cert/signed_tree_h... net/cert/signed_tree_head.cc:50: lhs.tree_size == rhs.tree_size && lhs.log_id == rhs.log_id && On 2016/05/17 22:20:16, Ryan Sleevi wrote: > fwiw, you could use > > return std::tie(lhs.version, lhs.timestamp, lhs.tree_size, lhs.log_id) == > std::tie(rhs.version, rhs.timestamp, rhs.tree_size, rhs.log_id) && > memcmp(...) == 0 && > lhs.signature.SignatureParamsMatch(...) && > lhs.signature.signature_data == ... > > If you opt-not to (fine), or do, drop the extra parens around the memcmp and > lhs.signature.SignatureParametersMatch Done - switched to using std::tie and removed the extra parens. https://codereview.chromium.org/1968053002/diff/100001/net/cert/signed_tree_h... net/cert/signed_tree_head.cc:60: return !operator==(lhs, rhs); On 2016/05/17 22:20:16, Ryan Sleevi wrote: > !(lhs == rhs) > > No need to directly invoke operator== here (we only do that for the > member-methods, which are discouraged by > https://google.github.io/styleguide/cppguide.html#Operator_Overloading anyways) Done. https://codereview.chromium.org/1968053002/diff/100001/net/cert/sth_distribut... File net/cert/sth_distributor_unittest.cc (right): https://codereview.chromium.org/1968053002/diff/100001/net/cert/sth_distribut... net/cert/sth_distributor_unittest.cc:121: // STH On 2016/05/17 22:20:16, Ryan Sleevi wrote: > grammar: STH. (end with a period/full-stop) Done.
The CQ bit was checked by eranm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/1968053002/#ps120001 (title: "Final operator, test changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1968053002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1968053002/120001
Message was sent while issue was closed.
Description was changed from ========== Certificate Transparency: Notify STH Observers of known STHs When new STHObservers are registered, provide them with an STH from each log. BUG=611021 ========== to ========== Certificate Transparency: Notify STH Observers of known STHs When new STHObservers are registered, provide them with an STH from each log. BUG=611021 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Certificate Transparency: Notify STH Observers of known STHs When new STHObservers are registered, provide them with an STH from each log. BUG=611021 ========== to ========== Certificate Transparency: Notify STH Observers of known STHs When new STHObservers are registered, provide them with an STH from each log. BUG=611021 Committed: https://crrev.com/e2c861428cf7096b1d5317f4c033d55180618d2a Cr-Commit-Position: refs/heads/master@{#394385} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/e2c861428cf7096b1d5317f4c033d55180618d2a Cr-Commit-Position: refs/heads/master@{#394385} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
