Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2017563002:
Add Certificate Transparency logs auditing (Closed)
|
Created:
4 years, 7 months ago by Eran Messeri Modified:
3 years, 11 months ago CC:
chromium-reviews, certificate-transparency-chrome_googlegroups.com, cbentzel+watch_chromium.org, Eran Messeri Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAudit CT logs by requesting inclusion proofs for
observed Signed Certificate Timestamps from the
CT log that issued them.
To verify that a CT log behaves correctly and indeed
publishes all the certificates it committed to publishing,
it is necessary to verify that each observed log entry
(as denoted by an SCT and the corresponding
certificate) is included in the log.
Check for inclusion of observed SCTs by finding out
the leaf index of each observed SCT and requesting
an inclusion proof from the CT log for it over DNS
(using the LogDnsClient).
Note that no action is taken based on the inclusion
check results in this change. Follow-up changes
will add telemetry.
BUG=613495
Review-Url: https://codereview.chromium.org/2017563002
Cr-Commit-Position: refs/heads/master@{#445513}
Committed: https://chromium.googlesource.com/chromium/src/+/1b5a833bca72a9f89e8c6e8c592037540eaf1411
Patch Set 1 #
Total comments: 12
Patch Set 2 : Switching to TimestampedLeaf #
Total comments: 10
Patch Set 3 : Addressing all comments #
Total comments: 46
Patch Set 4 : ObservedLeaf -> LeafState #Patch Set 5 : Marking comparator const #
Total comments: 2
Patch Set 6 : Getting rid of the LeafState, MerkleTreeLeaf usage #
Total comments: 16
Patch Set 7 : Significant re-work to separate entry & STT states #Patch Set 8 : Fully-implemented SingleTreeTracker #Patch Set 9 : Simplified STT with throttling, memory pressure handling #
Total comments: 42
Patch Set 10 : Addressing Ryan's comments #Patch Set 11 : Completing implementation of tests #Patch Set 12 : Fixing compilation issues #Patch Set 13 : Merging with master #Patch Set 14 : Ready for review #
Total comments: 45
Patch Set 15 : Addressed review comments #Patch Set 16 : Removed unused function in the test. #
Total comments: 2
Patch Set 17 : Addressed in-person review comments #Patch Set 18 : (Re-upload) #
Total comments: 64
Patch Set 19 : Addressed review comments #
Total comments: 34
Patch Set 20 : Simplified test. #
Total comments: 8
Patch Set 21 : Review comments & one less TODO #
Total comments: 23
Patch Set 22 : Improving documentation #Patch Set 23 : TODO for hooking up NetLOg #Patch Set 24 : Fix Windows compilation issue #
Total comments: 10
Patch Set 25 : Remove templated function from tests #
Total comments: 4
Patch Set 26 : Addressing Ryan's two final comments #Messages
Total messages: 99 (64 generated)
Description was changed from ========== Certificate Transparency: Use MerkleTreeLeaf Another step towards inclusion checking: Construct MerkleTreeLeaf instances in the SingleTreeTracker, so their inclusion status could be checked and they could be hashed for verifying inclusion proofs once those are fetched. BUG=613495 ========== to ========== Certificate Transparency: Use MerkleTreeLeaf Another step towards inclusion checking: Construct MerkleTreeLeaf instances in the SingleTreeTracker, so their inclusion status could be checked and they could be hashed for verifying inclusion proofs once those are fetched. BUG=613495 ==========
eranm@chromium.org changed reviewers: + robpercival@chromium.org
https://codereview.chromium.org/2017563002/diff/1/components/certificate_tran... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/1/components/certificate_tran... components/certificate_transparency/single_tree_tracker.cc:43: rhs_entry.issuer_key_hash); ".operator()" is redundant here, isn't it? https://codereview.chromium.org/2017563002/diff/1/components/certificate_tran... components/certificate_transparency/single_tree_tracker.cc:78: pending_inclusion_check_.insert(std::move(leaf)); Can you move a MerkleTreeLeaf? You've declared a copy constructor in this CL, which should suppress generation of a default move constructor. https://codereview.chromium.org/2017563002/diff/1/components/certificate_tran... components/certificate_transparency/single_tree_tracker.cc:139: TODO here for looking up result of inclusion check. https://codereview.chromium.org/2017563002/diff/1/components/certificate_tran... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/2017563002/diff/1/components/certificate_tran... components/certificate_transparency/single_tree_tracker.h:117: std::set<net::ct::MerkleTreeLeaf, OrderByTimestamp> pending_new_sth_; Are you thinking of adding a timestamp to MerkleTreeLeaf for recording the time of observation (or something like that)? Otherwise, a set might be a bad choice. https://codereview.chromium.org/2017563002/diff/1/net/cert/merkle_tree_leaf.cc File net/cert/merkle_tree_leaf.cc (right): https://codereview.chromium.org/2017563002/diff/1/net/cert/merkle_tree_leaf.c... net/cert/merkle_tree_leaf.cc:60: const MerkleTreeLeaf& rhs); I assume this shouldn't be here? https://codereview.chromium.org/2017563002/diff/1/net/cert/merkle_tree_leaf.h File net/cert/merkle_tree_leaf.h (right): https://codereview.chromium.org/2017563002/diff/1/net/cert/merkle_tree_leaf.h... net/cert/merkle_tree_leaf.h:26: MerkleTreeLeaf(const MerkleTreeLeaf& other); Why define a copy constructor?
https://chromiumcodereview-hr.appspot.com/2017563002/diff/20001/components/ce... File components/certificate_transparency/single_tree_tracker.cc (right): https://chromiumcodereview-hr.appspot.com/2017563002/diff/20001/components/ce... components/certificate_transparency/single_tree_tracker.cc:73: (sct->timestamp + base::TimeDelta::FromHours(24)))) { Extract "base::TimeDelta::FromHours(24)" to a constant named something like kMaximumMergeDelay. If I were pedantic, I'd say this should be a property of CtLog, but I doubt there will ever be a log that doesn't have an MMD of 24 hours so that's unnecessary. https://chromiumcodereview-hr.appspot.com/2017563002/diff/20001/components/ce... File components/certificate_transparency/timestamped_leaf.h (right): https://chromiumcodereview-hr.appspot.com/2017563002/diff/20001/components/ce... components/certificate_transparency/timestamped_leaf.h:10: #include "net/cert/merkle_tree_leaf.h" #include <stdint.h> // for uint64_t https://chromiumcodereview-hr.appspot.com/2017563002/diff/20001/components/ce... components/certificate_transparency/timestamped_leaf.h:12: class TimestampedLeaf { It might be better to have this inherit from MerkleTreeLeaf (it looks like an "is-a" relationship). https://chromiumcodereview-hr.appspot.com/2017563002/diff/20001/components/ce... components/certificate_transparency/timestamped_leaf.h:21: uint64_t GetLeafIndex(); HasLeafIndex() and GetLeafIndex() should be const. https://chromiumcodereview-hr.appspot.com/2017563002/diff/20001/components/ce... components/certificate_transparency/timestamped_leaf.h:22: net::ct::MerkleTreeLeaf GetLeaf(); What's the point in having a non-const version of GetLeaf that returns by value?
eranm@chromium.org changed reviewers: + rsleevi@chromium.org
Ryan - please take a look, I believe this is ready for you to review. https://codereview.chromium.org/2017563002/diff/1/components/certificate_tran... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/1/components/certificate_tran... components/certificate_transparency/single_tree_tracker.cc:43: rhs_entry.issuer_key_hash); On 2016/05/26 17:33:15, Rob Percival wrote: > ".operator()" is redundant here, isn't it? Indeed. Done. https://codereview.chromium.org/2017563002/diff/1/components/certificate_tran... components/certificate_transparency/single_tree_tracker.cc:78: pending_inclusion_check_.insert(std::move(leaf)); On 2016/05/26 17:33:15, Rob Percival wrote: > Can you move a MerkleTreeLeaf? You've declared a copy constructor in this CL, > which should suppress generation of a default move constructor. I'd get a compilation error in that case, wouldn't I? If I explicitly delete the move c'tor on the type I move I definitely get an error. I could not find documentation on what happens when an implicitly-declared move c'tor is deleted. Either way, I've forced the creation of one. https://codereview.chromium.org/2017563002/diff/1/components/certificate_tran... components/certificate_transparency/single_tree_tracker.cc:139: On 2016/05/26 17:33:15, Rob Percival wrote: > TODO here for looking up result of inclusion check. Done. https://codereview.chromium.org/2017563002/diff/1/components/certificate_tran... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/2017563002/diff/1/components/certificate_tran... components/certificate_transparency/single_tree_tracker.h:117: std::set<net::ct::MerkleTreeLeaf, OrderByTimestamp> pending_new_sth_; On 2016/05/26 17:33:15, Rob Percival wrote: > Are you thinking of adding a timestamp to MerkleTreeLeaf for recording the time > of observation (or something like that)? Otherwise, a set might be a bad choice. I've added another class. https://codereview.chromium.org/2017563002/diff/1/net/cert/merkle_tree_leaf.cc File net/cert/merkle_tree_leaf.cc (right): https://codereview.chromium.org/2017563002/diff/1/net/cert/merkle_tree_leaf.c... net/cert/merkle_tree_leaf.cc:60: const MerkleTreeLeaf& rhs); On 2016/05/26 17:33:15, Rob Percival wrote: > I assume this shouldn't be here? Correct. https://codereview.chromium.org/2017563002/diff/1/net/cert/merkle_tree_leaf.h File net/cert/merkle_tree_leaf.h (right): https://codereview.chromium.org/2017563002/diff/1/net/cert/merkle_tree_leaf.h... net/cert/merkle_tree_leaf.h:26: MerkleTreeLeaf(const MerkleTreeLeaf& other); On 2016/05/26 17:33:15, Rob Percival wrote: > Why define a copy constructor? The compiler produces an error about "Complex class/struct needs an explicit out-of-line copy constructor.". That indicates the class is copied somewhere, but I didn't find exactly where (probably because it's used in STL containers and I'm using algorithms to move them around). More than happy to hear if you have a clue why it does that (note this now applies to the ObservedLeaf class I've added as well). https://codereview.chromium.org/2017563002/diff/20001/components/certificate_... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/20001/components/certificate_... components/certificate_transparency/single_tree_tracker.cc:73: (sct->timestamp + base::TimeDelta::FromHours(24)))) { On 2016/06/30 14:15:57, Rob Percival wrote: > Extract "base::TimeDelta::FromHours(24)" to a constant named something like > kMaximumMergeDelay. If I were pedantic, I'd say this should be a property of > CtLog, but I doubt there will ever be a log that doesn't have an MMD of 24 hours > so that's unnecessary. Done. The Chrome policy is not clear about whether there's a maximum MMD, but you are right that it is a property of the log and should probably be in CTLogVerifier. https://codereview.chromium.org/2017563002/diff/20001/components/certificate_... File components/certificate_transparency/timestamped_leaf.h (right): https://codereview.chromium.org/2017563002/diff/20001/components/certificate_... components/certificate_transparency/timestamped_leaf.h:10: #include "net/cert/merkle_tree_leaf.h" On 2016/06/30 14:15:58, Rob Percival wrote: > #include <stdint.h> // for uint64_t Done. https://codereview.chromium.org/2017563002/diff/20001/components/certificate_... components/certificate_transparency/timestamped_leaf.h:12: class TimestampedLeaf { On 2016/06/30 14:15:58, Rob Percival wrote: > It might be better to have this inherit from MerkleTreeLeaf (it looks like an > "is-a" relationship). I'd rather keep it a separate class: - Detangling inheritance in the codebase is usually painful. - I'd argue there's only one type of MerkleTreeLeaf - the one in the Merkle tree. This class contains metadata about a particular MerkleTreeLeaf. - Inheritance is more meaningful when there are behavioural changes - here we'd just be overloading it with data fields. https://codereview.chromium.org/2017563002/diff/20001/components/certificate_... components/certificate_transparency/timestamped_leaf.h:21: uint64_t GetLeafIndex(); On 2016/06/30 14:15:58, Rob Percival wrote: > HasLeafIndex() and GetLeafIndex() should be const. Done. https://codereview.chromium.org/2017563002/diff/20001/components/certificate_... components/certificate_transparency/timestamped_leaf.h:22: net::ct::MerkleTreeLeaf GetLeaf(); On 2016/06/30 14:15:58, Rob Percival wrote: > What's the point in having a non-const version of GetLeaf that returns by value? No point, removed.
Does ObservedLeaf really need to be a dedicated class? If so, where are the unittests? If not, why isn't it just an implementation detail of the TreeStateTracker? Why doesn't a tuple suffice or an appropriate data structure separate for those pending STHs and those pending inclusion checks? https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... File components/certificate_transparency/observed_leaf.cc (right): https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/observed_leaf.cc:24: CHECK(!leaf_index_); Then programming errors should be met with DCHECK, generally. https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... File components/certificate_transparency/observed_leaf.h (right): https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/observed_leaf.h:13: should be in the certificate_transparency namespace https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/observed_leaf.h:17: class ObservedLeaf { 1) Why do we need a dedicated class for this? 2) Why does this belong here vs //net? https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/observed_leaf.h:26: void SetLeafIndex(uint64_t index); Use vertical whitespace (between each and every one of these methods) https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/observed_leaf.h:31: // was called. line wrapping https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/observed_leaf.h:32: uint64_t GetLeafIndex() const; DESIGN: API design wise, "May only be called ..." is generally a big red flag. https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/single_tree_tracker.cc:20: bool OrderByTimestamp::operator()(const ObservedLeaf& lhs, Should have had a line break after the namespace https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/single_tree_tracker.cc:26: DCHECK(lhs_leaf.log_id == rhs_leaf.log_id); DCHECK_EQ https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/single_tree_tracker.cc:33: // LogEntries to find out. Grammatical comments about position tend to read weird. Be active // |lhs| and |rhs| are either the same leaf, or they're two entries that // share the same timestamp. That said, it's unclear why this comment is even needed. A less-than comparator has to assume that |lhs| and |rhs| are equal for some inputs, so it's not clear why it needs stating. You sort by timestamp. Then contents. Great. From a design perspective, why isn't the less-than check for LogEntry's a factor of the LogEntry? Why does it get moved here? For that matter, why have the comparator here - rather than on the ObservedLeaf itself? This generally only needs the separate-comparator if we're ignoring parts of the structure due to specific reasons. https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/single_tree_tracker.cc:114: // Find out which SCTs can now be checked for inclusion. Use vertical whitespace https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/single_tree_tracker.cc:117: }; Don't create a temporary variable for this one call. https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/single_tree_tracker.cc:121: // the set of entries pending inclusion check. Grammatically, this reads weird. "move all entries up to the first entry" can be parsed as "[move all entries up to] the first entry" or "Begin inclusion checks for all entries covered by this STH". I'm suspecting you mean the latter. https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/single_tree_tracker.cc:125: // Clear moved entries. Doesn't add value with the above rewrite https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/single_tree_tracker.h:33: }; Does not need to be global; should be a private class instance. https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/single_tree_tracker.h:105: // Returns true if |leaf| is pending a newer STH. From reading this header, it's unclear what this method does. https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/single_tree_tracker.h:108: // Returns true if |leaf| is pending inclusion check. Grammatically, this reads weird. It feels like an article is missing (pending an inclusion check? pending being checked for inclusion?) https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/single_tree_tracker.h:120: // Set of log entries pending inclusion check. grammatically, this reads weird. It feels like there is disagreement between the subject and verb (pending being checked for inclusion? pending inclusion checks?) https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... File components/certificate_transparency/single_tree_tracker_unittest.cc (right): https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/single_tree_tracker_unittest.cc:70: // solely on |chain_|. LAYERING: This comment strikes me as over-explaining something future / unrelated. Are you sure this comment belongs? https://codereview.chromium.org/2017563002/diff/40001/net/cert/merkle_tree_le... File net/cert/merkle_tree_leaf.h (right): https://codereview.chromium.org/2017563002/diff/40001/net/cert/merkle_tree_le... net/cert/merkle_tree_leaf.h:21: // Represents a MerkleTreeLeaf as defined in RFC6962, section 3.4. Unfortunately, finding an explanation about how this relates to the overall narrative is quite difficult. The documentation could have been improved here. https://codereview.chromium.org/2017563002/diff/40001/net/cert/merkle_tree_le... net/cert/merkle_tree_leaf.h:23: // slightly differently. Does RFC 6962-bis do something differently? It's unclear why we need this structure. https://codereview.chromium.org/2017563002/diff/40001/net/cert/merkle_tree_le... net/cert/merkle_tree_leaf.h:23: // slightly differently. Why is it arranged differently? https://codereview.chromium.org/2017563002/diff/40001/net/cert/merkle_tree_le... net/cert/merkle_tree_leaf.h:43: NET_EXPORT bool GetMerkleTreeLeaf(const X509Certificate* cert, Should have been documented. https://codereview.chromium.org/2017563002/diff/40001/net/cert/merkle_tree_le... net/cert/merkle_tree_leaf.h:48: // Returns true if the hash was generated, false if an error occurred. Same here - does 6962-bis change this? Naming wise, this should have been named something better HashMerkleTreeLeaf or something; reading net::ct::Hash(...) doesn't really give much insight.
Ryan, PTAL. I've changed the design based on your comments, now the leaf state is kept separate of the MerkleTreeLeaf itself, in a map<MerkleTreeLeaf, LeafState>. That simplifies figuring out the state of the tree and reasoning about the sort order of the container. https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... File components/certificate_transparency/observed_leaf.cc (right): https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/observed_leaf.cc:24: CHECK(!leaf_index_); On 2016/06/30 22:48:18, Ryan Sleevi wrote: > Then programming errors should be met with DCHECK, generally. Done. https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... File components/certificate_transparency/observed_leaf.h (right): https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/observed_leaf.h:13: On 2016/06/30 22:48:18, Ryan Sleevi wrote: > should be in the certificate_transparency namespace Moved to the SingleTreeTracker class. https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/observed_leaf.h:17: class ObservedLeaf { On 2016/06/30 22:48:18, Ryan Sleevi wrote: > 1) Why do we need a dedicated class for this? > 2) Why does this belong here vs //net? I've significantly changed this class and moved it into SingleTreeTracker: - It now only holds the state, without the leaf itself. - Since it's an implementation detail of the SingleTreeTracker it should not exist on its own. https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/observed_leaf.h:26: void SetLeafIndex(uint64_t index); On 2016/06/30 22:48:18, Ryan Sleevi wrote: > Use vertical whitespace (between each and every one of these methods) Done. https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/observed_leaf.h:31: // was called. On 2016/06/30 22:48:18, Ryan Sleevi wrote: > line wrapping Done. https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/observed_leaf.h:32: uint64_t GetLeafIndex() const; On 2016/06/30 22:48:18, Ryan Sleevi wrote: > DESIGN: API design wise, "May only be called ..." is generally a big red flag. Can you elaborate? Also, is it a significant issue now that it's a nested class in SingleTreeTracker? https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/single_tree_tracker.cc:20: bool OrderByTimestamp::operator()(const ObservedLeaf& lhs, On 2016/06/30 22:48:19, Ryan Sleevi wrote: > Should have had a line break after the namespace Done. https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/single_tree_tracker.cc:26: DCHECK(lhs_leaf.log_id == rhs_leaf.log_id); On 2016/06/30 22:48:19, Ryan Sleevi wrote: > DCHECK_EQ Done. https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/single_tree_tracker.cc:33: // LogEntries to find out. On 2016/06/30 22:48:18, Ryan Sleevi wrote: > Grammatical comments about position tend to read weird. Be active > > // |lhs| and |rhs| are either the same leaf, or they're two entries that > // share the same timestamp. > > > That said, it's unclear why this comment is even needed. A less-than comparator > has to assume that |lhs| and |rhs| are equal for some inputs, so it's not clear > why it needs stating. > > You sort by timestamp. Then contents. Great. > > From a design perspective, why isn't the less-than check for LogEntry's a factor > of the LogEntry? Why does it get moved here? > > For that matter, why have the comparator here - rather than on the ObservedLeaf > itself? > > This generally only needs the separate-comparator if we're ignoring parts of the > structure due to specific reasons. Removed the comment, since, as you say, it was not adding much. Since this now compares two MerkleTreeLeaf instances you could argue it should live there. I don't mind moving it, wondering if we should wait to see if it's useful for any other user of the MerkleTreeLeaf before moving. https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/single_tree_tracker.cc:114: // Find out which SCTs can now be checked for inclusion. On 2016/06/30 22:48:19, Ryan Sleevi wrote: > Use vertical whitespace Done. https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/single_tree_tracker.cc:117: }; On 2016/06/30 22:48:18, Ryan Sleevi wrote: > Don't create a temporary variable for this one call. Done. https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/single_tree_tracker.cc:121: // the set of entries pending inclusion check. On 2016/06/30 22:48:19, Ryan Sleevi wrote: > Grammatically, this reads weird. "move all entries up to the first entry" can be > parsed as "[move all entries up to] the first entry" or "Begin inclusion checks > for all entries covered by this STH". > > I'm suspecting you mean the latter. Correct, but that piece of code is gone. https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/single_tree_tracker.cc:125: // Clear moved entries. On 2016/06/30 22:48:19, Ryan Sleevi wrote: > Doesn't add value with the above rewrite Done. https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/single_tree_tracker.h:33: }; On 2016/06/30 22:48:19, Ryan Sleevi wrote: > Does not need to be global; should be a private class instance. Done. https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/single_tree_tracker.h:105: // Returns true if |leaf| is pending a newer STH. On 2016/06/30 22:48:19, Ryan Sleevi wrote: > From reading this header, it's unclear what this method does. This method is gone, replaced with LeafAlreadyEncountered. https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/single_tree_tracker.h:108: // Returns true if |leaf| is pending inclusion check. On 2016/06/30 22:48:19, Ryan Sleevi wrote: > Grammatically, this reads weird. It feels like an article is missing (pending an > inclusion check? pending being checked for inclusion?) This method is gone. https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/single_tree_tracker.h:120: // Set of log entries pending inclusion check. On 2016/06/30 22:48:19, Ryan Sleevi wrote: > grammatically, this reads weird. It feels like there is disagreement between the > subject and verb (pending being checked for inclusion? pending inclusion > checks?) I now use a map of (MerkleTreeLeaf, LeafState), so should be easier to reason about. https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... File components/certificate_transparency/single_tree_tracker_unittest.cc (right): https://codereview.chromium.org/2017563002/diff/40001/components/certificate_... components/certificate_transparency/single_tree_tracker_unittest.cc:70: // solely on |chain_|. On 2016/06/30 22:48:19, Ryan Sleevi wrote: > LAYERING: This comment strikes me as over-explaining something future / > unrelated. > > Are you sure this comment belongs? I've simply removed the comment. Regardless of the explanation there it's the right thing to set. https://codereview.chromium.org/2017563002/diff/40001/net/cert/merkle_tree_le... File net/cert/merkle_tree_leaf.h (right): https://codereview.chromium.org/2017563002/diff/40001/net/cert/merkle_tree_le... net/cert/merkle_tree_leaf.h:21: // Represents a MerkleTreeLeaf as defined in RFC6962, section 3.4. On 2016/06/30 22:48:19, Ryan Sleevi wrote: > Unfortunately, finding an explanation about how this relates to the overall > narrative is quite difficult. The documentation could have been improved here. Expanded the documentation (in another CL) to indicate what's the purpose of this struct. https://codereview.chromium.org/2017563002/diff/40001/net/cert/merkle_tree_le... net/cert/merkle_tree_leaf.h:23: // slightly differently. On 2016/06/30 22:48:19, Ryan Sleevi wrote: > Does RFC 6962-bis do something differently? It's unclear why we need this > structure. Yes, 6962-bis does (see details in the other comment). I think we'll have to carefully inspect all CT-related data structures when we come to implement 6962-bis, as serialization is different and not all fields have the same meaning or structure. https://codereview.chromium.org/2017563002/diff/40001/net/cert/merkle_tree_le... net/cert/merkle_tree_leaf.h:23: // slightly differently. On 2016/06/30 22:48:19, Ryan Sleevi wrote: > Why is it arranged differently? Explained (in another CL). https://codereview.chromium.org/2017563002/diff/40001/net/cert/merkle_tree_le... net/cert/merkle_tree_leaf.h:43: NET_EXPORT bool GetMerkleTreeLeaf(const X509Certificate* cert, On 2016/06/30 22:48:19, Ryan Sleevi wrote: > Should have been documented. Done (in a separate CL). https://codereview.chromium.org/2017563002/diff/40001/net/cert/merkle_tree_le... net/cert/merkle_tree_leaf.h:48: // Returns true if the hash was generated, false if an error occurred. On 2016/06/30 22:48:19, Ryan Sleevi wrote: > Same here - does 6962-bis change this? > > Naming wise, this should have been named something better HashMerkleTreeLeaf or > something; > > reading net::ct::Hash(...) doesn't really give much insight. (1) Renamed to HashMerkleTreeLeaf (in a separate CL). (2) Serialization in 6962-bis is different: * Data structures are different: in 6962-bis everything is a TransItem, including the tree leaf (https://tools.ietf.org/html/draft-ietf-trans-rfc6962-bis-16#section-5.4), so the hash is over a structure serialized in a different way. (2) The LogEntry from 6962 is incompatible with the equivalent in 6962-bis, because in 6962 the hash is over the entire X.509 certificate (if the entry is *not* a precertificate) or over the TBSCertificate + issuer key hash (if the entry *is* a precertificate). In 6962-bis the hash is always over the tbscertificate + issuer key hash. My suggestion is to reason about reconciling these structures with 6962-bis implementation once 6962-bis is finalized.
Hi Eran, I've revisited this CL, and perhaps a key piece I'm missing is why do you need the MerkleTreeLeaf structure at all? This naturally extends to the LeafStatus class. I'm having a difficult time picturing how this CL fits in overall with the goals linked on the bug and design doc, and I think it'd be great if you could help me understand that. I've tried to restate the problem, to make sure that my feedback is meaningful and not sending you down a rabbit hole, but this confusion may be useful to figure out how to reduce round trip time in the future - that is, where opportunities might exist to better explain the CL or document the code in the future. Part of this concern comes from your design seeming very inefficient, and I'm trying to wrap my head around its necessity. For example, storing the LogID on the MerkleTreeLeaf - if the MTL is just an aspect of the SingleTreeTracker's needs, and a SingleTreeTracker tracks a *single* LogID, then it seems extremely wasteful of memory to be storing the log_id on the MTL for every entry (C++ strings are not copy-on-write or shared-pool semantics) So let's make sure I'm stating the goals here: - SingleTreeTracker wants to observe SCTs as they come in (OnSCTVerified) - If the SCT timestamp is > (Now - MMD), then we want to wait until OnSTHObserved for a span that covers the timestamp - If the SCT timestamp is < (Now - MMD), then we want to see if we have an STH covering this timestamp. If we don't, it's the same as the previous step, but if we do, we want to go and make sure that the SCT is incorporated into the STH Finally, we want to make all of this information available for subsequent queries ( GetLogEntryInclusionStatus ) Am I good so far? My concerns about the efficiency of your design, beyond the log_id issue I touched on above, is that it seems that the more sites you visit, the more this memory will grow - we will unbounded-ly always track the included/pending inclusion status - is that correct? Is that desirable? Now, as to your specific goals, a given SCT can be in one of several states: - Waiting for a new STH - Waiting for an inclusion check after a suitable STH is found - Dormant, having been checked for inclusion and status information Am I right so far? For an SCT waiting for an STH, you need to keep track of the SCT (which implicitly contains the log_id by virtue of the STT being associated with, but explicitly contains it in the SCT), and when the SCT was first observed (why? I don't understand this) For an SCT pending inclusion check, you just need to track the STH you're comparing against (plus the previous information) For a checked SCT, you need to track the SCT and its inclusion status, right? (It's unclear to me why you need to track the leaf_index_ or observed_at_) From stating like that, it sounds like a simple state machine transition for a given SCT, and it seems like the information you're currently storing is entirely unnecessary. That's why I have trouble understanding the 'why' behind several portions of this CL. Now, I've read through the other bugs, and I want to make sure I'm fairly stating with the design of MerkleTreeLeaf: MTL itself is a subset of the information contained in the SCT. In particular, it removes the signature from the SCT (which seems like it would make it useless for gossip or audit reporting). It also removes the log_description (which we should probably rethinking about whether it belongs on the SCT anyways), version (which seems to highlight the design issue between MTL vs TransItem but OK, I can buy that), and the origin (which does seem unnecessary). Put differently, you could just skip storing an MTL entirely and store the original SignedCertificateTimestamp, since it has everything necessary to construct the MTL - but a few extra fields we don't want, and perhaps one we should want. But in the context of this CL, we just treat the switch to using MTL as the map key as an optimization - it's less than the full SCT, but more than the timestamp (since two SCTs can contain the same timestamp) Is that a fair summary statement of the problem? I just want to make sure I'm accurately understanding so that my further review comments and suggestions aren't unhelpful :) https://codereview.chromium.org/2017563002/diff/80001/components/certificate_... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/2017563002/diff/80001/components/certificate_... components/certificate_transparency/single_tree_tracker.h:103: class LeafState { Declaring all of this in the .h defeats the very purpose of having a private member like this - which is hiding the interface. You want to forward declare this.
Ryan, PTAL at the revised design. Based on your suggestions: - Instead of keeping the MerkleTreeLeaf I've switched to an internal struct that holds the SCT and the LogEntry, which is the minimum needed for auditing an entry. The MerkleTreeLeaf will be created and hashed on the fly when integrating with the DnsLogClient. - I got rid of the LeafState class as it is unnecessary if all it keeps is the state. NOTE: There are two hacks in this class, each clearly marked and, if the design is acceptable, will be fixed in a separate CL *before* this CL will be submitted. https://codereview.chromium.org/2017563002/diff/80001/components/certificate_... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/2017563002/diff/80001/components/certificate_... components/certificate_transparency/single_tree_tracker.h:103: class LeafState { On 2016/07/01 22:15:50, Ryan Sleevi (extremely slow) wrote: > Declaring all of this in the .h defeats the very purpose of having a private > member like this - which is hiding the interface. > > You want to forward declare this. Done.
https://codereview.chromium.org/2017563002/diff/100001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/100001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:41: // to the SignedCertificateTimestamp so we can avoid copying it. I don't understand why this hack is necessary? Because our refcounting is implemented intrusively, rather than externally (e.g. like shared_ptr is), you can always turn a raw pointer back into it's scoped_refptr<> I don't disagree that the API, if we're going to be holding references, should be passing the scoped_refptr<> to make that clearer, but you can always do entry.sct = sct and It Just Works https://codereview.chromium.org/2017563002/diff/100001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:57: struct SingleTreeTracker::EntryToAudit { Documentation :) https://codereview.chromium.org/2017563002/diff/100001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:84: const base::TimeDelta kMaximumMergeDelay = base::TimeDelta::FromHours(24); you can make this static const (TimeDelta is constexpr and thus doesn't add static initializers) https://codereview.chromium.org/2017563002/diff/100001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:93: // TODO(eranm): UMA - how often inclusion can be checked immediately. Why? Is this just a proxy metric for something else? What would this data be used to inform? If you're going to keep this TODO, at least explain more of the context for why this data might be useful, otherwise this feels a bit like Fermat's Last Theorem :) https://codereview.chromium.org/2017563002/diff/100001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:128: auto first_entry_not_included = std::find_if( if |observed_entries_| is sorted on timestamp, shouldn't this be std::lower_bound() ? https://codereview.chromium.org/2017563002/diff/100001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:151: auto entry_iterator = observed_entries_.find(entry); Do you actually need to reconstitute the LogEntry from the SCT? Couldn't you just find for an equality check on the SCT? https://codereview.chromium.org/2017563002/diff/100001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:179: return observed_entries_.find(entry) != observed_entries_.end(); This doesn't seem like it should be a member function (especially since you're forcing public exposure of it) https://codereview.chromium.org/2017563002/diff/100001/components/certificate... File components/certificate_transparency/single_tree_tracker.h (left): https://codereview.chromium.org/2017563002/diff/100001/components/certificate... components/certificate_transparency/single_tree_tracker.h:28: keep this. Consider the examples in https://google.github.io/styleguide/cppguide.html#Namespaces as equally normative, and you'll see that the namespaces all have newlines separating, except for incredibly small forward declarations (e.g. 20/21) I would even argue that we should have a newline at 16/17
Description was changed from ========== Certificate Transparency: Use MerkleTreeLeaf Another step towards inclusion checking: Construct MerkleTreeLeaf instances in the SingleTreeTracker, so their inclusion status could be checked and they could be hashed for verifying inclusion proofs once those are fetched. BUG=613495 ========== to ========== Implementation of CT log auditing over DNS. Audit CT logs by requesting inclusion proofs of observed SCTs from CT logs over DNS (using the LogDnsClient). BUG=613495 ==========
Description was changed from ========== Implementation of CT log auditing over DNS. Audit CT logs by requesting inclusion proofs of observed SCTs from CT logs over DNS (using the LogDnsClient). BUG=613495 ========== to ========== Audit CT logs by requesting inclusion proofs for observed Signed Certificate Timestamps from the CT log that issued them. To verify that a CT log behaves correctly and indeed publishes all the certificates it committed to publishing, it is necessary to verify that each observed log entry (as denoted by an SCT and the corresponding certificate) is included in the log. Check for inclusion of observed SCTs by finding out the leaf index of each observed SCT and requesting an inclusion proof from the CT log for it over DNS (using the LogDnsClient). Note that no action is taken based on the inclusion check results in this change. Follow-up changes will add telemetry. BUG=613495 ==========
Ryan, please take a look at patchset 9. This patchset incorporates all the feedback from our design discussions, in particular: - Only stores the leaf hashes for inclusion checking rather than the entire entry, as we currently do nothing with it (this is the minimal amount of information needed for auditing). - Uses a queue with bound size for entries pending inclusion check. - Uses an MRUCache for entries that were successfully checked for inclusion. - Implements LowMemoryObserver to clear both queues in case of memory pressure. There's also handling of asynchronous throttling by the LogDnsClient, which we still have to settle. NOTE: A couple of tests are missing (but are clearly documented) and TestEntryIncludedAfterInclusionCheckSuccess currently fails because I may be holding the MockLogDnsTraffic wrong. But the other tests pass. https://codereview.chromium.org/2017563002/diff/100001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/100001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:41: // to the SignedCertificateTimestamp so we can avoid copying it. On 2016/07/18 23:38:01, Ryan Sleevi (slow) wrote: > I don't understand why this hack is necessary? > > Because our refcounting is implemented intrusively, rather than externally (e.g. > like shared_ptr is), you can always turn a raw pointer back into it's > scoped_refptr<> > > I don't disagree that the API, if we're going to be holding references, should > be passing the scoped_refptr<> to make that clearer, but you can always do > > entry.sct = sct > > and It Just Works This is now obsolete. https://codereview.chromium.org/2017563002/diff/100001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:57: struct SingleTreeTracker::EntryToAudit { On 2016/07/18 23:38:01, Ryan Sleevi (slow) wrote: > Documentation :) Done. https://codereview.chromium.org/2017563002/diff/100001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:84: const base::TimeDelta kMaximumMergeDelay = base::TimeDelta::FromHours(24); On 2016/07/18 23:38:01, Ryan Sleevi (slow) wrote: > you can make this static const (TimeDelta is constexpr and thus doesn't add > static initializers) Done. https://codereview.chromium.org/2017563002/diff/100001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:93: // TODO(eranm): UMA - how often inclusion can be checked immediately. On 2016/07/18 23:38:01, Ryan Sleevi (slow) wrote: > Why? Is this just a proxy metric for something else? What would this data be > used to inform? > > If you're going to keep this TODO, at least explain more of the context for why > this data might be useful, otherwise this feels a bit like Fermat's Last Theorem > :) Obsolete - I've simply added the UMA logging. This will inform us: * How often can SCTs be checked immediately (i.e. how often SCTs are so fresh that clients see them before they see an STH covering them. If that is very rare we could make a case for requiring an inclusion proof together with the SCT when we implement 6962-bis, which supports that). * How many clients don't have valid STHs at all. * If the queue size we choose for the pending entries is enough (i.e. how often SCTs are dropped because the pending entries queue is full). https://codereview.chromium.org/2017563002/diff/100001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:128: auto first_entry_not_included = std::find_if( On 2016/07/18 23:38:01, Ryan Sleevi (slow) wrote: > if |observed_entries_| is sorted on timestamp, shouldn't this be > std::lower_bound() ? What I'd like to do is process entries whose timestamp + MMD > sth.timestamp. Since |pending_entries_| is sorted on timestamp, I have to find the first entry that's in PENDING_NEWER_STH state and the last entry whose timestamp + MMD < sth.timestamp. So I should use find_if to find the first entry that's in a pending state and lower_bound to find the entry I should stop at. https://codereview.chromium.org/2017563002/diff/100001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:151: auto entry_iterator = observed_entries_.find(entry); On 2016/07/18 23:38:01, Ryan Sleevi (slow) wrote: > Do you actually need to reconstitute the LogEntry from the SCT? Couldn't you > just find for an equality check on the SCT? Obsolete - The SCT is no longer stored, only the hash. https://codereview.chromium.org/2017563002/diff/100001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:179: return observed_entries_.find(entry) != observed_entries_.end(); On 2016/07/18 23:38:01, Ryan Sleevi (slow) wrote: > This doesn't seem like it should be a member function (especially since you're > forcing public exposure of it) Obsolete - removed. https://codereview.chromium.org/2017563002/diff/100001/components/certificate... File components/certificate_transparency/single_tree_tracker.h (left): https://codereview.chromium.org/2017563002/diff/100001/components/certificate... components/certificate_transparency/single_tree_tracker.h:28: On 2016/07/18 23:38:01, Ryan Sleevi (slow) wrote: > keep this. Consider the examples in > https://google.github.io/styleguide/cppguide.html#Namespaces as equally > normative, and you'll see that the namespaces all have newlines separating, > except for incredibly small forward declarations (e.g. 20/21) > > I would even argue that we should have a newline at 16/17 Done - for consistency I've added newlines after each namespace.
More of a nit, but since I keep fixing them up: Please make sure your commit messages are nicely wrapped (see http://chris.beams.io/posts/git-commit/ ). :)
Description was changed from ========== Audit CT logs by requesting inclusion proofs for observed Signed Certificate Timestamps from the CT log that issued them. To verify that a CT log behaves correctly and indeed publishes all the certificates it committed to publishing, it is necessary to verify that each observed log entry (as denoted by an SCT and the corresponding certificate) is included in the log. Check for inclusion of observed SCTs by finding out the leaf index of each observed SCT and requesting an inclusion proof from the CT log for it over DNS (using the LogDnsClient). Note that no action is taken based on the inclusion check results in this change. Follow-up changes will add telemetry. BUG=613495 ========== to ========== Audit CT logs by requesting inclusion proofs for observed Signed Certificate Timestamps from the CT log that issued them. To verify that a CT log behaves correctly and indeed publishes all the certificates it committed to publishing, it is necessary to verify that each observed log entry (as denoted by an SCT and the corresponding certificate) is included in the log. Check for inclusion of observed SCTs by finding out the leaf index of each observed SCT and requesting an inclusion proof from the CT log for it over DNS (using the LogDnsClient). Note that no action is taken based on the inclusion check results in this change. Follow-up changes will add telemetry. BUG=613495 ==========
Description was changed from ========== Audit CT logs by requesting inclusion proofs for observed Signed Certificate Timestamps from the CT log that issued them. To verify that a CT log behaves correctly and indeed publishes all the certificates it committed to publishing, it is necessary to verify that each observed log entry (as denoted by an SCT and the corresponding certificate) is included in the log. Check for inclusion of observed SCTs by finding out the leaf index of each observed SCT and requesting an inclusion proof from the CT log for it over DNS (using the LogDnsClient). Note that no action is taken based on the inclusion check results in this change. Follow-up changes will add telemetry. BUG=613495 ========== to ========== Audit CT logs by requesting inclusion proofs for observed Signed Certificate Timestamps from the CT log that issued them. To verify that a CT log behaves correctly and indeed publishes all the certificates it committed to publishing, it is necessary to verify that each observed log entry (as denoted by an SCT and the corresponding certificate) is included in the log. Check for inclusion of observed SCTs by finding out the leaf index of each observed SCT and requesting an inclusion proof from the CT log for it over DNS (using the LogDnsClient). Note that no action is taken based on the inclusion check results in this change. Follow-up changes will add telemetry. BUG=613495 ==========
https://codereview.chromium.org/2017563002/diff/160001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:68: return std::string(""); std::string() https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:72: return std::string(""); std::string() https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:90: }; Unfortunately, I found this "spread-apart" state a bit harder to reason about than what we'd talked about. The reason for this is now the state (and state transitions) are no longer logically part of one class, but spread throughout the SingleTreeTracker. The benefit of state machines is in trying to group the state transitions and edges together, which, combined with a class outline (e.g. separate from the implementation, even when in the .cc), can make it easier to get the high-level summary of how edges transition and what state is conveyed through them. https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:109: base::TimeDelta::FromHours(kMaximumMergeDelayInHours); Why do this as a function-level static? Why not just make kMaximumMergeDelay a static base::TimeDelta (which can be constexpr init'd)? https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:120: const std::string leaf_hash; Why string rather than SHA256HashValue? (see a bunch of the remarks below though) https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:129: leaf_hash(GetLogEntryLeafHash(cert, sct)) {} https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors "avoid initialization that can fail if you can't signal an error" https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:145: // necessary to re-fetch it. "it won't be necessary to re-fetch it" reads weird - is 'it' the newer STH or the inclusion proof? I presume you mean "so it won't be necessary to re-fetch the inclusion proof", but the subtlety of why this is still non-obvious to me, so it may be useful to expand the comment here. https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:170: // to work with, that type is chosen for |leaf_hash|. It's like you already knew where my objection was going to be. This feels very inefficient. We shouldn't do this :) https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:173: std::min(32ul, lhs.leaf_hash.size())); When is it ever valid for leaf_hash.size() to != 32? Isn't that something that should be guaranteed before it ever gets to this place in code? Why .c_str(), rather than .data()? Why marshal to SHA256HashValue, when you could just compare the strings directly? Why .operator() rather than SHA256HashValueLessThan()(lhs_hash, rhs_hash)? Is this a Most Vexing Parse? solve all your problems at once: return std::tie(lhs.sct_timestamp, lhs.hash) < std::tie(rhs.sct_timestamp, rhs.hash); https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:236: // Since the LogDnsClient will invoke the callback with Right, LogDnsClient absolutely needs a way to directly signal to consumers once it's no longer throttling. The pattern recommended here would be something like int rv = dns_client_->DoStuff(...); if (rv == ERR_TEMPORARILY_TRHOTTLED) { dns_client_->NotifyWhenNotBusy(base::Bind(&SingleTreeTracker::OnSomething, weak_factory_.GetWeakPtr())); return; } And dns_client_ can then just // Do work // Enter a state where it is about to yield, and is ready for more work for (const auto& observer : observers_) { PostTask(observer); } observers_.clear(); // Yield Note that I wrote it in the way above because there's all sorts of nuances to consider here. An Observer* pattern would likely feed the first observer while constantly starving out other observers, so you likely don't want the watching "permanent". You also don't want to PostTask the moment you've drained something, since you may need to invoke it's callback, which could cause the draining-tasks SingleTreeTracker to ProcessPendingEntries again and fill the queue. Granted, you still have a starvation risk with the above pattern, but unless you implement the queuing in the SingleTreeTracker along with some round-robin scheme of notification, I don't think you an escape that. https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:273: return; Rewrite the condition then to make the 'return' the fast-path https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:280: }; My same remarks about callback temporaries apply to lambda temporaries that are only used once. https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:297: std::lower_bound(pending_entries_.begin(), pending_entries_.end(), BUG? Shouldn't this be first_entry_to_audit, not pending_entries_.begin() ? Otherwise, what guarantee are you to have that curr_entry < entry_to_stop_at ? https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:300: auto curr_entry = first_entry_to_audit; for (auto curr_entry = first_entry_to_audit; curr_entry != entry_to_stop_at; ++curr_entry) { DCHECK_EQ(PENDING_NEWER_STH, curr_entry->second.state); curr_entry->second.state = PENDING_LEAF_INDEX_REQUEST; } ? https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:316: return GetLogEntryInclusionStatus(entry); There's a noticeable asymmetry here with the callers to GetLogEntryInclusionStatus - one which checks entry.is_valid() before calling, the other (this) which depends on GetLogEntryInclusionStatus doing that check. I'm not clear which is better, although I suspect it's "caller checks" given you've only got two callers. https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:320: const EntryToAudit* entry_ptr = &entry; This makes me *really* nervous - |entry| could be a stack-local with the way this function is written. I cannot convince myself this is safe. https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:324: weak_factory_.GetWeakPtr(), entry_ptr); I think I may have mentioned in other CLs, but I personally find this approach - of stashing callbacks in temporaries - hinders, rather than helps, readability in understanding how things work/are executed, especially in CodeSearch. Unless there is specific and concrete need, please prefer to avoid ever creating stack-local callbacks. Beyond avoiding some additional refcount churn, I believe this helps keep the code more readable :) https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:461: break; Is this necessary? I'm guessing yes (Compiler warning?), but might be useful for documentation. https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:463: pending_entries_.clear(); https://google.github.io/styleguide/cppguide.html#Loops_and_Switch_Statements "Annotate non-trivial fall-through between cases" This one threw me for a loop when reading, whether or not the break was intentional. https://codereview.chromium.org/2017563002/diff/160001/components/certificate... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.h:110: private: In general, whenever there's a significant expansion of the "private" section of a class, it generally is something that you can expect I'll flag as a design smell :) Anything that requires a lot of private: implementation generally signals that 1) The implementing class itself is large and complex and may be violating the Single Responsibility Principle 2) All consumers are implicitly being made dependent on the implementation details of this class, since they'd have to be recompiled any time this header changes. That's usually a red flag That's not to say private methods are forbidden, by no means, just that you're going to get a lot of extra scrutiny from me ;) https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.h:119: using EntriesMap = std::map<EntryToAudit, EntryAuditState, OrderByTimestamp>; Does this typedef (which is, effectively, only used once) provide value? Why? Seems like we could just defer all this to the pending_entries_ and use auto / typename when appropriate. std::map<EntryToAudit, EntryAuditState, OrderByTimestamp> entries_map_;
Ryan, I've addressed your comments, but I suggest holding off further review until we figure out the right interface for the LogDnsClient, which will then affect this change. https://codereview.chromium.org/2017563002/diff/160001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:68: return std::string(""); On 2016/09/22 07:08:29, Ryan Sleevi (slow) wrote: > std::string() Done. https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:72: return std::string(""); On 2016/09/22 07:08:28, Ryan Sleevi (slow) wrote: > std::string() Done. https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:90: }; On 2016/09/22 07:08:28, Ryan Sleevi (slow) wrote: > Unfortunately, I found this "spread-apart" state a bit harder to reason about > than what we'd talked about. > > The reason for this is now the state (and state transitions) are no longer > logically part of one class, but spread throughout the SingleTreeTracker. The > benefit of state machines is in trying to group the state transitions and edges > together, which, combined with a class outline (e.g. separate from the > implementation, even when in the .cc), can make it easier to get the high-level > summary of how edges transition and what state is conveyed through them. Acknowledged; Having the state transitions in one place would make it easier to reason about them. Do you suggest putting this state in the EntryToAudit struct and making it a full-fledged class? I don't mind doing that, but it may take up the SingleTreeTracker's ability to handle throttling at a class level (and instead each entry would throttle itself). I think we could avoid many of the states, potentially all of them, if the LogDnsClient only had one method that would return the leaf index and inclusion proof nodes based on the leaf hash and throttled requests asynchronously. https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:109: base::TimeDelta::FromHours(kMaximumMergeDelayInHours); On 2016/09/22 07:08:28, Ryan Sleevi (slow) wrote: > Why do this as a function-level static? Why not just make kMaximumMergeDelay a > static base::TimeDelta (which can be constexpr init'd)? Done; haven't noticed the exception to https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables made for global variables of class type that can be constexpr initialized. https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:120: const std::string leaf_hash; On 2016/09/22 07:08:28, Ryan Sleevi (slow) wrote: > Why string rather than SHA256HashValue? (see a bunch of the remarks below > though) Done. https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:129: leaf_hash(GetLogEntryLeafHash(cert, sct)) {} On 2016/09/22 07:08:28, Ryan Sleevi (slow) wrote: > https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors > > "avoid initialization that can fail if you can't signal an error" Done - changed GetLogEntryLeafHash to be called before the EntryToAudit is created. https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:145: // necessary to re-fetch it. On 2016/09/22 07:08:29, Ryan Sleevi (slow) wrote: > "it won't be necessary to re-fetch it" reads weird - is 'it' the newer STH or > the inclusion proof? I presume you mean "so it won't be necessary to re-fetch > the inclusion proof", but the subtlety of why this is still non-obvious to me, > so it may be useful to expand the comment here. Done - I've re-worded that comment a bit. The gist is that all of the following are inputs to the inclusion proof verification method and they all have to match: * leaf index (fixed). * leaf hash (also fixed) * tree size (gets updated when new STHs are delivered). * inclusion proof nodes (depend on the tree size provided to QueryInclusionProofNodes). * tree root hash (gets updated when new STHs are delivered). So if we've requested an inclusion proof for node K in tree of size N, then a newer STH with N' is received, the obtained inclusion proof will not be valid in the tree N' and a new one will have to be fetched - or we just store N. https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:170: // to work with, that type is chosen for |leaf_hash|. On 2016/09/22 07:08:28, Ryan Sleevi (slow) wrote: > It's like you already knew where my objection was going to be. > > This feels very inefficient. We shouldn't do this :) Done - EntryToAudit now holds a SHA256HashValue. https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:173: std::min(32ul, lhs.leaf_hash.size())); On 2016/09/22 07:08:28, Ryan Sleevi (slow) wrote: > When is it ever valid for leaf_hash.size() to != 32? Isn't that something that > should be guaranteed before it ever gets to this place in code? > > Why .c_str(), rather than .data()? > > Why marshal to SHA256HashValue, when you could just compare the strings > directly? > > Why .operator() rather than SHA256HashValueLessThan()(lhs_hash, rhs_hash)? Is > this a Most Vexing Parse? > > solve all your problems at once: > > return std::tie(lhs.sct_timestamp, lhs.hash) < std::tie(rhs.sct_timestamp, > rhs.hash); Done - obsolete - I can now use SHA256HashValueLessThan directly. https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:236: // Since the LogDnsClient will invoke the callback with On 2016/09/22 07:08:29, Ryan Sleevi (slow) wrote: > Right, LogDnsClient absolutely needs a way to directly signal to consumers once > it's no longer throttling. > > The pattern recommended here would be something like > > int rv = dns_client_->DoStuff(...); > if (rv == ERR_TEMPORARILY_TRHOTTLED) { > dns_client_->NotifyWhenNotBusy(base::Bind(&SingleTreeTracker::OnSomething, > weak_factory_.GetWeakPtr())); > return; > } > > And dns_client_ can then just > > // Do work > // Enter a state where it is about to yield, and is ready for more work > for (const auto& observer : observers_) { > PostTask(observer); > } > observers_.clear(); > // Yield > > > Note that I wrote it in the way above because there's all sorts of nuances to > consider here. An Observer* pattern would likely feed the first observer while > constantly starving out other observers, so you likely don't want the watching > "permanent". You also don't want to PostTask the moment you've drained > something, since you may need to invoke it's callback, which could cause the > draining-tasks SingleTreeTracker to ProcessPendingEntries again and fill the > queue. Granted, you still have a starvation risk with the above pattern, but > unless you implement the queuing in the SingleTreeTracker along with some > round-robin scheme of notification, I don't think you an escape that. Thanks, that's exactly the advice I was after on how to un-throttle. I'm not too worried about starvation with the above pattern since not all SingleTreeTrackers will have queues of the same size - some logs are more popular than others and so more SCTs from the popular logs will be observed. https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:273: return; On 2016/09/22 07:08:28, Ryan Sleevi (slow) wrote: > Rewrite the condition then to make the 'return' the fast-path Done. https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:280: }; On 2016/09/22 07:08:28, Ryan Sleevi (slow) wrote: > My same remarks about callback temporaries apply to lambda temporaries that are > only used once. Done. https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:297: std::lower_bound(pending_entries_.begin(), pending_entries_.end(), On 2016/09/22 07:08:28, Ryan Sleevi (slow) wrote: > BUG? Shouldn't this be first_entry_to_audit, not pending_entries_.begin() ? > Otherwise, what guarantee are you to have that curr_entry < entry_to_stop_at ? Changed to first_entry_to_audit rather than pending_entries_.begin(). Given the ordering by timestamp and the fact that only entries before a certain timestamp change state to other than pending, there should not be any entry in a state other than PENDING_NEWER_STH after first_entry_to_audit. We could DCHECK for this invariant if you think that's valuable? https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:300: auto curr_entry = first_entry_to_audit; On 2016/09/22 07:08:28, Ryan Sleevi (slow) wrote: > for (auto curr_entry = first_entry_to_audit; curr_entry != entry_to_stop_at; > ++curr_entry) { > DCHECK_EQ(PENDING_NEWER_STH, curr_entry->second.state); > curr_entry->second.state = PENDING_LEAF_INDEX_REQUEST; > } > > ? Done. https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:316: return GetLogEntryInclusionStatus(entry); On 2016/09/22 07:08:29, Ryan Sleevi (slow) wrote: > There's a noticeable asymmetry here with the callers to > GetLogEntryInclusionStatus - one which checks entry.is_valid() before calling, > the other (this) which depends on GetLogEntryInclusionStatus doing that check. > > I'm not clear which is better, although I suspect it's "caller checks" given > you've only got two callers. Obsolete - the entry no longer has is_valid method and we don't create it if we don't have a valid hash. https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:320: const EntryToAudit* entry_ptr = &entry; On 2016/09/22 07:08:28, Ryan Sleevi (slow) wrote: > This makes me *really* nervous - |entry| could be a stack-local with the way > this function is written. > > I cannot convince myself this is safe. Obsolete - inlined this function (though my preference would have been to leave it, but that meant passing in a pointer as input parameter, which is against the style guide AFAIK). https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:324: weak_factory_.GetWeakPtr(), entry_ptr); On 2016/09/22 07:08:28, Ryan Sleevi (slow) wrote: > I think I may have mentioned in other CLs, but I personally find this approach - > of stashing callbacks in temporaries - hinders, rather than helps, readability > in understanding how things work/are executed, especially in CodeSearch. > > Unless there is specific and concrete need, please prefer to avoid ever creating > stack-local callbacks. Beyond avoiding some additional refcount churn, I believe > this helps keep the code more readable :) Done - in-lined callback creation. You may have mentioned that, I find the other approach works better for me - that keeping intermediate values in temporary variables, has two benefits in my opinion: (1) Documentation: The variable name serves as a description of what the intermediate value is. (2) Readability of complex/long statements: where it reduces the nesting of expressions. In this case, it makes it easier to see what's being passed into QueryLeafIndex. Since you find the other way more readable and you do have to review everything going into //components/certificate_transparency, I'll stick to your suggested approach. https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:461: break; On 2016/09/22 07:08:28, Ryan Sleevi (slow) wrote: > Is this necessary? I'm guessing yes (Compiler warning?), but might be useful for > documentation. Yes, it is. Are you suggesting I document it? https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:463: pending_entries_.clear(); On 2016/09/22 07:08:29, Ryan Sleevi (slow) wrote: > https://google.github.io/styleguide/cppguide.html#Loops_and_Switch_Statements > > "Annotate non-trivial fall-through between cases" > > This one threw me for a loop when reading, whether or not the break was > intentional. Done. https://codereview.chromium.org/2017563002/diff/160001/components/certificate... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.h:110: private: On 2016/09/22 07:08:29, Ryan Sleevi (slow) wrote: > In general, whenever there's a significant expansion of the "private" section of > a class, it generally is something that you can expect I'll flag as a design > smell :) > > Anything that requires a lot of private: implementation generally signals that > 1) The implementing class itself is large and complex and may be violating the > Single Responsibility Principle > 2) All consumers are implicitly being made dependent on the implementation > details of this class, since they'd have to be recompiled any time this header > changes. That's usually a red flag > > That's not to say private methods are forbidden, by no means, just that you're > going to get a lot of extra scrutiny from me ;) Acknowledged and partially addressed: To address your other comments I've removed two private methods, FetchIndexForEntry and FetchInclusionForEntry. The rest is mostly callbacks. If there are other private methods that you think could be removed, do point them out. In that case I've encapsulated the different states for entries in the .cc. If we change the LogDnsClient to have a single method that returns the inclusion proof and leaf index, then the size of the private section is cut by a third - maybe even more if throttling is synchronous (then the entry does not a state at all). https://codereview.chromium.org/2017563002/diff/160001/components/certificate... components/certificate_transparency/single_tree_tracker.h:119: using EntriesMap = std::map<EntryToAudit, EntryAuditState, OrderByTimestamp>; On 2016/09/22 07:08:29, Ryan Sleevi (slow) wrote: > Does this typedef (which is, effectively, only used once) provide value? Why? > Seems like we could just defer all this to the pending_entries_ and use auto / > typename when appropriate. > > std::map<EntryToAudit, EntryAuditState, OrderByTimestamp> entries_map_; It does not - removed.
The CQ bit was checked by eranm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by eranm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by eranm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by eranm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Ryan, this is now ready for you to review. The latest patchset utilizes all of the functionality added to LogDnsClient, adds tests for these cases and emits telemetry as we discussed.
First pass to make sure there aren't any significant design issues. Let me know when you're comfortable for round two :) https://codereview.chromium.org/2017563002/diff/260001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:100: DCHECK(leaf_hash_str.size() == crypto::kSHA256Length); 1) Why not DCHECK_EQ 2) Is DCHECK vs CHECK the right answer, given the memcpy below? https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:144: EntryToAudit(base::Time timestamp) : sct_timestamp(timestamp) {} explicit? Necessary? https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:166: EntryAuditState(AuditState state) : state(state) {} explicit? Necessary? https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:189: &SingleTreeTracker::OnMemoryPressure, base::Unretained(this)))); Are you guaranteed this is safe? That is, I can't see any documentation in https://cs.chromium.org/chromium/src/base/memory/memory_pressure_listener.h?r... that suggests base::Unretained() is safe here - that you won't have a pending PostTask floating out there. https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:225: } else { Should these } else { have short-circuited returns? That is if (verified_sth_...) { ... } else { ... } return; } LogCanBeChecked... ... ProcessPendingEntries(); https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:303: while (it != pending_entries_.end()) { DANGER: Why a while loop versus a for loop? It seems like you could do the for loop, which at least provides better ease at reasoning about the termination of this loop eventually happening. https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:315: weak_factory_.GetWeakPtr(), &(it->first))); DANGER: Messing with the key on a map strikes me as super-dangerous and hard to reason about the safety and mutability (if you mutate it->first, you can easily violate all of the invariants) https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:316: DCHECK(result != net::OK) DCHECK_EQ https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:317: << "Handling proofs returned synchronously is not implemeted."; Unnecessary https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:334: pending_entries_.erase(it++); BUG: This triggers all of the heuristics for "This is likely a violation of some important semantic". The normal pattern you expect is soemthing like it = pending_entries_.erase(it); (or something similar). This is because for most STL containers, a mutation invalidates iterators. https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:336: // BUG: an invalid argument was provided or an unexpected error Is this a bug in your code? Or are you saying it's a violation of an invariant? It sounds like bug in your code. https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:338: DCHECK(result == net::ERR_INVALID_ARGUMENT); DCHECK_EQ https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:343: ++it; nit: error control goes first https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:350: auto checked_entries_iterator = checked_entries_.Get( auto or const auto? https://codereview.chromium.org/2017563002/diff/260001/components/certificate... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.h:113: // Orders instances by the timestamp of the SCT they contain. nit: What sort of ordering? :) // Less-than comparator that orders entries from the oldest SCT timestamp to the newest SCT timestamp ? https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.h:119: // (until throttled by the LogDnsClient). nit: is the parenthetical necessary? Seems significant - not incidental. https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.h:126: SCTInclusionStatus GetLogEntryInclusionStatus(const EntryToAudit& entry); Given the public/private split and subtlety, I don't think this meets the criteria of https://google.github.io/styleguide/cppguide.html#Function_Overloading and should be renamed GetAuditedEntryInclusionStatus ? https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.h:128: // LogDnsClient callbacks nit: I haven't really seen comments like this (e.g. "callbacks"). It doesn't tell me what this does/what it's for (other than documenting an implementation detail). // Invoked when [X], does [Y] might be more useful here. Just going from the header. https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.h:141: // Map of *pending* log entries to their state. nit: omit * https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.h:148: // values are expected to be 'true'. So why store by bool, and not an empty no-op class? https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.h:150: // possible to provide a comparator for it to the MRUCache. Is there a bug #? Is there an explanation about why that would or would not be desirable? Is there a context for why you're using SHA256 as the hash versus something else? https://codereview.chromium.org/2017563002/diff/260001/components/certificate... File components/certificate_transparency/tree_state_tracker.h (right): https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/tree_state_tracker.h:63: // trackers. Must be deleted *after* the tree trackers. nit: remove **
Thanks for the quick first pass, PTAL. I've addressed all of your comments, except one. * Note the restructuring of SingleTreeTracker::ProcessPendingEntries, which I believe is in line with the spirit of your comments (not just the letter). The un-addressed comments is in SingleTreeTracker::ProcessPendingEntries, where I wasn't sure how to deal with the cases where the net error returned could either be unexpected or indicating incorrect use of the LogDnsClient. https://codereview.chromium.org/2017563002/diff/260001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:100: DCHECK(leaf_hash_str.size() == crypto::kSHA256Length); On 2016/11/14 05:03:28, Ryan Sleevi (at IETF til 11-20 wrote: > 1) Why not DCHECK_EQ > 2) Is DCHECK vs CHECK the right answer, given the memcpy below? (1) Done (2) Switched to CHECK, good point. https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:144: EntryToAudit(base::Time timestamp) : sct_timestamp(timestamp) {} On 2016/11/14 05:03:27, Ryan Sleevi (at IETF til 11-20 wrote: > explicit? Done, added. > Necessary? Not essential, but spares setting it after creating the object (since it's pointless to create an EntryToAudit instance that does not relate to a specific entry, already known at the time of instantiation of the EntryToAudit entry). https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:166: EntryAuditState(AuditState state) : state(state) {} On 2016/11/14 05:03:28, Ryan Sleevi (at IETF til 11-20 wrote: > explicit? Done, added. >Necessary? Same as above, not absolutely necessary but means instances can be created in-line. https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:189: &SingleTreeTracker::OnMemoryPressure, base::Unretained(this)))); On 2016/11/14 05:03:28, Ryan Sleevi (at IETF til 11-20 wrote: > Are you guaranteed this is safe? That is, I can't see any documentation in > https://cs.chromium.org/chromium/src/base/memory/memory_pressure_listener.h?r... > that suggests base::Unretained() is safe here - that you won't have a pending > PostTask floating out there. Switched to using a weakptr. The use of base::Unretained is the pattern I saw, so I assumed it's safe (I don't see *any* other way it's called: https://cs.chromium.org/search/?q=%22new+base::MemoryPressureListener%22&sq=p...) https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:225: } else { On 2016/11/14 05:03:28, Ryan Sleevi (at IETF til 11-20 wrote: > Should these } else { have short-circuited returns? > > That is > > if (verified_sth_...) { > ... > } else { > ... > } > return; > } > > LogCanBeChecked... > ... > ProcessPendingEntries(); Done. https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:303: while (it != pending_entries_.end()) { On 2016/11/14 05:03:28, Ryan Sleevi (at IETF til 11-20 wrote: > DANGER: Why a while loop versus a for loop? It seems like you could do the for > loop, which at least provides better ease at reasoning about the termination of > this loop eventually happening. Done - switched to a for loop. https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:315: weak_factory_.GetWeakPtr(), &(it->first))); On 2016/11/14 05:03:27, Ryan Sleevi (at IETF til 11-20 wrote: > DANGER: Messing with the key on a map strikes me as super-dangerous and hard to > reason about the safety and mutability (if you mutate it->first, you can easily > violate all of the invariants) I wast taking a pointer that was used in a const context, but instead I bind a const reference. So should be addressed. https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:316: DCHECK(result != net::OK) On 2016/11/14 05:03:28, Ryan Sleevi (at IETF til 11-20 wrote: > DCHECK_EQ Done. https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:317: << "Handling proofs returned synchronously is not implemeted."; On 2016/11/14 05:03:27, Ryan Sleevi (at IETF til 11-20 wrote: > Unnecessary Done. https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:334: pending_entries_.erase(it++); On 2016/11/14 05:03:28, Ryan Sleevi (at IETF til 11-20 wrote: > BUG: This triggers all of the heuristics for "This is likely a violation of some > important semantic". The normal pattern you expect is soemthing like > > it = pending_entries_.erase(it); > > (or something similar). This is because for most STL containers, a mutation > invalidates iterators. Done. https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:336: // BUG: an invalid argument was provided or an unexpected error On 2016/11/14 05:03:27, Ryan Sleevi (at IETF til 11-20 wrote: > Is this a bug in your code? Or are you saying it's a violation of an invariant? > It sounds like bug in your code. There are two cases: * ERR_INVALID_ARGUMENT was returned - this is a bug in my code. * something other than ERR_INVALID_ARGUMENT was returned - this is a violation of the contract with the LogDnsClient. How do you suggest I express it? In theory I could DCHECK_EQ(...) for the first case, and then remove the entry so no future calls to QueryAuditProof are made and have a NOTREACHED for the other case. Does that seem sensible or an overkill to you? https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:338: DCHECK(result == net::ERR_INVALID_ARGUMENT); On 2016/11/14 05:03:27, Ryan Sleevi (at IETF til 11-20 wrote: > DCHECK_EQ Done. https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:343: ++it; On 2016/11/14 05:03:27, Ryan Sleevi (at IETF til 11-20 wrote: > nit: error control goes first Done. https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:350: auto checked_entries_iterator = checked_entries_.Get( On 2016/11/14 05:03:28, Ryan Sleevi (at IETF til 11-20 wrote: > auto or const auto? Done. https://codereview.chromium.org/2017563002/diff/260001/components/certificate... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.h:113: // Orders instances by the timestamp of the SCT they contain. On 2016/11/14 05:03:28, Ryan Sleevi (at IETF til 11-20 wrote: > nit: What sort of ordering? :) > > // Less-than comparator that orders entries from the oldest SCT timestamp to the > newest SCT timestamp > > ? Done. https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.h:119: // (until throttled by the LogDnsClient). On 2016/11/14 05:03:28, Ryan Sleevi (at IETF til 11-20 wrote: > nit: is the parenthetical necessary? Seems significant - not incidental. Done. https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.h:126: SCTInclusionStatus GetLogEntryInclusionStatus(const EntryToAudit& entry); On 2016/11/14 05:03:28, Ryan Sleevi (at IETF til 11-20 wrote: > Given the public/private split and subtlety, I don't think this meets the > criteria of > https://google.github.io/styleguide/cppguide.html#Function_Overloading and > should be renamed > > GetAuditedEntryInclusionStatus ? Done. https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.h:128: // LogDnsClient callbacks On 2016/11/14 05:03:28, Ryan Sleevi (at IETF til 11-20 wrote: > nit: I haven't really seen comments like this (e.g. "callbacks"). It doesn't > tell me what this does/what it's for (other than documenting an implementation > detail). > > // Invoked when [X], does [Y] > > might be more useful here. Just going from the header. Done. https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.h:141: // Map of *pending* log entries to their state. On 2016/11/14 05:03:28, Ryan Sleevi (at IETF til 11-20 wrote: > nit: omit * Done. https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.h:148: // values are expected to be 'true'. On 2016/11/14 05:03:28, Ryan Sleevi (at IETF til 11-20 wrote: > So why store by bool, and not an empty no-op class? Done, I hope: I've added a struct (EntryAuditResult) with no fields as the value type. https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.h:150: // possible to provide a comparator for it to the MRUCache. On 2016/11/14 05:03:28, Ryan Sleevi (at IETF til 11-20 wrote: > Is there a bug #? Is there an explanation about why that would or would not be > desirable? Is there a context for why you're using SHA256 as the hash versus > something else? Done: (1) Explicitly state that the hash here is the leaf hash of the entry, which is already calculated and is the way to uniquely identify entries in the tree. (2) Filed a bug (and added a link from here) explaining why the SHA256HashValue type can't be used in the MRUCache. https://codereview.chromium.org/2017563002/diff/260001/components/certificate... File components/certificate_transparency/tree_state_tracker.h (right): https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/tree_state_tracker.h:63: // trackers. Must be deleted *after* the tree trackers. On 2016/11/14 05:03:28, Ryan Sleevi (at IETF til 11-20 wrote: > nit: remove ** Done.
The CQ bit was checked by eranm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by eranm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Ryan, PTAL - addressed your in-person review comments (but haven't mailed the MemoryPressureListener owners yet). https://codereview.chromium.org/2017563002/diff/300001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/300001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:191: &SingleTreeTracker::OnMemoryPressure, weak_factory_.GetWeakPtr()))); Follow up with MemoryPressureListener on whether any pending callbacks are cancelled when the MemoryPressureListener is deleted - if not, there may be a pending callback with a pointer to the object. Likely it's safe to use base::Unretained because it's using ObserverList (which does cancel pending tasks) but it should be documented clearly. (ask bashi, gab and haraken)
And I found out why this missed multiple review passes - it was hidden in the UI with pending comments. https://codereview.chromium.org/2017563002/diff/260001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/260001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:189: &SingleTreeTracker::OnMemoryPressure, base::Unretained(this)))); On 2016/11/16 14:24:10, Eran Messeri wrote: > Switched to using a weakptr. The use of base::Unretained is the pattern I saw, > so I assumed it's safe (I don't see *any* other way it's called: > https://cs.chromium.org/search/?q=%22new+base::MemoryPressureListener%22&sq=p...) Apologies for not providing better guidance. While I think a WeakPtr is 'safe' to use here (e.g. it reflects the uncertainty), my hope was that it would have been pushed to 'upstream'. That is, if upstream already guarantees it's safe, if it's a pattern used by other code, then the only 'issue' here is just that the header didn't document it as safe. In that case, the response could have been "Yes, it's safe, but I see that's not clear, so I'll submit a CL to update the documentation of MemoryPressureListener". Or "Yes, it's safe, you can see this pattern in [X]/[Y]" https://codereview.chromium.org/2017563002/diff/300001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/300001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:399: } else { Similar to previous notes if (net_error != net::OK) { ... } else { ... } Put the error case first, common case (larger case) second https://codereview.chromium.org/2017563002/diff/340001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:100: CHECK_EQ(leaf_hash_str.size(), crypto::kSHA256Length); Is this CHECK necessary? It seems like you're defending against HashMerkleTreeLeaf violating its API contract ( https://cs.chromium.org/chromium/src/net/cert/merkle_tree_leaf.h?rcl=0&l=59 ) - a DCHECK or just assuming/expecting "API works as documented" seems fine. (FWIW, when looking deeper at the API, it does raise whether HashMerkleTreeLeaf should populate a SHA256HashValue, which would allow for use of crypto::SHA256HashBytes and avoid the memcpies - from SHA-256 to string, and from string to here) https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:357: crypto::kSHA256Length)); Dang, this seems to be non-ideal for performance ;( https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:387: if (net_error == net::OK) { if (net_error != net::OK) { LogInclusionCheckResult(FAILED_GETTING_INCLUSION_PROOF); pending_entries_.erase(it); return; } ? https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:389: reinterpret_cast<const char*>(it->first.leaf_hash.data), Isn't it->first the same as using |entry| ? https://codereview.chromium.org/2017563002/diff/340001/components/certificate... File components/certificate_transparency/single_tree_tracker_unittest.cc (right): https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:99: T... scts) { This seems like a lot of... undesirable template overhead. What's legal isn't always what's desirable - from codegen or readability. I'm trying to parse what this function does (could have used more documentation) and how it's called (the use of var-args == super subtle about number and ordering of params) https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:111: base::TimeDelta::FromMilliseconds(INT64_C(1365354256089)); Document. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:116: 0x0a, 0x79, 0x0d, 0x9e, 0x40, 0x55, 0xc3, 0xe6}; Document. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:121: decoded_root_hash.size()); Why is 117-121 necessary? Can't you just copy in directly to the sth? https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:131: 0x07, 0xb3, 0x4b, 0xea, 0x5b, 0xdd, 0x81, 0x2d}; Document. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:165: ClearDnsConfig(); Why is this necessary? Why can't it just be handled by the ctor (see below) https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:177: net_change_notifier_.reset(net::NetworkChangeNotifier::CreateMock()); Every test creates a new instance, so the first .reset() is unnecessary. If the dtor for net_change_notifier_ doesn't cleanup global state, that sounds like a bug? https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:180: mock_dns_->InitializeDnsConfig(); Do these need to be unique? Can they just be members? https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:183: void CreateTreeTracker() { Doesn't seem necessary to have this helper function - but am I misreading? https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:190: bool ExpectLeafHashRequestAndThrottle( Document https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:192: scoped_refptr<SignedCertificateTimestamp> sct) { Doesn't seem like you need to be passing these byval - you're not taking ownership of them. Pass either by naked pointers or const-ref? https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:195: ".hash.dns.example." Mostly since you're adding it in this test, I think it's non-obvious the relationship between all of these "hash.dns.example.com" (with their weird line breaking - is this cl formatt'd?) and line 153 (which actually sets it up). This seems like the sort of thing you'd express via two constants. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:339: ClearDnsConfig(); Unnecessary? https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:366: ClearDnsConfig(); Unnecessary? https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:372: "0.12.21.tree.dns.example.com", net::Error::ERR_FAILED)); Document why 0.12.21 is meaningful here. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:398: ClearDnsConfig(); Unnecessary? https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:404: "0.0.2.tree.dns.example.com", audit_proof.begin(), Document why 0.0.2 is meaningful here https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:431: // in the order of their timestamps). Follow markdown style indents * Add a ... in the order of their timestamps. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:436: // not audited. Is it wholly necessary to test this, as large as you've done? It doesn't seem like it's testing the interface contract, or at best, that it's testing multiple aspects of the interface contract. That seems to make it much more complex to read, parse, and understand - and I'm curious if you can simplify this. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:442: net::ct::GetSampleSignedTreeHead(&new_sth); You removed other net::ct:: qualifications, but added it here (e.g. see line 187) https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:478: ClearDnsConfig(); This is the only one that seems 'necessary' (e.g. it happens elsewhere), but it could entirely be an incorrect read based on the surrounding code (which looks like, on quick visual scan, only to be setting up temporary variables) https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:538: ClearDnsConfig(); Unnecessary? https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:578: // Pump the message loop. It's obvious from line 579 that the message loop will be pumped - and thus, as a comment, is unnecessary (which you seem to have omitted from line 420) What's not obvious is why. :) https://codereview.chromium.org/2017563002/diff/340001/components/certificate... File components/certificate_transparency/tree_state_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/tree_state_tracker.cc:33: base::FEATURE_DISABLED_BY_DEFAULT}; https://www.chromium.org/developers/coding-style/cpp-dos-and-donts?pli=1#TOC-... "Use the C++11 uniform init syntax ({} without =) only when neither of the above work. https://codereview.chromium.org/2017563002/diff/340001/net/cert/merkle_audit_... File net/cert/merkle_audit_proof.cc (right): https://codereview.chromium.org/2017563002/diff/340001/net/cert/merkle_audit_... net/cert/merkle_audit_proof.cc:34: : leaf_index(other.leaf_index), nodes(other.nodes) {} Why do you not need to copy tree size?
The CQ bit was checked by eranm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the thorough review, Ryan, PTAL. I acknowledge the test is not the most elegant one, mostly due to the need to fake DNS replies a few layers down. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:100: CHECK_EQ(leaf_hash_str.size(), crypto::kSHA256Length); On 2016/12/14 01:34:15, Ryan Sleevi wrote: > Is this CHECK necessary? It seems like you're defending against > HashMerkleTreeLeaf violating its API contract ( > https://cs.chromium.org/chromium/src/net/cert/merkle_tree_leaf.h?rcl=0&l=59 ) - > a DCHECK or just assuming/expecting "API works as documented" seems fine. Switched to DCHECK. > > (FWIW, when looking deeper at the API, it does raise whether HashMerkleTreeLeaf > should populate a SHA256HashValue, which would allow for use of > crypto::SHA256HashBytes and avoid the memcpies - from SHA-256 to string, and > from string to here) I couldn't find SHA256HashBytes, only SHA256HashString in crypto/sha2.h. Another way I've seen for calculating SHA-256 straight into SHA256HashValue is using crypto::SecureHash, but then a new SecureHash object has to be created, I'm not sure the trade-off is justified. It seems like if I'd change the HashMerkleTreeLeaf to use the SHA256HashString variant where an output buffer is specified, it could fill in a SHA256HashValue directly - if that's OK with you, I'll do it in a follow-up CL immediately after this one lands. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:357: crypto::kSHA256Length)); On 2016/12/14 01:34:15, Ryan Sleevi wrote: > Dang, this seems to be non-ideal for performance ;( See the TODO for checked_entries_ in single_tree_tracker.h. I notice you've made it possible to specify a comparator for MRUCache. I'll address the TODO after this CL lands, if that's OK with you - it could be the same CL that changes the HashMerkleTreeLeaf API, so we consistently switch to using SHA256HashValue throughout this class and the code that uses it. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:387: if (net_error == net::OK) { On 2016/12/14 01:34:15, Ryan Sleevi wrote: > if (net_error != net::OK) { > LogInclusionCheckResult(FAILED_GETTING_INCLUSION_PROOF); > pending_entries_.erase(it); > return; > } > > ? Done. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:389: reinterpret_cast<const char*>(it->first.leaf_hash.data), On 2016/12/14 01:34:15, Ryan Sleevi wrote: > Isn't it->first the same as using |entry| ? Yes, done. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... File components/certificate_transparency/single_tree_tracker_unittest.cc (right): https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:99: T... scts) { On 2016/12/14 01:34:15, Ryan Sleevi wrote: > This seems like a lot of... undesirable template overhead. What's legal isn't > always what's desirable - from codegen or readability. > > I'm trying to parse what this function does (could have used more documentation) > and how it's called (the use of var-args == super subtle about number and > ordering of params) Added documentation. I've created this function because I've noticed I repeatedly make assertions about the state of a group of SCTs in the SingleTreeTracker . I use a parameter pack because the group is not held in another container - I could make this function take a list/vector of SCTs and create it each time I call the funciton, if that's preferable? https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:111: base::TimeDelta::FromMilliseconds(INT64_C(1365354256089)); On 2016/12/14 01:34:16, Ryan Sleevi wrote: > Document. Done. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:116: 0x0a, 0x79, 0x0d, 0x9e, 0x40, 0x55, 0xc3, 0xe6}; On 2016/12/14 01:34:16, Ryan Sleevi wrote: > Document. Done. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:121: decoded_root_hash.size()); On 2016/12/14 01:34:15, Ryan Sleevi wrote: > Why is 117-121 necessary? Can't you just copy in directly to the sth? Not necessary, removed. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:131: 0x07, 0xb3, 0x4b, 0xea, 0x5b, 0xdd, 0x81, 0x2d}; On 2016/12/14 01:34:16, Ryan Sleevi wrote: > Document. Done. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:165: ClearDnsConfig(); On 2016/12/14 01:34:16, Ryan Sleevi wrote: > Why is this necessary? Why can't it just be handled by the ctor (see below) If I understand correctly, the question is why isn't there a member of type net::NetworkChangeNotifier that's initialized with net::NetworkChangeNotifier::CreateMock ? Or do you suggest there'd still be a member of type std::unique_ptr<net::NetworkChangeNotifier> but it gets initialized in the c'tor? https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:177: net_change_notifier_.reset(net::NetworkChangeNotifier::CreateMock()); On 2016/12/14 01:34:15, Ryan Sleevi wrote: > Every test creates a new instance, so the first .reset() is unnecessary. If the > dtor for net_change_notifier_ doesn't cleanup global state, that sounds like a > bug? It's true that every test creates a new instance. But in some test cases a different instance is created (with different DNS expectations), so the previous one has to be cleared first. My understanding is that when: net_change_notifier_.reset(net::NetworkChangeNotifier::CreateMock()); is called, first CreateMock is called to create a new object to pass into the reset() method, which will delete the old instance and set the new instance created by CreateMock. However there's a check in the NetworkChangeNotifier that fails when a new NetworkChangeNotifier is created when a global one is already set: https://cs.chromium.org/chromium/src/net/base/network_change_notifier.cc?dr=C... Which is why I have to reset the previous instance - so the global one gets nullified, only then a new NetworkChangeNotifier can be created. I've added comment to that effect. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:180: mock_dns_->InitializeDnsConfig(); On 2016/12/14 01:34:16, Ryan Sleevi wrote: > Do these need to be unique? Can they just be members? Not sure about the question: Are you suggesting it can be a member of type MockLogDnsTraffic ? If so, I'd need a way to either clear the currently-set expectations (to re-use the member object with some expectations already set) or create an instance local to the test method (more boilerplate code in each test method). https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:183: void CreateTreeTracker() { On 2016/12/14 01:34:16, Ryan Sleevi wrote: > Doesn't seem necessary to have this helper function - but am I misreading? Do you mean it seems like this function is not called? It's used in TestEntryNotPendingAfterLeafIndexFetchFailure, for example. More broadly the problem with this test class is that for some tests a simple configuration of DNS expectation is enough (lines 166-169), but in other tests another set of DNS expectations is needed. When a custom set of DNS expectations (and replies) is needed, then they have to be set on mock_dns_ (of type MockLogDnsTraffic), which *then* is used to create a DnsClient which will have these expectations/return the specified replies (and as you can see in CreateTreeTracker a log_dns_client_ is created from the mock_dns_ instance and a SingleTreeTracker created with it). That's why it has to manually be reset every time (the other option being to specify the expected DNS requests / responses in every test case). https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:190: bool ExpectLeafHashRequestAndThrottle( On 2016/12/14 01:34:16, Ryan Sleevi wrote: > Document Done, also renamed to ExpectLeafIndexRequestAndThrottle. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:192: scoped_refptr<SignedCertificateTimestamp> sct) { On 2016/12/14 01:34:16, Ryan Sleevi wrote: > Doesn't seem like you need to be passing these byval - you're not taking > ownership of them. Pass either by naked pointers or const-ref? Done (const-ref rather than naked pointers because then I'd have to call .get() everywhere this method is invoked.). https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:195: ".hash.dns.example." On 2016/12/14 01:34:16, Ryan Sleevi wrote: > Mostly since you're adding it in this test, I think it's non-obvious the > relationship between all of these "hash.dns.example.com" (with their weird line > breaking - is this cl formatt'd?) and line 153 (which actually sets it up). This > seems like the sort of thing you'd express via two constants. Fixed throughout by adding a constant for the prefix (kDNSRequestSuffix) Do you think a constant for the ".hash." portion is also necessary? https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:339: ClearDnsConfig(); On 2016/12/14 01:34:16, Ryan Sleevi wrote: > Unnecessary? Done. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:366: ClearDnsConfig(); On 2016/12/14 01:34:16, Ryan Sleevi wrote: > Unnecessary? Acknowledged, see explanation below why we need to call ClearDnsConfig in every test case where different DNS expectations have to be set. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:372: "0.12.21.tree.dns.example.com", net::Error::ERR_FAILED)); On 2016/12/14 01:34:16, Ryan Sleevi wrote: > Document why 0.12.21 is meaningful here. Done. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:398: ClearDnsConfig(); On 2016/12/14 01:34:16, Ryan Sleevi wrote: > Unnecessary? Acknowledged, see explanation below. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:404: "0.0.2.tree.dns.example.com", audit_proof.begin(), On 2016/12/14 01:34:16, Ryan Sleevi wrote: > Document why 0.0.2 is meaningful here Done. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:431: // in the order of their timestamps). On 2016/12/14 01:34:16, Ryan Sleevi wrote: > Follow markdown style indents > * Add a ... > in the order of their timestamps. Done. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:436: // not audited. On 2016/12/14 01:34:15, Ryan Sleevi wrote: > Is it wholly necessary to test this, as large as you've done? It doesn't seem > like it's testing the interface contract, or at best, that it's testing multiple > aspects of the interface contract. That seems to make it much more complex to > read, parse, and understand - and I'm curious if you can simplify this. I acknowledge this is a large and complex test. And there are other tests that test particular aspects of the interface contract. However, because of the use of a data structure with custom/complex comparator in the code-under-test, I didn't feel comfortable not having that test. Ultimately the goal of tests is to provide confidence in the correctness of the code-under-test, and I wouldn't have enough confidence in that code unless I exercised it in that particular way. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:442: net::ct::GetSampleSignedTreeHead(&new_sth); On 2016/12/14 01:34:15, Ryan Sleevi wrote: > You removed other net::ct:: qualifications, but added it here (e.g. see line > 187) Done. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:478: ClearDnsConfig(); On 2016/12/14 01:34:15, Ryan Sleevi wrote: > This is the only one that seems 'necessary' (e.g. it happens elsewhere), but it > could entirely be an incorrect read based on the surrounding code (which looks > like, on quick visual scan, only to be setting up temporary variables) Trying to clarify the relationship between the mock_dns_, net_change_notifier_, log_dns_client and tree_tracker_: tree_tracker_ takes a LogDnsClient in the c'tor (does not take ownership), which, for tests, has to be configured with expected requests and replies. The MockLogDnsTraffic creates such a LogDnsClient, after the expectations have been set on the MockLogDnsTraffic instance. That instance is not (currently) re-usable. A new one has to be created to clear existing expectations. InitializeDnsConfig has to be called to configure the "global" dns configuration and prevent it from actually sending DNS queries. Since InitializeDnsConfig calls DnsChangeNotifier::SetInitialDnsConfig, which asserts no config previously existed, then the NetworkChangeNotifier has to also be reset and a new one (which did not have any dns config previously) created. That's why every time unique DNS expectations are needed in a test method we have to clear the DNS config. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:538: ClearDnsConfig(); On 2016/12/14 01:34:15, Ryan Sleevi wrote: > Unnecessary? Acknowledged, see above. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:578: // Pump the message loop. On 2016/12/14 01:34:16, Ryan Sleevi wrote: > It's obvious from line 579 that the message loop will be pumped - and thus, as a > comment, is unnecessary (which you seem to have omitted from line 420) > > What's not obvious is why. :) Done - replaced the comment. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... File components/certificate_transparency/tree_state_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/tree_state_tracker.cc:33: base::FEATURE_DISABLED_BY_DEFAULT}; On 2016/12/14 01:34:16, Ryan Sleevi wrote: > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts?pli=1#TOC-... > > "Use the C++11 uniform init syntax ({} without =) only when neither of the above > work. Done (though I note everywhere else the Feature struct is initialized it's using the C++11 uniform init syntax). https://codereview.chromium.org/2017563002/diff/340001/net/cert/merkle_audit_... File net/cert/merkle_audit_proof.cc (right): https://codereview.chromium.org/2017563002/diff/340001/net/cert/merkle_audit_... net/cert/merkle_audit_proof.cc:34: : leaf_index(other.leaf_index), nodes(other.nodes) {} On 2016/12/14 01:34:16, Ryan Sleevi wrote: > Why do you not need to copy tree size? I do - done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by eranm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I've found a way to simplify the test a bit, particularly around setting expectations for DNS queries and DNS configuration. PTAL.
https://codereview.chromium.org/2017563002/diff/340001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:100: CHECK_EQ(leaf_hash_str.size(), crypto::kSHA256Length); On 2016/12/22 16:12:11, Eran Messeri wrote: > I couldn't find SHA256HashBytes, only SHA256HashString in crypto/sha2.h. Sorry, naming was wrong - more relevant was the fact that: 1) It uses a base::StringPiece (avoiding the copy) 2) You can store directly into |output| and |len| (avoiding the other copy) - where output & len come from the SHA256HashValue's ->data & kSHA256Length Since it requires changing HashMerkleTreeLeaf, I'm fine with deferring that optimization from the more substantial CL, although perhaps it may make sense to group up several of the perf optimizations in a single CL, rather than a bunch of 'tiny CL per perf tweak'. Many times, the performance implications of a design aren't obvious until you look at the usage (like here), which is why I only noticed it now. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:357: crypto::kSHA256Length)); On 2016/12/22 16:12:11, Eran Messeri wrote: > I notice you've made it possible to specify a comparator for MRUCache. > I'll address the TODO after this CL lands, if that's OK with you - it could be > the same CL that changes the HashMerkleTreeLeaf API, so we consistently switch > to using SHA256HashValue throughout this class and the code that uses it. Considering you're introducing this code in this CL, I'd prefer you address it in this CL. That's why I landed the change. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... File components/certificate_transparency/single_tree_tracker_unittest.cc (right): https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:165: ClearDnsConfig(); On 2016/12/22 16:12:11, Eran Messeri wrote: > On 2016/12/14 01:34:16, Ryan Sleevi wrote: > > Why is this necessary? Why can't it just be handled by the ctor (see below) > > If I understand correctly, the question is why isn't there a member of type > net::NetworkChangeNotifier that's initialized with > net::NetworkChangeNotifier::CreateMock ? Or do you suggest there'd still be a > member of type std::unique_ptr<net::NetworkChangeNotifier> but it gets > initialized in the c'tor? The latter - initialize the pointer in the ctor, as it doesn't seem to have any dependencies here re: SetUp and, as a subroutine, doesn't trigger any error handling anyways. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:180: mock_dns_->InitializeDnsConfig(); On 2016/12/22 16:12:12, Eran Messeri wrote: > On 2016/12/14 01:34:16, Ryan Sleevi wrote: > > Do these need to be unique? Can they just be members? > > Not sure about the question: Are you suggesting it can be a member of type > MockLogDnsTraffic ? Yes. > If so, I'd need a way to either clear the currently-set expectations (to re-use > the member object with some expectations already set) or create an instance > local to the test method (more boilerplate code in each test method). I'm not sure I fully understand this comment. I tried re-reading through mock_log_dns_traffic again, and even after letting it sit for a few hours, I'm still a bit confused. Mostly, I think MockLogDnsTraffic may be doing too much at once, which is what creates come of the trouble here, but that's very much a 'hot take'. And while I've got some suggestions on how to perhaps make that code easier (I think decoupling the 'produce simulated socket events' from 'manage the DNS config' are probably better things to do here), I more want to make sure I understand your concern here. https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:183: void CreateTreeTracker() { On 2016/12/22 16:12:11, Eran Messeri wrote: > On 2016/12/14 01:34:16, Ryan Sleevi wrote: > > Doesn't seem necessary to have this helper function - but am I misreading? > > Do you mean it seems like this function is not called? It's used in > TestEntryNotPendingAfterLeafIndexFetchFailure, for example. No. I meant more specifically: It does not seem necessary to uplift this to a dedicated function, versus putting it in the specific test itself. It seems to obscure the code under test, rather than help readability - but that's more of a $.02 > That's why it has to manually be reset every time (the other option being to > specify the expected DNS requests / responses in every test case). I feel that's more desirable, on the basis of the above link, because that's literally part of your test :) https://codereview.chromium.org/2017563002/diff/340001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:195: ".hash.dns.example." On 2016/12/22 16:12:11, Eran Messeri wrote: > Do you think a constant for the ".hash." portion is also necessary? No, I think you're fine. https://codereview.chromium.org/2017563002/diff/360001/components/certificate... File components/certificate_transparency/log_dns_client.cc (right): https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/log_dns_client.cc:349: */ Accidental paste? https://codereview.chromium.org/2017563002/diff/360001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:70: GOT_VALID_INCLUSION_PROOF = 0, nit: My inclination is to suggest vertical whitespace in between these variables, to aid readability. I can see an argument against, but especially when reviewing code (e.g. no syntax highlighting), it makes it easier to visually distinguish at a glance. I would argue the same for 32-52 as well. That said, this is not a hill I'm going to die on fighting for, especially in the effort to minimize the delta of this CL, and as you've written it now, it's internally consistent, so that's probably for the best. https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:104: // Audit state of a log entry. Since this represents a finite flow, perhaps it's worth explaining here the logical process for how audit stats progress. I couldn't find any good "high level" overview that explains how auditing works. Alternatively, if you want more robust control of formatting, markdown may be useful. However, I suspect some integrated "overview" documentation about the state machine that an STT embodies may be useful - either around line 29 (to give the general "This is what the file does") or perhaps here ("These are the states and their transitions"). My gut is line 29. I mention this because right now, much of the documentation is associated with the members/variables/types, which is good (yay documentation), but the comprehensive overview of the interconnections and what's going on is less clear - so there's a lot more jumping around code to grok (I think an example of this is 154-162 gets to documenting some of the state machine flow, but it's not obvious without understanding the relationship between both 105-113 and the inline comments in 237-291) I suspect this is *largely* for the .cc file - I think you've got the right level of detail in the .h. To some extent, something like https://cs.chromium.org/chromium/src/base/message_loop/message_pump.h?rcl=0&l=49 is a fairly detailed processing model, but that's more suited for the public interface. The goal here is to explain a bit more about "the lifecycle of an audit request" - so that the implementation itself has a good overview of the states. However, happy to discuss further if you feel differently. Mostly, my goal is: Can someone who has limited knowledge about CT pick up this file and reasonably get an understanding of what's going on? Obviously, further reading is expected, but are the concepts and transitions clear? https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:136: namespace certificate_transparency { note: Nothing wrong with moving this line up to 29/30 https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:137: // The entry that is being audited. Immutable - once created, the entry linebreak between 136/137 https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:138: // to audit does not change. As a class comment, "Immutable" isn't a guarantee you can make about the type - merely about how it's used, and unless someone checks every single EntryToAudit (including its indirection through iterators), it's hard to guarantee. I'd be more inclined to drop the "Immutable" (as a promise) - but happy to explore if you feel it's appropriate to indicate something about "How it should be used" (rather than "How it is used") https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:151: AuditState state; same remarks about newlines for large blocks, especially when transitioning from something like 153-154 https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:153: net::ct::MerkleAuditProof proof; It's a little surprising (and seemingly inconsistent) here about the use of the net::ct:: type here, versus the using declarations above (warning; I'm not a big fan of 'using' in general, but I recognize the style guide permitting both) It just stood out when (re-)reading this code. https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:277: auto entry_to_stop_at = std::lower_bound( nit: another symmetry remark: [first_entry_to_audit, entry_to_stop_at] represent a bounding range, but aren't named in a way that reflects that. A simple way to address this might be "last_entry_to_audit" or go further (and more mirror std::begin()/std::end() style naming) or "auditable_entries_begin" / "auditable_entries_end"... There's probably other ways to word that too, and if it ends up too unnatural, then it may not be worth doing, but I wanted to highlight it in case you had any ideas. https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:315: crypto::kSHA256Length); nit: More copying that we might want to optimize in the future to avoid. https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:373: return SCT_NOT_OBSERVED; Does it make more sense to inverse this conditional, following the "error checking first" argument? auto pending_iterator = pending_entries_.find(entry); if (pending_iterator == pending_entries_.end()) { return SCT_NOT_OBSERVED; } switch (pending_iterator->second.state) { ... } I realize that this may have been written as such due to MSVC complaining about a possible lack of return (that is, I don't think the warnings engine considers the fact that the switch statement would be a complete switch state, if only because it's considering invalid enum values and we don't want to explicitly add a default to the switch), so you'll likely *still* need a return value here (which you could annotate with a NOTREACHED(), like you did on 370), but I think it may make the flow clearer by reducing a level of nesting of complex logic. https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:402: } Similar remarks about error handling first. Does it make sense to bool verified = ct_log_->... pending_entries_.erase(it); if (!verified) { LogInclusionCheckResult(GOT_INVALID_INCLUSION_PROOF); return; } checked_entries_.Put(leaf_hash, EntryAuditResult()); LogInclusionCheckResult(GOT_VALID_INCLUSION_PROOF); https://codereview.chromium.org/2017563002/diff/360001/components/certificate... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/single_tree_tracker.h:123: // an entry can be at. Considering that EntryAuditState is in the .cc file, I suspect we can ditch this NOTE entirely. If you absolutely, positively feel it's necessary, perhaps it's more suitable to being moved to the .cc, per the "Function Definitions" section of https://google.github.io/styleguide/cppguide.html#Function_Comments https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/single_tree_tracker.h:157: // struct is empty. wrapping? https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/single_tree_tracker.h:164: // A DNS client for querying the log. This seems to be more descriptive of "what it is" (e.g. it's a redundant comment) rather than "what it does" - https://google.github.io/styleguide/cppguide.html#Variable_Comments "However, if the type and name suffice (int num_events_;), no comment is needed." (This is a relatively recent-ish update to the style guide - https://github.com/google/styleguide/commit/f15e633de5b716a966504e0652fb0c85c... - so you no longer have to worry about me nagging :P) https://codereview.chromium.org/2017563002/diff/360001/components/certificate... File components/certificate_transparency/single_tree_tracker_unittest.cc (right): https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:201: mock_dns_.reset(new MockLogDnsTraffic); the new hotness (for both of these) is net_change_notifier_ = base::WrapUnique(net::NetworkChangeNotifier::CreateMock()); mock_dns_ = base::MakeUnique<MockLogDnsTraffic>(); https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:209: tree_tracker_.reset(new SingleTreeTracker(log_, log_dns_client_.get())); similarly, it's a little inconsistent that you .reset(new) here, but do MakeUnique on line 206. tree_tracker_ = base::MakeUnique<SingleTreeTracker>(log_, log_dns_client_.get());
https://codereview.chromium.org/2017563002/diff/380001/components/certificate... File components/certificate_transparency/single_tree_tracker_unittest.cc (right): https://codereview.chromium.org/2017563002/diff/380001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:50: base::TimeDelta kMoreThanMMD = base::TimeDelta::FromHours(25); constexpr base::TimeDelta kMoreThanMMD - avoid static initializers :) https://codereview.chromium.org/2017563002/diff/380001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:109: const int sct_list_size = sizeof...(scts); sizeof returns a size_t, not an int. However, you could avoid the initializer-list dependency entirely (on 110) by simply template <size_t N> void AssertSCTsMatchingState(..., SignedCertificateTimestamp* sct_list[N]) { }
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by eranm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Ryan, PTAL - addressed all your comments and made use of the fact that MRUCache can now take a comparator to have the MRUCache in the SingleTreeTracker store SHA256HashValues. There are two API changes needed to get rid of two additional copies between SHA256HashValue to std::string, which will be done in a follow-up CL (as a side note, for RFC6962, it's OK to assume SHA256 throughout, but for 6962-bis the CT log can choose the hash algorithm used). https://codereview.chromium.org/2017563002/diff/360001/components/certificate... File components/certificate_transparency/log_dns_client.cc (right): https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/log_dns_client.cc:349: */ On 2016/12/22 21:33:20, Ryan Sleevi wrote: > Accidental paste? Fixed - bad merge. https://codereview.chromium.org/2017563002/diff/360001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:70: GOT_VALID_INCLUSION_PROOF = 0, On 2016/12/22 21:33:21, Ryan Sleevi wrote: > nit: My inclination is to suggest vertical whitespace in between these > variables, to aid readability. I can see an argument against, but especially > when reviewing code (e.g. no syntax highlighting), it makes it easier to > visually distinguish at a glance. > > I would argue the same for 32-52 as well. > > That said, this is not a hill I'm going to die on fighting for, especially in > the effort to minimize the delta of this CL, and as you've written it now, it's > internally consistent, so that's probably for the best. Done here and lines 32-52. https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:104: // Audit state of a log entry. On 2016/12/22 21:33:21, Ryan Sleevi wrote: > Since this represents a finite flow, perhaps it's worth explaining here the > logical process for how audit stats progress. I couldn't find any good "high > level" overview that explains how auditing works. > > Alternatively, if you want more robust control of formatting, markdown may be > useful. However, I suspect some integrated "overview" documentation about the > state machine that an STT embodies may be useful - either around line 29 (to > give the general "This is what the file does") or perhaps here ("These are the > states and their transitions"). My gut is line 29. > > I mention this because right now, much of the documentation is associated with > the members/variables/types, which is good (yay documentation), but the > comprehensive overview of the interconnections and what's going on is less clear > - so there's a lot more jumping around code to grok (I think an example of this > is 154-162 gets to documenting some of the state machine flow, but it's not > obvious without understanding the relationship between both 105-113 and the > inline comments in 237-291) > > I suspect this is *largely* for the .cc file - I think you've got the right > level of detail in the .h. > > To some extent, something like > https://cs.chromium.org/chromium/src/base/message_loop/message_pump.h?rcl=0&l=49 > is a fairly detailed processing model, but that's more suited for the public > interface. > > The goal here is to explain a bit more about "the lifecycle of an audit request" > - so that the implementation itself has a good overview of the states. > > However, happy to discuss further if you feel differently. Mostly, my goal is: > Can someone who has limited knowledge about CT pick up this file and reasonably > get an understanding of what's going on? Obviously, further reading is expected, > but are the concepts and transitions clear? Done - I've added documentation with an overview of the process to line 29. Please let me know if it needs further refining - obviously it is tied to the SingleTreeTracker's implementation. https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:136: namespace certificate_transparency { On 2016/12/22 21:33:21, Ryan Sleevi wrote: > note: Nothing wrong with moving this line up to 29/30 Done. https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:137: // The entry that is being audited. Immutable - once created, the entry On 2016/12/22 21:33:21, Ryan Sleevi wrote: > linebreak between 136/137 Shouldn't be needed now that the namespace declaration is on line 29, right? https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:138: // to audit does not change. On 2016/12/22 21:33:21, Ryan Sleevi wrote: > As a class comment, "Immutable" isn't a guarantee you can make about the type - > merely about how it's used, and unless someone checks every single EntryToAudit > (including its indirection through iterators), it's hard to guarantee. > > I'd be more inclined to drop the "Immutable" (as a promise) - but happy to > explore if you feel it's appropriate to indicate something about "How it should > be used" (rather than "How it is used") Done - removed the 2nd sentence entirely. https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:151: AuditState state; On 2016/12/22 21:33:21, Ryan Sleevi wrote: > same remarks about newlines for large blocks, especially when transitioning from > something like 153-154 Done. https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:153: net::ct::MerkleAuditProof proof; On 2016/12/22 21:33:21, Ryan Sleevi wrote: > It's a little surprising (and seemingly inconsistent) here about the use of the > net::ct:: type here, versus the using declarations above (warning; I'm not a big > fan of 'using' in general, but I recognize the style guide permitting both) > > It just stood out when (re-)reading this code. Done - added a using declaration at the top, removed the net::ct:: namespace qualification. https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:277: auto entry_to_stop_at = std::lower_bound( On 2016/12/22 21:33:21, Ryan Sleevi wrote: > nit: another symmetry remark: [first_entry_to_audit, entry_to_stop_at] represent > a bounding range, but aren't named in a way that reflects that. A simple way to > address this might be "last_entry_to_audit" or go further (and more mirror > std::begin()/std::end() style naming) or "auditable_entries_begin" / > "auditable_entries_end"... > > There's probably other ways to word that too, and if it ends up too unnatural, > then it may not be worth doing, but I wanted to highlight it in case you had any > ideas. Done - changed to auditable_entries_begin / auditable_entries_end. https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:315: crypto::kSHA256Length); On 2016/12/22 21:33:21, Ryan Sleevi wrote: > nit: More copying that we might want to optimize in the future to avoid. Acknowledged - as mentioned, I'll do a pass to see where copying can be avoided once this CL lands. https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:373: return SCT_NOT_OBSERVED; On 2016/12/22 21:33:21, Ryan Sleevi wrote: > Does it make more sense to inverse this conditional, following the "error > checking first" argument? > > auto pending_iterator = pending_entries_.find(entry); > if (pending_iterator == pending_entries_.end()) { > return SCT_NOT_OBSERVED; > } > > switch (pending_iterator->second.state) { > ... > } > > > I realize that this may have been written as such due to MSVC complaining about > a possible lack of return (that is, I don't think the warnings engine considers > the fact that the switch statement would be a complete switch state, if only > because it's considering invalid enum values and we don't want to explicitly add > a default to the switch), so you'll likely *still* need a return value here > (which you could annotate with a NOTREACHED(), like you did on 370), but I think > it may make the flow clearer by reducing a level of nesting of complex logic. Done (I suspect you have to make this comment a lot because of my preference to have as few return statements as possible, I'll make a note to short-circuit and return early if it simplifies the logic). https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:402: } On 2016/12/22 21:33:20, Ryan Sleevi wrote: > Similar remarks about error handling first. Does it make sense to > > bool verified = ct_log_->... > pending_entries_.erase(it); > > if (!verified) { > LogInclusionCheckResult(GOT_INVALID_INCLUSION_PROOF); > return; > } > > checked_entries_.Put(leaf_hash, EntryAuditResult()); > LogInclusionCheckResult(GOT_VALID_INCLUSION_PROOF); Partly done: Because I've changed checked_entries_ to store net::SHA256HashValue, then entry.leaf_hash must live until it's added to checked_entries_, so the entry cannot be erased beforehand. https://codereview.chromium.org/2017563002/diff/360001/components/certificate... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/single_tree_tracker.h:123: // an entry can be at. On 2016/12/22 21:33:21, Ryan Sleevi wrote: > Considering that EntryAuditState is in the .cc file, I suspect we can ditch this > NOTE entirely. If you absolutely, positively feel it's necessary, perhaps it's > more suitable to being moved to the .cc, per the "Function Definitions" section > of https://google.github.io/styleguide/cppguide.html#Function_Comments Done - removed the NOTE. https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/single_tree_tracker.h:157: // struct is empty. On 2016/12/22 21:33:21, Ryan Sleevi wrote: > wrapping? Done. https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/single_tree_tracker.h:164: // A DNS client for querying the log. On 2016/12/22 21:33:21, Ryan Sleevi wrote: > This seems to be more descriptive of "what it is" (e.g. it's a redundant > comment) rather than "what it does" - > https://google.github.io/styleguide/cppguide.html#Variable_Comments > > "However, if the type and name suffice (int num_events_;), no comment is > needed." > > (This is a relatively recent-ish update to the style guide - > https://github.com/google/styleguide/commit/f15e633de5b716a966504e0652fb0c85c... > - so you no longer have to worry about me nagging :P) Done - per your recommendation & the recent update to the style guide, removed the comment. https://codereview.chromium.org/2017563002/diff/360001/components/certificate... File components/certificate_transparency/single_tree_tracker_unittest.cc (right): https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:201: mock_dns_.reset(new MockLogDnsTraffic); On 2016/12/22 21:33:22, Ryan Sleevi wrote: > the new hotness (for both of these) is > > net_change_notifier_ = > base::WrapUnique(net::NetworkChangeNotifier::CreateMock()); > mock_dns_ = base::MakeUnique<MockLogDnsTraffic>(); Done, throughout the file. https://codereview.chromium.org/2017563002/diff/360001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:209: tree_tracker_.reset(new SingleTreeTracker(log_, log_dns_client_.get())); On 2016/12/22 21:33:22, Ryan Sleevi wrote: > similarly, it's a little inconsistent that you .reset(new) here, but do > MakeUnique on line 206. > > tree_tracker_ = base::MakeUnique<SingleTreeTracker>(log_, > log_dns_client_.get()); Done. https://codereview.chromium.org/2017563002/diff/380001/components/certificate... File components/certificate_transparency/single_tree_tracker_unittest.cc (right): https://codereview.chromium.org/2017563002/diff/380001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:50: base::TimeDelta kMoreThanMMD = base::TimeDelta::FromHours(25); On 2016/12/22 21:50:28, Ryan Sleevi wrote: > constexpr base::TimeDelta kMoreThanMMD - avoid static initializers :) Done. https://codereview.chromium.org/2017563002/diff/380001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:109: const int sct_list_size = sizeof...(scts); On 2016/12/22 21:50:28, Ryan Sleevi wrote: > sizeof returns a size_t, not an int. > > However, you could avoid the initializer-list dependency entirely (on 110) by > simply > > template <size_t N> > void AssertSCTsMatchingState(..., > SignedCertificateTimestamp* sct_list[N]) { > } > I tried, but couldn't get it to work, for two reasons: (1) When the type of the array is templated on the size, I get the following compiler error: error: invalid range expression of type 'SignedCertificateTimestamp **'; no viable 'begin' function available. That can be worked around by passing in the array by reference. (2) I couldn't find a way to create an array of fixed size as rvalue - may be missing something obvious here, was on the plane while doing that.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
rsleevi@chromium.org changed reviewers: + eroman@chromium.org
I have a specific question for Eric below (Eric, see "eroman:") - feel free to remove yourself after if you don't want to get CC'd Other than that, shaping up, a few more suggestions. I need to revisit the tests, but I'd like to suggest one more set of changes prior to really deep-diving that code. https://codereview.chromium.org/2017563002/diff/380001/components/certificate... File components/certificate_transparency/single_tree_tracker_unittest.cc (right): https://codereview.chromium.org/2017563002/diff/380001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:109: const int sct_list_size = sizeof...(scts); On 2017/01/03 23:07:41, Eran Messeri wrote: > On 2016/12/22 21:50:28, Ryan Sleevi wrote: > > sizeof returns a size_t, not an int. > > > > However, you could avoid the initializer-list dependency entirely (on 110) by > > simply > > > > template <size_t N> > > void AssertSCTsMatchingState(..., > > SignedCertificateTimestamp* sct_list[N]) { > > } > > > > I tried, but couldn't get it to work, for two reasons: > (1) When the type of the array is templated on the size, I get the following > compiler error: > error: invalid range expression of type 'SignedCertificateTimestamp **'; no > viable 'begin' function available. > That can be worked around by passing in the array by reference. > (2) I couldn't find a way to create an array of fixed size as rvalue - may be > missing something obvious here, was on the plane while doing that. template <size_t N> void AssertSCTsMatchingState(..., SignedCertificateTimestamp* (&sct_list)[N]) { for (const auto& sct : scts) // Should bind to "template <class T, std::size_t N> T* begin(T (&array)[N]); for (size_t = 0; i < N; ++i) // Should also work } https://codereview.chromium.org/2017563002/diff/380001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:526: AssertSCTsMatchingState<SignedCertificateTimestamp*>( Does it make sense to continue the test if this precondition is violated? Seems like it doesn't - and it seems like this should be wrapped in something like ASSERT_NO_FATAL_TEST_FAILURES that said, I think I'd be much happier if this was in-source simply expanded to for (SignedCertificateTimestamp* sct : { oldest_sct.get(), not_auditable_by_old_sth_sct.get(), ... } ) { ASSERT_EQ(status, tree_tracker->GetLogEntryInclusionStatus(chain, sct)) << "SCT age: " << sct->timestamp; } (and similarly repeated for every place you're calling this function) https://codereview.chromium.org/2017563002/diff/400001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:48: // successfully requested (i.e. it has not been throttled), it moves to the e.g. not i.e. https://codereview.chromium.org/2017563002/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:52: // |pending_entries_|. If the inclusion check has been successful, success what is "success indication"? https://codereview.chromium.org/2017563002/diff/400001/components/certificate... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/2017563002/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker.h:116: // the newest SCT timestamp grammar nit: missing a full-stop here. // Less-than comparator that sorts EntryToAudits based on the SCT // timestamp, with smaller (older) SCTs appearing less than larger (newer) SCTs. https://codereview.chromium.org/2017563002/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker.h:126: // operates on an |entry| rather than cert, SCT combination. Grammatically, there's an article missing here (unclear if it's "a" or "the" or requiring a reword)... "rather than the combination of cert and SCT" (for example) From a "read the header", it's ambiguous how this relates to GetLogEntryInclusionStatus, because EntryToAudit is an opaque struct, so the tuple-comparison is... well, it's not clear how "cert, SCT combination" matches "EntryToAudit" Perhaps just describe what this does? (Note: If you're going to keep the reference to GetLogEntryInclusionStatus in the comment, include the ()" https://codereview.chromium.org/2017563002/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker.h:129: // Invoked by the LogDnsClient once an audit proof request was completed. This line describes how it is used, not what it does - and is thus superfluous. https://codereview.chromium.org/2017563002/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker.h:130: // Verifies the audit proof and updates the state of the entry accordingly: the audit proof of what? ("of |entry|") What is "the state of the entry"? // Processes the result of obtaining an audit proof for |entry|. // * If an audit proof was successfully obtained and validated, // updates |checked_entries_| so that future calls to // GetLogEntryInclusionStatus() will indicate the entry's // inclusion. // * If there was a failure to obtain or validate an inclusion // proof, removes |entry| from the queue of entries to validate. // Future calls to GetLogEntryInclusionStatus() will force this // entry to be re-checked. (Is that correct?) https://codereview.chromium.org/2017563002/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker.h:139: // Clear entries on low memory notifications callback. // Clears entries to reduce memory overhead. Explanation: What does "callback" mean here in this context? Are you using it a verb or a noun? Is Clear passive or active voice? https://codereview.chromium.org/2017563002/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker.h:149: // Map of pending log entries to their state. "Map of" is redundant here From a header documentation: |pending_entries_| also tracks in-progress entries, right? Entries waiting to be checked or being checked? https://codereview.chromium.org/2017563002/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker.h:155: // EntryAuditResult struct is empty. I'm not sure how to parse this comment. Either a) It does, and on failure, the EntryAuditResult struct is filled b) It doesn't, in which case, the hash value wouldn't be found https://codereview.chromium.org/2017563002/diff/400001/components/certificate... File components/certificate_transparency/tree_state_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/400001/components/certificate... components/certificate_transparency/tree_state_tracker.cc:40: net::NetLogWithSource net_log; I'm a little squicky about this and whether it's OK. eroman: Can you offer thoughts on this?
https://codereview.chromium.org/2017563002/diff/400001/components/certificate... File components/certificate_transparency/tree_state_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/400001/components/certificate... components/certificate_transparency/tree_state_tracker.cc:40: net::NetLogWithSource net_log; On 2017/01/10 03:15:54, Ryan Sleevi wrote: > I'm a little squicky about this and whether it's OK. > > eroman: Can you offer thoughts on this? This is technically a supported pattern, in that it has the effect of passing a null NetLog dependency. The consequence is that any NetLog events emitted by this DnsClient and LogDnsClient are sent to oblivion. This is not generally something we want to do. I haven't read the surrounding code to determine whether in this instance this is reasonable. Without knowing more, my recommendation is to forward the NetLog dependency so this DNS activity shows up in our ChromeNetLog, or justify why we don't need to log it. Cheers.
The CQ bit was checked by eranm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Ryan, PTAL. It'd help to get an overview of what are the next steps for this CL and what's needed before it's ready to be submitted. The design hasn't changed since we documented it last and we can't make progress with any other aspect of log auditing until we land this and get some telemetry. I'll get chrome-privacy to flip the privacy bit on the launch bug if that's a concern. https://codereview.chromium.org/2017563002/diff/400001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:48: // successfully requested (i.e. it has not been throttled), it moves to the On 2017/01/10 03:15:53, Ryan Sleevi wrote: > e.g. not i.e. Done. https://codereview.chromium.org/2017563002/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:52: // |pending_entries_|. If the inclusion check has been successful, success On 2017/01/10 03:15:53, Ryan Sleevi wrote: > what is "success indication"? Re-worded to say that if the inclusion check has been successful, the entry is added to |checked_entries_|. https://codereview.chromium.org/2017563002/diff/400001/components/certificate... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/2017563002/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker.h:116: // the newest SCT timestamp On 2017/01/10 03:15:53, Ryan Sleevi wrote: > grammar nit: missing a full-stop here. > > // Less-than comparator that sorts EntryToAudits based on the SCT > // timestamp, with smaller (older) SCTs appearing less than larger (newer) SCTs. Done. https://codereview.chromium.org/2017563002/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker.h:126: // operates on an |entry| rather than cert, SCT combination. On 2017/01/10 03:15:53, Ryan Sleevi wrote: > Grammatically, there's an article missing here (unclear if it's "a" or "the" or > requiring a reword)... "rather than the combination of cert and SCT" (for > example) > > From a "read the header", it's ambiguous how this relates to > GetLogEntryInclusionStatus, because EntryToAudit is an opaque struct, so the > tuple-comparison is... well, it's not clear how "cert, SCT combination" matches > "EntryToAudit" > > Perhaps just describe what this does? > > (Note: If you're going to keep the reference to GetLogEntryInclusionStatus in > the comment, include the ()" Done, PTAL - I've simply explained what it does and ended up referring to GetLogEntryInclusionStatus(). https://codereview.chromium.org/2017563002/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker.h:129: // Invoked by the LogDnsClient once an audit proof request was completed. On 2017/01/10 03:15:53, Ryan Sleevi wrote: > This line describes how it is used, not what it does - and is thus superfluous. Removed. https://codereview.chromium.org/2017563002/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker.h:130: // Verifies the audit proof and updates the state of the entry accordingly: On 2017/01/10 03:15:53, Ryan Sleevi wrote: > the audit proof of what? ("of |entry|") > > What is "the state of the entry"? > > // Processes the result of obtaining an audit proof for |entry|. > // * If an audit proof was successfully obtained and validated, > // updates |checked_entries_| so that future calls to > // GetLogEntryInclusionStatus() will indicate the entry's > // inclusion. > // * If there was a failure to obtain or validate an inclusion > // proof, removes |entry| from the queue of entries to validate. > // Future calls to GetLogEntryInclusionStatus() will force this > // entry to be re-checked. > > > (Is that correct?) Replaced my text with yours. It's mostly correct - if there was failure to obtain or validate an inclusion proof then future calls to GetLogEntryInclusionStatus will indicate this entry has not been observed (as we don't have a negative cache, only positive one). https://codereview.chromium.org/2017563002/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker.h:139: // Clear entries on low memory notifications callback. On 2017/01/10 03:15:53, Ryan Sleevi wrote: > // Clears entries to reduce memory overhead. Done. > > Explanation: What does "callback" mean here in this context? Are you using it a > verb or a noun? Is Clear passive or active voice? Should have been more explicit here and indicated this is the callback function provided to the MemoryPressureListener. https://codereview.chromium.org/2017563002/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker.h:149: // Map of pending log entries to their state. On 2017/01/10 03:15:53, Ryan Sleevi wrote: > "Map of" is redundant here > > From a header documentation: |pending_entries_| also tracks in-progress entries, > right? Entries waiting to be checked or being checked? Correct - all entries in this map are waiting to be checked for inclusion or being checked for inclusion (there are in-progress DNS queries for getting the inclusion proof). I've changed the documentation accordingly. https://codereview.chromium.org/2017563002/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker.h:155: // EntryAuditResult struct is empty. On 2017/01/10 03:15:53, Ryan Sleevi wrote: > I'm not sure how to parse this comment. > > Either > a) It does, and on failure, the EntryAuditResult struct is filled > b) It doesn't, in which case, the hash value wouldn't be found I've clarified that currently the presence of an entry in |checked_entries_| indicates successful inclusion check, and that in order to expand this field to cache failures, the EntryAuditResult struct has to include a success indicator. Is that better? https://codereview.chromium.org/2017563002/diff/400001/components/certificate... File components/certificate_transparency/tree_state_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/400001/components/certificate... components/certificate_transparency/tree_state_tracker.cc:40: net::NetLogWithSource net_log; I can fix it the following way: * Pass in a NetLog from the IOThread when creating the TreeStateTracker: In https://cs.chromium.org/chromium/src/chrome/browser/io_thread.cc?q=TreeStateT... , where the TreeStateTracker is created, there's already an initialized ChromeNetLog instance (net_log_). So a NetLog instance would be passed into the TreeStateTracker c'tor. Here, a NetLogWithSource will be created from the NetLog instance (I would add a new source type in net_log_source_type_list.h, to indicate all dns queries are related to CT inclusion proof fetching). Would that address your concern, Ryan? (I'd rather do it in a follow-up CL to avoid increasing the scope of this CL, since such a change would touch more net/ code, the io_thread and profile_io_data).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2017563002/diff/400001/components/certificate... File components/certificate_transparency/tree_state_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/400001/components/certificate... components/certificate_transparency/tree_state_tracker.cc:40: net::NetLogWithSource net_log; On 2017/01/17 11:37:57, Eran Messeri wrote: > I can fix it the following way: > * Pass in a NetLog from the IOThread when creating the TreeStateTracker: > In > https://cs.chromium.org/chromium/src/chrome/browser/io_thread.cc?q=TreeStateT... > , where the TreeStateTracker is created, there's already an initialized > ChromeNetLog instance (net_log_). > > So a NetLog instance would be passed into the TreeStateTracker c'tor. > Here, a NetLogWithSource will be created from the NetLog instance (I would add a > new source type in net_log_source_type_list.h, to indicate all dns queries are > related to CT inclusion proof fetching). > > Would that address your concern, Ryan? > (I'd rather do it in a follow-up CL to avoid increasing the scope of this CL, > since such a change would touch more net/ code, the io_thread and > profile_io_data). That works for me. Stick a TODO by this line and you can follow-up with me separately.
https://codereview.chromium.org/2017563002/diff/400001/components/certificate... File components/certificate_transparency/tree_state_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/400001/components/certificate... components/certificate_transparency/tree_state_tracker.cc:40: net::NetLogWithSource net_log; On 2017/01/20 00:03:49, eroman (slow) wrote: > On 2017/01/17 11:37:57, Eran Messeri wrote: > > I can fix it the following way: > > * Pass in a NetLog from the IOThread when creating the TreeStateTracker: > > In > > > https://cs.chromium.org/chromium/src/chrome/browser/io_thread.cc?q=TreeStateT... > > , where the TreeStateTracker is created, there's already an initialized > > ChromeNetLog instance (net_log_). > > > > So a NetLog instance would be passed into the TreeStateTracker c'tor. > > Here, a NetLogWithSource will be created from the NetLog instance (I would add > a > > new source type in net_log_source_type_list.h, to indicate all dns queries are > > related to CT inclusion proof fetching). > > > > Would that address your concern, Ryan? > > (I'd rather do it in a follow-up CL to avoid increasing the scope of this CL, > > since such a change would touch more net/ code, the io_thread and > > profile_io_data). > > That works for me. > Stick a TODO by this line and you can follow-up with me separately. Done.
The CQ bit was checked by eranm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LG % one remaining issue from the previous round. I'm really not a fan of the template approach :) https://codereview.chromium.org/2017563002/diff/460001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/460001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:323: ProcessPendingEntries(); If no entries were updated (auditable_entries_begin == auditable_entries_end), do you still want to call processPendingEntries? I suspect it would end up busy-looping through pending_entries_ and noop, so perhaps we want to short-circuit? https://codereview.chromium.org/2017563002/diff/460001/components/certificate... File components/certificate_transparency/single_tree_tracker_unittest.cc (right): https://codereview.chromium.org/2017563002/diff/460001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:106: net::X509Certificate* chain, Did you see the previous remarks ( https://codereview.chromium.org/2017563002/diff/380001/components/certificate... ) ? https://codereview.chromium.org/2017563002/diff/460001/components/certificate... File components/certificate_transparency/tree_state_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/460001/components/certificate... components/certificate_transparency/tree_state_tracker.cc:40: //TODO(eranm): Hook up a real NetLog this way: // TODO(eranm): (add space) https://codereview.chromium.org/2017563002/diff/460001/components/certificate... components/certificate_transparency/tree_state_tracker.cc:49: // NetLogWithSource::Make. Let's drop this comment, if anything because it's a layering violation (talking about //chrome) :) But also because I know you're going to do it right away ;) https://codereview.chromium.org/2017563002/diff/460001/net/cert/merkle_audit_... File net/cert/merkle_audit_proof.cc (right): https://codereview.chromium.org/2017563002/diff/460001/net/cert/merkle_audit_... net/cert/merkle_audit_proof.cc:36: nodes(other.nodes) {} Why not just = default?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ryan, PTAL. https://codereview.chromium.org/2017563002/diff/380001/components/certificate... File components/certificate_transparency/single_tree_tracker_unittest.cc (right): https://codereview.chromium.org/2017563002/diff/380001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:109: const int sct_list_size = sizeof...(scts); On 2017/01/10 03:15:53, Ryan Sleevi (OOO) wrote: > On 2017/01/03 23:07:41, Eran Messeri wrote: > > On 2016/12/22 21:50:28, Ryan Sleevi wrote: > > > sizeof returns a size_t, not an int. > > > > > > However, you could avoid the initializer-list dependency entirely (on 110) > by > > > simply > > > > > > template <size_t N> > > > void AssertSCTsMatchingState(..., > > > SignedCertificateTimestamp* sct_list[N]) { > > > } > > > > > > > I tried, but couldn't get it to work, for two reasons: > > (1) When the type of the array is templated on the size, I get the following > > compiler error: > > error: invalid range expression of type 'SignedCertificateTimestamp **'; no > > viable 'begin' function available. > > That can be worked around by passing in the array by reference. > > (2) I couldn't find a way to create an array of fixed size as rvalue - may be > > missing something obvious here, was on the plane while doing that. > > template <size_t N> > void AssertSCTsMatchingState(..., > SignedCertificateTimestamp* (&sct_list)[N]) { > for (const auto& sct : scts) > // Should bind to "template <class T, std::size_t N> T* begin(T (&array)[N]); > > for (size_t = 0; i < N; ++i) > // Should also work > } N/A anymore. https://codereview.chromium.org/2017563002/diff/380001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:526: AssertSCTsMatchingState<SignedCertificateTimestamp*>( On 2017/01/10 03:15:53, Ryan Sleevi (OOO) wrote: > Does it make sense to continue the test if this precondition is violated? Seems > like it doesn't - and it seems like this should be wrapped in something like > > ASSERT_NO_FATAL_TEST_FAILURES > > that said, I think I'd be much happier if this was in-source simply expanded to > > for (SignedCertificateTimestamp* sct : { oldest_sct.get(), > not_auditable_by_old_sth_sct.get(), ... } ) { > ASSERT_EQ(status, tree_tracker->GetLogEntryInclusionStatus(chain, sct)) << > "SCT age: " << sct->timestamp; > } > > (and similarly repeated for every place you're calling this function) Done. https://codereview.chromium.org/2017563002/diff/460001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/460001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:323: ProcessPendingEntries(); On 2017/01/20 12:53:21, Ryan Sleevi (OOO) wrote: > If no entries were updated (auditable_entries_begin == auditable_entries_end), > do you still want to call processPendingEntries? > > I suspect it would end up busy-looping through pending_entries_ and noop, so > perhaps we want to short-circuit? Done. https://codereview.chromium.org/2017563002/diff/460001/components/certificate... File components/certificate_transparency/single_tree_tracker_unittest.cc (right): https://codereview.chromium.org/2017563002/diff/460001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:106: net::X509Certificate* chain, On 2017/01/20 12:53:21, Ryan Sleevi (OOO) wrote: > Did you see the previous remarks ( > https://codereview.chromium.org/2017563002/diff/380001/components/certificate... > ) ? My bad, I've missed it, changed to your suggestion from that comment. https://codereview.chromium.org/2017563002/diff/460001/components/certificate... File components/certificate_transparency/tree_state_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/460001/components/certificate... components/certificate_transparency/tree_state_tracker.cc:40: //TODO(eranm): Hook up a real NetLog this way: On 2017/01/20 12:53:21, Ryan Sleevi (OOO) wrote: > // TODO(eranm): > > (add space) Done. https://codereview.chromium.org/2017563002/diff/460001/components/certificate... components/certificate_transparency/tree_state_tracker.cc:49: // NetLogWithSource::Make. On 2017/01/20 12:53:21, Ryan Sleevi (OOO) wrote: > Let's drop this comment, if anything because it's a layering violation (talking > about //chrome) :) > > But also because I know you're going to do it right away ;) Done. https://codereview.chromium.org/2017563002/diff/460001/net/cert/merkle_audit_... File net/cert/merkle_audit_proof.cc (right): https://codereview.chromium.org/2017563002/diff/460001/net/cert/merkle_audit_... net/cert/merkle_audit_proof.cc:36: nodes(other.nodes) {} On 2017/01/20 12:53:21, Ryan Sleevi (OOO) wrote: > Why not just = default? Done (may not have been possible to use = default when I started working on this CL :))
The CQ bit was checked by eranm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2017563002/diff/480001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/480001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:323: if (auditable_entries_begin != auditable_entries_end) { nit: (only because you mentioned it's not your default way of thinking) if (auditable_entries_begin == auditable_entries_end) return ProcessPendingEntries(); https://codereview.chromium.org/2017563002/diff/480001/components/certificate... File components/certificate_transparency/single_tree_tracker_unittest.cc (right): https://codereview.chromium.org/2017563002/diff/480001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:516: newer_than_new_sth_sct.get()}) { FWIW, I think you could do for (const auto& sct : { oldest_sct, non_auditable, ... etc }) { } here I don't have a strong preference either way, but I just mention it as a way to elide the .get() calls from the pointer. I'm not sure if that syntax would work though (that is, whether it requires the initializer list have an explicit type by way of the range-for initializer), but just a thought.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
eranm@chromium.org changed reviewers: + holte@chromium.org
Steven, please review the histogram.xml changes. https://codereview.chromium.org/2017563002/diff/480001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/2017563002/diff/480001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:323: if (auditable_entries_begin != auditable_entries_end) { On 2017/01/23 17:37:31, Ryan Sleevi (OOO) wrote: > nit: (only because you mentioned it's not your default way of thinking) > > if (auditable_entries_begin == auditable_entries_end) > return > > ProcessPendingEntries(); Done. https://codereview.chromium.org/2017563002/diff/480001/components/certificate... File components/certificate_transparency/single_tree_tracker_unittest.cc (right): https://codereview.chromium.org/2017563002/diff/480001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:516: newer_than_new_sth_sct.get()}) { On 2017/01/23 17:37:31, Ryan Sleevi (OOO) wrote: > FWIW, I think you could do > > for (const auto& sct : { oldest_sct, non_auditable, ... etc }) { > } > > here > > I don't have a strong preference either way, but I just mention it as a way to > elide the .get() calls from the pointer. I'm not sure if that syntax would work > though (that is, whether it requires the initializer list have an explicit type > by way of the range-for initializer), but just a thought. Done (throughout)
The CQ bit was checked by eranm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
histograms lgtm
The CQ bit was unchecked by eranm@chromium.org
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/2017563002/#ps500001 (title: "Addressing Ryan's two final comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 500001, "attempt_start_ts": 1485207182018430,
"parent_rev": "73d9d4ac7b327770e2b8a4d8782abe5a62a2c91c", "commit_rev":
"1b5a833bca72a9f89e8c6e8c592037540eaf1411"}
Message was sent while issue was closed.
Description was changed from ========== Audit CT logs by requesting inclusion proofs for observed Signed Certificate Timestamps from the CT log that issued them. To verify that a CT log behaves correctly and indeed publishes all the certificates it committed to publishing, it is necessary to verify that each observed log entry (as denoted by an SCT and the corresponding certificate) is included in the log. Check for inclusion of observed SCTs by finding out the leaf index of each observed SCT and requesting an inclusion proof from the CT log for it over DNS (using the LogDnsClient). Note that no action is taken based on the inclusion check results in this change. Follow-up changes will add telemetry. BUG=613495 ========== to ========== Audit CT logs by requesting inclusion proofs for observed Signed Certificate Timestamps from the CT log that issued them. To verify that a CT log behaves correctly and indeed publishes all the certificates it committed to publishing, it is necessary to verify that each observed log entry (as denoted by an SCT and the corresponding certificate) is included in the log. Check for inclusion of observed SCTs by finding out the leaf index of each observed SCT and requesting an inclusion proof from the CT log for it over DNS (using the LogDnsClient). Note that no action is taken based on the inclusion check results in this change. Follow-up changes will add telemetry. BUG=613495 Review-Url: https://codereview.chromium.org/2017563002 Cr-Commit-Position: refs/heads/master@{#445513} Committed: https://chromium.googlesource.com/chromium/src/+/1b5a833bca72a9f89e8c6e8c5920... ==========
Message was sent while issue was closed.
Committed patchset #26 (id:500001) as https://chromium.googlesource.com/chromium/src/+/1b5a833bca72a9f89e8c6e8c5920... |
