Certificate Transparency: Start tracking logs' state

components/certificate_transparency/single_tree_tracker_unittest.cc

Index: components/certificate_transparency/single_tree_tracker_unittest.cc
diff --git a/components/certificate_transparency/single_tree_tracker_unittest.cc b/components/certificate_transparency/single_tree_tracker_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..132404f3fdf186c75aad8f1629a83d7393ed9d43
--- /dev/null
+++ b/components/certificate_transparency/single_tree_tracker_unittest.cc
@@ -0,0 +1,131 @@
+// 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 "components/certificate_transparency/single_tree_tracker.h"
+
+#include <string>
+#include <utility>
+
+#include "base/strings/string_piece.h"
+#include "net/cert/ct_log_verifier.h"
+#include "net/cert/ct_serialization.h"
+#include "net/cert/signed_certificate_timestamp.h"
+#include "net/cert/signed_tree_head.h"
+#include "net/cert/x509_certificate.h"
+#include "net/test/ct_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace certificate_transparency {
+
+namespace {
+
+bool GetOldSignedTreeHead(net::ct::SignedTreeHead* sth) {
+  sth->version = net::ct::SignedTreeHead::V1;
+  sth->timestamp = base::Time::UnixEpoch() +
+                   base::TimeDelta::FromMilliseconds(INT64_C(1348589665525));
+  sth->tree_size = 12u;
+
+  const uint8_t kOldSTHRootHash[] = {
+      0x18, 0x04, 0x1b, 0xd4, 0x66, 0x50, 0x83, 0x00, 0x1f, 0xba, 0x8c,
+      0x54, 0x11, 0xd2, 0xd7, 0x48, 0xe8, 0xab, 0xbf, 0xdc, 0xdf, 0xd9,
+      0x21, 0x8c, 0xb0, 0x2b, 0x68, 0xa7, 0x8e, 0x7d, 0x4c, 0x23};
+  memcpy(sth->sha256_root_hash, kOldSTHRootHash, net::ct::kSthRootHashLength);
+
+  sth->log_id = net::ct::GetTestPublicKeyId();
+
+  const uint8_t kOldSTHSignatureData[] = {
+      0x04, 0x03, 0x00, 0x47, 0x30, 0x45, 0x02, 0x20, 0x15, 0x7b, 0x23,
+      0x42, 0xa2, 0x5f, 0x88, 0xc9, 0x0b, 0x30, 0xa6, 0xb4, 0x49, 0x50,
+      0xb3, 0xab, 0xf5, 0x25, 0xfe, 0x27, 0xf0, 0x3f, 0x9a, 0xbf, 0xc1,
+      0x16, 0x5a, 0x7a, 0xc0, 0x62, 0x2b, 0xbb, 0x02, 0x21, 0x00, 0xe6,
+      0x57, 0xa3, 0xfe, 0xfc, 0x5a, 0x82, 0x9b, 0x29, 0x46, 0x15, 0x1d,
+      0xbc, 0xfd, 0x9e, 0x87, 0x7f, 0xd0, 0x00, 0x5d, 0x62, 0x4f, 0x9a,
+      0x1a, 0x9f, 0x20, 0x79, 0xd0, 0xc1, 0x34, 0x2e, 0x08};
+  base::StringPiece sp(reinterpret_cast<const char*>(kOldSTHSignatureData),
+                       sizeof(kOldSTHSignatureData));
+  return DecodeDigitallySigned(&sp, &(sth->signature)) && sp.empty();
+}
+
+}  // namespace
+
+class SingleTreeTrackerTest : public ::testing::Test {
+  void SetUp() override {
+    log_ = net::CTLogVerifier::Create(net::ct::GetTestPublicKey(), "testlog",
+                                      "https://ct.example.com");
+
+    ASSERT_TRUE(log_);
+    ASSERT_EQ(log_->key_id(), net::ct::GetTestPublicKeyId());
+
+    tree_tracker_.reset(new SingleTreeTracker(log_));
+    const std::string der_test_cert(net::ct::GetDerEncodedX509Cert());
+    chain_ = net::X509Certificate::CreateFromBytes(der_test_cert.data(),
+                                                   der_test_cert.length());
+    ASSERT_TRUE(chain_.get());
+    net::ct::GetX509CertSCT(&cert_sct_);
+  }
+
+ protected:
+  scoped_refptr<const net::CTLogVerifier> log_;
+  std::unique_ptr<SingleTreeTracker> tree_tracker_;
+  scoped_refptr<net::X509Certificate> chain_;
+  scoped_refptr<net::ct::SignedCertificateTimestamp> cert_sct_;
+};
+
+TEST_F(SingleTreeTrackerTest, TestCorrectlyClassifiesUnobservedSCTNoSTH) {

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

Same remarks as the previous CL regarding being clearer about the tests here.
For example, it's unclear what "classifies" means, and what "NoSTH" means given
the call

Whenever in doubt, prefer documenting too much. You can see my comments below on
the vertical whitespace, which I think highlights the challenge here - trying to
figure out what it is  you're testing. Is 76-78 testing that an STH not observed
yet is "SCT_NOT_OBSERVED", or is it testing that when OnSCTVerified is called,
that the status is SCT_PENDING_NEWER_STH? The test name leaves it ambiguous as
to what's a precondition (e.g. making sure the test environment is sane) and
what's intrinsic to your logic (e.g. making sure that SCTs are in the
NOT_OBSERVED state)

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

