[Sync] Enable Compression from Client to Server by experiment

[Sync] Enable Compression from Client to Server by experiment

Add padding to sync protobuff for mitigating CRIME attacks.

BUG=653629
Committed: https://crrev.com/56d48222c5118c41a4233f75b627ff7d6d1ae268
Cr-Commit-Position: refs/heads/master@{#426343}

[Gang Wu, 2016-09-29 22:04:42]

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

[commit-bot: I haz the power, 2016-09-29 22:05:15]

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, 2016-09-29 22:09:11]

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

[commit-bot: I haz the power, 2016-09-29 22:09:12]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Gang Wu, 2016-09-29 22:34:28]

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

[commit-bot: I haz the power, 2016-09-29 22:34:46]

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, 2016-09-30 00:09:30]

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

[commit-bot: I haz the power, 2016-09-30 00:09:31]

Dry run: This issue passed the CQ dry run.

[Gang Wu, 2016-09-30 00:18:15]

Patchset #2 (id:20001) has been deleted

[Gang Wu, 2016-10-04 23:03:36]

Description was changed from

==========
prototype

BUG=
==========

to

==========
[Sync] Enable Compression from Client to Server by experiment

Add padding to sync protobuff for mitigating CRIME attacks.

BUG=
==========

[Gang Wu, 2016-10-06 18:30:10]

Description was changed from

==========
[Sync] Enable Compression from Client to Server by experiment

Add padding to sync protobuff for mitigating CRIME attacks.

BUG=
==========

to

==========
[Sync] Enable Compression from Client to Server by experiment

Add padding to sync protobuff for mitigating CRIME attacks.

BUG=653629
==========

[Gang Wu, 2016-10-17 21:13:08]

Patchset #3 (id:60001) has been deleted

[Gang Wu, 2016-10-17 21:22:47]

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

[commit-bot: I haz the power, 2016-10-17 21:23:27]

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

[Gang Wu, 2016-10-17 21:25:47]

Patchset #2 (id:40001) has been deleted

[commit-bot: I haz the power, 2016-10-18 00:35:00]

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

[commit-bot: I haz the power, 2016-10-18 00:35:01]

Dry run: This issue passed the CQ dry run.

[Gang Wu, 2016-10-18 17:26:41]

gangwu@chromium.org changed reviewers:
+ zea@chromium.org

[Gang Wu, 2016-10-18 17:26:42]

PTAL

[Nicolas Zea, 2016-10-18 20:23:08]

LGTM with some nits

https://codereview.chromium.org/2380143002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/commit.cc (right):

https://codereview.chromium.org/2380143002/diff/80001/components/sync/engine_...
components/sync/engine_impl/commit.cc:62:
commit_message->set_padding(base::RandBytesAsString(256));
prefer having the length defined at the top of file as a const size_t. As part
of that definition, comment why 256 was chosen.

https://codereview.chromium.org/2380143002/diff/80001/components/sync/protoco...
File components/sync/protocol/sync.proto (right):

https://codereview.chromium.org/2380143002/diff/80001/components/sync/protoco...
components/sync/protocol/sync.proto:436: // attacks when sync communicate from
client to server.
Call out that this is a 256 byte random string.

Also mention that this is explicitly for clients with compression of client to
server data enabled. If no compression is used, this should not be set.

[Gang Wu, 2016-10-18 22:14:39]

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

[commit-bot: I haz the power, 2016-10-18 22:15:19]

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, 2016-10-19 00:05:51]

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

[commit-bot: I haz the power, 2016-10-19 00:05:52]

Dry run: This issue passed the CQ dry run.

[Gang Wu, 2016-10-19 00:06:14]

https://codereview.chromium.org/2380143002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/commit.cc (right):

https://codereview.chromium.org/2380143002/diff/80001/components/sync/engine_...
components/sync/engine_impl/commit.cc:62:
commit_message->set_padding(base::RandBytesAsString(256));
On 2016/10/18 20:23:08, Nicolas Zea wrote:
> prefer having the length defined at the top of file as a const size_t. As part
> of that definition, comment why 256 was chosen.

Done.

https://codereview.chromium.org/2380143002/diff/80001/components/sync/protoco...
File components/sync/protocol/sync.proto (right):

