From 9ebbb6172261ab7c4a5a611cced7ca81e2c6f503 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Apr 2026 20:43:32 +0000 Subject: [PATCH] Make main() and the security gates testable Closes #13. main() previously did all gate sequencing inline and log.Fatal'd on each failure, which makes it impossible to exercise in-process. Two pieces of the test gap were tied to that: (1) main() was 0% covered. Coverage came from leaf utilities, not from the orchestration. A regression in the gate ordering (or in the new validateArgs from #38) couldn't be caught by `go test`. (2) The security gates themselves were 0% covered. isExecutableWritable looked at /proc/self/exe; isRunningAsRoot called os.Getuid() directly; isFileWorldWritable was defined but never called or tested. None could be driven by a test without elaborate chmod-the-test-binary-then-re-exec trickery. Refactor: - security.go: isPathUnsafelyWritable(path) extracted from isExecutableWritable; the latter now just resolves os.Executable() and delegates. Same goes for uidIsRoot(uid), called by isRunningAsRoot. Production callers see no behavior change; tests can chmod arbitrary temp files and pass them to the predicate, or pass uid 0/1/1000 directly to uidIsRoot. - ussher.go: runDeps struct holds the side-effecting hooks (executableWritable, runningAsRoot, initLog, validUser, loadConfig, runFn). defaultDeps() wires production. runMain(args, deps) error carries the same gate ordering as the old main but returns an error instead of calling log.Fatal. main() is now four lines: call runMain(os.Args, defaultDeps()), log.Fatal on error. - User-visible error messages preserved verbatim ("Refusing to run due to permissions issue on the ussher executable", "Refusing to run as root", "User not found") so the existing README troubleshooting headings still match what operators see in their log files. - The fmt.Println diagnostics inside isPathUnsafelyWritable now include the path being checked so a future error log says "/usr/local/bin/ussher is group writable" instead of the hard-coded "ussher binary is group writable". (fmt.Println still goes to stdout, which is the wrong channel for sshd-consumed output - that's #36, deliberately not in this PR.) Tests added: - ussher_test.go grows TestRunMain (table-driven, six cases covering missing args / version short-circuit / each gate's failure path / happy path) and TestDefaultDeps_AllSet (sanity check that the production wiring leaves nothing nil). The captureRunMain helper builds a deps that records initLog/loadConfig/runFn invocations so tests can assert "did we short-circuit before we got here?". - security_test.go grows TestUidIsRoot, TestIsPathUnsafelyWritable (table-driven across 0700/0750/0755/0770/0775/0707/0757/0777 plus a missing-path failsafe case), and TestIsFileWorldWritable (already-public function that wasn't previously tested). Coverage 42.7% -> 62.4% (+19.7pts). All tests pass under -race. #14 (integration test for Run) is still distinct - this PR doesn't exercise the goroutine fan-out itself, only the path that decides whether Run gets called. #14 lands the runFn coverage; this PR lands the main()/gate coverage. CHANGELOG bullet under [Unreleased] / Changed (refactor, no adopter-visible behavior change). --- CHANGELOG.md | 10 ++++ security.go | 23 +++++++-- security_test.go | 103 +++++++++++++++++++++++++++++++++++++++ ussher.go | 66 +++++++++++++++++++------ ussher_test.go | 123 ++++++++++++++++++++++++++++++++++++++++++++++- 5 files changed, 304 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 76d0ee1..c5b242f 100644 --- a/CHANGELOG.md +++ b/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 ` @@ -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 diff --git a/security.go b/security.go index d95c177..5cc2784 100644 --- a/security.go +++ b/security.go @@ -14,10 +14,17 @@ 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 } @@ -25,13 +32,13 @@ func isExecutableWritable() bool { // 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. diff --git a/security_test.go b/security_test.go index 6781d51..a63a279 100644 --- a/security_test.go +++ b/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)") + } + }) +} diff --git a/ussher.go b/ussher.go index 46a58e2..994259d 100644 --- a/ussher.go +++ b/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) + } } diff --git a/ussher_test.go b/ussher_test.go index 1ad342a..9bc8e44 100644 --- a/ussher_test.go +++ b/ussher_test.go @@ -1,6 +1,9 @@ package main -import "testing" +import ( + "strings" + "testing" +) func TestValidateArgs(t *testing.T) { cases := []struct { @@ -28,3 +31,121 @@ func TestValidateArgs(t *testing.T) { }) } } + +// captureRunMain runs runMain with a deps that records which side-effect +// hooks fired and returns those flags alongside the error. +type runMainResult struct { + err error + initLogCalled bool + loadCalled bool + loadUsername string + runCalled bool +} + +func captureRunMain(args []string, execWritable, asRoot, validUser bool) runMainResult { + var r runMainResult + d := runDeps{ + executableWritable: func() bool { return execWritable }, + runningAsRoot: func() bool { return asRoot }, + initLog: func() { r.initLogCalled = true }, + validUser: func(string) bool { return validUser }, + loadConfig: func(u string, _ *Config) { + r.loadCalled = true + r.loadUsername = u + }, + runFn: func(*Config) { r.runCalled = true }, + } + r.err = runMain(args, d) + return r +} + +func TestRunMain(t *testing.T) { + cases := []struct { + name string + args []string + execWritable bool + asRoot bool + validUser bool + wantErrSubstring string // empty = expect nil + wantInitLog bool + wantRun bool + }{ + { + name: "missing args", + args: []string{"ussher"}, + validUser: true, + wantErrSubstring: "usage:", + }, + { + name: "version short-circuits before any gate", + args: []string{"ussher", "--version"}, + // gates flipped to "would fail" — none should fire. + execWritable: true, + asRoot: true, + validUser: false, + }, + { + name: "executable writable", + args: []string{"ussher", "alice"}, + execWritable: true, + validUser: true, + wantErrSubstring: "permissions issue", + }, + { + name: "running as root", + args: []string{"ussher", "alice"}, + asRoot: true, + validUser: true, + wantErrSubstring: "as root", + }, + { + name: "invalid user", + args: []string{"ussher", "alice"}, + validUser: false, + wantErrSubstring: "User not found", + wantInitLog: true, + }, + { + name: "happy path", + args: []string{"ussher", "alice"}, + validUser: true, + wantInitLog: true, + wantRun: true, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + r := captureRunMain(tc.args, tc.execWritable, tc.asRoot, tc.validUser) + + if (r.err != nil) != (tc.wantErrSubstring != "") { + t.Fatalf("err = %v, want substring %q", r.err, tc.wantErrSubstring) + } + if r.err != nil && !strings.Contains(r.err.Error(), tc.wantErrSubstring) { + t.Errorf("err = %q, want substring %q", r.err.Error(), tc.wantErrSubstring) + } + if r.initLogCalled != tc.wantInitLog { + t.Errorf("initLog called = %v, want %v", r.initLogCalled, tc.wantInitLog) + } + if r.runCalled != tc.wantRun { + t.Errorf("runFn called = %v, want %v", r.runCalled, tc.wantRun) + } + if r.runCalled { + if !r.loadCalled || r.loadUsername != "alice" { + t.Errorf("loadConfig called with %q, want alice", r.loadUsername) + } + } + }) + } +} + +func TestDefaultDeps_AllSet(t *testing.T) { + d := defaultDeps() + if d.executableWritable == nil || + d.runningAsRoot == nil || + d.initLog == nil || + d.validUser == nil || + d.loadConfig == nil || + d.runFn == nil { + t.Error("defaultDeps left a hook unset") + } +}