Bug 164722 – Update SubtleCrypto::exportKey to match the latest spec

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jiewen Tan]

Update SubtleCrypto::exportKey to match the latest spec.

[Radar WebKit Bug Importer]

<rdar://problem/29251740>

[Jiewen Tan]

Created attachment 294732 [details]
Patch

[Brent Fulgham]

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()) {"

[Brent Fulgham]

Comment on attachment 294732 [details]
Patch

r- because the patch does not apply cleanly. Can you please address my comments and re-upload?

[Jiewen Tan]

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.

[Jiewen Tan]

Created attachment 294770 [details]
Patch

[WebKit Commit Bot]

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.

[Brent Fulgham]

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?

[Jiewen Tan]

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.

[Yusuke Suzuki]

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);

[Jiewen Tan]

Created attachment 294808 [details]
Patch for landing

[WebKit Commit Bot]

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.

[Jiewen Tan]

Committed r208737: <http://trac.webkit.org/changeset/208737>

[Yusuke Suzuki]

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"));`

[Jiewen Tan]

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"));`

[Jiewen Tan]

A followup patch is landed:
Committed r208751: <http://trac.webkit.org/changeset/208751>

[Yusuke Suzuki]

(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. :)

[Jiewen Tan]

(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>