From 1276a1ebaaa1e9447a3c6ac813a9cc7a463d6766 Mon Sep 17 00:00:00 2001 From: Sahilb315 Date: Tue, 14 Jul 2026 00:44:44 +0530 Subject: [PATCH] fix: address system-install review findings - shim: make system executable resolution injectable so tests pass under umask 002; skip the root-owner test when running as root - doctor: treat resolution into either the system or per-user shim dir as intercepted, and collapse the shim-in-PATH check to a single call site - setup: make remove (both --system and per-user) best-effort with errors.Join so one failed step no longer strands the other artifact - shim: allow a group-writable install parent dir (Debian/Ubuntu ship /usr/local/bin as root:staff 2775) while still rejecting world-writable and non-root-owned parents - audit: attribute cloud events to SUDO_USER when running under sudo - docs: drop the soft-fail event-logging claim (hard-fail is retained) --- cmd/setup/doctor.go | 55 ++++++++++++++++++++++++--------- cmd/setup/doctor_system_test.go | 25 ++++++++++++++- cmd/setup/setup.go | 36 ++++++++++++++------- cmd/setup/setup_test.go | 2 +- docs/system-install.md | 2 +- internal/audit/cloud_sink.go | 18 +++++++++-- internal/shim/shim.go | 20 ++++++------ internal/shim/system.go | 26 +++++++++++----- internal/shim/system_test.go | 13 ++++++++ 9 files changed, 150 insertions(+), 47 deletions(-) diff --git a/cmd/setup/doctor.go b/cmd/setup/doctor.go index 3068b4b..e169691 100644 --- a/cmd/setup/doctor.go +++ b/cmd/setup/doctor.go @@ -103,7 +103,7 @@ func runCoreChecks(cfg *config.RuntimeConfig) []doctor.CheckResult { info, err := os.Stat(cfg.EventLogDir()) if err != nil { return doctor.CheckResult{ - Status: doctor.StatusWarn, + Status: doctor.StatusFail, Message: "Event log directory not found", } } @@ -304,14 +304,18 @@ func pathIsUnderDir(path, dir string) bool { return strings.HasPrefix(cleanPath, prefix) } -func classifyPackageManagerResolutions(packageManagers []string, shimDir string, lookPath func(string) (string, error)) (underShim, shadowed []string) { +// classifyPackageManagerResolutions splits package managers by where they +// resolve on PATH: underShim means the command runs through a pmg shim (so it +// is intercepted), shadowed means a real npm/pip sits ahead of the shims (so +// interception is bypassed and the user should be warned). +func classifyPackageManagerResolutions(packageManagers []string, shimDirs []string, lookPath func(string) (string, error)) (underShim, shadowed []string) { for _, pm := range packageManagers { resolved, err := lookPath(pm) if err != nil { continue } - if pathIsUnderDir(resolved, shimDir) { + if resolvesUnderAny(resolved, shimDirs) { underShim = append(underShim, pm) continue } @@ -320,10 +324,30 @@ func classifyPackageManagerResolutions(packageManagers []string, shimDir string, return underShim, shadowed } +func resolvesUnderAny(path string, dirs []string) bool { + for _, dir := range dirs { + if pathIsUnderDir(path, dir) { + return true + } + } + return false +} + +// shimDirs lists every directory a package manager may legitimately resolve +// into. System and per-user installs are supported side by side, so resolution +// into either shim directory counts as intercepted rather than shadowed. +func shimDirs() []string { + dirs := []string{shim.SystemBinDir()} + if userDir, err := shim.UserBinDir(); err == nil { + dirs = append(dirs, userDir) + } + return dirs +} + func checkShimDirResolution(shimDir, pathLabel string, pathEntries []string) doctor.CheckResult { underShim, shadowed := classifyPackageManagerResolutions( alias.DefaultConfig().PackageManagers, - shimDir, + shimDirs(), exec.LookPath, ) @@ -362,18 +386,21 @@ func checkShimDirResolution(shimDir, pathLabel string, pathEntries []string) doc func checkShimInPathResult() doctor.CheckResult { pathEntries := filepath.SplitList(os.Getenv("PATH")) - if shim.SystemShimsInstalled() { - return checkShimDirResolution(shim.SystemBinDir(), "System shim directory", pathEntries) + // The primary directory only picks the label and PATH-membership target; + // classification accepts resolution into either shim dir regardless. + shimDir, pathLabel := shim.SystemBinDir(), "System shim directory" + if !shim.SystemShimsInstalled() { + userDir, err := shim.UserBinDir() + if err != nil { + return doctor.CheckResult{ + Status: doctor.StatusWarn, + Message: fmt.Sprintf("Could not resolve shim directory: %v", err), + } + } + shimDir, pathLabel = userDir, "Shim directory" } - userDir, err := shim.UserBinDir() - if err != nil { - return doctor.CheckResult{ - Status: doctor.StatusWarn, - Message: fmt.Sprintf("Could not resolve shim directory: %v", err), - } - } - return checkShimDirResolution(userDir, "Shim directory", pathEntries) + return checkShimDirResolution(shimDir, pathLabel, pathEntries) } func runProtectionChecks(coreResults []doctor.CheckResult) []doctor.CheckResult { diff --git a/cmd/setup/doctor_system_test.go b/cmd/setup/doctor_system_test.go index 3d198fb..da033b7 100644 --- a/cmd/setup/doctor_system_test.go +++ b/cmd/setup/doctor_system_test.go @@ -73,9 +73,32 @@ func TestClassifyPackageManagerResolutions(t *testing.T) { under, shadowed := classifyPackageManagerResolutions( []string{"npm", "pip", "uv"}, - shimDir, + []string{shimDir}, lookPath, ) assert.Equal(t, []string{"npm"}, under) assert.Equal(t, []string{"pip"}, shadowed) } + +func TestClassifyPackageManagerResolutionsAcceptsEitherShimDir(t *testing.T) { + systemDir := "/usr/local/lib/pmg/bin" + userDir := "/home/dev/.pmg/bin" + lookPath := func(name string) (string, error) { + switch name { + case "npm": + return systemDir + "/npm", nil + case "pip": + return userDir + "/pip", nil + default: + return "/usr/bin/" + name, nil + } + } + + under, shadowed := classifyPackageManagerResolutions( + []string{"npm", "pip", "yarn"}, + []string{systemDir, userDir}, + lookPath, + ) + assert.ElementsMatch(t, []string{"npm", "pip"}, under) + assert.Equal(t, []string{"yarn"}, shadowed) +} diff --git a/cmd/setup/setup.go b/cmd/setup/setup.go index 08133e2..3b5af04 100644 --- a/cmd/setup/setup.go +++ b/cmd/setup/setup.go @@ -152,15 +152,21 @@ func remove(system, removeConfig bool) error { return removeSystem(removeConfig) } + // Best-effort: attempt every cleanup step so one failure does not strand the + // other artifacts and force a rerun. + var errs []error if removeConfig { // Only ever remove the per-user file; the globally managed // config is not ours to delete from a per-user uninstall. if err := config.RemoveUserConfigFile(); err != nil { - return err + errs = append(errs, err) } } if runtime.GOOS == "windows" { + if len(errs) > 0 { + return errors.Join(errs...) + } fmt.Printf("%s %s\n", ui.Colors.Green("✓"), "PMG config removed. No aliases or shims to clean up on Windows.") return nil } @@ -168,21 +174,20 @@ func remove(system, removeConfig bool) error { cfg := alias.DefaultConfig() rcFileManager, err := alias.NewDefaultRcFileManager(cfg.RcFileName) if err != nil { - return err - } - - aliasManager := alias.New(cfg, rcFileManager) - if err := aliasManager.Remove(); err != nil { - return fmt.Errorf("failed to remove aliases: %w", err) + errs = append(errs, err) + } else if err := alias.New(cfg, rcFileManager).Remove(); err != nil { + errs = append(errs, fmt.Errorf("failed to remove aliases: %w", err)) } shimMgr, err := shim.NewDefaultShimManager() if err != nil { - return fmt.Errorf("failed to create shim manager: %w", err) + errs = append(errs, fmt.Errorf("failed to create shim manager: %w", err)) + } else if err := shimMgr.Remove(); err != nil { + errs = append(errs, fmt.Errorf("failed to remove shims: %w", err)) } - if err := shimMgr.Remove(); err != nil { - return fmt.Errorf("failed to remove shims: %w", err) + if len(errs) > 0 { + return errors.Join(errs...) } fmt.Printf("%s %s\n", ui.Colors.Green("✓"), "PMG aliases and shims removed. Restart your terminal for changes to take effect") @@ -199,14 +204,21 @@ func removeSystem(removeConfig bool) error { return fmt.Errorf("failed to create system shim manager: %w", err) } + // Best-effort: attempt config removal even if shim removal fails, so one + // failed step does not strand the other artifact and force a rerun. Shims + // are removed first so interception stops before the managed config goes. + var errs []error if err := shimMgr.Remove(); err != nil { - return fmt.Errorf("failed to remove system shims: %w", err) + errs = append(errs, fmt.Errorf("failed to remove system shims: %w", err)) } if removeConfig { if err := config.RemoveSystemConfigFile(); err != nil { - return err + errs = append(errs, err) } } + if len(errs) > 0 { + return errors.Join(errs...) + } fmt.Printf("%s %s\n", ui.Colors.Green("✓"), "PMG system install removed") return nil diff --git a/cmd/setup/setup_test.go b/cmd/setup/setup_test.go index a881b77..38aa3df 100644 --- a/cmd/setup/setup_test.go +++ b/cmd/setup/setup_test.go @@ -10,7 +10,7 @@ import ( "github.com/stretchr/testify/require" ) -func TestErrIfSystemInstallAllowed(t *testing.T) { +func TestRequireSystemInstallSupported(t *testing.T) { orig := setupGeteuid t.Cleanup(func() { setupGeteuid = orig }) diff --git a/docs/system-install.md b/docs/system-install.md index 9247bcf..00579cf 100644 --- a/docs/system-install.md +++ b/docs/system-install.md @@ -112,7 +112,7 @@ Shared policy lives under `/etc/safedep/pmg`. Runtime data stays per user: You can relocate these with `PMG_CONFIG_DIR` and `PMG_CACHE_DIR`. -The invoking user must be able to write their config directory. If they cannot, PMG skips event logging for that run (and prints a warning) and continues the package-manager command. Cloud sync also needs that directory to store pending events. +The invoking user must be able to write their config directory. In Docker images, avoid creating `/home//.config/safedep` as root during the build. Either fix ownership for the runtime user, or set `PMG_CONFIG_DIR` to a writable location. diff --git a/internal/audit/cloud_sink.go b/internal/audit/cloud_sink.go index 68face1..7dfaf94 100644 --- a/internal/audit/cloud_sink.go +++ b/internal/audit/cloud_sink.go @@ -95,8 +95,7 @@ func (s *cloudSink) buildInvocationContext() *controltowerv1.EndpointInvocationC ctx.SetCommand(s.command) ctx.SetWorkingDirectory(s.workingDir) - u, err := user.Current() - if err == nil { + if u := invokingUser(); u != nil { ctx.SetUsername(u.Username) ctx.SetUsernameUid(u.Uid) } @@ -119,6 +118,21 @@ func (s *cloudSink) buildInvocationContext() *controltowerv1.EndpointInvocationC return ctx } +// invokingUser resolves the human behind the command, preferring SUDO_USER so a +// `sudo npm ...` is attributed to the operator rather than root. +func invokingUser() *user.User { + if name := os.Getenv("SUDO_USER"); name != "" { + if u, err := user.Lookup(name); err == nil { + return u + } + } + u, err := user.Current() + if err != nil { + return nil + } + return u +} + func buildCommand(packageManager string, args []string) string { if packageManager == "" { return "" diff --git a/internal/shim/shim.go b/internal/shim/shim.go index 5b71f7a..69743f5 100644 --- a/internal/shim/shim.go +++ b/internal/shim/shim.go @@ -1,6 +1,7 @@ package shim import ( + "errors" "fmt" "os" "path/filepath" @@ -99,25 +100,26 @@ func (m *ShimManager) Install() error { } func (m *ShimManager) Remove() error { + // Best-effort: a failure removing the shim directory must not skip profile + // and rc cleanup, otherwise a rerun is needed to fully uninstall. + var errs []error if err := os.RemoveAll(m.config.BinDir); err != nil { - return fmt.Errorf("failed to remove shim directory %s: %w", m.config.BinDir, err) + errs = append(errs, fmt.Errorf("failed to remove shim directory %s: %w", m.config.BinDir, err)) } if m.config.ManageProfile { if err := removeSystemProfile(); err != nil { - return fmt.Errorf("failed to remove system profile: %w", err) + errs = append(errs, fmt.Errorf("failed to remove system profile: %w", err)) } } - if m.config.SkipShellRc { - return nil + if !m.config.SkipShellRc { + if err := m.removePathFromShells(); err != nil { + errs = append(errs, fmt.Errorf("failed to clean shell configs: %w", err)) + } } - if err := m.removePathFromShells(); err != nil { - return fmt.Errorf("failed to clean shell configs: %w", err) - } - - return nil + return errors.Join(errs...) } func (m *ShimManager) IsInstalled() (bool, error) { diff --git a/internal/shim/system.go b/internal/shim/system.go index 58ae892..a9bed00 100644 --- a/internal/shim/system.go +++ b/internal/shim/system.go @@ -23,6 +23,10 @@ var ( // systemExecutableOwnershipCheck requires root ownership of the binary and // its parent directory. Disabled in tests that cannot create root-owned files. systemExecutableOwnershipCheck = true + // resolveExecutable resolves the running pmg binary for system install. + // Overridable in tests so validation does not run against the go-build test + // binary, which is group-writable under a 002 umask. + resolveExecutable = currentExecutable ) // SystemBinDir returns the directory for system-wide PMG shims. @@ -57,7 +61,7 @@ func NewSystemShimManagerForRemove() (*ShimManager, error) { func newSystemShimManager(validateExecutable bool) (*ShimManager, error) { aliasCfg := alias.DefaultConfig() - pmgBin, err := currentExecutable() + pmgBin, err := resolveExecutable() if err != nil { return nil, err } @@ -81,8 +85,8 @@ func newSystemShimManager(validateExecutable bool) (*ShimManager, error) { // validateSystemExecutable rejects binaries unsafe for system-wide shims. // Shims hard-code this path, so the binary must be executable by all users, -// not writable by group/others, and owned by root in a root-owned parent -// directory that is not writable by group/others. +// not writable by group/others, and owned by root in a root-owned, non-world- +// writable parent. func validateSystemExecutable(path string) error { info, err := os.Stat(path) if err != nil { @@ -126,16 +130,24 @@ func requireRootOwnedPath(path string, info os.FileInfo) error { return nil } +// requireSafeParentDir validates only the immediate parent of the executable, +// not the full chain up to /. It requires a root-owned, non-world-writable +// parent so an unprivileged account cannot swap the shared binary that every +// user's shims exec; a maliciously writable grandparent is out of scope. +// +// Group-writable is allowed deliberately: Debian/Ubuntu ship /usr/local/bin as +// root:staff mode 2775, so rejecting group-writable would refuse the documented +// install location out of the box. The tradeoff is that a member of the parent +// directory's group can replace the binary — harden the directory (chmod g-w) +// on multi-user hosts where that group is not trusted. func requireSafeParentDir(dir string) error { info, err := os.Stat(dir) if err != nil { return fmt.Errorf("failed to inspect directory %s: %w", dir, err) } - groupOrOtherWrite := os.FileMode(0o022) - - if info.Mode().Perm()&groupOrOtherWrite != 0 { - return fmt.Errorf("directory %s containing pmg executable is writable by group or others", dir) + if info.Mode().Perm()&os.FileMode(0o002) != 0 { + return fmt.Errorf("directory %s containing pmg executable is writable by others", dir) } uid, ok := fileOwnerUID(info) diff --git a/internal/shim/system_test.go b/internal/shim/system_test.go index 6f577d4..ff4e571 100644 --- a/internal/shim/system_test.go +++ b/internal/shim/system_test.go @@ -14,10 +14,19 @@ func useSystemPaths(t *testing.T, dir string) { systemBinDirOverride = filepath.Join(dir, "bin") systemProfilePathOverride = filepath.Join(dir, "profile.d", "pmg.sh") systemExecutableOwnershipCheck = false + + // The go-build test binary is group-writable under a 002 umask, which the + // executable validation rightly rejects. Point resolution at a crafted + // 0755 binary so the manager validates a realistic path, not the harness. + exe := filepath.Join(dir, "pmg") + require.NoError(t, os.WriteFile(exe, []byte("#!/bin/sh\n"), 0o755)) + resolveExecutable = func() (string, error) { return exe, nil } + t.Cleanup(func() { systemBinDirOverride = "" systemProfilePathOverride = "" systemExecutableOwnershipCheck = true + resolveExecutable = currentExecutable }) } @@ -142,6 +151,10 @@ func TestValidateSystemExecutableRejectsGroupWritable(t *testing.T) { } func TestValidateSystemExecutableRejectsNonRootOwner(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("running as root: temp file is root-owned, so the owner check passes") + } + dir := t.TempDir() path := filepath.Join(dir, "pmg") require.NoError(t, os.WriteFile(path, []byte("binary"), 0o755))