Bug 133122

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.

Created attachment 232126 [details] Patch

Created attachment 232128 [details] Patch

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.

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

Created attachment 232132 [details] Patch

Comment on attachment 232128 [details] Patch this is now obsolete, lacks ChangeLog updates

Created attachment 232135 [details] Patch

Created attachment 232138 [details] Patch

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.

Adding dependency on bug 133316.

Adding dependency on bug 133317.

Adding dependency on bug 133319.

Adding dependency on bug 133320.

Adding dependency on bug 133344.

Adding Carlos García Campos to CC.

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.

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.

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

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?

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

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

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

(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

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

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

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

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

Created attachment 305835 [details] WIP patch

Created attachment 306484 [details] WIP patch

Created attachment 309512 [details] WIP Patch Remaining changes based on top of changes in #171219, #171220, #171222, #171420.

Created attachment 312638 [details] WIP patch Now with more libtasn1.

Created attachment 314466 [details] Remaining changes Remaining changes that are spread across the blocker bugs.