crypto: Ed25519 fromSecretKey takes pure 32 bytes privkey (MystenLabs…

…#6798) MystenLabs#6006 ## What This change makes `fromSecretKey` to take in an input of 32 bytes used as the pure privkey before hashing and bit clamping. Previously it took in a 64 byte privkey where the last 32 bytes is the public key and optionally validate it. The `fromSeed` method is deprecated. ## Why Taking in the 64 bytes privkey with baked in pubkey as the last 32 bytes is an API subject to misuse. Especially with skipValidation = true. ## How to fix If you run into errors probably because you are using 64 bytes privkey, truncating it to the first 32 bytes (.slice(0, 32)) will just work. ## Test plan Test added to confirm the sui.keystore vs typescript are the same when importing via mnenomics vs via raw secret key bytes. Also added an example in comments since we got a lot of qs on how to import a sui keystore format to typescript
wmira · Feb 20, 2023 · 4baf554 · 4baf554
1 parent d0da017
commit 4baf554
Show file tree

Hide file tree

Showing 7 changed files with 112 additions and 87 deletions.
diff --git a/.changeset/sharp-dragons-refuse.md b/.changeset/sharp-dragons-refuse.md
@@ -0,0 +1,5 @@
+---
+"@mysten/sui.js": minor
+---
+
+Make fromSecretKey take the 32 bytes privkey
diff --git a/apps/wallet/src/test-utils/vault.ts b/apps/wallet/src/test-utils/vault.ts
@@ -15,15 +15,28 @@ export const testMnemonic =
     'loud eye weather change muffin brisk episode dance mirror smart image energy';
 export const testEntropySerialized = '842a27e29319123892f9ba8d9991c525';
 export const testEntropy = toEntropy(testEntropySerialized);
-export const testEd25519Serialized = Object.freeze({
+export const testEd25519SerializedLegacy = Object.freeze({
     schema: 'ED25519',
     privateKey:
         'a3R0jvXpEziZLHsbX1DogdyGm8AK87HScEK+JJHwaV99nEpOfYblbYS3ci9wP2DT5YZtE3e4v/HBsN39kRz60A==',
 } as const);
+
+export const testEd25519Legacy = fromExportedKeypair(
+    testEd25519SerializedLegacy
+);
+export const testEd25519AddressLegacy = `0x${testEd25519Legacy
+    .getPublicKey()
+    .toSuiAddress()}`;
+
+export const testEd25519Serialized = Object.freeze({
+    schema: 'ED25519',
+    privateKey: 'a3R0jvXpEziZLHsbX1DogdyGm8AK87HScEK+JJHwaV8=',
+} as const);
 export const testEd25519 = fromExportedKeypair(testEd25519Serialized);
 export const testEd25519Address = `0x${testEd25519
     .getPublicKey()
     .toSuiAddress()}`;
+
 export const testSecp256k1Serialized = Object.freeze({
     schema: 'Secp256k1',
     privateKey: '4DD3CUtZvbc9Ur69tTvKaLeIDptxNa9qZcpkyXWjVGY=',

diff --git a/crates/sui/src/unit_tests/keytool_tests.rs b/crates/sui/src/unit_tests/keytool_tests.rs
@@ -1,6 +1,8 @@
 // Copyright (c) Mysten Labs, Inc.
 // SPDX-License-Identifier: Apache-2.0
 
+use std::str::FromStr;
+
 use crate::keytool::read_authority_keypair_from_file;
 use crate::keytool::read_keypair_from_file;
 
@@ -160,25 +162,23 @@ fn test_load_keystore_err() {
 #[test]
 fn test_mnemonics_ed25519() -> Result<(), anyhow::Error> {
     // Test case matches with /mysten/sui/sdk/typescript/test/unit/cryptography/ed25519-keypair.test.ts
-    let mut keystore = Keystore::from(InMemKeystore::new(0));
-    KeyToolCommand::Import {
-        mnemonic_phrase: TEST_MNEMONIC.to_string(),
-        key_scheme: SignatureScheme::ED25519,
-        derivation_path: None,
+    const TEST_CASES: [[&str; 3]; 3] = [["film crazy soon outside stand loop subway crumble thrive popular green nuclear struggle pistol arm wife phrase warfare march wheat nephew ask sunny firm", "AN0JMHpDum3BhrVwnkylH0/HGRHBQ/fO/8+MYOawO8j6", "8867068daf9111ee013450eea1b1e10ffd62fc87"],
+    ["require decline left thought grid priority false tiny gasp angle royal system attack beef setup reward aunt skill wasp tray vital bounce inflict level", "AJrA997C1eVz6wYIp7bO8dpITSRBXpvg1m70/P3gusu2", "29bb131378438b6c7f50526e6a853a72ed97f10b"],
+    ["organ crash swim stick traffic remember army arctic mesh slice swear summer police vast chaos cradle squirrel hood useless evidence pet hub soap lake", "AAEMSIQeqyz09StSwuOW4MElQcZ+4jHW4/QcWlJEf5Yk", "6e5387db7249f6b0dc5b68eb095109157dc192a0"]];
+
+    for t in TEST_CASES {
+        let mut keystore = Keystore::from(InMemKeystore::new(0));
+        KeyToolCommand::Import {
+            mnemonic_phrase: t[0].to_string(),
+            key_scheme: SignatureScheme::ED25519,
+            derivation_path: None,
+        }
+        .execute(&mut keystore)?;
+        let kp = SuiKeyPair::decode_base64(t[1]).unwrap();
+        let addr = SuiAddress::from_str(t[2]).unwrap();
+        assert_eq!(SuiAddress::from(&kp.public()), addr);
+        assert!(keystore.addresses().contains(&addr));
     }
-    .execute(&mut keystore)?;
-    keystore.keys().iter().for_each(|pk| {
-        assert_eq!(
-            Hex::encode(pk.as_ref()),
-            "685b2d6f98784dd763249af21c92f588ca1be80c40a98c55bf7c91b74e5ac1e2"
-        );
-    });
-    keystore.addresses().iter().for_each(|addr| {
-        assert_eq!(
-            addr.to_string(),
-            "0x1a4623343cd42be47d67314fce0ad042f3c82685"
-        );
-    });
     Ok(())
 }
 

diff --git a/sdk/typescript/src/cryptography/ed25519-keypair.ts b/sdk/typescript/src/cryptography/ed25519-keypair.ts
@@ -2,7 +2,7 @@
 // SPDX-License-Identifier: Apache-2.0
 
 import nacl from 'tweetnacl';
-import type { ExportedKeypair, Keypair } from './keypair';
+import { ExportedKeypair, Keypair, PRIVATE_KEY_SIZE } from './keypair';
 import { Ed25519PublicKey } from './ed25519-publickey';
 import { isValidHardenedPath, mnemonicToSeedHex } from './mnemonics';
 import { derivePath, getPublicKey } from '../utils/ed25519-hd-key';
@@ -54,11 +54,22 @@ export class Ed25519Keypair implements Keypair {
   }
 
   /**
-   * Create a Ed25519 keypair from a raw secret key byte array.
-   *
-   * This method should only be used to recreate a keypair from a previously
-   * generated secret key.
+   * Create a Ed25519 keypair from a raw secret key byte array, also known as seed.
+   * This is NOT the private scalar which is result of hashing and bit clamping of
+   * the raw secret key.
    *
+   * The sui.keystore key is a list of Base64 encoded `flag || privkey`. To import
+   * a key from sui.keystore to typescript, decode from base64 and remove the first
+   * flag byte after checking it is indeed the Ed25519 scheme flag 0x00 (See more
+   * on flag for signature scheme: https://github.com/MystenLabs/sui/blob/818406c5abdf7de1b80915a0519071eec3a5b1c7/crates/sui-types/src/crypto.rs#L1650):
+   * ```
+   * import { Ed25519Keypair, fromB64 } from '@mysten/sui.js';
+   * const raw = fromB64(t[1]);
+   * if (raw[0] !== 0 || raw.length !== PRIVATE_KEY_SIZE + 1) {
+   *   throw new Error('invalid key');
+   * }
+   * const imported = Ed25519Keypair.fromSecretKey(raw.slice(1))
+   * ```
    * @throws error if the provided secret key is invalid and validation is not skipped.
    *
    * @param secretKey secret key byte array
@@ -69,18 +80,12 @@ export class Ed25519Keypair implements Keypair {
     options?: { skipValidation?: boolean },
   ): Ed25519Keypair {
     const secretKeyLength = secretKey.length;
-    if (secretKeyLength !== 64) {
-      // Many users actually wanted to invoke fromSeed(seed: Uint8Array), especially when reading from keystore.
-      if (secretKeyLength === 32) {
-        throw new Error(
-          'Wrong secretKey size. Expected 64 bytes, got 32. Similar function exists: fromSeed(seed: Uint8Array)',
-        );
-      }
+    if (secretKeyLength !== PRIVATE_KEY_SIZE) {
       throw new Error(
-        `Wrong secretKey size. Expected 64 bytes, got ${secretKeyLength}.`,
+        `Wrong secretKey size. Expected ${PRIVATE_KEY_SIZE} bytes, got ${secretKeyLength}.`,
       );
     }
-    const keypair = nacl.sign.keyPair.fromSecretKey(secretKey);
+    const keypair = nacl.sign.keyPair.fromSeed(secretKey);
     if (!options || !options.skipValidation) {
       const encoder = new TextEncoder();
       const signData = encoder.encode('sui validation');
@@ -92,19 +97,6 @@ export class Ed25519Keypair implements Keypair {
     return new Ed25519Keypair(keypair);
   }
 
-  /**
-   * Generate an Ed25519 keypair from a 32 byte seed.
-   *
-   * @param seed seed byte array
-   */
-  static fromSeed(seed: Uint8Array): Ed25519Keypair {
-    const seedLength = seed.length;
-    if (seedLength !== 32) {
-      throw new Error(`Wrong seed size. Expected 32 bytes, got ${seedLength}.`);
-    }
-    return new Ed25519Keypair(nacl.sign.keyPair.fromSeed(seed));
-  }
-
   /**
    * The public key for this Ed25519 keypair
    */
@@ -136,12 +128,7 @@ export class Ed25519Keypair implements Keypair {
     const { key } = derivePath(path, mnemonicToSeedHex(mnemonics));
     const pubkey = getPublicKey(key, false);
 
-    // Ed25519 private key returned here has 32 bytes. NaCl expects 64 bytes where the last 32 bytes are the public key.
-    let fullPrivateKey = new Uint8Array(64);
-    fullPrivateKey.set(key);
-    fullPrivateKey.set(pubkey, 32);
-
-    return new Ed25519Keypair({ publicKey: pubkey, secretKey: fullPrivateKey });
+    return new Ed25519Keypair({ publicKey: pubkey, secretKey: key });
   }
 
   export(): ExportedKeypair {

diff --git a/sdk/typescript/src/cryptography/keypair.ts b/sdk/typescript/src/cryptography/keypair.ts
@@ -7,6 +7,9 @@ import { PublicKey } from './publickey';
 import { Secp256k1Keypair } from './secp256k1-keypair';
 import { SignatureScheme } from './signature';
 
+export const PRIVATE_KEY_SIZE = 32;
+export const LEGACY_PRIVATE_KEY_SIZE = 64;
+
 export type ExportedKeypair = {
   schema: SignatureScheme;
   privateKey: string;
@@ -38,7 +41,12 @@ export function fromExportedKeypair(keypair: ExportedKeypair): Keypair {
   const secretKey = fromB64(keypair.privateKey);
   switch (keypair.schema) {
     case 'ED25519':
-      return Ed25519Keypair.fromSecretKey(secretKey);
+      let pureSecretKey = secretKey;
+      if (secretKey.length === LEGACY_PRIVATE_KEY_SIZE) {
+        // This is a legacy secret key, we need to strip the public key bytes and only read the first 32 bytes
+        pureSecretKey = secretKey.slice(0, PRIVATE_KEY_SIZE);
+      }
+      return Ed25519Keypair.fromSecretKey(pureSecretKey);
     case 'Secp256k1':
       return Secp256k1Keypair.fromSecretKey(secretKey);
     default:

diff --git a/sdk/typescript/test/unit/cryptography/ed25519-keypair.test.ts b/sdk/typescript/test/unit/cryptography/ed25519-keypair.test.ts
@@ -4,12 +4,29 @@
 import { fromB64 } from '@mysten/bcs';
 import nacl from 'tweetnacl';
 import { describe, it, expect } from 'vitest';
-import { Ed25519Keypair } from '../../../src';
+import { Ed25519Keypair, PRIVATE_KEY_SIZE } from '../../../src';
+
+const VALID_SECRET_KEY = 'mdqVWeFekT7pqy5T49+tV12jO0m+ESW7ki4zSU9JiCg=';
+
+// Test case generated against rust keytool cli. See https://github.com/MystenLabs/sui/blob/edd2cd31e0b05d336b1b03b6e79a67d8dd00d06b/crates/sui/src/unit_tests/keytool_tests.rs#L165
+const TEST_CASES = [
+  [
+    'film crazy soon outside stand loop subway crumble thrive popular green nuclear struggle pistol arm wife phrase warfare march wheat nephew ask sunny firm',
+    'AN0JMHpDum3BhrVwnkylH0/HGRHBQ/fO/8+MYOawO8j6',
+    '8867068daf9111ee013450eea1b1e10ffd62fc87',
+  ],
+  [
+    'require decline left thought grid priority false tiny gasp angle royal system attack beef setup reward aunt skill wasp tray vital bounce inflict level',
+    'AJrA997C1eVz6wYIp7bO8dpITSRBXpvg1m70/P3gusu2',
+    '29bb131378438b6c7f50526e6a853a72ed97f10b',
+  ],
+  [
+    'organ crash swim stick traffic remember army arctic mesh slice swear summer police vast chaos cradle squirrel hood useless evidence pet hub soap lake',
+    'AAEMSIQeqyz09StSwuOW4MElQcZ+4jHW4/QcWlJEf5Yk',
+    '6e5387db7249f6b0dc5b68eb095109157dc192a0',
+  ],
+];
 
-const VALID_SECRET_KEY =
-  'mdqVWeFekT7pqy5T49+tV12jO0m+ESW7ki4zSU9JiCgbL0kJbj5dvQ/PqcDAzZLZqzshVEs01d1KZdmLh4uZIg==';
-const INVALID_SECRET_KEY =
-  'mdqVWeFekT7pqy5T49+tV12jO0m+ESW7ki4zSU9JiCgbL0kJbj5dvQ/PqcDAzZLZqzshVEs01d1KZdmLh4uZIG==';
 const TEST_MNEMONIC =
   'result crisp session latin must fruit genuine question prevent start coconut brave speak student dismiss';
 
@@ -28,25 +45,26 @@ describe('ed25519-keypair', () => {
     );
   });
 
-  it('creating keypair from invalid secret key throws error', () => {
-    const secretKey = fromB64(INVALID_SECRET_KEY);
-    expect(() => {
-      Ed25519Keypair.fromSecretKey(secretKey);
-    }).toThrow('provided secretKey is invalid');
-  });
+  it('create keypair from secret key and mnemonics matches keytool', () => {
+    for (const t of TEST_CASES) {
+      // Keypair derived from mnemonic
+      const keypair = Ed25519Keypair.deriveKeypair(t[0]);
+      expect(keypair.getPublicKey().toSuiAddress()).toEqual(t[2]);
 
-  it('creating keypair from invalid secret key succeeds if validation is skipped', () => {
-    const secretKey = fromB64(INVALID_SECRET_KEY);
-    const keypair = Ed25519Keypair.fromSecretKey(secretKey, {
-      skipValidation: true,
-    });
-    expect(keypair.getPublicKey().toBase64()).toEqual(
-      'Gy9JCW4+Xb0Pz6nAwM2S2as7IVRLNNXdSmXZi4eLmSA=',
-    );
+      // Keypair derived from 32-byte secret key
+      const raw = fromB64(t[1]);
+      if (raw[0] !== 0 || raw.length !== PRIVATE_KEY_SIZE + 1) {
+        throw new Error('invalid key');
+      }
+      const imported = Ed25519Keypair.fromSecretKey(raw.slice(1));
+      expect(imported.getPublicKey().toSuiAddress()).toEqual(t[2]);
+    }
   });
 
   it('generate keypair from random seed', () => {
-    const keypair = Ed25519Keypair.fromSeed(Uint8Array.from(Array(32).fill(8)));
+    const keypair = Ed25519Keypair.fromSecretKey(
+      Uint8Array.from(Array(PRIVATE_KEY_SIZE).fill(8)),
+    );
     expect(keypair.getPublicKey().toBase64()).toEqual(
       'E5j2LG0aRXxRumpLXz29L2n8qTIWIY3ImX5Ba9F9k8o=',
     );
@@ -64,17 +82,6 @@ describe('ed25519-keypair', () => {
     expect(isValid).toBeTruthy();
   });
 
-  it('derive ed25519 keypair from path and mnemonics', () => {
-    // Test case generated against rust: /sui/crates/sui/src/unit_tests/keytool_tests.rs#L149
-    const keypair = Ed25519Keypair.deriveKeypair(TEST_MNEMONIC);
-    expect(keypair.getPublicKey().toBase64()).toEqual(
-      'aFstb5h4TddjJJryHJL1iMob6AxAqYxVv3yRt05aweI=',
-    );
-    expect(keypair.getPublicKey().toSuiAddress()).toEqual(
-      '1a4623343cd42be47d67314fce0ad042f3c82685',
-    );
-  });
-
   it('incorrect coin type node for ed25519 derivation path', () => {
     expect(() => {
       Ed25519Keypair.deriveKeypair(`m/44'/0'/0'/0'/0'`, TEST_MNEMONIC);

diff --git a/sdk/typescript/test/unit/cryptography/secp256k1-keypair.test.ts b/sdk/typescript/test/unit/cryptography/secp256k1-keypair.test.ts
@@ -3,6 +3,7 @@
 
 import {
   DEFAULT_SECP256K1_DERIVATION_PATH,
+  PRIVATE_KEY_SIZE,
   Secp256k1Keypair,
 } from '../../../src';
 import { describe, it, expect } from 'vitest';
@@ -23,10 +24,14 @@ export const VALID_SECP256K1_PUBLIC_KEY = [
 ];
 
 // Invalid private key with incorrect length
-export const INVALID_SECP256K1_SECRET_KEY = Uint8Array.from(Array(31).fill(1));
+export const INVALID_SECP256K1_SECRET_KEY = Uint8Array.from(
+  Array(PRIVATE_KEY_SIZE - 1).fill(1),
+);
 
 // Invalid public key with incorrect length
-export const INVALID_SECP256K1_PUBLIC_KEY = Uint8Array.from(Array(32).fill(1));
+export const INVALID_SECP256K1_PUBLIC_KEY = Uint8Array.from(
+  Array(PRIVATE_KEY_SIZE).fill(1),
+);
 
 const TEST_MNEMONIC =
   'result crisp session latin must fruit genuine question prevent start coconut brave speak student dismiss';
@@ -58,7 +63,7 @@ describe('secp256k1-keypair', () => {
 
   it('generate keypair from random seed', () => {
     const keypair = Secp256k1Keypair.fromSeed(
-      Uint8Array.from(Array(32).fill(8)),
+      Uint8Array.from(Array(PRIVATE_KEY_SIZE).fill(8)),
     );
     expect(keypair.getPublicKey().toBase64()).toEqual(
       'A/mR+UTR4ZVKf8i5v2Lg148BX0wHdi1QXiDmxFJgo2Yb',