#37 lockdown other grants in sudoers.d

[matthewdevenny]

Closes #37

Adds hardening to sudo lockdown mode to ensure other existing grants aren't allowing sudo to continue to work

[Copilot]

Pull request overview

Hardens the sudo lockdown mechanism to prevent bypass via pre-existing sudoers.d grants (issue #37) by disabling competing sudoers entries and tracking changes for reversal.

Changes:

• Remove target user from common sudo-granting groups and disable competing sudoers.d files by renaming them.
• Introduce a persisted “lockdown state” to restore renamed files and group membership during unlock.
• Update tests and design documentation to cover/describe the new behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File	Description
pkg/lockdown/sudo.go	Adds competing-sudoers disabling, sudo-group removal, and stateful enable/disable flows.
pkg/lockdown/sudo_test.go	Adds unit tests for grant detection, file disabling/restoring, and state JSON round-trips.
design.md	Documents new sudo lockdown behavior and file naming.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

pkg/lockdown/sudo.go:331

• EnableSudoLockdown makes irreversible system changes (removing sudo-granting groups, renaming sudoers.d files, removing docker group) before building/validating/writing the new sudoers file, but on this error return path there is no rollback. This can leave the runner partially locked down (and cmd/start.go continues without lockdown on error). Consider validating inputs up front and adding a consistent rollback/cleanup on every failure after modifications (restore renamed files, restore groups, re-add docker if removed).

	// Build the sudoers configuration
	sudoersContent, err := buildSudoersConfig(cfg, username)
	if err != nil {
		return fmt.Errorf("failed to build sudoers config: %w", err)
	}

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

Comments suppressed due to low confidence (1)

pkg/lockdown/sudo.go:367

• EnableSudoLockdown performs irreversible changes (group removals / sudoers.d renames / docker group removal) before buildSudoersConfig, but if buildSudoersConfig fails the function returns without any rollback. That can leave the user removed from sudo-granting groups and sudoers files disabled while no lockdown file was installed. Consider validating/building the sudoers content before making system changes, and/or add a deferred rollback that restores groups, docker membership, and disabled sudoers files on any later error path.

	// Build the sudoers configuration
	sudoersContent, err := buildSudoersConfig(cfg, username)
	if err != nil {
		return fmt.Errorf("failed to build sudoers config: %w", err)
	}

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

pkg/lockdown/sudo.go:75

• validateSudoersInput checks for unsafe characters, but it doesn’t enforce that allowed commands are absolute paths. A non-absolute entry (e.g. iptables) is valid sudoers syntax and can match whichever binary is found via sudo’s PATH/secure_path, which is risky for an allowlist. Consider rejecting commands that don’t start with / (and possibly normalizing/cleaning paths) here for defense-in-depth, since buildSudoersConfig can be called from places other than the CLI parser.

	if strings.ContainsAny(username, sudoersUnsafeChars) {
		return fmt.Errorf("sudoers username %q contains unsafe characters", username)
	}
	for _, c := range cmds {
		if strings.ContainsAny(c, sudoersUnsafeChars) {
			return fmt.Errorf("sudoers command %q contains unsafe characters", c)
		}
	}

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

os.Remove(path) is called without checking its error. If the remove fails (permissions, immutable bit, etc.), the original sudoers file remains in place and continues to grant sudo, while the .cargowall-disabled hardlink is created. Treat a remove failure as a lockdown failure (and ideally clean up the disabledPath you just linked) so lockdown can’t silently be bypassed.

Suggested change

	os.Remove(path)
	if err := os.Remove(path); err != nil {
	logger.Warn("Failed to remove original sudoers file after creating disabled link",
	"path", path, "disabledPath", disabledPath, "error", err)
	if cleanupErr := os.Remove(disabledPath); cleanupErr != nil {
	logger.Warn("Failed to clean up disabled sudoers link after rollback",
	"path", path, "disabledPath", disabledPath, "error", cleanupErr)
	}
	renameFailed = append(renameFailed, path)
	continue
	}

[Copilot]

sudoersFileGrantsUser treats a User_Alias line that contains the username as an immediate “grant”, but it does not detect the common case where the alias is defined in one sudoers.d file and the actual grant (ALIAS ALL=(ALL) ...) is in a different file. In that scenario this function will disable the alias-definition file (making the config invalid) but will not disable the granting file, causing visudo -c to fail and lockdown to roll back (leaving sudo unrestricted). Consider scanning the directory as a whole: collect alias names that include the username, then also treat lines starting with any of those alias names as grants when evaluating other files.