Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Interchangeable key signing and addressing #4941

Closed
4 tasks
austinabell opened this issue Aug 21, 2019 · 5 comments · Fixed by #5278
Closed
4 tasks

Interchangeable key signing and addressing #4941

austinabell opened this issue Aug 21, 2019 · 5 comments · Fixed by #5278
Labels
C:Keys Keybase, KMS and HSMs

Comments

@austinabell
Copy link
Contributor

austinabell commented Aug 21, 2019

Summary

Currently, the tendermint crypto Private and Public key implementations are defaulted to the Tendermint secp256k1 implementation here0, here1, here2 and here3 and it would be nice for this to be modular and have the option to specify the interface implementation for application specific signing and addressing needs.

Problem Definition + Proposal

This feature is needed to be able to swap out signing, addressing and signature verification from the defaulted implementations. For example if an application wanted to be able to use Ethereum signatures and addressing to be able to keep consistent state or just be able to import a key and have the same address (reformatted to bech32 style but still able to retrieve the same address from bytes) they would have to reimplement all of the keys commands and functionality just to change the type. In the case of Ethereum signatures and addressing, the signature may be interchangeable in some way (I'm not sure the exact differences but they are both ECDSA signatures) but the addressing that is done implicitly from the implementation that follows part of the bitcoin addressing style of RIPEMD160(SHA256(pubkey)) cannot be changed and from that you cannot derive, for example, an Ethereum address.

This feature would greatly help applications like Ethermint which has state and balances that would need to be mapped from their existing private keys/ seed but also for any bridge contract to a CosmosSDK zone which uses a consistent key implementation as the departure chain to not rely on a user to generate a new key and provide that address as extra data.

In the future, it would be nice to also use other curves other than secp256k1 (seems this framework is set up already so I won't go into detail).

The only issue I see with this is when a user imports a key from a familiar chain to Cosmos, they may not notice the bech32 reformatting of the address, so would be nice to also add a way to print out an additional field where an application developer can add an additional KeyOutput field to indicate a derivation from the key of their choosing.

This proposal would be to allow the Tendermint Privkey and Pubkey interface implementations to be swapped out for any application specific needs.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@alexanderbez
Copy link
Contributor

Thanks @austinabell! Of the links you posted only the first two are related to this issue as the Keybase makes these assumptions -- the other two links are specific to secp256k1 Ledger so they should remain as such unless I'm mistaken?

The basic gist that I gather from this is that the Keybase needs to support different addressing schemes and the CLI should support outputting in various formats apart from Bech32?

@alexanderbez alexanderbez added the C:Keys Keybase, KMS and HSMs label Aug 23, 2019
@alexanderbez alexanderbez added this to the v0.38.0 milestone Aug 23, 2019
@austinabell
Copy link
Contributor Author

Thanks @austinabell! Of the links you posted only the first two are related to this issue as the Keybase makes these assumptions -- the other two links are specific to secp256k1 Ledger so they should remain as such unless I'm mistaken?

I'm not sure about your ledger interaction as it isn't something I have looked into yet but I think we may be talking about different things. I am not talking about overriding the secp256k1 curve for the private and public keys, I am just simply saying with those keys to be able to change the functions for signing and more importantly addressing from the key, which would require a different interface implementation than the one that is defaulted in these scenarios.

In all cases, the structs that are generated are used as tendermint PrivKey and PubKey interfaces and it would be beneficial for an application developer to be able to change the implementation from the defaulted functionality defined here to one that would allow them to derive their own addresses based on their needs instead of using the tendermint specific implementation.

Another alternative would maybe be to just add a flag to allow an Ethereum compatible key implementation to allow consistent addressing and signing, which wouldn't give someone complete control over compatibility with other chains/ key signing types but would allow compatibility with Ethereum and Bitcoin because correct me if I'm wrong:

  • Bitcoin address can be derived from the address you generate by adding the networkID byte to the front and the 2xSHA256 checksum of the address bytes you generate through your implementation as the last 4 bytes and it'll equal what a bitcoin address from the same key would generate. I will assume the signatures are consistent but I'm not sure
  • Ethereum implementation would just take the last 20 bytes of the public key which would be compatible with Ethereum state, and same with the signature.

This alternative may be easier and solve the needs of basically every developer, which would make it easier for developers trying to migrate from Ethereum so it may be better that way anyway for what you guys want.

The basic gist that I gather from this is that the Keybase needs to support different addressing schemes and the CLI should support outputting in various formats apart from Bech32?

I guess I could have separated talking about the bech32 output I mentioned in the issue, but I just mentioned that as a user experience benefit to be able to show a different addressing format. The reason I mentioned that in the context of the other changes is that if the key implementation is set to follow, for example, an Ethereum specification for addressing, a user would not be able to verify that the address is consistent since it would be reformatted as a bech32 encoded string.

If this part is confusing I can try to explain differently or just remove that reference from the issue and just come back to it at a later time as it is entirely cosmetic and does not change the functionality of the underlying signing and addressing.

@alexanderbez
Copy link
Contributor

alexanderbez commented Aug 26, 2019

I'm not sure about your ledger interaction as it isn't something I have looked into yet but I think we may be talking about different things.

You referenced them: here and here.

I am not talking about overriding the secp256k1 curve for the private and public keys, I am just simply saying with those keys to be able to change the functions for signing and more importantly addressing from the key, which would require a different interface implementation than the one that is defaulted in these scenarios....

In all cases, the structs that are generated are used as tendermint PrivKey and PubKey interfaces and it would be beneficial for an application developer to be able to change the implementation from the defaulted functionality defined here to one that would allow them to derive their own addresses based on their needs instead of using the tendermint specific implementation....

I believe I follow. Hence my comment on refactoring Keybase to support an interface instead of hard-coded functionality. Am I missing anything else?

@austinabell
Copy link
Contributor Author

You referenced them: here and here.

Sorry I just meant I haven't looked into detail about how specific the integration is and if its possible to change function implementations of these keys that are based on the same curve is why I included.

I believe I follow. Hence my comment on refactoring Keybase to support an interface instead of hard-coded functionality. Am I missing anything else?

Nope, that should be about it! I just figured I'd give as much detail and context as I thought necessary for a proposal.

@jackzampolin
Copy link
Member

Related to #4789

@alexanderbez alexanderbez removed this from the v0.38.0 milestone Nov 30, 2019
alessio pushed a commit that referenced this issue Dec 12, 2019
Allow for the keybase to be configured to override the implementation
of the key that is saved to the keybase.

Closes: #4941
xiangjianmeng pushed a commit to xiangjianmeng/cosmos-sdk that referenced this issue Dec 18, 2019
Allow for the keybase to be configured to override the implementation
of the key that is saved to the keybase.

Closes: cosmos#4941
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:Keys Keybase, KMS and HSMs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants