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

Use node:crypto instead of noble/curves #2936

Merged
merged 12 commits into from
Nov 7, 2024
Merged

Conversation

rafaelbsky
Copy link
Contributor

@rafaelbsky rafaelbsky commented Oct 31, 2024

@atproto/crypto currently does signature validation with @noble/curves, which is a JS implementation.

Our CPU profiling shows that validation takes a good CPU share, so we want to see how that improves by switching to the native node:crypto, but not yet changing the current implementation in @atproto/crypto.

The solution was to make customizable the signature verification function that happens inside the JWT validation, which happens on @atproto/xrpc-server.

So in short:

  1. @atproto/crypto exports the utils to avoid duplication.
  2. @aproto/xrpc-server allows customization of the signature verification function.
  3. @atproto/bsky uses 1 to build a customization for 2, using node:crypto instead of @noble/curves.

@rafaelbsky rafaelbsky force-pushed the bsky-use-native-crypto branch from 06b4809 to 37368a2 Compare November 1, 2024 23:32
@rafaelbsky rafaelbsky marked this pull request as ready for review November 2, 2024 01:44
Copy link
Contributor

@matthieusieben matthieusieben 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 good. It's kind of a bummer that we are able to benefit from this change everywhere.

Wouldn't be a better solution to add your changes to the following function instead ?

const verifySignatureWithKey = async (key: string) => {
return crypto.verifySignature(key, msgBytes, sigBytes, {
jwtAlg: header.alg,
allowMalleableSig: true,
})
}

packages/xrpc-server/src/auth.ts Show resolved Hide resolved
packages/xrpc-server/src/auth.ts Show resolved Hide resolved
@rafaelbsky
Copy link
Contributor Author

@matthieusieben I think your 3 comments fall into the same concern, so I'll reply to them all here.

Your suggestion was to make the change in xrpc-server so it would benefit more services at once. I think that's what we're trying to avoid: we want to only have it affect appview, to assess the performance difference. Once we confirm that, we will make the change directly in crypto (which would benefit all services that use it).

Do you think it makes sense? LMK if I misunderstood something.

Copy link
Contributor

@matthieusieben matthieusieben left a comment

Choose a reason for hiding this comment

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

Gotcha !

@rafaelbsky rafaelbsky force-pushed the bsky-use-native-crypto branch from e943449 to 10e8c60 Compare November 5, 2024 16:22
@rafaelbsky rafaelbsky merged commit 1982693 into main Nov 7, 2024
11 checks passed
@rafaelbsky rafaelbsky deleted the bsky-use-native-crypto branch November 7, 2024 16:29
@github-actions github-actions bot mentioned this pull request Nov 7, 2024
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