Bug 172758 - [GTK] Enable SUBTLE_CRYPTO in GTK+ releases
Summary: [GTK] Enable SUBTLE_CRYPTO in GTK+ releases
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-05-31 09:01 PDT by Zan Dobersek
Modified: 2017-09-19 12:03 PDT (History)
7 users (show)

See Also:


Attachments
Patch (2.67 KB, patch)
2017-08-08 23:53 PDT, Carlos Garcia Campos
zan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2017-05-31 09:01:54 PDT
There's been discussions to enable the SUBTLE_CRYPTO (i.e. the WebCrypto API support) already in WebKitGTK+ releases.

However, we also said that we'd prefer to use something like ENABLE_WEB_CRYPTO as the configuration flag, instead of the current ENABLE_SUBTLE_CRYPTO.

What I don't know is how to execute this -- do we want to go out and propose changing ENABLE_SUBTLE_CRYPTO to ENABLE_WEB_CRYPTO throughout the code, or do we make an additional ENABLE_WEB_CRYPTO CMake option that aliases the ENABLE_SUBTLE_CRYPTO one?
Comment 1 Carlos Garcia Campos 2017-05-31 23:19:27 PDT
I think I would just rename the SUBTLE_CRYPTO as WEB_CRYPTO.
Comment 2 Jiewen Tan 2017-06-01 01:11:36 PDT
(In reply to Carlos Garcia Campos from comment #1)
> I think I would just rename the SUBTLE_CRYPTO as WEB_CRYPTO.

That's not right. WebCrypto refers to the whole Crypto Interface while SubtleCrypto refers only to the SubtleCrypto interface.
Comment 3 Michael Catanzaro 2017-06-01 07:54:56 PDT
We can also keep both flags and only expose WEB_CRYPTO as public. (Exposing both is too much configurability.)
Comment 4 Adrian Perez 2017-06-01 12:10:00 PDT
(In reply to Michael Catanzaro from comment #3)
> We can also keep both flags and only expose WEB_CRYPTO as public. (Exposing
> both is too much configurability.)

This is probably the path of least resistance, I'd go with this.
Comment 5 Michael Catanzaro 2017-06-01 12:38:05 PDT
Well the WEB_CRYPTO flag does not currently exist, so some work is required to figure out which parts of the API are the SubtleCrypto portions and which are higher-level. And then we're stuck with two flags that we expect will always be OFF or ON together; I doubt anybody would be interested in supporting subtle crypto without the rest of WebCrypto. So it doesn't seem like a clear choice to me.
Comment 6 Carlos Garcia Campos 2017-07-26 01:45:26 PDT
Do we really need a public build option? Can't we just enable current build options unconditionally?
Comment 7 Michael Catanzaro 2017-07-26 09:20:35 PDT
(In reply to Carlos Garcia Campos from comment #6)
> Do we really need a public build option? Can't we just enable current build
> options unconditionally?

I think we can make it unconditional. Check the version of gcrypt available in Ubuntu 16.04 against our dependencies policy. It's probably fine.

On that note, we can probably remove more options now, and e.g. require libhyphen etc.
Comment 8 Carlos Garcia Campos 2017-08-05 02:51:57 PDT
Could we fix this before the next release (next week)? I don't mind how, but I want this enabled by default in 2.18 and we are about to branch.
Comment 9 Michael Catanzaro 2017-08-07 07:08:20 PDT
Today is supposed to be feature freeze. I would say it's missed the boat for 2.18 at this point. But it's your call... if we're going to enable it, though, let's do so *now*.
Comment 10 Carlos Garcia Campos 2017-08-07 08:13:44 PDT
(In reply to Michael Catanzaro from comment #9)
> Today is supposed to be feature freeze. I would say it's missed the boat for
> 2.18 at this point. But it's your call... if we're going to enable it,
> though, let's do so *now*.

Yes, I'll branch and release tomorrow or on Wednesday. The thing is how to enable this, let's just find a good name and enable whatever builds flags are needed.
Comment 11 Michael Catanzaro 2017-08-07 08:25:30 PDT
(In reply to Jiewen Tan from comment #2)
> That's not right. WebCrypto refers to the whole Crypto Interface while
> SubtleCrypto refers only to the SubtleCrypto interface.

I don't understand why it's not right. All of SubtleCrypto is part of WebCrypto, so using the WebCrypto flag should be just fine, right? But using a SubtleCrypto flag to guard other WebCrypto features would not be right. Have we really failed to implement any WebCrypto besides the SubtleCrypto interface, or do I misunderstand something?
Comment 12 Zan Dobersek 2017-08-08 01:01:44 PDT
(In reply to Michael Catanzaro from comment #11)
> (In reply to Jiewen Tan from comment #2)
> > That's not right. WebCrypto refers to the whole Crypto Interface while
> > SubtleCrypto refers only to the SubtleCrypto interface.
> 
> I don't understand why it's not right. All of SubtleCrypto is part of
> WebCrypto, so using the WebCrypto flag should be just fine, right? But using
> a SubtleCrypto flag to guard other WebCrypto features would not be right.
> Have we really failed to implement any WebCrypto besides the SubtleCrypto
> interface, or do I misunderstand something?

SUBTLE_CRYPTO covers a subset of the Web Crypto API spec, the one that's exposed through `window.crypto.subtle`. There's also `window.crypto.getRandomValues()` that's not covered by SUBTLE_CRYPTO.
https://www.w3.org/TR/WebCryptoAPI/#crypto-interface

SUBTLE_CRYPTO does expose the prefixed (and to-be-deprecated) `window.crypto.webkitSubtle` interface. Most of that is not implemented.
Comment 13 Zan Dobersek 2017-08-08 01:05:48 PDT
IMO we don't need an additional flag. The existing one should be turned on unconditionally, but for it to work the libgcrypt dependency has to be of 1.7.0 version at minimum.

The drawback is that at this point enabling SUBTLE_CRYPTO also exposes the prefixed WebKitSubtleCrypto interface, which would be deprecated in the future. But I don't remember this being a blocker in the past.
Comment 14 Carlos Alberto Lopez Perez 2017-08-08 02:19:54 PDT
(In reply to Zan Dobersek from comment #13)
> IMO we don't need an additional flag. The existing one should be turned on
> unconditionally, but for it to work the libgcrypt dependency has to be of
> 1.7.0 version at minimum.
> 
> The drawback is that at this point enabling SUBTLE_CRYPTO also exposes the
> prefixed WebKitSubtleCrypto interface, which would be deprecated in the
> future. But I don't remember this being a blocker in the past.

Ubuntu 16.04 has libgcrypt 1.6.5.
Comment 15 Michael Catanzaro 2017-08-08 05:43:27 PDT
OK, so the only part of the WebCrypto API that is not SubtleCrypto is the get random numbers function?

(In reply to Zan Dobersek from comment #13)
> IMO we don't need an additional flag. The existing one should be turned on
> unconditionally, but for it to work the libgcrypt dependency has to be of
> 1.7.0 version at minimum.

We have a distributor that wants to upgrade WebKit but isn't able to upgrade gcrypt for whatever reason (probably enterprise security policy). They have gcrypt 1.5.8. And anyway, Ubuntu having only 1.6.5 indicates this really must have a build flag.

I would still expose a flag named ENABLE_WEB_CRYPTO flag and keep ENABLE_SUBTLE_CRYPTO private. ENABLE_SUBTLE_CRYPTO is fine to use in our code, if Jiewen doesn't want to rename it, but I still think it's too low-level to expose as a public option. We can make ENABLE_WEB_CRYPTO toggle ENABLE_SUBTLE_CRYPTO via a dependency.

(In reply to Zan Dobersek from comment #13) 
> The drawback is that at this point enabling SUBTLE_CRYPTO also exposes the
> prefixed WebKitSubtleCrypto interface, which would be deprecated in the
> future. But I don't remember this being a blocker in the past.

I don't really care. It would probably be better for web compatibility to match Apple anyway.
Comment 16 Carlos Alberto Lopez Perez 2017-08-08 08:14:16 PDT
(In reply to Michael Catanzaro from comment #15)
> We have a distributor that wants to upgrade WebKit but isn't able to upgrade
> gcrypt for whatever reason (probably enterprise security policy). They have
> gcrypt 1.5.8. And anyway, Ubuntu having only 1.6.5 indicates this really
> must have a build flag.

I agree this should have a public build flag.

I wonder if we should enable the flag by default now or not? If we enable it, that will mean the default build configuration won't be build-able on Ubuntu 16.04 anymore.
However https://trac.webkit.org/wiki/WebKitGTK/DependenciesPolicy doesn't say anything about the default configuration.
Comment 17 Carlos Garcia Campos 2017-08-08 08:19:48 PDT
(In reply to Carlos Alberto Lopez Perez from comment #16)
> (In reply to Michael Catanzaro from comment #15)
> > We have a distributor that wants to upgrade WebKit but isn't able to upgrade
> > gcrypt for whatever reason (probably enterprise security policy). They have
> > gcrypt 1.5.8. And anyway, Ubuntu having only 1.6.5 indicates this really
> > must have a build flag.
> 
> I agree this should have a public build flag.
> 
> I wonder if we should enable the flag by default now or not? If we enable
> it, that will mean the default build configuration won't be build-able on
> Ubuntu 16.04 anymore.
> However https://trac.webkit.org/wiki/WebKitGTK/DependenciesPolicy doesn't
> say anything about the default configuration.

As long as old distros can just disable it, I don't think we should penalize users using newer versions.
Comment 18 Carlos Garcia Campos 2017-08-08 08:20:27 PDT
IIRC we want to enable this because it's required by popular web sites/apps.
Comment 19 Adrian Perez 2017-08-08 08:36:08 PDT
(In reply to Carlos Garcia Campos from comment #17)
> (In reply to Carlos Alberto Lopez Perez from comment #16)
> > (In reply to Michael Catanzaro from comment #15)
> > > We have a distributor that wants to upgrade WebKit but isn't able to upgrade
> > > gcrypt for whatever reason (probably enterprise security policy). They have
> > > gcrypt 1.5.8. And anyway, Ubuntu having only 1.6.5 indicates this really
> > > must have a build flag.
> > 
> > I agree this should have a public build flag.
> > 
> > I wonder if we should enable the flag by default now or not? If we enable
> > it, that will mean the default build configuration won't be build-able on
> > Ubuntu 16.04 anymore.
> > However https://trac.webkit.org/wiki/WebKitGTK/DependenciesPolicy doesn't
> > say anything about the default configuration.
> 
> As long as old distros can just disable it, I don't think we should penalize
> users using newer versions.

I agree with this. Let's use the wording of the policy to the advantage
of our users and have the feature enabled by default.
Comment 20 Michael Catanzaro 2017-08-08 14:56:50 PDT
It should default to on. Ubuntu can just disable that option.
Comment 21 Zan Dobersek 2017-08-08 23:44:18 PDT
(In reply to Michael Catanzaro from comment #15)
> OK, so the only part of the WebCrypto API that is not SubtleCrypto is the
> get random numbers function?
> 

Yes, but to alias ENABLE_SUBTLE_CRYPTO against a new ENABLE_WEB_CRYPTO option is still a bit misleading.
Comment 22 Carlos Garcia Campos 2017-08-08 23:53:27 PDT
Created attachment 317683 [details]
Patch
Comment 23 Carlos Garcia Campos 2017-08-09 00:13:16 PDT
Committed r220445: <http://trac.webkit.org/changeset/220445>
Comment 24 Carlos Alberto Lopez Perez 2017-08-09 05:06:37 PDT
Done:

 * Debian Stable bot (on Jessie still): https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20Debian%20Stable%20%28Build%29
   - Installed libgcrypt 1.7 from the jessie backports repository.
 * Ubuntu LTS Bot (16.04): https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20Ubuntu%20LTS%20%28Build%29
   - Configured it to build with -DENABLE_WEB_CRYPTO=OFF via the environment variable  BUILD_WEBKIT_ARGS supported by the script build-webkit.

That way:

 * The default configuration of WebKitGTK+ remains build-able on Debian 8 (Jessie) (but you have to use a few packages from backports like clang-3.8 and newer libgcrypt)

 * The default configuration of WebKitGTK+ is not longer build-able on Ubuntu 16.04, but you can build without web crypto by simply passing -DENABLE_WEB_CRYPTO=OFF to CMake. This bot will test this to ensure no breakage happens with WEB_CRYPTO=OFF.