amadan
scan: the blocker a final review found, and three tests that proved nothing
A last adversarial review of the finished feature. It cleared the parts that matter most — the consent chain, SSRF, the signing scheme, and concurrency under -race — and found one blocker that had to be fixed before a person is asked to merge. THE CALLBACK CLAIMED PROVENANCE IT DID NOT HAVE. drainOne stamped every outcome with the engine and signature database THIS TICK is configured with, before it knew whether the engine had run. On the branch that reuses an existing tag it returned them unchanged, and delivery SIGNED that into the body. The scenario is ordinary, not exotic. The hourly sweep tags an object clean under signature database 27100. A week later the app asks for a scan. The drain reads the tag, does not scan, and POSTs a signed body saying the object was judged by today's engine and today's database. No such judgement happened. The status door already refuses to make exactly this guess for a tag verdict — so the two surfaces disagreed, and the one lying was the signed audit record. That is the failure this whole feature exists to prevent: a scanner reporting coverage it cannot justify is worse than no scanner. Provenance is now filled in only where the engine actually ran. A verdict with no provenance is honest and useful; a verdict with invented provenance is not. The tag does carry its own timestamp, which readSignedTag parses and Status discards — widening that seam is how these fields get filled in truthfully, and it stays a separate change. Nothing caught it. TestDrainAppReusesAJudgedVerdict asserted the verdict and the Scanned flag and nothing else, so the whole objstore package went green over it. TestReusedVerdictCarriesNoProvenance is new, and TestScannedVerdictCarriesProvenance is its other half — without which the first passes against a drain that reports no provenance at all. THREE DELIVERY TESTS PROVED NOTHING, all the same way. TestNeverPostsUnsigned used a bare Server with no AppsDomain, so delivery returned at "no platform host" long before the signer was consulted: the test passed with the guard deleted. Worse, with the guard gone and a real host it nil-derefs and panics a worker — which is what the mutation now does, and why the rewritten test reaches that code at all. TestCallbackBodyCarriesRecorded marshalled a literal and inspected its JSON, which passes against a deliver() that hard-codes Recorded: true. That is the exact field the blocker above is about: it tells an app whether an infected verdict is backed by the enforcement deny. TestDeliveredSecretVerifiesTheHeaderItReceives is new and covers the round trip nothing did. The delivered secret is HEX, because EnvironmentFile values are text, while the MAC is keyed on the RAW 32 bytes. An author who feeds the environment string straight into HMAC fails every verification, and the natural next move is to stop verifying. The test asserts the key as delivered, hex-decoded, verifies the header the pool actually sent. ALSO FIXED, all from the same review: the long-poll now re-reads on its deadline instead of answering with the value captured before parking — its comment claimed it did and the code did not, and because the drain notifies only after a whole app page returns, an object scanned early in a busy page was answered `pending` by a waiter that had the answer available. Delivery's consent re-check ran on the pool's lifetime context with no deadline, so one hung release-store read held a worker for the life of the process. And a refused redirect was silent, which to the one person who can fix it is indistinguishable from success. Left as known and recorded rather than fixed: a tag verdict still answers without `at`, and `skipped` without a reason (§5.2 wants both; the Status seam does not carry them yet); metering counts a size-ceiling `skipped` as bytes, which is wrong but unpriced and should be fixed before pricing; and the long-poll checks consent once for up to 30 seconds of waiting, which answers from tags rather than bytes. One of my own mutations was mislabelled again — it moved the version token into the engine field and claimed to test the absence of a version field, which two other assertions already cover. Reported here rather than counted as a survivor.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
5 files changed,
+225
−12
internal/console/apiscanstatus.go+10 −0internal/console/scandeliver.go+16 −1internal/console/scandeliver_test.go+103 −9internal/objstore/queue.go+26 −2internal/objstore/queue_test.go+70 −0
CI cancelled — superseded by a newer push — run details
diff --git a/internal/console/apiscanstatus.go b/internal/console/apiscanstatus.go| index 032a6da2a..89f624b46 100644 |
| --- a/internal/console/apiscanstatus.go |
| +++ b/internal/console/apiscanstatus.go |
| @@ -228,6 +228,16 @@ func (s *Server) handleAPIScanObject(w http.ResponseWriter, r *http.Request) { |
| // notification, so a spurious wake costs one read and nothing |
| // else. |
| case <-deadline.C: |
| + // RE-READ, rather than answering with the value captured before |
| + // parking. The comment above promised this and the code did not |
| + // do it, which a review caught. It matters because the drain |
| + // notifies only after a whole app page returns — up to the drain's |
| + // per-app budget — so an object scanned early in a busy page would |
| + // have been answered `pending` by a waiter that had the answer |
| + // available the moment it looked again. |
| + if final, err := s.scanStatusOf(ctx, acct.Sqid, app, bucket, key); err == nil { |
| + out = final |
| + } |
| writeJSON(w, http.StatusOK, out) |
| return |
| case <-ctx.Done(): |
diff --git a/internal/console/scandeliver.go b/internal/console/scandeliver.go| index 00388b8a9..b9a734475 100644 |
| --- a/internal/console/scandeliver.go |
| +++ b/internal/console/scandeliver.go |
| @@ -175,7 +175,12 @@ func (p *scanDeliveryPool) deliver(job scanDeliveryJob) { |
| log.Printf("console: scan callback: %s/%s: %v", job.ref.Account, job.ref.App, err) |
| return |
| } |
| - rec, refusal, err := objstore.QueuePermitted(p.ctx, p.s.cfg.Releases, job.ref.Account, job.ref.App) |
| + // BOUNDED. p.ctx is the pool's lifetime context and carries no deadline of |
| + // its own, so a release store that stops answering would hold this worker |
| + // for the life of the process — a quarter of the pool per hung read. |
| + consentCtx, consentCancel := context.WithTimeout(p.ctx, storeTimeout) |
| + defer consentCancel() |
| + rec, refusal, err := objstore.QueuePermitted(consentCtx, p.s.cfg.Releases, job.ref.Account, job.ref.App) |
| if err != nil || refusal != objstore.RefusalNone || rec == nil || rec.Scanning == nil || rec.Scanning.WebhookGen != job.gen { |
| return |
| } |
| @@ -226,6 +231,16 @@ func (p *scanDeliveryPool) post(job scanDeliveryJob, target string, body []byte, |
| if res.StatusCode >= 200 && res.StatusCode < 300 { |
| return drainErr != nil, drainErr |
| } |
| + if res.StatusCode >= 300 && res.StatusCode < 400 { |
| + // SAID OUT LOUD, because silence here is indistinguishable from |
| + // success to the only person who can fix it. Redirects are refused by |
| + // design (a tenant answering 302 to an internal address is the attack), |
| + // but the ordinary cause is dull: a framework that redirects /hook to |
| + // /hook/ turns every callback into a refusal, and nothing anywhere |
| + // would otherwise say so. |
| + log.Printf("console: scan callback %s/%s: %d redirect refused — a callback path must answer directly", |
| + job.ref.Account, job.ref.App, res.StatusCode) |
| + } |
| retry := res.StatusCode == http.StatusTooManyRequests || res.StatusCode >= 500 |
| return retry, nil |
| } |
diff --git a/internal/console/scandeliver_test.go b/internal/console/scandeliver_test.go| index a9e4bb9cc..b52e83930 100644 |
| --- a/internal/console/scandeliver_test.go |
| +++ b/internal/console/scandeliver_test.go |
| @@ -2,6 +2,7 @@ package console |
| import ( |
| "context" |
| + "encoding/hex" |
| "encoding/json" |
| "errors" |
| "io" |
| @@ -145,16 +146,76 @@ func TestTransportDoesNotSkipTLSVerification(t *testing.T) { |
| } |
| } |
| +// TestNeverPostsUnsigned. The first version used a bare &Server{}, which has |
| +// no AppsDomain — so delivery returned at "no platform host" long before the |
| +// signer mattered, and the test passed with the signer guard deleted. Worse, |
| +// with the guard gone AND a real host it would nil-deref and panic a worker. |
| +// |
| +// The fixture therefore has a resolvable host and a real queued item, exactly |
| +// as a delivering deployment does, and only the signer is missing. |
| func TestNeverPostsUnsigned(t *testing.T) { |
| var calls atomic.Int32 |
| - s := &Server{cfg: Config{CallbackTransport: callbackRoundTripper(func(*http.Request) (*http.Response, error) { |
| + fx, _ := deliveryArmed(t, "code-deliver-unsigned", callbackRoundTripper(func(*http.Request) (*http.Response, error) { |
| calls.Add(1) |
| return callbackResponse(204), nil |
| - })}} |
| - p := &scanDeliveryPool{s: s} |
| - p.deliver(scanDeliveryJob{}) |
| - if calls.Load() != 0 { |
| - t.Fatal("made an unsigned request") |
| + })) |
| + fx.srv.cfg.CallbackSigner = nil // the one thing under test |
| + |
| + ctx, cancel := context.WithCancel(context.Background()) |
| + defer cancel() |
| + fx.srv.scanDelivery = newScanDeliveryPool(ctx, fx.srv) |
| + if err := fx.srv.scanDrainTick(ctx); err != nil { |
| + t.Fatal(err) |
| + } |
| + // Give any wrongly-made request time to arrive before concluding none did. |
| + time.Sleep(300 * time.Millisecond) |
| + if n := calls.Load(); n != 0 { |
| + t.Fatalf("made %d request(s) with no signer — an unsigned callback is the hole §6.3 exists to close", n) |
| + } |
| +} |
| + |
| +// TestDeliveredSecretVerifiesTheHeaderItReceives is the round trip nothing |
| +// covered, and the one an app author's first hour depends on. |
| +// |
| +// The secret handed to an app is HEX (EnvironmentFile values are text), while |
| +// the MAC is keyed on the RAW 32 bytes. An author who feeds the environment |
| +// string straight into HMAC gets a failure on every callback, and the natural |
| +// next move is to stop verifying. So this asserts the whole path: the key as |
| +// delivered, hex-decoded, verifies the header the pool actually sent. |
| +func TestDeliveredSecretVerifiesTheHeaderItReceives(t *testing.T) { |
| + type sent struct { |
| + header string |
| + body []byte |
| + } |
| + got := make(chan sent, 4) |
| + fx, _ := deliveryArmed(t, "code-deliver-roundtrip", callbackRoundTripper(func(r *http.Request) (*http.Response, error) { |
| + body, _ := io.ReadAll(r.Body) |
| + got <- sent{header: r.Header.Get("Carlos-Signature"), body: body} |
| + return callbackResponse(204), nil |
| + })) |
| + |
| + ctx, cancel := context.WithCancel(context.Background()) |
| + defer cancel() |
| + fx.srv.scanDelivery = newScanDeliveryPool(ctx, fx.srv) |
| + if err := fx.srv.scanDrainTick(ctx); err != nil { |
| + t.Fatal(err) |
| + } |
| + select { |
| + case s := <-got: |
| + if s.header == "" { |
| + t.Fatal("no Carlos-Signature header") |
| + } |
| + // Exactly what the app is given, and exactly what it must do with it. |
| + delivered := fx.srv.cfg.CallbackSigner.DerivedKey(fixtureAcct, fixtureApp, 0) |
| + key, err := hex.DecodeString(string(delivered)) |
| + if err != nil { |
| + t.Fatalf("the delivered secret is not hex: %v", err) |
| + } |
| + if err := VerifyCallback(key, s.header, s.body, time.Now()); err != nil { |
| + t.Fatalf("the delivered secret does not verify the header we sent: %v", err) |
| + } |
| + case <-time.After(5 * time.Second): |
| + t.Fatal("no callback was delivered") |
| } |
| } |
| @@ -200,10 +261,43 @@ func TestDroppedCallbackStillReadableFromStatusDoor(t *testing.T) { |
| } |
| } |
| +// TestCallbackBodyCarriesRecorded inspects the body the pool BUILDS, not one |
| +// the test marshals itself. The first version constructed a literal and |
| +// checked its JSON, which passes just as well against a deliver() that |
| +// hard-codes Recorded: true — and Recorded is the field that tells an app |
| +// whether an infected verdict is actually backed by the enforcement deny. |
| func TestCallbackBodyCarriesRecorded(t *testing.T) { |
| - b, err := json.Marshal(scanCallbackBody{Recorded: true}) |
| - if err != nil || !strings.Contains(string(b), `"recorded":true`) { |
| - t.Fatalf("body = %s, err = %v", b, err) |
| + got := make(chan map[string]any, 4) |
| + fx, sc := deliveryArmed(t, "code-deliver-recorded", callbackRoundTripper(func(r *http.Request) (*http.Response, error) { |
| + raw, _ := io.ReadAll(r.Body) |
| + var body map[string]any |
| + _ = json.Unmarshal(raw, &body) |
| + got <- body |
| + return callbackResponse(204), nil |
| + })) |
| + // The ten-tag case: a verdict is reached and cannot be written onto the |
| + // object, so the app must be told the deny is not in force. |
| + sc.Unrecordable("uploads/x.pdf") |
| + |
| + ctx, cancel := context.WithCancel(context.Background()) |
| + defer cancel() |
| + fx.srv.scanDelivery = newScanDeliveryPool(ctx, fx.srv) |
| + if err := fx.srv.scanDrainTick(ctx); err != nil { |
| + t.Fatal(err) |
| + } |
| + select { |
| + case body := <-got: |
| + if rec, ok := body["recorded"].(bool); !ok || rec { |
| + t.Fatalf("recorded = %v, want false — the verdict is not on the object, so no deny matches it", body["recorded"]) |
| + } |
| + if _, ok := body["version"]; ok { |
| + t.Errorf("body carries the opaque version token: %v", body) |
| + } |
| + if body["etag"] == "" || body["etag"] == nil { |
| + t.Errorf("body carries no etag, which is what an app matches on: %v", body) |
| + } |
| + case <-time.After(5 * time.Second): |
| + t.Fatal("no callback was delivered") |
| } |
| } |
diff --git a/internal/objstore/queue.go b/internal/objstore/queue.go| index 28666e556..1ddf279b3 100644 |
| --- a/internal/objstore/queue.go |
| +++ b/internal/objstore/queue.go |
| @@ -611,9 +611,13 @@ func drainOne(ctx context.Context, s release.Store, sc Scanner, acct, app, bucke |
| if err != nil { |
| return Drained{}, err |
| } |
| + // PROVENANCE IS NOT FILLED IN YET, and that is the whole of the fix below. |
| + // engine and signature describe the engine THIS TICK is configured with; |
| + // whether they describe the thing that judged this object is not known |
| + // until we know whether the engine ran. |
| out := Drained{ |
| Key: g.key, Version: obj.Version(), ETag: obj.ETag, Size: obj.Size, |
| - Engine: engine, Signature: signature, CallbackPaths: g.callbackPaths, |
| + CallbackPaths: g.callbackPaths, |
| } |
| // The drain applies the re-issue rule ITSELF: denied and failed are |
| @@ -622,7 +626,24 @@ func drainOne(ctx context.Context, s release.Store, sc Scanner, acct, app, bucke |
| // to improve on. |
| needsScan := verdict == VerdictUnknown || verdict == VerdictFailed || verdict == VerdictDenied |
| if !needsScan { |
| - // The object carries this verdict, at this version, right now. |
| + // The object carries this verdict, at this version, right now — but |
| + // NOTHING HERE KNOWS WHAT PRODUCED IT. Engine and signature stay |
| + // empty, and the reason is worth stating because filling them in is |
| + // the obvious-looking thing to do. |
| + // |
| + // The tag may have been written by the hourly sweep last week under a |
| + // different signature database, and this deployment's engine may have |
| + // been replaced since. Stamping today's provenance onto it produces a |
| + // SIGNED callback body naming a signature database that never judged |
| + // this object — the status door already refuses to make exactly that |
| + // guess for a tag verdict (apiscanstatus.go), so the two surfaces |
| + // would disagree and the one lying would be the audit record. |
| + // |
| + // A verdict with no provenance is honest and useful. A verdict with |
| + // invented provenance is the failure this whole feature exists to |
| + // avoid. The tag does carry its own `at`, which readSignedTag parses |
| + // and Status discards; widening that seam is how these fields get |
| + // filled in truthfully, and it is a separate change. |
| out.Verdict, out.Recorded = verdict, true |
| // And any fallback for this key is now superseded — §5.2.1 says the |
| // drain drops it as soon as a real verdict is persisted, and the |
| @@ -650,7 +671,10 @@ func drainOne(ctx context.Context, s release.Store, sc Scanner, acct, app, bucke |
| if err != nil { |
| return Drained{}, err |
| } |
| + // The engine ran, here, now — so this is the one place the configured |
| + // engine and signature database really do describe what judged the bytes. |
| out.Verdict, out.Recorded, out.Scanned = v, recorded, true |
| + out.Engine, out.Signature = engine, signature |
| if recorded { |
| // A real verdict is on the object now, so any fallback must go. |
diff --git a/internal/objstore/queue_test.go b/internal/objstore/queue_test.go| index f8f4384a1..30d90436a 100644 |
| --- a/internal/objstore/queue_test.go |
| +++ b/internal/objstore/queue_test.go |
| @@ -1223,3 +1223,73 @@ type deleteErrorStore struct { |
| } |
| func (d *deleteErrorStore) Delete(context.Context, string) error { return d.err } |
| + |
| +// TestReusedVerdictCarriesNoProvenance is the one an adversarial review found |
| +// missing, and the defect it pins was live: the outcome was stamped with the |
| +// engine and signature database THIS TICK is configured with, before anything |
| +// knew whether the engine had run. |
| +// |
| +// The scenario is ordinary. The hourly sweep tags an object clean under |
| +// signature database 27100. A week later the app asks for a scan; the drain |
| +// reads the tag, does not scan, and — before this — reported engine "clamav" |
| +// and today's database 27412. That outcome becomes a SIGNED callback body, |
| +// so the platform would have put its name to a claim about which database |
| +// judged an object when no such judgement happened. The status door already |
| +// refuses to make that guess, so the two surfaces disagreed and the signed |
| +// one was the one lying. |
| +// |
| +// Empty is the honest answer. A verdict with no provenance is useful; a |
| +// verdict with invented provenance is the failure the feature exists to stop. |
| +func TestReusedVerdictCarriesNoProvenance(t *testing.T) { |
| + ctx := context.Background() |
| + s, sc, idx, bucket := queueFixture(t, 1) |
| + |
| + // Pre-tag the object at its current version, as a sweep would have. |
| + obj := ScanObject{Key: objectName(0), ETag: "etag-" + objectName(0), Size: 10} |
| + status, seen, err := sc.Status(ctx, bucket, objectName(0)) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + _ = status |
| + obj = seen |
| + sc.Put(bucket, objectName(0), obj.Version(), VerdictClean) |
| + enqueueN(t, s, idx, 1) |
| + |
| + done, _, err := DrainApp(ctx, s, sc, idx, "bab", "titogo", 10, time.Now()) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if len(done) != 1 { |
| + t.Fatalf("outcomes = %+v, want one", done) |
| + } |
| + got := done[0] |
| + if got.Verdict != VerdictClean || !got.Recorded { |
| + t.Fatalf("outcome = %+v, want the tag's clean verdict", got) |
| + } |
| + if got.Scanned { |
| + t.Fatal("the engine ran for an object that already carried a verdict") |
| + } |
| + if got.Engine != "" || got.Signature != "" { |
| + t.Errorf("reused verdict claims engine %q signature %q — neither judged this object", |
| + got.Engine, got.Signature) |
| + } |
| +} |
| + |
| +// TestScannedVerdictCarriesProvenance is the other half, without which the |
| +// test above passes against a drain that never reports provenance at all. |
| +func TestScannedVerdictCarriesProvenance(t *testing.T) { |
| + ctx := context.Background() |
| + s, sc, idx, _ := queueFixture(t, 1) |
| + enqueueN(t, s, idx, 1) |
| + |
| + done, _, err := DrainApp(ctx, s, sc, idx, "bab", "titogo", 10, time.Now()) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if len(done) != 1 || !done[0].Scanned { |
| + t.Fatalf("outcomes = %+v, want one that was actually scanned", done) |
| + } |
| + if done[0].Engine == "" { |
| + t.Error("an outcome the engine really produced carries no engine name") |
| + } |
| +} |