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: Implement subset of crypto.subtle.importKey which can import a JSONWebKey using RSASSA-PKCS1-v1_5 #472

Merged
merged 12 commits into from Apr 9, 2023

Conversation

JakeChampion
Copy link
Contributor

No description provided.

@JakeChampion JakeChampion force-pushed the jake/crypto-5 branch 5 times, most recently from 5b2f113 to 873819c Compare March 30, 2023 12:22
@JakeChampion JakeChampion marked this pull request as ready for review March 30, 2023 13:10
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.

This is looking great!

Comment on lines +383 to +386
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wdeprecated-declarations"
auto rsa = RSA_new();
#pragma clang diagnostic pop
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 add an issue to update this according to the migration guide? It looks like there's an alternate api that we should use instead.

https://www.openssl.org/docs/man3.0/man7/migration_guide.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Issue at #477

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.

This looks great! Just a few suggestions before merging 👍

Co-authored-by: Trevor Elliott <telliott@fastly.com>
@JakeChampion JakeChampion merged commit 110e7f4 into main Apr 9, 2023
63 checks passed
@JakeChampion JakeChampion deleted the jake/crypto-5 branch April 9, 2023 15:43
@stefan-guggisberg
Copy link

@JakeChampion Thanks for landing this PR in 1.7.0.

I was hoping this would solve our problem (3rd party dependencies relying on subtle.crypto e.g. for signing AWS requests. However, we're now seeing different errors, such as 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.

Would this PR solve our problem? #440

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

@JakeChampion
Copy link
Contributor Author

@stefan-guggisberg - #440 does solve that issue 👍 - I am breaking that PR up into smaller more manageable PR for the team to review. If you subscribe to #440, when it is closed you should be able to run the code you've posted without any issues 👍

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

3 participants