amadan

carlos / platform Public

objstore: rotation review fixes — coordinate cover, CAS retry with a guard

A lost CAS on the rotate's final Put was the sharp one: the record would go
on naming the OLD key while the app already holds the new one, and the next
rotation's reap would then delete the credential actually in use. The window
is reachable — the console's declare and grant doors write the same object.
The Put now retries once against a re-read record, carrying the competitor's
blocks and only our own Provisioned one, and ONLY when that re-read still
names the key this walk started from. A blind retry would clobber a
concurrent rotation and strand ITS delivered key, which is the very failure
the retry exists to prevent, so rotate-vs-rotate surfaces the conflict.

Coordinates: every rotation test ran on an empty-endpoint fixture, so
delivering "" for region and endpoint stayed green. Rotate takes no
Deployment — the record is the only source for those values — and a rotate
that dropped them would move every host onto the backend's own address with
a green rotate next to it. Covered on an endpoint-bearing fixture.

Doc: the old comment claimed the record keeps naming the key the app holds
until delivery succeeds, which is false once delivery succeeds and the Put
fails. The true invariant is one-way — the record never names a key whose
secret was never delivered — and the reverse gap is survivable because both
keys are live.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Paul Campbell 36263d6e1c6ea61ba473b6acf500d033c81830eb parent 68b4b34
2 files changed, +187 −6
  • internal/objstore/walk.go +39 −6
  • internal/objstore/walk_test.go +148 −0
