platformKeys: Add policy and corporate key tagging.

platformKeys: Add policy and corporate key tagging.

BUG=460232
Committed: https://crrev.com/dabdfe91a4ed78566c072ea32d305bb7bab733ba
Cr-Commit-Position: refs/heads/master@{#335029}

[pneubeck (no reviews), 2015-06-11 19:05:08]

Patchset #1 (id:1) has been deleted

[pneubeck (no reviews), 2015-06-11 19:05:17]

Patchset #1 (id:20001) has been deleted

[pneubeck (no reviews), 2015-06-11 19:05:24]

Patchset #1 (id:40001) has been deleted

[pneubeck (no reviews), 2015-06-11 19:05:46]

Patchset #1 (id:60001) has been deleted

[bartfab (slow), 2015-06-11 23:33:08]

bartfab@chromium.org changed reviewers:
+ bartfab@chromium.org

[bartfab (slow), 2015-06-11 23:33:09]

The PolicyTestHelper looks reasonable to me, but I would move it to its own
header/implementation files and remove login_policy_test_base.*.

https://codereview.chromium.org/1150373002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/login_policy_test_base.cc (right):

https://codereview.chromium.org/1150373002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/login_policy_test_base.cc:58:
policy_type_dict->Set("mandatory", mandatory.CreateDeepCopy());
I wonder whether the JSON will be slightly different now in case one of the
policy dictionaries is empty. Previously, we would have done a Set() with a
|nullptr| as argument. Now we always pass a dictionary - an empty one in that
case. It should not make a semantic difference... and it is well possible that a
|nullptr| would have actually caused a crash.

https://codereview.chromium.org/1150373002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/login_policy_test_base.cc:94:
CHECK(temp_dir_.CreateUniqueTempDir());
Since this will be used in tests only, I think an ASSERT() would be better. You
cannot use ASSERT() in the constructor, but you could move this initialization
to an Init() method and use it there.

https://codereview.chromium.org/1150373002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/login_policy_test_base.cc:105: void
PolicyTestHelper::UpdatePolicy(Profile* profile,
Nit: Missing blank line.

https://codereview.chromium.org/1150373002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/login_policy_test_base.cc:110:
policy::ProfilePolicyConnector* profile_connector =
Nit: Here and below: const pointers.

https://codereview.chromium.org/1150373002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/login_policy_test_base.cc:133:
enterprise_management::DeviceRegisterRequest::Type registration_type =
Nit: const.

https://codereview.chromium.org/1150373002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/login_policy_test_base.cc:149:
PolicyTestHelper::~PolicyTestHelper() {
Nit: Reorder methods so that declaration and definition orders match.

https://codereview.chromium.org/1150373002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/login_policy_test_base.cc:167:
scoped_ptr<base::DictionaryValue> mandatory(GetMandatoryPoliciesValue());
If you are switching from scoped_ptrs to deep copies in the helper, why not do
it throughout?

[pneubeck (no reviews), 2015-06-12 08:55:18]

splitted the CL and addressed comments there:
https://codereview.chromium.org/1185593002

https://codereview.chromium.org/1150373002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/login_policy_test_base.cc (right):

https://codereview.chromium.org/1150373002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/login_policy_test_base.cc:58:
policy_type_dict->Set("mandatory", mandatory.CreateDeepCopy());
On 2015/06/11 23:33:09, bartfab wrote:
> I wonder whether the JSON will be slightly different now in case one of the
> policy dictionaries is empty. Previously, we would have done a Set() with a
> |nullptr| as argument. Now we always pass a dictionary - an empty one in that
> case. It should not make a semantic difference... and it is well possible that
a
> |nullptr| would have actually caused a crash.

good point and indeed nullptr would lead to a crash.

https://codereview.chromium.org/1150373002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/login_policy_test_base.cc:94:
CHECK(temp_dir_.CreateUniqueTempDir());
On 2015/06/11 23:33:09, bartfab wrote:
> Since this will be used in tests only, I think an ASSERT() would be better.
You
> cannot use ASSERT() in the constructor, but you could move this initialization
> to an Init() method and use it there.

Done.

https://codereview.chromium.org/1150373002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/login_policy_test_base.cc:105: void
PolicyTestHelper::UpdatePolicy(Profile* profile,
On 2015/06/11 23:33:09, bartfab wrote:
> Nit: Missing blank line.

Done.

https://codereview.chromium.org/1150373002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/login_policy_test_base.cc:110:
policy::ProfilePolicyConnector* profile_connector =
On 2015/06/11 23:33:09, bartfab wrote:
> Nit: Here and below: const pointers.

Done.

https://codereview.chromium.org/1150373002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/login_policy_test_base.cc:133:
enterprise_management::DeviceRegisterRequest::Type registration_type =
On 2015/06/11 23:33:09, bartfab wrote:
> Nit: const.

Done.

https://codereview.chromium.org/1150373002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/login_policy_test_base.cc:149:
PolicyTestHelper::~PolicyTestHelper() {
On 2015/06/11 23:33:09, bartfab wrote:
> Nit: Reorder methods so that declaration and definition orders match.

Done.

https://codereview.chromium.org/1150373002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/login_policy_test_base.cc:167:
scoped_ptr<base::DictionaryValue> mandatory(GetMandatoryPoliciesValue());
On 2015/06/11 23:33:09, bartfab wrote:
> If you are switching from scoped_ptrs to deep copies in the helper, why not do
> it throughout?

Done.

[pneubeck (no reviews), 2015-06-12 11:24:20]

Patchset #3 (id:120001) has been deleted

[pneubeck (no reviews), 2015-06-12 12:10:34]

Patchset #3 (id:140001) has been deleted

[pneubeck (no reviews), 2015-06-12 12:56:38]

Patchset #4 (id:180001) has been deleted

[pneubeck (no reviews), 2015-06-12 13:05:57]

Patchset #4 (id:200001) has been deleted

[pneubeck (no reviews), 2015-06-12 13:11:23]

Patchset #4 (id:220001) has been deleted

[pneubeck (no reviews), 2015-06-12 13:18:00]

should be got good to review now.

@Bartosz, please check whether the policy is hooked up correctly, i.e. at least
the files
  chrome/browser/policy/*
  chrome/test/data/policy/*
  components/policy/*
  tools/metrics/histograms/histograms.xml
  chrome/browser/chromeos/platform_keys/key_permissions_policy_handlers.*

@Christoph, please review the implementation of the policy logic in platformKeys
(at least, all other files than the above).

[pneubeck (no reviews), 2015-06-12 13:18:19]

pneubeck@chromium.org changed reviewers:
+ cschuet@chromium.org

[pneubeck (no reviews), 2015-06-12 13:18:19]

+Christoph

@Christoph, please review the implementation of the policy logic in platformKeys
(at least, all other files than the above).

[bartfab (slow), 2015-06-15 17:28:16]

As requested, I reviewed:

chrome/browser/policy/*
chrome/test/data/policy/*
components/policy/*
tools/metrics/histograms/histograms.xml
chrome/browser/chromeos/platform_keys/key_permissions_policy_handlers.*

https://codereview.chromium.org/1150373002/diff/260001/chrome/browser/chromeo...
File chrome/browser/chromeos/platform_keys/key_permissions_policy_handlers.cc
(right):

https://codereview.chromium.org/1150373002/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions_policy_handlers.cc:15:
chrome_schema.GetKnownProperty(policy::key::kKeyPermissions),
Nit: #include "components/policy/core/common/schema.h"

https://codereview.chromium.org/1150373002/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions_policy_handlers.cc:25: }
We normally use a pref as the vehicle to move policy values from here to other
subsystems. Even if there is no actual pref that expresses key permissions, the
idiomatic thing to do is to introduce such a (synthetic) pref and set its value
here, rather than accessing policy directly from KeyPermissions.

https://codereview.chromium.org/1150373002/diff/260001/chrome/browser/chromeo...
File chrome/browser/chromeos/platform_keys/key_permissions_policy_handlers.h
(right):

https://codereview.chromium.org/1150373002/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions_policy_handlers.h:5:
#ifndef CHROME_BROWSER_CHROMEOS_PLATFORM_KEYS_KEY_PERMISSIONS_POLICY_HANDLERS_H_
Nit: Why is the file name in the plural (handler*s*)?

https://codereview.chromium.org/1150373002/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions_policy_handlers.h:8:
#include "base/memory/scoped_ptr.h"
Nit: Not used.

https://codereview.chromium.org/1150373002/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions_policy_handlers.h:9:
#include "base/values.h"
Nit: Not used.

https://codereview.chromium.org/1150373002/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions_policy_handlers.h:17:
explicit KeyPermissionsPolicyHandler(const policy::Schema& chrome_schema);
Nit: Forward-declare |policy::Schema|.

https://codereview.chromium.org/1150373002/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions_policy_handlers.h:18:
~KeyPermissionsPolicyHandler() override;
Nit: No need to override the destructor here.

https://codereview.chromium.org/1150373002/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions_policy_handlers.h:20: //
ConfigurationPolicyHandler:
Nit: s|// |// policy::|

https://codereview.chromium.org/1150373002/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions_policy_handlers.h:25:
DISALLOW_COPY_AND_ASSIGN(KeyPermissionsPolicyHandler);
Nit: #include "base/macros.h"

https://codereview.chromium.org/1150373002/diff/260001/chrome/test/data/polic...
File chrome/test/data/policy/policy_test_cases.json (right):

https://codereview.chromium.org/1150373002/diff/260001/chrome/test/data/polic...
chrome/test/data/policy/policy_test_cases.json:2244: },
Nit: If you KeyPermissionsPolicyHandler populate a synthetic pref, you will be
able to test it here - an added bonus of using a pref.

https://codereview.chromium.org/1150373002/diff/260001/components/policy/reso...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/1150373002/diff/260001/components/policy/reso...
components/policy/resources/policy_templates.json:7377: 'description': '''If set
to true, this extension can use all corporate keys to sign arbitrary data. If
set to false, it cannot access any corporate key and the user can not grant such
permission either.''',
1: What is a corporate key? I never heard or saw the term before in our
documentation.
2: Nit: s/key and/keys and/

https://codereview.chromium.org/1150373002/diff/260001/components/policy/reso...
components/policy/resources/policy_templates.json:7400: By default an extension
cannot use a private key for signing data, which is equivalent to setting
allowCorporateKeyUsage to false for that extension. A user cannot workaround
that restriction.
Nit 1: Of course you can use "private keys" in general. You just cannot use
private keys associated with corporate key pairs.
Nit 2: s/workaround/work around/ or better, s/workaround/circumvent/

https://codereview.chromium.org/1150373002/diff/260001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1150373002/diff/260001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:53051: +  <int value="302" label="Grants
extensions access to private keys"/>
Nit: Please keep this in sync with the caption in policy_templates.json

[pneubeck (no reviews), 2015-06-17 08:59:57]

https://codereview.chromium.org/1150373002/diff/260001/chrome/browser/chromeo...
File chrome/browser/chromeos/platform_keys/key_permissions_policy_handlers.cc
(right):

https://codereview.chromium.org/1150373002/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions_policy_handlers.cc:15:
chrome_schema.GetKnownProperty(policy::key::kKeyPermissions),
On 2015/06/15 17:28:15, bartfab wrote:
> Nit: #include "components/policy/core/common/schema.h"

Done.

https://codereview.chromium.org/1150373002/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions_policy_handlers.cc:25: }
On 2015/06/15 17:28:15, bartfab wrote:
> We normally use a pref as the vehicle to move policy values from here to other
> subsystems. Even if there is no actual pref that expresses key permissions,
the
> idiomatic thing to do is to introduce such a (synthetic) pref and set its
value
> here, rather than accessing policy directly from KeyPermissions.

Opinions on this seem to deviate as Mattias agreed that without equivalent user
setting it's fine to omit the pref.

If you don't feel strong about this, I'd prefer to keep it as is.

https://codereview.chromium.org/1150373002/diff/260001/chrome/browser/chromeo...
File chrome/browser/chromeos/platform_keys/key_permissions_policy_handlers.h
(right):

https://codereview.chromium.org/1150373002/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions_policy_handlers.h:5:
#ifndef CHROME_BROWSER_CHROMEOS_PLATFORM_KEYS_KEY_PERMISSIONS_POLICY_HANDLERS_H_
On 2015/06/15 17:28:15, bartfab wrote:
> Nit: Why is the file name in the plural (handler*s*)?

Done.

https://codereview.chromium.org/1150373002/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions_policy_handlers.h:8:
#include "base/memory/scoped_ptr.h"
On 2015/06/15 17:28:15, bartfab wrote:
> Nit: Not used.

Done.

https://codereview.chromium.org/1150373002/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions_policy_handlers.h:9:
#include "base/values.h"
On 2015/06/15 17:28:15, bartfab wrote:
> Nit: Not used.

Done.

https://codereview.chromium.org/1150373002/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions_policy_handlers.h:17:
explicit KeyPermissionsPolicyHandler(const policy::Schema& chrome_schema);
On 2015/06/15 17:28:15, bartfab wrote:
> Nit: Forward-declare |policy::Schema|.

Done.

https://codereview.chromium.org/1150373002/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions_policy_handlers.h:18:
~KeyPermissionsPolicyHandler() override;
On 2015/06/15 17:28:15, bartfab wrote:
> Nit: No need to override the destructor here.

Done.

https://codereview.chromium.org/1150373002/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions_policy_handlers.h:20: //
ConfigurationPolicyHandler:
On 2015/06/15 17:28:15, bartfab wrote:
> Nit: s|// |// policy::|

Done.

https://codereview.chromium.org/1150373002/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions_policy_handlers.h:25:
DISALLOW_COPY_AND_ASSIGN(KeyPermissionsPolicyHandler);
On 2015/06/15 17:28:15, bartfab wrote:
> Nit: #include "base/macros.h"

Done.

https://codereview.chromium.org/1150373002/diff/260001/chrome/test/data/polic...
File chrome/test/data/policy/policy_test_cases.json (right):

https://codereview.chromium.org/1150373002/diff/260001/chrome/test/data/polic...
chrome/test/data/policy/policy_test_cases.json:2244: },
On 2015/06/15 17:28:15, bartfab wrote:
> Nit: If you KeyPermissionsPolicyHandler populate a synthetic pref, you will be
> able to test it here - an added bonus of using a pref.

but what would it test? surely not the policy application that I care about.

I have an browser test that covers the whole chain from policy dictionary to
enforced key permission in the extension API.

https://codereview.chromium.org/1150373002/diff/260001/components/policy/reso...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/1150373002/diff/260001/components/policy/reso...
components/policy/resources/policy_templates.json:7377: 'description': '''If set
to true, this extension can use all corporate keys to sign arbitrary data. If
set to false, it cannot access any corporate key and the user can not grant such
permission either.''',
On 2015/06/15 17:28:16, bartfab wrote:
> 1: What is a corporate key? I never heard or saw the term before in our
> documentation.
Changed it to "keys designated for corporate usage".
The concept of "corporate usage" is effectively introduced by this CL and means:
a key created by the enterprise.platformKeys API is by default "for corporate
usage". currently no other keys have that intended purpose. In the future, I
would like to add however the option to also mark imported keys corporate usage.
If marked that way, the admin can allow certain extensions to use these keys
through this policy. That is not possible for other types of keys nor can the
user do this for corporate keys himself.

Do you think more of this explanation should be part of the policy? Maybe it
will make more sense in an admin help document.

> 2: Nit: s/key and/keys and/
done.

https://codereview.chromium.org/1150373002/diff/260001/components/policy/reso...
components/policy/resources/policy_templates.json:7400: By default an extension
cannot use a private key for signing data, which is equivalent to setting
allowCorporateKeyUsage to false for that extension. A user cannot workaround
that restriction.
On 2015/06/15 17:28:15, bartfab wrote:
> Nit 1: Of course you can use "private keys" in general. You just cannot use
> private keys associated with corporate key pairs.
> Nit 2: s/workaround/work around/ or better, s/workaround/circumvent/

Done.

https://codereview.chromium.org/1150373002/diff/260001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1150373002/diff/260001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:53051: +  <int value="302" label="Grants
extensions access to private keys"/>
On 2015/06/15 17:28:16, bartfab wrote:
> Nit: Please keep this in sync with the caption in policy_templates.json

Done.

[bartfab (slow), 2015-06-17 09:57:40]

chrome/browser/policy/*
chrome/test/data/policy/*
components/policy/*
tools/metrics/histograms/histograms.xml
chrome/browser/chromeos/platform_keys/key_permissions_policy_handlers.*

LGTM but I think the description in policy_templates.json can do with a bit more
tweaking.

https://codereview.chromium.org/1150373002/diff/260001/components/policy/reso...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/1150373002/diff/260001/components/policy/reso...
components/policy/resources/policy_templates.json:7377: 'description': '''If set
to true, this extension can use all corporate keys to sign arbitrary data. If
set to false, it cannot access any corporate key and the user can not grant such
permission either.''',
On 2015/06/17 08:59:56, pneubeck wrote:
> On 2015/06/15 17:28:16, bartfab wrote:
> > 1: What is a corporate key? I never heard or saw the term before in our
> > documentation.
> Changed it to "keys designated for corporate usage".
> The concept of "corporate usage" is effectively introduced by this CL and
means:
> a key created by the enterprise.platformKeys API is by default "for corporate
> usage". currently no other keys have that intended purpose. In the future, I
> would like to add however the option to also mark imported keys corporate
usage.
> If marked that way, the admin can allow certain extensions to use these keys
> through this policy. That is not possible for other types of keys nor can the
> user do this for corporate keys himself.
> 
> Do you think more of this explanation should be part of the policy? Maybe it
> will make more sense in an admin help document.
> 
> > 2: Nit: s/key and/keys and/
> done.

1: I think a bit more explanation would be useful.
2: Nit: s/keys,/keys/
3: Nit: s/usage,/usage/

https://codereview.chromium.org/1150373002/diff/280001/components/policy/reso...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/1150373002/diff/280001/components/policy/reso...
components/policy/resources/policy_templates.json:7398: 'desc': '''Grants usage
of platform keys by extensions.
Nit 1: s/usage of/acces to/
Nit 2: s/platform/corportate platform/
Nit 3: s/by/to/

https://codereview.chromium.org/1150373002/diff/280001/components/policy/reso...
components/policy/resources/policy_templates.json:7403: },
If you do decide to add a bit of explanation, I think it would be easiest to
phrase this description the other way around:

1. Describe what "corporate key" means.
2. State the default (no access).
3. Indicate that this policy can grant access.

[cschuet (SLOW), 2015-06-17 11:25:37]

LGTM with nits.

https://codereview.chromium.org/1150373002/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/platform_keys/key_permissions.cc (right):

https://codereview.chromium.org/1150373002/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions.cc:40: // Do not change
this constant as clients will lose their existing state.
In general what is missing here is short description of the scenarios: managed
users vs. unmanaged. Where do corporate keys come from. What is special if an
extension used the enterprisePlatformKeys API to generate a key...

https://codereview.chromium.org/1150373002/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions.cc:100: 
So sign_once overrides in a way corporate key policy? That means if an extension
generated a key, it can use it for signing once regardless of whether the key is
corporate or not and regardless of the state of the policy?

Maybe this deserves a comment somewhere.

[pneubeck (no reviews), 2015-06-17 13:00:36]

Christoph, ptal at the changed comments in key_permissions.* and the slightly
changed implementation,.


Bartosz, ptal at the changed documentation of the policy.

https://codereview.chromium.org/1150373002/diff/260001/components/policy/reso...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/1150373002/diff/260001/components/policy/reso...
components/policy/resources/policy_templates.json:7377: 'description': '''If set
to true, this extension can use all corporate keys to sign arbitrary data. If
set to false, it cannot access any corporate key and the user can not grant such
permission either.''',
i extended the description of the whole policy.

On 2015/06/17 09:57:40, bartfab wrote:
> On 2015/06/17 08:59:56, pneubeck wrote:
> > On 2015/06/15 17:28:16, bartfab wrote:
> > > 1: What is a corporate key? I never heard or saw the term before in our
> > > documentation.
> > Changed it to "keys designated for corporate usage".
> > The concept of "corporate usage" is effectively introduced by this CL and
> means:
> > a key created by the enterprise.platformKeys API is by default "for
corporate
> > usage". currently no other keys have that intended purpose. In the future, I
> > would like to add however the option to also mark imported keys corporate
> usage.
> > If marked that way, the admin can allow certain extensions to use these keys
> > through this policy. That is not possible for other types of keys nor can
the
> > user do this for corporate keys himself.
> > 
> > Do you think more of this explanation should be part of the policy? Maybe it
> > will make more sense in an admin help document.
> > 
> > > 2: Nit: s/key and/keys and/
> > done.
> 
> 1: I think a bit more explanation would be useful.
> 2: Nit: s/keys,/keys/
> 3: Nit: s/usage,/usage/

https://codereview.chromium.org/1150373002/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/platform_keys/key_permissions.cc (right):

https://codereview.chromium.org/1150373002/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions.cc:100: 
On 2015/06/17 11:25:36, cschuet wrote:
> So sign_once overrides in a way corporate key policy? That means if an
extension
> generated a key, it can use it for signing once regardless of whether the key
is
> corporate or not and regardless of the state of the policy?
> 
> Maybe this deserves a comment somewhere.

Done.

https://codereview.chromium.org/1150373002/diff/280001/components/policy/reso...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/1150373002/diff/280001/components/policy/reso...
components/policy/resources/policy_templates.json:7398: 'desc': '''Grants usage
of platform keys by extensions.
On 2015/06/17 09:57:40, bartfab wrote:
> Nit 1: s/usage of/acces to/
> Nit 2: s/platform/corportate platform/
> Nit 3: s/by/to/

Done.

https://codereview.chromium.org/1150373002/diff/280001/components/policy/reso...
components/policy/resources/policy_templates.json:7403: },
On 2015/06/17 09:57:40, bartfab wrote:
> If you do decide to add a bit of explanation, I think it would be easiest to
> phrase this description the other way around:
> 
> 1. Describe what "corporate key" means.
> 2. State the default (no access).
> 3. Indicate that this policy can grant access.

Done.

[pneubeck (no reviews), 2015-06-17 13:00:46]

Dominic, please review changes in 
  chrome/browser/prefs/browser_prefs.cc

[pneubeck (no reviews), 2015-06-17 13:00:56]

pneubeck@chromium.org changed reviewers:
+ battre@chromium.org

[pneubeck (no reviews), 2015-06-17 13:00:57]

@Dominic, please review changes in 
  chrome/browser/prefs/browser_prefs.cc

[pneubeck (no reviews), 2015-06-17 13:01:43]

pneubeck@chromium.org changed reviewers:
+ asvitkine@chromium.org

[pneubeck (no reviews), 2015-06-17 13:01:44]

asvitkine@chromium.org: Please review changes in 
 tools/metrics/histograms/histograms.xml

[pneubeck (no reviews), 2015-06-17 13:04:02]

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

[pneubeck (no reviews), 2015-06-17 13:04:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
bartfab@chromium.org, cschuet@chromium.org
Link to the patchset: https://codereview.chromium.org/1150373002/#ps320001
(title: "Rebased.")

[commit-bot: I haz the power, 2015-06-17 13:04:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1150373002/320001

[Alexei Svitkine (slow), 2015-06-17 14:47:04]

lgtm

[cschuet (SLOW), 2015-06-17 15:03:26]

On 2015/06/17 14:47:04, Alexei Svitkine wrote:
> lgtm

lgtm++

[cschuet (SLOW), 2015-06-17 15:03:26]

On 2015/06/17 14:47:04, Alexei Svitkine wrote:
> lgtm

lgtm++

[battre, 2015-06-17 15:11:48]

chrome/browser/prefs/browser_prefs.cc LGTM

I did not do a proper review of the rest, just noticed some nits.

https://codereview.chromium.org/1150373002/diff/320001/chrome/browser/chromeo...
File chrome/browser/chromeos/platform_keys/key_permissions.cc (right):

https://codereview.chromium.org/1150373002/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions.cc:46: const char
kPrefKeyUsage[] = "keyUsage";
Add an example how these are used like above?

https://codereview.chromium.org/1150373002/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions.cc:104: // except if
explicitly permitted by the Administrator.
nit: administrator

https://codereview.chromium.org/1150373002/diff/320001/chrome/browser/chromeo...
File chrome/browser/chromeos/platform_keys/key_permissions.h (right):

https://codereview.chromium.org/1150373002/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions.h:41: // The user is under
full control of the keys are generated or imported while
grammar "The user is under full control of the keys are generated"

https://codereview.chromium.org/1150373002/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions.h:138: bool
PolicyAllowsCorporateKeyUsage();
The name of the function suggests that it might be const.

https://codereview.chromium.org/1150373002/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions.h:149: // |profile_prefs|
and |extensions_state_store| must not be null and outlive
and must outlive

https://codereview.chromium.org/1150373002/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions.h:152: //
|profile_policies| must not be null and outlive this object.
and must outlive

https://codereview.chromium.org/1150373002/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions.h:179: bool
IsCorporateKey(const std::string& public_key_spki_der_b64);
const?

https://codereview.chromium.org/1150373002/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions.h:193: const std::string&
public_key_spki_der_b64);
const?

[commit-bot: I haz the power, 2015-06-17 15:47:11]

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

[commit-bot: I haz the power, 2015-06-17 15:47:12]

Dry run: This issue passed the CQ dry run.

[pneubeck (no reviews), 2015-06-18 08:45:16]

thanks for the reviews!

https://codereview.chromium.org/1150373002/diff/320001/chrome/browser/chromeo...
File chrome/browser/chromeos/platform_keys/key_permissions.cc (right):

https://codereview.chromium.org/1150373002/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions.cc:46: const char
kPrefKeyUsage[] = "keyUsage";
On 2015/06/17 15:11:47, battre wrote:
> Add an example how these are used like above?

Done.

https://codereview.chromium.org/1150373002/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions.cc:104: // except if
explicitly permitted by the Administrator.
On 2015/06/17 15:11:47, battre wrote:
> nit: administrator

Done.

https://codereview.chromium.org/1150373002/diff/320001/chrome/browser/chromeo...
File chrome/browser/chromeos/platform_keys/key_permissions.h (right):

https://codereview.chromium.org/1150373002/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions.h:41: // The user is under
full control of the keys are generated or imported while
On 2015/06/17 15:11:47, battre wrote:
> grammar "The user is under full control of the keys are generated"

Done.

https://codereview.chromium.org/1150373002/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions.h:138: bool
PolicyAllowsCorporateKeyUsage();
On 2015/06/17 15:11:47, battre wrote:
> The name of the function suggests that it might be const.

Done.

https://codereview.chromium.org/1150373002/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions.h:149: // |profile_prefs|
and |extensions_state_store| must not be null and outlive
On 2015/06/17 15:11:47, battre wrote:
> and must outlive

Done.

https://codereview.chromium.org/1150373002/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions.h:152: //
|profile_policies| must not be null and outlive this object.
On 2015/06/17 15:11:47, battre wrote:
> and must outlive

Done.

https://codereview.chromium.org/1150373002/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions.h:179: bool
IsCorporateKey(const std::string& public_key_spki_der_b64);
On 2015/06/17 15:11:47, battre wrote:
> const?

Done.

https://codereview.chromium.org/1150373002/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/key_permissions.h:193: const std::string&
public_key_spki_der_b64);
On 2015/06/17 15:11:47, battre wrote:
> const?

Done.

[pneubeck (no reviews), 2015-06-18 08:45:32]

The CQ bit was checked by pneubeck@chromium.org

[pneubeck (no reviews), 2015-06-18 08:45:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
bartfab@chromium.org, asvitkine@chromium.org, cschuet@chromium.org,
battre@chromium.org
Link to the patchset: https://codereview.chromium.org/1150373002/#ps340001
(title: "Addressed comments.")

[commit-bot: I haz the power, 2015-06-18 08:45:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1150373002/340001

[commit-bot: I haz the power, 2015-06-18 08:50:42]

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

[commit-bot: I haz the power, 2015-06-18 08:50:44]

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

[pneubeck (no reviews), 2015-06-18 09:21:56]

The CQ bit was checked by pneubeck@chromium.org

[pneubeck (no reviews), 2015-06-18 09:21:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
bartfab@chromium.org, asvitkine@chromium.org, cschuet@chromium.org,
battre@chromium.org
Link to the patchset: https://codereview.chromium.org/1150373002/#ps360001
(title: "Rebased.")

[commit-bot: I haz the power, 2015-06-18 09:22:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1150373002/360001

[commit-bot: I haz the power, 2015-06-18 10:42:45]

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

[commit-bot: I haz the power, 2015-06-18 10:42:47]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[pneubeck (no reviews), 2015-06-18 11:15:28]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2015-06-18 11:15:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1150373002/360001

[commit-bot: I haz the power, 2015-06-18 12:38:32]

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

[commit-bot: I haz the power, 2015-06-18 12:38:34]

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

[pneubeck (no reviews), 2015-06-18 12:42:35]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2015-06-18 12:43:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1150373002/360001

[commit-bot: I haz the power, 2015-06-18 13:42:27]

Committed patchset #10 (id:360001)

[commit-bot: I haz the power, 2015-06-18 13:43:16]

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