Certificate Transparency: Discard entries pending auditing on network change

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/+/2d5b01268c06d638debcebbde4c2b204c55798aa

[Eran Messeri, 2017-02-01 11:36:49]

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
==========

[Eran Messeri, 2017-02-01 11:36:49]

eranm@chromium.org changed reviewers:
+ robpercival@chromium.org, rsleevi@chromium.org

[Eran Messeri, 2017-02-01 14:14:19]

The CQ bit was checked by eranm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-01 14:14:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-01 15:36:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-01 15:36:21]

Dry run: This issue passed the CQ dry run.

[Ryan Sleevi, 2017-02-01 22:26:58]

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.

[Eran Messeri, 2017-02-02 14:54:07]

The CQ bit was checked by eranm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-02 14:54:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eran Messeri, 2017-02-02 14:54:40]

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.

[commit-bot: I haz the power, 2017-02-02 16:46:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-02 16:46:48]

Dry run: This issue passed the CQ dry run.

[Ryan Sleevi, 2017-02-02 22:09:54]

lgtm

[Eran Messeri, 2017-02-03 11:53:19]

The CQ bit was checked by eranm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 11:53:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eran Messeri, 2017-02-03 12:27:32]

The CQ bit was unchecked by eranm@chromium.org

[Eran Messeri, 2017-02-03 12:27:45]

The CQ bit was checked by eranm@chromium.org

[Eran Messeri, 2017-02-03 12:27:45]

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")

[commit-bot: I haz the power, 2017-02-03 12:27:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 13:04:25]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486124865439800,
"parent_rev": "e1b2ceb34172034512c26b014f8c2e4900187171", "commit_rev":
"2d5b01268c06d638debcebbde4c2b204c55798aa"}

[commit-bot: I haz the power, 2017-02-03 13:05:02]

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...
==========

[commit-bot: I haz the power, 2017-02-03 13:05:03]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/2d5b01268c06d638debcebbde4c2...