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

Implement GroupDigest for NistP256 and Secp256k1 #503

Merged
merged 13 commits into from
Jan 15, 2022

Conversation

daxpedda
Copy link
Contributor

@daxpedda daxpedda commented Jan 7, 2022

See RustCrypto/traits#865.
This replaces #495 where I copied some code from @mikelodder7. The reason I did that is to test changes, for example, for RustCrypto/traits#871.

Currently builds on top of RustCrypto/traits#876.

@mikelodder7
Copy link

I’d like to not have to copy osswu into k256. I’ve got it working locally with only those two changes

@daxpedda
Copy link
Contributor Author

daxpedda commented Jan 7, 2022

I agree, we will need to fix this in elliptic-curve.

EDIT: with the two changes you mean adding normalize inside the OSSWU implementation in elliptic-curve, yes?

Comment on lines +261 to +278
const TEST_VECTORS: &[TestVector] = &[
TestVector {
dst: b"HashToScalar-VOPRF08-\x00\x00\x03",
seed: &hex!("a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3"),
sk_sm: &hex!("c15d9e9ab36d495d9d62954db6aafe06d3edabf41600d58f9be0737af2719e97"),
},
TestVector {
dst: b"HashToScalar-VOPRF08-\x01\x00\x03",
seed: &hex!("a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3"),
sk_sm: &hex!("7f62054fcd598b5e023c08ef0f04e05e26867438d5e355e846c9d8788d5c7a12"),
},
];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied these test vectors from the VOPRF spec, I don't know any other spec providing test vectors for hash_to_scalar. The VOPRF spec also doesn't define any for k256.

@mikelodder7
Copy link

I'm happy to move my other PR which implements all the hash2curve to something similar to this unless you are planning to turn this into a regular PR. Thoughts?

@daxpedda
Copy link
Contributor Author

The plan is to turn this into a regular PR actually, I just can't until a new version of elliptic-curve is released. So as soon as that's done I'm going to update this.

@daxpedda
Copy link
Contributor Author

I can't make sense of this error. infinity should be a Choice, not a u8.

    Checking k256 v0.10.1 (/home/runner/work/elliptic-curves/elliptic-curves/k256)
error[E0308]: mismatched types
   --> k256/src/arithmetic/hash2curve.rs:144:23
    |
144 |             infinity: Choice::from(0u8),
    |                       ^^^^^^^^^^^^^^^^^ expected `u8`, found struct `elliptic_curve::subtle::Choice`

For more information about this error, try `rustc --explain E0308`.
error: could not compile `k256` due to previous error

@daxpedda daxpedda marked this pull request as ready for review January 14, 2022 20:55
@tarcieri
Copy link
Member

@daxpedda that was a recent change (#511) which was needed to allow an inherent constants for IDENTITY and GENERATOR, since Choice can't be used in a const context

@daxpedda
Copy link
Contributor Author

Thanks, this should be good to go now.

// if e2, y = y1, else y = y2
let mut y = Self::conditional_select(&y2, &y1, e2);

y.conditional_assign(&-y, self.normalize().sgn0() ^ y.normalize().sgn0());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed when removing the sgn0 method and internally using is_odd instead, it complained that it wasn't normalized. Does that look alright @mikelodder7?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pub fn is_odd(&self) -> Choice {
debug_assert!(self.normalized);
self.value.is_odd()
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fjarri do you think is_odd should call normalize first?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anytime comparisons are used for osswu it has to be normalized otherwise it gets wrong results

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I would assume you need to normalize, since the modulus is odd, so x and x+modulus (possible even if you have magnitude=1, but not normalized) will have different parity.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, will change is_odd to normalize first then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@daxpedda
Copy link
Contributor Author

Re-based because of #506.

@tarcieri tarcieri merged commit 8b75a07 into RustCrypto:master Jan 15, 2022
This was referenced Jan 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants