-
Notifications
You must be signed in to change notification settings - Fork 17
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
hacl rs p256 #779
base: main
Are you sure you want to change the base?
hacl rs p256 #779
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 think in general this looks good and my feedback is mostly nits. I do have one somewhat substantial question regarding both safety and structure, let me know what your thoughts in the matter are.
p256/src/p256.rs
Outdated
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 think the way this crate is structured is different from the way i did it with hacl-rs crates so far. I am not sure if that a problem, but I wanted to flag it.
So far the crates using hacl-rs code had the code in a hacl submodule, and the crate provides a more usable API to the functions defined by hacl-rs. This crate just exposes the hacl-rs functions. I think these functions might also have some preconditions that need to be satisfied in order for them to work well. I haven't looked at the F* code, so I don't know for sure, but I've seen this with other code. In that case, we need some wrapper code to make the use of the functions safe.
Do you think we need a wrapper here or should this stay as-is?
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.
As discussed offline, I added a feature gate to exposing the code.
This PR adds hacl-rs P256 for ECDH and ECDSA.
With this we can close #537. P384 and 521 aren't merged yet in hacl and blocked on hacl-star/hacl-star#989.
Closes #537