[Sync] Add intelligent re-encryption support.

chrome/browser/sync/internal_api/write_node.cc

 // Copyright (c) 2011 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 "chrome/browser/sync/internal_api/write_node.h"
 
 #include "base/json/json_writer.h"
 #include "base/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/browser/sync/engine/nigori_util.h"
@@ skipping 20 matching lines @@
 namespace sync_api {
 
 static const char kDefaultNameForNewNodes[] = " ";
 
 bool WriteNode::UpdateEntryWithEncryption(
     browser_sync::Cryptographer* cryptographer,
     const sync_pb::EntitySpecifics& new_specifics,
     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) ||
       !cryptographer->is_initialized()) {
     // No encryption required or we are unable to encrypt.
     generated_specifics.CopyFrom(new_specifics);
   } else {
     // Encrypt new_specifics into generated_specifics.
     if (VLOG_IS_ON(2)) {
       scoped_ptr<DictionaryValue> value(entry->ToValue());
       std::string info;
       base::JSONWriter::Write(value.get(), true, &info);
       DVLOG(2) << "Encrypting specifics of type "
                << syncable::ModelTypeToString(type)
                << " 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()) {
     // Overwrite the possibly sensitive non-specifics data.
     entry->Put(syncable::NON_UNIQUE_NAME, kEncryptedString);
     // For bookmarks we actually put bogus data into the unencrypted specifics,
     // else the server will try to do it for us.
     if (type == syncable::BOOKMARKS) {
       sync_pb::BookmarkSpecifics* bookmark_specifics =
           generated_specifics.MutableExtension(sync_pb::bookmark);
@@ skipping 85 matching lines @@
   entity_specifics.MutableExtension(sync_pb::nigori)->CopyFrom(new_value);
   SetEntitySpecifics(entity_specifics);
 }
 
 void WriteNode::SetPasswordSpecifics(
     const sync_pb::PasswordSpecificsData& data) {
   DCHECK_EQ(syncable::PASSWORDS, GetModelType());
 
   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;
   }
 
   sync_pb::EntitySpecifics entity_specifics;
   entity_specifics.MutableExtension(sync_pb::password)->CopyFrom(new_value);
   SetEntitySpecifics(entity_specifics);
 }
 
@@ skipping 305 matching lines @@
   sync_pb::BookmarkSpecifics new_value = GetBookmarkSpecifics();
   new_value.set_favicon(bytes.empty() ? NULL : &bytes[0], bytes.size());
   SetBookmarkSpecifics(new_value);
 }
 
 void WriteNode::MarkForSyncing() {
   syncable::MarkForSyncing(entry_);
 }
 
 } // namespace sync_api