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

Add ncrypto dependency, implement key handling operations #3463

Merged
merged 24 commits into from
Feb 14, 2025

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Feb 4, 2025

Still very much a work in progress.

This is gonna be a big one.

@jasnell jasnell requested a review from anonrig February 4, 2025 23:18
@jasnell jasnell force-pushed the jasnell/start-adding-ncrypto branch 3 times, most recently from dc7c1a4 to b1546c4 Compare February 5, 2025 01:34
@jasnell jasnell force-pushed the jasnell/start-adding-ncrypto branch from 050b394 to 82287e0 Compare February 6, 2025 00:31
Copy link

github-actions bot commented Feb 6, 2025

The generated output of @cloudflare/workers-types matches the snapshot in types/generated-snapshot 🎉

@jasnell jasnell force-pushed the jasnell/start-adding-ncrypto branch 2 times, most recently from 6492b8c to ac64627 Compare February 6, 2025 21:09
@jasnell jasnell force-pushed the jasnell/start-adding-ncrypto branch 2 times, most recently from 71b1cb4 to 7f8727a Compare February 7, 2025 19:39
@jasnell jasnell force-pushed the jasnell/start-adding-ncrypto branch 4 times, most recently from b231446 to f1adaf1 Compare February 8, 2025 00:30
@jasnell jasnell force-pushed the jasnell/start-adding-ncrypto branch from f1adaf1 to c32c8ba Compare February 10, 2025 16:53
@jasnell jasnell force-pushed the jasnell/start-adding-ncrypto branch 2 times, most recently from 2143423 to 71959bb Compare February 11, 2025 18:01
@anonrig
Copy link
Member

anonrig commented Feb 12, 2025

Amazing work as always @jasnell. There are some TS related comments that I'm not happy about, but we can fix it afterwards. No need to block progress!

@jasnell jasnell force-pushed the jasnell/start-adding-ncrypto branch from 71959bb to 13de810 Compare February 13, 2025 17:55
@jasnell
Copy link
Member Author

jasnell commented Feb 13, 2025

While this is generally ready to go for workerd, the production project uses a different version of boringssl that will require some additional tweaks in ncrypto to get working. This PR will remain in draft until those are done.

@jasnell jasnell force-pushed the jasnell/start-adding-ncrypto branch from 45074d7 to 56857fc Compare February 13, 2025 23:04
@jasnell jasnell force-pushed the jasnell/start-adding-ncrypto branch from 56857fc to 186fa11 Compare February 14, 2025 20:13
@jasnell jasnell changed the title [WIP] Add ncrypto dependency Add ncrypto dependency, implement key handling operations Feb 14, 2025
@jasnell jasnell marked this pull request as ready for review February 14, 2025 20:16
@jasnell jasnell requested review from a team as code owners February 14, 2025 20:16
@jasnell jasnell requested review from dom96 and vickykont February 14, 2025 20:16
@vicb
Copy link
Contributor

vicb commented Feb 14, 2025

Does this unblock some of

// Classes
or is this PR only adding thing that will be used later to implement those?

@jasnell
Copy link
Member Author

jasnell commented Feb 14, 2025

@vicb ... it adds implementations for the following:

  • createPrivateKey()
  • createPublicKey()
  • generateKeyPair()
  • generateKeyPairSync()
  • PublicKeyObject
  • PrivateKeyObject

With caveats, of course. Key generation of DSA and DH key pairs do not work, for instance, due to current limitations in boringssl. There will be several followup PRs soon with more implementation detail.

@jasnell
Copy link
Member Author

jasnell commented Feb 14, 2025

@panva ... fyi ... just a heads up that this is landing.

@jasnell jasnell merged commit 69ce716 into main Feb 14, 2025
16 of 18 checks passed
@jasnell jasnell deleted the jasnell/start-adding-ncrypto branch February 14, 2025 20:51
@panva
Copy link
Contributor

panva commented Feb 14, 2025

@jasnell thanks for the heads up. I think it's a move in the right direction for compatibility.

That being said I'm switching jose fully to WebCryptoAPI with a new major and ESM-only as soon as 20.19.0 releases with unflagged require(esm). It will still support KeyObject as inputs (and will use KeyObject.prototype.toCryptoKey to crossover KeyObject to CryptoKey efficiently) and will also use globalThis?.process?.getBuiltinModule?.(id) for small optimizations when available here and there, e.g. new X509Certificate(pem).publicKey).

If you don't support KeyObject.prototype.toCryptoKey yet I'd suggest getting that in.

@jasnell
Copy link
Member Author

jasnell commented Feb 14, 2025

Yep, KeyObject.prototype.toCryptoKey(...) will be coming soon (hopefully in the next couple of weeks).

@vicb
Copy link
Contributor

vicb commented Feb 15, 2025

use globalThis?.process?.getBuiltinModule?.(id)

@panva could you expand how you will use that? FYI when using node compatibility with workerd, the return value could be different from an import.

@panva
Copy link
Contributor

panva commented Feb 15, 2025

@vicb here are all three instances in the v6.x-wip state at the moment

https://github.com/panva/jose/blob/151000e84b94551e7408f988e3d52f8ccf530a64/src/lib/asn1.ts#L253-L266
https://github.com/panva/jose/blob/151000e84b94551e7408f988e3d52f8ccf530a64/src/lib/buffer_utils.ts#L6-L21 (used in other files for base64url handling, this will be replaced with https://github.com/tc39/proposal-arraybuffer-base64 when available)
https://github.com/panva/jose/blob/151000e84b94551e7408f988e3d52f8ccf530a64/src/lib/decrypt.ts#L11-L33

