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") + } +}