chrome.platformKeys: Require a algorithm name to getKeyPair().

chrome.platformKeys: Require a algorithm name to getKeyPair().

This makes the algorithm name a mandatory argument of getKeyPair().
In a later CL, this name will be compared with the key restrictions that the certificate puts onto the key usage.

BUG=466584
Committed: https://crrev.com/c44add2d43f452684d512e5fb298518f75137bfd
Cr-Commit-Position: refs/heads/master@{#321956}

[pneubeck (no reviews), 2015-03-12 13:47:36]

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

[pneubeck (no reviews), 2015-03-12 13:47:37]

ptal

[pneubeck (no reviews), 2015-03-16 17:43:54]

pneubeck@chromium.org changed reviewers:
+ atwilson@chromium.org
- bartfab@chromium.org

[pneubeck (no reviews), 2015-03-16 17:43:54]

changing reviewer.
ptal.

[Andrew T Wilson (Slow), 2015-03-19 07:31:50]

Sorry about the delay. LGTM as-is, but consider addressing my concerns either
here or in a followup CL because normalizeAlgorithm() seems a bit fragile now.

https://codereview.chromium.org/1001853003/diff/1/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/platform_keys/get_public_key.js
(right):

https://codereview.chromium.org/1001853003/diff/1/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/platform_keys/get_public_key.js:12: throw
new Error('Algorithm: name: Missing or not a string');
I guess some other code is checking if the supplied name was a string? Because
this code doesn't check this (will happily accept importParams.name === 1.

https://codereview.chromium.org/1001853003/diff/1/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/platform_keys/get_public_key.js:20:
importParams.hash.name.toLowerCase() === 'none') {
SO, if I pass in :

importParams.hash = { name: 'none', foobar: 'philipp' }

This code will happily pass this through and change it to {name: 'none'}

Is that really what you want? I wonder if what you should do instead is:

if (hash.name.toLowerCase() === 'none') {
  hashIsNone = true;
  hash.name = 'SHA-1';
}

normalizeAlgorithm(...)
if (hashIsNone) {
   hash.name = 'none';
}

That way you still get normalization/rejection of any extraneous attributes?

https://codereview.chromium.org/1001853003/diff/1/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/platform_keys/get_public_key.js:20:
importParams.hash.name.toLowerCase() === 'none') {
Also, note that there's kind of an implicit contract in this function that the
passed-in importParams object won't be modified. But if you pass in hash =
{name: none} this contract is violated.

Can you copy importParams before modifying it and just modify the copy?

[pneubeck (no reviews), 2015-03-23 13:13:28]

Patchset #3 (id:40001) has been deleted

[pneubeck (no reviews), 2015-03-23 13:20:08]

Patchset #2 (id:20001) has been deleted

[pneubeck (no reviews), 2015-03-23 13:21:00]

https://codereview.chromium.org/1001853003/diff/1/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/platform_keys/get_public_key.js
(right):

https://codereview.chromium.org/1001853003/diff/1/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/platform_keys/get_public_key.js:12: throw
new Error('Algorithm: name: Missing or not a string');
On 2015/03/19 07:31:49, Andrew T Wilson wrote:
> I guess some other code is checking if the supplied name was a string? Because
> this code doesn't check this (will happily accept importParams.name === 1.

Done.

https://codereview.chromium.org/1001853003/diff/1/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/platform_keys/get_public_key.js:20:
importParams.hash.name.toLowerCase() === 'none') {
On 2015/03/19 07:31:49, Andrew T Wilson wrote:
> SO, if I pass in :
> 
> importParams.hash = { name: 'none', foobar: 'philipp' }
> 
> This code will happily pass this through and change it to {name: 'none'}
> 
> Is that really what you want? I wonder if what you should do instead is:
> 
> if (hash.name.toLowerCase() === 'none') {
>   hashIsNone = true;
>   hash.name = 'SHA-1';
> }
> 
> normalizeAlgorithm(...)
> if (hashIsNone) {
>    hash.name = 'none';
> }
> 
> That way you still get normalization/rejection of any extraneous attributes?

yes. extraneous attributes shall be removed. I added documentation for that.

https://codereview.chromium.org/1001853003/diff/1/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/platform_keys/get_public_key.js:20:
importParams.hash.name.toLowerCase() === 'none') {
On 2015/03/19 07:31:49, Andrew T Wilson wrote:
> Also, note that there's kind of an implicit contract in this function that the
> passed-in importParams object won't be modified. But if you pass in hash =
> {name: none} this contract is violated.
> 
> Can you copy importParams before modifying it and just modify the copy?

Done.

[pneubeck (no reviews), 2015-03-23 13:38:19]

Patchset #2 (id:60001) has been deleted

[pneubeck (no reviews), 2015-03-24 08:22:57]

The CQ bit was checked by pneubeck@chromium.org

[pneubeck (no reviews), 2015-03-24 08:22:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
atwilson@chromium.org
Link to the patchset: https://codereview.chromium.org/1001853003/#ps80001
(title: "Addressed comments.")

[commit-bot: I haz the power, 2015-03-24 08:23:08]

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

[commit-bot: I haz the power, 2015-03-24 08:52:20]

Committed patchset #2 (id:80001)

[commit-bot: I haz the power, 2015-03-24 08:53:57]

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