amadan

carlos / platform Public

Never destroy a bucket another app holds; never resolve a ledger's first hour

Teardown reads the bucket's ownership ledger before it destroys anything
and leaves the bucket and its scanner alone when another app has an open
interval. That closes two data-loss paths: a re-run of a teardown that
already deleted its bucket and failed at the user, after which another
app was granted the name and created the bucket again (discussion 191,
which both round-four reviews said must be guarded, not noted), and two
apps an operator handed one adopted bucket. The app's own credentials
are still revoked and its interval closed; the last holder's teardown
deletes the bucket.

A ledger now answers only for hours that begin at or after the instant
it began vouching (Astra round 4, R2). A probe that finds no bucket and
no principal says nothing about the minutes before it looked, and the
report bills the hour as one line, so the mint hour resolved a new owner
for an earlier owner's transfer. A charged line in a store's first clock
hour now holds, like any other line with no provable owner.

The GranteeProber contract says what an implementation must cover
(every kind of principal, inline and managed policies, bucket policies,
any resource pattern that matches the name) and that the fake's literal
match is not the contract (Opus N8). The no-inventory test's wrapper
keeps the bucket probe, so it tests the missing inventory and nothing
else.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
claude f60f547fb430519af57994fb96282cf188819d00 parent 5f2b4fa
4 files changed, +261 −43
  • internal/objstore/ownership.go +31 −14
  • internal/objstore/ownership_test.go +173 −7
  • internal/objstore/provisioner.go +11 −0
  • internal/objstore/walk.go +46 −22
