Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reconsider "in parallel"? #389

Open
annevk opened this issue Dec 19, 2024 · 11 comments
Open

Reconsider "in parallel"? #389

annevk opened this issue Dec 19, 2024 · 11 comments

Comments

@annevk
Copy link
Member

annevk commented Dec 19, 2024

From WebKit's source code:

The 11 December 2014 version of the specification suggests we should perform the following task asynchronously:
https://www.w3.org/TR/WebCryptoAPI/#SubtleCrypto-method-importKey
It is not beneficial for less time consuming operations. Therefore, we perform it synchronously.

This came up in #368 first.

It's probably worth writing some tests to see when the promises are getting resolved relative to the running task.

@annevk
Copy link
Member Author

annevk commented Dec 19, 2024

And also, it seems good to investigate this in general before building on top of the current API shape.

@twiss
Copy link
Member

twiss commented Dec 19, 2024

The way I interpreted @davidben's suggestion in #368 (comment) is not to change the API shape, but rather to do something like:

  1. Let promise be a new Promise.
  2. Return promise and queue a microtask with the remaining steps.
  3. ...
  4. Resolve promise with result.

Given the loose definition of running in parallel I think an implementation is already free to implement stuff as above today?

However, we could also change the spec text to the above (in the cases where running in parallel doesn't matter). That way, we don't have to bother with task queues, while not introducing any observable difference.

In any case, I don't think it's strictly necessary to change the API if we want to run things synchronously. We could add a variant of the functions that's required to run synchronously and returns the result instead of a Promise, for the convenience of callers, as requested in #167 at some point, but that's a slightly orthogonal question.


It's probably worth writing some tests to see when the promises are getting resolved relative to the running task.

This I do agree with :)

If there are already implementations that resolve the Promise before returning it, then perhaps it also doesn't matter as much exactly how we define the above, though it might still be nice to be consistent with everything else.

@annevk
Copy link
Member Author

annevk commented Dec 19, 2024

There's an observable difference between queueing a task and queuing a microtask/resolving the promise immediately. Always queueing a task (and not caring about blocking) could be considered about equivalent to going in parallel and queueing a task and the difference might not be test-observable, but it's not clear this is what implementations actually do. And it's also not clear if that's what we want going forward if we were to do something new.

@twiss
Copy link
Member

twiss commented Dec 19, 2024

OK yeah, I guess it'd need to be a full task and not a microtask, then. But yeah I agree it'd be good to test this first.

@twiss
Copy link
Member

twiss commented Dec 19, 2024

However, for #73 (comment) specifically (which indirectly triggered this discussion), we still need to return a Promise when hashing an AsyncIterable, even if we want to do the hashing on the main thread. So, creating a new API for synchronous hashing (and other operations) actually wouldn't help us there, so I think it's fairly orthogonal.

@annevk
Copy link
Member Author

annevk commented Dec 19, 2024

It would impact queue a task vs resolve immediately, I suspect.

@twiss
Copy link
Member

twiss commented Dec 19, 2024

We can't resolve immediately if we have to wait for the AsyncIterable to produce data; so it'd essentially force us to queue a task as we're doing now, I imagine.

@davidben
Copy link
Collaborator

davidben commented Dec 19, 2024

The way I interpreted @davidben's suggestion in #368 (comment) is not to change the API shape, but rather to do something like: [...]

