Certificate Transparency: Discard entries pending auditing on network change

components/certificate_transparency/single_tree_tracker.h

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef COMPONENTS_CERTIFICATE_TRANSPARENCY_SINGLE_TREE_TRACKER_H_
 #define COMPONENTS_CERTIFICATE_TRANSPARENCY_SINGLE_TREE_TRACKER_H_
 
 #include <map>
 #include <memory>
 #include <string>
 
 #include "base/containers/mru_cache.h"
 #include "base/memory/memory_pressure_monitor.h"
 #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"
 
 namespace net {
 
 class CTLogVerifier;
 class X509Certificate;
 
 namespace ct {
@@ skipping 21 matching lines @@
 // observed, their status is checked against the latest STH to ensure they were
 // properly logged. If an SCT is newer than the latest STH, then this class
 // verifies that when an STH is observed that should have incorporated those
 // SCTs, the SCTs (and their corresponding entries) are present in the log.
 //
 // 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.
-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
     // inclusion check. As there's no evidence the SCT this status relates
     // to is verified (it was never observed via OnSCTVerified), nothing
     // is done with it.
     SCT_NOT_OBSERVED,
 
     // SCT was observed but the STH known to this class is not old
     // enough to check for inclusion, so a newer STH is needed first.
@@ skipping 22 matching lines @@
   // here as this callback is invoked during certificate validation.
   void OnSCTVerified(net::X509Certificate* cert,
                      const net::ct::SignedCertificateTimestamp* sct) override;
 
   // net::ct::STHObserver implementation.
   // After verification of the signature over the |sth|, uses this
   // STH for future inclusion checks.
   // 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,
   // SCT_NOT_OBSERVED will be returned.
   SCTInclusionStatus GetLogEntryInclusionStatus(
       net::X509Certificate* cert,
       const net::ct::SignedCertificateTimestamp* sct);
 
  private:
   struct EntryToAudit;
@@ skipping 56 matching lines @@
   std::unique_ptr<base::MemoryPressureListener> memory_pressure_listener_;
 
   base::WeakPtrFactory<SingleTreeTracker> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(SingleTreeTracker);
 };
 
 }  // namespace certificate_transparency
 
 #endif  // COMPONENTS_CERTIFICATE_TRANSPARENCY_SINGLE_TREE_TRACKER_H_