|
|
Created:
3 years, 10 months ago by Eran Messeri Modified:
3 years, 10 months ago CC:
chromium-reviews, rsleevi+watch_chromium.org, certificate-transparency-chrome_googlegroups.com, martijn+crwatch_martijnc.be, Eran Messeri, msramek Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCertificate Transparency: Discard entries pending auditing on network change
On network change, discard all the CT log entries that are pending auditing
in the SingleTreeTracker.
Doing so will ensure inclusion proofs for these log entries are not looked up
on a different network than the one in which the original certificates
were obtained, thus preventing state leak between networks.
BUG=506227
Review-Url: https://codereview.chromium.org/2668803004
Cr-Commit-Position: refs/heads/master@{#447982}
Committed: https://chromium.googlesource.com/chromium/src/+/2d5b01268c06d638debcebbde4c2b204c55798aa
Patch Set 1 #
Total comments: 4
Patch Set 2 : Move listener to private class #Patch Set 3 : Merging with master #
Messages
Total messages: 23 (17 generated)
Description was changed from ========== Certificate Transparency: Discard entries pending auditing on network change On network change, discard all the CT log entries that are pending auditing in the SingleTreeTracker. Doing so will ensure inclusion proofs for these log entries are not looked up on a different network than the one in which the original certificates were obtained, thus preventing state leak between networks. BUG=506227 ========== to ========== Certificate Transparency: Discard entries pending auditing on network change On network change, discard all the CT log entries that are pending auditing in the SingleTreeTracker. Doing so will ensure inclusion proofs for these log entries are not looked up on a different network than the one in which the original certificates were obtained, thus preventing state leak between networks. BUG=506227 ==========
eranm@chromium.org changed reviewers: + robpercival@chromium.org, rsleevi@chromium.org
The CQ bit was checked by eranm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM % updating the test. If you do integrate any of the more substantive nits (and you're not required to), I'd ask you have me re-review since that may change things more significantly. https://codereview.chromium.org/2668803004/diff/1/components/certificate_tran... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/2668803004/diff/1/components/certificate_tran... components/certificate_transparency/single_tree_tracker.h:62: public net::NetworkChangeNotifier::NetworkChangeObserver { nit: I'm not going to block this CL, but one thing I like to encourage people to consider is whether there is 'too much' multiple inheritence going on. The usual threshold I suggest is "Does the consumer of this class need to know it implements this particular interface in order to use it, or is implementing this particular interface an implementation detail of how this class works" I haven't carefully looked at the CTVerifier::Observer/STHObserver side, but the NCO is definitely an opaque implementation detail (because it self-registers itself, rather than explicit registration) The way I recommend people handle this is to consider encapsulating that implementation in a helper class that hides these details. It does mean more code (particularly 'boiler-platey'), but better hides your implementation details, while also better encapsulates the dependency-inversion principle of dealing with abstractions. Concretely, if you removed the public inheritance, then you could alternatively: private: class NetworkObserver; friend class NetworkObserver; void ResetPendingQueue(); std::unique_ptr<NetworkObserver> observer_; and in the .cc class SingleTreeTracker::NetworkObserver : public net::NetworkChangeNotifier::NetworkChangeObserver { public: explicit NetworkObserver(SingleTreeTracker* parent) : parent_(parent) { AddObserver(this); } ~NetworkObserver() { RemoveObserver(this); } void OnNetworkChanged(...) { parent_->ResetPendingQueue(); } SingleTreeTracker* parent_; } The pros to this approach are: - Don't expose that you're an NCO to the world (anyone is allowed to call that public method) - Don't make your .h coupled to NCO's .h (reducing coupling/better encapsulating) - Makes the STT's interface more obvious about what it logically does, rather than how The cons to this approach are: - Heap allocation for the pimpl pattern Finding where that balance is is predominantly a judgement call that depends; you can over pimpl and make code more confusing, unquestionably, but it can offer better encapsulation and separation of concerns. Further, that particular pimpl (of interface-implementation) encapsulates the Adapter pattern that loosens coupling and often shakes out more design issues. Anyways, this is broadly a $.02 approach on something to consider, but not a hard request. https://codereview.chromium.org/2668803004/diff/1/components/certificate_tran... File components/certificate_transparency/single_tree_tracker_unittest.cc (right): https://codereview.chromium.org/2668803004/diff/1/components/certificate_tran... components/certificate_transparency/single_tree_tracker_unittest.cc:674: net::NetworkChangeNotifier::CONNECTION_UNKNOWN); Why are you pumping this method directly, rather than using the Mock you create on line 169? It would seem that it would better cover the test scenario (e.g. that you remembered to register the STT as an NCO) by pumping the mock and making sure it's reflected in the STT.
The CQ bit was checked by eranm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Ryan, PTAL since I've adopted your suggestion. https://codereview.chromium.org/2668803004/diff/1/components/certificate_tran... File components/certificate_transparency/single_tree_tracker.h (right): https://codereview.chromium.org/2668803004/diff/1/components/certificate_tran... components/certificate_transparency/single_tree_tracker.h:62: public net::NetworkChangeNotifier::NetworkChangeObserver { On 2017/02/01 22:26:58, Ryan Sleevi wrote: > nit: I'm not going to block this CL, but one thing I like to encourage people to > consider is whether there is 'too much' multiple inheritence going on. > > The usual threshold I suggest is "Does the consumer of this class need to know > it implements this particular interface in order to use it, or is implementing > this particular interface an implementation detail of how this class works" > > I haven't carefully looked at the CTVerifier::Observer/STHObserver side, but the > NCO is definitely an opaque implementation detail (because it self-registers > itself, rather than explicit registration) > > The way I recommend people handle this is to consider encapsulating that > implementation in a helper class that hides these details. It does mean more > code (particularly 'boiler-platey'), but better hides your implementation > details, while also better encapsulates the dependency-inversion principle of > dealing with abstractions. > > Concretely, if you removed the public inheritance, then you could alternatively: > > private: > class NetworkObserver; > friend class NetworkObserver; > > void ResetPendingQueue(); > > std::unique_ptr<NetworkObserver> observer_; > > > and in the .cc > > class SingleTreeTracker::NetworkObserver : public > net::NetworkChangeNotifier::NetworkChangeObserver { > public: > explicit NetworkObserver(SingleTreeTracker* parent) : parent_(parent) { > AddObserver(this); > } > ~NetworkObserver() { > RemoveObserver(this); > } > > void OnNetworkChanged(...) { > parent_->ResetPendingQueue(); > } > > SingleTreeTracker* parent_; > } > > > The pros to this approach are: > - Don't expose that you're an NCO to the world (anyone is allowed to call that > public method) > - Don't make your .h coupled to NCO's .h (reducing coupling/better > encapsulating) > - Makes the STT's interface more obvious about what it logically does, rather > than how > > The cons to this approach are: > - Heap allocation for the pimpl pattern > > > Finding where that balance is is predominantly a judgement call that depends; > you can over pimpl and make code more confusing, unquestionably, but it can > offer better encapsulation and separation of concerns. Further, that particular > pimpl (of interface-implementation) encapsulates the Adapter pattern that > loosens coupling and often shakes out more design issues. > > Anyways, this is broadly a $.02 approach on something to consider, but not a > hard request. Done - adopted your suggestion of having a private class that implements the NetworkChangeObserver and registers itself. I agree that implementing too many interfaces leads to messy code and bloat in the class that implements them all, so I've switched to the private class you suggested, especially since (as you pointed out) it registers itself. https://codereview.chromium.org/2668803004/diff/1/components/certificate_tran... File components/certificate_transparency/single_tree_tracker_unittest.cc (right): https://codereview.chromium.org/2668803004/diff/1/components/certificate_tran... components/certificate_transparency/single_tree_tracker_unittest.cc:674: net::NetworkChangeNotifier::CONNECTION_UNKNOWN); On 2017/02/01 22:26:58, Ryan Sleevi wrote: > Why are you pumping this method directly, rather than using the Mock you create > on line 169? > > It would seem that it would better cover the test scenario (e.g. that you > remembered to register the STT as an NCO) by pumping the mock and making sure > it's reflected in the STT. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by eranm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by eranm@chromium.org
The CQ bit was checked by eranm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/2668803004/#ps40001 (title: "Merging with master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486124865439800, "parent_rev": "e1b2ceb34172034512c26b014f8c2e4900187171", "commit_rev": "2d5b01268c06d638debcebbde4c2b204c55798aa"}
Message was sent while issue was closed.
Description was changed from ========== Certificate Transparency: Discard entries pending auditing on network change On network change, discard all the CT log entries that are pending auditing in the SingleTreeTracker. Doing so will ensure inclusion proofs for these log entries are not looked up on a different network than the one in which the original certificates were obtained, thus preventing state leak between networks. BUG=506227 ========== to ========== Certificate Transparency: Discard entries pending auditing on network change On network change, discard all the CT log entries that are pending auditing in the SingleTreeTracker. Doing so will ensure inclusion proofs for these log entries are not looked up on a different network than the one in which the original certificates were obtained, thus preventing state leak between networks. BUG=506227 Review-Url: https://codereview.chromium.org/2668803004 Cr-Commit-Position: refs/heads/master@{#447982} Committed: https://chromium.googlesource.com/chromium/src/+/2d5b01268c06d638debcebbde4c2... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/2d5b01268c06d638debcebbde4c2... |