[experimental] Validate that the public key generated from createKeyPairFromBytes() belongs to the private key #2329

luu-alex · 2024-03-16T19:04:00Z

updated createKeyPairFromBytes. Adds a validation check to verify the public and private key by signing data and verifying that the signed data matches the public key.

Added createKeyPairFromBytes_DANGEROUSLY_SKIP_VALIDATION that skips the validation above ^

Adds error code SOLANA_ERROR__KEYS__VERIFY_SIGNATURE_FAILURE

Included in this PR is testcases to test the possible errors within these functions and creating a succesful keypair from a 64-byte array.

Closes #2289.

changeset-bot · 2024-03-16T19:04:03Z

🦋 Changeset detected

Latest commit: 6e50c49

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 35 packages

Name	Type
@solana/assertions	Patch
@solana/errors	Patch
@solana/keys	Patch
@solana/addresses	Patch
@solana/accounts	Patch
@solana/codecs-core	Patch
@solana/codecs-data-structures	Patch
@solana/codecs-numbers	Patch
@solana/codecs-strings	Patch
@solana/compat	Patch
@solana/instructions	Patch
@solana/web3.js-experimental	Patch
@solana/rpc-api	Patch
@solana/rpc-spec	Patch
@solana/rpc-subscriptions-spec	Patch
@solana/rpc-subscriptions-transport-websocket	Patch
@solana/rpc-subscriptions	Patch
@solana/rpc-transport-http	Patch
@solana/rpc-types	Patch
@solana/rpc	Patch
@solana/signers	Patch
@solana/sysvars	Patch
@solana/transaction-confirmation	Patch
@solana/transactions	Patch
@solana/rpc-graphql	Patch
@solana/rpc-subscriptions-api	Patch
@solana/web3.js-legacy-sham	Patch
@solana/programs	Patch
@solana/rpc-parsed-types	Patch
@solana/codecs	Patch
@solana/options	Patch
@solana/rpc-transformers	Patch
@solana/functional	Patch
@solana/rpc-spec-types	Patch
@solana/webcrypto-ed25519-polyfill	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

packages/errors/src/codes.ts

packages/errors/src/messages.ts

packages/keys/src/key-pair.ts

lorisleiva · 2024-03-17T20:43:57Z

Thank you for your contribution! I just added a few comments. 🙂

luu-alex · 2024-03-18T03:10:00Z

Thanks for the quick feedback! Your comments make sense, changes has been applied. lemme know if theres anything else

lorisleiva · 2024-03-18T18:09:41Z

packages/keys/src/key-pair.ts

