From 235758e5b29cb308195c05d7995eab262cc493e3 Mon Sep 17 00:00:00 2001 From: Thales Maciel Date: Thu, 23 Apr 2026 13:04:33 -0300 Subject: [PATCH] =?UTF-8?q?workspace:=20drop=20--readonly=20flag=20?= =?UTF-8?q?=E2=80=94=20advisory=20only=20against=20root=20guests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --readonly ran `chmod -R a-w` over the workspace after copying, but every banger guest boots as root, and root bypasses DAC mode checks. So a user running `vm workspace prepare ... --readonly` got the mode bits set to 0444 but `echo x >> file` in the guest still succeeded. The flag promised enforcement it couldn't deliver. The feature also doesn't match the product model: workspaces are prepared precisely so the guest CAN edit them, and `workspace export` exists to pull those edits back as a patch. A "read-only workspace" contradicts that loop. Removed: - CLI flag `--readonly` on `vm workspace prepare` - api.VMWorkspacePrepareParams.ReadOnly field - model.WorkspacePrepareResult.ReadOnly field - daemon chmod dispatch in prepareVMWorkspaceGuestIO - smoke scenario pinning the (advisory) mode-bit behavior - misleading "exportbox-readonly" VM name in an unrelated export test (the test is about not mutating the real git index; renamed to exportbox-noindex-mutation) If real enforcement becomes a user need later, the right primitive is `chattr +i` (immutable bit — root CAN'T write) or a ro bind-mount. Reintroducing a new flag is cheaper than debugging what the current one actually guarantees. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/api/types.go | 1 - internal/cli/commands_vm.go | 5 +---- internal/daemon/workspace.go | 17 ++++------------ internal/daemon/workspace_test.go | 2 +- internal/model/types.go | 1 - scripts/smoke.sh | 34 ------------------------------- 6 files changed, 6 insertions(+), 54 deletions(-) diff --git a/internal/api/types.go b/internal/api/types.go index 82eb27a..d471995 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -143,7 +143,6 @@ type VMWorkspacePrepareParams struct { Branch string `json:"branch,omitempty"` From string `json:"from,omitempty"` Mode string `json:"mode,omitempty"` - ReadOnly bool `json:"readonly,omitempty"` IncludeUntracked bool `json:"include_untracked,omitempty"` } diff --git a/internal/cli/commands_vm.go b/internal/cli/commands_vm.go index 21618f7..62b195b 100644 --- a/internal/cli/commands_vm.go +++ b/internal/cli/commands_vm.go @@ -583,7 +583,6 @@ func (d *deps) newVMWorkspacePrepareCommand() *cobra.Command { var branchName string var fromRef string var mode string - var readOnly bool var includeUntracked bool var dryRun bool cmd := &cobra.Command{ @@ -594,7 +593,7 @@ func (d *deps) newVMWorkspacePrepareCommand() *cobra.Command { ValidArgsFunction: d.completeVMNameOnlyAtPos0, Example: strings.TrimSpace(` banger vm workspace prepare devbox - banger vm workspace prepare devbox ../repo --guest-path /root/repo --readonly + banger vm workspace prepare devbox ../repo --guest-path /root/repo banger vm workspace prepare devbox ../repo --mode full_copy `), RunE: func(cmd *cobra.Command, args []string) error { @@ -634,7 +633,6 @@ func (d *deps) newVMWorkspacePrepareCommand() *cobra.Command { Branch: branchName, From: prepareFrom, Mode: mode, - ReadOnly: readOnly, IncludeUntracked: includeUntracked, }) if err != nil { @@ -647,7 +645,6 @@ func (d *deps) newVMWorkspacePrepareCommand() *cobra.Command { cmd.Flags().StringVar(&branchName, "branch", "", "create and switch to a new guest branch") cmd.Flags().StringVar(&fromRef, "from", "HEAD", "base ref for --branch") cmd.Flags().StringVar(&mode, "mode", string(model.WorkspacePrepareModeShallowOverlay), "workspace mode: shallow_overlay, full_copy, metadata_only") - cmd.Flags().BoolVar(&readOnly, "readonly", false, "make the prepared workspace read-only") cmd.Flags().BoolVar(&includeUntracked, "include-untracked", false, "also copy untracked non-ignored files into the guest workspace (default: tracked files only)") cmd.Flags().BoolVar(&dryRun, "dry-run", false, "list the files that would be copied and exit without touching the guest") return cmd diff --git a/internal/daemon/workspace.go b/internal/daemon/workspace.go index 9e0e97d..2dcf441 100644 --- a/internal/daemon/workspace.go +++ b/internal/daemon/workspace.go @@ -1,7 +1,6 @@ package daemon import ( - "bytes" "context" "errors" "fmt" @@ -182,13 +181,13 @@ func (s *WorkspaceService) PrepareVMWorkspace(ctx context.Context, params api.VM unlock := s.workspaceLocks.lock(vm.ID) defer unlock() - return s.prepareVMWorkspaceGuestIO(ctx, vm, strings.TrimSpace(params.SourcePath), guestPath, branchName, fromRef, mode, params.ReadOnly, params.IncludeUntracked) + return s.prepareVMWorkspaceGuestIO(ctx, vm, strings.TrimSpace(params.SourcePath), guestPath, branchName, fromRef, mode, params.IncludeUntracked) } // prepareVMWorkspaceGuestIO performs the actual guest-side work: -// inspect the local repo, dial SSH, stream the tar, optionally chmod -// readonly. It is called without holding the VM mutex. -func (s *WorkspaceService) prepareVMWorkspaceGuestIO(ctx context.Context, vm model.VMRecord, sourcePath, guestPath, branchName, fromRef string, mode model.WorkspacePrepareMode, readOnly, includeUntracked bool) (model.WorkspacePrepareResult, error) { +// inspect the local repo, dial SSH, stream the tar. Called without +// holding the VM mutex. +func (s *WorkspaceService) prepareVMWorkspaceGuestIO(ctx context.Context, vm model.VMRecord, sourcePath, guestPath, branchName, fromRef string, mode model.WorkspacePrepareMode, includeUntracked bool) (model.WorkspacePrepareResult, error) { spec, err := s.workspaceInspectRepoHook(ctx, sourcePath, branchName, fromRef, includeUntracked) if err != nil { return model.WorkspacePrepareResult{}, err @@ -208,13 +207,6 @@ func (s *WorkspaceService) prepareVMWorkspaceGuestIO(ctx context.Context, vm mod if err := s.workspaceImportHook(ctx, client, spec, guestPath, mode); err != nil { return model.WorkspacePrepareResult{}, err } - if readOnly { - var chmodLog bytes.Buffer - chmodScript := fmt.Sprintf("set -euo pipefail\nchmod -R a-w %s\n", ws.ShellQuote(guestPath)) - if err := client.RunScript(ctx, chmodScript, &chmodLog); err != nil { - return model.WorkspacePrepareResult{}, ws.FormatStepError("set workspace readonly", err, chmodLog.String()) - } - } return model.WorkspacePrepareResult{ VMID: vm.ID, SourcePath: spec.SourcePath, @@ -222,7 +214,6 @@ func (s *WorkspaceService) prepareVMWorkspaceGuestIO(ctx context.Context, vm mod RepoName: spec.RepoName, GuestPath: guestPath, Mode: mode, - ReadOnly: readOnly, HeadCommit: spec.HeadCommit, CurrentBranch: spec.CurrentBranch, BranchName: spec.BranchName, diff --git a/internal/daemon/workspace_test.go b/internal/daemon/workspace_test.go index 8fde215..b2d08f3 100644 --- a/internal/daemon/workspace_test.go +++ b/internal/daemon/workspace_test.go @@ -555,7 +555,7 @@ func TestExportVMWorkspace_DoesNotMutateRealIndex(t *testing.T) { apiSock := filepath.Join(t.TempDir(), "fc.sock") firecracker := startFakeFirecracker(t, apiSock) - vm := testVM("exportbox-readonly", "image-export", "172.16.0.107") + vm := testVM("exportbox-noindex-mutation", "image-export", "172.16.0.107") vm.State = model.VMStateRunning vm.Runtime.State = model.VMStateRunning vm.Runtime.APISockPath = apiSock diff --git a/internal/model/types.go b/internal/model/types.go index bbda953..6c04034 100644 --- a/internal/model/types.go +++ b/internal/model/types.go @@ -172,7 +172,6 @@ type WorkspacePrepareResult struct { RepoName string `json:"repo_name"` GuestPath string `json:"guest_path"` Mode WorkspacePrepareMode `json:"mode"` - ReadOnly bool `json:"readonly"` HeadCommit string `json:"head_commit,omitempty"` CurrentBranch string `json:"current_branch,omitempty"` BranchName string `json:"branch_name,omitempty"` diff --git a/scripts/smoke.sh b/scripts/smoke.sh index 7bc824b..3803f4b 100755 --- a/scripts/smoke.sh +++ b/scripts/smoke.sh @@ -392,40 +392,6 @@ grep -q 'sshd' <<<"$ports_out" \ # shellcheck disable=SC2064 trap "rm -rf '$runtime_dir'" EXIT -# --- workspace prepare --readonly ------------------------------------- -# --readonly runs `chmod -R a-w` over the workspace. Root in the -# guest bypasses DAC anyway, so this is advisory rather than -# enforced — the point of the flag is tooling contract: "the -# mode bits SAY readonly". Assert that contract: the write bit -# must be cleared on the guest file after --readonly prepare, and -# set without it. A regression where the chmod silently no-op'd -# would leave the bits unchanged. -log 'workspace prepare --readonly: mode bits reflect the request' -# shellcheck disable=SC2064 -trap "\"$BANGER\" vm delete smoke-ro >/dev/null 2>&1 || true; rm -rf '$runtime_dir'" EXIT - -"$BANGER" vm create --name smoke-ro >/dev/null || die 'workspace ro: create failed' -"$BANGER" vm workspace prepare smoke-ro "$repodir" --readonly >/dev/null \ - || die 'workspace ro: prepare --readonly failed' - -# stat octal mode. a-w clears the 0222 write bits across u/g/o, so -# none of the write bits should be set on the file. -ro_mode="$("$BANGER" vm ssh smoke-ro -- stat -c '%a' /root/repo/smoke-file.txt | tr -d '[:space:]')" -[[ -n "$ro_mode" ]] || die 'workspace ro: could not read mode bits' -case "$ro_mode" in - *[2367]) - die "workspace ro: file still has write bit set after --readonly (mode=$ro_mode)" - ;; -esac - -# Read must still succeed (--readonly means a-w, not a-r). -"$BANGER" vm ssh smoke-ro -- cat /root/repo/smoke-file.txt >/dev/null \ - || die 'workspace ro: read denied — --readonly dropped read perm too' - -"$BANGER" vm delete smoke-ro >/dev/null || die 'workspace ro: delete failed' -# shellcheck disable=SC2064 -trap "rm -rf '$runtime_dir'" EXIT - # --- workspace prepare --mode full_copy ------------------------------- # Default mode is shallow_overlay. full_copy copies the repo via a # different transfer path (tar stream into the guest's rootfs with