From 10d614893b26d3e412704d4aacb65604b1a6543b Mon Sep 17 00:00:00 2001 From: Roberto Leinardi Date: Tue, 26 May 2026 16:48:10 +0200 Subject: [PATCH] Fix Directory bind-mounts silently excluded when allow-glob targets children --- README.md | 4 + internal/processor/processor_test.go | 112 ++++++++++++++++- internal/processor/rules.go | 181 ++++++++++++++++++++++----- 3 files changed, 263 insertions(+), 34 deletions(-) diff --git a/README.md b/README.md index 300d934..f647334 100644 --- a/README.md +++ b/README.md @@ -156,6 +156,10 @@ label-driven tooling). The daemon also reads top-level `labels:` if you need to override a service-wide value on a single task; per-container values win on conflict. +If you bind-mount a directory (for example `source: /dev/dri`), the +`device-allow` glob is evaluated **per child node** inside that directory — +write the glob against the children, e.g. `/dev/dri/*` or `/dev/dri/renderD128`. + Global `-device-allow` and `-device-deny` define the broadest access the daemon may grant. Per-container labels can only narrow that access. Deny rules always win. diff --git a/internal/processor/processor_test.go b/internal/processor/processor_test.go index cc8f14c..9dc8e5d 100644 --- a/internal/processor/processor_test.go +++ b/internal/processor/processor_test.go @@ -465,16 +465,126 @@ func TestCollectMountRules_BadPath(t *testing.T) { } } +// TestCollectMountRules_DirectoryMount_NoChildrenMatch checks that a WARN is emitted +// when a directory mount has children but none match the allow/deny policy. +func TestCollectMountRules_DirectoryMount_NoChildrenMatch(t *testing.T) { + dir := t.TempDir() + + err := os.WriteFile(filepath.Join(dir, "card0"), []byte{}, 0o600) + if err != nil { + t.Fatalf("create card0: %v", err) + } + + err = os.WriteFile(filepath.Join(dir, "renderD128"), []byte{}, 0o600) + if err != nil { + t.Fatalf("create renderD128: %v", err) + } + + gpol := policy.Global{ + Mode: policy.ModeAll, + DeviceAllow: []string{filepath.Join(dir, "nonexistent")}, + } + + buf := captureLogger(t) + + rules, errs := CollectMountRules(dir, gpol, policy.Container{}) + + if len(rules) != 0 { + t.Errorf("expected no rules, got %v", rules) + } + + if len(errs) != 0 { + t.Errorf("expected no errors, got %v", errs) + } + + logOutput := buf.String() + if !strings.Contains(logOutput, "mount excluded: no children matched") { + t.Errorf("expected WARN log about no children matched, got: %s", logOutput) + } + + if !strings.Contains(logOutput, "card0") || !strings.Contains(logOutput, "renderD128") { + t.Errorf("expected child names in WARN log, got: %s", logOutput) + } +} + +// TestCollectMountRules_DirectoryMount_SymlinkToDirSkipped checks that a symlink inside +// a directory mount that points to another directory is not recursed into. +func TestCollectMountRules_DirectoryMount_SymlinkToDirSkipped(t *testing.T) { + dir := t.TempDir() + subDir := filepath.Join(dir, "subdir") + + err := os.Mkdir(subDir, 0o755) + if err != nil { + t.Fatalf("create subdir: %v", err) + } + + err = os.Symlink(subDir, filepath.Join(dir, "linktodir")) + if err != nil { + t.Fatalf("create symlink: %v", err) + } + + gpol := policy.Global{Mode: policy.ModeAll} + + buf := captureLogger(t) + + rules, errs := CollectMountRules(dir, gpol, policy.Container{}) + + if len(rules) != 0 { + t.Errorf("expected no rules for dir-only mount, got %v", rules) + } + + if len(errs) != 0 { + t.Errorf("unexpected errors: %v", errs) + } + + if !strings.Contains(buf.String(), "symlink to directory skipped") { + t.Errorf("expected debug log about skipped dir symlink, got: %s", buf.String()) + } +} + +// TestCollectMountRules_DirectoryMount_SymlinkToDevice is the core regression test: +// a directory mount whose children include a symlink to a real device gets a cgroup rule +// injected even though the allow-glob targets the resolved path, not the mount source. +func TestCollectMountRules_DirectoryMount_SymlinkToDevice(t *testing.T) { + dir := t.TempDir() + + err := os.Symlink("/dev/null", filepath.Join(dir, "null")) + if err != nil { + t.Fatalf("create symlink: %v", err) + } + + gpol := policy.Global{ + Mode: policy.ModeAll, + DeviceAllow: []string{"/dev/null"}, + } + + rules, errs := CollectMountRules(dir, gpol, policy.Container{}) + + if len(errs) != 0 { + t.Fatalf("unexpected errors: %v", errs) + } + + if len(rules) != 1 { + t.Fatalf("expected 1 rule for /dev/null, got %d", len(rules)) + } + + if !rules[0].Allow || rules[0].Access != "rwm" { + t.Errorf("rule has unexpected allow/access: %+v", rules[0]) + } +} + // captureLogger sets logger.L() to write to a buffer for the duration of the // test and restores the previous logger when the test ends. func captureLogger(t *testing.T) *bytes.Buffer { t.Helper() + prev := logger.L() + var buf bytes.Buffer logger.Set(slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{ Level: slog.LevelDebug, }))) - t.Cleanup(func() { logger.Set(nil) }) + t.Cleanup(func() { logger.Set(prev) }) return &buf } diff --git a/internal/processor/rules.go b/internal/processor/rules.go index 8541abd..0016fca 100644 --- a/internal/processor/rules.go +++ b/internal/processor/rules.go @@ -20,6 +20,7 @@ package processor import ( "fmt" + "io/fs" "os" "path/filepath" "strings" @@ -31,6 +32,13 @@ import ( "github.com/leinardi/swarm-device-access/internal/policy" ) +const ( + maxDirDepth = 8 + maxLogChildren = 32 + + deviceAccessAll = "rwm" +) + // IsMountSource reports whether path is /dev or a path under /dev/. func IsMountSource(path string) bool { return path == "/dev" || strings.HasPrefix(path, "/dev/") @@ -43,19 +51,34 @@ func CollectMountRules( gpol policy.Global, cpol policy.Container, ) ([]cgroup.DeviceRule, []error) { - if !gpol.DeviceAllowed(cpol, mountPath) { - logger.L().Debug("device mount excluded by policy", "path", mountPath) - - return nil, nil - } - - fileInfo, err := os.Stat(mountPath) + linfo, err := os.Lstat(mountPath) if err != nil { return nil, []error{fmt.Errorf("stat %q: %w", mountPath, err)} } - if !fileInfo.IsDir() { - rule, ruleErr := collectDeviceRule(mountPath) + effectivePath := mountPath + if linfo.Mode()&os.ModeSymlink != 0 { + resolved, resolveErr := filepath.EvalSymlinks(mountPath) + if resolveErr != nil { + return nil, []error{fmt.Errorf("resolve symlink %q: %w", mountPath, resolveErr)} + } + + effectivePath = resolved + + linfo, err = os.Lstat(effectivePath) + if err != nil { + return nil, []error{fmt.Errorf("stat %q: %w", effectivePath, err)} + } + } + + if !linfo.IsDir() { + if !gpol.DeviceAllowed(cpol, effectivePath) { + logger.L().Debug("device mount excluded by policy", "path", effectivePath) + + return nil, nil + } + + rule, ruleErr := collectDeviceRule(effectivePath) if ruleErr != nil { return nil, []error{ruleErr} } @@ -63,44 +86,136 @@ func CollectMountRules( return []cgroup.DeviceRule{rule}, nil } - var ( - rules []cgroup.DeviceRule - errs []error - ) + state := &mountWalkState{ + mountPath: effectivePath, + gpol: gpol, + cpol: cpol, + } - walkErr := filepath.Walk(mountPath, func(walkedPath string, info os.FileInfo, err error) error { - if err != nil { - errs = append(errs, err) + walkErr := filepath.WalkDir(effectivePath, state.visitEntry) + if walkErr != nil { + state.errs = append(state.errs, fmt.Errorf("walk %q: %w", effectivePath, walkErr)) + } - return nil - } + if len(state.rules) == 0 && len(state.childrenSeen) > 0 && len(state.errs) == 0 { + logger.L().Warn("mount excluded: no children matched allow/deny policy", + "path", effectivePath, + "children_seen", state.childrenSeen, + "allow_globs", append(gpol.DeviceAllow, cpol.DeviceAllow...), + "deny_globs", append(gpol.DeviceDeny, cpol.DeviceDeny...), + ) + } - if info.IsDir() { - return nil - } + return state.rules, state.errs +} - if !gpol.DeviceAllowed(cpol, walkedPath) { - logger.L().Debug("device file excluded by policy", "path", walkedPath) +type mountWalkState struct { + mountPath string + gpol policy.Global + cpol policy.Container + rules []cgroup.DeviceRule + errs []error + childrenSeen []string +} +func (s *mountWalkState) visitEntry(walkedPath string, entry fs.DirEntry, entryErr error) error { + if entryErr != nil { + s.errs = append(s.errs, entryErr) + + return nil + } + + if entry.IsDir() { + if walkedPath == s.mountPath { return nil } - rule, ruleErr := collectDeviceRule(walkedPath) - if ruleErr != nil { - errs = append(errs, fmt.Errorf("device rule for %q: %w", walkedPath, ruleErr)) + depth := strings.Count( + strings.TrimPrefix(walkedPath, s.mountPath), + string(filepath.Separator), + ) + if depth > maxDirDepth { + logger.L().Debug("walk depth cap reached", "path", walkedPath) - return nil + return filepath.SkipDir } - rules = append(rules, rule) + return nil + } + + if entry.Type()&os.ModeSymlink != 0 { + return s.visitSymlink(walkedPath) + } + + return s.visitRegularFile(walkedPath) +} + +func (s *mountWalkState) visitSymlink(symlinkPath string) error { + realPath, resolveErr := filepath.EvalSymlinks(symlinkPath) + if resolveErr != nil { + s.errs = append(s.errs, fmt.Errorf("resolve symlink %q: %w", symlinkPath, resolveErr)) + + return nil + } + + targetInfo, statErr := os.Stat(realPath) + if statErr != nil { + s.errs = append(s.errs, fmt.Errorf("stat symlink target %q: %w", realPath, statErr)) + + return nil + } + + if targetInfo.IsDir() { + logger.L().Debug("symlink to directory skipped", "path", symlinkPath, "target", realPath) + + return nil + } + + s.trackChild(realPath) + + if !s.gpol.DeviceAllowed(s.cpol, realPath) { + logger.L().Debug("device file excluded by policy", "path", realPath) + + return nil + } + + rule, ruleErr := collectDeviceRule(realPath) + if ruleErr != nil { + s.errs = append(s.errs, fmt.Errorf("device rule for %q: %w", realPath, ruleErr)) return nil - }) - if walkErr != nil { - errs = append(errs, fmt.Errorf("walk %q: %w", mountPath, walkErr)) } - return rules, errs + s.rules = append(s.rules, rule) + + return nil +} + +func (s *mountWalkState) visitRegularFile(filePath string) error { + s.trackChild(filePath) + + if !s.gpol.DeviceAllowed(s.cpol, filePath) { + logger.L().Debug("device file excluded by policy", "path", filePath) + + return nil + } + + rule, ruleErr := collectDeviceRule(filePath) + if ruleErr != nil { + s.errs = append(s.errs, fmt.Errorf("device rule for %q: %w", filePath, ruleErr)) + + return nil + } + + s.rules = append(s.rules, rule) + + return nil +} + +func (s *mountWalkState) trackChild(path string) { + if len(s.childrenSeen) < maxLogChildren { + s.childrenSeen = append(s.childrenSeen, filepath.Base(path)) + } } // collectDeviceRule returns the DeviceRule for a single (non-directory) device file. @@ -112,7 +227,7 @@ func collectDeviceRule(devicePath string) (cgroup.DeviceRule, error) { return cgroup.DeviceRule{ Allow: true, - Access: "rwm", + Access: deviceAccessAll, Type: deviceType, Major: &major, Minor: &minor,