Add AffineCoordinates trait + DRY out point serialization
#50
Conversation
Adds a trait for fixed-base scalar multiplication which accepts `&ScalarBytes` as input and returns an associated point type, whose bounds allow for a `From` conversion to either the `CompressedCurvePoint` or `UncompressedCurvePoint` for a given curve. Using this trait, a `weierstrass::PublicKey<C>::from_secret_key` method is conditionally implemented when the curve `C` impls the `FixedBaseScalarMul` trait, allowing generic computation of a public key from a secret key, with optional point compression (selected via a `compress` argument).
Codecov Report
@@ Coverage Diff @@
## master #50 +/- ##
==========================================
- Coverage 82.02% 81.46% -0.56%
==========================================
Files 12 12
Lines 1296 1354 +58
==========================================
+ Hits 1063 1103 +40
- Misses 233 251 +18
Continue to review full report at Codecov.
|
Adds a trait for obtaining the x and y-coordinates of an affine point. So as to avoid exposing field elements as part of the public API, the coordinates are first serialized as bytes. Using this trait, it's possible to generically implement compressed and uncompressed point serialization according to the SEC-1 Elliptic-Curve-Point-to-Octet-String encoding. This commit also modifies the trait bounds of `FixedBaseScalarMul` to bound on the `AffineCoordinates` trait, allowing the result to be an affine point for the curve which can be passed as input to the newly added `from_affine_point` methods on `CompressedCurvePoint` and `UncompressedCurvePoint`.
tarcieri
force-pushed
the
affine-coordinates-trait
branch
from
9fd8ae5
to
7edcd0c
Compare
| /// Trait for obtaining the coordinates of an affine point | ||
| pub trait AffineCoordinates { | ||
| /// Size of a byte array representing an affine coordinate | ||
| type ScalarSize: ArrayLength<u8>; | ||
|
|
||
| /// x-coordinate | ||
| fn x(&self) -> ScalarBytes<Self::ScalarSize>; | ||
|
|
||
| /// y-coordinate | ||
| fn y(&self) -> ScalarBytes<Self::ScalarSize>; | ||
| } |
There was a problem hiding this comment.
This abstraction directly exposes the coordinates instead of the effect of using them. Making it a separate trait is better than having it be part of the core traits, but it still implies that coordinates are an API surface that is supported and encouraged.
I would prefer instead e.g. an EcdsaPrimitive trait that requires implementing the inner part of the ECDSA algorithm that requires coordinates, and then EcdsaCurve: EcdsaPrimitive that provides the full algorithm, which users can import without exposing the inner API unnecessarily. This is similar to the rsa::PrivateKey: rsa::DecryptionPrimitive split.
Separately, using ScalarBytes for the coordinates strongly encourages type errors during implementation of the trait. The coordinates are field elements, and scalars (of the affine point's curve) are field elements (of a different field), but the coordinates are not scalars (in the sense that elliptic curve scalars are generally understood).
| UncompressedPointSize<Self::ScalarSize>: ArrayLength<u8>, | ||
| { | ||
| /// Affine point type for this elliptic curve | ||
| type Point: AffineCoordinates + ConditionallySelectable + Default; |
There was a problem hiding this comment.
Fixed-base scalar multiplication doesn't require exposing affine coordinates, so this is a more restrictive abstraction than necessary. I suspect we can address the type bounds issue in another way.
|
Okay, based on @str4d's comments this is too leaky a design, so let me propose an alternative.
What if we had something like: pub struct Coordinate<C: Curve> {
bytes: ScalarBytes<C::ScalarSize>
}...but with a twist: while it's possible for library-level code to instantiate, say with this sort of API: impl<C: Curve> From<ScalarBytes<C::ScalarSize>> {
...
}...to everything but the This could be used by the |
|
Alternatively to that, we could keep the bounds from #49, but still add Concrete curve implementations could then invoke those methods directly, rather than exposing them as a public trait. This keeps the coordinates encapsulated while still allowing for generic SEC-1 serialization, which seems like a nice middle ground. I might open a separate PR based on #49 which implements that idea. |
|
Closing out this PR as this does not seem like the right abstraction. I'll open another PR to DRY out SEC-1 encoding by incorporating it directly into the |
Protocols like ECDSA need access to affine coordinates: the x-coordinate for ECDSA itself, but also the y-coordinate for things like recovering public keys from ECDSA signatures (as seen in Ethereum). This commit adds support for accessing affine coordinates for use in these protocols, taking into account some feedback from the previous attempt to do this (#50): - Trait is placed inside a `legacy` module with comments strongly suggesting it should only be used for implementing legacy protocols which explicitly need coordinate access - Feature is hidden from https://docs.rs to discourage (mis)use - Coordinates are represented using a `FieldElementBytes` type alias which is also defined in the `legacy` module
Protocols like ECDSA need access to affine coordinates: the x-coordinate for ECDSA itself, but also the y-coordinate for things like recovering public keys from ECDSA signatures (as seen in Ethereum). This commit adds support for accessing affine coordinates for use in these protocols, taking into account some feedback from the previous attempt to do this (#50): - Trait is placed inside a `legacy` module with comments strongly suggesting it should only be used for implementing legacy protocols which explicitly need coordinate access - Feature is hidden from https://docs.rs to discourage (mis)use - Coordinates are represented using a `FieldElementBytes` type alias which is also defined in the `legacy` module
See RustCrypto/elliptic-curves#50 for some historic context. After being able to get by on `AffineXCoordinate` for generic ECDH and ECDSA, #1199 added an `AffineYIsOdd` trait which was needed to enable the generic ECDSA implementation in the `ecdsa` crate to compute the "recovery ID" for signatures (which is effectively point compression for the `R` curve point). This commit consolidates `AffineXCoordinate` and `AffineYIsOdd` into an `AffineCoordinates` trait. Some observations since prior discussion in RustCrypto/elliptic-curves#50: - Access to coordinates is through bytes, namely `FieldBytes`. This is so as to avoid exposing a crate's field element type. This approach isn't type safe (base field elements and scalar field elements share the same serialization) but does make ECDSA's weird reduction of a base field element into the scalar field straightforward in generic code. - Prior to this attempts were made to extract ECDSA-specific bits into a trait to handle these conversions, but it complicates both writing generic code and optimizing performance. While this still might be worth exploring, so far those explorations have largely failed. - Generally there have been a lot of requests for coordinate access specifically for things like point serialization formats. We ended up adding "compaction" support upstream but we have had requests for several other formats (e.g. Elligator Squared) where direct coordinate access would be useful. This trait can hopefully be replaced by a coordinate access API provided by the `group` crate in the future. See zkcrypto/group#30
See RustCrypto/elliptic-curves#50 for some historic context. After being able to get by on `AffineXCoordinate` for generic ECDH and ECDSA, #1199 added an `AffineYIsOdd` trait which was needed to enable the generic ECDSA implementation in the `ecdsa` crate to compute the "recovery ID" for signatures (which is effectively point compression for the `R` curve point). This commit consolidates `AffineXCoordinate` and `AffineYIsOdd` into an `AffineCoordinates` trait. Some observations since prior discussion in RustCrypto/elliptic-curves#50: - Access to coordinates is through bytes, namely `FieldBytes`. This is so as to avoid exposing a crate's field element type. This approach isn't type safe (base field elements and scalar field elements share the same serialization) but does make ECDSA's weird reduction of a base field element into the scalar field straightforward in generic code. - Prior to this attempts were made to extract ECDSA-specific bits into a trait to handle these conversions, but it complicates both writing generic code and optimizing performance. While this still might be worth exploring, so far those explorations have largely failed. - Generally there have been a lot of requests for coordinate access specifically for things like point serialization formats. We ended up adding "compaction" support upstream but we have had requests for several other formats (e.g. Elligator Squared) where direct coordinate access would be useful. This trait can hopefully be replaced by a coordinate access API provided by the `group` crate in the future. See zkcrypto/group#30
NOTE: this PR includes a commit from #49 which should get merged first
Adds a trait for obtaining the x and y-coordinates of an affine point.
So as to avoid exposing field elements as part of the public API, the coordinates are first serialized as bytes.
Using this trait, it's possible to generically implement compressed and uncompressed point serialization according to the SEC-1 Elliptic-Curve-Point-to-Octet-String encoding.
This commit also modifies the trait bounds of
FixedBaseScalarMulto bound on theAffineCoordinatestrait, allowing the result to be an affine point for the curve which can be passed as input to the newly addedfrom_affine_pointmethods onCompressedCurvePointandUncompressedCurvePoint.