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.

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

Use fixed width serde impls for keys #406

Merged
merged 4 commits into from
Jun 15, 2022
Merged

Use fixed width serde impls for keys #406

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
6842383
Use fixed width serde impls for keys
tcharding Mar 1, 2022
c28808c
Improve rustdocs for KeyPair
tcharding Mar 8, 2022
bf9f556
Add rustdocs describing fixed width serde
tcharding Mar 8, 2022
3ca7f49
Add fixed-width-serde integration tests
tcharding Mar 8, 2022
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
6 changes: 6 additions & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,12 @@ rand = "0.8"
rand_core = "0.6"
serde_test = "1.0"
bitcoin_hashes = "0.10"
bincode = "1.3.3"

# cbor does not build on WASM, we use it in a single trivial test (an example of when
# fixed-width-serde breaks down). Just run the test when on an x86_64 machine.
[target.'cfg(target_arch = "x86_64")'.dependencies]
cbor = "0.4.1"

[target.wasm32-unknown-unknown.dev-dependencies]
wasm-bindgen-test = "0.3"
Expand Down
161 changes: 128 additions & 33 deletions src/key.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,22 @@ use crate::Error::{self, InvalidPublicKey, InvalidPublicKeySum, InvalidSecretKey
use crate::ffi::{self, CPtr, impl_array_newtype};
use crate::ffi::types::c_uint;

#[cfg(feature = "serde")]
use serde::ser::SerializeTuple;

#[cfg(feature = "global-context")]
use crate::{Message, ecdsa, SECP256K1};
#[cfg(all(feature = "global-context", feature = "rand-std"))]
use crate::schnorr;

/// Secret 256-bit key used as `x` in an ECDSA signature.
///
/// # Serde support
///
/// Implements de/serialization with the `serde` feature enabled. We treat the byte value as a tuple
/// of 32 `u8`s for non-human-readable formats. This representation is optimal for for some formats
/// (e.g. [`bincode`]) however other formats may be less optimal (e.g. [`cbor`]).
///
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it'd be very useful to explicitly say we treat it as tuple of 32 u8s for non-human formats. I think it's a good idea to mention serialization formats but I would word it like this:

"This representation is optimal for e.g. bincode format. Some formats may be less optimal."

Copy link
Member Author

Choose a reason for hiding this comment

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

While implementing this suggestion I realized that we do not differentiate between human-readable and non-humand-readable in secp as we do in bitcoin. Is this by design or an omission?

Copy link
Member Author

Choose a reason for hiding this comment

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

This also means that the docs are now slightly misleading because they imply that we do something different for human-readable formats.

Implements de/serialization with the serde feature enabled. We treat the byte value as a tuple of 33 u8s for non-human-readable formats. This representation is optimal for for some formats (e.g. bincode) however other formats may be less optimal (e.g. cbor).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Damn, I realized it may sound like we don't implement human-readable format. So should say we do it as hex.

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 documented the hex string for human-readable formats.

/// # Examples
///
/// Basic usage:
Expand All @@ -44,6 +53,8 @@ use crate::schnorr;
/// let secret_key = SecretKey::new(&mut rand::thread_rng());
/// # }
/// ```
/// [`bincode`]: https://docs.rs/bincode
/// [`cbor`]: https://docs.rs/cbor
pub struct SecretKey([u8; constants::SECRET_KEY_SIZE]);
impl_array_newtype!(SecretKey, u8, constants::SECRET_KEY_SIZE);
impl_display_secret!(SecretKey);
Expand All @@ -67,6 +78,12 @@ pub const ONE_KEY: SecretKey = SecretKey([0, 0, 0, 0, 0, 0, 0, 0,

/// A Secp256k1 public key, used for verification of signatures.
///
/// # Serde support
///
/// Implements de/serialization with the `serde` feature enabled. We treat the byte value as a tuple
/// of 33 `u8`s for non-human-readable formats. This representation is optimal for for some formats
/// (e.g. [`bincode`]) however other formats may be less optimal (e.g. [`cbor`]).
///
/// # Examples
///
/// Basic usage:
Expand All @@ -80,6 +97,8 @@ pub const ONE_KEY: SecretKey = SecretKey([0, 0, 0, 0, 0, 0, 0, 0,
/// let public_key = PublicKey::from_secret_key(&secp, &secret_key);
/// # }
/// ```
/// [`bincode`]: https://docs.rs/bincode
/// [`cbor`]: https://docs.rs/cbor
#[derive(Copy, Clone, PartialEq, Eq, Debug, Hash)]
#[repr(transparent)]
pub struct PublicKey(ffi::PublicKey);
Expand Down Expand Up @@ -322,7 +341,11 @@ impl serde::Serialize for SecretKey {
let mut buf = [0u8; constants::SECRET_KEY_SIZE * 2];
s.serialize_str(crate::to_hex(&self.0, &mut buf).expect("fixed-size hex serialization"))
} else {
s.serialize_bytes(&self[..])
let mut tuple = s.serialize_tuple(constants::SECRET_KEY_SIZE)?;
for byte in self.0.iter() {
tuple.serialize_element(byte)?;
}
tuple.end()
}
}
}
Expand All @@ -336,10 +359,11 @@ impl<'de> serde::Deserialize<'de> for SecretKey {
"a hex string representing 32 byte SecretKey"
))
} else {
d.deserialize_bytes(super::serde_util::BytesVisitor::new(
let visitor = super::serde_util::Tuple32Visitor::new(
"raw 32 bytes SecretKey",
SecretKey::from_slice
))
);
d.deserialize_tuple(constants::SECRET_KEY_SIZE, visitor)
}
}
}
Expand Down Expand Up @@ -664,7 +688,11 @@ impl serde::Serialize for PublicKey {
if s.is_human_readable() {
s.collect_str(self)
} else {
s.serialize_bytes(&self.serialize())
let mut tuple = s.serialize_tuple(constants::PUBLIC_KEY_SIZE)?;
for byte in self.serialize().iter() { // Serialize in compressed form.
tuple.serialize_element(&byte)?;
}
tuple.end()
}
}
}
Expand All @@ -678,10 +706,11 @@ impl<'de> serde::Deserialize<'de> for PublicKey {
"an ASCII hex string representing a public key"
))
} else {
d.deserialize_bytes(super::serde_util::BytesVisitor::new(
"a bytestring representing a public key",
let visitor = super::serde_util::Tuple33Visitor::new(
"33 bytes compressed public key",
PublicKey::from_slice
))
);
d.deserialize_tuple(constants::PUBLIC_KEY_SIZE, visitor)
}
}
}
Expand All @@ -702,13 +731,10 @@ impl Ord for PublicKey {
///
/// # Serde support
///
/// [`Serialize`] and [`Deserialize`] are not implemented for this type, even with the `serde`
/// feature active. This is due to security considerations, see the [`serde_keypair`] documentation
/// for details.
///
/// If the `serde` and `global-context` features are active `KeyPair`s can be serialized and
/// deserialized by annotating them with `#[serde(with = "secp256k1::serde_keypair")]`
/// inside structs or enums for which [`Serialize`] and [`Deserialize`] are being derived.
/// Implements de/serialization with the `serde` and_`global-context` features enabled. Serializes
/// the secret bytes only. We treat the byte value as a tuple of 32 `u8`s for non-human-readable
/// formats. This representation is optimal for for some formats (e.g. [`bincode`]) however other
/// formats may be less optimal (e.g. [`cbor`]). For human-readable formats we use a hex string.
///
/// # Examples
///
Expand All @@ -723,8 +749,8 @@ impl Ord for PublicKey {
/// let key_pair = KeyPair::from_secret_key(&secp, &secret_key);
/// # }
/// ```
/// [`Deserialize`]: serde::Deserialize
/// [`Serialize`]: serde::Serialize
/// [`bincode`]: https://docs.rs/bincode
/// [`cbor`]: https://docs.rs/cbor
#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub struct KeyPair(ffi::KeyPair);
impl_display_secret!(KeyPair);
Expand Down Expand Up @@ -987,7 +1013,11 @@ impl serde::Serialize for KeyPair {
s.serialize_str(crate::to_hex(&self.secret_bytes(), &mut buf)
.expect("fixed-size hex serialization"))
} else {
s.serialize_bytes(&self.0[..])
let mut tuple = s.serialize_tuple(constants::SECRET_KEY_SIZE)?;
for byte in self.secret_bytes().iter() {
tuple.serialize_element(&byte)?;
}
tuple.end()
}
}
}
Expand All @@ -1001,19 +1031,26 @@ impl<'de> serde::Deserialize<'de> for KeyPair {
"a hex string representing 32 byte KeyPair"
))
} else {
d.deserialize_bytes(super::serde_util::BytesVisitor::new(
let visitor = super::serde_util::Tuple32Visitor::new(
"raw 32 bytes KeyPair",
|data| unsafe {
let ctx = Secp256k1::from_raw_all(ffi::secp256k1_context_no_precomp as *mut ffi::Context);
KeyPair::from_seckey_slice(&ctx, data)
}
))
);
d.deserialize_tuple(constants::SECRET_KEY_SIZE, visitor)
}
}
}

/// An x-only public key, used for verification of Schnorr signatures and serialized according to BIP-340.
///
/// # Serde support
///
/// Implements de/serialization with the `serde` feature enabled. We treat the byte value as a tuple
/// of 32 `u8`s for non-human-readable formats. This representation is optimal for for some formats
/// (e.g. [`bincode`]) however other formats may be less optimal (e.g. [`cbor`]).
///
/// # Examples
///
/// Basic usage:
Expand All @@ -1027,6 +1064,8 @@ impl<'de> serde::Deserialize<'de> for KeyPair {
/// let xonly = XOnlyPublicKey::from_keypair(&key_pair);
/// # }
/// ```
/// [`bincode`]: https://docs.rs/bincode
/// [`cbor`]: https://docs.rs/cbor
#[derive(Copy, Clone, PartialEq, Eq, Debug, PartialOrd, Ord, Hash)]
pub struct XOnlyPublicKey(ffi::XOnlyPublicKey);

Expand Down Expand Up @@ -1438,7 +1477,11 @@ impl serde::Serialize for XOnlyPublicKey {
if s.is_human_readable() {
s.collect_str(self)
} else {
s.serialize_bytes(&self.serialize())
let mut tuple = s.serialize_tuple(constants::SCHNORR_PUBLIC_KEY_SIZE)?;
for byte in self.serialize().iter() {
tuple.serialize_element(&byte)?;
}
tuple.end()
}
}
}
Expand All @@ -1452,10 +1495,11 @@ impl<'de> serde::Deserialize<'de> for XOnlyPublicKey {
"a hex string representing 32 byte schnorr public key"
))
} else {
d.deserialize_bytes(super::serde_util::BytesVisitor::new(
let visitor = super::serde_util::Tuple32Visitor::new(
"raw 32 bytes schnorr public key",
XOnlyPublicKey::from_slice
))
);
d.deserialize_tuple(constants::SCHNORR_PUBLIC_KEY_SIZE, visitor)
}
}
}
Expand Down Expand Up @@ -1501,6 +1545,8 @@ pub mod serde_keypair {
#[cfg(test)]
#[allow(unused_imports)]
mod test {
use super::*;

use core::str::FromStr;

#[cfg(any(feature = "alloc", feature = "std"))]
Expand Down Expand Up @@ -1953,6 +1999,7 @@ mod test {
static SK_STR: &'static str = "\
01010101010101010001020304050607ffff0000ffff00006363636363636363\
";
#[cfg(fuzzing)]
static PK_BYTES: [u8; 33] = [
0x02,
0x18, 0x84, 0x57, 0x81, 0xf6, 0x31, 0xc4, 0x8f,
Expand All @@ -1975,22 +2022,32 @@ mod test {
#[cfg(fuzzing)]
let pk = PublicKey::from_slice(&PK_BYTES).expect("pk");

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.compact(), &[
Token::Tuple{ len: 32 },
Token::U8(1), Token::U8(1), Token::U8(1), Token::U8(1), Token::U8(1), Token::U8(1), Token::U8(1), Token::U8(1),
Token::U8(0), Token::U8(1), Token::U8(2), Token::U8(3), Token::U8(4), Token::U8(5), Token::U8(6), Token::U8(7),
Token::U8(0xff), Token::U8(0xff), Token::U8(0), Token::U8(0), Token::U8(0xff), Token::U8(0xff), Token::U8(0), Token::U8(0),
Token::U8(99), Token::U8(99), Token::U8(99), Token::U8(99), Token::U8(99), Token::U8(99), Token::U8(99), Token::U8(99),
Token::TupleEnd
]);

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.compact(), &[
Token::Tuple{ len: 33 },
Token::U8(0x02),
Token::U8(0x18), Token::U8(0x84), Token::U8(0x57), Token::U8(0x81), Token::U8(0xf6), Token::U8(0x31), Token::U8(0xc4), Token::U8(0x8f),
Token::U8(0x1c), Token::U8(0x97), Token::U8(0x09), Token::U8(0xe2), Token::U8(0x30), Token::U8(0x92), Token::U8(0x06), Token::U8(0x7d),
Token::U8(0x06), Token::U8(0x83), Token::U8(0x7f), Token::U8(0x30), Token::U8(0xaa), Token::U8(0x0c), Token::U8(0xd0), Token::U8(0x54),
Token::U8(0x4a), Token::U8(0xc8), Token::U8(0x87), Token::U8(0xfe), Token::U8(0x91), Token::U8(0xdd), Token::U8(0xd1), Token::U8(0x66),
Token::TupleEnd
]);

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

}

#[test]
Expand Down Expand Up @@ -2071,10 +2128,14 @@ mod test {
";

let sk = KeyPairWrapper(KeyPair::from_seckey_slice(&crate::SECP256K1, &SK_BYTES).unwrap());

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.compact(), &[
Token::Tuple{ len: 32 },
Token::U8(1), Token::U8(1), Token::U8(1), Token::U8(1), Token::U8(1), Token::U8(1), Token::U8(1), Token::U8(1),
Token::U8(0), Token::U8(1), Token::U8(2), Token::U8(3), Token::U8(4), Token::U8(5), Token::U8(6), Token::U8(7),
Token::U8(0xff), Token::U8(0xff), Token::U8(0), Token::U8(0), Token::U8(0xff), Token::U8(0xff), Token::U8(0), Token::U8(0),
Token::U8(99), Token::U8(99), Token::U8(99), Token::U8(99), Token::U8(99), Token::U8(99), Token::U8(99), Token::U8(99),
Token::TupleEnd
]);

assert_tokens(&sk.readable(), &[Token::BorrowedStr(SK_STR)]);
assert_tokens(&sk.readable(), &[Token::Str(SK_STR)]);
Expand Down Expand Up @@ -2226,4 +2287,38 @@ mod test {

assert_eq!(got, want)
}

#[test]
#[cfg(not(fuzzing))]
#[cfg(all(feature = "global-context", feature = "serde"))]
fn test_serde_x_only_pubkey() {
use serde_test::{Configure, Token, assert_tokens};

static SK_BYTES: [u8; 32] = [
1, 1, 1, 1, 1, 1, 1, 1,
0, 1, 2, 3, 4, 5, 6, 7,
0xff, 0xff, 0, 0, 0xff, 0xff, 0, 0,
99, 99, 99, 99, 99, 99, 99, 99
];

static PK_STR: &'static str = "\
18845781f631c48f1c9709e23092067d06837f30aa0cd0544ac887fe91ddd166\
";

let kp = KeyPair::from_seckey_slice(&crate::SECP256K1, &SK_BYTES).unwrap();
let (pk, _parity) = XOnlyPublicKey::from_keypair(&kp);

assert_tokens(&pk.compact(), &[
Token::Tuple{ len: 32 },
Token::U8(0x18), Token::U8(0x84), Token::U8(0x57), Token::U8(0x81), Token::U8(0xf6), Token::U8(0x31), Token::U8(0xc4), Token::U8(0x8f),
Token::U8(0x1c), Token::U8(0x97), Token::U8(0x09), Token::U8(0xe2), Token::U8(0x30), Token::U8(0x92), Token::U8(0x06), Token::U8(0x7d),
Token::U8(0x06), Token::U8(0x83), Token::U8(0x7f), Token::U8(0x30), Token::U8(0xaa), Token::U8(0x0c), Token::U8(0xd0), Token::U8(0x54),
Token::U8(0x4a), Token::U8(0xc8), Token::U8(0x87), Token::U8(0xfe), Token::U8(0x91), Token::U8(0xdd), Token::U8(0xd1), Token::U8(0x66),
Token::TupleEnd
]);

assert_tokens(&pk.readable(), &[Token::BorrowedStr(PK_STR)]);
assert_tokens(&pk.readable(), &[Token::Str(PK_STR)]);
assert_tokens(&pk.readable(), &[Token::String(PK_STR)]);
}
}
11 changes: 8 additions & 3 deletions src/schnorr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -569,9 +569,14 @@ mod tests {
assert_tokens(&sig.readable(), &[Token::Str(SIG_STR)]);
assert_tokens(&sig.readable(), &[Token::String(SIG_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.compact(), &[
Token::Tuple{ len: 32 },
Token::U8(24), Token::U8(132), Token::U8(87), Token::U8(129), Token::U8(246), Token::U8(49), Token::U8(196), Token::U8(143),
Token::U8(28), Token::U8(151), Token::U8(9), Token::U8(226), Token::U8(48), Token::U8(146), Token::U8(6), Token::U8(125),
Token::U8(6), Token::U8(131), Token::U8(127), Token::U8(48), Token::U8(170), Token::U8(12), Token::U8(208), Token::U8(84),
Token::U8(74), Token::U8(200), Token::U8(135), Token::U8(254), Token::U8(145), Token::U8(221), Token::U8(209), Token::U8(102),
Token::TupleEnd
]);

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