Bug 164722 - Update SubtleCrypto::exportKey to match the latest spec
Summary: Update SubtleCrypto::exportKey to match the latest spec
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
Depends on:
Blocks: 160883
  Show dependency treegraph
 
Reported: 2016-11-14 12:06 PST by Jiewen Tan
Modified: 2016-11-17 12:42 PST (History)
10 users (show)

See Also:


Attachments
Patch (163.68 KB, patch)
2016-11-14 12:58 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (165.70 KB, patch)
2016-11-14 16:09 PST, Jiewen Tan
bfulgham: review+
Details | Formatted Diff | Diff
Patch for landing (165.70 KB, patch)
2016-11-14 21:35 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2016-11-14 12:06:53 PST
Update SubtleCrypto::exportKey to match the latest spec.
Comment 1 Radar WebKit Bug Importer 2016-11-14 12:10:43 PST
<rdar://problem/29251740>
Comment 2 Jiewen Tan 2016-11-14 12:58:03 PST
Created attachment 294732 [details]
Patch
Comment 3 Brent Fulgham 2016-11-14 13:24:33 PST
Comment on attachment 294732 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294732&action=review

> Source/WebCore/ChangeLog:12
> +           It also refers to the latest Editor's Draft at a certain degree:

"at a certain degree" --> "to a certain degree:"

> Source/WebCore/ChangeLog:14
> +        2. It implements exportKey operations of following algorithms: AES-CBC, AES-KW,

"... of THE following algorithms..."

> Source/WebCore/ChangeLog:19
> +        P.S. We currently only support Raw and Jwk key format.

"P.S." -> "Note: "

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:352
> +            JSObject* info = constructEmptyObject(&state);

Could be auto* here. Can 'constructEmptyObject' return nullptr?

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:358
> +        result->putDirect(vm, Identifier::fromString(&vm, "oth"), constructArray(&state, static_cast<Structure*>(0), list));

I think this should be "static_cast<Structure*>(nullptr)"

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:524
> +    // The spec suggests we should perform the following task asynchronously as of 11 December 2014:

Unless the specification is requiring asynchronous handling to only happen on 11 December 2014 and beyond, I think you want to phrase it:

"// The 11 December 2014 version of the specification suggests we should perform the following task asynchronously:"

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:526
> +    // That's simply not necessary. Therefore, we perform it synchronously.

I'm not sure I understand this comment. Why is it not necessary?

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:152
> +            jwk.alg = String(ALG128);

Is this costly? Should we have defined ALG128 (and others) as ASCIILiterals?

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:151
> +            jwk.alg = String(ALG128);

Ditto my earlier comments about ASCIILiteral?

> Source/WebCore/crypto/algorithms/CryptoAlgorithmHMAC.cpp:169
> +            jwk.alg = String(ALG1);

Again (ASCIILiteral)?

> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:42
> +static const char* const ALG = "RSA1_5";

Would it be better if each of these classes had a static method "algorithmName()" or something that returned this value as an ASCIILiteral?

> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:106
> +        if (key.alg && key.alg.value() != ALG) {

.. this this would be "if (key.alg && key.alg.value() != algorithmName()) {"
Comment 4 Brent Fulgham 2016-11-14 13:25:12 PST
Comment on attachment 294732 [details]
Patch

r- because the patch does not apply cleanly. Can you please address my comments and re-upload?
Comment 5 Jiewen Tan 2016-11-14 15:44:49 PST
Comment on attachment 294732 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294732&action=review

First of all, thanks Brent for reviewing my patch.

>> Source/WebCore/ChangeLog:12
>> +           It also refers to the latest Editor's Draft at a certain degree:
> 
> "at a certain degree" --> "to a certain degree:"

Fixed.

>> Source/WebCore/ChangeLog:14
>> +        2. It implements exportKey operations of following algorithms: AES-CBC, AES-KW,
> 
> "... of THE following algorithms..."

Fixed.

>> Source/WebCore/ChangeLog:19
>> +        P.S. We currently only support Raw and Jwk key format.
> 
> "P.S." -> "Note: "

Fixed.

>> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:352
>> +            JSObject* info = constructEmptyObject(&state);
> 
> Could be auto* here. Can 'constructEmptyObject' return nullptr?

Fixed. Should not be able.

>> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:358
>> +        result->putDirect(vm, Identifier::fromString(&vm, "oth"), constructArray(&state, static_cast<Structure*>(0), list));
> 
> I think this should be "static_cast<Structure*>(nullptr)"

Fixed.

>> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:524
>> +    // The spec suggests we should perform the following task asynchronously as of 11 December 2014:
> 
> Unless the specification is requiring asynchronous handling to only happen on 11 December 2014 and beyond, I think you want to phrase it:
> 
> "// The 11 December 2014 version of the specification suggests we should perform the following task asynchronously:"

Fixed.

>> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:526
>> +    // That's simply not necessary. Therefore, we perform it synchronously.
> 
> I'm not sure I understand this comment. Why is it not necessary?

The spec suggests all methods should be performed asynchronously. This suggestions, in my opinion, is beneficial only for time consuming operations like RSA key generation. Such that, they will not block the UI. For other operations, however, it will add extra complexity/time to the codes, which is not beneficial.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:152
>> +            jwk.alg = String(ALG128);
> 
> Is this costly? Should we have defined ALG128 (and others) as ASCIILiterals?

Should be okay.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:151
>> +            jwk.alg = String(ALG128);
> 
> Ditto my earlier comments about ASCIILiteral?

Ditto.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmHMAC.cpp:169
>> +            jwk.alg = String(ALG1);
> 
> Again (ASCIILiteral)?

Ditto.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:42
>> +static const char* const ALG = "RSA1_5";
> 
> Would it be better if each of these classes had a static method "algorithmName()" or something that returned this value as an ASCIILiteral?

I think it will be better if we keep these variables visible only in this cpp at least at this moment.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:106
>> +        if (key.alg && key.alg.value() != ALG) {
> 
> .. this this would be "if (key.alg && key.alg.value() != algorithmName()) {"

Ditto.
Comment 6 Jiewen Tan 2016-11-14 16:09:32 PST
Created attachment 294770 [details]
Patch
Comment 7 WebKit Commit Bot 2016-11-14 16:11:58 PST
Attachment 294770 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:160:  CryptoAlgorithmRSA_OAEP::exportKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:120:  CryptoAlgorithmAES_KW::exportKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:149:  CryptoAlgorithmRSASSA_PKCS1_v1_5::exportKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:116:  CryptoAlgorithmRSAES_PKCS1_v1_5::exportKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:124:  CryptoAlgorithmAES_CBC::exportKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 5 in 101 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Brent Fulgham 2016-11-14 16:32:50 PST
Comment on attachment 294770 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294770&action=review

r=me

> Source/WebCore/crypto/keys/CryptoKeyAES.h:79
> +    Vector<uint8_t> exportRaw() const { return m_key; }

Is this really necessary? Couldn't someone who wants the raw key bytes just use the "key" method and assign it to their own mutable variable?
Comment 9 Jiewen Tan 2016-11-14 17:00:20 PST
Comment on attachment 294770 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294770&action=review

>> Source/WebCore/crypto/keys/CryptoKeyAES.h:79
>> +    Vector<uint8_t> exportRaw() const { return m_key; }
> 
> Is this really necessary? Couldn't someone who wants the raw key bytes just use the "key" method and assign it to their own mutable variable?

Haven't consider this possibility. Fixed.
Comment 10 Yusuke Suzuki 2016-11-14 17:13:11 PST
Comment on attachment 294770 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294770&action=review

Quick comment.

> LayoutTests/crypto/subtle/hmac-export-key-malformed-parameters.html:26
> +crypto.subtle.generateKey({name: "hmac", hash: "sha-1"}, extractable, ["sign", "verify"]).then(function(result) {
> +    key = result;
> +
> +    // Not support format.
> +    shouldReject('crypto.subtle.exportKey("spki", key)');
> +    shouldReject('crypto.subtle.exportKey("pkcs8", key)');
> +
> +    finishJSTest();
> +});

In that case, I think we can rewrite this test like,

crypto.subtle.generateKey({name: "hmac", hash: "sha-1"}, extractable, ["sign", "verify"]).then(function(result) {
    key = result;

     // Not support format.
    return shouldReject('crypto.subtle.exportKey("spki", key)');
}).then(function () {
    return shouldReject('crypto.subtle.exportKey("pkcs8", key)');
}).then(finishJSTest, finishJSTest);
Comment 11 Jiewen Tan 2016-11-14 21:35:26 PST
Created attachment 294808 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2016-11-14 21:39:15 PST
Attachment 294808 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:160:  CryptoAlgorithmRSA_OAEP::exportKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:120:  CryptoAlgorithmAES_KW::exportKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:149:  CryptoAlgorithmRSASSA_PKCS1_v1_5::exportKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:116:  CryptoAlgorithmRSAES_PKCS1_v1_5::exportKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:124:  CryptoAlgorithmAES_CBC::exportKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 5 in 101 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Jiewen Tan 2016-11-15 11:08:38 PST
Committed r208737: <http://trac.webkit.org/changeset/208737>
Comment 14 Yusuke Suzuki 2016-11-15 11:23:12 PST
Comment on attachment 294808 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=294808&action=review

Post review, r=me too. You can just land the follow-up patch.

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:475
> +        promise->reject<JSValue>(createTypeError(&state, ASCIILiteral("Invalid CryptoKey")));

You can use `promise->reject(TypeError, ASCIILiteral("Invalid CryptoKey"));`.

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:493
> +        promise->reject<JSValue>(createDOMException(&state, NOT_SUPPORTED_ERR, ASCIILiteral("The operation is not supported")));

You can write `promise->reject(NOT_SUPPORTED_ERR, ASCIILiteral("The operation is not supported"));`

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:498
> +        promise->reject<JSValue>(createDOMException(&state, INVALID_ACCESS_ERR, ASCIILiteral("The CryptoKey is nonextractable")));

You can write `promise->reject(INVALID_ACCESS_ERR, ASCIILiteral("The CryptoKey is nonextractable"));`
Comment 15 Jiewen Tan 2016-11-15 11:34:33 PST
Thanks, Yusuke. Working on a follow up patch now.

(In reply to comment #14)
> Comment on attachment 294808 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=294808&action=review
> 
> Post review, r=me too. You can just land the follow-up patch.
> 
> > Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:475
> > +        promise->reject<JSValue>(createTypeError(&state, ASCIILiteral("Invalid CryptoKey")));
> 
> You can use `promise->reject(TypeError, ASCIILiteral("Invalid CryptoKey"));`.
> 
> > Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:493
> > +        promise->reject<JSValue>(createDOMException(&state, NOT_SUPPORTED_ERR, ASCIILiteral("The operation is not supported")));
> 
> You can write `promise->reject(NOT_SUPPORTED_ERR, ASCIILiteral("The
> operation is not supported"));`
> 
> > Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:498
> > +        promise->reject<JSValue>(createDOMException(&state, INVALID_ACCESS_ERR, ASCIILiteral("The CryptoKey is nonextractable")));
> 
> You can write `promise->reject(INVALID_ACCESS_ERR, ASCIILiteral("The
> CryptoKey is nonextractable"));`
Comment 16 Jiewen Tan 2016-11-15 13:49:07 PST
A followup patch is landed:
Committed r208751: <http://trac.webkit.org/changeset/208751>
Comment 17 Yusuke Suzuki 2016-11-15 13:56:43 PST
(In reply to comment #16)
> A followup patch is landed:
> Committed r208751: <http://trac.webkit.org/changeset/208751>

I think that the second change should not be TypeError. NOT_SUPPORTED_ERR. :)
Comment 18 Jiewen Tan 2016-11-15 14:26:22 PST
(In reply to comment #17)
> (In reply to comment #16)
> > A followup patch is landed:
> > Committed r208751: <http://trac.webkit.org/changeset/208751>
> 
> I think that the second change should not be TypeError. NOT_SUPPORTED_ERR. :)

Oops! A quick fix: Committed r208757: <http://trac.webkit.org/changeset/208757>