Skip to content

[Alternative] Allow deserializing from owned types #218

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

[Alternative] Allow deserializing from owned types #218

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1961af9
Fix SecretKey's serde impl to support owned data
elichai May 15, 2020
53c8ded
Fix Signature's serde impl to support owned data
elichai May 15, 2020
88bf5b3
Add more serde tests for owned data buffers
elichai May 15, 2020
7289fac
Deduplicate Visitor code
sgeisler May 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/ecdh.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,6 @@ mod tests {
use rand::thread_rng;
use super::SharedSecret;
use super::super::Secp256k1;
use Error;

#[test]
fn ecdh() {
Expand All @@ -187,7 +186,7 @@ mod tests {
let s = Secp256k1::signing_only();
let (sk1, pk1) = s.generate_keypair(&mut thread_rng());
let (sk2, pk2) = s.generate_keypair(&mut thread_rng());

let sec1 = SharedSecret::new_with_hash(&pk1, &sk2, |x,_| x.into());
let sec2 = SharedSecret::new_with_hash(&pk2, &sk1, |x,_| x.into());
let sec_odd = SharedSecret::new_with_hash(&pk1, &sk1, |x,_| x.into());
Expand Down
94 changes: 47 additions & 47 deletions src/key.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,34 @@ impl SecretKey {
}
}

serde_impl!(SecretKey, constants::SECRET_KEY_SIZE);

#[cfg(feature = "serde")]
impl ::serde::Serialize for SecretKey {
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this is encouraging user to serialize their secrets. Do we really want to be doing that?

Copy link
Contributor

@thomaseizinger thomaseizinger Jan 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

As an alternative, we could provide a module so users have to explicitly opt-in to this by saying #[serde(with = "secp256k1::serde::secret_key")]. This has the downside that we need to provide a module for Option and Vec as well if we want it to work out of the box with those containers.

However, I am not sure if this makes things actually better? Some situations require secret keys to be serialized (a backup functionality for example). Additionally, SecretKey also implements Debug and Display which are basically the same.

I am not convinced that we should be introducing a policy on this abstraction level. Whether or not serializing a secret key is dangerous depends completely on the application.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll just note that this is already the case: https://docs.rs/secp256k1/0.20.0/secp256k1/key/struct.SecretKey.html#impl-Serialize

Copy link
Collaborator

Choose a reason for hiding this comment

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

(At least as long as we don't have HD derivation natively in this library), serializing secret keys is essential. Keys need to be stored.

In one case, the caller has the secret key in serialized form already and just passes it to the library. Then letting the user serialize keys won't hurt either.

In the other case, the caller relies on the library to generate (or tweak) keys. Then it's essential to serialize them for storage.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, concept ACK having serialization. You need to store secret key material somehow.

This PR needs a rebase though.

fn serialize<S: ::serde::Serializer>(&self, s: S) -> Result<S::Ok, S::Error> {
if s.is_human_readable() {
s.collect_str(self)
} else {
s.serialize_bytes(&self[..])
}
}
}

#[cfg(feature = "serde")]
impl<'de> ::serde::Deserialize<'de> for SecretKey {
fn deserialize<D: ::serde::Deserializer<'de>>(d: D) -> Result<Self, D::Error> {
if d.is_human_readable() {
d.deserialize_str(super::serde_util::HexVisitor::new(
"a hex string representing 32 byte SecretKey"
))
} else {
d.deserialize_bytes(super::serde_util::BytesVisitor::new(
"raw 32 bytes SecretKey",
SecretKey::from_slice
))
}
}
}


impl PublicKey {
/// Obtains a raw const pointer suitable for use with FFI functions
Expand Down Expand Up @@ -392,53 +419,14 @@ impl ::serde::Serialize for PublicKey {
impl<'de> ::serde::Deserialize<'de> for PublicKey {
fn deserialize<D: ::serde::Deserializer<'de>>(d: D) -> Result<PublicKey, D::Error> {
if d.is_human_readable() {
struct HexVisitor;

impl<'de> ::serde::de::Visitor<'de> for HexVisitor {
type Value = PublicKey;

fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
formatter.write_str("an ASCII hex string")
}

fn visit_bytes<E>(self, v: &[u8]) -> Result<Self::Value, E>
where
E: ::serde::de::Error,
{
if let Ok(hex) = str::from_utf8(v) {
str::FromStr::from_str(hex).map_err(E::custom)
} else {
Err(E::invalid_value(::serde::de::Unexpected::Bytes(v), &self))
}
}

fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
where
E: ::serde::de::Error,
{
str::FromStr::from_str(v).map_err(E::custom)
}
}
d.deserialize_str(HexVisitor)
d.deserialize_str(super::serde_util::HexVisitor::new(
"an ASCII hex string representing a public key"
))
} else {
struct BytesVisitor;

impl<'de> ::serde::de::Visitor<'de> for BytesVisitor {
type Value = PublicKey;

fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
formatter.write_str("a bytestring")
}

fn visit_bytes<E>(self, v: &[u8]) -> Result<Self::Value, E>
where
E: ::serde::de::Error,
{
PublicKey::from_slice(v).map_err(E::custom)
}
}

d.deserialize_bytes(BytesVisitor)
d.deserialize_bytes(super::serde_util::BytesVisitor::new(
"a bytestring representing a public key",
PublicKey::from_slice
))
}
}
}
Expand Down Expand Up @@ -848,8 +836,20 @@ mod test {
let pk = PublicKey::from_secret_key(&s, &sk);

assert_tokens(&sk.compact(), &[Token::BorrowedBytes(&SK_BYTES[..])]);
assert_tokens(&sk.compact(), &[Token::Bytes(&SK_BYTES)]);
assert_tokens(&sk.compact(), &[Token::ByteBuf(&SK_BYTES)]);

assert_tokens(&sk.readable(), &[Token::BorrowedStr(SK_STR)]);
assert_tokens(&sk.readable(), &[Token::Str(SK_STR)]);
assert_tokens(&sk.readable(), &[Token::String(SK_STR)]);

assert_tokens(&pk.compact(), &[Token::BorrowedBytes(&PK_BYTES[..])]);
assert_tokens(&pk.compact(), &[Token::Bytes(&PK_BYTES)]);
assert_tokens(&pk.compact(), &[Token::ByteBuf(&PK_BYTES)]);

assert_tokens(&pk.readable(), &[Token::BorrowedStr(PK_STR)]);
assert_tokens(&pk.readable(), &[Token::Str(PK_STR)]);
assert_tokens(&pk.readable(), &[Token::String(PK_STR)]);

}
}
23 changes: 16 additions & 7 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,8 @@ pub mod ecdh;
pub mod key;
#[cfg(feature = "recovery")]
pub mod recovery;
#[cfg(feature = "serde")]
mod serde_util;

