amadan
spec: draft 4 — close review 3
Five required edits, plus the mediums: - Enforcement deadlock. Draft 3 refused a repair when the scanner was nil and Enforce was set, while the operator doors 503 without an engine — so unsetting CARLOS_STORE_SCANNER on a deployment with any enforcing record left the store unrepairable and the deny unclearable. Grant now writes the deny from the record whatever the scanner is, which is why the record holds a tag and not a bool; revoke and enforce off are record-only doors. - Verdicts were not bound to the bytes they judged. PutObjectTagging is unconditional, so scan-fetches-v1 / app-overwrites-v2 / scan-tags-clean marks bytes nobody read. The scanned ETag now goes in the tag set and Verdict compares it against the one ListObjectsV2 already returns. - The self-hoster claim was untrue. The store primitive still fatals on any backend but "aws" and its IAM is real AWS, so there is no store to scan without an AWS account. Corrected, with the non-AWS provisioner named as the actual dependency. - The audit sentence hid unscannable objects: a bucket of over-size objects could report "0 objects carried no recorded verdict". It now names both numbers. - Encryption was undetectable and unrecorded, so scan status could not know which answer to give. scan request now takes an explicit --plaintext/--client-encrypted attestation. Also distinguishes client-side encryption (defeats a scanner) from SSE (does not). The ClamAV engine is now specified rather than gestured at: INSTREAM framing, size check before fetch, timeouts, clamd-down behaviour that does not stamp thousands of failed tags, re-scan policy, VERSION parsing, socket group, and the memory a self-hoster needs to know about. Two GuardDuty facts the reviewer doubted are verified against AWS docs and cited: SendObjectMalwareScan exists and takes an object path, and the object ceiling is 100 GB.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 file changed,
+477
−364
docs/superpowers/specs/2026-09-01-object-storage-virus-scanning-design.md+477 −364
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 d87083a44..915854f69 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 |
| @@ -1,16 +1,17 @@ |
| # Virus scanning for the object storage primitive |
| **Date:** 2026-09-01 |
| -**Status:** approved in principle (Paul, 2026-09-01). Tony raised the |
| -question. Items marked **(ruling)** were decided in conversation; items marked |
| -**(decision)** are this spec's reasoning and can be argued with. |
| - |
| -**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)** |
| +**Status:** approved in principle (Paul, 2026-09-01). Items marked **(ruling)** |
| +were decided in conversation; items marked **(decision)** are this spec's |
| +reasoning and can be argued with. |
| + |
| +**Revision history.** Three review rounds, fourteen blockers, all fixed. |
| +Draft 3 carried Paul's direction change — **ClamAV primary, GuardDuty an |
| +option, self-hosters included** — after GuardDuty's 25-bucket cap surfaced. |
| +Draft 4 (this one) closes review 3: the enforcement deadlock, verdicts not |
| +bound to the bytes they judged, the untrue self-hoster claim, an audit |
| +sentence that hid unscannable objects, and the ClamAV engine's missing |
| +protocol detail. |
| ## 1. What is being asked, and what can honestly be claimed |
| @@ -20,24 +21,25 @@ 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:** |
| +**The three sentences an 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. |
| +> bucket carried no recorded verdict, and a further S were recorded as not |
| +> scannable — too large, unreadable, or the engine failed on them. |
| -> For apps that store encrypted objects, scanning is not applicable — see §2.1. |
| +> Objects scanned clean were scanned by ENGINE against signature database |
| +> VERSION. |
| -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. |
| +> For apps whose objects are encrypted before they reach the store, scanning |
| +> is not applicable and no coverage is claimed — see §2.1. |
| + |
| +The first sentence names **two** numbers. Draft 3 named only the unscanned |
| +count, which let a bucket of 12,000 over-size objects report "0 objects |
| +carried no recorded verdict" — technically true under §3's taxonomy and |
| +thoroughly misleading. *Completed* matters too: a sweep in progress is not an |
| +answer, which is why §5 keeps the last complete one separately. And T is as |
| +wide as a sweep takes, so §7.1 gives it a budget and `scan status` prints the |
| +projection when it spans ticks. |
| 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 |
| @@ -51,86 +53,117 @@ CARLOS never sees the PUT. `internal/objstore` is a control plane and says so: |
| > the delivered credentials. |
| 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. |
| +two statements (`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. |
| 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. |
| +control plane can gate reads on a verdict without being in the data path (§8). |
| ## 2. Scope, and who this is for |
| -**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)** |
| +**The primary engine is ClamAV on the deployment's own infrastructure.** No |
| +bucket cap, any S3-compatible store, no cloud service to buy. **(ruling)** |
| + |
| +**GuardDuty is an option for AWS deployments that would rather buy it.** Not |
| +the default: 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`), so it runs out at 25 apps. |
| -**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. |
| +### 2.1 Who can actually turn this on today |
| -### 2.1 Scanning ciphertext is meaningless, and this must be said out loud |
| +Draft 3 said a self-hoster "sets `CARLOS_STORE_SCANNER=clamav` and runs a |
| +clamd unit — no AWS account." **That is not true today and the correction |
| +matters.** |
| + |
| +The store primitive itself is AWS-bound: `cmd/console/main.go:497-498` fatals |
| +on any `CARLOS_STORE_BACKEND` but `"aws"`, and `aws.go:73-75` records that a |
| +non-AWS endpoint "still means IAM-against-real-AWS in v1 — there is nothing |
| +else for it to point at." There is no store for a scanner to scan without AWS |
| +IAM. |
| + |
| +So the honest statement: **scanning is available to any deployment that has |
| +the store primitive, and today that still needs an AWS account for IAM.** The |
| +scanner itself is backend-agnostic and will follow a non-AWS provisioner the |
| +moment one lands — that provisioner, promised in `provisioner.go`'s doc |
| +comment, is the actual dependency for a fully self-hosted story, and this spec |
| +does not deliver it. What draft 4 does deliver is a scanner that will not have |
| +to change when it arrives. **(decision)** |
| + |
| +### 2.2 Scanning ciphertext is meaningless |
| 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. |
| - |
| -**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: |
| - |
| -- **`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. |
| - |
| -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)** |
| +compromised, the attacker learns nothing"* (`building-carlos-apps/SKILL.md` |
| +line 60). An app following it encrypts objects **client-side**, before they |
| +reach the store. |
| + |
| +**A scanner cannot scan ciphertext.** Every object in such a bucket scans |
| +clean forever, whatever it contains. |
| + |
| +Note the distinction draft 3 blurred: this is about **client-side** |
| +encryption. Server-side encryption (SSE-S3, SSE-KMS — on by default for every |
| +AWS bucket since 2023) is transparent to a reader holding the right |
| +credentials and scans perfectly well. Only client-side encryption defeats a |
| +scanner. |
| + |
| +**The platform cannot detect which it is, so it asks.** `scan request` takes |
| +an explicit attestation, recorded on the record: |
| + |
| +- `--plaintext` — objects are readable by the deployment; scanning is |
| + meaningful; the §1 sentences apply. |
| +- `--client-encrypted` — objects are ciphertext. The sweep marks the sidecar |
| + `not_applicable` and does no work, `scan status` prints §1's third sentence |
| + and **refuses to print the first**, and `enforce` is rejected as inert |
| + (nothing will ever be tagged infected). |
| + |
| +Without a recorded attestation, §1's third sentence has no data behind it and |
| +`scan status` cannot know which answer to give. **(decision)** |
| + |
| +### 2.3 What the member is actually consenting to |
| + |
| +The ClamAV engine reads tenant object bytes, and the per-app opt-in is where |
| +the app owner consents to that. One honest qualification, because the consent |
| +story would otherwise overclaim: |
| + |
| +**The console can already read every store.** It holds `PutUserPolicy` and |
| +`CreateAccessKey` over every store user (`aws.go:282-311`), so it can mint |
| +itself a credential to any tenant bucket at will. §10's `GetObject` grant |
| +makes an existing capability explicit rather than creating a new one. What the |
| +opt-in governs is what the platform **does**, not what it **could** do. Say |
| +that plainly rather than implying scanning is what first gives the platform |
| +reach into a member's bucket. **(decision)** |
| ## 3. The Scanner seam |
| -`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 that drives one must be re-runnable after a half-failure. |
| +// idempotent — "already enabled" and "already gone" are successes. |
| type Scanner interface { |
| EnsureEnabled(ctx context.Context, bucket string) error |
| - // EnsureDisabled releases whatever EnsureEnabled took. Teardown calls it |
| + // EnsureDisabled releases what 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. 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 may |
| - // return before a verdict exists; the next sweep reads the result. |
| - Scan(ctx context.Context, bucket, key 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 (§3.1). |
| + Verdict(ctx context.Context, bucket, key, etag string) (Verdict, error) |
| + // Scan scans one object and records its verdict against its etag. It |
| + // returns the verdict when the engine is synchronous (ClamAV) and |
| + // VerdictUnknown when the engine scans asynchronously (GuardDuty). |
| + Scan(ctx context.Context, bucket, key, etag string) (Verdict, error) |
| // Enforcement names the tag this engine writes, so the policy writer can |
| - // deny reads on it (§8). Engine-specific by necessity. |
| + // deny reads on it (§8). |
| Enforcement() DenyTag |
| - // Capacity reports buckets still protectable and the cap; (-1, -1) is |
| - // uncapped. ClamAV is uncapped; GuardDuty is not (§2). |
| + // Capacity reports buckets still protectable and the cap; (-1,-1) is |
| + // uncapped. ClamAV is uncapped; GuardDuty is not. |
| 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 names what did the scanning, for the evidence record. |
| Provenance(ctx context.Context) (engine, version string, err error) |
| } |
| type Verdict string |
| const ( |
| - VerdictUnknown Verdict = "" // no recorded verdict |
| + VerdictUnknown Verdict = "" // no recorded verdict for these bytes |
| VerdictClean Verdict = "clean" |
| VerdictInfected Verdict = "infected" |
| VerdictSkipped Verdict = "skipped" // too large, unsupported, unextractable |
| @@ -141,111 +174,141 @@ const ( |
| type DenyTag struct{ Key, Value string } |
| ``` |
| -`Verdict` hides that each engine reads its own tag key, so the sweep never |
| -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. |
| +`Scan` returning a `Verdict` is draft 4's correction to a subtle counting bug. |
| +ClamAV is synchronous — fetch, stream, tag — so the verdict exists the moment |
| +`Scan` returns. Draft 3's "the next sweep reads the result" was GuardDuty's |
| +shape applied to both, which would have made the **first complete sweep over |
| +every bucket report every object as unscanned**, and with a 24-hour re-sweep |
| +floor the audit sentence would only become true two days in. The sweep counts |
| +a verdict `Scan` hands back. |
| + |
| +### 3.1 Verdicts are bound to the bytes they judged |
| + |
| +`PutObjectTagging` replaces a tag set unconditionally and knows nothing about |
| +versions, so a verdict tag alone says "some bytes under this key were clean |
| +once." The window: `Scan` fetches v1, the app overwrites with v2, `Scan` tags |
| +clean — and v2 is marked clean having never been read. Narrow, but it is |
| +precisely the window worth timing. |
| + |
| +So the engine writes the scanned object's **ETag** into the tag set alongside |
| +the verdict, and `Verdict` compares it against the ETag `ListObjectsV2` |
| +already returns for free on the page being swept. A mismatch is |
| +`VerdictUnknown` — the object gets rescanned, which is the correct outcome for |
| +bytes nobody has looked at. This also closes the forgery story properly: a |
| +re-uploaded object cannot inherit its predecessor's verdict. **(decision — |
| +draft 3 had this only as a Hetzner fallback; it is the primary design.)** |
| ## 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 tagging cost, on the deployment's account. Objects |
| -over 100 GB are not scanned. |
| +Fetch the object, stream it to `clamd`, write `carlos-scan`, |
| +`carlos-scan-etag` and `carlos-scan-at` tags. `Capacity` is uncapped. |
| + |
| +**The read path already exists.** `sizeProvisionedStore` |
| +(`economicspass.go:990`) lists a tenant's provisioned bucket with |
| +`objstore.S3ClientFor(ctx, p.Region, p.Endpoint)` (`aws.go:95`) and the |
| +console's ambient credentials, from a scheduled pass, with no IAM involved. |
| +Scanning adds `GetObject`, `GetObjectTagging` and `PutObjectTagging` to that |
| +same principal. Stream `GetObject.Body` straight into the socket; never buffer |
| +an object in memory. |
| + |
| +**Transport.** `clamd`'s INSTREAM protocol is unauthenticated, so it is never |
| +exposed to a network. The console reaches it over a **unix socket** |
| +(`CARLOS_CLAMD_SOCKET`, default `/run/clamav/clamd.ctl`). No TCP option in v1: |
| +an unauthenticated scanning service reachable over a network is a worse |
| +problem than the one being solved. **(decision)** |
| + |
| +**Protocol, stated so it is not invented twice.** Send `zINSTREAM\0`, then |
| +chunks of a 4-byte big-endian length followed by that many bytes, terminated |
| +by a zero-length chunk. Replies are null-terminated: `stream: OK\0` → clean, |
| +`stream: <Signature> FOUND\0` → infected, `INSTREAM size limit exceeded. |
| +ERROR\0` → skipped. The `z` (null-terminated) command form is used throughout, |
| +not the newline form. One connection per object; `IDSESSION` is not used in |
| +v1, and §11 measures whether that costs enough to revisit. |
| + |
| +**Size ceiling, checked before the fetch.** `ListObjectsV2` already returns |
| +`Size`, so an object above `CARLOS_CLAMD_MAX_BYTES` is tagged `skipped` |
| +without a `GetObject` — paying egress to fetch bytes clamd will refuse is |
| +pure waste. If the variable is set above clamd's own `StreamMaxLength`, clamd |
| +answers `size limit exceeded ... ERROR` mid-stream; that classifies as |
| +**skipped, not failed**, or the counters lie about what happened. |
| + |
| +**Timeouts.** Dial deadline, per-chunk write deadline and reply read deadline |
| +on our side; clamd's `ReadTimeout` and `CommandReadTimeout` on its side. All |
| +configurable, all defaulted, none infinite. |
| + |
| +**When clamd is down.** A dial failure aborts that app's pass **without |
| +writing any verdict** and is reported as a pass error to retry next tick. It |
| +must never stamp thousands of objects `failed`: "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. |
| + |
| +**Re-scan policy.** `failed` and `denied` are re-issued every sweep — both are |
| +usually transient or a permissions fix away. `skipped` is not re-issued unless |
| +the ETag changed or the ceiling moved. `clean` and `infected` are re-issued |
| +only on ETag mismatch (§3.1). |
| + |
| +**Provenance.** `zVERSION\0` answers e.g. `ClamAV 1.4.1/27500/Mon Sep 1 |
| +08:00:00 2026` — engine version, signature database version, database date. |
| +`Sweep.Signature` records all three verbatim. `scan status` warns when the |
| +database date is older than `CARLOS_CLAMD_MAX_SIG_AGE` (default 7 days), |
| +because a clean result from stale signatures should not read like a fresh one. |
| + |
| +**Operational facts self-hosters need up front**, and §10 must carry: clamd |
| +holds roughly 1–1.5 GB of signatures resident; `freshclam` must run on a timer |
| +or the database rots silently; and the console's unit user must be in the |
| +socket's group (`/run/clamav/clamd.ctl` is group `clamav`). |
| + |
| +### 4.2 `guarddutyScanner` — optional, AWS only, gated |
| + |
| +`EnsureEnabled` creates a Malware Protection plan with tagging on; `Verdict` |
| +reads `GuardDutyMalwareScanStatus`; `Capacity` reports against the cap. |
| + |
| +Verified against AWS documentation rather than assumed: |
| + |
| +- **`SendObjectMalwareScan` is real**, takes an S3 object path (optionally a |
| + version), and is the documented on-demand path for objects that predate the |
| + plan. It needs `AmazonGuardDutyFullAccess_v2` on the caller and is excluded |
| + from the free tier, so the sweep pays per unknown object per pass. |
| +- **The object ceiling is 100 GB** and the bucket cap is 25, both from the |
| + quotas table, both non-adjustable. |
| + |
| +**The gate that must be measured before any GuardDuty work starts.** AWS |
| +states tagging "must be enabled before an object gets uploaded" or GuardDuty |
| +"will not be able to add tags to the scanned object." Every object in an |
| +existing store predates its plan. If an on-demand scan of such an object is |
| +not tagged, `Verdict` never converges on any existing store, and this engine |
| +needs an EventBridge consumer reading scan results instead of tags — a |
| +different architecture. **Measure it against a real bucket first.** ClamAV is |
| +unaffected either way, which is why it is the primary engine. |
| ## 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.)** |
| +in `store.json` and needed a page of CAS choreography to survive it.)** |
| -**`store.json` gains a small `scanning` block, written only by the doors:** |
| +**`store.json` gains a `scanning` block, written only by the doors:** |
| ```go |
| type ScanRequested struct { |
| - At time.Time `json:"at"` |
| - By string `json:"by"` // the member |
| + At time.Time `json:"at"` |
| + By string `json:"by"` |
| + Encrypted bool `json:"encrypted"` // §2.2 attestation |
| } |
| type ScanEnabled struct { |
| Engine string `json:"engine"` // "clamav" | "guardduty" |
| At time.Time `json:"at"` |
| - By string `json:"by"` // the operator |
| + By string `json:"by"` |
| } |
| type ScanBlock struct { |
| Requested *ScanRequested `json:"requested,omitempty"` |
| Enabled *ScanEnabled `json:"enabled,omitempty"` |
| - // 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 holds the tag itself, not a bool: Grant takes a Provisioner |
| + // and has no Scanner to ask, so the record carries what the policy |
| + // writer needs (§8). |
| Enforce *DenyTag `json:"enforce,omitempty"` |
| } |
| ``` |
| @@ -254,17 +317,19 @@ type ScanBlock struct { |
| ```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 |
| + Schema int `json:"schema"` |
| + Account string `json:"account"` // §5.1 |
| + App string `json:"app"` |
| + NotApplicable bool `json:"not_applicable,omitempty"` // §2.2 |
| + Current *Sweep `json:"current,omitempty"` |
| + LastComplete *Sweep `json:"last_complete,omitempty"` // the audit answer |
| } |
| type Sweep struct { |
| StartedAt time.Time `json:"started_at"` |
| At time.Time `json:"at"` |
| Engine string `json:"engine"` |
| - Signature string `json:"signature,omitempty"` // §3 Provenance |
| + Signature string `json:"signature,omitempty"` |
| Objects int64 `json:"objects"` |
| Scanned int64 `json:"scanned"` |
| Infected int64 `json:"infected"` |
| @@ -273,272 +338,320 @@ type Sweep struct { |
| Failed int64 `json:"failed"` |
| Unscanned int64 `json:"unscanned"` |
| Bytes int64 `json:"bytes"` |
| - After string `json:"after,omitempty"` // resume key |
| + After string `json:"after,omitempty"` |
| } |
| ``` |
| -**`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. |
| +`Current` and `LastComplete` are separate because draft 2's single `*Sweep` |
| +overloaded `After == ""` to mean both "complete" and "start fresh", so the |
| +tick after a sweep finished overwrote the audit answer with page-one partials. |
| +A sweep accumulates in `Current`; on its last page it moves to `LastComplete` |
| +and `Current` clears. A completed sweep does not restart before |
| +`CARLOS_STORE_SCAN_INTERVAL` (default 24h), so `LastComplete` stays stable |
| +long enough to quote. |
| -## 6. Who turns it on |
| +`Schema` is refused when unknown, the rule `ReadAppTombstone` already applies |
| +(`trash.go:76-80`). |
| -Three actors, mirroring the store's own shape. **(decision, accepted)** |
| +### 5.1 Why the sidecar carries its own account |
| -- **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. |
| +`accounts migrate` copies every key under the source prefix |
| +(`accounts.go:587-597`), so a sidecar would silently follow an app to a new |
| +account carrying rollups for a bucket named after the old one. The reader |
| +refuses a body whose `Account`/`App` disagree with its path, exactly as |
| +`AppTombstone` does. |
| -`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`. |
| +(`store.json` has the same latent problem — its bucket and IAM user are named |
| +after the original account — but that predates this spec and is noted, not |
| +fixed here.) |
| -**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. |
| +### 5.2 The PARSER LIST hazard |
| + |
| +`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:214-217` ignores unknown keys), |
| +so `scanning` is fine, but those two names must not move. The full consumer |
| +list for this shape is `objstore.Record`, `PurgeApp`'s local struct, and |
| +`handleAPIStoreStatus`'s map (`apistore.go:130-141`). §13 adds the additive |
| +subtest, which `TestPurgeGuardSeesOurRecord` does not cover today. |
| + |
| +## 6. Who turns it on |
| + |
| +- **The member requests** — `carlos store scan request --plaintext` or |
| + `--client-encrypted` (§2.2). Records the ask and the attestation, spends |
| + nothing. |
| +- **An operator grants** — `carlos store scan grant`. Calls the cloud account |
| + and starts a meter, so it sits behind `CARLOS_STORE_OPERATORS`, the 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 and `grant`/`sweep` 503 |
| +naming the variable. The **member's** request door still records, following |
| +the 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. |
| + |
| +**`revoke` and `enforce off` never 503**, and this is draft 4's fix for a |
| +deadlock. Draft 3 had §8 refuse a repair when the scanner was nil and |
| +`Enforce` was set, while §6 had the operator doors 503 without an engine — so |
| +unsetting `CARLOS_STORE_SCANNER` on a deployment with any enforcing record |
| +left the store unrepairable and the deny unclearable without hand-editing the |
| +bucket. Both are **record-only doors that work with no engine built**, which |
| +is the whole point of keeping the `DenyTag` in the record. |
| ## 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. |
| +1. Read `store.json`. No `scanning.enabled`, or `Requested.Encrypted` — write |
| + the sidecar `not_applicable` if needed and return. Skip trashed apps |
| + (`deleted.json`, `trash.go:34-36`): a trashed store is destroyed within 30 |
| + days and scanning it bills for data on its way out. |
| +2. Read `scan.json`; resume from `Current.After` or start fresh. |
| 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`. |
| + `ContinuationToken`, whose cross-process lifetime is unspecified and, on |
| + Ceph, unmeasured. A key is a portable resume point. The page gives `Size` |
| + and `ETag` for free, which §3.1 and §4.1 both need. |
| +4. For each key, `Verdict(key, etag)`; count it; `Scan` when unknown and count |
| + the verdict it returns. |
| +5. **Re-read `store.json`, then write the sidecar** (§7.2), setting `After` to |
| + the page's last key. On the final page move `Current` to `LastComplete`. |
| **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. |
| +reconciliation. |
| + |
| +### 7.1 The driver and its budget |
| + |
| +Its own goroutine on the `StartInfraPass` shape (`infrapass.go:49`), started |
| +from `serve()` in `cmd/console/main.go` alongside `StartEconomicsPass` and |
| +`StartInfraPass` (`main.go:326-330`) — **never from `NewServer`**, the rule |
| +`economicspass.go:35-38` states. Not inside `economicsPass`, whose single |
| +goroutine already runs census, statements and billing and already worries |
| +about a slow step delaying the others. |
| + |
| +Draft 2 wanted to ride `storagePass`; that was wrong. `storagePass` returns at |
| +`economicspass.go:137-140` once the day's object exists, so it iterates once |
| +per UTC day. |
| + |
| +**Enumeration.** The pass finds enabled stores the way `storagePass` does — |
| +`ListDirs("apps/")`, per-account `ListDirs`, per-app `objstore.Get` |
| +(`economicspass.go:150-181`). Hourly, that is 24× that pass's listing cost. |
| +Accepted for v1 and named here so it is a known cost rather than a surprise; |
| +an index is the fix if it bites. |
| + |
| +**Budget, with the arithmetic stated.** `CARLOS_STORE_SCAN_BUDGET` bounds work |
| +per app per tick by time (default 60s). An hourly tick at 60s is 24 minutes of |
| +scanning per app per day; at a few hundred objects a minute over INSTREAM, |
| +a million-object bucket takes months, not days. **So the default is honest |
| +only for small and medium stores**, and `scan status` prints the projected |
| +completion whenever a sweep spans more than one tick. A deployment with large |
| +stores raises the budget or shortens the tick, and §1's T widens accordingly. |
| +Apps are swept sequentially in the one goroutine; when N apps × budget exceeds |
| +the interval the ticker simply runs back to back, which is correct behaviour |
| +and not an error. |
| + |
| +**Two drivers, one recorder.** The `scan sweep` verb and the ticker can both |
| +list a page before either writes. For ClamAV that costs a duplicate fetch; for |
| +GuardDuty on-demand it is double-billed. The guarantee is **at most one driver |
| +records**, not that two never run — draft 3 overstated it. |
| + |
| +### 7.2 The pre-write re-read |
| + |
| +Before every sidecar write, re-read `store.json` and stand down if it is gone |
| +or `scanning.enabled` is nil. |
| + |
| +This closes a hole the retry guard alone does not. A **fresh** sweep's first |
| +write is create-only by construction — no sidecar exists, etag `""`. Sequence: |
| +the sweep reads an enabled `store.json`, lists a page over some seconds, |
| +`Teardown` runs and `PurgeApp` deletes the whole prefix |
| +(`trash.go:229-238`), and the sweep then writes `scan.json` create-only, which |
| +succeeds. The result is an orphan object under a purged app, and that orphan |
| +is expensive: the app-name claim gate refuses a name whose prefix holds any |
| +key (`server.go:2876-2890`), rename is refused (`appsettings.go:468-479`), and |
| +box-side auto-purge never reaches it because the tombstone went last. |
| + |
| +A one-round-trip tolerance is the same one `deleteRecordIfUnchanged` |
| +(`walk.go:519-540`) already accepts. |
| + |
| +**The retry guard still applies to replaces:** on a lost CAS, continue only if |
| +`Current` still exists and its `After` matches where this page started; |
| +otherwise stand down. A conditional write against a deleted key reads as |
| +`ErrCASConflict` (`release/store.go:521-534`; `DirStore` matches at `:260`), |
| +so a blind retry would write create-only and resurrect the sidecar. |
| ### 7.3 Teardown disables first |
| -`Teardown` (`walk.go:390-496`) empties the bucket, deletes it, deletes the |
| -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` (`walk.go:390-538`) empties the bucket, deletes it, deletes the |
| +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 |
| +`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:827-833`) tests only `StoreProvisioner` 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. |
| +destroy" flash. Both 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 non-S3 bucket. Refused at boot, where it is one loud failure rather |
| - than a per-app mystery. |
| +Both cases refuse rather than guess. A record naming another engine: the sweep |
| +refuses for that app, because verdicts in the bucket were written under a tag |
| +key the current engine does not read, and recounting them as unscanned would |
| +destroy the coverage history — an operator revokes and re-grants deliberately. |
| +Engine `guardduty` with `Deployment.Endpoint != ""`: refused at boot, since |
| +GuardDuty cannot protect a non-S3 bucket. |
| ## 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. |
| +`ScanBlock.Enforce` — nil by default — adds a Deny statement: deny |
| +`s3:GetObject` where `s3:ExistingObjectTag/<Key>` equals `<Value>`. It stores |
| +the tag rather than a bool because `Grant` (`walk.go:133`) takes a |
| +`Provisioner` and has no `Scanner` to ask. |
| + |
| +**Grant writes the deny from the record whatever the scanner is.** A nil |
| +scanner never blocks a repair and never silently drops the deny — the record |
| +holds everything the policy writer needs, which is why it holds a tag. |
| -**How the policy actually gets rewritten — the bug draft 1 and 2 both had.** |
| +**How the policy gets rewritten — the bug drafts 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 |
| +from one line (`walk.go:184`), and `Grant` returns early before it for any |
| +healthy store (`walk.go:169-177`). `Rotate` never writes a policy. A flag flip |
| +would have been permanently inert. |
| + |
| +**This repo already fixed this exact bug once.** `internal/mail/walk.go:653-671` |
| +("C2 — review follow-up") hoisted `EnsureUser` above the idempotence |
| +short-circuit, with a comment recording 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. |
| +- `Grant` calls it **before** the short-circuit, passing the record's |
| + `Enforce`. It needs only the bucket ARN string, resolved at `walk.go:143-151` |
| + before the guard, so `EnsureBucket` stays behind the guard and the |
| + record-before-create ordering survives (the grant block Put at |
| + `walk.go:160-165` still precedes it). |
| +- `scan enforce on|off` and `scan revoke` write the record and re-run the |
| + grant walk. One convergence path, no second policy writer. Clearing the |
| + record alone would leave the deny live until something re-walked. |
| + |
| +This makes every `store grant` re-run do three IAM calls on a healthy store |
| +where it previously did none. That is the intended trade and the plan should |
| +expect it. |
| + |
| +`policyStatement` (`aws.go:112-116`) has no `Condition` field today; it gains |
| +one, marshalled `omitempty`, so a nil deny still produces today's document |
| +byte for byte. `MemProvisioner` mirrors the signature and gains policy state, |
| +which `MemUser` lacks (`memprovisioner.go:12-15`). |
| + |
| +**Anti-forgery, stated at its real strength.** AWS's documented tag-based |
| +access control denies reads *unless* the tag says clean, so unscanned objects |
| +are blocked too. **This design denies only known-infected objects; unscanned |
| +ones stay readable.** That is the right trade under §1's ruling and it must be |
| +said, not implied, to anyone quoting this in a compliance answer. It is safe |
| +only because the app user has no `s3:PutObjectTagging` (`aws.go:130`) and so |
| +cannot mark its own object clean — **that must never be added to the Allow |
| +statement.** The app gains `s3:GetObjectTagging` only, so it can read its own |
| +verdicts; the console gains `GetObject`, `GetObjectTagging` and |
| +`PutObjectTagging` (§10). |
| + |
| +For a `--client-encrypted` store, `enforce` is rejected: nothing will ever be |
| +tagged infected, so it would be theatre. |
| ## 9. Surface |
| | command | actor | effect | |
| |---|---|---| |
| -| `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 request --plaintext\|--client-encrypted` | member | records the ask and the §2.2 attestation | |
| +| `carlos store scan status` | member | the §1 sentences, engine, signature, projection | |
| | `carlos store scan grant` | operator | enables the engine on the bucket | |
| -| `carlos store scan revoke` | operator | disables it; clears `Enforce` | |
| +| `carlos store scan revoke` | operator | disables it, clears `Enforce`, re-walks | |
| | `carlos store scan sweep` | operator | advances a sweep by one budget | |
| -| `carlos store scan enforce on\|off` | operator | flips the §8 Deny | |
| +| `carlos store scan enforce on\|off` | operator | flips the §8 Deny, re-walks | |
| -`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`. |
| +`request`/`grant` mirrors `create`/`grant` on the store itself. 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. |
| +switch (`cliscan.go:22`) and cannot see sub-verbs, so this is a house-rule |
| +obligation. |
| ## 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 |
| +- **clamd**, per console box: a systemd unit, `freshclam` on a timer, a unix |
| + socket the console's unit user can reach (group `clamav`), |
| + `StreamMaxLength` matched to `CARLOS_CLAMD_MAX_BYTES`, and ~1–1.5 GB RAM for |
| + resident signatures. This is the primary path and all a self-hoster needs |
| + beyond the store primitive itself. |
| +- **Console S3 permissions**: `GetObject`, `GetObjectTagging`, |
| + `PutObjectTagging` on `arn:aws:s3:::<BucketPrefix>*`, alongside the listing |
| + it already does. |
| +- **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 |
| + 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. |
| + `arn:aws:s3:::<BucketPrefix>*` so it needs no per-app edit. |
| ## 11. Gates and risks |
| -**Gate — measure before building the GuardDuty engine.** Whether |
| +**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. |
| +(§4.2). Blocks GuardDuty only. |
| **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. |
| +- **Hetzner object tagging.** `s3compat`'s quirks table exists because Ceph |
| + RGW already differed from S3 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. |
| + fallback is per-object verdicts in the sidecar keyed by key and ETag. |
| +- **clamd throughput.** One connection per object with a size ceiling sets the |
| + real budget in §7.1. Measure before promising any completion window, and |
| + revisit `IDSESSION` if per-connection cost dominates. |
| +- **Enumeration cost** at hourly cadence (§7.1). |
| ## 12. Out of scope |
| - Scanning the deployment's control-plane bucket, release artifacts, or |
| instance replicas. App stores only. |
| +- A non-AWS store provisioner (§2.1) — the dependency for a fully self-hosted |
| + story, not delivered here. |
| - 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. |
| -- Notifying members of an infected object. Visible in `scan status`; a push |
| - path can come later. |
| +- Notifying members of an infected object. |
| ## 13. Testing |
| -- **`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. |
| +- **`Scanner` contract test** over `MemScanner` and each real engine: enable |
| + twice, disable twice, scan twice, verdict on an absent object, verdict on a |
| + changed ETag. 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. |
| +- **ETag binding.** An object rewritten after being tagged reads |
| + `VerdictUnknown` and is rescanned (§3.1). |
| +- **Synchronous counting.** A ClamAV sweep's *first* pass reports objects as |
| + scanned, not unscanned. |
| +- **`LastComplete` survives** the tick after completion — draft 2's bug. |
| +- **Resurrection, both paths.** Fresh sweep with no sidecar, `Teardown` plus |
| + `PurgeApp` between list and write: no `scan.json` remains and the app name |
| + is claimable. And the retry path on a replace. |
| +- **Migration.** A sidecar whose `Account` disagrees with its path is refused. |
| - **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; 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. |
| + `PurgeApp` refuse on `grant`/`provisioned` and ignore the new key. |
| +- **Policy round-trip.** `EnsureUser` with `deny == nil` produces today's |
| + two-statement document byte for byte; with a `DenyTag`, three statements, |
| + a `Condition`, and no `PutObjectTagging`. |
| +- **Enforcement converges** on a healthy provisioned store, and `enforce off` |
| + works with no engine built (§6's deadlock, asserted). |
| +- **Teardown** disables first, deletes the sidecar, refuses on a nil scanner. |
| +- **Encrypted attestation.** A `--client-encrypted` store is swept |
| + `not_applicable`, `scan status` refuses §1's first sentence, `enforce` is |
| + rejected. |
| +- **clamd down** produces a pass error and zero written verdicts. |
| +- **Skipped is not clean**, and an over-ceiling object is tagged without a |
| + `GetObject`. |
| +- **Trashed apps are skipped.** |
| +- **Gate order and inert deployment.** `grant`/`sweep` 503 without an engine; |
| + `request`, `revoke` and `enforce off` still work. |