From b28ebca2d7d8de5c5a97b02fc32150da69af2cf5 Mon Sep 17 00:00:00 2001 From: proxyctl coordinator Date: Fri, 17 Jul 2026 03:50:18 +0800 Subject: [PATCH] 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"}} }