Skip to content

Conversation

@tarcieri
Copy link
Member

@tarcieri tarcieri commented Oct 9, 2021

Adds a Drop handler which calls zeroize() on the NonZeroScalar which the SigningKey newtype wraps.

Adds a `Drop` handler which calls `zeroize()` on the `NonZeroScalar`
which the `SigningKey` newtype wraps.
@codecov-commenter
Copy link

Codecov Report

Merging #449 (0ceaf97) into master (daa50d5) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #449      +/-   ##
==========================================
- Coverage   65.03%   65.00%   -0.04%     
==========================================
  Files          28       28              
  Lines        3598     3600       +2     
==========================================
  Hits         2340     2340              
- Misses       1258     1260       +2     
Impacted Files Coverage Δ
k256/src/ecdsa/sign.rs 18.07% <0.00%> (-0.45%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update daa50d5...0ceaf97. Read the comment docs.

@tarcieri tarcieri merged commit c9a4d35 into master Oct 9, 2021
@tarcieri tarcieri deleted the k256/impl-drop-for-signingkey branch October 9, 2021 18:52
@ggutoski
Copy link

Yay! 🥳

Is there a reason you don't derive Zeroize for SigningKey and use #[zeroize(drop)]?

@tarcieri
Copy link
Member Author

It's to avoid people accidentally using the SigningKey after it's been zeroized.

There's a bit of a tricky tradeoff here which we're aware causes some ergonomic problems. See iqlusioninc/crates#757

It would probably be good if the custom derive support could allow skipping of individual fields which are known to be self-zeroizing. Something like #[zeroize(skip)]

This was referenced Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants