Introduce sync TrustedVaultClient

The abstraction allows injecting platform-specific logic about
retrieving locally known encryption keys, as well as the optional
ability to store them (independently of sign-out state or sync engine
initialization state).

The API is designed to be asynchronous in order to support IO or IPCs.

SyncServiceCrypto is extended with an intermediate state where keys
are being fetched, before actually prompting the user for action.

Bug: 1012659, 1012660
Change-Id: Ia8daa317a4fbc1cbb50be7e2c0db40a1705f8b66
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1849381
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704644}

chrome/browser/sync/chrome_sync_client.cc

@@ -415,6 +415,17 @@ BookmarkUndoService* ChromeSyncClient::GetBookmarkUndoService() {
   return BookmarkUndoServiceFactory::GetForProfile(profile_);
 }
 
+syncer::TrustedVaultClient* ChromeSyncClient::GetTrustedVaultClient() {
+#if defined(OS_ANDROID)
+  // TODO(crbug.com/1012659): Instantiate a client for Android.
+  NOTIMPLEMENTED();
+#else
+  // TODO(crbug.com/1012660): Instantiate a generic client for other platforms.
+  NOTIMPLEMENTED();
+#endif  // defined(OS_ANDROID)
+  return nullptr;
+}
+
 invalidation::InvalidationService* ChromeSyncClient::GetInvalidationService() {
   invalidation::ProfileInvalidationProvider* provider =
       invalidation::ProfileInvalidationProviderFactory::GetForProfile(profile_);

chrome/browser/sync/chrome_sync_client.h

@@ -51,6 +51,7 @@ class ChromeSyncClient : public browser_sync::BrowserSyncClient {
   base::Closure GetPasswordStateChangedCallback() override;
   syncer::DataTypeController::TypeVector CreateDataTypeControllers(
       syncer::SyncService* sync_service) override;
+  syncer::TrustedVaultClient* GetTrustedVaultClient() override;
   invalidation::InvalidationService* GetInvalidationService() override;
   BookmarkUndoService* GetBookmarkUndoService() override;
   scoped_refptr<syncer::ExtensionsActivity> GetExtensionsActivity() override;

components/sync/BUILD.gn

@@ -590,6 +590,7 @@ source_set("unit_tests") {
     "driver/shared_change_processor_unittest.cc",
     "driver/startup_controller_unittest.cc",
     "driver/sync_auth_manager_unittest.cc",
+    "driver/sync_service_crypto_unittest.cc",
     "driver/sync_service_utils_unittest.cc",
     "driver/sync_session_durations_metrics_recorder_unittest.cc",
     "driver/sync_stopped_reporter_unittest.cc",

components/sync/driver/BUILD.gn

@@ -90,6 +90,7 @@ jumbo_static_library("driver") {
     "sync_util.h",
     "syncable_service_based_model_type_controller.cc",
     "syncable_service_based_model_type_controller.h",
+    "trusted_vault_client.h",
   ]
 
   configs += [ "//build/config:precompiled_headers" ]

components/sync/driver/glue/sync_engine_impl.cc

@@ -120,12 +120,14 @@ void SyncEngineImpl::SetDecryptionPassphrase(const std::string& passphrase) {
 }
 
 void SyncEngineImpl::AddTrustedVaultDecryptionKeys(
-    const std::vector<std::string>& keys) {
+    const std::vector<std::string>& keys,
+    base::OnceClosure done_cb) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  sync_task_runner_->PostTask(
+  sync_task_runner_->PostTaskAndReply(
       FROM_HERE,
       base::BindOnce(&SyncEngineBackend::DoAddTrustedVaultDecryptionKeys,
-                     backend_, keys));
+                     backend_, keys),
+      std::move(done_cb));
 }
 
 void SyncEngineImpl::StopSyncingForShutdown() {

components/sync/driver/glue/sync_engine_impl.h

@@ -64,8 +64,8 @@ class SyncEngineImpl : public SyncEngine, public InvalidationHandler {
   void StartSyncingWithServer() override;
   void SetEncryptionPassphrase(const std::string& passphrase) override;
   void SetDecryptionPassphrase(const std::string& passphrase) override;
-  void AddTrustedVaultDecryptionKeys(
-      const std::vector<std::string>& keys) override;
+  void AddTrustedVaultDecryptionKeys(const std::vector<std::string>& keys,
+                                     base::OnceClosure done_cb) override;
   void StopSyncingForShutdown() override;
   void Shutdown(ShutdownReason reason) override;
   void ConfigureDataTypes(ConfigureParams params) override;

components/sync/driver/profile_sync_service.cc

@@ -149,7 +149,8 @@ ProfileSyncService::ProfileSyncService(InitParams init_params)
                               base::Unretained(this)),
           base::BindRepeating(&ProfileSyncService::ReconfigureDueToPassphrase,
                               base::Unretained(this)),
-          &sync_prefs_),
+          &sync_prefs_,
+          sync_client_->GetTrustedVaultClient()),
       network_time_update_callback_(
           std::move(init_params.network_time_update_callback)),
       url_loader_factory_(std::move(init_params.url_loader_factory)),

components/sync/driver/sync_client.h

@@ -27,6 +27,7 @@ class SyncApiComponentFactory;
 class SyncableService;
 class SyncService;
 class SyncTypePreferenceProvider;
+class TrustedVaultClient;
 
 // Interface for clients of the Sync API to plumb through necessary dependent
 // components. This interface is purely for abstracting dependencies, and
@@ -53,6 +54,7 @@ class SyncClient {
       SyncService* sync_service) = 0;
 
   virtual invalidation::InvalidationService* GetInvalidationService() = 0;
+  virtual TrustedVaultClient* GetTrustedVaultClient() = 0;
   virtual scoped_refptr<ExtensionsActivity> GetExtensionsActivity() = 0;
 
   // Returns a weak pointer to the syncable service specified by |type|.

components/sync/driver/sync_client_mock.h

@@ -24,6 +24,7 @@ class SyncClientMock : public SyncClient {
   MOCK_METHOD0(GetPasswordStateChangedCallback, base::RepeatingClosure());
 
   MOCK_METHOD0(GetInvalidationService, invalidation::InvalidationService*());
+  MOCK_METHOD0(GetTrustedVaultClient, TrustedVaultClient*());
   MOCK_METHOD0(GetExtensionsActivity, scoped_refptr<ExtensionsActivity>());
   MOCK_METHOD1(GetSyncableServiceForType,
                base::WeakPtr<SyncableService>(ModelType type));

components/sync/driver/sync_service_crypto.cc

@@ -7,14 +7,17 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/feature_list.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/no_destructor.h"
 #include "base/sequenced_task_runner.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "components/sync/base/passphrase_enums.h"
 #include "components/sync/base/sync_prefs.h"
 #include "components/sync/driver/sync_driver_switches.h"
 #include "components/sync/driver/sync_service.h"
+#include "components/sync/driver/trusted_vault_client.h"
 #include "components/sync/engine/sync_string_conversions.h"
 #include "components/sync/nigori/nigori.h"
 
@@ -22,6 +25,23 @@ namespace syncer {
 
 namespace {
 
+// Used for the case where a null client is passed to SyncServiceCrypto.
+class EmptyTrustedVaultClient : public TrustedVaultClient {
+ public:
+  EmptyTrustedVaultClient() = default;
+  ~EmptyTrustedVaultClient() override = default;
+
+  // TrustedVaultClient implementatio.
+  void FetchKeys(
+      const CoreAccountId& account_id,
+      base::OnceCallback<void(const std::vector<std::string>&)> cb) override {
+    std::move(cb).Run({});
+  }
+
+  void StoreKeys(const CoreAccountId& account_id,
+                 const std::vector<std::string>& keys) override {}
+};
+
 // A SyncEncryptionHandler::Observer implementation that simply posts all calls
 // to another task runner.
 class SyncEncryptionObserverProxy : public SyncEncryptionHandler::Observer {
@@ -112,6 +132,15 @@ class SyncEncryptionObserverProxy : public SyncEncryptionHandler::Observer {
   scoped_refptr<base::SequencedTaskRunner> task_runner_;
 };
 
+TrustedVaultClient* ResoveNullClient(TrustedVaultClient* client) {
+  if (client) {
+    return client;
+  }
+
+  static base::NoDestructor<EmptyTrustedVaultClient> empty_client;
+  return empty_client.get();
+}
+
 // Checks if |passphrase| can be used to decrypt the given pending keys. Returns
 // true if decryption was successful. Returns false otherwise. Must be called
 // with non-empty pending keys cache.
@@ -147,13 +176,16 @@ SyncServiceCrypto::State::~State() = default;
 SyncServiceCrypto::SyncServiceCrypto(
     const base::RepeatingClosure& notify_observers,
     const base::RepeatingCallback<void(ConfigureReason)>& reconfigure,
-    CryptoSyncPrefs* sync_prefs)
+    CryptoSyncPrefs* sync_prefs,
+    TrustedVaultClient* trusted_vault_client)
     : notify_observers_(notify_observers),
       reconfigure_(reconfigure),
-      sync_prefs_(sync_prefs) {
+      sync_prefs_(sync_prefs),
+      trusted_vault_client_(ResoveNullClient(trusted_vault_client)) {
   DCHECK(notify_observers_);
   DCHECK(reconfigure_);
   DCHECK(sync_prefs_);
+  DCHECK(trusted_vault_client_);
 }
 
 SyncServiceCrypto::~SyncServiceCrypto() = default;
@@ -172,6 +204,7 @@ bool SyncServiceCrypto::IsPassphraseRequired() const {
 
   switch (state_.required_user_action) {
     case RequiredUserAction::kNone:
+    case RequiredUserAction::kFetchingTrustedVaultKeys:
     case RequiredUserAction::kTrustedVaultKeyRequired:
       return false;
     case RequiredUserAction::kPassphraseRequiredForDecryption:
@@ -286,8 +319,10 @@ bool SyncServiceCrypto::SetDecryptionPassphrase(const std::string& passphrase) {
 void SyncServiceCrypto::AddTrustedVaultDecryptionKeys(
     const CoreAccountId& account_id,
     const std::vector<std::string>& keys) {
+  trusted_vault_client_->StoreKeys(account_id, keys);
+
   if (state_.engine && state_.account_id == account_id) {
-    state_.engine->AddTrustedVaultDecryptionKeys(keys);
+    state_.engine->AddTrustedVaultDecryptionKeys(keys, base::DoNothing());
   }
 }
 
@@ -360,19 +395,25 @@ void SyncServiceCrypto::OnTrustedVaultKeyRequired() {
     return;
   }
 
-  state_.required_user_action = RequiredUserAction::kTrustedVaultKeyRequired;
+  state_.required_user_action = RequiredUserAction::kFetchingTrustedVaultKeys;
 
-  // Reconfigure without the encrypted types (excluded implicitly via the
-  // failed datatypes handler).
-  reconfigure_.Run(CONFIGURE_REASON_CRYPTO);
+  trusted_vault_client_->FetchKeys(
+      state_.account_id,
+      base::BindOnce(&SyncServiceCrypto::TrustedVaultKeysFetched,
+                     weak_factory_.GetWeakPtr()));
 }
 
 void SyncServiceCrypto::OnTrustedVaultKeyAccepted() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  if (state_.required_user_action !=
-      RequiredUserAction::kTrustedVaultKeyRequired) {
-    return;
+  switch (state_.required_user_action) {
+    case RequiredUserAction::kNone:
+    case RequiredUserAction::kPassphraseRequiredForDecryption:
+    case RequiredUserAction::kPassphraseRequiredForEncryption:
+      return;
+    case RequiredUserAction::kFetchingTrustedVaultKeys:
+    case RequiredUserAction::kTrustedVaultKeyRequired:
+      break;
   }
 
   state_.required_user_action = RequiredUserAction::kNone;
@@ -450,4 +491,29 @@ SyncServiceCrypto::GetEncryptionObserverProxy() {
       weak_factory_.GetWeakPtr(), base::SequencedTaskRunnerHandle::Get());
 }
 
+void SyncServiceCrypto::TrustedVaultKeysFetched(
+    const std::vector<std::string>& keys) {
+  // The engine could have been shut down while keys were being fetched.
+  if (!state_.engine) {
+    return;
+  }
+
+  state_.engine->AddTrustedVaultDecryptionKeys(
+      keys, base::BindOnce(&SyncServiceCrypto::TrustedVaultKeysAdded,
+                           weak_factory_.GetWeakPtr()));
+}
+
+void SyncServiceCrypto::TrustedVaultKeysAdded() {
+  if (state_.required_user_action !=
+      RequiredUserAction::kFetchingTrustedVaultKeys) {
+    return;
+  }
+
+  // Reaching this codepath indicates OnTrustedVaultKeyAccepted() was not
+  // triggered, so reconfigure without the encrypted types (excluded implicitly
+  // via the failed datatypes handler).
+  state_.required_user_action = RequiredUserAction::kTrustedVaultKeyRequired;
+  reconfigure_.Run(CONFIGURE_REASON_CRYPTO);
+}
+
 }  // namespace syncer

components/sync/driver/sync_service_crypto.h

@@ -21,16 +21,21 @@
 namespace syncer {
 
 class CryptoSyncPrefs;
+class TrustedVaultClient;
 
 // This class functions as mostly independent component of SyncService that
 // handles things related to encryption, including holding lots of state and
 // encryption communications with the sync thread.
 class SyncServiceCrypto : public SyncEncryptionHandler::Observer {
  public:
+  // |sync_prefs| must not be null and must outlive this object.
+  // |trusted_vault_client| may be null, but if non-null, the pointee must
+  // outlive this object.
   SyncServiceCrypto(
       const base::RepeatingClosure& notify_observers,
       const base::RepeatingCallback<void(ConfigureReason)>& reconfigure,
-      CryptoSyncPrefs* sync_prefs);
+      CryptoSyncPrefs* sync_prefs,
+      TrustedVaultClient* trusted_vault_client);
   ~SyncServiceCrypto() override;
 
   void Reset();
@@ -84,9 +89,19 @@ class SyncServiceCrypto : public SyncEncryptionHandler::Observer {
     kNone,
     kPassphraseRequiredForDecryption,
     kPassphraseRequiredForEncryption,
+    // Trusted vault keys are required but a silent attempt to fetch keys is in
+    // progress before prompting the user.
+    kFetchingTrustedVaultKeys,
+    // Silent attempt is completed and user action is definitely required to
+    // retrieve trusted vault keys.
     kTrustedVaultKeyRequired,
   };
 
+  // Called at various stages of asynchronously fetching and processing trusted
+  // vault encryption keys.
+  void TrustedVaultKeysFetched(const std::vector<std::string>& keys);
+  void TrustedVaultKeysAdded();
+
   // Calls SyncServiceBase::NotifyObservers(). Never null.
   const base::RepeatingClosure notify_observers_;
 
@@ -96,6 +111,9 @@ class SyncServiceCrypto : public SyncEncryptionHandler::Observer {
   // outlive us.
   CryptoSyncPrefs* const sync_prefs_;
 
+  // Never null and guaranteed to outlive us.
+  TrustedVaultClient* const trusted_vault_client_;
+
   // All the mutable state is wrapped in a struct so that it can be easily
   // reset to its default values.
   struct State {

components/sync/driver/sync_service_crypto_unittest.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/sync/driver/sync_service_crypto.h"
+
+#include <utility>
+
+#include "base/bind_helpers.h"
+#include "base/run_loop.h"
+#include "base/test/mock_callback.h"
+#include "components/sync/base/sync_prefs.h"
+#include "components/sync/driver/trusted_vault_client.h"
+#include "components/sync/engine/mock_sync_engine.h"
+#include "components/sync/nigori/nigori.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace syncer {
+
+namespace {
+
+using testing::_;
+
+sync_pb::EncryptedData MakeEncryptedData(
+    const std::string& passphrase,
+    const KeyDerivationParams& derivation_params) {
+  std::unique_ptr<Nigori> nigori =
+      Nigori::CreateByDerivation(derivation_params, passphrase);
+
+  std::string nigori_name;
+  EXPECT_TRUE(
+      nigori->Permute(Nigori::Type::Password, kNigoriKeyName, &nigori_name));
+
+  const std::string unencrypted = "test";
+  sync_pb::EncryptedData encrypted;
+  encrypted.set_key_name(nigori_name);
+  EXPECT_TRUE(nigori->Encrypt(unencrypted, encrypted.mutable_blob()));
+  return encrypted;
+}
+
+class MockCryptoSyncPrefs : public CryptoSyncPrefs {
+ public:
+  MockCryptoSyncPrefs() = default;
+  ~MockCryptoSyncPrefs() override = default;
+
+  MOCK_CONST_METHOD0(GetEncryptionBootstrapToken, std::string());
+  MOCK_METHOD1(SetEncryptionBootstrapToken, void(const std::string&));
+  MOCK_CONST_METHOD0(GetKeystoreEncryptionBootstrapToken, std::string());
+  MOCK_METHOD1(SetKeystoreEncryptionBootstrapToken, void(const std::string&));
+};
+
+class MockTrustedVaultClient : public TrustedVaultClient {
+ public:
+  MockTrustedVaultClient() = default;
+  ~MockTrustedVaultClient() override = default;
+
+  MOCK_METHOD2(
+      FetchKeys,
+      void(const CoreAccountId& account_id,
+           base::OnceCallback<void(const std::vector<std::string>&)> cb));
+  MOCK_METHOD2(StoreKeys,
+               void(const CoreAccountId& account_id,
+                    const std::vector<std::string>& keys));
+};
+
+class SyncServiceCryptoTest : public testing::Test {
+ protected:
+  SyncServiceCryptoTest()
+      : crypto_(notify_observers_cb_.Get(),
+                reconfigure_cb_.Get(),
+                &prefs_,
+                &trusted_vault_client_) {}
+
+  ~SyncServiceCryptoTest() override = default;
+
+  bool VerifyAndClearExpectations() {
+    return testing::Mock::VerifyAndClearExpectations(&notify_observers_cb_) &&
+           testing::Mock::VerifyAndClearExpectations(&notify_observers_cb_) &&
+           testing::Mock::VerifyAndClearExpectations(&trusted_vault_client_) &&
+           testing::Mock::VerifyAndClearExpectations(&engine_);
+  }
+
+  testing::NiceMock<base::MockCallback<base::RepeatingClosure>>
+      notify_observers_cb_;
+  testing::NiceMock<
+      base::MockCallback<base::RepeatingCallback<void(ConfigureReason)>>>
+      reconfigure_cb_;
+  testing::NiceMock<MockCryptoSyncPrefs> prefs_;
+  testing::NiceMock<MockTrustedVaultClient> trusted_vault_client_;
+  testing::NiceMock<MockSyncEngine> engine_;
+  SyncServiceCrypto crypto_;
+};
+
+TEST_F(SyncServiceCryptoTest, ShouldExposePassphraseRequired) {
+  const std::string kTestPassphrase = "somepassphrase";
+
+  crypto_.SetSyncEngine(CoreAccountId(), &engine_);
+  ASSERT_FALSE(crypto_.IsPassphraseRequired());
+
+  // Mimic the engine determining that a passphrase is required.
+  EXPECT_CALL(reconfigure_cb_, Run(CONFIGURE_REASON_CRYPTO));
+  crypto_.OnPassphraseRequired(
+      REASON_DECRYPTION, KeyDerivationParams::CreateForPbkdf2(),
+      MakeEncryptedData(kTestPassphrase,
+                        KeyDerivationParams::CreateForPbkdf2()));
+  EXPECT_TRUE(crypto_.IsPassphraseRequired());
+  VerifyAndClearExpectations();
+
+  // Entering the wrong passphrase should be rejected.
+  EXPECT_CALL(reconfigure_cb_, Run(_)).Times(0);
+  EXPECT_CALL(engine_, SetDecryptionPassphrase(_)).Times(0);
+  EXPECT_FALSE(crypto_.SetDecryptionPassphrase("wrongpassphrase"));
+  EXPECT_TRUE(crypto_.IsPassphraseRequired());
+
+  // Entering the correct passphrase should be accepted.
+  EXPECT_CALL(engine_, SetDecryptionPassphrase(kTestPassphrase))
+      .WillOnce([&](const std::string&) { crypto_.OnPassphraseAccepted(); });
+  // The current implementation issues two reconfigurations: one immediately
+  // after checking the passphase in the UI thread and a second time later when
+  // the engine confirms with OnPassphraseAccepted().
+  EXPECT_CALL(reconfigure_cb_, Run(CONFIGURE_REASON_CRYPTO)).Times(2);
+  EXPECT_TRUE(crypto_.SetDecryptionPassphrase(kTestPassphrase));
+  EXPECT_FALSE(crypto_.IsPassphraseRequired());
+}
+
+TEST_F(SyncServiceCryptoTest,
+       ShouldStoreTrustedVaultKeysBeforeEngineInitialization) {
+  const CoreAccountId kAccount = CoreAccountId("account1");
+  const std::vector<std::string> kKeys = {"key1"};
+  EXPECT_CALL(trusted_vault_client_, StoreKeys(kAccount, kKeys));
+  crypto_.AddTrustedVaultDecryptionKeys(kAccount, kKeys);
+}
+
+TEST_F(SyncServiceCryptoTest,
+       ShouldStoreTrustedVaultKeysAfterEngineInitialization) {
+  const CoreAccountId kSyncingAccount = CoreAccountId("syncingaccount");
+  const CoreAccountId kOtherAccount = CoreAccountId("otheraccount");
+  const std::vector<std::string> kSyncingAccountKeys = {"key1"};
+  const std::vector<std::string> kOtherAccountKeys = {"key2"};
+
+  crypto_.SetSyncEngine(kSyncingAccount, &engine_);
+
+  EXPECT_CALL(trusted_vault_client_,
+              StoreKeys(kOtherAccount, kOtherAccountKeys));
+  EXPECT_CALL(trusted_vault_client_,
+              StoreKeys(kSyncingAccount, kSyncingAccountKeys));
+
+  // Only the sync-ing account should be propagated to the engine.
+  EXPECT_CALL(engine_, AddTrustedVaultDecryptionKeys(kOtherAccountKeys, _))
+      .Times(0);
+  EXPECT_CALL(engine_, AddTrustedVaultDecryptionKeys(kSyncingAccountKeys, _));
+  crypto_.AddTrustedVaultDecryptionKeys(kOtherAccount, kOtherAccountKeys);
+  crypto_.AddTrustedVaultDecryptionKeys(kSyncingAccount, kSyncingAccountKeys);
+}
+
+TEST_F(SyncServiceCryptoTest, ShouldReadValidTrustedVaultKeysFromClient) {
+  const CoreAccountId kSyncingAccount = CoreAccountId("syncingaccount");
+  const std::vector<std::string> kFetchedKeys = {"key1"};
+
+  EXPECT_CALL(reconfigure_cb_, Run(_)).Times(0);
+  ASSERT_FALSE(crypto_.IsTrustedVaultKeyRequired());
+
+  base::OnceCallback<void(const std::vector<std::string>&)> fetch_keys_cb;
+  EXPECT_CALL(trusted_vault_client_, FetchKeys(kSyncingAccount, _))
+      .WillOnce(
+          [&](const CoreAccountId& account_id,
+              base::OnceCallback<void(const std::vector<std::string>&)> cb) {
+            fetch_keys_cb = std::move(cb);
+          });
+
+  // Mimic the engine determining that trusted vault keys are required.
+  crypto_.SetSyncEngine(kSyncingAccount, &engine_);
+  crypto_.OnTrustedVaultKeyRequired();
+  VerifyAndClearExpectations();
+
+  // While there is an ongoing fetch, there should be no user action required.
+  ASSERT_TRUE(fetch_keys_cb);
+  EXPECT_FALSE(crypto_.IsTrustedVaultKeyRequired());
+
+  base::OnceClosure add_keys_cb;
+  EXPECT_CALL(engine_, AddTrustedVaultDecryptionKeys(kFetchedKeys, _))
+      .WillOnce(
+          [&](const std::vector<std::string>& keys, base::OnceClosure done_cb) {
+            add_keys_cb = std::move(done_cb);
+          });
+
+  // Mimic completion of the fetch.
+  std::move(fetch_keys_cb).Run(kFetchedKeys);
+  ASSERT_TRUE(add_keys_cb);
+  EXPECT_FALSE(crypto_.IsTrustedVaultKeyRequired());
+
+  // Mimic completion of the engine.
+  EXPECT_CALL(reconfigure_cb_, Run(CONFIGURE_REASON_CRYPTO));
+  crypto_.OnTrustedVaultKeyAccepted();
+  std::move(add_keys_cb).Run();
+  EXPECT_FALSE(crypto_.IsTrustedVaultKeyRequired());
+}
+
+TEST_F(SyncServiceCryptoTest, ShouldReadInvalidTrustedVaultKeysFromClient) {
+  const CoreAccountId kSyncingAccount = CoreAccountId("syncingaccount");
+  const std::vector<std::string> kFetchedKeys = {"key1"};
+
+  ASSERT_FALSE(crypto_.IsTrustedVaultKeyRequired());
+
+  base::OnceCallback<void(const std::vector<std::string>&)> fetch_keys_cb;
+  EXPECT_CALL(trusted_vault_client_, FetchKeys(kSyncingAccount, _))
+      .WillOnce(
+          [&](const CoreAccountId& account_id,
+              base::OnceCallback<void(const std::vector<std::string>&)> cb) {
+            fetch_keys_cb = std::move(cb);
+          });
+
+  // Mimic the engine determining that trusted vault keys are required.
+  crypto_.SetSyncEngine(kSyncingAccount, &engine_);
+  crypto_.OnTrustedVaultKeyRequired();
+  VerifyAndClearExpectations();
+
+  // While there is an ongoing fetch, there should be no user action required.
+  ASSERT_TRUE(fetch_keys_cb);
+  EXPECT_FALSE(crypto_.IsTrustedVaultKeyRequired());
+
+  base::OnceClosure add_keys_cb;
+  EXPECT_CALL(engine_, AddTrustedVaultDecryptionKeys(kFetchedKeys, _))
+      .WillOnce(
+          [&](const std::vector<std::string>& keys, base::OnceClosure done_cb) {
+            add_keys_cb = std::move(done_cb);
+          });
+
+  // Mimic completion of the client.
+  std::move(fetch_keys_cb).Run(kFetchedKeys);
+  ASSERT_TRUE(add_keys_cb);
+  EXPECT_FALSE(crypto_.IsTrustedVaultKeyRequired());
+
+  // Mimic completion of the engine, without OnTrustedVaultKeyAccepted().
+  EXPECT_CALL(reconfigure_cb_, Run(CONFIGURE_REASON_CRYPTO));
+  std::move(add_keys_cb).Run();
+  EXPECT_TRUE(crypto_.IsTrustedVaultKeyRequired());
+}
+
+}  // namespace
+
+}  // namespace syncer

components/sync/driver/sync_user_settings_unittest.cc

@@ -33,7 +33,8 @@ class SyncUserSettingsTest : public testing::Test {
 
     sync_service_crypto_ = std::make_unique<SyncServiceCrypto>(
         /*notify_observers=*/base::DoNothing(),
-        /*reconfigure=*/base::DoNothing(), sync_prefs_.get());
+        /*reconfigure=*/base::DoNothing(), sync_prefs_.get(),
+        /*trusted_vault_client=*/nullptr);
   }
 
   std::unique_ptr<SyncUserSettingsImpl> MakeSyncUserSettings(

components/sync/driver/trusted_vault_client.h

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_SYNC_DRIVER_TRUSTED_VAULT_CLIENT_H_
+#define COMPONENTS_SYNC_DRIVER_TRUSTED_VAULT_CLIENT_H_
+
+#include <string>
+#include <vector>
+
+#include "base/callback_forward.h"
+#include "base/macros.h"
+
+struct CoreAccountId;
+
+namespace syncer {
+
+// Interface that allows platform-specific logic related to accessing locally
+// available trusted vault encryption keys.
+class TrustedVaultClient {
+ public:
+  TrustedVaultClient() = default;
+  virtual ~TrustedVaultClient() = default;
+
+  // Attempts to fetch decryption keys, required by sync to resume.
+  // Implementations are expected to NOT prompt the user for actions. |cb| is
+  // called on completion with known keys or an empty list if none known.
+  virtual void FetchKeys(
+      const CoreAccountId& account_id,
+      base::OnceCallback<void(const std::vector<std::string>&)> cb) = 0;
+
+  // Allows implementations to store encryption keys fetched by other means such
+  // as Web interactions. Implementations are free to completely ignore these
+  // keys, so callers may not assume that later calls to FetchKeys() would
+  // necessarily return the keys passed here.
+  virtual void StoreKeys(const CoreAccountId& account_id,
+                         const std::vector<std::string>& keys) = 0;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TrustedVaultClient);
+};
+
+}  // namespace syncer
+
+#endif  // COMPONENTS_SYNC_DRIVER_TRUSTED_VAULT_CLIENT_H_

components/sync/engine/fake_sync_engine.cc

@@ -4,6 +4,8 @@
 
 #include "components/sync/engine/fake_sync_engine.h"
 
+#include <utility>
+
 #include "components/sync/engine/data_type_activation_response.h"
 #include "components/sync/engine/sync_engine_host.h"
 #include "components/sync/model/model_type_controller_delegate.h"
@@ -47,7 +49,10 @@ void FakeSyncEngine::SetEncryptionPassphrase(const std::string& passphrase) {}
 void FakeSyncEngine::SetDecryptionPassphrase(const std::string& passphrase) {}
 
 void FakeSyncEngine::AddTrustedVaultDecryptionKeys(
-    const std::vector<std::string>& keys) {}
+    const std::vector<std::string>& keys,
+    base::OnceClosure done_cb) {
+  std::move(done_cb).Run();
+}
 
 void FakeSyncEngine::StopSyncingForShutdown() {}
 

components/sync/engine/fake_sync_engine.h

@@ -46,8 +46,8 @@ class FakeSyncEngine : public SyncEngine {
 
   void SetDecryptionPassphrase(const std::string& passphrase) override;
 
-  void AddTrustedVaultDecryptionKeys(
-      const std::vector<std::string>& keys) override;
+  void AddTrustedVaultDecryptionKeys(const std::vector<std::string>& keys,
+                                     base::OnceClosure done_cb) override;
 
   void StopSyncingForShutdown() override;
 

components/sync/engine/mock_sync_engine.h

@@ -45,8 +45,8 @@ class MockSyncEngine : public SyncEngine {
   MOCK_METHOD0(StartSyncingWithServer, void());
   MOCK_METHOD1(SetEncryptionPassphrase, void(const std::string&));
   MOCK_METHOD1(SetDecryptionPassphrase, void(const std::string&));
-  MOCK_METHOD1(AddTrustedVaultDecryptionKeys,
-               void(const std::vector<std::string>&));
+  MOCK_METHOD2(AddTrustedVaultDecryptionKeys,
+               void(const std::vector<std::string>&, base::OnceClosure));
   MOCK_METHOD0(StopSyncingForShutdown, void());
   MOCK_METHOD1(Shutdown, void(ShutdownReason));
   MOCK_METHOD0(EnableEncryptEverything, void());

components/sync/engine/sync_engine.h

@@ -139,9 +139,10 @@ class SyncEngine : public ModelTypeConfigurer {
   // TRUSTED_VAULT_PASSPHRASE: it provides new decryption keys that could
   // allow decrypting pending Nigori keys. Notifies observers of the result of
   // the operation via OnTrustedVaultKeyAccepted if the provided keys
-  // successfully decrypted pending keys.
+  // successfully decrypted pending keys. |done_cb| is invoked at the very end.
   virtual void AddTrustedVaultDecryptionKeys(
-      const std::vector<std::string>& keys) = 0;
+      const std::vector<std::string>& keys,
+      base::OnceClosure done_cb) = 0;
 
   // Kick off shutdown procedure. Attempts to cut short any long-lived or
   // blocking sync thread tasks so that the shutdown on sync thread task that

ios/chrome/browser/sync/ios_chrome_sync_client.h

@@ -49,6 +49,7 @@ class IOSChromeSyncClient : public browser_sync::BrowserSyncClient {
   syncer::DataTypeController::TypeVector CreateDataTypeControllers(
       syncer::SyncService* sync_service) override;
   invalidation::InvalidationService* GetInvalidationService() override;
+  syncer::TrustedVaultClient* GetTrustedVaultClient() override;
   BookmarkUndoService* GetBookmarkUndoService() override;
   scoped_refptr<syncer::ExtensionsActivity> GetExtensionsActivity() override;
   base::WeakPtr<syncer::SyncableService> GetSyncableServiceForType(

ios/chrome/browser/sync/ios_chrome_sync_client.mm

@@ -190,6 +190,12 @@ IOSChromeSyncClient::GetInvalidationService() {
   return nullptr;
 }
 
+syncer::TrustedVaultClient* IOSChromeSyncClient::GetTrustedVaultClient() {
+  // TODO(crbug.com/1012660): Instantiate a generic client for ios.
+  NOTIMPLEMENTED();
+  return nullptr;
+}
+
 scoped_refptr<syncer::ExtensionsActivity>
 IOSChromeSyncClient::GetExtensionsActivity() {
   return nullptr;

ios/web_view/internal/sync/web_view_sync_client.h

@@ -52,6 +52,7 @@ class WebViewSyncClient : public browser_sync::BrowserSyncClient {
   syncer::DataTypeController::TypeVector CreateDataTypeControllers(
       syncer::SyncService* sync_service) override;
   invalidation::InvalidationService* GetInvalidationService() override;
+  syncer::TrustedVaultClient* GetTrustedVaultClient() override;
   BookmarkUndoService* GetBookmarkUndoService() override;
   scoped_refptr<syncer::ExtensionsActivity> GetExtensionsActivity() override;
   base::WeakPtr<syncer::SyncableService> GetSyncableServiceForType(

ios/web_view/internal/sync/web_view_sync_client.mm

@@ -161,6 +161,10 @@ invalidation::InvalidationService* WebViewSyncClient::GetInvalidationService() {
   return nullptr;
 }
 
+syncer::TrustedVaultClient* WebViewSyncClient::GetTrustedVaultClient() {
+  return nullptr;
+}
+
 scoped_refptr<syncer::ExtensionsActivity>
 WebViewSyncClient::GetExtensionsActivity() {
   return nullptr;