From e108d07e63097c744ef2fb0e2dfa359110dca370 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Sat, 14 Nov 2026 18:51:17 +0100 Subject: [PATCH] fix(security): an address is routed only onto an instance the emulator 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) --- internal/core/machine/incus_address.go | 83 ++++-- .../machine/incus_address_ownership_test.go | 272 ++++++++++++++++++ internal/core/machine/incus_ovn.go | 21 ++ .../specs/public-address-migration.json | 6 +- .../route-address-asks-whose-instance.json | 62 ++++ tools/falsify/specs/routed-nic.json | 6 +- 6 files changed, 423 insertions(+), 27 deletions(-) create mode 100644 internal/core/machine/incus_address_ownership_test.go create mode 100644 tools/falsify/specs/route-address-asks-whose-instance.json diff --git a/internal/core/machine/incus_address.go b/internal/core/machine/incus_address.go index ff5a16c2..23d2f234 100644 --- a/internal/core/machine/incus_address.go +++ b/internal/core/machine/incus_address.go @@ -28,6 +28,35 @@ func (d *Incus) RouteAddress(ctx context.Context, spec AddressSpec) error { if !safeName.MatchString(spec.Machine) { return fmt.Errorf("invalid machine name %q", spec.Machine) } + // Is this instance ours — asked once, here, for every branch below (#933). + // + // Each branch reconfigures the instance: the routed one edits its NIC's + // ipv4.routes, the migration releases the address from it, and the two + // mode branches set a route key on a device and `exec` into the guest. + // safeName above has said the name could be a command argument, never + // that the instance is ours, and the name arrives from Resource.Runtime, + // which PUT /_feint/state and `feint snapshot load` restore verbatim into + // a format documented as meant to outlive its instance. + // + // At the entry rather than in each branch, because per-branch is exactly + // what failed: the routed branch asked this, the managed branch asked + // mustOwn of the *network* instead — which says nothing about the + // instance whose device it edits — and a foreign instance holding one NIC + // on an emulated network took `config device set eth1 + // ipv4.routes=203.0.113.9/32` and the `ip address add` behind it, + // measured through the runner — and in OVN mode the same gap wrote the + // uplink route, then the guest's own private address, resolver and + // routes, through `exec` into somebody else's container. + // + // One call before anything is sent has no second copy to drift, and it is + // the one call the three tests below measure, one per branch: + // TestRouteAddressRefusesAManagedNICOnAnInstanceTheEmulatorDidNotCreate, + // TestRouteAddressRefusesARoutedMachineTheEmulatorDidNotCreate and + // TestMigrationRefusesAnInstanceTheEmulatorDidNotCreate all fail without + // it. + if err := d.mustOwnInstance(ctx, spec.Machine); err != nil { + return err + } network, device, err := d.interfaceFor(ctx, spec.Machine, spec.Network) if err != nil { return err @@ -35,15 +64,11 @@ func (d *Incus) RouteAddress(ctx context.Context, spec AddressSpec) error { if network == "" { // A routed NIC (#202): the interface of a machine that joins no // emulated network. There is no network whose ownership could vouch for - // this call, so the question moves to the object actually reconfigured - // — the instance — and its label answers it. The mechanism is the NIC's - // own in both driver modes: a routed NIC has no OVN port, so the OVN - // forward machinery below has nothing to say about it. - // TestRouteAddressRefusesARoutedMachineTheEmulatorDidNotCreate fails - // without the ownership half. - if err := d.mustOwnInstance(ctx, spec.Machine); err != nil { - return err - } + // this call, which is why the question above is asked of the object + // actually reconfigured — the instance — and answered by the label the + // emulator itself wrote. The mechanism is the NIC's own in both driver + // modes: a routed NIC has no OVN port, so the OVN forward machinery + // below has nothing to say about it. return d.routeOntoRoutedNIC(ctx, spec.Machine, device, spec.Address) } // The machine answers on this address through a routed NIC, and the pack @@ -88,7 +113,9 @@ func (d *Incus) RouteAddress(ctx context.Context, spec AddressSpec) error { // create — would leave the machine having lost the address it // answered on, for a route it never got. A network name reaching here // comes from the pack's Plan.RouteVia, which is built from stored - // values a restored snapshot controls. + // values a restored snapshot controls. The other half of the + // question, whether the instance being reconfigured is ours, is + // answered at the entry of this function. // TestAMigrationIsNotStartedForANetworkTheEmulatorDoesNotOwn fails // without this. if err := d.mustOwn(ctx, network); err != nil { @@ -105,7 +132,10 @@ func (d *Incus) RouteAddress(ctx context.Context, spec AddressSpec) error { } // Never route through a network the emulator did not create: an operator's // own bridge is not ours to reconfigure, and a route left on it would - // survive every sweep. + // survive every sweep. Whose instance the device below belongs to is the + // other question, answered at the entry. + // TestRouteAddressRefusesAManagedNICOnANetworkTheEmulatorDoesNotOwn fails + // without this. if err := d.mustOwn(ctx, network); err != nil { return err } @@ -210,17 +240,28 @@ func (d *Incus) routedNICCarrying(ctx context.Context, machine, address string) // TestAHotMigrationTellsNetworkdToLetGoBeforeTheRePlug and // TestAColdMigrationDoesNotWaitForTheGuest fail without the two halves. // -// Ownership before shape, and this call is why: safeName has said the name -// could be a command argument, never that the instance is ours, and this -// reconfigures an instance's devices from a name a restored snapshot controls. -// RouteAddress's routed branch asks the same question for the same reason; the -// managed branch below asks it of the network instead, which says nothing -// about the instance whose device this edits. -// TestMigrationRefusesAnInstanceTheEmulatorDidNotCreate fails without it. +// Whether the instance is ours is not asked again here, and that is +// deliberate (#933). RouteAddress is this function's only caller and asks it at +// its entry, ahead of every branch; a second call on the same path could never +// fail, so no mutation could measure it. +// +// Being unmeasurable is not on its own what decided it, and the narrower rule +// is worth stating because this driver does keep checks of that kind: +// routeAddressOVN re-asks safeName of a name RouteAddress has already checked, +// and attachExtra re-asks ownedAttachments of a slice Start has already +// checked. Both are pure local predicates — free, and unable to answer +// differently the second time — and both say in place that they are defence in +// depth rather than the guard. mustOwnInstance is not one of those: it is an +// `incus config get` on the host, so a duplicate is a second command and a +// second reading of a label, and on this single path it can answer nothing the +// reading at the entry has not already answered. So what #933 removed is a +// duplicated runtime probe, not redundancy as such. +// +// A caller added later answers the question itself, as RouteAddress does, +// before reconfiguring anybody's device. +// TestMigrationRefusesAnInstanceTheEmulatorDidNotCreate holds the refusal, and +// it fails when the entry call is neutralised. func (d *Incus) releaseFromRoutedNIC(ctx context.Context, machine, device, address string) error { - if err := d.mustOwnInstance(ctx, machine); err != nil { - return err - } devices, err := d.instanceDevices(ctx, machine) if err != nil { return fmt.Errorf("inspect %s: %w", machine, err) diff --git a/internal/core/machine/incus_address_ownership_test.go b/internal/core/machine/incus_address_ownership_test.go new file mode 100644 index 00000000..ee39862b --- /dev/null +++ b/internal/core/machine/incus_address_ownership_test.go @@ -0,0 +1,272 @@ +package machine + +import ( + "context" + "errors" + "fmt" + "strings" + "testing" +) + +// RouteAddress had three branches and asked the ownership question in two of +// them (#933). +// +// The routed branch asked it of the instance, the migration asked it of the +// network before releasing the address, and the managed branch — both mode +// halves of it — asked it of the network only, then wrote to the instance: +// `config device set eth1 ipv4.routes=
/32` and the +// `ip address add` behind it, on an instance carrying none of the emulator's +// labels. Measured through the injectable runner, which is the only place an +// argument-level fact like that is visible before it hurts. +// +// The machine name arrives from Resource.Runtime, restored verbatim by +// PUT /_feint/state and `feint snapshot load`, so it is an input from outside +// however internal it looks. +// +// These hold the two questions and the shape check, one test per question, +// each driving a value that passes every other question and fails only its +// own: a row that fails two questions at once cannot tell them apart, and +// neutralising one of them would change no verdict (#909, #932). + +// routeRuntime answers a plausible Incus for one RouteAddress call on a +// managed network. +// +// The machine is every Scaleway server once its private NIC exists: a routed +// NIC carrying the launch address 203.0.113.4, and a managed NIC on `network`. +// The address the tests route is 203.0.113.9, which no routed NIC carries, so +// the call lands on the managed branch rather than on the migration. +// +// The two labels are the two ownership questions: `config get +// user.feint.provider` for the instance, `network get +// user.feint.provider` for the network. Either answers empty when the test +// says the object is not the emulator's, which is what the host's own objects +// answer. +func routeRuntime(machine, network string, oursInstance, oursNetwork bool) *fakeRuntime { + devices := fmt.Sprintf(`{ + "eth0": {"type": "nic", "nictype": "routed", + "ipv4.address": "203.0.113.4", "ipv4.host_address": "169.254.0.1"}, + "eth1": {"type": "nic", "network": %q, "ipv4.address": "10.30.1.10"} + }`, network) + instance := fmt.Sprintf(`{"devices": %s, "expanded_devices": %s}`, devices, devices) + + label := func(ours bool) []byte { + if ours { + return []byte("feint\n") + } + return []byte("\n") + } + f := &fakeRuntime{} + f.hook = func(_ int, args []string) ([]byte, error, bool) { + switch strings.Join(args, " ") { + case "query /1.0/instances/" + machine: + return []byte(instance), nil, true + case "config get " + machine + " user." + LabelKey: + return label(oursInstance), nil, true + case "network get " + network + " user." + LabelKey: + return label(oursNetwork), nil, true + case "network get " + network + " ipv4.address": + return []byte("10.30.1.1/24\n"), nil, true + case "network get " + DefaultUplinkName + " ipv4.routes": + return []byte("\n"), nil, true + case "config device get " + machine + " eth1 ipv4.routes": + return []byte("\n"), nil, true + case "list " + machine + " --format json": + return []byte(`[{"name":"` + machine + `","status":"Running","state":{"network":{}}}]`), nil, true + } + return nil, nil, false + } + return f +} + +// changesIssued returns the commands that change something on the host: +// everything that is not one of the read verbs the driver asks its questions +// with. A guard that returns an error after sending the command has guarded +// nothing, and this is what tells the two apart. +func changesIssued(f *fakeRuntime) []string { + var out []string + for _, cmd := range f.commands() { + read := false + for _, verb := range []string{"query ", "config get ", "network get ", "list "} { + if strings.HasPrefix(cmd, verb) { + read = true + } + } + if !read { + out = append(out, cmd) + } + } + return out +} + +// TestRouteAddressRefusesAManagedNICOnAnInstanceTheEmulatorDidNotCreate is the +// authorisation question, on the branch that did not ask it. +// +// `production-database` is well formed, so safeName lets it through, and the +// network it is routed through is one the emulator created and labelled, so +// the network question passes too: this row fails the instance question and +// nothing else. Only the label the emulator wrote can tell the instance from +// one of ours. +func TestRouteAddressRefusesAManagedNICOnAnInstanceTheEmulatorDidNotCreate(t *testing.T) { + const foreign = "production-database" + const network = "fnt-368798629f8" + for mode, ovn := range map[string]bool{"bridge mode": false, "ovn mode": true} { + t.Run(mode, func(t *testing.T) { + f := routeRuntime(foreign, network, false, true) + d := newFakeDriver(f) + d.OVN = ovn + + err := d.RouteAddress(context.Background(), AddressSpec{ + Machine: foreign, Address: "203.0.113.9", Network: network, + }) + if err == nil { + t.Fatal("an address was routed onto a device of an instance the emulator never created") + } + if changed := changesIssued(f); len(changed) != 0 { + t.Fatalf("the refusal came after the host was changed:\n%s", strings.Join(changed, "\n")) + } + // And the name reached no command but the question itself: a + // device set, an `exec` into the guest or a wait on its agent + // would each be the defect. + for _, cmd := range f.commands() { + if strings.Contains(cmd, foreign) && cmd != "config get "+foreign+" user."+LabelKey { + t.Errorf("a command reached the operator's own instance: %q\nall commands: %v", + cmd, f.commands()) + } + } + }) + } +} + +// TestRouteAddressRefusesAManagedNICOnANetworkTheEmulatorDoesNotOwn is the +// same pair of questions on the other name. +// +// `incusbr0` is the host's default bridge: well formed, so safeName lets it +// through, and the instance here is one of ours and carries the label, so this +// row fails the network question and nothing else. A route left on the +// operator's bridge would survive every sweep. +func TestRouteAddressRefusesAManagedNICOnANetworkTheEmulatorDoesNotOwn(t *testing.T) { + const mine = "feint-scw-mine" + for mode, ovn := range map[string]bool{"bridge mode": false, "ovn mode": true} { + t.Run(mode, func(t *testing.T) { + f := routeRuntime(mine, "incusbr0", true, false) + d := newFakeDriver(f) + d.OVN = ovn + + err := d.RouteAddress(context.Background(), AddressSpec{ + Machine: mine, Address: "203.0.113.9", Network: "incusbr0", + }) + if err == nil { + t.Fatal("an address was routed through the host's own bridge") + } + if changed := changesIssued(f); len(changed) != 0 { + t.Fatalf("the refusal came after the host was changed:\n%s", strings.Join(changed, "\n")) + } + for _, cmd := range f.commands() { + if strings.Contains(cmd, "incusbr0") && cmd != "network get incusbr0 user."+LabelKey { + t.Errorf("a command named the operator's own bridge: %q\nall commands: %v", + cmd, f.commands()) + } + } + }) + } +} + +// TestRouteAddressRefusesAMachineNameThatIsNotACommandArgument is the shape +// question, and it needs its own row because the two ownership questions above +// would accept this name: the instance it names is labelled as ours and so is +// the network, and only safeName refuses a name carrying a path separator. +// +// Nothing at all may be sent, not even a read: a name `incus` would read as a +// flag, or as another object's, must not become an argument of any command. +func TestRouteAddressRefusesAMachineNameThatIsNotACommandArgument(t *testing.T) { + const illFormed = "feint-scw-a/b" + const network = "fnt-368798629f8" + for mode, ovn := range map[string]bool{"bridge mode": false, "ovn mode": true} { + t.Run(mode, func(t *testing.T) { + f := routeRuntime(illFormed, network, true, true) + d := newFakeDriver(f) + d.OVN = ovn + + err := d.RouteAddress(context.Background(), AddressSpec{ + Machine: illFormed, Address: "203.0.113.9", Network: network, + }) + if err == nil { + t.Fatalf("a machine name no command may carry was accepted: %q", illFormed) + } + if len(f.calls) != 0 { + t.Fatalf("the name reached the runtime before the refusal:\n%s", + strings.Join(f.commands(), "\n")) + } + }) + } +} + +// TestRouteAddressStillRoutesOnAnOwnedInstanceAndNetwork is the accepting +// half, and it is half the point: a guard that refuses everything passes every +// test above and breaks the product. Two networks, because both must keep +// working — the one NetworkName derives for a pack's private network, and +// fnt-default, which is the network Outscale's public-cloud Vm path boots on. +func TestRouteAddressStillRoutesOnAnOwnedInstanceAndNetwork(t *testing.T) { + derived := NetworkName(NetworkPrefix, "0b9c1d2e-aaaa-bbbb-cccc-111122223333") + if len(derived) > MaxNetworkNameLen || !ownedNetwork(derived) { + t.Fatalf("NetworkName produced %q, which the runtime would reject or a guard would refuse", derived) + } + const mine = "feint-scw-mine" + for _, network := range []string{derived, DefaultMachineNetwork} { + for mode, ovn := range map[string]bool{"bridge mode": false, "ovn mode": true} { + t.Run(network+" in "+mode, func(t *testing.T) { + f := routeRuntime(mine, network, true, true) + d := newFakeDriver(f) + d.OVN = ovn + + if err := d.RouteAddress(context.Background(), AddressSpec{ + Machine: mine, Address: "203.0.113.9", Network: network, + }); err != nil { + t.Fatalf("routing through a network and an instance the emulator created must succeed, got %v", err) + } + wanted := []string{ + "config device set " + mine + " eth1 ipv4.routes=203.0.113.9/32", + "exec " + mine + " -- ip address add 203.0.113.9/32 dev eth1", + } + if ovn { + wanted = []string{ + "network set " + DefaultUplinkName + " ipv4.routes=203.0.113.9/32", + "config device set " + mine + " eth1 ipv4.routes.external=203.0.113.9/32", + } + } + for _, must := range wanted { + if len(f.matching(must)) == 0 { + t.Errorf("the address never reached the machine, missing %q in:\n%s", + must, strings.Join(f.commands(), "\n")) + } + } + }) + } + } +} + +// TestRouteAddressOnAnInstanceThatIsGoneIsReported holds the one thing an +// ownership probe must not turn into a success. The caller is about to publish +// this address to a client, so an instance that is not there is an error and +// not "nothing to do" — that tolerance belongs to UnrouteAddress, which has an +// uplink route to withdraw either way. The error keeps its meaning, because +// callers read it with isNotFound. +func TestRouteAddressOnAnInstanceThatIsGoneIsReported(t *testing.T) { + f := &fakeRuntime{fail: map[string]error{ + "config get feint-scw-gone": errors.New("Error: Instance not found"), + }} + d := newFakeDriver(f) + + err := d.RouteAddress(context.Background(), AddressSpec{ + Machine: "feint-scw-gone", Address: "203.0.113.9", Network: "fnt-368798629f8", + }) + if err == nil { + t.Fatal("routing an address to an instance that does not exist answered success") + } + if !isNotFound(err) { + t.Fatalf("the error lost its meaning, and callers read it with isNotFound: %v", err) + } + if changed := changesIssued(f); len(changed) != 0 { + t.Fatalf("a command changed something for an instance that is gone:\n%s", strings.Join(changed, "\n")) + } +} diff --git a/internal/core/machine/incus_ovn.go b/internal/core/machine/incus_ovn.go index ddb0b01c..ac878354 100644 --- a/internal/core/machine/incus_ovn.go +++ b/internal/core/machine/incus_ovn.go @@ -805,6 +805,27 @@ func (d *Incus) networkPeers(ctx context.Context, network string) ([]peerRow, er // puts them back; the observable effect is a brief interface bounce at attach // time, which real clouds also produce when reshaping a live NIC. func (d *Incus) routeAddressOVN(ctx context.Context, spec AddressSpec) error { + // Defence in depth, not the guard, and which of the two it is was measured + // rather than assumed (#933). RouteAddress is this function's only caller + // and asks safeName at its entry, so no mutation can redden this call: + // deleting it leaves every test in this package green, run on 2026-11-14. + // The refusal itself is held there, by + // TestRouteAddressRefusesAMachineNameThatIsNotACommandArgument, which + // requires that an ill-formed name reach no command at all. + // + // It stays for the reason attachExtra re-asks ownedAttachments of a slice + // Start has already checked: a caller added later is safe by construction + // instead of inheriting an obligation written in a comment, and the + // predicate is pure and local, so the second call costs nothing and cannot + // answer differently than the first. + // + // That purity is the line, and it is why the ownership probe #933 took out + // of releaseFromRoutedNIC was not kept on these same terms: mustOwnInstance + // is an `incus config get` on the host, so a duplicate is a second command + // and a second reading of a label, which buys nothing the reading at the + // entry has not already given on this one path. Every call site of + // mustOwnInstance in this driver is a public entry point of it, asked once, + // before anything is sent. if !safeName.MatchString(spec.Machine) { return fmt.Errorf("invalid machine name %q", spec.Machine) } diff --git a/tools/falsify/specs/public-address-migration.json b/tools/falsify/specs/public-address-migration.json index c28280c4..6ff76c81 100644 --- a/tools/falsify/specs/public-address-migration.json +++ b/tools/falsify/specs/public-address-migration.json @@ -59,10 +59,10 @@ "test": "TestAGuestWithoutNetworkdIsLeftAloneByTheMigration" }, { - "label": "the migration reconfigures an instance without asking whether the emulator created it", + "label": "the migration reconfigures an instance without asking whether the emulator created it. The question moved to the entry of RouteAddress with #933 \u2014 releaseFromRoutedNIC's only caller \u2014 so this is that call neutralised, judged by the migration's own test", "file": "internal/core/machine/incus_address.go", - "find": "func (d *Incus) releaseFromRoutedNIC(ctx context.Context, machine, device, address string) error {\n\tif err := d.mustOwnInstance(ctx, machine); err != nil {\n\t\treturn err\n\t}", - "replace": "func (d *Incus) releaseFromRoutedNIC(ctx context.Context, machine, device, address string) error {\n\tif err := d.mustOwnInstance(ctx, machine); err != nil && false {\n\t\treturn err\n\t}", + "find": "\tif err := d.mustOwnInstance(ctx, spec.Machine); err != nil {\n\t\treturn err\n\t}\n\tnetwork, device, err := d.interfaceFor(ctx, spec.Machine, spec.Network)", + "replace": "\tif err := d.mustOwnInstance(ctx, spec.Machine); err != nil && false {\n\t\treturn err\n\t}\n\tnetwork, device, err := d.interfaceFor(ctx, spec.Machine, spec.Network)", "test": "TestMigrationRefusesAnInstanceTheEmulatorDidNotCreate" }, { diff --git a/tools/falsify/specs/route-address-asks-whose-instance.json b/tools/falsify/specs/route-address-asks-whose-instance.json new file mode 100644 index 00000000..dca8f89c --- /dev/null +++ b/tools/falsify/specs/route-address-asks-whose-instance.json @@ -0,0 +1,62 @@ +{ + "package": "./internal/core/machine/", + "mutations": [ + { + "label": "RouteAddress stops asking whose instance it is, and the managed branch writes to it anyway: `config device set eth1 ipv4.routes=203.0.113.9/32` and the `ip address add` behind it, which is #933 as it was measured through the runner. Judged in bridge mode here and in OVN mode by the next mutation: one call, two mode branches, two witnesses", + "file": "internal/core/machine/incus_address.go", + "find": "\tif err := d.mustOwnInstance(ctx, spec.Machine); err != nil {\n\t\treturn err\n\t}\n\tnetwork, device, err := d.interfaceFor(ctx, spec.Machine, spec.Network)", + "replace": "\tif err := d.mustOwnInstance(ctx, spec.Machine); err != nil && false {\n\t\treturn err\n\t}\n\tnetwork, device, err := d.interfaceFor(ctx, spec.Machine, spec.Network)", + "test": "TestRouteAddressRefusesAManagedNICOnAnInstanceTheEmulatorDidNotCreate/bridge_mode" + }, + { + "label": "the same removal, byte-identical, judged by the OVN mode branch instead. It is worth its own witness because the OVN half writes more: the uplink route, the NIC's external route, and then the guest's own address, resolver and routes through `exec`, all on an instance carrying none of the emulator's labels", + "file": "internal/core/machine/incus_address.go", + "find": "\tif err := d.mustOwnInstance(ctx, spec.Machine); err != nil {\n\t\treturn err\n\t}\n\tnetwork, device, err := d.interfaceFor(ctx, spec.Machine, spec.Network)", + "replace": "\tif err := d.mustOwnInstance(ctx, spec.Machine); err != nil && false {\n\t\treturn err\n\t}\n\tnetwork, device, err := d.interfaceFor(ctx, spec.Machine, spec.Network)", + "test": "TestRouteAddressRefusesAManagedNICOnAnInstanceTheEmulatorDidNotCreate/ovn_mode" + }, + { + "label": "the shape half is neutralised and the ownership half kept, so a stored machine name carrying a path separator becomes an argument of `query`, `config device set` and `exec`. It needs its own mutation and its own row because the two ownership questions accept that name: the instance it names is labelled as ours and so is its network", + "file": "internal/core/machine/incus_address.go", + "find": "\tif !safeName.MatchString(spec.Machine) {\n\t\treturn fmt.Errorf(\"invalid machine name %q\", spec.Machine)", + "replace": "\tif !safeName.MatchString(spec.Machine) && false {\n\t\treturn fmt.Errorf(\"invalid machine name %q\", spec.Machine)", + "test": "TestRouteAddressRefusesAMachineNameThatIsNotACommandArgument" + }, + { + "label": "the managed branch stops asking whose network it is, so an address is routed through the operator's own bridge and the route left on it survives every sweep", + "file": "internal/core/machine/incus_address.go", + "find": "\tif err := d.mustOwn(ctx, network); err != nil {\n\t\treturn err\n\t}\n\n\tif err := d.setDeviceRoutes(ctx, spec.Machine, device, spec.Address, true); err != nil {", + "replace": "\tif err := d.mustOwn(ctx, network); err != nil && false {\n\t\treturn err\n\t}\n\n\tif err := d.setDeviceRoutes(ctx, spec.Machine, device, spec.Address, true); err != nil {", + "test": "TestRouteAddressRefusesAManagedNICOnANetworkTheEmulatorDoesNotOwn/bridge_mode" + }, + { + "label": "the same question in the OVN mode branch, which has its own call and therefore its own mutation: without it the uplink carries a /32 towards a network the emulator never created", + "file": "internal/core/machine/incus_ovn.go", + "find": "\tif err := d.mustOwn(ctx, network); err != nil {\n\t\treturn err\n\t}\n\t// Exclusive on the NIC's network for the whole edit.", + "replace": "\tif err := d.mustOwn(ctx, network); err != nil && false {\n\t\treturn err\n\t}\n\t// Exclusive on the NIC's network for the whole edit.", + "test": "TestRouteAddressRefusesAManagedNICOnANetworkTheEmulatorDoesNotOwn/ovn_mode" + }, + { + "label": "the ownership question refuses every instance, which passes all four attack tests above and breaks the product: no pack can publish an address any more, and the refusal returns a nil error so the caller is not even told", + "file": "internal/core/machine/incus_address.go", + "find": "\tif err := d.mustOwnInstance(ctx, spec.Machine); err != nil {\n\t\treturn err\n\t}\n\tnetwork, device, err := d.interfaceFor(ctx, spec.Machine, spec.Network)", + "replace": "\tif err := d.mustOwnInstance(ctx, spec.Machine); err != nil || len(spec.Machine) > 0 {\n\t\treturn err\n\t}\n\tnetwork, device, err := d.interfaceFor(ctx, spec.Machine, spec.Network)", + "test": "TestRouteAddressStillRoutesOnAnOwnedInstanceAndNetwork" + }, + { + "label": "the network question is narrowed to refuse fnt-default, the realistic future defect: every NetworkName-derived network keeps working and Outscale's public-cloud Vm path, which boots on DefaultMachineNetwork, stops getting its address", + "file": "internal/core/machine/incus_address.go", + "find": "\tif err := d.mustOwn(ctx, network); err != nil {\n\t\treturn err\n\t}\n\n\tif err := d.setDeviceRoutes(ctx, spec.Machine, device, spec.Address, true); err != nil {", + "replace": "\tif err := d.mustOwn(ctx, network); err != nil || network == DefaultMachineNetwork {\n\t\treturn err\n\t}\n\n\tif err := d.setDeviceRoutes(ctx, spec.Machine, device, spec.Address, true); err != nil {", + "test": "TestRouteAddressStillRoutesOnAnOwnedInstanceAndNetwork/fnt-default_in_bridge_mode" + }, + { + "label": "an instance that is gone is read as nothing to do, so RouteAddress answers success for a machine that will never carry the address its pack is about to publish", + "file": "internal/core/machine/incus_address.go", + "find": "\tif err := d.mustOwnInstance(ctx, spec.Machine); err != nil {\n\t\treturn err\n\t}\n\tnetwork, device, err := d.interfaceFor(ctx, spec.Machine, spec.Network)", + "replace": "\tif err := d.mustOwnInstance(ctx, spec.Machine); err != nil {\n\t\tif isNotFound(err) {\n\t\t\treturn nil\n\t\t}\n\t\treturn err\n\t}\n\tnetwork, device, err := d.interfaceFor(ctx, spec.Machine, spec.Network)", + "test": "TestRouteAddressOnAnInstanceThatIsGoneIsReported" + } + ], + "note": "#933. RouteAddress had three branches and asked whose instance it is in two of them; the managed branch asked mustOwn of the network only and then set a route key on the instance's device and `exec`ed into its guest. The question is asked once now, at the entry of RouteAddress, ahead of every branch and of every command: one named predicate, one call site, no second copy to drift, which is what the asymmetry of #932 cost. Two consequences this spec carries and a reader should not have to infer. The move displaced two mutations that used to name the per-branch calls, in routed-nic.json and public-address-migration.json; both are retargeted at the entry call and judged by their own branch's test, so the routed branch and the migration are still measured, each by the test that names it. And releaseFromRoutedNIC no longer asks the question itself: with the entry call in place a second check on the same path could never fail, so no mutation could measure it. Being unmeasurable is not on its own the reason, and the narrower rule is written where the code is: this driver does keep re-checks no mutation can redden when they are pure local predicates, routeAddressOVN re-asking safeName and attachExtra re-asking ownedAttachments both being that, and both saying in place that they are defence in depth rather than the guard. mustOwnInstance is an `incus config get` on the host, so what #933 took out is a duplicated runtime probe, not redundancy as such. What this spec does NOT hold is the position of the entry call, and what it does instead is refuse rather than judge. Four of these eight mutations anchor their fragment on the entry call followed by the interfaceFor line, so moving the call below interfaceFor does not pass quietly: the fragment stops matching, `mise run prepush` turns red on falsify:lint in about 2.5 s with 'fragment not found', and a replay of this spec prints DID NOT APPLY and exits 1. That is the harness catching a spec which describes code that is no longer what it thinks, which is the one failure it must never have silently, and it is a refusal, not a measurement of the move. What the move would actually cost, 'nothing is sent before the question', is held by the tests instead, which require that the only command naming a refused instance is the label probe itself, and that an ill-formed name reaches no command at all." +} diff --git a/tools/falsify/specs/routed-nic.json b/tools/falsify/specs/routed-nic.json index bbf45be2..b3227584 100644 --- a/tools/falsify/specs/routed-nic.json +++ b/tools/falsify/specs/routed-nic.json @@ -9,10 +9,10 @@ "test": "TestRouteAddressReachesARoutedNIC" }, { - "label": "the ownership question is no longer asked before routing into a routed machine", + "label": "the ownership question is no longer asked before routing into a routed machine. Since #933 the question lives at the entry of RouteAddress, where one call covers the three branches, so this is that call neutralised and judged by the routed branch's own test", "file": "internal/core/machine/incus_address.go", - "find": "\t\tif err := d.mustOwnInstance(ctx, spec.Machine); err != nil {\n\t\t\treturn err\n\t\t}\n\t\treturn d.routeOntoRoutedNIC(ctx, spec.Machine, device, spec.Address)", - "replace": "\t\tif err := d.mustOwnInstance(ctx, spec.Machine); err != nil && false {\n\t\t\treturn err\n\t\t}\n\t\treturn d.routeOntoRoutedNIC(ctx, spec.Machine, device, spec.Address)", + "find": "\tif err := d.mustOwnInstance(ctx, spec.Machine); err != nil {\n\t\treturn err\n\t}\n\tnetwork, device, err := d.interfaceFor(ctx, spec.Machine, spec.Network)", + "replace": "\tif err := d.mustOwnInstance(ctx, spec.Machine); err != nil && false {\n\t\treturn err\n\t}\n\tnetwork, device, err := d.interfaceFor(ctx, spec.Machine, spec.Network)", "test": "TestRouteAddressRefusesARoutedMachineTheEmulatorDidNotCreate" }, {