Make main() and the security gates testable (closes #13)

CHANGELOG.md

@@ -7,6 +7,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Changed
+
+- `main()`, the security gates, and the `isExecutableWritable` permission
+  check are now structured for testability. `runMain(args, deps) error`
+  carries the gate sequence; `isPathUnsafelyWritable(path)` and
+  `uidIsRoot(uid)` carry the gate logic. No behavior change for production
+  invocations — the same gates fire in the same order with the same
+  user-visible messages. Coverage rises from 42% to 62%. ([#13])
+
 ### Fixed
 
 - `ussher` invoked with no arguments now prints `usage: ussher <username>`
@@ -100,6 +109,7 @@ Initial release.
 [#8]: https://github.com/dolph/ussher/pull/8
 [#10]: https://github.com/dolph/ussher/pull/10
 [#12]: https://github.com/dolph/ussher/issues/12
+[#13]: https://github.com/dolph/ussher/issues/13
 [#25]: https://github.com/dolph/ussher/pull/25
 [#29]: https://github.com/dolph/ussher/issues/29
 [#31]: https://github.com/dolph/ussher/issues/31

security.go

@@ -14,24 +14,31 @@ func isExecutableWritable() bool {
 		fmt.Printf("Failed to get a path to ussher executable: %v\n", err)
 		return true
 	}
+	return isPathUnsafelyWritable(executablePath)
+}
 
-	fileInfo, err := os.Stat(executablePath)
+// isPathUnsafelyWritable returns true if path's mode has the group-write
+// or other-write bit set, or if path can't be stat'd (failsafe). Extracted
+// from isExecutableWritable so the bit-mask logic is testable against an
+// arbitrary chmod'd file rather than /proc/self/exe.
+func isPathUnsafelyWritable(path string) bool {
+	fileInfo, err := os.Stat(path)
 	if err != nil {
-		fmt.Printf("Failed to stat ussher executable: %v\n", err)
+		fmt.Printf("Failed to stat %s: %v\n", path, err)
 		return true
 	}
 
 	mode := fileInfo.Mode()
 
 	// Check for group writable
 	if mode&0020 != 0 {
-		fmt.Println("ussher binary is group writable")
+		fmt.Printf("%s is group writable\n", path)
 		return true
 	}
 
 	// Check for world writable
 	if mode&0002 != 0 {
-		fmt.Println("ussher binary is world writable")
+		fmt.Printf("%s is world writable\n", path)
 		return true
 	}
 
@@ -41,7 +48,13 @@ func isExecutableWritable() bool {
 // Return true if ussher is running as the root user, which would violate
 // the principle of least-privilege.
 func isRunningAsRoot() bool {
-	return os.Getuid() == 0
+	return uidIsRoot(os.Getuid())
+}
+
+// uidIsRoot returns true iff the given uid is 0. Extracted so the
+// "is root?" predicate can be tested without actually running as root.
+func uidIsRoot(uid int) bool {
+	return uid == 0
 }
 
 // Ensure that the input string is a valid Linux account name on this host.

security_test.go

@@ -1,6 +1,8 @@
 package main
 
 import (
+	"os"
+	"path/filepath"
 	"testing"
 )
 
@@ -51,3 +53,104 @@ func TestIsValidUser(t *testing.T) {
 		})
 	}
 }
+
+func TestUidIsRoot(t *testing.T) {
+	cases := []struct {
+		uid  int
+		want bool
+	}{
+		{0, true},
+		{1, false},
+		{500, false},
+		{1000, false},
+		{-1, false},
+	}
+	for _, tc := range cases {
+		if got := uidIsRoot(tc.uid); got != tc.want {
+			t.Errorf("uidIsRoot(%d) = %v, want %v", tc.uid, got, tc.want)
+		}
+	}
+}
+
+// writeFileMode creates path with the exact mode requested, defeating umask.
+func writeFileMode(t *testing.T, path string, mode os.FileMode) {
+	t.Helper()
+	if err := os.WriteFile(path, []byte("x"), 0644); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Chmod(path, mode); err != nil {
+		t.Fatal(err)
+	}
+}
+
+func TestIsPathUnsafelyWritable(t *testing.T) {
+	dir := t.TempDir()
+
+	cases := []struct {
+		name string
+		mode os.FileMode
+		want bool
+	}{
+		{"safe 0755", 0755, false},
+		{"safe 0750", 0750, false},
+		{"safe 0700", 0700, false},
+		{"group writable 0775", 0775, true},
+		{"group writable 0770", 0770, true},
+		{"world writable 0757", 0757, true},
+		{"world writable 0707", 0707, true},
+		{"both 0777", 0777, true},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			path := filepath.Join(dir, tc.name)
+			writeFileMode(t, path, tc.mode)
+			if got := isPathUnsafelyWritable(path); got != tc.want {
+				t.Errorf("isPathUnsafelyWritable(mode=%o) = %v, want %v", tc.mode, got, tc.want)
+			}
+		})
+	}
+
+	t.Run("missing path fails safe to true", func(t *testing.T) {
+		if !isPathUnsafelyWritable(filepath.Join(dir, "does-not-exist")) {
+			t.Error("missing path should be reported as unsafely writable (failsafe)")
+		}
+	})
+}
+
+func TestIsFileWorldWritable(t *testing.T) {
+	dir := t.TempDir()
+
+	t.Run("non-world-writable", func(t *testing.T) {
+		path := filepath.Join(dir, "safe")
+		writeFileMode(t, path, 0640)
+		got, err := isFileWorldWritable(path)
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if got {
+			t.Errorf("isFileWorldWritable(0640) = true, want false")
+		}
+	})
+
+	t.Run("world-writable", func(t *testing.T) {
+		path := filepath.Join(dir, "permissive")
+		writeFileMode(t, path, 0666)
+		got, err := isFileWorldWritable(path)
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if !got {
+			t.Errorf("isFileWorldWritable(0666) = false, want true")
+		}
+	})
+
+	t.Run("missing file returns true and an error", func(t *testing.T) {
+		got, err := isFileWorldWritable(filepath.Join(dir, "missing"))
+		if err == nil {
+			t.Error("expected error for missing file")
+		}
+		if !got {
+			t.Error("missing file should be reported as world-writable (failsafe)")
+		}
+	})
+}

ussher.go

@@ -36,41 +36,77 @@ func validateArgs(args []string) (string, error) {
 	return args[1], nil
 }
 
-func main() {
-	arg, err := validateArgs(os.Args)
+// runDeps wires the side-effecting bits main() depends on (security gates,
+// log setup, the actual fetch loop) so tests can substitute fakes. Only the
+// fields that have side effects or read host state need to be injected;
+// pure helpers like validateArgs are called directly.
+type runDeps struct {
+	executableWritable func() bool
+	runningAsRoot      func() bool
+	initLog            func()
+	validUser          func(name string) bool
+	loadConfig         func(username string, c *Config)
+	runFn              func(*Config)
+}
+
+// defaultDeps wires the production implementations. main uses this; tests
+// supply their own.
+func defaultDeps() runDeps {
+	return runDeps{
+		executableWritable: isExecutableWritable,
+		runningAsRoot:      isRunningAsRoot,
+		initLog:            initLog,
+		validUser:          isValidUser,
+		loadConfig:         func(u string, c *Config) { c.LoadConfigByUser(u) },
+		runFn:              Run,
+	}
+}
+
+// runMain is main's testable body: same gate ordering, but every gate's
+// outcome is observable as an error return rather than a process-killing
+// log.Fatal. main() is the thin wrapper that calls log.Fatal on error.
+func runMain(args []string, d runDeps) error {
+	arg, err := validateArgs(args)
 	if err != nil {
-		log.Fatal(err)
+		return err
 	}
 
 	// Support `ussher --version` to print versioning info about the executable
 	// before proceeding to security-hardening checks.
 	if arg == "--version" {
 		PrintVersion()
-		return
+		return nil
 	}
 
 	// Security sanity checks
-	if isExecutableWritable() {
-		log.Fatal("Refusing to run due to permissions issue on the ussher executable")
+	if d.executableWritable() {
+		return errors.New("Refusing to run due to permissions issue on the ussher executable")
 	}
-	if isRunningAsRoot() {
-		log.Fatal("Refusing to run as root")
+	if d.runningAsRoot() {
+		return errors.New("Refusing to run as root")
 	}
 
 	// Initialize logging AFTER security checks to ensure we're writing logs as
-	// a non-root user
-	initLog()
+	// a non-root user.
+	d.initLog()
 
-	// Check if the input username is valid
+	// Check if the input username is valid.
 	username := arg
-	if !isValidUser(username) {
-		log.Fatal("User not found")
+	if !d.validUser(username) {
+		return errors.New("User not found")
 	}
 
 	// At this point, we know that the input username is valid and safe to use.
 	log.Print("Sourcing authorized_keys for ", username)
 
 	var c Config
-	c.LoadConfigByUser(username)
-	Run(&c)
+	d.loadConfig(username, &c)
+	d.runFn(&c)
+	return nil
+}
+
+func main() {
+	if err := runMain(os.Args, defaultDeps()); err != nil {
+		log.Fatal(err)
+	}
 }