Implement component cloud policy signature validation

Implement component cloud policy signature validation

This adds signature validation for the component cloud policy
(e.g. policy for extensions). The signature is validated against
the same key that is used for the "superior" policy (e.g. the
user policy, the device local account policy, etc.).

This CL also adds keeping a copy of the most recent component
cloud policy and rechecking it when some of the credentials
change. This allows to handle key rotations gracefully: even
though the component cloud policy may fail the validation
immediately after the cloud policy refresh with the rotated key,
it will be re-validated and applied when the superior policy gets
processed and the credentials get propagated.

BUG=644632
TEST=existing tests (now with the signature checks enabled),
     new unit tests and new browser test

Committed: https://crrev.com/4df0d565e3cd9258f0a5fc4517bfb739a7b28d4f
Cr-Commit-Position: refs/heads/master@{#434730}

[emaxx, 2016-11-09 20:41:30]

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

[commit-bot: I haz the power, 2016-11-09 20:41:53]

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-11-09 21:26:35]

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

[commit-bot: I haz the power, 2016-11-09 21:26:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[emaxx, 2016-11-09 22:16:55]

emaxx@chromium.org changed reviewers:
+ atwilson@chromium.org

[emaxx, 2016-11-09 22:16:56]

Drew, PTAL.

This is the second part of the bigger change. This CL relies on work that is
being reviewed in http://crrev.com/2488573003 , which is adding the ways of
obtaining credentials from user/device/etc. cloud policy stores.


There's an open question regarding the new browser test in this CL. Also I'd
like to tackle one security issue in a separate CL. Please see my comments
below.

https://codereview.chromium.org/2493603002/diff/1/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud/component_cloud_policy_browsertest.cc (right):

https://codereview.chromium.org/2493603002/diff/1/chrome/browser/policy/cloud...
chrome/browser/policy/cloud/component_cloud_policy_browsertest.cc:330:
EXPECT_LT(public_key_version, new_public_key_version);
This new test is failing here due to the policy test server not performing key
rotation: http://crbug.com/663870 .
I'd like to leave this test in the review as is until there's a feedback for
that issue.

https://codereview.chromium.org/2493603002/diff/1/components/policy/core/comm...
File components/policy/core/common/cloud/component_cloud_policy_service.cc
(right):

https://codereview.chromium.org/2493603002/diff/1/components/policy/core/comm...
components/policy/core/common/cloud/component_cloud_policy_service.cc:247: //
TODO(emaxx): This is insecure, as it happens before the policy validation.
I'd like to tackle this issue in a separate CL.
This issue was present in the original code as well, I just moved this piece of
code from ComponentCloudPolicyService::Backend::SetCurrentPolicies.

[emaxx, 2016-11-11 16:14:05]

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

[commit-bot: I haz the power, 2016-11-11 16:14:20]

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-11-11 16:26:57]

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

[commit-bot: I haz the power, 2016-11-11 16:26:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[emaxx, 2016-11-11 16:55:23]

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

[commit-bot: I haz the power, 2016-11-11 16:55:45]

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-11-11 17:37:36]

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

[commit-bot: I haz the power, 2016-11-11 17:37:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[emaxx, 2016-11-11 17:39:07]

Removed passing of the owning domain as per http://crrev.com/2488573003 and
http://crrev.com/2494843002 .

[emaxx, 2016-11-21 15:20:20]

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

[commit-bot: I haz the power, 2016-11-21 15:20:35]

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-11-21 15:50:24]

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

[commit-bot: I haz the power, 2016-11-21 15:50:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[emaxx, 2016-11-21 16:33:35]

Drew, friendly ping.
I've updated the CL to rebase on top of changes in other CLs.

[emaxx, 2016-11-24 16:47:51]

Description was changed from

==========
Implement component cloud policy signature validation

This adds signature validation for the component cloud policy
(e.g. policy for extensions). The signature is validated against
the same key that is used for the "superior" policy (e.g. the
user policy, the device local account policy, etc.).

This CL also adds keeping a copy of the most recent component
cloud policy and rechecking it when some of the credentials
change. This allows to handle key rotations gracefully: even
though the component cloud policy may fail the validation
immediately after the cloud policy refresh with the rotated key,
it will be re-validated and applied when the superior policy gets
processed and the credentials get propagated.

BUG=644632
TEST=new unit tests, new browser test
==========

to

==========
Implement component cloud policy signature validation

This adds signature validation for the component cloud policy
(e.g. policy for extensions). The signature is validated against
the same key that is used for the "superior" policy (e.g. the
user policy, the device local account policy, etc.).

This CL also adds keeping a copy of the most recent component
cloud policy and rechecking it when some of the credentials
change. This allows to handle key rotations gracefully: even
though the component cloud policy may fail the validation
immediately after the cloud policy refresh with the rotated key,
it will be re-validated and applied when the superior policy gets
processed and the credentials get propagated.

BUG=644632
TEST=existing tests (now with the added signature checks), new unit tests and
new browser test
==========

[emaxx, 2016-11-24 16:50:50]

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

[commit-bot: I haz the power, 2016-11-24 16:51:05]

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

[emaxx, 2016-11-24 16:52:55]

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

[commit-bot: I haz the power, 2016-11-24 16:53:14]

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

[emaxx, 2016-11-24 16:54:31]

Rebased again after the first half has landed (
http://crrev.com/cc11f1e65bc21e264ec3a763e9c0d36f2d9025b1 ) has landed.

I also disabled temporarily one of the new tests until the corresponding issue
with the policy test server is fixed ( http://crbug.com/663870 ).

[commit-bot: I haz the power, 2016-11-24 17:43:45]

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

[commit-bot: I haz the power, 2016-11-24 17:43:46]

Dry run: Try jobs failed on following builders:
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[emaxx, 2016-11-24 18:02:24]

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

[commit-bot: I haz the power, 2016-11-24 18:02:50]

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-11-24 18:17:43]

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

[commit-bot: I haz the power, 2016-11-24 18:17:44]

Dry run: This issue passed the CQ dry run.

[emaxx, 2016-11-24 21:04:08]

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

[commit-bot: I haz the power, 2016-11-24 21:04:24]

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-11-24 21:29:48]

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

[commit-bot: I haz the power, 2016-11-24 21:29:50]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[emaxx, 2016-11-24 21:31:26]

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

[commit-bot: I haz the power, 2016-11-24 21:31:54]

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-11-24 22:02:49]

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

[commit-bot: I haz the power, 2016-11-24 22:02:50]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[emaxx, 2016-11-24 22:03:46]

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

[commit-bot: I haz the power, 2016-11-24 22:04: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-11-24 23:06:12]

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

[commit-bot: I haz the power, 2016-11-24 23:06:14]

Dry run: This issue passed the CQ dry run.

[emaxx, 2016-11-24 23:25:31]

Enabled test back due to rebasing on top http://crrev.com/2530023002 .

[Andrew T Wilson (Slow), 2016-11-25 16:13:24]

LGTM with nits/comments

https://codereview.chromium.org/2493603002/diff/120001/components/policy/core...
File components/policy/core/common/cloud/component_cloud_policy_service.cc
(right):

https://codereview.chromium.org/2493603002/diff/120001/components/policy/core...
components/policy/core/common/cloud/component_cloud_policy_service.cc:86: void
ClearCache();
Useful to document what this does/when it should be invoked.

https://codereview.chromium.org/2493603002/diff/120001/components/policy/core...
components/policy/core/common/cloud/component_cloud_policy_service.cc:116: void
UpdateWithMostRecentPolicies();
Document this?

https://codereview.chromium.org/2493603002/diff/120001/components/policy/core...
components/policy/core/common/cloud/component_cloud_policy_service.cc:245: //
TODO(emaxx): This is insecure, as it happens before the policy validation.
Add a bug for this and put a link to the bug with the TODO

https://codereview.chromium.org/2493603002/diff/120001/components/policy/core...
File
components/policy/core/common/cloud/component_cloud_policy_service_unittest.cc
(right):

https://codereview.chromium.org/2493603002/diff/120001/components/policy/core...
components/policy/core/common/cloud/component_cloud_policy_service_unittest.cc:8:
#include <map>
blank line between C and stl includes?

https://codereview.chromium.org/2493603002/diff/120001/components/policy/core...
File components/policy/core/common/cloud/component_cloud_policy_store.cc
(right):

https://codereview.chromium.org/2493603002/diff/120001/components/policy/core...
components/policy/core/common/cloud/component_cloud_policy_store.cc:299:
base::Time time_not_before;
comment what we're validating here

https://codereview.chromium.org/2493603002/diff/120001/components/policy/core...
components/policy/core/common/cloud/component_cloud_policy_store.cc:354:
policy_data->Swap(validator->policy_data().get());
This is fine, but why did we move this around?

https://codereview.chromium.org/2493603002/diff/120001/components/policy/core...
File
components/policy/core/common/cloud/component_cloud_policy_store_unittest.cc
(right):

https://codereview.chromium.org/2493603002/diff/120001/components/policy/core...
components/policy/core/common/cloud/component_cloud_policy_store_unittest.cc:292:
"wrongtoken",
This is fine, but I note it just tests the case where *everything* is wrong - it
doesn't check that having a single incorrect credential (i.e. wrong device id)
causes validation to fail.

https://codereview.chromium.org/2493603002/diff/120001/components/policy/core...
File
components/policy/core/common/cloud/component_cloud_policy_updater_unittest.cc
(right):

https://codereview.chromium.org/2493603002/diff/120001/components/policy/core...
components/policy/core/common/cloud/component_cloud_policy_updater_unittest.cc:91:
ComponentCloudPolicyUpdaterTest::ComponentCloudPolicyUpdaterTest()
QQ: is there a test that checks that policy validation fails here if the signing
keys don't match the cached policy?

[emaxx, 2016-11-25 20:13:03]

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

[commit-bot: I haz the power, 2016-11-25 20:13:18]

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

[emaxx, 2016-11-25 20:14:04]

Description was changed from

==========
Implement component cloud policy signature validation

This adds signature validation for the component cloud policy
(e.g. policy for extensions). The signature is validated against
the same key that is used for the "superior" policy (e.g. the
user policy, the device local account policy, etc.).

This CL also adds keeping a copy of the most recent component
cloud policy and rechecking it when some of the credentials
change. This allows to handle key rotations gracefully: even
though the component cloud policy may fail the validation
immediately after the cloud policy refresh with the rotated key,
it will be re-validated and applied when the superior policy gets
processed and the credentials get propagated.

BUG=644632
TEST=existing tests (now with the added signature checks), new unit tests and
new browser test
==========

to

==========
Implement component cloud policy signature validation

This adds signature validation for the component cloud policy
(e.g. policy for extensions). The signature is validated against
the same key that is used for the "superior" policy (e.g. the
user policy, the device local account policy, etc.).

This CL also adds keeping a copy of the most recent component
cloud policy and rechecking it when some of the credentials
change. This allows to handle key rotations gracefully: even
though the component cloud policy may fail the validation
immediately after the cloud policy refresh with the rotated key,
it will be re-validated and applied when the superior policy gets
processed and the credentials get propagated.

BUG=644632
TEST=existing tests (now with the signature checks enabled),
     new unit tests and new browser test
==========

[emaxx, 2016-11-25 21:58:48]

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

[commit-bot: I haz the power, 2016-11-25 21:59: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-11-25 23:22:48]

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

[commit-bot: I haz the power, 2016-11-25 23:22:49]

Dry run: This issue passed the CQ dry run.

[emaxx, 2016-11-25 23:30:05]

https://codereview.chromium.org/2493603002/diff/120001/components/policy/core...
File components/policy/core/common/cloud/component_cloud_policy_service.cc
(right):

https://codereview.chromium.org/2493603002/diff/120001/components/policy/core...
components/policy/core/common/cloud/component_cloud_policy_service.cc:86: void
ClearCache();
On 2016/11/25 16:13:24, Andrew T Wilson (Slow) wrote:
> Useful to document what this does/when it should be invoked.

Done.

https://codereview.chromium.org/2493603002/diff/120001/components/policy/core...
components/policy/core/common/cloud/component_cloud_policy_service.cc:116: void
UpdateWithMostRecentPolicies();
On 2016/11/25 16:13:24, Andrew T Wilson (Slow) wrote:
> Document this?

Done.

https://codereview.chromium.org/2493603002/diff/120001/components/policy/core...
components/policy/core/common/cloud/component_cloud_policy_service.cc:245: //
TODO(emaxx): This is insecure, as it happens before the policy validation.
On 2016/11/25 16:13:24, Andrew T Wilson (Slow) wrote:
> Add a bug for this and put a link to the bug with the TODO

Good point, done.
I planned to resolve this together with crbug.com/650785, but, surely, it's
important to track this so that it doesn't drop out from radars.

https://codereview.chromium.org/2493603002/diff/120001/components/policy/core...
File
components/policy/core/common/cloud/component_cloud_policy_service_unittest.cc
(right):

https://codereview.chromium.org/2493603002/diff/120001/components/policy/core...
components/policy/core/common/cloud/component_cloud_policy_service_unittest.cc:8:
#include <map>
On 2016/11/25 16:13:24, Andrew T Wilson (Slow) wrote:
> blank line between C and stl includes?

Done.

https://codereview.chromium.org/2493603002/diff/120001/components/policy/core...
File components/policy/core/common/cloud/component_cloud_policy_store.cc
(right):

https://codereview.chromium.org/2493603002/diff/120001/components/policy/core...
components/policy/core/common/cloud/component_cloud_policy_store.cc:299:
base::Time time_not_before;
On 2016/11/25 16:13:24, Andrew T Wilson (Slow) wrote:
> comment what we're validating here

Done.

https://codereview.chromium.org/2493603002/diff/120001/components/policy/core...
components/policy/core/common/cloud/component_cloud_policy_store.cc:354:
policy_data->Swap(validator->policy_data().get());
On 2016/11/25 16:13:24, Andrew T Wilson (Slow) wrote:
> This is fine, but why did we move this around?

You mean, why do I have to return PolicyData from this method? That's needed for
keeping the |stored_policy_times_| member up to date: it has to be updated in
ComponentCloudPolicyStore::Store, but the policy timestamp is stored in the
PolicyData which was not previously passed to it from
ComponentCloudPolicyUpdater::UpdateExternalPolicy.

https://codereview.chromium.org/2493603002/diff/120001/components/policy/core...
File
components/policy/core/common/cloud/component_cloud_policy_store_unittest.cc
(right):

https://codereview.chromium.org/2493603002/diff/120001/components/policy/core...
components/policy/core/common/cloud/component_cloud_policy_store_unittest.cc:292:
"wrongtoken",
On 2016/11/25 16:13:24, Andrew T Wilson (Slow) wrote:
> This is fine, but I note it just tests the case where *everything* is wrong -
it
> doesn't check that having a single incorrect credential (i.e. wrong device id)
> causes validation to fail.

That's true, probably the original author was a bit lazy to test all scenarios,
but now I was stretching this even more.

Fixed now.

https://codereview.chromium.org/2493603002/diff/120001/components/policy/core...
File
components/policy/core/common/cloud/component_cloud_policy_updater_unittest.cc
(right):

https://codereview.chromium.org/2493603002/diff/120001/components/policy/core...
components/policy/core/common/cloud/component_cloud_policy_updater_unittest.cc:91:
ComponentCloudPolicyUpdaterTest::ComponentCloudPolicyUpdaterTest()
On 2016/11/25 16:13:24, Andrew T Wilson (Slow) wrote:
> QQ: is there a test that checks that policy validation fails here if the
signing
> keys don't match the cached policy?

