-
Notifications
You must be signed in to change notification settings - Fork 111
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
Upgrade curv
interface
#120
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like the design overall
The idea to use Option<SK/PK> to represent the point at infinity is VERY interesting, on one hand it's a good solution that doesn't violate the underlying library's API, on the other hand it makes future constant time operations harder, but we don't worry about this right now too much.
src/elliptic/curves/traits.rs
Outdated
/// | ||
/// Returns None if point doesn't have coordinates, ie. it is "at infinity". If point isn't | ||
/// at infinity, serialize always succeeds. | ||
fn serialize(&self, compressed: bool) -> Option<Vec<u8>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this integrates with curves like curve25519 (that don't have compressed/uncompressed)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question! Old ECPoint trait had methods pk_to_key_slice
and bytes_compressed_to_big_int
that return point in uncompressed/compressed form respectively. Let's see how they were implemented for ed25519:
fn pk_to_key_slice(&self) -> Vec<u8> {
let result = self.ge.to_bytes();
result.to_vec()
}
fn bytes_compressed_to_big_int(&self) -> BigInt {
BigInt::from_bytes(self.ge.to_bytes()[0..self.ge.to_bytes().len()].as_ref())
}
Seems they return semantically the same value. So I guess I should do the same in the new ed25519 implementation?
src/elliptic/curves/wrappers.rs
Outdated
/// | ||
/// Holds internally a reference on [`Point<E>`](Point), refer to its documentation to learn | ||
/// more about Point/PointRef guarantees, security notes, and arithmetics. | ||
pub struct PointRef<'p, E: Curve> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is PointRef
needed? (over &Point
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have a look at Point::base_point2() and PointRef::base_point2() methods.
They use ECPoint::base_point2()
method underhood that returns a static reference to the 2nd base point: &'static Self
. PointRef<'p, E>
wraps &'p E
(in the same way as Point<E>
wraps E
) and provides the same abilities as Point<E>
has (arithmetic, serialization, etc.).
I'm very like this mechanism - old interface base_point2()
method is less efficient as it constructs a point every time it called. I managed to workaround it, now the base_point2 is constructed only once, and the trait gives you a reference to its value.
src/elliptic/curves/wrappers.rs
Outdated
} | ||
} | ||
|
||
macro_rules! matrix { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh god
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So Mul/Add etc are implemented but via the macro
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can probably uplift all the reference variations of the implementation into the macro, so it will implement it for the references too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the same (I think) idea of uplifting the reference, but things got more difficult when I introduced PointRef
. We should not implement operators for &PointRef
, as it's the same as implementing operators for &&Point
(we can do this, but I don't like it).
If you have any ideas how to simplify the macro - feel free to give it a try, and suggest your changes!
src/elliptic/curves/secp256_k1.rs
Outdated
let n_reduced = BigInt::mod_add(n, &BigInt::from(0), &curve_order); | ||
let mut v = BigInt::to_bytes(&n_reduced); | ||
let curve_order = Self::curve_order(); | ||
let n_reduced = n.modulus(curve_order); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about returning an error if n
is bigger than the curve order instead of reducing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the code we have now relies on from_bigint
reducing n
. I think if we just change behaviour, something will get broken without notifying us! But it might make sense to add method try_from_bigint
, what do you think?
@elichai thank you for review! Your comments are very valuable!
I think, to support constant time operations, we need underlying library to support points at infinity. E.g. C library that |
Yeah I've been thinking about this for a while, See my comment here: #43 (comment) |
} | ||
|
||
fn from_coor(_x: &BigInt, _y: &BigInt) -> RistrettoCurvPoint { | ||
unimplemented!(); | ||
fn x_coord(&self) -> Option<BigInt> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
x_coord
and from_coord
should be kept unimplemented . The discussion you point to is not good enough evidence to take the hash of y
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to define function similar to xrecover from ed25519 but for ristretto curve?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed ristretto curve to return x=None instead of x=hash(y), and from_coords to always returns Err(NotOnCurve)
86a1844
to
1357bd5
Compare
Progress:
Unresolved questions (subject to move to separate issues):
Bunch of libraries need to be updated, including (but not limiting):
p256
,digest
,sha2
See the badge:
curve25519-dalek
for ed25519 implementation instead ofcryptoxide
curve25519-dalek
seems to be well-designed and well-documented, it's more actively used thancryptoxide
(3 500 000 vs 100 000 total downloads).Besides, we already rely on
curve25519-dalek
crate to supply ristretto curveI'd like to add
from_uniform_bytes
constructor to ECPoint trait that maps uniformly distributed bytes to a point with unknown discrete log. Only ristretto, bls12_381, and ed25519 curves have implementation of hash-to-curve. P256 and K256 require research on this.Current implementation, in order to validate the point (whether is has order=group_order), it just checks if
rP = O
(r=group_order). There are new techniques making use of endomorphisms for performing faster subgroup checks