diff options
15 files changed, 284 insertions, 339 deletions
diff --git a/chrome/browser/net/certificate_error_reporter.cc b/chrome/browser/net/certificate_error_reporter.cc index 077b0ab..cd0e861 100644 --- a/chrome/browser/net/certificate_error_reporter.cc +++ b/chrome/browser/net/certificate_error_reporter.cc @@ -20,7 +20,8 @@ namespace { -// Constants used for crypto +// Constants used for crypto. The corresponding private key is used by +// the SafeBrowsing client-side detection server to decrypt reports. static const uint8 kServerPublicKey[] = { 0x51, 0xcc, 0x52, 0x67, 0x42, 0x47, 0x3b, 0x10, 0xe8, 0x63, 0x18, 0x3c, 0x61, 0xa7, 0x96, 0x76, 0x86, 0x91, 0x40, 0x71, 0x39, 0x5f, @@ -31,6 +32,30 @@ static const uint32 kServerPublicKeyVersion = 1; static const char kHkdfLabel[] = "certificate report"; +std::string GetHkdfSubkeySecret(size_t subkey_length, + const uint8* private_key, + const uint8* public_key) { + uint8 shared_secret[crypto::curve25519::kBytes]; + crypto::curve25519::ScalarMult(private_key, public_key, shared_secret); + + // By mistake, the HKDF label here ends up with an extra null byte on + // the end, due to using sizeof(kHkdfLabel) in the StringPiece + // constructor instead of strlen(kHkdfLabel). Ideally this code should + // be just passing kHkdfLabel directly into the HKDF constructor. + // + // TODO(estark): fix this in coordination with the server-side code -- + // perhaps by rolling the public key version forward and using the + // version to decide whether to use the extra-null-byte version of the + // label. https://crbug.com/517746 + crypto::HKDF hkdf(base::StringPiece(reinterpret_cast<char*>(shared_secret), + sizeof(shared_secret)), + "" /* salt */, + base::StringPiece(kHkdfLabel, sizeof(kHkdfLabel)), + 0 /* key bytes */, 0 /* iv bytes */, subkey_length); + + return hkdf.subkey_secret().as_string(); +} + bool EncryptSerializedReport( const uint8* server_public_key, uint32 server_public_key_version, @@ -39,25 +64,18 @@ bool EncryptSerializedReport( // Generate an ephemeral key pair to generate a shared secret. uint8 public_key[crypto::curve25519::kBytes]; uint8 private_key[crypto::curve25519::kScalarBytes]; - uint8 shared_secret[crypto::curve25519::kBytes]; crypto::RandBytes(private_key, sizeof(private_key)); crypto::curve25519::ScalarBaseMult(private_key, public_key); - crypto::curve25519::ScalarMult(private_key, server_public_key, shared_secret); crypto::Aead aead(crypto::Aead::AES_128_CTR_HMAC_SHA256); - crypto::HKDF hkdf(std::string(reinterpret_cast<char*>(shared_secret), - sizeof(shared_secret)), - std::string(), - base::StringPiece(kHkdfLabel, sizeof(kHkdfLabel)), 0, 0, - aead.KeyLength()); - - const std::string key(hkdf.subkey_secret().data(), - hkdf.subkey_secret().size()); + const std::string key = + GetHkdfSubkeySecret(aead.KeyLength(), private_key, + reinterpret_cast<const uint8*>(server_public_key)); aead.Init(&key); // Use an all-zero nonce because the key is random per-message. - std::string nonce(aead.NonceLength(), 0); + std::string nonce(aead.NonceLength(), '\0'); std::string ciphertext; if (!aead.Seal(report, nonce, std::string(), &ciphertext)) { @@ -67,8 +85,8 @@ bool EncryptSerializedReport( encrypted_report->set_encrypted_report(ciphertext); encrypted_report->set_server_public_key_version(server_public_key_version); - encrypted_report->set_client_public_key( - std::string(reinterpret_cast<char*>(public_key), sizeof(public_key))); + encrypted_report->set_client_public_key(reinterpret_cast<char*>(public_key), + sizeof(public_key)); encrypted_report->set_algorithm( chrome_browser_net::EncryptedCertLoggerRequest:: AEAD_ECDH_AES_128_CTR_HMAC_SHA256); @@ -93,7 +111,7 @@ CertificateErrorReporter::CertificateErrorReporter( CertificateErrorReporter::CertificateErrorReporter( const GURL& upload_url, - const uint8 server_public_key[32], + const uint8 server_public_key[/* 32 */], const uint32 server_public_key_version, scoped_ptr<net::CertificateReportSender> certificate_report_sender) : certificate_report_sender_(certificate_report_sender.Pass()), @@ -107,38 +125,31 @@ CertificateErrorReporter::CertificateErrorReporter( CertificateErrorReporter::~CertificateErrorReporter() { } -void CertificateErrorReporter::SendReport( - ReportType type, +void CertificateErrorReporter::SendExtendedReportingReport( const std::string& serialized_report) { - switch (type) { - case REPORT_TYPE_PINNING_VIOLATION: - certificate_report_sender_->Send(upload_url_, serialized_report); - break; - case REPORT_TYPE_EXTENDED_REPORTING: - if (upload_url_.SchemeIsCryptographic()) { - certificate_report_sender_->Send(upload_url_, serialized_report); - } else { - DCHECK(IsHttpUploadUrlSupported()); + if (upload_url_.SchemeIsCryptographic()) { + certificate_report_sender_->Send(upload_url_, serialized_report); + } else { + DCHECK(IsHttpUploadUrlSupported()); #if defined(USE_OPENSSL) - EncryptedCertLoggerRequest encrypted_report; - if (!EncryptSerializedReport(server_public_key_, - server_public_key_version_, - serialized_report, &encrypted_report)) { - LOG(ERROR) << "Failed to encrypt serialized report."; - return; - } - std::string serialized_encrypted_report; - encrypted_report.SerializeToString(&serialized_encrypted_report); - certificate_report_sender_->Send(upload_url_, - serialized_encrypted_report); + EncryptedCertLoggerRequest encrypted_report; + if (!EncryptSerializedReport(server_public_key_, server_public_key_version_, + serialized_report, &encrypted_report)) { + LOG(ERROR) << "Failed to encrypt serialized report."; + return; + } + std::string serialized_encrypted_report; + encrypted_report.SerializeToString(&serialized_encrypted_report); + certificate_report_sender_->Send(upload_url_, serialized_encrypted_report); #endif - } - break; - default: - NOTREACHED(); } } +void CertificateErrorReporter::SendPinningViolationReport( + const std::string& serialized_report) { + certificate_report_sender_->Send(upload_url_, serialized_report); +} + bool CertificateErrorReporter::IsHttpUploadUrlSupported() { #if defined(USE_OPENSSL) return true; @@ -153,21 +164,11 @@ bool CertificateErrorReporter::DecryptCertificateErrorReport( const uint8 server_private_key[32], const EncryptedCertLoggerRequest& encrypted_report, std::string* decrypted_serialized_report) { - uint8 shared_secret[crypto::curve25519::kBytes]; - crypto::curve25519::ScalarMult( - server_private_key, reinterpret_cast<const uint8*>( - encrypted_report.client_public_key().data()), - shared_secret); - crypto::Aead aead(crypto::Aead::AES_128_CTR_HMAC_SHA256); - crypto::HKDF hkdf(std::string(reinterpret_cast<char*>(shared_secret), - sizeof(shared_secret)), - std::string(), - base::StringPiece(kHkdfLabel, sizeof(kHkdfLabel)), 0, 0, - aead.KeyLength()); - - const std::string key(hkdf.subkey_secret().data(), - hkdf.subkey_secret().size()); + const std::string key = + GetHkdfSubkeySecret(aead.KeyLength(), server_private_key, + reinterpret_cast<const uint8*>( + encrypted_report.client_public_key().data())); aead.Init(&key); // Use an all-zero nonce because the key is random per-message. diff --git a/chrome/browser/net/certificate_error_reporter.h b/chrome/browser/net/certificate_error_reporter.h index cebb780..f0d020f 100644 --- a/chrome/browser/net/certificate_error_reporter.h +++ b/chrome/browser/net/certificate_error_reporter.h @@ -26,16 +26,6 @@ class EncryptedCertLoggerRequest; // certificate chains to a report collection server. class CertificateErrorReporter { public: - // These represent the types of reports that can be sent. - enum ReportType { - // A report of a certificate chain that failed a certificate pinning - // check. - REPORT_TYPE_PINNING_VIOLATION, - // A report for an invalid certificate chain that is being sent for - // a user who has opted-in to the extended reporting program. - REPORT_TYPE_EXTENDED_REPORTING - }; - // Creates a certificate error reporter that will send certificate // error reports to |upload_url|, using |request_context| as the // context for the reports. |cookies_preference| controls whether @@ -46,10 +36,11 @@ class CertificateErrorReporter { net::CertificateReportSender::CookiesPreference cookies_preference); // Allows tests to use a server public key with known private key and - // a mock CertificateReportSender. + // a mock CertificateReportSender. |server_public_key| must outlive + // the CertificateErrorReporter. CertificateErrorReporter( const GURL& upload_url, - const uint8 server_public_key[32], + const uint8 server_public_key[/* 32 */], const uint32 server_public_key_version, scoped_ptr<net::CertificateReportSender> certificate_report_sender); @@ -69,11 +60,15 @@ class CertificateErrorReporter { // an HTTP endpoint to send encrypted extended reporting reports. On // unsupported platforms, callers must send extended reporting reports // over SSL. - virtual void SendReport(ReportType type, - const std::string& serialized_report); + virtual void SendExtendedReportingReport( + const std::string& serialized_report); + + // As |SendExtendedReportingReport|, but does not encrypt reports, + // since the pinning violation server is not capable of handling + // encrypted reports. + virtual void SendPinningViolationReport(const std::string& serialized_report); - // Callers can use this method to determine if sending reports over - // HTTP is supported. + // Whether sending reports over HTTP is supported. static bool IsHttpUploadUrlSupported(); #if defined(USE_OPENSSL) diff --git a/chrome/browser/net/certificate_error_reporter_unittest.cc b/chrome/browser/net/certificate_error_reporter_unittest.cc index 5a7f94f..f154daf 100644 --- a/chrome/browser/net/certificate_error_reporter_unittest.cc +++ b/chrome/browser/net/certificate_error_reporter_unittest.cc @@ -23,7 +23,7 @@ namespace { const char kDummyHttpReportUri[] = "http://example.test"; const char kDummyHttpsReportUri[] = "https://example.test"; const char kDummyReport[] = "a dummy report"; -const uint32 kServerPublicKeyVersion = 1; +const uint32 kServerPublicKeyTestVersion = 16; // A mock CertificateReportSender that keeps track of the last report // sent. @@ -58,41 +58,36 @@ class CertificateErrorReporterTest : public ::testing::Test { ~CertificateErrorReporterTest() override {} - const uint8_t* server_public_key() { return server_public_key_; } - const uint8_t* server_private_key() { return server_private_key_; } - - private: + protected: uint8_t server_public_key_[32]; uint8_t server_private_key_[32]; }; -// Test that CertificateErrorReporter::SendReport sends a plaintext -// report for pinning violation reports. +// Test that CertificateErrorReporter::SendPinningViolationReport sends +// a plaintext report for pinning violation reports. TEST_F(CertificateErrorReporterTest, PinningViolationSendReport) { GURL url(kDummyHttpReportUri); MockCertificateReportSender* mock_report_sender = new MockCertificateReportSender(); - CertificateErrorReporter reporter(url, server_public_key(), - kServerPublicKeyVersion, + CertificateErrorReporter reporter(url, server_public_key_, + kServerPublicKeyTestVersion, make_scoped_ptr(mock_report_sender)); - reporter.SendReport(CertificateErrorReporter::REPORT_TYPE_PINNING_VIOLATION, - kDummyReport); + reporter.SendPinningViolationReport(kDummyReport); EXPECT_EQ(mock_report_sender->latest_report_uri(), url); EXPECT_EQ(mock_report_sender->latest_report(), kDummyReport); } -// Test that CertificateErrorReporter::SendReport sends an encrypted or -// plaintext extended reporting report as appropriate. +// Test that CertificateErrorReporter::SendExtendedReportingReport sends +// an encrypted or plaintext extended reporting report as appropriate. TEST_F(CertificateErrorReporterTest, ExtendedReportingSendReport) { // Data should not be encrypted when sent to an HTTPS URL. MockCertificateReportSender* mock_report_sender = new MockCertificateReportSender(); GURL https_url(kDummyHttpsReportUri); - CertificateErrorReporter https_reporter(https_url, server_public_key(), - kServerPublicKeyVersion, + CertificateErrorReporter https_reporter(https_url, server_public_key_, + kServerPublicKeyTestVersion, make_scoped_ptr(mock_report_sender)); - https_reporter.SendReport( - CertificateErrorReporter::REPORT_TYPE_EXTENDED_REPORTING, kDummyReport); + https_reporter.SendExtendedReportingReport(kDummyReport); EXPECT_EQ(mock_report_sender->latest_report_uri(), https_url); EXPECT_EQ(mock_report_sender->latest_report(), kDummyReport); @@ -102,10 +97,9 @@ TEST_F(CertificateErrorReporterTest, ExtendedReportingSendReport) { new MockCertificateReportSender(); GURL http_url(kDummyHttpReportUri); CertificateErrorReporter http_reporter( - http_url, server_public_key(), kServerPublicKeyVersion, + http_url, server_public_key_, kServerPublicKeyTestVersion, make_scoped_ptr(http_mock_report_sender)); - http_reporter.SendReport( - CertificateErrorReporter::REPORT_TYPE_EXTENDED_REPORTING, kDummyReport); + http_reporter.SendExtendedReportingReport(kDummyReport); EXPECT_EQ(http_mock_report_sender->latest_report_uri(), http_url); @@ -114,13 +108,13 @@ TEST_F(CertificateErrorReporterTest, ExtendedReportingSendReport) { chrome_browser_net::EncryptedCertLoggerRequest encrypted_request; ASSERT_TRUE(encrypted_request.ParseFromString( http_mock_report_sender->latest_report())); - EXPECT_EQ(kServerPublicKeyVersion, + EXPECT_EQ(kServerPublicKeyTestVersion, encrypted_request.server_public_key_version()); EXPECT_EQ(chrome_browser_net::EncryptedCertLoggerRequest:: AEAD_ECDH_AES_128_CTR_HMAC_SHA256, encrypted_request.algorithm()); ASSERT_TRUE(CertificateErrorReporter::DecryptCertificateErrorReport( - server_private_key(), encrypted_request, &uploaded_report)); + server_private_key_, encrypted_request, &uploaded_report)); #else ADD_FAILURE() << "Only supported in OpenSSL ports"; #endif @@ -288,7 +282,7 @@ TEST_F(CertificateErrorReporterTest, DecryptExampleReport) { sizeof(kSerializedEncryptedReport)))); ASSERT_TRUE( chrome_browser_net::CertificateErrorReporter:: - DecryptCertificateErrorReport(server_private_key(), encrypted_request, + DecryptCertificateErrorReport(server_private_key_, encrypted_request, &decrypted_serialized_report)); } #endif diff --git a/chrome/browser/safe_browsing/ping_manager.cc b/chrome/browser/safe_browsing/ping_manager.cc index 7256f5d..f4b2dfe 100644 --- a/chrome/browser/safe_browsing/ping_manager.cc +++ b/chrome/browser/safe_browsing/ping_manager.cc @@ -138,9 +138,7 @@ void SafeBrowsingPingManager::ReportMalwareDetails( void SafeBrowsingPingManager::ReportInvalidCertificateChain( const std::string& serialized_report) { DCHECK(certificate_error_reporter_); - certificate_error_reporter_->SendReport( - CertificateErrorReporter::REPORT_TYPE_EXTENDED_REPORTING, - serialized_report); + certificate_error_reporter_->SendExtendedReportingReport(serialized_report); } void SafeBrowsingPingManager::SetCertificateErrorReporterForTesting( diff --git a/chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc b/chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc index 6330b0e..3f1a4dc 100644 --- a/chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc +++ b/chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc @@ -73,7 +73,7 @@ class FakeConnectionInfoDelegate : public CaptivePortalBlockingPage::Delegate { }; class CaptivePortalBlockingPageTest - : public CertificateReportingTestUtils::CertificateReportingTest { + : public certificate_reporting_test_utils::CertificateReportingTest { public: CaptivePortalBlockingPageTest() {} @@ -101,7 +101,7 @@ class CaptivePortalBlockingPageTest ExpectLoginURL expect_login_url, scoped_ptr<SSLCertReporter> ssl_cert_reporter); - void TestCertReporting(CertificateReportingTestUtils::OptIn opt_in); + void TestCertReporting(certificate_reporting_test_utils::OptIn opt_in); private: DISALLOW_COPY_AND_ASSIGN(CaptivePortalBlockingPageTest); @@ -178,17 +178,17 @@ void CaptivePortalBlockingPageTest::TestInterstitial( } void CaptivePortalBlockingPageTest::TestCertReporting( - CertificateReportingTestUtils::OptIn opt_in) { + certificate_reporting_test_utils::OptIn opt_in) { ASSERT_NO_FATAL_FAILURE(SetUpMockReporter()); - CertificateReportingTestUtils::SetCertReportingOptIn(browser(), opt_in); + certificate_reporting_test_utils::SetCertReportingOptIn(browser(), opt_in); base::RunLoop run_loop; scoped_ptr<SSLCertReporter> ssl_cert_reporter = - CertificateReportingTestUtils::SetUpMockSSLCertReporter( + certificate_reporting_test_utils::SetUpMockSSLCertReporter( &run_loop, - opt_in == CertificateReportingTestUtils::EXTENDED_REPORTING_OPT_IN - ? CertificateReportingTestUtils::CERT_REPORT_EXPECTED - : CertificateReportingTestUtils::CERT_REPORT_NOT_EXPECTED); + opt_in == certificate_reporting_test_utils::EXTENDED_REPORTING_OPT_IN + ? certificate_reporting_test_utils::CERT_REPORT_EXPECTED + : certificate_reporting_test_utils::CERT_REPORT_NOT_EXPECTED); const GURL kLandingUrl(captive_portal::CaptivePortalDetector::kDefaultURL); TestInterstitial(true, std::string(), kLandingUrl, EXPECT_WIFI_YES, @@ -201,7 +201,7 @@ void CaptivePortalBlockingPageTest::TestCertReporting( browser()->tab_strip_model()->GetActiveWebContents(); tab->GetInterstitialPage()->DontProceed(); - if (opt_in == CertificateReportingTestUtils::EXTENDED_REPORTING_OPT_IN) { + if (opt_in == certificate_reporting_test_utils::EXTENDED_REPORTING_OPT_IN) { // Check that the mock reporter received a request to send a report. run_loop.Run(); EXPECT_EQ(GURL(kBrokenSSL).host(), GetLatestHostnameReported()); @@ -284,15 +284,16 @@ IN_PROC_BROWSER_TEST_F(CaptivePortalBlockingPageTest, CertReportingOptIn) { // the user opts in under such a Finch config, and the below test // tests that a report *is not* sent when the user doesn't opt in // (under any Finch config). - if (CertificateReportingTestUtils::GetReportExpectedFromFinch() == - CertificateReportingTestUtils::CERT_REPORT_EXPECTED) { - TestCertReporting(CertificateReportingTestUtils::EXTENDED_REPORTING_OPT_IN); + if (certificate_reporting_test_utils::GetReportExpectedFromFinch() == + certificate_reporting_test_utils::CERT_REPORT_EXPECTED) { + TestCertReporting( + certificate_reporting_test_utils::EXTENDED_REPORTING_OPT_IN); } } IN_PROC_BROWSER_TEST_F(CaptivePortalBlockingPageTest, CertReportingOptOut) { TestCertReporting( - CertificateReportingTestUtils::EXTENDED_REPORTING_DO_NOT_OPT_IN); + certificate_reporting_test_utils::EXTENDED_REPORTING_DO_NOT_OPT_IN); } class CaptivePortalBlockingPageIDNTest : public SecurityInterstitialIDNTest { diff --git a/chrome/browser/ssl/certificate_error_report.cc b/chrome/browser/ssl/certificate_error_report.cc index c63e897..49ca918 100644 --- a/chrome/browser/ssl/certificate_error_report.cc +++ b/chrome/browser/ssl/certificate_error_report.cc @@ -18,36 +18,27 @@ namespace { void AddCertStatusToReportErrors(net::CertStatus cert_status, CertLoggerRequest* report) { - if (cert_status & net::CERT_STATUS_REVOKED) - report->add_cert_error(CertLoggerRequest::ERR_CERT_REVOKED); - if (cert_status & net::CERT_STATUS_INVALID) - report->add_cert_error(CertLoggerRequest::ERR_CERT_INVALID); - if (cert_status & net::CERT_STATUS_PINNED_KEY_MISSING) - report->add_cert_error( - CertLoggerRequest::ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN); - if (cert_status & net::CERT_STATUS_AUTHORITY_INVALID) - report->add_cert_error(CertLoggerRequest::ERR_CERT_AUTHORITY_INVALID); - if (cert_status & net::CERT_STATUS_COMMON_NAME_INVALID) - report->add_cert_error(CertLoggerRequest::ERR_CERT_COMMON_NAME_INVALID); - if (cert_status & net::CERT_STATUS_NON_UNIQUE_NAME) - report->add_cert_error(CertLoggerRequest::ERR_CERT_NON_UNIQUE_NAME); - if (cert_status & net::CERT_STATUS_NAME_CONSTRAINT_VIOLATION) - report->add_cert_error( - CertLoggerRequest::ERR_CERT_NAME_CONSTRAINT_VIOLATION); - if (cert_status & net::CERT_STATUS_WEAK_SIGNATURE_ALGORITHM) - report->add_cert_error( - CertLoggerRequest::ERR_CERT_WEAK_SIGNATURE_ALGORITHM); - if (cert_status & net::CERT_STATUS_WEAK_KEY) - report->add_cert_error(CertLoggerRequest::ERR_CERT_WEAK_KEY); - if (cert_status & net::CERT_STATUS_DATE_INVALID) - report->add_cert_error(CertLoggerRequest::ERR_CERT_DATE_INVALID); - if (cert_status & net::CERT_STATUS_VALIDITY_TOO_LONG) - report->add_cert_error(CertLoggerRequest::ERR_CERT_VALIDITY_TOO_LONG); - if (cert_status & net::CERT_STATUS_UNABLE_TO_CHECK_REVOCATION) - report->add_cert_error( - CertLoggerRequest::ERR_CERT_UNABLE_TO_CHECK_REVOCATION); - if (cert_status & net::CERT_STATUS_NO_REVOCATION_MECHANISM) - report->add_cert_error(CertLoggerRequest::ERR_CERT_NO_REVOCATION_MECHANISM); +#define COPY_CERT_STATUS(error) RENAME_CERT_STATUS(error, CERT_##error) +#define RENAME_CERT_STATUS(status_error, logger_error) \ + if (cert_status & net::CERT_STATUS_##status_error) \ + report->add_cert_error(CertLoggerRequest::ERR_##logger_error); + + COPY_CERT_STATUS(REVOKED) + COPY_CERT_STATUS(INVALID) + RENAME_CERT_STATUS(PINNED_KEY_MISSING, SSL_PINNED_KEY_NOT_IN_CERT_CHAIN) + COPY_CERT_STATUS(AUTHORITY_INVALID) + COPY_CERT_STATUS(COMMON_NAME_INVALID) + COPY_CERT_STATUS(NON_UNIQUE_NAME) + COPY_CERT_STATUS(NAME_CONSTRAINT_VIOLATION) + COPY_CERT_STATUS(WEAK_SIGNATURE_ALGORITHM) + COPY_CERT_STATUS(WEAK_KEY) + COPY_CERT_STATUS(DATE_INVALID) + COPY_CERT_STATUS(VALIDITY_TOO_LONG) + COPY_CERT_STATUS(UNABLE_TO_CHECK_REVOCATION) + COPY_CERT_STATUS(NO_REVOCATION_MECHANISM) + +#undef RENAME_CERT_STATUS +#undef COPY_CERT_STATUS } bool CertificateChainToString(scoped_refptr<net::X509Certificate> cert, @@ -56,7 +47,7 @@ bool CertificateChainToString(scoped_refptr<net::X509Certificate> cert, if (!cert->GetPEMEncodedChain(&pem_encoded_chain)) return false; - *result = base::JoinString(pem_encoded_chain, base::StringPiece()); + *result = base::JoinString(pem_encoded_chain, ""); return true; } diff --git a/chrome/browser/ssl/certificate_error_report_unittest.cc b/chrome/browser/ssl/certificate_error_report_unittest.cc index 72d379c..5c712ee 100644 --- a/chrome/browser/ssl/certificate_error_report_unittest.cc +++ b/chrome/browser/ssl/certificate_error_report_unittest.cc @@ -16,9 +16,11 @@ #include "net/cert/cert_status_flags.h" #include "net/ssl/ssl_info.h" #include "net/test/cert_test_util.h" +#include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" using net::SSLInfo; +using testing::UnorderedElementsAre; namespace { @@ -28,7 +30,6 @@ const char kTestCertFilename[] = "test_mail_google_com.pem"; const net::CertStatus kCertStatus = net::CERT_STATUS_COMMON_NAME_INVALID | net::CERT_STATUS_REVOKED; -const size_t kNumCertErrors = 2; const CertLoggerRequest::CertError kFirstReportedCertError = CertLoggerRequest::ERR_CERT_COMMON_NAME_INVALID; @@ -69,11 +70,10 @@ std::string GetPEMEncodedChain() { // a CertLoggerRequest protobuf (which is the format that the receiving // server expects it in) with the right data in it. TEST(CertificateErrorReportTest, SerializedReportAsProtobuf) { - SSLInfo ssl_info = GetTestSSLInfo(INCLUDE_UNVERIFIED_CERT_CHAIN); - std::string serialized_report; - CertificateErrorReport report(kDummyHostname, ssl_info); - report.Serialize(&serialized_report); + CertificateErrorReport report(kDummyHostname, + GetTestSSLInfo(INCLUDE_UNVERIFIED_CERT_CHAIN)); + ASSERT_TRUE(report.Serialize(&serialized_report)); CertLoggerRequest deserialized_report; ASSERT_TRUE(deserialized_report.ParseFromString(serialized_report)); @@ -83,36 +83,25 @@ TEST(CertificateErrorReportTest, SerializedReportAsProtobuf) { EXPECT_EQ(1, deserialized_report.pin().size()); EXPECT_EQ(kDummyFailureLog, deserialized_report.pin().Get(0)); - std::set<CertLoggerRequest::CertError> reported_errors; - reported_errors.insert(static_cast<CertLoggerRequest::CertError>( - deserialized_report.cert_error().Get(0))); - reported_errors.insert(static_cast<CertLoggerRequest::CertError>( - deserialized_report.cert_error().Get(1))); - EXPECT_EQ(kNumCertErrors, reported_errors.size()); - EXPECT_EQ(1u, reported_errors.count(kFirstReportedCertError)); - EXPECT_EQ(1u, reported_errors.count(kSecondReportedCertError)); + EXPECT_THAT( + deserialized_report.cert_error(), + UnorderedElementsAre(kFirstReportedCertError, kSecondReportedCertError)); } TEST(CertificateErrorReportTest, SerializedReportAsProtobufWithInterstitialInfo) { + std::string serialized_report; // Use EXCLUDE_UNVERIFIED_CERT_CHAIN here to exercise the code path // where SSLInfo does not contain the unverified cert chain. (The test // above exercises the path where it does.) - SSLInfo ssl_info = GetTestSSLInfo(EXCLUDE_UNVERIFIED_CERT_CHAIN); - - std::string serialized_report; - CertificateErrorReport report(kDummyHostname, ssl_info); - - const CertificateErrorReport::InterstitialReason interstitial_reason = - CertificateErrorReport::INTERSTITIAL_CLOCK; - const CertificateErrorReport::ProceedDecision proceeded = - CertificateErrorReport::USER_PROCEEDED; - const CertificateErrorReport::Overridable overridable = - CertificateErrorReport::INTERSTITIAL_OVERRIDABLE; + CertificateErrorReport report(kDummyHostname, + GetTestSSLInfo(EXCLUDE_UNVERIFIED_CERT_CHAIN)); - report.SetInterstitialInfo(interstitial_reason, proceeded, overridable); + report.SetInterstitialInfo(CertificateErrorReport::INTERSTITIAL_CLOCK, + CertificateErrorReport::USER_PROCEEDED, + CertificateErrorReport::INTERSTITIAL_OVERRIDABLE); - report.Serialize(&serialized_report); + ASSERT_TRUE(report.Serialize(&serialized_report)); CertLoggerRequest deserialized_report; ASSERT_TRUE(deserialized_report.ParseFromString(serialized_report)); @@ -127,24 +116,18 @@ TEST(CertificateErrorReportTest, EXPECT_EQ(true, deserialized_report.interstitial_info().user_proceeded()); EXPECT_EQ(true, deserialized_report.interstitial_info().overridable()); - std::set<CertLoggerRequest::CertError> reported_errors; - reported_errors.insert(static_cast<CertLoggerRequest::CertError>( - deserialized_report.cert_error().Get(0))); - reported_errors.insert(static_cast<CertLoggerRequest::CertError>( - deserialized_report.cert_error().Get(1))); - EXPECT_EQ(kNumCertErrors, reported_errors.size()); - EXPECT_EQ(1u, reported_errors.count(kFirstReportedCertError)); - EXPECT_EQ(1u, reported_errors.count(kSecondReportedCertError)); + EXPECT_THAT( + deserialized_report.cert_error(), + UnorderedElementsAre(kFirstReportedCertError, kSecondReportedCertError)); } // Test that a serialized report can be parsed. TEST(CertificateErrorReportTest, ParseSerializedReport) { - SSLInfo ssl_info = GetTestSSLInfo(EXCLUDE_UNVERIFIED_CERT_CHAIN); - std::string serialized_report; - CertificateErrorReport report(kDummyHostname, ssl_info); + CertificateErrorReport report(kDummyHostname, + GetTestSSLInfo(EXCLUDE_UNVERIFIED_CERT_CHAIN)); EXPECT_EQ(kDummyHostname, report.hostname()); - report.Serialize(&serialized_report); + ASSERT_TRUE(report.Serialize(&serialized_report)); CertificateErrorReport parsed; ASSERT_TRUE(parsed.InitializeFromString(serialized_report)); diff --git a/chrome/browser/ssl/certificate_reporting_test_utils.cc b/chrome/browser/ssl/certificate_reporting_test_utils.cc index 74e9fa8..b61340e 100644 --- a/chrome/browser/ssl/certificate_reporting_test_utils.cc +++ b/chrome/browser/ssl/certificate_reporting_test_utils.cc @@ -36,24 +36,60 @@ void SetMockReporter(SafeBrowsingService* safe_browsing_service, reporter.Pass()); } +// This is a test implementation of the interface that blocking pages +// use to send certificate reports. It checks that the blocking page +// calls or does not call the report method when a report should or +// should not be sent, respectively. +class MockSSLCertReporter : public SSLCertReporter { + public: + MockSSLCertReporter( + const scoped_refptr<SafeBrowsingUIManager>& safe_browsing_ui_manager, + const base::Closure& report_sent_callback) + : safe_browsing_ui_manager_(safe_browsing_ui_manager), + reported_(false), + expect_report_(false), + report_sent_callback_(report_sent_callback) {} + + ~MockSSLCertReporter() override { EXPECT_EQ(expect_report_, reported_); } + + // SSLCertReporter implementation. + void ReportInvalidCertificateChain( + const std::string& serialized_report) override { + reported_ = true; + if (expect_report_) { + safe_browsing_ui_manager_->ReportInvalidCertificateChain( + serialized_report, report_sent_callback_); + } + } + + void set_expect_report(bool expect_report) { expect_report_ = expect_report; } + + private: + const scoped_refptr<SafeBrowsingUIManager> safe_browsing_ui_manager_; + bool reported_; + bool expect_report_; + base::Closure report_sent_callback_; +}; + } // namespace -namespace CertificateReportingTestUtils { +namespace certificate_reporting_test_utils { // This class is used to test invalid certificate chain reporting when // the user opts in to do so on the interstitial. It keeps track of the -// most recent hostname for which a report would have been sent over the -// network. -class MockReporter : public chrome_browser_net::CertificateErrorReporter { +// most recent hostname for which an extended reporting report would +// have been sent over the network. +class CertificateReportingTest::MockReporter + : public chrome_browser_net::CertificateErrorReporter { public: MockReporter( net::URLRequestContext* request_context, const GURL& upload_url, net::CertificateReportSender::CookiesPreference cookies_preference); - // CertificateErrorReporter implementation - void SendReport(CertificateErrorReporter::ReportType type, - const std::string& serialized_report) override; + // CertificateErrorReporter implementation. + void SendExtendedReportingReport( + const std::string& serialized_report) override; // Returns the hostname in the report for the last call to // |SendReport|. @@ -67,7 +103,7 @@ class MockReporter : public chrome_browser_net::CertificateErrorReporter { DISALLOW_COPY_AND_ASSIGN(MockReporter); }; -MockReporter::MockReporter( +CertificateReportingTest::MockReporter::MockReporter( net::URLRequestContext* request_context, const GURL& upload_url, net::CertificateReportSender::CookiesPreference cookies_preference) @@ -75,11 +111,10 @@ MockReporter::MockReporter( upload_url, cookies_preference) {} -void MockReporter::SendReport(CertificateErrorReporter::ReportType type, - const std::string& serialized_report) { +void CertificateReportingTest::MockReporter::SendExtendedReportingReport( + const std::string& serialized_report) { CertificateErrorReport report; ASSERT_TRUE(report.InitializeFromString(serialized_report)); - EXPECT_EQ(CertificateErrorReporter::REPORT_TYPE_EXTENDED_REPORTING, type); latest_hostname_reported_ = report.hostname(); } @@ -89,9 +124,9 @@ void CertificateReportingTest::SetUpMockReporter() { // because the MockReporter doesn't actually use a // request_context. (In order to pass a real request_context, the // reporter would have to be constructed on the IO thread.) - reporter_ = - new MockReporter(nullptr, GURL("http://example.test"), - net::CertificateReportSender::DO_NOT_SEND_COOKIES); + reporter_ = new CertificateReportingTest::MockReporter( + nullptr, GURL("http://example.test"), + net::CertificateReportSender::DO_NOT_SEND_COOKIES); scoped_refptr<SafeBrowsingService> safe_browsing_service = g_browser_process->safe_browsing_service(); @@ -108,41 +143,6 @@ const std::string& CertificateReportingTest::GetLatestHostnameReported() const { return reporter_->latest_hostname_reported(); } -// This is a test implementation of the interface that blocking pages -// use to send certificate reports. It checks that the blocking page -// calls or does not call the report method when a report should or -// should not be sent, respectively. -class MockSSLCertReporter : public SSLCertReporter { - public: - MockSSLCertReporter( - const scoped_refptr<SafeBrowsingUIManager>& safe_browsing_ui_manager, - const base::Closure& report_sent_callback) - : safe_browsing_ui_manager_(safe_browsing_ui_manager), - reported_(false), - expect_report_(false), - report_sent_callback_(report_sent_callback) {} - - ~MockSSLCertReporter() override { EXPECT_EQ(expect_report_, reported_); } - - // SSLCertReporter implementation - void ReportInvalidCertificateChain( - const std::string& serialized_report) override { - reported_ = true; - if (expect_report_) { - safe_browsing_ui_manager_->ReportInvalidCertificateChain( - serialized_report, report_sent_callback_); - } - } - - void set_expect_report(bool expect_report) { expect_report_ = expect_report; } - - private: - const scoped_refptr<SafeBrowsingUIManager> safe_browsing_ui_manager_; - bool reported_; - bool expect_report_; - base::Closure report_sent_callback_; -}; - void SetCertReportingOptIn(Browser* browser, OptIn opt_in) { browser->profile()->GetPrefs()->SetBoolean( prefs::kSafeBrowsingExtendedReportingEnabled, @@ -180,9 +180,9 @@ ExpectReport GetReportExpectedFromFinch() { return CERT_REPORT_NOT_EXPECTED; if (sendingThreshold == 1.0) - return CertificateReportingTestUtils::CERT_REPORT_EXPECTED; + return certificate_reporting_test_utils::CERT_REPORT_EXPECTED; } - return CertificateReportingTestUtils::CERT_REPORT_NOT_EXPECTED; + return certificate_reporting_test_utils::CERT_REPORT_NOT_EXPECTED; } -} // namespace CertificateReportingTestUtils +} // namespace certificate_reporting_test_utils diff --git a/chrome/browser/ssl/certificate_reporting_test_utils.h b/chrome/browser/ssl/certificate_reporting_test_utils.h index a397e70..81d3d91 100644 --- a/chrome/browser/ssl/certificate_reporting_test_utils.h +++ b/chrome/browser/ssl/certificate_reporting_test_utils.h @@ -18,14 +18,10 @@ namespace net { class URLRequestContext; } -namespace CertificateReportingTestUtils { - -class MockReporter; +namespace certificate_reporting_test_utils { enum OptIn { EXTENDED_REPORTING_OPT_IN, EXTENDED_REPORTING_DO_NOT_OPT_IN }; -enum Proceed { SSL_INTERSTITIAL_PROCEED, SSL_INTERSTITIAL_DO_NOT_PROCEED }; - enum ExpectReport { CERT_REPORT_EXPECTED, CERT_REPORT_NOT_EXPECTED }; // A test class that tracks the latest hostname for which a certificate @@ -37,10 +33,12 @@ class CertificateReportingTest : public InProcessBrowserTest { void SetUpMockReporter(); protected: - // Get the latest hostname for which a certificate report was sent. + // Get the latest hostname for which a certificate report was + // sent. SetUpMockReporter() must have been called before this. const std::string& GetLatestHostnameReported() const; private: + class MockReporter; MockReporter* reporter_; }; @@ -59,6 +57,6 @@ scoped_ptr<SSLCertReporter> SetUpMockSSLCertReporter( // if the user opts in. ExpectReport GetReportExpectedFromFinch(); -} // namespace CertificateReportingTestUtils +} // namespace certificate_reporting_test_utils #endif // CHROME_BROWSER_SSL_CERTIFICATE_REPORTING_TEST_UTILS_H_ diff --git a/chrome/browser/ssl/chrome_fraudulent_certificate_reporter.cc b/chrome/browser/ssl/chrome_fraudulent_certificate_reporter.cc index 4e229b5..0447b0e 100644 --- a/chrome/browser/ssl/chrome_fraudulent_certificate_reporter.cc +++ b/chrome/browser/ssl/chrome_fraudulent_certificate_reporter.cc @@ -52,8 +52,5 @@ void ChromeFraudulentCertificateReporter::SendReport( return; } - certificate_reporter_->SendReport( - chrome_browser_net::CertificateErrorReporter:: - REPORT_TYPE_PINNING_VIOLATION, - serialized_report); + certificate_reporter_->SendPinningViolationReport(serialized_report); } diff --git a/chrome/browser/ssl/chrome_fraudulent_certificate_reporter_unittest.cc b/chrome/browser/ssl/chrome_fraudulent_certificate_reporter_unittest.cc index 40051f9..4f81a6b 100644 --- a/chrome/browser/ssl/chrome_fraudulent_certificate_reporter_unittest.cc +++ b/chrome/browser/ssl/chrome_fraudulent_certificate_reporter_unittest.cc @@ -152,11 +152,10 @@ class MockReporter : public CertificateErrorReporter { request_context, net::CertificateReportSender::DO_NOT_SEND_COOKIES))) {} - void SendReport(ReportType type, - const std::string& serialized_report) override { - EXPECT_EQ(type, REPORT_TYPE_PINNING_VIOLATION); + void SendPinningViolationReport( + const std::string& serialized_report) override { EXPECT_FALSE(serialized_report.empty()); - CertificateErrorReporter::SendReport(type, serialized_report); + CertificateErrorReporter::SendPinningViolationReport(serialized_report); } }; diff --git a/chrome/browser/ssl/ssl_browser_tests.cc b/chrome/browser/ssl/ssl_browser_tests.cc index 7391243..66f94a7 100644 --- a/chrome/browser/ssl/ssl_browser_tests.cc +++ b/chrome/browser/ssl/ssl_browser_tests.cc @@ -89,6 +89,11 @@ const base::FilePath::CharType kDocRoot[] = namespace { +enum ProceedDecision { + SSL_INTERSTITIAL_PROCEED, + SSL_INTERSTITIAL_DO_NOT_PROCEED +}; + class ProvisionalLoadWaiter : public content::WebContentsObserver { public: explicit ProvisionalLoadWaiter(WebContents* tab) @@ -197,7 +202,7 @@ void CheckSecurityState(WebContents* tab, } // namespace class SSLUITest - : public CertificateReportingTestUtils::CertificateReportingTest { + : public certificate_reporting_test_utils::CertificateReportingTest { public: SSLUITest() : https_server_(net::SpawnedTestServer::TYPE_HTTPS, @@ -378,9 +383,9 @@ class SSLUITest // Helper function for testing invalid certificate chain reporting. void TestBrokenHTTPSReporting( - CertificateReportingTestUtils::OptIn opt_in, - CertificateReportingTestUtils::Proceed proceed, - CertificateReportingTestUtils::ExpectReport expect_report, + certificate_reporting_test_utils::OptIn opt_in, + ProceedDecision proceed, + certificate_reporting_test_utils::ExpectReport expect_report, Browser* browser) { base::RunLoop run_loop; ASSERT_TRUE(https_server_expired_.Start()); @@ -388,7 +393,7 @@ class SSLUITest ASSERT_NO_FATAL_FAILURE(SetUpMockReporter()); // Opt in to sending reports for invalid certificate chains. - CertificateReportingTestUtils::SetCertReportingOptIn(browser, opt_in); + certificate_reporting_test_utils::SetCertReportingOptIn(browser, opt_in); ui_test_utils::NavigateToURL(browser, https_server_expired_.GetURL("/")); @@ -397,8 +402,8 @@ class SSLUITest AuthState::SHOWING_INTERSTITIAL); scoped_ptr<SSLCertReporter> ssl_cert_reporter = - CertificateReportingTestUtils::SetUpMockSSLCertReporter(&run_loop, - expect_report); + certificate_reporting_test_utils::SetUpMockSSLCertReporter( + &run_loop, expect_report); SSLBlockingPage* interstitial_page = static_cast<SSLBlockingPage*>( tab->GetInterstitialPage()->GetDelegateForTesting()); @@ -407,7 +412,7 @@ class SSLUITest EXPECT_EQ(std::string(), GetLatestHostnameReported()); // Leave the interstitial (either by proceeding or going back) - if (proceed == CertificateReportingTestUtils::SSL_INTERSTITIAL_PROCEED) { + if (proceed == SSL_INTERSTITIAL_PROCEED) { ProceedThroughInterstitial(tab); } else { // Click "Take me back" @@ -416,7 +421,8 @@ class SSLUITest interstitial_page->DontProceed(); } - if (expect_report == CertificateReportingTestUtils::CERT_REPORT_EXPECTED) { + if (expect_report == + certificate_reporting_test_utils::CERT_REPORT_EXPECTED) { // Check that the mock reporter received a request to send a report. run_loop.Run(); EXPECT_EQ(https_server_expired_.GetURL("/").host(), @@ -1151,24 +1157,22 @@ IN_PROC_BROWSER_TEST_F(SSLUITest, MAYBE_TestDisplaysInsecureContent) { // is sent or not sent depending on the Finch config. IN_PROC_BROWSER_TEST_F(SSLUITestWithExtendedReporting, TestBrokenHTTPSProceedReporting) { - CertificateReportingTestUtils::ExpectReport expect_report = - CertificateReportingTestUtils::GetReportExpectedFromFinch(); + certificate_reporting_test_utils::ExpectReport expect_report = + certificate_reporting_test_utils::GetReportExpectedFromFinch(); TestBrokenHTTPSReporting( - CertificateReportingTestUtils::EXTENDED_REPORTING_OPT_IN, - CertificateReportingTestUtils::SSL_INTERSTITIAL_PROCEED, expect_report, - browser()); + certificate_reporting_test_utils::EXTENDED_REPORTING_OPT_IN, + SSL_INTERSTITIAL_PROCEED, expect_report, browser()); } // Test that if the user goes back and the checkbox is checked, a report // is sent or not sent depending on the Finch config. IN_PROC_BROWSER_TEST_F(SSLUITestWithExtendedReporting, TestBrokenHTTPSGoBackReporting) { - CertificateReportingTestUtils::ExpectReport expect_report = - CertificateReportingTestUtils::GetReportExpectedFromFinch(); + certificate_reporting_test_utils::ExpectReport expect_report = + certificate_reporting_test_utils::GetReportExpectedFromFinch(); TestBrokenHTTPSReporting( - CertificateReportingTestUtils::EXTENDED_REPORTING_OPT_IN, - CertificateReportingTestUtils::SSL_INTERSTITIAL_DO_NOT_PROCEED, - expect_report, browser()); + certificate_reporting_test_utils::EXTENDED_REPORTING_OPT_IN, + SSL_INTERSTITIAL_DO_NOT_PROCEED, expect_report, browser()); } // User proceeds, checkbox is shown but unchecked. Reports should never @@ -1176,9 +1180,9 @@ IN_PROC_BROWSER_TEST_F(SSLUITestWithExtendedReporting, IN_PROC_BROWSER_TEST_F(SSLUITestWithExtendedReporting, TestBrokenHTTPSProceedReportingWithNoOptIn) { TestBrokenHTTPSReporting( - CertificateReportingTestUtils::EXTENDED_REPORTING_DO_NOT_OPT_IN, - CertificateReportingTestUtils::SSL_INTERSTITIAL_PROCEED, - CertificateReportingTestUtils::CERT_REPORT_NOT_EXPECTED, browser()); + certificate_reporting_test_utils::EXTENDED_REPORTING_DO_NOT_OPT_IN, + SSL_INTERSTITIAL_PROCEED, + certificate_reporting_test_utils::CERT_REPORT_NOT_EXPECTED, browser()); } // User goes back, checkbox is shown but unchecked. Reports should never @@ -1186,9 +1190,9 @@ IN_PROC_BROWSER_TEST_F(SSLUITestWithExtendedReporting, IN_PROC_BROWSER_TEST_F(SSLUITestWithExtendedReporting, TestBrokenHTTPSGoBackShowYesCheckNoParamYesReportNo) { TestBrokenHTTPSReporting( - CertificateReportingTestUtils::EXTENDED_REPORTING_DO_NOT_OPT_IN, - CertificateReportingTestUtils::SSL_INTERSTITIAL_DO_NOT_PROCEED, - CertificateReportingTestUtils::CERT_REPORT_NOT_EXPECTED, browser()); + certificate_reporting_test_utils::EXTENDED_REPORTING_DO_NOT_OPT_IN, + SSL_INTERSTITIAL_DO_NOT_PROCEED, + certificate_reporting_test_utils::CERT_REPORT_NOT_EXPECTED, browser()); } // User proceeds, checkbox is not shown but checked -> we expect no @@ -1199,9 +1203,9 @@ IN_PROC_BROWSER_TEST_F(SSLUITestWithExtendedReporting, CertReportHelper::kFinchExperimentName) == CertReportHelper::kFinchGroupDontShowDontSend) { TestBrokenHTTPSReporting( - CertificateReportingTestUtils::EXTENDED_REPORTING_OPT_IN, - CertificateReportingTestUtils::SSL_INTERSTITIAL_PROCEED, - CertificateReportingTestUtils::CERT_REPORT_NOT_EXPECTED, browser()); + certificate_reporting_test_utils::EXTENDED_REPORTING_OPT_IN, + SSL_INTERSTITIAL_PROCEED, + certificate_reporting_test_utils::CERT_REPORT_NOT_EXPECTED, browser()); } } @@ -1210,9 +1214,9 @@ IN_PROC_BROWSER_TEST_F(SSLUITestWithExtendedReporting, IN_PROC_BROWSER_TEST_F(SSLUITestWithExtendedReporting, TestBrokenHTTPSInIncognitoReportNo) { TestBrokenHTTPSReporting( - CertificateReportingTestUtils::EXTENDED_REPORTING_OPT_IN, - CertificateReportingTestUtils::SSL_INTERSTITIAL_PROCEED, - CertificateReportingTestUtils::CERT_REPORT_NOT_EXPECTED, + certificate_reporting_test_utils::EXTENDED_REPORTING_OPT_IN, + SSL_INTERSTITIAL_PROCEED, + certificate_reporting_test_utils::CERT_REPORT_NOT_EXPECTED, CreateIncognitoBrowser()); } @@ -1223,9 +1227,9 @@ IN_PROC_BROWSER_TEST_F(SSLUITestWithExtendedReporting, browser()->profile()->GetPrefs()->SetBoolean( prefs::kSafeBrowsingExtendedReportingOptInAllowed, false); TestBrokenHTTPSReporting( - CertificateReportingTestUtils::EXTENDED_REPORTING_OPT_IN, - CertificateReportingTestUtils::SSL_INTERSTITIAL_PROCEED, - CertificateReportingTestUtils::CERT_REPORT_NOT_EXPECTED, browser()); + certificate_reporting_test_utils::EXTENDED_REPORTING_OPT_IN, + SSL_INTERSTITIAL_PROCEED, + certificate_reporting_test_utils::CERT_REPORT_NOT_EXPECTED, browser()); } // Visits a page that runs insecure content and tries to suppress the insecure diff --git a/net/url_request/certificate_report_sender.cc b/net/url_request/certificate_report_sender.cc index 0361a32..fd5d11c 100644 --- a/net/url_request/certificate_report_sender.cc +++ b/net/url_request/certificate_report_sender.cc @@ -21,6 +21,7 @@ CertificateReportSender::CertificateReportSender( cookies_preference_(cookies_preference) {} CertificateReportSender::~CertificateReportSender() { + // Cancel all of the uncompleted requests. STLDeleteElements(&inflight_requests_); } @@ -43,7 +44,11 @@ void CertificateReportSender::Send(const GURL& report_uri, void CertificateReportSender::OnResponseStarted(URLRequest* request) { // TODO(estark): call a callback so that the caller can print a // warning on failure. - RequestComplete(request); + DVLOG(1) << "Failed to send certificate report for " << request->url().host(); + + CHECK_GT(inflight_requests_.erase(request), 0u); + // Clean up the request, which cancels it. + delete request; } void CertificateReportSender::OnReadCompleted(URLRequest* request, @@ -66,11 +71,4 @@ scoped_ptr<URLRequest> CertificateReportSender::CreateURLRequest( return request.Pass(); } -void CertificateReportSender::RequestComplete(URLRequest* request) { - std::set<URLRequest*>::iterator i = inflight_requests_.find(request); - CHECK(i != inflight_requests_.end()); - scoped_ptr<URLRequest> url_request(*i); - inflight_requests_.erase(i); -} - } // namespace net diff --git a/net/url_request/certificate_report_sender.h b/net/url_request/certificate_report_sender.h index 9073836..dac8a57 100644 --- a/net/url_request/certificate_report_sender.h +++ b/net/url_request/certificate_report_sender.h @@ -34,17 +34,17 @@ class NET_EXPORT CertificateReportSender // Constructs a CertificateReportSender that sends reports with the // given |request_context| and includes or excludes cookies based on - // |cookies_preference|. Ownership of |request_context| is not - // transferred, so it must outlive the CertificateReportSender. + // |cookies_preference|. |request_context| must outlive the + // CertificateReportSender. CertificateReportSender(URLRequestContext* request_context, CookiesPreference cookies_preference); ~CertificateReportSender() override; - // TransportSecurityState::ReportSender + // TransportSecurityState::ReportSender implementation. void Send(const GURL& report_uri, const std::string& report) override; - // net::URLRequest::Delegate + // net::URLRequest::Delegate implementation. void OnResponseStarted(URLRequest* request) override; void OnReadCompleted(URLRequest* request, int bytes_read) override; @@ -58,9 +58,6 @@ class NET_EXPORT CertificateReportSender net::URLRequestContext* context, const GURL& report_uri); - // Performs post-report cleanup. - void RequestComplete(net::URLRequest* request); - net::URLRequestContext* const request_context_; CookiesPreference cookies_preference_; diff --git a/net/url_request/certificate_report_sender_unittest.cc b/net/url_request/certificate_report_sender_unittest.cc index ca652f7..0024bcb 100644 --- a/net/url_request/certificate_report_sender_unittest.cc +++ b/net/url_request/certificate_report_sender_unittest.cc @@ -21,7 +21,6 @@ #include "testing/gtest/include/gtest/gtest.h" namespace net { - namespace { const char kDummyReport[] = "foo.test"; @@ -31,20 +30,11 @@ void MarkURLRequestDestroyed(bool* url_request_destroyed) { *url_request_destroyed = true; } -void SetUrlRequestMocksEnabled(bool enable) { - URLRequestFilter::GetInstance()->ClearHandlers(); - if (!enable) - return; - - URLRequestFailedJob::AddUrlHandler(); - URLRequestMockDataJob::AddUrlHandler(); -} - -// Check that data uploaded in the request matches the test report -// data. The sent reports will be erased from |expect_reports|. -void CheckUploadData(URLRequest* request, +// Checks that data uploaded in the request matches the test report +// data. Erases the sent reports from |expect_reports|. +void CheckUploadData(const URLRequest& request, std::set<std::string>* expect_reports) { - const UploadDataStream* upload = request->get_upload(); + const UploadDataStream* upload = request.get_upload(); ASSERT_TRUE(upload); ASSERT_TRUE(upload->GetElementReaders()); ASSERT_EQ(1u, upload->GetElementReaders()->size()); @@ -69,20 +59,16 @@ class TestCertificateReportSenderNetworkDelegate : public NetworkDelegateImpl { num_requests_(0), expect_cookies_(false) {} - ~TestCertificateReportSenderNetworkDelegate() override {} - void ExpectReport(const std::string& report) { expect_reports_.insert(report); } - void set_all_url_requests_destroyed_callback( - const base::Closure& all_url_requests_destroyed_callback) { - all_url_requests_destroyed_callback_ = all_url_requests_destroyed_callback; + void set_all_url_requests_destroyed_callback(const base::Closure& callback) { + all_url_requests_destroyed_callback_ = callback; } - void set_url_request_destroyed_callback( - const base::Closure& url_request_destroyed_callback) { - url_request_destroyed_callback_ = url_request_destroyed_callback; + void set_url_request_destroyed_callback(const base::Closure& callback) { + url_request_destroyed_callback_ = callback; } void set_expect_url(const GURL& expect_url) { expect_url_ = expect_url; } @@ -94,7 +80,7 @@ class TestCertificateReportSenderNetworkDelegate : public NetworkDelegateImpl { expect_cookies_ = expect_cookies; } - // NetworkDelegateImpl implementation + // NetworkDelegateImpl implementation. int OnBeforeURLRequest(URLRequest* request, const CompletionCallback& callback, GURL* new_url) override { @@ -110,7 +96,10 @@ class TestCertificateReportSenderNetworkDelegate : public NetworkDelegateImpl { EXPECT_TRUE(request->load_flags() & LOAD_DO_NOT_SAVE_COOKIES); } - CheckUploadData(request, &expect_reports_); + CheckUploadData(*request, &expect_reports_); + + // Unconditionally return OK, since the sender ignores the results + // anyway. return OK; } @@ -134,17 +123,17 @@ class TestCertificateReportSenderNetworkDelegate : public NetworkDelegateImpl { class CertificateReportSenderTest : public ::testing::Test { public: CertificateReportSenderTest() : context_(true) { - SetUrlRequestMocksEnabled(true); context_.set_network_delegate(&network_delegate_); context_.Init(); } - ~CertificateReportSenderTest() override { SetUrlRequestMocksEnabled(false); } - - TestCertificateReportSenderNetworkDelegate* network_delegate() { - return &network_delegate_; + void SetUp() override { + URLRequestFailedJob::AddUrlHandler(); + URLRequestMockDataJob::AddUrlHandler(); } + void TearDown() override { URLRequestFilter::GetInstance()->ClearHandlers(); } + TestURLRequestContext* context() { return &context_; } protected: @@ -171,8 +160,9 @@ class CertificateReportSenderTest : public ::testing::Test { EXPECT_EQ(request_sequence_number + 1, network_delegate_.num_requests()); } - private: TestCertificateReportSenderNetworkDelegate network_delegate_; + + private: TestURLRequestContext context_; }; @@ -195,47 +185,47 @@ TEST_F(CertificateReportSenderTest, SendMultipleReportsSequentially) { TEST_F(CertificateReportSenderTest, SendMultipleReportsSimultaneously) { base::RunLoop run_loop; - network_delegate()->set_all_url_requests_destroyed_callback( + network_delegate_.set_all_url_requests_destroyed_callback( run_loop.QuitClosure()); GURL url = URLRequestMockDataJob::GetMockHttpsUrl("dummy data", 1); - network_delegate()->set_expect_url(url); - network_delegate()->ExpectReport(kDummyReport); - network_delegate()->ExpectReport(kSecondDummyReport); + network_delegate_.set_expect_url(url); + network_delegate_.ExpectReport(kDummyReport); + network_delegate_.ExpectReport(kSecondDummyReport); CertificateReportSender reporter( context(), CertificateReportSender::DO_NOT_SEND_COOKIES); - EXPECT_EQ(0u, network_delegate()->num_requests()); + EXPECT_EQ(0u, network_delegate_.num_requests()); reporter.Send(url, kDummyReport); reporter.Send(url, kSecondDummyReport); run_loop.Run(); - EXPECT_EQ(2u, network_delegate()->num_requests()); + EXPECT_EQ(2u, network_delegate_.num_requests()); } // Test that pending URLRequests get cleaned up when the report sender // is deleted. TEST_F(CertificateReportSenderTest, PendingRequestGetsDeleted) { bool url_request_destroyed = false; - network_delegate()->set_url_request_destroyed_callback(base::Bind( + network_delegate_.set_url_request_destroyed_callback(base::Bind( &MarkURLRequestDestroyed, base::Unretained(&url_request_destroyed))); GURL url = URLRequestFailedJob::GetMockHttpUrlWithFailurePhase( URLRequestFailedJob::START, ERR_IO_PENDING); - network_delegate()->set_expect_url(url); - network_delegate()->ExpectReport(kDummyReport); + network_delegate_.set_expect_url(url); + network_delegate_.ExpectReport(kDummyReport); - EXPECT_EQ(0u, network_delegate()->num_requests()); + EXPECT_EQ(0u, network_delegate_.num_requests()); scoped_ptr<CertificateReportSender> reporter(new CertificateReportSender( context(), CertificateReportSender::DO_NOT_SEND_COOKIES)); reporter->Send(url, kDummyReport); reporter.reset(); - EXPECT_EQ(1u, network_delegate()->num_requests()); + EXPECT_EQ(1u, network_delegate_.num_requests()); EXPECT_TRUE(url_request_destroyed); } @@ -256,7 +246,7 @@ TEST_F(CertificateReportSenderTest, SendCookiesPreference) { CertificateReportSender reporter(context(), CertificateReportSender::SEND_COOKIES); - network_delegate()->set_expect_cookies(true); + network_delegate_.set_expect_cookies(true); SendReport(&reporter, kDummyReport, url, 0); } @@ -265,10 +255,9 @@ TEST_F(CertificateReportSenderTest, DoNotSendCookiesPreference) { CertificateReportSender reporter( context(), CertificateReportSender::DO_NOT_SEND_COOKIES); - network_delegate()->set_expect_cookies(false); + network_delegate_.set_expect_cookies(false); SendReport(&reporter, kDummyReport, url, 0); } } // namespace - } // namespace net |