Reuse WebCrypto's normalizeCryptoAlgorithm in enterprise.platformKeys.

Reuse WebCrypto's normalizeCryptoAlgorithm in enterprise.platformKeys.

The enterprise.platformKeys extension API implements a WebCrypto.SubtleCrypto and thus has to apply the normalization of algorithm parameters as required by the SubtleCrypto definition.
To minimize redundancy in the extension API, this commit is a first step to reuse code of the WebCrypto implementation which lives partially in blink. A new ObjectBackedNativeHandler is added which allows to call the normalizeCryptoAlgorithm function exposed by blink from the API's custom binding in Javascript.

Depends on these blink changes:
https://codereview.chromium.org/295423004/
https://codereview.chromium.org/312393004/

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

[pneubeck (no reviews), 2014-05-29 13:04:30]

Eric, please also review this part (or hand over to Ryan)

@Benjamin,
Please check c/r/e/chrome_extensions_dispatcher_delegate.cc and whether the use
of the ObjectBackedNativeHandler is idiomatic.

[not at google - send to devlin, 2014-05-29 15:06:57]

https://codereview.chromium.org/298073009/diff/1/chrome/renderer/extensions/e...
File chrome/renderer/extensions/enterprise_platform_keys_natives.cc (right):

https://codereview.chromium.org/298073009/diff/1/chrome/renderer/extensions/e...
chrome/renderer/extensions/enterprise_platform_keys_natives.cc:48: default:
if you leave out the default case it will be a compile error. usually that is
preferable, but it might complicate blink rolls.

just noting it in case it's important.

https://codereview.chromium.org/298073009/diff/1/chrome/renderer/extensions/e...
chrome/renderer/extensions/enterprise_platform_keys_natives.cc:58:
scoped_ptr<base::DictionaryValue> dict(new base::DictionaryValue);
is this method likely to get much more complicated? if not, you might be better
off saving the effort of the V8ValueConversion and setting the v8 API directly;
it's more verbose, but not that much more verbose.

up to you.

https://codereview.chromium.org/298073009/diff/1/chrome/renderer/extensions/e...
chrome/renderer/extensions/enterprise_platform_keys_natives.cc:88:
DCHECK(call_info.Length() == 2);
DCHECK_EQ

https://codereview.chromium.org/298073009/diff/1/chrome/renderer/extensions/e...
chrome/renderer/extensions/enterprise_platform_keys_natives.cc:92: std::string
operation(*v8::String::Utf8Value(call_info[1]));
not used?

https://codereview.chromium.org/298073009/diff/1/chrome/renderer/extensions/e...
chrome/renderer/extensions/enterprise_platform_keys_natives.cc:105:
scoped_ptr<base::DictionaryValue> algorithmDict;
algorithm_dict

https://codereview.chromium.org/298073009/diff/1/chrome/renderer/extensions/e...
chrome/renderer/extensions/enterprise_platform_keys_natives.cc:110:
call_info.GetReturnValue().SetNull();
there are a couple of other ways you could fail here:
- return undefined (i.e. never set the value)
- throw an exception

returning null seems correct enough, so again, just noting it...

https://codereview.chromium.org/298073009/diff/1/chrome/renderer/extensions/e...
File chrome/renderer/extensions/enterprise_platform_keys_natives.h (right):

https://codereview.chromium.org/298073009/diff/1/chrome/renderer/extensions/e...
chrome/renderer/extensions/enterprise_platform_keys_natives.h:19: void
NormalizeAlgorithm(const v8::FunctionCallbackInfo<v8::Value>& call_info);
it's nice to document what the expected parameters are.

[pneubeck (no reviews), 2014-06-02 14:01:55]

https://codereview.chromium.org/298073009/diff/1/chrome/renderer/extensions/e...
File chrome/renderer/extensions/enterprise_platform_keys_natives.cc (right):

https://codereview.chromium.org/298073009/diff/1/chrome/renderer/extensions/e...
chrome/renderer/extensions/enterprise_platform_keys_natives.cc:48: default:
On 2014/05/29 15:06:57, kalman wrote:
> if you leave out the default case it will be a compile error. usually that is
> preferable, but it might complicate blink rolls.
> 
> just noting it in case it's important.

Done.

https://codereview.chromium.org/298073009/diff/1/chrome/renderer/extensions/e...
chrome/renderer/extensions/enterprise_platform_keys_natives.cc:58:
scoped_ptr<base::DictionaryValue> dict(new base::DictionaryValue);
On 2014/05/29 15:06:57, kalman wrote:
> is this method likely to get much more complicated? if not, you might be
better
> off saving the effort of the V8ValueConversion and setting the v8 API
directly;
> it's more verbose, but not that much more verbose.
> 
> up to you.

I expect it to become more complicated once we have to support more such
algorithm dictionaries.
Dictionary seems more convenient then.

https://codereview.chromium.org/298073009/diff/1/chrome/renderer/extensions/e...
chrome/renderer/extensions/enterprise_platform_keys_natives.cc:88:
DCHECK(call_info.Length() == 2);
On 2014/05/29 15:06:57, kalman wrote:
> DCHECK_EQ

Done.

https://codereview.chromium.org/298073009/diff/1/chrome/renderer/extensions/e...
chrome/renderer/extensions/enterprise_platform_keys_natives.cc:92: std::string
operation(*v8::String::Utf8Value(call_info[1]));
On 2014/05/29 15:06:57, kalman wrote:
> not used?

Now it is.

https://codereview.chromium.org/298073009/diff/1/chrome/renderer/extensions/e...
chrome/renderer/extensions/enterprise_platform_keys_natives.cc:105:
scoped_ptr<base::DictionaryValue> algorithmDict;
On 2014/05/29 15:06:57, kalman wrote:
> algorithm_dict

Done.

https://codereview.chromium.org/298073009/diff/1/chrome/renderer/extensions/e...
chrome/renderer/extensions/enterprise_platform_keys_natives.cc:110:
call_info.GetReturnValue().SetNull();
On 2014/05/29 15:06:57, kalman wrote:
> there are a couple of other ways you could fail here:
> - return undefined (i.e. never set the value)
> - throw an exception
> 
> returning null seems correct enough, so again, just noting it...

changed to the simpler undefined.
I have to extend this to throw an exception in a follow up (when I will use
DOMExceptions as WebCrypto does).

https://codereview.chromium.org/298073009/diff/1/chrome/renderer/extensions/e...
File chrome/renderer/extensions/enterprise_platform_keys_natives.h (right):

https://codereview.chromium.org/298073009/diff/1/chrome/renderer/extensions/e...
chrome/renderer/extensions/enterprise_platform_keys_natives.h:19: void
NormalizeAlgorithm(const v8::FunctionCallbackInfo<v8::Value>& call_info);
On 2014/05/29 15:06:57, kalman wrote:
> it's nice to document what the expected parameters are.

Done.

[pneubeck (no reviews), 2014-06-03 16:16:51]

