Adopt new sync Cryptographer for Nigori USS

NigoriSyncBridgeImpl represents the new (experimental) codepath for
implementing the Nigori datatype. This patch adopts the recently
introduced CryptographerImpl class, which has a cleaner and safer API.

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

components/sync/nigori/DEPS

@@ -4,8 +4,5 @@ include_rules = [
   "+components/sync/model",
   "+components/sync/model_impl",
   "+components/sync/protocol",
-  # TODO(crbug.com/967417): Remove once the USS code doesn't use the legacy
-  # cryptographer.
-  "+components/sync/syncable",
   "+crypto",
 ]

components/sync/nigori/cryptographer_impl.cc

@@ -83,6 +83,14 @@ void CryptographerImpl::ClearDefaultEncryptionKey() {
   default_encryption_key_name_.clear();
 }
 
+sync_pb::NigoriKey CryptographerImpl::ExportDefaultKeyWithoutName() const {
+  DCHECK(CanEncrypt());
+
+  sync_pb::NigoriKey key = key_bag_.ExportKey(default_encryption_key_name_);
+  key.clear_name();
+  return key;
+}
+
 std::unique_ptr<Cryptographer> CryptographerImpl::Clone() const {
   return base::WrapUnique(
       new CryptographerImpl(key_bag_.Clone(), default_encryption_key_name_));

components/sync/nigori/cryptographer_impl.h

@@ -65,6 +65,11 @@ class CryptographerImpl : public Cryptographer {
   // false.
   void ClearDefaultEncryptionKey();
 
+  // Returns a proto representation of the default encryption key, without the
+  // name field populated. |*this| must have a default encryption key set,
+  // as reflected by CanEncrypt().
+  sync_pb::NigoriKey ExportDefaultKeyWithoutName() const;
+
   // Cryptographer overrides.
   std::unique_ptr<Cryptographer> Clone() const override;
   bool CanEncrypt() const override;

components/sync/nigori/cryptographer_impl_unittest.cc

@@ -132,4 +132,26 @@ TEST(CryptographerImplTest, ShouldSerializeToAndFromProto) {
   EXPECT_THAT(decrypted, Eq(kText2));
 }
 
+TEST(CryptographerImplTest, ShouldExportDefaultKeyWithoutName) {
+  std::unique_ptr<CryptographerImpl> cryptographer =
+      CryptographerImpl::CreateEmpty();
+  ASSERT_THAT(cryptographer, NotNull());
+
+  const std::string key_name = cryptographer->EmplaceKey(
+      "password1", KeyDerivationParams::CreateForPbkdf2());
+  ASSERT_THAT(key_name, Ne(std::string()));
+
+  cryptographer->SelectDefaultEncryptionKey(key_name);
+  ASSERT_TRUE(cryptographer->CanEncrypt());
+
+  sync_pb::NigoriKey exported_key =
+      cryptographer->ExportDefaultKeyWithoutName();
+  EXPECT_FALSE(exported_key.has_name());
+
+  // The exported key, even without name, should be importable, and the
+  // resulting key name should match the original.
+  EXPECT_THAT(NigoriKeyBag::CreateEmpty().AddKeyFromProto(exported_key),
+              Eq(key_name));
+}
+
 }  // namespace syncer

components/sync/nigori/nigori_sync_bridge_impl.h

@@ -18,10 +18,10 @@
 #include "components/sync/engine/sync_encryption_handler.h"
 #include "components/sync/model/conflict_resolution.h"
 #include "components/sync/model/model_error.h"
+#include "components/sync/nigori/cryptographer_impl.h"
 #include "components/sync/nigori/keystore_keys_handler.h"
 #include "components/sync/nigori/nigori_local_change_processor.h"
 #include "components/sync/nigori/nigori_sync_bridge.h"
-#include "components/sync/syncable/directory_cryptographer.h"
 
 namespace sync_pb {
 class NigoriLocalData;
@@ -81,13 +81,14 @@ class NigoriSyncBridgeImpl : public KeystoreKeysHandler,
   // TODO(crbug.com/922900): investigate whether we need this getter outside of
   // tests and decide whether this method should be a part of
   // SyncEncryptionHandler interface.
-  const DirectoryCryptographer& GetCryptographerForTesting() const;
+  const Cryptographer& GetCryptographerForTesting() const;
   sync_pb::NigoriSpecifics::PassphraseType GetPassphraseTypeForTesting() const;
   ModelTypeSet GetEncryptedTypesForTesting() const;
+  bool HasPendingKeysForTesting() const;
 
   static std::string PackExplicitPassphraseKeyForTesting(
       const Encryptor& encryptor,
-      const DirectoryCryptographer& cryptographer);
+      const CryptographerImpl& cryptographer);
 
  private:
   base::Optional<ModelError> UpdateLocalState(
@@ -134,7 +135,15 @@ class NigoriSyncBridgeImpl : public KeystoreKeysHandler,
   // separately. Should be encrypted with OSCrypt before persisting.
   std::vector<std::string> keystore_keys_;
 
-  DirectoryCryptographer cryptographer_;
+  std::unique_ptr<CryptographerImpl> cryptographer_;
+
+  // Pending keys represent a remote update that contained a keybag that cannot
+  // be decrypted (e.g. user needs to enter a custom passphrase). If pending
+  // keys are present, |*cryptographer_| does not have a default encryption key
+  // set and instead the should-be default encryption key is determined by the
+  // key in |pending_keys_|.
+  base::Optional<sync_pb::EncryptedData> pending_keys_;
+
   // TODO(mmoskvitin): Consider adopting the C++ enum PassphraseType here and
   // if so remove function ProtoPassphraseInt32ToProtoEnum() from
   // passphrase_enums.h.

components/sync/nigori/nigori_sync_bridge_impl_unittest.cc

@@ -126,6 +126,11 @@ MATCHER_P2(IsDummyNigoriMetadataBatchWithTokenAndSequenceNumber,
          expected.entity_metadata->SerializeAsString();
 }
 
+struct KeyParams {
+  KeyDerivationParams derivation_params;
+  std::string password;
+};
+
 KeyParams Pbkdf2KeyParams(std::string key) {
   return {KeyDerivationParams::CreateForPbkdf2(), std::move(key)};
 }
@@ -144,10 +149,9 @@ KeyParams ScryptKeyParams(const std::string& key) {
 
 std::string PackKeyAsExplicitPassphrase(const KeyParams& key_params,
                                         const Encryptor& encryptor) {
-  DirectoryCryptographer cryptographer;
-  cryptographer.AddKey(key_params);
   return NigoriSyncBridgeImpl::PackExplicitPassphraseKeyForTesting(
-      encryptor, cryptographer);
+      encryptor, *CryptographerImpl::FromSingleKeyForTesting(
+                     key_params.password, key_params.derivation_params));
 }
 
 // Builds NigoriSpecifics with following fields:
@@ -166,18 +170,25 @@ sync_pb::NigoriSpecifics BuildKeystoreNigoriSpecifics(
     const KeyParams& keystore_key_params) {
   sync_pb::NigoriSpecifics specifics;
 
-  DirectoryCryptographer cryptographer;
-  cryptographer.AddKey(keystore_decryptor_params);
+  std::unique_ptr<CryptographerImpl> cryptographer =
+      CryptographerImpl::FromSingleKeyForTesting(
+          keystore_decryptor_params.password,
+          keystore_decryptor_params.derivation_params);
   for (const KeyParams& key_params : keybag_keys_params) {
-    cryptographer.AddNonDefaultKey(key_params);
+    cryptographer->EmplaceKey(key_params.password,
+                              key_params.derivation_params);
   }
-  EXPECT_TRUE(cryptographer.GetKeys(specifics.mutable_encryption_keybag()));
+
+  EXPECT_TRUE(cryptographer->Encrypt(cryptographer->ToProto().key_bag(),
+                                     specifics.mutable_encryption_keybag()));
 
   std::string serialized_keystore_decryptor =
-      cryptographer.GetDefaultNigoriKeyData();
-  DirectoryCryptographer keystore_cryptographer;
-  keystore_cryptographer.AddKey(keystore_key_params);
-  EXPECT_TRUE(keystore_cryptographer.EncryptString(
+      cryptographer->ExportDefaultKeyWithoutName().SerializeAsString();
+
+  std::unique_ptr<CryptographerImpl> keystore_cryptographer =
+      CryptographerImpl::FromSingleKeyForTesting(
+          keystore_key_params.password, keystore_key_params.derivation_params);
+  EXPECT_TRUE(keystore_cryptographer->EncryptString(
       serialized_keystore_decryptor,
       specifics.mutable_keystore_decryptor_token()));
 
@@ -187,13 +198,17 @@ sync_pb::NigoriSpecifics BuildKeystoreNigoriSpecifics(
 
 sync_pb::NigoriSpecifics BuildTrustedVaultNigoriSpecifics(
     const std::vector<KeyParams>& trusted_vault_key_params) {
-  DirectoryCryptographer cryptographer;
+  std::unique_ptr<CryptographerImpl> cryptographer =
+      CryptographerImpl::CreateEmpty();
   for (const KeyParams& key_params : trusted_vault_key_params) {
-    cryptographer.AddKey(key_params);
+    const std::string key_name = cryptographer->EmplaceKey(
+        key_params.password, key_params.derivation_params);
+    cryptographer->SelectDefaultEncryptionKey(key_name);
   }
 
   sync_pb::NigoriSpecifics specifics;
-  EXPECT_TRUE(cryptographer.GetKeys(specifics.mutable_encryption_keybag()));
+  EXPECT_TRUE(cryptographer->Encrypt(cryptographer->ToProto().key_bag(),
+                                     specifics.mutable_encryption_keybag()));
   specifics.set_passphrase_type(
       sync_pb::NigoriSpecifics::TRUSTED_VAULT_PASSPHRASE);
   return specifics;
@@ -210,14 +225,18 @@ sync_pb::NigoriSpecifics BuildTrustedVaultNigoriSpecifics(
 sync_pb::NigoriSpecifics BuildCustomPassphraseNigoriSpecifics(
     const KeyParams& passphrase_key_params,
     const base::Optional<KeyParams>& old_key_params = base::nullopt) {
-  sync_pb::NigoriSpecifics specifics;
-
-  DirectoryCryptographer cryptographer;
-  cryptographer.AddKey(passphrase_key_params);
+  std::unique_ptr<CryptographerImpl> cryptographer =
+      CryptographerImpl::FromSingleKeyForTesting(
+          passphrase_key_params.password,
+          passphrase_key_params.derivation_params);
   if (old_key_params) {
-    cryptographer.AddNonDefaultKey(*old_key_params);
+    cryptographer->EmplaceKey(old_key_params->password,
+                              old_key_params->derivation_params);
   }
-  EXPECT_TRUE(cryptographer.GetKeys(specifics.mutable_encryption_keybag()));
+
+  sync_pb::NigoriSpecifics specifics;
+  EXPECT_TRUE(cryptographer->Encrypt(cryptographer->ToProto().key_bag(),
+                                     specifics.mutable_encryption_keybag()));
 
   specifics.set_custom_passphrase_key_derivation_method(
       EnumKeyDerivationMethodToProto(
@@ -505,7 +524,7 @@ TEST_F(NigoriSyncBridgeImplTest,
                                       Not(NullTime())));
   EXPECT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
               Eq(base::nullopt));
-  EXPECT_TRUE(bridge()->GetCryptographerForTesting().has_pending_keys());
+  EXPECT_TRUE(bridge()->HasPendingKeysForTesting());
 }
 
 // Tests that we can process remote update with custom passphrase Nigori, while
@@ -537,7 +556,7 @@ TEST_F(NigoriSyncBridgeImplTest, ShouldTransitToCustomPassphrase) {
                                       Not(NullTime())));
   EXPECT_THAT(bridge()->ApplySyncChanges(std::move(new_entity_data)),
               Eq(base::nullopt));
-  EXPECT_TRUE(bridge()->GetCryptographerForTesting().has_pending_keys());
+  EXPECT_TRUE(bridge()->HasPendingKeysForTesting());
 }
 
 // Tests that we don't try to overwrite default passphrase type and report
@@ -569,10 +588,11 @@ TEST_F(NigoriSyncBridgeImplTest, ShouldClearDataWhenSyncDisabled) {
   ASSERT_TRUE(bridge()->SetKeystoreKeys({kRawKeystoreKey}));
   ASSERT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
               Eq(base::nullopt));
+  ASSERT_TRUE(bridge()->GetCryptographerForTesting().CanEncrypt());
 
   EXPECT_CALL(*storage(), ClearData);
   bridge()->ApplyDisableSyncChanges();
-  EXPECT_FALSE(bridge()->GetCryptographerForTesting().is_initialized());
+  EXPECT_FALSE(bridge()->GetCryptographerForTesting().CanEncrypt());
 }
 
 // Tests decryption logic for explicit passphrase. In order to check that we're
@@ -789,13 +809,13 @@ TEST_F(NigoriSyncBridgeImplTest,
               Eq(sync_pb::NigoriSpecifics::TRUSTED_VAULT_PASSPHRASE));
   EXPECT_THAT(bridge()->GetEncryptedTypesForTesting(),
               Eq(SyncEncryptionHandler::SensitiveTypes()));
-  EXPECT_TRUE(bridge()->GetCryptographerForTesting().has_pending_keys());
+  EXPECT_TRUE(bridge()->HasPendingKeysForTesting());
 
   EXPECT_CALL(*observer(), OnPassphraseAccepted());
   EXPECT_CALL(*observer(), OnCryptographerStateChanged(NotNull()));
   EXPECT_CALL(*observer(), OnBootstrapTokenUpdated(_, _)).Times(0);
   bridge()->SetDecryptionPassphrase(kTrustedVaultPassphrase);
-  EXPECT_FALSE(bridge()->GetCryptographerForTesting().has_pending_keys());
+  EXPECT_FALSE(bridge()->HasPendingKeysForTesting());
 }
 
 // Tests the processing of a remote incremental update that transitions from
@@ -835,13 +855,13 @@ TEST_F(NigoriSyncBridgeImplTest,
               Eq(sync_pb::NigoriSpecifics::TRUSTED_VAULT_PASSPHRASE));
   EXPECT_THAT(bridge()->GetEncryptedTypesForTesting(),
               Eq(SyncEncryptionHandler::SensitiveTypes()));
-  EXPECT_TRUE(bridge()->GetCryptographerForTesting().has_pending_keys());
+  EXPECT_TRUE(bridge()->HasPendingKeysForTesting());
 
   EXPECT_CALL(*observer(), OnPassphraseAccepted());
   EXPECT_CALL(*observer(), OnCryptographerStateChanged(NotNull()));
   EXPECT_CALL(*observer(), OnBootstrapTokenUpdated(_, _)).Times(0);
   bridge()->SetDecryptionPassphrase(kTrustedVaultPassphrase);
-  EXPECT_FALSE(bridge()->GetCryptographerForTesting().has_pending_keys());
+  EXPECT_FALSE(bridge()->HasPendingKeysForTesting());
 }
 
 // Tests the processing of a remote incremental update that rotates the trusted
@@ -858,9 +878,9 @@ TEST_F(NigoriSyncBridgeImplTest,
   ASSERT_TRUE(bridge()->SetKeystoreKeys({"keystore_key"}));
   ASSERT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
               Eq(base::nullopt));
-  ASSERT_TRUE(bridge()->GetCryptographerForTesting().has_pending_keys());
+  ASSERT_TRUE(bridge()->HasPendingKeysForTesting());
   bridge()->SetDecryptionPassphrase(kTrustedVaultPassphrase);
-  ASSERT_FALSE(bridge()->GetCryptographerForTesting().has_pending_keys());
+  ASSERT_FALSE(bridge()->HasPendingKeysForTesting());
   ASSERT_THAT(bridge()->GetPassphraseTypeForTesting(),
               Eq(sync_pb::NigoriSpecifics::TRUSTED_VAULT_PASSPHRASE));
   // Mimic remote key rotation.
@@ -879,12 +899,12 @@ TEST_F(NigoriSyncBridgeImplTest,
 
   EXPECT_THAT(bridge()->ApplySyncChanges(std::move(new_entity_data)),
               Eq(base::nullopt));
-  EXPECT_TRUE(bridge()->GetCryptographerForTesting().has_pending_keys());
+  EXPECT_TRUE(bridge()->HasPendingKeysForTesting());
 
   EXPECT_CALL(*observer(), OnPassphraseAccepted());
   EXPECT_CALL(*observer(), OnCryptographerStateChanged(NotNull()));
   bridge()->SetDecryptionPassphrase(kRotatedTrustedVaultPassphrase);
-  EXPECT_FALSE(bridge()->GetCryptographerForTesting().has_pending_keys());
+  EXPECT_FALSE(bridge()->HasPendingKeysForTesting());
 }
 
 // Tests transitioning locally from trusted vault passphrase to custom
@@ -901,9 +921,9 @@ TEST_F(NigoriSyncBridgeImplTest,
   ASSERT_TRUE(bridge()->SetKeystoreKeys({"keystore_key"}));
   ASSERT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
               Eq(base::nullopt));
-  ASSERT_TRUE(bridge()->GetCryptographerForTesting().has_pending_keys());
+  ASSERT_TRUE(bridge()->HasPendingKeysForTesting());
   bridge()->SetDecryptionPassphrase(kTrustedVaultPassphrase);
-  ASSERT_FALSE(bridge()->GetCryptographerForTesting().has_pending_keys());
+  ASSERT_FALSE(bridge()->HasPendingKeysForTesting());
   ASSERT_THAT(bridge()->GetPassphraseTypeForTesting(),
               Eq(sync_pb::NigoriSpecifics::TRUSTED_VAULT_PASSPHRASE));
   ASSERT_THAT(bridge()->GetData(), Not(HasCustomPassphraseNigori()));

components/sync/syncable/directory_cryptographer.cc

@@ -164,21 +164,6 @@ bool DirectoryCryptographer::AddNonDefaultKey(const KeyParams& params) {
       /*set_as_default=*/false);
 }
 
-void DirectoryCryptographer::AddAllUnknownKeysFrom(const NigoriKeyBag& other) {
-  key_bag_.AddAllUnknownKeysFrom(other);
-}
-
-void DirectoryCryptographer::SelectDefaultEncryptionKey(
-    const std::string& key_name) {
-  DCHECK(!key_name.empty());
-  DCHECK(key_bag_.HasKey(key_name));
-  default_nigori_name_ = key_name;
-}
-
-void DirectoryCryptographer::ClearPendingKeys() {
-  pending_keys_.reset();
-}
-
 bool DirectoryCryptographer::AddKeyFromBootstrapToken(
     const Encryptor& encryptor,
     const std::string& restored_bootstrap_token) {

components/sync/syncable/directory_cryptographer.h

@@ -123,12 +123,6 @@ class DirectoryCryptographer : public Cryptographer {
   // will become the new default).
   bool AddNonDefaultKey(const KeyParams& params);
 
-  // TODO(crbug.com/967417): Remove when transition of NigoriSyncBridgeImpl is
-  // finished.
-  void AddAllUnknownKeysFrom(const NigoriKeyBag& other);
-  void SelectDefaultEncryptionKey(const std::string& key_name);
-  void ClearPendingKeys();
-
   // Decrypts |encrypted| and uses its contents to initialize Nigori instances.
   // Returns true unless decryption of |encrypted| fails. The caller is
   // responsible for checking that CanDecrypt(encrypted) == true.