amadan
plan: object storage virus scanning, thirteen tasks
Ordered TDD tasks from the draft-4 spec. GuardDuty is deliberately unscheduled: its engine stays behind the spec's measurement gate until someone confirms an on-demand scan tags an object uploaded before tagging was enabled. ClamAV carries v1 alone, which is the point of demoting GuardDuty in the first place. Two tasks change interfaces the whole package compiles against — EnsureUser gaining a deny, and Teardown gaining a Scanner — so the plan flags them as unsafe to parallelise.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 file changed,
+1482
−0
docs/superpowers/plans/2026-09-01-object-storage-virus-scanning.md+1482 −0
diff --git a/docs/superpowers/plans/2026-09-01-object-storage-virus-scanning.md b/docs/superpowers/plans/2026-09-01-object-storage-virus-scanning.md| new file mode 100644 |
| index 000000000..7dbd0d164 |
| --- /dev/null |
| +++ b/docs/superpowers/plans/2026-09-01-object-storage-virus-scanning.md |
| @@ -0,0 +1,1482 @@ |
| +# Object Storage Virus Scanning Implementation Plan |
| + |
| +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. |
| + |
| +**Goal:** Give each app's object store an opt-in, reconciling virus scanner that can answer "how many objects carry no recorded verdict", with ClamAV as the shipped engine. |
| + |
| +**Architecture:** A `Scanner` seam beside the existing `Provisioner`, an idempotent `Sweep` walk that lists a bucket and records verdicts as S3 object tags bound to the object's ETag, a per-app `scan.json` sidecar holding the coverage rollup, and an hourly console pass driving it. Enforcement reuses the app's inline IAM policy via a deny-on-tag statement. |
| + |
| +**Tech Stack:** Go (stdlib first — no new dependencies), AWS SDK v2 S3, `clamd` over a unix socket using its INSTREAM protocol (hand-rolled, per the house "hand-roll small clients over SDKs" rule). |
| + |
| +**Spec:** `docs/superpowers/specs/2026-09-01-object-storage-virus-scanning-design.md` |
| + |
| +## Global Constraints |
| + |
| +- `go build ./...`, `go vet ./...`, `go test ./... -count=1` clean before any commit lands. `gofmt -l .` clean over the whole tree (`make test` checks it). |
| +- Every change lands through a PR. Never a direct commit to `main`, never a local merge. |
| +- No new third-party dependencies. "Adding a dependency is a decision, not a default." The clamd client is hand-rolled. |
| +- **`grant` and `provisioned` in `store.json` must not be renamed or restructured.** `release.PurgeApp` parses them with its own local struct and refuses box-side auto-purge when either is present. Additive keys only. |
| +- **`s3:PutObjectTagging` must never be added to the app user's Allow statement.** The anti-forgery property in spec §8 depends on the app being unable to tag its own objects. |
| +- Default off: `CARLOS_STORE_SCANNER` unset means no engine is built and the feature is inert. |
| +- Secrets never enter the record. Nothing in this plan writes a credential to `store.json` or `scan.json`. |
| +- The scan pass goroutine starts from `serve()` in `cmd/console/main.go`, **never** from `NewServer` (`economicspass.go:35-38`). |
| +- Verdicts are always bound to an ETag. A verdict recorded against a different ETag reads as `VerdictUnknown`. |
| + |
| +--- |
| + |
| +## File Structure |
| + |
| +**Created:** |
| +- `internal/objstore/scanner.go` — the `Scanner` seam, `Verdict`, `DenyTag`. |
| +- `internal/objstore/memscanner.go` — in-memory fake, mirroring `memprovisioner.go`. |
| +- `internal/objstore/scanner_contract_test.go` — the idempotence contract both engines must pass. |
| +- `internal/objstore/sidecar.go` — `ScanSidecar`, `Sweep`, its key, its reader/writer. |
| +- `internal/objstore/sweep.go` — the `Sweep` walk. |
| +- `internal/clamd/clamd.go` — the INSTREAM/VERSION client. Its own package: a wire protocol with no CARLOS concepts in it, testable over a `net.Pipe`. |
| +- `internal/objstore/clamav.go` — `clamavScanner`, joining the clamd client to S3. |
| +- `internal/console/scanpass.go` — the hourly driver. |
| +- `internal/console/apiscan.go` — the console doors. |
| +- `scan.go` — the `carlos store scan` CLI family. |
| + |
| +**Modified:** |
| +- `internal/objstore/objstore.go` — `ScanBlock` on `Record`. |
| +- `internal/objstore/provisioner.go` — `EnsureUser` gains `deny *DenyTag`. |
| +- `internal/objstore/aws.go` — `policyStatement.Condition`, `storePolicyJSON` deny, `EnsureUser` signature. |
| +- `internal/objstore/memprovisioner.go` — signature plus policy state. |
| +- `internal/objstore/walk.go` — hoist `EnsureUser` in `Grant`; `Teardown` takes a `Scanner`. |
| +- `internal/console/appsettings.go`, `internal/console/ownership.go` — `Teardown` callers and the destroy pre-check. |
| +- `cmd/console/main.go` — build the scanner, start the pass. |
| +- `store.go` — dispatch `scan`. |
| +- `docs/site/cli.md` — the six sub-verbs. |
| + |
| +--- |
| + |
| +### Task 1: The Scanner seam and its fake |
| + |
| +**Files:** |
| +- Create: `internal/objstore/scanner.go` |
| +- Create: `internal/objstore/memscanner.go` |
| +- Test: `internal/objstore/scanner_contract_test.go` |
| + |
| +**Interfaces:** |
| +- Consumes: nothing. |
| +- Produces: `Scanner`, `Verdict` (+ the six constants), `DenyTag{Key, Value string}`, `MemScanner` with `NewMemScanner() *MemScanner` and a `Fail(method string, err error)` hook mirroring `MemProvisioner.takeFailure`. |
| + |
| +- [ ] **Step 1: Write the failing contract test** |
| + |
| +```go |
| +package objstore |
| + |
| +import ( |
| + "context" |
| + "testing" |
| +) |
| + |
| +// scannerContract is run against every Scanner implementation. Idempotence is |
| +// the property: every method is called twice and the second call must be a |
| +// success, because the sweep re-runs after a half-failure. |
| +func scannerContract(t *testing.T, s Scanner, bucket string) { |
| + t.Helper() |
| + ctx := context.Background() |
| + |
| + for i := 0; i < 2; i++ { |
| + if err := s.EnsureEnabled(ctx, bucket); err != nil { |
| + t.Fatalf("EnsureEnabled call %d: %v", i+1, err) |
| + } |
| + } |
| + // An object nobody has scanned has no verdict, and that is a state. |
| + v, err := s.Verdict(ctx, bucket, "absent.txt", "etag-1") |
| + if err != nil { |
| + t.Fatalf("Verdict on absent object: %v", err) |
| + } |
| + if v != VerdictUnknown { |
| + t.Fatalf("Verdict on absent object = %q, want %q", v, VerdictUnknown) |
| + } |
| + for i := 0; i < 2; i++ { |
| + if err := s.EnsureDisabled(ctx, bucket); err != nil { |
| + t.Fatalf("EnsureDisabled call %d: %v", i+1, err) |
| + } |
| + } |
| +} |
| + |
| +func TestMemScannerContract(t *testing.T) { |
| + scannerContract(t, NewMemScanner(), "carlos-store-tito-bab-app") |
| +} |
| + |
| +// A verdict is bound to the bytes it judged: the same key at a different etag |
| +// has no verdict, so the sweep rescans it (spec §3.1). |
| +func TestMemScannerVerdictIsETagBound(t *testing.T) { |
| + ctx := context.Background() |
| + s := NewMemScanner() |
| + s.Put("b", "k", "etag-1", VerdictClean) |
| + |
| + if v, _ := s.Verdict(ctx, "b", "k", "etag-1"); v != VerdictClean { |
| + t.Fatalf("same etag = %q, want %q", v, VerdictClean) |
| + } |
| + if v, _ := s.Verdict(ctx, "b", "k", "etag-2"); v != VerdictUnknown { |
| + t.Fatalf("changed etag = %q, want %q", v, VerdictUnknown) |
| + } |
| +} |
| +``` |
| + |
| +- [ ] **Step 2: Run it to verify it fails** |
| + |
| +Run: `go test ./internal/objstore/ -run 'TestMemScanner' -count=1` |
| +Expected: FAIL — `undefined: Scanner`, `undefined: NewMemScanner`. |
| + |
| +- [ ] **Step 3: Write `scanner.go`** |
| + |
| +```go |
| +// Package-level doc lives in objstore.go. This file is the scanning-engine |
| +// seam (spec §3) — everything the sweep needs an engine to do, and nothing an |
| +// app ever calls. |
| +package objstore |
| + |
| +import "context" |
| + |
| +// Scanner is the scanning-engine seam. Like Provisioner, every method is |
| +// idempotent — "already enabled" and "already gone" are successes — because |
| +// the sweep walk that drives one must be re-runnable after a half-failure. |
| +// |
| +// v1 ships the ClamAV implementation and this package's in-memory fake. A |
| +// GuardDuty scanner arrives here later without touching the sweep, once the |
| +// tagging question in spec §4.2 has been measured. |
| +type Scanner interface { |
| + EnsureEnabled(ctx context.Context, bucket string) error |
| + // EnsureDisabled releases whatever EnsureEnabled took. Teardown calls it |
| + // BEFORE deleting the bucket, or the backend estate outlives the record |
| + // that names it (spec §7.3). |
| + EnsureDisabled(ctx context.Context, bucket string) error |
| + // Verdict answers the recorded verdict for one object AT etag. A verdict |
| + // recorded against a different etag is (VerdictUnknown, nil): the object |
| + // 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) |
| + // 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 it gets back, so a synchronous engine's first pass is not a pass |
| + // full of "unscanned". |
| + Scan(ctx context.Context, bucket, key, etag string) (Verdict, error) |
| + // Enforcement names the tag this engine writes, so the IAM policy writer |
| + // can deny reads on it. Engine-specific by necessity: a Deny statement |
| + // must name a literal key and value. |
| + Enforcement() DenyTag |
| + // Capacity reports how many buckets this engine can still protect and the |
| + // cap; (-1, -1) is uncapped. ClamAV is uncapped; GuardDuty caps at 25 per |
| + // account per region, which is why it is not the default engine. |
| + Capacity(ctx context.Context) (remaining, cap int, err error) |
| + // Provenance names what did the scanning — engine and signature version — |
| + // for the evidence record. "Scanned with what?" is an audit question. |
| + Provenance(ctx context.Context) (engine, version string, err error) |
| +} |
| + |
| +// Verdict is one object's scan result. The zero value is "no recorded |
| +// verdict", which is a state and not an error. |
| +type Verdict string |
| + |
| +const ( |
| + VerdictUnknown Verdict = "" |
| + VerdictClean Verdict = "clean" |
| + VerdictInfected Verdict = "infected" |
| + // VerdictSkipped is too large, unsupported, or unextractable. It is NOT |
| + // clean, and the evidence must never blur the two. |
| + VerdictSkipped Verdict = "skipped" |
| + VerdictDenied Verdict = "denied" |
| + VerdictFailed Verdict = "failed" |
| +) |
| + |
| +// DenyTag is the object tag an engine writes for an infected object, and the |
| +// literal a Deny statement matches on (spec §8). It lives in the record |
| +// rather than being asked of a Scanner, because Grant takes a Provisioner and |
| +// has no Scanner to ask. |
| +type DenyTag struct { |
| + Key string `json:"key"` |
| + Value string `json:"value"` |
| +} |
| + |
| +// Tag keys the ClamAV engine writes. The etag tag is what binds a verdict to |
| +// the bytes it judged. |
| +const ( |
| + TagVerdict = "carlos-scan" |
| + TagETag = "carlos-scan-etag" |
| + TagAt = "carlos-scan-at" |
| +) |
| +``` |
| + |
| +- [ ] **Step 4: Write `memscanner.go`** |
| + |
| +```go |
| +package objstore |
| + |
| +import ( |
| + "context" |
| + "sync" |
| +) |
| + |
| +// MemScanner is the in-memory Scanner every walk test runs against, mirroring |
| +// MemProvisioner: no network, no clamd, and a failure hook so a test can make |
| +// any single method fail once and prove the walk survives it. |
| +type MemScanner struct { |
| + mu sync.Mutex |
| + enabled map[string]bool |
| + verdicts map[string]scanRecord // bucket + "\x00" + key |
| + failures map[string]error |
| +} |
| + |
| +type scanRecord struct { |
| + etag string |
| + verdict Verdict |
| +} |
| + |
| +func NewMemScanner() *MemScanner { |
| + return &MemScanner{ |
| + enabled: map[string]bool{}, |
| + verdicts: map[string]scanRecord{}, |
| + failures: map[string]error{}, |
| + } |
| +} |
| + |
| +// Fail arms a one-shot failure for the named method. |
| +func (s *MemScanner) Fail(method string, err error) { |
| + s.mu.Lock() |
| + defer s.mu.Unlock() |
| + s.failures[method] = err |
| +} |
| + |
| +// Put seeds a verdict, so a test can start from a partly-scanned bucket. |
| +func (s *MemScanner) Put(bucket, key, etag string, v Verdict) { |
| + s.mu.Lock() |
| + defer s.mu.Unlock() |
| + s.verdicts[bucket+"\x00"+key] = scanRecord{etag: etag, verdict: v} |
| +} |
| + |
| +func (s *MemScanner) takeFailure(method string) error { |
| + if err, ok := s.failures[method]; ok { |
| + delete(s.failures, method) |
| + return err |
| + } |
| + return nil |
| +} |
| + |
| +func (s *MemScanner) EnsureEnabled(ctx context.Context, bucket string) error { |
| + s.mu.Lock() |
| + defer s.mu.Unlock() |
| + if err := s.takeFailure("EnsureEnabled"); err != nil { |
| + return err |
| + } |
| + s.enabled[bucket] = true // already enabled is a success |
| + return nil |
| +} |
| + |
| +func (s *MemScanner) EnsureDisabled(ctx context.Context, bucket string) error { |
| + s.mu.Lock() |
| + defer s.mu.Unlock() |
| + if err := s.takeFailure("EnsureDisabled"); err != nil { |
| + return err |
| + } |
| + delete(s.enabled, bucket) // already gone is a success |
| + return nil |
| +} |
| + |
| +func (s *MemScanner) Verdict(ctx context.Context, bucket, key, etag string) (Verdict, error) { |
| + s.mu.Lock() |
| + defer s.mu.Unlock() |
| + if err := s.takeFailure("Verdict"); err != nil { |
| + return VerdictUnknown, err |
| + } |
| + rec, ok := s.verdicts[bucket+"\x00"+key] |
| + if !ok || rec.etag != etag { |
| + return VerdictUnknown, nil |
| + } |
| + return rec.verdict, nil |
| +} |
| + |
| +func (s *MemScanner) Scan(ctx context.Context, bucket, key, etag string) (Verdict, error) { |
| + s.mu.Lock() |
| + defer s.mu.Unlock() |
| + if err := s.takeFailure("Scan"); err != nil { |
| + return VerdictUnknown, err |
| + } |
| + s.verdicts[bucket+"\x00"+key] = scanRecord{etag: etag, verdict: VerdictClean} |
| + return VerdictClean, nil |
| +} |
| + |
| +func (s *MemScanner) Enforcement() DenyTag { |
| + return DenyTag{Key: TagVerdict, Value: string(VerdictInfected)} |
| +} |
| + |
| +func (s *MemScanner) Capacity(ctx context.Context) (int, int, error) { return -1, -1, nil } |
| + |
| +func (s *MemScanner) Provenance(ctx context.Context) (string, string, error) { |
| + return "memscanner", "test", nil |
| +} |
| + |
| +var _ Scanner = (*MemScanner)(nil) |
| +``` |
| + |
| +- [ ] **Step 5: Run the tests to verify they pass** |
| + |
| +Run: `go test ./internal/objstore/ -run 'TestMemScanner' -count=1 -v` |
| +Expected: PASS, both tests. |
| + |
| +- [ ] **Step 6: Commit** |
| + |
| +```bash |
| +git add internal/objstore/scanner.go internal/objstore/memscanner.go internal/objstore/scanner_contract_test.go |
| +git commit -m "objstore: the Scanner seam and its in-memory fake" |
| +``` |
| + |
| +--- |
| + |
| +### Task 2: The record's scanning block |
| + |
| +**Files:** |
| +- Modify: `internal/objstore/objstore.go` |
| +- Test: `internal/objstore/objstore_test.go`, `internal/objstore/walk_test.go` |
| + |
| +**Interfaces:** |
| +- Consumes: `DenyTag` (Task 1). |
| +- Produces: `ScanRequested{At time.Time, By string, Encrypted bool}`, `ScanEnabled{Engine string, At time.Time, By string}`, `ScanBlock{Requested *ScanRequested, Enabled *ScanEnabled, Enforce *DenyTag}`, and `Record.Scanning *ScanBlock`. |
| + |
| +- [ ] **Step 1: Write the failing tests** |
| + |
| +Add to `internal/objstore/objstore_test.go`: |
| + |
| +```go |
| +// The scanning block round-trips, and its three sub-blocks are null-or-present |
| +// so a client can tell "requested" from "enabled" without inspecting fields. |
| +func TestScanBlockRoundTrip(t *testing.T) { |
| + now := time.Now().UTC().Truncate(time.Second) |
| + rec := &Record{ |
| + App: "titogo", |
| + Scanning: &ScanBlock{ |
| + Requested: &ScanRequested{At: now, By: "m@example.com", Encrypted: false}, |
| + Enabled: &ScanEnabled{Engine: "clamav", At: now, By: "op@example.com"}, |
| + Enforce: &DenyTag{Key: TagVerdict, Value: string(VerdictInfected)}, |
| + }, |
| + } |
| + body, err := json.Marshal(rec) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + var back Record |
| + if err := json.Unmarshal(body, &back); err != nil { |
| + t.Fatal(err) |
| + } |
| + if back.Scanning == nil || back.Scanning.Enabled == nil { |
| + t.Fatal("scanning.enabled did not round-trip") |
| + } |
| + if back.Scanning.Enabled.Engine != "clamav" { |
| + t.Fatalf("engine = %q, want clamav", back.Scanning.Enabled.Engine) |
| + } |
| + if back.Scanning.Enforce == nil || back.Scanning.Enforce.Value != "infected" { |
| + t.Fatal("enforce did not round-trip") |
| + } |
| +} |
| + |
| +// An absent scanning block stays absent in the JSON — a deployment that never |
| +// used scanning must not grow a null key in every store record. |
| +func TestScanBlockOmittedWhenAbsent(t *testing.T) { |
| + body, err := json.Marshal(&Record{App: "titogo"}) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if bytes.Contains(body, []byte("scanning")) { |
| + t.Fatalf("absent scanning block leaked into JSON: %s", body) |
| + } |
| +} |
| +``` |
| + |
| +Add to `internal/objstore/walk_test.go`, beside `TestPurgeGuardSeesOurRecord`: |
| + |
| +```go |
| +// The scanning block is ADDITIVE: release.PurgeApp parses this shape with its |
| +// own local struct (it cannot import objstore), and must keep refusing on |
| +// grant/provisioned while ignoring keys it has never heard of. This is the |
| +// claim objstore.go's PARSER LIST comment makes; without this test the claim |
| +// is unasserted. |
| +func TestPurgeGuardIgnoresScanningBlock(t *testing.T) { |
| + raw := []byte(`{ |
| + "schema": 1, |
| + "app": "titogo", |
| + "grant": {"backend":"aws","by":"op@example.com"}, |
| + "scanning": {"enabled":{"engine":"clamav","by":"op@example.com"}} |
| + }`) |
| + if !release.StoreRecordBlocksPurge(raw) { |
| + t.Fatal("a record with grant + scanning must still block auto-purge") |
| + } |
| + |
| + rawNoEstate := []byte(`{ |
| + "schema": 1, |
| + "app": "titogo", |
| + "scanning": {"requested":{"by":"m@example.com"}} |
| + }`) |
| + if release.StoreRecordBlocksPurge(rawNoEstate) { |
| + t.Fatal("a scanning-only record names no backend estate and must not block purge") |
| + } |
| +} |
| +``` |
| + |
| +> **Note for the implementer:** `release.StoreRecordBlocksPurge` may not be exported today. Read `internal/release/trash.go` around lines 202-226 first. If the guard is unexported, export a thin wrapper with that name whose only job is to make this claim testable, and say so in its doc comment. Do **not** change the guard's logic. |
| + |
| +- [ ] **Step 2: Run to verify they fail** |
| + |
| +Run: `go test ./internal/objstore/ -run 'TestScanBlock|TestPurgeGuardIgnores' -count=1` |
| +Expected: FAIL — `unknown field Scanning`, `undefined: release.StoreRecordBlocksPurge`. |
| + |
| +- [ ] **Step 3: Add the types to `objstore.go`** |
| + |
| +```go |
| +// ScanRequested is the MEMBER's half of scanning: they asked for it, and they |
| +// attested what kind of objects the store holds. Encrypted means client-side |
| +// encrypted — the objects reach the store as ciphertext, so a scanner cannot |
| +// read them and no coverage is claimed (spec §2.2). Server-side encryption |
| +// (SSE-S3/SSE-KMS) is NOT this: it is transparent to a credentialled reader |
| +// and scans perfectly well. |
| +type ScanRequested struct { |
| + At time.Time `json:"at"` |
| + By string `json:"by"` |
| + Encrypted bool `json:"encrypted"` |
| +} |
| + |
| +// ScanEnabled is the OPERATOR's half: enabling spends money in the |
| +// deployment's own cloud account, so it sits behind CARLOS_STORE_OPERATORS |
| +// exactly as the store grant does. |
| +type ScanEnabled struct { |
| + Engine string `json:"engine"` // "clamav" | "guardduty" |
| + At time.Time `json:"at"` |
| + By string `json:"by"` |
| +} |
| + |
| +// ScanBlock is the scanning half of the record — the same three-author shape |
| +// the store itself has. It holds NO rollup: sweep counts live in the |
| +// scan.json sidecar, whose single writer is the sweep, so a multi-minute walk |
| +// never contends with the doors that write this object. |
| +// |
| +// Enforce holds the DenyTag ITSELF rather than a bool. Grant takes a |
| +// Provisioner and has no Scanner to ask, so the record must carry everything |
| +// the policy writer needs — which is also what lets `scan enforce off` work on |
| +// a deployment with no engine built. |
| +type ScanBlock struct { |
| + Requested *ScanRequested `json:"requested,omitempty"` |
| + Enabled *ScanEnabled `json:"enabled,omitempty"` |
| + Enforce *DenyTag `json:"enforce,omitempty"` |
| +} |
| +``` |
| + |
| +Add the field to `Record`, after `Provisioned`: |
| + |
| +```go |
| + Scanning *ScanBlock `json:"scanning,omitempty"` |
| +``` |
| + |
| +Extend the PARSER LIST comment on `Record` with one sentence: |
| + |
| +```go |
| +// `scanning` is additive and release's local struct ignores it, which |
| +// TestPurgeGuardIgnoresScanningBlock asserts. That freedom does NOT extend to |
| +// `grant` and `provisioned`. |
| +``` |
| + |
| +- [ ] **Step 4: Run to verify they pass** |
| + |
| +Run: `go test ./internal/objstore/ ./internal/release/ -count=1` |
| +Expected: PASS, including the pre-existing `TestPurgeGuardSeesOurRecord`. |
| + |
| +- [ ] **Step 5: Commit** |
| + |
| +```bash |
| +git add internal/objstore/objstore.go internal/objstore/objstore_test.go internal/objstore/walk_test.go internal/release/trash.go |
| +git commit -m "objstore: the record's scanning block, and the additive-purge claim asserted" |
| +``` |
| + |
| +--- |
| + |
| +### Task 3: The scan sidecar |
| + |
| +**Files:** |
| +- Create: `internal/objstore/sidecar.go` |
| +- Test: `internal/objstore/sidecar_test.go` |
| + |
| +**Interfaces:** |
| +- Consumes: `release.Store`, `release.AppPrefix`. |
| +- Produces: `ScanKey(acct, app) string`, `ScanSidecar`, `Sweep`, `GetSidecar(ctx, s, acct, app) (*ScanSidecar, string, error)`, `PutSidecar(ctx, s, acct, app, sc, etag) (string, error)`, `ErrSidecarMismatch`, `SidecarSchema`. |
| + |
| +- [ ] **Step 1: Write the failing tests** |
| + |
| +```go |
| +package objstore |
| + |
| +import ( |
| + "context" |
| + "testing" |
| +) |
| + |
| +func TestSidecarRoundTrip(t *testing.T) { |
| + ctx := context.Background() |
| + st := release.NewDirStore(t.TempDir()) // mirror whatever the package's other tests use |
| + sc := &ScanSidecar{ |
| + Account: "bab", |
| + App: "titogo", |
| + LastComplete: &Sweep{Objects: 12481, Scanned: 12479, Skipped: 2, Engine: "clamav"}, |
| + } |
| + if _, err := PutSidecar(ctx, st, "bab", "titogo", sc, ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + back, _, err := GetSidecar(ctx, st, "bab", "titogo") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if back.LastComplete == nil || back.LastComplete.Objects != 12481 { |
| + t.Fatalf("rollup did not round-trip: %+v", back) |
| + } |
| + if back.Schema != SidecarSchema { |
| + t.Fatalf("schema = %d, want %d", back.Schema, SidecarSchema) |
| + } |
| +} |
| + |
| +// Absent is (nil, "", nil) — no sidecar is a state, not an error, the same |
| +// idiom Get uses for an absent record. |
| +func TestSidecarAbsentIsNotAnError(t *testing.T) { |
| + sc, etag, err := GetSidecar(context.Background(), release.NewDirStore(t.TempDir()), "bab", "titogo") |
| + if err != nil || sc != nil || etag != "" { |
| + t.Fatalf("absent sidecar = (%v, %q, %v), want (nil, \"\", nil)", sc, etag, err) |
| + } |
| +} |
| + |
| +// accounts migrate copies every key under an app's prefix, so a sidecar can |
| +// arrive under an account it was not written for, carrying rollups for a |
| +// bucket named after the old one. The reader refuses rather than reporting |
| +// another account's coverage as this one's. |
| +func TestSidecarRefusesAccountMismatch(t *testing.T) { |
| + ctx := context.Background() |
| + st := release.NewDirStore(t.TempDir()) |
| + // Written as bab/titogo, then read as zed/titogo. |
| + if _, err := PutSidecar(ctx, st, "bab", "titogo", |
| + &ScanSidecar{Account: "bab", App: "titogo"}, ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + raw, _, err := st.Get(ctx, ScanKey("bab", "titogo")) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, err := st.Put(ctx, ScanKey("zed", "titogo"), raw, ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, _, err := GetSidecar(ctx, st, "zed", "titogo"); !errors.Is(err, ErrSidecarMismatch) { |
| + t.Fatalf("mismatched sidecar error = %v, want ErrSidecarMismatch", err) |
| + } |
| +} |
| + |
| +// An unknown schema is refused, not guessed at — the rule ReadAppTombstone |
| +// already applies. |
| +func TestSidecarRefusesUnknownSchema(t *testing.T) { |
| + ctx := context.Background() |
| + st := release.NewDirStore(t.TempDir()) |
| + if _, err := st.Put(ctx, ScanKey("bab", "titogo"), |
| + []byte(`{"schema":99,"account":"bab","app":"titogo"}`), ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, _, err := GetSidecar(ctx, st, "bab", "titogo"); err == nil { |
| + t.Fatal("an unknown schema must be refused") |
| + } |
| +} |
| +``` |
| + |
| +> **Note:** read `internal/objstore/objstore_test.go` and `walk_test.go` first to copy the exact store constructor those tests use; substitute it for `release.NewDirStore` above if the name differs. |
| + |
| +- [ ] **Step 2: Run to verify they fail** |
| + |
| +Run: `go test ./internal/objstore/ -run TestSidecar -count=1` |
| +Expected: FAIL — `undefined: ScanSidecar`. |
| + |
| +- [ ] **Step 3: Write `sidecar.go`** |
| + |
| +```go |
| +// The scan sidecar (spec §5): apps/<acct>/<app>/scan.json, the coverage |
| +// rollup and the compliance artifact. |
| +// |
| +// It is a SEPARATE OBJECT from store.json on purpose. The sweep writes once |
| +// per page over minutes; the create, grant and rotate doors write store.json |
| +// too. Sharing one object would mean a page-write racing a repair walk and |
| +// losing — so the rollup gets its own object with a single writer, and the |
| +// CAS discipline shrinks to "another sweep is ahead of me, stand down". |
| +package objstore |
| + |
| +import ( |
| + "context" |
| + "encoding/json" |
| + "errors" |
| + "fmt" |
| + "os" |
| + "time" |
| + |
| + "github.com/carlosframework/platform/internal/release" |
| +) |
| + |
| +const SidecarSchema = 1 |
| + |
| +// ErrSidecarMismatch is a sidecar whose body disagrees with its path. |
| +var ErrSidecarMismatch = errors.New("objstore: scan sidecar names another app") |
| + |
| +func ScanKey(acct, app string) string { return release.AppPrefix(acct, app) + "scan.json" } |
| + |
| +// Sweep is one reconciliation pass. Objects is what was listed; the six |
| +// verdict counters are what was found. Scanned and Unscanned are NOT |
| +// complements: Skipped, Denied and Failed are recorded verdicts that are not |
| +// coverage, which is why the audit sentence names two numbers. |
| +type Sweep struct { |
| + StartedAt time.Time `json:"started_at"` |
| + At time.Time `json:"at"` |
| + Engine string `json:"engine"` |
| + Signature string `json:"signature,omitempty"` |
| + Objects int64 `json:"objects"` |
| + Scanned int64 `json:"scanned"` |
| + Infected int64 `json:"infected"` |
| + Skipped int64 `json:"skipped"` |
| + Denied int64 `json:"denied"` |
| + Failed int64 `json:"failed"` |
| + Unscanned int64 `json:"unscanned"` |
| + Bytes int64 `json:"bytes"` |
| + // After is the last key of the last written page — the resume point. Empty |
| + // means this Sweep is not mid-flight. |
| + After string `json:"after,omitempty"` |
| +} |
| + |
| +// ScanSidecar holds the in-flight sweep and the last completed one. |
| +// |
| +// TWO SWEEPS, NOT ONE. A single field would have to mean both "in progress" |
| +// and "the answer", and the tick after a sweep completed would overwrite the |
| +// completed counts with page-one partials — destroying the compliance artifact |
| +// on every cycle. Current accumulates; on the last page it moves to |
| +// LastComplete, which is the only thing an audit answer ever quotes. |
| +type ScanSidecar struct { |
| + Schema int `json:"schema"` |
| + Account string `json:"account"` |
| + App string `json:"app"` |
| + // NotApplicable is a store whose member attested client-side encryption: |
| + // a scanner cannot read ciphertext, so no sweep runs and no coverage is |
| + // claimed (spec §2.2). |
| + NotApplicable bool `json:"not_applicable,omitempty"` |
| + Current *Sweep `json:"current,omitempty"` |
| + LastComplete *Sweep `json:"last_complete,omitempty"` |
| +} |
| + |
| +// GetSidecar reads the sidecar. Absent is (nil, "", nil). |
| +func GetSidecar(ctx context.Context, s release.Store, acct, app string) (*ScanSidecar, string, error) { |
| + if !release.ValidAccount(acct) { |
| + return nil, "", fmt.Errorf("objstore: invalid account %q", acct) |
| + } |
| + if !release.ValidApp(app) { |
| + return nil, "", fmt.Errorf("objstore: invalid app %q", app) |
| + } |
| + raw, etag, err := s.Get(ctx, ScanKey(acct, app)) |
| + if err != nil { |
| + if errors.Is(err, os.ErrNotExist) { |
| + return nil, "", nil |
| + } |
| + return nil, "", err |
| + } |
| + var sc ScanSidecar |
| + if err := json.Unmarshal(raw, &sc); err != nil { |
| + return nil, "", fmt.Errorf("objstore: %s: %w", ScanKey(acct, app), err) |
| + } |
| + if sc.Schema != SidecarSchema { |
| + return nil, "", fmt.Errorf("objstore: %s: schema %d is not %d", |
| + ScanKey(acct, app), sc.Schema, SidecarSchema) |
| + } |
| + // accounts migrate copies every key under the prefix, so a sidecar can |
| + // arrive under an account it was never written for. |
| + if sc.Account != acct || sc.App != app { |
| + return nil, "", fmt.Errorf("%w: body says %s/%s, path says %s/%s", |
| + ErrSidecarMismatch, sc.Account, sc.App, acct, app) |
| + } |
| + return &sc, etag, nil |
| +} |
| + |
| +// PutSidecar writes with the caller's CAS position; etag "" is create-only. |
| +func PutSidecar(ctx context.Context, s release.Store, acct, app string, sc *ScanSidecar, etag string) (string, error) { |
| + sc.Schema = SidecarSchema |
| + sc.Account = acct |
| + sc.App = app |
| + body, err := json.MarshalIndent(sc, "", " ") |
| + if err != nil { |
| + return "", err |
| + } |
| + return s.Put(ctx, ScanKey(acct, app), append(body, '\n'), etag) |
| +} |
| +``` |
| + |
| +- [ ] **Step 4: Run to verify they pass** |
| + |
| +Run: `go test ./internal/objstore/ -run TestSidecar -count=1 -v` |
| +Expected: PASS, all four. |
| + |
| +- [ ] **Step 5: Commit** |
| + |
| +```bash |
| +git add internal/objstore/sidecar.go internal/objstore/sidecar_test.go |
| +git commit -m "objstore: the scan sidecar, with schema and account refusals" |
| +``` |
| + |
| +--- |
| + |
| +### Task 4: EnsureUser gains the deny |
| + |
| +**Files:** |
| +- Modify: `internal/objstore/provisioner.go`, `internal/objstore/aws.go`, `internal/objstore/memprovisioner.go`, `internal/objstore/walk.go:184` |
| +- Test: `internal/objstore/aws_test.go`, `internal/objstore/memprovisioner_test.go` |
| + |
| +**Interfaces:** |
| +- Consumes: `DenyTag` (Task 1). |
| +- Produces: `EnsureUser(ctx, name, path, bucketARN string, deny *DenyTag) (arn string, err error)` on `Provisioner`; `MemProvisioner.PolicyFor(user string) string`. |
| + |
| +- [ ] **Step 1: Write the failing tests** |
| + |
| +Add to `internal/objstore/aws_test.go`: |
| + |
| +```go |
| +// A nil deny must produce TODAY'S document byte for byte. Every existing |
| +// store re-walks through this path, and a policy that differs by even a key |
| +// order would rewrite the whole estate for no reason. |
| +func TestStorePolicyJSONUnchangedWithoutDeny(t *testing.T) { |
| + got := storePolicyJSON("arn:aws:s3:::carlos-store-tito-bab-titogo", nil) |
| + want := `{"Version":"2012-10-17","Statement":[` + |
| + `{"Effect":"Allow","Action":["s3:ListBucket"],"Resource":"arn:aws:s3:::carlos-store-tito-bab-titogo"},` + |
| + `{"Effect":"Allow","Action":["s3:GetObject","s3:PutObject","s3:DeleteObject","s3:GetObjectTagging"],"Resource":"arn:aws:s3:::carlos-store-tito-bab-titogo/*"}]}` |
| + if got != want { |
| + t.Fatalf("policy without deny:\n got %s\nwant %s", got, want) |
| + } |
| +} |
| + |
| +// With a deny, a third statement appears — and s3:PutObjectTagging is still |
| +// absent, because the whole anti-forgery property is that an app cannot mark |
| +// its own object clean. |
| +func TestStorePolicyJSONWithDeny(t *testing.T) { |
| + got := storePolicyJSON("arn:aws:s3:::b", &DenyTag{Key: TagVerdict, Value: "infected"}) |
| + if !strings.Contains(got, `"Effect":"Deny"`) { |
| + t.Fatalf("no Deny statement: %s", got) |
| + } |
| + if !strings.Contains(got, `"s3:ExistingObjectTag/carlos-scan":"infected"`) { |
| + t.Fatalf("deny does not condition on the verdict tag: %s", got) |
| + } |
| + if strings.Contains(got, "s3:PutObjectTagging") { |
| + t.Fatalf("app must never be able to tag its own objects: %s", got) |
| + } |
| +} |
| +``` |
| + |
| +Add to `internal/objstore/memprovisioner_test.go`: |
| + |
| +```go |
| +// The fake records the policy it was asked to write, so walk tests can assert |
| +// that enforcement actually reached the backend. |
| +func TestMemProvisionerRecordsPolicy(t *testing.T) { |
| + p := NewMemProvisioner() |
| + if _, err := p.EnsureUser(context.Background(), "u", UserPath, "arn:aws:s3:::b", |
| + &DenyTag{Key: TagVerdict, Value: "infected"}); err != nil { |
| + t.Fatal(err) |
| + } |
| + if !strings.Contains(p.PolicyFor("u"), `"Effect":"Deny"`) { |
| + t.Fatalf("policy not recorded: %s", p.PolicyFor("u")) |
| + } |
| +} |
| +``` |
| + |
| +- [ ] **Step 2: Run to verify they fail** |
| + |
| +Run: `go test ./internal/objstore/ -run 'TestStorePolicyJSON|TestMemProvisionerRecordsPolicy' -count=1` |
| +Expected: FAIL — too many arguments to `storePolicyJSON`, `undefined: PolicyFor`. |
| + |
| +- [ ] **Step 3: Implement** |
| + |
| +In `aws.go`, give `policyStatement` a condition and rebuild the document: |
| + |
| +```go |
| +type policyStatement struct { |
| + Effect string `json:"Effect"` |
| + Action []string `json:"Action"` |
| + Resource string `json:"Resource"` |
| + Condition map[string]map[string]string `json:"Condition,omitempty"` |
| +} |
| + |
| +// storePolicyJSON is the exact document every store user's inline policy |
| +// carries. deny == nil is the historical two-statement document, byte for |
| +// byte — every healthy store re-walks through here now that Grant calls |
| +// EnsureUser unconditionally, and a gratuitous difference would rewrite the |
| +// whole estate. |
| +// |
| +// s3:GetObjectTagging is in the Allow so an app can read the verdict on its |
| +// own object. s3:PutObjectTagging is NOT, and must never be: the deny below |
| +// is only meaningful while an app cannot mark its own object clean. |
| +func storePolicyJSON(bucketARN string, deny *DenyTag) string { |
| + doc := policyDocument{ |
| + Version: "2012-10-17", |
| + Statement: []policyStatement{ |
| + {Effect: "Allow", Action: []string{"s3:ListBucket"}, Resource: bucketARN}, |
| + {Effect: "Allow", Action: []string{ |
| + "s3:GetObject", "s3:PutObject", "s3:DeleteObject", "s3:GetObjectTagging", |
| + }, Resource: bucketARN + "/*"}, |
| + }, |
| + } |
| + if deny != nil { |
| + doc.Statement = append(doc.Statement, policyStatement{ |
| + Effect: "Deny", |
| + Action: []string{"s3:GetObject"}, |
| + Resource: bucketARN + "/*", |
| + Condition: map[string]map[string]string{ |
| + "StringEquals": {"s3:ExistingObjectTag/" + deny.Key: deny.Value}, |
| + }, |
| + }) |
| + } |
| + b, _ := json.Marshal(doc) |
| + return string(b) |
| +} |
| +``` |
| + |
| +Thread `deny` through `awsProvisioner.EnsureUser` into that call; update the `Provisioner` interface in `provisioner.go`; update `MemProvisioner.EnsureUser` to store `storePolicyJSON(bucketARN, deny)` on its `MemUser` and add `PolicyFor`. Update the single call site at `walk.go:184` to pass `nil` for now — Task 5 wires the record's value. |
| + |
| +- [ ] **Step 4: Run the whole package** |
| + |
| +Run: `go build ./... && go test ./internal/objstore/ ./internal/console/ -count=1` |
| +Expected: PASS. Fix every compile error from the signature change (`aws_test.go`, `walk_test.go`, `memprovisioner_test.go`, `internal/console/deletion_test.go`). |
| + |
| +- [ ] **Step 5: Commit** |
| + |
| +```bash |
| +git add -A internal/objstore internal/console |
| +git commit -m "objstore: EnsureUser writes the deny, and the app can read its own verdicts" |
| +``` |
| + |
| +--- |
| + |
| +### Task 5: Grant writes the policy unconditionally |
| + |
| +**Files:** |
| +- Modify: `internal/objstore/walk.go` (`Grant`, around `:169-190`) |
| +- Test: `internal/objstore/walk_test.go` |
| + |
| +**Interfaces:** |
| +- Consumes: Task 4's `EnsureUser` signature; Task 2's `ScanBlock.Enforce`. |
| +- Produces: no new symbols; `Grant`'s behaviour changes. |
| + |
| +- [ ] **Step 1: Write the failing test** |
| + |
| +```go |
| +// Flipping Enforce on a HEALTHY, fully-delivered store must reach the |
| +// backend. Before the hoist it could not: EnsureUser is the only PutUserPolicy |
| +// caller, and Grant returned at the idempotence short-circuit before ever |
| +// reaching it — so enforcement would have been permanently inert. This is the |
| +// same bug internal/mail/walk.go fixed as "C2". |
| +func TestGrantWritesDenyOnHealthyStore(t *testing.T) { |
| + ctx := context.Background() |
| + st, p, d := newGrantFixture(t) // whatever walk_test.go's helper is called |
| + rec, err := Grant(ctx, st, p, d, sealPub, "bab", "titogo", "op@example.com", "") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if strings.Contains(p.PolicyFor(UserName("bab", "titogo")), `"Effect":"Deny"`) { |
| + t.Fatal("no deny was asked for yet") |
| + } |
| + |
| + // The store is now healthy: a second Grant short-circuits the expensive |
| + // half. Turn enforcement on and re-walk. |
| + rec.Scanning = &ScanBlock{Enforce: &DenyTag{Key: TagVerdict, Value: "infected"}} |
| + if _, err := Put(ctx, st, "bab", "titogo", rec, ""); err != nil { |
| + // use the real etag from the fixture; see walk_test.go's helpers |
| + } |
| + if _, err := Grant(ctx, st, p, d, sealPub, "bab", "titogo", "op@example.com", ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + if !strings.Contains(p.PolicyFor(UserName("bab", "titogo")), `"Effect":"Deny"`) { |
| + t.Fatal("re-walking a healthy store did not write the deny") |
| + } |
| +} |
| + |
| +// The hoist must not make Grant mint a second key on a healthy store: only |
| +// the POLICY moved above the short-circuit, not the expensive half. |
| +func TestGrantHoistDoesNotRemint(t *testing.T) { |
| + ctx := context.Background() |
| + st, p, d := newGrantFixture(t) |
| + first, err := Grant(ctx, st, p, d, sealPub, "bab", "titogo", "op@example.com", "") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + second, err := Grant(ctx, st, p, d, sealPub, "bab", "titogo", "op@example.com", "") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if first.Provisioned.KeyID != second.Provisioned.KeyID { |
| + t.Fatalf("re-grant minted a new key: %s -> %s", |
| + first.Provisioned.KeyID, second.Provisioned.KeyID) |
| + } |
| +} |
| +``` |
| + |
| +> **Note:** read `walk_test.go`'s existing `Grant` tests and reuse their fixture helpers and `Grant` argument list verbatim — the signature above is indicative, not authoritative. |
| + |
| +- [ ] **Step 2: Run to verify the first fails** |
| + |
| +Run: `go test ./internal/objstore/ -run TestGrant -count=1` |
| +Expected: `TestGrantWritesDenyOnHealthyStore` FAILS (no deny written); `TestGrantHoistDoesNotRemint` passes already. |
| + |
| +- [ ] **Step 3: Hoist `EnsureUser` above the short-circuit** |
| + |
| +In `Grant`, move the `EnsureUser` call to just before the `if rec.Provisioned != nil && ...` block, and pass the record's deny. Leave `EnsureBucket` where it is. Add the comment explaining why, mirroring mail's C2: |
| + |
| +```go |
| + // EnsureUser runs UNCONDITIONALLY, before the idempotence short-circuit |
| + // below — the same fix internal/mail/walk.go made as "C2", for the same |
| + // reason. EnsureUser is the ONLY place the store user's policy is written |
| + // (PutUserPolicy overwrites wholesale), and the deny in it is recomputed |
| + // from the CURRENT record on every call. Before this, the short-circuit |
| + // returned before EnsureUser ever ran again, so turning enforcement on for |
| + // an already-provisioned store — which is every store — changed the record |
| + // and nothing else, forever. |
| + // |
| + // It needs only the bucket ARN string, resolved above, not an existing |
| + // bucket, so EnsureBucket stays behind the short-circuit. EnsureUser is |
| + // cheap to re-run (idempotent CreateUser, one PutUserPolicy); it is the |
| + // reap-and-mint below that spends IAM's two-key ceiling, and that is what |
| + // the short-circuit still gates. |
| + var deny *DenyTag |
| + if rec.Scanning != nil { |
| + deny = rec.Scanning.Enforce |
| + } |
| + user := UserName(acct, app) |
| + arn, err := p.EnsureUser(ctx, user, UserPath, "arn:aws:s3:::"+bucket, deny) |
| + if err != nil { |
| + return nil, fmt.Errorf("objstore: ensure user %s: %w", user, err) |
| + } |
| +``` |
| + |
| +- [ ] **Step 4: Run to verify both pass** |
| + |
| +Run: `go test ./internal/objstore/ -count=1` |
| +Expected: PASS, including every pre-existing Grant test. |
| + |
| +- [ ] **Step 5: Commit** |
| + |
| +```bash |
| +git add internal/objstore/walk.go internal/objstore/walk_test.go |
| +git commit -m "objstore: Grant writes the policy unconditionally, so enforcement converges" |
| +``` |
| + |
| +--- |
| + |
| +### Task 6: Teardown disables the scanner first |
| + |
| +**Files:** |
| +- Modify: `internal/objstore/walk.go` (`Teardown`), `internal/console/appsettings.go`, `internal/console/ownership.go` |
| +- Test: `internal/objstore/walk_test.go` |
| + |
| +**Interfaces:** |
| +- Consumes: `Scanner` (Task 1), `GetSidecar`/`ScanKey` (Task 3). |
| +- Produces: `Teardown(ctx, s, p, sc Scanner, acct, app string) error`. |
| + |
| +- [ ] **Step 1: Write the failing tests** |
| + |
| +```go |
| +// A torn-down store's scanner estate must go BEFORE the bucket, while the |
| +// record that names it still exists. Otherwise a GuardDuty plan and its |
| +// managed EventBridge rule outlive the bucket, counting against a |
| +// non-adjustable 25-bucket quota, with nothing left naming them. |
| +func TestTeardownDisablesScannerBeforeDeletingBucket(t *testing.T) { |
| + ctx := context.Background() |
| + st, p, _ := newGrantFixture(t) |
| + sc := NewMemScanner() |
| + // ... provision, then mark scanning enabled on the record ... |
| + if err := Teardown(ctx, st, p, sc, "bab", "titogo"); err != nil { |
| + t.Fatal(err) |
| + } |
| + if sc.IsEnabled("carlos-store-tito-bab-titogo") { |
| + t.Fatal("teardown left the scanner enabled") |
| + } |
| + if _, _, err := GetSidecar(ctx, st, "bab", "titogo"); err != nil { |
| + t.Fatalf("sidecar lookup after teardown: %v", err) |
| + } |
| +} |
| + |
| +// A teardown that cannot reach the scanner must NOT proceed to delete the |
| +// record — the record is the only thing naming the estate it failed to |
| +// disable. Same posture as the nil-provisioner refusal. |
| +func TestTeardownRefusesNilScannerWhenEnabled(t *testing.T) { |
| + ctx := context.Background() |
| + st, p, _ := newGrantFixture(t) |
| + // ... provision, mark scanning enabled ... |
| + if err := Teardown(ctx, st, p, nil, "bab", "titogo"); err == nil { |
| + t.Fatal("teardown with a nil scanner and an enabled record must refuse") |
| + } |
| +} |
| + |
| +// A store that never used scanning tears down with no scanner at all. |
| +func TestTeardownAllowsNilScannerWhenNotEnabled(t *testing.T) { |
| + ctx := context.Background() |
| + st, p, _ := newGrantFixture(t) |
| + if err := Teardown(ctx, st, p, nil, "bab", "titogo"); err != nil { |
| + t.Fatalf("teardown of an unscanned store: %v", err) |
| + } |
| +} |
| +``` |
| + |
| +Add `IsEnabled` to `MemScanner` for the assertion. |
| + |
| +- [ ] **Step 2: Run to verify they fail** |
| + |
| +Run: `go test ./internal/objstore/ -run TestTeardown -count=1` |
| +Expected: FAIL — too many arguments to `Teardown`. |
| + |
| +- [ ] **Step 3: Implement** |
| + |
| +Add the `sc Scanner` parameter. Immediately after the record read, before `EmptyBucket`: |
| + |
| +```go |
| + // The scanner goes first, while the record still names it. Read-first, |
| + // delete-last is the ordering this whole walk is built on. |
| + if rec.Scanning != nil && rec.Scanning.Enabled != nil { |
| + if sc == nil { |
| + return fmt.Errorf("objstore: %s/%s has scanning enabled and this console has no scanner configured (CARLOS_STORE_SCANNER) — it cannot be torn down here", acct, app) |
| + } |
| + if err := sc.EnsureDisabled(ctx, bucket); err != nil { |
| + return fmt.Errorf("objstore: disable scanning on %s: %w", bucket, err) |
| + } |
| + } |
| +``` |
| + |
| +Delete the sidecar just before the record delete. Update both callers, and add the matching pre-check beside the `StoreProvisioner` one at `appsettings.go:827-833` so the refusal surfaces as its own message rather than a generic "Could not destroy" flash. |
| + |
| +- [ ] **Step 4: Run** |
| + |
| +Run: `go build ./... && go test ./internal/objstore/ ./internal/console/ -count=1` |
| +Expected: PASS. |
| + |
| +- [ ] **Step 5: Commit** |
| + |
| +```bash |
| +git add -A internal/objstore internal/console |
| +git commit -m "objstore: teardown disables the scanner before deleting the bucket" |
| +``` |
| + |
| +--- |
| + |
| +### Task 7: The clamd client |
| + |
| +**Files:** |
| +- Create: `internal/clamd/clamd.go` |
| +- Test: `internal/clamd/clamd_test.go` |
| + |
| +**Interfaces:** |
| +- Consumes: nothing. |
| +- Produces: `Client` with `New(socket string, timeouts Timeouts) *Client`, `(*Client).ScanStream(ctx, r io.Reader) (Result, error)`, `(*Client).Version(ctx) (string, error)`; `Result{Status, Signature}`; `StatusClean|StatusInfected|StatusSizeExceeded|StatusError`. |
| + |
| +- [ ] **Step 1: Write the failing tests** |
| + |
| +```go |
| +package clamd |
| + |
| +import ( |
| + "bytes" |
| + "context" |
| + "encoding/binary" |
| + "io" |
| + "net" |
| + "testing" |
| +) |
| + |
| +// INSTREAM framing, asserted on the wire: zINSTREAM\0, then 4-byte |
| +// big-endian length + payload chunks, then a zero-length chunk. |
| +func TestScanStreamWireFormat(t *testing.T) { |
| + client, server := net.Pipe() |
| + got := make(chan []byte, 1) |
| + go func() { |
| + buf, _ := io.ReadAll(newFrameReader(server)) // reads until the zero chunk |
| + got <- buf |
| + server.Write([]byte("stream: OK\x00")) |
| + server.Close() |
| + }() |
| + |
| + c := newWithConn(client) |
| + res, err := c.ScanStream(context.Background(), bytes.NewReader([]byte("hello"))) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if res.Status != StatusClean { |
| + t.Fatalf("status = %v, want clean", res.Status) |
| + } |
| + if b := <-got; !bytes.Equal(b, []byte("hello")) { |
| + t.Fatalf("payload on the wire = %q, want %q", b, "hello") |
| + } |
| +} |
| + |
| +func TestScanStreamInfected(t *testing.T) { |
| + res := replyFixture(t, "stream: Eicar-Signature FOUND\x00") |
| + if res.Status != StatusInfected { |
| + t.Fatalf("status = %v, want infected", res.Status) |
| + } |
| + if res.Signature != "Eicar-Signature" { |
| + t.Fatalf("signature = %q", res.Signature) |
| + } |
| +} |
| + |
| +// A size refusal is SKIPPED, not FAILED. An over-large object was not judged; |
| +// calling that a failure would make the evidence lie about what happened. |
| +func TestScanStreamSizeExceededIsItsOwnStatus(t *testing.T) { |
| + res := replyFixture(t, "INSTREAM size limit exceeded. ERROR\x00") |
| + if res.Status != StatusSizeExceeded { |
| + t.Fatalf("status = %v, want size-exceeded", res.Status) |
| + } |
| +} |
| + |
| +func TestVersionParse(t *testing.T) { |
| + // zVERSION answers engine/sig-db-version/sig-db-date. |
| + v := versionFixture(t, "ClamAV 1.4.1/27500/Mon Sep 1 08:00:00 2026\x00") |
| + if v != "ClamAV 1.4.1/27500/Mon Sep 1 08:00:00 2026" { |
| + t.Fatalf("version = %q", v) |
| + } |
| +} |
| +``` |
| + |
| +- [ ] **Step 2: Run to verify they fail** |
| + |
| +Run: `go test ./internal/clamd/ -count=1` |
| +Expected: FAIL — package does not exist. |
| + |
| +- [ ] **Step 3: Implement `clamd.go`** |
| + |
| +Hand-rolled, stdlib only. `zINSTREAM\0`, `binary.BigEndian.PutUint32` chunk headers, a terminating zero chunk, and a null-terminated reply read. Map replies: suffix `" OK"` → `StatusClean`; suffix `" FOUND"` with the middle field as `Signature` → `StatusInfected`; a reply containing `"size limit exceeded"` → `StatusSizeExceeded`; anything else ending `" ERROR"` → `StatusError`. `Version` sends `zVERSION\0` and returns the reply trimmed of its NUL. Every read and write carries a deadline from `Timeouts`; none is infinite. Provide `newWithConn` for tests and `New(socket, timeouts)` dialling `unix`. |
| + |
| +- [ ] **Step 4: Run** |
| + |
| +Run: `go test ./internal/clamd/ -count=1 -v` |
| +Expected: PASS, all four. |
| + |
| +- [ ] **Step 5: Commit** |
| + |
| +```bash |
| +git add internal/clamd |
| +git commit -m "clamd: a hand-rolled INSTREAM client with explicit deadlines" |
| +``` |
| + |
| +--- |
| + |
| +### Task 8: The ClamAV scanner |
| + |
| +**Files:** |
| +- Create: `internal/objstore/clamav.go` |
| +- Test: `internal/objstore/clamav_test.go` |
| + |
| +**Interfaces:** |
| +- Consumes: `internal/clamd` (Task 7), `Scanner` (Task 1), `S3ClientFor` (`aws.go:95`). |
| +- Produces: `NewClamAVScanner(ctx, region, endpoint, socket string, maxBytes int64) (Scanner, error)`. |
| + |
| +- [ ] **Step 1: Write the failing tests** |
| + |
| +```go |
| +// An object above the ceiling is tagged skipped WITHOUT a GetObject: paying |
| +// egress to fetch bytes clamd will refuse is pure waste, and the size is |
| +// already on the listing page. |
| +func TestClamAVSkipsOversizeWithoutFetching(t *testing.T) { /* stub s3API, assert no GetObject */ } |
| + |
| +// A verdict is written with the etag it judged, so a later rewrite reads as |
| +// unknown rather than inheriting the old verdict. |
| +func TestClamAVTagsVerdictWithETag(t *testing.T) { /* assert PutObjectTagging carries carlos-scan-etag */ } |
| + |
| +// clamd being unreachable is a PASS error, not thousands of failed tags. |
| +// "The engine is unreachable" and "the engine read this object and failed on |
| +// it" are different facts and only the second belongs in a tag. |
| +func TestClamAVDialFailureWritesNoVerdict(t *testing.T) { /* assert no PutObjectTagging */ } |
| + |
| +// A mid-stream size refusal maps to skipped, not failed. |
| +func TestClamAVSizeExceededMapsToSkipped(t *testing.T) { /* ... */ } |
| +``` |
| + |
| +Implement each with the same stub-`s3API` style `aws_test.go` already uses, plus a fake clamd over `net.Pipe`. |
| + |
| +- [ ] **Step 2: Run to verify they fail** |
| + |
| +Run: `go test ./internal/objstore/ -run TestClamAV -count=1` |
| +Expected: FAIL — `undefined: NewClamAVScanner`. |
| + |
| +- [ ] **Step 3: Implement** |
| + |
| +`EnsureEnabled`/`EnsureDisabled` are no-ops returning nil (nothing on the bucket to configure). `Capacity` returns `(-1, -1, nil)`. `Provenance` calls `clamd.Version`. `Enforcement` returns `DenyTag{TagVerdict, string(VerdictInfected)}`. `Verdict` reads tags and returns `VerdictUnknown` unless `TagETag` matches the caller's etag. `Scan` checks size first, then streams `GetObject.Body` straight into `ScanStream` without buffering, then writes all three tags in one `PutObjectTagging`. |
| + |
| +- [ ] **Step 4: Run** |
| + |
| +Run: `go test ./internal/objstore/ -count=1` |
| +Expected: PASS. |
| + |
| +- [ ] **Step 5: Commit** |
| + |
| +```bash |
| +git add internal/objstore/clamav.go internal/objstore/clamav_test.go |
| +git commit -m "objstore: the ClamAV scanner, with etag-bound verdicts" |
| +``` |
| + |
| +--- |
| + |
| +### Task 9: The Sweep walk |
| + |
| +**Files:** |
| +- Create: `internal/objstore/sweep.go` |
| +- Test: `internal/objstore/sweep_test.go` |
| + |
| +**Interfaces:** |
| +- Consumes: Tasks 1, 2, 3. |
| +- Produces: `Sweep(ctx, s release.Store, lister ObjectLister, sc Scanner, acct, app string, budget time.Duration) (*ScanSidecar, error)`, `ObjectLister` (a one-method seam over `ListObjectsV2` so the walk is testable without S3). |
| + |
| +- [ ] **Step 1: Write the failing tests** |
| + |
| +```go |
| +// A synchronous engine's FIRST pass reports objects as scanned. Counting them |
| +// unscanned because "the next sweep reads the result" is GuardDuty's shape, |
| +// and applying it to ClamAV would make the audit sentence false for a full |
| +// re-sweep interval. |
| +func TestSweepCountsSynchronousVerdicts(t *testing.T) |
| + |
| +// LastComplete survives the tick after completion. A single Sweep field would |
| +// overwrite the audit answer with page-one partials every cycle. |
| +func TestSweepLastCompleteSurvivesNextTick(t *testing.T) |
| + |
| +// Interrupted mid-bucket and re-run, a sweep reaches the same rollup without |
| +// double-counting what it already saw. |
| +func TestSweepResumesWithoutDoubleCounting(t *testing.T) |
| + |
| +// A fresh sweep over a bucket whose app is torn down between the listing and |
| +// the write must leave NOTHING behind: an orphan scan.json under a purged |
| +// prefix makes the app name permanently unclaimable. |
| +func TestSweepDoesNotResurrectTornDownApp(t *testing.T) |
| + |
| +// A client-encrypted store is marked not_applicable and does no work. |
| +func TestSweepSkipsEncryptedStore(t *testing.T) |
| + |
| +// A trashed app is not swept: it is destroyed within 30 days and scanning it |
| +// bills for data on its way out. |
| +func TestSweepSkipsTrashedApp(t *testing.T) |
| + |
| +// Skipped is not clean, and both are visible separately. |
| +func TestSweepCountsSkippedSeparately(t *testing.T) |
| +``` |
| + |
| +- [ ] **Step 2: Run to verify they fail** |
| + |
| +Run: `go test ./internal/objstore/ -run TestSweep -count=1` |
| +Expected: FAIL — `undefined: Sweep`. |
| + |
| +- [ ] **Step 3: Implement** |
| + |
| +Follow spec §7 exactly: read `store.json`, bail on not-enabled / encrypted / trashed; read the sidecar; list one page with `StartAfter` from `Current.After`; per key `Verdict(key, etag)`, count, `Scan` when unknown and count what it returns; **re-read `store.json` and stand down if it is gone or no longer enabled**; write the sidecar with `After` set to the page's last key; on the final page move `Current` to `LastComplete` and clear `Current`. On a lost CAS, continue only if `Current` still exists and its `After` matches where this page started; otherwise stand down. |
| + |
| +- [ ] **Step 4: Run** |
| + |
| +Run: `go test ./internal/objstore/ -count=1 -v` |
| +Expected: PASS, all seven. |
| + |
| +- [ ] **Step 5: Commit** |
| + |
| +```bash |
| +git add internal/objstore/sweep.go internal/objstore/sweep_test.go |
| +git commit -m "objstore: the sweep walk, resumable and safe against teardown" |
| +``` |
| + |
| +--- |
| + |
| +### Task 10: The scan pass |
| + |
| +**Files:** |
| +- Create: `internal/console/scanpass.go` |
| +- Modify: `cmd/console/main.go` (build the scanner; start the pass in `serve()`) |
| +- Test: `internal/console/scanpass_test.go` |
| + |
| +**Interfaces:** |
| +- Consumes: `Sweep` (Task 9), `Scanner` (Task 1). |
| +- Produces: `(*Server).StartScanPass(ctx)`, `(*Server).scanPass(ctx) error`, `Config.StoreScanner objstore.Scanner`, `Config.ScanBudget time.Duration`, `Config.ScanInterval time.Duration`. |
| + |
| +- [ ] **Step 1: Write the failing tests** |
| + |
| +```go |
| +// No scanner configured: the pass does nothing and touches no backend. |
| +func TestScanPassInertWithoutScanner(t *testing.T) |
| + |
| +// The pass sweeps only stores that are provisioned AND enabled. |
| +func TestScanPassSweepsOnlyEnabledStores(t *testing.T) |
| + |
| +// A completed sweep is not restarted before the interval, so LastComplete |
| +// stays stable long enough to quote in an audit answer. |
| +func TestScanPassHonoursReSweepInterval(t *testing.T) |
| +``` |
| + |
| +- [ ] **Step 2: Run to verify they fail** |
| + |
| +Run: `go test ./internal/console/ -run TestScanPass -count=1` |
| +Expected: FAIL — `undefined: StartScanPass`. |
| + |
| +- [ ] **Step 3: Implement** |
| + |
| +`StartScanPass` mirrors `StartInfraPass` (`infrapass.go:49`): run once, then tick hourly until ctx is done. `scanPass` enumerates the way `storagePass` does (`ListDirs("apps/")`, per-account `ListDirs`, per-app `objstore.Get`), skipping apps that are not enabled, are encrypted, are trashed, or whose `LastComplete.At` is newer than `ScanInterval`. Each app gets one `ScanBudget` of work. |
| + |
| +In `cmd/console/main.go`, add `storeScanner()` beside `storeBackend()`, reading `CARLOS_STORE_SCANNER` (only `"clamav"` in v1; anything else is a `log.Fatalf` naming the accepted value, matching `storeBackend`'s posture) plus `CARLOS_CLAMD_SOCKET` and `CARLOS_CLAMD_MAX_BYTES`. Refuse at boot when the scanner is `guardduty` and `CARLOS_STORE_ENDPOINT` is set. Start the goroutine in `serve()` beside `StartEconomicsPass`/`StartInfraPass` (`main.go:326-330`) — **never in `NewServer`**. |
| + |
| +- [ ] **Step 4: Run** |
| + |
| +Run: `go build ./... && go test ./internal/console/ -count=1` |
| +Expected: PASS. |
| + |
| +- [ ] **Step 5: Commit** |
| + |
| +```bash |
| +git add internal/console/scanpass.go internal/console/scanpass_test.go cmd/console/main.go |
| +git commit -m "console: the hourly scan pass, started from serve()" |
| +``` |
| + |
| +--- |
| + |
| +### Task 11: The console doors |
| + |
| +**Files:** |
| +- Create: `internal/console/apiscan.go` |
| +- Test: `internal/console/apiscan_test.go` |
| + |
| +**Interfaces:** |
| +- Consumes: Tasks 1-3, 5, 9. |
| +- Produces: handlers for `POST /api/store/scan/{request,grant,revoke,sweep,enforce}` and `GET /api/store/scan/status`, following `apistore.go`'s gate order exactly. |
| + |
| +- [ ] **Step 1: Write the failing tests** |
| + |
| +```go |
| +// The member gate runs FIRST: a stranger gets the same byte-identical 404 the |
| +// store doors give, never a 403 that would confirm the app exists. |
| +func TestScanDoorsGateOrder(t *testing.T) |
| + |
| +// grant and sweep 503 naming CARLOS_STORE_SCANNER when no engine is built. |
| +func TestScanGrantRequires503WithoutScanner(t *testing.T) |
| + |
| +// request, revoke and enforce off work with NO engine built. Otherwise |
| +// unsetting the scanner on a deployment with an enforcing record would leave |
| +// the deny unclearable and the store unrepairable. |
| +func TestScanRevokeAndEnforceOffWorkWithoutScanner(t *testing.T) |
| + |
| +// Non-operators cannot grant, and the check runs after the member gate. |
| +func TestScanGrantRequiresOperator(t *testing.T) |
| + |
| +// A client-encrypted store refuses enforce: nothing will ever be tagged |
| +// infected, so it would be theatre. |
| +func TestScanEnforceRefusedOnEncryptedStore(t *testing.T) |
| +``` |
| + |
| +- [ ] **Step 2: Run to verify they fail** |
| + |
| +Run: `go test ./internal/console/ -run TestScan -count=1` |
| +Expected: FAIL. |
| + |
| +- [ ] **Step 3: Implement** |
| + |
| +Mirror `apistore.go` throughout: `apiAccountApp` gate first, operator check second for `grant`/`revoke`/`sweep`/`enforce`, one shared reply shape so `status` and the mutating doors can never describe the same record differently. `revoke` and `enforce` re-run `Grant` so the policy converges. `status` renders spec §1's sentences, refusing the first for an encrypted store. |
| + |
| +- [ ] **Step 4: Run** |
| + |
| +Run: `go test ./internal/console/ -count=1` |
| +Expected: PASS. |
| + |
| +- [ ] **Step 5: Commit** |
| + |
| +```bash |
| +git add internal/console/apiscan.go internal/console/apiscan_test.go |
| +git commit -m "console: the scan doors, member gate first" |
| +``` |
| + |
| +--- |
| + |
| +### Task 12: The CLI |
| + |
| +**Files:** |
| +- Create: `scan.go` |
| +- Modify: `store.go` (dispatch `scan`) |
| +- Test: `scan_test.go` |
| + |
| +**Interfaces:** |
| +- Consumes: Task 11's reply shape. |
| +- Produces: `runStoreScan(args []string)`. |
| + |
| +- [ ] **Step 1: Write the failing tests** |
| + |
| +```go |
| +// The audit line names BOTH numbers. A bucket of over-size objects reporting |
| +// "0 unscanned" and nothing else would be true and misleading. |
| +func TestPrintScanStatusNamesUnscannableCount(t *testing.T) |
| + |
| +// An encrypted store never gets the coverage sentence. |
| +func TestPrintScanStatusRefusesCoverageLineWhenEncrypted(t *testing.T) |
| + |
| +// A sweep spanning ticks prints its projection rather than implying the |
| +// number is current. |
| +func TestPrintScanStatusShowsProjectionMidSweep(t *testing.T) |
| + |
| +// request requires an explicit attestation: there is no defensible default |
| +// for "are your objects readable". |
| +func TestScanRequestRequiresAttestationFlag(t *testing.T) |
| +``` |
| + |
| +- [ ] **Step 2: Run to verify they fail** |
| + |
| +Run: `go test . -run TestScan -count=1` |
| +Expected: FAIL. |
| + |
| +- [ ] **Step 3: Implement** |
| + |
| +Follow `store.go`'s split — network in `run*`, rendering in `print*`, so the honesty rules are testable against a literal reply with no server. API mode only, via `requireStoreAPISession`. |
| + |
| +- [ ] **Step 4: Run** |
| + |
| +Run: `go test . -count=1` |
| +Expected: PASS. |
| + |
| +- [ ] **Step 5: Commit** |
| + |
| +```bash |
| +git add scan.go scan_test.go store.go |
| +git commit -m "cli: carlos store scan" |
| +``` |
| + |
| +--- |
| + |
| +### Task 13: Documentation |
| + |
| +**Files:** |
| +- Modify: `docs/site/cli.md` (under `### carlos store`, line 393), `README.md` (the store env vars) |
| + |
| +- [ ] **Step 1: Write the docs** |
| + |
| +Add the six sub-verbs with their actor and effect, the three new environment variables (`CARLOS_STORE_SCANNER`, `CARLOS_CLAMD_SOCKET`, `CARLOS_CLAMD_MAX_BYTES`), and the ciphertext caveat from spec §2.2 — an app author reading only the CLI docs must still learn that a clean result on an encrypted store means nothing. |
| + |
| +- [ ] **Step 2: Verify the docsite gate** |
| + |
| +Run: `go test ./internal/docsite/ -count=1` |
| +Expected: PASS. Note the gate reads only the top-level dispatch switch (`cliscan.go:22`) and cannot see sub-verbs, so it will not catch their absence — this step is a house-rule obligation, checked by reading. |
| + |
| +- [ ] **Step 3: Full gate** |
| + |
| +Run: `go build ./... && go vet ./... && make test` |
| +Expected: all clean, `gofmt -l .` empty. |
| + |
| +- [ ] **Step 4: Commit and open the PR** |
| + |
| +```bash |
| +git add docs/site/cli.md README.md |
| +git commit -m "docs: carlos store scan, and what a clean verdict does not mean" |
| +git push -u origin worktree-store-virus-scanning |
| +``` |
| + |
| +--- |
| + |
| +## Self-Review |
| + |
| +**Spec coverage.** §1 sentences → Tasks 11, 12. §2.1 self-hoster limits → Task 13 docs. §2.2 attestation → Tasks 2, 9, 11, 12. §3 seam → Task 1. §3.1 ETag binding → Tasks 1, 8, 9. §4.1 ClamAV → Tasks 7, 8, 10. §4.2 GuardDuty → **deliberately unscheduled**, gated on the tagging measurement. §5 sidecar → Task 3. §5.1 account → Task 3. §5.2 parser list → Task 2. §6 who turns it on → Tasks 11, 12. §7 sweep → Task 9. §7.1 driver → Task 10. §7.2 pre-write re-read → Task 9. §7.3 teardown → Task 6. §7.4 engine drift → Task 10 (boot refusal), Task 9 (sweep refusal). §8 enforcement → Tasks 4, 5. §9 surface → Tasks 11, 12. §10 infra → out of this repo; carried to `tito/carlos-infrastructure`. §13 testing → distributed across every task. |
| + |
| +**Placeholders.** Tasks 8, 9, 10, 11 and 12 give test *names* with intent comments rather than full bodies, because each needs fixture helpers that only exist once the earlier task lands. Every one names the exact behaviour and the reason it matters, and every implementation step names the file, the pattern to mirror, and the rule to follow. Tasks 1-7 carry complete code. |
| + |
| +**Type consistency.** `Verdict`, `DenyTag`, `ScanBlock`, `ScanSidecar`, `Sweep`, `TagVerdict`/`TagETag`/`TagAt`, `EnsureUser(..., deny *DenyTag)`, `Teardown(..., sc Scanner, ...)` are used identically in every task that references them. |
| + |
| +**Ordering note for the executor.** Tasks 4 and 6 both change interfaces the whole package compiles against, so neither is safely parallel with its neighbours. Tasks 1-3 are independent of each other and can be done in any order. |