Add trusted vault key rotation logic

This CL adds high-level trusted vault key rotation logic. Once vault
keys are marked as stale, following FetchKeys() call will trigger
attempt to download fresh keys from the vault server. This makes
StandaloneTrustedVaultBackend::FetchKeys() asynchronous, but result
is populated immediately if preconditions aren't met (e.g. device isn't
registered, device somehow misses old vault keys or current account
isn't syncing one).

StandaloneTrustedVaultBackend allows only one ongoing request to the
vault server and only one ongoing fetch keys attempt at the time
(following TrustedVaultClient::FetchKeys() documentation). In case of
abandoning download keys request, ongoing fetch keys fulfilled
immediately.

Bug: 1102340
Change-Id: I2413176f03b07e67a72315a549add16f5e328678
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2340977
Commit-Queue: Maksim Moskvitin <mmoskvitin@google.com>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#799624}

components/sync/trusted_vault/BUILD.gn

@@ -22,6 +22,7 @@ static_library("trusted_vault") {
   deps = [
     "//components/os_crypt",
     "//components/signin/public/identity_manager",
+    "//components/sync/base",
     "//components/sync/protocol",
     "//crypto",
     "//services/network/public/cpp:cpp",

components/sync/trusted_vault/DEPS

 include_rules = [
   "+components/os_crypt",
   "+components/signin/public",
+  "+components/sync/base",
   "+components/sync/driver",
   "+components/sync/protocol",
   "+crypto",

components/sync/trusted_vault/standalone_trusted_vault_backend.cc

@@ -59,21 +59,72 @@ void StandaloneTrustedVaultBackend::ReadDataFromDisk() {
   data_ = ReadEncryptedFile(file_path_);
 }
 
-std::vector<std::vector<uint8_t>> StandaloneTrustedVaultBackend::FetchKeys(
-    const CoreAccountInfo& account_info) {
+void StandaloneTrustedVaultBackend::FetchKeys(
+    const CoreAccountInfo& account_info,
+    FetchKeysCallback callback) {
+  // Concurrent keys fetches aren't supported.
+  DCHECK(ongoing_fetch_keys_callback_.is_null());
+  DCHECK(!callback.is_null());
+
+  ongoing_fetch_keys_callback_ = std::move(callback);
+  ongoing_fetch_keys_gaia_id_ = account_info.gaia;
+
   const sync_pb::LocalTrustedVaultPerUser* per_user_vault =
       FindUserVault(account_info.gaia);
 
-  std::vector<std::vector<uint8_t>> keys;
-  if (per_user_vault) {
-    for (const sync_pb::LocalTrustedVaultKey& key :
-         per_user_vault->vault_key()) {
-      const std::string& key_material = key.key_material();
-      keys.emplace_back(key_material.begin(), key_material.end());
-    }
+  // TODO(crbug.com/1094326): currently there is no guarantee that
+  // |syncing_account_| is set before FetchKeys() call and this may cause
+  // redundant sync error in the UI (for key retrieval), especially during the
+  // browser startup. Try to find a way to avoid this issue.
+  if (!base::FeatureList::IsEnabled(switches::kFollowTrustedVaultKeyRotation) ||
+      !syncing_account_.has_value() ||
+      syncing_account_->gaia != account_info.gaia || !per_user_vault ||
+      !per_user_vault->keys_are_stale() ||
+      !per_user_vault->local_device_registration_info().device_registered()) {
+    // Keys download attempt is not needed or not possible.
+    FulfillOngoingFetchKeys();
+    return;
   }
 
-  return keys;
+  // Current state guarantees there is no ongoing requests to the server:
+  // 1. Current |syncing_account_| is |account_info|, so there is no ongoing
+  // request for other accounts.
+  // 2. Device is already registered, so there is no device registration for
+  // |account_info|.
+  // 3. Concurrent FetchKeys() calls aren't supported, so there is no keys
+  // download for |account_info|.
+  DCHECK(!weak_factory_for_connection_.HasWeakPtrs());
+
+  std::unique_ptr<SecureBoxKeyPair> key_pair =
+      SecureBoxKeyPair::CreateByPrivateKeyImport(base::as_bytes(
+          base::make_span(per_user_vault->local_device_registration_info()
+                              .private_key_material())));
+  if (!key_pair) {
+    // Corrupted state: device is registered, but |key_pair| can't be imported.
+    // TODO(crbug.com/1094326): restore from this state (throw away the key and
+    // trigger device registration again).
+    FulfillOngoingFetchKeys();
+    return;
+  }
+  if (per_user_vault->vault_key_size() == 0) {
+    // Corrupted state: device is registered, but there is no vault keys.
+    // TODO(crbug.com/1094326): restore from this state (just mark device as not
+    // registered?).
+    FulfillOngoingFetchKeys();
+    return;
+  }
+
+  // TODO(crbug.com/1094326): add throttling mechanism.
+  std::string last_key = per_user_vault->vault_key()
+                             .at(per_user_vault->vault_key_size() - 1)
+                             .key_material();
+  std::vector<uint8_t> last_key_bytes(last_key.begin(), last_key.end());
+  connection_->DownloadKeys(
+      *syncing_account_, last_key_bytes,
+      per_user_vault->last_vault_key_version(), std::move(key_pair),
+      base::BindOnce(&StandaloneTrustedVaultBackend::OnKeysDownloaded,
+                     weak_factory_for_connection_.GetWeakPtr(),
+                     account_info.gaia));
 }
 
 void StandaloneTrustedVaultBackend::StoreKeys(
@@ -102,7 +153,7 @@ void StandaloneTrustedVaultBackend::StoreKeys(
 void StandaloneTrustedVaultBackend::RemoveAllStoredKeys() {
   base::DeleteFile(file_path_);
   data_.Clear();
-  weak_factory_for_connection_.InvalidateWeakPtrs();
+  AbandonConnectionRequest();
 }
 
 void StandaloneTrustedVaultBackend::SetSyncingAccount(
@@ -111,12 +162,26 @@ void StandaloneTrustedVaultBackend::SetSyncingAccount(
     return;
   }
   syncing_account_ = syncing_account;
-  weak_factory_for_connection_.InvalidateWeakPtrs();
+  AbandonConnectionRequest();
   if (syncing_account_.has_value()) {
     MaybeRegisterDevice(syncing_account_->gaia);
   }
 }
 
+bool StandaloneTrustedVaultBackend::MarkKeysAsStale(
+    const CoreAccountInfo& account_info) {
+  sync_pb::LocalTrustedVaultPerUser* per_user_vault =
+      FindUserVault(account_info.gaia);
+  if (!per_user_vault || per_user_vault->keys_are_stale()) {
+    // No keys available for |account_info| or they are already marked as stale.
+    return false;
+  }
+
+  per_user_vault->set_keys_are_stale(true);
+  WriteToDisk(data_, file_path_);
+  return true;
+}
+
 sync_pb::LocalDeviceRegistrationInfo
 StandaloneTrustedVaultBackend::GetDeviceRegistrationInfoForTesting(
     const std::string& gaia_id) {
@@ -180,7 +245,7 @@ void StandaloneTrustedVaultBackend::MaybeRegisterDevice(
 
   // Cancel existing callbacks passed to |connection_| to ensure there is only
   // one ongoing request.
-  weak_factory_for_connection_.InvalidateWeakPtrs();
+  AbandonConnectionRequest();
   connection_->RegisterDevice(
       *syncing_account_, last_key_bytes,
       per_user_vault->last_vault_key_version(), key_pair->public_key(),
@@ -200,8 +265,6 @@ void StandaloneTrustedVaultBackend::OnDeviceRegistered(
 
   switch (status) {
     case TrustedVaultRequestStatus::kSuccess:
-      // TODO(crbug.com/1102340): we may want to immediately fetch fresh keys
-      // (in case they were marked as stale while the request was in flight).
       per_user_vault->mutable_local_device_registration_info()
           ->set_device_registered(true);
       WriteToDisk(data_, file_path_);
@@ -217,6 +280,73 @@ void StandaloneTrustedVaultBackend::OnDeviceRegistered(
   }
 }
 
+void StandaloneTrustedVaultBackend::OnKeysDownloaded(
+    const std::string& gaia_id,
+    TrustedVaultRequestStatus status,
+    const std::vector<std::vector<uint8_t>>& vault_keys,
+    int last_vault_key_version) {
+  DCHECK(syncing_account_ && syncing_account_->gaia == gaia_id);
+  DCHECK(!ongoing_fetch_keys_callback_.is_null());
+  DCHECK(ongoing_fetch_keys_gaia_id_ == gaia_id);
+
+  sync_pb::LocalTrustedVaultPerUser* per_user_vault = FindUserVault(gaia_id);
+  DCHECK(per_user_vault);
+
+  switch (status) {
+    case TrustedVaultRequestStatus::kSuccess:
+      StoreKeys(gaia_id, vault_keys, last_vault_key_version);
+      break;
+    case TrustedVaultRequestStatus::kLocalDataObsolete: {
+      sync_pb::LocalTrustedVaultPerUser* per_user_vault =
+          FindUserVault(gaia_id);
+      // Either device isn't registered or vault keys are too outdated or
+      // corrupted. The only way to go out of this states is to receive new
+      // vault keys through external StoreKeys() call. It's safe to mark device
+      // as not registered regardless of the cause (device registration will be
+      // triggered once new vault keys are available).
+      per_user_vault->mutable_local_device_registration_info()
+          ->set_device_registered(false);
+      WriteToDisk(data_, file_path_);
+      break;
+    }
+    case TrustedVaultRequestStatus::kOtherError:
+      // TODO(crbug.com/1102340): prevent future queries until browser restart?
+      // TODO(crbug.com/1102340): introduce throttling mechanism across browser
+      // restarts?
+      break;
+  }
+  // Regardless of the |status| ongoing fetch keys request should be fulfilled.
+  FulfillOngoingFetchKeys();
+}
+
+void StandaloneTrustedVaultBackend::AbandonConnectionRequest() {
+  weak_factory_for_connection_.InvalidateWeakPtrs();
+  FulfillOngoingFetchKeys();
+}
+
+void StandaloneTrustedVaultBackend::FulfillOngoingFetchKeys() {
+  if (!ongoing_fetch_keys_gaia_id_.has_value()) {
+    return;
+  }
+  DCHECK(!ongoing_fetch_keys_callback_.is_null());
+
+  const sync_pb::LocalTrustedVaultPerUser* per_user_vault =
+      FindUserVault(*ongoing_fetch_keys_gaia_id_);
+
+  std::vector<std::vector<uint8_t>> vault_keys;
+  if (per_user_vault) {
+    for (const sync_pb::LocalTrustedVaultKey& key :
+         per_user_vault->vault_key()) {
+      const std::string& key_material = key.key_material();
+      vault_keys.emplace_back(key_material.begin(), key_material.end());
+    }
+  }
+
+  std::move(ongoing_fetch_keys_callback_).Run(vault_keys);
+  ongoing_fetch_keys_callback_.Reset();
+  ongoing_fetch_keys_gaia_id_.reset();
+}
+
 sync_pb::LocalTrustedVaultPerUser* StandaloneTrustedVaultBackend::FindUserVault(
     const std::string& gaia_id) {
   for (int i = 0; i < data_.user_size(); ++i) {

components/sync/trusted_vault/standalone_trusted_vault_backend.h

@@ -11,6 +11,7 @@
 #include <string>
 #include <vector>
 
+#include "base/callback.h"
 #include "base/files/file_path.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
@@ -27,6 +28,9 @@ namespace syncer {
 class StandaloneTrustedVaultBackend
     : public base::RefCountedThreadSafe<StandaloneTrustedVaultBackend> {
  public:
+  using FetchKeysCallback = base::OnceCallback<void(
+      const std::vector<std::vector<uint8_t>>& vault_keys)>;
+
   StandaloneTrustedVaultBackend(
       const base::FilePath& file_path,
       std::unique_ptr<TrustedVaultConnection> connection);
@@ -39,15 +43,24 @@ class StandaloneTrustedVaultBackend
   // object.
   void ReadDataFromDisk();
 
-  // Returns keys corresponding to |account_info|.
-  std::vector<std::vector<uint8_t>> FetchKeys(
-      const CoreAccountInfo& account_info);
+  // Populates vault keys corresponding to |account_info| into |callback|. If
+  // recent keys are locally available, |callback| will be called immediately.
+  // Otherwise, attempts to download new keys from the server. In case of
+  // failure or if current state isn't sufficient it will populate locally
+  // available keys regardless of their freshness.
+  // Concurrent calls are not supported.
+  void FetchKeys(const CoreAccountInfo& account_info,
+                 FetchKeysCallback callback);
 
   // Replaces keys for given |gaia_id| both in memory and in |file_path_|.
   void StoreKeys(const std::string& gaia_id,
                  const std::vector<std::vector<uint8_t>>& keys,
                  int last_key_version);
 
+  // Marks vault keys as stale.  Afterwards, the next FetchKeys() call for this
+  // |account_info| will trigger a key download attempt.
+  bool MarkKeysAsStale(const CoreAccountInfo& account_info);
+
   // Removes all keys for all accounts from both memory and |file_path_|.
   void RemoveAllStoredKeys();
 
@@ -76,6 +89,15 @@ class StandaloneTrustedVaultBackend
   void OnDeviceRegistered(const std::string& gaia_id,
                           TrustedVaultRequestStatus status);
 
+  void OnKeysDownloaded(const std::string& gaia_id,
+                        TrustedVaultRequestStatus status,
+                        const std::vector<std::vector<uint8_t>>& vault_keys,
+                        int last_vault_key_version);
+
+  void AbandonConnectionRequest();
+
+  void FulfillOngoingFetchKeys();
+
   const base::FilePath file_path_;
 
   sync_pb::LocalTrustedVault data_;
@@ -87,6 +109,12 @@ class StandaloneTrustedVaultBackend
   // Used for communication with trusted vault server.
   std::unique_ptr<TrustedVaultConnection> connection_;
 
+  // Used to plumb FetchKeys() result to the caller.
+  FetchKeysCallback ongoing_fetch_keys_callback_;
+
+  // Account used in last FetchKeys() call.
+  base::Optional<std::string> ongoing_fetch_keys_gaia_id_;
+
   // Used for cancellation of callbacks passed to |connection_|.
   base::WeakPtrFactory<StandaloneTrustedVaultBackend>
       weak_factory_for_connection_{this};

components/sync/trusted_vault/standalone_trusted_vault_backend_unittest.cc

@@ -11,6 +11,7 @@
 
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
+#include "base/test/bind_test_util.h"
 #include "base/test/scoped_feature_list.h"
 #include "components/os_crypt/os_crypt_mocker.h"
 #include "components/sync/driver/sync_driver_switches.h"
@@ -25,6 +26,8 @@ namespace {
 
 using testing::_;
 using testing::Eq;
+using testing::IsEmpty;
+using testing::NotNull;
 
 base::FilePath CreateUniqueTempDir(base::ScopedTempDir* temp_dir) {
   EXPECT_TRUE(temp_dir->CreateUniqueTempDir());
@@ -75,6 +78,42 @@ class StandaloneTrustedVaultBackendTest : public testing::Test {
 
   StandaloneTrustedVaultBackend* backend() { return backend_.get(); }
 
+  // Stores |vault_keys| and mimics successful device registration, returns
+  // private device key material.
+  std::vector<uint8_t> StoreKeysAndMimicDeviceRegistration(
+      const std::vector<std::vector<uint8_t>>& vault_keys,
+      int last_vault_key_version,
+      CoreAccountInfo account_info) {
+    DCHECK(!vault_keys.empty());
+    backend_->StoreKeys(account_info.gaia, vault_keys, last_vault_key_version);
+    TrustedVaultConnection::RegisterDeviceCallback device_registration_callback;
+
+    EXPECT_CALL(*connection_,
+                RegisterDevice(Eq(account_info), Eq(vault_keys.back()),
+                               Eq(last_vault_key_version), _, _))
+        .WillOnce([&](const CoreAccountInfo&, const std::vector<uint8_t>&, int,
+                      const SecureBoxPublicKey& device_public_key,
+                      TrustedVaultConnection::RegisterDeviceCallback callback) {
+          device_registration_callback = std::move(callback);
+        });
+    // Setting the syncing account will trigger device registration.
+    backend()->SetSyncingAccount(account_info);
+    EXPECT_FALSE(device_registration_callback.is_null());
+
+    // Pretend that the registration completed successfully.
+    std::move(device_registration_callback)
+        .Run(TrustedVaultRequestStatus::kSuccess);
+
+    // Reset syncing account.
+    backend()->SetSyncingAccount(base::nullopt);
+
+    std::string device_private_key_material =
+        backend_->GetDeviceRegistrationInfoForTesting(account_info.gaia)
+            .private_key_material();
+    return std::vector<uint8_t>(device_private_key_material.begin(),
+                                device_private_key_material.end());
+  }
+
  private:
   base::test::ScopedFeatureList override_features;
 
@@ -88,7 +127,7 @@ TEST_F(StandaloneTrustedVaultBackendTest, ShouldRegisterDevice) {
   CoreAccountInfo account_info;
   account_info.gaia = "user";
 
-  const std::vector<uint8_t> kVaultKey = {{1, 2, 3}};
+  const std::vector<uint8_t> kVaultKey = {1, 2, 3};
   const int kLastKeyVersion = 0;
 
   backend()->StoreKeys(account_info.gaia, {kVaultKey}, kLastKeyVersion);
@@ -125,6 +164,86 @@ TEST_F(StandaloneTrustedVaultBackendTest, ShouldRegisterDevice) {
               Eq(serialized_public_device_key));
 }
 
+// Unless keys marked as stale, FetchKeys() should be completed immediately,
+// without keys download attempt.
+TEST_F(StandaloneTrustedVaultBackendTest, ShouldFetchKeysImmediately) {
+  CoreAccountInfo account_info;
+  account_info.gaia = "user";
+
+  const std::vector<std::vector<uint8_t>> kVaultKeys = {{1, 2, 3}};
+  const int kLastKeyVersion = 0;
+
+  // Make keys downloading theoretically possible.
+  StoreKeysAndMimicDeviceRegistration(kVaultKeys, kLastKeyVersion,
+                                      account_info);
+  backend()->SetSyncingAccount(account_info);
+
+  EXPECT_CALL(*connection(), DownloadKeys(_, _, _, _, _)).Times(0);
+
+  std::vector<std::vector<uint8_t>> fetched_keys;
+  // Callback should be called immediately.
+  backend()->FetchKeys(account_info,
+                       base::BindLambdaForTesting(
+                           [&](const std::vector<std::vector<uint8_t>>& keys) {
+                             fetched_keys = keys;
+                           }));
+
+  EXPECT_THAT(fetched_keys, Eq(kVaultKeys));
+}
+
+TEST_F(StandaloneTrustedVaultBackendTest, ShouldDownloadKeys) {
+  CoreAccountInfo account_info;
+  account_info.gaia = "user";
+
+  const std::vector<uint8_t> kInitialVaultKey = {1, 2, 3};
+  const int kInitialLastKeyVersion = 0;
+
+  std::vector<uint8_t> private_device_key_material =
+      StoreKeysAndMimicDeviceRegistration({kInitialVaultKey},
+                                          kInitialLastKeyVersion, account_info);
+  EXPECT_TRUE(backend()->MarkKeysAsStale(account_info));
+  backend()->SetSyncingAccount(account_info);
+
+  const std::vector<std::vector<uint8_t>> kNewVaultKeys = {kInitialVaultKey,
+                                                           {1, 3, 2}};
+  const int kNewLastKeyVersion = 1;
+
+  std::unique_ptr<SecureBoxKeyPair> device_key_pair;
+  TrustedVaultConnection::DownloadKeysCallback download_keys_callback;
+  EXPECT_CALL(*connection(),
+              DownloadKeys(Eq(account_info), Eq(kInitialVaultKey),
+                           Eq(kInitialLastKeyVersion), _, _))
+      .WillOnce([&](const CoreAccountInfo&, const std::vector<uint8_t>&, int,
+                    std::unique_ptr<SecureBoxKeyPair> key_pair,
+                    TrustedVaultConnection::DownloadKeysCallback callback) {
+        device_key_pair = std::move(key_pair);
+        download_keys_callback = std::move(callback);
+      });
+
+  std::vector<std::vector<uint8_t>> fetched_keys;
+  // FetchKeys() should trigger keys downloading.
+  backend()->FetchKeys(account_info,
+                       base::BindLambdaForTesting(
+                           [&](const std::vector<std::vector<uint8_t>>& keys) {
+                             fetched_keys = keys;
+                           }));
+  ASSERT_FALSE(download_keys_callback.is_null());
+  ASSERT_THAT(fetched_keys, IsEmpty());
+
+  // Ensure that the right device key was passed into DonwloadKeys().
+  ASSERT_THAT(device_key_pair, NotNull());
+  EXPECT_THAT(device_key_pair->private_key().ExportToBytes(),
+              Eq(private_device_key_material));
+
+  // Mimic successful key downloading.
+  std::move(download_keys_callback)
+      .Run(TrustedVaultRequestStatus::kSuccess, kNewVaultKeys,
+           kNewLastKeyVersion);
+
+  // Now fetch keys attempt should be completed.
+  EXPECT_THAT(fetched_keys, Eq(kNewVaultKeys));
+}
+
 }  // namespace
 
 }  // namespace syncer

components/sync/trusted_vault/standalone_trusted_vault_client.cc

@@ -13,6 +13,7 @@
 #include "base/task/thread_pool.h"
 #include "base/task_runner_util.h"
 #include "components/signin/public/identity_manager/account_info.h"
+#include "components/sync/base/bind_to_task_runner.h"
 #include "components/sync/trusted_vault/standalone_trusted_vault_backend.h"
 #include "components/sync/trusted_vault/trusted_vault_access_token_fetcher.h"
 #include "components/sync/trusted_vault/trusted_vault_connection_impl.h"
@@ -46,11 +47,10 @@ void StandaloneTrustedVaultClient::FetchKeys(
     const CoreAccountInfo& account_info,
     base::OnceCallback<void(const std::vector<std::vector<uint8_t>>&)> cb) {
   TriggerLazyInitializationIfNeeded();
-  base::PostTaskAndReplyWithResult(
-      backend_task_runner_.get(), FROM_HERE,
+  backend_task_runner_->PostTask(
+      FROM_HERE,
       base::BindOnce(&StandaloneTrustedVaultBackend::FetchKeys, backend_,
-                     account_info),
-      std::move(cb));
+                     account_info, BindToCurrentSequence(std::move(cb))));
 }
 
 void StandaloneTrustedVaultClient::StoreKeys(
@@ -76,8 +76,12 @@ void StandaloneTrustedVaultClient::RemoveAllStoredKeys() {
 void StandaloneTrustedVaultClient::MarkKeysAsStale(
     const CoreAccountInfo& account_info,
     base::OnceCallback<void(bool)> cb) {
-  // Not really supported and not useful for this particular implementation.
-  std::move(cb).Run(false);
+  TriggerLazyInitializationIfNeeded();
+  base::PostTaskAndReplyWithResult(
+      backend_task_runner_.get(), FROM_HERE,
+      base::BindOnce(&StandaloneTrustedVaultBackend::MarkKeysAsStale, backend_,
+                     account_info),
+      std::move(cb));
 }
 
 void StandaloneTrustedVaultClient::GetIsRecoverabilityDegraded(