diff --git a/crates/slipstream-client/src/dns/poll.rs b/crates/slipstream-client/src/dns/poll.rs index b28ceb41..d0436636 100644 --- a/crates/slipstream-client/src/dns/poll.rs +++ b/crates/slipstream-client/src/dns/poll.rs @@ -1,5 +1,5 @@ use crate::error::ClientError; -use slipstream_dns::{build_qname, encode_query, QueryParams, CLASS_IN, RR_TXT}; +use slipstream_dns::{build_qname_with_limit, encode_query, QueryParams, CLASS_IN, RR_TXT}; use slipstream_ffi::picoquic::{ picoquic_cnx_t, picoquic_current_time, picoquic_prepare_packet_ex, slipstream_request_poll, }; @@ -85,8 +85,12 @@ pub(crate) async fn send_poll_queries( resolver.debug.polls_sent = resolver.debug.polls_sent.saturating_add(1); let poll_id = *dns_id; - let qname = build_qname(&send_buf[..send_length], config.domain) - .map_err(|err| ClientError::new(err.to_string()))?; + let qname = build_qname_with_limit( + &send_buf[..send_length], + config.domain, + config.max_qname_len, + ) + .map_err(|err| ClientError::new(err.to_string()))?; let params = QueryParams { id: poll_id, qname: &qname, diff --git a/crates/slipstream-client/src/main.rs b/crates/slipstream-client/src/main.rs index 929b6715..5ee53a73 100644 --- a/crates/slipstream-client/src/main.rs +++ b/crates/slipstream-client/src/main.rs @@ -47,6 +47,13 @@ struct Args { gso: bool, #[arg(long = "domain", short = 'd', value_parser = parse_domain)] domain: String, + #[arg( + long = "max-qname-len", + value_name = "LEN", + default_value_t = 253, + value_parser = parse_max_qname_len + )] + max_qname_len: usize, #[arg(long = "cert", value_name = "PATH")] cert: Option, #[arg(long = "keep-alive-interval", short = 't', default_value_t = 400)] @@ -72,6 +79,7 @@ fn main() { congestion_control: args.congestion_control.as_deref(), gso: args.gso, domain: &args.domain, + max_qname_len: args.max_qname_len, cert: args.cert.as_deref(), keep_alive_interval: args.keep_alive_interval as usize, debug_poll: args.debug_poll, @@ -109,6 +117,16 @@ fn parse_resolver(input: &str) -> Result { parse_host_port(input, 53, AddressKind::Resolver).map_err(|err| err.to_string()) } +fn parse_max_qname_len(input: &str) -> Result { + let value: usize = input + .parse() + .map_err(|_| "Max QNAME length must be a positive integer".to_string())?; + if !(1..=253).contains(&value) { + return Err("Max QNAME length must be between 1 and 253".to_string()); + } + Ok(value) +} + fn build_resolvers(matches: &clap::ArgMatches) -> Result, String> { let mut ordered = Vec::new(); collect_resolvers(matches, "resolver", ResolverMode::Recursive, &mut ordered)?; @@ -151,6 +169,33 @@ fn collect_resolvers( mod tests { use super::*; + #[test] + fn max_qname_len_defaults_to_253() { + let args = Args::try_parse_from([ + "slipstream-client", + "--resolver", + "1.1.1.1", + "--domain", + "example.com", + ]) + .expect("parse args"); + assert_eq!(args.max_qname_len, 253); + } + + #[test] + fn max_qname_len_rejects_out_of_range() { + let result = Args::try_parse_from([ + "slipstream-client", + "--resolver", + "1.1.1.1", + "--domain", + "example.com", + "--max-qname-len", + "254", + ]); + assert!(result.is_err()); + } + #[test] fn preserves_ordered_resolvers() { let matches = Args::command() diff --git a/crates/slipstream-client/src/runtime.rs b/crates/slipstream-client/src/runtime.rs index 291d5dd4..54443e51 100644 --- a/crates/slipstream-client/src/runtime.rs +++ b/crates/slipstream-client/src/runtime.rs @@ -17,7 +17,7 @@ use crate::pinning::configure_pinned_certificate; use crate::streams::{ client_callback, drain_commands, drain_stream_data, handle_command, spawn_acceptor, ClientState, }; -use slipstream_dns::{build_qname, encode_query, QueryParams, CLASS_IN, RR_TXT}; +use slipstream_dns::{build_qname_with_limit, encode_query, QueryParams, CLASS_IN, RR_TXT}; use slipstream_ffi::{ configure_quic_with_custom, picoquic::{ @@ -47,8 +47,7 @@ const DNS_WAKE_DELAY_MAX_US: i64 = 10_000_000; const DNS_POLL_SLICE_US: u64 = 50_000; pub async fn run_client(config: &ClientConfig<'_>) -> Result { - let domain_len = config.domain.len(); - let mtu = compute_mtu(domain_len)?; + let mtu = compute_mtu(config.domain, config.max_qname_len)?; let mut resolvers = resolve_resolvers(config.resolvers, mtu, config.debug_poll)?; if resolvers.is_empty() { return Err(ClientError::new("At least one resolver is required")); @@ -342,8 +341,12 @@ pub async fn run_client(config: &ClientConfig<'_>) -> Result { } } - let qname = build_qname(&send_buf[..send_length], config.domain) - .map_err(|err| ClientError::new(err.to_string()))?; + let qname = build_qname_with_limit( + &send_buf[..send_length], + config.domain, + config.max_qname_len, + ) + .map_err(|err| ClientError::new(err.to_string()))?; let params = QueryParams { id: dns_id, qname: &qname, diff --git a/crates/slipstream-client/src/runtime/setup.rs b/crates/slipstream-client/src/runtime/setup.rs index 0aad621a..856c4e8a 100644 --- a/crates/slipstream-client/src/runtime/setup.rs +++ b/crates/slipstream-client/src/runtime/setup.rs @@ -1,20 +1,17 @@ use crate::error::ClientError; +use slipstream_dns::max_payload_len_for_domain_with_limit; use std::net::{Ipv6Addr, SocketAddr, SocketAddrV6}; use tokio::net::UdpSocket as TokioUdpSocket; -pub(crate) fn compute_mtu(domain_len: usize) -> Result { - if domain_len >= 240 { +pub(crate) fn compute_mtu(domain: &str, max_qname_len: usize) -> Result { + let max_payload = max_payload_len_for_domain_with_limit(domain, max_qname_len) + .map_err(|err| ClientError::new(err.to_string()))?; + if max_payload == 0 { return Err(ClientError::new( - "Domain name is too long for DNS transport", + "Max QNAME length leaves no room for payload; adjust --max-qname-len or domain", )); } - let mtu = ((240.0 - domain_len as f64) / 1.6) as u32; - if mtu == 0 { - return Err(ClientError::new( - "MTU computed to zero; check domain length", - )); - } - Ok(mtu) + Ok(max_payload as u32) } pub(crate) async fn bind_udp_socket() -> Result { diff --git a/crates/slipstream-client/tests/max_qname_len_e2e.rs b/crates/slipstream-client/tests/max_qname_len_e2e.rs new file mode 100644 index 00000000..7bb35dac --- /dev/null +++ b/crates/slipstream-client/tests/max_qname_len_e2e.rs @@ -0,0 +1,92 @@ +use slipstream_dns::decode_query; +use std::net::UdpSocket; +use std::path::PathBuf; +use std::process::{Child, Command, Stdio}; +use std::time::{Duration, Instant}; + +struct ChildGuard { + child: Child, +} + +impl Drop for ChildGuard { + fn drop(&mut self) { + let _ = self.child.kill(); + let _ = self.child.wait(); + } +} + +fn bind_resolver_socket() -> std::io::Result<(UdpSocket, String)> { + if let Ok(socket) = UdpSocket::bind("[::1]:0") { + let port = socket.local_addr()?.port(); + return Ok((socket, format!("[::1]:{}", port))); + } + let socket = UdpSocket::bind("127.0.0.1:0")?; + let port = socket.local_addr()?.port(); + Ok((socket, format!("127.0.0.1:{}", port))) +} + +#[test] +fn max_qname_len_e2e() { + let domain = "example.com"; + let max_qname_len = 101usize; + let (socket, resolver) = match bind_resolver_socket() { + Ok(value) => value, + Err(err) => { + eprintln!("skipping max qname len e2e test: {}", err); + return; + } + }; + socket + .set_read_timeout(Some(Duration::from_secs(5))) + .expect("set UDP timeout"); + + let client_bin = PathBuf::from(env!("CARGO_BIN_EXE_slipstream-client")); + let child = Command::new(client_bin) + .arg("--tcp-listen-port") + .arg("0") + .arg("--resolver") + .arg(resolver) + .arg("--domain") + .arg(domain) + .arg("--max-qname-len") + .arg(max_qname_len.to_string()) + .env("RUST_LOG", "error") + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .expect("start slipstream-client"); + let _child_guard = ChildGuard { child }; + + let deadline = Instant::now() + Duration::from_secs(5); + let mut buf = [0u8; 2048]; + let mut observed_len = None; + + while Instant::now() < deadline { + match socket.recv_from(&mut buf) { + Ok((size, _)) => { + if let Ok(decoded) = decode_query(&buf[..size], domain) { + let qname_len = decoded.question.name.trim_end_matches('.').len(); + observed_len = Some(qname_len); + break; + } + } + Err(err) + if matches!( + err.kind(), + std::io::ErrorKind::WouldBlock | std::io::ErrorKind::TimedOut + ) => + { + continue; + } + Err(err) => panic!("failed to read UDP query: {}", err), + } + } + + let observed_len = observed_len.expect("no DNS query captured from client"); + assert!( + observed_len <= max_qname_len, + "observed QNAME length {} exceeds limit {}", + observed_len, + max_qname_len + ); +} diff --git a/crates/slipstream-dns/src/bin/bench_dns.rs b/crates/slipstream-dns/src/bin/bench_dns.rs index 81ac3395..4e6b90eb 100644 --- a/crates/slipstream-dns/src/bin/bench_dns.rs +++ b/crates/slipstream-dns/src/bin/bench_dns.rs @@ -1,6 +1,6 @@ use slipstream_dns::{ - build_qname, decode_query, decode_response, encode_query, encode_response, - max_payload_len_for_domain, QueryParams, Question, ResponseParams, CLASS_IN, RR_TXT, + build_qname_with_limit, decode_query, decode_response, encode_query, encode_response, + max_payload_len_for_domain_with_limit, QueryParams, Question, ResponseParams, CLASS_IN, RR_TXT, }; use std::env; use std::time::Instant; @@ -12,6 +12,7 @@ fn main() { let mut iterations = 10_000usize; let mut payload_len = 256usize; let mut domain = "test.com".to_string(); + let mut max_qname_len = 253usize; for arg in env::args().skip(1) { if let Some(value) = arg.strip_prefix("--iterations=") { @@ -20,13 +21,25 @@ fn main() { payload_len = value.parse().unwrap_or(payload_len); } else if let Some(value) = arg.strip_prefix("--domain=") { domain = value.to_string(); + } else if let Some(value) = arg.strip_prefix("--max-qname-len=") { + match value.parse::() { + Ok(value) if (1..=253).contains(&value) => max_qname_len = value, + Ok(_) => { + error!("Max QNAME length must be between 1 and 253."); + std::process::exit(1); + } + Err(_) => { + error!("Max QNAME length must be a positive integer."); + std::process::exit(1); + } + } } else if arg == "--help" { print_usage(); return; } } - let max_payload = match max_payload_len_for_domain(&domain) { + let max_payload = match max_payload_len_for_domain_with_limit(&domain, max_qname_len) { Ok(limit) => limit, Err(err) => { error!("Invalid domain: {}", err); @@ -46,7 +59,7 @@ fn main() { } let payload: Vec = (0..payload_len).map(|i| (i % 256) as u8).collect(); - let qname = match build_qname(&payload, &domain) { + let qname = match build_qname_with_limit(&payload, &domain, max_qname_len) { Ok(name) => name, Err(err) => { error!("Failed to build qname: {}", err); @@ -81,7 +94,7 @@ fn main() { let response = encode_response(&response_params).expect("encode response"); bench("build_qname", iterations, payload_len, || { - let _ = build_qname(&payload, &domain).expect("build qname"); + let _ = build_qname_with_limit(&payload, &domain, max_qname_len).expect("build qname"); }); bench("encode_query", iterations, query.len(), || { let _ = encode_query(&query_params).expect("encode query"); @@ -121,7 +134,9 @@ fn bench(label: &str, iterations: usize, bytes_per_iter: usize, mut f: impl FnMu } fn print_usage() { - println!("Usage: bench_dns [--iterations=N] [--payload-len=N] [--domain=NAME]"); + println!( + "Usage: bench_dns [--iterations=N] [--payload-len=N] [--domain=NAME] [--max-qname-len=N]" + ); } fn init_logging() { diff --git a/crates/slipstream-dns/src/lib.rs b/crates/slipstream-dns/src/lib.rs index 950c4bbd..1d90158f 100644 --- a/crates/slipstream-dns/src/lib.rs +++ b/crates/slipstream-dns/src/lib.rs @@ -17,11 +17,23 @@ pub use types::{ }; pub fn build_qname(payload: &[u8], domain: &str) -> Result { + build_qname_with_limit(payload, domain, name::MAX_DNS_NAME_LEN) +} + +pub fn max_payload_len_for_domain(domain: &str) -> Result { + max_payload_len_for_domain_with_limit(domain, name::MAX_DNS_NAME_LEN) +} + +pub fn build_qname_with_limit( + payload: &[u8], + domain: &str, + max_name_len: usize, +) -> Result { let domain = domain.trim_end_matches('.'); if domain.is_empty() { return Err(DnsError::new("domain must not be empty")); } - let max_payload = max_payload_len_for_domain(domain)?; + let max_payload = max_payload_len_for_domain_with_limit(domain, max_name_len)?; if payload.len() > max_payload { return Err(DnsError::new("payload too large for domain")); } @@ -30,15 +42,23 @@ pub fn build_qname(payload: &[u8], domain: &str) -> Result { Ok(format!("{}.{}.", dotted, domain)) } -pub fn max_payload_len_for_domain(domain: &str) -> Result { +pub fn max_payload_len_for_domain_with_limit( + domain: &str, + max_name_len: usize, +) -> Result { let domain = domain.trim_end_matches('.'); if domain.is_empty() { return Err(DnsError::new("domain must not be empty")); } - if domain.len() > name::MAX_DNS_NAME_LEN { + if max_name_len == 0 { + return Err(DnsError::new("max name length must be positive")); + } + if max_name_len > name::MAX_DNS_NAME_LEN { + return Err(DnsError::new("max name length too long")); + } + if domain.len() > max_name_len { return Err(DnsError::new("domain too long")); } - let max_name_len = name::MAX_DNS_NAME_LEN; let max_dotted_len = max_name_len.saturating_sub(domain.len() + 1); if max_dotted_len == 0 { return Ok(0); @@ -68,7 +88,10 @@ fn base32_len(payload_len: usize) -> usize { #[cfg(test)] mod tests { - use super::{build_qname, max_payload_len_for_domain}; + use super::{ + build_qname, build_qname_with_limit, max_payload_len_for_domain, + max_payload_len_for_domain_with_limit, + }; #[test] fn build_qname_rejects_payload_overflow() { @@ -84,4 +107,30 @@ mod tests { let payload = vec![0u8; 1]; assert!(build_qname(&payload, &domain).is_err()); } + + #[test] + fn max_payload_len_with_limit_bounds_payload() { + let domain = "test.com"; + let max_name_len = domain.len() + 3; + let max_payload = + max_payload_len_for_domain_with_limit(domain, max_name_len).expect("max payload"); + assert_eq!(max_payload, 1); + } + + #[test] + fn build_qname_with_limit_rejects_payload_overflow() { + let domain = "test.com"; + let max_name_len = domain.len() + 3; + let payload = vec![0u8; 2]; + assert!(build_qname_with_limit(&payload, domain, max_name_len).is_err()); + } + + #[test] + fn build_qname_with_limit_respects_name_len() { + let domain = "test.com"; + let max_name_len = domain.len() + 3; + let payload = vec![0u8; 1]; + let qname = build_qname_with_limit(&payload, domain, max_name_len).expect("build qname"); + assert!(qname.trim_end_matches('.').len() <= max_name_len); + } } diff --git a/crates/slipstream-ffi/src/lib.rs b/crates/slipstream-ffi/src/lib.rs index 38ae2b12..b78310dd 100644 --- a/crates/slipstream-ffi/src/lib.rs +++ b/crates/slipstream-ffi/src/lib.rs @@ -24,6 +24,7 @@ pub struct ClientConfig<'a> { pub tcp_listen_port: u16, pub resolvers: &'a [ResolverSpec], pub domain: &'a str, + pub max_qname_len: usize, pub cert: Option<&'a str>, pub congestion_control: Option<&'a str>, pub gso: bool, diff --git a/docs/dns-codec.md b/docs/dns-codec.md index ee7eebdc..19634532 100644 --- a/docs/dns-codec.md +++ b/docs/dns-codec.md @@ -12,7 +12,7 @@ This document captures the DNS codec behavior and how it is validated. - Server decode rules: - QR=1 or QDCOUNT!=1 -> FORMAT_ERROR. - QTYPE!=TXT -> NAME_ERROR. - - Empty subdomain or suffix mismatch -> NAME_ERROR. + - Empty subdomain (including empty payload labels) or suffix mismatch -> NAME_ERROR. - If multiple suffixes match, use the longest matching domain. - Base32 decode failure -> SERVER_FAILURE. - Parse errors -> drop the message (no response). @@ -49,6 +49,7 @@ This validates query/response encoding, error behavior, and raw packet drop case The Rust CLI enforces the following constraints: - Client requires --domain and at least one --resolver. +- Client --max-qname-len defaults to 253 and must be between 1 and 253. - Resolver parsing supports IPv4, bracketed IPv6, and optional :port. - Resolver lists may mix IPv4 and IPv6 entries. - Server requires at least one --domain (repeatable); --target-address defaults to 127.0.0.1:5201. diff --git a/docs/protocol.md b/docs/protocol.md index 7a452173..680eec0c 100644 --- a/docs/protocol.md +++ b/docs/protocol.md @@ -61,7 +61,7 @@ codec is intentionally minimal and focused on speed and compatibility. - If the DNS message is not a query (QR=1): respond with FORMAT_ERROR. - If QDCOUNT != 1: respond with FORMAT_ERROR. - If QTYPE != TXT: respond with NAME_ERROR (ignore query). -- If the QNAME subdomain is empty: respond with NAME_ERROR. +- If the QNAME payload labels are empty: respond with NAME_ERROR. - If base32 decode fails: respond with SERVER_FAILURE. - If the DNS parser fails (decode error): drop the message (no response). - The server must verify that QNAME ends with a configured domain suffix; if not, respond with NAME_ERROR. @@ -109,7 +109,10 @@ Otherwise, the response is ignored (including NAME_ERROR, which signals no data) - Inline dots ensure label length <= 57 chars. - EDNS0 is always included on outbound messages and advertises udp_payload=1232; incoming messages are accepted regardless of OPT presence. -- Client MTU is derived from the domain length: floor((240 - domain_len) / 1.6). +- Client max QNAME length defaults to 253 (excluding the trailing dot) and is + configurable via --max-qname-len. +- Client payload size is capped by the configured max QNAME length; the payload + budget accounts for base32 expansion and inline dots. - Server MTU is fixed at 900. ## References diff --git a/docs/usage.md b/docs/usage.md index 92f54f0d..5f58c6ad 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -17,6 +17,7 @@ Common flags: - --authoritative (repeatable; mark a resolver path as authoritative and use pacing-based polling) - --gso (currently not implemented in the Rust loop; prints a warning) - --keep-alive-interval (default: 400) +- --max-qname-len (default: 253; caps the total QNAME length for payload labels + "." + domain, excluding the trailing dot) Example: @@ -35,6 +36,7 @@ Notes: - IPv4 resolvers require an IPv6 dual-stack UDP socket (e.g., IPV6_V6ONLY=0 via OS defaults or sysctl). - Provide --cert to enable strict leaf pinning; omit it for legacy/no-verification behavior. - The pinned certificate must match the server leaf exactly; CA bundles are not supported. +- If you need to tighten DNS name length behavior, lower --max-qname-len (for example, 101). - Resolver order follows the CLI; the first resolver becomes path 0. - Resolver addresses must be unique; duplicates are rejected. - --authoritative keeps the DNS wire format unchanged and remains C interop safe.