Deprecate Nigori key name in proto NigoriKey

The field is redundant and consuming it can be a source for issues,
possibly vulnerabilities. Therefore, let's deprecate it and only
populate it for the cases where we require backward-compatibility.

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

components/sync/engine_impl/model_type_worker_unittest.cc

@@ -249,9 +249,9 @@ class ModelTypeWorkerTest : public ::testing::Test {
 
       sync_pb::NigoriKey* key = bag.add_key();
 
-      key->set_name(GetNigoriName(*nigori));
-      nigori->ExportKeys(key->mutable_user_key(), key->mutable_encryption_key(),
-                         key->mutable_mac_key());
+      key->set_deprecated_name(GetNigoriName(*nigori));
+      nigori->ExportKeys(key->mutable_deprecated_user_key(),
+                         key->mutable_encryption_key(), key->mutable_mac_key());
     }
 
     // Re-create the last nigori from that loop.

components/sync/engine_impl/sync_encryption_handler_impl_unittest.cc

@@ -480,8 +480,8 @@ class SyncEncryptionHandlerImplTest : public ::testing::Test {
     sync_pb::NigoriKey key;
     std::unique_ptr<Nigori> nigori =
         Nigori::CreateByDerivation(key_derivation_params, passphrase);
-    nigori->ExportKeys(key.mutable_user_key(), key.mutable_encryption_key(),
-                       key.mutable_mac_key());
+    nigori->ExportKeys(key.mutable_deprecated_user_key(),
+                       key.mutable_encryption_key(), key.mutable_mac_key());
     return key.SerializeAsString();
   }
 

components/sync/nigori/cryptographer_impl.cc

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

components/sync/nigori/cryptographer_impl.h

@@ -65,10 +65,9 @@ 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;
+  // Returns a proto representation of the default encryption key. |*this| must
+  // have a default encryption key set, as reflected by CanEncrypt().
+  sync_pb::NigoriKey ExportDefaultKey() const;
 
   // Cryptographer overrides.
   std::unique_ptr<Cryptographer> Clone() const override;

components/sync/nigori/cryptographer_impl_unittest.cc

@@ -132,7 +132,7 @@ TEST(CryptographerImplTest, ShouldSerializeToAndFromProto) {
   EXPECT_THAT(decrypted, Eq(kText2));
 }
 
-TEST(CryptographerImplTest, ShouldExportDefaultKeyWithoutName) {
+TEST(CryptographerImplTest, ShouldExportDefaultKey) {
   std::unique_ptr<CryptographerImpl> cryptographer =
       CryptographerImpl::CreateEmpty();
   ASSERT_THAT(cryptographer, NotNull());
@@ -144,9 +144,8 @@ TEST(CryptographerImplTest, ShouldExportDefaultKeyWithoutName) {
   cryptographer->SelectDefaultEncryptionKey(key_name);
   ASSERT_TRUE(cryptographer->CanEncrypt());
 
-  sync_pb::NigoriKey exported_key =
-      cryptographer->ExportDefaultKeyWithoutName();
-  EXPECT_FALSE(exported_key.has_name());
+  sync_pb::NigoriKey exported_key = cryptographer->ExportDefaultKey();
+  EXPECT_FALSE(exported_key.has_deprecated_name());
 
   // The exported key, even without name, should be importable, and the
   // resulting key name should match the original.

components/sync/nigori/nigori_key_bag.cc

@@ -26,9 +26,9 @@ sync_pb::NigoriKey NigoriToProto(const Nigori& nigori,
   DCHECK_EQ(key_name, ComputeNigoriName(nigori));
 
   sync_pb::NigoriKey proto;
-  proto.set_name(key_name);
-  nigori.ExportKeys(proto.mutable_user_key(), proto.mutable_encryption_key(),
-                    proto.mutable_mac_key());
+  proto.set_deprecated_name(key_name);
+  nigori.ExportKeys(proto.mutable_deprecated_user_key(),
+                    proto.mutable_encryption_key(), proto.mutable_mac_key());
   return proto;
 }
 
@@ -98,7 +98,12 @@ bool NigoriKeyBag::HasKey(const std::string& key_name) const {
 
 sync_pb::NigoriKey NigoriKeyBag::ExportKey(const std::string& key_name) const {
   DCHECK(HasKey(key_name));
-  return NigoriToProto(*nigori_map_.find(key_name)->second, key_name);
+  sync_pb::NigoriKey key =
+      NigoriToProto(*nigori_map_.find(key_name)->second, key_name);
+  // For exported keys, clients never consumed the key name, so it's safe to
+  // clear the deprecated field.
+  key.clear_deprecated_name();
+  return key;
 }
 
 std::string NigoriKeyBag::AddKey(std::unique_ptr<Nigori> nigori) {
@@ -114,7 +119,7 @@ std::string NigoriKeyBag::AddKey(std::unique_ptr<Nigori> nigori) {
 
 std::string NigoriKeyBag::AddKeyFromProto(const sync_pb::NigoriKey& key) {
   std::unique_ptr<Nigori> nigori = Nigori::CreateByImport(
-      key.user_key(), key.encryption_key(), key.mac_key());
+      key.deprecated_user_key(), key.encryption_key(), key.mac_key());
   if (!nigori) {
     return std::string();
   }

components/sync/nigori/nigori_key_bag_unittest.cc

@@ -44,6 +44,24 @@ TEST(NigoriKeyBagTest, ShouldAddKeys) {
   EXPECT_TRUE(key_bag.HasKey(key_name2));
 }
 
+TEST(NigoriKeyBagTest, ShouldExportKey) {
+  NigoriKeyBag key_bag = NigoriKeyBag::CreateEmpty();
+  const std::string key_name1 = key_bag.AddKey(CreateTestNigori("password1"));
+  ASSERT_THAT(key_bag, SizeIs(1));
+  ASSERT_THAT(key_name1, Ne(""));
+  ASSERT_TRUE(key_bag.HasKey(key_name1));
+
+  sync_pb::NigoriKey exported_key = key_bag.ExportKey(key_name1);
+
+  // Callers of ExportKey() rely on the deprecated field *NOT* being populated.
+  EXPECT_FALSE(exported_key.has_deprecated_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_name1));
+}
+
 TEST(NigoriKeyBagTest, ShouldConvertEmptyToProto) {
   EXPECT_EQ(sync_pb::NigoriKeyBag().SerializeAsString(),
             NigoriKeyBag::CreateEmpty().ToProto().SerializeAsString());
@@ -55,8 +73,11 @@ TEST(NigoriKeyBagTest, ShouldConvertNonEmptyToProto) {
 
   sync_pb::NigoriKeyBag proto = key_bag.ToProto();
   ASSERT_THAT(proto.key(), SizeIs(1));
-  EXPECT_THAT(proto.key(0).name(), Eq(key_name));
-  EXPECT_THAT(proto.key(0).user_key(), Ne(""));
+  // Callers of ToProto() rely on the deprecated name field being populated,
+  // since it gets exposed to the sync protocol, and hence subject to backward
+  // compatibility.
+  EXPECT_THAT(proto.key(0).deprecated_name(), Eq(key_name));
+  EXPECT_THAT(proto.key(0).deprecated_user_key(), Ne(""));
   EXPECT_THAT(proto.key(0).encryption_key(), Ne(""));
   EXPECT_THAT(proto.key(0).mac_key(), Ne(""));
 }
@@ -114,7 +135,9 @@ TEST(NigoriKeyBagTest, ShouldClone) {
   EXPECT_TRUE(cloned_key_bag.HasKey(key_name2));
 }
 
-TEST(NigoriKeyBagTest, ShouldIgnoreKeyNameProtoField) {
+// This holds true for M79 and above, but older clients rely on the field being
+// set.
+TEST(NigoriKeyBagTest, ShouldIgnoreDeprecatedKeyNameProtoField) {
   NigoriKeyBag original_key_bag = NigoriKeyBag::CreateEmpty();
   const std::string real_key_name =
       original_key_bag.AddKey(CreateTestNigori("password1"));
@@ -124,7 +147,7 @@ TEST(NigoriKeyBagTest, ShouldIgnoreKeyNameProtoField) {
       NigoriKeyBag::CreateEmpty().AddKey(CreateTestNigori("password2"));
 
   sync_pb::NigoriKeyBag proto = original_key_bag.ToProto();
-  proto.mutable_key(0)->set_name(actual_key_name_in_proto);
+  proto.mutable_key(0)->set_deprecated_name(actual_key_name_in_proto);
 
   NigoriKeyBag restored_key_bag = NigoriKeyBag::CreateFromProto(proto);
 

components/sync/nigori/nigori_sync_bridge_impl.cc

@@ -78,8 +78,7 @@ bool EncryptKeystoreDecryptorToken(
     sync_pb::EncryptedData* keystore_decryptor_token) {
   DCHECK(keystore_decryptor_token);
 
-  const sync_pb::NigoriKey default_key =
-      cryptographer.ExportDefaultKeyWithoutName();
+  const sync_pb::NigoriKey default_key = cryptographer.ExportDefaultKey();
 
   return cryptographer.EncryptString(default_key.SerializeAsString(),
                                      keystore_decryptor_token);
@@ -440,7 +439,7 @@ std::string PackExplicitPassphraseKey(const Encryptor& encryptor,
 
   // Explicit passphrase key should always be default one.
   std::string serialized_key =
-      cryptographer.ExportDefaultKeyWithoutName().SerializeAsString();
+      cryptographer.ExportDefaultKey().SerializeAsString();
 
   if (serialized_key.empty()) {
     DLOG(ERROR) << "Failed to serialize explicit passphrase key.";

components/sync/nigori/nigori_sync_bridge_impl_unittest.cc

@@ -183,7 +183,7 @@ sync_pb::NigoriSpecifics BuildKeystoreNigoriSpecifics(
                                      specifics.mutable_encryption_keybag()));
 
   std::string serialized_keystore_decryptor =
-      cryptographer->ExportDefaultKeyWithoutName().SerializeAsString();
+      cryptographer->ExportDefaultKey().SerializeAsString();
 
   std::unique_ptr<CryptographerImpl> keystore_cryptographer =
       CryptographerImpl::FromSingleKeyForTesting(

components/sync/protocol/nigori_specifics.proto

@@ -19,8 +19,10 @@ package sync_pb;
 import "encryption.proto";
 
 message NigoriKey {
-  optional string name = 1;
-  optional bytes user_key = 2 [deprecated = true];
+  // Note that M78 and before rely on the name being populated, at least for
+  // the main encrypted keybag within NigoriSpecifics.
+  optional string deprecated_name = 1 [deprecated = true];
+  optional bytes deprecated_user_key = 2 [deprecated = true];
   optional bytes encryption_key = 3;
   optional bytes mac_key = 4;
 }

components/sync/syncable/directory_cryptographer.cc

@@ -321,9 +321,7 @@ std::string DirectoryCryptographer::GetDefaultEncryptionKeyName() const {
 std::string DirectoryCryptographer::GetDefaultNigoriKeyData() const {
   if (!is_initialized())
     return std::string();
-  sync_pb::NigoriKey key = key_bag_.ExportKey(default_nigori_name_);
-  key.clear_name();
-  return key.SerializeAsString();
+  return key_bag_.ExportKey(default_nigori_name_).SerializeAsString();
 }
 
 bool DirectoryCryptographer::ImportNigoriKey(
@@ -336,7 +334,7 @@ bool DirectoryCryptographer::ImportNigoriKey(
     return false;
 
   std::unique_ptr<Nigori> nigori = Nigori::CreateByImport(
-      key.user_key(), key.encryption_key(), key.mac_key());
+      key.deprecated_user_key(), key.encryption_key(), key.mac_key());
 
   if (!nigori) {
     DLOG(ERROR) << "Ignoring invalid Nigori when importing";

components/sync/syncable/directory_cryptographer_unittest.cc

@@ -320,10 +320,11 @@ TEST_F(DirectoryCryptographerTest,
       ->Permute(Nigori::Password, kNigoriKeyName, &expected_key_name);
   EXPECT_THAT(serialized.cryptographer_data.default_key_name(),
               Eq(expected_key_name));
-  EXPECT_THAT(serialized.cryptographer_data.key_bag().key(0).name(),
+  EXPECT_THAT(serialized.cryptographer_data.key_bag().key(0).deprecated_name(),
               Eq(expected_key_name));
-  EXPECT_THAT(serialized.cryptographer_data.key_bag().key(0).user_key(),
-              Ne(""));
+  EXPECT_THAT(
+      serialized.cryptographer_data.key_bag().key(0).deprecated_user_key(),
+      Ne(""));
   EXPECT_THAT(serialized.cryptographer_data.key_bag().key(0).encryption_key(),
               Ne(""));
   EXPECT_THAT(serialized.cryptographer_data.key_bag().key(0).mac_key(), Ne(""));