Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1845113003:
Certificate Transparency: Start tracking logs' state (Closed)
|
Created:
4 years, 8 months ago by Eran Messeri Modified:
4 years, 7 months ago CC:
chromium-reviews, certificate-transparency-chrome_googlegroups.com, cbentzel+watch_chromium.org, droger+watchlist_chromium.org, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, waffles Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCertificate Transparency: Start tracking the logs' STHs.
This introduces the framework for actual inclusion checking of
SCTs, and the certs the SCTs are associated with. This is done by
introducing a new class, the TreeStateTracker, which can receive
notifications of new STHs (which will be provided by the component
updater), as well as notifications of verified STHs (provided by CTLogVerifiers).
Right now, the only logic is tracking the state of observed SCTs to
determine if they can be checked for inclusion or not - that is,
whether or not an STH is available that is newer than the SCT's
timestamp + the logs maximum merge delay. Except for freshly
issued/logged certificates, this is expected to generally be the case.
The next change will gather metrics about this.
BUG=506227
Committed: https://crrev.com/a824b1534a7a52132d1767a789d8cffd603ff20b
Cr-Commit-Position: refs/heads/master@{#395045}
Patch Set 1 #Patch Set 2 : Catching up with master #Patch Set 3 : Getting rid of a redundant class #Patch Set 4 : Rebasing on issue 1853753003 #Patch Set 5 : ChromeOS compilation woes #Patch Set 6 : Removing browser thread include #Patch Set 7 : Correct patchset #Patch Set 8 : Catching up with master #Patch Set 9 : Merging with master #Patch Set 10 : Splitting classes #Patch Set 11 : Testing SingleTreeTracker. #Patch Set 12 : Ready for review #
Total comments: 25
Patch Set 13 : Addressing review comments #
Total comments: 20
Patch Set 14 : Addressing 2nd round of review comments #Patch Set 15 : Catching up with master #
Total comments: 12
Patch Set 16 : Removing plumbing bits #
Total comments: 26
Patch Set 17 : Addressing review comments #
Total comments: 24
Patch Set 18 : #Patch Set 19 : Using std::move as suggested #
Total comments: 1
Patch Set 20 : Fixed passing of scoped_refptr #
Total comments: 26
Patch Set 21 : Review comments, test improvement #
Total comments: 28
Patch Set 22 : Documenting tests, etc #
Total comments: 10
Patch Set 23 : Improving documentation. #Messages
Total messages: 36 (10 generated)
Description was changed from ========== EXPERIMENTAL: Certificate Transparency: STH distribution. This CL is only to demonstrate planned plumbing of STH distribution from the component updater (STHSetComponentInstallerTraits) to TreeStateTracker instances in the profiles via the IOThread. BUG= ========== to ========== EXPERIMENTAL: Certificate Transparency: STH distribution. This CL is only to demonstrate planned plumbing of STH distribution from the component updater (STHSetComponentInstallerTraits) to TreeStateTracker instances in the profiles via the IOThread. BUG= ==========
eranm@chromium.org changed reviewers: + estark@chromium.org
Hey Eran, I took a look at single_tree_tracker.cc to think about how to test it but didn't look at much else; let me know if you want me to review more thoroughly. Testing histograms is pretty straightforward with base::HistogramTester. Here's an example: https://code.google.com/p/chromium/codesearch#chromium/src/net/http/transport... For testing other stuff, here are a couple ideas: - Use FRIEND_TEST_ALL_PREFIXES to friend the tests and let them poke at internal data structures. This is kind of ugly (testing the implementation), but it sounds like this might be a temporary state where you can later transition to testing the interface and drop the friended tests? - Instead of having |pending_timestamps_| be a std::set, could you define an interface, like SingleTreeTracker::PendingTimestampSet, and pass in an implementation of it to the tracker? Then you could mock it out for tests. Do you think either of those would work for you?
I've settled on a different approach where the SingleTreeTracker can be queried about the status of a particular SCT+Cert. This serves two purposes: (1) Allowing tests to validate correct behaviour of the SingleTreeTracker. (2) Exporting that information for later use in the UI - for example the inclusion status of a particular SCT+Cert would be passed from this class back to the Security panel in DevTools. I'll clean up this CL and send it out for review - I believe there's enough functionality here even though there are some big TODOs.
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
Description was changed from ========== EXPERIMENTAL: Certificate Transparency: STH distribution. This CL is only to demonstrate planned plumbing of STH distribution from the component updater (STHSetComponentInstallerTraits) to TreeStateTracker instances in the profiles via the IOThread. BUG= ========== to ========== This change lays the groundwork for actual inclusion checking of observed SCTs (and the certs they belong to). This is done by introducing a new class, TreeStateTracker, that receives notifications of new STHs (provided from the component updater) and new, verified SCTs (from the CTLogVerifier in the IOThread and each ProfileIOData). Right now the only functionality is tracking the state of observed SCTs to figure out if they can already be checked for inclusion or not. The next change will log some UMA on this. BUG=506227 ==========
Ryan, please review this change towards inclusion checking of SCTs. Emily, FYI (if you have the bandwidth feedback is welcome).
https://codereview.chromium.org/1845113003/diff/220001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/1845113003/diff/220001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:18: bool STHIsTooOld(const SignedTreeHead& sth, const base::Time& sct_timestamp) { Doesn't seem like this needs to be a utility function, given it's one use in a single conditional https://codereview.chromium.org/1845113003/diff/220001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:28: : ct_log_(ct_log) {} When you update to be by-value, move https://codereview.chromium.org/1845113003/diff/220001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:60: // component updater. What sort of error is this? Developer error? In which case, DCHECK and don't have conditionals like this. https://codereview.chromium.org/1845113003/diff/220001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:79: const base::Time& timestamp(it->first); Seems unnecessary https://codereview.chromium.org/1845113003/diff/220001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:99: if (log_id != ct_log_->key_id()) { Explain why or when this happens. As far as I can tell, it's developer error, and never legitimate, in which case, drop this method entirely, and DCHECK_EQ(ct_log->key_id(), log_id) https://codereview.chromium.org/1845113003/diff/220001/components/certificate... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/1845113003/diff/220001/components/certificate... components/certificate_transparency/single_tree_tracker.h:39: // SCT was observed but the STH known to this class is not old Readability: Newlines between 38/39, 41/42 https://codereview.chromium.org/1845113003/diff/220001/components/certificate... components/certificate_transparency/single_tree_tracker.h:47: explicit SingleTreeTracker(scoped_refptr<const net::CTLogVerifier>& ct_log); If you're passing by ref, pass it const. But in the case of scoped_refptr<>, pass by value. https://codereview.chromium.org/1845113003/diff/220001/components/certificate... components/certificate_transparency/single_tree_tracker.h:53: // the SCT for future checking later on. Does this do the checking synchronously? CT is already showing up in connection latency costs, and considering this will add to the crypto operations, it may be better to defer this work to the background. https://codereview.chromium.org/1845113003/diff/220001/components/certificate... File components/certificate_transparency/tree_state_tracker.cc (right): https://codereview.chromium.org/1845113003/diff/220001/components/certificate... components/certificate_transparency/tree_state_tracker.cc:22: for (auto it = ct_logs.begin(); it != ct_logs.end(); ++it) { Use a range iterator for (const auto& log : ct_logs) { tree_trackers_[log->key_id()].reset(new SingleTreeTracker(log)); } https://codereview.chromium.org/1845113003/diff/220001/components/certificate... File components/certificate_transparency/tree_state_tracker.h (right): https://codereview.chromium.org/1845113003/diff/220001/components/certificate... components/certificate_transparency/tree_state_tracker.h:36: const std::vector<scoped_refptr<const net::CTLogVerifier>>& ct_logs); Pass by value? https://codereview.chromium.org/1845113003/diff/220001/net/cert/ct_observer.h File net/cert/ct_observer.h (right): https://codereview.chromium.org/1845113003/diff/220001/net/cert/ct_observer.h... net/cert/ct_observer.h:16: class NET_EXPORT CTObserver : public CTVerifier::Observer, public STHObserver { Seems unnecessary. Why?
Thanks for the prompt review, Ryan, PTAL. https://codereview.chromium.org/1845113003/diff/220001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/1845113003/diff/220001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:18: bool STHIsTooOld(const SignedTreeHead& sth, const base::Time& sct_timestamp) { On 2016/04/14 16:56:15, Ryan Sleevi wrote: > Doesn't seem like this needs to be a utility function, given it's one use in a > single conditional Done, removed. https://codereview.chromium.org/1845113003/diff/220001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:28: : ct_log_(ct_log) {} On 2016/04/14 16:56:15, Ryan Sleevi wrote: > When you update to be by-value, move Done. https://codereview.chromium.org/1845113003/diff/220001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:60: // component updater. On 2016/04/14 16:56:15, Ryan Sleevi wrote: > What sort of error is this? Developer error? In which case, DCHECK and don't > have conditionals like this. Operational error? This would only happen if we (Google) pushed an STH that was not verified, I think that a DCHECK here would be inappropriate. I have removed the TODO however as it should not be common occurrence at all and below, in CheckLogID, added a DCHECK. https://codereview.chromium.org/1845113003/diff/220001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:79: const base::Time& timestamp(it->first); On 2016/04/14 16:56:15, Ryan Sleevi wrote: > Seems unnecessary Done - removed. https://codereview.chromium.org/1845113003/diff/220001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:99: if (log_id != ct_log_->key_id()) { On 2016/04/14 16:56:16, Ryan Sleevi wrote: > Explain why or when this happens. > > As far as I can tell, it's developer error, and never legitimate, in which case, > drop this method entirely, and > > DCHECK_EQ(ct_log->key_id(), log_id) You're right - it's a developer error, so changed to DCHECK_EQ. https://codereview.chromium.org/1845113003/diff/220001/components/certificate... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/1845113003/diff/220001/components/certificate... components/certificate_transparency/single_tree_tracker.h:39: // SCT was observed but the STH known to this class is not old On 2016/04/14 16:56:16, Ryan Sleevi wrote: > Readability: Newlines between 38/39, 41/42 Done. https://codereview.chromium.org/1845113003/diff/220001/components/certificate... components/certificate_transparency/single_tree_tracker.h:47: explicit SingleTreeTracker(scoped_refptr<const net::CTLogVerifier>& ct_log); On 2016/04/14 16:56:16, Ryan Sleevi wrote: > If you're passing by ref, pass it const. But in the case of scoped_refptr<>, > pass by value. Done. https://codereview.chromium.org/1845113003/diff/220001/components/certificate... components/certificate_transparency/single_tree_tracker.h:53: // the SCT for future checking later on. On 2016/04/14 16:56:16, Ryan Sleevi wrote: > Does this do the checking synchronously? CT is already showing up in connection > latency costs, and considering this will add to the crypto operations, it may be > better to defer this work to the background. No - it does not do the checking synchronously. Right now nothing is done in this method (other than filing the SCT), I'll add a TODO not to do any synchronous operations here (SCT verification does not need to happen again as this callback is invoked only for SCTs that were verified already) https://codereview.chromium.org/1845113003/diff/220001/components/certificate... File components/certificate_transparency/tree_state_tracker.cc (right): https://codereview.chromium.org/1845113003/diff/220001/components/certificate... components/certificate_transparency/tree_state_tracker.cc:22: for (auto it = ct_logs.begin(); it != ct_logs.end(); ++it) { On 2016/04/14 16:56:16, Ryan Sleevi wrote: > Use a range iterator > > for (const auto& log : ct_logs) { > tree_trackers_[log->key_id()].reset(new SingleTreeTracker(log)); > } Done. https://codereview.chromium.org/1845113003/diff/220001/components/certificate... File components/certificate_transparency/tree_state_tracker.h (right): https://codereview.chromium.org/1845113003/diff/220001/components/certificate... components/certificate_transparency/tree_state_tracker.h:36: const std::vector<scoped_refptr<const net::CTLogVerifier>>& ct_logs); On 2016/04/14 16:56:16, Ryan Sleevi wrote: > Pass by value? Done. https://codereview.chromium.org/1845113003/diff/220001/net/cert/ct_observer.h File net/cert/ct_observer.h (right): https://codereview.chromium.org/1845113003/diff/220001/net/cert/ct_observer.h... net/cert/ct_observer.h:16: class NET_EXPORT CTObserver : public CTVerifier::Observer, public STHObserver { On 2016/04/14 16:56:16, Ryan Sleevi wrote: > Seems unnecessary. Why? To abstract the TreeStateTracker from the IOThread / ProfileIOData. As far as the IOThread/ProfileIOData are concerned, the ct_observer they hold cares about STH & SCT notifications. By creating this interface I avoid having the IOThread/ProfileIOData know about TreeStateTracker and depending on its definition.
Mostly focusing on style & design issues before reviewing the meat of the code. https://codereview.chromium.org/1845113003/diff/220001/net/cert/ct_observer.h File net/cert/ct_observer.h (right): https://codereview.chromium.org/1845113003/diff/220001/net/cert/ct_observer.h... net/cert/ct_observer.h:16: class NET_EXPORT CTObserver : public CTVerifier::Observer, public STHObserver { On 2016/04/18 22:02:12, Eran Messeri wrote: > On 2016/04/14 16:56:16, Ryan Sleevi wrote: > > Seems unnecessary. Why? > > To abstract the TreeStateTracker from the IOThread / ProfileIOData. > As far as the IOThread/ProfileIOData are concerned, the ct_observer they hold > cares about STH & SCT notifications. > By creating this interface I avoid having the IOThread/ProfileIOData know about > TreeStateTracker and depending on its definition. That's not true for IOThread - https://codereview.chromium.org/1845113003/diff/240001/chrome/browser/io_thre... It's not true for ProfileIOData - https://codereview.chromium.org/1845113003/diff/240001/chrome/browser/profile... So what are you trying to accomplish? https://codereview.chromium.org/1845113003/diff/240001/chrome/browser/compone... File chrome/browser/component_updater/sth_set_component_installer.cc (right): https://codereview.chromium.org/1845113003/diff/240001/chrome/browser/compone... chrome/browser/component_updater/sth_set_component_installer.cc:194: DCHECK(g_browser_process->io_thread()); This defeats the abstraction of lines 195-196 (content::BrowserThread::IO) and inappropriately couples it. If you want to DCHECK, then use GetMessageLoopProxyForThread and DCHECK. But that still doesn't guarantee correct results - because you can get the object even if there's no underlying backing thread. If you want correct results, look at the return result of the PostTask. https://codereview.chromium.org/1845113003/diff/240001/chrome/browser/compone... chrome/browser/component_updater/sth_set_component_installer.cc:198: base::Unretained(g_browser_process->io_thread()), g_browser_process has an incredibly high bar to justify its usage. What are you trying to do? https://codereview.chromium.org/1845113003/diff/240001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1845113003/diff/240001/chrome/browser/io_thre... chrome/browser/io_thread.cc:883: for (auto observer : sth_observers_) { const auto& observer See https://chromium-cpp.appspot.com/ ("Range-Based For Loops") https://codereview.chromium.org/1845113003/diff/240001/chrome/browser/io_thre... chrome/browser/io_thread.cc:1422: for (auto observer : sth_observers_) { const auto& https://codereview.chromium.org/1845113003/diff/240001/chrome/browser/profile... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/1845113003/diff/240001/chrome/browser/profile... chrome/browser/profiles/profile_io_data.cc:1155: cert_transparency_observer_.get()); Seems entirely unnecessary to bundle this in a callback - just run this code in line 1308 https://codereview.chromium.org/1845113003/diff/240001/components/certificate... File components/certificate_transparency/OWNERS (right): https://codereview.chromium.org/1845113003/diff/240001/components/certificate... components/certificate_transparency/OWNERS:3: robpercival@chromium.org This should be in a separate CL Please review https://www.chromium.org/developers/owners-files before doing so, as it explains why I'm a bit uncomfortable with this and would want to see evidence demonstrating it. https://codereview.chromium.org/1845113003/diff/240001/components/certificate... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/1845113003/diff/240001/components/certificate... components/certificate_transparency/single_tree_tracker.h:58: void OnSCTVerified(net::X509Certificate* cert, Note: If you ever expect |cert| to be retained, you'll need to change this signature. I'm not sure what your thoughts are for 56/57, but worth calling out now. https://codereview.chromium.org/1845113003/diff/240001/components/certificate... File components/certificate_transparency/tree_state_tracker.cc (right): https://codereview.chromium.org/1845113003/diff/240001/components/certificate... components/certificate_transparency/tree_state_tracker.cc:24: } Omit braces - be consistent with the rest of this file.
Ryan, PTAL. Regarding propagation of data from the ComponentInstallerTraits implementation to the IOThread, if the current reasoning for the way things are done is not satisfactory I propose we follow up over VC and try to come up with the right way of propagating STHs from the component updater. https://codereview.chromium.org/1845113003/diff/220001/net/cert/ct_observer.h File net/cert/ct_observer.h (right): https://codereview.chromium.org/1845113003/diff/220001/net/cert/ct_observer.h... net/cert/ct_observer.h:16: class NET_EXPORT CTObserver : public CTVerifier::Observer, public STHObserver { On 2016/04/21 14:05:15, Ryan Sleevi wrote: > On 2016/04/18 22:02:12, Eran Messeri wrote: > > On 2016/04/14 16:56:16, Ryan Sleevi wrote: > > > Seems unnecessary. Why? > > > > To abstract the TreeStateTracker from the IOThread / ProfileIOData. > > As far as the IOThread/ProfileIOData are concerned, the ct_observer they hold > > cares about STH & SCT notifications. > > By creating this interface I avoid having the IOThread/ProfileIOData know > about > > TreeStateTracker and depending on its definition. > > That's not true for IOThread - > https://codereview.chromium.org/1845113003/diff/240001/chrome/browser/io_thre... > > It's not true for ProfileIOData - > https://codereview.chromium.org/1845113003/diff/240001/chrome/browser/profile... > > So what are you trying to accomplish? The main reasons I've created the CTObserver interface under net/cert rather than directly use the TreeStateTracker class: - Not create a dependency on the components/certificate_transparency code everywhere I have to pass the object that receives and distributes STH notifications (depending on net/cert seems more natural). - Clearly document what's the interface client code should care about - so client code never relies on implementation details of the TreeStateTracker. - Facilitate testing - that interface could be implemented in test code to test correct interaction with the IOThread, for example. If you think all of these reasons are invalid I could remove this interface, but having it used throughout the code helps me reason about the role(s) of the TreeStateTracker. https://codereview.chromium.org/1845113003/diff/240001/chrome/browser/compone... File chrome/browser/component_updater/sth_set_component_installer.cc (right): https://codereview.chromium.org/1845113003/diff/240001/chrome/browser/compone... chrome/browser/component_updater/sth_set_component_installer.cc:194: DCHECK(g_browser_process->io_thread()); On 2016/04/21 14:05:15, Ryan Sleevi wrote: > This defeats the abstraction of lines 195-196 (content::BrowserThread::IO) and > inappropriately couples it. > > If you want to DCHECK, then use GetMessageLoopProxyForThread and DCHECK. But > that still doesn't guarantee correct results - because you can get the object > even if there's no underlying backing thread. > > If you want correct results, look at the return result of the PostTask. Apologies, but I am not sure I understand this comment. It reads to me as if I was (wrongly) checking that the function is called on the IO thread, but what I was attempting to do is check that the IOThread object from the g_browser_process global is not null. Are you suggesting to completely remove this DCHECK, call PostTask (below) and DCHECK on the result? https://codereview.chromium.org/1845113003/diff/240001/chrome/browser/compone... chrome/browser/component_updater/sth_set_component_installer.cc:198: base::Unretained(g_browser_process->io_thread()), On 2016/04/21 14:05:15, Ryan Sleevi wrote: > g_browser_process has an incredibly high bar to justify its usage. What are you > trying to do? I'm trying to share an STHDistributor instance between the class implementing ComponentInstallerTraits and receiving the STH updates and the STHObserver (which lives in the IOThread / ProfileIOData) that needs these STHs. My original plan was to pass this instance into RegisterSTHSetComponent and share it by passing it into the IOThread via BrowserProcessImpl, but sorin@ asked for a less invasive change (see https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/chrome_b...) and suggested the use of the g_browser_process global to pass the object rather than pass it into RegisterSTHSetComponent. Since I'm creating the STHDistributor instance here, I have no way of getting it to the IOThread/STHObserver other than through globals. How do you suggest I get STHs out from this class to objects created on the IOThread/ProfileIOData? https://codereview.chromium.org/1845113003/diff/240001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1845113003/diff/240001/chrome/browser/io_thre... chrome/browser/io_thread.cc:883: for (auto observer : sth_observers_) { On 2016/04/21 14:05:15, Ryan Sleevi wrote: > const auto& observer > > See https://chromium-cpp.appspot.com/ ("Range-Based For Loops") Done. https://codereview.chromium.org/1845113003/diff/240001/chrome/browser/io_thre... chrome/browser/io_thread.cc:1422: for (auto observer : sth_observers_) { On 2016/04/21 14:05:16, Ryan Sleevi wrote: > const auto& Done. https://codereview.chromium.org/1845113003/diff/240001/chrome/browser/profile... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/1845113003/diff/240001/chrome/browser/profile... chrome/browser/profiles/profile_io_data.cc:1155: cert_transparency_observer_.get()); On 2016/04/21 14:05:16, Ryan Sleevi wrote: > Seems entirely unnecessary to bundle this in a callback - just run this code in > line 1308 In ProfileIOData::DestroyResourceContext I no longer have access to the io_thread pointer. The profile_params_, which provides the io_thread pointer (line 1018) is reset at the end of this function. The most scope-reducing mechanism I could think of to retain a pointer to the IOThread is by binding it in a callback - can you see another way? https://codereview.chromium.org/1845113003/diff/240001/components/certificate... File components/certificate_transparency/OWNERS (right): https://codereview.chromium.org/1845113003/diff/240001/components/certificate... components/certificate_transparency/OWNERS:3: robpercival@chromium.org On 2016/04/21 14:05:16, Ryan Sleevi wrote: > This should be in a separate CL > > Please review https://www.chromium.org/developers/owners-files before doing so, > as it explains why I'm a bit uncomfortable with this and would want to see > evidence demonstrating it. Understood, reverted. https://codereview.chromium.org/1845113003/diff/240001/components/certificate... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/1845113003/diff/240001/components/certificate... components/certificate_transparency/single_tree_tracker.h:58: void OnSCTVerified(net::X509Certificate* cert, On 2016/04/21 14:05:16, Ryan Sleevi wrote: > Note: If you ever expect |cert| to be retained, you'll need to change this > signature. I'm not sure what your thoughts are for 56/57, but worth calling out > now. Acknowledged - I don't yet know if the |cert| will be retained or portions copied to address the TODO in lines 82-83. Is there anywhere I can read on the life-cycle of X509Certificate objects? Ideally I could just hold onto the pointer for calculating the leaf hash asynchronously (i.e. after OnSCTVerified) was called. https://codereview.chromium.org/1845113003/diff/240001/components/certificate... File components/certificate_transparency/tree_state_tracker.cc (right): https://codereview.chromium.org/1845113003/diff/240001/components/certificate... components/certificate_transparency/tree_state_tracker.cc:24: } On 2016/04/21 14:05:16, Ryan Sleevi wrote: > Omit braces - be consistent with the rest of this file. Done.
Concrete suggestion: Let's separate out the implementation of the tree state tracker and notification bits from the 'glue it up' bits. The 'glue it up' bits are the ones I'm having trouble with, and I'd rather allow you to get some forward progress on the tree state trackers (which, because of the issues with the glue it up bits, I haven't really reviewed yet) Any time there's "glue it up" bits, there's almost always questions about lifetime and threading, so it's usually good to separate those out (is my gut) I'm still trying to wrap my head around your CTObserver bits, however, and understanding how this is supposed to fit or why it needs to exist. It *seems* like an artifact of the "glue it up" bits, so maybe they aren't separable, but I've tried to explain my concern below. https://codereview.chromium.org/1845113003/diff/220001/net/cert/ct_observer.h File net/cert/ct_observer.h (right): https://codereview.chromium.org/1845113003/diff/220001/net/cert/ct_observer.h... net/cert/ct_observer.h:16: class NET_EXPORT CTObserver : public CTVerifier::Observer, public STHObserver { On 2016/04/25 14:50:59, Eran Messeri wrote: > The main reasons I've created the CTObserver interface under net/cert rather > than directly use the TreeStateTracker class: > - Not create a dependency on the components/certificate_transparency code > everywhere I have to pass the object that receives and distributes STH > notifications (depending on net/cert seems more natural). Why would you have to? If they're both pure virtual interfaces, and the certificate_transparency code implements both, then you just pass the object as the base pointer relevant to the place you're passing it to. > - Clearly document what's the interface client code should care about - so > client code never relies on implementation details of the TreeStateTracker. I don't understand this? > - Facilitate testing - that interface could be implemented in test code to test > correct interaction with the IOThread, for example. I don't understand this? > If you think all of these reasons are invalid I could remove this interface, but > having it used throughout the code helps me reason about the role(s) of the > TreeStateTracker. It's not that I think they're invalid, it's that I'm having trouble following the logic. To me, this class serves no purpose but to say "These two interfaces might be coupled" - but the whole point of separate interfaces is because you're saying "These are distinct functions". If you're having to pass something to code as a "CTObserver*", it sounds like the dependencies are broken or there's a short-circuit being taken somewhere. But as I mentioned, I could be misunderstanding. https://codereview.chromium.org/1845113003/diff/240001/chrome/browser/compone... File chrome/browser/component_updater/sth_set_component_installer.cc (right): https://codereview.chromium.org/1845113003/diff/240001/chrome/browser/compone... chrome/browser/component_updater/sth_set_component_installer.cc:194: DCHECK(g_browser_process->io_thread()); On 2016/04/25 14:50:59, Eran Messeri wrote: > On 2016/04/21 14:05:15, Ryan Sleevi wrote: > > This defeats the abstraction of lines 195-196 (content::BrowserThread::IO) and > > inappropriately couples it. > > > > If you want to DCHECK, then use GetMessageLoopProxyForThread and DCHECK. But > > that still doesn't guarantee correct results - because you can get the object > > even if there's no underlying backing thread. > > > > If you want correct results, look at the return result of the PostTask. > > Apologies, but I am not sure I understand this comment. It reads to me as if I > was (wrongly) checking that the function is called on the IO thread, but what I > was attempting to do is check that the IOThread object from the > g_browser_process global is not null. > > Are you suggesting to completely remove this DCHECK, call PostTask (below) and > DCHECK on the result? Yes. The point of using content::BrowserThread::PostTask(content::BrowserThread::IO ... ) is that you're supposed to be decoupled from g_browser_process. The fact that it exists is one of those necessary evils, but you should never be assuming that, say, the content::BrowserThread::IO == g_browser_process->io_thread(), because that's not the contract of content::BrowserThread::IO If you want to know if the thread still exists (more aptly, if the object was posted correctly), then just look at the result - which will return false if the thread is gone/shutting down. But it doesn't strike me as clear why you'd ever DCHECK() on this, or what the "developer error" scenario would be (but here, perhaps, I'm not understanding) https://codereview.chromium.org/1845113003/diff/240001/chrome/browser/compone... chrome/browser/component_updater/sth_set_component_installer.cc:198: base::Unretained(g_browser_process->io_thread()), On 2016/04/25 14:50:59, Eran Messeri wrote: > I'm trying to share an STHDistributor instance between the class implementing > ComponentInstallerTraits and receiving the STH updates and the STHObserver > (which lives in the IOThread / ProfileIOData) that needs these STHs. Having objects live on multiple threads - whether born on one and used on another or, worse, used on both - is usually an anti-pattern. > My original plan was to pass this instance into RegisterSTHSetComponent and > share it by passing it into the IOThread via BrowserProcessImpl, but sorin@ > asked for a less invasive change (see > https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/chrome_b...) > and suggested the use of the g_browser_process global to pass the object rather > than pass it into RegisterSTHSetComponent. > > Since I'm creating the STHDistributor instance here, I have no way of getting it > to the IOThread/STHObserver other than through globals. > > How do you suggest I get STHs out from this class to objects created on the > IOThread/ProfileIOData? Right, I'm not sure I understand sorin's objection, or that I'd agree with it. Using globals is, as you note, not desirable. Especially g_browser_process stuff. Sorin, could you explain more your comments? Did I miss a reply to Eran's request for more details? https://codereview.chromium.org/1845113003/diff/240001/components/certificate... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/1845113003/diff/240001/components/certificate... components/certificate_transparency/single_tree_tracker.h:58: void OnSCTVerified(net::X509Certificate* cert, On 2016/04/25 14:50:59, Eran Messeri wrote: > Is there anywhere I can read on the life-cycle of X509Certificate objects? > Ideally I could just hold onto the pointer for calculating the leaf hash > asynchronously (i.e. after OnSCTVerified) was called. I'm not sure I know what you're asking. They're reference-counted. If you're using it for just the invocation of the function, using X509Certificate* / const X509Certificate& is fine. If you're storing it / handling asynchronously, make your API contract clear about that by taking it as a reference-counted object (as a scoped_refptr), so that callers know "This will be retained" In short, think of it like "shared_ptr" - if it's naked, it's only valid for the duration of the call, but if you want to retain it, you need to take it as shared. Anything else is a recipe for lifetime/ownership hell. https://codereview.chromium.org/1845113003/diff/280001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1845113003/diff/280001/chrome/browser/io_thre... chrome/browser/io_thread.cc:1419: DCHECK(globals()->cert_transparency_observer.get()); Why? The justification for why this is done is unclear. https://codereview.chromium.org/1845113003/diff/280001/chrome/browser/io_thre... chrome/browser/io_thread.cc:1433: if (sth_reporter_) { Dominant style in this file seems to be no braces for one-lines. https://codereview.chromium.org/1845113003/diff/280001/chrome/browser/io_thre... chrome/browser/io_thread.cc:1439: DCHECK_NE(sth_observers_.count(observer), 0u); Why? It's clear that you intend to make sure the observer is already registered, but why don't you check during register if an object already has been registered once before? It's not clear what pattern(s) you're concerned about, because it feels asymmetric. https://codereview.chromium.org/1845113003/diff/280001/chrome/browser/io_thre... chrome/browser/io_thread.cc:1444: } Is this level of reciprocity (set a reporter it registers observers, set an observer it registers reporters) necessary? It seems to make the API itself somewhat confusing. For example, what if someone does RegisterSTHReporter after an sth_reporter_ is set? None of the observers are unregistered from the old reporter. Is that a BUG? I don't know, because it's unclear what the right API usage scenario is or should be. What if I register an STH observer, set a new reporter, then unregister? That seems like it would give to a BUG (the new sth_reporter_ doesn't have the observer registered). Is that a BUG? I don't know, because it's unclear what the right API usage scenario is or should be. https://codereview.chromium.org/1845113003/diff/280001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/1845113003/diff/280001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:56: << " could not be verified."; So who is expected to look at this? Under what scenarios? This is another example where I don't believe the logging is useful. https://codereview.chromium.org/1845113003/diff/280001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:94: << base::HexEncode(ct_log_->key_id().data(), ct_log_->key_id().size()); Seems overly verbose for a DCHECK. Because it's a developer error, and it triggers a debug breakpoint, anyone hitting this should just attach with a debugger.
Ryan, per your suggestion I've removed all the wiring code from this change, focusing on the TreeStateTracker. PTAL - I have scheduled a meeting to discuss the bigger design issues I have not replied to in this change. https://codereview.chromium.org/1845113003/diff/280001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1845113003/diff/280001/chrome/browser/io_thre... chrome/browser/io_thread.cc:1419: DCHECK(globals()->cert_transparency_observer.get()); On 2016/04/27 23:05:59, Ryan Sleevi wrote: > Why? The justification for why this is done is unclear. Removed, this was to verify assumptions during development. (Note this file was removed in the next patchset) https://codereview.chromium.org/1845113003/diff/280001/chrome/browser/io_thre... chrome/browser/io_thread.cc:1433: if (sth_reporter_) { On 2016/04/27 23:05:59, Ryan Sleevi wrote: > Dominant style in this file seems to be no braces for one-lines. Done. (Note this file was removed in the next patchset) https://codereview.chromium.org/1845113003/diff/280001/chrome/browser/io_thre... chrome/browser/io_thread.cc:1439: DCHECK_NE(sth_observers_.count(observer), 0u); On 2016/04/27 23:05:59, Ryan Sleevi wrote: > Why? It's clear that you intend to make sure the observer is already registered, > but why don't you check during register if an object already has been registered > once before? > > It's not clear what pattern(s) you're concerned about, because it feels > asymmetric. I'd like to verify that the object was (1) registered only once and (2) that everything that's unregistered was at one point registered. To verify (1) I've added DCHECK_EQ(sth_observers_.count(observer), 0u) to RegisterSTHObserver. Does the DCHECK here now make sense? https://codereview.chromium.org/1845113003/diff/280001/chrome/browser/io_thre... chrome/browser/io_thread.cc:1444: } On 2016/04/27 23:05:59, Ryan Sleevi wrote: > Is this level of reciprocity (set a reporter it registers observers, set an > observer it registers reporters) necessary? It seems to make the API itself > somewhat confusing. > > For example, what if someone does RegisterSTHReporter after an sth_reporter_ is > set? None of the observers are unregistered from the old reporter. > > Is that a BUG? I don't know, because it's unclear what the right API usage > scenario is or should be. > > What if I register an STH observer, set a new reporter, then unregister? That > seems like it would give to a BUG (the new sth_reporter_ doesn't have the > observer registered). > > Is that a BUG? I don't know, because it's unclear what the right API usage > scenario is or should be. Good point. I have: (1) Added a DCHECK to verify that the sth_reporter_ is not set in RegisterSTHReporter. Current design is to have exactly one sth_reporter_ during the lifetime of IOThread (as I don't see a need to change it). (2) Clarified in the documentation that RegisterSTHReporter is expected to be called only once, all observers registered with RegisterSTHObserver before it was called will be registered and all observers registered with RegisterSTHObserver after it was called will be registered immediately. https://codereview.chromium.org/1845113003/diff/280001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/1845113003/diff/280001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:56: << " could not be verified."; On 2016/04/27 23:05:59, Ryan Sleevi wrote: > So who is expected to look at this? Under what scenarios? > > This is another example where I don't believe the logging is useful. Removed. https://codereview.chromium.org/1845113003/diff/280001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:94: << base::HexEncode(ct_log_->key_id().data(), ct_log_->key_id().size()); On 2016/04/27 23:05:59, Ryan Sleevi wrote: > Seems overly verbose for a DCHECK. Because it's a developer error, and it > triggers a debug breakpoint, anyone hitting this should just attach with a > debugger. Done, removed verbose printout, left only the DCHECK_EQ.
sorin@chromium.org changed reviewers: + sorin@chromium.org
Ryan, please see my rationale below. It is a minor point and perhaps not a concern at all in the big picture. https://codereview.chromium.org/1845113003/diff/240001/chrome/browser/compone... File chrome/browser/component_updater/sth_set_component_installer.cc (right): https://codereview.chromium.org/1845113003/diff/240001/chrome/browser/compone... chrome/browser/component_updater/sth_set_component_installer.cc:198: base::Unretained(g_browser_process->io_thread()), On 2016/04/27 23:05:58, Ryan Sleevi wrote: > On 2016/04/25 14:50:59, Eran Messeri wrote: > > I'm trying to share an STHDistributor instance between the class implementing > > ComponentInstallerTraits and receiving the STH updates and the STHObserver > > (which lives in the IOThread / ProfileIOData) that needs these STHs. > > Having objects live on multiple threads - whether born on one and used on > another or, worse, used on both - is usually an anti-pattern. > > > My original plan was to pass this instance into RegisterSTHSetComponent and > > share it by passing it into the IOThread via BrowserProcessImpl, but sorin@ > > asked for a less invasive change (see > > > https://codereview.chromium.org/1853753003/diff/20001/chrome/browser/chrome_b...) > > and suggested the use of the g_browser_process global to pass the object > rather > > than pass it into RegisterSTHSetComponent. > > > > Since I'm creating the STHDistributor instance here, I have no way of getting > it > > to the IOThread/STHObserver other than through globals. > > > > How do you suggest I get STHs out from this class to objects created on the > > IOThread/ProfileIOData? > > Right, I'm not sure I understand sorin's objection, or that I'd agree with it. > Using globals is, as you note, not desirable. Especially g_browser_process > stuff. > > Sorin, could you explain more your comments? Did I miss a reply to Eran's > request for more details? I have a minor concern regarding a coupling issue that occurs in the function RegisterComponentsForUpdate. This is really minor in the big scheme of things but we'd like to have it resolved favorably, within reason. To understand the concern, let's take a look at the patchset 1, as this change has been broken off into many pieces by now. https://codereview.chromium.org/1845113003/diff/1/chrome/browser/chrome_brows... line 1710: we can see that what was a nice, cohesive piece of code, now has some dependencies on a STHDistributor. Also, note that something gets registered at line 1721, and that registration is not encapsulated anymore inside the actual registration of the STHSet component with the component updater, which happens under conditional and runtime branches at line 499 in the same file. Please noticed how the instance of STHDistributor is moved from call site to call site. These are small kinks but they raise some concerns about coupling, encapsulation, cohesion, and ownership of objects. Then, moving down the execution path, at line 447, we see how that function is given a parameter only to be used by the RegisterSTHSetComponent call below. We also don't like the coupling of this function with net:: artifacts. We'd prefer these implementation details to be encapsulated by the RegisterSTHSetComponent. The function RegisterComponentsForUpdate needs some further refactoring. It's not in the scope of this CL to clean this function up but if possible, we want to avoid additional dependencies, to make it easier for us to move the code in the future. In my opinion, further refactoring of the code in this CL could consider the following: * is the ownership of sth_distributor correct. Currently, that object is owned by the STHSetComponentInstallerTraits instance, but is that the right owner? Could g_browser_process own that, like it already owns other component dependencies? * can we use a singleton in the net::, as the CRLSet is using? As a closing thought, we are hitting some sharp edge in the design of the Chrome browser process instance. On one hand, we have components, that are related to how files are installed on the disk, but these files need to come alive, be instantiated, and hook with the rest of the browser at the right time, so that can be functionally useful. Each component has hacked its way in, either by using a global or a singleton to connect with the rest of the browser at runtime. We don't want to hold up this CL for pedantic reasons, especially since the code that deals with installers and updaters is already messy to start with. If these concerns are valid and they could be addressed reasonably, it would help the sanity of the code.
https://codereview.chromium.org/1845113003/diff/300001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/1845113003/diff/300001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:28: if (entries_status_.find(sct->timestamp) != entries_status_.end()) Document why the short-circuit https://codereview.chromium.org/1845113003/diff/300001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:31: if (verified_sth_.timestamp.is_null() || Document what this conditional is measuring https://codereview.chromium.org/1845113003/diff/300001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:53: // invalid STH should never be observed by Chrome clients. So why are we re-checking it here? Why shouldn't this be an implied API contract enforced at the higher layer? Why is silently failing on operation error desirable? What are the risks here of not doing this? I'm extremely uncomfortable with this because it means extra CPU load for validating ECDSA signatures; yes, it's per STH, so we would think this would be small (per component update cycle), but we should be trying to keep things nice and small. https://codereview.chromium.org/1845113003/diff/300001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:60: bool received_sth_is_newer = (sth.timestamp > verified_sth_.timestamp); Documentation explaining why this matters (either at 56 or at 61; I think 56 might make more sense when coupled with the conditional of 62-65) https://codereview.chromium.org/1845113003/diff/300001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:73: } These are sorted by timestamp, right? So once you reach a timestamp that is greater, you can stop iterating. Or am I missing something? Doesn't this also mean you're updating entries that are already pending repeatedly? Shouldn't you move them perhaps from one queue to another? In bulk, to minimize operations? https://codereview.chromium.org/1845113003/diff/300001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:88: DCHECK_EQ(ct_log_->key_id(), log_id) << "Got called for a different log."; See previous remarks about folding this Check into the methods (e.g. not as a member method) It's not clear why this is a programming error / DCHECK worthy, since that contract is not spelled out in the header at all. https://codereview.chromium.org/1845113003/diff/300001/components/certificate... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/1845113003/diff/300001/components/certificate... components/certificate_transparency/single_tree_tracker.h:31: // observed SCTs. Can you expand this comment more? What is involved with "tracks the state". What's the state for tracking? The second sentence seems to talk about "potentially can be used for", rather than "is used for" // Tracks the state of an individual Certificate Transparency Log's Merkle Tree. A // CT Log is made up of many Signed Tree Heads, for which every older STH must // be incorporated into the current/newer STH. As new certificates are logged, // new SCTs are produced, and eventually, those SCTs are incorporated into the log // and a new STH is produced, with there being a consistency proof between the // SCTs and the new STH, and between the old STH and the new STH. // This class tracks STHs that are observed, and ensures that as newer STHs are // observed, they consistently incorporate older STHs, establishing that the log is // consistent. Further, as SCTs are observed, their status is checked against the STH // to ensure that they were properly logged. If an SCT is newer than the latest STH, // then this class observes that when an STH is observed that should have incorporated // those SCTs, the SCTs are present. // // To accomplish this, this class needs to be notified of when new SCTs are observed // (which it does by implementing net::CTVerifier::Observer) and when new STHs are // observed (which it does by implementing net::ct::STHObserver). Once connected to // sources providing that data, the status for a given SCT can be queried by calling // GetLogEntryInclusionCheck ... That's a super-wordy explanation, but try to be explicit about the details here. The goal is to explain how this class fits within the CT ecosystem, minimizing shared/assumed knowledge when possible. I'm handwaving away how the consistency proofs are fetched, or proven, but I'm establishing terms like SCT and STH, since those are abbreviations being used here in this class and serve to contextualize the interface and all the comments on the methods, and also tries to set up the "Well, once you have all this, what's it good for?" https://codereview.chromium.org/1845113003/diff/300001/components/certificate... components/certificate_transparency/single_tree_tracker.h:53: // net::ct::CTVerifier::Observer implementation. Now that we have net::ct::STHObserver, it does make me wonder whether we should refactoring CTVerifier::Observer to instead be net::ct::SCTObserver, and have net::ct::CTVerifier take it (that is, moving it from being an inner class to being a standalone pure virtual interface) https://codereview.chromium.org/1845113003/diff/300001/components/certificate... components/certificate_transparency/single_tree_tracker.h:68: // |cert| and |sct|). You're implicitly dropping something here, which is that |cert| and |sct| must have been previously observed via OnSCTVerified; this isn't a general querying mechanism. https://codereview.chromium.org/1845113003/diff/300001/components/certificate... components/certificate_transparency/single_tree_tracker.h:82: // List of log entries pending inclusion check. Some newlines between each of these members would help readability. // A explanation A a_; // B explanation B b_; // C explanation C c_ https://codereview.chromium.org/1845113003/diff/300001/components/certificate... File components/certificate_transparency/tree_state_tracker.cc (right): https://codereview.chromium.org/1845113003/diff/300001/components/certificate... components/certificate_transparency/tree_state_tracker.cc:31: // Is the SCT from a known log? Isn't this part of the API contract? That is, that you must only be a net::ct::CTVerifier::Observer or net::ct::STHObserver for logs registered in the ctor? As such, calling it either for 32/33 or for 41/42 should be a DCHECK/CHECK error, because it means that additional logs are being added that aren't registered, and that's a programmer error. https://codereview.chromium.org/1845113003/diff/300001/components/certificate... File components/certificate_transparency/tree_state_tracker.h (right): https://codereview.chromium.org/1845113003/diff/300001/components/certificate... components/certificate_transparency/tree_state_tracker.h:32: // browsing session. Again, more documentation https://codereview.chromium.org/1845113003/diff/300001/net/cert/ct_verifier.h File net/cert/ct_verifier.h (right): https://codereview.chromium.org/1845113003/diff/300001/net/cert/ct_verifier.h... net/cert/ct_verifier.h:29: virtual ~Observer() {} Doesn't seem like this change belongs in this CL. Should it instead be protected? This makes it clear that the API contract of CTVerifier::Observer is that CTVerifier will not take ownership of the Observer - that the Observer's lifetime semantics are left up to those implementing the Observer interface.
Thanks for the review - PTAL. There are a few more TODOs in this CL as a result - it seems to me that this CL would significantly grow if I were to address them in this change. https://codereview.chromium.org/1845113003/diff/300001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/1845113003/diff/300001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:28: if (entries_status_.find(sct->timestamp) != entries_status_.end()) On 2016/05/09 17:03:49, Ryan Sleevi wrote: > Document why the short-circuit Done. https://codereview.chromium.org/1845113003/diff/300001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:31: if (verified_sth_.timestamp.is_null() || On 2016/05/09 17:03:48, Ryan Sleevi wrote: > Document what this conditional is measuring Done. https://codereview.chromium.org/1845113003/diff/300001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:53: // invalid STH should never be observed by Chrome clients. On 2016/05/09 17:03:48, Ryan Sleevi wrote: > So why are we re-checking it here? Why shouldn't this be an implied API contract > enforced at the higher layer? This is the first time we're verifying the STH on the client side. This could be an implied API contract, for example by validating STHs in the STHDistributor before sending them to the STHObservers. > > Why is silently failing on operation error desirable? What are the risks here of > not doing this? The implication of silently failing on operation error is that all pending SCTs will wait longer until they can be checked for inclusion. On the other hand, if the STH received is corrupted somehow and we did not validate it, we might falsely indicate that some SCTs could not be checked for inclusion (because the leaf hash was also corrupted, for example). > > I'm extremely uncomfortable with this because it means extra CPU load for > validating ECDSA signatures; yes, it's per STH, so we would think this would be > small (per component update cycle), but we should be trying to keep things nice > and small. we could avoid multiple checks of the same STH by checking signatures in the STHDistributor; Do you prefer that? If so I'll move the code there (but prefer to do so in a follow-up change as there will be some wiring changes). https://codereview.chromium.org/1845113003/diff/300001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:60: bool received_sth_is_newer = (sth.timestamp > verified_sth_.timestamp); On 2016/05/09 17:03:49, Ryan Sleevi wrote: > Documentation explaining why this matters (either at 56 or at 61; I think 56 > might make more sense when coupled with the conditional of 62-65) Done. https://codereview.chromium.org/1845113003/diff/300001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:73: } On 2016/05/09 17:03:49, Ryan Sleevi wrote: > These are sorted by timestamp, right? So once you reach a timestamp that is > greater, you can stop iterating. Or am I missing something? Correct - I've changed the code to stop iterating when encountering a timestamp that's greater than or equal to the STH timestamp. > > Doesn't this also mean you're updating entries that are already pending > repeatedly? Shouldn't you move them perhaps from one queue to another? In bulk, > to minimize operations? Yes, I could - once Rob's changes to the MerkleTreeLeaf are in I will have two maps, one for leaves pending inclusion check and one for tree leaves pending a newer STH. I've added a TODO to split this map into two once Rob's changes land. https://codereview.chromium.org/1845113003/diff/300001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:88: DCHECK_EQ(ct_log_->key_id(), log_id) << "Got called for a different log."; On 2016/05/09 17:03:49, Ryan Sleevi wrote: > See previous remarks about folding this Check into the methods (e.g. not as a > member method) Done. > > It's not clear why this is a programming error / DCHECK worthy, since that > contract is not spelled out in the header at all. Documented the contract. https://codereview.chromium.org/1845113003/diff/300001/components/certificate... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/1845113003/diff/300001/components/certificate... components/certificate_transparency/single_tree_tracker.h:31: // observed SCTs. On 2016/05/09 17:03:49, Ryan Sleevi wrote: > Can you expand this comment more? What is involved with "tracks the state". > What's the state for tracking? The second sentence seems to talk about > "potentially can be used for", rather than "is used for" > > // Tracks the state of an individual Certificate Transparency Log's Merkle Tree. > A > // CT Log is made up of many Signed Tree Heads, for which every older STH must > // be incorporated into the current/newer STH. As new certificates are logged, > // new SCTs are produced, and eventually, those SCTs are incorporated into the > log > // and a new STH is produced, with there being a consistency proof between the > // SCTs and the new STH, and between the old STH and the new STH. > // This class tracks STHs that are observed, and ensures that as newer STHs are > // observed, they consistently incorporate older STHs, establishing that the log > is > // consistent. Further, as SCTs are observed, their status is checked against > the STH > // to ensure that they were properly logged. If an SCT is newer than the latest > STH, > // then this class observes that when an STH is observed that should have > incorporated > // those SCTs, the SCTs are present. > // > // To accomplish this, this class needs to be notified of when new SCTs are > observed > // (which it does by implementing net::CTVerifier::Observer) and when new STHs > are > // observed (which it does by implementing net::ct::STHObserver). Once connected > to > // sources providing that data, the status for a given SCT can be queried by > calling > // GetLogEntryInclusionCheck ... > > > That's a super-wordy explanation, but try to be explicit about the details here. > The goal is to explain how this class fits within the CT ecosystem, minimizing > shared/assumed knowledge when possible. I'm handwaving away how the consistency > proofs are fetched, or proven, but I'm establishing terms like SCT and STH, > since those are abbreviations being used here in this class and serve to > contextualize the interface and all the comments on the methods, and also tries > to set up the "Well, once you have all this, what's it good for?" Done - used your text with some adaptations. https://codereview.chromium.org/1845113003/diff/300001/components/certificate... components/certificate_transparency/single_tree_tracker.h:53: // net::ct::CTVerifier::Observer implementation. On 2016/05/09 17:03:49, Ryan Sleevi wrote: > Now that we have net::ct::STHObserver, it does make me wonder whether we should > refactoring CTVerifier::Observer to instead be net::ct::SCTObserver, and have > net::ct::CTVerifier take it (that is, moving it from being an inner class to > being a standalone pure virtual interface) Acknowledged - can this be done in a follow-up CL? Added a TODO. https://codereview.chromium.org/1845113003/diff/300001/components/certificate... components/certificate_transparency/single_tree_tracker.h:68: // |cert| and |sct|). On 2016/05/09 17:03:49, Ryan Sleevi wrote: > You're implicitly dropping something here, which is that |cert| and |sct| must > have been previously observed via OnSCTVerified; this isn't a general querying > mechanism. I don't quite grasp the reasoning behind this - right now if |cert| and |sct| weren't previously observed, SCT_NOT_OBSERVED is returned. I've updated the documentation to reflect that if |cert| and |sct| were not observed, do not match each other or the |sct| is not for this log then SCT_NOT_OBSERVED will be returned. https://codereview.chromium.org/1845113003/diff/300001/components/certificate... components/certificate_transparency/single_tree_tracker.h:82: // List of log entries pending inclusion check. On 2016/05/09 17:03:49, Ryan Sleevi wrote: > Some newlines between each of these members would help readability. > > // A explanation > A a_; > > // B explanation > B b_; > > // C explanation > C c_ Done. https://codereview.chromium.org/1845113003/diff/300001/components/certificate... File components/certificate_transparency/tree_state_tracker.cc (right): https://codereview.chromium.org/1845113003/diff/300001/components/certificate... components/certificate_transparency/tree_state_tracker.cc:31: // Is the SCT from a known log? On 2016/05/09 17:03:49, Ryan Sleevi wrote: > Isn't this part of the API contract? That is, that you must only be a > net::ct::CTVerifier::Observer or net::ct::STHObserver for logs registered in the > ctor? > > As such, calling it either for 32/33 or for 41/42 should be a DCHECK/CHECK > error, because it means that additional logs are being added that aren't > registered, and that's a programmer error. Good point, there's a difference between OnSCTVerified and NewSTHObserved: - Since we don't currently allow modifying the logs list after start-up, OnSCTVerified should not be called with a log_id that the TreeStateTracker does not know about. - However, STHs for unknown logs could be provided by the component updater (for example, if we start pushing STHs for a new log, but old Chrome builds don't know about it yet). I've changed OnSCTVerified to DCHECK that the SCT is from a known log, but not NewSTHObserved. I've added a TODO for NewSTHObserved to DCHECK as well when the STHDistributor filters STHs from unknown logs. https://codereview.chromium.org/1845113003/diff/300001/components/certificate... File components/certificate_transparency/tree_state_tracker.h (right): https://codereview.chromium.org/1845113003/diff/300001/components/certificate... components/certificate_transparency/tree_state_tracker.h:32: // browsing session. On 2016/05/09 17:03:49, Ryan Sleevi wrote: > Again, more documentation Done. https://codereview.chromium.org/1845113003/diff/300001/net/cert/ct_verifier.h File net/cert/ct_verifier.h (right): https://codereview.chromium.org/1845113003/diff/300001/net/cert/ct_verifier.h... net/cert/ct_verifier.h:29: virtual ~Observer() {} On 2016/05/09 17:03:49, Ryan Sleevi wrote: > Doesn't seem like this change belongs in this CL. > > Should it instead be protected? This makes it clear that the API contract of > CTVerifier::Observer is that CTVerifier will not take ownership of the Observer > - that the Observer's lifetime semantics are left up to those implementing the > Observer interface. Done - made it protected.
lgtm Thank you Eran. Please consider fixing some of the const correctness issues, as needed, given the code style in net::. Also, double check the logic in a few boolean expressions. https://codereview.chromium.org/1845113003/diff/320001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/1845113003/diff/320001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:64: bool sths_for_same_tree = (verified_sth_.tree_size == sth.tree_size); can any of the local vars be declared const? Also, () not needed in the expression on the rhs. https://codereview.chromium.org/1845113003/diff/320001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:81: while (entry->first < verified_sth_.timestamp && Do we want to the check all the entries in entries_status_ or iterate over some of them only? https://codereview.chromium.org/1845113003/diff/320001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:94: if (it == entries_status_.end()) could use return ?: https://codereview.chromium.org/1845113003/diff/320001/components/certificate... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/1845113003/diff/320001/components/certificate... components/certificate_transparency/single_tree_tracker.h:67: explicit SingleTreeTracker(scoped_refptr<const net::CTLogVerifier> ct_log); sometimes, people pass this by ref to const for small perf gains (avoids the copy, not sure if this is still an issue in C++11 due to the move semantics, I need to look this up). https://codereview.chromium.org/1845113003/diff/320001/components/certificate... File components/certificate_transparency/single_tree_tracker_unittest.cc (right): https://codereview.chromium.org/1845113003/diff/320001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:29: size_t kOldSTHTreeSize = 12u; const? https://codereview.chromium.org/1845113003/diff/320001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:45: std::string sha256_root_hash(hex_output.begin(), hex_output.end()); const? https://codereview.chromium.org/1845113003/diff/320001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:53: std::string tree_head_signature(hex_output.begin(), hex_output.end()); const? https://codereview.chromium.org/1845113003/diff/320001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:55: return DecodeDigitallySigned(&sp, &(sth->signature)) && sp.empty(); Is && sp.empty() correct? Not sure what the logic is here, just asking to double check. https://codereview.chromium.org/1845113003/diff/320001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:69: std::string der_test_cert(net::ct::GetDerEncodedX509Cert()); const? https://codereview.chromium.org/1845113003/diff/320001/components/certificate... File components/certificate_transparency/tree_state_tracker.h (right): https://codereview.chromium.org/1845113003/diff/320001/components/certificate... components/certificate_transparency/tree_state_tracker.h:42: std::vector<scoped_refptr<const net::CTLogVerifier>> ct_logs); maybe pass by ref to const?
Thanks for the review, Sorin, addressed your comments. Ryan, PTAL. https://codereview.chromium.org/1845113003/diff/320001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/1845113003/diff/320001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:64: bool sths_for_same_tree = (verified_sth_.tree_size == sth.tree_size); On 2016/05/11 17:03:39, Sorin Jianu wrote: > can any of the local vars be declared const? > Also, () not needed in the expression on the rhs. They can all be! Done and done. https://codereview.chromium.org/1845113003/diff/320001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:81: while (entry->first < verified_sth_.timestamp && On 2016/05/11 17:03:39, Sorin Jianu wrote: > Do we want to the check all the entries in entries_status_ or iterate over some > of them only? Only some - as Ryan pointed out the entries are sorted by timestamp, so when we encounter the first entry whose timestamp is greater than the verified_sth_'s timestamp we can stop. https://codereview.chromium.org/1845113003/diff/320001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:94: if (it == entries_status_.end()) On 2016/05/11 17:03:39, Sorin Jianu wrote: > could use return ?: Done. https://codereview.chromium.org/1845113003/diff/320001/components/certificate... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/1845113003/diff/320001/components/certificate... components/certificate_transparency/single_tree_tracker.h:67: explicit SingleTreeTracker(scoped_refptr<const net::CTLogVerifier> ct_log); On 2016/05/11 17:03:39, Sorin Jianu wrote: > sometimes, people pass this by ref to const for small perf gains (avoids the > copy, not sure if this is still an issue in C++11 due to the move semantics, I > need to look this up). Done - since these are created once per profile creation, there shouldn't be a big performance impact overall. Changed anyway. https://codereview.chromium.org/1845113003/diff/320001/components/certificate... File components/certificate_transparency/single_tree_tracker_unittest.cc (right): https://codereview.chromium.org/1845113003/diff/320001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:29: size_t kOldSTHTreeSize = 12u; On 2016/05/11 17:03:39, Sorin Jianu wrote: > const? Done. https://codereview.chromium.org/1845113003/diff/320001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:45: std::string sha256_root_hash(hex_output.begin(), hex_output.end()); On 2016/05/11 17:03:39, Sorin Jianu wrote: > const? Done. https://codereview.chromium.org/1845113003/diff/320001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:53: std::string tree_head_signature(hex_output.begin(), hex_output.end()); On 2016/05/11 17:03:39, Sorin Jianu wrote: > const? Done. https://codereview.chromium.org/1845113003/diff/320001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:55: return DecodeDigitallySigned(&sp, &(sth->signature)) && sp.empty(); On 2016/05/11 17:03:39, Sorin Jianu wrote: > Is && sp.empty() correct? Not sure what the logic is here, just asking to double > check. Yes - DecodeDigitallySigned modifies the StringPiece, so not only should it succeed, but there should be no bytes left over in the StringPiece afterwards. https://codereview.chromium.org/1845113003/diff/320001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:69: std::string der_test_cert(net::ct::GetDerEncodedX509Cert()); On 2016/05/11 17:03:39, Sorin Jianu wrote: > const? Done. https://codereview.chromium.org/1845113003/diff/320001/components/certificate... File components/certificate_transparency/tree_state_tracker.h (right): https://codereview.chromium.org/1845113003/diff/320001/components/certificate... components/certificate_transparency/tree_state_tracker.h:42: std::vector<scoped_refptr<const net::CTLogVerifier>> ct_logs); On 2016/05/11 17:03:39, Sorin Jianu wrote: > maybe pass by ref to const? IIRC Ryan discouraged me from doing so in another change (there are very few items in this vector anyway).
https://codereview.chromium.org/1845113003/diff/320001/components/certificate... File components/certificate_transparency/tree_state_tracker.h (right): https://codereview.chromium.org/1845113003/diff/320001/components/certificate... components/certificate_transparency/tree_state_tracker.h:42: std::vector<scoped_refptr<const net::CTLogVerifier>> ct_logs); On 2016/05/12 17:04:49, Eran Messeri wrote: > On 2016/05/11 17:03:39, Sorin Jianu wrote: > > maybe pass by ref to const? > > IIRC Ryan discouraged me from doing so in another change (there are very few > items in this vector anyway). That was only for when you were taking ownership (so you could std::move()). You're making copies here, so you're getting no benefit. That is, if you had a member std::vector<scoped_refptr<...>> ct_logs_, then having this as non-const, non-ref would let you ct_logs_ = std::move(ct_logs), and at the call site, you could "new TreeStateTracker(std::move(my_ct_logs))". But that doesn't really apply here
https://codereview.chromium.org/1845113003/diff/320001/components/certificate... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/1845113003/diff/320001/components/certificate... components/certificate_transparency/single_tree_tracker.h:67: explicit SingleTreeTracker(scoped_refptr<const net::CTLogVerifier> ct_log); On 2016/05/12 17:04:49, Eran Messeri wrote: > Done - since these are created once per profile creation, there shouldn't be a > big performance impact overall. Changed anyway. This is where you std::move (because you have ct_log_), and pass as you were (non-const non-ref)
Fixed passing of scoped_refptr and usage of std::move per suggestion in a thread Ryan kindly pointed to me. PTAL, Ryan. https://codereview.chromium.org/1845113003/diff/320001/components/certificate... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/1845113003/diff/320001/components/certificate... components/certificate_transparency/single_tree_tracker.h:67: explicit SingleTreeTracker(scoped_refptr<const net::CTLogVerifier> ct_log); On 2016/05/12 17:10:04, Ryan Sleevi wrote: > On 2016/05/12 17:04:49, Eran Messeri wrote: > > Done - since these are created once per profile creation, there shouldn't be a > > big performance impact overall. Changed anyway. > > This is where you std::move (because you have ct_log_), and pass as you were > (non-const non-ref) Done - I std::move into ct_log_ in the SingleTreeTracker c'tor. https://codereview.chromium.org/1845113003/diff/320001/components/certificate... File components/certificate_transparency/tree_state_tracker.h (right): https://codereview.chromium.org/1845113003/diff/320001/components/certificate... components/certificate_transparency/tree_state_tracker.h:42: std::vector<scoped_refptr<const net::CTLogVerifier>> ct_logs); On 2016/05/12 17:09:12, Ryan Sleevi wrote: > On 2016/05/12 17:04:49, Eran Messeri wrote: > > On 2016/05/11 17:03:39, Sorin Jianu wrote: > > > maybe pass by ref to const? > > > > IIRC Ryan discouraged me from doing so in another change (there are very few > > items in this vector anyway). > > That was only for when you were taking ownership (so you could std::move()). > You're making copies here, so you're getting no benefit. > > That is, if you had a member > > std::vector<scoped_refptr<...>> ct_logs_, then having this as non-const, non-ref > would let you ct_logs_ = std::move(ct_logs), and at the call site, you could > "new TreeStateTracker(std::move(my_ct_logs))". But that doesn't really apply > here Acknowledged. https://codereview.chromium.org/1845113003/diff/360001/components/certificate... File components/certificate_transparency/tree_state_tracker.cc (right): https://codereview.chromium.org/1845113003/diff/360001/components/certificate... components/certificate_transparency/tree_state_tracker.cc:23: tree_trackers_[log->key_id()].reset(new SingleTreeTracker(log)); Note I am *not* calling std::move here as I use the log instance for the key_id() call. Since (as far as I understand) evaluation order of these expressions is unspecified, I shouldn't std::move(log) into the SingleTreeTracker constructor.
https://codereview.chromium.org/1845113003/diff/380001/components/certificate... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/1845113003/diff/380001/components/certificate... components/certificate_transparency/single_tree_tracker.h:36: // This class periodically receives STHs provided via Chrome's component Layering: Seems inappropriate to talk about how this is used (by //chrome) in //components, especially talking about Chrome (the product) https://codereview.chromium.org/1845113003/diff/380001/components/certificate... components/certificate_transparency/single_tree_tracker.h:38: // via the component updater). As SCTs are observed, their status is checked e.g. This class receives STHs provided by/observed by the embedder, with the assumption that STHs have been checked for consistency already. As SCTs are observed, their status is checked against the latest STH to ensure they were properly logged. ... https://codereview.chromium.org/1845113003/diff/380001/components/certificate... components/certificate_transparency/single_tree_tracker.h:48: No newline https://codereview.chromium.org/1845113003/diff/380001/components/certificate... components/certificate_transparency/single_tree_tracker.h:55: // SCT was not observed by this class. Do we start inclusion checking if this is the case? Why or why not? Document more :) https://codereview.chromium.org/1845113003/diff/380001/components/certificate... components/certificate_transparency/single_tree_tracker.h:74: // STH known for this log is older than sct.timestamp + MMD, enqueues s/MMD/Maximum Merge Delay/ https://codereview.chromium.org/1845113003/diff/380001/components/certificate... components/certificate_transparency/single_tree_tracker.h:85: // Should only be called for STHs issued by the log this instance tracks. "Should" is weak. This is a must, correct? https://codereview.chromium.org/1845113003/diff/380001/components/certificate... components/certificate_transparency/single_tree_tracker.h:88: // Returns the status of a given log entry (that is assembled from s/(that is/that is/ (e.g. drop the parens) https://codereview.chromium.org/1845113003/diff/380001/components/certificate... components/certificate_transparency/single_tree_tracker.h:97: // Holds the latest STH fetched and verified for each log. "for each log" - this is a single log. https://codereview.chromium.org/1845113003/diff/380001/components/certificate... components/certificate_transparency/single_tree_tracker.h:100: // The log tracked. The log being tracked https://codereview.chromium.org/1845113003/diff/380001/components/certificate... components/certificate_transparency/single_tree_tracker.h:105: // whole MerkleTreeLeaf (RFC6962, section 3.4.) as a key. See crbug.com/506227 s/crbug.com/https://crbug.com/ However, this bug doesn't provide any explanation when I first load it. It's not until #c22 that I even begin to see the plan, but you don't explain why, just what. Document better in that bug :) https://codereview.chromium.org/1845113003/diff/380001/components/certificate... File components/certificate_transparency/tree_state_tracker.cc (right): https://codereview.chromium.org/1845113003/diff/380001/components/certificate... components/certificate_transparency/tree_state_tracker.cc:32: DCHECK(it != tree_trackers_.end()); CHECK, right? Otherwise 34 might be a security bug/crash. https://codereview.chromium.org/1845113003/diff/380001/components/certificate... components/certificate_transparency/tree_state_tracker.cc:40: // component updater can be for logs not yet recognized older clients layering: Talking about Chrome details (component updater) in //components feels wrong. https://codereview.chromium.org/1845113003/diff/380001/components/certificate... File components/certificate_transparency/tree_state_tracker.h (right): https://codereview.chromium.org/1845113003/diff/380001/components/certificate... components/certificate_transparency/tree_state_tracker.h:42: std::vector<scoped_refptr<const net::CTLogVerifier>> ct_logs); Document more :) In particular, that the set of logs cannot change over time/what happens. This feels more important than the comments on 47/48 and 54/55
Ryan, PTAL - in addition to addressing your comments I've modified the unit test based on comments from another review. https://codereview.chromium.org/1845113003/diff/380001/components/certificate... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/1845113003/diff/380001/components/certificate... components/certificate_transparency/single_tree_tracker.h:36: // This class periodically receives STHs provided via Chrome's component On 2016/05/16 19:29:56, Ryan Sleevi wrote: > Layering: Seems inappropriate to talk about how this is used (by //chrome) in > //components, especially talking about Chrome (the product) Done - removed. https://codereview.chromium.org/1845113003/diff/380001/components/certificate... components/certificate_transparency/single_tree_tracker.h:38: // via the component updater). As SCTs are observed, their status is checked On 2016/05/16 19:29:56, Ryan Sleevi wrote: > e.g. > This class receives STHs provided by/observed by the embedder, with the > assumption that STHs have been checked for consistency already. As SCTs are > observed, their status is checked against the latest STH to ensure they were > properly logged. ... Done, used your suggested text. https://codereview.chromium.org/1845113003/diff/380001/components/certificate... components/certificate_transparency/single_tree_tracker.h:48: On 2016/05/16 19:29:55, Ryan Sleevi wrote: > No newline Done. https://codereview.chromium.org/1845113003/diff/380001/components/certificate... components/certificate_transparency/single_tree_tracker.h:55: // SCT was not observed by this class. On 2016/05/16 19:29:56, Ryan Sleevi wrote: > Do we start inclusion checking if this is the case? Why or why not? Document > more :) Done - I've documented that this status is for unknown SCTs where we don't do anything since there's no evidence they were verified (i.e. never observed via OnSCTVerified). https://codereview.chromium.org/1845113003/diff/380001/components/certificate... components/certificate_transparency/single_tree_tracker.h:74: // STH known for this log is older than sct.timestamp + MMD, enqueues On 2016/05/16 19:29:56, Ryan Sleevi wrote: > s/MMD/Maximum Merge Delay/ Done. https://codereview.chromium.org/1845113003/diff/380001/components/certificate... components/certificate_transparency/single_tree_tracker.h:85: // Should only be called for STHs issued by the log this instance tracks. On 2016/05/16 19:29:56, Ryan Sleevi wrote: > "Should" is weak. This is a must, correct? Yes, done. https://codereview.chromium.org/1845113003/diff/380001/components/certificate... components/certificate_transparency/single_tree_tracker.h:88: // Returns the status of a given log entry (that is assembled from On 2016/05/16 19:29:56, Ryan Sleevi wrote: > s/(that is/that is/ > > (e.g. drop the parens) Done. https://codereview.chromium.org/1845113003/diff/380001/components/certificate... components/certificate_transparency/single_tree_tracker.h:97: // Holds the latest STH fetched and verified for each log. On 2016/05/16 19:29:56, Ryan Sleevi wrote: > "for each log" - this is a single log. Done. https://codereview.chromium.org/1845113003/diff/380001/components/certificate... components/certificate_transparency/single_tree_tracker.h:100: // The log tracked. On 2016/05/16 19:29:55, Ryan Sleevi wrote: > The log being tracked Done. https://codereview.chromium.org/1845113003/diff/380001/components/certificate... components/certificate_transparency/single_tree_tracker.h:105: // whole MerkleTreeLeaf (RFC6962, section 3.4.) as a key. See crbug.com/506227 On 2016/05/16 19:29:56, Ryan Sleevi wrote: > s/crbug.com/https://crbug.com/ > > However, this bug doesn't provide any explanation when I first load it. It's not > until #c22 that I even begin to see the plan, but you don't explain why, just > what. Document better in that bug :) https reference done. As bugs.chromium.org seems down at the moment, I'll document when it's up. https://codereview.chromium.org/1845113003/diff/380001/components/certificate... File components/certificate_transparency/tree_state_tracker.cc (right): https://codereview.chromium.org/1845113003/diff/380001/components/certificate... components/certificate_transparency/tree_state_tracker.cc:32: DCHECK(it != tree_trackers_.end()); On 2016/05/16 19:29:56, Ryan Sleevi wrote: > CHECK, right? > > Otherwise 34 might be a security bug/crash. Right, done. https://codereview.chromium.org/1845113003/diff/380001/components/certificate... components/certificate_transparency/tree_state_tracker.cc:40: // component updater can be for logs not yet recognized older clients On 2016/05/16 19:29:56, Ryan Sleevi wrote: > layering: Talking about Chrome details (component updater) in //components feels > wrong. Done. https://codereview.chromium.org/1845113003/diff/380001/components/certificate... File components/certificate_transparency/tree_state_tracker.h (right): https://codereview.chromium.org/1845113003/diff/380001/components/certificate... components/certificate_transparency/tree_state_tracker.h:42: std::vector<scoped_refptr<const net::CTLogVerifier>> ct_logs); On 2016/05/16 19:29:56, Ryan Sleevi wrote: > Document more :) > > In particular, that the set of logs cannot change over time/what happens. > > This feels more important than the comments on 47/48 and 54/55 Done. I've attempted to document the assumption that the list of recognized logs is known at creation time and does not change during an instance's lifetime.
Description was changed from ========== This change lays the groundwork for actual inclusion checking of observed SCTs (and the certs they belong to). This is done by introducing a new class, TreeStateTracker, that receives notifications of new STHs (provided from the component updater) and new, verified SCTs (from the CTLogVerifier in the IOThread and each ProfileIOData). Right now the only functionality is tracking the state of observed SCTs to figure out if they can already be checked for inclusion or not. The next change will log some UMA on this. BUG=506227 ========== to ========== Certificate Transparency: Start tracking the logs' STHs. This introduces the framework for actual inclusion checking of SCTs, and the certs the SCTs are associated with. This is done by introducing a new class, the TreeStateTracker, which can receive notifications of new STHs (which will be provided by the component updater), as well as notifications of verified STHs (provided by CTLogVerifiers). Right now, the only logic is tracking the state of observed SCTs to determine if they can be checked for inclusion or not - that is, whether or not an STH is available that is newer than the SCT's timestamp + the logs maximum merge delay. Except for freshly issued/logged certificates, this is expected to generally be the case. The next change will gather metrics about this. BUG=506227 ==========
Reworded the CL description as well. Remember, the CL Subject doesn't end up in source control - so that often means repeating it, as only the Message appears as the commit message. https://codereview.chromium.org/1845113003/diff/400001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/1845113003/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:27: << "Got called for a different log."; Drop the << - the DCHECK message will have plenty of context showing these two aren't equal. https://codereview.chromium.org/1845113003/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:29: // SCT was previously observed so its status should not be changed. observed, so https://codereview.chromium.org/1845113003/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:34: if (verified_sth_.timestamp.is_null() || Why is timestamp.is_null() valid, out of curiousity? https://codereview.chromium.org/1845113003/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:51: DCHECK_EQ(ct_log_->key_id(), sth.log_id) << "Got called for a different log."; Drop the << https://codereview.chromium.org/1845113003/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:56: // invalid STH should never be observed by Chrome clients. Again, layering - Chrome, component updater - shouldn't be talked about here // Sanity check the STH; the caller should have done this // already, but being paranoid here. if (!ct_log_->VerifySignedTreeHead(sth)) { ... } (I still don't think it's right to do here, so maybe a TODO if we think we'll get a better place for this, or at least document that you don't think you'll ever find a way to get rid of this check) https://codereview.chromium.org/1845113003/diff/400001/components/certificate... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/1845113003/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker.h:41: // SCTs, the SCTs are present. s/are present/are present in the log./ https://codereview.chromium.org/1845113003/diff/400001/components/certificate... File components/certificate_transparency/single_tree_tracker_unittest.cc (right): https://codereview.chromium.org/1845113003/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:75: TEST_F(SingleTreeTrackerTest, TestCorrectlyClassifiesUnobservedSCTNoSTH) { Same remarks as the previous CL regarding being clearer about the tests here. For example, it's unclear what "classifies" means, and what "NoSTH" means given the call Whenever in doubt, prefer documenting too much. You can see my comments below on the vertical whitespace, which I think highlights the challenge here - trying to figure out what it is you're testing. Is 76-78 testing that an STH not observed yet is "SCT_NOT_OBSERVED", or is it testing that when OnSCTVerified is called, that the status is SCT_PENDING_NEWER_STH? The test name leaves it ambiguous as to what's a precondition (e.g. making sure the test environment is sane) and what's intrinsic to your logic (e.g. making sure that SCTs are in the NOT_OBSERVED state) https://codereview.chromium.org/1845113003/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:79: tree_tracker_->OnSCTVerified(chain_.get(), cert_sct_.get()); Newline here between 78 & 79 - That is, it seems like 76-78 are testing an explicit condition (not a precondition), line 79 is the state change, and line 81-83 are the post-conditions/expectations of the state change. Is that correct? If so, logically grouping them with vertical whitespace can help. https://codereview.chromium.org/1845113003/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:103: TEST_F(SingleTreeTrackerTest, TestCorrectlyUpdatesSCTStatusOnNewSTH) { the "Test" in all of these tests is redundant SingleTreeTrackerTest.CorrectlyUpdatesSCTStatusOnNewSTH https://codereview.chromium.org/1845113003/diff/400001/components/certificate... File components/certificate_transparency/tree_state_tracker.cc (right): https://codereview.chromium.org/1845113003/diff/400001/components/certificate... components/certificate_transparency/tree_state_tracker.cc:22: for (auto& log : ct_logs) const auto& https://codereview.chromium.org/1845113003/diff/400001/components/certificate... components/certificate_transparency/tree_state_tracker.cc:32: CHECK(it != tree_trackers_.end()); See remarks in header. Having reviewed this again, the asymmetry is enough that it's going to continue to bother me as I don't believe it's justified in this class/headers (but is an implementation detail of how it's used), so I'd rather soft-fail here and ignore it, for consistency, tighten up the calling code (in both cases), and then move BOTH of these to stronger guarantees. https://codereview.chromium.org/1845113003/diff/400001/components/certificate... File components/certificate_transparency/tree_state_tracker.h (right): https://codereview.chromium.org/1845113003/diff/400001/components/certificate... components/certificate_transparency/tree_state_tracker.h:48: // one of the logs in the list provided during construction. Better, but not quite - now that's documenting the details of the precondition, rather than the precondition itself. That said, I realize I really am quite uncomfortable with this precondition asymmetry. I cannot logically make sense of it in this code, without knowing that it's simply being used as a short-circuit for your higher level code (e.g. which should only send STHs for which it's observed), or that we should be ignoring unrecognized logs in OnSCTVerified. Alternatively, we should be registering this class ourselves with ct_logs as the Observer, so that we manage the conditions correctly (namely, that we're only observing our logs - and that means removing the public inheritance from net::CTVerifier::Observer and using an inner class helper to manage that relationship, to guarantee no one sends us SCTs we don't care about) I'd like to settle on a clearer separation between these, but I'd also like to keep this CL small, so I think we need to address this as a design follow-up; in the mean time // Tracks the state of the logs provided in |ct_logs|. https://codereview.chromium.org/1845113003/diff/400001/components/certificate... components/certificate_transparency/tree_state_tracker.h:55: // Must only be called with SCTs from logs present during construction. Happy to drop 55/61 in light of the ctor comment.
Addressed all comments, PTAL. https://codereview.chromium.org/1845113003/diff/400001/components/certificate... File components/certificate_transparency/single_tree_tracker.cc (right): https://codereview.chromium.org/1845113003/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:27: << "Got called for a different log."; On 2016/05/18 16:58:18, Ryan Sleevi wrote: > Drop the << - the DCHECK message will have plenty of context showing these two > aren't equal. Done. https://codereview.chromium.org/1845113003/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:29: // SCT was previously observed so its status should not be changed. On 2016/05/18 16:58:18, Ryan Sleevi wrote: > observed, so Done. https://codereview.chromium.org/1845113003/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:34: if (verified_sth_.timestamp.is_null() || On 2016/05/18 16:58:18, Ryan Sleevi wrote: > Why is timestamp.is_null() valid, out of curiousity? My bad, documentation was bad - what we're checking here is if there's no valid STH or the STH we have is too old. I've changed the comment to reflect it. https://codereview.chromium.org/1845113003/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:51: DCHECK_EQ(ct_log_->key_id(), sth.log_id) << "Got called for a different log."; On 2016/05/18 16:58:18, Ryan Sleevi wrote: > Drop the << Done. https://codereview.chromium.org/1845113003/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker.cc:56: // invalid STH should never be observed by Chrome clients. On 2016/05/18 16:58:18, Ryan Sleevi wrote: > Again, layering - Chrome, component updater - shouldn't be talked about here > > // Sanity check the STH; the caller should have done this > // already, but being paranoid here. > if (!ct_log_->VerifySignedTreeHead(sth)) { > ... > } > > (I still don't think it's right to do here, so maybe a TODO if we think we'll > get a better place for this, or at least document that you don't think you'll > ever find a way to get rid of this check) Done - switched to your suggested documentation and added a note saying we can't get rid of this check here at the moment. https://codereview.chromium.org/1845113003/diff/400001/components/certificate... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/1845113003/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker.h:41: // SCTs, the SCTs are present. On 2016/05/18 16:58:18, Ryan Sleevi wrote: > s/are present/are present in the log./ Done. https://codereview.chromium.org/1845113003/diff/400001/components/certificate... File components/certificate_transparency/single_tree_tracker_unittest.cc (right): https://codereview.chromium.org/1845113003/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:75: TEST_F(SingleTreeTrackerTest, TestCorrectlyClassifiesUnobservedSCTNoSTH) { On 2016/05/18 16:58:19, Ryan Sleevi wrote: > Same remarks as the previous CL regarding being clearer about the tests here. > For example, it's unclear what "classifies" means, and what "NoSTH" means given > the call > > Whenever in doubt, prefer documenting too much. You can see my comments below on > the vertical whitespace, which I think highlights the challenge here - trying to > figure out what it is you're testing. Is 76-78 testing that an STH not observed > yet is "SCT_NOT_OBSERVED", or is it testing that when OnSCTVerified is called, > that the status is SCT_PENDING_NEWER_STH? The test name leaves it ambiguous as > to what's a precondition (e.g. making sure the test environment is sane) and > what's intrinsic to your logic (e.g. making sure that SCTs are in the > NOT_OBSERVED state) Done, added documentation to all test cases. (as a side note, other projects I've worked on had the philosophy that if the name of the test + the test code aren't clear enough, then the test case is too cumbersome, which is why I've left out documentation). https://codereview.chromium.org/1845113003/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:79: tree_tracker_->OnSCTVerified(chain_.get(), cert_sct_.get()); On 2016/05/18 16:58:19, Ryan Sleevi wrote: > Newline here between 78 & 79 - > > That is, it seems like 76-78 are testing an explicit condition (not a > precondition), line 79 is the state change, and line 81-83 are the > post-conditions/expectations of the state change. Is that correct? If so, > logically grouping them with vertical whitespace can help. Done (added comments too). https://codereview.chromium.org/1845113003/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:103: TEST_F(SingleTreeTrackerTest, TestCorrectlyUpdatesSCTStatusOnNewSTH) { On 2016/05/18 16:58:19, Ryan Sleevi wrote: > the "Test" in all of these tests is redundant > > SingleTreeTrackerTest.CorrectlyUpdatesSCTStatusOnNewSTH Done. https://codereview.chromium.org/1845113003/diff/400001/components/certificate... File components/certificate_transparency/tree_state_tracker.cc (right): https://codereview.chromium.org/1845113003/diff/400001/components/certificate... components/certificate_transparency/tree_state_tracker.cc:22: for (auto& log : ct_logs) On 2016/05/18 16:58:19, Ryan Sleevi wrote: > const auto& Done. https://codereview.chromium.org/1845113003/diff/400001/components/certificate... components/certificate_transparency/tree_state_tracker.cc:32: CHECK(it != tree_trackers_.end()); On 2016/05/18 16:58:19, Ryan Sleevi wrote: > See remarks in header. Having reviewed this again, the asymmetry is enough that > it's going to continue to bother me as I don't believe it's justified in this > class/headers (but is an implementation detail of how it's used), so I'd rather > soft-fail here and ignore it, for consistency, tighten up the calling code (in > both cases), and then move BOTH of these to stronger guarantees. Done. https://codereview.chromium.org/1845113003/diff/400001/components/certificate... File components/certificate_transparency/tree_state_tracker.h (right): https://codereview.chromium.org/1845113003/diff/400001/components/certificate... components/certificate_transparency/tree_state_tracker.h:48: // one of the logs in the list provided during construction. On 2016/05/18 16:58:19, Ryan Sleevi wrote: > Better, but not quite - now that's documenting the details of the precondition, > rather than the precondition itself. > > That said, I realize I really am quite uncomfortable with this precondition > asymmetry. I cannot logically make sense of it in this code, without knowing > that it's simply being used as a short-circuit for your higher level code (e.g. > which should only send STHs for which it's observed), or that we should be > ignoring unrecognized logs in OnSCTVerified. > > Alternatively, we should be registering this class ourselves with ct_logs as the > Observer, so that we manage the conditions correctly (namely, that we're only > observing our logs - and that means removing the public inheritance from > net::CTVerifier::Observer and using an inner class helper to manage that > relationship, to guarantee no one sends us SCTs we don't care about) > > I'd like to settle on a clearer separation between these, but I'd also like to > keep this CL small, so I think we need to address this as a design follow-up; in > the mean time > > // Tracks the state of the logs provided in |ct_logs|. I understand the asymmetry problem of CHECK-failing when data related to unknown logs arrives in one callback, but simply ignoring it in the other. The way around it I can suggest (in a follow-up CL) is to add a filter to the STHDistributor whose sole purpose would be to drop STHs from unknown logs. Would that alleviate the concern, given we'd then be able to CHECK-fail in the TreeStateTracker when either an SCT or an STH from an unknown log are provided? https://codereview.chromium.org/1845113003/diff/400001/components/certificate... components/certificate_transparency/tree_state_tracker.h:55: // Must only be called with SCTs from logs present during construction. On 2016/05/18 16:58:19, Ryan Sleevi wrote: > Happy to drop 55/61 in light of the ctor comment. Done.
A few comment nits, but I'm going to LGTM, under the assumption you'll review them all carefully (some were leftover from previous changesets) https://codereview.chromium.org/1845113003/diff/400001/components/certificate... File components/certificate_transparency/single_tree_tracker_unittest.cc (right): https://codereview.chromium.org/1845113003/diff/400001/components/certificate... components/certificate_transparency/single_tree_tracker_unittest.cc:75: TEST_F(SingleTreeTrackerTest, TestCorrectlyClassifiesUnobservedSCTNoSTH) { On 2016/05/19 12:35:00, Eran Messeri wrote: > Done, added documentation to all test cases. > (as a side note, other projects I've worked on had the philosophy that if the > name of the test + the test code aren't clear enough, then the test case is too > cumbersome, which is why I've left out documentation). I think I'd disagree pretty heavily with that philosophy :) Test code makes it easy to document what's being tested, but makes it impossible to document what's "expected" - that is, whether an edge case (that may cause a test to fail) was anticipated or not. That's why I find it useful to document both the intent (e.g. in prosaic comments) and the actual execution, so that when things fail, it can be clear whether they were intended to fail, or if they're just failing for other reasons (e.g. the test was written poorly, the edge case was unanticipated, the API has changed, etc) Otherwise, when a test fails, the natural reaction is to 'fix' the test, without understanding whether or not the failure is 'expected' or not. https://codereview.chromium.org/1845113003/diff/400001/components/certificate... File components/certificate_transparency/tree_state_tracker.h (right): https://codereview.chromium.org/1845113003/diff/400001/components/certificate... components/certificate_transparency/tree_state_tracker.h:48: // one of the logs in the list provided during construction. On 2016/05/19 12:35:00, Eran Messeri wrote: > I understand the asymmetry problem of CHECK-failing when data related to unknown > logs arrives in one callback, but simply ignoring it in the other. > The way around it I can suggest (in a follow-up CL) is to add a filter to the > STHDistributor whose sole purpose would be to drop STHs from unknown logs. > Would that alleviate the concern, given we'd then be able to CHECK-fail in the > TreeStateTracker when either an SCT or an STH from an unknown log are provided? That's one approach, but the other I think makes it clearer the relationship between things, with a harder to misuse API. That is, with your proposed approach, the caller has to be aware of the documentation aspect of the classes to know that a filter is necessary. That is, it makes calling it correctly an "external" problem. Having this class, as an implementation detail, register the observer for the logs makes that logic internal, and thus impossible to misuse (short of screwing up the .cc). That makes it an "internal" problem - and thus, only one place to worry about, rather than N (every user of this class needing to filter out unknown SCTs) https://codereview.chromium.org/1845113003/diff/420001/components/certificate... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/1845113003/diff/420001/components/certificate... components/certificate_transparency/single_tree_tracker.h:80: // Should only be called with SCTs issued by the log this instance tracks. Seems like 78-79 and 80 should be swapped (80 documents calling convention, 78-79 is perhaps an implementation comment for the .cc) https://codereview.chromium.org/1845113003/diff/420001/components/certificate... components/certificate_transparency/single_tree_tracker.h:108: // https://crbug.com/506227 You still haven't addressed https://codereview.chromium.org/1845113003/diff/380001/components/certificate... Note: You can also link directly to comments (e.g. https://crbug.com/506227#c22 ). https://codereview.chromium.org/1845113003/diff/420001/components/certificate... File components/certificate_transparency/tree_state_tracker.h (right): https://codereview.chromium.org/1845113003/diff/420001/components/certificate... components/certificate_transparency/tree_state_tracker.h:32: // a particular browsing session. This comment should be deleted. It's documenting how it's used (by something above it) - not what it does. https://codereview.chromium.org/1845113003/diff/420001/components/certificate... components/certificate_transparency/tree_state_tracker.h:37: // be used in the DevTools Security panel, for example - crbug.com/506227 Same remarks about https://codereview.chromium.org/1845113003/diff/380001/components/certificate... (and https) https://codereview.chromium.org/1845113003/diff/420001/components/certificate... components/certificate_transparency/tree_state_tracker.h:48: // one of the logs in the list provided during construction. You changed the implementation, but didn't update the comment here (which can be removed for 46-48)
Addressed all comments, attempting to commit now. https://codereview.chromium.org/1845113003/diff/420001/components/certificate... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/1845113003/diff/420001/components/certificate... components/certificate_transparency/single_tree_tracker.h:80: // Should only be called with SCTs issued by the log this instance tracks. On 2016/05/19 17:20:57, Ryan Sleevi wrote: > Seems like 78-79 and 80 should be swapped (80 documents calling convention, > 78-79 is perhaps an implementation comment for the .cc) Done. https://codereview.chromium.org/1845113003/diff/420001/components/certificate... components/certificate_transparency/single_tree_tracker.h:108: // https://crbug.com/506227 On 2016/05/19 17:20:57, Ryan Sleevi wrote: > You still haven't addressed > https://codereview.chromium.org/1845113003/diff/380001/components/certificate... > > Note: You can also link directly to comments (e.g. https://crbug.com/506227#c22 > ). Done: Linked to https://crbug.com/613495 which explains in detail what should be done here. https://codereview.chromium.org/1845113003/diff/420001/components/certificate... File components/certificate_transparency/tree_state_tracker.h (right): https://codereview.chromium.org/1845113003/diff/420001/components/certificate... components/certificate_transparency/tree_state_tracker.h:32: // a particular browsing session. On 2016/05/19 17:20:57, Ryan Sleevi wrote: > This comment should be deleted. It's documenting how it's used (by something > above it) - not what it does. Done. https://codereview.chromium.org/1845113003/diff/420001/components/certificate... components/certificate_transparency/tree_state_tracker.h:37: // be used in the DevTools Security panel, for example - crbug.com/506227 On 2016/05/19 17:20:57, Ryan Sleevi wrote: > Same remarks about > https://codereview.chromium.org/1845113003/diff/380001/components/certificate... > (and https) Done (linked to comment 22 via https). https://codereview.chromium.org/1845113003/diff/420001/components/certificate... components/certificate_transparency/tree_state_tracker.h:48: // one of the logs in the list provided during construction. On 2016/05/19 17:20:57, Ryan Sleevi wrote: > You changed the implementation, but didn't update the comment here (which can be > removed for 46-48) Done.
The CQ bit was checked by eranm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sorin@chromium.org, rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/1845113003/#ps440001 (title: "Improving documentation.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1845113003/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1845113003/440001
Message was sent while issue was closed.
Description was changed from ========== Certificate Transparency: Start tracking the logs' STHs. This introduces the framework for actual inclusion checking of SCTs, and the certs the SCTs are associated with. This is done by introducing a new class, the TreeStateTracker, which can receive notifications of new STHs (which will be provided by the component updater), as well as notifications of verified STHs (provided by CTLogVerifiers). Right now, the only logic is tracking the state of observed SCTs to determine if they can be checked for inclusion or not - that is, whether or not an STH is available that is newer than the SCT's timestamp + the logs maximum merge delay. Except for freshly issued/logged certificates, this is expected to generally be the case. The next change will gather metrics about this. BUG=506227 ========== to ========== Certificate Transparency: Start tracking the logs' STHs. This introduces the framework for actual inclusion checking of SCTs, and the certs the SCTs are associated with. This is done by introducing a new class, the TreeStateTracker, which can receive notifications of new STHs (which will be provided by the component updater), as well as notifications of verified STHs (provided by CTLogVerifiers). Right now, the only logic is tracking the state of observed SCTs to determine if they can be checked for inclusion or not - that is, whether or not an STH is available that is newer than the SCT's timestamp + the logs maximum merge delay. Except for freshly issued/logged certificates, this is expected to generally be the case. The next change will gather metrics about this. BUG=506227 ==========
Message was sent while issue was closed.
Committed patchset #23 (id:440001)
Message was sent while issue was closed.
Description was changed from ========== Certificate Transparency: Start tracking the logs' STHs. This introduces the framework for actual inclusion checking of SCTs, and the certs the SCTs are associated with. This is done by introducing a new class, the TreeStateTracker, which can receive notifications of new STHs (which will be provided by the component updater), as well as notifications of verified STHs (provided by CTLogVerifiers). Right now, the only logic is tracking the state of observed SCTs to determine if they can be checked for inclusion or not - that is, whether or not an STH is available that is newer than the SCT's timestamp + the logs maximum merge delay. Except for freshly issued/logged certificates, this is expected to generally be the case. The next change will gather metrics about this. BUG=506227 ========== to ========== Certificate Transparency: Start tracking the logs' STHs. This introduces the framework for actual inclusion checking of SCTs, and the certs the SCTs are associated with. This is done by introducing a new class, the TreeStateTracker, which can receive notifications of new STHs (which will be provided by the component updater), as well as notifications of verified STHs (provided by CTLogVerifiers). Right now, the only logic is tracking the state of observed SCTs to determine if they can be checked for inclusion or not - that is, whether or not an STH is available that is newer than the SCT's timestamp + the logs maximum merge delay. Except for freshly issued/logged certificates, this is expected to generally be the case. The next change will gather metrics about this. BUG=506227 Committed: https://crrev.com/a824b1534a7a52132d1767a789d8cffd603ff20b Cr-Commit-Position: refs/heads/master@{#395045} ==========
Message was sent while issue was closed.
Patchset 23 (id:??) landed as https://crrev.com/a824b1534a7a52132d1767a789d8cffd603ff20b Cr-Commit-Position: refs/heads/master@{#395045} |
