Extension API enterprise.platformKeys.

Extension API enterprise.platformKeys.

First version that implements all proposed functions backed by the TPM; enabled on dev-channel only.

This is still missing several features:
- Reusing algorithm normalization of WebCrypto.
- Allow calling Sign() at most once per key.
- Storing which extension imported a certificate.
- Device-wide token.
- Passing publicExponent to generateKey.
- Throwing DOMExceptions instead of Errors.
- ...

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

[pneubeck (no reviews), 2014-04-10 15:18:37]

FYI
This is the CL where I'll regularly upload my current implementation. So all
WIP.
Just in case, if you want to prevent early going astray :-)

[eroman, 2014-04-10 18:45:46]

I only looked at the file which interfaces with WebCrypto, gave some initial
comments.

https://codereview.chromium.org/214863002/diff/100001/chrome/renderer/extensi...
File chrome/renderer/extensions/enterprise_certificates_natives.cc (right):

https://codereview.chromium.org/214863002/diff/100001/chrome/renderer/extensi...
chrome/renderer/extensions/enterprise_certificates_natives.cc:22:
base::Unretained(this)));
I haven't read through the rest of this, change, but always very cautions when I
see base::Unretained(). Please verify that class remains alive at least as long
as the callback (i.e. that cancellation/shutdown paths are reasonable).

https://codereview.chromium.org/214863002/diff/100001/chrome/renderer/extensi...
chrome/renderer/extensions/enterprise_certificates_natives.cc:28:
blink::WebScriptBindings::toWebCryptoKey(call_info[0].As<v8::Object>());
This function doesn't exist, so presumably depends on a dependent blink
changelist?

Can this return a WebCryptoKey rather than pointer?

https://codereview.chromium.org/214863002/diff/100001/chrome/renderer/extensi...
chrome/renderer/extensions/enterprise_certificates_natives.cc:29: if (!key)
IMPORTANT: You should also check the key type. Since this function expects
private keys, make sure that:

key->type() == blink::WebCryptoKeyTypePrivate

https://codereview.chromium.org/214863002/diff/100001/chrome/renderer/extensi...
chrome/renderer/extensions/enterprise_certificates_natives.cc:32:
blink::WebVector<unsigned char> keyData;
style: keyData --> key_data

https://codereview.chromium.org/214863002/diff/100001/chrome/renderer/extensi...
chrome/renderer/extensions/enterprise_certificates_natives.cc:33:
blink::Platform::current()->crypto()->serializeKeyForClone(*key, keyData);
IMPORTANT: serializeKeyForClone() returns a boolean for success, which should be
checked here.

It isn't likely to be an issue for private keys since on failure keyData will
likely be empty (although that is not guaranteed by the API). But for other key
types this would be ambiguous -- a symmetric key with empty data is a valid key
as well.

https://codereview.chromium.org/214863002/diff/100001/chrome/renderer/extensi...
chrome/renderer/extensions/enterprise_certificates_natives.cc:36:
v8::Array::New(call_info.GetIsolate(), keyData.size());
IMPORTANT: Please explain this. Why is the key being being copied into a
javascript object?

The key data should not be exposed to javascript as it may have been
unextractable.

My understanding is it was going to be serialized here as a means to pass it
over to the browser process.

I haven't read the rest of the change in detail, but making sure the key data is
not exposed to javascript APIs is very important.

https://codereview.chromium.org/214863002/diff/100001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/enterprise_certificates/import.js
(right):

https://codereview.chromium.org/214863002/diff/100001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/enterprise_certificates/import.js:104: var
algorithmKeyGen = {name: "RSASSA-PKCS1-v1_5", hash: {name: "SHA-256"}};
nit: "algorithmKeyGen" --> "importAlgorithm" ?

https://codereview.chromium.org/214863002/diff/100001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/enterprise_certificates/import.js:111:
.importKey('pkcs8', privateKeyDer, algorithmKeyGen, false, ["sign"])
The tests should also exercise calling importClientCertificate() with other key
types.
Namely: public key, and symmetric key.

Since there is not currently any type checking on the key, it is possible to
construct a symmetric key that happens to be a valid PrivateKeyInfo, and that
will be imported by the extension API. This seems like a bug to me.

[pneubeck (no reviews), 2014-04-11 14:13:59]

Thanks Eric for the early feedback.
The most important change is that the private key is now never exposed to JS.

I'll ping you once I made further progress.

https://codereview.chromium.org/214863002/diff/100001/chrome/renderer/extensi...
File chrome/renderer/extensions/enterprise_certificates_natives.cc (right):

https://codereview.chromium.org/214863002/diff/100001/chrome/renderer/extensi...
chrome/renderer/extensions/enterprise_certificates_natives.cc:22:
base::Unretained(this)));
On 2014/04/10 18:45:46, eroman wrote:
> I haven't read through the rest of this, change, but always very cautions when
I
> see base::Unretained(). Please verify that class remains alive at least as
long
> as the callback (i.e. that cancellation/shutdown paths are reasonable).
This seems to be the way how ObjectBackedNativeHandlers work.

https://codereview.chromium.org/214863002/diff/100001/chrome/renderer/extensi...
chrome/renderer/extensions/enterprise_certificates_natives.cc:28:
blink::WebScriptBindings::toWebCryptoKey(call_info[0].As<v8::Object>());
On 2014/04/10 18:45:46, eroman wrote:
> This function doesn't exist, so presumably depends on a dependent blink
> changelist?

Added a link to the blink change to the commit message.

> 
> Can this return a WebCryptoKey rather than pointer?
Yes, I didn't see that WebCryptoKey itself is a refptr. Done.

https://codereview.chromium.org/214863002/diff/100001/chrome/renderer/extensi...
chrome/renderer/extensions/enterprise_certificates_natives.cc:29: if (!key)
On 2014/04/10 18:45:46, eroman wrote:
> IMPORTANT: You should also check the key type. Since this function expects
> private keys, make sure that:
> 
> key->type() == blink::WebCryptoKeyTypePrivate

Done.

https://codereview.chromium.org/214863002/diff/100001/chrome/renderer/extensi...
chrome/renderer/extensions/enterprise_certificates_natives.cc:32:
blink::WebVector<unsigned char> keyData;
On 2014/04/10 18:45:46, eroman wrote:
> style: keyData --> key_data

Done.

https://codereview.chromium.org/214863002/diff/100001/chrome/renderer/extensi...
chrome/renderer/extensions/enterprise_certificates_natives.cc:33:
blink::Platform::current()->crypto()->serializeKeyForClone(*key, keyData);
On 2014/04/10 18:45:46, eroman wrote:
> IMPORTANT: serializeKeyForClone() returns a boolean for success, which should
be
> checked here.
> 
> It isn't likely to be an issue for private keys since on failure keyData will
> likely be empty (although that is not guaranteed by the API). But for other
key
> types this would be ambiguous -- a symmetric key with empty data is a valid
key
> as well.

Done.

https://codereview.chromium.org/214863002/diff/100001/chrome/renderer/extensi...
chrome/renderer/extensions/enterprise_certificates_natives.cc:36:
v8::Array::New(call_info.GetIsolate(), keyData.size());
On 2014/04/10 18:45:46, eroman wrote:
> IMPORTANT: Please explain this. Why is the key being being copied into a
> javascript object?
> 
> The key data should not be exposed to javascript as it may have been
> unextractable.
> 
> My understanding is it was going to be serialized here as a means to pass it
> over to the browser process.
> 
> I haven't read the rest of the change in detail, but making sure the key data
is
> not exposed to javascript APIs is very important.

kalman@ mentioned that the javascript side of the custom binding is in principal
as safe as C++ except that one must take more care during the implementation.

To be on the safe side, I changed this to C++ only nonetheless.

https://codereview.chromium.org/214863002/diff/100001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/enterprise_certificates/import.js
(right):

https://codereview.chromium.org/214863002/diff/100001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/enterprise_certificates/import.js:104: var
algorithmKeyGen = {name: "RSASSA-PKCS1-v1_5", hash: {name: "SHA-256"}};
On 2014/04/10 18:45:46, eroman wrote:
> nit: "algorithmKeyGen" --> "importAlgorithm" ?

Done.

https://codereview.chromium.org/214863002/diff/100001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/enterprise_certificates/import.js:111:
.importKey('pkcs8', privateKeyDer, algorithmKeyGen, false, ["sign"])
On 2014/04/10 18:45:46, eroman wrote:
> The tests should also exercise calling importClientCertificate() with other
key
> types.
> Namely: public key, and symmetric key.
> 
> Since there is not currently any type checking on the key, it is possible to
> construct a symmetric key that happens to be a valid PrivateKeyInfo, and that
> will be imported by the extension API. This seems like a bug to me.

Done.

[pneubeck (no reviews), 2014-04-14 17:43:21]

@Benjamin, please check whether the structuring into custom_bindings /
...natives.cc / internal API / external API is idiomatic. (The implementation is
still far from complete)
Thanks!

[Ryan Sleevi, 2014-04-15 00:15:03]

https://codereview.chromium.org/214863002/diff/140001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_certificates/enterprise_certificates_internal_api.cc
(right):

https://codereview.chromium.org/214863002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_certificates/enterprise_certificates_internal_api.cc:26:
GetProfile(), base::Bind(&ECIImport::DidGetCertDB, this));
This file should either be suffixed with _nss or in a chromeos folder.

With the migration to OpenSSL, I'd much prefer a _nss suffix, since eventually
we'll want to have an _openssl version as we build out the features (and which
will replace/delete the _nss version)

https://codereview.chromium.org/214863002/diff/140001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_certificates/enterprise_certificates_internal_api.h
(right):

https://codereview.chromium.org/214863002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_certificates/enterprise_certificates_internal_api.h:22:
class ECIImport : public ChromeAsyncExtensionFunction {
Naming is hard, but abbreviations of uncommon bits are harder on everyone.

https://codereview.chromium.org/214863002/diff/140001/chrome/renderer/extensi...
File chrome/renderer/extensions/enterprise_certificates_natives.cc (right):

https://codereview.chromium.org/214863002/diff/140001/chrome/renderer/extensi...
chrome/renderer/extensions/enterprise_certificates_natives.cc:60: if
(!blink::Platform::current()->crypto()->serializeKeyForClone(key,
You'll want to add comments about why this is OK ;)

https://codereview.chromium.org/214863002/diff/140001/chrome/renderer/extensi...
chrome/renderer/extensions/enterprise_certificates_natives.cc:72:
(char*)key_data.data(), key_data.size()));
C++ casts

[pneubeck (no reviews), 2014-05-02 17:44:01]

-eroman

Ptal. I changed to the new proposal that incorporates a WebCrypto::SubtleCrypto
member.

I'm not sure yet, how to reference the SubtleCrypto type in the api's json
schema, so that is currently incomplete.

https://codereview.chromium.org/214863002/diff/140001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_certificates/enterprise_certificates_internal_api.cc
(right):

https://codereview.chromium.org/214863002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_certificates/enterprise_certificates_internal_api.cc:26:
GetProfile(), base::Bind(&ECIImport::DidGetCertDB, this));
On 2014/04/15 00:15:04, Ryan Sleevi wrote:
> This file should either be suffixed with _nss or in a chromeos folder.
> 
> With the migration to OpenSSL, I'd much prefer a _nss suffix, since eventually
> we'll want to have an _openssl version as we build out the features (and which
> will replace/delete the _nss version)

Done.

[not at google - send to devlin, 2014-05-02 18:31:55]

looked at json, idl, js files.

https://codereview.chromium.org/214863002/diff/330001/chrome/common/extension...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/214863002/diff/330001/chrome/common/extension...
chrome/common/extensions/api/_api_features.json:280: "channel": "dev",
this rule should be in the permission file. restrict the *permission* from being
granted in all but dev channel, not just the API.

same should go for the "platforms": ["chromeos"] part, unfortuately there is a
precedent to be putting those in the API schema files. should fix that..

https://codereview.chromium.org/214863002/diff/330001/chrome/common/extension...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/214863002/diff/330001/chrome/common/extension...
chrome/common/extensions/api/_permission_features.json:340: "extension_types":
["extension", "legacy_packaged_app"]
restrict to policy?

https://codereview.chromium.org/214863002/diff/330001/chrome/common/extension...
File chrome/common/extensions/api/enterprise_platform_keys.json (right):

https://codereview.chromium.org/214863002/diff/330001/chrome/common/extension...
chrome/common/extensions/api/enterprise_platform_keys.json:5: [{
why is this JSON while the other is IDL?

https://codereview.chromium.org/214863002/diff/330001/chrome/common/extension...
chrome/common/extensions/api/enterprise_platform_keys.json:23: "id" :
"SubtleCrypto",
SubtleCrypto is something that's supposed to be defined in a standard somewhere?
if you're going to mimic the interface then you might want to "nodoc" this and
point to the spec.

might :) up to yuo.

https://codereview.chromium.org/214863002/diff/330001/chrome/common/extension...
File chrome/common/extensions/api/enterprise_platform_keys_internal.idl (right):

https://codereview.chromium.org/214863002/diff/330001/chrome/common/extension...
chrome/common/extensions/api/enterprise_platform_keys_internal.idl:19:
SignCallback callback);
functions need comments

https://codereview.chromium.org/214863002/diff/330001/chrome/renderer/resourc...
File
chrome/renderer/resources/extensions/enterprise_platform_keys_custom_bindings.js
(right):

https://codereview.chromium.org/214863002/diff/330001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys_custom_bindings.js:6:

could you explain what is going on here? there is a lot of code.

