amadan

carlos / platform Public

objstore: Grant's final Put must retry a lost CAS, as Rotate's does

Rotate's step five retries its Put against a re-read record, guarded on the
record still naming the key the walk started from. Grant's step six — the same
write, the same window, the same object the console's declare door writes —
returned the conflict and gave up.

The gap is the one Rotate's doc comment calls NOT survivable. Step five has
already delivered the secret, so the app is running on the new key while the
record keeps naming whatever it named before; the next rotation reaps every key
the record does not name, and deletes the one in use.

Until now this needed two operators running `grant` simultaneously. Self-serve
object storage (#381) puts this walk behind a member's own `carlos store
create`, which the create door's own comment says people re-run from second
terminals and deploy scripts — so the window stops being exotic.

The retry carries the competitor's blocks and replaces only Provisioned, and
refuses when the re-read record names a different key: that means a concurrent
walk minted and delivered a credential of its own, and overwriting it would
strand exactly what the retry exists to protect.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev 09d18f58fb1f1ab537d7796118a1139a40cfb456 parent 38bd8dc
2 files changed, +156 −1
  • internal/objstore/walk.go +44 −1
  • internal/objstore/walk_test.go +112 −0
diff --git a/internal/objstore/walk.go b/internal/objstore/walk.go
index 94475421e..4dc62e139 100644
--- a/internal/objstore/walk.go
+++ b/internal/objstore/walk.go
@@ -140,6 +140,13 @@ func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment,
if rec == nil || rec.Declared == nil {
return nil, ErrNotDeclared
}
+ // The key this walk started from, for the step-6 retry's guard below. ""
+ // is the ordinary case: a first grant, or a re-run of one that died before
+ // it recorded anything.
+ startKeyID := ""
+ if rec.Provisioned != nil {
+ startKeyID = rec.Provisioned.KeyID
+ }
bucket := adoptBucket
if rec.Grant != nil && rec.Grant.Bucket != "" {
bucket = rec.Grant.Bucket // a re-run never re-chooses the bucket
@@ -216,7 +223,43 @@ func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment,
rec.Provisioned = &Provisioned{Bucket: bucket, Region: region, Endpoint: d.Endpoint,
IAMUser: user, UserARN: arn, KeyID: id, PrevKeyID: prev, At: now}
if _, err := Put(ctx, s, acct, app, rec, etag); err != nil {
- return nil, err
+ if !errors.Is(err, release.ErrCASConflict) {
+ return nil, err
+ }
+ // Same window, same rule, same retry as Rotate's step five — see its
+ // doc comment for the full argument. A lost CAS here is the one shape
+ // of the gap that is NOT survivable: the secret has already been
+ // delivered (step 5), so the app is running on `id`, while the record
+ // would keep naming whatever it named before. The next rotation reaps
+ // every key the record does not name, and would delete the key in use.
+ //
+ // Before self-serve object storage (#381) this needed two operators
+ // running `grant` at once. Now a member's own `carlos store create`
+ // drives this walk, and the console's declare door writes this very
+ // object, so the window is ordinary rather than exotic.
+ //
+ // Re-read and try once more, carrying THEIR blocks and only our
+ // Provisioned one — but only if the record still names the key this
+ // walk started from. If it names another, a concurrent walk minted and
+ // DELIVERED a credential of its own; overwriting it would strand
+ // exactly what this retry protects against, so that conflict is the
+ // caller's to see.
+ fresh, freshEtag, gerr := Get(ctx, s, acct, app)
+ if gerr != nil {
+ return nil, err
+ }
+ freshKeyID := ""
+ if fresh != nil && fresh.Provisioned != nil {
+ freshKeyID = fresh.Provisioned.KeyID
+ }
+ if fresh == nil || freshKeyID != startKeyID {
+ return nil, err
+ }
+ fresh.Provisioned = rec.Provisioned
+ if _, perr := Put(ctx, s, acct, app, fresh, freshEtag); perr != nil {
+ return nil, perr
+ }
+ return fresh, nil
}
return rec, nil
}
diff --git a/internal/objstore/walk_test.go b/internal/objstore/walk_test.go
index 518a968c6..a11b459c8 100644
--- a/internal/objstore/walk_test.go
+++ b/internal/objstore/walk_test.go
@@ -1408,3 +1408,115 @@ func TestPurgeGuardSeesOurRecord(t *testing.T) {
}
})
}
+
+// grantBlockOnlyFixture leaves the record exactly as a walk that died mid-flight
+// leaves it: the grant block written (Grant's step 1) and no provisioned block.
+// A re-run then skips step 1's Put, so the next write it makes to the record is
+// step 6's — the one these two tests race.
+func grantBlockOnlyFixture(t *testing.T, s release.Store) {
+ t.Helper()
+ ctx := context.Background()
+ declare(t, s, "bab", "titogo", "TITO_S3")
+ rec, etag, err := Get(ctx, s, "bab", "titogo")
+ if err != nil {
+ t.Fatal(err)
+ }
+ rec.Grant = &GrantBlock{
+ Backend: testDeployment().Backend,
+ Bucket: MintedBucket(testDeployment(), "bab", "titogo"),
+ At: time.Now().UTC(),
+ By: "op",
+ }
+ if _, err := Put(ctx, s, "bab", "titogo", rec, etag); err != nil {
+ t.Fatal(err)
+ }
+}
+
+// Grant's final Put has the same window Rotate's does — the console's declare
+// door writes this very object, and under self-serve storage (#381) a member's
+// own `carlos store create` drives the walk, so the window is ordinary rather
+// than rare. Losing that CAS outright leaves the record naming no key (or an
+// older one) while the app already holds the delivered credential, and the next
+// rotate's reap then deletes the key actually in use — the outcome Rotate's own
+// doc comment calls NOT survivable. So Grant retries against a re-read record,
+// and a competitor's change to blocks it does not own must survive.
+func TestGrantRetriesLostCAS(t *testing.T) {
+ ctx := context.Background()
+ priv, pub := testSealKeypair(t)
+ inner := release.NewDirStore(t.TempDir())
+ s := &racingStore{Store: inner}
+ p := NewMemProvisioner("eu-west-1")
+ grantBlockOnlyFixture(t, s)
+
+ // A competing writer touches a block this walk does not own — exactly what
+ // a re-declare through the console's create door does.
+ s.raceKeyOnce = Key("bab", "titogo")
+ s.race = func(in release.Store) {
+ other, etag, err := Get(ctx, in, "bab", "titogo")
+ if err != nil {
+ t.Fatal(err)
+ }
+ other.Declared.By = "someone-else@example.com"
+ if _, err := Put(ctx, in, "bab", "titogo", other, etag); err != nil {
+ t.Fatal(err)
+ }
+ }
+
+ rec, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", "", time.Now().UTC())
+ if err != nil {
+ t.Fatalf("grant lost its CAS and gave up: %v", err)
+ }
+ if rec.Provisioned == nil || rec.Provisioned.KeyID == "" {
+ t.Fatalf("provisioned = %+v", rec.Provisioned)
+ }
+ if rec.Declared.By != "someone-else@example.com" {
+ t.Fatalf("retry clobbered the competing write: Declared.By = %q", rec.Declared.By)
+ }
+ stored, _, err := Get(ctx, s, "bab", "titogo")
+ if err != nil || stored.Declared.By != "someone-else@example.com" ||
+ stored.Provisioned == nil || stored.Provisioned.KeyID != rec.Provisioned.KeyID {
+ t.Fatalf("stored record = %+v / %+v, %v", stored.Declared, stored.Provisioned, err)
+ }
+ // The record and the app agree on the live key — the whole point.
+ if got := openVar(t, priv, s, "bab", "titogo", "TITO_S3_ACCESS_KEY"); got != stored.Provisioned.KeyID {
+ t.Fatalf("delivered access key = %q, record says %q", got, stored.Provisioned.KeyID)
+ }
+}
+
+// Grant-vs-grant is the case the retry must NOT paper over: a competing walk
+// minted and DELIVERED a key of its own, so overwriting its record would strand
+// that key exactly as a lost CAS would strand ours. The conflict is surfaced.
+func TestGrantRefusesRetryAgainstAnotherProvision(t *testing.T) {
+ ctx := context.Background()
+ _, pub := testSealKeypair(t)
+ inner := release.NewDirStore(t.TempDir())
+ s := &racingStore{Store: inner}
+ p := NewMemProvisioner("eu-west-1")
+ grantBlockOnlyFixture(t, s)
+
+ s.raceKeyOnce = Key("bab", "titogo")
+ s.race = func(in release.Store) {
+ other, etag, err := Get(ctx, in, "bab", "titogo")
+ if err != nil {
+ t.Fatal(err)
+ }
+ other.Provisioned = &Provisioned{
+ Bucket: other.Grant.Bucket,
+ Region: "eu-west-1",
+ IAMUser: UserName("bab", "titogo"),
+ KeyID: "MEMKEY99", // another grant landed first, and delivered
+ At: time.Now().UTC(),
+ }
+ if _, err := Put(ctx, in, "bab", "titogo", other, etag); err != nil {
+ t.Fatal(err)
+ }
+ }
+
+ if _, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", "", time.Now().UTC()); !errors.Is(err, release.ErrCASConflict) {
+ t.Fatalf("grant over a concurrent grant: %v, want ErrCASConflict", err)
+ }
+ stored, _, err := Get(ctx, s, "bab", "titogo")
+ if err != nil || stored.Provisioned == nil || stored.Provisioned.KeyID != "MEMKEY99" {
+ t.Fatalf("the competing grant was clobbered: %+v, %v", stored.Provisioned, err)
+ }
+}