Skip to content

Commit

Permalink
Revert "variations: Add SeedStore override for CrOS."
Browse files Browse the repository at this point in the history
This reverts commit ba0105e.

Reason for revert: test `EarlyBootSeedStoreTest.LoadSafeSeed_Unspecified` added by this CL is failing on https://ci.chromium.org/ui/p/chrome/builders/ci/linux-chromeos-chrome

First failed build https://ci.chromium.org/ui/p/chrome/builders/ci/linux-chromeos-chrome/35356/overview

Original change's description:
> variations: Add SeedStore override for CrOS.
>
> CrOS early boot needs to have its own notion of what the "safe seed" is
> (and when to use it), so override the VariationsSeedStore class's
> LoadSafeSeed method to load a distinct seed from the one in local state.
>
> BUG=b:263975722
> TEST=included unit tests
>
> Change-Id: I87c5135bb6438056f5018c722c888c430da8c35d
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4685411
> Commit-Queue: Miriam Zimmerman <mutexlox@chromium.org>
> Reviewed-by: Ian Barkley-Yeung <iby@chromium.org>
> Reviewed-by: Ilya Sherman <isherman@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1187997}

BUG=b:263975722
No-Presubmit: true
No-Tree-Checks: true
No-Try: true

Change-Id: If31ab38b4955881eaa791abb74b6a7cd031f2c34
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4812159
Owners-Override: Chan Li <chanli@chromium.org>
Commit-Queue: Chan Li <chanli@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Chan Li <chanli@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1188113}
  • Loading branch information
