Don't use OSCrypt for trusted vault data
This CL updates a way how trusted vault data is stored locally, to not
use OSCrypt and add MD5 hash verification to control data integrity.
The data is migrated to new format upon ReadDataFromDisk().
The change is guarded by the killswitch.
Bug: 1374650
Change-Id: I6ce13700dc0c7add1df905988f126135fd803396
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3899194
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Commit-Queue: Maksim Moskvitin <mmoskvitin@google.com>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Rushan Suleymanov <rushans@google.com>
Cr-Commit-Position: refs/heads/main@{#1059980}
diff --git a/chrome/browser/ash/crosapi/browser_data_migrator_util.h b/chrome/browser/ash/crosapi/browser_data_migrator_util.h
index ef6b1c8..f8808ee 100644
--- a/chrome/browser/ash/crosapi/browser_data_migrator_util.h
+++ b/chrome/browser/ash/crosapi/browser_data_migrator_util.h
@@ -134,6 +134,7 @@
"structured_metrics",
"Sync Data",
"Trusted Vault",
+ "trusted_vault.pb",
"WebRTC Logs",
"webrtc_event_logs",
"zero_state_group_ranker.pb",
diff --git a/chrome/browser/sync/chrome_sync_client.cc b/chrome/browser/sync/chrome_sync_client.cc
index 4f79125..882278f 100644
--- a/chrome/browser/sync/chrome_sync_client.cc
+++ b/chrome/browser/sync/chrome_sync_client.cc
@@ -10,6 +10,7 @@
#include "base/bind.h"
#include "base/feature_list.h"
+#include "base/files/file_path.h"
#include "base/path_service.h"
#include "base/syslog_logging.h"
#include "build/build_config.h"
@@ -158,12 +159,14 @@
namespace {
#if !BUILDFLAG(IS_ANDROID)
-const base::FilePath::CharType kTrustedVaultFilename[] =
+constexpr base::FilePath::CharType kTrustedVaultFilename[] =
+ FILE_PATH_LITERAL("trusted_vault.pb");
+constexpr base::FilePath::CharType kDeprecatedTrustedVaultFilename[] =
FILE_PATH_LITERAL("Trusted Vault");
#endif // !BUILDFLAG(IS_ANDROID)
#if BUILDFLAG(IS_WIN)
-const base::FilePath::CharType kLoopbackServerBackendFilename[] =
+constexpr base::FilePath::CharType kLoopbackServerBackendFilename[] =
FILE_PATH_LITERAL("profile.pb");
#endif // BUILDFLAG(IS_WIN)
@@ -267,7 +270,9 @@
#else
trusted_vault_client_ =
std::make_unique<syncer::StandaloneTrustedVaultClient>(
- profile_->GetPath().Append(kTrustedVaultFilename), identity_manager,
+ profile_->GetPath().Append(kTrustedVaultFilename),
+ profile_->GetPath().Append(kDeprecatedTrustedVaultFilename),
+ identity_manager,
profile_->GetDefaultStoragePartition()
->GetURLLoaderFactoryForBrowserProcess());
#endif // BUILDFLAG(IS_ANDROID)
diff --git a/components/sync/base/features.cc b/components/sync/base/features.cc
index 41f925f25..26cce5a 100644
--- a/components/sync/base/features.cc
+++ b/components/sync/base/features.cc
@@ -3,6 +3,7 @@
// found in the LICENSE file.
#include "components/sync/base/features.h"
+#include "base/feature_list.h"
namespace syncer {
@@ -110,6 +111,10 @@
"SyncTrustedVaultResetKeysAreStale",
base::FEATURE_ENABLED_BY_DEFAULT);
+BASE_FEATURE(kSyncTrustedVaultUseMD5HashedFile,
+ "SyncTrustedVaultUseMD5HashedFile",
+ base::FEATURE_ENABLED_BY_DEFAULT);
+
BASE_FEATURE(kUseSyncInvalidations,
"UseSyncInvalidations",
base::FEATURE_DISABLED_BY_DEFAULT);
diff --git a/components/sync/base/features.h b/components/sync/base/features.h
index fbdeac4..87c5c10b 100644
--- a/components/sync/base/features.h
+++ b/components/sync/base/features.h
@@ -134,6 +134,9 @@
// registration attempt if previous was failed.
BASE_DECLARE_FEATURE(kSyncTrustedVaultResetKeysAreStale);
+// Enables storing MD5 hashed trusted vault file instead of OSCrypt encrypted.
+BASE_DECLARE_FEATURE(kSyncTrustedVaultUseMD5HashedFile);
+
// If enabled, the device will register with FCM and listen to new
// invalidations. Also, FCM token will be set in DeviceInfo, which signals to
// the server that device listens to new invalidations.
diff --git a/components/sync/driver/trusted_vault_histograms.cc b/components/sync/driver/trusted_vault_histograms.cc
index ff51079..34ae44d 100644
--- a/components/sync/driver/trusted_vault_histograms.cc
+++ b/components/sync/driver/trusted_vault_histograms.cc
@@ -104,4 +104,8 @@
}
}
+void RecordTrustedVaultFileReadStatus(TrustedVaultFileReadStatusForUMA status) {
+ base::UmaHistogramEnumeration("Sync.TrustedVaultFileReadStatus", status);
+}
+
} // namespace syncer
diff --git a/components/sync/driver/trusted_vault_histograms.h b/components/sync/driver/trusted_vault_histograms.h
index 06e55b0..e02f9e25 100644
--- a/components/sync/driver/trusted_vault_histograms.h
+++ b/components/sync/driver/trusted_vault_histograms.h
@@ -55,6 +55,18 @@
kMaxValue = kAborted
};
+// These values are persisted to logs. Entries should not be renumbered and
+// numeric values should never be reused.
+enum class TrustedVaultFileReadStatusForUMA {
+ kSuccess = 0,
+ kNotFound = 1,
+ kFileReadFailed = 2,
+ kMD5DigestMismatch = 3,
+ kFileProtoDeserializationFailed = 4,
+ kDataProtoDeserializationFailed = 5,
+ kMaxValue = kDataProtoDeserializationFailed
+};
+
void RecordTrustedVaultDeviceRegistrationState(
TrustedVaultDeviceRegistrationStateForUMA registration_state);
@@ -80,6 +92,8 @@
bool sample,
const SyncStatus& sync_status);
+void RecordTrustedVaultFileReadStatus(TrustedVaultFileReadStatusForUMA status);
+
} // namespace syncer
#endif // COMPONENTS_SYNC_DRIVER_TRUSTED_VAULT_HISTOGRAMS_H_
diff --git a/components/sync/protocol/local_trusted_vault.proto b/components/sync/protocol/local_trusted_vault.proto
index 48c05e5..98a56b9 100644
--- a/components/sync/protocol/local_trusted_vault.proto
+++ b/components/sync/protocol/local_trusted_vault.proto
@@ -90,3 +90,14 @@
// Version of the stored data, used to perform data migrations.
optional int32 data_version = 2;
}
+
+// Encapsulates serialized local data (LocalTrustedVault) together with its MD5
+// digest, used to store data in the file and verify its integrity.
+message LocalTrustedVaultFileContent {
+ // Serialized LocalTrustedVault.
+ optional string serialized_local_trusted_vault = 1;
+
+ // MD5 digest of `serialized_local_trusted_vault` formatted as hexadecimal
+ // string.
+ optional string md5_digest_hex_string = 2;
+}
\ No newline at end of file
diff --git a/components/sync/trusted_vault/standalone_trusted_vault_backend.cc b/components/sync/trusted_vault/standalone_trusted_vault_backend.cc
index 9d9ea0f5..ac9268e 100644
--- a/components/sync/trusted_vault/standalone_trusted_vault_backend.cc
+++ b/components/sync/trusted_vault/standalone_trusted_vault_backend.cc
@@ -16,6 +16,7 @@
#include "base/feature_list.h"
#include "base/files/file_util.h"
#include "base/files/important_file_writer.h"
+#include "base/hash/md5.h"
#include "base/logging.h"
#include "base/memory/scoped_refptr.h"
#include "base/metrics/histogram_functions.h"
@@ -30,6 +31,7 @@
#include "components/signin/public/identity_manager/accounts_in_cookie_jar_info.h"
#include "components/sync/base/features.h"
#include "components/sync/base/time.h"
+#include "components/sync/driver/trusted_vault_histograms.h"
#include "components/sync/protocol/local_trusted_vault.pb.h"
#include "components/sync/trusted_vault/proto_string_bytes_conversion.h"
#include "components/sync/trusted_vault/securebox.h"
@@ -64,8 +66,46 @@
return proto;
}
-void WriteToDisk(const sync_pb::LocalTrustedVault& data,
- const base::FilePath& file_path) {
+sync_pb::LocalTrustedVault ReadMD5HashedFile(const base::FilePath& file_path) {
+ std::string file_content;
+
+ sync_pb::LocalTrustedVault data_proto;
+ if (!base::PathExists(file_path)) {
+ RecordTrustedVaultFileReadStatus(
+ TrustedVaultFileReadStatusForUMA::kNotFound);
+ return data_proto;
+ }
+ if (!base::ReadFileToString(file_path, &file_content)) {
+ RecordTrustedVaultFileReadStatus(
+ TrustedVaultFileReadStatusForUMA::kFileReadFailed);
+ return data_proto;
+ }
+ sync_pb::LocalTrustedVaultFileContent file_proto;
+ if (!file_proto.ParseFromString(file_content)) {
+ RecordTrustedVaultFileReadStatus(
+ TrustedVaultFileReadStatusForUMA::kFileProtoDeserializationFailed);
+ return data_proto;
+ }
+
+ if (base::MD5String(file_proto.serialized_local_trusted_vault()) !=
+ file_proto.md5_digest_hex_string()) {
+ RecordTrustedVaultFileReadStatus(
+ TrustedVaultFileReadStatusForUMA::kMD5DigestMismatch);
+ return data_proto;
+ }
+
+ if (!data_proto.ParseFromString(
+ file_proto.serialized_local_trusted_vault())) {
+ RecordTrustedVaultFileReadStatus(
+ TrustedVaultFileReadStatusForUMA::kDataProtoDeserializationFailed);
+ return data_proto;
+ }
+ RecordTrustedVaultFileReadStatus(TrustedVaultFileReadStatusForUMA::kSuccess);
+ return data_proto;
+}
+
+void WriteEncryptedFileToDisk(const sync_pb::LocalTrustedVault& data,
+ const base::FilePath& file_path) {
std::string encrypted_data;
const bool encryption_success =
OSCrypt::EncryptString(data.SerializeAsString(), &encrypted_data);
@@ -82,6 +122,36 @@
}
}
+void WriteMD5HashedFileToDisk(const sync_pb::LocalTrustedVault& data,
+ const base::FilePath& file_path) {
+ sync_pb::LocalTrustedVaultFileContent file_proto;
+ file_proto.set_serialized_local_trusted_vault(data.SerializeAsString());
+ file_proto.set_md5_digest_hex_string(
+ base::MD5String(file_proto.serialized_local_trusted_vault()));
+ bool success = base::ImportantFileWriter::WriteFileAtomically(
+ file_path, file_proto.SerializeAsString());
+ if (!success) {
+ DLOG(ERROR) << "Failed to write trusted vault file.";
+ }
+ base::UmaHistogramBoolean("Sync.TrustedVaultFileWriteSuccess", success);
+}
+
+void MaybeMigrateDataFile(const base::FilePath& old_file_path,
+ const base::FilePath& new_file_path) {
+ if (!base::PathExists(old_file_path)) {
+ return;
+ }
+ if (!base::PathExists(new_file_path)) {
+ // Only write to `new_file_path` if it doesn't exist yet to prevent
+ // overwriting the content with stale data.
+ sync_pb::LocalTrustedVault proto = ReadEncryptedFile(old_file_path);
+ WriteMD5HashedFileToDisk(proto, new_file_path);
+ }
+ if (base::PathExists(new_file_path)) {
+ base::DeleteFile(old_file_path);
+ }
+}
+
bool HasNonConstantKey(
const sync_pb::LocalTrustedVaultPerUser& per_user_vault) {
std::string constant_key_as_proto_string;
@@ -216,10 +286,12 @@
}
StandaloneTrustedVaultBackend::StandaloneTrustedVaultBackend(
- const base::FilePath& file_path,
+ const base::FilePath& md5_hashed_file_path,
+ const base::FilePath& deprecated_encrypted_file_path,
std::unique_ptr<Delegate> delegate,
std::unique_ptr<TrustedVaultConnection> connection)
- : file_path_(file_path),
+ : md5_hashed_file_path_(md5_hashed_file_path),
+ deprecated_encrypted_file_path_(deprecated_encrypted_file_path),
delegate_(std::move(delegate)),
connection_(std::move(connection)),
clock_(base::DefaultClock::GetInstance()) {}
@@ -234,7 +306,7 @@
FindUserVault(primary_account_->gaia);
*per_user_vault->mutable_degraded_recoverability_state() =
degraded_recoverability_state;
- WriteToDisk(data_, file_path_);
+ WriteDataToDisk();
}
void StandaloneTrustedVaultBackend::OnDegradedRecoverabilityChanged() {
@@ -242,7 +314,14 @@
}
void StandaloneTrustedVaultBackend::ReadDataFromDisk() {
- data_ = ReadEncryptedFile(file_path_);
+ if (base::FeatureList::IsEnabled(kSyncTrustedVaultUseMD5HashedFile)) {
+ MaybeMigrateDataFile(deprecated_encrypted_file_path_,
+ md5_hashed_file_path_);
+ data_ = ReadMD5HashedFile(md5_hashed_file_path_);
+ } else {
+ data_ = ReadEncryptedFile(deprecated_encrypted_file_path_);
+ }
+
if (data_.user_size() == 0) {
// No data, set the current version and omit writing the file.
data_.set_data_version(kCurrentLocalTrustedVaultVersion);
@@ -250,13 +329,13 @@
if (data_.data_version() == 0) {
UpgradeToVersion1(&data_);
- WriteToDisk(data_, file_path_);
+ WriteDataToDisk();
}
if (base::FeatureList::IsEnabled(kSyncTrustedVaultResetKeysAreStale) &&
data_.data_version() == 1) {
UpgradeToVersion2(&data_);
- WriteToDisk(data_, file_path_);
+ WriteDataToDisk();
}
// TODO(crbug.com/1362513): DCHECK against kCurrentLocalTrustedVaultVersion
@@ -380,7 +459,7 @@
key, per_user_vault->add_vault_key()->mutable_key_material());
}
- WriteToDisk(data_, file_path_);
+ WriteDataToDisk();
MaybeRegisterDevice();
}
@@ -501,7 +580,7 @@
data_.mutable_user()->erase(
base::ranges::remove_if(*data_.mutable_user(), should_remove_user_data),
data_.mutable_user()->end());
- WriteToDisk(data_, file_path_);
+ WriteDataToDisk();
}
bool StandaloneTrustedVaultBackend::MarkLocalKeysAsStale(
@@ -514,7 +593,7 @@
}
per_user_vault->set_keys_marked_as_stale_by_consumer(true);
- WriteToDisk(data_, file_path_);
+ WriteDataToDisk();
return true;
}
@@ -619,7 +698,7 @@
*per_user_vault = sync_pb::LocalTrustedVaultPerUser();
per_user_vault->set_gaia_id(account_info.gaia);
- WriteToDisk(data_, file_path_);
+ WriteDataToDisk();
// This codepath invoked as part of sync reset. While sync reset can cause
// resetting primary account, this is not the case for Chrome OS and Butter
@@ -656,7 +735,7 @@
DCHECK(per_user_vault);
per_user_vault->mutable_local_device_registration_info()
->set_device_registered_version(version);
- WriteToDisk(data_, file_path_);
+ WriteDataToDisk();
}
void StandaloneTrustedVaultBackend::
@@ -666,7 +745,7 @@
DCHECK(per_user_vault);
per_user_vault->mutable_local_device_registration_info()
->set_last_registration_returned_local_data_obsolete(true);
- WriteToDisk(data_, file_path_);
+ WriteDataToDisk();
}
void StandaloneTrustedVaultBackend::SetClockForTesting(base::Clock* clock) {
@@ -754,7 +833,7 @@
key_pair->private_key().ExportToBytes(),
per_user_vault->mutable_local_device_registration_info()
->mutable_private_key_material());
- WriteToDisk(data_, file_path_);
+ WriteDataToDisk();
}
// Cancel existing callbacks passed to |connection_| to ensure there is only
@@ -840,12 +919,12 @@
->set_device_registered(true);
per_user_vault->mutable_local_device_registration_info()
->set_device_registered_version(kCurrentDeviceRegistrationVersion);
- WriteToDisk(data_, file_path_);
+ WriteDataToDisk();
return;
case TrustedVaultRegistrationStatus::kLocalDataObsolete:
per_user_vault->mutable_local_device_registration_info()
->set_last_registration_returned_local_data_obsolete(true);
- WriteToDisk(data_, file_path_);
+ WriteDataToDisk();
return;
case TrustedVaultRegistrationStatus::kAccessTokenFetchingFailure:
// Request wasn't sent to the server, so there is no need for throttling.
@@ -945,7 +1024,7 @@
->set_device_registered(false);
per_user_vault->mutable_local_device_registration_info()
->clear_device_registered_version();
- WriteToDisk(data_, file_path_);
+ WriteDataToDisk();
break;
}
case TrustedVaultDownloadKeysStatus::kNoNewKeys:
@@ -1051,7 +1130,7 @@
FindUserVault(primary_account_->gaia)
->set_last_failed_request_millis_since_unix_epoch(
TimeToProtoTime(clock_->Now()));
- WriteToDisk(data_, file_path_);
+ WriteDataToDisk();
}
void StandaloneTrustedVaultBackend::
@@ -1067,7 +1146,7 @@
data_.mutable_user()->erase(
base::ranges::remove_if(*data_.mutable_user(), should_remove_user_data),
data_.mutable_user()->end());
- WriteToDisk(data_, file_path_);
+ WriteDataToDisk();
}
sync_pb::LocalTrustedVaultPerUser* StandaloneTrustedVaultBackend::FindUserVault(
@@ -1141,4 +1220,12 @@
device_registered_version));
}
+void StandaloneTrustedVaultBackend::WriteDataToDisk() {
+ if (base::FeatureList::IsEnabled(kSyncTrustedVaultUseMD5HashedFile)) {
+ WriteMD5HashedFileToDisk(data_, md5_hashed_file_path_);
+ } else {
+ WriteEncryptedFileToDisk(data_, deprecated_encrypted_file_path_);
+ }
+}
+
} // namespace syncer
diff --git a/components/sync/trusted_vault/standalone_trusted_vault_backend.h b/components/sync/trusted_vault/standalone_trusted_vault_backend.h
index 7ef6eba2..313f2dbb 100644
--- a/components/sync/trusted_vault/standalone_trusted_vault_backend.h
+++ b/components/sync/trusted_vault/standalone_trusted_vault_backend.h
@@ -59,7 +59,8 @@
// interaction with vault service (such as device registration, keys
// downloading, etc.) will be disabled.
StandaloneTrustedVaultBackend(
- const base::FilePath& file_path,
+ const base::FilePath& md5_hashed_file_path,
+ const base::FilePath& deprecated_encrypted_file_path,
std::unique_ptr<Delegate> delegate,
std::unique_ptr<TrustedVaultConnection> connection);
StandaloneTrustedVaultBackend(const StandaloneTrustedVaultBackend& other) =
@@ -199,7 +200,10 @@
void VerifyDeviceRegistrationForUMA(const std::string& gaia_id);
- const base::FilePath file_path_;
+ void WriteDataToDisk();
+
+ const base::FilePath md5_hashed_file_path_;
+ const base::FilePath deprecated_encrypted_file_path_;
const std::unique_ptr<Delegate> delegate_;
diff --git a/components/sync/trusted_vault/standalone_trusted_vault_backend_unittest.cc b/components/sync/trusted_vault/standalone_trusted_vault_backend_unittest.cc
index 712b88e..5b1d189d 100644
--- a/components/sync/trusted_vault/standalone_trusted_vault_backend_unittest.cc
+++ b/components/sync/trusted_vault/standalone_trusted_vault_backend_unittest.cc
@@ -10,8 +10,10 @@
#include <vector>
#include "base/callback_helpers.h"
+#include "base/files/file_path.h"
#include "base/files/file_util.h"
#include "base/files/scoped_temp_dir.h"
+#include "base/hash/md5.h"
#include "base/memory/raw_ptr.h"
#include "base/test/metrics/histogram_tester.h"
#include "base/test/mock_callback.h"
@@ -83,18 +85,27 @@
return account_info;
}
-bool WriteLocalTrustedVaultFile(const sync_pb::LocalTrustedVault& content,
+bool WriteLocalTrustedVaultFile(const sync_pb::LocalTrustedVault& proto,
const base::FilePath& path) {
+ sync_pb::LocalTrustedVaultFileContent file_proto;
+ file_proto.set_serialized_local_trusted_vault(proto.SerializeAsString());
+ file_proto.set_md5_digest_hex_string(
+ base::MD5String(file_proto.serialized_local_trusted_vault()));
+ return base::WriteFile(path, file_proto.SerializeAsString());
+}
+
+bool WriteLocalEncryptedTrustedVaultFile(
+ const sync_pb::LocalTrustedVault& proto,
+ const base::FilePath& path) {
std::string encrypted_content;
- if (!OSCrypt::EncryptString(content.SerializeAsString(),
- &encrypted_content)) {
+ if (!OSCrypt::EncryptString(proto.SerializeAsString(), &encrypted_content)) {
return false;
}
return base::WriteFile(path, encrypted_content.c_str(),
encrypted_content.size()) != -1;
}
-sync_pb::LocalTrustedVault ReadLocalTrustedVaultFile(
+sync_pb::LocalTrustedVault ReadLocalEncryptedTrustedVaultFile(
const base::FilePath& path) {
std::string ciphertext;
base::ReadFileToString(path, &ciphertext);
@@ -107,6 +118,27 @@
return proto;
}
+sync_pb::LocalTrustedVault ReadLocalTrustedVaultFile(
+ const base::FilePath& path) {
+ std::string file_content;
+ sync_pb::LocalTrustedVault data_proto;
+ if (!base::ReadFileToString(path, &file_content)) {
+ return data_proto;
+ }
+ sync_pb::LocalTrustedVaultFileContent file_proto;
+ if (!file_proto.ParseFromString(file_content)) {
+ return data_proto;
+ }
+
+ if (base::MD5String(file_proto.serialized_local_trusted_vault()) !=
+ file_proto.md5_digest_hex_string()) {
+ return data_proto;
+ }
+
+ data_proto.ParseFromString(file_proto.serialized_local_trusted_vault());
+ return data_proto;
+}
+
class MockDelegate : public StandaloneTrustedVaultBackend::Delegate {
public:
MockDelegate() = default;
@@ -152,9 +184,10 @@
class StandaloneTrustedVaultBackendTest : public testing::Test {
public:
StandaloneTrustedVaultBackendTest()
- : file_path_(
- CreateUniqueTempDir(&temp_dir_)
- .Append(base::FilePath(FILE_PATH_LITERAL("some_file")))) {
+ : file_path_(CreateUniqueTempDir(&temp_dir_)
+ .Append(base::FilePath(FILE_PATH_LITERAL("some_file")))),
+ deprecated_file_path_(temp_dir_.GetPath().Append(
+ base::FilePath(FILE_PATH_LITERAL("deprecated_file")))) {
clock_.SetNow(base::Time::Now());
ResetBackend();
}
@@ -174,7 +207,8 @@
connection_ = connection.get();
backend_ = base::MakeRefCounted<StandaloneTrustedVaultBackend>(
- file_path_, std::move(delegate), std::move(connection));
+ file_path_, deprecated_file_path_, std::move(delegate),
+ std::move(connection));
backend_->SetClockForTesting(&clock_);
backend_->ReadDataFromDisk();
@@ -199,6 +233,8 @@
const base::FilePath& file_path() { return file_path_; }
+ const base::FilePath& deprecated_file_path() { return deprecated_file_path_; }
+
// Stores |vault_keys| and mimics successful device registration, returns
// private device key material.
std::vector<uint8_t> StoreKeysAndMimicDeviceRegistration(
@@ -254,6 +290,7 @@
private:
base::ScopedTempDir temp_dir_;
const base::FilePath file_path_;
+ const base::FilePath deprecated_file_path_;
raw_ptr<testing::NiceMock<MockDelegate>> delegate_;
raw_ptr<testing::NiceMock<MockTrustedVaultConnection>> connection_;
base::SimpleTestClock clock_;
@@ -307,6 +344,60 @@
backend()->FetchKeys(account_info, fetch_keys_callback.Get());
}
+TEST_F(StandaloneTrustedVaultBackendTest, ShouldRecordNotFoundWhenReadingFile) {
+ base::HistogramTester histogram_tester;
+ backend()->ReadDataFromDisk();
+ histogram_tester.ExpectUniqueSample(
+ "Sync.TrustedVaultFileReadStatus",
+ /*sample=*/TrustedVaultFileReadStatusForUMA::kNotFound,
+ /*expected_bucket_count=*/1);
+}
+
+TEST_F(StandaloneTrustedVaultBackendTest,
+ ShouldRecordMD5DigestMismatchWhenReadingFile) {
+ sync_pb::LocalTrustedVaultFileContent file_proto;
+ file_proto.set_md5_digest_hex_string("corrupted_md5_digest");
+ ASSERT_TRUE(base::WriteFile(file_path(), file_proto.SerializeAsString()));
+
+ base::HistogramTester histogram_tester;
+ backend()->ReadDataFromDisk();
+ histogram_tester.ExpectUniqueSample(
+ "Sync.TrustedVaultFileReadStatus",
+ /*sample=*/TrustedVaultFileReadStatusForUMA::kMD5DigestMismatch,
+ /*expected_bucket_count=*/1);
+}
+
+TEST_F(StandaloneTrustedVaultBackendTest,
+ ShouldRecordFileProtoDeserializationFailedWhenReadingFile) {
+ ASSERT_TRUE(base::WriteFile(file_path(), "corrupted_proto"));
+
+ base::HistogramTester histogram_tester;
+ backend()->ReadDataFromDisk();
+ histogram_tester.ExpectUniqueSample(
+ "Sync.TrustedVaultFileReadStatus",
+ /*sample=*/
+ TrustedVaultFileReadStatusForUMA::kFileProtoDeserializationFailed,
+ /*expected_bucket_count=*/1);
+}
+
+TEST_F(StandaloneTrustedVaultBackendTest,
+ ShouldRecordDataProtoDeserializationFailedWhenReadingFile) {
+ const std::string kCorruptedSerializedDataProto = "corrupted_proto";
+ sync_pb::LocalTrustedVaultFileContent file_proto;
+ file_proto.set_serialized_local_trusted_vault(kCorruptedSerializedDataProto);
+ file_proto.set_md5_digest_hex_string(
+ base::MD5String(kCorruptedSerializedDataProto));
+ ASSERT_TRUE(base::WriteFile(file_path(), file_proto.SerializeAsString()));
+
+ base::HistogramTester histogram_tester;
+ backend()->ReadDataFromDisk();
+ histogram_tester.ExpectUniqueSample(
+ "Sync.TrustedVaultFileReadStatus",
+ /*sample=*/
+ TrustedVaultFileReadStatusForUMA::kDataProtoDeserializationFailed,
+ /*expected_bucket_count=*/1);
+}
+
TEST_F(StandaloneTrustedVaultBackendTest, ShouldReadAndFetchNonEmptyKeys) {
const CoreAccountInfo account_info_1 = MakeAccountInfoWithGaiaId("user1");
const CoreAccountInfo account_info_2 = MakeAccountInfoWithGaiaId("user2");
@@ -325,6 +416,45 @@
user_data2->add_vault_key()->set_key_material(kKey3.data(), kKey3.size());
ASSERT_TRUE(WriteLocalTrustedVaultFile(initial_data, file_path()));
+ base::HistogramTester histogram_tester;
+ backend()->ReadDataFromDisk();
+ histogram_tester.ExpectUniqueSample(
+ "Sync.TrustedVaultFileReadStatus",
+ /*sample=*/TrustedVaultFileReadStatusForUMA::kSuccess,
+ /*expected_bucket_count=*/1);
+
+ // Keys should be fetched immediately for both accounts.
+ base::MockCallback<StandaloneTrustedVaultBackend::FetchKeysCallback>
+ fetch_keys_callback;
+ EXPECT_CALL(fetch_keys_callback, Run(/*keys=*/ElementsAre(kKey1)));
+ backend()->FetchKeys(account_info_1, fetch_keys_callback.Get());
+ EXPECT_CALL(fetch_keys_callback, Run(/*keys=*/ElementsAre(kKey2, kKey3)));
+ backend()->FetchKeys(account_info_2, fetch_keys_callback.Get());
+}
+
+TEST_F(StandaloneTrustedVaultBackendTest,
+ ShouldReadAndFetchNonEmptyKeysFromDeprecatedFile) {
+ base::test::ScopedFeatureList scoped_feature_list;
+ scoped_feature_list.InitAndDisableFeature(kSyncTrustedVaultUseMD5HashedFile);
+
+ const CoreAccountInfo account_info_1 = MakeAccountInfoWithGaiaId("user1");
+ const CoreAccountInfo account_info_2 = MakeAccountInfoWithGaiaId("user2");
+
+ const std::vector<uint8_t> kKey1 = {0, 1, 2, 3, 4};
+ const std::vector<uint8_t> kKey2 = {1, 2, 3, 4};
+ const std::vector<uint8_t> kKey3 = {2, 3, 4};
+
+ sync_pb::LocalTrustedVault initial_data;
+ sync_pb::LocalTrustedVaultPerUser* user_data1 = initial_data.add_user();
+ sync_pb::LocalTrustedVaultPerUser* user_data2 = initial_data.add_user();
+ user_data1->set_gaia_id(account_info_1.gaia);
+ user_data2->set_gaia_id(account_info_2.gaia);
+ user_data1->add_vault_key()->set_key_material(kKey1.data(), kKey1.size());
+ user_data2->add_vault_key()->set_key_material(kKey2.data(), kKey2.size());
+ user_data2->add_vault_key()->set_key_material(kKey3.data(), kKey3.size());
+
+ ASSERT_TRUE(WriteLocalEncryptedTrustedVaultFile(initial_data,
+ deprecated_file_path()));
backend()->ReadDataFromDisk();
// Keys should be fetched immediately for both accounts.
@@ -336,6 +466,38 @@
backend()->FetchKeys(account_info_2, fetch_keys_callback.Get());
}
+TEST_F(StandaloneTrustedVaultBackendTest, ShouldMigrateDataFromDeprecatedFile) {
+ const CoreAccountInfo account_info = MakeAccountInfoWithGaiaId("user1");
+ const std::vector<uint8_t> kKey = {0, 1, 2, 3, 4};
+ const int kLastKeyVersion = 1;
+
+ sync_pb::LocalTrustedVault initial_data;
+ // Migration from version 0 to version 1 makes test more complex, bypass it.
+ initial_data.set_data_version(1);
+
+ sync_pb::LocalTrustedVaultPerUser* user_data = initial_data.add_user();
+ user_data->set_gaia_id(account_info.gaia);
+ user_data->add_vault_key()->set_key_material(kKey.data(), kKey.size());
+ user_data->set_last_vault_key_version(kLastKeyVersion);
+
+ ASSERT_TRUE(WriteLocalEncryptedTrustedVaultFile(initial_data,
+ deprecated_file_path()));
+ backend()->ReadDataFromDisk();
+
+ // Ensure that backend is able to use data from deprecated file.
+ base::MockCallback<StandaloneTrustedVaultBackend::FetchKeysCallback>
+ fetch_keys_callback;
+ EXPECT_CALL(fetch_keys_callback, Run(/*keys=*/ElementsAre(kKey)));
+ backend()->FetchKeys(account_info, fetch_keys_callback.Get());
+
+ // Ensure that backend completed file migration.
+ EXPECT_FALSE(base::PathExists(deprecated_file_path()));
+ sync_pb::LocalTrustedVault proto = ReadLocalTrustedVaultFile(file_path());
+ ASSERT_THAT(proto.user_size(), Eq(1));
+ EXPECT_THAT(proto.user(0).vault_key(), ElementsAre(KeyMaterialEq(kKey)));
+ EXPECT_THAT(proto.user(0).last_vault_key_version(), Eq(kLastKeyVersion));
+}
+
TEST_F(StandaloneTrustedVaultBackendTest, ShouldFilterOutConstantKey) {
const CoreAccountInfo account_info = MakeAccountInfoWithGaiaId("user1");
const std::vector<uint8_t> kKey = {1, 2, 3, 4};
@@ -365,13 +527,44 @@
const std::vector<uint8_t> kKey3 = {2, 3, 4};
const std::vector<uint8_t> kKey4 = {3, 4};
+ base::HistogramTester histogram_tester;
+ backend()->StoreKeys(kGaiaId1, {kKey1}, /*last_key_version=*/7);
+ backend()->StoreKeys(kGaiaId2, {kKey2}, /*last_key_version=*/8);
+ // Keys for |kGaiaId2| overridden, so |kKey2| should be lost.
+ backend()->StoreKeys(kGaiaId2, {kKey3, kKey4}, /*last_key_version=*/9);
+ histogram_tester.ExpectUniqueSample("Sync.TrustedVaultFileWriteSuccess",
+ /*sample=*/true,
+ /*expected_bucket_count=*/3);
+
+ // Read the file from disk.
+ sync_pb::LocalTrustedVault proto = ReadLocalTrustedVaultFile(file_path());
+ ASSERT_THAT(proto.user_size(), Eq(2));
+ EXPECT_THAT(proto.user(0).vault_key(), ElementsAre(KeyMaterialEq(kKey1)));
+ EXPECT_THAT(proto.user(0).last_vault_key_version(), Eq(7));
+ EXPECT_THAT(proto.user(1).vault_key(),
+ ElementsAre(KeyMaterialEq(kKey3), KeyMaterialEq(kKey4)));
+ EXPECT_THAT(proto.user(1).last_vault_key_version(), Eq(9));
+}
+
+TEST_F(StandaloneTrustedVaultBackendTest, ShouldStoreKeysInDeprecatedFile) {
+ base::test::ScopedFeatureList scoped_feature_list;
+ scoped_feature_list.InitAndDisableFeature(kSyncTrustedVaultUseMD5HashedFile);
+
+ const std::string kGaiaId1 = "user1";
+ const std::string kGaiaId2 = "user2";
+ const std::vector<uint8_t> kKey1 = {0, 1, 2, 3, 4};
+ const std::vector<uint8_t> kKey2 = {1, 2, 3, 4};
+ const std::vector<uint8_t> kKey3 = {2, 3, 4};
+ const std::vector<uint8_t> kKey4 = {3, 4};
+
backend()->StoreKeys(kGaiaId1, {kKey1}, /*last_key_version=*/7);
backend()->StoreKeys(kGaiaId2, {kKey2}, /*last_key_version=*/8);
// Keys for |kGaiaId2| overridden, so |kKey2| should be lost.
backend()->StoreKeys(kGaiaId2, {kKey3, kKey4}, /*last_key_version=*/9);
// Read the file from disk.
- sync_pb::LocalTrustedVault proto = ReadLocalTrustedVaultFile(file_path());
+ sync_pb::LocalTrustedVault proto =
+ ReadLocalEncryptedTrustedVaultFile(deprecated_file_path());
ASSERT_THAT(proto.user_size(), Eq(2));
EXPECT_THAT(proto.user(0).vault_key(), ElementsAre(KeyMaterialEq(kKey1)));
EXPECT_THAT(proto.user(0).last_vault_key_version(), Eq(7));
@@ -466,7 +659,8 @@
// Instantiate a second backend to read the file.
auto other_backend = base::MakeRefCounted<StandaloneTrustedVaultBackend>(
- file_path(), std::make_unique<testing::NiceMock<MockDelegate>>(),
+ file_path(), deprecated_file_path(),
+ std::make_unique<testing::NiceMock<MockDelegate>>(),
std::make_unique<testing::NiceMock<MockTrustedVaultConnection>>());
other_backend->ReadDataFromDisk();
@@ -562,7 +756,7 @@
// Mimic browser restart and reset primary account.
auto new_backend = base::MakeRefCounted<StandaloneTrustedVaultBackend>(
- file_path(),
+ file_path(), deprecated_file_path(),
/*delegate=*/std::make_unique<testing::NiceMock<MockDelegate>>(),
/*connection=*/nullptr);
new_backend->ReadDataFromDisk();
@@ -671,13 +865,7 @@
.Run(TrustedVaultRegistrationStatus::kLocalDataObsolete);
// Verify persisted file state.
- std::string ciphertext;
- std::string decrypted_content;
- sync_pb::LocalTrustedVault proto;
- EXPECT_TRUE(base::ReadFileToString(file_path(), &ciphertext));
- EXPECT_THAT(ciphertext, Ne(""));
- EXPECT_TRUE(OSCrypt::DecryptString(ciphertext, &decrypted_content));
- EXPECT_TRUE(proto.ParseFromString(decrypted_content));
+ sync_pb::LocalTrustedVault proto = ReadLocalTrustedVaultFile(file_path());
ASSERT_THAT(proto.user_size(), Eq(1));
// Ensure that the failure is remembered, so there are no retries. This is a
// regression test for crbug.com/1358015.
diff --git a/components/sync/trusted_vault/standalone_trusted_vault_client.cc b/components/sync/trusted_vault/standalone_trusted_vault_client.cc
index 060d5e9..4f165893 100644
--- a/components/sync/trusted_vault/standalone_trusted_vault_client.cc
+++ b/components/sync/trusted_vault/standalone_trusted_vault_client.cc
@@ -223,6 +223,7 @@
StandaloneTrustedVaultClient::StandaloneTrustedVaultClient(
const base::FilePath& file_path,
+ const base::FilePath& deprecated_file_path,
signin::IdentityManager* identity_manager,
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
: backend_task_runner_(
@@ -240,7 +241,7 @@
}
backend_ = base::MakeRefCounted<StandaloneTrustedVaultBackend>(
- file_path,
+ file_path, deprecated_file_path,
std::make_unique<
BackendDelegate>(BindToCurrentSequence(base::BindRepeating(
&StandaloneTrustedVaultClient::NotifyRecoverabilityDegradedChanged,
diff --git a/components/sync/trusted_vault/standalone_trusted_vault_client.h b/components/sync/trusted_vault/standalone_trusted_vault_client.h
index baa2cce0..27b8b28f 100644
--- a/components/sync/trusted_vault/standalone_trusted_vault_client.h
+++ b/components/sync/trusted_vault/standalone_trusted_vault_client.h
@@ -41,6 +41,7 @@
// |url_loader_factory| must not be null.
StandaloneTrustedVaultClient(
const base::FilePath& file_path,
+ const base::FilePath& deprecated_file_path,
signin::IdentityManager* identity_manager,
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory);
diff --git a/tools/metrics/histograms/enums.xml b/tools/metrics/histograms/enums.xml
index 5ca67b9..43279be 100644
--- a/tools/metrics/histograms/enums.xml
+++ b/tools/metrics/histograms/enums.xml
@@ -98679,6 +98679,15 @@
<int value="1" label="Second attempt"/>
</enum>
+<enum name="TrustedVaultFileReadStatus">
+ <int value="0" label="Success"/>
+ <int value="1" label="File not found"/>
+ <int value="2" label="File read failed"/>
+ <int value="3" label="MD5 digest mismatch"/>
+ <int value="4" label="File proto deserialization failed"/>
+ <int value="5" label="Data proto deserialization failed"/>
+</enum>
+
<enum name="TrustedVaultUserActionTrigger">
<int value="0" label="Settings page"/>
<int value="1" label="Profile menu"/>
diff --git a/tools/metrics/histograms/metadata/sync/histograms.xml b/tools/metrics/histograms/metadata/sync/histograms.xml
index dea969ba..2ed8c4d 100644
--- a/tools/metrics/histograms/metadata/sync/histograms.xml
+++ b/tools/metrics/histograms/metadata/sync/histograms.xml
@@ -1446,6 +1446,24 @@
<summary>Recorded when fetching trusted vault keys is attempted.</summary>
</histogram>
+<histogram name="Sync.TrustedVaultFileReadStatus"
+ enum="TrustedVaultFileReadStatus" expires_after="2023-04-09">
+ <owner>mmoskvitin@google.com</owner>
+ <owner>mastiz@chromium.org</owner>
+ <component>Services>Sync</component>
+ <summary>Recorded when reading local trusted vault file.</summary>
+</histogram>
+
+<histogram name="Sync.TrustedVaultFileWriteSuccess" enum="Boolean"
+ expires_after="2023-04-09">
+ <owner>mmoskvitin@google.com</owner>
+ <owner>mastiz@chromium.org</owner>
+ <component>Services>Sync</component>
+ <summary>
+ Records whether writing local trusted vault file upon each write.
+ </summary>
+</histogram>
+
<histogram name="Sync.TrustedVaultKeyRetrievalTrigger"
enum="TrustedVaultUserActionTrigger" expires_after="2023-04-09">
<owner>mmoskvitin@google.com</owner>