Nope, there were no tests for the signature validation failures.
Fixed now.

[emaxx, 2016-11-28 18:13:51]

The CQ bit was checked by emaxx@chromium.org

[emaxx, 2016-11-28 18:13:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
atwilson@chromium.org
Link to the patchset: https://codereview.chromium.org/2493603002/#ps160001
(title: "Add comment")

[commit-bot: I haz the power, 2016-11-28 18:14:29]

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-11-28 21:54:30]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1480356831063950,
"parent_rev": "77d23df1093915147287f0c917ca206e2b71a793", "commit_rev":
"0ae4fbb15aecc1d5b89f3f8d8e4d219b7039fc54"}

[commit-bot: I haz the power, 2016-11-28 21:54:59]

Description was changed from

==========
Implement component cloud policy signature validation

This adds signature validation for the component cloud policy
(e.g. policy for extensions). The signature is validated against
the same key that is used for the "superior" policy (e.g. the
user policy, the device local account policy, etc.).

This CL also adds keeping a copy of the most recent component
cloud policy and rechecking it when some of the credentials
change. This allows to handle key rotations gracefully: even
though the component cloud policy may fail the validation
immediately after the cloud policy refresh with the rotated key,
it will be re-validated and applied when the superior policy gets
processed and the credentials get propagated.

BUG=644632
TEST=existing tests (now with the signature checks enabled),
     new unit tests and new browser test
==========

to

==========
Implement component cloud policy signature validation

This adds signature validation for the component cloud policy
(e.g. policy for extensions). The signature is validated against
the same key that is used for the "superior" policy (e.g. the
user policy, the device local account policy, etc.).

This CL also adds keeping a copy of the most recent component
cloud policy and rechecking it when some of the credentials
change. This allows to handle key rotations gracefully: even
though the component cloud policy may fail the validation
immediately after the cloud policy refresh with the rotated key,
it will be re-validated and applied when the superior policy gets
processed and the credentials get propagated.

BUG=644632
TEST=existing tests (now with the signature checks enabled),
     new unit tests and new browser test
==========

[commit-bot: I haz the power, 2016-11-28 21:55:00]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-11-28 21:56:48]

Description was changed from

==========
Implement component cloud policy signature validation

This adds signature validation for the component cloud policy
(e.g. policy for extensions). The signature is validated against
the same key that is used for the "superior" policy (e.g. the
user policy, the device local account policy, etc.).

This CL also adds keeping a copy of the most recent component
cloud policy and rechecking it when some of the credentials
change. This allows to handle key rotations gracefully: even
though the component cloud policy may fail the validation
immediately after the cloud policy refresh with the rotated key,
it will be re-validated and applied when the superior policy gets
processed and the credentials get propagated.

BUG=644632
TEST=existing tests (now with the signature checks enabled),
     new unit tests and new browser test
==========

to

==========
Implement component cloud policy signature validation

This adds signature validation for the component cloud policy
(e.g. policy for extensions). The signature is validated against
the same key that is used for the "superior" policy (e.g. the
user policy, the device local account policy, etc.).

This CL also adds keeping a copy of the most recent component
cloud policy and rechecking it when some of the credentials
change. This allows to handle key rotations gracefully: even
though the component cloud policy may fail the validation
immediately after the cloud policy refresh with the rotated key,
it will be re-validated and applied when the superior policy gets
processed and the credentials get propagated.

BUG=644632
TEST=existing tests (now with the signature checks enabled),
     new unit tests and new browser test

Committed: https://crrev.com/4df0d565e3cd9258f0a5fc4517bfb739a7b28d4f
Cr-Commit-Position: refs/heads/master@{#434730}
==========

[commit-bot: I haz the power, 2016-11-28 21:56:50]

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