Chan Li authored and Chromium LUCI CQ committed Aug 25, 2023
1 parent 42bc651 commit e60c2c5
Show file tree
Hide file tree
Showing 8 changed files with 22 additions and 312 deletions.
4 changes: 0 additions & 4 deletions components/variations/cros_evaluate_seed/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@ source_set("evaluate_seed_lib") {
"cros_safe_seed_manager.h",
"cros_variations_field_trial_creator.cc",
"cros_variations_field_trial_creator.h",
"early_boot_seed_store.cc",
"early_boot_seed_store.h",
"evaluate_seed.cc",
"evaluate_seed.h",
]
Expand All @@ -43,7 +41,6 @@ source_set("unit_tests") {
testonly = true
sources = [
"cros_safe_seed_manager_unittest.cc",
"early_boot_seed_store_unittest.cc",
"evaluate_seed_unittest.cc",
]
deps = [
Expand All @@ -53,7 +50,6 @@ source_set("unit_tests") {
"//build:branding_buildflags",
"//build/config/chromebox_for_meetings:buildflags",
"//chromeos/ash/components/dbus/featured:proto",
"//components/prefs:test_support",
"//components/test:test_support",
"//components/variations",
"//components/variations/service:service",
Expand Down
19 changes: 6 additions & 13 deletions components/variations/cros_evaluate_seed/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,23 +12,16 @@ each early-boot experiment should be in, as well as any parameters for the
experiment. It lives here so that it is trivial to keep the code in sync between
ChromeOS's platform layer and chrome.

It will be built alongside ash.
It will be built alongside ash, and use the same seed it uses in `Local State`
in normal operation.

It will be executed primarily by `featured`, which lives in
`//platform2/featured/`.

In safe seed cases, the **platform** code (featured) will determine whether to
use the safe (or null) seed, and pass that information along with the value of
the safe seed to use along to this code.

`evaluate_seed` will write a serialized version of the computed state to stdout,
along with a representation of the seed used for computation (for purposes of
determining whether the seed can be marked as "safe").

## Seed usage

When the device starts up, `featured` will exec `evaluate_seed`, which will by
default use whatever the latest seed in `/home/chronos/Local State` is. The
state computed by `evaluate_seed` will be cached in a tmpfs until the next
reboot, so even if ash later downloads and applies different seeds,
`evaluate_seed` will not re-evaluate the seed until the next device reboot.

For disaster recovery, featured will determine whether to use the safe (or null)
seed, and pass that information along with the value of the safe seed to use
along to `evaluate_seed`.
45 changes: 0 additions & 45 deletions components/variations/cros_evaluate_seed/early_boot_seed_store.cc

This file was deleted.

51 changes: 0 additions & 51 deletions components/variations/cros_evaluate_seed/early_boot_seed_store.h

This file was deleted.

This file was deleted.

53 changes: 15 additions & 38 deletions components/variations/variations_seed_store.cc
Original file line number Diff line number Diff line change
Expand Up @@ -485,67 +485,44 @@ void VariationsSeedStore::ImportInitialSeed(
}
#endif // BUILDFLAG(IS_ANDROID) || BUILDFLAG(IS_IOS)

LoadSeedResult VariationsSeedStore::VerifyAndParseSeed(
VariationsSeed* seed,
const std::string& seed_data,
const std::string& base64_seed_signature,
absl::optional<VerifySignatureResult>* verify_signature_result) {
// TODO(crbug/1335082): get rid of |signature_verification_enabled_| and only
// support switches::kAcceptEmptySeedSignatureForTesting.
if (signature_verification_enabled_ &&
!AcceptEmptySeedSignatureForTesting(base64_seed_signature)) {
*verify_signature_result =
VerifySeedSignature(seed_data, base64_seed_signature);
if (*verify_signature_result != VerifySignatureResult::VALID_SIGNATURE) {
return LoadSeedResult::kInvalidSignature;
}
}

if (!seed->ParseFromString(seed_data)) {
return LoadSeedResult::kCorruptProtobuf;
}

return LoadSeedResult::kSuccess;
}

LoadSeedResult VariationsSeedStore::LoadSeedImpl(
SeedType seed_type,
VariationsSeed* seed,
std::string* seed_data,
std::string* base64_seed_signature) {
LoadSeedResult read_result = ReadSeedData(seed_type, seed_data);
if (read_result != LoadSeedResult::kSuccess) {
if (read_result != LoadSeedResult::kSuccess)
return read_result;
}

*base64_seed_signature = local_state_->GetString(
seed_type == SeedType::LATEST ? prefs::kVariationsSeedSignature
: prefs::kVariationsSafeSeedSignature);

absl::optional<VerifySignatureResult> verify_signature_result;
LoadSeedResult result = VerifyAndParseSeed(
seed, *seed_data, *base64_seed_signature, &verify_signature_result);
if (verify_signature_result.has_value()) {
VerifySignatureResult signature_result = verify_signature_result.value();
// TODO(crbug/1335082): get rid of |signature_verification_enabled_| and only
// support switches::kAcceptEmptySeedSignatureForTesting.
if (signature_verification_enabled_ &&
!AcceptEmptySeedSignatureForTesting(*base64_seed_signature)) {
const VerifySignatureResult result =
VerifySeedSignature(*seed_data, *base64_seed_signature);
if (seed_type == SeedType::LATEST) {
UMA_HISTOGRAM_ENUMERATION("Variations.LoadSeedSignature",
signature_result,
UMA_HISTOGRAM_ENUMERATION("Variations.LoadSeedSignature", result,
VerifySignatureResult::ENUM_SIZE);
} else {
UMA_HISTOGRAM_ENUMERATION(
"Variations.SafeMode.LoadSafeSeed.SignatureValidity",
signature_result, VerifySignatureResult::ENUM_SIZE);
"Variations.SafeMode.LoadSafeSeed.SignatureValidity", result,
VerifySignatureResult::ENUM_SIZE);
}
if (signature_result != VerifySignatureResult::VALID_SIGNATURE) {
if (result != VerifySignatureResult::VALID_SIGNATURE) {
ClearPrefs(seed_type);
return LoadSeedResult::kInvalidSignature;
}
}

if (result == LoadSeedResult::kCorruptProtobuf) {
if (!seed->ParseFromString(*seed_data)) {
ClearPrefs(seed_type);
return LoadSeedResult::kCorruptProtobuf;
}

return result;
return LoadSeedResult::kSuccess;
}

LoadSeedResult VariationsSeedStore::ReadSeedData(SeedType seed_type,
Expand Down
13 changes: 1 addition & 12 deletions components/variations/variations_seed_store.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
#include "components/variations/metrics.h"
#include "components/variations/proto/variations_seed.pb.h"
#include "components/variations/seed_response.h"
#include "third_party/abseil-cpp/absl/types/optional.h"

class PrefService;
class PrefRegistrySimple;
Expand Down Expand Up @@ -102,8 +101,7 @@ class COMPONENT_EXPORT(VARIATIONS) VariationsSeedStore {
// Side effect: Upon failing to read or validate the safe seed, clears all
// of the safe seed pref values.
//
// Virtual for testing and for early-boot CrOS experiments to use a different
// safe seed.
// Virtual for testing.
[[nodiscard]] virtual bool LoadSafeSeed(VariationsSeed* seed,
ClientFilterableState* client_state);

Expand Down Expand Up @@ -156,15 +154,6 @@ class COMPONENT_EXPORT(VARIATIONS) VariationsSeedStore {
const std::string& seed_bytes,
const std::string& base64_seed_signature);

protected:
// Verify an already-loaded |seed_data| along with its |base64_seed_signature|
// and, if verification passes, parse it into |*seed|.
[[nodiscard]] LoadSeedResult VerifyAndParseSeed(
VariationsSeed* seed,
const std::string& seed_data,
const std::string& base64_seed_signature,
absl::optional<VerifySignatureResult>* verify_signature_result);

private:
FRIEND_TEST_ALL_PREFIXES(VariationsSeedStoreTest, VerifySeedSignature);
FRIEND_TEST_ALL_PREFIXES(VariationsSeedStoreTest, ApplyDeltaPatch);
Expand Down

0 comments on commit e60c2c5

Please sign in to comment.