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

On Browser DOM scenarios use the SubtleCrypto API #49511

Closed
wants to merge 36 commits into from

Conversation

AaronRobinsonMSFT
Copy link
Member

See https://github.com/AaronRobinsonMSFT/SyncOverAsync for a design overview of the implementation here.

Added new System.Security.Cryptography.Native.Browser native library.

/cc @bartonjs @GrabYourPitchforks @lewing @jkoritzinsky @elinor-fung

Consume the library via new P/Invokes in a Browser impl of the Crypto API.
Tell the WASM build about System.Security.Cryptography.Native.Browser
  in order to generated the pinvoke-table.
Defer to the Crypto worker for SHA hashing functions.
 - Moved ChannelLib to be pre-js so worker init can happen earlier.
Add function for WebWorker shutdown. This is needed to not deadlock test
harness.
Document where SubtleCrypto API was looked for in the v8 test harness. It
was not found.
@ghost
Copy link

ghost commented Mar 11, 2021

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

See https://github.com/AaronRobinsonMSFT/SyncOverAsync for a design overview of the implementation here.

Added new System.Security.Cryptography.Native.Browser native library.

/cc @bartonjs @GrabYourPitchforks @lewing @jkoritzinsky @elinor-fung

Author: AaronRobinsonMSFT
Assignees: -
Labels:

arch-wasm, area-System.Security

Milestone: 6.0.0

@ghost
Copy link

ghost commented Mar 11, 2021

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

See https://github.com/AaronRobinsonMSFT/SyncOverAsync for a design overview of the implementation here.

Added new System.Security.Cryptography.Native.Browser native library.

/cc @bartonjs @GrabYourPitchforks @lewing @jkoritzinsky @elinor-fung

Author: AaronRobinsonMSFT
Assignees: -
Labels:

arch-wasm, area-System.Security

Milestone: 6.0.0

@lewing
Copy link
Member

lewing commented Mar 11, 2021

For background: This implements the platform hash functions on browser when both Subtle.crypto and SharedArrayBuffer are available. This is currently true for Edge, Chrome and Firefox but untrue for Safari which doesn't currently support SharedArrayBuffer.

@kjpou1
Copy link
Contributor

kjpou1 commented Mar 15, 2021

Just to open for discussion with the naming. crypto_worker.js seems a little generic and may have a naming clash. Something less generic and more obvious to identify the process would be dotnet_crypto_worker.js.

This is definitely not a showstopper.

@AaronRobinsonMSFT
Copy link
Member Author

Just to open for discussion with the naming. crypto_worker.js seems a little generic and may have a naming clash. Something less generic and more obvious to identify the process would be dotnet_crypto_worker.js.

That is a reasonable name. I will update to that.

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Mar 31, 2021

I know this is a total pain, but it would be great to have a sense of what sorts of data sizes can realistically be processed on low-end hardware via these APIs before the operation would have to be aborted (or the app typically gets killed). This would help to clarify whether the timeout concerns are just a theoretical edge case we can ignore, or whether this approach would really solve customer problems or create new ones.

When we decided to implement these APIs for WebAssembly, was this based on any particular customer requests, and is there any info about what the scenarios were (and hence how much data they are likely to be hashing/encrypting/decrypting)?

Another possible approach we could consider (which would also avoid the Safari problem) is supporting a mode where all the .NET code runs in a worker thread - something we considered very early on but didn't pursue due to lack of a reason. This would complicate numerous things for how we implement Blazor, but for customers who are willing to give up on unmarshalled interop, would mean we could freely block the .NET execution any time we want. Update: No wait, Safari still wouldn't support blocking unless it supported SharedArrayBuffer. So I guess the asyncify solution would still be much more flexible.

To be clear, I really don't have any agenda to derail your solution here. If it works with the realistic customer scenarios we have in mind and are OK with the Safari limitation, then it's much easier than changing how the whole runtime is hosted!

@AaronRobinsonMSFT
Copy link
Member Author

AaronRobinsonMSFT commented Mar 31, 2021

I know this is a total pain, but it would be great to have a sense of what sorts of data sizes can realistically be processed on low-end hardware via these APIs before the operation would have to be aborted (or the app typically gets killed).

@SteveSandersonMS Not a pain at all. These are all important questions to ensure we provide stable platform support. I'm not sure the best way to collect this data. I can spin something up in a local browser environment and push it to the limit, but a mobile scenario is not something I'm the best person to collect.

supporting a mode where all the .NET code runs in a worker thread - something we considered very early on but didn't pursue due to lack of a reason.

Yeah... that is likely a much bigger work item than I can address in the time I've allocated for this work. It would also be something @lewing, @kg, or one of the real WASM experts should weight in on.

@danmoseley
Copy link
Member

danmoseley commented Mar 31, 2021

Or, we could simulate blocking via re-entrancy (same as the "asyncify" tool you linked in the design doc).

Just for my education, how would this be done? It sounds like "pumping messages" in Windows terms. Perhaps I'm forgetting how one does this in Javascript. (Popping an alert?)

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Mar 31, 2021

