diff --git a/deploy/upcloud/configs/scanner.yaml.tmpl b/deploy/upcloud/configs/scanner.yaml.tmpl index 59845a7..eca1469 100644 --- a/deploy/upcloud/configs/scanner.yaml.tmpl +++ b/deploy/upcloud/configs/scanner.yaml.tmpl @@ -47,3 +47,9 @@ vuln: # reached 561 MiB RSS with the memory limit applied. Images above this are # rejected before any blob is downloaded. max_image_size: 536870912 + # Reclaim scan directories left behind by a scanner that was killed + # mid-scan (a restart or a deploy). Those never run their own cleanup, and + # on this host they reached 8.8 GB on a 20 GB disk before scans started + # failing with "no space left on device". Above the 8m job timeout, so a + # scan in flight elsewhere in the same directory is never touched. + sweep_max_age: 1h diff --git a/docs/SBOM_SCANNING.md b/docs/SBOM_SCANNING.md index 3011312..0119ff2 100644 --- a/docs/SBOM_SCANNING.md +++ b/docs/SBOM_SCANNING.md @@ -110,6 +110,7 @@ a YAML file or pure env vars with the `SCANNER_` prefix. Run with | `vuln.db_path` | `SCANNER_VULN_DB_PATH` | `/var/lib/atcr-scanner/vulndb` | Directory for the Grype vulnerability database. | | `vuln.tmp_dir` | `SCANNER_VULN_TMP_DIR` | `/var/lib/atcr-scanner/tmp` | Directory for layer extraction and DB download. Also exported as `TMPDIR`; point it at a large partition, **not** tmpfs. | | `vuln.max_image_size`| `SCANNER_VULN_MAX_IMAGE_SIZE`| `2147483648` (2 GiB) | Max total compressed image size. Larger images are skipped with an error. `0` = no limit. | +| `vuln.sweep_max_age`| `SCANNER_VULN_SWEEP_MAX_AGE` | `1h` | Age threshold for the startup sweep of `vuln.tmp_dir`. Leftover `scan-*`, `syft-scan-*` and `syft-cataloger-*` directories older than this are removed before workers start; a scan killed by a restart never cleans up after itself. Keep it above the longest scan so a second scanner sharing the directory keeps its live work. `0` disables the sweep. | | `server.addr` | `SCANNER_SERVER_ADDR` | `:9090` | Listen address for the scanner's health endpoint. | Both `hold.url` and `hold.secret` are required; `LoadConfig` errors out if either is @@ -420,6 +421,11 @@ admin endpoint `POST /admin/api/scan-backfill` instead. - **Layer extraction or Grype DB download fails mid-process.** `vuln.tmp_dir` is too small or on tmpfs. Point it at a large persistent partition; the scanner sets `TMPDIR` to this directory. +- **`vuln.tmp_dir` fills up with `scan-*` / `syft-scan-*` directories.** These are + scans the process was killed in the middle of, which never ran their own cleanup. + The startup sweep reclaims them on the next restart; if it is not doing so, check + that `vuln.sweep_max_age` is not `0` and look for `Swept scanner tmp dir` in the + startup logs. - **SBOM present but no vulnerability counts.** `vuln.enabled` is false on the scanner, or the Grype DB failed to initialize (check startup logs). - **Helm/attestation artifacts show "scanning isn't applied".** Expected — these are diff --git a/scanner/internal/config/config.go b/scanner/internal/config/config.go index ff717f2..e839fe1 100644 --- a/scanner/internal/config/config.go +++ b/scanner/internal/config/config.go @@ -68,6 +68,13 @@ type VulnConfig struct { // Maximum total compressed image size in bytes. Images exceeding this are skipped. 0 = no limit. MaxImageSize int64 `yaml:"max_image_size" comment:"Maximum total compressed image size in bytes. 0 = no limit. Default: 2 GiB."` + + // Age threshold for the startup sweep of tmp_dir. A scan killed mid-flight + // (a restart, a deploy) never runs its own cleanup, so its layout and + // extraction directories stay on disk forever; the sweep reclaims the ones + // older than this. It must stay well above the longest a scan can take, so + // that a second scanner sharing the directory keeps its live work. + SweepMaxAge time.Duration `yaml:"sweep_max_age" comment:"Age threshold for the startup sweep of stale scan directories in tmp_dir (scan-*, syft-scan-*, syft-cataloger-*), which a scan interrupted by a restart leaves behind. Must stay above the longest scan so a second scanner sharing the directory keeps its live work. 0 disables the sweep. Default: 1h."` } // setScannerDefaults registers all default values on the given Viper instance. @@ -92,6 +99,7 @@ func setScannerDefaults(v *viper.Viper) { v.SetDefault("vuln.db_path", "/var/lib/atcr-scanner/vulndb") v.SetDefault("vuln.tmp_dir", "/var/lib/atcr-scanner/tmp") v.SetDefault("vuln.max_image_size", 2*1024*1024*1024) // 2 GiB + v.SetDefault("vuln.sweep_max_age", "1h") // Log shipper defaults v.SetDefault("log_shipper.batch_size", 100) diff --git a/scanner/internal/scan/extractor.go b/scanner/internal/scan/extractor.go index 914ca9f..95c5ef3 100644 --- a/scanner/internal/scan/extractor.go +++ b/scanner/internal/scan/extractor.go @@ -62,7 +62,7 @@ type ociIndex struct { // waiting out the HTTP client's own per-request timeout, and every exit from // here removes the scan directory — the deadline path included. func buildOCILayout(ctx context.Context, job *scanner.ScanJob, tmpDir, secret string, maxBytes int64) (string, func(), error) { - scanDir, err := os.MkdirTemp(tmpDir, "scan-*") + scanDir, err := os.MkdirTemp(tmpDir, scanDirPrefix+"*") if err != nil { return "", nil, fmt.Errorf("failed to create temp directory: %w", err) } diff --git a/scanner/internal/scan/sweep.go b/scanner/internal/scan/sweep.go new file mode 100644 index 0000000..c2759b4 --- /dev/null +++ b/scanner/internal/scan/sweep.go @@ -0,0 +1,130 @@ +package scan + +import ( + "errors" + "io/fs" + "log/slog" + "os" + "path/filepath" + "strings" + "time" +) + +// The three directory shapes a scan leaves directly under the configured tmp +// dir. Everything created here is per-job and disposable: +// +// scan-* buildOCILayout's downloaded OCI layout +// syft-scan-* stereoscope's extraction root (syftTempDirName) +// syft-cataloger-* Syft's own cataloging scratch, created inside itself +// +// Only these are ever swept. The Grype database lives beside the tmp dir at +// /vulndb and go-getter unpacks its downloads into a grype-dl +// directory underneath it; both are state the scanner needs and neither +// matches a prefix here. +const ( + scanDirPrefix = "scan-" + // syftTempDirName is the root prefix handed to stereoscope's + // TempDirGenerator in syft.go, which appends "-" and a random suffix. + syftTempDirName = "syft-scan" + // syftCatalogerPrefix is Syft's, not ours: it opens its own generator + // during cataloging. Named here because the sweep is the only thing that + // reclaims one after a crash. + syftCatalogerPrefix = "syft-cataloger-" +) + +// sweepPrefixes is the full set, defined once so the creators above and the +// sweeper below cannot drift apart. +var sweepPrefixes = []string{scanDirPrefix, syftTempDirName + "-", syftCatalogerPrefix} + +// sweepTmpDir removes scan leftovers older than maxAge from tmpDir. +// +// The per-job error paths already clean up after themselves; what they cannot +// do is clean up after a process that dies mid-scan. A restart — a deploy +// included — permanently orphans whatever was in flight, and on seamark-hold +// that reached 8.8 GB on a 20 GB disk before scans started failing with "no +// space left on device". Nothing else ever reclaims these, so startup does. +// +// Only direct children matching sweepPrefixes are considered, and only +// directories: the tmp dir is shared with state the scanner needs. maxAge is +// what makes this safe to run while another scanner shares the directory — +// its in-flight scan-* dir is minutes old, not hours. A maxAge of zero or less +// disables the sweep entirely rather than removing a peer's live work. +// +// Nothing here is fatal. Every failure is logged and the sweep moves on, so a +// permission problem in the tmp dir cannot stop the scanner from starting. +func sweepTmpDir(tmpDir string, maxAge time.Duration) { + if tmpDir == "" || maxAge <= 0 { + return + } + + entries, err := os.ReadDir(tmpDir) + if err != nil { + // A tmp dir that does not exist yet is the normal first-boot state, + // not something to warn about. + if !errors.Is(err, fs.ErrNotExist) { + slog.Warn("Failed to sweep scanner tmp dir", "dir", tmpDir, "error", err) + } + return + } + + cutoff := time.Now().Add(-maxAge) + var removed int + var reclaimed int64 + for _, entry := range entries { + if !entry.IsDir() || !sweepable(entry.Name()) { + continue + } + info, err := entry.Info() + if err != nil { + slog.Warn("Failed to stat stale scan directory", "name", entry.Name(), "error", err) + continue + } + if info.ModTime().After(cutoff) { + continue + } + + // Measured before the removal, because afterwards there is nothing + // left to measure. Best effort: a directory that cannot be walked is + // still worth removing, it just goes unaccounted for. + path := filepath.Join(tmpDir, entry.Name()) + size := dirSize(path) + if err := os.RemoveAll(path); err != nil { + slog.Warn("Failed to remove stale scan directory", "dir", path, "error", err) + continue + } + removed++ + reclaimed += size + } + + slog.Info("Swept scanner tmp dir", + "dir", tmpDir, + "removed", removed, + "bytes", reclaimed, + "older_than", maxAge) +} + +// sweepable reports whether a directory name is one of ours. +func sweepable(name string) bool { + for _, prefix := range sweepPrefixes { + if strings.HasPrefix(name, prefix) { + return true + } + } + return false +} + +// dirSize sums the file sizes under path, ignoring anything it cannot read. +// It exists only to put a number in the sweep's log line. +func dirSize(path string) int64 { + var total int64 + _ = filepath.WalkDir(path, func(_ string, d fs.DirEntry, err error) error { + if err != nil || d.IsDir() { + return nil //nolint:nilerr // an unreadable subtree is skipped, not fatal + } + if info, err := d.Info(); err == nil { + total += info.Size() + } + return nil + }) + return total +} diff --git a/scanner/internal/scan/sweep_test.go b/scanner/internal/scan/sweep_test.go new file mode 100644 index 0000000..e339c48 --- /dev/null +++ b/scanner/internal/scan/sweep_test.go @@ -0,0 +1,148 @@ +package scan + +import ( + "os" + "path/filepath" + "testing" + "time" +) + +// staleAge is comfortably past the maxAge every case below sweeps with, so a +// "stale" fixture is stale regardless of filesystem timestamp granularity. +const staleAge = 3 * time.Hour + +// mkStaleDir creates a directory under parent holding one file of the given +// size, then backdates it so the sweep sees it as old. +func mkStaleDir(t *testing.T, parent, name string, size int, age time.Duration) string { + t.Helper() + + path := filepath.Join(parent, name) + if err := os.MkdirAll(path, 0o755); err != nil { + t.Fatalf("mkdir %s: %v", path, err) + } + if size > 0 { + if err := os.WriteFile(filepath.Join(path, "blob"), make([]byte, size), 0o644); err != nil { + t.Fatalf("write blob in %s: %v", path, err) + } + } + when := time.Now().Add(-age) + if err := os.Chtimes(path, when, when); err != nil { + t.Fatalf("chtimes %s: %v", path, err) + } + return path +} + +// TestSweepTmpDirRemovesOnlyStaleScanDirs is the whole contract of the sweep: +// the three shapes a dead scan leaves behind go, and everything else in the +// tmp dir stays. The preserved entries are not hypothetical — the Grype +// database lives at /vulndb and go-getter unpacks into grype-dl under +// the tmp dir, so a sweep that took either would cost a full DB download on +// every restart. +func TestSweepTmpDirRemovesOnlyStaleScanDirs(t *testing.T) { + tests := []struct { + name string + dir string + age time.Duration + wantGone bool + }{ + {name: "orphaned OCI layout", dir: "scan-1234567890", age: staleAge, wantGone: true}, + {name: "orphaned stereoscope extraction", dir: "syft-scan-1546187834", age: staleAge, wantGone: true}, + {name: "orphaned syft cataloging scratch", dir: "syft-cataloger-99", age: staleAge, wantGone: true}, + {name: "scan a peer may still be running", dir: "scan-inflight", age: time.Minute}, + {name: "grype download dir", dir: "grype-dl", age: staleAge}, + {name: "vulndb alongside the tmp dir", dir: "vulndb", age: staleAge}, + {name: "unrelated directory", dir: "cache", age: staleAge}, + } + + tmpDir := t.TempDir() + for _, tt := range tests { + mkStaleDir(t, tmpDir, tt.dir, 512, tt.age) + } + // A file, not a directory, that matches a swept prefix. Only directories + // are ours; a stray file is somebody else's business. + stray := filepath.Join(tmpDir, "scan-notes.txt") + if err := os.WriteFile(stray, []byte("hello"), 0o644); err != nil { + t.Fatalf("write %s: %v", stray, err) + } + when := time.Now().Add(-staleAge) + if err := os.Chtimes(stray, when, when); err != nil { + t.Fatalf("chtimes %s: %v", stray, err) + } + + sweepTmpDir(tmpDir, time.Hour) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := os.Stat(filepath.Join(tmpDir, tt.dir)) + switch { + case tt.wantGone && err == nil: + t.Errorf("%s survived the sweep; nothing else ever reclaims it", tt.dir) + case !tt.wantGone && err != nil: + t.Errorf("%s was removed by the sweep: %v", tt.dir, err) + } + }) + } + + if _, err := os.Stat(stray); err != nil { + t.Errorf("scan-notes.txt was removed by the sweep: %v", err) + } + if _, err := os.Stat(tmpDir); err != nil { + t.Errorf("the tmp dir itself was removed: %v", err) + } +} + +// TestSweepTmpDirDisabledAndMissing covers the two ways the sweep is asked to +// do nothing. Neither may touch anything, and neither may fail: this runs on +// the startup path, before any worker exists to report an error. +func TestSweepTmpDirDisabledAndMissing(t *testing.T) { + t.Run("zero max age disables the sweep", func(t *testing.T) { + tmpDir := t.TempDir() + stale := mkStaleDir(t, tmpDir, "scan-old", 16, staleAge) + + sweepTmpDir(tmpDir, 0) + + if _, err := os.Stat(stale); err != nil { + t.Errorf("sweep ran with max age 0 and removed %s: %v", stale, err) + } + }) + + t.Run("tmp dir does not exist", func(t *testing.T) { + sweepTmpDir(filepath.Join(t.TempDir(), "never-created"), time.Hour) + }) + + t.Run("empty tmp dir path", func(t *testing.T) { + sweepTmpDir("", time.Hour) + }) +} + +// TestSweepTmpDirContinuesPastAnUnremovableEntry is the "never fatal" half of +// the contract. One directory the process cannot remove must not cost the +// sweep the rest of the reclaim, and must not stop the scanner from starting. +func TestSweepTmpDirContinuesPastAnUnremovableEntry(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root ignores the directory permissions this test relies on") + } + + tmpDir := t.TempDir() + + // A stale scan dir made read-only, so RemoveAll cannot unlink what is + // inside it and fails partway through, the way a root-owned leftover or a + // busy mount would. + locked := mkStaleDir(t, tmpDir, "scan-locked", 32, staleAge) + if err := os.Chmod(locked, 0o555); err != nil { + t.Fatalf("chmod %s: %v", locked, err) + } + // Restore write permission or t.TempDir's own cleanup fails too. + t.Cleanup(func() { _ = os.Chmod(locked, 0o755) }) + + removable := mkStaleDir(t, tmpDir, "syft-scan-removable", 32, staleAge) + + sweepTmpDir(tmpDir, time.Hour) + + if _, err := os.Stat(locked); err != nil { + t.Errorf("expected the unremovable dir to still be there, got: %v", err) + } + if _, err := os.Stat(removable); err == nil { + t.Error("the sweep stopped at the unremovable entry instead of continuing") + } +} diff --git a/scanner/internal/scan/syft.go b/scanner/internal/scan/syft.go index be7dc31..2ae67ec 100644 --- a/scanner/internal/scan/syft.go +++ b/scanner/internal/scan/syft.go @@ -29,7 +29,7 @@ func generateSBOM(ctx context.Context, ociLayoutDir, sourceRef string) (*sbom.SB slog.Info("Generating SBOM with Syft", "ociLayout", ociLayoutDir, "source", sourceRef) // Create stereoscope OCI directory provider - tmpGen := file.NewTempDirGenerator("syft-scan") + tmpGen := file.NewTempDirGenerator(syftTempDirName) defer tmpGen.Cleanup() provider := oci.NewDirectoryProvider(tmpGen, ociLayoutDir) diff --git a/scanner/internal/scan/worker.go b/scanner/internal/scan/worker.go index df5cb06..b5b7ca4 100644 --- a/scanner/internal/scan/worker.go +++ b/scanner/internal/scan/worker.go @@ -61,6 +61,12 @@ func (wp *WorkerPool) Start(ctx context.Context) { os.Setenv("TMPDIR", wp.cfg.Vuln.TmpDir) } + // Reclaim what earlier processes left behind before any worker starts + // filling the directory again. A scan that is interrupted by a restart + // never runs its own cleanup, so without this every restart leaks the + // in-flight layout and extraction permanently. + sweepTmpDir(wp.cfg.Vuln.TmpDir, wp.cfg.Vuln.SweepMaxAge) + // Initialize vuln database on startup if enabled if wp.cfg.Vuln.Enabled { go func() {