[sync] Fix decryption failure caused by missing user_key

[sync] Fix decryption failure caused by missing user_key

Recently, a change removed code that populated the user_key field of
nigori nodes. Clients running code from before this change fail to
import keys that don't have user_key populated. If a post-change
client commits a nigori node, a pre-change client will fail to
subsequently import it.
This change restores code that populates user_key, but omits code
that fails the import if the user_key is absent.

R=pavely@chromium.org
BUG=707967
Review-Url: https://codereview.chromium.org/2813453004
Cr-Commit-Position: refs/heads/master@{#463455}
Committed: https://chromium.googlesource.com/chromium/src/+/399458cbfa1d8458842e9119e6ad063716c7e66e

[Patrick Noland, 2017-04-10 19:11:33]

The CQ bit was checked by pnoland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 19:12:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Patrick Noland, 2017-04-10 19:17:25]

The CQ bit was checked by pnoland@chromium.org to run a CQ dry run

[Patrick Noland, 2017-04-10 19:17:56]

Patchset #1 (id:1) has been deleted

[Patrick Noland, 2017-04-10 19:18:10]

The CQ bit was checked by pnoland@chromium.org to run a CQ dry run

[Patrick Noland, 2017-04-10 19:18:14]

The CQ bit was checked by pnoland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 19:18:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Patrick Noland, 2017-04-10 20:14:14]

Description was changed from

==========
[sync] Fix decryption failure caused by missing user_key

Recently, a change removed code that populated the user_key field of
nigori nodes. Clients running code from before this change fail to
import keys that don't have user_key populated. If a post-change
client commits a nigori node, a pre-change client will fail to
subsequently import it.
This change restores code that populates user_key, but omits code
that fails the import if the user_key is absent.

R=pavely@chromium.org
BUG=707967
==========

to

==========
[sync] Fix decryption failure caused by missing user_key

Recently, a change removed code that populated the user_key field of
nigori nodes. Clients running code from before this change fail to
import keys that don't have user_key populated. If a post-change
client commits a nigori node, a pre-change client will fail to
subsequently import it.
This change restores code that populates user_key, but omits code
that fails the import if the user_key is absent.

R=pavely@chromium.org
BUG=707967
==========

[Patrick Noland, 2017-04-10 20:14:23]

Pavel, PTAL

[commit-bot: I haz the power, 2017-04-10 21:09:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-10 21:09:56]

Dry run: This issue passed the CQ dry run.

[pavely, 2017-04-10 21:27:29]

lgtm % three comments.

https://codereview.chromium.org/2813453004/diff/20001/components/sync/base/ni...
File components/sync/base/nigori.cc (right):

https://codereview.chromium.org/2813453004/diff/20001/components/sync/base/ni...
components/sync/base/nigori.cc:85: kDerivedKeySizeInBits);
Could you keep DCHECK and return value check for InitByDerivation as it
generates nigori to be consumed by older clients.

https://codereview.chromium.org/2813453004/diff/20001/components/sync/base/ni...
components/sync/base/nigori.cc:105: user_key_ =
SymmetricKey::Import(SymmetricKey::AES, user_key);
Could you add a comment here or in nigori.h explaining that user_key_ is not
used anywhere, but since legacy clients check for it we should keep it around.

https://codereview.chromium.org/2813453004/diff/20001/components/sync/base/ni...
File components/sync/base/nigori_unittest.cc (right):

https://codereview.chromium.org/2813453004/diff/20001/components/sync/base/ni...
components/sync/base/nigori_unittest.cc:13: TEST(SyncNigoriTest, Permute) {
Could you add tests that InitByDerivation populates user_key_ while InitByImport
doesn't fail if user_key_ is not present.

[Patrick Noland, 2017-04-10 21:40:26]

The CQ bit was checked by pnoland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 21:41:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-10 23:25:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-10 23:25:21]

Dry run: This issue passed the CQ dry run.

[Patrick Noland, 2017-04-10 23:43:06]

https://codereview.chromium.org/2813453004/diff/20001/components/sync/base/ni...
File components/sync/base/nigori.cc (right):

https://codereview.chromium.org/2813453004/diff/20001/components/sync/base/ni...
components/sync/base/nigori.cc:85: kDerivedKeySizeInBits);
On 2017/04/10 21:27:29, pavely wrote:
> Could you keep DCHECK and return value check for InitByDerivation as it
> generates nigori to be consumed by older clients. 

Done.

https://codereview.chromium.org/2813453004/diff/20001/components/sync/base/ni...
components/sync/base/nigori.cc:105: user_key_ =
SymmetricKey::Import(SymmetricKey::AES, user_key);
On 2017/04/10 21:27:29, pavely wrote:
> Could you add a comment here or in nigori.h explaining that user_key_ is not
> used anywhere, but since legacy clients check for it we should keep it around.

Done.

https://codereview.chromium.org/2813453004/diff/20001/components/sync/base/ni...
File components/sync/base/nigori_unittest.cc (right):

https://codereview.chromium.org/2813453004/diff/20001/components/sync/base/ni...
components/sync/base/nigori_unittest.cc:13: TEST(SyncNigoriTest, Permute) {
On 2017/04/10 21:27:29, pavely wrote:
> Could you add tests that InitByDerivation populates user_key_ while
InitByImport
> doesn't fail if user_key_ is not present.

Done.

[Patrick Noland, 2017-04-10 23:43:12]

The CQ bit was checked by pnoland@chromium.org

[Patrick Noland, 2017-04-10 23:43:12]

The patchset sent to the CQ was uploaded after l-g-t-m from pavely@chromium.org
Link to the patchset: https://codereview.chromium.org/2813453004/#ps40001
(title: "Restore derivation check, add tests")

[commit-bot: I haz the power, 2017-04-10 23:44:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-10 23:57:41]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491867792171520,
"parent_rev": "2edae96f04b07205ce7b3ad83af879bcec2318a2", "commit_rev":
"399458cbfa1d8458842e9119e6ad063716c7e66e"}

[commit-bot: I haz the power, 2017-04-10 23:58:32]

Description was changed from

==========
[sync] Fix decryption failure caused by missing user_key

Recently, a change removed code that populated the user_key field of
nigori nodes. Clients running code from before this change fail to
import keys that don't have user_key populated. If a post-change
client commits a nigori node, a pre-change client will fail to
subsequently import it.
This change restores code that populates user_key, but omits code
that fails the import if the user_key is absent.

R=pavely@chromium.org
BUG=707967
==========

to

==========
[sync] Fix decryption failure caused by missing user_key

Recently, a change removed code that populated the user_key field of
nigori nodes. Clients running code from before this change fail to
import keys that don't have user_key populated. If a post-change
client commits a nigori node, a pre-change client will fail to
subsequently import it.
This change restores code that populates user_key, but omits code
that fails the import if the user_key is absent.

R=pavely@chromium.org
BUG=707967

Review-Url: https://codereview.chromium.org/2813453004
Cr-Commit-Position: refs/heads/master@{#463455}
Committed:
https://chromium.googlesource.com/chromium/src/+/399458cbfa1d8458842e9119e6ad...
==========

[commit-bot: I haz the power, 2017-04-10 23:58:33]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/399458cbfa1d8458842e9119e6ad...

[Patrick Noland, 2017-04-11 16:43:03]

A revert of this CL (patchset #2 id:40001) has been created in
https://codereview.chromium.org/2809853003/ by pnoland@chromium.org.

The reason for reverting is: Causing crashes on all platforms.