Implement Enterprise Key API.

Implement Enterprise Key API.

Proposal: https://docs.google.com/a/google.com/document/d/1KxtQCk4SJdAZId8WxtnCxR3kS2KvhFDsU0SdAeP18Xc/edit

The logic of the API is implemented. Add prefernces and policies are added.
The underlying cryptohome_client will be implemented in another CL.

BUG=chromium:219963
TEST=unit_tests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195043

[Mattias Nissler (ping if slow), 2013-04-04 12:51:34]

some initial comments, you also want to run this by extension people.

https://codereview.chromium.org/13132004/diff/40001/chrome/app/policy/policy_...
File chrome/app/policy/policy_templates.json (right):

https://codereview.chromium.org/13132004/diff/40001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:4516: 'per_profile': False,
Any chance to make this per-profile?

https://codereview.chromium.org/13132004/diff/40001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:4521: 'desc': '''Allow the user to
remote attest using the device TPM.''',
Please elaborate on the description. It should specify behavior for the
different boolean values and also for the case that the policy is not set.

https://codereview.chromium.org/13132004/diff/40001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:4533: 'per_profile': False,
ditto

https://codereview.chromium.org/13132004/diff/40001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:4538: 'desc': '''Only extenions which
are whitelisted can use the enterprise remote attestation API.''',
ditto

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
File
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc
(right):

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:7:
#include <string>
newline

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:48:
install_attributes_ = new policy::EnterpriseInstallAttributes(cryptohome);
Please use the instance that you can retrieve from
g_browser_process()->browser_policy_connector().

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:52:
delete install_attributes_;
This should have been wrapped in a scoped_ptr, but no longer applies if you use
the globe instance.

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:58:
return settings->GetBoolean(chromeos::kDeviceAttestationEnabled, &value) &&
This should probably retrieve the trusted value, see comments in CrosSettings

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:109:
LOG(ERROR) << "About to call TpmAttestationSignEnterpriseChallenge()";
DVLOG would be more appropriate.

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:146:
// ChallengeUserKey()
huh?

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:200:
if (user_domain != GetEnterpriseDomain()) {
You could use BrowserPolicyConnector::GetUserAffiliation for this.

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:329:
// TODO(davidyu): right now we just simply reject the request before we have
That'll make testing rather hard?

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:340:
if (id == extension_->id()) {
no need for braces for single-line conditional/body constructs.

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
File
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.h
(right):

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.h:8:
#include <string>
newline

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.h:39:
~EEKPChallengeKeyBase();
should have a virtual dtor

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.h:71:
EnterpriseEnterpriseKeyPrivateChallengeMachineKeyFunction;
indentation

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.h:112:
EnterpriseEnterpriseKeyPrivateChallengeUserKeyFunction;
indentation
0

https://codereview.chromium.org/13132004/diff/40001/chrome/common/extensions/...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/13132004/diff/40001/chrome/common/extensions/...
chrome/common/extensions/api/_permission_features.json:159:
"ghdilpkmfbfdnomkmaiogjhjnggaggoi"  // Named Key Extension
I think this shouldn't be here anymore.

https://codereview.chromium.org/13132004/diff/40001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/13132004/diff/40001/chrome/common/pref_names....
chrome/common/pref_names.cc:2368: #if defined(OS_CHROMEOS)
This should be added to the relevant section of the file. There's local state,
user prefs, service prefs, and we have OS_CHROMEOS blocks already.

https://codereview.chromium.org/13132004/diff/40001/chrome/common/pref_names....
chrome/common/pref_names.cc:2371: // Indicates whether the remote attestation is
enabled for the user.
fix comment

[davidyu, 2013-04-09 09:30:08]

https://codereview.chromium.org/13132004/diff/40001/chrome/app/policy/policy_...
File chrome/app/policy/policy_templates.json (right):

https://codereview.chromium.org/13132004/diff/40001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:4516: 'per_profile': False,
On 2013/04/04 12:51:34, Mattias Nissler wrote:
> Any chance to make this per-profile?

I'm not quite sure about the difference for this feature. This policy is
supposed to apply on the user account, no matter which machine it is used.

https://codereview.chromium.org/13132004/diff/40001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:4521: 'desc': '''Allow the user to
remote attest using the device TPM.''',
On 2013/04/04 12:51:34, Mattias Nissler wrote:
> Please elaborate on the description. It should specify behavior for the
> different boolean values and also for the case that the policy is not set.

Done.

https://codereview.chromium.org/13132004/diff/40001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:4538: 'desc': '''Only extenions which
are whitelisted can use the enterprise remote attestation API.''',
On 2013/04/04 12:51:34, Mattias Nissler wrote:
> ditto

Done.

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
File
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc
(right):

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:7:
#include <string>
On 2013/04/04 12:51:34, Mattias Nissler wrote:
> newline

Done.

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:48:
install_attributes_ = new policy::EnterpriseInstallAttributes(cryptohome);
On 2013/04/04 12:51:34, Mattias Nissler wrote:
> Please use the instance that you can retrieve from
> g_browser_process()->browser_policy_connector().

Done.

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:52:
delete install_attributes_;
On 2013/04/04 12:51:34, Mattias Nissler wrote:
> This should have been wrapped in a scoped_ptr, but no longer applies if you
use
> the globe instance.

Done.

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:58:
return settings->GetBoolean(chromeos::kDeviceAttestationEnabled, &value) &&
On 2013/04/04 12:51:34, Mattias Nissler wrote:
> This should probably retrieve the trusted value, see comments in CrosSettings

Done.

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:109:
LOG(ERROR) << "About to call TpmAttestationSignEnterpriseChallenge()";
On 2013/04/04 12:51:34, Mattias Nissler wrote:
> DVLOG would be more appropriate.

Removed.

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:146:
// ChallengeUserKey()
On 2013/04/04 12:51:34, Mattias Nissler wrote:
> huh?

I just want to say that the methods below are for implementing
ChallengeUserKey(), since the class name is really long and hard to read.

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:200:
if (user_domain != GetEnterpriseDomain()) {
On 2013/04/04 12:51:34, Mattias Nissler wrote:
> You could use BrowserPolicyConnector::GetUserAffiliation for this.

I could. However, for personal devices, we still need to extract the domain from
the user email, using GetUserAffiliation doesn't save us much. I fixed the
implementation of the domain extracting code with Gaia util function instead.

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:329:
// TODO(davidyu): right now we just simply reject the request before we have
On 2013/04/04 12:51:34, Mattias Nissler wrote:
> That'll make testing rather hard?

I'll fix this once we have a conclusion about how to get user consent. As of
testing, it should be fine not to test this part as long as it is disabled in
the code. I'll add the test once if becomes more testable.

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:340:
if (id == extension_->id()) {
On 2013/04/04 12:51:34, Mattias Nissler wrote:
> no need for braces for single-line conditional/body constructs.

Done.

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
File
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.h
(right):

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.h:8:
#include <string>
On 2013/04/04 12:51:34, Mattias Nissler wrote:
> newline

Done.

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.h:39:
~EEKPChallengeKeyBase();
On 2013/04/04 12:51:34, Mattias Nissler wrote:
> should have a virtual dtor

Nice catch. Done.

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.h:71:
EnterpriseEnterpriseKeyPrivateChallengeMachineKeyFunction;
On 2013/04/04 12:51:34, Mattias Nissler wrote:
> indentation

Done.

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.h:112:
EnterpriseEnterpriseKeyPrivateChallengeUserKeyFunction;
On 2013/04/04 12:51:34, Mattias Nissler wrote:
> indentation
> 0

Done.

https://codereview.chromium.org/13132004/diff/40001/chrome/common/extensions/...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/13132004/diff/40001/chrome/common/extensions/...
chrome/common/extensions/api/_permission_features.json:159:
"ghdilpkmfbfdnomkmaiogjhjnggaggoi"  // Named Key Extension
On 2013/04/04 12:51:34, Mattias Nissler wrote:
> I think this shouldn't be here anymore.

Done.

https://codereview.chromium.org/13132004/diff/40001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/13132004/diff/40001/chrome/common/pref_names....
chrome/common/pref_names.cc:2368: #if defined(OS_CHROMEOS)
On 2013/04/04 12:51:34, Mattias Nissler wrote:
> This should be added to the relevant section of the file. There's local state,
> user prefs, service prefs, and we have OS_CHROMEOS blocks already.

Done.

https://codereview.chromium.org/13132004/diff/40001/chrome/common/pref_names....
chrome/common/pref_names.cc:2371: // Indicates whether the remote attestation is
enabled for the user.
On 2013/04/04 12:51:34, Mattias Nissler wrote:
> fix comment

Done.

[Mattias Nissler (ping if slow), 2013-04-10 16:53:25]

More prefs/policy comments, I'll leave it to Matt to review  the extensions
side.

https://codereview.chromium.org/13132004/diff/40001/chrome/app/policy/policy_...
File chrome/app/policy/policy_templates.json (right):

https://codereview.chromium.org/13132004/diff/40001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:4516: 'per_profile': False,
On 2013/04/09 09:30:08, davidyu wrote:
> On 2013/04/04 12:51:34, Mattias Nissler wrote:
> > Any chance to make this per-profile?
> 
> I'm not quite sure about the difference for this feature. This policy is
> supposed to apply on the user account, no matter which machine it is used.

The definition of this flag is: "Can we set the policy differently for different
Profiles that are loaded in the same browser instance?" On the technical level,
this means basically "are you reading the policy via Profile::GetPrefs()"?

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
File
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc
(right):

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:200:
if (user_domain != GetEnterpriseDomain()) {
On 2013/04/09 09:30:08, davidyu wrote:
> On 2013/04/04 12:51:34, Mattias Nissler wrote:
> > You could use BrowserPolicyConnector::GetUserAffiliation for this.
> 
> I could. However, for personal devices, we still need to extract the domain
from
> the user email, using GetUserAffiliation doesn't save us much. I fixed the
> implementation of the domain extracting code with Gaia util function instead.

Wait, this code only runs for enterprise devices, I don't get your comment on
personal devices.

https://codereview.chromium.org/13132004/diff/47001/chrome/app/policy/policy_...
File chrome/app/policy/policy_templates.json (right):

https://codereview.chromium.org/13132004/diff/47001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:4516: 'per_profile': False,
per_profile: False implies that this will not work with multi-profile. New
features should be per_profile: True if possible, any thoughts on this?

https://codereview.chromium.org/13132004/diff/47001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:4533: 'per_profile': False,
same here, I guess this probably should be True.

https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
File
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc
(right):

https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:18:
#include "chrome/browser/infobars/infobar_service.h"
used?

https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:36:

remove extra newline

https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:40:
const char* EEKPChallengeKeyBase::kKeyName = "attest-ent-default";
array for consistency with other code

https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:350:
profile()->GetPrefs()->GetList(prefs::kAttestationExtensionWhitelist);
Ah, so you're per_profile : True after all :)

https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:352:
for (int i = 0; i < static_cast<int>(list->GetSize()); ++i) {
How about base::ListValue::Find()?

https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
File
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.h
(right):

https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.h:36:
static const char* kKeyName;
This should be an array.

https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.h:40:
const;
We usually break the line before the parameter in these case.

https://codereview.chromium.org/13132004/diff/47001/chrome/browser/policy/con...
File chrome/browser/policy/configuration_policy_handler_list.cc (right):

https://codereview.chromium.org/13132004/diff/47001/chrome/browser/policy/con...
chrome/browser/policy/configuration_policy_handler_list.cc:383: Value::TYPE_LIST
},
This should use ExtensionListPolicyHandler instead so we can detect badly
formatted extension ids and report them on about:policy instead.

https://codereview.chromium.org/13132004/diff/47001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/13132004/diff/47001/chrome/common/pref_names....
chrome/common/pref_names.cc:2362: const char kAttestationExtensionWhitelist[] =
"attestation.extension_whitelist";
This is still not in the right section. People keep adding to the end, and seem
to have a lot recently. This should be in the PROFILE PREFS section starting in
line 12 not in the SERVICE PREFS section starting in line 2076.

And yes, I should commit a big comment here to stop people from mixing prefs
even further.

[Matt Perry, 2013-04-10 21:14:01]

https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
File
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc
(right):

https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:115:
AddRef();
You should be able to remove the manual AddRef and Releases. Bind() adds a
reference for you until the callback is actually invoked.

https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
File
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.h
(right):

https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.h:39:
void GetDeviceAttestationEnabled(const base::Callback<void(bool)>& callback)
comment these methods

https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.h:62:
"enterprise.enterpriseKeyPrivate.challengeMachineKey",
this is a mouthful. Have you considered naming the API enterprise.keyPrivate?

[davidyu, 2013-04-11 07:04:21]

https://codereview.chromium.org/13132004/diff/40001/chrome/app/policy/policy_...
File chrome/app/policy/policy_templates.json (right):

https://codereview.chromium.org/13132004/diff/40001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:4516: 'per_profile': False,
On 2013/04/10 16:53:25, Mattias Nissler wrote:
> On 2013/04/09 09:30:08, davidyu wrote:
> > On 2013/04/04 12:51:34, Mattias Nissler wrote:
> > > Any chance to make this per-profile?
> > 
> > I'm not quite sure about the difference for this feature. This policy is
> > supposed to apply on the user account, no matter which machine it is used.
> 
> The definition of this flag is: "Can we set the policy differently for
different
> Profiles that are loaded in the same browser instance?" On the technical
level,
> this means basically "are you reading the policy via Profile::GetPrefs()"?

Got it. Changed to true.

https://codereview.chromium.org/13132004/diff/40001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:4533: 'per_profile': False,
On 2013/04/04 12:51:34, Mattias Nissler wrote:
> ditto

Done.

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
File
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc
(right):

https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:200:
if (user_domain != GetEnterpriseDomain()) {
Right. For this branch, it runs for enterprise devices. But I still need to have
a function to get user domain for the other branch for non-enterprise devices.
So calling GetUserAffiliation basically just equivalent to a simple string
comparison.

On 2013/04/10 16:53:25, Mattias Nissler wrote:
> On 2013/04/09 09:30:08, davidyu wrote:
> > On 2013/04/04 12:51:34, Mattias Nissler wrote:
> > > You could use BrowserPolicyConnector::GetUserAffiliation for this.
> > 
> > I could. However, for personal devices, we still need to extract the domain
> from
> > the user email, using GetUserAffiliation doesn't save us much. I fixed the
> > implementation of the domain extracting code with Gaia util function
instead.
> 
> Wait, this code only runs for enterprise devices, I don't get your comment on
> personal devices.

https://codereview.chromium.org/13132004/diff/47001/chrome/app/policy/policy_...
File chrome/app/policy/policy_templates.json (right):

https://codereview.chromium.org/13132004/diff/47001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:4516: 'per_profile': False,
On 2013/04/10 16:53:26, Mattias Nissler wrote:
> per_profile: False implies that this will not work with multi-profile. New
> features should be per_profile: True if possible, any thoughts on this?

Done.

https://codereview.chromium.org/13132004/diff/47001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:4533: 'per_profile': False,
On 2013/04/10 16:53:26, Mattias Nissler wrote:
> same here, I guess this probably should be True.

Done.

https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
File
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc
(right):

https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:18:
#include "chrome/browser/infobars/infobar_service.h"
On 2013/04/10 16:53:26, Mattias Nissler wrote:
> used?

Done.

https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:36:

On 2013/04/10 16:53:26, Mattias Nissler wrote:
> remove extra newline

Done.

https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:40:
const char* EEKPChallengeKeyBase::kKeyName = "attest-ent-default";
On 2013/04/10 16:53:26, Mattias Nissler wrote:
> array for consistency with other code

Done.

https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:115:
AddRef();
On 2013/04/10 21:14:01, Matt Perry wrote:
> You should be able to remove the manual AddRef and Releases. Bind() adds a
> reference for you until the callback is actually invoked.

Done.

https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:350:
profile()->GetPrefs()->GetList(prefs::kAttestationExtensionWhitelist);
On 2013/04/10 16:53:26, Mattias Nissler wrote:
> Ah, so you're per_profile : True after all :)

Done.

https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:352:
for (int i = 0; i < static_cast<int>(list->GetSize()); ++i) {
On 2013/04/10 16:53:26, Mattias Nissler wrote:
> How about base::ListValue::Find()?

Done.

https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
File
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.h
(right):

https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.h:36:
static const char* kKeyName;
On 2013/04/10 16:53:26, Mattias Nissler wrote:
> This should be an array.

Done.

https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.h:39:
void GetDeviceAttestationEnabled(const base::Callback<void(bool)>& callback)
On 2013/04/10 21:14:01, Matt Perry wrote:
> comment these methods

Done.

https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.h:40:
const;
On 2013/04/10 16:53:26, Mattias Nissler wrote:
> We usually break the line before the parameter in these case.

Done.

https://codereview.chromium.org/13132004/diff/47001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/13132004/diff/47001/chrome/common/pref_names....
chrome/common/pref_names.cc:2362: const char kAttestationExtensionWhitelist[] =
"attestation.extension_whitelist";
On 2013/04/10 16:53:26, Mattias Nissler wrote:
> This is still not in the right section. People keep adding to the end, and
seem
> to have a lot recently. This should be in the PROFILE PREFS section starting
in
> line 12 not in the SERVICE PREFS section starting in line 2076.
> 
> And yes, I should commit a big comment here to stop people from mixing prefs
> even further.

Done.

[davidyu, 2013-04-16 05:47:36]

Reviewers, PTAL. Thanks!

On 2013/04/11 07:04:21, davidyu wrote:
>
https://codereview.chromium.org/13132004/diff/40001/chrome/app/policy/policy_...
> File chrome/app/policy/policy_templates.json (right):
> 
>
https://codereview.chromium.org/13132004/diff/40001/chrome/app/policy/policy_...
> chrome/app/policy/policy_templates.json:4516: 'per_profile': False,
> On 2013/04/10 16:53:25, Mattias Nissler wrote:
> > On 2013/04/09 09:30:08, davidyu wrote:
> > > On 2013/04/04 12:51:34, Mattias Nissler wrote:
> > > > Any chance to make this per-profile?
> > > 
> > > I'm not quite sure about the difference for this feature. This policy is
> > > supposed to apply on the user account, no matter which machine it is used.
> > 
> > The definition of this flag is: "Can we set the policy differently for
> different
> > Profiles that are loaded in the same browser instance?" On the technical
> level,
> > this means basically "are you reading the policy via Profile::GetPrefs()"?
> 
> Got it. Changed to true.
> 
>
https://codereview.chromium.org/13132004/diff/40001/chrome/app/policy/policy_...
> chrome/app/policy/policy_templates.json:4533: 'per_profile': False,
> On 2013/04/04 12:51:34, Mattias Nissler wrote:
> > ditto
> 
> Done.
> 
>
https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
> File
>
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc
> (right):
> 
>
https://codereview.chromium.org/13132004/diff/40001/chrome/browser/extensions...
>
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:200:
> if (user_domain != GetEnterpriseDomain()) {
> Right. For this branch, it runs for enterprise devices. But I still need to
have
> a function to get user domain for the other branch for non-enterprise devices.
> So calling GetUserAffiliation basically just equivalent to a simple string
> comparison.
> 
> On 2013/04/10 16:53:25, Mattias Nissler wrote:
> > On 2013/04/09 09:30:08, davidyu wrote:
> > > On 2013/04/04 12:51:34, Mattias Nissler wrote:
> > > > You could use BrowserPolicyConnector::GetUserAffiliation for this.
> > > 
> > > I could. However, for personal devices, we still need to extract the
domain
> > from
> > > the user email, using GetUserAffiliation doesn't save us much. I fixed the
> > > implementation of the domain extracting code with Gaia util function
> instead.
> > 
> > Wait, this code only runs for enterprise devices, I don't get your comment
on
> > personal devices.
> 
>
https://codereview.chromium.org/13132004/diff/47001/chrome/app/policy/policy_...
> File chrome/app/policy/policy_templates.json (right):
> 
>
https://codereview.chromium.org/13132004/diff/47001/chrome/app/policy/policy_...
> chrome/app/policy/policy_templates.json:4516: 'per_profile': False,
> On 2013/04/10 16:53:26, Mattias Nissler wrote:
> > per_profile: False implies that this will not work with multi-profile. New
> > features should be per_profile: True if possible, any thoughts on this?
> 
> Done.
> 
>
https://codereview.chromium.org/13132004/diff/47001/chrome/app/policy/policy_...
> chrome/app/policy/policy_templates.json:4533: 'per_profile': False,
> On 2013/04/10 16:53:26, Mattias Nissler wrote:
> > same here, I guess this probably should be True.
> 
> Done.
> 
>
https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
> File
>
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc
> (right):
> 
>
https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
>
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:18:
> #include "chrome/browser/infobars/infobar_service.h"
> On 2013/04/10 16:53:26, Mattias Nissler wrote:
> > used?
> 
> Done.
> 
>
https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
>
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:36:
> 
> On 2013/04/10 16:53:26, Mattias Nissler wrote:
> > remove extra newline
> 
> Done.
> 
>
https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
>
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:40:
> const char* EEKPChallengeKeyBase::kKeyName = "attest-ent-default";
> On 2013/04/10 16:53:26, Mattias Nissler wrote:
> > array for consistency with other code
> 
> Done.
> 
>
https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
>
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:115:
> AddRef();
> On 2013/04/10 21:14:01, Matt Perry wrote:
> > You should be able to remove the manual AddRef and Releases. Bind() adds a
> > reference for you until the callback is actually invoked.
> 
> Done.
> 
>
https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
>
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:350:
> profile()->GetPrefs()->GetList(prefs::kAttestationExtensionWhitelist);
> On 2013/04/10 16:53:26, Mattias Nissler wrote:
> > Ah, so you're per_profile : True after all :)
> 
> Done.
> 
>
https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
>
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.cc:352:
> for (int i = 0; i < static_cast<int>(list->GetSize()); ++i) {
> On 2013/04/10 16:53:26, Mattias Nissler wrote:
> > How about base::ListValue::Find()?
> 
> Done.
> 
>
https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
> File
>
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.h
> (right):
> 
>
https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
>
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.h:36:
> static const char* kKeyName;
> On 2013/04/10 16:53:26, Mattias Nissler wrote:
> > This should be an array.
> 
> Done.
> 
>
https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
>
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.h:39:
> void GetDeviceAttestationEnabled(const base::Callback<void(bool)>& callback)
> On 2013/04/10 21:14:01, Matt Perry wrote:
> > comment these methods
> 
> Done.
> 
>
https://codereview.chromium.org/13132004/diff/47001/chrome/browser/extensions...
>
chrome/browser/extensions/api/enterprise_enterprise_key_private/enterprise_enterprise_key_private_api.h:40:
> const;
> On 2013/04/10 16:53:26, Mattias Nissler wrote:
> > We usually break the line before the parameter in these case.
> 
> Done.
> 
>
https://codereview.chromium.org/13132004/diff/47001/chrome/common/pref_names.cc
> File chrome/common/pref_names.cc (right):
> 
>
https://codereview.chromium.org/13132004/diff/47001/chrome/common/pref_names....
> chrome/common/pref_names.cc:2362: const char kAttestationExtensionWhitelist[]
=
> "attestation.extension_whitelist";
> On 2013/04/10 16:53:26, Mattias Nissler wrote:
> > This is still not in the right section. People keep adding to the end, and
> seem
> > to have a lot recently. This should be in the PROFILE PREFS section starting
> in
> > line 12 not in the SERVICE PREFS section starting in line 2076.
> > 
> > And yes, I should commit a big comment here to stop people from mixing prefs
> > even further.
> 
> Done.

[Mattias Nissler (ping if slow), 2013-04-16 15:50:25]

Only a couple minor things from my side.

https://codereview.chromium.org/13132004/diff/57001/chrome/app/policy/policy_...
File chrome/app/policy/policy_templates.json (right):

https://codereview.chromium.org/13132004/diff/57001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:4521: 'desc': '''If true, the user can
use the hardware on Chrome devices to remote attest its identity to the privacy
CA. Otherwise, this function is disabled.''',
Please explain here what "disabled" means.

Also needs a sentence explaining what happens when not set. This may seem silly,
but admins have specifically asked us for this piece in the part :)

https://codereview.chromium.org/13132004/diff/57001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:4538: 'desc': '''This policy specifies
the allowed extenions to use Enterprise Key API
chrome.enterprise.enterpriseKeyPrivate.*. Extensions must be added to this list
to use the API.''',
spelling: extensions

Same comments on elaborating what happens if an extension is not on the list and
if the policy is not set.

https://codereview.chromium.org/13132004/diff/59001/chrome/browser/extensions...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc
(right):

https://codereview.chromium.org/13132004/diff/59001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc:246:

nit: remove blank line.

https://codereview.chromium.org/13132004/diff/59001/chrome/browser/extensions...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.h
(right):

https://codereview.chromium.org/13132004/diff/59001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.h:48:
// Returns the enrolled enterprise domain.
nit: "the enterprise domain the device is enrolled to"

https://codereview.chromium.org/13132004/diff/59001/chrome/browser/policy/con...
File chrome/browser/policy/configuration_policy_handler_list.cc (right):

https://codereview.chromium.org/13132004/diff/59001/chrome/browser/policy/con...
chrome/browser/policy/configuration_policy_handler_list.cc:382: Value::TYPE_LIST
},
I had asked in a previous comment whether you can use ExtensionListPolicyHandler
here?

https://codereview.chromium.org/13132004/diff/59001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/13132004/diff/59001/chrome/common/pref_names....
chrome/common/pref_names.h:897: #endif
proper sorting here as well (so we're in sync with pref_names.cc).

[Matt Perry, 2013-04-16 17:48:08]

lgtm

[davidyu, 2013-04-17 05:39:15]

https://codereview.chromium.org/13132004/diff/47001/chrome/browser/policy/con...
File chrome/browser/policy/configuration_policy_handler_list.cc (right):

https://codereview.chromium.org/13132004/diff/47001/chrome/browser/policy/con...
chrome/browser/policy/configuration_policy_handler_list.cc:383: Value::TYPE_LIST
},
On 2013/04/10 16:53:26, Mattias Nissler wrote:
> This should use ExtensionListPolicyHandler instead so we can detect badly
> formatted extension ids and report them on about:policy instead.

Done.

https://codereview.chromium.org/13132004/diff/57001/chrome/app/policy/policy_...
File chrome/app/policy/policy_templates.json (right):

https://codereview.chromium.org/13132004/diff/57001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:4521: 'desc': '''If true, the user can
use the hardware on Chrome devices to remote attest its identity to the privacy
CA. Otherwise, this function is disabled.''',
On 2013/04/16 15:50:26, Mattias Nissler wrote:
> Please explain here what "disabled" means.
> 
> Also needs a sentence explaining what happens when not set. This may seem
silly,
> but admins have specifically asked us for this piece in the part :)

Done.

https://codereview.chromium.org/13132004/diff/57001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:4538: 'desc': '''This policy specifies
the allowed extenions to use Enterprise Key API
chrome.enterprise.enterpriseKeyPrivate.*. Extensions must be added to this list
to use the API.''',
On 2013/04/16 15:50:26, Mattias Nissler wrote:
> spelling: extensions
> 
> Same comments on elaborating what happens if an extension is not on the list
and
> if the policy is not set.

Done.

https://codereview.chromium.org/13132004/diff/59001/chrome/browser/extensions...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc
(right):

https://codereview.chromium.org/13132004/diff/59001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc:246:

On 2013/04/16 15:50:26, Mattias Nissler wrote:
> nit: remove blank line.

Done.

https://codereview.chromium.org/13132004/diff/59001/chrome/browser/extensions...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.h
(right):

https://codereview.chromium.org/13132004/diff/59001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.h:48:
// Returns the enrolled enterprise domain.
On 2013/04/16 15:50:26, Mattias Nissler wrote:
> nit: "the enterprise domain the device is enrolled to"

Done.

https://codereview.chromium.org/13132004/diff/59001/chrome/browser/policy/con...
File chrome/browser/policy/configuration_policy_handler_list.cc (right):

https://codereview.chromium.org/13132004/diff/59001/chrome/browser/policy/con...
chrome/browser/policy/configuration_policy_handler_list.cc:382: Value::TYPE_LIST
},
On 2013/04/16 15:50:26, Mattias Nissler wrote:
> I had asked in a previous comment whether you can use
ExtensionListPolicyHandler
> here?

Done.

https://codereview.chromium.org/13132004/diff/59001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/13132004/diff/59001/chrome/common/pref_names....
chrome/common/pref_names.h:897: #endif
On 2013/04/16 15:50:26, Mattias Nissler wrote:
> proper sorting here as well (so we're in sync with pref_names.cc).

Done.

[Mattias Nissler (ping if slow), 2013-04-17 08:09:09]

LGTM w/ nits to fix and pending green trybots.

https://codereview.chromium.org/13132004/diff/70001/chrome/app/policy/policy_...
File chrome/app/policy/policy_templates.json (right):

https://codereview.chromium.org/13132004/diff/70001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:4521: 'desc': '''If true, the user can
use the hardware on Chrome devices to remote attest its identity to the privacy
CA via the Enterprise Platform Keys API
chrome.enterprise.platformKeysPrivate.challengeUserKey(). If it is set to false,
or if it is not set, calls to the API will be rejected.''',
suggestion: "fail with an error code" instead of "rejected".

Also, please put the last sentence in its own paragraph for consistency with the
rest of this file.

https://codereview.chromium.org/13132004/diff/70001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:4538: 'desc': '''This policy specifies
the allowed extensions to use Enterprise Platform Keys API
chrome.enterprise.platformKeysPrivate.challengeUserKey() for remote attestation.
Extensions must be added to this list to use the API. If an extension is not in
the list, or the list is not set, the call to the API will be rejected.''',
ditto

https://codereview.chromium.org/13132004/diff/70001/chrome/browser/extensions...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc
(right):

https://codereview.chromium.org/13132004/diff/70001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc:344:
return true;  // XXX
I take it this is not to be committed?

https://codereview.chromium.org/13132004/diff/70001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc:354:
return true;  // XXX
ditto

https://codereview.chromium.org/13132004/diff/70001/chrome/browser/policy/con...
File chrome/browser/policy/configuration_policy_handler_list.cc (right):

https://codereview.chromium.org/13132004/diff/70001/chrome/browser/policy/con...
chrome/browser/policy/configuration_policy_handler_list.cc:520: false));
Please move to line 438 where the other extension-related handlers are
initialized.

https://codereview.chromium.org/13132004/diff/70001/chrome/browser/policy/con...
File chrome/browser/policy/configuration_policy_pref_store_unittest.cc (right):

https://codereview.chromium.org/13132004/diff/70001/chrome/browser/policy/con...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:125:
prefs::kAttestationExtensionWhitelist)));
This probably no longer passes when using the handler. You can just remove this.

[davidyu, 2013-04-17 08:24:19]

https://codereview.chromium.org/13132004/diff/70001/chrome/app/policy/policy_...
File chrome/app/policy/policy_templates.json (right):

https://codereview.chromium.org/13132004/diff/70001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:4521: 'desc': '''If true, the user can
use the hardware on Chrome devices to remote attest its identity to the privacy
CA via the Enterprise Platform Keys API
chrome.enterprise.platformKeysPrivate.challengeUserKey(). If it is set to false,
or if it is not set, calls to the API will be rejected.''',
On 2013/04/17 08:09:09, Mattias Nissler wrote:
> suggestion: "fail with an error code" instead of "rejected".
> 
> Also, please put the last sentence in its own paragraph for consistency with
the
> rest of this file.

Done.

https://codereview.chromium.org/13132004/diff/70001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:4538: 'desc': '''This policy specifies
the allowed extensions to use Enterprise Platform Keys API
chrome.enterprise.platformKeysPrivate.challengeUserKey() for remote attestation.
Extensions must be added to this list to use the API. If an extension is not in
the list, or the list is not set, the call to the API will be rejected.''',
On 2013/04/17 08:09:09, Mattias Nissler wrote:
> ditto

Done.

https://codereview.chromium.org/13132004/diff/70001/chrome/browser/extensions...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc
(right):

https://codereview.chromium.org/13132004/diff/70001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc:344:
return true;  // XXX
On 2013/04/17 08:09:09, Mattias Nissler wrote:
> I take it this is not to be committed?

ah, right. sorry about that. removed.

https://codereview.chromium.org/13132004/diff/70001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc:354:
return true;  // XXX
On 2013/04/17 08:09:09, Mattias Nissler wrote:
> ditto

Done.

https://codereview.chromium.org/13132004/diff/70001/chrome/browser/policy/con...
File chrome/browser/policy/configuration_policy_handler_list.cc (right):

https://codereview.chromium.org/13132004/diff/70001/chrome/browser/policy/con...
chrome/browser/policy/configuration_policy_handler_list.cc:520: false));
On 2013/04/17 08:09:09, Mattias Nissler wrote:
> Please move to line 438 where the other extension-related handlers are
> initialized.

Done.

https://codereview.chromium.org/13132004/diff/70001/chrome/browser/policy/con...
File chrome/browser/policy/configuration_policy_pref_store_unittest.cc (right):

https://codereview.chromium.org/13132004/diff/70001/chrome/browser/policy/con...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:125:
prefs::kAttestationExtensionWhitelist)));
On 2013/04/17 08:09:09, Mattias Nissler wrote:
> This probably no longer passes when using the handler. You can just remove
this.

Done.

[davidyu, 2013-04-17 08:29:48]

Hi jochen,

I need your approval on chrome/chrome_browser_extensions.gypi. Thanks!

On 2013/04/17 08:24:19, davidyu wrote:
>
https://codereview.chromium.org/13132004/diff/70001/chrome/app/policy/policy_...
> File chrome/app/policy/policy_templates.json (right):
> 
>
https://codereview.chromium.org/13132004/diff/70001/chrome/app/policy/policy_...
> chrome/app/policy/policy_templates.json:4521: 'desc': '''If true, the user can
> use the hardware on Chrome devices to remote attest its identity to the
privacy
> CA via the Enterprise Platform Keys API
> chrome.enterprise.platformKeysPrivate.challengeUserKey(). If it is set to
false,
> or if it is not set, calls to the API will be rejected.''',
> On 2013/04/17 08:09:09, Mattias Nissler wrote:
> > suggestion: "fail with an error code" instead of "rejected".
> > 
> > Also, please put the last sentence in its own paragraph for consistency with
> the
> > rest of this file.
> 
> Done.
> 
>
https://codereview.chromium.org/13132004/diff/70001/chrome/app/policy/policy_...
> chrome/app/policy/policy_templates.json:4538: 'desc': '''This policy specifies
> the allowed extensions to use Enterprise Platform Keys API
> chrome.enterprise.platformKeysPrivate.challengeUserKey() for remote
attestation.
> Extensions must be added to this list to use the API. If an extension is not
in
> the list, or the list is not set, the call to the API will be rejected.''',
> On 2013/04/17 08:09:09, Mattias Nissler wrote:
> > ditto
> 
> Done.
> 
>
https://codereview.chromium.org/13132004/diff/70001/chrome/browser/extensions...
> File
>
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc
> (right):
> 
>
https://codereview.chromium.org/13132004/diff/70001/chrome/browser/extensions...
>
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc:344:
> return true;  // XXX
> On 2013/04/17 08:09:09, Mattias Nissler wrote:
> > I take it this is not to be committed?
> 
> ah, right. sorry about that. removed.
> 
>
https://codereview.chromium.org/13132004/diff/70001/chrome/browser/extensions...
>
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc:354:
> return true;  // XXX
> On 2013/04/17 08:09:09, Mattias Nissler wrote:
> > ditto
> 
> Done.
> 
>
https://codereview.chromium.org/13132004/diff/70001/chrome/browser/policy/con...
> File chrome/browser/policy/configuration_policy_handler_list.cc (right):
> 
>
https://codereview.chromium.org/13132004/diff/70001/chrome/browser/policy/con...
> chrome/browser/policy/configuration_policy_handler_list.cc:520: false));
> On 2013/04/17 08:09:09, Mattias Nissler wrote:
> > Please move to line 438 where the other extension-related handlers are
> > initialized.
> 
> Done.
> 
>
https://codereview.chromium.org/13132004/diff/70001/chrome/browser/policy/con...
> File chrome/browser/policy/configuration_policy_pref_store_unittest.cc
(right):
> 
>
https://codereview.chromium.org/13132004/diff/70001/chrome/browser/policy/con...
> chrome/browser/policy/configuration_policy_pref_store_unittest.cc:125:
> prefs::kAttestationExtensionWhitelist)));
> On 2013/04/17 08:09:09, Mattias Nissler wrote:
> > This probably no longer passes when using the handler. You can just remove
> this.
> 
> Done.

[jochen (gone - plz use gerrit), 2013-04-17 08:39:41]

*gypi* lgtm

[commit-bot: I haz the power, 2013-04-17 23:59:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidyu@chromium.org/13132004/78001

[commit-bot: I haz the power, 2013-04-18 00:19:09]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[dkrahn, 2013-04-18 01:11:44]

A few late comments which can be addressed in a subsequent CL.  It would also be
nice to see unit tests for enterprise_platform_keys_private_api.cc.

https://chromiumcodereview.appspot.com/13132004/diff/78001/chrome/browser/ext...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc
(right):

https://chromiumcodereview.appspot.com/13132004/diff/78001/chrome/browser/ext...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc:207:
// we use the key.
We only need to get consent if the key needs to be certified by the PCA, right? 
If it already exists we don't want to bug the user again.

https://chromiumcodereview.appspot.com/13132004/diff/78001/chrome/browser/ext...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc:232:
challenge, register_key, domain, true));
Can we just call UserConsentCallback instead of PostTask?

https://chromiumcodereview.appspot.com/13132004/diff/78001/chrome/browser/ext...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc:281:
void EPKPChallengeUserKey::GetPublicKeyCallback(
The public key has now been replaced by a SPKAC embedded in the
challenge-response.  You should be able to remove this step and the public_key
output parameter.

https://chromiumcodereview.appspot.com/13132004/diff/78001/chrome/browser/ext...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.h
(right):

https://chromiumcodereview.appspot.com/13132004/diff/78001/chrome/browser/ext...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.h:36:
static const char kKeyName[];
Should this be defined for each subclass?  See
chromeos/attestation/attestation_flow.h for supported names.

https://chromiumcodereview.appspot.com/13132004/diff/78001/chrome/browser/ext...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.h:55:
cryptohome::AsyncMethodCaller* async_caller_;
I think you'll need an AttestationFlow* too.  If the key being challenged needs
to be generated and certified, you'll have to call
AttestationFlow::GetCertificate().

[davidyu, 2013-04-18 06:08:11]

https://codereview.chromium.org/13132004/diff/78001/chrome/browser/extensions...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc
(right):

https://codereview.chromium.org/13132004/diff/78001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc:232:
challenge, register_key, domain, true));
On 2013/04/18 01:11:44, Darren Krahn wrote:
> Can we just call UserConsentCallback instead of PostTask?

I suppose I can replace all PostTask() with just callback.Run(). What's the
convention here?

https://codereview.chromium.org/13132004/diff/78001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc:281:
void EPKPChallengeUserKey::GetPublicKeyCallback(
On 2013/04/18 01:11:44, Darren Krahn wrote:
> The public key has now been replaced by a SPKAC embedded in the
> challenge-response.  You should be able to remove this step and the public_key
> output parameter.

Done.

https://codereview.chromium.org/13132004/diff/78001/chrome/browser/extensions...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.h
(right):

https://codereview.chromium.org/13132004/diff/78001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.h:36:
static const char kKeyName[];
On 2013/04/18 01:11:44, Darren Krahn wrote:
> Should this be defined for each subclass?  See
> chromeos/attestation/attestation_flow.h for supported names.

Done.

https://codereview.chromium.org/13132004/diff/78001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.h:55:
cryptohome::AsyncMethodCaller* async_caller_;
It looks like I need your pending CL to create the flow object. I'll wait until
your code is in.

On 2013/04/18 01:11:44, Darren Krahn wrote:
> I think you'll need an AttestationFlow* too.  If the key being challenged
needs
> to be generated and certified, you'll have to call
> AttestationFlow::GetCertificate().

[dkrahn, 2013-04-18 17:04:52]

https://codereview.chromium.org/13132004/diff/78001/chrome/browser/extensions...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc
(right):

https://codereview.chromium.org/13132004/diff/78001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.cc:232:
challenge, register_key, domain, true));
On 2013/04/18 06:08:11, davidyu wrote:
> On 2013/04/18 01:11:44, Darren Krahn wrote:
> > Can we just call UserConsentCallback instead of PostTask?
> 
> I suppose I can replace all PostTask() with just callback.Run(). What's the
> convention here?

I don't know what the convention is but if I want to stay on the same thread and
want to call the callback now, then I just Run() it.  If you need to post to a
particular thread or delay the task then PostTask() is the right thing.

https://codereview.chromium.org/13132004/diff/78001/chrome/browser/extensions...
File
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.h
(right):

https://codereview.chromium.org/13132004/diff/78001/chrome/browser/extensions...
chrome/browser/extensions/api/enterprise_platform_keys_private/enterprise_platform_keys_private_api.h:55:
cryptohome::AsyncMethodCaller* async_caller_;
On 2013/04/18 06:08:11, davidyu wrote:
> It looks like I need your pending CL to create the flow object. I'll wait
until
> your code is in.
> 
> On 2013/04/18 01:11:44, Darren Krahn wrote:
> > I think you'll need an AttestationFlow* too.  If the key being challenged
> needs
> > to be generated and certified, you'll have to call
> > AttestationFlow::GetCertificate().
> 
Yes, you will.  I don't mind if you submit this CL and have another one for
AttestationFlow.  This CL is already pretty large.

[commit-bot: I haz the power, 2013-04-19 01:48:45]

Change committed as 195043