From d4cade09085cb9de6693775ec3792ee5d903f30d Mon Sep 17 00:00:00 2001 From: Young Joon Lee Date: Tue, 10 Nov 2026 09:18:48 +0900 Subject: [PATCH] fix(config): keep config init captures private when overwriting config init --file wrote with os.WriteFile, which sets the mode only on create, so an overwritten 0644 file stayed world-readable even when a --from-system capture held the tunnel token. Write a 0600 temp file in the destination directory, sync it and rename it over the destination. A failed write keeps the previous file, and a symlink, special file or file owned by another non-root user is refused. --- docs/commands.md | 2 +- docs/specs/config.md | 4 +- internal/cli/configcmd.go | 55 ++++++++++++++++++++-- internal/cli/configcmd_test.go | 85 ++++++++++++++++++++++++++++++++++ 4 files changed, 138 insertions(+), 8 deletions(-) diff --git a/docs/commands.md b/docs/commands.md index 1d28461..2b6f717 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -160,7 +160,7 @@ Write a site config: a commented template that extends a profile, or a capture o | Flag | Default | Purpose | |---|---|---| | `--extends` | detected suggestion | Profile the template extends. | -| `--file` | stdout | Write to this file instead of stdout. A new file is created with mode 0600; an existing file overwritten with `--overwrite` keeps its current mode and owner, so restrict it (`chmod 600`) first, because a `--from-system` capture can hold the tunnel token (#119). | +| `--file` | stdout | Write to this file instead of stdout. The file ends with mode 0600 and owned by you, new or overwritten, because a `--from-system` capture can hold the tunnel token. A symlink, special file or file owned by another non-root user is refused; a failed write leaves the previous file unchanged. | | `--from-system` | `false` | Capture the running host's settings instead of the template; works on hosts not set up by rootfiles. | | `--overwrite` | `false` | Replace an existing `--file`. | diff --git a/docs/specs/config.md b/docs/specs/config.md index 8c74074..b98cd90 100644 --- a/docs/specs/config.md +++ b/docs/specs/config.md @@ -44,7 +44,7 @@ The config schema (`Config` and its module structs), the embedded profiles and t - `rootfiles config show [--reveal]`: prints the merged config for the same target `check` would use (`loadForInspection` via `resolveTarget`), with env and flag overrides; the tunnel token is masked unless `--reveal`. - `rootfiles config validate`: same loading, prints the source and exits non-zero on any error. On Rocky it also runs `ValidateCapabilities`. -- `rootfiles config init [--extends P] [--file F] [--overwrite] [--from-system]`: writes a template extending the suggested profile, or a snapshot of the running host (`SnapshotCurrent` in `internal/config/snapshot.go`, works on hosts not set up by rootfiles); a new file is created 0600 because a capture may hold the tunnel token, while an existing destination keeps its mode and owner (`os.WriteFile` semantics; hardening tracked in #119); an existing file needs `--overwrite` (`--force` is a deprecated alias). +- `rootfiles config init [--extends P] [--file F] [--overwrite] [--from-system]`: writes a template extending the suggested profile, or a snapshot of the running host (`SnapshotCurrent` in `internal/config/snapshot.go`, works on hosts not set up by rootfiles); a capture may hold the tunnel token, so the file always ends 0600 and owned by the caller, new or overwritten; an existing file needs `--overwrite` (`--force` is a deprecated alias), and a symlink, special file or file owned by another non-root user is refused. ## Distro support @@ -55,7 +55,7 @@ The config schema (`Config` and its module structs), the embedded profiles and t - Secrets: config show and interactive apply summaries share `maskSecret` (short values fully hidden, long values reveal at most four leading bytes). - Secrets: the inline `tunnel_token` never enters the fingerprint, the applied copy or `config show` output (masked); prefer `tunnel_token_file`. - Detection reads the host; tests redirect `useraddDefaultsPath`, `hostRoot` and `rootUID` instead of reading `/etc`. -- `config init --file` writes with `os.WriteFile` directly; it is an operator file, not a managed host file. +- `config init --file` writes a temp file in the destination directory and renames it over the destination (`writePrivateConfig` in `internal/cli/configcmd.go`), so a failed write leaves the previous file unchanged and a symlink is never followed; it is an operator file, not a managed host file, so it does not go through the Runner or backups. ## Tests that pin it diff --git a/internal/cli/configcmd.go b/internal/cli/configcmd.go index 0f3e58d..dd5a2a2 100644 --- a/internal/cli/configcmd.go +++ b/internal/cli/configcmd.go @@ -3,7 +3,9 @@ package cli import ( "fmt" "os" + "path/filepath" "strings" + "syscall" "github.com/spf13/cobra" @@ -131,11 +133,7 @@ func newConfigInitCmd() *cobra.Command { _, err := cmd.OutOrStdout().Write(data) return err } - if _, err := os.Stat(file); err == nil && !overwrite { - return fmt.Errorf("%s already exists (use --overwrite to replace it)", file) - } - // 0600: captured configs may contain the tunnel token. - if err := os.WriteFile(file, data, 0600); err != nil { + if err := writePrivateConfig(file, data, overwrite); err != nil { return err } fmt.Fprintf(cmd.OutOrStdout(), "wrote %s — check it with: rootfiles config validate --config %s\n", file, file) @@ -185,3 +183,50 @@ extends: %s `, extends) return b.String() } + +// writePrivateConfig writes a config that may hold the tunnel token. The file +// always ends 0600 and owned by the caller: data goes to a new temp file in the +// same directory and is renamed over the destination, so a failed write keeps +// the previous file and a symlink is replaced, never followed. +func writePrivateConfig(file string, data []byte, overwrite bool) error { + if fi, err := os.Lstat(file); err == nil { + if !overwrite { + return fmt.Errorf("%s already exists (use --overwrite to replace it)", file) + } + if !fi.Mode().IsRegular() { + return fmt.Errorf("%s is not a regular file (symlink or special file); remove it or choose another --file", file) + } + if st, ok := fi.Sys().(*syscall.Stat_t); ok && int(st.Uid) != os.Geteuid() && st.Uid != 0 { + return fmt.Errorf("%s is owned by uid %d, not the user running this command; remove it or choose another --file", file, st.Uid) + } + } else if !os.IsNotExist(err) { + return fmt.Errorf("checking %s: %w", file, err) + } + tmp, err := os.CreateTemp(filepath.Dir(file), ".rootfiles-config.*.tmp") + if err != nil { + return fmt.Errorf("creating a temporary file next to %s: %w (check the directory exists and is writable)", file, err) + } + tmpPath := tmp.Name() + fail := func(step string, err error) error { + _ = tmp.Close() + _ = os.Remove(tmpPath) + return fmt.Errorf("%s for %s: %w; the previous file, if any, is unchanged", step, file, err) + } + if err := tmp.Chmod(0600); err != nil { + return fail("setting mode 0600", err) + } + if _, err := tmp.Write(data); err != nil { + return fail("writing config", err) + } + if err := tmp.Sync(); err != nil { + return fail("syncing config", err) + } + if err := tmp.Close(); err != nil { + return fail("closing config", err) + } + if err := os.Rename(tmpPath, file); err != nil { + _ = os.Remove(tmpPath) + return fmt.Errorf("replacing %s: %w; the previous file, if any, is unchanged", file, err) + } + return nil +} diff --git a/internal/cli/configcmd_test.go b/internal/cli/configcmd_test.go index 16a5beb..6e87fe5 100644 --- a/internal/cli/configcmd_test.go +++ b/internal/cli/configcmd_test.go @@ -71,3 +71,88 @@ func TestConfigValidateRejectsPrivateNetworkWithoutAddress(t *testing.T) { t.Errorf("expected private_network.address validation error, got %v", err) } } + +func TestConfigInitOverwriteTightensMode(t *testing.T) { + file := filepath.Join(t.TempDir(), "site.yaml") + if err := os.WriteFile(file, []byte("old"), 0644); err != nil { + t.Fatal(err) + } + if err := os.Chmod(file, 0644); err != nil { + t.Fatal(err) + } + var out bytes.Buffer + root := NewRootCmd("test", "test") + root.SetOut(&out) + root.SetErr(&out) + root.SetArgs([]string{"config", "init", "--extends", "minimal", "--file", file, "--overwrite"}) + if err := root.Execute(); err != nil { + t.Fatal(err) + } + if fi, err := os.Stat(file); err != nil || fi.Mode().Perm() != 0600 { + t.Fatalf("mode after overwrite: %v %v", fi.Mode(), err) + } + if strings.Contains(out.String(), "extends: minimal") { + t.Fatalf("config content printed with --file: %q", out.String()) + } +} + +func TestWritePrivateConfigNewFileIsPrivate(t *testing.T) { + dir := t.TempDir() + file := filepath.Join(dir, "capture.yaml") + token := "synthetic-test-token-not-real" + if err := writePrivateConfig(file, []byte("tunnel_token: "+token+"\n"), false); err != nil { + t.Fatal(err) + } + fi, err := os.Stat(file) + if err != nil || fi.Mode().Perm() != 0600 { + t.Fatalf("new capture mode: %v %v", fi.Mode(), err) + } + entries, _ := os.ReadDir(dir) + if len(entries) != 1 { + t.Fatalf("temp file left behind: %v", entries) + } + if err := writePrivateConfig(file, []byte("x"), false); err == nil || !strings.Contains(err.Error(), "--overwrite") { + t.Fatalf("existing file without overwrite: %v", err) + } +} + +func TestWritePrivateConfigRefusesSymlink(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "target") + if err := os.WriteFile(target, []byte("keep"), 0644); err != nil { + t.Fatal(err) + } + link := filepath.Join(dir, "site.yaml") + if err := os.Symlink(target, link); err != nil { + t.Fatal(err) + } + err := writePrivateConfig(link, []byte("secret"), true) + if err == nil || !strings.Contains(err.Error(), "not a regular file") { + t.Fatalf("symlink destination: %v", err) + } + if data, _ := os.ReadFile(target); string(data) != "keep" { + t.Fatalf("symlink target changed: %q", data) + } +} + +func TestWritePrivateConfigFailureKeepsPreviousFile(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root ignores directory write permission") + } + dir := t.TempDir() + file := filepath.Join(dir, "site.yaml") + if err := os.WriteFile(file, []byte("previous"), 0600); err != nil { + t.Fatal(err) + } + if err := os.Chmod(dir, 0500); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(dir, 0700) }) + err := writePrivateConfig(file, []byte("new"), true) + if err == nil || !strings.Contains(err.Error(), "writable") { + t.Fatalf("read-only directory: %v", err) + } + if data, _ := os.ReadFile(file); string(data) != "previous" { + t.Fatalf("previous file changed: %q", data) + } +}