Update Ed25519Signature ser/de logic (#756)

* Update signature serde

* fixup! Update signature serde

* fix Ed25519Signature ser/de

* add comment

Author: mwtian

Cargo.lock

@@ -16,12 +16,13 @@ hkdf = { version = "0.12.3", features = ["std"] }
 rand = { version = "0.8.5", features = ["std"] }
 rust_secp256k1 = { version = "0.24.0", package = "secp256k1", features = ["recovery", "rand-std", "bitcoin_hashes", "global-context"] }
 serde = { version = "1.0.142", features = ["derive"] }
+serde_bytes = "0.11.7"
 serde_with = "2.0.0"
 signature = "1.6.0"
 tokio = { version = "1.20.1", features = ["sync", "rt", "macros"] }
 zeroize = "1.5.7"
 
-# TODO: switch to https://github.com/celo-org/celo-bls-snark-rs 
+# TODO: switch to https://github.com/celo-org/celo-bls-snark-rs
 # when https://github.com/celo-org/celo-bls-snark-rs/issues/228 is solved
 celo-bls = { git = "https://github.com/huitseeker/celo-bls-snark-rs", branch = "updates-2", package = "bls-crypto", optional = true }
 
@@ -55,3 +56,4 @@ proptest = "1.0.0"
 proptest-derive = "0.3.0"
 serde_json = "1.0.83"
 sha3 = "0.10.2"
+serde-reflection = "0.3.6"

crypto/Cargo.toml

@@ -3,7 +3,12 @@
 use base64ct::{Base64, Encoding};
 use ed25519_consensus::{batch, VerificationKeyBytes};
 use once_cell::sync::OnceCell;
-use serde::{de, Deserialize, Serialize};
+use serde::{
+    de::{self, MapAccess, SeqAccess, Visitor},
+    ser::SerializeStruct,
+    Deserialize, Serialize,
+};
+use serde_bytes::{ByteBuf, Bytes};
 use signature::{rand_core::OsRng, Signature, Signer, Verifier};
 use std::{
     fmt::{self, Display},
@@ -23,6 +28,9 @@ pub const ED25519_PRIVATE_KEY_LENGTH: usize = 32;
 pub const ED25519_PUBLIC_KEY_LENGTH: usize = 32;
 pub const ED25519_SIGNATURE_LENGTH: usize = 64;
 
+const BASE64_FIELD_NAME: &str = "base64";
+const RAW_FIELD_NAME: &str = "raw";
+
 ///
 /// Define Structs
 ///
@@ -46,6 +54,7 @@ pub struct Ed25519KeyPair {
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub struct Ed25519Signature {
     pub sig: ed25519_consensus::Signature,
+    // Helps implementing AsRef<[u8]>.
     pub bytes: OnceCell<[u8; ED25519_SIGNATURE_LENGTH]>,
 }
 
@@ -246,16 +255,32 @@ impl Default for Ed25519Signature {
     }
 }
 
+// Notes for Serialize and Deserialize implementations of Ed25519Signature:
+// - Since `bytes` field contains serialized `sig` field, it can be used directly for ser/de of
+//   the Ed25519Signature struct.
+// - The `serialize_struct()` function and deserialization visitor add complexity, but they are necessary for
+//   Ed25519Signature ser/de to work with `serde_reflection`.
+//   `serde_reflection` works poorly [with aliases and nameless types](https://docs.rs/serde-reflection/latest/serde_reflection/index.html#unsupported-idioms).
+// - Serialization output and deserialization input support human readable (base64) and non-readable (binary) formats
+//   separately (supported for other schemes since #460). Different struct field names ("base64" vs "raw") are used
+//   to disambiguate the formats.
+// These notes may help if Ed25519Signature needs to change the struct layout, or its ser/de implementation.
 impl Serialize for Ed25519Signature {
     fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
     where
         S: serde::Serializer,
     {
-        if serializer.is_human_readable() {
-            base64ct::Base64::encode_string(self.as_ref()).serialize(serializer)
+        let readable = serializer.is_human_readable();
+        let mut state = serializer.serialize_struct("Ed25519Signature", 1)?;
+        if readable {
+            state.serialize_field(
+                BASE64_FIELD_NAME,
+                &base64ct::Base64::encode_string(self.as_ref()),
+            )?;
         } else {
-            self.as_ref().serialize(serializer)
+            state.serialize_field(RAW_FIELD_NAME, Bytes::new(self.as_ref()))?;
         }
+        state.end()
     }
 }
 
@@ -264,14 +289,73 @@ impl<'de> Deserialize<'de> for Ed25519Signature {
     where
         D: serde::Deserializer<'de>,
     {
-        let bytes = if deserializer.is_human_readable() {
-            let s = String::deserialize(deserializer)?;
-            base64ct::Base64::decode_vec(&s).map_err(|e| de::Error::custom(e.to_string()))?
+        struct Ed25519SignatureVisitor {
+            readable: bool,
+        }
+
+        impl<'de> Visitor<'de> for Ed25519SignatureVisitor {
+            type Value = Ed25519Signature;
+
+            fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
+                formatter.write_str("struct Ed25519Signature")
+            }
+
+            fn visit_seq<V>(self, mut seq: V) -> Result<Ed25519Signature, V::Error>
+            where
+                V: SeqAccess<'de>,
+            {
+                if self.readable {
+                    let s = seq
+                        .next_element::<String>()?
+                        .ok_or_else(|| de::Error::missing_field(BASE64_FIELD_NAME))?;
+                    Ed25519Signature::decode_base64(&s)
+                        .map_err(|e| de::Error::custom(e.to_string()))
+                } else {
+                    let b = seq
+                        .next_element::<ByteBuf>()?
+                        .ok_or_else(|| de::Error::missing_field(RAW_FIELD_NAME))?;
+                    <Ed25519Signature as Signature>::from_bytes(&b)
+                        .map_err(|e| de::Error::custom(e.to_string()))
+                }
+            }
+
+            fn visit_map<V>(self, mut map: V) -> Result<Ed25519Signature, V::Error>
+            where
+                V: MapAccess<'de>,
+            {
+                if self.readable {
+                    let entry = map
+                        .next_entry::<&str, String>()?
+                        .ok_or_else(|| de::Error::missing_field(BASE64_FIELD_NAME))?;
+                    if entry.0 != BASE64_FIELD_NAME {
+                        return Err(de::Error::unknown_field(entry.0, &[BASE64_FIELD_NAME]));
+                    }
+                    Ed25519Signature::decode_base64(&entry.1)
+                        .map_err(|e| de::Error::custom(e.to_string()))
+                } else {
+                    let entry = map
+                        .next_entry::<&str, &[u8]>()?
+                        .ok_or_else(|| de::Error::missing_field(RAW_FIELD_NAME))?;
+                    if entry.0 != RAW_FIELD_NAME {
+                        return Err(de::Error::unknown_field(entry.0, &[RAW_FIELD_NAME]));
+                    }
+                    <Ed25519Signature as Signature>::from_bytes(entry.1)
+                        .map_err(|e| de::Error::custom(e.to_string()))
+                }
+            }
+        }
+
+        let readable = deserializer.is_human_readable();
+        let fields: &[&str; 1] = if readable {
+            &[BASE64_FIELD_NAME]
         } else {
-            Vec::deserialize(deserializer)?
+            &[RAW_FIELD_NAME]
         };
-        <Ed25519Signature as signature::Signature>::from_bytes(&bytes)
-            .map_err(|e| de::Error::custom(e.to_string()))
+        deserializer.deserialize_struct(
+            "Ed25519Signature",
+            fields,
+            Ed25519SignatureVisitor { readable },
+        )
     }
 }
 

crypto/src/ed25519.rs

@@ -11,9 +11,10 @@ use crate::{
     hkdf::hkdf_generate_from_ikm,
     traits::{AggregateAuthenticator, EncodeDecodeBase64, KeyPair, ToFromBytes, VerifyingKey},
 };
-
 use blake2::digest::Update;
+use ed25519_consensus::VerificationKey;
 use rand::{rngs::StdRng, SeedableRng as _};
+use serde_reflection::{Samples, Tracer, TracerConfig};
 use sha3::Sha3_256;
 use signature::{Signer, Verifier};
 
@@ -51,6 +52,38 @@ fn serialize_deserialize() {
     assert!(bincode::deserialize::<Ed25519PublicKey>(&bytes).is_err());
 }
 
+#[test]
+fn custom_serde_reflection() {
+    let config = TracerConfig::default()
+        .record_samples_for_newtype_structs(true)
+        .record_samples_for_structs(true)
+        .record_samples_for_tuple_structs(true);
+    let mut tracer = Tracer::new(config);
+    let mut samples = Samples::new();
+
+    let message = b"hello, narwhal";
+    let sig = keys().pop().unwrap().sign(message);
+    tracer
+        .trace_value(&mut samples, &sig)
+        .expect("trace value Ed25519Signature");
+    assert!(samples.value("Ed25519Signature").is_some());
+    tracer
+        .trace_type::<Ed25519Signature>(&samples)
+        .expect("trace type Ed25519PublicKey");
+
+    let kpref = keys().pop().unwrap();
+    let public_key = kpref.public();
+    tracer
+        .trace_value(&mut samples, public_key)
+        .expect("trace value Ed25519PublicKey");
+    assert!(samples.value("Ed25519PublicKey").is_some());
+    // The Ed25519PublicKey struct and its ser/de implementation treats itself as a "newtype struct".
+    // But `trace_type()` only supports the base type.
+    tracer
+        .trace_type::<VerificationKey>(&samples)
+        .expect("trace type VerificationKey");
+}
+
 #[test]
 fn test_serde_signatures_non_human_readable() {
     let message = b"hello, narwhal";
@@ -68,10 +101,9 @@ fn test_serde_signatures_human_readable() {
     let signature = kp.sign(message);
 
     let serialized = serde_json::to_string(&signature).unwrap();
-    println!("{:?}", serialized);
     assert_eq!(
         format!(
-            "\"{}\"",
+            r#"{{"base64":"{}"}}"#,
             base64ct::Base64::encode_string(&signature.sig.to_bytes())
         ),
         serialized

crypto/src/tests/ed25519_tests.rs

@@ -126,18 +126,6 @@ fn get_registry() -> Result<Registry> {
     tracer.trace_type::<HeaderDigest>(&samples)?;
     tracer.trace_type::<CertificateDigest>(&samples)?;
 
-    // Caveat: the following (trace_type) won't work, but not because of generics.
-    //
-    // Generic types instantiated multiple times in the same tracing session requires a work around.
-    // https://docs.rs/serde-reflection/latest/serde_reflection/#features-and-limitations
-    // but here we should be fine.
-    //
-    // This doesn't work because of the custom ser/de in PublicKey, which carries through to most top-level messages
-    //
-    // tracer.trace_type::<Header<Ed25519PublicKey>>(&samples)?;
-    // tracer.trace_type::<Certificate<Ed25519PublicKey>>(&samples)?;
-    // tracer.trace_type::<PrimaryWorkerMessage<Ed25519PublicKey>>(&samples)?;
-
     // The final entry points that we must document
     tracer.trace_type::<WorkerPrimaryMessage>(&samples)?;
     tracer.trace_type::<WorkerPrimaryError>(&samples)?;

node/src/generate_format.rs

@@ -26,7 +26,7 @@ Certificate:
         SEQ:
           TUPLE:
             - TYPENAME: Ed25519PublicKey
-            - SEQ: U8
+            - TYPENAME: Ed25519Signature
 CertificateDigest:
   NEWTYPESTRUCT:
     TUPLEARRAY:
@@ -43,6 +43,9 @@ Committee:
     - epoch: U64
 Ed25519PublicKey:
   NEWTYPESTRUCT: STR
+Ed25519Signature:
+  STRUCT:
+    - raw: BYTES
 Header:
   STRUCT:
     - author:
@@ -60,7 +63,7 @@ Header:
     - id:
         TYPENAME: HeaderDigest
     - signature:
-        SEQ: U8
+        TYPENAME: Ed25519Signature
 HeaderDigest:
   NEWTYPESTRUCT:
     TUPLEARRAY:
@@ -152,4 +155,3 @@ WorkerPrimaryMessage:
       Reconfigure:
         NEWTYPE:
           TYPENAME: ReconfigureNotification
-