amadan

carlos / platform Public

mail: close CreateStandalone pause bypass, harden pause assertions (round 2)

CreateStandalone minted on its own IAM principal without ever calling
enforceKeyActive, so a member verb (carlos email credentials create) could
hand a paused tenant a live standalone credential on the same verified
domain the pause was meant to protect — the same C1 shape, on a third mint
path the first fix round didn't cover. Closed by routing CreateStandalone
through the same enforcement point as Enable and Rotate.

The C1/I1 assertions and the standalone-pause test indexed MemUser.Keys (a
value map) with single-value lookups, so a key that was missing entirely
would read as the zero MemKey{Active: false} and pass as "correctly
inactive" — vacuously, on the guards protecting the Critical itself. Switched
every Active assertion in this file to the two-value form, and to the outer
p.Users map too, matching the pattern the round-1 tests already used and
fixing a nil-pointer panic when the outer entry was absent.

Corrected Teardown's I5 doc comment: the reason a crash victim's stale
EnvDelivered check stays satisfied is that Teardown never unsets the app's
SMTP env, not that there is "nothing left to compare against."
Paul Campbell pushed by paul@keymail.dev 6f028f3af44820c8725db913bbaf003abc043cb1 parent 1006bea
2 files changed, +134 −11
  • internal/mail/walk.go +22 −7
  • internal/mail/walk_lifecycle_test.go +112 −4