On 2014/06/02 14:01:55, pneubeck wrote:
>
https://codereview.chromium.org/298073009/diff/1/chrome/renderer/extensions/e...
> File chrome/renderer/extensions/enterprise_platform_keys_natives.cc (right):
> 
>
https://codereview.chromium.org/298073009/diff/1/chrome/renderer/extensions/e...
> chrome/renderer/extensions/enterprise_platform_keys_natives.cc:48: default:
> On 2014/05/29 15:06:57, kalman wrote:
> > if you leave out the default case it will be a compile error. usually that
is
> > preferable, but it might complicate blink rolls.
> > 
> > just noting it in case it's important.
> 
> Done.
> 
>
https://codereview.chromium.org/298073009/diff/1/chrome/renderer/extensions/e...
> chrome/renderer/extensions/enterprise_platform_keys_natives.cc:58:
> scoped_ptr<base::DictionaryValue> dict(new base::DictionaryValue);
> On 2014/05/29 15:06:57, kalman wrote:
> > is this method likely to get much more complicated? if not, you might be
> better
> > off saving the effort of the V8ValueConversion and setting the v8 API
> directly;
> > it's more verbose, but not that much more verbose.
> > 
> > up to you.
> 
> I expect it to become more complicated once we have to support more such
> algorithm dictionaries.
> Dictionary seems more convenient then.
> 
>
https://codereview.chromium.org/298073009/diff/1/chrome/renderer/extensions/e...
> chrome/renderer/extensions/enterprise_platform_keys_natives.cc:88:
> DCHECK(call_info.Length() == 2);
> On 2014/05/29 15:06:57, kalman wrote:
> > DCHECK_EQ
> 
> Done.
> 
>
https://codereview.chromium.org/298073009/diff/1/chrome/renderer/extensions/e...
> chrome/renderer/extensions/enterprise_platform_keys_natives.cc:92: std::string
> operation(*v8::String::Utf8Value(call_info[1]));
> On 2014/05/29 15:06:57, kalman wrote:
> > not used?
> 
> Now it is.
> 
>
https://codereview.chromium.org/298073009/diff/1/chrome/renderer/extensions/e...
> chrome/renderer/extensions/enterprise_platform_keys_natives.cc:105:
> scoped_ptr<base::DictionaryValue> algorithmDict;
> On 2014/05/29 15:06:57, kalman wrote:
> > algorithm_dict
> 
> Done.
> 
>
https://codereview.chromium.org/298073009/diff/1/chrome/renderer/extensions/e...
> chrome/renderer/extensions/enterprise_platform_keys_natives.cc:110:
> call_info.GetReturnValue().SetNull();
> On 2014/05/29 15:06:57, kalman wrote:
> > there are a couple of other ways you could fail here:
> > - return undefined (i.e. never set the value)
> > - throw an exception
> > 
> > returning null seems correct enough, so again, just noting it...
> 
> changed to the simpler undefined.
> I have to extend this to throw an exception in a follow up (when I will use
> DOMExceptions as WebCrypto does).
> 
>
https://codereview.chromium.org/298073009/diff/1/chrome/renderer/extensions/e...
> File chrome/renderer/extensions/enterprise_platform_keys_natives.h (right):
> 
>
https://codereview.chromium.org/298073009/diff/1/chrome/renderer/extensions/e...
> chrome/renderer/extensions/enterprise_platform_keys_natives.h:19: void
> NormalizeAlgorithm(const v8::FunctionCallbackInfo<v8::Value>& call_info);
> On 2014/05/29 15:06:57, kalman wrote:
> > it's nice to document what the expected parameters are.
> 
> Done.

@Eric ping

[pneubeck (no reviews), 2014-06-04 12:43:20]

@Ryan: Eric seems to be busy or OOO. Could you take a look instead?

Thanks.

[Ryan Sleevi, 2014-06-04 18:41:17]

Eric is best qualified to review this. I am not.

[eroman, 2014-06-05 23:38:28]

(I had been waiting on the blink side before commenting more here)

https://codereview.chromium.org/298073009/diff/60001/chrome/renderer/extensio...
File chrome/renderer/extensions/enterprise_platform_keys_natives.cc (right):

https://codereview.chromium.org/298073009/diff/60001/chrome/renderer/extensio...
chrome/renderer/extensions/enterprise_platform_keys_natives.cc:21: std::string
AlgorithmIdToName(blink::WebCryptoAlgorithmId id) {
This is a duplication of algorithmIdToName(blink::WebCryptoAlgorithmId) in
NormalizeAlgorithm.h.

You can expose that instead (perhaps as a WebCryptoAlgorithm::idToName()
method?).

https://codereview.chromium.org/298073009/diff/60001/chrome/renderer/extensio...
chrome/renderer/extensions/enterprise_platform_keys_natives.cc:44: return
"RSASSA-PKCS1-V1_5";
Note that the canonicalized name used by the spec and elsewhere in webcrypto has
a lowercase v:
  RSASSA-PKCS1-v1_5

https://codereview.chromium.org/298073009/diff/60001/chrome/renderer/extensio...
chrome/renderer/extensions/enterprise_platform_keys_natives.cc:78:
rsaHashedKeyGen->modulusLengthBits());
What about the hash property?

I am not sure I understand the usage of normalizeCryptoAlgorithm here. Why is it
necessary to convert back to a javascript object. Can you instead call
normalizeCryptoAlgorithm at at point where it is last needed?

Is the issue that you need to do the conversion in the renderer, and are
converting it to a value to be sent to the browser here?

https://codereview.chromium.org/298073009/diff/60001/chrome/renderer/extensio...
chrome/renderer/extensions/enterprise_platform_keys_natives.cc:108:
blink::WebString error_details;
The exception information is being disregarded, is that intentional?

https://codereview.chromium.org/298073009/diff/60001/chrome/renderer/resource...
File
chrome/renderer/resources/extensions/enterprise_platform_keys/subtle_crypto.js
(right):

https://codereview.chromium.org/298073009/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/extensions/enterprise_platform_keys/subtle_crypto.js:91:
internalAPI.generateKey(subtleCrypto.tokenId,
where does the hash come into play? i don't see it either in the generation or
signing.

https://codereview.chromium.org/298073009/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/extensions/enterprise_platform_keys/subtle_crypto.js:114:
// TODO(pneubeck): It's not clear from the WebCrypto spec which error to
The methods you plumbed through set the exception code and message to use. Not
sure which ambiguity in the spec you are refering to, but if there is one please
file it at
https://www.w3.org/Bugs/Public/enter_bug.cgi?product=Web%20Cryptography&compo...

https://codereview.chromium.org/298073009/diff/60001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/enterprise_platform_keys/basic.js
(right):

https://codereview.chromium.org/298073009/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/enterprise_platform_keys/basic.js:379: new
Error('Error: A required parameter was missing our out-of-range'),
our --> or

https://codereview.chromium.org/298073009/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/enterprise_platform_keys/basic.js:398: new
Error('Error: A required parameter was missing our out-of-range'),
our --> or

[pneubeck (no reviews), 2014-06-06 12:39:46]

-Ryan
@Eric, ptal

https://codereview.chromium.org/298073009/diff/60001/chrome/renderer/extensio...
File chrome/renderer/extensions/enterprise_platform_keys_natives.cc (right):

https://codereview.chromium.org/298073009/diff/60001/chrome/renderer/extensio...
chrome/renderer/extensions/enterprise_platform_keys_natives.cc:21: std::string
AlgorithmIdToName(blink::WebCryptoAlgorithmId id) {
On 2014/06/05 23:38:28, eroman wrote:
> This is a duplication of algorithmIdToName(blink::WebCryptoAlgorithmId) in
> NormalizeAlgorithm.h.
> 
> You can expose that instead (perhaps as a WebCryptoAlgorithm::idToName()
> method?).

Done.

https://codereview.chromium.org/298073009/diff/60001/chrome/renderer/extensio...
chrome/renderer/extensions/enterprise_platform_keys_natives.cc:44: return
"RSASSA-PKCS1-V1_5";
On 2014/06/05 23:38:28, eroman wrote:
> Note that the canonicalized name used by the spec and elsewhere in webcrypto
has
> a lowercase v:
>   RSASSA-PKCS1-v1_5

Done.

https://codereview.chromium.org/298073009/diff/60001/chrome/renderer/extensio...
chrome/renderer/extensions/enterprise_platform_keys_natives.cc:78:
rsaHashedKeyGen->modulusLengthBits());
On 2014/06/05 23:38:27, eroman wrote:
> What about the hash property?
Like pulic exponent, I'd put that into a follow up.

> 
> I am not sure I understand the usage of normalizeCryptoAlgorithm here. Why is
it
> necessary to convert back to a javascript object. Can you instead call
> normalizeCryptoAlgorithm at at point where it is last needed?
> 
> Is the issue that you need to do the conversion in the renderer, and are
> converting it to a value to be sent to the browser here?
Exactly. We maybe could get rid of this conversion, if normalizeCryptoAlgorithm
wouldn't depend on V8 (e.g. base::Value instead of v8's Dictionary)

