mirror of
https://tangled.org/evan.jarrett.net/at-container-registry
synced 2026-09-20 01:04:15 +00:00
docs: record which scanner findings were reviewed and accepted
Two of the open findings were reviewed and are working as intended, which is worth writing down: an unrecorded decision reads identically to an oversight six months later, and both of these look like bugs from the code alone. The severity strip shows four buckets and does not account for Grype's Negligible and Unknown. That is deliberate. Two more colours would clutter the strip and those severities rarely merit attention, so the four boxes summarise what matters rather than partitioning the total, and the detail table still lists everything. The two places the gap stays visible are recorded with the cheaper route, should either ever be worth closing. GOMEMLIMIT not scaling with scanner.workers is a safeguard for a memory-constrained host, not a tuning parameter, and the intended direction is to remove it once scanners have their own nodes rather than to grow it. The separate open question about the cgroup ceiling is unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U1Km3N3uUmeGaj7VbaM8PF
This commit is contained in:
co-authored by
Claude Opus 5
parent
850c416dc8
commit
7cf58de78e
+38
-4
@@ -56,11 +56,10 @@ existing holds, plus an `origin, status` index.
|
||||
| Severity | What | Detail |
|
||||
|---|---|---|
|
||||
| HIGH | **No keepalive, read deadline or read limit on the scanner WebSocket.** Now *more* load-bearing than when audited: a half-open connection holds its advertised capacity out of the dispatch budget and keeps its rows marked as its own until the timeouts fire. | section 4 |
|
||||
| HIGH | **Severity buckets do not sum to the reported total.** Measured: debian:11-slim reports 211 vulnerabilities beside four boxes summing to 105. User-visible and wrong. | section 6 |
|
||||
| MEDIUM | **Rescanning identical content produces different digests**, so every rescan uploads a fresh blob and orphans the previous one. Accumulates on the stale-scan schedule. | section 6 |
|
||||
| MEDIUM | **`checkPredecessor` caches a `false` from an unreachable hold forever.** `pkg/hold/gc/gc.go:2027` already carries the corrected logic for the same problem; the scan broadcaster was never brought along. | section 5 |
|
||||
| MEDIUM | **`handleResult` does two S3 uploads and a CAR commit inline on the reader goroutine**, on `context.Background()` with no deadline. That scanner's messages are not read while it runs. | section 5 |
|
||||
| MEDIUM | **Vertical concurrency is still constrained**: `GOMEMLIMIT` is process-wide and does not scale with `scanner.workers`, a database refresh still stalls one process's workers behind the write lock, and the scanner queue has no seq or digest dedupe. Scaling out sidesteps all three; scaling up does not. | section 4 |
|
||||
| MEDIUM | **Vertical concurrency is still constrained**: a database refresh stalls one process's workers behind the write lock, and the scanner queue has no seq or digest dedupe. Scaling out sidesteps both; scaling up does not. (`GOMEMLIMIT` is a separate matter, see "Accepted" below.) | section 4 |
|
||||
| MEDIUM | **The cgroup memory ceiling needs rechecking.** See section 3. | section 3 |
|
||||
| LOW | Reports are ~14.5 KB per rendered row (1.8 MB for 125 matches), parsed on every render. The report descriptor hardcodes Grype `v0.107.1` against a `v0.118.0` dependency. The report's embedded `summary` is read by nothing. | section 6 |
|
||||
| LOW | `pkg/config/viper.go:34` discards `ReadInConfig()`'s error, so a malformed YAML file is silently ignored whole, in scanner, hold and appview alike. | section 9 |
|
||||
@@ -69,6 +68,39 @@ existing holds, plus an `origin, status` index.
|
||||
| LOW | `assigned_at` is stored as local-offset RFC3339 while `created_at` normalises to `...Z`. Correct today, breaks across a DST transition on a non-UTC host. | section 12 |
|
||||
| — | Test infrastructure: `e2e.Start` writes the package-level `scan.JobCooldown`, so two harnesses alive in one test race. Bound each harness in a `t.Run` subtest, or make the cooldown per-pool. | section 6 |
|
||||
|
||||
### Accepted, not defects
|
||||
|
||||
**Severity buckets do not sum to the reported total.** Measured: debian:11-slim
|
||||
reports 211 vulnerabilities beside four boxes summing to 105, because Grype's
|
||||
Negligible and Unknown severities have no bucket anywhere in the chain. Section
|
||||
6 has the full data.
|
||||
|
||||
**Decision: current behaviour is as intended.** Two more colours on the
|
||||
vulnerability strip would clutter the UI, and Negligible and Unknown are not
|
||||
worth flagging in most cases. The four boxes are a deliberate summary of what
|
||||
merits attention, not an exhaustive partition of the total, and the detail table
|
||||
already lists every finding including the unbucketed ones.
|
||||
|
||||
Two narrow consequences remain visible and are recorded so the next reader does
|
||||
not mistake them for oversights: the headline "N vulnerabilities" beside the
|
||||
four boxes is a larger number than they sum to on multi-severity distros, and an
|
||||
image whose findings are *entirely* Negligible or Unknown has `Total > 0`, so it
|
||||
skips the "Clean" branch and renders a coloured strip reading `0 0 0 0`. If
|
||||
either is ever worth addressing, the cheap route is to derive the headline from
|
||||
the four counts rather than adding a fifth bucket, which needs no lexicon or
|
||||
protocol change.
|
||||
|
||||
**The 512 MiB `GOMEMLIMIT` does not scale with `scanner.workers`.** Process-wide
|
||||
rather than per-worker, so it is divided among concurrent scans: +1.7% for a
|
||||
single scan, +24% time and 5.2x GC cycles at two, +92% and 9.4x at four.
|
||||
|
||||
**Decision: keep as is for now.** It exists as a safeguard for a
|
||||
memory-constrained production host, not as a tuning parameter. The intended
|
||||
direction is to **remove the limit** once scanners run on their own nodes,
|
||||
rather than to scale it with the worker count. Until then it is doing the job it
|
||||
was added for. See section 3 for the deployment context, and note the separate
|
||||
open question there about the cgroup ceiling, which is a different thing.
|
||||
|
||||
### Deliberately not fixed
|
||||
|
||||
Blob 404 from a garbage-collected layer, a config blob that is not JSON, and a
|
||||
@@ -129,8 +161,10 @@ The most serious class in this report after the crashes, and the least fixed.
|
||||
|
||||
Content verification landed, so the scanner no longer vouches for bytes a
|
||||
compliant OCI client would reject. But a zero-byte layer still scans clean,
|
||||
producing an empty SBOM and a healthy-looking result, and the severity buckets
|
||||
shown to users do not account for every vulnerability counted.
|
||||
producing an empty SBOM and a healthy-looking result.
|
||||
|
||||
The severity buckets not accounting for every counted vulnerability was
|
||||
reviewed and accepted as intended; see "Accepted, not defects" in section 1.
|
||||
|
||||
### Workarounds
|
||||
|
||||
|
||||
Reference in New Issue
Block a user