Expose signing key from cloud policy stores

Expose signing key from cloud policy stores

This change makes the cloud policy stores (user, device, device-local)
to expose more information about the policies that they own: the public
part of the signing key. This was not exposed by their interfaces
previously.

No behavior change is expected to be introduced by this CL.

This change is intended to be a base for the subsequent implementation
of the component cloud policy signature verification.

BUG=644632
TEST=new unit tests

Committed: https://crrev.com/cc11f1e65bc21e264ec3a763e9c0d36f2d9025b1
Cr-Commit-Position: refs/heads/master@{#433902}

[emaxx, 2016-11-08 16:40:19]

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

[commit-bot: I haz the power, 2016-11-08 16:40:57]

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-08 17:20:35]

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

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

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

[emaxx, 2016-11-08 17:52:13]

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

[commit-bot: I haz the power, 2016-11-08 17:53:04]

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-08 18:34:47]

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

[commit-bot: I haz the power, 2016-11-08 18:34:48]

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

[emaxx, 2016-11-08 20:38:18]

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

[commit-bot: I haz the power, 2016-11-08 20:39:03]

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-08 20:41:53]

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

[emaxx, 2016-11-08 20:41:54]

Thiemo, may I ask you to review this CL?
It's part of a bigger change, but this CL should be quite self-contained.
In short, the background is the following: validation of the signature for
policy for extensions has to be added, and it was decided that it should use the
same key as the "superior" policy (e.g. user policy).

[emaxx, 2016-11-08 20:53:37]

https://codereview.chromium.org/2488573003/diff/40001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos_unittest.cc
(right):

https://codereview.chromium.org/2488573003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos_unittest.cc:134:
std::string GetPolicyPublicKeyAsString() {
I'd like to land a separate CL that would move these two operations (the public
key export as a string) straight into PolicyBuilder.
Just didn't want to bloat this CL with this too.

https://codereview.chromium.org/2488573003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos_unittest.cc
(right):

https://codereview.chromium.org/2488573003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos_unittest.cc:409:
EXPECT_EQ(std::string(), store_->public_key());
There's a tricky semantics here, and I'm not sure whether it's reasonable.
Short explanation:
In this test the public_key_ member got cleared due to the failed session
manager operation. Meanwhile, in several tests above and below the public_key_
was kept after attempts to store invalid policy. So the difference was caused by
the point of the failure: in a pre-validation or during the session manager
operation.
Thiemo, WDYT?

https://codereview.chromium.org/2488573003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos_unittest.cc:472:
EXPECT_EQ(GetPolicyPublicKeyAsString(), store_->public_key());
Here's another tricky case.
Here the policy was not loaded (because it's invalid), but the public key was
loaded and exposed. Meanwhile in the two previous tests the key was not exposed
(because it's loaded by UserCloudPolicyStoreChromeOS only _after_ querying
session manager).

[commit-bot: I haz the power, 2016-11-08 22:49:06]

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

[commit-bot: I haz the power, 2016-11-08 22:49:07]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[emaxx, 2016-11-09 13:06:25]

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

[commit-bot: I haz the power, 2016-11-09 13:06:33]

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 13:47:25]

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

[commit-bot: I haz the power, 2016-11-09 13:47:26]

Dry run: This issue passed the CQ dry run.

[emaxx, 2016-11-10 19:12:31]

As per our offline discussion, looks like owning domain is actually not needed
to be exposed.

So, Thiemo, please wait until I update the CL to remove this part.

[Thiemo Nagel, 2016-11-10 19:26:37]

https://codereview.chromium.org/2488573003/diff/40001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_store.h (right):

https://codereview.chromium.org/2488573003/diff/40001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_store.h:87: std::string
owning_domain() const { return owning_domain_; }
Nit: What about using const std::string& return type instead?  It should be
faster.

https://codereview.chromium.org/2488573003/diff/40001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_store.h:162: // Empty value
means that the public key is yet unknown.
Nit: Suggest to wrap that into the previous line.

[emaxx, 2016-11-10 20:08:30]

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

[commit-bot: I haz the power, 2016-11-10 20:08:56]

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-10 20:20:05]

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

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

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-10 21:06:09]

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

