Make Web Crypto build with OpenSSL.
Created attachment 388106 [details] Patch
Created attachment 388107 [details] Patch Make style checker as happy as its realistically going to be.
Comment on attachment 388107 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388107&action=review > Source/WebCore/crypto/openssl/CryptoKeyRSAOpenSSL.cpp:56 > + notImplemented(); Shouldn't we call back the "failureCallback" just in case?
Comment on attachment 388107 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388107&action=review LGTM. r=me. Please address the following minor issues. > Source/WebCore/crypto/keys/CryptoKeyRSA.h:55 > +typedef std::unique_ptr<void*> PlatformRSAKeyContainer; PlatformRSAKey, I believe. > Source/WebCore/crypto/openssl/CryptoAlgorithmAES_CBCOpenSSL.cpp:29 > +#if ENABLE(WEB_CRYPTO) I wonder after your change if this ENABLE macro is still needed. > Source/WebCore/crypto/openssl/CryptoAlgorithmRSA_PSSOpenSSL.cpp:29 > +#if ENABLE(WEB_CRYPTO) && HAVE(RSA_PSS) You probably don't need to include the HAVE(RSA_PSS) macro. And you probably need to set it to 1 for your ports.
Comment on attachment 388107 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388107&action=review >> Source/WebCore/crypto/openssl/CryptoAlgorithmAES_CBCOpenSSL.cpp:29 >> +#if ENABLE(WEB_CRYPTO) > > I wonder after your change if this ENABLE macro is still needed. WEB_CRYPTO can be enabled and disabled so all the .cpp files should technically have these blocks. >> Source/WebCore/crypto/openssl/CryptoAlgorithmRSA_PSSOpenSSL.cpp:29 >> +#if ENABLE(WEB_CRYPTO) && HAVE(RSA_PSS) > > You probably don't need to include the HAVE(RSA_PSS) macro. And you probably need to set it to 1 for your ports. It looks like this is OpenSSL 1.1.1 where support was added. Things get a bit more tricky with LibreSSL because it always reports as OpenSSL 2.0. When enabling this I think we need to do some kind of HAVE check within the CMake and I'd just want to put that off at the moment.
(In reply to Don Olmstead from comment #5) > Comment on attachment 388107 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=388107&action=review > > >> Source/WebCore/crypto/openssl/CryptoAlgorithmAES_CBCOpenSSL.cpp:29 > >> +#if ENABLE(WEB_CRYPTO) > > > > I wonder after your change if this ENABLE macro is still needed. > > WEB_CRYPTO can be enabled and disabled so all the .cpp files should > technically have these blocks. I think there is no points to have an ENABLE compile time flag if all ports have the implementation. > > >> Source/WebCore/crypto/openssl/CryptoAlgorithmRSA_PSSOpenSSL.cpp:29 > >> +#if ENABLE(WEB_CRYPTO) && HAVE(RSA_PSS) > > > > You probably don't need to include the HAVE(RSA_PSS) macro. And you probably need to set it to 1 for your ports. > > It looks like this is OpenSSL 1.1.1 where support was added. Things get a > bit more tricky with LibreSSL because it always reports as OpenSSL 2.0. When > enabling this I think we need to do some kind of HAVE check within the CMake > and I'd just want to put that off at the moment. Apple ports have this because RSA-PSS was added to CommonCrypto recently. Didn't know that OpenSSL has the same issue.
Created attachment 388495 [details] Patch
(In reply to Jiewen Tan from comment #6) > (In reply to Don Olmstead from comment #5) > > Comment on attachment 388107 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=388107&action=review > > > > >> Source/WebCore/crypto/openssl/CryptoAlgorithmAES_CBCOpenSSL.cpp:29 > > >> +#if ENABLE(WEB_CRYPTO) > > > > > > I wonder after your change if this ENABLE macro is still needed. > > > > WEB_CRYPTO can be enabled and disabled so all the .cpp files should > > technically have these blocks. > > I think there is no points to have an ENABLE compile time flag if all ports > have the implementation. > I'd be happy to remove the ENABLE flags after there's actual implementations for everything.
(In reply to Don Olmstead from comment #8) > (In reply to Jiewen Tan from comment #6) > > (In reply to Don Olmstead from comment #5) > > > Comment on attachment 388107 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=388107&action=review > > > > > > >> Source/WebCore/crypto/openssl/CryptoAlgorithmAES_CBCOpenSSL.cpp:29 > > > >> +#if ENABLE(WEB_CRYPTO) > > > > > > > > I wonder after your change if this ENABLE macro is still needed. > > > > > > WEB_CRYPTO can be enabled and disabled so all the .cpp files should > > > technically have these blocks. > > > > I think there is no points to have an ENABLE compile time flag if all ports > > have the implementation. > > > > I'd be happy to remove the ENABLE flags after there's actual implementations > for everything. Great!
The commit-queue encountered the following flaky tests while processing attachment 388495 [details]: imported/w3c/web-platform-tests/IndexedDB/fire-error-event-exception.html bug 201481 (authors: shvaikalesh@gmail.com and youennf@gmail.com) The commit-queue is continuing to process your patch.
Comment on attachment 388495 [details] Patch Clearing flags on attachment: 388495 Committed r254958: <https://trac.webkit.org/changeset/254958>
All reviewed patches have been landed. Closing bug.
<rdar://problem/58820660>