From dd03af1e6caa9d3b419c18cc239144ed310eb7bc Mon Sep 17 00:00:00 2001 From: Isabel Atkinson Date: Mon, 11 Nov 2024 15:47:58 -0700 Subject: [PATCH 1/5] RUST-229 Parse IPv6 addresses in the connection string --- src/client/options.rs | 151 ++++++++++++++++++++----------------- src/client/options/test.rs | 51 +------------ 2 files changed, 85 insertions(+), 117 deletions(-) diff --git a/src/client/options.rs b/src/client/options.rs index c58be4e4c..06287f96b 100644 --- a/src/client/options.rs +++ b/src/client/options.rs @@ -128,9 +128,29 @@ impl<'de> Deserialize<'de> for ServerAddress { where D: Deserializer<'de>, { - let s: String = Deserialize::deserialize(deserializer)?; - Self::parse(s.as_str()) - .map_err(|e| ::custom(format!("{}", e))) + #[derive(Deserialize)] + #[serde(untagged)] + enum ServerAddressHelper { + String(String), + Object { host: String, port: Option }, + } + + let helper = ServerAddressHelper::deserialize(deserializer)?; + match helper { + ServerAddressHelper::String(string) => { + Self::parse(string).map_err(serde::de::Error::custom) + } + ServerAddressHelper::Object { host, port } => { + #[cfg(unix)] + if host.ends_with("sock") { + return Ok(Self::Unix { + path: PathBuf::from(host), + }); + } + + Ok(Self::Tcp { host, port }) + } + } } } @@ -185,74 +205,83 @@ impl FromStr for ServerAddress { } impl ServerAddress { - /// Parses an address string into a `ServerAddress`. + /// Parses an address string into a [ServerAddress]. pub fn parse(address: impl AsRef) -> Result { let address = address.as_ref(); - // checks if the address is a unix domain socket + #[cfg(unix)] { if address.ends_with(".sock") { - return Ok(ServerAddress::Unix { + let address = percent_decode(address, "unix domain sockets must be URL-encoded")?; + return Ok(Self::Unix { path: PathBuf::from(address), }); } } - let mut parts = address.split(':'); - let hostname = match parts.next() { - Some(part) => { - if part.is_empty() { - return Err(ErrorKind::InvalidArgument { - message: format!( - "invalid server address: \"{}\"; hostname cannot be empty", - address - ), - } - .into()); + + let (hostname, port) = if let Some(ip_literal) = address.strip_prefix("[") { + let Some((hostname, port)) = ip_literal.split_once("]") else { + return Err(ErrorKind::InvalidArgument { + message: format!( + "invalid server address {}: missing closing ']' in IP literal hostname", + address + ), } - part - } - None => { + .into()); + }; + + let port = if port.is_empty() { + None + } else if let Some(port) = port.strip_prefix(":") { + Some(port) + } else { return Err(ErrorKind::InvalidArgument { - message: format!("invalid server address: \"{}\"", address), + message: format!( + "invalid server address {}: the hostname can only be followed by a port \ + prefixed with ':', got {}", + address, port + ), } - .into()) + .into()); + }; + + (hostname, port) + } else { + match address.split_once(":") { + Some((hostname, port)) => (hostname, Some(port)), + None => (address, None), } }; - let port = match parts.next() { - Some(part) => { - let port = u16::from_str(part).map_err(|_| ErrorKind::InvalidArgument { - message: format!( - "port must be valid 16-bit unsigned integer, instead got: {}", - part - ), - })?; + if hostname.is_empty() { + return Err(ErrorKind::InvalidArgument { + message: format!( + "invalid server address {}: the hostname cannot be empty", + address + ), + } + .into()); + } - if port == 0 { - return Err(ErrorKind::InvalidArgument { - message: format!( - "invalid server address: \"{}\"; port must be non-zero", - address - ), - } - .into()); - } - if parts.next().is_some() { + let port = if let Some(port) = port { + match u16::from_str(port) { + Ok(0) | Err(_) => { return Err(ErrorKind::InvalidArgument { message: format!( - "address \"{}\" contains more than one unescaped ':'", - address + "invalid server address {}: the port must be an integer between 1 and \ + 65535, got {}", + address, port ), } - .into()); + .into()) } - - Some(port) + Ok(port) => Some(port), } - None => None, + } else { + None }; - Ok(ServerAddress::Tcp { + Ok(Self::Tcp { host: hostname.to_lowercase(), port, }) @@ -1440,31 +1469,15 @@ impl ConnectionString { None => (None, None), }; - let mut host_list = Vec::with_capacity(hosts_section.len()); - for host in hosts_section.split(',') { - let address = if host.ends_with(".sock") { - #[cfg(unix)] - { - ServerAddress::parse(percent_decode( - host, - "Unix domain sockets must be URL-encoded", - )?) - } - #[cfg(not(unix))] - return Err(ErrorKind::InvalidArgument { - message: "Unix domain sockets are not supported on this platform".to_string(), - } - .into()); - } else { - ServerAddress::parse(host) - }?; - host_list.push(address); - } + let hosts = hosts_section + .split(',') + .map(ServerAddress::parse) + .collect::>>()?; let host_info = if !srv { - HostInfo::HostIdentifiers(host_list) + HostInfo::HostIdentifiers(hosts) } else { - match &host_list[..] { + match &hosts[..] { [ServerAddress::Tcp { host, port: None }] => HostInfo::DnsRecord(host.clone()), [ServerAddress::Tcp { host: _, diff --git a/src/client/options/test.rs b/src/client/options/test.rs index 863be8f93..3268c7774 100644 --- a/src/client/options/test.rs +++ b/src/client/options/test.rs @@ -9,7 +9,7 @@ use crate::{ bson::{Bson, Document}, bson_util::get_int, client::options::{ClientOptions, ConnectionString, ServerAddress}, - error::{Error, ErrorKind, Result}, + error::ErrorKind, test::spec::deserialize_spec_tests, Client, }; @@ -22,13 +22,6 @@ static SKIPPED_TESTS: Lazy> = Lazy::new(|| { "maxPoolSize=0 does not error", // TODO RUST-226: unskip this test "Valid tlsCertificateKeyFilePassword is parsed correctly", - // TODO RUST-229: unskip the following tests - "Single IP literal host without port", - "Single IP literal host with port", - "Multiple hosts (mixed formats)", - "User info for single IP literal host without database", - "User info for single IP literal host with database", - "User info for multiple hosts with database", ]; // TODO RUST-1896: unskip this test when openssl-tls is enabled @@ -65,43 +58,11 @@ struct TestCase { uri: String, valid: bool, warning: Option, - hosts: Option>, + hosts: Option>, auth: Option, options: Option, } -// The connection string tests' representation of a server address. We use this indirection to avoid -// deserialization failures when the tests specify an IPv6 address. -// -// TODO RUST-229: remove this struct and deserialize directly into ServerAddress -#[derive(Debug, Deserialize)] -struct TestServerAddress { - #[serde(rename = "type")] - host_type: String, - host: String, - port: Option, -} - -impl TryFrom<&TestServerAddress> for ServerAddress { - type Error = Error; - - fn try_from(test_server_address: &TestServerAddress) -> Result { - if test_server_address.host_type.as_str() == "ip_literal" { - return Err(ErrorKind::Internal { - message: "test using ip_literal host type should be skipped".to_string(), - } - .into()); - } - - let mut address = Self::parse(&test_server_address.host)?; - if let ServerAddress::Tcp { ref mut port, .. } = address { - *port = test_server_address.port; - } - - Ok(address) - } -} - #[derive(Debug, Deserialize)] #[serde(rename_all = "camelCase", deny_unknown_fields)] struct TestAuth { @@ -138,14 +99,8 @@ async fn run_tests(path: &[&str], skipped_files: &[&str]) { let client_options = client_options_result.expect(&test_case.description); if let Some(ref expected_hosts) = test_case.hosts { - let expected_hosts = expected_hosts - .iter() - .map(TryFrom::try_from) - .collect::>>() - .expect(&test_case.description); - assert_eq!( - client_options.hosts, expected_hosts, + &client_options.hosts, expected_hosts, "{}", test_case.description ); From 4abdc234523491199644be2ebe7609cb7f922504 Mon Sep 17 00:00:00 2001 From: Isabel Atkinson Date: Mon, 11 Nov 2024 15:49:46 -0700 Subject: [PATCH 2/5] add back unix error --- src/client/options.rs | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/client/options.rs b/src/client/options.rs index 06287f96b..c7fb678ff 100644 --- a/src/client/options.rs +++ b/src/client/options.rs @@ -212,10 +212,19 @@ impl ServerAddress { #[cfg(unix)] { if address.ends_with(".sock") { - let address = percent_decode(address, "unix domain sockets must be URL-encoded")?; - return Ok(Self::Unix { - path: PathBuf::from(address), - }); + #[cfg(unix)] + { + let address = + percent_decode(address, "unix domain sockets must be URL-encoded")?; + return Ok(Self::Unix { + path: PathBuf::from(address), + }); + } + #[cfg(not(unix))] + return Err(ErrorKind::InvalidArgument { + message: "unix domain sockets are not supported on this platform", + } + .into()); } } From d9851c2e6b5db32be5335efb7233e004d7f2d06d Mon Sep 17 00:00:00 2001 From: Isabel Atkinson Date: Tue, 12 Nov 2024 09:01:39 -0700 Subject: [PATCH 3/5] test invalid ipv6 --- src/client/options.rs | 10 +++++++++- src/client/options/test.rs | 30 ++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/src/client/options.rs b/src/client/options.rs index c7fb678ff..ee7118d0b 100644 --- a/src/client/options.rs +++ b/src/client/options.rs @@ -11,6 +11,7 @@ use std::{ convert::TryFrom, fmt::{self, Display, Formatter, Write}, hash::{Hash, Hasher}, + net::Ipv6Addr, path::PathBuf, str::FromStr, time::Duration, @@ -205,7 +206,7 @@ impl FromStr for ServerAddress { } impl ServerAddress { - /// Parses an address string into a [ServerAddress]. + /// Parses an address string into a [`ServerAddress`]. pub fn parse(address: impl AsRef) -> Result { let address = address.as_ref(); @@ -239,6 +240,13 @@ impl ServerAddress { .into()); }; + if let Err(parse_error) = Ipv6Addr::from_str(hostname) { + return Err(ErrorKind::InvalidArgument { + message: format!("invalid server address {}: {}", address, parse_error), + } + .into()); + } + let port = if port.is_empty() { None } else if let Some(port) = port.strip_prefix(":") { diff --git a/src/client/options/test.rs b/src/client/options/test.rs index 3268c7774..a744d70a5 100644 --- a/src/client/options/test.rs +++ b/src/client/options/test.rs @@ -319,3 +319,33 @@ async fn options_enforce_min_heartbeat_frequency() { Client::with_options(options).unwrap_err(); } + +#[test] +fn invalid_ipv6() { + // invalid hostname for ipv6 + let address = "[localhost]:27017"; + let error = ServerAddress::parse(address).unwrap_err(); + let message = error.message().unwrap(); + assert!(message.contains("invalid IPv6 address syntax"), "{message}"); + + // invalid character after hostname + let address = "[::1]a"; + let error = ServerAddress::parse(address).unwrap_err(); + let message = error.message().unwrap(); + assert!( + message.contains("the hostname can only be followed by a port"), + "{message}" + ); + + // missing bracket + let address = "[::1:27017"; + let error = ServerAddress::parse(address).unwrap_err(); + let message = error.message().unwrap(); + assert!(message.contains("missing closing ']'"), "{message}"); + + // extraneous bracket + let address = "[::1]:27017]"; + let error = ServerAddress::parse(address).unwrap_err(); + let message = error.message().unwrap(); + assert!(message.contains("the port must be an integer"), "{message}"); +} From 52da3e3c8d410ae9d1d5b814ef762935ec7d0465 Mon Sep 17 00:00:00 2001 From: Isabel Atkinson Date: Tue, 12 Nov 2024 09:31:27 -0700 Subject: [PATCH 4/5] fix cfg --- src/client/options.rs | 28 ++++++++++++---------------- src/client/options/test.rs | 12 ++++++++++++ 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/src/client/options.rs b/src/client/options.rs index ee7118d0b..346ebcd26 100644 --- a/src/client/options.rs +++ b/src/client/options.rs @@ -210,23 +210,19 @@ impl ServerAddress { pub fn parse(address: impl AsRef) -> Result { let address = address.as_ref(); - #[cfg(unix)] - { - if address.ends_with(".sock") { - #[cfg(unix)] - { - let address = - percent_decode(address, "unix domain sockets must be URL-encoded")?; - return Ok(Self::Unix { - path: PathBuf::from(address), - }); - } - #[cfg(not(unix))] - return Err(ErrorKind::InvalidArgument { - message: "unix domain sockets are not supported on this platform", - } - .into()); + if address.ends_with(".sock") { + #[cfg(unix)] + { + let address = percent_decode(address, "unix domain sockets must be URL-encoded")?; + return Ok(Self::Unix { + path: PathBuf::from(address), + }); + } + #[cfg(not(unix))] + return Err(ErrorKind::InvalidArgument { + message: "unix domain sockets are not supported on this platform", } + .into()); } let (hostname, port) = if let Some(ip_literal) = address.strip_prefix("[") { diff --git a/src/client/options/test.rs b/src/client/options/test.rs index a744d70a5..3d1f4da9e 100644 --- a/src/client/options/test.rs +++ b/src/client/options/test.rs @@ -349,3 +349,15 @@ fn invalid_ipv6() { let message = error.message().unwrap(); assert!(message.contains("the port must be an integer"), "{message}"); } + +#[cfg(not(unix))] +#[test] +fn unix_domain_socket_not_allowed() { + let address = "address.sock"; + let error = ServerAddress::parse(address).unwrap_err(); + let message = error.message().unwrap(); + assert!( + message.contains("not supported on this platform"), + "{message}" + ); +} From da5ec56307648d254a3edb188911d52a71e04cc3 Mon Sep 17 00:00:00 2001 From: Isabel Atkinson Date: Tue, 12 Nov 2024 09:53:38 -0700 Subject: [PATCH 5/5] fix windows --- src/client/options.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/client/options.rs b/src/client/options.rs index 346ebcd26..c794bca02 100644 --- a/src/client/options.rs +++ b/src/client/options.rs @@ -220,7 +220,7 @@ impl ServerAddress { } #[cfg(not(unix))] return Err(ErrorKind::InvalidArgument { - message: "unix domain sockets are not supported on this platform", + message: "unix domain sockets are not supported on this platform".to_string(), } .into()); } @@ -1207,6 +1207,7 @@ impl ClientOptions { .iter() .filter_map(|addr| match addr { ServerAddress::Tcp { host, .. } => Some(host.to_ascii_lowercase()), + #[cfg(unix)] _ => None, }) .collect()