amadan

carlos / platform Public

mail: address review — pause survives re-walk, USER unsealed for staleness check

Six fixes from Task 12 review:

- Enable now carries Provisioned.Active forward through a re-walk instead
  of hardcoding true, so a repair pass can never silently resume a paused
  credential (Task 13 adds the pause verb; nothing must undo it by
  accident).
- _USER is delivered as a plain env var, not sealed. Reversing the
  original design here: sealing it protected nothing (the same access key
  id already sits in plaintext in email.json, in the same bucket as
  config.json), and it cost the one cheap staleness check available —
  EnvDelivered now compares the delivered _USER value against
  rec.Provisioned.KeyID, catching a stale record (e.g. a failed final Put
  after a successful mint+deliver) that would otherwise read as healthy
  forever. _PASS stays sealed; it is the actual secret.
- EnvDelivered takes a wantKeyID parameter for that comparison ("" skips
  it).
- Declare validates the From address with appconfig.ValidVarValue before
  anything is spent, so an unrenderable From fails before MintKey runs
  instead of after — the earlier check only caught the missing "@".
- SMTPHost's doc comment no longer claims per-instance resolution that
  does not exist; it says what v1 actually delivers (one host per
  deployment region, fleet-wide) and notes _PASS must move with _HOST
  whenever that lands, since the password is derived per region.
- Removed two dead assignments in the From-domain test that F4 should
  have caught.

New tests: a paused re-walk stays paused; EnvDelivered and Enable both
catch a record whose KeyID disagrees with what was actually delivered.
Paul Campbell 74df38e16bf5c42c8d1a0131235535ea497b8beb parent 395de53
2 files changed, +158 −34
  • internal/mail/walk.go +72 −28
  • internal/mail/walk_enable_test.go +86 −6
