Bug 167026 – [WebCrypto] remove toJSValueFromJsonWebKey from custom SubtleCrypto binding codes

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

[Jiewen Tan]

Remove toJSValueFromJsonWebKey from custom SubtleCrypto binding codes.

[Jiewen Tan]

Created attachment 298801 [details]
Patch

[Sam Weinig]

Comment on attachment 298801 [details]
Patch

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

> Source/WebCore/ChangeLog:36
> +        * bindings/js/JSSubtleCryptoCustom.cpp:
> +        (WebCore::jsSubtleCryptoFunctionExportKeyPromise):
> +        (WebCore::jsSubtleCryptoFunctionWrapKeyPromise):
> +        (WebCore::toJSValueFromJsonWebKey): Deleted.
> +        * crypto/JsonWebKey.h:
> +        * crypto/JsonWebKey.idl:
> +        * crypto/RsaOtherPrimesInfo.idl:
> +        * crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:
> +        (WebCore::CryptoAlgorithmAES_CBC::importKey):
> +        * crypto/algorithms/CryptoAlgorithmAES_KW.cpp:
> +        (WebCore::CryptoAlgorithmAES_KW::importKey):
> +        * crypto/algorithms/CryptoAlgorithmHMAC.cpp:
> +        (WebCore::CryptoAlgorithmHMAC::importKey):
> +        * crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:
> +        (WebCore::CryptoAlgorithmRSAES_PKCS1_v1_5::importKey):
> +        * crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:
> +        (WebCore::CryptoAlgorithmRSASSA_PKCS1_v1_5::importKey):
> +        * crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:
> +        (WebCore::CryptoAlgorithmRSA_OAEP::importKey):
> +        * crypto/keys/CryptoKeyAES.cpp:
> +        (WebCore::CryptoKeyAES::importJwk):
> +        * crypto/keys/CryptoKeyAES.h:
> +        * crypto/keys/CryptoKeyHMAC.cpp:
> +        (WebCore::CryptoKeyHMAC::importJwk):
> +        * crypto/keys/CryptoKeyHMAC.h:
> +        * crypto/keys/CryptoKeyRSA.cpp:
> +        (WebCore::CryptoKeyRSA::importJwk):

Please add more information to this ChangeLog.

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:906
> +            auto jwk = toJS<IDLDictionary<JsonWebKey>>(*(promise->globalObject()->globalExec()), *(promise->globalObject()), WTFMove(WTF::get<JsonWebKey>(key)));
>              String jwkString = JSONStringify(promise->globalObject()->globalExec(), jwk, 0);
>              CString jwkUtf8String = jwkString.utf8(StrictConversion);
>              bytes.append(jwkUtf8String.data(), jwkUtf8String.length());

It seems unfortunate that we have to convert to a JSValue, only to call JSONStringify on it. Seems like in the future, this should be optimized to have a JSONStringification directly from the JsonWebKey.

> Source/WebCore/crypto/keys/CryptoKeyAES.h:71
> +    using CheckAlgCallback = WTF::Function<bool(size_t, const String&)>;

I don't think the WTF:: is required here.

> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:67
> +    using CheckAlgCallback = WTF::Function<bool(CryptoAlgorithmIdentifier, const String&)>;

No need for WTF::

> LayoutTests/ChangeLog:10
> +        * crypto/subtle/aes-cbc-import-key-wrap-jwk-rsa-key-private.html:
> +        * crypto/subtle/aes-cbc-import-key-wrap-jwk-rsa-key-public.html:
> +        * crypto/workers/subtle/resources/aes-cbc-import-key-wrap-key.js:

Why are these changing?

[Jiewen Tan]

Comment on attachment 298801 [details]
Patch

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

Thanks Sam for reviewing my patch.

>> Source/WebCore/ChangeLog:36
>> +        (WebCore::CryptoKeyRSA::importJwk):
> 
> Please add more information to this ChangeLog.

Fixed.

>> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:906
>>              bytes.append(jwkUtf8String.data(), jwkUtf8String.length());
> 
> It seems unfortunate that we have to convert to a JSValue, only to call JSONStringify on it. Seems like in the future, this should be optimized to have a JSONStringification directly from the JsonWebKey.

Sure.

>> Source/WebCore/crypto/keys/CryptoKeyAES.h:71
>> +    using CheckAlgCallback = WTF::Function<bool(size_t, const String&)>;
> 
> I don't think the WTF:: is required here.

Fixed.

>> Source/WebCore/crypto/keys/CryptoKeyHMAC.h:67
>> +    using CheckAlgCallback = WTF::Function<bool(CryptoAlgorithmIdentifier, const String&)>;
> 
> No need for WTF::

