From bbe9b751a9ee983dd4386ce15214551460dfea31 Mon Sep 17 00:00:00 2001 From: kacerr Date: Thu, 10 Sep 2026 22:31:34 +0000 Subject: [PATCH] Reject KVM host names and image keys that would break the identifiers they land in A host name is pasted into the providerID hosts//domains/, which parseProviderID splits on "/", so a name containing a slash produced an ID that never parsed again: Get, Delete and GC all failed permanently and the VM leaked with the Proxy's finalizer stuck. The image map key has the same defect and lands in a file name rather than an ID: base-. in the host's storage pool. Both now have to match a plain identifier pattern. Machine-type keys are left alone; they are only ever compared, never embedded. Co-Authored-By: Claude Opus 5 (1M context) --- docs/kvm-provider.md | 3 ++- .../2026-09-10-2222-kvm-review-fixes.md | 22 ++++++++++++++++++- internal/provider/config_kvm.go | 16 ++++++++++++++ internal/provider/config_kvm_test.go | 13 +++++++++++ 4 files changed, 52 insertions(+), 2 deletions(-) diff --git a/docs/kvm-provider.md b/docs/kvm-provider.md index f262f6b..96b8ef3 100644 --- a/docs/kvm-provider.md +++ b/docs/kvm-provider.md @@ -132,12 +132,13 @@ ssh virt@nas1.home.lan ip link show br0 | `insecureIgnoreHostKey` | | `false` | skip host-key verification entirely | | `diskSizeGb` | | `10` | capacity of each VM's overlay disk; cloud-init's growpart expands the root fs into it | | `vendorData` | | | extra cloud-init vendor-data placed on every seed next to the Proxy's user-data; cloud-init drops any top-level key here that the user-data also defines. The guest-agent bootstrap is not configured here, see *What lands on the host* | +| `images.` | | | the key names the image in `placement.image`. Letters, digits, dots, dashes and underscores only — it becomes the cached volume name `base-.` | | `images..url` | yes per entry | | http(s) URL of a cloud image (qcow2 or raw) | | `images..sha256` | | | expected digest; verified after download, a change triggers a re-download | | `images..format` | | `qcow2` | `qcow2` or `raw` | | `images..firmware` | | `uefi` | `uefi` (OVMF, secure boot off) or `bios` (SeaBIOS) for images without an EFI system partition; Debian 13 hangs in GRUB under BIOS on a headless VM, see *Known gaps* | | `machineTypes.` | ≥ 1 | | `{vcpus, memoryMiB}` size profile | -| `hosts[].name` | yes | | the `placement.zone` value that selects this host; unique | +| `hosts[].name` | yes | | the `placement.zone` value that selects this host; unique. Letters, digits, dots, dashes and underscores only — it becomes part of the providerID `hosts//domains/` | | `hosts[].address` | yes | | SSH endpoint, `host:port` | | `hosts[].user` | yes | | SSH user, member of the `libvirt` group | | `hosts[].socket` | | `/var/run/libvirt/libvirt-sock` | libvirt socket path on the host | diff --git a/docs/plans-executions/2026-09-10-2222-kvm-review-fixes.md b/docs/plans-executions/2026-09-10-2222-kvm-review-fixes.md index ea4bc6d..8dfc80d 100644 --- a/docs/plans-executions/2026-09-10-2222-kvm-review-fixes.md +++ b/docs/plans-executions/2026-09-10-2222-kvm-review-fixes.md @@ -3,7 +3,7 @@ Plan: [docs/plans/2026-09-10-2222-kvm-review-fixes.md](../plans/2026-09-10-2222- - [x] Step 0 — Save plan into the repo - [x] Step 1 — Unwedge deletion (finding 1) -- [ ] Step 2 — Validate names that become identifiers (finding 2) +- [x] Step 2 — Validate names that become identifiers (finding 2) - [ ] Step 3 — Read the SSH material lazily (finding 3) - [ ] Step 4 — Guest agent via vendor-data script (findings 4 and 5) - [ ] Step 5 — Validate, record, push @@ -45,6 +45,26 @@ that host, with the originals still running where GC cannot see them, since `ListByTag` only sweeps configured hosts. The asymmetry is now pinned by a test that asserts both halves at once. +## Step 2 — Validate names that become identifiers + +One regexp, `^[A-Za-z0-9][A-Za-z0-9_.-]*$`, applied to `hosts[].name` and to +the `images` map keys. Dots stay legal because a host name like +`nas1.home.lan` is ordinary and harmless; the character that actually +breaks things is `/`. + +The review only reported the host name. The image key has the same defect +and is arguably worse: it is not merely embedded in an ID but pasted into a +file name, `base-.`, so `../etc/x` would be a path traversal +into the storage pool rather than a parse failure. Both are validated in the +same pass. Machine-type keys are left alone deliberately — they are only +ever compared against `placement.machineType`, never embedded anywhere. + +Worth noting: this is validation of the operator's own config file, which +only a cluster admin can write, so the traversal was never a privilege +boundary. It is still cheaper to reject at load than to explain a stuck +Proxy later. Both new table cases assert the message text, so the wording +that names the consequence is now pinned by tests. + Worth noting: the review filed this as a KVM finding, but the fix is mostly in the controller, because KVM is the only provider that resolves a providerID segment against operator config at all. GCP and Kubernetes hand diff --git a/internal/provider/config_kvm.go b/internal/provider/config_kvm.go index 4a094e8..d8010f1 100644 --- a/internal/provider/config_kvm.go +++ b/internal/provider/config_kvm.go @@ -83,6 +83,14 @@ type KVMHost struct { var sha256Hex = regexp.MustCompile(`^[0-9a-fA-F]{64}$`) +// kvmIdentifier constrains the config names that are pasted into +// identifiers elsewhere: a host name goes into the providerID +// hosts//domains/, which is split on "/" to parse, and an image +// key becomes the file name of the cached base volume. A slash in either +// produces an ID or a path that never resolves again, so the VM leaks with +// nothing able to address it. +var kvmIdentifier = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9_.-]*$`) + func validateKVM(prefix string, c *KVMConfig) error { if c == nil { return fmt.Errorf("%s: kvm block is required", prefix) @@ -98,6 +106,10 @@ func validateKVM(prefix string, c *KVMConfig) error { } for name, img := range c.Images { p := fmt.Sprintf("%s: kvm.images[%q]", prefix, name) + if !kvmIdentifier.MatchString(name) { + return fmt.Errorf("%s: key must be letters, digits, dots, dashes or underscores"+ + " — it becomes the cached volume name base-.", p) + } u, err := url.Parse(img.URL) if err != nil || (u.Scheme != "http" && u.Scheme != "https") || u.Host == "" { return fmt.Errorf("%s.url must be an http(s) URL (got %q)", p, img.URL) @@ -129,6 +141,10 @@ func validateKVM(prefix string, c *KVMConfig) error { if h.Name == "" { return fmt.Errorf("%s: name is required", p) } + if !kvmIdentifier.MatchString(h.Name) { + return fmt.Errorf("%s: name %q must be letters, digits, dots, dashes or underscores"+ + " — it becomes part of the providerID hosts//domains/", p, h.Name) + } if seen[h.Name] { return fmt.Errorf("%s: duplicate host name %q", p, h.Name) } diff --git a/internal/provider/config_kvm_test.go b/internal/provider/config_kvm_test.go index 7438e38..c44ca2c 100644 --- a/internal/provider/config_kvm_test.go +++ b/internal/provider/config_kvm_test.go @@ -129,6 +129,19 @@ hosts: yaml: minimal("sshKeyFile: /key\nknownHostsFile: /kh\nmachineTypes:\n small: {vcpus: 1, memoryMiB: 1024}\nhosts:\n - {name: nas1, address: a:22, user: virt, bridge: br0}\n - {name: nas1, address: b:22, user: virt, bridge: br0}\n"), wantErrSub: `duplicate host name "nas1"`, }, + { + // A slash would produce hosts/dc1/rack2/domains/, which + // parseProviderID can never split back, so the VM would be + // unreachable for Get, Delete and GC alike. + name: "host name with a slash", + yaml: minimal("sshKeyFile: /key\nknownHostsFile: /kh\nmachineTypes:\n small: {vcpus: 1, memoryMiB: 1024}\nhosts:\n - {name: dc1/rack2, address: a:22, user: virt, bridge: br0}\n"), + wantErrSub: `name "dc1/rack2" must be letters, digits, dots, dashes or underscores`, + }, + { + name: "image key with a slash", + yaml: minimal("sshKeyFile: /key\nknownHostsFile: /kh\nimages:\n ../etc/d: {url: https://x/y.qcow2}\n" + hostsAndTypes), + wantErrSub: `images["../etc/d"]: key must be letters, digits, dots, dashes or underscores`, + }, { name: "host missing address", yaml: minimal("sshKeyFile: /key\nknownHostsFile: /kh\nmachineTypes:\n small: {vcpus: 1, memoryMiB: 1024}\nhosts:\n - {name: nas1, user: virt, bridge: br0}\n"),