-
Notifications
You must be signed in to change notification settings - Fork 560
Optionally implement zerocopy traits on relevant curve25519_dalek and x25519_dalek types #825
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
base: main
Are you sure you want to change the base?
Conversation
Fixes dalek-cryptography#821 Signed-off-by: David Anderson <[email protected]>
For dalek-cryptography#821 Signed-off-by: David Anderson <[email protected]>
Signed-off-by: David Anderson <[email protected]>
#[cfg_attr( | ||
feature = "zerocopy", | ||
derive( | ||
zerocopy::FromBytes, | ||
zerocopy::IntoBytes, | ||
zerocopy::Immutable, | ||
zerocopy::KnownLayout | ||
) | ||
)] | ||
#[derive(Clone)] | ||
pub struct StaticSecret([u8; 32]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Impls on the public key types are fine, but impls on secrets are slightly worrisome.
One scenario is someone may unintentionally pass the wrong type and could accidentally expose key material because both the public and secret types impl the same trait.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same concern applies to the serde traits, no? By definition, StaticSecret is transformable into a form that can be stored, that's why it exists as a separate (riskier) type than ReusableSecret. You could also as_bytes() the key and pass it to anything that handles bytes and the type system won't stop you.
I can see an argument for only allowing serialization via explicit, non-trait methods in order to make it harder to accidentally trigger serialization, but in that case the same argument should apply to the serde derivations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, FWIW the @RustCrypto crates don't implement the serde
traits on secrets, though I guess that happened in curve25519-dalek
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my purposes, zerocopy is most useful for serializing data into network packets, and indeed in that case it would be quite bad to serialize a cleartext secret key :)
I'm okay with either outcome, either consistency with the serde traits or treating the serde traits as a historical mistake (I assume we can't remove them without a breaking semver release). In the latter case I can remove the derivations from StaticSecret.
... although tbh that means this entire change, with a new cargo feature and all the plumbing, is effectively to support zerocopy on a single type (x25519 PublicKey), and maybe we're back to the discussion of "is this even worth it" :)
Fixes #821
Overview
The PR has 3 self-contained commits:
Testing
Tested both modified crates with:
I didn't add any new tests, since the change is merely deriving new traits that come with their own compile-time correctness checking. If you like, I could add a test that does nothing more than verify that the trait methods are available when the feature's enabled, to avoid a regression where the derives are mistakenly removed?