Fixed.

>> LayoutTests/ChangeLog:10
>> +        * crypto/workers/subtle/resources/aes-cbc-import-key-wrap-key.js:
> 
> Why are these changing?

Because the order of the attributes is different between toJS<IDLDictionary<JsonWebKey>> and the removed toJSValueFromJsonWebKey.

[Jiewen Tan]

Created attachment 300962 [details]
Patch

[Jiewen Tan]

Created attachment 301105 [details]
Patch

[Darin Adler]

Comment on attachment 301105 [details]
Patch

GTK build fails to link:

lib/libWebCoreDerivedSources.a(lib/../Source/WebCore/CMakeFiles/WebCoreDerivedSources.dir/__/__/DerivedSources/WebCore/JSJsonWebKey.cpp.o):JSJsonWebKey.cpp:function WebCore::convertDictionaryToJS(JSC::ExecState&, WebCore::JSDOMGlobalObject&, WebCore::JsonWebKey const&): error: undefined reference to 'WebCore::convertDictionaryToJS(JSC::ExecState&, WebCore::JSDOMGlobalObject&, WebCore::RsaOtherPrimesInfo const&)'

Need to figure out how to resolve this.

[Fujii Hironori]

Created attachment 301325 [details]
My build log of GTK+ port

I tried following, but succeeded to build. I'm not using ccache.

> ./Tools/Scripts/build-webkit --gtk --release --64-bit
> ./Tools/Scripts/webkit-patch apply-from-bug --no-update --port gtk 167026
> ./Tools/Scripts/build-webkit --gtk --release --64-bit

Looking at the build log of EWS.
JsonWebKey.idl and RsaOtherPrimesInfo.idl were recompiled.
But, only JSJsonWebKey.cpp.o was recompiled.

> [1/2] JsonWebKey.idl
> [2/2] RsaOtherPrimesInfo.idl
> [6/136] Building CXX object Source/WebCore/CMakeFiles/WebCoreDerivedSources.dir/__/__/DerivedSources/WebCore/JSJsonWebKey.cpp.o

[Jiewen Tan]

(In reply to comment #7)
> Created attachment 301325 [details]
> My build log of GTK+ port
> 
> I tried following, but succeeded to build. I'm not using ccache.
> 
> > ./Tools/Scripts/build-webkit --gtk --release --64-bit
> > ./Tools/Scripts/webkit-patch apply-from-bug --no-update --port gtk 167026
> > ./Tools/Scripts/build-webkit --gtk --release --64-bit
> 
> Looking at the build log of EWS.
> JsonWebKey.idl and RsaOtherPrimesInfo.idl were recompiled.
> But, only JSJsonWebKey.cpp.o was recompiled.
> 
> > [1/2] JsonWebKey.idl
> > [2/2] RsaOtherPrimesInfo.idl
> > [6/136] Building CXX object Source/WebCore/CMakeFiles/WebCoreDerivedSources.dir/__/__/DerivedSources/WebCore/JSJsonWebKey.cpp.o

Are you suggesting that there could be something wrong with the buildbot? I will upload the same patch again to see if the buildbot is fixed.

[Jiewen Tan]

Created attachment 301360 [details]
Patch

[Jiewen Tan]

Just talked to GTK people. They say we can ignore failures on the GTK port for now. They will fix the issue after this patch is landed.

[Carlos Alberto Lopez Perez]

For the record: I have checked the patch builds fine on the GTK+ port.

It seems to be an incremental build issue.

If the GTK+ build breaks after this lands, I think is a matter of triggering clean builds on the GTK+ bots.

[Jiewen Tan]

Created attachment 301650 [details]
Patch

[Chris Dumez]

Comment on attachment 301650 [details]
Patch

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

> LayoutTests/ChangeLog:8
> +        * crypto/subtle/aes-cbc-import-key-wrap-jwk-rsa-key-private.html:

Can you explain these changes in the ChangeLog? I have no idea why these were modified.

[Jiewen Tan]

Comment on attachment 301650 [details]
Patch

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

>> LayoutTests/ChangeLog:8
>> +        * crypto/subtle/aes-cbc-import-key-wrap-jwk-rsa-key-private.html:
> 
> Can you explain these changes in the ChangeLog? I have no idea why these were modified.

Sure.

[Chris Dumez]

Comment on attachment 301650 [details]
Patch

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

> Source/WebCore/crypto/JsonWebKey.h:40
>      std::optional<Vector<CryptoKeyUsage>> key_ops;

Does this really need to be optional?

> Source/WebCore/crypto/JsonWebKey.h:57
>      std::optional<Vector<RsaOtherPrimesInfo>> oth;

Does this really need to be optional?

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:126
> +                return alg.isEmpty() || alg == ALG128;

If the JS passes "", should it be treated the same way as if the member was omitted in the dictionary? Because isNull() means member was missing. isEmpty() means member was missing or the empty string.

[Jiewen Tan]

Comment on attachment 301650 [details]
Patch

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

Thanks Chris for reviewing my patch.

>> Source/WebCore/crypto/JsonWebKey.h:40
>>      std::optional<Vector<CryptoKeyUsage>> key_ops;
> 
> Does this really need to be optional?

It does make a  difference.

>> Source/WebCore/crypto/JsonWebKey.h:57
>>      std::optional<Vector<RsaOtherPrimesInfo>> oth;
> 
> Does this really need to be optional?

Ditto.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:126
>> +                return alg.isEmpty() || alg == ALG128;
> 
> If the JS passes "", should it be treated the same way as if the member was omitted in the dictionary? Because isNull() means member was missing. isEmpty() means member was missing or the empty string.

Before I think we should treat it in the same way. After revisiting the spec, I think we should not. Thanks for pointing out.

[Jiewen Tan]

Created attachment 301699 [details]
Patch

[Build Bot]

Comment on attachment 301699 [details]
Patch

Attachment 301699 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3126352

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 301704 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 301699 [details]
Patch

Attachment 301699 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3126333

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 301705 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 301699 [details]
Patch

Attachment 301699 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3126349

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 301706 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 301699 [details]
Patch

Attachment 301699 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3126465

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 301710 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Jiewen Tan]

