hashgraph / hashgraph/hedera-sdk-reference

Refactor key derivation

Open
#73 17 comments 0 reactions 0 assignees View on GitHub
discussion
Dominant language
HTML
Stars
7
Forks
3
PR merge metrics
No merged PRs in 30d

Description

There are problems with how key derivations are currently implemented. This issue is for discussing the best solution to the existing problems, and settling on a course of action.

## Background

- https://github.com/hashgraph/hedera-sdk-java/issues/1263
- https://github.com/hashgraph/hedera-sdk-js/issues/1350

## Proposal (will be updated as discussion occurs)

### Converting Mnemonics to Private Keys

1. `Mnemonic.toStandardEd25519PrivateKey(passphrase="", index=0)`
- returns the child key at derivation path `m/44'/3030'/0'/0'/index'`
- this function automatically hardens `index`. Passing in a pre-hardened index **should fail**
2. `Mnemonic.toStandardECDSAsecp256k1PrivateKey(passphrase="", index=0)`
- returns the child key at derivation path `m/44'/3030'/0'/0/index`
- if the user wants a hardened child, they must **manually** harden `index`
3. `Mnemonic.toLegacyPrivateKey()` returns an `Ed25519` private key
- this function should choose the *type* of legacy derivation based on mnemonic word count / composition
- 22 words from the non-standard list means legacy derivation v1
- 24 words from the standard list means legacy derivation v2

### Deriving Child Keys

1. `PrivateKey.derive(index)`
- returns the child key at `index`
- if `PrivateKey` is an Ed25519 key, `index` is automatically hardened. passing in a pre-hardened index **should fail**
- if `PrivateKey` is an ECDSA key, `index` must be manually hardened, if desired
2. `PrivateKey.legacyDerive(index)`
- returns the child key at `index`, using the legacy algorithm
- only valid for Ed25519 keys. Calling this on an ECDSA key **should fail**

### Deprecated Functions (including but not necessarily limited to)

1. `Mnemonic.toEd25519PrivateKey(passphrase = "", path = ...)`
- since using the default path returns a key that shouldn't be used
2. `Mnemonic.toECDSAPrivateKey(passphrase = "", path = ...)`
- since the default path is entirely wrong, and returns a non-standard key (JS only)

### Tests

1. All test vectors described in [SLIP10](https://github.com/satoshilabs/slips/blob/master/slip-0010.md#test-vectors), ~[BIP32](https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki)~, and [BIP39](https://github.com/bitcoin/bips/blob/master/bip-0039.mediawiki#generating-the-mnemonic) should be checked for all key types
- Some refactoring will be necessary to support test vectors from seed
- BIP-32 test requirement removed for now, since base58 extended key encoding isn't supported
2. The following test vectors should be created and standardized across SDKs
- `legacyMnemonicV1`
- mnemonic to key
- legacy derivation (if applicable, see question # 1 below)
- `legacyMnemonicV2`
- mnemonic to key
- legacy derivation (if applicable, see question # 1 below)
- `toStandardEd25519PrivateKey`
- mnemonic to key with no passphrase and index `0`
- mnemonic to key with passphrase and index `0`
- mnemonic to key with passphrase and index `max`
- `toStandardECDSAsecp256k1PrivateKey`
- mnemonic to key with no passphrase and index `0`
- mnemonic to key with no passphrase and index `0'`
- mnemonic to key with passphrase and index `0`
- mnemonic to key with passphrase and index `0'`
- mnemonic to key with passphrase and index `max`
- mnemonic to key with passphrase and index `max'`

### Possible future functionality (not included in the current proposal)

1. Functions that allow the user to explicitly define a full derivation path
- The functions that do this are being deprecated
- Since this is presumably not a commonly used feature, it doesn't need to be added urgently
2. *Public key* derivation
- inherently not possible with Ed25519
- should eventually be implemented for ECDSA

### Questions

1. When is `PrivateKey.legacyDerive(index)` actually needed? Is this just for *one* of the legacy versions, or *both*?
- this needs to be figured out and documented
2. Can we remove the `isLegacy` flag from `Mnemonic`?
- There currently exists some functionality to construct a mnemonic with an `isLegacy` flag, or to infer `isLegacy` based on word count
- I personally don't like this at all. Not all SDKs do this the same way, and `isLegacy` actually seems to mean "is legacy v1", since legacy v2 mnemonics are notably *not* `isLegacy`
- IMO we should rip this confusing bandaid off now, and just require the wallet implementor to explicitly call `toLegacyPrivateKey`, rather than inconsistently and imperfectly inferring `isLegacy`
4. Should we specify the curve in the ECDSA function names? (see [below](https://github.com/hashgraph/hedera-sdk-reference/issues/73#issuecomment-1353196841) for details)

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.