Certificate Transparency: Notify STH Observers of known STHs

net/cert/sth_distributor_unittest.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 <map>
+#include <string>
+
+#include "base/test/histogram_tester.h"
+#include "crypto/sha2.h"
+#include "net/cert/signed_tree_head.h"
+#include "net/cert/sth_observer.h"
+#include "net/test/ct_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace net {
+
+namespace ct {
+
+namespace {
+
+// An STHObserver implementation that simply stores all
+// observed STHs, keyed by log ID.
+class StoringSTHObserver : public STHObserver {
+ public:
+  void NewSTHObserved(const SignedTreeHead& sth) override {
+    sths[sth.log_id] = sth;
+  }
+
+  std::map<std::string, SignedTreeHead> sths;
+};
+
+class STHDistributorTest : public ::testing::Test {
+ public:
+  STHDistributorTest() {}
+
+  void SetUp() override {
+    ASSERT_TRUE(GetSampleSignedTreeHead(&sample_sth_));
+    sample_sth_.log_id = GetTestPublicKeyId();
+  }
+
+ protected:
+  STHDistributor distributor_;
+  SignedTreeHead sample_sth_;
+};
+
+TEST_F(STHDistributorTest, NotifiesOfExistingSTHs) {

[Ryan Sleevi, 2016/05/17 17:57:17]

Test style - I haven't pushed as hard on this with the CTVerifier tests, but I
regret it, because it actually was rather confusing.

I'm much more inclined to focus on making sure there's clear documentation in
the test, beyond just the name, that tries to capture the *why*, and not just
the *what* (that the name captures)

// Test that registering a new observer, after some set of STHs have been
observed, will notify the newly
// added observer of the most recent STHs.
TEST_F(...)

[Eran Messeri, 2016/05/17 21:35:07]

Done.

+  const std::string other_log = "another log";
+  SignedTreeHead second_sth(sample_sth_);
+  second_sth.log_id = other_log;
+
+  distributor_.NewSTHObserved(sample_sth_);
+  distributor_.NewSTHObserved(second_sth);
+
+  StoringSTHObserver observer;
+  distributor_.RegisterObserver(&observer);
+
+  // Check that two STHs from different logs received prior to observer
+  // registration were reported to the observer once registered.
+  EXPECT_EQ(2u, observer.sths.size());
+  EXPECT_EQ(1u, observer.sths.count(other_log));
+}
+
+TEST_F(STHDistributorTest, LogsUMAForPilotSTH) {

[Ryan Sleevi, 2016/05/17 17:57:17]

// Test that histograms are properly recorded for [blah blah] of the Google
Pilot Log, as they're
// used for [blah blah].

[Eran Messeri, 2016/05/17 21:35:07]

Done.

+  const char kPilotSTHAgeHistogram[] =
+      "Net.CertificateTransparency.PilotSTHAge";
+  base::HistogramTester histograms;
+  histograms.ExpectTotalCount(kPilotSTHAgeHistogram, 0);
+
+  const uint8_t kPilotLogID[] = {
+      0xa4, 0xb9, 0x09, 0x90, 0xb4, 0x18, 0x58, 0x14, 0x87, 0xbb, 0x13,
+      0xa2, 0xcc, 0x67, 0x70, 0x0a, 0x3c, 0x35, 0x98, 0x04, 0xf9, 0x1b,
+      0xdf, 0xb8, 0xe3, 0x77, 0xcd, 0x0e, 0xc8, 0x0d, 0xdc, 0x10};
+  sample_sth_.log_id = std::string(reinterpret_cast<const char*>(kPilotLogID),
+                                   crypto::kSHA256Length);
+
+  distributor_.NewSTHObserved(sample_sth_);
+  histograms.ExpectTotalCount(kPilotSTHAgeHistogram, 1);
+}
+
+TEST_F(STHDistributorTest, UpdatesObservedSTHData) {

[Ryan Sleevi, 2016/05/17 17:57:17]

Test that... blah

[Eran Messeri, 2016/05/17 21:35:07]

Done.

+  uint64_t orig_tree_size = sample_sth_.tree_size;
+  distributor_.NewSTHObserved(sample_sth_);
+  sample_sth_.tree_size += 1;
+  distributor_.NewSTHObserved(sample_sth_);
+
+  StoringSTHObserver observer;
+  distributor_.RegisterObserver(&observer);
+  EXPECT_EQ(1u, observer.sths.size());
+  EXPECT_EQ(orig_tree_size + 1, observer.sths[GetTestPublicKeyId()].tree_size);

[Ryan Sleevi, 2016/05/17 17:57:17]

This is even less clear than what you originally had. Why store orig_tree_size?
Why not just

EXPECT_EQ(sample_sth_.tree_size, observer.sths[...].tree_size)

There's also the concern that perhaps I didn't make clearer, which is that, as
this function reads, it's "magic". It's not documented sufficiently to explain
what you're testing, why you're changing the value, or how it accomplishes the
result.

It's definitely been something I've encouraged more of - clearer documentation
not just in the test name, but how the test accomplishes its goal.

Consider something like:

// Observe an initial STH
StoringSTHObserver observer;
distributor_.RegisterObserver(&observer);

distributor.NewSTHObserved(sample_sth_);

EXPECT_EQ(1u, observer.sths.size());
EXPECT_EQ(sample_sth_, observer.sths[GetTestPublicKeyId()]);

// Observe a new STH. "new" simply means that it is a more recently observed
SignedTreeHead
// for the given log ID, not necessarily that it's newer chronologically (the
timestamp) or the log
// state (the tree size).
// To make sure the more recently observed SignedTreeHead is returned, just
modify some fields.
SignedTreeHead new_sth = sample_sth;
new_sth.tree_size++;
new_sth.timestamp -= base::TimeDelta::FromSeconds(3);

distributor_.NewSTHObserved(new_sth);

// The STH should have been broadcast to existing observers.
EXPECT_EQ(1u, observer.sths.size());
EXPECT_NE(sample_sth_, observer.sths[GetTestPublicKeyId()]);
EXPECT_EQ(new_sth, observer.sths[GetTestPublicKeyId()]);

// Registering a new observer should only receive the most recently observed STH
StoringSTHObserver new_observer;
distributor_.RegisterObserver(&new_observer);
EXPECT_EQ(1u, new_observer.sths.size());
EXPECT_NE(sample_sth_, new_observer.sths[GetTestPublicKeyId()]);
EXPECT_EQ(new_sth, new_observer.sths[GetTestPublicKeyId()]);



This tries to make it clear what the conditions you are testing. It tries to
actually test the condition - don't just test the tree_size, make sure that no
other fields were mutated. It tries to cover both scenarios relevant for testing
here. Does this make it clearer? I probably have bugs in the above, but it tries
to document clearly what's going on.

[Eran Messeri, 2016/05/17 21:35:07]

Done - adapted your suggestion, thanks for providing such a clear test case.
Note I had to implement operator== and operator!= for SignedTreeHead for this to
work.

+}
+
+}  // namespace
+
+}  // namespace ct
+
+}  // namespace net