pub use key::SecretKey;
pub use key::PublicKey;
Expand Down Expand Up @@ -432,15 +434,16 @@ impl ::serde::Serialize for Signature {

#[cfg(feature = "serde")]
impl<'de> ::serde::Deserialize<'de> for Signature {
fn deserialize<D: ::serde::Deserializer<'de>>(d: D) -> Result<Signature, D::Error> {
use ::serde::de::Error;
use str::FromStr;
fn deserialize<D: ::serde::Deserializer<'de>>(d: D) -> Result<Self, D::Error> {
if d.is_human_readable() {
let sl: &str = ::serde::Deserialize::deserialize(d)?;
Signature::from_str(sl).map_err(D::Error::custom)
d.deserialize_str(serde_util::HexVisitor::new(
"a hex string representing a DER encoded Signature"
))
} else {
let sl: &[u8] = ::serde::Deserialize::deserialize(d)?;
Signature::from_der(sl).map_err(D::Error::custom)
d.deserialize_bytes(serde_util::BytesVisitor::new(
"raw byte stream, that represents a DER encoded Signature",
Signature::from_der
))
}
}
}
Expand Down Expand Up @@ -1081,7 +1084,13 @@ mod tests {
";

assert_tokens(&sig.compact(), &[Token::BorrowedBytes(&SIG_BYTES[..])]);
assert_tokens(&sig.compact(), &[Token::Bytes(&SIG_BYTES)]);
assert_tokens(&sig.compact(), &[Token::ByteBuf(&SIG_BYTES)]);

assert_tokens(&sig.readable(), &[Token::BorrowedStr(SIG_STR)]);
assert_tokens(&sig.readable(), &[Token::Str(SIG_STR)]);
assert_tokens(&sig.readable(), &[Token::String(SIG_STR)]);

}

