vincenzopalazzo
/
arti
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Merge branch 'bridge-transp-parse' into 'main' 

Use TransportId FromStr in BridgeConfig parsing

Closes #653

See merge request tpo/core/arti!881
This commit is contained in:
Ian Jackson 2022-11-22 16:27:43 +00:00
parent 2327e1f60f 822a6ce32b
commit 7a082cb8f0
3 changed files with 124 additions and 52 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

71
crates/tor-guardmgr/src/bridge/config.rs
View File

@ -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<Vec<_>>` 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"
}"#],

35
crates/tor-linkspec/src/traits.rs
View File

@ -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]"
);
}
}
}

70
crates/tor-linkspec/src/transport.rs
View File

@ -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 `"<none>"` 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 = "<none>";
// 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", "<none>"];
impl FromStr for TransportId {
type Err = TransportIdError;
fn from_str(s: &str) -> Result<Self, Self::Err> {
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<PtTransportName>` 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<PtTransportName> {
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("<none>").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(_))
));
}