From 6cb7896e1740c6fdf0ce2ad9e35563a70a43f579 Mon Sep 17 00:00:00 2001 From: Olivia Lee Date: Mon, 16 Dec 2024 01:12:06 -0800 Subject: [PATCH] use trust-dns for all DNS queries Previously we were only using trust-dns for resolving SRV records in server discovery, and then for resolving the hostname from the SRV record target if one exists. With the previous behavior, admins need to ensure that both their system resolver and trust-dns are working correctly in order for outgoing traffic to work reliably. This can be confusing to debug, because it's not obvious to the admin if or when each resolver are being used. Now, everything goes through trust-dns and outgoing federation DNS should fail/succeed more predictably. I also expect some performance improvement from having an in-process DNS cache, but haven't taken measurements yet. --- Cargo.lock | 1 - Cargo.toml | 1 - book/changelog.md | 3 ++ src/service/globals.rs | 82 +++++++++++++++++++++++++----------------- 4 files changed, 52 insertions(+), 35 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 59f2ff41..d257d117 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -929,7 +929,6 @@ dependencies = [ "http", "http-body-util", "hyper", - "hyper-util", "image", "insta", "jsonwebtoken", diff --git a/Cargo.toml b/Cargo.toml index 00971197..0cf41846 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -102,7 +102,6 @@ html-escape = "0.2.13" http = "1.3.1" http-body-util = "0.1.3" hyper = "1.6.0" -hyper-util = { version = "0.1.10", features = ["client", "client-legacy", "service"] } image = { version = "0.25.6", default-features = false, features = ["jpeg", "png", "gif"] } jsonwebtoken = "9.3.1" lru-cache = "0.1.2" diff --git a/book/changelog.md b/book/changelog.md index 7ed662b9..b0baf25f 100644 --- a/book/changelog.md +++ b/book/changelog.md @@ -150,6 +150,9 @@ This will be the first release of Grapevine since it was forked from Conduit database path. ([!140](https://gitlab.computer.surgery/matrix/grapevine/-/merge_requests/140), [!170](https://gitlab.computer.surgery/matrix/grapevine/-/merge_requests/170)) +14. Use trust-dns for all DNS queries, instead of only for SRV records and SRV + record targets in server discovery. + ([!156](https://gitlab.computer.surgery/matrix/grapevine/-/merge_requests/156)) ### Fixed diff --git a/src/service/globals.rs b/src/service/globals.rs index 0f42d9fc..ede9f5c6 100644 --- a/src/service/globals.rs +++ b/src/service/globals.rs @@ -1,6 +1,5 @@ use std::{ collections::{BTreeMap, HashMap}, - error::Error as StdError, fs, future::{self, Future}, iter, @@ -14,11 +13,6 @@ use std::{ }; use base64::{engine::general_purpose, Engine as _}; -use futures_util::FutureExt; -use hyper::service::Service as _; -use hyper_util::{ - client::legacy::connect::dns::GaiResolver, service::TowerToHyperService, -}; use reqwest::dns::{Addrs, Name, Resolve, Resolving}; use ruma::{ api::federation::discovery::ServerSigningKeys, @@ -141,6 +135,38 @@ impl Default for RotationHandler { } } +/// Wrapper around [`trust_dns_resolver`]'s [`TokioAsyncResolver`] that can be +/// used with reqwest. +pub(crate) struct DefaultResolver { + inner: Arc, +} + +impl DefaultResolver { + fn new(inner: Arc) -> Self { + DefaultResolver { + inner, + } + } + + fn resolve_inner(&self, name: Name) -> Resolving { + let inner = Arc::clone(&self.inner); + let future = async move { + let lookup = inner.lookup_ip(name.as_str()).await?; + let addrs: Addrs = + Box::new(lookup.into_iter().map(|ip| SocketAddr::new(ip, 0))); + Ok(addrs) + }; + Box::pin(future.in_current_span()) + } +} + +impl Resolve for DefaultResolver { + #[tracing::instrument(skip(self))] + fn resolve(&self, name: Name) -> Resolving { + self.resolve_inner(name) + } +} + /// Resolver used for outgoing requests to the federation API. /// /// Hostnames that have been mapped to a different domain by SRV records in @@ -150,14 +176,17 @@ impl Default for RotationHandler { /// /// [1]: https://spec.matrix.org/v1.12/server-server-api/#server-discovery pub(crate) struct FederationResolver { - inner: GaiResolver, + inner: Arc, overrides: Arc>, } impl FederationResolver { - pub(crate) fn new(overrides: Arc>) -> Self { + pub(crate) fn new( + inner: Arc, + overrides: Arc>, + ) -> Self { FederationResolver { - inner: GaiResolver::new(), + inner, overrides, } } @@ -181,26 +210,7 @@ impl Resolve for FederationResolver { x }) }) - .unwrap_or_else(|| { - // This should never fail because reqwest's type is a wrapper - // around hyper-utils' type - let name = name.as_str().parse().expect("name should be valid"); - - Box::pin( - TowerToHyperService::new(self.inner.clone()) - .call(name) - .map(|result| { - result - .map(|addrs| -> Addrs { Box::new(addrs) }) - .map_err( - |err| -> Box { - Box::new(err) - }, - ) - }) - .in_current_span(), - ) - }) + .unwrap_or_else(|| self.inner.resolve_inner(name)) } } @@ -235,16 +245,22 @@ impl Service { ) })?, ); + let default_resolver = + Arc::new(DefaultResolver::new(Arc::clone(&dns_resolver))); + let federation_resolver = Arc::new(FederationResolver::new( + Arc::clone(&default_resolver), + Arc::clone(&tls_name_override), + )); let jwt_decoding_key = config.jwt_secret.as_ref().map(|secret| { jsonwebtoken::DecodingKey::from_secret(secret.as_bytes()) }); - let default_client = reqwest_client_builder(&config)?.build()?; + let default_client = reqwest_client_builder(&config)? + .dns_resolver(default_resolver) + .build()?; let federation_client = reqwest_client_builder(&config)? - .dns_resolver(Arc::new(FederationResolver::new( - tls_name_override.clone(), - ))) + .dns_resolver(federation_resolver) .build()?; // Supported and stable room versions