cryptohome: simplify the timestamp file interaction
Instead of dealing with markers of ts file existence in the keyset, just
unconditionally read it on the load path and just use the legacy field
if the file is missing. This CL is a preparation to the untying
timestamp from the keyset entirely by introducing a single ts file (in
contrast with current per-keyset ones).
BUG=b:172344610
TEST=unittest
Change-Id: I6f0273005e83198e8561664c36dfa0b9c1443e19
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2569474
Commit-Queue: Daniil Lunev <dlunev@chromium.org>
Tested-by: Daniil Lunev <dlunev@chromium.org>
Reviewed-by: Leo Lai <cylai@google.com>
diff --git a/cryptohome/cryptohome.cc b/cryptohome/cryptohome.cc
index 982a101..7223793 100644
--- a/cryptohome/cryptohome.cc
+++ b/cryptohome/cryptohome.cc
@@ -1538,28 +1538,26 @@
printf(" Password rounds:\n");
printf(" %d\n", serialized.password_rounds());
}
- if (serialized.has_timestamp_file_exists()) {
- FilePath timestamp_path = vault_path.AddExtension("timestamp");
- brillo::Blob tcontents;
- if (!platform.ReadFile(timestamp_path, &tcontents)) {
- printf("Couldn't load timestamp contents: %s.\n",
- timestamp_path.value().c_str());
- }
+
+ base::Time last_activity =
+ base::Time::FromInternalValue(serialized.last_activity_timestamp());
+ FilePath timestamp_path = vault_path.AddExtension("timestamp");
+ brillo::Blob tcontents;
+ if (platform.ReadFile(timestamp_path, &tcontents)) {
cryptohome::Timestamp timestamp;
if (!timestamp.ParseFromArray(tcontents.data(), tcontents.size())) {
printf("Couldn't parse timestamp contents: %s.\n",
timestamp_path.value().c_str());
}
- const base::Time last_activity =
- base::Time::FromInternalValue(timestamp.timestamp());
- printf(" Last activity (days ago):\n");
- printf(" %d\n", (base::Time::Now() - last_activity).InDays());
- } else if (serialized.has_last_activity_timestamp()) {
- const base::Time last_activity =
- base::Time::FromInternalValue(serialized.last_activity_timestamp());
- printf(" Last activity (days ago):\n");
- printf(" %d\n", (base::Time::Now() - last_activity).InDays());
+ last_activity = base::Time::FromInternalValue(timestamp.timestamp());
+ } else {
+ printf("Couldn't load timestamp contents: %s.\n",
+ timestamp_path.value().c_str());
}
+
+ printf(" Last activity (days ago):\n");
+ printf(" %d\n", (base::Time::Now() - last_activity).InDays());
+
} else if (!strcmp(switches::kActions[switches::ACTION_DUMP_LAST_ACTIVITY],
action.c_str())) {
std::vector<FilePath> user_dirs;
@@ -1584,7 +1582,7 @@
if (file_name.value() != cryptohome::kKeyFile)
continue;
brillo::Blob contents;
- if (!platform.ReadFile(next_path, &contents)) {
+ if (platform.ReadFile(next_path, &contents)) {
LOG(ERROR) << "Couldn't load keyset: " << next_path.value();
continue;
}
@@ -1593,28 +1591,25 @@
LOG(ERROR) << "Couldn't parse keyset: " << next_path.value();
continue;
}
- if (keyset.has_timestamp_file_exists()) {
- FilePath timestamp_path = next_path.AddExtension("timestamp");
- brillo::Blob tcontents;
- if (!platform.ReadFile(timestamp_path, &tcontents)) {
- printf("Couldn't load timestamp contents: %s.\n",
- timestamp_path.value().c_str());
- }
+ base::Time last_activity =
+ base::Time::FromInternalValue(keyset.last_activity_timestamp());
+
+ FilePath timestamp_path = next_path.AddExtension("timestamp");
+ brillo::Blob tcontents;
+ if (platform.ReadFile(timestamp_path, &tcontents)) {
cryptohome::Timestamp timestamp;
if (!timestamp.ParseFromArray(tcontents.data(), tcontents.size())) {
printf("Couldn't parse timestamp contents: %s.\n",
timestamp_path.value().c_str());
}
- const base::Time last_activity =
- base::Time::FromInternalValue(timestamp.timestamp());
- if (last_activity > max_activity) {
- max_activity = last_activity;
- }
- } else if (keyset.has_last_activity_timestamp()) {
- const base::Time last_activity =
- base::Time::FromInternalValue(keyset.last_activity_timestamp());
- if (last_activity > max_activity)
- max_activity = last_activity;
+ last_activity = base::Time::FromInternalValue(timestamp.timestamp());
+ } else {
+ printf("Couldn't load timestamp contents: %s.\n",
+ timestamp_path.value().c_str());
+ }
+
+ if (last_activity > max_activity) {
+ max_activity = last_activity;
}
}
if (max_activity > base::Time::UnixEpoch()) {
diff --git a/cryptohome/homedirs.cc b/cryptohome/homedirs.cc
index c5e8f9e..f697e16 100644
--- a/cryptohome/homedirs.cc
+++ b/cryptohome/homedirs.cc
@@ -368,6 +368,7 @@
LOG(ERROR) << "Failed to encrypt and write keyset for the new user.";
return false;
}
+ UpdateActivityTimestamp(obfuscated_username, kInitialKeysetIndex, 0);
return true;
}
@@ -522,11 +523,6 @@
bool HomeDirs::UpdateActivityTimestamp(const std::string& obfuscated,
int index,
int time_shift_sec) {
- std::unique_ptr<VaultKeyset> keyset(
- LoadVaultKeysetForUser(obfuscated, index));
- if (!keyset) {
- return false;
- }
base::Time timestamp = platform_->GetCurrentTime();
if (time_shift_sec > 0) {
timestamp -= base::TimeDelta::FromSeconds(time_shift_sec);
@@ -546,19 +542,6 @@
return false;
}
- // The first time we write to a timestamp file we need to update the
- // vault_keyset to indicate that the timestamp is stored separately.
- // The initial 0 timestamp is also written to the vault_keyset which
- // means a timestamp will exist and can be read in case of a rollback.
- if (!keyset->serialized().timestamp_file_exists()) {
- keyset->mutable_serialized()->set_timestamp_file_exists(true);
- if (!keyset->Save(keyset->source_file())) {
- LOG(ERROR) << "Failed updating ts marker in keyset: "
- << keyset->source_file();
- return false;
- }
- }
-
if (timestamp_cache_ && timestamp_cache_->initialized()) {
timestamp_cache_->UpdateExistingUser(obfuscated, timestamp);
}
@@ -658,8 +641,6 @@
if (new_data) {
*(vk->mutable_serialized()->mutable_key_data()) = *new_data;
}
- // The new keyset doesn't have an associated timestamp file.
- vk->mutable_serialized()->set_timestamp_file_exists(false);
// Repersist the VK with the new creds.
CryptohomeErrorCode added = CRYPTOHOME_ERROR_NOT_SET;
diff --git a/cryptohome/homedirs_unittest.cc b/cryptohome/homedirs_unittest.cc
index fed82eb..618c864 100644
--- a/cryptohome/homedirs_unittest.cc
+++ b/cryptohome/homedirs_unittest.cc
@@ -445,7 +445,6 @@
users_[2].homedir_path.Append(kKeyFile).AddExtension(
kKeyFileTimestampSuffix),
timestamp_str, 0600));
- vk.mutable_serialized()->set_timestamp_file_exists(true);
ASSERT_TRUE(vk.Encrypt(brillo::SecureBlob("random"), users_[2].obfuscated));
ASSERT_TRUE(vk.Save(users_[2].homedir_path.Append(kKeyFile).AddExtension(
kKeyFileIndexSuffix)));
diff --git a/cryptohome/make_tests.cc b/cryptohome/make_tests.cc
index 9a2cdb6..ec78897 100644
--- a/cryptohome/make_tests.cc
+++ b/cryptohome/make_tests.cc
@@ -274,6 +274,8 @@
.WillRepeatedly(Return(true));
EXPECT_CALL(*platform, ReadFile(keyset_path, _))
.WillRepeatedly(DoAll(SetArgPointee<1>(credentials), Return(true)));
+ EXPECT_CALL(*platform, ReadFile(timestamp_path, _))
+ .WillRepeatedly(Return(false));
if (enumerate) {
EXPECT_CALL(*platform, GetFileEnumerator(base_path, false, _))
.WillRepeatedly(Invoke([&](base::FilePath path, bool rec, int type) {
diff --git a/cryptohome/user_session_unittest.cc b/cryptohome/user_session_unittest.cc
index 1d4e257..970588f 100644
--- a/cryptohome/user_session_unittest.cc
+++ b/cryptohome/user_session_unittest.cc
@@ -142,7 +142,8 @@
.WillOnce(Return(true));
EXPECT_CALL(*mount_, IsNonEphemeralMounted()).WillOnce(Return(true));
EXPECT_CALL(platform_, GetCurrentTime())
- .WillOnce(Return(base::Time::FromInternalValue(kTs1)));
+ .Times(2) // Initial set and update on mount.
+ .WillRepeatedly(Return(base::Time::FromInternalValue(kTs1)));
// TEST
@@ -233,7 +234,8 @@
.WillOnce(Return(true));
EXPECT_CALL(*mount_, IsNonEphemeralMounted()).WillOnce(Return(true));
EXPECT_CALL(platform_, GetCurrentTime())
- .WillOnce(Return(base::Time::FromInternalValue(kTs1)));
+ .Times(2) // Initial set and update on mount.
+ .WillRepeatedly(Return(base::Time::FromInternalValue(kTs1)));
ASSERT_EQ(MOUNT_ERROR_NONE,
session_->MountVault(users_[0].credentials, mount_args_create));
diff --git a/cryptohome/vault_keyset.cc b/cryptohome/vault_keyset.cc
index dded6a8..0e987aa 100644
--- a/cryptohome/vault_keyset.cc
+++ b/cryptohome/vault_keyset.cc
@@ -235,34 +235,20 @@
->mutable_policy()
->set_low_entropy_credential(true);
}
- if (serialized_.has_timestamp_file_exists() &&
- serialized_.timestamp_file_exists()) {
- bool set_from_file_successfully = false;
- FilePath timestamp_path = filename.AddExtension("timestamp");
- brillo::Blob tcontents;
- if (platform_->ReadFile(timestamp_path, &tcontents)) {
- cryptohome::Timestamp timestamp;
- if (timestamp.ParseFromArray(tcontents.data(), tcontents.size())) {
- serialized_.set_last_activity_timestamp(timestamp.timestamp());
- set_from_file_successfully = true;
- } else {
- LOG(WARNING) << "Failure to parse timestamp file: " << timestamp_path;
- }
- } else {
- LOG(WARNING) << "Failure to read timestamp file: " << timestamp_path;
- }
- if (!set_from_file_successfully) {
- // We don't fail the VaultKeyset load here because if it fails, the user
- // may have to recreate their entire cryptohome for this minor error.
- // Instead, we log the error (because it's minor), and let it pass with
- // a reasonable default value for last_activity_timestamp, and that is
- // the current time.
- LOG(WARNING) << "Not failing attempt to Load() due to timestamp file "
- "problem. Setting last activity timestamp to now";
- serialized_.set_last_activity_timestamp(
- platform_->GetCurrentTime().ToInternalValue());
+ FilePath timestamp_path = filename.AddExtension("timestamp");
+ brillo::Blob tcontents;
+ // If we fail to read the ts file, just use whatever is stored in the
+ // serialized field.
+ if (platform_->ReadFile(timestamp_path, &tcontents)) {
+ cryptohome::Timestamp timestamp;
+ if (timestamp.ParseFromArray(tcontents.data(), tcontents.size())) {
+ serialized_.set_last_activity_timestamp(timestamp.timestamp());
+ } else {
+ LOG(WARNING) << "Failure to parse timestamp file: " << timestamp_path;
}
+ } else {
+ LOG(WARNING) << "Failure to read timestamp file: " << timestamp_path;
}
}
return loaded_;
diff --git a/cryptohome/vault_keyset.proto b/cryptohome/vault_keyset.proto
index 1af242f..e0941d7 100644
--- a/cryptohome/vault_keyset.proto
+++ b/cryptohome/vault_keyset.proto
@@ -130,12 +130,9 @@
// the case when PCR is extended with user specific value.
optional bytes extended_tpm_key = 18;
- // Set if the timestamp has been moved to its own file.
- optional bool timestamp_file_exists = 19;
-
// Set if the vault keyset should be used with fscrypt v2 encryption policies.
optional int32 fscrypt_policy_version = 20;
// Deprecated fields that were removed.
- reserved 7;
+ reserved 7, 19;
}
diff --git a/cryptohome/vault_keyset_unittest.cc b/cryptohome/vault_keyset_unittest.cc
index b6f25d7..b6f2e27 100644
--- a/cryptohome/vault_keyset_unittest.cc
+++ b/cryptohome/vault_keyset_unittest.cc
@@ -155,7 +155,6 @@
static_cast<google::protobuf::uint8*>(tbytes.data());
timestamp.SerializeWithCachedSizesToArray(buf);
- keyset.mutable_serialized()->set_timestamp_file_exists(true);
keyset.SetFscryptPolicyVersion(kFscryptPolicyVersion);
EXPECT_CALL(platform, WriteFileAtomicDurable(FilePath("foo"), _, _))
