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

hacl rs p256 #779

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

hacl rs p256 #779

wants to merge 8 commits into from

Conversation

franziskuskiefer
Copy link
Member

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

@franziskuskiefer franziskuskiefer requested a review from a team as a code owner January 30, 2025 09:39
@franziskuskiefer franziskuskiefer self-assigned this Jan 30, 2025
keks
keks previously requested changes Feb 4, 2025
Copy link
Member

@keks keks left a 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.

ecdsa/src/p256.rs Outdated Show resolved Hide resolved
ecdsa/tests/util.rs Outdated Show resolved Hide resolved
p256/src/p256.rs Outdated
Copy link
Member

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?

Copy link
Member Author

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.

@franziskuskiefer franziskuskiefer requested a review from keks February 4, 2025 16:04
@github-actions github-actions bot dismissed keks’s stale review February 4, 2025 16:04

Review re-requested

@franziskuskiefer franziskuskiefer added the waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Meta] Integrate hacl-rs
2 participants