Just for my education, how would this be done? It sounds like "pumping messages" in Windows terms. Perhaps I'm forgetting how one does this in Javascript.

Yes, more or less. The .NET interpreter could return from its invocation before finishing the code it's interpreting through, keeping track of all the state it needs to resume from that instruction on a subsequent call in from JS. As far as the .NET code is concerned, the universe was frozen for a while (like being blocked) but no other difference in behavior is observable. We actually did this on the original DotNetAnywhere runtime (before the migration to Mono WebAssembly) in order to support some sync calls.

However JavaScript code would be able to observe this happening. Synchronous (blocking) calls from JS to .NET would cease to be blocking if any part of the callee's operations involved one of these .NET-universe-suspending phases. We would need some way to make this an error, i.e., when calling from JS to .NET we'd have to know whether the caller is expecting to get back a promise or not, and if they are not that means they want the call to be synchronous, and therefore any suspension becomes an exception. If the caller is getting back a promise, then we could so some stuff in JS to make the .NET-universe-suspended-phase part of the JS promise so the caller can't tell the difference.

In summary, it's complicated and still has edge cases. It might be the best available solution even so. I have no idea how this would interact with AOT, however.

@AaronRobinsonMSFT
Copy link
Member Author

In summary, it's complicated and still has edge cases. It might be the best available solution even so.

Agree. It could also be done in vNext if the current solution is an acceptable stop-gap for the current level of support.

Also I would very much recommend going down the path of adding support for Asyncify, #48713, rather than doing some bespoke for Blazor.

@bartonjs
Copy link
Member

we'd have to know whether the caller is expecting to get back a promise or not, and if they are not that means they want the call to be synchronous, and therefore any suspension becomes an exception

I don't know how the JS -> .NET calls work, but would it make sense to just make them all be promises in that case? Sure, it's how we got into this problem in the first place, but it's sort of a "when in Rome..."

@SteveSandersonMS
Copy link
Member

We already support both synchronous and asynchronous interop APIs. Both are valuable for different use cases.

@marek-safar
Copy link
Contributor

@SteveSandersonMS we'll also support "proper" async crypto in the form of new APIs. We discussed that with Jeremy to look into if it's worth having that as a generic API or whether we will add it as browser-specific. This has lower priority because we expect the usage to be lower than existing crypto APIs but it'd be the right options for those targetting the primary web.

/cc @lewing

@lewing
Copy link
Member

lewing commented Apr 21, 2021

@AaronRobinsonMSFT I'm happy to resolve the conflict or something like should fine for now.

     <_DotnetJSSrcWorkerFile Include="$(_WasmRuntimePackSrcDir)pal_crypto_webworker.js" />
     <_DotnetJSSrcPreFile Include="$(_WasmRuntimePackSrcDir)library_channel.js" />

     <_DotnetJSSrcFile Include="$(_WasmRuntimePackSrcDir)\*.js" />
     <_DotnetJSSrcFile Remove="@(_DotnetJSSrcWorkerFile)" />
     <_DotnetJSSrcFile Remove="@(_DotnetJSSrcPreFile)" />

@SteveSandersonMS
Copy link
Member

@lewing I see you're doing some work to make this PR build pass, but have we reached a conclusion on whether it should be merged?

The last I understood (as per this comment) was that we saw it would be necessary to do some more research to determine the realistic scenarios and find out whether the spin-blocking was actually viable for those scenarios, or whether it would just result in people's apps being terminated by the browser.

@lewing
Copy link
Member

lewing commented Apr 23, 2021

@lewing I see you're doing some work to make this PR build pass, but have we reached a conclusion on whether it should be merged?

The last I understood (as per this comment) was that we saw it would be necessary to do some more research to determine the realistic scenarios and find out whether the spin-blocking was actually viable for those scenarios, or whether it would just result in people's apps being terminated by the browser.

Your understanding is still correct. I'm keeping this PR green while we discuss (for testing) and also making sure the native build work can support similar changes.

@jeffhandley
Copy link
Member

jeffhandley commented May 11, 2021

I pushed a merge with main that resolved the conflict introduced from #52303.

@stephentoub stephentoub added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label May 26, 2021
@jeffhandley
Copy link
Member

I'm going to convert this PR to a draft for now, to be revisited again soon.

@jeffhandley jeffhandley marked this pull request as draft July 3, 2021 06:50
@lewing lewing modified the milestones: 6.0.0, 7.0.0 Jul 6, 2021
@jeffhandley
Copy link
Member

Assigning to @GrabYourPitchforks to track the need of documenting guidance for 6.0 and our plan for 7.0.

@ghost ghost closed this Aug 22, 2021
@ghost
Copy link

ghost commented Aug 22, 2021

Draft Pull Request was automatically closed for inactivity. Please let us know if you'd like to reopen it.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 21, 2021
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the wasm_crypto branch June 18, 2022 05:02
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-System.Security NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.