[commit-bot: I haz the power, 2016-11-10 21:06:10]

Dry run: This issue passed the CQ dry run.

[emaxx, 2016-11-10 21:07:19]

Thiemo, PTAL.

I removed the owning domain exposing, and addressed the feedback from your first
iteration.

https://codereview.chromium.org/2488573003/diff/40001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_store.h (right):

https://codereview.chromium.org/2488573003/diff/40001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_store.h:87: std::string
owning_domain() const { return owning_domain_; }
On 2016/11/10 19:26:37, Thiemo Nagel (slow) wrote:
> Nit: What about using const std::string& return type instead?  It should be
> faster.

Done.

https://codereview.chromium.org/2488573003/diff/40001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_store.h:162: // Empty value
means that the public key is yet unknown.
On 2016/11/10 19:26:37, Thiemo Nagel (slow) wrote:
> Nit: Suggest to wrap that into the previous line.

Done.

[emaxx, 2016-11-11 16:16:01]

Description was changed from

==========
Expose signing key and owning domain from cloud policy stores

This change makes the cloud policy stores (user, device, device-local)
to expose more information about the policies that they own: the public
part of the signing key and the owning domain. This was not exposed by
their interfaces previously.

No behavior change is expected to be introduced by this CL.

This change is intended to be a base for the subsequent implementation
of the component cloud policy signature verification.

BUG=644632
TEST=new unit tests
==========

to

==========
Expose signing key from cloud policy stores

This change makes the cloud policy stores (user, device, device-local)
to expose more information about the policies that they own: the public
part of the signing key. This was not exposed by their interfaces
previously.

No behavior change is expected to be introduced by this CL.

This change is intended to be a base for the subsequent implementation
of the component cloud policy signature verification.

BUG=644632
TEST=new unit tests
==========

[emaxx, 2016-11-11 21:30:48]

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

[commit-bot: I haz the power, 2016-11-11 21:31: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-11 22:27:01]

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

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

Dry run: This issue passed the CQ dry run.

[emaxx, 2016-11-11 22:56:52]

Reduced changes in DeviceLocalAccountPolicyStore.

[Thiemo Nagel, 2016-11-16 16:50:06]

I guess the CL needs a rebase and rerunning of the tests...

https://codereview.chromium.org/2488573003/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc
(right):

https://codereview.chromium.org/2488573003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc:203:
public_key_ = key ? key->as_string() : std::string();
public_key_ doesn't seem to exist yet?

[emaxx, 2016-11-16 17:20:23]

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

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

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-16 17:24:42]

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

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

Dry run: 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...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[emaxx, 2016-11-16 18:29:42]

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

[commit-bot: I haz the power, 2016-11-16 18:30: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-16 18:46:54]

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

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

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

[emaxx, 2016-11-16 18:48:17]

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

[commit-bot: I haz the power, 2016-11-16 18:49:17]

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-16 21:34:46]

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

[commit-bot: I haz the power, 2016-11-16 21:34:46]

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

[emaxx, 2016-11-16 22:05:58]

https://codereview.chromium.org/2488573003/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc
(right):

https://codereview.chromium.org/2488573003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc:203:
public_key_ = key ? key->as_string() : std::string();
On 2016/11/16 16:50:06, Thiemo Nagel (slow) wrote:
> public_key_ doesn't seem to exist yet?

Nope, it's defined in the base class CloudPolicyStore.
The idea is the same as for the existing CloudPolicyStore::policy_,
CloudPolicyStore::status_, etc.: the base class defines members and public
accessors, while subclasses implement filling these members with the actual
values.

[emaxx, 2016-11-16 22:06:25]

On 2016/11/16 16:50:06, Thiemo Nagel (slow) wrote:
> I guess the CL needs a rebase and rerunning of the tests...

Done.

[emaxx, 2016-11-16 22:06:51]

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

[commit-bot: I haz the power, 2016-11-16 22:07:49]

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-16 22:41:18]

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

[commit-bot: I haz the power, 2016-11-16 22:41:18]

Dry run: This issue passed the CQ dry run.

[Thiemo Nagel, 2016-11-17 17:19:56]

https://codereview.chromium.org/2488573003/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc
(right):

