update_engine: Fix enrollment recovery mode logic
Retrieve EnrollmentRecoveryRequired from Local state and ignore
non-critical update if enrollment recovery mode is set to true.
BUG=chromium:1355305
TEST=Run all the update_engines tests
Change-Id: I2e3f59087662f94fa0afa825e5513dde3352c9ac
Reviewed-on: https://chromium-review.googlesource.com/c/aosp/platform/system/update_engine/+/3940187
Reviewed-by: Roman Sorokin <rsorokin@google.com>
Reviewed-by: Jae Hoon Kim <kimjae@chromium.org>
Commit-Queue: Brahim Chikhaoui <bchikhaoui@google.com>
Reviewed-by: Miriam Polzer <mpolzer@google.com>
Tested-by: Brahim Chikhaoui <bchikhaoui@google.com>
diff --git a/common/error_code.h b/common/error_code.h
index 26bd1a9..e9e1156 100644
--- a/common/error_code.h
+++ b/common/error_code.h
@@ -91,6 +91,7 @@
kInvalidateLastUpdate = 65,
kOmahaUpdateIgnoredOverMetered = 66,
kScaledInstallationError = 67,
+ kNonCriticalUpdateEnrollmentRecovery = 68,
// VERY IMPORTANT! When adding new error codes:
//
diff --git a/common/error_code_utils.cc b/common/error_code_utils.cc
index 8af51c2..39f495c 100644
--- a/common/error_code_utils.cc
+++ b/common/error_code_utils.cc
@@ -183,6 +183,8 @@
return "ErrorCode::kOmahaUpdateIgnoredOverMetered";
case ErrorCode::kScaledInstallationError:
return "ErrorCode::kScaledInstallationError";
+ case ErrorCode::kNonCriticalUpdateEnrollmentRecovery:
+ return "ErrorCode::kNonCriticalUpdateEnrollmentRecovery";
// Don't add a default case to let the compiler warn about newly added
// error codes which should be added here.
}
diff --git a/common/fake_hardware.h b/common/fake_hardware.h
index e4abb01..f883217 100644
--- a/common/fake_hardware.h
+++ b/common/fake_hardware.h
@@ -18,9 +18,11 @@
#define UPDATE_ENGINE_COMMON_FAKE_HARDWARE_H_
#include <map>
+#include <memory>
#include <string>
#include <utility>
+#include <base/json/json_string_value_serializer.h>
#include <base/time/time.h>
#include "update_engine/common/error_code.h"
@@ -104,6 +106,20 @@
return true;
}
+ bool IsEnrollmentRecoveryModeEnabled(
+ const base::Value* local_state) const override {
+ return is_enrollment_recovery_enabled_;
+ }
+
+ std::unique_ptr<base::Value> ReadLocalState() const override {
+ int error_code;
+ std::string error_msg;
+
+ JSONStringValueDeserializer deserializer(local_state_contents_);
+
+ return deserializer.Deserialize(&error_code, &error_msg);
+ }
+
bool SetMaxKernelKeyRollforward(int kernel_max_rollforward) override {
kernel_max_rollforward_ = kernel_max_rollforward;
return true;
@@ -180,6 +196,14 @@
void UnsetIsOOBEComplete() { is_oobe_complete_ = false; }
+ void SetIsEnrollmentRecoveryMode(bool enrollment_recovery) {
+ is_enrollment_recovery_enabled_ = enrollment_recovery;
+ }
+
+ void SetLocalState(std::string local_state) {
+ local_state_contents_ = local_state;
+ }
+
void SetHardwareClass(const std::string& hardware_class) {
hardware_class_ = hardware_class;
}
@@ -233,6 +257,8 @@
bool are_dev_features_enabled_{false};
bool is_oobe_enabled_{true};
bool is_oobe_complete_{true};
+ bool is_enrollment_recovery_enabled_{false};
+ std::string local_state_contents_;
// Jan 20, 2007
base::Time oobe_timestamp_{base::Time::FromTimeT(1169280000)};
std::string hardware_class_{"Fake HWID BLAH-1234"};
diff --git a/common/hardware_interface.h b/common/hardware_interface.h
index 645f929..89c9b01 100644
--- a/common/hardware_interface.h
+++ b/common/hardware_interface.h
@@ -19,11 +19,13 @@
#include <stdint.h>
+#include <memory>
#include <string>
#include <vector>
#include <base/files/file_path.h>
#include <base/time/time.h>
+#include <base/values.h>
#include "update_engine/common/error_code.h"
@@ -71,6 +73,14 @@
// not have a requisition, or if not running Chrome OS.
virtual std::string GetDeviceRequisition() const = 0;
+ // Returns the Local State root as base::Value
+ virtual std::unique_ptr<base::Value> ReadLocalState() const = 0;
+
+ // Returns true if enrollment recovery mode is set on
+ // given Local State
+ virtual bool IsEnrollmentRecoveryModeEnabled(
+ const base::Value* local_state) const = 0;
+
// Returns the minimum kernel key version that verified boot on Chrome OS
// will allow to boot. This is the value of crossystem tpm_kernver. Returns
// -1 on error, or if not running on Chrome OS.
diff --git a/cros/hardware_chromeos.cc b/cros/hardware_chromeos.cc
index 4e74793..3e52808 100644
--- a/cros/hardware_chromeos.cc
+++ b/cros/hardware_chromeos.cc
@@ -20,6 +20,7 @@
#include <base/files/file_path.h>
#include <base/files/file_util.h>
+#include <base/json/json_file_value_serializer.h>
#include <base/logging.h>
#include <base/strings/string_number_conversions.h>
#include <base/strings/string_util.h>
@@ -91,6 +92,9 @@
const char kRunningFromMiniOSLabel[] = "cros_minios";
+constexpr char kLocalStatePath[] = "/home/chronos/Local State";
+
+constexpr char kEnrollmentRecoveryRequired[] = "EnrollmentRecoveryRequired";
} // namespace
namespace chromeos_update_engine {
@@ -209,8 +213,7 @@
string HardwareChromeOS::GetDeviceRequisition() const {
#if USE_CFM || USE_REPORT_REQUISITION
- const char* kLocalStatePath = "/home/chronos/Local State";
- return ReadDeviceRequisition(base::FilePath(kLocalStatePath));
+ return ReadDeviceRequisition(ReadLocalState().get());
#else
return "";
#endif
@@ -379,6 +382,46 @@
return true;
}
+std::unique_ptr<base::Value> HardwareChromeOS::ReadLocalState() const {
+ base::FilePath local_state_file = base::FilePath(kLocalStatePath);
+
+ JSONFileValueDeserializer deserializer(local_state_file);
+
+ int error_code;
+ std::string error_msg;
+ std::unique_ptr<base::Value> root =
+ deserializer.Deserialize(&error_code, &error_msg);
+
+ if (!root) {
+ if (error_code != 0) {
+ LOG(ERROR) << "Unable to deserialize Local State with exit code: "
+ << error_code << " and error: " << error_msg;
+ }
+ return nullptr;
+ }
+
+ return root;
+}
+
+// Check for given given Local State the value of the enrollment
+// recovery mode. Returns true if Recoverymode is set on CrOS.
+bool HardwareChromeOS::IsEnrollmentRecoveryModeEnabled(
+ const base::Value* local_state) const {
+ if (!local_state) {
+ return false;
+ }
+
+ auto* path = local_state->FindPath(kEnrollmentRecoveryRequired);
+
+ if (!path || !path->is_bool()) {
+ LOG(INFO) << "EnrollmentRecoveryRequired path does not exist in"
+ << "Local State or is incorrectly formatted.";
+ return false;
+ }
+
+ return path->GetBool();
+}
+
int HardwareChromeOS::GetActiveMiniOsPartition() const {
char value_buffer[VB_MAX_STRING_PROPERTY];
const char* rv = VbGetSystemPropertyString(
diff --git a/cros/hardware_chromeos.h b/cros/hardware_chromeos.h
index fe7b77d..089fe0d 100644
--- a/cros/hardware_chromeos.h
+++ b/cros/hardware_chromeos.h
@@ -50,6 +50,9 @@
bool IsOOBEComplete(base::Time* out_time_of_oobe) const override;
std::string GetHardwareClass() const override;
std::string GetDeviceRequisition() const override;
+ std::unique_ptr<base::Value> ReadLocalState() const override;
+ bool IsEnrollmentRecoveryModeEnabled(
+ const base::Value* local_state) const override;
int GetMinKernelKeyVersion() const override;
int GetMinFirmwareKeyVersion() const override;
int GetMaxFirmwareKeyRollforward() const override;
diff --git a/cros/hardware_chromeos_unittest.cc b/cros/hardware_chromeos_unittest.cc
index 872e7e5..83425bd 100644
--- a/cros/hardware_chromeos_unittest.cc
+++ b/cros/hardware_chromeos_unittest.cc
@@ -20,6 +20,7 @@
#include <base/files/file_util.h>
#include <base/files/scoped_temp_dir.h>
+#include <base/json/json_string_value_serializer.h>
#include <brillo/file_utils.h>
#include <gtest/gtest.h>
@@ -31,6 +32,30 @@
using chromeos_update_engine::test_utils::WriteFileString;
using std::string;
+namespace {
+
+constexpr char kEnrollmentReCoveryTrueJSON[] = R"({
+ "the_list": [ "val1", "val2" ],
+ "EnrollmentRecoveryRequired": true,
+ "some_String": "1337",
+ "some_int": 42
+})";
+
+constexpr char kEnrollmentReCoveryFalseJSON[] = R"({
+ "the_list": [ "val1", "val2" ],
+ "EnrollmentRecoveryRequired": false,
+ "some_String": "1337",
+ "some_int": 42
+})";
+
+constexpr char kNoEnrollmentRecoveryJSON[] = R"({
+ "the_list": [ "val1", "val2" ],
+ "some_String": "1337",
+ "some_int": 42
+})";
+
+} // namespace
+
namespace chromeos_update_engine {
class HardwareChromeOSTest : public ::testing::Test {
@@ -56,10 +81,43 @@
hardware_.LoadConfig(root_dir_.GetPath().value(), normal_mode);
}
+ std::unique_ptr<base::Value> JSONToUniquePtrValue(const string& json_string) {
+ int error_code;
+ std::string error_msg;
+
+ JSONStringValueDeserializer deserializer(json_string);
+
+ return deserializer.Deserialize(&error_code, &error_msg);
+ }
+
HardwareChromeOS hardware_;
base::ScopedTempDir root_dir_;
};
+TEST_F(HardwareChromeOSTest, NoLocalFile) {
+ std::unique_ptr<base::Value> root = nullptr;
+
+ EXPECT_FALSE(hardware_.IsEnrollmentRecoveryModeEnabled(root.get()));
+}
+
+TEST_F(HardwareChromeOSTest, LocalFileWithEnrollmentRecoveryTrue) {
+ std::unique_ptr<base::Value> root =
+ JSONToUniquePtrValue(kEnrollmentReCoveryTrueJSON);
+ EXPECT_TRUE(hardware_.IsEnrollmentRecoveryModeEnabled(root.get()));
+}
+
+TEST_F(HardwareChromeOSTest, LocalFileWithEnrollmentRecoveryFalse) {
+ std::unique_ptr<base::Value> root =
+ JSONToUniquePtrValue(kEnrollmentReCoveryFalseJSON);
+ EXPECT_FALSE(hardware_.IsEnrollmentRecoveryModeEnabled(root.get()));
+}
+
+TEST_F(HardwareChromeOSTest, LocalFileWithNoEnrollmentRecoveryPath) {
+ std::unique_ptr<base::Value> root =
+ JSONToUniquePtrValue(kNoEnrollmentRecoveryJSON);
+ EXPECT_FALSE(hardware_.IsEnrollmentRecoveryModeEnabled(root.get()));
+}
+
TEST_F(HardwareChromeOSTest, NoFileFoundReturnsDefault) {
CallLoadConfig(true /* normal_mode */);
EXPECT_TRUE(hardware_.IsOOBEEnabled());
diff --git a/cros/omaha_request_action.cc b/cros/omaha_request_action.cc
index 388eda8..6708e5a 100644
--- a/cros/omaha_request_action.cc
+++ b/cros/omaha_request_action.cc
@@ -63,6 +63,9 @@
namespace chromeos_update_engine {
namespace {
+
+constexpr char kCriticalAppVersion[] = "ForcedUpdate";
+
// Parses |str| and returns |true| iff its value is "true".
bool ParseBool(const string& str) {
return str == "true";
@@ -1240,8 +1243,9 @@
}
bool OmahaRequestAction::ShouldIgnoreUpdate(ErrorCode* error) const {
+ const auto* hardware = SystemState::Get()->hardware();
// Never ignore valid update when running from MiniOS.
- if (SystemState::Get()->hardware()->IsRunningFromMiniOs())
+ if (hardware->IsRunningFromMiniOs())
return false;
// Note: policy decision to not update to a version we rolled back from.
@@ -1261,16 +1265,25 @@
return true;
}
- if (SystemState::Get()->hardware()->IsOOBEEnabled() &&
- !SystemState::Get()->hardware()->IsOOBEComplete(nullptr) &&
+ if (hardware->IsOOBEEnabled() && !hardware->IsOOBEComplete(nullptr) &&
(response_.deadline.empty() ||
SystemState::Get()->payload_state()->GetRollbackHappened()) &&
- params->app_version() != "ForcedUpdate") {
+ params->app_version() != kCriticalAppVersion) {
LOG(INFO) << "Ignoring a non-critical Omaha update before OOBE completion.";
*error = ErrorCode::kNonCriticalUpdateInOOBE;
return true;
}
+ if (hardware->IsEnrollmentRecoveryModeEnabled(
+ hardware->ReadLocalState().get()) &&
+ response_.deadline.empty() &&
+ params->app_version() != kCriticalAppVersion) {
+ LOG(INFO) << "Ignoring non-critical Omaha update as enrollment "
+ << "recovery mode is enabled.";
+ *error = ErrorCode::kNonCriticalUpdateEnrollmentRecovery;
+ return true;
+ }
+
if (params->is_install()) {
LOG(INFO) << "Skipping connection type check for DLC installations.";
} else if (!IsUpdateAllowedOverCurrentConnection(error)) {
diff --git a/cros/omaha_request_action_unittest.cc b/cros/omaha_request_action_unittest.cc
index e4b8588..0e6b49d 100644
--- a/cros/omaha_request_action_unittest.cc
+++ b/cros/omaha_request_action_unittest.cc
@@ -1128,6 +1128,29 @@
}
// Verify that non-critical updates are skipped by reporting the
+// kNonCriticalUpdateEnrollmentRecovery error code.
+TEST_F(OmahaRequestActionTest, SkipNonCriticalUpdatesEnrollmentRecoveryMode) {
+ FakeSystemState::Get()->fake_hardware()->SetIsEnrollmentRecoveryMode(true);
+ tuc_params_.http_response = fake_update_response_.GetUpdateResponse();
+ tuc_params_.expected_code = ErrorCode::kNonCriticalUpdateEnrollmentRecovery;
+ tuc_params_.expected_check_result = metrics::CheckResult::kParsingError;
+ tuc_params_.expected_check_reaction = metrics::CheckReaction::kUnset;
+
+ ASSERT_FALSE(TestUpdateCheck());
+
+ EXPECT_FALSE(response_.update_exists);
+}
+
+TEST_F(OmahaRequestActionTest, ValidUpdateForNonEnrollmentRecoveryMode) {
+ FakeSystemState::Get()->fake_hardware()->SetIsEnrollmentRecoveryMode(false);
+ tuc_params_.http_response = fake_update_response_.GetUpdateResponse();
+
+ ASSERT_TRUE(TestUpdateCheck());
+
+ EXPECT_TRUE(response_.update_exists);
+}
+
+// Verify that non-critical updates are skipped by reporting the
// kNonCriticalUpdateInOOBE error code when attempted over cellular network -
// i.e. when the update would need user permission. Note that reporting
// kOmahaUpdateIgnoredOverCellular error in this case might cause undesired UX
diff --git a/cros/payload_state.cc b/cros/payload_state.cc
index 007bef8..1937fb9 100644
--- a/cros/payload_state.cc
+++ b/cros/payload_state.cc
@@ -380,6 +380,7 @@
case ErrorCode::kInvalidateLastUpdate:
case ErrorCode::kOmahaUpdateIgnoredOverMetered:
case ErrorCode::kScaledInstallationError:
+ case ErrorCode::kNonCriticalUpdateEnrollmentRecovery:
LOG(INFO) << "Not incrementing URL index or failure count for this error";
break;
diff --git a/cros/requisition_util.cc b/cros/requisition_util.cc
index 9ac5740..12f2654 100644
--- a/cros/requisition_util.cc
+++ b/cros/requisition_util.cc
@@ -39,7 +39,7 @@
namespace chromeos_update_engine {
-string ReadDeviceRequisition(const base::FilePath& local_state) {
+string ReadDeviceRequisition(const base::Value* local_state) {
string requisition;
bool vpd_retval = utils::GetVpdValue(kOemRequisitionKey, &requisition);
@@ -49,20 +49,8 @@
// OR
// 2. Requisition value mistakenly set to "none".
if ((requisition.empty() || requisition == kNoRequisition || !vpd_retval) &&
- base::PathExists(local_state)) {
- int error_code;
- std::string error_msg;
- JSONFileValueDeserializer deserializer(local_state);
- std::unique_ptr<base::Value> root =
- deserializer.Deserialize(&error_code, &error_msg);
- if (!root) {
- if (error_code != 0) {
- LOG(ERROR) << "Unable to deserialize Local State with exit code: "
- << error_code << " and error: " << error_msg;
- }
- return "";
- }
- auto* path = root->FindPath({"enrollment", "device_requisition"});
+ local_state) {
+ auto* path = local_state->FindPath({"enrollment", "device_requisition"});
if (!path || !path->is_string()) {
return "";
}
diff --git a/cros/requisition_util.h b/cros/requisition_util.h
index 6ec4783..4203157 100644
--- a/cros/requisition_util.h
+++ b/cros/requisition_util.h
@@ -17,15 +17,17 @@
#ifndef UPDATE_ENGINE_CROS_REQUISITION_UTIL_H_
#define UPDATE_ENGINE_CROS_REQUISITION_UTIL_H_
+#include <memory>
#include <string>
#include <base/files/file_path.h>
+#include <base/values.h>
namespace chromeos_update_engine {
// Checks the VPD and Local State for the device's requisition and returns it,
// or an empty string if the device has no requisition.
-std::string ReadDeviceRequisition(const base::FilePath& local_state);
+std::string ReadDeviceRequisition(const base::Value* local_state);
} // namespace chromeos_update_engine
diff --git a/cros/requisition_util_unittest.cc b/cros/requisition_util_unittest.cc
index 269585e..c22fb8e 100644
--- a/cros/requisition_util_unittest.cc
+++ b/cros/requisition_util_unittest.cc
@@ -17,10 +17,12 @@
#include "update_engine/cros/requisition_util.h"
#include <string>
+#include <utility>
#include <base/files/file_path.h>
#include <base/files/file_util.h>
#include <base/files/scoped_temp_dir.h>
+#include <base/json/json_string_value_serializer.h>
#include <gtest/gtest.h>
#include "update_engine/common/test_utils.h"
@@ -59,36 +61,34 @@
class RequisitionUtilTest : public ::testing::Test {
protected:
- void SetUp() override { ASSERT_TRUE(root_dir_.CreateUniqueTempDir()); }
+ std::unique_ptr<base::Value> JsonToUniquePtrValue(const string& json_string) {
+ int error_code;
+ std::string error_msg;
- void WriteJsonToFile(const string& json) {
- path_ =
- base::FilePath(root_dir_.GetPath().value() + "/chronos/Local State");
- ASSERT_TRUE(base::CreateDirectory(path_.DirName()));
- ASSERT_TRUE(WriteFileString(path_.value(), json));
+ JSONStringValueDeserializer deserializer(json_string);
+
+ return deserializer.Deserialize(&error_code, &error_msg);
}
-
- base::ScopedTempDir root_dir_;
- base::FilePath path_;
};
TEST_F(RequisitionUtilTest, BadJsonReturnsEmpty) {
- WriteJsonToFile("this isn't JSON");
- EXPECT_EQ("", ReadDeviceRequisition(path_));
+ std::unique_ptr<base::Value> root = JsonToUniquePtrValue("this isn't JSON");
+ EXPECT_EQ("", ReadDeviceRequisition(root.get()));
}
TEST_F(RequisitionUtilTest, NoFileReturnsEmpty) {
- EXPECT_EQ("", ReadDeviceRequisition(path_));
+ std::unique_ptr<base::Value> root = nullptr;
+ EXPECT_EQ("", ReadDeviceRequisition(root.get()));
}
TEST_F(RequisitionUtilTest, EnrollmentRequisition) {
- WriteJsonToFile(kRemoraJSON);
- EXPECT_EQ("remora", ReadDeviceRequisition(path_));
+ std::unique_ptr<base::Value> root = JsonToUniquePtrValue(kRemoraJSON);
+ EXPECT_EQ("remora", ReadDeviceRequisition(root.get()));
}
TEST_F(RequisitionUtilTest, BlankEnrollment) {
- WriteJsonToFile(kNoEnrollmentJSON);
- EXPECT_EQ("", ReadDeviceRequisition(path_));
+ std::unique_ptr<base::Value> root = JsonToUniquePtrValue(kNoEnrollmentJSON);
+ EXPECT_EQ("", ReadDeviceRequisition(root.get()));
}
} // namespace chromeos_update_engine
diff --git a/metrics_utils.cc b/metrics_utils.cc
index 7b21afa..6196d98 100644
--- a/metrics_utils.cc
+++ b/metrics_utils.cc
@@ -129,6 +129,7 @@
case ErrorCode::kOmahaUpdateDeferredPerPolicy:
case ErrorCode::kNonCriticalUpdateInOOBE:
case ErrorCode::kDownloadCancelledPerPolicy:
+ case ErrorCode::kNonCriticalUpdateEnrollmentRecovery:
case ErrorCode::kRepeatedFpFromOmahaError:
return metrics::AttemptResult::kUpdateSkipped;
@@ -252,6 +253,7 @@
case ErrorCode::kInvalidateLastUpdate:
case ErrorCode::kOmahaUpdateIgnoredOverMetered:
case ErrorCode::kScaledInstallationError:
+ case ErrorCode::kNonCriticalUpdateEnrollmentRecovery:
break;
// Special flags. These can't happen (we mask them out above) but
diff --git a/update_manager/update_can_start_policy.cc b/update_manager/update_can_start_policy.cc
index 1fd62b4..164d1d6 100644
--- a/update_manager/update_can_start_policy.cc
+++ b/update_manager/update_can_start_policy.cc
@@ -146,6 +146,7 @@
case ErrorCode::kInvalidateLastUpdate:
case ErrorCode::kOmahaUpdateIgnoredOverMetered:
case ErrorCode::kScaledInstallationError:
+ case ErrorCode::kNonCriticalUpdateEnrollmentRecovery:
LOG(INFO) << "Not changing URL index or failure count due to error "
<< chromeos_update_engine::utils::ErrorCodeToString(err_code)
<< " (" << static_cast<int>(err_code) << ")";