@@ -29,21 +29,8 @@ export async function createKeyPairFromBytes(bytes: Uint8Array, extractable?: bo
    const signedData = await signBytes(privateKey, TEST_DATA);
    const isValid = await verifySignature(publicKey, signedData, TEST_DATA);
    if (!isValid) {
-        throw new SolanaError(SOLANA_ERROR__KEYS__VERIFY_SIGNATURE_FAILURE);
+        throw new SolanaError(SOLANA_ERROR__KEYS__PUBLIC_KEY_MUST_MATCH_PRIVATE_KEY, { address: bytes.slice(32) });


Thanks for the update! Just one more comment. It's probably more useful for the end-user if that address is base-58 encoded.

You can use string codecs to transform it like so:

import { getStringDecoder, getBase58Decoder } from "@solana/codecs-strings"; const addressDecoder = getStringDecoder({ encoding: getBase58Decoder(), size: 32 }); const address = addressDecoder.decode(bytes.slice(32));

Note that you could also use getAddressDecoder from @solana/addresses but that would add a new dependency to @solana/keys when it's not really needed.

I actually think we should probably not log the bytes here. Imagine that someone got the order wrong, and now their private bytes are logged to the console. Include no context.

That's a very good point.

lorisleiva · 2024-03-18T18:10:18Z

Ah also, don't forget to fix the formatting. pnpm style:fix at the root should do the trick.

steveluscher

steveluscher · 2024-03-18T19:40:07Z

packages/keys/src/key-pair.ts


 import { createPrivateKeyFromBytes } from './private-key';
+import { verifySignature, signBytes } from './signatures';
+
+const TEST_DATA = new Uint8Array([1, 2, 3, 4, 5]);


I'd like this to be randomly generated on each pass through the function.

steveluscher · 2024-03-18T19:43:21Z

packages/keys/src/key-pair.ts

+    const signedData = await signBytes(privateKey, TEST_DATA);
+    const isValid = await verifySignature(publicKey, signedData, TEST_DATA);


Suggested change

const signedData = await signBytes(privateKey, TEST_DATA);

const isValid = await verifySignature(publicKey, signedData, TEST_DATA);

const randomBytes = new Uint8Array(32);

crypto.getRandomValues(randomBytes);

const signedData = await signBytes(privateKey, randomBytes);

const isValid = await verifySignature(publicKey, signedData, randomBytes);

steveluscher · 2024-03-18T19:45:12Z

packages/keys/src/key-pair.ts


 import { createPrivateKeyFromBytes } from './private-key';
+import { verifySignature, signBytes } from './signatures';
+
+const TEST_DATA = new Uint8Array([1, 2, 3, 4, 5]);

 export async function generateKeyPair(): Promise<CryptoKeyPair> {
    await assertKeyGenerationIsAvailable();


To generate a test message randomly each time, we'll need to assert that the PRNG is available (that crypto.getRandomValues is a function. Can you add that, and a related error code, to @solana/assertions`.

Suggested change

await assertKeyGenerationIsAvailable();

await Promise.all([

assertKeyGenerationIsAvailable(),

assertPRNGIsAvailable(), // <-- add this to `@solana/assertions` & `@solana/errors`

]);

I think it would make more sense to put assertPRNGIsAvailable() in createKeyPairFromBytes than generateKeyPair right?

Oh, of course. Oops; I left my comment in the wrong spot!

steveluscher · 2024-03-18T19:46:20Z

packages/keys/src/key-pair.ts

@@ -29,21 +29,8 @@ export async function createKeyPairFromBytes(bytes: Uint8Array, extractable?: bo
    const signedData = await signBytes(privateKey, TEST_DATA);
    const isValid = await verifySignature(publicKey, signedData, TEST_DATA);
    if (!isValid) {
-        throw new SolanaError(SOLANA_ERROR__KEYS__VERIFY_SIGNATURE_FAILURE);
+        throw new SolanaError(SOLANA_ERROR__KEYS__PUBLIC_KEY_MUST_MATCH_PRIVATE_KEY, { address: bytes.slice(32) });


I actually think we should probably not log the bytes here. Imagine that someone got the order wrong, and now their private bytes are logged to the console. Include no context.

steveluscher · 2024-03-18T19:47:16Z

packages/errors/src/messages.ts

@@ -402,6 +403,7 @@ export const SolanaErrorMessages: Readonly<{
        'Expected base58-encoded signature to decode to a byte array of length 64. Actual length: $actualLength.',
    [SOLANA_ERROR__KEYS__SIGNATURE_STRING_LENGTH_OUT_OF_RANGE]:
        'Expected base58-encoded signature string of length in the range [64, 88]. Actual length: $actualLength.',
+    [SOLANA_ERROR__KEYS__VERIFY_SIGNATURE_FAILURE]: 'The provided private key does not match the provided public key at address [$address].',


This is the wrong error code, right?

Suggested change

[SOLANA_ERROR__KEYS__VERIFY_SIGNATURE_FAILURE]: 'The provided private key does not match the provided public key at address [$address].',

[SOLANA_ERROR__KEYS__PUBLIC_KEY_MUST_MATCH_PRIVATE_KEY]: 'The provided private key does not match the provided public key at address [$address].',

Good catch, thanks!

steveluscher · 2024-03-18T19:49:04Z

Thanks for doing this @luu-alex!

packages/errors/src/codes.ts

luu-alex · 2024-03-20T02:51:10Z

packages/keys/src/key-pair.ts


 import { createPrivateKeyFromBytes } from './private-key';
+import { verifySignature, signBytes } from './signatures';
+
+const TEST_DATA = new Uint8Array([1, 2, 3, 4, 5]);

 export async function generateKeyPair(): Promise<CryptoKeyPair> {
    await assertKeyGenerationIsAvailable();


I think it would make more sense to put assertPRNGIsAvailable() in createKeyPairFromBytes than generateKeyPair right?

luu-alex · 2024-03-20T03:05:09Z

Thanks for the feedback! 👍 let me know if theres anything else :)

steveluscher

I don't have the rights to push to your master to update this PR, so here's the final boss level before merge!

diff --git a/packages/assertions/src/__tests__/crypto-test.ts b/packages/assertions/src/__tests__/crypto-test.ts
index a676917c4..5357cd790 100644
--- a/packages/assertions/src/__tests__/crypto-test.ts
+++ b/packages/assertions/src/__tests__/crypto-test.ts
@@ -3,9 +3,13 @@ import { SOLANA_ERROR__CRYPTO__RANDOM_VALUES_FUNCTION_UNIMPLEMENTED, SolanaError
 import { assertPRNGIsAvailable } from '../crypto';
 
 describe('assertPRNGIsAvailable()', () => {
-    it('resolves to `undefined` without throwing', async () => {
-        expect.assertions(1);
-        await expect(assertPRNGIsAvailable()).resolves.toBeUndefined();
+    describe('when getRandomValues is available', () => {
+        it('does not throw', () => {
+            expect(assertPRNGIsAvailable).not.toThrow();
+        });
+        it('returns `undefined`', () => {
+            expect(assertPRNGIsAvailable()).toBeUndefined();
+        });
     });
     describe('when getRandomValues is not available', () => {
         let oldCrypto: InstanceType<typeof Crypto>['getRandomValues'];
@@ -18,9 +22,8 @@ describe('assertPRNGIsAvailable()', () => {
         afterEach(() => {
             globalThis.crypto.getRandomValues = oldCrypto;
         });
-        it('rejects', async () => {
-            expect.assertions(1);
-            await expect(() => assertPRNGIsAvailable()).rejects.toThrow(
+        it('throws', () => {
+            expect(assertPRNGIsAvailable).toThrow(
                 new SolanaError(SOLANA_ERROR__CRYPTO__RANDOM_VALUES_FUNCTION_UNIMPLEMENTED),
             );
         });
diff --git a/packages/assertions/src/crypto.ts b/packages/assertions/src/crypto.ts
index bd62651c5..8752465e8 100644
--- a/packages/assertions/src/crypto.ts
+++ b/packages/assertions/src/crypto.ts
@@ -1,6 +1,6 @@
 import { SOLANA_ERROR__CRYPTO__RANDOM_VALUES_FUNCTION_UNIMPLEMENTED, SolanaError } from '@solana/errors';
 
-export async function assertPRNGIsAvailable() {
+export function assertPRNGIsAvailable() {
     if (typeof globalThis.crypto === 'undefined' || typeof globalThis.crypto.getRandomValues !== 'function') {
         throw new SolanaError(SOLANA_ERROR__CRYPTO__RANDOM_VALUES_FUNCTION_UNIMPLEMENTED);
     }
diff --git a/packages/assertions/src/index.ts b/packages/assertions/src/index.ts
index b3383c7a9..4d4f2dd31 100644
--- a/packages/assertions/src/index.ts
+++ b/packages/assertions/src/index.ts
@@ -1 +1,2 @@
+export * from './crypto';
 export * from './subtle-crypto';
diff --git a/packages/errors/src/codes.ts b/packages/errors/src/codes.ts
index feaac895e..5bac1bfea 100644
--- a/packages/errors/src/codes.ts
+++ b/packages/errors/src/codes.ts
@@ -92,7 +92,7 @@ export const SOLANA_ERROR__SUBTLE_CRYPTO__VERIFY_FUNCTION_UNIMPLEMENTED = 361000
 
 // Crypto-related errors.
 // Reserve error codes in the range [3611000-3611050].
-export const SOLANA_ERROR__CRYPTO__RANDOM_VALUES_FUNCTION_UNIMPLEMENTED = 3610007 as const;
+export const SOLANA_ERROR__CRYPTO__RANDOM_VALUES_FUNCTION_UNIMPLEMENTED = 3611000 as const;
 
 // Key-related errors.
 // Reserve error codes in the range [3704000-3704999].
diff --git a/packages/keys/src/key-pair.ts b/packages/keys/src/key-pair.ts
index 4adc36823..32c3264e8 100644
--- a/packages/keys/src/key-pair.ts
+++ b/packages/keys/src/key-pair.ts
@@ -1,4 +1,4 @@
-import { assertKeyGenerationIsAvailable } from '@solana/assertions';
+import { assertKeyGenerationIsAvailable, assertPRNGIsAvailable } from '@solana/assertions';
 import {
     SOLANA_ERROR__KEYS__INVALID_KEY_PAIR_BYTE_LENGTH,
     SOLANA_ERROR__KEYS__PUBLIC_KEY_MUST_MATCH_PRIVATE_KEY,
@@ -19,6 +19,8 @@ export async function generateKeyPair(): Promise<CryptoKeyPair> {
 }
 
 export async function createKeyPairFromBytes(bytes: Uint8Array, extractable?: boolean): Promise<CryptoKeyPair> {
+    assertPRNGIsAvailable();
+
     if (bytes.byteLength !== 64) {
         throw new SolanaError(SOLANA_ERROR__KEYS__INVALID_KEY_PAIR_BYTE_LENGTH, { byteLength: bytes.byteLength });
     }

steveluscher · 2024-03-20T22:03:21Z

packages/assertions/src/crypto.ts

@@ -0,0 +1,7 @@
+import { SOLANA_ERROR__CRYPTO__RANDOM_VALUES_FUNCTION_UNIMPLEMENTED, SolanaError } from '@solana/errors';
+
+export async function assertPRNGIsAvailable() {


Hmm… I wonder why some of these were made async. Seems like a mistake we shouldn't replicate here.

luu-alex · 2024-03-23T03:15:51Z

Cool. feedback addressed 👍

steveluscher · 2024-03-23T07:27:55Z

Awesome! I'd do it myself, but I can't push to your master, so can you run a quick pnpm style:fix in the root to get CI to pass?

steveluscher

luu-alex · 2024-03-23T16:58:47Z

boom. tests are passing, thanks

steveluscher

Thank you for doing this! Where did you come from? How did you get here?!

github-actions · 2024-03-26T19:55:31Z

🎉 This PR is included in version 1.91.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

luu-alex · 2024-03-27T13:37:09Z

Was learning more about solana and saw solana-web3 to be interesting!

github-actions · 2024-04-16T08:02:14Z

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.

Alex Luu added 4 commits March 16, 2024 00:18

add createKeyPairFromBytes_DANGEROUSLY_SKIP_VALIDATION and validation

3e8eb1d

add testcases and error codes for createKeyPairFromBytes

1c3c7c0

update error message

4e9304b

formatting

0e7548f

mergify bot added the community label Mar 16, 2024

mergify bot requested a review from a team March 16, 2024 19:04

lorisleiva reviewed Mar 17, 2024

View reviewed changes

packages/errors/src/codes.ts Outdated Show resolved Hide resolved

packages/errors/src/messages.ts Outdated Show resolved Hide resolved

packages/keys/src/key-pair.ts Outdated Show resolved Hide resolved

address feedback

62c7121

lorisleiva reviewed Mar 18, 2024

View reviewed changes

steveluscher requested changes Mar 18, 2024

View reviewed changes

address feedback add assertPRNGIsAvailable

70611e0

luu-alex commented Mar 20, 2024

View reviewed changes

update

99ff817

steveluscher reviewed Mar 20, 2024

View reviewed changes

address feedback

9292acf

steveluscher approved these changes Mar 23, 2024

View reviewed changes

steveluscher and others added 2 commits March 23, 2024 00:32

Create thick-radios-search.md

0f1a0ea

run prettier

6e50c49

steveluscher approved these changes Mar 26, 2024

View reviewed changes

steveluscher merged commit 478443f into solana-labs:master Mar 26, 2024
6 checks passed

github-actions bot mentioned this pull request Mar 26, 2024

Version Packages (preview) #2348

Closed

github-actions bot added the released label Mar 26, 2024

This was referenced Mar 26, 2024

Version Packages (preview) shellnoq/solana-web3.js#1

Closed

Version Packages (preview) lucia-w/solana-web3.js#1

Closed

github-actions bot mentioned this pull request Mar 28, 2024

Version Packages (preview) scottym5797/solana-web3.js#1

Open

github-actions bot locked as resolved and limited conversation to collaborators Apr 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[experimental] Validate that the public key generated from createKeyPairFromBytes() belongs to the private key #2329

[experimental] Validate that the public key generated from createKeyPairFromBytes() belongs to the private key #2329

luu-alex commented Mar 16, 2024 •

edited by steveluscher

Loading

changeset-bot bot commented Mar 16, 2024 •

edited

Loading

lorisleiva commented Mar 17, 2024

luu-alex commented Mar 18, 2024

lorisleiva Mar 18, 2024

steveluscher Mar 18, 2024

lorisleiva Mar 20, 2024

lorisleiva commented Mar 18, 2024

steveluscher left a comment

steveluscher Mar 18, 2024

steveluscher Mar 18, 2024

steveluscher Mar 18, 2024

luu-alex Mar 20, 2024

steveluscher Mar 20, 2024

steveluscher Mar 18, 2024

steveluscher Mar 18, 2024

luu-alex Mar 20, 2024

steveluscher commented Mar 18, 2024

luu-alex Mar 20, 2024

luu-alex commented Mar 20, 2024

steveluscher left a comment

steveluscher Mar 20, 2024

luu-alex commented Mar 23, 2024

steveluscher commented Mar 23, 2024

steveluscher left a comment

luu-alex commented Mar 23, 2024

steveluscher left a comment

github-actions bot commented Mar 26, 2024

luu-alex commented Mar 27, 2024

github-actions bot commented Apr 16, 2024

		const signedData = await signBytes(privateKey, TEST_DATA);
		const isValid = await verifySignature(publicKey, signedData, TEST_DATA);

-    const signedData = await signBytes(privateKey, TEST_DATA);
-    const isValid = await verifySignature(publicKey, signedData, TEST_DATA);
+    const randomBytes = new Uint8Array(32);
+    crypto.getRandomValues(randomBytes);
+    const signedData = await signBytes(privateKey, randomBytes);
+    const isValid = await verifySignature(publicKey, signedData, randomBytes);

-    await assertKeyGenerationIsAvailable();
+    await Promise.all([
+        assertKeyGenerationIsAvailable(),
+        assertPRNGIsAvailable(), // <-- add this to `@solana/assertions` & `@solana/errors`
+    ]);

	[SOLANA_ERROR__KEYS__VERIFY_SIGNATURE_FAILURE]: 'The provided private key does not match the provided public key at address [$address].',
	[SOLANA_ERROR__KEYS__PUBLIC_KEY_MUST_MATCH_PRIVATE_KEY]: 'The provided private key does not match the provided public key at address [$address].',

		@@ -0,0 +1,7 @@
		import { SOLANA_ERROR__CRYPTO__RANDOM_VALUES_FUNCTION_UNIMPLEMENTED, SolanaError } from '@solana/errors';

		export async function assertPRNGIsAvailable() {

[experimental] Validate that the public key generated from createKeyPairFromBytes() belongs to the private key #2329

[experimental] Validate that the public key generated from createKeyPairFromBytes() belongs to the private key #2329

Conversation

luu-alex commented Mar 16, 2024 • edited by steveluscher Loading

changeset-bot bot commented Mar 16, 2024 • edited Loading

🦋 Changeset detected

lorisleiva commented Mar 17, 2024

luu-alex commented Mar 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorisleiva commented Mar 18, 2024

steveluscher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveluscher commented Mar 18, 2024

Choose a reason for hiding this comment

luu-alex commented Mar 20, 2024

steveluscher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luu-alex commented Mar 23, 2024

steveluscher commented Mar 23, 2024

steveluscher left a comment

Choose a reason for hiding this comment

luu-alex commented Mar 23, 2024

steveluscher left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 26, 2024

luu-alex commented Mar 27, 2024

github-actions bot commented Apr 16, 2024

luu-alex commented Mar 16, 2024 •

edited by steveluscher

Loading

changeset-bot bot commented Mar 16, 2024 •

edited

Loading