https://codereview.chromium.org/2488573003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc:203:
public_key_ = key ? key->as_string() : std::string();
Sorry, I've overlooked that.

https://codereview.chromium.org/2488573003/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_local_account_policy_store.cc
(right):

https://codereview.chromium.org/2488573003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_store.cc:150:
public_key_ = key->as_string();
What about moving this next to the InstallPolicy() call?  It would seem cleaner
to me to only store the key when validation has been successful and policy is
being stored as well, though that probably would require to add an accessor for
the key to the validator.  Wdyt?

[emaxx, 2016-11-17 18:44:55]

https://codereview.chromium.org/2488573003/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_local_account_policy_store.cc
(right):

https://codereview.chromium.org/2488573003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_store.cc:150:
public_key_ = key->as_string();
On 2016/11/17 17:19:56, Thiemo Nagel (slow) wrote:
> What about moving this next to the InstallPolicy() call?  It would seem
cleaner
> to me to only store the key when validation has been successful and policy is
> being stored as well, though that probably would require to add an accessor
for
> the key to the validator.  Wdyt?

Sounds reasonable. Do you think it's better to have this semantics ("update key
only after successful policy load/store") in all touched classes?
I had a similar concern for other classes - see my comments in
user_cloud_policy_store_chromeos_unittest.cc:
https://codereview.chromium.org/2488573003/diff/40001/chrome/browser/chromeos...
https://codereview.chromium.org/2488573003/diff/40001/chrome/browser/chromeos...
I was not sure because, for example, in case of user store it's not clear which
should be trusted more: policy blob or public key.

[emaxx, 2016-11-17 19:24:37]

On 2016/11/17 18:44:55, emaxx wrote:
>
https://codereview.chromium.org/2488573003/diff/180001/chrome/browser/chromeo...
> File chrome/browser/chromeos/policy/device_local_account_policy_store.cc
> (right):
> 
>
https://codereview.chromium.org/2488573003/diff/180001/chrome/browser/chromeo...
> chrome/browser/chromeos/policy/device_local_account_policy_store.cc:150:
> public_key_ = key->as_string();
> On 2016/11/17 17:19:56, Thiemo Nagel (slow) wrote:
> > What about moving this next to the InstallPolicy() call?  It would seem
> cleaner
> > to me to only store the key when validation has been successful and policy
is
> > being stored as well, though that probably would require to add an accessor
> for
> > the key to the validator.  Wdyt?
> 
> Sounds reasonable. Do you think it's better to have this semantics ("update
key
> only after successful policy load/store") in all touched classes?
> I had a similar concern for other classes - see my comments in
> user_cloud_policy_store_chromeos_unittest.cc:
>
https://codereview.chromium.org/2488573003/diff/40001/chrome/browser/chromeos...
>
https://codereview.chromium.org/2488573003/diff/40001/chrome/browser/chromeos...
> I was not sure because, for example, in case of user store it's not clear
which
> should be trusted more: policy blob or public key.

As per offline discussion, I'll try to address this in the next patch, and add a
comment for CloudPolicyStore::public_key describing the expectations from it.

[emaxx, 2016-11-21 14:26:51]

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

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

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-21 15:19:55]

Thiemo, PTAL. I think I've made the semantics much clearer now: public key is
updated only together with update of the current active policy.

[Thiemo Nagel, 2016-11-21 17:45:34]

https://codereview.chromium.org/2488573003/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_local_account_policy_store.cc
(right):

