[Sync] Add intelligent re-encryption support.

chrome/browser/sync/internal_api/write_node.cc

Index: chrome/browser/sync/internal_api/write_node.cc
diff --git a/chrome/browser/sync/internal_api/write_node.cc b/chrome/browser/sync/internal_api/write_node.cc
index b088dc1a28d84fbecd2c51548eae62331b45c5d8..97f0fe0352089a707e86c283d66bfe8c7a53513b 100644
--- a/chrome/browser/sync/internal_api/write_node.cc
+++ b/chrome/browser/sync/internal_api/write_node.cc
@@ -38,6 +38,7 @@ bool WriteNode::UpdateEntryWithEncryption(
     syncable::MutableEntry* entry) {
   syncable::ModelType type = syncable::GetModelTypeFromSpecifics(new_specifics);
   DCHECK_GE(type, syncable::FIRST_REAL_MODEL_TYPE);
+  const sync_pb::EntitySpecifics& old_specifics = entry->Get(SPECIFICS);
   syncable::ModelTypeSet encrypted_types = cryptographer->GetEncryptedTypes();
   sync_pb::EntitySpecifics generated_specifics;
   if (!SpecificsNeedsEncryption(encrypted_types, new_specifics) ||
@@ -55,30 +56,35 @@ bool WriteNode::UpdateEntryWithEncryption(
                << " with content: "
                << info;
     }
-    syncable::AddDefaultExtensionValue(type, &generated_specifics);
-    if (!cryptographer->Encrypt(new_specifics,
-                                generated_specifics.mutable_encrypted())) {
+    // Only copy over the old specifics if it is of the right type and already
+    // encrypted. The first time we encrypt a node we start from scratch, hence
+    // removing all the unencrypted data, but from then on we only want to
+    // update the node if the data changes or the encryption key changes.
+    if (syncable::GetModelTypeFromSpecifics(old_specifics) == type &&

[akalin, 2011/12/06 18:28:02]

use braces for complex if-statement

[Nicolas Zea, 2011/12/06 20:45:36]

Done.

+        old_specifics.has_encrypted())
+      generated_specifics.CopyFrom(old_specifics);
+    else
+      syncable::AddDefaultExtensionValue(type, &generated_specifics);
+    // Does not change anything if underlying encrypted blob was already up
+    // to date and encrypted with the default key.
+    if (!cryptographer->EncryptIfDifferent(
+            new_specifics,
+            generated_specifics.mutable_encrypted())) {
       NOTREACHED() << "Could not encrypt data for node of type "
                    << syncable::ModelTypeToString(type);
       return false;
     }
   }
 
-  const sync_pb::EntitySpecifics& old_specifics = entry->Get(SPECIFICS);
-  if (AreSpecificsEqual(cryptographer, old_specifics, generated_specifics) &&
+  // No need to account for encryption here, EncryptIfDifferent(..) would only
+  // have modified generated_specifics if something changed.
+  if (old_specifics.SerializeAsString() ==
+          generated_specifics.SerializeAsString() &&
       (entry->Get(syncable::NON_UNIQUE_NAME) == kEncryptedString ||

[akalin, 2011/12/06 18:28:02]

Hmm do we need the 2nd && clause of this if statement?  Since we know
old_specifics == generated_specifics, doesn't this reduce to "entry is encrypted
|| entry is not encrypted"?  Maybe I'm missing something.

[Nicolas Zea, 2011/12/06 20:45:36]

We only want to enforce the non_unique_name if we're encrypted. It's possible
old_specifics == generated_specifics but not be encrypted. And it's possible
we're already encrypted but the non_unique_name was not set properly (due to an
old bug that has since been fixed). This just ensures everything is correct.

In other words it should read as "we're safe to drop the change if: the
generated specifics match the old specifics and we're not encrypted. Or the
specifics match and the non unique name is properly set to kEncryptedString."

[akalin, 2011/12/09 23:52:42]

So, if I understand this correctly, if it wasn't for that old bug on
non_unique_name, then a node is already encrypted iff non_unique_name is set
properly.  Therefore, if it wasn't for that old bug, this condition collapses to
"if (serialized-strings-are-equal && (node-is-encrypted || !node-is-encrypted))"
which is equivalent to just "if (serialized-strings-are-equal)".

Maybe the structure of the code should make the clear.  How about:

// Comment about this, possibly linking to old bug.
bool is_encrypted_node_with_improper_name =
  old_specifics.has_encrypted() && (entry->Get(syncable::NON_UNIQUE_NAME) !=
kEncryptedString);

// If we're an encrypted node, we still want to encrypt even if the serialized
strings match to set the correct name.
if (!is_encrypted_node_with_improper_name &&
  (old_specifics.SerializeAsString() ==
generated_specifics.SerializeAsString())) {
  DVLOG(...) ...;
  return true;
}

I think that's clearer.

[Nicolas Zea, 2011/12/12 20:12:26]

Done.

        !generated_specifics.has_encrypted())) {
-    // Even if the data is the same but the old specifics are encrypted with an
-    // old key, we should go ahead and re-encrypt with the new key.
-    if ((!old_specifics.has_encrypted() &&
-         !generated_specifics.has_encrypted()) ||
-         cryptographer->CanDecryptUsingDefaultKey(old_specifics.encrypted())) {
-      DVLOG(2) << "Specifics of type " << syncable::ModelTypeToString(type)
-               << " already match, dropping change.";
-      return true;
-    }
-    // TODO(zea): Add some way to keep track of how often we're reencrypting
-    // because of a passphrase change.
+    DVLOG(2) << "Specifics of type " << syncable::ModelTypeToString(type)
+             << " already match, dropping change.";
+    return true;
   }
 
   if (generated_specifics.has_encrypted()) {
@@ -184,22 +190,8 @@ void WriteNode::SetPasswordSpecifics(
 
   Cryptographer* cryptographer = GetTransaction()->GetCryptographer();
 
-  // Idempotency check to prevent unnecessary syncing: if the plaintexts match
-  // and the old ciphertext is encrypted with the most current key, there's
-  // nothing to do here.  Because each encryption is seeded with a different
-  // random value, checking for equivalence post-encryption doesn't suffice.
-  const sync_pb::EncryptedData& old_ciphertext =
-      GetEntry()->Get(SPECIFICS).GetExtension(sync_pb::password).encrypted();
-  scoped_ptr<sync_pb::PasswordSpecificsData> old_plaintext(
-      DecryptPasswordSpecifics(GetEntry()->Get(SPECIFICS), cryptographer));
-  if (old_plaintext.get() &&
-      old_plaintext->SerializeAsString() == data.SerializeAsString() &&
-      cryptographer->CanDecryptUsingDefaultKey(old_ciphertext)) {
-    return;
-  }
-
   sync_pb::PasswordSpecifics new_value;
-  if (!cryptographer->Encrypt(data, new_value.mutable_encrypted())) {
+  if (!cryptographer->EncryptIfDifferent(data, new_value.mutable_encrypted())) {
     NOTREACHED() << "Failed to encrypt password, possibly due to sync node "
                  << "corruption";
     return;