Add the ability to send Expect-Staple reports.

net/http/transport_security_state_unittest.cc

Index: net/http/transport_security_state_unittest.cc
diff --git a/net/http/transport_security_state_unittest.cc b/net/http/transport_security_state_unittest.cc
index 20c5619555b49809c75dd4c8fe26238f6fedf622..6a26efa01fc43b89e46f1ea4b1bedb081905656f 100644
--- a/net/http/transport_security_state_unittest.cc
+++ b/net/http/transport_security_state_unittest.cc
@@ -240,6 +240,132 @@ void CheckHPKPReport(
       validated_certificate_chain, report_validated_certificate_chain));
 }
 
+bool ResponseStatusFromString(std::string in,

[Ryan Sleevi, 2016/07/19 00:02:49]

From a testing design standpoint, it doesn't seem you need this. If you're
wanting to test the serialized reports, shouldn't you do the string comparison?

You only call this function once, does it really need to be a helper?

[dadrian, 2016/07/19 00:18:54]

I did it this way since the reverse function is in a private namespace in a .cc
file. That, and testing it against itself doesn't actually test we set the right
value---this way I'd have to make the same naming error twice to miss a name
mismatch.

I could move it back into CheckExpectStaple(), but this seemed a little more
clear the first time around.

[Ryan Sleevi, 2016/07/19 01:17:22]

Unfortunately, I'm having a lot of trouble understanding your reply, so I'll try
to phrase it differently to see if perhaps I didn't communicate it well.

I was specifically trying to suggest that, rather than lines 302 - 305, which
parse the string value into a numeric value and compare the numeric value, what
you should be testing is that the string value is what is expected. And that
means supplying the string value you expect.

That is, instead of 302-305, you have
EXPECT_EQ(some_variable_you_pass_as_a_string, report_response_status)

This function seems superfluous - you're trying to test that the report is
properly generated, but what you're testing here is that the report was properly
generated and your utility function doesn't have issues. And that seems to be
unnecessary to test.

Naturally, this means threading the string parameter through both
CheckExpectStapleReport and CheckExpectStaple, which then *also* means that
lines 2063-2070 need to supply the numeric value to use to construct the report,
and the expected string value of the constructed report.

The reason I mention all of this is that it seems you should be more explicit
that you're expecting a specific string value in the test; this mapping feels
like it duplicates code under test (as you note, it's a counterpart to a .cc),
and isn't "really" what you're wanting to test (you're wanting to test the
string is a specific value, but you're hiding it via an enum for convenience)

[dadrian, 2016/07/19 18:48:40]

I reworked the tests to be table-driven with TEST_P. I was trying to avoid that
the first time around, primarily because I hate creating TEST_P scaffolding, but
I think it works well now.

+                              OCSPVerifyResult::ResponseStatus* status) {
+  if (in == "MISSING") {
+    *status = OCSPVerifyResult::MISSING;
+  } else if (in == "PROVIDED") {
+    *status = OCSPVerifyResult::PROVIDED;
+  } else if (in == "ERROR_RESPONSE") {
+    *status = OCSPVerifyResult::ERROR_RESPONSE;
+  } else if (in == "BAD_PRODUCED_AT") {
+    *status = OCSPVerifyResult::BAD_PRODUCED_AT;
+  } else if (in == "NO_MATCHING_RESPONSE") {
+    *status = OCSPVerifyResult::NO_MATCHING_RESPONSE;
+  } else if (in == "INVALID_DATE") {
+    *status = OCSPVerifyResult::INVALID_DATE;
+  } else if (in == "PARSE_RESPONSE_ERROR") {
+    *status = OCSPVerifyResult::PARSE_RESPONSE_ERROR;
+  } else if (in == "PARSE_RESPONSE_DATA_ERROR") {
+    *status = OCSPVerifyResult::PARSE_RESPONSE_DATA_ERROR;
+  } else {
+    return false;
+  }
+  return true;
+}
+
+bool RevocationStatusFromString(std::string in, OCSPRevocationStatus* status) {

[Ryan Sleevi, 2016/07/19 00:02:49]

Same remarks

[dadrian, 2016/07/19 18:48:39]

Done.

+  if (in == "GOOD") {
+    *status = OCSPRevocationStatus::GOOD;
+  } else if (in == "REVOKED") {
+    *status = OCSPRevocationStatus::REVOKED;
+  } else if (in == "UNKNOWN") {
+    *status = OCSPRevocationStatus::UNKNOWN;
+  } else {
+    return false;
+  }
+  return true;
+}
+
+void CheckExpectStapleReport(const std::string& report,
+                             const HostPortPair& host_port_pair,
+                             const SSLInfo& ssl_info,
+                             const std::string& ocsp_response) {
+  std::unique_ptr<base::Value> value(base::JSONReader::Read(report));
+  ASSERT_TRUE(value);
+  ASSERT_TRUE(value->IsType(base::Value::TYPE_DICTIONARY));
+
+  base::DictionaryValue* report_dict;
+  ASSERT_TRUE(value->GetAsDictionary(&report_dict));
+
+  std::string report_hostname;
+  EXPECT_TRUE(report_dict->GetString("hostname", &report_hostname));
+  EXPECT_EQ(host_port_pair.host(), report_hostname);
+
+  int report_port;
+  EXPECT_TRUE(report_dict->GetInteger("port", &report_port));
+  EXPECT_EQ(host_port_pair.port(), report_port);
+
+  std::string report_response_status;
+  EXPECT_TRUE(
+      report_dict->GetString("response-status", &report_response_status));
+  OCSPVerifyResult::ResponseStatus response_status;
+  EXPECT_TRUE(
+      ResponseStatusFromString(report_response_status, &response_status));
+  EXPECT_EQ(ssl_info.ocsp_result.response_status, response_status);
+
+  std::string report_ocsp_response;
+  bool has_ocsp_response =
+      report_dict->GetString("ocsp-response", &report_ocsp_response);
+
+  if (!ocsp_response.empty()) {
+    EXPECT_TRUE(has_ocsp_response);
+    std::string decoded_ocsp_response;
+    EXPECT_TRUE(
+        base::Base64Decode(report_ocsp_response, &decoded_ocsp_response));
+    EXPECT_EQ(ocsp_response, decoded_ocsp_response);
+  } else {
+    EXPECT_FALSE(has_ocsp_response);
+  }
+
+  std::string report_cert_status;
+  bool has_cert_status =
+      report_dict->GetString("cert-status", &report_cert_status);
+  if (ssl_info.ocsp_result.response_status == OCSPVerifyResult::PROVIDED) {
+    EXPECT_TRUE(has_cert_status);
+    OCSPRevocationStatus revocation_status;
+    EXPECT_TRUE(
+        RevocationStatusFromString(report_cert_status, &revocation_status));
+    EXPECT_EQ(ssl_info.ocsp_result.revocation_status, revocation_status);
+  } else {
+    EXPECT_FALSE(has_cert_status);
+  }
+
+  base::ListValue* report_served_certificate_chain;
+  bool has_served_chain = report_dict->GetList(
+      "served-certificate-chain", &report_served_certificate_chain);
+
+  base::ListValue* report_validated_certificate_chain;
+  bool has_validated_chain = report_dict->GetList(
+      "validated-certificate-chain", &report_validated_certificate_chain);
+
+  if (ssl_info.is_issued_by_known_root) {
+    EXPECT_TRUE(has_served_chain);
+    ASSERT_NO_FATAL_FAILURE(CompareCertificateChainWithList(
+        ssl_info.unverified_cert, report_served_certificate_chain));
+
+    EXPECT_TRUE(has_validated_chain);
+    ASSERT_NO_FATAL_FAILURE(CompareCertificateChainWithList(
+        ssl_info.cert, report_validated_certificate_chain));
+  } else {
+    EXPECT_FALSE(has_served_chain);
+    EXPECT_FALSE(has_validated_chain);
+  }
+}
+
+void CheckExpectStaple(TransportSecurityState* state,
+                       MockCertificateReportSender* reporter,
+                       const SSLInfo& ssl_info,
+                       const std::string ocsp_response) {
+  HostPortPair host_port(kExpectStapleStaticHostname, 443);
+  state->SetReportSender(reporter);
+  state->ProcessExpectStaple(host_port, ssl_info, ocsp_response);
+  EXPECT_EQ(GURL(kExpectStapleStaticReportURI), reporter->latest_report_uri());
+  std::string serialized_report = reporter->latest_report();
+  EXPECT_NO_FATAL_FAILURE(CheckExpectStapleReport(serialized_report, host_port,
+                                                  ssl_info, ocsp_response));
+}
+
 }  // namespace
 
 class TransportSecurityStateTest : public testing::Test {
@@ -1902,6 +2028,122 @@ TEST_F(TransportSecurityStateTest, ExpectCTReporter) {
   EXPECT_EQ(GURL(kExpectCTStaticReportURI), reporter.report_uri());
 }
 
+TEST_F(TransportSecurityStateTest, ExpectStapleReporter) {
+  TransportSecurityState state;
+  TransportSecurityStateTest::EnableStaticExpectStaple(&state);
+  MockCertificateReportSender reporter;
+
+  std::string ocsp_response;
+
+  // Two dummy certs to use as the server-sent and validated chains. The
+  // contents don't matter.
+  scoped_refptr<X509Certificate> cert1 =
+      ImportCertFromFile(GetTestCertsDirectory(), "test_mail_google_com.pem");
+  scoped_refptr<X509Certificate> cert2 =
+      ImportCertFromFile(GetTestCertsDirectory(), "expired_cert.pem");
+
+  SSLInfo ssl_info;
+  ssl_info.is_issued_by_known_root = true;
+  ssl_info.cert = cert1;
+  ssl_info.unverified_cert = cert2;
+  ssl_info.ocsp_result.response_status = OCSPVerifyResult::MISSING;
+
+  ASSERT_NO_FATAL_FAILURE(
+      CheckExpectStaple(&state, &reporter, ssl_info, ocsp_response));
+
+  // Certs should not be included for non-public roots.
+  ssl_info.is_issued_by_known_root = false;
+  ASSERT_NO_FATAL_FAILURE(
+      CheckExpectStaple(&state, &reporter, ssl_info, ocsp_response));
+
+  // Check response statuses that correspond with a stapled response, but not
+  // revocation status.
+  ocsp_response = "dummy response";
+  ssl_info.is_issued_by_known_root = true;
+  std::vector<OCSPVerifyResult::ResponseStatus> response_statuses{
+      OCSPVerifyResult::ERROR_RESPONSE,
+      OCSPVerifyResult::BAD_PRODUCED_AT,
+      OCSPVerifyResult::NO_MATCHING_RESPONSE,
+      OCSPVerifyResult::INVALID_DATE,
+      OCSPVerifyResult::PARSE_RESPONSE_ERROR,
+      OCSPVerifyResult::PARSE_RESPONSE_DATA_ERROR,
+  };
+  for (const auto& response_status : response_statuses) {
+    ssl_info.ocsp_result.response_status = response_status;
+    ASSERT_NO_FATAL_FAILURE(
+        CheckExpectStaple(&state, &reporter, ssl_info, ocsp_response));
+  }
+
+  // Check each revocation status that should trigger a report.
+  ssl_info.ocsp_result.response_status = OCSPVerifyResult::PROVIDED;
+  std::vector<OCSPRevocationStatus> cert_statuses{
+      OCSPRevocationStatus::REVOKED, OCSPRevocationStatus::UNKNOWN,
+  };
+  for (const auto& cert_status : cert_statuses) {
+    ssl_info.ocsp_result.revocation_status = cert_status;
+    ASSERT_NO_FATAL_FAILURE(
+        CheckExpectStaple(&state, &reporter, ssl_info, ocsp_response));
+  }
+}
+
+TEST_F(TransportSecurityStateTest, ExpectStapleDoesNotReportValidStaple) {
+  TransportSecurityState state;
+  TransportSecurityStateTest::EnableStaticExpectStaple(&state);
+  MockCertificateReportSender reporter;
+  state.SetReportSender(&reporter);
+
+  HostPortPair host_port(kExpectStapleStaticHostname, 443);
+
+  // Two dummy certs to use as the server-sent and validated chains. The
+  // contents don't matter.
+  scoped_refptr<X509Certificate> cert1 =
+      ImportCertFromFile(GetTestCertsDirectory(), "test_mail_google_com.pem");
+  scoped_refptr<X509Certificate> cert2 =
+      ImportCertFromFile(GetTestCertsDirectory(), "expired_cert.pem");
+
+  SSLInfo ssl_info;
+  ssl_info.is_issued_by_known_root = true;
+  ssl_info.cert = cert1;
+  ssl_info.unverified_cert = cert2;
+  ssl_info.ocsp_result.response_status = OCSPVerifyResult::PROVIDED;
+  ssl_info.ocsp_result.revocation_status = OCSPRevocationStatus::GOOD;
+
+  std::string ocsp_response = "dummy response";
+
+  state.ProcessExpectStaple(host_port, ssl_info, ocsp_response);
+  EXPECT_EQ(GURL(), reporter.latest_report_uri());
+  EXPECT_TRUE(reporter.latest_report().empty());
+}
+
+TEST_F(TransportSecurityStateTest, ExpectStapleRequiresPreload) {
+  TransportSecurityState state;
+  TransportSecurityStateTest::EnableStaticExpectStaple(&state);
+  MockCertificateReportSender reporter;
+  state.SetReportSender(&reporter);
+
+  HostPortPair host_port("not-preloaded.badssl.com", 443);
+
+  // Two dummy certs to use as the server-sent and validated chains. The
+  // contents don't matter.
+  scoped_refptr<X509Certificate> cert1 =
+      ImportCertFromFile(GetTestCertsDirectory(), "test_mail_google_com.pem");
+  scoped_refptr<X509Certificate> cert2 =
+      ImportCertFromFile(GetTestCertsDirectory(), "expired_cert.pem");
+
+  SSLInfo ssl_info;
+  ssl_info.is_issued_by_known_root = true;
+  ssl_info.cert = cert1;
+  ssl_info.unverified_cert = cert2;
+  ssl_info.ocsp_result.response_status = OCSPVerifyResult::MISSING;
+
+  // Empty response
+  std::string ocsp_response;
+
+  state.ProcessExpectStaple(host_port, ssl_info, ocsp_response);
+  EXPECT_EQ(GURL(), reporter.latest_report_uri());
+  EXPECT_TRUE(reporter.latest_report().empty());
+}
+
 // Tests that TransportSecurityState always consults the RequireCTDelegate,
 // if supplied.
 TEST_F(TransportSecurityStateTest, RequireCTConsultsDelegate) {