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

refactor(experimental): a function for generating secret keys #1379

Merged
merged 3 commits into from
Jul 14, 2023

Conversation

steveluscher
Copy link
Collaborator

@steveluscher steveluscher commented Jul 1, 2023

refactor(experimental): a function for generating secret keys

Summary

This PR introduces generateSecretKey(). You might need to use this when you need to sign for the creation of an account, for instance.

Instead of vending the bytes of a secret key, however, we use JS-native CryptoKey instances. These are opaque tokens that you can return at a later time to perform some action, like deriving the public key for the secret they represent, or signing a message.

The idea is that you can freely pass these CryptoKey instances around your application without worrying about accidentally logging the key material itself – ie. to Sentry or to the browser console.

The only environments that support Ed25519 key generation at the moment:

  • Node >=17.4
  • Safari 17

For other environments, we'll supply a polyfill that implements key generation, signing, encryption, decryption, and verification in userspace.

Spec: https://wicg.github.io/webcrypto-secure-curves/#ed25519
Proposal repo: https://github.com/WICG/webcrypto-secure-curves
Implementation status: WICG/webcrypto-secure-curves#20

Test Plan

cd packages/keys/
pnpm test:unit:browser
pnpm test:unit:node

Stack created with Sapling. Best reviewed with ReviewStack.

Copy link
Contributor

@mcintyre94 mcintyre94 left a comment

Choose a reason for hiding this comment

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

Nice, this looks great! Really like the ability to keep the generated keys from being logged etc

Have you thought about if/how we should support use cases where you do need the raw private keys? One example that comes to mind would be Metaplex's keypairIdentity

packages/keys/src/secrets.ts Outdated Show resolved Hide resolved
@steveluscher steveluscher marked this pull request as ready for review July 6, 2023 21:01
@steveluscher
Copy link
Collaborator Author

Have you thought about if/how we should support use cases where you do need the raw private keys? One example that comes to mind would be Metaplex's keypairIdentity

Say more!

My rough plan if/when someone asks for this is:

  1. Ask them to generate the key themselves
  2. If they need interop with web3.js at that point, export createSecretKeyFromBytes for them to use

@steveluscher
Copy link
Collaborator Author

For the Metaplex case I'm pretty certain we just write a new IdentityDriver that takes in a SecretKey, implement all of these methods, and we're done.

https://github.com/ledamint-IO/js/blob/e10eba4c136d0ded2e495e671a2c8d76ac3e6270/src/drivers/identity/IdentityDriver.ts#L5-L23

@steveluscher steveluscher force-pushed the pr1379 branch 2 times, most recently from 3f79574 to 7683f54 Compare July 6, 2023 21:23
@steveluscher
Copy link
Collaborator Author

I'm going to massively rework this.

I just learned that ED25519 keys are coming to SubtleCrypto. It's already in Node 18.16.1 LTS and Safari 17.

  1. Awesome.
  2. CryptoKey instances can actually be made un-exfiltratable, which is great.
  3. All of these APIs are going to have to become async.

@mcintyre94
Copy link
Contributor

I think you're right about IdentityDriver for Metaplex, and anything using keypairs/secrets should ideally be structured like that so we can just write drivers without having to pass raw secret keys around.

expect.assertions(1);
const { privateKey } = await generateKeyPair();
expect(privateKey).toHaveProperty('usages', ['sign']);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, this API looks great! 🔥

Copy link
Collaborator Author

@steveluscher steveluscher Jul 14, 2023

Choose a reason for hiding this comment

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

It's SO good. Except for the part where it's all stupid magic combinations you have to know (ie. you can generate Ed25519 keys with "sign" and "verify" usages, but supply the "deriveKey" usage and it will fatal at runtime). That part I hate.

…nments

## Summary

The idea here is that we're going to _presume_ that the browser has an Ed25519-compatible key generator, then polyfill it otherwise. From that perspective, the tests should just presume that it's present, and we should leave the testing of the polyfill up to the polyfill tests.
…mine whether Ed25519 is supported

## Summary

It's just nice to have these well tested, and all in one place. These will only become more complicated, so it's best to get them out of the code.

Only thing is, I wish that TypeScript could assert about globals: https://stackoverflow.com/q/76657623/802047

## Test Plan

```
cd packages/keys/
pnpm test:unit:browser
pnpm test:unit:node
```
## Summary

This PR introduces `generateSecretKey()`. You might need to use this when you need to sign for the creation of an account, for instance.

Instead of vending the _bytes_ of a secret key, however, we use JS-native `CryptoKey` instances. These are opaque tokens that you can return at a later time to perform some action, like deriving the public key for the secret they represent, or signing a message.

The idea is that you can freely pass these `CryptoKey` instances around your application without worrying about accidentally logging the key material itself – ie. to Sentry or to the browser console.

The only environments that support Ed25519 key generation at the moment:

* Node >=17.4
* Safari 17

For other environments, we'll supply a polyfill that implements key generation, signing, encryption, decryption, and verification in userspace.

Spec: https://wicg.github.io/webcrypto-secure-curves/#ed25519
Proposal repo: https://github.com/WICG/webcrypto-secure-curves
Implementation status: WICG/webcrypto-secure-curves#20

## Test Plan

```
cd packages/keys/
pnpm test:unit:browser
pnpm test:unit:node
```
@steveluscher steveluscher merged commit cb1a5a0 into master Jul 14, 2023
@steveluscher steveluscher deleted the pr1379 branch July 14, 2023 23:19
@github-actions
Copy link
Contributor

🎉 This PR is included in version 1.78.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link
Contributor

Because there has been no activity on this PR for 14 days since it was merged, it has been automatically locked. Please open a new issue if it requires a follow up.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants