Bug 164722

<rdar://problem/29251740>

Created attachment 294732 [details] Patch

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 on attachment 294732 [details] Patch r- because the patch does not apply cleanly. Can you please address my comments and re-upload?

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.

Created attachment 294770 [details] Patch

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

Created attachment 294808 [details] Patch for landing

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.

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

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

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

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

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

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