diff --git a/cmd/proxyctl/main.go b/cmd/proxyctl/main.go index 159635b..3bb34d9 100644 --- a/cmd/proxyctl/main.go +++ b/cmd/proxyctl/main.go @@ -12,7 +12,6 @@ import ( "syscall" tea "github.com/charmbracelet/bubbletea" - "golang.org/x/sys/unix" "proxyctl/internal/app" "proxyctl/internal/apt" @@ -244,44 +243,22 @@ func diagnosticChecks(configuration profile.Configuration) []target.Check { } func storagePermissions(configuration profile.Configuration) (bool, string) { - ok, evidence := writableLocation(configuration.Path) - info, err := os.Stat(configuration.Path) + authority := target.ReadOnlyWriteAuthority(configuration.Path) + if !authority.OK { + return false, authority.Evidence + } + info, err := os.Lstat(configuration.Path) if os.IsNotExist(err) { - return ok, evidence + "; Profile configuration does not exist yet" + return true, authority.Evidence + "; Profile configuration does not exist yet" } if err != nil { - return false, evidence + "; Profile configuration cannot be inspected: " + err.Error() + return false, authority.Evidence + "; Profile configuration cannot be inspected: " + err.Error() } if info.Mode().Perm() != 0o600 { - return false, fmt.Sprintf("%s; Profile configuration mode is %o, want 600", evidence, info.Mode().Perm()) + return false, fmt.Sprintf("%s; Profile configuration mode is %o, want 600", authority.Evidence, info.Mode().Perm()) } if stat, present := info.Sys().(*syscall.Stat_t); present && int(stat.Uid) != configuration.Owner.UID { - return false, fmt.Sprintf("%s; Profile configuration owner uid is %d, want %d", evidence, stat.Uid, configuration.Owner.UID) - } - return ok, evidence + "; Profile configuration is owner-only" -} - -// writableLocation observes current authority without creating or changing files. -func writableLocation(path string) (bool, string) { - candidate := path - for { - info, err := os.Stat(candidate) - if err == nil { - if !info.IsDir() { - candidate = filepath.Dir(candidate) - } - if err := unix.Access(candidate, unix.W_OK); err != nil { - return false, candidate + " is not writable: " + err.Error() - } - return true, candidate + " is writable" - } - if !os.IsNotExist(err) { - return false, candidate + " cannot be inspected: " + err.Error() - } - parent := filepath.Dir(candidate) - if parent == candidate { - return false, path + " has no writable existing ancestor" - } - candidate = parent + return false, fmt.Sprintf("%s; Profile configuration owner uid is %d, want %d", authority.Evidence, stat.Uid, configuration.Owner.UID) } + return true, authority.Evidence + "; Profile configuration is owner-only" } diff --git a/cmd/proxyctl/maintenance_test.go b/cmd/proxyctl/maintenance_test.go index a2ec729..f1707d7 100644 --- a/cmd/proxyctl/maintenance_test.go +++ b/cmd/proxyctl/maintenance_test.go @@ -8,6 +8,7 @@ import ( "os" "os/exec" "path/filepath" + "reflect" "strings" "testing" @@ -98,6 +99,69 @@ func TestDoctorReturnsFailureWhenAnyCheckFails(t *testing.T) { } } +func TestDoctorStorageAuthorityRejectsSymlinkedPathsWithoutTouchingVictim(t *testing.T) { + for _, test := range []struct { + name string + path func(t *testing.T, root, victim string) string + }{ + { + name: "final path", + path: func(t *testing.T, root, victim string) string { + t.Helper() + path := filepath.Join(root, "config", "profiles.json") + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + t.Fatal(err) + } + if err := os.Symlink(victim, path); err != nil { + t.Fatal(err) + } + return path + }, + }, + { + name: "ancestor path", + path: func(t *testing.T, root, victim string) string { + t.Helper() + link := filepath.Join(root, "config") + if err := os.Symlink(filepath.Dir(victim), link); err != nil { + t.Fatal(err) + } + return filepath.Join(link, filepath.Base(victim)) + }, + }, + } { + t.Run(test.name, func(t *testing.T) { + root := t.TempDir() + victim := filepath.Join(t.TempDir(), "victim.json") + const content = `{"profiles":[]}` + if err := os.WriteFile(victim, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + before, err := os.Stat(victim) + if err != nil { + t.Fatal(err) + } + configuration := profile.Configuration{Path: test.path(t, root, victim), Owner: profile.UserIdentity{UID: os.Getuid(), GID: os.Getgid()}} + + checks := diagnosticChecks(configuration) + if len(checks) != 2 || checks[1].OK || !strings.Contains(checks[1].Evidence, "symbolic link") { + t.Fatalf("Profile storage diagnostics accepted symlink: %#v", checks) + } + after, err := os.Stat(victim) + if err != nil { + t.Fatal(err) + } + got, err := os.ReadFile(victim) + if err != nil { + t.Fatal(err) + } + if string(got) != content || !reflect.DeepEqual(before.Sys(), after.Sys()) { + t.Fatalf("storage diagnostics changed victim: before=%#v after=%#v content=%q", before, after, got) + } + }) + } +} + func TestDoctorStoragePermissionCheckIsReadOnly(t *testing.T) { path := filepath.Join(t.TempDir(), "proxyctl", "profiles.json") writeConfig(t, path, `{"profiles":[]}`) diff --git a/internal/dockerdaemon/dockerdaemon.go b/internal/dockerdaemon/dockerdaemon.go index ea84e18..894925d 100644 --- a/internal/dockerdaemon/dockerdaemon.go +++ b/internal/dockerdaemon/dockerdaemon.go @@ -336,12 +336,30 @@ func (a *Adapter) disable(ctx context.Context) error { if err != nil { return fmt.Errorf("read Docker daemon recovery metadata: %w", err) } + + // daemon.json is a shared, whole-file configuration surface. Restore it only + // while the complete current document is exactly the document proxyctl last + // applied. This protects both managed proxy keys and unrelated administrator + // edits from being overwritten by the original snapshot. + current, currentErr := a.files.ReadFile(a.configuration.DaemonConfigPath) + if currentErr != nil && !errors.Is(currentErr, fs.ErrNotExist) { + return fmt.Errorf("read Docker daemon configuration before restore: %w", currentErr) + } + alreadyOriginal := metadata.OriginalExists && currentErr == nil && fingerprint(current) == fingerprint(metadata.Original) + alreadyAbsent := !metadata.OriginalExists && errors.Is(currentErr, fs.ErrNotExist) + if alreadyOriginal || alreadyAbsent { + if err := a.files.Remove(a.configuration.RecoveryPath); err != nil && !errors.Is(err, fs.ErrNotExist) { + return fmt.Errorf("clean Docker daemon recovery metadata for already-restored configuration: %w", err) + } + return nil + } + if currentErr != nil || fingerprint(current) != metadata.ConfigFingerprint { + return errors.New("Docker daemon configuration changed after proxyctl applied it; refusing to overwrite current configuration or discard recovery metadata") + } + if metadata.OriginalExists { - current, _ := a.files.ReadFile(a.configuration.DaemonConfigPath) - if fingerprint(current) != fingerprint(metadata.Original) { - if err := a.files.AtomicWrite(a.configuration.DaemonConfigPath, metadata.Original, os.FileMode(metadata.OriginalMode)); err != nil { - return fmt.Errorf("restore Docker daemon configuration: %w", err) - } + if err := a.files.AtomicWrite(a.configuration.DaemonConfigPath, metadata.Original, os.FileMode(metadata.OriginalMode)); err != nil { + return fmt.Errorf("restore Docker daemon configuration: %w", err) } } else if err := a.files.Remove(a.configuration.DaemonConfigPath); err != nil && !errors.Is(err, fs.ErrNotExist) { return fmt.Errorf("remove managed Docker daemon configuration: %w", err) diff --git a/internal/dockerdaemon/dockerdaemon_test.go b/internal/dockerdaemon/dockerdaemon_test.go index f07448a..249c68e 100644 --- a/internal/dockerdaemon/dockerdaemon_test.go +++ b/internal/dockerdaemon/dockerdaemon_test.go @@ -355,6 +355,126 @@ func TestDockerDaemonEnableOrdersManagedWriteReloadRestartAndPostInspect(t *test } } +func TestDockerDaemonDisableAndResetRejectDriftWithoutMutatingDaemonOrRecovery(t *testing.T) { + for _, test := range []struct { + name string + mutate func(t *testing.T, managed []byte) []byte + }{ + { + name: "unrelated administrator edit", + mutate: func(t *testing.T, managed []byte) []byte { + t.Helper() + var document map[string]any + if err := json.Unmarshal(managed, &document); err != nil { + t.Fatal(err) + } + document["log-level"] = "debug" + changed, err := json.MarshalIndent(document, "", " ") + if err != nil { + t.Fatal(err) + } + return append(changed, '\n') + }, + }, + { + name: "managed proxy edit", + mutate: func(t *testing.T, managed []byte) []byte { + t.Helper() + var document map[string]any + if err := json.Unmarshal(managed, &document); err != nil { + t.Fatal(err) + } + document["proxies"].(map[string]any)["http-proxy"] = "http://administrator.example:3128" + changed, err := json.MarshalIndent(document, "", " ") + if err != nil { + t.Fatal(err) + } + return append(changed, '\n') + }, + }, + } { + t.Run(test.name, func(t *testing.T) { + a, record, daemonPath, recoveryPath, _ := fixture(t) + if result := a.SetDesired(context.Background(), true, activeProfile()); result.Err != nil { + t.Fatal(result.Err) + } + managed, err := os.ReadFile(daemonPath) + if err != nil { + t.Fatal(err) + } + changed := test.mutate(t, managed) + if err := os.WriteFile(daemonPath, changed, 0o600); err != nil { + t.Fatal(err) + } + recoveryBefore, err := os.ReadFile(recoveryPath) + if err != nil { + t.Fatal(err) + } + record.events = nil + + reset := app.NewMaintenance(profile.State{Profiles: []profile.Profile{*activeProfile()}, ActiveProfile: activeProfile().Name}, []target.Target{a}, immediateLock{}, nil).Reset(context.Background(), true) + if reset.Err == nil || len(reset.Targets) != 1 || reset.Targets[0].Err == nil || reset.Targets[0].Inspection.State != target.Drifted { + t.Fatalf("conflicting reset = %#v", reset) + } + if events := mutationEvents(record.events); len(events) != 0 { + t.Fatalf("conflicting reset mutated daemon state: %#v", events) + } + if got, err := os.ReadFile(daemonPath); err != nil || !reflect.DeepEqual(got, changed) { + t.Fatalf("current daemon configuration changed to %q: %v", got, err) + } + if got, err := os.ReadFile(recoveryPath); err != nil || !reflect.DeepEqual(got, recoveryBefore) { + t.Fatalf("recovery metadata changed to %q: %v", got, err) + } + }) + } +} + +func TestDockerDaemonResetCleansAlreadyRestoredRecoveryWithoutDaemonWriteOrSystemctl(t *testing.T) { + a, record, daemonPath, recoveryPath, _ := fixture(t) + original := []byte("{\n \"log-level\": \"debug\",\n \"proxies\": {\"http-proxy\": \"http://prior.example:3128\"}\n}\n") + if err := os.MkdirAll(filepath.Dir(daemonPath), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(daemonPath, original, 0o640); err != nil { + t.Fatal(err) + } + if result := a.SetDesired(context.Background(), true, activeProfile()); result.Err != nil { + t.Fatal(result.Err) + } + if err := os.WriteFile(daemonPath, original, 0o640); err != nil { + t.Fatal(err) + } + record.events = nil + + reset := app.NewMaintenance(profile.State{Profiles: []profile.Profile{*activeProfile()}, ActiveProfile: activeProfile().Name}, []target.Target{a}, immediateLock{}, nil).Reset(context.Background(), true) + if reset.Err != nil || len(reset.Targets) != 1 || reset.Targets[0].Err != nil || reset.Targets[0].Inspection.State != target.External { + t.Fatalf("already-restored reset = %#v", reset) + } + for _, event := range record.events { + if event == "write daemon.json" || strings.Contains(event, "daemon-reload") || strings.Contains(event, " restart ") { + t.Fatalf("already-restored reset changed daemon or systemd state: %#v", record.events) + } + } + if !contains(record.events, "remove docker-daemon.json") { + t.Fatalf("already-restored reset did not clean recovery metadata: %#v", record.events) + } + if got, err := os.ReadFile(daemonPath); err != nil || !reflect.DeepEqual(got, original) { + t.Fatalf("already-restored daemon configuration changed to %q: %v", got, err) + } + if info, err := os.Stat(daemonPath); err != nil || info.Mode().Perm() != 0o640 { + t.Fatalf("already-restored daemon metadata changed: info=%v err=%v", info, err) + } + if _, err := os.Stat(recoveryPath); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("recovery metadata remains: %v", err) + } + + record.events = nil + retry := a.SetDesired(context.Background(), false, nil) + if retry.Err != nil || retry.Inspection.State != target.External || len(mutationEvents(record.events)) != 0 { + t.Fatalf("already-restored cleanup retry = %#v events=%#v", retry, record.events) + } +} + func TestDockerDaemonPreservesAndRestoresExistingConfiguration(t *testing.T) { a, _, daemonPath, recoveryPath, _ := fixture(t) original := []byte("{\n \"log-level\": \"debug\",\n \"proxies\": {\"http-proxy\": \"http://prior.example:3128\"}\n}\n") diff --git a/internal/shell/shell.go b/internal/shell/shell.go index f01eb46..9bdaeab 100644 --- a/internal/shell/shell.go +++ b/internal/shell/shell.go @@ -14,8 +14,6 @@ import ( "path/filepath" "strings" - "golang.org/x/sys/unix" - "proxyctl/internal/profile" "proxyctl/internal/securefs" "proxyctl/internal/target" @@ -194,41 +192,9 @@ func (a *Adapter) StartupPath() string { return a.configuration.StartupFile // DiagnosticChecks performs no writes or command execution. func (a *Adapter) DiagnosticChecks(_ context.Context, _ *profile.Profile) []target.Check { - paths := []string{a.configuration.EnvironmentFile, a.configuration.RecoveryFile} - writable := true - var evidence []string - for _, path := range paths { - ok, detail := writablePath(path) - writable = writable && ok - evidence = append(evidence, detail) - } return []target.Check{ {Name: "required commands", OK: true, Evidence: "Shell Target requires no external commands"}, - {Name: "write authority", OK: writable, Evidence: strings.Join(evidence, "; ")}, - } -} - -func writablePath(path string) (bool, string) { - candidate := path - for { - info, err := os.Stat(candidate) - if err == nil { - if !info.IsDir() { - candidate = filepath.Dir(candidate) - } - if err := unix.Access(candidate, unix.W_OK); err != nil { - return false, candidate + " is not writable: " + err.Error() - } - return true, candidate + " is writable" - } - if !errors.Is(err, fs.ErrNotExist) { - return false, candidate + " cannot be inspected: " + err.Error() - } - parent := filepath.Dir(candidate) - if parent == candidate { - return false, path + " has no writable existing ancestor" - } - candidate = parent + target.ReadOnlyWriteAuthority(a.configuration.EnvironmentFile, a.configuration.RecoveryFile), } } diff --git a/internal/shell/shell_test.go b/internal/shell/shell_test.go index 9dfbc84..1b122d0 100644 --- a/internal/shell/shell_test.go +++ b/internal/shell/shell_test.go @@ -6,6 +6,7 @@ import ( "os" "os/exec" "path/filepath" + "reflect" "strings" "testing" @@ -15,6 +16,70 @@ import ( "proxyctl/internal/target/targettest" ) +func TestShellDiagnosticsRejectSymlinkedPathsWithoutTouchingVictim(t *testing.T) { + for _, test := range []struct { + name string + path func(t *testing.T, root, victim string) string + }{ + { + name: "final path", + path: func(t *testing.T, root, victim string) string { + t.Helper() + path := filepath.Join(root, "config", "shell.env") + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + t.Fatal(err) + } + if err := os.Symlink(victim, path); err != nil { + t.Fatal(err) + } + return path + }, + }, + { + name: "ancestor path", + path: func(t *testing.T, root, victim string) string { + t.Helper() + link := filepath.Join(root, "config") + if err := os.Symlink(filepath.Dir(victim), link); err != nil { + t.Fatal(err) + } + return filepath.Join(link, filepath.Base(victim)) + }, + }, + } { + t.Run(test.name, func(t *testing.T) { + root := t.TempDir() + victim := filepath.Join(t.TempDir(), "victim") + const content = "shell-diagnostic-victim" + if err := os.WriteFile(victim, []byte(content), 0o640); err != nil { + t.Fatal(err) + } + before, err := os.Stat(victim) + if err != nil { + t.Fatal(err) + } + environmentPath := test.path(t, root, victim) + a := shell.New(shell.Configuration{Shell: "/bin/bash", EnvironmentFile: environmentPath, RecoveryFile: filepath.Join(root, "state", "shell.json")}, nil, shell.MapEnvironment{}) + + checks := a.DiagnosticChecks(context.Background(), nil) + if len(checks) != 2 || checks[1].OK || !strings.Contains(checks[1].Evidence, "symbolic link") { + t.Fatalf("Shell diagnostics accepted symlink: %#v", checks) + } + after, err := os.Stat(victim) + if err != nil { + t.Fatal(err) + } + got, err := os.ReadFile(victim) + if err != nil { + t.Fatal(err) + } + if string(got) != content || !reflect.DeepEqual(before.Sys(), after.Sys()) { + t.Fatalf("diagnostics changed victim: before=%#v after=%#v content=%q", before, after, got) + } + }) + } +} + func TestShellAvailabilitySupportsOnlyBashAndZsh(t *testing.T) { for _, tc := range []struct { shell string