What
deprovision gates the destroy on is_defined() but removes the firewall
unconditionally.
src/backends/lxc/common/src/state_aware.rs:666-676
if container.is_defined() {
// destroy force-stops and removes the container ...
container.destroy().map_err(..)?;
}
cleanup_network_authoritative(container_name, veth.as_deref(), &mut logger);And is_defined cannot tell "absent" from "could not ask"
(lxc_bindings.rs:248-251):
let output = self.lxc_command("lxc-info").output();
matches!(output, Ok(o) if o.status.success())A failure to spawn lxc-info returns Err and a non-zero exit returns
Ok(non-success). Both become false.
Why it matters
So when lxc-info cannot be run -- transient fork/exec failure, PATH problem,
resource exhaustion -- deprovision concludes the container does not exist, skips
destroy(), and then goes on to authoritatively tear down the firewall for a
container that may still be running. The result is a live container with
its egress filtering removed: a fail-open, and the exact outcome the ordering
comment immediately above is written to prevent.
The comment states the invariant plainly -- destroy first "so nothing runs
without egress filtering during teardown; if the destroy fails, leave the rules
in place." The is_defined() false negative walks around that invariant rather
than through it, because it never reaches destroy() at all.
Fix
Make the probe distinguish its two failure modes. Have is_defined return
something three-valued (present / absent / unknown), and treat unknown the way
a failed destroy is already treated: leave the firewall in place and report the
failure, rather than tearing down filtering on a guess.
This is the same fail-closed reasoning applied to the FORWARD-hook probe in
PR #849 (network_iptables.rs, commit a481f3d), but it is a signature change
on a widely called helper -- is_defined also gates provision adoption and the
start not-provisioned check -- so it wants its own review rather than a
drive-by.
Provenance
Reported by Copilot in a suppressed comment on PR #849. Mechanism verified
against source.
What
deprovisiongates the destroy onis_defined()but removes the firewallunconditionally.
src/backends/lxc/common/src/state_aware.rs:666-676And
is_definedcannot tell "absent" from "could not ask"(
lxc_bindings.rs:248-251):A failure to spawn
lxc-inforeturnsErrand a non-zero exit returnsOk(non-success). Both becomefalse.Why it matters
So when
lxc-infocannot be run -- transient fork/exec failure, PATH problem,resource exhaustion -- deprovision concludes the container does not exist, skips
destroy(), and then goes on to authoritatively tear down the firewall for acontainer that may still be running. The result is a live container with
its egress filtering removed: a fail-open, and the exact outcome the ordering
comment immediately above is written to prevent.
The comment states the invariant plainly -- destroy first "so nothing runs
without egress filtering during teardown; if the destroy fails, leave the rules
in place." The
is_defined()false negative walks around that invariant ratherthan through it, because it never reaches
destroy()at all.Fix
Make the probe distinguish its two failure modes. Have
is_definedreturnsomething three-valued (present / absent / unknown), and treat unknown the way
a failed destroy is already treated: leave the firewall in place and report the
failure, rather than tearing down filtering on a guess.
This is the same fail-closed reasoning applied to the FORWARD-hook probe in
PR #849 (
network_iptables.rs, commit a481f3d), but it is a signature changeon a widely called helper --
is_definedalso gatesprovisionadoption and thestartnot-provisioned check -- so it wants its own review rather than adrive-by.
Provenance
Reported by Copilot in a suppressed comment on PR #849. Mechanism verified
against source.