[Sync:USS] Implement cryptographer sync for keystore Nigori

Bug: 922900
Change-Id: I61d54db699e00a99ddbbcc95a6516411ab520ee0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1552806
Commit-Queue: Maksim Moskvitin <mmoskvitin@google.com>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#653568}

components/sync/BUILD.gn

@@ -757,6 +757,7 @@ source_set("unit_tests") {
     "model_impl/processor_entity_unittest.cc",
     "model_impl/syncable_service_based_bridge_unittest.cc",
     "nigori/nigori_model_type_processor_unittest.cc",
+    "nigori/nigori_sync_bridge_impl_unittest.cc",
     "protocol/proto_enum_conversions_unittest.cc",
     "protocol/proto_value_conversions_unittest.cc",
     "syncable/change_record_unittest.cc",

components/sync/nigori/nigori_sync_bridge_impl.cc

@@ -4,9 +4,54 @@
 
 #include "components/sync/nigori/nigori_sync_bridge_impl.h"
 
+#include "base/base64.h"
+#include "base/location.h"
+#include "components/sync/base/nigori.h"
+#include "components/sync/base/passphrase_enums.h"
+#include "components/sync/model/entity_data.h"
+#include "components/sync/protocol/encryption.pb.h"
+#include "components/sync/protocol/nigori_specifics.pb.h"
+
 namespace syncer {
 
-NigoriSyncBridgeImpl::NigoriSyncBridgeImpl() = default;
+namespace {
+
+// Attempts to decrypt |keystore_decryptor_token| with |keystore_keys|. Returns
+// serialized Nigori key if successful and base::nullopt otherwise.
+base::Optional<std::string> DecryptKeystoreDecryptor(
+    const std::vector<std::string> keystore_keys,
+    const sync_pb::EncryptedData& keystore_decryptor_token,
+    Encryptor* const encryptor) {
+  if (keystore_decryptor_token.blob().empty()) {
+    return base::nullopt;
+  }
+
+  Cryptographer cryptographer(encryptor);
+  for (const std::string& key : keystore_keys) {
+    KeyParams key_params = {KeyDerivationParams::CreateForPbkdf2(), key};
+    // TODO(crbug.com/922900): possible behavioral change. Old implementation
+    // fails only if we failed to add current keystore key. Failing to add any
+    // of these keys doesn't seem valid. This line seems to be a good candidate
+    // for UMA, as it's not a normal situation, if we fail to add any key.
+    if (!cryptographer.AddKey(key_params)) {
+      return base::nullopt;
+    }
+  }
+
+  std::string serialized_nigori_key;
+  // This check should never fail as long as we don't receive invalid data.
+  if (!cryptographer.CanDecrypt(keystore_decryptor_token) ||
+      !cryptographer.DecryptToString(keystore_decryptor_token,
+                                     &serialized_nigori_key)) {
+    return base::nullopt;
+  }
+  return serialized_nigori_key;
+}
+
+}  // namespace
+
+NigoriSyncBridgeImpl::NigoriSyncBridgeImpl(Encryptor* encryptor)
+    : cryptographer_(encryptor) {}
 
 NigoriSyncBridgeImpl::~NigoriSyncBridgeImpl() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -52,27 +97,97 @@ bool NigoriSyncBridgeImpl::IsEncryptEverythingEnabled() const {
 
 bool NigoriSyncBridgeImpl::NeedKeystoreKey() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  NOTIMPLEMENTED();
+  // We never need to explicitly ask the server to provide the new keystore
+  // key, because server sends keystore keys every time it sends keystore-based
+  // Nigori node.
+  // TODO(crbug.com/922900): verify logic above. Old implementation is
+  // different, but it's probably only related to Nigori migration to keystore
+  // logic: there was no need to send new Nigori node from the server, because
+  // the migration was implemented on client side, but client needs
+  // keystore keys in order to perform the migration.
   return false;
 }
 
 bool NigoriSyncBridgeImpl::SetKeystoreKeys(
     const std::vector<std::string>& keys) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  if (keys.empty() || keys.back().empty()) {
+    return false;
+  }
+
+  keystore_keys_.resize(keys.size());
+  for (size_t i = 0; i < keys.size(); ++i) {
+    // We need to apply base64 encoding before using the keys to provide
+    // backward compatibility with non-USS implementation. It's actually needed
+    // only for the keys persisting, but was applied before passing keys to
+    // cryptographer, so we have to do the same.
+    base::Base64Encode(keys[i], &keystore_keys_[i]);
+  }
+
+  // TODO(crbug.com/922900): persist keystore keys.
+  // TODO(crbug.com/922900): support keystore rotation.
+  // TODO(crbug.com/922900): verify that this method is always called before
+  // update or init of Nigori node. If this is the case we don't need to touch
+  // cryptographer here. If this is not the case, old code is actually broken:
+  // 1. Receive and persist the Nigori node after keystore rotation on
+  // different client.
+  // 2. Browser crash.
+  // 3. After load we don't request new Nigori node from the server (we already
+  // have the newest one), so logic with simultaneous sending of Nigori node
+  // and keystore keys doesn't help. We don't request new keystore keys
+  // explicitly (we already have one). We can't decrypt and use Nigori node
+  // with old keystore keys.
   NOTIMPLEMENTED();
-  return false;
+  return true;
 }
 
 base::Optional<ModelError> NigoriSyncBridgeImpl::MergeSyncData(
     const base::Optional<EntityData>& data) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  NOTIMPLEMENTED();
-  return base::nullopt;
+  if (!data) {
+    return ModelError(FROM_HERE,
+                      "Received empty EntityData during initial "
+                      "sync of Nigori.");
+  }
+  return ApplySyncChanges(*data);
 }
 
 base::Optional<ModelError> NigoriSyncBridgeImpl::ApplySyncChanges(
     const EntityData& data) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(data.specifics.has_nigori());
