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
feat: add serde support #780
Conversation
zeroize/Cargo.toml
Outdated
@@ -17,6 +17,7 @@ keywords = ["memory", "memset", "secure", "volatile", "zero"] | |||
edition = "2018" | |||
|
|||
[dependencies] | |||
serde = { version = "1.0", optional = true } |
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.
serde = { version = "1.0", optional = true } | |
serde = { version = "1.0", default-features = false, optional = true } |
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.
@daxpedda isn't that flag best used by binaries rather than libraries? I don't care either way.
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.
We generally try to enable the minimum number of features needed for a particular purpose.
Using default-features = false
makes sense for both binaries and libraries, and is a requirement to use a prospective serde
feature in no_std
environments. See: https://serde.rs/no-std.html
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.
Done.
I'm curious if there's an attribute recipe we could come up with so as to avoid directly coupling
|
This (optional) feature implements Serialize/Deserialize for Zeroizing when the inner type implements these traits. This makes it possible to use Zeroizing in serde contexts. Signed-off-by: Nathaniel McCallum <nathaniel@profian.com>
@tarcieri I don't think that's desirable. An optional coupling between zeroize and serde is zero cost for everyone not using the feature. It is obvious and well-documented with no side-effects. That seems to me the best approach. |
In this application, It would fully work for the |
@tarcieri are you saying we should use |
No, I was suggesting that downstream users could use container-level attributes rather than modifying use serde::Deserialize;
use zeroize::Zeroizing;
#[derive(Deserialize)]
pub struct Foo {
#[serde(from = "Bar")]
pub bar: Zeroizing<Bar>
} Unfortunately I don't see a similar solution for |
So can we merge this then? |
After investigating some more It might be possible to get it to work with remote derive and using |
This (optional) feature implements Serialize/Deserialize for Zeroizing when
the inner type implements these traits. This makes it possible to use
Zeroizing in serde contexts.
Signed-off-by: Nathaniel McCallum nathaniel@profian.com