feat(keyring): add secp256r1 support to keyring

[robert-zaremba]

Description

Closes: #8861
Depends on: Migrate Keyring to Protobuf #7108

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Targeted PR against correct branch (see CONTRIBUTING.md)
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards.
• Wrote unit and integration tests
• Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• Added relevant godoc comments.
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer
• Review Codecov Report in the comment section below once CI passes

[orijbot]

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=5676309b6ed51002eaa0201fbb4d752a6c5cc567

[orijbot]

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=3219c12907f361d701b980e30094a75260aff1f6

[orijbot]

Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=76be6294ee424380bb4a91f95bf3f2f3

[aaronc]

I'm not sure what the motivation is for adding this, but thanks for working on it! Do we need to add a PrivateKey proto definition?

[robert-zaremba]

I'm not sure what the motivation is for adding this, but thanks for working on it!

@aaronc I responded in the issue. Without keyring, it's hard to play with new keys. Next one will be the sr25519 probably (though we don't need HD derivation for it).

Do we need to add a PrivateKey proto definition?

We already have it -> #8559 (comment)
The bigger issue is with Amino in fact.
Adding things to keyring requires either

• implementing Amino,
• or adding an if check in the keyring code (if key type is not secp256k1 then use protobuf, otherwise use amino).

The if check is ugly. The former solution requires adding Amino support to new key types.

[robert-zaremba]

NOTE

What left here:
decide if we want to add Amino support to secp256r1 or add a switch for codec used for marshaling.

[amaury1093]

In general, i don't see anyone asking for secp256r1 in the keyring, so I would not add it (YAGNI). But also won't block it, since it's done and I don't want your work to go to waste.

decide if we want to add Amino support

could you explain what this is concretely? afaict this PR is okay as-is.

[robert-zaremba]

could you explain what this is concretely? afaict this PR is okay as-is.

@AmauryM Saving secp256r1 keys won't work because the Amino marshaling is failing.

Main motivation of adding this support is to allow easily using it (I wanted to check a CLI transaction with secp256r1, which at the end I hackend in the code)

[orijbot]

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=4ea851fb60c0323a016f5cf2bedaeee51e0c5ae1

[amaury1093]

I believe the decision was to 1/ migrate keyring to protobuf then 2/ add secp256r1 to keyring, correct? In this case, should we draft this PR for now?

[orijbot]

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=35d57e665e42e0d11dbc3bcf093052f7f2bad59f

[alessio]

Although we can do both, keyring migration is not hard, so it would make sense to deliver both in v0.43.

That's fine. I am just wary of blocking 0.43 release on a keyring proto migration that seems definitely less important and impactful than the bugfixes for the vesting account and the new distribution params issues. That's why I think we should really work out a list of priorities and focus on what matters the most first.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[robert-zaremba]

Note: this is blocked by #9221

[webmaster128]

Are we confident that BIP32 can safely be adapted to secp256r1? It is only defined over secp256k1.

By the way, there are some references regarding BIP39 here and in #8861. But BIP39 does not care about the curve at all. It is not much more than the mnemonic encoder and a hasher. It can be used for a million things. BIP32 is where the HD derivation is specified.

[webmaster128]

BIP32 hardcodes the constant "Bitcoin seed" which is clearly not what a secp256r1 derivation algorithm should contain. SLIP-0010 is a generalization of BIP32 to some other curves. There the "Bitcoin seed" is replaces with other curve identifiers for other curves.

I think any attempt to implement HD derivation on secp256r1 should have a clean specification first.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[robert-zaremba]

This branch is waiting for Migrate Keyring to Protobuf #7108 to be merged.

[robert-zaremba]

Are we confident that BIP32 can safely be adapted to secp256r1? It is only defined over secp256k1.

Thanks @webmaster128 for chiming in. This is dully noted. As you noted, BIP39 uses BIP32. I assumed that BIP32 will work with any ECDSA scheme.

How can we verify that?

[webmaster128]

As you noted, BIP39 uses BIP32.

BIP39 (text mnemonic + password -> binary "seed") and BIP32 (HD derivation) are independent things. They are often used together, but don't need to.

BIP32 is what concerns me here. It is specified and peer-reviewed for exactly one algorithm: ECDSA over secp256k1 (In the rest of this text we will assume the public key cryptography used in Bitcoin, namely elliptic curve cryptography using the field and curve parameters defined by secp256k1 (http://www.secg.org/sec2-v2.pdf). https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki)

I assumed that BIP32 will work with any ECDSA scheme.

How can we verify that?

That is what different people seem to assume. I tried to find some Ledger dev responsible for firmware development where secp256r1 and an HD derivation for it is implemented. But I did not get any satisfying answer. Everyone seems to just assume it works. Or I did not yet find the right people.

Personally I have no clue if it works and if it is safe. But it feels like this is too important to just re-use a derivation schema for something else without any publicly available documentation and discussion on why this is safe.

[webmaster128]

As mentioned by Chris Lin in a Telegram group (💕 Chris):

secp256r1 is also called prime256v1 or NIST P-256. Its only difference from secp256k1 is the group order. Mostly you can think it as compatible with BIP32, but usually we use a different HMAC-SHA512 Key ("Nist256p1 seed")

I believe https://github.com/satoshilabs/slips/blob/master/slip-0010.md is what you are looking for.

and from that spec:

Private and public key derivation for NIST P-256 is identical to the generation for secp256k1 but uses the order of that curve as modulo. We change BIP-32 to not fail if the resulting key is not valid but retry hashing until a valid key is found.

Given that I think we should use SLIP-0010 for secp256r1 (aka. prime256v1 aka. NIST P-256) path derivation. This provides a clear document to point to. It is out there for many year already and many qualified people had the chace to raise their concerns with this construction.

We have a SLIP-0010 implementation in CosmJS already (only secp256k1 and Ed25519 for far). This can easily be extended to provide or validate test vectors.

[frumioj]

I believe https://github.com/satoshilabs/slips/blob/master/slip-0010.md is what you are looking for.

Noting that for secp256r1, that spec also says the crucial thing:

Private and public key derivation for NIST P-256 is identical to the generation for secp256k1 but uses the order of that curve as modulo. We change BIP-32 to not fail if the resulting key is not valid but retry hashing until a valid key is found.

So BIP-32 itself would work, with only one change - supporting a different curve order.

Is it worth suggesting that specific change to the authors of BIP-32, and seeking to amend that specification?

[webmaster128]

Is it worth suggesting that specific change to the authors of BIP-32, and seeking to amend that specification?

The BIPs repository authors make it very clear on every possible occasion that their specs are Bitcoin only. Since there is no use case for secp256r1 in Bitcoin, I don't see this as a promising approach. This is also the reason why to many similar but different type of specs are maintained by Satoshilabs.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[tac0turtle]

@robert-zaremba please close this if its stale.

[webmaster128]

Please ping me if this work is picked up again somewhere. I'd like to make sure we have a compatible implementation in CosmJS.