[Sync] Add intelligent re-encryption support.

[Sync] Add intelligent re-encryption support.

Add an EncryptIfDifferent method that only encrypts something if the underlying
unencrypted data is either different or encrypted with an old key. Modify sync
encryption code to use this method instead of their own difference checks. 

Because of this change, we also now always ensure the nigori has the full set of
encryption keys during any SetPassphrase call

BUG=107297
TEST=sync_unit_tests, cryptographer's unit_tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114269

[Nicolas Zea, 2011-12-05 21:26:26]

PTAL

[akalin, 2011-12-06 18:28:00]

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/internal...
File chrome/browser/sync/internal_api/base_node.h (right):

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/base_node.h:211: friend class SyncApiTest;
i think the 'friend class' declarations are unnecessary

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/internal...
File chrome/browser/sync/internal_api/sync_manager.cc (right):

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/sync_manager.cc:1071: // TODO(tim): Bug 58231.
It would be nice if SetPassphrase didn't require
any reason why you moved this out of the if statement?  Maybe mention it in the
CL description.

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/sync_manager.cc:1078:
cryptographer->GetKeys(specifics.mutable_encrypted());
check return value?

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/internal...
File chrome/browser/sync/internal_api/write_node.cc (right):

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/write_node.cc:63: if
(syncable::GetModelTypeFromSpecifics(old_specifics) == type &&
use braces for complex if-statement

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/write_node.cc:83:
(entry->Get(syncable::NON_UNIQUE_NAME) == kEncryptedString ||
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.

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/util/cry...
File chrome/browser/sync/util/cryptographer.h (right):

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/util/cry...
chrome/browser/sync/util/cryptographer.h:116: bool Encrypt(const
::google::protobuf::MessageLite& message,
Is there anything now besides tests that use Encrypt(), and can those tests use
EncryptIfDifferent?  It would be nice to have a single entry point for
encryption.

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/util/cry...
chrome/browser/sync/util/cryptographer.h:129: // successful.
update comment to reflect change-only-if-different semantics

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

PTAL

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/internal...
File chrome/browser/sync/internal_api/base_node.h (right):

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/base_node.h:211: friend class SyncApiTest;
On 2011/12/06 18:28:02, akalin wrote:
> i think the 'friend class' declarations are unnecessary

One of them is, the other not. The FREIND_TEST_ALL... only friend the test
methods themselves, which doesn't apply to the test class members.

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/internal...
File chrome/browser/sync/internal_api/sync_manager.cc (right):

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/sync_manager.cc:1071: // TODO(tim): Bug 58231.
It would be nice if SetPassphrase didn't require
On 2011/12/06 18:28:02, akalin wrote:
> any reason why you moved this out of the if statement?  Maybe mention it in
the
> CL description.

To ensure we always have the full set of keys, now that we don't need to worry
about unnecessary changes. This becomes more relevant during nigori conflicts
(which are handled in a separate CL). Updated CL description.

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/sync_manager.cc:1078:
cryptographer->GetKeys(specifics.mutable_encrypted());
On 2011/12/06 18:28:02, akalin wrote:
> check return value?

Done.

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/internal...
File chrome/browser/sync/internal_api/write_node.cc (right):

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/write_node.cc:63: if
(syncable::GetModelTypeFromSpecifics(old_specifics) == type &&
On 2011/12/06 18:28:02, akalin wrote:
> use braces for complex if-statement

Done.

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/write_node.cc:83:
(entry->Get(syncable::NON_UNIQUE_NAME) == kEncryptedString ||
On 2011/12/06 18:28:02, akalin wrote:
> 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.

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."

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/util/cry...
File chrome/browser/sync/util/cryptographer.h (right):

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/util/cry...
chrome/browser/sync/util/cryptographer.h:116: bool Encrypt(const
::google::protobuf::MessageLite& message,
On 2011/12/06 18:28:02, akalin wrote:
> Is there anything now besides tests that use Encrypt(), and can those tests
use
> EncryptIfDifferent?  It would be nice to have a single entry point for
> encryption.

There isn't, but I'm not sure that we want to remove the ability to force a
re-encryption with salting.

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/util/cry...
chrome/browser/sync/util/cryptographer.h:129: // successful.
On 2011/12/06 18:28:02, akalin wrote:
> update comment to reflect change-only-if-different semantics

Done.

[Nicolas Zea, 2011-12-09 20:14:06]

ping!

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

Few more

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/internal...
File chrome/browser/sync/internal_api/base_node.h (right):

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/base_node.h:211: friend class SyncApiTest;
On 2011/12/06 20:45:36, nzea wrote:
> On 2011/12/06 18:28:02, akalin wrote:
> > i think the 'friend class' declarations are unnecessary
> 
> One of them is, the other not. The FREIND_TEST_ALL... only friend the test
> methods themselves, which doesn't apply to the test class members.

Ah, right, can you add a comment explaining why the class needs to be friended?

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/internal...
File chrome/browser/sync/internal_api/write_node.cc (right):

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/write_node.cc:83:
(entry->Get(syncable::NON_UNIQUE_NAME) == kEncryptedString ||
On 2011/12/06 20:45:36, nzea wrote:
> On 2011/12/06 18:28:02, akalin wrote:
> > 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.
> 
> 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."

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.

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/util/cry...
File chrome/browser/sync/util/cryptographer.h (right):

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/util/cry...
chrome/browser/sync/util/cryptographer.h:116: bool Encrypt(const
::google::protobuf::MessageLite& message,
On 2011/12/06 20:45:36, nzea wrote:
> On 2011/12/06 18:28:02, akalin wrote:
> > Is there anything now besides tests that use Encrypt(), and can those tests
> use
> > EncryptIfDifferent?  It would be nice to have a single entry point for
> > encryption.
> 
> There isn't, but I'm not sure that we want to remove the ability to force a
> re-encryption with salting.

That's true.  But I'd like to remove it for now; we can always add it back in if
we need to.  If we do, we should probably rename it ForceEncryption or
something.

http://codereview.chromium.org/8759019/diff/10008/chrome/browser/sync/interna...
File chrome/browser/sync/internal_api/sync_manager.cc (right):

http://codereview.chromium.org/8759019/diff/10008/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/sync_manager.cc:857:
DCHECK(cryptographer->GetKeys(nigori.mutable_encrypted()));
Change to a CHECK, or rewrite as:

if (!...) {
  NOTREACHED();
}

Otherwise, this will get compiled out in a release build!

http://codereview.chromium.org/8759019/diff/10008/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/sync_manager.cc:1073:
DCHECK(cryptographer->GetKeys(specifics.mutable_encrypted()));
here, too

http://codereview.chromium.org/8759019/diff/10008/chrome/browser/sync/interna...
File chrome/browser/sync/internal_api/syncapi_unittest.cc (right):

http://codereview.chromium.org/8759019/diff/10008/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/syncapi_unittest.cc:787: bool
SetUpEncryption(bool write_to_nigori, bool encrypt_everything) {
use enums instead of bools?

http://codereview.chromium.org/8759019/diff/10008/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/syncapi_unittest.cc:1609: // New node shouldn't
start off unsynced.
can you put newlines between the 'sections' of this test so it's easier to read?

http://codereview.chromium.org/8759019/diff/10008/chrome/browser/sync/util/cr...
File chrome/browser/sync/util/cryptographer.cc (right):

http://codereview.chromium.org/8759019/diff/10008/chrome/browser/sync/util/cr...
chrome/browser/sync/util/cryptographer.cc:76: std::string original_serialized =
DecryptToString(*encrypted);
const ref

http://codereview.chromium.org/8759019/diff/10008/chrome/browser/sync/util/cr...
File chrome/browser/sync/util/cryptographer.h (right):

http://codereview.chromium.org/8759019/diff/10008/chrome/browser/sync/util/cr...
chrome/browser/sync/util/cryptographer.h:113: // Encrypts |message| into
|encrypted|, irregardless of what was previously
irregardless -> regardless

( see http://en.wikipedia.org/wiki/Irregardless )

[Nicolas Zea, 2011-12-12 20:12:20]

All comments addressed, PTAL

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/internal...
File chrome/browser/sync/internal_api/base_node.h (right):

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/base_node.h:211: friend class SyncApiTest;
On 2011/12/09 23:52:42, akalin wrote:
> On 2011/12/06 20:45:36, nzea wrote:
> > On 2011/12/06 18:28:02, akalin wrote:
> > > i think the 'friend class' declarations are unnecessary
> > 
> > One of them is, the other not. The FREIND_TEST_ALL... only friend the test
> > methods themselves, which doesn't apply to the test class members.
> 
> Ah, right, can you add a comment explaining why the class needs to be
friended?

Done.

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/internal...
File chrome/browser/sync/internal_api/write_node.cc (right):

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/internal...
chrome/browser/sync/internal_api/write_node.cc:83:
(entry->Get(syncable::NON_UNIQUE_NAME) == kEncryptedString ||
On 2011/12/09 23:52:42, akalin wrote:
> On 2011/12/06 20:45:36, nzea wrote:
> > On 2011/12/06 18:28:02, akalin wrote:
> > > 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.
> > 
> > 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."
> 
> 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.

Done.

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/util/cry...
File chrome/browser/sync/util/cryptographer.h (right):

http://codereview.chromium.org/8759019/diff/2001/chrome/browser/sync/util/cry...
chrome/browser/sync/util/cryptographer.h:116: bool Encrypt(const
::google::protobuf::MessageLite& message,
On 2011/12/09 23:52:42, akalin wrote:
> On 2011/12/06 20:45:36, nzea wrote:
> > On 2011/12/06 18:28:02, akalin wrote:
> > > Is there anything now besides tests that use Encrypt(), and can those
tests
> > use
> > > EncryptIfDifferent?  It would be nice to have a single entry point for
> > > encryption.
> > 
> > There isn't, but I'm not sure that we want to remove the ability to force a
> > re-encryption with salting.
> 
> That's true.  But I'd like to remove it for now; we can always add it back in
if
> we need to.  If we do, we should probably rename it ForceEncryption or
> something.

Done.

http://codereview.chromium.org/8759019/diff/10008/chrome/browser/sync/interna...
File chrome/browser/sync/internal_api/sync_manager.cc (right):

http://codereview.chromium.org/8759019/diff/10008/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/sync_manager.cc:857:
DCHECK(cryptographer->GetKeys(nigori.mutable_encrypted()));
On 2011/12/09 23:52:42, akalin wrote:
> Change to a CHECK, or rewrite as:
> 
> if (!...) {
>   NOTREACHED();
> }
> 
> Otherwise, this will get compiled out in a release build!

Done.

http://codereview.chromium.org/8759019/diff/10008/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/sync_manager.cc:1073:
DCHECK(cryptographer->GetKeys(specifics.mutable_encrypted()));
On 2011/12/09 23:52:42, akalin wrote:
> here, too

Done.

http://codereview.chromium.org/8759019/diff/10008/chrome/browser/sync/interna...
File chrome/browser/sync/internal_api/syncapi_unittest.cc (right):

http://codereview.chromium.org/8759019/diff/10008/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/syncapi_unittest.cc:787: bool
SetUpEncryption(bool write_to_nigori, bool encrypt_everything) {
On 2011/12/09 23:52:42, akalin wrote:
> use enums instead of bools?

Done.

http://codereview.chromium.org/8759019/diff/10008/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/syncapi_unittest.cc:1609: // New node shouldn't
start off unsynced.
On 2011/12/09 23:52:42, akalin wrote:
> can you put newlines between the 'sections' of this test so it's easier to
read?

Done.

http://codereview.chromium.org/8759019/diff/10008/chrome/browser/sync/util/cr...
File chrome/browser/sync/util/cryptographer.cc (right):

http://codereview.chromium.org/8759019/diff/10008/chrome/browser/sync/util/cr...
chrome/browser/sync/util/cryptographer.cc:76: std::string original_serialized =
DecryptToString(*encrypted);
On 2011/12/09 23:52:42, akalin wrote:
> const ref

Done.

http://codereview.chromium.org/8759019/diff/10008/chrome/browser/sync/util/cr...
File chrome/browser/sync/util/cryptographer.h (right):

http://codereview.chromium.org/8759019/diff/10008/chrome/browser/sync/util/cr...
chrome/browser/sync/util/cryptographer.h:113: // Encrypts |message| into
|encrypted|, irregardless of what was previously
On 2011/12/09 23:52:42, akalin wrote:
> irregardless -> regardless
> 
> ( see http://en.wikipedia.org/wiki/Irregardless )

Done.

[akalin, 2011-12-12 22:05:21]

LGTM after trybots

http://codereview.chromium.org/8759019/diff/20001/chrome/browser/sync/interna...
File chrome/browser/sync/internal_api/syncapi_unittest.cc (right):

http://codereview.chromium.org/8759019/diff/20001/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/syncapi_unittest.cc:896: else
no need for 'else' since if branch returns

http://codereview.chromium.org/8759019/diff/20001/chrome/browser/sync/util/cr...
File chrome/browser/sync/util/cryptographer.cc (right):

http://codereview.chromium.org/8759019/diff/20001/chrome/browser/sync/util/cr...
chrome/browser/sync/util/cryptographer.cc:83: return EncryptImpl(serialized,
default_nigori_, encrypted);
do we still need the Encrypt/EncryptImpl divide?

[Nicolas Zea, 2011-12-13 00:43:30]

Committing once bots go green.

http://codereview.chromium.org/8759019/diff/20001/chrome/browser/sync/interna...
File chrome/browser/sync/internal_api/syncapi_unittest.cc (right):

http://codereview.chromium.org/8759019/diff/20001/chrome/browser/sync/interna...
chrome/browser/sync/internal_api/syncapi_unittest.cc:896: else
On 2011/12/12 22:05:22, akalin wrote:
> no need for 'else' since if branch returns

Done.

http://codereview.chromium.org/8759019/diff/20001/chrome/browser/sync/util/cr...
File chrome/browser/sync/util/cryptographer.cc (right):

http://codereview.chromium.org/8759019/diff/20001/chrome/browser/sync/util/cr...
chrome/browser/sync/util/cryptographer.cc:83: return EncryptImpl(serialized,
default_nigori_, encrypted);
On 2011/12/12 22:05:22, akalin wrote:
> do we still need the Encrypt/EncryptImpl divide?

Done.

[commit-bot: I haz the power, 2011-12-13 18:10:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/8759019/28001

[commit-bot: I haz the power, 2011-12-13 18:10:53]

Can't apply patch for file chrome/browser/sync/internal_api/syncapi_unittest.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/sync/internal_api/syncapi_unittest.cc
Hunk #1 FAILED at 73.
Hunk #3 succeeded at 682 with fuzz 1.
Hunk #7 succeeded at 1261 with fuzz 1.
Hunk #8 succeeded at 1292 with fuzz 1.
Hunk #9 succeeded at 1312 with fuzz 1.
1 out of 14 hunks FAILED -- saving rejects to file
chrome/browser/sync/internal_api/syncapi_unittest.cc.rej

[commit-bot: I haz the power, 2011-12-13 19:40:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/8759019/31001

[commit-bot: I haz the power, 2011-12-13 20:38:10]

Try job failure for 8759019-31001 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...