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

Add ZeroizeOnDrop to ed25519 keys #1956

Merged
merged 4 commits into from
Oct 24, 2023
Merged

Conversation

sug0
Copy link
Contributor

@sug0 sug0 commented Sep 28, 2023

Describe your changes

  • Tag ed25519 keys with ZeroizeOnDrop, to indicate their bytes are cleared uppon the key being dropped.
  • Switch to k256 for secp256k1 keys. It natively implements Zeroize - done in base deps: switch from libsecp256k1 to k256 #1958

Indicate on which release or other PRs this topic is based on

#1958 (first commit here 609e70d)

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

sug0 added a commit that referenced this pull request Sep 28, 2023
@sug0 sug0 marked this pull request as ready for review September 28, 2023 18:42
@brentstone
Copy link
Collaborator

DId these keys not already have this behavior? See the tests types::key::more_tests::zeroize_keypair_ed25519 and types::key::more_tests_zeroize_keypair_secp256k1?

@sug0
Copy link
Contributor Author

sug0 commented Sep 28, 2023

oh @brentstone you're right. I didn't realize we were using a fork of libsecp that cleared these keys on drop. to be honest though, we should switch back to the upstream version of the crate. we can clear the sk in a way similar to what has been implemented in this pr (albeit it involves unsafe code). lmk what you think

@brentstone
Copy link
Collaborator

@sug0 yeah you're prob right that we should avoid using the fork if we can accomplish the same thing with your method. I defer to your judgment.

@tzemanovic
Copy link
Member

looking back at this I think we should switch to use https://crates.io/crates/k256

@sug0 sug0 changed the title Zeroize secret keys on drop Switch to k256 as the provider of secp256k1 key signatures Sep 29, 2023
@sug0 sug0 marked this pull request as draft September 29, 2023 09:23
tzemanovic pushed a commit that referenced this pull request Sep 29, 2023
@tzemanovic tzemanovic marked this pull request as ready for review September 29, 2023 13:21
@tzemanovic tzemanovic changed the title Switch to k256 as the provider of secp256k1 key signatures Add ZeroizeOnDrop to secp256k1 and ed25519 and Zeroize to secp Sep 29, 2023
@tzemanovic tzemanovic marked this pull request as draft September 29, 2023 13:28
tzemanovic pushed a commit that referenced this pull request Sep 29, 2023
tzemanovic pushed a commit that referenced this pull request Sep 29, 2023
tzemanovic pushed a commit that referenced this pull request Sep 29, 2023
@tzemanovic tzemanovic marked this pull request as ready for review September 29, 2023 13:38
@sug0 sug0 mentioned this pull request Oct 3, 2023
2 tasks
@sug0 sug0 changed the title Add ZeroizeOnDrop to secp256k1 and ed25519 and Zeroize to secp Add ZeroizeOnDrop to ed25519 keys Oct 4, 2023
sug0 added a commit that referenced this pull request Oct 4, 2023
@sug0 sug0 force-pushed the tiago/zeroize-secret-keys branch from 4a706ad to b2c4919 Compare October 4, 2023 08:13
@sug0 sug0 force-pushed the tiago/zeroize-secret-keys branch from b2c4919 to 5d7c951 Compare October 4, 2023 08:56
sug0 added a commit that referenced this pull request Oct 13, 2023
@sug0 sug0 mentioned this pull request Oct 13, 2023
@cwgoes cwgoes mentioned this pull request Oct 20, 2023
29 tasks
Fraccaman added a commit that referenced this pull request Oct 23, 2023
* origin/tiago/zeroize-secret-keys:
  Changelog for #1956
  Implement ZeroizeOnDrop on ed25519 keys
@tzemanovic tzemanovic mentioned this pull request Oct 24, 2023
@tzemanovic tzemanovic merged commit 5d7c951 into main Oct 24, 2023
10 of 12 checks passed
@tzemanovic tzemanovic deleted the tiago/zeroize-secret-keys branch October 24, 2023 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants