diff --git a/crates/tor-guardmgr/src/bridge/config.rs b/crates/tor-guardmgr/src/bridge/config.rs index 11ad8ddc9..2b28df165 100644 --- a/crates/tor-guardmgr/src/bridge/config.rs +++ b/crates/tor-guardmgr/src/bridge/config.rs @@ -12,6 +12,7 @@ use tor_basic_utils::derive_serde_raw; use tor_config::define_list_builder_accessors; use tor_config::{impl_standard_builder, ConfigBuildError}; use tor_linkspec::RelayId; +use tor_linkspec::TransportId; use tor_linkspec::{ChanTarget, ChannelMethod, HasChanMethod}; use tor_linkspec::{HasAddrs, HasRelayIds, RelayIdRef, RelayIdType}; use tor_llcrypto::pk::{ed25519::Ed25519Identity, rsa::RsaIdentity}; @@ -216,15 +217,21 @@ impl BridgeConfigBuilder { problem: problem.to_string(), }; - #[allow(clippy::comparison_to_empty)] // .is_empty() would *not* be clearer - let addrs = if transport == "" || transport == "-" || transport == "bridge" { - if !settings.is_empty() { - return Err(inconsist_transp( - "settings", - "Specified `settings` for a direct bridge connection", - )); - } - let addrs = addrs.iter().filter_map(|pta| match pta { + let transp: TransportId = transport + .parse() + .map_err(|e| invalid("transport".into(), &e))?; + + // This match seems redundant, but it allows us to apply #[cfg] to the branches, + // which isn't possible with `if ... else ...`. + let addrs = match () { + () if transp.is_builtin() => { + if !settings.is_empty() { + return Err(inconsist_transp( + "settings", + "Specified `settings` for a direct bridge connection", + )); + } + let addrs = addrs.iter().filter_map(|pta| match pta { BridgeAddr::IpPort(sa) => Some(Ok(*sa)), BridgeAddr::HostPort(..) => Some(Err( "`addrs` contains hostname and port, but only numeric addresses are supported for a direct bridge connection", @@ -237,23 +244,18 @@ impl BridgeConfigBuilder { "addrs", problem, ))?; - if addrs.is_empty() { - return Err(inconsist_transp( - "addrs", - "Missing `addrs` for a direct bridge connection", - )); - } - ChannelMethod::Direct(addrs) - } else { - #[cfg(not(feature = "pt-client"))] - { - return Err(unsupported( - "transport".into(), - &"pluggable transport support disabled in tor-guardmgr cargo features", - )); + if addrs.is_empty() { + return Err(inconsist_transp( + "addrs", + "Missing `addrs` for a direct bridge connection", + )); + } + ChannelMethod::Direct(addrs) } + #[cfg(feature = "pt-client")] - { + () if transp.as_pluggable().is_some() => { + let transport = transp.into_pluggable().expect("became not pluggable!"); let addr = match addrs { [] => BridgeAddr::None, @@ -263,9 +265,6 @@ impl BridgeConfigBuilder { "Transport (non-direct bridge) only supports a single nominal address", )), }; - let transport = transport - .parse() - .map_err(|e| invalid("transport".into(), &e))?; let mut target = PtTarget::new(transport, addr); for (i, (k, v)) in settings.iter().enumerate() { // Using PtTargetSettings TryFrom would prevent us reporting the index i @@ -275,6 +274,17 @@ impl BridgeConfigBuilder { } ChannelMethod::Pluggable(target) } + + () => { + // With current code, this can only happen if tor-linkspec has pluggable + // transports enabled, but we don't. But if `TransportId` gains other + // inner variants, it would trigger. + return Err(unsupported( + "transport".into(), + &format_args!("support for selected transport '{}' disabled in tor-guardmgr cargo features", + transp), + )); + } }; let mut rsa_id = None; @@ -820,9 +830,10 @@ mod test { let got_emsg = err.to_string(); assert!( got_emsg.contains(emsg), - "wrong error message: got_emsg={:?} err={:?}", + "wrong error message: got_emsg={:?} err={:?} expected={:?}", &got_emsg, - &err + &err, + emsg, ); // This is a kludge. When we serialize `Option>` as JSON, @@ -849,7 +860,7 @@ mod test { #[cfg(not(feature = "pt-client"))] chk_broken( - "pluggable transport support disabled in tor-guardmgr cargo features", + "Not compiled with pluggable transport support", &[r#"{ "transport": "obfs4" }"#], diff --git a/crates/tor-linkspec/src/traits.rs b/crates/tor-linkspec/src/traits.rs index 74d5c6b56..e22d3f070 100644 --- a/crates/tor-linkspec/src/traits.rs +++ b/crates/tor-linkspec/src/traits.rs @@ -430,8 +430,6 @@ mod test { #[test] fn display() { - use crate::PtTarget; - let e1 = example(); assert_eq!( e1.display_chan_target().to_string(), @@ -439,19 +437,24 @@ mod test { $1234567890abcdef12341234567890abcdef1234]" ); - let rsa = hex!("234461644a6f6b6523436f726e794f6e4d61696e").into(); - let mut b = crate::OwnedChanTarget::builder(); - b.ids().rsa_identity(rsa); - let e2 = b - .method(ChannelMethod::Pluggable(PtTarget::new( - "obfs4".parse().unwrap(), - "127.0.0.1:99".parse().unwrap(), - ))) - .build() - .unwrap(); - assert_eq!( - e2.to_string(), - "[127.0.0.1:99 via obfs4 $234461644a6f6b6523436f726e794f6e4d61696e]" - ); + #[cfg(feature = "pt-client")] + { + use crate::PtTarget; + + let rsa = hex!("234461644a6f6b6523436f726e794f6e4d61696e").into(); + let mut b = crate::OwnedChanTarget::builder(); + b.ids().rsa_identity(rsa); + let e2 = b + .method(ChannelMethod::Pluggable(PtTarget::new( + "obfs4".parse().unwrap(), + "127.0.0.1:99".parse().unwrap(), + ))) + .build() + .unwrap(); + assert_eq!( + e2.to_string(), + "[127.0.0.1:99 via obfs4 $234461644a6f6b6523436f726e794f6e4d61696e]" + ); + } } } diff --git a/crates/tor-linkspec/src/transport.rs b/crates/tor-linkspec/src/transport.rs index dbee4795f..c62e754dd 100644 --- a/crates/tor-linkspec/src/transport.rs +++ b/crates/tor-linkspec/src/transport.rs @@ -21,6 +21,12 @@ use crate::HasAddrs; /// If this crate is compiled with the `pt-client` feature, this type can /// support pluggable transports; otherwise, only the built-in transport type is /// supported. +/// +/// This can be displayed as, or parsed from, a string. +/// `"-"` is used to indicate the builtin transport, +/// and `""` and `"bridge"` and `""` are also recognised for that. +// +// We recognise "bridge" as pluggable; "BRIDGE" is rejected as invalid. #[derive(Debug, Clone, Default, Eq, PartialEq, Hash)] pub struct TransportId(Inner); @@ -95,17 +101,20 @@ impl Display for PtTransportName { } } -/// This identifier is used to indicate the built-in transport. +/// These identifiers are used to indicate the built-in transport. +/// +/// When outputting string representations, the first (`"-"`) is used. // // Actual pluggable transport names are restricted to the syntax of C identifiers. -// This string deliberately is not in that syntax so as to avoid clashes. -const BUILT_IN_ID: &str = ""; +// These strings are deliberately not in that syntax so as to avoid clashes. +// `"bridge"` is likewise prohibited by the spec. +const BUILT_IN_IDS: &[&str] = &["-", "", "bridge", ""]; impl FromStr for TransportId { type Err = TransportIdError; fn from_str(s: &str) -> Result { - if s == BUILT_IN_ID { + if BUILT_IN_IDS.contains(&s) { return Ok(TransportId(Inner::BuiltIn)); }; @@ -123,7 +132,7 @@ impl FromStr for TransportId { impl Display for TransportId { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match &self.0 { - Inner::BuiltIn => write!(f, "{}", BUILT_IN_ID), + Inner::BuiltIn => write!(f, "{}", BUILT_IN_IDS[0]), #[cfg(feature = "pt-client")] Inner::Pluggable(name) => write!(f, "{}", name), } @@ -171,10 +180,51 @@ pub enum TransportIdError { } impl TransportId { + /// Return a new `TransportId` referencing the builtin transport + /// + /// This is equivalent to the `Default` impl. + pub fn new_builtin() -> Self { + TransportId(Inner::BuiltIn) + } + + /// Return a new `TransportId` referencing a pluggable transport + /// + /// This is equivalent to the `From` impl. + #[cfg(feature = "pt-client")] + pub fn new_pluggable(pt: PtTransportName) -> Self { + pt.into() + } + /// Return true if this is the built-in transport. pub fn is_builtin(&self) -> bool { self.0 == Inner::BuiltIn } + + /// Returns the pluggable transport name + /// + /// Or `None` if `self` doesn't specify a pluggable transport + /// (e.g. if it specifies the builtin transport). + #[cfg(feature = "pt-client")] + pub fn as_pluggable(&self) -> Option<&PtTransportName> { + match &self.0 { + Inner::BuiltIn => None, + #[cfg(feature = "pt-client")] + Inner::Pluggable(pt) => Some(pt), + } + } + + /// Consumes this `TransportId` and returns the pluggable transport name + /// + /// Or `None` if `self` doesn't specify a pluggable transport + /// (e.g. if it specifies the builtin transport). + #[cfg(feature = "pt-client")] + pub fn into_pluggable(self) -> Option { + match self.0 { + Inner::BuiltIn => None, + #[cfg(feature = "pt-client")] + Inner::Pluggable(pt) => Some(pt), + } + } } /// This identifier is used to indicate no transport address. @@ -584,20 +634,28 @@ mod test { let obfs = TransportId::from_str("obfs4").unwrap(); let dflt = TransportId::default(); let dflt2 = TransportId::from_str("").unwrap(); + let dflt3 = TransportId::from_str("-").unwrap(); + let dflt4 = TransportId::from_str("").unwrap(); + let dflt5 = TransportId::from_str("bridge").unwrap(); let snow = TransportId::from_str("snowflake").unwrap(); let obfs_again = TransportId::from_str("obfs4").unwrap(); assert_eq!(obfs, obfs_again); assert_eq!(dflt, dflt2); + assert_eq!(dflt, dflt3); + assert_eq!(dflt, dflt4); + assert_eq!(dflt, dflt5); assert_ne!(snow, obfs); assert_ne!(snow, dflt); + assert_eq!(dflt.to_string(), "-"); + assert!(matches!( TransportId::from_str("12345"), Err(TransportIdError::BadId(_)) )); assert!(matches!( - TransportId::from_str("bridge"), + TransportId::from_str("Bridge"), Err(TransportIdError::BadId(_)) )); }