Support Cardano key derivation according to CIP3-Icarus #158

PeterBenc · 2023-12-06T16:04:31Z

What is the current state of things and why does it need to change?

currently, this lib supports secp256k1 and ed25519
while developing a Snap for Cardano, we realized that keys derived by snap, using (getBip32Entropy) are different from keys that would standard Cardano wallet give, for the same mnemonic
this inconsistency could lead to confusion and interoperability issues
currently developed Cardano snap, relies on these changes, and is blocked by this. (note that changes need to be made also to metamask-snaps and metamask-extension repos, which are ready, awaiting feedback on this)

What is the solution your changes offer and how does it work?

by supporting derivation which is consistent with Cardano standard, we enable current Cardano users to use their existing mnemonics in Metamask and get the same wallet they used until now.
similarily, we would enable current Metamask users to use their mnemonic in standard Cardano wallets and retrieve same addresses as they would through Cardano-Metamask snap.

For this reasons

we add a new derivation curve, called ed25519Bip32, this curve implements a slight modification of how standard ed25519 is used in SLIP10.
we add a new deriver, called cip3Icarus which derives keys according to CIP3 (which is the standard for Cardano)
from the interface point of view, these changes allow us to call
```
await deriveKeyFromPath({
   path: ["cip3Icarus:1852'", "cip3Icarus: 1815'"],
   curve: 'ed25519Bip32',
})
```
which returns keys consistent with Cardano derivation standard

A longer explanation, also present in the code

CIP-3 defines standards for deriving keys on Cardano.

Key attributes

Root/Master key is derived from entropy, not seed. For this implementation we work with Icarus standard as it is the most widely used. See https://github.com/cardano-foundation/CIPs/blob/09d7d8ee1bd64f7e6b20b5a6cae088039dce00cb/CIP-0003/Icarus.md.
HD node consists of a 64 byte private key, 32 byte public key and 32 byte chain code. See https://github.com/cardano-foundation/CIPs/blob/09d7d8ee1bd64f7e6b20b5a6cae088039dce00cb/CIP-0003/CIP-0003.md#master-key-generation.
For derivation of BIP32 HD nodes, it uses modified implementation of ed25519 called BIP32-Ed25519. See https://input-output-hk.github.io/adrestia/static/Ed25519_BIP.pdf.

Are there any issues or other links reviewers should consult to understand this pull request better? For instance:

All sources are referenced in the code, and explained quite extensively, namely

Reference implementations

PeterBenc

Please review by commits, thank you 🙏

PeterBenc · 2023-12-06T16:07:45Z

src/curves/ed25519Bip32.ts

+export const isValidPrivateKey = (_privateKey: Uint8Array | string | bigint) =>
+  true;
+
+export const deriveUnhardenedKeys = true;


Note that that the important difference of the bip32 version ed25519 is that it allows unhardened key derivation

PeterBenc · 2023-12-06T16:10:24Z

src/curves/curve.ts

