Update SCT serialization format in Expect-CT reports

chrome/browser/ssl/chrome_expect_ct_reporter_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 "chrome/browser/ssl/chrome_expect_ct_reporter.h"
 
 #include <string>
 
 #include "base/base64.h"
 #include "base/command_line.h"
 #include "base/json/json_reader.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/test/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/values.h"
 #include "chrome/common/chrome_features.h"
 #include "content/public/test/test_browser_thread_bundle.h"
+#include "net/cert/ct_serialization.h"
 #include "net/cert/signed_certificate_timestamp_and_status.h"
 #include "net/test/cert_test_util.h"
 #include "net/test/test_data_directory.h"
 #include "net/test/url_request/url_request_failed_job.h"
 #include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
 #include "net/url_request/report_sender.h"
 #include "net/url_request/url_request_filter.h"
 #include "net/url_request/url_request_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
@@ skipping 74 matching lines @@
     EXPECT_EQ(pem_encoded_chain[i], cert_pem);
   }
 }
 
 // Converts the string value of a reported SCT's origin to a
 // net::ct::SignedCertificateTimestamp::Origin value.
 net::ct::SignedCertificateTimestamp::Origin SCTOriginStringToOrigin(
     const std::string& origin_string) {
   if (origin_string == "embedded")
     return net::ct::SignedCertificateTimestamp::SCT_EMBEDDED;
-  if (origin_string == "from-tls-extension")
+  if (origin_string == "tls-extension")
     return net::ct::SignedCertificateTimestamp::SCT_FROM_TLS_EXTENSION;
-  if (origin_string == "from-ocsp-response")
+  if (origin_string == "ocsp")
     return net::ct::SignedCertificateTimestamp::SCT_FROM_OCSP_RESPONSE;
   NOTREACHED();
   return net::ct::SignedCertificateTimestamp::SCT_EMBEDDED;
 }
 
