amadan
mail: close cross-account domain claim gap (Task 17a, spec §4a)
SES identities are per-AWS-account, not per-tenant, so any domain one CARLOS account has verified is verified GLOBALLY at SES. EnsureDomain computed §4's IAM policy from a domain set that was never checked for ownership: a second account could call EnsureDomain on a domain another account already controlled, walk straight to EnsureIdentity, obtain the SAME identity ARN with no proof of its own, and Enable would hand it a policy granting ses:SendRawEmail at that domain. Both of §4's existing rails (verified-only ARNs, From-must-match-a-granted-domain) held perfectly and the tenant boundary still fell. Add a create-only, GLOBALLY-keyed claim (email/domain-claims/<domain>.json) mirroring internal/domains/claims.go's discipline: the bucket key IS the cross-account uniqueness check. EnsureDomain now claims the domain as its first durable side effect — before EnsureTenant, not just before EnsureIdentity, so a losing account's call buys no wasted backend work before being refused. The refusal (ErrDomainClaimed) never names the holding account; the caller's own account holding the claim is an ordinary idempotent success. Teardown releases the claim exactly when it deletes the account's own domain record — i.e. only when the last referring app goes — so a sibling app still using a domain keeps it held. Five mutations run and caught by the existing tests: dropping the claim check, scoping the key by account instead of globally, writing the claim after EnsureIdentity, naming the holder in the refusal, and skipping the release in Teardown — all five fail one or more tests with no changes needed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3 files changed,
+385
−5
internal/mail/domainclaim.go+144 −0internal/mail/walk.go+47 −5internal/mail/walk_domainclaim_test.go+194 −0
diff --git a/internal/mail/domainclaim.go b/internal/mail/domainclaim.go| new file mode 100644 |
| index 000000000..ed225ebe9 |
| --- /dev/null |
| +++ b/internal/mail/domainclaim.go |
| @@ -0,0 +1,144 @@ |
| +// The cross-account domain claim (Task 17a, spec §4a) — added after review |
| +// found a CRITICAL cross-tenant sending path in Task 17. |
| +// |
| +// SES identities are per-AWS-ACCOUNT, not per-tenant: a domain any CARLOS |
| +// account has verified is verified GLOBALLY at SES. Before this file |
| +// existed, EnsureDomain computed §4's IAM policy from a domain set that was |
| +// never checked for ownership — a second account could call EnsureDomain on |
| +// a domain the first account already controlled, obtain the SAME identity |
| +// ARN with no proof of its own, and Enable would build it a policy granting |
| +// ses:SendRawEmail with ses:FromAddress at that domain. Both of §4's rails |
| +// (verified-only ARNs, From-address-must-match-a-granted-domain) held |
| +// perfectly and the tenant boundary still fell, because both are computed |
| +// downstream of a domain set nothing ever gated on ownership. |
| +// |
| +// The fix mirrors internal/domains/claims.go's discipline rather than |
| +// inventing a second pattern: a create-only bucket key whose WRITE is the |
| +// uniqueness check. domains.AttachClaim gates hostnames the same way, at |
| +// the SAME key regardless of which account or app is asking, so two |
| +// accounts racing for one host cannot both win — there is only ever one |
| +// object to Put. This claim key is deliberately GLOBAL for the identical |
| +// reason: keying it under an account prefix (accounts/<sqid>/email/..., |
| +// the shape DomainKey already uses for the per-account identity record) |
| +// would let every account hold its OWN claim on the same domain, and the |
| +// whole point of this file is that they cannot. |
| +package mail |
| + |
| +import ( |
| + "context" |
| + "encoding/json" |
| + "errors" |
| + "fmt" |
| + "os" |
| + "time" |
| + |
| + "github.com/carlosframework/platform/internal/release" |
| +) |
| + |
| +// domainClaimsPrefix is where every email domain claim lives — a namespace |
| +// distinct from DomainsPrefix (which is per-account) precisely because this |
| +// one is not. |
| +const domainClaimsPrefix = "email/domain-claims/" |
| + |
| +func domainClaimKey(domain string) string { return domainClaimsPrefix + domain + ".json" } |
| + |
| +// domainClaim is the wire shape at domainClaimKey(Domain) — the global |
| +// record of which account claimed a sending domain first. |
| +type domainClaim struct { |
| + Schema int `json:"schema"` // 1 |
| + Domain string `json:"domain"` |
| + Account string `json:"account"` |
| + CreatedAt time.Time `json:"created_at"` |
| +} |
| + |
| +// ErrDomainClaimed is returned by EnsureDomain when the domain's global |
| +// claim is held by a DIFFERENT account. Deliberately neutral — it never |
| +// names the holder, so a caller cannot use this door as a cross-account |
| +// oracle for who owns a domain they do not. The caller's OWN account |
| +// holding the claim is never this error; see claimDomain. |
| +var ErrDomainClaimed = errors.New("mail: domain is claimed") |
| + |
| +// getDomainClaim reads domain's claim. A not-yet-claimed domain returns |
| +// (nil, nil), matching GetDomain's convention rather than propagating |
| +// os.ErrNotExist to every caller. |
| +func getDomainClaim(ctx context.Context, s release.Store, domain string) (*domainClaim, error) { |
| + c, _, err := getDomainClaimETag(ctx, s, domain) |
| + if errors.Is(err, os.ErrNotExist) { |
| + return nil, nil |
| + } |
| + return c, err |
| +} |
| + |
| +func getDomainClaimETag(ctx context.Context, s release.Store, domain string) (*domainClaim, string, error) { |
| + raw, etag, err := s.Get(ctx, domainClaimKey(domain)) |
| + if err != nil { |
| + return nil, "", err |
| + } |
| + var c domainClaim |
| + if err := json.Unmarshal(raw, &c); err != nil { |
| + return nil, "", fmt.Errorf("mail: domain claim %s: %w", domain, err) |
| + } |
| + // A schema newer than this binary understands is refused rather than |
| + // silently truncated by json.Unmarshal — domains.getClaimETag's |
| + // precedent for the same reason. |
| + if c.Schema > 1 { |
| + return nil, "", fmt.Errorf("mail: domain claim %s: schema %d is newer than this binary understands — upgrade carlos", domain, c.Schema) |
| + } |
| + return &c, etag, nil |
| +} |
| + |
| +// claimDomain is the gate: it must run, and succeed, before EnsureIdentity |
| +// ever does. The bucket key is the cross-account uniqueness check, exactly |
| +// as domains.AttachClaim's key is for hostnames — two accounts racing to |
| +// claim one domain cannot both win, because there is only ever one object |
| +// at domainClaimKey(domain) to Put. |
| +// |
| +// Same account re-claiming is idempotent success (a re-run of EnsureDomain |
| +// by its own owner must never be refused by the gate it itself holds). Any |
| +// other account gets ErrDomainClaimed, with no identity ever minted and no |
| +// domain record ever written — the refusal happens before either exists. |
| +func claimDomain(ctx context.Context, s release.Store, domain, acct string, now time.Time) error { |
| + body, err := json.Marshal(domainClaim{Schema: 1, Domain: domain, Account: acct, CreatedAt: now}) |
| + if err != nil { |
| + return err |
| + } |
| + for attempt := 0; ; attempt++ { |
| + cur, _, err := getDomainClaimETag(ctx, s, domain) |
| + if errors.Is(err, os.ErrNotExist) { |
| + if _, err := s.Put(ctx, domainClaimKey(domain), body, ""); err == nil { |
| + return nil |
| + } else if errors.Is(err, release.ErrCASConflict) && attempt == 0 { |
| + continue // lost the create race — re-read and judge the winner |
| + } else { |
| + return err |
| + } |
| + } else if err != nil { |
| + return err |
| + } |
| + if cur.Account != acct { |
| + return ErrDomainClaimed |
| + } |
| + return nil // already ours: idempotent no-op |
| + } |
| +} |
| + |
| +// releaseDomainClaim removes domain's claim, but only when it is held by |
| +// acct — the same owner-gated neutrality domains.DetachClaim observes, so |
| +// this can never be used to discover or disturb another account's claim. |
| +// A claim already absent is a quiet no-op: release, like the claim itself, |
| +// is idempotent. |
| +// |
| +// Callers: ONLY Teardown, and only after it has deleted the account's own |
| +// domain record — i.e. only when the last app referencing the domain has |
| +// gone. Releasing any earlier would let a stranger claim a domain a |
| +// sibling app is still sending from. |
| +func releaseDomainClaim(ctx context.Context, s release.Store, domain, acct string) error { |
| + cur, err := getDomainClaim(ctx, s, domain) |
| + if err != nil { |
| + return err |
| + } |
| + if cur == nil || cur.Account != acct { |
| + return nil |
| + } |
| + return s.Delete(ctx, domainClaimKey(domain)) |
| +} |
diff --git a/internal/mail/walk.go b/internal/mail/walk.go| index 5fb0f2e29..63b811615 100644 |
| --- a/internal/mail/walk.go |
| +++ b/internal/mail/walk.go |
| @@ -67,12 +67,34 @@ func DomainKindFor(d Deployment, domain string) string { |
| // It is one code path for both kinds because the record SET is identical; |
| // only the publisher differs. For a platform domain the walk writes every |
| // record and returns nothing pending. For a custom domain it writes nothing |
| -// and returns the whole set for the member to publish. |
| +// and returns the whole set for the member to publish. Platform domains are |
| +// NOT exempt from the claim below — the names are account-scoped by |
| +// construction so they are not more exposed, but the claim costs nothing |
| +// there and one code path beats a special case. |
| // |
| -// The order is: create the identity FIRST (it is what mints the DKIM tokens |
| -// the records name), then publish, then record. A crash between any two is |
| -// repaired by running again — the identity is idempotent and token-stable, |
| -// and the DNS writes are upserts. |
| +// The order, after validation: CLAIM the domain first, then create the |
| +// identity (which mints the DKIM tokens the records name), then publish, |
| +// then record. The claim is deliberately the very first durable side |
| +// effect this walk performs — before EnsureTenant, not just before |
| +// EnsureIdentity. Nothing about the domain the caller named is trustworthy |
| +// until the claim says it is theirs, and EnsureTenant is a real backend |
| +// call; doing it (or anything else) for a request the claim is about to |
| +// refuse would mean a losing account's call still bought it a wasted |
| +// SES/IAM round trip. Once the claim is confirmed, the rest of the walk is |
| +// self-healing on a re-run (identity idempotent and token-stable, DNS |
| +// writes are upserts) exactly as before — this task changes nothing about |
| +// that repair story, it only closes what happens before it starts. |
| +// |
| +// This ordering is spec §4a, added after review found the cross-tenant |
| +// hole this closes: SES identities are per-AWS-ACCOUNT, not per-tenant, so |
| +// any domain one account has verified is verified globally at SES. Without |
| +// the claim, a second account could call EnsureDomain on a domain another |
| +// account already controlled, walk straight to EnsureIdentity, and obtain |
| +// the SAME identity ARN with no proof of its own — Enable would then build |
| +// it an IAM policy granting ses:SendRawEmail at that domain. Writing the |
| +// claim AFTER EnsureIdentity (the reverse order) would not close this: two |
| +// accounts could each mint an identity and only discover the conflict from |
| +// the fleet, by which point both already hold a working ARN. |
| func EnsureDomain(ctx context.Context, s release.Store, p Provisioner, w dnsrec.Writer, |
| d Deployment, acct, app, domain string, now time.Time) (*DomainRecord, []dnsrec.Record, error) { |
| @@ -85,6 +107,14 @@ func EnsureDomain(ctx context.Context, s release.Store, p Provisioner, w dnsrec. |
| if !ValidDomain(domain) { |
| return nil, nil, fmt.Errorf("mail: invalid domain %q", domain) |
| } |
| + if err := claimDomain(ctx, s, domain, acct, now); err != nil { |
| + // Wrapping preserves errors.Is(err, ErrDomainClaimed) for callers |
| + // (the console door translates it into its own neutral refusal) — |
| + // %w keeps the chain — and the added context is just the domain |
| + // name the caller already supplied, never the account that holds |
| + // the claim. |
| + return nil, nil, fmt.Errorf("mail: claim domain %s: %w", domain, err) |
| + } |
| if err := p.EnsureTenant(ctx, acct); err != nil { |
| return nil, nil, fmt.Errorf("mail: ensure tenant %s: %w", acct, err) |
| } |
| @@ -887,6 +917,18 @@ func Teardown(ctx context.Context, s release.Store, p Provisioner, w dnsrec.Writ |
| if err := s.Delete(ctx, DomainKey(acct, cur.Domain)); err != nil { |
| return err |
| } |
| + // The global claim (Task 17a, spec §4a) releases HERE and only |
| + // here — right after, in the same branch as, deleting the account's |
| + // own domain record, which is the exact moment the last app |
| + // referencing this domain has gone. Releasing any earlier (e.g. at |
| + // the top of Teardown, or keyed off "this app is going" rather than |
| + // "the domain record is going") would let a stranger claim a domain |
| + // a sibling app is still sending from — the same class of bug |
| + // RemoveApp's own comment warns about for the identity/DNS cleanup |
| + // two lines above. |
| + if err := releaseDomainClaim(ctx, s, cur.Domain, acct); err != nil { |
| + return err |
| + } |
| } |
| if rec != nil { |
diff --git a/internal/mail/walk_domainclaim_test.go b/internal/mail/walk_domainclaim_test.go| new file mode 100644 |
| index 000000000..ceebe4905 |
| --- /dev/null |
| +++ b/internal/mail/walk_domainclaim_test.go |
| @@ -0,0 +1,194 @@ |
| +// Task 17a: the cross-account domain claim. Closes the hole review found in |
| +// Task 17 — SES identities are per-AWS-ACCOUNT, not per-tenant, so any |
| +// domain one CARLOS account verifies is verified globally. Before this, |
| +// EnsureDomain computed §4's IAM policy from a domain set that was never |
| +// checked for OWNERSHIP: a second account could call EnsureDomain on |
| +// "victim.com", get the same identity ARN with no proof of control, and |
| +// Enable would hand it a policy granting ses:SendRawEmail with |
| +// ses:FromAddress *@victim.com. These tests pin the fix: a create-only, |
| +// globally-keyed claim decided BEFORE EnsureIdentity ever runs. |
| +package mail |
| + |
| +import ( |
| + "context" |
| + "errors" |
| + "strings" |
| + "testing" |
| + "time" |
| + |
| + "github.com/carlosframework/platform/internal/dnsrec" |
| + "github.com/carlosframework/platform/internal/release" |
| +) |
| + |
| +// A second account claiming a domain the first already holds must be |
| +// refused outright: no identity, no domain record. This is the vulnerable |
| +// path from Task 17's review, closed. |
| +func TestEnsureDomainRefusesASecondAccountsClaim(t *testing.T) { |
| + ctx := context.Background() |
| + s, w := release.NewDirStore(t.TempDir()), dnsrec.NewMemWriter() |
| + pA, pB := NewMemProvisioner("A"), NewMemProvisioner("B") |
| + now := time.Date(2026, 8, 19, 12, 0, 0, 0, time.UTC) |
| + d := testDeployment() |
| + |
| + if _, _, err := EnsureDomain(ctx, s, pA, w, d, "aaa", "victimapp", "victim.com", now); err != nil { |
| + t.Fatalf("account A's first claim: %v", err) |
| + } |
| + |
| + _, _, err := EnsureDomain(ctx, s, pB, w, d, "bbb", "evilapp", "victim.com", now) |
| + if err == nil { |
| + t.Fatal("account B was allowed to claim a domain account A already holds") |
| + } |
| + if len(pB.Identities) != 0 { |
| + t.Fatalf("account B obtained an SES identity for a domain it does not own: %d identities", len(pB.Identities)) |
| + } |
| + if rec, _, err := GetDomain(ctx, s, "bbb", "victim.com"); err != nil { |
| + t.Fatalf("GetDomain: %v", err) |
| + } else if rec != nil { |
| + t.Fatalf("account B got a domain record for a domain it does not own: %+v", rec) |
| + } |
| +} |
| + |
| +// The refusal must be a cross-account information leak of exactly nothing: |
| +// a losing caller must not learn WHO holds the domain by reading the error. |
| +func TestEnsureDomainRefusalDoesNotNameTheHolder(t *testing.T) { |
| + ctx := context.Background() |
| + s, w := release.NewDirStore(t.TempDir()), dnsrec.NewMemWriter() |
| + pA, pB := NewMemProvisioner("A"), NewMemProvisioner("B") |
| + now := time.Date(2026, 8, 19, 12, 0, 0, 0, time.UTC) |
| + d := testDeployment() |
| + |
| + if _, _, err := EnsureDomain(ctx, s, pA, w, d, "secretacct", "victimapp", "victim.com", now); err != nil { |
| + t.Fatalf("account A's first claim: %v", err) |
| + } |
| + _, _, err := EnsureDomain(ctx, s, pB, w, d, "bbb", "evilapp", "victim.com", now) |
| + if err == nil { |
| + t.Fatal("expected a refusal") |
| + } |
| + if strings.Contains(err.Error(), "secretacct") { |
| + t.Fatalf("refusal named the holding account: %v", err) |
| + } |
| + if !errors.Is(err, ErrDomainClaimed) { |
| + t.Fatalf("refusal was not ErrDomainClaimed: %v", err) |
| + } |
| +} |
| + |
| +// The owning account re-running EnsureDomain on its own claimed domain is |
| +// ordinary idempotent success — a claim must never block its own holder. |
| +func TestEnsureDomainOwnAccountReRunIsIdempotent(t *testing.T) { |
| + ctx := context.Background() |
| + s, p, w := release.NewDirStore(t.TempDir()), NewMemProvisioner("A"), dnsrec.NewMemWriter() |
| + now := time.Date(2026, 8, 19, 12, 0, 0, 0, time.UTC) |
| + d := testDeployment() |
| + |
| + first, _, err := EnsureDomain(ctx, s, p, w, d, "aaa", "myapp", "mine.com", now) |
| + if err != nil { |
| + t.Fatalf("first: %v", err) |
| + } |
| + second, _, err := EnsureDomain(ctx, s, p, w, d, "aaa", "myapp", "mine.com", now) |
| + if err != nil { |
| + t.Fatalf("second: %v", err) |
| + } |
| + f, sec := first.Regions["eu-west-1"], second.Regions["eu-west-1"] |
| + if f.IdentityARN != sec.IdentityARN || f.DKIMTokens[0] != sec.DKIMTokens[0] { |
| + t.Fatal("re-running under the SAME account changed identity/tokens") |
| + } |
| + if len(p.Identities) != 1 { |
| + t.Fatalf("identities = %d, want 1", len(p.Identities)) |
| + } |
| +} |
| + |
| +// Teardown of the last app referencing a domain must release the global |
| +// claim, and only then — a sibling app still using the domain must keep it |
| +// held. Once released, another account can claim it. |
| +func TestTeardownReleasesClaimOnlyWhenLastAppGoes(t *testing.T) { |
| + ctx := context.Background() |
| + s, p, w := release.NewDirStore(t.TempDir()), NewMemProvisioner("A"), dnsrec.NewMemWriter() |
| + now := time.Date(2026, 8, 19, 12, 0, 0, 0, time.UTC) |
| + d := testDeployment() |
| + |
| + if _, _, err := EnsureDomain(ctx, s, p, w, d, "aaa", "app1", "shared.com", now); err != nil { |
| + t.Fatalf("app1 ensure: %v", err) |
| + } |
| + if _, _, err := EnsureDomain(ctx, s, p, w, d, "aaa", "app2", "shared.com", now); err != nil { |
| + t.Fatalf("app2 ensure: %v", err) |
| + } |
| + |
| + // Tear down app1: app2 still references the domain, so the claim must |
| + // stay held — a stranger must NOT be able to claim a live domain out |
| + // from under a sibling app. |
| + if err := Teardown(ctx, s, p, w, d, "aaa", "app1"); err != nil { |
| + t.Fatalf("teardown app1: %v", err) |
| + } |
| + pStranger := NewMemProvisioner("B") |
| + if _, _, err := EnsureDomain(ctx, s, pStranger, w, d, "bbb", "strangerapp", "shared.com", now); err == nil { |
| + t.Fatal("a stranger claimed a domain still referenced by a sibling app") |
| + } |
| + |
| + // Tear down app2, the last referrer: NOW the claim must release. |
| + if err := Teardown(ctx, s, p, w, d, "aaa", "app2"); err != nil { |
| + t.Fatalf("teardown app2: %v", err) |
| + } |
| + rec, _, err := EnsureDomain(ctx, s, pStranger, w, d, "bbb", "strangerapp", "shared.com", now) |
| + if err != nil { |
| + t.Fatalf("stranger should be able to claim the released domain: %v", err) |
| + } |
| + if rec == nil || len(pStranger.Identities) != 1 { |
| + t.Fatal("stranger did not actually get a fresh identity after the claim released") |
| + } |
| +} |
| + |
| +// A platform-suffixed domain is not exempt from the claim gate — one code |
| +// path, no special case, even though these names are account-scoped by |
| +// construction. |
| +func TestEnsureDomainClaimsAPlatformDomainToo(t *testing.T) { |
| + ctx := context.Background() |
| + s, p, w := release.NewDirStore(t.TempDir()), NewMemProvisioner("A"), dnsrec.NewMemWriter() |
| + now := time.Date(2026, 8, 19, 12, 0, 0, 0, time.UTC) |
| + d := testDeployment() |
| + |
| + if _, _, err := EnsureDomain(ctx, s, p, w, d, "aaa", "seapointish", "seapointish.aaa.oncarlos.com", now); err != nil { |
| + t.Fatalf("EnsureDomain: %v", err) |
| + } |
| + claim, err := getDomainClaim(ctx, s, "seapointish.aaa.oncarlos.com") |
| + if err != nil { |
| + t.Fatalf("getDomainClaim: %v", err) |
| + } |
| + if claim == nil || claim.Account != "aaa" { |
| + t.Fatalf("platform domain did not get a claim: %+v", claim) |
| + } |
| +} |
| + |
| +// The claim must be the FIRST durable side effect — written before |
| +// EnsureIdentity ever runs. Inject a one-shot failure at EnsureIdentity: |
| +// the claim must already be there, but no identity was created. A re-run by |
| +// the SAME account must then succeed — the claim must not block its own |
| +// owner, even though it was written before the walk that owns it finished. |
| +func TestEnsureDomainWritesClaimBeforeIdentity(t *testing.T) { |
| + ctx := context.Background() |
| + s, p, w := release.NewDirStore(t.TempDir()), NewMemProvisioner("A"), dnsrec.NewMemWriter() |
| + now := time.Date(2026, 8, 19, 12, 0, 0, 0, time.UTC) |
| + d := testDeployment() |
| + |
| + p.FailNext["EnsureIdentity"] = errors.New("SES down") |
| + if _, _, err := EnsureDomain(ctx, s, p, w, d, "aaa", "myapp", "mine.com", now); err == nil { |
| + t.Fatal("EnsureDomain succeeded despite an injected EnsureIdentity failure") |
| + } |
| + claim, err := getDomainClaim(ctx, s, "mine.com") |
| + if err != nil { |
| + t.Fatalf("getDomainClaim: %v", err) |
| + } |
| + if claim == nil || claim.Account != "aaa" { |
| + t.Fatalf("claim missing after the injected failure: %+v", claim) |
| + } |
| + if len(p.Identities) != 0 { |
| + t.Fatalf("identity was created despite the injected failure: %d", len(p.Identities)) |
| + } |
| + |
| + rec, _, err := EnsureDomain(ctx, s, p, w, d, "aaa", "myapp", "mine.com", now) |
| + if err != nil { |
| + t.Fatalf("re-run by the claim's own owner must succeed: %v", err) |
| + } |
| + if rec == nil || len(p.Identities) != 1 { |
| + t.Fatal("re-run did not complete provisioning") |
| + } |
| +} |