ZenGo-X / curv

Rust language general purpose elliptic curve cryptography.
MIT License
264 stars 111 forks source link

curv exports only 1 curve implementation #91

Closed survived closed 3 years ago

survived commented 3 years ago

curv exports only 1 curve implementation (secp256k1 / secp256r1 / ed25519 / etc). It may not be convenient in some cases, e.g. if I have to deal with both secp256k1 and secp256r1 simultaneously. With Rust's powerful type system, this limitation can be worked out.

Currently, to support multiple curve implementation, you define type aliases at the root of crate (FE, GE, PK, SK) which points to selected implementation based on features, and all other cryptographic code relies on those type aliases, e.g. that's the definition of VSS primitive:

#[derive(Clone, PartialEq, Debug, Serialize, Deserialize)]
pub struct VerifiableSS {
    pub parameters: ShamirSecretSharing,
    pub commitments: Vec<GE>,
}

I'd suggest using type generics here:

#[derive(Clone, PartialEq, Debug, Serialize, Deserialize)]
pub struct VerifiableSS<P> {
    pub parameters: ShamirSecretSharing,
    pub commitments: Vec<P>,
}

impl<P: ECPoint> VerifiableSS<P> {
  // ...
}

However, this is a big-scale refactoring as it will touch all dependent crates, like multi-party-ecdsa, which rely on those type synonyms too. But it will make code more flexible and Rust-ish.

omershlo commented 3 years ago

Hi @survived ! Thank you for raising this issue! I completely agree, originally, exporting only a single curve was by design: in elliptic curve cryptography applications you only need one curve and it is dangerous to work with multiple curves at the same time (if you get confused between curves even at a single operation it will have major impact on security).

Since then I realised that the above assumption is wrong (cc: @oleiba ):

As you pointed out this is a significant refactor, although I have no concerns about changing dependent crates - this would be quick and painless and since we use tagging the change can be gradual.

Would you be interested championing such contribution ?

survived commented 3 years ago

Hi @omershlo! Agreed, approach in exporting 1 curve sounds reasonable at the point of security. But sometimes we have to work with several curves.

I'm interested in this change so I'd be happy to do a contribution. I'll open PR as soon as I finish the refactoring.

omershlo commented 3 years ago

awesome - ping me via telegram @Omershlo for questions

survived commented 3 years ago

Fixed by #92