Certificate Transparency: Notify STH Observers of known STHs

net/cert/sth_distributor.cc

 // 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.
 
 #include "net/cert/sth_distributor.h"
 
 #include "base/metrics/histogram_macros.h"
 #include "base/time/time.h"
 #include "net/cert/signed_tree_head.h"
 
 namespace {
 const char kPilotLogID[33] =
     "\xa4\xb9\x09\x90\xb4\x18\x58\x14\x87\xbb\x13\xa2\xcc\x67\x70\x0a\x3c\x35"
     "\x98\x04\xf9\x1b\xdf\xb8\xe3\x77\xcd\x0e\xc8\x0d\xdc\x10";
 }
 
 namespace net {
 
 namespace ct {
 
+namespace {
+
+// Compares SignedTreeHeads by their log ID, to facilitate creation of a
+// map of STHs where there's only one STH for each log.

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

Documentation isn't correct because this isn't a map

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

Removed the struct so the documentation is no longer there.

+struct STHLogIDComparator {
+  STHLogIDComparator(const std::string& log_id) : log_id_(log_id) {}
+  bool operator()(const SignedTreeHead& other) {
+    return other.log_id == log_id_;
+  }
+
+ private:
+  std::string log_id_;
+};
+
+}  // namespace
+
 STHDistributor::STHDistributor()
     : observer_list_(base::ObserverList<STHObserver>::NOTIFY_EXISTING_ONLY) {}
 
 STHDistributor::~STHDistributor() {}
 
 void STHDistributor::NewSTHObserved(const SignedTreeHead& sth) {
+  auto it = std::find_if(observed_sths_.begin(), observed_sths_.end(),
+                         STHLogIDComparator(sth.log_id));

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

auto it = std::find_if(observed_sths_.begin(), observed_sths_.end(),
[&sth](const SignedTreeHead& other) { return sth.log_id == other.log_id });

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

Thanks, I wasn't aware lambdas were allowed in Chromium code.

+
+  if (it == observed_sths_.end())
+    observed_sths_.push_back(sth);
+  else
+    *it = sth;
+
+  for (auto observed_sth : observed_sths_) {
+    if (observed_sth.log_id == sth.log_id) {
+      observed_sth = sth;
+    }
+  }

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

Why do you do this for loop? It seems entirely redundant with 46-49?

Also, I feel like I've pointed out a few times before, but please read
https://chromium-cpp.appspot.com/ on Range-based for loops, and see the rule of
thumb.

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

My bad - leftover code. Removed. You have pointed out chromium-cpp resource
about Range-based for loops in the past. Removed.

+
   FOR_EACH_OBSERVER(STHObserver, observer_list_, NewSTHObserved(sth));
 
   if (sth.log_id.compare(0, sth.log_id.size(), kPilotLogID,
                          arraysize(kPilotLogID) - 1) != 0)
     return;
 
   const base::TimeDelta sth_age = base::Time::Now() - sth.timestamp;
   UMA_HISTOGRAM_CUSTOM_TIMES("Net.CertificateTransparency.PilotSTHAge", sth_age,
                              base::TimeDelta::FromHours(1),
                              base::TimeDelta::FromDays(4), 100);
 }
 
 void STHDistributor::RegisterObserver(STHObserver* observer) {
   observer_list_.AddObserver(observer);
+  for (const auto& sth : observed_sths_)
+    observer->NewSTHObserved(sth);
 }
 
 void STHDistributor::UnregisterObserver(STHObserver* observer) {
   observer_list_.RemoveObserver(observer);
 }
 
 }  // namespace ct
 
 }  // namespace net