dearsky/numa
1
0
Fork 0
Code Issues 10 Pull Requests 1 Actions Packages Projects Releases 25 Wiki Activity

fix: prevent self-referential DNS backup on re-install (#40)

Browse Source 
* fix: prevent self-referential DNS backup on re-install

The install flow previously captured current system DNS servers
verbatim into the backup file. If numa was already installed, current
DNS was 127.0.0.1, so the "backup" recorded 127.0.0.1 as the "original"
— making a subsequent uninstall a no-op self-reference.

Reproduced 2026-04-08 during v0.10.0 brew dogfood: after
`sudo numa uninstall; sudo /opt/homebrew/bin/numa install`,
`sudo numa uninstall` printed `restored DNS for "Wi-Fi" -> 127.0.0.1`
because the brew binary's install step had overwritten the backup with
the already-stub state.

Fix (all three platforms):
- macOS/Windows: if the existing backup already contains at least one
  non-loopback/non-stub upstream, preserve it as-is. If writing a fresh
  backup, filter loopback/stub addresses first so a capture from
  already-numa-managed state isn't self-referential.
- Linux (resolv.conf fallback path): detect numa-managed or all-loopback
  resolv.conf content and skip the file copy in that case; preserve an
  existing useful backup rather than overwriting it. systemd-resolved
  path is unaffected (uses a drop-in, no backup file).

Adds three unit tests for the predicates: macOS HashMap detection,
Windows interface filter, and resolv.conf parsing (real upstream,
self-referential, numa-marker, systemd stub, mixed).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* refactor: share iter_nameservers helper and reuse resolv.conf content

Post-review simplifications on the stale-backup fix:

- Extract iter_nameservers(&str) helper used by both parse_resolv_conf
  and resolv_conf_has_real_upstream. Eliminates the duplicated
  line-by-line nameserver parsing (findings from reuse review).
- install_linux: reuse the already-read resolv.conf content via
  std::fs::write instead of a second read via std::fs::copy.
- install_macos / install_windows: flatten the conditional eprintln
  pattern — always print a blank line, conditionally print the save
  message. Equivalent output, less branching.

Net −12 lines. All 130 tests still pass, clippy clean.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: drop redundant trim before split_whitespace

CI caught `clippy::trim_split_whitespace` on Rust 1.94: `split_whitespace()`
already skips leading/trailing whitespace, so `.trim()` first is redundant.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* refactor: extract load_backup helper

Remove duplicated read+deserialize boilerplate shared by install_macos
and install_windows. The two call sites each had an identical 4-line
chain of read_to_string().ok().and_then(serde_json::from_str).ok() —
collapse into a single generic helper load_backup<T>().

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Revert "refactor: extract load_backup helper"

This reverts commit a54fb99428.

* test: drop windows_backup_filters_loopback

The test inlined the 3-line filter block from install_windows rather
than calling a production helper, so it was testing stdlib Vec::retain
+ is_loopback_or_stub — both already covered elsewhere. Deleting it
removes a test that would silently pass even if install_windows stopped
filtering altogether.

The predicate logic for macOS-shaped backups stays covered by
macos_backup_real_upstream_detection (same inner Vec<String> type).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* test: add windows_backup_filters_loopback unit test

The PR description mentioned this test but it was missing from the
diff, leaving backup_has_real_upstream_windows untested. Mirrors the
shape of macos_backup_real_upstream_detection: empty map → false,
all-loopback (127.0.0.1, ::1, 0.0.0.0) → false, one real entry
alongside loopback → true.

Also relax the cfg gate on backup_has_real_upstream_windows from
cfg(windows) to cfg(any(windows, test)) so the test compiles
cross-platform, matching how backup_has_real_upstream_macos and
the resolv_conf helpers are gated.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit was merged in pull request #40.
This commit is contained in:
Razvan Dimescu
2026-04-08 16:38:37 +03:00
committed by GitHub
parent 039254280b
commit 679b346246
1 changed files with 220 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files

254
src/system_dns.rs
View File

@@ -214,7 +214,18 @@ fn discover_linux() -> SystemDnsInfo {
}
}
/// Parse resolv.conf in a single pass, extracting both the first non-loopback
/// Yield each `nameserver` address from resolv.conf content. No filtering —
/// callers decide what counts as a real upstream.
#[cfg(any(target_os = "linux", test))]
fn iter_nameservers(content: &str) -> impl Iterator<Item = &str> {
content.lines().filter_map(|line| {
let mut parts = line.split_whitespace();
(parts.next() == Some("nameserver")).then_some(())?;
parts.next()
})
}
/// Parse resolv.conf in a single pass, extracting the first non-loopback
/// nameserver and all search domains.
#[cfg(target_os = "linux")]
fn parse_resolv_conf(path: &str) -> (Option<String>, Vec<String>) {
@@ -222,19 +233,13 @@ fn parse_resolv_conf(path: &str) -> (Option<String>, Vec<String>) {
Ok(t) => t,
Err(_) => return (None, Vec::new()),
};
let mut upstream = None;
let upstream = iter_nameservers(&text)
.find(|ns| !is_loopback_or_stub(ns))
.map(str::to_string);
let mut search_domains = Vec::new();
for line in text.lines() {
let line = line.trim();
if line.starts_with("nameserver") {
if upstream.is_none() {
if let Some(ns) = line.split_whitespace().nth(1) {
if !is_loopback_or_stub(ns) {
upstream = Some(ns.to_string());
}
}
}
} else if line.starts_with("search") || line.starts_with("domain") {
if line.starts_with("search") || line.starts_with("domain") {
for domain in line.split_whitespace().skip(1) {
search_domains.push(domain.to_string());
}
@@ -243,6 +248,21 @@ fn parse_resolv_conf(path: &str) -> (Option<String>, Vec<String>) {
(upstream, search_domains)
}
/// True if the resolv.conf *content* appears to be written by numa itself,
/// or has no real upstream — either way, it's not a safe source of truth
/// for a backup.
#[cfg(any(target_os = "linux", test))]
fn resolv_conf_is_numa_managed(content: &str) -> bool {
content.contains("Generated by Numa") || !resolv_conf_has_real_upstream(content)
}
/// True if the resolv.conf content has at least one non-loopback, non-stub
/// nameserver. An all-loopback resolv.conf is self-referential.
#[cfg(any(target_os = "linux", test))]
fn resolv_conf_has_real_upstream(content: &str) -> bool {
iter_nameservers(content).any(|ns| !is_loopback_or_stub(ns))
}
/// Query resolvectl for the real upstream DNS server (e.g. VPC resolver on AWS).
#[cfg(target_os = "linux")]
fn resolvectl_dns_server() -> Option<String> {
@@ -526,9 +546,19 @@ fn enable_dnscache() {
.status();
}
/// True if the backup map has at least one real upstream (non-loopback, non-stub).
#[cfg(any(windows, test))]
fn backup_has_real_upstream_windows(
interfaces: &std::collections::HashMap<String, WindowsInterfaceDns>,
) -> bool {
interfaces
.values()
.any(|iface| iface.servers.iter().any(|s| !is_loopback_or_stub(s)))
}
#[cfg(windows)]
fn install_windows() -> Result<(), String> {
let interfaces = get_windows_interfaces()?;
let mut interfaces = get_windows_interfaces()?;
if interfaces.is_empty() {
return Err("no active network interfaces found".to_string());
}
@@ -538,9 +568,30 @@ fn install_windows() -> Result<(), String> {
std::fs::create_dir_all(parent)
.map_err(|e| format!("failed to create {}: {}", parent.display(), e))?;
}
let json = serde_json::to_string_pretty(&interfaces)
.map_err(|e| format!("failed to serialize backup: {}", e))?;
std::fs::write(&path, json).map_err(|e| format!("failed to write backup: {}", e))?;
// Preserve an existing useful backup rather than overwriting it with
// numa-managed state (which would be self-referential after uninstall).
let existing: Option<std::collections::HashMap<String, WindowsInterfaceDns>> =
std::fs::read_to_string(&path)
.ok()
.and_then(|json| serde_json::from_str(&json).ok());
let has_useful_existing = existing
.as_ref()
.map(backup_has_real_upstream_windows)
.unwrap_or(false);
if has_useful_existing {
eprintln!(" Existing DNS backup preserved at {}", path.display());
} else {
// Filter loopback/stub addresses before saving so a fresh backup
// captured from already-numa-managed state isn't self-referential.
for iface in interfaces.values_mut() {
iface.servers.retain(|s| !is_loopback_or_stub(s));
}
let json = serde_json::to_string_pretty(&interfaces)
.map_err(|e| format!("failed to serialize backup: {}", e))?;
std::fs::write(&path, json).map_err(|e| format!("failed to write backup: {}", e))?;
}
for name in interfaces.keys() {
let status = std::process::Command::new("netsh")
@@ -570,7 +621,10 @@ fn install_windows() -> Result<(), String> {
let needs_reboot = disable_dnscache()?;
register_autostart();
eprintln!("\n Original DNS saved to {}", path.display());
eprintln!();
if !has_useful_existing {
eprintln!(" Original DNS saved to {}", path.display());
}
eprintln!(" Run 'numa uninstall' to restore.\n");
if needs_reboot {
eprintln!(" *** Reboot required. Numa will start automatically. ***\n");
@@ -754,27 +808,60 @@ fn get_dns_servers(service: &str) -> Result<Vec<String>, String> {
}
}
/// True if the backup map has at least one real upstream (non-loopback, non-stub).
/// An all-loopback backup is self-referential — restoring it is a no-op.
#[cfg(any(target_os = "macos", test))]
fn backup_has_real_upstream_macos(
servers: &std::collections::HashMap<String, Vec<String>>,
) -> bool {
servers
.values()
.any(|list| list.iter().any(|s| !is_loopback_or_stub(s)))
}
#[cfg(target_os = "macos")]
fn install_macos() -> Result<(), String> {
use std::collections::HashMap;
let services = get_network_services()?;
let mut original: HashMap<String, Vec<String>> = HashMap::new();
// Save current DNS for each service
for service in &services {
let servers = get_dns_servers(service)?;
original.insert(service.clone(), servers);
}
// Save backup
let dir = numa_data_dir();
std::fs::create_dir_all(&dir)
.map_err(|e| format!("failed to create {}: {}", dir.display(), e))?;
let json = serde_json::to_string_pretty(&original)
.map_err(|e| format!("failed to serialize backup: {}", e))?;
std::fs::write(backup_path(), json).map_err(|e| format!("failed to write backup: {}", e))?;
// If a useful backup already exists (at least one non-loopback upstream),
// preserve it — overwriting would destroy the original DNS state when
// re-installing on top of a numa-managed configuration.
let existing_backup: Option<HashMap<String, Vec<String>>> =
std::fs::read_to_string(backup_path())
.ok()
.and_then(|json| serde_json::from_str(&json).ok());
let has_useful_existing = existing_backup
.as_ref()
.map(backup_has_real_upstream_macos)
.unwrap_or(false);
if has_useful_existing {
eprintln!(
" Existing DNS backup preserved at {}",
backup_path().display()
);
} else {
// Capture fresh, filtering out loopback and stub addresses so we
// never record a self-referential backup.
let mut original: HashMap<String, Vec<String>> = HashMap::new();
for service in &services {
let servers: Vec<String> = get_dns_servers(service)?
.into_iter()
.filter(|s| !is_loopback_or_stub(s))
.collect();
original.insert(service.clone(), servers);
}
let json = serde_json::to_string_pretty(&original)
.map_err(|e| format!("failed to serialize backup: {}", e))?;
std::fs::write(backup_path(), json)
.map_err(|e| format!("failed to write backup: {}", e))?;
}
// Set DNS to 127.0.0.1 and add "numa" search domain for each service
for service in &services {
@@ -795,7 +882,10 @@ fn install_macos() -> Result<(), String> {
.status();
}
eprintln!("\n Original DNS saved to {}", backup_path().display());
eprintln!();
if !has_useful_existing {
eprintln!(" Original DNS saved to {}", backup_path().display());
}
eprintln!(" Run 'sudo numa uninstall' to restore.\n");
Ok(())
@@ -1132,11 +1222,31 @@ fn install_linux() -> Result<(), String> {
.map_err(|e| format!("failed to create {}: {}", parent.display(), e))?;
}
// Back up current resolv.conf (ignore NotFound)
match std::fs::copy(resolv, &backup) {
Ok(_) => eprintln!(" Saved /etc/resolv.conf to {}", backup.display()),
Err(e) if e.kind() == std::io::ErrorKind::NotFound => {}
Err(e) => return Err(format!("failed to backup /etc/resolv.conf: {}", e)),
// Back up current resolv.conf, but never overwrite a useful existing
// backup with a numa-managed file — that would leave uninstall with
// nothing to restore to.
let current = std::fs::read_to_string(resolv).ok();
let current_is_numa_managed = current
.as_deref()
.map(resolv_conf_is_numa_managed)
.unwrap_or(false);
let existing_backup_is_useful = std::fs::read_to_string(&backup)
.ok()
.as_deref()
.map(resolv_conf_has_real_upstream)
.unwrap_or(false);
if existing_backup_is_useful {
eprintln!(
" Existing resolv.conf backup preserved at {}",
backup.display()
);
} else if current_is_numa_managed {
eprintln!(" warning: /etc/resolv.conf is already numa-managed; no fresh backup written");
} else if let Some(content) = current.as_deref() {
std::fs::write(&backup, content)
.map_err(|e| format!("failed to backup /etc/resolv.conf: {}", e))?;
eprintln!(" Saved /etc/resolv.conf to {}", backup.display());
}
if resolv
@@ -1539,6 +1649,82 @@ Wireless LAN adapter Wi-Fi:
assert!(!result.contains("{{exe_path}}"));
}
#[test]
fn macos_backup_real_upstream_detection() {
use std::collections::HashMap;
let mut map: HashMap<String, Vec<String>> = HashMap::new();
// Empty backup → no real upstream
assert!(!backup_has_real_upstream_macos(&map));
// All-loopback backup → still no real upstream (the bug case)
map.insert("Wi-Fi".into(), vec!["127.0.0.1".into()]);
map.insert("Ethernet".into(), vec!["::1".into()]);
assert!(!backup_has_real_upstream_macos(&map));
// One real entry → useful
map.insert("Tailscale".into(), vec!["192.168.1.1".into()]);
assert!(backup_has_real_upstream_macos(&map));
}
#[test]
fn windows_backup_filters_loopback() {
use std::collections::HashMap;
let mut map: HashMap<String, WindowsInterfaceDns> = HashMap::new();
// Empty backup → no real upstream
assert!(!backup_has_real_upstream_windows(&map));
// All-loopback backup → still no real upstream (the bug case)
map.insert(
"Wi-Fi".into(),
WindowsInterfaceDns {
dhcp: false,
servers: vec!["127.0.0.1".into()],
},
);
map.insert(
"Ethernet".into(),
WindowsInterfaceDns {
dhcp: false,
servers: vec!["::1".into(), "0.0.0.0".into()],
},
);
assert!(!backup_has_real_upstream_windows(&map));
// One real entry alongside loopback → useful
map.insert(
"Ethernet 2".into(),
WindowsInterfaceDns {
dhcp: false,
servers: vec!["192.168.1.1".into()],
},
);
assert!(backup_has_real_upstream_windows(&map));
}
#[test]
fn resolv_conf_real_upstream_detection() {
let real = "nameserver 192.168.1.1\nsearch lan\n";
assert!(resolv_conf_has_real_upstream(real));
assert!(!resolv_conf_is_numa_managed(real));
let self_ref = "nameserver 127.0.0.1\nsearch numa\n";
assert!(!resolv_conf_has_real_upstream(self_ref));
assert!(resolv_conf_is_numa_managed(self_ref));
let numa_marker =
"# Generated by Numa — run 'sudo numa uninstall' to restore\nnameserver 127.0.0.1\nsearch numa\n";
assert!(resolv_conf_is_numa_managed(numa_marker));
let systemd_stub = "nameserver 127.0.0.53\noptions edns0\n";
assert!(!resolv_conf_has_real_upstream(systemd_stub));
let mixed = "nameserver 127.0.0.1\nnameserver 1.1.1.1\n";
assert!(resolv_conf_has_real_upstream(mixed));
assert!(!resolv_conf_is_numa_managed(mixed));
}
#[test]
fn parse_ipconfig_skips_disconnected() {
let sample = "\