https://codereview.chromium.org/298073009/diff/60001/chrome/renderer/extensio...
chrome/renderer/extensions/enterprise_platform_keys_natives.cc:108:
blink::WebString error_details;
On 2014/06/05 23:38:27, eroman wrote:
> The exception information is being disregarded, is that intentional?

I wanted to do another pass to fix error handling in a follow up. E.g. it's
unclear whether I should also use DOMExceptions as these are reserved for Web
APIs, AFAIK.

https://codereview.chromium.org/298073009/diff/60001/chrome/renderer/resource...
File
chrome/renderer/resources/extensions/enterprise_platform_keys/subtle_crypto.js
(right):

https://codereview.chromium.org/298073009/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/extensions/enterprise_platform_keys/subtle_crypto.js:91:
internalAPI.generateKey(subtleCrypto.tokenId,
On 2014/06/05 23:38:28, eroman wrote:
> where does the hash come into play? i don't see it either in the generation or
> signing.
-> follow up CL.

https://codereview.chromium.org/298073009/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/extensions/enterprise_platform_keys/subtle_crypto.js:114:
// TODO(pneubeck): It's not clear from the WebCrypto spec which error to
On 2014/06/05 23:38:28, eroman wrote:
> The methods you plumbed through set the exception code and message to use. Not
> sure which ambiguity in the spec you are refering to, but if there is one
please
> file it at
>
https://www.w3.org/Bugs/Public/enter_bug.cgi?product=Web%20Cryptography&compo...

Yeah, this comment is not accurate.
a follow up CL will go over all errors / see other answer.

https://codereview.chromium.org/298073009/diff/60001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/enterprise_platform_keys/basic.js
(right):

https://codereview.chromium.org/298073009/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/enterprise_platform_keys/basic.js:379: new
Error('Error: A required parameter was missing our out-of-range'),
On 2014/06/05 23:38:28, eroman wrote:
> our --> or

Done.

https://codereview.chromium.org/298073009/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/enterprise_platform_keys/basic.js:398: new
Error('Error: A required parameter was missing our out-of-range'),
On 2014/06/05 23:38:28, eroman wrote:
> our --> or

Done.

[pneubeck (no reviews), 2014-06-06 12:40:11]

oops, removed the wrong mail.

[eroman, 2014-06-09 22:40:15]

lgtm

[pneubeck (no reviews), 2014-06-10 08:24:23]

The CQ bit was checked by pneubeck@chromium.org

[pneubeck (no reviews), 2014-06-10 08:24:35]

The CQ bit was unchecked by pneubeck@chromium.org

[pneubeck (no reviews), 2014-06-10 08:27:06]

@Kalman, friendly ping

[not at google - send to devlin, 2014-06-10 18:00:45]

lgtm

https://codereview.chromium.org/298073009/diff/80001/chrome/renderer/extensio...
File chrome/renderer/extensions/enterprise_platform_keys_natives.cc (right):

https://codereview.chromium.org/298073009/diff/80001/chrome/renderer/extensio...
chrome/renderer/extensions/enterprise_platform_keys_natives.cc:50: return
dict.Pass();
nit: looks like you return dict.Pass() either way, so you can remove this

[pneubeck (no reviews), 2014-06-11 12:49:09]

https://codereview.chromium.org/298073009/diff/80001/chrome/renderer/extensio...
File chrome/renderer/extensions/enterprise_platform_keys_natives.cc (right):

https://codereview.chromium.org/298073009/diff/80001/chrome/renderer/extensio...
chrome/renderer/extensions/enterprise_platform_keys_natives.cc:50: return
dict.Pass();
On 2014/06/10 18:00:44, kalman wrote:
> nit: looks like you return dict.Pass() either way, so you can remove this

Done.

[pneubeck (no reviews), 2014-06-11 12:49:40]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-06-11 12:52:55]

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

[pneubeck (no reviews), 2014-06-11 12:54:29]

The CQ bit was unchecked by pneubeck@chromium.org

[pneubeck (no reviews), 2014-06-16 17:56:54]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-06-16 17:59:33]

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

[commit-bot: I haz the power, 2014-06-16 21:44:31]

Change committed as 277542