enterprise.platformKeys: Respect the 'hash' argument of generateKey.

enterprise.platformKeys: Respect the 'hash' argument of generateKey.

Before, the hash argument was simply ignored and defaulted to SHA-1. Instead it accepts now all values specified by WebCrypto, i.e. SHA-{1,256,384,512}.

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

[pneubeck (no reviews), 2014-06-16 14:18:12]

@Eric, please take a look for WebCrypto compatibility.

@Benjamin, I need your rubber stamp for (no principal change this time):
chrome/common/extensions/api/enterprise_platform_keys_internal.idl 
chrome/renderer/extensions/enterprise_platform_keys_natives.cc

[not at google - send to devlin, 2014-06-16 17:05:32]

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

https://codereview.chromium.org/331173002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:343: sign_alg_tag =
SEC_OID_PKCS1_SHA1_WITH_RSA_ENCRYPTION;
can you just pass in this enum rather than parsing it?

https://codereview.chromium.org/331173002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/enterprise_platform_keys_internal.idl (right):

https://codereview.chromium.org/331173002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/enterprise_platform_keys_internal.idl:42: //    
SHA-{1,256,384,512}.
make it an enum?

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

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

https://codereview.chromium.org/331173002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:343: sign_alg_tag =
SEC_OID_PKCS1_SHA1_WITH_RSA_ENCRYPTION;
On 2014/06/16 17:05:31, kalman wrote:
> can you just pass in this enum rather than parsing it?

SECOidTag is NSS specific and can't be moved to the header.
I added an enum specific to platform_keys, which makes this implementation at
least independent of these particular strings.

https://codereview.chromium.org/331173002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/enterprise_platform_keys_internal.idl (right):

https://codereview.chromium.org/331173002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/enterprise_platform_keys_internal.idl:42: //    
SHA-{1,256,384,512}.
On 2014/06/16 17:05:32, kalman wrote:
> make it an enum?

This is a property forwarded from the Key.algorithm dictionary. I don't want to
translate these values and instead forward them without modification. (added a
comment for that)

I could still add an enum, _if_ the current idl_parser would correctly support
arbitrary strings for enum values. But it parses them as values and thus
interprets the '-'.

I'll post a bug for that, and if fixed I can add an enum here.

[not at google - send to devlin, 2014-06-17 18:18:48]

The code to change is here:
https://code.google.com/p/chromium/codesearch#chromium/src/tools/json_schema_...

I think it's a 1-line change.

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

https://codereview.chromium.org/331173002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:343: sign_alg_tag =
SEC_OID_PKCS1_SHA1_WITH_RSA_ENCRYPTION;
On 2014/06/17 09:12:17, pneubeck wrote:
> On 2014/06/16 17:05:31, kalman wrote:
> > can you just pass in this enum rather than parsing it?
> 
> SECOidTag is NSS specific and can't be moved to the header.
> I added an enum specific to platform_keys, which makes this implementation at
> least independent of these particular strings.

I see. If the schema compiler issue were fixed to generate enums properly then
you could pass those in right?

https://codereview.chromium.org/331173002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/enterprise_platform_keys_internal.idl (right):

https://codereview.chromium.org/331173002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/enterprise_platform_keys_internal.idl:42: //    
SHA-{1,256,384,512}.
On 2014/06/17 09:12:17, pneubeck wrote:
> On 2014/06/16 17:05:32, kalman wrote:
> > make it an enum?
> 
> This is a property forwarded from the Key.algorithm dictionary. I don't want
to
> translate these values and instead forward them without modification. (added a
> comment for that)
> 
> I could still add an enum, _if_ the current idl_parser would correctly support
> arbitrary strings for enum values. But it parses them as values and thus
> interprets the '-'.
> 
> I'll post a bug for that, and if fixed I can add an enum here.

Ah, right. That seems like a schema compiler problem and should be pretty easy
to fix actually, like, convert - to _ in the enum names. The string value
conversions should stay the same.

[eroman, 2014-06-17 18:25:22]

LGTM after responding to
https://codereview.chromium.org/331173002/diff/80001/chrome/renderer/resource...

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

https://codereview.chromium.org/331173002/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/enterprise_platform_keys_natives.cc:66:
hash_dict->SetStringWithoutPathExpansion("name", hash_info->name);
isn't there a more direct id to name function? (rather than looking up and then
hash_info->name).

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

https://codereview.chromium.org/331173002/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/extensions/enterprise_platform_keys/subtle_crypto.js:146:
key.algorithm.hash.name,
Are users able to manipulate key.algorith.hash.name? Without your other freezing
change, this would then allow using the key with different hashes, which is not
desirable for the API.

To avoid this, can the hash instead be stored in the table that maps the public
key? I am not sure that it is really possible to prevent key.algorithm.hash.name
from being manipulated.

[pneubeck (no reviews), 2014-06-17 18:31:31]

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

https://codereview.chromium.org/331173002/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/extensions/enterprise_platform_keys/subtle_crypto.js:146:
key.algorithm.hash.name,
On 2014/06/17 18:25:22, eroman wrote:
> Are users able to manipulate key.algorith.hash.name? Without your other
freezing
> change, this would then allow using the key with different hashes, which is
not
> desirable for the API.
> 
> To avoid this, can the hash instead be stored in the table that maps the
public
> key? I am not sure that it is really possible to prevent
key.algorithm.hash.name
> from being manipulated.
I'll update the other CL now, which will copy the whole dictionary on each
access.

[not at google - send to devlin, 2014-06-17 19:58:52]

lgtm

[pneubeck (no reviews), 2014-06-18 18:01:30]

The CQ bit was checked by pneubeck@chromium.org

[pneubeck (no reviews), 2014-06-18 18:01:37]

The CQ bit was unchecked by pneubeck@chromium.org

[pneubeck (no reviews), 2014-06-18 22:38:42]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-06-18 22:39:36]

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

[commit-bot: I haz the power, 2014-06-19 07:00:59]

Change committed as 278293