[Sync:USS] Ask for passphrase in pending keystore mode

This CL has two goals:
1. Prevent setting custom passphrase while we have pending keys. We
can't build correct keybag otherwise.
2. Allow users with backward-compatible keystore Nigori exit pending
encryption state by providing old passphrase.

Bug: 922900
Change-Id: I34e62ca41c1baf48ffc66fb9a13ea1bde438a8a1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1855965
Commit-Queue: Maksim Moskvitin <mmoskvitin@google.com>
Auto-Submit: Maksim Moskvitin <mmoskvitin@google.com>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#706014}

components/sync/nigori/nigori_sync_bridge_impl.cc

@@ -594,8 +594,16 @@ void NigoriSyncBridgeImpl::SetEncryptionPassphrase(
       return;
     case NigoriSpecifics::KEYSTORE_PASSPHRASE:
     case NigoriSpecifics::TRUSTED_VAULT_PASSPHRASE:
+      if (state_.pending_keys.has_value()) {
+        // TODO(crbug.com/922900): investigate whether we need to call
+        // MaybeNotifyOfPendingKeys() to prompt for decryption passphrase.
+        DVLOG(1) << "Attempt to set explicit passphrase failed, because there "
+                 << "are pending keys.";
+        return;
+      }
       break;
   }
+  DCHECK(!state_.pending_keys.has_value());
   DCHECK(state_.cryptographer->CanEncrypt());
 
   const KeyDerivationParams custom_passphrase_key_derivation_params =
