search

tendermint / go-crypto

DEPRECATED: Merged into https://github.com/tendermint/tendermint under `crypto`
Other
44 stars 32 forks source link

Use non-Amino encoding for PrivKey, PubKey, Signature #121

Closed cwgoes closed 6 years ago

cwgoes commented 6 years ago

Do we really want to encode private keys, public keys, and signatures with Amino? Those are all essentially just bytes anyways, and we'll need them to interoperate with external tools, many of which would probably prefer a simpler encoding scheme. At minimum, we should offer users the option.

ValarDragon commented 6 years ago

I definitely support not using Amino for these. If keys are being stored amino-fied on-chain, it will make parsing significantly harder, since most things expect DER or PEM or already decoded values.

I think it would be nice to use one of the standards for encoding private keys (i.e. PEM). DER would work well for signatures and pubkeys too. (Not suggesting PEM for these, since we don't want the header values on-chain)

liamsi commented 6 years ago

I think it would be nice to use one of the standards for encoding private keys (i.e. PEM). DER would work well for signatures and pubkeys too.

I think that's a good suggestion. In any case, I wonder if the crypto library shouldn't just return raw bytes instead and let the caller decide which encoding to choose (in cosmos-sdk / tendermint)?

ValarDragon commented 6 years ago

That would be fine too, as long as we document the raw byte representation well. (Big endian vs little endian, is length always padded to a fixed length, and if so is it prefixed by a null byte, etc.)

ValarDragon commented 6 years ago

Here is my concrete proposal for how to do this. If this sounds good, I'll just go ahead and implement this.

We make an enum for pubkey types. (value being a uint). We use uvarint encoding, and we marshal / unmarshal these pubkeys as varuint(PUBKEY_TYPE_CONSTANT) || canonical pubkey encoding We do not need upgradability in the pubkey encoding, other than what is already in the standard encoding. We also include the pubkey type constant into addresses. If there is a critical change that needs to upgrade the encoding format, I argue that it should have a different encoding code, rather than using the existing code. (If the vulnerability is anything other than the old modulus size is unsafe, I believe this will be the case. For elliptic curves, you can't just upgrade the modulus size. We have to encode modulus size in the pubkeys for most things over modular rings)

Signatures can be encoded as raw byteslices, and be parsed by their respective pubkey. (This means removing the Signature struct / interface)

ValarDragon commented 6 years ago

Closing in favor of the tendermint issue. Lets continue conversation there.