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