From 0b56589dce90d2ee2f551a421b9a4efdd606ab6a Mon Sep 17 00:00:00 2001 From: Gabriel Souza Franco Date: Thu, 15 Apr 2021 22:07:27 -0300 Subject: [PATCH 1/5] feat: add handling of tls cert for delegated hosts --- CROSS_COMPILE.md | 2 +- Cargo.lock | 151 +++++++--------------------------------- Cargo.toml | 7 +- src/database/globals.rs | 40 ++++++++++- src/server_server.rs | 10 +++ 5 files changed, 82 insertions(+), 128 deletions(-) diff --git a/CROSS_COMPILE.md b/CROSS_COMPILE.md index 4728768c..366fd23c 100644 --- a/CROSS_COMPILE.md +++ b/CROSS_COMPILE.md @@ -5,6 +5,6 @@ $ sudo usermod -aG docker $USER $ exec sudo su -l $USER $ sudo systemctl start docker $ cargo install cross -$ cross build --release --features tls_vendored --target armv7-unknown-linux-musleabihf +$ cross build --release --target armv7-unknown-linux-musleabihf ``` The cross-compiled binary is at target/armv7-unknown-linux-musleabihf/release/conduit diff --git a/Cargo.lock b/Cargo.lock index c5c79b8e..84001dcc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -198,6 +198,8 @@ dependencies = [ "rocket", "ruma", "rust-argon2", + "rustls", + "rustls-native-certs", "serde", "serde_json", "serde_yaml", @@ -209,6 +211,7 @@ dependencies = [ "tracing-opentelemetry", "tracing-subscriber", "trust-dns-resolver", + "webpki", ] [[package]] @@ -420,21 +423,6 @@ version = "1.0.7" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3f9eec918d3f24069decb9af1554cad7c880e2da24a9afd88aca000531ab82c1" -[[package]] -name = "foreign-types" -version = "0.3.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f6f339eb8adc052cd2ca78910fda869aefa38d22d5cb648e6485e4d3fc06f3b1" -dependencies = [ - "foreign-types-shared", -] - -[[package]] -name = "foreign-types-shared" -version = "0.1.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "00b0228411908ca8685dba7fc2cdd70ec9990a6e753e89b6ac91a84c40fbaf4b" - [[package]] name = "form_urlencoded" version = "1.0.1" @@ -716,16 +704,18 @@ dependencies = [ ] [[package]] -name = "hyper-tls" -version = "0.5.0" +name = "hyper-rustls" +version = "0.22.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d6183ddfa99b85da61a140bea0efc93fdf56ceaa041b37d553518030827f9905" +checksum = "5f9f7a97316d44c0af9b0301e65010573a853a9fc97046d7331d7f6bc0fd5a64" dependencies = [ - "bytes", + "futures-util", "hyper", - "native-tls", + "log", + "rustls", "tokio", - "tokio-native-tls", + "tokio-rustls", + "webpki", ] [[package]] @@ -1001,24 +991,6 @@ dependencies = [ "winapi", ] -[[package]] -name = "native-tls" -version = "0.2.7" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b8d96b2e1c8da3957d58100b09f102c6d9cfdfced01b7ec5a8974044bb09dbd4" -dependencies = [ - "lazy_static", - "libc", - "log", - "openssl", - "openssl-probe", - "openssl-sys", - "schannel", - "security-framework", - "security-framework-sys", - "tempfile", -] - [[package]] name = "ntapi" version = "0.3.6" @@ -1096,49 +1068,12 @@ version = "1.7.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "af8b08b04175473088b46763e51ee54da5f9a164bc162f615b91bc179dbf15a3" -[[package]] -name = "openssl" -version = "0.10.33" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a61075b62a23fef5a29815de7536d940aa35ce96d18ce0cc5076272db678a577" -dependencies = [ - "bitflags", - "cfg-if", - "foreign-types", - "libc", - "once_cell", - "openssl-sys", -] - [[package]] name = "openssl-probe" version = "0.1.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "77af24da69f9d9341038eba93a073b1fdaaa1b788221b00a69bce9e762cb32de" -[[package]] -name = "openssl-src" -version = "111.15.0+1.1.1k" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b1a5f6ae2ac04393b217ea9f700cd04fa9bf3d93fae2872069f3d15d908af70a" -dependencies = [ - "cc", -] - -[[package]] -name = "openssl-sys" -version = "0.9.61" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "313752393519e876837e09e1fa183ddef0be7735868dced3196f4472d536277f" -dependencies = [ - "autocfg", - "cc", - "libc", - "openssl-src", - "pkg-config", - "vcpkg", -] - [[package]] name = "opentelemetry" version = "0.12.0" @@ -1280,12 +1215,6 @@ version = "0.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8b870d8c151b6f2fb93e84a13146138f05d02ed11c7e7c54f8826aaaf7c9f184" -[[package]] -name = "pkg-config" -version = "0.3.19" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3831453b3449ceb48b6d9c7ad7c96d5ea673e9b470a1dc578c2ce6521230884c" - [[package]] name = "png" version = "0.16.8" @@ -1486,15 +1415,6 @@ version = "0.6.23" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "24d5f089152e60f62d28b835fbff2cd2e8dc0baf1ac13343bef92ab7eed84548" -[[package]] -name = "remove_dir_all" -version = "0.5.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3acd125665422973a33ac9d3dd2df85edad0f4ae9b00dafb1a05e43a9f5ef8e7" -dependencies = [ - "winapi", -] - [[package]] name = "reqwest" version = "0.11.3" @@ -1509,19 +1429,20 @@ dependencies = [ "http", "http-body", "hyper", - "hyper-tls", + "hyper-rustls", "ipnet", "js-sys", "lazy_static", "log", "mime", - "native-tls", "percent-encoding", "pin-project-lite", + "rustls", + "rustls-native-certs", "serde", "serde_urlencoded", "tokio", - "tokio-native-tls", + "tokio-rustls", "url", "wasm-bindgen", "wasm-bindgen-futures", @@ -1886,6 +1807,18 @@ dependencies = [ "webpki", ] +[[package]] +name = "rustls-native-certs" +version = "0.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5a07b7c1885bd8ed3831c289b7870b13ef46fe0e856d288c30d9cc17d75a2092" +dependencies = [ + "openssl-probe", + "rustls", + "schannel", + "security-framework", +] + [[package]] name = "ryu" version = "1.0.5" @@ -2191,20 +2124,6 @@ dependencies = [ "unicode-xid", ] -[[package]] -name = "tempfile" -version = "3.2.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "dac1c663cfc93810f88aed9b8941d48cabf856a1b111c29a40439018d870eb22" -dependencies = [ - "cfg-if", - "libc", - "rand", - "redox_syscall 0.2.5", - "remove_dir_all", - "winapi", -] - [[package]] name = "termcolor" version = "1.1.2" @@ -2358,16 +2277,6 @@ dependencies = [ "syn", ] -[[package]] -name = "tokio-native-tls" -version = "0.3.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f7d995660bd2b7f8c1568414c1126076c13fbb725c40112dc0120b78eb9b717b" -dependencies = [ - "native-tls", - "tokio", -] - [[package]] name = "tokio-rustls" version = "0.22.0" @@ -2613,12 +2522,6 @@ dependencies = [ "percent-encoding", ] -[[package]] -name = "vcpkg" -version = "0.2.11" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b00bca6106a5e23f3eee943593759b7fcddb00554332e856d990c893966879fb" - [[package]] name = "version_check" version = "0.9.3" diff --git a/Cargo.toml b/Cargo.toml index 57b6f7fe..fa495dfb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -49,7 +49,11 @@ rand = "0.8.3" # Used to hash passwords rust-argon2 = "0.8.3" # Used to send requests -reqwest = { version = "0.11.1" } +reqwest = { version = "0.11.3", default-features = false, features = ["rustls-tls-native-roots"] } +# Custom TLS verifier +rustls = { version = "0.19", features = ["dangerous_configuration"] } +rustls-native-certs = "0.5.0" +webpki = "0.21.0" # Used for conduit::Error type thiserror = "1.0.24" # Used to generate thumbnails for images @@ -75,7 +79,6 @@ pretty_env_logger = "0.4.0" [features] default = ["conduit_bin"] conduit_bin = [] # TODO: add rocket to this when it is optional -tls_vendored = ["reqwest/native-tls-vendored"] [[bin]] name = "conduit" diff --git a/src/database/globals.rs b/src/database/globals.rs index bad9c89e..e48c8491 100644 --- a/src/database/globals.rs +++ b/src/database/globals.rs @@ -10,13 +10,16 @@ use std::{ time::Duration, }; use trust_dns_resolver::TokioAsyncResolver; +use rustls::{ServerCertVerifier, WebPKIVerifier}; pub const COUNTER: &str = "c"; type WellKnownMap = HashMap, (String, String)>; +type TlsNameMap = HashMap; #[derive(Clone)] pub struct Globals { pub actual_destination_cache: Arc>, // actual_destination, host + pub tls_name_override: Arc>, pub(super) globals: sled::Tree, config: Config, keypair: Arc, @@ -26,6 +29,33 @@ pub struct Globals { pub(super) servertimeout_signingkey: sled::Tree, // ServerName + Timeout Timestamp -> algorithm:key + pubkey } +struct MatrixServerVerifier { + inner: WebPKIVerifier, + tls_name_override: Arc>, +} + +impl ServerCertVerifier for MatrixServerVerifier { + fn verify_server_cert( + &self, + roots: &rustls::RootCertStore, + presented_certs: &[rustls::Certificate], + dns_name: webpki::DNSNameRef<'_>, + ocsp_response: &[u8], + ) -> std::result::Result { + let cache = self.tls_name_override.read().unwrap(); + log::debug!("Searching for override for {:?}", dns_name); + log::debug!("Cache: {:?}", cache); + let override_name = match cache.get(dns_name.into()) { + Some(host) => { + log::debug!("Override found! {:?}", host); + host.as_ref() + }, + None => dns_name + }; + self.inner.verify_server_cert(roots, presented_certs, override_name, ocsp_response) + } +} + impl Globals { pub fn load( globals: sled::Tree, @@ -66,10 +96,17 @@ impl Globals { } }; + let tls_name_override = Arc::new(RwLock::new(TlsNameMap::new())); + let verifier = Arc::new(MatrixServerVerifier { inner: WebPKIVerifier::new(), tls_name_override: tls_name_override.clone() }); + let mut tlsconfig = rustls::ClientConfig::new(); + tlsconfig.dangerous().set_certificate_verifier(verifier); + tlsconfig.root_store = rustls_native_certs::load_native_certs().expect("Error loading system certificates"); + let reqwest_client = reqwest::Client::builder() .connect_timeout(Duration::from_secs(30)) .timeout(Duration::from_secs(60 * 3)) .pool_max_idle_per_host(1) + .use_preconfigured_tls(tlsconfig) .build() .unwrap(); @@ -86,7 +123,8 @@ impl Globals { dns_resolver: TokioAsyncResolver::tokio_from_system_conf().map_err(|_| { Error::bad_config("Failed to set up trust dns resolver with system config.") })?, - actual_destination_cache: Arc::new(RwLock::new(HashMap::new())), + actual_destination_cache: Arc::new(RwLock::new(WellKnownMap::new())), + tls_name_override, servertimeout_signingkey, jwt_decoding_key, }) diff --git a/src/server_server.rs b/src/server_server.rs index 5b49472a..1bee9d50 100644 --- a/src/server_server.rs +++ b/src/server_server.rs @@ -74,6 +74,16 @@ where .write() .unwrap() .insert(Box::::from(destination), result.clone()); + let actual_destination = result.0.strip_prefix("https://").unwrap().splitn(2, ':').next().unwrap(); + let host = result.1.splitn(2, ':').next().unwrap_or(&result.1); + if actual_destination != host { + globals.tls_name_override.write().unwrap().insert( + actual_destination.to_owned(), + webpki::DNSNameRef::try_from_ascii_str(&host) + .unwrap() + .to_owned(), + ); + } result }; From b4c001de2f921b78997407034bf54b83e10a2c3e Mon Sep 17 00:00:00 2001 From: Gabriel Souza Franco Date: Fri, 16 Apr 2021 00:27:26 -0300 Subject: [PATCH 2/5] chore: cleanup string-based code in find_actual_destination --- src/server_server.rs | 101 ++++++++++++++++++++++++++++++------------- 1 file changed, 70 insertions(+), 31 deletions(-) diff --git a/src/server_server.rs b/src/server_server.rs index 1bee9d50..e85532e7 100644 --- a/src/server_server.rs +++ b/src/server_server.rs @@ -45,6 +45,35 @@ use std::{ #[cfg(feature = "conduit_bin")] use rocket::{get, post, put}; +#[derive(Clone, Debug, PartialEq)] +enum FederationDestination { + Literal(SocketAddr), + Named(String, String), +} + +impl FederationDestination { + fn into_url(self) -> String { + match self { + Self::Literal(addr) => format!("https://{}", addr), + Self::Named(host, port) => format!("https://{}{}", host, port), + } + } + + fn into_uri(self) -> String { + match self { + Self::Literal(addr) => addr.to_string(), + Self::Named(host, ref port) => host + port, + } + } + + fn host(&self) -> String { + match &self { + Self::Literal(addr) => addr.ip().to_string(), + Self::Named(host, _) => host.clone() + } + } +} + #[tracing::instrument(skip(globals))] pub async fn send_request( globals: &crate::database::globals::Globals, @@ -69,17 +98,17 @@ where result } else { let result = find_actual_destination(globals, &destination).await; + let (actual_destination, host) = result.clone(); + let result = (result.0.into_url(), result.1.into_uri()); globals .actual_destination_cache .write() .unwrap() .insert(Box::::from(destination), result.clone()); - let actual_destination = result.0.strip_prefix("https://").unwrap().splitn(2, ':').next().unwrap(); - let host = result.1.splitn(2, ':').next().unwrap_or(&result.1); if actual_destination != host { globals.tls_name_override.write().unwrap().insert( - actual_destination.to_owned(), - webpki::DNSNameRef::try_from_ascii_str(&host) + actual_destination.host(), + webpki::DNSNameRef::try_from_ascii_str(&host.host()) .unwrap() .to_owned(), ); @@ -212,22 +241,23 @@ where } #[tracing::instrument] -fn get_ip_with_port(destination_str: String) -> Option { - if destination_str.parse::().is_ok() { - Some(destination_str) +fn get_ip_with_port(destination_str: &str) -> Option { + if let Ok(destination) = destination_str.parse::() { + Some(FederationDestination::Literal(destination)) } else if let Ok(ip_addr) = destination_str.parse::() { - Some(SocketAddr::new(ip_addr, 8448).to_string()) + Some(FederationDestination::Literal(SocketAddr::new(ip_addr, 8448))) } else { None } } #[tracing::instrument] -fn add_port_to_hostname(destination_str: String) -> String { - match destination_str.find(':') { - None => destination_str.to_owned() + ":8448", - Some(_) => destination_str.to_string(), - } +fn add_port_to_hostname(destination_str: &str) -> FederationDestination { + let (host, port) = match destination_str.find(':') { + None => (destination_str, ":8448"), + Some(pos) => destination_str.split_at(pos), + }; + FederationDestination::Named(host.to_string(), port.to_string()) } /// Returns: actual_destination, host header @@ -237,36 +267,37 @@ fn add_port_to_hostname(destination_str: String) -> String { async fn find_actual_destination( globals: &crate::database::globals::Globals, destination: &'_ ServerName, -) -> (String, String) { +) -> (FederationDestination, FederationDestination) { let destination_str = destination.as_str().to_owned(); - let mut host = destination_str.clone(); - let actual_destination = "https://".to_owned() - + &match get_ip_with_port(destination_str.clone()) { + let mut hostname = destination_str.clone(); + let actual_destination = match get_ip_with_port(&destination_str) { Some(host_port) => { // 1: IP literal with provided or default port host_port } None => { - if destination_str.find(':').is_some() { + if let Some(pos) = destination_str.find(':') { // 2: Hostname with included port - destination_str + let (host, port) = destination_str.split_at(pos); + FederationDestination::Named(host.to_string(), port.to_string()) } else { match request_well_known(globals, &destination.as_str()).await { // 3: A .well-known file is available Some(delegated_hostname) => { - host = delegated_hostname.clone(); - match get_ip_with_port(delegated_hostname.clone()) { + hostname = delegated_hostname.clone(); + match get_ip_with_port(&delegated_hostname) { Some(host_and_port) => host_and_port, // 3.1: IP literal in .well-known file None => { - if destination_str.find(':').is_some() { + if let Some(pos) = destination_str.find(':') { // 3.2: Hostname with port in .well-known file - destination_str + let (host, port) = destination_str.split_at(pos); + FederationDestination::Named(host.to_string(), port.to_string()) } else { match query_srv_record(globals, &delegated_hostname).await { // 3.3: SRV lookup successful Some(hostname) => hostname, // 3.4: No SRV records, just use the hostname from .well-known - None => add_port_to_hostname(delegated_hostname), + None => add_port_to_hostname(&delegated_hostname), } } } @@ -278,7 +309,7 @@ async fn find_actual_destination( // 4: SRV record found Some(hostname) => hostname, // 5: No SRV record found - None => add_port_to_hostname(destination_str.to_string()), + None => add_port_to_hostname(&destination_str), } } } @@ -286,24 +317,32 @@ async fn find_actual_destination( } }; - (actual_destination, host) + let hostname = get_ip_with_port(&hostname).unwrap_or_else(|| { + match hostname.find(':') { + Some(pos) => { + let (host, port) = hostname.split_at(pos); + FederationDestination::Named(host.to_string(), port.to_string()) + } + None => FederationDestination::Named(hostname, "".to_string()) + } + }); + (actual_destination, hostname) } #[tracing::instrument(skip(globals))] async fn query_srv_record( globals: &crate::database::globals::Globals, hostname: &'_ str, -) -> Option { +) -> Option { if let Ok(Some(host_port)) = globals .dns_resolver() .srv_lookup(format!("_matrix._tcp.{}", hostname)) .await .map(|srv| { srv.iter().next().map(|result| { - format!( - "{}:{}", - result.target().to_string().trim_end_matches('.'), - result.port().to_string() + FederationDestination::Named( + result.target().to_string().trim_end_matches('.').to_string(), + format!(":{}", result.port()) ) }) }) From e73de2317eda6eb565a3c43e24e5164eb739b750 Mon Sep 17 00:00:00 2001 From: Gabriel Souza Franco Date: Fri, 16 Apr 2021 00:36:35 -0300 Subject: [PATCH 3/5] fix: verify tls cert for non-conformant servers --- src/database/globals.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/database/globals.rs b/src/database/globals.rs index e48c8491..fd51e355 100644 --- a/src/database/globals.rs +++ b/src/database/globals.rs @@ -52,7 +52,11 @@ impl ServerCertVerifier for MatrixServerVerifier { }, None => dns_name }; - self.inner.verify_server_cert(roots, presented_certs, override_name, ocsp_response) + + self.inner.verify_server_cert(roots, presented_certs, override_name, ocsp_response).or_else(|_| { + log::warn!("Server is non-compliant, retrying with original name!"); + self.inner.verify_server_cert(roots, presented_certs, dns_name, ocsp_response) + }) } } From 7faa021ff5656570aae815739dd3bc44ce18b5b8 Mon Sep 17 00:00:00 2001 From: Gabriel Souza Franco Date: Fri, 16 Apr 2021 12:18:22 -0300 Subject: [PATCH 4/5] chore: code formatting and cleanup --- src/database/globals.rs | 41 +++++++------ src/server_server.rs | 129 +++++++++++++++++++++------------------- 2 files changed, 89 insertions(+), 81 deletions(-) diff --git a/src/database/globals.rs b/src/database/globals.rs index fd51e355..acecf02a 100644 --- a/src/database/globals.rs +++ b/src/database/globals.rs @@ -1,16 +1,16 @@ use crate::{database::Config, utils, Error, Result}; -use log::error; +use log::{error, info}; use ruma::{ api::federation::discovery::{ServerSigningKeys, VerifyKey}, ServerName, ServerSigningKeyId, }; +use rustls::{ServerCertVerifier, WebPKIVerifier}; use std::{ collections::{BTreeMap, HashMap}, sync::{Arc, RwLock}, time::Duration, }; use trust_dns_resolver::TokioAsyncResolver; -use rustls::{ServerCertVerifier, WebPKIVerifier}; pub const COUNTER: &str = "c"; @@ -42,21 +42,20 @@ impl ServerCertVerifier for MatrixServerVerifier { dns_name: webpki::DNSNameRef<'_>, ocsp_response: &[u8], ) -> std::result::Result { - let cache = self.tls_name_override.read().unwrap(); - log::debug!("Searching for override for {:?}", dns_name); - log::debug!("Cache: {:?}", cache); - let override_name = match cache.get(dns_name.into()) { - Some(host) => { - log::debug!("Override found! {:?}", host); - host.as_ref() - }, - None => dns_name - }; - - self.inner.verify_server_cert(roots, presented_certs, override_name, ocsp_response).or_else(|_| { - log::warn!("Server is non-compliant, retrying with original name!"); - self.inner.verify_server_cert(roots, presented_certs, dns_name, ocsp_response) - }) + if let Some(override_name) = self.tls_name_override.read().unwrap().get(dns_name.into()) { + let result = self.inner.verify_server_cert( + roots, + presented_certs, + override_name.as_ref(), + ocsp_response, + ); + if result.is_ok() { + return result; + } + info!("Server {:?} is non-compliant, retrying TLS verification with original name", dns_name); + } + self.inner + .verify_server_cert(roots, presented_certs, dns_name, ocsp_response) } } @@ -101,10 +100,14 @@ impl Globals { }; let tls_name_override = Arc::new(RwLock::new(TlsNameMap::new())); - let verifier = Arc::new(MatrixServerVerifier { inner: WebPKIVerifier::new(), tls_name_override: tls_name_override.clone() }); + let verifier = Arc::new(MatrixServerVerifier { + inner: WebPKIVerifier::new(), + tls_name_override: tls_name_override.clone(), + }); let mut tlsconfig = rustls::ClientConfig::new(); tlsconfig.dangerous().set_certificate_verifier(verifier); - tlsconfig.root_store = rustls_native_certs::load_native_certs().expect("Error loading system certificates"); + tlsconfig.root_store = + rustls_native_certs::load_native_certs().expect("Error loading system certificates"); let reqwest_client = reqwest::Client::builder() .connect_timeout(Duration::from_secs(30)) diff --git a/src/server_server.rs b/src/server_server.rs index e85532e7..ac38f4d6 100644 --- a/src/server_server.rs +++ b/src/server_server.rs @@ -46,13 +46,13 @@ use std::{ use rocket::{get, post, put}; #[derive(Clone, Debug, PartialEq)] -enum FederationDestination { +enum FedDest { Literal(SocketAddr), Named(String, String), } -impl FederationDestination { - fn into_url(self) -> String { +impl FedDest { + fn into_https_url(self) -> String { match self { Self::Literal(addr) => format!("https://{}", addr), Self::Named(host, port) => format!("https://{}{}", host, port), @@ -69,7 +69,7 @@ impl FederationDestination { fn host(&self) -> String { match &self { Self::Literal(addr) => addr.ip().to_string(), - Self::Named(host, _) => host.clone() + Self::Named(host, _) => host.clone(), } } } @@ -99,13 +99,13 @@ where } else { let result = find_actual_destination(globals, &destination).await; let (actual_destination, host) = result.clone(); - let result = (result.0.into_url(), result.1.into_uri()); + let result = (result.0.into_https_url(), result.1.into_uri()); globals .actual_destination_cache .write() .unwrap() .insert(Box::::from(destination), result.clone()); - if actual_destination != host { + if actual_destination.host() != host.host() { globals.tls_name_override.write().unwrap().insert( actual_destination.host(), webpki::DNSNameRef::try_from_ascii_str(&host.host()) @@ -241,23 +241,23 @@ where } #[tracing::instrument] -fn get_ip_with_port(destination_str: &str) -> Option { +fn get_ip_with_port(destination_str: &str) -> Option { if let Ok(destination) = destination_str.parse::() { - Some(FederationDestination::Literal(destination)) + Some(FedDest::Literal(destination)) } else if let Ok(ip_addr) = destination_str.parse::() { - Some(FederationDestination::Literal(SocketAddr::new(ip_addr, 8448))) + Some(FedDest::Literal(SocketAddr::new(ip_addr, 8448))) } else { None } } #[tracing::instrument] -fn add_port_to_hostname(destination_str: &str) -> FederationDestination { +fn add_port_to_hostname(destination_str: &str) -> FedDest { let (host, port) = match destination_str.find(':') { None => (destination_str, ":8448"), Some(pos) => destination_str.split_at(pos), }; - FederationDestination::Named(host.to_string(), port.to_string()) + FedDest::Named(host.to_string(), port.to_string()) } /// Returns: actual_destination, host header @@ -267,65 +267,66 @@ fn add_port_to_hostname(destination_str: &str) -> FederationDestination { async fn find_actual_destination( globals: &crate::database::globals::Globals, destination: &'_ ServerName, -) -> (FederationDestination, FederationDestination) { +) -> (FedDest, FedDest) { let destination_str = destination.as_str().to_owned(); let mut hostname = destination_str.clone(); let actual_destination = match get_ip_with_port(&destination_str) { - Some(host_port) => { - // 1: IP literal with provided or default port - host_port - } - None => { - if let Some(pos) = destination_str.find(':') { - // 2: Hostname with included port - let (host, port) = destination_str.split_at(pos); - FederationDestination::Named(host.to_string(), port.to_string()) - } else { - match request_well_known(globals, &destination.as_str()).await { - // 3: A .well-known file is available - Some(delegated_hostname) => { - hostname = delegated_hostname.clone(); - match get_ip_with_port(&delegated_hostname) { - Some(host_and_port) => host_and_port, // 3.1: IP literal in .well-known file - None => { - if let Some(pos) = destination_str.find(':') { - // 3.2: Hostname with port in .well-known file - let (host, port) = destination_str.split_at(pos); - FederationDestination::Named(host.to_string(), port.to_string()) - } else { - match query_srv_record(globals, &delegated_hostname).await { - // 3.3: SRV lookup successful - Some(hostname) => hostname, - // 3.4: No SRV records, just use the hostname from .well-known - None => add_port_to_hostname(&delegated_hostname), - } + Some(host_port) => { + // 1: IP literal with provided or default port + host_port + } + None => { + if let Some(pos) = destination_str.find(':') { + // 2: Hostname with included port + let (host, port) = destination_str.split_at(pos); + FedDest::Named(host.to_string(), port.to_string()) + } else { + match request_well_known(globals, &destination.as_str()).await { + // 3: A .well-known file is available + Some(delegated_hostname) => { + hostname = delegated_hostname.clone(); + match get_ip_with_port(&delegated_hostname) { + Some(host_and_port) => host_and_port, // 3.1: IP literal in .well-known file + None => { + if let Some(pos) = destination_str.find(':') { + // 3.2: Hostname with port in .well-known file + let (host, port) = destination_str.split_at(pos); + FedDest::Named(host.to_string(), port.to_string()) + } else { + match query_srv_record(globals, &delegated_hostname).await { + // 3.3: SRV lookup successful + Some(hostname) => hostname, + // 3.4: No SRV records, just use the hostname from .well-known + None => add_port_to_hostname(&delegated_hostname), } } } } - // 4: No .well-known or an error occured - None => { - match query_srv_record(globals, &destination_str).await { - // 4: SRV record found - Some(hostname) => hostname, - // 5: No SRV record found - None => add_port_to_hostname(&destination_str), - } + } + // 4: No .well-known or an error occured + None => { + match query_srv_record(globals, &destination_str).await { + // 4: SRV record found + Some(hostname) => hostname, + // 5: No SRV record found + None => add_port_to_hostname(&destination_str), } } } } - }; - - let hostname = get_ip_with_port(&hostname).unwrap_or_else(|| { - match hostname.find(':') { - Some(pos) => { - let (host, port) = hostname.split_at(pos); - FederationDestination::Named(host.to_string(), port.to_string()) - } - None => FederationDestination::Named(hostname, "".to_string()) } - }); + }; + + let hostname = if let Ok(addr) = hostname.parse::() { + FedDest::Literal(addr) + } else if let Ok(addr) = hostname.parse::() { + FedDest::Named(addr.to_string(), "".to_string()) + } else if let Some(pos) = hostname.find(':') { + let (host, port) = hostname.split_at(pos); + FedDest::Named(host.to_string(), port.to_string()) + } else { + FedDest::Named(hostname, "".to_string()) + }; (actual_destination, hostname) } @@ -333,16 +334,20 @@ async fn find_actual_destination( async fn query_srv_record( globals: &crate::database::globals::Globals, hostname: &'_ str, -) -> Option { +) -> Option { if let Ok(Some(host_port)) = globals .dns_resolver() .srv_lookup(format!("_matrix._tcp.{}", hostname)) .await .map(|srv| { srv.iter().next().map(|result| { - FederationDestination::Named( - result.target().to_string().trim_end_matches('.').to_string(), - format!(":{}", result.port()) + FedDest::Named( + result + .target() + .to_string() + .trim_end_matches('.') + .to_string(), + format!(":{}", result.port()), ) }) }) From ed253e236cff1cdab7bd18dc03a4046da4ca3c4f Mon Sep 17 00:00:00 2001 From: Gabriel Souza Franco Date: Wed, 21 Apr 2021 00:35:44 -0300 Subject: [PATCH 5/5] chore: document FedDest, fix tests --- src/server_server.rs | 62 ++++++++++++++++++++++++++++---------------- 1 file changed, 40 insertions(+), 22 deletions(-) diff --git a/src/server_server.rs b/src/server_server.rs index ac38f4d6..553f9449 100644 --- a/src/server_server.rs +++ b/src/server_server.rs @@ -45,6 +45,20 @@ use std::{ #[cfg(feature = "conduit_bin")] use rocket::{get, post, put}; +/// Wraps either an literal IP address plus port, or a hostname plus complement +/// (colon-plus-port if it was specified). +/// +/// Note: A `FedDest::Named` might contain an IP address in string form if there +/// was no port specified to construct a SocketAddr with. +/// +/// # Examples: +/// ```rust,ignore +/// FedDest::Literal("198.51.100.3:8448".parse()?); +/// FedDest::Literal("[2001:db8::4:5]:443".parse()?); +/// FedDest::Named("matrix.example.org".to_owned(), "".to_owned()); +/// FedDest::Named("matrix.example.org".to_owned(), ":8448".to_owned()); +/// FedDest::Named("198.51.100.5".to_owned(), "".to_owned()); +/// ``` #[derive(Clone, Debug, PartialEq)] enum FedDest { Literal(SocketAddr), @@ -52,21 +66,21 @@ enum FedDest { } impl FedDest { - fn into_https_url(self) -> String { + fn into_https_string(self) -> String { match self { Self::Literal(addr) => format!("https://{}", addr), Self::Named(host, port) => format!("https://{}{}", host, port), } } - fn into_uri(self) -> String { + fn into_uri_string(self) -> String { match self { Self::Literal(addr) => addr.to_string(), Self::Named(host, ref port) => host + port, } } - fn host(&self) -> String { + fn hostname(&self) -> String { match &self { Self::Literal(addr) => addr.ip().to_string(), Self::Named(host, _) => host.clone(), @@ -99,21 +113,23 @@ where } else { let result = find_actual_destination(globals, &destination).await; let (actual_destination, host) = result.clone(); - let result = (result.0.into_https_url(), result.1.into_uri()); + let result_string = (result.0.into_https_string(), result.1.into_uri_string()); globals .actual_destination_cache .write() .unwrap() - .insert(Box::::from(destination), result.clone()); - if actual_destination.host() != host.host() { + .insert(Box::::from(destination), result_string.clone()); + let dest_hostname = actual_destination.hostname(); + let host_hostname = host.hostname(); + if dest_hostname != host_hostname { globals.tls_name_override.write().unwrap().insert( - actual_destination.host(), - webpki::DNSNameRef::try_from_ascii_str(&host.host()) + dest_hostname, + webpki::DNSNameRef::try_from_ascii_str(&host_hostname) .unwrap() .to_owned(), ); } - result + result_string }; let mut http_request = request @@ -317,6 +333,8 @@ async fn find_actual_destination( } }; + // Can't use get_ip_with_port here because we don't want to add a port + // to an IP address if it wasn't specified let hostname = if let Ok(addr) = hostname.parse::() { FedDest::Literal(addr) } else if let Ok(addr) = hostname.parse::() { @@ -1743,45 +1761,45 @@ pub async fn fetch_required_signing_keys( #[cfg(test)] mod tests { - use super::{add_port_to_hostname, get_ip_with_port}; + use super::{FedDest, add_port_to_hostname, get_ip_with_port}; #[test] fn ips_get_default_ports() { assert_eq!( - get_ip_with_port(String::from("1.1.1.1")), - Some(String::from("1.1.1.1:8448")) + get_ip_with_port("1.1.1.1"), + Some(FedDest::Literal("1.1.1.1:8448".parse().unwrap())) ); assert_eq!( - get_ip_with_port(String::from("dead:beef::")), - Some(String::from("[dead:beef::]:8448")) + get_ip_with_port("dead:beef::"), + Some(FedDest::Literal("[dead:beef::]:8448".parse().unwrap())) ); } #[test] fn ips_keep_custom_ports() { assert_eq!( - get_ip_with_port(String::from("1.1.1.1:1234")), - Some(String::from("1.1.1.1:1234")) + get_ip_with_port("1.1.1.1:1234"), + Some(FedDest::Literal("1.1.1.1:1234".parse().unwrap())) ); assert_eq!( - get_ip_with_port(String::from("[dead::beef]:8933")), - Some(String::from("[dead::beef]:8933")) + get_ip_with_port("[dead::beef]:8933"), + Some(FedDest::Literal("[dead::beef]:8933".parse().unwrap())) ); } #[test] fn hostnames_get_default_ports() { assert_eq!( - add_port_to_hostname(String::from("example.com")), - "example.com:8448" + add_port_to_hostname("example.com"), + FedDest::Named(String::from("example.com"), String::from(":8448")) ) } #[test] fn hostnames_keep_custom_ports() { assert_eq!( - add_port_to_hostname(String::from("example.com:1337")), - "example.com:1337" + add_port_to_hostname("example.com:1337"), + FedDest::Named(String::from("example.com"), String::from(":1337")) ) } }