Skip to content

fix(security): a boot asks whose network it is, not only whether the name is well formed (#932) - #943

Merged
stephrobert merged 1 commit into
mainfrom
fix/932-the-cold-attach-asks-too
Nov 14, 2026
Merged

stephrobert merged 1 commit into
mainfrom
fix/932-the-cold-attach-asks-too

Conversation

@stephrobert

Copy link
Copy Markdown
Owner

Summary

Attach has asked both questions since #909. The two cold paths that plug the same kind of NIC asked only safeName: the primary attachment inside Start, and attachExtra, which adds the extra NICs of a boot.

incusbr0 passes safeName. The name comes from Resource.Runtime, which PUT /_feint/state and feint snapshot load restore verbatim, and every pack reads it straight off into plan.Boot and then Spec.Attachments. Measured at argv level before the fix:

init images:alpine/3.21/cloud feint-scw-victim --network incusbr0
config device add feint-scw-victim eth1 nic network=incusbr0 ipv4.address=192.168.1.250

A machine the emulator starts landed on the operator's own bridge, took an address from their DHCP and reached their LAN, which incus_address.go's own comment calls the one thing this emulator must never do. It was also unremovable afterwards, since Detach refuses a network outside the prefix.

This is the busier half of the family #909 closed: the boot path runs on every poweron, not only on a hot join.

The change

Both questions now live once, in ownedAttachments, called at Start's entry over every attachment and at attachExtra's own entry. One named predicate rather than a check per call site, and the reasoning matters: the asymmetry this lot found, two safeName-only checks against one safeName && ownedNetwork check, came from two copies of the rule, not from two call sites. A predicate called twice has no copy that can drift.

Type of change

  • Bug fix
  • A route added or changed
  • Refactor
  • Documentation
  • Chore / tooling
  • Security / supply chain

What the contradicteurs found, each closed with its measurement

Two roles, two cycles each. One BLOCKER and two MUST.

BLOCKER, and it is the lesson of the day. The shape half of the new guard had no biting test. Neutralise safeName in the loop and keep ownedNetwork, and nothing in internal/core/machine went red, because every row of the attack table named a network ownedNetwork also refuses. grep -rn 'invalid network name' --include='*_test.go' returned zero.

That is the #909 defect one question over: there, every row shared a machine name the guard also refused. Two rows now pass one half and fail the other, fnt-a/b and fnt-a b, and the proof is a before and after pair rather than a claim: the same mutation printed TEST STILL PASSED before the rows and bites after them. The reviewer reproduced it independently in two trees differing by exactly those two lines.

MUST-1: attachExtra was protected by prose. The lot deleted its check and put a comment in its place, true today, with nothing failing the day it stops being true. A second caller added in six months would inherit an obligation stated in a comment, and no mutation can express "add a second caller". Hence the extracted predicate, called at both entries.

MUST-2: the comment claimed restart-branch coverage no test measured, and the behaviour it hid was untested. A poweron of an already-running machine whose stored plan names a foreign network is now refused before the Inspect, and Binding.PowerOn publishes FailedState for a machine the runtime is running. That is deliberate — a plan the driver cannot trust is a plan it does not execute — and TestARestartIsRefusedWhenThePlanNamesAForeignNetwork measures it instead of the comment asserting it.

A gap closed in the same pass. fnt-default's acceptance was held by one subtest and nothing else, measured: a mutation refusing only DefaultMachineNetwork reddens TestStartStillBootsOnTheNetworksTheEmulatorCreated/the_default_machine_network_Outscale_asks_for and leaves the sibling subtest and six other start tests green. Narrowing ownership to the derived shape would otherwise break Outscale's public-cloud Vm path with nothing anywhere to see it. The spec now carries that mutation, the first here to judge a subtest path — and #923's new lint, merged just before this, confirms it enters a real subtest.

One claim that could not become a mutation, and says so

Deleting only attachExtra's inner call leaves TestStartRefusesAForeignNetworkOnAnExtraNIC green, which is the honest reading of defence in depth. But falsify.py has a single verdict, "the test must bite", so an expect-green entry would redden the gate for ever, and a gate people must ignore is worse than no gate. It was measured by hand in a copy outside the repository, and the result is written where a reader meets the call, with its date.

The reviewer confirmed the measurement and found it stronger than claimed: nothing in the package measures that inner call, not only the one test. It also proposed the better answer, which is not taken here and is getting its own issue: a test driving attachExtra directly would let a gate carry the claim, and is strictly better than an expect-green verdict, since an expect-green mutation is also green when the test rots away entirely.

Runs

run verdict
mise run prepush green, on the rebase onto ef7e03b
a-boot-asks-whose-network-it-is.json, 9 mutations all 9 bite, all compiled and vet-clean, restoration green
python3 tools/falsify/falsify.py --lint tools/falsify/specs green, 1683 mutations over 253 specs, 14 subtests really entered
go test ./internal/core/machine/ green
FEINT_VM=incus-ovn mise run conformance:leg -- runtime green, 1433 s
station afterwards 0 machine, 0 fnt- network, no surviving DHCP service

Mode, stated rather than left blank: incus-ovn, on the maintainer's station, checked free first (free -g, incus list) with no lab running.

The leg's own closing verdicts, from its log: no machine or network of this run is left on this runtime, nothing on this runtime is beyond an ordinary sweep, no DHCP service of this emulator's outlives its network on this host.

Why the leg was not replayed after the last rebase

It ran on the previous base, and main then gained #942. #942 touches no file under internal/core/machine — checked with git log --name-only on that merge, not assumed: it changed tools/falsify/falsify.py, mise.toml, tools/testplan/specs.go and the two conformance pages. So the leg's verdict is about the same code. The lint half of #942 does read this lot's spec, and that was re-run: green, and it is what counts the fourteenth subtest.

On the leg's cost

CLAUDE.md still announces 590 s. Three independent measurements now: 1433 s (2026-11-13), 1455 s (2026-11-14, against a concurrent 8.6 GB build, so 1.4 % of contention) and 1433 s (2026-11-14, quiet station). The announced figure is the one that is wrong, and part of the gap is identified: six machine starts each wait 90 s for a resolver the guest never names.

Checklist

Always

  • mise run check passes (inside prepush, on the rebase)
  • No new external Go dependency
  • internal/core still knows no provider: the predicate reads a prefix this layer derives, and no provider spelling enters it
  • Nothing in the diff could be written identically for another provider: the guard is at the shared choke point, so all three packs get it at once
  • mise run testplan was run, and what it printed was run

When a model wrote a substantive part of this

  • An Assisted-by: trailer names the tool and the model version
  • The leg was run, not assumed, and its measured time is in the table
  • No field name and no response shape changes

When a route is added or changed

N/A.

When behaviour a client can observe changes

One refusal that was already supposed to happen now happens, plus the restart refusal above, which is deliberate and measured. No documented limit moves.

When .github/workflows/ is touched

N/A.

When a machine runtime is involved

  • Tested with FEINT_VM=incus-ovn
  • Nothing the emulator did not create was touched: that is the subject, and the runner tests assert it at argv level
  • The run leaves nothing behind, checked: incus list and incus network list after the leg, plus its own three closing verdicts

Left to their own issues

  • Attachment.Address reaches argv unparsed on these paths, and it is the only address in Spec that does. The "always one prefixed element" argument holds at the two sites this lot touches, and not on the round trip: restorePinnedAddress reads the value back off the device and configureGuestAddress passes it to ip address add <cidr> as a bare argv element.
  • attachExtra's runtime-caused failure still returns without abandonStart, so the instance stays running while the pack publishes FailedState. This lot removed the name-caused version of that.
  • A machine already on a foreign network cannot have that NIC removed through the API, because Detach refuses the network. Terminate still works, since the instance carries the emulator's label, so nothing is stuck.

Related issues

Closes #932.

🤖 Generated with Claude Code

…name is well formed (#932)

Attach has asked both questions since #909. The two cold paths that plug the
same kind of NIC asked only safeName: the primary attachment inside Start, and
attachExtra, which adds the extra NICs of a boot.

incusbr0 passes safeName. The name comes from Resource.Runtime, which
PUT /_feint/state and `feint snapshot load` restore verbatim, and every pack
reads it straight off into plan.Boot and then Spec.Attachments. Measured at
argv level before the fix: `init … --network incusbr0` and
`config device add … network=incusbr0`. A machine the emulator starts landed on
the operator's own bridge, took an address from their DHCP and reached their
LAN, which incus_address.go's own comment calls the one thing this emulator must
never do. It was also unremovable afterwards, since Detach refuses a network
outside the prefix.

Both questions now live once, in ownedAttachments, called at Start's entry over
every attachment and at attachExtra's own entry. One named predicate rather than
a check per call site: the asymmetry this lot found, two safeName-only checks
against one safeName-and-ownedNetwork check, came from two copies of the rule,
and a predicate called twice has no copy that can drift.

Three findings of the contradicteurs, each closed with its measurement:

The shape half had no biting test. Neutralise safeName in the loop and keep
ownedNetwork, and nothing in internal/core/machine went red, because every row
of the attack table named a network ownedNetwork also refuses. That is the #909
defect one question over, where every row shared a machine name the guard also
refused. Two rows now pass one half and fail the other -- fnt-a/b and fnt-a b --
and the proof is a before and after pair: the same mutation printed TEST STILL
PASSED before the rows and bites after them.

fnt-default's acceptance was held by one subtest and nothing else, measured: a
mutation refusing only DefaultMachineNetwork reddens
TestStartStillBootsOnTheNetworksTheEmulatorCreated/the_default_machine_network_Outscale_asks_for
and leaves the sibling subtest and six other start tests green. The spec now
carries that mutation, the first here to judge a subtest path. Narrowing
ownership to the derived shape would otherwise break Outscale's public-cloud Vm
path with no test anywhere to see it.

The restart branch claim was vacuous and the behaviour it hides was untested. A
poweron of an already-running machine whose stored plan names a foreign network
is now refused before the Inspect, and Binding.PowerOn publishes FailedState for
a machine the runtime is running. That is deliberate -- a plan the driver cannot
trust is a plan it does not execute -- and
TestARestartIsRefusedWhenThePlanNamesAForeignNetwork measures it instead of the
comment asserting it.

One claim could not become a mutation and says so. Deleting only attachExtra's
inner call leaves TestStartRefusesAForeignNetworkOnAnExtraNIC green, which is
the honest reading of defence in depth, but falsify.py has a single verdict and
an expect-green entry would redden the gate for ever. It was measured by hand in
a copy outside the repository and the result is written where a reader meets the
call, with its date. The better answer exists and is not taken here: a test
driving attachExtra directly would let a gate carry the claim.

Proven through the injectable runner, plus the runtime leg: prepush green;
a-boot-asks-whose-network-it-is.json nine mutations, all compiled, all biting,
restoration green; falsify:lint green over 253 specs;
FEINT_VM=incus-ovn conformance:leg -- runtime green in 1433 s with no machine,
no fnt- network and no DHCP service left on the host.

Left to their own issues: Attachment.Address reaches argv unparsed on these
paths, and it is the only address in Spec that does; attachExtra's runtime-caused
failure still returns without abandonStart, so the instance stays running while
the pack publishes FailedState.

Assisted-by: Claude Code (claude-opus-5)
@stephrobert
stephrobert merged commit 4a9c1b3 into main Nov 14, 2026
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The cold twin of #909: a boot puts our machine on the operator's own network

1 participant