diff --git a/internal/mail/walk.go b/internal/mail/walk.go
index 10cb56ec6..2cedc1de9 100644
--- a/internal/mail/walk.go
+++ b/internal/mail/walk.go
@@ -214,9 +214,16 @@ func VerifyDomain(ctx context.Context, s release.Store, p Provisioner,
// STARTTLS is the whole of the delivered contract's transport (spec §1).
const SMTPPort = "587"
-// SMTPHost is the submission endpoint for a region. Delivery resolves this
-// per instance, which is what makes region-local sending a lookup rather
-// than a subsystem (spec §8).
+// SMTPHost is the submission endpoint for a deployment's region. v1 delivers
+// exactly ONE _HOST, fleet-wide, from d.Region — every instance of an app
+// gets the same host regardless of where it actually runs. Per-instance
+// resolution (a host that follows the instance's own region) is deferred,
+// and is recorded in the spec.
+//
+// When per-instance resolution lands, _PASS must move with it, not stay
+// behind: SMTPPassword is derived PER REGION, so a per-instance host paired
+// with today's fleet-wide password would be wrong for every instance
+// outside the region that password was derived for.
func SMTPHost(region string) string { return "email-smtp." + region + ".amazonaws.com" }
// Declare records the member's half. It does NOT validate the From address
@@ -234,6 +241,14 @@ func Declare(ctx context.Context, s release.Store, acct, app, envPrefix, from, a
if !strings.Contains(from, "@") {
return nil, fmt.Errorf("mail: From %q is not an address", from)
}
+ // Checked here, before anything is spent: From is delivered verbatim as
+ // SEAPOINTISH_SMTP_FROM, and an unrenderable byte caught only inside
+ // writeSMTPEnv would fail AFTER MintKey has already run — stranding a
+ // key on every retry. The From address is not a secret, so it is safe
+ // to name in the error.
+ if err := appconfig.ValidVarValue(from); err != nil {
+ return nil, fmt.Errorf("mail: From %q is not EnvironmentFile-safe: %w", from, err)
+ }
rec, etag, err := Get(ctx, s, acct, app)
if err != nil {
return nil, err
@@ -281,7 +296,18 @@ func grantedDomains(ctx context.Context, s release.Store, acct, app string) ([]*
// right kind, in the app's config. It is the walk's idempotence check and
// the console's "what is missing" answer, and it exists because "the record
// says provisioned" is not the same claim as "the app can actually send".
-func EnvDelivered(ctx context.Context, s release.Store, acct, app, prefix string) ([]string, error) {
+//
+// wantKeyID is the access key id delivery is supposed to be carrying in
+// _USER — normally rec.Provisioned.KeyID. Pass "" to skip the value check
+// (there is no expected key yet). _USER's VALUE is compared, not just its
+// kind, because _USER is delivered unsealed (writeSMTPEnv) and is therefore
+// the one credential half this function can cheaply verify end to end: a
+// failed final Put after a successful mint+deliver would otherwise leave
+// the app running on a key no record names, reading as "delivered"
+// regardless — a disagreement that is permanent, because nothing else ever
+// re-checks it. _PASS is sealed, so its value cannot be compared this way;
+// Kind is the best this function can do for it.
+func EnvDelivered(ctx context.Context, s release.Store, acct, app, prefix, wantKeyID string) ([]string, error) {
man, err := appconfig.Get(ctx, s, acct, app)
if err != nil {
return nil, err
@@ -297,8 +323,10 @@ func EnvDelivered(ctx context.Context, s release.Store, acct, app, prefix string
check(host, "env")
check(port, "env")
check(from, "env")
- check(user, "secret")
check(pass, "secret")
+ if v, ok := man.Vars[user]; !ok || v.Kind != "env" || (wantKeyID != "" && v.Value != wantKeyID) {
+ missing = append(missing, user)
+ }
sort.Strings(missing)
return missing, nil
}
@@ -349,7 +377,7 @@ func Enable(ctx context.Context, s release.Store, p Provisioner, sealPub *ecdh.P
// Idempotence short-circuit.
if rec.Provisioned != nil {
- missing, err := EnvDelivered(ctx, s, acct, app, rec.Declared.EnvPrefix)
+ missing, err := EnvDelivered(ctx, s, acct, app, rec.Declared.EnvPrefix, rec.Provisioned.KeyID)
if err != nil {
return nil, err
}
@@ -385,12 +413,18 @@ func Enable(ctx context.Context, s release.Store, p Provisioner, sealPub *ecdh.P
return nil, err
}
- prev := ""
+ // Active is carried forward, not reset. A re-walk that repairs missing
+ // delivery for a paused app is legitimate; silently un-pausing it is
+ // not — Active only ever moves from false to true through the pause
+ // verb itself (Task 13), never as a side effect of this walk running.
+ // A first-time enable (no prior Provisioned block) starts active.
+ active, prev := true, ""
if rec.Provisioned != nil {
prev = rec.Provisioned.PrevKeyID
+ active = rec.Provisioned.Active
}
rec.Provisioned = &Provisioned{
- IAMUser: user, UserARN: arn, KeyID: keyID, PrevKeyID: prev, Active: true, At: now,
+ IAMUser: user, UserARN: arn, KeyID: keyID, PrevKeyID: prev, Active: active, At: now,
}
if _, err := Put(ctx, s, acct, app, rec, etag); err != nil {
return nil, err
@@ -411,36 +445,46 @@ func containsString(xs []string, want string) bool {
// never the IAM secret — delivering the secret would authenticate against
// nothing and would put a live AWS credential into an app's environment.
//
-// Both credential halves are sealed. The username is not itself sensitive,
-// but a plaintext access key id in a config manifest is a free inventory of
-// the estate for anyone who reads one, and sealing both costs nothing.
+// _USER (the access key id) is delivered as a plain "env" var, NOT sealed —
+// deliberately, reversing an earlier design here. It protects nothing:
+// rec.Provisioned.KeyID already sits in plaintext in email.json, in the
+// same bucket as config.json, so sealing it hides the value from nobody who
+// can't already read it there. Leaving it readable buys something real:
+// EnvDelivered can compare the delivered value against
+// rec.Provisioned.KeyID and catch the one failure mode that would
+// otherwise be permanent — a failed final Put after a successful
+// mint+deliver leaves the app running on a key no record names, the pause
+// aimed at a dead key, and the next rotation hitting IAM's two-key ceiling.
+// _PASS remains sealed — it IS the secret.
func writeSMTPEnv(ctx context.Context, s release.Store, sealPub *ecdh.PublicKey,
acct, app, prefix, region, keyID, secret, from string) error {
hostName, portName, userName, passName, fromName := EnvNames(prefix)
password := SMTPPassword(secret, region)
+ // Validate BEFORE sealing/delivering. A sealed value reaches appconfig
+ // as an opaque envelope, so an unrenderable byte would sail through and
+ // stall the app's whole config render on every host, hours later and
+ // out of sight. The errors name the VARIABLE and never the value —
+ // these strings reach logs and HTTP bodies, and one half is a live
+ // credential.
+ if err := appconfig.ValidVarValue(keyID); err != nil {
+ return fmt.Errorf("mail: minted credential for %s is not EnvironmentFile-safe: %w", userName, err)
+ }
+ if err := appconfig.ValidVarValue(password); err != nil {
+ return fmt.Errorf("mail: minted credential for %s is not EnvironmentFile-safe: %w", passName, err)
+ }
+ env, err := appconfig.Seal(sealPub, []byte(password))
+ if err != nil {
+ return fmt.Errorf("mail: seal %s: %w", passName, err)
+ }
+
sets := map[string]appconfig.Var{
hostName: {Kind: "env", Value: SMTPHost(region)},
portName: {Kind: "env", Value: SMTPPort},
fromName: {Kind: "env", Value: from},
- }
- // Validate BEFORE sealing. A sealed value reaches appconfig as an opaque
- // envelope, so an unrenderable byte would sail through and stall the
- // app's whole config render on every host, hours later and out of sight.
- // The errors name the VARIABLE and never the value — these strings reach
- // logs and HTTP bodies, and one half is a live credential.
- for _, half := range []struct{ name, plain string }{
- {userName, keyID}, {passName, password},
- } {
- if err := appconfig.ValidVarValue(half.plain); err != nil {
- return fmt.Errorf("mail: minted credential for %s is not EnvironmentFile-safe: %w", half.name, err)
- }
- env, err := appconfig.Seal(sealPub, []byte(half.plain))
- if err != nil {
- return fmt.Errorf("mail: seal %s: %w", half.name, err)
- }
- sets[half.name] = appconfig.Var{Kind: "secret", Envelope: env}
+ userName: {Kind: "env", Value: keyID},
+ passName: {Kind: "secret", Envelope: env},
}
if _, err := appconfig.Apply(ctx, s, acct, app, sets, nil); err != nil {
return fmt.Errorf("mail: deliver smtp config for %s/%s: %w", acct, app, err)
diff --git a/internal/mail/walk_enable_test.go b/internal/mail/walk_enable_test.go
index 49701750b..11bfddf9f 100644
--- a/internal/mail/walk_enable_test.go
+++ b/internal/mail/walk_enable_test.go
@@ -68,10 +68,12 @@ func TestEnableDeliversSeapointishsEnv(t *testing.T) {
if got := man.Vars["SEAPOINTISH_SMTP_FROM"].Value; got != "noreply@seapointish.bag.oncarlos.com" {
t.Fatalf("FROM = %q", got)
}
- // The username is the access key id and is NOT a secret; the password
- // is, and must arrive sealed.
- if man.Vars["SEAPOINTISH_SMTP_USER"].Kind != "secret" {
- t.Fatal("USER must be delivered as a secret alongside PASS")
+ // The username is the access key id and is NOT a secret — it is
+ // delivered as a plain env var, unsealed, precisely so it can be
+ // compared against rec.Provisioned.KeyID as a staleness check. The
+ // password IS the secret and must arrive sealed.
+ if got := man.Vars["SEAPOINTISH_SMTP_USER"]; got.Kind != "env" || got.Value != rec.Provisioned.KeyID {
+ t.Fatalf("USER = %+v, want Kind=env Value=%q", got, rec.Provisioned.KeyID)
}
if man.Vars["SEAPOINTISH_SMTP_PASS"].Kind != "secret" {
t.Fatal("PASS was not delivered as a sealed secret")
@@ -191,8 +193,6 @@ func TestEnableRefusesAnUndeclaredApp(t *testing.T) {
// one outside every verified domain.
func TestDeclareAcceptsAnyFromButEnableRefusesAnUngrantedOne(t *testing.T) {
ctx, s, p, pub, now := enableFixture(t)
- _ = p
- _ = pub
if _, err := Declare(ctx, s, "bag", "seapointish", "SEAPOINTISH_SMTP",
"noreply@somewhere-else.com", "paul@rslw.com", now); err != nil {
t.Fatalf("Declare itself may accept any From: %v", err)
@@ -202,3 +202,83 @@ func TestDeclareAcceptsAnyFromButEnableRefusesAnUngrantedOne(t *testing.T) {
t.Fatal("Enable accepted a From address outside every granted domain")
}
}
+
+// A pause must survive a re-walk. Task 13 adds the pause verb
+// (Provisioned.Active = false); nothing writes it yet, but Enable must
+// already carry an existing Active value forward rather than hardcoding
+// true, or the pause — whose whole purpose is stopping one tenant's bounce
+// rate from putting the shared SES account under review — would be undone
+// silently the next time delivery needed repair.
+func TestEnableCarriesAPausedActiveForwardThroughARewalk(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("first Enable: %v", err)
+ }
+
+ // Simulate a pause directly on the record — the way the pause verb
+ // (Task 13) will, once it exists.
+ got, etag, err := Get(ctx, s, "bag", "seapointish")
+ if err != nil {
+ t.Fatalf("Get: %v", err)
+ }
+ got.Provisioned.Active = false
+ if _, err := Put(ctx, s, "bag", "seapointish", got, etag); err != nil {
+ t.Fatalf("Put: %v", err)
+ }
+
+ // Remove a delivered var so the idempotence short-circuit cannot fire —
+ // this forces Enable to actually re-walk rather than return early.
+ if _, err := appconfig.Unset(ctx, s, "bag", "seapointish", []string{"SEAPOINTISH_SMTP_HOST"}); err != nil {
+ t.Fatalf("Unset: %v", err)
+ }
+
+ second, err := Enable(ctx, s, p, pub, testDeployment(), "bag", "seapointish", now)
+ if err != nil {
+ t.Fatalf("re-walk Enable: %v", err)
+ }
+ if second.Provisioned.Active {
+ t.Fatal("a re-walk silently resumed a paused credential")
+ }
+}
+
+// If the record ever names a key that is not what was actually delivered
+// — e.g. a failed final Put after a successful mint+deliver, or hand
+// edited bucket data — EnvDelivered must catch it via the _USER value
+// comparison, and Enable must re-walk rather than short-circuit on a
+// disagreement that would otherwise be permanent.
+func TestEnvDeliveredAndEnableCatchAStaleRecordedKeyID(t *testing.T) {
+ ctx, s, p, pub, now := enableFixture(t)
+ first, err := Enable(ctx, s, p, pub, testDeployment(), "bag", "seapointish", now)
+ if err != nil {
+ t.Fatalf("first Enable: %v", err)
+ }
+ deliveredKeyID := first.Provisioned.KeyID
+
+ got, etag, err := Get(ctx, s, "bag", "seapointish")
+ if err != nil {
+ t.Fatalf("Get: %v", err)
+ }
+ got.Provisioned.KeyID = "AKIASTALE00000000"
+ if _, err := Put(ctx, s, "bag", "seapointish", got, etag); err != nil {
+ t.Fatalf("Put: %v", err)
+ }
+
+ missing, err := EnvDelivered(ctx, s, "bag", "seapointish", "SEAPOINTISH_SMTP", "AKIASTALE00000000")
+ if err != nil {
+ t.Fatalf("EnvDelivered: %v", err)
+ }
+ if !containsString(missing, "SEAPOINTISH_SMTP_USER") {
+ t.Fatalf("missing = %v, want SEAPOINTISH_SMTP_USER reported", missing)
+ }
+
+ second, err := Enable(ctx, s, p, pub, testDeployment(), "bag", "seapointish", now)
+ if err != nil {
+ t.Fatalf("second Enable: %v", err)
+ }
+ if second.Provisioned.KeyID == "AKIASTALE00000000" {
+ t.Fatal("Enable short-circuited on a stale recorded key id instead of re-walking")
+ }
+ if second.Provisioned.KeyID == deliveredKeyID {
+ t.Fatal("re-walk should mint a fresh key, not silently reuse the originally delivered one")
+ }
+}