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

Expose blst internals #6829

Merged
merged 12 commits into from
Feb 24, 2025
Merged

Expose blst internals #6829

merged 12 commits into from
Feb 24, 2025

Conversation

dknopik
Copy link
Member

@dknopik dknopik commented Jan 21, 2025

Add several functions to Generic* in bls, in cases where access to the underlying blst types is needed.

This will be needed in Anchor.

@dknopik dknopik marked this pull request as ready for review February 17, 2025 16:00
Sec: TSecretKey<Sig, Pub> + Clone,
{
/// Instantiates `Self` from a `point`.
/// Takes a reference, as moves might accidentally leave behind key material
Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate on this? Is it because zeroize only happens on drop, and a move is implemented as a memcpy without a drop?

Copy link
Member Author

@dknopik dknopik Feb 18, 2025

Choose a reason for hiding this comment

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

Exactly.

Cloning here ensures that we have two owned blst::min_pk::SecretKey, which will be zeroed each on drop. Moving may or may not do a memcpy (up to rustc to inline or do other optimisations), and may cause data to be left behind.

See also: https://docs.rs/zeroize/latest/zeroize/index.html#stackheap-zeroing-notes

Copy link
Member

Choose a reason for hiding this comment

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

I suspect there are other places we've run afoul of this. Good to know.

I wonder how invasive it would be to use Pin on secret keys. Might be an exploration for another day

Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

LGTM.

@michaelsproul michaelsproul added crypto An issue/PR that touches cryptography code. ready-for-merge This PR is ready to merge. labels Feb 24, 2025
mergify bot added a commit that referenced this pull request Feb 24, 2025
@michaelsproul
Copy link
Member

@mergify dequeue

Copy link

mergify bot commented Feb 24, 2025

This pull request has been removed from the queue for the following reason: pull request dequeued.

Pull request #6829 has been dequeued by a dequeue command

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

Copy link

mergify bot commented Feb 24, 2025

dequeue

✅ The pull request has been removed from the queue default

@michaelsproul
Copy link
Member

CI will fail until we merge:

@michaelsproul
Copy link
Member

@mergify requeue

Copy link

mergify bot commented Feb 24, 2025

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

mergify bot added a commit that referenced this pull request Feb 24, 2025
@mergify mergify bot merged commit 60964fc into sigp:unstable Feb 24, 2025
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto An issue/PR that touches cryptography code. ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants