Certificate Transparency: Fetching of Signed Tree Heads (DRAFT)

net/cert/ct_verifier.h

Index: net/cert/ct_verifier.h
diff --git a/net/cert/ct_verifier.h b/net/cert/ct_verifier.h
index 290a0474a649138733c902fdc5a8e47b6210f12d..18e187b07f9aae30ab1b2e3597cda2d1e779c1d1 100644
--- a/net/cert/ct_verifier.h
+++ b/net/cert/ct_verifier.h
@@ -5,21 +5,26 @@
 #ifndef NET_CERT_CT_VERIFIER_H_
 #define NET_CERT_CT_VERIFIER_H_
 
+#include "base/memory/ref_counted.h"
+#include "base/observer_list.h"
 #include "net/base/net_export.h"
 
 namespace net {
 
 namespace ct {
 struct CTVerifyResult;
+struct SignedCertificateTimestamp;
 }  // namespace ct
 
 class BoundNetLog;
+class CTLogVerifier;
 class X509Certificate;
 
 // Interface for verifying Signed Certificate Timestamps over a certificate.
 class NET_EXPORT CTVerifier {
  public:
-  virtual ~CTVerifier() {}
+  CTVerifier();
+  virtual ~CTVerifier();
 
   // Verifies SCTs embedded in the certificate itself, SCTs embedded in a
   // stapled OCSP response, and SCTs obtained via the
@@ -36,6 +41,39 @@ class NET_EXPORT CTVerifier {
                      const std::string& sct_list_from_tls_extension,
                      ct::CTVerifyResult* result,
                      const BoundNetLog& net_log) = 0;
+
+  class NET_EXPORT Observer {
+   public:
+    virtual ~Observer() {}
+

[davidben, 2015/05/07 21:59:38]

Add a comment for when this is called and what |verifier| means here (since you
have both CTVerifier and CTLogVerifier). Maybe verifier -> log_verifier or log?

[Eran Messeri, 2015/07/10 13:15:48]

Done.

+    virtual void OnSCTVerified(const ct::SignedCertificateTimestamp* sct,
+                               CTLogVerifier* verifier) {}

[davidben, 2015/05/07 21:59:38]

I wonder if this is better in MultiLogCTVerifier instead. Then you keep the
interface pure. CTVerifier doesn't know that a single verifier secretly
multiplexes between multiple CTLogVerifiers. It seems //net itself only cares
about the Verify hook. You could argue that anything keying off of OnSCTVerified
is a detail of MultiLogCTVerifier's Verify implementation: "hey net stack, here
is how you verify SCTs. Oh, don't worry, I'll deal with checking up with the log
asynchronously".

I don't feel that strongly either way. Mostly the CTLogVerifier part was a
little confusing.

[Eran Messeri, 2015/07/10 13:15:48]

The distinction between CTVerifier and MultiLogCTVerifier is a bit artificial -
mostly for describing a clear interface. I don't expect another class
implementing CTVerifier.

For now, if that's OK with you, let's leave it like this and change later if we
see the plumbing does not make sense.

[Ryan Sleevi, 2015/07/10 13:45:02]

I seem to recall us discussing this in person and agreeing that it wasn't
needed?

I do want to echo David's remark of not wanting to pass this along - would
prefer multiplexing be coalesced in a subclass.

[Ryan Sleevi, 2015/07/10 13:45:02]

Well, no, it was somewhat intentional, not artificial :)

For example, I anticipate we may have separate subclasses for V1 verifications
vs V2, although that remains TBD based on how much is changing (the precert
aspect is certainly one area of weirdness), such that the MultiLogCertVerifier
instantiates an appropriate Verifier subclass based on the version the log
supports, so that the MultiLog can handle (transparently) validation of both V1
and V2 SCTs.

But it was also to keep the separations cleaner - CTVerifier knows about the
crypto and the protocol, whereas MultiLogCTVerifier nows about the conceptual
mappings and collations and how to handle missing logs.

[Eran Messeri, 2015/07/13 10:58:22]

Acknowledged - the CTLogVerifier* is not needed here and indeed the
TreeStateTracker can hold onto the verifiers so there's no need to pass them in.

I agree with the comment about separation between crypto/protocol and conceptual
mappings in the MultiLogCTVerifier - it would make it much simpler to upgrade to
V2 with this approach.

+
+   protected:
+    Observer() {}
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(Observer);
+  };
+
+  // Registers |observer| to receive notifications of validated SCTs. The
+  // thread on which this is called is the thread on which |observer| will be
+  // called back with notifications.

[davidben, 2015/05/07 21:59:38]

Is this true? We're not using an ObserverListThreadSafe, don't need this
cross-threading behavior (only called on IO thread) and the rest of this
interface doesn't have much need for thread-safety.

I think it's better to use the default //net "everything is single-threaded"
contract.

[Eran Messeri, 2015/07/10 13:15:48]

Right, corrected the comment.

+  void AddObserver(Observer* observer);
+
+  // Unregisters |observer| from receiving notifications. This must be called
+  // on the same thread on which AddObserver() was called.
+  void RemoveObserver(Observer* observer);
+
+ protected:
+  void NotifyObserversOfSCTVerified(
+      scoped_refptr<ct::SignedCertificateTimestamp> sct,
+      CTLogVerifier* verifier);
+
+ private:
+  ObserverList<Observer> observer_list_;
+
+  DISALLOW_COPY_AND_ASSIGN(CTVerifier);
 };
 
 }  // namespace net