Report Enum metrics from CertificateChecker.
The certificate checker was reporting a "user action" whenever an
update check HTTPS connection or HTTPS payload download had an invalid
HTTPS certificate or a valid one that was changed since the last
connection to the same server.
This patch sends an Enum metric for every HTTPS connection to check for
and update or download the payload with one of the three options: an
invalid certificate, a valid one already seen or a valid but different
certificate.
This patch also moves these metrics to the metrics.{h,cc} module, where
all the other metrics are reported, using an observer pattern in the
CertificateChecker, needed to remove the dependency on the metrics
library from the libpayload_consumer.
Bug: 25818567
TEST=FEATURES=test emerge-link update_engine; mma;
Change-Id: Ia1b6eb799e13b439b520ba14549d8973e18bcbfa
diff --git a/common/certificate_checker.cc b/common/certificate_checker.cc
index 87f9848..b8d8c94 100644
--- a/common/certificate_checker.cc
+++ b/common/certificate_checker.cc
@@ -18,15 +18,16 @@
#include <string>
+#include <base/lazy_instance.h>
#include <base/logging.h>
#include <base/strings/string_number_conversions.h>
#include <base/strings/string_util.h>
#include <base/strings/stringprintf.h>
+#include <base/threading/thread_local.h>
#include <curl/curl.h>
#include <openssl/evp.h>
#include <openssl/ssl.h>
-#include "metrics/metrics_library.h"
#include "update_engine/common/constants.h"
#include "update_engine/common/prefs_interface.h"
#include "update_engine/common/utils.h"
@@ -36,11 +37,10 @@
namespace chromeos_update_engine {
namespace {
-// This should be in the same order of CertificateChecker::ServerToCheck, with
-// the exception of kNone.
-static const char* kReportToSendKey[2] =
- {kPrefsCertificateReportToSendUpdate,
- kPrefsCertificateReportToSendDownload};
+// A lazily created thread local storage for passing the current certificate
+// checker to the openssl callback.
+base::LazyInstance<base::ThreadLocalPointer<CertificateChecker>>::Leaky
+ lazy_tls_ptr;
} // namespace
bool OpenSSLWrapper::GetCertificateDigest(X509_STORE_CTX* x509_ctx,
@@ -63,69 +63,53 @@
return success;
}
-// static
-SystemState* CertificateChecker::system_state_ = nullptr;
-
-// static
-OpenSSLWrapper* CertificateChecker::openssl_wrapper_ = nullptr;
+CertificateChecker::CertificateChecker(PrefsInterface* prefs,
+ OpenSSLWrapper* openssl_wrapper,
+ ServerToCheck server_to_check)
+ : prefs_(prefs),
+ openssl_wrapper_(openssl_wrapper),
+ server_to_check_(server_to_check) {
+}
// static
CURLcode CertificateChecker::ProcessSSLContext(CURL* curl_handle,
SSL_CTX* ssl_ctx,
void* ptr) {
+ CertificateChecker* cert_checker = reinterpret_cast<CertificateChecker*>(ptr);
+
// From here we set the SSL_CTX to another callback, from the openssl library,
// which will be called after each server certificate is validated. However,
// since openssl does not allow us to pass our own data pointer to the
- // callback, the certificate check will have to be done statically. Since we
- // need to know which update server we are using in order to check the
- // certificate, we hardcode Chrome OS's two known update servers here, and
- // define a different static callback for each. Since this code should only
- // run in official builds, this should not be a problem. However, if an update
- // server different from the ones listed here is used, the check will not
- // take place.
- ServerToCheck* server_to_check = reinterpret_cast<ServerToCheck*>(ptr);
-
- // We check which server to check and set the appropriate static callback.
- if (*server_to_check == kUpdate)
- SSL_CTX_set_verify(ssl_ctx, SSL_VERIFY_PEER, VerifySSLCallbackUpdateCheck);
- if (*server_to_check == kDownload)
- SSL_CTX_set_verify(ssl_ctx, SSL_VERIFY_PEER, VerifySSLCallbackDownload);
+ // callback, the certificate check will have to be done statically. To pass
+ // the pointer to this instance, we use a thread-safe pointer in lazy_tls_ptr
+ // during the calls and clear them after it.
+ CHECK(cert_checker != nullptr);
+ lazy_tls_ptr.Pointer()->Set(cert_checker);
+ SSL_CTX_set_verify(ssl_ctx, SSL_VERIFY_PEER, VerifySSLCallback);
+ // Sanity check: We should not re-enter this method during certificate
+ // checking.
+ CHECK(lazy_tls_ptr.Pointer()->Get() == cert_checker);
+ lazy_tls_ptr.Pointer()->Set(nullptr);
return CURLE_OK;
}
// static
-int CertificateChecker::VerifySSLCallbackUpdateCheck(int preverify_ok,
- X509_STORE_CTX* x509_ctx) {
- return CertificateChecker::CheckCertificateChange(
- kUpdate, preverify_ok, x509_ctx) ? 1 : 0;
+int CertificateChecker::VerifySSLCallback(int preverify_ok,
+ X509_STORE_CTX* x509_ctx) {
+ CertificateChecker* cert_checker = lazy_tls_ptr.Pointer()->Get();
+ CHECK(cert_checker != nullptr);
+ return cert_checker->CheckCertificateChange(preverify_ok, x509_ctx) ? 1 : 0;
}
-// static
-int CertificateChecker::VerifySSLCallbackDownload(int preverify_ok,
- X509_STORE_CTX* x509_ctx) {
- return CertificateChecker::CheckCertificateChange(
- kDownload, preverify_ok, x509_ctx) ? 1 : 0;
-}
-
-// static
-bool CertificateChecker::CheckCertificateChange(
- ServerToCheck server_to_check, int preverify_ok,
- X509_STORE_CTX* x509_ctx) {
- static const char kUMAActionCertChanged[] =
- "Updater.ServerCertificateChanged";
- static const char kUMAActionCertFailed[] = "Updater.ServerCertificateFailed";
- TEST_AND_RETURN_FALSE(system_state_ != nullptr);
- TEST_AND_RETURN_FALSE(system_state_->prefs() != nullptr);
- TEST_AND_RETURN_FALSE(server_to_check != kNone);
+bool CertificateChecker::CheckCertificateChange(int preverify_ok,
+ X509_STORE_CTX* x509_ctx) {
+ TEST_AND_RETURN_FALSE(prefs_ != nullptr);
// If pre-verification failed, we are not interested in the current
// certificate. We store a report to UMA and just propagate the fail result.
if (!preverify_ok) {
- LOG_IF(WARNING, !system_state_->prefs()->SetString(
- kReportToSendKey[server_to_check], kUMAActionCertFailed))
- << "Failed to store UMA report on a failure to validate "
- << "certificate from update server.";
+ NotifyCertificateChecked(CertificateCheckResult::kFailed);
return false;
}
@@ -139,6 +123,7 @@
digest)) {
LOG(WARNING) << "Failed to generate digest of X509 certificate "
<< "from update server.";
+ NotifyCertificateChecked(CertificateCheckResult::kValid);
return true;
}
@@ -146,57 +131,40 @@
// prefs.
string digest_string = base::HexEncode(digest, digest_length);
- string storage_key = base::StringPrintf("%s-%d-%d",
- kPrefsUpdateServerCertificate,
- server_to_check,
- depth);
+ string storage_key =
+ base::StringPrintf("%s-%d-%d", kPrefsUpdateServerCertificate,
+ static_cast<int>(server_to_check_), depth);
string stored_digest;
// If there's no stored certificate, we just store the current one and return.
- if (!system_state_->prefs()->GetString(storage_key, &stored_digest)) {
- LOG_IF(WARNING, !system_state_->prefs()->SetString(storage_key,
- digest_string))
- << "Failed to store server certificate on storage key " << storage_key;
+ if (!prefs_->GetString(storage_key, &stored_digest)) {
+ if (!prefs_->SetString(storage_key, digest_string)) {
+ LOG(WARNING) << "Failed to store server certificate on storage key "
+ << storage_key;
+ }
+ NotifyCertificateChecked(CertificateCheckResult::kValid);
return true;
}
// Certificate changed, we store a report to UMA and store the most recent
// certificate.
if (stored_digest != digest_string) {
- LOG_IF(WARNING, !system_state_->prefs()->SetString(
- kReportToSendKey[server_to_check], kUMAActionCertChanged))
- << "Failed to store UMA report on a change on the "
- << "certificate from update server.";
- LOG_IF(WARNING, !system_state_->prefs()->SetString(storage_key,
- digest_string))
- << "Failed to store server certificate on storage key " << storage_key;
+ if (!prefs_->SetString(storage_key, digest_string)) {
+ LOG(WARNING) << "Failed to store server certificate on storage key "
+ << storage_key;
+ }
+ NotifyCertificateChecked(CertificateCheckResult::kValidChanged);
+ return true;
}
+ NotifyCertificateChecked(CertificateCheckResult::kValid);
// Since we don't perform actual SSL verification, we return success.
return true;
}
-// static
-void CertificateChecker::FlushReport() {
- // This check shouldn't be needed, but it is useful for testing.
- TEST_AND_RETURN(system_state_);
- TEST_AND_RETURN(system_state_->metrics_lib());
- TEST_AND_RETURN(system_state_->prefs());
-
- // We flush reports for both servers.
- for (size_t i = 0; i < arraysize(kReportToSendKey); i++) {
- string report_to_send;
- if (system_state_->prefs()->GetString(kReportToSendKey[i], &report_to_send)
- && !report_to_send.empty()) {
- // There is a report to be sent. We send it and erase it.
- LOG(INFO) << "Found report #" << i << ". Sending it";
- LOG_IF(WARNING, !system_state_->metrics_lib()->SendUserActionToUMA(
- report_to_send))
- << "Failed to send server certificate report to UMA: "
- << report_to_send;
- LOG_IF(WARNING, !system_state_->prefs()->Delete(kReportToSendKey[i]))
- << "Failed to erase server certificate report to be sent to UMA";
- }
- }
+void CertificateChecker::NotifyCertificateChecked(
+ CertificateCheckResult result) {
+ if (observer_)
+ observer_->CertificateChecked(server_to_check_, result);
}
} // namespace chromeos_update_engine
diff --git a/common/certificate_checker.h b/common/certificate_checker.h
index 83c09e1..bc675be 100644
--- a/common/certificate_checker.h
+++ b/common/certificate_checker.h
@@ -25,15 +25,15 @@
#include <base/macros.h>
#include <gtest/gtest_prod.h> // for FRIEND_TEST
-#include "update_engine/system_state.h"
-
namespace chromeos_update_engine {
+class PrefsInterface;
+
// Wrapper for openssl operations with the certificates.
class OpenSSLWrapper {
public:
- OpenSSLWrapper() {}
- virtual ~OpenSSLWrapper() {}
+ OpenSSLWrapper() = default;
+ virtual ~OpenSSLWrapper() = default;
// Takes an openssl X509_STORE_CTX, extracts the corresponding certificate
// from it and calculates its fingerprint (SHA256 digest). Returns true on
@@ -54,78 +54,102 @@
DISALLOW_COPY_AND_ASSIGN(OpenSSLWrapper);
};
+// The values in this enum are replicated in the metrics server. See metrics.h
+// for instructions on how to update these values in the server side.
+enum class CertificateCheckResult {
+ // The certificate is valid and the same as seen before or the first time we
+ // see a certificate.
+ kValid,
+ // The certificate is valid, but is different than a previously seen
+ // certificate for the selected server.
+ kValidChanged,
+ // The certificate validation failed.
+ kFailed,
+
+ // This value must be the last entry.
+ kNumConstants
+};
+
+// These values are used to generate the keys of files persisted via prefs.
+// This means that changing these will cause loss of information on metrics
+// reporting, during the transition. These values are also mapped to a metric
+// name in metrics.cc, so adding values here requires to assign a new metric
+// name in that file.
+enum class ServerToCheck {
+ kUpdate = 0,
+ kDownload,
+};
+
// Responsible for checking whether update server certificates change, and
// reporting to UMA when this happens. Since all state information is persisted,
// and openssl forces us to use a static callback with no data pointer, this
// class is entirely static.
class CertificateChecker {
public:
- // These values are used to generate the keys of files persisted via prefs.
- // This means that changing these will cause loss of information on metrics
- // reporting, during the transition.
- enum ServerToCheck {
- kUpdate = 0,
- kDownload = 1,
- kNone = 2 // This needs to be the last element. Changing its value is ok.
+ class Observer {
+ public:
+ virtual ~Observer() = default;
+
+ // Called whenever a certificate is checked for the server |server_to_check|
+ // passing the result of said certificate check.
+ virtual void CertificateChecked(ServerToCheck server_to_check,
+ CertificateCheckResult result) = 0;
};
- CertificateChecker() {}
- virtual ~CertificateChecker() {}
+ CertificateChecker(PrefsInterface* prefs,
+ OpenSSLWrapper* openssl_wrapper,
+ ServerToCheck server_to_check);
+ virtual ~CertificateChecker() = default;
// This callback is called by libcurl just before the initialization of an
// SSL connection after having processed all other SSL related options. Used
- // to check if server certificates change. |ptr| is expected to be a
- // pointer to a ServerToCheck.
- static CURLcode ProcessSSLContext(CURL* curl_handle, SSL_CTX* ssl_ctx,
- void* ptr);
+ // to check if server certificates change. |cert_checker| is expected to be a
+ // pointer to the CertificateChecker instance.
+ static CURLcode ProcessSSLContext(CURL* curl_handle,
+ SSL_CTX* ssl_ctx,
+ void* cert_checker);
- // Flushes to UMA any certificate-related report that was persisted.
- static void FlushReport();
-
- // Setters.
- static void set_system_state(SystemState* system_state) {
- system_state_ = system_state;
- }
-
- static void set_openssl_wrapper(OpenSSLWrapper* openssl_wrapper) {
- openssl_wrapper_ = openssl_wrapper;
- }
+ // Set the certificate observer to the passed instance. To remove the
+ // observer, pass a nullptr. The |observer| instance must be valid while this
+ // CertificateChecker verifies certificates.
+ void SetObserver(Observer* observer) { observer_ = observer; }
private:
FRIEND_TEST(CertificateCheckerTest, NewCertificate);
FRIEND_TEST(CertificateCheckerTest, SameCertificate);
FRIEND_TEST(CertificateCheckerTest, ChangedCertificate);
FRIEND_TEST(CertificateCheckerTest, FailedCertificate);
- FRIEND_TEST(CertificateCheckerTest, FlushReport);
- FRIEND_TEST(CertificateCheckerTest, FlushNothingToReport);
- // These callbacks are called by openssl after initial SSL verification. They
- // are used to perform any additional security verification on the connection,
+ // This callback is called by openssl after initial SSL verification. It is
+ // used to perform any additional security verification on the connection,
// but we use them here to get hold of the server certificate, in order to
// determine if it has changed since the last connection. Since openssl forces
- // us to do this statically, we define two different callbacks for the two
- // different official update servers, and only assign the correspondent one.
- // The assigned callback is then called once per each certificate on the
- // server and returns 1 for success and 0 for failure.
- static int VerifySSLCallbackUpdateCheck(int preverify_ok,
- X509_STORE_CTX* x509_ctx);
- static int VerifySSLCallbackDownload(int preverify_ok,
- X509_STORE_CTX* x509_ctx);
+ // us to do this statically, we pass a pointer to this instance via a
+ // thread-safe global pointer. The assigned callback is then called once per
+ // each certificate on the server and returns 1 for success and 0 for failure.
+ static int VerifySSLCallback(int preverify_ok, X509_STORE_CTX* x509_ctx);
- // Checks if server certificate for |server_to_check|, stored in |x509_ctx|,
- // has changed since last connection to that same server. This is called by
- // one of the two callbacks defined above. If certificate fails to check or
- // changes, a report is generated and persisted, to be later sent by
- // FlushReport. Returns true on success and false otherwise.
- static bool CheckCertificateChange(ServerToCheck server_to_check,
- int preverify_ok,
- X509_STORE_CTX* x509_ctx);
+ // Checks if server certificate stored in |x509_ctx| has changed since last
+ // connection to that same server, specified by the |server_to_check_| member.
+ // This is called by the callbacks defined above. The result of the
+ // certificate check is passed to the observer, if any. Returns true on
+ // success and false otherwise.
+ bool CheckCertificateChange(int preverify_ok, X509_STORE_CTX* x509_ctx);
- // Global system context.
- static SystemState* system_state_;
+ // Notifies the observer, if any, of a certificate checking.
+ void NotifyCertificateChecked(CertificateCheckResult result);
+
+ // Prefs instance used to store the certificates seen in the past.
+ PrefsInterface* prefs_;
// The wrapper for openssl operations.
- static OpenSSLWrapper* openssl_wrapper_;
+ OpenSSLWrapper* openssl_wrapper_;
+
+ // The server to check from this certificate checker instance.
+ ServerToCheck server_to_check_;
+
+ // The observer called whenever a certificate is checked, if not null.
+ Observer* observer_{nullptr};
DISALLOW_COPY_AND_ASSIGN(CertificateChecker);
};
diff --git a/common/certificate_checker_unittest.cc b/common/certificate_checker_unittest.cc
index 3dfdadb..15811c0 100644
--- a/common/certificate_checker_unittest.cc
+++ b/common/certificate_checker_unittest.cc
@@ -22,12 +22,10 @@
#include <base/strings/stringprintf.h>
#include <gmock/gmock.h>
#include <gtest/gtest.h>
-#include <metrics/metrics_library_mock.h>
#include "update_engine/common/constants.h"
#include "update_engine/common/mock_certificate_checker.h"
#include "update_engine/common/mock_prefs.h"
-#include "update_engine/fake_system_state.h"
using ::testing::DoAll;
using ::testing::Return;
@@ -38,49 +36,41 @@
namespace chromeos_update_engine {
-class CertificateCheckerTest : public testing::Test {
+class MockCertificateCheckObserver : public CertificateChecker::Observer {
public:
- CertificateCheckerTest() {}
+ MOCK_METHOD2(CertificateChecked,
+ void(ServerToCheck server_to_check,
+ CertificateCheckResult result));
+};
+class CertificateCheckerTest : public testing::Test {
protected:
void SetUp() override {
- depth_ = 0;
- length_ = 4;
- digest_[0] = 0x17;
- digest_[1] = 0x7D;
- digest_[2] = 0x07;
- digest_[3] = 0x5F;
- digest_hex_ = "177D075F";
- diff_digest_hex_ = "1234ABCD";
- cert_key_prefix_ = kPrefsUpdateServerCertificate;
- server_to_check_ = CertificateChecker::kUpdate;
cert_key_ = base::StringPrintf("%s-%d-%d",
cert_key_prefix_.c_str(),
- server_to_check_,
+ static_cast<int>(server_to_check_),
depth_);
- kCertChanged = "Updater.ServerCertificateChanged";
- kCertFailed = "Updater.ServerCertificateFailed";
- CertificateChecker::set_system_state(&fake_system_state_);
- CertificateChecker::set_openssl_wrapper(&openssl_wrapper_);
- prefs_ = fake_system_state_.mock_prefs();
+ cert_checker.SetObserver(&observer_);
}
- void TearDown() override {}
+ void TearDown() override {
+ cert_checker.SetObserver(nullptr);
+ }
- FakeSystemState fake_system_state_;
- MockPrefs* prefs_; // shortcut to fake_system_state_.mock_prefs()
+ MockPrefs prefs_;
MockOpenSSLWrapper openssl_wrapper_;
// Parameters of our mock certificate digest.
- int depth_;
- unsigned int length_;
- uint8_t digest_[4];
- string digest_hex_;
- string diff_digest_hex_;
- string cert_key_prefix_;
- CertificateChecker::ServerToCheck server_to_check_;
+ int depth_{0};
+ unsigned int length_{4};
+ uint8_t digest_[4]{0x17, 0x7D, 0x07, 0x5F};
+ string digest_hex_{"177D075F"};
+ string diff_digest_hex_{"1234ABCD"};
+ string cert_key_prefix_{kPrefsUpdateServerCertificate};
+ ServerToCheck server_to_check_{ServerToCheck::kUpdate};
string cert_key_;
- string kCertChanged;
- string kCertFailed;
+
+ testing::StrictMock<MockCertificateCheckObserver> observer_;
+ CertificateChecker cert_checker{&prefs_, &openssl_wrapper_, server_to_check_};
};
// check certificate change, new
@@ -91,12 +81,12 @@
SetArgumentPointee<2>(length_),
SetArrayArgument<3>(digest_, digest_ + 4),
Return(true)));
- EXPECT_CALL(*prefs_, GetString(cert_key_, _))
- .WillOnce(Return(false));
- EXPECT_CALL(*prefs_, SetString(cert_key_, digest_hex_))
- .WillOnce(Return(true));
- ASSERT_TRUE(CertificateChecker::CheckCertificateChange(
- server_to_check_, 1, nullptr));
+ EXPECT_CALL(prefs_, GetString(cert_key_, _)).WillOnce(Return(false));
+ EXPECT_CALL(prefs_, SetString(cert_key_, digest_hex_)).WillOnce(Return(true));
+ EXPECT_CALL(observer_,
+ CertificateChecked(server_to_check_,
+ CertificateCheckResult::kValid));
+ ASSERT_TRUE(cert_checker.CheckCertificateChange(1, nullptr));
}
// check certificate change, unchanged
@@ -107,13 +97,13 @@
SetArgumentPointee<2>(length_),
SetArrayArgument<3>(digest_, digest_ + 4),
Return(true)));
- EXPECT_CALL(*prefs_, GetString(cert_key_, _))
- .WillOnce(DoAll(
- SetArgumentPointee<1>(digest_hex_),
- Return(true)));
- EXPECT_CALL(*prefs_, SetString(_, _)).Times(0);
- ASSERT_TRUE(CertificateChecker::CheckCertificateChange(
- server_to_check_, 1, nullptr));
+ EXPECT_CALL(prefs_, GetString(cert_key_, _))
+ .WillOnce(DoAll(SetArgumentPointee<1>(digest_hex_), Return(true)));
+ EXPECT_CALL(prefs_, SetString(_, _)).Times(0);
+ EXPECT_CALL(observer_,
+ CertificateChecked(server_to_check_,
+ CertificateCheckResult::kValid));
+ ASSERT_TRUE(cert_checker.CheckCertificateChange(1, nullptr));
}
// check certificate change, changed
@@ -124,61 +114,22 @@
SetArgumentPointee<2>(length_),
SetArrayArgument<3>(digest_, digest_ + 4),
Return(true)));
- EXPECT_CALL(*prefs_, GetString(cert_key_, _))
- .WillOnce(DoAll(
- SetArgumentPointee<1>(diff_digest_hex_),
- Return(true)));
- EXPECT_CALL(*prefs_, SetString(kPrefsCertificateReportToSendUpdate,
- kCertChanged))
- .WillOnce(Return(true));
- EXPECT_CALL(*prefs_, SetString(cert_key_, digest_hex_))
- .WillOnce(Return(true));
- ASSERT_TRUE(CertificateChecker::CheckCertificateChange(
- server_to_check_, 1, nullptr));
+ EXPECT_CALL(prefs_, GetString(cert_key_, _))
+ .WillOnce(DoAll(SetArgumentPointee<1>(diff_digest_hex_), Return(true)));
+ EXPECT_CALL(observer_,
+ CertificateChecked(server_to_check_,
+ CertificateCheckResult::kValidChanged));
+ EXPECT_CALL(prefs_, SetString(cert_key_, digest_hex_)).WillOnce(Return(true));
+ ASSERT_TRUE(cert_checker.CheckCertificateChange(1, nullptr));
}
// check certificate change, failed
TEST_F(CertificateCheckerTest, FailedCertificate) {
- EXPECT_CALL(*prefs_, SetString(kPrefsCertificateReportToSendUpdate,
- kCertFailed))
- .WillOnce(Return(true));
- EXPECT_CALL(*prefs_, GetString(_, _)).Times(0);
+ EXPECT_CALL(observer_, CertificateChecked(server_to_check_,
+ CertificateCheckResult::kFailed));
+ EXPECT_CALL(prefs_, GetString(_, _)).Times(0);
EXPECT_CALL(openssl_wrapper_, GetCertificateDigest(_, _, _, _)).Times(0);
- ASSERT_FALSE(CertificateChecker::CheckCertificateChange(
- server_to_check_, 0, nullptr));
-}
-
-// flush send report
-TEST_F(CertificateCheckerTest, FlushReport) {
- EXPECT_CALL(*prefs_, GetString(kPrefsCertificateReportToSendUpdate, _))
- .WillOnce(DoAll(
- SetArgumentPointee<1>(kCertChanged),
- Return(true)));
- EXPECT_CALL(*prefs_, GetString(kPrefsCertificateReportToSendDownload, _))
- .WillOnce(Return(false));
- EXPECT_CALL(*fake_system_state_.mock_metrics_lib(),
- SendUserActionToUMA(kCertChanged))
- .WillOnce(Return(true));
- EXPECT_CALL(*prefs_, Delete(kPrefsCertificateReportToSendUpdate))
- .WillOnce(Return(true));
- EXPECT_CALL(*prefs_, SetString(kPrefsCertificateReportToSendDownload, _))
- .Times(0);
- CertificateChecker::FlushReport();
-}
-
-// flush nothing to report
-TEST_F(CertificateCheckerTest, FlushNothingToReport) {
- string empty = "";
- EXPECT_CALL(*prefs_, GetString(kPrefsCertificateReportToSendUpdate, _))
- .WillOnce(DoAll(
- SetArgumentPointee<1>(empty),
- Return(true)));
- EXPECT_CALL(*prefs_, GetString(kPrefsCertificateReportToSendDownload, _))
- .WillOnce(Return(false));
- EXPECT_CALL(*fake_system_state_.mock_metrics_lib(),
- SendUserActionToUMA(_)).Times(0);
- EXPECT_CALL(*prefs_, SetString(_, _)).Times(0);
- CertificateChecker::FlushReport();
+ ASSERT_FALSE(cert_checker.CheckCertificateChange(0, nullptr));
}
} // namespace chromeos_update_engine
diff --git a/common/constants.cc b/common/constants.cc
index 26f3628..fe4e643 100644
--- a/common/constants.cc
+++ b/common/constants.cc
@@ -33,10 +33,6 @@
const char kPrefsAttemptInProgress[] = "attempt-in-progress";
const char kPrefsBackoffExpiryTime[] = "backoff-expiry-time";
const char kPrefsBootId[] = "boot-id";
-const char kPrefsCertificateReportToSendDownload[] =
- "certificate-report-to-send-download";
-const char kPrefsCertificateReportToSendUpdate[] =
- "certificate-report-to-send-update";
const char kPrefsCurrentBytesDownloaded[] = "current-bytes-downloaded";
const char kPrefsCurrentResponseSignature[] = "current-response-signature";
const char kPrefsCurrentUrlFailureCount[] = "current-url-failure-count";
diff --git a/common/constants.h b/common/constants.h
index ecdf51c..f20de8e 100644
--- a/common/constants.h
+++ b/common/constants.h
@@ -39,8 +39,6 @@
extern const char kPrefsAttemptInProgress[];
extern const char kPrefsBackoffExpiryTime[];
extern const char kPrefsBootId[];
-extern const char kPrefsCertificateReportToSendDownload[];
-extern const char kPrefsCertificateReportToSendUpdate[];
extern const char kPrefsCurrentBytesDownloaded[];
extern const char kPrefsCurrentResponseSignature[];
extern const char kPrefsCurrentUrlFailureCount[];
diff --git a/common/http_fetcher.h b/common/http_fetcher.h
index 1d4dba9..11e8e9f 100644
--- a/common/http_fetcher.h
+++ b/common/http_fetcher.h
@@ -28,7 +28,6 @@
#include "update_engine/common/http_common.h"
#include "update_engine/proxy_resolver.h"
-#include "update_engine/system_state.h"
// This class is a simple wrapper around an HTTP library (libcurl). We can
// easily mock out this interface for testing.
@@ -45,14 +44,13 @@
// |proxy_resolver| is the resolver that will be consulted for proxy
// settings. It may be null, in which case direct connections will
// be used. Does not take ownership of the resolver.
- HttpFetcher(ProxyResolver* proxy_resolver, SystemState* system_state)
+ HttpFetcher(ProxyResolver* proxy_resolver)
: post_data_set_(false),
http_response_code_(0),
delegate_(nullptr),
proxies_(1, kNoProxy),
proxy_resolver_(proxy_resolver),
- callback_(nullptr),
- system_state_(system_state) {}
+ callback_(nullptr) {}
virtual ~HttpFetcher();
void set_delegate(HttpFetcherDelegate* delegate) { delegate_ = delegate; }
@@ -129,11 +127,6 @@
ProxyResolver* proxy_resolver() const { return proxy_resolver_; }
- // Returns the global SystemState.
- SystemState* GetSystemState() {
- return system_state_;
- }
-
protected:
// The URL we're actively fetching from
std::string url_;
@@ -163,9 +156,6 @@
// Callback for when we are resolving proxies
std::unique_ptr<base::Closure> callback_;
- // Global system context.
- SystemState* system_state_;
-
private:
// Callback from the proxy resolver
void ProxiesResolved(const std::deque<std::string>& proxies);
diff --git a/common/http_fetcher_unittest.cc b/common/http_fetcher_unittest.cc
index a6ba9c8..654cb1d 100644
--- a/common/http_fetcher_unittest.cc
+++ b/common/http_fetcher_unittest.cc
@@ -39,6 +39,7 @@
#include <brillo/streams/stream.h>
#include <gtest/gtest.h>
+#include "update_engine/common/fake_hardware.h"
#include "update_engine/common/http_common.h"
#include "update_engine/common/libcurl_http_fetcher.h"
#include "update_engine/common/mock_http_fetcher.h"
@@ -212,6 +213,10 @@
virtual HttpServer* CreateServer() = 0;
+ FakeHardware* fake_hardware() {
+ return fake_system_state_.fake_hardware();
+ }
+
protected:
DirectProxyResolver proxy_resolver_;
FakeSystemState fake_system_state_;
@@ -913,6 +918,7 @@
};
void MultiTest(HttpFetcher* fetcher_in,
+ FakeHardware* fake_hardware,
const string& url,
const vector<pair<off_t, off_t>>& ranges,
const string& expected_prefix,
@@ -937,8 +943,7 @@
}
LOG(INFO) << "added range: " << tmp_str;
}
- dynamic_cast<FakeSystemState*>(fetcher_in->GetSystemState())
- ->fake_hardware()->SetIsOfficialBuild(false);
+ fake_hardware->SetIsOfficialBuild(false);
multi_fetcher->set_delegate(&delegate);
MessageLoop::current()->PostTask(
@@ -963,6 +968,7 @@
ranges.push_back(make_pair(0, 25));
ranges.push_back(make_pair(99, 0));
MultiTest(this->test_.NewLargeFetcher(),
+ this->test_.fake_hardware(),
this->test_.BigUrl(server->GetPort()),
ranges,
"abcdefghijabcdefghijabcdejabcdefghijabcdef",
@@ -980,6 +986,7 @@
vector<pair<off_t, off_t>> ranges;
ranges.push_back(make_pair(0, 24));
MultiTest(this->test_.NewLargeFetcher(),
+ this->test_.fake_hardware(),
this->test_.BigUrl(server->GetPort()),
ranges,
"abcdefghijabcdefghijabcd",
@@ -998,6 +1005,7 @@
ranges.push_back(make_pair(kBigLength - 2, 0));
ranges.push_back(make_pair(kBigLength - 3, 0));
MultiTest(this->test_.NewLargeFetcher(),
+ this->test_.fake_hardware(),
this->test_.BigUrl(server->GetPort()),
ranges,
"ijhij",
@@ -1017,6 +1025,7 @@
for (int i = 0; i < 2; ++i) {
LOG(INFO) << "i = " << i;
MultiTest(this->test_.NewLargeFetcher(),
+ this->test_.fake_hardware(),
this->test_.BigUrl(server->GetPort()),
ranges,
"ij",
@@ -1042,6 +1051,7 @@
ranges.push_back(make_pair(0, 25));
ranges.push_back(make_pair(99, 0));
MultiTest(this->test_.NewLargeFetcher(3),
+ this->test_.fake_hardware(),
LocalServerUrlForPath(server->GetPort(),
base::StringPrintf("/error-if-offset/%d/2",
kBigLength)),
@@ -1064,6 +1074,7 @@
ranges.push_back(make_pair(0, 25));
ranges.push_back(make_pair(99, 0));
MultiTest(this->test_.NewLargeFetcher(2),
+ this->test_.fake_hardware(),
LocalServerUrlForPath(server->GetPort(),
base::StringPrintf("/error-if-offset/%d/3",
kBigLength)),
@@ -1103,8 +1114,7 @@
unique_ptr<HttpFetcher> fetcher(fetcher_test->NewLargeFetcher());
LOG(INFO) << "is_official_build: " << is_official_build;
// NewLargeFetcher creates the HttpFetcher* with a FakeSystemState.
- dynamic_cast<FakeSystemState*>(fetcher->GetSystemState())
- ->fake_hardware()->SetIsOfficialBuild(is_official_build);
+ fetcher_test->fake_hardware()->SetIsOfficialBuild(is_official_build);
fetcher->set_delegate(&delegate);
MessageLoop::current()->PostTask(FROM_HERE, base::Bind(
diff --git a/common/libcurl_http_fetcher.cc b/common/libcurl_http_fetcher.cc
index d36e32b..5a248e6 100644
--- a/common/libcurl_http_fetcher.cc
+++ b/common/libcurl_http_fetcher.cc
@@ -29,6 +29,7 @@
#include "update_engine/common/certificate_checker.h"
#include "update_engine/common/hardware_interface.h"
#include "update_engine/common/platform_constants.h"
+#include "update_engine/system_state.h"
using base::TimeDelta;
using brillo::MessageLoop;
@@ -44,6 +45,21 @@
const int kNoNetworkRetrySeconds = 10;
} // namespace
+LibcurlHttpFetcher::LibcurlHttpFetcher(
+ ProxyResolver* proxy_resolver,
+ SystemState* system_state,
+ std::unique_ptr<CertificateChecker> certificate_checker)
+ : HttpFetcher(proxy_resolver),
+ hardware_(system_state->hardware()),
+ certificate_checker_(std::move(certificate_checker)) {
+ // Dev users want a longer timeout (180 seconds) because they may
+ // be waiting on the dev server to build an image.
+ if (!hardware_->IsOfficialBuild())
+ low_speed_time_seconds_ = kDownloadDevModeLowSpeedTimeSeconds;
+ if (!hardware_->IsOOBEComplete(nullptr))
+ max_retry_count_ = kDownloadMaxRetryCountOobeNotComplete;
+}
+
LibcurlHttpFetcher::~LibcurlHttpFetcher() {
LOG_IF(ERROR, transfer_in_progress_)
<< "Destroying the fetcher while a transfer is in progress.";
@@ -181,7 +197,7 @@
// Lock down the appropriate curl options for HTTP or HTTPS depending on
// the url.
- if (GetSystemState()->hardware()->IsOfficialBuild()) {
+ if (hardware_->IsOfficialBuild()) {
if (base::StartsWithASCII(url_, "http://", false))
SetCurlOptionsForHttp();
else
@@ -222,9 +238,9 @@
CURLE_OK);
CHECK_EQ(curl_easy_setopt(curl_handle_, CURLOPT_SSL_CIPHER_LIST, "HIGH:!ADH"),
CURLE_OK);
- if (check_certificate_ != CertificateChecker::kNone) {
+ if (certificate_checker_ != nullptr) {
CHECK_EQ(curl_easy_setopt(curl_handle_, CURLOPT_SSL_CTX_DATA,
- &check_certificate_),
+ certificate_checker_.get()),
CURLE_OK);
CHECK_EQ(curl_easy_setopt(curl_handle_, CURLOPT_SSL_CTX_FUNCTION,
CertificateChecker::ProcessSSLContext),
diff --git a/common/libcurl_http_fetcher.h b/common/libcurl_http_fetcher.h
index 52a4111..9e8abda 100644
--- a/common/libcurl_http_fetcher.h
+++ b/common/libcurl_http_fetcher.h
@@ -18,6 +18,7 @@
#define UPDATE_ENGINE_COMMON_LIBCURL_HTTP_FETCHER_H_
#include <map>
+#include <memory>
#include <string>
#include <utility>
@@ -32,7 +33,6 @@
#include "update_engine/common/http_fetcher.h"
#include "update_engine/system_state.h"
-
// This is a concrete implementation of HttpFetcher that uses libcurl to do the
// http work.
@@ -41,15 +41,10 @@
class LibcurlHttpFetcher : public HttpFetcher {
public:
LibcurlHttpFetcher(ProxyResolver* proxy_resolver,
- SystemState* system_state)
- : HttpFetcher(proxy_resolver, system_state) {
- // Dev users want a longer timeout (180 seconds) because they may
- // be waiting on the dev server to build an image.
- if (!system_state->hardware()->IsOfficialBuild())
- low_speed_time_seconds_ = kDownloadDevModeLowSpeedTimeSeconds;
- if (!system_state_->hardware()->IsOOBEComplete(nullptr))
- max_retry_count_ = kDownloadMaxRetryCountOobeNotComplete;
- }
+ SystemState* system_state,
+ std::unique_ptr<CertificateChecker> certificate_checker);
+ LibcurlHttpFetcher(ProxyResolver* proxy_resolver, SystemState* system_state)
+ : LibcurlHttpFetcher(proxy_resolver, system_state, nullptr) {}
// Cleans up all internal state. Does not notify delegate
~LibcurlHttpFetcher() override;
@@ -90,11 +85,6 @@
no_network_max_retries_ = retries;
}
- void set_check_certificate(
- CertificateChecker::ServerToCheck check_certificate) {
- check_certificate_ = check_certificate;
- }
-
size_t GetBytesDownloaded() override {
return static_cast<size_t>(bytes_downloaded_);
}
@@ -180,6 +170,9 @@
// written to |out_type|).
bool GetProxyType(const std::string& proxy, curl_proxytype* out_type);
+ // Hardware interface used to query dev-mode and official build settings.
+ HardwareInterface* hardware_;
+
// Handles for the libcurl library
CURLM* curl_multi_handle_{nullptr};
CURL* curl_handle_{nullptr};
@@ -238,11 +231,9 @@
// if we get a terminate request, queue it until we can handle it.
bool terminate_requested_{false};
- // Represents which server certificate to be checked against this
- // connection's certificate. If no certificate check needs to be performed,
- // this should be kNone.
- CertificateChecker::ServerToCheck check_certificate_{
- CertificateChecker::kNone};
+ // The CertificateChecker used to check this connection's certificate. If no
+ // certificate check needs to be performed, this should be empty.
+ std::unique_ptr<CertificateChecker> certificate_checker_;
int low_speed_limit_bps_{kDownloadLowSpeedLimitBps};
int low_speed_time_seconds_{kDownloadLowSpeedTimeSeconds};
diff --git a/common/mock_http_fetcher.cc b/common/mock_http_fetcher.cc
index f556c34..f3fa70d 100644
--- a/common/mock_http_fetcher.cc
+++ b/common/mock_http_fetcher.cc
@@ -18,7 +18,9 @@
#include <algorithm>
+#include <base/bind.h>
#include <base/logging.h>
+#include <base/time/time.h>
#include <gtest/gtest.h>
// This is a mock implementation of HttpFetcher which is useful for testing.
diff --git a/common/mock_http_fetcher.h b/common/mock_http_fetcher.h
index 16d0504..90d34dd 100644
--- a/common/mock_http_fetcher.h
+++ b/common/mock_http_fetcher.h
@@ -24,8 +24,6 @@
#include <brillo/message_loops/message_loop.h>
#include "update_engine/common/http_fetcher.h"
-#include "update_engine/fake_system_state.h"
-#include "update_engine/mock_connection_manager.h"
// This is a mock implementation of HttpFetcher which is useful for testing.
// All data must be passed into the ctor. When started, MockHttpFetcher will
@@ -46,13 +44,12 @@
MockHttpFetcher(const uint8_t* data,
size_t size,
ProxyResolver* proxy_resolver)
- : HttpFetcher(proxy_resolver, &fake_system_state_),
+ : HttpFetcher(proxy_resolver),
sent_size_(0),
timeout_id_(brillo::MessageLoop::kTaskIdNull),
paused_(false),
fail_transfer_(false),
never_use_(false) {
- fake_system_state_.set_connection_manager(&mock_connection_manager_);
data_.insert(data_.end(), data, data + size);
}
@@ -141,9 +138,6 @@
// Set to true if BeginTransfer should EXPECT fail.
bool never_use_;
- FakeSystemState fake_system_state_;
- MockConnectionManager mock_connection_manager_;
-
DISALLOW_COPY_AND_ASSIGN(MockHttpFetcher);
};
diff --git a/common/multi_range_http_fetcher.h b/common/multi_range_http_fetcher.h
index 265d4cc..8158a22 100644
--- a/common/multi_range_http_fetcher.h
+++ b/common/multi_range_http_fetcher.h
@@ -46,8 +46,7 @@
public:
// Takes ownership of the passed in fetcher.
explicit MultiRangeHttpFetcher(HttpFetcher* base_fetcher)
- : HttpFetcher(base_fetcher->proxy_resolver(),
- base_fetcher->GetSystemState()),
+ : HttpFetcher(base_fetcher->proxy_resolver()),
base_fetcher_(base_fetcher),
base_fetcher_active_(false),
pending_transfer_ended_(false),