// For WASM, just run through our general tests in this file all at once.
Expand Down
44 changes: 0 additions & 44 deletions src/macros.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,47 +43,3 @@ macro_rules! impl_from_array_len {
)+
}
}

#[cfg(feature="serde")]
/// Implements `Serialize` and `Deserialize` for a type `$t` which represents
/// a newtype over a byte-slice over length `$len`. Type `$t` must implement
/// the `FromStr` and `Display` trait.
macro_rules! serde_impl(
($t:ident, $len:expr) => (
impl ::serde::Serialize for $t {
fn serialize<S: ::serde::Serializer>(&self, s: S) -> Result<S::Ok, S::Error> {
if s.is_human_readable() {
s.collect_str(self)
} else {
s.serialize_bytes(&self[..])
}
}
}

impl<'de> ::serde::Deserialize<'de> for $t {
fn deserialize<D: ::serde::Deserializer<'de>>(d: D) -> Result<$t, D::Error> {
use ::serde::de::Error;
use core::str::FromStr;

if d.is_human_readable() {
let sl: &str = ::serde::Deserialize::deserialize(d)?;
SecretKey::from_str(sl).map_err(D::Error::custom)
} else {
let sl: &[u8] = ::serde::Deserialize::deserialize(d)?;
if sl.len() != $len {
Err(D::Error::invalid_length(sl.len(), &stringify!($len)))
} else {
let mut ret = [0; $len];
ret.copy_from_slice(sl);
Ok($t(ret))
}
}
}
}
)
);

#[cfg(not(feature="serde"))]
macro_rules! serde_impl(
($t:ident, $len:expr) => ()
);
76 changes: 76 additions & 0 deletions src/serde_util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
use core::fmt;
use core::marker::PhantomData;
use core::str::{self, FromStr};
use serde::de;

pub struct HexVisitor<T> {
expectation: &'static str,
_pd: PhantomData<T>,
}

impl<T> HexVisitor<T> {
pub fn new(expectation: &'static str) -> Self {
HexVisitor {
expectation,
_pd: PhantomData,
}
}
}

impl<'de, T> de::Visitor<'de> for HexVisitor<T>
where
T: FromStr,
<T as FromStr>::Err: fmt::Display,
{
type Value = T;

fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
formatter.write_str(self.expectation)
}

fn visit_bytes<E: de::Error>(self, v: &[u8]) -> Result<Self::Value, E> {
if let Ok(hex) = str::from_utf8(v) {
FromStr::from_str(hex).map_err(E::custom)
} else {
Err(E::invalid_value(de::Unexpected::Bytes(v), &self))
}
}

fn visit_str<E: de::Error>(self, v: &str) -> Result<Self::Value, E> {
FromStr::from_str(v).map_err(E::custom)
}
}

pub struct BytesVisitor<F> {
expectation: &'static str,
parse_fn: F,
}

impl<F, T, Err> BytesVisitor<F>
where
F: FnOnce(&[u8]) -> Result<T, Err>,
Err: fmt::Display,
{
pub fn new(expectation: &'static str, parse_fn: F) -> Self {
BytesVisitor {
expectation,
parse_fn,
}
}
}

impl<'de, F, T, Err> de::Visitor<'de> for BytesVisitor<F>
where
F: FnOnce(&[u8]) -> Result<T, Err>,
Err: fmt::Display,
{
type Value = T;

fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
formatter.write_str(self.expectation)
}

fn visit_bytes<E: de::Error>(self, v: &[u8]) -> Result<Self::Value, E> {
(self.parse_fn)(v).map_err(E::custom)
}
}