Huh? We've talked at length about this, so to suggest that I want to keep WebCrypto's API shape is quite surprising. WebCrypto's API shape is bad and we absolutely should be re-evaluating it before building on it. It does not match best practices for either cryptography APIs or JavaScript APIs in general.

  • It was a mistake to make WebCrypto asynchronous. Even if we mitigate it by resolving the promise immediately, we still pay for task or microtask overhead (and inefficiencies up the stack), as well as ergonomics problems, which is significant at the scale of these operations. It also makes it hard for, say, a Wasm module to outsource its cryptography to WebCrypto, if you want to take advantage of CPU acceleration or the underlying library's side channel protections.

  • It was a mistake to make WebCrypto take algorithms by objects with strings. We have a perfectly serviceable way to name APIs (crypto.subtle.sha256 or whatever) and didn't need to invent a meta one.

  • It was a mistake to make WebCrypto impossible to feature-detect. To feature-detect an algorithm in WebCrypto, you have to actually perform it. This is completely inconsistent with how many protocols work, where you have to advertise ahead of time what you support. As a result, feature detection requires you perform an extra operation for no reason.

  • It was a mistake to jam all of WebCrypto into a common set of verbs. See all the problems we've had around truncation and pretending ECDH and HKDF were the same operation.

Instead, we could have just built a very natural, straightfoward set of functions that go from bytes to bytes, matching what everyone else does. For example hashing could simply have been a family of crypto.subtle.sha256(), crypto.subtle.sha384(), etc. functions. And if we wanted streaming, the standard, low-level API would be something like new crypto.subtle.SHA256Context() giving back an object with update() and final() methods.

Of course, that still doesn't answer what to do with the current API. The current API must be retained for compatibility, in which case we probably need to mitigate its badness as much as we can. The mitigation will be imperfect, so we still need to build a replacement API, but I agree that just running the operations immediately is a good idea.

How much to enshrine in the spec, I'm not sure. There are a lot of open questions in my mind around classifying operations as fast or slow (which we really shouldn't have to do), etc. And Chromium does not currently run them immediately (but I think we should). So we could simply make the spec admit both implementations, or we could go change the spec now. The latter runs the risk that we'll find we changed the spec prematurely, which has been a problem here before. Though that Safari already does this is definitely promising.

@twiss
Copy link
Member

twiss commented Dec 19, 2024

Yes, I'm aware that you think the API is bad and was a mistake :) All of your points are valid. However, since you said (in the comment I linked)

I think we probably should make Chromium run it on the same thread (which further demonstrates that this was just a bad API because now we're paying for task overhead for no reason)

I read that as talking specifically about running the functions as currently defined on the same thread, and then schedule a task to resolve the Promise which that API prescribes to return. Otherwise, if we change the API shape the comment wouldn't make any sense since we wouldn't be paying for the overhead of queuing a task :)

Of course, that still doesn't answer what to do with the current API. The current API must be retained for compatibility, in which case we probably need to mitigate its badness as much as we can. The mitigation will be imperfect, so we still need to build a replacement API, but I agree that just running the operations immediately is a good idea.

How much to enshrine in the spec, I'm not sure. There are a lot of open questions in my mind around classifying operations as fast or slow (which we really shouldn't have to do), etc. And Chromium does not currently run them immediately (but I think we should). So we could simply make the spec admit both implementations, or we could go change the spec now. The latter runs the risk that we'll find we changed the spec prematurely, which has been a problem here before. Though that Safari already does this is definitely promising.

The spec already allows running things on the main thread (although it does require queuing a task to resolve the promise indeed). So if you want to experiment with running stuff on the main thread that can be done without any changes to the spec. I'd propose we start with that before we consider changing the spec or (even more drastically) creating an entirely new API.

@davidben
Copy link
Collaborator

Oh, no implementation work we do to save this API will change the need to make a new WebCrypto API if we want a performant and usable way for the web platform to get at cryptography. We don't need to block that question on anything.

@twiss
Copy link
Member

twiss commented Dec 19, 2024

Sure, I just don't think it's very useful to mix every single issue with "no, we need to throw everything away and start over". The question of whether we want to run the current functions on the main thread is somewhat orthogonal from whether we want a new synchronous API. And the question of streaming is again orthogonal because we can't hash an AsyncIterable synchronously anyway.

Also, creating an entirely new API seems fairly out of scope for the current charter of the WebAppSec WG, which says:

The WG may adopt well-supported proposals from incubation for maintenance of the Web Cryptography API

(emphasis mine). So, if you want to advocate for creating a new API, please also propose to expand the charter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants