[Sync] Improve nigori conflict and rollback handling.

We now ensure we don't change to an old default key when we receive a nigori
update. We also preserve the local explicit passphrase state if we get into
conflict with a nigori node with old encryption keys. Lastly, we write to the
node at the end of each sync cycle toensure it has the current encrypted types
and encryption keys (assuming the cryptographer is ready). This way, we'll
never lose the current encryption state due to a blank/old nigori.

BUG=119207
TEST=

Review URL: https://chromiumcodereview.appspot.com/9815018

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@128567 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 152 insertions, 27 deletions

diff --git a/sync/engine/conflict_resolver.cc b/sync/engine/conflict_resolver.cc
index 17f76a4..a365e1c 100644
--- a/sync/engine/conflict_resolver.cc
+++ b/sync/engine/conflict_resolver.cc
@@ -231,13 +231,34 @@ ConflictResolver::ProcessSimpleConflict(WriteTransaction* trans,
       // Store the merged set of encrypted types (cryptographer->Update(..) will
       // have merged the local types already).
       cryptographer->UpdateNigoriFromEncryptedTypes(server_nigori);
-      // The local set of keys is already merged with the server's set within
-      // the cryptographer. If we don't have pending keys we can store the
-      // merged set back immediately. Else we preserve the server keys and will
-      // update the nigori when the user provides the pending passphrase via
-      // SetDecryptionPassphrase(..).
+      // The cryptographer has the both the local and remote encryption keys
+      // (added at cryptographer->Update(..) time).
+      // If the cryptographer is ready, then it already merged both sets of keys
+      // and we can store them back in. In that case, the remote key was already
+      // part of the local keybag, so we preserve the local key as the default
+      // (including whether it's an explicit key).
+      // If the cryptographer is not ready, then the user will have to provide
+      // the passphrase to decrypt the pending keys. When they do so, the
+      // SetDecryptionPassphrase code will act based on whether the server
+      // update has an explicit passphrase or not.
+      // - If the server had an explicit passphrase, that explicit passphrase
+      //   will be preserved as the default encryption key.
+      // - If the server did not have an explicit passphrase, we assume the
+      //   local passphrase is the most up to date and preserve the local
+      //   default encryption key marked as an implicit passphrase.
+      // This works fine except for the case where we had locally set an
+      // explicit passphrase. In that case the nigori node will have the default
+      // key based on the local explicit passphassphrase, but will not have it
+      // marked as explicit. To fix this we'd have to track whether we have a
+      // explicit passphrase or not separate from the nigori, which would
+      // introduce even more complexity, so we leave it up to the user to
+      // reset that passphrase as an explicit one via settings. The goal here
+      // is to ensure both sets of encryption keys are preserved.
       if (cryptographer->is_ready()) {
         cryptographer->GetKeys(server_nigori->mutable_encrypted());
+        server_nigori->set_using_explicit_passphrase(
+            entry.Get(syncable::SPECIFICS).nigori().
+                using_explicit_passphrase());
       }
       // TODO(zea): Find a better way of doing this. As it stands, we have to
       // update this code whenever we add a new non-cryptographer related field
diff --git a/sync/engine/syncer_unittest.cc b/sync/engine/syncer_unittest.cc
index 9a66d98..56908df 100644
--- a/sync/engine/syncer_unittest.cc
+++ b/sync/engine/syncer_unittest.cc
@@ -943,6 +943,94 @@ TEST_F(SyncerTest, EncryptionAwareConflicts) {
 
 #undef VERIFY_ENTRY
 
+// Receive an old nigori with old encryption keys and encrypted types. We should
+// not revert our default key or encrypted types.
+TEST_F(SyncerTest, ReceiveOldNigori) {
+  KeyParams old_key = {"localhost", "dummy", "old"};
+  KeyParams current_key = {"localhost", "dummy", "cur"};
+
+  // Data for testing encryption/decryption.
+  browser_sync::Cryptographer other_cryptographer(&encryptor_);
+  other_cryptographer.AddKey(old_key);
+  sync_pb::EntitySpecifics other_encrypted_specifics;
+  other_encrypted_specifics.mutable_bookmark()->set_title("title");
+  other_cryptographer.Encrypt(
+      other_encrypted_specifics,
+      other_encrypted_specifics.mutable_encrypted());
+  sync_pb::EntitySpecifics our_encrypted_specifics;
+  our_encrypted_specifics.mutable_bookmark()->set_title("title2");
+  syncable::ModelTypeSet encrypted_types = syncable::ModelTypeSet::All();
+
+
+  // Receive the initial nigori node.
+  sync_pb::EntitySpecifics initial_nigori_specifics;
+  initial_nigori_specifics.mutable_nigori();
+  mock_server_->SetNigori(1, 10, 10, initial_nigori_specifics);
+  SyncShareAsDelegate();
+
+  {
+    // Set up the current nigori node (containing both keys and encrypt
+    // everything).
+    WriteTransaction wtrans(FROM_HERE, UNITTEST, directory());
+    sync_pb::EntitySpecifics specifics;
+    sync_pb::NigoriSpecifics* nigori = specifics.mutable_nigori();
+    cryptographer(&wtrans)->AddKey(old_key);
+    cryptographer(&wtrans)->AddKey(current_key);
+    cryptographer(&wtrans)->Encrypt(
+        our_encrypted_specifics,
+        our_encrypted_specifics.mutable_encrypted());
+    cryptographer(&wtrans)->GetKeys(
+        nigori->mutable_encrypted());
+    cryptographer(&wtrans)->set_encrypt_everything();
+    cryptographer(&wtrans)->UpdateNigoriFromEncryptedTypes(nigori);
+    MutableEntry nigori_entry(&wtrans, GET_BY_SERVER_TAG,
+                              syncable::ModelTypeToRootTag(syncable::NIGORI));
+    ASSERT_TRUE(nigori_entry.good());
+    nigori_entry.Put(SPECIFICS, specifics);
+    nigori_entry.Put(IS_UNSYNCED, true);
+    EXPECT_FALSE(cryptographer(&wtrans)->has_pending_keys());
+    EXPECT_TRUE(encrypted_types.Equals(
+        cryptographer(&wtrans)->GetEncryptedTypes()));
+  }
+
+  SyncShareAsDelegate();  // Commit it.
+
+  // Now set up the old nigori node and add it as a server update.
+  sync_pb::EntitySpecifics old_nigori_specifics;
+  sync_pb::NigoriSpecifics *old_nigori = old_nigori_specifics.mutable_nigori();
+  other_cryptographer.GetKeys(old_nigori->mutable_encrypted());
+  other_cryptographer.UpdateNigoriFromEncryptedTypes(old_nigori);
+  mock_server_->SetNigori(1, 30, 30, old_nigori_specifics);
+
+  SyncShareAsDelegate();  // Download the old nigori and apply it.
+
+  {
+    // Ensure everything is committed and stable now. The cryptographer
+    // should be able to decrypt both sets of keys and still be encrypting with
+    // the newest, and the encrypted types should be the most recent
+    ReadTransaction trans(FROM_HERE, directory());
+    Entry nigori_entry(&trans, GET_BY_SERVER_TAG,
+                       syncable::ModelTypeToRootTag(syncable::NIGORI));
+    ASSERT_TRUE(nigori_entry.good());
+    EXPECT_FALSE(nigori_entry.Get(IS_UNAPPLIED_UPDATE));
+    EXPECT_FALSE(nigori_entry.Get(IS_UNSYNCED));
+    const sync_pb::NigoriSpecifics& nigori =
+        nigori_entry.Get(SPECIFICS).nigori();
+    EXPECT_TRUE(cryptographer(&trans)->CanDecryptUsingDefaultKey(
+        our_encrypted_specifics.encrypted()));
+    EXPECT_TRUE(cryptographer(&trans)->CanDecrypt(
+        other_encrypted_specifics.encrypted()));
+    EXPECT_TRUE(cryptographer(&trans)->CanDecrypt(
+        nigori.encrypted()));
+    EXPECT_TRUE(cryptographer(&trans)->encrypt_everything());
+  }
+}
+
+// Resolve a confict between two nigori's with different encrypted types,
+// sync_tabs bits, and encryption keys (remote is explicit). Afterwards, the
+// encrypted types should be unioned, the sync_tab bit should be true, and the
+// cryptographer should have both keys and be encrypting with the remote
+// encryption key by default.
 TEST_F(SyncerTest, NigoriConflicts) {
   KeyParams local_key_params = {"localhost", "dummy", "blargle"};
   KeyParams other_key_params = {"localhost", "dummy", "foobar"};
@@ -995,6 +1083,7 @@ TEST_F(SyncerTest, NigoriConflicts) {
     nigori->set_encrypt_bookmarks(true);
     nigori->set_encrypt_preferences(true);
     nigori->set_encrypt_everything(false);
+    nigori->set_using_explicit_passphrase(true);
     mock_server_->SetNigori(1, 20, 20, specifics);
   }
 
@@ -1019,6 +1108,7 @@ TEST_F(SyncerTest, NigoriConflicts) {
             cryptographer(&wtrans)->GetEncryptedTypes()));
     EXPECT_TRUE(cryptographer(&wtrans)->encrypt_everything());
     EXPECT_TRUE(specifics.nigori().sync_tabs());
+    EXPECT_TRUE(specifics.nigori().using_explicit_passphrase());
     // Supply the pending keys. Afterwards, we should be able to decrypt both
     // our own encrypted data and data encrypted by the other cryptographer,
     // but the key provided by the other cryptographer should be the default.
@@ -1053,6 +1143,8 @@ TEST_F(SyncerTest, NigoriConflicts) {
     EXPECT_TRUE(cryptographer(&wtrans)->
         CanDecryptUsingDefaultKey(other_encrypted_specifics.encrypted()));
     EXPECT_TRUE(nigori_entry.Get(SPECIFICS).nigori().sync_tabs());
+    EXPECT_TRUE(nigori_entry.Get(SPECIFICS).nigori().
+        using_explicit_passphrase());
   }
 }
 
diff --git a/sync/util/cryptographer.cc b/sync/util/cryptographer.cc
index b3d1833..e2ab5e2 100644
--- a/sync/util/cryptographer.cc
+++ b/sync/util/cryptographer.cc
@@ -168,15 +168,13 @@ bool Cryptographer::AddKeyImpl(Nigori* initialized_nigori) {
   return true;
 }
 
-bool Cryptographer::SetKeys(const sync_pb::EncryptedData& encrypted) {
+void Cryptographer::InstallKeys(const sync_pb::EncryptedData& encrypted) {
   DCHECK(CanDecrypt(encrypted));
 
   sync_pb::NigoriKeyBag bag;
-  if (!Decrypt(encrypted, &bag)) {
-    return false;
-  }
-  InstallKeys(encrypted.key_name(), bag);
-  return true;
+  if (!Decrypt(encrypted, &bag))
+    return;
+  InstallKeyBag(bag);
 }
 
 void Cryptographer::SetPendingKeys(const sync_pb::EncryptedData& encrypted) {
@@ -207,7 +205,10 @@ bool Cryptographer::DecryptPendingKeys(const KeyParams& params) {
     NOTREACHED();
     return false;
   }
-  InstallKeys(pending_keys_->key_name(), bag);
+  InstallKeyBag(bag);
+  const std::string& new_default_key_name = pending_keys_->key_name();
+  DCHECK(nigoris_.end() != nigoris_.find(new_default_key_name));
+  default_nigori_ = &*nigoris_.find(new_default_key_name);
   pending_keys_.reset();
   return true;
 }
@@ -289,7 +290,15 @@ Cryptographer::UpdateResult Cryptographer::Update(
   UpdateEncryptedTypesFromNigori(nigori);
   if (!nigori.encrypted().blob().empty()) {
     if (CanDecrypt(nigori.encrypted())) {
-      SetKeys(nigori.encrypted());
+      InstallKeys(nigori.encrypted());
+      // We only update the default passphrase if this was a new explicit
+      // passphrase. Else, since it was decryptable, it must not have been a new
+      // key.
+      if (nigori.using_explicit_passphrase()) {
+        std::string new_default_key_name = nigori.encrypted().key_name();
+        DCHECK(nigoris_.end() != nigoris_.find(new_default_key_name));
+        default_nigori_ = &*nigoris_.find(new_default_key_name);
+      }
       return Cryptographer::SUCCESS;
     } else {
       SetPendingKeys(nigori.encrypted());
@@ -423,8 +432,7 @@ void Cryptographer::EmitEncryptedTypesChangedNotification() {
       OnEncryptedTypesChanged(encrypted_types_, encrypt_everything_));
 }
 
-void Cryptographer::InstallKeys(const std::string& default_key_name,
-                                const sync_pb::NigoriKeyBag& bag) {
+void Cryptographer::InstallKeyBag(const sync_pb::NigoriKeyBag& bag) {
   int key_size = bag.key_size();
   for (int i = 0; i < key_size; ++i) {
     const sync_pb::NigoriKey key = bag.key(i);
@@ -440,8 +448,6 @@ void Cryptographer::InstallKeys(const std::string& default_key_name,
       nigoris_[key.name()] = make_linked_ptr(new_nigori.release());
     }
   }
-  DCHECK(nigoris_.end() != nigoris_.find(default_key_name));
-  default_nigori_ = &*nigoris_.find(default_key_name);
 }
 
 }  // namespace browser_sync
diff --git a/sync/util/cryptographer.h b/sync/util/cryptographer.h
index af7b272..12f3412 100644
--- a/sync/util/cryptographer.h
+++ b/sync/util/cryptographer.h
@@ -139,11 +139,6 @@ class Cryptographer {
   // with a cryptographer that has already been initialized.
   bool AddKeyFromBootstrapToken(const std::string restored_bootstrap_token);
 
-  // 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.
-  bool SetKeys(const sync_pb::EncryptedData& encrypted);
-
   // Makes a local copy of |encrypted| to later be decrypted by
   // DecryptPendingKeys. This should only be used if CanDecrypt(encrypted) ==
   // false.
@@ -157,7 +152,8 @@ class Cryptographer {
 
   // Attempts to decrypt the set of keys that was copied in the previous call to
   // SetPendingKeys using |params|. Returns true if the pending keys were
-  // successfully decrypted and installed.
+  // successfully decrypted and installed. If successful, the default key
+  // is updated.
   bool DecryptPendingKeys(const KeyParams& params);
 
   bool is_initialized() const { return !nigoris_.empty() && default_nigori_; }
@@ -178,6 +174,10 @@ class Cryptographer {
   // This updates both the encryption keys and the set of encrypted types.
   // Returns NEEDS_PASSPHRASE if was unable to decrypt the pending keys,
   // SUCCESS otherwise.
+  // Note: will not change the default key. If the nigori's keybag
+  // is decryptable, all keys are added to the local keybag and the current
+  // default is preserved. If the nigori's keybag is not decryptable, it is
+  // stored in the |pending_keys_|.
   UpdateResult Update(const sync_pb::NigoriSpecifics& nigori);
 
   // The set of types that are always encrypted.
@@ -214,12 +214,18 @@ class Cryptographer {
 
   void EmitEncryptedTypesChangedNotification();
 
+  // 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.
+  // Does not update the default nigori.
+  void InstallKeys(const sync_pb::EncryptedData& encrypted);
+
   // Helper method to instantiate Nigori instances for each set of key
-  // parameters in |bag| and setting the default encryption key to
-  // |default_key_name|.
-  void InstallKeys(const std::string& default_key_name,
-                   const sync_pb::NigoriKeyBag& bag);
+  // parameters in |bag|.
+  // Does not update the default nigori.
+  void InstallKeyBag(const sync_pb::NigoriKeyBag& bag);
 
+  // Helper method to add a nigori as the new default nigori.
   bool AddKeyImpl(Nigori* nigori);
 
   // Functions to serialize + encrypt a Nigori object in an opaque format for