diff --git a/internal/objstore/ownership.go b/internal/objstore/ownership.go
index 2a8cf309a..01937f594 100644
--- a/internal/objstore/ownership.go
+++ b/internal/objstore/ownership.go
@@ -317,18 +317,38 @@ func CloseBucketOwner(ctx context.Context, s release.Store, bucket, acct, app st
})
}
+// openOwnerOtherThan returns an open interval naming an app other than
+// acct/app, if the ledger has one: someone else currently holds the bucket.
+func (b *BucketOwnership) openOwnerOtherThan(acct, app string) (OwnerInterval, bool) {
+ for _, iv := range b.Intervals {
+ if iv.open() && (iv.Account != acct || iv.App != app) {
+ return iv, true
+ }
+ }
+ return OwnerInterval{}, false
+}
+
// OwnerAt answers who owned the bucket for the whole UTC hour containing
-// hour. ok is true only when the ledger is complete (CompleteSince), at
-// least one interval touches the hour, and every interval that does names
-// the same account and app.
+// hour. ok is true only when the ledger is complete (CompleteSince), the
+// hour begins at or after the instant it began vouching, at least one
+// interval touches the hour, and every interval that does names the same
+// account and app.
+//
+// The hour the ledger began vouching IN does not resolve. The ledger was
+// created when a probe found no bucket and no principal naming it, and
+// that says nothing about the minutes before the probe: a store that held
+// the name earlier in the same hour, torn down by a build that left no
+// ledger, moved bytes the report bills in that same hourly line. Present
+// absence is not past absence, so the first hour the ledger answers for is
+// the first one that starts after it looked. A charged line in the hour
+// before that holds, like any other line with no provable owner.
//
-// An hour only partly covered by its one owner (the bucket was minted at
-// 10:20, or deleted at 10:40) still resolves to that owner, and only
-// because the ledger is complete: the bucket did not exist before it was
-// minted, the ledger was opened before any credential existed and closed
-// after the bucket was gone, so no one else could have moved bytes out of
-// it in the rest of that hour. For an adopted or backfilled bucket none of
-// that holds, which is why such a ledger resolves nothing.
+// A later hour only partly covered by its one owner (the bucket was
+// deleted at 10:40) still resolves to that owner, and only because the
+// ledger is complete: it was closed after the bucket and its credentials
+// were gone, and anyone granted the name afterwards has an interval of
+// their own that makes the hour shared. For an adopted or backfilled
+// bucket none of that holds, which is why such a ledger resolves nothing.
func (b *BucketOwnership) OwnerAt(hour time.Time) (acct, app string, ok bool) {
if b == nil || b.CompleteSince == nil {
// No ledger, or one that cannot vouch for having seen everyone.
@@ -336,10 +356,7 @@ func (b *BucketOwnership) OwnerAt(hour time.Time) (acct, app string, ok bool) {
}
from := hour.UTC().Truncate(time.Hour)
to := from.Add(time.Hour)
- // The hour the ledger began vouching in resolves too: it was created
- // while the bucket did not exist and nobody held the name, so nothing
- // could have left a bucket of that name earlier in the hour.
- if !b.CompleteSince.Before(to) {
+ if b.CompleteSince.After(from) {
return "", "", false
}
for _, iv := range b.Intervals {
diff --git a/internal/objstore/ownership_test.go b/internal/objstore/ownership_test.go
index 78909ed3c..a2ef3507e 100644
--- a/internal/objstore/ownership_test.go
+++ b/internal/objstore/ownership_test.go
@@ -115,7 +115,8 @@ func TestOwnerAt(t *testing.T) {
ok bool
}{
{time.Date(2026, 9, 3, 9, 0, 0, 0, time.UTC), false}, // before the store existed
- {time.Date(2026, 9, 3, 10, 0, 0, 0, time.UTC), true}, // provisioned at 10:20
+ {time.Date(2026, 9, 3, 10, 0, 0, 0, time.UTC), false}, // the ledger began vouching at 10:20, inside this hour
+ {time.Date(2026, 9, 3, 11, 0, 0, 0, time.UTC), true}, // the first hour that starts after it looked
{time.Date(2026, 9, 3, 12, 0, 0, 0, time.UTC), true}, // whole hour
{time.Date(2026, 9, 3, 14, 0, 0, 0, time.UTC), true}, // deleted at 14:40
{time.Date(2026, 9, 3, 15, 0, 0, 0, time.UTC), false}, // after deletion
@@ -156,8 +157,8 @@ func TestOwnerAtRefusesSharedHour(t *testing.T) {
if _, _, ok := owners.OwnerAt(ledgerBucket, time.Date(2026, 9, 3, 11, 0, 0, 0, time.UTC)); ok {
t.Fatal("the 11:00 hour has two owners and must not resolve")
}
- if acct, _, ok := owners.OwnerAt(ledgerBucket, time.Date(2026, 9, 3, 10, 0, 0, 0, time.UTC)); !ok || acct != "bab" {
- t.Fatal("the 10:00 hour belongs to bab alone")
+ if _, _, ok := owners.OwnerAt(ledgerBucket, time.Date(2026, 9, 3, 10, 0, 0, 0, time.UTC)); ok {
+ t.Fatal("the 10:00 hour is the one the ledger began vouching in and must not resolve")
}
if acct, _, ok := owners.OwnerAt(ledgerBucket, time.Date(2026, 9, 3, 13, 0, 0, 0, time.UTC)); !ok || acct != "bac" {
t.Fatal("the 13:00 hour belongs to bac alone")
@@ -367,8 +368,13 @@ func TestMintingGrantCreatesACompleteLedger(t *testing.T) {
if b.CompleteSince == nil || !b.CompleteSince.Equal(grantAt) {
t.Fatalf("complete_since = %v, want the grant time", b.CompleteSince)
}
- if acct, app, ok := b.OwnerAt(time.Date(2026, 9, 1, 8, 0, 0, 0, time.UTC)); !ok || acct != "bab" || app != "titogo" {
- t.Fatalf("mint hour: %q %q %v", acct, app, ok)
+ // The hour of the mint itself does not resolve: the probe says nothing
+ // about the twenty minutes before it looked. The next hour does.
+ if acct, _, ok := b.OwnerAt(time.Date(2026, 9, 1, 8, 0, 0, 0, time.UTC)); ok {
+ t.Fatalf("the mint hour resolved to %q", acct)
+ }
+ if acct, app, ok := b.OwnerAt(time.Date(2026, 9, 1, 9, 0, 0, 0, time.UTC)); !ok || acct != "bab" || app != "titogo" {
+ t.Fatalf("the hour after the mint: %q %q %v", acct, app, ok)
}
if _, _, ok := b.OwnerAt(time.Date(2026, 9, 1, 7, 0, 0, 0, time.UTC)); ok {
t.Fatal("an hour before the bucket existed resolved an owner")
@@ -737,10 +743,15 @@ func TestFailedLegacyTeardownLeavesARecordTheNextGrantInherits(t *testing.T) {
}
}
-// A provisioner that cannot list principals claims nothing. This is the
-// AWS provisioner today.
+// A provisioner that can say whether a bucket exists and cannot list
+// principals claims nothing. This is the AWS provisioner today. The wrapper
+// keeps the bucket probe, so the only thing missing is the inventory.
type noInventory struct{ Provisioner }
+func (n noInventory) BucketExists(ctx context.Context, name string) (bool, error) {
+ return n.Provisioner.(BucketProber).BucketExists(ctx, name)
+}
+
func TestGrantClaimsNothingWithoutAPrincipalInventory(t *testing.T) {
ctx := context.Background()
s := release.NewDirStore(t.TempDir())
@@ -753,6 +764,9 @@ func TestGrantClaimsNothingWithoutAPrincipalInventory(t *testing.T) {
t.Fatalf("claimed completeness with no way to list principals: %v", b.CompleteSince)
}
var _ GranteeProber = NewMemProvisioner("eu-west-1")
+ if _, ok := Provisioner(noInventory{NewMemProvisioner("eu-west-1")}).(BucketProber); !ok {
+ t.Fatal("fixture: the wrapper must still probe for the bucket")
+ }
if _, ok := Provisioner(&awsProvisioner{}).(GranteeProber); ok {
t.Fatal("the AWS provisioner gained a principal inventory: review the completeness claim and its IAM grant before relying on it")
}
@@ -781,4 +795,156 @@ func TestCompletenessDatesFromTheObservation(t *testing.T) {
if _, _, ok := b.OwnerAt(t0.Add(24 * time.Hour)); ok {
t.Fatal("resolved an owner for an hour before the ledger looked")
}
+ if _, _, ok := b.OwnerAt(rerun); ok {
+ t.Fatal("resolved an owner for the hour the ledger looked in")
+ }
+ if acct, _, ok := b.OwnerAt(rerun.Add(time.Hour)); !ok || acct != "bab" {
+ t.Fatalf("the hour after the observation: %q %v", acct, ok)
+ }
+}
+
+// Before ledgers existed B adopted the name A mints. B moved bytes out at
+// 10:05. An older build's teardown removed B's bucket and B's credentials
+// at 10:10 and left no ledger. A's minting grant runs at 10:40: both probes
+// honestly find nothing, and the ledger is complete from 10:40. The report
+// bills the 10:00 hour as one line. That line holds B's 10:05 transfer, and
+// nothing A's ledger saw can say otherwise, so the hour must not resolve
+// to A. The next hour is A's.
+func TestObservationHourDoesNotResolveToTheNewOwner(t *testing.T) {
+ ctx := context.Background()
+ s := release.NewDirStore(t.TempDir())
+ _, pub := testSealKeypair(t)
+ p := NewMemProvisioner("eu-west-1")
+ // B's whole life left nothing behind: no bucket, no user, no ledger.
+ declare(t, s, "bab", "titogo", "TITO_S3")
+ grantAt := time.Date(2026, 9, 3, 10, 40, 0, 0, time.UTC)
+ if _, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, "", grantAt); err != nil {
+ t.Fatal(err)
+ }
+ b := mustLedger(t, s, ledgerBucket)
+ if b.CompleteSince == nil || !b.CompleteSince.Equal(grantAt) {
+ t.Fatalf("fixture: complete_since = %v, want the grant instant", b.CompleteSince)
+ }
+ owners := BucketOwners{ledgerBucket: b}
+ if acct, _, ok := owners.OwnerAt(ledgerBucket, time.Date(2026, 9, 3, 10, 5, 0, 0, time.UTC)); ok {
+ t.Fatalf("transfer at 10:05, before the ledger looked at 10:40, resolved to %q", acct)
+ }
+ if acct, _, ok := owners.OwnerAt(ledgerBucket, time.Date(2026, 9, 3, 10, 59, 0, 0, time.UTC)); ok {
+ t.Fatalf("the rest of the observation hour resolved to %q: the report bills the hour as one line", acct)
+ }
+ if acct, _, ok := owners.OwnerAt(ledgerBucket, time.Date(2026, 9, 3, 11, 0, 0, 0, time.UTC)); !ok || acct != "bab" {
+ t.Fatalf("the 11:00 hour: %q %v, want the new owner", acct, ok)
+ }
+ // A ledger that began vouching exactly on the hour answers for that hour.
+ onHour := time.Date(2026, 9, 3, 12, 0, 0, 0, time.UTC)
+ exact := &BucketOwnership{V: 1, Bucket: ledgerBucket, CompleteSince: &onHour, Intervals: []OwnerInterval{{Account: "bab", App: "titogo", From: onHour}}}
+ if _, _, ok := exact.OwnerAt(onHour); !ok {
+ t.Fatal("a ledger complete from 12:00 exactly did not resolve the 12:00 hour")
+ }
+}
+
+// The retried teardown of amadan discussion 191. B adopted the name A
+// mints. B's teardown deleted the bucket and failed to delete B's user. A
+// was then granted the name, created the bucket again and wrote to it. B's
+// teardown is retried: it must finish B (user, interval, record) and leave
+// A's bucket and A's objects exactly as they are.
+func TestRetriedTeardownLeavesABucketAnotherAppNowHolds(t *testing.T) {
+ ctx := context.Background()
+ s := release.NewDirStore(t.TempDir())
+ _, pub := testSealKeypair(t)
+ p := NewMemProvisioner("eu-west-1")
+ declare(t, s, "bac", "other", "OTHER_S3")
+ if _, err := Grant(ctx, s, p, testDeployment(), pub, "bac", "other", "op", GrantSourceOperator, ledgerBucket, t0); err != nil {
+ t.Fatal(err)
+ }
+ p.FailNext = map[string]error{"DeleteUser": errors.New("iam had a bad moment")}
+ if err := Teardown(ctx, s, p, nil, "bac", "other"); err == nil {
+ t.Fatal("the injected DeleteUser failure did not stop the teardown")
+ }
+ p.FailNext = nil
+ declare(t, s, "bab", "titogo", "TITO_S3")
+ if _, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, "", t0.Add(72*time.Hour)); err != nil {
+ t.Fatal(err)
+ }
+ p.Buckets[ledgerBucket]["invoice.pdf"] = []byte("the new owner's data")
+ sc := NewMemScanner()
+ if err := sc.EnsureEnabled(ctx, ledgerBucket); err != nil {
+ t.Fatal(err)
+ }
+ enableScanning(t, s, "bac", "other")
+
+ if err := Teardown(ctx, s, p, sc, "bac", "other"); err != nil {
+ t.Fatalf("the retry: %v", err)
+ }
+ if got := string(p.Buckets[ledgerBucket]["invoice.pdf"]); got != "the new owner's data" {
+ t.Fatalf("the retried teardown destroyed the new owner's bucket (object now %q, bucket present %v)", got, p.Buckets[ledgerBucket] != nil)
+ }
+ if !sc.IsEnabled(ledgerBucket) {
+ t.Fatal("the retried teardown disabled scanning on the new owner's bucket")
+ }
+ if _, ok := p.Users[UserName("bac", "other")]; ok {
+ t.Fatal("the retried teardown left its own user alive")
+ }
+ if rec, _, err := Get(ctx, s, "bac", "other"); err != nil || rec != nil {
+ t.Fatalf("the retried teardown left its record: %+v %v", rec, err)
+ }
+ if _, ok := p.Users[UserName("bab", "titogo")]; !ok {
+ t.Fatal("the new owner's user was removed")
+ }
+ b := mustLedger(t, s, ledgerBucket)
+ open := 0
+ for _, iv := range b.Intervals {
+ if iv.Until == nil {
+ open++
+ if iv.Account != "bab" || iv.App != "titogo" {
+ t.Fatalf("an interval is still open for %s/%s", iv.Account, iv.App)
+ }
+ }
+ }
+ if open != 1 {
+ t.Fatalf("open intervals = %d, want only the new owner's: %+v", open, b.Intervals)
+ }
+ // The last holder's teardown takes the bucket with it.
+ if err := Teardown(ctx, s, p, sc, "bab", "titogo"); err != nil {
+ t.Fatal(err)
+ }
+ if _, exists := p.Buckets[ledgerBucket]; exists {
+ t.Fatal("the last holder's teardown left the bucket")
+ }
+}
+
+// Two apps an operator handed one adopted bucket. Tearing one down revokes
+// its credentials and leaves the bucket, and the other app's access, alone.
+// Tearing the other down then deletes the bucket.
+func TestTeardownOfOneSharerLeavesASharedBucket(t *testing.T) {
+ ctx := context.Background()
+ s := release.NewDirStore(t.TempDir())
+ _, pub := testSealKeypair(t)
+ p := NewMemProvisioner("eu-west-1")
+ const shared = "carlos-store-shared-archive"
+ for _, app := range []string{"one", "two"} {
+ declare(t, s, "bab", app, "ARCHIVE_S3")
+ if _, err := Grant(ctx, s, p, testDeployment(), pub, "bab", app, "op", GrantSourceOperator, shared, t0); err != nil {
+ t.Fatal(err)
+ }
+ }
+ p.Buckets[shared]["kept"] = []byte("still needed by two")
+ if err := Teardown(ctx, s, p, nil, "bab", "one"); err != nil {
+ t.Fatal(err)
+ }
+ if string(p.Buckets[shared]["kept"]) != "still needed by two" {
+ t.Fatal("tearing down one sharer destroyed the shared bucket")
+ }
+ if _, ok := p.Users[UserName("bab", "two")]; !ok {
+ t.Fatal("the other sharer lost its user")
+ }
+ if _, ok := p.Users[UserName("bab", "one")]; ok {
+ t.Fatal("the torn-down sharer kept its user")
+ }
+ if err := Teardown(ctx, s, p, nil, "bab", "two"); err != nil {
+ t.Fatal(err)
+ }
+ if _, exists := p.Buckets[shared]; exists {
+ t.Fatal("the last sharer's teardown left the bucket")
+ }
}
diff --git a/internal/objstore/provisioner.go b/internal/objstore/provisioner.go
index 5580fc0f0..59e1c3b29 100644
--- a/internal/objstore/provisioner.go
+++ b/internal/objstore/provisioner.go
@@ -50,6 +50,17 @@ type BucketProber interface {
// moment it is created again. Only an inventory of principals can show
// that nobody is waiting on a name.
//
+// What an implementation must cover, or it must not exist: every kind of
+// principal (users, roles and groups, not only the store users this package
+// creates), both inline and attached managed policies, any bucket policy on
+// the name, and every resource pattern that MATCHES the bucket name, not
+// only one that spells it. A wildcard or prefix grant ("carlos-store-*") on
+// a platform or legacy role reaches a bucket of this name as surely as a
+// literal ARN does. The in-memory fake matches the literal ARN in a store
+// user's inline policy, which is all it can hold; it is not the contract.
+// An implementation that cannot answer for all of these returns an error,
+// and an error claims nothing.
+//
// The AWS provisioner does not implement it yet: it needs to list the store
// users and read their policies, which the console's role is not granted
// today. Until it does, no bucket's ownership ledger claims completeness on
diff --git a/internal/objstore/walk.go b/internal/objstore/walk.go
index 439a42689..bd4ec4094 100644
--- a/internal/objstore/walk.go
+++ b/internal/objstore/walk.go
@@ -9,6 +9,7 @@ import (
"crypto/ecdh"
"errors"
"fmt"
+ "log"
"regexp"
"time"
@@ -538,6 +539,49 @@ func Teardown(ctx context.Context, s release.Store, p Provisioner, sc Scanner, a
default:
bucket = rec.Grant.Bucket
}
+ // The bucket's ownership ledger is read before anything is destroyed,
+ // for two reasons.
+ //
+ // A store provisioned before ledgers existed has none. It is recorded
+ // now, incomplete: if this teardown deletes the bucket and then fails to
+ // delete the user, the surviving key still names the bucket, and the
+ // open interval written here is the only record that it does. Anyone
+ // granted the name afterwards finds a ledger that claims nothing and
+ // shares its hours with this app. Only when no ledger exists at all: an
+ // existing one already says what it knows, and a re-run after the close
+ // below must not reopen it.
+ //
+ // And a bucket ANOTHER app holds is not this teardown's to destroy. The
+ // ledger shows an open interval for every app granted the name, so it
+ // catches both ways that happens: two apps an operator handed one
+ // adopted bucket, and a re-run of a teardown that already deleted its
+ // bucket and failed at the user, after which another app was granted the
+ // name and created the bucket again. Emptying and deleting it then would
+ // destroy that app's data; disabling its scanner would strip that app's
+ // protection. Both are skipped. This app's credentials are still revoked
+ // and its interval closed, and when the last holder is torn down, no
+ // other interval is open and the bucket goes with it.
+ //
+ // What this cannot see: a grant of the name that lands between this read
+ // and the delete below (the walk has no cross-process lock on a bucket
+ // name), and a holder provisioned before ledgers existed whose interval
+ // was never written.
+ sharedWith := ""
+ if bucket != "" && ValidBucketName(bucket) {
+ existing, _, lerr := ReadBucketOwnership(ctx, s, bucket)
+ if lerr != nil {
+ return fmt.Errorf("objstore: read ownership of %s: %w", bucket, lerr)
+ }
+ if existing == nil {
+ now := ownershipNow()
+ if err := OpenBucketOwner(ctx, s, bucket, acct, app, grantOwnershipFrom(rec.Grant, now), now, false); err != nil {
+ return fmt.Errorf("objstore: record ownership of %s: %w", bucket, err)
+ }
+ } else if other, ok := existing.openOwnerOtherThan(acct, app); ok {
+ sharedWith = other.Account + "/" + other.App
+ log.Printf("objstore: teardown of %s/%s leaves bucket %s and its scanner in place: %s holds it", acct, app, bucket, sharedWith)
+ }
+ }
// 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
@@ -548,7 +592,7 @@ func Teardown(ctx context.Context, s release.Store, p Provisioner, sc Scanner, a
// 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 rec.Scanning != nil && rec.Scanning.Enabled != nil && bucket != "" && sharedWith == "" {
if sc == nil {
return fmt.Errorf("objstore: %s/%s has scanning enabled and this deployment has no scanner configured (CARLOS_STORE_SCANNER)", acct, app)
}
@@ -560,27 +604,7 @@ func Teardown(ctx context.Context, s release.Store, p Provisioner, sc Scanner, a
// and validates a name before writing the block), so there is nothing
// nameable to destroy — but the deterministic user might exist, and the
// delivered vars might too, so only the bucket steps are skipped.
- // A store provisioned before ownership ledgers existed has none. Record
- // it, incomplete, BEFORE anything is destroyed: if this teardown deletes
- // the bucket and then fails to delete the user, the surviving key still
- // names the bucket, and the open interval written here is the only
- // record that it does. Anyone granted the name afterwards finds a
- // ledger that claims nothing and shares its hours with this app. Only
- // when no ledger exists at all: an existing one already says what it
- // knows, and a re-run after the close below must not reopen it.
- if bucket != "" && ValidBucketName(bucket) {
- existing, _, lerr := ReadBucketOwnership(ctx, s, bucket)
- if lerr != nil {
- return fmt.Errorf("objstore: read ownership of %s: %w", bucket, lerr)
- }
- if existing == nil {
- now := ownershipNow()
- if err := OpenBucketOwner(ctx, s, bucket, acct, app, grantOwnershipFrom(rec.Grant, now), now, false); err != nil {
- return fmt.Errorf("objstore: record ownership of %s: %w", bucket, err)
- }
- }
- }
- if bucket != "" {
+ if bucket != "" && sharedWith == "" {
// Empty before delete: a real backend refuses to delete a bucket with
// objects in it, and EmptyBucket is paginated and resumable for exactly
// the bucket too big to clear in one call.