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

fix: use publicKey.x instead of privateKey.x #2926

Merged
merged 2 commits into from
Feb 3, 2025
Merged

Conversation

d70-t
Copy link
Contributor

@d70-t d70-t commented Jan 27, 2025

fix: ed25519 generateKeyPairSync: use publicKey.x instead of privateKey.x

Description

Although RFC7518 6.3.2 specifies that private key JWKs must include all the fields present in the public key, this is not the case for the implementation of node:crypto.generateKeyPairSync in the deno runtime.

While Node returns the (same) x property in both, the privateKey and the publicKey, Deno only returns the x property in publicKey (i.e. no x here).

This change should not affect any Node users, but would enable the use of libp2p on Deno.

Notes & open questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works

Although [RFC7518 6.3.2](https://datatracker.ietf.org/doc/html/rfc7518#section-6.3.2) specifies that private key JWKs must include all the fields present in the public key, this is not the case for the implementation of `node:crypto.generateKeyPairSync` in the deno runtime.

While Node returns the (same) `x` property in both, the `privateKey` and the `publicKey`, Deno only returns the `x` property in `publicKey` (i.e. [no x here](https://github.com/denoland/deno/blob/88490d092751288f736855b2418a4da606a31ce7/ext/node/ops/crypto/keys.rs#L1475)).

This change should not affect any Node users, but would enable the use of libp2p on Deno.
@d70-t d70-t requested a review from a team as a code owner January 27, 2025 19:00
@d70-t d70-t changed the title ed25519 generateKeyPairSync: use publicKey.x instead of privateKey.x fix: ed25519 generateKeyPairSync: use publicKey.x instead of privateKey.x Jan 27, 2025
@d70-t d70-t changed the title fix: ed25519 generateKeyPairSync: use publicKey.x instead of privateKey.x fix: use publicKey.x instead of privateKey.x Jan 27, 2025
@achingbrain
Copy link
Member

That RFC link only refers to RSA keys, the code change here is for Ed25519 keys?

Seems fine though, thanks for opening this PR.

@achingbrain achingbrain merged commit 723bde6 into libp2p:main Feb 3, 2025
34 checks passed
achingbrain added a commit that referenced this pull request Feb 3, 2025
Although [RFC7518 6.3.2](https://datatracker.ietf.org/doc/html/rfc7518#section-6.3.2) specifies that private key JWKs must include all the fields present in the public key, this is not the case for the implementation of `node:crypto.generateKeyPairSync` in the deno runtime.

While Node returns the (same) `x` property in both, the `privateKey` and the `publicKey`, Deno only returns the `x` property in `publicKey` (i.e. [no x here](https://github.com/denoland/deno/blob/88490d092751288f736855b2418a4da606a31ce7/ext/node/ops/crypto/keys.rs#L1475)).

This change should not affect any Node users, but would enable the use of libp2p on Deno.

Co-authored-by: Alex Potsides <[email protected]>
@d70-t
Copy link
Contributor Author

d70-t commented Feb 3, 2025

Sorry, my bad. Elliptic curves are in section 6.2 right above. But it's the same story.

@achingbrain achingbrain mentioned this pull request Feb 3, 2025
@achingbrain
Copy link
Member

Very true. Is there an issue on the Deno repo about this? It should implement the MUST parts of the RFC completely.

@d70-t d70-t deleted the patch-1 branch February 5, 2025 09:56
@d70-t
Copy link
Contributor Author

d70-t commented Feb 5, 2025

I opened another issue at denoland/deno#27972

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.

2 participants