| 1 | # Scan on demand: design |
| 2 | |
| 3 | **Status:** draft 7 — final, cleared to plan from (closes reviews 1-6). |
| 4 | **Builds on:** `2026-09-01-object-storage-virus-scanning-design.md` — the sweep, |
| 5 | the `Scanner` seam, verdict tags, enforcement. Landed and reviewed. |
| 6 | |
| 7 | **It contradicts that spec in one place, deliberately.** Prior spec §4.1 |
| 8 | records: *"No TCP option in v1: an unauthenticated scanning service reachable |
| 9 | over a network is a worse problem than the one being solved. **(decision)**"*. |
| 10 | §9 reverses it, and argues the reversal rather than assuming it. |
| 11 | |
| 12 | --- |
| 13 | |
| 14 | ## 1. What is being asked |
| 15 | |
| 16 | The shipped scanner answers "is this bucket clean?" hourly. It does not answer |
| 17 | "is *this object* clean, now?" — the question an app has when someone has just |
| 18 | uploaded a file it is about to serve. The current docs tell app authors to |
| 19 | solve that themselves, which is the platform declining work it is better placed |
| 20 | to do. |
| 21 | |
| 22 | **The delay is scheduling, not scanning.** `clamd` answers on a typical upload |
| 23 | in well under a second. So this is queueing and notification. |
| 24 | |
| 25 | ### 1.1 What can honestly be claimed |
| 26 | |
| 27 | > Object `<key>` was scanned at `<T>` against `<engine>/<signature-db>` and |
| 28 | > found clean. |
| 29 | |
| 30 | plus the sentence that makes the first one worth anything: |
| 31 | |
| 32 | > `N` objects in this store carry no verdict. |
| 33 | |
| 34 | On-demand scanning shortens T. It does not make scanning synchronous with |
| 35 | upload: **CARLOS never sees the PUT**. |
| 36 | |
| 37 | --- |
| 38 | |
| 39 | ## 2. Scope |
| 40 | |
| 41 | In: `svcrole.Service`; `carlos system antivirus`; a priority scan door and a |
| 42 | long-polling status door; a queue; an opt-in signed webhook; metering, recorded |
| 43 | and unpriced; clamd over TCP on a services box. |
| 44 | |
| 45 | Out (§12): synchronous scan on PUT, GuardDuty, Gotenberg as a capability, |
| 46 | pricing. |
| 47 | |
| 48 | --- |
| 49 | |
| 50 | ## 3. The service role |
| 51 | |
| 52 | ### 3.1 Why not `Instance` |
| 53 | |
| 54 | `svcrole.go:34-36` calls `Instance` *"the LEGACY app-token authority, defined by |
| 55 | observation of today's call sites rather than by its name."* A role defined by |
| 56 | what happens to call it is not one to hang a new API class on, and a token |
| 57 | minted to read logs must not silently gain the ability to spend deployment CPU. |
| 58 | |
| 59 | Enforced, not merely intended: |
| 60 | `TestInstanceRoleMatchesTodaysAppTokenReach` (`svcroutes_test.go:262-291`) pins |
| 61 | `Instance` to exactly nine doors and fails with "widening the legacy role". |
| 62 | |
| 63 | ### 3.2 `svcrole.Service` |
| 64 | |
| 65 | **An app, at runtime, asking a platform service to do work on its behalf** — |
| 66 | work the platform performs on the app's own data that costs the deployment |
| 67 | resources. Scanning is the first; PDF rendering is next. |
| 68 | |
| 69 | ```go |
| 70 | Service Role = "service" |
| 71 | ``` |
| 72 | |
| 73 | Also update `svcrole.go:41-44` ("the union of the three above") and the CLI's |
| 74 | hardcoded role list in `services.go:164` — "mintable with no further change" is |
| 75 | true of the API, not of the CLI's usage string. |
| 76 | |
| 77 | **`Allow` unconditionally adds `Admin`** (`svcrole.go:109`), so an Admin |
| 78 | credential reaches the new doors by construction, and Admin *can* be |
| 79 | account-scoped. That is within one account, not a tenant-boundary hole, but |
| 80 | §11.5's property is scoped to role `Service` and must say so. |
| 81 | |
| 82 | ### 3.3 There is no migration |
| 83 | |
| 84 | Draft 1 called this the document's biggest risk and proposed |
| 85 | `Allow(Service, Instance)`. Both were wrong. |
| 86 | |
| 87 | | kind | minted by | roles | rotatable | |
| 88 | |---|---|---|---| |
| 89 | | person (`kind=""`) | `carlos auth login` | none — never meets the role gate (`authcli.go:425`) | n/a | |
| 90 | | legacy app token (`kind="app"`) | a person, `POST .../instance-token` | `Instance`, hardcoded (`svcprincipal.go:124`) | **no** (`svcprincipal.go:244-246`) | |
| 91 | | named service (`kind="service"`) | account **owner** (`apiservices.go:102-112,143`) | any role `svcrole.Parse`s (`apiservices.go:180`) | yes | |
| 92 | |
| 93 | Adding `Service` to the catalogue makes it mintable: the mint door, the mint |
| 94 | form (`svcpage.go:176`) and `doorsFor` all read `svcrole.All()`. |
| 95 | |
| 96 | Nothing migrates. A `kind='app'` token can never become `Service`, so an app |
| 97 | that wants on-demand scanning is issued a *new* credential: |
| 98 | |
| 99 | ``` |
| 100 | carlos services create -account <a> -app <app> -role service |
| 101 | carlos secrets set -app <app> CARLOS_SCAN_TOKEN=<...> |
| 102 | ``` |
| 103 | |
| 104 | the same delivery step it already performed for its instance token. |
| 105 | **`Allow(Service, Instance)` is deleted**: it cannot terminate (legacy tokens |
| 106 | do not rotate into anything) and it would fail the pinned test above — the |
| 107 | exact widening §3.1 argues against. |
| 108 | |
| 109 | ### 3.4 Scope must be per-app, enforced in one place |
| 110 | |
| 111 | `NewServicePrincipal` permits `app == ""` (`svcprincipal.go:140-141`) and |
| 112 | `apiAccountAppBy` binds only when `p.App != ""` (`apicli.go:70`), so an |
| 113 | account-scoped credential reaches every app in its account. |
| 114 | |
| 115 | There are **two** mint doors — the API (`apiservices.go:211`) and the browser |
| 116 | form (`svcpage.go:299`, which explicitly offers "an account-wide credential"). |
| 117 | So the refusal goes in **`NewServicePrincipal`** (`svcprincipal.go:170-172`, |
| 118 | beside the existing role parse), not in a handler, or one door forgets it. It |
| 119 | needs a named sentinel — `ErrAccountScopedService` — because the API door |
| 120 | today matches only `ErrServiceNameTaken` (`apiservices.go:211-215`) and turns |
| 121 | anything else into a generic failure. Both doors then surface the same 400. |
| 122 | |
| 123 | --- |
| 124 | |
| 125 | ## 4. `carlos system antivirus` |
| 126 | |
| 127 | ### 4.1 Wiring versus policy |
| 128 | |
| 129 | `CARLOS_STORE_SCANNER` conflates *there is a clamd at this address* with |
| 130 | *scanning is on*. Wiring belongs in the unit file; policy does not. Turning a |
| 131 | compliance control on should not mean editing a systemd unit and restarting the |
| 132 | control plane, and today the only way to know whether scanning is healthy is to |
| 133 | log into the box — which the no-infra-access rule forbids. |
| 134 | |
| 135 | | what | where | |
| 136 | |---|---| |
| 137 | | `CARLOS_CLAMD_ADDR` — `host:port`, or a path when it begins `/` | env, boot | |
| 138 | | `CARLOS_CLAMD_MAX_BYTES` | env, boot | |
| 139 | | `CARLOS_SCAN_WEBHOOK_KEY` (+ `_PREVIOUS`, §6.3) | env, boot | |
| 140 | | enabled / disabled, engine, at, by | **record** | |
| 141 | |
| 142 | There is no clamd timeout variable today (`clamd.Timeouts{}` defaults, |
| 143 | `clamav.go:71`); this spec does not add one. |
| 144 | |
| 145 | ### 4.2 What replaces the env var |
| 146 | |
| 147 | **Every place that keys off `CARLOS_STORE_SCANNER` or a nil Scanner:** |
| 148 | |
| 149 | 1. `storeScannerFor` (`main.go:755-758`) builds the Scanner whenever |
| 150 | `CARLOS_CLAMD_ADDR`, the retained `CARLOS_CLAMD_SOCKET` alias, **or the |
| 151 | deprecated `CARLOS_STORE_SCANNER=clamav`** is set — the last of those |
| 152 | retaining today's socket default (`/run/clamav/clamd.ctl`, `main.go:774`). |
| 153 | Without that third case a deployment that set only the old variable gets a |
| 154 | nil Scanner, no goroutine (`scanpass.go:37-39`) and nothing to seed a record |
| 155 | from: scanning stops silently, which is what §4.2.1 exists to prevent. |
| 156 | Safe because `NewClamAVScanner` does not ping (`clamav.go:56-70`). |
| 157 | `main.go:772`'s fatal when no store backend is configured stays. |
| 158 | 2. `StartScanPass` (`scanpass.go:37-39`) loops whenever `Scanner != nil` and |
| 159 | **reads `system/antivirus.json` at the top of each tick**. Disabled means the |
| 160 | tick does nothing — not that there is no goroutine. That is what makes |
| 161 | `enable` restartless. A per-tick GET of one small object is negligible |
| 162 | against the enumeration the tick already does. |
| 163 | 3. `scan grant` (`apiscan.go:252`) and `scan sweep` (`scanpass.go:198`) check |
| 164 | the **record**, and 503 naming `carlos system antivirus enable`. |
| 165 | 4. `RunSweep`'s refusal (`sweep.go:163`) checks the record. |
| 166 | 5. `apiscan.go:71-81` — the `engine_error` branch — reports *disabled* as its |
| 167 | own state, distinct from *unreachable*. |
| 168 | 6. `apiscan.go:45`'s message names the command, not the variable. |
| 169 | |
| 170 | **Still correctly branching on wiring, and unchanged:** `appsettings.go:838-840` |
| 171 | and `walk.go:466` (Teardown must release an engine that exists regardless of |
| 172 | policy), `apiscan.go:351` (revoke's best-effort `EnsureDisabled`), |
| 173 | `apiscan.go:419` (enforce-on needs an engine to name a tag). |
| 174 | |
| 175 | **Comments and message strings naming the variable, which are reworded rather |
| 176 | than rewired:** `objstore.go:86`, `scanner.go:89`, `server.go:522-523`, and |
| 177 | `main.go:772`'s message. |
| 178 | |
| 179 | Two documents change with this: `docs/site/cli.md:474-488`, which lists the |
| 180 | variables, and **`docs/superpowers/plans/2026-09-01-carloku-scanning-rollout.md`**, |
| 181 | whose step 4 sets `CARLOS_STORE_SCANNER=clamav` and whose rollback is "unset it |
| 182 | and restart". Step 4 becomes `CARLOS_CLAMD_ADDR` plus `carlos system antivirus |
| 183 | enable`; the rollback becomes `carlos system antivirus disable`, which needs no |
| 184 | restart — the feature's own selling point. |
| 185 | |
| 186 | **`enable` refuses with 503 when no Scanner is wired**, or the record can claim |
| 187 | enabled with nothing behind it. |
| 188 | |
| 189 | ### 4.2.1 Migration — an upgrade must not silently stop scanning |
| 190 | |
| 191 | Today `CARLOS_STORE_SCANNER=clamav` is the only gate. After this change the |
| 192 | pass reads a record that **exists on no current deployment**, so every tick |
| 193 | would do nothing, and an operator who did not touch the unit file would get a |
| 194 | nil Scanner as well. That is precisely the failure `main.go:751-753,766-768` |
| 195 | refuses to allow: *"a typo that silently disabled scanning would leave an |
| 196 | operator believing a compliance control is on when nothing is running."* |
| 197 | |
| 198 | **Draft 3 answered this with a boot `log.Fatal` on the retired variable. That |
| 199 | was worse than the problem.** Console env is unit-file managed, so an operator |
| 200 | who ships the build before editing env gets a console that will not start and |
| 201 | has to edit a unit file on the box — the exact access §4.1 says this design |
| 202 | should remove the need for. (Draft 4 justified this partly by claiming the |
| 203 | box's adopt pass does not restart the unit on rollback. **That is wrong** — |
| 204 | `WithStartUnit` (`adopt.go:204-215`) exists to start the unit after a |
| 205 | rolled-back failed restart, and the host agent passes it in production |
| 206 | (`host/agent.go:602,1155,2960`), so a fataling console is rolled back *and* |
| 207 | started on the old build. The decision stands on the other leg alone.) The |
| 208 | other order — edit env first — requires restarting the *old* binary, which then |
| 209 | sees no engine and stops scanning anyway. One order costs a scanning outage; |
| 210 | the other costs a trip to the box. |
| 211 | |
| 212 | So the migration preserves behaviour instead of demanding attention: |
| 213 | |
| 214 | - `CARLOS_STORE_SCANNER=clamav` is accepted as a **deprecated alias for |
| 215 | wiring**, logged loudly on every boot. Any other value stays fatal, which is |
| 216 | today's behaviour for an unknown engine. |
| 217 | - `CARLOS_CLAMD_SOCKET` is retained as an alias for `CARLOS_CLAMD_ADDR`; |
| 218 | setting both to different values is fatal. |
| 219 | - **the record is seeded enabled, once, when the retired variable is set and |
| 220 | `system/antivirus.json` is absent**, stamped `by: "migration from |
| 221 | CARLOS_STORE_SCANNER"`. Draft 3 refused to seed on the grounds that |
| 222 | auto-enabling a control that reads tenant bytes is the wrong default. That |
| 223 | reasoning does not apply to a *migration*: the operator already enabled it, |
| 224 | explicitly, and per-app grants still gate every byte. Refusing to seed does |
| 225 | not withhold consent — it silently withdraws consent already given, which is |
| 226 | the failure `main.go:751-753` exists to prevent. |
| 227 | - **where the seed happens**: at the top of the first `StartScanPass` tick — |
| 228 | the same record read §4.2 item 2 already performs — not in |
| 229 | `storeScannerFor`, which runs at `main.go:262` before `openStore()` at |
| 230 | `:269` and has no release store to write to. |
| 231 | - the write is **create-only** (`Put(…, "")`, `release/store.go:38,516` — the |
| 232 | `IfNoneMatch:*` that `TrashApp` already relies on, |
| 233 | `release/trash.go:51-54`), so two |
| 234 | consoles or a restart loop converge; `ErrCASConflict` means "someone else |
| 235 | seeded it" and is success. |
| 236 | - seeding is therefore idempotent: it happens only when no record exists, so an |
| 237 | operator who later runs `disable` is not overridden on the next restart. |
| 238 | - **a read error at boot neither seeds nor fatals** — log and retry on the next |
| 239 | tick. Seeding on a failed read would enable scanning on a deployment that may |
| 240 | have disabled it. |
| 241 | - **the seed fires only while the retired variable is still set**, so the |
| 242 | upgrade order matters and the rollout must say it: deploy the new build with |
| 243 | `CARLOS_STORE_SCANNER` still in place (one tick seeds the record), *then* |
| 244 | swap it for `CARLOS_CLAMD_ADDR`. Swapping in the same restart as the upgrade |
| 245 | seeds nothing and stops scanning — warned every tick, but stopped. Running |
| 246 | `carlos system antivirus enable` also settles it at any point. |
| 247 | - `main.go` carries "the deprecated variable was set" to the first tick as a |
| 248 | `Config` field (`ScanSeedFromDeprecatedEnv bool`); the pass does not read the |
| 249 | environment itself. |
| 250 | - the first tick warns when a Scanner is wired and no record exists — not boot, |
| 251 | which has no store — and `system antivirus status` reports "engine wired, no |
| 252 | record" as its own state. |
| 253 | |
| 254 | ### 4.3 The record |
| 255 | |
| 256 | `system/antivirus.json`, deployment-scoped — sibling in spirit to |
| 257 | `infraview.InventoryKey` (`internal/infraview/infraview.go:25`), with the |
| 258 | helper beside `release.FeaturesKey` (`internal/release/features.go:53`): |
| 259 | |
| 260 | ```go |
| 261 | // package release |
| 262 | func SystemKey(name string) string { return "system/" + name + ".json" } |
| 263 | |
| 264 | type AntivirusSettings struct { |
| 265 | Schema int `json:"schema"` |
| 266 | Enabled bool `json:"enabled"` |
| 267 | Engine string `json:"engine"` |
| 268 | At time.Time `json:"at"` |
| 269 | By string `json:"by"` |
| 270 | } |
| 271 | ``` |
| 272 | |
| 273 | CAS read/write, schema pinned, unknown schema refuses — the rules |
| 274 | `objstore.Record` and `ScanSidecar` follow, and for the same reason: this one |
| 275 | decides whether tenant bytes get read. |
| 276 | |
| 277 | **A typed record per feature, not a generic map.** A map is `features.json` by |
| 278 | another name: uninterpreted and unpinnable. `system/pdf.json` later is a new |
| 279 | type and a new door, not a new magic string. |
| 280 | |
| 281 | ### 4.4 The doors |
| 282 | |
| 283 | ``` |
| 284 | carlos system antivirus status|enable|disable|rotate-webhook-key |
| 285 | ``` |
| 286 | |
| 287 | A deployment-wide door has **no `{account}` segment**, so `apiPrincipalAccount` |
| 288 | (`apicli.go:176-187`) cannot resolve it. Precedent exists: `GET /api/cli/apps` |
| 289 | (`authcli.go:471`) is already a no-`{account}` door. These need a person-only |
| 290 | resolver, registered `svcrole.Set{}` so no service principal reaches them, and |
| 291 | added to `closedDoors` (`svcroutes_test.go:83`) or |
| 292 | `TestEveryCLIDoorNamesItsRoles` (`svcroutes_test.go:140-172`) fails. |
| 293 | |
| 294 | Authority is `storeOperator(addr)` — the `CARLOS_STORE_OPERATORS` list that |
| 295 | already governs `store grant` (`apistore.go:520`) and every scan operator verb. |
| 296 | Draft 1's "not `Admin` alone" confused a service role with person authority: |
| 297 | operators are addresses (`server.go:558-571`), a service actor is |
| 298 | `service:`/`app:` (`apicli.go:89-100`), and the two can never match. |
| 299 | |
| 300 | `status` answers "is scanning healthy" without SSH: engine version, whether it |
| 301 | answers, signature age, queue depth. |
| 302 | |
| 303 | ### 4.5 Two layers |
| 304 | |
| 305 | Deployment enable scans nothing by itself. Per-app `store scan request` |
| 306 | (member) and `store scan grant` (operator) are unchanged. |
| 307 | |
| 308 | **Per-app `store scan status` must surface the deployment state**, or a record |
| 309 | saying `enabled: clamav` reads as healthy while nothing runs — a new way for |
| 310 | the status output to lie. Note `apiscan_test.go:254` asserts `engine == |
| 311 | "clamav"` on a fixture with a Scanner and no record; that fixture must write an |
| 312 | enabled record. |
| 313 | |
| 314 | --- |
| 315 | |
| 316 | ## 5. The pull path |
| 317 | |
| 318 | ### 5.1 Request |
| 319 | |
| 320 | ``` |
| 321 | POST /api/cli/apps/{account}/{app}/store/scan/object |
| 322 | {"key": "uploads/x.pdf", "callback_path": "/hooks/scan"} // callback_path optional |
| 323 | → 202 {"state":"pending"} |
| 324 | → 409 {"state":"unavailable","reason":"not_granted"|"not_requested"|"encrypted"|"not_enabled"} |
| 325 | → 503 {"state":"unavailable","reason":"disabled"|"no_engine"} |
| 326 | → 429 on rate limit |
| 327 | ``` |
| 328 | |
| 329 | Role `Service`. It enqueues; it does not scan inline. Inline scanning would |
| 330 | hold a console goroutine for the duration and hand one tenant a trivial way to |
| 331 | occupy them all. |
| 332 | |
| 333 | ### 5.1.1 The consent chain applies here too |
| 334 | |
| 335 | **This was the hole in draft 2.** The landed sweep refuses to read tenant bytes |
| 336 | without the member's attestation (`sweep.go:151-160`), skips stores attested |
| 337 | encrypted (`:147-149`), and `scan grant` refuses without a request |
| 338 | (`apiscan.go:268-285`) on the stated ground that *an operator must not be able |
| 339 | to start that on their own*. Draft 2's enqueue checked only the deployment |
| 340 | switch — so an app's own credential could make the console fetch and scan |
| 341 | objects in a store that was never requested, never granted, or attested |
| 342 | client-encrypted. A new door straight through the property the whole feature |
| 343 | exists to protect. |
| 344 | |
| 345 | Both the enqueue **and the drain** refuse unless all of: |
| 346 | |
| 347 | - `rec != nil && rec.Provisioned != nil` → else `not_granted`, the state |
| 348 | `apiscan.go:263-266` already names |
| 349 | - the app is not in the trash — `release.ReadAppTombstone`, which the sweep |
| 350 | checks at `sweep.go:135-141` → else the drain deletes the items |
| 351 | - `rec.Scanning != nil && rec.Scanning.Requested != nil` → else `not_requested` |
| 352 | - `!rec.Scanning.Requested.Encrypted` → else `encrypted` |
| 353 | - `rec.Scanning.Enabled != nil` → else `not_enabled` |
| 354 | - the deployment record is enabled → else `disabled` |
| 355 | |
| 356 | Draft 3 omitted the first two. The trash one matters specifically for the |
| 357 | drain: `HasAppIn` filters `deleted_at = 0` (`console/store.go:1208`) so a trashed app |
| 358 | 404s at the door, but items enqueued *before* `TrashApp` sit under the prefix |
| 359 | for the 30-day cooloff with `store.json` intact and `Enabled` non-nil. Without |
| 360 | the tombstone check the drain would read bytes from a store the sweep |
| 361 | deliberately will not touch. |
| 362 | |
| 363 | The drain re-checks all of it because the record can change between enqueue and |
| 364 | drain, and the drain is where bytes are actually read. It also applies |
| 365 | `RunSweep`'s engine-drift refusal (`sweep.go:175-178`). |
| 366 | |
| 367 | **On a read error — not an absence — the drain keeps the items and the index |
| 368 | entry, logs, and retries next tick.** `objstore.Get` returns `(nil, "", nil)` |
| 369 | for absent and an error for failure; §7.5's "delete when `store.json` is gone" |
| 370 | must never be implemented as "delete on any error", or a transient bucket |
| 371 | failure discards a tenant's queue. |
| 372 | |
| 373 | ### 5.2 Read |
| 374 | |
| 375 | ``` |
| 376 | GET .../store/scan/object?key=uploads/x.pdf&wait=15s |
| 377 | → {"state":"clean","at":"...","engine":"clamav","signature":"..."} |
| 378 | → {"state":"infected","signature":"Eicar-Test"} |
| 379 | → {"state":"skipped","reason":"too_large"} |
| 380 | → {"state":"failed","detail":"..."} // the engine read it and could not judge it |
| 381 | → {"state":"denied"} // the console may not read it |
| 382 | → {"state":"pending"} |
| 383 | → {"state":"gone"} // object does not exist |
| 384 | → {"state":"unavailable","reason":"..."} // as §5.1 |
| 385 | ``` |
| 386 | |
| 387 | **The console does the version comparison, and the token never leaves it.** The |
| 388 | tag records `etag@RFC3339` (`sweep.go:49-51`). An app must not reconstruct |
| 389 | that: it is an internal format, and an app comparing it wrongly gets the |
| 390 | laundering bug *as a correctness bug* — upload v2 over a key whose v1 was |
| 391 | tagged clean, read the stale tag, serve the payload. |
| 392 | |
| 393 | Draft 2 said "the seam widens" for `HeadObject`. That understates it: |
| 394 | `scanS3API` (`clamav.go:25-29`) is the **scanner's private** seam, and the door |
| 395 | reaches the scanner only through `Scanner` (`scanner.go`), which cannot ask |
| 396 | about one key. So `Scanner` gains: |
| 397 | |
| 398 | ```go |
| 399 | // Status answers what is known about the CURRENT version of one object, and |
| 400 | // returns the object as a listing page would have described it so a caller |
| 401 | // can go on to Scan it without a HeadObject of its own. |
| 402 | // |
| 403 | // Verdict is the recorded one when a tag matches the current version, |
| 404 | // VerdictUnknown when the tag is absent or stale. ErrObjectGone when the |
| 405 | // object is not there. |
| 406 | Status(ctx context.Context, bucket, key string) (Verdict, ScanObject, error) |
| 407 | ``` |
| 408 | |
| 409 | **`Status` reads the tag as written; it does not apply `Verdict`'s re-issue |
| 410 | rule.** The landed reader maps a stored `denied` or `failed` tag back to |
| 411 | `VerdictUnknown` (`clamav.go:160-163`) so the sweep retries it — correct as |
| 412 | *sweep retry policy*, and wrong as *an answer to a question about an object*. |
| 413 | Left in place here, a failed scan reads as `pending` forever: the drain deletes |
| 414 | the item as scanned, and nothing ever changes the answer until a sweep re-fails |
| 415 | it. `Verdict` keeps the re-issue rule; `Status` reports what the tag says. |
| 416 | Different questions, and the split is the point. |
| 417 | |
| 418 | **The `ScanObject` return is not decoration.** Draft 3 had `Status` return a |
| 419 | verdict alone, and §7.4 has the drain call `Status` before `Scan` — but `Scan` |
| 420 | takes `(bucket, key, version, size)` (`scanner.go:42`) and the drain has no |
| 421 | listing page. Without the object it would need its own `HeadObject`, which is |
| 422 | exactly what keeping the version token inside the scanner forbids. Draft 2 had |
| 423 | the same hole under a different name. |
| 424 | |
| 425 | `Status` needs `HeadObject`, which `scanS3API` (`clamav.go:25-29`) and |
| 426 | `stubScanS3` (`clamav_test.go:24`) both gain. |
| 427 | |
| 428 | **When `HeadObject` itself is denied** there is no version or size to hand |
| 429 | `Scan`, so `Status` returns `(VerdictDenied, ScanObject{}, nil)` and the drain |
| 430 | does not call `Scan` on a zero `ScanObject`. The SSE-KMS case the landed |
| 431 | comment names behaves differently and correctly: `HeadObject` succeeds, |
| 432 | `GetObject` is denied, so the drain gets a populated object, scans, and gets |
| 433 | `Denied` from the fetch. `memscanner.go` and |
| 434 | `scanner_contract_test.go` follow: `MemScanner` has no object model, so it |
| 435 | gains a seeding call — `PutObject(bucket, key, etag string, modified |
| 436 | time.Time, size int64)` — and the contract test asserts an absent object gives |
| 437 | `ErrObjectGone`. |
| 438 | |
| 439 | **`wait` long-polls**, capped at 30s, woken by **in-process notification from |
| 440 | the drain** — a polling waiter costs one tag read per second per waiter. Cap |
| 441 | concurrent waiters **globally** as well as per app. |
| 442 | |
| 443 | **`skipped`, `gone` and `unavailable` are not `clean`.** Distinct states with |
| 444 | reasons, because an app treating them as success reproduces the dishonesty the |
| 445 | two-number rule exists to prevent. |
| 446 | |
| 447 | ### 5.2.1 When a verdict is reached but cannot be recorded |
| 448 | |
| 449 | `clamavScanner.tag` returns success **without writing** in two landed cases: |
| 450 | an object already carrying eight foreign tags (`clamav.go:306-308`, the ten-tag |
| 451 | cap) and a pre-write tag read that is denied or gone (`clamav.go:289-291`). |
| 452 | Both were deliberate — they stop one object stalling a whole sweep — and the |
| 453 | sweep tolerates them because it re-lists every 24h. |
| 454 | |
| 455 | **A queue item does not.** The drain would get a real verdict from `Scan`, send |
| 456 | `clean` in the callback, delete the item — and `Status` would keep answering |
| 457 | `pending` forever, because the tag is not there. The app's declared source of |
| 458 | truth (§5.3) would never arrive, and the two answers it did get would disagree. |
| 459 | |
| 460 | **The `Scanner` interface changes**, which draft 4 hid inside a sentence: |
| 461 | |
| 462 | ```go |
| 463 | Scan(ctx, bucket, key, version string, size int64) (Verdict, bool, error) |
| 464 | // ^ recorded |
| 465 | ``` |
| 466 | |
| 467 | The sweep's call site (`sweep.go:235`) takes `_` for the flag and its counts |
| 468 | are unaffected — Fresh and Reused count verdicts *reached*, which is what |
| 469 | `clamav.go:302-303` already says it is doing. `MemScanner.Scan` |
| 470 | (`memscanner.go:102`) gains an `Unrecordable(key)` hook so the case is testable |
| 471 | without a live S3 stub. The third return value touches roughly fifteen call |
| 472 | sites in `clamav_test.go` and the contract test, not only the sweep. |
| 473 | `MemScanner.Verdict` (`memscanner.go:89-100`) does **not** apply the re-issue |
| 474 | rule today, so it gains it — otherwise the `Verdict`/`Status` split cannot be |
| 475 | exercised against the fake — and `scanner_contract_test.go` pins both halves. |
| 476 | The plan will extract a shared `readTag` helper: `Verdict` is called only from |
| 477 | `sweep.go:215` and no such helper exists yet. |
| 478 | |
| 479 | **Where the answer goes when the tag cannot hold it.** Draft 4 said "keep the |
| 480 | queue item, terminal" — but items are ULID-named and the door asks by key, so |
| 481 | that would mean listing the whole queue prefix on every poll. Instead the drain |
| 482 | writes a **key-derived result object** and deletes the queue item as normal: |
| 483 | |
| 484 | ``` |
| 485 | apps/<acct>/<app>/scan-results/<sha256(key)>.json |
| 486 | {"schema":1, "key":"uploads/x.pdf", "verdict":"clean", |
| 487 | "version":"<etag>@<RFC3339>", "engine":"clamav", "signature":"...", |
| 488 | "at":"..."} |
| 489 | ``` |
| 490 | |
| 491 | **It carries the version, and that is not optional.** Draft 5 keyed it by |
| 492 | `sha256(key)` with the verdict alone — which reintroduces, through the new |
| 493 | mechanism, exactly the bug §5.2 exists to prevent: v1 scanned, tag not written |
| 494 | because the cap fired, result object says clean; the tenant overwrites with v2; |
| 495 | the tag is still absent, so the door falls through to the result object and |
| 496 | answers `clean` for bytes nobody has read. The door compares the recorded |
| 497 | version against the `ScanObject` that `Status` already returns and answers |
| 498 | `pending` on a mismatch. The token stays inside the console throughout, which |
| 499 | is what §5.2 requires. |
| 500 | |
| 501 | - one GET for the door, on the `pending` path only |
| 502 | - durable across restarts, so §11.10 holds without soft state |
| 503 | - under the app prefix, so `PurgeApp` and the claim gate cover it (§7.1's |
| 504 | argument for keeping the queue there applies unchanged) |
| 505 | |
| 506 | **Written read-then-CAS.** `release.Store.Put` has exactly two modes |
| 507 | (`release/store.go:37-45`): create-only with `ifMatch ""`, or replace with an |
| 508 | etag. The second write to the same key is a *replace* — v1's result object is |
| 509 | present, the tenant uploads v2, v2 is scanned and again unrecordable — so a |
| 510 | create-only write there returns `ErrCASConflict` and silently drops the v2 |
| 511 | verdict, leaving the door on `pending` until a sweep tags it. Get, then Put |
| 512 | with the etag if present or create-only if absent, retrying once on conflict: |
| 513 | the `writeSidecar` posture at `sweep.go:370-395`. |
| 514 | |
| 515 | It carries the engine and signature alongside the verdict, because the |
| 516 | `recorded:false` answer is still an audit answer and needs its provenance. |
| 517 | |
| 518 | **Written with the same guard as a sidecar.** The drain re-reads `store.json` |
| 519 | immediately before writing, the way `writeSidecar` does (`sweep.go:358-367`), |
| 520 | because the identical race applies: read the record, scan for seconds, a |
| 521 | teardown and `PurgeApp` land, and a create-only write then *succeeds* under a |
| 522 | purged prefix. A stranded queue item is drained on the next tick because its |
| 523 | index entry survives; **a stranded result object is found by nothing**, and it |
| 524 | makes the app name permanently unclaimable (`server.go:2921`). |
| 525 | |
| 526 | **Collected**, three ways — draft 5 said "by TTL" and named no collector: |
| 527 | |
| 528 | - deleted by the drain as soon as a later scan persists a real verdict for that |
| 529 | key |
| 530 | - deleted with the app's queue items whenever §7.5 deletes them for absence, |
| 531 | tombstone, or a record that no longer permits scanning |
| 532 | - swept by the boot rebuild (§7.1), which lists `scan-results/` as well as |
| 533 | `scan-queue/` and collects anything older than the TTL by `ListMod` age — |
| 534 | the name carries no timestamp, so age has to come from the store |
| 535 | - and swept the same way by the drain for each app it visits, so a console that |
| 536 | never restarts does not accumulate them. Note a *sweep* that later tags the |
| 537 | object does not delete the result object; only the on-demand drain does. |
| 538 | |
| 539 | Age-based collection needs `release.ModLister`, which is optional and |
| 540 | unimplemented by the test fakes. Type-assert it and fall back to skipping age |
| 541 | collection with a single logged line, exactly as `storagePass` does for |
| 542 | `SizeLister` (`economicspass.go:141-145`). |
| 543 | |
| 544 | **TTL for both kinds is 24h**, one sweep cycle: after that the ordinary sweep |
| 545 | has had its chance to write a real verdict, and a stale on-demand answer is |
| 546 | worth less than no answer. |
| 547 | |
| 548 | **`infected` with `recorded:false` means enforcement is NOT in effect.** The |
| 549 | Deny statement matches `s3:ExistingObjectTag/carlos-scan=infected` |
| 550 | (`aws.go:159-165`), and a verdict that could not be tagged does not trigger it. |
| 551 | The app can still read the object and must act on the verdict itself. This goes |
| 552 | in the docs sentence too, not only here. |
| 553 | |
| 554 | ### 5.3 Pull is the source of truth |
| 555 | |
| 556 | The tag on the object is the record; the status door reads it. Webhooks |
| 557 | accelerate that and never replace it (§6.4). |
| 558 | |
| 559 | --- |
| 560 | |
| 561 | ## 6. The push path |
| 562 | |
| 563 | ### 6.1 Why |
| 564 | |
| 565 | Long-poll covers the app being inside a request. **Presigned direct-to-S3 |
| 566 | uploads break that**: the browser PUTs straight to the bucket and the app is |
| 567 | never in a request. Common enough that designing without it is a mistake. |
| 568 | |
| 569 | ### 6.2 Where a callback may go |
| 570 | |
| 571 | Draft 1 claimed taking a path rather than a URL meant the vulnerability "does |
| 572 | not exist". **Overstated.** The host is still tenant-influenced: the outbound |
| 573 | fetcher picks the first ready non-static route (`instances.go:88-97`), which can |
| 574 | be a tenant's custom domain whose DNS they control. And Go follows redirects by |
| 575 | default — a tenant server answering `302 → http://10.x/` turns a signed POST |
| 576 | into a GET at an internal address. The codebase already knows this |
| 577 | (`sesevents.go:143-158`). |
| 578 | |
| 579 | So, all of: |
| 580 | |
| 581 | - POST only to the **platform-assigned host**, `<app>.<sqid>.<AppsDomain>` |
| 582 | (`apiinstances.go:566,1250`) — never a custom domain. |
| 583 | **Correction (review 7): an app with only its own domain DOES get |
| 584 | callbacks.** The domains loop mints the platform host for any app with a |
| 585 | ready route, and `firstReadyUpstream` (`domains/reconcile.go:268`) accepts a |
| 586 | custom-domain route as the source it copies an upstream from. The true |
| 587 | condition is narrower and duller: an app with **no ready route at all** has |
| 588 | no platform host yet, so it has nowhere to be called back and uses long-poll |
| 589 | until it does. The docs say that, not the old sentence. |
| 590 | - **Why the host cannot be moved, with the evidence.** `app` is tenant-chosen |
| 591 | but `^[a-z][a-z0-9-]{0,31}$` (`release.go:242`) — no dots, so no escaping the |
| 592 | label; `sqid` and `AppsDomain` are the platform's. A tenant cannot attach a |
| 593 | custom domain under `AppsDomain` (`apidomains.go:96`). DNS for the name is |
| 594 | written by the fleet's domains loop to edge IPs, not by a tenant. **DNS |
| 595 | rebinding is closed twice**, and the second reason is the one that matters: |
| 596 | TLS name verification against a public CA means a poisoned resolver still |
| 597 | cannot present a valid certificate for that name. `InsecureSkipVerify` on the |
| 598 | callback transport is therefore a **tested absence**, not a code-review item. |
| 599 | - **https only** |
| 600 | - `CheckRedirect` **refuses every redirect** |
| 601 | - bounded timeout, `LimitReader` on the response |
| 602 | |
| 603 | **Path validation as a rule.** Refuse: absolute URLs; protocol-relative |
| 604 | `//evil/x` and `/\evil/x`; anything not beginning with exactly one `/`; raw or |
| 605 | percent-encoded CR/LF; fragments; `..` segments; over 1KB. Then: |
| 606 | |
| 607 | ```go |
| 608 | u, err := url.Parse(p) // must yield Scheme=="" Host=="" User==nil Opaque=="" |
| 609 | target := url.URL{Scheme: "https", Host: appHost, Path: u.Path, RawQuery: u.RawQuery} |
| 610 | ``` |
| 611 | |
| 612 | Built structurally — **never string concatenation**. |
| 613 | |
| 614 | ### 6.3 Signing |
| 615 | |
| 616 | An unsigned callback is a hole wearing a convenience's clothes: anyone who |
| 617 | learns the path POSTs `{"verdict":"clean"}` and the app serves a payload |
| 618 | believing the platform cleared it. |
| 619 | |
| 620 | **What can and cannot be reused from store credentials.** Draft 2 said the key |
| 621 | "cannot be delivered the same way". That was wrong about delivery and right |
| 622 | about retention. Delivery is exactly the store path — `appconfig.Seal` against |
| 623 | `CARLOS_SECRETS_PUBKEY` then one `appconfig.Apply`, which bumps the serial and |
| 624 | reconverges the fleet (`walk.go:602-640`). What cannot be reused is *keeping |
| 625 | nothing*: the console must be able to sign later, and a sealed value it cannot |
| 626 | open is no use to it. |
| 627 | |
| 628 | - one boot master, `CARLOS_SCAN_WEBHOOK_KEY` |
| 629 | - **per-app key derived at sign time**: |
| 630 | `HMAC(master, "carlos-scan-webhook-v1" + 0x00 + sqid + "/" + app + "/" + gen)`. |
| 631 | Nothing extra is stored and per-app isolation falls out — without it a single |
| 632 | deployment secret lets app A forge callbacks to app B. The delimiter is |
| 633 | injection-safe: a sqid is `^[a-z]{3,16}$` (`qualified.go:56`) and an app is |
| 634 | `^[a-z][a-z0-9-]{0,31}$` (`release.go:242`), so neither can contain `/`, and |
| 635 | `gen` is an integer. |
| 636 | *(Corrected: this bullet said `HMAC(master, sqid + "/" + app)` while the |
| 637 | bullets below it added the domain label and the generation, so the section |
| 638 | specified two different schemes. The implementer read both, followed the |
| 639 | later one, and said so — which is the only reason it was caught before the |
| 640 | wire format froze.)* |
| 641 | - **written by `store scan grant`**, gated by `storeSealKey` |
| 642 | (`apistore.go:425`) — one config write on an operator verb, never per enqueue |
| 643 | - header `Carlos-Signature: t=<unix>,v1=<keyid>:<hex>[,v1=<keyid>:<hex>]` over |
| 644 | `t + "." + body`. Each signature carries its own key id; draft 3's single |
| 645 | `k=` could not label two of them, and an app does not need the id anyway — it |
| 646 | holds one derived key and tries it against each `v1`. |
| 647 | - **rotation carries two masters.** `CARLOS_SCAN_WEBHOOK_KEY_PREVIOUS` is |
| 648 | accepted; the console signs under **each** master and the app accepts if |
| 649 | either verifies. Without that window every app is broken between the |
| 650 | console's rotation and its own re-seal. |
| 651 | |
| 652 | **What rotation actually does, stated because it is not free.** |
| 653 | `appconfig.Apply` bumps the generation, and the box's Sync **restarts the app |
| 654 | unit** on a bundle change (`sync.go:33,42,331`). So: |
| 655 | |
| 656 | - `rotate-webhook-key` restarts every scanning-enabled app on the deployment. |
| 657 | It paces the re-seal rather than doing them at once, and says so before it |
| 658 | starts. |
| 659 | - `store scan grant` restarts the tenant's app as a side effect, because that |
| 660 | is where the key is written. The docs sentence for `grant` says so. |
| 661 | - **"done" means sealed, not converged.** The console cannot see when a host |
| 662 | has picked up the new bundle — `server.go:566-569` makes exactly this point |
| 663 | for store rotation. So dropping `_PREVIOUS` is a separate operator step after |
| 664 | convergence, not something `rotate-webhook-key` does. Dropping it early |
| 665 | degrades to one lost callback per stale host, which is survivable because |
| 666 | pull is the source of truth — but it should be a choice, not a surprise. |
| 667 | - `rotate-webhook-key` is idempotent and re-runnable after a partial failure, |
| 668 | and is also the backfill for apps granted before this ships. A re-run with an |
| 669 | unchanged master re-seals the same plaintext and therefore does **not** |
| 670 | restart anything: Sync restarts only when the rendered bundle changes |
| 671 | (`sync.go:340+`). |
| 672 | - **"Paced" needs a mechanism, and it is `scan sweep`'s.** One budget per |
| 673 | call: the door re-seals as many apps as it can inside `scanSweepTimeout`, |
| 674 | answers with how many remain, and the CLI loops until none do. No background |
| 675 | job and no pacing state — idempotence by key id is what makes a re-run safe, |
| 676 | so a console that dies mid-rotation simply resumes. |
| 677 | - **Every console must be restarted with the new pair BEFORE the rotate runs.** |
| 678 | With more than one console, a console still holding only the old master will |
| 679 | sign only under the old key, while apps another console has already re-sealed |
| 680 | hold only the new derived key — and those callbacks fail verification. |
| 681 | Order: restart all consoles with `KEY=new, PREVIOUS=old`, then run |
| 682 | `rotate-webhook-key`, then drop `_PREVIOUS` once converged. |
| 683 | - `store scan revoke` **unsets** the app's callback secret. A key outliving the |
| 684 | consent it was issued under is the same mistake as a result object outliving |
| 685 | it. |
| 686 | - constant-time compare; skew window 5 minutes |
| 687 | - the body carries `recorded`, so a callback saying `clean` and a door saying |
| 688 | `recorded:false` cannot disagree — the disagreement §5.2.1 exists to prevent |
| 689 | - the body carries account, app, key and the **bare ETag** — and **not** the |
| 690 | opaque version. Draft 4 carried both, which contradicted §11.7 ("no door or |
| 691 | app sees the token") in the same document; a review found the two sentences |
| 692 | facing each other. §11.7 wins, because it is the property the version binding |
| 693 | exists to protect: an app that holds the token is an app that can compare it |
| 694 | wrongly, and that is the laundering bug arriving as a correctness bug. The |
| 695 | app has the ETag from its own PUT response and matches on that. |
| 696 | - **Replay, stated plainly because there is no nonce.** A callback replayed |
| 697 | inside the 5-minute skew window verifies and is accepted. That is harmless |
| 698 | and it is the design: the body is idempotent on `(account, app, key, etag, |
| 699 | verdict)`, which is the tuple an app uses as its idempotency key, and it is |
| 700 | named here so two implementers do not choose differently. A nonce is |
| 701 | deliberately NOT added — apps have no durable nonce store to check one |
| 702 | against, so it would be assurance in name only. |
| 703 | - **Clock.** `t` is the console's clock. Apps run on platform boxes and do have |
| 704 | a clock, so the skew check is expected of them; skipping it only widens the |
| 705 | (harmless) replay window above. The verifier must tolerate two `v1=` entries |
| 706 | and unknown key ids. |
| 707 | - **Key id**: `HMAC("carlos-scan-webhook-key-id", master)[:16]` — a *different* |
| 708 | label from `scantag.go`'s tag-key id, so the two schemes cannot produce a |
| 709 | colliding id from the same bytes. `PREVIOUS == current` is refused, exactly |
| 710 | as `NewTagSigner` refuses it: a rotation that rotated to nothing. |
| 711 | - **Derivation carries a domain label**: `HMAC(master, "carlos-scan-webhook-v1" |
| 712 | + 0x00 + sqid + "/" + app)`. The label costs nothing and means no future |
| 713 | `HMAC(master, x)` use can collide with a derived per-app key. |
| 714 | - **Why a delimiter here when `scantag.go` uses length prefixes.** `t + "." + |
| 715 | body` is injective because `t` is decimal digits — it cannot contain a `.` — |
| 716 | so the first `.` splits it unambiguously. The tag scheme needs length |
| 717 | prefixes because an object key can contain any delimiter you pick. Stated |
| 718 | because the two schemes now look inconsistent, and the next person to notice |
| 719 | should find the reason rather than "fix" it: length-prefixing here would make |
| 720 | every non-Go verifier harder for no gain. |
| 721 | - **Per-app rotation.** `ScanBlock` carries a non-secret `webhook_gen`, and the |
| 722 | derivation is over `sqid + "/" + app + "/" + gen`. Without it a tenant that |
| 723 | leaks its callback key has no remedy short of an operator rotating the |
| 724 | deployment master and restarting every scanning app. The tag key needs no |
| 725 | such thing because it never leaves the console; this one is handed to |
| 726 | tenants, so it needs an owner-side rotation. |
| 727 | - **`webhook_gen` IS RANDOM, NOT A COUNTER FROM ZERO**, and the reclaimed-name |
| 728 | property depends entirely on that. Draft 5 said a generation stopped a |
| 729 | reclaimed app name inheriting the deleted app's key. **It did not.** The |
| 730 | implementer pointed out that `PurgeApp` lists the whole app prefix and |
| 731 | deletes every key in it (`trash.go:229-241`), `store.json` included — so a |
| 732 | counter starting at zero starts at zero again for the next app to claim that |
| 733 | name, and derives the identical key. The claim was false as written. |
| 734 | A fresh random value at grant makes it true, and makes it true *because* the |
| 735 | record is destroyed rather than in spite of it: mint a random positive |
| 736 | integer below 2^53 (JSON-number safe, since the console serves this to |
| 737 | browsers) at the first grant, and mint a new one on every per-app rotation. |
| 738 | Zero remains a legal derivation input, because records written before this |
| 739 | ships have no field and their apps already hold a key derived with zero; |
| 740 | `rotate-webhook-key` is the backfill that moves them onto a random one. |
| 741 | - **With no master configured**, the enqueue door refuses a `callback_path` |
| 742 | with 503 `callbacks_not_configured` rather than accepting it. A 202 would |
| 743 | promise a delivery that cannot happen. Delivery refuses to build a request |
| 744 | without a signer, which is a test (`TestNeverPostsUnsigned`) rather than a |
| 745 | convention — §6.3 exists because an unsigned callback is the hole. |
| 746 | |
| 747 | Considered and rejected: Gotenberg's replay-my-headers trick. Neat, avoids a |
| 748 | signing scheme, but gives no body integrity, no replay protection, and puts a |
| 749 | tenant secret in request logs. |
| 750 | |
| 751 | ### 6.4 Delivery is best effort |
| 752 | |
| 753 | **No dead-letter queue**: the verdict is durably on the object and the status |
| 754 | door always answers, so a dropped callback costs one poll rather than stranding |
| 755 | a file. That is the dividend of building pull first. |
| 756 | |
| 757 | **The guarantee is AT-MOST-ONCE, not at-least-once.** Draft 4 said the |
| 758 | opposite and a review caught it: `DrainApp` deletes the queue items before |
| 759 | delivery is attempted, so a console that dies between the delete and the POST |
| 760 | loses that callback and nothing re-sends it — the sweep re-tags the object but |
| 761 | never delivers. Duplicates are still possible (a retry after an ambiguous |
| 762 | failure, or two consoles draining the same prefix), so apps must be idempotent |
| 763 | **and** must not treat the callback as the only way an answer arrives. The |
| 764 | docs sentence is: *if no callback arrives within your own timeout, poll the |
| 765 | status door.* |
| 766 | |
| 767 | The combination that actually hurts is `infected` + `recorded:false` + a lost |
| 768 | callback: enforcement is not in effect (§5.2.1), the object is servable, and |
| 769 | the app is never told unless it polls. That is the case the docs sentence is |
| 770 | written for. |
| 771 | |
| 772 | **Delivery does not run in the drain's goroutine.** Inline delivery would put a |
| 773 | tenant's hanging endpoint directly in the path of every other tenant's verdict |
| 774 | — 50 items times a bounded retry schedule, inside a tick that sweeps apps |
| 775 | sequentially, which is the exact starvation §7.2 and §7.3 exist to prevent and |
| 776 | a trivial tenant-triggered denial of service. Delivery is a bounded worker pool |
| 777 | fed from `done` after the waiters are notified, with a bounded in-memory queue |
| 778 | (overflow drops and logs — it is best effort by construction), a per-app |
| 779 | in-flight cap, and retries scheduled by the pool rather than by the tick. |
| 780 | |
| 781 | **The policy, written down so it is not invented twice:** success is 2xx only; |
| 782 | 3xx is a refusal and is not retried, since redirects are refused by design; |
| 783 | 4xx is not retried; 5xx, 429, timeouts and network errors are retried, three |
| 784 | attempts at 1s/4s/16s; `http.Client.Timeout` around 10s so it covers connect, |
| 785 | TLS, headers *and* body; the response body is drained through |
| 786 | `io.LimitReader(…, 4KiB)` and discarded, because the console needs nothing from |
| 787 | it; headers are `Content-Type: application/json`, a User-Agent and |
| 788 | `Carlos-Signature`, and nothing else — no cookies, no credentials. Ordering is |
| 789 | not guaranteed: retries reorder, and two verdicts for one key can arrive |
| 790 | newest-first, which is why the body carries the ETag. One delivery per distinct |
| 791 | callback path per key, which is already what `Drained.CallbackPaths` means. |
| 792 | |
| 793 | --- |
| 794 | |
| 795 | ## 7. The queue |
| 796 | |
| 797 | ### 7.1 Location and index |
| 798 | |
| 799 | Items live in the app prefix: |
| 800 | |
| 801 | ``` |
| 802 | apps/<acct>/<app>/scan-queue/<ulid>.json |
| 803 | ``` |
| 804 | |
| 805 | One object per item, not one queue object: distinct keys never collide, so an |
| 806 | enqueue needs no coordination. ULIDs sort by time, so a listing is the queue in |
| 807 | order. |
| 808 | |
| 809 | A top-level `scan-queue/<acct>/<app>/` was rejected: `PurgeApp` lists only the |
| 810 | app prefix (`trash.go:229-238`) and the name-claim gate (`server.go:2875+`) |
| 811 | never looks outside it, so items would survive app deletion invisibly. |
| 812 | |
| 813 | But S3 cannot list `*/scan-queue/*`, so finding non-empty queues would be the |
| 814 | full `ListDirs("apps/")` walk. Hence an index — and draft 2 left its removal |
| 815 | rule unstated, which is a stranding race: |
| 816 | |
| 817 | > drain lists (empty) → enqueue writes the item → enqueue marks the app → |
| 818 | > drain removes the mark. The item is now in S3 with nothing pointing at it. |
| 819 | |
| 820 | **The rule, stated:** |
| 821 | |
| 822 | ```go |
| 823 | type queueIndex struct { |
| 824 | mu sync.Mutex |
| 825 | gen map[string]uint64 // "acct/app" → generation |
| 826 | } |
| 827 | ``` |
| 828 | |
| 829 | - enqueue: **write the object first**, then `gen[key]++` under the mutex, and |
| 830 | only then answer 202 — a 202 sent between the two invites a client to stop |
| 831 | retrying an item nothing points at |
| 832 | - drain: read `observed := gen[key]`, drain, then remove the entry **only if |
| 833 | `gen[key] == observed` and the listing returned zero items**. Draft 4 said |
| 834 | "fully drained — listing not truncated"; `release.Store.List` never truncates |
| 835 | (`release/store.go:555-569`), so that clause was vacuous. Zero-items is the test that |
| 836 | actually means "nothing is left", and it costs one empty listing per tick |
| 837 | until the entry goes. |
| 838 | - the drain needs a **bounded, oldest-first listing**, which does not exist |
| 839 | today: `List` returns every key under a prefix (`release/store.go:42-55`), so |
| 840 | a flooded app's whole queue is enumerated every 30s regardless of §7.3's cap. |
| 841 | This is an **optional interface**, `release.PageLister`, type-asserted at the |
| 842 | call site exactly as `release.SizeLister` (`release/store.go:181-185`) is at |
| 843 | `economicspass.go:141` — *not* a new method on `release.Store`, which has |
| 844 | eleven implementations (`DirStore`, `S3Store`, `prefixStore`, |
| 845 | `fleetapi.store` and seven test fakes). `DirStore` can serve it because |
| 846 | `ListMod` is key-sorted (`release/store.go:290`), and S3 has `StartAfter` + |
| 847 | `MaxKeys`, already used at `sweep.go:71-78`. |
| 848 | - a crash between the object write and the increment strands that item until |
| 849 | the next console start |
| 850 | - **boot rebuilds the index with one full enumeration**, and in the same pass |
| 851 | deletes items past their TTL — the only thing that collects a stranded item, |
| 852 | so it is not optional |
| 853 | |
| 854 | **The removal condition is what draft 3 got wrong.** §7.3 caps work per app per |
| 855 | tick, so on a flooded app the drain deliberately leaves items behind; with |
| 856 | generation-equality as the only condition it would then delete the entry |
| 857 | pointing at them. The same happens when the drain aborts mid-app on an engine |
| 858 | error — the `RunSweep` posture at `sweep.go:222-240` that the drain inherits — |
| 859 | or when `Status` errors on one item. Those items would sit until the next boot, |
| 860 | and the callback and long-poll wake would be lost for every capped item, on |
| 861 | precisely the app the cap exists for. |
| 862 | |
| 863 | ### 7.2 The drain is its own goroutine |
| 864 | |
| 865 | Draft 1 put a queue phase inside the existing pass. That cannot deliver seconds |
| 866 | of latency: `scanPass` sweeps apps sequentially, each for a full budget |
| 867 | (`scanpass.go:86-103`), and the prior spec accepts ticks running back to back. |
| 868 | A queue phase behind that starves for up to N × 60s. |
| 869 | |
| 870 | Its own goroutine, its own ticker (30s). It never touches the sidecar; the only |
| 871 | state shared with the sweep is object tags, idempotent per version. |
| 872 | |
| 873 | ### 7.3 Fairness |
| 874 | |
| 875 | Round-robin across apps, **50 items per app per tick** — at a 30s tick that is |
| 876 | 100/minute, close to the door's own 120/minute limit, so a well-behaved app is |
| 877 | never throttled by the drain. Not FIFO across the deployment. FIFO lets one tenant's ten thousand objects delay every other |
| 878 | tenant's single urgent upload. |
| 879 | |
| 880 | ### 7.4 Amplification |
| 881 | |
| 882 | A rate limit alone does not close this: at `svcWriteBurst` (120/min, |
| 883 | `authcli.go:140`) one app can enqueue ~172k items a day, each up to the size |
| 884 | ceiling. |
| 885 | |
| 886 | - the drain calls `Status` **before** `Scan`, as `RunSweep` calls `Verdict` |
| 887 | first (`sweep.go:215`), so a key already judged clean, infected or skipped at |
| 888 | its current version costs a tag read and nothing else |
| 889 | - but the drain **applies the re-issue rule itself**: it scans when `Status` |
| 890 | returns `Unknown`, `Failed` or `Denied`. Those two are transient by the |
| 891 | landed code's own reasoning — "a permissions fix, a Glacier restore, an |
| 892 | engine that was having a bad day" (`clamav.go:151-163`) — and the sweep |
| 893 | re-issues them. Without this the on-demand path would be *less* able than the |
| 894 | hourly one: an object the sweep last tagged `failed` would be answered from |
| 895 | that tag forever and never rescanned on request. The cost is one failed |
| 896 | `GetObject` on an object that is still denied. |
| 897 | - items **deduplicated by key per tick** |
| 898 | - per-app token bucket — the existing `svcWrite` limiter, keyed on principal |
| 899 | name (`svcprincipal.go:375-380`), not a new one. Exceeding it is a 429, never |
| 900 | a silent drop: an app being throttled needs to know, because its upload flow |
| 901 | depends on the answer |
| 902 | |
| 903 | ### 7.5 Deletion and the purge race |
| 904 | |
| 905 | This is the single deletion list; §5.1.1's "refuse" means "does not read |
| 906 | bytes", not "does not delete". |
| 907 | |
| 908 | **Delete the item** when it has been scanned (including §5.2.1, where a result |
| 909 | object is written first); when its TTL has passed; when `store.json` is absent; |
| 910 | when the app is in the trash; and when the record no longer permits scanning — |
| 911 | `Provisioned == nil`, `Requested == nil`, the attestation is encrypted, or |
| 912 | `Scanning.Enabled == nil`. None of those can change back in a way that makes |
| 913 | the item useful. |
| 914 | |
| 915 | **Keep the item — up to the TTL** when the deployment record is `disabled`, on |
| 916 | engine drift, on an unreachable engine, and on any read error. All of those are |
| 917 | conditions an operator can reverse. A deployment disabled for longer than a day |
| 918 | drops its queue, which is right: both doors answered `disabled` at the time, |
| 919 | and the sweep re-covers those objects once it is re-enabled. |
| 920 | |
| 921 | Deletion is driven by **absence**, never by error — and absence at the release |
| 922 | store layer is `os.ErrNotExist` (`trash.go:64`), not a nil value, so queue |
| 923 | items and the system record must test for it explicitly. |
| 924 | |
| 925 | "Inherits the purge machinery" inherits the hazard too: `sweep.go:346-355` — |
| 926 | any object written under a purged prefix makes the app name permanently |
| 927 | unclaimable, and an enqueue racing `PurgeApp` leaves one behind. The orphan |
| 928 | persists for **up to one drain tick**, during which `claimApp` refuses the |
| 929 | name. This spec accepts that, the way `writeSidecar` accepts its |
| 930 | one-round-trip race; §13's test asserts "not longer than one tick", not "never". |
| 931 | |
| 932 | --- |
| 933 | |
| 934 | ## 8. Metering |
| 935 | |
| 936 | `usage.go:26-38` carries `MeterBackhaulGB` recorded and explicitly unpriced. |
| 937 | Scanning follows that idiom — but not into an existing object. `EdgeDay` is |
| 938 | per-box (`edge.go:41-46`) and `StorageDay` is written once daily by a sweep |
| 939 | rather than incremented (`storage.go:13-18`); neither takes increments from a |
| 940 | door. |
| 941 | |
| 942 | - a new `usage/scan/<date>.json`, CAS-merged the way `UpsertEdgeDay` does |
| 943 | (`edge.go:85-116`) |
| 944 | - **folded once per drain tick, not per object** — each fold is a GET plus a |
| 945 | conditional PUT of a deployment-wide object |
| 946 | - new meter names in `usage.go`, **frozen once merged** (`usage.go:4-5`) |
| 947 | - counts objects and bytes scanned **on demand**, per account per day |
| 948 | - unpriced |
| 949 | |
| 950 | Sweep-driven scanning is **not** attributed to a tenant: the operator asked for |
| 951 | it, not them. |
| 952 | |
| 953 | --- |
| 954 | |
| 955 | ## 9. Where clamd lives |
| 956 | |
| 957 | ### 9.1 Draft 1's argument was wrong |
| 958 | |
| 959 | It said the console "shovels every tenant object through its own memory". It |
| 960 | does not: `clamd.go:76-79,173-187` streams 64KB chunks and never buffers. The |
| 961 | `MemoryMax` in the rollout plan is on **clamd's** cgroup, protecting the |
| 962 | console from clamd's ~1.5GB signature RSS — not mitigation for a |
| 963 | byte-shovelling smell, because there is not one. |
| 964 | |
| 965 | ### 9.2 The argument that holds |
| 966 | |
| 967 | Moving clamd to a services box **relocates the resident cost** — ~1.5GB of |
| 968 | signatures and the scanning CPU leave a control-plane box, permanently, for a |
| 969 | feature idle by default. That is the whole benefit. It **costs doubled network |
| 970 | traffic**, since the console still fetches from S3 and streams onward (§9.4), |
| 971 | and it **does not remove tenant bytes from the console**; they still transit |
| 972 | it. |
| 973 | |
| 974 | A resource-placement decision, worth doing, worth stating at that size. |
| 975 | |
| 976 | ### 9.3 Reversing the prior decision |
| 977 | |
| 978 | Prior spec §4.1: *"No TCP option in v1: an unauthenticated scanning service |
| 979 | reachable over a network is a worse problem than the one being solved."* |
| 980 | |
| 981 | That objection is right and is not answered by wanting a services box. It is |
| 982 | answered by the network: |
| 983 | |
| 984 | - **clamd has no authentication whatsoever** |
| 985 | - private interface only (Tailscale or equivalent), **never** a public port, |
| 986 | and the services box firewalls the port to the console's address |
| 987 | - a hard constraint on the infra ask, not a recommendation |
| 988 | |
| 989 | `internal/clamd` has one `"unix"` literal (`clamd.go:130`); framing, deadlines |
| 990 | and parsing are transport-independent. `New` gains a network parameter, and |
| 991 | `CARLOS_CLAMD_ADDR` beginning with `/` means unix. |
| 992 | |
| 993 | ### 9.4 The console keeps fetching |
| 994 | |
| 995 | Rather than giving the services box S3 read and tag permissions. Bytes move |
| 996 | twice; tenant-data credentials stay on one box. Revisit with numbers. |
| 997 | |
| 998 | --- |
| 999 | |
| 1000 | ## 10. Surface |
| 1001 | |
| 1002 | ``` |
| 1003 | carlos system antivirus status|enable|disable|rotate-webhook-key |
| 1004 | carlos store scan request|status member, per app (shipped) |
| 1005 | carlos store scan grant|revoke|sweep|enforce operator, per app (shipped) |
| 1006 | ``` |
| 1007 | |
| 1008 | App-facing HTTP, role `Service`: |
| 1009 | |
| 1010 | ``` |
| 1011 | POST .../store/scan/object request a scan |
| 1012 | GET .../store/scan/object?key=&wait= read the verdict |
| 1013 | ``` |
| 1014 | |
| 1015 | `store scan status` gains queue depth and the deployment state (§4.5); |
| 1016 | `system antivirus status` gains queue depth deployment-wide. |
| 1017 | |
| 1018 | --- |
| 1019 | |
| 1020 | ## 11. Security properties |
| 1021 | |
| 1022 | Each is a test. |
| 1023 | |
| 1024 | 1. No callback reaches a host the app was not assigned — absolute URLs, |
| 1025 | `//host`, `/\host`, `@`-authority, traversal, CRLF, and encoded variants. |
| 1026 | 2. No redirect is followed. |
| 1027 | 3. A tampered body fails; app A's signature does not verify for app B; a stale |
| 1028 | timestamp is refused; both masters verify during rotation. **These are |
| 1029 | properties of the APP's verifier**, which is tenant code — so the console |
| 1030 | ships `VerifyCallback(key, header, body, now)` as the reference |
| 1031 | implementation, the docs reproduce it, and these tests run against it. |
| 1032 | Without that, the test tests nothing anybody runs. The signature is over the |
| 1033 | RAW BODY BYTES: an app that re-serialises the JSON before verifying will |
| 1034 | fail, and the docs say so. |
| 1035 | 4. The app policy still has no `s3:PutObjectTagging` (`aws.go:152-165`). |
| 1036 | 5. A role-`Service` credential cannot be minted account-scoped (§3.4) and |
| 1037 | cannot act on another app. (Admin reaches these doors by construction, |
| 1038 | §3.2 — this property is about `Service`.) |
| 1039 | 6. A `Service` credential cannot reach `system antivirus`, `scan grant`, |
| 1040 | `revoke`, `sweep` or `enforce`. |
| 1041 | 7. Version comparison is inside the scanner; no door or app sees the token. |
| 1042 | 8. Enqueue and drain both enforce the consent chain (§5.1.1). |
| 1043 | 9. Rate limits precede enqueue; a re-request of a key judged **clean, infected |
| 1044 | or skipped** costs a tag read and no fetch. (`failed` and `denied` are |
| 1045 | deliberately rescanned — §7.4.) |
| 1046 | 10. A verdict that could not be persisted is reported as `recorded:false`, |
| 1047 | never as `pending` and never as a silent success (§5.2.1); and a `failed` |
| 1048 | or `denied` verdict is reported as itself, never as `pending` (§5.2). |
| 1049 | |
| 1050 | --- |
| 1051 | |
| 1052 | ## 12. Out of scope |
| 1053 | |
| 1054 | Synchronous scan on PUT (CARLOS does not see the PUT); Gotenberg as a |
| 1055 | capability (the role is designed to be reused, the rendering service is its own |
| 1056 | brainstorm); GuardDuty (gated on the tagging measurement); pricing. |
| 1057 | |
| 1058 | --- |
| 1059 | |
| 1060 | ## 13. Testing |
| 1061 | |
| 1062 | - callback host and path validation as a table of hostile inputs; redirects |
| 1063 | refused |
| 1064 | - signature: tamper, cross-app, replay, skew, two-master rotation |
| 1065 | - `pending` for a stale tag, never the stale verdict; `gone` for a missing key |
| 1066 | - **the consent chain**: enqueue and drain both refuse `not_requested`, |
| 1067 | `encrypted`, `not_enabled`, `disabled`, and engine drift |
| 1068 | - one app flooding the queue does not delay another's item beyond one tick |
| 1069 | - a dropped callback leaves the verdict readable through the status door |
| 1070 | - `system antivirus disable` stops scanning without a restart and without |
| 1071 | touching per-app records — and per-app `scan status` then reports the |
| 1072 | deployment state rather than claiming health |
| 1073 | - `request`, `revoke`, `enforce off` still work while disabled |
| 1074 | - boot accepts `CARLOS_STORE_SCANNER=clamav` as a deprecated wiring alias, |
| 1075 | retaining the socket default; any other value stays fatal (§4.2.1) |
| 1076 | - the **first pass tick** — not boot — seeds the record and warns when a |
| 1077 | Scanner is wired with no record |
| 1078 | - a `failed` and a `denied` verdict each answer as themselves, not `pending` |
| 1079 | - unknown `system/antivirus.json` schema refuses |
| 1080 | - metering counts on-demand scans and not sweep scans |
| 1081 | - **the index**: no item strands while the process stays up (enqueue racing a |
| 1082 | drain's removal); **an app flooded past the per-tick cap is fully drained |
| 1083 | across successive ticks** — the test that would have caught review 3's |
| 1084 | blocker; an item written before a crash is drained after restart; TTL'd items |
| 1085 | are collected by the boot rebuild |
| 1086 | - an enqueue racing `PurgeApp` leaves no orphan for longer than one drain tick |
| 1087 | - a verdict reached but not persisted answers `recorded:false`, not `pending` |
| 1088 | (§5.2.1) |
| 1089 | - **a result object written for v1 does not answer for v2** — the door |
| 1090 | compares versions and says `pending` |
| 1091 | - a result object is not written under a prefix purged mid-scan, and is |
| 1092 | collected by the boot rebuild once past TTL |
| 1093 | - the drain rescans an object whose tag says `failed` or `denied`, rather than |
| 1094 | answering from the tag (§7.4) |
| 1095 | - `enable` 503s when no Scanner is wired |
| 1096 | - the seed happens once from the retired variable and does **not** override an |
| 1097 | existing record (§4.2.1) |
| 1098 | - a second result object for the same key **replaces** the first (§5.2.1), and |
| 1099 | does not fail create-only |
| 1100 | - the drain keeps items and the index entry on a record **read error**, and |
| 1101 | deletes only on absence |
| 1102 | |
| 1103 | --- |
| 1104 | |
| 1105 | ## 14. Risks |
| 1106 | |
| 1107 | - **The index is soft state, and losing an entry loses the callback.** The |
| 1108 | fallback is the ordinary sweep, which re-sweeps a completed bucket only every |
| 1109 | `ScanInterval` — **24h by default** (`scanpass.go:31,171`), not hourly. The |
| 1110 | object gets a verdict eventually and a polling app sees it; the webhook is |
| 1111 | simply not sent, and the queue item is collected at the next boot. Test by |
| 1112 | killing the console mid-queue. |
| 1113 | - **Boot rebuild cost.** One full enumeration at start on a deployment with |
| 1114 | many apps. Measure it. |
| 1115 | - **Two new object kinds under the app prefix cost something.** `storagePass` |
| 1116 | (`economicspass.go:163`) folds queue and result bytes into the tenant's |
| 1117 | `BucketBytes`, and the three full `List("apps/")` passes |
| 1118 | (`provision/reconcile.go:586,1050`, `domains/reconcile.go:87`, |
| 1119 | `materialize.go:292`) grow with a flooded queue until the TTL collects it. |
| 1120 | `provision` and `domains` skip unknown key shapes correctly, so for them |
| 1121 | this is cost, not breakage. **`materialize` does not, by design**: it counts |
| 1122 | *any* key under an app prefix (`materialize.go:333-345`, "convergence fails |
| 1123 | toward KEEPING the row") and restores a trashed row to live when it finds one |
| 1124 | (`:377-382`). So a result object stranded under a purged prefix does not only |
| 1125 | block the claim gate — it **resurrects the app row** on the next boot. That |
| 1126 | is the safe direction for materialize, and one more reason §5.2.1's purge |
| 1127 | guard is not optional. |
| 1128 | - **Index size is bounded** by the number of apps that have enqueued since boot |
| 1129 | and not yet been seen empty — at most the number of provisioned stores, a few |
| 1130 | dozen bytes each. Trashed and purged apps clear on their next empty listing, |
| 1131 | since `PurgeApp` deletes the items (`trash.go:229-238`). |
| 1132 | - **Long-poll occupancy.** Bounded wait, global and per-app waiter caps. |
| 1133 | |
amadan