Bug 133122 - [META] [GTK] Implement WebCrypto SubtleCrypto interface
Summary: [META] [GTK] Implement WebCrypto SubtleCrypto interface
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on: 173550 177350 182972 133316 133317 133319 133320 133344 159189 159260 169272 169528 170231 170232 170238 170269 170270 170271 170273 170274 170345 170350 170546 170550 171070 171074 171103 171213 171219 171220 171222 171420 171535 172594 172856 172857 172870 172894 172927 173253 173543 173547 173589 173646 173647 173648 173694 173695 173696 173697 173883 174091 174253 175199 175582
Blocks:
  Show dependency treegraph
 
Reported: 2014-05-20 09:04 PDT by Eduardo Lima Mitev
Modified: 2018-02-20 07:16 PST (History)
15 users (show)

See Also:


Attachments
Patch (35.60 KB, patch)
2014-05-27 07:35 PDT, Eduardo Lima Mitev
no flags Details | Formatted Diff | Diff
Patch (3.55 KB, patch)
2014-05-27 07:37 PDT, Eduardo Lima Mitev
elima: review-
elima: commit-queue-
Details | Formatted Diff | Diff
Patch (5.18 KB, patch)
2014-05-27 08:57 PDT, Eduardo Lima Mitev
no flags Details | Formatted Diff | Diff
Patch (3.41 KB, patch)
2014-05-27 09:43 PDT, Eduardo Lima Mitev
no flags Details | Formatted Diff | Diff
Patch (4.81 KB, patch)
2014-05-27 10:03 PDT, Eduardo Lima Mitev
no flags Details | Formatted Diff | Diff
WIP Patch (208.16 KB, patch)
2017-03-28 13:30 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
WIP patch (150.18 KB, patch)
2017-03-29 22:51 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
WIP patch (113.84 KB, patch)
2017-04-07 03:38 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
WIP Patch (78.03 KB, patch)
2017-05-09 10:22 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
WIP patch (75.05 KB, patch)
2017-06-12 00:35 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Remaining changes (522 bytes, patch)
2017-07-03 01:59 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eduardo Lima Mitev 2014-05-20 09:04:20 PDT
http://www.w3.org/TR/2014/WD-WebCryptoAPI-20140325/

Cross platform code already exists and the Mac port has platform-specific implementation for several algorithms (bug 122679).

This bug is to track implementation for the GTK port.
Comment 1 Eduardo Lima Mitev 2014-05-20 09:41:00 PDT
I have a WIP implementation at:

https://github.com/Igalia/webkit/tree/subtle-crypto

which I plan to submit for review in the coming days.
Comment 2 Eduardo Lima Mitev 2014-05-27 07:35:09 PDT
Created attachment 232126 [details]
Patch
Comment 3 Eduardo Lima Mitev 2014-05-27 07:37:27 PDT
Created attachment 232128 [details]
Patch
Comment 4 WebKit Commit Bot 2014-05-27 07:37:34 PDT
Attachment 232126 [details] did not pass style-queue:


