Certificate Transparency: Start tracking logs' state

components/certificate_transparency/tree_state_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_TREE_STATE_TRACKER_H_
+#define COMPONENTS_CERTIFICATE_TRANSPARENCY_TREE_STATE_TRACKER_H_
+
+#include <map>
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "base/memory/ref_counted.h"
+#include "net/cert/ct_verifier.h"
+#include "net/cert/sth_observer.h"
+
+namespace net {
+class CTLogVerifier;
+class X509Certificate;
+
+namespace ct {
+struct SignedCertificateTimestamp;
+struct SignedTreeHead;
+}  // namespace ct
+
+}  // namespace net
+
+namespace certificate_transparency {
+class SingleTreeTracker;
+
+// Starting point for handling observed Certificate Transparency data in
+// a particular browsing session.
+// This class receives notifications of new Signed Tree Heads (STHs) and
+// verified Signed Certificate Timestamps (SCTs) and delegates them to
+// the SingleTreeTracker tracking the CT log they relate to.
+// TODO(eranm): Export the inclusion check status of SCTs+Certs so it can
+// be used in the DevTools Security panel, for example - crbug.com/506227
+class TreeStateTracker : public net::CTVerifier::Observer,
+                         public net::ct::STHObserver {
+ public:
+  // Track the set of logs provided in |ct_logs|. An instance of this class
+  // only tracks the logs provided in the constructor. The implementation
+  // is based on the assumption that the list of recognized logs does not change
+  // during the object's life time.
+  // Observed STHs from logs not in this list will be simply ignored.
+  // As the assumption is that all recognized logs are known during creation
+  // time, there's a CHECK in OnSCTVerified that the verified SCT is from
+  // one of the logs in the list provided during construction.

[Ryan Sleevi, 2016/05/18 16:58:19]

Better, but not quite - now that's documenting the details of the precondition,
rather than the precondition itself.

That said, I realize I really am quite uncomfortable with this precondition
asymmetry. I cannot logically make sense of it in this code, without knowing
that it's simply being used as a short-circuit for your higher level code (e.g.
which should only send STHs for which it's observed), or that we should be
ignoring unrecognized logs in OnSCTVerified.

Alternatively, we should be registering this class ourselves with ct_logs as the
Observer, so that we manage the conditions correctly (namely, that we're only
observing our logs - and that means removing the public inheritance from
net::CTVerifier::Observer and using an inner class helper to manage that
relationship, to guarantee no one sends us SCTs we don't care about)

I'd like to settle on a clearer separation between these, but I'd also like to
keep this CL small, so I think we need to address this as a design follow-up; in
the mean time

// Tracks the state of the logs provided in |ct_logs|.

[Eran Messeri, 2016/05/19 12:35:00]

I understand the asymmetry problem of CHECK-failing when data related to unknown
logs arrives in one callback, but simply ignoring it in the other. 
The way around it I can suggest (in a follow-up CL) is to add a filter to the
STHDistributor whose sole purpose would be to drop STHs from unknown logs.
Would that alleviate the concern, given we'd then be able to CHECK-fail in the
TreeStateTracker when either an SCT or an STH from an unknown log are provided?

[Ryan Sleevi, 2016/05/19 17:20:56]

That's one approach, but the other I think makes it clearer the relationship
between things, with a harder to misuse API.

That is, with your proposed approach, the caller has to be aware of the
documentation aspect of the classes to know that a filter is necessary. That is,
it makes calling it correctly an "external" problem.

Having this class, as an implementation detail, register the observer for the
logs makes that logic internal, and thus impossible to misuse (short of screwing
up the .cc). That makes it an "internal" problem - and thus, only one place to
worry about, rather than N (every user of this class needing to filter out
unknown SCTs) 

+  explicit TreeStateTracker(
+      std::vector<scoped_refptr<const net::CTLogVerifier>> ct_logs);
+  ~TreeStateTracker() override;
+
+  // net::ct::CTVerifier::Observer implementation.
+  // Delegates to the tree tracker corresponding to the log that issued the SCT.
+  // Must only be called with SCTs from logs present during construction.

[Ryan Sleevi, 2016/05/18 16:58:19]

Happy to drop 55/61 in light of the ctor comment.

[Eran Messeri, 2016/05/19 12:35:00]

Done.

+  void OnSCTVerified(net::X509Certificate* cert,
+                     const net::ct::SignedCertificateTimestamp* sct) override;
+
+  // net::ct::STHObserver implementation.
+  // Delegates to the tree tracker corresponding to the log that issued the STH.
+  // Does nothing if the STH is from a log not present during construction.
+  void NewSTHObserved(const net::ct::SignedTreeHead& sth) override;
+
+ private:
+  // Holds the SingleTreeTracker for each log
+  std::map<std::string, std::unique_ptr<SingleTreeTracker>> tree_trackers_;
+
+  DISALLOW_COPY_AND_ASSIGN(TreeStateTracker);
+};
+
+}  // namespace certificate_transparency
+
+#endif  // COMPONENTS_CERTIFICATE_TRANSPARENCY_TREE_STATE_TRACKER_H_