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

feat: add serde support #780

Merged
merged 1 commit into from Jul 20, 2022
Merged

feat: add serde support #780

merged 1 commit into from Jul 20, 2022

Conversation

npmccallum
Copy link
Contributor

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

@@ -17,6 +17,7 @@ keywords = ["memory", "memset", "secure", "volatile", "zero"]
edition = "2018"

[dependencies]
serde = { version = "1.0", optional = true }
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
serde = { version = "1.0", optional = true }
serde = { version = "1.0", default-features = false, optional = true }

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@tarcieri
Copy link
Member

I'm curious if there's an attribute recipe we could come up with so as to avoid directly coupling zeroize to serde.

#[serde(from = "T")] should suffice for the deserializer. Offhand I'm not sure if there's a way to get serde to leverage traits like AsRef/Borrow/Deref to obtain a reference the inner type prior invoking Serialize::serialize, however.

See: https://serde.rs/container-attrs.html

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>
@npmccallum
Copy link
Contributor Author

@tarcieri I don't think that's desirable. #[serde(from = "T")] is for containers. So you'd have to do a newtype for every use. And even if we found an indirect way to leverage serde, it would probably have non-obvious side-effects.

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.

@tarcieri
Copy link
Member

tarcieri commented Jul 19, 2022

#[serde(from = "T")] is for containers. So you'd have to do a newtype for every use.

In this application, Zeroizing is the newtype container. from = "T" would be the inner type for Zeroizing<T>. It can leverage impl From<T> for Zeroizing<T>.

It would fully work for the Deserialize use case. The blocker is Serialize. I'm not sure of a good solution there.

@daxpedda
Copy link
Contributor

@tarcieri are you saying we should use #[serde(from = "T")] on Zeroizing instead of doing a manual implementation? I don't understand how this avoids coupling zeroize to serde though.

@tarcieri
Copy link
Member

No, I was suggesting that downstream users could use container-level attributes rather than modifying zeroize:

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 Serialize.

@npmccallum
Copy link
Contributor Author

So can we merge this then?

@tarcieri tarcieri merged commit f096d5f into RustCrypto:master Jul 20, 2022
@tarcieri
Copy link
Member

tarcieri commented Jul 20, 2022

After investigating some more #[serde(from = "Bar")] doesn't work at the field-level (only the toplevel on a struct itself).

It might be possible to get it to work with remote derive and using #[serde(getter = "...")] at the field-level but it's obtuse enough I guess a first-class feature is warranted.

@tarcieri tarcieri mentioned this pull request Jul 20, 2022
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.

None yet

3 participants