diff --git a/internal/objstore/walk.go b/internal/objstore/walk.go
index 034e0a3c7..6032e3321 100644
--- a/internal/objstore/walk.go
+++ b/internal/objstore/walk.go
@@ -246,11 +246,22 @@ func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment,
// the bucket an app's data is in is never re-derived from a deployment that
// may have moved on.
//
-// Step five records, last, as in Grant: until the secret is delivered the
-// record must keep naming the key the app is actually holding. A record that
-// claimed the new key after a failed delivery would point every reader at a
-// key whose secret died with the write, and the re-run would then reap the ONE
-// key still working.
+// Step five records, last, as in Grant. The invariant it buys is one-way: the
+// record never names a key whose secret was never delivered. A record that
+// claimed the new key after a FAILED delivery would point every reader at a
+// key whose secret died with the write, and the next run would reap the ONE
+// key still working. The reverse gap is survivable and deliberate — after a
+// successful delivery whose Put then fails, the record names the old key
+// while the app already holds the new one, and both are live, so the app
+// keeps working and a re-run converges.
+//
+// The one shape of that gap which is NOT survivable is a lost CAS: the record
+// would keep naming the old key permanently, and the next rotation's reap
+// would delete the delivered key the app is actually running on. So the final
+// Put retries once against a re-read record — but only when that re-read still
+// names the key this walk started from, i.e. no other rotation landed in
+// between. A blind retry would clobber a concurrent rotate and strand ITS
+// delivered key, which is the exact failure the retry exists to prevent.
func Rotate(ctx context.Context, s release.Store, p Provisioner, sealPub *ecdh.PublicKey,
acct, app string, now time.Time) (*Record, error) {
rec, etag, err := Get(ctx, s, acct, app)
@@ -264,6 +275,7 @@ func Rotate(ctx context.Context, s release.Store, p Provisioner, sealPub *ecdh.P
return nil, ErrNotDeclared // no prefix to deliver into
}
pr := rec.Provisioned
+ startKeyID := pr.KeyID // the key the app is holding, and the retry guard
// 1. finish the pending rotation late. The reap below would collect this
// key too, but doing it by name first keeps the intent legible and means
// a backend that refuses the delete stops the walk here, before a mint.
@@ -290,7 +302,28 @@ func Rotate(ctx context.Context, s release.Store, p Provisioner, sealPub *ecdh.P
// 5. record: the key just retired from live becomes the pending one.
pr.PrevKeyID, pr.KeyID, pr.At = pr.KeyID, id, now
if _, err := Put(ctx, s, acct, app, rec, etag); err != nil {
- return nil, err
+ if !errors.Is(err, release.ErrCASConflict) {
+ return nil, err
+ }
+ // Someone else wrote this record between our Get and our Put — the
+ // console's declare/grant doors write the same object. Re-read and
+ // try once more, carrying THEIR blocks and only our Provisioned one,
+ // but only if the record still names the key we started from. If it
+ // names another key, a concurrent rotation delivered a credential of
+ // its own; overwriting it would strand exactly what we are protecting
+ // against, so the conflict is the caller's to see.
+ fresh, freshEtag, gerr := Get(ctx, s, acct, app)
+ if gerr != nil {
+ return nil, err
+ }
+ if fresh == nil || fresh.Provisioned == nil || fresh.Provisioned.KeyID != startKeyID {
+ return nil, err
+ }
+ fresh.Provisioned = pr
+ 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 ca0c716a1..d41463f7d 100644
--- a/internal/objstore/walk_test.go
+++ b/internal/objstore/walk_test.go
@@ -844,3 +844,151 @@ func TestRotateUnprovisionedRefused(t *testing.T) {
t.Fatalf("refused rotation wrote config: %v", v)
}
}
+
+// A rotation changes credentials and NOTHING else. The coordinates it
+// redelivers come from the record, so an endpoint-bearing store must come out
+// of a rotate still pointing at its endpoint — a rotate that re-derived them
+// (or dropped them) would silently move every host onto the backend's own
+// address, which is a data-plane outage with a green rotate next to it.
+func TestRotateKeepsRecordedCoordinates(t *testing.T) {
+ ctx := context.Background()
+ s := release.NewDirStore(t.TempDir())
+ priv, pub := testSealKeypair(t)
+ p := NewMemProvisioner("eu-central-1")
+ declare(t, s, "bab", "titogo", "TITO_S3")
+
+ d := testDeployment()
+ d.Endpoint = "https://s3.example.net"
+ granted, err := Grant(ctx, s, p, d, pub, "bab", "titogo", "op", "", time.Now().UTC())
+ if err != nil {
+ t.Fatal(err)
+ }
+ if granted.Provisioned.Endpoint != d.Endpoint || granted.Provisioned.Region != "eu-central-1" {
+ t.Fatalf("fixture did not record the coordinates: %+v", granted.Provisioned)
+ }
+
+ // Rotate takes no Deployment: the only source for these values is the
+ // record, so this is the assertion that it reads them from there.
+ rec, err := Rotate(ctx, s, p, pub, "bab", "titogo", time.Now().UTC())
+ if err != nil {
+ t.Fatal(err)
+ }
+ if rec.Provisioned.Endpoint != d.Endpoint || rec.Provisioned.Region != "eu-central-1" ||
+ rec.Provisioned.Bucket != granted.Provisioned.Bucket {
+ t.Fatalf("rotate moved the coordinates: %+v", rec.Provisioned)
+ }
+ vars := mustVars(t, s, "bab", "titogo")
+ if v := vars["TITO_S3_ENDPOINT"]; v.Kind != "env" || v.Value != d.Endpoint {
+ t.Fatalf("TITO_S3_ENDPOINT after rotate = %+v, want %q", v, d.Endpoint)
+ }
+ if v := vars["TITO_S3_REGION"]; v.Kind != "env" || v.Value != "eu-central-1" {
+ t.Fatalf("TITO_S3_REGION after rotate = %+v", v)
+ }
+ if v := vars["TITO_S3_BUCKET"]; v.Kind != "env" || v.Value != granted.Provisioned.Bucket {
+ t.Fatalf("TITO_S3_BUCKET after rotate = %+v", v)
+ }
+ if got := openVar(t, priv, s, "bab", "titogo", "TITO_S3_ACCESS_KEY"); got != rec.Provisioned.KeyID {
+ t.Fatalf("delivered access key = %q, want the new id %q", got, rec.Provisioned.KeyID)
+ }
+ if missing, err := EnvDelivered(ctx, s, "bab", "titogo", "TITO_S3", true); err != nil || len(missing) != 0 {
+ t.Fatalf("after rotate: missing = %v, %v", missing, err)
+ }
+}
+
+// racingStore lands a competing write on the record immediately BEFORE the
+// walk's own Put reaches the store — the interleaving that turns the walk's
+// etag stale. One shot, so only the write under test races.
+type racingStore struct {
+ release.Store
+ raceKeyOnce string
+ race func(inner release.Store)
+}
+
+func (r *racingStore) Put(ctx context.Context, key string, body []byte, ifMatch string) (string, error) {
+ if key != "" && key == r.raceKeyOnce {
+ r.raceKeyOnce = ""
+ r.race(r.Store)
+ }
+ return r.Store.Put(ctx, key, body, ifMatch)
+}
+
+// The window between a rotate's Get and its Put is real: the console's
+// declare/grant doors write the same object. Losing that CAS outright would
+// leave the record naming the OLD key while the app runs on the new one —
+// and the next rotate's reap would then delete the key actually in use. So
+// the walk retries against a re-read record, and the competitor's change to
+// the blocks it does not own must survive.
+func TestRotateRetriesLostCAS(t *testing.T) {
+ ctx := context.Background()
+ priv, pub := testSealKeypair(t)
+ inner := release.NewDirStore(t.TempDir())
+ s := &racingStore{Store: inner}
+ p := NewMemProvisioner("eu-west-1")
+ oldID := rotateFixture(t, s, pub, p).Provisioned.KeyID
+
+ // A competing writer touches a block this walk does not own.
+ 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 := Rotate(ctx, s, p, pub, "bab", "titogo", time.Now().UTC())
+ if err != nil {
+ t.Fatalf("rotate lost its CAS and gave up: %v", err)
+ }
+ if rec.Provisioned.PrevKeyID != oldID || rec.Provisioned.KeyID == oldID {
+ 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.KeyID != rec.Provisioned.KeyID || stored.Provisioned.PrevKeyID != oldID {
+ 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)
+ }
+}
+
+// Rotate-vs-rotate is the case the retry must NOT paper over: the competitor
+// delivered a credential of its own, so overwriting its record would strand
+// that key exactly as a lost CAS would strand ours. The conflict is surfaced.
+func TestRotateRefusesRetryAgainstAnotherRotation(t *testing.T) {
+ ctx := context.Background()
+ _, pub := testSealKeypair(t)
+ inner := release.NewDirStore(t.TempDir())
+ s := &racingStore{Store: inner}
+ p := NewMemProvisioner("eu-west-1")
+ oldID := rotateFixture(t, s, pub, p).Provisioned.KeyID
+
+ 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.KeyID = "MEMKEY99" // another rotation landed first
+ other.Provisioned.PrevKeyID = oldID
+ if _, err := Put(ctx, in, "bab", "titogo", other, etag); err != nil {
+ t.Fatal(err)
+ }
+ }
+
+ if _, err := Rotate(ctx, s, p, pub, "bab", "titogo", time.Now().UTC()); !errors.Is(err, release.ErrCASConflict) {
+ t.Fatalf("rotate over a concurrent rotation: %v, want ErrCASConflict", err)
+ }
+ stored, _, err := Get(ctx, s, "bab", "titogo")
+ if err != nil || stored.Provisioned.KeyID != "MEMKEY99" || stored.Provisioned.PrevKeyID != oldID {
+ t.Fatalf("the competing rotation was clobbered: %+v, %v", stored.Provisioned, err)
+ }
+}