diff --git a/README.md b/README.md index d316f43..93a98ac 100644 --- a/README.md +++ b/README.md @@ -71,6 +71,7 @@ code-vm # interactive shell | `code-vm recreate` | Delete and rebuild the guest from scratch | | `code-vm proxy-log [all\|denied\|allowed\|follow]` | Read the Squid access log | | `code-vm allow [domain...]` | Add domains to the allowlist and apply them live | +| `code-vm secrets` | List secrets/vars the active profiles declare, mapped or not | | `code-vm doctor` | Check host prerequisites | ## Configuration @@ -106,29 +107,38 @@ the accelerated one for the host: | Linux | `qemu` | KVM | the `virtiofsd` package | | macOS 13.5+ | `vz` | Hypervisor.framework (HVF) | Virtualization.framework | -### Credentials +### No workspace credentials -There is **no credential injection mechanism**. If a build needs a private -registry, write the credential file into the guest home once — it persists across -restarts, because the guest disk is the sandbox's durable state: +Nothing agent- or workspace-authored is ever a credential source. The +previous `.sandbox-secrets.yaml` mechanism was removed rather than fixed: it +resolved each secret by running its `source:` command **on the host** — from +a file inside the workspace, which the agent can write. That is host command +execution reachable from inside the sandbox, which defeats the boundary the +whole design exists to draw. Its stated protection did not hold either: +rendered files were group-readable by the agent, and the generated deny +rules only matched commands where the path appeared as a separate argument, +which `python -c` (an allowed command) sidesteps. That class of mechanism +stays removed, and the integration suite pins it. -```bash -code-vm # shell into the guest -$ install -d -m 0700 ~/.gradle -$ cat > ~/.gradle/gradle.properties # paste, or pipe it in -``` +Credentials enter the sandbox in one of two ways: -Assume the agent can read anything you put there, and use credentials created -for the sandbox rather than your personal ones, so revoking them is cheap. +- Written directly into the guest home, once — it persists across restarts, + because the guest disk is the sandbox's durable state: -The previous `.sandbox-secrets.yaml` mechanism was removed rather than fixed. It -resolved each secret by running its `source:` command **on the host** — from a -file inside the workspace, which the agent can write. That is host command -execution reachable from inside the sandbox, which defeats the boundary the whole -design exists to draw. Its stated protection did not hold either: rendered files -were group-readable by the agent, and the generated deny rules only matched -commands where the path appeared as a separate argument, which `python -c` (an -allowed command) sidesteps. + ```bash + code-vm # shell into the guest + $ install -d -m 0700 ~/.gradle + $ cat > ~/.gradle/gradle.properties # paste, or pipe it in + ``` + + Assume the agent can read anything you put there, and use credentials + created for the sandbox rather than your personal ones, so revoking them + is cheap. + +- Through a profile's `templates/` tree (see [Credentials](#credentials) under + Profiles) — the host-trusted mechanism for sharing a credentialed config's + shape while keeping the credential itself in the user's own + `secrets.yaml`. ### Extending the allowlist @@ -249,6 +259,102 @@ Notes: revert the shell, or delete files already in the home. `code-vm recreate` is the clean-slate path. +### Credentials + +Profiles can also ship **templates**: files rendered from placeholders +before delivery, so a team can share the whole shape of a credentialed +config (proxies, mirrors, server IDs) while each user supplies only their +own credential sources. + +``` +wetf-maven/ + profile.yaml + templates/ # home-mirroring tree, rendered before delivery + .m2/settings.xml +``` + +`profile.yaml` gains two more sections: + +```yaml +secrets: + wetf-repo-user: + description: Artifactory user for wetf-snapshots/releases + suggest: gopass show -o wetf/artifactory-user # inert hint, never executed + wetf-repo-password: + suggest: gopass show -o wetf/artifactory-password +vars: + artifactory-url: + description: Base URL of the Artifactory instance +``` + +A template uses `${secret:name}` and `${var:name}` placeholders; anything +else — Maven properties, `${env.FOO}` — passes through untouched. A shipped +`.m2/settings.xml` typically writes the static parts (proxies, mirrors) +verbatim and reserves placeholders only for the credentialed bits: + +```xml + + + + wetf-snapshots + ${secret:wetf-repo-user} + ${secret:wetf-repo-password} + + + ... + ... + +``` + +`description` and `suggest` are inert display strings on the *host*: nothing +a profile ships is ever run there, and these two fields specifically are +never even parsed as commands. The user's own `secrets.yaml` `command` is +the only host execution in this mechanism. (Profile hooks are the deliberate +exception to "profiles don't execute" — they do run profile-shipped code, +but in the guest, as the agent, as described above.) Values come only from +the user's own mapping: + +- **`~/.config/code-vm/secrets.yaml`** — 0600, host-trusted like + `config.yaml`, never distributed with the profile: + + ```yaml + secrets: + wetf-repo-user: + command: gopass show -o wetf/artifactory-user + wetf-repo-password: + command: gopass show -o wetf/artifactory-password + ``` + + `command` runs on the host through the shell; its stdout, with one + trailing newline stripped, is the value. `value:` is also accepted for a + literal — a footgun for a real credential, fine for a low-value token. + (Neither stdin nor a tty is wired up, so a command that needs interactive + pinentry rather than an already-cached agent/keyring will hang or fail.) + +- **`vars:` in `config.yaml`** — the same non-secret literal map as any + other config key, for things like `artifactory-url: https://...`. + +Notes: + +- Mapping a secret makes its value readable by the agent, and therefore by + every active profile's hook, not just the one that declared it. Install + profiles from sources you trust with the secrets you map, and map only + sandbox-appropriate credentials. +- A declared-but-unmapped secret or var fails `code-vm start` / + `profile apply` with the exact snippet to paste into `secrets.yaml` or + `config.yaml`; nothing partial reaches the guest. `code-vm secrets` lists + every secret and var the active profiles declare, mapped or not, without + ever printing a value. +- Resolution happens at `code-vm start`, `code-vm profile apply`, and any + invocation that has to boot the VM — never per invocation against an + already-running VM, so there is no per-command secret-manager prompt. +- On a cold boot, hooks run as part of the guest's own boot sequence, + before `code-vm start` pushes the first rendered template: a hook that + reads a profile-shipped template must tolerate it not existing yet. +- Rotation: rotate the credential at its source (gopass, pass, op, …), then + either restart the VM or run `code-vm profile apply` — code-vm + re-resolves and re-pushes every time, never caching an old value. + ## Security model The perimeter is the VM boundary. Inside it, the agent is separated from guest diff --git a/docs/superpowers/plans/2026-08-20-profile-secrets.md b/docs/superpowers/plans/2026-08-20-profile-secrets.md new file mode 100644 index 0000000..6c3a1b6 --- /dev/null +++ b/docs/superpowers/plans/2026-08-20-profile-secrets.md @@ -0,0 +1,1318 @@ +# Profile Secrets and Templates Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Profiles ship credentialed config templates (`${secret:name}`/`${var:name}` placeholders); secrets resolve on the host from the user's own `secrets.yaml` (command or literal), vars from `config.yaml`; rendered files push agent-owned 0600 into the guest home at start/apply/boot only. + +**Architecture:** Extends `internal/profile` (declarations, `templates/` tree, placeholder scan, rendering, resolution), `internal/config` (`secrets.yaml` loader, `Config.Vars`), and `internal/session` (a new agent-privilege user-file push that also replaces git-identity's root install). A new guest helper script relays staged content to an agent-identity install. CLI wires resolution into `start`, `profile apply`, and boot-causing invocations, plus a `code-vm secrets` listing command. + +**Tech Stack:** Go 1.26.5, Cobra, `gopkg.in/yaml.v3`, bash guest script (shellcheck/shfmt-clean), table-driven tests + fake runners, `test-vm-sandbox.sh`. + +**Spec:** `docs/superpowers/specs/2026-08-20-profile-secrets-design.md` + +## Global Constraints + +- Rendered/secret content must NEVER travel via `mode: data` (persists in `~/.lima//lima.yaml`), never enter `/usr/local/share/sandbox-profiles` (world-readable), and never be installed into the agent home by root (the TOCTOU class closed in the profiles PR). Delivery: staged push → root relays to a `root:AGENT_GID 0640` tmpfs drop → agent-identity install, final mode 0600 agent-owned, no exec bit. +- `suggest:`/`description:` are inert display strings: never executed, never substituted, never delivered to the guest. +- Host command execution comes ONLY from the user's `secrets.yaml` (`command:`) — never from profiles, the workspace, or the guest. +- Placeholders: exactly `${secret:}` and `${var:}` with names matching `[a-zA-Z0-9][a-zA-Z0-9-]{0,62}`; every other `${...}` passes through byte-for-byte. Undeclared placeholder = load-time error; declared-but-unmapped = start/apply-time error with a ready-to-paste snippet. +- Resolution/render/push happens ONLY at: `code-vm start`, `code-vm profile apply`, and an invocation whose `ensureRunning` actually booted the VM. +- Verification before every commit: `mise run test:unit && mise run lint && mise run fmt-check` (add `mise run build` before the last commit of a task touching cli). +- Commit style: conventional commits ending `Co-Authored-By: Claude Fable 5 `. +- Comments state constraints and reasons, matching the existing density. +- Golden file: profile declarations/templates must NOT change the Lima template rendering (templates are not DataFiles). Any golden drift is a bug. + +## File Structure + +| File | Responsibility | +|---|---| +| `internal/profile/profile.go` | Manifest gains `Secrets`/`Vars`; `templates/` tree loading; collision + undeclared-placeholder validation | +| `internal/profile/template.go` (new) | Placeholder scan (`FindRefs`), `RenderTemplates`, `DeclaredSecrets`/`DeclaredVars`, `ResolveSecrets`/`ResolveVars` | +| `internal/profile/profile_test.go`, `template_test.go` (new) | Tests | +| `internal/config/secrets.go` (new) | `SecretSource`, `LoadSecrets`, `SecretsPathFor` | +| `internal/config/config.go` | `Config.Vars` + key validation | +| `internal/guest/files/scripts/install-user-file.sh` (new) | Root relay → agent-identity install of one staged file | +| `internal/session/stage.go` | Factor out `stageFile` | +| `internal/session/userfiles.go` (new) | `PushUserFile` | +| `internal/session/gitidentity.go` | Migrate to `PushUserFile` | +| `internal/cli/start.go` | `ensureRunning` returns `(started bool, err)`; `pushRenderedTemplates` helper | +| `internal/cli/{shell,recreate,mount,profile}.go` | Trigger wiring | +| `internal/cli/secrets.go` (new) | `code-vm secrets` listing | +| `internal/cli/profile.go` | `add` trust warning lists declared secrets; `list` marks secret-declaring profiles | +| `test-vm-sandbox.sh`, `README.md` | Integration + docs | + +--- + +### Task 1: Manifest declarations and the templates tree + +**Files:** +- Modify: `internal/profile/profile.go` +- Test: `internal/profile/profile_test.go` + +**Interfaces:** +- Consumes: existing `Manifest`, `File`, `Profile`, `loadFiles`, `ValidateName`, `isBlank`, `relPathRe`, `forbiddenFiles`. +- Produces: + - `type SecretSpec struct { Description string \`yaml:"description"\`; Suggest string \`yaml:"suggest"\` }` + - `type VarSpec struct { Description string \`yaml:"description"\` }` + - `Manifest.Secrets map[string]SecretSpec \`yaml:"secrets"\``, `Manifest.Vars map[string]VarSpec \`yaml:"vars"\`` + - `Profile.Templates []File` (loaded from `templates/`, sorted by Rel; `Executable` ignored downstream) + - `refRe` (exported via Task 2's `FindRefs`; defined here or in template.go — put it in template.go, Task 2; THIS task only loads/validates trees and declarations, the placeholder cross-check moves in with Task 2's scanner via a shared `validateTemplateRefs` call added in Task 2) + +- [ ] **Step 1: Write the failing tests** + +Append to `internal/profile/profile_test.go` (reuse `writeProfile`; it takes a `files` map keyed by profile-relative path, so `templates/.m2/settings.xml` entries work as-is): + +```go +func TestLoadTemplatesAndDeclarations(t *testing.T) { + dir := t.TempDir() + writeProfile(t, dir, "maven", ` +description: maven setup +secrets: + repo-user: + description: Artifactory user + suggest: gopass show -o wetf/artifactory-user + repo-password: {} +vars: + artifactory-url: + description: Base URL +`, map[string]string{ + "templates/.m2/settings.xml": "${secret:repo-user}/${var:artifactory-url}\n", + }) + p, err := Load(dir, "maven") + if err != nil { + t.Fatalf("Load: %v", err) + } + if p.Manifest.Secrets["repo-user"].Suggest != "gopass show -o wetf/artifactory-user" { + t.Errorf("Suggest not loaded: %+v", p.Manifest.Secrets) + } + if _, ok := p.Manifest.Secrets["repo-password"]; !ok { + t.Error("empty-spec secret not loaded") + } + if len(p.Templates) != 1 || p.Templates[0].Rel != ".m2/settings.xml" { + t.Fatalf("Templates = %+v", p.Templates) + } +} + +// A profile carrying only declarations and templates is a valid profile. +func TestLoadTemplatesOnlyProfileIsNotEmpty(t *testing.T) { + dir := t.TempDir() + writeProfile(t, dir, "p", "secrets:\n tok: {}\n", map[string]string{ + "templates/.npmrc": "//registry/:_authToken=${secret:tok}\n", + }) + if _, err := Load(dir, "p"); err != nil { + t.Fatalf("Load: %v", err) + } +} + +func TestLoadRejectsInvalidDeclarations(t *testing.T) { + tests := []struct { + name string + manifest string + files map[string]string + wantErr string + }{ + {"bad secret name", "secrets:\n 'has space': {}\n", nil, "secret name"}, + {"bad var name", "vars:\n 'has/slash': {}\n", nil, "var name"}, + {"template/file collision", "description: x\n", + map[string]string{"files/.npmrc": "a\n", "templates/.npmrc": "b\n"}, + "both files/ and templates/"}, + {"template ships locked settings", "description: x\n", + map[string]string{"templates/.claude/settings.json": "{}\n"}, + "locked Claude settings"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + writeProfile(t, dir, "p", tt.manifest, tt.files) + _, err := Load(dir, "p") + if err == nil || !strings.Contains(err.Error(), tt.wantErr) { + t.Errorf("Load error = %v, want it to contain %q", err, tt.wantErr) + } + }) + } +} + +func TestLoadRejectsSymlinkedTemplatesRoot(t *testing.T) { + dir := t.TempDir() + writeProfile(t, dir, "p", "description: x\n", map[string]string{"files/a": "x\n"}) + if err := os.Symlink(t.TempDir(), filepath.Join(dir, "p", "templates")); err != nil { + t.Skip("symlinks unavailable") + } + if _, err := Load(dir, "p"); err == nil || !strings.Contains(err.Error(), "symlinks are rejected") { + t.Errorf("Load error = %v, want symlink rejection", err) + } +} +``` + +- [ ] **Step 2: Run to verify failure** — `go test ./internal/profile/` fails (unknown manifest fields, no Templates). + +- [ ] **Step 3: Implement** + +In `internal/profile/profile.go`: + +1. Add the `SecretSpec`/`VarSpec` types and the two `Manifest` fields (after `Hook`). +2. Factor the body of `loadFiles` into `loadTree(dir, subdir string) ([]File, error)` — identical logic, with `subdir` replacing the literal `"files"` in the root join and every error message prefix (use `fmt.Sprintf("%s/%s", subdir, rel)` where messages currently say `files/...`; keep the blank-content rejection for both trees — a blank template is a bundle bug, and one rule is easier to hold than two). `loadFiles(dir)` becomes `loadTree(dir, "files")`; add `loadTemplates(dir)` = `loadTree(dir, "templates")`. +3. In `Load`, after `p.Files`: `p.Templates, err = loadTemplates(dir)` with the same error wrapping. +4. In `validateManifest` (change its signature to `validateManifest(m Manifest) error` stays; add): + +```go + for name := range m.Secrets { + if err := ValidateName(name); err != nil { + return fmt.Errorf("secret name %q: must look like %q", name, "repo-user") + } + } + for name := range m.Vars { + if err := ValidateName(name); err != nil { + return fmt.Errorf("var name %q: must look like %q", name, "artifactory-url") + } + } +``` + +(Iteration order does not matter — validation only.) +5. After templates load in `Load`, reject collisions within the bundle: + +```go + fileRels := map[string]bool{} + for _, f := range p.Files { + fileRels[f.Rel] = true + } + for _, tpl := range p.Templates { + if fileRels[tpl.Rel] { + return Profile{}, fmt.Errorf("profile %s: %s is shipped by both files/ and templates/; pick one", name, tpl.Rel) + } + } +``` + +6. Extend the empty-profile check: `... && len(p.Templates) == 0 && len(m.Secrets) == 0 && len(m.Vars) == 0` (and add "templates, secrets or vars" to its message). + +- [ ] **Step 4: Run to verify pass** — `go test ./internal/profile/ -v`. +- [ ] **Step 5: Verify no golden drift** — `go test ./internal/lima/` must pass UNCHANGED (templates are not DataFiles). +- [ ] **Step 6: Lint, format, commit** + +```bash +git add internal/profile/ && git commit -m "feat: profiles declare secrets/vars and ship templates" +``` + +--- + +### Task 2: Placeholder scan, rendering, declaration merge, resolution + +**Files:** +- Create: `internal/profile/template.go` +- Modify: `internal/profile/profile.go` (wire undeclared-placeholder validation into `Load`) +- Test: `internal/profile/template_test.go` + +**Interfaces:** +- Consumes: `Profile`, `File`, `SecretSpec`, `VarSpec` (Task 1); `config.SecretSource` (Task 3 — see note below). +- Produces: + - `type Ref struct { Kind string // "secret" | "var"; Name string }` + - `func FindRefs(content []byte) []Ref` — deduplicated, in first-appearance order + - `type Rendered struct { Rel string; Content []byte }` + - `func RenderTemplates(profiles []Profile, secrets, vars map[string]string) []Rendered` — later profiles win Rel collisions; output sorted by Rel + - `type DeclaredSecret struct { Name string; Profiles []string; Description, Suggest string }` (first non-empty Description/Suggest wins); `func DeclaredSecrets(profiles []Profile) []DeclaredSecret` — sorted by name; analogous `DeclaredVar`/`DeclaredVars` + - `type CommandRunner func(ctx context.Context, command string) ([]byte, error)` — the host-exec seam + - `func ResolveSecrets(ctx context.Context, declared []DeclaredSecret, sources map[string]config.SecretSource, run CommandRunner) (map[string]string, error)` + - `func ResolveVars(declared []DeclaredVar, values map[string]string) (map[string]string, error)` + - `func MissingSecretSnippet(d DeclaredSecret) string` — the ready-to-paste `secrets.yaml` block + +**Ordering note:** this task needs `config.SecretSource{Command, Value string}` from Task 3. Tasks 2 and 3 may land in either order; whichever goes first defines the struct (if Task 2 goes first, add the two-field struct to `internal/config/secrets.go` with a doc comment and let Task 3 build the loader around it). Execute Task 3 first to keep it simple. + +- [ ] **Step 1: Write the failing tests** + +`internal/profile/template_test.go`: + +```go +package profile + +import ( + "context" + "errors" + "fmt" + "reflect" + "strings" + "testing" + + "github.com/wetransform/code-vm/internal/config" +) + +func TestFindRefs(t *testing.T) { + content := []byte(`user=${secret:repo-user} url=${var:base-url} +again=${secret:repo-user} passthrough=${env.FOO} ${prop} $secret:no ${secret:BAD NAME}`) + got := FindRefs(content) + want := []Ref{{Kind: "secret", Name: "repo-user"}, {Kind: "var", Name: "base-url"}} + if !reflect.DeepEqual(got, want) { + t.Errorf("FindRefs = %v, want %v", got, want) + } +} + +func TestRenderTemplatesSubstitutesAndPassesThrough(t *testing.T) { + profiles := []Profile{{ + Name: "a", + Templates: []File{{Rel: ".m2/settings.xml", Content: []byte( + "${secret:repo-user}${var:base-url}${env.HOME}")}}, + }} + out := RenderTemplates(profiles, + map[string]string{"repo-user": "simon"}, + map[string]string{"base-url": "https://x.example"}) + if len(out) != 1 { + t.Fatalf("Rendered = %+v", out) + } + want := "simonhttps://x.example${env.HOME}" + if string(out[0].Content) != want { + t.Errorf("Content = %q, want %q", out[0].Content, want) + } +} + +func TestRenderTemplatesLaterProfileWins(t *testing.T) { + profiles := []Profile{ + {Name: "a", Templates: []File{{Rel: ".npmrc", Content: []byte("from-a")}}}, + {Name: "b", Templates: []File{{Rel: ".npmrc", Content: []byte("from-b")}}}, + } + out := RenderTemplates(profiles, nil, nil) + if len(out) != 1 || string(out[0].Content) != "from-b" { + t.Errorf("collision must resolve to the later profile, got %+v", out) + } +} + +func TestDeclaredSecretsMergesAcrossProfiles(t *testing.T) { + profiles := []Profile{ + {Name: "a", Manifest: Manifest{Secrets: map[string]SecretSpec{ + "tok": {Description: "token", Suggest: "gopass show -o t"}}}}, + {Name: "b", Manifest: Manifest{Secrets: map[string]SecretSpec{"tok": {}}}}, + } + got := DeclaredSecrets(profiles) + if len(got) != 1 || got[0].Name != "tok" || got[0].Suggest != "gopass show -o t" || + !reflect.DeepEqual(got[0].Profiles, []string{"a", "b"}) { + t.Errorf("DeclaredSecrets = %+v", got) + } +} + +func TestResolveSecrets(t *testing.T) { + declared := []DeclaredSecret{ + {Name: "from-cmd", Profiles: []string{"p"}}, + {Name: "from-val", Profiles: []string{"p"}}, + } + sources := map[string]config.SecretSource{ + "from-cmd": {Command: "get-it"}, + "from-val": {Value: "literal"}, + } + calls := 0 + run := func(_ context.Context, command string) ([]byte, error) { + calls++ + if command != "get-it" { + t.Errorf("command = %q", command) + } + return []byte("resolved\n"), nil + } + got, err := ResolveSecrets(context.Background(), declared, sources, run) + if err != nil { + t.Fatalf("ResolveSecrets: %v", err) + } + // Exactly one trailing newline stripped; command runs once per secret. + if got["from-cmd"] != "resolved" || got["from-val"] != "literal" || calls != 1 { + t.Errorf("got %v, calls=%d", got, calls) + } +} + +func TestResolveSecretsMissingMappingHasSnippet(t *testing.T) { + declared := []DeclaredSecret{{ + Name: "repo-user", Profiles: []string{"maven"}, + Description: "Artifactory user", Suggest: "gopass show -o wetf/user", + }} + _, err := ResolveSecrets(context.Background(), declared, nil, nil) + if err == nil { + t.Fatal("expected an error for an unmapped secret") + } + for _, want := range []string{"repo-user", "maven", "Artifactory user", + "secrets:", "command: gopass show -o wetf/user"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error missing %q:\n%s", want, err) + } + } +} + +func TestResolveSecretsCommandFailure(t *testing.T) { + declared := []DeclaredSecret{{Name: "tok", Profiles: []string{"p"}}} + sources := map[string]config.SecretSource{"tok": {Command: "boom"}} + run := func(context.Context, string) ([]byte, error) { + return []byte("stderr text"), errors.New("exit status 1") + } + _, err := ResolveSecrets(context.Background(), declared, sources, run) + if err == nil || !strings.Contains(err.Error(), "tok") || !strings.Contains(err.Error(), "exit status 1") { + t.Errorf("ResolveSecrets error = %v", err) + } +} + +func TestResolveVars(t *testing.T) { + declared := []DeclaredVar{{Name: "url", Profiles: []string{"p"}, Description: "Base URL"}} + got, err := ResolveVars(declared, map[string]string{"url": "https://x"}) + if err != nil || got["url"] != "https://x" { + t.Errorf("ResolveVars = %v, %v", got, err) + } + _, err = ResolveVars(declared, nil) + if err == nil || !strings.Contains(err.Error(), "vars:") || !strings.Contains(err.Error(), "url") { + t.Errorf("missing var must produce a config.yaml snippet, got %v", err) + } +} + +// Load must reject a template referencing an undeclared name (wired in this task). +func TestLoadRejectsUndeclaredPlaceholder(t *testing.T) { + dir := t.TempDir() + writeProfile(t, dir, "p", "secrets:\n known: {}\n", map[string]string{ + "templates/.npmrc": "a=${secret:known} b=${var:never-declared}\n", + }) + _, err := Load(dir, "p") + if err == nil || !strings.Contains(err.Error(), "never-declared") { + t.Errorf("Load = %v, want undeclared-placeholder rejection", err) + } +} + +``` + +(Import list for this test file: `context`, `errors`, `reflect`, `strings`, `testing`, and the config package — no `fmt`.) + +- [ ] **Step 2: Run to verify failure** — `go test ./internal/profile/`. + +- [ ] **Step 3: Implement `internal/profile/template.go`** + +```go +package profile + +import ( + "context" + "fmt" + "regexp" + "sort" + "strings" + + "github.com/wetransform/code-vm/internal/config" +) + +// refRe matches exactly the two placeholder forms templates may use. The name +// charset mirrors ValidateName, so anything else — Maven properties, +// ${env.FOO} — is left untouched by both the scanner and the renderer. +var refRe = regexp.MustCompile(`\$\{(secret|var):([a-zA-Z0-9][a-zA-Z0-9-]{0,62})\}`) + +// Ref is one placeholder occurrence kind+name. +type Ref struct { + Kind string // "secret" or "var" + Name string +} + +// FindRefs returns the distinct placeholder references in content, in first- +// appearance order. +func FindRefs(content []byte) []Ref { + seen := map[Ref]bool{} + var out []Ref + for _, m := range refRe.FindAllSubmatch(content, -1) { + r := Ref{Kind: string(m[1]), Name: string(m[2])} + if !seen[r] { + seen[r] = true + out = append(out, r) + } + } + return out +} + +// Rendered is one template after substitution, destined for the agent home. +type Rendered struct { + Rel string + Content []byte +} + +// RenderTemplates substitutes secret and var values into every active +// profile's templates. Later profiles win Rel collisions, matching the files/ +// rule. Values are opaque bytes: no escaping layer, exactly as the spec +// states — a value that breaks the target format is the user's own. +func RenderTemplates(profiles []Profile, secrets, vars map[string]string) []Rendered { + byRel := map[string][]byte{} + for _, p := range profiles { + for _, tpl := range p.Templates { + byRel[tpl.Rel] = refRe.ReplaceAllFunc(tpl.Content, func(m []byte) []byte { + sub := refRe.FindSubmatch(m) + if string(sub[1]) == "secret" { + return []byte(secrets[string(sub[2])]) + } + return []byte(vars[string(sub[2])]) + }) + } + } + out := make([]Rendered, 0, len(byRel)) + for rel, content := range byRel { + out = append(out, Rendered{Rel: rel, Content: content}) + } + sort.Slice(out, func(i, j int) bool { return out[i].Rel < out[j].Rel }) + return out +} + +// DeclaredSecret is one secret name unioned across the active profiles. +type DeclaredSecret struct { + Name string + Profiles []string // declaring profiles, in activation order + Description string // first non-empty wins + Suggest string // first non-empty wins; inert display string +} + +// DeclaredVar is the var analog. +type DeclaredVar struct { + Name string + Profiles []string + Description string +} + +// DeclaredSecrets unions secret declarations across profiles, sorted by name. +func DeclaredSecrets(profiles []Profile) []DeclaredSecret { + byName := map[string]*DeclaredSecret{} + for _, p := range profiles { + names := make([]string, 0, len(p.Manifest.Secrets)) + for n := range p.Manifest.Secrets { + names = append(names, n) + } + sort.Strings(names) // map order is random; keep Profiles deterministic + for _, n := range names { + spec := p.Manifest.Secrets[n] + d, ok := byName[n] + if !ok { + d = &DeclaredSecret{Name: n} + byName[n] = d + } + d.Profiles = append(d.Profiles, p.Name) + if d.Description == "" { + d.Description = spec.Description + } + if d.Suggest == "" { + d.Suggest = spec.Suggest + } + } + } + out := make([]DeclaredSecret, 0, len(byName)) + for _, d := range byName { + out = append(out, *d) + } + sort.Slice(out, func(i, j int) bool { return out[i].Name < out[j].Name }) + return out +} + +// DeclaredVars unions var declarations across profiles, sorted by name. +func DeclaredVars(profiles []Profile) []DeclaredVar { + byName := map[string]*DeclaredVar{} + for _, p := range profiles { + names := make([]string, 0, len(p.Manifest.Vars)) + for n := range p.Manifest.Vars { + names = append(names, n) + } + sort.Strings(names) + for _, n := range names { + spec := p.Manifest.Vars[n] + d, ok := byName[n] + if !ok { + d = &DeclaredVar{Name: n} + byName[n] = d + } + d.Profiles = append(d.Profiles, p.Name) + if d.Description == "" { + d.Description = spec.Description + } + } + } + out := make([]DeclaredVar, 0, len(byName)) + for _, d := range byName { + out = append(out, *d) + } + sort.Slice(out, func(i, j int) bool { return out[i].Name < out[j].Name }) + return out +} + +// CommandRunner executes a user-authored secrets.yaml command on the host and +// returns its combined output. Injectable for tests. +type CommandRunner func(ctx context.Context, command string) ([]byte, error) + +// MissingSecretSnippet renders the ready-to-paste secrets.yaml block for an +// unmapped secret. The suggest hint is copied verbatim as the command — +// display only until the user adopts it by saving this snippet themselves. +func MissingSecretSnippet(d DeclaredSecret) string { + cmd := d.Suggest + if cmd == "" { + cmd = "" + } + return fmt.Sprintf("secrets:\n %s:\n command: %s\n", d.Name, cmd) +} + +// ResolveSecrets resolves every declared secret from the user's sources. Each +// command runs exactly once per resolve pass with one trailing newline +// stripped (the gopass/pass convention). A missing mapping fails with an +// actionable snippet rather than prompting or falling back to hints: hints +// never execute. +func ResolveSecrets(ctx context.Context, declared []DeclaredSecret, sources map[string]config.SecretSource, run CommandRunner) (map[string]string, error) { + out := make(map[string]string, len(declared)) + for _, d := range declared { + src, ok := sources[d.Name] + if !ok { + desc := d.Description + if desc == "" { + desc = "no description" + } + return nil, fmt.Errorf( + "profile %s needs secret %q (%s), but secrets.yaml does not map it.\nAdd to ~/.config/code-vm/secrets.yaml:\n\n%s", + strings.Join(d.Profiles, ", "), d.Name, desc, MissingSecretSnippet(d)) + } + if src.Command != "" { + b, err := run(ctx, src.Command) + if err != nil { + return nil, fmt.Errorf("secret %q: command failed: %w: %s", d.Name, err, strings.TrimSpace(string(b))) + } + out[d.Name] = strings.TrimSuffix(string(b), "\n") + continue + } + out[d.Name] = src.Value + } + return out, nil +} + +// ResolveVars resolves declared vars from config.yaml's literal map. +func ResolveVars(declared []DeclaredVar, values map[string]string) (map[string]string, error) { + out := make(map[string]string, len(declared)) + for _, d := range declared { + v, ok := values[d.Name] + if !ok { + desc := d.Description + if desc == "" { + desc = "no description" + } + return nil, fmt.Errorf( + "profile %s needs var %q (%s), but config.yaml does not set it.\nAdd to config.yaml:\n\nvars:\n %s: \n", + strings.Join(d.Profiles, ", "), d.Name, desc, d.Name) + } + out[d.Name] = v + } + return out, nil +} +``` + +Wire undeclared-placeholder validation into `Load` (profile.go), after templates load and before the empty check: + +```go + for _, tpl := range p.Templates { + for _, ref := range FindRefs(tpl.Content) { + declared := false + if ref.Kind == "secret" { + _, declared = m.Secrets[ref.Name] + } else { + _, declared = m.Vars[ref.Name] + } + if !declared { + return Profile{}, fmt.Errorf( + "profile %s: templates/%s references ${%s:%s}, which the manifest does not declare", + name, tpl.Rel, ref.Kind, ref.Name) + } + } + } +``` + +- [ ] **Step 4: Run to verify pass** — `go test ./internal/profile/ -v` (Task 3's `config.SecretSource` must exist first — execute Task 3 before this one if it hasn't landed; see Ordering note). +- [ ] **Step 5: Lint, format, commit** + +```bash +git add internal/profile/ && git commit -m "feat: template rendering and secret/var resolution" +``` + +--- + +### Task 3: `secrets.yaml` loader and `Config.Vars` + +**Files:** +- Create: `internal/config/secrets.go` +- Modify: `internal/config/config.go` (Vars field + validation) +- Test: `internal/config/secrets_test.go`, `internal/config/config_test.go` + +**Interfaces:** +- Produces: + - `type SecretSource struct { Command string \`yaml:"command"\`; Value string \`yaml:"value"\` }` + - `func SecretsPathFor(configPath string) string` — `secrets.yaml` next to the config file + - `func LoadSecrets(path string) (map[string]SecretSource, []string, error)` — `(sources, warnings, err)`; missing file → empty map, no error; unknown keys rejected (KnownFields); per-entry exactly one of command/value; group/world-readable file → warning string + - `Config.Vars map[string]string \`yaml:"vars,omitempty"\`` — keys validated against the name pattern in `Config.Validate` + +- [ ] **Step 1: Write the failing tests** + +`internal/config/secrets_test.go`: + +```go +package config + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestSecretsPathFor(t *testing.T) { + if got := SecretsPathFor("/home/st/.config/code-vm/config.yaml"); got != "/home/st/.config/code-vm/secrets.yaml" { + t.Errorf("SecretsPathFor = %q", got) + } +} + +func TestLoadSecrets(t *testing.T) { + dir := t.TempDir() + p := filepath.Join(dir, "secrets.yaml") + content := "secrets:\n a:\n command: gopass show -o x\n b:\n value: literal\n" + if err := os.WriteFile(p, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + sources, warnings, err := LoadSecrets(p) + if err != nil { + t.Fatalf("LoadSecrets: %v", err) + } + if len(warnings) != 0 { + t.Errorf("warnings = %v, want none for 0600", warnings) + } + if sources["a"].Command != "gopass show -o x" || sources["b"].Value != "literal" { + t.Errorf("sources = %+v", sources) + } +} + +func TestLoadSecretsMissingFileIsEmpty(t *testing.T) { + sources, warnings, err := LoadSecrets(filepath.Join(t.TempDir(), "secrets.yaml")) + if err != nil || len(sources) != 0 || len(warnings) != 0 { + t.Errorf("missing file must load empty: %v %v %v", sources, warnings, err) + } +} + +func TestLoadSecretsRejectsBadEntries(t *testing.T) { + tests := []struct{ name, content, wantErr string }{ + {"both command and value", "secrets:\n a:\n command: c\n value: v\n", "exactly one"}, + {"neither", "secrets:\n a: {}\n", "exactly one"}, + {"bad name", "secrets:\n 'has space':\n value: v\n", "secret name"}, + {"unknown key", "secrets:\n a:\n comand: typo\n", "not found"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + p := filepath.Join(t.TempDir(), "secrets.yaml") + if err := os.WriteFile(p, []byte(tt.content), 0o600); err != nil { + t.Fatal(err) + } + _, _, err := LoadSecrets(p) + if err == nil || !strings.Contains(err.Error(), tt.wantErr) { + t.Errorf("LoadSecrets = %v, want %q", err, tt.wantErr) + } + }) + } +} + +func TestLoadSecretsWarnsOnLoosePermissions(t *testing.T) { + p := filepath.Join(t.TempDir(), "secrets.yaml") + if err := os.WriteFile(p, []byte("secrets:\n a:\n value: v\n"), 0o644); err != nil { + t.Fatal(err) + } + _, warnings, err := LoadSecrets(p) + if err != nil || len(warnings) != 1 || !strings.Contains(warnings[0], "0600") { + t.Errorf("want a permissions warning recommending 0600, got %v %v", warnings, err) + } +} +``` + +Append to `internal/config/config_test.go`: + +```go +func TestValidateVars(t *testing.T) { + c := Default() + c.ProjectsRoot = "/home/st/projects" + c.Vars = map[string]string{"artifactory-url": "https://x"} + if err := c.Validate(); err != nil { + t.Errorf("Validate: %v", err) + } + c.Vars = map[string]string{"has space": "v"} + if err := c.Validate(); err == nil || !strings.Contains(err.Error(), "vars") { + t.Errorf("Validate = %v, want vars key rejection", err) + } +} +``` + +- [ ] **Step 2: Run to verify failure** — `go test ./internal/config/`. + +- [ ] **Step 3: Implement** + +`internal/config/secrets.go`: + +```go +package config + +import ( + "errors" + "fmt" + "os" + "path/filepath" + + "gopkg.in/yaml.v3" +) + +// SecretSource is one user-authored mapping in secrets.yaml: exactly one of a +// host command (stdout is the value) or a literal. This file is host-trusted +// like config.yaml — it lives in the same mount-guarded tree and only the +// user writes it; profiles can only *suggest* entries, never install them. +type SecretSource struct { + Command string `yaml:"command"` + Value string `yaml:"value"` +} + +// secretsFile is the secrets.yaml schema. +type secretsFile struct { + Secrets map[string]SecretSource `yaml:"secrets"` +} + +// SecretsPathFor returns the secrets file belonging to a config file: a +// secrets.yaml next to it, protected by the same mount-exclusion guards. +func SecretsPathFor(configPath string) string { + return filepath.Join(filepath.Dir(configPath), "secrets.yaml") +} + +// LoadSecrets reads and validates secrets.yaml. A missing file is an empty +// mapping — profiles without secrets must not require one. Warnings (not +// errors) report loose file permissions: the file holds commands and possibly +// literal credentials. +func LoadSecrets(path string) (map[string]SecretSource, []string, error) { + data, err := os.ReadFile(path) + if errors.Is(err, os.ErrNotExist) { + return map[string]SecretSource{}, nil, nil + } + if err != nil { + return nil, nil, fmt.Errorf("read %s: %w", path, err) + } + var warnings []string + if fi, err := os.Stat(path); err == nil && fi.Mode().Perm()&0o077 != 0 { + warnings = append(warnings, fmt.Sprintf( + "%s is readable by group/others; recommend chmod 0600", path)) + } + var f secretsFile + dec := yaml.NewDecoder(bytes.NewReader(data)) + dec.KnownFields(true) + // An empty file decodes to io.EOF, and an empty secrets.yaml is as valid + // as a missing one. + if err := dec.Decode(&f); err != nil && !errors.Is(err, io.EOF) { + return nil, nil, fmt.Errorf("parse %s: %w", path, err) + } + for name, src := range f.Secrets { + if !instanceRe.MatchString(name) { + return nil, nil, fmt.Errorf("%s: secret name %q: must look like %q", path, name, "repo-user") + } + if (src.Command == "") == (src.Value == "") { + return nil, nil, fmt.Errorf("%s: secret %q: exactly one of command or value must be set", path, name) + } + } + if f.Secrets == nil { + f.Secrets = map[string]SecretSource{} + } + return f.Secrets, warnings, nil +} +``` + +(`bytesReader` = `bytes.NewReader`; import `bytes` directly — the helper name above is illustrative, use `bytes.NewReader(data)`. An empty file decodes to `io.EOF`: handle it like profile.go does, treating EOF as an empty document.) + +`internal/config/config.go`: add after `Profiles`: + +```go + // Vars are non-secret literal values available to profile templates as + // ${var:name}. Secrets never belong here — they go in secrets.yaml. + Vars map[string]string `yaml:"vars,omitempty"` +``` + +and in `Validate`, after the profiles loop: + +```go + for name := range c.Vars { + if !instanceRe.MatchString(name) { + return fmt.Errorf("vars: key %q must be a name like %q", name, "artifactory-url") + } + } +``` + +- [ ] **Step 4: Run to verify pass**, **Step 5: Lint, format, commit** + +```bash +git add internal/config/ && git commit -m "feat: user-side secrets.yaml and config vars" +``` + +--- + +### Task 4: Guest relay script and `session.PushUserFile`; migrate git identity + +**Files:** +- Create: `internal/guest/files/scripts/install-user-file.sh` +- Create: `internal/session/userfiles.go` +- Modify: `internal/session/stage.go` (factor `stageFile`), `internal/session/gitidentity.go` +- Test: `internal/session/userfiles_test.go`, update `gitidentity_test.go`, `internal/guest/embed_test.go` + +**Interfaces:** +- Consumes: staging plumbing in `stage.go`; the hardened setpriv pattern (see `apply-profiles.sh`'s `run_as_agent_sh`). +- Produces: + - `func stageFile(ctx context.Context, d Deps, content []byte) (string, error)` — the temp-file + `install -d` staging dir + `Copy` half of today's `installContent`, returning the staged guest path + - `func PushUserFile(ctx context.Context, d Deps, content []byte, rel, mode string) error` — stages content, then `Admin(["/usr/local/lib/sandbox/install-user-file.sh", staged, rel, mode])` + - Guest contract: `install-user-file.sh ` — root relays the staged file to a `root:AGENT_GID 0640` drop under `/run/sandbox/user-files/`, then an agent-identity install places it (mkdir -p, rm -f, install -m), then the drop and staged copies are removed. + +**Why the relay:** the staging dir is limaadmin-0700 (agent cannot read it), and a direct root `install` into the agent home is the TOCTOU class the profiles PR closed. The drop directory gives the agent read access without world-readability, and the final write runs with agent privileges only. + +- [ ] **Step 1: Write the failing session tests** + +`internal/session/userfiles_test.go` (reuse `fakeRunner`/`testDeps`): + +```go +package session + +import ( + "context" + "testing" +) + +func TestPushUserFileStagesAndRelays(t *testing.T) { + r := &fakeRunner{} + d := testDeps(t, r) + if err := PushUserFile(context.Background(), d, []byte("content"), ".m2/settings.xml", "0600"); err != nil { + t.Fatalf("PushUserFile: %v", err) + } + copies := 0 + for _, c := range r.calls { + if len(c) > 0 && c[0] == "copy" { + copies++ + } + } + if copies != 1 { + t.Errorf("staged copies = %d, want 1", copies) + } + if !r.ranAny("/usr/local/lib/sandbox/install-user-file.sh") { + t.Errorf("relay script not invoked: %v", r.calls) + } + if !r.ranAny(".m2/settings.xml") || !r.ranAny("0600") { + t.Errorf("dst/mode not passed to the relay: %v", r.calls) + } + // The old direct-to-home root install must NOT happen for user files. + if r.ranAny("install -D -m 0600") { + t.Errorf("user files must not be root-installed into the home: %v", r.calls) + } +} + +func TestGitIdentityUsesUserFilePush(t *testing.T) { + r := &fakeRunner{} + d := testDeps(t, r) + d.Host = func(ctx context.Context, name string, args ...string) ([]byte, error) { + return []byte("simon\n"), nil + } + if err := ApplyGitIdentity(context.Background(), d); err != nil { + t.Fatalf("ApplyGitIdentity: %v", err) + } + if !r.ranAny("install-user-file.sh") || !r.ranAny(".gitconfig") { + t.Errorf("git identity must go through the relay: %v", r.calls) + } + if r.ranAny("install -D -m 0644") { + t.Errorf("git identity must no longer be root-installed: %v", r.calls) + } +} +``` + +- [ ] **Step 2: Run to verify failure** — `go test ./internal/session/`. + +- [ ] **Step 3: Implement** + +In `stage.go`, split `installContent`: extract everything up to and including the `Copy` into `stageFile(ctx, d, content) (staged string, err)`; `installContent` calls it then keeps its root `install -D` + `rm -f` (still used by the allowlist fragment and profile-tree pushes, whose destinations are root-owned paths — the comment should say that is exactly why root install remains correct there). + +`internal/session/userfiles.go`: + +```go +package session + +import ( + "context" + "fmt" +) + +// PushUserFile delivers content into the agent's home at rel with the given +// mode, without ever writing there as root. The staged copy is relayed by +// install-user-file.sh: root moves it to an agent-group-readable tmpfs drop, +// and an agent-identity install places it — a symlink the agent plants can +// only redirect a write the agent could already make (the same posture as +// profile file installs). Used for rendered templates (0600) and the git +// identity (0644); rel comes from host-validated input only. +func PushUserFile(ctx context.Context, d Deps, content []byte, rel, mode string) error { + staged, err := stageFile(ctx, d, content) + if err != nil { + return err + } + if err := d.Client.Admin(ctx, []string{ + "/usr/local/lib/sandbox/install-user-file.sh", staged, rel, mode, + }); err != nil { + return fmt.Errorf("install %s: %w", rel, err) + } + return nil +} +``` + +`internal/guest/files/scripts/install-user-file.sh` (auto-delivered 0755 by the existing scripts mapping): + +```bash +#!/bin/bash +############################################################################### +# install-user-file.sh — place one host-staged file into the agent home +# +# Invoked as root by code-vm: install-user-file.sh +# +# The staged source sits in limaadmin's 0700 staging dir, unreadable by the +# agent; and a root write into the agent-owned home is the TOCTOU class the +# profile applier already closed. So root only RELAYS: the file moves to a +# root:AGENT_GID 0640 drop on tmpfs, and the final install runs with agent +# privileges — a planted symlink can only redirect a write the agent could +# already make. The drop is removed afterwards; rendered secrets exist there +# only for the moment between relay and install. +############################################################################### +set -euo pipefail + +# shellcheck source=/dev/null +. /etc/sandbox/provision.env + +src="$1" +rel="$2" +mode="$3" +AGENT_HOME="/home/${AGENT_USER}" + +DROP_DIR=/run/sandbox/user-files +install -d -m 0750 -o root -g "$AGENT_GID" "$DROP_DIR" +drop=$(mktemp "$DROP_DIR/file-XXXXXXXX") +install -m 0640 -o root -g "$AGENT_GID" "$src" "$drop" +rm -f "$src" + +cleanup() { rm -f "$drop"; } +trap cleanup EXIT + +# Same hardened pattern as the profile applier's agent runner: no login +# shell, system PATH only, BASH_ENV/ENV cleared. Positional args, not string +# interpolation. +setpriv --reuid "$AGENT_UID" --regid "$AGENT_GID" --init-groups \ + env -u BASH_ENV -u ENV \ + HOME="$AGENT_HOME" \ + USER="$AGENT_USER" \ + XDG_RUNTIME_DIR="/run/user/${AGENT_UID}" \ + PATH=/usr/local/bin:/usr/bin:/bin \ + bash -c 'dst="$1/$2"; mkdir -p "$(dirname "$dst")" && rm -f "$dst" && install -m "$3" "$4" "$dst"' \ + _ "$AGENT_HOME" "$rel" "$mode" "$drop" +``` + +Migrate `gitidentity.go`: replace the `installContent(...)` call with `PushUserFile(ctx, d, []byte(GitConfigContent(name, email)), ".gitconfig", "0644")` and drop the now-unused numeric-id comment/imports (the relay script owns identity now). + +Add an embed test asserting `install-user-file.sh` is delivered 0755 (mirror `TestApplyProfilesScriptIsDelivered`). + +- [ ] **Step 4: Run to verify pass** — `go test ./internal/session/ ./internal/guest/` plus `bash -n` on the new script. +- [ ] **Step 5: Golden regen check** — the new script is a DataFile in `guest.DataFiles()` but the golden test passes explicit files, so the golden should be UNCHANGED; if `TestRenderInstanceFileIsPrivateAndComplete`-style assertions or the golden do move, only the new script's mode:data entry may appear. +- [ ] **Step 6: Lint (shellcheck/shfmt cover the script), format, commit** + +```bash +git add internal/session/ internal/guest/ && git commit -m "feat: agent-privilege user-file push; migrate git identity to it" +``` + +--- + +### Task 5: Trigger plumbing and orchestration + +**Files:** +- Modify: `internal/cli/start.go` (`ensureRunning` returns `(bool, error)`; new `pushRenderedTemplates`), `internal/cli/shell.go`, `internal/cli/recreate.go`, `internal/cli/mount.go`, `internal/cli/profile.go` (apply) +- Test: `internal/cli/start_test.go`, `internal/cli/secrets_push_test.go` (new) + +**Interfaces:** +- Consumes: Tasks 1–4 (`DeclaredSecrets/Vars`, `ResolveSecrets/Vars`, `RenderTemplates`, `config.LoadSecrets/SecretsPathFor`, `session.PushUserFile`). +- Produces: + - `ensureRunning(ctx, cl, c, profiles) (started bool, err error)` — `started` is true only when this call actually booted the VM (status was not "Running"). + - `func pushRenderedTemplates(ctx context.Context, cl lima.Client, c config.Config, profiles []profile.Profile, cfgPath string, out io.Writer) error` — fast no-op when no active profile has templates/secrets/vars; otherwise LoadSecrets (print warnings to out), resolve (host runner = `exec.CommandContext(ctx, "sh", "-c", command).CombinedOutput()`), render, `PushUserFile(..., rel, "0600")` per rendered file, and print one `Rendered N template(s).` line. + - Trigger contract: `start` always pushes after `ensureRunning`; `runDefault`, `recreate`, `mount` push only when `started` came back true; `profile apply` pushes after `ApplyAllowlist` and BEFORE `ApplyProfiles` (hooks may consume rendered configs). Note: at boot, hooks run before the post-readiness push — hooks must tolerate absent templates on first boot; the README documents this (Task 7). + +- [ ] **Step 1: Write the failing tests** + +Update `internal/cli/start_test.go`: `ensureRunning` calls gain the second return; add assertions that status "Running" → `started == false` and statuses ""/"Stopped" → `started == true`. + +`internal/cli/secrets_push_test.go` — drive `pushRenderedTemplates` directly with a `recordingRunner`-backed client and a scratch config tree: + +```go +func TestPushRenderedTemplatesNoOpWithoutDeclarations(t *testing.T) { + r := &recordingRunner{statusOut: "Running"} + c := testCfg(t) + profiles := []profile.Profile{{Name: "plain", Manifest: profile.Manifest{Packages: []string{"git"}}}} + if err := pushRenderedTemplates(context.Background(), lima.Client{R: r}, c, profiles, filepath.Join(t.TempDir(), "config.yaml"), io.Discard); err != nil { + t.Fatalf("pushRenderedTemplates: %v", err) + } + if len(r.calls) != 0 { + t.Errorf("no declarations must mean no guest traffic, got %v", r.calls) + } +} + +func TestPushRenderedTemplatesResolvesAndPushes(t *testing.T) { + dir := t.TempDir() + cfgPath := filepath.Join(dir, "config.yaml") + os.WriteFile(filepath.Join(dir, "secrets.yaml"), []byte("secrets:\n tok:\n value: sekrit\n"), 0o600) + r := &recordingRunner{statusOut: "Running"} + c := testCfg(t) + c.Vars = map[string]string{"url": "https://x"} + profiles := []profile.Profile{{ + Name: "p", + Manifest: profile.Manifest{ + Secrets: map[string]profile.SecretSpec{"tok": {}}, + Vars: map[string]profile.VarSpec{"url": {}}, + }, + Templates: []profile.File{{Rel: ".npmrc", Content: []byte("t=${secret:tok};u=${var:url}")}}, + }} + if err := pushRenderedTemplates(context.Background(), lima.Client{R: r}, c, profiles, cfgPath, io.Discard); err != nil { + t.Fatalf("pushRenderedTemplates: %v", err) + } + if !ranAny(r.calls, "install-user-file.sh") || !ranAny(r.calls, ".npmrc") || !ranAny(r.calls, "0600") { + t.Errorf("expected a relay push of .npmrc at 0600, got %v", r.calls) + } +} + +func TestPushRenderedTemplatesMissingMappingFails(t *testing.T) { + r := &recordingRunner{} + c := testCfg(t) + profiles := []profile.Profile{{ + Name: "p", + Manifest: profile.Manifest{Secrets: map[string]profile.SecretSpec{"tok": {Suggest: "gopass show -o t"}}}, + Templates: []profile.File{{Rel: ".npmrc", Content: []byte("${secret:tok}")}}, + }} + err := pushRenderedTemplates(context.Background(), lima.Client{R: r}, c, profiles, filepath.Join(t.TempDir(), "config.yaml"), io.Discard) + if err == nil || !strings.Contains(err.Error(), "gopass show -o t") { + t.Errorf("missing mapping must fail with the snippet, got %v", err) + } + if len(r.calls) != 0 { + t.Errorf("nothing may reach the guest on resolution failure, got %v", r.calls) + } +} +``` + +- [ ] **Step 2: Run to verify failure**, then **Step 3: Implement** + +`ensureRunning`: change the three `return ...` exits — "Running" → `(false, nil)`; the two start paths → `(true, cl.Start(...))` / `(true, cl.StartExisting(...))` (return `(false, err)` on pre-start errors). Update every caller: + +- `start.go` `newStartCmd`: `if _, err := ensureRunning(...); err != nil { return err }` then ALWAYS `pushRenderedTemplates(...)` (needs the config path — `loadConfigWithProfiles` already returns it). +- `shell.go` `runDefault`: capture `started`; after `session.Setup`, `if started { pushRenderedTemplates(...) }` — before `cl.Agent(...)` so the first command sees the configs. +- `recreate.go`: capture both; push when `started` (recreate always boots, so effectively always). +- `mount.go`: same pattern after its restart. +- `profile.go` apply: insert `pushRenderedTemplates` between `ApplyAllowlist` and `session.ApplyProfiles`. + +`pushRenderedTemplates` (in start.go, near `agentDeps`): + +```go +// pushRenderedTemplates resolves secrets/vars and pushes rendered templates +// into the agent home. Callers gate it to start, apply, and boot-causing +// invocations only: resolution may invoke the user's secret manager +// (pinentry), so it must never run on every command. +func pushRenderedTemplates(ctx context.Context, cl lima.Client, c config.Config, profiles []profile.Profile, cfgPath string, out io.Writer) error { + secretsDecl := profile.DeclaredSecrets(profiles) + varsDecl := profile.DeclaredVars(profiles) + templated := false + for _, p := range profiles { + if len(p.Templates) > 0 { + templated = true + } + } + if !templated && len(secretsDecl) == 0 && len(varsDecl) == 0 { + return nil + } + sources, warnings, err := config.LoadSecrets(config.SecretsPathFor(cfgPath)) + if err != nil { + return err + } + for _, w := range warnings { + fmt.Fprintf(out, "warning: %s\n", w) + } + secrets, err := profile.ResolveSecrets(ctx, secretsDecl, sources, hostCommand) + if err != nil { + return err + } + vars, err := profile.ResolveVars(varsDecl, c.Vars) + if err != nil { + return err + } + rendered := profile.RenderTemplates(profiles, secrets, vars) + d := agentDeps(cl, c, profiles) + for _, r := range rendered { + if err := session.PushUserFile(ctx, d, r.Content, r.Rel, "0600"); err != nil { + return err + } + } + if len(rendered) > 0 { + fmt.Fprintf(out, "Rendered %d template(s) into the sandbox.\n", len(rendered)) + } + return nil +} + +// hostCommand runs a secrets.yaml command through the user's shell on the +// host. CombinedOutput so a failure's stderr reaches the error message. +func hostCommand(ctx context.Context, command string) ([]byte, error) { + return exec.CommandContext(ctx, "sh", "-c", command).CombinedOutput() +} +``` + +- [ ] **Step 4: Run to verify pass** — `go test ./internal/cli/ -v` (fix any callers/tests still using the one-value `ensureRunning`). +- [ ] **Step 5: Lint, format, build, commit** + +```bash +git add internal/cli/ && git commit -m "feat: resolve and push rendered templates at start, apply, and boot" +``` + +--- + +### Task 6: `code-vm secrets` and profile CLI surfacing + +**Files:** +- Create: `internal/cli/secrets.go` +- Modify: `internal/cli/profile.go` (`add` warning, `list` marker), `internal/cli/root.go` (register) +- Test: `internal/cli/secrets_test.go`, extend `internal/cli/profile_test.go` + +**Interfaces:** +- Consumes: `profile.DeclaredSecrets/Vars`, `MissingSecretSnippet`, `config.LoadSecrets/SecretsPathFor`, `loadConfigWithProfiles`. +- Produces: `newSecretsCmd()` registered on root. Output: one line per declared secret/var — `name mapped|UNMAPPED (profiles) description` — names and status only, never values; unmapped secrets with a hint get the snippet printed after the table. Exit code 0 either way (it is a report, not a gate). + +- [ ] **Step 1: Write the failing tests** + +`internal/cli/secrets_test.go` (reuse `withScratchConfig` + a scratch profile fixture as in profile_test.go): + +```go +func TestSecretsListsMappedAndUnmapped(t *testing.T) { + root := NewRootCmd() + dir := withScratchConfig(t) + pdir := filepath.Join(dir, "profiles", "p") + os.MkdirAll(filepath.Join(pdir, "templates"), 0o755) + os.WriteFile(filepath.Join(pdir, "profile.yaml"), []byte( + "secrets:\n mapped-one:\n description: has a mapping\n missing-one:\n suggest: gopass show -o x\nvars:\n url: {}\n"), 0o644) + os.WriteFile(filepath.Join(pdir, "templates", ".npmrc"), []byte("${secret:mapped-one}${secret:missing-one}${var:url}\n"), 0o644) + os.WriteFile(filepath.Join(dir, "secrets.yaml"), []byte("secrets:\n mapped-one:\n value: v\n"), 0o600) + appendConfig(t, "profiles:\n - p\n") // helper: append to the scratch config file (add it if profile_test.go lacks one) + + var out bytes.Buffer + root.SetOut(&out) + root.SetErr(&out) + root.SetArgs([]string{"secrets"}) + if err := root.Execute(); err != nil { + t.Fatalf("secrets: %v", err) + } + s := out.String() + for _, want := range []string{"mapped-one", "missing-one", "UNMAPPED", "url", + "command: gopass show -o x"} { + if !strings.Contains(s, want) { + t.Errorf("output missing %q:\n%s", want, s) + } + } + if strings.Contains(s, "v\n") && strings.Contains(s, "value") { + t.Errorf("secret values must never be printed:\n%s", s) + } +} +``` + +Extend profile tests: `TestProfileAddWarnsAboutDeclaredSecrets` (add a git fixture whose profile.yaml declares a secret + a matching template; assert the add output contains the secret name) and extend `TestProfileListShowsStatus` (a secret-declaring profile's row contains a `secrets` marker). + +- [ ] **Step 2: Run to verify failure**, **Step 3: Implement** + +`newSecretsCmd`: `loadConfigWithProfiles()`; collect `DeclaredSecrets`/`DeclaredVars`; `config.LoadSecrets(config.SecretsPathFor(path))`; print the table (`%-24s %-9s %-20s %s` name/status/profiles/description; vars check `c.Vars`); after the table, for each unmapped secret print a blank line, `# add to :` and `profile.MissingSecretSnippet(d)`. Register in `root.go`. + +`profile.go` `add`: after `profile.Load`, when the loaded manifest declares secrets, append to the printed output: `This profile declares secrets: . Mapping them in secrets.yaml makes their values readable by the agent (and every active profile's hook).` + +`profile.go` `list`: append a ` secrets` marker column entry for profiles with declarations (keep the existing column layout stable: add the marker to the description column's line end, e.g. `desc + " [secrets: a, b]"` — simplest change that satisfies "marks profiles that declare secrets"). + +- [ ] **Step 4: Run to verify pass**, **Step 5: Lint, format, build, commit** + +```bash +git add internal/cli/ && git commit -m "feat: add code-vm secrets and surface secret declarations in profile add/list" +``` + +--- + +### Task 7: Integration coverage and documentation + +**Files:** +- Modify: `test-vm-sandbox.sh` (inside the existing "Profiles" section), `README.md` + +**Interfaces:** +- Consumes: everything; suite conventions (`pass`/`fail`/`assert_ok`/`assert_fails`, `adm`, `agent`, `yq -i`, `$TEST_CONFIG_DIR`). + +- [ ] **Step 1: Extend the suite's Profiles section** + +Extend the existing fixture profile (`$PROFILE_FIXTURE`) BEFORE the `profile apply` call: add to its `profile.yaml`: + +```yaml +secrets: + test-token: + description: integration fixture token +vars: + test-url: {} +``` + +plus `mkdir -p "$PROFILE_FIXTURE/templates/.config"` and a template: + +```bash +printf 'token=${secret:test-token}\nurl=${var:test-url}\nkeep=${env.HOME}\n' \ + > "$PROFILE_FIXTURE/templates/.config/fixture.conf" +``` + +Map the inputs in the scratch config tree: + +```bash +printf 'secrets:\n test-token:\n value: sekrit-value\n' > "$TEST_CONFIG_DIR/secrets.yaml" +chmod 0600 "$TEST_CONFIG_DIR/secrets.yaml" +yq -i '.vars = {"test-url": "https://fixture.example"}' "$CONFIG_FILE" +``` + +After the existing `profile apply` assertions, add: + +```bash +RENDERED="/home/$AGENT_USER/.config/fixture.conf" +if adm cat "$RENDERED" 2> /dev/null | grep -q 'token=sekrit-value'; then + pass "template secret is substituted in the guest" +else + fail "template secret is substituted in the guest" +fi +assert_ok "template var is substituted" \ + adm grep -q 'url=https://fixture.example' "$RENDERED" +assert_ok "unrelated placeholders pass through" \ + adm grep -qF 'keep=${env.HOME}' "$RENDERED" +if [ "$(adm stat -c '%u %a' "$RENDERED")" = "$(id -u) 600" ]; then + pass "rendered template is agent-owned 0600" +else + fail "rendered template is agent-owned 0600 (got $(adm stat -c '%u %a' "$RENDERED"))" +fi + +# Rotation: change the mapped value, re-apply, and the rendered file updates. +printf 'secrets:\n test-token:\n value: rotated-value\n' > "$TEST_CONFIG_DIR/secrets.yaml" +"${CODE_VM_ARGS[@]}" profile apply > /dev/null 2>&1 +assert_ok "a mapping change plus apply updates the rendered template" \ + adm grep -q 'token=rotated-value' "$RENDERED" + +# A declared-but-unmapped secret must fail apply with the snippet, before +# anything reaches the guest. yq, not printf-append: a second top-level +# `secrets:` key would be a YAML duplicate-key parse error, a different +# failure than the one under test. +yq -i '.secrets.extra-unmapped = {"suggest": "gopass show -o nope"}' "$PROFILE_FIXTURE/profile.yaml" +printf 'x=${secret:extra-unmapped}\n' > "$PROFILE_FIXTURE/templates/.config/extra.conf" +UNMAPPED_OUT=$("${CODE_VM_ARGS[@]}" profile apply 2>&1) +if echo "$UNMAPPED_OUT" | grep -q 'gopass show -o nope'; then + pass "unmapped secret fails apply with the ready-to-paste snippet" +else + fail "unmapped secret fails apply with the ready-to-paste snippet (got: $UNMAPPED_OUT)" +fi +# Restore the fixture to the mapped-only state for the deactivation steps. +rm -f "$PROFILE_FIXTURE/templates/.config/extra.conf" +yq -i 'del(.secrets.extra-unmapped)' "$PROFILE_FIXTURE/profile.yaml" +"${CODE_VM_ARGS[@]}" profile apply > /dev/null 2>&1 + +adm rm -f "$RENDERED" > /dev/null 2>&1 # cleanup with the other fixture artifacts +``` + +(Place the `adm rm -f "$RENDERED"` with the section's existing cleanup block, after the deactivation assertions; the fixture profile dir removal already covers the host side. Deliberate omission: the spec's "survives a restart" integration bullet is not asserted — persistence of a regular file in the ext4 home exercises no mechanism of this feature, and a dedicated restart would add minutes to the suite; the start-time push path is the same code the apply path already covers.) + +Run `bash -n test-vm-sandbox.sh` and `mise run lint`. + +- [ ] **Step 2: README** + +Add a `### Credentials` subsection under Profiles documenting: the trust model (hints never execute; mapping a secret exposes it to the agent and every active profile's hook), `secrets.yaml` with the gopass example, `vars:` in config.yaml, the Maven `settings.xml` walkthrough (placeholders for ``/``, static proxies/mirrors shipped as-is), when resolution happens (start/apply/boot — never per invocation), the boot-ordering caveat (hooks run before the first push after a cold boot; hooks must tolerate absent templates), and rotation (rotate at source → restart or `profile apply`). Also add `code-vm secrets` to the command list. + +- [ ] **Step 3: Full verification** + +`mise run test:unit && mise run lint && mise run fmt-check && mise run build`, then `mise run test:vm` (controller may run this; all profile-section assertions plus the pre-existing 111 must pass). + +- [ ] **Step 4: Commit** + +```bash +git add test-vm-sandbox.sh README.md && git commit -m "test: cover profile secrets/templates in the suite; document credentials" +``` diff --git a/docs/superpowers/specs/2026-08-20-profile-secrets-design.md b/docs/superpowers/specs/2026-08-20-profile-secrets-design.md new file mode 100644 index 0000000..08166d9 --- /dev/null +++ b/docs/superpowers/specs/2026-08-20-profile-secrets-design.md @@ -0,0 +1,243 @@ +# Profile Secrets and Templates — Design + +**Date:** 2026-08-20 +**Status:** Approved design, pending implementation plan + +## Context + +Credentialed tool configuration is the remaining gap in the sandbox: a Maven +`~/.m2/settings.xml` mixes shareable shape (proxies, mirrors) with per-user +credentials (repository server auth). The predecessor mechanism +(`.sandbox-secrets.yaml`) was removed deliberately: it resolved `source:` +commands on the host from an agent-authored workspace file — host command +execution from inside the sandbox. The integration suite still pins that +removal. + +Profiles (2026-08-19 design) now provide a host-trusted, team-shareable +bundle format. This design extends them with **templates** rendered from +**secrets** (resolved on the host from the user's own configuration) and +**variables** (plain literals), delivered per start/apply directly into the +agent's home. + +## Goals + +- A team profile ships the whole shape of a credentialed config (e.g. + `settings.xml` with proxies and mirrors) once; each user supplies only + their credential sources. +- Secret values come from the host — a user-authored command (gopass, pass, + op, …) or a literal — never from the guest, the workspace, or the profile. +- Profile authors never gain host command execution: profiles may *suggest* + a source as inert, displayed-only metadata; nothing executes until the + user copies it into their own mapping. +- No per-invocation secret-manager calls (pinentry): resolution happens at + VM start and explicit `profile apply` only. +- Secrets never travel through the Lima template (`mode: data` persists in + `~/.lima//lima.yaml`) and never land in the world-readable guest + profile tree. + +## Non-goals (v1) + +- Environment-variable or interactive-prompt sources (command + literal + only). +- Per-profile secret scoping. Any active profile's hook can read the whole + agent home, so scoping names to profiles would be an illusion; the design + is honest about the real boundary instead (see Security). +- Removing rendered files on profile deactivation (consistent with the + existing files/ non-goal; `code-vm recreate` is the clean slate). +- Escaping/encoding of substituted values (values are opaque bytes; a value + that breaks the target format is the user's own, as in any hand-written + config). +- Secret rotation detection. Rotation = rotate at the source, then restart + or `profile apply`. + +## Decisions + +| Area | Decision | +|---|---| +| Trust line | Hybrid: profiles declare secret/var *names* with optional inert `suggest:`/`description:`; only the user's local mapping executes anything | +| Templates | Shipped by profiles under `templates/`, a home-mirroring tree with placeholders; generic across tools (Maven, npm, Gradle, …) | +| Secret sources (v1) | `command:` (host shell, stdout is the value, trailing newline stripped) and `value:` (literal) | +| Variables | Plain literals in `config.yaml` under `vars:`; non-secret knobs | +| Resolve/render/push timing | `code-vm start`, `code-vm profile apply`, and any invocation whose `ensureRunning` actually booted the VM | +| Delivery | Host-side render, staged push directly to `/home//`, agent-owned, 0600, no exec bit — the git-identity path | +| Placeholder syntax | `${secret:name}` and `${var:name}` only; all other `${...}` passes through untouched | +| Failure mode | A declared-but-unmapped secret/var fails start/apply with an actionable error carrying a ready-to-paste snippet | + +## Profile bundle extensions + +``` +wetf-maven/ + profile.yaml + templates/ # home-mirroring tree, rendered before delivery + .m2/settings.xml + files/ ... # unchanged +``` + +`profile.yaml` gains two sections: + +```yaml +secrets: + wetf-repo-user: + description: Artifactory user for wetf-snapshots/releases + suggest: gopass show -o wetf/artifactory-user # inert hint, never executed + wetf-repo-password: + suggest: gopass show -o wetf/artifactory-password +vars: + artifactory-url: + description: Base URL of the Artifactory instance +``` + +Validation, host-side at load, extending the existing rules: + +- Secret and var names match the profile-name pattern + (`[a-zA-Z0-9][a-zA-Z0-9-]{0,62}`): they appear in placeholders and error + messages, never in shell commands or guest paths. +- `description` and `suggest` are inert display strings — never executed, + never substituted into anything, never delivered to the guest. +- `templates/` entries get exactly the `files/` treatment: conservative + charset, no `..`, no absolute paths, no symlinks anywhere (tree root, + directories, entries), locked Claude settings paths rejected. +- A template and a `files/` entry targeting the same destination within one + profile is a validation error. Across profiles, a same-kind collision + (`files/` vs `files/`, `templates/` vs `templates/`) still resolves by list + order, later wins. A `files/`-vs-`templates/` collision across profiles is + rejected as a `LoadAll` validation error instead: boot delivers rendered + templates after the file tree (template wins) while `profile apply` pushes + rendered templates before re-laying the file tree (`files/` wins), so which + entry would actually win differs between the two paths for the same + config — order-respecting suppression can't be made consistent across both. +- A template referencing an undeclared `${secret:...}` or `${var:...}` name + is a load-time validation error: bundles cannot quietly depend on inputs + they never declared. Declared-but-unreferenced names are allowed (hooks + may not need templates). +- A profile with `secrets:`/`vars:`/`templates/` but nothing else remains a + valid, non-empty profile. + +## User-side values + +Sensitivity decides the home: + +- **`~/.config/code-vm/secrets.yaml`** — 0600, user-authored, never + distributed. It lives inside the config tree the mount guards already + protect, and is host-trusted exactly like `config.yaml`: + + ```yaml + secrets: + wetf-repo-user: + command: gopass show -o wetf/artifactory-user + wetf-repo-password: + command: gopass show -o wetf/artifactory-password + low-value-token: + value: abc123 # literal; a footgun for real credentials + ``` + + `command` runs on the host through the shell at resolve time; stdout with + one trailing newline stripped is the value. A failing command fails the + start/apply with the command's stderr. `command` and `value` are mutually + exclusive per entry. code-vm warns when the file is group/world readable. + +- **`vars:` in `config.yaml`** — a plain `map[string]string` of non-secret + literals (URLs, org names), handled like every other config key. + +Mappings are global, not per-profile: a name maps once, and every active +profile declaring that name receives the same value (see Security for why +scoping would be an illusion). + +## Rendering + +Plain string substitution, host-side. Only the exact forms `${secret:name}` +and `${var:name}` substitute; any other `${...}` (Maven properties, +`${env.FOO}`) passes through byte-for-byte. Values are opaque bytes with no +escaping layer. Each secret's command runs once per resolve pass, however +many templates reference it. Rendering happens after profile load and before +any push; a resolution or substitution failure aborts the whole start/apply +before anything reaches the guest. + +## Delivery and lifecycle + +Resolve → render → push runs at exactly three moments: + +1. `code-vm start`, after the readiness gate; +2. `code-vm profile apply`; +3. any invocation whose `ensureRunning` actually booted the VM — the first + command after a cold boot must not see a half-configured home. + +A plain invocation against a running VM resolves nothing — no pinentry. + +Delivery is a direct staged push (the git-identity path: host temp file, +0600, random staging name, agent-privilege install) to +`/home//`, owner agent, mode 0600, no exec bit. Rendered content +never enters `mode: data`, never enters `/usr/local/share/sandbox-profiles`, +and exists on the host only as the transient staging temp file. Rendered +files persist in the agent home across restarts and are canonically +re-pushed on every start/apply while the profile is active. The guest +applier is uninvolved: an unattended guest boot has no host to resolve +secrets, and `code-vm start` — the only way users boot — is host-driven and +pushes immediately after readiness. + +## Security + +The trust statement, extended (printed by `profile add`, now including the +profile's declared secret names): + +> Mapping a secret makes its value readable by the agent — and therefore by +> every active profile's hook. Install profiles from sources you trust with +> the secrets you map. + +The real boundaries: + +- **The user chooses what exists at all.** Only names mapped in the user's + own `secrets.yaml` ever resolve; declared-but-unmapped names fail loudly + and execute nothing. +- **Hints never execute.** `suggest:` strings are displayed and offered as + copy-paste snippets, nothing more. Host command execution comes only from + the user's own file. +- **Guest exposure is inherent and bounded.** Tools read their configs as + the agent, so rendered values are agent-readable by design. Exfiltration + is bounded by the egress allowlist and recorded in the proxy log. Users + should map only sandbox-appropriate credentials (e.g. a repo-read token, + not an org admin password). +- **Nothing agent- or workspace-authored reaches resolution.** Profile + sources are mount-guarded and symlink-rejected (2026-08-19 design, as + hardened); `secrets.yaml` sits in the same guarded config tree; the + workspace remains untrusted. The old mechanism's regression guards stay. +- **Host residue is transient.** Rendered values exist on the host only in + the 0600 staging temp file, deleted after the push; never in + `~/.lima/*/lima.yaml`. +- `profile list` marks profiles that declare secrets. + +## CLI/UX + +- `code-vm secrets` — lists the union of declared secrets and vars across + active profiles: name, declaring profile(s), description, mapped/unmapped + (names and status only, never values), and for unmapped secrets with a + hint, a ready-to-paste `secrets.yaml` snippet. +- Start/apply failure for a missing mapping names the profile, the key, the + description, and prints the same snippet. +- README gains a "Credentials" section documenting the trust model, the + Maven example end to end, and the rotation story (rotate at source → + restart or `profile apply`). + +## Testing + +Existing style: table-driven unit tests, fake runners, integration suite. + +- **Manifest/template validation** — secret/var name rules, inert hint + handling, undeclared-placeholder rejection, template/file collision, + symlinked `templates/` rejection, locked-settings rejection. +- **Rendering** — substitution of both namespaces, `${...}` passthrough, + opaque-bytes fidelity, one-command-per-secret resolution with an injected + host runner, command-failure and missing-mapping error text (including + the snippet). +- **secrets.yaml** — load, 0600 permission warning, command/value mutual + exclusion. +- **Session push** — fake-runner argv assertions: staged install to home + paths, agent uid/gid numeric, 0600. +- **CLI** — resolve/push triggered by start, apply, and boot-causing + invocations, and by nothing else; `code-vm secrets` output for + mapped/unmapped states. +- **Integration** — fixture profile with a template using one + literal-mapped secret and one var: rendered file lands 0600 agent-owned + with substituted content, survives a restart, updates after a mapping + change plus `profile apply`; a declared-but-unmapped secret fails apply + with the snippet in the error. diff --git a/internal/cli/mount.go b/internal/cli/mount.go index 2b06c85..f331f84 100644 --- a/internal/cli/mount.go +++ b/internal/cli/mount.go @@ -80,11 +80,22 @@ func newMountCmd() *cobra.Command { fmt.Fprintln(out, "VM is not running; the new mount applies on next start.") return nil } + // Resolve (and render) before stopping the VM at all: resolution + // needs nothing from the guest, so an unmapped secret must abort + // before the running VM is even stopped, let alone left stopped + // on a failed restart. + rendered, err := resolveRendered(cmd.Context(), updated, profiles, path, out) + if err != nil { + return err + } fmt.Fprintln(out, "Restarting the VM to apply the new mount...") if err := cl.Stop(cmd.Context()); err != nil { return err } - return ensureRunning(cmd.Context(), cl, updated, profiles) + if _, err := ensureRunning(cmd.Context(), cl, updated, profiles); err != nil { + return err + } + return pushRendered(cmd.Context(), cl, updated, profiles, rendered, out) }, } } diff --git a/internal/cli/mount_test.go b/internal/cli/mount_test.go index a2ad58b..79cc5fd 100644 --- a/internal/cli/mount_test.go +++ b/internal/cli/mount_test.go @@ -110,3 +110,41 @@ func TestMountRefusesProfilesDirectoryWithoutSavingOrRestarting(t *testing.T) { t.Errorf("expected no guest interaction before the guard runs, got %v", r.calls) } } + +// Resolution needs nothing from the guest, so an unmapped secret must fail +// `code-vm mount` before the running VM is stopped for the restart — leaving +// it running rather than stopped-and-not-restarted. +func TestMountFailsBeforeStoppingOnUnmappedSecret(t *testing.T) { + root := NewRootCmd() + dir := withScratchConfig(t) + pdir := filepath.Join(dir, "profiles", "p") + if err := os.MkdirAll(filepath.Join(pdir, "templates"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(pdir, "profile.yaml"), + []byte("secrets:\n tok:\n suggest: gopass show -o t\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(pdir, "templates", ".npmrc"), []byte("${secret:tok}\n"), 0o644); err != nil { + t.Fatal(err) + } + appendConfig(t, "profiles:\n - p\n") + // No secrets.yaml: "tok" is unmapped. + + other := t.TempDir() + r := installFakeClient(t, "Running") + var out bytes.Buffer + root.SetOut(&out) + root.SetErr(&out) + root.SetArgs([]string{"mount", other}) + err := root.Execute() + if err == nil || !strings.Contains(err.Error(), "gopass show -o t") { + t.Fatalf("mount = %v, want an unmapped-secret error with the suggest snippet; output:\n%s", err, out.String()) + } + if ranAny(r.calls, "stop") { + t.Errorf("VM must not be stopped before resolution succeeds, calls=%v", r.calls) + } + if ranAny(r.calls, "copy") { + t.Errorf("no file may be staged into the guest before resolution succeeds, calls=%v", r.calls) + } +} diff --git a/internal/cli/profile.go b/internal/cli/profile.go index 43bee04..7f7c12f 100644 --- a/internal/cli/profile.go +++ b/internal/cli/profile.go @@ -109,7 +109,8 @@ func newProfileAddCmd() *cobra.Command { if err := runGit(cmd.Context(), cmd, "", "clone", url, dst); err != nil { return err } - if _, loadErr := profile.Load(dir, name); loadErr != nil { + loaded, loadErr := profile.Load(dir, name) + if loadErr != nil { // A broken bundle must not linger: it would fail every // loadConfigWithProfiles the moment someone activates it. if rmErr := os.RemoveAll(dst); rmErr != nil { @@ -120,6 +121,19 @@ func newProfileAddCmd() *cobra.Command { out := cmd.OutOrStdout() fmt.Fprintf(out, "Installed profile %s.\n\n%s\n\n", name, trustWarning) fmt.Fprintf(out, "Activate it by adding to your config:\n\nprofiles:\n - %s\n", name) + // Flagged separately from the generic trust warning above: a + // mapped secret's value becomes readable by the agent and by + // every other active profile's hook, not just this one, which + // is a sharper claim than "this profile is host-trusted". + if declared := profile.DeclaredSecrets([]profile.Profile{loaded}); len(declared) > 0 { + names := make([]string, len(declared)) + for i, d := range declared { + names[i] = d.Name + } + fmt.Fprintf(out, "\nThis profile declares secrets: %s. Mapping them in secrets.yaml makes "+ + "their values readable by the agent (and every active profile's hook).\n", + strings.Join(names, ", ")) + } return nil }, } @@ -229,6 +243,16 @@ func newProfileListCmd() *cobra.Command { desc = err.Error() } else { desc = p.Manifest.Description + // Surfaced right in the list a user already reaches for, + // so a profile's secret needs are visible before they + // even get as far as `code-vm secrets`. + if declared := profile.DeclaredSecrets([]profile.Profile{p}); len(declared) > 0 { + names := make([]string, len(declared)) + for i, d := range declared { + names[i] = d.Name + } + desc += fmt.Sprintf(" [secrets: %s]", strings.Join(names, ", ")) + } } origin := gitOrigin(filepath.Join(dir, name)) fmt.Fprintf(out, "%-24s %-8s %-40s %s\n", name, state, origin, desc) @@ -286,7 +310,7 @@ func newProfileApplyCmd() *cobra.Command { Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { ctx := cmd.Context() - c, profiles, _, err := loadConfigWithProfiles() + c, profiles, cfgPath, err := loadConfigWithProfiles() if err != nil { return err } @@ -298,6 +322,15 @@ func newProfileApplyCmd() *cobra.Command { if status != "Running" { return fmt.Errorf("the VM is not running; profiles apply automatically at boot — start it with `code-vm start`") } + // Resolve (and render) before touching the guest at all: + // resolution needs nothing from the guest, and PushProfiles/ + // ApplyAllowlist below stage a new profile tree and make its + // domains live. An unmapped-secret failure must abort before + // any of that happens, not after. + rendered, err := resolveRendered(ctx, c, profiles, cfgPath, cmd.OutOrStdout()) + if err != nil { + return err + } d := agentDeps(cl, c, profiles) if err := session.PushProfiles(ctx, d, profile.GuestFiles(profiles)); err != nil { return fmt.Errorf("push profiles: %w", err) @@ -307,6 +340,12 @@ func newProfileApplyCmd() *cobra.Command { if err := session.ApplyAllowlist(ctx, d); err != nil { return fmt.Errorf("apply allowlist: %w", err) } + // Rendered templates land before hooks run: a hook may consume + // the configs a profile's own templates just produced. Already + // resolved above, so this only pushes — no second resolution. + if err := pushRendered(ctx, cl, c, profiles, rendered, cmd.OutOrStdout()); err != nil { + return err + } if err := session.ApplyProfiles(ctx, d); err != nil { return fmt.Errorf("apply profiles: %w", err) } diff --git a/internal/cli/profile_test.go b/internal/cli/profile_test.go index f78f64e..8ae7aca 100644 --- a/internal/cli/profile_test.go +++ b/internal/cli/profile_test.go @@ -100,6 +100,21 @@ func withScratchConfig(t *testing.T) string { return dir } +// appendConfig appends raw YAML to the scratch config file written by +// withScratchConfig, the same read-append-rewrite dance several tests below +// already did inline for activating profiles or setting vars. +func appendConfig(t *testing.T, extra string) { + t.Helper() + cfg := configPath + base, err := os.ReadFile(cfg) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(cfg, append(base, []byte(extra)...), 0o600); err != nil { + t.Fatal(err) + } +} + func TestProfileAddClonesAndValidates(t *testing.T) { root := NewRootCmd() dir := withScratchConfig(t) @@ -127,6 +142,57 @@ func TestProfileAddClonesAndValidates(t *testing.T) { } } +// makeGitProfileWithSecret creates a local git repo laid out as a valid +// profile that declares one secret and references it from a template, so +// `profile add` has something to warn about. +func makeGitProfileWithSecret(t *testing.T) string { + t.Helper() + src := filepath.Join(t.TempDir(), "secret-profile") + if err := os.MkdirAll(filepath.Join(src, "templates"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(src, "profile.yaml"), []byte( + "description: fixture with a secret\nsecrets:\n api-token:\n description: an API token\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(src, "templates", ".netrc"), []byte("${secret:api-token}\n"), 0o644); err != nil { + t.Fatal(err) + } + for _, args := range [][]string{ + {"init", "-q"}, {"add", "."}, + {"-c", "user.email=t@t", "-c", "user.name=t", "commit", "-q", "-m", "v1"}, + } { + cmd := exec.Command("git", args...) + cmd.Dir = src + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + } + return src +} + +func TestProfileAddWarnsAboutDeclaredSecrets(t *testing.T) { + root := NewRootCmd() + _ = withScratchConfig(t) + src := makeGitProfileWithSecret(t) + + var out bytes.Buffer + root.SetOut(&out) + root.SetErr(&out) + root.SetArgs([]string{"profile", "add", src}) + if err := root.Execute(); err != nil { + t.Fatalf("profile add: %v\n%s", err, out.String()) + } + + got := out.String() + if !strings.Contains(got, "declares secrets") || !strings.Contains(got, "api-token") { + t.Errorf("expected a warning naming the declared secret, got:\n%s", got) + } + if !strings.Contains(got, "secrets.yaml") { + t.Errorf("expected the warning to point at secrets.yaml, got:\n%s", got) + } +} + func TestProfileAddRejectsInvalidBundle(t *testing.T) { root := NewRootCmd() dir := withScratchConfig(t) @@ -181,6 +247,7 @@ func TestProfileListShowsStatus(t *testing.T) { writeProfile(t, profilesRoot, "alpha", "description: active one\npackages: [fish]\n") writeProfile(t, profilesRoot, "beta", "description: inactive one\npackages: [fish]\n") writeProfile(t, profilesRoot, "gamma", "description: broken\npackages: [Not_Valid]\n") + writeProfile(t, profilesRoot, "delta", "description: has a secret\nsecrets:\n a:\n description: d\n") cfg := filepath.Join(dir, "config.yaml") base, err := os.ReadFile(cfg) @@ -203,12 +270,14 @@ func TestProfileListShowsStatus(t *testing.T) { // Parse per-line rather than substring-matching the whole output: "active" // is itself a substring of "inactive". states := map[string]string{} + lines := map[string]string{} for _, line := range strings.Split(strings.TrimSpace(out.String()), "\n") { fields := strings.Fields(line) if len(fields) < 2 { continue } states[fields[0]] = fields[1] + lines[fields[0]] = line } if states["alpha"] != "active" { t.Errorf("alpha state = %q, want %q; full output:\n%s", states["alpha"], "active", out.String()) @@ -219,6 +288,12 @@ func TestProfileListShowsStatus(t *testing.T) { if states["gamma"] != "invalid" { t.Errorf("gamma state = %q, want %q; full output:\n%s", states["gamma"], "invalid", out.String()) } + if !strings.Contains(lines["delta"], "[secrets: a]") { + t.Errorf("delta row should carry a secrets marker, got: %q", lines["delta"]) + } + if strings.Contains(lines["alpha"], "[secrets:") { + t.Errorf("alpha row should not carry a secrets marker, got: %q", lines["alpha"]) + } } // The design spec's CLI section says `profile list` shows "git origin if @@ -569,6 +644,67 @@ func TestProfileApplyPushesAndRuns(t *testing.T) { } } +// Resolution (LoadSecrets/ResolveSecrets/ResolveVars) needs nothing from the +// guest, so an unmapped secret must fail apply before PushProfiles stages a +// new profile tree or ApplyAllowlist makes its domains live — otherwise a +// failed apply leaves a half-applied profile (new tree, live domains, no +// templates) whose hooks still run on the next boot. +func TestProfileApplyFailsBeforeTouchingGuestOnUnmappedSecret(t *testing.T) { + root := NewRootCmd() + dir := withScratchConfig(t) + pdir := filepath.Join(dir, "profiles", "p") + if err := os.MkdirAll(filepath.Join(pdir, "templates"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(pdir, "profile.yaml"), + []byte("secrets:\n tok:\n suggest: gopass show -o t\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(pdir, "templates", ".npmrc"), []byte("${secret:tok}\n"), 0o644); err != nil { + t.Fatal(err) + } + cfg := configPath + b, err := os.ReadFile(cfg) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(cfg, append(b, []byte("profiles:\n - p\n")...), 0o600); err != nil { + t.Fatal(err) + } + // No secrets.yaml is written at all: "tok" is unmapped. + + r := installFakeClient(t, "Running") + root.SetArgs([]string{"profile", "apply"}) + err = root.Execute() + if err == nil || !strings.Contains(err.Error(), "gopass show -o t") { + t.Fatalf("profile apply = %v, want an unmapped-secret error with the suggest snippet", err) + } + // "status" is the only guest call resolution's own precondition check + // makes; nothing that mutates guest state (push, rm -rf, allowlist) may + // have run. + for _, unwanted := range []string{ + "rm -rf /usr/local/share/sandbox-profiles", + "install -d -m 0755 /usr/local/share/sandbox-profiles", + "squid -k reconfigure", + "apply-profiles.sh", + } { + if ranAny(r.calls, unwanted) { + t.Errorf("guest call %q must not happen before resolution succeeds, calls=%v", unwanted, r.calls) + } + } + if copies := func() int { + n := 0 + for _, c := range r.calls { + if len(c) > 0 && c[0] == "copy" { + n++ + } + } + return n + }(); copies != 0 { + t.Errorf("no file may be staged into the guest before resolution succeeds, copies=%d calls=%v", copies, r.calls) + } +} + func TestProfileApplyRequiresRunningVM(t *testing.T) { root := NewRootCmd() withScratchConfig(t) diff --git a/internal/cli/recreate.go b/internal/cli/recreate.go index e172ac8..f04c61d 100644 --- a/internal/cli/recreate.go +++ b/internal/cli/recreate.go @@ -19,7 +19,7 @@ func newRecreateCmd() *cobra.Command { "installed plugins and the Docker image cache. Workspace files live\n" + "on the host and are unaffected.", RunE: func(cmd *cobra.Command, _ []string) error { - c, profiles, _, err := loadConfigWithProfiles() + c, profiles, cfgPath, err := loadConfigWithProfiles() if err != nil { return err } @@ -32,11 +32,22 @@ func newRecreateCmd() *cobra.Command { return fmt.Errorf("aborted") } } + // Resolve (and render) before deleting anything: resolution needs + // nothing from the guest, and an unmapped-secret failure must + // never destroy the user's VM — critical here, since Delete is + // irreversible (guest disk, Claude auth, Docker image cache). + rendered, err := resolveRendered(cmd.Context(), c, profiles, cfgPath, cmd.OutOrStdout()) + if err != nil { + return err + } cl := clientFor(c) if err := cl.Delete(cmd.Context()); err != nil { return err } - return ensureRunning(cmd.Context(), cl, c, profiles) + if _, err := ensureRunning(cmd.Context(), cl, c, profiles); err != nil { + return err + } + return pushRendered(cmd.Context(), cl, c, profiles, rendered, cmd.OutOrStdout()) }, } cmd.Flags().BoolVar(&yes, "yes", false, "skip the confirmation prompt") diff --git a/internal/cli/recreate_test.go b/internal/cli/recreate_test.go new file mode 100644 index 0000000..db41e1e --- /dev/null +++ b/internal/cli/recreate_test.go @@ -0,0 +1,43 @@ +package cli + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// Resolution needs nothing from the guest, so an unmapped secret must fail +// `code-vm recreate` before the VM is deleted — Delete is irreversible (guest +// disk, Claude auth, Docker image cache), so a resolvable-beforehand error +// must never destroy the user's VM. +func TestRecreateFailsBeforeDeletingOnUnmappedSecret(t *testing.T) { + root := NewRootCmd() + dir := withScratchConfig(t) + pdir := filepath.Join(dir, "profiles", "p") + if err := os.MkdirAll(filepath.Join(pdir, "templates"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(pdir, "profile.yaml"), + []byte("secrets:\n tok:\n suggest: gopass show -o t\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(pdir, "templates", ".npmrc"), []byte("${secret:tok}\n"), 0o644); err != nil { + t.Fatal(err) + } + appendConfig(t, "profiles:\n - p\n") + // No secrets.yaml: "tok" is unmapped. + + r := installFakeClient(t, "Running") + root.SetArgs([]string{"recreate", "--yes"}) + err := root.Execute() + if err == nil || !strings.Contains(err.Error(), "gopass show -o t") { + t.Fatalf("recreate = %v, want an unmapped-secret error with the suggest snippet", err) + } + if ranAny(r.calls, "delete") { + t.Errorf("VM must not be deleted before resolution succeeds, calls=%v", r.calls) + } + if ranAny(r.calls, "copy") { + t.Errorf("no file may be staged into the guest before resolution succeeds, calls=%v", r.calls) + } +} diff --git a/internal/cli/root.go b/internal/cli/root.go index 044dfc8..1df754e 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -39,6 +39,7 @@ func NewRootCmd() *cobra.Command { root.AddCommand(newFirewallCmd()) root.AddCommand(newAllowCmd()) root.AddCommand(newProfileCmd()) + root.AddCommand(newSecretsCmd()) return root } diff --git a/internal/cli/secrets.go b/internal/cli/secrets.go new file mode 100644 index 0000000..9358bb7 --- /dev/null +++ b/internal/cli/secrets.go @@ -0,0 +1,71 @@ +package cli + +import ( + "fmt" + "strings" + + "github.com/spf13/cobra" + + "github.com/wetransform/code-vm/internal/config" + "github.com/wetransform/code-vm/internal/profile" +) + +// newSecretsCmd reports the union of secrets and vars declared by the active +// profiles, and which of them are mapped. It is a report, not a gate: unlike +// loadConfigWithProfiles's other callers, an unmapped secret must not fail +// the command, only be called out — the reader needs to see the whole +// picture (mapped and unmapped alike) in one place. Names and status only: +// a secret's resolved value is never read here, let alone printed. +func newSecretsCmd() *cobra.Command { + return &cobra.Command{ + Use: "secrets", + Short: "List secrets and vars declared by the active profiles", + Long: "Lists every secret and var the active profiles declare, whether it is\n" + + "mapped in secrets.yaml (or config.yaml's vars), which profiles declare\n" + + "it, and its description. Never prints a secret's value. For each\n" + + "unmapped secret with a suggested command, also prints a ready-to-paste\n" + + "secrets.yaml snippet.", + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, _ []string) error { + c, profiles, path, err := loadConfigWithProfiles() + if err != nil { + return err + } + sources, warnings, err := config.LoadSecrets(config.SecretsPathFor(path)) + if err != nil { + return err + } + out := cmd.OutOrStdout() + for _, w := range warnings { + fmt.Fprintln(out, w) + } + + declaredSecrets := profile.DeclaredSecrets(profiles) + declaredVars := profile.DeclaredVars(profiles) + fmt.Fprintf(out, "%-24s %-9s %-20s %s\n", "NAME", "STATUS", "PROFILES", "DESCRIPTION") + for _, d := range declaredSecrets { + status := "UNMAPPED" + if _, ok := sources[d.Name]; ok { + status = "mapped" + } + fmt.Fprintf(out, "%-24s %-9s %-20s %s\n", d.Name, status, strings.Join(d.Profiles, ", "), d.Description) + } + for _, d := range declaredVars { + status := "UNMAPPED" + if _, ok := c.Vars[d.Name]; ok { + status = "mapped" + } + fmt.Fprintf(out, "%-24s %-9s %-20s %s\n", d.Name, status, strings.Join(d.Profiles, ", "), d.Description) + } + + secretsPath := config.SecretsPathFor(path) + for _, d := range declaredSecrets { + if _, ok := sources[d.Name]; ok { + continue + } + fmt.Fprintf(out, "\n# add to %s:\n%s", secretsPath, profile.MissingSecretSnippet(d)) + } + return nil + }, + } +} diff --git a/internal/cli/secrets_push_test.go b/internal/cli/secrets_push_test.go new file mode 100644 index 0000000..0ab7a87 --- /dev/null +++ b/internal/cli/secrets_push_test.go @@ -0,0 +1,87 @@ +package cli + +import ( + "context" + "io" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/wetransform/code-vm/internal/lima" + "github.com/wetransform/code-vm/internal/profile" +) + +func TestPushRenderedTemplatesNoOpWithoutDeclarations(t *testing.T) { + r := &recordingRunner{statusOut: "Running"} + c := testCfg(t) + profiles := []profile.Profile{{Name: "plain", Manifest: profile.Manifest{Packages: []string{"git"}}}} + if err := pushRenderedTemplates(context.Background(), lima.Client{R: r}, c, profiles, filepath.Join(t.TempDir(), "config.yaml"), io.Discard); err != nil { + t.Fatalf("pushRenderedTemplates: %v", err) + } + if len(r.calls) != 0 { + t.Errorf("no declarations must mean no guest traffic, got %v", r.calls) + } +} + +func TestPushRenderedTemplatesResolvesAndPushes(t *testing.T) { + dir := t.TempDir() + cfgPath := filepath.Join(dir, "config.yaml") + if err := os.WriteFile(filepath.Join(dir, "secrets.yaml"), []byte("secrets:\n tok:\n value: sekrit\n"), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + r := &recordingRunner{statusOut: "Running"} + c := testCfg(t) + c.Vars = map[string]string{"url": "https://x"} + profiles := []profile.Profile{{ + Name: "p", + Manifest: profile.Manifest{ + Secrets: map[string]profile.SecretSpec{"tok": {}}, + Vars: map[string]profile.VarSpec{"url": {}}, + }, + Templates: []profile.File{{Rel: ".npmrc", Content: []byte("t=${secret:tok};u=${var:url}")}}, + }} + if err := pushRenderedTemplates(context.Background(), lima.Client{R: r}, c, profiles, cfgPath, io.Discard); err != nil { + t.Fatalf("pushRenderedTemplates: %v", err) + } + if !ranAny(r.calls, "install-user-file.sh") || !ranAny(r.calls, ".npmrc") || !ranAny(r.calls, "0600") { + t.Errorf("expected a relay push of .npmrc at 0600, got %v", r.calls) + } +} + +func TestHostCommandReturnsStdoutOnlyOnSuccess(t *testing.T) { + out, err := hostCommand(context.Background(), "echo warn >&2; echo value") + if err != nil { + t.Fatalf("hostCommand: %v", err) + } + if string(out) != "value\n" { + t.Errorf("hostCommand output = %q, want %q (stderr must not leak into the value)", out, "value\n") + } +} + +func TestHostCommandReturnsStderrOnFailure(t *testing.T) { + out, err := hostCommand(context.Background(), "echo oops >&2; exit 3") + if err == nil { + t.Fatalf("hostCommand: expected an error, got nil (out=%q)", out) + } + if !strings.Contains(string(out), "oops") { + t.Errorf("hostCommand output on failure = %q, want it to contain %q", out, "oops") + } +} + +func TestPushRenderedTemplatesMissingMappingFails(t *testing.T) { + r := &recordingRunner{} + c := testCfg(t) + profiles := []profile.Profile{{ + Name: "p", + Manifest: profile.Manifest{Secrets: map[string]profile.SecretSpec{"tok": {Suggest: "gopass show -o t"}}}, + Templates: []profile.File{{Rel: ".npmrc", Content: []byte("${secret:tok}")}}, + }} + err := pushRenderedTemplates(context.Background(), lima.Client{R: r}, c, profiles, filepath.Join(t.TempDir(), "config.yaml"), io.Discard) + if err == nil || !strings.Contains(err.Error(), "gopass show -o t") { + t.Errorf("missing mapping must fail with the snippet, got %v", err) + } + if len(r.calls) != 0 { + t.Errorf("nothing may reach the guest on resolution failure, got %v", r.calls) + } +} diff --git a/internal/cli/secrets_test.go b/internal/cli/secrets_test.go new file mode 100644 index 0000000..1a9e70d --- /dev/null +++ b/internal/cli/secrets_test.go @@ -0,0 +1,62 @@ +package cli + +import ( + "bytes" + "os" + "path/filepath" + "strings" + "testing" +) + +func TestSecretsListsMappedAndUnmapped(t *testing.T) { + root := NewRootCmd() + dir := withScratchConfig(t) + pdir := filepath.Join(dir, "profiles", "p") + if err := os.MkdirAll(filepath.Join(pdir, "templates"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(pdir, "profile.yaml"), []byte( + "secrets:\n mapped-one:\n description: has a mapping\n missing-one:\n suggest: gopass show -o x\nvars:\n url: {}\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(pdir, "templates", ".npmrc"), []byte("${secret:mapped-one}${secret:missing-one}${var:url}\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "secrets.yaml"), []byte("secrets:\n mapped-one:\n value: v\n"), 0o600); err != nil { + t.Fatal(err) + } + appendConfig(t, "profiles:\n - p\n") + + var out bytes.Buffer + root.SetOut(&out) + root.SetErr(&out) + root.SetArgs([]string{"secrets"}) + if err := root.Execute(); err != nil { + t.Fatalf("secrets: %v", err) + } + s := out.String() + for _, want := range []string{"mapped-one", "missing-one", "UNMAPPED", "url", + `command: "gopass show -o x"`} { + if !strings.Contains(s, want) { + t.Errorf("output missing %q:\n%s", want, s) + } + } + if strings.Contains(s, "v\n") && strings.Contains(s, "value") { + t.Errorf("secret values must never be printed:\n%s", s) + } +} + +// TestSecretsExitsZeroWithNoProfiles guards the "report, not a gate" contract: +// an empty union (no active profiles declare anything) is not an error. +func TestSecretsExitsZeroWithNoProfiles(t *testing.T) { + root := NewRootCmd() + withScratchConfig(t) + + var out bytes.Buffer + root.SetOut(&out) + root.SetErr(&out) + root.SetArgs([]string{"secrets"}) + if err := root.Execute(); err != nil { + t.Fatalf("secrets with nothing declared should exit 0: %v", err) + } +} diff --git a/internal/cli/shell.go b/internal/cli/shell.go index 8785a67..3f4407f 100644 --- a/internal/cli/shell.go +++ b/internal/cli/shell.go @@ -7,6 +7,7 @@ import ( "strings" "github.com/wetransform/code-vm/internal/config" + "github.com/wetransform/code-vm/internal/profile" "github.com/wetransform/code-vm/internal/session" ) @@ -34,7 +35,7 @@ func resolveWorkdir(c config.Config, cwd string) (string, error) { // runDefault is the root command's action: bring the VM up, verify the current // directory is shared, then run the command as the agent user at that path. func runDefault(ctx context.Context, args []string) error { - c, profiles, _, err := loadConfigWithProfiles() + c, profiles, cfgPath, err := loadConfigWithProfiles() if err != nil { return err } @@ -47,11 +48,38 @@ func runDefault(ctx context.Context, args []string) error { return err } cl := clientFor(c) - if err := ensureRunning(ctx, cl, c, profiles); err != nil { + // Resolve BEFORE booting, but only when a boot is actually about to + // happen: an already-running VM keeps today's fast path exactly as it + // is — no resolution, no pinentry — since this is the per-invocation hot + // path, not an explicit command like `start`. Status is observed exactly + // once and that single value both gates resolution and is handed to + // ensureRunningWithStatus to decide whether to boot — a second, separate + // Status call here would reopen a TOCTOU: the VM could stop between the + // two checks, so the outer check sees "Running" (skipping resolution) + // while a re-check inside ensureRunning boots it anyway, silently + // skipping template rendering for that boot. + status, err := cl.Status(ctx) + if err != nil { + return err + } + var rendered []profile.Rendered + if status != "Running" { + rendered, err = resolveRendered(ctx, c, profiles, cfgPath, os.Stdout) + if err != nil { + return err + } + } + started, err := ensureRunningWithStatus(ctx, cl, c, profiles, status) + if err != nil { return err } if err := session.Setup(ctx, agentDeps(cl, c, profiles)); err != nil { return fmt.Errorf("session setup: %w", err) } + if started { + if err := pushRendered(ctx, cl, c, profiles, rendered, os.Stdout); err != nil { + return err + } + } return cl.Agent(ctx, workdir, agentCommand(args)) } diff --git a/internal/cli/shell_test.go b/internal/cli/shell_test.go index d0ae2ab..3680682 100644 --- a/internal/cli/shell_test.go +++ b/internal/cli/shell_test.go @@ -1,6 +1,8 @@ package cli import ( + "os" + "path/filepath" "reflect" "strings" "testing" @@ -40,6 +42,127 @@ func TestResolveWorkdirAcceptsCoveredPath(t *testing.T) { } } +// setupShellFixture writes a scratch config with a profile "p" that declares +// an unmapped secret used by a template, activates it, and chdirs into a +// directory the config shares with the guest (resolveWorkdir requires this). +// No secrets.yaml is written, so "tok" stays unmapped throughout. +func setupShellFixture(t *testing.T) { + t.Helper() + dir := withScratchConfig(t) + c, _, err := loadConfig() + if err != nil { + t.Fatal(err) + } + workdir := filepath.Join(c.ProjectsRoot, "repo") + if err := os.MkdirAll(workdir, 0o755); err != nil { + t.Fatal(err) + } + t.Chdir(workdir) + + pdir := filepath.Join(dir, "profiles", "p") + if err := os.MkdirAll(filepath.Join(pdir, "templates"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(pdir, "profile.yaml"), + []byte("secrets:\n tok:\n suggest: gopass show -o t\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(pdir, "templates", ".npmrc"), []byte("${secret:tok}\n"), 0o644); err != nil { + t.Fatal(err) + } + appendConfig(t, "profiles:\n - p\n") +} + +// Resolution needs nothing from the guest, so an unmapped secret must abort +// the bare `code-vm` invocation before ensureRunning ever boots the VM. +func TestRunDefaultFailsBeforeBootingOnUnmappedSecret(t *testing.T) { + root := NewRootCmd() + setupShellFixture(t) + + r := installFakeClient(t, "") // absent: a bare invocation would boot it + root.SetArgs([]string{}) + err := root.Execute() + if err == nil || !strings.Contains(err.Error(), "gopass show -o t") { + t.Fatalf("bare invocation = %v, want an unmapped-secret error with the suggest snippet", err) + } + if r.started() { + t.Errorf("VM must not be started before resolution succeeds, calls=%v", r.calls) + } + if ranAny(r.calls, "copy") { + t.Errorf("no file may be staged into the guest before resolution succeeds, calls=%v", r.calls) + } +} + +// The already-running fast path must keep today's behavior exactly: no +// resolution (so no pinentry) and no push, even when a declared secret has no +// mapping — resolution never even looks at secrets.yaml in this path. +func TestRunDefaultRunningFastPathSkipsResolution(t *testing.T) { + root := NewRootCmd() + setupShellFixture(t) + + r := installFakeClient(t, "Running") + root.SetArgs([]string{}) + if err := root.Execute(); err != nil { + t.Fatalf("bare invocation against a running VM = %v, want no error (fast path must not resolve)", err) + } + if ranAny(r.calls, ".npmrc") { + t.Errorf("fast path must not render/push templates, calls=%v", r.calls) + } +} + +// countStatusCalls reports how many times the runner was asked for `limactl +// list ...` (the status query behind cl.Status). +func countStatusCalls(calls [][]string) int { + n := 0 + for _, c := range calls { + if len(c) > 0 && c[0] == "list" { + n++ + } + } + return n +} + +// runDefault must decide "is it running" exactly once: the same observation +// both gates resolution and is handed to ensureRunningWithStatus to decide +// whether to boot. A second, independent Status call here would reopen the +// TOCTOU this fix closes — the VM stopping between two separate checks could +// leave resolution skipped (outer check saw Running) while ensureRunning +// boots anyway, silently dropping template rendering for that boot. +func TestRunDefaultQueriesStatusExactlyOnce(t *testing.T) { + root := NewRootCmd() + setupShellFixture(t) + + r := installFakeClient(t, "Running") + root.SetArgs([]string{}) + if err := root.Execute(); err != nil { + t.Fatalf("bare invocation against a running VM = %v", err) + } + if n := countStatusCalls(r.calls); n != 1 { + t.Errorf("expected exactly one status query, got %d, calls=%v", n, r.calls) + } +} + +// A Stopped VM is the boot path: resolution must happen before ensureRunning +// boots it, keyed off the very same status observation used to decide the +// boot itself (single-status-decision — see the comment on ensureRunning). +func TestRunDefaultStoppedResolvesBeforeBoot(t *testing.T) { + root := NewRootCmd() + setupShellFixture(t) + + r := installFakeClient(t, "Stopped") + root.SetArgs([]string{}) + err := root.Execute() + if err == nil || !strings.Contains(err.Error(), "gopass show -o t") { + t.Fatalf("bare invocation against a stopped VM = %v, want an unmapped-secret error with the suggest snippet", err) + } + if r.started() { + t.Errorf("VM must not be started before resolution succeeds, calls=%v", r.calls) + } + if n := countStatusCalls(r.calls); n != 1 { + t.Errorf("expected exactly one status query, got %d, calls=%v", n, r.calls) + } +} + func TestResolveWorkdirRejectsUncoveredPathWithActionableError(t *testing.T) { c := config.Default() c.ProjectsRoot = "/home/st/projects" diff --git a/internal/cli/start.go b/internal/cli/start.go index a12ecec..b361571 100644 --- a/internal/cli/start.go +++ b/internal/cli/start.go @@ -2,8 +2,11 @@ package cli import ( "context" + "errors" "fmt" + "io" "os" + "os/exec" "path/filepath" "runtime" @@ -101,36 +104,54 @@ func renderInstanceFile(c config.Config, profiles []profile.Profile) (string, er // from the rendered template; an existing one cannot be started with a // template argument (limactl refuses), so its stored config is replaced with // a freshly resolved render first. -func ensureRunning(ctx context.Context, cl lima.Client, c config.Config, profiles []profile.Profile) error { - if err := c.Validate(); err != nil { - return err - } +// +// started reports whether this call actually booted the VM (status was not +// "Running" on entry). Callers use it to gate work that must run once per +// boot but never on a plain invocation against an already-running VM. +// +// This is a thin wrapper around ensureRunningWithStatus for callers that have +// not already observed the status themselves. A caller that must make another +// decision (e.g. whether to resolve templates) keyed on the same "is it +// running" fact should call cl.Status itself and pass the result to +// ensureRunningWithStatus directly — see runDefault in shell.go — rather than +// go through this wrapper, which would re-query the status and reopen the +// TOCTOU where the VM stops between the two checks. +func ensureRunning(ctx context.Context, cl lima.Client, c config.Config, profiles []profile.Profile) (bool, error) { status, err := cl.Status(ctx) if err != nil { - return err + return false, err + } + return ensureRunningWithStatus(ctx, cl, c, profiles, status) +} + +// ensureRunningWithStatus is ensureRunning's body, taking an already-observed +// status instead of querying it again itself. +func ensureRunningWithStatus(ctx context.Context, cl lima.Client, c config.Config, profiles []profile.Profile, status string) (bool, error) { + if err := c.Validate(); err != nil { + return false, err } if status == "Running" { - return nil + return false, nil } path, err := renderInstanceFile(c, profiles) if err != nil { - return err + return false, err } defer os.Remove(path) if status == "" { - return cl.Start(ctx, path) + return true, cl.Start(ctx, path) } dir, err := cl.InstanceDir(ctx) if err != nil { - return err + return false, err } if dir == "" { - return fmt.Errorf("cannot locate the %s instance directory", lima.InstanceName) + return false, fmt.Errorf("cannot locate the %s instance directory", lima.InstanceName) } if err := cl.ResolveConfigInto(ctx, path, filepath.Join(dir, "lima.yaml")); err != nil { - return err + return false, err } - return cl.StartExisting(ctx) + return true, cl.StartExisting(ctx) } func newStartCmd() *cobra.Command { @@ -138,11 +159,109 @@ func newStartCmd() *cobra.Command { Use: "start", Short: "Start the sandbox VM (idempotent)", RunE: func(cmd *cobra.Command, _ []string) error { - c, profiles, _, err := loadConfigWithProfiles() + c, profiles, cfgPath, err := loadConfigWithProfiles() if err != nil { return err } - return ensureRunning(cmd.Context(), clientFor(c), c, profiles) + ctx := cmd.Context() + // Resolve (and render) before touching the guest at all: resolution + // needs nothing from the guest, so an unmapped secret must abort + // before ensureRunning boots anything. Unlike shell's fast path, + // start always resolves — it is an explicit command, not the + // per-invocation hot path, so resolving even against an + // already-running VM is the right cost/behavior tradeoff. + rendered, err := resolveRendered(ctx, c, profiles, cfgPath, cmd.OutOrStdout()) + if err != nil { + return err + } + cl := clientFor(c) + if _, err := ensureRunning(ctx, cl, c, profiles); err != nil { + return err + } + return pushRendered(ctx, cl, c, profiles, rendered, cmd.OutOrStdout()) }, } } + +// resolveRendered resolves secrets/vars and renders every active profile's +// templates, without touching the guest at all. Split out from +// pushRenderedTemplates so callers that must not mutate any guest state +// before resolution can fail (profile apply: PushProfiles/ApplyAllowlist +// stage a new profile tree and make its domains live, and must not run +// ahead of an unmapped-secret failure) can resolve first and push later. +func resolveRendered(ctx context.Context, c config.Config, profiles []profile.Profile, cfgPath string, out io.Writer) ([]profile.Rendered, error) { + secretsDecl := profile.DeclaredSecrets(profiles) + varsDecl := profile.DeclaredVars(profiles) + templated := false + for _, p := range profiles { + if len(p.Templates) > 0 { + templated = true + } + } + if !templated && len(secretsDecl) == 0 && len(varsDecl) == 0 { + return nil, nil + } + sources, warnings, err := config.LoadSecrets(config.SecretsPathFor(cfgPath)) + if err != nil { + return nil, err + } + for _, w := range warnings { + fmt.Fprintf(out, "warning: %s\n", w) + } + secrets, err := profile.ResolveSecrets(ctx, secretsDecl, sources, hostCommand) + if err != nil { + return nil, err + } + vars, err := profile.ResolveVars(varsDecl, c.Vars) + if err != nil { + return nil, err + } + return profile.RenderTemplates(profiles, secrets, vars), nil +} + +// pushRendered pushes already-resolved rendered templates into the agent +// home. Callers that already called resolveRendered use this directly, so +// resolution never runs twice. +func pushRendered(ctx context.Context, cl lima.Client, c config.Config, profiles []profile.Profile, rendered []profile.Rendered, out io.Writer) error { + d := agentDeps(cl, c, profiles) + for _, r := range rendered { + if err := session.PushUserFile(ctx, d, r.Content, r.Rel, "0600"); err != nil { + return err + } + } + if len(rendered) > 0 { + fmt.Fprintf(out, "Rendered %d template(s) into the sandbox.\n", len(rendered)) + } + return nil +} + +// pushRenderedTemplates resolves secrets/vars and pushes rendered templates +// into the agent home. Callers gate it to start, apply, and boot-causing +// invocations only: resolution may invoke the user's secret manager +// (pinentry), so it must never run on every command. +func pushRenderedTemplates(ctx context.Context, cl lima.Client, c config.Config, profiles []profile.Profile, cfgPath string, out io.Writer) error { + rendered, err := resolveRendered(ctx, c, profiles, cfgPath, out) + if err != nil { + return err + } + return pushRendered(ctx, cl, c, profiles, rendered, out) +} + +// hostCommand runs a secrets.yaml command through the user's shell on the +// host. The value is stdout ONLY — stderr chatter from a succeeding command +// (gpg warnings, deprecation notices) must never leak into a credential. +// On failure, the captured stderr is returned as display text for the error. +// Neither stdin nor a tty is wired up: a command that needs interactive +// pinentry (rather than an already-cached agent/keyring) will hang or fail +// rather than prompt. +func hostCommand(ctx context.Context, command string) ([]byte, error) { + out, err := exec.CommandContext(ctx, "sh", "-c", command).Output() + if err != nil { + var ee *exec.ExitError + if errors.As(err, &ee) { + return ee.Stderr, err + } + return nil, err + } + return out, nil +} diff --git a/internal/cli/start_test.go b/internal/cli/start_test.go index 8c6bcde..381ce2f 100644 --- a/internal/cli/start_test.go +++ b/internal/cli/start_test.go @@ -3,6 +3,7 @@ package cli import ( "context" "os" + "path/filepath" "runtime" "strings" "testing" @@ -46,12 +47,16 @@ func TestEnsureRunningStartsWhenAbsentOrStopped(t *testing.T) { for _, status := range []string{"", "Stopped", "Broken"} { t.Run("status="+status, func(t *testing.T) { r := &recordingRunner{statusOut: status} - if err := ensureRunning(context.Background(), lima.Client{R: r}, testCfg(t), nil); err != nil { + started, err := ensureRunning(context.Background(), lima.Client{R: r}, testCfg(t), nil) + if err != nil { t.Fatalf("ensureRunning: %v", err) } if !r.started() { t.Errorf("expected a start call for status %q, calls=%v", status, r.calls) } + if !started { + t.Errorf("started = false for status %q, want true", status) + } }) } } @@ -61,7 +66,7 @@ func TestEnsureRunningStartsWhenAbsentOrStopped(t *testing.T) { // config via `template copy --embed-all` and then start by name. func TestEnsureRunningUpdatesStoredConfigWhenStopped(t *testing.T) { r := &recordingRunner{statusOut: "Stopped"} - if err := ensureRunning(context.Background(), lima.Client{R: r}, testCfg(t), nil); err != nil { + if _, err := ensureRunning(context.Background(), lima.Client{R: r}, testCfg(t), nil); err != nil { t.Fatalf("ensureRunning: %v", err) } var sawResolve, sawPlainStart bool @@ -87,12 +92,16 @@ func TestEnsureRunningUpdatesStoredConfigWhenStopped(t *testing.T) { func TestEnsureRunningIsNoOpWhenRunning(t *testing.T) { r := &recordingRunner{statusOut: "Running"} - if err := ensureRunning(context.Background(), lima.Client{R: r}, testCfg(t), nil); err != nil { + started, err := ensureRunning(context.Background(), lima.Client{R: r}, testCfg(t), nil) + if err != nil { t.Fatalf("ensureRunning: %v", err) } if r.started() { t.Errorf("must not start an already-running instance, calls=%v", r.calls) } + if started { + t.Errorf("started = true for a Running instance, want false") + } } func TestRenderInstanceFileIsPrivateAndComplete(t *testing.T) { @@ -174,3 +183,37 @@ func TestRenderParamsRejectsTheOtherHostsHypervisor(t *testing.T) { t.Errorf("renderParams with vmType %q on %s = nil error, want a failure", c.VMType, runtime.GOOS) } } + +// Resolution needs nothing from the guest, so an unmapped secret must abort +// `code-vm start` before ensureRunning ever boots the VM — otherwise a VM +// with a stale/half-rendered template set is left running. +func TestStartFailsBeforeBootingOnUnmappedSecret(t *testing.T) { + root := NewRootCmd() + dir := withScratchConfig(t) + pdir := filepath.Join(dir, "profiles", "p") + if err := os.MkdirAll(filepath.Join(pdir, "templates"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(pdir, "profile.yaml"), + []byte("secrets:\n tok:\n suggest: gopass show -o t\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(pdir, "templates", ".npmrc"), []byte("${secret:tok}\n"), 0o644); err != nil { + t.Fatal(err) + } + appendConfig(t, "profiles:\n - p\n") + // No secrets.yaml: "tok" is unmapped. + + r := installFakeClient(t, "") // absent: start would create+boot the VM + root.SetArgs([]string{"start"}) + err := root.Execute() + if err == nil || !strings.Contains(err.Error(), "gopass show -o t") { + t.Fatalf("start = %v, want an unmapped-secret error with the suggest snippet", err) + } + if r.started() { + t.Errorf("VM must not be started before resolution succeeds, calls=%v", r.calls) + } + if ranAny(r.calls, "copy") { + t.Errorf("no file may be staged into the guest before resolution succeeds, calls=%v", r.calls) + } +} diff --git a/internal/config/config.go b/internal/config/config.go index e387fd9..e3a5f46 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -52,6 +52,9 @@ type Config struct { // wins. Each name must exist under the profiles directory next to this // config file; that is checked when profiles are loaded, not here. Profiles []string `yaml:"profiles,omitempty"` + // Vars are non-secret literal values available to profile templates as + // ${var:name}. Secrets never belong here — they go in secrets.yaml. + Vars map[string]string `yaml:"vars,omitempty"` } // Default returns the built-in configuration. Disk is large because Docker @@ -157,6 +160,11 @@ func (c Config) Validate() error { } seenProfiles[p] = true } + for name := range c.Vars { + if !instanceRe.MatchString(name) { + return fmt.Errorf("vars: key %q must be a name like %q", name, "artifactory-url") + } + } return nil } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 0e0cbde..8f1daf2 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -292,3 +292,16 @@ func TestMountsExcludeTree(t *testing.T) { }) } } + +func TestValidateVars(t *testing.T) { + c := Default() + c.ProjectsRoot = "/home/st/projects" + c.Vars = map[string]string{"artifactory-url": "https://x"} + if err := c.Validate(); err != nil { + t.Errorf("Validate: %v", err) + } + c.Vars = map[string]string{"has space": "v"} + if err := c.Validate(); err == nil || !strings.Contains(err.Error(), "vars") { + t.Errorf("Validate = %v, want vars key rejection", err) + } +} diff --git a/internal/config/secrets.go b/internal/config/secrets.go new file mode 100644 index 0000000..b53bdea --- /dev/null +++ b/internal/config/secrets.go @@ -0,0 +1,71 @@ +package config + +import ( + "bytes" + "errors" + "fmt" + "io" + "os" + "path/filepath" + + "gopkg.in/yaml.v3" +) + +// SecretSource is one user-authored mapping in secrets.yaml: exactly one of a +// host command (stdout is the value) or a literal. This file is host-trusted +// like config.yaml — it lives in the same mount-guarded tree and only the +// user writes it; profiles can only *suggest* entries, never install them. +type SecretSource struct { + Command string `yaml:"command"` + Value string `yaml:"value"` +} + +// secretsFile is the secrets.yaml schema. +type secretsFile struct { + Secrets map[string]SecretSource `yaml:"secrets"` +} + +// SecretsPathFor returns the secrets file belonging to a config file: a +// secrets.yaml next to it, protected by the same mount-exclusion guards. +func SecretsPathFor(configPath string) string { + return filepath.Join(filepath.Dir(configPath), "secrets.yaml") +} + +// LoadSecrets reads and validates secrets.yaml. A missing file is an empty +// mapping — profiles without secrets must not require one. Warnings (not +// errors) report loose file permissions: the file holds commands and possibly +// literal credentials. +func LoadSecrets(path string) (map[string]SecretSource, []string, error) { + data, err := os.ReadFile(path) + if errors.Is(err, os.ErrNotExist) { + return map[string]SecretSource{}, nil, nil + } + if err != nil { + return nil, nil, fmt.Errorf("read %s: %w", path, err) + } + var warnings []string + if fi, err := os.Stat(path); err == nil && fi.Mode().Perm()&0o077 != 0 { + warnings = append(warnings, fmt.Sprintf( + "%s is readable by group/others; recommend chmod 0600", path)) + } + var f secretsFile + dec := yaml.NewDecoder(bytes.NewReader(data)) + dec.KnownFields(true) + // An empty file decodes to io.EOF, and an empty secrets.yaml is as valid + // as a missing one. + if err := dec.Decode(&f); err != nil && !errors.Is(err, io.EOF) { + return nil, nil, fmt.Errorf("parse %s: %w", path, err) + } + for name, src := range f.Secrets { + if !instanceRe.MatchString(name) { + return nil, nil, fmt.Errorf("%s: secret name %q: must look like %q", path, name, "repo-user") + } + if (src.Command == "") == (src.Value == "") { + return nil, nil, fmt.Errorf("%s: secret %q: exactly one of command or value must be set", path, name) + } + } + if f.Secrets == nil { + f.Secrets = map[string]SecretSource{} + } + return f.Secrets, warnings, nil +} diff --git a/internal/config/secrets_test.go b/internal/config/secrets_test.go new file mode 100644 index 0000000..e92688f --- /dev/null +++ b/internal/config/secrets_test.go @@ -0,0 +1,72 @@ +package config + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestSecretsPathFor(t *testing.T) { + if got := SecretsPathFor("/home/st/.config/code-vm/config.yaml"); got != "/home/st/.config/code-vm/secrets.yaml" { + t.Errorf("SecretsPathFor = %q", got) + } +} + +func TestLoadSecrets(t *testing.T) { + dir := t.TempDir() + p := filepath.Join(dir, "secrets.yaml") + content := "secrets:\n a:\n command: gopass show -o x\n b:\n value: literal\n" + if err := os.WriteFile(p, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + sources, warnings, err := LoadSecrets(p) + if err != nil { + t.Fatalf("LoadSecrets: %v", err) + } + if len(warnings) != 0 { + t.Errorf("warnings = %v, want none for 0600", warnings) + } + if sources["a"].Command != "gopass show -o x" || sources["b"].Value != "literal" { + t.Errorf("sources = %+v", sources) + } +} + +func TestLoadSecretsMissingFileIsEmpty(t *testing.T) { + sources, warnings, err := LoadSecrets(filepath.Join(t.TempDir(), "secrets.yaml")) + if err != nil || len(sources) != 0 || len(warnings) != 0 { + t.Errorf("missing file must load empty: %v %v %v", sources, warnings, err) + } +} + +func TestLoadSecretsRejectsBadEntries(t *testing.T) { + tests := []struct{ name, content, wantErr string }{ + {"both command and value", "secrets:\n a:\n command: c\n value: v\n", "exactly one"}, + {"neither", "secrets:\n a: {}\n", "exactly one"}, + {"bad name", "secrets:\n 'has space':\n value: v\n", "secret name"}, + {"unknown key", "secrets:\n a:\n comand: typo\n", "not found"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + p := filepath.Join(t.TempDir(), "secrets.yaml") + if err := os.WriteFile(p, []byte(tt.content), 0o600); err != nil { + t.Fatal(err) + } + _, _, err := LoadSecrets(p) + if err == nil || !strings.Contains(err.Error(), tt.wantErr) { + t.Errorf("LoadSecrets = %v, want %q", err, tt.wantErr) + } + }) + } +} + +func TestLoadSecretsWarnsOnLoosePermissions(t *testing.T) { + p := filepath.Join(t.TempDir(), "secrets.yaml") + if err := os.WriteFile(p, []byte("secrets:\n a:\n value: v\n"), 0o644); err != nil { + t.Fatal(err) + } + _, warnings, err := LoadSecrets(p) + if err != nil || len(warnings) != 1 || !strings.Contains(warnings[0], "0600") { + t.Errorf("want a permissions warning recommending 0600, got %v %v", warnings, err) + } +} diff --git a/internal/guest/embed_test.go b/internal/guest/embed_test.go index bbc6d4d..b8ecbec 100644 --- a/internal/guest/embed_test.go +++ b/internal/guest/embed_test.go @@ -70,3 +70,19 @@ func TestApplyProfilesScriptIsDelivered(t *testing.T) { } t.Error("apply-profiles.sh is not delivered to the guest") } + +func TestInstallUserFileScriptIsDelivered(t *testing.T) { + files, err := DataFiles() + if err != nil { + t.Fatalf("DataFiles: %v", err) + } + for _, f := range files { + if f.Path == "/usr/local/lib/sandbox/install-user-file.sh" { + if f.Permissions != "0755" { + t.Errorf("install-user-file.sh permissions = %s, want 0755", f.Permissions) + } + return + } + } + t.Error("install-user-file.sh is not delivered to the guest") +} diff --git a/internal/guest/files/scripts/install-user-file.sh b/internal/guest/files/scripts/install-user-file.sh new file mode 100644 index 0000000..e46c3b1 --- /dev/null +++ b/internal/guest/files/scripts/install-user-file.sh @@ -0,0 +1,60 @@ +#!/bin/bash +############################################################################### +# install-user-file.sh — place one host-staged file into the agent home +# +# Invoked as root by code-vm: install-user-file.sh +# +# The staged source sits in limaadmin's 0700 staging dir, unreadable by the +# agent; and a root write into the agent-owned home is the TOCTOU class the +# profile applier already closed. So root only RELAYS: the file moves to a +# root:AGENT_GID 0640 drop on tmpfs, and the final install runs with agent +# privileges — a planted symlink can only redirect a write the agent could +# already make. The drop is removed afterwards; rendered secrets exist there +# only for the moment between relay and install. +############################################################################### +set -euo pipefail + +# shellcheck source=/dev/null +. /etc/sandbox/provision.env + +src="$1" +rel="$2" +mode="$3" + +# Defense in depth: the host already validates rel before it ever reaches +# here, but this relay is root, so a path that escapes the agent home is +# rejected again on this end too. +case "$rel" in + /* | */../* | */.. | ../* | ..) + echo "install-user-file.sh: rejected unsafe rel path: $rel" >&2 + exit 1 + ;; +esac + +AGENT_HOME="/home/${AGENT_USER}" + +DROP_DIR=/run/sandbox/user-files +install -d -m 0750 -o root -g "$AGENT_GID" "$DROP_DIR" +drop=$(mktemp "$DROP_DIR/file-XXXXXXXX") + +# Trapped immediately after mktemp creates the placeholder: if the install +# below fails, cleanup still removes it instead of leaving an empty drop on +# tmpfs. +cleanup() { rm -f "$drop"; } +trap cleanup EXIT + +install -m 0640 -o root -g "$AGENT_GID" "$src" "$drop" +rm -f "$src" + +# Same hardened pattern as the profile applier's agent runner: no login +# shell, system PATH only, BASH_ENV/ENV cleared. Positional args, not string +# interpolation. +# shellcheck disable=SC2016 # the inner bash -c program expands its own args +setpriv --reuid "$AGENT_UID" --regid "$AGENT_GID" --init-groups \ + env -u BASH_ENV -u ENV \ + HOME="$AGENT_HOME" \ + USER="$AGENT_USER" \ + XDG_RUNTIME_DIR="/run/user/${AGENT_UID}" \ + PATH=/usr/local/bin:/usr/bin:/bin \ + bash -c 'dst="$1/$2"; mkdir -p "$(dirname "$dst")" && rm -f "$dst" && install -m "$3" "$4" "$dst"' \ + _ "$AGENT_HOME" "$rel" "$mode" "$drop" diff --git a/internal/profile/profile.go b/internal/profile/profile.go index 3d0dd10..ffb03b8 100644 --- a/internal/profile/profile.go +++ b/internal/profile/profile.go @@ -17,6 +17,7 @@ import ( "regexp" "sort" "strings" + "unicode" "gopkg.in/yaml.v3" @@ -38,7 +39,7 @@ var shellRe = regexp.MustCompile(`^/[a-zA-Z0-9._/-]+$`) // relPathRe matches the file paths a profile may ship. Deliberately // conservative: paths are written line-by-line into files.list, which the // guest applier reads back, so whitespace and metacharacters are rejected -// wholesale. ".." is excluded by the per-segment check in loadFiles. +// wholesale. ".." is excluded by the per-segment check in loadTree. var relPathRe = regexp.MustCompile(`^[a-zA-Z0-9._/-]+$`) // hookRe matches the manifest's hook entry: a plain file name inside the @@ -57,6 +58,35 @@ func isBlank(content []byte) bool { return len(bytes.Trim(content, "\n")) == 0 } +// isSingleLinePrintable reports whether s contains no control or invisible- +// formatting characters: no newline, tab, ESC, or other C0 code; no DEL +// (0x7f); no C1 control (0x80-0x9f); nothing in Unicode's Cf (format) +// category, which covers bidi overrides/isolates (U+202A-202E, U+2066-2069 — +// the Trojan Source class, CVE-2021-42574) and zero-width characters +// (U+200B ZWSP, U+200E/U+200F, U+061C, …); and nothing in Zl/Zp (U+2028 LINE +// SEPARATOR, U+2029 PARAGRAPH SEPARATOR) — line-breaking characters outside +// the Cc/Cf categories that would otherwise visually split a supposedly +// single-line string. Ordinary printable text — letters, digits, spaces, +// punctuation, and legitimate non-ASCII letters like accented characters — +// is unaffected: only Cc/C1/Cf/Zl/Zp are rejected, not all non-ASCII. +// +// Applied to manifest strings that are printed verbatim by `code-vm secrets` +// and `profile list`, and — most sensitively — copied byte-for-byte into the +// ready-to-paste MissingSecretSnippet a user pastes into secrets.yaml. +// Without this check, a hostile bundle could hide a shell command behind a +// terminal escape sequence or a bidi override (displays clean, copies with +// hidden or reordered content) or inject a newline (or U+2028/U+2029) to +// forge extra YAML entries in the pasted snippet. +func isSingleLinePrintable(s string) bool { + for _, r := range s { + if r < 0x20 || r == 0x7f || (r >= 0x80 && r <= 0x9f) || + unicode.Is(unicode.Cf, r) || unicode.Is(unicode.Zl, r) || unicode.Is(unicode.Zp, r) { + return false + } + } + return true +} + // forbiddenFiles are agent-home paths a profile may never ship: the // security-critical files lock-settings.sh owns and locks. var forbiddenFiles = map[string]bool{ @@ -64,14 +94,27 @@ var forbiddenFiles = map[string]bool{ ".claude/settings.local.json": true, } +// SecretSpec declares a secret the profile requires. +type SecretSpec struct { + Description string `yaml:"description"` + Suggest string `yaml:"suggest"` +} + +// VarSpec declares a variable the profile requires. +type VarSpec struct { + Description string `yaml:"description"` +} + // Manifest is the profile.yaml schema. Every key is optional, but a profile // that declares nothing at all is rejected. type Manifest struct { - Description string `yaml:"description"` - Packages []string `yaml:"packages"` - Shell string `yaml:"shell"` - Domains []string `yaml:"domains"` - Hook string `yaml:"hook"` + Description string `yaml:"description"` + Packages []string `yaml:"packages"` + Shell string `yaml:"shell"` + Domains []string `yaml:"domains"` + Hook string `yaml:"hook"` + Secrets map[string]SecretSpec `yaml:"secrets"` + Vars map[string]VarSpec `yaml:"vars"` } // File is one file a profile ships into the agent home. @@ -83,11 +126,12 @@ type File struct { // Profile is a loaded, validated bundle. type Profile struct { - Name string - Dir string - Manifest Manifest - Files []File // sorted by Rel - Hook []byte // nil when the manifest declares no hook + Name string + Dir string + Manifest Manifest + Files []File // sorted by Rel + Templates []File // sorted by Rel + Hook []byte // nil when the manifest declares no hook } // ValidateName reports whether name is usable as a profile name. Exported @@ -145,6 +189,35 @@ func Load(profilesDir, name string) (Profile, error) { if p.Files, err = loadFiles(dir); err != nil { return Profile{}, fmt.Errorf("profile %s: %w", name, err) } + if p.Templates, err = loadTemplates(dir); err != nil { + return Profile{}, fmt.Errorf("profile %s: %w", name, err) + } + // Validate that templates only reference declared secrets and vars. + for _, tpl := range p.Templates { + for _, ref := range FindRefs(tpl.Content) { + declared := false + if ref.Kind == "secret" { + _, declared = m.Secrets[ref.Name] + } else { + _, declared = m.Vars[ref.Name] + } + if !declared { + return Profile{}, fmt.Errorf( + "profile %s: templates/%s references ${%s:%s}, which the manifest does not declare", + name, tpl.Rel, ref.Kind, ref.Name) + } + } + } + // Check for collisions between files/ and templates/. + fileRels := map[string]bool{} + for _, f := range p.Files { + fileRels[f.Rel] = true + } + for _, tpl := range p.Templates { + if fileRels[tpl.Rel] { + return Profile{}, fmt.Errorf("profile %s: %s is shipped by both files/ and templates/; pick one", name, tpl.Rel) + } + } if m.Hook != "" { hookPath := filepath.Join(dir, m.Hook) // Lstat, not Stat: a hostile bundle could symlink its hook at a file @@ -167,16 +240,29 @@ func Load(profilesDir, name string) (Profile, error) { } p.Hook = b } - if len(p.Files) == 0 && len(m.Packages) == 0 && m.Shell == "" && len(m.Domains) == 0 && m.Hook == "" { - return Profile{}, fmt.Errorf("profile %s: declares nothing: no files, packages, shell, domains or hook", name) + if len(p.Files) == 0 && len(m.Packages) == 0 && m.Shell == "" && len(m.Domains) == 0 && m.Hook == "" && len(p.Templates) == 0 && len(m.Secrets) == 0 && len(m.Vars) == 0 { + return Profile{}, fmt.Errorf("profile %s: declares nothing: no files, packages, shell, domains, hook, templates, secrets or vars", name) } return p, nil } +// shippedAs records which profile shipped a given Rel and how (as a file or +// as a template), for LoadAll's cross-profile collision check. +type shippedAs struct { + profile string + kind string // "files/" or "templates/" +} + // LoadAll loads the named profiles in order. Order is meaningful and -// preserved: later profiles win file collisions, and hooks run in this order. +// preserved: later profiles win same-kind file collisions (file/file, +// template/template), and hooks run in this order. A files/-vs-templates/ +// collision across profiles is rejected outright rather than resolved by +// order: boot (template wins) and apply (files/ wins) run the two delivery +// mechanisms in different orders, so which one would actually win differs +// between the two paths for the same config — see the design spec. func LoadAll(profilesDir string, names []string) ([]Profile, error) { seen := map[string]bool{} + shipped := map[string]shippedAs{} // Rel -> who shipped it, and how out := make([]Profile, 0, len(names)) for _, n := range names { if seen[n] { @@ -187,12 +273,35 @@ func LoadAll(profilesDir string, names []string) ([]Profile, error) { if err != nil { return nil, err } + for _, f := range p.Files { + if prior, ok := shipped[f.Rel]; ok && prior.kind != "files/" { + return nil, fmt.Errorf( + "%s: profile %s ships files/%s but profile %s already ships templates/%s; "+ + "a files/-vs-templates/ collision across profiles cannot be resolved consistently "+ + "between boot and apply, so it is rejected outright", + f.Rel, n, f.Rel, prior.profile, f.Rel) + } + shipped[f.Rel] = shippedAs{profile: n, kind: "files/"} + } + for _, t := range p.Templates { + if prior, ok := shipped[t.Rel]; ok && prior.kind != "templates/" { + return nil, fmt.Errorf( + "%s: profile %s ships templates/%s but profile %s already ships files/%s; "+ + "a files/-vs-templates/ collision across profiles cannot be resolved consistently "+ + "between boot and apply, so it is rejected outright", + t.Rel, n, t.Rel, prior.profile, t.Rel) + } + shipped[t.Rel] = shippedAs{profile: n, kind: "templates/"} + } out = append(out, p) } return out, nil } func validateManifest(m Manifest) error { + if !isSingleLinePrintable(m.Description) { + return fmt.Errorf("description must be a single-line printable string (no control characters)") + } for i, pkg := range m.Packages { if !packageRe.MatchString(pkg) { return fmt.Errorf("packages[%d]: not a Debian package name: %q", i, pkg) @@ -209,15 +318,34 @@ func validateManifest(m Manifest) error { if m.Hook != "" && !hookRe.MatchString(m.Hook) { return fmt.Errorf("hook must be a plain file name inside the profile, got %q", m.Hook) } + for name, spec := range m.Secrets { + if err := ValidateName(name); err != nil { + return fmt.Errorf("secret name %q: must look like %q", name, "repo-user") + } + if !isSingleLinePrintable(spec.Description) { + return fmt.Errorf("secret %q: description must be a single-line printable string (no control characters)", name) + } + if !isSingleLinePrintable(spec.Suggest) { + return fmt.Errorf("secret %q: suggest must be a single-line printable string (no control characters)", name) + } + } + for name, spec := range m.Vars { + if err := ValidateName(name); err != nil { + return fmt.Errorf("var name %q: must look like %q", name, "artifactory-url") + } + if !isSingleLinePrintable(spec.Description) { + return fmt.Errorf("var %q: description must be a single-line printable string (no control characters)", name) + } + } return nil } -// loadFiles reads the files/ tree. Only regular files are accepted: a symlink +// loadTree reads a tree from dir/subdir. Only regular files are accepted: a symlink // could escape the tree on the host, or change content between validation and -// delivery. -func loadFiles(dir string) ([]File, error) { - root := filepath.Join(dir, "files") - // Lstat, not Stat: a symlinked files/ root would let WalkDir walk +// delivery. subdir is used as the prefix in error messages and the subdirectory name. +func loadTree(dir, subdir string) ([]File, error) { + root := filepath.Join(dir, subdir) + // Lstat, not Stat: a symlinked root would let WalkDir walk // wherever it points, including a directory an agent controls. info, err := os.Lstat(root) if errors.Is(err, os.ErrNotExist) { @@ -227,13 +355,13 @@ func loadFiles(dir string) ([]File, error) { return nil, err } if !info.Mode().IsDir() { - // Folds in the former "files is a regular file" case: a non-directory - // "files" makes WalkDir yield a single entry with rel ".", which + // Folds in the former "subdir is a regular file" case: a non-directory + // subdir makes WalkDir yield a single entry with rel ".", which // passes every check below (it is a regular file, it matches // relPathRe, it is not forbidden) and would otherwise be silently - // installed as-is. A symlinked "files" (to a directory or otherwise) + // installed as-is. A symlinked subdir (to a directory or otherwise) // is rejected the same way, by the same message. - return nil, fmt.Errorf("files must be a real directory (symlinks are rejected)") + return nil, fmt.Errorf("%s must be a real directory (symlinks are rejected)", subdir) } var out []File err = filepath.WalkDir(root, func(p string, d fs.DirEntry, err error) error { @@ -249,7 +377,7 @@ func loadFiles(dir string) ([]File, error) { } rel = filepath.ToSlash(rel) if !d.Type().IsRegular() { - return fmt.Errorf("files/%s: only regular files may be shipped (symlinks are rejected)", rel) + return fmt.Errorf("%s/%s: only regular files may be shipped (symlinks are rejected)", subdir, rel) } if !relPathRe.MatchString(rel) { return fmt.Errorf("file path %q: only [a-zA-Z0-9._/-] is allowed", rel) @@ -260,14 +388,14 @@ func loadFiles(dir string) ([]File, error) { } } if forbiddenFiles[rel] { - return fmt.Errorf("files/%s: profiles may not ship the locked Claude settings", rel) + return fmt.Errorf("%s/%s: profiles may not ship the locked Claude settings", subdir, rel) } b, err := os.ReadFile(p) if err != nil { return err } if isBlank(b) { - return fmt.Errorf("files/%s: must not be blank (empty or newline-only content cannot be embedded)", rel) + return fmt.Errorf("%s/%s: must not be blank (empty or newline-only content cannot be embedded)", subdir, rel) } info, err := d.Info() if err != nil { @@ -282,3 +410,17 @@ func loadFiles(dir string) ([]File, error) { sort.Slice(out, func(i, j int) bool { return out[i].Rel < out[j].Rel }) return out, nil } + +// loadFiles reads the files/ tree. Only regular files are accepted: a symlink +// could escape the tree on the host, or change content between validation and +// delivery. +func loadFiles(dir string) ([]File, error) { + return loadTree(dir, "files") +} + +// loadTemplates reads the templates/ tree. Only regular files are accepted: a symlink +// could escape the tree on the host, or change content between validation and +// delivery. +func loadTemplates(dir string) ([]File, error) { + return loadTree(dir, "templates") +} diff --git a/internal/profile/profile_test.go b/internal/profile/profile_test.go index 12376dd..6b0107c 100644 --- a/internal/profile/profile_test.go +++ b/internal/profile/profile_test.go @@ -290,3 +290,200 @@ func TestLoadAllPreservesOrderAndRejectsDuplicates(t *testing.T) { t.Errorf("no names must load cleanly to an empty slice, got %v, %v", ps, err) } } + +// A files/ entry in one profile and a templates/ entry in another, at the +// same Rel, cannot be resolved consistently by list order: boot delivers +// templates last (template wins) while apply pushes rendered templates +// before ApplyProfiles re-lays the file tree (files/ wins). Same config, +// different winner depending on path, so LoadAll rejects it outright. +func TestLoadAllRejectsCrossProfileFileTemplateCollision(t *testing.T) { + dir := t.TempDir() + writeProfile(t, dir, "a", "description: a\n", map[string]string{"files/.npmrc": "from-a\n"}) + writeProfile(t, dir, "b", "secrets:\n tok: {}\n", map[string]string{"templates/.npmrc": "${secret:tok}\n"}) + + _, err := LoadAll(dir, []string{"a", "b"}) + if err == nil { + t.Fatal("expected a cross-profile files/-vs-templates/ collision error") + } + if !strings.Contains(err.Error(), "a") || !strings.Contains(err.Error(), "b") || !strings.Contains(err.Error(), ".npmrc") { + t.Errorf("error must name both profiles and the path, got %v", err) + } +} + +// Two profiles that both ship the same Rel as files/ is the ordinary, +// well-defined case: later wins, no error. +func TestLoadAllAllowsSameKindCrossProfileCollision(t *testing.T) { + dir := t.TempDir() + writeProfile(t, dir, "a", "description: a\n", map[string]string{"files/.npmrc": "from-a\n"}) + writeProfile(t, dir, "b", "description: b\n", map[string]string{"files/.npmrc": "from-b\n"}) + + ps, err := LoadAll(dir, []string{"a", "b"}) + if err != nil { + t.Fatalf("LoadAll: %v", err) + } + if len(ps) != 2 { + t.Fatalf("LoadAll returned %d profiles, want 2", len(ps)) + } +} + +func TestLoadTemplatesAndDeclarations(t *testing.T) { + dir := t.TempDir() + writeProfile(t, dir, "maven", ` +description: maven setup +secrets: + repo-user: + description: Artifactory user + suggest: gopass show -o wetf/artifactory-user + repo-password: {} +vars: + artifactory-url: + description: Base URL +`, map[string]string{ + "templates/.m2/settings.xml": "${secret:repo-user}/${var:artifactory-url}\n", + }) + p, err := Load(dir, "maven") + if err != nil { + t.Fatalf("Load: %v", err) + } + if p.Manifest.Secrets["repo-user"].Suggest != "gopass show -o wetf/artifactory-user" { + t.Errorf("Suggest not loaded: %+v", p.Manifest.Secrets) + } + if _, ok := p.Manifest.Secrets["repo-password"]; !ok { + t.Error("empty-spec secret not loaded") + } + if len(p.Templates) != 1 || p.Templates[0].Rel != ".m2/settings.xml" { + t.Fatalf("Templates = %+v", p.Templates) + } +} + +// A profile carrying only declarations and templates is a valid profile. +func TestLoadTemplatesOnlyProfileIsNotEmpty(t *testing.T) { + dir := t.TempDir() + writeProfile(t, dir, "p", "secrets:\n tok: {}\n", map[string]string{ + "templates/.npmrc": "//registry/:_authToken=${secret:tok}\n", + }) + if _, err := Load(dir, "p"); err != nil { + t.Fatalf("Load: %v", err) + } +} + +func TestLoadRejectsInvalidDeclarations(t *testing.T) { + tests := []struct { + name string + manifest string + files map[string]string + wantErr string + }{ + {"bad secret name", "secrets:\n 'has space': {}\n", nil, "secret name"}, + {"bad var name", "vars:\n 'has/slash': {}\n", nil, "var name"}, + {"ESC in secret suggest", "secrets:\n tok:\n suggest: \"gopass show \\efoo\"\n", nil, "suggest must be a single-line printable string"}, + {"newline in secret description", "secrets:\n tok:\n description: |\n line one\n line two\n", nil, "description must be a single-line printable string"}, + {"tab in var description", "vars:\n url:\n description: \"a\\tb\"\n", nil, "description must be a single-line printable string"}, + {"newline in top-level description", "description: |\n line one\n line two\n", nil, "description must be a single-line printable string"}, + // Trojan Source / CVE-2021-42574 class: bidi overrides and isolates + // reorder how surrounding text renders, so a suggest command can + // display one thing and paste another. + {"RLO (U+202E) in secret suggest", "secrets:\n tok:\n suggest: \"gopass show \\u202Efoo\"\n", nil, "suggest must be a single-line printable string"}, + {"LRI (U+2066) in secret suggest", "secrets:\n tok:\n suggest: \"gopass show \\u2066foo\"\n", nil, "suggest must be a single-line printable string"}, + {"ZWSP (U+200B) in var description", "vars:\n url:\n description: \"a\\u200Bb\"\n", nil, "description must be a single-line printable string"}, + {"NEL (U+0085, C1) in top-level description", "description: \"a\\u0085b\"\n", nil, "description must be a single-line printable string"}, + {"C1 CSI (U+009B) in secret description", "secrets:\n tok:\n description: \"a\\u009Bb\"\n", nil, "description must be a single-line printable string"}, + {"template/file collision", "description: x\n", + map[string]string{"files/.npmrc": "a\n", "templates/.npmrc": "b\n"}, + "both files/ and templates/"}, + {"template ships locked settings", "description: x\n", + map[string]string{"templates/.claude/settings.json": "{}\n"}, + "locked Claude settings"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + writeProfile(t, dir, "p", tt.manifest, tt.files) + _, err := Load(dir, "p") + if err == nil || !strings.Contains(err.Error(), tt.wantErr) { + t.Errorf("Load error = %v, want it to contain %q", err, tt.wantErr) + } + }) + } +} + +// Ordinary single-line text with spaces is exactly the common case and must +// not be rejected by the control-character check. +func TestLoadAcceptsOrdinaryDescriptionsAndSuggestions(t *testing.T) { + dir := t.TempDir() + writeProfile(t, dir, "p", ` +description: A profile with ordinary spaced-out words +secrets: + tok: + description: A token with spaces in its description + suggest: gopass show -o some/path with spaces +vars: + url: + description: A var description with spaces +`, nil) + if _, err := Load(dir, "p"); err != nil { + t.Fatalf("Load: %v", err) + } +} + +// Legitimate non-ASCII letters (accented characters and the like) must not +// be swept up by the Cc/C1/Cf rejection: only control and invisible- +// formatting characters are disallowed, not all non-ASCII text. +func TestLoadAcceptsAccentedDescription(t *testing.T) { + dir := t.TempDir() + writeProfile(t, dir, "p", "description: Café token\n", map[string]string{"files/a": "x\n"}) + if _, err := Load(dir, "p"); err != nil { + t.Fatalf("Load: %v", err) + } +} + +// TestSingleLinePrintable exercises isSingleLinePrintable directly, covering +// the Trojan Source / CVE-2021-42574 class (bidi overrides and isolates), +// zero-width formatting characters, C1 controls, and legitimate non-ASCII +// text, alongside the original C0/DEL cases. +func TestSingleLinePrintable(t *testing.T) { + reject := map[string]string{ + "newline": "a\nb", + "tab": "a\tb", + "ESC (C0)": "a\x1bb", + "DEL (0x7f)": "a\x7fb", + "C1 CSI (U+009B)": "a\u009bb", + "NEL (U+0085, C1)": "a\u0085b", + "RLO (U+202E)": "a\u202eb", + "LRI (U+2066)": "a\u2066b", + "ZWSP (U+200B)": "a\u200bb", + "LINE SEPARATOR (U+2028, Zl)": "a\u2028b", + "PARAGRAPH SEPARATOR (U+2029, Zp)": "a\u2029b", + } + for name, s := range reject { + t.Run("rejects "+name, func(t *testing.T) { + if isSingleLinePrintable(s) { + t.Errorf("isSingleLinePrintable(%q) = true, want false", s) + } + }) + } + accept := []string{ + "ordinary text with spaces", + "gopass show -o some/path with spaces", + "Caf\u00e9 token", + "\u65e5\u672c\u8a9e\u306e\u30c6\u30ad\u30b9\u30c8", + } + for _, s := range accept { + t.Run("accepts "+s, func(t *testing.T) { + if !isSingleLinePrintable(s) { + t.Errorf("isSingleLinePrintable(%q) = false, want true", s) + } + }) + } +} + +func TestLoadRejectsSymlinkedTemplatesRoot(t *testing.T) { + dir := t.TempDir() + writeProfile(t, dir, "p", "description: x\n", map[string]string{"files/a": "x\n"}) + if err := os.Symlink(t.TempDir(), filepath.Join(dir, "p", "templates")); err != nil { + t.Skip("symlinks unavailable") + } + if _, err := Load(dir, "p"); err == nil || !strings.Contains(err.Error(), "symlinks are rejected") { + t.Errorf("Load error = %v, want symlink rejection", err) + } +} diff --git a/internal/profile/template.go b/internal/profile/template.go new file mode 100644 index 0000000..939a019 --- /dev/null +++ b/internal/profile/template.go @@ -0,0 +1,236 @@ +package profile + +import ( + "context" + "fmt" + "regexp" + "sort" + "strings" + + "github.com/wetransform/code-vm/internal/config" +) + +// refRe matches exactly the two placeholder forms templates may use. The name +// charset mirrors ValidateName, so anything else — Maven properties, +// ${env.FOO} — is left untouched by both the scanner and the renderer. +var refRe = regexp.MustCompile(`\$\{(secret|var):([a-zA-Z0-9][a-zA-Z0-9-]{0,62})\}`) + +// Ref is one placeholder occurrence kind+name. +type Ref struct { + Kind string // "secret" or "var" + Name string +} + +// FindRefs returns the distinct placeholder references in content, in first- +// appearance order. +func FindRefs(content []byte) []Ref { + seen := map[Ref]bool{} + var out []Ref + for _, m := range refRe.FindAllSubmatch(content, -1) { + r := Ref{Kind: string(m[1]), Name: string(m[2])} + if !seen[r] { + seen[r] = true + out = append(out, r) + } + } + return out +} + +// Rendered is one template after substitution, destined for the agent home. +type Rendered struct { + Rel string + Content []byte +} + +// RenderTemplates substitutes secret and var values into every active +// profile's templates. Later profiles win Rel collisions, matching the files/ +// rule. Values are opaque bytes: no escaping layer, exactly as the spec +// states — a value that breaks the target format is the user's own. +func RenderTemplates(profiles []Profile, secrets, vars map[string]string) []Rendered { + byRel := map[string][]byte{} + for _, p := range profiles { + for _, tpl := range p.Templates { + byRel[tpl.Rel] = refRe.ReplaceAllFunc(tpl.Content, func(m []byte) []byte { + sub := refRe.FindSubmatch(m) + if string(sub[1]) == "secret" { + return []byte(secrets[string(sub[2])]) + } + return []byte(vars[string(sub[2])]) + }) + } + } + out := make([]Rendered, 0, len(byRel)) + for rel, content := range byRel { + out = append(out, Rendered{Rel: rel, Content: content}) + } + sort.Slice(out, func(i, j int) bool { return out[i].Rel < out[j].Rel }) + return out +} + +// DeclaredSecret is one secret name unioned across the active profiles. +type DeclaredSecret struct { + Name string + Profiles []string // declaring profiles, in activation order + Description string // first non-empty wins + Suggest string // first non-empty wins; inert display string +} + +// DeclaredVar is the var analog. +type DeclaredVar struct { + Name string + Profiles []string + Description string +} + +// DeclaredSecrets unions secret declarations across profiles, sorted by name. +func DeclaredSecrets(profiles []Profile) []DeclaredSecret { + byName := map[string]*DeclaredSecret{} + for _, p := range profiles { + names := make([]string, 0, len(p.Manifest.Secrets)) + for n := range p.Manifest.Secrets { + names = append(names, n) + } + sort.Strings(names) // map order is random; keep Profiles deterministic + for _, n := range names { + spec := p.Manifest.Secrets[n] + d, ok := byName[n] + if !ok { + d = &DeclaredSecret{Name: n} + byName[n] = d + } + d.Profiles = append(d.Profiles, p.Name) + if d.Description == "" { + d.Description = spec.Description + } + if d.Suggest == "" { + d.Suggest = spec.Suggest + } + } + } + out := make([]DeclaredSecret, 0, len(byName)) + for _, d := range byName { + out = append(out, *d) + } + sort.Slice(out, func(i, j int) bool { return out[i].Name < out[j].Name }) + return out +} + +// DeclaredVars unions var declarations across profiles, sorted by name. +func DeclaredVars(profiles []Profile) []DeclaredVar { + byName := map[string]*DeclaredVar{} + for _, p := range profiles { + names := make([]string, 0, len(p.Manifest.Vars)) + for n := range p.Manifest.Vars { + names = append(names, n) + } + sort.Strings(names) + for _, n := range names { + spec := p.Manifest.Vars[n] + d, ok := byName[n] + if !ok { + d = &DeclaredVar{Name: n} + byName[n] = d + } + d.Profiles = append(d.Profiles, p.Name) + if d.Description == "" { + d.Description = spec.Description + } + } + } + out := make([]DeclaredVar, 0, len(byName)) + for _, d := range byName { + out = append(out, *d) + } + sort.Slice(out, func(i, j int) bool { return out[i].Name < out[j].Name }) + return out +} + +// CommandRunner executes a user-authored secrets.yaml command on the host. +// On success the returned bytes are stdout — the value — with stderr chatter +// excluded; on failure the returned bytes are stderr, as display text for the +// error. Injectable for tests. +type CommandRunner func(ctx context.Context, command string) ([]byte, error) + +// yamlDoubleQuote renders s as a YAML double-quoted scalar, so the value that +// lands in secrets.yaml when a user pastes the snippet is byte-for-byte s — +// regardless of characters (#, leading -, ": ", …) that would otherwise be +// reinterpreted as YAML syntax rather than literal command text. Callers only +// ever pass isSingleLinePrintable strings (no control characters, no +// newlines), so the only two characters a double-quoted YAML scalar requires +// escaped are backslash and the double quote itself. +func yamlDoubleQuote(s string) string { + var b strings.Builder + b.WriteByte('"') + for _, r := range s { + if r == '\\' || r == '"' { + b.WriteByte('\\') + } + b.WriteRune(r) + } + b.WriteByte('"') + return b.String() +} + +// MissingSecretSnippet renders the ready-to-paste secrets.yaml block for an +// unmapped secret. The suggest hint is copied verbatim as the command — +// display only until the user adopts it by saving this snippet themselves. +// The command is emitted as a quoted YAML scalar: an unquoted suggest like +// `printf '#token'` would otherwise have its trailing `'#token'` reparsed as +// a YAML comment the moment the snippet is pasted into secrets.yaml. +func MissingSecretSnippet(d DeclaredSecret) string { + cmd := d.Suggest + if cmd == "" { + cmd = "" + } + return fmt.Sprintf("secrets:\n %s:\n command: %s\n", d.Name, yamlDoubleQuote(cmd)) +} + +// ResolveSecrets resolves every declared secret from the user's sources. Each +// command runs exactly once per resolve pass with one trailing newline +// stripped (the gopass/pass convention). A missing mapping fails with an +// actionable snippet rather than prompting or falling back to hints: hints +// never execute. +func ResolveSecrets(ctx context.Context, declared []DeclaredSecret, sources map[string]config.SecretSource, run CommandRunner) (map[string]string, error) { + out := make(map[string]string, len(declared)) + for _, d := range declared { + src, ok := sources[d.Name] + if !ok { + desc := d.Description + if desc == "" { + desc = "no description" + } + return nil, fmt.Errorf( + "profile %s needs secret %q (%s), but secrets.yaml does not map it.\nAdd to ~/.config/code-vm/secrets.yaml:\n\n%s", + strings.Join(d.Profiles, ", "), d.Name, desc, MissingSecretSnippet(d)) + } + if src.Command != "" { + b, err := run(ctx, src.Command) + if err != nil { + return nil, fmt.Errorf("secret %q: command failed: %w: %s", d.Name, err, strings.TrimSpace(string(b))) + } + out[d.Name] = strings.TrimSuffix(string(b), "\n") + continue + } + out[d.Name] = src.Value + } + return out, nil +} + +// ResolveVars resolves declared vars from config.yaml's literal map. +func ResolveVars(declared []DeclaredVar, values map[string]string) (map[string]string, error) { + out := make(map[string]string, len(declared)) + for _, d := range declared { + v, ok := values[d.Name] + if !ok { + desc := d.Description + if desc == "" { + desc = "no description" + } + return nil, fmt.Errorf( + "profile %s needs var %q (%s), but config.yaml does not set it.\nAdd to config.yaml:\n\nvars:\n %s: ", + strings.Join(d.Profiles, ", "), d.Name, desc, d.Name) + } + out[d.Name] = v + } + return out, nil +} diff --git a/internal/profile/template_test.go b/internal/profile/template_test.go new file mode 100644 index 0000000..24e5728 --- /dev/null +++ b/internal/profile/template_test.go @@ -0,0 +1,175 @@ +package profile + +import ( + "context" + "errors" + "reflect" + "strings" + "testing" + + "gopkg.in/yaml.v3" + + "github.com/wetransform/code-vm/internal/config" +) + +func TestFindRefs(t *testing.T) { + content := []byte(`user=${secret:repo-user} url=${var:base-url} +again=${secret:repo-user} passthrough=${env.FOO} ${prop} $secret:no ${secret:BAD NAME}`) + got := FindRefs(content) + want := []Ref{{Kind: "secret", Name: "repo-user"}, {Kind: "var", Name: "base-url"}} + if !reflect.DeepEqual(got, want) { + t.Errorf("FindRefs = %v, want %v", got, want) + } +} + +func TestRenderTemplatesSubstitutesAndPassesThrough(t *testing.T) { + profiles := []Profile{{ + Name: "a", + Templates: []File{{Rel: ".m2/settings.xml", Content: []byte( + "${secret:repo-user}${var:base-url}${env.HOME}")}}, + }} + out := RenderTemplates(profiles, + map[string]string{"repo-user": "simon"}, + map[string]string{"base-url": "https://x.example"}) + if len(out) != 1 { + t.Fatalf("Rendered = %+v", out) + } + want := "simonhttps://x.example${env.HOME}" + if string(out[0].Content) != want { + t.Errorf("Content = %q, want %q", out[0].Content, want) + } +} + +func TestRenderTemplatesLaterProfileWins(t *testing.T) { + profiles := []Profile{ + {Name: "a", Templates: []File{{Rel: ".npmrc", Content: []byte("from-a")}}}, + {Name: "b", Templates: []File{{Rel: ".npmrc", Content: []byte("from-b")}}}, + } + out := RenderTemplates(profiles, nil, nil) + if len(out) != 1 || string(out[0].Content) != "from-b" { + t.Errorf("collision must resolve to the later profile, got %+v", out) + } +} + +func TestDeclaredSecretsMergesAcrossProfiles(t *testing.T) { + profiles := []Profile{ + {Name: "a", Manifest: Manifest{Secrets: map[string]SecretSpec{ + "tok": {Description: "token", Suggest: "gopass show -o t"}}}}, + {Name: "b", Manifest: Manifest{Secrets: map[string]SecretSpec{"tok": {}}}}, + } + got := DeclaredSecrets(profiles) + if len(got) != 1 || got[0].Name != "tok" || got[0].Suggest != "gopass show -o t" || + !reflect.DeepEqual(got[0].Profiles, []string{"a", "b"}) { + t.Errorf("DeclaredSecrets = %+v", got) + } +} + +func TestResolveSecrets(t *testing.T) { + declared := []DeclaredSecret{ + {Name: "from-cmd", Profiles: []string{"p"}}, + {Name: "from-val", Profiles: []string{"p"}}, + } + sources := map[string]config.SecretSource{ + "from-cmd": {Command: "get-it"}, + "from-val": {Value: "literal"}, + } + calls := 0 + run := func(_ context.Context, command string) ([]byte, error) { + calls++ + if command != "get-it" { + t.Errorf("command = %q", command) + } + return []byte("resolved\n"), nil + } + got, err := ResolveSecrets(context.Background(), declared, sources, run) + if err != nil { + t.Fatalf("ResolveSecrets: %v", err) + } + // Exactly one trailing newline stripped; command runs once per secret. + if got["from-cmd"] != "resolved" || got["from-val"] != "literal" || calls != 1 { + t.Errorf("got %v, calls=%d", got, calls) + } +} + +func TestResolveSecretsMissingMappingHasSnippet(t *testing.T) { + declared := []DeclaredSecret{{ + Name: "repo-user", Profiles: []string{"maven"}, + Description: "Artifactory user", Suggest: "gopass show -o wetf/user", + }} + _, err := ResolveSecrets(context.Background(), declared, nil, nil) + if err == nil { + t.Fatal("expected an error for an unmapped secret") + } + for _, want := range []string{"repo-user", "maven", "Artifactory user", + "secrets:", `command: "gopass show -o wetf/user"`} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error missing %q:\n%s", want, err) + } + } +} + +func TestResolveSecretsCommandFailure(t *testing.T) { + declared := []DeclaredSecret{{Name: "tok", Profiles: []string{"p"}}} + sources := map[string]config.SecretSource{"tok": {Command: "boom"}} + run := func(context.Context, string) ([]byte, error) { + return []byte("stderr text"), errors.New("exit status 1") + } + _, err := ResolveSecrets(context.Background(), declared, sources, run) + if err == nil || !strings.Contains(err.Error(), "tok") || !strings.Contains(err.Error(), "exit status 1") { + t.Errorf("ResolveSecrets error = %v", err) + } +} + +func TestResolveVars(t *testing.T) { + declared := []DeclaredVar{{Name: "url", Profiles: []string{"p"}, Description: "Base URL"}} + got, err := ResolveVars(declared, map[string]string{"url": "https://x"}) + if err != nil || got["url"] != "https://x" { + t.Errorf("ResolveVars = %v, %v", got, err) + } + _, err = ResolveVars(declared, nil) + if err == nil || !strings.Contains(err.Error(), "vars:") || !strings.Contains(err.Error(), "url") { + t.Errorf("missing var must produce a config.yaml snippet, got %v", err) + } +} + +// MissingSecretSnippet's command must be a YAML double-quoted scalar so a +// suggest containing a YAML-significant character round-trips byte-for-byte +// when the snippet is pasted into secrets.yaml and parsed back. An unquoted +// `printf '#token'` would otherwise have its trailing `'#token'` reparsed as +// a comment, and one containing ": " would be split into extra mapping keys. +func TestMissingSecretSnippetQuotesSuggestForYAMLRoundTrip(t *testing.T) { + for _, suggest := range []string{ + `printf '#token'`, + `echo foo: bar`, + `echo "quoted"`, + `echo \backslash`, + } { + t.Run(suggest, func(t *testing.T) { + snippet := MissingSecretSnippet(DeclaredSecret{Name: "tok", Suggest: suggest}) + var doc struct { + Secrets map[string]struct { + Command string `yaml:"command"` + } `yaml:"secrets"` + } + if err := yaml.Unmarshal([]byte(snippet), &doc); err != nil { + t.Fatalf("snippet did not parse as YAML: %v\nsnippet:\n%s", err, snippet) + } + got := doc.Secrets["tok"].Command + if got != suggest { + t.Errorf("round-tripped command = %q, want %q\nsnippet:\n%s", got, suggest, snippet) + } + }) + } +} + +// Load must reject a template referencing an undeclared name (wired in this task). +func TestLoadRejectsUndeclaredPlaceholder(t *testing.T) { + dir := t.TempDir() + writeProfile(t, dir, "p", "secrets:\n known: {}\n", map[string]string{ + "templates/.npmrc": "a=${secret:known} b=${var:never-declared}\n", + }) + _, err := Load(dir, "p") + if err == nil || !strings.Contains(err.Error(), "never-declared") { + t.Errorf("Load = %v, want undeclared-placeholder rejection", err) + } +} diff --git a/internal/session/gitidentity.go b/internal/session/gitidentity.go index 05f0b1d..3b22c47 100644 --- a/internal/session/gitidentity.go +++ b/internal/session/gitidentity.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "os/exec" - "strconv" "strings" ) @@ -47,9 +46,8 @@ func ApplyGitIdentity(ctx context.Context, d Deps) error { return nil } - // Numeric ids, not names: the guest group carrying AgentGID may be a stock - // group with a different name (see Deps). - dst := "/home/" + d.AgentUser + "/.gitconfig" - return installContent(ctx, d, []byte(GitConfigContent(name, email)), dst, "0644", - strconv.Itoa(d.AgentUID), strconv.Itoa(d.AgentGID)) + // Delivered through the relay (install-user-file.sh), not a root install: + // the file lands in the agent's home, so the final write must run with + // agent privileges rather than root's (see userfiles.go). + return PushUserFile(ctx, d, []byte(GitConfigContent(name, email)), ".gitconfig", "0644") } diff --git a/internal/session/gitidentity_test.go b/internal/session/gitidentity_test.go index ee08c73..a49191a 100644 --- a/internal/session/gitidentity_test.go +++ b/internal/session/gitidentity_test.go @@ -25,10 +25,11 @@ func TestGitConfigContentOmitsMissingFields(t *testing.T) { } } -// Group ownership must be set by numeric GID. The guest group carrying the -// host's GID is often a stock group with a different name — a host user with -// GID 100 lands in "users" — so `install -g devuser` fails outright there. -func TestApplyGitIdentityInstallsByNumericIDs(t *testing.T) { +// The gitconfig is delivered through the agent-privilege relay, not a root +// install: ownership then falls out of which user runs the final install, +// not an explicit -o/-g pair, so there is no numeric-vs-named-group pitfall +// left here (see userfiles_test.go for the relay assertions). +func TestApplyGitIdentityDeliversToDotfile(t *testing.T) { r := &fakeRunner{} d := testDeps(t, r) d.AgentUID, d.AgentGID = 1000, 100 @@ -41,13 +42,8 @@ func TestApplyGitIdentityInstallsByNumericIDs(t *testing.T) { if err := ApplyGitIdentity(context.Background(), d); err != nil { t.Fatalf("ApplyGitIdentity: %v", err) } - if !r.ranAny("install -D -m 0644 -o 1000 -g 100") { - t.Errorf("gitconfig must be installed with numeric owner/group, got %v", r.calls) - } - for _, c := range r.calls { - if strings.Contains(strings.Join(c, " "), "-g devuser") { - t.Errorf("must not set the group by name: %v", c) - } + if !r.ranAny(".gitconfig") || !r.ranAny("0644") { + t.Errorf("gitconfig must be relayed with its home-relative path and mode, got %v", r.calls) } } diff --git a/internal/session/stage.go b/internal/session/stage.go index 0e22a9d..4ef97ec 100644 --- a/internal/session/stage.go +++ b/internal/session/stage.go @@ -30,38 +30,59 @@ func stagedPath() (string, error) { return stageDir + "/stage-" + hex.EncodeToString(b[:]), nil } -// installContent writes content into the guest at dst, owned by owner:group -// with the given mode. The content travels through the admin user's staging -// directory and the staged copy is removed afterwards, so it is never readable -// at a path the agent can reach. -func installContent(ctx context.Context, d Deps, content []byte, dst, mode, owner, group string) error { +// stageFile writes content to a local temp file and copies it into the +// guest's admin-only staging directory, returning the staged guest path. +// Shared by installContent (root destinations) and PushUserFile (agent-home +// destinations, which relay the staged copy onward instead of root-installing +// it directly — see userfiles.go). +func stageFile(ctx context.Context, d Deps, content []byte) (string, error) { tmp, err := os.CreateTemp("", "code-vm-stage-*") if err != nil { - return fmt.Errorf("create temp file: %w", err) + return "", fmt.Errorf("create temp file: %w", err) } defer os.Remove(tmp.Name()) if err := tmp.Chmod(0o600); err != nil { tmp.Close() - return fmt.Errorf("chmod temp file: %w", err) + return "", fmt.Errorf("chmod temp file: %w", err) } if _, err := tmp.Write(content); err != nil { tmp.Close() - return fmt.Errorf("write temp file: %w", err) + return "", fmt.Errorf("write temp file: %w", err) } if err := tmp.Close(); err != nil { - return fmt.Errorf("close temp file: %w", err) + return "", fmt.Errorf("close temp file: %w", err) } staged, err := stagedPath() if err != nil { - return err + return "", err } if err := d.Client.Admin(ctx, []string{ "install", "-d", "-m", "0700", "-o", adminUser, "-g", adminUser, stageDir, }); err != nil { - return err + return "", err } if err := d.Client.Copy(ctx, tmp.Name(), staged); err != nil { + // Copy can leave a partial file at staged on failure or cancellation. + // Every other stageFile caller only learns of the guest path once + // Copy succeeds, so this is the only chance to clean it up — best + // effort, on an independent context, the same as PushUserFile's + // deferred cleanup (see cleanupStaged in userfiles.go). + cleanupStaged(d, staged) + return "", fmt.Errorf("copy staged file: %w", err) + } + return staged, nil +} + +// installContent writes content into the guest at dst, owned by owner:group +// with the given mode, as root. Only correct for root-owned destinations — +// the allowlist fragment and profile tree — where there is no agent-owned +// home path for a planted symlink to redirect the write into; for anything +// landing in the agent's home, use PushUserFile instead so root never writes +// there directly. +func installContent(ctx context.Context, d Deps, content []byte, dst, mode, owner, group string) error { + staged, err := stageFile(ctx, d, content) + if err != nil { return err } // -D creates root-owned parents for nested per-profile paths; a no-op for diff --git a/internal/session/stage_test.go b/internal/session/stage_test.go index d2ee5d4..ae7cfe5 100644 --- a/internal/session/stage_test.go +++ b/internal/session/stage_test.go @@ -2,6 +2,7 @@ package session import ( "context" + "errors" "strings" "testing" ) @@ -50,6 +51,55 @@ func TestInstallContentStagesOutsideAgentReach(t *testing.T) { } } +// copyFailingRunner fails every `limactl copy` invocation while otherwise +// behaving like fakeRunner, so tests can exercise stageFile's cleanup path +// without a real guest. +type copyFailingRunner struct { + fakeRunner +} + +func (r *copyFailingRunner) Run(ctx context.Context, args ...string) error { + if err := r.fakeRunner.Run(ctx, args...); err != nil { + return err + } + if len(args) > 0 && args[0] == "copy" { + return errors.New("simulated copy failure") + } + return nil +} + +// A Copy failure can leave a partial file behind at the staged guest path. +// stageFile must attempt a best-effort removal of it before returning the +// error, rather than leaving rendered credential bytes sitting in the +// admin-only staging dir — the same posture PushUserFile's deferred cleanup +// takes on its own failure paths. +func TestStageFileCleansUpPartialFileOnCopyFailure(t *testing.T) { + r := ©FailingRunner{} + d := testDeps(t, r) + _, err := stageFile(context.Background(), d, []byte("secret\n")) + if err == nil { + t.Fatal("expected stageFile to return the Copy error") + } + if !strings.Contains(err.Error(), "simulated copy failure") { + t.Errorf("stageFile error = %v, want it to wrap the Copy failure", err) + } + + var stagedGuestPath string + for _, call := range r.calls { + if len(call) >= 2 && call[0] == "copy" { + // copy : + dst := call[2] + stagedGuestPath = strings.TrimPrefix(dst, "code-sandbox:") + } + } + if stagedGuestPath == "" { + t.Fatal("expected a copy call attempting to stage the file") + } + if !r.ranAny("rm -f " + stagedGuestPath) { + t.Errorf("expected a best-effort cleanup of the partial staged file %q, calls=%v", stagedGuestPath, r.calls) + } +} + func TestStagedNamesAreUnpredictable(t *testing.T) { seen := map[string]bool{} for i := 0; i < 20; i++ { diff --git a/internal/session/userfiles.go b/internal/session/userfiles.go new file mode 100644 index 0000000..6d0fbb4 --- /dev/null +++ b/internal/session/userfiles.go @@ -0,0 +1,83 @@ +package session + +import ( + "context" + "fmt" + "path" + "regexp" + "strings" + "time" +) + +// stagedCleanupTimeout bounds the best-effort removal of a staged file so it +// cannot hang the process: it runs even when the caller's own ctx is +// cancelled or already past its deadline (see cleanupStaged). +const stagedCleanupTimeout = 10 * time.Second + +// relCharsetRe mirrors the charset profile.relPathRe enforces on the host +// side: conservative on purpose, since rel is written into a root-run +// install command on the other end of the relay. Defense in depth — every +// current caller already passes a host-validated rel — for future ones. +var relCharsetRe = regexp.MustCompile(`^[a-zA-Z0-9._/-]+$`) + +// PushUserFile delivers content into the agent's home at rel with the given +// mode, without ever writing there as root. The staged copy is relayed by +// install-user-file.sh: root moves it to an agent-group-readable tmpfs drop, +// and an agent-identity install places it — a symlink the agent plants can +// only redirect a write the agent could already make (the same posture as +// profile file installs). Used for rendered templates (0600) and the git +// identity (0644); rel comes from host-validated input only. +func PushUserFile(ctx context.Context, d Deps, content []byte, rel, mode string) error { + // Defense in depth: every current caller passes a host-validated rel, but + // this guards future ones from ever reaching the guest with a path that + // escapes the agent home or carries a shell metacharacter into the + // relay's positional args. + if rel == "" || path.IsAbs(rel) || hasDotDotSegment(rel) || !relCharsetRe.MatchString(rel) { + return fmt.Errorf("user file path %q: must be a clean relative path inside the agent home", rel) + } + staged, err := stageFile(ctx, d, content) + if err != nil { + return err + } + // Best-effort cleanup on every return path, not just failure: the relay + // script itself removes the staged copy on success (see + // install-user-file.sh), so this is a harmless double-delete then. It is + // load-bearing on failure — notably the supported old-VM/missing-script + // case below — where the relay never touches the staged copy at all, + // which would otherwise leave a rendered credential sitting in the + // admin-only staging dir indefinitely. + defer cleanupStaged(d, staged) + if err := d.Client.Admin(ctx, []string{ + "/usr/local/lib/sandbox/install-user-file.sh", staged, rel, mode, + }); err != nil { + // A VM booted from a pre-this-feature code-vm binary lacks + // install-user-file.sh entirely, so this failure is otherwise an + // opaque exec error with no clue that the fix is a restart. + return fmt.Errorf("install %s: %w (the relay script may be missing because the VM predates this "+ + "code-vm version; restart it with `code-vm stop && code-vm start`)", rel, err) + } + return nil +} + +// cleanupStaged best-effort removes a staged file via the admin channel. It +// uses an independent, bounded context rather than the caller's ctx so it +// still runs when that ctx is cancelled or already past its deadline — +// exactly the case a failed Admin call above may have left it in. The error +// is intentionally discarded: this is cleanup of a temporary drop, not a +// step whose failure should mask (or be conflated with) the actual result of +// PushUserFile. +func cleanupStaged(d Deps, staged string) { + ctx, cancel := context.WithTimeout(context.Background(), stagedCleanupTimeout) + defer cancel() + _ = d.Client.Admin(ctx, []string{"rm", "-f", staged}) +} + +// hasDotDotSegment reports whether rel contains a literal ".." path segment. +func hasDotDotSegment(rel string) bool { + for _, seg := range strings.Split(rel, "/") { + if seg == ".." { + return true + } + } + return false +} diff --git a/internal/session/userfiles_test.go b/internal/session/userfiles_test.go new file mode 100644 index 0000000..43eb03c --- /dev/null +++ b/internal/session/userfiles_test.go @@ -0,0 +1,126 @@ +package session + +import ( + "context" + "errors" + "strings" + "testing" +) + +// erroringRunner fails the Run call whose args join contains match; every +// other call is delegated to the embedded fakeRunner so it still records +// calls and behaves normally. +type erroringRunner struct { + fakeRunner + match string + err error +} + +func (r *erroringRunner) Run(ctx context.Context, args ...string) error { + if strings.Contains(strings.Join(args, " "), r.match) { + r.calls = append(r.calls, args) + return r.err + } + return r.fakeRunner.Run(ctx, args...) +} + +// A VM booted from a pre-this-feature code-vm binary lacks +// install-user-file.sh, so PushUserFile's Admin call fails with an opaque +// exec error. The wrapped message must point at the fix: restarting the VM. +func TestPushUserFileWrapsAdminFailureWithUpgradeHint(t *testing.T) { + r := &erroringRunner{match: "install-user-file.sh", err: errors.New("exec format error")} + d := testDeps(t, r) + err := PushUserFile(context.Background(), d, []byte("content"), ".gitconfig", "0644") + if err == nil { + t.Fatal("PushUserFile: expected an error, got nil") + } + if !strings.Contains(err.Error(), "code-vm stop") || !strings.Contains(err.Error(), "code-vm start") { + t.Errorf("PushUserFile error = %v, want it to mention `code-vm stop && code-vm start`", err) + } + if !errors.Is(err, r.err) { + t.Errorf("PushUserFile error = %v, want it to wrap the underlying error (%%w)", err) + } +} + +// When the relay Admin call fails — the supported old-VM/missing-script +// case — the staged copy must still be cleaned up: otherwise a rendered +// credential is left sitting in the admin-only staging dir indefinitely. +func TestPushUserFileCleansUpStagedFileOnRelayFailure(t *testing.T) { + r := &erroringRunner{match: "install-user-file.sh", err: errors.New("exec format error")} + d := testDeps(t, r) + err := PushUserFile(context.Background(), d, []byte("content"), ".gitconfig", "0644") + if err == nil { + t.Fatal("PushUserFile: expected an error, got nil") + } + if !r.ranAny("rm -f") { + t.Errorf("expected a best-effort staged-file cleanup after the relay failed, calls=%v", r.calls) + } +} + +func TestPushUserFileStagesAndRelays(t *testing.T) { + r := &fakeRunner{} + d := testDeps(t, r) + if err := PushUserFile(context.Background(), d, []byte("content"), ".m2/settings.xml", "0600"); err != nil { + t.Fatalf("PushUserFile: %v", err) + } + copies := 0 + for _, c := range r.calls { + if len(c) > 0 && c[0] == "copy" { + copies++ + } + } + if copies != 1 { + t.Errorf("staged copies = %d, want 1", copies) + } + if !r.ranAny("/usr/local/lib/sandbox/install-user-file.sh") { + t.Errorf("relay script not invoked: %v", r.calls) + } + if !r.ranAny(".m2/settings.xml") || !r.ranAny("0600") { + t.Errorf("dst/mode not passed to the relay: %v", r.calls) + } + // The old direct-to-home root install must NOT happen for user files. + if r.ranAny("install -D -m 0600") { + t.Errorf("user files must not be root-installed into the home: %v", r.calls) + } +} + +func TestPushUserFileRejectsUnsafeRel(t *testing.T) { + for _, rel := range []string{"../x", "/abs", "", "has space", "semi;colon", "dollar$sign"} { + t.Run(rel, func(t *testing.T) { + r := &fakeRunner{} + d := testDeps(t, r) + err := PushUserFile(context.Background(), d, []byte("content"), rel, "0600") + if err == nil { + t.Fatalf("PushUserFile(%q) = nil error, want a rejection", rel) + } + if len(r.calls) != 0 { + t.Errorf("unsafe rel must not reach the guest, calls=%v", r.calls) + } + }) + } +} + +func TestPushUserFileAcceptsCleanRel(t *testing.T) { + r := &fakeRunner{} + d := testDeps(t, r) + if err := PushUserFile(context.Background(), d, []byte("content"), ".m2/settings.xml", "0600"); err != nil { + t.Fatalf("PushUserFile: %v", err) + } +} + +func TestGitIdentityUsesUserFilePush(t *testing.T) { + r := &fakeRunner{} + d := testDeps(t, r) + d.Host = func(ctx context.Context, name string, args ...string) ([]byte, error) { + return []byte("simon\n"), nil + } + if err := ApplyGitIdentity(context.Background(), d); err != nil { + t.Fatalf("ApplyGitIdentity: %v", err) + } + if !r.ranAny("install-user-file.sh") || !r.ranAny(".gitconfig") { + t.Errorf("git identity must go through the relay: %v", r.calls) + } + if r.ranAny("install -D -m 0644") { + t.Errorf("git identity must no longer be root-installed: %v", r.calls) + } +} diff --git a/test-vm-sandbox.sh b/test-vm-sandbox.sh index dfc9dfd..5fb25d3 100644 --- a/test-vm-sandbox.sh +++ b/test-vm-sandbox.sh @@ -529,8 +529,22 @@ cat > "$PROFILE_FIXTURE/hook.sh" << 'SH' set -eu echo hook-ran > "$HOME/.profile-hook-ran" SH +# The original heredoc above declares neither key, so appending is safe here; +# the extra-unmapped secret added later must use `yq -i` instead, since a +# second top-level `secrets:` key would be a YAML duplicate-key parse error. +printf 'secrets:\n test-token:\n description: integration fixture token\nvars:\n test-url: {}\n' \ + >> "$PROFILE_FIXTURE/profile.yaml" +mkdir -p "$PROFILE_FIXTURE/templates/.config" +# shellcheck disable=SC2016 # placeholders, substituted host-side at apply +printf 'token=${secret:test-token}\nurl=${var:test-url}\nkeep=${env.HOME}\n' \ + > "$PROFILE_FIXTURE/templates/.config/fixture.conf" yq -i '.profiles = ["test-profile"]' "$CONFIG_FILE" +# Map the declared secret/var to inputs, in the scratch config tree. +printf 'secrets:\n test-token:\n value: sekrit-value\n' > "$TEST_CONFIG_DIR/secrets.yaml" +chmod 0600 "$TEST_CONFIG_DIR/secrets.yaml" +yq -i '.vars = {"test-url": "https://fixture.example"}' "$CONFIG_FILE" + if "${CODE_VM_ARGS[@]}" profile apply > /dev/null 2>&1; then pass "profile apply succeeds" else @@ -581,6 +595,47 @@ assert_ok "profile domain is allowed live" \ assert_fails "agent cannot write the guest profile tree" \ agent bash -c 'echo x > /usr/local/share/sandbox-profiles/manifest.env' +RENDERED="/home/$AGENT_USER/.config/fixture.conf" +if adm cat "$RENDERED" 2> /dev/null | grep -q 'token=sekrit-value'; then + pass "template secret is substituted in the guest" +else + fail "template secret is substituted in the guest" +fi +assert_ok "template var is substituted" \ + adm grep -q 'url=https://fixture.example' "$RENDERED" +# shellcheck disable=SC2016 # literal placeholder text, must NOT expand here +assert_ok "unrelated placeholders pass through" \ + adm grep -qF 'keep=${env.HOME}' "$RENDERED" +if [ "$(adm stat -c '%u %a' "$RENDERED")" = "$(id -u) 600" ]; then + pass "rendered template is agent-owned 0600" +else + fail "rendered template is agent-owned 0600 (got $(adm stat -c '%u %a' "$RENDERED"))" +fi + +# Rotation: change the mapped value, re-apply, and the rendered file updates. +printf 'secrets:\n test-token:\n value: rotated-value\n' > "$TEST_CONFIG_DIR/secrets.yaml" +"${CODE_VM_ARGS[@]}" profile apply > /dev/null 2>&1 +assert_ok "a mapping change plus apply updates the rendered template" \ + adm grep -q 'token=rotated-value' "$RENDERED" + +# A declared-but-unmapped secret must fail apply with the snippet, before +# anything reaches the guest. yq, not printf-append: a second top-level +# `secrets:` key would be a YAML duplicate-key parse error, a different +# failure than the one under test. +yq -i '.secrets.extra-unmapped = {"suggest": "gopass show -o nope"}' "$PROFILE_FIXTURE/profile.yaml" +# shellcheck disable=SC2016 # placeholder, substituted host-side at apply +printf 'x=${secret:extra-unmapped}\n' > "$PROFILE_FIXTURE/templates/.config/extra.conf" +UNMAPPED_OUT=$("${CODE_VM_ARGS[@]}" profile apply 2>&1); UNMAPPED_RC=$? +if [ "$UNMAPPED_RC" -ne 0 ] && echo "$UNMAPPED_OUT" | grep -q 'gopass show -o nope'; then + pass "unmapped secret fails apply (nonzero) with the ready-to-paste snippet" +else + fail "unmapped secret fails apply with the ready-to-paste snippet (rc=$UNMAPPED_RC, got: $UNMAPPED_OUT)" +fi +# Restore the fixture to the mapped-only state for the deactivation steps. +rm -f "$PROFILE_FIXTURE/templates/.config/extra.conf" +yq -i 'del(.secrets.extra-unmapped)' "$PROFILE_FIXTURE/profile.yaml" +"${CODE_VM_ARGS[@]}" profile apply > /dev/null 2>&1 + # Deactivate: the guest tree is cleared and the domain revoked. Installed # package and shell deliberately survive (documented non-goal). mv "$CONFIG_FILE.profiles-backup" "$CONFIG_FILE" @@ -590,6 +645,7 @@ assert_fails "deactivated profile's guest tree is gone" \ assert_fails "deactivated profile's domain is revoked" \ agent curl -fsS -o /dev/null --max-time 20 https://example.org adm rm -f "/home/$AGENT_USER/.profile-hook-ran" > /dev/null 2>&1 +adm rm -f "$RENDERED" > /dev/null 2>&1 # cleanup with the other fixture artifacts rm -rf "$TEST_CONFIG_DIR/profiles" echo ""