Done, added documentation to all test cases.
(as a side note, other projects I've worked on had the philosophy that if the
name of the test + the test code aren't clear enough, then the test case is too
cumbersome, which is why I've left out documentation).

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

I think I'd disagree pretty heavily with that philosophy :)

Test code makes it easy to document what's being tested, but makes it impossible
to document what's "expected" - that is, whether an edge case (that may cause a
test to fail) was anticipated or not. That's why I find it useful to document
both the intent (e.g. in prosaic comments) and the actual execution, so that
when things fail, it can be clear whether they were intended to fail, or if
they're just failing for other reasons (e.g. the test was written poorly, the
edge case was unanticipated, the API has changed, etc)

Otherwise, when a test fails, the natural reaction is to 'fix' the test, without
understanding whether or not the failure is 'expected' or not.

+  EXPECT_EQ(
+      SingleTreeTracker::SCT_NOT_OBSERVED,
+      tree_tracker_->GetLogEntryInclusionStatus(chain_.get(), cert_sct_.get()));
+  tree_tracker_->OnSCTVerified(chain_.get(), cert_sct_.get());

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

Newline here between 78 & 79 - 

That is, it seems like 76-78 are testing an explicit condition (not a
precondition), line 79 is the state change, and line 81-83 are the
post-conditions/expectations of the state change. Is that correct? If so,
logically grouping them with vertical whitespace can help.

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

Done (added comments too).

+
+  EXPECT_EQ(
+      SingleTreeTracker::SCT_PENDING_NEWER_STH,
+      tree_tracker_->GetLogEntryInclusionStatus(chain_.get(), cert_sct_.get()));
+}
+
+TEST_F(SingleTreeTrackerTest,
+       TestCorrectlyClassifiesUnobservedSCTWithRecentSTH) {
+  net::ct::SignedTreeHead sth;
+  net::ct::GetSampleSignedTreeHead(&sth);
+  tree_tracker_->NewSTHObserved(sth);
+
+  EXPECT_EQ(
+      SingleTreeTracker::SCT_NOT_OBSERVED,
+      tree_tracker_->GetLogEntryInclusionStatus(chain_.get(), cert_sct_.get()));
+
+  tree_tracker_->OnSCTVerified(chain_.get(), cert_sct_.get());
+
+  EXPECT_EQ(
+      SingleTreeTracker::SCT_PENDING_INCLUSION_CHECK,
+      tree_tracker_->GetLogEntryInclusionStatus(chain_.get(), cert_sct_.get()));
+}
+
+TEST_F(SingleTreeTrackerTest, TestCorrectlyUpdatesSCTStatusOnNewSTH) {

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

the "Test" in all of these tests is redundant

SingleTreeTrackerTest.CorrectlyUpdatesSCTStatusOnNewSTH

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

Done.

+  tree_tracker_->OnSCTVerified(chain_.get(), cert_sct_.get());
+  EXPECT_EQ(
+      SingleTreeTracker::SCT_PENDING_NEWER_STH,
+      tree_tracker_->GetLogEntryInclusionStatus(chain_.get(), cert_sct_.get()));
+
+  net::ct::SignedTreeHead sth;
+  net::ct::GetSampleSignedTreeHead(&sth);
+  tree_tracker_->NewSTHObserved(sth);
+  EXPECT_EQ(
+      SingleTreeTracker::SCT_PENDING_INCLUSION_CHECK,
+      tree_tracker_->GetLogEntryInclusionStatus(chain_.get(), cert_sct_.get()));
+}
+
+TEST_F(SingleTreeTrackerTest, TestDoesNotUpdatesSCTStatusOnOldSTH) {
+  tree_tracker_->OnSCTVerified(chain_.get(), cert_sct_.get());
+  EXPECT_EQ(
+      SingleTreeTracker::SCT_PENDING_NEWER_STH,
+      tree_tracker_->GetLogEntryInclusionStatus(chain_.get(), cert_sct_.get()));
+
+  net::ct::SignedTreeHead sth;
+  GetOldSignedTreeHead(&sth);
+  tree_tracker_->NewSTHObserved(sth);
+  EXPECT_EQ(
+      SingleTreeTracker::SCT_PENDING_NEWER_STH,
+      tree_tracker_->GetLogEntryInclusionStatus(chain_.get(), cert_sct_.get()));
+}
+
+}  // namespace certificate_transparency