From 479f546ebc615e2c1550d43da473ac7e2b5a1642 Mon Sep 17 00:00:00 2001 From: Sahilb315 Date: Tue, 14 Jul 2026 02:40:08 +0530 Subject: [PATCH] fix: actionable remedy for root-created per-user config dir A pmg run as root with a preserved HOME (GitHub runners, sudo -E, su without -) creates the invoking user's ~/.config/safedep as root-owned, and event-log init then fail-closes every later non-root command. Make that state self-solvable: - event-log init permission errors exit with a usefulerror naming the likely cause and the chown fix instead of a bare fatal - pmg setup doctor probes event-log dir writability and reports the same fix via a new per-result Fix override - document the mechanism and remedy in system-install.md, along with the binary ownership requirements for --system - consolidate this branch's doctor tests into doctor_test.go --- cmd/setup/doctor.go | 74 +++++++++++++------ .../{doctor_system_test.go => doctor_test.go} | 48 ++++++++++++ docs/system-install.md | 19 +++-- internal/doctor/doctor.go | 2 + main.go | 25 ++++++- 5 files changed, 139 insertions(+), 29 deletions(-) rename cmd/setup/{doctor_system_test.go => doctor_test.go} (63%) diff --git a/cmd/setup/doctor.go b/cmd/setup/doctor.go index e169691..68c2df6 100644 --- a/cmd/setup/doctor.go +++ b/cmd/setup/doctor.go @@ -7,6 +7,7 @@ import ( "path/filepath" "strings" + "github.com/safedep/dry/log" "github.com/safedep/pmg/config" "github.com/safedep/pmg/internal/alias" "github.com/safedep/pmg/internal/doctor" @@ -94,29 +95,7 @@ func runCoreChecks(cfg *config.RuntimeConfig) []doctor.CheckResult { Name: checkEventLogDir, Category: "Configuration", Run: func() doctor.CheckResult { - if cfg.Config.SkipEventLogging { - return doctor.CheckResult{ - Status: doctor.StatusWarn, - Message: "Event logging is disabled", - } - } - info, err := os.Stat(cfg.EventLogDir()) - if err != nil { - return doctor.CheckResult{ - Status: doctor.StatusFail, - Message: "Event log directory not found", - } - } - if !info.IsDir() { - return doctor.CheckResult{ - Status: doctor.StatusFail, - Message: "Event log path is not a directory", - } - } - return doctor.CheckResult{ - Status: doctor.StatusPass, - Message: "Event log directory found", - } + return checkEventLogDirResult(cfg.Config.SkipEventLogging, cfg.EventLogDir(), cfg.ConfigDir()) }, }, { @@ -278,6 +257,52 @@ func runCoreChecks(cfg *config.RuntimeConfig) []doctor.CheckResult { return doctor.RunChecks(checks) } +// checkEventLogDirResult is the testable core of the event-log dir check. +// Event logging is mandatory (init failure is fatal), so an unwritable dir +// fail-closes every pmg command for this user. The common cause is a root or +// sudo run having created the per-user directory as root, hence the chown fix. +func checkEventLogDirResult(skipEventLogging bool, logDir, configDir string) doctor.CheckResult { + if skipEventLogging { + return doctor.CheckResult{ + Status: doctor.StatusWarn, + Message: "Event logging is disabled", + } + } + info, err := os.Stat(logDir) + if err != nil { + return doctor.CheckResult{ + Status: doctor.StatusFail, + Message: "Event log directory not found", + } + } + if !info.IsDir() { + return doctor.CheckResult{ + Status: doctor.StatusFail, + Message: "Event log path is not a directory", + } + } + + probe, err := os.CreateTemp(logDir, ".pmg-doctor-*") + if err != nil { + return doctor.CheckResult{ + Status: doctor.StatusFail, + Message: "Event log directory not writable", + Fix: fmt.Sprintf("sudo chown -R $(id -un) %s", configDir), + } + } + if err := probe.Close(); err != nil { + log.Warnf("failed to close doctor probe file: %v", err) + } + if err := os.Remove(probe.Name()); err != nil { + log.Warnf("failed to remove doctor probe file: %v", err) + } + + return doctor.CheckResult{ + Status: doctor.StatusPass, + Message: "Event log directory found", + } +} + func pathContainsDir(pathEntries []string, dir string) bool { if dir == "" { return false @@ -520,6 +545,9 @@ func printResults(results []doctor.CheckResult) { fix := ui.Colors.Dim("—") if r.Status != doctor.StatusPass { fix = fixHint(r.Name) + if r.Fix != "" { + fix = r.Fix + } } rows = append(rows, []string{ statusBadge(r.Status), diff --git a/cmd/setup/doctor_system_test.go b/cmd/setup/doctor_test.go similarity index 63% rename from cmd/setup/doctor_system_test.go rename to cmd/setup/doctor_test.go index da033b7..f037908 100644 --- a/cmd/setup/doctor_system_test.go +++ b/cmd/setup/doctor_test.go @@ -1,11 +1,14 @@ package setup import ( + "os" "os/exec" + "path/filepath" "testing" "github.com/safedep/pmg/internal/doctor" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestPathContainsDir(t *testing.T) { @@ -102,3 +105,48 @@ func TestClassifyPackageManagerResolutionsAcceptsEitherShimDir(t *testing.T) { assert.ElementsMatch(t, []string{"npm", "pip"}, under) assert.Equal(t, []string{"yarn"}, shadowed) } + +func TestCheckEventLogDirResult(t *testing.T) { + configDir := "/home/dev/.config/safedep/pmg" + + t.Run("skipped when event logging disabled", func(t *testing.T) { + result := checkEventLogDirResult(true, t.TempDir(), configDir) + assert.Equal(t, doctor.StatusWarn, result.Status) + }) + + t.Run("missing directory fails", func(t *testing.T) { + result := checkEventLogDirResult(false, filepath.Join(t.TempDir(), "absent"), configDir) + assert.Equal(t, doctor.StatusFail, result.Status) + assert.Equal(t, "Event log directory not found", result.Message) + }) + + t.Run("file instead of directory fails", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "logs") + require.NoError(t, os.WriteFile(path, []byte("x"), 0o644)) + + result := checkEventLogDirResult(false, path, configDir) + assert.Equal(t, doctor.StatusFail, result.Status) + }) + + t.Run("writable directory passes", func(t *testing.T) { + result := checkEventLogDirResult(false, t.TempDir(), configDir) + assert.Equal(t, doctor.StatusPass, result.Status) + }) + + t.Run("unwritable directory fails with chown fix", func(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("running as root: directory permissions are not enforced") + } + dir := t.TempDir() + require.NoError(t, os.Chmod(dir, 0o555)) + t.Cleanup(func() { + require.NoError(t, os.Chmod(dir, 0o755)) + }) + + result := checkEventLogDirResult(false, dir, configDir) + assert.Equal(t, doctor.StatusFail, result.Status) + assert.Equal(t, "Event log directory not writable", result.Message) + assert.Contains(t, result.Fix, "sudo chown -R") + assert.Contains(t, result.Fix, configDir) + }) +} diff --git a/docs/system-install.md b/docs/system-install.md index 00579cf..d680b00 100644 --- a/docs/system-install.md +++ b/docs/system-install.md @@ -6,16 +6,17 @@ Use system install when one machine or image should protect every user account: sudo pmg setup install --system ``` -Requires Linux and root. To uninstall: +Requires Linux and root. Because every user's shims execute the PMG binary by its absolute path, `--system` validates it first: the binary must be **root-owned**, world-executable, and not writable by group or others, and it must sit in a **root-owned directory** that is not world-writable. Install PMG as root into a standard path such as `/usr/local/bin`; a user-local build (e.g. `~/go/bin/pmg`) is rejected. + +Per-user `pmg setup install` remains available and does not conflict with a system install. + +To uninstall: ```bash sudo pmg setup remove --system sudo pmg setup remove --system --config-file # also remove the system config file ``` -Per-user `pmg setup install` remains available and does not conflict with a system install. -The PMG executable used during setup must be executable by every user. Install PMG under a system path such as `/usr/local/bin`. - ## Files created @@ -112,10 +113,18 @@ 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. +The invoking user must be able to write their config directory. PMG records an event log there on each run and fails the command if it cannot (unless event logging is disabled in config). 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. +If every `pmg` command fails with `permission denied` on the event log, a root run created the per-user directory as root. This happens where `sudo` preserves `HOME` (GitHub-hosted runners, `sudo -E`, `su` without `-`) and in images that set `ENV HOME` before dropping root. Restore ownership: + +```bash +sudo chown -R $(id -un) ~/.config/safedep +``` + +`pmg setup doctor` detects an unwritable event log directory and prints the same fix. + For cloud sync, enable cloud in the system config and provide credentials (`SAFEDEP_API_KEY` and `SAFEDEP_TENANT_ID`, or a keychain login on developer machines). ## Certificates diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index 23e4c04..2ee8fb4 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -14,6 +14,8 @@ type CheckResult struct { Status CheckStatus Message string ImpliesInterception bool + // Fix overrides the check's static fix hint for this specific result. + Fix string } type Check struct { diff --git a/main.go b/main.go index 3d42efb..64b4f88 100644 --- a/main.go +++ b/main.go @@ -1,12 +1,15 @@ package main import ( + "errors" "fmt" + "io/fs" "os" "runtime" "strings" "github.com/safedep/dry/log" + "github.com/safedep/dry/usefulerror" "github.com/safedep/pmg/cmd/cloud" configCmd "github.com/safedep/pmg/cmd/config" "github.com/safedep/pmg/cmd/executors" @@ -19,6 +22,7 @@ import ( "github.com/safedep/pmg/cmd/setup" "github.com/safedep/pmg/cmd/version" "github.com/safedep/pmg/config" + "github.com/safedep/pmg/errcodes" "github.com/safedep/pmg/internal/analytics" "github.com/safedep/pmg/internal/audit" "github.com/safedep/pmg/internal/eventlog" @@ -103,7 +107,7 @@ func main() { } if eventlogErr != nil { - ui.Fatalf("failed to initialize event logging: %v", eventlogErr) + ui.ErrorExit(eventlogInitError(eventlogErr)) } if err := audit.Initialize(config.Get()); err != nil { @@ -215,6 +219,25 @@ func main() { } } +// eventlogInitError classifies event-log init failures. Event logging is +// mandatory, so init failure stays fatal; the permission case gets an +// actionable remedy because the common cause is a root or sudo run having +// created the per-user directory as root (some environments preserve HOME +// under sudo). +func eventlogInitError(err error) error { + if errors.Is(err, fs.ErrPermission) { + return usefulerror.NewUsefulError(). + WithCode(errcodes.PermissionDenied). + WithHumanError("event logging is required but its directory is not writable"). + WithHelp(fmt.Sprintf("If a root or sudo run created it, restore ownership: sudo chown -R $(id -un) %s", config.Get().ConfigDir())). + Wrap(err) + } + return usefulerror.NewUsefulError(). + WithCode(errcodes.Lifecycle). + WithHumanError("failed to initialize event logging"). + Wrap(err) +} + func logDebugContext() { cfg := config.Get()