Remove toJSValueFromJsonWebKey from custom SubtleCrypto binding codes.
Created attachment 298801 [details] Patch
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?
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.
Created attachment 300962 [details] Patch
Created attachment 301105 [details] Patch
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.
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
(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.
Created attachment 301360 [details] Patch
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.
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.
Created attachment 301650 [details] Patch
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.
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.
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.
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.
Created attachment 301699 [details] Patch
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.
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
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.
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
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.
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
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.
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
Created attachment 301791 [details] Patch
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.
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.
Created attachment 301802 [details] Patch
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.
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.
Created attachment 301826 [details] Patch for landing
Committed r212465: <http://trac.webkit.org/changeset/212465>