Bug 206449 – [OpenSSL] Add stubs and build for Web Crypto

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Don Olmstead]

Make Web Crypto build with OpenSSL.

[Don Olmstead]

Created attachment 388106 [details]
Patch

[Don Olmstead]

Created attachment 388107 [details]
Patch

Make style checker as happy as its realistically going to be.

[Yoshiaki Jitsukawa]

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?

[Jiewen Tan]

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.

[Don Olmstead]

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.

[Jiewen Tan]

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

[Don Olmstead]

Created attachment 388495 [details]
Patch

[Don Olmstead]

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

[Jiewen Tan]

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

[WebKit Commit Bot]

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: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 388495 [details]
Patch

Clearing flags on attachment: 388495

Committed r254958: <https://trac.webkit.org/changeset/254958>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58820660>