feat(op_crates/crypto): Introduce Web Crypto API #8984

yacinehmito · 2021-01-04T10:13:28Z

First step to solve #1891.

This PR adds the common code for the Web Crypto API, i.e.:

TypeScript Type definitions
Core logic in the JS wrapper related to algorithm normalisation

Progress

We document progress on solving #1891 here:

Design

Type definitions

The type definitions have been taken from https://github.com/microsoft/TypeScript/blob/2428ade1a91248e847f3e1561e31a9426650efee/src/lib/webworker.generated.d.ts#L3060.
They are consistent with the W3C recommendation.
I made a slight change regarding the keyUsages parameters by giving them the type Iterable<KeyUsage> instead of KeyUsage[]. Indeed, Chrome, Firefox and Safari all accept iterables of strings, not just arrays (the specs don't specify the exact type; they just mention "sequences").

JavaScript wrapper

crypto.subtle

I went with a simple literal object for crypto.subtle, as we had with crypto itself. It's not how Chrome, Firefox and Safari do it: they expose the "classes" Crypto and SubtleCrypto, which can't be instantiated. The methods of crypto and crypto.subtle aren't in the object itself but on the respective prototype.
Let me know if you want me to mirror that behaviour in Deno.

JS side vs. Rust side

The validation logic and the dictionary of algorithms are handled on the JS side because:

It more convenient for me. I wouldn't know how to do the same thing with Rust without some research.
Building a new object that we control before serialising it for Rust is probably faster and safer than serialising whatever the caller provides directly. If the caller provides an object with circular references and a lot of data, this won't impact performance as we only serialise what we need.

Returning buffers

We instantiate buffers on the JS side as ArrayBuffer, pass them wrapped in an Uint8Array to the Rust side as zero-copy buffers, then fill them with the data that is supposed to be returned.
That way, we don't have to serialise the data returned by the underlying crypto library when it is large.

However, this assumes that we know in advance the size of the ArrayBuffer on the JS side. So far, this hasn't been a big issue, but it does add complexity.

It would be handy if we could instantiate an ArrayBuffer directly on the Rust side and return a handle to the JS side.

Underlying crypto library

Deliberating which crypto library to pick

It's not clear which crypto library I should use to implement the Rust ops.

ring seems to be the go-to crypto library in Rust, but it doesn't support RSA key generation (see Add RSA key generation briansmith/ring#219). Someone attempted a pull request a while back (see [WIP]: Rsa keygen briansmith/ring#733) but the maintainer seemed unresponsive and it stalled. I find that a bit concerning, so I don't know if I should go with ring.
Firefox relies on NSS for the Web Crypto API. Sadly there are no up-to-date crate with bindings for it.
Safari and Chrome seem to mostly rely on OpenSSL. That would be the safe choice, but integrating OpenSSL has some non-trivial requirements. I'm too green on Deno to understand the implications and determine whether this would be a benign change or something to avoid.

I require some guidance as to which library should be included. Once it's decided, I expect the implementations to be rather easy.

We will go for ring, as per @lucacasonato's recommendation. It won't be enough to implement all operations, but we'll see when that comes up.

Testing

Chrome, Firefox and Safari all share the same test suite for the Web Crypto API, as they just vendored the relevant tests from web-platform-tests. I figured that the safest and easiest way to test the Web Crypto API on deno would be to do the same.

However, web-platform-tests are meant to run in browsers. There is a heavy reliance on a DOM, and scripts are assumed to be running in the global scope. To be able to run those tests unchanged in Deno, we would need to reimplement their JavaScript test harness from scratch.

I deemed this not worth the effort, and settled on rewriting the tests so that they can directly run as unit tests in Deno. It has the advantage of making the test suite more idiomatic (and arguably easier to read as it's in modern TypeScript, as opposed to ES5). However it makes it all the more challenging to follow upstream. I don't think it's a big deal given the very slow rate of changes there, but it ought to be mentioned.

@lucacasonato added the the web-platform-tests as part of #8990.

yacinehmito · 2021-01-04T10:20:39Z

This PR is very WIP. I opened it so that you can provide guidance early enough.

lucacasonato · 2021-01-05T02:47:57Z

@yacinehmito FYI I have started implementing WPT in #8990. That might be helpful for this PR.

Regarding which crypto lib to use: lets use ring, even if it doesn't have support for RSA key gen yet. I am sure we can figure something out once we get to the point where that becomes an issue (maybe rsa crate?). I think starting with the more "simple" stuff like digest would be great.

yacinehmito · 2021-01-09T13:22:26Z

I updated the PR description to account for the decisions that have been made and the changes in approach, and added a few more information on:

Whether to put code on the JS side or on the Rust side
How to return a buffer without serialising it

yacinehmito · 2021-01-09T13:24:10Z

I pushed a rough implementation of digest using ring.
There are a few things to sort out; I'll detail them in a self-review.

Also, I wonder if it wouldn't be preferable to put the SubtleCrypto interface behind the --unstable flag, so that we can merge the implementations little by little instead of doing a mega PR. What do you think?

yacinehmito · 2021-01-09T13:26:00Z

core/zero_copy_buf.rs

+  // TODO: Determine whether it's worth exposing just to assert the size
+  pub byte_length: usize,


This is supposed to be private, but I needed it to check that the size of the provided buffer is the expected one.
I don't think exposing that data is harmful. Let me know what you prefer.

If we want to keep this field private, we can also manually pass the size of the instantiated buffer as part of Value. It's a bit more work but it's doable.

Please revert, you don't need to change ZeroCopyBuf to check length; ZeroCopyBuf derefs to [u8] and you can use its size() to check it.

yacinehmito · 2021-01-09T13:26:28Z

op_crates/crypto/01_crypto.js

+  // Algorithm normalization, which involves storing the expected data for
+  // each pair of algorithm and crypto operation, is done on the JS side because
+  // it is convenient.
+  // Shall it stay that way, or are we better to move it on the Rust side?


What do you think?

yacinehmito · 2021-01-09T13:28:28Z

op_crates/crypto/01_crypto.js

+  const supportedAlgorithms = {
+    digest: new RegisteredAlgorithmsContainer({


This structure follows the spec. It mentions that implementations need a container called supportedAlgorithms that maps operation names to registeredAlgorithms, which in turn maps algorithm names to the expected shape of the algorithm input parameters.

yacinehmito · 2021-01-09T13:31:39Z

op_crates/crypto/01_crypto.js

+  const digestByteLengths = {
+    "SHA-1": 20,
+    "SHA-256": 32,
+    "SHA-384": 48,
+    "SHA-512": 64,
+  };


I don't like that we need to repeat that information on the JS side. ring already knows about this.
I opted for copying it here and asserting that the size of the instantiated buffer matches the one expected by ring.
(This is why I exposed ZeroCopyBuf.byte_length earlier, although it is not strictly necessary)

I see two other options to avoid this:

Instantiate the ArrayBuffer on the Rust side. IMHO it is preferable, but would require significant work.

Expose an OP to fetch the sizes defined on the Rust side. It's not as good in terms of guarantees, but at least we don't have to hardcode the lengths like that.

Thoughts?

These are fixed values that will not change in the future (defined by spec), so it is fine to hard code them. Sending over the information in an extra op adds too much complexity and overhead.

yacinehmito · 2021-01-09T13:33:03Z

op_crates/crypto/01_crypto.js

+    "SHA-512": 64,
+  };
+
+  function normalizeAlgorithm(algorithm, registeredAlgorithms) {


Doing all that work in JS because:

It's easier

We only ever have to serialise algorithm parameters that are normalised, meaning we will never attempt to serialise the input as provided by the user (which could be huge, be of a weird type, or contain circular references)

yacinehmito · 2021-01-09T13:34:03Z

op_crates/crypto/lib.rs

+) -> Result<Value, AnyError> {
+  let mut zc = zero_copy;
+
+  // FIXME: This chain of `unwrap` is meh.


Is it deemed not clean enough, and if so how would you fix that?
Because normalisation has already happened on the JS side, unwrap() should never panic.

Users can still send garbage data manually, you can use something like let args: SubtleDigestArgs = serde_json::from_value(args)?; like it's done in other ops.

Users can still send garbage data manually

How? Can they call an OP directly without going through the JS API?

Regardless, I'll implement your suggestion.

Can they call an OP directly without going through the JS API?

Yes. This is not covered under any stability guarantee though, and is not officially documented. We should try minimize rust panics where possible and gracefully return errors to JS.

How? Can they call an OP directly without going through the JS API?

They can using JS API:

Deno.core.dispatch("op_crypto", new TextEncoder().encode("some garbage data that is not valid json"));

yacinehmito · 2021-01-09T13:36:05Z

op_crates/crypto/lib.rs

+  // TODO: This should run in another thread
+  // Otherwise, even though the API is async, it is blocking the main thread
+  // See https://stackoverflow.com/questions/61292425/how-to-run-an-asynchronous-task-from-a-non-main-thread-in-tokio


From what I understand, jsonOpAsync() doesn't necessarily mean that the code runs asynchronously. It just means that it returns a Promise (a Future on Rust side).
So, to make it truly asynchronous, we'd have to run the call to ring's digest() in another thread. I have no idea how to do this. Is there an example of this already in the codebase?

There's no point in making this op async if the Rust API is sync; just use sync op and if you need to return promise do it manually in JS

If you have some synchronous CPU intensive operation with an async API in JS, you can run it in the tokio blocking thread pool (tokio::task::spawn_blocking).

Good point 👍

@lucacasonato Thank you very much, it's exactly what I was looking for.

lucacasonato

Looks very good so far. Regarding your question about merging this in smaller PRs: yes please. It makes reviewing a lot easier. I don't think there is a need to put this behind unstable, because there will be no interface changes in the future as the interface is defined by the WebCrypto spec. We don't consider bug fixes (or alignment with browser) in Web APIs breaking changes.

If you think it makes sense to put this behind --unstable though, we can do that too. @bartlomieju do you have opinions on this?

lucacasonato · 2021-01-09T14:12:39Z

op_crates/crypto/01_crypto.js

+      const input = data.slice(0);
+      const output = new ArrayBuffer(digestByteLengths[alg.name]);
+      await core.jsonOpAsync(
+        "op_subtle_digest",


I would prefer if all ops in the crypto op crate would start with with op_crypto.

Shall I also rename the one for getRandomValues()?

Yes please.

bartlomieju · 2021-01-09T14:26:33Z

If you think it makes sense to put this behind --unstable though, we can do that too. @bartlomieju do you have opinions on this?

Yeah I agree; unstable is superfluous in this case.

yacinehmito · 2021-01-09T15:16:33Z

I'll split the PR the following way:

This one will have the common stuff: TS type definitions & common code on the JS side. It should be mergeable very quickly.
I'll create a new PR to rename the op of getRandomValues() -> fix(op_crates/crypto): Prefix op of getRandomValues() by op_crypto #9067
I'll create a new PR for the digest implementation specifically, and then one for each new operation after that (or more)

I'll keep reporting on the status of the overall task on this PR as #1891 is a bit too cluttered for that. Else, I can create a new issue to replace #1891. It's however you want.

yacinehmito · 2021-01-09T17:54:07Z

PRs #9016 and #9067 need to be merged first.

yacinehmito · 2021-01-30T11:05:44Z

The PR is up-to-date with master.

The type definitions have been moved to the op crate, following the convention set by the others (web, fetch and websocket).

yacinehmito · 2021-02-05T07:51:18Z

@lucacasonato @bartlomieju I think it is ready for merging.

caspervonb · 2021-02-05T09:27:21Z

Could you enable the relevant wpt tests? see the contributing docs for how the new wpt runner works.

yacinehmito · 2021-02-05T10:32:22Z

This PR has no implementation. The tests shouldn't be enabled. They will be selectively added as more crypto functions are implemented.

bartlomieju · 2021-02-09T15:38:14Z

@yacinehmito thanks for the PR, there's a lot of changes but I can find any implementation of actual APIs. Is this a rebase mistake? If not then I'd prefer to land a PR that contains implementation of at least one algo.

bartlomieju · 2021-02-09T15:38:48Z

op_crates/crypto/01_crypto.js

+  // a provided algorithm. We store it here to prevent prototype pollution.
+  const toUpperCase = String.prototype.toUpperCase;
+
+  class RegisteredAlgorithmsContainer {


This class is never used

It is meant to be used in further PRs.

bartlomieju · 2021-02-09T15:39:33Z

op_crates/crypto/Cargo.toml

+ring = "0.16.19"
+tokio = { version = "1.1.1", features = ["full"] }
+serde_json = { version = "1.0.61", features = ["preserve_order"] }
+serde = { version = "1.0.121", features = ["derive"] }


Why are all those dependencies added here? serde and serde_json are reexported from deno_core anyway, so no need to declare them here

@bartlomieju All other op_crates include serde as a dependency. Why would this one be any different?
If they shouldn't, I'd be happy to open a PR to remove them.

yacinehmito · 2021-02-10T17:06:39Z

@bartlomieju It used to have one, but if you go back the comments thread you'll see that I was asked to separate implementations in multiple PRs.
I'm fine with the decision itself, it's just that I'd be going back on something I have already done for a value that is not very tangible to me; the PR with the digest implementation is already open, and most operations are not implemented anyway.

caspervonb · 2021-02-10T17:15:13Z

This PR has no implementation.

Okay I can see what you're going for here.

I am strongly against implementing throwing stubs; there are packages in the wild that do progressive enhancement so these should just still be undefined so they can be tested for using normal property lookups.

yacinehmito · 2021-02-10T19:08:48Z

This is sensible, and why I suggested to put this behind unstable, but it was rejected.
I'll let you core contributors figure out a good rollout strategy for the WebCrypto API.

In the meantime I will work on other implementations and open draft PRs as I go.

Let me know once you have a definitive plan that people are aligned on, and I will execute on it.

yacinehmito · 2021-02-20T13:09:06Z

@caspervonb @bartlomieju @lucacasonato
Have you settled on how you would wish to incrementally rollout the WebCrypto API to the codebase?

Exploring options

Introduce the ops one by one, then the TypeScript declaration last

It's very simple to do, and nobody using TypeScript will see the WebCrypto API until it is fully implemented. However, the API would still be exposed because we need it for wpt tests.

Introduce the API first, then the ops one by one

This is what this PR does. Undesirable at this leads to runtime errors. We are bettern

Put the API behind the unstable flag

Would allow us plenty of iteration without being too concerned about exposing a half-finished API. Proposition already rejected though.

Use a feature branch

Have a branch dedicated to implementing the WebCrypto API, then merge smaller PRs into it. Would allow for releasing the WebCrypto API at once, but may lead to integration issues if the branch is not constantly rebased.

Thoughts?

stale · 2021-04-21T18:05:40Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

laughedelic · 2021-04-29T02:04:53Z

Is there any chance this PR could be recovered? Are there any other efforts/plans to add Web Crypto?

caspervonb · 2021-04-29T19:22:18Z

Yeah, it's coming soon.

yacinehmito force-pushed the webcrypto branch from cb4a4f8 to 1da94b8 Compare January 4, 2021 10:32

yacinehmito mentioned this pull request Jan 5, 2021

fix(op_crates/crypto): Test crypto.getRandomValues() with wpt #9016

Merged

yacinehmito force-pushed the webcrypto branch from d94ec0c to 5dce656 Compare January 9, 2021 13:16

yacinehmito commented Jan 9, 2021

View reviewed changes

lucacasonato reviewed Jan 9, 2021

View reviewed changes

yacinehmito changed the title ~~feat(runtime): Implement Web Crypto API~~ feat(runtime): Introduce Web Crypto API Jan 9, 2021

yacinehmito force-pushed the webcrypto branch 2 times, most recently from d219dc5 to 7405496 Compare January 9, 2021 15:32

yacinehmito mentioned this pull request Jan 9, 2021

fix(op_crates/crypto): Prefix op of getRandomValues() by op_crypto #9067

Merged

yacinehmito changed the title ~~feat(runtime): Introduce Web Crypto API~~ feat(op_crates/crypto): Introduce Web Crypto API Jan 9, 2021

yacinehmito mentioned this pull request Jan 9, 2021

feat(op_crates/crypto): Implement crypto.subtle.digest() #9069

Closed

yacinehmito force-pushed the webcrypto branch from 7405496 to 6b468b5 Compare January 9, 2021 15:54

yacinehmito force-pushed the webcrypto branch from 6b468b5 to 79e7ecb Compare January 9, 2021 17:56

yacinehmito force-pushed the webcrypto branch 3 times, most recently from 21c610a to 9efdb00 Compare January 30, 2021 11:04

yacinehmito marked this pull request as ready for review January 30, 2021 11:04

yacinehmito requested review from bartlomieju and lucacasonato January 30, 2021 11:05

bartlomieju reviewed Feb 9, 2021

View reviewed changes

yacinehmito marked this pull request as draft February 10, 2021 19:08

Base automatically changed from master to main February 19, 2021 14:58

yacinehmito force-pushed the webcrypto branch 3 times, most recently from d931930 to 3c8a26d Compare February 20, 2021 13:22

Introduce WebCrypto API

9bbde5a

yacinehmito force-pushed the webcrypto branch from 3c8a26d to 9bbde5a Compare February 20, 2021 17:07

stale bot added the stale label Apr 21, 2021

stale bot closed this Apr 29, 2021

		// TODO: Determine whether it's worth exposing just to assert the size
		pub byte_length: usize,

		const supportedAlgorithms = {
		digest: new RegisteredAlgorithmsContainer({

feat(op_crates/crypto): Introduce Web Crypto API #8984

feat(op_crates/crypto): Introduce Web Crypto API #8984

Conversation

yacinehmito commented Jan 4, 2021 • edited Loading

Progress

Design

Type definitions

JavaScript wrapper

crypto.subtle

JS side vs. Rust side

Returning buffers

Underlying crypto library

Testing

yacinehmito commented Jan 4, 2021 • edited Loading

lucacasonato commented Jan 5, 2021

yacinehmito commented Jan 9, 2021

yacinehmito commented Jan 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yacinehmito Jan 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yacinehmito Jan 9, 2021 • edited Loading

Choose a reason for hiding this comment

bartlomieju Jan 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucacasonato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartlomieju commented Jan 9, 2021

yacinehmito commented Jan 9, 2021 • edited Loading

yacinehmito commented Jan 9, 2021

yacinehmito commented Jan 30, 2021

yacinehmito commented Feb 5, 2021

caspervonb commented Feb 5, 2021 • edited Loading

yacinehmito commented Feb 5, 2021

bartlomieju commented Feb 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yacinehmito commented Feb 10, 2021 • edited Loading

caspervonb commented Feb 10, 2021

yacinehmito commented Feb 10, 2021 • edited Loading

yacinehmito commented Feb 20, 2021 • edited Loading

Introduce the ops one by one, then the TypeScript declaration last

Introduce the API first, then the ops one by one

Put the API behind the unstable flag

Use a feature branch

stale bot commented Apr 21, 2021

laughedelic commented Apr 29, 2021

caspervonb commented Apr 29, 2021

yacinehmito commented Jan 4, 2021 •

edited

Loading

yacinehmito commented Jan 4, 2021 •

edited

Loading

yacinehmito Jan 9, 2021 •

edited

Loading

yacinehmito Jan 9, 2021 •

edited

Loading

bartlomieju Jan 9, 2021 •

edited

Loading

yacinehmito commented Jan 9, 2021 •

edited

Loading

caspervonb commented Feb 5, 2021 •

edited

Loading

yacinehmito commented Feb 10, 2021 •

edited

Loading

yacinehmito commented Feb 10, 2021 •

edited

Loading

yacinehmito commented Feb 20, 2021 •

edited

Loading