carlos / platform Public

objstore: teardown releases the scanner before the bucket, and deletes the sidecar

A Malware Protection plan and its managed EventBridge rule outlive the
bucket they protect, counting against a non-adjustable per-region quota
— and once the record is gone nothing names them any more. So the
scanner is released while the record still exists, and a nil scanner
over an enabled record refuses rather than proceeding, exactly as a nil
provisioner already does.

The sidecar goes too. An orphan scan.json under a purged app's prefix is
not cosmetic: the app-name claim gate refuses any name whose prefix
still holds a key, rename is refused, and the box-side auto-purge never
reaches it because the tombstone is written last. The name would be dead
until someone went looking for the object.

Config gains StoreScanner, and the destroy door gets the matching
pre-check so the refusal arrives as its own message instead of the
generic "Could not destroy" flash.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Paul Campbell pushed by paul@keymail.dev 1 month ago 3f0d15506c33f99c520806055431f0c8bfa66ec0 parent f60f19e

6 files changed, +179 −15

internal/console/appsettings.go +11 −1
internal/console/ownership.go +1 −1
internal/console/server.go +11 −0
internal/objstore/teardown_scan_test.go +116 −0
internal/objstore/walk.go +28 −1
internal/objstore/walk_test.go +12 −12

diff --git a/internal/console/appsettings.go b/internal/console/appsettings.go

index 88a16d976..ba6703eab 100644

--- a/internal/console/appsettings.go

+++ b/internal/console/appsettings.go

@@ -831,6 +831,16 @@ func (s *Server) handleStoreDestroy(w http.ResponseWriter, r *http.Request) {

"this deployment has to tear it down.")

return

}

+ // The same shape for the scanner. objstore.Teardown refuses a store whose

+ // scanning is enabled when it has no engine to release it with — without

+ // this pre-check that refusal arrives as the generic "Could not destroy"

+ // flash below, which tells the member nothing they can act on.

+ if rec.Scanning != nil && rec.Scanning.Enabled != nil && s.cfg.StoreScanner == nil {

+ fail("“" + app + "” has virus scanning enabled and this console has no scanner configured " +

+ "(CARLOS_STORE_SCANNER), so it cannot release the scanning engine — nothing was destroyed. " +

+ "An operator of this deployment has to tear it down.")

+ return

+ }

if !s.requireFreshAuth(w, r, addr, raw) {

return

}

