Start9/start-os
2
0
Fork 0
mirror of https://github.com/Start9Labs/start-os.git synced 2026-03-30 20:14:49 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: replace .status() with .invoke() for iptables/ip commands

Browse Source 
Using .status() leaks stderr directly to system logs, causing noisy
iptables error messages. Switch all networking CLI invocations to use
.invoke() which captures stderr properly. For check-then-act patterns
(iptables -C), use .invoke().await.is_err() instead of
.status().await.map_or(false, |s| s.success()).
This commit is contained in:
Aiden McClelland
2026-02-17 14:12:29 -07:00
parent bc4478b0b9
commit 5fbc73755d
4 changed files with 183 additions and 100 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
core/CLAUDE.md
View File

@@ -24,3 +24,4 @@ cd sdk && make baseDist dist # Rebuild SDK after ts-bindings
- When modifying `#[ts(export)]` types, regenerate bindings and rebuild the SDK (see [ARCHITECTURE.md](../ARCHITECTURE.md#build-pipeline)) - When modifying `#[ts(export)]` types, regenerate bindings and rebuild the SDK (see [ARCHITECTURE.md](../ARCHITECTURE.md#build-pipeline))
- When adding i18n keys, add all 5 locales in `core/locales/i18n.yaml` (see [i18n-patterns.md](i18n-patterns.md)) - When adding i18n keys, add all 5 locales in `core/locales/i18n.yaml` (see [i18n-patterns.md](i18n-patterns.md))
- When using DB watches, follow the `TypedDbWatch<T>` patterns in [patchdb.md](patchdb.md) - When using DB watches, follow the `TypedDbWatch<T>` patterns in [patchdb.md](patchdb.md)
- **Always use `.invoke(ErrorKind::...)` instead of `.status()` when running CLI commands** via `tokio::process::Command`. The `Invoke` trait (from `crate::util::Invoke`) captures stdout/stderr and checks exit codes properly. Using `.status()` leaks stderr directly to system logs, creating noise. For check-then-act patterns (e.g. `iptables -C`), use `.invoke(...).await.is_ok()` / `.is_err()` instead of `.status().await.map_or(false, |s| s.success())`.

3
core/src/net/forward.rs
View File

@@ -254,7 +254,8 @@ pub async fn add_iptables_rule(nat: bool, undo: bool, args: &[&str]) -> Result<(
if nat { if nat {
cmd.arg("-t").arg("nat"); cmd.arg("-t").arg("nat");
} }
if undo != !cmd.arg("-C").args(args).status().await?.success() { let exists = cmd.arg("-C").args(args).invoke(ErrorKind::Network).await.is_ok();
if undo != !exists {
let mut cmd = Command::new("iptables"); let mut cmd = Command::new("iptables");
if nat { if nat {
cmd.arg("-t").arg("nat"); cmd.arg("-t").arg("nat");

263
core/src/net/gateway.rs
View File

@@ -931,14 +931,14 @@ async fn watch_ip(
// when the packet has no existing fwmark, preserving marks // when the packet has no existing fwmark, preserving marks
// set by WireGuard on encapsulation packets. // set by WireGuard on encapsulation packets.
for chain in ["PREROUTING", "OUTPUT"] { for chain in ["PREROUTING", "OUTPUT"] {
if !Command::new("iptables") if Command::new("iptables")
.arg("-t").arg("mangle") .arg("-t").arg("mangle")
.arg("-C").arg(chain) .arg("-C").arg(chain)
.arg("-m").arg("mark").arg("--mark").arg("0") .arg("-m").arg("mark").arg("--mark").arg("0")
.arg("-j").arg("CONNMARK") .arg("-j").arg("CONNMARK")
.arg("--restore-mark") .arg("--restore-mark")
.status().await .invoke(ErrorKind::Network).await
.map_or(false, |s| s.success()) .is_err()
{ {
Command::new("iptables") Command::new("iptables")
.arg("-t").arg("mangle") .arg("-t").arg("mangle")
@@ -954,7 +954,7 @@ async fn watch_ip(
// Mark NEW connections arriving on this interface // Mark NEW connections arriving on this interface
// with its routing table ID via conntrack mark // with its routing table ID via conntrack mark
if !Command::new("iptables") if Command::new("iptables")
.arg("-t").arg("mangle") .arg("-t").arg("mangle")
.arg("-C").arg("PREROUTING") .arg("-C").arg("PREROUTING")
.arg("-i").arg(iface.as_str()) .arg("-i").arg(iface.as_str())
@@ -962,8 +962,8 @@ async fn watch_ip(
.arg("--ctstate").arg("NEW") .arg("--ctstate").arg("NEW")
.arg("-j").arg("CONNMARK") .arg("-j").arg("CONNMARK")
.arg("--set-mark").arg(&table_str) .arg("--set-mark").arg(&table_str)
.status().await .invoke(ErrorKind::Network).await
.map_or(false, |s| s.success()) .is_err()
{ {
Command::new("iptables") Command::new("iptables")
.arg("-t").arg("mangle") .arg("-t").arg("mangle")
@@ -1257,99 +1257,180 @@ impl NetworkInterfaceController {
default_outbound: &Option<GatewayId>, default_outbound: &Option<GatewayId>,
ip_info: &OrdMap<GatewayId, NetworkInterfaceInfo>, ip_info: &OrdMap<GatewayId, NetworkInterfaceInfo>,
) { ) {
// Clean up all our policy routing rules (loop because multiple may exist // 1. Snapshot existing rules at priorities 74 and 75.
// at the same priority, and `ip rule del` only removes one at a time) // Priority 74: fwmark-based exemptions (WireGuard encap packets)
for prio in ["74", "75"] { // Priority 75: catch-all routing to the chosen gateway's table
loop { let (existing_74, existing_75) = match async {
let ok = Command::new("ip") let output = String::from_utf8(
.arg("rule").arg("del") Command::new("ip")
.arg("priority").arg(prio) .arg("rule")
.status() .arg("show")
.await .invoke(ErrorKind::Network)
.map_or(false, |s| s.success()); .await?,
if !ok { )?;
break; let mut fwmarks_74 = BTreeSet::<u32>::new();
let mut tables_75 = BTreeSet::<u32>::new();
for line in output.lines() {
let line = line.trim();
if let Some(rest) = line.strip_prefix("74:") {
if let Some(pos) = rest.find("fwmark ") {
let after = &rest[pos + 7..];
let token = after.split_whitespace().next().unwrap_or("");
if let Ok(v) =
u32::from_str_radix(token.strip_prefix("0x").unwrap_or(token), 16)
{
fwmarks_74.insert(v);
}
}
} else if let Some(rest) = line.strip_prefix("75:") {
if let Some(pos) = rest.find("lookup ") {
let after = &rest[pos + 7..];
let token = after.split_whitespace().next().unwrap_or("");
if let Ok(v) = token.parse::<u32>() {
tables_75.insert(v);
}
}
}
}
Ok::<_, Error>((fwmarks_74, tables_75))
}
.await
{
Ok(v) => v,
Err(e) => {
tracing::error!("failed to snapshot outbound rules: {e}");
(BTreeSet::new(), BTreeSet::new())
}
};
// 2. Compute desired rules
let mut desired_74 = BTreeSet::<u32>::new();
let mut desired_75 = BTreeSet::<u32>::new();
if let Some(gw_id) = default_outbound {
let connected = ip_info
.get(gw_id)
.map_or(false, |info| info.ip_info.is_some());
if !connected {
if ip_info.contains_key(gw_id) {
tracing::warn!("default outbound gateway {gw_id} is not connected");
} else {
tracing::warn!("default outbound gateway {gw_id} not found in ip_info");
}
} else {
match if_nametoindex(gw_id.as_str()) {
Ok(idx) => {
let table_id = 1000 + idx;
desired_75.insert(table_id);
// Exempt ALL active WireGuard interfaces' encapsulation packets.
// Our priority-75 catch-all would otherwise swallow their encap
// traffic before NM's fwmark rules at priority 31610 can route
// it correctly.
for (iface_id, iface_info) in ip_info {
let Some(ref ip) = iface_info.ip_info else {
continue;
};
if ip.device_type != Some(NetworkInterfaceType::Wireguard) {
continue;
}
match Command::new("wg")
.arg("show")
.arg(iface_id.as_str())
.arg("fwmark")
.invoke(ErrorKind::Network)
.await
{
Ok(output) => {
let fwmark_hex =
String::from_utf8_lossy(&output).trim().to_owned();
if fwmark_hex.is_empty() || fwmark_hex == "off" {
continue;
}
match u32::from_str_radix(
fwmark_hex.strip_prefix("0x").unwrap_or(&fwmark_hex),
16,
) {
Ok(v) => {
desired_74.insert(v);
}
Err(e) => {
tracing::error!(
"failed to parse WireGuard fwmark '{fwmark_hex}' for {iface_id}: {e}"
);
}
}
}
Err(e) => {
tracing::error!(
"failed to read WireGuard fwmark for {iface_id}: {e}"
);
}
}
}
}
Err(e) => {
tracing::error!("failed to get ifindex for {gw_id}: {e}");
}
} }
} }
} }
// Nothing more to do if no default outbound is selected // 3. Add rules in desired set but not in existing set
let Some(gw_id) = default_outbound else { for fwmark in desired_74.difference(&existing_74) {
return; Command::new("ip")
}; .arg("rule")
let Some(info) = ip_info.get(gw_id) else { .arg("add")
tracing::warn!("default outbound gateway {gw_id} not found in ip_info"); .arg("fwmark")
return; .arg(fwmark.to_string())
}; .arg("lookup")
if info.ip_info.is_none() { .arg("main")
tracing::warn!("default outbound gateway {gw_id} is not connected"); .arg("priority")
return; .arg("74")
}
let table_id = match if_nametoindex(gw_id.as_str()) {
Ok(idx) => 1000 + idx,
Err(e) => {
tracing::error!("failed to get ifindex for {gw_id}: {e}");
return;
}
};
let table_str = table_id.to_string();
// Exempt ALL active WireGuard interfaces' encapsulation packets.
// Our priority-75 catch-all would otherwise swallow their encap traffic
// before NM's fwmark rules at priority 31610 can route it correctly.
for (iface_id, iface_info) in ip_info {
let Some(ref ip) = iface_info.ip_info else {
continue;
};
if ip.device_type != Some(NetworkInterfaceType::Wireguard) {
continue;
}
match Command::new("wg")
.arg("show").arg(iface_id.as_str()).arg("fwmark")
.invoke(ErrorKind::Network) .invoke(ErrorKind::Network)
.await .await
{ .log_err();
Ok(output) => { }
let fwmark_hex = String::from_utf8_lossy(&output).trim().to_owned(); for table in desired_75.difference(&existing_75) {
if fwmark_hex.is_empty() || fwmark_hex == "off" { Command::new("ip")
continue; .arg("rule")
} .arg("add")
let fwmark = match u32::from_str_radix( .arg("table")
fwmark_hex.strip_prefix("0x").unwrap_or(&fwmark_hex), .arg(table.to_string())
16, .arg("priority")
) { .arg("75")
Ok(v) => v, .invoke(ErrorKind::Network)
Err(e) => { .await
tracing::error!( .log_err();
"failed to parse WireGuard fwmark '{fwmark_hex}' for {iface_id}: {e}"
);
continue;
}
};
Command::new("ip")
.arg("rule").arg("add")
.arg("fwmark").arg(fwmark.to_string())
.arg("lookup").arg("main")
.arg("priority").arg("74")
.invoke(ErrorKind::Network)
.await
.log_err();
}
Err(e) => {
tracing::error!("failed to read WireGuard fwmark for {iface_id}: {e}");
}
}
} }
// Route all other traffic through the gateway's per-interface table // 4. Delete rules in existing set but not in desired set
Command::new("ip") for fwmark in existing_74.difference(&desired_74) {
.arg("rule").arg("add") Command::new("ip")
.arg("table").arg(&table_str) .arg("rule")
.arg("priority").arg("75") .arg("del")
.invoke(ErrorKind::Network) .arg("fwmark")
.await .arg(fwmark.to_string())
.log_err(); .arg("lookup")
.arg("main")
.arg("priority")
.arg("74")
.invoke(ErrorKind::Network)
.await
.log_err();
}
for table in existing_75.difference(&desired_75) {
Command::new("ip")
.arg("rule")
.arg("del")
.arg("table")
.arg(table.to_string())
.arg("priority")
.arg("75")
.invoke(ErrorKind::Network)
.await
.log_err();
}
} }
pub fn new(db: TypedPatchDb<Database>) -> Self { pub fn new(db: TypedPatchDb<Database>) -> Self {

16
core/src/net/net_controller.rs
View File

@@ -483,13 +483,13 @@ impl NetService {
let service_ip = ip.to_string(); let service_ip = ip.to_string();
// Purge any stale rules from a previous instance // Purge any stale rules from a previous instance
loop { loop {
if !Command::new("ip") if Command::new("ip")
.arg("rule").arg("del") .arg("rule").arg("del")
.arg("from").arg(&service_ip) .arg("from").arg(&service_ip)
.arg("priority").arg("100") .arg("priority").arg("100")
.status() .invoke(ErrorKind::Network)
.await .await
.map_or(false, |s| s.success()) .is_err()
{ {
break; break;
} }
@@ -559,7 +559,7 @@ impl NetService {
.arg("from").arg(&service_ip) .arg("from").arg(&service_ip)
.arg("lookup").arg(&old_table_str) .arg("lookup").arg(&old_table_str)
.arg("priority").arg("100") .arg("priority").arg("100")
.status() .invoke(ErrorKind::Network)
.await; .await;
} }
// Read current outbound gateway from DB // Read current outbound gateway from DB
@@ -610,7 +610,7 @@ impl NetService {
.arg("from").arg(&service_ip) .arg("from").arg(&service_ip)
.arg("lookup").arg(&table_str) .arg("lookup").arg(&table_str)
.arg("priority").arg("100") .arg("priority").arg("100")
.status() .invoke(ErrorKind::Network)
.await; .await;
} }
} else { } else {
@@ -762,13 +762,13 @@ impl NetService {
// Clean up any outbound gateway ip rules for this service // Clean up any outbound gateway ip rules for this service
let service_ip = self.data.lock().await.ip.to_string(); let service_ip = self.data.lock().await.ip.to_string();
loop { loop {
if !Command::new("ip") if Command::new("ip")
.arg("rule").arg("del") .arg("rule").arg("del")
.arg("from").arg(&service_ip) .arg("from").arg(&service_ip)
.arg("priority").arg("100") .arg("priority").arg("100")
.status() .invoke(ErrorKind::Network)
.await .await
.map_or(false, |s| s.success()) .is_err()
{ {
break; break;
} }