diff --git a/internal/dockerclient/dockerclient.go b/internal/dockerclient/dockerclient.go index e183b97..55136a8 100644 --- a/internal/dockerclient/dockerclient.go +++ b/internal/dockerclient/dockerclient.go @@ -16,6 +16,7 @@ import ( "path/filepath" "strings" + "proxyctl/internal/pathutil" "proxyctl/internal/profile" "proxyctl/internal/securefs" "proxyctl/internal/target" @@ -79,24 +80,7 @@ func ResolveConfiguration(environment profile.IdentityEnvironment) (Configuratio }, nil } -func ResolveConfigurationForCurrentProcess() (Configuration, error) { - return ResolveConfiguration(systemEnvironment{}) -} - -type systemEnvironment struct{} - -func (systemEnvironment) CurrentUser() (profile.UserIdentity, error) { - return profile.SystemCurrentUser() -} -func (systemEnvironment) LookupUser(name string) (profile.UserIdentity, error) { - return profile.SystemLookupUser(name) -} -func (systemEnvironment) LookupEnv(name string) (string, bool) { return os.LookupEnv(name) } - -func pathWithin(path, parent string) bool { - relative, err := filepath.Rel(parent, path) - return err == nil && relative != ".." && !strings.HasPrefix(relative, ".."+string(filepath.Separator)) -} +func pathWithin(path, parent string) bool { return pathutil.Within(path, parent) } type Ownership struct{ UID, GID int } diff --git a/internal/gnome/gnome.go b/internal/gnome/gnome.go index 0883758..9e6b00b 100644 --- a/internal/gnome/gnome.go +++ b/internal/gnome/gnome.go @@ -20,6 +20,7 @@ import ( "syscall" "unicode/utf8" + "proxyctl/internal/pathutil" "proxyctl/internal/profile" "proxyctl/internal/securefs" "proxyctl/internal/target" @@ -194,24 +195,7 @@ func ResolveConfiguration(environment profile.IdentityEnvironment) (Configuratio return Configuration{CommandName: "gsettings", UID: owner.UID, GID: owner.GID, Username: owner.Username, HomeDir: owner.HomeDir, ConfigDir: configDir, DataDir: dataDir, CacheDir: cacheDir, RuntimeDir: runtimeDir, SessionBus: sessionBus, StateFile: filepath.Join(stateDir, "proxyctl", "gnome.json"), TrustedRoot: trustedRoot}, nil } -func ResolveConfigurationForCurrentProcess() (Configuration, error) { - return ResolveConfiguration(systemEnvironment{}) -} - -type systemEnvironment struct{} - -func (systemEnvironment) CurrentUser() (profile.UserIdentity, error) { - return profile.SystemCurrentUser() -} -func (systemEnvironment) LookupUser(name string) (profile.UserIdentity, error) { - return profile.SystemLookupUser(name) -} -func (systemEnvironment) LookupEnv(name string) (string, bool) { return os.LookupEnv(name) } - -func pathWithin(path, parent string) bool { - relative, err := filepath.Rel(parent, path) - return err == nil && relative != ".." && !strings.HasPrefix(relative, ".."+string(filepath.Separator)) -} +func pathWithin(path, parent string) bool { return pathutil.Within(path, parent) } type settings struct { Mode string `json:"mode"` diff --git a/internal/pathutil/pathutil.go b/internal/pathutil/pathutil.go new file mode 100644 index 0000000..617ed27 --- /dev/null +++ b/internal/pathutil/pathutil.go @@ -0,0 +1,14 @@ +// Package pathutil contains lexical path-policy helpers. It does not replace +// descriptor-anchored filesystem access for security-sensitive operations. +package pathutil + +import ( + "path/filepath" + "strings" +) + +// Within reports whether path is parent itself or a lexical descendant. +func Within(path, parent string) bool { + relative, err := filepath.Rel(parent, path) + return err == nil && relative != ".." && !strings.HasPrefix(relative, ".."+string(filepath.Separator)) +} diff --git a/internal/pathutil/pathutil_test.go b/internal/pathutil/pathutil_test.go new file mode 100644 index 0000000..e38ea37 --- /dev/null +++ b/internal/pathutil/pathutil_test.go @@ -0,0 +1,29 @@ +package pathutil_test + +import ( + "path/filepath" + "testing" + + "proxyctl/internal/pathutil" +) + +func TestWithinUsesComponentBoundaries(t *testing.T) { + parent := filepath.Join(string(filepath.Separator), "home", "alice") + for _, test := range []struct { + name string + path string + want bool + }{ + {name: "same", path: parent, want: true}, + {name: "child", path: filepath.Join(parent, ".config", "proxyctl"), want: true}, + {name: "cleaned child", path: filepath.Join(parent, "tmp", "..", ".config"), want: true}, + {name: "parent", path: filepath.Dir(parent), want: false}, + {name: "sibling prefix", path: parent + "-backup", want: false}, + } { + t.Run(test.name, func(t *testing.T) { + if got := pathutil.Within(test.path, parent); got != test.want { + t.Fatalf("Within(%q, %q) = %v, want %v", test.path, parent, got, test.want) + } + }) + } +} diff --git a/internal/profile/store.go b/internal/profile/store.go index e0245bf..48c30ff 100644 --- a/internal/profile/store.go +++ b/internal/profile/store.go @@ -14,6 +14,7 @@ import ( "golang.org/x/sys/unix" + "proxyctl/internal/pathutil" "proxyctl/internal/securefs" ) @@ -184,10 +185,7 @@ func ConfigPathWith(environment IdentityEnvironment) (string, error) { return configuration.Path, err } -func pathWithin(path, parent string) bool { - relative, err := filepath.Rel(parent, path) - return err == nil && relative != ".." && !strings.HasPrefix(relative, ".."+string(filepath.Separator)) -} +func pathWithin(path, parent string) bool { return pathutil.Within(path, parent) } type CommitError struct { Err error diff --git a/internal/securefs/securefs.go b/internal/securefs/securefs.go index db4fc90..cc6542c 100644 --- a/internal/securefs/securefs.go +++ b/internal/securefs/securefs.go @@ -14,6 +14,8 @@ import ( "strings" "golang.org/x/sys/unix" + + "proxyctl/internal/pathutil" ) type Owner struct{ UID, GID int } @@ -341,7 +343,4 @@ func regular(file *os.File) error { return nil } -func within(path, root string) bool { - relative, err := filepath.Rel(root, path) - return err == nil && relative != ".." && !strings.HasPrefix(relative, ".."+string(filepath.Separator)) -} +func within(path, root string) bool { return pathutil.Within(path, root) } diff --git a/internal/securefs/securefs_test.go b/internal/securefs/securefs_test.go new file mode 100644 index 0000000..06060e1 --- /dev/null +++ b/internal/securefs/securefs_test.go @@ -0,0 +1,68 @@ +package securefs_test + +import ( + "errors" + "os" + "path/filepath" + "testing" + + "proxyctl/internal/securefs" +) + +func TestAtomicWriteCreatesAndReplacesRegularFile(t *testing.T) { + root := t.TempDir() + path := filepath.Join(root, "state", "config.json") + store := securefs.Store{TrustedRoot: root} + if err := store.AtomicWrite(path, []byte("first\n"), 0o640); err != nil { + t.Fatal(err) + } + if err := store.AtomicWrite(path, []byte("second\n"), 0o600); err != nil { + t.Fatal(err) + } + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if string(data) != "second\n" || info.Mode().Perm() != 0o600 { + t.Fatalf("content=%q mode=%o", data, info.Mode().Perm()) + } +} + +func TestAtomicWriteRejectsSymlinkWithoutChangingVictim(t *testing.T) { + root := t.TempDir() + victim := filepath.Join(root, "victim") + if err := os.WriteFile(victim, []byte("unchanged"), 0o600); err != nil { + t.Fatal(err) + } + path := filepath.Join(root, "managed") + if err := os.Symlink(victim, path); err != nil { + t.Fatal(err) + } + store := securefs.Store{TrustedRoot: root} + if err := store.AtomicWrite(path, []byte("replaced"), 0o600); err == nil { + t.Fatal("AtomicWrite followed a final symlink") + } + data, err := os.ReadFile(victim) + if err != nil { + t.Fatal(err) + } + if string(data) != "unchanged" { + t.Fatalf("victim changed: %q", data) + } +} + +func TestStoreRejectsPathsOutsideTrustedRoot(t *testing.T) { + root := t.TempDir() + outside := filepath.Join(t.TempDir(), "outside") + store := securefs.Store{TrustedRoot: root} + if err := store.AtomicWrite(outside, []byte("data"), 0o600); err == nil { + t.Fatal("AtomicWrite escaped TrustedRoot") + } + if _, err := os.Stat(outside); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("outside path changed: %v", err) + } +} diff --git a/internal/shell/shell.go b/internal/shell/shell.go index 1b81fe9..ea7d6b6 100644 --- a/internal/shell/shell.go +++ b/internal/shell/shell.go @@ -14,6 +14,7 @@ import ( "path/filepath" "strings" + "proxyctl/internal/pathutil" "proxyctl/internal/profile" "proxyctl/internal/securefs" "proxyctl/internal/target" @@ -84,24 +85,7 @@ func ResolveConfiguration(environment profile.IdentityEnvironment) (Configuratio }, nil } -func ResolveConfigurationForCurrentProcess() (Configuration, error) { - return ResolveConfiguration(systemEnvironment{}) -} - -type systemEnvironment struct{} - -func (systemEnvironment) CurrentUser() (profile.UserIdentity, error) { - return profile.SystemCurrentUser() -} -func (systemEnvironment) LookupUser(name string) (profile.UserIdentity, error) { - return profile.SystemLookupUser(name) -} -func (systemEnvironment) LookupEnv(name string) (string, bool) { return os.LookupEnv(name) } - -func pathWithin(path, parent string) bool { - relative, err := filepath.Rel(parent, path) - return err == nil && relative != ".." && !strings.HasPrefix(relative, ".."+string(filepath.Separator)) -} +func pathWithin(path, parent string) bool { return pathutil.Within(path, parent) } type Environment interface{ LookupEnv(string) (string, bool) } type MapEnvironment map[string]string diff --git a/internal/tui/model.go b/internal/tui/model.go index c7d5ea8..d803ea3 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -243,67 +243,79 @@ func driftResolutionAction(selected app.TargetSnapshot, kind app.TargetActionKin return app.TargetAction{Kind: kind}, false } +func (m Model) updateProfilePicker(key tea.KeyMsg) (tea.Model, tea.Cmd) { + switch key.String() { + case "esc": + m.picker.close() + return m, nil + case "enter": + selected, ok := m.picker.selected(m.snapshot) + if !ok { + return m, nil + } + m.picker.close() + return m.beginProfileActivation(app.Intent{Kind: app.ActivateProfile, Name: selected.Name}) + default: + return m, m.picker.update(key, m.snapshot) + } +} + +func (m Model) updateOperationConfirmation(key tea.KeyMsg) (tea.Model, tea.Cmd) { + switch m.operation.confirmationDialog.update(key.String()) { + case confirmationAccepted: + confirmed, ok := m.operation.confirmedIntent() + if !ok { + return m, nil + } + m.status = "" + return m, m.startOperation(confirmed) + case confirmationCancelled: + m.operation.cancelConfirmation() + } + return m, nil +} + +func (m Model) updateDriftResolution(key tea.KeyMsg) (tea.Model, tea.Cmd) { + switch key.String() { + case "enter", "esc", "n", "q": + m.driftResolutionTargetID = "" + return m, nil + case "r", "o": + kind := app.ReapplyActiveProfileAction + if key.String() == "o" { + kind = app.RestoreOriginalAction + } + selected, ok := newTargetCatalog(m.snapshot, true, m.driftResolutionTargetID).selected(m.driftResolutionTargetID) + if !ok { + m.driftResolutionTargetID = "" + m.operation.status = app.OperationStatus{Kind: app.StatusBlocked, Summary: "Drift Resolution is unavailable because the Target is no longer present"} + return m, nil + } + action, ok := driftResolutionAction(selected, kind) + if !ok || !action.Available { + reason := action.DisabledReason + if reason == "" { + reason = "This Drift Resolution action is unavailable" + } + m.operation.status = app.OperationStatus{Kind: app.StatusBlocked, Summary: reason} + return m, nil + } + intent, _ := kind.Intent(selected.ID) + m.driftResolutionTargetID = "" + return m, m.startOperation(intent) + } + return m, nil +} + func (m Model) updateMain(key tea.KeyMsg) (tea.Model, tea.Cmd) { if m.picker.open { - switch key.String() { - case "esc": - m.picker.close() - return m, nil - case "enter": - selected, ok := m.picker.selected(m.snapshot) - if !ok { - return m, nil - } - m.picker.close() - return m.beginProfileActivation(app.Intent{Kind: app.ActivateProfile, Name: selected.Name}) - default: - return m, m.picker.update(key, m.snapshot) - } + return m.updateProfilePicker(key) } if m.operation.confirmation != nil { - switch m.operation.confirmationDialog.update(key.String()) { - case confirmationAccepted: - confirmed, ok := m.operation.confirmedIntent() - if !ok { - return m, nil - } - m.status = "" - return m, m.startOperation(confirmed) - case confirmationCancelled: - m.operation.cancelConfirmation() - } - return m, nil + return m.updateOperationConfirmation(key) } if m.driftResolutionTargetID != "" { - switch key.String() { - case "enter", "esc", "n", "q": - m.driftResolutionTargetID = "" - return m, nil - case "r", "o": - kind := app.ReapplyActiveProfileAction - if key.String() == "o" { - kind = app.RestoreOriginalAction - } - selected, ok := newTargetCatalog(m.snapshot, true, m.driftResolutionTargetID).selected(m.driftResolutionTargetID) - if !ok { - m.driftResolutionTargetID = "" - m.operation.status = app.OperationStatus{Kind: app.StatusBlocked, Summary: "Drift Resolution is unavailable because the Target is no longer present"} - return m, nil - } - action, ok := driftResolutionAction(selected, kind) - if !ok || !action.Available { - reason := action.DisabledReason - if reason == "" { - reason = "This Drift Resolution action is unavailable" - } - m.operation.status = app.OperationStatus{Kind: app.StatusBlocked, Summary: reason} - return m, nil - } - intent, _ := kind.Intent(selected.ID) - m.driftResolutionTargetID = "" - return m, m.startOperation(intent) - } - return m, nil + return m.updateDriftResolution(key) } if key.String() == "e" && m.operation.currentError != nil { m.operation.errorExpanded = !m.operation.errorExpanded