diff --git a/internal/mail/walk.go b/internal/mail/walk.go
index 28c04054f..5fb0f2e29 100644
--- a/internal/mail/walk.go
+++ b/internal/mail/walk.go
@@ -537,7 +537,7 @@ type StandaloneCreds struct {
// enforceKeyActive sets a freshly minted key's IAM active state to match the
// record's OWN Active flag. It is the single enforcement point every
-// mint-and-deliver path (Enable, Rotate) must call.
+// mint-and-deliver path — Enable, Rotate, AND CreateStandalone — must call.
//
// MintKey always returns an ACTIVE key. Skipping this after a mint silently
// lifts a standing pause at IAM even though the record keeps saying paused —
@@ -727,6 +727,19 @@ func CreateStandalone(ctx context.Context, s release.Store, p Provisioner, d Dep
if err != nil {
return StandaloneCreds{}, err
}
+ // A THIRD mint path, and enforceKeyActive's contract applies here just
+ // as it does to Enable and Rotate: minting a standalone credential for a
+ // PAUSED app must not hand back a live key just because the mint went
+ // through a different member verb. An app with no Provisioned block yet
+ // (declared and granted, never enabled) has nothing to be paused, so it
+ // defaults active.
+ active := true
+ if rec.Provisioned != nil {
+ active = rec.Provisioned.Active
+ }
+ if err := enforceKeyActive(ctx, p, user, keyID, active); err != nil {
+ return StandaloneCreds{}, err
+ }
replaced := false
for _, sa := range rec.Standalone {
@@ -793,12 +806,14 @@ func RevokeStandalone(ctx context.Context, s release.Store, p Provisioner, acct,
//
// NOT SELF-HEALING across a crash (I5): a crash between the DeleteUser calls
// below and the domain loop leaves a zombie — the IAM user is gone, the
-// record still says Provisioned, and EnvDelivered is satisfied (there is
-// nothing left to compare against), so Enable will short-circuit rather than
-// recreate the user, and Rotate will fail at MintKey because there is no
-// user to mint against. Nothing re-runs Teardown automatically on a
-// half-failure; the caller must retry it to completion. A reconciler that
-// notices and repairs a half-torn-down app is out of scope here.
+// record still says Provisioned, and Teardown never unset the app's SMTP
+// env, so the stale delivered *_SMTP_USER still equals rec.Provisioned.KeyID
+// and EnvDelivered stays satisfied on that comparison. Enable therefore
+// short-circuits rather than recreate the user, and Rotate fails at MintKey
+// because there is no user left to mint against. Nothing re-runs Teardown
+// automatically on a half-failure; the caller must retry it to completion.
+// A reconciler that notices and repairs a half-torn-down app is out of
+// scope here.
func Teardown(ctx context.Context, s release.Store, p Provisioner, w dnsrec.Writer,
d Deployment, acct, app string) error {
diff --git a/internal/mail/walk_lifecycle_test.go b/internal/mail/walk_lifecycle_test.go
index 53da9b1ac..dabdc6f48 100644
--- a/internal/mail/walk_lifecycle_test.go
+++ b/internal/mail/walk_lifecycle_test.go
@@ -62,7 +62,11 @@ func TestRotateDoesNotLiftAPauseAtIAM(t *testing.T) {
t.Fatal("rotate flipped the record back to active")
}
user := UserName("bag", "seapointish")
- if p.Users[user].Keys[rec.Provisioned.KeyID].Active {
+ k, ok := p.Users[user].Keys[rec.Provisioned.KeyID]
+ if !ok {
+ t.Fatalf("minted key %s does not exist at all", rec.Provisioned.KeyID)
+ }
+ if k.Active {
t.Fatal("rotate minted a LIVE key for a paused app — the pause was silently lifted at IAM")
}
}
@@ -114,7 +118,11 @@ func TestEnableRepairDoesNotLiftAPauseAtIAM(t *testing.T) {
t.Fatal("repair re-walk flipped the record back to active")
}
user := UserName("bag", "seapointish")
- if p.Users[user].Keys[rec.Provisioned.KeyID].Active {
+ k, ok := p.Users[user].Keys[rec.Provisioned.KeyID]
+ if !ok {
+ t.Fatalf("minted key %s does not exist at all", rec.Provisioned.KeyID)
+ }
+ if k.Active {
t.Fatal("repair re-walk minted a LIVE key for a paused app — the pause was silently lifted at IAM")
}
}
@@ -212,18 +220,118 @@ func TestPauseReachesStandaloneCredentials(t *testing.T) {
if _, err := SetPaused(ctx, s, p, "bag", "seapointish", true, now); err != nil {
t.Fatalf("SetPaused(true): %v", err)
}
- if p.Users[standaloneUser].Keys[keyID].Active {
+ su, ok := p.Users[standaloneUser]
+ if !ok {
+ t.Fatalf("standalone user %s does not exist at all", standaloneUser)
+ }
+ k, ok := su.Keys[keyID]
+ if !ok {
+ t.Fatalf("standalone key %s does not exist at all", keyID)
+ }
+ if k.Active {
t.Fatal("pause did not reach the standalone credential — it can still send")
}
if _, err := SetPaused(ctx, s, p, "bag", "seapointish", false, now); err != nil {
t.Fatalf("SetPaused(false): %v", err)
}
- if !p.Users[standaloneUser].Keys[keyID].Active {
+ su, ok = p.Users[standaloneUser]
+ if !ok {
+ t.Fatalf("resume left the standalone user %s gone entirely", standaloneUser)
+ }
+ k, ok = su.Keys[keyID]
+ if !ok {
+ t.Fatalf("resume left the standalone key %s gone entirely", keyID)
+ }
+ if !k.Active {
t.Fatal("resume did not reach the standalone credential")
}
}
+// Round 2, item 1: CreateStandalone is a THIRD mint path and must go
+// through the same enforcement point as Enable and Rotate. Without it, a
+// member verb (`carlos email credentials create`) could hand a paused
+// tenant a live sending credential on the same verified domain the pause
+// was meant to protect — exactly the C1 shape, on a path C1's original fix
+// did not cover.
+func TestCreateStandaloneDoesNotBypassAPause(t *testing.T) {
+ ctx, s, p, pub, now := enableFixture(t)
+ if _, err := Enable(ctx, s, p, pub, testDeployment(), "bag", "seapointish", now); err != nil {
+ t.Fatalf("Enable: %v", err)
+ }
+ if _, err := SetPaused(ctx, s, p, "bag", "seapointish", true, now); err != nil {
+ t.Fatalf("SetPaused(true): %v", err)
+ }
+
+ creds, err := CreateStandalone(ctx, s, p, testDeployment(), "bag", "seapointish",
+ "laptop", "paul@rslw.com", now)
+ if err != nil {
+ t.Fatalf("CreateStandalone: %v", err)
+ }
+ standaloneUser := StandaloneUserName("bag", "seapointish", "laptop")
+ su, ok := p.Users[standaloneUser]
+ if !ok {
+ t.Fatalf("standalone user %s does not exist at all", standaloneUser)
+ }
+ k, ok := su.Keys[creds.Username]
+ if !ok {
+ t.Fatalf("minted standalone key %s does not exist at all", creds.Username)
+ }
+ if k.Active {
+ t.Fatal("CreateStandalone minted a LIVE key for a paused app — the pause was bypassed")
+ }
+}
+
+// A re-mint for an existing standalone name while paused must not reactivate
+// it either, and a resume must reach a standalone credential created while
+// paused.
+func TestCreateStandaloneReMintWhilePausedStaysInactiveAndResumeReactivates(t *testing.T) {
+ ctx, s, p, pub, now := enableFixture(t)
+ if _, err := Enable(ctx, s, p, pub, testDeployment(), "bag", "seapointish", now); err != nil {
+ t.Fatalf("Enable: %v", err)
+ }
+ if _, err := SetPaused(ctx, s, p, "bag", "seapointish", true, now); err != nil {
+ t.Fatalf("SetPaused(true): %v", err)
+ }
+ if _, err := CreateStandalone(ctx, s, p, testDeployment(), "bag", "seapointish",
+ "laptop", "paul@rslw.com", now); err != nil {
+ t.Fatalf("CreateStandalone: %v", err)
+ }
+ // Re-mint the same name while still paused.
+ creds, err := CreateStandalone(ctx, s, p, testDeployment(), "bag", "seapointish",
+ "laptop", "paul@rslw.com", now)
+ if err != nil {
+ t.Fatalf("CreateStandalone (re-mint): %v", err)
+ }
+ standaloneUser := StandaloneUserName("bag", "seapointish", "laptop")
+ su, ok := p.Users[standaloneUser]
+ if !ok {
+ t.Fatalf("standalone user %s does not exist at all", standaloneUser)
+ }
+ k, ok := su.Keys[creds.Username]
+ if !ok {
+ t.Fatalf("minted standalone key %s does not exist at all", creds.Username)
+ }
+ if k.Active {
+ t.Fatal("a re-mint while paused produced a LIVE key")
+ }
+
+ if _, err := SetPaused(ctx, s, p, "bag", "seapointish", false, now); err != nil {
+ t.Fatalf("SetPaused(false): %v", err)
+ }
+ su, ok = p.Users[standaloneUser]
+ if !ok {
+ t.Fatalf("resume left the standalone user %s gone entirely", standaloneUser)
+ }
+ k, ok = su.Keys[creds.Username]
+ if !ok {
+ t.Fatalf("resume left the standalone key %s gone entirely", creds.Username)
+ }
+ if !k.Active {
+ t.Fatal("resume did not reach a standalone credential created while paused")
+ }
+}
+
// A standalone credential lives on its OWN IAM user, because the delivered
// user's two key slots are spent on the delivered key and the rotation
// overlap. Sharing would work until the first rotation and then break it.