@@ -24,7 +25,16 @@ export type Curve = {
  publicAdd: (publicKey: Uint8Array, tweak: Uint8Array) => Uint8Array;
  compressPublicKey: (publicKey: Uint8Array) => Uint8Array;
  decompressPublicKey: (publicKey: Uint8Array) => Uint8Array;
-};
+} & (


The motivation for this change, was to have place which decides whether the master node should be derived from seed (already implemented) or the new cip3Icarus way, from entropy, as defined in entropyToCip3IcarusMasterNode.

Tying masterNodeGenerationSpec to curve, seemed as the way which would require minimal changes to existing logic.

src/utils.ts

PeterBenc · 2023-12-06T16:14:20Z

src/utils.ts

 * @returns The `Uint8Array` corresponding to the `bigint` value.
 */
-export function numberToUint32(value: number) {
+export function numberToUint32(value: number, littleEndian = false) {


Needed later, to encode keyindex, for Cardano, the values are littleEndian

PeterBenc · 2023-12-06T16:16:05Z

test/vectors/derivation.json

Please note that there are no deletions in this file, just additions. Only new cip3Icarus were generated.

src/derivers/cip3Icarus.ts

Mrtenz · 2024-02-09T19:02:16Z

At first glance this looks great. I will be doing a proper review next week, thanks for your patience!

Mrtenz

I didn't get through everything yet, but here's some initial comments for you. I'm not familiar with CIP-3, so I will take some time to read more about it to properly review the algorithms added here.

Some nits related to the code style we use. Sorry for that!

src/curves/curve.ts

src/curves/ed25519Bip32.test.ts

src/curves/ed25519Bip32.ts

src/derivers/cip3Icarus.test.ts

src/derivers/cip3Icarus.ts

PeterBenc

@Mrtenz I was initially waiting for your full review, but then thought that you might be waiting for the cip3Icarus => cip3 refactor, so I did it. Your review came literally one second after I pushed :D. Sorry for that, please check these 3 commits too.

PeterBenc · 2024-02-19T12:30:09Z

test/vectors/derivation.json

Did this in separate commit so I can squash it easier

PeterBenc · 2024-02-19T12:31:23Z

src/derivers/bip39.ts

-      curve,
-    );
+  switch (curve.masterNodeGenerationSpec) {
+    case 'slip10':


You haven't asked for this, but, I noticed it now, the switch is IMO safer

PeterBenc · 2024-03-04T10:17:37Z

@Mrtenz have you had time to look into cip3?
PS: all relevant links are in the PR description :)

Mrtenz · 2024-03-04T10:20:05Z

@Mrtenz have you had time to look into cip3? PS: all relevant links are in the PR description :)

Not yet, sorry about that. Will make time to look into this more this week!

Mrtenz

I reviewed the cryptography to the best of my ability and it looks good. Just a few more minor nits, and some code is missing coverage. Once that's fixed I will approve!

src/curves/ed25519Bip32.ts

src/derivers/cip3.ts

src/utils.ts

Mrtenz · 2024-03-06T17:48:31Z

Can you rebase your branch?

…rationSpec`

To simplify the interface

So we can validate against it when creating fingerprint

…to switch

Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>

PeterBenc · 2024-03-06T20:09:46Z

@Mrtenz the last couple of issues should be resolved. Should I squash the commits or do you want to do it yourself?

Mrtenz · 2024-03-06T20:38:06Z

src/index.ts is missing coverage(?), and you need to run yarn lint:fix. Looks good to me other than that!

PeterBenc · 2024-03-07T09:18:45Z

Should be fixed

Should I squash the commits into some reasonable chunks, would that be relevant for audit too? do you want to do it yourself? Let me know :)

Mrtenz · 2024-03-07T09:56:34Z

No need to. We want to do an audit for some other smaller changes since the last audit anyway, so I will just squash when merging this PR.

PeterBenc requested a review from a team as a code owner December 6, 2023 16:04

PeterBenc commented Dec 6, 2023

View reviewed changes

PeterBenc changed the title ~~Support cardano key derivation cip3 icarus~~ Support Cardano key derivation according to CIP3-Icarus Dec 6, 2023

Mrtenz reviewed Feb 7, 2024

View reviewed changes

src/derivers/cip3Icarus.ts Outdated Show resolved Hide resolved

Mrtenz requested changes Feb 19, 2024

View reviewed changes

PeterBenc commented Feb 19, 2024

View reviewed changes

Mrtenz requested changes Mar 6, 2024

View reviewed changes

src/curves/ed25519Bip32.ts Outdated Show resolved Hide resolved

src/derivers/cip3.ts Show resolved Hide resolved

src/derivers/cip3.ts Outdated Show resolved Hide resolved

src/utils.ts Outdated Show resolved Hide resolved

PeterBenc and others added 19 commits March 6, 2024 20:41

Add cip3Icarus fixtures

e656167

Add ed25519Bip32 derivation curve

d51f073

Differentiate master node derivation based on curve `masterNodeGene…

d054935

…rationSpec`

Support 64 byte long private keys

996576d

Support fingerprints from 32 byte long public keys

03034a5

Allow littleEndian in numberToUint32 util

b8b70a8

Add cip3Icarus deriver

be62e89

Add CIP3IcarusNode

e6cf55b

Generate and test cip3Icarus vectors

c22acb3

Add cip3Icarus test vectors

48d0dfa

Rename cip3Icarus to cip3

f7a56f2

To simplify the interface

fixup! rename cip3Icarus to cip3 in derivation test vectors

97c8050

Add compressedPublicKeyLength curve parameter

c719f5a

So we can validate against it when creating fingerprint

Refactor bip39 deriveChildKey masterNodeGenerationSpec condition …

f18654a

…to switch

fixup! curve specification to separate type

8d16e64

fixup! remove underscore

004ed52

fixup! inline unnecessary functions

8a51dd8

fixup! add JSDOCs to ed25519Bip32 file

23bca32

fixup! quotes in src/curves/ed25519Bip32.test.ts

70d89a3

Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>

PeterBenc and others added 12 commits March 6, 2024 20:43

fixup! quotes in src/curves/ed25519Bip32.test.ts

8ce367b

Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>

fixyup! empty line in src/derivation.test.ts

2ebaf4e

Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>

fixup! eslint-enable no-bitwise in src/derivers/bip39.ts

44bec07

Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>

fixup! add empty lines between test in cip3 file

963fd3a

fixup! add JSDOCs to cip3 file

08288d3

fixup! _ in src/curves/ed25519Bip32.ts

94ff5d3

Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>

fixup! improve error message in src/derivers/cip3.ts

269db31

Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>

fixup! improve error message in src/utils.ts

ab81cfa

Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>

fixup! fix quotes and test in fixes from github

f2233e5

Test all fns in ed25519Bip32

68f5ade

Test all fns in cip3

af22882

fixup! add test to have 100% coverage

e6b759a

PeterBenc force-pushed the support-cardano-key-derivation-cip3-icarus branch from 4da9f3a to e6b759a Compare March 6, 2024 20:07

PeterBenc added 2 commits March 7, 2024 10:08

fixup! test coverage

31f072b

fixup! lint

5979566

Mrtenz approved these changes Mar 7, 2024

View reviewed changes

Mrtenz merged commit bfa679e into MetaMask:main Mar 7, 2024
17 checks passed

doperiddle mentioned this pull request Nov 8, 2024

[Snyk] Upgrade @metamask/key-tree from 7.0.0 to 9.1.2 doperiddle/aptos-snap#3

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Cardano key derivation according to CIP3-Icarus #158

Support Cardano key derivation according to CIP3-Icarus #158

PeterBenc commented Dec 6, 2023

PeterBenc left a comment

PeterBenc Dec 6, 2023

PeterBenc Dec 6, 2023

PeterBenc Dec 6, 2023

PeterBenc Dec 6, 2023

Mrtenz commented Feb 9, 2024

Mrtenz left a comment

PeterBenc left a comment

PeterBenc Feb 19, 2024

PeterBenc Feb 19, 2024

PeterBenc commented Mar 4, 2024

Mrtenz commented Mar 4, 2024

Mrtenz left a comment •

edited

Loading

Mrtenz commented Mar 6, 2024

PeterBenc commented Mar 6, 2024

Mrtenz commented Mar 6, 2024

PeterBenc commented Mar 7, 2024

Mrtenz commented Mar 7, 2024

Support Cardano key derivation according to CIP3-Icarus #158

Support Cardano key derivation according to CIP3-Icarus #158

Conversation

PeterBenc commented Dec 6, 2023

What is the current state of things and why does it need to change?

What is the solution your changes offer and how does it work?

Are there any issues or other links reviewers should consult to understand this pull request better? For instance:

PeterBenc left a comment

Choose a reason for hiding this comment

PeterBenc Dec 6, 2023

Choose a reason for hiding this comment

PeterBenc Dec 6, 2023

Choose a reason for hiding this comment

PeterBenc Dec 6, 2023

Choose a reason for hiding this comment

PeterBenc Dec 6, 2023

Choose a reason for hiding this comment

Mrtenz commented Feb 9, 2024

Mrtenz left a comment

Choose a reason for hiding this comment

PeterBenc left a comment

Choose a reason for hiding this comment

PeterBenc Feb 19, 2024

Choose a reason for hiding this comment

PeterBenc Feb 19, 2024

Choose a reason for hiding this comment

PeterBenc commented Mar 4, 2024

Mrtenz commented Mar 4, 2024

Mrtenz left a comment • edited Loading

Choose a reason for hiding this comment

Mrtenz commented Mar 6, 2024

PeterBenc commented Mar 6, 2024

Mrtenz commented Mar 6, 2024

PeterBenc commented Mar 7, 2024

Mrtenz commented Mar 7, 2024

Mrtenz left a comment •

edited

Loading