Skip to content

x25519: use manual impls instead of zeroize_derive - #776

Merged
rozbb merged 1 commit into
mainfrom
x25519-dalek/remove-zeroize-derive
Jul 7, 2025
Merged

x25519: use manual impls instead of zeroize_derive#776
rozbb merged 1 commit into
mainfrom
x25519-dalek/remove-zeroize-derive

Conversation

@tarcieri

@tarcieri tarcieri commented Jul 7, 2025

Copy link
Copy Markdown
Contributor

The types involved are all simple 1-tuple newtypes where zeroization only involves calling zeroize on the inner type, making all of the involved impls relatively trivial.

Avoiding custom derive arguably improves auditability as you don't need to expand a proc macro to see the resulting code. It decreases the number of required dependencies in order for the zeroize feature to work, where some of those dependencies are incredibly heavy (particularly syn).

The types involved are all simple 1-tuple newtypes where zeroization
only involves calling `zeroize` on the inner type, making all of the
involved impls relatively trivial.

Avoiding custom derive arguably improves auditability as you don't need
to expand a proc macro to see the resulting code. It decreases the
number of required dependencies in order for the `zeroize` feature to
work, where some of those dependencies are incredibly heavy
(particularly `syn`).
Comment on lines +119 to +124
impl Drop for EphemeralSecret {
fn drop(&mut self) {
#[cfg(feature = "zeroize")]
self.zeroize();
}
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The reason for gating the inner zeroize invocation on the feature as opposed to the whole Drop impl is so enabling the zeroize feature doesn't change the Drop bounds / presence of drop glue. There are some weird edge cases around this approach avoids.

@tarcieri
tarcieri requested a review from rozbb July 7, 2025 13:32
@tarcieri

tarcieri commented Jul 7, 2025

Copy link
Copy Markdown
Contributor Author

Note: this is largely a workaround for the x25519 build failure in #775, but I think the case for improved auditability and fewer dependencies is also valid.

I think this upstream change should take care of the problem going forward, as well: RustCrypto/utils#1190

@rozbb rozbb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM thanks!

@rozbb
rozbb merged commit 37d3ae9 into main Jul 7, 2025
@rozbb
rozbb deleted the x25519-dalek/remove-zeroize-derive branch July 3, 2026 18:12
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.

2 participants