Repository navigation
fix(security): an address is routed only onto an instance the emulator created (#933) - #952
Merged
Merged
Conversation
…r created (#933) RouteAddress asked whose network a managed NIC sat on and never whose instance it was writing into. A crafted snapshot naming the operator's own instance on an emulator-owned network got its routes set and an address added inside it. The OVN branch had the same hole and wrote more: the uplink route, ipv4.routes.external, resolvectl dns, and a private address inside the operator's container. mustOwnInstance now runs at RouteAddress's entry, before interfaceFor, so no command names the instance before the question is asked. Every write path of the function goes through it: the routed NIC, the migration, OVN and the managed bridge each have RouteAddress as their only caller. The ownership probe in releaseFromRoutedNIC is removed: RouteAddress is its only caller, so a second runtime probe on the same path could never fail and no mutation could measure it. The safeName in routeAddressOVN stays: it is a pure local predicate, the kind #932 keeps as defence in depth, and the comment says so with the measurement that it cannot be falsified. Held by incus_address_ownership_test.go, one case per question, bridge and OVN: an instance that is not ours (TestRouteAddressRefusesARoutedMachine TheEmulatorDidNotCreate, TestMigrationRefusesAnInstanceTheEmulatorDidNot Create), a network that is not ours with an instance that is (TestRouteAddressRefusesAManagedNICOnANetworkTheEmulatorDoesNotOwn), a name that is not a command argument, and the accepting half including fnt-default. tools/falsify/specs/route-address-asks-whose-instance.json holds eight mutations, one per question; routed-nic.json and public-address-migration.json are retargeted onto the moved call. Measured: FEINT_VM=incus-ovn conformance:leg -- runtime green, 1505 s (1433 s on 2026-11-13); prepush green. Sibling defect found and left out: #947. Assisted-by: Claude Code (claude-opus-5-5)
11 of 15 tasks
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
RouteAddressasked whose network a managed NIC sat on, and never whose instance it was writing into. A crafted snapshot naming the operator's own instance on an emulator-owned network got routes set and an address added inside it. The OVN branch had the same hole and wrote more: the uplink route,ipv4.routes.external,resolvectl dns, and a private address inside the operator's container. The author found the OVN half by writing the tests first and running them against the unfixed code; the issue only described the bridge.mustOwnInstancenow runs atRouteAddress's entry, beforeinterfaceFor, so no command names the instance before the question is asked. Every write path goes through it: the routed NIC, the migration, OVN and the managed bridge each haveRouteAddressas their only caller (enumerated by the reviewer, re-checked by grep).Two deliberate choices, both reviewed:
releaseFromRoutedNICis removed.RouteAddressis its only caller, so a second probe on the same path could never fail and no mutation could measure it.safeNameinrouteAddressOVNstays. It is a pure local predicate, the kind The cold twin of #909: a boot puts our machine on the operator's own network #932 already keeps as defence in depth inattachExtra. Removing it was measured (package green without it), so the comment says it is unfalsifiable rather than implying it is held. The rule the lot first stated, "no guard that cannot fail", was too broad; both places that stated it now say that what goes is a duplicated runtime probe, not redundancy as such.Type of change
Checklist
Always
mise run checkpasses, throughmise run prepush(green, 77.92 s, after the last edit)internal/corestill knows no provider: the guard reads theuser.feint.providerlabel the driver itself writes, never a provider's spellingmise run testplanwas run, and what it printed was run:falsify:lint, the 15 falsify specs it names and theruntimeleg. All 15 specs exit 0 (163 mutations across this lot's replays, every one biting, every restoration green), plusroute-address-asks-whose-instance8/8,routed-nic10/10,public-address-migration15/15.When a model wrote a substantive part of this
Assisted-by:trailer names the tool and the model versionmise run conformancemyself: not the full pass. Whattestplanasks for this diff is theruntimeleg, and it was run (below). The client legs (scw, Terraform, OpenTofu,octl,exo) drive no machine with--vm offand do not reach this code; CI plays them on this pull request. What skipping the local full pass does not prove: nothing about this change, since no route or response shape moved.When a route is added or changed
N/A, no route changed.
When behaviour a client can observe changes
N/A: the change only refuses an instance the emulator did not create, which no legitimate client produces. No documented limit moves.
When
.github/workflows/is touchedN/A.
When a machine runtime is involved
FEINT_VM=incus-ovn:mise run conformance:leg -- runtimegreen, 1505 s on 2026-11-14 (1433 s on 2026-11-13; the gap was measured with two compiling agents on the same station). Real machines, a security group closing a port for real, authorise and revoke without restart, a reset putting the rule back on the NIC, a flexible IP closed by the group. The station was read first: 25 GB available of 46, no Incus instance running.incus listshows only the operator's ownbanc-vbox(stopped, untouched), and nofnt-network remains.Tests
internal/core/machine/incus_address_ownership_test.go, one case per question, in bridge and OVN mode, each value chosen to fail one question and pass the others:TestRouteAddressRefusesARoutedMachineTheEmulatorDidNotCreate,TestMigrationRefusesAnInstanceTheEmulatorDidNotCreate;incusbr0) with an instance that is:TestRouteAddressRefusesAManagedNICOnANetworkTheEmulatorDoesNotOwn;TestRouteAddressRefusesAMachineNameThatIsNotACommandArgument;fnt-defaultincluded:TestRouteAddressStillRoutesOnAnOwnedInstanceAndNetwork.tools/falsify/specs/route-address-asks-whose-instance.json: eight mutations, one per question plus the accepting half.Reviewed before the commit
security-challengeron the uncommitted tree: no blocker. It confirmed the removal is safe, the guard sits at the real choke point, both ownership questions are intact and now witnessed separately, and the new guard opens no availability window (the label is set inlaunch's arguments, not after boot). Its two SHOULDs: the unfalsifiablesafeName(settled above, in this lot) andplan.go:443not askingourson the address path, which is #947.Related issues
Closes #933.
Sibling defect, filed and left out: #947.
🤖 Generated with Claude Code