[TrustedVault] Backend: support registration with unknown epoch

This CL adds logic in StandaloneTrustedVaultBackend to support device
registration before vault keys are available; it also allows to follow
very first key rotation, when there are no vault keys locally available,
but device was registered.

Bug: 1094326
Change-Id: If238944a15db550938fc48b9acf3e37bbd7f0e18
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2624650
Commit-Queue: Maksim Moskvitin <mmoskvitin@google.com>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#843718}

components/sync/driver/sync_driver_switches.cc

@@ -63,6 +63,13 @@ const base::Feature kDecoupleSyncFromAndroidMasterSync{
 const base::Feature kFollowTrustedVaultKeyRotation{
     "FollowTrustedVaultKeyRotation", base::FEATURE_DISABLED_BY_DEFAULT};
 
+// Allows device registration within trusted vault server without having trusted
+// vault key. Effectively disabled if kFollowTrustedVaultKeyRotation is
+// disabled.
+const base::Feature kAllowSilentTrustedVaultDeviceRegistration{
+    "AllowSilentTrustedVaultDeviceRegistration",
+    base::FEATURE_ENABLED_BY_DEFAULT};
+
 // Specifies how long requests to vault service shouldn't be retried after
 // encountering transient error.
 const base::FeatureParam<base::TimeDelta>

components/sync/driver/sync_driver_switches.h

@@ -31,6 +31,7 @@ extern const base::Feature kSyncAutofillWalletOfferData;
 extern const base::Feature kSyncWifiConfigurations;
 extern const base::Feature kDecoupleSyncFromAndroidMasterSync;
 extern const base::Feature kFollowTrustedVaultKeyRotation;
+extern const base::Feature kAllowSilentTrustedVaultDeviceRegistration;
 extern const base::FeatureParam<base::TimeDelta>
     kTrustedVaultServiceThrottlingDuration;
 

components/sync/trusted_vault/standalone_trusted_vault_backend.cc

@@ -95,12 +95,17 @@ void StandaloneTrustedVaultBackend::FetchKeys(
   // |primary_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 (!connection_ || !primary_account_.has_value() ||
-      primary_account_->gaia != account_info.gaia || !per_user_vault ||
-      !per_user_vault->keys_are_stale() ||
+  if (!connection_ || !primary_account_ || !per_user_vault ||
       !per_user_vault->local_device_registration_info().device_registered() ||
       AreConnectionRequestsThrottled(account_info.gaia)) {
-    // Keys download attempt is not needed or not possible.
+    // Keys download attempt is not possible.
+    FulfillOngoingFetchKeys();
+    return;
+  }
+  if (per_user_vault->vault_key_size() != 0 &&
+      !per_user_vault->keys_are_stale()) {
+    // There are locally available keys, which weren't marked as stale. Keys
+    // download attempt is not needed.
     FulfillOngoingFetchKeys();
     return;
   }
@@ -125,19 +130,11 @@ void StandaloneTrustedVaultBackend::FetchKeys(
     FulfillOngoingFetchKeys();
     return;
   }
-  base::Optional<TrustedVaultKeyAndVersion> last_trusted_vault_key_and_version =
-      GetLastTrustedVaultKeyAndVersion(*per_user_vault);
-  if (!last_trusted_vault_key_and_version.has_value()) {
-    // TODO(crbug.com/1094326): properly support this state (constant key case).
-    FulfillOngoingFetchKeys();
-    NOTIMPLEMENTED();
-    return;
-  }
 
   // |this| outlives |connection_| and |ongoing_connection_request_|, so it's
   // safe to use base::Unretained() here.
   ongoing_connection_request_ = connection_->DownloadKeys(
-      *primary_account_, *last_trusted_vault_key_and_version,
+      *primary_account_, GetLastTrustedVaultKeyAndVersion(*per_user_vault),
       std::move(key_pair),
       base::BindOnce(&StandaloneTrustedVaultBackend::OnKeysDownloaded,
                      base::Unretained(this), account_info.gaia));
@@ -181,9 +178,17 @@ void StandaloneTrustedVaultBackend::SetPrimaryAccount(
   }
   primary_account_ = primary_account;
   AbandonConnectionRequest();
-  if (primary_account_.has_value()) {
-    MaybeRegisterDevice(primary_account_->gaia);
+  if (!primary_account_.has_value()) {
+    return;
+  }
+
+  sync_pb::LocalTrustedVaultPerUser* per_user_vault =
+      FindUserVault(primary_account->gaia);
+  if (!per_user_vault) {
+    per_user_vault = data_.add_user();
+    per_user_vault->set_gaia_id(primary_account->gaia);
   }
+  MaybeRegisterDevice(primary_account_->gaia);
 }
 
 bool StandaloneTrustedVaultBackend::MarkKeysAsStale(
@@ -258,20 +263,23 @@ void StandaloneTrustedVaultBackend::MaybeRegisterDevice(
     // Device registration is supported only for |primary_account_|.
     return;
   }
+
+  // |per_user_vault| must be created before calling this function.
   sync_pb::LocalTrustedVaultPerUser* per_user_vault = FindUserVault(gaia_id);
-  if (!per_user_vault) {
-    // TODO(crbug.com/1102340): make non-null |per_user_vault| a precondition
-    // for this function?
-    return;
-  }
+  DCHECK(per_user_vault);
+
   base::Optional<TrustedVaultKeyAndVersion> last_trusted_vault_key_and_version =
       GetLastTrustedVaultKeyAndVersion(*per_user_vault);
-  if (!last_trusted_vault_key_and_version.has_value() ||
-      per_user_vault->keys_are_stale()) {
-    // Fresh vault key is required to register the device.
-    // TODO(crbug.com/1102340): relax this condition to support device
-    // registration without real trusted vault key.
-    NOTIMPLEMENTED();
+  if (!last_trusted_vault_key_and_version.has_value() &&
+      !base::FeatureList::IsEnabled(
+          switches::kAllowSilentTrustedVaultDeviceRegistration)) {
+    // Either vault keys should be available or registration without them should
+    // be allowed through feature flag.
+    return;
+  }
+  if (per_user_vault->keys_are_stale()) {
+    // Client already knows that existing vault keys (or their absence) isn't
+    // sufficient for device registration. Fresh keys should be obtained first.
     return;
   }
   if (per_user_vault->local_device_registration_info().device_registered()) {
@@ -313,7 +321,7 @@ void StandaloneTrustedVaultBackend::MaybeRegisterDevice(
   // |this| outlives |connection_| and |ongoing_connection_request_|, so it's
   // safe to use base::Unretained() here.
   ongoing_connection_request_ = connection_->RegisterAuthenticationFactor(
-      *primary_account_, *last_trusted_vault_key_and_version,
+      *primary_account_, last_trusted_vault_key_and_version,
       key_pair->public_key(),
       base::BindOnce(&StandaloneTrustedVaultBackend::OnDeviceRegistered,
                      base::Unretained(this), gaia_id));

components/sync/trusted_vault/standalone_trusted_vault_backend.h

@@ -120,7 +120,8 @@ class StandaloneTrustedVaultBackend
   void MaybeRegisterDevice(const std::string& gaia_id);
 
   // Called when device registration for |gaia_id| is completed (either
-  // successfully or not).
+  // successfully or not). |data_| must contain LocalTrustedVaultPerUser for
+  // given |gaia_id|.
   void OnDeviceRegistered(const std::string& gaia_id,
                           TrustedVaultRequestStatus status);
 

components/sync/trusted_vault/standalone_trusted_vault_backend_unittest.cc

@@ -592,6 +592,74 @@ TEST_F(StandaloneTrustedVaultBackendTest,
   EXPECT_FALSE(download_keys_callback.is_null());
 }
 
+// Tests silent device registration (when no vault keys available yet). After
+// successful registration, the client should be able to download keys.
+TEST_F(StandaloneTrustedVaultBackendTest,
+       ShouldSilentlyRegisterDeviceAndDownloadKeys) {
+  CoreAccountInfo account_info;
+  account_info.gaia = "user";
+
+  TrustedVaultConnection::RegisterAuthenticationFactorCallback
+      device_registration_callback;
+  EXPECT_CALL(
+      *connection(),
+      RegisterAuthenticationFactor(
+          account_info,
+          /*last_trusted_vault_key_and_version=*/Eq(base::nullopt), _, _))
+      .WillOnce([&](const CoreAccountInfo&,
+                    const base::Optional<TrustedVaultKeyAndVersion>&,
+                    const SecureBoxPublicKey& device_public_key,
+                    TrustedVaultConnection::RegisterAuthenticationFactorCallback
+                        callback) {
+        device_registration_callback = std::move(callback);
+        return std::make_unique<TrustedVaultConnection::Request>();
+      });
+
+  // Setting the primary account will trigger device registration.
+  backend()->SetPrimaryAccount(account_info);
+  ASSERT_FALSE(device_registration_callback.is_null());
+
+  // Pretend that the registration completed successfully.
+  std::move(device_registration_callback)
+      .Run(TrustedVaultRequestStatus::kSuccess);
+
+  // Now the device should be registered.
+  sync_pb::LocalDeviceRegistrationInfo registration_info =
+      backend()->GetDeviceRegistrationInfoForTesting(account_info.gaia);
+  EXPECT_TRUE(registration_info.device_registered());
+  EXPECT_TRUE(registration_info.has_private_key_material());
+
+  TrustedVaultConnection::DownloadKeysCallback download_keys_callback;
+  ON_CALL(*connection(),
+          DownloadKeys(account_info,
+                       /*last_trusted_vault_key_and_version=*/Eq(base::nullopt),
+                       _, _))
+      .WillByDefault(
+          [&](const CoreAccountInfo&,
+              const base::Optional<TrustedVaultKeyAndVersion>&,
+              std::unique_ptr<SecureBoxKeyPair> key_pair,
+              TrustedVaultConnection::DownloadKeysCallback callback) {
+            download_keys_callback = std::move(callback);
+            return std::make_unique<TrustedVaultConnection::Request>();
+          });
+
+  // FetchKeys() should trigger keys downloading. Note: unlike tests with
+  // following regular key rotation, in this case MarkKeysAsStale() isn't
+  // called intentionally.
+  base::MockCallback<StandaloneTrustedVaultBackend::FetchKeysCallback>
+      fetch_keys_callback;
+  backend()->FetchKeys(account_info, fetch_keys_callback.Get());
+  ASSERT_FALSE(download_keys_callback.is_null());
+
+  // Mimic successful key downloading, it should make fetch keys attempt
+  // completed.
+  const std::vector<std::vector<uint8_t>> kNewVaultKeys = {{1, 2, 3}};
+  EXPECT_CALL(fetch_keys_callback, Run(/*keys=*/kNewVaultKeys));
+  std::move(download_keys_callback)
+      .Run(TrustedVaultRequestStatus::kSuccess, kNewVaultKeys,
+           /*last_key_version=*/40);
+}
+
 }  // namespace
 
 }  // namespace syncer