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

feat: Add support for SubtleCrypto #440

Closed
wants to merge 6 commits into from

Conversation

JakeChampion
Copy link
Contributor

This adds an implementation of a subset of SubtleCrypto, specifically for JSON Web Token signing and validating.

  • SubtleCrypto.prototype.generateKey
  • SubtleCrypto.prototype.importKey
  • SubtleCrypto.prototype.sign
  • SubtleCrypto.prototype.verify with the following algorithms:
  • RSASSA_PKCS1_v1_5
  • RSA_OAEP and the following digest algorithms:
  • SHA_1
  • SHA_224
  • SHA_256
  • SHA_384
  • SHA_512

Work in the future will be done to add the remaining algorithms and SubtleCrypto method implementations

@JakeChampion JakeChampion force-pushed the jake/subtlecrypto-verify branch 2 times, most recently from 5500fba to ad97434 Compare March 20, 2023 18:08
This adds an implementation of a subset of SubtleCrypto, specifically for JSON Web Token signing and validating.
- SubtleCrypto.prototype.generateKey
- SubtleCrypto.prototype.importKey
- SubtleCrypto.prototype.sign
- SubtleCrypto.prototype.verify
with the following algorithms:
- RSASSA_PKCS1_v1_5
- RSA_OAEP
and the following digest algorithms:
- SHA_1
- SHA_224
- SHA_256
- SHA_384
- SHA_512

Work in the future will be done to add the remaining algorithms and SubtleCrypto method implementations
@JakeChampion JakeChampion marked this pull request as ready for review March 20, 2023 18:11

constexpr uint8_t nonAlphabet = 255;

static const uint8_t base64DecodeTable[] = {
Copy link
Contributor

@elliottt elliottt Mar 21, 2023

Choose a reason for hiding this comment

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

Could you move this back to js-compute-builtins.cpp and make this declaration extern const uint8_t base64DecodeTable[]; instead? Otherwise this constant will be inlined into every file that includes it, adding a lot of duplicate declarations of the same data that won't inline.

Comment on lines +356 to +357
inline JS::Result<mozilla::Ok> base64Decode4to3(std::string_view input, std::string &output,
const uint8_t *decodeTable) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What drove making the decode table a parameter here?

Comment on lines +2 to +4
"Context is discarded in generateKey": {
"status": "FAIL"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these all expected to fail with this pr?

Copy link
Contributor

@elliottt elliottt left a comment

Choose a reason for hiding this comment

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

Would you mind pulling out all the new json test results into a separate pr? That way we could see what the diff in behavior between main and this pr is.

@@ -0,0 +1,942 @@
#pragma once
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you switch to header guards? If we ever change build systems, #pragma once could become problematic.

@JakeChampion JakeChampion marked this pull request as draft March 22, 2023 15:37
@JakeChampion
Copy link
Contributor Author

Changing this to a draft as I will be breaking the PR up into a bunch of smaller PRs

@panva
Copy link

panva commented May 4, 2023

If I could lobby for a few more algorithms to be supported 🙏

Also pkcs8 and spki key format import/export are not to be overlooked. Developers LOOOVE to use in favour of JWKs.

I have a comprehensive test suite for different runtimes' webcrypto implementations that I could unload on js-compute-runtime eventually.

@stefan-guggisberg
Copy link

stefan-guggisberg commented May 15, 2023

@JakeChampion We are using 3rd party dependencies relying on subtle.crypto, e.g. for signing AWS requests.

We're still getting errors like Supplied algorithm is not yet supported.

Here's a code fragment from https://github.com/mhart/aws4fetch which seems to trigger this error:

async function hmac(key2, string3) {
  const cryptoKey = await crypto.subtle.importKey(
    "raw",
    typeof key2 === "string" ? encoder2.encode(key2) : key2,
    { name: "HMAC", hash: { name: "SHA-256" } },
    false,
    ["sign"]
  );
  return crypto.subtle.sign("HMAC", cryptoKey, encoder2.encode(string3));
}

It seems like the lack of HMAC support is causing the error.

This is a blocker for us. Without this support we won't be able to use C@E.

Can you give us an ETA when this PR finally lands in production?

@mlegenhausen
Copy link

mlegenhausen commented May 17, 2023

For a complete move to compute edge we would need support for the following API

  • SubtleCrypto.prototype.encrypt
  • SubtleCrypto.prototype.decrypt

Both with support for AES-CBC and AES-GCM

@JakeChampion
Copy link
Contributor Author

Thanks all -- we've added HMAC and raw importkey support and I have opened issues for the other requests which have been added to this draft pull-request.

I will be closing this pull-request as it is never going to be merged, all the work is happening in other, smaller pull-requests 👍

@JakeChampion JakeChampion deleted the jake/subtlecrypto-verify branch August 9, 2023 23:31
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

Successfully merging this pull request may close these issues.

None yet

5 participants