Created attachment 301791 [details]
Patch

[Chris Dumez]

Comment on attachment 301791 [details]
Patch

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

> Source/WebCore/crypto/keys/CryptoKeyAES.cpp:99
> +    if (keyData.key_ops && ((keyData.usages & usages) != usages))

This is a behavior change. Please explain in the changelog why this is the right thing to do.

> Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:136
> +    if (keyData.type() == CryptoKeyDataRSAComponents::Type::Private && !keyData.firstPrimeInfo().primeFactor.size())

Please use keyData.firstPrimeInfo().primeFactor.isEmpty() instead of !keyData.firstPrimeInfo().primeFactor.size(). This is clearer.

> LayoutTests/ChangeLog:7
> +

This changelog still does not explain the changes. In particular, the changes to expected wrapped keys deserve an explanation.

[Jiewen Tan]

Comment on attachment 301791 [details]
Patch

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

>> Source/WebCore/crypto/keys/CryptoKeyAES.cpp:99
>> +    if (keyData.key_ops && ((keyData.usages & usages) != usages))
> 
> This is a behavior change. Please explain in the changelog why this is the right thing to do.

Sure.

>> Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:136
>> +    if (keyData.type() == CryptoKeyDataRSAComponents::Type::Private && !keyData.firstPrimeInfo().primeFactor.size())
> 
> Please use keyData.firstPrimeInfo().primeFactor.isEmpty() instead of !keyData.firstPrimeInfo().primeFactor.size(). This is clearer.

Fixed.

>> LayoutTests/ChangeLog:7
>> +
> 
> This changelog still does not explain the changes. In particular, the changes to expected wrapped keys deserve an explanation.

It explains after the tests.

> LayoutTests/ChangeLog:10
> +        Order of attributes inside JWK is different after this patch.

Here is the explanation.

[Jiewen Tan]

Created attachment 301802 [details]
Patch

[Chris Dumez]

Comment on attachment 301802 [details]
Patch

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

r=me

> Source/WebCore/ChangeLog:32
> +        We should check for if key_ops presents or not instead of usages, as value [ ] for

I still think this is not clear. I would say:
Only check if key_ops contains all of the specified usages when key_ops field of jwk is present, as per the specification).

> LayoutTests/ChangeLog:10
> +        Order of attributes inside JWK is different after this patch.

Maybe add a blank line after this comment so it is not as easy to miss.

[Jiewen Tan]

Comment on attachment 301802 [details]
Patch

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

Thanks Chris for r+ my patch.

>> Source/WebCore/ChangeLog:32
>> +        We should check for if key_ops presents or not instead of usages, as value [ ] for
> 
> I still think this is not clear. I would say:
> Only check if key_ops contains all of the specified usages when key_ops field of jwk is present, as per the specification).

Fixed.

>> LayoutTests/ChangeLog:10
>> +        Order of attributes inside JWK is different after this patch.
> 
> Maybe add a blank line after this comment so it is not as easy to miss.

Fixed.

[Jiewen Tan]

Created attachment 301826 [details]
Patch for landing

[Jiewen Tan]

Committed r212465: <http://trac.webkit.org/changeset/212465>