Skip to content

fix(security): an address is routed only onto an instance the emulator created (#933) - #952

Merged
stephrobert merged 1 commit into
mainfrom
fix/933-the-managed-branch-asks-the-instance
Nov 14, 2026
Merged

stephrobert merged 1 commit into
mainfrom
fix/933-the-managed-branch-asks-the-instance

Conversation

@stephrobert

Copy link
Copy Markdown
Owner

Summary

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 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.

mustOwnInstance now runs at RouteAddress's entry, before interfaceFor, 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 have RouteAddress as their only caller (enumerated by the reviewer, re-checked by grep).

Two deliberate choices, both reviewed:

  • The runtime probe in releaseFromRoutedNIC is removed. RouteAddress is its only caller, so a second 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 The cold twin of #909: a boot puts our machine on the operator's own network #932 already keeps as defence in depth in attachExtra. 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

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

Checklist

Always

  • mise run check passes, through mise run prepush (green, 77.92 s, after the last edit)
  • No new external Go dependency
  • internal/core still knows no provider: the guard reads the user.feint.provider label the driver itself writes, never a provider's spelling
  • Nothing in the diff could be written identically for another provider: it is in the shared driver, for all three packs at once
  • mise run testplan was run, and what it printed was run: falsify:lint, the 15 falsify specs it names and the runtime leg. All 15 specs exit 0 (163 mutations across this lot's replays, every one biting, every restoration green), plus route-address-asks-whose-instance 8/8, routed-nic 10/10, public-address-migration 15/15.

When a model wrote a substantive part of this

  • Assisted-by: trailer names the tool and the model version
  • I ran mise run conformance myself: not the full pass. What testplan asks for this diff is the runtime leg, and it was run (below). The client legs (scw, Terraform, OpenTofu, octl, exo) drive no machine with --vm off and 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.
  • Field names: N/A, no response shape changed

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 touched

N/A.

When a machine runtime is involved

  • Tested with FEINT_VM=incus-ovn: mise run conformance:leg -- runtime green, 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.
  • Nothing the emulator did not create was touched: that is what this pull request enforces, held by tests through the injectable runner, which assert that the only command naming a refused instance is the label probe itself.
  • The run leaves nothing behind, checked after the leg: incus list shows only the operator's own banc-vbox (stopped, untouched), and no fnt- 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:

  • an instance that is not ours, on a network that is: TestRouteAddressRefusesARoutedMachineTheEmulatorDidNotCreate, TestMigrationRefusesAnInstanceTheEmulatorDidNotCreate;
  • a network that is not ours (incusbr0) with an instance that is: TestRouteAddressRefusesAManagedNICOnANetworkTheEmulatorDoesNotOwn;
  • a name that is not a command argument: TestRouteAddressRefusesAMachineNameThatIsNotACommandArgument;
  • the accepting half, fnt-default included: TestRouteAddressStillRoutesOnAnOwnedInstanceAndNetwork.

tools/falsify/specs/route-address-asks-whose-instance.json: eight mutations, one per question plus the accepting half.

Reviewed before the commit

security-challenger on 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 in launch's arguments, not after boot). Its two SHOULDs: the unfalsifiable safeName (settled above, in this lot) and plan.go:443 not asking ours on the address path, which is #947.

Related issues

Closes #933.

Sibling defect, filed and left out: #947.

🤖 Generated with Claude Code

…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)
@stephrobert
stephrobert merged commit ecd639a 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.

RouteAddress's managed branch asks the network and never the instance it writes to

1 participant