Update SCT serialization format in Expect-CT reports

chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc

Index: chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc
diff --git a/chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc b/chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc
index 5ea097e20a26fdd69ac5ef010df9d2aab4be1bec..3f3bded7f41400c0ddd430ac94b88726c87fc373 100644
--- a/chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc
+++ b/chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc
@@ -16,6 +16,7 @@
 #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"
@@ -110,123 +111,71 @@ 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 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 origin;
-    ASSERT_TRUE(report_sct->GetString("origin", &origin));
+    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));
   }
 }
 
@@ -242,8 +191,12 @@ void CheckExpectCTReport(const std::string& 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));
@@ -268,16 +221,11 @@ void CheckExpectCTReport(const std::string& serialized_report,
   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