From 0fbaa71e37ce90b7b3dff68d7e9d58c60c1deb4a Mon Sep 17 00:00:00 2001 From: proxyctl coordinator Date: Fri, 17 Jul 2026 10:29:41 +0800 Subject: [PATCH 1/5] fix: complete target diagnostics and reset inspection --- cmd/proxyctl/main.go | 2 +- cmd/proxyctl/main_test.go | 3 + cmd/proxyctl/maintenance_test.go | 4 +- internal/app/maintenance.go | 15 +++-- internal/app/maintenance_test.go | 38 ++++++++++++ internal/apt/apt.go | 28 +++++++++ internal/apt/apt_test.go | 24 ++++++++ internal/dockerclient/dockerclient.go | 30 +++++++++- internal/dockerclient/dockerclient_test.go | 23 +++++++ internal/dockerdaemon/dockerdaemon.go | 38 ++++++++++++ internal/dockerdaemon/dockerdaemon_test.go | 24 ++++++++ internal/gnome/gnome.go | 70 +++++++++++++++++++++- internal/gnome/gnome_test.go | 26 ++++++++ internal/target/diagnostic.go | 51 ++++++++++++++++ 14 files changed, 366 insertions(+), 10 deletions(-) create mode 100644 internal/target/diagnostic.go diff --git a/cmd/proxyctl/main.go b/cmd/proxyctl/main.go index d22ee25..159635b 100644 --- a/cmd/proxyctl/main.go +++ b/cmd/proxyctl/main.go @@ -224,7 +224,7 @@ func runMaintenanceCommand(args []string, input io.Reader, output io.Writer, sto continue } if item.Err != nil { - fmt.Fprintf(output, "%s: failed (%s) — %s\n", item.TargetID, item.Inspection.State, profile.RedactText(item.Err.Error())) + fmt.Fprintf(output, "%s: failed (%s) — %s; fresh inspection: %s\n", item.TargetID, item.Inspection.State, profile.RedactText(item.Err.Error()), profile.RedactText(item.Inspection.Evidence)) continue } fmt.Fprintf(output, "%s: reset (%s) — %s\n", item.TargetID, item.Inspection.State, profile.RedactText(item.Inspection.Evidence)) diff --git a/cmd/proxyctl/main_test.go b/cmd/proxyctl/main_test.go index cdef224..7009b45 100644 --- a/cmd/proxyctl/main_test.go +++ b/cmd/proxyctl/main_test.go @@ -50,6 +50,9 @@ func TestProductionCompositionRegistersAPTContainerGNOMEAndDaemonTargets(t *test var ids []string for _, adapter := range targets { ids = append(ids, adapter.ID()) + if _, ok := adapter.(target.Diagnosable); !ok { + t.Errorf("production Target %s does not contribute doctor checks", adapter.ID()) + } } want := []string{"apt", "docker-containers", "gnome", "docker-daemon"} if strings.Join(ids, ",") != strings.Join(want, ",") { diff --git a/cmd/proxyctl/maintenance_test.go b/cmd/proxyctl/maintenance_test.go index 3b9393f..a10bf71 100644 --- a/cmd/proxyctl/maintenance_test.go +++ b/cmd/proxyctl/maintenance_test.go @@ -232,10 +232,10 @@ func TestResetCommandCancellationAndTruthfulPartialFailureWithFakeTargets(t *tes t.Fatal("partial reset returned success") } text := output.String() - if good.calls != 1 || bad.calls != 1 || external.calls != 0 || !strings.Contains(text, "good: reset (Disabled)") || !strings.Contains(text, "bad: failed (Drifted)") || !strings.Contains(text, "external: skipped (External)") { + if good.calls != 1 || bad.calls != 1 || external.calls != 0 || !strings.Contains(text, "good: reset (Disabled)") || !strings.Contains(text, "bad: failed (Drifted)") || !strings.Contains(text, "fresh inspection: fake evidence for http://alice:REDACTED@proxy.test") || !strings.Contains(text, "external: skipped (External)") { t.Fatalf("partial reset output=%q calls=%d/%d/%d", text, good.calls, bad.calls, external.calls) } - if strings.Contains(text, "hunter2") || strings.Contains(err.Error(), "hunter2") { + if strings.Contains(text, "hunter2") || strings.Contains(text, "secret") || strings.Contains(err.Error(), "hunter2") { t.Fatalf("reset leaked credentials: output=%q err=%v", text, err) } logData, readErr := os.ReadFile(filepath.Join(stateDir, "operations.log")) diff --git a/internal/app/maintenance.go b/internal/app/maintenance.go index 9b5fd4b..133da03 100644 --- a/internal/app/maintenance.go +++ b/internal/app/maintenance.go @@ -80,9 +80,9 @@ func (m *Maintenance) Doctor(ctx context.Context, applicationChecks []target.Che active := activeFromState(m.state) for _, adapter := range m.targets { availability := adapter.Availability(ctx) - report.Diagnostics = append(report.Diagnostics, Diagnostic{TargetID: adapter.ID(), Check: target.Check{Name: "Target availability", OK: availability.Available, Evidence: availability.Evidence}}) + report.Diagnostics = append(report.Diagnostics, Diagnostic{TargetID: adapter.ID(), Check: target.Check{Name: "Target availability", OK: availability.Available, Evidence: profile.RedactText(availability.Evidence)}}) inspection := adapter.Inspect(ctx, active) - report.Diagnostics = append(report.Diagnostics, Diagnostic{TargetID: adapter.ID(), Check: target.Check{Name: "detected state", OK: inspection.State != target.Drifted, Evidence: string(inspection.State) + ": " + inspection.Evidence}}) + report.Diagnostics = append(report.Diagnostics, Diagnostic{TargetID: adapter.ID(), Check: target.Check{Name: "detected state", OK: inspection.State != target.Drifted, Evidence: profile.RedactText(string(inspection.State) + ": " + inspection.Evidence)}}) if diagnosable, ok := adapter.(target.Diagnosable); ok { for _, check := range diagnosable.DiagnosticChecks(ctx, active) { check.Evidence = profile.RedactText(check.Evidence) @@ -126,17 +126,22 @@ func (m *Maintenance) Reset(ctx context.Context, confirmed bool) ResetResult { continue } operation := adapter.SetDesired(ctx, false, active) - detail, outcome := operation.Inspection.Evidence, "success" + finalInspection := operation.Inspection + detail, outcome := finalInspection.Evidence, "success" if operation.Err != nil { + // OperationResult inspection is explicitly advisory on failure. Reset + // must publish and log a fresh observation just like interactive + // Target operations do. + finalInspection = adapter.Inspect(ctx, active) outcome = "failure" - detail = operation.Err.Error() + detail = fmt.Sprintf("%v; fresh inspection %s: %s", operation.Err, finalInspection.State, finalInspection.Evidence) result.Err = errors.Join(result.Err, fmt.Errorf("reset %s Target: %w", adapter.ID(), redactError(operation.Err))) } detail = RedactLogDetail(detail) if m.logger != nil { _ = m.logger.Write(LogEntry{Time: time.Now(), Operation: "reset", Target: adapter.ID(), Result: outcome, Detail: detail}) } - result.Targets = append(result.Targets, ResetTargetResult{TargetID: adapter.ID(), Inspection: operation.Inspection, Err: redactError(operation.Err)}) + result.Targets = append(result.Targets, ResetTargetResult{TargetID: adapter.ID(), Inspection: finalInspection, Err: redactError(operation.Err)}) } return result } diff --git a/internal/app/maintenance_test.go b/internal/app/maintenance_test.go index 4ebc173..d4022a6 100644 --- a/internal/app/maintenance_test.go +++ b/internal/app/maintenance_test.go @@ -43,6 +43,28 @@ func (f *maintenanceTarget) DiagnosticChecks(context.Context, *profile.Profile) return append([]target.Check(nil), f.checks...) } +type staleFailureTarget struct{ inspectCalls int } + +func (*staleFailureTarget) ID() string { return "stale" } +func (*staleFailureTarget) Description() target.Description { return target.Description{Name: "stale"} } +func (*staleFailureTarget) Availability(context.Context) target.Availability { + return target.Availability{Available: true} +} +func (*staleFailureTarget) Confirmation(bool) string { return "" } +func (f *staleFailureTarget) Inspect(context.Context, *profile.Profile) target.Inspection { + f.inspectCalls++ + if f.inspectCalls == 1 { + return target.Inspection{State: target.Enabled, Evidence: "initial managed state"} + } + return target.Inspection{State: target.External, Evidence: "fresh evidence http://alice:secret@proxy.example:8080"} +} +func (*staleFailureTarget) SetDesired(context.Context, bool, *profile.Profile) target.OperationResult { + return target.OperationResult{ + Inspection: target.Inspection{State: target.Drifted, Evidence: "stale returned evidence"}, + Err: errors.New("failed https://bob:hunter2@proxy.example/path"), + } +} + type recordingLock struct{ held, acquired int } func (l *recordingLock) Lock(context.Context) (func(), error) { @@ -111,6 +133,22 @@ func TestApplicationSerializesAndLogsTargetMutations(t *testing.T) { } } +func TestResetFailureUsesFreshInspectionForResultAndRedactedLog(t *testing.T) { + adapter := &staleFailureTarget{} + log := &recordingLog{} + result := app.NewMaintenance(profile.State{}, []target.Target{adapter}, &recordingLock{}, log).Reset(context.Background(), true) + if result.Err == nil || adapter.inspectCalls != 2 || len(result.Targets) != 1 { + t.Fatalf("reset result=%#v inspect calls=%d", result, adapter.inspectCalls) + } + got := result.Targets[0].Inspection + if got.State != target.External || !strings.Contains(got.Evidence, "fresh evidence") || strings.Contains(got.Evidence, "stale") { + t.Fatalf("failure inspection = %#v", got) + } + if len(log.entries) != 1 || !strings.Contains(log.entries[0].Detail, "fresh inspection External") || strings.Contains(log.entries[0].Detail, "secret") || strings.Contains(log.entries[0].Detail, "hunter2") { + t.Fatalf("failure log = %#v", log.entries) + } +} + func TestResetRequiresConfirmationAndReportsTruthfulPartialResults(t *testing.T) { good := &maintenanceTarget{id: "good", available: target.Availability{Available: true}, inspection: target.Inspection{State: target.Enabled, Evidence: "managed"}} failure := errors.New("remove https://alice:secret@proxy.example failed: permission denied") diff --git a/internal/apt/apt.go b/internal/apt/apt.go index faa1038..c9bf917 100644 --- a/internal/apt/apt.go +++ b/internal/apt/apt.go @@ -82,6 +82,8 @@ type Adapter struct { runner Runner } +var _ target.Diagnosable = (*Adapter)(nil) + func New(configuration Configuration, files FileSystem, runner Runner) *Adapter { defaults := DefaultConfiguration() if configuration.DebianVersionFile == "" { @@ -116,6 +118,32 @@ func (a *Adapter) Description() target.Description { func (a *Adapter) Confirmation(bool) string { return "" } +// DiagnosticChecks observes the APT command, configuration schema, and current +// filesystem authority without changing the overlay or effective settings. +func (a *Adapter) DiagnosticChecks(ctx context.Context, _ *profile.Profile) []target.Check { + commandCheck := target.Check{Name: "required command", OK: true, Evidence: "apt-config is available"} + command, err := a.runner.LookPath("apt-config") + if err != nil { + commandCheck.OK = false + commandCheck.Evidence = "apt-config is unavailable: " + profile.RedactText(err.Error()) + } else if _, err := a.runner.Output(ctx, command, "dump"); err != nil { + commandCheck.OK = false + commandCheck.Evidence = "apt-config dump failed: " + profile.RedactText(err.Error()) + } + marker, markerErr := os.Stat(a.configuration.DebianVersionFile) + directory, directoryErr := os.Stat(a.configuration.ConfigDirectory) + schemaOK := markerErr == nil && marker.Mode().IsRegular() && directoryErr == nil && directory.IsDir() + schemaEvidence := "Debian marker and apt.conf.d configuration directory are present" + if !schemaOK { + schemaEvidence = profile.RedactText(fmt.Sprintf("Debian marker or apt.conf.d configuration directory is unusable: marker=%v directory=%v", markerErr, directoryErr)) + } + return []target.Check{ + commandCheck, + {Name: "configuration schema", OK: schemaOK, Evidence: schemaEvidence}, + target.ReadOnlyWriteAuthority(a.configuration.OverlayFile), + } +} + func (a *Adapter) Availability(ctx context.Context) target.Availability { marker, err := os.Stat(a.configuration.DebianVersionFile) if err != nil || !marker.Mode().IsRegular() { diff --git a/internal/apt/apt_test.go b/internal/apt/apt_test.go index b6975ad..c50e9d1 100644 --- a/internal/apt/apt_test.go +++ b/internal/apt/apt_test.go @@ -46,6 +46,30 @@ func TestAvailabilityRequiresDebianMarkerAPTToolAndConfigurationDirectory(t *tes } } +func TestDiagnosticChecksCoverCommandSchemaAndAuthorityWithoutMutation(t *testing.T) { + a, _ := newAdapter(t, nil) + checks := a.DiagnosticChecks(context.Background(), compatibleProfile()) + assertDiagnosticNames(t, checks, "required command", "configuration schema", "write authority") + if _, err := os.Stat(a.OverlayPath()); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("diagnostics created APT overlay: %v", err) + } +} + +func assertDiagnosticNames(t *testing.T, checks []target.Check, names ...string) { + t.Helper() + for _, name := range names { + found := false + for _, check := range checks { + if check.Name == name { + found = true + } + } + if !found { + t.Errorf("diagnostics missing %q: %#v", name, checks) + } + } +} + func TestDescriptionExplainsEnvironmentPersistenceAndPrivilegeBoundary(t *testing.T) { a, _ := newAdapter(t, nil) got := a.Description() diff --git a/internal/dockerclient/dockerclient.go b/internal/dockerclient/dockerclient.go index 9e65681..43409bc 100644 --- a/internal/dockerclient/dockerclient.go +++ b/internal/dockerclient/dockerclient.go @@ -154,7 +154,10 @@ type Adapter struct { files FileSystem } -var _ target.Target = (*Adapter)(nil) +var ( + _ target.Target = (*Adapter)(nil) + _ target.Diagnosable = (*Adapter)(nil) +) func New(configuration Configuration, files FileSystem) *Adapter { if files == nil { @@ -174,6 +177,31 @@ func (a *Adapter) Description() target.Description { } func (a *Adapter) Confirmation(bool) string { return "" } +// DiagnosticChecks validates the readable JSON surfaces and observes write +// authority without taking the adapter lock or creating any files. +func (a *Adapter) DiagnosticChecks(_ context.Context, _ *profile.Profile) []target.Check { + commandCheck := target.Check{Name: "required commands", OK: true, Evidence: "Docker client defaults require no external commands"} + schemaCheck := target.Check{Name: "configuration schema", OK: true, Evidence: "Docker client configuration and recovery metadata are absent or valid JSON objects"} + if config, _, err := a.readConfig(); err != nil { + schemaCheck.OK = false + schemaCheck.Evidence = profile.RedactText("Docker client config.json is invalid or unreadable: " + err.Error()) + } else if proxies, _, err := childObject(config, "proxies"); err != nil { + schemaCheck.OK = false + schemaCheck.Evidence = profile.RedactText(err.Error()) + } else if _, _, err := childObject(proxies, "default"); err != nil { + schemaCheck.OK = false + schemaCheck.Evidence = profile.RedactText(err.Error()) + } else if _, _, err := a.readRecovery(); err != nil && !errors.Is(err, fs.ErrNotExist) { + schemaCheck.OK = false + schemaCheck.Evidence = profile.RedactText("Docker client recovery metadata is invalid or unreadable: " + err.Error()) + } + return []target.Check{ + commandCheck, + schemaCheck, + target.ReadOnlyWriteAuthority(a.configuration.ConfigFile, a.configuration.RecoveryFile), + } +} + func (a *Adapter) Availability(context.Context) target.Availability { if !a.usablePath(a.configuration.ConfigFile) || !a.usablePath(a.configuration.RecoveryFile) { return target.Availability{Evidence: "standard Docker client configuration paths are not usable for the owning user"} diff --git a/internal/dockerclient/dockerclient_test.go b/internal/dockerclient/dockerclient_test.go index 3236b02..b9f1f06 100644 --- a/internal/dockerclient/dockerclient_test.go +++ b/internal/dockerclient/dockerclient_test.go @@ -44,6 +44,29 @@ func TestAvailabilityAndDescriptionDescribeOnlyDockerClientDefaults(t *testing.T } } +func TestDiagnosticChecksCoverSchemaAndAuthorityWithoutMutation(t *testing.T) { + files := &countingFS{FileSystem: dockerclient.OSFileSystem{}} + a := newAdapterWithFS(t, files) + checks := a.DiagnosticChecks(context.Background(), activeProfile()) + for _, name := range []string{"required commands", "configuration schema", "write authority"} { + found := false + for _, check := range checks { + found = found || check.Name == name + } + if !found { + t.Errorf("diagnostics missing %q: %#v", name, checks) + } + } + if files.writes != 0 { + t.Fatalf("diagnostics wrote Docker client state %d times", files.writes) + } + for _, path := range []string{a.ConfigPath(), a.RecoveryPath(), a.LockPath()} { + if _, err := os.Stat(path); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("diagnostics created %s: %v", path, err) + } + } +} + func TestDockerClientAdapterRunsReusableTargetContract(t *testing.T) { targettest.Run(t, func(t *testing.T) targettest.Fixture { a := newAdapter(t, nil) diff --git a/internal/dockerdaemon/dockerdaemon.go b/internal/dockerdaemon/dockerdaemon.go index 3a888d5..ddd9496 100644 --- a/internal/dockerdaemon/dockerdaemon.go +++ b/internal/dockerdaemon/dockerdaemon.go @@ -74,6 +74,8 @@ type Adapter struct { runner CommandRunner } +var _ target.Diagnosable = (*Adapter)(nil) + func New(configuration Configuration, files FileSystem, runner CommandRunner) *Adapter { if configuration.DaemonConfigPath == "" { configuration.DaemonConfigPath = "/etc/docker/daemon.json" @@ -102,6 +104,42 @@ func (a *Adapter) Description() target.Description { func (a *Adapter) Confirmation(bool) string { return "Docker will restart immediately. This may disrupt running containers and other work. Confirm before proxyctl changes daemon configuration." } + +// DiagnosticChecks performs only systemctl show and filesystem reads/metadata +// probes; service reload/restart and all configuration mutations are excluded. +func (a *Adapter) DiagnosticChecks(ctx context.Context, _ *profile.Profile) []target.Check { + commandCheck := target.Check{Name: "required command and service", OK: true, Evidence: "systemctl can inspect the loaded docker.service"} + if availability, err := a.detect(ctx); err != nil || !availability.Available { + commandCheck.OK = false + if err != nil { + commandCheck.Evidence = profile.RedactText("systemctl show failed: " + err.Error()) + } else { + commandCheck.Evidence = availability.Evidence + } + } + schemaCheck := target.Check{Name: "configuration schema", OK: true, Evidence: "Docker daemon configuration and recovery metadata are absent or valid JSON"} + if data, err := a.files.ReadFile(a.configuration.DaemonConfigPath); err == nil { + if _, schemaErr := mergeConfig(data, profile.Profile{}); schemaErr != nil { + schemaCheck.OK = false + schemaCheck.Evidence = profile.RedactText("Docker daemon configuration cannot be safely merged: " + schemaErr.Error()) + } + } else if !errors.Is(err, fs.ErrNotExist) { + schemaCheck.OK = false + schemaCheck.Evidence = profile.RedactText("Docker daemon configuration cannot be read: " + err.Error()) + } + if schemaCheck.OK { + if _, _, err := a.readRecovery(); err != nil && !errors.Is(err, fs.ErrNotExist) { + schemaCheck.OK = false + schemaCheck.Evidence = profile.RedactText("Docker daemon recovery metadata is invalid or unreadable: " + err.Error()) + } + } + return []target.Check{ + commandCheck, + schemaCheck, + target.ReadOnlyWriteAuthority(a.configuration.DaemonConfigPath, a.configuration.RecoveryPath), + } +} + func (a *Adapter) Availability(ctx context.Context) target.Availability { availability, _ := a.detect(ctx) return availability diff --git a/internal/dockerdaemon/dockerdaemon_test.go b/internal/dockerdaemon/dockerdaemon_test.go index bd8b592..4f5d631 100644 --- a/internal/dockerdaemon/dockerdaemon_test.go +++ b/internal/dockerdaemon/dockerdaemon_test.go @@ -102,6 +102,30 @@ func (f *faultFS) AtomicWrite(path string, data []byte, mode os.FileMode) error return f.recordingFS.AtomicWrite(path, data, mode) } +func TestDiagnosticChecksCoverSystemdSchemaAndAuthorityWithoutMutation(t *testing.T) { + a, record, daemonPath, recoveryPath, _ := fixture(t) + checks := a.DiagnosticChecks(context.Background(), activeProfile()) + for _, name := range []string{"required command and service", "configuration schema", "write authority"} { + found := false + for _, check := range checks { + found = found || check.Name == name + } + if !found { + t.Errorf("diagnostics missing %q: %#v", name, checks) + } + } + for _, event := range record.events { + if strings.HasPrefix(event, "write ") || strings.HasPrefix(event, "remove ") || strings.Contains(event, " restart ") || strings.Contains(event, "daemon-reload") { + t.Fatalf("diagnostics mutated daemon state: %#v", record.events) + } + } + for _, path := range []string{daemonPath, recoveryPath} { + if _, err := os.Stat(path); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("diagnostics created %s: %v", path, err) + } + } +} + func TestDockerDaemonRunsReusableTargetContract(t *testing.T) { targettest.Run(t, func(t *testing.T) targettest.Fixture { a, _, daemonPath, _, _ := fixture(t) diff --git a/internal/gnome/gnome.go b/internal/gnome/gnome.go index a242739..085c424 100644 --- a/internal/gnome/gnome.go +++ b/internal/gnome/gnome.go @@ -34,6 +34,12 @@ const ( var requiredSchemas = []string{proxySchema, httpSchema, httpsSchema, socksSchema} +var managedSettings = []struct{ schema, key string }{ + {proxySchema, "mode"}, {proxySchema, "autoconfig-url"}, {proxySchema, "ignore-hosts"}, + {httpSchema, "host"}, {httpSchema, "port"}, {httpSchema, "use-authentication"}, {httpSchema, "authentication-user"}, {httpSchema, "authentication-password"}, + {httpsSchema, "host"}, {httpsSchema, "port"}, {socksSchema, "host"}, {socksSchema, "port"}, +} + // Command is the complete command contract used by the adapter. UID, GID, and // Env make user-session identity explicit when proxyctl itself runs under sudo. type Command struct { @@ -230,7 +236,10 @@ type Adapter struct { state StateStore } -var _ target.Target = (*Adapter)(nil) +var ( + _ target.Target = (*Adapter)(nil) + _ target.Diagnosable = (*Adapter)(nil) +) func New(configuration Configuration, runner CommandRunner, state StateStore) *Adapter { if configuration.CommandName == "" { @@ -271,6 +280,65 @@ func (a *Adapter) Description() target.Description { func (a *Adapter) Confirmation(bool) string { return "" } +// DiagnosticChecks uses only gsettings list/get operations and filesystem +// metadata probes. In particular, it never invokes gsettings set or locks or +// writes the recovery StateStore. +func (a *Adapter) DiagnosticChecks(ctx context.Context, _ *profile.Profile) []target.Check { + commandCheck := target.Check{Name: "required command", OK: true, Evidence: a.configuration.CommandName + " is usable for the owning user"} + schemaCheck := target.Check{Name: "required schemas", OK: true, Evidence: "all GNOME proxy schemas are present"} + sessionCheck := target.Check{Name: "desktop session", OK: true, Evidence: "GNOME proxy settings are readable through the configured session bus"} + output, err := a.run(ctx, "list-schemas") + if err != nil { + commandCheck.OK = false + commandCheck.Evidence = profile.RedactText(a.configuration.CommandName + " failed: " + err.Error()) + schemaCheck.OK = false + schemaCheck.Evidence = "GNOME schemas could not be listed" + sessionCheck.OK = false + sessionCheck.Evidence = "desktop session could not be inspected" + } else { + present := make(map[string]bool) + for _, schema := range strings.Fields(output) { + present[schema] = true + } + var missing []string + for _, schema := range requiredSchemas { + if !present[schema] { + missing = append(missing, schema) + } + } + if len(missing) != 0 { + schemaCheck.OK = false + schemaCheck.Evidence = "missing GNOME proxy schemas: " + strings.Join(missing, ", ") + sessionCheck.OK = false + sessionCheck.Evidence = "desktop session settings cannot be checked without required schemas" + } else if _, err := a.readSettings(ctx); err != nil { + sessionCheck.OK = false + sessionCheck.Evidence = profile.RedactText("GNOME proxy settings are unreadable: " + err.Error()) + } + } + authorityCheck := target.ReadOnlyWriteAuthority(a.configuration.StateFile) + if !commandCheck.OK || !schemaCheck.OK || !sessionCheck.OK { + authorityCheck.OK = false + authorityCheck.Evidence += "; GNOME setting authority could not be inspected" + } else { + for _, setting := range managedSettings { + output, err := a.run(ctx, "writable", setting.schema, setting.key) + if err != nil || strings.TrimSpace(output) != "true" { + authorityCheck.OK = false + detail := strings.TrimSpace(output) + if err != nil { + detail = err.Error() + } + authorityCheck.Evidence += profile.RedactText(fmt.Sprintf("; %s %s is not writable: %s", setting.schema, setting.key, detail)) + } + } + if authorityCheck.OK { + authorityCheck.Evidence += "; all managed GNOME settings report writable" + } + } + return []target.Check{commandCheck, schemaCheck, sessionCheck, authorityCheck} +} + func (a *Adapter) Availability(ctx context.Context) target.Availability { output, err := a.run(ctx, "list-schemas") if err != nil { diff --git a/internal/gnome/gnome_test.go b/internal/gnome/gnome_test.go index 4c0f306..526b2e7 100644 --- a/internal/gnome/gnome_test.go +++ b/internal/gnome/gnome_test.go @@ -55,6 +55,9 @@ func (r *fakeRunner) Run(_ context.Context, c gnome.Command) (string, error) { } return value + "\n", nil } + if len(c.Args) == 3 && c.Args[0] == "writable" { + return "true\n", nil + } if len(c.Args) == 4 && c.Args[0] == "set" { r.values[c.Args[1]+" "+c.Args[2]] = c.Args[3] return "", nil @@ -105,6 +108,29 @@ func active() *profile.Profile { return &profile.Profile{Name: "office", Mode: profile.AdvancedMode, HTTPURL: "http://proxy.example:8080", HTTPSURL: "http://secure.example:8443", AllURL: "socks5://socks.example:1080", Bypass: []string{"localhost", ".example.com", "10.0.0.0/8", "2001:db8::1"}} } +func TestDiagnosticChecksCoverCommandSchemasSessionAndAuthorityWithoutMutation(t *testing.T) { + r, state := newRunner(), &memoryState{} + a := adapter(r, state) + checks := a.DiagnosticChecks(context.Background(), active()) + for _, name := range []string{"required command", "required schemas", "desktop session", "write authority"} { + found := false + for _, check := range checks { + found = found || check.Name == name + } + if !found { + t.Errorf("diagnostics missing %q: %#v", name, checks) + } + } + for _, call := range r.calls { + if len(call.Args) > 0 && call.Args[0] == "set" { + t.Fatalf("diagnostics mutated GNOME settings: %#v", call) + } + } + if state.data != nil { + t.Fatalf("diagnostics wrote GNOME recovery state: %q", state.data) + } +} + func TestGNOMEAdapterRunsReusableTargetContract(t *testing.T) { targettest.Run(t, func(t *testing.T) targettest.Fixture { r, state := newRunner(), &memoryState{} diff --git a/internal/target/diagnostic.go b/internal/target/diagnostic.go new file mode 100644 index 0000000..9c39b06 --- /dev/null +++ b/internal/target/diagnostic.go @@ -0,0 +1,51 @@ +package target + +import ( + "errors" + "io/fs" + "os" + "path/filepath" + "strings" + + "golang.org/x/sys/unix" + + "proxyctl/internal/profile" +) + +// ReadOnlyWriteAuthority observes whether the current process can write the +// configured locations. It never creates a probe file or changes permissions. +func ReadOnlyWriteAuthority(paths ...string) Check { + check := Check{Name: "write authority", OK: true} + var evidence []string + for _, path := range paths { + ok, detail := readOnlyWritablePath(path) + check.OK = check.OK && ok + evidence = append(evidence, detail) + } + check.Evidence = profile.RedactText(strings.Join(evidence, "; ")) + return check +} + +func readOnlyWritablePath(path string) (bool, string) { + candidate := filepath.Clean(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 + } +} From a5360028caa164f5ae041ade43396676e98cdbb8 Mon Sep 17 00:00:00 2001 From: proxyctl coordinator Date: Fri, 17 Jul 2026 10:42:48 +0800 Subject: [PATCH 2/5] fix: harden read-only target diagnostics --- internal/dockerclient/dockerclient.go | 22 +++++- internal/dockerclient/dockerclient_test.go | 37 ++++++++++ internal/dockerdaemon/dockerdaemon.go | 65 ++++++++++++++--- internal/dockerdaemon/dockerdaemon_test.go | 78 ++++++++++++++++++++- internal/gnome/gnome.go | 18 ++++- internal/gnome/gnome_test.go | 43 ++++++++++++ internal/target/diagnostic.go | 81 ++++++++++++++++------ internal/target/diagnostic_test.go | 74 ++++++++++++++++++++ 8 files changed, 385 insertions(+), 33 deletions(-) create mode 100644 internal/target/diagnostic_test.go diff --git a/internal/dockerclient/dockerclient.go b/internal/dockerclient/dockerclient.go index 43409bc..2e4a997 100644 --- a/internal/dockerclient/dockerclient.go +++ b/internal/dockerclient/dockerclient.go @@ -188,9 +188,12 @@ func (a *Adapter) DiagnosticChecks(_ context.Context, _ *profile.Profile) []targ } else if proxies, _, err := childObject(config, "proxies"); err != nil { schemaCheck.OK = false schemaCheck.Evidence = profile.RedactText(err.Error()) - } else if _, _, err := childObject(proxies, "default"); err != nil { + } else if defaults, _, err := childObject(proxies, "default"); err != nil { schemaCheck.OK = false schemaCheck.Evidence = profile.RedactText(err.Error()) + } else if err := validateManagedProxyStrings(defaults); err != nil { + schemaCheck.OK = false + schemaCheck.Evidence = err.Error() } else if _, _, err := a.readRecovery(); err != nil && !errors.Is(err, fs.ErrNotExist) { schemaCheck.OK = false schemaCheck.Evidence = profile.RedactText("Docker client recovery metadata is invalid or unreadable: " + err.Error()) @@ -305,6 +308,9 @@ func (a *Adapter) enable(active *profile.Profile) error { if err != nil { return err } + if err := validateManagedProxyStrings(defaults); err != nil { + return err + } if !managed { metadata = recovery{ConfigPresent: configPresent, ProxiesPresent: proxiesPresent, DefaultPresent: defaultPresent, Original: make(map[string]priorValue)} for _, key := range managedKeys { @@ -463,6 +469,20 @@ func childObject(parent map[string]json.RawMessage, key string) (map[string]json object, err := decodeObject(raw, "Docker client "+key+" setting") return object, true, err } +func validateManagedProxyStrings(defaults map[string]json.RawMessage) error { + for _, key := range managedKeys { + raw, present := defaults[key] + if !present { + continue + } + var value string + if err := json.Unmarshal(raw, &value); err != nil { + return fmt.Errorf("Docker client proxies.default.%s must be a JSON string", key) + } + } + return nil +} + func setChildObject(parent map[string]json.RawMessage, key string, child map[string]json.RawMessage) { data, _ := json.Marshal(child) parent[key] = data diff --git a/internal/dockerclient/dockerclient_test.go b/internal/dockerclient/dockerclient_test.go index b9f1f06..2268713 100644 --- a/internal/dockerclient/dockerclient_test.go +++ b/internal/dockerclient/dockerclient_test.go @@ -67,6 +67,43 @@ func TestDiagnosticChecksCoverSchemaAndAuthorityWithoutMutation(t *testing.T) { } } +func TestDiagnosticsAndEnableRejectMalformedManagedProxyTypesWithoutMutation(t *testing.T) { + for _, document := range []string{ + `{"proxies":[]}`, + `{"proxies":{"default":[]}}`, + `{"proxies":{"default":{"httpProxy":42}}}`, + `{"proxies":{"default":{"httpsProxy":false}}}`, + `{"proxies":{"default":{"allProxy":{}}}}`, + `{"proxies":{"default":{"noProxy":[]}}}`, + } { + t.Run(document, func(t *testing.T) { + files := &countingFS{FileSystem: dockerclient.OSFileSystem{}} + a := newAdapterWithFS(t, files) + writeConfig(t, a.ConfigPath(), document) + before, err := os.ReadFile(a.ConfigPath()) + if err != nil { + t.Fatal(err) + } + checks := a.DiagnosticChecks(context.Background(), activeProfile()) + if checks[1].OK { + t.Fatalf("malformed schema passed diagnostics: %#v", checks) + } + writesBefore := files.writes + result := a.SetDesired(context.Background(), true, activeProfile()) + if result.Err == nil { + t.Fatalf("enable accepted malformed schema: %#v", result) + } + after, err := os.ReadFile(a.ConfigPath()) + if err != nil { + t.Fatal(err) + } + if string(after) != string(before) || files.writes != writesBefore { + t.Fatalf("malformed config was modified: writes %d -> %d", writesBefore, files.writes) + } + }) + } +} + func TestDockerClientAdapterRunsReusableTargetContract(t *testing.T) { targettest.Run(t, func(t *testing.T) targettest.Fixture { a := newAdapter(t, nil) diff --git a/internal/dockerdaemon/dockerdaemon.go b/internal/dockerdaemon/dockerdaemon.go index ddd9496..c5471a4 100644 --- a/internal/dockerdaemon/dockerdaemon.go +++ b/internal/dockerdaemon/dockerdaemon.go @@ -68,15 +68,24 @@ func (ExecRunner) Run(ctx context.Context, name string, args ...string) ([]byte, return output, nil } +type ProcessAuthority interface { + EffectiveUID() int +} + +type processAuthority struct{} + +func (processAuthority) EffectiveUID() int { return os.Geteuid() } + type Adapter struct { configuration Configuration files FileSystem runner CommandRunner + authority ProcessAuthority } var _ target.Diagnosable = (*Adapter)(nil) -func New(configuration Configuration, files FileSystem, runner CommandRunner) *Adapter { +func New(configuration Configuration, files FileSystem, runner CommandRunner, authorities ...ProcessAuthority) *Adapter { if configuration.DaemonConfigPath == "" { configuration.DaemonConfigPath = "/etc/docker/daemon.json" } @@ -90,7 +99,11 @@ func New(configuration Configuration, files FileSystem, runner CommandRunner) *A if runner == nil { runner = ExecRunner{} } - return &Adapter{configuration: configuration, files: files, runner: runner} + authority := ProcessAuthority(processAuthority{}) + if len(authorities) != 0 && authorities[0] != nil { + authority = authorities[0] + } + return &Adapter{configuration: configuration, files: files, runner: runner, authority: authority} } func (a *Adapter) ID() string { return "docker-daemon" } @@ -119,9 +132,9 @@ func (a *Adapter) DiagnosticChecks(ctx context.Context, _ *profile.Profile) []ta } schemaCheck := target.Check{Name: "configuration schema", OK: true, Evidence: "Docker daemon configuration and recovery metadata are absent or valid JSON"} if data, err := a.files.ReadFile(a.configuration.DaemonConfigPath); err == nil { - if _, schemaErr := mergeConfig(data, profile.Profile{}); schemaErr != nil { + if schemaErr := validateConfigSchema(data); schemaErr != nil { schemaCheck.OK = false - schemaCheck.Evidence = profile.RedactText("Docker daemon configuration cannot be safely merged: " + schemaErr.Error()) + schemaCheck.Evidence = profile.RedactText("Docker daemon configuration schema is invalid: " + schemaErr.Error()) } } else if !errors.Is(err, fs.ErrNotExist) { schemaCheck.OK = false @@ -133,10 +146,17 @@ func (a *Adapter) DiagnosticChecks(ctx context.Context, _ *profile.Profile) []ta schemaCheck.Evidence = profile.RedactText("Docker daemon recovery metadata is invalid or unreadable: " + err.Error()) } } + serviceAuthority := target.Check{Name: "service control authority", OK: a.authority.EffectiveUID() == 0} + if serviceAuthority.OK { + serviceAuthority.Evidence = "effective process authority can control the system Docker service" + } else { + serviceAuthority.Evidence = "effective process authority cannot reload or restart the system Docker service; run proxyctl explicitly with sufficient authority" + } return []target.Check{ commandCheck, schemaCheck, target.ReadOnlyWriteAuthority(a.configuration.DaemonConfigPath, a.configuration.RecoveryPath), + serviceAuthority, } } @@ -366,16 +386,45 @@ func (a *Adapter) writeRecovery(value recovery) error { return a.files.AtomicWrite(a.configuration.RecoveryPath, append(data, '\n'), 0o600) } -func mergeConfig(data []byte, p profile.Profile) ([]byte, error) { +func validateConfigSchema(data []byte) error { + if len(strings.TrimSpace(string(data))) == 0 { + return errors.New("configuration must be a JSON object") + } var document map[string]any + if err := json.Unmarshal(data, &document); err != nil { + return err + } + if document == nil { + return errors.New("configuration must be a JSON object") + } + existing, present := document["proxies"] + if !present { + return nil + } + proxies, ok := existing.(map[string]any) + if !ok { + return errors.New("the existing proxies field is not an object") + } + for _, key := range []string{"http-proxy", "https-proxy", "no-proxy"} { + if value, present := proxies[key]; present { + if _, ok := value.(string); !ok { + return fmt.Errorf("the existing proxies.%s field is not a string", key) + } + } + } + return nil +} + +func mergeConfig(data []byte, p profile.Profile) ([]byte, error) { if len(strings.TrimSpace(string(data))) == 0 { data = []byte("{}") } - if err := json.Unmarshal(data, &document); err != nil { + if err := validateConfigSchema(data); err != nil { return nil, err } - if document == nil { - document = map[string]any{} + var document map[string]any + if err := json.Unmarshal(data, &document); err != nil { + return nil, err } proxies := map[string]any{} if existing, ok := document["proxies"].(map[string]any); ok { diff --git a/internal/dockerdaemon/dockerdaemon_test.go b/internal/dockerdaemon/dockerdaemon_test.go index 4f5d631..94af2ba 100644 --- a/internal/dockerdaemon/dockerdaemon_test.go +++ b/internal/dockerdaemon/dockerdaemon_test.go @@ -102,10 +102,15 @@ func (f *faultFS) AtomicWrite(path string, data []byte, mode os.FileMode) error return f.recordingFS.AtomicWrite(path, data, mode) } +type fixedAuthority int + +func (a fixedAuthority) EffectiveUID() int { return int(a) } + func TestDiagnosticChecksCoverSystemdSchemaAndAuthorityWithoutMutation(t *testing.T) { - a, record, daemonPath, recoveryPath, _ := fixture(t) + a, record, daemonPath, recoveryPath, runner := fixture(t) + a = dockerdaemon.New(dockerdaemon.Configuration{DaemonConfigPath: daemonPath, RecoveryPath: recoveryPath, TrustedRoot: filepath.Dir(filepath.Dir(daemonPath))}, recordingFS{recorder: record}, runner, fixedAuthority(0)) checks := a.DiagnosticChecks(context.Background(), activeProfile()) - for _, name := range []string{"required command and service", "configuration schema", "write authority"} { + for _, name := range []string{"required command and service", "configuration schema", "write authority", "service control authority"} { found := false for _, check := range checks { found = found || check.Name == name @@ -126,6 +131,75 @@ func TestDiagnosticChecksCoverSystemdSchemaAndAuthorityWithoutMutation(t *testin } } +func TestDiagnosticServiceControlAuthorityUsesInjectedEUIDWithoutMutation(t *testing.T) { + for _, test := range []struct { + name string + uid int + ok bool + }{ + {name: "unprivileged", uid: 1000, ok: false}, + {name: "root", uid: 0, ok: true}, + } { + t.Run(test.name, func(t *testing.T) { + _, record, daemonPath, recoveryPath, runner := fixture(t) + a := dockerdaemon.New(dockerdaemon.Configuration{DaemonConfigPath: daemonPath, RecoveryPath: recoveryPath}, recordingFS{recorder: record}, runner, fixedAuthority(test.uid)) + checks := a.DiagnosticChecks(context.Background(), activeProfile()) + var found *target.Check + for i := range checks { + if checks[i].Name == "service control authority" { + found = &checks[i] + } + } + if found == nil || found.OK != test.ok { + t.Fatalf("service authority check = %#v", found) + } + for _, event := range record.events { + if strings.Contains(event, "restart") || strings.Contains(event, "daemon-reload") { + t.Fatalf("authority diagnosis mutated service: %#v", record.events) + } + } + }) + } +} + +func TestDiagnosticsAndEnableRejectMalformedDaemonProxyTypesWithoutMutation(t *testing.T) { + for _, document := range []string{ + `[]`, + `{"proxies":[]}`, + `{"proxies":{"http-proxy":42}}`, + `{"proxies":{"https-proxy":false}}`, + `{"proxies":{"no-proxy":[]}}`, + } { + t.Run(document, func(t *testing.T) { + a, record, daemonPath, _, _ := fixture(t) + if err := os.MkdirAll(filepath.Dir(daemonPath), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(daemonPath, []byte(document), 0o600); err != nil { + t.Fatal(err) + } + checks := a.DiagnosticChecks(context.Background(), activeProfile()) + if checks[1].OK { + t.Fatalf("malformed schema passed diagnostics: %#v", checks) + } + record.events = nil + result := a.SetDesired(context.Background(), true, activeProfile()) + if result.Err == nil { + t.Fatalf("enable accepted malformed schema: %#v", result) + } + for _, event := range record.events { + if strings.HasPrefix(event, "write ") || strings.Contains(event, "restart") || strings.Contains(event, "daemon-reload") { + t.Fatalf("malformed schema caused mutation: %#v", record.events) + } + } + got, err := os.ReadFile(daemonPath) + if err != nil || string(got) != document { + t.Fatalf("daemon config changed: %q, %v", got, err) + } + }) + } +} + func TestDockerDaemonRunsReusableTargetContract(t *testing.T) { targettest.Run(t, func(t *testing.T) targettest.Fixture { a, _, daemonPath, _, _ := fixture(t) diff --git a/internal/gnome/gnome.go b/internal/gnome/gnome.go index 085c424..297faa8 100644 --- a/internal/gnome/gnome.go +++ b/internal/gnome/gnome.go @@ -535,7 +535,7 @@ func (a *Adapter) readSettings(ctx context.Context) (settings, error) { if result.HTTPAuthUser, err = a.getString(ctx, httpSchema, "authentication-user"); err != nil { return settings{}, err } - if result.HTTPAuthPassword, err = a.getString(ctx, httpSchema, "authentication-password"); err != nil { + if result.HTTPAuthPassword, err = a.getSensitiveString(ctx, httpSchema, "authentication-password"); err != nil { return settings{}, err } if result.HTTPSHost, err = a.getString(ctx, httpsSchema, "host"); err != nil { @@ -560,6 +560,22 @@ func (a *Adapter) getString(ctx context.Context, schema, key string) (string, er } return parseString(raw) } + +// getSensitiveString deliberately suppresses both command error detail and raw +// output. A password value is secret even when malformed, and command runners +// are not trusted to keep that value out of their errors. +func (a *Adapter) getSensitiveString(ctx context.Context, schema, key string) (string, error) { + raw, err := a.run(ctx, "get", schema, key) + if err != nil { + return "", safeRedaction(fmt.Sprintf("get %s %s failed; sensitive output suppressed", schema, key), err) + } + value, err := parseString(strings.TrimSpace(raw)) + if err != nil { + return "", errors.New("parse " + schema + " " + key + " failed; sensitive output suppressed") + } + return value, nil +} + func (a *Adapter) getInt(ctx context.Context, schema, key string) (int, error) { raw, err := a.get(ctx, schema, key) if err != nil { diff --git a/internal/gnome/gnome_test.go b/internal/gnome/gnome_test.go index 526b2e7..8638e2f 100644 --- a/internal/gnome/gnome_test.go +++ b/internal/gnome/gnome_test.go @@ -108,6 +108,49 @@ func active() *profile.Profile { return &profile.Profile{Name: "office", Mode: profile.AdvancedMode, HTTPURL: "http://proxy.example:8080", HTTPSURL: "http://secure.example:8443", AllURL: "socks5://socks.example:1080", Bypass: []string{"localhost", ".example.com", "10.0.0.0/8", "2001:db8::1"}} } +func TestDiagnosticsSuppressSensitiveGSettingsOutputAndRunnerErrors(t *testing.T) { + const secret = "GNOME-DIAGNOSTIC-SECRET-7f3a" + for _, test := range []struct { + name string + value string + fail bool + }{ + {name: "malformed value", value: "unterminated-" + secret}, + {name: "runner error", value: secret, fail: true}, + } { + t.Run(test.name, func(t *testing.T) { + r := newRunner() + key := "org.gnome.system.proxy.http authentication-password" + r.values[key] = test.value + if test.fail { + r.fail = func(c gnome.Command) error { + if reflect.DeepEqual(c.Args, []string{"get", "org.gnome.system.proxy.http", "authentication-password"}) { + return errors.New("runner exposed " + secret) + } + return nil + } + } + a := adapter(r, &memoryState{}) + checks := a.DiagnosticChecks(context.Background(), active()) + for _, check := range checks { + if strings.Contains(check.Evidence, secret) { + t.Fatalf("diagnostic evidence exposed password: %#v", checks) + } + } + result := a.SetDesired(context.Background(), true, active()) + operationError := "" + if result.Err != nil { + operationError = result.Err.Error() + } + for _, evidence := range []string{a.Availability(context.Background()).Evidence, a.Inspect(context.Background(), active()).Evidence, operationError} { + if strings.Contains(evidence, secret) { + t.Fatalf("Target evidence or loggable operation error exposed password: %q", evidence) + } + } + }) + } +} + func TestDiagnosticChecksCoverCommandSchemasSessionAndAuthorityWithoutMutation(t *testing.T) { r, state := newRunner(), &memoryState{} a := adapter(r, state) diff --git a/internal/target/diagnostic.go b/internal/target/diagnostic.go index 9c39b06..4f8e622 100644 --- a/internal/target/diagnostic.go +++ b/internal/target/diagnostic.go @@ -3,7 +3,6 @@ package target import ( "errors" "io/fs" - "os" "path/filepath" "strings" @@ -26,26 +25,66 @@ func ReadOnlyWriteAuthority(paths ...string) Check { return check } +// readOnlyWritablePath anchors traversal at an open root descriptor and refuses +// symbolic links in every component. The authority observation therefore +// cannot be redirected to a victim between path lookup steps, and it never +// opens victim content or mutates filesystem metadata. func readOnlyWritablePath(path string) (bool, string) { - candidate := filepath.Clean(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 + absolute, err := filepath.Abs(path) + if err != nil { + return false, path + " cannot be inspected: " + err.Error() } + absolute = filepath.Clean(absolute) + components := strings.Split(strings.TrimPrefix(absolute, string(filepath.Separator)), string(filepath.Separator)) + if len(components) == 1 && components[0] == "" { + components = nil + } + + current, err := unix.Open(string(filepath.Separator), unix.O_PATH|unix.O_DIRECTORY|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0) + if err != nil { + return false, absolute + " cannot be inspected: " + err.Error() + } + defer func() { _ = unix.Close(current) }() + currentPath := string(filepath.Separator) + + for index, component := range components { + child, openErr := unix.Openat(current, component, unix.O_PATH|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0) + if errors.Is(openErr, fs.ErrNotExist) { + return writableDirectory(current, currentPath, absolute) + } + if openErr != nil { + return false, filepath.Join(currentPath, component) + " cannot be inspected without following links: " + openErr.Error() + } + var stat unix.Stat_t + statErr := unix.Fstat(child, &stat) + if statErr != nil { + _ = unix.Close(child) + return false, filepath.Join(currentPath, component) + " cannot be inspected: " + statErr.Error() + } + kind := stat.Mode & unix.S_IFMT + componentPath := filepath.Join(currentPath, component) + if kind == unix.S_IFLNK { + _ = unix.Close(child) + return false, componentPath + " is a symbolic link and is unavailable for safe write observation" + } + if index == len(components)-1 { + _ = unix.Close(child) + return writableDirectory(current, currentPath, absolute) + } + if kind != unix.S_IFDIR { + _ = unix.Close(child) + return false, componentPath + " is not a directory" + } + _ = unix.Close(current) + current = child + currentPath = componentPath + } + return writableDirectory(current, currentPath, absolute) +} + +func writableDirectory(descriptor int, directory, targetPath string) (bool, string) { + if err := unix.Faccessat(descriptor, ".", unix.W_OK|unix.X_OK, unix.AT_EACCESS); err != nil { + return false, directory + " is not writable for " + targetPath + ": " + err.Error() + } + return true, directory + " is writable for " + targetPath } diff --git a/internal/target/diagnostic_test.go b/internal/target/diagnostic_test.go new file mode 100644 index 0000000..4a93cd5 --- /dev/null +++ b/internal/target/diagnostic_test.go @@ -0,0 +1,74 @@ +package target_test + +import ( + "os" + "path/filepath" + "reflect" + "strings" + "testing" + + "proxyctl/internal/target" +) + +func TestReadOnlyWriteAuthorityRejectsSymlinkedFinalAndAncestorWithoutTouchingVictim(t *testing.T) { + for _, test := range []struct { + name string + path func(root, victim string) string + }{ + { + name: "final path", + path: func(root, victim string) string { + link := filepath.Join(root, "managed.json") + if err := os.Symlink(victim, link); err != nil { + t.Fatal(err) + } + return link + }, + }, + { + name: "ancestor path", + path: func(root, victim string) string { + victimDir := filepath.Dir(victim) + link := filepath.Join(root, "managed") + if err := os.Symlink(victimDir, link); err != nil { + t.Fatal(err) + } + return filepath.Join(link, filepath.Base(victim)) + }, + }, + } { + t.Run(test.name, func(t *testing.T) { + root := t.TempDir() + victimDir := t.TempDir() + victim := filepath.Join(victimDir, "victim.json") + const content = "VICTIM-CONTENT-MUST-STAY" + if err := os.WriteFile(victim, []byte(content), 0o640); err != nil { + t.Fatal(err) + } + beforeContent, err := os.ReadFile(victim) + if err != nil { + t.Fatal(err) + } + before, err := os.Stat(victim) + if err != nil { + t.Fatal(err) + } + + check := target.ReadOnlyWriteAuthority(test.path(root, victim)) + if check.OK || (!strings.Contains(check.Evidence, "symbolic link") && !strings.Contains(check.Evidence, "symlink")) { + t.Fatalf("symlink path reported writable: %#v", check) + } + after, err := os.Stat(victim) + if err != nil { + t.Fatal(err) + } + data, err := os.ReadFile(victim) + if err != nil { + t.Fatal(err) + } + if string(beforeContent) != content || string(data) != content || !reflect.DeepEqual(before.Sys(), after.Sys()) { + t.Fatalf("victim changed: before=%#v after=%#v content=%q", before, after, data) + } + }) + } +} From 613fe565404c30b08c1d28ebaa29e7364378069e Mon Sep 17 00:00:00 2001 From: proxyctl coordinator Date: Fri, 17 Jul 2026 10:59:14 +0800 Subject: [PATCH 3/5] fix: harden target recovery metadata safety --- cmd/proxyctl/maintenance_test.go | 89 ++++++++++++++++++++++ internal/dockerdaemon/dockerdaemon.go | 56 +++++++++++++- internal/dockerdaemon/dockerdaemon_test.go | 56 ++++++++++++++ internal/gnome/gnome.go | 69 +++++++++++++++-- internal/gnome/gnome_test.go | 85 +++++++++++++++++++++ internal/shell/shell.go | 54 ++++++++++++- internal/shell/shell_test.go | 37 +++++++++ 7 files changed, 436 insertions(+), 10 deletions(-) diff --git a/cmd/proxyctl/maintenance_test.go b/cmd/proxyctl/maintenance_test.go index a10bf71..a2ec729 100644 --- a/cmd/proxyctl/maintenance_test.go +++ b/cmd/proxyctl/maintenance_test.go @@ -4,12 +4,14 @@ import ( "bytes" "context" "errors" + "fmt" "os" "os/exec" "path/filepath" "strings" "testing" + "proxyctl/internal/gnome" "proxyctl/internal/operationlock" "proxyctl/internal/oplog" "proxyctl/internal/profile" @@ -206,6 +208,93 @@ func isolatedProcessEnvironment(root string) []string { ) } +type passwordEchoRunner struct { + values map[string]string + failPasswordSet bool +} + +func newPasswordEchoRunner(savedPassword string) *passwordEchoRunner { + return &passwordEchoRunner{values: map[string]string{ + "org.gnome.system.proxy mode": "'auto'", "org.gnome.system.proxy autoconfig-url": "''", "org.gnome.system.proxy ignore-hosts": "[]", + "org.gnome.system.proxy.http host": "''", "org.gnome.system.proxy.http port": "0", "org.gnome.system.proxy.http use-authentication": "false", + "org.gnome.system.proxy.http authentication-user": "''", "org.gnome.system.proxy.http authentication-password": "'" + savedPassword + "'", + "org.gnome.system.proxy.https host": "''", "org.gnome.system.proxy.https port": "0", "org.gnome.system.proxy.socks host": "''", "org.gnome.system.proxy.socks port": "0", + }} +} + +func (r *passwordEchoRunner) Run(_ context.Context, command gnome.Command) (string, error) { + if len(command.Args) == 1 && command.Args[0] == "list-schemas" { + return "org.gnome.system.proxy\norg.gnome.system.proxy.http\norg.gnome.system.proxy.https\norg.gnome.system.proxy.socks\n", nil + } + if len(command.Args) == 3 && command.Args[0] == "get" { + return r.values[command.Args[1]+" "+command.Args[2]] + "\n", nil + } + if len(command.Args) == 4 && command.Args[0] == "set" { + if r.failPasswordSet && command.Args[2] == "authentication-password" { + return "", fmt.Errorf("runner echoed saved argument %s: %w", command.Args[3], os.ErrPermission) + } + r.values[command.Args[1]+" "+command.Args[2]] = command.Args[3] + return "", nil + } + return "", fmt.Errorf("unexpected command: %v", command.Args) +} + +func passwordRestoreFixture(t *testing.T, secret string) (*gnome.Adapter, *passwordEchoRunner) { + t.Helper() + root := t.TempDir() + runner := newPasswordEchoRunner(secret) + statePath := filepath.Join(root, "state", "gnome.json") + adapter := gnome.New(gnome.Configuration{CommandName: "gsettings", UID: os.Getuid(), GID: os.Getgid(), HomeDir: root, StateFile: statePath}, runner, gnome.NewFileState(statePath, -1, -1, root)) + active := &profile.Profile{Name: "office", Mode: profile.AdvancedMode, HTTPURL: "http://proxy.example:8080", Bypass: []string{"localhost"}} + if result := adapter.SetDesired(context.Background(), true, active); result.Err != nil || result.Inspection.State != target.Enabled { + t.Fatalf("prepare managed GNOME Target: %#v", result) + } + runner.failPasswordSet = true + return adapter, runner +} + +func TestResetSuppressesSavedGNOMEPasswordFromOperationCLIAndLogsWithoutActiveProfile(t *testing.T) { + const secret = "GNOME-ORIGINAL-PASSWORD-FULL-CHAIN-07" + + direct, _ := passwordRestoreFixture(t, secret) + operation := direct.SetDesired(context.Background(), false, nil) + if operation.Err == nil { + t.Fatal("password restore unexpectedly succeeded") + } + if !errors.Is(operation.Err, os.ErrPermission) { + t.Fatalf("OperationResult lost safe permission classification: %v", operation.Err) + } + if rendered := fmt.Sprint(operation); strings.Contains(rendered, secret) || strings.Contains(rendered, "runner echoed") { + t.Fatalf("unsafe OperationResult: %s", rendered) + } + + managed, _ := passwordRestoreFixture(t, secret) + root := t.TempDir() + config := filepath.Join(root, "config", "profiles.json") + writeConfig(t, config, `{"profiles":[]}`) + store := profile.NewFileStore(config) + stateDir := filepath.Join(root, "state") + lock := operationlock.New(filepath.Join(stateDir, "operation.lock")) + logger := oplog.New(filepath.Join(stateDir, "operations.log"), 1024, 1) + var output bytes.Buffer + err := runMaintenanceCommand([]string{"reset"}, strings.NewReader("yes\n"), &output, store, []target.Target{managed}, lock, logger, nil) + if err == nil { + t.Fatal("reset unexpectedly succeeded") + } + logData, readErr := os.ReadFile(filepath.Join(stateDir, "operations.log")) + if readErr != nil { + t.Fatal(readErr) + } + for surface, value := range map[string]string{"CLI output": output.String(), "returned error": err.Error(), "operation log": string(logData)} { + if strings.Contains(value, secret) || strings.Contains(value, "runner echoed") { + t.Fatalf("%s exposed saved password or raw runner error: %q", surface, value) + } + if !strings.Contains(value, "sensitive argument and runner error suppressed") { + t.Fatalf("%s lacks fixed safe failure message: %q", surface, value) + } + } +} + func TestResetCommandCancellationAndTruthfulPartialFailureWithFakeTargets(t *testing.T) { root := t.TempDir() config := filepath.Join(root, "xdg-config", "proxyctl", "profiles.json") diff --git a/internal/dockerdaemon/dockerdaemon.go b/internal/dockerdaemon/dockerdaemon.go index c5471a4..ea84e18 100644 --- a/internal/dockerdaemon/dockerdaemon.go +++ b/internal/dockerdaemon/dockerdaemon.go @@ -20,7 +20,10 @@ import ( "proxyctl/internal/target" ) -const service = "docker.service" +const ( + service = "docker.service" + recoveryVersion = 1 +) type Configuration struct { DaemonConfigPath string @@ -188,9 +191,10 @@ func parseProperties(output string) map[string]string { } type recovery struct { + Version int `json:"version"` Original []byte `json:"original,omitempty"` OriginalExists bool `json:"originalExists"` - OriginalMode uint32 `json:"originalMode,omitempty"` + OriginalMode uint32 `json:"originalMode"` ConfigFingerprint string `json:"configFingerprint"` RestartedFingerprint string `json:"restartedFingerprint,omitempty"` } @@ -277,6 +281,7 @@ func (a *Adapter) enable(ctx context.Context, p profile.Profile) error { return fmt.Errorf("read Docker daemon configuration: %w", readErr) } if !managed { + metadata.Version = recoveryVersion metadata.OriginalExists = readErr == nil if metadata.OriginalExists { metadata.Original = append([]byte(nil), current...) @@ -372,12 +377,59 @@ func (a *Adapter) readRecovery() (recovery, bool, error) { if err != nil { return recovery{}, false, err } + var fields map[string]json.RawMessage + if err := json.Unmarshal(data, &fields); err != nil { + return recovery{}, true, err + } + if fields == nil { + return recovery{}, true, errors.New("Docker daemon recovery metadata must be a JSON object") + } var value recovery if err := json.Unmarshal(data, &value); err != nil { return recovery{}, true, err } + if err := validateRecovery(value, fields); err != nil { + return recovery{}, true, err + } return value, true, nil } + +func validateRecovery(value recovery, fields map[string]json.RawMessage) error { + for _, name := range []string{"version", "originalExists", "originalMode", "configFingerprint"} { + if _, present := fields[name]; !present { + return fmt.Errorf("Docker daemon recovery metadata is missing required field %q", name) + } + } + if value.Version != recoveryVersion { + return fmt.Errorf("unsupported Docker daemon recovery metadata version %d", value.Version) + } + if !validFingerprint(value.ConfigFingerprint) { + return errors.New("Docker daemon recovery metadata has an invalid configuration fingerprint") + } + if value.RestartedFingerprint != "" && (value.RestartedFingerprint != value.ConfigFingerprint || !validFingerprint(value.RestartedFingerprint)) { + return errors.New("Docker daemon recovery metadata has an inconsistent restarted fingerprint") + } + _, hasOriginal := fields["original"] + if value.OriginalExists { + if !hasOriginal { + return errors.New("Docker daemon recovery metadata is missing the recorded original configuration or mode") + } + if value.OriginalMode > 0o777 { + return errors.New("Docker daemon recovery metadata has an invalid original file mode") + } + if err := validateConfigSchema(value.Original); err != nil { + return fmt.Errorf("Docker daemon recovery metadata has an invalid original configuration: %w", err) + } + } else if hasOriginal || len(value.Original) != 0 || value.OriginalMode != 0 { + return errors.New("Docker daemon recovery metadata records original data although the original did not exist") + } + return nil +} + +func validFingerprint(value string) bool { + decoded, err := hex.DecodeString(value) + return err == nil && len(decoded) == sha256.Size +} func (a *Adapter) writeRecovery(value recovery) error { data, err := json.MarshalIndent(value, "", " ") if err != nil { diff --git a/internal/dockerdaemon/dockerdaemon_test.go b/internal/dockerdaemon/dockerdaemon_test.go index 94af2ba..f07448a 100644 --- a/internal/dockerdaemon/dockerdaemon_test.go +++ b/internal/dockerdaemon/dockerdaemon_test.go @@ -11,6 +11,7 @@ import ( "strings" "testing" + "proxyctl/internal/app" "proxyctl/internal/dockerdaemon" "proxyctl/internal/profile" "proxyctl/internal/target" @@ -106,6 +107,61 @@ type fixedAuthority int func (a fixedAuthority) EffectiveUID() int { return int(a) } +type immediateLock struct{} + +func (immediateLock) Lock(context.Context) (func(), error) { return func() {}, nil } + +func TestUnsafeRecoveryMetadataIsDriftedAndResetNeverMutatesDaemon(t *testing.T) { + fingerprint := strings.Repeat("a", 64) + cases := map[string]string{ + "empty object": `{}`, + "truncated": `{"version":1`, + "unsupported version": `{"version":2,"originalExists":false,"configFingerprint":"` + fingerprint + `"}`, + "missing original": `{"version":1,"originalExists":true,"originalMode":384,"configFingerprint":"` + fingerprint + `"}`, + "unexpected original": `{"version":1,"originalExists":false,"original":"e30=","configFingerprint":"` + fingerprint + `"}`, + "mode without original": `{"version":1,"originalExists":false,"originalMode":384,"configFingerprint":"` + fingerprint + `"}`, + "invalid mode": `{"version":1,"originalExists":true,"original":"e30=","originalMode":4096,"configFingerprint":"` + fingerprint + `"}`, + "invalid fingerprint": `{"version":1,"originalExists":false,"configFingerprint":"short"}`, + "contradictory restart record": `{"version":1,"originalExists":false,"configFingerprint":"` + fingerprint + `","restartedFingerprint":"` + strings.Repeat("b", 64) + `"}`, + } + for name, metadata := range cases { + t.Run(name, func(t *testing.T) { + a, record, daemonPath, recoveryPath, _ := fixture(t) + originalConfig := []byte(`{"log-level":"debug"}`) + if err := os.MkdirAll(filepath.Dir(daemonPath), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(daemonPath, originalConfig, 0o640); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Dir(recoveryPath), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(recoveryPath, []byte(metadata), 0o600); err != nil { + t.Fatal(err) + } + record.events = nil + + if inspection := a.Inspect(context.Background(), nil); inspection.State != target.Drifted { + t.Fatalf("inspection = %#v", inspection) + } + reset := app.NewMaintenance(profile.State{}, []target.Target{a}, immediateLock{}, nil).Reset(context.Background(), true) + if reset.Err == nil || len(reset.Targets) != 1 || reset.Targets[0].Err == nil { + t.Fatalf("unsafe reset = %#v", reset) + } + if events := mutationEvents(record.events); len(events) != 0 { + t.Fatalf("unsafe metadata caused mutation: %#v", events) + } + if got, err := os.ReadFile(daemonPath); err != nil || !reflect.DeepEqual(got, originalConfig) { + t.Fatalf("daemon changed to %q: %v", got, err) + } + if got, err := os.ReadFile(recoveryPath); err != nil || string(got) != metadata { + t.Fatalf("recovery changed to %q: %v", got, err) + } + }) + } +} + func TestDiagnosticChecksCoverSystemdSchemaAndAuthorityWithoutMutation(t *testing.T) { a, record, daemonPath, recoveryPath, runner := fixture(t) a = dockerdaemon.New(dockerdaemon.Configuration{DaemonConfigPath: daemonPath, RecoveryPath: recoveryPath, TrustedRoot: filepath.Dir(filepath.Dir(daemonPath))}, recordingFS{recorder: record}, runner, fixedAuthority(0)) diff --git a/internal/gnome/gnome.go b/internal/gnome/gnome.go index 297faa8..dab8d96 100644 --- a/internal/gnome/gnome.go +++ b/internal/gnome/gnome.go @@ -26,10 +26,11 @@ import ( ) const ( - proxySchema = "org.gnome.system.proxy" - httpSchema = proxySchema + ".http" - httpsSchema = proxySchema + ".https" - socksSchema = proxySchema + ".socks" + gnomeRecoveryVersion = 1 + proxySchema = "org.gnome.system.proxy" + httpSchema = proxySchema + ".http" + httpsSchema = proxySchema + ".https" + socksSchema = proxySchema + ".socks" ) var requiredSchemas = []string{proxySchema, httpSchema, httpsSchema, socksSchema} @@ -226,6 +227,7 @@ type settings struct { } type recovery struct { + Version int `json:"version"` Original settings `json:"original"` LastFingerprint string `json:"lastFingerprint"` } @@ -421,6 +423,7 @@ func (a *Adapter) enable(ctx context.Context, active profile.Profile) error { return fmt.Errorf("read GNOME recovery metadata: %w", err) } if !managed { + metadata.Version = gnomeRecoveryVersion original, err := a.readSettings(ctx) if err != nil { return fmt.Errorf("snapshot original GNOME settings: %w", err) @@ -480,13 +483,59 @@ func (a *Adapter) readRecovery() (recovery, bool, error) { if err != nil { return recovery{}, false, err } + var fields map[string]json.RawMessage + if err := json.Unmarshal(data, &fields); err != nil { + return recovery{}, true, err + } + if fields == nil { + return recovery{}, true, errors.New("GNOME recovery metadata must be a JSON object") + } + for _, name := range []string{"version", "original", "lastFingerprint"} { + if _, present := fields[name]; !present { + return recovery{}, true, fmt.Errorf("GNOME recovery metadata is missing required field %q", name) + } + } + var originalFields map[string]json.RawMessage + if err := json.Unmarshal(fields["original"], &originalFields); err != nil || originalFields == nil { + return recovery{}, true, errors.New("GNOME recovery metadata original settings must be a JSON object") + } + for _, name := range []string{"mode", "autoConfigUrl", "ignoreHosts", "httpHost", "httpPort", "httpUseAuthentication", "httpAuthenticationUser", "httpAuthenticationPassword", "httpsHost", "httpsPort", "socksHost", "socksPort"} { + if _, present := originalFields[name]; !present { + return recovery{}, true, fmt.Errorf("GNOME recovery metadata original settings are missing required field %q", name) + } + } var value recovery if err := json.Unmarshal(data, &value); err != nil { return recovery{}, true, err } + if err := validateRecovery(value); err != nil { + return recovery{}, true, err + } return value, true, nil } +func validateRecovery(value recovery) error { + if value.Version != gnomeRecoveryVersion { + return fmt.Errorf("unsupported GNOME recovery metadata version %d", value.Version) + } + decoded, err := hex.DecodeString(value.LastFingerprint) + if err != nil || len(decoded) != sha256.Size { + return errors.New("GNOME recovery metadata has an invalid fingerprint") + } + if value.Original.Mode != "none" && value.Original.Mode != "manual" && value.Original.Mode != "auto" { + return errors.New("GNOME recovery metadata has an invalid original proxy mode") + } + if value.Original.IgnoreHosts == nil { + return errors.New("GNOME recovery metadata has an incomplete original ignore-hosts snapshot") + } + for _, port := range []int{value.Original.HTTPPort, value.Original.HTTPSPort, value.Original.SocksPort} { + if port < 0 || port > 65535 { + return errors.New("GNOME recovery metadata has an invalid original proxy port") + } + } + return nil +} + func (a *Adapter) command(args ...string) Command { return Command{Name: a.configuration.CommandName, Args: append([]string(nil), args...), UID: a.configuration.UID, GID: a.configuration.GID, Env: map[string]string{ "HOME": a.configuration.HomeDir, "USER": a.configuration.Username, "LOGNAME": a.configuration.Username, @@ -511,6 +560,13 @@ func (a *Adapter) set(ctx context.Context, schema, key, value string) error { return nil } +func (a *Adapter) setSensitiveString(ctx context.Context, schema, key, value string) error { + if _, err := a.run(ctx, "set", schema, key, value); err != nil { + return safeRedaction("set GNOME authentication password failed; sensitive argument and runner error suppressed", err) + } + return nil +} + func (a *Adapter) readSettings(ctx context.Context) (settings, error) { var result settings var err error @@ -615,7 +671,7 @@ func (a *Adapter) writeSettings(ctx context.Context, value settings) error { {proxySchema, "autoconfig-url", quote(value.AutoConfigURL)}, {proxySchema, "ignore-hosts", quoteStrings(value.IgnoreHosts)}, {httpSchema, "host", quote(value.HTTPHost)}, {httpSchema, "port", strconv.Itoa(value.HTTPPort)}, - {httpSchema, "use-authentication", strconv.FormatBool(value.HTTPUseAuth)}, {httpSchema, "authentication-user", quote(value.HTTPAuthUser)}, {httpSchema, "authentication-password", quote(value.HTTPAuthPassword)}, + {httpSchema, "use-authentication", strconv.FormatBool(value.HTTPUseAuth)}, {httpSchema, "authentication-user", quote(value.HTTPAuthUser)}, {httpsSchema, "host", quote(value.HTTPSHost)}, {httpsSchema, "port", strconv.Itoa(value.HTTPSPort)}, {socksSchema, "host", quote(value.SocksHost)}, {socksSchema, "port", strconv.Itoa(value.SocksPort)}, } @@ -624,6 +680,9 @@ func (a *Adapter) writeSettings(ctx context.Context, value settings) error { return err } } + if err := a.setSensitiveString(ctx, httpSchema, "authentication-password", quote(value.HTTPAuthPassword)); err != nil { + return err + } return a.set(ctx, proxySchema, "mode", quote(value.Mode)) } diff --git a/internal/gnome/gnome_test.go b/internal/gnome/gnome_test.go index 8638e2f..cc05d64 100644 --- a/internal/gnome/gnome_test.go +++ b/internal/gnome/gnome_test.go @@ -108,6 +108,91 @@ func active() *profile.Profile { return &profile.Profile{Name: "office", Mode: profile.AdvancedMode, HTTPURL: "http://proxy.example:8080", HTTPSURL: "http://secure.example:8443", AllURL: "socks5://socks.example:1080", Bypass: []string{"localhost", ".example.com", "10.0.0.0/8", "2001:db8::1"}} } +func TestAuthenticationPasswordSetFailureSuppressesRunnerErrorAndProfileArgument(t *testing.T) { + const profilePassword = "GNOME-PROFILE-PASSWORD-07" + r, state := newRunner(), &memoryState{} + r.fail = func(c gnome.Command) error { + if len(c.Args) == 4 && c.Args[0] == "set" && c.Args[2] == "authentication-password" { + return fmt.Errorf("runner echoed argument %s: %w", c.Args[3], os.ErrPermission) + } + return nil + } + p := *active() + p.HTTPURL = "http://alice:" + profilePassword + "@proxy.example:8080" + result := adapter(r, state).SetDesired(context.Background(), true, &p) + if result.Err == nil || !errors.Is(result.Err, os.ErrPermission) { + t.Fatalf("set error classification = %#v", result) + } + if strings.Contains(result.Err.Error(), profilePassword) || strings.Contains(result.Err.Error(), "runner echoed") { + t.Fatalf("set exposed runner error or password argument: %v", result.Err) + } + if !strings.Contains(result.Err.Error(), "sensitive argument and runner error suppressed") { + t.Fatalf("set lacks fixed safe message: %v", result.Err) + } +} + +func TestAuthenticationPasswordRestoreFailureSuppressesRunnerErrorAndSavedArgument(t *testing.T) { + const savedPassword = "GNOME-SAVED-ORIGINAL-PASSWORD-07" + r, state := newRunner(), &memoryState{} + r.values["org.gnome.system.proxy mode"] = "'auto'" + r.values["org.gnome.system.proxy.http authentication-password"] = "'" + savedPassword + "'" + a := adapter(r, state) + if result := a.SetDesired(context.Background(), true, active()); result.Err != nil { + t.Fatalf("enable = %#v", result) + } + r.fail = func(c gnome.Command) error { + if reflect.DeepEqual(c.Args[:min(len(c.Args), 3)], []string{"set", "org.gnome.system.proxy.http", "authentication-password"}) { + return fmt.Errorf("runner echoed argument %s: %w", c.Args[3], os.ErrPermission) + } + return nil + } + result := a.SetDesired(context.Background(), false, nil) + if result.Err == nil || !errors.Is(result.Err, os.ErrPermission) { + t.Fatalf("restore error classification = %#v", result) + } + if strings.Contains(result.Err.Error(), savedPassword) || strings.Contains(result.Err.Error(), "runner echoed") { + t.Fatalf("restore exposed runner error or password argument: %v", result.Err) + } + if !strings.Contains(result.Err.Error(), "sensitive argument and runner error suppressed") { + t.Fatalf("restore lacks fixed safe message: %v", result.Err) + } +} + +func TestIncompleteRecoveryMetadataRejectsMutationAndPreservesGNOMESettings(t *testing.T) { + cases := map[string]string{ + "empty object": `{}`, + "missing original": `{"version":1,"lastFingerprint":"` + strings.Repeat("a", 64) + `"}`, + "incomplete original": `{"version":1,"lastFingerprint":"` + strings.Repeat("a", 64) + `","original":{"mode":"none"}}`, + "invalid fingerprint": `{"version":1,"lastFingerprint":"short","original":{}}`, + } + for name, metadata := range cases { + t.Run(name, func(t *testing.T) { + r, state := newRunner(), &memoryState{data: []byte(metadata)} + before := make(map[string]string, len(r.values)) + for key, value := range r.values { + before[key] = value + } + a := adapter(r, state) + if inspection := a.Inspect(context.Background(), nil); inspection.State != target.Drifted { + t.Fatalf("inspection = %#v", inspection) + } + callStart := len(r.calls) + result := a.SetDesired(context.Background(), false, nil) + if result.Err == nil || result.Inspection.State != target.Drifted { + t.Fatalf("unsafe disable = %#v", result) + } + if !reflect.DeepEqual(r.values, before) || string(state.data) != metadata { + t.Fatalf("configuration mutated: values=%#v metadata=%q", r.values, state.data) + } + for _, call := range r.calls[callStart:] { + if len(call.Args) > 0 && call.Args[0] == "set" { + t.Fatalf("unsafe metadata caused set: %#v", call) + } + } + }) + } +} + func TestDiagnosticsSuppressSensitiveGSettingsOutputAndRunnerErrors(t *testing.T) { const secret = "GNOME-DIAGNOSTIC-SECRET-7f3a" for _, test := range []struct { diff --git a/internal/shell/shell.go b/internal/shell/shell.go index 350e803..f01eb46 100644 --- a/internal/shell/shell.go +++ b/internal/shell/shell.go @@ -22,8 +22,9 @@ import ( ) const ( - integrationStart = "# >>> proxyctl shell integration >>>" - integrationEnd = "# <<< proxyctl shell integration <<<" + integrationStart = "# >>> proxyctl shell integration >>>" + integrationEnd = "# <<< proxyctl shell integration <<<" + shellRecoveryVersion = 1 ) var proxyVariables = []string{ @@ -149,7 +150,8 @@ func (f OSFileSystem) AtomicWrite(path string, data []byte, mode os.FileMode, ow } type recovery struct { - Original map[string]string `json:"original,omitempty"` + Version int `json:"version"` + Original map[string]string `json:"original"` OriginalScript *string `json:"originalScript,omitempty"` Fingerprint string `json:"fingerprint"` } @@ -288,6 +290,7 @@ func (a *Adapter) enable(active profile.Profile) error { return fmt.Errorf("read Shell recovery metadata: %w", err) } if !managed { + metadata.Version = shellRecoveryVersion if existing, readErr := a.files.ReadFile(a.configuration.EnvironmentFile); readErr == nil && recognizesProxy(existing) { original := string(existing) metadata.OriginalScript = &original @@ -344,12 +347,57 @@ func (a *Adapter) readRecovery() (recovery, bool, error) { if err != nil { return recovery{}, false, err } + var fields map[string]json.RawMessage + if err := json.Unmarshal(data, &fields); err != nil { + return recovery{}, true, err + } + if fields == nil { + return recovery{}, true, errors.New("Shell recovery metadata must be a JSON object") + } var value recovery if err := json.Unmarshal(data, &value); err != nil { return recovery{}, true, err } + if err := validateRecovery(value, fields); err != nil { + return recovery{}, true, err + } return value, true, nil } + +func validateRecovery(value recovery, fields map[string]json.RawMessage) error { + for _, name := range []string{"version", "original", "fingerprint"} { + if _, present := fields[name]; !present { + return fmt.Errorf("Shell recovery metadata is missing required field %q", name) + } + } + if value.Version != shellRecoveryVersion { + return fmt.Errorf("unsupported Shell recovery metadata version %d", value.Version) + } + decoded, err := hex.DecodeString(value.Fingerprint) + if err != nil || len(decoded) != sha256.Size { + return errors.New("Shell recovery metadata has an invalid fingerprint") + } + _, hasOriginalScript := fields["originalScript"] + if hasOriginalScript { + if value.OriginalScript == nil || value.Original != nil || len(*value.OriginalScript) == 0 || !recognizesProxy([]byte(*value.OriginalScript)) { + return errors.New("Shell recovery metadata has inconsistent original script state") + } + return nil + } + if value.Original == nil { + return errors.New("Shell recovery metadata is missing the original environment snapshot") + } + for name := range value.Original { + known := false + for _, candidate := range proxyVariables { + known = known || name == candidate + } + if !known { + return fmt.Errorf("Shell recovery metadata contains unsupported variable %q", name) + } + } + return nil +} func (a *Adapter) ownership() *Ownership { if !a.configuration.OwnFiles { return nil diff --git a/internal/shell/shell_test.go b/internal/shell/shell_test.go index bb9313e..9dfbc84 100644 --- a/internal/shell/shell_test.go +++ b/internal/shell/shell_test.go @@ -77,6 +77,43 @@ func TestShellAdapterRunsReusableTargetContract(t *testing.T) { } } +func TestIncompleteRecoveryMetadataRejectsRestoreWithoutChangingShellConfiguration(t *testing.T) { + fingerprint := strings.Repeat("a", 64) + cases := map[string]string{ + "empty object": `{}`, + "missing restoration": `{"version":1,"fingerprint":"` + fingerprint + `","original":null}`, + "invalid fingerprint": `{"version":1,"fingerprint":"short","original":{}}`, + "conflicting originals": `{"version":1,"fingerprint":"` + fingerprint + `","original":{"HTTP_PROXY":"http://prior.example:3128"},"originalScript":"export HTTP_PROXY=http://prior.example:3128\\n"}`, + } + for name, metadata := range cases { + t.Run(name, func(t *testing.T) { + a := newAdapter(t, "/bin/bash", nil) + environment := []byte("export HTTP_PROXY='http://managed.example:8080'\n") + for path, data := range map[string][]byte{a.EnvironmentPath(): environment, a.RecoveryPath(): []byte(metadata)} { + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, data, 0o600); err != nil { + t.Fatal(err) + } + } + if inspection := a.Inspect(context.Background(), nil); inspection.State != target.Drifted { + t.Fatalf("inspection = %#v", inspection) + } + result := a.SetDesired(context.Background(), false, nil) + if result.Err == nil || result.Inspection.State != target.Drifted { + t.Fatalf("unsafe disable = %#v", result) + } + if got := readFile(t, a.EnvironmentPath()); got != string(environment) { + t.Fatalf("environment changed to %q", got) + } + if got := readFile(t, a.RecoveryPath()); got != metadata { + t.Fatalf("recovery changed to %q", got) + } + }) + } +} + func TestRecoveryFailureRetryPreservesOriginalShellState(t *testing.T) { files := &failingFS{FileSystem: shell.OSFileSystem{}, failRecoveryOnce: true} a := newAdapterWithFS(t, "/bin/bash", map[string]string{"HTTP_PROXY": "http://prior.example:3128"}, files) From 9e2418f45e5200b7394c00f8007608e149eadac8 Mon Sep 17 00:00:00 2001 From: proxyctl coordinator Date: Fri, 17 Jul 2026 11:11:56 +0800 Subject: [PATCH 4/5] fix: make daemon reset conflict safe --- cmd/proxyctl/main.go | 43 ++------ cmd/proxyctl/maintenance_test.go | 64 +++++++++++ internal/dockerdaemon/dockerdaemon.go | 28 ++++- internal/dockerdaemon/dockerdaemon_test.go | 120 +++++++++++++++++++++ internal/shell/shell.go | 36 +------ internal/shell/shell_test.go | 65 +++++++++++ 6 files changed, 283 insertions(+), 73 deletions(-) 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 From 4cf9731db492cf4172abdd1c50a91cd13da14db3 Mon Sep 17 00:00:00 2001 From: proxyctl coordinator Date: Fri, 17 Jul 2026 11:25:09 +0800 Subject: [PATCH 5/5] fix: complete reset recovery safety --- cmd/proxyctl/main.go | 22 ++++- cmd/proxyctl/maintenance_test.go | 68 ++++++++++++-- cmd/proxyctl/process_testenv.go | 28 +++++- internal/app/maintenance.go | 39 ++++++-- internal/app/maintenance_test.go | 45 ++++++++- internal/dockerclient/dockerclient.go | 26 ++++++ internal/dockerclient/dockerclient_test.go | 55 +++++++++++ internal/dockerdaemon/dockerdaemon.go | 6 ++ internal/dockerdaemon/dockerdaemon_test.go | 102 ++++++++++++++------- 9 files changed, 339 insertions(+), 52 deletions(-) diff --git a/cmd/proxyctl/main.go b/cmd/proxyctl/main.go index 3bb34d9..791e142 100644 --- a/cmd/proxyctl/main.go +++ b/cmd/proxyctl/main.go @@ -197,7 +197,16 @@ func runMaintenanceCommand(args []string, input io.Reader, output io.Writer, sto if args[0] != "reset" { return errors.New("invalid maintenance command") } - fmt.Fprint(output, "Reset every managed Target and restore recorded original state? [y/N] ") + readOnlyState, readOnlyErr := store.ReadOnlyLoad() + if readOnlyErr != nil { + return fmt.Errorf("inspect Profile state before reset confirmation: %w", readOnlyErr) + } + warning := app.NewMaintenance(readOnlyState, targets, nil, nil).ResetConfirmation(context.Background()) + fmt.Fprintln(output, "Reset every managed Target and restore recorded original state?") + if warning != "" { + fmt.Fprintln(output, warning) + } + fmt.Fprint(output, "Continue? [y/N] ") answer, _ := bufio.NewReader(input).ReadString('\n') answer = strings.ToLower(strings.TrimSpace(answer)) if answer != "y" && answer != "yes" { @@ -222,8 +231,15 @@ func runMaintenanceCommand(args []string, input io.Reader, output io.Writer, sto fmt.Fprintf(output, "%s: skipped (%s) — %s\n", item.TargetID, item.Inspection.State, profile.RedactText(item.Inspection.Evidence)) continue } - if item.Err != nil { - fmt.Fprintf(output, "%s: failed (%s) — %s; fresh inspection: %s\n", item.TargetID, item.Inspection.State, profile.RedactText(item.Err.Error()), profile.RedactText(item.Inspection.Evidence)) + if item.OperationErr != nil { + fmt.Fprintf(output, "%s: failed (%s) — %s; fresh inspection: %s\n", item.TargetID, item.Inspection.State, profile.RedactText(item.OperationErr.Error()), profile.RedactText(item.Inspection.Evidence)) + if item.LogErr != nil { + fmt.Fprintf(output, "%s: operation logging also failed — %s\n", item.TargetID, profile.RedactText(item.LogErr.Error())) + } + continue + } + if item.LogErr != nil { + fmt.Fprintf(output, "%s: reset (%s) — %s; operation logging failed — %s\n", item.TargetID, item.Inspection.State, profile.RedactText(item.Inspection.Evidence), profile.RedactText(item.LogErr.Error())) continue } fmt.Fprintf(output, "%s: reset (%s) — %s\n", item.TargetID, item.Inspection.State, profile.RedactText(item.Inspection.Evidence)) diff --git a/cmd/proxyctl/maintenance_test.go b/cmd/proxyctl/maintenance_test.go index f1707d7..9ccf39a 100644 --- a/cmd/proxyctl/maintenance_test.go +++ b/cmd/proxyctl/maintenance_test.go @@ -12,6 +12,7 @@ import ( "strings" "testing" + "proxyctl/internal/app" "proxyctl/internal/gnome" "proxyctl/internal/operationlock" "proxyctl/internal/oplog" @@ -29,11 +30,16 @@ func (failingIdentityEnvironment) LookupUser(string) (profile.UserIdentity, erro } func (failingIdentityEnvironment) LookupEnv(string) (string, bool) { return "", false } +type failingOperationLogger struct{ err error } + +func (l failingOperationLogger) Write(app.LogEntry) error { return l.err } + type processFakeTarget struct { - id string - state target.State - err error - calls int + id string + state target.State + err error + calls int + confirmation string } func (f *processFakeTarget) ID() string { return f.id } @@ -41,7 +47,7 @@ func (f *processFakeTarget) Description() target.Description { return target.Des func (f *processFakeTarget) Availability(context.Context) target.Availability { return target.Availability{Available: true, Evidence: "fake executable found"} } -func (f *processFakeTarget) Confirmation(bool) string { return "" } +func (f *processFakeTarget) Confirmation(bool) string { return f.confirmation } func (f *processFakeTarget) Inspect(context.Context, *profile.Profile) target.Inspection { return target.Inspection{State: f.state, Evidence: "fake evidence for http://alice:secret@proxy.test"} } @@ -217,14 +223,21 @@ func TestCommandsRunAsRealProcessesInsideIsolatedUserState(t *testing.T) { if !strings.Contains(string(doctorOutput), "Profile validity") || (doctorErr != nil && !strings.Contains(string(doctorOutput), "[FAIL]")) { t.Fatalf("doctor process err=%v output=%q", doctorErr, doctorOutput) } + resetMarker := filepath.Join(root, "home", "reset-mutation-marker") resetOutput, err := invoke("no\n", "reset") - if err != nil || !strings.Contains(string(resetOutput), "Reset cancelled") { + if err != nil || !strings.Contains(string(resetOutput), "Reset cancelled") || !strings.Contains(string(resetOutput), "Docker will restart immediately") { t.Fatalf("reset cancellation process err=%v output=%q", err, resetOutput) } + if _, statErr := os.Stat(resetMarker); !errors.Is(statErr, os.ErrNotExist) { + t.Fatalf("cancelled real process mutated reset fixture: %v", statErr) + } resetOutput, err = invoke("yes\n", "reset") - if err != nil || !strings.Contains(string(resetOutput), "shell: skipped (Disabled)") { + if err != nil || !strings.Contains(string(resetOutput), "shell: skipped (Disabled)") || !strings.Contains(string(resetOutput), "docker-daemon-test: reset (Disabled)") { t.Fatalf("confirmed reset process err=%v output=%q", err, resetOutput) } + if data, readErr := os.ReadFile(resetMarker); readErr != nil || string(data) != "reset\n" { + t.Fatalf("confirmed real process marker err=%v data=%q", readErr, data) + } installOutput, err := invoke("", "shell", "install") if err != nil || !strings.Contains(string(installOutput), "installed") { t.Fatalf("shell install process err=%v output=%q", err, installOutput) @@ -359,6 +372,47 @@ func TestResetSuppressesSavedGNOMEPasswordFromOperationCLIAndLogsWithoutActivePr } } +func TestResetPromptIncludesEveryManagedTargetWarningAndCancellationDoesNotMutate(t *testing.T) { + root := t.TempDir() + config := filepath.Join(root, "profiles.json") + writeConfig(t, config, `{"profiles":[]}`) + store := profile.NewFileStore(config) + daemon := &processFakeTarget{id: "docker-daemon", state: target.Drifted, confirmation: "Docker will restart immediately and may disrupt running containers and other work."} + other := &processFakeTarget{id: "other", state: target.Enabled, confirmation: "Other managed warning."} + external := &processFakeTarget{id: "external", state: target.External, confirmation: "must not appear"} + var output bytes.Buffer + + err := runMaintenanceCommand([]string{"reset"}, strings.NewReader("no\n"), &output, store, []target.Target{daemon, other, external}, operationlock.New(filepath.Join(root, "operation.lock")), nil, nil) + if err != nil { + t.Fatal(err) + } + text := output.String() + for _, want := range []string{"docker-daemon: Docker will restart immediately", "other: Other managed warning", "Reset cancelled"} { + if !strings.Contains(text, want) { + t.Errorf("reset output missing %q: %q", want, text) + } + } + if strings.Contains(text, "must not appear") || daemon.calls+other.calls+external.calls != 0 { + t.Fatalf("cancel output=%q calls=%d/%d/%d", text, daemon.calls, other.calls, external.calls) + } +} + +func TestResetCommandReportsCompletedTargetTruthWhenOperationLoggingFails(t *testing.T) { + root := t.TempDir() + config := filepath.Join(root, "profiles.json") + writeConfig(t, config, `{"profiles":[]}`) + managed := &processFakeTarget{id: "managed", state: target.Enabled} + var output bytes.Buffer + + err := runMaintenanceCommand([]string{"reset"}, strings.NewReader("yes\n"), &output, profile.NewFileStore(config), []target.Target{managed}, operationlock.New(filepath.Join(root, "operation.lock")), failingOperationLogger{err: errors.New("log device full")}, nil) + if err == nil || !strings.Contains(err.Error(), "log device full") { + t.Fatalf("logger failure err=%v output=%q", err, output.String()) + } + if managed.calls != 1 || managed.state != target.Disabled || !strings.Contains(output.String(), "managed: reset (Disabled)") || !strings.Contains(output.String(), "operation logging failed") { + t.Fatalf("logger failure truth output=%q calls=%d state=%s", output.String(), managed.calls, managed.state) + } +} + func TestResetCommandCancellationAndTruthfulPartialFailureWithFakeTargets(t *testing.T) { root := t.TempDir() config := filepath.Join(root, "xdg-config", "proxyctl", "profiles.json") diff --git a/cmd/proxyctl/process_testenv.go b/cmd/proxyctl/process_testenv.go index 3e5f493..88a3810 100644 --- a/cmd/proxyctl/process_testenv.go +++ b/cmd/proxyctl/process_testenv.go @@ -3,21 +3,47 @@ package main import ( + "context" "errors" "os" "os/user" + "path/filepath" "strconv" "proxyctl/internal/profile" "proxyctl/internal/target" ) +type processResetTarget struct{ marker string } + +func (t processResetTarget) ID() string { return "docker-daemon-test" } +func (t processResetTarget) Description() target.Description { + return target.Description{Name: "Docker daemon"} +} +func (t processResetTarget) Availability(context.Context) target.Availability { + return target.Availability{Available: true, Evidence: "isolated process fixture"} +} +func (t processResetTarget) Inspect(context.Context, *profile.Profile) target.Inspection { + if _, err := os.Stat(t.marker); err == nil { + return target.Inspection{State: target.Disabled, Evidence: "isolated managed configuration removed"} + } + return target.Inspection{State: target.Drifted, Evidence: "isolated recovery metadata exists"} +} +func (t processResetTarget) Confirmation(bool) string { + return "Docker will restart immediately. This may disrupt running containers and other work." +} +func (t processResetTarget) SetDesired(context.Context, bool, *profile.Profile) target.OperationResult { + err := os.WriteFile(t.marker, []byte("reset\n"), 0o600) + return target.OperationResult{Inspection: t.Inspect(context.Background(), nil), Err: err} +} + // This file is compiled only into the process-smoke binary. It gives that real // executable an identity whose home is inside the test sandbox and omits // production system Targets, so no command can reach developer or host state. func init() { processProductionTargets = func(profile.IdentityEnvironment) ([]target.Target, error) { - return []target.Target{}, nil + home := os.Getenv("PROXYCTL_PROCESS_TEST_HOME") + return []target.Target{processResetTarget{marker: filepath.Join(home, "reset-mutation-marker")}}, nil } processCurrentUser = func() (profile.UserIdentity, error) { home := os.Getenv("PROXYCTL_PROCESS_TEST_HOME") diff --git a/internal/app/maintenance.go b/internal/app/maintenance.go index 133da03..07ceb6f 100644 --- a/internal/app/maintenance.go +++ b/internal/app/maintenance.go @@ -94,10 +94,12 @@ func (m *Maintenance) Doctor(ctx context.Context, applicationChecks []target.Che } type ResetTargetResult struct { - TargetID string - Inspection target.Inspection - Skipped bool - Err error + TargetID string + Inspection target.Inspection + Skipped bool + OperationErr error + LogErr error + Err error } type ResetResult struct { Cancelled bool @@ -105,6 +107,23 @@ type ResetResult struct { Err error } +// ResetConfirmation performs only read-only inspection and returns every +// warning required by Targets that a reset would attempt to disable. +func (m *Maintenance) ResetConfirmation(ctx context.Context) string { + active := activeFromState(m.state) + var warnings []string + for _, adapter := range m.targets { + inspection := adapter.Inspect(ctx, active) + if inspection.State != target.Enabled && inspection.State != target.Drifted { + continue + } + if warning := adapter.Confirmation(false); warning != "" { + warnings = append(warnings, adapter.ID()+": "+warning) + } + } + return strings.Join(warnings, "\n") +} + func (m *Maintenance) Reset(ctx context.Context, confirmed bool) ResetResult { if !confirmed { return ResetResult{Cancelled: true} @@ -138,10 +157,18 @@ func (m *Maintenance) Reset(ctx context.Context, confirmed bool) ResetResult { result.Err = errors.Join(result.Err, fmt.Errorf("reset %s Target: %w", adapter.ID(), redactError(operation.Err))) } detail = RedactLogDetail(detail) + operationErr := redactError(operation.Err) + var logErr error if m.logger != nil { - _ = m.logger.Write(LogEntry{Time: time.Now(), Operation: "reset", Target: adapter.ID(), Result: outcome, Detail: detail}) + if err := m.logger.Write(LogEntry{Time: time.Now(), Operation: "reset", Target: adapter.ID(), Result: outcome, Detail: detail}); err != nil { + logErr = redactError(err) + result.Err = errors.Join(result.Err, fmt.Errorf("record reset %s Target operation log: %w", adapter.ID(), logErr)) + } } - result.Targets = append(result.Targets, ResetTargetResult{TargetID: adapter.ID(), Inspection: finalInspection, Err: redactError(operation.Err)}) + result.Targets = append(result.Targets, ResetTargetResult{ + TargetID: adapter.ID(), Inspection: finalInspection, + OperationErr: operationErr, LogErr: logErr, Err: errors.Join(operationErr, logErr), + }) } return result } diff --git a/internal/app/maintenance_test.go b/internal/app/maintenance_test.go index d4022a6..d4b52ca 100644 --- a/internal/app/maintenance_test.go +++ b/internal/app/maintenance_test.go @@ -19,6 +19,7 @@ type maintenanceTarget struct { checks []target.Check disableErr error disableCalls int + confirmation string } func (f *maintenanceTarget) ID() string { return f.id } @@ -26,7 +27,7 @@ func (f *maintenanceTarget) Description() target.Description { return target.Description{Name: strings.ToUpper(f.id)} } func (f *maintenanceTarget) Availability(context.Context) target.Availability { return f.available } -func (f *maintenanceTarget) Confirmation(bool) string { return "" } +func (f *maintenanceTarget) Confirmation(bool) string { return f.confirmation } func (f *maintenanceTarget) Inspect(context.Context, *profile.Profile) target.Inspection { return f.inspection } @@ -73,11 +74,14 @@ func (l *recordingLock) Lock(context.Context) (func(), error) { return func() { l.held-- }, nil } -type recordingLog struct{ entries []app.LogEntry } +type recordingLog struct { + entries []app.LogEntry + err error +} func (l *recordingLog) Write(entry app.LogEntry) error { l.entries = append(l.entries, entry) - return nil + return l.err } func TestDoctorReportsReadOnlyChecksEvidenceAndRedactsCredentials(t *testing.T) { @@ -149,6 +153,41 @@ func TestResetFailureUsesFreshInspectionForResultAndRedactedLog(t *testing.T) { } } +func TestResetConfirmationAggregatesWarningsOnlyForManagedCandidatesWithoutMutation(t *testing.T) { + managed := &maintenanceTarget{id: "managed", inspection: target.Inspection{State: target.Enabled, Evidence: "managed"}} + drifted := &maintenanceTarget{id: "drifted", inspection: target.Inspection{State: target.Drifted, Evidence: "recovery metadata remains"}} + external := &maintenanceTarget{id: "external", inspection: target.Inspection{State: target.External, Evidence: "administrator owned"}} + managed.confirmation = "managed warning" + drifted.confirmation = "drifted warning" + external.confirmation = "must not be shown" + maintenance := app.NewMaintenance(profile.State{}, []target.Target{managed, drifted, external}, &recordingLock{}, nil) + + confirmation := maintenance.ResetConfirmation(context.Background()) + for _, want := range []string{"managed: managed warning", "drifted: drifted warning"} { + if !strings.Contains(confirmation, want) { + t.Errorf("confirmation missing %q: %q", want, confirmation) + } + } + if strings.Contains(confirmation, "must not be shown") || managed.disableCalls+drifted.disableCalls+external.disableCalls != 0 { + t.Fatalf("confirmation=%q mutation calls=%d/%d/%d", confirmation, managed.disableCalls, drifted.disableCalls, external.disableCalls) + } +} + +func TestResetLoggerFailureIsReportedWithoutUndoingCompletedConfiguration(t *testing.T) { + adapter := &maintenanceTarget{id: "managed", inspection: target.Inspection{State: target.Enabled, Evidence: "managed"}} + logger := &recordingLog{err: errors.New("operation log disk full")} + + result := app.NewMaintenance(profile.State{}, []target.Target{adapter}, &recordingLock{}, logger).Reset(context.Background(), true) + + if result.Err == nil || !strings.Contains(result.Err.Error(), "operation log disk full") || len(result.Targets) != 1 { + t.Fatalf("reset result = %#v", result) + } + item := result.Targets[0] + if item.Inspection.State != target.Disabled || item.OperationErr != nil || item.LogErr == nil || item.Err == nil || adapter.disableCalls != 1 { + t.Fatalf("logger failure outcome=%#v disable calls=%d", item, adapter.disableCalls) + } +} + func TestResetRequiresConfirmationAndReportsTruthfulPartialResults(t *testing.T) { good := &maintenanceTarget{id: "good", available: target.Availability{Available: true}, inspection: target.Inspection{State: target.Enabled, Evidence: "managed"}} failure := errors.New("remove https://alice:secret@proxy.example failed: permission denied") diff --git a/internal/dockerclient/dockerclient.go b/internal/dockerclient/dockerclient.go index 2e4a997..1c386d1 100644 --- a/internal/dockerclient/dockerclient.go +++ b/internal/dockerclient/dockerclient.go @@ -358,6 +358,19 @@ func (a *Adapter) disable() error { if err != nil { return err } + if err := validateManagedProxyStrings(defaults); err != nil { + return err + } + currentFingerprint := fingerprintCurrent(config) + if currentFingerprint == fingerprintOriginal(metadata) { + if err := a.files.Remove(a.configuration.RecoveryFile); err != nil && !errors.Is(err, fs.ErrNotExist) { + return fmt.Errorf("clean Docker client recovery metadata for already-restored proxy defaults: %w", err) + } + return nil + } + if currentFingerprint != metadata.Fingerprint { + return errors.New("Docker client managed proxy defaults changed after proxyctl applied them; refusing to overwrite current configuration or discard recovery metadata") + } for _, key := range managedKeys { prior := metadata.Original[key] if prior.Present { @@ -533,6 +546,19 @@ func fingerprintDefaults(values map[string]string) string { return fingerprintSnapshot(snapshot) } +func fingerprintOriginal(metadata recovery) string { + snapshot := make(map[string]managedValue, len(managedKeys)) + for _, key := range managedKeys { + prior := metadata.Original[key] + value := managedValue{Present: prior.Present} + if prior.Present && json.Unmarshal(prior.Value, &value.Value) == nil { + value.Valid = true + } + snapshot[key] = value + } + return fingerprintSnapshot(snapshot) +} + func fingerprintCurrent(config map[string]json.RawMessage) string { snapshot := make(map[string]managedValue, len(managedKeys)) proxies, _, proxyErr := childObject(config, "proxies") diff --git a/internal/dockerclient/dockerclient_test.go b/internal/dockerclient/dockerclient_test.go index 2268713..5866403 100644 --- a/internal/dockerclient/dockerclient_test.go +++ b/internal/dockerclient/dockerclient_test.go @@ -13,6 +13,7 @@ import ( "testing" "time" + "proxyctl/internal/app" "proxyctl/internal/dockerclient" "proxyctl/internal/profile" "proxyctl/internal/target" @@ -202,6 +203,56 @@ func TestEnableSafelyMergesAndDisableRestoresPriorDefaults(t *testing.T) { } } +func TestResetDriftedDockerClientRejectsAdministratorManagedKeyEditsWithoutWritesOrCleanup(t *testing.T) { + files := &countingFS{FileSystem: dockerclient.OSFileSystem{}} + a := newAdapterWithFS(t, files) + writeConfig(t, a.ConfigPath(), `{"auths":{"keep":{}},"proxies":{"default":{"httpProxy":"http://prior.example:3128"}}}`) + if result := a.SetDesired(context.Background(), true, activeProfile()); result.Err != nil { + t.Fatal(result.Err) + } + updateDefaultKey(t, a.ConfigPath(), "httpProxy", json.RawMessage(`"http://administrator.example:3128"`)) + configBefore := readFile(t, a.ConfigPath()) + metadataBefore := readFile(t, a.RecoveryPath()) + writesBefore := files.writes + + 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].Inspection.State != target.Drifted || reset.Targets[0].Err == nil { + t.Fatalf("conflicting reset = %#v", reset) + } + if !strings.Contains(reset.Targets[0].Err.Error(), "changed after proxyctl applied") || files.writes != writesBefore { + t.Fatalf("conflict outcome=%#v writes=%d->%d", reset.Targets[0], writesBefore, files.writes) + } + if got := readFile(t, a.ConfigPath()); got != configBefore { + t.Fatalf("conflict rewrote config:\n%s", got) + } + if got := readFile(t, a.RecoveryPath()); got != metadataBefore { + t.Fatalf("conflict changed metadata:\n%s", got) + } +} + +func TestResetDriftedDockerClientAlreadyAtOriginalOnlyCleansMetadata(t *testing.T) { + files := &countingFS{FileSystem: dockerclient.OSFileSystem{}} + a := newAdapterWithFS(t, files) + original := `{"auths":{"keep":{}},"proxies":{"default":{"httpProxy":"http://prior.example:3128","custom":{"keep":true}}}}` + writeConfig(t, a.ConfigPath(), original) + if result := a.SetDesired(context.Background(), true, activeProfile()); result.Err != nil { + t.Fatal(result.Err) + } + writeConfig(t, a.ConfigPath(), original) + writesBefore := files.writes + + 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].Inspection.State != target.External { + t.Fatalf("already-original reset = %#v", reset) + } + if files.writes != writesBefore || readFile(t, a.ConfigPath()) != original { + t.Fatalf("already-original reset wrote config: writes=%d->%d", writesBefore, files.writes) + } + if _, err := os.Stat(a.RecoveryPath()); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("recovery metadata remains: %v", err) + } +} + func TestInspectionDistinguishesAllStatesAndManagedValueEditsDrift(t *testing.T) { a := newAdapter(t, nil) assertState(t, a, target.Disabled) @@ -563,6 +614,10 @@ func assertState(t *testing.T, a *dockerclient.Adapter, want target.State) { } } +type immediateLock struct{} + +func (immediateLock) Lock(context.Context) (func(), error) { return func() {}, nil } + type failingFS struct { dockerclient.FileSystem permission, failConfig, leak, restoreLeak, failRecoveryOnce bool diff --git a/internal/dockerdaemon/dockerdaemon.go b/internal/dockerdaemon/dockerdaemon.go index 894925d..0201908 100644 --- a/internal/dockerdaemon/dockerdaemon.go +++ b/internal/dockerdaemon/dockerdaemon.go @@ -348,6 +348,12 @@ func (a *Adapter) disable(ctx context.Context) error { alreadyOriginal := metadata.OriginalExists && currentErr == nil && fingerprint(current) == fingerprint(metadata.Original) alreadyAbsent := !metadata.OriginalExists && errors.Is(currentErr, fs.ErrNotExist) if alreadyOriginal || alreadyAbsent { + // Recovery metadata proves proxyctl previously changed the effective + // daemon configuration. Even when the file has already been restored, + // complete the service effects before relinquishing that recovery record. + if err := a.reloadRestartInspect(ctx); err != nil { + return err + } 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) } diff --git a/internal/dockerdaemon/dockerdaemon_test.go b/internal/dockerdaemon/dockerdaemon_test.go index 249c68e..596ccc8 100644 --- a/internal/dockerdaemon/dockerdaemon_test.go +++ b/internal/dockerdaemon/dockerdaemon_test.go @@ -23,6 +23,7 @@ type recorder struct{ events []string } type fakeRunner struct { recorder *recorder show string + active string fail map[string]error } @@ -36,7 +37,7 @@ func (r *fakeRunner) Run(_ context.Context, name string, args ...string) ([]byte return []byte(r.show), nil } if strings.Contains(command, " is-active ") { - return []byte("active\n"), nil + return []byte(r.active), nil } return nil, nil } @@ -73,7 +74,7 @@ func fixture(t *testing.T) (*dockerdaemon.Adapter, *recorder, string, string, *f daemonPath := filepath.Join(directory, "etc", "docker", "daemon.json") recoveryPath := filepath.Join(directory, "state", "docker-daemon.json") record := &recorder{} - runner := &fakeRunner{recorder: record, show: "LoadState=loaded\nFragmentPath=/lib/systemd/system/docker.service\n", fail: map[string]error{}} + runner := &fakeRunner{recorder: record, show: "LoadState=loaded\nFragmentPath=/lib/systemd/system/docker.service\n", active: "active\n", fail: map[string]error{}} adapter := dockerdaemon.New(dockerdaemon.Configuration{DaemonConfigPath: daemonPath, RecoveryPath: recoveryPath, TrustedRoot: directory}, recordingFS{recorder: record}, runner) return adapter, record, daemonPath, recoveryPath, runner } @@ -429,9 +430,75 @@ func TestDockerDaemonDisableAndResetRejectDriftWithoutMutatingDaemonOrRecovery(t } } -func TestDockerDaemonResetCleansAlreadyRestoredRecoveryWithoutDaemonWriteOrSystemctl(t *testing.T) { +func TestDockerDaemonResetAlreadyRestoredReloadsRestartsAndInspectsBeforeMetadataCleanup(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") + prepareAlreadyRestoredDaemon(t, a, daemonPath, original) + 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) + } + assertSubsequence(t, record.events, []string{"systemctl daemon-reload", "systemctl restart docker.service", "systemctl is-active docker.service", "remove docker-daemon.json"}) + if contains(record.events, "write daemon.json") { + t.Fatalf("already-restored reset rewrote daemon config: %#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 _, err := os.Stat(recoveryPath); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("recovery metadata remains: %v", err) + } +} + +func TestDockerDaemonAlreadyRestoredPreservesMetadataAndReportsFreshDriftOnEveryServiceFailure(t *testing.T) { + failures := []struct { + name string + command string + active string + }{ + {name: "daemon reload command", command: "systemctl daemon-reload"}, + {name: "restart command", command: "systemctl restart docker.service"}, + {name: "active inspection command", command: "systemctl is-active docker.service"}, + {name: "inactive service state", active: "failed\n"}, + } + for _, failure := range failures { + t.Run(failure.name, func(t *testing.T) { + a, record, daemonPath, recoveryPath, runner := fixture(t) + original := []byte("{\n \"log-level\": \"debug\"\n}\n") + prepareAlreadyRestoredDaemon(t, a, daemonPath, original) + metadataBefore, err := os.ReadFile(recoveryPath) + if err != nil { + t.Fatal(err) + } + if failure.command != "" { + runner.fail[failure.command] = errors.New("service failure") + } + if failure.active != "" { + runner.active = failure.active + } + 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].Inspection.State != target.Drifted { + t.Fatalf("failed already-restored reset = %#v", reset) + } + if contains(record.events, "write daemon.json") || contains(record.events, "remove docker-daemon.json") { + t.Fatalf("service failure mutated configuration or metadata: %#v", record.events) + } + if got, err := os.ReadFile(daemonPath); err != nil || !reflect.DeepEqual(got, original) { + t.Fatalf("daemon config changed to %q: %v", got, err) + } + if got, err := os.ReadFile(recoveryPath); err != nil || !reflect.DeepEqual(got, metadataBefore) { + t.Fatalf("metadata changed to %q: %v", got, err) + } + }) + } +} + +func prepareAlreadyRestoredDaemon(t *testing.T, a *dockerdaemon.Adapter, daemonPath string, original []byte) { + t.Helper() if err := os.MkdirAll(filepath.Dir(daemonPath), 0o700); err != nil { t.Fatal(err) } @@ -444,35 +511,6 @@ func TestDockerDaemonResetCleansAlreadyRestoredRecoveryWithoutDaemonWriteOrSyste 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) {