sui icon indicating copy to clipboard operation
sui copied to clipboard

crypto: Ed25519 fromSecretKey takes pure 32 bytes privkey

Open joyqvq opened this issue 3 years ago • 6 comments

https://github.com/MystenLabs/sui/issues/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

joyqvq avatar Dec 14 '22 14:12 joyqvq

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
explorer ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 20, 2023 at 4:26PM (UTC)
explorer-storybook ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 20, 2023 at 4:26PM (UTC)
frenemies ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 20, 2023 at 4:26PM (UTC)
wallet-adapter ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 20, 2023 at 4:26PM (UTC)

vercel[bot] avatar Dec 14 '22 14:12 vercel[bot]

💳 Wallet Extension has been built, you can download the packaged extension here: https://github.com/MystenLabs/sui/actions/runs/3695948697#artifacts

github-actions[bot] avatar Dec 14 '22 14:12 github-actions[bot]

do we have any length constraint on the privkey field here``` export type ExportedKeypair = { schema: SignatureScheme; privateKey: string; };

joyqvq avatar Dec 14 '22 15:12 joyqvq

@joyqvq I think this will break fromExportedKeypair

Because when we export it here we use the private key as it comes from nacl keypair that also includes the public key, right?

pchrysochoidis avatar Dec 14 '22 15:12 pchrysochoidis

We should update this to export only the 32 bytes of the private key otherwise it breaks the wallet. Alternatively we can ensure that the private key (secretKey) we store in the constructor is 32 bytes and doesn't include the public key

Also question, why we want to change the private key (as defined from nacl) to 32 bytes? If nacl uses 64 why not just keep that?

there is indeed confusion since the rust library we use takes 32 bytes as privkey and the key derivation library in typescript we use also uses 32 bytes privkey.

export const getPublicKey = (
  privateKey: Uint8Array,
  withZeroByte = true
): Uint8Array => {
  const keyPair = nacl.sign.keyPair.fromSeed(privateKey);
  const signPk = keyPair.secretKey.subarray(32);
  const newArr = new Uint8Array(signPk.length + 1);
  newArr.set([0]);
  newArr.set(signPk, 1);
  return withZeroByte ? newArr : signPk;
};

https://github.com/dchest/tweetnacl-js/issues/166 there is some discussion in the nacl library on this too...

imo we should keep this consistent with rust, need to fix the conversion and document them in comment extensively in typescript.

joyqvq avatar Dec 16 '22 21:12 joyqvq

i realized that fromSeed and fromPrivateKey is not the same (although they return the same result some times). privkey is the result of hashing and bit-twiddling of the seed. in our rust library we use privkey as seed and so should typescript. going to rework this PR to ensure they are consistent.

joyqvq avatar Jan 13 '23 07:01 joyqvq