Certificate Transparency: Notify STH Observers of known STHs

net/cert/sth_distributor.h

Index: net/cert/sth_distributor.h
diff --git a/net/cert/sth_distributor.h b/net/cert/sth_distributor.h
index 3d9691cc56c029a47c627f6aee0987d7e2b5c3cf..08fa332838051625048a4b125968bcbdba9f7c04 100644
--- a/net/cert/sth_distributor.h
+++ b/net/cert/sth_distributor.h
@@ -5,6 +5,8 @@
 #ifndef NET_CERT_STH_DISTRIBUTOR_H_
 #define NET_CERT_STH_DISTRIBUTOR_H_
 
+#include <vector>
+
 #include "base/observer_list.h"
 #include "net/base/net_export.h"
 #include "net/cert/sth_observer.h"
@@ -28,10 +30,20 @@ class NET_EXPORT STHDistributor : public STHObserver, public STHReporter {
   void NewSTHObserved(const SignedTreeHead& sth) override;
 
   // STHReporter implementation

[Ryan Sleevi, 2016/05/13 15:41:12]

[NOT FOR THIS CL]

I'm not sure how or why I missed this when you originally landed it, but now
this whole interface seems weird.

That is, STHReporter's interface fundamentally suggests that it handles
notifying all STHObservers. Of course, STHReporter isn't documented, so it's not
clear if or why this is *not* the case (e.g. can there only be one
RegisterObserver call?).

Put differently, if the contract is "STHReporter says there can only be one
RegisterObserver call", then this is NOT an STHReporter. If "STHReporter says
there can be many RegisterObserver calls", then *this* class (the
STHDistributor) seems redundant.

Further, given that this class is-a STHObserver, the whole abstraction thing
seems overwrought - whomever is taking an STHObserver wired up to the
STHDistributor here, aren't they an STHReporter? If not, what value is
STHReporter serving?

My concern here is that we have unnecessary interfaces that are non-obvious. I
apologize for letting this design land, I think I was a bit exhausted after the
original review, and I think we need to think carefully about this class
hierarchy, because it already seems like we have unnecessary abstractions here.

[Eran Messeri, 2016/05/16 13:55:14]

Acknowledged - it seems possible to remove the STHReporter interface.
The original reason for the creation of the STHReporter interface is role
separation, so it's clear that no code which deals with STHObserver
registration/unregirstration can publish new STHs to the STHDistributor and the
code that publishes new STHs can only do that, not manipulate the STHObservers
registered.

If that is not a valid approach in Chromium I'll remove the STHReporter
interface and leave only the STHDistributor.

+  // Registers |observer| for new STH notifications and invokes the
+  // new STH notification of this |observer| with observed STHs (one
+  // per log).
   void RegisterObserver(STHObserver* observer) override;
+
+  // Unregisters |observer|, should only be called with observers
+  // that were previously registered  via RegisterObserver;
   void UnregisterObserver(STHObserver* observer) override;
 
  private:
+  // STHs from logs, one for each log.
+  std::vector<SignedTreeHead> observed_sths_;

[Ryan Sleevi, 2016/05/13 15:41:12]

Why doesn't this need to be included or forward declared?

You fixed the vector issue, but now it seems like you're relying on
SignedTreeHead to be carried over from the STHObserver, and that's not
necessarily true.

That is, previously, the only use of SignedTreeHead was as part of implementing
the STHObserver interface; now, you've got more usages of it (outside of the
method signature), and you should either IWYU or forward declare, as
appropriate.

[Eran Messeri, 2016/05/16 13:55:14]

Done - as you point out I relied on the STHObserver include defining
SignedTreeHead, so I've included signed_tree_head.h.

+
+  // The observers for new STH notifications.
   base::ObserverList<STHObserver> observer_list_;
 };