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.

Sign up for GitHub

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

Jump to bottom

Use SubtleCrypto API on browser DOM scenarios #65966

Merged
merged 22 commits into from
May 24, 2022
Merged

Use SubtleCrypto API on browser DOM scenarios #65966

merged 22 commits into from
May 24, 2022

Conversation

layomia
Copy link
Contributor

@layomia layomia commented Feb 28, 2022 •
edited
Loading

Contributes to #40074. For WASM, uses the SubtleCrypto browser-native implementation for the SHA1, SHA256, SHA384, SHA512 algorithms, and adds infrastructure to support using the native implementation. Support for the rest of the algorithms will come in a single follow-up PR.

@layomia layomia added this to the 7.0.0 milestone Feb 28, 2022
@layomia layomia self-assigned this Feb 28, 2022
@ghost
Copy link

ghost commented Feb 28, 2022

Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: layomia
Assignees: layomia
Labels:

area-System.Security
Milestone: 7.0.0

@runfoapp runfoapp bot mentioned this pull request Feb 28, 2022
Closed
bartonjs
bartonjs reviewed Mar 1, 2022
View reviewed changes
src/mono/wasm/wasm.proj Outdated Show resolved Hide resolved
@pavelsavara
Copy link
Member

I wonder if we should rather add it to main typescript codebase in src\mono\wasm\runtime\, instead of separate src/mono/wasm/runtime/library_channel.js

@layomia layomia force-pushed the WasmCrypto branch 2 times, most recently from 235212a to 4f88dee Compare March 16, 2022 17:20
@layomia layomia force-pushed the WasmCrypto branch 3 times, most recently from c292c73 to 16b0267 Compare April 13, 2022 00:27
@layomia layomia force-pushed the WasmCrypto branch 6 times, most recently from 651d0a4 to ac117cb Compare April 13, 2022 21:39
@dotnet dotnet deleted a comment from azure-pipelines bot May 17, 2022
@layomia
Copy link
Contributor Author

layomia commented May 17, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@layomia layomia mentioned this pull request May 18, 2022
Closed
@layomia
Copy link
Contributor Author

layomia commented May 18, 2022

I tried it locally, and the tests pass with --web-server-use-cop used instead of the browser-arg.
You might also want to print the hash, and check that, to make sure that everything worked as expected.

Done, this works thanks.

AaronRobinsonMSFT
AaronRobinsonMSFT reviewed May 19, 2022
View reviewed changes
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

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

LGTM.

src/mono/wasm/runtime/crypto-worker.ts
msg_char_len: chan.get_msg_len()
});
worker.onerror = event => {
console.warn(`MONO_WASM: Error in Crypto WebWorker. Cryptography digest calls will fallback to managed implementation. Error: ${event.message}`);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we want this fallback. I thought it was all or nothing. @bartonjs?

Copy link
Member

Choose a reason for hiding this comment

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

All or nothing was the original plan. But since Safari can't (at least, couldn't) do it we got authorization to do native-preferred with managed fallback. (That's why we already have managed SHA1/SHA256/SHA384/SHA512 checked in for wasm)

Copy link
Member

Choose a reason for hiding this comment

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

Safari is always a no, right? The never okay I get and that check is at the top of this method. However, this error callback path seems to imply that if a catastrophic error occurs during a call where the SubtleCrypto libraries are supported, on the next call we will fallback to the managed implementation. The message might be confusing me, but setting mono_wasm_crypto to null after it was set appears we are falling back on a supported platform. Is that fallback also supported?

Copy link
Member

Choose a reason for hiding this comment

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

Oooh. Um, no, that probably shouldn't fall back. I think that we're expecting to be able to say "these are the characteristics, and if your browser has them then you get native crypto (which may or may not be FIPS/etc-acceptable) over managed crypto (which never is)". Changing after the first call seems... bad.

@blowdart or @GrabYourPitchforks may wish to disagree and say that if stuff breaks it's OK to fall back to managed... but I think that makes Barry's job harder, so I don't expect they'll feel differently.

Copy link
Contributor

Choose a reason for hiding this comment

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

I seem to remember discussing this and talking about addressing it with documentation and no promises ever of fips compliance (I still hate it though). @GrabYourPitchforks does that ring a bell?

Copy link
Member

Choose a reason for hiding this comment

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

Because of https://github.com/dotnet/runtime/pull/65966/files#diff-46676f417228802cba4f54049a643b76f11d4b8feb4953f39cb439ecaeba4097R10, I don't think this is a dynamic fallback... just the startup code. But it'd be good to get full closure on this in the fullness of time.

src/mono/wasm/runtime/crypto-worker.ts Show resolved Hide resolved
radical
radical reviewed May 19, 2022
View reviewed changes
src/mono/wasm/runtime/crypto-worker.ts Show resolved Hide resolved
src/mono/wasm/runtime/dotnet-crypto-worker.js
Comment on lines +31 to +34
// Read in request
var req = this._read_request();
if (req === this.STATE_SHUTDOWN)
break;
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be in try/catch block too, so we can send an error response?

src/mono/wasm/runtime/dotnet-crypto-worker.js
}
catch (err) {
console.log("Request error: " + err);
resp = JSON.stringify(err);
Copy link
Member

@radical radical May 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Should there be a way to indicate that this is returning an error, or that the operation failed? Do IUC, that the caller will get the response, and if the length happens to match the expected one, then it will think that the error response is actually valid?

src/mono/wasm/runtime/crypto-worker.ts
return ii - start;
}

