[Sync] Implement encryption-aware conflict resolution.

[Sync] Implement encryption-aware conflict resolution.

There are two primary changes in this patch.
1. Added a new syncable column PREV_SERVER_SPECIFICS. This field stores the
most recent set of decryptable SERVER_SPECIFICS iff the current
SERVER_SPECIFICS is not decryptable and only the specific have been modified.
The field gets set in process_update_command and reset in conflict_resolver
and process_update_command.

2. Due to the use of PREV_SERVER_SPECFICS, we now take into account whether
a server change actually modified anything when we perform conflict resolution.
If the current SERVER_SPECIFICS matches PREV_SERVER_SPECIFICS, irregardless of
encryption, then we can overwrite the server data with the local data. This
is possible because PREV_SERVER_SPECIFICS is only set when an update modifies
specifics only (ignoring non_unique_name and mtime), and reset if a subsequent
change modifies non-specifics data. As a result, we know that if
PREV_SERVER_SPECIFICS are set, then we can determine whether a change is
encryption only by inspecting SERVER_SPECIFICS alone.

BUG=76596, 105615
TEST=sync_unit_tests. Manually getting into a state where a passphrase is
needed but local changes are made, and then verifying that the local changes
persist after the passphrase is supplied.


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

[Nicolas Zea, 2011-12-02 01:13:49]

I'm still going through some manual test cases with this, and need clean it up a
bit, so it's not quite ready for review. But putting it out there in case you
have some initial feedback.

[Nicolas Zea, 2011-12-02 01:14:59]

Oh, and the reason I went for the full specifics vs the hash we discussed is
that I was concerned an upgrade of the protobuf version might make the hash
different, even when the underlying specifics are the same (for example if the
way a default value is encoded changes from version to version). It makes more
sense I think to perform the comparison of serialized specifics with the same
version.

[Nicolas Zea, 2011-12-06 02:27:23]

Ready for review, PTAL. Note that this patch is diffed off
http://codereview.chromium.org/8759019/.

[Nicolas Zea, 2011-12-09 22:55:12]

Ping! Went ahead and rebased as well as cleaned up some obsolete test
expectations.

[Nicolas Zea, 2011-12-14 01:15:28]

I've gone ahead and updated the patch to diff against the nigori patch I sent
you as well.

[tim (not reviewing), 2011-12-15 20:55:11]

http://codereview.chromium.org/8770032/diff/26001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/conflict_resolver.cc (right):

http://codereview.chromium.org/8770032/diff/26001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/conflict_resolver.cc:109: //    conflict encryption
and not attempting to resolve).
In the case we just discussed offline (two clients write keys atop the same base
revision), one of the two clients will win, and that client will not be able to
decrypt things the other client started re-encrypting.  Does that case
degenerate to conflict encryption where we're essentially waiting for a nigori
update to come down?  Will that happen? The other client will merge keys, update
the node, and eventually that will trickle over here?

http://codereview.chromium.org/8770032/diff/26001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/process_updates_command.cc (right):

http://codereview.chromium.org/8770032/diff/26001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/process_updates_command.cc:175:
!cryptographer->CanDecrypt(update.specifics().encrypted())) {
holy crap, complicated

http://codereview.chromium.org/8770032/diff/26001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/process_updates_command.cc:187:
target_entry.Put(syncable::PREV_SERVER_SPECIFICS, prev_specifics);
I don't really like PREV_SERVER_SPECIFICS, it's easy to confuse it with PREV as
in PREV_ID.

BASE_SERVER_SPECIFICS? ORIGINAL_

http://codereview.chromium.org/8770032/diff/26001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/process_updates_command.cc:190: IsInitialized()) {
indent

http://codereview.chromium.org/8770032/diff/26001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/process_updates_command.cc:191: // We have a
PREV_SERVER_SPECIFICS, but a subsequent non-encryption-only
how do we know it is non-encryption only?

http://codereview.chromium.org/8770032/diff/26001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_unittest.cc (right):

http://codereview.chromium.org/8770032/diff/26001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_unittest.cc:748: TEST_F(SyncerTest,
EncryptionAwareConflicts) {
I like the coverage but.. it would be _REALLY_ nice to split this into multiple
subtests :/ If something breaks it's a mess to dig through...

[Nicolas Zea, 2011-12-16 19:25:14]

Addressed the questions, will upload a patch soon that renames
PREV_SERVER_SPECIFICS to BASE_SERVER_SPECIFICS and breaks up the unit test.

http://codereview.chromium.org/8770032/diff/26001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/conflict_resolver.cc (right):

http://codereview.chromium.org/8770032/diff/26001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/conflict_resolver.cc:109: //    conflict encryption
and not attempting to resolve).
On 2011/12/15 20:55:11, timsteele wrote:
> In the case we just discussed offline (two clients write keys atop the same
base
> revision), one of the two clients will win, and that client will not be able
to
> decrypt things the other client started re-encrypting.  Does that case
> degenerate to conflict encryption where we're essentially waiting for a nigori
> update to come down?  Will that happen? The other client will merge keys,
update
> the node, and eventually that will trickle over here?

Yes, depending on the consistency at the server one of the two clients will have
the "last" version, of both the items that were encrypted and the nigori node.
We will then wait until the client that _doesn't_ have the last version of the
nigori node to reconcile the differences.

In that scenario, the client that receives an undecryptable nigori node is the
one that will need to:
a) Have the user enter the passphrase provided on the other client (this client
will be in a passphrase required state)
b) Store the updated set of encryption keys (which is a superset of all seen
encryption keys) into the nigori node. This is done in SetPassphrase.

At that point, the newest nigori node will have all encryption keys, and it
doesn't matter which client "won" in encrypting the data, both clients will be
able to decrypt all data.

http://codereview.chromium.org/8770032/diff/26001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/process_updates_command.cc (right):

http://codereview.chromium.org/8770032/diff/26001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/process_updates_command.cc:175:
!cryptographer->CanDecrypt(update.specifics().encrypted())) {
On 2011/12/15 20:55:11, timsteele wrote:
> holy crap, complicated

Yeah :-/ Is there a better way of detecting when only the specifics have changed
and are encrypted? (disregarding mtime, ctime, and non_unique_name)

http://codereview.chromium.org/8770032/diff/26001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/process_updates_command.cc:191: // We have a
PREV_SERVER_SPECIFICS, but a subsequent non-encryption-only
On 2011/12/15 20:55:11, timsteele wrote:
> how do we know it is non-encryption only?
You're right, the comment is inaccurate. This should read non-specifics-only
change. Whether it's an encryption only change or not is unknown until we
actually attempt to resolve conflicts.

[tim (not reviewing), 2011-12-20 15:45:25]

LGTM, pending BASE_ rename.

[Nicolas Zea, 2011-12-22 18:35:27]

Changes made, landing today.

http://codereview.chromium.org/8770032/diff/26001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/process_updates_command.cc (right):

http://codereview.chromium.org/8770032/diff/26001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/process_updates_command.cc:187:
target_entry.Put(syncable::PREV_SERVER_SPECIFICS, prev_specifics);
On 2011/12/15 20:55:11, timsteele wrote:
> I don't really like PREV_SERVER_SPECIFICS, it's easy to confuse it with PREV
as
> in PREV_ID.
> 
> BASE_SERVER_SPECIFICS? ORIGINAL_

Done.

http://codereview.chromium.org/8770032/diff/26001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/process_updates_command.cc:190: IsInitialized()) {
On 2011/12/15 20:55:11, timsteele wrote:
> indent

Done.

[commit-bot: I haz the power, 2011-12-22 21:14:34]

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

[commit-bot: I haz the power, 2011-12-23 01:02:34]

Change committed as 115650