ERROR: Source/WebCore/CMakeLists.txt:30:  There should be exactly one empty line instead of 0 between "inspector" and "loader/appcache".  [list/emptyline] [5]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Eduardo Lima Mitev 2014-05-27 07:52:14 PDT
(In reply to comment #4)
> Attachment 232126 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebCore/CMakeLists.txt:30:  There should be exactly one empty line instead of 0 between "inspector" and "loader/appcache".  [list/emptyline] [5]
> Total errors found: 1 in 17 files
> 

I think this is a false positive. The patch has nothing to do with the error mentioned. The occurrence of "inspector" and "loader/appcache" happens at lines 545 and 547 (not 30), and there is an empty line between them.
Comment 6 Eduardo Lima Mitev 2014-05-27 08:57:43 PDT
Created attachment 232132 [details]
Patch
Comment 7 Eduardo Lima Mitev 2014-05-27 08:59:05 PDT
Comment on attachment 232128 [details]
Patch

this is now obsolete, lacks ChangeLog updates
Comment 8 Eduardo Lima Mitev 2014-05-27 09:43:18 PDT
Created attachment 232135 [details]
Patch
Comment 9 Eduardo Lima Mitev 2014-05-27 10:03:32 PDT
Created attachment 232138 [details]
Patch
Comment 10 Eduardo Lima Mitev 2014-05-27 11:38:10 PDT
To make these changes easier to track and review (and for compliance and sanity), I will make this bug a metabug, cancel all uploaded patches, and create one new bug for each patch. This is a similar approach as in bug 122679. 

I'm really sorry for the noise and the mess.
Comment 11 Eduardo Lima Mitev 2014-05-27 12:00:25 PDT
Adding dependency on bug 133316.
Comment 12 Eduardo Lima Mitev 2014-05-27 12:09:47 PDT
Adding dependency on bug 133317.
Comment 13 Eduardo Lima Mitev 2014-05-27 12:19:36 PDT
Adding dependency on bug 133319.
Comment 14 Eduardo Lima Mitev 2014-05-27 12:45:43 PDT
Adding dependency on bug 133320.
Comment 15 Eduardo Lima Mitev 2014-05-28 03:58:32 PDT
Adding dependency on bug 133344.
Comment 16 Eduardo Lima Mitev 2014-05-29 06:40:05 PDT
Adding Carlos García Campos to CC.
Comment 17 Michael Catanzaro 2016-10-17 14:52:54 PDT
Note that r206883 "Add a dummy SubtleCrypto interface" broke the tests under crypto/workers/subtle:

crypto/workers/subtle/aes-postMessage-worker.html
crypto/workers/subtle/hmac-postMessage-worker.html
crypto/workers/subtle/hrsa-postMessage-worker.html
crypto/workers/subtle/multiple-postMessage-worker.html
crypto/workers/subtle/rsa-postMessage-worker.html

I have skipped this entire test directory.
Comment 18 Zan Dobersek 2017-03-28 13:30:33 PDT
Created attachment 305635 [details]
WIP Patch

This patch implements all the symmetric cipher and public key algorithms used in the Web Crypto standard that are currently supported and tested in WebKit, not including the webkit-prefixed interfaces. The implementations leverage libgcrypt and are in majority complete, but there's still a lot of opportunities to polish and improve the existing code.

All but two relevant tests currently pass, and the two failures that are marked in the TestExpectations file fail because more sub-tests run successfully than the text baseline expects.

This patch contains the whole changeset, but is of course inconvenient to review. I'll be splitting it up into smaller patches instead. I'll try to update this patch regularly to present the set of changes that still has to be landed.
Comment 19 Jiewen Tan 2017-03-28 14:09:33 PDT
(In reply to Zan Dobersek from comment #18)
> Created attachment 305635 [details]
> WIP Patch
> 
> This patch implements all the symmetric cipher and public key algorithms
> used in the Web Crypto standard that are currently supported and tested in
> WebKit, not including the webkit-prefixed interfaces. The implementations
> leverage libgcrypt and are in majority complete, but there's still a lot of
> opportunities to polish and improve the existing code.
> 
> All but two relevant tests currently pass, and the two failures that are
> marked in the TestExpectations file fail because more sub-tests run
> successfully than the text baseline expects.
> 
> This patch contains the whole changeset, but is of course inconvenient to
> review. I'll be splitting it up into smaller patches instead. I'll try to
> update this patch regularly to present the set of changes that still has to
> be landed.

Please split it. Thanks! It will be good to split it according to specific algorithms.
Comment 20 Michael Catanzaro 2017-03-28 19:37:44 PDT
WTF, you did all this over the weekend...? Wow! 

Why are the gcrypt headers missing license notices?

Can you delete FindGnutls.cmake and references to it?
Comment 21 Carlos Alberto Lopez Perez 2017-03-28 20:14:33 PDT
(In reply to Michael Catanzaro from comment #20)
> WTF, you did all this over the weekend...? Wow! 
> 

I'm also shocked. This is completely amazing. \o/

> Why are the gcrypt headers missing license notices?
> 
> Can you delete FindGnutls.cmake and references to it?

Some quick comments:

I tried the patch and I'm getting several timeouts on the tests.
Also this test page https://vibornoff.github.io/webcrypto-examples/index.html that I use as reference for this WebCrypto stuff loads very slow (takes several minutes to appear some of the content, and the page doesn't finish to load). Does it work for you properly?
Comment 22 Eduardo Lima Mitev 2017-03-28 23:25:17 PDT
(In reply to Zan Dobersek from comment #18)
> The implementations leverage libgcrypt and are in majority complete,
> but there's still a lot of opportunities to polish and improve the
> existing code.
> 

Did you considered using libnettle instead of gcrypt? This is the first question I faced when I started implementing the first algorithms, and libnettle was a more or less clear choice:

* gnutls moved away from gcrypt in favor of libnettle,

* libnettle API is certainly easier to work with (standard C API as opposed working with to S-expressions),

* libnettle is arguably faster, though I don't have data to back that up. libnettle includes CPU-native implementations for some algos in some archs.

* libnettle is (again arguably) better maintained, hence more future proof.

But I have not worked on this for a long time, so maybe this question was already answered before, or there are other arguments in favor of gcrypt that I'm not aware of (e.g, license issues). If that's the case, sorry for the noise.

I'm so looking forward to see good support of WebCrypto on the WebkitGTK shipped by distros. And speaking about that, Martin Robinson once suggested that we should probably audit this code before un-flagging the feature. Any plan/idea about that?

Kudos for the awesome work!
Comment 23 Zan Dobersek 2017-03-29 01:07:35 PDT
(In reply to Michael Catanzaro from comment #20)
> Why are the gcrypt headers missing license notices?
> 

Forgot.

> Can you delete FindGnutls.cmake and references to it?

Yes!
Comment 24 Zan Dobersek 2017-03-29 01:23:41 PDT
(In reply to Carlos Alberto Lopez Perez from comment #21)
> Some quick comments:
> 
> I tried the patch and I'm getting several timeouts on the tests.

Forgot to mention, the implementation depends on the 8-bit cihper feedback mode, represented by the GCRY_CIPHER_MODE_CFB8 constant. That was implemented in February, after the latest 1.7.6 release:
https://git.gnupg.org/cgi-bin/gitweb.cgi?p=libgcrypt.git;a=commit;h=d1ee9a660571ce4a998c9ab2299d4f2419f99127
Comment 25 Zan Dobersek 2017-03-29 01:48:04 PDT
(In reply to Eduardo Lima Mitev from comment #22)
> (In reply to Zan Dobersek from comment #18)
> > The implementations leverage libgcrypt and are in majority complete,
> > but there's still a lot of opportunities to polish and improve the
> > existing code.
> > 
> 
> Did you considered using libnettle instead of gcrypt? This is the first
> question I faced when I started implementing the first algorithms, and
> libnettle was a more or less clear choice:
> 
> * gnutls moved away from gcrypt in favor of libnettle,
> 
> * libnettle API is certainly easier to work with (standard C API as opposed
> working with to S-expressions),
> 
> * libnettle is arguably faster, though I don't have data to back that up.
> libnettle includes CPU-native implementations for some algos in some archs.
> 
> * libnettle is (again arguably) better maintained, hence more future proof.
> 
> But I have not worked on this for a long time, so maybe this question was
> already answered before, or there are other arguments in favor of gcrypt
> that I'm not aware of (e.g, license issues). If that's the case, sorry for
> the noise.
> 
> I'm so looking forward to see good support of WebCrypto on the WebkitGTK
> shipped by distros. And speaking about that, Martin Robinson once suggested
> that we should probably audit this code before un-flagging the feature. Any
> plan/idea about that?
> 
> Kudos for the awesome work!

I don't remember the specifics or conclusions from any previous discussions, but I didn't consider libnettle. libgcrypt is already being used for the digest algorithms by default, and it offers coverage of all the ciphers and modes that are required in the W3C spec.

I looked at libnettle briefly now, and there appear to be missing cipher modes that are needed to implement the AES-CFB-8 and AES-KW support. (Although the AES-CFB-8 cipher was removed from the spec, so the CFB8 might not be required in the future.)

But in general, I'm OK with using libnettle if there aren't any potential licensing issues and if we're able to fill in the feature gap.
Comment 26 Zan Dobersek 2017-03-29 01:49:31 PDT
(In reply to Carlos Alberto Lopez Perez from comment #21)
> Also this test page
> https://vibornoff.github.io/webcrypto-examples/index.html that I use as
> reference for this WebCrypto stuff loads very slow (takes several minutes to
> appear some of the content, and the page doesn't finish to load). Does it
> work for you properly?

It loads fine for me, using the libgcrypt 1.7.6 release or the more recent gcrypt build that includes the CFB8 support.
Comment 27 Michael Catanzaro 2017-03-29 08:17:16 PDT
(In reply to Zan Dobersek from comment #25)
> But in general, I'm OK with using libnettle if there aren't any potential
> licensing issues and if we're able to fill in the feature gap.

We added libgcrypt support to allow avoiding libnettle (GPLv2+ or LGPLv3+). It's not any problem for Linux distros, but it will be a problem for customers. We should prefer libgcrypt.

(In reply to Zan Dobersek from comment #24)
> (In reply to Carlos Alberto Lopez Perez from comment #21)
> > Some quick comments:
> > 
> > I tried the patch and I'm getting several timeouts on the tests.
> 
> Forgot to mention, the implementation depends on the 8-bit cihper feedback
> mode, represented by the GCRY_CIPHER_MODE_CFB8 constant. That was
> implemented in February, after the latest 1.7.6 release:
> https://git.gnupg.org/cgi-bin/gitweb.cgi?p=libgcrypt.git;a=commit;
> h=d1ee9a660571ce4a998c9ab2299d4f2419f99127

Then we need to update OptionsGTK.cmake accordingly:

find_package(LibGcrypt 1.6.0 REQUIRED)
Comment 28 Michael Catanzaro 2017-03-29 08:20:14 PDT
(In reply to Eduardo Lima Mitev from comment #22)
> I'm so looking forward to see good support of WebCrypto on the WebkitGTK
> shipped by distros. And speaking about that, Martin Robinson once suggested
> that we should probably audit this code before un-flagging the feature. Any
> plan/idea about that?

We absolutely should. Cost would be high, so I suspect it's not going to happen.
Comment 29 Zan Dobersek 2017-03-29 22:51:20 PDT
Created attachment 305835 [details]
WIP patch
Comment 30 Zan Dobersek 2017-04-07 03:38:12 PDT
Created attachment 306484 [details]
WIP patch
Comment 31 Zan Dobersek 2017-05-09 10:22:17 PDT
Created attachment 309512 [details]
WIP Patch

Remaining changes based on top of changes in #171219, #171220, #171222, #171420.
Comment 32 Zan Dobersek 2017-06-12 00:35:09 PDT
Created attachment 312638 [details]
WIP patch

Now with more libtasn1.
Comment 33 Zan Dobersek 2017-07-03 01:59:18 PDT
Created attachment 314466 [details]
Remaining changes

Remaining changes that are spread across the blocker bugs.