I am specifically not interested in using an import() here because of how bundlers tend to either error out or insert shitty polyfills when they detect node modules in non-node runtime targets.

@vicb
Copy link
Contributor

vicb commented Feb 15, 2025

@vicb here are all three instances in the v6.x-wip state at the moment

https://github.com/panva/jose/blob/151000e84b94551e7408f988e3d52f8ccf530a64/src/lib/asn1.ts#L253-L266 https://github.com/panva/jose/blob/151000e84b94551e7408f988e3d52f8ccf530a64/src/lib/buffer_utils.ts#L6-L21 (used in other files for base64url handling, this will be replaced with https://github.com/tc39/proposal-arraybuffer-base64 when available) https://github.com/panva/jose/blob/151000e84b94551e7408f988e3d52f8ccf530a64/src/lib/decrypt.ts#L11-L33

I am specifically not interested in using an import() here because of how bundlers tend to either error out or insert shitty polyfills when they detect node modules in non-node runtime targets.

Thanks - I'll look more deeply next week.

One thing that could be better for workers is that you have an adapter that could be implemented for different platforms.

i.e.

import type { X509Certificate } from "node:crypto";

interface CryptoAdapter {
  X509Certificate: X509Certificate;
  // ...
}

You can have a default impl based on get getBuiltinModule but it leaves an option for platforms not (fully) supporting it.

For workers getBuiltinModule will return the workerd module which might not implement all the methods. However when you import crypto from 'node:crypto'; we return an hybrid implementation (some APIs are implemented in polyfill).

(Note that we do not polyfill getBuiltinModule to return the polyfills because it would mean that we can not detect the unused polyfills at build time and tree shake them).

@panva
Copy link
Contributor

panva commented Feb 15, 2025

@vicb how can i enable node-compat in this capnp definition here, that way I could easily test the impact of node compat on jose (which i've seen people struggle with when node compat is enabled for them in wrangler)

I ask because when I add compatibilityFlags = ["nodejs_compat_v2"], then my test script starts failing with

workerd/server/server.c++:4470: debug: [ TEST ] main
A hanging Promise was canceled. This happens when the worker runtime is waiting for a Promise from JavaScript to resolve, but has detected that the Promise cannot possibly ever resolve because all code and events related to the Promise's I/O context have already finished.
workerd/server/server.c++:4478: debug: [ FAIL ] main
Tests failed!

That's certainly not expected just because a compat flag was added so I'm assuming I'm doing something wrong.

Repro with

gh repo clone panva/jose -- --branch v6.x-wip
cd jose
npm i workerd

// either add compat flag in tap/.workerd.sh or leave it be

npm run tap:workerd

@vicb
Copy link
Contributor

vicb commented Feb 15, 2025

compatibilityFlags = ["nodejs_compat_v2"]

That should do.

However the recommended way it to use compatibilityFlags = ["nodejs_compat"] as long as your compatibility date is >= 2024-09-23 (because nodejs_compat will also activate v2 after this date).

Also note that when using wrangler, we add polyfills when the node compatibility is activated (v2 or nodejs_compat with a date >= 2024-09-23).

If you want to test with the polyfill, you would have to use wrangler.

For example, instead of invoking esbuild in your script, you can invoke wrangler deploy --dry-run -outdir=/path/to/dist (You can also pass it the entry point, the compat date, the the compat flags - because the entry point and the flags aren't dynamic, they could also be in your wrangler.{toml,json}). That's similar to this test in the workers-sdk.

@panva
Copy link
Contributor

panva commented Feb 15, 2025

@vicb i get the same failure regardless of the compat flag version. the test works as expected without a node compact flag

@vicb
Copy link
Contributor

vicb commented Feb 15, 2025

Yep, that's expected because they should be the same :)

To debug that, it's probably easier to create a wrangler.json in this directory, point to a worker.ts that should look something like:

// worker.ts
export default {
  async fetch(request, env, ctx) {
    // call your test from here (i.e. run.ts)
    await test();
    return new Response('Hello World!');
  },
};

Then you can wrangler dev, open the dev tools, add breakpoints or at least break on exception and then open the worker URL in a browser / curl it.

Let me know if you need more info (feel free to email me at the address on my GH profile).

yj7o5 pushed a commit to yj7o5/workerd that referenced this pull request Feb 25, 2025
…#3463)

* Add ncrypto dependency

* Use ncrypto for key derivation operations

* Use ncrypto for spkac apis

* Use ncrypto for prime generation/checks

* Refinements to the node:crypto SecretKey impl

* Begin working on createPrivateKey

* Implement more of the private key details

* Implement createPublicKey

* Consolidate ncrypto::Buffer creation into utility

* Handle additional code review nits

* node:crypto asymmetric key jwk export

* Implement JWK import/export for RSA, EC, OKP keys

* Temporarily use ncrypto dev branch

* Remove now obsolete todo comment

* Support creating public key from PrivateKeyObject

* Implement generateKey()/generateKeySync() for aes/hmac keys

* Address review feedback

* Argument validation for generateKeyPair

* Implement more key pair generation

* Additional keygen details

* Fixups to support internal use of boringssl+fips

* Temporarily use ncrypto dev branch

* Add some additional comments/notes on crypto keys impl

* Update ncrypto details in WORKSPACE
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.

4 participants