update_engine: Store and test kern/fw versions as 4 values
- Refactors the handling of _firmware_version and _kernel_version
attributes to split as 4x 16 bit values.
- Based on discussion, we are trying to avoid using the client
specific version format in the network protocol.
- Each of firmware and kernel versions will be stored separately
as uint16_t rather than combined as a uint32_t.
- In the Omaha response the two fields of each version type will
be encoded as key_version.version
- Adds tests for the actual parsing of the response.
BUG=chromium:840432
TEST=cros_run_unit_tests --board=samus --packages update_engine
Change-Id: I4da02e3f4715a132db7e96e55c30139fff6fe6ea
Reviewed-on: https://chromium-review.googlesource.com/1123106
Commit-Ready: Marton Hunyady <[email protected]>
Tested-by: Zentaro Kavanagh <[email protected]>
Reviewed-by: Amin Hassani <[email protected]>
diff --git a/common/utils.cc b/common/utils.cc
index b06954b..1a8fd53 100644
--- a/common/utils.cc
+++ b/common/utils.cc
@@ -61,6 +61,7 @@
using base::Time;
using base::TimeDelta;
using std::min;
+using std::numeric_limits;
using std::pair;
using std::string;
using std::vector;
@@ -1082,6 +1083,36 @@
return value;
}
+void ParseRollbackKeyVersion(const string& raw_version,
+ uint16_t* high_version,
+ uint16_t* low_version) {
+ DCHECK(high_version);
+ DCHECK(low_version);
+ *high_version = numeric_limits<uint16_t>::max();
+ *low_version = numeric_limits<uint16_t>::max();
+
+ vector<string> parts = base::SplitString(
+ raw_version, ".", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
+ if (parts.size() != 2) {
+ // The version string must have exactly one period.
+ return;
+ }
+
+ int high;
+ int low;
+ if (!(base::StringToInt(parts[0], &high) &&
+ base::StringToInt(parts[1], &low))) {
+ // Both parts of the version could not be parsed correctly.
+ return;
+ }
+
+ if (high >= 0 && high < numeric_limits<uint16_t>::max() && low >= 0 &&
+ low < numeric_limits<uint16_t>::max()) {
+ *high_version = static_cast<uint16_t>(high);
+ *low_version = static_cast<uint16_t>(low);
+ }
+}
+
} // namespace utils
} // namespace chromeos_update_engine
diff --git a/common/utils.h b/common/utils.h
index cbc5eb9..ecb97a3 100644
--- a/common/utils.h
+++ b/common/utils.h
@@ -21,6 +21,7 @@
#include <unistd.h>
#include <algorithm>
+#include <limits>
#include <map>
#include <memory>
#include <set>
@@ -322,6 +323,13 @@
// first section of |version| is invalid (e.g. not a number).
int VersionPrefix(const std::string& version);
+// Parses a string in the form high.low, where high and low are 16 bit unsigned
+// integers. If there is more than 1 dot, or if either of the two parts are
+// not valid 16 bit unsigned numbers, then 0xffff is returned for both.
+void ParseRollbackKeyVersion(const std::string& raw_version,
+ uint16_t* high_version,
+ uint16_t* low_version);
+
} // namespace utils
diff --git a/common/utils_unittest.cc b/common/utils_unittest.cc
index 16fa481..1590eeb 100644
--- a/common/utils_unittest.cc
+++ b/common/utils_unittest.cc
@@ -22,6 +22,7 @@
#include <sys/stat.h>
#include <sys/types.h>
+#include <limits>
#include <string>
#include <vector>
@@ -32,6 +33,7 @@
#include "update_engine/common/test_utils.h"
+using std::numeric_limits;
using std::string;
using std::vector;
@@ -450,6 +452,22 @@
*ret = true;
}
+static void ExpectParseRollbackKeyVersion(const string& version,
+ uint16_t expected_high,
+ uint16_t expected_low) {
+ uint16_t actual_high;
+ uint16_t actual_low;
+ utils::ParseRollbackKeyVersion(version, &actual_high, &actual_low);
+ EXPECT_EQ(expected_high, actual_high);
+ EXPECT_EQ(expected_low, actual_low);
+}
+
+static void ExpectInvalidParseRollbackKeyVersion(const string& version) {
+ ExpectParseRollbackKeyVersion(version,
+ numeric_limits<uint16_t>::max(),
+ numeric_limits<uint16_t>::max());
+}
+
TEST(UtilsTest, TestMacros) {
bool void_test = false;
VoidMacroTestHelper(&void_test);
@@ -523,4 +541,29 @@
EXPECT_EQ(-1, utils::VersionPrefix("x.1"));
}
+TEST(UtilsTest, ParseDottedVersion) {
+ // Valid case.
+ ExpectParseRollbackKeyVersion("2.3", 2, 3);
+ ExpectParseRollbackKeyVersion("65535.65535", 65535, 65535);
+
+ // Zero is technically allowed but never actually used.
+ ExpectParseRollbackKeyVersion("0.0", 0, 0);
+
+ // Invalid cases.
+ ExpectInvalidParseRollbackKeyVersion("");
+ ExpectInvalidParseRollbackKeyVersion("2");
+ ExpectInvalidParseRollbackKeyVersion("2.");
+ ExpectInvalidParseRollbackKeyVersion(".2");
+ ExpectInvalidParseRollbackKeyVersion("2.2.");
+ ExpectInvalidParseRollbackKeyVersion("2.2.3");
+ ExpectInvalidParseRollbackKeyVersion(".2.2");
+ ExpectInvalidParseRollbackKeyVersion("a.b");
+ ExpectInvalidParseRollbackKeyVersion("1.b");
+ ExpectInvalidParseRollbackKeyVersion("a.2");
+ ExpectInvalidParseRollbackKeyVersion("65536.65536");
+ ExpectInvalidParseRollbackKeyVersion("99999.99999");
+ ExpectInvalidParseRollbackKeyVersion("99999.1");
+ ExpectInvalidParseRollbackKeyVersion("1.99999");
+}
+
} // namespace chromeos_update_engine
diff --git a/omaha_request_action.cc b/omaha_request_action.cc
index ca6e348..3502048 100644
--- a/omaha_request_action.cc
+++ b/omaha_request_action.cc
@@ -18,6 +18,7 @@
#include <inttypes.h>
+#include <limits>
#include <map>
#include <sstream>
#include <string>
@@ -55,6 +56,7 @@
using base::TimeDelta;
using chromeos_update_manager::kRollforwardInfinity;
using std::map;
+using std::numeric_limits;
using std::string;
using std::vector;
@@ -938,6 +940,20 @@
return true;
}
+// Parses the 2 key version strings kernel_version and firmware_version. If the
+// field is not present, or cannot be parsed the values default to 0xffff.
+void ParseRollbackVersions(OmahaParserData* parser_data,
+ OmahaResponse* output_object) {
+ utils::ParseRollbackKeyVersion(
+ parser_data->updatecheck_attrs[kFirmwareVersion],
+ &output_object->rollback_key_version.firmware_key,
+ &output_object->rollback_key_version.firmware);
+ utils::ParseRollbackKeyVersion(
+ parser_data->updatecheck_attrs[kKernelVersion],
+ &output_object->rollback_key_version.kernel_key,
+ &output_object->rollback_key_version.kernel);
+}
+
} // namespace
bool OmahaRequestAction::ParseResponse(OmahaParserData* parser_data,
@@ -1004,14 +1020,10 @@
// Defaults to false if attribute is not present.
output_object->is_rollback =
ParseBool(parser_data->updatecheck_attrs[kRollback]);
- // Defaults to 0 if attribute is not present. This is fine for these values,
- // because it's the lowest possible value, and the rollback image won't be
- // installed, if the values in the TPM are larger than these, and in case the
- // values in the TPM are 0, all images will be able to boot up.
- output_object->firmware_version = static_cast<uint32_t>(
- ParseInt(parser_data->updatecheck_attrs[kFirmwareVersion]));
- output_object->kernel_version = static_cast<uint32_t>(
- ParseInt(parser_data->updatecheck_attrs[kKernelVersion]));
+
+ // Parses the rollback versions of the current image. If the fields do not
+ // exist they default to 0xffff for the 4 key versions.
+ ParseRollbackVersions(parser_data, output_object);
if (!ParseStatus(parser_data, output_object, completer))
return false;
diff --git a/omaha_request_action_unittest.cc b/omaha_request_action_unittest.cc
index 7c9fe41..21aecb2 100644
--- a/omaha_request_action_unittest.cc
+++ b/omaha_request_action_unittest.cc
@@ -84,6 +84,16 @@
// This is a helper struct to allow unit tests build an update response with the
// values they care about.
struct FakeUpdateResponse {
+ string GetRollbackVersionAttributes() const {
+ return (rollback ? " _rollback=\"true\"" : "") +
+ (!rollback_firmware_version.empty()
+ ? " _firmware_version=\"" + rollback_firmware_version + "\""
+ : "") +
+ (!rollback_kernel_version.empty()
+ ? " _kernel_version=\"" + rollback_kernel_version + "\""
+ : "");
+ }
+
string GetNoUpdateResponse() const {
string entity_str;
if (include_entity)
@@ -121,8 +131,8 @@
"\" cohortname=\"" + cohortname + "\" "
: "") +
" status=\"ok\">"
- "<ping status=\"ok\"/><updatecheck status=\"ok\">"
- "<urls><url codebase=\"" +
+ "<ping status=\"ok\"/><updatecheck status=\"ok\"" +
+ GetRollbackVersionAttributes() + ">" + "<urls><url codebase=\"" +
codebase +
"\"/></urls>"
"<manifest version=\"" +
@@ -220,6 +230,13 @@
bool multi_app_no_update = false;
// Whether to include more than one package in an app.
bool multi_package = false;
+
+ // Whether the payload is a rollback.
+ bool rollback = false;
+ // The verified boot firmware key version for the rollback image.
+ string rollback_firmware_version = "";
+ // The verified boot kernel key version for the rollback image.
+ string rollback_kernel_version = "";
};
} // namespace
@@ -292,7 +309,8 @@
void TestRollbackCheck(bool is_consumer_device,
int rollback_allowed_milestones,
- bool is_policy_loaded);
+ bool is_policy_loaded,
+ OmahaResponse* out_response);
// Runs and checks a ping test. |ping_only| indicates whether it should send
// only a ping or also an updatecheck.
@@ -493,8 +511,8 @@
void OmahaRequestActionTest::TestRollbackCheck(bool is_consumer_device,
int rollback_allowed_milestones,
- bool is_policy_loaded) {
- OmahaResponse response;
+ bool is_policy_loaded,
+ OmahaResponse* out_response) {
fake_update_response_.deadline = "20101020";
ASSERT_TRUE(TestUpdateCheck(fake_update_response_.GetUpdateResponse(),
-1,
@@ -506,9 +524,9 @@
metrics::CheckResult::kUpdateAvailable,
metrics::CheckReaction::kUpdating,
metrics::DownloadErrorCode::kUnset,
- &response,
+ out_response,
nullptr));
- ASSERT_TRUE(response.update_exists);
+ ASSERT_TRUE(out_response->update_exists);
}
// Tests Event requests -- they should always succeed. |out_post_data|
@@ -2822,9 +2840,11 @@
ReportKeyVersionMetrics(min_kernel_version, min_kernel_version, true))
.Times(1);
+ OmahaResponse response;
TestRollbackCheck(false /* is_consumer_device */,
3 /* rollback_allowed_milestones */,
- false /* is_policy_loaded */);
+ false /* is_policy_loaded */,
+ &response);
// Verify kernel_max_rollforward was set to the current minimum
// kernel key version. This has the effect of freezing roll
@@ -2854,9 +2874,11 @@
ReportKeyVersionMetrics(min_kernel_version, kRollforwardInfinity, true))
.Times(1);
+ OmahaResponse response;
TestRollbackCheck(true /* is_consumer_device */,
3 /* rollback_allowed_milestones */,
- false /* is_policy_loaded */);
+ false /* is_policy_loaded */,
+ &response);
// Verify that with rollback disabled that kernel_max_rollforward
// was set to logical infinity. This is the expected behavior for
@@ -2885,9 +2907,11 @@
ReportKeyVersionMetrics(min_kernel_version, min_kernel_version, true))
.Times(1);
+ OmahaResponse response;
TestRollbackCheck(false /* is_consumer_device */,
allowed_milestones,
- true /* is_policy_loaded */);
+ true /* is_policy_loaded */,
+ &response);
// Verify that with rollback enabled that kernel_max_rollforward
// was set to the current minimum kernel key version. This has
@@ -2917,9 +2941,11 @@
ReportKeyVersionMetrics(min_kernel_version, kRollforwardInfinity, true))
.Times(1);
+ OmahaResponse response;
TestRollbackCheck(false /* is_consumer_device */,
allowed_milestones,
- true /* is_policy_loaded */);
+ true /* is_policy_loaded */,
+ &response);
// Verify that with rollback disabled that kernel_max_rollforward
// was set to logical infinity.
@@ -2927,4 +2953,30 @@
EXPECT_EQ(kRollforwardInfinity, fake_hw->GetMaxKernelKeyRollforward());
}
+TEST_F(OmahaRequestActionTest, RollbackResponseParsedNoEntries) {
+ OmahaResponse response;
+ fake_update_response_.rollback = true;
+ TestRollbackCheck(false /* is_consumer_device */,
+ 4 /* rollback_allowed_milestones */,
+ true /* is_policy_loaded */,
+ &response);
+ EXPECT_TRUE(response.is_rollback);
+}
+
+TEST_F(OmahaRequestActionTest, RollbackResponseValidVersionsParsed) {
+ OmahaResponse response;
+ fake_update_response_.rollback_firmware_version = "1.2";
+ fake_update_response_.rollback_kernel_version = "3.4";
+ fake_update_response_.rollback = true;
+ TestRollbackCheck(false /* is_consumer_device */,
+ 4 /* rollback_allowed_milestones */,
+ true /* is_policy_loaded */,
+ &response);
+ EXPECT_TRUE(response.is_rollback);
+ EXPECT_EQ(1, response.rollback_key_version.firmware_key);
+ EXPECT_EQ(2, response.rollback_key_version.firmware);
+ EXPECT_EQ(3, response.rollback_key_version.kernel_key);
+ EXPECT_EQ(4, response.rollback_key_version.kernel);
+}
+
} // namespace chromeos_update_engine
diff --git a/omaha_response.h b/omaha_response.h
index ef69de2..0ac09df 100644
--- a/omaha_response.h
+++ b/omaha_response.h
@@ -21,6 +21,7 @@
#include <sys/stat.h>
#include <sys/types.h>
+#include <limits>
#include <string>
#include <vector>
@@ -86,12 +87,21 @@
// True if the returned image is a rollback for the device.
bool is_rollback = false;
- // Kernel version of the returned rollback image. 0 if the image is not a
- // rollback.
- uint32_t kernel_version = 0;
- // Firmware version of the returned rollback image. 0 if the image is not a
- // rollback.
- uint32_t firmware_version = 0;
+
+ struct RollbackKeyVersion {
+ // Kernel key version. 0xffff if the value is unknown.
+ uint16_t kernel_key = std::numeric_limits<uint16_t>::max();
+ // Kernel version. 0xffff if the value is unknown.
+ uint16_t kernel = std::numeric_limits<uint16_t>::max();
+ // Firmware key verison. 0xffff if the value is unknown.
+ uint16_t firmware_key = std::numeric_limits<uint16_t>::max();
+ // Firmware version. 0xffff if the value is unknown.
+ uint16_t firmware = std::numeric_limits<uint16_t>::max();
+ };
+
+ // Key versions of the returned rollback image. Values are 0xffff if the
+ // image not a rollback, or the fields were not present.
+ RollbackKeyVersion rollback_key_version;
};
static_assert(sizeof(off_t) == 8, "off_t not 64 bit");
diff --git a/omaha_response_handler_action.cc b/omaha_response_handler_action.cc
index 775c0a8..3a8439d 100644
--- a/omaha_response_handler_action.cc
+++ b/omaha_response_handler_action.cc
@@ -16,6 +16,7 @@
#include "update_engine/omaha_response_handler_action.h"
+#include <limits>
#include <string>
#include <base/logging.h>
@@ -36,6 +37,7 @@
using chromeos_update_manager::Policy;
using chromeos_update_manager::UpdateManager;
+using std::numeric_limits;
using std::string;
namespace chromeos_update_engine {
@@ -149,8 +151,20 @@
system_state_->hardware()->GetMinKernelKeyVersion());
auto min_firmware_key_version = static_cast<uint32_t>(
system_state_->hardware()->GetMinFirmwareKeyVersion());
- if (response.kernel_version < min_kernel_key_version ||
- response.firmware_version < min_firmware_key_version) {
+ uint32_t kernel_key_version =
+ static_cast<uint32_t>(response.rollback_key_version.kernel_key) << 16 |
+ static_cast<uint32_t>(response.rollback_key_version.kernel);
+ uint32_t firmware_key_version =
+ static_cast<uint32_t>(response.rollback_key_version.firmware_key)
+ << 16 |
+ static_cast<uint32_t>(response.rollback_key_version.firmware);
+
+ // Don't attempt a rollback if the versions are incompatible or the
+ // target image does not specify the version information.
+ if (kernel_key_version == numeric_limits<uint32_t>::max() ||
+ firmware_key_version == numeric_limits<uint32_t>::max() ||
+ kernel_key_version < min_kernel_key_version ||
+ firmware_key_version < min_firmware_key_version) {
LOG(ERROR) << "Device won't be able to boot up the rollback image.";
completer.set_code(ErrorCode::kRollbackNotPossible);
return;
diff --git a/omaha_response_handler_action_unittest.cc b/omaha_response_handler_action_unittest.cc
index 7437f50..c598c89 100644
--- a/omaha_response_handler_action_unittest.cc
+++ b/omaha_response_handler_action_unittest.cc
@@ -502,8 +502,10 @@
.size = 1,
.hash = kPayloadHashHex});
in.is_rollback = true;
- in.kernel_version = 0x00010002;
- in.firmware_version = 0x00030004;
+ in.rollback_key_version.kernel = 1;
+ in.rollback_key_version.kernel = 2;
+ in.rollback_key_version.firmware_key = 3;
+ in.rollback_key_version.firmware = 4;
fake_system_state_.fake_hardware()->SetMinKernelKeyVersion(0x00010002);
fake_system_state_.fake_hardware()->SetMinFirmwareKeyVersion(0x00030004);
@@ -524,8 +526,10 @@
.size = 1,
.hash = kPayloadHashHex});
in.is_rollback = true;
- in.kernel_version = 0x00010001; // This is lower than the minimum.
- in.firmware_version = 0x00030004;
+ in.rollback_key_version.kernel_key = 1;
+ in.rollback_key_version.kernel = 1; // This is lower than the minimum.
+ in.rollback_key_version.firmware_key = 3;
+ in.rollback_key_version.firmware = 4;
fake_system_state_.fake_hardware()->SetMinKernelKeyVersion(0x00010002);
fake_system_state_.fake_hardware()->SetMinFirmwareKeyVersion(0x00030004);
@@ -545,8 +549,10 @@
.size = 1,
.hash = kPayloadHashHex});
in.is_rollback = true;
- in.kernel_version = 0x00010002;
- in.firmware_version = 0x00030003; // This is lower than the minimum.
+ in.rollback_key_version.kernel_key = 1;
+ in.rollback_key_version.kernel = 2;
+ in.rollback_key_version.firmware_key = 3;
+ in.rollback_key_version.firmware = 3; // This is lower than the minimum.
fake_system_state_.fake_hardware()->SetMinKernelKeyVersion(0x00010002);
fake_system_state_.fake_hardware()->SetMinFirmwareKeyVersion(0x00030004);
