Certificate Transparency: Add observer for Signed Certificate Timestamps

net/cert/ct_verifier.h

 // Copyright 2013 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 NET_CERT_CT_VERIFIER_H_
 #define NET_CERT_CT_VERIFIER_H_
 
+#include <string>
+
+#include "base/memory/ref_counted.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() {}
+  class NET_EXPORT Observer {
+   public:
+    // Called for each Signed Certificate Timestamp from a known log that vas
+    // verified successfully (i.e. the signature verifies). |sct| is the
+    // Signed Certificate Timestamp, |log_verifier| is the log issuing this
+    // SCT.
+    // TODO(eranm): The certificate itself is also necessary for calculating
+    // the hash of the log entry. Will be added when implementing inclusion
+    // checking functionality.

[Ryan Sleevi, 2015/07/02 13:23:08]

Why not have this interface provide the certificate now? Doesn't seem to be
coupled to inclusion checking.

[Eran Messeri, 2015/07/02 14:12:51]

Done.

+    virtual void OnSCTVerified(const ct::SignedCertificateTimestamp* sct,
+                               scoped_refptr<CTLogVerifier> log_verifier) = 0;

[Ryan Sleevi, 2015/07/02 13:23:08]

FIRST ORDER DESIGN: Why pass this as a scoped_refptr<> [if it was, prefer
const-ref]

That is, why not pass it as CTLogVerifier*? Is there an asynchronous delivery
issue (doesn't seem to be? Certainly not from the base interface)

SECOND ORDER DESIGN: Why pass the verifier at all? This presumes a particular
implementation strategy (that some higher-order consumer will re-use the same
Observer across multiple Verifiers), but it's questionable about whether or not
that should influence the base design.

If it does, then it arguably warrants a discussion via comments on the Observer
that the same Observer may be used across multiple verifiers, so as to make it
obvious/intuitive to the reader that providing the Verifier via OnSCTVerified is
an intentional design decision of an intended API contract, rather than just an
incidental effect of weird layering.

nit: Assuming that the second order doesn't change, I'm slightly inclined to
suggest the order is |log_verifier|, |sct| rather than |sct|, |log_verifier|,
since if there's an argument for keeping |log_verifier| in the API signature,
it's likely the high-order bit for processing, and thus somewhat argues for
providing first.

[Eran Messeri, 2015/07/02 14:12:52]

Good point, the observer can hold a list of CTLogVerifiers and match the right
one based on the log_id in the SCT, so it's unnecessary here. Removed.

+  };
+
+  virtual ~CTVerifier();

[Ryan Sleevi, 2015/07/02 13:23:08]

It's unclear why you're no longer inlining this? This class remains a pure
interface class, thus inlining the dtor is fine.

[Eran Messeri, 2015/07/02 14:12:51]

Reverted.

 
   // Verifies SCTs embedded in the certificate itself, SCTs embedded in a
   // stapled OCSP response, and SCTs obtained via the
   // signed_certificate_timestamp TLS extension on the given |cert|.
   // A certificate is permitted but not required to use multiple sources for
   // SCTs. It is expected that most certificates will use only one source
   // (embedding, TLS extension or OCSP stapling). If no stapled OCSP response
   // is available, |stapled_ocsp_response| should be an empty string. If no SCT
   // TLS extension was negotiated, |sct_list_from_tls_extension| should be an
   // empty string. |result| will be filled with the SCTs present, divided into
   // categories based on the verification result.
   virtual int Verify(X509Certificate* cert,
                      const std::string& stapled_ocsp_response,
                      const std::string& sct_list_from_tls_extension,
                      ct::CTVerifyResult* result,
                      const BoundNetLog& net_log) = 0;
+
+  virtual void StopNotifications() = 0;

[Ryan Sleevi, 2015/07/02 13:23:08]

Document.

Seems like this would be more logical after SetObserver.

DESIGN: Does it make more sense to do SetObserver(nullptr) as the unregistration
action? Why or why not?

[Eran Messeri, 2015/07/02 14:12:52]

Using SetObserver(nullptr) would definitely do the job, removing this method.

+
+  // Registers |observer| to receive notifications of validated SCTs. Does not
+  // take ownership of the observer as the observer may be performing
+  // URLRequests which have to be cancelled before this object is destroyed.
+  virtual void SetObserver(Observer* observer) = 0;
 };
 
 }  // namespace net
 
 #endif  // NET_CERT_CT_VERIFIER_H_