Expose last keystore key name from KeystoreKeysCryptographer

Exposing last keystore key name allows the following:
1. Avoiding unnecessary copies of underlying CryptographerImpl in
NigoriState::NeedsKeystoreKeyRotation().
2. Removing ComputePbkdf2KeyName() from nigori_state.cc
3. Exposing HasKey(key_name) instead of HasKey(key_proto) from
CryptographerImpl.

This CL also adds unittests for KeystoreKeysCryptographer.

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

components/sync/BUILD.gn

@@ -650,6 +650,7 @@ source_set("unit_tests") {
     "model_impl/processor_entity_unittest.cc",
     "model_impl/syncable_service_based_bridge_unittest.cc",
     "nigori/cryptographer_impl_unittest.cc",
+    "nigori/keystore_keys_cryptographer_unittest.cc",
     "nigori/nigori_key_bag_unittest.cc",
     "nigori/nigori_model_type_processor_unittest.cc",
     "nigori/nigori_state_unittest.cc",

components/sync/nigori/cryptographer_impl.cc

@@ -83,8 +83,8 @@ void CryptographerImpl::ClearDefaultEncryptionKey() {
   default_encryption_key_name_.clear();
 }
 
-bool CryptographerImpl::HasKey(const sync_pb::NigoriKey& key) const {
-  return key_bag_.HasKey(key);
+bool CryptographerImpl::HasKey(const std::string& key_name) const {
+  return key_bag_.HasKey(key_name);
 }
 
 sync_pb::NigoriKey CryptographerImpl::ExportDefaultKey() const {

components/sync/nigori/cryptographer_impl.h

@@ -65,8 +65,8 @@ class CryptographerImpl : public Cryptographer {
   // false.
   void ClearDefaultEncryptionKey();
 
-  // Determines whether |key| is already known.
-  bool HasKey(const sync_pb::NigoriKey& key) const;
+  // Determines whether |key_name| represents a known key.
+  bool HasKey(const std::string& key_name) const;
 
   // Returns a proto representation of the default encryption key. |*this| must
   // have a default encryption key set, as reflected by CanEncrypt().

components/sync/nigori/keystore_keys_cryptographer.cc

@@ -13,18 +13,27 @@
 
 namespace syncer {
 
-namespace {
+// static
+std::unique_ptr<KeystoreKeysCryptographer>
+KeystoreKeysCryptographer::CreateEmpty() {
+  return base::WrapUnique(new KeystoreKeysCryptographer(
+      CryptographerImpl::CreateEmpty(),
+      /*keystore_keys=*/std::vector<std::string>()));
+}
 
-std::unique_ptr<CryptographerImpl> CreateCryptographerFromKeystoreKeys(
+// static
+std::unique_ptr<KeystoreKeysCryptographer>
+KeystoreKeysCryptographer::FromKeystoreKeys(
     const std::vector<std::string>& keystore_keys) {
-  std::unique_ptr<CryptographerImpl> cryptographer =
-      CryptographerImpl::CreateEmpty();
-
   if (keystore_keys.empty()) {
-    return cryptographer;
+    return CreateEmpty();
   }
 
+  std::unique_ptr<CryptographerImpl> cryptographer =
+      CryptographerImpl::CreateEmpty();
+
   std::string last_key_name;
+
   for (const std::string& key : keystore_keys) {
     last_key_name =
         cryptographer->EmplaceKey(key, KeyDerivationParams::CreateForPbkdf2());
@@ -40,28 +49,6 @@ std::unique_ptr<CryptographerImpl> CreateCryptographerFromKeystoreKeys(
   DCHECK(!last_key_name.empty());
   cryptographer->SelectDefaultEncryptionKey(last_key_name);
 
-  return cryptographer;
-}
-
-}  // namespace
-
-// static
-std::unique_ptr<KeystoreKeysCryptographer>
-KeystoreKeysCryptographer::CreateEmpty() {
-  return base::WrapUnique(new KeystoreKeysCryptographer(
-      CryptographerImpl::CreateEmpty(),
-      /*keystore_keys=*/std::vector<std::string>()));
-}
-
-// static
-std::unique_ptr<KeystoreKeysCryptographer>
-KeystoreKeysCryptographer::FromKeystoreKeys(
-    const std::vector<std::string>& keystore_keys) {
-  std::unique_ptr<CryptographerImpl> cryptographer =
-      CreateCryptographerFromKeystoreKeys(keystore_keys);
-  if (!cryptographer) {
-    return nullptr;
-  }
   return base::WrapUnique(
       new KeystoreKeysCryptographer(std::move(cryptographer), keystore_keys));
 }
@@ -75,6 +62,10 @@ KeystoreKeysCryptographer::KeystoreKeysCryptographer(
 
 KeystoreKeysCryptographer::~KeystoreKeysCryptographer() = default;
 
+std::string KeystoreKeysCryptographer::GetLastKeystoreKeyName() const {
+  return cryptographer_->GetDefaultEncryptionKeyName();
+}
+
 bool KeystoreKeysCryptographer::IsEmpty() const {
   return keystore_keys_.empty();
 }

components/sync/nigori/keystore_keys_cryptographer.h

@@ -38,6 +38,10 @@ class KeystoreKeysCryptographer {
     return keystore_keys_;
   }
 
+  // Returns name of Nigori key derived from last keystore key if !IsEmpty()
+  // and empty string otherwise.
+  std::string GetLastKeystoreKeyName() const;
+
   bool IsEmpty() const;
 
   std::unique_ptr<KeystoreKeysCryptographer> Clone() const;

components/sync/nigori/keystore_keys_cryptographer_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/keystore_keys_cryptographer.h"
+
+#include "components/sync/nigori/cryptographer_impl.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::Eq;
+using testing::NotNull;
+
+std::string ComputeKeystoreKeyName(const std::string& keystore_key) {
+  std::string key_name;
+  Nigori::CreateByDerivation(KeyDerivationParams::CreateForPbkdf2(),
+                             keystore_key)
+      ->Permute(Nigori::Password, kNigoriKeyName, &key_name);
+  return key_name;
+}
+
+TEST(KeystoreKeysCryptographerTest, ShouldCreateEmpty) {
+  std::unique_ptr<KeystoreKeysCryptographer> keystore_keys_cryptographer =
+      KeystoreKeysCryptographer::CreateEmpty();
+
+  EXPECT_TRUE(keystore_keys_cryptographer->IsEmpty());
+  EXPECT_TRUE(keystore_keys_cryptographer->keystore_keys().empty());
+  EXPECT_TRUE(keystore_keys_cryptographer->GetLastKeystoreKeyName().empty());
+
+  std::unique_ptr<CryptographerImpl> underlying_cryptographer =
+      keystore_keys_cryptographer->ToCryptographerImpl();
+  ASSERT_THAT(underlying_cryptographer, NotNull());
+  EXPECT_FALSE(underlying_cryptographer->CanEncrypt());
+}
+
+TEST(KeystoreKeysCryptographerTest, ShouldCreateNonEmpty) {
+  const std::vector<std::string> kKeystoreKeys = {"key1", "key2"};
+  const std::string keystore_key_name1 =
+      ComputeKeystoreKeyName(kKeystoreKeys[0]);
+  const std::string keystore_key_name2 =
+      ComputeKeystoreKeyName(kKeystoreKeys[1]);
+  std::unique_ptr<KeystoreKeysCryptographer> keystore_keys_cryptographer =
+      KeystoreKeysCryptographer::FromKeystoreKeys(kKeystoreKeys);
+
+  EXPECT_FALSE(keystore_keys_cryptographer->IsEmpty());
+  EXPECT_THAT(keystore_keys_cryptographer->keystore_keys(), Eq(kKeystoreKeys));
+  EXPECT_THAT(keystore_keys_cryptographer->GetLastKeystoreKeyName(),
+              Eq(keystore_key_name2));
+
+  std::unique_ptr<CryptographerImpl> underlying_cryptographer =
+      keystore_keys_cryptographer->ToCryptographerImpl();
+  ASSERT_THAT(underlying_cryptographer, NotNull());
+  EXPECT_TRUE(underlying_cryptographer->CanEncrypt());
+  EXPECT_TRUE(underlying_cryptographer->HasKey(keystore_key_name1));
+  EXPECT_THAT(underlying_cryptographer->GetDefaultEncryptionKeyName(),
+              Eq(keystore_key_name2));
+}
+
+}  // namespace
+
+}  // namespace syncer

components/sync/nigori/nigori_key_bag.cc

@@ -96,12 +96,6 @@ bool NigoriKeyBag::HasKey(const std::string& key_name) const {
   return nigori_map_.count(key_name) != 0;
 }
 
-bool NigoriKeyBag::HasKey(const sync_pb::NigoriKey& key) const {
-  std::unique_ptr<Nigori> nigori = Nigori::CreateByImport(
-      key.deprecated_user_key(), key.encryption_key(), key.mac_key());
-  return HasKey(ComputeNigoriName(*nigori));
-}
-
 sync_pb::NigoriKey NigoriKeyBag::ExportKey(const std::string& key_name) const {
   DCHECK(HasKey(key_name));
   sync_pb::NigoriKey key =

components/sync/nigori/nigori_key_bag.h

@@ -40,7 +40,6 @@ class NigoriKeyBag {
 
   size_t size() const;
   bool HasKey(const std::string& key_name) const;
-  bool HasKey(const sync_pb::NigoriKey& key) const;
 
   // |key_name| must exist in this keybag.
   sync_pb::NigoriKey ExportKey(const std::string& key_name) const;

components/sync/nigori/nigori_state.cc

@@ -16,13 +16,6 @@ namespace syncer {
 
 namespace {
 
-std::string ComputePbkdf2KeyName(const std::string& password) {
-  std::string key_name;
-  Nigori::CreateByDerivation(KeyDerivationParams::CreateForPbkdf2(), password)
-      ->Permute(Nigori::Password, kNigoriKeyName, &key_name);
-  return key_name;
-}
-
 sync_pb::CustomPassphraseKeyDerivationParams
 CustomPassphraseKeyDerivationParamsToProto(const KeyDerivationParams& params) {
   sync_pb::CustomPassphraseKeyDerivationParams output;
@@ -181,11 +174,9 @@ sync_pb::NigoriModel NigoriState::ToLocalProto() const {
   if (pending_keys.has_value()) {
     *proto.mutable_pending_keys() = *pending_keys;
   }
-  const std::vector<std::string>& keystore_keys =
-      keystore_keys_cryptographer->keystore_keys();
-  if (!keystore_keys.empty()) {
+  if (!keystore_keys_cryptographer->IsEmpty()) {
     proto.set_current_keystore_key_name(
-        ComputePbkdf2KeyName(keystore_keys.back()));
+        keystore_keys_cryptographer->GetLastKeystoreKeyName());
   }
   proto.set_passphrase_type(passphrase_type);
   if (!keystore_migration_time.is_null()) {
@@ -212,7 +203,8 @@ sync_pb::NigoriModel NigoriState::ToLocalProto() const {
   // allow rollback of USS Nigori. Having keybag with all keystore keys and
   // |current_keystore_key_name| is enough to support all logic. We should
   // remove them few milestones after USS migration completed.
-  for (const std::string& keystore_key : keystore_keys) {
+  for (const std::string& keystore_key :
+       keystore_keys_cryptographer->keystore_keys()) {
     proto.add_keystore_key(keystore_key);
   }
   if (pending_keystore_decryptor_token.has_value()) {
@@ -290,15 +282,11 @@ NigoriState NigoriState::Clone() const {
 }
 
 bool NigoriState::NeedsKeystoreKeyRotation() const {
-  if (keystore_keys_cryptographer->IsEmpty() ||
-      passphrase_type != sync_pb::NigoriSpecifics::KEYSTORE_PASSPHRASE ||
-      pending_keys.has_value()) {
-    return false;
-  }
-
-  const sync_pb::NigoriKey rotated_default_key =
-      keystore_keys_cryptographer->ToCryptographerImpl()->ExportDefaultKey();
-  return !cryptographer->HasKey(rotated_default_key);
+  return !keystore_keys_cryptographer->IsEmpty() &&
+         passphrase_type == sync_pb::NigoriSpecifics::KEYSTORE_PASSPHRASE &&
+         !pending_keys.has_value() &&
+         !cryptographer->HasKey(
+             keystore_keys_cryptographer->GetLastKeystoreKeyName());
 }
 
 }  // namespace syncer