-// Checks that an SCT |sct| appears (with the format determined by
-// |status|) in |report_list|, a list of SCTs from an Expect CT
-// report. |status| determines the format in that only certain fields
-// are reported for certain verify statuses; SCTs from unknown logs
-// contain very little information, for example, to avoid compromising
-// privacy.
-void FindSCTInReportList(
+// Checks that an SCT |sct| appears with status |status| in |report_list|, a
+// list of SCTs from an Expect-CT report.
+::testing::AssertionResult FindSCTInReportList(
     const scoped_refptr<net::ct::SignedCertificateTimestamp>& sct,
     net::ct::SCTVerifyStatus status,

[mattm, 2017/06/28 01:25:22]

could be clearer if these were named expected_sct and expected_status

[estark, 2017/06/28 04:44:03]

Done.

     const base::ListValue& report_list) {
-  bool found = false;
-  for (size_t i = 0; !found && i < report_list.GetSize(); i++) {
+  std::string expected_serialized_sct;
+  net::ct::EncodeSignedCertificateTimestamp(sct, &expected_serialized_sct);
+
+  for (size_t i = 0; i < report_list.GetSize(); i++) {
     const base::DictionaryValue* report_sct;
-    ASSERT_TRUE(report_list.GetDictionary(i, &report_sct));
+    EXPECT_TRUE(report_list.GetDictionary(i, &report_sct));

[mattm, 2017/06/28 01:25:23]

Shouldn't this one still be ASSERT_TRUE? Otherwise it might dereference an
uninitialized report_sct pointer. (Well I guess it can't use ASSERT_* since the
function has a return value, but it shouldn't continue if that fails)

[estark, 2017/06/28 04:44:03]

Done.

 
-    std::string origin;
-    ASSERT_TRUE(report_sct->GetString("origin", &origin));
+    std::string serialized_sct;
+    EXPECT_TRUE(report_sct->GetString("serialized_sct", &serialized_sct));
+    std::string decoded_serialized_sct;
+    EXPECT_TRUE(base::Base64Decode(serialized_sct, &decoded_serialized_sct));
+    if (decoded_serialized_sct != expected_serialized_sct)
+      continue;
 
+    std::string source;
+    EXPECT_TRUE(report_sct->GetString("source", &source));
+    EXPECT_EQ(sct->origin, SCTOriginStringToOrigin(source));
+
+    std::string report_status;
+    EXPECT_TRUE(report_sct->GetString("status", &report_status));
     switch (status) {
       case net::ct::SCT_STATUS_LOG_UNKNOWN:
-        // SCTs from unknown logs only have an origin.
-        EXPECT_FALSE(report_sct->HasKey("sct"));
-        EXPECT_FALSE(report_sct->HasKey("id"));
-        if (SCTOriginStringToOrigin(origin) == sct->origin)
-          found = true;
+        EXPECT_EQ("unknown", report_status);
         break;
-
       case net::ct::SCT_STATUS_INVALID_SIGNATURE:
       case net::ct::SCT_STATUS_INVALID_TIMESTAMP: {
-        // Invalid SCTs have a log id and an origin and nothing else.
-        EXPECT_FALSE(report_sct->HasKey("sct"));
-        std::string id_base64;
-        ASSERT_TRUE(report_sct->GetString("id", &id_base64));
-        std::string id;
-        ASSERT_TRUE(base::Base64Decode(id_base64, &id));
-        if (SCTOriginStringToOrigin(origin) == sct->origin && id == sct->log_id)
-          found = true;
+        EXPECT_EQ("invalid", report_status);
         break;
       }
-
       case net::ct::SCT_STATUS_OK: {
-        // Valid SCTs have the full SCT.
-        const base::DictionaryValue* report_sct_object;
-        ASSERT_TRUE(report_sct->GetDictionary("sct", &report_sct_object));
-        int version;
-        ASSERT_TRUE(report_sct_object->GetInteger("sct_version", &version));
-        std::string id_base64;
-        ASSERT_TRUE(report_sct_object->GetString("id", &id_base64));
-        std::string id;
-        ASSERT_TRUE(base::Base64Decode(id_base64, &id));
-        std::string extensions_base64;
-        ASSERT_TRUE(
-            report_sct_object->GetString("extensions", &extensions_base64));
-        std::string extensions;
-        ASSERT_TRUE(base::Base64Decode(extensions_base64, &extensions));
-        std::string signature_data_base64;
-        ASSERT_TRUE(
-            report_sct_object->GetString("signature", &signature_data_base64));
-        std::string signature_data;
-        ASSERT_TRUE(base::Base64Decode(signature_data_base64, &signature_data));
-
-        if (version == sct->version &&
-            SCTOriginStringToOrigin(origin) == sct->origin &&
-            id == sct->log_id && extensions == sct->extensions &&
-            signature_data == sct->signature.signature_data) {
-          found = true;
-        }
+        EXPECT_EQ("valid", report_status);
         break;
       }
       default:

[mattm, 2017/06/28 01:25:23]

same comment about avoiding default:

[estark, 2017/06/28 04:44:03]

Done.

         NOTREACHED();
     }
+    return ::testing::AssertionSuccess();
   }
-  EXPECT_TRUE(found);
+
+  return ::testing::AssertionFailure() << "Failed to find SCT in report list";
 }
 
 // Checks that all |expected_scts| appears in the given lists of SCTs
 // from an Expect CT report.
 void CheckReportSCTs(
     const net::SignedCertificateTimestampAndStatusList& expected_scts,
-    const base::ListValue& unknown_scts,
-    const base::ListValue& invalid_scts,
-    const base::ListValue& valid_scts) {
-  EXPECT_EQ(
-      expected_scts.size(),
-      unknown_scts.GetSize() + invalid_scts.GetSize() + valid_scts.GetSize());
+    const base::ListValue& scts) {
+  EXPECT_EQ(expected_scts.size(), scts.GetSize());
   for (const auto& expected_sct : expected_scts) {
-    switch (expected_sct.status) {
-      case net::ct::SCT_STATUS_LOG_UNKNOWN:
-        ASSERT_NO_FATAL_FAILURE(FindSCTInReportList(
-            expected_sct.sct, net::ct::SCT_STATUS_LOG_UNKNOWN, unknown_scts));
-        break;
-      case net::ct::SCT_STATUS_INVALID_SIGNATURE:
-        ASSERT_NO_FATAL_FAILURE(FindSCTInReportList(
-            expected_sct.sct, net::ct::SCT_STATUS_INVALID_SIGNATURE,
-            invalid_scts));
-        break;
-      case net::ct::SCT_STATUS_INVALID_TIMESTAMP:
-        ASSERT_NO_FATAL_FAILURE(FindSCTInReportList(
-            expected_sct.sct, net::ct::SCT_STATUS_INVALID_TIMESTAMP,
-            invalid_scts));
-        break;
-      case net::ct::SCT_STATUS_OK:
-        ASSERT_NO_FATAL_FAILURE(FindSCTInReportList(
-            expected_sct.sct, net::ct::SCT_STATUS_OK, valid_scts));
-        break;
-      default:
-        NOTREACHED();
-    }
+    ASSERT_TRUE(
+        FindSCTInReportList(expected_sct.sct, expected_sct.status, scts));
   }
 }
 
 // Checks that the |serialized_report| deserializes properly and
 // contains the correct information (hostname, port, served and
 // validated certificate chains, SCTs) for the given |host_port| and
 // |ssl_info|.
 void CheckExpectCTReport(const std::string& serialized_report,
                          const net::HostPortPair& host_port,
                          const std::string expiration_time,
                          const net::SSLInfo& ssl_info) {
   std::unique_ptr<base::Value> value(base::JSONReader::Read(serialized_report));
   ASSERT_TRUE(value);
   ASSERT_TRUE(value->IsType(base::Value::Type::DICTIONARY));
 
+  base::DictionaryValue* outer_report_dict;
+  ASSERT_TRUE(value->GetAsDictionary(&outer_report_dict));
+
   base::DictionaryValue* report_dict;
-  ASSERT_TRUE(value->GetAsDictionary(&report_dict));
+  ASSERT_TRUE(
+      outer_report_dict->GetDictionary("expect-ct-report", &report_dict));
 
   std::string report_hostname;
   EXPECT_TRUE(report_dict->GetString("hostname", &report_hostname));
   EXPECT_EQ(host_port.host(), report_hostname);
   int report_port;
   EXPECT_TRUE(report_dict->GetInteger("port", &report_port));
   EXPECT_EQ(host_port.port(), report_port);
 
   std::string expiration;
   EXPECT_TRUE(report_dict->GetString("effective-expiration-date", &expiration));
   EXPECT_EQ(expiration_time, expiration);
 
   const base::ListValue* report_served_certificate_chain = nullptr;
   ASSERT_TRUE(report_dict->GetList("served-certificate-chain",
                                    &report_served_certificate_chain));
   ASSERT_NO_FATAL_FAILURE(CheckReportCertificateChain(
       ssl_info.unverified_cert, *report_served_certificate_chain));
 
   const base::ListValue* report_validated_certificate_chain = nullptr;
   ASSERT_TRUE(report_dict->GetList("validated-certificate-chain",
                                    &report_validated_certificate_chain));
   ASSERT_NO_FATAL_FAILURE(CheckReportCertificateChain(
       ssl_info.cert, *report_validated_certificate_chain));
 
-  const base::ListValue* report_unknown_scts = nullptr;
-  ASSERT_TRUE(report_dict->GetList("unknown-scts", &report_unknown_scts));
-  const base::ListValue* report_invalid_scts = nullptr;
-  ASSERT_TRUE(report_dict->GetList("invalid-scts", &report_invalid_scts));
-  const base::ListValue* report_valid_scts = nullptr;
-  ASSERT_TRUE(report_dict->GetList("valid-scts", &report_valid_scts));
+  const base::ListValue* report_scts = nullptr;
+  ASSERT_TRUE(report_dict->GetList("scts", &report_scts));
 
-  ASSERT_NO_FATAL_FAILURE(CheckReportSCTs(
-      ssl_info.signed_certificate_timestamps, *report_unknown_scts,
-      *report_invalid_scts, *report_valid_scts));
+  ASSERT_NO_FATAL_FAILURE(
+      CheckReportSCTs(ssl_info.signed_certificate_timestamps, *report_scts));
 }
 
 // A test network delegate that allows the user to specify a callback to
 // be run whenever a net::URLRequest is destroyed.
 class TestExpectCTNetworkDelegate : public net::NetworkDelegateImpl {
  public:
   TestExpectCTNetworkDelegate()
       : url_request_destroyed_callback_(base::Bind(&base::DoNothing)) {}
 
   void set_url_request_destroyed_callback(const base::Closure& callback) {
@@ skipping 216 matching lines @@
   EXPECT_FALSE(sender->latest_serialized_report().empty());
   EXPECT_EQ("application/json; charset=utf-8", sender->latest_content_type());
   ASSERT_NO_FATAL_FAILURE(
       CheckExpectCTReport(sender->latest_serialized_report(), host_port,
                           kExpirationTimeStr, ssl_info));
 
   histograms.ExpectTotalCount(kFailureHistogramName, 0);
   histograms.ExpectTotalCount(kSendHistogramName, 1);
   histograms.ExpectBucketCount(kSendHistogramName, true, 1);
 }