From e36b1c42016f376be559952b87f8d912966c5a51 Mon Sep 17 00:00:00 2001 From: Matthew DeVenny Date: Mon, 6 Apr 2026 12:56:47 -0700 Subject: [PATCH 1/3] #37 lockdown other grants in sudoers.d Signed-off-by: Matthew DeVenny --- .github/workflows/ci.yml | 363 +++++++++++++++++++++++++ design.md | 2 +- pkg/lockdown/sudo.go | 553 ++++++++++++++++++++++++++++++++++---- pkg/lockdown/sudo_test.go | 325 ++++++++++++++++++++-- 4 files changed, 1164 insertions(+), 79 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b7116c1..5575252 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -294,6 +294,369 @@ jobs: curl -sf --connect-timeout 10 --max-time 30 https://example.com > /dev/null echo "✅ example.com allowed in audit mode" + test-sudo-lockdown: + name: Integration Test (Sudo Lockdown) + needs: build-binary + runs-on: ubuntu-latest + permissions: + contents: read + actions: read + steps: + - uses: actions/checkout@v6 + + - name: Download binary + uses: actions/download-artifact@v8 + with: + name: cargowall-binary + path: bin + + - name: Make binary executable + run: chmod +x bin/cargowall + + - name: Snapshot pre-lockdown sudo state + run: | + echo "--- Runner user info ---" + id runner + echo "--- Groups ---" + groups runner + echo "--- sudoers.d contents ---" + sudo ls -la /etc/sudoers.d/ + echo "--- Verify runner has full sudo ---" + sudo whoami + echo "--- Save sudoers.d file list for later comparison ---" + sudo ls /etc/sudoers.d/ | sort > /tmp/sudoers-before.txt + + - name: Start CargoWall with sudo lockdown + run: | + # Start cargowall inside a root wrapper that watches for a stop + # trigger. Once lockdown is active the runner user cannot sudo kill, + # so we need an already-root process that can send SIGTERM. + sudo bash -c ' + ./bin/cargowall start \ + --github-action \ + --audit-mode \ + --sudo-lockdown \ + --sudo-allow-commands "/usr/sbin/iptables,/usr/sbin/ip6tables" \ + --dns-upstream "8.8.8.8:53" & + CW_PID=$! + echo $CW_PID > /tmp/cargowall-pid + # Wait for stop trigger (created by a later step — no sudo needed) + while [ ! -f /tmp/cargowall-stop ]; do sleep 1; done + kill -TERM $CW_PID + wait $CW_PID 2>/dev/null + ' & + + # Wait for cargowall to be ready + for i in $(seq 1 30); do + if [ -f /tmp/cargowall-ready ]; then + echo "CargoWall ready after ${i}s" + break + fi + sleep 1 + done + if [ ! -f /tmp/cargowall-ready ]; then + echo "CargoWall did not become ready" + exit 1 + fi + + - name: Test allowed sudo command works + run: | + if sudo /usr/sbin/iptables -L -n > /dev/null 2>&1; then + echo "✅ Allowed command (iptables) works via sudo" + else + echo "❌ Allowed command (iptables) failed via sudo" + exit 1 + fi + + - name: Test blocked sudo command is denied + run: | + OUTPUT=$(sudo -n /usr/bin/apt-get --version 2>&1) && { + echo "❌ apt-get should be blocked by sudo lockdown" + exit 1 + } + if echo "$OUTPUT" | grep -qiE "not allowed|not in sudoers|may not run|a password is required"; then + echo "✅ Blocked command (apt-get) denied by sudo" + else + echo "❌ apt-get failed but not due to sudoers denial: $OUTPUT" + exit 1 + fi + + - name: Test sudo bash is denied + run: | + OUTPUT=$(sudo -n /bin/bash -c "echo pwned" 2>&1) && { + echo "❌ sudo bash should be blocked" + exit 1 + } + if echo "$OUTPUT" | grep -qiE "not allowed|not in sudoers|may not run|a password is required"; then + echo "✅ sudo bash denied" + else + echo "❌ bash failed but not due to sudoers denial: $OUTPUT" + exit 1 + fi + + - name: Test sudo kill is denied + run: | + OUTPUT=$(sudo -n /usr/bin/kill -0 1 2>&1) && { + echo "❌ sudo kill should be blocked (not in allow list)" + exit 1 + } + if echo "$OUTPUT" | grep -qiE "not allowed|not in sudoers|may not run|a password is required"; then + echo "✅ sudo kill denied" + else + echo "❌ kill failed but not due to sudoers denial: $OUTPUT" + exit 1 + fi + + - name: Stop CargoWall and verify cleanup + run: | + CW_PID=$(cat /tmp/cargowall-pid) + + # Signal the root wrapper to send SIGTERM — no sudo needed + touch /tmp/cargowall-stop + + # Wait for cargowall to exit. Use /proc to check since kill -0 + # requires permission on a root-owned process. + for i in $(seq 1 15); do + if [ ! -d "/proc/$CW_PID" ]; then + echo "CargoWall stopped after ${i}s" + break + fi + sleep 1 + done + if [ -d "/proc/$CW_PID" ]; then + echo "❌ CargoWall did not stop within 15s" + exit 1 + fi + + - name: Verify sudo access restored + run: | + echo "--- sudoers.d after cleanup ---" + sudo ls -la /etc/sudoers.d/ + + # Lockdown file should be removed + if sudo test -f /etc/sudoers.d/zz-cargowall-lockdown; then + echo "❌ Lockdown sudoers file still exists after cleanup" + exit 1 + fi + echo "✅ Lockdown sudoers file removed" + + # State file should be removed + if sudo test -f /etc/sudoers.d/.cargowall-lockdown-state; then + echo "❌ State file still exists after cleanup" + exit 1 + fi + echo "✅ State file removed" + + # Disabled files should be restored + STILL_DISABLED=$(sudo ls /etc/sudoers.d/ | grep -c '\.cargowall-disabled$' || true) + if [ "$STILL_DISABLED" -gt 0 ]; then + echo "❌ $STILL_DISABLED sudoers files still disabled after cleanup" + sudo ls -la /etc/sudoers.d/ + exit 1 + fi + echo "✅ All disabled sudoers files restored" + + # sudoers.d should be back to original state + sudo ls /etc/sudoers.d/ | sort > /tmp/sudoers-after.txt + if ! diff /tmp/sudoers-before.txt /tmp/sudoers-after.txt; then + echo "❌ sudoers.d contents differ from pre-lockdown state" + exit 1 + fi + echo "✅ sudoers.d contents match pre-lockdown state" + + - name: Verify runner has full sudo again + run: | + if sudo whoami > /dev/null 2>&1; then + echo "✅ Full sudo access restored" + else + echo "❌ sudo access not restored after cleanup" + exit 1 + fi + + # Verify unrestricted commands work again + if sudo apt-get --version > /dev/null 2>&1; then + echo "✅ Previously blocked commands now work" + else + echo "❌ apt-get still blocked after cleanup" + exit 1 + fi + + - name: Emergency cleanup + if: always() + run: | + # Signal the root wrapper to stop cargowall gracefully + touch /tmp/cargowall-stop + sleep 5 + # If we still have sudo, clean up any remnants + if sudo -n true 2>/dev/null; then + sudo rm -f /etc/sudoers.d/zz-cargowall-lockdown + sudo rm -f /etc/sudoers.d/.cargowall-lockdown-state + for f in /etc/sudoers.d/*.cargowall-disabled; do + [ -f "$f" ] || continue + sudo mv "$f" "${f%.cargowall-disabled}" + done + sudo gpasswd -a runner sudo 2>/dev/null || true + fi + echo "Emergency cleanup complete" + + test-sudo-lockdown-full-block: + name: Integration Test (Sudo Lockdown - Full Block) + needs: build-binary + runs-on: ubuntu-latest + permissions: + contents: read + actions: read + steps: + - uses: actions/checkout@v6 + + - name: Download binary + uses: actions/download-artifact@v8 + with: + name: cargowall-binary + path: bin + + - name: Make binary executable + run: chmod +x bin/cargowall + + - name: Snapshot pre-lockdown sudo state + run: | + sudo ls /etc/sudoers.d/ | sort > /tmp/sudoers-before.txt + + - name: Start CargoWall with sudo lockdown (no allowed commands) + run: | + sudo bash -c ' + ./bin/cargowall start \ + --github-action \ + --audit-mode \ + --sudo-lockdown \ + --dns-upstream "8.8.8.8:53" & + CW_PID=$! + echo $CW_PID > /tmp/cargowall-pid + while [ ! -f /tmp/cargowall-stop ]; do sleep 1; done + kill -TERM $CW_PID + wait $CW_PID 2>/dev/null + ' & + + for i in $(seq 1 30); do + if [ -f /tmp/cargowall-ready ]; then + echo "CargoWall ready after ${i}s" + break + fi + sleep 1 + done + if [ ! -f /tmp/cargowall-ready ]; then + echo "CargoWall did not become ready" + exit 1 + fi + + - name: Test iptables is denied (no commands allowed) + run: | + OUTPUT=$(sudo -n /usr/sbin/iptables -L -n 2>&1) && { + echo "❌ iptables should be blocked with no allowed commands" + exit 1 + } + if echo "$OUTPUT" | grep -qiE "not allowed|not in sudoers|may not run|a password is required"; then + echo "✅ iptables denied (full block mode)" + else + echo "❌ iptables failed but not due to sudoers denial: $OUTPUT" + exit 1 + fi + + - name: Test apt-get is denied + run: | + OUTPUT=$(sudo -n /usr/bin/apt-get --version 2>&1) && { + echo "❌ apt-get should be blocked" + exit 1 + } + if echo "$OUTPUT" | grep -qiE "not allowed|not in sudoers|may not run|a password is required"; then + echo "✅ apt-get denied (full block mode)" + else + echo "❌ apt-get failed but not due to sudoers denial: $OUTPUT" + exit 1 + fi + + - name: Test sudo bash is denied + run: | + OUTPUT=$(sudo -n /bin/bash -c "echo pwned" 2>&1) && { + echo "❌ sudo bash should be blocked" + exit 1 + } + if echo "$OUTPUT" | grep -qiE "not allowed|not in sudoers|may not run|a password is required"; then + echo "✅ sudo bash denied (full block mode)" + else + echo "❌ bash failed but not due to sudoers denial: $OUTPUT" + exit 1 + fi + + - name: Stop CargoWall and verify cleanup + run: | + CW_PID=$(cat /tmp/cargowall-pid) + touch /tmp/cargowall-stop + for i in $(seq 1 15); do + if [ ! -d "/proc/$CW_PID" ]; then + echo "CargoWall stopped after ${i}s" + break + fi + sleep 1 + done + if [ -d "/proc/$CW_PID" ]; then + echo "❌ CargoWall did not stop within 15s" + exit 1 + fi + + - name: Verify sudo access restored + run: | + if sudo test -f /etc/sudoers.d/zz-cargowall-lockdown; then + echo "❌ Lockdown sudoers file still exists after cleanup" + exit 1 + fi + echo "✅ Lockdown sudoers file removed" + + if sudo test -f /etc/sudoers.d/.cargowall-lockdown-state; then + echo "❌ State file still exists after cleanup" + exit 1 + fi + echo "✅ State file removed" + + STILL_DISABLED=$(sudo ls /etc/sudoers.d/ | grep -c '\.cargowall-disabled$' || true) + if [ "$STILL_DISABLED" -gt 0 ]; then + echo "❌ $STILL_DISABLED sudoers files still disabled after cleanup" + exit 1 + fi + echo "✅ All disabled sudoers files restored" + + sudo ls /etc/sudoers.d/ | sort > /tmp/sudoers-after.txt + if ! diff /tmp/sudoers-before.txt /tmp/sudoers-after.txt; then + echo "❌ sudoers.d contents differ from pre-lockdown state" + exit 1 + fi + echo "✅ sudoers.d contents match pre-lockdown state" + + - name: Verify runner has full sudo again + run: | + if sudo whoami > /dev/null 2>&1; then + echo "✅ Full sudo access restored" + else + echo "❌ sudo access not restored after cleanup" + exit 1 + fi + + - name: Emergency cleanup + if: always() + run: | + touch /tmp/cargowall-stop + sleep 5 + if sudo -n true 2>/dev/null; then + sudo rm -f /etc/sudoers.d/zz-cargowall-lockdown + sudo rm -f /etc/sudoers.d/.cargowall-lockdown-state + for f in /etc/sudoers.d/*.cargowall-disabled; do + [ -f "$f" ] || continue + sudo mv "$f" "${f%.cargowall-disabled}" + done + sudo gpasswd -a runner sudo 2>/dev/null || true + fi + echo "Emergency cleanup complete" + test-cargowall-saas: name: Integration Test (SaaS) needs: build-binary diff --git a/design.md b/design.md index a20e5c9..23f4512 100644 --- a/design.md +++ b/design.md @@ -444,6 +444,6 @@ The DNS proxy handles Kubernetes service discovery by: ## GitHub Actions Integration - **DNS redirect:** iptables DNAT rules redirect all outbound DNS (port 53) to `127.0.0.1:53`, exempting the proxy's own upstream queries via `SO_MARK` (`0xCA12`) -- **Sudo lockdown:** writes `/etc/sudoers.d/cargowall-lockdown` with a NOPASSWD allowlist; removes the runner user from the `docker` group +- **Sudo lockdown:** writes `/etc/sudoers.d/zz-cargowall-lockdown` with a NOPASSWD allowlist; removes the runner user from sudo-granting groups (`sudo`, `admin`, `wheel`) and the `docker` group; disables competing sudoers.d files by renaming them to `*.cargowall-disabled` - **Auto-infrastructure:** `EnsureInfraAllowed()` and `EnsureHostnameAllowed()` add rules for platform services (Azure IMDS, GitHub API, etc.) - **Logging:** `slog.Handler` that formats messages as GitHub workflow commands (`::error::`, `::warning::`, `::debug::`) diff --git a/pkg/lockdown/sudo.go b/pkg/lockdown/sudo.go index c551b52..6a1c0ef 100644 --- a/pkg/lockdown/sudo.go +++ b/pkg/lockdown/sudo.go @@ -17,21 +17,27 @@ package lockdown import ( + "encoding/json" "errors" "fmt" "log/slog" "os" "os/exec" "os/user" + "path/filepath" "slices" "strings" ) const ( - sudoersDir = "/etc/sudoers.d" - sudoersFile = "/etc/sudoers.d/cargowall-lockdown" + sudoersDir = "/etc/sudoers.d" + sudoersFile = "/etc/sudoers.d/zz-cargowall-lockdown" + stateFile = "/etc/sudoers.d/.cargowall-lockdown-state" + disabledSuffix = ".cargowall-disabled" ) +var sudoGrantingGroups = []string{"sudo", "admin", "wheel"} + // SudoLockdownConfig configures the sudo lockdown behavior type SudoLockdownConfig struct { // AllowCommands is a list of command paths to whitelist via NOPASSWD @@ -40,12 +46,23 @@ type SudoLockdownConfig struct { Username string } -// sudoersUnsafeChars are characters that could be used for sudoers injection. +// lockdownState records changes made during EnableSudoLockdown so they can be +// reversed by DisableSudoLockdown. +type lockdownState struct { + Username string `json:"username"` + RemovedGroups []string `json:"removedGroups"` + DisabledFiles []string `json:"disabledFiles"` + DockerRemoved bool `json:"dockerRemoved"` +} + const sudoersUnsafeChars = " ,#\n\r\t`;&!()\\|" // validateSudoersInput checks that a username and command paths do not contain // characters that could alter sudoers semantics. func validateSudoersInput(username string, cmds []string) error { + if username == "" { + return fmt.Errorf("sudoers username must not be empty") + } if strings.ContainsAny(username, sudoersUnsafeChars) { return fmt.Errorf("sudoers username %q contains unsafe characters", username) } @@ -57,43 +74,402 @@ func validateSudoersInput(username string, cmds []string) error { return nil } -// EnableSudoLockdown configures sudoers to restrict what commands can be run with sudo -// and removes the target user from the docker group. -func EnableSudoLockdown(cfg *SudoLockdownConfig, logger *slog.Logger) error { - logger.Info("Enabling sudo lockdown") +// resolveUsername determines the target username from config or auto-detection. +// When cfgUsername is set it is returned directly. Otherwise the current user +// is used, falling back to common CI usernames when running as root. +func resolveUsername(cfgUsername string) (string, error) { + if cfgUsername != "" { + return cfgUsername, nil + } + currentUser, err := user.Current() + if err != nil { + return "", fmt.Errorf("failed to get current user: %w", err) + } + if currentUser.Uid != "0" { + return currentUser.Username, nil + } + // Running as root — try common CI usernames + for _, u := range []string{"runner", "github", "ci"} { + if _, err := user.Lookup(u); err == nil { + return u, nil + } + } + return "", fmt.Errorf("could not determine target username for sudo lockdown") +} - // Determine the target username - username := cfg.Username - if username == "" { - // Try to detect the current non-root user - currentUser, err := user.Current() +// lookupUserGIDs returns the set of group IDs the user belongs to. +func lookupUserGIDs(username string) (map[string]bool, error) { + u, err := user.Lookup(username) + if err != nil { + return nil, err + } + gids, err := u.GroupIds() + if err != nil { + return nil, err + } + set := make(map[string]bool, len(gids)) + for _, gid := range gids { + set[gid] = true + } + return set, nil +} + +// removeFromSudoGroups removes the user from sudo-granting groups and returns +// the list of groups the user was actually removed from. Returns an error if +// any group removal fails. Groups that don't exist or that the user is not a +// member of are skipped (checked via os/user, not gpasswd output parsing). +func removeFromSudoGroups(username string, logger *slog.Logger) ([]string, error) { + userGIDs, gidErr := lookupUserGIDs(username) + if gidErr != nil { + logger.Warn("Failed to look up user groups, skipping group removal", + "username", username, "error", gidErr) + } + + var removed []string + var failures []string + for _, group := range sudoGrantingGroups { + grp, err := user.LookupGroup(group) + if err != nil || userGIDs == nil || !userGIDs[grp.Gid] { + logger.Debug("User not in group or group does not exist, skipping", + "username", username, "group", group) + continue + } + out, err := exec.Command("gpasswd", "-d", username, group).CombinedOutput() + if err != nil { + logger.Warn("Failed to remove user from group", + "username", username, "group", group, + "error", err, "output", strings.TrimSpace(string(out))) + failures = append(failures, group) + continue + } + logger.Info("Removed user from group", "username", username, "group", group) + removed = append(removed, group) + } + if len(failures) > 0 { + return removed, fmt.Errorf("failed to remove user from groups: %v", failures) + } + return removed, nil +} + +// restoreToGroups re-adds the user to the given groups. Returns true if all +// groups were restored successfully (or the list was empty). +func restoreToGroups(username string, groups []string, logger *slog.Logger) bool { + allOk := true + for _, group := range groups { + out, err := exec.Command("gpasswd", "-a", username, group).CombinedOutput() if err != nil { - return fmt.Errorf("failed to get current user: %w", err) - } - if currentUser.Uid == "0" { - // We're root, try common CI usernames - for _, u := range []string{"runner", "github", "ci"} { - if _, err := user.Lookup(u); err == nil { - username = u - break - } + logger.Warn("Failed to re-add user to group", + "username", username, "group", group, + "error", err, "output", strings.TrimSpace(string(out))) + allOk = false + continue + } + logger.Info("Restored user to group", "username", username, "group", group) + } + return allOk +} + +// disableCompetingSudoersFiles scans /etc/sudoers.d/ for files that contain +// direct grants for the given username and renames them to *.cargowall-disabled. +// Returns the list of original file paths that were disabled and an error if +// any matching file could not be disabled (lockdown may be incomplete). +func disableCompetingSudoersFiles(dir, username string, logger *slog.Logger) ([]string, error) { + entries, err := os.ReadDir(dir) + if err != nil { + return nil, fmt.Errorf("failed to read sudoers.d directory: %w", err) + } + + // Resolve user's group memberships once for all file checks. + userGIDs, gidErr := lookupUserGIDs(username) + if gidErr != nil { + logger.Warn("Failed to look up user groups for sudoers scan, %group detection disabled", + "username", username, "error", gidErr) + } + + var disabled []string + var renameFailed []string + for _, entry := range entries { + if entry.IsDir() { + continue + } + name := entry.Name() + // Skip our own files and already-disabled files + if name == filepath.Base(sudoersFile) || + name == filepath.Base(stateFile) || + strings.HasSuffix(name, ".tmp") || + strings.HasSuffix(name, disabledSuffix) { + continue + } + + path := filepath.Join(dir, name) + grants, readErr := sudoersFileGrantsUser(path, username, userGIDs) + if readErr != nil { + logger.Warn("Cannot read sudoers file, treating as potential grant", + "path", path, "error", readErr) + renameFailed = append(renameFailed, path) + continue + } + if !grants { + continue + } + + disabledPath := path + disabledSuffix + if _, err := os.Lstat(disabledPath); err == nil { + logger.Warn("Disabled target already exists, cannot safely disable sudoers file", + "path", path, "disabledPath", disabledPath) + renameFailed = append(renameFailed, path) + continue + } else if !errors.Is(err, os.ErrNotExist) { + logger.Warn("Failed to check disabled sudoers file path", + "path", path, "disabledPath", disabledPath, "error", err) + renameFailed = append(renameFailed, path) + continue + } + if err := os.Rename(path, disabledPath); err != nil { + logger.Warn("Failed to disable sudoers file", + "path", path, "error", err) + renameFailed = append(renameFailed, path) + continue + } + logger.Info("Disabled competing sudoers file", "path", path) + disabled = append(disabled, path) + } + if len(renameFailed) > 0 { + return disabled, fmt.Errorf("failed to disable competing sudoers files: %v", renameFailed) + } + return disabled, nil +} + +// sudoersFileGrantsUser returns true if the file contains a non-comment line +// that either: +// - starts with the username followed by whitespace (direct grant), +// - contains the username in a User_Alias definition (indirect grant via alias), or +// - contains a %group grant where the user is a member of that group. +// +// Returns an error if the file cannot be read or contains #include directives +// that cannot be inspected, so the caller can fail closed. +func sudoersFileGrantsUser(path, username string, userGIDs map[string]bool) (bool, error) { + data, err := os.ReadFile(path) + if err != nil { + return false, fmt.Errorf("failed to read sudoers file %s: %w", path, err) + } + directPrefix := username + " " + directPrefixTab := username + "\t" + for _, line := range strings.Split(string(data), "\n") { + trimmed := strings.TrimSpace(line) + if trimmed == "" { + continue + } + // #include/#includedir are sudoers directives, NOT comments. + if strings.HasPrefix(trimmed, "#include") { + return false, fmt.Errorf("sudoers file %s contains #include directive that cannot be inspected", path) + } + if strings.HasPrefix(trimmed, "#") { + continue + } + if strings.HasPrefix(trimmed, directPrefix) || strings.HasPrefix(trimmed, directPrefixTab) { + return true, nil + } + if strings.HasPrefix(trimmed, "User_Alias ") { + if userAliasContains(trimmed, username) { + return true, nil } - } else { - username = currentUser.Username + } + // Detect %group grants where the user is a member + if strings.HasPrefix(trimmed, "%") && userGIDs != nil { + groupName := strings.Fields(trimmed)[0][1:] + if grp, err := user.LookupGroup(groupName); err == nil && userGIDs[grp.Gid] { + return true, nil + } + } + } + return false, nil +} + +// userAliasContains checks whether a User_Alias line includes the given +// username as a member. Format: "User_Alias NAME = user1, user2, ..." +// Strips trailing inline comments (# ...) before parsing. +// Note: sudoers line continuations (\) are not handled; this is rare for +// User_Alias definitions in practice. +func userAliasContains(line, username string) bool { + _, after, found := strings.Cut(line, "=") + if !found { + return false + } + // Strip trailing inline comment + if idx := strings.Index(after, "#"); idx >= 0 { + after = after[:idx] + } + for _, member := range strings.Split(after, ",") { + if strings.TrimSpace(member) == username { + return true } } + return false +} - if username == "" { - return fmt.Errorf("could not determine target username for sudo lockdown") +// restoreDisabledSudoersFiles renames *.cargowall-disabled files back to their +// original paths. Skips files where the original already exists to avoid +// clobbering entries recreated while lockdown was active. Returns true if all +// files were restored successfully (or the list was empty). +func restoreDisabledSudoersFiles(files []string, logger *slog.Logger) bool { + allOk := true + for _, original := range files { + disabledPath := original + disabledSuffix + if _, err := os.Stat(original); err == nil { + logger.Warn("Sudoers file already exists at original path, skipping restore", + "original", original) + allOk = false + continue + } + if err := os.Rename(disabledPath, original); err != nil { + if !errors.Is(err, os.ErrNotExist) { + logger.Warn("Failed to restore sudoers file", + "original", original, "error", err) + allOk = false + } + continue + } + logger.Info("Restored sudoers file", "path", original) + } + return allOk +} + +// findDisabledSudoersFiles scans the sudoers.d directory for files ending in +// disabledSuffix and returns their original paths (with the suffix stripped). +// Used as a fallback when the state file is unavailable. +func findDisabledSudoersFiles(dir string, logger *slog.Logger) []string { + entries, err := os.ReadDir(dir) + if err != nil { + logger.Warn("Failed to scan sudoers.d for orphaned disabled files", "error", err) + return nil + } + var originals []string + for _, entry := range entries { + if entry.IsDir() { + continue + } + name := entry.Name() + if strings.HasSuffix(name, disabledSuffix) { + original := filepath.Join(dir, strings.TrimSuffix(name, disabledSuffix)) + originals = append(originals, original) + } + } + return originals +} + +// rollbackState tracks accumulated system changes during EnableSudoLockdown +// so they can be undone if the operation fails at any point. +type rollbackState struct { + lockdownState + logger *slog.Logger +} + +func (r *rollbackState) rollback() { + os.Remove(sudoersFile) + restoreDisabledSudoersFiles(r.DisabledFiles, r.logger) + if r.Username != "" { + restoreToGroups(r.Username, r.RemovedGroups, r.logger) + if r.DockerRemoved { + exec.Command("gpasswd", "-a", r.Username, "docker").CombinedOutput() //nolint:gosec // rollback only + } + } +} + +// saveLockdownState atomically writes the lockdown state to disk. +func saveLockdownState(state *lockdownState) error { + data, err := json.Marshal(state) + if err != nil { + return fmt.Errorf("failed to marshal lockdown state: %w", err) } + tmp := stateFile + ".tmp" + if err := os.WriteFile(tmp, data, 0o600); err != nil { + return fmt.Errorf("failed to write lockdown state: %w", err) + } + if err := os.Rename(tmp, stateFile); err != nil { + os.Remove(tmp) + return fmt.Errorf("failed to install lockdown state: %w", err) + } + return nil +} + +// loadLockdownState reads the lockdown state from disk. Returns nil if no +// state file exists. +func loadLockdownState() (*lockdownState, error) { + data, err := os.ReadFile(stateFile) + if errors.Is(err, os.ErrNotExist) { + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("failed to read lockdown state: %w", err) + } + var state lockdownState + if err := json.Unmarshal(data, &state); err != nil { + return nil, fmt.Errorf("failed to parse lockdown state: %w", err) + } + return &state, nil +} + +func removeLockdownState() { + os.Remove(stateFile) +} +// EnableSudoLockdown configures sudoers to restrict what commands can be run +// with sudo, removes the target user from sudo-granting and docker groups, +// and disables competing sudoers.d files. +func EnableSudoLockdown(cfg *SudoLockdownConfig, logger *slog.Logger) error { + logger.Info("Enabling sudo lockdown") + + username, err := resolveUsername(cfg.Username) + if err != nil { + return err + } logger.Debug("Target user for sudo lockdown", "username", username) + // Clean up stale state from a previous run that was not shut down cleanly + prev, prevErr := loadLockdownState() + if prevErr != nil { + logger.Warn("Failed to load stale lockdown state, proceeding with fresh lockdown", + "error", prevErr, "state_file", stateFile) + } + if prevErr == nil && prev != nil { + logger.Warn("Found stale lockdown state from previous run, cleaning up first") + stale := &rollbackState{lockdownState: *prev, logger: logger} + stale.rollback() + removeLockdownState() + } + + // Set up deferred rollback — disabled on success. Every error path after + // this point automatically undoes all system modifications. + rb := &rollbackState{ + lockdownState: lockdownState{Username: username}, + logger: logger, + } + success := false + defer func() { + if !success { + rb.rollback() + } + }() + + // Remove user from sudo-granting groups to neutralize group-based grants + rb.RemovedGroups, err = removeFromSudoGroups(username, logger) + if err != nil { + return fmt.Errorf("sudo lockdown incomplete, group removal failed: %w", err) + } + + // Disable competing sudoers.d files that grant this user access + rb.DisabledFiles, err = disableCompetingSudoersFiles(sudoersDir, username, logger) + if err != nil { + return fmt.Errorf("sudo lockdown incomplete, competing grants remain active: %w", err) + } + // Remove user from docker group to prevent docker-based firewall bypass if out, err := exec.Command("gpasswd", "-d", username, "docker").CombinedOutput(); err != nil { logger.Warn("Failed to remove user from docker group (user may not be in group)", "error", err, "output", strings.TrimSpace(string(out))) } else { logger.Info("Removed user from docker group", "username", username) + rb.DockerRemoved = true } // Build the sudoers configuration @@ -123,77 +499,142 @@ func EnableSudoLockdown(cfg *SudoLockdownConfig, logger *slog.Logger) error { return fmt.Errorf("failed to install sudoers file: %w", err) } + // Validate the overall sudoers configuration after all modifications. + // If disabling files broke alias/Defaults dependencies, roll back. + if out, err := exec.Command("visudo", "-c").CombinedOutput(); err != nil { + logger.Warn("Overall sudoers validation failed after modifications, rolling back", + "error", err, "output", strings.TrimSpace(string(out))) + return fmt.Errorf("sudoers validation failed after lockdown modifications: %w", err) + } + + // Save state so DisableSudoLockdown can reverse all changes + if err := saveLockdownState(&rb.lockdownState); err != nil { + logger.Warn("Failed to save lockdown state, rolling back", "error", err) + return fmt.Errorf("sudo lockdown aborted, state could not be persisted: %w", err) + } + + success = true logger.Info("Sudo lockdown enabled", "sudoers_file", sudoersFile, "allow_commands", cfg.AllowCommands, - "username", username) + "username", username, + "removed_groups", rb.RemovedGroups, + "disabled_files", rb.DisabledFiles) return nil } -// DisableSudoLockdown removes the sudoers lockdown configuration and restores -// the user to the docker group. +// DisableSudoLockdown removes the sudoers lockdown configuration, restores +// disabled sudoers.d files, and re-adds the user to removed groups. func DisableSudoLockdown(cfg *SudoLockdownConfig, logger *slog.Logger) error { logger.Info("Disabling sudo lockdown") - // Remove sudoers file + // Load state to know what to restore + state, err := loadLockdownState() + if err != nil { + logger.Warn("Failed to load lockdown state, falling back to best-effort cleanup", "error", err) + } + + // Remove lockdown sudoers file first so that a crash during the + // remaining restore steps leaves the user with their original grants + // (safe direction) rather than both lockdown + original grants active. if err := os.Remove(sudoersFile); err != nil && !errors.Is(err, os.ErrNotExist) { logger.Warn("Failed to remove sudoers file", "error", err) } - // Determine the target username for docker group restore - username := cfg.Username - if username == "" { - currentUser, err := user.Current() - if err == nil { - if currentUser.Uid == "0" { - for _, u := range []string{"runner", "github", "ci"} { - if _, err := user.Lookup(u); err == nil { - username = u - break - } - } - } else { - username = currentUser.Username + // Restore disabled sudoers.d files + restoreFailed := false + if state != nil { + if !restoreDisabledSudoersFiles(state.DisabledFiles, logger) { + restoreFailed = true + } + } else { + // State is unavailable — scan for orphaned *.cargowall-disabled files + // so they don't remain disabled indefinitely. + if orphaned := findDisabledSudoersFiles(sudoersDir, logger); len(orphaned) > 0 { + logger.Warn("Found orphaned disabled sudoers files without state, restoring", + "files", orphaned) + if !restoreDisabledSudoersFiles(orphaned, logger) { + restoreFailed = true } } } - // Re-add user to docker group - if username != "" { + // Determine username for group restoration + username := "" + if state != nil { + username = state.Username + } + if username == "" { + resolvedUsername, resolveErr := resolveUsername(cfg.Username) + if resolveErr != nil { + logger.Warn("Failed to resolve username for group/docker restoration, keeping state for manual recovery", + "error", resolveErr) + restoreFailed = true + } else { + username = resolvedUsername + } + } + + // Re-add to sudo-granting groups + if state != nil && username != "" { + if !restoreToGroups(username, state.RemovedGroups, logger) { + restoreFailed = true + } + } + + // Re-add user to docker group only if state confirms we removed them + if username != "" && state != nil && state.DockerRemoved { if out, err := exec.Command("gpasswd", "-a", username, "docker").CombinedOutput(); err != nil { logger.Warn("Failed to re-add user to docker group", "error", err, "output", strings.TrimSpace(string(out))) + restoreFailed = true } else { logger.Info("Restored user to docker group", "username", username) } } + // Only remove state file if all restores succeeded; keep it for manual + // recovery otherwise. + if restoreFailed { + logger.Warn("Some changes could not be restored; keeping state file for manual recovery", + "state_file", stateFile) + } else { + removeLockdownState() + } + logger.Info("Sudo lockdown disabled") + if restoreFailed { + return fmt.Errorf("sudo lockdown disable incomplete: some changes could not be restored; see state file %s", stateFile) + } return nil } // buildSudoersConfig generates the sudoers configuration content. -// The lockdown relies on restricting NOPASSWD to a specific set of commands -// and removing the user from the docker group. Note: sudoers negation (!ALL) -// is intentionally not used because it does not reliably override grants from -// other sudoers files and gives a false sense of security. +// The lockdown relies on restricting NOPASSWD to a specific set of commands, +// removing the user from sudo-granting groups, and disabling competing +// sudoers.d files. Sudoers negation (!ALL) is intentionally not used because +// it does not reliably override grants from other sudoers files. func buildSudoersConfig(cfg *SudoLockdownConfig, username string) (string, error) { - // Merge user commands + always-allowed /usr/bin/kill cmds := slices.Clone(cfg.AllowCommands) - cmds = append(cmds, "/usr/bin/kill") if err := validateSudoersInput(username, cmds); err != nil { return "", err } - content := fmt.Sprintf(`# CargoWall sudo lockdown configuration + header := fmt.Sprintf(`# CargoWall sudo lockdown configuration # This file restricts sudo access to prevent firewall bypass # Generated by cargowall - DO NOT EDIT +# +# Effective only when combined with disabling competing sudoers.d files +# (renamed to *%s) and removing the user from sudo-granting groups. +`, disabledSuffix) + + if len(cmds) == 0 { + return header, nil + } -# Allowed commands (lockdown relies on restricting NOPASSWD commands -# and removing the user from the docker group) -%s ALL=(ALL) NOPASSWD: %s -`, username, strings.Join(cmds, ", ")) + content := header + fmt.Sprintf("\n%s ALL=(ALL) NOPASSWD: %s\n", + username, strings.Join(cmds, ", ")) return content, nil } diff --git a/pkg/lockdown/sudo_test.go b/pkg/lockdown/sudo_test.go index 4ad9ddb..bb29a67 100644 --- a/pkg/lockdown/sudo_test.go +++ b/pkg/lockdown/sudo_test.go @@ -17,6 +17,11 @@ package lockdown import ( + "encoding/json" + "io" + "log/slog" + "os" + "path/filepath" "strings" "testing" ) @@ -84,9 +89,11 @@ func TestValidateSudoersInput(t *testing.T) { errSubstr: "command", }, { - name: "empty username is valid", - username: "", - cmds: []string{"/usr/bin/kill"}, + name: "empty username returns error", + username: "", + cmds: []string{"/usr/bin/kill"}, + wantErr: true, + errSubstr: "empty", }, { name: "empty commands slice is valid", @@ -116,43 +123,47 @@ func TestValidateSudoersInput(t *testing.T) { func TestBuildSudoersConfig(t *testing.T) { tests := []struct { - name string - cfg *SudoLockdownConfig - username string - wantErr bool - wantContains []string - wantExactLine string // if set, output must contain this exact line + name string + cfg *SudoLockdownConfig + username string + wantErr bool + wantContains []string + wantAbsent string }{ { - name: "single allowed command includes kill", + name: "single allowed command", cfg: &SudoLockdownConfig{ AllowCommands: []string{"/usr/sbin/iptables"}, }, username: "runner", wantContains: []string{ "/usr/sbin/iptables", - "/usr/bin/kill", "runner ALL=(ALL) NOPASSWD:", }, }, { - name: "multiple allowed commands comma-separated with kill at end", + name: "multiple allowed commands comma-separated", cfg: &SudoLockdownConfig{ AllowCommands: []string{"/usr/sbin/iptables", "/usr/sbin/ip6tables"}, }, - username: "runner", - wantExactLine: "runner ALL=(ALL) NOPASSWD: /usr/sbin/iptables, /usr/sbin/ip6tables, /usr/bin/kill", + username: "runner", + wantContains: []string{ + "runner ALL=(ALL) NOPASSWD: /usr/sbin/iptables, /usr/sbin/ip6tables", + }, }, { - name: "empty allow commands only has kill", - cfg: &SudoLockdownConfig{}, - username: "runner", - wantExactLine: "runner ALL=(ALL) NOPASSWD: /usr/bin/kill", + name: "empty allow commands produces no grant line", + cfg: &SudoLockdownConfig{}, + username: "runner", + wantContains: []string{ + "# CargoWall sudo lockdown configuration", + }, + wantAbsent: "NOPASSWD", }, { name: "unsafe username returns error", cfg: &SudoLockdownConfig{ - AllowCommands: []string{"/usr/bin/kill"}, + AllowCommands: []string{"/usr/sbin/iptables"}, }, username: "bad;user", wantErr: true, @@ -166,7 +177,18 @@ func TestBuildSudoersConfig(t *testing.T) { wantContains: []string{ "# CargoWall sudo lockdown configuration", "# Generated by cargowall - DO NOT EDIT", - "ci ALL=(ALL) NOPASSWD: /usr/sbin/iptables, /usr/bin/kill\n", + "ci ALL=(ALL) NOPASSWD: /usr/sbin/iptables\n", + }, + }, + { + name: "header mentions disabled files", + cfg: &SudoLockdownConfig{ + AllowCommands: []string{"/usr/sbin/iptables"}, + }, + username: "runner", + wantContains: []string{ + disabledSuffix, + "sudo-granting groups", }, }, } @@ -190,9 +212,268 @@ func TestBuildSudoersConfig(t *testing.T) { } } - if tt.wantExactLine != "" && !strings.Contains(got, tt.wantExactLine) { - t.Errorf("output should contain exact line %q\ngot:\n%s", tt.wantExactLine, got) + if tt.wantAbsent != "" && strings.Contains(got, tt.wantAbsent) { + t.Errorf("output should NOT contain %q\ngot:\n%s", tt.wantAbsent, got) + } + }) + } +} + +func TestSudoersFileGrantsUser(t *testing.T) { + tests := []struct { + name string + content string + username string + want bool + wantErr bool + }{ + { + name: "direct grant matches", + content: "runner ALL=(ALL) NOPASSWD:ALL\n", + username: "runner", + want: true, + }, + { + name: "grant with tab separator", + content: "runner\tALL=(ALL) NOPASSWD:ALL\n", + username: "runner", + want: true, + }, + { + name: "comment line ignored", + content: "# runner ALL=(ALL) NOPASSWD:ALL\n", + username: "runner", + want: false, + }, + { + name: "different user no match", + content: "admin ALL=(ALL) NOPASSWD:ALL\n", + username: "runner", + want: false, + }, + { + name: "group grant does not match username", + content: "%sudo ALL=(ALL:ALL) ALL\n", + username: "runner", + want: false, + }, + { + name: "empty file", + content: "", + username: "runner", + want: false, + }, + { + name: "username as substring does not match", + content: "runner2 ALL=(ALL) ALL\n", + username: "runner", + want: false, + }, + { + name: "match among multiple lines", + content: `# Defaults +Defaults env_reset +runner ALL=(ALL) NOPASSWD:ALL +`, + username: "runner", + want: true, + }, + { + name: "User_Alias with target user matches", + content: "User_Alias RUNNERS = runner, admin\nRUNNERS ALL=(ALL) ALL\n", + username: "runner", + want: true, + }, + { + name: "User_Alias without target user does not match", + content: "User_Alias ADMINS = admin, root\nADMINS ALL=(ALL) ALL\n", + username: "runner", + want: false, + }, + { + name: "User_Alias single user", + content: "User_Alias CI = runner\nCI ALL=(ALL) NOPASSWD:ALL\n", + username: "runner", + want: true, + }, + { + name: "User_Alias with inline comment", + content: "User_Alias CI = runner # CI user\nCI ALL=(ALL) ALL\n", + username: "runner", + want: true, + }, + { + name: "include directive returns error", + content: "#include /etc/sudoers.d/extra\n", + username: "runner", + wantErr: true, + }, + { + name: "includedir directive returns error", + content: "#includedir /etc/sudoers.d/extra.d\n", + username: "runner", + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + path := filepath.Join(t.TempDir(), "sudoers") + if err := os.WriteFile(path, []byte(tt.content), 0o644); err != nil { + t.Fatal(err) + } + got, err := sudoersFileGrantsUser(path, tt.username, nil) + if tt.wantErr { + if err == nil { + t.Fatal("expected error, got nil") + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != tt.want { + t.Errorf("sudoersFileGrantsUser() = %v, want %v", got, tt.want) } }) } } + +func TestSudoersFileGrantsUser_unreadableFile(t *testing.T) { + _, err := sudoersFileGrantsUser("/nonexistent/path", "runner", nil) + if err == nil { + t.Fatal("expected error for unreadable file, got nil") + } +} + +func TestDisableCompetingSudoersFiles(t *testing.T) { + dir := t.TempDir() + logger := discardLogger() + + // Create files that grant the user access + writeFile(t, filepath.Join(dir, "90-runner"), "runner ALL=(ALL) NOPASSWD:ALL\n") + writeFile(t, filepath.Join(dir, "50-cloud-init"), "# cloud-init defaults\nDefaults env_reset\n") + writeFile(t, filepath.Join(dir, "91-other"), "otheruser ALL=(ALL) ALL\n") + + disabled, err := disableCompetingSudoersFiles(dir, "runner", logger) + if err != nil { + t.Fatal(err) + } + + if len(disabled) != 1 { + t.Fatalf("expected 1 disabled file, got %d: %v", len(disabled), disabled) + } + if filepath.Base(disabled[0]) != "90-runner" { + t.Errorf("expected 90-runner to be disabled, got %s", disabled[0]) + } + + // Verify the file was renamed + if _, err := os.Stat(filepath.Join(dir, "90-runner")); !os.IsNotExist(err) { + t.Error("original file should not exist after disable") + } + if _, err := os.Stat(filepath.Join(dir, "90-runner"+disabledSuffix)); err != nil { + t.Error("disabled file should exist") + } + + // Non-matching files should be untouched + if _, err := os.Stat(filepath.Join(dir, "50-cloud-init")); err != nil { + t.Error("non-matching file should be untouched") + } + if _, err := os.Stat(filepath.Join(dir, "91-other")); err != nil { + t.Error("other user file should be untouched") + } +} + +func TestDisableCompetingSudoersFiles_skipsOwnFiles(t *testing.T) { + dir := t.TempDir() + logger := discardLogger() + + // Create our own files — they should never be disabled + writeFile(t, filepath.Join(dir, filepath.Base(sudoersFile)), "runner ALL=(ALL) NOPASSWD: /usr/sbin/iptables\n") + writeFile(t, filepath.Join(dir, filepath.Base(stateFile)), `{"username":"runner"}`) + + disabled, err := disableCompetingSudoersFiles(dir, "runner", logger) + if err != nil { + t.Fatal(err) + } + if len(disabled) != 0 { + t.Errorf("expected 0 disabled files, got %d: %v", len(disabled), disabled) + } +} + +func TestRestoreDisabledSudoersFiles(t *testing.T) { + dir := t.TempDir() + logger := discardLogger() + + original := filepath.Join(dir, "90-runner") + disabledPath := original + disabledSuffix + writeFile(t, disabledPath, "runner ALL=(ALL) NOPASSWD:ALL\n") + + if ok := restoreDisabledSudoersFiles([]string{original}, logger); !ok { + t.Error("restoreDisabledSudoersFiles should return true on success") + } + + if _, err := os.Stat(original); err != nil { + t.Error("original file should be restored") + } + if _, err := os.Stat(disabledPath); !os.IsNotExist(err) { + t.Error("disabled file should not exist after restore") + } +} + +func TestRestoreDisabledSudoersFiles_toleratesMissing(t *testing.T) { + logger := discardLogger() + // Should not panic or error on missing files; missing disabled file is tolerated + if ok := restoreDisabledSudoersFiles([]string{"/nonexistent/file"}, logger); !ok { + t.Error("restoreDisabledSudoersFiles should return true for missing files (tolerated)") + } +} + +// TestLockdownStateJSONRoundTrip verifies JSON round-trip of lockdownState. +// The real saveLockdownState/loadLockdownState functions use atomic write +// (tmp+rename) and file permissions, which are exercised by the CI +// integration test (test-sudo-lockdown). +func TestLockdownStateJSONRoundTrip(t *testing.T) { + state := &lockdownState{ + Username: "runner", + RemovedGroups: []string{"sudo", "wheel"}, + DisabledFiles: []string{"/etc/sudoers.d/90-runner"}, + DockerRemoved: true, + } + + data, err := json.Marshal(state) + if err != nil { + t.Fatal(err) + } + + var loaded lockdownState + if err := json.Unmarshal(data, &loaded); err != nil { + t.Fatal(err) + } + + if loaded.Username != state.Username { + t.Errorf("username: got %q, want %q", loaded.Username, state.Username) + } + if len(loaded.RemovedGroups) != len(state.RemovedGroups) { + t.Errorf("removedGroups length: got %d, want %d", len(loaded.RemovedGroups), len(state.RemovedGroups)) + } + if len(loaded.DisabledFiles) != len(state.DisabledFiles) { + t.Errorf("disabledFiles length: got %d, want %d", len(loaded.DisabledFiles), len(state.DisabledFiles)) + } + if loaded.DockerRemoved != state.DockerRemoved { + t.Errorf("dockerRemoved: got %v, want %v", loaded.DockerRemoved, state.DockerRemoved) + } +} + +// helpers + +func writeFile(t *testing.T, path, content string) { + t.Helper() + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } +} + +func discardLogger() *slog.Logger { + return slog.New(slog.NewTextHandler(io.Discard, nil)) +} From 9ca291d286b8335cd613245e51b00bc6e4257070 Mon Sep 17 00:00:00 2001 From: Matthew DeVenny Date: Mon, 6 Apr 2026 13:20:36 -0700 Subject: [PATCH 2/3] review updates Signed-off-by: Matthew DeVenny --- .github/workflows/ci.yml | 8 ++++++-- pkg/lockdown/sudo.go | 24 ++++++++++++++++-------- 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5575252..bf31ec9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -495,7 +495,9 @@ jobs: [ -f "$f" ] || continue sudo mv "$f" "${f%.cargowall-disabled}" done - sudo gpasswd -a runner sudo 2>/dev/null || true + for group in sudo admin wheel; do + sudo gpasswd -a runner "$group" 2>/dev/null || true + done fi echo "Emergency cleanup complete" @@ -653,7 +655,9 @@ jobs: [ -f "$f" ] || continue sudo mv "$f" "${f%.cargowall-disabled}" done - sudo gpasswd -a runner sudo 2>/dev/null || true + for group in sudo admin wheel; do + sudo gpasswd -a runner "$group" 2>/dev/null || true + done fi echo "Emergency cleanup complete" diff --git a/pkg/lockdown/sudo.go b/pkg/lockdown/sudo.go index 6a1c0ef..d10ea8e 100644 --- a/pkg/lockdown/sudo.go +++ b/pkg/lockdown/sudo.go @@ -121,7 +121,8 @@ func lookupUserGIDs(username string) (map[string]bool, error) { func removeFromSudoGroups(username string, logger *slog.Logger) ([]string, error) { userGIDs, gidErr := lookupUserGIDs(username) if gidErr != nil { - logger.Warn("Failed to look up user groups, skipping group removal", + // Fail closed: attempt removal from all groups rather than skipping + logger.Warn("Failed to look up user groups, attempting removal from all sudo-granting groups", "username", username, "error", gidErr) } @@ -129,8 +130,13 @@ func removeFromSudoGroups(username string, logger *slog.Logger) ([]string, error var failures []string for _, group := range sudoGrantingGroups { grp, err := user.LookupGroup(group) - if err != nil || userGIDs == nil || !userGIDs[grp.Gid] { - logger.Debug("User not in group or group does not exist, skipping", + if err != nil { + logger.Debug("Group does not exist, skipping", + "username", username, "group", group) + continue + } + if userGIDs != nil && !userGIDs[grp.Gid] { + logger.Debug("User not in group, skipping", "username", username, "group", group) continue } @@ -169,10 +175,12 @@ func restoreToGroups(username string, groups []string, logger *slog.Logger) bool return allOk } -// disableCompetingSudoersFiles scans /etc/sudoers.d/ for files that contain -// direct grants for the given username and renames them to *.cargowall-disabled. -// Returns the list of original file paths that were disabled and an error if -// any matching file could not be disabled (lockdown may be incomplete). +// disableCompetingSudoersFiles scans /etc/sudoers.d/ for files that grant +// sudo access to the given username — whether via a direct user entry, +// User_Alias membership, or a matching %group grant — and renames them to +// *.cargowall-disabled. Returns the list of original file paths that were +// disabled and an error if any matching file could not be disabled (lockdown +// may be incomplete). func disableCompetingSudoersFiles(dir, username string, logger *slog.Logger) ([]string, error) { entries, err := os.ReadDir(dir) if err != nil { @@ -182,7 +190,7 @@ func disableCompetingSudoersFiles(dir, username string, logger *slog.Logger) ([] // Resolve user's group memberships once for all file checks. userGIDs, gidErr := lookupUserGIDs(username) if gidErr != nil { - logger.Warn("Failed to look up user groups for sudoers scan, %group detection disabled", + logger.Warn("Failed to look up user groups for sudoers scan, group detection disabled", "username", username, "error", gidErr) } From 4cb4d910bf28de284a50ffb325d1266f6d4a2d7c Mon Sep 17 00:00:00 2001 From: Matthew DeVenny Date: Mon, 6 Apr 2026 15:23:44 -0700 Subject: [PATCH 3/3] review changes Signed-off-by: Matthew DeVenny --- cmd/start.go | 16 ++++- pkg/lockdown/sudo.go | 84 ++++++++++++++++------- pkg/lockdown/sudo_test.go | 137 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 208 insertions(+), 29 deletions(-) diff --git a/cmd/start.go b/cmd/start.go index b020f70..c3012de 100644 --- a/cmd/start.go +++ b/cmd/start.go @@ -741,9 +741,21 @@ func enableSudoLockdown(cmd *StartCmd, logger *slog.Logger) (bool, *lockdown.Sud if cmd.SudoAllowCommands != "" { for _, c := range strings.Split(cmd.SudoAllowCommands, ",") { c = strings.TrimSpace(c) - if c != "" { - allowCmds = append(allowCmds, c) + if c == "" { + continue } + // Sudoers commands must be absolute paths with no arguments. + // Strip arguments (anything after the first space) and warn. + if idx := strings.IndexByte(c, ' '); idx >= 0 { + logger.Warn("Stripping arguments from sudo allow command (sudoers requires bare paths)", + "original", c, "path", c[:idx]) + c = c[:idx] + } + if !strings.HasPrefix(c, "/") { + logger.Warn("Skipping non-absolute sudo allow command", "command", c) + continue + } + allowCmds = append(allowCmds, c) } } cfg := &lockdown.SudoLockdownConfig{ diff --git a/pkg/lockdown/sudo.go b/pkg/lockdown/sudo.go index d10ea8e..b8b95f4 100644 --- a/pkg/lockdown/sudo.go +++ b/pkg/lockdown/sudo.go @@ -30,8 +30,10 @@ import ( ) const ( - sudoersDir = "/etc/sudoers.d" - sudoersFile = "/etc/sudoers.d/zz-cargowall-lockdown" + sudoersDir = "/etc/sudoers.d" + sudoersFile = "/etc/sudoers.d/zz-cargowall-lockdown" + // stateFile uses a dotfile name — sudo's #includedir/@includedir processing + // ignores files containing '.' so this cannot interfere with sudoers parsing. stateFile = "/etc/sudoers.d/.cargowall-lockdown-state" disabledSuffix = ".cargowall-disabled" ) @@ -55,7 +57,7 @@ type lockdownState struct { DockerRemoved bool `json:"dockerRemoved"` } -const sudoersUnsafeChars = " ,#\n\r\t`;&!()\\|" +const sudoersUnsafeChars = " ,#\n\r\t`;&!()\\|*?[]" // validateSudoersInput checks that a username and command paths do not contain // characters that could alter sudoers semantics. @@ -222,26 +224,48 @@ func disableCompetingSudoersFiles(dir, username string, logger *slog.Logger) ([] } disabledPath := path + disabledSuffix - if _, err := os.Lstat(disabledPath); err == nil { - logger.Warn("Disabled target already exists, cannot safely disable sudoers file", - "path", path, "disabledPath", disabledPath) - renameFailed = append(renameFailed, path) - continue - } else if !errors.Is(err, os.ErrNotExist) { - logger.Warn("Failed to check disabled sudoers file path", - "path", path, "disabledPath", disabledPath, "error", err) - renameFailed = append(renameFailed, path) - continue - } - if err := os.Rename(path, disabledPath); err != nil { - logger.Warn("Failed to disable sudoers file", - "path", path, "error", err) + // Use Link+Remove instead of Rename to atomically fail if the + // disabled target already exists (Link returns EEXIST). + if err := os.Link(path, disabledPath); err != nil { + if errors.Is(err, os.ErrExist) { + logger.Warn("Disabled target already exists, cannot safely disable sudoers file", + "path", path, "disabledPath", disabledPath) + } else { + logger.Warn("Failed to disable sudoers file", + "path", path, "error", err) + } renameFailed = append(renameFailed, path) continue } + os.Remove(path) logger.Info("Disabled competing sudoers file", "path", path) disabled = append(disabled, path) } + + // Verification re-scan: catch any files that appeared during the disable + // pass (narrows the TOCTOU window to microseconds). + if len(renameFailed) == 0 { + recheck, _ := os.ReadDir(dir) + for _, entry := range recheck { + if entry.IsDir() { + continue + } + name := entry.Name() + if name == filepath.Base(sudoersFile) || + name == filepath.Base(stateFile) || + strings.HasSuffix(name, ".tmp") || + strings.HasSuffix(name, disabledSuffix) { + continue + } + path := filepath.Join(dir, name) + if grants, _ := sudoersFileGrantsUser(path, username, userGIDs); grants { + logger.Warn("New sudoers file appeared during lockdown scan", + "path", path) + renameFailed = append(renameFailed, path) + } + } + } + if len(renameFailed) > 0 { return disabled, fmt.Errorf("failed to disable competing sudoers files: %v", renameFailed) } @@ -268,13 +292,19 @@ func sudoersFileGrantsUser(path, username string, userGIDs map[string]bool) (boo if trimmed == "" { continue } - // #include/#includedir are sudoers directives, NOT comments. - if strings.HasPrefix(trimmed, "#include") { - return false, fmt.Errorf("sudoers file %s contains #include directive that cannot be inspected", path) + // #include/#includedir and @include/@includedir are sudoers directives, + // NOT comments. Modern sudo (1.9.1+) uses the @ form. + if strings.HasPrefix(trimmed, "#include") || strings.HasPrefix(trimmed, "@include") { + return false, fmt.Errorf("sudoers file %s contains include directive that cannot be inspected", path) } if strings.HasPrefix(trimmed, "#") { continue } + // Defaults !authenticate disables password prompts for all users, + // effectively granting passwordless sudo to anyone with a grant. + if strings.HasPrefix(trimmed, "Defaults") && strings.Contains(trimmed, "!authenticate") { + return false, fmt.Errorf("sudoers file %s contains 'Defaults !authenticate' which bypasses password requirements", path) + } if strings.HasPrefix(trimmed, directPrefix) || strings.HasPrefix(trimmed, directPrefixTab) { return true, nil } @@ -283,8 +313,13 @@ func sudoersFileGrantsUser(path, username string, userGIDs map[string]bool) (boo return true, nil } } - // Detect %group grants where the user is a member - if strings.HasPrefix(trimmed, "%") && userGIDs != nil { + // Detect %group grants where the user is a member. + // When userGIDs is nil (lookup failed), treat any %group line as a + // potential grant (fail-closed) rather than skipping. + if strings.HasPrefix(trimmed, "%") { + if userGIDs == nil { + return true, nil + } groupName := strings.Fields(trimmed)[0][1:] if grp, err := user.LookupGroup(groupName); err == nil && userGIDs[grp.Gid] { return true, nil @@ -379,7 +414,10 @@ func (r *rollbackState) rollback() { if r.Username != "" { restoreToGroups(r.Username, r.RemovedGroups, r.logger) if r.DockerRemoved { - exec.Command("gpasswd", "-a", r.Username, "docker").CombinedOutput() //nolint:gosec // rollback only + if out, err := exec.Command("gpasswd", "-a", r.Username, "docker").CombinedOutput(); err != nil { + r.logger.Warn("Failed to restore docker group during rollback", + "error", err, "output", strings.TrimSpace(string(out))) + } } } } diff --git a/pkg/lockdown/sudo_test.go b/pkg/lockdown/sudo_test.go index bb29a67..0f493d9 100644 --- a/pkg/lockdown/sudo_test.go +++ b/pkg/lockdown/sudo_test.go @@ -252,10 +252,10 @@ func TestSudoersFileGrantsUser(t *testing.T) { want: false, }, { - name: "group grant does not match username", + name: "group grant matches when GIDs unknown (fail-closed)", content: "%sudo ALL=(ALL:ALL) ALL\n", username: "runner", - want: false, + want: true, // nil GIDs = any %group is a potential grant }, { name: "empty file", @@ -303,17 +303,47 @@ runner ALL=(ALL) NOPASSWD:ALL want: true, }, { - name: "include directive returns error", + name: "hash include directive returns error", content: "#include /etc/sudoers.d/extra\n", username: "runner", wantErr: true, }, { - name: "includedir directive returns error", + name: "hash includedir directive returns error", content: "#includedir /etc/sudoers.d/extra.d\n", username: "runner", wantErr: true, }, + { + name: "at-include directive returns error", + content: "@include /etc/sudoers.d/extra\n", + username: "runner", + wantErr: true, + }, + { + name: "at-includedir directive returns error", + content: "@includedir /etc/sudoers.d/extra.d\n", + username: "runner", + wantErr: true, + }, + { + name: "Defaults !authenticate returns error", + content: "Defaults !authenticate\n", + username: "runner", + wantErr: true, + }, + { + name: "Defaults with authenticate negation in context returns error", + content: "Defaults:runner !authenticate\n", + username: "runner", + wantErr: true, + }, + { + name: "group grant matches when GIDs nil (fail-closed)", + content: "%ops ALL=(ALL) ALL\n", + username: "runner", + want: true, // nil GIDs = treat any %group as potential grant + }, } for _, tt := range tests { @@ -465,6 +495,105 @@ func TestLockdownStateJSONRoundTrip(t *testing.T) { } } +func TestUserAliasContains(t *testing.T) { + tests := []struct { + name string + line string + username string + want bool + }{ + { + name: "exact match", + line: "User_Alias CI = runner", + username: "runner", + want: true, + }, + { + name: "among multiple members", + line: "User_Alias RUNNERS = admin, runner, ci", + username: "runner", + want: true, + }, + { + name: "not present", + line: "User_Alias ADMINS = admin, root", + username: "runner", + want: false, + }, + { + name: "substring does not match", + line: "User_Alias CI = runner2, runner3", + username: "runner", + want: false, + }, + { + name: "no equals sign", + line: "User_Alias BROKEN", + username: "runner", + want: false, + }, + { + name: "empty after equals", + line: "User_Alias EMPTY = ", + username: "runner", + want: false, + }, + { + name: "inline comment stripped", + line: "User_Alias CI = runner # the CI user", + username: "runner", + want: true, + }, + { + name: "inline comment hides member", + line: "User_Alias CI = admin # runner", + username: "runner", + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := userAliasContains(tt.line, tt.username); got != tt.want { + t.Errorf("userAliasContains() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestFindDisabledSudoersFiles(t *testing.T) { + dir := t.TempDir() + logger := discardLogger() + + writeFile(t, filepath.Join(dir, "90-runner"+disabledSuffix), "runner ALL=(ALL) ALL\n") + writeFile(t, filepath.Join(dir, "50-other"+disabledSuffix), "other ALL=(ALL) ALL\n") + writeFile(t, filepath.Join(dir, "normal-file"), "something\n") + + found := findDisabledSudoersFiles(dir, logger) + if len(found) != 2 { + t.Fatalf("expected 2 disabled files, got %d: %v", len(found), found) + } +} + +func TestFindDisabledSudoersFiles_emptyDir(t *testing.T) { + dir := t.TempDir() + logger := discardLogger() + + found := findDisabledSudoersFiles(dir, logger) + if len(found) != 0 { + t.Errorf("expected 0 disabled files, got %d", len(found)) + } +} + +func TestValidateSudoersInput_rejectsWildcards(t *testing.T) { + // Wildcards in sudoers command paths could grant unintended access + for _, cmd := range []string{"/usr/bin/*", "/usr/bin/apt?", "/usr/bin/[a-z]"} { + if err := validateSudoersInput("runner", []string{cmd}); err == nil { + t.Errorf("expected error for wildcard command %q, got nil", cmd) + } + } +} + // helpers func writeFile(t *testing.T, path, content string) {