| index 35c38b601..00bff4ce8 100644 |
| --- a/docs/superpowers/specs/2026-10-04-scanner-host-rollout-design.md |
| +++ b/docs/superpowers/specs/2026-10-04-scanner-host-rollout-design.md |
| @@ -1,6 +1,7 @@ |
| # Scanner host: getting object-storage scanning running on Carloku and Tito |
| |
| -**Status:** draft 1, design agreed in conversation 2026-10-04. |
| +**Status:** draft 2. Closes Astra review 1 (`CODEX-FINDINGS-SCANNER-HOST.md`, |
| +five findings, all accepted; marked **(codex N)**). |
| **Builds on:** `2026-09-01-object-storage-virus-scanning-design.md` (the sweep, |
| verdict tags, enforcement) and `2026-09-02-scan-on-demand-design.md` §9 (clamd |
| off the console box, reachable over TCP). |
| @@ -20,7 +21,8 @@ path (`2eaf2aae`, in every release since v0.18.0). |
| |
| What neither deployment has is a clamd to talk to, permission for the console |
| to read and tag store objects, or a scan tag key. This spec supplies those three |
| -things and nothing else. **No platform Go code changes.** |
| +things, plus one small client change (§4.3) that stops a partly scanned archive |
| +being signed clean. **(codex 4)** |
| |
| ## 2. Decisions |
| |
| @@ -33,6 +35,7 @@ things and nothing else. **No platform Go code changes.** |
| | D5 | Both pieces are platform modules | Carloku consumes the platform the way a self-hoster does. Hand-written root resources would be a workaround and would duplicate clamd's setup per root. | |
| | D6 | Carloku first, then Tito | Tito is a third party, and its console was only unpinned on 2026-10-03 with #201 phase 2 pending. Prove it on our own flagship first. | |
| | D7 | Scope ends at "engine wired, switched on, no member granted" | Rollout plan steps 7–8: a member grant follows a member's request and the disclosure, never an operator's speculation. | |
| +| D8 | An object clamd only partly scanned is recorded **unscanned**, never clean or infected | A signed clean verdict over bytes nobody read is the failure the verdict design exists to prevent; flagging large legitimate archives infected would make enforce block them. Unscanned fits the existing audit arithmetic (`unscanned = skipped + denied + failed`). **(Paul, 2026-10-04, on codex 4)** | |
| |
| **Out of scope:** the unmerged scan-on-demand API (`worktree-store-scan-on-demand`), |
| TLS to clamd, alerting on signature age, GuardDuty, per-region scanning. If |
| @@ -58,38 +61,66 @@ which reopens D4. That is a new decision, not more boxes. |
| lands in the client security group's VPC (`vpc_id` from a data source on that |
| group), so the two can never end up in different VPCs. |
| - **IAM role and instance profile**: `AmazonSSMManagedInstanceCore` attached, |
| - nothing else. No bucket, no parameter, no KMS. The box cannot fetch tenant |
| - data even if clamd were compromised. |
| + for Session Manager. That managed policy grants `ssm:GetParameter*` on `*`, |
| + which on its own would let the box read `/carlos/scan-tag-key` and |
| + `/carlos/release-key`. So the role also carries an explicit Deny on |
| + `ssm:GetParameter`, `GetParameters`, `GetParameterHistory` and |
| + `GetParametersByPath` on `*`, the same shape as `region-host`'s |
| + `host_ssm_parameter_scope` (`main.tf:418-450`) with no allowed parameter at |
| + all. Session Manager's channel actions are untouched. No bucket, no KMS. The |
| + box cannot fetch tenant data or the key even if clamd were compromised. |
| + **(codex 1)** |
| |
| ### 3.2 `user_data` |
| |
| -A templated script (`scanner-init.sh.tftpl`): |
| - |
| -1. Install `clamav-daemon` and `clamav-freshclam` from Debian. |
| -2. Run `freshclam` once in the foreground and **refuse to continue** until |
| - `main.cvd` (or `.cld`), `daily` and `bytecode` exist. clamd never starts on an |
| - empty database. |
| -3. Set these keys in `/etc/clamav/clamd.conf` (clamd has no include |
| - directory, so the script rewrites each key idempotently, replacing any |
| - existing line): |
| +A templated script (`scanner-init.sh.tftpl`), `set -eu`, each step logged to |
| +the serial console so `aws ec2 get-console-output` shows how far it got. |
| + |
| +1. **SSM agent first.** The AMI is the base Debian image `region-host` boots |
| + from (`outputs.tf:26`), not a configured host, and Debian ships no SSM agent. |
| + Install the arm64 `amazon-ssm-agent` `.deb` from the regional bucket and |
| + enable it, exactly as `host-init.sh.tftpl` does. With SSH closed this is the |
| + only way onto the box, so it goes before anything that can fail. A |
| + `region` variable feeds the URL. **(codex 5)** |
| +2. **Install without starting.** Write a `policy-rc.d` that exits 101, then |
| + `apt-get install clamav-daemon clamav-freshclam`, then remove the |
| + `policy-rc.d`. Debian's postinst would otherwise start `clamav-freshclam` |
| + (`clamav-freshclam.postinst.in:333`) and the socket-activated daemon. |
| + **(codex 3)** |
| +3. **Set `clamd.conf`.** clamd has no include directory, so the script rewrites |
| + each key idempotently, replacing any existing line: |
| - `TCPSocket 3310` |
| - `TCPAddr <private IPv4>` from IMDSv2. **Never `0.0.0.0`.** The security |
| group is the fence; binding the private address is the second rail. |
| - - `StreamMaxLength <stream_max_bytes>`, plus `MaxScanSize` and `MaxFileSize` |
| - no lower than it, so clamd never silently truncates a stream the console |
| - believes it sent whole. |
| + - `LocalSocket /run/clamav/clamd.ctl` stays, for local checks over SSM. |
| + - `StreamMaxLength <stream_max_bytes>`, and `MaxFileSize` no lower than it, |
| + so a stream the console sends whole is never truncated as a single file. |
| + - `AlertExceedsMax yes`. With the default `no`, an archive that expands past |
| + `MaxScanSize`, `MaxFileSize`, `MaxFiles` or `MaxRecursion` is partly |
| + scanned and answered `OK`. With `yes` clamd answers |
| + `Heuristics.Limits.Exceeded.<limit> FOUND`, which §4.3 records as |
| + unscanned (D8). **(codex 4)** |
| - `ConcurrentDatabaseReload` from a variable, default `yes`. |
| - - The unix socket `LocalSocket /run/clamav/clamd.ctl` stays, for local checks |
| - over SSM. |
| -4. A systemd drop-in for `clamav-daemon.service`: `MemoryMax` from a variable |
| - (default `3G`), `MemoryHigh` at about 85% of it, `Restart=on-failure`. |
| -5. `freshclam` keeps its Debian service (checks hourly by default). |
| -6. Debian ships `clamav-daemon.socket` for systemd socket activation, which |
| - passes clamd only the unix socket. Mask the socket unit so clamd binds both |
| - listeners from its own config; the check below fails if TCP is not up. |
| -7. Enable and start `clamav-daemon`; finish by checking `zVERSION` answers on |
| - both the socket and `<private ip>:3310`, and print the result to the console |
| - log so `aws ec2 get-console-output` shows it. |
| +4. **Replace the daemon's unit.** Debian's `clamav-daemon.service` carries |
| + `Requires=clamav-daemon.socket` and `Also=clamav-daemon.socket`, and socket |
| + activation hands clamd only the unix socket. Masking the socket alone leaves |
| + a service that cannot start. So: stop and mask `clamav-daemon.socket`, then |
| + install a **full replacement** `/etc/systemd/system/clamav-daemon.service` |
| + (not a drop-in): no socket dependency, no `Also=`, the package's |
| + `ConditionPathExistsGlob` database checks kept, `ExecStart=/usr/sbin/clamd |
| + --foreground=true`, `ExecReload` as packaged, `MemoryMax` from a variable |
| + (default `3G`), `MemoryHigh` at about 85% of it, `Restart=on-failure`, |
| + `RuntimeDirectory=clamav` owned by `clamav`. `systemctl daemon-reload`. |
| + **(codex 2)** |
| +5. **First signatures.** `freshclam` once in the foreground, with no |
| + `clamav-freshclam` service running (step 2 kept it stopped), so the two never |
| + contend for the lock or the update log. Refuse to continue unless `main`, |
| + `daily` and `bytecode` (`.cvd` or `.cld`) exist. Then enable and start |
| + `clamav-freshclam` (hourly checks by default). **(codex 3)** |
| +6. **Start clamd** and verify: `zVERSION` answers on `/run/clamav/clamd.ctl` |
| + **and** on `<private ip>:3310`; `ss -ltn` shows 3310 bound to the private IP |
| + only, never `0.0.0.0` or `[::]`. Any failure exits non-zero and logs which |
| + check failed. Print the `zVERSION` line last. |
| |
| ### 3.3 Variables and outputs |
| |
| @@ -103,6 +134,7 @@ A templated script (`scanner-init.sh.tftpl`): |
| | `memory_max` | `"3G"` | clamd's cgroup ceiling | |
| | `concurrent_database_reload` | `true` | | |
| | `subnet_id` | `null` | | |
| +| `region` | — | For the SSM agent download URL | |
| |
| Validation: `instance_type` beginning `t4g.small`/`t4g.micro`/`t4g.nano` with |
| `concurrent_database_reload = true` is refused with a message naming the reload |
| @@ -142,9 +174,32 @@ actions on `arn:aws:s3:::carlos-store-tito-*/*` and |
| `arn:aws:s3:::titogo-uploads-665106592142/*`. Tito keeps its own store policy |
| (#201 settled this), so `store_scanning` is not used there. |
| |
| +### 4.3 The client change: limits exceeded is unscanned (D8) |
| + |
| +`internal/clamd/clamd.go` `parseReply` (`:273`) maps any `FOUND` to |
| +`StatusInfected`. With `AlertExceedsMax yes`, a limit hit arrives as |
| +`stream: Heuristics.Limits.Exceeded.MaxScanSize FOUND` (likewise `.MaxFileSize`, |
| +`.MaxFiles`, `.MaxRecursion`, `.MaxScanTime`). Add one case **before** the |
| +`FOUND` case: a `FOUND` reply whose signature begins |
| +`Heuristics.Limits.Exceeded` becomes `StatusSizeExceeded`, keeping the raw reply. |
| +`objstore/clamav.go:374-383` already maps `StatusSizeExceeded` to |
| +`VerdictSkipped`, so it lands in `skipped` and `unscanned`, never in `clean` or |
| +`infected`, and nothing else changes. |
| + |
| +Tests in `clamd_test.go`: each of the five limit names maps to size-exceeded; |
| +`Eicar-Signature FOUND` still maps to infected; a signature that merely |
| +contains the phrase elsewhere (`Win.Heuristics.Limits.Exceeded.Fake`) still |
| +maps to infected, so the match is a prefix on the signature, not a substring of |
| +the reply. The docs for `carlos store scan status` that explain `skipped` gain |
| +one sentence naming over-limit archives. |
| + |
| +Because this is console code, **both consoles roll to a build containing it |
| +before they are wired** (§6.2). A console without it, wired to a clamd with |
| +`AlertExceedsMax yes`, would record over-limit archives as infected. |
| + |
| ## 5. Release and consumption |
| |
| -- The module change and `scanner-host` land on amadan main through a branch and |
| +- The module change, `scanner-host` and the §4.3 client change land on amadan main through a branch and |
| `amadan branch merge -squash`, then the GitHub mirror is pushed forward |
| (fast-forward checked first), then **v0.23.0** is cut per the README's |
| "Releasing the platform" recipe and pushed to the mirror. |
| @@ -175,6 +230,11 @@ actions on `arn:aws:s3:::carlos-store-tito-*/*` and |
| |
| ### 6.2 The console's environment |
| |
| +**Precondition:** the console runs v0.23.0 or a later main build (§4.3). On |
| +Carloku that is `carlos ship` plus `carlos promote --app console` in |
| +direct-bucket mode, boxes untouched (the console app is independent of the box |
| +binary). On Tito it is the same, from Tito's bucket. |
| + |
| First confirm over SSM, read-only, which file `console.service` loads. Then, |
| after a dated backup of that file: |
| |
| @@ -208,12 +268,16 @@ On a throwaway app we own, with a plaintext store: |
| `grant`, `sweep`. **Pass:** `scan status` shows a completed sweep, one clean, |
| one infected; both objects' tags verify under this deployment's key and carry |
| the object's current ETag. |
| -2. Overwrite the clean object, `sweep`. **Pass:** it is re-scanned, not carried |
| +2. Upload a small archive that expands past `MaxScanSize` (a zip of a few |
| + hundred MB of zeros is a few hundred KB) and `sweep`. **Pass:** it counts as |
| + `skipped` and `unscanned`, never `clean` or `infected`; its tag carries no |
| + clean verdict. **(codex 4)** |
| +3. Overwrite the clean object, `sweep`. **Pass:** it is re-scanned, not carried |
| forward. **If it is carried forward, stop the rollout:** a verdict that |
| outlives its bytes is worse than none. |
| -3. `enforce on`. Remove `CARLOS_CLAMD_ADDR`, restart. **Pass:** `enforce off` |
| +4. `enforce on`. Remove `CARLOS_CLAMD_ADDR`, restart. **Pass:** `enforce off` |
| still works with no engine built. Restore the address, restart. |
| -4. Destroy the app. **Pass:** no `scan.json` remains under its prefix and the |
| +5. Destroy the app. **Pass:** no `scan.json` remains under its prefix and the |
| name is claimable at once. |
| |
| Then stop. Scanning stays enabled; no member store is granted. |
| @@ -221,8 +285,8 @@ Then stop. Scanning stays enabled; no member store is granted. |
| ## 8. Tito |
| |
| After §7 passes and after #201 phase 2 has landed (so two changes do not edit |
| -Tito's console environment on the same day): §5's Tito apply, §6 in Tito's |
| -account, `antivirus status`, `enable`. **Tito's proof stops at status and |
| +Tito's console environment on the same day): §5's Tito apply, the Tito console |
| +roll (§6.2's precondition), §6 in Tito's account, `antivirus status`, `enable`. **Tito's proof stops at status and |
| enable.** §7 exercises platform code identical to Carloku's, and a throwaway app |
| in a customer's deployment needs the customer's say-so. |
| |
amadan