Certificate Transparency: Discard entries pending auditing on network change

components/certificate_transparency/single_tree_tracker.h

Index: components/certificate_transparency/single_tree_tracker.h
diff --git a/components/certificate_transparency/single_tree_tracker.h b/components/certificate_transparency/single_tree_tracker.h
index eb9cd7f8ca0f178e894cc1a90444e7c84335070b..cf9ca88f612190aff8963aed43f91e89eb65744c 100644
--- a/components/certificate_transparency/single_tree_tracker.h
+++ b/components/certificate_transparency/single_tree_tracker.h
@@ -14,6 +14,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "net/base/hash_value.h"
+#include "net/base/network_change_notifier.h"
 #include "net/cert/ct_verifier.h"
 #include "net/cert/signed_tree_head.h"
 #include "net/cert/sth_observer.h"
@@ -55,8 +56,10 @@ class LogDnsClient;
 // 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.
-class SingleTreeTracker : public net::CTVerifier::Observer,
-                          public net::ct::STHObserver {
+class SingleTreeTracker
+    : public net::CTVerifier::Observer,
+      public net::ct::STHObserver,
+      public net::NetworkChangeNotifier::NetworkChangeObserver {

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

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.

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

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.

  public:
   enum SCTInclusionStatus {
     // SCT was not observed by this class and is not currently pending
@@ -99,6 +102,14 @@ class SingleTreeTracker : public net::CTVerifier::Observer,
   // Must only be called for STHs issued by the log this instance tracks.
   void NewSTHObserved(const net::ct::SignedTreeHead& sth) override;
 
+  // net::NetworkChangeNotifier::NetworkChangeObserver implementation.
+  // Discards all entries pending inclusion check on network change.
+  // That is done to prevent the client looking up inclusion proofs for
+  // certificates received from one network, on another network, thus
+  // leaking state between networks.
+  void OnNetworkChanged(
+      net::NetworkChangeNotifier::ConnectionType type) override;
+
   // Returns the status of a given log entry that is assembled from
   // |cert| and |sct|. If |cert| and |sct| were not previously observed,
   // |sct| is not an SCT for |cert| or |sct| is not for this log,