| index a4c701a58..602391c56 100644 |
| --- a/docs/superpowers/plans/2026-09-04-scanning-to-deployable.md |
| +++ b/docs/superpowers/plans/2026-09-04-scanning-to-deployable.md |
| @@ -1,7 +1,7 @@ |
| # Getting scanning onto real infrastructure |
| |
| -**Status:** draft 2 — closes a Fable review of draft 1, which found three |
| -blockers and nineteen findings in total. Sequencing, not tasks. |
| +**Status:** draft 3 — closes a second Fable review, which found four blockers |
| +(two of them security) and seventeen findings. Sequencing, not tasks. |
| **Covers:** the branch stack from where it stands to a scanner running against |
| real objects, twice. |
| **Plans it sequences:** `2026-09-01-object-storage-virus-scanning.md` (the |
| @@ -14,9 +14,11 @@ changes the order of work already planned. |
| |
| **Draft 1's central argument was wrong and is gone.** It claimed three facts |
| could only be learned by deploying, and made that the reason to deploy the |
| -sweep first. Two of the three have since been answered without deploying |
| -anything, by a test that now runs on every push; the third never needed a |
| -deployment either. What survives is a better reason, and one draft 1 did not |
| +sweep first. The version question is now answered and pinned on every push. The |
| +tagging question is **answered for MinIO and still open for AWS** — the |
| +deployment actually being planned — which is a weaker sentence than draft 2's |
| +"two of the three have been answered" and the true one. The third never needed |
| +a deployment either. What survives is a better reason, and one draft 1 did not |
| have at all. |
| |
| --- |
| @@ -29,8 +31,8 @@ Draft 1 measured against a **stale local `main`**. Corrected: |
| |---|---| |
| | `origin/main` | `6a59497` (#386, path-based routing) | |
| | local `main` | `97eb53f` — **24 commits behind** | |
| -| `worktree-store-scan-on-demand` | **50 commits above `origin/main`** | |
| -| `worktree-store-virus-scanning` | 25 of those; a strict ancestor | |
| +| `worktree-store-scan-on-demand` | **57 commits above `origin/main`** | |
| +| `worktree-store-virus-scanning` | 24 of those; a strict ancestor | |
| | the sweep branch's pushed tip | `37d955d` — **local tip `e7236b7` is not pushed** | |
| |
| The sweep layer is complete: 14 tasks, inert until three separate things are |
| @@ -88,17 +90,33 @@ This one did not survive contact with a real server, and it is the reason draft |
| 1 exists at all. |
| |
| The anti-forgery property is that the console can write the `carlos-scan` tag |
| -and the app cannot. `storePolicyJSON` said so in capitals; AGENTS.md carries it |
| -as a standing rule; a purpose-built helper enforced it. **All true, and none of |
| -it sufficient** — because there are two ways to tag an object, and leaving |
| -`s3:PutObjectTagging` out of the Allow closes only the explicit call. The |
| -`x-amz-tagging` header on `PutObject` names no action at all, so no search for |
| -an action name could ever have found it. |
| +and the app cannot. `storePolicyJSON` said so in capitals, and a purpose-built |
| +helper enforced it. **Both true, and neither sufficient** — because there are |
| +two ways to tag an object, and leaving `s3:PutObjectTagging` out of the Allow |
| +closes only the explicit call. The `x-amz-tagging` header on `PutObject` names |
| +no action at all, so no search for an action name could ever have found it. |
| + |
| +(Draft 2 also said "AGENTS.md carries it as a standing rule", and so did |
| +`6fba2b3`'s commit message. **Both were false** — AGENTS.md had no mention of |
| +it. It does now, in the two-route form, and it says the absence of the |
| +permission is not the property.) |
| |
| Against MinIO, given the app policy verbatim, the pre-tagged upload **succeeded**. |
| An app could have marked its own malware clean, and the sweep — finding a |
| version-matching clean tag — would never have scanned it. |
| |
| +**And the fix had a second hole behind it**, which the same review found: |
| +closing "an app can mark itself clean" left "an app can make itself |
| +*unmarkable*". S3 caps an object at ten tags; `tag()` kept every foreign tag, |
| +appended our three, and on overflow returned `(false, nil)` silently. Upload |
| +malware with eight tags of your own and `carlos-scan=infected` can never be |
| +written — so the `enforce` Deny, which keys on that tag being *present*, never |
| +engages, and the object stays readable through the app's own credential. Every |
| +surface reports a system working normally: the sweep counts it reached, the |
| +result object says `recorded:false`, the status door answers unknown. Fixed at |
| +`63e80cc` — an infected verdict now evicts foreign tags to fit, clean still |
| +yields. |
| + |
| The obvious fix does not work: an explicit `Deny` on `s3:PutObjectTagging` left |
| the upload route wide open. Only denying `s3:PutObject` conditioned on |
| `s3:RequestObjectTagKeys` closed it. Fixed at `6fba2b3`, verified with the |
| @@ -126,28 +144,39 @@ guess. Draft 1 asked for `systemd-cgtop` and did not ask for throughput at all. |
| |
| Still split. **On different reasons.** |
| |
| -### 1. A 50-commit branch carrying two features is not a reviewable diff |
| +### 1. One branch carrying two features is not a reviewable diff |
| |
| -`origin/main` has moved 24 commits. AGENTS.md's stated purpose for the gate is |
| -that the PR is "the one place the branch's diff is read as a single piece" — 50 |
| -commits spanning a sweep and an on-demand queue is not that, and every task |
| -added makes it less so. |
| +AGENTS.md's stated purpose for the gate is that the PR is "the one place the |
| +branch's diff is read as a single piece". A branch spanning a sweep and an |
| +on-demand queue is not that, whatever the number — and the honest form of this |
| +reason is **two features, two PRs**, which needs no count. (Draft 2 argued from |
| +"50 commits"; that cuts both ways, since the on-demand half will pass 30 by |
| +task 17.) |
| |
| ### 2. Tito's half has a lead time we do not control |
| |
| `tito/carlos-infrastructure` **#6** — clamd unit, freshclam timer, cgroup limits, |
| the console's S3 permissions — is filed, detailed and **unanswered**. Nothing |
| -past rollout step 2 moves without it. Starting that clock now overlaps someone |
| -else's work with tasks 11–17 rather than queueing behind them. |
| +past rollout step 2 moves without it. |
| + |
| +Precisely: the split does not *start* that clock — filing #6 did. What it buys |
| +is that **if #6 is answered before tasks 11–17 finish, the sweep can deploy |
| +without waiting for them.** And the second ask of Tito (the console unit's env |
| +change and restart, Phase 2 step 4) is **not filed at all**, so half the wait is |
| +not yet on anyone's list. File it in Phase 0 or this reason is about half of |
| +what it claims. |
| + |
| +### 3. ~~The sweep is what ships the policy fix~~ — withdrawn |
| |
| -### 3. The sweep is what ships the policy fix |
| +Draft 2 called this the strongest of the three. It is not a reason at all, and |
| +the review was right to say so: **if the stack is not split, the policy fixes |
| +ship by construction**, on the branch that merges. "The sweep is what ships the |
| +fix" is only true *after* deciding the sweep ships alone, so it has zero weight |
| +on the split question. It is a **constraint on Phase 1**, not an argument for |
| +Phase 1, and it has moved there. |
| |
| -New, and the strongest of the three. `6fba2b3` closes a hole in the app policy |
| -that every store carries. It is on the on-demand branch; the sweep is what will |
| -be deployed first and what writes those policies. **The fix must be cherry-picked |
| -onto the sweep branch** (Phase 1), or the first deployment ships the sweep with |
| -the pre-`6fba2b3` policy — the exact configuration a self-hosted gateway would |
| -let an app forge against. |
| +Draft 2 also had it both ways — a *must* here and "a tidy-up" in the closing |
| +section. The decision stands on reasons 1 and 2, honestly, or not at all. |
| |
| ### What the split actually costs |
| |
| @@ -169,9 +198,14 @@ draft 1 did not mention it. See Phase 4. |
| there is no GitHub, no `amadan` remote, and `gh` cannot see it. Landing is |
| `amadan branch describe` + `amadan branch merge`, gated by `amadan ci status`. |
| Phase 1 cannot follow a document that describes a different repository. |
| +- [x] AGENTS.md gains the anti-forgery rule, in the two-route form. Draft 2 said |
| + "AGENTS.md carries it as a standing rule" and `6fba2b3`'s message said the |
| + same; **both were false** — AGENTS.md had no mention of it. Now it does, and |
| + it says that the absence of the permission is not the property. |
| - [ ] Zero known-failing tests, and a line in AGENTS.md that a known-failing |
| - test is a defect in its own right. This is the bullet that pays for the |
| - others. |
| + test is a defect in its own right — for the reason two sections up: the count |
| + was read instead of the names, and a week of red went unexamined. |
| +- [ ] File the second Tito ask (see reason 2). |
| |
| ## Phase 0.5 — the checks that never needed a deployment |
| |
| @@ -180,36 +214,68 @@ which delayed them behind another team. |
| |
| - [x] Head/List version agreement — a contract subtest, on every push. |
| - [x] The app cannot tag its own object, **by either route** — same test. |
| -- [ ] `x-amz-tagging` against **real AWS**, once, to confirm the documented |
| - behaviour matches the deployment Tito actually has. |
| -- [ ] The `enforce` Deny actually denies: an app-credential `GET` of an object |
| - tagged infected. Rollout step 6 exercises `enforce on`/`off` under engine |
| - removal and never once reads an infected object. If this fails the repair is |
| - a `storePolicyJSON` change — a rewrite of every existing store's policy on the |
| - next re-walk, which is itself a rollout event and must precede any member |
| - grant. |
| +- [ ] **THREE MORE TAGGING ROUTES, untested and each authorised as plain |
| + `s3:PutObject`.** "By either route" is two routes; the property has more: |
| + `CreateMultipartUpload` with `x-amz-tagging` (any upload over the SDK's |
| + multipart threshold), `CopyObject` with `x-amz-tagging-directive: REPLACE`, |
| + and — the one that matters most — a **POST-policy form upload with a |
| + `tagging` field**, which is the standard direct-from-browser pattern, i.e. |
| + exactly what an app storing user uploads does. Whether a gateway populates |
| + `s3:RequestObjectTagKeys` in those handlers is the same gateway-specific fact |
| + that already bit once. Three more contract subtests; cheap. |
| +- [ ] Add `s3:PutObjectVersionTagging`/`s3:DeleteObjectVersionTagging` to the |
| + first Deny. Nothing in `internal/objstore` ever calls `PutBucketVersioning`, |
| + so it costs nothing and closes the shape before a bucket is ever versioned. |
| +- [ ] `x-amz-tagging` against **real AWS** — and for all the routes above, not |
| + just the header on `PutObject`. AWS documents that one as requiring |
| + `s3:PutObjectTagging`; it is far less explicit about multipart and POST |
| + policy. |
| +- [ ] **The fixture must be checked against the policy, not commented at.** |
| + `d848c57` says the CI fixture "has to be the policy" and nothing enforces it; |
| + "verified against a live server" was true once, by hand. Move the policy out |
| + of the heredoc into a file the script `cat`s, and add a unit test comparing it |
| + semantically to `storePolicyJSON("arn:aws:s3:::contract-bucket", nil)`. |
| +- [ ] The `enforce` Deny actually denies — **and this needs no deployment |
| + either.** The contract fixture's scoped user *is* an app-shaped credential |
| + against a real IAM: root tags an object `carlos-scan=infected`, the fixture |
| + gains the `ExistingObjectTag` Deny, and the scoped `GetObject` must be |
| + `AccessDenied`. Then the variant that matters: a scoped `CopyObject` from the |
| + infected source with no tags, and a `GET` of the copy — on a gateway that does |
| + not evaluate the source's tags, that is a clean enforcement bypass. If any of |
| + this fails the repair is a `storePolicyJSON` change, which rewrites every |
| + existing store's policy on the next re-walk — itself a rollout event, and it |
| + must precede any member grant. |
| - [ ] clamd resident memory and reload spike, on any box with the package. |
| |
| -**How the app's credential is obtained** — draft 1 asked for these tests and |
| -never said. Delivered credentials are sealed to the box's key (`writeEnv`), so |
| -an operator cannot read them out of the bucket. Either run as an exec tenant we |
| -own, from inside its own process, or mint a second key for |
| -`carlos-store-<acct>-<app>` in IAM. Both are legitimate platform work; pick one |
| -before writing the step. |
| +Draft 2 said obtaining an app credential was "legitimate platform work; pick one |
| +before writing the step". That was draft 1's own mistake wearing a later date: |
| +none of these need a deployed app at all. They need a scoped credential against |
| +a real IAM, which the contract fixture already mints on every push. |
| |
| ## Phase 1 — land the sweep half |
| |
| **Half a day. Order matters and draft 1 had it wrong.** |
| |
| -- [ ] **Push the sweep branch's local tip.** `e7236b7` is not on the hub; |
| - `37d955d` is. Any CI read before this tests the wrong commit. |
| -- [ ] **Cherry-pick `3e0c990`** (the usage assertion) and **`6fba2b3`** (the |
| - anti-forgery policy fix) onto `worktree-store-virus-scanning`. The second is |
| - not optional — see decision reason 3. |
| -- [ ] **Rebase the sweep branch onto `origin/main`.** Draft 1 rebased only the |
| - on-demand branch, *after* the merge, which would have landed code CI never ran |
| - against #386. Trial merges are conflict-free today, so this is cheap — but it |
| - is not free, and it was missing. |
| +**Do not cherry-pick. Reorder, then advance the pointer.** Draft 2 listed two |
| +commits to pick and would have left the sweep branch **red twice**: |
| + |
| +- `6fba2b3` adds the contract subtest for the tagging routes, but the sweep |
| + branch's `.amadan/ci.d/30-minio-contract` fixture has no Deny statements — the |
| + upload succeeds and the MinIO gate fails. That needs `d848c57` too. |
| +- The sweep's `internal/console/apiscan_test.go` asserts `!Contains(policy, |
| + "Effect":"Deny")` after grant, which the unconditional anti-forgery Denys |
| + break. That needs `d6142ac` too. |
| + |
| +And `63e80cc` (the untaggable fix) is now a third. Picking a list that has to be |
| +kept complete by hand is the wrong shape: **rebase the on-demand commits so the |
| +fix commits precede them, then advance `worktree-store-virus-scanning` over |
| +them.** The on-demand branch stays a strict descendant, no patch is duplicated, |
| +and the last Phase 1 step stops depending on patch-id dedupe surviving an |
| +amend. |
| + |
| +- [ ] **Rebase onto `origin/main`.** Draft 1 rebased only the on-demand branch, |
| + *after* the merge, which would have landed code CI never ran against #386. |
| + Trial merges are conflict-free today — cheap, not free, and it was missing. |
| - [ ] Post-rebase, run the two places a textually clean merge can still be |
| semantically short: `internal/docsite` (the heading gate) and |
| `TestEveryClosedDoorRefusesAServiceCredential` (`closedDoors`). Both #385/#386 |
| @@ -218,7 +284,10 @@ before writing the step. |
| - [ ] `amadan branch describe` — the description is the PR body, and carries |
| what fourteen tasks and four review rounds decided, written for someone who |
| has read none of it. |
| -- [ ] Green on **that branch's pushed tip**, then merge. |
| +- [ ] Green on **that branch's pushed tip**, then merge. (Draft 2 opened Phase 1 |
| + by pushing `e7236b7` and then immediately rewrote it; any green that produced |
| + would have been exactly the misleading reading the bullet warns about. Push |
| + once, after the reorder.) |
| - [ ] Rebase `worktree-store-scan-on-demand` onto the new `main`. |
| |
| ## Phase 2 — deploy the sweep to Tito, and test it |
| @@ -250,7 +319,10 @@ is not there. |
| - [ ] **Overwrite a tagged-clean object** and confirm the fresh count rises. |
| |
| **Step 6 gains the rollback that does not roll back.** Teardown refuses when a |
| -record has scanning enabled and the scanner is nil (`walk.go:465`), so after |
| +record has scanning enabled and the scanner is nil |
| +(`internal/objstore/walk.go:465` — named, because |
| +`internal/console/appsettings.go:838` is a second, UI-level copy of the same |
| +refusal that step 6 also exercises), so after |
| unset-and-restart **no scanning-enabled app can be destroyed** until scanning is |
| re-enabled or revoked. The rollout plan's Rollback names only the enforce Deny. |
| Destroy an enabled app during rollback, observe the refusal, confirm `scan |
| @@ -264,9 +336,13 @@ the console unit's env change and restart for step 4 are also |
| |
| Sequence 11 → 12 → 13 → (14 → 15) → 16 → 17, with two changes: |
| |
| -- **Task 11 opens with the deferred refactor.** `queuePermitted` still returns |
| - `(rec, permitted bool, err error)`; the door and the drain must not be able to |
| - answer the consent chain differently. §5.1.1. |
| +- [x] **The deferred refactor is done** (`4e2bfb8`). `queuePermitted` returns |
| + `(*Record, QueueRefusal, error)`, and `Permanent()` is §7.5's delete-vs-keep |
| + split in the one place both callers read it from. §5.1.1's concern is closed. |
| +- [x] **The callback path validator is done** (`4846e42`), ahead of the door |
| + that accepts the field rather than four commits behind it. |
| +- [ ] The enqueue door itself: the route, the six refusals, the attestation |
| + hole, the Service-role gate, 429. |
| - **Task 12 delivers `QueueIndex.Depth()`**, deferred out of task 10. |
| |
| Phase 2's throughput numbers land in task 12's budget. |
| @@ -284,7 +360,11 @@ This code path has only ever run in tests. So: |
| - [ ] Deploy on-demand with `CARLOS_STORE_SCANNER` **still set**. |
| - [ ] Confirm `carlos system antivirus status` reports the record seeded by |
| migration. |
| -- [ ] *Then* swap to `CARLOS_CLAMD_ADDR`, and only then. |
| +- [ ] *Then* swap to `CARLOS_CLAMD_ADDR` — **removing `CARLOS_STORE_SCANNER` |
| + and `CARLOS_CLAMD_SOCKET` in the same edit.** `cmd/console/main.go` is fatal |
| + when `CARLOS_CLAMD_ADDR` and `CARLOS_CLAMD_SOCKET` are both set and disagree, |
| + and rollout step 4 sets the socket explicitly. Leave both and the console |
| + fatals, and the adopt pass rolls back to the old binary. |
| |
| Followed by: mint a Service credential, enqueue one object, watch it drain, |
| receive one signed callback, verify the signature app-side. Then the failures |
| @@ -308,10 +388,10 @@ section is not shipped twice. |
| |
| Not the timestamps — that is settled and pinned. The live ones: |
| |
| -- **`x-amz-tagging` on real AWS behaving like MinIO.** Then `6fba2b3` is not |
| - belt-and-braces but the only thing standing between an app and a forged clean |
| - verdict, and Phase 1's cherry-pick becomes the gating item for the whole |
| - rollout rather than a tidy-up. |
| +- **`x-amz-tagging` on real AWS behaving like MinIO**, by any of the routes in |
| + Phase 0.5. Then the policy fixes are not belt-and-braces on Tito but the only |
| + thing between an app and a forged clean verdict — which changes the urgency of |
| + Phase 0.5, not the shape of Phase 1: the fixes ride the first half either way. |
| - **The `enforce` Deny not denying.** A `storePolicyJSON` change rewrites every |
| existing store's policy on the next re-walk — a rollout event that must land |
| before the first member grant, not after it. |