@@ -842,7 +852,7 @@ func (s *Server) handleStoreDestroy(w http.ResponseWriter, r *http.Request) {

// button says "Destroy store", and the success flash below names only

// the object store. Email teardown belongs to ownership.go's

// handlePurgeApp, which destroys the whole app and says so.

- if err := objstore.Teardown(ctx, s.cfg.Releases, s.cfg.StoreProvisioner, acct.Sqid, app); err != nil {

+ if err := objstore.Teardown(ctx, s.cfg.Releases, s.cfg.StoreProvisioner, s.cfg.StoreScanner, acct.Sqid, app); err != nil {

log.Printf("console: store destroy %s/%s: %v", acct.Sqid, app, err)

if errors.Is(err, release.ErrCASConflict) {

fail("The store changed while you confirmed — nothing was destroyed; reload and try again.")

diff --git a/internal/console/ownership.go b/internal/console/ownership.go

index cb8f847ee..9fc8450db 100644

--- a/internal/console/ownership.go

+++ b/internal/console/ownership.go

@@ -414,7 +414,7 @@ func (s *Server) handlePurgeApp(w http.ResponseWriter, r *http.Request) {

// and it must not be able to hold this handler open indefinitely.

tctx, cancel := context.WithTimeout(r.Context(), storeTeardownTimeout)

defer cancel()

- if terr := objstore.Teardown(tctx, s.cfg.Releases, s.cfg.StoreProvisioner, acct.Sqid, app); terr != nil {

+ if terr := objstore.Teardown(tctx, s.cfg.Releases, s.cfg.StoreProvisioner, s.cfg.StoreScanner, acct.Sqid, app); terr != nil {

log.Printf("console: purge store teardown %s/%s: %v", acct.Sqid, app, terr)

fail("Could not tear down “" + app + "”'s object store — nothing was deleted; try again.")

return

diff --git a/internal/console/server.go b/internal/console/server.go

index 1acdcf8bd..4b866b258 100644

--- a/internal/console/server.go

+++ b/internal/console/server.go

@@ -519,6 +519,17 @@ type Config struct {

// create buckets and mint keys refuse. The console holds the identity

// that MINTS app credentials and never an app's own.

StoreProvisioner objstore.Provisioner

+ // StoreScanner is the virus-scanning engine, built at boot from

+ // CARLOS_STORE_SCANNER. nil is the ordinary shape — scanning is opt-in and

+ // off by default — and it means the sweep never runs and the operator

+ // doors that enable an engine refuse. It does NOT stop a member RECORDING

+ // a request, for the same reason a nil StoreProvisioner does not stop a

+ // declaration: a member cannot see whether their deployment has one.

+ //

+ // Teardown needs it too. A store with scanning enabled must have its

+ // engine released before its bucket goes, so a nil scanner over an enabled

+ // record is a refusal rather than a skip.

+ StoreScanner objstore.Scanner

// StoreDeployment is the deployment-wide half of every store: which

// backend, which region and endpoint buckets are made in, and the prefix

// minted bucket names carry. Threaded into every walk so this config is

diff --git a/internal/objstore/teardown_scan_test.go b/internal/objstore/teardown_scan_test.go

new file mode 100644

index 000000000..5282d5330

--- /dev/null

+++ b/internal/objstore/teardown_scan_test.go

@@ -0,0 +1,116 @@

+package objstore

+import (

+ "context"

+ "errors"

+ "os"

+ "strings"

+ "testing"

+ "time"

+ "github.com/carlosframework/platform/internal/release"

+// enableScanning marks a provisioned store as scanned, the way the operator

+// grant door does.

+func enableScanning(t *testing.T, s release.Store, acct, app string) {

+ t.Helper()

+ ctx := context.Background()

+ rec, etag, err := Get(ctx, s, acct, app)

+ if err != nil || rec == nil {

+ t.Fatalf("re-read %s/%s: %+v, %v", acct, app, rec, err)

+ }

+ rec.Scanning = &ScanBlock{

+ Requested: &ScanRequested{At: time.Now().UTC(), By: "m@example.com"},

+ Enabled: &ScanEnabled{Engine: "clamav", At: time.Now().UTC(), By: "op@example.com"},

+ }

+ if _, err := Put(ctx, s, acct, app, rec, etag); err != nil {

+ t.Fatal(err)

+ }

+// The scanner is released BEFORE the bucket goes, while the record naming it

+// still exists. A plan that outlives its bucket counts against a

+// non-adjustable per-region quota with nothing left pointing at it.

+func TestTeardownDisablesScannerBeforeDeletingBucket(t *testing.T) {

+ ctx := context.Background()

+ s := release.NewDirStore(t.TempDir())

+ _, pub := testSealKeypair(t)

+ p := NewMemProvisioner("eu-west-1")

+ rec := teardownFixture(t, s, pub, p)

+ bucket := rec.Provisioned.Bucket

+ sc := NewMemScanner()

+ if err := sc.EnsureEnabled(ctx, bucket); err != nil {

+ t.Fatal(err)

+ }

+ enableScanning(t, s, "bab", "titogo")

+ if err := Teardown(ctx, s, p, sc, "bab", "titogo"); err != nil {

+ t.Fatal(err)

+ }

+ if sc.IsEnabled(bucket) {

+ t.Fatal("teardown left the scanning engine enabled on a destroyed bucket")

+ }

+// A teardown that cannot reach the engine must NOT go on to delete the record

+// — the record is the only thing naming what it failed to release. Same

+// posture as the nil-provisioner refusal.

+func TestTeardownRefusesNilScannerWhenEnabled(t *testing.T) {

+ ctx := context.Background()

+ s := release.NewDirStore(t.TempDir())

+ _, pub := testSealKeypair(t)

+ p := NewMemProvisioner("eu-west-1")

+ teardownFixture(t, s, pub, p)

+ enableScanning(t, s, "bab", "titogo")

+ err := Teardown(ctx, s, p, nil, "bab", "titogo")

+ if err == nil {

+ t.Fatal("teardown with a nil scanner over an enabled record must refuse")

+ }

+ if !strings.Contains(err.Error(), "CARLOS_STORE_SCANNER") {

+ t.Fatalf("the refusal must name the variable an operator can act on: %v", err)

+ }

+ // The refusal owes the record intact.

+ if rec, _, gerr := Get(ctx, s, "bab", "titogo"); gerr != nil || rec == nil {

+ t.Fatalf("a refused teardown destroyed the record: %+v, %v", rec, gerr)

+ }

+// A store that never used scanning tears down with no scanner at all — which

+// is every store on a deployment that has not opted in.

+func TestTeardownAllowsNilScannerWhenNotEnabled(t *testing.T) {

+ ctx := context.Background()

+ s := release.NewDirStore(t.TempDir())

+ _, pub := testSealKeypair(t)

+ p := NewMemProvisioner("eu-west-1")

+ teardownFixture(t, s, pub, p)

+ if err := Teardown(ctx, s, p, nil, "bab", "titogo"); err != nil {

+ t.Fatalf("teardown of an unscanned store: %v", err)

+ }

+// The sidecar goes with everything else. An orphan under a purged app's prefix

+// makes the app NAME unusable: the claim gate refuses a name whose prefix still

+// holds any key, and the box-side auto-purge never reaches it because the

+// tombstone is written last.

+func TestTeardownDeletesScanSidecar(t *testing.T) {

+ ctx := context.Background()

+ s := release.NewDirStore(t.TempDir())

+ _, pub := testSealKeypair(t)

+ p := NewMemProvisioner("eu-west-1")

+ teardownFixture(t, s, pub, p)

+ if _, err := PutSidecar(ctx, s, "bab", "titogo",

+ &ScanSidecar{LastComplete: &Sweep{Objects: 3, Scanned: 3, Engine: "clamav"}}, ""); err != nil {

+ t.Fatal(err)

+ }

+ if err := Teardown(ctx, s, p, NewMemScanner(), "bab", "titogo"); err != nil {

+ t.Fatal(err)

+ }

+ if _, _, err := s.Get(ctx, ScanKey("bab", "titogo")); !errors.Is(err, os.ErrNotExist) {

+ t.Fatalf("scan.json survived teardown: %v", err)

+ }

diff --git a/internal/objstore/walk.go b/internal/objstore/walk.go

index ff596cfc4..663b8b9e8 100644

--- a/internal/objstore/walk.go

+++ b/internal/objstore/walk.go

@@ -408,7 +408,7 @@ func RotateFinish(ctx context.Context, s release.Store, p Provisioner, acct, app

// it as trivially deletable would erase the only record naming that estate,

// which is precisely the failure §9's ordering rule exists to prevent. Only a

// record with NO grant at all — declared, never acted on — dies trivially.

-func Teardown(ctx context.Context, s release.Store, p Provisioner, acct, app string) error {

+func Teardown(ctx context.Context, s release.Store, p Provisioner, sc Scanner, acct, app string) error {

rec, _, err := Get(ctx, s, acct, app)

if err != nil {

return err

@@ -451,6 +451,24 @@ func Teardown(ctx context.Context, s release.Store, p Provisioner, acct, app str

default:

bucket = rec.Grant.Bucket

}

+ // The scanner goes BEFORE the bucket, while the record that names it still

+ // exists. A Malware Protection plan (and its managed EventBridge rule)

+ // outlives the bucket it protects, counting against a non-adjustable

+ // 25-buckets-per-region quota, and once the record is gone nothing names

+ // it any more — the read-first, delete-last failure this walk's ordering

+ // exists to prevent.

+ //

+ // A nil scanner over an enabled record REFUSES, for the same reason a nil

+ // provisioner does above: a teardown that cannot reach the engine must not

+ // go on to delete the only record of what it should have disabled.

+ if rec.Scanning != nil && rec.Scanning.Enabled != nil && bucket != "" {

+ if sc == nil {

+ return fmt.Errorf("objstore: %s/%s has scanning enabled and this deployment has no scanner configured (CARLOS_STORE_SCANNER)", acct, app)

+ }

+ if err := sc.EnsureDisabled(ctx, bucket); err != nil {

+ return fmt.Errorf("objstore: disable scanning on %s: %w", bucket, err)

+ }

// A grant block with no bucket cannot come from the walk (Grant resolves

// and validates a name before writing the block), so there is nothing

// nameable to destroy — but the deterministic user might exist, and the

@@ -512,6 +530,15 @@ func Teardown(ctx context.Context, s release.Store, p Provisioner, acct, app str

return fmt.Errorf("objstore: remove store config for %s/%s: %w", acct, app, err)

}

+ // The scan sidecar, before the record. It is the only other object under

+ // this app's prefix that this package writes, and an orphan left behind

+ // makes the app NAME unusable: the claim gate refuses a name whose prefix

+ // still holds any key, and the box-side auto-purge never reaches it

+ // because the tombstone goes last. Delete is idempotent on an absent key,

+ // so a store that was never swept costs one no-op.

+ if err := s.Delete(ctx, ScanKey(acct, app)); err != nil {

+ return fmt.Errorf("objstore: delete scan sidecar for %s/%s: %w", acct, app, err)

+ }

// Last (§9). Everything it named is gone.

return deleteRecordIfUnchanged(ctx, s, acct, app, startKeyID, startBucket)

}

diff --git a/internal/objstore/walk_test.go b/internal/objstore/walk_test.go

index 994589a6c..d99ca1c76 100644

--- a/internal/objstore/walk_test.go

+++ b/internal/objstore/walk_test.go

@@ -1018,7 +1018,7 @@ func TestTeardownFullSweep(t *testing.T) {

// would fail on a real backend, so the walk must empty it first.

p.Buckets[rec.Provisioned.Bucket]["uploads/one.txt"] = []byte("payload")

- if err := Teardown(ctx, s, p, "bab", "titogo"); err != nil {

+ if err := Teardown(ctx, s, p, nil, "bab", "titogo"); err != nil {

t.Fatal(err)

}

if len(p.Buckets) != 0 {

@@ -1049,7 +1049,7 @@ func TestTeardownResumable(t *testing.T) {

p.Buckets[rec.Provisioned.Bucket]["uploads/one.txt"] = []byte("payload")

p.FailNext[method] = errors.New("injected: " + method + " failed")

- if err := Teardown(ctx, s, p, "bab", "titogo"); err == nil {

+ if err := Teardown(ctx, s, p, nil, "bab", "titogo"); err == nil {

t.Fatalf("%s failure: Teardown returned no error", method)

}

// The record is the ONLY name for what is left standing (§9), so a

@@ -1062,7 +1062,7 @@ func TestTeardownResumable(t *testing.T) {

t.Fatalf("record coordinates changed after %s failure: %+v", method, half.Provisioned)

}

- if err := Teardown(ctx, s, p, "bab", "titogo"); err != nil {

+ if err := Teardown(ctx, s, p, nil, "bab", "titogo"); err != nil {

t.Fatalf("resume after %s failure: %v", method, err)

}

if len(p.Buckets) != 0 || len(p.Users) != 0 {

@@ -1087,7 +1087,7 @@ func TestTeardownDeclaredOnly(t *testing.T) {

p := NewMemProvisioner("eu-west-1")

declare(t, s, "bab", "titogo", "TITO_S3")

- if err := Teardown(ctx, s, p, "bab", "titogo"); err != nil {

+ if err := Teardown(ctx, s, p, nil, "bab", "titogo"); err != nil {

t.Fatal(err)

}

stored, _, err := Get(ctx, s, "bab", "titogo")

@@ -1110,7 +1110,7 @@ func TestTeardownWithNoProvisioner(t *testing.T) {

s := release.NewDirStore(t.TempDir())

declare(t, s, "bab", "titogo", "TITO_S3")

- if err := Teardown(ctx, s, nil, "bab", "titogo"); err != nil {

+ if err := Teardown(ctx, s, nil, nil, "bab", "titogo"); err != nil {

t.Fatalf("declared-only teardown with no provisioner: %v", err)

}

@@ -1124,7 +1124,7 @@ func TestTeardownWithNoProvisioner(t *testing.T) {

if _, err := Put(ctx, s, "bab", "titogo", rec, etag); err != nil {

t.Fatal(err)

}

- if err := Teardown(ctx, s, nil, "bab", "titogo"); err == nil {

+ if err := Teardown(ctx, s, nil, nil, "bab", "titogo"); err == nil {

t.Fatal("teardown of a granted store with no provisioner returned nil — it cannot have destroyed anything")

}

if stored, _, err := Get(ctx, s, "bab", "titogo"); err != nil || stored == nil || stored.Grant == nil {

@@ -1136,7 +1136,7 @@ func TestTeardownIdempotentOnAbsent(t *testing.T) {

ctx := context.Background()

s := release.NewDirStore(t.TempDir())

p := NewMemProvisioner("eu-west-1")

- if err := Teardown(ctx, s, p, "bab", "titogo"); err != nil {

+ if err := Teardown(ctx, s, p, nil, "bab", "titogo"); err != nil {

t.Fatalf("teardown with no record: %v", err)

}

if len(p.Buckets) != 0 || len(p.Users) != 0 {

@@ -1168,7 +1168,7 @@ func TestTeardownConfigObjectAbsent(t *testing.T) {

t.Fatal(err)

}

- if err := Teardown(ctx, s, p, "bab", "titogo"); err != nil {

+ if err := Teardown(ctx, s, p, nil, "bab", "titogo"); err != nil {

t.Fatalf("teardown with no config object: %v", err)

}

if len(p.Buckets) != 0 || len(p.Users) != 0 {

@@ -1231,7 +1231,7 @@ func TestTeardownCrashedGrantSweep(t *testing.T) {

bucket, _ := crashedGrant(t, s, p)

p.Buckets[bucket]["uploads/one.txt"] = []byte("payload")

- if err := Teardown(ctx, s, p, "bab", "titogo"); err != nil {

+ if err := Teardown(ctx, s, p, nil, "bab", "titogo"); err != nil {

t.Fatal(err)

}

if len(p.Buckets) != 0 || len(p.Users) != 0 {

@@ -1256,7 +1256,7 @@ func TestTeardownCrashedGrantResumable(t *testing.T) {

p.Buckets[bucket]["uploads/one.txt"] = []byte("payload")

p.FailNext["DeleteBucket"] = errors.New("injected: DeleteBucket failed")

- if err := Teardown(ctx, s, p, "bab", "titogo"); err == nil {

+ if err := Teardown(ctx, s, p, nil, "bab", "titogo"); err == nil {

t.Fatal("DeleteBucket failure: Teardown returned no error")

}

half, _, err := Get(ctx, s, "bab", "titogo")

@@ -1264,7 +1264,7 @@ func TestTeardownCrashedGrantResumable(t *testing.T) {

t.Fatalf("record after DeleteBucket failure = %+v, %v", half, err)

}

- if err := Teardown(ctx, s, p, "bab", "titogo"); err != nil {

+ if err := Teardown(ctx, s, p, nil, "bab", "titogo"); err != nil {

t.Fatalf("resume after DeleteBucket failure: %v", err)

}

if len(p.Buckets) != 0 || len(p.Users) != 0 {

@@ -1319,7 +1319,7 @@ func TestTeardownRefusesConcurrentRegrant(t *testing.T) {

}

- if err := Teardown(ctx, s, p, "bab", "titogo"); !errors.Is(err, release.ErrCASConflict) {

+ if err := Teardown(ctx, s, p, nil, "bab", "titogo"); !errors.Is(err, release.ErrCASConflict) {

t.Fatalf("teardown over a concurrent grant: %v, want ErrCASConflict", err)

}

stored, _, err := Get(ctx, inner, "bab", "titogo")