amadan
spec: virus scanning for the object storage primitive
Tony asked whether we can scan objects for viruses; the answer to "whose problem does that solve" was a compliance checkbox, where coverage and evidence beat detection latency. That ruling picks the design: a reconciling sweep that can count objects lacking a verdict, rather than a scan-on-upload pipeline whose misses are invisible. objstore never touches the data plane, so there is no PUT to hook. The lever that survives is the IAM policy CARLOS already writes for each app, which can deny reads on an infected object without the control plane reading a byte. Draft 2. Draft 1 was reviewed and six blockers found: the deny needed no migration (false - EnsureUser is the only policy writer and Grant short-circuits before it), GuardDuty covers a bucket's existing objects (false - it scans new uploads only; SendObjectMalwareScan is the on-demand path), GuardDuty caps at 25 protected buckets per account per region and we mint one per app, Teardown orphaned the plan, the record block used omitempty on a time.Time, and the sweep had no driver. All corrected, and the compliance claim weakened to what is true: as of the last completed sweep, N objects carried no verdict.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E1vDqqMC44bNatFmt6F2Ce
1 file changed,
+548
−0
docs/superpowers/specs/2026-09-01-object-storage-virus-scanning-design.md+548 −0
diff --git a/docs/superpowers/specs/2026-09-01-object-storage-virus-scanning-design.md b/docs/superpowers/specs/2026-09-01-object-storage-virus-scanning-design.md| new file mode 100644 |
| index 000000000..6cd33980f |
| --- /dev/null |
| +++ b/docs/superpowers/specs/2026-09-01-object-storage-virus-scanning-design.md |
| @@ -0,0 +1,548 @@ |
| +# Virus scanning for the object storage primitive |
| + |
| +**Date:** 2026-09-01 |
| +**Status:** approved in principle (Paul, 2026-09-01). Tony raised the |
| +question; Paul chose the shape from four options and accepted opt-in, |
| +default-off. Items marked **(ruling)** were decided in conversation and are |
| +not cheap to overturn; items marked **(decision)** are this spec's own |
| +reasoning and can be argued with. |
| + |
| +**Revision:** draft 2. Draft 1 was reviewed and six blockers were found; every |
| +one is corrected below. Draft 1 claimed the deny statement needed no migration |
| +(false — §8), that GuardDuty covers a bucket's existing objects (false — §4), |
| +and it never named the sweep's driver (§7) or GuardDuty's bucket cap (§2.1). |
| +The compliance claim in §1 has been weakened to what is actually true. |
| + |
| +## 1. What is actually being asked |
| + |
| +Tony asked whether CARLOS can scan objects for viruses. Asked whose problem |
| +that solves, the answer was: **a compliance checkbox** — someone (a customer, |
| +an auditor, a procurement form) wants to know whether stored files are |
| +scanned, and the deployment needs a defensible yes. **(ruling)** |
| + |
| +That answer, not the phrase "virus scanning", determines the design. Coverage |
| +and evidence matter; detection latency and enforcement strength do not. The |
| +question a deployment must answer is not "how fast did we catch it" but |
| +**"how many objects lack a verdict?"**, and the answer has to be a number |
| +somebody can check. |
| + |
| +**What this design can honestly claim, stated precisely.** Not "every object |
| +was scanned" — that is a proof no design here can deliver. What it delivers |
| +is: *as of the last completed sweep at time T, N objects in this bucket |
| +carried no verdict.* That is a point-in-time reconciliation, not a |
| +continuous guarantee, and it is worth more than a scan-on-upload pipeline can |
| +offer, because a pipeline's misses are invisible while a reconciler's are |
| +counted. Anyone writing the audit response should use that sentence and not a |
| +stronger one. |
| + |
| +## 2. The constraint that rules out the obvious hook |
| + |
| +`internal/objstore` is a control plane and says so in its own package |
| +comment: |
| + |
| +> It never touches the data plane — apps talk to their bucket directly with |
| +> the delivered credentials. |
| + |
| +The engine mints a bucket, creates one IAM user per app whose inline policy is |
| +exactly two statements (`s3:ListBucket` on the bucket; `s3:GetObject`, |
| +`s3:PutObject`, `s3:DeleteObject` under it, no conditions — |
| +`internal/objstore/aws.go:129-130`), seals the secret into the app's config, |
| +and steps out. Every verb is an idempotent, re-runnable walk over bucket |
| +state (`Grant`, `Rotate`, `RotateFinish`, `Teardown` in `walk.go`). |
| + |
| +**CARLOS never sees the PUT.** There is no request path to hook, because there |
| +is no request path. Any design claiming to scan "on upload" is either buying |
| +that from the backend or inserting a new component into the data plane. |
| + |
| +The lever that survives: **CARLOS writes the app's IAM policy**, so the |
| +control plane can gate access to an object by its verdict without reading a |
| +byte. §8 is where that is cashed in, and §8 is also where draft 1 was wrong |
| +about how. |
| + |
| +### 2.1 GuardDuty protects at most 25 buckets per account per Region |
| + |
| +This is a hard architectural constraint and it was missed in draft 1. |
| + |
| +AWS's quota table gives **Maximum protected buckets: 25, per account in each |
| +Region, not adjustable.** CARLOS mints one bucket per app (`MintedBucket`, |
| +`walk.go:62`). So on a single-account deployment the GuardDuty engine covers |
| +**at most 25 apps per Region**, and the 26th `scan grant` fails. |
| + |
| +This does not sink the design, for one reason that follows directly from the |
| +ruling in §1: the feature is opt-in and default-off, and the population that |
| +opts in is the population with a compliance requirement — not every app on the |
| +deployment. Twenty-five is plausibly years of headroom for that population. |
| +But it must be surfaced rather than discovered: |
| + |
| +- `scan grant` counts the deployment's protected buckets before enabling, and |
| + refuses at the cap with the count and the number in the message, naming the |
| + quota. A refusal that says "25 of 25 buckets protected in eu-west-1; this is |
| + a non-adjustable AWS quota" is actionable; a raw AWS error is not. |
| +- `store scan status` for the deployment reports the count against the cap, so |
| + an operator sees the ceiling approaching rather than hitting it. |
| +- The long-term answer past 25 is the ClamAV engine (§4.2), which has no such |
| + cap. That is the honest reason the seam exists rather than a bare GuardDuty |
| + integration. **(decision)** |
| + |
| +## 3. Shape: a Scanner seam beside Provisioner |
| + |
| +`Provisioner` is the existing backend seam, and its doc comment already |
| +promises this move: |
| + |
| +> v1 ships the AWS implementation and this package's in-memory fake. A |
| +> Hetzner/R2/Minio provisioner arrives here later without touching a walk. |
| + |
| +Scanning gets the same treatment. **(decision)** |
| + |
| +```go |
| +// 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. |
| +type Scanner interface { |
| + // EnsureEnabled turns scanning on for a bucket. Engines that scan by |
| + // fetching have nothing to configure and answer nil. |
| + EnsureEnabled(ctx context.Context, bucket string) error |
| + // EnsureDisabled turns it off and releases whatever EnsureEnabled took. |
| + // Teardown calls this BEFORE deleting the bucket (§7.2). |
| + EnsureDisabled(ctx context.Context, bucket string) error |
| + // Verdict answers the recorded verdict for one object. Unscanned is |
| + // (VerdictUnknown, nil) — a state, not an error, the same idiom |
| + // objstore.Get uses for an absent record. |
| + Verdict(ctx context.Context, bucket, key string) (Verdict, error) |
| + // Scan causes one object to be scanned and its verdict recorded. It is |
| + // how the sweep closes a coverage gap, so every engine implements it for |
| + // real: the GuardDuty engine by asking for an on-demand scan, the ClamAV |
| + // engine by fetching and scanning. It may return before a verdict exists |
| + // (GuardDuty scans asynchronously); the next sweep reads the result. |
| + Scan(ctx context.Context, bucket, key string) error |
| + // Enforcement names the object tag this engine writes, so the IAM policy |
| + // writer can deny reads on it (§8). Engine-specific by necessity: the |
| + // key and value differ per engine. |
| + Enforcement() (tagKey, infectedValue string) |
| + // Capacity reports how many buckets this engine can still protect on |
| + // this deployment, and the cap. (-1, -1) means uncapped (§2.1). |
| + Capacity(ctx context.Context) (remaining, cap int, err error) |
| +} |
| + |
| +type Verdict string |
| + |
| +const ( |
| + VerdictUnknown Verdict = "" // no verdict recorded yet |
| + VerdictClean Verdict = "clean" |
| + VerdictInfected Verdict = "infected" |
| + VerdictSkipped Verdict = "skipped" // too large, unsupported, or unextractable |
| + VerdictFailed Verdict = "failed" // the engine tried and could not |
| +) |
| +``` |
| + |
| +`Verdict` hides that each engine reads its own tag key, so the sweep never |
| +branches on engine name. `Enforcement` is the one place that difference has to |
| +surface, because a Deny statement must name a literal tag key and value — |
| +draft 1 claimed the walk never branches on engine and then wrote an |
| +engine-agnostic Deny, which is not possible. Naming the seam is the fix. |
| + |
| +**Draft 1 had `Scan` return `ErrScannedByBackend` for the GuardDuty engine.** |
| +That sentinel is deleted. It encoded a belief that turned out to be false |
| +(§4.1) and it made the sweep's gap permanently uncloseable. |
| + |
| +## 4. The two engines |
| + |
| +### 4.1 `guarddutyScanner` (AWS) |
| + |
| +`EnsureEnabled` creates a Malware Protection plan on the bucket with object |
| +tagging on. `Verdict` reads GuardDuty's `GuardDutyMalwareScanStatus` tag. |
| +`EnsureDisabled` deletes the plan. |
| + |
| +**GuardDuty scans newly uploaded objects only.** Objects already in the bucket |
| +when the plan is created are never scanned by the plan, and would sit at |
| +`Unscanned` forever — which would mean the number in §1 never reaches zero on |
| +any bucket enabled after first use, i.e. every existing store. `Scan` |
| +therefore calls **`SendObjectMalwareScan`** for the key, which is the |
| +documented on-demand path for exactly this. That single call is what turns the |
| +sweep from a counter into a reconciler. |
| + |
| +Two consequences worth stating rather than discovering: |
| + |
| +- **Tagging must be on before the first upload for the steady-state path to |
| + work.** AWS is explicit: enable tagging after an object is uploaded and |
| + GuardDuty cannot tag that object. For an existing store this is what the |
| + on-demand path above is for; for a new store, `scan grant` should precede |
| + first use. |
| +- **GuardDuty is itself event-driven and at-least-once.** The argument in §1 |
| + against event pipelines applies to it too. The sweep is what covers its |
| + misses, which is the reason the sweep exists even on the engine that |
| + advertises automatic scanning. |
| + |
| +Priced per object evaluated plus per GB scanned ($0.09/GB in us-east-1 after |
| +the 2025-02 reduction), plus S3 object-tagging cost, billed to the |
| +deployment's own cloud account. Objects over 100 GB are not scanned. That |
| +price tag is load-bearing for §6. |
| + |
| +### 4.2 `clamavScanner` — specified, not shipped in v1 |
| + |
| +`EnsureEnabled`/`EnsureDisabled` are no-ops. `Scan` fetches the object, |
| +streams it to a `clamd` worker, and writes a `carlos-scan` tag. `Verdict` |
| +reads it back. `Capacity` is uncapped, which is what makes it the answer to |
| +§2.1. |
| + |
| +**It is out of v1 scope, because the backend it exists for does not exist |
| +yet.** `cmd/console/main.go:497` rejects every `CARLOS_STORE_BACKEND` but |
| +`"aws"` with a `log.Fatal`, and with `CARLOS_STORE_ENDPOINT` set the IAM half |
| +is still real AWS (`aws.go:73-75`) writing policies that name |
| +`arn:aws:s3:::<bucket>`, which a Ceph gateway ignores. Shipping a ClamAV |
| +engine before a non-AWS provisioner exists would be shipping a wing without an |
| +aeroplane. **(decision)** |
| + |
| +v1 ships the seam, the GuardDuty engine, and a `MemScanner` fake — which is |
| +precisely the "capability to build upon" that was asked for. The ClamAV engine |
| +lands with, or after, a Hetzner provisioner. |
| + |
| +When it does land, note the honest cost: on that engine CARLOS reads tenant |
| +data, giving up the property in §2. Operators should be told that in the |
| +documentation, not left to infer it. |
| + |
| +## 5. The record is the compliance artifact |
| + |
| +`objstore.Record` gains one additive block. Its shape mirrors the record's |
| +existing three-actor idiom — separate pointer blocks, each null-or-present — |
| +rather than draft 1's flat struct with `omitempty` timestamps, which does not |
| +work: `omitempty` has no effect on a `time.Time`, so a zero time serialises as |
| +`0001-01-01T00:00:00Z` and "not enabled" becomes indistinguishable from |
| +"enabled at the zero time" without an `IsZero()` check the JSON does not |
| +suggest. |
| + |
| +```go |
| +type ScanRequested struct { |
| + At time.Time `json:"at"` |
| + By string `json:"by"` // the member |
| +} |
| + |
| +type ScanEnabled struct { |
| + Engine string `json:"engine"` // "guardduty" | "clamav" |
| + At time.Time `json:"at"` |
| + By string `json:"by"` // the operator |
| +} |
| + |
| +type ScanBlock struct { |
| + Requested *ScanRequested `json:"requested,omitempty"` |
| + Enabled *ScanEnabled `json:"enabled,omitempty"` |
| + Sweep *Sweep `json:"sweep,omitempty"` |
| + Enforce bool `json:"enforce,omitempty"` // §8, default false |
| +} |
| + |
| +type Sweep struct { |
| + StartedAt time.Time `json:"started_at"` |
| + At time.Time `json:"at"` // last page written |
| + Objects int64 `json:"objects"` |
| + Scanned int64 `json:"scanned"` |
| + Infected int64 `json:"infected"` |
| + Skipped int64 `json:"skipped"` |
| + Failed int64 `json:"failed"` |
| + Unscanned int64 `json:"unscanned"` |
| + After string `json:"after,omitempty"` // resume key, "" = complete |
| +} |
| +``` |
| + |
| +Draft 1 reused `*Declared` for the member's request. It carries `EnvPrefix`, |
| +which is meaningless for scanning and would serialise as `""`; `ScanRequested` |
| +replaces it. |
| + |
| +`Sweep` is the deliverable. `carlos store scan status` renders it as one line: |
| + |
| +``` |
| +12,481 objects · 12,479 scanned · 2 skipped · 0 infected · 0 unscanned |
| +last complete sweep 2026-09-01T14:02Z |
| +``` |
| + |
| +A sweep still in progress says so and prints the previous complete one, because |
| +a partial count is not an audit answer. |
| + |
| +**A hazard this spec must not walk into.** `objstore.go` carries a PARSER LIST |
| +comment: `release.PurgeApp` parses `grant` and `provisioned` out of these bytes |
| +with its own local struct — release cannot import objstore, because objstore |
| +imports release — and refuses the box-side auto-purge when either block is |
| +present. Additive changes are safe; those two names are not free to move. A |
| +`scanning` key is additive and `trash.go:213-216` ignores unknown keys, so this |
| +is safe. `TestPurgeGuardSeesOurRecord` pins the two names but never writes a |
| +`scanning` key, so the additive claim is not currently asserted — §13 adds the |
| +subtest that asserts it. |
| + |
| +## 6. Who turns it on: the same three authors |
| + |
| +The store record has three blocks for three actors — the member declares, an |
| +operator grants, the console records what it provisioned. Scanning mirrors it. |
| +**(decision, accepted 2026-09-01)** |
| + |
| +- **The member requests.** `carlos store scan request` records the ask. It |
| + spends nothing and provisions nothing. |
| +- **An operator grants.** `carlos store scan grant` calls out to the cloud |
| + account and starts a per-GB meter. That is exactly the test `apistore.go` |
| + already applies: "Granting is the one verb that spends money in the |
| + DEPLOYMENT's own cloud account, so it needs an address on |
| + `CARLOS_STORE_OPERATORS` as well." Same gate, same ordering — **after** the |
| + member gate, so a 403 never confirms to a stranger which accounts and apps |
| + exist. |
| + |
| +Default off at two levels. `CARLOS_STORE_SCANNER` unset means no engine is |
| +built at boot, no sweep runs, and the **operator** door answers 503 naming the |
| +variable. And with an engine configured, an app is unscanned until requested |
| +and granted. |
| + |
| +**The member's request door does not 503 when the scanner is unset.** Draft 1 |
| +had it do so; that contradicts the precedent the store's own create door sets |
| +deliberately (`apistore.go:145-152`): a member cannot see whether their |
| +deployment has a backend, and refusing them would teach them nothing they can |
| +act on. The request is recorded either way and `scan status` reports |
| +`engine: none`, which is the same shape `store create` already uses for a |
| +deployment with no backend. |
| + |
| +## 7. The sweep walk |
| + |
| +`Sweep` joins `Grant`, `Rotate` and `Teardown` in `walk.go`, with their |
| +contract: check-then-act, re-runnable after a failure at any point. |
| + |
| +1. Read the record. No `scanning.enabled` — return without touching the |
| + backend. An ungranted app costs a sweep nothing. |
| +2. List the bucket page by page with `ListObjectsV2`, resuming with |
| + `StartAfter` set to `Sweep.After`. **Not a `ContinuationToken`**: draft 1 |
| + cited `EmptyBucket` as precedent for a persisted token, but `EmptyBucket` |
| + (`aws.go:200-221`) persists nothing — it restarts from the first page and is |
| + resumable only because deletes are idempotent. A continuation token's |
| + lifetime across processes is unspecified and, on Ceph, unmeasured. A key is |
| + a portable resume point. |
| +3. For each key, ask `Verdict` and count it into the right bucket. If unknown, |
| + call `Scan`, which asks GuardDuty for an on-demand scan (§4.1). The verdict |
| + arrives asynchronously; this sweep counts the object as unscanned and the |
| + next one counts it as scanned. That is the reconciliation loop closing. |
| +4. **Write the rollup after every page**, carrying `After`. Not once at the |
| + end: a crash between pages would otherwise lose the whole pass, and a |
| + resume that re-counted from a stale rollup would double-count. Counters |
| + reset when a sweep starts fresh (`After == ""`). |
| + |
| +**CAS discipline.** Draft 1 said a lost race is the caller's to surface. That |
| +is right for a single-shot verb and wrong for a multi-minute walk writing once |
| +per page against a record the create, grant and rotate doors also write — it |
| +would fail constantly. Sweep owns `scanning.sweep` outright and follows the |
| +pattern `Rotate` already uses (`walk.go:304-327`): on conflict, re-read, drop |
| +only its own block onto the fresh record, and retry once. Anything else is the |
| +caller's. |
| + |
| +**Honest semantics, stated because they will be asked about.** A key added |
| +lexically before the resume point during a sweep is missed by that sweep and |
| +caught by the next. A key deleted after being counted inflates `Objects` for |
| +that pass. Neither is a correctness bug in a point-in-time reconciliation, and |
| +§1's sentence is worded to survive both. |
| + |
| +### 7.1 The driver |
| + |
| +Draft 1 never said what runs `Sweep`, which left the single most operationally |
| +consequential question to the implementer. |
| + |
| +It runs as a scheduled pass beside `storagePass` (`economicspass.go:136`), |
| +which already wakes hourly and already iterates every provisioned store — so |
| +the iteration, the bucket resolution and the cadence all exist. The scan pass |
| +takes one page per app per tick, so a large bucket makes progress every hour |
| +without any single tick running long. |
| + |
| +It is **not** driven inline from a console door. `Verdict` is one |
| +`GetObjectTagging` per object; 12,481 objects is 12,481 round trips against a |
| +door bounded by `storeGrantTimeout` at 60s (`apistore.go:339`). The operator |
| +verb `scan sweep` starts a pass, or advances it by one page, and says which — |
| +it never blocks on a whole bucket. |
| + |
| +### 7.2 Teardown must disable before it deletes |
| + |
| +`Teardown` (`walk.go:390-496`) empties the bucket, deletes it, deletes the |
| +user, unsets the config and then deletes the record — which is the only place |
| +`scanning.enabled.engine` is written down. Nothing in draft 1 called |
| +`EnsureDisabled`, so a torn-down store would leave its Malware Protection plan |
| +and the managed EventBridge rule behind it alive, counting against the |
| +25-bucket cap in §2.1, with the record that named them gone. That is precisely |
| +the read-first-delete-last failure the walk's ordering exists to prevent. |
| + |
| +`Teardown` takes a `Scanner` and calls `EnsureDisabled` before `DeleteBucket`. |
| +A nil scanner with an enabled record **refuses**, the same way a nil |
| +provisioner already refuses at `walk.go:417` — a teardown that cannot reach the |
| +scanner must not proceed to delete the only record of what it should have |
| +disabled. Both destroy callers (`appsettings.go:845`, `ownership.go:417`) pass |
| +only a `Provisioner` today and must be updated with the signature. |
| + |
| +### 7.3 Engine drift |
| + |
| +`scanning.enabled.engine` is stamped at grant and the deployment's configured |
| +engine can change under it. Two cases, both refusals rather than guesses: |
| + |
| +- **Record says one engine, deployment now runs another.** The sweep refuses |
| + for that app and says so; the verdicts in the bucket were written by an |
| + engine whose tag key the current one does not read, and silently recounting |
| + them as unscanned would destroy the coverage history. An operator revokes and |
| + re-grants deliberately. |
| +- **Engine is `guardduty` and `Deployment.Endpoint != ""`.** GuardDuty cannot |
| + protect a bucket that is not in S3. This is refused at boot, where a |
| + misconfiguration is one loud failure rather than a per-app mystery later. |
| + |
| +## 8. Enforcement: tags by default, deny behind a per-app flag |
| + |
| +Ship verdicts as tags and let apps decide. **(decision)** |
| + |
| +`ScanBlock.Enforce`, default false, adds a Deny statement to the app's inline |
| +policy: deny `s3:GetObject` where `s3:ExistingObjectTag/<tagKey>` equals the |
| +engine's infected value, both supplied by `Scanner.Enforcement()`. The |
| +compliance answer does not need it — §1 ruled enforcement strength is not what |
| +is being asked — and a deny that misfires takes a live app's reads down. |
| + |
| +**Draft 1's migration claim was false and is the reason this section changed.** |
| +It said grant and rotate rewrite the policy unconditionally, so flipping the |
| +flag would be picked up by the next walk. In fact `PutUserPolicy` is reachable |
| +only through `EnsureUser` (`aws.go:302`), which is called from exactly one line |
| +(`walk.go:184`), and `Grant` returns early before it whenever the store is |
| +healthy (`walk.go:169-177`): |
| + |
| +```go |
| +if rec.Provisioned != nil && rec.Provisioned.Endpoint == d.Endpoint { |
| + missing, err := EnvDelivered(...) |
| + if len(missing) == 0 { return rec, nil } |
| +} |
| +``` |
| + |
| +`Rotate` never calls it at all. So on every healthy provisioned store — |
| +which is all of them — flipping `Enforce` would have changed nothing, forever. |
| + |
| +The correction, and it is an interface change: **`EnsureUser` gains the deny |
| +state.** |
| + |
| +```go |
| +EnsureUser(ctx context.Context, name, path, bucketARN string, deny *DenyTag) (arn string, err error) |
| + |
| +type DenyTag struct{ Key, Value string } // nil = no deny statement |
| +``` |
| + |
| +One policy writer, not two, so the base policy and the enforced policy cannot |
| +drift. `Grant` passes the record's current `Enforce` state, so a repair walk |
| +can never silently drop an operator's enforcement. The scan walk calls |
| +`EnsureUser` directly when `Enforce` changes — it is idempotent and the user |
| +already exists. `MemProvisioner` mirrors the signature; every existing caller |
| +and test updates with it. |
| + |
| +**Anti-forgery.** AWS's documented tag-based access control uses a *bucket* |
| +policy with `NotPrincipal` on the GuardDuty role plus a statement denying |
| +`s3:PutObjectTagging` on the scan tag key, so a caller cannot mark its own |
| +object clean. This design's identity-policy deny is a valid and simpler |
| +variant, and it is safe **only because the app user has no `PutObjectTagging` |
| +today** (`aws.go:130`). That is now load-bearing: **`s3:PutObjectTagging` must |
| +never be added to the Allow statement.** §9's widening adds |
| +`s3:GetObjectTagging` and nothing else. |
| + |
| +## 9. Surface |
| + |
| +`carlos store` has four sub-verbs — `create`, `status`, `grant`, `rotate` |
| +(`store.go`). Scanning adds a nested family, reusing the existing verb names |
| +so the actor of each is obvious from the word: |
| + |
| +| command | actor | effect | |
| +|---|---|---| |
| +| `carlos store scan request` | member | records that this app wants scanning | |
| +| `carlos store scan status` | member | prints the §5 line | |
| +| `carlos store scan grant` | operator | enables the engine on the bucket | |
| +| `carlos store scan revoke` | operator | disables it | |
| +| `carlos store scan sweep` | operator | starts or advances a sweep | |
| +| `carlos store scan enforce on\|off` | operator | flips the §8 Deny | |
| + |
| +Draft 1 used `enable`/`on` for the member/operator pair, which is opaque about |
| +who does what; `request`/`grant` matches `create`/`grant` on the store itself. |
| + |
| +API mode only, like the rest of `store.go`, for the reason its header gives: |
| +these verbs drive walks that call out to the deployment's cloud account, an |
| +authority a headless box cannot exercise. |
| + |
| +`store status` gains the scan line, so the existing verb does not become a |
| +half-truth. `docs/site/cli.md` gains the six verbs under its `### carlos |
| +store` section (line 393) — the docsite gate only checks top-level headings, |
| +so this is a house-rule obligation, not something CI will catch. |
| + |
| +## 10. The infra half |
| + |
| +Belongs in `tito/carlos-infrastructure`: |
| + |
| +- **The GuardDuty scanning role.** Enabling a plan requires an IAM role |
| + GuardDuty assumes — EventBridge managed rule, `GetObject`, |
| + `PutObjectTagging`, and KMS where the bucket is encrypted — passed at plan |
| + creation, so the console additionally needs `iam:PassRole` on it. One |
| + per-deployment role scoped to `arn:aws:s3:::<BucketPrefix>*` covers every |
| + minted bucket without an edit per app; a role scoped bucket-by-bucket would |
| + need updating on every `scan grant`. |
| +- **Console permissions** to create, read and delete Malware Protection plans, |
| + and to read the resulting tags. |
| +- **`clamd` placement** — deferred with §4.2, but when it lands: its own unit |
| + on a box with a current signature database, reached over an authenticated |
| + transport. `clamd`'s INSTREAM protocol is unauthenticated TCP, so the |
| + transport is a decision that spec must make, not inherit. |
| + |
| +Note the honest correction to draft 1: moving `clamd` out of the console moves |
| +the *signature database*, not the tenant bytes — the sweep still fetches the |
| +object into the console process to stream it. Only a design where the worker |
| +holds its own read credential and the console never sees the bytes would move |
| +the blast radius, and that is the shape §4.2's spec should choose. |
| + |
| +## 11. Risks to verify during implementation |
| + |
| +- **GuardDuty tag semantics.** Confirm the tag key, every value it can take, |
| + and whether skipped, too-large, access-denied and failed objects are tagged |
| + at all. An untagged object is indistinguishable from an unscanned one, which |
| + would make `Unscanned` overcount forever. Map every documented status onto |
| + the §3 verdict set before writing the counter. |
| +- **`SendObjectMalwareScan` permissions and rate.** The on-demand path in §4.1 |
| + is what closes the coverage gap; confirm the permission it needs, whether it |
| + is throttled, and what it costs per call, because the sweep may issue one per |
| + unscanned object. |
| +- **The bucket cap in practice.** Confirm how `Capacity` can read the current |
| + protected-bucket count, so §2.1's refusal is real rather than a guess. |
| +- **Hetzner object tagging** (deferred with §4.2). `s3compat`'s quirks table |
| + exists because Ceph RGW already differed from S3 once in a way that broke |
| + every conditional write. Measure `PutObjectTagging`/`GetObjectTagging` |
| + before relying on them; the fallback is a verdict sidecar object under a |
| + reserved prefix. |
| + |
| +## 12. Out of scope |
| + |
| +- **The ClamAV engine** (§4.2) — specified, deferred until a non-AWS |
| + provisioner exists. |
| +- Scanning the deployment's own control-plane bucket, release artifacts, or |
| + instance replicas. This covers app stores only. Whether the compliance |
| + question reaches further is for whoever is asking it. |
| +- Quarantine as object movement. Verdicts are tags; nothing is copied aside or |
| + deleted. Deleting a member's data on a scanner's say-so is a policy decision |
| + nobody has made. |
| +- Scan-on-upload latency guarantees. The sweep is a reconciler. |
| +- Notifying members of an infected object. Visible through `scan status`; a |
| + push path can come later. |
| + |
| +## 13. Testing |
| + |
| +- **Contract test for `Scanner`**, beside `contract_test.go`'s existing one for |
| + `Provisioner`, run against `MemScanner` and the GuardDuty engine. |
| + Idempotence is the property: enable twice, disable twice, scan twice. |
| +- **Sweep resumption.** A sweep interrupted mid-bucket and re-run reaches the |
| + same rollup without double-counting; a fresh sweep resets counters. |
| +- **Sweep CAS.** A declare or rotate landing mid-sweep does not fail the sweep, |
| + and does not lose the other writer's block. |
| +- **The purge guard, asserted.** A new subtest writes a record carrying a |
| + `scanning` block and proves `release.PurgeApp` still refuses on `grant`/ |
| + `provisioned` and ignores the new key — the claim §5 makes and |
| + `TestPurgeGuardSeesOurRecord` does not currently cover. |
| +- **Policy round-trip.** `EnsureUser` with `deny == nil` produces today's exact |
| + two-statement document, byte for byte, so enforcement-off stores are |
| + unchanged; with a `DenyTag` it produces three statements and no |
| + `PutObjectTagging`. |
| +- **Enforcement actually applies.** Flipping `Enforce` on a healthy provisioned |
| + store changes the live policy — the regression draft 1 would have shipped. |
| +- **Cap refusal.** At the quota, `scan grant` refuses with the count and the |
| + cap, and enables nothing. |
| +- **Teardown disables first.** A torn-down store with scanning granted calls |
| + `EnsureDisabled` before `DeleteBucket`, and a nil scanner with an enabled |
| + record refuses the same way a nil provisioner does (`walk.go:417`). |
| +- **Gate order.** A non-operator hitting the operator door gets the grant |
| + door's byte-identical posture, member gate first. |
| +- **Inert deployment.** With `CARLOS_STORE_SCANNER` unset the operator doors |
| + 503 naming the variable, the member request door still records, and no sweep |
| + touches a backend. |