amadan
spec: draft 3 — ClamAV primary, GuardDuty optional, self-hosters included
Direction change from Paul: the 25-bucket GuardDuty cap (per account per region, not adjustable) is a poor fit for a bucket-per-app platform, so ClamAV becomes the primary engine and GuardDuty an option behind the seam. The feature is offered to self-hosters rather than being a Carloku-only control: set CARLOS_STORE_SCANNER=clamav, run a clamd unit, done. Draft 2's review found four more holes, all fixed here: - The compliance artifact was destroyed every cycle. One *Sweep served both "in progress" and "complete", and After == "" meant both "finished" and "start fresh", so the tick after completion overwrote the audit answer with page-one partials. Current and LastComplete are now separate. - Grant could not build a DenyTag: it takes a Provisioner and has no Scanner to ask. The record now carries the tag itself, not a bool. - The retry rule could resurrect a torn-down app. A conditional write against a key Teardown just deleted reads as ErrCASConflict, and a blind retry writes create-only. Guarded. - storagePass does not run hourly; it returns once the day's object exists. The scan pass gets its own ticker on the StartInfraPass shape. Two things the reviews did not find. Evidence moves to an apps/<acct>/<app>/scan.json sidecar with a single writer, which dissolves the CAS choreography draft 2 needed to write rollups into a record three doors also write. And scanning ciphertext is meaningless: the family default is server-blindness, so for an encrypted store every object scans clean forever. The spec now says that plainly and gives the audit response a second true sentence rather than letting someone enable scanning on an encrypted bucket and believe the result. Enforcement follows internal/mail/walk.go's "C2" fix, which solved this identical bug: EnsureUser runs unconditionally before the idempotence short-circuit, so one convergence path rewrites the policy.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E1vDqqMC44bNatFmt6F2Ce
1 file changed,
+449
−453
docs/superpowers/specs/2026-09-01-object-storage-virus-scanning-design.md+449 −453
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| index 6cd33980f..d87083a44 100644 |
| --- 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 |
| @@ -2,214 +2,231 @@ |
| **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. |
| +question. Items marked **(ruling)** were decided in conversation; items marked |
| +**(decision)** are this spec's 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. |
| +**Revision history.** Draft 1 was reviewed and six blockers found. Draft 2 |
| +fixed them and was reviewed again, finding four more. Draft 3 (this one) fixes |
| +those and carries a **direction change from Paul, 2026-09-01**: after the |
| +25-bucket GuardDuty cap surfaced, **ClamAV becomes the primary engine and |
| +GuardDuty an option**, and the feature is offered to self-hosters rather than |
| +being a Carloku-only control. **(ruling)** |
| -## 1. What is actually being asked |
| +## 1. What is being asked, and what can honestly be claimed |
| 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: |
| +that solves, the answer was a **compliance checkbox** — someone wants to know |
| +whether stored files are scanned and the deployment needs a defensible yes. |
| +Coverage and evidence matter; detection latency and enforcement strength do |
| +not. **(ruling)** |
| + |
| +So the question to answer is not "how fast did we catch it" but **"how many |
| +objects lack a recorded verdict?"**, and the answer must be a number someone |
| +can check. |
| + |
| +**The two sentences the audit response may use, and no stronger ones:** |
| + |
| +> As of the last completed sweep, which finished at time T, N objects in this |
| +> bucket carried no recorded verdict. |
| + |
| +> For apps that store encrypted objects, scanning is not applicable — see §2.1. |
| + |
| +Three words in the first sentence are load-bearing. *Completed* — a sweep in |
| +progress is not an answer, which is why §5 keeps the last complete one |
| +separately. *Recorded* — an absent verdict has causes other than "unscanned" |
| +(a tag ceiling, a permissions failure, an engine that could not read the |
| +object), and the count must not silently assert otherwise. *T* — the window is |
| +as wide as a sweep takes, so §7.1 gives it a budget rather than leaving it |
| +open-ended. |
| + |
| +This is a point-in-time reconciliation, not a continuous guarantee. It is |
| +still worth more than a scan-on-upload pipeline, whose misses are invisible |
| +while a reconciler's are counted. |
| + |
| +### 1.1 Why a reconciler and not a pipeline |
| + |
| +CARLOS never sees the PUT. `internal/objstore` is a control plane and says so: |
| > 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`). |
| +It 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 — `aws.go:129-130`), seals the |
| +secret into the app's config, and steps out. There is no request path to hook |
| +because there is no request path. |
| -**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 is that **CARLOS writes the app's IAM policy**, so the |
| +control plane can gate reads on a verdict without being in the data path. §8 |
| +cashes that in. |
| -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. Scope, and who this is for |
| -### 2.1 GuardDuty protects at most 25 buckets per account per Region |
| +**The primary engine is ClamAV, running on the deployment's own |
| +infrastructure.** It has no bucket cap, works against any S3-compatible store, |
| +and is available to any deployment including self-hosted ones. **(ruling)** |
| -This is a hard architectural constraint and it was missed in draft 1. |
| +**GuardDuty is an option for AWS deployments that prefer to buy it**, kept |
| +behind the seam. It is not the default, for a reason worth recording: AWS caps |
| +Malware Protection at **25 protected buckets per account per Region, not |
| +adjustable**, and CARLOS mints one bucket per app (`MintedBucket`, |
| +`walk.go:62`). A bucket-per-app platform hits that ceiling at 25 apps. §4.2 |
| +covers what the engine must therefore do. |
| -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. |
| +### 2.1 Scanning ciphertext is meaningless, and this must be said out loud |
| -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: |
| +CARLOS's stated family default is server-blindness — *"If the server is |
| +compromised, the attacker learns nothing"* (`building-carlos-apps/SKILL.md`). |
| +An app following that default stores encrypted objects. |
| -- `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)** |
| +**A virus scanner cannot scan ciphertext.** Every object in such a bucket |
| +scans clean, forever, whatever it contains. Two consequences the design must |
| +carry rather than let someone discover after enabling it: |
| -## 3. Shape: a Scanner seam beside Provisioner |
| +- **`scan status` must never present a clean result for an encrypted store as |
| + though it meant something.** The compliance answer for those apps is the |
| + second sentence in §1, not the first. |
| +- **The ClamAV engine reads tenant object bytes.** For a server-blind app that |
| + is both useless and a trust regression, and it is the strongest argument for |
| + the per-app opt-in already chosen in §6: the app owner is the only party who |
| + knows whether their objects are plaintext, and the right party to consent to |
| + the deployment reading them. |
| -`Provisioner` is the existing backend seam, and its doc comment already |
| -promises this move: |
| +The platform cannot reliably detect encryption, so it does not try to guess. |
| +`scan request` states plainly what the member is consenting to, and `scan |
| +status` repeats it. **(decision)** |
| -> v1 ships the AWS implementation and this package's in-memory fake. A |
| -> Hetzner/R2/Minio provisioner arrives here later without touching a walk. |
| +## 3. The Scanner seam |
| -Scanning gets the same treatment. **(decision)** |
| +`Provisioner`'s doc comment already promises this move ("A Hetzner/R2/Minio |
| +provisioner arrives here later without touching a walk"). Scanning mirrors it. |
| ```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. |
| +// the sweep 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 releases whatever EnsureEnabled took. Teardown calls it |
| + // BEFORE deleting the bucket (§7.3). |
| 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 answers the recorded verdict for one object. No verdict is |
| + // (VerdictUnknown, nil) — a state, not an error. |
| 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 causes one object to be scanned and its verdict recorded. It may |
| + // return before a verdict exists; 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). |
| + // Enforcement names the tag this engine writes, so the policy writer can |
| + // deny reads on it (§8). Engine-specific by necessity. |
| + Enforcement() DenyTag |
| + // Capacity reports buckets still protectable and the cap; (-1, -1) is |
| + // uncapped. ClamAV is uncapped; GuardDuty is not (§2). |
| Capacity(ctx context.Context) (remaining, cap int, err error) |
| + // Provenance names what did the scanning — engine and signature version — |
| + // for the evidence record (§5). |
| + Provenance(ctx context.Context) (engine, version string, err error) |
| } |
| type Verdict string |
| const ( |
| - VerdictUnknown Verdict = "" // no verdict recorded yet |
| + VerdictUnknown Verdict = "" // no recorded verdict |
| VerdictClean Verdict = "clean" |
| VerdictInfected Verdict = "infected" |
| - VerdictSkipped Verdict = "skipped" // too large, unsupported, or unextractable |
| - VerdictFailed Verdict = "failed" // the engine tried and could not |
| + VerdictSkipped Verdict = "skipped" // too large, unsupported, unextractable |
| + VerdictDenied Verdict = "denied" // the engine could not read the object |
| + VerdictFailed Verdict = "failed" // the engine tried and failed |
| ) |
| + |
| +type DenyTag struct{ Key, Value string } |
| ``` |
| `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. |
| +branches on engine. `Enforcement` is where that difference must surface, |
| +because a Deny statement names a literal key and value. |
| + |
| +`Provenance` is new in draft 3 and exists for the audit answer: "scanned with |
| +what?" deserves a reply. The sweep records the engine and its signature |
| +database version, so a pass run against three-month-old signatures is visible |
| +rather than indistinguishable from a current one. |
| + |
| +## 4. The engines |
| + |
| +### 4.1 `clamavScanner` — primary, shipped in v1 |
| + |
| +`EnsureEnabled`/`EnsureDisabled` are no-ops; there is nothing on the bucket to |
| +configure. `Scan` fetches the object, streams it to `clamd`, and writes the |
| +verdict as a `carlos-scan` object tag. `Verdict` reads it back. `Capacity` is |
| +uncapped. `Provenance` returns the ClamAV engine and signature-database |
| +version from clamd's `VERSION` command. |
| + |
| +**The read path already exists in this codebase**, which is what makes this the |
| +cheap option rather than the expensive one. `sizeProvisionedStore` |
| +(`economicspass.go:990`) already lists a tenant's provisioned bucket directly |
| +with `objstore.S3ClientFor` — region plus optional endpoint override, no IAM |
| +involved — from a scheduled console pass. The sweep needs the same client with |
| +`GetObject` added. |
| + |
| +**Transport.** `clamd`'s INSTREAM protocol is unauthenticated, so it is not |
| +exposed to a network. The console reaches clamd over a **unix socket** on the |
| +same box (`CARLOS_CLAMD_SOCKET`, default `/run/clamav/clamd.ctl`). A TCP |
| +option is deliberately not offered in v1: an unauthenticated scanning service |
| +reachable over a network is a worse problem than the one being solved. |
| +**(decision)** |
| + |
| +**Size ceiling.** clamd's `StreamMaxLength` defaults to 25 MB and objects above |
| +it cannot be streamed. Such objects record `VerdictSkipped` with the reason, |
| +and `Sweep.Skipped` counts them — a skipped object is not a clean one and the |
| +evidence must not blur them. The ceiling is configurable |
| +(`CARLOS_CLAMD_MAX_BYTES`) and reported by `scan status`, because an auditor |
| +told "12,000 clean" deserves to know 400 were too large to look at. |
| + |
| +**Signature freshness.** `freshclam` keeps the database current and is the |
| +operator's responsibility. §5 records the version per sweep so a stale |
| +database is visible in the evidence rather than silently producing clean |
| +results. |
| + |
| +**Egress.** Every unscanned object is fetched once. Same-region reads are free |
| +on AWS and on Hetzner; a cross-region deployment pays. `scan status` reports |
| +bytes fetched per sweep. |
| + |
| +### 4.2 `guarddutyScanner` — optional, AWS only |
| + |
| +`EnsureEnabled` creates a Malware Protection plan with object tagging on; |
| +`Verdict` reads `GuardDutyMalwareScanStatus`; `EnsureDisabled` deletes the |
| +plan; `Capacity` reports against the 25-bucket cap. |
| + |
| +**Three constraints, all of which the engine must surface rather than hide:** |
| + |
| +- **The cap.** `scan grant` counts protected buckets first and refuses at the |
| + ceiling with the count and the quota named — "25 of 25 protected in |
| + eu-west-1; this is a non-adjustable AWS quota" — never a raw AWS error. |
| +- **It scans new uploads only.** Objects already present when the plan is |
| + created are not scanned by it. `Scan` therefore calls |
| + `SendObjectMalwareScan`, the documented on-demand path. On-demand scans are |
| + excluded from the free tier, so the sweep pays per unknown object per pass; |
| + §7.1's budget bounds that. |
| +- **Tagging must precede upload, and whether on-demand escapes that is |
| + UNVERIFIED.** AWS states: "You must enable this option before an object gets |
| + uploaded. If you enable tagging after an object gets uploaded … GuardDuty |
| + will not be able to add tags to the scanned object." Every object in an |
| + existing store predates the plan. **If on-demand scans of such objects are |
| + not tagged, `Verdict` never converges on any existing store and this engine |
| + needs an EventBridge consumer reading `statusReasons` instead of tags — a |
| + different architecture.** This must be measured against a real bucket before |
| + any GuardDuty implementation work starts. It is the first item in §11 and it |
| + is a gate, not a risk. |
| 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. |
| +the 2025-02 reduction) plus tagging cost, on the deployment's account. Objects |
| +over 100 GB are not scanned. |
| + |
| +## 5. Evidence: a sidecar, not the store record |
| + |
| +Two objects, two writers, no contention. **(decision — draft 2 put everything |
| +in `store.json` and needed a page of CAS discipline to survive it.)** |
| + |
| +**`store.json` gains a small `scanning` block, written only by the doors:** |
| ```go |
| type ScanRequested struct { |
| @@ -218,7 +235,7 @@ type ScanRequested struct { |
| } |
| type ScanEnabled struct { |
| - Engine string `json:"engine"` // "guardduty" | "clamav" |
| + Engine string `json:"engine"` // "clamav" | "guardduty" |
| At time.Time `json:"at"` |
| By string `json:"by"` // the operator |
| } |
| @@ -226,323 +243,302 @@ type ScanEnabled struct { |
| 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 |
| + // Enforce is the deny in force, nil when off. It holds the tag itself, |
| + // not a bool: Grant takes a Provisioner and has no Scanner to ask, so |
| + // the record must carry what the policy writer needs (§8). |
| + Enforce *DenyTag `json:"enforce,omitempty"` |
| +} |
| +``` |
| + |
| +**`apps/<acct>/<app>/scan.json` holds the rollups, written only by the sweep:** |
| + |
| +```go |
| +type ScanSidecar struct { |
| + Schema int `json:"schema"` |
| + App string `json:"app"` |
| + Current *Sweep `json:"current,omitempty"` // in progress |
| + LastComplete *Sweep `json:"last_complete,omitempty"` // the audit answer |
| } |
| type Sweep struct { |
| StartedAt time.Time `json:"started_at"` |
| - At time.Time `json:"at"` // last page written |
| + At time.Time `json:"at"` |
| + Engine string `json:"engine"` |
| + Signature string `json:"signature,omitempty"` // §3 Provenance |
| 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"` |
| - After string `json:"after,omitempty"` // resume key, "" = complete |
| + Bytes int64 `json:"bytes"` |
| + After string `json:"after,omitempty"` // resume key |
| } |
| ``` |
| -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 |
| +**`Current` and `LastComplete` are separate, and this is the fix for draft 2's |
| +worst bug.** There, one `*Sweep` served both and `After == ""` meant both |
| +"complete" and "start fresh" — so the tick after a sweep finished overwrote the |
| +completed counts with page-one partials, destroying the compliance artifact |
| +every cycle. Now a sweep accumulates in `Current`; on its last page it moves to |
| +`LastComplete` and `Current` clears. `scan status` reads `LastComplete` for the |
| +audit line and mentions `Current` only as progress. |
| + |
| +The separation also removes all of draft 2's CAS choreography. The sweep is |
| +the only writer of `scan.json`, so a lost race means another sweep is running |
| +and this one stands down — it does not merge blocks into a record three doors |
| +are also writing. `Teardown` deletes the sidecar; the trash purge sweeps it. |
| + |
| +**The PARSER LIST hazard, still live.** `objstore.go` records that |
| +`release.PurgeApp` parses `grant` and `provisioned` with its own local struct |
| +and refuses box-side auto-purge when either is present. Additive changes are |
| +safe (`trash.go:213-216` ignores unknown keys), so `scanning` is fine, but |
| +`grant` and `provisioned` must not move. §13 adds the subtest that asserts the |
| +additive claim, which `TestPurgeGuardSeesOurRecord` does not currently cover. |
| + |
| +## 6. Who turns it on |
| + |
| +Three actors, mirroring the store's own shape. **(decision, accepted)** |
| + |
| +- **The member requests** — `carlos store scan request`. Records the ask, |
| + spends nothing, and states what §2.1 says about plaintext and about the |
| + deployment reading their objects. |
| +- **An operator grants** — `carlos store scan grant`. Calls the cloud account |
| + and starts a meter, so it sits behind `CARLOS_STORE_OPERATORS`, the same |
| + gate `apistore.go` applies to store grants for the same reason, checked |
| + **after** the member gate so a 403 never confirms which apps exist. |
| + |
| +`CARLOS_STORE_SCANNER` unset means no engine is built at boot and the operator |
| +doors 503 naming the variable. The **member's** request door still records — |
| +following the store create door's deliberate precedent (`apistore.go:145-152`): |
| +a member cannot see whether their deployment has a backend, and refusing them |
| +teaches them nothing they can act on. `scan status` reports `engine: none`. |
| + |
| +**Self-hosters** set `CARLOS_STORE_SCANNER=clamav` and run a clamd unit. That |
| +is the whole opt-in: no AWS account, no cloud service, no per-GB meter. |
| + |
| +## 7. The sweep |
| + |
| +1. Read `store.json`. No `scanning.enabled` — return without touching the |
| + backend or the sidecar. |
| +2. Read `scan.json`. Resume from `Current.After` when set, else start a fresh |
| + `Current` with zeroed counters. |
| +3. List a page with `ListObjectsV2` using `StartAfter` — not a |
| + `ContinuationToken`, whose lifetime across processes is unspecified and, on |
| + Ceph, unmeasured. A key is a portable resume point. |
| +4. For each key ask `Verdict`, count it, and call `Scan` when unknown. |
| +5. Write the sidecar with `After` set to the last key of the page. On the final |
| + page, move `Current` to `LastComplete` and clear `Current`. |
| + |
| +**Honest semantics.** A key added lexically before the resume point mid-sweep |
| +is missed by that sweep and caught by the next. A key deleted after being |
| +counted inflates `Objects` for that pass. Neither breaks a point-in-time |
| +reconciliation, and §1's sentence is worded to survive both. |
| + |
| +**Re-sweep interval.** A completed sweep does not immediately restart. |
| +`CARLOS_STORE_SCAN_INTERVAL` (default 24h) is the floor, so `LastComplete` |
| +stays stable long enough to be quoted. |
| + |
| +### 7.1 The driver, and its budget |
| + |
| +Its own goroutine on the `StartInfraPass` pattern (`infrapass.go:49`): run |
| +once at boot, then hourly, ticking independently. **Not inside |
| +`economicsPass`** — that pass runs census, statements and billing in one |
| +goroutine and already worries about a slow step delaying the others. |
| + |
| +Draft 2 claimed the scan pass could ride `storagePass`. That was wrong: |
| +`storagePass` returns at `economicspass.go:138-140` once the day's object |
| +exists, so it iterates once per UTC day, not hourly. The scan pass iterates |
| +provisioned-and-enabled stores itself. |
| + |
| +**Budget.** One page of 1,000 keys per app per tick would take 42 days for a |
| +million-object bucket, which would make T in §1 a six-week window. |
| +`CARLOS_STORE_SCAN_BUDGET` bounds work per app per tick by time (default 60s), |
| +not by pages, so a fast store completes in one tick and a slow one still |
| +progresses. `scan status` reports the projected completion when a sweep spans |
| +more than one tick, because an audit answer six weeks stale should say so. |
| + |
| +**Mutual exclusion.** The `scan sweep` verb and the ticker are two drivers of |
| +one sidecar. The verb advances the sweep synchronously for one budget and |
| +returns; the ticker skips any app whose `Current.At` is newer than the tick |
| +started. A sweep is never run concurrently with itself. |
| + |
| +### 7.2 Retry guard |
| + |
| +If the sidecar write loses a CAS race, re-read and continue **only if** |
| +`Current` still exists and its `After` matches the value this page started |
| +from. Otherwise stand down: another driver is ahead. |
| + |
| +The guard exists for a specific failure draft 2 would have shipped. A |
| +conditional write against a key `Teardown` has just deleted is classified as |
| +`ErrCASConflict` (`release/store.go:521-534`); a blind re-read returns nothing, |
| +and a blind retry writes create-only — resurrecting a sidecar for an app that |
| +no longer exists. A sidecar write for a torn-down app must fail, not retry. |
| + |
| +### 7.3 Teardown disables first |
| `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. |
| +user, unsets config, then deletes the record. Nothing today disables a scanner, |
| +so a GuardDuty plan and its managed EventBridge rule would outlive the bucket, |
| +counting against the §2 cap, with the record naming them gone. |
| + |
| +`Teardown` takes a `Scanner`, calls `EnsureDisabled` before `DeleteBucket`, and |
| +deletes the sidecar. A nil scanner with an enabled record **refuses**, as a nil |
| +provisioner already does (`walk.go:417`). The destroy door's pre-check |
| +(`appsettings.go:828-833`) tests only `StoreProvisioner` today and needs the |
| +matching scanner check, or the refusal surfaces as a generic "Could not |
| +destroy" flash. Both destroy callers (`appsettings.go:845`, |
| +`ownership.go:417`) update with the signature. |
| + |
| +### 7.4 Engine drift |
| + |
| +`scanning.enabled.engine` is stamped at grant and the deployment's engine can |
| +change under it. Both cases refuse rather than guess: |
| + |
| +- **Record names another engine.** The sweep refuses for that app. Verdicts in |
| + the bucket were written under a tag key the current engine 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 |
| + protect a non-S3 bucket. Refused at boot, where it is one loud failure rather |
| + than a per-app mystery. |
| + |
| +## 8. Enforcement |
| + |
| +`ScanBlock.Enforce` — nil by default — adds a Deny statement to the app's |
| +inline policy: deny `s3:GetObject` where `s3:ExistingObjectTag/<Key>` equals |
| +`<Value>`. It stores the `DenyTag` itself rather than a bool because `Grant` |
| +takes a `Provisioner` and has no `Scanner` to ask; the record carries what the |
| +policy writer needs, and engine drift becomes visible in the record instead of |
| +being silently recomputed. |
| + |
| +**How the policy actually gets rewritten — the bug draft 1 and 2 both had.** |
| +`PutUserPolicy` is reachable only through `EnsureUser` (`aws.go:302`), called |
| +from exactly one line (`walk.go:184`), and `Grant` returns early before it for |
| +any healthy store (`walk.go:169-177`). `Rotate` never writes a policy. So a |
| +flag flip would have been permanently inert. |
| + |
| +**This repo has already fixed this exact bug once, and the fix is the |
| +precedent to follow.** `internal/mail/walk.go:654-670` (marked "C2 — review |
| +follow-up") moved `EnsureUser` to run **unconditionally, before the idempotence |
| +short-circuit**, with a comment explaining that the short-circuit had left the |
| +policy naming only the first domain forever. The same move here: |
| + |
| +- `EnsureUser` gains `deny *DenyTag` and stays the single policy writer. |
| +- `Grant` calls it **before** the short-circuit, passing the record's current |
| + `Enforce`. The short-circuit continues to gate only the expensive mint and |
| + deliver, which is what it was for. |
| +- `scan enforce on|off` writes the record and re-runs the grant walk. One |
| + convergence path, no second policy writer. |
| +- **A nil scanner with `Enforce` set refuses the repair** rather than |
| + rewriting the policy without the deny. Dropping enforcement silently is the |
| + failure this section exists to prevent. |
| + |
| +`MemProvisioner` mirrors the signature and gains policy state, which it lacks |
| +today (`MemUser` is ARN plus keys), so §13's policy tests have something to |
| +assert against. |
| + |
| +**Anti-forgery.** AWS's documented tag-based access control uses a bucket |
| +policy denying reads unless the tag says clean, plus a statement stopping a |
| +caller retagging its own object. This design's identity-policy deny is a |
| +weaker variant in a way a compliance reader must not be allowed to miss: |
| +**AWS's blocks unscanned objects; this one blocks only known-infected ones.** |
| +That is the right trade under §1's ruling, and it must be stated, not implied. |
| +It is safe only because the app user has no `s3:PutObjectTagging` |
| +(`aws.go:130`) and therefore cannot mark its own object clean — so |
| +**`s3:PutObjectTagging` must never be added to the Allow statement.** §9 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 request` | member | records the ask; states §2.1 | |
| +| `carlos store scan status` | member | the §1 line, plus engine and signature | |
| | `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 revoke` | operator | disables it; clears `Enforce` | |
| +| `carlos store scan sweep` | operator | advances a sweep by one budget | |
| | `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. |
| +`request`/`grant` mirrors `create`/`grant` on the store itself, so the actor is |
| +readable from the verb. API mode only, like the rest of `store.go`. |
| + |
| +`store status` gains the scan line. `docs/site/cli.md` gains the six verbs |
| +under `### carlos store` (line 393) — the docsite gate reads only the dispatch |
| +switch (`cliscan.go:22`), so this is a house-rule obligation, not one CI |
| +catches. |
| + |
| +## 10. The infra half (`tito/carlos-infrastructure`) |
| + |
| +- **clamd.** A systemd unit per console box with `freshclam` on a timer, a |
| + unix socket the console can reach, and `StreamMaxLength` matched to |
| + `CARLOS_CLAMD_MAX_BYTES`. This is the primary path and the only one a |
| + self-hoster needs. |
| +- **Console S3 permissions.** `GetObject` and `PutObjectTagging` on |
| + `arn:aws:s3:::<BucketPrefix>*`. It already lists those buckets |
| + (`sizeProvisionedStore`); scanning adds read and tag. |
| +- **GuardDuty, only if used.** A scanning role GuardDuty assumes (EventBridge |
| + managed rule, `GetObject`, `PutObjectTagging`, KMS where encrypted), passed |
| + at plan creation, so the console needs `iam:PassRole` on it plus |
| + `guardduty:SendObjectMalwareScan`. Scope the role to |
| + `arn:aws:s3:::<BucketPrefix>*` so it needs no edit per app. |
| + |
| +## 11. Gates and risks |
| + |
| +**Gate — measure before building the GuardDuty engine.** Whether |
| +`SendObjectMalwareScan` tags an object uploaded before tagging was enabled |
| +(§4.2). If it does not, that engine needs an EventBridge consumer instead of |
| +tags. This blocks GuardDuty work only; ClamAV is unaffected. |
| + |
| +**Risks to verify during implementation:** |
| + |
| +- **Hetzner object tagging.** `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 per-object verdicts in the sidecar keyed by object key and etag. |
| +- **Verdict mapping.** Map every GuardDuty status — including `ACCESS_DENIED` |
| + and `UNSUPPORTED` — onto §3's set before writing the counters, and decide |
| + whether `Scan` is re-issued for `failed` and `unknown` on every pass, since |
| + on-demand calls are billed each time. |
| +- **clamd throughput.** One connection per object with a 25 MB ceiling sets the |
| + real budget in §7.1. Measure before promising a completion window. |
| ## 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 |
| +- Scanning the deployment's control-plane bucket, release artifacts, or |
| + instance replicas. App stores only. |
| +- Quarantine as 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. |
| +- Scan-on-upload latency guarantees. |
| +- Notifying members of an infected object. Visible in `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. |
| +- **`Scanner` contract test** over `MemScanner` and each real engine: |
| + enable twice, disable twice, scan twice, verdict on an absent object. Note |
| + `contract_test.go` today is `TestDeliveredContractMinio`, a delivered- |
| + credential wire test — there is no `Provisioner` idempotence contract to sit |
| + beside, so this is a new file, not an addition to an existing pattern. |
| +- **Sweep resumption** reaches the same rollup without double-counting; a |
| + fresh sweep resets counters. |
| +- **`LastComplete` survives** the tick after completion — draft 2's bug, |
| + asserted. |
| +- **Retry guard** stands down rather than resurrecting a sidecar for a |
| + torn-down app. |
| +- **Purge guard, additive.** A record carrying `scanning` still makes |
| + `release.PurgeApp` refuse on `grant`/`provisioned` and ignore the new key. |
| - **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. |
| + two-statement document byte for byte; with a `DenyTag`, three statements and |
| + no `PutObjectTagging`. |
| +- **Enforcement converges.** Flipping `Enforce` on a healthy provisioned store |
| + changes the live policy — the regression drafts 1 and 2 would have shipped — |
| + and a nil scanner with `Enforce` set refuses. |
| +- **Teardown disables first**, deletes the sidecar, and refuses on a nil |
| + scanner with scanning enabled. |
| +- **Gate order and inert deployment.** Operator doors 503 with |
| + `CARLOS_STORE_SCANNER` unset; the member request door still records. |
| +- **Skipped is not clean.** An object over the ceiling counts in `Skipped` and |
| + `scan status` shows it separately. |