https://codereview.chromium.org/2488573003/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_store.cc:142:
chromeos::DeviceSettingsService::OwnershipStatus ownership_status) {
Nit: Since you're touching this code, I'd suggest to rename to
|ownership_status_unused|.

https://codereview.chromium.org/2488573003/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_local_account_policy_store.h (right):

https://codereview.chromium.org/2488573003/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_store.h:64: void
UpdatePolicy(const std::string& used_public_key,
Nit: "used" doesn't seem to convey meaningful information (after all if it was
unused, it probably wouldn't be a parameter in the first place).  How about
"policy_key"?

https://codereview.chromium.org/2488573003/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h (right):

https://codereview.chromium.org/2488573003/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h:137: const
std::string owning_domain_;
Afaics, |owning_domain_| is only used in one place.  Maybe it would be better to
remove it again?

https://codereview.chromium.org/2488573003/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h:141:
std::string loaded_policy_key_;
I think |loaded_policy_key_| deserves a comment.

Maybe you could also come up with a better name?  Maybe |cached_policy_key_| or
something else?

Or maybe it would work to rename |public_key_| to |policy_signing_key_| or
|policy_signature_key_| to differentiate it more?

https://codereview.chromium.org/2488573003/diff/200001/components/policy/core...
File components/policy/core/common/cloud/user_cloud_policy_store.cc (right):

https://codereview.chromium.org/2488573003/diff/200001/components/policy/core...
components/policy/core/common/cloud/user_cloud_policy_store.cc:412: // need to
Nit: wrap line

[commit-bot: I haz the power, 2016-11-21 18:28:43]

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

[commit-bot: I haz the power, 2016-11-21 18:28:44]

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

[emaxx, 2016-11-21 19:28:32]

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

[commit-bot: I haz the power, 2016-11-21 19:29: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-21 20:04:58]

https://codereview.chromium.org/2488573003/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_local_account_policy_store.cc
(right):

https://codereview.chromium.org/2488573003/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_store.cc:142:
chromeos::DeviceSettingsService::OwnershipStatus ownership_status) {
On 2016/11/21 17:45:33, Thiemo Nagel (slow) wrote:
> Nit: Since you're touching this code, I'd suggest to rename to
> |ownership_status_unused|.

Done.

https://codereview.chromium.org/2488573003/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_local_account_policy_store.h (right):

https://codereview.chromium.org/2488573003/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_store.h:64: void
UpdatePolicy(const std::string& used_public_key,
On 2016/11/21 17:45:33, Thiemo Nagel (slow) wrote:
> Nit: "used" doesn't seem to convey meaningful information (after all if it was
> unused, it probably wouldn't be a parameter in the first place).  How about
> "policy_key"?

Good point.
Replaced this now with "signature_validation_public_key" - do you think it's
clear enough?

https://codereview.chromium.org/2488573003/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h (right):

https://codereview.chromium.org/2488573003/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h:137: const
std::string owning_domain_;
On 2016/11/21 17:45:34, Thiemo Nagel (slow) wrote:
> Afaics, |owning_domain_| is only used in one place.  Maybe it would be better
to
> remove it again?

That's true, rebasing eliminated all other usages.
Done.

https://codereview.chromium.org/2488573003/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h:141:
std::string loaded_policy_key_;
On 2016/11/21 17:45:34, Thiemo Nagel (slow) wrote:
> I think |loaded_policy_key_| deserves a comment.
> 
> Maybe you could also come up with a better name?  Maybe |cached_policy_key_|
or
> something else?
> 
> Or maybe it would work to rename |public_key_| to |policy_signing_key_| or
> |policy_signature_key_| to differentiate it more?

Done. Added comment, renamed to "cached_policy_key_".

https://codereview.chromium.org/2488573003/diff/200001/components/policy/core...
File components/policy/core/common/cloud/user_cloud_policy_store.cc (right):

https://codereview.chromium.org/2488573003/diff/200001/components/policy/core...
components/policy/core/common/cloud/user_cloud_policy_store.cc:412: // need to
On 2016/11/21 17:45:34, Thiemo Nagel (slow) wrote:
> Nit: wrap line

Done.

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

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

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

Dry run: This issue passed the CQ dry run.

[Thiemo Nagel, 2016-11-22 11:57:15]

Lgtm.  Thank you for your patience.

https://codereview.chromium.org/2488573003/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc (right):

https://codereview.chromium.org/2488573003/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:444:
std::string() /* public_key */);
Nit: policy_signature_public_key

https://codereview.chromium.org/2488573003/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h (right):

https://codereview.chromium.org/2488573003/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h:135: bool
is_legacy_caches_load_performed_ = false;
I guess this will go away after rebase.

https://codereview.chromium.org/2488573003/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h:137: // The
current key used to verify signatures of policy. This value is
Nit: Please wrap at 80 characters.

https://codereview.chromium.org/2488573003/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h:143: bool
is_cached_policy_key_loaded_ = false;
Nit: Imho the "is_" part of the name redundant and can be removed.

[emaxx, 2016-11-22 15:06:39]

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

[commit-bot: I haz the power, 2016-11-22 15:06:56]

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-22 15:09:35]

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

[commit-bot: I haz the power, 2016-11-22 15:09:35]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[emaxx, 2016-11-22 15:25:19]

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

[commit-bot: I haz the power, 2016-11-22 15:25:28]

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-22 16:16:52]

Thank you for reviewing this big CL!

https://codereview.chromium.org/2488573003/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h (right):

https://codereview.chromium.org/2488573003/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h:135: bool
is_legacy_caches_load_performed_ = false;
On 2016/11/22 11:57:15, Thiemo Nagel (slow) wrote:
> I guess this will go away after rebase.

Glad to hear that! :)

https://codereview.chromium.org/2488573003/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h:137: // The
current key used to verify signatures of policy. This value is
On 2016/11/22 11:57:15, Thiemo Nagel (slow) wrote:
> Nit: Please wrap at 80 characters.

Done.

https://codereview.chromium.org/2488573003/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h:143: bool
is_cached_policy_key_loaded_ = false;
On 2016/11/22 11:57:15, Thiemo Nagel (slow) wrote:
> Nit: Imho the "is_" part of the name redundant and can be removed.

Done.

[commit-bot: I haz the power, 2016-11-22 16:19:41]

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

[commit-bot: I haz the power, 2016-11-22 16:19:42]

Dry run: This issue passed the CQ dry run.

[Thiemo Nagel, 2016-11-22 16:49:36]

still lgtm

[emaxx, 2016-11-22 17:23:39]

The CQ bit was checked by emaxx@chromium.org

[commit-bot: I haz the power, 2016-11-22 17:24:03]

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-22 17:28:54]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1479835419608720,
"parent_rev": "590f80f5c44dc52480ff82f1583f153b37e8a9f0", "commit_rev":
"bd5f510eef6bf44f77171427cfac8a5f7f2fd89f"}

[commit-bot: I haz the power, 2016-11-22 17:29:22]

Description was changed from

==========
Expose signing key from cloud policy stores

This change makes the cloud policy stores (user, device, device-local)
to expose more information about the policies that they own: the public
part of the signing key. This was not exposed by their interfaces
previously.

No behavior change is expected to be introduced by this CL.

This change is intended to be a base for the subsequent implementation
of the component cloud policy signature verification.

BUG=644632
TEST=new unit tests
==========

to

==========
Expose signing key from cloud policy stores

This change makes the cloud policy stores (user, device, device-local)
to expose more information about the policies that they own: the public
part of the signing key. This was not exposed by their interfaces
previously.

No behavior change is expected to be introduced by this CL.

This change is intended to be a base for the subsequent implementation
of the component cloud policy signature verification.

BUG=644632
TEST=new unit tests
==========

[commit-bot: I haz the power, 2016-11-22 17:29:23]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-11-22 17:31:45]

Description was changed from

==========
Expose signing key from cloud policy stores

This change makes the cloud policy stores (user, device, device-local)
to expose more information about the policies that they own: the public
part of the signing key. This was not exposed by their interfaces
previously.

No behavior change is expected to be introduced by this CL.

This change is intended to be a base for the subsequent implementation
of the component cloud policy signature verification.

BUG=644632
TEST=new unit tests
==========

to

==========
Expose signing key from cloud policy stores

This change makes the cloud policy stores (user, device, device-local)
to expose more information about the policies that they own: the public
part of the signing key. This was not exposed by their interfaces
previously.

No behavior change is expected to be introduced by this CL.

This change is intended to be a base for the subsequent implementation
of the component cloud policy signature verification.

BUG=644632
TEST=new unit tests

Committed: https://crrev.com/cc11f1e65bc21e264ec3a763e9c0d36f2d9025b1
Cr-Commit-Position: refs/heads/master@{#433902}
==========

[commit-bot: I haz the power, 2016-11-22 17:31:46]

Patchset 14 (id:??) landed as
https://crrev.com/cc11f1e65bc21e264ec3a763e9c0d36f2d9025b1
Cr-Commit-Position: refs/heads/master@{#433902}