amadan
objstore+cli: the review's should-fixes, and a real ETag laundering path
**Verdicts now bind to ETag AND last-modified, not the ETag alone.** For a single-part upload the ETag is the content MD5, and chosen-prefix MD5 collisions are practical on ordinary hardware — an attacker who controls both uploads could give a payload the ETag of the clean object it replaces and inherit its clean verdict, walking straight through the Deny. That is the exact laundering the binding was meant to stop. Last-modified moves on every overwrite and is not theirs to choose. Both halves come free on the listing page, so this costs nothing. The tag is now carlos-scan-version and the seam speaks of an object version rather than an etag, because that is what it is. **Tag writes preserve tags this scanner does not own.** PutObjectTagging replaces the whole set. The old reasoning — that an app cannot tag its own objects, so nothing could be there — holds for a bucket CARLOS minted and NOT for an adopted one, whose objects can carry tags from tooling still using them for lifecycle or cost allocation. Read-merge- write, at one extra call per object per lifetime rather than per sweep. **The signature line no longer reads as a claim about every object.** A verdict is kept while the bytes do not change, so a quiet store can report full coverage built entirely from old judgements against a database that is not the one printed beside them. It now says "engine database now:" and prints how many verdicts this sweep reached versus carried forward. **A failed sweep says which failure it was.** Engine unreachable and engine drift are the two an operator can act on, so they get sentinels and their own sentences; everything else stays "see the log", because backend errors carry bucket names, ARNs and request ids. **`scan enforce` no longer eats an app named `on` or `off`.** A bare on/off is the switch; one in a flag's value position is an app name. Swallowing it sent the enforcement change to whatever app the session defaulted to — a wrong-target mutation that looks like it worked. Docs: the "these verbs refuse without an engine" claim was false for the four that deliberately do not, and "three environment variables" was six. The three undocumented ones are now documented, with the budget arithmetic stated rather than left to be discovered.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YJkJkxZZLgW8X8A5tk69Sq
9 files changed,
+263
−77
apimode.go+7 −2docs/site/cli.md+20 −4internal/console/apiscan.go+15 −3internal/objstore/clamav.go+54 −23internal/objstore/clamav_test.go+74 −18internal/objstore/memscanner.go+11 −10internal/objstore/scanner.go+11 −4internal/objstore/sweep.go+42 −11scan.go+29 −2
diff --git a/apimode.go b/apimode.go| index 4d247f794..8d9c7365e 100644 |
| --- a/apimode.go |
| +++ b/apimode.go |
| @@ -1605,8 +1605,13 @@ type apiSweep struct { |
| Denied int64 `json:"denied"` |
| Failed int64 `json:"failed"` |
| Unscanned int64 `json:"unscanned"` |
| - Bytes int64 `json:"bytes"` |
| - After string `json:"after,omitempty"` |
| + // Fresh and Reused split Scanned by WHEN the verdict was reached. A clean |
| + // tag is kept while the object's bytes do not change, so a rollup can be |
| + // built almost entirely from old judgements against an older database. |
| + Fresh int64 `json:"fresh"` |
| + Reused int64 `json:"reused"` |
| + Bytes int64 `json:"bytes"` |
| + After string `json:"after,omitempty"` |
| } |
| type apiScanCoverage struct { |
diff --git a/docs/site/cli.md b/docs/site/cli.md| index d38f49c5d..e3d9db41b 100644 |
| --- a/docs/site/cli.md |
| +++ b/docs/site/cli.md |
| @@ -513,8 +513,10 @@ would confirm the app exists. |
| #### carlos store scan |
| Virus scanning over the objects in an app's store. **Off by default and |
| -opt-in**: a deployment that sets nothing has no engine, runs no sweep, and |
| -these verbs refuse with a sentence naming the variable that would wire one up. |
| +opt-in**: a deployment that sets nothing has no engine and runs no sweep. The |
| +verbs that need an engine — `grant`, `sweep`, `enforce on` — refuse with a |
| +sentence naming the variable that would wire one up. The rest keep working |
| +without one, deliberately; see below. |
| ```sh |
| carlos store scan request --app hello --plaintext |
| @@ -554,19 +556,33 @@ actually been looked at. A sweep still in progress says so and says which |
| numbers are stale; a first sweep that has not finished claims no coverage at |
| all. Signature-database age is printed, and called out when it is old, because |
| a clean verdict from three-month-old definitions is a weaker claim than a |
| -fresh one. |
| +fresh one. The engine's database version is reported as what it is **now** — |
| +a verdict is kept for as long as an object's bytes do not change, so a store |
| +that has been quiet for months can show full coverage made up entirely of older |
| +judgements. `status` prints how many verdicts this sweep actually reached and |
| +how many it carried forward, so the two are never confused. |
| **Enabled is not enforcing.** With scanning on but enforcement off, infected |
| objects are recorded and still readable. `carlos store status` says which of |
| the two you have. |
| -The operator side is three environment variables on the console: |
| +The operator side is six environment variables on the console: |
| | variable | default | meaning | |
| | --- | --- | --- | |
| | `CARLOS_STORE_SCANNER` | unset | `clamav` is the only engine this build accepts. Unset means no scanning at all. | |
| | `CARLOS_CLAMD_SOCKET` | `/run/clamav/clamd.ctl` | the unix socket to reach `clamd` on | |
| | `CARLOS_CLAMD_MAX_BYTES` | 25 MB | objects larger than this are recorded as skipped without being fetched; mirror `clamd`'s own `StreamMaxLength` | |
| +| `CARLOS_CLAMD_MAX_SIG_AGE` | 168h | how old the signature database may get before `scan status` warns | |
| +| `CARLOS_STORE_SCAN_BUDGET` | 60s | how much scanning one app gets per hourly tick | |
| +| `CARLOS_STORE_SCAN_INTERVAL` | 24h | the floor before a *completed* sweep is run again. A sweep still in progress ignores it — that is how a bucket bigger than one budget ever finishes. | |
| + |
| +The budget arithmetic is worth stating rather than discovering. An hourly tick |
| +at 60s is 24 minutes of scanning per app per day; at a few hundred objects a |
| +minute over `clamd`'s INSTREAM, a million-object bucket takes months, not days. |
| +The default is honest for small and medium stores. A deployment with large ones |
| +raises the budget or shortens the tick, and `scan status` says when a sweep has |
| +spanned more than one tick rather than implying its numbers are current. |
| Setting `CARLOS_STORE_SCANNER` without `CARLOS_STORE_BACKEND` is refused at |
| boot — a scanner with no object stores to scan is a misconfiguration, and one |
diff --git a/internal/console/apiscan.go b/internal/console/apiscan.go| index ba8ead373..f8e1e0399 100644 |
| --- a/internal/console/apiscan.go |
| +++ b/internal/console/apiscan.go |
| @@ -24,7 +24,6 @@ import ( |
| "context" |
| "crypto/ecdh" |
| "errors" |
| - "fmt" |
| "log" |
| "net/http" |
| "time" |
| @@ -533,8 +532,21 @@ func (s *Server) handleAPIScanSweep(w http.ResponseWriter, r *http.Request) { |
| return |
| } |
| log.Printf("console: api scan sweep %s/%s: %v", acct.Sqid, app, err) |
| - jsonError(w, http.StatusBadGateway, |
| - fmt.Sprintf("the sweep did not finish: %s", "see the console log; re-run to resume")) |
| + // The two causes an operator can act on are named. Everything else |
| + // stays "see the log": a backend error carries bucket names, ARNs and |
| + // request ids, and none of that belongs in a client sentence. |
| + switch { |
| + case errors.Is(err, objstore.ErrEngineUnreachable): |
| + jsonError(w, http.StatusServiceUnavailable, |
| + "the scanning engine is not answering — check clamd on the console box, then re-run") |
| + case errors.Is(err, objstore.ErrEngineDrift): |
| + jsonError(w, http.StatusConflict, |
| + "this store was scanned by a different engine — `carlos store scan revoke` then "+ |
| + "`carlos store scan grant` to move it, which restarts its coverage history") |
| + default: |
| + jsonError(w, http.StatusBadGateway, |
| + "the sweep did not finish — see the console log; re-run to resume from where it stopped") |
| + } |
| return |
| } |
| rec, _, ok := s.scanRecord(ctx, w, acct, app) |
diff --git a/internal/objstore/clamav.go b/internal/objstore/clamav.go| index 67c4643d0..a31246fbe 100644 |
| --- a/internal/objstore/clamav.go |
| +++ b/internal/objstore/clamav.go |
| @@ -106,13 +106,19 @@ func (c *clamavScanner) Enforcement() DenyTag { |
| // Verdict reports what the engine previously said about this exact version of |
| // the object. |
| // |
| -// A verdict is bound to the ETag it judged. An object rewritten under the same |
| -// key is a different object, and inheriting the old verdict would let anyone |
| -// launder a payload through a key that once held something clean — upload |
| -// clean, get tagged clean, overwrite with the payload, keep the tag. So a tag |
| -// whose recorded ETag does not match the caller's reads as unknown, and the |
| -// sweep scans it again. |
| -func (c *clamavScanner) Verdict(ctx context.Context, bucket, key, etag string) (Verdict, error) { |
| +// A verdict is bound to the object VERSION it judged — see ObjectVersion: the |
| +// ETag and the last-modified time together. |
| +// |
| +// Without that binding a key that once held something clean is a laundering |
| +// channel: upload clean, get tagged clean, overwrite with the payload, keep |
| +// the tag. The ETag alone is not quite enough for it, either. For a |
| +// single-part upload the ETag is the content MD5, and chosen-prefix MD5 |
| +// collisions are practical on ordinary hardware — so an attacker who controls |
| +// both uploads could produce a payload with the ETag of the clean object it |
| +// replaces. Last-modified moves on every overwrite and is not theirs to |
| +// choose. A tag whose recorded version does not match reads as unknown, and |
| +// the sweep scans it again. |
| +func (c *clamavScanner) Verdict(ctx context.Context, bucket, key, version string) (Verdict, error) { |
| out, err := c.s3.GetObjectTagging(ctx, &awss3.GetObjectTaggingInput{ |
| Bucket: &bucket, |
| Key: &key, |
| @@ -130,19 +136,19 @@ func (c *clamavScanner) Verdict(ctx context.Context, bucket, key, etag string) ( |
| return VerdictUnknown, fmt.Errorf("objstore: read tags %s/%s: %w", bucket, key, err) |
| } |
| - var verdict, taggedETag string |
| + var verdict, taggedVersion string |
| for _, t := range out.TagSet { |
| switch aws2s(t.Key) { |
| case TagVerdict: |
| verdict = aws2s(t.Value) |
| - case TagETag: |
| - taggedETag = aws2s(t.Value) |
| + case TagVersion: |
| + taggedVersion = aws2s(t.Value) |
| } |
| } |
| - if verdict == "" || taggedETag == "" { |
| + if verdict == "" || taggedVersion == "" { |
| return VerdictUnknown, nil |
| } |
| - if normalizeETag(taggedETag) != normalizeETag(etag) { |
| + if taggedVersion != version { |
| // The bytes changed under the verdict. Not a failure — just unknown |
| // again, which is exactly what makes the next sweep re-scan it. |
| return VerdictUnknown, nil |
| @@ -151,13 +157,13 @@ func (c *clamavScanner) Verdict(ctx context.Context, bucket, key, etag string) ( |
| } |
| // Scan streams one object past the engine and records the result. |
| -func (c *clamavScanner) Scan(ctx context.Context, bucket, key, etag string, size int64) (Verdict, error) { |
| +func (c *clamavScanner) Scan(ctx context.Context, bucket, key, version string, size int64) (Verdict, error) { |
| // The ceiling is checked BEFORE the fetch. The size is already on the |
| // listing page that got us here, so paying egress to pull bytes clamd has |
| // told us it will refuse is pure waste — and on a bucket full of large |
| // media that waste is the entire bill. |
| if c.maxBytes > 0 && size > c.maxBytes { |
| - if err := c.tag(ctx, bucket, key, etag, VerdictSkipped); err != nil { |
| + if err := c.tag(ctx, bucket, key, version, VerdictSkipped); err != nil { |
| return VerdictUnknown, err |
| } |
| return VerdictSkipped, nil |
| @@ -176,7 +182,7 @@ func (c *clamavScanner) Scan(ctx context.Context, bucket, key, etag string, size |
| // account in an adopted bucket, would stall that store's coverage |
| // forever while every surface reported a sweep merely "in progress". |
| if isDenied(err) { |
| - if terr := c.tag(ctx, bucket, key, etag, VerdictDenied); terr != nil { |
| + if terr := c.tag(ctx, bucket, key, version, VerdictDenied); terr != nil { |
| // Tagging it is best effort: if we cannot read the object we |
| // may well not be able to tag it either. The verdict still |
| // stands for this sweep's count. |
| @@ -210,7 +216,7 @@ func (c *clamavScanner) Scan(ctx context.Context, bucket, key, etag string, size |
| } |
| v := verdictFor(res.Status) |
| - if err := c.tag(ctx, bucket, key, etag, v); err != nil { |
| + if err := c.tag(ctx, bucket, key, version, v); err != nil { |
| return VerdictUnknown, err |
| } |
| return v, nil |
| @@ -236,18 +242,37 @@ func verdictFor(s clamd.Status) Verdict { |
| // tag writes the verdict, the ETag it applies to, and when it was reached. |
| // |
| -// PutObjectTagging replaces the whole tag set, which is safe here precisely |
| -// because of the permission the anti-forgery property already depends on: the |
| +// PutObjectTagging REPLACES the whole tag set, so anything already on the |
| +// object is preserved explicitly here rather than assumed absent. |
| +// |
| +// An earlier version of this reasoned that nothing could be there to lose: the |
| // store user has no s3:PutObjectTagging, and AWS requires that same permission |
| -// to set tags through x-amz-tagging on upload. So an app cannot have put a tag |
| -// on its own object by either route, and there is nothing of the tenant's to |
| -// destroy. One permission, two properties. |
| -func (c *clamavScanner) tag(ctx context.Context, bucket, key, etag string, v Verdict) error { |
| +// to set tags via x-amz-tagging on upload, so an app cannot tag its own object |
| +// by either route. That holds for a bucket CARLOS minted. It does not hold for |
| +// an ADOPTED one — `store grant --bucket` takes over a bucket whose objects |
| +// may carry tags written long before CARLOS scoped any credentials, by tooling |
| +// that is still using them for lifecycle rules or cost allocation. Replacing |
| +// the set blindly would delete them, silently, on first scan. |
| +func (c *clamavScanner) tag(ctx context.Context, bucket, key, version string, v Verdict) error { |
| set := []s3types.Tag{ |
| {Key: s2aws(TagVerdict), Value: s2aws(string(v))}, |
| - {Key: s2aws(TagETag), Value: s2aws(normalizeETag(etag))}, |
| + {Key: s2aws(TagVersion), Value: s2aws(version)}, |
| {Key: s2aws(TagAt), Value: s2aws(time.Now().UTC().Format(time.RFC3339))}, |
| } |
| + // Read-merge-write. One extra call per object per LIFETIME, not per sweep: |
| + // an object whose verdict still matches its ETag never reaches Scan again. |
| + if existing, err := c.s3.GetObjectTagging(ctx, &awss3.GetObjectTaggingInput{ |
| + Bucket: &bucket, |
| + Key: &key, |
| + }); err == nil { |
| + for _, t := range existing.TagSet { |
| + if !isScanTag(aws2s(t.Key)) { |
| + set = append(set, t) |
| + } |
| + } |
| + } |
| + // A tag set we cannot read is not a reason to refuse to record a verdict: |
| + // the verdict is the point, and S3 caps the set at ten anyway. |
| _, err := c.s3.PutObjectTagging(ctx, &awss3.PutObjectTaggingInput{ |
| Bucket: &bucket, |
| Key: &key, |
| @@ -259,6 +284,12 @@ func (c *clamavScanner) tag(ctx context.Context, bucket, key, etag string, v Ver |
| return nil |
| } |
| +// isScanTag is the set this scanner owns and may overwrite. Everything else on |
| +// the object belongs to whoever put it there. |
| +func isScanTag(key string) bool { |
| + return key == TagVerdict || key == TagVersion || key == TagAt |
| +} |
| + |
| // normalizeETag strips the quotes S3 wraps an ETag in. The listing and the tag |
| // must compare equal or every object re-scans forever, and only one of the two |
| // arrives quoted. |
diff --git a/internal/objstore/clamav_test.go b/internal/objstore/clamav_test.go| index 5c89494f6..21b042a09 100644 |
| --- a/internal/objstore/clamav_test.go |
| +++ b/internal/objstore/clamav_test.go |
| @@ -7,6 +7,7 @@ import ( |
| "io" |
| "strings" |
| "testing" |
| + "time" |
| "github.com/aws/smithy-go" |
| @@ -76,6 +77,15 @@ func (f *fakeEngine) Version(_ context.Context) (string, error) { |
| return f.version, nil |
| } |
| +// allowTagRead lets tag()'s read-merge-write see an empty tag set. Scan now |
| +// reads the existing tags before replacing them, so that a bucket CARLOS |
| +// adopted does not lose tags someone else's tooling put there. |
| +func (s *stubScanS3) allowTagRead(tags ...string) { |
| + s.getTagging = func(*awss3.GetObjectTaggingInput) (*awss3.GetObjectTaggingOutput, error) { |
| + return taggingOutput(tags...), nil |
| + } |
| +} |
| + |
| func objectBody(s string) *awss3.GetObjectOutput { |
| return &awss3.GetObjectOutput{Body: io.NopCloser(strings.NewReader(s))} |
| } |
| @@ -97,6 +107,7 @@ func tagsOf(in *awss3.PutObjectTaggingInput) map[string]string { |
| func TestClamAVSkipsOversizeWithoutFetching(t *testing.T) { |
| var wrote map[string]string |
| s3 := &stubScanS3{t: t} // getObject deliberately unset: calling it fails the test |
| + s3.allowTagRead() |
| s3.putTagging = func(in *awss3.PutObjectTaggingInput) (*awss3.PutObjectTaggingOutput, error) { |
| wrote = tagsOf(in) |
| return &awss3.PutObjectTaggingOutput{}, nil |
| @@ -127,6 +138,7 @@ func TestClamAVTagsVerdictWithETag(t *testing.T) { |
| s3.getObject = func(*awss3.GetObjectInput) (*awss3.GetObjectOutput, error) { |
| return objectBody("hello"), nil |
| } |
| + s3.allowTagRead() |
| s3.putTagging = func(in *awss3.PutObjectTaggingInput) (*awss3.PutObjectTaggingOutput, error) { |
| wrote = tagsOf(in) |
| return &awss3.PutObjectTaggingOutput{}, nil |
| @@ -134,15 +146,15 @@ func TestClamAVTagsVerdictWithETag(t *testing.T) { |
| eng := &fakeEngine{res: clamd.Result{Status: clamd.StatusClean}} |
| c := &clamavScanner{s3: s3, engine: eng, maxBytes: 1 << 20} |
| - v, err := c.Scan(context.Background(), "b", "k", `"d41d8cd9"`, 5) |
| + v, err := c.Scan(context.Background(), "b", "k", "d41d8cd9@2026-09-01T10:00:00Z", 5) |
| if err != nil { |
| t.Fatal(err) |
| } |
| if v != VerdictClean { |
| t.Fatalf("verdict = %q", v) |
| } |
| - if wrote[TagETag] != "d41d8cd9" { |
| - t.Fatalf("verdict is not bound to the etag it judged: %v", wrote) |
| + if wrote[TagVersion] != "d41d8cd9@2026-09-01T10:00:00Z" { |
| + t.Fatalf("verdict is not bound to the object version it judged: %v", wrote) |
| } |
| if wrote[TagAt] == "" { |
| t.Fatalf("no scan time recorded: %v", wrote) |
| @@ -179,6 +191,7 @@ func TestClamAVSizeExceededMapsToSkipped(t *testing.T) { |
| s3.getObject = func(*awss3.GetObjectInput) (*awss3.GetObjectOutput, error) { |
| return objectBody("hello"), nil |
| } |
| + s3.allowTagRead() |
| s3.putTagging = func(in *awss3.PutObjectTaggingInput) (*awss3.PutObjectTaggingOutput, error) { |
| wrote = tagsOf(in) |
| return &awss3.PutObjectTaggingOutput{}, nil |
| @@ -206,6 +219,7 @@ func TestClamAVEngineErrorIsRecordedAsFailed(t *testing.T) { |
| s3.getObject = func(*awss3.GetObjectInput) (*awss3.GetObjectOutput, error) { |
| return objectBody("hello"), nil |
| } |
| + s3.allowTagRead() |
| s3.putTagging = func(in *awss3.PutObjectTaggingInput) (*awss3.PutObjectTaggingOutput, error) { |
| wrote = tagsOf(in) |
| return &awss3.PutObjectTaggingOutput{}, nil |
| @@ -229,6 +243,7 @@ func TestClamAVInfectedVerdictMatchesEnforcementTag(t *testing.T) { |
| s3.getObject = func(*awss3.GetObjectInput) (*awss3.GetObjectOutput, error) { |
| return objectBody("x5O!P%"), nil |
| } |
| + s3.allowTagRead() |
| var wrote map[string]string |
| s3.putTagging = func(in *awss3.PutObjectTaggingInput) (*awss3.PutObjectTaggingOutput, error) { |
| wrote = tagsOf(in) |
| @@ -261,7 +276,7 @@ func taggingOutput(pairs ...string) *awss3.GetObjectTaggingOutput { |
| func TestClamAVVerdictIgnoresStaleETag(t *testing.T) { |
| s3 := &stubScanS3{t: t} |
| s3.getTagging = func(*awss3.GetObjectTaggingInput) (*awss3.GetObjectTaggingOutput, error) { |
| - return taggingOutput(TagVerdict, "clean", TagETag, "old-etag"), nil |
| + return taggingOutput(TagVerdict, "clean", TagVersion, "old-etag"), nil |
| } |
| c := &clamavScanner{s3: s3, engine: &fakeEngine{}} |
| @@ -274,22 +289,32 @@ func TestClamAVVerdictIgnoresStaleETag(t *testing.T) { |
| } |
| } |
| -// The listing gives a quoted ETag and the tag holds an unquoted one. If those |
| -// never compare equal, every object re-scans on every sweep forever — an |
| -// invisible bug that costs money rather than correctness. |
| -func TestClamAVVerdictMatchesAcrossETagQuoting(t *testing.T) { |
| - s3 := &stubScanS3{t: t} |
| - s3.getTagging = func(*awss3.GetObjectTaggingInput) (*awss3.GetObjectTaggingOutput, error) { |
| - return taggingOutput(TagVerdict, "clean", TagETag, "d41d8cd9"), nil |
| +// S3 hands back a QUOTED ETag. Normalisation happens once, where the version |
| +// token is built, so a stored tag and a fresh listing compare equal. If they |
| +// never did, every object would re-scan on every sweep forever — an invisible |
| +// bug that costs money rather than correctness. |
| +func TestScanObjectVersionNormalisesQuoting(t *testing.T) { |
| + at := time.Date(2026, 9, 1, 10, 0, 0, 0, time.UTC) |
| + quoted := ScanObject{Key: "k", ETag: `"d41d8cd9"`, Modified: at} |
| + bare := ScanObject{Key: "k", ETag: "d41d8cd9", Modified: at} |
| + if quoted.Version() != bare.Version() { |
| + t.Fatalf("quoting changed the version token: %q vs %q", quoted.Version(), bare.Version()) |
| } |
| - c := &clamavScanner{s3: s3, engine: &fakeEngine{}} |
| +} |
| - v, err := c.Verdict(context.Background(), "b", "k", `"d41d8cd9"`) |
| - if err != nil { |
| - t.Fatal(err) |
| - } |
| - if v != VerdictClean { |
| - t.Fatalf("verdict = %q, want clean — quoting broke the comparison", v) |
| +// The version moves when the object is overwritten, even if the bytes hash to |
| +// the same ETag. That is the whole point of folding last-modified in: a |
| +// single-part upload's ETag is the content MD5, and chosen-prefix MD5 |
| +// collisions are practical — an attacker controlling both uploads could give a |
| +// payload the ETag of the clean object it replaces and inherit its verdict. |
| +// Last-modified is not theirs to choose. |
| +func TestScanObjectVersionChangesOnOverwriteWithCollidingETag(t *testing.T) { |
| + etag := "d41d8cd9" // same hash, as a collision would give |
| + before := ScanObject{Key: "k", ETag: etag, Modified: time.Date(2026, 9, 1, 10, 0, 0, 0, time.UTC)} |
| + after := ScanObject{Key: "k", ETag: etag, Modified: time.Date(2026, 9, 1, 11, 0, 0, 0, time.UTC)} |
| + if before.Version() == after.Version() { |
| + t.Fatalf("an overwrite kept the same version token (%q) — the old verdict would be inherited", |
| + before.Version()) |
| } |
| } |
| @@ -372,6 +397,7 @@ func TestClamAVUnreadableObjectIsDeniedNotAPassFailure(t *testing.T) { |
| s3.getObject = func(*awss3.GetObjectInput) (*awss3.GetObjectOutput, error) { |
| return nil, apiErr(code) |
| } |
| + s3.allowTagRead() |
| s3.putTagging = func(in *awss3.PutObjectTaggingInput) (*awss3.PutObjectTaggingOutput, error) { |
| wrote = tagsOf(in) |
| return &awss3.PutObjectTaggingOutput{}, nil |
| @@ -442,3 +468,33 @@ func TestClamAVTransportErrorStillFailsThePass(t *testing.T) { |
| t.Fatalf("verdict = %q, want unknown", v) |
| } |
| } |
| + |
| +// An adopted bucket's objects can carry tags written long before CARLOS ever |
| +// scoped credentials to it — lifecycle rules, cost allocation. PutObjectTagging |
| +// replaces the whole set, so those have to be carried across explicitly. |
| +// Deleting a tenant's tags silently on first scan would be a real loss with no |
| +// trace of what went. |
| +func TestClamAVPreservesForeignTags(t *testing.T) { |
| + var wrote map[string]string |
| + s3 := &stubScanS3{t: t} |
| + s3.getObject = func(*awss3.GetObjectInput) (*awss3.GetObjectOutput, error) { |
| + return objectBody("hello"), nil |
| + } |
| + s3.allowTagRead("cost-centre", "research", TagVerdict, "clean", TagVersion, "stale") |
| + s3.putTagging = func(in *awss3.PutObjectTaggingInput) (*awss3.PutObjectTaggingOutput, error) { |
| + wrote = tagsOf(in) |
| + return &awss3.PutObjectTaggingOutput{}, nil |
| + } |
| + c := &clamavScanner{s3: s3, engine: &fakeEngine{res: clamd.Result{Status: clamd.StatusClean}}, maxBytes: 1 << 20} |
| + |
| + if _, err := c.Scan(context.Background(), "b", "k", "fresh", 5); err != nil { |
| + t.Fatal(err) |
| + } |
| + if wrote["cost-centre"] != "research" { |
| + t.Fatalf("a tag this scanner does not own was destroyed: %v", wrote) |
| + } |
| + // Its own tags ARE replaced, not duplicated. |
| + if wrote[TagVersion] != "fresh" { |
| + t.Fatalf("the scanner did not overwrite its own stale etag tag: %v", wrote) |
| + } |
| +} |
diff --git a/internal/objstore/memscanner.go b/internal/objstore/memscanner.go| index 550cb2dd4..fb0701435 100644 |
| --- a/internal/objstore/memscanner.go |
| +++ b/internal/objstore/memscanner.go |
| @@ -16,11 +16,12 @@ type MemScanner struct { |
| scanAs map[string]Verdict |
| } |
| -// scanRecord is one object's verdict AND the etag it was judged against. The |
| -// etag is not decoration: Verdict answers Unknown when it disagrees, which is |
| -// what stops a rewritten object inheriting the old bytes' verdict. |
| +// scanRecord is one object's verdict AND the object version it was judged |
| +// against. The version is not decoration: Verdict answers Unknown when it |
| +// disagrees, which is what stops a rewritten object inheriting the old bytes' |
| +// verdict. |
| type scanRecord struct { |
| - etag string |
| + version string |
| verdict Verdict |
| } |
| @@ -43,10 +44,10 @@ func (s *MemScanner) Fail(method string, err error) { |
| } |
| // Put seeds a verdict so a test can start from a partly-scanned bucket. |
| -func (s *MemScanner) Put(bucket, key, etag string, v Verdict) { |
| +func (s *MemScanner) Put(bucket, key, version string, v Verdict) { |
| s.mu.Lock() |
| defer s.mu.Unlock() |
| - s.verdicts[memScanKey(bucket, key)] = scanRecord{etag: etag, verdict: v} |
| + s.verdicts[memScanKey(bucket, key)] = scanRecord{version: version, verdict: v} |
| } |
| // IsEnabled lets Teardown's test prove the scanner was disabled while the |
| @@ -85,20 +86,20 @@ func (s *MemScanner) EnsureDisabled(ctx context.Context, bucket string) error { |
| return nil |
| } |
| -func (s *MemScanner) Verdict(ctx context.Context, bucket, key, etag string) (Verdict, error) { |
| +func (s *MemScanner) Verdict(ctx context.Context, bucket, key, version string) (Verdict, error) { |
| s.mu.Lock() |
| defer s.mu.Unlock() |
| if err := s.takeFailure("Verdict"); err != nil { |
| return VerdictUnknown, err |
| } |
| rec, ok := s.verdicts[memScanKey(bucket, key)] |
| - if !ok || rec.etag != etag { |
| + if !ok || rec.version != version { |
| return VerdictUnknown, nil |
| } |
| return rec.verdict, nil |
| } |
| -func (s *MemScanner) Scan(ctx context.Context, bucket, key, etag string, size int64) (Verdict, error) { |
| +func (s *MemScanner) Scan(ctx context.Context, bucket, key, version string, size int64) (Verdict, error) { |
| s.mu.Lock() |
| defer s.mu.Unlock() |
| if err := s.takeFailure("Scan"); err != nil { |
| @@ -108,7 +109,7 @@ func (s *MemScanner) Scan(ctx context.Context, bucket, key, etag string, size in |
| if got, ok := s.scanAs[key]; ok { |
| v = got |
| } |
| - s.verdicts[memScanKey(bucket, key)] = scanRecord{etag: etag, verdict: v} |
| + s.verdicts[memScanKey(bucket, key)] = scanRecord{version: version, verdict: v} |
| return v, nil |
| } |
diff --git a/internal/objstore/scanner.go b/internal/objstore/scanner.go| index 4011d7501..d3c1bbee3 100644 |
| --- a/internal/objstore/scanner.go |
| +++ b/internal/objstore/scanner.go |
| @@ -27,7 +27,7 @@ type Scanner interface { |
| // has been rewritten since it was judged, and bytes nobody looked at have |
| // no verdict. Absent is (VerdictUnknown, nil) too — the same idiom Get |
| // uses for an absent record, because "not scanned" is a state. |
| - Verdict(ctx context.Context, bucket, key, etag string) (Verdict, error) |
| + Verdict(ctx context.Context, bucket, key, version string) (Verdict, error) |
| // Scan scans one object and records its verdict against etag. It returns |
| // the verdict when the engine is synchronous (ClamAV) and VerdictUnknown |
| // when the engine scans asynchronously (GuardDuty). The sweep counts what |
| @@ -39,7 +39,7 @@ type Scanner interface { |
| // egress for bytes the scanner will reject is pure waste, and the |
| // alternative — a HeadObject per object per sweep — is a round trip on |
| // every object forever. |
| - Scan(ctx context.Context, bucket, key, etag string, size int64) (Verdict, error) |
| + Scan(ctx context.Context, bucket, key, version string, size int64) (Verdict, error) |
| // Enforcement names the object tag this engine writes for an infected |
| // object, so the IAM policy writer can deny reads on it (spec §8). |
| // Engine-specific by necessity: a Deny statement must name a literal key |
| @@ -93,12 +93,19 @@ type DenyTag struct { |
| Value string `json:"value"` |
| } |
| -// The tags the ClamAV engine writes. TagETag is what binds a verdict to the |
| +// The tags the ClamAV engine writes. TagVersion is what binds a verdict to the |
| // bytes it judged: without it a tag says only "some bytes under this key were |
| // clean once", and a re-uploaded object would inherit its predecessor's |
| // verdict. |
| +// |
| +// The version is the ETag AND the last-modified time (ScanObject.Version), not |
| +// the ETag alone. A single-part upload's ETag is the content MD5, and |
| +// chosen-prefix MD5 collisions are practical on ordinary hardware — an |
| +// attacker controlling both uploads could otherwise give a payload the ETag of |
| +// the clean object it replaces. Last-modified moves on every overwrite and is |
| +// not theirs to choose. |
| const ( |
| TagVerdict = "carlos-scan" |
| - TagETag = "carlos-scan-etag" |
| + TagVersion = "carlos-scan-version" |
| TagAt = "carlos-scan-at" |
| ) |
diff --git a/internal/objstore/sweep.go b/internal/objstore/sweep.go| index 4da9e8a02..aa834015d 100644 |
| --- a/internal/objstore/sweep.go |
| +++ b/internal/objstore/sweep.go |
| @@ -26,13 +26,28 @@ import ( |
| // work lost when a tick's budget runs out mid-page. |
| const sweepPageSize = 1000 |
| -// ScanObject is one key as the listing reports it. Size and ETag come free on |
| -// the listing page, which is why the sweep never needs a HeadObject: the size |
| -// decides the ceiling before any fetch, and the ETag binds the verdict. |
| +// ScanObject is one key as the listing reports it. Size, ETag and |
| +// last-modified all come free on the listing page, which is why the sweep |
| +// never needs a HeadObject: the size decides the ceiling before any fetch, and |
| +// the other two bind the verdict. |
| type ScanObject struct { |
| - Key string |
| - ETag string |
| - Size int64 |
| + Key string |
| + ETag string |
| + Modified time.Time |
| + Size int64 |
| +} |
| + |
| +// Version is the token a verdict is bound to: this exact version of these |
| +// exact bytes. |
| +// |
| +// Both halves earn their place. The ETag alone would be the content hash, and |
| +// for a single-part upload that hash is MD5 — chosen-prefix MD5 collisions are |
| +// practical, so an attacker controlling both uploads could give a payload the |
| +// ETag of the clean object it replaces and inherit its verdict. Last-modified |
| +// alone would be too coarse. Together they are cheap and neither is the |
| +// uploader's to choose. |
| +func (o ScanObject) Version() string { |
| + return normalizeETag(o.ETag) + "@" + o.Modified.UTC().Format(time.RFC3339) |
| } |
| // ObjectLister is the one listing call the sweep makes, narrowed so the walk |
| @@ -71,6 +86,9 @@ func (l *s3Lister) ListPage(ctx context.Context, bucket, startAfter string, limi |
| if o.Size != nil { |
| obj.Size = *o.Size |
| } |
| + if o.LastModified != nil { |
| + obj.Modified = *o.LastModified |
| + } |
| objs = append(objs, obj) |
| } |
| truncated := out.IsTruncated != nil && *out.IsTruncated |
| @@ -82,6 +100,19 @@ func (l *s3Lister) ListPage(ctx context.Context, bucket, startAfter string, limi |
| // to stop writing. RunSweep converts it to a nil error. |
| var errStandDown = errors.New("objstore: sweep standing down") |
| +// The two sweep failures an operator can actually act on, named so a door can |
| +// tell them apart from a transport error and say which it was. Everything else |
| +// stays a logged detail: bucket names, ARNs and request ids do not belong in a |
| +// client sentence. |
| +var ( |
| + // ErrEngineUnreachable is the engine not answering at all. |
| + ErrEngineUnreachable = errors.New("objstore: the scanning engine is not answering") |
| + // ErrEngineDrift is a record naming a different engine than the one |
| + // configured. Refusing beats recounting another engine's verdicts as |
| + // unscanned, which would destroy the coverage history. |
| + ErrEngineDrift = errors.New("objstore: the record names a different scanning engine") |
| +) |
| + |
| // RunSweep advances one bucket's coverage by up to budget of work and returns |
| // the sidecar as it now stands. |
| // |
| @@ -124,15 +155,15 @@ func RunSweep(ctx context.Context, s release.Store, lister ObjectLister, sc Scan |
| engine, signature, err := sc.Provenance(ctx) |
| if err != nil { |
| - return nil, fmt.Errorf("objstore: %s/%s: engine unreachable: %w", acct, app, err) |
| + return nil, fmt.Errorf("%w (%s/%s): %v", ErrEngineUnreachable, acct, app, err) |
| } |
| // Engine drift refuses rather than guesses. Verdicts already in the bucket |
| // were written by a different engine under its own conventions, and |
| // recounting them as unscanned would destroy the coverage history. An |
| // operator changes engine by revoking and re-granting, deliberately. |
| if rec.Scanning.Enabled.Engine != engine { |
| - return nil, fmt.Errorf("objstore: %s/%s: record says engine %q, configured engine is %q", |
| - acct, app, rec.Scanning.Enabled.Engine, engine) |
| + return nil, fmt.Errorf("%w (%s/%s): record says %q, configured engine is %q", |
| + ErrEngineDrift, acct, app, rec.Scanning.Enabled.Engine, engine) |
| } |
| side, etag, err := GetSidecar(ctx, s, acct, app) |
| @@ -170,7 +201,7 @@ func RunSweep(ctx context.Context, s release.Store, lister ObjectLister, sc Scan |
| if err := ctx.Err(); err != nil { |
| return nil, err |
| } |
| - v, err := sc.Verdict(ctx, bucket, o.Key, o.ETag) |
| + v, err := sc.Verdict(ctx, bucket, o.Key, o.Version()) |
| if errors.Is(err, ErrObjectGone) { |
| continue // deleted since the listing; not ours to count |
| } |
| @@ -190,7 +221,7 @@ func RunSweep(ctx context.Context, s release.Store, lister ObjectLister, sc Scan |
| // neither can stall the page: an error here abandons the page |
| // unwritten, and a page that fails identically every tick |
| // means the bucket's sweep never advances past it. |
| - v, err = sc.Scan(ctx, bucket, o.Key, o.ETag, o.Size) |
| + v, err = sc.Scan(ctx, bucket, o.Key, o.Version(), o.Size) |
| if errors.Is(err, ErrObjectGone) { |
| continue |
| } |
diff --git a/scan.go b/scan.go| index 49e3b2491..61bfd65e6 100644 |
| --- a/scan.go |
| +++ b/scan.go |
| @@ -166,7 +166,18 @@ func runStoreScanEnforce(args []string) { |
| var on bool |
| var seen bool |
| rest := make([]string, 0, len(args)) |
| - for _, a := range args { |
| + // Only a BARE on/off is the switch. `--app on` names an app called "on", |
| + // and eating it here would send the enforcement change to whatever app the |
| + // session happened to default to — a wrong-target mutation that looks like |
| + // it worked. Anything sitting in a flag's value position is left alone. |
| + takesValue := map[string]bool{"--app": true, "-app": true, |
| + "--account": true, "-account": true, "--account-id": true, "-account-id": true, |
| + "--console": true, "-console": true} |
| + for i, a := range args { |
| + if i > 0 && takesValue[args[i-1]] { |
| + rest = append(rest, a) |
| + continue |
| + } |
| switch a { |
| case "on": |
| on, seen = true, true |
| @@ -268,7 +279,10 @@ func printScanSignature(w io.Writer, res apiScanReply) { |
| if res.Signature == "" { |
| return |
| } |
| - fmt.Fprintf(w, " signatures: %s\n", res.Signature) |
| + // "now", not bare "signatures". A clean tag is reused for as long as the |
| + // object's ETag matches, so most of the rollup below may have been judged |
| + // against an older database than this one. printScanCoverage says how much. |
| + fmt.Fprintf(w, " engine database now: %s\n", res.Signature) |
| age, ok := signatureAge(res.Signature, time.Now()) |
| if !ok { |
| return |
| @@ -327,6 +341,19 @@ func printScanCoverage(w io.Writer, res apiScanReply) { |
| fmt.Fprintf(w, " not scanned: %d too large or refused, %d denied, %d failed\n", |
| last.Skipped, last.Denied, last.Failed) |
| } |
| + // How much of that coverage was actually reached by THIS sweep. A verdict |
| + // is kept for as long as the object's bytes do not change, so a store that |
| + // has been quiet for months can report full coverage made up entirely of |
| + // old judgements — against signatures that are not the ones named above. |
| + // Printing the split is the difference between an honest number and a |
| + // number that merely looks current. |
| + if last.Reused > 0 { |
| + fmt.Fprintf(w, " of those, %d were judged during this sweep and %d carried forward\n", |
| + last.Fresh, last.Reused) |
| + mutedf(w, " a carried-forward verdict was reached against whatever database was") |
| + mutedf(w, " current then, not the one above; carlos store scan sweep re-runs nothing") |
| + mutedf(w, " whose bytes have not changed") |
| + } |
| if last.Infected > 0 { |
| warnf(w, "%d infected object(s) found — carlos store scan enforce --app %s on blocks reads of them", |
| last.Infected, res.App) |