private read_response(): string {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should detect that the response was an error, and throw

bartonjs
bartonjs approved these changes May 24, 2022
View reviewed changes
Copy link
Member

@bartonjs bartonjs left a comment

Choose a reason for hiding this comment

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

C# LGTM. The JS looks as reasonable as I can assess it to be.

lewing
lewing approved these changes May 24, 2022
View reviewed changes
Copy link
Member

@lewing lewing left a comment

Choose a reason for hiding this comment

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

I think we can land it in this state and address any remaining issues in follow up work

@AaronRobinsonMSFT AaronRobinsonMSFT mentioned this pull request May 24, 2022
Closed
@lewing lewing mentioned this pull request May 24, 2022
Closed
@radical
Copy link
Member

radical commented May 24, 2022 •
edited
Loading

Browser builds failing with:

  exports.ts → ../../../../artifacts/bin/native/net7.0-Browser-Release-wasm/src/cjs/runtime.cjs.iffe.js, ../../../../artifacts/bin/native/net7.0-Browser-Release-wasm/src/es6/runtime.es6.iffe.js...
  [!] (plugin typescript) Error: @rollup/plugin-typescript TS2305: Module '"./types"' has no exported member 'assert'.
  crypto-worker.ts (5:10)
  
  5 import { assert } from "./types";
             ~~~~~~
  
  Error: @rollup/plugin-typescript TS2305: Module '"./types"' has no exported member 'assert'.
      at error (/__w/1/s/src/mono/wasm/runtime/node_modules/rollup/dist/shared/rollup.js:151:30)
      at throwPluginError (/__w/1/s/src/mono/wasm/runtime/node_modules/rollup/dist/shared/rollup.js:19344:12)
      at Object.error (/__w/1/s/src/mono/wasm/runtime/node_modules/rollup/dist/shared/rollup.js:20013:20)
      at emitDiagnostic (/__w/1/s/src/mono/wasm/runtime/node_modules/@rollup/plugin-typescript/dist/index.js:511:17)
      at reportDiagnostics (/__w/1/s/src/mono/wasm/runtime/node_modules/@rollup/plugin-typescript/dist/index.js:519:9)
      at Array.forEach (<anonymous>)
      at emitFilesAndReportErrors (/__w/1/s/src/mono/wasm/runtime/node_modules/typescript/lib/typescript.js:117783:21)
      at Object.result.afterProgramCreate (/__w/1/s/src/mono/wasm/runtime/node_modules/typescript/lib/typescript.js:117961:13)
      at Object.afterProgramCreate (/__w/1/s/src/mono/wasm/runtime/node_modules/@rollup/plugin-typescript/dist/index.js:674:29)
      at synchronizeProgram (/__w/1/s/src/mono/wasm/runtime/node_modules/typescript/lib/typescript.js:118258:22)

@radical
Copy link
Member

radical commented May 24, 2022

It was renamed to mono_assert.

@radical
Copy link
Member

radical commented May 24, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@radical
Copy link
Member

radical commented May 24, 2022

The wasm/aot test failure is #69681 , and unrelated to this PR.

@ericstj ericstj merged commit bfbb783 into dotnet:main May 24, 2022
@eerhardt eerhardt mentioned this pull request Jun 14, 2022
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Jun 24, 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.
Reviewers

@pavelsavara pavelsavara pavelsavara left review comments

@blowdart blowdart blowdart left review comments

@jkotas jkotas jkotas left review comments

@ericstj ericstj ericstj left review comments

@radical radical radical approved these changes

@lewing lewing lewing approved these changes

@bartonjs bartonjs bartonjs approved these changes

@AaronRobinsonMSFT AaronRobinsonMSFT AaronRobinsonMSFT approved these changes

@SteveSandersonMS SteveSandersonMS Awaiting requested review from SteveSandersonMS

@stephentoub stephentoub Awaiting requested review from stephentoub

@GrabYourPitchforks GrabYourPitchforks Awaiting requested review from GrabYourPitchforks

@marek-safar marek-safar Awaiting requested review from marek-safar

Assignees

@layomia layomia

Labels
area-System.Security needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.
10 participants
@layomia @pavelsavara @AaronRobinsonMSFT @bartonjs @radical @ericstj @lewing @blowdart @stephentoub @jkotas