From d4c76e4b3cb50237fc1e346174efae5abe7faff4 Mon Sep 17 00:00:00 2001 From: proxyctl coordinator Date: Fri, 17 Jul 2026 03:28:29 +0800 Subject: [PATCH 1/2] feat: add diagnostics and safe reset maintenance --- cmd/proxyctl/main.go | 163 ++++++++++++++++++++++++--- cmd/proxyctl/maintenance_test.go | 128 +++++++++++++++++++++ internal/app/app.go | 43 ++++++- internal/app/maintenance.go | 166 ++++++++++++++++++++++++++++ internal/app/maintenance_test.go | 140 +++++++++++++++++++++++ internal/operationlock/lock.go | 84 ++++++++++++++ internal/operationlock/lock_test.go | 30 +++++ internal/oplog/logger.go | 113 +++++++++++++++++++ internal/oplog/logger_test.go | 49 ++++++++ internal/profile/diagnostic.go | 58 ++++++++++ internal/profile/diagnostic_test.go | 35 ++++++ internal/shell/shell.go | 42 +++++++ internal/target/target.go | 13 +++ 13 files changed, 1041 insertions(+), 23 deletions(-) create mode 100644 cmd/proxyctl/maintenance_test.go create mode 100644 internal/app/maintenance.go create mode 100644 internal/app/maintenance_test.go create mode 100644 internal/operationlock/lock.go create mode 100644 internal/operationlock/lock_test.go create mode 100644 internal/oplog/logger.go create mode 100644 internal/oplog/logger_test.go create mode 100644 internal/profile/diagnostic.go create mode 100644 internal/profile/diagnostic_test.go diff --git a/cmd/proxyctl/main.go b/cmd/proxyctl/main.go index 8bb4e23..a8e05bf 100644 --- a/cmd/proxyctl/main.go +++ b/cmd/proxyctl/main.go @@ -1,14 +1,22 @@ package main import ( + "bufio" "context" + "errors" "fmt" "io" "os" + "path/filepath" + "strings" + "syscall" tea "github.com/charmbracelet/bubbletea" + "golang.org/x/sys/unix" "proxyctl/internal/app" + "proxyctl/internal/operationlock" + "proxyctl/internal/oplog" "proxyctl/internal/profile" "proxyctl/internal/shell" "proxyctl/internal/target" @@ -50,31 +58,56 @@ func runWithEnvironment(args []string, input io.Reader, output io.Writer, enviro return err } shellTarget := shell.New(shellConfiguration, files, environment) + targets := []target.Target{shellTarget} + stateDirectory := filepath.Dir(shellConfiguration.RecoveryFile) + mutationLock := operationlock.NewOwned(filepath.Join(stateDirectory, "operation.lock"), profileConfiguration.Owner.UID, profileConfiguration.Owner.GID) + logger := oplog.NewOwned(filepath.Join(stateDirectory, "operations.log"), 64*1024, 3, profileConfiguration.Owner.UID, profileConfiguration.Owner.GID) + store := profile.NewOwnedFileStoreAt(profileConfiguration.Path, profileConfiguration.Owner, profileConfiguration.TrustedRoot) if len(args) != 0 { - if len(args) != 2 || args[0] != "shell" || (args[1] != "install" && args[1] != "uninstall") { - return fmt.Errorf("usage: proxyctl [shell install|shell uninstall]") - } - availability := shellTarget.Availability(context.Background()) - if !availability.Available { - return fmt.Errorf("Shell Target unavailable: %s", availability.Evidence) - } - if args[1] == "install" { - if err := shellTarget.Install(context.Background(), executable); err != nil { - return err + switch { + case len(args) == 2 && args[0] == "shell" && (args[1] == "install" || args[1] == "uninstall"): + availability := shellTarget.Availability(context.Background()) + if !availability.Available { + return fmt.Errorf("Shell Target unavailable: %s", availability.Evidence) + } + unlock, err := mutationLock.Lock(context.Background()) + if err != nil { + return fmt.Errorf("lock proxyctl mutation: %w", err) + } + defer unlock() + operation := "shell-" + args[1] + var operationErr error + if args[1] == "install" { + operationErr = shellTarget.Install(context.Background(), executable) + } else { + operationErr = shellTarget.Uninstall(context.Background()) + } + result, detail := "success", "Shell integration changed" + if operationErr != nil { + result, detail = "failure", operationErr.Error() + } + _ = logger.Write(app.LogEntry{Operation: operation, Target: shellTarget.ID(), Result: result, Detail: detail}) + if operationErr != nil { + return operationErr + } + if args[1] == "install" { + fmt.Fprintln(output, "Shell integration installed; restart or source your Shell startup file once.") + } else { + fmt.Fprintln(output, "Shell integration uninstalled.") } - fmt.Fprintln(output, "Shell integration installed; restart or source your Shell startup file once.") return nil + case len(args) == 1 && args[0] == "doctor": + checks := diagnosticChecks(profileConfiguration) + return runMaintenanceCommand(args, input, output, store, targets, mutationLock, logger, checks) + case len(args) == 1 && args[0] == "reset": + return runMaintenanceCommand(args, input, output, store, targets, mutationLock, logger, nil) + default: + return fmt.Errorf("usage: proxyctl [doctor|reset|shell install|shell uninstall]") } - if err := shellTarget.Uninstall(context.Background()); err != nil { - return err - } - fmt.Fprintln(output, "Shell integration uninstalled.") - return nil } - store := profile.NewOwnedFileStoreAt(profileConfiguration.Path, profileConfiguration.Owner, profileConfiguration.TrustedRoot) - application, err := app.NewWithTargets(store, []target.Target{shellTarget}) + application, err := app.NewWithTargetsAndRuntime(store, targets, mutationLock, logger) if err != nil { return err } @@ -82,3 +115,97 @@ func runWithEnvironment(args []string, input io.Reader, output io.Writer, enviro _, err = program.Run() return err } + +func runMaintenanceCommand(args []string, input io.Reader, output io.Writer, store *profile.FileStore, targets []target.Target, mutationLock app.MutationLock, logger app.OperationLogger, checks []target.Check) error { + if len(args) != 1 { + return errors.New("invalid maintenance command") + } + if args[0] == "doctor" { + state, err := store.ReadOnlyLoad() + profileCheck := target.Check{Name: "Profile validity", OK: err == nil, Evidence: "Profile configuration is valid"} + if err != nil { + profileCheck.Evidence = err.Error() + } + checks = append([]target.Check{profileCheck}, checks...) + report := app.NewMaintenance(state, targets, nil, nil).Doctor(context.Background(), checks) + fmt.Fprint(output, report.String()) + return nil + } + if args[0] != "reset" { + return errors.New("invalid maintenance command") + } + fmt.Fprint(output, "Reset every managed Target and restore recorded original state? [y/N] ") + answer, _ := bufio.NewReader(input).ReadString('\n') + answer = strings.ToLower(strings.TrimSpace(answer)) + if answer != "y" && answer != "yes" { + fmt.Fprintln(output, "Reset cancelled; no Targets changed.") + return nil + } + state, err := store.Load() + if err != nil { + return err + } + result := app.NewMaintenance(state, targets, mutationLock, logger).Reset(context.Background(), true) + for _, item := range result.Targets { + if item.Skipped { + 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\n", item.TargetID, item.Inspection.State, profile.RedactText(item.Err.Error())) + continue + } + fmt.Fprintf(output, "%s: reset (%s) — %s\n", item.TargetID, item.Inspection.State, profile.RedactText(item.Inspection.Evidence)) + } + return result.Err +} + +func diagnosticChecks(configuration profile.Configuration) []target.Check { + identity := target.Check{Name: "identity resolution", OK: true, Evidence: fmt.Sprintf("configuration owner %s (uid %d, gid %d)", configuration.Owner.Username, configuration.Owner.UID, configuration.Owner.GID)} + ok, evidence := storagePermissions(configuration) + storage := target.Check{Name: "storage permissions", OK: ok, Evidence: evidence} + return []target.Check{identity, storage} +} + +func storagePermissions(configuration profile.Configuration) (bool, string) { + ok, evidence := writableLocation(configuration.Path) + info, err := os.Stat(configuration.Path) + if os.IsNotExist(err) { + return ok, evidence + "; Profile configuration does not exist yet" + } + if err != nil { + return false, 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()) + } + 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 + } +} diff --git a/cmd/proxyctl/maintenance_test.go b/cmd/proxyctl/maintenance_test.go new file mode 100644 index 0000000..fef7b72 --- /dev/null +++ b/cmd/proxyctl/maintenance_test.go @@ -0,0 +1,128 @@ +package main + +import ( + "bytes" + "context" + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "proxyctl/internal/operationlock" + "proxyctl/internal/oplog" + "proxyctl/internal/profile" + "proxyctl/internal/target" +) + +type processFakeTarget struct { + id string + state target.State + err error + calls int +} + +func (f *processFakeTarget) ID() string { return f.id } +func (f *processFakeTarget) Description() target.Description { return target.Description{Name: f.id} } +func (f *processFakeTarget) Availability(context.Context) target.Availability { + return target.Availability{Available: true, Evidence: "fake executable found"} +} +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"} +} +func (f *processFakeTarget) SetDesired(context.Context, bool, *profile.Profile) target.OperationResult { + f.calls++ + if f.err == nil { + f.state = target.Disabled + } + return target.OperationResult{Inspection: target.Inspection{State: f.state, Evidence: "re-inspected fake state"}, Err: f.err} +} +func (f *processFakeTarget) DiagnosticChecks(context.Context, *profile.Profile) []target.Check { + return []target.Check{{Name: "required commands", OK: true, Evidence: "fake helper found"}, {Name: "write authority", OK: true, Evidence: "fake location writable"}} +} + +func TestDoctorCommandUsesFakeTargetsAndIsolatedXDGWithoutMutation(t *testing.T) { + xdg := filepath.Join(t.TempDir(), "xdg-config") + store := profile.NewFileStore(filepath.Join(xdg, "proxyctl", "profiles.json")) + fake := &processFakeTarget{id: "fake", state: target.External} + var output bytes.Buffer + if err := runMaintenanceCommand([]string{"doctor"}, strings.NewReader(""), &output, store, []target.Target{fake}, nil, nil, []target.Check{{Name: "identity resolution", OK: true, Evidence: "fake user"}, {Name: "storage permissions", OK: true, Evidence: xdg}}); err != nil { + t.Fatal(err) + } + text := output.String() + for _, want := range []string{"Profile validity", "identity resolution", "storage permissions", "Target availability", "External", "required commands", "write authority"} { + if !strings.Contains(text, want) { + t.Errorf("doctor output missing %q: %s", want, text) + } + } + if strings.Contains(text, "secret") { + t.Fatalf("doctor leaked credentials: %s", text) + } + if fake.calls != 0 { + t.Fatalf("doctor mutated fake Target %d times", fake.calls) + } + if _, err := os.Stat(xdg); !os.IsNotExist(err) { + t.Fatalf("doctor created isolated XDG path: %v", err) + } +} + +func TestDoctorStoragePermissionCheckIsReadOnly(t *testing.T) { + path := filepath.Join(t.TempDir(), "proxyctl", "profiles.json") + writeConfig(t, path, `{"profiles":[]}`) + if err := os.Chmod(path, 0o644); err != nil { + t.Fatal(err) + } + configuration := profile.Configuration{Path: path, Owner: profile.UserIdentity{UID: os.Getuid(), GID: os.Getgid()}} + checks := diagnosticChecks(configuration) + if checks[1].OK || !strings.Contains(checks[1].Evidence, "want 600") { + t.Fatalf("storage check = %#v", checks[1]) + } + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if info.Mode().Perm() != 0o644 { + t.Fatalf("doctor permission check changed mode to %o", info.Mode().Perm()) + } +} + +func TestResetCommandCancellationAndTruthfulPartialFailureWithFakeTargets(t *testing.T) { + root := t.TempDir() + config := filepath.Join(root, "xdg-config", "proxyctl", "profiles.json") + writeConfig(t, config, `{"profiles":[]}`) + store := profile.NewFileStore(config) + good := &processFakeTarget{id: "good", state: target.Enabled} + bad := &processFakeTarget{id: "bad", state: target.Drifted, err: errors.New("remove http://bob:hunter2@proxy.test failed")} + external := &processFakeTarget{id: "external", state: target.External} + stateDir := filepath.Join(root, "xdg-state", "proxyctl") + lock := operationlock.New(filepath.Join(stateDir, "operation.lock")) + logger := oplog.New(filepath.Join(stateDir, "operations.log"), 1024, 1) + + var output bytes.Buffer + if err := runMaintenanceCommand([]string{"reset"}, strings.NewReader("no\n"), &output, store, []target.Target{good, bad, external}, lock, logger, nil); err != nil { + t.Fatal(err) + } + if good.calls+bad.calls+external.calls != 0 || !strings.Contains(output.String(), "cancelled") { + t.Fatalf("cancel result output=%q calls=%d/%d/%d", output.String(), good.calls, bad.calls, external.calls) + } + + output.Reset() + err := runMaintenanceCommand([]string{"reset"}, strings.NewReader("yes\n"), &output, store, []target.Target{good, bad, external}, lock, logger, nil) + if err == nil { + 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)") { + 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") { + t.Fatalf("reset leaked credentials: output=%q err=%v", text, err) + } + logData, readErr := os.ReadFile(filepath.Join(stateDir, "operations.log")) + if readErr != nil { + t.Fatal(readErr) + } + if strings.Contains(string(logData), "hunter2") || strings.Contains(string(logData), "bob:") { + t.Fatalf("log leaked full authenticated URL: %s", logData) + } +} diff --git a/internal/app/app.go b/internal/app/app.go index 006010e..a4fab84 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "time" "proxyctl/internal/profile" "proxyctl/internal/target" @@ -55,10 +56,12 @@ type Module interface { } type Application struct { - store profile.Store - state profile.State - targets []target.Target - inspections map[string]target.Inspection + store profile.Store + state profile.State + targets []target.Target + inspections map[string]target.Inspection + mutationLock MutationLock + logger OperationLogger } func New(store profile.Store) (*Application, error) { @@ -66,11 +69,23 @@ func New(store profile.Store) (*Application, error) { } func NewWithTargets(store profile.Store, targets []target.Target) (*Application, error) { + return NewWithTargetsAndLock(store, targets, nil) +} + +// NewWithTargetsAndLock configures the cross-process lock used by every +// Profile or Target mutation. A nil lock is intended only for isolated tests. +func NewWithTargetsAndLock(store profile.Store, targets []target.Target, mutationLock MutationLock) (*Application, error) { + return NewWithTargetsAndRuntime(store, targets, mutationLock, nil) +} + +// NewWithTargetsAndRuntime configures shared mutation serialization and local +// operation diagnostics for production callers. +func NewWithTargetsAndRuntime(store profile.Store, targets []target.Target, mutationLock MutationLock, logger OperationLogger) (*Application, error) { state, err := store.Load() if err != nil { return nil, err } - a := &Application{store: store, state: state, targets: append([]target.Target(nil), targets...), inspections: make(map[string]target.Inspection)} + a := &Application{store: store, state: state, targets: append([]target.Target(nil), targets...), inspections: make(map[string]target.Inspection), mutationLock: mutationLock, logger: logger} a.refreshTargets() return a, nil } @@ -87,6 +102,13 @@ func (a *Application) Snapshot() Snapshot { } func (a *Application) Dispatch(intent Intent) Result { + if a.mutationLock != nil { + unlock, err := a.mutationLock.Lock(context.Background()) + if err != nil { + return Result{Snapshot: a.Snapshot(), Err: fmt.Errorf("lock proxyctl mutation: %w", err)} + } + defer unlock() + } if intent.Kind == SetTargetDesired { return a.setTargetDesired(intent) } @@ -161,6 +183,17 @@ func (a *Application) setTargetDesired(intent Intent) Result { } result := selected.SetDesired(context.Background(), intent.Enabled, active) a.inspections[selected.ID()] = result.Inspection + outcome, detail := "success", result.Inspection.Evidence + if result.Err != nil { + outcome, detail = "failure", result.Err.Error() + } + if a.logger != nil { + operation := "disable" + if intent.Enabled { + operation = "enable" + } + _ = a.logger.Write(LogEntry{Time: time.Now(), Operation: operation, Target: selected.ID(), Result: outcome, Detail: RedactLogDetail(detail)}) + } notice := "" if result.Err == nil { notice = fmt.Sprintf("%s Target is now %s", selected.Description().Name, result.Inspection.State) diff --git a/internal/app/maintenance.go b/internal/app/maintenance.go new file mode 100644 index 0000000..9b5fd4b --- /dev/null +++ b/internal/app/maintenance.go @@ -0,0 +1,166 @@ +package app + +import ( + "context" + "errors" + "fmt" + "regexp" + "strings" + "time" + + "proxyctl/internal/profile" + "proxyctl/internal/target" +) + +// MutationLock serializes all state-changing operations across processes. +type MutationLock interface { + Lock(context.Context) (unlock func(), err error) +} + +// LogEntry is the privacy-safe operation record written to bounded local logs. +type LogEntry struct { + Time time.Time + Operation string + Target string + Result string + Detail string +} + +type OperationLogger interface{ Write(LogEntry) error } + +type Maintenance struct { + state profile.State + targets []target.Target + lock MutationLock + logger OperationLogger +} + +func NewMaintenance(state profile.State, targets []target.Target, lock MutationLock, logger OperationLogger) *Maintenance { + return &Maintenance{state: state, targets: append([]target.Target(nil), targets...), lock: lock, logger: logger} +} + +type Diagnostic struct { + TargetID string + Check target.Check +} + +type DoctorReport struct{ Diagnostics []Diagnostic } + +func (r DoctorReport) Healthy() bool { + for _, diagnostic := range r.Diagnostics { + if !diagnostic.Check.OK { + return false + } + } + return true +} +func (r DoctorReport) String() string { + var output strings.Builder + for _, diagnostic := range r.Diagnostics { + status := "OK" + if !diagnostic.Check.OK { + status = "FAIL" + } + name := diagnostic.Check.Name + if diagnostic.TargetID != "" { + name = diagnostic.TargetID + ": " + name + } + fmt.Fprintf(&output, "[%s] %s — %s\n", status, name, profile.RedactText(diagnostic.Check.Evidence)) + } + return output.String() +} + +// Doctor only invokes observational Target methods. +func (m *Maintenance) Doctor(ctx context.Context, applicationChecks []target.Check) DoctorReport { + report := DoctorReport{} + for _, check := range applicationChecks { + check.Evidence = profile.RedactText(check.Evidence) + report.Diagnostics = append(report.Diagnostics, Diagnostic{Check: check}) + } + 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}}) + 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}}) + if diagnosable, ok := adapter.(target.Diagnosable); ok { + for _, check := range diagnosable.DiagnosticChecks(ctx, active) { + check.Evidence = profile.RedactText(check.Evidence) + report.Diagnostics = append(report.Diagnostics, Diagnostic{TargetID: adapter.ID(), Check: check}) + } + } + } + return report +} + +type ResetTargetResult struct { + TargetID string + Inspection target.Inspection + Skipped bool + Err error +} +type ResetResult struct { + Cancelled bool + Targets []ResetTargetResult + Err error +} + +func (m *Maintenance) Reset(ctx context.Context, confirmed bool) ResetResult { + if !confirmed { + return ResetResult{Cancelled: true} + } + if m.lock == nil { + return ResetResult{Err: errors.New("mutation lock is not configured")} + } + unlock, err := m.lock.Lock(ctx) + if err != nil { + return ResetResult{Err: fmt.Errorf("lock proxyctl mutation: %w", err)} + } + defer unlock() + active := activeFromState(m.state) + result := ResetResult{} + for _, adapter := range m.targets { + inspection := adapter.Inspect(ctx, active) + if inspection.State != target.Enabled && inspection.State != target.Drifted { + result.Targets = append(result.Targets, ResetTargetResult{TargetID: adapter.ID(), Inspection: inspection, Skipped: true}) + continue + } + operation := adapter.SetDesired(ctx, false, active) + detail, outcome := operation.Inspection.Evidence, "success" + if operation.Err != nil { + outcome = "failure" + detail = operation.Err.Error() + 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)}) + } + return result +} + +func activeFromState(state profile.State) *profile.Profile { + for i := range state.Profiles { + if state.Profiles[i].Name == state.ActiveProfile { + value := state.Profiles[i] + return &value + } + } + return nil +} + +var authenticatedURL = regexp.MustCompile(`(?i)(?:https?|socks5h?)://[^\s'\"<>]*@[^\s'\"<>]+`) + +// RedactLogDetail omits complete authenticated URLs, not just their passwords. +func RedactLogDetail(value string) string { + value = authenticatedURL.ReplaceAllString(value, "[REDACTED AUTHENTICATED URL]") + return profile.RedactText(value) +} +func redactError(err error) error { + if err == nil { + return nil + } + return errors.New(profile.RedactText(err.Error())) +} diff --git a/internal/app/maintenance_test.go b/internal/app/maintenance_test.go new file mode 100644 index 0000000..f084442 --- /dev/null +++ b/internal/app/maintenance_test.go @@ -0,0 +1,140 @@ +package app_test + +import ( + "context" + "errors" + "path/filepath" + "strings" + "testing" + + "proxyctl/internal/app" + "proxyctl/internal/profile" + "proxyctl/internal/target" +) + +type maintenanceTarget struct { + id string + available target.Availability + inspection target.Inspection + checks []target.Check + disableErr error + disableCalls int +} + +func (f *maintenanceTarget) ID() string { return f.id } +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) Inspect(context.Context, *profile.Profile) target.Inspection { + return f.inspection +} +func (f *maintenanceTarget) SetDesired(_ context.Context, enabled bool, _ *profile.Profile) target.OperationResult { + if !enabled { + f.disableCalls++ + if f.disableErr == nil { + f.inspection = target.Inspection{State: target.Disabled, Evidence: "managed state removed"} + } + } + return target.OperationResult{Inspection: f.inspection, Err: f.disableErr} +} +func (f *maintenanceTarget) DiagnosticChecks(context.Context, *profile.Profile) []target.Check { + return append([]target.Check(nil), f.checks...) +} + +type recordingLock struct{ held, acquired int } + +func (l *recordingLock) Lock(context.Context) (func(), error) { + l.held++ + l.acquired++ + return func() { l.held-- }, nil +} + +type recordingLog struct{ entries []app.LogEntry } + +func (l *recordingLog) Write(entry app.LogEntry) error { + l.entries = append(l.entries, entry) + return nil +} + +func TestDoctorReportsReadOnlyChecksEvidenceAndRedactsCredentials(t *testing.T) { + adapter := &maintenanceTarget{ + id: "fake", available: target.Availability{Available: true, Evidence: "fake command found"}, + inspection: target.Inspection{State: target.External, Evidence: "found http://alice:secret@proxy.example:8080 in fake settings"}, + checks: []target.Check{{Name: "required commands", OK: false, Evidence: "helper failed for https://bob:hunter2@proxy.example/path"}, {Name: "write authority", OK: true, Evidence: "settings directory is writable"}}, + } + maintenance := app.NewMaintenance(profile.State{}, []target.Target{adapter}, nil, nil) + + report := maintenance.Doctor(context.Background(), []target.Check{{Name: "Profile configuration", OK: true, Evidence: "valid"}, {Name: "identity resolution", OK: true, Evidence: "tester (1000)"}}) + + if adapter.disableCalls != 0 { + t.Fatalf("doctor mutated Target %d times", adapter.disableCalls) + } + text := report.String() + for _, want := range []string{"Profile configuration", "identity resolution", "Target availability", "External", "required commands", "write authority", "found http://alice:REDACTED@proxy.example:8080"} { + if !strings.Contains(text, want) { + t.Errorf("doctor report missing %q: %s", want, text) + } + } + for _, secret := range []string{"secret", "hunter2"} { + if strings.Contains(text, secret) { + t.Errorf("doctor leaked %q: %s", secret, text) + } + } +} + +func TestApplicationSerializesAndLogsTargetMutations(t *testing.T) { + adapter := &maintenanceTarget{id: "fake", available: target.Availability{Available: true}, inspection: target.Inspection{State: target.External}} + lock := &recordingLock{} + log := &recordingLog{} + application, err := app.NewWithTargetsAndRuntime(profile.NewFileStore(filepath.Join(t.TempDir(), "profiles.json")), []target.Target{adapter}, lock, log) + if err != nil { + t.Fatal(err) + } + if result := application.Dispatch(app.Intent{Kind: app.CreateProfile, Profile: profile.Profile{Name: "office", SingleURL: "http://proxy.test:8080"}}); result.Err != nil { + t.Fatal(result.Err) + } + if result := application.Dispatch(app.Intent{Kind: app.ActivateProfile, Name: "office"}); result.Err != nil { + t.Fatal(result.Err) + } + adapter.disableErr = errors.New("failed http://alice:secret@proxy.test/private") + result := application.Dispatch(app.Intent{Kind: app.SetTargetDesired, TargetID: "fake", Enabled: false}) + if result.Err == nil { + t.Fatal("Target failure reported success") + } + if lock.acquired != 3 || lock.held != 0 { + t.Fatalf("all mutation lock acquired=%d held=%d", lock.acquired, lock.held) + } + if len(log.entries) != 1 || log.entries[0].Operation != "disable" || strings.Contains(log.entries[0].Detail, "secret") || strings.Contains(log.entries[0].Detail, "proxy.test/private") { + t.Fatalf("Target operation 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") + bad := &maintenanceTarget{id: "bad", available: target.Availability{Available: true}, inspection: target.Inspection{State: target.Drifted, Evidence: "managed fragment remains"}, disableErr: failure} + external := &maintenanceTarget{id: "external", available: target.Availability{Available: true}, inspection: target.Inspection{State: target.External, Evidence: "user owned"}} + lock := &recordingLock{} + log := &recordingLog{} + maintenance := app.NewMaintenance(profile.State{}, []target.Target{good, bad, external}, lock, log) + + cancelled := maintenance.Reset(context.Background(), false) + if !cancelled.Cancelled || cancelled.Err != nil || good.disableCalls+bad.disableCalls+external.disableCalls != 0 || lock.acquired != 0 { + t.Fatalf("cancelled reset = %#v, calls %d/%d/%d lock=%d", cancelled, good.disableCalls, bad.disableCalls, external.disableCalls, lock.acquired) + } + + result := maintenance.Reset(context.Background(), true) + if result.Err == nil || len(result.Targets) != 3 || good.disableCalls != 1 || bad.disableCalls != 1 || external.disableCalls != 0 { + t.Fatalf("confirmed reset = %#v, calls %d/%d/%d", result, good.disableCalls, bad.disableCalls, external.disableCalls) + } + if result.Targets[0].Inspection.State != target.Disabled || result.Targets[1].Inspection.State != target.Drifted || !result.Targets[2].Skipped { + t.Fatalf("reset did not preserve per-Target truth: %#v", result.Targets) + } + if lock.acquired != 1 || lock.held != 0 { + t.Fatalf("lock lifecycle acquired=%d held=%d", lock.acquired, lock.held) + } + if len(log.entries) != 2 || strings.Contains(log.entries[1].Detail, "secret") { + t.Fatalf("redacted operation log = %#v", log.entries) + } +} diff --git a/internal/operationlock/lock.go b/internal/operationlock/lock.go new file mode 100644 index 0000000..44f6653 --- /dev/null +++ b/internal/operationlock/lock.go @@ -0,0 +1,84 @@ +// Package operationlock serializes proxyctl state mutations across processes. +package operationlock + +import ( + "context" + "errors" + "fmt" + "os" + "path/filepath" + "time" + + "golang.org/x/sys/unix" + + "proxyctl/internal/securefs" +) + +type Lock struct { + path string + uid int + gid int + owned bool +} + +func New(path string) *Lock { return &Lock{path: path} } +func NewOwned(path string, uid, gid int) *Lock { + return &Lock{path: path, uid: uid, gid: gid, owned: true} +} + +func (l *Lock) Lock(ctx context.Context) (func(), error) { + if !filepath.IsAbs(l.path) { + return nil, errors.New("operation lock path must be absolute") + } + directory := filepath.Dir(l.path) + if err := os.MkdirAll(directory, 0o700); err != nil { + return nil, fmt.Errorf("create operation lock directory: %w", err) + } + if err := os.Chmod(directory, 0o700); err != nil { + return nil, fmt.Errorf("secure operation lock directory: %w", err) + } + if l.owned && (os.Geteuid() != l.uid || os.Getegid() != l.gid) { + if err := os.Chown(directory, l.uid, l.gid); err != nil { + return nil, fmt.Errorf("set operation lock directory owner: %w", err) + } + } + file, err := securefs.OpenNoFollow(l.path, unix.O_CREAT|unix.O_RDWR|unix.O_NONBLOCK, 0o600) + if err != nil { + return nil, fmt.Errorf("open operation lock: %w", err) + } + info, err := file.Stat() + if err != nil || !info.Mode().IsRegular() { + file.Close() + if err != nil { + return nil, fmt.Errorf("inspect operation lock: %w", err) + } + return nil, errors.New("operation lock is not a regular file") + } + if err := file.Chmod(0o600); err != nil { + file.Close() + return nil, fmt.Errorf("secure operation lock: %w", err) + } + if l.owned && (os.Geteuid() != l.uid || os.Getegid() != l.gid) { + if err := file.Chown(l.uid, l.gid); err != nil { + file.Close() + return nil, fmt.Errorf("set operation lock owner: %w", err) + } + } + for { + err = unix.Flock(int(file.Fd()), unix.LOCK_EX|unix.LOCK_NB) + if err == nil { + break + } + if !errors.Is(err, unix.EWOULDBLOCK) && !errors.Is(err, unix.EAGAIN) { + file.Close() + return nil, fmt.Errorf("acquire operation lock: %w", err) + } + select { + case <-ctx.Done(): + file.Close() + return nil, ctx.Err() + case <-time.After(10 * time.Millisecond): + } + } + return func() { _ = unix.Flock(int(file.Fd()), unix.LOCK_UN); _ = file.Close() }, nil +} diff --git a/internal/operationlock/lock_test.go b/internal/operationlock/lock_test.go new file mode 100644 index 0000000..fd396bc --- /dev/null +++ b/internal/operationlock/lock_test.go @@ -0,0 +1,30 @@ +package operationlock_test + +import ( + "context" + "testing" + "time" + + "proxyctl/internal/operationlock" +) + +func TestLockSerializesIndependentInstancesAndHonorsContext(t *testing.T) { + path := t.TempDir() + "/proxyctl.lock" + first := operationlock.New(path) + second := operationlock.New(path) + unlock, err := first.Lock(context.Background()) + if err != nil { + t.Fatal(err) + } + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Millisecond) + defer cancel() + if _, err := second.Lock(ctx); err == nil { + t.Fatal("second lock acquired while first held") + } + unlock() + unlockSecond, err := second.Lock(context.Background()) + if err != nil { + t.Fatal(err) + } + unlockSecond() +} diff --git a/internal/oplog/logger.go b/internal/oplog/logger.go new file mode 100644 index 0000000..22765c2 --- /dev/null +++ b/internal/oplog/logger.go @@ -0,0 +1,113 @@ +// Package oplog writes bounded, privacy-safe local operation logs. +package oplog + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "time" + + "proxyctl/internal/app" + "proxyctl/internal/securefs" + + "golang.org/x/sys/unix" +) + +type Logger struct { + path string + maxBytes int64 + rotations int + uid int + gid int + owned bool +} + +func New(path string, maxBytes int64, rotations int) *Logger { + return &Logger{path: path, maxBytes: maxBytes, rotations: rotations} +} +func NewOwned(path string, maxBytes int64, rotations, uid, gid int) *Logger { + return &Logger{path: path, maxBytes: maxBytes, rotations: rotations, uid: uid, gid: gid, owned: true} +} + +func (l *Logger) Write(entry app.LogEntry) error { + if l.maxBytes <= 0 { + return nil + } + directory := filepath.Dir(l.path) + if err := os.MkdirAll(directory, 0o700); err != nil { + return fmt.Errorf("create log directory: %w", err) + } + if err := os.Chmod(directory, 0o700); err != nil { + return fmt.Errorf("secure log directory: %w", err) + } + if l.owned && (os.Geteuid() != l.uid || os.Getegid() != l.gid) { + if err := os.Chown(directory, l.uid, l.gid); err != nil { + return fmt.Errorf("set log directory owner: %w", err) + } + } + detail := strings.NewReplacer("\n", " ", "\r", " ").Replace(app.RedactLogDetail(entry.Detail)) + if entry.Time.IsZero() { + entry.Time = time.Now() + } + line := fmt.Sprintf("time=%s operation=%s target=%s result=%s detail=%s\n", entry.Time.UTC().Format("2006-01-02T15:04:05Z"), field(entry.Operation), field(entry.Target), field(entry.Result), detail) + if int64(len(line)) > l.maxBytes { + truncated := []byte(line[:l.maxBytes]) + if len(truncated) > 0 { + truncated[len(truncated)-1] = '\n' + } + line = string(truncated) + } + if info, err := os.Stat(l.path); err == nil && info.Size()+int64(len(line)) > l.maxBytes { + if err := l.rotate(); err != nil { + return err + } + } else if err != nil && !os.IsNotExist(err) { + return fmt.Errorf("inspect operation log: %w", err) + } + file, err := securefs.OpenNoFollow(l.path, unix.O_CREAT|unix.O_APPEND|unix.O_WRONLY|unix.O_NONBLOCK, 0o600) + if err != nil { + return fmt.Errorf("open operation log: %w", err) + } + defer file.Close() + fileInfo, err := file.Stat() + if err != nil { + return fmt.Errorf("inspect operation log: %w", err) + } + if !fileInfo.Mode().IsRegular() { + return fmt.Errorf("operation log is not a regular file") + } + if err := file.Chmod(0o600); err != nil { + return fmt.Errorf("secure operation log: %w", err) + } + if l.owned && (os.Geteuid() != l.uid || os.Getegid() != l.gid) { + if err := file.Chown(l.uid, l.gid); err != nil { + return fmt.Errorf("set operation log owner: %w", err) + } + } + if _, err := file.WriteString(line); err != nil { + return fmt.Errorf("write operation log: %w", err) + } + return nil +} + +func field(value string) string { + return strings.NewReplacer(" ", "_", "\n", "_", "\r", "_").Replace(value) +} + +func (l *Logger) rotate() error { + if l.rotations <= 0 { + return os.Remove(l.path) + } + _ = os.Remove(fmt.Sprintf("%s.%d", l.path, l.rotations)) + for i := l.rotations - 1; i >= 1; i-- { + oldPath, newPath := fmt.Sprintf("%s.%d", l.path, i), fmt.Sprintf("%s.%d", l.path, i+1) + if err := os.Rename(oldPath, newPath); err != nil && !os.IsNotExist(err) { + return fmt.Errorf("rotate operation log: %w", err) + } + } + if err := os.Rename(l.path, l.path+".1"); err != nil && !os.IsNotExist(err) { + return fmt.Errorf("rotate operation log: %w", err) + } + return nil +} diff --git a/internal/oplog/logger_test.go b/internal/oplog/logger_test.go new file mode 100644 index 0000000..cb7653a --- /dev/null +++ b/internal/oplog/logger_test.go @@ -0,0 +1,49 @@ +package oplog_test + +import ( + "os" + "path/filepath" + "strings" + "testing" + "time" + + "proxyctl/internal/app" + "proxyctl/internal/oplog" +) + +func TestLoggerRotatesAtBoundAndNeverStoresAuthenticatedURL(t *testing.T) { + path := filepath.Join(t.TempDir(), "state", "proxyctl.log") + logger := oplog.New(path, 180, 2) + for i := 0; i < 20; i++ { + err := logger.Write(app.LogEntry{Time: time.Unix(int64(i), 0), Operation: "reset", Target: "fake", Result: "failure", Detail: "request http://alice:secret@proxy.example:8080/private failed"}) + if err != nil { + t.Fatal(err) + } + } + for _, candidate := range []string{path, path + ".1", path + ".2"} { + data, err := os.ReadFile(candidate) + if os.IsNotExist(err) { + continue + } + if err != nil { + t.Fatal(err) + } + if len(data) > 180 { + t.Errorf("%s has %d bytes, max 180", filepath.Base(candidate), len(data)) + } + text := string(data) + if strings.Contains(text, "secret") || strings.Contains(text, "proxy.example:8080/private") { + t.Errorf("log leaked authenticated URL: %q", text) + } + if !strings.Contains(text, "operation=reset") || !strings.Contains(text, "target=fake") || !strings.Contains(text, "result=failure") { + t.Errorf("log lacks operation fields: %q", text) + } + } + matches, err := filepath.Glob(path + "*") + if err != nil { + t.Fatal(err) + } + if len(matches) > 3 { + t.Fatalf("retained %d logs, want at most 3: %v", len(matches), matches) + } +} diff --git a/internal/profile/diagnostic.go b/internal/profile/diagnostic.go new file mode 100644 index 0000000..b49e51a --- /dev/null +++ b/internal/profile/diagnostic.go @@ -0,0 +1,58 @@ +package profile + +import ( + "encoding/json" + "errors" + "fmt" + "io" + "io/fs" + "os" + + "golang.org/x/sys/unix" +) + +// ReadOnlyLoad reads and validates Profile configuration without creating, +// chmodding, chowning, locking, or replacing any filesystem object. +func (s *FileStore) ReadOnlyLoad() (State, error) { + file, err := os.OpenFile(s.path, unix.O_RDONLY|unix.O_NOFOLLOW|unix.O_NONBLOCK, 0) + if errors.Is(err, fs.ErrNotExist) { + return State{}, nil + } + if err != nil { + return State{}, fmt.Errorf("open Profile configuration read-only: %w", err) + } + defer file.Close() + info, err := file.Stat() + if err != nil { + return State{}, fmt.Errorf("inspect Profile configuration: %w", err) + } + if !info.Mode().IsRegular() { + return State{}, errors.New("Profile configuration is not a regular file") + } + data, err := io.ReadAll(file) + if err != nil { + return State{}, fmt.Errorf("read Profile configuration: %w", err) + } + var state State + if err := json.Unmarshal(data, &state); err != nil { + return State{}, fmt.Errorf("parse Profile configuration: %w", err) + } + for _, candidate := range state.Profiles { + if _, err := ValidateAndNormalize(candidate); err != nil { + return State{}, fmt.Errorf("Profile %q is invalid: %w", candidate.Name, err) + } + } + if state.ActiveProfile != "" && indexProfile(state.Profiles, state.ActiveProfile) < 0 { + return State{}, fmt.Errorf("Active Profile %q does not exist", state.ActiveProfile) + } + return state, nil +} + +func indexProfile(profiles []Profile, name string) int { + for i := range profiles { + if profiles[i].Name == name { + return i + } + } + return -1 +} diff --git a/internal/profile/diagnostic_test.go b/internal/profile/diagnostic_test.go new file mode 100644 index 0000000..6aae118 --- /dev/null +++ b/internal/profile/diagnostic_test.go @@ -0,0 +1,35 @@ +package profile_test + +import ( + "os" + "path/filepath" + "testing" + + "proxyctl/internal/profile" +) + +func TestReadOnlyLoadValidatesWithoutRepairingOrCreatingStorage(t *testing.T) { + directory := filepath.Join(t.TempDir(), "proxyctl") + if err := os.MkdirAll(directory, 0o755); err != nil { + t.Fatal(err) + } + path := filepath.Join(directory, "profiles.json") + if err := os.WriteFile(path, []byte(`{"profiles":[{"name":"office","mode":"single","singleUrl":"http://proxy.test:8080","httpUrl":"http://proxy.test:8080","httpsUrl":"http://proxy.test:8080","allUrl":"http://proxy.test:8080"}],"activeProfile":"office"}`), 0o644); err != nil { + t.Fatal(err) + } + store := profile.NewFileStore(path) + state, err := store.ReadOnlyLoad() + if err != nil || state.ActiveProfile != "office" { + t.Fatalf("read-only load state=%#v err=%v", state, err) + } + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if info.Mode().Perm() != 0o644 { + t.Fatalf("read-only load changed mode to %o", info.Mode().Perm()) + } + if _, err := os.Stat(path + ".lock"); !os.IsNotExist(err) { + t.Fatalf("read-only load created lock: %v", err) + } +} diff --git a/internal/shell/shell.go b/internal/shell/shell.go index 5de23c8..411685d 100644 --- a/internal/shell/shell.go +++ b/internal/shell/shell.go @@ -14,6 +14,8 @@ import ( "path/filepath" "strings" + "golang.org/x/sys/unix" + "proxyctl/internal/profile" "proxyctl/internal/securefs" "proxyctl/internal/target" @@ -187,6 +189,46 @@ func (a *Adapter) EnvironmentPath() string { return a.configuration.EnvironmentF func (a *Adapter) RecoveryPath() string { return a.configuration.RecoveryFile } 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 + } +} + func (a *Adapter) Inspect(_ context.Context, active *profile.Profile) target.Inspection { metadata, managed, metadataErr := a.readRecovery() generated, generatedErr := a.files.ReadFile(a.configuration.EnvironmentFile) diff --git a/internal/target/target.go b/internal/target/target.go index ef717fe..86fd8ff 100644 --- a/internal/target/target.go +++ b/internal/target/target.go @@ -38,6 +38,19 @@ type OperationResult struct { Err error } +// Check is one read-only diagnostic observation with the evidence behind it. +type Check struct { + Name string + OK bool + Evidence string +} + +// Diagnosable is the optional Target capability for command and write-authority +// checks which cannot be inferred from availability and inspection alone. +type Diagnosable interface { + DiagnosticChecks(context.Context, *profile.Profile) []Check +} + // Target hides platform paths and commands from the application and TUI. type Target interface { ID() string From b28ebca2d7d8de5c5a97b02fc32150da69af2cf5 Mon Sep 17 00:00:00 2001 From: proxyctl coordinator Date: Fri, 17 Jul 2026 03:50:18 +0800 Subject: [PATCH 2/2] fix: harden maintenance operations --- cmd/proxyctl/main.go | 23 ++++++- cmd/proxyctl/maintenance_test.go | 82 +++++++++++++++++++++++++ internal/operationlock/lock.go | 37 ++---------- internal/operationlock/lock_test.go | 24 ++++++++ internal/oplog/logger.go | 94 ++++++++++++++--------------- internal/oplog/logger_test.go | 22 +++++++ internal/securefs/securefs.go | 55 +++++++++++++++++ internal/shell/shell.go | 4 +- internal/shell/shell_test.go | 15 +++++ 9 files changed, 273 insertions(+), 83 deletions(-) diff --git a/cmd/proxyctl/main.go b/cmd/proxyctl/main.go index a8e05bf..32b05df 100644 --- a/cmd/proxyctl/main.go +++ b/cmd/proxyctl/main.go @@ -51,10 +51,16 @@ func (processEnvironment) LookupEnv(name string) (string, bool) { return os.Look func runWithEnvironment(args []string, input io.Reader, output io.Writer, environment profile.IdentityEnvironment, files shell.FileSystem, executable string) error { profileConfiguration, err := profile.ResolveConfiguration(environment) if err != nil { + if len(args) == 1 && args[0] == "doctor" { + fmt.Fprintf(output, "[FAIL] identity resolution — %s\n", profile.RedactText(err.Error())) + } return err } shellConfiguration, err := shell.ResolveConfiguration(environment) if err != nil { + if len(args) == 1 && args[0] == "doctor" { + fmt.Fprintf(output, "[FAIL] Shell Target configuration — %s\n", profile.RedactText(err.Error())) + } return err } shellTarget := shell.New(shellConfiguration, files, environment) @@ -129,6 +135,9 @@ func runMaintenanceCommand(args []string, input io.Reader, output io.Writer, sto checks = append([]target.Check{profileCheck}, checks...) report := app.NewMaintenance(state, targets, nil, nil).Doctor(context.Background(), checks) fmt.Fprint(output, report.String()) + if !report.Healthy() { + return errors.New("doctor found configuration problems") + } return nil } if args[0] != "reset" { @@ -141,11 +150,19 @@ func runMaintenanceCommand(args []string, input io.Reader, output io.Writer, sto fmt.Fprintln(output, "Reset cancelled; no Targets changed.") return nil } + if mutationLock == nil { + return errors.New("mutation lock is not configured") + } + unlock, err := mutationLock.Lock(context.Background()) + if err != nil { + return fmt.Errorf("lock proxyctl mutation: %w", err) + } + defer unlock() state, err := store.Load() if err != nil { return err } - result := app.NewMaintenance(state, targets, mutationLock, logger).Reset(context.Background(), true) + result := app.NewMaintenance(state, targets, heldMutationLock{}, logger).Reset(context.Background(), true) for _, item := range result.Targets { if item.Skipped { fmt.Fprintf(output, "%s: skipped (%s) — %s\n", item.TargetID, item.Inspection.State, profile.RedactText(item.Inspection.Evidence)) @@ -160,6 +177,10 @@ func runMaintenanceCommand(args []string, input io.Reader, output io.Writer, sto return result.Err } +type heldMutationLock struct{} + +func (heldMutationLock) Lock(context.Context) (func(), error) { return func() {}, nil } + func diagnosticChecks(configuration profile.Configuration) []target.Check { identity := target.Check{Name: "identity resolution", OK: true, Evidence: fmt.Sprintf("configuration owner %s (uid %d, gid %d)", configuration.Owner.Username, configuration.Owner.UID, configuration.Owner.GID)} ok, evidence := storagePermissions(configuration) diff --git a/cmd/proxyctl/maintenance_test.go b/cmd/proxyctl/maintenance_test.go index fef7b72..994bb5b 100644 --- a/cmd/proxyctl/maintenance_test.go +++ b/cmd/proxyctl/maintenance_test.go @@ -5,6 +5,7 @@ import ( "context" "errors" "os" + "os/exec" "path/filepath" "strings" "testing" @@ -15,6 +16,16 @@ import ( "proxyctl/internal/target" ) +type failingIdentityEnvironment struct{} + +func (failingIdentityEnvironment) CurrentUser() (profile.UserIdentity, error) { + return profile.UserIdentity{}, errors.New("identity lookup failed") +} +func (failingIdentityEnvironment) LookupUser(string) (profile.UserIdentity, error) { + return profile.UserIdentity{}, os.ErrNotExist +} +func (failingIdentityEnvironment) LookupEnv(string) (string, bool) { return "", false } + type processFakeTarget struct { id string state target.State @@ -66,6 +77,24 @@ func TestDoctorCommandUsesFakeTargetsAndIsolatedXDGWithoutMutation(t *testing.T) } } +func TestDoctorReportsIdentityResolutionFailureStructurally(t *testing.T) { + var output bytes.Buffer + err := runWithEnvironment([]string{"doctor"}, strings.NewReader(""), &output, failingIdentityEnvironment{}, nil, "/test/proxyctl") + if err == nil || !strings.Contains(output.String(), "[FAIL] identity resolution") || !strings.Contains(output.String(), "identity lookup failed") { + t.Fatalf("doctor identity failure err=%v output=%q", err, output.String()) + } +} + +func TestDoctorReturnsFailureWhenAnyCheckFails(t *testing.T) { + store := profile.NewFileStore(filepath.Join(t.TempDir(), "profiles.json")) + fake := &processFakeTarget{id: "fake", state: target.Drifted} + var output bytes.Buffer + err := runMaintenanceCommand([]string{"doctor"}, strings.NewReader(""), &output, store, []target.Target{fake}, nil, nil, []target.Check{{Name: "identity", OK: true, Evidence: "ok"}}) + if err == nil || !strings.Contains(output.String(), "[FAIL]") { + t.Fatalf("unhealthy doctor err=%v output=%q", err, output.String()) + } +} + func TestDoctorStoragePermissionCheckIsReadOnly(t *testing.T) { path := filepath.Join(t.TempDir(), "proxyctl", "profiles.json") writeConfig(t, path, `{"profiles":[]}`) @@ -86,6 +115,59 @@ func TestDoctorStoragePermissionCheckIsReadOnly(t *testing.T) { } } +func TestMaintenanceCommandsRunThroughBuiltBinaryWithIsolatedXDG(t *testing.T) { + root := t.TempDir() + binary := filepath.Join(root, "proxyctl") + build := exec.Command("go", "build", "-o", binary, ".") + if output, err := build.CombinedOutput(); err != nil { + t.Fatalf("build proxyctl: %v\n%s", err, output) + } + environment := isolatedProcessEnvironment(root) + + doctor := exec.Command(binary, "doctor") + doctor.Env = environment + doctorOutput, doctorErr := doctor.CombinedOutput() + if doctorErr == nil || !strings.Contains(string(doctorOutput), "[FAIL]") || !strings.Contains(string(doctorOutput), "Target availability") { + t.Fatalf("doctor process err=%v output=%q", doctorErr, doctorOutput) + } + + cancel := exec.Command(binary, "reset") + cancel.Env = environment + cancel.Stdin = strings.NewReader("no\n") + cancelOutput, err := cancel.CombinedOutput() + if err != nil || !strings.Contains(string(cancelOutput), "Reset cancelled") { + t.Fatalf("cancel process err=%v output=%q", err, cancelOutput) + } + + confirmed := exec.Command(binary, "reset") + confirmed.Env = environment + confirmed.Stdin = strings.NewReader("yes\n") + confirmedOutput, err := confirmed.CombinedOutput() + if err != nil || !strings.Contains(string(confirmedOutput), "shell: skipped (Disabled)") { + t.Fatalf("confirmed process err=%v output=%q", err, confirmedOutput) + } + if _, err := os.Stat(filepath.Join(root, "home", ".bashrc")); !os.IsNotExist(err) { + t.Fatalf("process smoke test touched a Shell startup file: %v", err) + } +} + +func isolatedProcessEnvironment(root string) []string { + blocked := map[string]bool{"HOME": true, "XDG_CONFIG_HOME": true, "XDG_STATE_HOME": true, "SHELL": true, "SUDO_USER": true, "HTTP_PROXY": true, "http_proxy": true, "HTTPS_PROXY": true, "https_proxy": true, "ALL_PROXY": true, "all_proxy": true, "NO_PROXY": true, "no_proxy": true} + environment := make([]string, 0, len(os.Environ())+4) + for _, item := range os.Environ() { + name, _, _ := strings.Cut(item, "=") + if !blocked[name] { + environment = append(environment, item) + } + } + return append(environment, + "HOME="+filepath.Join(root, "home"), + "XDG_CONFIG_HOME="+filepath.Join(root, "config"), + "XDG_STATE_HOME="+filepath.Join(root, "state"), + "SHELL=/bin/fish", + ) +} + func TestResetCommandCancellationAndTruthfulPartialFailureWithFakeTargets(t *testing.T) { root := t.TempDir() config := filepath.Join(root, "xdg-config", "proxyctl", "profiles.json") diff --git a/internal/operationlock/lock.go b/internal/operationlock/lock.go index 44f6653..c7c952a 100644 --- a/internal/operationlock/lock.go +++ b/internal/operationlock/lock.go @@ -5,7 +5,6 @@ import ( "context" "errors" "fmt" - "os" "path/filepath" "time" @@ -30,39 +29,13 @@ func (l *Lock) Lock(ctx context.Context) (func(), error) { if !filepath.IsAbs(l.path) { return nil, errors.New("operation lock path must be absolute") } - directory := filepath.Dir(l.path) - if err := os.MkdirAll(directory, 0o700); err != nil { - return nil, fmt.Errorf("create operation lock directory: %w", err) + store := securefs.Store{TrustedRoot: securefs.ExistingTrustedRoot(l.path)} + if l.owned { + store.Owner = &securefs.Owner{UID: l.uid, GID: l.gid} } - if err := os.Chmod(directory, 0o700); err != nil { - return nil, fmt.Errorf("secure operation lock directory: %w", err) - } - if l.owned && (os.Geteuid() != l.uid || os.Getegid() != l.gid) { - if err := os.Chown(directory, l.uid, l.gid); err != nil { - return nil, fmt.Errorf("set operation lock directory owner: %w", err) - } - } - file, err := securefs.OpenNoFollow(l.path, unix.O_CREAT|unix.O_RDWR|unix.O_NONBLOCK, 0o600) + file, err := store.OpenRegular(l.path, unix.O_CREAT|unix.O_RDWR|unix.O_NONBLOCK, 0o600) if err != nil { - return nil, fmt.Errorf("open operation lock: %w", err) - } - info, err := file.Stat() - if err != nil || !info.Mode().IsRegular() { - file.Close() - if err != nil { - return nil, fmt.Errorf("inspect operation lock: %w", err) - } - return nil, errors.New("operation lock is not a regular file") - } - if err := file.Chmod(0o600); err != nil { - file.Close() - return nil, fmt.Errorf("secure operation lock: %w", err) - } - if l.owned && (os.Geteuid() != l.uid || os.Getegid() != l.gid) { - if err := file.Chown(l.uid, l.gid); err != nil { - file.Close() - return nil, fmt.Errorf("set operation lock owner: %w", err) - } + return nil, fmt.Errorf("open operation lock securely: %w", err) } for { err = unix.Flock(int(file.Fd()), unix.LOCK_EX|unix.LOCK_NB) diff --git a/internal/operationlock/lock_test.go b/internal/operationlock/lock_test.go index fd396bc..06aa54b 100644 --- a/internal/operationlock/lock_test.go +++ b/internal/operationlock/lock_test.go @@ -2,12 +2,36 @@ package operationlock_test import ( "context" + "os" + "path/filepath" "testing" "time" "proxyctl/internal/operationlock" ) +func TestLockRejectsSymlinkedStateAncestor(t *testing.T) { + root := t.TempDir() + victim := filepath.Join(root, "victim") + if err := os.Mkdir(victim, 0o700); err != nil { + t.Fatal(err) + } + if err := os.Symlink(victim, filepath.Join(root, "state")); err != nil { + t.Fatal(err) + } + lock := operationlock.New(filepath.Join(root, "state", "proxyctl", "operation.lock")) + if _, err := lock.Lock(context.Background()); err == nil { + t.Fatal("lock followed a symlinked state ancestor") + } + entries, err := os.ReadDir(victim) + if err != nil { + t.Fatal(err) + } + if len(entries) != 0 { + t.Fatalf("victim changed: %v", entries) + } +} + func TestLockSerializesIndependentInstancesAndHonorsContext(t *testing.T) { path := t.TempDir() + "/proxyctl.lock" first := operationlock.New(path) diff --git a/internal/oplog/logger.go b/internal/oplog/logger.go index 22765c2..dcaf5c5 100644 --- a/internal/oplog/logger.go +++ b/internal/oplog/logger.go @@ -2,16 +2,15 @@ package oplog import ( + "errors" "fmt" + "io/fs" "os" - "path/filepath" "strings" "time" "proxyctl/internal/app" "proxyctl/internal/securefs" - - "golang.org/x/sys/unix" ) type Logger struct { @@ -34,18 +33,6 @@ func (l *Logger) Write(entry app.LogEntry) error { if l.maxBytes <= 0 { return nil } - directory := filepath.Dir(l.path) - if err := os.MkdirAll(directory, 0o700); err != nil { - return fmt.Errorf("create log directory: %w", err) - } - if err := os.Chmod(directory, 0o700); err != nil { - return fmt.Errorf("secure log directory: %w", err) - } - if l.owned && (os.Geteuid() != l.uid || os.Getegid() != l.gid) { - if err := os.Chown(directory, l.uid, l.gid); err != nil { - return fmt.Errorf("set log directory owner: %w", err) - } - } detail := strings.NewReplacer("\n", " ", "\r", " ").Replace(app.RedactLogDetail(entry.Detail)) if entry.Time.IsZero() { entry.Time = time.Now() @@ -58,56 +45,65 @@ func (l *Logger) Write(entry app.LogEntry) error { } line = string(truncated) } - if info, err := os.Stat(l.path); err == nil && info.Size()+int64(len(line)) > l.maxBytes { - if err := l.rotate(); err != nil { + store := l.store() + current, err := readOptional(store, l.path) + if err != nil { + return fmt.Errorf("read operation log securely: %w", err) + } + if int64(len(current)+len(line)) > l.maxBytes { + if err := l.rotate(store, current); err != nil { return err } - } else if err != nil && !os.IsNotExist(err) { - return fmt.Errorf("inspect operation log: %w", err) + current = nil } - file, err := securefs.OpenNoFollow(l.path, unix.O_CREAT|unix.O_APPEND|unix.O_WRONLY|unix.O_NONBLOCK, 0o600) - if err != nil { - return fmt.Errorf("open operation log: %w", err) - } - defer file.Close() - fileInfo, err := file.Stat() - if err != nil { - return fmt.Errorf("inspect operation log: %w", err) - } - if !fileInfo.Mode().IsRegular() { - return fmt.Errorf("operation log is not a regular file") - } - if err := file.Chmod(0o600); err != nil { - return fmt.Errorf("secure operation log: %w", err) - } - if l.owned && (os.Geteuid() != l.uid || os.Getegid() != l.gid) { - if err := file.Chown(l.uid, l.gid); err != nil { - return fmt.Errorf("set operation log owner: %w", err) - } - } - if _, err := file.WriteString(line); err != nil { - return fmt.Errorf("write operation log: %w", err) + current = append(current, line...) + if err := store.AtomicWrite(l.path, current, 0o600); err != nil { + return fmt.Errorf("write operation log atomically: %w", err) } return nil } +func (l *Logger) store() securefs.Store { + store := securefs.Store{TrustedRoot: securefs.ExistingTrustedRoot(l.path)} + if l.owned { + store.Owner = &securefs.Owner{UID: l.uid, GID: l.gid} + } + return store +} + func field(value string) string { return strings.NewReplacer(" ", "_", "\n", "_", "\r", "_").Replace(value) } -func (l *Logger) rotate() error { +func (l *Logger) rotate(store securefs.Store, current []byte) error { if l.rotations <= 0 { - return os.Remove(l.path) + return nil } - _ = os.Remove(fmt.Sprintf("%s.%d", l.path, l.rotations)) - for i := l.rotations - 1; i >= 1; i-- { - oldPath, newPath := fmt.Sprintf("%s.%d", l.path, i), fmt.Sprintf("%s.%d", l.path, i+1) - if err := os.Rename(oldPath, newPath); err != nil && !os.IsNotExist(err) { + for i := l.rotations; i >= 2; i-- { + previous := fmt.Sprintf("%s.%d", l.path, i-1) + data, err := readOptional(store, previous) + if err != nil { + return fmt.Errorf("read rotated operation log: %w", err) + } + if data == nil { + continue + } + if err := store.AtomicWrite(fmt.Sprintf("%s.%d", l.path, i), data, 0o600); err != nil { return fmt.Errorf("rotate operation log: %w", err) } } - if err := os.Rename(l.path, l.path+".1"); err != nil && !os.IsNotExist(err) { - return fmt.Errorf("rotate operation log: %w", err) + if current != nil { + if err := store.AtomicWrite(l.path+".1", current, 0o600); err != nil { + return fmt.Errorf("rotate operation log: %w", err) + } } return nil } + +func readOptional(store securefs.Store, path string) ([]byte, error) { + data, err := store.ReadFile(path) + if errors.Is(err, fs.ErrNotExist) || errors.Is(err, os.ErrNotExist) { + return nil, nil + } + return data, err +} diff --git a/internal/oplog/logger_test.go b/internal/oplog/logger_test.go index cb7653a..b5af955 100644 --- a/internal/oplog/logger_test.go +++ b/internal/oplog/logger_test.go @@ -11,6 +11,28 @@ import ( "proxyctl/internal/oplog" ) +func TestLoggerRejectsSymlinkedStateAncestor(t *testing.T) { + root := t.TempDir() + victim := filepath.Join(root, "victim") + if err := os.Mkdir(victim, 0o700); err != nil { + t.Fatal(err) + } + if err := os.Symlink(victim, filepath.Join(root, "state")); err != nil { + t.Fatal(err) + } + logger := oplog.New(filepath.Join(root, "state", "proxyctl", "operations.log"), 1024, 1) + if err := logger.Write(app.LogEntry{Operation: "reset", Result: "failure"}); err == nil { + t.Fatal("logger followed a symlinked state ancestor") + } + entries, err := os.ReadDir(victim) + if err != nil { + t.Fatal(err) + } + if len(entries) != 0 { + t.Fatalf("victim changed: %v", entries) + } +} + func TestLoggerRotatesAtBoundAndNeverStoresAuthenticatedURL(t *testing.T) { path := filepath.Join(t.TempDir(), "state", "proxyctl.log") logger := oplog.New(path, 180, 2) diff --git a/internal/securefs/securefs.go b/internal/securefs/securefs.go index ad882ac..01777ea 100644 --- a/internal/securefs/securefs.go +++ b/internal/securefs/securefs.go @@ -18,6 +18,22 @@ import ( type Owner struct{ UID, GID int } +// ExistingTrustedRoot returns the nearest existing ancestor without resolving +// symlinks. Descriptor traversal will subsequently reject a symlink ancestor. +func ExistingTrustedRoot(path string) string { + candidate := filepath.Dir(filepath.Clean(path)) + for { + if _, err := os.Lstat(candidate); err == nil || !errors.Is(err, fs.ErrNotExist) { + return candidate + } + parent := filepath.Dir(candidate) + if parent == candidate { + return candidate + } + candidate = parent + } +} + func OpenNoFollow(path string, flags int, mode uint32) (*os.File, error) { fd, err := unix.Open(path, flags|unix.O_CLOEXEC|unix.O_NOFOLLOW, mode) if err != nil { @@ -55,6 +71,45 @@ type Store struct { Owner *Owner } +// OpenRegular opens a regular file through a descriptor-anchored parent path. +// Parent components and the final file are never followed through symlinks. +func (s Store) OpenRegular(path string, flags int, mode os.FileMode) (*os.File, error) { + directory, base, err := s.openParent(path, flags&unix.O_CREAT != 0) + if err != nil { + return nil, err + } + if err := directory.Chmod(0o700); err != nil { + _ = directory.Close() + return nil, err + } + if s.Owner != nil && (s.Owner.UID != os.Geteuid() || s.Owner.GID != os.Getegid()) { + if err := directory.Chown(s.Owner.UID, s.Owner.GID); err != nil { + _ = directory.Close() + return nil, err + } + } + file, err := OpenAt(directory, base, flags, uint32(mode.Perm())) + _ = directory.Close() + if err != nil { + return nil, err + } + if err := regular(file); err != nil { + _ = file.Close() + return nil, err + } + if err := file.Chmod(mode.Perm()); err != nil { + _ = file.Close() + return nil, err + } + if s.Owner != nil && (s.Owner.UID != os.Geteuid() || s.Owner.GID != os.Getegid()) { + if err := file.Chown(s.Owner.UID, s.Owner.GID); err != nil { + _ = file.Close() + return nil, err + } + } + return file, nil +} + func (s Store) ReadFile(path string) ([]byte, error) { directory, base, err := s.openParent(path, false) if err != nil { diff --git a/internal/shell/shell.go b/internal/shell/shell.go index 411685d..8f2e02d 100644 --- a/internal/shell/shell.go +++ b/internal/shell/shell.go @@ -256,7 +256,9 @@ func (a *Adapter) Inspect(_ context.Context, active *profile.Profile) target.Ins } func (a *Adapter) SetDesired(ctx context.Context, enabled bool, active *profile.Profile) target.OperationResult { - if !a.Availability(ctx).Available { + // Enabling requires a supported current Shell, but disabling must remain + // available so reset can remove managed state after the user's Shell changes. + if enabled && !a.Availability(ctx).Available { return target.OperationResult{Inspection: a.Inspect(ctx, active), Err: errors.New("Shell Target is unavailable: Bash or Zsh was not detected")} } if enabled && active == nil { diff --git a/internal/shell/shell_test.go b/internal/shell/shell_test.go index 68cf8ee..bb9313e 100644 --- a/internal/shell/shell_test.go +++ b/internal/shell/shell_test.go @@ -472,6 +472,21 @@ func TestRecoveryAndGeneratedFilesAreAtomicOwnerOnly(t *testing.T) { } } +func TestDisableManagedShellAfterCurrentShellBecomesUnsupported(t *testing.T) { + root := t.TempDir() + configuration := shell.Configuration{Shell: "/bin/bash", StartupFile: filepath.Join(root, ".bashrc"), EnvironmentFile: filepath.Join(root, "config", "shell.env"), RecoveryFile: filepath.Join(root, "state", "shell.json")} + enabled := shell.New(configuration, shell.OSFileSystem{}, shell.MapEnvironment{}) + if result := enabled.SetDesired(context.Background(), true, activeProfile()); result.Err != nil { + t.Fatal(result.Err) + } + configuration.Shell = "/bin/fish" + unsupported := shell.New(configuration, shell.OSFileSystem{}, shell.MapEnvironment{}) + result := unsupported.SetDesired(context.Background(), false, nil) + if result.Err != nil || result.Inspection.State != target.Disabled { + t.Fatalf("disable from unsupported Shell = %#v", result) + } +} + func activeProfile() *profile.Profile { return &profile.Profile{Name: "office", Mode: profile.AdvancedMode, HTTPURL: "http://alice:secret@proxy.example:8080", HTTPSURL: "https://proxy.example:8443", AllURL: "socks5h://proxy.example:1080", Bypass: []string{"localhost", ".example.com", "10.0.0.0/8"}} }