amadan
spec: draft 2 — close review 1
Three blockers, two false claims of my own, and one recorded decision I
reversed while asserting I had not.
The role risk dissolved. Draft 1 called it the document's biggest
unknown and proposed a transitional Allow(Service, Instance). Both were
wrong: named service credentials already carry any role svcrole.Parse
accepts, so adding Service to the catalogue makes it mintable with no
further change and nothing migrates. The fallback would also have failed
TestInstanceRoleMatchesTodaysAppTokenReach, which exists precisely to
stop the legacy role widening — I proposed the thing the codebase has a
test to prevent. Deleted, and replaced with the actual mint path.
The queue contradicted itself. Items under the app prefix AND a drain
that avoids full enumeration are incompatible: S3 cannot list
*/scan-queue/*. Resolved by keeping items in the prefix — a top-level
queue survives PurgeApp and is invisible to the name-claim gate — and
adding an in-memory index of apps with pending work, rebuilt by one
enumeration at boot. Losing it costs latency, never items.
The drain cannot share the sweep's goroutine. scanPass sweeps apps
sequentially for a full budget each, so a queue phase behind it starves
for up to N x 60s. Own goroutine, own ticker.
Draft 1 retired CARLOS_STORE_SCANNER without saying what replaced it,
and six places key off it. Now stated: the Scanner builds from
CARLOS_CLAMD_ADDR, the pass loops whenever it is non-nil and reads the
record each tick, and the doors check the record. A deployment-wide door
also has no {account} segment, so it needs a person-only resolver and a
closedDoors entry or the route-equality test fails.
My own false claims, corrected. The console does not shovel bytes
through its memory — clamd.go streams 64KB chunks — and MemoryMax is on
clamd's cgroup, so the services box is a resource-placement decision
worth about 1.5GB of RSS, not a data-flow fix. And the prior spec
recorded "no TCP option in v1"; draft 1 reversed it while claiming to
contradict nothing. Now argued on its merits.
Security fixes: the callback host is tenant-influenced through custom
domains, and Go follows redirects by default, so a tenant 302 turns a
signed POST into a GET at an internal address. Platform-assigned host
only, https, redirects refused. Path validation written as a rule rather
than an intention. And the HMAC key cannot be delivered like a store
secret, because the console must hold it to sign — one master, per-app
keys derived at sign time, or one app forges callbacks to another.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 file changed,
+385
−272
docs/superpowers/specs/2026-09-02-scan-on-demand-design.md+385 −272
diff --git a/docs/superpowers/specs/2026-09-02-scan-on-demand-design.md b/docs/superpowers/specs/2026-09-02-scan-on-demand-design.md| index 59798a01e..b4d2ba51e 100644 |
| --- a/docs/superpowers/specs/2026-09-02-scan-on-demand-design.md |
| +++ b/docs/superpowers/specs/2026-09-02-scan-on-demand-design.md |
| @@ -1,57 +1,50 @@ |
| # Scan on demand: design |
| -**Status:** draft 1, for review. |
| -**Builds on:** `2026-09-01-object-storage-virus-scanning-design.md` (the sweep, |
| -the `Scanner` seam, verdict tags, enforcement). That work is landed on |
| -`worktree-store-virus-scanning` and reviewed; nothing here contradicts it. |
| +**Status:** draft 2 (closes review 1). |
| +**Builds on:** `2026-09-01-object-storage-virus-scanning-design.md` — the sweep, |
| +the `Scanner` seam, verdict tags, enforcement. Landed and reviewed. |
| + |
| +**It contradicts that spec in one place, deliberately.** Prior spec §4.1 |
| +records: *"No TCP option in v1: an unauthenticated scanning service reachable |
| +over a network is a worse problem than the one being solved. **(decision)**"*. |
| +§9 here reverses it. The reversal is argued there rather than assumed. |
| --- |
| ## 1. What is being asked |
| The shipped scanner answers "is this bucket clean?" on an hourly cycle. It does |
| -not answer "is *this object* clean, now?" — and that is the question an app |
| -actually has when someone has just uploaded a file it is about to serve. |
| - |
| -The current documentation tells app authors to build that guarantee |
| -themselves. That is the platform declining work it is far better placed to do. |
| +not answer "is *this object* clean, now?" — the question an app has when |
| +someone has just uploaded a file it is about to serve. The current docs tell |
| +app authors to solve that themselves, which is the platform declining work it |
| +is better placed to do. |
| **The delay is scheduling, not scanning.** `clamd` answers on a typical upload |
| -in well under a second. An hour of latency is entirely "the pass has not come |
| -round yet". So this is a queueing and notification problem. |
| +in well under a second. So this is queueing and notification. |
| -### 1.1 What can honestly be claimed afterwards |
| +### 1.1 What can honestly be claimed |
| > Object `<key>` was scanned at `<T>` against `<engine>/<signature-db>` and |
| > found clean. |
| -and, unchanged from the sweep design, the second sentence that makes the first |
| -one worth anything: |
| +plus the sentence that makes the first one worth anything, unchanged: |
| > `N` objects in this store carry no verdict. |
| -On-demand scanning shortens T. It does not change what a verdict means, and it |
| -must not be allowed to imply that scanning is now synchronous with upload. It |
| -is not: **CARLOS never sees the PUT**, and no design here changes that. |
| +On-demand scanning shortens T. It does not make scanning synchronous with |
| +upload: **CARLOS never sees the PUT**, and nothing here changes that. |
| --- |
| ## 2. Scope |
| -In: |
| - |
| -- a **new service role** for apps calling platform services at runtime |
| -- `carlos system antivirus` — deployment-level operator control, replacing the |
| - on/off half of `CARLOS_STORE_SCANNER` |
| -- a **priority scan door** and a **status door** with long-poll |
| -- a **queue** that survives restarts and cannot be starved by one tenant |
| -- an opt-in **signed webhook** |
| -- **metering** of on-demand scans, recorded and unpriced |
| -- **clamd over TCP** on a services box, rather than a unix socket on the |
| - console box |
| +In: a new service role; `carlos system antivirus`; a priority scan door and a |
| +long-polling status door; a queue that survives restarts and cannot be starved |
| +by one tenant; an opt-in signed webhook; metering, recorded and unpriced; |
| +clamd over TCP on a services box. |
| -Out (§12 has the reasoning): synchronous scan-on-PUT, GuardDuty, Gotenberg as |
| -a capability, pricing. |
| +Out (§12): synchronous scan on PUT, GuardDuty, Gotenberg as a capability, |
| +pricing. |
| --- |
| @@ -59,406 +52,526 @@ a capability, pricing. |
| ### 3.1 Why not `Instance` |
| -`svcrole.Instance` is the app-token authority today and already covers runtime |
| -reads — `apifeatures.go` has the app's own instance credential reading |
| -`features.json`, and never writing. So an app calling the console at runtime is |
| -not new. |
| +`svcrole.go:34-36` describes `Instance` as *"the LEGACY app-token authority, |
| +defined by observation of today's call sites rather than by its name."* A role |
| +defined by what happens to call it is not one to hang a new API class on, and a |
| +token minted to read logs must not silently gain the ability to spend |
| +deployment CPU. |
| -But `svcrole.go:35` describes `Instance` as *"the LEGACY app-token authority, |
| -defined by observation of today's call sites rather than by its name — it |
| -includes domains and logs."* A role defined by what happens to call it is not a |
| -role to hang a new class of API on. Widening it further makes it less |
| -describable, not more useful, and a token issued for log-reading would silently |
| -gain the ability to spend deployment CPU. |
| +This is enforced, not merely intended: |
| +`TestInstanceRoleMatchesTodaysAppTokenReach` (`svcroutes_test.go:262-300`) pins |
| +`Instance` to exactly nine doors and fails with "widening the legacy role". |
| ### 3.2 `svcrole.Service` |
| A fifth role: **an app, at runtime, asking a platform service to do work on its |
| -behalf.** |
| +behalf** — work the platform performs on the app's own data that costs the |
| +deployment resources. Scanning is the first; PDF rendering is the next. |
| ```go |
| -// Service is an app calling a platform SERVICE at runtime — work the |
| -// platform performs for the app, on the app's own data, that costs the |
| -// deployment resources. Scanning an object is the first; PDF rendering is |
| -// the next. It is deliberately NOT Instance: that role is defined by |
| -// observation of its call sites, and a token minted to read logs must not |
| -// silently gain the ability to spend the deployment's CPU. |
| Service Role = "service" |
| ``` |
| -`Admin` continues to be the union of everything, so existing CLI tokens keep |
| -working unchanged. |
| +Also update `svcrole.go:41-44`, which describes `Admin` as "the union of the |
| +three above". |
| -**What it is not.** Not a tenant-data role — it grants no read of objects, no |
| -config, no credentials. Every `Service` door acts on the app named in its own |
| -path and nothing else. |
| +### 3.3 There is no migration — this replaced draft 1's §3.3 |
| -### 3.3 What still needs checking during planning |
| +Draft 1 flagged this as the document's biggest risk and proposed a |
| +transitional `Allow(Service, Instance)`. Both the risk and the fallback were |
| +wrong. How credentials actually work: |
| -How tokens are minted and which roles an app's delivered credential actually |
| -carries. `Instance` tokens exist and reach `apifeatures.go`; whether a token |
| -can be minted carrying `Service`, and where that happens, is a code question |
| -this spec does not pretend to have answered. **If it turns out apps cannot be |
| -issued a new role without a credential migration, that changes the plan** — the |
| -fallback is `Allow(Service, Instance)` on the new doors for one release, with |
| -`Instance` removed once credentials have rotated. |
| +| kind | minted by | roles | rotatable | |
| +|---|---|---|---| |
| +| person (`kind=""`) | `carlos auth login` | none — never meets the role gate (`authcli.go:425`) | n/a | |
| +| legacy app token (`kind="app"`) | a person, `POST .../instance-token` | `Instance`, **hardcoded** at `svcprincipal.go:124` | **no** (`svcprincipal.go:244-246`) | |
| +| named service (`kind="service"`) | account **owner**, `POST /api/cli/services/{account}` | any role `svcrole.Parse`s (`apiservices.go:180`) | yes | |
| ---- |
| +**Adding `Service` to the catalogue makes it mintable with no further change** |
| +— the mint door, the mint form (`svcpage.go:176`) and `doorsFor` all read |
| +`svcrole.All()`. |
| -## 4. `carlos system antivirus` |
| +Nothing migrates. A legacy `kind='app'` token can never become `Service`, so an |
| +app that wants on-demand scanning is issued a *new* credential: |
| -### 4.1 The problem with the env var |
| +``` |
| +carlos services create -account <a> -app <app> -role service |
| +carlos secrets set -app <app> CARLOS_SCAN_TOKEN=<...> |
| +``` |
| -`CARLOS_STORE_SCANNER` conflates two things: **wiring** (there is a clamd, it |
| -is at this address) and **policy** (scanning is on for this deployment). |
| +which is the same delivery step it already performed for its instance token. |
| +Per-app, opt-in, one command. **The `Allow(Service, Instance)` fallback is |
| +deleted**: it cannot terminate (legacy tokens do not rotate into anything) and |
| +it would fail the pinned test above — the exact widening §3.1 argues against. |
| -Wiring belongs in the unit file. Policy does not: turning a compliance control |
| -on should not require editing a systemd unit and restarting the control plane, |
| -and today the only way to know whether scanning is healthy is to log into the |
| -box — which AGENTS.md's no-infra-access rule says it must not be. |
| +### 3.4 Scope must be per-app at mint time |
| -### 4.2 The split |
| +`NewServicePrincipal` permits `app == ""` (`svcprincipal.go:140-141`), and |
| +`apiAccountAppBy` binds only when `p.App != ""` (`apicli.go:70`). An |
| +account-scoped credential therefore reaches every app in the account. |
| -| what | where | why | |
| -|---|---|---| |
| -| `CARLOS_CLAMD_ADDR` | env, boot | coordinates. Same argument as `CARLOS_STORE_REGION`: a console that guessed would scan against the wrong engine | |
| -| `CARLOS_CLAMD_MAX_BYTES`, timeouts | env, boot | must match the engine's own config | |
| -| enabled / disabled | **record** | policy, changeable without a restart | |
| -| engine name, enabled at, by whom | **record** | audit | |
| +**The mint door refuses account scope for role `Service`.** Without that, |
| +§11.4 is false. |
| -`CARLOS_STORE_SCANNER` is **retired**. Its on/off meaning moves to the record; |
| -its "which engine" meaning is implied by the address. |
| +--- |
| -### 4.3 The record, and the reusable pattern |
| +## 4. `carlos system antivirus` |
| -`system/antivirus.json`, a sibling of `infra/inventory.json` |
| -(`infraview.go:25`). Deployment-scoped, not per-account. |
| +### 4.1 Wiring versus policy |
| + |
| +`CARLOS_STORE_SCANNER` conflates *there is a clamd at this address* with |
| +*scanning is on*. Wiring belongs in the unit file. Policy does not: turning a |
| +compliance control on should not mean editing a systemd unit and restarting the |
| +control plane, and today the only way to know whether scanning is healthy is to |
| +log into the box — which the no-infra-access rule says it must not be. |
| + |
| +| what | where | |
| +|---|---| |
| +| `CARLOS_CLAMD_ADDR` (`host:port` or a socket path) | env, boot | |
| +| `CARLOS_CLAMD_MAX_BYTES`, timeouts | env, boot | |
| +| `CARLOS_SCAN_WEBHOOK_KEY` (§6.3) | env, boot | |
| +| enabled / disabled, engine, at, by | **record** | |
| + |
| +### 4.2 What replaces the env var, concretely |
| + |
| +Draft 1 retired `CARLOS_STORE_SCANNER` without saying what took over. Six |
| +places key off it. All of them change: |
| + |
| +1. `storeScannerFor` (`main.go:755-758`) builds the Scanner whenever |
| + **`CARLOS_CLAMD_ADDR`** is set. This is safe because `NewClamAVScanner` does |
| + not ping (`clamav.go:56-70`) — a Scanner can be constructed from wiring |
| + alone. The `main.go:772` fatal when no store backend is configured stays. |
| +2. `StartScanPass` (`scanpass.go:37-39`) loops whenever `Scanner != nil`, and |
| + **reads `system/antivirus.json` at the top of each tick**. Disabled means |
| + the tick does nothing — not that there is no goroutine. That is what makes |
| + `enable` restartless. |
| +3. `scan grant` and `scan sweep` check the **record**, not the nil-ness of the |
| + Scanner, and 503 naming `carlos system antivirus enable`. |
| +4. The `apiscan.go:14-20` rule is preserved verbatim for the disabled state: |
| + `request`, `revoke` and `enforce off` must keep working when scanning is |
| + off, or disabling strands every enforcing record. |
| +5. `RunSweep`'s refusal (`sweep.go:163`) becomes a check against the record. |
| +6. Error strings naming `CARLOS_STORE_SCANNER` (`apiscan.go:45`) name the |
| + command instead. |
| + |
| +### 4.3 The record |
| + |
| +`system/antivirus.json` — deployment-scoped, sibling in spirit to |
| +`infraview.InventoryKey` (`internal/infraview/infraview.go:25`), but the helper |
| +lives beside `release.FeaturesKey` (`internal/release/features.go:53`): |
| ```go |
| // package release |
| -// SystemKey is a deployment-wide settings record: system/<name>.json. |
| -// Distinct from features.json, which is per-app and which the platform |
| -// stores WITHOUT interpreting (apifeatures.go). These the platform reads |
| -// and acts on, so each has a typed shape and a pinned schema. |
| func SystemKey(name string) string { return "system/" + name + ".json" } |
| -``` |
| -```go |
| type AntivirusSettings struct { |
| - Schema int `json:"schema"` // pinned; unknown refuses |
| + Schema int `json:"schema"` |
| Enabled bool `json:"enabled"` |
| - Engine string `json:"engine"` // "clamav" |
| + Engine string `json:"engine"` |
| At time.Time `json:"at"` |
| By string `json:"by"` |
| } |
| ``` |
| -Read with CAS, written with CAS, schema pinned and an unknown schema refused — |
| -the rules `objstore.Record` and `ScanSidecar` already follow, for the same |
| -reason: a future shape could mean anything, and this one drives whether tenant |
| -bytes get read. |
| +CAS read/write, schema pinned, unknown schema refuses — the rules |
| +`objstore.Record` and `ScanSidecar` follow, for the same reason: this one |
| +drives whether tenant bytes get read. |
| -**Why a typed record per feature rather than one generic settings map.** A map |
| -is a features.json by another name — untyped, uninterpreted, and impossible to |
| -schema-pin meaningfully. Each `system/<name>.json` gets a struct, so adding |
| +**Typed record per feature, not a generic settings map.** A map is |
| +`features.json` by another name — uninterpreted and impossible to schema-pin. |
| `system/pdf.json` later is a new type and a new door, not a new magic string. |
| -### 4.4 The doors |
| +### 4.4 The doors, and the authority problem |
| ``` |
| -carlos system antivirus status # engine, reachable?, signature age, queue depth |
| -carlos system antivirus enable |
| -carlos system antivirus disable |
| +carlos system antivirus status|enable|disable |
| ``` |
| -Operator authority — `CARLOS_STORE_OPERATORS`, the list that already governs |
| -`store grant`. Not `Admin` alone: enabling this starts reading tenant bytes |
| -across the deployment, which is a deployment-owner decision, not any admin's. |
| +**A deployment-wide door has no `{account}` segment**, so |
| +`apiPrincipalAccount` (`apicli.go:176-187`) cannot resolve it. These need a |
| +person-only resolver, registered with `svcrole.Set{}` (no service principal may |
| +reach them) and added to `closedDoors` or `svcroutes_test.go:83+` fails on the |
| +equality check. |
| -`status` is the answer to "is scanning healthy" without SSH. It reports the |
| -engine version, whether it answers at all, how old the signature database is, |
| -and how deep the queue is. |
| +Authority is then `storeOperator(addr)` — the `CARLOS_STORE_OPERATORS` address |
| +list that already governs `store grant` (`apistore.go:520`) and every scan |
| +operator verb (`apiscan.go:245, 333, 396, 514`). Draft 1 said "not `Admin` |
| +alone", which confused a service role with person authority: operators are |
| +addresses (`server.go:558-571`) and a service principal's actor is |
| +`service:…`/`app:…` (`apicli.go:89-100`), which can never match one. |
| -### 4.5 Two layers, deliberately |
| +`status` answers "is scanning healthy" without SSH: engine version, whether it |
| +answers at all, signature age, queue depth. |
| -Deployment-level enable does **not** start scanning anything. Per-app |
| -`store scan request` (member) and `store scan grant` (operator) remain exactly |
| -as shipped. The layers answer different questions: |
| +### 4.5 Two layers |
| -- `system antivirus` — does this deployment have scanning at all? |
| -- `store scan grant` — is it on for this store? |
| +Deployment enable does not scan anything. Per-app `store scan request` (member) |
| +and `store scan grant` (operator) are unchanged. Collapsing the layers would |
| +either scan every bucket the moment an operator enabled the engine, or make the |
| +deployment switch meaningless. |
| -Collapsing them would either scan every bucket the moment an operator enabled |
| -the engine, or make the deployment switch meaningless. |
| +**Per-app `store scan status` must surface the deployment state.** Otherwise a |
| +record saying `enabled: clamav` reads as healthy while the deployment switch is |
| +off and nothing runs — a new way for the status output to lie, which is the one |
| +thing the shipped design was careful about. |
| --- |
| ## 5. The pull path |
| -### 5.1 Request a scan |
| +### 5.1 Request |
| ``` |
| POST /api/cli/apps/{account}/{app}/store/scan/object |
| {"key": "uploads/x.pdf"} |
| -→ 202 {"state": "pending"} |
| +→ 202 {"state":"pending"} |
| +→ 503 {"state":"unavailable","reason":"disabled"} |
| ``` |
| -Role: `Service`. Enqueues; does not scan inline. A door that scanned inline |
| -would hold a console goroutine for the duration and give one tenant a trivial |
| -way to occupy them all. |
| +Role `Service`. It enqueues; it does not scan inline. Inline scanning would |
| +hold a console goroutine for the duration and hand one tenant a trivial way to |
| +occupy them all. |
| -### 5.2 Read the verdict |
| +**Enqueue refuses while scanning is disabled**, or items accumulate forever |
| +against a drain that will never run. |
| + |
| +### 5.2 Read |
| ``` |
| -GET /api/cli/apps/{account}/{app}/store/scan/object?key=uploads/x.pdf&wait=15s |
| -→ {"state":"clean", "at":"...", "engine":"clamav", "signature":"..."} |
| -→ {"state":"infected", "signature":"Eicar-Test"} |
| -→ {"state":"skipped", "reason":"too_large"} |
| +GET .../store/scan/object?key=uploads/x.pdf&wait=15s |
| +→ {"state":"clean","at":"...","engine":"clamav","signature":"..."} |
| +→ {"state":"infected","signature":"Eicar-Test"} |
| +→ {"state":"skipped","reason":"too_large"} |
| → {"state":"pending"} |
| -→ {"state":"unavailable", "reason":"no_engine"} |
| +→ {"state":"unavailable","reason":"no_engine"|"disabled"} |
| ``` |
| -**The console does the version comparison.** The verdict tag records |
| -`etag@last-modified` (§3.1 of the sweep spec). An app must not be asked to |
| -reconstruct that: it is an internal format, and an app comparing it wrongly |
| -gets the *laundering bug as a correctness bug* — upload v2 over a key whose v1 |
| -was tagged clean, read the stale tag, serve the payload. So the door |
| -HEADs the object, compares, and answers `pending` when the tag is absent **or** |
| +**The console does the version comparison.** The tag records `etag@RFC3339` |
| +(`sweep.go:52-54`). An app must not reconstruct that: it is an internal format, |
| +and an app comparing it wrongly gets the laundering bug *as a correctness bug* |
| +— upload v2 over a key whose v1 was tagged clean, read the stale tag, serve the |
| +payload. The door compares and answers `pending` when the tag is absent **or** |
| stale. |
| -**`wait` long-polls**, bounded (say 30s max). For the upload-then-serve flow |
| -the app is already inside a request, so this delivers push latency with none of |
| -the webhook tax. It is the primary mechanism; §6 is for when nobody is waiting. |
| +This needs `HeadObject`, which `scanS3API` (`clamav.go:26-30`) does not have. |
| +The seam widens; the permission is already there. |
| + |
| +**`wait` long-polls**, capped at 30s. Waking is **in-process notification from |
| +the drain**, not per-waiter polling of S3 — a polling waiter costs one |
| +`GetObjectTagging` per second per waiter. Cap concurrent waiters **globally** |
| +as well as per app; otherwise long-poll is a cheap way to occupy the console. |
| -**`skipped` and `unavailable` are not `clean`.** An app that treats them as |
| -success reproduces exactly the dishonesty the two-number rule exists to |
| -prevent, so they are distinct states with reasons attached rather than an |
| -error or an empty verdict. |
| +**`skipped` and `unavailable` are not `clean`.** Distinct states with reasons, |
| +because an app treating them as success reproduces exactly the dishonesty the |
| +two-number rule exists to prevent. |
| ### 5.3 Pull is the source of truth |
| -The tag on the object is the record. The status door reads it. Webhooks are an |
| -accelerator over this, never a replacement — see §6.4. |
| +The tag on the object is the record; the status door reads it. Webhooks |
| +accelerate this and never replace it (§6.4). |
| --- |
| ## 6. The push path |
| -### 6.1 Why it is needed at all |
| +### 6.1 Why it is needed |
| -Long-poll covers the case where the app is in a request. **Presigned |
| -direct-to-S3 uploads break that assumption**: the browser PUTs straight to the |
| -bucket and the app is never in a request at all. That is a common S3 pattern |
| -and designing without it would be a mistake. |
| +Long-poll covers the app being inside a request. **Presigned direct-to-S3 |
| +uploads break that**: the browser PUTs straight to the bucket and the app is |
| +never in a request at all. Common enough that designing without it is a |
| +mistake. |
| -### 6.2 No caller-supplied host |
| +### 6.2 Where a callback may go |
| -Gotenberg takes a full webhook URL. That is safe in its deployment model — |
| -self-hosted, caller and operator the same party. **It is not safe here**: the |
| -caller is a tenant and the fetcher is the console, a box that can reach the |
| -deployment's internal network and cloud metadata endpoint. A tenant-supplied |
| -URL is a straightforward SSRF. |
| +Draft 1 said taking a path rather than a URL meant "the parameter that creates |
| +the vulnerability does not exist". **That was overstated.** The host is still |
| +tenant-influenced: the existing outbound fetcher picks the first ready |
| +non-static route (`instances.go:86-97`), which can be a tenant's own custom |
| +domain whose DNS they control. And Go's default client follows redirects — a |
| +tenant server answering `302 → http://10.x/` converts a signed POST into a GET |
| +against an internal address. The codebase already knows this hazard |
| +(`sesevents.go:143-154` sets `CheckRedirect`). |
| -The platform already knows where the app lives. So the request carries a |
| -**path**, never a host: |
| +So, all of: |
| -```json |
| -{"key": "uploads/x.pdf", "callback_path": "/hooks/scan"} |
| +- POST only to the **platform-assigned host** (`<app>.<sqid>.<AppsDomain>`), |
| + never a custom domain |
| +- **https only** |
| +- `CheckRedirect` **refuses every redirect** |
| +- bounded timeout, `LimitReader` on the response body |
| + |
| +**Path validation, stated as a rule** rather than an intention. Refuse: |
| +absolute URLs (`https://evil/x`); protocol-relative `//evil/x` and `/\evil/x`; |
| +anything not beginning with exactly one `/`; raw or percent-encoded CR/LF; |
| +fragments; `..` segments; longer than 1KB. Then: |
| + |
| +```go |
| +u, err := url.Parse(p) // must yield Scheme=="" Host=="" User==nil Opaque=="" |
| +target := url.URL{Scheme: "https", Host: appHost, Path: u.Path, RawQuery: u.RawQuery} |
| ``` |
| -and the console POSTs to the app's own host at that path. The parameter that |
| -creates the vulnerability does not exist. |
| +Built structurally — **never by string concatenation**. |
| + |
| +### 6.3 Signing |
| -### 6.3 Signed, or it is worse than nothing |
| +An unsigned callback is a hole wearing a convenience's clothes: anyone who |
| +learns the path POSTs `{"verdict":"clean"}` and the app serves a payload |
| +believing the platform cleared it. |
| -An unsigned callback is a security hole wearing a convenience's clothes: |
| -anyone who learns the path POSTs `{"verdict":"clean"}` and the app serves a |
| -payload believing the platform cleared it. |
| +Draft 1 said the key would be delivered "the same way store credentials are". |
| +**It cannot be**: a store secret exists only sealed in the app's `config.json` |
| +and never console-side (`apistore.go:21`), whereas the console must *hold* the |
| +key in order to sign. |
| -HMAC-SHA256 over `timestamp + "." + body`, header |
| -`Carlos-Signature: t=<unix>,v1=<hex>`, key delivered to the app as config |
| -(`CARLOS_SCAN_WEBHOOK_SECRET`) the same way store credentials are. Apps reject |
| -a stale timestamp, which gives replay protection body-signing alone does not. |
| +- one boot secret, `CARLOS_SCAN_WEBHOOK_KEY` |
| +- **per-app key derived at sign time**: `HMAC(master, acct+"/"+app)`. Nothing |
| + new is stored, and per-app isolation falls out — without it a single |
| + deployment-wide secret lets app A forge callbacks to app B |
| +- the derived key is delivered sealed into the app's config |
| +- `Carlos-Signature: t=<unix>,k=<keyid>,v1=<hex>` over `t + "." + body`; |
| + `keyid` allows master rotation |
| +- the body carries account, app, key and object version, so a signature cannot |
| + be replayed onto a different object |
| +- constant-time compare; documented skew window (5 minutes) |
| -Considered and rejected: Gotenberg's trick of replaying caller-supplied |
| -headers. It is neat and it avoids a signing scheme, but it gives no body |
| -integrity and no replay protection, and it puts a tenant secret in a request |
| -log. |
| +Considered and rejected: Gotenberg's replay-my-headers trick. Neat, and it |
| +avoids a signing scheme, but it gives no body integrity, no replay protection, |
| +and it puts a tenant secret in request logs. |
| -### 6.4 Delivery is best effort, and that is fine |
| +### 6.4 Delivery is best effort |
| -Retries with exponential backoff, a small bounded number, then give up. **No |
| -dead-letter queue is needed**, because the verdict is durably on the object and |
| -the status door always answers — a dropped callback costs an app one poll, not |
| -a stuck file. That is the whole benefit of building pull first: retries can be |
| -lazy rather than heroic. |
| +Retries with exponential backoff, a small bounded count, then stop. **No |
| +dead-letter queue**: the verdict is durably on the object and the status door |
| +always answers, so a dropped callback costs one poll rather than stranding a |
| +file. That is the dividend of building pull first — retries can be lazy. |
| -Apps must treat callbacks as at-least-once and idempotent. |
| +Callbacks are at-least-once; apps must be idempotent. |
| --- |
| ## 7. The queue |
| -### 7.1 Shape |
| +### 7.1 Location, and the contradiction draft 1 shipped |
| -One object per item, under the app's own prefix: |
| +Draft 1 put items at `apps/<acct>/<app>/scan-queue/` *and* claimed the queue |
| +phase would avoid full enumeration. Those are incompatible: S3 cannot list |
| +`*/scan-queue/*`, so finding non-empty queues **is** the full |
| +`ListDirs("apps/")` walk. |
| + |
| +**Decision: keep items in the app prefix, and add an index.** |
| ``` |
| apps/<acct>/<app>/scan-queue/<ulid>.json |
| ``` |
| -**Not one queue object.** Many writers doing read-modify-write on a shared |
| -object is CAS contention — the exact problem the sidecar design avoided by |
| -having a single writer. Distinct keys never collide, so an enqueue needs no |
| -coordination at all. ULID keys sort by time, so a prefix listing is the queue |
| -in order. |
| +- the enqueue door adds `acct/app` to an **in-memory set of apps with pending |
| + items**; the drain lists only those prefixes |
| +- the set is rebuilt by **one full enumeration at boot**, so the failure mode |
| + of losing it is a single startup cost, not a lost item |
| +- items carry a **TTL**; expired ones are deleted rather than scanned |
| + |
| +The alternative — a top-level `scan-queue/<acct>/<app>/` that one `ListDirs` |
| +finds — was rejected because `PurgeApp` lists only the app prefix |
| +(`trash.go:228-235`) and the name-claim gate (`server.go:2875+`) never sees |
| +outside it. Queue items would survive app deletion and be invisible to both. |
| -It also inherits the purge machinery: the prefix goes when the app does, which |
| -is the failure the sweep's pre-write re-read exists to prevent. |
| +One object per item, not one queue object: distinct keys never collide, so an |
| +enqueue needs no coordination. ULIDs sort by time, so a prefix listing is the |
| +queue in order. |
| -### 7.2 Draining |
| +### 7.2 The drain is its own goroutine |
| -The scan pass gains a queue phase that runs **before** the sweep on each tick, |
| -and a much shorter tick (30s, say) so on-demand latency is seconds. Sweep |
| -budget is unchanged and still runs on the hourly cycle. |
| +Draft 1 put a queue phase "before the sweep on each tick" inside the existing |
| +pass. **That cannot deliver seconds of latency.** `scanPass` sweeps apps |
| +sequentially, each for the full budget (`scanpass.go:86-103`), and the prior |
| +spec accepts ticks running back to back when N apps × budget exceeds the |
| +interval. A queue phase behind that is starved for up to N × 60s. |
| + |
| +The drain gets its **own goroutine and its own ticker** (30s). It never touches |
| +the sidecar; the only state it shares with the sweep is object tags, which are |
| +idempotent per version. The sweep keeps its hourly cycle and its budget. |
| ### 7.3 Fairness |
| -Round-robin across apps with a per-tick cap per app, not FIFO across the whole |
| +Round-robin across apps with a per-tick cap per app, not FIFO across the |
| deployment. FIFO lets one tenant enqueuing ten thousand objects delay every |
| -other tenant's single urgent upload behind them. |
| +other tenant's single urgent upload. |
| + |
| +### 7.4 Amplification |
| -### 7.4 Rate limiting |
| +A rate limit alone does not close this. At `svcWriteBurst` (120/min, |
| +`authcli.go:139`) one app can enqueue ~172k items a day, each up to the size |
| +ceiling. |
| -Per-app token bucket on the request door. Exceeding it is a 429, not a silent |
| -drop — an app that is being throttled needs to know, because its own |
| -upload flow depends on the answer. |
| +- the drain calls `Verdict(version)` **before** `Scan`, exactly as `RunSweep` |
| + does (`sweep.go:227`), so a key already judged at that version costs a tag |
| + read and nothing else |
| +- items are **deduplicated by key per tick** |
| +- per-app token bucket on the door; exceeding it is a 429, not a silent drop — |
| + an app being throttled needs to know, because its upload flow depends on the |
| + answer |
| + |
| +### 7.5 The purge race |
| + |
| +"Inherits the purge machinery" inherits the hazard as well as the sweep. |
| +`sweep.go:346-355`: any object written under a purged prefix makes the app name |
| +permanently unclaimable, and an enqueue racing `PurgeApp` leaves an item |
| +behind. |
| + |
| +The drain deletes items whose `store.json` has gone, and this spec accepts the |
| +same one-round-trip race `writeSidecar` already documents. |
| --- |
| ## 8. Metering |
| -`usage.go` already carries units recorded as data and explicitly unpriced |
| -pending a price sheet. On-demand scans follow that idiom exactly: |
| +`usage.go:26-38` already carries `MeterBackhaulGB` recorded and explicitly |
| +unpriced. Scanning follows that idiom — but **not into an existing object**. |
| +`EdgeDay` is per-box (`edge.go:59-69`) and `StorageDay` is written once daily |
| +by a sweep rather than incremented (`storage.go:13-18`); neither takes |
| +increments from a door. |
| -- count **objects scanned on demand** and **bytes scanned on demand**, per |
| - account per day, in the existing daily usage object |
| -- ship unpriced |
| +- a new `usage/scan/<date>.json`, CAS-merged the way `UpsertEdgeDay` does |
| + (`edge.go:108-139`) |
| +- new meter names in `usage.go`, which are **frozen once merged** (`usage.go:4-5`) |
| +- counts objects and bytes scanned **on demand**, per account per day |
| +- ships unpriced |
| -Sweep-driven scanning is *not* counted against a tenant: they did not ask for |
| -it, the deployment operator did. Only work an app requested is attributed to |
| -it. |
| +Sweep-driven scanning is **not** attributed to a tenant: the deployment |
| +operator asked for it, not them. Only work an app requested is billed to it. |
| -Counting from day one is the point. Retrofitting attribution after apps depend |
| -on the endpoint means either guessing at history or starting the meter at zero |
| -with usage already in flight. |
| +Counting from day one is the point — retrofitting attribution after apps depend |
| +on the endpoint means guessing at history or starting the meter mid-flight. |
| --- |
| ## 9. Where clamd lives |
| -The sweep spec put `clamd` on the console box over a unix socket. That was |
| -wrong in a way worth naming: **the console is a control plane, and this made it |
| -shovel every tenant object through its own memory.** The cgroup limits in the |
| -rollout plan mitigate the symptom. |
| +### 9.1 The argument draft 1 made was wrong |
| + |
| +Draft 1 said the console "shovels every tenant object through its own memory". |
| +It does not: `clamd.go:173-175,187` streams 64KB chunks and the prior spec |
| +mandates never buffering. The `MemoryMax` in the rollout plan is on **clamd's** |
| +cgroup, protecting the console from clamd's ~1.5GB signature RSS — it is not |
| +mitigation for a byte-shovelling smell, because there is not one. |
| + |
| +### 9.2 The argument that actually holds |
| + |
| +Moving clamd to a services box: |
| + |
| +- **relocates the resident cost.** ~1.5GB of signatures and the scanning CPU |
| + leave a control-plane box, permanently, for a feature that is idle by |
| + default. That is the whole of the benefit. |
| +- **costs doubled network traffic.** The console still fetches from S3 and |
| + streams onward (§9.4), so every byte now crosses the network twice. |
| +- **does not remove tenant bytes from the console.** They still transit it. |
| + |
| +So this is a resource-placement decision, not a data-flow one. It is worth |
| +doing, and it is worth stating at that size. |
| + |
| +### 9.3 Reversing the prior decision |
| + |
| +Prior spec §4.1: *"No TCP option in v1: an unauthenticated scanning service |
| +reachable over a network is a worse problem than the one being solved."* |
| + |
| +That objection is correct and is not answered by wanting a services box. It is |
| +answered by the network: |
| + |
| +- **clamd has no authentication whatsoever.** Anything reaching the port can |
| + have it scan arbitrary bytes. |
| +- so: a private interface only (Tailscale or equivalent), **never** a public |
| + port, and the services box firewalls the port to the console's address |
| +- this is a hard constraint on the infra ask, not a recommendation |
| -`clamd` moves to a **services box**, reached over TCP. `internal/clamd` dials |
| -`unix` today; the INSTREAM framing, deadlines and reply parsing are already |
| -transport-agnostic, so this is a dialer change and a config change. |
| +`internal/clamd` has a single `"unix"` literal (`clamd.go:130`); framing, |
| +deadlines and reply parsing are transport-independent. `New` gains a network |
| +parameter. |
| -**clamd has no authentication whatsoever.** Anything that reaches the port can |
| -have it scan arbitrary bytes. So: private interface only (Tailscale or |
| -equivalent), never a public port, and the services box firewalls the port to |
| -the console. This is a hard constraint, not a recommendation — it belongs in |
| -the infra ask and in the docs. |
| +### 9.4 The console keeps fetching |
| -**The console keeps fetching from S3** and streaming to the engine, rather than |
| -the services box fetching for itself. That moves bytes twice and is less |
| -efficient, but it keeps tenant-data credentials on exactly one box. Giving the |
| -services box its own S3 read is a trade to make later with numbers. |
| +Rather than giving the services box its own S3 read and tag permissions. It |
| +moves bytes twice, and it keeps tenant-data credentials on exactly one box. |
| +Revisit with numbers, not on principle. |
| --- |
| ## 10. Surface |
| ``` |
| -carlos system antivirus status|enable|disable operator, deployment-wide |
| +carlos system antivirus status|enable|disable operator, deployment-wide |
| carlos store scan request|status member, per app (shipped) |
| carlos store scan grant|revoke|sweep|enforce operator, per app (shipped) |
| ``` |
| -App-facing HTTP (role `Service`): |
| +App-facing HTTP, role `Service`: |
| ``` |
| -POST .../store/scan/object request a scan |
| -GET .../store/scan/object?key=&wait= read the verdict |
| +POST .../store/scan/object request a scan |
| +GET .../store/scan/object?key=&wait= read the verdict |
| ``` |
| -`store scan status` gains the queue depth for that app, and |
| -`system antivirus status` gains it deployment-wide, because "why is my scan |
| -slow" should be answerable without a log. |
| +`store scan status` gains queue depth for that app and the deployment-level |
| +state (§4.5); `system antivirus status` gains it deployment-wide. "Why is my |
| +scan slow" should be answerable without a log. |
| --- |
| ## 11. Security properties |
| -Each of these is a test, not a hope. |
| - |
| -1. **No caller-supplied host reaches an outbound request.** Callbacks go to the |
| - app's own host; the path is validated and cannot escape it. |
| -2. **Every callback is signed**, and a body altered in flight fails |
| - verification. |
| -3. **The app policy still has no `s3:PutObjectTagging`.** Unchanged from the |
| - sweep design and still the thing that stops an app marking its own object |
| - clean. |
| -4. **A `Service` token cannot act on another app.** The path names the app and |
| - the token is scoped to it. |
| -5. **A `Service` token cannot reach operator doors** — `system antivirus`, |
| - `scan grant`, `enforce`. |
| -6. **Version comparison happens console-side**, so a stale tag can never be |
| - served to an app as a fresh verdict. |
| -7. **Rate limits are enforced before work is enqueued**, so the queue cannot be |
| - used as an amplifier. |
| +Each is a test, not a hope. |
| + |
| +1. No callback reaches a host the app was not assigned — for absolute URLs, |
| + `//host`, `/\host`, `@`-authority tricks, traversal, CRLF, and encoded |
| + variants of each. |
| +2. No redirect is followed. |
| +3. A tampered body fails verification; a signature from app A does not verify |
| + for app B; a stale timestamp is refused. |
| +4. The app policy still has no `s3:PutObjectTagging` (`aws.go:152-165`). |
| +5. A `Service` credential cannot be minted account-scoped (§3.4), and cannot |
| + act on another app. |
| +6. A `Service` credential cannot reach `system antivirus`, `scan grant`, |
| + `revoke`, `sweep` or `enforce`. |
| +7. Version comparison is console-side; a stale tag is never served as fresh. |
| +8. Rate limits are enforced before enqueue, and re-requesting a judged key |
| + costs a tag read rather than a fetch. |
| --- |
| ## 12. Out of scope |
| -- **Synchronous scan on PUT.** CARLOS does not see the PUT and giving it that |
| - would mean proxying tenant data through the control plane — the opposite of |
| - the direction this spec moves in. |
| -- **Gotenberg as a capability.** The `Service` role is designed so it can reuse |
| - it, but PDF rendering is its own brainstorm. |
| +- **Synchronous scan on PUT.** CARLOS does not see the PUT; giving it that |
| + means proxying tenant data through the control plane. |
| +- **Gotenberg as a capability.** `Service` is designed so it can reuse the |
| + role; PDF rendering is its own brainstorm. |
| - **GuardDuty.** Still gated on the tagging-cost measurement. |
| -- **Pricing.** Units are recorded; a price sheet is a separate decision. |
| +- **Pricing.** Units recorded; a price sheet is a separate decision. |
| --- |
| ## 13. Testing |
| -Beyond the per-task tests, the properties worth naming: |
| - |
| -- a callback POST is never issued to a host the app did not come from, for any |
| - `callback_path` including absolute URLs, `//host`, and traversal |
| -- a tampered body fails signature verification |
| -- `pending` is returned for a stale tag, not the stale verdict |
| -- one app flooding the queue does not delay another app's item beyond one tick |
| -- a dropped callback still leaves the verdict readable through the status door |
| -- disabling via `system antivirus disable` stops the pass without a restart and |
| - without touching per-app records |
| -- an unknown `system/antivirus.json` schema refuses rather than guessing |
| +- callback host and path validation, per §11.1, as a table of hostile inputs |
| +- redirects refused |
| +- signature: tamper, cross-app, replay, skew |
| +- `pending` returned for a stale tag, never the stale verdict |
| +- one app flooding the queue does not delay another's item beyond one tick |
| +- a dropped callback leaves the verdict readable through the status door |
| +- `system antivirus disable` stops scanning **without a restart** and without |
| + touching per-app records — and per-app `scan status` then reports the |
| + deployment state rather than claiming health |
| +- `request`, `revoke`, `enforce off` still work while disabled |
| +- unknown `system/antivirus.json` schema refuses |
| - metering counts on-demand scans and does **not** count sweep scans |
| +- an enqueue racing `PurgeApp` leaves no orphan that blocks the name |
| --- |
| ## 14. Risks |
| -- **The role migration is the unknown.** §3.3. If existing app credentials |
| - cannot carry a new role without rotation, the transitional |
| - `Allow(Service, Instance)` costs a release cycle. |
| -- **A shorter tick multiplies the enumeration cost.** The scan pass lists every |
| - account and app each tick; at 30s that is 120× the hourly cost. The queue |
| - phase should list the queue prefixes only, and fall back to full enumeration |
| - on the sweep cycle. Named here so it is measured rather than discovered. |
| -- **Long-poll holds console goroutines.** Bounded wait and a cap on concurrent |
| - waiters per app; otherwise it is a cheap way to occupy the console. |
| +- **Tick cost.** The drain lists only indexed prefixes, so the 120× enumeration |
| + draft 1 implied does not occur — but the index is in memory, and the boot |
| + rebuild is a full enumeration. Measure it on a deployment with many apps. |
| +- **Long-poll occupancy.** Bounded wait, global and per-app waiter caps. |
| +- **The index is soft state.** Losing it loses latency, never items: the sweep |
| + still finds unscanned objects on its hourly cycle. That is the designed |
| + fallback and should be tested by killing the console mid-queue. |