amadan
Merge pull request #279 from carlosframework/mail-refresh-policies
Grant before bind: domains add must not 554 the app it repairs. Merged with main brought in and the gate reproduced locally — GitHub Actions was retired from this repo (.github/workflows/ci.yml deleted) in favour of the amadan runner. 36 packages green, vet and gofmt clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3 files changed,
+182
−4
internal/mail/walk.go+96 −3internal/mail/walk_enable_test.go+84 −0mail_cli.go+2 −1
diff --git a/internal/mail/walk.go b/internal/mail/walk.go| index 687069906..cf2ada639 100644 |
| --- a/internal/mail/walk.go |
| +++ b/internal/mail/walk.go |
| @@ -227,9 +227,15 @@ func EnsureDomain(ctx context.Context, s release.Store, p Provisioner, w dnsrec. |
| // (this function's own doc comment on Deployment), and calling |
| // this with an empty name would either error at a real backend or |
| // silently bind to nothing. |
| - if err := p.SetIdentityConfigSet(ctx, d.Region, domain, configSet); err != nil { |
| - return nil, nil, fmt.Errorf("mail: bind identity %s to config set %s: %w", domain, configSet, err) |
| - } |
| + // |
| + // THE BIND ITSELF HAPPENS AT THE END OF THIS FUNCTION, not here, |
| + // and the gap is the whole point. SES authorises a configuration |
| + // set as a resource in its own right (#271), so an identity bound |
| + // to a set that the app's IAM policy does not name refuses every |
| + // send with a 554. Creating the set is harmless; binding it is not |
| + // — so the order has to be create, record, GRANT, then bind. Bind |
| + // here and there is a window, however short, in which this app |
| + // cannot send at all. |
| } |
| records := RecordSet(domain, d.Region, ident.DKIMTokens, d.DMARCRua) |
| @@ -274,9 +280,96 @@ func EnsureDomain(ctx context.Context, s release.Store, p Provisioner, w dnsrec. |
| if _, err := PutDomain(ctx, s, acct, rec, etag); err != nil { |
| return nil, nil, err |
| } |
| + |
| + // GRANT BEFORE BIND. Every app that sends on this domain — and every |
| + // standalone credential those apps hold — gets its policy recomputed |
| + // from the record just written, so the configuration-set ARN is in |
| + // place before anything is bound to it. |
| + // |
| + // This is not an optimisation, it is the difference between a repair |
| + // and an outage. EnsureUser is the only place a policy is written, and |
| + // it runs in Enable and CreateStandalone — never here. So before this, |
| + // an app enabled BEFORE a config set existed (every app on a |
| + // deployment that arms SNS later) got the set bound underneath it with |
| + // a policy that never mentioned it: 554 on every send. `carlos email |
| + // domains add` was the documented repair for missing events, and it |
| + // was also the thing that broke sending. |
| + if err := RefreshPolicies(ctx, s, p, acct, rec.Apps); err != nil { |
| + return nil, nil, err |
| + } |
| + if configSet != "" { |
| + if err := p.SetIdentityConfigSet(ctx, d.Region, domain, configSet); err != nil { |
| + return nil, nil, fmt.Errorf("mail: bind identity %s to config set %s: %w", domain, configSet, err) |
| + } |
| + } |
| return rec, pending, nil |
| } |
| +// RefreshPolicies rewrites the inline policy of every principal belonging to |
| +// each of apps, from the CURRENT granted domain set — the delivered |
| +// credential and every standalone one alike. |
| +// |
| +// It exists because the grant is a function of state that changes OUTSIDE |
| +// the walks that write it. Enable and CreateStandalone each recompute it for |
| +// one app at the moment they mint a key; nothing recomputed it when a |
| +// DOMAIN changed underneath an app that was already provisioned. Binding a |
| +// configuration set is exactly that kind of change, and so is a domain |
| +// joining or leaving the granted set. |
| +// |
| +// Silent on an app with nothing to grant: no record, no declaration, no |
| +// granted domains, or nothing provisioned yet all mean there is no principal |
| +// whose policy could be stale. An app that IS provisioned and whose grant |
| +// cannot be computed is an error, not a skip — that is a policy this |
| +// function was asked to refresh and could not. |
| +func RefreshPolicies(ctx context.Context, s release.Store, p Provisioner, acct string, apps []string) error { |
| + for _, app := range apps { |
| + rec, _, err := Get(ctx, s, acct, app) |
| + if err != nil { |
| + return err |
| + } |
| + if rec == nil || rec.Declared == nil { |
| + continue |
| + } |
| + if rec.Provisioned == nil && len(rec.Standalone) == 0 { |
| + continue |
| + } |
| + domains, err := grantedDomains(ctx, s, acct, app) |
| + if err != nil { |
| + return err |
| + } |
| + if len(domains) == 0 { |
| + continue |
| + } |
| + var arns, configSets, names []string |
| + for _, dom := range domains { |
| + arns = append(arns, dom.VerifiedARNs()...) |
| + configSets = append(configSets, dom.VerifiedConfigSetARNs()...) |
| + names = append(names, dom.Domain) |
| + } |
| + sort.Strings(arns) |
| + sort.Strings(configSets) |
| + sort.Strings(names) |
| + if len(arns) == 0 { |
| + // Granted domains exist but none is verified in any region, so |
| + // SendPolicyJSON would refuse anyway. Nothing to write. |
| + continue |
| + } |
| + if rec.Provisioned != nil { |
| + user := UserName(acct, app) |
| + if _, err := p.EnsureUser(ctx, user, UserPath, arns, configSets, names); err != nil { |
| + return fmt.Errorf("mail: refresh policy for %s: %w", user, err) |
| + } |
| + } |
| + for _, sa := range rec.Standalone { |
| + user := StandaloneUserName(acct, app, sa.Name) |
| + if _, err := p.EnsureUser(ctx, user, UserPath, arns, configSets, names); err != nil { |
| + return fmt.Errorf("mail: refresh policy for %s: %w", user, err) |
| + } |
| + } |
| + } |
| + return nil |
| +} |
| + |
| // ConfigSetName is the per-account SES configuration set that carries the |
| // event destination feeding bounce, complaint and send counts (spec §9). |
| func ConfigSetName(acct string) string { return "carlos-" + acct } |
diff --git a/internal/mail/walk_enable_test.go b/internal/mail/walk_enable_test.go| index 7977d968c..45561feb5 100644 |
| --- a/internal/mail/walk_enable_test.go |
| +++ b/internal/mail/walk_enable_test.go |
| @@ -142,6 +142,90 @@ func TestEnableDeliversTheDerivedPasswordNotTheSecret(t *testing.T) { |
| // A healthy provisioned app makes a re-run a no-op: no fresh key, no |
| // rewrite. Minting on every pass would burn IAM's two-key ceiling and |
| // churn every instance's config. |
| +// TestEnsureDomainRefreshesPoliciesWhenItBindsAConfigSet is the trap that |
| +// shipped in v0.12.0 and that #268's own advice walks a member straight into. |
| +// |
| +// EnsureDomain binds the identity to a configuration set. EnsureUser — the |
| +// only place a credential's policy is written — runs in Enable and |
| +// CreateStandalone, NOT here. So for an app enabled BEFORE a config set |
| +// existed (every app on a deployment that arms SNS later), `carlos email |
| +// domains add` bound the set and left the policy naming only the identity, |
| +// which SES answers with a 554 on every send. That is the #271 shape |
| +// arriving through the repair path instead of the provisioning one, and it |
| +// is strictly worse than the drift it was meant to fix: silent-no-events |
| +// becomes silent-no-sending. |
| +// |
| +// The fix is that binding and granting happen together. This test pins the |
| +// pairing rather than either half. |
| +func TestEnsureDomainRefreshesPoliciesWhenItBindsAConfigSet(t *testing.T) { |
| + ctx := context.Background() |
| + s, p := release.NewDirStore(t.TempDir()), NewMemProvisioner("1") |
| + now := time.Date(2026, 8, 24, 12, 0, 0, 0, time.UTC) |
| + |
| + _, pubB64, err := appconfig.GenerateKey() |
| + if err != nil { |
| + t.Fatalf("GenerateKey: %v", err) |
| + } |
| + pub, err := appconfig.ParsePublicKey(pubB64, "test") |
| + if err != nil { |
| + t.Fatalf("ParsePublicKey: %v", err) |
| + } |
| + |
| + // A deployment with NO SNS topic: the domain verifies and the app is |
| + // enabled with no configuration set anywhere — the ordinary shape |
| + // before event ingest is armed. |
| + noSNS := Deployment{Region: "eu-west-1", PlatformSuffix: "oncarlos.com"} |
| + if _, err := Declare(ctx, s, "bag", "seapointish", "SEAPOINTISH_SMTP", |
| + "noreply@seapointish.bag.oncarlos.com", "paul@rslw.com", now); err != nil { |
| + t.Fatalf("Declare: %v", err) |
| + } |
| + if _, _, err := EnsureDomain(ctx, s, p, dnsrec.NewMemWriter(), noSNS, |
| + "bag", "seapointish", "seapointish.bag.oncarlos.com", now); err != nil { |
| + t.Fatalf("EnsureDomain: %v", err) |
| + } |
| + p.SetIdentityStatus("eu-west-1", "seapointish.bag.oncarlos.com", |
| + Status{DKIM: "SUCCESS", MailFrom: "SUCCESS", Sending: "SUCCESS"}) |
| + if _, err := VerifyDomain(ctx, s, p, "bag", "seapointish.bag.oncarlos.com", "eu-west-1", now); err != nil { |
| + t.Fatalf("VerifyDomain: %v", err) |
| + } |
| + if _, err := Enable(ctx, s, p, pub, noSNS, "bag", "seapointish", now); err != nil { |
| + t.Fatalf("Enable: %v", err) |
| + } |
| + if _, err := CreateStandalone(ctx, s, p, noSNS, "bag", "seapointish", "smoke", "paul@rslw.com", now); err != nil { |
| + t.Fatalf("CreateStandalone: %v", err) |
| + } |
| + |
| + // Now the deployment arms SNS and the member follows the advice the |
| + // status warning prints. |
| + withSNS := testDeployment() |
| + if _, _, err := EnsureDomain(ctx, s, p, dnsrec.NewMemWriter(), withSNS, |
| + "bag", "seapointish", "seapointish.bag.oncarlos.com", now); err != nil { |
| + t.Fatalf("EnsureDomain (with SNS): %v", err) |
| + } |
| + |
| + dom, _, err := GetDomain(ctx, s, "bag", "seapointish.bag.oncarlos.com") |
| + if err != nil { |
| + t.Fatalf("GetDomain: %v", err) |
| + } |
| + want := dom.VerifiedConfigSetARNs() |
| + if len(want) == 0 { |
| + t.Fatal("no configuration set was bound, so this test proves nothing") |
| + } |
| + for _, u := range []string{UserName("bag", "seapointish"), StandaloneUserName("bag", "seapointish", "smoke")} { |
| + mu, ok := p.Users[u] |
| + if !ok { |
| + t.Fatalf("user %s does not exist", u) |
| + } |
| + for _, arn := range want { |
| + if !strings.Contains(mu.Policy, arn) { |
| + t.Errorf("%s: policy does not grant %s after the set was bound\n"+ |
| + "binding without refreshing the policy is a 554 on every send — worse than the drift it repairs\npolicy = %s", |
| + u, arn, mu.Policy) |
| + } |
| + } |
| + } |
| +} |
| + |
| // TestEnableGrantsTheConfigSetToTheDeliveredUser is the WALK half of issue |
| // #271, and it exists because mutation testing showed the policy-level tests |
| // could not see this: deleting the line in Enable that gathers config-set |
diff --git a/mail_cli.go b/mail_cli.go| index 05275a66a..e00d7dfba 100644 |
| --- a/mail_cli.go |
| +++ b/mail_cli.go |
| @@ -861,7 +861,8 @@ func printEmailStatus(w io.Writer, res apiEmailReply) { |
| if len(eventless) > 0 { |
| warnf(w, " no SES event destination in %s — sends, bounces and complaints are never counted for this domain, so the daily cap and the automatic pause cannot fire", |
| strings.Join(eventless, ", ")) |
| - mutedf(w, " re-run `carlos email domains add --app %s %s` to bind one — it is idempotent and leaves a verified identity verified", |
| + mutedf(w, " re-run `carlos email domains add --app %s %s` to bind one — it binds the event "+ |
| + "destination and refreshes this app's sending policy together, and leaves a verified identity verified", |
| res.App, d.Domain) |
| } |
| } |