From caa6a2b996d2f92b10cf7f0a32d9917c1a316e31 Mon Sep 17 00:00:00 2001 From: Thales Maciel Date: Thu, 23 Apr 2026 14:06:40 -0300 Subject: [PATCH] model: validate VM names as DNS labels at CLI + daemon MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A VM name flows into five places that all have narrower grammars than "arbitrary string": - the guest's /etc/hostname (vm_disk.patchRootOverlay) - the guest's /etc/hosts (same) - the .vm DNS record (vmdns.RecordName) - the kernel command line (system.BuildBootArgs*) - VM-dir file-path fragments (layout.VMsDir/, etc.) Nothing in the chain was validating the input. A name with whitespace, newline, dot, slash, colon, or = would produce broken hostnames, weird DNS labels, smuggled kernel cmdline tokens, or (in the worst case) surprising traversal through the on-disk layout. Not host shell injection — we already avoid shelling out with the raw name — but a real correctness and supportability bug. New: model.ValidateVMName. Rules: - 1..63 chars (DNS label max per RFC 1123; also a comfortable /etc/hostname cap) - lowercase ASCII letters, digits, '-' only - no leading or trailing '-' - no normalization — the name is the user-visible identifier (store key, `ssh .vm`, `vm show`); silently rewriting "MyVM" → "myvm" would hand the user back something different than they typed Called from two places: - internal/cli/commands_vm.go vmCreateParamsFromFlags — rejects bad `--name` values before any RPC. Empty name still passes through so the daemon can generate one. - internal/daemon/vm_create.go reserveVM — defense in depth for any non-CLI RPC caller (SDK, direct JSON over the socket). Tests: - internal/model/vm_name_test.go — exhaustive character-class matrix (space, newline, tab, dot, slash, colon, equals, quote, control chars, unicode letters, uppercase, leading/trailing hyphen, over-length, max-length-exact, digits-only). - internal/cli TestVMCreateParamsFromFlagsRejectsInvalidName — CLI wire-through + empty-name passthrough. - internal/daemon TestReserveVMRejectsInvalidName — daemon defense-in-depth (including `box/../evil` path-traversal). - scripts/smoke.sh — end-to-end rejection + no-leaked-row assertion. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/cli/cli_test.go | 41 +++++++++++++++++++ internal/cli/commands_vm.go | 5 +++ internal/daemon/vm_create.go | 8 ++++ internal/daemon/vm_create_test.go | 37 +++++++++++++++++ internal/model/vm_name.go | 45 ++++++++++++++++++++ internal/model/vm_name_test.go | 68 +++++++++++++++++++++++++++++++ scripts/smoke.sh | 21 ++++++++++ 7 files changed, 225 insertions(+) create mode 100644 internal/model/vm_name.go create mode 100644 internal/model/vm_name_test.go diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 38df3f3..faef9a9 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -443,6 +443,47 @@ func TestVMCreateParamsFromFlagsRejectsNonPositive(t *testing.T) { } } +func TestVMCreateParamsFromFlagsRejectsInvalidName(t *testing.T) { + cmd := NewBangerCommand() + vm, _, err := cmd.Find([]string{"vm"}) + if err != nil { + t.Fatalf("find vm: %v", err) + } + create, _, err := vm.Find([]string{"create"}) + if err != nil { + t.Fatalf("find create: %v", err) + } + + // A sampling of failure modes; the exhaustive character-class + // matrix lives in internal/model/vm_name_test.go. Here we just + // prove the CLI wires the validator in and surfaces its errors + // before any RPC call is made. + cases := []struct { + name string + input string + }{ + {"space", "my box"}, + {"uppercase", "MyBox"}, + {"dot", "box.vm"}, + {"leading hyphen", "-box"}, + {"newline", "my\nbox"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if _, err := vmCreateParamsFromFlags(create, tc.input, "", 2, 1024, "8G", "8G", false, false); err == nil { + t.Fatalf("vmCreateParamsFromFlags(%q) = nil error, want rejection", tc.input) + } + }) + } + + // Empty name must STILL be accepted at the CLI layer — the daemon + // generates one when the flag is unset. Rejecting here would + // break `banger vm create` with no --name. + if _, err := vmCreateParamsFromFlags(create, "", "", 2, 1024, "8G", "8G", false, false); err != nil { + t.Fatalf("vmCreateParamsFromFlags(empty name) = %v, want nil (daemon generates)", err) + } +} + func TestVMCreateParamsFromFlagsIncludesChangedDiskFlags(t *testing.T) { cmd := NewBangerCommand() vm, _, err := cmd.Find([]string{"vm"}) diff --git a/internal/cli/commands_vm.go b/internal/cli/commands_vm.go index 62b195b..f4c31fd 100644 --- a/internal/cli/commands_vm.go +++ b/internal/cli/commands_vm.go @@ -925,6 +925,11 @@ func vmCreateParamsFromFlags(cmd *cobra.Command, name, imageName string, vcpu, m // command-build time, so we always forward the flag values. The CLI // becomes the single source of truth for effective defaults and the // progress renderer shows the exact sizing. + if strings.TrimSpace(name) != "" { + if err := model.ValidateVMName(name); err != nil { + return api.VMCreateParams{}, err + } + } if err := validatePositiveSetting("vcpu", vcpu); err != nil { return api.VMCreateParams{}, err } diff --git a/internal/daemon/vm_create.go b/internal/daemon/vm_create.go index 0c468da..8946228 100644 --- a/internal/daemon/vm_create.go +++ b/internal/daemon/vm_create.go @@ -115,6 +115,14 @@ func (s *VMService) reserveVM(ctx context.Context, requestedName string, image m } name = generated } + // Defense in depth: CLI has already validated the flag, but any + // other RPC caller (SDK, direct JSON over the socket) lands here + // without going through the CLI flag parser. The name flows into + // /etc/hostname, kernel boot args, DNS records, and file paths — + // it has to be DNS-label-safe. + if err := model.ValidateVMName(name); err != nil { + return model.VMRecord{}, err + } // Exact-name lookup. Using FindVM here would also match a new name // that merely prefixes some existing VM's id or another VM's name, // falsely rejecting perfectly valid names. diff --git a/internal/daemon/vm_create_test.go b/internal/daemon/vm_create_test.go index 78c2690..2517033 100644 --- a/internal/daemon/vm_create_test.go +++ b/internal/daemon/vm_create_test.go @@ -86,3 +86,40 @@ func TestReserveVMRejectsExactDuplicateName(t *testing.T) { t.Fatalf("err = %v, want 'already exists'", err) } } + +// TestReserveVMRejectsInvalidName pins defense-in-depth: the CLI +// already validates, but any other RPC caller (banger SDK, direct +// JSON over the socket) lands here without going through the CLI. +// The name ends up in /etc/hostname, kernel boot args, DNS records, +// and file paths — the daemon must refuse anything that isn't a +// valid DNS label. +func TestReserveVMRejectsInvalidName(t *testing.T) { + ctx := context.Background() + tmp := t.TempDir() + d := &Daemon{ + store: openDaemonStore(t), + layout: paths.Layout{VMsDir: filepath.Join(tmp, "vms"), RuntimeDir: filepath.Join(tmp, "runtime")}, + config: model.DaemonConfig{BridgeIP: model.DefaultBridgeIP}, + } + wireServices(d) + + image := testImage("image-x") + image.ID = "image-x" + image.Name = "image-x" + if err := d.store.UpsertImage(ctx, image); err != nil { + t.Fatalf("UpsertImage: %v", err) + } + + for _, bad := range []string{ + "MyBox", // uppercase + "my box", // space + "my.box", // dot + "box\n", // newline + "-box", // leading hyphen + "box/../evil", // path separator + traversal + } { + if _, err := d.vm.reserveVM(ctx, bad, image, model.VMSpec{VCPUCount: 1, MemoryMiB: 128}); err == nil { + t.Fatalf("reserveVM(%q) = nil error, want rejection", bad) + } + } +} diff --git a/internal/model/vm_name.go b/internal/model/vm_name.go new file mode 100644 index 0000000..c45a43d --- /dev/null +++ b/internal/model/vm_name.go @@ -0,0 +1,45 @@ +package model + +import ( + "errors" + "fmt" +) + +// MaxVMNameLen is the upper bound on a user-provided VM name. DNS +// labels (RFC 1123) allow up to 63 octets; the name ends up as the +// first label of `.vm` records served by banger's vmdns, and +// also as the guest's /etc/hostname — so fitting both invariants in +// a single ceiling keeps the model simple. +const MaxVMNameLen = 63 + +// ValidateVMName rejects names that aren't safe to use as a DNS +// label, a Linux hostname, a kernel-command-line token, or a +// file-path component. Concretely: lowercase ASCII letters, digits, +// and '-', 1..MaxVMNameLen chars, no leading or trailing hyphen. +// +// No normalization (trimming, case folding) — the VM name becomes +// the user-visible identifier (store lookup key, `ssh .vm`, +// `vm show `), and a silent rewrite would hand the user back +// a different name than they typed. Reject early with an explicit +// message instead. +func ValidateVMName(name string) error { + if name == "" { + return errors.New("vm name is required") + } + if len(name) > MaxVMNameLen { + return fmt.Errorf("vm name %q is %d characters; max is %d (DNS label limit)", name, len(name), MaxVMNameLen) + } + if name[0] == '-' || name[len(name)-1] == '-' { + return fmt.Errorf("vm name %q cannot start or end with '-'", name) + } + for i, r := range name { + switch { + case r >= 'a' && r <= 'z': + case r >= '0' && r <= '9': + case r == '-': + default: + return fmt.Errorf("vm name %q has invalid character %q at position %d (allowed: lowercase a-z, 0-9, '-')", name, r, i) + } + } + return nil +} diff --git a/internal/model/vm_name_test.go b/internal/model/vm_name_test.go new file mode 100644 index 0000000..656837e --- /dev/null +++ b/internal/model/vm_name_test.go @@ -0,0 +1,68 @@ +package model + +import ( + "strings" + "testing" +) + +func TestValidateVMName(t *testing.T) { + cases := []struct { + name string + input string + wantOK bool + wantErrSub string + }{ + // Happy path. + {"simple", "mybox", true, ""}, + {"with-hyphen", "my-box", true, ""}, + {"digits", "box-123", true, ""}, + {"digits-only", "1234", true, ""}, + {"single-char", "a", true, ""}, + {"max length", strings.Repeat("a", MaxVMNameLen), true, ""}, + {"namegen style", "ace-fox", true, ""}, + + // Empty / length. + {"empty", "", false, "required"}, + {"over max length", strings.Repeat("a", MaxVMNameLen+1), false, "max is"}, + + // Hyphen position. + {"leading hyphen", "-box", false, "cannot start or end with '-'"}, + {"trailing hyphen", "box-", false, "cannot start or end with '-'"}, + {"lone hyphen", "-", false, "cannot start or end with '-'"}, + + // Character class. + {"uppercase", "MyBox", false, "invalid character"}, + {"space", "my box", false, "invalid character"}, + {"newline", "my\nbox", false, "invalid character"}, + {"tab", "my\tbox", false, "invalid character"}, + {"dot", "my.box", false, "invalid character"}, + {"dot-vm suffix", "box.vm", false, "invalid character"}, + {"slash", "my/box", false, "invalid character"}, + {"underscore", "my_box", false, "invalid character"}, + {"at sign", "user@box", false, "invalid character"}, + {"colon (kernel cmdline separator)", "my:box", false, "invalid character"}, + {"equals (kernel cmdline)", "a=b", false, "invalid character"}, + {"quote", "my\"box", false, "invalid character"}, + {"unicode letter", "box-α", false, "invalid character"}, + {"leading space", " box", false, "invalid character"}, + {"trailing space", "box ", false, "invalid character"}, + {"control char NUL", "my\x00box", false, "invalid character"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := ValidateVMName(tc.input) + if tc.wantOK { + if err != nil { + t.Fatalf("ValidateVMName(%q) = %v, want nil", tc.input, err) + } + return + } + if err == nil { + t.Fatalf("ValidateVMName(%q) = nil, want error containing %q", tc.input, tc.wantErrSub) + } + if !strings.Contains(err.Error(), tc.wantErrSub) { + t.Fatalf("ValidateVMName(%q) = %v, want error containing %q", tc.input, err, tc.wantErrSub) + } + }) + } +} diff --git a/scripts/smoke.sh b/scripts/smoke.sh index 3803f4b..616d062 100755 --- a/scripts/smoke.sh +++ b/scripts/smoke.sh @@ -561,6 +561,27 @@ set -e post_vms="$("$BANGER" vm list --all 2>/dev/null | wc -l)" [[ "$pre_vms" == "$post_vms" ]] || die "invalid spec leaked a VM row: pre=$pre_vms, post=$post_vms" +# --- invalid name rejection ------------------------------------------ +# VM names become DNS labels, guest hostnames, kernel-cmdline tokens +# and file-path fragments — the validator (ValidateVMName) must reject +# anything that isn't [a-z0-9-] with no leading/trailing hyphen and no +# dots. Smoke covers a few of the worst offenders end-to-end through +# the CLI; the full character-class matrix lives in +# internal/model/vm_name_test.go. Rejected names must also leave no +# VM row behind. +log 'invalid name rejection: uppercase / space / dot / leading-hyphen must all fail' +pre_vms="$("$BANGER" vm list --all 2>/dev/null | wc -l)" +for bad in 'MyBox' 'my box' 'box.vm' '-box'; do + set +e + "$BANGER" vm create --name "$bad" --no-start >/dev/null 2>&1 + rc=$? + set -e + [[ "$rc" -ne 0 ]] || die "invalid name: vm create accepted '$bad'" +done +post_vms="$("$BANGER" vm list --all 2>/dev/null | wc -l)" +[[ "$pre_vms" == "$post_vms" ]] \ + || die "invalid name leaked VM row(s): pre=$pre_vms, post=$post_vms" + # --- daemon stop (flushes coverage) ----------------------------------- log 'stopping daemon so instrumented binaries flush coverage' "$BANGER" daemon stop >/dev/null 2>&1 || true