https://codereview.chromium.org/2380143002/diff/80001/components/sync/protoco...
components/sync/protocol/sync.proto:436: // attacks when sync communicate from
client to server.
On 2016/10/18 20:23:08, Nicolas Zea wrote:
> Call out that this is a 256 byte random string.
> 
> Also mention that this is explicitly for clients with compression of client to
> server data enabled. If no compression is used, this should not be set.

Done.

[Gang Wu, 2016-10-19 00:06:27]

The CQ bit was checked by gangwu@chromium.org

[Gang Wu, 2016-10-19 00:06:28]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org
Link to the patchset: https://codereview.chromium.org/2380143002/#ps100001
(title: "comments")

[commit-bot: I haz the power, 2016-10-19 00:07:10]

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, 2016-10-19 00:28:45]

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

[commit-bot: I haz the power, 2016-10-19 00:28:47]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Gang Wu, 2016-10-19 18:02:54]

The CQ bit was checked by gangwu@chromium.org

[commit-bot: I haz the power, 2016-10-19 18:03:25]

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, 2016-10-19 18:39:14]

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

[commit-bot: I haz the power, 2016-10-19 18:39:15]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Gang Wu, 2016-10-19 18:43:19]

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

[commit-bot: I haz the power, 2016-10-19 18:44:00]

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, 2016-10-19 21:49:31]

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

[commit-bot: I haz the power, 2016-10-19 21:49:32]

Dry run: This issue passed the CQ dry run.

[Gang Wu, 2016-10-19 21:57:13]

The CQ bit was checked by gangwu@chromium.org

[Gang Wu, 2016-10-19 21:57:13]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org
Link to the patchset: https://codereview.chromium.org/2380143002/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2016-10-19 21:57:54]

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, 2016-10-19 22:33:25]

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

[commit-bot: I haz the power, 2016-10-19 22:33:26]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Gang Wu, 2016-10-19 22:35:11]

gangwu@chromium.org changed reviewers:
+ isherman@chromium.org

[Gang Wu, 2016-10-19 22:35:13]

Hi isherman,

Please review for compression.

[Ilya Sherman, 2016-10-19 23:35:56]

https://codereview.chromium.org/2380143002/diff/120001/components/sync/BUILD.gn
File components/sync/BUILD.gn (right):

https://codereview.chromium.org/2380143002/diff/120001/components/sync/BUILD....
components/sync/BUILD.gn:564: "//third_party/zlib:compression_utils",
Did you just want me to review this added dependency?  If so, LGTM.

[Gang Wu, 2016-10-19 23:38:43]

Thanks!

https://codereview.chromium.org/2380143002/diff/120001/components/sync/BUILD.gn
File components/sync/BUILD.gn (right):

https://codereview.chromium.org/2380143002/diff/120001/components/sync/BUILD....
components/sync/BUILD.gn:564: "//third_party/zlib:compression_utils",
On 2016/10/19 23:35:56, Ilya Sherman wrote:
> Did you just want me to review this added dependency?  If so, LGTM.

Yes, for this dependency.

[Gang Wu, 2016-10-19 23:38:50]

The CQ bit was checked by gangwu@chromium.org

[commit-bot: I haz the power, 2016-10-19 23:39:19]

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, 2016-10-20 00:03:52]

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

[commit-bot: I haz the power, 2016-10-20 00:03:54]

Failed to apply the patch.
On branch working_branch
Your branch is up-to-date with 'origin/refs/pending/heads/master'.
nothing to commit, working tree clean

[Gang Wu, 2016-10-20 17:24:02]

It seems the code already committed, and can see this CL in codesearch, so close
this.

[commit-bot: I haz the power, 2016-10-21 13:13:30]

Description was changed from

==========
[Sync] Enable Compression from Client to Server by experiment

Add padding to sync protobuff for mitigating CRIME attacks.

BUG=653629
==========

to

==========
[Sync] Enable Compression from Client to Server by experiment

Add padding to sync protobuff for mitigating CRIME attacks.

BUG=653629

Committed: https://crrev.com/56d48222c5118c41a4233f75b627ff7d6d1ae268
Cr-Commit-Position: refs/heads/master@{#426343}
==========

[commit-bot: I haz the power, 2016-10-21 13:13:32]

Patchset 4 (id:??) landed as
https://crrev.com/56d48222c5118c41a4233f75b627ff7d6d1ae268
Cr-Commit-Position: refs/heads/master@{#426343}