From 21364f4ab021f71949b71af2dcbcfad1139c8a9b Mon Sep 17 00:00:00 2001 From: Ryan Goodfellow Date: Tue, 10 May 2022 13:58:03 -0700 Subject: [PATCH 1/2] internal-dns: NXDOMAIN/SERVFAIL where appropriate Currently when the internal DNS server encounters an error, or has no records to server it will just return a DNS response with no resource records. This causes issues for resolvers that expect: - A SERVFAIL for records the DNS server cannot resolve. - An NXDOMAIN for records the DNS server asserts do not exist. This patch updates the internal DNS server to conform to those expectations. This required adding a zone configuration option to the internal DNS server that identifies what domain it's presiding over. Any records that do not fall within that domain will result in a SERVFAIL, causing resolvers to look for other DNS servers for answers. This particular failure mode falls under the SERVFAIL DNS Error Code 7 [1] "No Reachable Authority" as the internal DNS server does not take responsibility for recursive resolution. [1] https://tools.ietf.org/id/draft-ietf-dnsop-extended-error-05.html#rfc.section.4.2.7 --- Cargo.lock | 1 + internal-dns/Cargo.toml | 1 + internal-dns/src/bin/apigen.rs | 2 +- internal-dns/src/bin/dns-server.rs | 5 ++ internal-dns/src/dns_server.rs | 55 +++++++++++++++---- internal-dns/tests/basic_test.rs | 87 ++++++++++++++++++++++++++++-- smf/internal-dns/manifest.xml | 3 +- 7 files changed, 138 insertions(+), 16 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 15f46dbe666..7f3987071c3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2135,6 +2135,7 @@ dependencies = [ "tempdir", "tokio", "toml", + "trust-dns-client", "trust-dns-proto", "trust-dns-resolver", "trust-dns-server", diff --git a/internal-dns/Cargo.toml b/internal-dns/Cargo.toml index 51db2a5f504..3f9bd86fa7c 100644 --- a/internal-dns/Cargo.toml +++ b/internal-dns/Cargo.toml @@ -24,6 +24,7 @@ tokio = { version = "1.18", features = [ "full" ] } toml = "0.5" trust-dns-proto = "0.21" trust-dns-server = "0.21" +trust-dns-client = "0.21" [dev-dependencies] expectorate = "1.0.5" diff --git a/internal-dns/src/bin/apigen.rs b/internal-dns/src/bin/apigen.rs index 095291c9571..b342f0b8233 100644 --- a/internal-dns/src/bin/apigen.rs +++ b/internal-dns/src/bin/apigen.rs @@ -7,7 +7,7 @@ use internal_dns::dropshot_server::api; use std::fs::File; use std::io; -fn usage(args: &Vec) -> String { +fn usage(args: &[String]) -> String { format!("{} [output path]", args[0]) } diff --git a/internal-dns/src/bin/dns-server.rs b/internal-dns/src/bin/dns-server.rs index b8cca5af301..6dc49133acc 100644 --- a/internal-dns/src/bin/dns-server.rs +++ b/internal-dns/src/bin/dns-server.rs @@ -27,6 +27,9 @@ struct Args { #[clap(long)] dns_address: SocketAddrV6, + + #[clap(long)] + dns_zone: String, } #[tokio::main] @@ -34,6 +37,7 @@ async fn main() -> Result<(), anyhow::Error> { let args = Args::parse(); let config_file = &args.config_file; let dns_address = &args.dns_address; + let zone = &args.dns_zone; let config_file_contents = std::fs::read_to_string(config_file) .with_context(|| format!("read config file {:?}", config_file))?; let mut config: internal_dns::Config = @@ -55,6 +59,7 @@ async fn main() -> Result<(), anyhow::Error> { let log = log.clone(); let dns_config = internal_dns::dns_server::Config { bind_address: dns_address.to_string(), + zone: zone.to_string(), }; tokio::spawn(async move { internal_dns::dns_server::run(log, db, dns_config).await diff --git a/internal-dns/src/dns_server.rs b/internal-dns/src/dns_server.rs index f6f5ed5209f..a2d26373dbc 100644 --- a/internal-dns/src/dns_server.rs +++ b/internal-dns/src/dns_server.rs @@ -12,7 +12,9 @@ use pretty_hex::*; use serde::Deserialize; use slog::{error, Logger}; use tokio::net::UdpSocket; +use trust_dns_client::rr::LowerName; use trust_dns_proto::op::header::Header; +use trust_dns_proto::op::response_code::ResponseCode; use trust_dns_proto::rr::rdata::SRV; use trust_dns_proto::rr::record_data::RData; use trust_dns_proto::rr::record_type::RecordType; @@ -27,6 +29,9 @@ use trust_dns_server::authority::{MessageRequest, MessageResponseBuilder}; pub struct Config { /// The address to listen for DNS requests on pub bind_address: String, + + /// The DNS zone this server presides over. This must be a valid DNS name + pub zone: String, } pub async fn run(log: Logger, db: Arc, config: Config) -> Result<()> { @@ -40,10 +45,11 @@ pub async fn run(log: Logger, db: Arc, config: Config) -> Result<()> { let socket = socket.clone(); let log = log.clone(); let db = db.clone(); + let zone = config.zone.clone(); - tokio::spawn( - async move { handle_req(log, db, socket, src, buf).await }, - ); + tokio::spawn(async move { + handle_req(log, db, socket, src, buf, zone).await + }); } } @@ -53,6 +59,7 @@ async fn handle_req<'a, 'b, 'c>( socket: Arc, src: SocketAddr, buf: Vec, + zone: String, ) { println!("{:?}", buf.hex_dump()); @@ -67,21 +74,51 @@ async fn handle_req<'a, 'b, 'c>( println!("{:#?}", mr); - let rb = MessageResponseBuilder::from_message_request(&mr); let header = Header::response_from_request(mr.header()); + let zone = LowerName::from(Name::from_str(&zone).unwrap()); + + // Ensure the query is for this zone, otherwise bail with servfail. This + // will cause resolvers to look to other DNS servers for this query. + let name = mr.query().name(); + if !zone.zone_of(name) { + nack(&log, &mr, &socket, &header, &src).await; + return; + } let name = mr.query().original().name().clone(); let key = name.to_string(); let key = key.trim_end_matches('.'); + let rb = MessageResponseBuilder::from_message_request(&mr); + let bits = match db.get(key.as_bytes()) { Ok(Some(bits)) => bits, - Err(e) => { - error!(log, "db get: {}", e); - nack(&log, &mr, &socket, &header, &src).await; + + // If no record is found bail with NXDOMAIN. + Ok(None) => { + let mresp = rb.error_msg(&header, ResponseCode::NXDomain); + let mut resp_data = Vec::new(); + let mut enc = BinEncoder::new(&mut resp_data); + match mresp.destructive_emit(&mut enc) { + Ok(_) => {} + Err(e) => { + error!(log, "NXDOMAIN destructive emit: {}", e); + nack(&log, &mr, &socket, &header, &src).await; + return; + } + } + match socket.send_to(&resp_data, &src).await { + Ok(_) => {} + Err(e) => { + error!(log, "NXDOMAIN send: {}", e); + } + } return; } - _ => { + + // If we encountered an error bail with SERVFAIL. + Err(e) => { + error!(log, "db get: {}", e); nack(&log, &mr, &socket, &header, &src).await; return; } @@ -166,7 +203,7 @@ async fn nack( src: &SocketAddr, ) { let rb = MessageResponseBuilder::from_message_request(mr); - let mresp = rb.build_no_records(*header); + let mresp = rb.error_msg(header, ResponseCode::ServFail); let mut resp_data = Vec::new(); let mut enc = BinEncoder::new(&mut resp_data); match mresp.destructive_emit(&mut enc) { diff --git a/internal-dns/tests/basic_test.rs b/internal-dns/tests/basic_test.rs index d03b1262c90..c2cfb71ad99 100644 --- a/internal-dns/tests/basic_test.rs +++ b/internal-dns/tests/basic_test.rs @@ -10,14 +10,16 @@ use internal_dns_client::{ types::{DnsKv, DnsRecord, DnsRecordKey, Srv}, Client, }; +use trust_dns_proto::op::response_code::ResponseCode; use trust_dns_resolver::config::{ NameServerConfig, Protocol, ResolverConfig, ResolverOpts, }; +use trust_dns_resolver::error::ResolveErrorKind; use trust_dns_resolver::TokioAsyncResolver; #[tokio::test] pub async fn aaaa_crud() -> Result<(), anyhow::Error> { - let test_ctx = init_client_server().await?; + let test_ctx = init_client_server("oxide.internal".into()).await?; let client = &test_ctx.client; let resolver = &test_ctx.resolver; @@ -26,7 +28,7 @@ pub async fn aaaa_crud() -> Result<(), anyhow::Error> { assert!(records.is_empty()); // add an aaaa record - let name = DnsRecordKey { name: "devron.system".into() }; + let name = DnsRecordKey { name: "devron.oxide.internal".into() }; let addr = Ipv6Addr::new(0xfd, 0, 0, 0, 0, 0, 0, 0x1); let aaaa = DnsRecord::Aaaa(addr); client @@ -60,7 +62,7 @@ pub async fn aaaa_crud() -> Result<(), anyhow::Error> { #[tokio::test] pub async fn srv_crud() -> Result<(), anyhow::Error> { - let test_ctx = init_client_server().await?; + let test_ctx = init_client_server("oxide.internal".into()).await?; let client = &test_ctx.client; let resolver = &test_ctx.resolver; @@ -69,7 +71,7 @@ pub async fn srv_crud() -> Result<(), anyhow::Error> { assert!(records.is_empty()); // add a srv record - let name = DnsRecordKey { name: "hromi.cluster".into() }; + let name = DnsRecordKey { name: "hromi.oxide.internal".into() }; let srv = Srv { prio: 47, weight: 74, port: 99, target: "outpost47".into() }; let rec = DnsRecord::Srv(srv.clone()); @@ -108,6 +110,78 @@ pub async fn srv_crud() -> Result<(), anyhow::Error> { Ok(()) } +#[tokio::test] +pub async fn nxdomain() -> Result<(), anyhow::Error> { + let test_ctx = init_client_server("oxide.internal".into()).await?; + let resolver = &test_ctx.resolver; + + // asking for a nonexistent record within the domain of the internal DNS + // server should result in an NXDOMAIN + match resolver.lookup_ip("unicorn.oxide.internal").await { + Ok(unexpected) => { + panic!("Expected NXDOMAIN, got record {:?}", unexpected); + } + Err(e) => match e.kind() { + ResolveErrorKind::NoRecordsFound { + response_code, + query: _, + soa: _, + negative_ttl: _, + trusted: _, + } => match response_code { + ResponseCode::NXDomain => {} + unexpected => { + panic!( + "Expected NXDOMAIN, got response code {:?}", + unexpected + ); + } + }, + unexpected => { + panic!("Expected NXDOMAIN, got error {:?}", unexpected); + } + }, + }; + + Ok(()) +} + +#[tokio::test] +pub async fn servfail() -> Result<(), anyhow::Error> { + let test_ctx = init_client_server("oxide.internal".into()).await?; + let resolver = &test_ctx.resolver; + + // asking for a record outside the domain of the internal DNS + // server should result in a SERVFAIL. + match resolver.lookup_ip("oxide.computer").await { + Ok(unexpected) => { + panic!("Expected SERVFAIL, got record {:?}", unexpected); + } + Err(e) => match e.kind() { + ResolveErrorKind::NoRecordsFound { + response_code, + query: _, + soa: _, + negative_ttl: _, + trusted: _, + } => match response_code { + ResponseCode::ServFail => {} + unexpected => { + panic!( + "Expected SERVFAIL, got response code {:?}", + unexpected + ); + } + }, + unexpected => { + panic!("Expected SERVFAIL, got error {:?}", unexpected); + } + }, + }; + + Ok(()) +} + struct TestContext { client: Client, resolver: TokioAsyncResolver, @@ -122,7 +196,9 @@ impl TestContext { } } -async fn init_client_server() -> Result { +async fn init_client_server( + zone: String, +) -> Result { // initialize dns server config let (tmp, config, dropshot_port, dns_port) = test_config()?; let log = config @@ -160,6 +236,7 @@ async fn init_client_server() -> Result { let log = log.clone(); let dns_config = internal_dns::dns_server::Config { bind_address: format!("[::1]:{}", dns_port), + zone, }; tokio::spawn(async move { diff --git a/smf/internal-dns/manifest.xml b/smf/internal-dns/manifest.xml index d7364ce12f1..96384d8dc0c 100644 --- a/smf/internal-dns/manifest.xml +++ b/smf/internal-dns/manifest.xml @@ -13,13 +13,14 @@ + From a301936c27f9b9ef425a53dd237c42ba9b8367ac Mon Sep 17 00:00:00 2001 From: Ryan Goodfellow Date: Tue, 10 May 2022 20:42:14 -0700 Subject: [PATCH 2/2] internal-dns: send SERVFAIL on deserialize error --- internal-dns/src/dns_server.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/internal-dns/src/dns_server.rs b/internal-dns/src/dns_server.rs index a2d26373dbc..46ee34ed855 100644 --- a/internal-dns/src/dns_server.rs +++ b/internal-dns/src/dns_server.rs @@ -129,6 +129,7 @@ async fn handle_req<'a, 'b, 'c>( Ok(r) => r, Err(e) => { error!(log, "deserialize record: {}", e); + nack(&log, &mr, &socket, &header, &src).await; return; } };