+  const sync_pb::NigoriSpecifics& nigori = data.specifics.nigori();
+
+  // TODO(crbug.com/922900): support other passphrase types.
+  if (ProtoPassphraseTypeToEnum(nigori.passphrase_type()) !=
+      PassphraseType::KEYSTORE_PASSPHRASE) {
+    NOTIMPLEMENTED();
+    return ModelError(FROM_HERE, "Only keystore Nigori node is supported.");
+  }
+
+  DCHECK(!keystore_keys_.empty());
+  // TODO(crbug.com/922900): verify that we don't need to check that
+  // nigori.encryption_keybag() and nigori.keystore_decryptor_token() are not
+  // empty.
+  sync_pb::EncryptedData keybag = nigori.encryption_keybag();
+  if (cryptographer_.CanDecrypt(keybag)) {
+    cryptographer_.InstallKeys(keybag);
+  } else {
+    cryptographer_.SetPendingKeys(keybag);
+    base::Optional<std::string> serialized_keystore_decryptor =
+        DecryptKeystoreDecryptor(keystore_keys_,
+                                 nigori.keystore_decryptor_token(),
+                                 cryptographer_.encryptor());
+    if (!serialized_keystore_decryptor ||
+        !cryptographer_.ImportNigoriKey(*serialized_keystore_decryptor) ||
+        !cryptographer_.is_ready()) {
+      return ModelError(FROM_HERE,
+                        "Failed to decrypt pending keys using the keystore "
+                        "decryptor token.");
+    }
+  }
+  // TODO(crbug.com/922900): implement updates of other data fields (e.g.
+  // passphrase type, encrypted types).
   NOTIMPLEMENTED();
   return base::nullopt;
 }
@@ -96,4 +211,8 @@ void NigoriSyncBridgeImpl::ApplyDisableSyncChanges() {
   NOTIMPLEMENTED();
 }
 
+const Cryptographer& NigoriSyncBridgeImpl::GetCryptographerForTesting() const {
+  return cryptographer_;
+}
+
 }  // namespace syncer

components/sync/nigori/nigori_sync_bridge_impl.h

@@ -12,6 +12,7 @@
 #include "base/macros.h"
 #include "base/optional.h"
 #include "base/sequence_checker.h"