@@ -645,7 +653,10 @@ void NigoriSyncBridgeImpl::SetDecryptionPassphrase(
   // |passphrase| should be a valid one already (verified by SyncServiceCrypto,
   // using pending keys exposed by OnPassphraseRequired()).
   DCHECK(!passphrase.empty());
-  DCHECK(state_.pending_keys);
+  if (!state_.pending_keys) {
+    DCHECK_EQ(state_.passphrase_type, NigoriSpecifics::KEYSTORE_PASSPHRASE);
+    return;
+  }
 
   const std::string new_key_name = state_.cryptographer->EmplaceKey(
       passphrase, GetKeyDerivationParamsForPendingKeys());
@@ -656,14 +667,11 @@ void NigoriSyncBridgeImpl::SetDecryptionPassphrase(
   }
 
   if (!TryDecryptPendingKeys()) {
-    // TODO(crbug.com/922900): old implementation assumes that pending keys
-    // encryption key may change in between of OnPassphraseRequired() and
-    // SetDecryptionPassphrase() calls, verify whether it's really possible.
-    // Hypothetical cases are transition from FROZEN_IMPLICIT_PASSPHRASE to
-    // CUSTOM_PASSPHRASE and changing of passphrase due to conflict resolution.
-    processor_->ReportError(ModelError(
-        FROM_HERE,
-        "Failed to decrypt pending keys with provided explicit passphrase."));
+    // |pending_keys| could be changed in between of OnPassphraseRequired()
+    // and SetDecryptionPassphrase() calls (remote update with different
+    // keystore Nigori or with transition from keystore to custom passphrase
+    // Nigori).
+    MaybeNotifyOfPendingKeys();
     return;
   }
 
@@ -783,7 +791,6 @@ bool NigoriSyncBridgeImpl::SetKeystoreKeys(
     // Newly arrived keystore keys could resolve pending encryption state in
     // keystore mode.
     DCHECK_EQ(state_.passphrase_type, NigoriSpecifics::KEYSTORE_PASSPHRASE);
-    DCHECK(state_.pending_keys.has_value());
     UpdateCryptographerFromKeystoreNigori(
         sync_pb::EncryptedData(*state_.pending_keys),
         sync_pb::EncryptedData(*state_.pending_keystore_decryptor_token));
@@ -998,7 +1005,12 @@ NigoriSyncBridgeImpl::UpdateCryptographerFromKeystoreNigori(
   state_.cryptographer->EmplaceKeysFrom(NigoriKeyBag::CreateFromProto(key_bag));
   state_.cryptographer->SelectDefaultEncryptionKey(
       encryption_keybag.key_name());
-  state_.pending_keys.reset();
+  if (state_.pending_keys) {
+    state_.pending_keys.reset();
+    for (auto& observer : observers_) {
+      observer.OnPassphraseAccepted();
+    }
+  }
   return base::nullopt;
 }
 
@@ -1207,9 +1219,9 @@ KeyDerivationParams NigoriSyncBridgeImpl::GetKeyDerivationParamsForPendingKeys()
   switch (state_.passphrase_type) {
     case NigoriSpecifics::UNKNOWN:
     case NigoriSpecifics::IMPLICIT_PASSPHRASE:
-    case NigoriSpecifics::KEYSTORE_PASSPHRASE:
       NOTREACHED();
       return KeyDerivationParams::CreateWithUnsupportedMethod();
+    case NigoriSpecifics::KEYSTORE_PASSPHRASE:
     case NigoriSpecifics::FROZEN_IMPLICIT_PASSPHRASE:
     case NigoriSpecifics::TRUSTED_VAULT_PASSPHRASE:
       return KeyDerivationParams::CreateForPbkdf2();
@@ -1229,11 +1241,6 @@ void NigoriSyncBridgeImpl::MaybeNotifyOfPendingKeys() const {
     case NigoriSpecifics::IMPLICIT_PASSPHRASE:
       return;
     case NigoriSpecifics::KEYSTORE_PASSPHRASE:
-      // TODO(crbug.com/922900): in backward-compatible keystore mode,
-      // |pending_keys| could be decrypted with user-provided passphrase.
-      // Consider calling OnPassphraseRequired() together with decryption
-      // logic.
-      return;
     case NigoriSpecifics::CUSTOM_PASSPHRASE:
     case NigoriSpecifics::FROZEN_IMPLICIT_PASSPHRASE:
       for (auto& observer : observers_) {

components/sync/nigori/nigori_sync_bridge_impl_unittest.cc

@@ -582,6 +582,13 @@ TEST_F(NigoriSyncBridgeImplTest, ShouldDecryptPendingKeysInKeystoreMode) {
 
   EXPECT_CALL(*observer(), OnCryptographerStateChanged(
                                NotNull(), /*has_pending_keys=*/true));
+  EXPECT_CALL(
+      *observer(),
+      OnPassphraseRequired(
+          /*reason=*/REASON_DECRYPTION,
+          /*key_derivation_params=*/KeyDerivationParams::CreateForPbkdf2(),
+          /*pending_keys=*/
+          EncryptedDataEq(entity_data.specifics.nigori().encryption_keybag())));
   EXPECT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
               Eq(base::nullopt));
   const Cryptographer& cryptographer = bridge()->GetCryptographerForTesting();
@@ -589,11 +596,98 @@ TEST_F(NigoriSyncBridgeImplTest, ShouldDecryptPendingKeysInKeystoreMode) {
 
   EXPECT_CALL(*observer(), OnCryptographerStateChanged(
                                NotNull(), /*has_pending_keys=*/false));
+  EXPECT_CALL(*observer(), OnPassphraseAccepted());
   EXPECT_TRUE(bridge()->SetKeystoreKeys({kRawKeystoreKey}));
   EXPECT_THAT(cryptographer, CanDecryptWith(kKeystoreKeyParams));
   EXPECT_THAT(cryptographer, HasDefaultKeyDerivedFrom(kKeystoreKeyParams));
 }
 
+// This test emulates late arrival of keystore keys in backward-compatible
+// keystore mode, so neither |keystore_decryptor_token| or |encryption_keybag|
+// could be decrypted at the moment NigoriSpecifics arrived. Since default key
+// is derived from legacy implicit passphrase, pending keys should be decrypted
+// once passphrase passed to SetDecryptionPassphrase(). SetKeystoreKeys()
+// intentionally not called in this test, to not allow decryption with
+// |keystore_decryptor_token|.
+TEST_F(NigoriSyncBridgeImplTest,
+       ShouldDecryptPendingKeysWithPassphraseInKeystoreMode) {
+  const KeyParams kKeystoreKeyParams = KeystoreKeyParams("keystore_key");
+  const KeyParams kPassphraseKeyParams = Pbkdf2KeyParams("passphrase");
+  EntityData entity_data;
+  *entity_data.specifics.mutable_nigori() = BuildKeystoreNigoriSpecifics(
+      /*keybag_keys_params=*/{kKeystoreKeyParams, kPassphraseKeyParams},
+      /*keystore_decryptor_params=*/kPassphraseKeyParams,
+      /*keystore_key_params=*/kKeystoreKeyParams);
+
+  ASSERT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
+              Eq(base::nullopt));
+
+  EXPECT_CALL(*observer(), OnCryptographerStateChanged(
+                               NotNull(), /*has_pending_keys=*/false));
+  EXPECT_CALL(*observer(), OnPassphraseAccepted());
+  bridge()->SetDecryptionPassphrase(kPassphraseKeyParams.password);
+
+  const Cryptographer& cryptographer = bridge()->GetCryptographerForTesting();
+  EXPECT_THAT(cryptographer, CanDecryptWith(kKeystoreKeyParams));
+  EXPECT_THAT(cryptographer, CanDecryptWith(kPassphraseKeyParams));
+  EXPECT_THAT(cryptographer, HasDefaultKeyDerivedFrom(kPassphraseKeyParams));
+}
+
+// Tests that unsuccessful attempt of |pending_keys| decryption ends up in
+// additional OnPassphraseRequired() call. This is allowed because of possible
+// change of |pending_keys| in keystore mode or due to transition from keystore
+// to custom passphrase.
+TEST_F(NigoriSyncBridgeImplTest,
+       ShouldNotifyWhenDecryptionWithPassphraseFailed) {
+  const KeyParams kKeystoreKeyParams = KeystoreKeyParams("keystore_key");
+  EntityData entity_data;
+  *entity_data.specifics.mutable_nigori() = BuildKeystoreNigoriSpecifics(
+      /*keybag_keys_params=*/{kKeystoreKeyParams},
+      /*keystore_decryptor_params=*/kKeystoreKeyParams,
+      /*keystore_key_params=*/kKeystoreKeyParams);
+
+  sync_pb::EncryptedData expected_pending_keys =
+      entity_data.specifics.nigori().encryption_keybag();
+  ASSERT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
+              Eq(base::nullopt));
+
+  EXPECT_CALL(
+      *observer(),
+      OnPassphraseRequired(
+          /*reason=*/REASON_DECRYPTION,
+          /*key_derivation_params=*/KeyDerivationParams::CreateForPbkdf2(),
+          /*pending_keys=*/
+          EncryptedDataEq(expected_pending_keys)));
+  bridge()->SetDecryptionPassphrase("wrong_passphrase");
+}
+
+// Tests that attempt to SetEncryptionPassphrase() has no effect (at least
+// that bridge's Nigori is still keystore one) if it was called, while bridge
+// has pending keys in keystore mode.
+TEST_F(NigoriSyncBridgeImplTest,
+       ShouldNotSetEncryptionPassphraseWithPendingKeys) {
+  const std::string kRawKeystoreKey = "raw_keystore_key";
+  const KeyParams kKeystoreKeyParams = KeystoreKeyParams(kRawKeystoreKey);
+  EntityData entity_data;
+  *entity_data.specifics.mutable_nigori() = BuildKeystoreNigoriSpecifics(
+      /*keybag_keys_params=*/{kKeystoreKeyParams},
+      /*keystore_decryptor_params=*/kKeystoreKeyParams,
+      /*keystore_key_params=*/kKeystoreKeyParams);
+
+  ASSERT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
+              Eq(base::nullopt));
+  bridge()->SetEncryptionPassphrase("passphrase");
+  bridge()->SetKeystoreKeys({kRawKeystoreKey});
+
+  // TODO(crbug.com/922900): revisit expectations once conflict resolution is
+  // implemented. They might be not properly working with deferred state
+  // change.
+  EXPECT_THAT(bridge()->GetData(), HasKeystoreNigori());
+  const Cryptographer& cryptographer = bridge()->GetCryptographerForTesting();
+  EXPECT_THAT(cryptographer, CanDecryptWith(kKeystoreKeyParams));
+  EXPECT_THAT(cryptographer, HasDefaultKeyDerivedFrom(kKeystoreKeyParams));
+}
+
 // Tests that we can perform initial sync with custom passphrase Nigori.
 // We should notify observers about encryption state changes and cryptographer
 // shouldn't be ready (by having pending keys) until user provides the