diff --git a/cmd/vm/clone.go b/cmd/vm/clone.go index c80da7e..60a4a00 100644 --- a/cmd/vm/clone.go +++ b/cmd/vm/clone.go @@ -30,8 +30,8 @@ func (h *Handler) Clone(cmd *cobra.Command, args []string) error { netMode, _ := cmd.Flags().GetString("net") vnc, _ := cmd.Flags().GetInt("vnc") vncPass, _ := cmd.Flags().GetString("vnc-password") - if netMode == netCNI && vnc >= 0 && vncPass == "" { // same pre-scaffold check as create - return errCNIVNCPassRequired + if err = requireCNIVNCPassword(netMode == netCNI, vnc, vncPass); err != nil { // same pre-scaffold check as create + return err } // the clone inherits SRC's data disks by name; extra --data-disk specs must not collide with them // and the combined count still honors the AHCI cap. Parse before scaffolding to fail fast. diff --git a/cmd/vm/handler.go b/cmd/vm/handler.go index da3893e..445bdc1 100644 --- a/cmd/vm/handler.go +++ b/cmd/vm/handler.go @@ -37,7 +37,7 @@ type record struct { CPUs int `json:"cpus"` Memory string `json:"memory"` VNCDisp int `json:"vnc"` - VNCPass string `json:"vnc_password,omitempty"` + VNCPass string `json:"-"` // launch-scoped, set from the flag each start; never persisted (would leak at rest) SSHPort int `json:"ssh_port"` NetMode string `json:"net_mode,omitempty"` Hugepages bool `json:"hugepages,omitempty"` diff --git a/cmd/vm/helper.go b/cmd/vm/helper.go index 90e27d5..5cdee8e 100644 --- a/cmd/vm/helper.go +++ b/cmd/vm/helper.go @@ -227,7 +227,8 @@ func setVNCPassword(ctx context.Context, monSock, pw string) error { _ = conn.SetReadDeadline(time.Now().Add(5 * time.Second)) out, _ := readUntil(conn, "(qemu)") if strings.Contains(out, "Could not") { - return fmt.Errorf("qemu: %s", strings.TrimSpace(out)) + // out echoes the typed "set_password vnc " line — never surface it. + return errors.New("qemu rejected set_password (vnc display not active?)") } return nil } diff --git a/cmd/vm/lifecycle.go b/cmd/vm/lifecycle.go index 29cd9a3..ed3ee8d 100644 --- a/cmd/vm/lifecycle.go +++ b/cmd/vm/lifecycle.go @@ -142,8 +142,8 @@ func (h *Handler) create(cmd *cobra.Command, image string) (*record, error) { vnc, _ := cmd.Flags().GetInt("vnc") vncPass, _ := cmd.Flags().GetString("vnc-password") netMode, _ := cmd.Flags().GetString("net") - if netMode == netCNI && vnc >= 0 && vncPass == "" { // fail before scaffolding leaves a half-made VM - return nil, errCNIVNCPassRequired + if err = requireCNIVNCPassword(netMode == netCNI, vnc, vncPass); err != nil { // fail before scaffolding leaves a half-made VM + return nil, err } oc, code, varsTmpl, err := resolveFirmware(cmd) if err != nil { @@ -182,8 +182,8 @@ func (h *Handler) create(cmd *cobra.Command, image string) (*record, error) { func (h *Handler) launch(cmd *cobra.Command, dir string, r *record) error { ctx := cliutil.CommandContext(cmd) logger := log.WithFunc("cmd.vm.launch") - if r.Netns != "" && r.VNCDisp >= 0 && r.VNCPass == "" { - return errCNIVNCPassRequired + if err := requireCNIVNCPassword(r.Netns != "", r.VNCDisp, r.VNCPass); err != nil { + return err } if hostIsAMD() { // macOS reads MSRs an AMD host lacks; without kvm.ignore_msrs KVM injects #GP. Best-effort, diff --git a/cmd/vm/vnc.go b/cmd/vm/vnc.go index 3dfc6fa..03d5e23 100644 --- a/cmd/vm/vnc.go +++ b/cmd/vm/vnc.go @@ -30,6 +30,16 @@ const ( // never expose that unauthenticated. var errCNIVNCPassRequired = errors.New("--vnc with --net cni serves VNC on a host port reachable off-box; --vnc-password is required") +// requireCNIVNCPassword rejects an unauthenticated VNC display on a CNI VM whose +// host port is reachable off-box. isCNI is the flag intent at create/clone +// (pre-scaffold) or the resolved Netns at launch (post-scaffold). +func requireCNIVNCPassword(isCNI bool, vncDisp int, vncPass string) error { + if isCNI && vncDisp >= 0 && vncPass == "" { + return errCNIVNCPassRequired + } + return nil +} + // vncProxyCommand is the hidden re-exec target that runs the forwarder for its lifetime, accepting // on the TCP listener inherited as fd 3 (bound by the parent so bind errors fail the launch). func vncProxyCommand() *cobra.Command { @@ -72,7 +82,11 @@ func startVNCProxy(ctx context.Context, dir string, disp int) error { if err := c.Start(); err != nil { return err } - return utils.WritePIDFile(filepath.Join(dir, vncProxyPID), c.Process.Pid) + if err := utils.WritePIDFile(filepath.Join(dir, vncProxyPID), c.Process.Pid); err != nil { + _ = c.Process.Kill() // no pidfile -> stopVNCProxy can't reap it; don't orphan the proxy + return err + } + return nil } // stopVNCProxy kills a running proxy (best-effort) and removes its pidfile. Zero grace: the CLI's diff --git a/cmd/vm/vnc_test.go b/cmd/vm/vnc_test.go new file mode 100644 index 0000000..2daf38e --- /dev/null +++ b/cmd/vm/vnc_test.go @@ -0,0 +1,32 @@ +package vm + +import ( + "errors" + "testing" +) + +func TestRequireCNIVNCPassword(t *testing.T) { + tests := []struct { + name string + isCNI bool + vncDisp int + vncPass string + wantErr bool + }{ + {"cni vnc no password rejected", true, 0, "", true}, + {"cni vnc with password ok", true, 0, "s3cret", false}, + {"cni vnc disabled ok", true, -1, "", false}, + {"non-cni vnc no password ok", false, 0, "", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := requireCNIVNCPassword(tt.isCNI, tt.vncDisp, tt.vncPass) + if tt.wantErr && !errors.Is(err, errCNIVNCPassRequired) { + t.Errorf("got %v, want errCNIVNCPassRequired", err) + } + if !tt.wantErr && err != nil { + t.Errorf("got %v, want nil", err) + } + }) + } +}