+#include "components/sync/base/cryptographer.h"
 #include "components/sync/engine/sync_encryption_handler.h"
 #include "components/sync/model/conflict_resolution.h"
 #include "components/sync/model/model_error.h"
@@ -20,6 +21,8 @@
 
 namespace syncer {
 
+class Encryptor;
+
 // USS implementation of SyncEncryptionHandler.
 // This class holds the current Nigori state and processes incoming changes and
 // queries:
@@ -32,7 +35,9 @@ class NigoriSyncBridgeImpl : public KeystoreKeysHandler,
                              public NigoriSyncBridge,
                              public SyncEncryptionHandler {
  public:
-  NigoriSyncBridgeImpl();
+  // |encryptor| must not be null and must outlive this object and any copies
+  // of the Cryptographer exposed by this object.
+  explicit NigoriSyncBridgeImpl(Encryptor* encryptor);
   ~NigoriSyncBridgeImpl() override;
 
   // SyncEncryptionHandler implementation.
@@ -57,7 +62,19 @@ class NigoriSyncBridgeImpl : public KeystoreKeysHandler,
                                      const EntityData& remote_data) override;
   void ApplyDisableSyncChanges() override;
 
+  // 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 Cryptographer& GetCryptographerForTesting() const;
+
  private:
+  // Base64 encoded keystore keys. The last element is the current keystore
+  // key. These keys are not a part of Nigori node and are persisted
+  // separately. Should be encrypted with OSCrypt before persisting.
+  std::vector<std::string> keystore_keys_;
+
+  Cryptographer cryptographer_;
+
   SEQUENCE_CHECKER(sequence_checker_);
 
   DISALLOW_COPY_AND_ASSIGN(NigoriSyncBridgeImpl);

components/sync/nigori/nigori_sync_bridge_impl_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/nigori/nigori_sync_bridge_impl.h"
+
+#include <utility>
+
+#include "base/base64.h"
+#include "components/sync/base/fake_encryptor.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace syncer {
+
+namespace {
+
+using testing::Eq;
+
+const char kNigoriKeyName[] = "nigori-key";
+
+KeyParams Pbkdf2KeyParams(std::string key) {
+  return {KeyDerivationParams::CreateForPbkdf2(), std::move(key)};
+}
+
+KeyParams KeystoreKeyParams(const std::string& key) {
+  // Due to mis-encode of keystore keys to base64 we have to always encode such
+  // keys to provide backward compatibility.
+  std::string encoded_key;
+  base::Base64Encode(key, &encoded_key);
+  return Pbkdf2KeyParams(std::move(encoded_key));
+}
+
+class NigoriSyncBridgeImplTest : public testing::Test {
+ protected:
+  NigoriSyncBridgeImplTest() {
+    bridge_ = std::make_unique<NigoriSyncBridgeImpl>(&encryptor_);
+  }
+
+  NigoriSyncBridgeImpl* bridge() { return bridge_.get(); }
+
+  // Builds NigoriSpecifics with following fields:
+  // 1. encryption_keybag contains all keys derived from |keybag_keys_params|
+  // and encrypted with a key derived from |keybag_decryptor_params|.
+  // keystore_decryptor_token is always saved in encryption_keybag, even if it
+  // is not derived from any params in |keybag_keys_params|.
+  // 2. keystore_decryptor_token contains the key derived from
+  // |keybag_decryptor_params| and encrypted with a key derived from
+  // |keystore_key_params|.
+  // 3. passphrase_type is KEYSTORE_PASSHPRASE.
+  // 4. Other fields are default.
+  sync_pb::NigoriSpecifics BuildKeystoreNigoriSpecifics(
+      const std::vector<KeyParams>& keybag_keys_params,
+      const KeyParams& keystore_decryptor_params,
+      const KeyParams& keystore_key_params) {
+    sync_pb::NigoriSpecifics specifics =
+        sync_pb::NigoriSpecifics::default_instance();
+
+    Cryptographer cryptographer(&encryptor_);
+    cryptographer.AddKey(keystore_decryptor_params);
+    for (const KeyParams& key_params : keybag_keys_params) {
+      cryptographer.AddNonDefaultKey(key_params);
+    }
+    EXPECT_TRUE(cryptographer.GetKeys(specifics.mutable_encryption_keybag()));
+
+    std::string serialized_keystore_decryptor =
+        cryptographer.GetDefaultNigoriKeyData();
+    Cryptographer keystore_cryptographer(&encryptor_);
+    keystore_cryptographer.AddKey(keystore_key_params);
+    EXPECT_TRUE(keystore_cryptographer.EncryptString(
+        serialized_keystore_decryptor,
+        specifics.mutable_keystore_decryptor_token()));
+
+    specifics.set_passphrase_type(
+        sync_pb::NigoriSpecifics::KEYSTORE_PASSPHRASE);
+    return specifics;
+  }
+
+ private:
+  // Don't change the order. |bridge_| should outlive |encryptor_|.
+  FakeEncryptor encryptor_;
+  std::unique_ptr<NigoriSyncBridgeImpl> bridge_;
+};
+
+MATCHER_P(CanDecryptWith, key_params, "") {
+  const Cryptographer& cryptographer = arg;
+  Nigori nigori;
+  nigori.InitByDerivation(key_params.derivation_params, key_params.password);
+  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()));
+
+  if (!cryptographer.CanDecrypt(encrypted)) {
+    return false;
+  }
+  std::string decrypted;
+  if (!cryptographer.DecryptToString(encrypted, &decrypted)) {
+    return false;
+  }
+  return decrypted == unencrypted;
+}
+
+MATCHER_P(HasDefaultKeyDerivedFrom, key_params, "") {
+  const Cryptographer& cryptographer = arg;
+  Nigori expected_default_nigori;
+  expected_default_nigori.InitByDerivation(key_params.derivation_params,
+                                           key_params.password);
+  std::string expected_default_key_name;
+  EXPECT_TRUE(expected_default_nigori.Permute(
+      Nigori::Type::Password, kNigoriKeyName, &expected_default_key_name));
+  return cryptographer.GetDefaultNigoriKeyName() == expected_default_key_name;
+}
+
+// Simplest case of keystore Nigori: we have only one keystore key and no old
+// keys. This keystore key is encrypted in both encryption_keybag and
+// keystore_decryptor_token. Client receives such Nigori if initialization of
+// Nigori node was done after keystore was introduced and no key rotations
+// happened.
+TEST_F(NigoriSyncBridgeImplTest, ShouldAcceptKeysFromKeystoreNigori) {
+  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);
+
+  EXPECT_TRUE(bridge()->SetKeystoreKeys({kRawKeystoreKey}));
+  EXPECT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
+              Eq(base::nullopt));
+
+  const Cryptographer& cryptographer = bridge()->GetCryptographerForTesting();
+  EXPECT_THAT(cryptographer, CanDecryptWith(kKeystoreKeyParams));
+  EXPECT_THAT(cryptographer, HasDefaultKeyDerivedFrom(kKeystoreKeyParams));
+}
+
+// Tests that client can properly process remote updates with rotated keystore
+// nigori. Cryptographer should be able to decrypt any data encrypted with any
+// keystore key and use current keystore key as default key.
+TEST_F(NigoriSyncBridgeImplTest, ShouldAcceptKeysFromRotatedKeystoreNigori) {
+  const std::string kRawOldKey = "raw_old_keystore_key";
+  const KeyParams kOldKeyParams = KeystoreKeyParams(kRawOldKey);
+  const std::string kRawCurrentKey = "raw_keystore_key";
+  const KeyParams kCurrentKeyParams = KeystoreKeyParams(kRawCurrentKey);
+  EntityData entity_data;
+  *entity_data.specifics.mutable_nigori() = BuildKeystoreNigoriSpecifics(
+      /*keybag_keys_params=*/{kOldKeyParams, kCurrentKeyParams},
+      /*keystore_decryptor_params=*/kCurrentKeyParams,
+      /*keystore_key_params=*/kCurrentKeyParams);
+
+  EXPECT_TRUE(bridge()->SetKeystoreKeys({kRawOldKey, kRawCurrentKey}));
+  EXPECT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
+              Eq(base::nullopt));
+
+  const Cryptographer& cryptographer = bridge()->GetCryptographerForTesting();
+  EXPECT_THAT(cryptographer, CanDecryptWith(kOldKeyParams));
+  EXPECT_THAT(cryptographer, CanDecryptWith(kCurrentKeyParams));
+  EXPECT_THAT(cryptographer, HasDefaultKeyDerivedFrom(kCurrentKeyParams));
+}
+
+// In the backward compatible mode keystore Nigori's keystore_decryptor_token
+// isn't a kestore key, however keystore_decryptor_token itself should be
+// encrypted with the keystore key.
+TEST_F(NigoriSyncBridgeImplTest,
+       ShouldAcceptKeysFromBackwardCompatibleKeystoreNigori) {
+  const KeyParams kGaiaKeyParams = Pbkdf2KeyParams("gaia_key");
+  const std::string kRawKeystoreKey = "raw_keystore_key";
+  const KeyParams kKeystoreKeyParams = KeystoreKeyParams(kRawKeystoreKey);
+  EntityData entity_data;
+  *entity_data.specifics.mutable_nigori() = BuildKeystoreNigoriSpecifics(
+      /*keybag_keys_params=*/{kGaiaKeyParams, kKeystoreKeyParams},
+      /*keystore_decryptor_params=*/kGaiaKeyParams,
+      /*keystore_key_params=*/kKeystoreKeyParams);
+
+  EXPECT_TRUE(bridge()->SetKeystoreKeys({kRawKeystoreKey}));
+  EXPECT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
+              Eq(base::nullopt));
+
+  const Cryptographer& cryptographer = bridge()->GetCryptographerForTesting();
+  EXPECT_THAT(cryptographer, CanDecryptWith(kGaiaKeyParams));
+  EXPECT_THAT(cryptographer, CanDecryptWith(kKeystoreKeyParams));
+  EXPECT_THAT(cryptographer, HasDefaultKeyDerivedFrom(kGaiaKeyParams));
+}
+
+// Tests that we can successfully use old keys from encryption_keybag in
+// backward compatible mode.
+TEST_F(NigoriSyncBridgeImplTest,
+       ShouldAcceptOldKeysFromBackwardCompatibleKeystoreNigori) {
+  // |kOldKeyParams| is needed to ensure we was able to decrypt
+  // encryption_keybag - there is no way to add key derived from
+  // |kOldKeyParams| to cryptographer without decrypting encryption_keybag.
+  const KeyParams kOldKeyParams = Pbkdf2KeyParams("old_key");
+  const KeyParams kCurrentKeyParams = Pbkdf2KeyParams("current_key");
+  const std::string kRawKeystoreKey = "raw_keystore_key";
+  const KeyParams kKeystoreKeyParams = KeystoreKeyParams(kRawKeystoreKey);
+  const std::vector<KeyParams> kAllKeyParams = {
+      kOldKeyParams, kCurrentKeyParams, kKeystoreKeyParams};
+  EntityData entity_data;
+  *entity_data.specifics.mutable_nigori() = BuildKeystoreNigoriSpecifics(
+      /*keybag_keys_params=*/kAllKeyParams,
+      /*keystore_decryptor_params=*/kCurrentKeyParams,
+      /*keystore_key_params=*/kKeystoreKeyParams);
+
+  EXPECT_TRUE(bridge()->SetKeystoreKeys({kRawKeystoreKey}));
+  EXPECT_THAT(bridge()->MergeSyncData(std::move(entity_data)),
+              Eq(base::nullopt));
+
+  const Cryptographer& cryptographer = bridge()->GetCryptographerForTesting();
+  for (const KeyParams& key_params : kAllKeyParams) {
+    EXPECT_THAT(cryptographer, CanDecryptWith(key_params));
+  }
+  EXPECT_THAT(cryptographer, HasDefaultKeyDerivedFrom(kCurrentKeyParams));
+}
+
+}  // namespace
+
+}  // namespace syncer