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

Use crypto.subtle for HMAC on Browser WASM #70745

Merged
merged 3 commits into from
Jun 21, 2022

Conversation

eerhardt
Copy link
Member

@eerhardt eerhardt commented Jun 14, 2022

Implement the browser "native" portion for HMAC on Browser WASM.

I also made a few refactoring / simplifications where necessary.

Contributes to #40074

This should probably wait for #70461 to be merged, to ensure both paths (managed and crypto.subtle) are being tested in CI. This is now merged - so both SubtleCrypto and managed fallback implementations are being tested in CI.

@ghost
Copy link

ghost commented Jun 14, 2022

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

Issue Details

Implement the browser "native" portion for HMAC on Browser WASM.

I also made a few refactoring / simplifications where necessary.

Contributes to #40074

This should probably wait for #70461 to be merged, to ensure both paths (managed and crypto.subtle) are being tested in CI.

Author: eerhardt
Assignees: -
Labels:

arch-wasm, area-System.Security

Milestone: -

@eerhardt eerhardt requested a review from vcsjones June 14, 2022 19:03
public override void AppendHashData(ReadOnlySpan<byte> data)
{
_buffer ??= new MemoryStream(1000);
_buffer.Write(data);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this buffering all of the data that's used to compute the hash?

Copy link
Member

@vcsjones vcsjones Jun 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. There is no other way with SubtleCrypto, currently. The browser-provided implementations are one-shots. You need to give it all of the data up-front. See w3c/webcrypto#73

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it could be a real pit of failure, especially if we may sometimes use SubtleCrypto and sometimes use the managed implementation based on configuration.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a real pit of failure

A performance pit of feature because of memory usage? Or something else?

and sometimes use the managed implementation based on configuration.

From my understanding, it is based on the browser's capabilities and even the settings/headers of the page it is hosted in. See https://developer.chrome.com/blog/enabling-shared-array-buffer/ for when we can use SharedArrayBuffer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, note that this is how the SHA algos are implemented as well in #65966.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A performance pit of feature because of memory usage? Or something else?

Memory, and then any knock-on effects from that, e.g. exceptions that result from the memory pressure (do the wasm environments enforce any kind of limits on working set size)? I'm just imagining someone downloading a gig of data to hash, expecting it to just stream through, but instead having the whole thing be buffered.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do the wasm environments enforce any kind of limits on working set size)

WASM is 32-bit, currently, so you have 4 GB of address space to work with, at most. In practice, v8 (Chromium)'s limit is more like 2 GB: https://v8.dev/blog/4gb-wasm-memory. The post is a few years old but I am not aware of that needle moving since then.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reality is even more complicated WebAssembly/design#1397 and because of that it can definitely make sense to do the buffering on the js side rather than in managed but I expect most of the uses on this would look more like https://en.wikipedia.org/wiki/JSON_Web_Token than a large binary

@eerhardt
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Implement the browser "native" portion for HMAC on Browser WASM.

I also made a few refactoring / simplifications where necessary.

Contributes to dotnet#40074
@eerhardt
Copy link
Member Author

I believe this PR is ready for review. I'd like to get it merged by the next preview cutoff so HMAC is completely implemented on WASM.

@eerhardt
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@radical
Copy link
Member

radical commented Jun 17, 2022

The Wasm.Build.Tests failures are due to some blazor tests getting oomkill'ed, unrelated to this PR. They can be ignored here.

Copy link
Member

@radical radical left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with the crypto side. The JS side of things look fine to me, and in line with the existing code. But I'm not an expert on it!

@eerhardt
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@eerhardt
Copy link
Member Author

runtime (Build Linux arm64 Release NativeAOT) failure is #70010

@eerhardt eerhardt merged commit eed18af into dotnet:main Jun 21, 2022
@eerhardt eerhardt deleted the HMacNativeBrowser2 branch June 21, 2022 21:27
@ghost ghost locked as resolved and limited conversation to collaborators Jul 23, 2022
@bartonjs bartonjs added the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Aug 26, 2022
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 needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants