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

Implement Secure Curves API #500

Merged
merged 2 commits into from
Apr 17, 2023
Merged

Implement Secure Curves API #500

merged 2 commits into from
Apr 17, 2023

Conversation

fhanau
Copy link
Collaborator

@fhanau fhanau commented Apr 1, 2023

This patch implements the Web Crypto Secure Curves X25519 and Ed25519.

Caveats/Potential issues:

  • Error messages for NODE-ED25519 have been largely updated to refer to Ed25519. This reduces code complexity and still results in descriptive messages as Ed25519 is the underlying algorithm.
  • Node no longer allows raw private imports for NODE-ED25519 – comments about deviating from Node's approach are no longer needed
  • Is there a better name for EdDsaKey now that it's shared between both Eddsa and Eddh?
  • There's ternary operator in a few spots – I can switch to branches if that style is preferred
  • It might be possible to do this in one class using kj::OneOf<KeyAlgorithm, EllipticKeyAlgorithm > – I wanted to avoid breaking the JSG integration for now
  • Test cases to be added

for (kj::byte b : sharedSecret) {
isNonZeroSecret |= b;
}
JSG_REQUIRE(isNonZeroSecret == 0, DOMOperationError,
Copy link

Choose a reason for hiding this comment

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

This condition seems odd to me at first sight. It seems to me that isNonZeroSecret == 0 if and only if all bytes of sharedSecret are 0, so shouldn't this assertion be negated? (I haven't looked at this codebase or the spec in a long time so I might be missing something.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right, the assertion is wrong here – I'll take care of this on Monday

@panva
Copy link
Contributor

panva commented Apr 1, 2023

Node no longer allows raw private imports for NODE-ED25519 – comments about deviating from Node's approach are no longer needed

The identifier NODE-ED25519 is no longer supported on any Node.js supported release line. All lines with webcrypto already use the identifiers from https://wicg.github.io/webcrypto-secure-curves/

EDDH

Because this appears in the public facing messages, i'm seeing this being referred to as EdDH for the first time.

@jasnell
Copy link
Member

jasnell commented Apr 1, 2023

The identifier NODE-ED25519 is no longer supported on any Noide.js supported release line.

Yeah, our backwards compat requirements, however, mean that we'll be keeping it around indefinitely.

@jasnell
Copy link
Member

jasnell commented Apr 1, 2023

Is there a better name for EdDsaKey ...

Perhaps just EdKey ? Or SecureCurvesKey ... I'm fine with leaving it as it is also tho.

There's ternary operator in a few spots ...

Perfectly fine with me.

It might be possible to do this is one class ...

Not necessary. I think what you have here is fine.

@fhanau fhanau marked this pull request as ready for review April 17, 2023 19:54
@fhanau fhanau merged commit 59e428e into main Apr 17, 2023
@fhanau fhanau deleted the felix/crypto-jwk-import branch April 18, 2023 00:30
mrbbot added a commit to cloudflare/miniflare that referenced this pull request May 4, 2023
- Accept `Ed25519` algorithm (supported as of cloudflare/workerd#500)
- Return `Ed25519` as the algorithm for `NODE-ED25519` keys
- Mark `X448` and `Ed448` algorithms as unsupported

This passes Miniflare's test suite on Node 16.13.0 and 20.1.0.

It also passes `panva/jose`'s `workerd` tap tests on Node 16.17.0 and
20.1.0. Some of those tests fail on 16.13.0 as support for the
`X25519` algorithm was only added in 16.17.0. See
https://nodejs.org/api/webcrypto.html#web-crypto-api for details.
@twiss twiss mentioned this pull request Feb 19, 2024
8 tasks
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