Certificate Transparency: Start tracking logs' state

chrome/browser/io_thread.cc

Index: chrome/browser/io_thread.cc
diff --git a/chrome/browser/io_thread.cc b/chrome/browser/io_thread.cc
index 99a914c8229e65878c55c0d0a8716ddabe5f111d..d4eff9b3a9144f5bc0e8710b7c336914f82cc1cf 100644
--- a/chrome/browser/io_thread.cc
+++ b/chrome/browser/io_thread.cc
@@ -43,6 +43,7 @@
 #include "chrome/common/chrome_content_client.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
+#include "components/certificate_transparency/tree_state_tracker.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_prefs.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_params.h"
 #include "components/data_usage/core/data_use_aggregator.h"
@@ -69,10 +70,13 @@
 #include "net/cert/cert_verify_proc.h"
 #include "net/cert/ct_known_logs.h"
 #include "net/cert/ct_log_verifier.h"
+#include "net/cert/ct_observer.h"
 #include "net/cert/ct_policy_enforcer.h"
 #include "net/cert/ct_verifier.h"
 #include "net/cert/multi_log_ct_verifier.h"
 #include "net/cert/multi_threaded_cert_verifier.h"
+#include "net/cert/sth_observer.h"
+#include "net/cert/sth_reporter.h"
 #include "net/cookies/cookie_store.h"
 #include "net/dns/host_cache.h"
 #include "net/dns/host_resolver.h"
@@ -430,6 +434,7 @@ IOThread::IOThread(
       globals_(NULL),
       is_spdy_allowed_by_policy_(true),
       is_quic_allowed_by_policy_(true),
+      sth_reporter_(nullptr),
       creation_time_(base::TimeTicks::Now()),
       weak_factory_(this) {
   scoped_refptr<base::SingleThreadTaskRunner> io_thread_proxy =
@@ -522,6 +527,7 @@ IOThread::~IOThread() {
   BrowserThread::SetDelegate(BrowserThread::IO, NULL);
 
   pref_proxy_config_tracker_->DetachFromPrefService();
+  DCHECK(sth_observers_.empty());
   DCHECK(!globals_);
 }
 
@@ -870,8 +876,18 @@ void IOThread::CleanUp() {
 
   system_url_request_context_getter_ = NULL;
 
-  // Release objects that the net::URLRequestContext could have been pointing
-  // to.
+  // Since the cert_transparency_observer will be deleted first, unlink
+  // it from the cert_transparency_verifier by nullifying the observer.
+  globals()->cert_transparency_verifier->SetObserver(nullptr);
+
+  if (sth_reporter_) {
+    for (const auto& observer : sth_observers_) {
+      sth_reporter_->UnregisterObserver(observer);
+    }
+    // Make sure no registration happens after CleanUp
+    sth_reporter_ = nullptr;
+  }
+  sth_observers_.clear();
 
   // Shutdown the HistogramWatcher on the IO thread.
   net::NetworkChangeNotifier::ShutdownHistogramWatcher();
@@ -1215,6 +1231,15 @@ void IOThread::InitSystemRequestContextOnIOThread() {
 
   globals_->system_request_context.reset(
       ConstructSystemRequestContext(globals_, params_, net_log_));
+
+  globals_->cert_transparency_observer.reset(
+      new certificate_transparency::TreeStateTracker(globals_->ct_logs));
+  RegisterSTHObserver(globals_->cert_transparency_observer.get());
+  // The |cert_transparency_verifier| is the same one held by
+  // the |proxy_script_fetcher_context| and |system_request_context|,
+  // so no need to set the observer in their cert_transparency_verifiers.
+  globals_->cert_transparency_verifier->SetObserver(
+      globals_->cert_transparency_observer.get());
 }
 
 void IOThread::UpdateDnsClientEnabled() {
@@ -1388,6 +1413,36 @@ bool IOThread::NetworkSessionConfigurator::ShouldEnableQuicPortSelection(
   return false;  // Default to disabling port selection on all channels.
 }
 
+void IOThread::RegisterSTHReporter(net::ct::STHReporter* reporter) {
+  DCHECK(globals());
+  // A sanity check to make sure the observer was created by now.
+  DCHECK(globals()->cert_transparency_observer.get());

[Ryan Sleevi, 2016/04/27 23:05:59]

Why? The justification for why this is done is unclear.

[Eran Messeri, 2016/05/03 15:00:12]

Removed, this was to verify assumptions during development.
(Note this file was removed in the next patchset)

+
+  sth_reporter_ = reporter;
+  // Register all observers that were created before the reporter was.
+  for (const auto& observer : sth_observers_) {
+    sth_reporter_->RegisterObserver(observer);
+  }
+}
+
+void IOThread::RegisterSTHObserver(net::ct::STHObserver* observer) {
+  sth_observers_.insert(observer);
+  // If a reporter was set, also register this observer with it.
+  // Otherwise it will be registered in RegisterSTHReporter with all
+  // other pending observers.
+  if (sth_reporter_) {

[Ryan Sleevi, 2016/04/27 23:05:59]

Dominant style in this file seems to be no braces for one-lines.

[Eran Messeri, 2016/05/03 15:00:12]

Done.
(Note this file was removed in the next patchset)

+    sth_reporter_->RegisterObserver(observer);
+  }
+}
+
+void IOThread::UnregisterSTHObserver(net::ct::STHObserver* observer) {
+  DCHECK_NE(sth_observers_.count(observer), 0u);

[Ryan Sleevi, 2016/04/27 23:05:59]

Why? It's clear that you intend to make sure the observer is already registered,
but why don't you check during register if an object already has been registered
once before?

It's not clear what pattern(s) you're concerned about, because it feels
asymmetric.

[Eran Messeri, 2016/05/03 15:00:12]

I'd like to verify that the object was (1) registered only once and (2) that
everything that's unregistered was at one point registered.

To verify (1) I've added DCHECK_EQ(sth_observers_.count(observer), 0u) to
RegisterSTHObserver.

Does the DCHECK here now make sense?

+  sth_observers_.erase(observer);
+  if (sth_reporter_) {
+    sth_reporter_->UnregisterObserver(observer);
+  }
+}

[Ryan Sleevi, 2016/04/27 23:05:59]

Is this level of reciprocity (set a reporter it registers observers, set an
observer it registers reporters) necessary? It seems to make the API itself
somewhat confusing.

For example, what if someone does RegisterSTHReporter after an sth_reporter_ is
set? None of the observers are unregistered from the old reporter.

Is that a BUG? I don't know, because it's unclear what the right API usage
scenario is or should be.

What if I register an STH observer, set a new reporter, then unregister? That
seems like it would give to a BUG (the new sth_reporter_ doesn't have the
observer registered).

Is that a BUG? I don't know, because it's unclear what the right API usage
scenario is or should be.

[Eran Messeri, 2016/05/03 15:00:12]

Good point. I have:
(1) Added a DCHECK to verify that the sth_reporter_ is not set in
RegisterSTHReporter. Current design is to have exactly one sth_reporter_ during
the lifetime of IOThread (as I don't see a need to change it).
(2) Clarified in the documentation that RegisterSTHReporter is expected to be
called only once, all observers registered with RegisterSTHObserver before it
was called will be registered and all observers registered with
RegisterSTHObserver after it was called will be registered immediately.

+
 // static
 net::QuicTagVector
 IOThread::NetworkSessionConfigurator::GetQuicConnectionOptions(