-
Notifications
You must be signed in to change notification settings - Fork 710
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 NIST P521 signature verification #1631
base: main
Are you sure you want to change the base?
Conversation
bfea65d
to
4608342
Compare
Vlad, my understanding is that you have it working for 64-bit but not 32-bit targets. is that right? Happy to have a chat with you about how to resolve the 32-bit issues. |
Yes, this is correct |
8440c57
to
4e94c35
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1631 +/- ##
==========================================
+ Coverage 96.01% 96.26% +0.24%
==========================================
Files 136 137 +1
Lines 20774 20764 -10
Branches 226 228 +2
==========================================
+ Hits 19946 19988 +42
+ Misses 793 742 -51
+ Partials 35 34 -1 ☔ View full report in Codecov by Sentry. |
4e94c35
to
5664a87
Compare
Vlad, I just created a new branch I am still looking for the program I used to generate the tops of |
It seems like I do have that program, but it isn't going to help us. I will review your PR #1702 and come back to this one. |
I copied the ec tests from boringssl, the ops tests could perhaps help understand why 32bit code fails, but my understanding that they are provided in montromery representation, which is different between 64 and 32 bits |
I will generate the ops tests for P-521 so that they work for 32-bit and 64-bit targets and share them in a PR. This should let us identify the failure for 32-bit targets. I think we need to also do merge 1758 and rebase this on top of that, as I'm not confortable with the type punning we're doing, especially when BoringSSL is no longer doing it. |
4a38b14
to
6b2b571
Compare
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.
Some more suggestions.
Regarding the missing tests: I am working on changing the point arithmetic tests so that the test data files are NOT Montgomery-encoded. See PR #1769 and PR #1770. Regarding generating tests for p521_elem_*, it looks like the Montgomery encoding issue only affects the multiplication tests. I guess I will do something similar to what I am doing in #1770 for those. |
Now that PR #1756 is merged, you can run |
@briansmith @vkrasnov Is there a way I can help getting this merged? (I really need this; see denoland/deno#21169). I can take action on the review, but I am wary of doing this without asking first. |
If it helps for reference, rustcrypto also very recently imlemented signing for P521 RustCrypto/elliptic-curves#953 RustCrypto/elliptic-curves#956 |
6b2b571
to
806e855
Compare
806e855
to
e864d37
Compare
The clippy failure is due to the newest Rust toolchain having a stricter clippy. If you rebase on top of main then you will get the fix, which was in #1888. |
e864d37
to
8086779
Compare
8086779
to
17c7ead
Compare
This change makes it easier to reuse the P384 code which is quite generic already. No algorithmic changes are made, only some code is shuffled around. This prepares the ground for P521 implementation.
This code almost entirely reuses the P384 code, and only implements the dedicated inversion routines on top of the code generated by generate_curves.py
17c7ead
to
561a613
Compare
561a613
to
932beef
Compare
How can I help move this forward? |
This could be interesting for the Rustup project as well. We've switched from It would definitely be nice if we had more options to choose from, such as |
Need a way to work on the 32bit part, as 521 is kinda awkward in this regard, because number of limbs in 32 bit mode is not double that of 521 mode.