From 595c6d740431a6aed74cf966ba524f174f62db24 Mon Sep 17 00:00:00 2001 From: Grzegorz Prusak Date: Wed, 25 Sep 2024 17:16:06 +0200 Subject: [PATCH] refactor: explicit deny_unknown_fields for protobuf serde deserialization (#200) It turned out to be too easy to use it incorrectly with an implicit default. --- node/Cargo.lock | 24 +-- .../examples/conformance_test/main.rs | 9 +- node/libs/protobuf/src/serde.rs | 163 +++++++++++++----- node/libs/protobuf/src/testonly/mod.rs | 24 ++- node/tools/src/bin/localnet_config.rs | 4 +- node/tools/src/config.rs | 24 --- node/tools/src/k8s.rs | 7 +- node/tools/src/main.rs | 11 +- 8 files changed, 166 insertions(+), 100 deletions(-) diff --git a/node/Cargo.lock b/node/Cargo.lock index 566e7d38..0bc1fdaf 100644 --- a/node/Cargo.lock +++ b/node/Cargo.lock @@ -3215,7 +3215,7 @@ dependencies = [ [[package]] name = "tester" -version = "0.1.1" +version = "0.2.0" dependencies = [ "anyhow", "clap", @@ -3923,7 +3923,7 @@ dependencies = [ [[package]] name = "zksync_concurrency" -version = "0.1.1" +version = "0.2.0" dependencies = [ "anyhow", "assert_matches", @@ -3941,7 +3941,7 @@ dependencies = [ [[package]] name = "zksync_consensus_bft" -version = "0.1.1" +version = "0.2.0" dependencies = [ "anyhow", "assert_matches", @@ -3965,7 +3965,7 @@ dependencies = [ [[package]] name = "zksync_consensus_crypto" -version = "0.1.1" +version = "0.2.0" dependencies = [ "anyhow", "blst", @@ -3985,7 +3985,7 @@ dependencies = [ [[package]] name = "zksync_consensus_executor" -version = "0.1.1" +version = "0.2.0" dependencies = [ "anyhow", "async-trait", @@ -4007,7 +4007,7 @@ dependencies = [ [[package]] name = "zksync_consensus_network" -version = "0.1.1" +version = "0.2.0" dependencies = [ "anyhow", "assert_matches", @@ -4044,7 +4044,7 @@ dependencies = [ [[package]] name = "zksync_consensus_roles" -version = "0.1.1" +version = "0.2.0" dependencies = [ "anyhow", "assert_matches", @@ -4065,7 +4065,7 @@ dependencies = [ [[package]] name = "zksync_consensus_storage" -version = "0.1.1" +version = "0.2.0" dependencies = [ "anyhow", "assert_matches", @@ -4087,7 +4087,7 @@ dependencies = [ [[package]] name = "zksync_consensus_tools" -version = "0.1.1" +version = "0.2.0" dependencies = [ "anyhow", "async-trait", @@ -4122,7 +4122,7 @@ dependencies = [ [[package]] name = "zksync_consensus_utils" -version = "0.1.1" +version = "0.2.0" dependencies = [ "anyhow", "rand", @@ -4132,7 +4132,7 @@ dependencies = [ [[package]] name = "zksync_protobuf" -version = "0.1.1" +version = "0.2.0" dependencies = [ "anyhow", "bit-vec", @@ -4154,7 +4154,7 @@ dependencies = [ [[package]] name = "zksync_protobuf_build" -version = "0.1.1" +version = "0.2.0" dependencies = [ "anyhow", "heck", diff --git a/node/libs/protobuf/examples/conformance_test/main.rs b/node/libs/protobuf/examples/conformance_test/main.rs index e432993b..5d36c8d8 100644 --- a/node/libs/protobuf/examples/conformance_test/main.rs +++ b/node/libs/protobuf/examples/conformance_test/main.rs @@ -18,7 +18,10 @@ mod proto; /// Decodes a generated proto message from json for arbitrary ReflectMessage. fn decode_json_proto(json: &str) -> anyhow::Result { let mut deserializer = serde_json::Deserializer::from_str(json); - let proto: T = zksync_protobuf::serde::deserialize_proto(&mut deserializer)?; + let proto: T = zksync_protobuf::serde::Deserialize { + deny_unknown_fields: true, + } + .proto(&mut deserializer)?; deserializer.end()?; Ok(proto) } @@ -26,7 +29,9 @@ fn decode_json_proto(json: &str) -> anyhow::Result< /// Encodes a generated proto message to json for arbitrary ReflectMessage. fn encode_json_proto(proto: &T) -> String { let mut serializer = serde_json::Serializer::pretty(vec![]); - zksync_protobuf::serde::serialize_proto(proto, &mut serializer).unwrap(); + zksync_protobuf::serde::Serialize + .proto(proto, &mut serializer) + .unwrap(); String::from_utf8(serializer.into_inner()).unwrap() } diff --git a/node/libs/protobuf/src/serde.rs b/node/libs/protobuf/src/serde.rs index 1a2308fc..413b7932 100644 --- a/node/libs/protobuf/src/serde.rs +++ b/node/libs/protobuf/src/serde.rs @@ -3,64 +3,135 @@ //! therefore it is suitable for version control. //! WARNING: Currently this serde implementation uses reflection, //! so it is not very efficient. -use crate::ProtoFmt; +use crate::{ProtoFmt, ProtoRepr}; use prost::Message as _; use prost_reflect::ReflectMessage; -/// ProtoFmt wrapper which implements serde Serialize/Deserialize. -#[derive(Debug, Clone)] -pub struct Serde(pub T); +/// Serialization options. +pub struct Serialize; -impl serde::Serialize for Serde { - fn serialize(&self, s: S) -> Result { - serialize(&self.0, s) +impl Serialize { + /// Serializes ReflectMessage. + pub fn proto( + &self, + x: &T, + s: S, + ) -> Result { + let opts = prost_reflect::SerializeOptions::new(); + x.transcode_to_dynamic().serialize_with_options(s, &opts) } -} -impl<'de, T: ProtoFmt> serde::Deserialize<'de> for Serde { - fn deserialize>(d: D) -> Result { - Ok(Self(deserialize(d)?)) + /// Serializes ProtoFmt. + pub fn proto_fmt( + &self, + x: &T, + s: S, + ) -> Result { + self.proto(&x.build(), s) } -} -/// Implementation of serde::Serialize for arbitrary ReflectMessage. -pub fn serialize_proto( - x: &T, - s: S, -) -> Result { - let opts = prost_reflect::SerializeOptions::new(); - x.transcode_to_dynamic().serialize_with_options(s, &opts) -} + /// Serializes ProtoFmt to json. + pub fn proto_fmt_to_json(&self, x: &T) -> String { + let mut s = serde_json::Serializer::pretty(vec![]); + self.proto_fmt(x, &mut s).unwrap(); + String::from_utf8(s.into_inner()).unwrap() + } -/// Implementation of serde::Serialize for arbitrary ProtoFmt. -pub fn serialize(x: &T, s: S) -> Result { - serialize_proto(&x.build(), s) -} + /// Serializes ProtoFmt to yaml + pub fn proto_fmt_to_yaml(&self, x: &T) -> String { + let mut s = serde_yaml::Serializer::new(vec![]); + self.proto_fmt(x, &mut s).unwrap(); + String::from_utf8(s.into_inner().unwrap()).unwrap() + } -/// Implementation of serde::Deserialize for arbitrary ReflectMessage denying unknown fields -pub fn deserialize_proto<'de, T: ReflectMessage + Default, D: serde::Deserializer<'de>>( - d: D, -) -> Result { - deserialize_proto_with_options(d, true) + /// Serializes ProtoRepr. + pub fn proto_repr( + &self, + x: &T::Type, + s: S, + ) -> Result { + self.proto(&T::build(x), s) + } + + /// Serializes ProtoRepr to json. + pub fn proto_repr_to_json(&self, x: &T::Type) -> String { + let mut s = serde_json::Serializer::pretty(vec![]); + self.proto_repr::(x, &mut s).unwrap(); + String::from_utf8(s.into_inner()).unwrap() + } + + /// Serializes ProtoRepr to yaml + pub fn proto_repr_to_yaml(&self, x: &T::Type) -> String { + let mut s = serde_yaml::Serializer::new(vec![]); + self.proto_repr::(x, &mut s).unwrap(); + String::from_utf8(s.into_inner().unwrap()).unwrap() + } } -/// Implementation of serde::Deserialize for arbitrary ReflectMessage with deny_unknown_fields option -pub fn deserialize_proto_with_options< - 'de, - T: ReflectMessage + Default, - D: serde::Deserializer<'de>, ->( - d: D, - deny_unknown_fields: bool, -) -> Result { - let mut p = T::default(); - let options = prost_reflect::DeserializeOptions::new().deny_unknown_fields(deny_unknown_fields); - let msg = prost_reflect::DynamicMessage::deserialize_with_options(p.descriptor(), d, &options)?; - p.merge(msg.encode_to_vec().as_slice()).unwrap(); - Ok(p) +/// Deserialization options. +#[derive(Default)] +pub struct Deserialize { + /// true => returns an error when an unknown field is found. + /// false => silently ignores unknown fields. + pub deny_unknown_fields: bool, } -/// Implementation of serde::Deserialize for arbitrary ProtoFmt. -pub fn deserialize<'de, T: ProtoFmt, D: serde::Deserializer<'de>>(d: D) -> Result { - T::read(&deserialize_proto(d)?).map_err(serde::de::Error::custom) +impl Deserialize { + /// Implementation of serde::Deserialize for arbitrary ReflectMessage with deny_unknown_fields option + pub fn proto<'de, T: ReflectMessage + Default, D: serde::Deserializer<'de>>( + &self, + d: D, + ) -> Result { + let mut p = T::default(); + let options = + prost_reflect::DeserializeOptions::new().deny_unknown_fields(self.deny_unknown_fields); + let msg = + prost_reflect::DynamicMessage::deserialize_with_options(p.descriptor(), d, &options)?; + p.merge(msg.encode_to_vec().as_slice()).unwrap(); + Ok(p) + } + + /// Implementation of serde::Deserialize for arbitrary ProtoFmt. + pub fn proto_fmt<'de, T: ProtoFmt, D: serde::Deserializer<'de>>( + &self, + d: D, + ) -> Result { + T::read(&self.proto(d)?).map_err(serde::de::Error::custom) + } + + /// Deserializes ProtoFmt from json. + pub fn proto_fmt_from_json(&self, json: &str) -> anyhow::Result { + let mut d = serde_json::Deserializer::from_str(json); + let p = self.proto_fmt(&mut d)?; + d.end()?; + Ok(p) + } + + /// Deserializes ProtoFmt from yaml. + pub fn proto_fmt_from_yaml(&self, yaml: &str) -> anyhow::Result { + Ok(self.proto_fmt(serde_yaml::Deserializer::from_str(yaml))?) + } + + /// Implementation of serde::Deserialize for arbitrary ProtoFmt. + pub fn proto_repr<'de, T: ProtoRepr, D: serde::Deserializer<'de>>( + &self, + d: D, + ) -> Result { + self.proto::(d)? + .read() + .map_err(serde::de::Error::custom) + } + + /// Deserializes ProtoRepr from json. + pub fn proto_repr_from_json(&self, json: &str) -> anyhow::Result { + let mut d = serde_json::Deserializer::from_str(json); + let p = self.proto_repr::(&mut d)?; + d.end()?; + Ok(p) + } + + /// Deserializes ProtoRepr from yaml. + pub fn proto_repr_from_yaml(&self, yaml: &str) -> anyhow::Result { + Ok(self.proto_repr::(serde_yaml::Deserializer::from_str(yaml))?) + } } diff --git a/node/libs/protobuf/src/testonly/mod.rs b/node/libs/protobuf/src/testonly/mod.rs index 8fbab552..2896acbf 100644 --- a/node/libs/protobuf/src/testonly/mod.rs +++ b/node/libs/protobuf/src/testonly/mod.rs @@ -1,5 +1,9 @@ //! Testonly utilities. -use super::{canonical, canonical_raw, decode, encode, read_fields, ProtoFmt, ProtoRepr, Wire}; +use super::{ + canonical, canonical_raw, decode, encode, read_fields, + serde::{Deserialize, Serialize}, + ProtoFmt, ProtoRepr, Wire, +}; use prost::Message as _; use prost_reflect::ReflectMessage; use rand::{ @@ -114,24 +118,34 @@ fn decode_proto(bytes: &[u8]) -> anyhow::Result { fn encode_json(msg: &X::Type) -> String { let mut s = serde_json::Serializer::pretty(vec![]); - crate::serde::serialize_proto(&X::build(msg), &mut s).unwrap(); + Serialize.proto(&X::build(msg), &mut s).unwrap(); String::from_utf8(s.into_inner()).unwrap() } fn decode_json(json: &str) -> anyhow::Result { let mut d = serde_json::Deserializer::from_str(json); - X::read(&crate::serde::deserialize_proto(&mut d)?) + X::read( + &Deserialize { + deny_unknown_fields: true, + } + .proto(&mut d)?, + ) } fn encode_yaml(msg: &X::Type) -> String { let mut s = serde_yaml::Serializer::new(vec![]); - crate::serde::serialize_proto(&X::build(msg), &mut s).unwrap(); + Serialize.proto(&X::build(msg), &mut s).unwrap(); String::from_utf8(s.into_inner().unwrap()).unwrap() } fn decode_yaml(yaml: &str) -> anyhow::Result { let d = serde_yaml::Deserializer::from_str(yaml); - X::read(&crate::serde::deserialize_proto(d)?) + X::read( + &Deserialize { + deny_unknown_fields: true, + } + .proto(d)?, + ) } /// Wrapper for `ProtoRepr`, implementing ProtoConv; diff --git a/node/tools/src/bin/localnet_config.rs b/node/tools/src/bin/localnet_config.rs index 197e90a1..90e603c8 100644 --- a/node/tools/src/bin/localnet_config.rs +++ b/node/tools/src/bin/localnet_config.rs @@ -11,7 +11,7 @@ use std::{ }; use zksync_consensus_roles::{node, validator}; use zksync_consensus_tools::config; -use zksync_protobuf::serde::Serde; +use zksync_protobuf::serde::Serialize; /// Command line arguments. #[derive(Debug, Parser)] @@ -111,7 +111,7 @@ fn main() -> anyhow::Result<()> { .context("fs::set_permissions()")?; let config_path = root.join("config.json"); - fs::write(&config_path, config::encode_json(&Serde(cfg))).context("fs::write()")?; + fs::write(&config_path, Serialize.proto_fmt_to_json(&cfg)).context("fs::write()")?; fs::set_permissions(&config_path, Permissions::from_mode(0o600)) .context("fs::set_permissions()")?; } diff --git a/node/tools/src/config.rs b/node/tools/src/config.rs index 26ffcb1d..be36dc56 100644 --- a/node/tools/src/config.rs +++ b/node/tools/src/config.rs @@ -1,7 +1,6 @@ //! Node configuration. use crate::{proto, store}; use anyhow::{anyhow, Context as _}; -use serde_json::ser::Formatter; use std::{ collections::{HashMap, HashSet}, fs, io, @@ -42,29 +41,6 @@ fn read_optional_secret_text(text: &Option) -> anyhow::Resul /// Ports for the nodes to listen on kubernetes pod. pub const NODES_PORT: u16 = 3054; -/// Decodes a proto message from json for arbitrary ProtoFmt. -pub fn decode_json(json: &str) -> anyhow::Result { - let mut d = serde_json::Deserializer::from_str(json); - let p = T::deserialize(&mut d)?; - d.end()?; - Ok(p) -} - -/// Encodes a generated proto message to json for arbitrary ProtoFmt. -pub fn encode_json(x: &T) -> String { - let s = serde_json::Serializer::pretty(vec![]); - encode_with_serializer(x, s) -} - -/// Encodes a generated proto message for arbitrary ProtoFmt with provided serializer. -pub(crate) fn encode_with_serializer( - x: &T, - mut serializer: serde_json::Serializer, F>, -) -> String { - T::serialize(x, &mut serializer).unwrap(); - String::from_utf8(serializer.into_inner()).unwrap() -} - /// Pair of (public key, host addr) for a gossip network node. #[derive(Debug, Clone)] pub struct NodeAddr { diff --git a/node/tools/src/k8s.rs b/node/tools/src/k8s.rs index 56b00b8e..29ab0961 100644 --- a/node/tools/src/k8s.rs +++ b/node/tools/src/k8s.rs @@ -18,7 +18,7 @@ use kube::{ use std::{collections::BTreeMap, net::SocketAddr, time::Duration}; use tokio::time; use tracing::log::info; -use zksync_protobuf::serde::Serde; +use zksync_protobuf::serde::Serialize; /// Docker image name for consensus nodes. const DOCKER_IMAGE_NAME: &str = "consensus-node"; @@ -335,10 +335,7 @@ fn is_pod_running(pod: &Pod) -> bool { fn get_cli_args(consensus_node: &ConsensusNode) -> Vec { vec![ "--config".to_string(), - config::encode_with_serializer( - &Serde(consensus_node.config.clone()), - serde_json::Serializer::new(vec![]), - ), + Serialize.proto_fmt_to_json(&consensus_node.config), ] } diff --git a/node/tools/src/main.rs b/node/tools/src/main.rs index 73e3a39b..bf79a9bd 100644 --- a/node/tools/src/main.rs +++ b/node/tools/src/main.rs @@ -8,7 +8,7 @@ use tracing_subscriber::{prelude::*, Registry}; use vise_exporter::MetricsExporter; use zksync_concurrency::{ctx, scope}; use zksync_consensus_tools::{config, RPCServer}; -use zksync_protobuf::serde::Serde; +use zksync_protobuf::serde::Deserialize; /// Command-line application launching a node executor. #[derive(Debug, Parser)] @@ -27,12 +27,15 @@ struct Cli { impl Cli { /// Extracts configuration from the cli args. fn load(&self) -> anyhow::Result { - let raw = match &self.config { - Some(raw) => raw.clone(), + let json = match &self.config { + Some(json) => json.clone(), None => fs::read_to_string(&self.config_path)?, }; Ok(config::Configs { - app: config::decode_json::>(&raw)?.0, + app: Deserialize { + deny_unknown_fields: true, + } + .proto_fmt_from_json(&json)?, database: self.database.clone(), }) }