From 71e81bf7b4888a1a60b2fc13a2e15b5d5387dfaa Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 24 Nov 2022 17:12:32 +0000 Subject: [PATCH 1/6] MultilineListBuilder: Use a manual implementation of Deserialize The error message from `#[serde(untagged)]` would otherwise start to appear when we try to deserialise unsupported PT configurations, when compiled with bridge but not PT support. --- crates/tor-config/src/list_builder.rs | 54 ++++++++++++++++++++++++++- 1 file changed, 52 insertions(+), 2 deletions(-) diff --git a/crates/tor-config/src/list_builder.rs b/crates/tor-config/src/list_builder.rs index d79dcee01..b5221e930 100644 --- a/crates/tor-config/src/list_builder.rs +++ b/crates/tor-config/src/list_builder.rs @@ -133,11 +133,13 @@ //! assert_eq!{ builder.build().unwrap().values, &[27, 12] } //! ``` +use std::fmt; +use std::marker::PhantomData; use std::str::FromStr; use educe::Educe; use itertools::Itertools; -use serde::{Deserialize, Serialize}; +use serde::{Deserialize, Deserializer, Serialize}; use thiserror::Error; pub use crate::define_list_builder_accessors; @@ -568,7 +570,7 @@ define_list_builder_helper! { /// let lc = lc.build().unwrap(); /// assert_eq!{ lc.winners, [4,5,6] } /// ``` -#[derive(Clone, Debug, Educe, Serialize, Deserialize)] +#[derive(Clone, Debug, Educe, Serialize)] #[serde(untagged)] #[educe(Default)] #[non_exhaustive] @@ -606,6 +608,54 @@ pub struct MultilineListBuilderError error: E, } +// We could derive this with `#[serde(untagged)]` but that produces quite terrible error +// messages, which do not reproduce the error messages from any of the variants. +// +// Instead, have a manual implementation, which can see whether the input is a list or a string. +impl<'de, EB: Deserialize<'de>> Deserialize<'de> for MultilineListBuilder { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + deserializer.deserialize_any(MllbVisitor::default()) + } +} + +/// Visitor for deserialize_any for [`MultilineListBuilder`] +#[derive(Educe)] +#[educe(Default)] +struct MllbVisitor { + /// Variance: this visitor constructs `EB`s + ret: PhantomData EB>, +} + +impl<'de, EB: Deserialize<'de>> serde::de::Visitor<'de> for MllbVisitor { + type Value = MultilineListBuilder; + + fn expecting(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "list of items, or multi-line string") + } + + fn visit_seq>(self, mut seq: A) -> Result { + let mut v = vec![]; + while let Some(e) = seq.next_element()? { + v.push(e); + } + Ok(MultilineListBuilder::List(v)) + } + + fn visit_str(self, v: &str) -> Result { + self.visit_string(v.to_owned()) + } + fn visit_string(self, v: String) -> Result { + Ok(MultilineListBuilder::String(v)) + } + + fn visit_none(self) -> Result { + Ok(MultilineListBuilder::Unspecified) + } +} + impl From>> for MultilineListBuilder { fn from(list: Option>) -> Self { use MultilineListBuilder as MlLB; From d05d383fa573fe4cd697b1d02854dd30dab7b86f Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 24 Nov 2022 16:50:32 +0000 Subject: [PATCH 2/6] arti cfg tests: Add some more debug output --- crates/arti/src/cfg.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/arti/src/cfg.rs b/crates/arti/src/cfg.rs index 75a16d449..79e1d479b 100644 --- a/crates/arti/src/cfg.rs +++ b/crates/arti/src/cfg.rs @@ -964,6 +964,7 @@ mod test { fn parse(&self) -> config::Config { let s: String = chain!(iter::once(&self.section), self.lines.iter(),).join("\n"); + eprintln!("parsing\n --\n{}\n --", &s); let c: toml::Value = toml::from_str(&s).expect(&s); config::Config::try_from(&c).expect(&s) } From c92dcb5f501d5f3035d3289e5a6a4b8098e8324f Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 29 Nov 2022 16:12:13 +0000 Subject: [PATCH 3/6] arti, arti-client: Conditionalise various things on pt-client --- crates/arti-client/src/config.rs | 7 ++++--- crates/arti/src/cfg.rs | 1 + 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/crates/arti-client/src/config.rs b/crates/arti-client/src/config.rs index 3180150f8..dfe5bee9b 100644 --- a/crates/arti-client/src/config.rs +++ b/crates/arti-client/src/config.rs @@ -44,9 +44,7 @@ pub mod dir { } /// Types for configuring pluggable transports. -// -// TODO pt-client: Could we make these ignored when pt-client is not enabled? If -// so, we could make the ptmgr dependency optional, which is something we wanted. +#[cfg(feature = "pt-client")] pub mod pt { pub use tor_ptmgr::config::{ManagedTransportConfig, ManagedTransportConfigBuilder}; } @@ -245,12 +243,15 @@ pub struct BridgesConfig { /// Configured list of pluggable transports. #[builder(sub_builder, setter(custom))] #[builder_field_attr(serde(default))] + #[cfg(feature = "pt-client")] pub(crate) transports: TransportConfigList, } /// A list of configured transport binaries (type alias for macrology). +#[cfg(feature = "pt-client")] type TransportConfigList = Vec; +#[cfg(feature = "pt-client")] define_list_builder_helper! { pub(crate) struct TransportConfigListBuilder { transports: [pt::ManagedTransportConfigBuilder], diff --git a/crates/arti/src/cfg.rs b/crates/arti/src/cfg.rs index 79e1d479b..a580c6e5d 100644 --- a/crates/arti/src/cfg.rs +++ b/crates/arti/src/cfg.rs @@ -569,6 +569,7 @@ mod test { // example present in ARTI_EXAMPLE_CONFIG. // https://gitlab.torproject.org/tpo/core/arti/-/merge_requests/823#note_2854365 // and bullet points 2 and 3 in the doc for `exhaustive_1`, below. + #[cfg(feature = "pt-client")] "bridges.transports", "tor_network.authorities", "tor_network.fallback_caches", From 1bd606ab5ceb844c1c15820e8e11564d59a64e06 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 29 Nov 2022 16:08:24 +0000 Subject: [PATCH 4/6] guardmgr: Conditionalise DisplayRule::Redacted This is unused in bridgeless configs. Fixes a compiler warning. --- crates/tor-guardmgr/src/guard.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/crates/tor-guardmgr/src/guard.rs b/crates/tor-guardmgr/src/guard.rs index d008aa28e..a791cd653 100644 --- a/crates/tor-guardmgr/src/guard.rs +++ b/crates/tor-guardmgr/src/guard.rs @@ -15,7 +15,10 @@ use crate::skew::SkewObservation; use crate::util::randomize_time; use crate::{ids::GuardId, GuardParams, GuardRestriction, GuardUsage}; use crate::{sample, ExternalActivity, GuardSetSelector, GuardUsageKind}; + +#[cfg(feature = "bridge-client")] use safelog::Redactable as _; + use tor_linkspec::{ ChanTarget, ChannelMethod, HasAddrs, HasChanMethod, HasRelayIds, PtTarget, RelayIds, }; @@ -86,6 +89,7 @@ pub(crate) enum DisplayRule { /// $ab...". /// /// We use this for bridges. + #[cfg(feature = "bridge-client")] Redacted, } @@ -847,6 +851,7 @@ impl std::fmt::Display for Guard { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self.sensitivity { DisplayRule::Sensitive => safelog::sensitive(self.display_chan_target()).fmt(f), + #[cfg(feature = "bridge-client")] DisplayRule::Redacted => self.display_chan_target().redacted().fmt(f), } } From 143b331ef78a25c7f36971e24d55c68a9a60fd37 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 24 Nov 2022 16:50:11 +0000 Subject: [PATCH 5/6] arti-client: Make ptmgr actually optional --- crates/arti-client/Cargo.toml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/arti-client/Cargo.toml b/crates/arti-client/Cargo.toml index f21d0fc5b..fe13fb505 100644 --- a/crates/arti-client/Cargo.toml +++ b/crates/arti-client/Cargo.toml @@ -36,7 +36,7 @@ async-std = ["tor-rtcompat/async-std"] bridge-client = ["tor-guardmgr/bridge-client", "tor-dirmgr/bridge-client"] tokio = ["tor-rtcompat/tokio", "tor-proto/tokio"] native-tls = ["tor-rtcompat/native-tls"] -pt-client = ["bridge-client", "tor-chanmgr/pt-client", "tor-guardmgr/pt-client"] +pt-client = ["bridge-client", "tor-chanmgr/pt-client", "tor-guardmgr/pt-client", "tor-ptmgr"] rustls = ["tor-rtcompat/rustls"] static = ["static-sqlite", "static-native-tls"] @@ -99,7 +99,7 @@ tor-netdir = { path = "../tor-netdir", version = "0.6.0" } tor-netdoc = { path = "../tor-netdoc", version = "0.5.2" } tor-persist = { path = "../tor-persist", version = "0.5.1" } tor-proto = { path = "../tor-proto", version = "0.7.0" } -tor-ptmgr = { path = "../tor-ptmgr", version = "0.1.0" } +tor-ptmgr = { path = "../tor-ptmgr", version = "0.1.0", optional = true } tor-rtcompat = { path = "../tor-rtcompat", version = "0.7.0" } tracing = "0.1.18" @@ -119,6 +119,7 @@ tokio-crate = { package = "tokio", version = "1.7", features = [ tokio-util = { version = "0.7.0", features = ["compat"] } tor-rtcompat = { path = "../tor-rtcompat", version = "0.7.0", features = ["tokio", "native-tls"] } tracing-subscriber = "0.3.0" + [package.metadata.docs.rs] all-features = true rustdoc-args = ["--cfg", "docsrs"] From b6a4f2388d437f71558a986debc4f8d98a7742d6 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 29 Nov 2022 16:29:59 +0000 Subject: [PATCH 6/6] CI: test that we can compile, and test cfg, with/without bridges/PTs --- .gitlab-ci.yml | 6 ++++++ maint/matrix_test_cfg | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+) create mode 100755 maint/matrix_test_cfg diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index b64bb18d8..d0e16bc11 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -287,6 +287,12 @@ matrix-test: - apt-get update && apt-get install -y python3-toml python-is-python3 - ./maint/matrix_test +matrix-test-cfg: + stage: test + image: rust:latest + script: + - ./maint/matrix_test_cfg + coverage-aggregated: rules: diff --git a/maint/matrix_test_cfg b/maint/matrix_test_cfg new file mode 100755 index 000000000..45238dd31 --- /dev/null +++ b/maint/matrix_test_cfg @@ -0,0 +1,18 @@ +#!/bin/bash +# +# Run cargo check and cargo test with various featuresets relevant to configuration + +set -euo pipefail + +: "${CARGO:=cargo}" + +set -x + +for feat in '' bridge-client pt-client; do + feat_args=(--no-default-features "--features=tokio,native-tls,$feat") + + $CARGO check "${feat_args[@]}" -p arti-client + $CARGO check "${feat_args[@]}" -p arti + $CARGO test "${feat_args[@]}" -p arti -- cfg +done +