Repository navigation
fix(security): a boot asks whose network it is, not only whether the name is well formed (#932) - #943
Merged
Merged
Conversation
…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)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Attachhas asked both questions since #909. The two cold paths that plug the same kind of NIC asked onlysafeName: the primary attachment insideStart, andattachExtra, which adds the extra NICs of a boot.incusbr0passessafeName. The name comes fromResource.Runtime, whichPUT /_feint/stateandfeint snapshot loadrestore verbatim, and every pack reads it straight off intoplan.Bootand thenSpec.Attachments. Measured at argv level before the fix: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, sinceDetachrefuses 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 atStart's entry over every attachment and atattachExtra's own entry. One named predicate rather than a check per call site, and the reasoning matters: the asymmetry this lot found, twosafeName-only checks against onesafeName && ownedNetworkcheck, 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
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
safeNamein the loop and keepownedNetwork, and nothing ininternal/core/machinewent red, because every row of the attack table named a networkownedNetworkalso 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/bandfnt-a b, and the proof is a before and after pair rather than a claim: the same mutation printedTEST STILL PASSEDbefore the rows and bites after them. The reviewer reproduced it independently in two trees differing by exactly those two lines.MUST-1:
attachExtrawas 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
poweronof an already-running machine whose stored plan names a foreign network is now refused before theInspect, andBinding.PowerOnpublishesFailedStatefor a machine the runtime is running. That is deliberate — a plan the driver cannot trust is a plan it does not execute — andTestARestartIsRefusedWhenThePlanNamesAForeignNetworkmeasures 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 onlyDefaultMachineNetworkreddensTestStartStillBootsOnTheNetworksTheEmulatorCreated/the_default_machine_network_Outscale_asks_forand leaves the sibling subtest and six other start tests green. Narrowing ownership to the derived shape would otherwise break Outscale's public-cloudVmpath 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 leavesTestStartRefusesAForeignNetworkOnAnExtraNICgreen, which is the honest reading of defence in depth. Butfalsify.pyhas 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
attachExtradirectly 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
mise run prepushef7e03ba-boot-asks-whose-network-it-is.json, 9 mutationspython3 tools/falsify/falsify.py --lint tools/falsify/specsgo test ./internal/core/machine/FEINT_VM=incus-ovn mise run conformance:leg -- runtimefnt-network, no surviving DHCP serviceMode, 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
mainthen gained #942. #942 touches no file underinternal/core/machine— checked withgit log --name-onlyon that merge, not assumed: it changedtools/falsify/falsify.py,mise.toml,tools/testplan/specs.goand the twoconformancepages. 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.mdstill 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 checkpasses (insideprepush, on the rebase)internal/corestill knows no provider: the predicate reads a prefix this layer derives, and no provider spelling enters itmise run testplanwas run, and what it printed was runWhen a model wrote a substantive part of this
Assisted-by:trailer names the tool and the model versionWhen 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 touchedN/A.
When a machine runtime is involved
FEINT_VM=incus-ovnrunnertests assert it at argv levelincus listandincus network listafter the leg, plus its own three closing verdictsLeft to their own issues
Attachment.Addressreaches argv unparsed on these paths, and it is the only address inSpecthat does. The "always one prefixed element" argument holds at the two sites this lot touches, and not on the round trip:restorePinnedAddressreads the value back off the device andconfigureGuestAddresspasses it toip address add <cidr>as a bare argv element.attachExtra's runtime-caused failure still returns withoutabandonStart, so the instance stays running while the pack publishesFailedState. This lot removed the name-caused version of that.Detachrefuses 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