diff --git a/ingestion/cmd/server/symlink_check_test.go b/ingestion/cmd/server/symlink_check_test.go index 0f6a357..4dcf12c 100644 --- a/ingestion/cmd/server/symlink_check_test.go +++ b/ingestion/cmd/server/symlink_check_test.go @@ -4,6 +4,7 @@ import ( "os" "path/filepath" "strings" + "syscall" "testing" "github.com/stretchr/testify/assert" @@ -63,3 +64,13 @@ func TestStartupSymlinkCheckRefusesAnUnlistableBrainRoot(t *testing.T) { t.Cleanup(func() { _ = os.Chmod(brain, 0o755) }) assert.Error(t, startupSymlinkCheck(brain)) } + +// hyperguild#179: a FIFO in the brain is refused at startup with a clear +// message, instead of hanging or exiting later in a reader. +func TestStartupSymlinkCheckRefusesABrainWithAFIFO(t *testing.T) { + brain := t.TempDir() + require.NoError(t, syscall.Mkfifo(filepath.Join(brain, "classification.yaml"), 0o644)) + err := startupSymlinkCheck(brain) + require.Error(t, err) + assert.Contains(t, err.Error(), "classification.yaml") +} diff --git a/ingestion/internal/symlinkguard/special_test.go b/ingestion/internal/symlinkguard/special_test.go new file mode 100644 index 0000000..24ad902 --- /dev/null +++ b/ingestion/internal/symlinkguard/special_test.go @@ -0,0 +1,104 @@ +package symlinkguard + +import ( + "context" + "net" + "os" + "path/filepath" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// hyperguild#179: the tripwire looked only for symlinks, so a FIFO (or +// socket) in the brain passed it. A FIFO at classification.yaml then made +// startup exit; at a note path it could hang readers that skip the handle +// check. +func TestScanReportsFIFOsAndSocketsAsSpecialFiles(t *testing.T) { + brain := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(brain, "wiki", "w"), 0o755)) + require.NoError(t, syscall.Mkfifo(filepath.Join(brain, "classification.yaml"), 0o644)) + require.NoError(t, syscall.Mkfifo(filepath.Join(brain, "wiki", "w", "f.md"), 0o644)) + l, err := net.Listen("unix", filepath.Join(brain, "s.sock")) + require.NoError(t, err) + defer func() { _ = l.Close() }() + require.NoError(t, os.WriteFile(filepath.Join(brain, "wiki", "w", "n.md"), []byte("n"), 0o644)) + + r, err := Scan(brain) + require.NoError(t, err) + assert.Equal(t, []string{"classification.yaml", "s.sock", "wiki/w/f.md"}, r.Special) + assert.False(t, r.Clean()) + assert.Contains(t, r.String(), "special file") +} + +// Watch removes a FIFO as it removes symlinks, and leaves a regular file +// (which Scan never reports) alone. +func TestWatchRemovesAFIFOAndLeavesAnUnreportedRegularFile(t *testing.T) { + brain := t.TempDir() + fifo := filepath.Join(brain, "f.md") + require.NoError(t, syscall.Mkfifo(fifo, 0o644)) + regular := filepath.Join(brain, "n.md") + require.NoError(t, os.WriteFile(regular, []byte("n"), 0o644)) + n := &recNotifier{} + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + go Watch(ctx, brain, 10*time.Millisecond, n) + require.Eventually(t, func() bool { + _, err := os.Lstat(fifo) + return os.IsNotExist(err) + }, 2*time.Second, 10*time.Millisecond, "the FIFO is removed") + require.Eventually(t, func() bool { return n.count() >= 1 }, 2*time.Second, 10*time.Millisecond) + b, err := os.ReadFile(regular) + require.NoError(t, err) + assert.Equal(t, "n", string(b)) + n.mu.Lock() + defer n.mu.Unlock() + assert.Contains(t, n.msgs[0], "f.md") +} + +// removeSpecial re-checks the type: a path that is a regular file or a +// directory at the re-check is not removed. (A swap between the re-check and +// the unlink can still remove one; Linux has no conditional unlink. The same +// window exists in removeSymlinks.) +func TestRemoveSpecialRefusesRegularFilesAndDirectories(t *testing.T) { + brain := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(brain, "n.md"), []byte("n"), 0o644)) + require.NoError(t, os.MkdirAll(filepath.Join(brain, "d"), 0o755)) + require.NoError(t, syscall.Mkfifo(filepath.Join(brain, "f.md"), 0o644)) + removed, failed := removeSpecial(brain, []string{"n.md", "d", "f.md"}) + assert.Equal(t, []string{"f.md"}, removed) + assert.Equal(t, []string{"n.md", "d"}, failed) + _, err := os.Stat(filepath.Join(brain, "n.md")) + assert.NoError(t, err) +} + +// A special file Watch cannot remove (its directory is read-only) stays +// reported: the persistent-finding alert names it. +func TestWatchReportsASpecialFileItCannotRemove(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root ignores directory permissions") + } + brain := t.TempDir() + ro := filepath.Join(brain, "ro") + require.NoError(t, os.MkdirAll(ro, 0o755)) + require.NoError(t, syscall.Mkfifo(filepath.Join(ro, "f.md"), 0o644)) + require.NoError(t, os.Chmod(ro, 0o555)) + t.Cleanup(func() { _ = os.Chmod(ro, 0o755) }) + n := &recNotifier{} + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + go Watch(ctx, brain, 10*time.Millisecond, n) + require.Eventually(t, func() bool { return n.count() >= 1 }, 2*time.Second, 10*time.Millisecond) + _, err := os.Lstat(filepath.Join(ro, "f.md")) + require.NoError(t, err, "the FIFO could not be removed") + n.mu.Lock() + defer n.mu.Unlock() + assert.Contains(t, n.msgs[0], "ro/f.md") + assert.Contains(t, n.msgs[0], "special file") + for _, m := range n.msgs { + assert.NotContains(t, m, "removed", "a file it could not remove was reported as removed") + } +} diff --git a/ingestion/internal/symlinkguard/symlinkguard.go b/ingestion/internal/symlinkguard/symlinkguard.go index b5bd5da..5086b8a 100644 --- a/ingestion/internal/symlinkguard/symlinkguard.go +++ b/ingestion/internal/symlinkguard/symlinkguard.go @@ -46,10 +46,16 @@ type Notifier interface { type Result struct { Symlinks []string Unreadable []string + // Special holds FIFOs, sockets and devices (hyperguild#179): a reader + // that opens one by path can hang, and a FIFO at classification.yaml + // stops startup. + Special []string } // Clean reports whether the scan found nothing to act on. -func (r Result) Clean() bool { return len(r.Symlinks) == 0 && len(r.Unreadable) == 0 } +func (r Result) Clean() bool { + return len(r.Symlinks) == 0 && len(r.Unreadable) == 0 && len(r.Special) == 0 +} // String describes the findings for a log line or alert. Paths are quoted, so // a name containing a newline cannot blur two findings together. @@ -59,6 +65,9 @@ func (r Result) String() string { if len(r.Symlinks) > 0 { parts = append(parts, fmt.Sprintf("%d symlink(s): %s", len(r.Symlinks), q(r.Symlinks))) } + if len(r.Special) > 0 { + parts = append(parts, fmt.Sprintf("%d special file(s) (FIFO, socket or device; delete them): %s", len(r.Special), q(r.Special))) + } if len(r.Unreadable) > 0 { parts = append(parts, fmt.Sprintf("%d unreadable dir(s): %s", len(r.Unreadable), q(r.Unreadable))) } @@ -113,12 +122,16 @@ func Scan(brainDir string) (Result, error) { if d.IsDir() && p == rootGit { return filepath.SkipDir } - if d.Type()&os.ModeSymlink != 0 { + switch { + case d.Type()&os.ModeSymlink != 0: r.Symlinks = append(r.Symlinks, rel) + case !d.IsDir() && !d.Type().IsRegular(): + r.Special = append(r.Special, rel) } return nil }) sort.Strings(r.Symlinks) + sort.Strings(r.Special) sort.Strings(r.Unreadable) if walkErr != nil { // Only the root's own error reaches here (deeper errors are recorded @@ -182,14 +195,20 @@ func Watch(ctx context.Context, brainDir string, interval time.Duration, n Notif q.push(fmt.Sprintf("brain symlink tripwire (hyperguild#157): removed %d symlink(s) from the brain dir: %s", len(removed), Result{}.quoted(removed))) } - key = Result{Symlinks: failed, Unreadable: r.Unreadable}.String() + sRemoved, sFailed := removeSpecial(brainDir, r.Special) + if len(sRemoved) > 0 { + slog.Error("brain symlink tripwire removed special files", "count", len(sRemoved)) + q.push(fmt.Sprintf("brain symlink tripwire (hyperguild#179): removed %d special file(s) (FIFO, socket or device) from the brain dir: %s", + len(sRemoved), Result{}.quoted(sRemoved))) + } + key = Result{Symlinks: failed, Unreadable: r.Unreadable, Special: sFailed}.String() } if key != reported { reported = key if key != "" { slog.Error("brain symlink tripwire", "finding", key) - q.push("brain symlink tripwire (hyperguild#157): " + key + - ". Readers may follow symlinks out of the brain; fix them.") + q.push("brain symlink tripwire (hyperguild#157, #179): " + key + + ". Symlinks can lead readers out of the brain and special files can hang them; remove them.") } } } @@ -322,6 +341,40 @@ func permanent(err error) bool { return errors.As(err, &p) && p.Permanent() } +// removeSpecial unlinks each brain-relative FIFO, socket or device through +// an os.Root on the brain dir, after re-checking that it is still one: a +// path that is a regular file, a directory or a symlink at the re-check is +// left alone and reported as failed. The re-check narrows, but cannot close, +// the window before the unlink (Linux has no conditional unlink); the same +// window exists in removeSymlinks, and only a local writer can use it. +func removeSpecial(brainDir string, rels []string) (removed, failed []string) { + if len(rels) == 0 { + return nil, nil + } + resolved, err := filepath.EvalSymlinks(brainDir) + if err != nil { + return nil, rels + } + root, err := os.OpenRoot(resolved) + if err != nil { + return nil, rels + } + defer func() { _ = root.Close() }() + for _, rel := range rels { + fi, err := root.Lstat(rel) + if err != nil || fi.IsDir() || fi.Mode().IsRegular() || fi.Mode()&os.ModeSymlink != 0 { + failed = append(failed, rel) + continue + } + if err := root.Remove(rel); err != nil { + failed = append(failed, rel) + continue + } + removed = append(removed, rel) + } + return removed, failed +} + // removeSymlinks unlinks each brain-relative symlink through an os.Root on // the brain dir, after re-checking it is still a symlink. Removing a symlink // never touches its target. It returns what was removed and what was not.