amadan

carlos / platform Public

Merge branch 'tenant-iso-objstore' into main

objstore: bucket and IAM user names can't collide across stores (#218, #191)
claude 2129e26ed9edc4b451f31ac52eca185b3e4e9c89 parent a6e4cd4parent 26d2bd0
9 files changed, +840 −11
  • internal/console/apistore.go +12 −0
  • internal/console/apistore_provision.go +4 −0
  • internal/console/apistore_test.go +26 −0
  • internal/console/instanceprivatestore.go +6 −0
  • internal/console/storelock.go +19 −7
  • internal/console/storelock_test.go +26 −0
  • internal/objstore/nameclaim.go +341 −0
  • internal/objstore/nameclaim_test.go +346 −0
  • internal/objstore/walk.go +60 −4

CI cancelled — superseded by a newer push — run details

diff --git a/internal/console/apistore.go b/internal/console/apistore.go
index 07019da19..4ff25ac19 100644
--- a/internal/console/apistore.go
+++ b/internal/console/apistore.go
@@ -590,6 +590,12 @@ func storeEnvSuffix(env string) string {
// ---- the walk doors: grant, rotate, finish -----------------------------------
+// storeNameTakenMsg answers objstore.ErrStoreNameTaken (#218). Bucket and IAM
+// user names join account, app and environment with "-", so app "shop" with
+// environment "eu" and app "shop-eu" would get one estate; the walk refuses
+// the second before creating anything, and the sentence says why.
+const storeNameTakenMsg = "another store already uses this store's bucket or IAM user name (app and environment names are joined with \"-\"). nothing was created"
+
// storeGrantTimeout bounds the grant walk. Longer than storeTimeout because a
// grant is not one round trip: it creates a bucket, an IAM principal with an
// inline policy, and a key, then writes config and the record. Bucket creation
@@ -809,6 +815,12 @@ func (s *Server) handleAPIStoreGrant(w http.ResponseWriter, r *http.Request) {
jsonError(w, http.StatusConflict,
"no store is declared for this app. run `carlos store create` first")
return
+ case errors.Is(err, objstore.ErrStoreNameTaken):
+ // Refused before anything was created (#218): the bucket, or the IAM
+ // user this store would be given, is already another store's.
+ log.Printf("console: api store grant %s/%s (bucket %q): %v", acct.Sqid, app, bucket, err)
+ jsonError(w, http.StatusConflict, storeNameTakenMsg)
+ return
case err != nil:
// Logged, not returned: a backend error carries bucket names, ARNs,
// endpoints and request ids. The client sentence promises exactly what
diff --git a/internal/console/apistore_provision.go b/internal/console/apistore_provision.go
index f57110aa1..e580bb993 100644
--- a/internal/console/apistore_provision.go
+++ b/internal/console/apistore_provision.go
@@ -61,6 +61,10 @@ func (s *Server) handleAPIStoreProvision(w http.ResponseWriter, r *http.Request)
case errors.Is(err, objstore.ErrNotDeclared):
jsonError(w, http.StatusConflict, "no store is declared for this app. run `carlos store create` first")
return
+ case errors.Is(err, objstore.ErrStoreNameTaken):
+ log.Printf("console: api store provision %s/%s: %v", acct.Sqid, app, err)
+ jsonError(w, http.StatusConflict, storeNameTakenMsg)
+ return
case err != nil:
log.Printf("console: api store provision %s/%s: %v", acct.Sqid, app, err)
jsonError(w, http.StatusBadGateway, "could not provision the store. re-run `carlos store create` to resume; after destroying a store, its bucket name may take time to become available again")
diff --git a/internal/console/apistore_test.go b/internal/console/apistore_test.go
index d0a91832f..018eb85cd 100644
--- a/internal/console/apistore_test.go
+++ b/internal/console/apistore_test.go
@@ -829,6 +829,32 @@ func TestAPIStoreGrantAdoptsANamedBucket(t *testing.T) {
auditedBy(t, fx, "titogo", "store-grant", "alice@keymail.dev", "titogo-uploads")
}
+// TestAPIStoreGrantRefusesAnotherStoresBucket (#218): adopting a bucket that
+// another app's store record already names would scope this app's credential
+// to that app's data — and tearing this store down later would delete it. The
+// door refuses before anything is created or recorded, including for a record
+// written before name claims existed, which holds no claim to find.
+func TestAPIStoreGrantRefusesAnotherStoresBucket(t *testing.T) {
+ fx, fake, token, _ := storeArmed(t, "code-store-grant-taken")
+ declareStore(t, fx, token)
+ putRecord(t, fx, "other", &objstore.Record{
+ Declared: &objstore.Declared{EnvPrefix: "OTHER_S3", At: time.Now(), By: "paul@keymail.dev"},
+ Grant: &objstore.GrantBlock{Backend: "mem", Bucket: "other-uploads", At: time.Now(), By: "op"},
+ })
+
+ status, out := postJSON(t, storeURL(fx, "titogo")+"/grant", map[string]any{"bucket": "other-uploads"}, token)
+ if status != http.StatusConflict {
+ t.Fatalf("adopting another store's bucket = %d %v, want 409", status, out)
+ }
+ if msg := str(t, out, "error"); !strings.Contains(msg, "another store") {
+ t.Fatalf("error = %q, want it to say the name belongs to another store", msg)
+ }
+ untouched(t, fake, "adopting another store's bucket")
+ if rec := storedRecord(t, fx, "titogo"); rec.Grant != nil {
+ t.Fatalf("a refused grant wrote a grant block: %+v", rec.Grant)
+ }
+}
+
// TestAPIStoreGrantRefusesAnInvalidBucketName: an adopt name is interpolated
// into the IAM policy ARN the credential is scoped by, so a bad one is refused
// AT THE DOOR — before the record is read and before a single backend call, so
diff --git a/internal/console/instanceprivatestore.go b/internal/console/instanceprivatestore.go
index b5a9d10aa..25736bf11 100644
--- a/internal/console/instanceprivatestore.go
+++ b/internal/console/instanceprivatestore.go
@@ -245,6 +245,12 @@ func (s *Server) instanceStoreProvision(ctx context.Context, w http.ResponseWrit
}
out, err := objstore.GrantIn(walkCtx, s.cfg.Releases, s.cfg.StoreProvisioner, s.cfg.StoreDeployment,
pub, acct.Sqid, app, env, e.Decision.By, objstore.GrantSourceAccountApproval, "", time.Now())
+ if errors.Is(err, objstore.ErrStoreNameTaken) {
+ log.Printf("console: instance store provision %s/%s/%s: %v", acct.Sqid, app, env, err)
+ jsonError(w, http.StatusConflict,
+ "another store already uses this instance's storage names (app and environment names are joined with \"-\"), so the instance was not created. use a different host name")
+ return nil, false
+ }
if err != nil {
// Logged, not returned: a walk error carries bucket names, IAM ARNs
// and request ids. The sentence the caller gets is the one they can
diff --git a/internal/console/storelock.go b/internal/console/storelock.go
index 588f94690..dbd1da778 100644
--- a/internal/console/storelock.go
+++ b/internal/console/storelock.go
@@ -1,6 +1,10 @@
package console
-import "sync"
+import (
+ "sync"
+
+ "github.com/carlosframework/platform/internal/objstore"
+)
// storeWalkLocks serialises the object-storage walks per app, which is what
// objstore has always said this package does.
@@ -52,19 +56,27 @@ func newStoreWalkLocks() *storeWalkLocks {
// on compiling while reading a refcount that could never move — they failed
// as "the waiter never registered", which is a sentence about the lock rather
// than about the key. Derive it here.
-func storeWalkKey(acct, app, env string) string { return acct + "/" + app + "/" + env }
+//
+// The key is the store's IAM user name rather than the triple spelled out,
+// because two different triples can resolve to ONE estate: app "shop" with
+// environment "eu" and app "shop-eu" mint the same bucket and the same user
+// (#218). objstore's name claims refuse the second of them whichever lock is
+// held, but walks over one estate are what this lock exists to serialise, so
+// it keys on the estate's name.
+func storeWalkKey(acct, app, env string) string { return objstore.UserNameIn(acct, app, env) }
// lock blocks until this store's walk lock is held, and returns the release.
// The caller defers it:
//
// defer s.storeWalks.lock(acct.Sqid, app, env)()
//
-// The key is the account, the app AND the environment ("" for the app's own
-// store): two apps in one account share no estate, and since 2026-09-19 two
-// STORES of one app share none either — a bucket, an IAM user and a config
-// bundle each, all named for the triple (objstore's MintedBucketIn and
+// The key covers the account, the app AND the environment ("" for the app's
+// own store): two apps in one account share no estate, and since 2026-09-19
+// two STORES of one app share none either — a bucket, an IAM user and a
+// config bundle each, all named for the triple (objstore's MintedBucketIn and
// UserNameIn). Serialising across any of them would cost concurrency and buy
-// nothing.
+// nothing. The one exception is triples whose joined names collide, which
+// storeWalkKey folds into one key.
func (l *storeWalkLocks) lock(acct, app, env string) func() {
key := storeWalkKey(acct, app, env)
l.mu.Lock()
diff --git a/internal/console/storelock_test.go b/internal/console/storelock_test.go
index ed8db43a4..7b55b6bf4 100644
--- a/internal/console/storelock_test.go
+++ b/internal/console/storelock_test.go
@@ -131,3 +131,29 @@ func TestStoreWalkLocksKeepEntryWhileWaitersQueue(t *testing.T) {
t.Fatalf("locks map holds %d entries after both released, want 0", n)
}
}
+
+// Except where the names collide (#218): app "shop" with environment "eu" and
+// app "shop-eu" resolve to one bucket and one IAM user, so their walks must
+// queue. The name claims refuse the second grant either way; the lock keeps
+// the two from interleaving their reads of each other's records.
+func TestStoreWalkLocksSerialiseCollidingStores(t *testing.T) {
+ l := newStoreWalkLocks()
+ release := l.lock("bab", "shop-eu", "")
+
+ acquired := make(chan struct{})
+ go func() {
+ l.lock("bab", "shop", "eu")()
+ close(acquired)
+ }()
+ select {
+ case <-acquired:
+ t.Fatal("a walk on a store whose names collide with this one's did not queue behind its lock")
+ case <-time.After(50 * time.Millisecond):
+ }
+ release()
+ select {
+ case <-acquired:
+ case <-time.After(2 * time.Second):
+ t.Fatal("the queued walk never got the lock")
+ }
+}
diff --git a/internal/objstore/nameclaim.go b/internal/objstore/nameclaim.go
new file mode 100644
index 000000000..cef513164
--- /dev/null
+++ b/internal/objstore/nameclaim.go
@@ -0,0 +1,341 @@
+// Name claims: one store per bucket name and per IAM user name (#218).
+//
+// MintedBucketIn and UserNameIn join account, app and environment with "-",
+// and app and environment names may both contain "-" — so app "shop" with
+// environment "eu" and app "shop-eu" resolve to ONE bucket and ONE user.
+// Nothing downstream notices: EnsureBucket takes BucketAlreadyOwnedByYou as
+// success, EnsureUser adopts an existing user, the grant walk's reap deletes
+// the other store's live key, and a later Teardown empties and deletes the
+// other store's bucket. An operator adopting a bucket by name reaches the
+// same place by a typo.
+//
+// The names themselves cannot change — production buckets and users carry
+// them — so the fix is to make a name belong to one store. Same discipline as
+// internal/mail/domainclaim.go: a create-only bucket key per name, GLOBAL
+// rather than per account (an adopted bucket name is not scoped to the
+// account that adopts it), whose write is the uniqueness check. Two walks
+// racing for one name cannot both win, in one console or in two, whichever
+// store was declared first.
+//
+// Stores granted before claims existed hold none. The walk backfills a claim
+// on its next run, and until then two checks stand in for it: the stores in
+// the same account whose names collide by construction are few and
+// computable (collidingStores), so they are read directly; and every first
+// grant scans every store record, which is the only way to find a name an
+// unclaimed record holds in some other account (an operator's adopt can be
+// shaped exactly like another account's minted name).
+package objstore
+
+import (
+ "context"
+ "encoding/json"
+ "errors"
+ "fmt"
+ "os"
+ "path"
+ "strings"
+ "time"
+
+ "github.com/carlosframework/platform/internal/appconfig"
+ "github.com/carlosframework/platform/internal/release"
+)
+
+// ErrStoreNameTaken refuses a grant whose bucket or IAM user name already
+// belongs to another store. It is raised before anything is created,
+// recorded or reaped.
+var ErrStoreNameTaken = errors.New("objstore: bucket or IAM user name belongs to another store")
+
+const nameClaimsPrefix = "objstore/name-claims/"
+
+func bucketClaimKey(bucket string) string { return nameClaimsPrefix + "buckets/" + bucket + ".json" }
+func userClaimKey(user string) string { return nameClaimsPrefix + "users/" + user + ".json" }
+
+// NameClaim is the wire shape at a claim key: which store holds the name.
+type NameClaim struct {
+ Schema int `json:"schema"` // 1
+ Name string `json:"name"`
+ Account string `json:"account"`
+ App string `json:"app"`
+ Environment string `json:"environment,omitempty"`
+ CreatedAt time.Time `json:"created_at"`
+}
+
+func (c *NameClaim) heldBy(acct, app, env string) bool {
+ return c.Account == acct && c.App == app && c.Environment == env
+}
+
+// getNameClaim reads a claim; absent is (nil, nil).
+func getNameClaim(ctx context.Context, s release.Store, key string) (*NameClaim, error) {
+ raw, _, err := s.Get(ctx, key)
+ if errors.Is(err, os.ErrNotExist) {
+ return nil, nil
+ }
+ if err != nil {
+ return nil, err
+ }
+ var c NameClaim
+ if err := json.Unmarshal(raw, &c); err != nil {
+ return nil, fmt.Errorf("objstore: name claim %s: %w", key, err)
+ }
+ if c.Schema > 1 {
+ return nil, fmt.Errorf("objstore: name claim %s: schema %d is newer than this binary understands — upgrade carlos", key, c.Schema)
+ }
+ return &c, nil
+}
+
+// claimName takes key for the store, or confirms the store already holds it.
+// The create-only Put is the check; a lost create race re-reads and judges
+// the winner, as claimDomain does.
+func claimName(ctx context.Context, s release.Store, key, name, acct, app, env string, now time.Time) error {
+ body, err := json.Marshal(NameClaim{Schema: 1, Name: name, Account: acct, App: app, Environment: env, CreatedAt: now})
+ if err != nil {
+ return err
+ }
+ for attempt := 0; ; attempt++ {
+ cur, err := getNameClaim(ctx, s, key)
+ if err != nil {
+ return err
+ }
+ if cur != nil {
+ if !cur.heldBy(acct, app, env) {
+ return fmt.Errorf("%w: %s", ErrStoreNameTaken, name)
+ }
+ return nil
+ }
+ _, err = s.Put(ctx, key, body, "")
+ if err == nil {
+ return nil
+ }
+ if !errors.Is(err, release.ErrCASConflict) || attempt > 0 {
+ return err
+ }
+ }
+}
+
+// releaseName deletes key only when this store holds it. A claim held by
+// another store is theirs, and an absent one is already released.
+func releaseName(ctx context.Context, s release.Store, key, acct, app, env string) error {
+ cur, err := getNameClaim(ctx, s, key)
+ if err != nil {
+ return err
+ }
+ if cur == nil || !cur.heldBy(acct, app, env) {
+ return nil
+ }
+ return s.Delete(ctx, key)
+}
+
+type storeRef struct{ app, env string }
+
+// collidingStores lists every OTHER store in the account whose minted bucket
+// and IAM user names are the same as this one's: every way of splitting
+// app[-env] at a hyphen into a valid app and an optional valid environment.
+func collidingStores(app, env string) []storeRef {
+ joined := app
+ if env != "" {
+ joined += "-" + env
+ }
+ var out []storeRef
+ for i := 0; i <= len(joined); i++ {
+ var ref storeRef
+ switch {
+ case i == len(joined):
+ ref = storeRef{joined, ""}
+ case joined[i] == '-':
+ ref = storeRef{joined[:i], joined[i+1:]}
+ default:
+ continue
+ }
+ if ref.app == app && ref.env == env {
+ continue
+ }
+ if !release.ValidApp(ref.app) || (ref.env != "" && !appconfig.ValidEnvironmentName(ref.env)) {
+ continue
+ }
+ out = append(out, ref)
+ }
+ return out
+}
+
+// grantedColliders reads the records of collidingStores and returns those a
+// grant has acted on — the ones holding the shared names, claimed or not.
+func grantedColliders(ctx context.Context, s release.Store, acct, app, env string) ([]*Record, error) {
+ var out []*Record
+ for _, ref := range collidingStores(app, env) {
+ rec, _, err := GetIn(ctx, s, acct, ref.app, ref.env)
+ if err != nil {
+ return nil, err
+ }
+ if rec != nil && (rec.Grant != nil || rec.Provisioned != nil) {
+ rec.App, rec.Environment = ref.app, ref.env // named by the key read, not the body
+ out = append(out, rec)
+ }
+ }
+ return out, nil
+}
+
+// recordNames is the bucket and user a record's estate is named by, read the
+// way Teardown reads them.
+func recordNames(acct string, rec *Record) (bucket, user string) {
+ user = UserNameIn(acct, rec.App, rec.Environment)
+ if rec.Grant != nil {
+ bucket = rec.Grant.Bucket
+ }
+ if rec.Provisioned != nil {
+ bucket, user = rec.Provisioned.Bucket, rec.Provisioned.IAMUser
+ }
+ return bucket, user
+}
+
+// storeNamingBucket scans every store record in the deployment for one other
+// than (acct, app, env) that names bucket. Every FIRST grant pays for it, not
+// only an operator's adopt: an adopt made before claims existed can hold a
+// name shaped exactly like another account's minted one (#191), and nothing
+// but the record says so. It walks with ListDirs, as mail.SweepLegacyRecords
+// does, so it never lists a release artifact; a re-run skips it, because by
+// then the walk holds the claim.
+func storeNamingBucket(ctx context.Context, s release.Store, bucket, acct, app, env string) (bool, error) {
+ accts, err := s.ListDirs(ctx, "apps/")
+ if err != nil {
+ return false, err
+ }
+ for _, acctPrefix := range accts {
+ a := path.Base(strings.TrimSuffix(acctPrefix, "/"))
+ if !release.ValidAccount(a) {
+ continue
+ }
+ apps, err := s.ListDirs(ctx, acctPrefix)
+ if err != nil {
+ return false, err
+ }
+ for _, appPrefix := range apps {
+ p := path.Base(strings.TrimSuffix(appPrefix, "/"))
+ if !release.ValidApp(p) {
+ continue
+ }
+ envs, err := StoreEnvironments(ctx, s, a, p)
+ if err != nil {
+ return false, err
+ }
+ for _, e := range append([]string{""}, envs...) {
+ if a == acct && p == app && e == env {
+ continue
+ }
+ rec, _, err := GetIn(ctx, s, a, p, e)
+ if err != nil {
+ return false, err
+ }
+ if rec == nil {
+ continue
+ }
+ if b, _ := recordNames(a, rec); b == bucket {
+ return true, nil
+ }
+ }
+ }
+ }
+ return false, nil
+}
+
+// claimStoreNames is the grant walk's gate, run before the grant block is
+// written and before any backend call. firstGrant is a grant with no block
+// yet — the only run that can be choosing a name another record holds.
+func claimStoreNames(ctx context.Context, s release.Store, acct, app, env, bucket, user string, firstGrant bool, now time.Time) error {
+ others, err := grantedColliders(ctx, s, acct, app, env)
+ if err != nil {
+ return err
+ }
+ if len(others) > 0 {
+ return fmt.Errorf("%w: %s is already the IAM user of %s/%s environment %q",
+ ErrStoreNameTaken, user, acct, others[0].App, others[0].Environment)
+ }
+ if firstGrant {
+ taken, err := storeNamingBucket(ctx, s, bucket, acct, app, env)
+ if err != nil {
+ return err
+ }
+ if taken {
+ return fmt.Errorf("%w: %s", ErrStoreNameTaken, bucket)
+ }
+ }
+ // Both are read before either is written, so a refusal on the second
+ // does not leave the first behind.
+ for _, key := range []string{userClaimKey(user), bucketClaimKey(bucket)} {
+ cur, err := getNameClaim(ctx, s, key)
+ if err != nil {
+ return err
+ }
+ if cur != nil && !cur.heldBy(acct, app, env) {
+ return fmt.Errorf("%w: %s", ErrStoreNameTaken, cur.Name)
+ }
+ }
+ if err := claimName(ctx, s, userClaimKey(user), user, acct, app, env, now); err != nil {
+ return err
+ }
+ return claimName(ctx, s, bucketClaimKey(bucket), bucket, acct, app, env, now)
+}
+
+// sharedNames reports, for Teardown, whether the bucket or the user it is
+// about to delete belongs to another store: by that store's claim, or — for
+// a pair granted before claims existed — by a colliding record that names it.
+// Either way the other store's estate is not this teardown's to destroy.
+func sharedNames(ctx context.Context, s release.Store, acct, app, env, bucket, user string) (bucketShared, userShared bool, err error) {
+ if bucket != "" {
+ c, err := getNameClaim(ctx, s, bucketClaimKey(bucket))
+ if err != nil {
+ return false, false, err
+ }
+ bucketShared = c != nil && !c.heldBy(acct, app, env)
+ }
+ c, err := getNameClaim(ctx, s, userClaimKey(user))
+ if err != nil {
+ return false, false, err
+ }
+ userShared = c != nil && !c.heldBy(acct, app, env)
+ others, err := grantedColliders(ctx, s, acct, app, env)
+ if err != nil {
+ return false, false, err
+ }
+ for _, o := range others {
+ b, u := recordNames(acct, o)
+ bucketShared = bucketShared || (bucket != "" && b == bucket)
+ userShared = userShared || u == user
+ }
+ return bucketShared, userShared, nil
+}
+
+// claimedBucket finds the bucket claim this store holds, for the one teardown
+// that has no record of the name: a grant that took its claims and died
+// before writing its block. Only a declared-only teardown pays for the
+// listing. "" means none.
+func claimedBucket(ctx context.Context, s release.Store, acct, app, env string) (string, error) {
+ keys, err := s.List(ctx, nameClaimsPrefix+"buckets/")
+ if err != nil {
+ return "", err
+ }
+ for _, key := range keys {
+ c, err := getNameClaim(ctx, s, key)
+ if err != nil {
+ return "", err
+ }
+ if c != nil && c.heldBy(acct, app, env) {
+ return c.Name, nil
+ }
+ }
+ return "", nil
+}
+
+// releaseStoreNames gives back whatever claims this store holds on bucket and
+// user. Owner-gated, so a name another store holds is never touched.
+func releaseStoreNames(ctx context.Context, s release.Store, acct, app, env, bucket, user string) error {
+ keys := []string{userClaimKey(user)}
+ if bucket != "" {
+ keys = append(keys, bucketClaimKey(bucket))
+ }
+ for _, key := range keys {
+ if err := releaseName(ctx, s, key, acct, app, env); err != nil {
+ return fmt.Errorf("objstore: release name claim %s: %w", key, err)
+ }
+ }
+ return nil
+}
diff --git a/internal/objstore/nameclaim_test.go b/internal/objstore/nameclaim_test.go
new file mode 100644
index 000000000..b1d805f74
--- /dev/null
+++ b/internal/objstore/nameclaim_test.go
@@ -0,0 +1,346 @@
+package objstore
+
+import (
+ "context"
+ "errors"
+ "testing"
+ "time"
+
+ "github.com/carlosframework/platform/internal/release"
+)
+
+// Bucket and IAM user names join account, app and environment with "-", and
+// app and environment names may both contain "-". So app "shop" with
+// environment "eu" and app "shop-eu" mint the SAME bucket and the SAME user
+// (#218). Everything here pins that two different stores can never end up
+// sharing either one, whichever is created first.
+
+// forgetNameClaims deletes every name claim, which is what a store granted
+// before claims existed looks like.
+func forgetNameClaims(t *testing.T, s release.Store) {
+ t.Helper()
+ ctx := context.Background()
+ keys, err := s.List(ctx, nameClaimsPrefix)
+ if err != nil {
+ t.Fatal(err)
+ }
+ for _, k := range keys {
+ if err := s.Delete(ctx, k); err != nil {
+ t.Fatal(err)
+ }
+ }
+}
+
+func grantStore(t *testing.T, s release.Store, p *MemProvisioner, acct, app, env, adopt string) (*Record, error) {
+ t.Helper()
+ _, pub := testSealKeypair(t)
+ return GrantIn(context.Background(), s, p, testDeployment(), pub, acct, app, env, "op", GrantSourceOperator, adopt, time.Now().UTC())
+}
+
+func TestCollidingStoresResolveToTheSameNames(t *testing.T) {
+ // The premise of every test below: if this ever stops being true the
+ // collision is gone and these tests are guarding nothing.
+ d := testDeployment()
+ if MintedBucketIn(d, "bab", "shop", "eu") != MintedBucketIn(d, "bab", "shop-eu", "") {
+ t.Fatal("bucket names no longer collide")
+ }
+ if UserNameIn("bab", "shop", "eu") != UserNameIn("bab", "shop-eu", "") {
+ t.Fatal("user names no longer collide")
+ }
+}
+
+// The attack in #218: shop-eu has a store; a token bound to shop creates an
+// environment "eu" store. It must be refused before it touches the estate —
+// no grant block, and shop-eu's key still alive.
+func TestGrantRefusesStoreThatCollidesWithAnExistingOne(t *testing.T) {
+ for _, legacy := range []bool{false, true} {
+ name := "claimed"
+ if legacy {
+ name = "granted-before-claims"
+ }
+ t.Run(name, func(t *testing.T) {
+ s := release.NewDirStore(t.TempDir())
+ p := NewMemProvisioner("eu-west-1")
+ declare(t, s, "bab", "shop-eu", "SHOP_S3")
+ victim, err := grantStore(t, s, p, "bab", "shop-eu", "", "")
+ if err != nil {
+ t.Fatal(err)
+ }
+ if legacy {
+ forgetNameClaims(t, s)
+ }
+ declareIn(t, s, "bab", "shop", "eu", "SHOP_S3")
+ _, err = grantStore(t, s, p, "bab", "shop", "eu", "")
+ if !errors.Is(err, ErrStoreNameTaken) {
+ t.Fatalf("colliding grant = %v, want ErrStoreNameTaken", err)
+ }
+ if _, ok := p.Users[victim.Provisioned.IAMUser].Keys[victim.Provisioned.KeyID]; !ok {
+ t.Error("the refused grant reaped the existing store's key")
+ }
+ rec, _, err := GetIn(context.Background(), s, "bab", "shop", "eu")
+ if err != nil {
+ t.Fatal(err)
+ }
+ if rec.Grant != nil {
+ t.Error("the refused grant wrote a grant block — Teardown would treat the other store's estate as its own")
+ }
+ // And the refused store's teardown leaves the other estate standing.
+ if err := TeardownIn(context.Background(), s, p, nil, "bab", "shop", "eu"); err != nil {
+ t.Fatal(err)
+ }
+ if _, ok := p.Buckets[victim.Provisioned.Bucket]; !ok {
+ t.Error("tearing down the refused store deleted the other store's bucket")
+ }
+ if _, ok := p.Users[victim.Provisioned.IAMUser]; !ok {
+ t.Error("tearing down the refused store deleted the other store's user")
+ }
+ })
+ }
+}
+
+// The same pair the other way round: the environment store first, then an
+// app named for the join.
+func TestGrantRefusesCollisionInEitherOrder(t *testing.T) {
+ s := release.NewDirStore(t.TempDir())
+ p := NewMemProvisioner("eu-west-1")
+ declareIn(t, s, "bab", "shop", "eu", "SHOP_S3")
+ if _, err := grantStore(t, s, p, "bab", "shop", "eu", ""); err != nil {
+ t.Fatal(err)
+ }
+ declare(t, s, "bab", "shop-eu", "SHOP_S3")
+ if _, err := grantStore(t, s, p, "bab", "shop-eu", "", ""); !errors.Is(err, ErrStoreNameTaken) {
+ t.Fatalf("colliding grant = %v, want ErrStoreNameTaken", err)
+ }
+ // A three-part join collides with two different splits.
+ declareIn(t, s, "bab", "shop-eu", "west", "SHOP_S3")
+ if _, err := grantStore(t, s, p, "bab", "shop-eu", "west", ""); err != nil {
+ t.Fatal(err)
+ }
+ declareIn(t, s, "bab", "shop", "eu-west", "SHOP_S3")
+ if _, err := grantStore(t, s, p, "bab", "shop", "eu-west", ""); !errors.Is(err, ErrStoreNameTaken) {
+ t.Fatalf("three-part colliding grant = %v, want ErrStoreNameTaken", err)
+ }
+}
+
+// A pair that already collides in production (both granted before this
+// change) must not let a teardown of one destroy the other's estate.
+func TestTeardownLeavesAnEstateAnotherStoreShares(t *testing.T) {
+ ctx := context.Background()
+ s := release.NewDirStore(t.TempDir())
+ p := NewMemProvisioner("eu-west-1")
+ declare(t, s, "bab", "shop-eu", "SHOP_S3")
+ owner, err := grantStore(t, s, p, "bab", "shop-eu", "", "")
+ if err != nil {
+ t.Fatal(err)
+ }
+ forgetNameClaims(t, s)
+ // The second store's record, as the pre-fix walk would have left it.
+ shared := &Record{
+ Declared: owner.Declared,
+ Grant: owner.Grant,
+ Provisioned: owner.Provisioned,
+ }
+ if _, err := PutIn(ctx, s, "bab", "shop", "eu", shared, ""); err != nil {
+ t.Fatal(err)
+ }
+ if err := TeardownIn(ctx, s, p, nil, "bab", "shop", "eu"); err != nil {
+ t.Fatal(err)
+ }
+ if _, ok := p.Buckets[owner.Provisioned.Bucket]; !ok {
+ t.Error("teardown deleted a bucket another store's record still names")
+ }
+ if _, ok := p.Users[owner.Provisioned.IAMUser]; !ok {
+ t.Error("teardown deleted a user another store's record still names")
+ }
+ if rec, _, _ := GetIn(ctx, s, "bab", "shop", "eu"); rec != nil {
+ t.Error("teardown kept the record of the store it was tearing down")
+ }
+}
+
+// Teardown gives the names back: once a store is gone, a store that would
+// have collided with it can be granted.
+func TestTeardownReleasesNames(t *testing.T) {
+ ctx := context.Background()
+ s := release.NewDirStore(t.TempDir())
+ p := NewMemProvisioner("eu-west-1")
+ declare(t, s, "bab", "shop-eu", "SHOP_S3")
+ if _, err := grantStore(t, s, p, "bab", "shop-eu", "", ""); err != nil {
+ t.Fatal(err)
+ }
+ if err := Teardown(ctx, s, p, nil, "bab", "shop-eu"); err != nil {
+ t.Fatal(err)
+ }
+ if keys, _ := s.List(ctx, nameClaimsPrefix); len(keys) != 0 {
+ t.Errorf("claims left after teardown: %v", keys)
+ }
+ declareIn(t, s, "bab", "shop", "eu", "SHOP_S3")
+ if _, err := grantStore(t, s, p, "bab", "shop", "eu", ""); err != nil {
+ t.Fatalf("grant after the colliding store was torn down: %v", err)
+ }
+}
+
+// An operator adopting a bucket another store's record already names is
+// refused, in any account, whether or not that store holds a claim.
+func TestGrantRefusesAdoptingAnotherStoresBucket(t *testing.T) {
+ for _, legacy := range []bool{false, true} {
+ name := "claimed"
+ if legacy {
+ name = "granted-before-claims"
+ }
+ t.Run(name, func(t *testing.T) {
+ s := release.NewDirStore(t.TempDir())
+ p := NewMemProvisioner("eu-west-1")
+ declare(t, s, "bab", "titogo", "TITO_S3")
+ victim, err := grantStore(t, s, p, "bab", "titogo", "", "")
+ if err != nil {
+ t.Fatal(err)
+ }
+ if legacy {
+ forgetNameClaims(t, s)
+ }
+ declare(t, s, "bac", "other", "OTHER_S3")
+ if _, err := grantStore(t, s, p, "bac", "other", "", victim.Provisioned.Bucket); !errors.Is(err, ErrStoreNameTaken) {
+ t.Fatalf("adopting another store's bucket = %v, want ErrStoreNameTaken", err)
+ }
+ // The same app adopting a bucket nobody names is fine.
+ if _, err := grantStore(t, s, p, "bac", "other", "", "acme-uploads"); err != nil {
+ t.Fatal(err)
+ }
+ })
+ }
+}
+
+// #191: a teardown that stopped after deleting the bucket keeps its claim on
+// the name, so nothing else can be granted that bucket until the teardown is
+// finished — and a re-run never deletes a bucket another store now holds.
+func TestRetriedTeardownCannotDeleteABucketGrantedInBetween(t *testing.T) {
+ ctx := context.Background()
+ s := release.NewDirStore(t.TempDir())
+ p := NewMemProvisioner("eu-west-1")
+ minted := MintedBucket(testDeployment(), "bab", "titogo")
+
+ declare(t, s, "bac", "other", "OTHER_S3")
+ if _, err := grantStore(t, s, p, "bac", "other", "", minted); err != nil {
+ t.Fatal(err)
+ }
+ p.FailNext["DeleteUser"] = errors.New("iam down")
+ if err := Teardown(ctx, s, p, nil, "bac", "other"); err == nil {
+ t.Fatal("teardown should have failed at DeleteUser")
+ }
+ declare(t, s, "bab", "titogo", "TITO_S3")
+ if _, err := grantStore(t, s, p, "bab", "titogo", "", ""); !errors.Is(err, ErrStoreNameTaken) {
+ t.Fatalf("grant of a bucket a half-finished teardown still names = %v, want ErrStoreNameTaken", err)
+ }
+ if err := Teardown(ctx, s, p, nil, "bac", "other"); err != nil {
+ t.Fatal(err)
+ }
+ if _, err := grantStore(t, s, p, "bab", "titogo", "", ""); err != nil {
+ t.Fatal(err)
+ }
+
+ // The legacy shape of the same race: the half-finished teardown predates
+ // claims. The minted grant's first run still finds the adopting record —
+ // a minted name is no guarantee nobody adopted it — and is refused.
+ s2 := release.NewDirStore(t.TempDir())
+ p2 := NewMemProvisioner("eu-west-1")
+ declare(t, s2, "bac", "other", "OTHER_S3")
+ if _, err := grantStore(t, s2, p2, "bac", "other", "", minted); err != nil {
+ t.Fatal(err)
+ }
+ p2.FailNext["DeleteUser"] = errors.New("iam down")
+ if err := Teardown(ctx, s2, p2, nil, "bac", "other"); err == nil {
+ t.Fatal("teardown should have failed at DeleteUser")
+ }
+ forgetNameClaims(t, s2)
+ declare(t, s2, "bab", "titogo", "TITO_S3")
+ if _, err := grantStore(t, s2, p2, "bab", "titogo", "", ""); !errors.Is(err, ErrStoreNameTaken) {
+ t.Fatalf("minted grant of a name an unclaimed record adopted = %v, want ErrStoreNameTaken", err)
+ }
+
+ // And if that grant happened before this change, the re-run of the
+ // teardown leaves titogo's bucket alone: titogo's next walk claims it.
+ otherRaw, _, err := s2.Get(ctx, Key("bac", "other"))
+ if err != nil {
+ t.Fatal(err)
+ }
+ if err := s2.Delete(ctx, Key("bac", "other")); err != nil {
+ t.Fatal(err)
+ }
+ if _, err := grantStore(t, s2, p2, "bab", "titogo", "", ""); err != nil {
+ t.Fatal(err)
+ }
+ if _, err := s2.Put(ctx, Key("bac", "other"), otherRaw, ""); err != nil {
+ t.Fatal(err)
+ }
+ p2.Buckets[minted]["data"] = []byte("titogo's")
+ if err := Teardown(ctx, s2, p2, nil, "bac", "other"); err != nil {
+ t.Fatal(err)
+ }
+ if _, ok := p2.Buckets[minted]["data"]; !ok {
+ t.Error("retried teardown emptied a bucket granted to another store in between")
+ }
+}
+
+// A user another store holds survives a teardown, but this store's own keys
+// on it do not: the tenant has seen them, and the user's policy names the
+// other store's bucket.
+func TestTeardownOfASharedUserDeletesThisStoresKeys(t *testing.T) {
+ ctx := context.Background()
+ s := release.NewDirStore(t.TempDir())
+ p := NewMemProvisioner("eu-west-1")
+ declare(t, s, "bab", "shop-eu", "SHOP_S3")
+ owner, err := grantStore(t, s, p, "bab", "shop-eu", "", "")
+ if err != nil {
+ t.Fatal(err)
+ }
+ forgetNameClaims(t, s)
+ user := owner.Provisioned.IAMUser
+ mine, _, err := p.MintKey(ctx, user)
+ if err != nil {
+ t.Fatal(err)
+ }
+ pending, _, err := p.MintKey(ctx, user)
+ if err != nil {
+ t.Fatal(err)
+ }
+ shared := &Record{
+ Declared: owner.Declared,
+ Grant: owner.Grant,
+ Provisioned: &Provisioned{Bucket: owner.Provisioned.Bucket, Region: owner.Provisioned.Region,
+ IAMUser: user, UserARN: owner.Provisioned.UserARN, KeyID: mine, PrevKeyID: pending, At: time.Now()},
+ }
+ if _, err := PutIn(ctx, s, "bab", "shop", "eu", shared, ""); err != nil {
+ t.Fatal(err)
+ }
+ if err := TeardownIn(ctx, s, p, nil, "bab", "shop", "eu"); err != nil {
+ t.Fatal(err)
+ }
+ keys := p.Users[user].Keys
+ for _, id := range []string{mine, pending} {
+ if _, ok := keys[id]; ok {
+ t.Errorf("key %s minted for the torn-down store is still live on the shared user", id)
+ }
+ }
+ if _, ok := keys[owner.Provisioned.KeyID]; !ok {
+ t.Error("teardown deleted the other store's key")
+ }
+}
+
+// A grant that took its claims and died before writing its grant block
+// leaves a declared-only record; tearing that down gives both claims back,
+// including one for an adopted bucket that no record names.
+func TestTeardownOfADeclaredOnlyStoreReleasesItsClaims(t *testing.T) {
+ ctx := context.Background()
+ s := release.NewDirStore(t.TempDir())
+ declare(t, s, "bab", "titogo", "TITO_S3")
+ now := time.Now()
+ if err := claimStoreNames(ctx, s, "bab", "titogo", "", "titogo-uploads", UserName("bab", "titogo"), true, now); err != nil {
+ t.Fatal(err)
+ }
+ if err := Teardown(ctx, s, nil, nil, "bab", "titogo"); err != nil {
+ t.Fatal(err)
+ }
+ if keys, _ := s.List(ctx, nameClaimsPrefix); len(keys) != 0 {
+ t.Errorf("claims left after a declared-only teardown: %v", keys)
+ }
+}
diff --git a/internal/objstore/walk.go b/internal/objstore/walk.go
index be0be2403..28b2527ec 100644
--- a/internal/objstore/walk.go
+++ b/internal/objstore/walk.go
@@ -239,6 +239,15 @@ func GrantIn(ctx context.Context, s release.Store, p Provisioner, d Deployment,
return nil, fmt.Errorf("objstore: IAM user name %q is %d characters, over AWS's %d — shorten the account, app or environment name",
user, len(user), MaxIAMUserName)
}
+ // Both names must be this store's alone (#218), and that is settled here,
+ // before the grant block names them and before EnsureUser, EnsureBucket or
+ // the reap below act on them: either of those, run against a name another
+ // store holds, hands this caller that store's estate and kills its key.
+ // Every run re-checks, which is also how a store granted before claims
+ // existed gets its claims.
+ if err := claimStoreNames(ctx, s, acct, app, env, bucket, user, rec.Grant == nil, now); err != nil {
+ return nil, err
+ }
if rec.Grant == nil {
rec.Grant = &GrantBlock{Backend: d.Backend, Bucket: bucket, Source: source, At: now, By: actor}
if etag, err = PutIn(ctx, s, acct, app, env, rec, etag); err != nil {
@@ -568,7 +577,15 @@ func TeardownIn(ctx context.Context, s release.Store, p Provisioner, sc Scanner,
startBucket = rec.Grant.Bucket
}
if rec.Grant == nil && rec.Provisioned == nil {
- return deleteRecordIfUnchanged(ctx, s, acct, app, env, startKeyID, startBucket)
+ // A grant that claimed its names and died before writing its block
+ // leaves both claims behind. The user's name is computable; the
+ // bucket it chose is recorded nowhere but in the claim itself, so
+ // the bucket claims are searched for this store's own.
+ bucket, err := claimedBucket(ctx, s, acct, app, env)
+ if err != nil {
+ return err
+ }
+ return deleteRecordIfUnchanged(ctx, s, acct, app, env, startKeyID, startBucket, bucket, UserNameIn(acct, app, env))
}
// Everything below spends backend authority, and a nil Provisioner is a
// deployment that has none — an ordinary shape (console Config.
@@ -594,6 +611,21 @@ func TeardownIn(ctx context.Context, s release.Store, p Provisioner, sc Scanner,
default:
bucket = rec.Grant.Bucket
}
+ // A bucket or user another store holds is left standing (#218, #191). Two
+ // shapes reach here: a pair whose joined names collided before claims
+ // existed, both granted; and a teardown re-run after its bucket was
+ // deleted, its name given to another store in between. Either way this
+ // record names an estate that is no longer — or never was — only its own,
+ // and deleting it would destroy the other store's data or credentials.
+ // The rest of the walk goes on: the vars and the record are this store's.
+ bucketShared, userShared, err := sharedNames(ctx, s, acct, app, env, bucket, user)
+ if err != nil {
+ return fmt.Errorf("objstore: check names for %s/%s: %w", acct, app, err)
+ }
+ estateBucket := bucket
+ if bucketShared {
+ 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
@@ -630,7 +662,23 @@ func TeardownIn(ctx context.Context, s release.Store, p Provisioner, sc Scanner,
// DeleteUser reaps the keys and the inline policy, so the credential dies
// with the estate it was scoped to — no orphan principal keeps a key that
// names a bucket that no longer exists.
- if err := p.DeleteUser(ctx, user); err != nil {
+ //
+ // A user another store holds stays, but the keys THIS store minted on it
+ // do not: the tenant has seen them, and the shared user's policy covers
+ // the other store's bucket. Deleting a key that is already gone is
+ // success, so a re-run converges.
+ if userShared {
+ if rec.Provisioned != nil {
+ for _, id := range []string{rec.Provisioned.KeyID, rec.Provisioned.PrevKeyID} {
+ if id == "" {
+ continue
+ }
+ if err := p.DeleteKey(ctx, user, id); err != nil {
+ return fmt.Errorf("objstore: delete key %s for %s: %w", id, user, err)
+ }
+ }
+ }
+ } else if err := p.DeleteUser(ctx, user); err != nil {
return fmt.Errorf("objstore: delete user %s: %w", user, err)
}
// The delivered set goes even though the credentials behind it are already
@@ -678,7 +726,7 @@ func TeardownIn(ctx context.Context, s release.Store, p Provisioner, sc Scanner,
}
}
// Last (§9). Everything it named is gone.
- return deleteRecordIfUnchanged(ctx, s, acct, app, env, startKeyID, startBucket)
+ return deleteRecordIfUnchanged(ctx, s, acct, app, env, startKeyID, startBucket, estateBucket, user)
}
// unsetDelivered removes the delivered set from the bundle a store delivers
@@ -755,7 +803,12 @@ func unsetDelivered(ctx context.Context, s release.Store, acct, app, env string,
// kind guarded these walks until #381 — so if this reference ever stops
// resolving, the sentence above is a claim about nothing and this window is a
// whole walk wide again.
-func deleteRecordIfUnchanged(ctx context.Context, s release.Store, acct, app, env, startKeyID, startBucket string) error {
+//
+// The store's name claims go in the same breath, after the re-read and before
+// the delete: given back any earlier, a grant landing mid-teardown would be
+// left holding names nothing claims; any later, a failed delete of the record
+// would leave its names claimed by a store that no longer exists.
+func deleteRecordIfUnchanged(ctx context.Context, s release.Store, acct, app, env, startKeyID, startBucket, claimBucket, claimUser string) error {
fresh, _, err := GetIn(ctx, s, acct, app, env)
if err != nil {
return fmt.Errorf("objstore: re-read store record for %s/%s: %w", acct, app, err)
@@ -773,6 +826,9 @@ func deleteRecordIfUnchanged(ctx context.Context, s release.Store, acct, app, en
if keyID != startKeyID || bucket != startBucket {
return fmt.Errorf("objstore: store record for %s/%s changed during teardown — re-run: %w", acct, app, release.ErrCASConflict)
}
+ if err := releaseStoreNames(ctx, s, acct, app, env, claimBucket, claimUser); err != nil {
+ return err
+ }
if err := s.Delete(ctx, KeyIn(acct, app, env)); err != nil {
return fmt.Errorf("objstore: delete store record for %s/%s: %w", acct, app, err)
}