have a look at how the Automation is implemented and documented as an example of
a complex custom bindings system:
-
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re...
-
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re...
-
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re...
-
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re...

i would find more structure, comments, type annotations, much easier to follow.

https://codereview.chromium.org/214863002/diff/330001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys_custom_bindings.js:23:
privates(this).type_ = type;
a nice idiom for privates(..) is to have a "KeyImpl" class (or even just a "Key"
class) and then use utils.expose to cherry-pick just the functions/properties
you want to expose to extensions.

https://codereview.chromium.org/214863002/diff/330001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys_custom_bindings.js:106:
catch (error) {
are exceptions not automatically propagated to failure handlers in promises?

[pneubeck (no reviews), 2014-05-05 20:09:34]

https://codereview.chromium.org/214863002/diff/330001/chrome/common/extension...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/214863002/diff/330001/chrome/common/extension...
chrome/common/extensions/api/_api_features.json:280: "channel": "dev",
On 2014/05/02 18:31:56, kalman wrote:
> this rule should be in the permission file. restrict the *permission* from
being
> granted in all but dev channel, not just the API.
> 
> same should go for the "platforms": ["chromeos"] part, unfortuately there is a
> precedent to be putting those in the API schema files. should fix that..

Done.

https://codereview.chromium.org/214863002/diff/330001/chrome/common/extension...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/214863002/diff/330001/chrome/common/extension...
chrome/common/extensions/api/_permission_features.json:340: "extension_types":
["extension", "legacy_packaged_app"]
On 2014/05/02 18:31:56, kalman wrote:
> restrict to policy?

How do I allow the test extension (of the *_apitest) using this permission?

https://codereview.chromium.org/214863002/diff/330001/chrome/common/extension...
File chrome/common/extensions/api/enterprise_platform_keys.json (right):

https://codereview.chromium.org/214863002/diff/330001/chrome/common/extension...
chrome/common/extensions/api/enterprise_platform_keys.json:5: [{
On 2014/05/02 18:31:56, kalman wrote:
> why is this JSON while the other is IDL?

I preferred IDL over JSON. However, I feared that our IDL parser doesn't provide
some features that I might need...

Happy to change this to IDL as well, if JSON isn't necessary.

https://codereview.chromium.org/214863002/diff/330001/chrome/common/extension...
chrome/common/extensions/api/enterprise_platform_keys.json:23: "id" :
"SubtleCrypto",
On 2014/05/02 18:31:56, kalman wrote:
> SubtleCrypto is something that's supposed to be defined in a standard
somewhere?
> if you're going to mimic the interface then you might want to "nodoc" this and
> point to the spec.
> 
> might :) up to yuo.

Is there a canonical way to declare this as a reference to an external type
(WebCrypto's in this case)?

Changed to nodoc for now. I might still declare the methods that I actually
implement and describe the additional restrictions that apply.

https://codereview.chromium.org/214863002/diff/330001/chrome/renderer/resourc...
File
chrome/renderer/resources/extensions/enterprise_platform_keys_custom_bindings.js
(right):

https://codereview.chromium.org/214863002/diff/330001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys_custom_bindings.js:6:

On 2014/05/02 18:31:56, kalman wrote:
> could you explain what is going on here? there is a lot of code.
> 
> have a look at how the Automation is implemented and documented as an example
of
> a complex custom bindings system:
> -
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re...
> -
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re...
> -
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re...
> -
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re...
> 
> i would find more structure, comments, type annotations, much easier to
follow.

Done.

https://codereview.chromium.org/214863002/diff/330001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys_custom_bindings.js:23:
privates(this).type_ = type;
On 2014/05/02 18:31:56, kalman wrote:
> a nice idiom for privates(..) is to have a "KeyImpl" class (or even just a
"Key"
> class) and then use utils.expose to cherry-pick just the functions/properties
> you want to expose to extensions.

Done.

https://codereview.chromium.org/214863002/diff/330001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys_custom_bindings.js:106:
catch (error) {
On 2014/05/02 18:31:56, kalman wrote:
> are exceptions not automatically propagated to failure handlers in promises?

Indeed they are.

[not at google - send to devlin, 2014-05-05 21:09:23]

https://codereview.chromium.org/214863002/diff/330001/chrome/common/extension...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/214863002/diff/330001/chrome/common/extension...
chrome/common/extensions/api/_permission_features.json:340: "extension_types":
["extension", "legacy_packaged_app"]
On 2014/05/05 20:09:35, pneubeck wrote:
> On 2014/05/02 18:31:56, kalman wrote:
> > restrict to policy?
> 
> How do I allow the test extension (of the *_apitest) using this permission?

You might need to add another method to extension_browsertest to load a policy
extension. We have that sort of thing for component extensions, see that code
(LoadExtensionAsComponent) hopefully it's fairly straighforward.

if it becomes tricky (i.e. factoring policy loading code a bit differently to
allow it) then you can do in a follow-up, but in that case please limit this to
'trunk' not 'dev'.

https://codereview.chromium.org/214863002/diff/330001/chrome/common/extension...
File chrome/common/extensions/api/enterprise_platform_keys.json (right):

https://codereview.chromium.org/214863002/diff/330001/chrome/common/extension...
chrome/common/extensions/api/enterprise_platform_keys.json:23: "id" :
"SubtleCrypto",
On 2014/05/05 20:09:35, pneubeck wrote:
> On 2014/05/02 18:31:56, kalman wrote:
> > SubtleCrypto is something that's supposed to be defined in a standard
> somewhere?
> > if you're going to mimic the interface then you might want to "nodoc" this
and
> > point to the spec.
> > 
> > might :) up to yuo.
> 
> Is there a canonical way to declare this as a reference to an external type
> (WebCrypto's in this case)?
> 
> Changed to nodoc for now. I might still declare the methods that I actually
> implement and describe the additional restrictions that apply.

isInstanceOf should do it, which in IDL is instanceOf:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

there isn't anything in here which looks unsupported by IDL.

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc
(right):

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc:130:
return true;
overriding ExtensionFunction::Run rather than RunImpl will make code like this
especially easy to understand.

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h
(right):

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h:13:
namespace enterprise_platform_keys {
this inner namespace is unusual. see comment below, typically you will namespace
via long function names, in order for the generated code to pick this up.

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h:15:
class TokenMethodExtensionFunction : public ChromeAsyncExtensionFunction {
I just submitted r268033 which encourages you to implement
ChromeUIThreadExtensionFunction instead of ChromeAsyncExtensionFunction. You
override Run() not RunImpl() and have understandable return values. It would be
nice to start using it here, but given you've already done the work, not a big
deal if you don't.

it's well commented at ExtensionFunction::Run.

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h:30:
class Generate : public TokenMethodExtensionFunction {
registration for these functions is generated by the schema compiler. you should
call them things like EnterprisePlatformKeysInternalGenerateKeyFunction (the
auto-generated name, I think).

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest.cc
(right):

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest.cc:98:
content::RunAllPendingInMessageLoop();
this implementation detail probably belongs in PrepareCertDatabase.

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest.cc:100:
ExtensionApiTest::SetUpOnMainThread();
usually calling the superclass' SetUp would happen first, not last.

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
File chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.h
(right):

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.h:25:
namespace enterprise_platform_keys {
same comment re namespace, and why does this functionality (which depends on
TokenMethodExtensionFunction) in a separate file?

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
File
chrome/renderer/resources/extensions/enterprise_platform_keys/enterprise_platform_keys_key.js
(right):

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/enterprise_platform_keys_key.js:6:
var intersect = require('enterprise.platformKeys.utils').intersect;
given these files are in the enterprise_platform_keys directory, calling them
'key.js', 'subtle_crypto.js', etc seems cleaner.

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/enterprise_platform_keys_key.js:9:
* Enum of possible key types (subset of WebCrypto.KeyType).
why is it a subset? will your code still work if you just use KeyType (or does
that require experimental blink turned on?)

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/enterprise_platform_keys_key.js:74:
exports.Key = Key;
ideally (from an OO perspective) you would only expose Key, because this file is
called "key".

I can see that's tricky when using privates() because I presume you want the
SubtleCrypto interface to be able to interact with it in a special way (see
comment in there)?

I'm wondering whether that should be the job of SubtleCrypto then. Make this
class implement everything SubtleCrypto needs, then expose those "safe" versions
of this object by calling utils.expose from subtle_crypto.js?

(depends on how this shakes out)

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/enterprise_platform_keys_key.js:76:
exports.KeyImpl = KeyImpl;
exposing the Impl is unusual

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
File
chrome/renderer/resources/extensions/enterprise_platform_keys/enterprise_platform_keys_subtle_crypto.js
(right):

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/enterprise_platform_keys_subtle_crypto.js:33:
}
all of this validation could happen before returning the Promise. it's probably
better to have these invalid-input checks up-front rather than on resolution of
the promise?

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/enterprise_platform_keys_subtle_crypto.js:35:
throw new Error('The algorithm is not supported');
maybe include the name of the algorithm in this failure?

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/enterprise_platform_keys_subtle_crypto.js:50:
var keyImpl = privates(key).impl;
same comments as above

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/enterprise_platform_keys_subtle_crypto.js:51:
if (!(keyImpl instanceof KeyImpl))
instanceof is dangerous in bindings files because extensions/apps can have
multiple contexts, meaning different copies of each API binding, and instanceof
doesn't work across bindings. it means you might have problems using keys from
the background page in an app window or popup etc.

besides which, exposing the Impl to this class is unusual. better for
encapsulation to have this logic be defined where the class is. e.g. can the
sign() functionality live in the key.js file somehow?

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/enterprise_platform_keys_subtle_crypto.js:71:
SubtleCryptoImpl.prototype.exportKey = function(format, key) {
same comments here

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
File
chrome/renderer/resources/extensions/enterprise_platform_keys_internal_custom_bindings.js
(right):

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys_internal_custom_bindings.js:7:
.generate();
this simple behaviour is generated for you. you only need the separate
custom_bindings file if you do the registerCustomHook stuff.

https://codereview.chromium.org/214863002/diff/410001/extensions/renderer/dis...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/214863002/diff/410001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:1068:
IDR_ENTERPRISE_PLATFORM_KEYS_INTERNAL_CUSTOM_BINDINGS_JS);
this is a chrome feature, so should go in
chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc

[pneubeck (no reviews), 2014-05-05 21:28:22]

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h
(right):

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h:13:
namespace enterprise_platform_keys {
On 2014/05/05 21:09:24, kalman wrote:
> this inner namespace is unusual. see comment below, typically you will
namespace
> via long function names, in order for the generated code to pick this up.

See typedefs at the end. TheNamesAreReallyExtreeeemelyLoooooooong....

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
File chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.h
(right):

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.h:25:
namespace enterprise_platform_keys {
On 2014/05/05 21:09:24, kalman wrote:
> same comment re namespace, and why does this functionality (which depends on
> TokenMethodExtensionFunction) in a separate file?

It's the NSS specific implementation (vs. OpenSSL). As suggested by Ryan, this
shall be easily switchable.

I tried to prepare this for the second implementation in a way that depends on
as few precompiler directives as possible. The way I did it, the _nss.cc/h files
have to be conditionally compiled and the namespace alias has to be switched
from 'nss' to 'openssl'.

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
File
chrome/renderer/resources/extensions/enterprise_platform_keys/enterprise_platform_keys_key.js
(right):

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/enterprise_platform_keys_key.js:9:
* Enum of possible key types (subset of WebCrypto.KeyType).
On 2014/05/05 21:09:24, kalman wrote:
> why is it a subset? 
WebCrypto has the additional KeyType 'secret' for symmetric keys. That one has
just no meaning in the context of platformKeys.

> will your code still work if you just use KeyType (or does
> that require experimental blink turned on?)
What do you mean with 'work'? Do you imply that the closure type-annotations are
actually checked? I never got an error because of the annotations. Maybe I have
to enable that 'strict' mode?

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
File
chrome/renderer/resources/extensions/enterprise_platform_keys/enterprise_platform_keys_subtle_crypto.js
(right):

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/enterprise_platform_keys_subtle_crypto.js:33:
}
On 2014/05/05 21:09:24, kalman wrote:
> all of this validation could happen before returning the Promise. it's
probably
> better to have these invalid-input checks up-front rather than on resolution
of
> the promise?

As far as I understand the Promise thing, reject should be called on any
error/exception.

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/enterprise_platform_keys_subtle_crypto.js:51:
if (!(keyImpl instanceof KeyImpl))
The only reason why I added this was, that I wanted to ensure that no one is
spoofing this implementation with a fake Key object.
I guess, I don't need the instanceOf check, if I already access
privates(key).impl, which shouldn't be fakeable.

On 2014/05/05 21:09:24, kalman wrote:
> instanceof is dangerous in bindings files because extensions/apps can have
> multiple contexts, meaning different copies of each API binding, and
instanceof
> doesn't work across bindings. it means you might have problems using keys from
> the background page in an app window or popup etc.
> 
> besides which, exposing the Impl to this class is unusual. better for
> encapsulation to have this logic be defined where the class is. e.g. can the
> sign() functionality live in the key.js file somehow?

[not at google - send to devlin, 2014-05-05 21:44:49]

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h
(right):

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h:13:
namespace enterprise_platform_keys {
On 2014/05/05 21:28:23, pneubeck wrote:
> On 2014/05/05 21:09:24, kalman wrote:
> > this inner namespace is unusual. see comment below, typically you will
> namespace
> > via long function names, in order for the generated code to pick this up.
> 
> See typedefs at the end. TheNamesAreReallyExtreeeemelyLoooooooong....

I see. Yeah, I dunno, I was confused, so chances are other people will be too.
The idiomatic thing is to not worry about this namespace + typedef thing, it's
quite unusual. "git cl format" handles long lines just fine.

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
File
chrome/renderer/resources/extensions/enterprise_platform_keys/enterprise_platform_keys_key.js
(right):

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/enterprise_platform_keys_key.js:9:
* Enum of possible key types (subset of WebCrypto.KeyType).
On 2014/05/05 21:28:23, pneubeck wrote:
> On 2014/05/05 21:09:24, kalman wrote:
> > why is it a subset? 
> WebCrypto has the additional KeyType 'secret' for symmetric keys. That one has
> just no meaning in the context of platformKeys.
> 
> > will your code still work if you just use KeyType (or does
> > that require experimental blink turned on?)
> What do you mean with 'work'? Do you imply that the closure type-annotations
are
> actually checked? I never got an error because of the annotations. Maybe I
have
> to enable that 'strict' mode?
> 
> 

oh, no, not what I meant. I meant that maybe these enums already exist somewhere
on the web platform and you can re-use those.

[pneubeck (no reviews), 2014-05-06 14:07:20]

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc
(right):

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc:130:
return true;
On 2014/05/05 21:09:24, kalman wrote:
> overriding ExtensionFunction::Run rather than RunImpl will make code like this
> especially easy to understand.

Done.

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h
(right):

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h:13:
namespace enterprise_platform_keys {
On 2014/05/05 21:44:49, kalman wrote:
> On 2014/05/05 21:28:23, pneubeck wrote:
> > On 2014/05/05 21:09:24, kalman wrote:
> > > this inner namespace is unusual. see comment below, typically you will
> > namespace
> > > via long function names, in order for the generated code to pick this up.
> > 
> > See typedefs at the end. TheNamesAreReallyExtreeeemelyLoooooooong....
> 
> I see. Yeah, I dunno, I was confused, so chances are other people will be too.
> The idiomatic thing is to not worry about this namespace + typedef thing, it's
> quite unusual. "git cl format" handles long lines just fine.

Long names are problematic because you have too read them and they're hiding the
important information.
Thus, I still disagree with 'namespacing by prefixes'. Luckily we're in C++ with
that nice feature called namespaces... ;-)

I added a comment at the top to hopefully prevent any future confusion.

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h:15:
class TokenMethodExtensionFunction : public ChromeAsyncExtensionFunction {
On 2014/05/05 21:09:24, kalman wrote:
> I just submitted r268033 which encourages you to implement
> ChromeUIThreadExtensionFunction instead of ChromeAsyncExtensionFunction. You
> override Run() not RunImpl() and have understandable return values. It would
be
> nice to start using it here, but given you've already done the work, not a big
> deal if you don't.
> 
> it's well commented at ExtensionFunction::Run.

Done.

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h:30:
class Generate : public TokenMethodExtensionFunction {
On 2014/05/05 21:09:24, kalman wrote:
> registration for these functions is generated by the schema compiler. you
should
> call them things like EnterprisePlatformKeysInternalGenerateKeyFunction (the
> auto-generated name, I think).

Done.

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest.cc
(right):

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest.cc:98:
content::RunAllPendingInMessageLoop();
On 2014/05/05 21:09:24, kalman wrote:
> this implementation detail probably belongs in PrepareCertDatabase.

Done.

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest.cc:100:
ExtensionApiTest::SetUpOnMainThread();
On 2014/05/05 21:09:24, kalman wrote:
> usually calling the superclass' SetUp would happen first, not last.

Done.

https://codereview.chromium.org/214863002/diff/410001/chrome/common/extension...
File chrome/common/extensions/api/enterprise_platform_keys_internal.idl (right):

https://codereview.chromium.org/214863002/diff/410001/chrome/common/extension...
chrome/common/extensions/api/enterprise_platform_keys_internal.idl:41:
ArrayBuffer publicKey,
@Benjamin,

all arguments which are currently ArrayBuffer should have type (ArrayBuffer or
ArrayBufferView).
This at first has the problem that ArrayBufferView does't have an exposed
interface object, so cannot be checked from JS.

Say, I'm using then instead (ArrayBuffer or Uint8Array).
This still has the second problem:
It's translated into
{ "name": "publicKey",
  "choices" [
  { "type": "binary",
    "isInstanceOf": "ArrayBuffer" },
  { "type": "binary",
    "isInstanceOf": "Uint8Array"}
  ]
}
(if adding support for Uint8Array).

The cc_generator.py / h_generator.py don't care about the 'isInstanceOf' and
will create a duplicate:

struct PublicKey {
 ....
 // Choices:
 scoped_ptr<std::string> as_binary;
 scoped_ptr<std::string> as_binary;
}

One way to solve this without breaking existing code is reducing redundant
Choices before passing the api schema to the h_/cc_generator.

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
File
chrome/renderer/resources/extensions/enterprise_platform_keys/enterprise_platform_keys_key.js
(right):

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/enterprise_platform_keys_key.js:9:
* Enum of possible key types (subset of WebCrypto.KeyType).
On 2014/05/05 21:44:49, kalman wrote:
> On 2014/05/05 21:28:23, pneubeck wrote:
> > On 2014/05/05 21:09:24, kalman wrote:
> > > why is it a subset? 
> > WebCrypto has the additional KeyType 'secret' for symmetric keys. That one
has
> > just no meaning in the context of platformKeys.
> > 
> > > will your code still work if you just use KeyType (or does
> > > that require experimental blink turned on?)
> > What do you mean with 'work'? Do you imply that the closure type-annotations
> are
> > actually checked? I never got an error because of the annotations. Maybe I
> have
> > to enable that 'strict' mode?
> > 
> > 
> 
> oh, no, not what I meant. I meant that maybe these enums already exist
somewhere
> on the web platform and you can re-use those.

WebCrypto defines only:
typedef DOMString KeyType;
typedef DOMString KeyUsage;

and in prosa that only certain strings are recognized.

The enums here are not that useful if no compiler is checking the identifiers...
If you think it's better, I can remove them.

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/enterprise_platform_keys_key.js:76:
exports.KeyImpl = KeyImpl;
On 2014/05/05 21:09:24, kalman wrote:
> exposing the Impl is unusual

Done.

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
File
chrome/renderer/resources/extensions/enterprise_platform_keys/enterprise_platform_keys_subtle_crypto.js
(right):

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/enterprise_platform_keys_subtle_crypto.js:35:
throw new Error('The algorithm is not supported');
On 2014/05/05 21:09:24, kalman wrote:
> maybe include the name of the algorithm in this failure?

The errors are specified by SubtleCrypto (using DOMException).
However, I don't know yet how to create DOMExceptions from JS.
I think that will need a native module, so I'd postpone that for now.

http://www.w3.org/TR/WebCryptoAPI/#SubtleCrypto-Exceptions

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/enterprise_platform_keys_subtle_crypto.js:50:
var keyImpl = privates(key).impl;
On 2014/05/05 21:09:24, kalman wrote:
> same comments as above

Done.

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/enterprise_platform_keys_subtle_crypto.js:51:
if (!(keyImpl instanceof KeyImpl))
On 2014/05/05 21:09:24, kalman wrote:
> besides which, exposing the Impl to this class is unusual. better for
> encapsulation to have this logic be defined where the class is. e.g. can the
> sign() functionality live in the key.js file somehow?

I removed the dependence on the class KeyImpl from this file.

I don't think that the crypto operations belong to the Key implementation. The
Key's public key information is just one of the arguments, the Token for example
being another.
The way it's currently, Key is only carrying some (private) data.
I made this now more explicit by exposing an internal getSpki function.

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/enterprise_platform_keys_subtle_crypto.js:71:
SubtleCryptoImpl.prototype.exportKey = function(format, key) {
On 2014/05/05 21:09:24, kalman wrote:
> same comments here

Done.

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
File
chrome/renderer/resources/extensions/enterprise_platform_keys_internal_custom_bindings.js
(right):

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys_internal_custom_bindings.js:7:
.generate();
On 2014/05/05 21:09:24, kalman wrote:
> this simple behaviour is generated for you. you only need the separate
> custom_bindings file if you do the registerCustomHook stuff.

Hm, would be nice, but if I only remove the according
source_map_.RegisterSource, I get the error
  (BLESSED_EXTENSION context for kmdkjjdaihhnmbakbnbmljnmfcgelkcg) No source for
require(enterprise.platformKeysInternal)

Which I think is expected because the internal API is marked
  "internal": true
in _api_features.json

https://codereview.chromium.org/214863002/diff/410001/extensions/renderer/dis...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/214863002/diff/410001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:1068:
IDR_ENTERPRISE_PLATFORM_KEYS_INTERNAL_CUSTOM_BINDINGS_JS);
On 2014/05/05 21:09:24, kalman wrote:
> this is a chrome feature, so should go in
> chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc

Done.

[not at google - send to devlin, 2014-05-07 00:00:11]

comments are drying up :)

you might want somebody familiar with the crypto stuff to have a look as well.

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h
(right):

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h:13:
namespace enterprise_platform_keys {
On 2014/05/06 14:07:21, pneubeck wrote:
> On 2014/05/05 21:44:49, kalman wrote:
> > On 2014/05/05 21:28:23, pneubeck wrote:
> > > On 2014/05/05 21:09:24, kalman wrote:
> > > > this inner namespace is unusual. see comment below, typically you will
> > > namespace
> > > > via long function names, in order for the generated code to pick this
up.
> > > 
> > > See typedefs at the end. TheNamesAreReallyExtreeeemelyLoooooooong....
> > 
> > I see. Yeah, I dunno, I was confused, so chances are other people will be
too.
> > The idiomatic thing is to not worry about this namespace + typedef thing,
it's
> > quite unusual. "git cl format" handles long lines just fine.
> 
> Long names are problematic because you have too read them and they're hiding
the
> important information.
> Thus, I still disagree with 'namespacing by prefixes'. Luckily we're in C++
with
> that nice feature called namespaces... ;-)
> 
> I added a comment at the top to hopefully prevent any future confusion.

Ok if you insist. This is just very unusual. No other function does it and it
looks weird to me.

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
File chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.h
(right):

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.h:25:
namespace enterprise_platform_keys {
On 2014/05/05 21:28:23, pneubeck wrote:
> On 2014/05/05 21:09:24, kalman wrote:
> > same comment re namespace, and why does this functionality (which depends on
> > TokenMethodExtensionFunction) in a separate file?
> 
> It's the NSS specific implementation (vs. OpenSSL). As suggested by Ryan, this
> shall be easily switchable.
> 
> I tried to prepare this for the second implementation in a way that depends on
> as few precompiler directives as possible. The way I did it, the _nss.cc/h
files
> have to be conditionally compiled and the namespace alias has to be switched
> from 'nss' to 'openssl'.

This doesn't look right, these are all effectively util functions which take
ExtensionFunctions as parameters. Just put them as member functions of the
extension functions themselves.

If you have some utility that you want to extract then sure, but not like this.

https://codereview.chromium.org/214863002/diff/410001/chrome/common/extension...
File chrome/common/extensions/api/enterprise_platform_keys_internal.idl (right):

https://codereview.chromium.org/214863002/diff/410001/chrome/common/extension...
chrome/common/extensions/api/enterprise_platform_keys_internal.idl:41:
ArrayBuffer publicKey,
On 2014/05/06 14:07:21, pneubeck wrote:
> @Benjamin,
> 
> all arguments which are currently ArrayBuffer should have type (ArrayBuffer or
> ArrayBufferView).
> This at first has the problem that ArrayBufferView does't have an exposed
> interface object, so cannot be checked from JS.
> 
> Say, I'm using then instead (ArrayBuffer or Uint8Array).
> This still has the second problem:
> It's translated into
> { "name": "publicKey",
>   "choices" [
>   { "type": "binary",
>     "isInstanceOf": "ArrayBuffer" },
>   { "type": "binary",
>     "isInstanceOf": "Uint8Array"}
>   ]
> }
> (if adding support for Uint8Array).
> 
> The cc_generator.py / h_generator.py don't care about the 'isInstanceOf' and
> will create a duplicate:
> 
> struct PublicKey {
>  ....
>  // Choices:
>  scoped_ptr<std::string> as_binary;
>  scoped_ptr<std::string> as_binary;
> }
> 
> One way to solve this without breaking existing code is reducing redundant
> Choices before passing the api schema to the h_/cc_generator.

Not sure what you mean by this last paragraph, but that does seem problematic.
One workaround would be to define a new dictionary type with each as a key. A
bit verbose but it's an internal API so manageable.

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
File
chrome/renderer/resources/extensions/enterprise_platform_keys/enterprise_platform_keys_key.js
(right):

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/enterprise_platform_keys_key.js:9:
* Enum of possible key types (subset of WebCrypto.KeyType).
On 2014/05/06 14:07:21, pneubeck wrote:
> On 2014/05/05 21:44:49, kalman wrote:
> > On 2014/05/05 21:28:23, pneubeck wrote:
> > > On 2014/05/05 21:09:24, kalman wrote:
> > > > why is it a subset? 
> > > WebCrypto has the additional KeyType 'secret' for symmetric keys. That one
> has
> > > just no meaning in the context of platformKeys.
> > > 
> > > > will your code still work if you just use KeyType (or does
> > > > that require experimental blink turned on?)
> > > What do you mean with 'work'? Do you imply that the closure
type-annotations
> > are
> > > actually checked? I never got an error because of the annotations. Maybe I
> > have
> > > to enable that 'strict' mode?
> > > 
> > > 
> > 
> > oh, no, not what I meant. I meant that maybe these enums already exist
> somewhere
> > on the web platform and you can re-use those.
> 
> WebCrypto defines only:
> typedef DOMString KeyType;
> typedef DOMString KeyUsage;
> 
> and in prosa that only certain strings are recognized.
> 
> The enums here are not that useful if no compiler is checking the
identifiers...
> If you think it's better, I can remove them.

Symbolic constants are nice, I won't argue with that.

I don't have an opinion, was just curious, so totally up to you.

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
File
chrome/renderer/resources/extensions/enterprise_platform_keys_internal_custom_bindings.js
(right):

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys_internal_custom_bindings.js:7:
.generate();
On 2014/05/06 14:07:21, pneubeck wrote:
> On 2014/05/05 21:09:24, kalman wrote:
> > this simple behaviour is generated for you. you only need the separate
> > custom_bindings file if you do the registerCustomHook stuff.
> 
> Hm, would be nice, but if I only remove the according
> source_map_.RegisterSource, I get the error
>   (BLESSED_EXTENSION context for kmdkjjdaihhnmbakbnbmljnmfcgelkcg) No source
for
> require(enterprise.platformKeysInternal)
> 
> Which I think is expected because the internal API is marked
>   "internal": true
> in _api_features.json

Ah right.

Yes, the problem is that you're accessing chrome.platformKeysInternal and
expecting that to be generated.

Internal APIs need to be expilicitly require(..)'d, like

var platformKeysInternal = require('platformKeysInternal')

they never get entries on the chrome object, otherwise extensions can access
them.

https://codereview.chromium.org/214863002/diff/450001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/214863002/diff/450001/base/logging.cc#newcode373
base/logging.cc:373: //    CHECK(!g_vlog_info_prev);
you might want to revert this

https://codereview.chromium.org/214863002/diff/450001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc
(right):

https://codereview.chromium.org/214863002/diff/450001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc:20:
namespace impl = nss;
is there a way to have correctly namespaced things without needing this?

https://codereview.chromium.org/214863002/diff/450001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc:44:
Respond(Error(error_message));
the fact that you need to make these alias is an indication that you're fighting
OO. better to embrace it...

[not at google - send to devlin, 2014-05-07 00:00:38]

(ah, I see Ryan is on the CL already)

[not at google - send to devlin, 2014-05-07 15:42:24]

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h
(right):

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h:13:
namespace enterprise_platform_keys {
> Ok if you insist. This is just very unusual. No other function does it and it
> looks weird to me.

I had a realisation last night that we should be generating these function names
into the right namespace. Should have been doing so all along, unfortunately.

I.e. you're on to something here.

This could be a change to make to the schema compiler, like, add an annotation
"sane_function_names" which instead of generating
"EnterprisePlatformKeysInternalGenerateKeyFunction" could generate
"extensions::api::enterprise_platform_keys_internal::GenerateKeyFunction". A
further nice property of that is that everybody wouldn't need to have those
namespace aliases in their CC files.

https://codereview.chromium.org/214863002/diff/470001/chrome/common/extension...
File chrome/common/extensions/api/enterprise_platform_keys_internal.idl (right):

https://codereview.chromium.org/214863002/diff/470001/chrome/common/extension...
chrome/common/extensions/api/enterprise_platform_keys_internal.idl:6: [ nodoc =
true,
you don't need nodoc=true if you don't add documentation for it (.html file)

[pneubeck (no reviews), 2014-05-07 21:44:18]

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
File chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.h
(right):

https://codereview.chromium.org/214863002/diff/410001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.h:25:
namespace enterprise_platform_keys {
On 2014/05/07 00:00:12, kalman wrote:
> On 2014/05/05 21:28:23, pneubeck wrote:
> > On 2014/05/05 21:09:24, kalman wrote:
> > > same comment re namespace, and why does this functionality (which depends
on
> > > TokenMethodExtensionFunction) in a separate file?
> > 
> > It's the NSS specific implementation (vs. OpenSSL). As suggested by Ryan,
this
> > shall be easily switchable.
> > 
> > I tried to prepare this for the second implementation in a way that depends
on
> > as few precompiler directives as possible. The way I did it, the _nss.cc/h
> files
> > have to be conditionally compiled and the namespace alias has to be switched
> > from 'nss' to 'openssl'.
> 
> This doesn't look right, these are all effectively util functions which take
> ExtensionFunctions as parameters. Just put them as member functions of the
> extension functions themselves.
> 
> If you have some utility that you want to extract then sure, but not like
this.

The reason was that I didn't dare to do such a crazy ownership (explicitly
AddRef/Release) or passing a scoped-ptr of an object to that object.
Thus, I instead tried it with a very clear ownership and external functions (not
methods).


Anyways, changed to AddRef/Release (which I think is in principal broken, but I
will not fix that in this CL). ptal.

https://codereview.chromium.org/214863002/diff/410001/chrome/common/extension...
File chrome/common/extensions/api/enterprise_platform_keys_internal.idl (right):

https://codereview.chromium.org/214863002/diff/410001/chrome/common/extension...
chrome/common/extensions/api/enterprise_platform_keys_internal.idl:41:
ArrayBuffer publicKey,
On 2014/05/07 00:00:12, kalman wrote:
> On 2014/05/06 14:07:21, pneubeck wrote:
> > @Benjamin,
> > 
> > all arguments which are currently ArrayBuffer should have type (ArrayBuffer
or
> > ArrayBufferView).
> > This at first has the problem that ArrayBufferView does't have an exposed
> > interface object, so cannot be checked from JS.
> > 
> > Say, I'm using then instead (ArrayBuffer or Uint8Array).
> > This still has the second problem:
> > It's translated into
> > { "name": "publicKey",
> >   "choices" [
> >   { "type": "binary",
> >     "isInstanceOf": "ArrayBuffer" },
> >   { "type": "binary",
> >     "isInstanceOf": "Uint8Array"}
> >   ]
> > }
> > (if adding support for Uint8Array).
> > 
> > The cc_generator.py / h_generator.py don't care about the 'isInstanceOf' and
> > will create a duplicate:
> > 
> > struct PublicKey {
> >  ....
> >  // Choices:
> >  scoped_ptr<std::string> as_binary;
> >  scoped_ptr<std::string> as_binary;
> > }
> > 
> > One way to solve this without breaking existing code is reducing redundant
> > Choices before passing the api schema to the h_/cc_generator.
> 
> Not sure what you mean by this last paragraph, but that does seem problematic.
> One workaround would be to define a new dictionary type with each as a key. A
> bit verbose but it's an internal API so manageable.

I have to do the same for the externally visible API (see CryptoOperationData at
http://www.w3.org/TR/WebCryptoAPI/#subtlecrypto-interface), so that doesn't
help.

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
File
chrome/renderer/resources/extensions/enterprise_platform_keys_internal_custom_bindings.js
(right):

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys_internal_custom_bindings.js:7:
.generate();
On 2014/05/07 00:00:12, kalman wrote:
> On 2014/05/06 14:07:21, pneubeck wrote:
> > On 2014/05/05 21:09:24, kalman wrote:
> > > this simple behaviour is generated for you. you only need the separate
> > > custom_bindings file if you do the registerCustomHook stuff.
> > 
> > Hm, would be nice, but if I only remove the according
> > source_map_.RegisterSource, I get the error
> >   (BLESSED_EXTENSION context for kmdkjjdaihhnmbakbnbmljnmfcgelkcg) No source
> for
> > require(enterprise.platformKeysInternal)
> > 
> > Which I think is expected because the internal API is marked
> >   "internal": true
> > in _api_features.json
> 
> Ah right.
> 
> Yes, the problem is that you're accessing chrome.platformKeysInternal and
> expecting that to be generated.
> 
> Internal APIs need to be expilicitly require(..)'d, like
> 
> var platformKeysInternal = require('platformKeysInternal')
> 
> they never get entries on the chrome object, otherwise extensions can access
> them.

Internal APIs are accessed by e.g.:
var binding = require('binding').Binding.create('webRequestInternal');

(see
https://code.google.com/p/chromium/codesearch#search/&q=require%5C(.*internal...
)

the code which prevents the creation of the binding is here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex...

I didn't want to generate the binding more than once, thus I put it into a
separate module.

https://codereview.chromium.org/214863002/diff/450001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc
(right):

https://codereview.chromium.org/214863002/diff/450001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc:20:
namespace impl = nss;
On 2014/05/07 00:00:12, kalman wrote:
> is there a way to have correctly namespaced things without needing this?

Done.

https://codereview.chromium.org/214863002/diff/450001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc:44:
Respond(Error(error_message));
On 2014/05/07 00:00:12, kalman wrote:
> the fact that you need to make these alias is an indication that you're
fighting
> OO. better to embrace it...

These functions constitute the interface that the crypto-library specific
implementation can call back.
No difference to, for example, a class exposing an interface to a delegate.


I didn't realize and couldn't have imagined that it is really a common pattern
to manually increase/decrease ref counts.
IMO, this is very broken and must be fixed.
However, by doing that, it's easier to transition to an explicit ownership by
the extension registry.

[not at google - send to devlin, 2014-05-08 00:13:13]

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
File
chrome/renderer/resources/extensions/enterprise_platform_keys_internal_custom_bindings.js
(right):

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys_internal_custom_bindings.js:7:
.generate();
On 2014/05/07 21:44:19, pneubeck wrote:
> On 2014/05/07 00:00:12, kalman wrote:
> > On 2014/05/06 14:07:21, pneubeck wrote:
> > > On 2014/05/05 21:09:24, kalman wrote:
> > > > this simple behaviour is generated for you. you only need the separate
> > > > custom_bindings file if you do the registerCustomHook stuff.
> > > 
> > > Hm, would be nice, but if I only remove the according
> > > source_map_.RegisterSource, I get the error
> > >   (BLESSED_EXTENSION context for kmdkjjdaihhnmbakbnbmljnmfcgelkcg) No
source
> > for
> > > require(enterprise.platformKeysInternal)
> > > 
> > > Which I think is expected because the internal API is marked
> > >   "internal": true
> > > in _api_features.json
> > 
> > Ah right.
> > 
> > Yes, the problem is that you're accessing chrome.platformKeysInternal and
> > expecting that to be generated.
> > 
> > Internal APIs need to be expilicitly require(..)'d, like
> > 
> > var platformKeysInternal = require('platformKeysInternal')
> > 
> > they never get entries on the chrome object, otherwise extensions can access
> > them.
> 
> Internal APIs are accessed by e.g.:
> var binding = require('binding').Binding.create('webRequestInternal');
> 
> (see
>
https://code.google.com/p/chromium/codesearch#search/&q=require%5C(.*internal...
> )
> 
> the code which prevents the creation of the binding is here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex...
> 
> I didn't want to generate the binding more than once, thus I put it into a
> separate module.

I see. Don't call it "custom bindings" though, it's confusing since it's not
actually a custom binding. That means a very specific thing. Perhaps an
"internal_api.js" file inside the enterprise_platform_keys directory. Loop over
the public interface to set on exports too; the 'binding' thing is also
confusing.

https://codereview.chromium.org/214863002/diff/470001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc
(right):

https://codereview.chromium.org/214863002/diff/470001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc:17:
namespace impl = enterprise_platform_keys_details;
this doesn't appear to be used

https://codereview.chromium.org/214863002/diff/470001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc:73:
AddRef();
you shouldn't need to manually AddRef or Release. If you want to maintain a
reference then force CreateGenerateKey, for example, to do the AddRef/Release
itself, preferably via a scoped_refptr.

Even better like I mentioned define the interface in terms of base::Callback
which will automatically manage lifetime for you.

https://codereview.chromium.org/214863002/diff/470001/chrome/browser/extensio...
File chrome/browser/extensions/api/enterprise_platform_keys/token_method.h
(right):

https://codereview.chromium.org/214863002/diff/470001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method.h:37:
EnterprisePlatformKeysTokenMethodExtensionFunction* func);
Sorry but passing Functions around just doesn't make sense to me. Pass
base::Callbacks.

[pneubeck (no reviews), 2014-05-08 15:04:16]

@Benjamin,
I started yet another variant in the last patchset. I'd like to get your opinion
on that one before I refactor again all methods.
Maybe we should just VC and solve this issue once and for all :-)

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
File
chrome/renderer/resources/extensions/enterprise_platform_keys_internal_custom_bindings.js
(right):

https://codereview.chromium.org/214863002/diff/410001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys_internal_custom_bindings.js:7:
.generate();
On 2014/05/08 00:13:14, kalman wrote:
> On 2014/05/07 21:44:19, pneubeck wrote:
> > On 2014/05/07 00:00:12, kalman wrote:
> > > On 2014/05/06 14:07:21, pneubeck wrote:
> > > > On 2014/05/05 21:09:24, kalman wrote:
> > > > > this simple behaviour is generated for you. you only need the separate
> > > > > custom_bindings file if you do the registerCustomHook stuff.
> > > > 
> > > > Hm, would be nice, but if I only remove the according
> > > > source_map_.RegisterSource, I get the error
> > > >   (BLESSED_EXTENSION context for kmdkjjdaihhnmbakbnbmljnmfcgelkcg) No
> source
> > > for
> > > > require(enterprise.platformKeysInternal)
> > > > 
> > > > Which I think is expected because the internal API is marked
> > > >   "internal": true
> > > > in _api_features.json
> > > 
> > > Ah right.
> > > 
> > > Yes, the problem is that you're accessing chrome.platformKeysInternal and
> > > expecting that to be generated.
> > > 
> > > Internal APIs need to be expilicitly require(..)'d, like
> > > 
> > > var platformKeysInternal = require('platformKeysInternal')
> > > 
> > > they never get entries on the chrome object, otherwise extensions can
access
> > > them.
> > 
> > Internal APIs are accessed by e.g.:
> > var binding = require('binding').Binding.create('webRequestInternal');
> > 
> > (see
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=require%5C(.*internal...
> > )
> > 
> > the code which prevents the creation of the binding is here:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex...
> > 
> > I didn't want to generate the binding more than once, thus I put it into a
> > separate module.
> 
> I see. Don't call it "custom bindings" though, it's confusing since it's not
> actually a custom binding. That means a very specific thing. Perhaps an
> "internal_api.js" file inside the enterprise_platform_keys directory. Loop
over
> the public interface to set on exports too; the 'binding' thing is also
> confusing.

Yes, that's better.
Done.

https://codereview.chromium.org/214863002/diff/470001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc
(right):

https://codereview.chromium.org/214863002/diff/470001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc:17:
namespace impl = enterprise_platform_keys_details;
On 2014/05/08 00:13:14, kalman wrote:
> this doesn't appear to be used

Done.

https://codereview.chromium.org/214863002/diff/470001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc:73:
AddRef();
On 2014/05/08 00:13:14, kalman wrote:
> you shouldn't need to manually AddRef or Release. If you want to maintain a
> reference then force CreateGenerateKey, for example, to do the AddRef/Release
> itself, preferably via a scoped_refptr.
In that case, doesn't it surprise you that the TokenMethod returned by
CreateGenerateKey is then owned by the ExtensionFunction and the
ExtensionFunction is owned by the scoped_refptr/base::Callback that is owned by
the TokenMethod?

> Even better like I mentioned define the interface in terms of base::Callback
> which will automatically manage lifetime for you.

I'm not sure that that is better.
The internal implementation (delegate) shouldn't have to care about the lifetime
of the ExtensionFunction but the other way round: The ExtensionFunction should
own the delegate. That's also why it's more common (at least from my experience)
that async Callbacks work with WeakPtrs; the owner is someone else.

And maybe the most critical: ExtensionFunctions cannot be shutdown in a
controlled way if they are owned by the callbacks. They should be owned by e.g.
ExtensionFunctionRegistry/ExtensionFunctionDispatcher.
If you agree, I could work on a solution for that (e.g. destroy the
ExtensionFunction in the next message loop cycle once a response was returned or
the JS/extension context was destroyed).

https://codereview.chromium.org/214863002/diff/470001/chrome/browser/extensio...
File chrome/browser/extensions/api/enterprise_platform_keys/token_method.h
(right):

https://codereview.chromium.org/214863002/diff/470001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method.h:37:
EnterprisePlatformKeysTokenMethodExtensionFunction* func);
On 2014/05/08 00:13:14, kalman wrote:
> Sorry but passing Functions around just doesn't make sense to me. Pass
> base::Callbacks.

Can you clarify what doesn't make sense to you?

The NSS specific implementation has to able to:
- get the Params
- get the current Profile()
- abort with an error message
- succeed with possibly a result value

doing that with explicit passing of the data+callbacks is less OO, which you
explicitly required before.
(Like, listener object vs. callback)

https://codereview.chromium.org/214863002/diff/470001/chrome/common/extension...
File chrome/common/extensions/api/enterprise_platform_keys_internal.idl (right):

https://codereview.chromium.org/214863002/diff/470001/chrome/common/extension...
chrome/common/extensions/api/enterprise_platform_keys_internal.idl:6: [ nodoc =
true,
On 2014/05/07 15:42:25, kalman wrote:
> you don't need nodoc=true if you don't add documentation for it (.html file)

Done.

[not at google - send to devlin, 2014-05-08 15:19:31]

https://codereview.chromium.org/214863002/diff/470001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc
(right):

https://codereview.chromium.org/214863002/diff/470001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc:73:
AddRef();
On 2014/05/08 15:04:17, pneubeck wrote:
> On 2014/05/08 00:13:14, kalman wrote:
> > you shouldn't need to manually AddRef or Release. If you want to maintain a
> > reference then force CreateGenerateKey, for example, to do the
AddRef/Release
> > itself, preferably via a scoped_refptr.
> In that case, doesn't it surprise you that the TokenMethod returned by
> CreateGenerateKey is then owned by the ExtensionFunction and the
> ExtensionFunction is owned by the scoped_refptr/base::Callback that is owned
by
> the TokenMethod?
> 
> > Even better like I mentioned define the interface in terms of base::Callback
> > which will automatically manage lifetime for you.
> 
> I'm not sure that that is better.
> The internal implementation (delegate) shouldn't have to care about the
lifetime
> of the ExtensionFunction but the other way round: The ExtensionFunction should
> own the delegate. That's also why it's more common (at least from my
experience)
> that async Callbacks work with WeakPtrs; the owner is someone else.

Where is this "delegate"? ExtensionFunction don't have owners, see comment
below.

> 
> And maybe the most critical: ExtensionFunctions cannot be shutdown in a
> controlled way if they are owned by the callbacks.

They get shut down when they're ready to respond.

> They should be owned by e.g.
> ExtensionFunctionRegistry/ExtensionFunctionDispatcher.
> If you agree, I could work on a solution for that (e.g. destroy the
> ExtensionFunction in the next message loop cycle once a response was returned
or
> the JS/extension context was destroyed).

ExtensionFunction are refcounted because they typically depends on asynchronous
operations. There is a new ExtensionFunction instance created per request. The
only thing that ExtensionFunctionRegistry owns is constructors; the
ExtensionFunctionDispatcher only creates them then sets them free. Often they're
synchronous, often they're not.

https://codereview.chromium.org/214863002/diff/470001/chrome/browser/extensio...
File chrome/browser/extensions/api/enterprise_platform_keys/token_method.h
(right):

https://codereview.chromium.org/214863002/diff/470001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method.h:37:
EnterprisePlatformKeysTokenMethodExtensionFunction* func);
On 2014/05/08 15:04:17, pneubeck wrote:
> On 2014/05/08 00:13:14, kalman wrote:
> > Sorry but passing Functions around just doesn't make sense to me. Pass
> > base::Callbacks.
> 
> Can you clarify what doesn't make sense to you?
> 
> The NSS specific implementation has to able to:
> - get the Params

Why can't the ExtensionFunction implementations get the Params, parse out the
important parts (like IDs etc), and pass them to these methods?

> - get the current Profile()

ExtensionFunctions are already scoped to Profiles.

> - abort with an error message
> - succeed with possibly a result value

Make these arguments to the Callback.

> 
> doing that with explicit passing of the data+callbacks is less OO, which you
> explicitly required before.
> (Like, listener object vs. callback)

The idiom for ExtensionFunctions is that they
- get created
- fire of asynchronous things passing base::Callbacks around (for non-legacy
code) or calling AddRef/Release (for legacy code which doesn't take Callbacks).
- auto refcounting by Callbacks releases.

[not at google - send to devlin, 2014-05-08 15:21:05]

And I'm still not understanding why you have 2 files (4 if you count the header
files) to implement this. Perhaps this is the confusion.

[not at google - send to devlin, 2014-05-08 15:55:20]

Reminder: add OWNERS file too.

[Ryan Sleevi, 2014-05-08 20:52:03]

https://codereview.chromium.org/214863002/diff/330001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/214863002/diff/330001/base/logging.cc#newcode373
base/logging.cc:373: //    CHECK(!g_vlog_info_prev);
WIP? :)

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc
(right):

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc:80:
/*
Why is this commented out?

Note: #if 0 / #endif is a far safer alternative, since if any code in between
used */, you'd end up in pain.

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h
(right):

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h:29:
bool ValidateToken(const std::string& token_id);
Document

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h:46:
void OnGeneratedKey(const std::string& public_key_der);
Document

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest.cc
(right):

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest.cc:5:
#include <cryptohi.h>
Same comments re: _nss suffix - you're relying on NSS internals here, good to
signal that it's _nss only.

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
File chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.cc
(right):

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.cc:7:
#include <cryptohi.h>
style: newline

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.cc:79:
void DidGetDB(crypto::ScopedPK11Slot slot, net::NSSCertDatabase* cert_db);
Document

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.cc:94:
void DidGetDB(crypto::ScopedPK11Slot slot, net::NSSCertDatabase* cert_db);
Document

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.cc:108:
void DidGetDB(crypto::ScopedPK11Slot slot, net::NSSCertDatabase* cert_db);
Document

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.cc:124:
void DidGetDB(crypto::ScopedPK11Slot slot, net::NSSCertDatabase* cert_db);
Document

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.cc:138:
void DidGetDB(crypto::ScopedPK11Slot slot, net::NSSCertDatabase* cert_db);
Document

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.cc:166:
if (modulus_length > 2048) {
Why do this check here?

Is it because the TPM doesn't support keys > 2048 bits? If so, let line 170-171
handle that.

That is, we shouldn't be hardcoding constants for what the TPM supports.

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.cc:171:
crypto::RSAPrivateKey::CreateSensitive(slot.get(), modulus_length));
What thread will this run on? It will block whatever thread it does - and all
calls to the TPM slot will be serialized.

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.cc:179:
if (!rsa_key->ExportPublicKey(&public_key_der)) {
Don't use this function if you want this API to be forward-compatible.

ExportPublicKey gives you an RSAPublicKey (PKCS#1), whereas ExportPublicKeyInfo
gives you a SubjectPublicKeyInfo (RFC 5280 and friends) - which includes the
algorithm OID (and matches the EC routines)

If you give callers the RSAPublicKey, you're forever coupling this API to RSA
without having them make code changes.

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.cc:201:
void Sign::DidGetDB(crypto::ScopedPK11Slot slot,
Should this class be "RSASign", since you've coupled explicitly to RSA keys?

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.cc:274:
//    client_certs.push_back(der_encoding);
delete?

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.cc:281:
//  results_ = api_eci::GetClientCertificates::Results::Create(client_certs);
delete?

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.cc:287:
: TokenMethod(func), params_(params.Pass()), weak_factory_(this) {
git cl format will show this is bad style.

https://codereview.chromium.org/214863002/diff/530001/chrome/common/extension...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/214863002/diff/530001/chrome/common/extension...
chrome/common/extensions/api/_permission_features.json:338: //    "location":
"policy"
leftover?

https://codereview.chromium.org/214863002/diff/530001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/enterprise_platform_keys/key.js
(right):

https://codereview.chromium.org/214863002/diff/530001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/key.js:46:
{readonly:['extractable', 'type', 'algorithm', 'usages']});
does this mean spki is writeable?

https://codereview.chromium.org/214863002/diff/530001/chrome/renderer/resourc...
File
chrome/renderer/resources/extensions/enterprise_platform_keys/subtle_crypto.js
(right):

https://codereview.chromium.org/214863002/diff/530001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/subtle_crypto.js:32:
'Unsupported keyUsages. Only "sign" and "verify" supported.');
WebCrypto spec has been updated about the error to throw

https://codereview.chromium.org/214863002/diff/530001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/subtle_crypto.js:35:
throw new Error('The algorithm is not supported');
ditto

https://codereview.chromium.org/214863002/diff/530001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/enterprise_platform_keys/basic.js
(right):

https://codereview.chromium.org/214863002/diff/530001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/enterprise_platform_keys/basic.js:313: //
ArrayBuffers.
This should be fixed as of yesterday

[pneubeck (no reviews), 2014-05-14 15:01:48]

ptal

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc
(right):

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc:80:
/*
On 2014/05/08 20:52:04, Ryan Sleevi wrote:
> Why is this commented out?
> 
> Note: #if 0 / #endif is a far safer alternative, since if any code in between
> used */, you'd end up in pain.

the last patchset was intended to show a variation to Benjamin, and I didn't
want to refactor everything before getting his feedback.
sorry for the confusion.

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h
(right):

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h:29:
bool ValidateToken(const std::string& token_id);
On 2014/05/08 20:52:04, Ryan Sleevi wrote:
> Document

Done.

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h:46:
void OnGeneratedKey(const std::string& public_key_der);
On 2014/05/08 20:52:04, Ryan Sleevi wrote:
> Document

Done.

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest.cc
(right):

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest.cc:5:
#include <cryptohi.h>
On 2014/05/08 20:52:04, Ryan Sleevi wrote:
> Same comments re: _nss suffix - you're relying on NSS internals here, good to
> signal that it's _nss only.

Done.

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
File chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.cc
(right):

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.cc:7:
#include <cryptohi.h>
On 2014/05/08 20:52:04, Ryan Sleevi wrote:
> style: newline

Done.

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.cc:79:
void DidGetDB(crypto::ScopedPK11Slot slot, net::NSSCertDatabase* cert_db);
On 2014/05/08 20:52:04, Ryan Sleevi wrote:
> Document

Done.

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.cc:94:
void DidGetDB(crypto::ScopedPK11Slot slot, net::NSSCertDatabase* cert_db);
On 2014/05/08 20:52:04, Ryan Sleevi wrote:
> Document

Done.

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.cc:108:
void DidGetDB(crypto::ScopedPK11Slot slot, net::NSSCertDatabase* cert_db);
On 2014/05/08 20:52:04, Ryan Sleevi wrote:
> Document

Done.

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.cc:124:
void DidGetDB(crypto::ScopedPK11Slot slot, net::NSSCertDatabase* cert_db);
On 2014/05/08 20:52:04, Ryan Sleevi wrote:
> Document

Done.

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.cc:138:
void DidGetDB(crypto::ScopedPK11Slot slot, net::NSSCertDatabase* cert_db);
On 2014/05/08 20:52:04, Ryan Sleevi wrote:
> Document

Done.

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.cc:166:
if (modulus_length > 2048) {
On 2014/05/08 20:52:04, Ryan Sleevi wrote:
> Why do this check here?
> 
> Is it because the TPM doesn't support keys > 2048 bits? If so, let line
170-171
> handle that.
> 
> That is, we shouldn't be hardcoding constants for what the TPM supports.

RSAPrivateKey seems to understand 'sensitive' as 'not exportable'.
However we require here that the key is generated in hardware (e.g. compared to
generating in software and encrypting by a hardware-backed key).

Not sure how we can ensure that. Darren Krahn said there is currently no other
way than looking at the key parameters.

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.cc:171:
crypto::RSAPrivateKey::CreateSensitive(slot.get(), modulus_length));
On 2014/05/08 20:52:04, Ryan Sleevi wrote:
> What thread will this run on? It will block whatever thread it does - and all
> calls to the TPM slot will be serialized.
Yes, I still have to move that to another thread. Thanks for the reminder.
If acceptable, I still would like to do that in a follow up. This will only be
enabled on dev channel for now.

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.cc:179:
if (!rsa_key->ExportPublicKey(&public_key_der)) {
On 2014/05/08 20:52:04, Ryan Sleevi wrote:
> Don't use this function if you want this API to be forward-compatible.
> 
> ExportPublicKey gives you an RSAPublicKey (PKCS#1), whereas
ExportPublicKeyInfo
> gives you a SubjectPublicKeyInfo (RFC 5280 and friends) - which includes the
> algorithm OID (and matches the EC routines)
> 
> If you give callers the RSAPublicKey, you're forever coupling this API to RSA
> without having them make code changes.

Hm. I thought about this one too and stopped investigating after reading this
comment in rsa_private_key.h:
  // Exports the public key to an X509 SubjectPublicKeyInfo block.
  bool ExportPublicKey(std::vector<uint8>* output) const;

The implementation looks also like what I want:
bool RSAPrivateKey::ExportPublicKey(std::vector<uint8>* output) const {
  ScopedSECItem der_pubkey(SECKEY_EncodeDERSubjectPublicKeyInfo(public_key_));

Did you mix this up with PrivateKeyInfoCodec::ExportPublicKey, maybe?
(Note, that the apitest also verifies that WebCrypto can import the public key)

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.cc:201:
void Sign::DidGetDB(crypto::ScopedPK11Slot slot,
On 2014/05/08 20:52:04, Ryan Sleevi wrote:
> Should this class be "RSASign", since you've coupled explicitly to RSA keys?

Done.

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.cc:274:
//    client_certs.push_back(der_encoding);
On 2014/05/08 20:52:04, Ryan Sleevi wrote:
> delete?

Done.

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.cc:281:
//  results_ = api_eci::GetClientCertificates::Results::Create(client_certs);
On 2014/05/08 20:52:04, Ryan Sleevi wrote:
> delete?

Done.

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.cc:287:
: TokenMethod(func), params_(params.Pass()), weak_factory_(this) {
On 2014/05/08 20:52:04, Ryan Sleevi wrote:
> git cl format will show this is bad style.
sorry to disappoint you, it doesn't.
I can change it, but then it's rather likely that I will revert it with 'git cl
format' at some time again...

https://codereview.chromium.org/214863002/diff/530001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/enterprise_platform_keys/key.js
(right):

https://codereview.chromium.org/214863002/diff/530001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/key.js:46:
{readonly:['extractable', 'type', 'algorithm', 'usages']});
On 2014/05/08 20:52:04, Ryan Sleevi wrote:
> does this mean spki is writeable?

no, this means that only these four members are exposed to the extension and
spki not at all.

https://codereview.chromium.org/214863002/diff/530001/chrome/renderer/resourc...
File
chrome/renderer/resources/extensions/enterprise_platform_keys/subtle_crypto.js
(right):

https://codereview.chromium.org/214863002/diff/530001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/subtle_crypto.js:35:
throw new Error('The algorithm is not supported');
On 2014/05/08 20:52:04, Ryan Sleevi wrote:
> ditto

Done.

https://codereview.chromium.org/214863002/diff/530001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/enterprise_platform_keys/basic.js
(right):

https://codereview.chromium.org/214863002/diff/530001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/enterprise_platform_keys/basic.js:313: //
ArrayBuffers.
On 2014/05/08 20:52:04, Ryan Sleevi wrote:
> This should be fixed as of yesterday

Done.

[pneubeck (no reviews), 2014-05-14 15:03:26]

please also take a look at the commit message which lists the important TODOs.

[pneubeck (no reviews), 2014-05-14 15:49:14]

https://codereview.chromium.org/214863002/diff/590001/chrome/renderer/extensi...
File chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc
(right):

https://codereview.chromium.org/214863002/diff/590001/chrome/renderer/extensi...
chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:164:
source_map->RegisterSource("enterprise.platformKeys",
@Benjamin, should these resources be registered only for chromeos?
similary should the resources be excluded from the renderer target if
chromeos==0 ?

[not at google - send to devlin, 2014-05-14 17:16:09]

about to have a look, responding to comment

https://codereview.chromium.org/214863002/diff/590001/chrome/renderer/extensi...
File chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc
(right):

https://codereview.chromium.org/214863002/diff/590001/chrome/renderer/extensi...
chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:164:
source_map->RegisterSource("enterprise.platformKeys",
On 2014/05/14 15:49:14, pneubeck wrote:
> @Benjamin, should these resources be registered only for chromeos?
> similary should the resources be excluded from the renderer target if
> chromeos==0 ?

we don't do that here for some reason. it doesn't really matter, particularly if
the resources aren't partitioned into chromeos only, so I wouldn't worry about
it. the features file stuff will prevent the APIs from being loaded on
non-chromeos (and non-policy for that matter).

[not at google - send to devlin, 2014-05-14 18:11:00]

extension system lgtm, thanks for bearing with me, some more comments (most
importantly the Validate one) but easy to deal with.

https://codereview.chromium.org/214863002/diff/610001/chrome/browser/chromeos...
File chrome/browser/chromeos/platform_keys/platform_keys.h (right):

https://codereview.chromium.org/214863002/diff/610001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:34: // generated key or an
error message.
maybe put the description of the arguments on the typedef?

it's the case that if |error_message| is non-empty then it's a failure? you
should say that.

same comment for everythng else in this file.

https://codereview.chromium.org/214863002/diff/610001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc
(right):

https://codereview.chromium.org/214863002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc:41:
EXTENSION_FUNCTION_VALIDATE(params && ValidateToken(params->token_id));
given that the ValidateFunction is part of the validation code here, I hope that
the custom bindings do this validation, so that the extension can't accidentally
terminate itself.

you should put a comment on ValidateToken explaining which method in the JS
custom bindings needs to be kept in sync with it, and vice versa.

https://codereview.chromium.org/214863002/diff/610001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h
(right):

https://codereview.chromium.org/214863002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h:24:
: public ChromeUIThreadExtensionFunction {
put some kind of public/private/etc scope on this.

https://codereview.chromium.org/214863002/diff/610001/chrome/common/extension...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/214863002/diff/610001/chrome/common/extension...
chrome/common/extensions/api/_permission_features.json:344: // TODO(pneubeck):
Restrict to force-installed extensions
s/force/policy ?

[pneubeck (no reviews), 2014-05-15 09:25:09]

https://codereview.chromium.org/214863002/diff/610001/chrome/browser/chromeos...
File chrome/browser/chromeos/platform_keys/platform_keys.h (right):

https://codereview.chromium.org/214863002/diff/610001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:34: // generated key or an
error message.
On 2014/05/14 18:11:01, kalman wrote:
> maybe put the description of the arguments on the typedef?
> 
> it's the case that if |error_message| is non-empty then it's a failure? you
> should say that.
> 
> same comment for everythng else in this file.

Done.

https://codereview.chromium.org/214863002/diff/610001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc
(right):

https://codereview.chromium.org/214863002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc:41:
EXTENSION_FUNCTION_VALIDATE(params && ValidateToken(params->token_id));
On 2014/05/14 18:11:01, kalman wrote:
> given that the ValidateFunction is part of the validation code here, I hope
that
> the custom bindings do this validation, so that the extension can't
accidentally
> terminate itself.
> 
> you should put a comment on ValidateToken explaining which method in the JS
> custom bindings needs to be kept in sync with it, and vice versa.

Not sure what you mean by 'accidentally terminate itself', but indeed this was
not handled correctly/consistently.

This error is now consistent over all token functions (certs and SubtleCrypto
functions).
It's not ideal that the error message (kErrorInvalidToken) exists both in C++
and Javascript and must be sync'ed manually.

Is there an easy automatic way for this?

https://codereview.chromium.org/214863002/diff/610001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h
(right):

https://codereview.chromium.org/214863002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.h:24:
: public ChromeUIThreadExtensionFunction {
On 2014/05/14 18:11:01, kalman wrote:
> put some kind of public/private/etc scope on this.

Done.

https://codereview.chromium.org/214863002/diff/610001/chrome/common/extension...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/214863002/diff/610001/chrome/common/extension...
chrome/common/extensions/api/_permission_features.json:344: // TODO(pneubeck):
Restrict to force-installed extensions
On 2014/05/14 18:11:01, kalman wrote:
> s/force/policy ?

Done.

[not at google - send to devlin, 2014-05-15 15:03:04]

https://codereview.chromium.org/214863002/diff/610001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc
(right):

https://codereview.chromium.org/214863002/diff/610001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc:41:
EXTENSION_FUNCTION_VALIDATE(params && ValidateToken(params->token_id));

> Not sure what you mean by 'accidentally terminate itself', but indeed this was
> not handled correctly/consistently.

I mean that a failure on EXTENSION_FUNCTION_VALIDATE means "terminate the
process".

> 
> This error is now consistent over all token functions (certs and SubtleCrypto
> functions).
> It's not ideal that the error message (kErrorInvalidToken) exists both in C++
> and Javascript and must be sync'ed manually.
> 
> Is there an easy automatic way for this?

Not really, at least not for significant strings. There is a way to share
constants (see
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...)
but that's not appropriate for a full error string.

If you want to share that, the only way currently would be to put the error
message somewhere in common/ and expose it to JS via a native handler
(requireNative etc).

[not at google - send to devlin, 2014-05-15 15:04:52]

> Not really, at least not for significant strings. There is a way to share
> constants (see
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...)
> but that's not appropriate for a full error string.

Eh, maybe it is appropriate.

[pneubeck (no reviews), 2014-05-15 15:14:11]

@Ryan, ptal if you have a chance.

[Ryan Sleevi, 2014-05-15 19:45:29]

Adding in eroman@ to sanity check

[Ryan Sleevi, 2014-05-15 19:57:12]

Ok, deferring the remaining bits to Eric and Matt

In this round, I specifically focused on the token_method_nss.cc bits, which
seem to LGTM, mod the threading issue, which I think is a blocker for enabling
this.

Matt: Please look at the use of profiles and token_method_nss, and make sure
you're happy with the NSS interactions here. Also, if you see anything that
would make it difficult for OpenSSL migration (that might need abstraction),
feel free to comment.

Eric: Could you take a look at the WebCrypto emulation, in
chrome/renderer/resources/extensions/enterprise_platform_keys/, and see if
you're happy with how it polyfills WebCrypto atop the IDL. Mostly the walks like
a duck/talks like a duck smell test.

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
File chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.cc
(right):

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.cc:166:
if (modulus_length > 2048) {
On 2014/05/14 15:01:49, pneubeck wrote:
> On 2014/05/08 20:52:04, Ryan Sleevi wrote:
> > Why do this check here?
> > 
> > Is it because the TPM doesn't support keys > 2048 bits? If so, let line
> 170-171
> > handle that.
> > 
> > That is, we shouldn't be hardcoding constants for what the TPM supports.
> 
> RSAPrivateKey seems to understand 'sensitive' as 'not exportable'.
> However we require here that the key is generated in hardware (e.g. compared
to
> generating in software and encrypting by a hardware-backed key).
> 
> Not sure how we can ensure that. Darren Krahn said there is currently no other
> way than looking at the key parameters.

Hrm. I really wish Chaps used separate slots then, if this is a distinction that
callers are meant to rely on.

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.cc:171:
crypto::RSAPrivateKey::CreateSensitive(slot.get(), modulus_length));
On 2014/05/14 15:01:49, pneubeck wrote:
> On 2014/05/08 20:52:04, Ryan Sleevi wrote:
> > What thread will this run on? It will block whatever thread it does - and
all
> > calls to the TPM slot will be serialized.
> Yes, I still have to move that to another thread. Thanks for the reminder.
> If acceptable, I still would like to do that in a follow up. This will only be
> enabled on dev channel for now.

I don't think we should enable this API until that's in place.

This will cause the entire UI to hang (because IO thread == GPU command buffer
thread). Hang to long, and it will force Chrome to terminate.

To me, that's unacceptable.

https://codereview.chromium.org/214863002/diff/530001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/token_method_nss.cc:179:
if (!rsa_key->ExportPublicKey(&public_key_der)) {
On 2014/05/14 15:01:49, pneubeck wrote:
> On 2014/05/08 20:52:04, Ryan Sleevi wrote:
> > Don't use this function if you want this API to be forward-compatible.
> > 
> > ExportPublicKey gives you an RSAPublicKey (PKCS#1), whereas
> ExportPublicKeyInfo
> > gives you a SubjectPublicKeyInfo (RFC 5280 and friends) - which includes the
> > algorithm OID (and matches the EC routines)
> > 
> > If you give callers the RSAPublicKey, you're forever coupling this API to
RSA
> > without having them make code changes.
> 
> Hm. I thought about this one too and stopped investigating after reading this
> comment in rsa_private_key.h:
>   // Exports the public key to an X509 SubjectPublicKeyInfo block.
>   bool ExportPublicKey(std::vector<uint8>* output) const;
> 
> The implementation looks also like what I want:
> bool RSAPrivateKey::ExportPublicKey(std::vector<uint8>* output) const {
>   ScopedSECItem der_pubkey(SECKEY_EncodeDERSubjectPublicKeyInfo(public_key_));
> 
> Did you mix this up with PrivateKeyInfoCodec::ExportPublicKey, maybe?
> (Note, that the apitest also verifies that WebCrypto can import the public
key)

Yeah, I did. Thanks for digging in.

https://codereview.chromium.org/214863002/diff/660001/chrome/browser/chromeos...
File chrome/browser/chromeos/platform_keys/platform_keys.h (right):

https://codereview.chromium.org/214863002/diff/660001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:27: // If the generation
was successful, |public_key_str| will contain the DER
naming: public_key_str -> public_key_der or public_key_spki

https://codereview.chromium.org/214863002/diff/660001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:39: int modulus_length,
Should be unsigned. What does it mean to generate with a negative modulus?

https://codereview.chromium.org/214863002/diff/660001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:87:
scoped_refptr<net::X509Certificate> certificate,
where do the intermediates in |certificate| go?

https://codereview.chromium.org/214863002/diff/660001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:101:
scoped_refptr<net::X509Certificate> certificate,
ditto intermediates (just need to document)

https://codereview.chromium.org/214863002/diff/660001/chrome/browser/chromeos...
File chrome/browser/chromeos/platform_keys/platform_keys_nss.cc (right):

https://codereview.chromium.org/214863002/diff/660001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:213: if
(state->modulus_length_ > 2048) {
and negative moduli?
should 2048 be a const? kMaxRsaModulus?

[pneubeck (no reviews), 2014-05-16 08:01:42]

For clarification: token_method_* was renamed to platform_keys_*

[pneubeck (no reviews), 2014-05-16 12:30:15]

NSS operations are now all run on a worker thread from the worker pool.

https://codereview.chromium.org/214863002/diff/660001/chrome/browser/chromeos...
File chrome/browser/chromeos/platform_keys/platform_keys.h (right):

https://codereview.chromium.org/214863002/diff/660001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:27: // If the generation
was successful, |public_key_str| will contain the DER
On 2014/05/15 19:57:13, Ryan Sleevi wrote:
> naming: public_key_str -> public_key_der or public_key_spki

Done.

https://codereview.chromium.org/214863002/diff/660001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:39: int modulus_length,
On 2014/05/15 19:57:13, Ryan Sleevi wrote:
> Should be unsigned. What does it mean to generate with a negative modulus?

Done.

https://codereview.chromium.org/214863002/diff/660001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:87:
scoped_refptr<net::X509Certificate> certificate,
On 2014/05/15 19:57:13, Ryan Sleevi wrote:
> where do the intermediates in |certificate| go? 

documented that they're ignored.

https://codereview.chromium.org/214863002/diff/660001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:101:
scoped_refptr<net::X509Certificate> certificate,
On 2014/05/15 19:57:13, Ryan Sleevi wrote:
> ditto intermediates (just need to document)

documented that they're ignored.

https://codereview.chromium.org/214863002/diff/660001/chrome/browser/chromeos...
File chrome/browser/chromeos/platform_keys/platform_keys_nss.cc (right):

https://codereview.chromium.org/214863002/diff/660001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:213: if
(state->modulus_length_ > 2048) {
On 2014/05/15 19:57:13, Ryan Sleevi wrote:
> and negative moduli?
> should 2048 be a const? kMaxRsaModulus?

Done.

[mattm, 2014-05-16 20:27:11]

https://codereview.chromium.org/214863002/diff/660001/chrome/browser/chromeos...
File chrome/browser/chromeos/platform_keys/platform_keys.h (right):

https://codereview.chromium.org/214863002/diff/660001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:105: }  // namespace 
platform_keys
extraneous space

https://codereview.chromium.org/214863002/diff/660001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc
(right):

https://codereview.chromium.org/214863002/diff/660001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc:134:
content::RunAllPendingInMessageLoop();
Using base::RunLoop for this would be preferable (call run_loop.Run() here, pass
the run_loop to DidGetCertDatabase and have it call run_loop.Quit())

https://codereview.chromium.org/214863002/diff/660001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database.cc (right):

https://codereview.chromium.org/214863002/diff/660001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.cc:381: CertificateList* certs) {
certs->clear();

https://codereview.chromium.org/214863002/diff/700001/chrome/browser/chromeos...
File chrome/browser/chromeos/platform_keys/platform_keys_nss.cc (right):

https://codereview.chromium.org/214863002/diff/700001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:89:
base::WorkerPool::PostTask(FROM_HERE, callback, true /*task is slow*/);
This is problematic, since the cert_db_ could go away before/during the worker
pool task. I think the asynchronous bits needs to be built into the
NSSCertDatabase interface, like it already is for ListCerts.

https://codereview.chromium.org/214863002/diff/700001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:112: void
CallBack(const std::string& public_key_spki_der,
Pass the FROM_HERE through the CallBack methods args, that way it provides a bit
more useful info when debugging.

[pneubeck (no reviews), 2014-05-19 19:17:18]

@Matt, ptal

https://codereview.chromium.org/214863002/diff/660001/chrome/browser/chromeos...
File chrome/browser/chromeos/platform_keys/platform_keys.h (right):

https://codereview.chromium.org/214863002/diff/660001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:105: }  // namespace 
platform_keys
On 2014/05/16 20:27:12, mattm wrote:
> extraneous space

Done.

https://codereview.chromium.org/214863002/diff/660001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc
(right):

https://codereview.chromium.org/214863002/diff/660001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_apitest_nss.cc:134:
content::RunAllPendingInMessageLoop();
On 2014/05/16 20:27:12, mattm wrote:
> Using base::RunLoop for this would be preferable (call run_loop.Run() here,
pass
> the run_loop to DidGetCertDatabase and have it call run_loop.Quit())

Done.

https://codereview.chromium.org/214863002/diff/660001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database.cc (right):

https://codereview.chromium.org/214863002/diff/660001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.cc:381: CertificateList* certs) {
On 2014/05/16 20:27:12, mattm wrote:
> certs->clear();

Merged with other ListCertsImpl.
Done.

https://codereview.chromium.org/214863002/diff/700001/chrome/browser/chromeos...
File chrome/browser/chromeos/platform_keys/platform_keys_nss.cc (right):

https://codereview.chromium.org/214863002/diff/700001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:89:
base::WorkerPool::PostTask(FROM_HERE, callback, true /*task is slow*/);
On 2014/05/16 20:27:12, mattm wrote:
> This is problematic, since the cert_db_ could go away before/during the worker
> pool task. I think the asynchronous bits needs to be built into the
> NSSCertDatabase interface, like it already is for ListCerts.

Thanks for pointing this out, I though of this too shallow.

I only made DeleteCertAndKey in NSSCertDatabase asynchronous. CertDatabase is
cros platform and is out of scope (in particular for this CL).

https://codereview.chromium.org/214863002/diff/700001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:112: void
CallBack(const std::string& public_key_spki_der,
On 2014/05/16 20:27:12, mattm wrote:
> Pass the FROM_HERE through the CallBack methods args, that way it provides a
bit
> more useful info when debugging.

Done.

[eroman, 2014-05-19 23:27:44]

https://codereview.chromium.org/214863002/diff/780001/chrome/browser/chromeos...
File chrome/browser/chromeos/platform_keys/platform_keys.h (right):

https://codereview.chromium.org/214863002/diff/780001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:36: // instead always the
user token associated to |profile| is used. |callback|
Something is gramatically wrong with this sentence.

https://codereview.chromium.org/214863002/diff/780001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:51: // the user token
associated to |profile| is used. |public_key| must be the DER
instead the user token ... is always used.

https://codereview.chromium.org/214863002/diff/780001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:84: // |token_id| is
currently ignored, instead always the user token associated to
same comment

https://codereview.chromium.org/214863002/diff/780001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:89: const
ImportCertificateCallback& callback,
Should this be indented further?

https://codereview.chromium.org/214863002/diff/780001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:99: // always the user
token associated to |profile| is used. |callback| will be
ditto

https://codereview.chromium.org/214863002/diff/780001/chrome/browser/chromeos...
File chrome/browser/chromeos/platform_keys/platform_keys_nss.cc (right):

https://codereview.chromium.org/214863002/diff/780001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:287: Profile*
profile)
The ownership model here seems ill defined.

There are raw pointers involved here for Profile, ResourceContext,
net::NSSCertDatabase.

It does not appear that there is any cancellation of requests, so what happens
if the Profile is destroyed and key generation is in progress? Could it result
in a use-after free on ResourceContext/NSSCertDatabase?

https://codereview.chromium.org/214863002/diff/780001/chrome/common/extension...
File chrome/common/extensions/api/enterprise_platform_keys.idl (right):

https://codereview.chromium.org/214863002/diff/780001/chrome/common/extension...
chrome/common/extensions/api/enterprise_platform_keys.idl:62: ArrayBuffer
certificate,
Regarding this API...

  * Seems like it would be better to use ArrayBufferView for input. ArrayBuffer
is fine for output, but somewhat inconvenient for input.

  * Are extensions being advised to use Promises? (The pattern of a done
completion callback seems like it could be a Promise instead).

https://codereview.chromium.org/214863002/diff/780001/chrome/common/extension...
chrome/common/extensions/api/enterprise_platform_keys.idl:69: // TODO: Instead
of ArrayBuffer should be (ArrayBuffer or ArrayBufferView),
Ah, you mention my comment from above :)
If you have to pick one, I would go with ArrayBufferView, much more convenient
for users.

Not sure how extension IDLs are compiled, however you can probably achieve the
"or" by adding overloads (that is what we do on the blink side anyhow.

https://codereview.chromium.org/214863002/diff/780001/chrome/common/extension...
File chrome/common/extensions/api/enterprise_platform_keys_internal.idl (right):

https://codereview.chromium.org/214863002/diff/780001/chrome/common/extension...
chrome/common/extensions/api/enterprise_platform_keys_internal.idl:34: long
modulusLength,
can this be "unsigned long" ?

https://codereview.chromium.org/214863002/diff/780001/chrome/renderer/resourc...
File
chrome/renderer/resources/extensions/enterprise_platform_keys/subtle_crypto.js
(right):

https://codereview.chromium.org/214863002/diff/780001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/subtle_crypto.js:81:
if (algorithm.name != 'RSASSA-PKCS1-v1_5') {
Should allow case-insensitive comparison.

[pneubeck (no reviews), 2014-05-20 09:29:20]

https://codereview.chromium.org/214863002/diff/780001/chrome/browser/chromeos...
File chrome/browser/chromeos/platform_keys/platform_keys.h (right):

https://codereview.chromium.org/214863002/diff/780001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:36: // instead always the
user token associated to |profile| is used. |callback|
On 2014/05/19 23:27:45, eroman wrote:
> Something is gramatically wrong with this sentence.

Done.

https://codereview.chromium.org/214863002/diff/780001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:51: // the user token
associated to |profile| is used. |public_key| must be the DER
On 2014/05/19 23:27:45, eroman wrote:
> instead the user token ... is always used.

Done.

https://codereview.chromium.org/214863002/diff/780001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:84: // |token_id| is
currently ignored, instead always the user token associated to
On 2014/05/19 23:27:45, eroman wrote:
> same comment

Done.

https://codereview.chromium.org/214863002/diff/780001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:89: const
ImportCertificateCallback& callback,
On 2014/05/19 23:27:45, eroman wrote:
> Should this be indented further?
I don't know what you mean here.
Anyways, I'll run 'git cl format' again before uploading.

https://codereview.chromium.org/214863002/diff/780001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:99: // always the user
token associated to |profile| is used. |callback| will be
On 2014/05/19 23:27:45, eroman wrote:
> ditto

Done.

https://codereview.chromium.org/214863002/diff/780001/chrome/browser/chromeos...
File chrome/browser/chromeos/platform_keys/platform_keys_nss.cc (right):

https://codereview.chromium.org/214863002/diff/780001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:287: Profile*
profile)
On 2014/05/19 23:27:45, eroman wrote:
> The ownership model here seems ill defined.
> 
> There are raw pointers involved here for Profile, ResourceContext,
> net::NSSCertDatabase.
> 
> It does not appear that there is any cancellation of requests, so what happens
> if the Profile is destroyed and key generation is in progress? Could it result
> in a use-after free on ResourceContext/NSSCertDatabase?

I fixed Profile*, by passing it explicitly. It's now used only synchronously on
the UI thread.

ResourceContext must to be used on the IO thread and it should be fine to use it
in the next posted task, since UI thread is not in the process of shutting down
IO thread when posting the task.
See nss_context.cc:GetNSSCertDatabaseForProfile for comparison.

The per-context NSSCertDatabases (there are multiple instances nowadays) live on
the IO thread and are owned by the respective ResourceContext. I use it only
synchronously on the IO thread after obtaining the pointer.

https://codereview.chromium.org/214863002/diff/780001/chrome/common/extension...
File chrome/common/extensions/api/enterprise_platform_keys.idl (right):

https://codereview.chromium.org/214863002/diff/780001/chrome/common/extension...
chrome/common/extensions/api/enterprise_platform_keys.idl:62: ArrayBuffer
certificate,
On 2014/05/19 23:27:45, eroman wrote:
> Regarding this API...
> 
>   * Seems like it would be better to use ArrayBufferView for input.
ArrayBuffer
> is fine for output, but somewhat inconvenient for input.
ArrayBufferView itself can't be used (even with adaptions to the idl parser /
schema-generator) because the it's defined as  [NoInterfaceObject].
Instead I would have to use e.g. (ArrayBuffer or Uint8Array).

Uint8Array will require adaptions to the generator, it must create the json
schema-type 'binary'.
I really want to punt this to a follow up.

> 
>   * Are extensions being advised to use Promises? (The pattern of a done
> completion callback seems like it could be a Promise instead).

No, according to the extensions team we shouldn't start a patch work with a few
extension APIs using promises and the majority using callbacks.

https://codereview.chromium.org/214863002/diff/780001/chrome/common/extension...
chrome/common/extensions/api/enterprise_platform_keys.idl:69: // TODO: Instead
of ArrayBuffer should be (ArrayBuffer or ArrayBufferView),
On 2014/05/19 23:27:45, eroman wrote:
> Ah, you mention my comment from above :)
> If you have to pick one, I would go with ArrayBufferView, much more convenient
> for users.
> 
> Not sure how extension IDLs are compiled, however you can probably achieve the
> "or" by adding overloads (that is what we do on the blink side anyhow.

Sadly it's very unclear how to realize the 'or' in this case.
At first both idl types are translated into the identical json-schema types
'binary' with the annotation 'instanceOf:ArrayBuffer' and
'instanceOf:Uint8Array'.
Then the generator produces code ignoring the annotation. Thus this leads to a
collision.
I'll have to figure out with @kalman how to resolve this.

https://codereview.chromium.org/214863002/diff/780001/chrome/common/extension...
File chrome/common/extensions/api/enterprise_platform_keys_internal.idl (right):

https://codereview.chromium.org/214863002/diff/780001/chrome/common/extension...
chrome/common/extensions/api/enterprise_platform_keys_internal.idl:34: long
modulusLength,
On 2014/05/19 23:27:45, eroman wrote:
> can this be "unsigned long" ?

I'd love to, but not supported so far either.

https://codereview.chromium.org/214863002/diff/780001/chrome/renderer/resourc...
File
chrome/renderer/resources/extensions/enterprise_platform_keys/subtle_crypto.js
(right):

https://codereview.chromium.org/214863002/diff/780001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/enterprise_platform_keys/subtle_crypto.js:81:
if (algorithm.name != 'RSASSA-PKCS1-v1_5') {
On 2014/05/19 23:27:45, eroman wrote:
> Should allow case-insensitive comparison.
Done.

Note that I want to reuse the normalization of the WebCrypto implementation in
the future.

[pneubeck (no reviews), 2014-05-20 12:40:37]

@Eric and Matt
please take another look. I answered/addressed all your comments.

[Ryan Sleevi, 2014-05-20 18:04:08]

https://codereview.chromium.org/214863002/diff/780001/chrome/common/extension...
File chrome/common/extensions/api/enterprise_platform_keys.idl (right):

https://codereview.chromium.org/214863002/diff/780001/chrome/common/extension...
chrome/common/extensions/api/enterprise_platform_keys.idl:62: ArrayBuffer
certificate,
On 2014/05/20 09:29:21, pneubeck wrote:
> On 2014/05/19 23:27:45, eroman wrote:
> > Regarding this API...
> > 
> >   * Seems like it would be better to use ArrayBufferView for input.
> ArrayBuffer
> > is fine for output, but somewhat inconvenient for input.
> ArrayBufferView itself can't be used (even with adaptions to the idl parser /
> schema-generator) because the it's defined as  [NoInterfaceObject].
> Instead I would have to use e.g. (ArrayBuffer or Uint8Array).
> 
> Uint8Array will require adaptions to the generator, it must create the json
> schema-type 'binary'.
> I really want to punt this to a follow up.

Are you looked at the old TypedArray spec?

This is fixed in ES6 and HTML - see "JavaScript" under
http://www.whatwg.org/specs/web-apps/current-work/multipage/infrastructure.ht...

"The following helper IDL is used for referring to ArrayBuffer-related types
typedef (Int8Array or Uint8Array or Uint8ClampedArray or
         Int16Array or Uint16Array or Int32Array or Uint32Array or
         Float32Array or Float64Array or DataView) ArrayBufferView"

If we have an old IDL definition, it should be easy to fix.
> 
> > 
> >   * Are extensions being advised to use Promises? (The pattern of a done
> > completion callback seems like it could be a Promise instead).
> 
> No, according to the extensions team we shouldn't start a patch work with a
few
> extension APIs using promises and the majority using callbacks.

[eroman, 2014-05-20 20:16:50]

lgtm for the webcrypto interface.

certainly not an exact match, but good enough for a first iteration.

note that I reviewed this CL as best I could, but did not fully understand the
extension interactions. i trust your other reviewers (kalman?) are suited for
this.

https://codereview.chromium.org/214863002/diff/780001/chrome/common/extension...
File chrome/common/extensions/api/enterprise_platform_keys.idl (right):

https://codereview.chromium.org/214863002/diff/780001/chrome/common/extension...
chrome/common/extensions/api/enterprise_platform_keys.idl:69: // TODO: Instead
of ArrayBuffer should be (ArrayBuffer or ArrayBufferView),
On 2014/05/20 09:29:21, pneubeck wrote:
> On 2014/05/19 23:27:45, eroman wrote:
> > Ah, you mention my comment from above :)
> > If you have to pick one, I would go with ArrayBufferView, much more
convenient
> > for users.
> > 
> > Not sure how extension IDLs are compiled, however you can probably achieve
the
> > "or" by adding overloads (that is what we do on the blink side anyhow.
> 
> Sadly it's very unclear how to realize the 'or' in this case.
> At first both idl types are translated into the identical json-schema types
> 'binary' with the annotation 'instanceOf:ArrayBuffer' and
> 'instanceOf:Uint8Array'.
> Then the generator produces code ignoring the annotation. Thus this leads to a
> collision.
> I'll have to figure out with @kalman how to resolve this.

I see. So the issue is that extensions use a different IDL generator from Blink
proper for their parameter type validation. My recommendations applied to the
Blink IDL generator, I do not know about the extension one. OK to punt to later
version then.

[mattm, 2014-05-20 21:10:15]

lgtm

[pneubeck (no reviews), 2014-05-21 15:01:39]

https://codereview.chromium.org/214863002/diff/780001/chrome/common/extension...
File chrome/common/extensions/api/enterprise_platform_keys.idl (right):

https://codereview.chromium.org/214863002/diff/780001/chrome/common/extension...
chrome/common/extensions/api/enterprise_platform_keys.idl:62: ArrayBuffer
certificate,
On 2014/05/20 18:04:09, Ryan Sleevi wrote:
> On 2014/05/20 09:29:21, pneubeck wrote:
> > On 2014/05/19 23:27:45, eroman wrote:
> > > Regarding this API...
> > > 
> > >   * Seems like it would be better to use ArrayBufferView for input.
> > ArrayBuffer
> > > is fine for output, but somewhat inconvenient for input.
> > ArrayBufferView itself can't be used (even with adaptions to the idl parser
/
> > schema-generator) because the it's defined as  [NoInterfaceObject].
> > Instead I would have to use e.g. (ArrayBuffer or Uint8Array).
> > 
> > Uint8Array will require adaptions to the generator, it must create the json
> > schema-type 'binary'.
> > I really want to punt this to a follow up.
> 
> Are you looked at the old TypedArray spec?
> 
> This is fixed in ES6 and HTML - see "JavaScript" under
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/infrastructure.ht...
> 
> "The following helper IDL is used for referring to ArrayBuffer-related types
> typedef (Int8Array or Uint8Array or Uint8ClampedArray or
>          Int16Array or Uint16Array or Int32Array or Uint32Array or
>          Float32Array or Float64Array or DataView) ArrayBufferView"
> 
> If we have an old IDL definition, it should be easy to fix.
> > 
> > > 
> > >   * Are extensions being advised to use Promises? (The pattern of a done
> > > completion callback seems like it could be a Promise instead).
> > 
> > No, according to the extensions team we shouldn't start a patch work with a
> few
> > extension APIs using promises and the majority using callbacks.
> 

Cool, that will be helpful to make it look like WebCrypto.
This will still depend on blink and the generator being updated also, so TBD.

https://codereview.chromium.org/214863002/diff/780001/chrome/common/extension...
chrome/common/extensions/api/enterprise_platform_keys.idl:69: // TODO: Instead
of ArrayBuffer should be (ArrayBuffer or ArrayBufferView),
On 2014/05/20 20:16:51, eroman wrote:
> On 2014/05/20 09:29:21, pneubeck wrote:
> > On 2014/05/19 23:27:45, eroman wrote:
> > > Ah, you mention my comment from above :)
> > > If you have to pick one, I would go with ArrayBufferView, much more
> convenient
> > > for users.
> > > 
> > > Not sure how extension IDLs are compiled, however you can probably achieve
> the
> > > "or" by adding overloads (that is what we do on the blink side anyhow.
> > 
> > Sadly it's very unclear how to realize the 'or' in this case.
> > At first both idl types are translated into the identical json-schema types
> > 'binary' with the annotation 'instanceOf:ArrayBuffer' and
> > 'instanceOf:Uint8Array'.
> > Then the generator produces code ignoring the annotation. Thus this leads to
a
> > collision.
> > I'll have to figure out with @kalman how to resolve this.
> 
> I see. So the issue is that extensions use a different IDL generator from
Blink
> proper for their parameter type validation. My recommendations applied to the
> Blink IDL generator, I do not know about the extension one. OK to punt to
later
> version then.

Yes, sadly we have many different IDL and json parsers and generators.

[pneubeck (no reviews), 2014-05-22 12:23:29]

@Jochen, please review
chrome/renderer/resources/renderer_resources.grd

@Jar, please review
extensions/browser/extension_function_histogram_value.h

Thanks

[jochen (gone - plz use gerrit), 2014-05-22 12:49:23]

is this intended for chromeos only? It looks like the API will be compiled in on
all platforms?

[pneubeck (no reviews), 2014-05-22 12:54:34]

On 2014/05/22 12:49:23, jochen wrote:
> is this intended for chromeos only? It looks like the API will be compiled in
on
> all platforms?

I asked Benjamin about this before:
https://codereview.chromium.org/214863002/diff/590001/chrome/renderer/extensi...

[jochen (gone - plz use gerrit), 2014-05-22 13:05:42]

grd changes lgtm

[pneubeck (no reviews), 2014-05-22 17:59:36]

https://codereview.chromium.org/214863002/diff/940001/chrome/common/extension...
File chrome/common/extensions/permissions/permission_set_unittest.cc (right):

https://codereview.chromium.org/214863002/diff/940001/chrome/common/extension...
chrome/common/extensions/permissions/permission_set_unittest.cc:678: // These
permissions are restricted to extensions force-installed by policy
@Benjamin, is this OK?

[not at google - send to devlin, 2014-05-22 18:03:00]

https://codereview.chromium.org/214863002/diff/940001/chrome/common/extension...
File chrome/common/extensions/permissions/permission_set_unittest.cc (right):

https://codereview.chromium.org/214863002/diff/940001/chrome/common/extension...
chrome/common/extensions/permissions/permission_set_unittest.cc:678: // These
permissions are restricted to extensions force-installed by policy
On 2014/05/22 17:59:37, pneubeck wrote:
> @Benjamin, is this OK?

is what ok?

[pneubeck (no reviews), 2014-05-22 18:06:26]

https://codereview.chromium.org/214863002/diff/940001/chrome/common/extension...
File chrome/common/extensions/permissions/permission_set_unittest.cc (right):

https://codereview.chromium.org/214863002/diff/940001/chrome/common/extension...
chrome/common/extensions/permissions/permission_set_unittest.cc:678: // These
permissions are restricted to extensions force-installed by policy
On 2014/05/22 18:03:01, kalman wrote:
> On 2014/05/22 17:59:37, pneubeck wrote:
> > @Benjamin, is this OK?
> 
> is what ok?

this change to this test.
Maybe we could change it in a follow up to skip all permissions with
location:policy.

[not at google - send to devlin, 2014-05-22 18:09:07]

https://codereview.chromium.org/214863002/diff/940001/chrome/common/extension...
File chrome/common/extensions/permissions/permission_set_unittest.cc (right):

https://codereview.chromium.org/214863002/diff/940001/chrome/common/extension...
chrome/common/extensions/permissions/permission_set_unittest.cc:678: // These
permissions are restricted to extensions force-installed by policy
On 2014/05/22 18:06:27, pneubeck wrote:
> On 2014/05/22 18:03:01, kalman wrote:
> > On 2014/05/22 17:59:37, pneubeck wrote:
> > > @Benjamin, is this OK?
> > 
> > is what ok?
> 
> this change to this test.
> Maybe we could change it in a follow up to skip all permissions with
> location:policy.

as in, is it ok that the platform keys API doesn't have a permission string? I
think yes.

[Alexei Svitkine (slow), 2014-05-23 16:22:47]

lgtm

[pneubeck (no reviews), 2014-05-23 16:23:23]

-jar@

[pneubeck (no reviews), 2014-05-23 16:23:53]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-05-23 16:24:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pneubeck@chromium.org/214863002/910041

[commit-bot: I haz the power, 2014-05-23 19:36:21]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-23 22:14:27]

Change committed as 272613