amadan
email: release a sending-domain claim without deleting the app
A sending domain's claim is deliberately global and create-only, so
exactly one CARLOS account can hold it. Nothing could ever give one
back. The only caller of releaseDomainClaim was mail.Teardown, and the
only caller of Teardown is the PERMANENT app deletion handler — so a
domain claimed by the wrong app was unavailable to every other account,
including the one that actually controls its DNS, for the life of the
deployment.
It bit console.platform.tito.io: stripe/jelly holds a claim on
jelly.tito.io, jam/jam serves that host and wanted to send from it, and
`email domains add` was refused with a conflict no command could clear.
The workaround was to put jam on a different domain.
This adds the door: `carlos email domains rm --app <a> <domain>`, the
DELETE route behind it, and mail.RemoveDomain.
The release sequence is not copied. Teardown's per-domain loop body is
extracted into detachDomainApp, which both callers now run, because its
ordering is load-bearing and subtle in three places at once: a domain a
sibling app still refers to keeps its identity, its DNS and its claim;
the claim releases only AFTER DeleteIdentity and only BEFORE the record
delete (the other order strands the claim forever, since ListDomains
would never name the domain again); and a custom domain's DNS is never
deleted, only reported. Teardown's orphan-claim reconciliation pass
stays where it is — a different concern.
Three refusals and one repair are RemoveDomain's own:
- the domain the app currently sends FROM is refused, naming the From
and the verb that moves it. Enable rejects a From that is not at a
granted domain, so allowing it would break sending at the next
re-enable instead of at the command that caused it.
- a domain this app does not use is a 404, never a quiet 200.
- a claim held by ANOTHER account is the same 404, in the same words,
so the door cannot be used as an oracle for who holds what.
releaseDomainClaim's owner gate is untouched and unrouted-around.
- a claim with NO backing record — what EnsureDomain leaves behind
when it fails after claiming and before writing — is swept, one
named domain at a time. Only Teardown could reach that state before,
and only for the same app.
RemoveDomain also shrinks the departing app's IAM policy. EnsureUser
writes a policy wholesale from the current granted set and nothing
recomputed it when a domain LEFT that set, so an app dropping a domain a
sibling keeps alive went on holding ses:SendRawEmail at a live identity
ARN — a real over-grant, not a stale string.
The CLI prints a custom domain's now-orphaned records rather than
deleting them, split into the three DKIM CNAMEs (dead with the identity)
and the MX/SPF/DMARC set (deterministic, still correct if the domain is
ever added back).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
9 files changed,
+1393
−110
docs/site/cli.md+13 −0internal/console/apimail.go+129 −7internal/console/apimail_test.go+196 −3internal/console/server.go+1 −1internal/mail/domainclaim.go+23 −11internal/mail/walk.go+335 −83internal/mail/walk_domainremove_test.go+442 −0mail_cli.go+160 −4mail_cli_test.go+94 −1
CI passed — run details
diff --git a/docs/site/cli.md b/docs/site/cli.md| index 6abbbc90f..e9f387255 100644 |
| --- a/docs/site/cli.md |
| +++ b/docs/site/cli.md |
| @@ -1100,6 +1100,19 @@ keeps polling until SES confirms them, giving up after ten minutes unless |
| SES never confirmed. `carlos email domains add` is that same half on its own, |
| for a second domain on an app already declared. |
| +`carlos email domains rm --app hello example.com` is the way back out. A |
| +sending domain can be claimed by exactly one CARLOS account at a time, so a |
| +domain one app holds is unavailable to everybody else — including the |
| +account that actually controls its DNS — until something releases it. `rm` |
| +is that something: it stops the app sending from the domain and, when it was |
| +the last app using it, deletes the SES identity and releases the claim, so |
| +another account can add and verify the domain. It refuses while the domain is |
| +where the app's own From address sits — move that first with `carlos email |
| +enable --app hello --from <addr>`. A custom domain's DNS records are yours, |
| +so `rm` prints them rather than deleting them: the DKIM CNAMEs are dead with |
| +the identity, while the MX, SPF and DMARC records stay correct if you ever |
| +add the domain back. |
| + |
| `carlos email test` sends a real message and reports what SES said about it. |
| It sends through a throwaway standalone credential it mints and revokes |
| around the send, so it never needs the delivered credential's password, and |
diff --git a/internal/console/apimail.go b/internal/console/apimail.go| index 5a22ae60a..baf362816 100644 |
| --- a/internal/console/apimail.go |
| +++ b/internal/console/apimail.go |
| @@ -499,7 +499,7 @@ func (s *Server) mailWalkReply(ctx context.Context, w http.ResponseWriter, acct |
| writeJSON(w, http.StatusOK, out) |
| } |
| -// registerEmailRoutes wires this file's nine doors onto s.mux, through the |
| +// registerEmailRoutes wires this file's ten doors onto s.mux, through the |
| // role gate (cliRoute) like every other /api/cli/ door. It stays a separate |
| // function from registerCLIRoutes only so the email family's reasoning lives |
| // beside the handlers it describes; svcroutes_test's AST pass is |
| @@ -507,12 +507,19 @@ func (s *Server) mailWalkReply(ctx context.Context, w http.ResponseWriter, acct |
| // |
| // THE ROLE TABLE, and why each door reads as it does. |
| // |
| -// The six OPEN doors take Allow(Admin), matching the store family verb for |
| -// verb — declare, ensure, verify, enable, rotate all move an account's |
| -// backend estate, and `enable` mints a path-scoped IAM user exactly as |
| -// `store grant` does. Admin is the union role, so Publish/Instance/Operate |
| -// are refused: a credential that may deploy an app must not thereby be able |
| -// to give it a sending identity on a domain the account owns. |
| +// The seven OPEN doors take Allow(Admin), matching the store family verb |
| +// for verb — declare, ensure, verify, REMOVE, enable, rotate all move an |
| +// account's backend estate, and `enable` mints a path-scoped IAM user |
| +// exactly as `store grant` does. Admin is the union role, so |
| +// Publish/Instance/Operate are refused: a credential that may deploy an app |
| +// must not thereby be able to give it a sending identity on a domain the |
| +// account owns. |
| +// |
| +// `domains rm` is Admin because `domains add` is: the same authority, run |
| +// backwards. It is deliberately NOT closed or operator-gated — the whole |
| +// defect it exists to fix is that releasing a claim required an authority |
| +// (permanent app deletion) far larger than claiming one ever did, which is |
| +// what left real deployments with a domain nobody could ever free. |
| // |
| // GET .../email is Admin rather than a read role for the same reason GET |
| // .../store is: the status reply names the IAM user, the key id and the |
| @@ -561,6 +568,7 @@ func (s *Server) registerEmailRoutes() { |
| s.cliRoute("DELETE /api/cli/apps/{account}/{app}/email/events/push", svcrole.Allow(svcrole.Admin), s.handleAPIEmailPushOff) |
| s.cliRoute("POST /api/cli/apps/{account}/{app}/email/domains", svcrole.Allow(svcrole.Admin), s.handleAPIEmailDomainEnsure) |
| s.cliRoute("POST /api/cli/apps/{account}/{app}/email/domains/{domain}/verify", svcrole.Allow(svcrole.Admin), s.handleAPIEmailDomainVerify) |
| + s.cliRoute("DELETE /api/cli/apps/{account}/{app}/email/domains/{domain}", svcrole.Allow(svcrole.Admin), s.handleAPIEmailDomainRemove) |
| s.cliRoute("POST /api/cli/apps/{account}/{app}/email/enable", svcrole.Allow(svcrole.Admin), s.handleAPIEmailEnable) |
| s.cliRoute("POST /api/cli/apps/{account}/{app}/email/rotate", svcrole.Allow(svcrole.Admin), s.handleAPIEmailRotate) |
| s.cliRoute("POST /api/cli/apps/{account}/{app}/email/credentials", svcrole.Set{}, s.handleAPIEmailCredentialsCreate) |
| @@ -1101,6 +1109,120 @@ func (s *Server) handleAPIEmailDomainVerify(w http.ResponseWriter, r *http.Reque |
| writeJSON(w, http.StatusOK, map[string]any{"domain": rec}) |
| } |
| +// handleAPIEmailDomainRemove answers `carlos email domains rm`: stop this |
| +// app sending from a domain and, when it was the last app doing so, give |
| +// the domain's GLOBAL claim back. |
| +// |
| +// THE DOOR THE CLAIM ALWAYS NEEDED. The claim (internal/mail/domainclaim.go) |
| +// is deliberately a create-only, globally-keyed object so two accounts |
| +// cannot both hold one domain — right, and unchanged by this door. What was |
| +// missing was the way out: until this existed, mail.Teardown, reached only |
| +// by PERMANENTLY DELETING the whole app, was the one caller that could ever |
| +// release a claim, so a domain claimed by the wrong app stayed unreachable |
| +// to every other account — including the account that actually controls its |
| +// DNS — for the life of the deployment. |
| +// |
| +// A MEMBER VERB, Admin like `domains add`, for the same reason: it spends |
| +// nothing that exceeds the account's tier, and it is the exact inverse of a |
| +// door an admin already has. It can never touch an estate outside the |
| +// caller's own account — mail.RemoveDomain reads and releases only through |
| +// the owner-gated releaseDomainClaim, and a claim held by anyone else is |
| +// refused in the same words as a domain that does not exist. |
| +// |
| +// Account-scoped like the record itself (domain.go's package comment), so |
| +// {app} is the membership gate AND — unlike verify — the referrer actually |
| +// being removed. The one exception is a claim with no record at all, where |
| +// there is no referrer set to be in; mail.removeOrphanedClaim documents why |
| +// the gate reading is the right one there. |
| +func (s *Server) handleAPIEmailDomainRemove(w http.ResponseWriter, r *http.Request) { |
| + addr, acct, app, ok := s.apiAccountApp(w, r) |
| + if !ok { |
| + return |
| + } |
| + if !s.mailWriteGates(w, addr) { |
| + return |
| + } |
| + domain := strings.ToLower(strings.TrimSpace(r.PathValue("domain"))) |
| + if !mail.ValidDomain(domain) { |
| + jsonError(w, http.StatusBadRequest, fmt.Sprintf("invalid domain %q", r.PathValue("domain"))) |
| + return |
| + } |
| + ctx, cancel := context.WithTimeout(r.Context(), mailDomainTimeout) |
| + defer cancel() |
| + d, writer := s.mailDeploymentFor() |
| + out, err := mail.RemoveDomain(ctx, s.cfg.Releases, s.cfg.MailProvisioner, writer, d, acct.Sqid, app, domain) |
| + if err != nil { |
| + // The From refusal is a 409, not a 400: it is a conflict with state |
| + // the caller can change (move the From, then remove the domain), |
| + // which is precisely what this message has to say. Contrast the |
| + // ensure door's 409, which is permanent — nothing the caller does |
| + // clears another account's claim. |
| + if errors.Is(err, mail.ErrDomainIsFrom) { |
| + jsonError(w, http.StatusConflict, s.mailFromConflictMessage(ctx, acct, app, domain)) |
| + return |
| + } |
| + // 404 covers both "this app does not use that domain" and "that |
| + // domain's claim belongs to another account", deliberately in the |
| + // same words — see mail.ErrDomainNotReferenced. |
| + if errors.Is(err, mail.ErrDomainNotReferenced) { |
| + jsonError(w, http.StatusNotFound, fmt.Sprintf( |
| + "no sending domain %q for this app — `carlos email status --app %s` lists the ones it has", domain, app)) |
| + return |
| + } |
| + if out != nil { |
| + // The removal LANDED; only the departing app's IAM policy |
| + // shrink failed (mail.RemoveDomain's own doc comment). Saying |
| + // "re-run" here would be wrong — a re-run answers 404 now — |
| + // so name the verb that actually repays the debt. |
| + log.Printf("console: api email domain remove %s/%s (%s): policy refresh: %v", acct.Sqid, app, domain, err) |
| + s.audit(acct, app, addr, "email-domain-remove", domain+" (cli; policy refresh failed)") |
| + jsonError(w, http.StatusBadGateway, |
| + "the domain was removed, but this app's sending policy could not be rewritten — run `carlos email enable --app "+app+"` to repair it") |
| + return |
| + } |
| + log.Printf("console: api email domain remove %s/%s (%s): %v", acct.Sqid, app, domain, err) |
| + jsonError(w, http.StatusBadGateway, |
| + "could not remove the sending domain — nothing unrecorded was left behind; re-run to resume") |
| + return |
| + } |
| + detail := domain + " (" + out.Kind + ")" |
| + switch { |
| + case out.ClaimOnly: |
| + detail += " released a claim with no domain record" |
| + case out.TornDown: |
| + detail += " torn down, claim released" |
| + default: |
| + detail += " still used by " + strings.Join(out.Remaining, ", ") |
| + } |
| + s.audit(acct, app, addr, "email-domain-remove", detail+" (cli)") |
| + // No bumpSerial: nothing in the app's own config changed — the delivered |
| + // SMTP variables are untouched by a domain leaving the granted set, and |
| + // the app's From is guaranteed by the refusal above to be somewhere |
| + // else. |
| + writeJSON(w, http.StatusOK, map[string]any{ |
| + "app": app, |
| + "domain": out.Domain, |
| + "kind": out.Kind, |
| + "torn_down": out.TornDown, |
| + "claim_only": out.ClaimOnly, |
| + "remaining_apps": out.Remaining, |
| + "orphaned": dnsRecordsOut(out.Orphaned), |
| + }) |
| +} |
| + |
| +// mailFromConflictMessage names the From that blocks a domain removal, and |
| +// the command that moves it. Re-reading the record on this one error path |
| +// costs nothing and is the difference between a member knowing what to do |
| +// next and being told only that they may not. |
| +func (s *Server) mailFromConflictMessage(ctx context.Context, acct *Account, app, domain string) string { |
| + msg := fmt.Sprintf("this app still sends from %s", domain) |
| + if rec, _, err := mail.Get(ctx, s.cfg.Releases, acct.Sqid, app); err == nil && rec != nil && rec.Declared != nil { |
| + msg = fmt.Sprintf("this app still sends as %s", rec.Declared.From) |
| + } |
| + return msg + fmt.Sprintf(" — move its From to another verified domain first"+ |
| + " (`carlos email enable --app %s --from <addr>`), then remove %s", app, domain) |
| +} |
| + |
| // ---- enable ----------------------------------------------------------------- |
| // handleAPIEmailEnable answers `carlos email enable`: provision the |
diff --git a/internal/console/apimail_test.go b/internal/console/apimail_test.go| index 0dad941d3..62af15b14 100644 |
| --- a/internal/console/apimail_test.go |
| +++ b/internal/console/apimail_test.go |
| @@ -2,7 +2,7 @@ package console |
| // The member half of the email API (apimail.go, tenant-email task 17). What |
| // is pinned here, in the order it would hurt if lost: a stranger probing any |
| -// of the nine doors learns nothing (the byte-identical 404 the whole |
| +// of the ten doors learns nothing (the byte-identical 404 the whole |
| // /api/cli surface keeps, and — new to this file — the same 404 when this |
| // deployment has no SES estate at all); the standalone-credential reply is |
| // the only one that ever carries a password, and it is uncacheable; a |
| @@ -150,7 +150,7 @@ func emailRecord(t *testing.T, fx *consoleFixture, app string) *mail.Record { |
| // ---- the gate --------------------------------------------------------------- |
| -// TestEmailDoorsAre404ForNonMembers: on all nine doors, a token that belongs |
| +// TestEmailDoorsAre404ForNonMembers: on all ten doors, a token that belongs |
| // to no account, an account segment that doesn't exist, and an app the |
| // account never claimed are one and the same 404 — byte-identical to |
| // http.NotFound's own output. This runs on an UNARMED fixture (no |
| @@ -183,6 +183,10 @@ func TestEmailDoorsAre404ForNonMembers(t *testing.T) { |
| {"POST domains verify", func(base, bearer string) (int, string) { |
| return postRaw(t, base+"/domains/example.com/verify", map[string]any{}, bearer) |
| }}, |
| + {"DELETE domains", func(base, bearer string) (int, string) { |
| + status, _, body := rawReq(t, http.MethodDelete, base+"/domains/example.com", bearer, "") |
| + return status, body |
| + }}, |
| {"POST enable", func(base, bearer string) (int, string) { |
| return postRaw(t, base+"/enable", map[string]any{}, bearer) |
| }}, |
| @@ -253,6 +257,10 @@ func TestEmailDoorsAre404WithNoProvisioner(t *testing.T) { |
| {"POST domains verify", func() (int, string) { |
| return postRaw(t, emailURL(fx, "titogo")+"/domains/example.com/verify", map[string]any{}, token) |
| }}, |
| + {"DELETE domains", func() (int, string) { |
| + status, _, body := rawReq(t, http.MethodDelete, emailURL(fx, "titogo")+"/domains/example.com", token, "") |
| + return status, body |
| + }}, |
| {"POST enable", func() (int, string) { |
| return postRaw(t, emailURL(fx, "titogo")+"/enable", map[string]any{}, token) |
| }}, |
| @@ -284,7 +292,7 @@ func TestEmailDoorsAre404WithNoProvisioner(t *testing.T) { |
| // TestOperatorStatusBuysNothingOnMemberDoors: I3 moved the pause door off |
| // apiAccountApp's member gate onto store.IsOperator — this proves that |
| -// move did not leak into the other eight. A fleet operator (IsOperator |
| +// move did not leak into the other nine. A fleet operator (IsOperator |
| // true) who is NOT a member of the target account gets the exact same |
| // byte-identical 404 an ordinary stranger gets on a WRITE (declare) and a |
| // READ (status) member door. Operator status must buy nothing here. |
| @@ -786,6 +794,191 @@ func TestAPIEmailDomainVerifyRecordsSESVerdict(t *testing.T) { |
| auditedBy(t, fx, app, "email-domain-verify", "alice@keymail.dev", "verified") |
| } |
| +// ---- domains rm: the release door ----------------------------------------- |
| + |
| +// removeEmailDomain drives the DELETE door and hands back the status and |
| +// the decoded reply — a 200 and a refusal are both interesting here, so it |
| +// never fails the test itself. |
| +func removeEmailDomain(t *testing.T, fx *consoleFixture, token, app, domain string) (int, map[string]any) { |
| + t.Helper() |
| + status, _, body := rawReq(t, http.MethodDelete, emailURL(fx, app)+"/domains/"+domain, token, "") |
| + out := map[string]any{} |
| + if err := json.Unmarshal([]byte(body), &out); err != nil { |
| + t.Fatalf("remove %s reply is not JSON (%d): %q", domain, status, body) |
| + } |
| + return status, out |
| +} |
| + |
| +// secondApp claims another app in the same account, so a domain can have |
| +// two referrers — the state the whole sibling rule exists for. |
| +func secondApp(t *testing.T, fx *consoleFixture, name string) string { |
| + t.Helper() |
| + if _, err := fx.st.CreateAppIn(OpsAccountID, name, "paul@keymail.dev", time.Now()); err != nil { |
| + t.Fatal(err) |
| + } |
| + return name |
| +} |
| + |
| +// TestAPIEmailDomainRemoveReleasesTheGlobalClaim: the defect this door |
| +// exists for. A claimed domain was unreachable to every other CARLOS |
| +// account — including the one that actually controls its DNS — until the |
| +// app holding it was PERMANENTLY DELETED, because mail.Teardown was the |
| +// only caller that could ever release a claim. After this, the claim object |
| +// is gone and the domain is free. |
| +func TestAPIEmailDomainRemoveReleasesTheGlobalClaim(t *testing.T) { |
| + fx, _, _, token := emailArmed(t, "code-email-domain-rm-claim") |
| + app, domain := "titogo", "mail.example.com" |
| + declareEmail(t, fx, token, app, "hello@elsewhere.example.com") |
| + ensureEmailDomain(t, fx, token, app, domain) |
| + |
| + claims, err := mail.ListDomainClaims(context.Background(), fx.srv.cfg.Releases) |
| + if err != nil || len(claims) != 1 || claims[0].Domain != domain { |
| + t.Fatalf("fixture broken: claims = %+v (err %v)", claims, err) |
| + } |
| + |
| + status, out := removeEmailDomain(t, fx, token, app, domain) |
| + if status != http.StatusOK { |
| + t.Fatalf("remove = %d %v, want 200", status, out) |
| + } |
| + if out["torn_down"] != true { |
| + t.Fatalf("torn_down = %v, want true — this was the only referrer", out["torn_down"]) |
| + } |
| + claims, err = mail.ListDomainClaims(context.Background(), fx.srv.cfg.Releases) |
| + if err != nil || len(claims) != 0 { |
| + t.Fatalf("the global claim survived the remove: %+v (err %v)", claims, err) |
| + } |
| + auditedBy(t, fx, app, "email-domain-remove", "alice@keymail.dev", domain) |
| + if got := serial(t, fx); got != 0 { |
| + t.Fatalf("serial after removing a domain = %d, want 0 — nothing in the app's own config changed", got) |
| + } |
| +} |
| + |
| +// TestAPIEmailDomainRemoveKeepsADomainASiblingAppUses: with another app |
| +// still referring to the domain, the door removes ONLY the referral. The |
| +// identity, the DNS and the claim all stay — releasing them here would |
| +// revoke a sibling app's verified domain silently. |
| +func TestAPIEmailDomainRemoveKeepsADomainASiblingAppUses(t *testing.T) { |
| + fx, fake, _, token := emailArmed(t, "code-email-domain-rm-sibling") |
| + domain := "shared.example.com" |
| + other := secondApp(t, fx, "marketing") |
| + declareEmail(t, fx, token, "titogo", "hello@elsewhere.example.com") |
| + ensureEmailDomain(t, fx, token, "titogo", domain) |
| + ensureEmailDomain(t, fx, token, other, domain) |
| + |
| + status, out := removeEmailDomain(t, fx, token, "titogo", domain) |
| + if status != http.StatusOK { |
| + t.Fatalf("remove = %d %v, want 200", status, out) |
| + } |
| + if out["torn_down"] != false { |
| + t.Fatalf("torn_down = %v, want false — %s still uses the domain", out["torn_down"], other) |
| + } |
| + if got := strList(t, out, "remaining_apps"); len(got) != 1 || got[0] != other { |
| + t.Fatalf("remaining_apps = %v, want [%s]", got, other) |
| + } |
| + claims, err := mail.ListDomainClaims(context.Background(), fx.srv.cfg.Releases) |
| + if err != nil || len(claims) != 1 { |
| + t.Fatalf("the claim was released while a sibling app still sends from the domain: %+v (err %v)", claims, err) |
| + } |
| + if len(fake.Identities) == 0 { |
| + t.Fatal("the sibling app's SES identity was destroyed") |
| + } |
| +} |
| + |
| +// TestAPIEmailDomainRemoveRefusesTheAppsOwnFromDomain: mail.Enable rejects a |
| +// From that is not at a granted domain, so removing the From's own domain |
| +// would break sending at the app's next re-enable rather than at the |
| +// command that caused it. 409, naming the From and the verb that moves it. |
| +func TestAPIEmailDomainRemoveRefusesTheAppsOwnFromDomain(t *testing.T) { |
| + fx, _, _, token := emailArmed(t, "code-email-domain-rm-from") |
| + app, domain := "titogo", "mail.example.com" |
| + declareEmail(t, fx, token, app, "hello@"+domain) |
| + ensureEmailDomain(t, fx, token, app, domain) |
| + |
| + status, out := removeEmailDomain(t, fx, token, app, domain) |
| + if status != http.StatusConflict { |
| + t.Fatalf("remove of the From's own domain = %d %v, want 409", status, out) |
| + } |
| + msg, _ := out["error"].(string) |
| + if !strings.Contains(msg, "hello@"+domain) { |
| + t.Fatalf("refusal does not name the From address: %q", msg) |
| + } |
| + if !strings.Contains(msg, "--from") { |
| + t.Fatalf("refusal does not name the verb that moves the From: %q", msg) |
| + } |
| + claims, err := mail.ListDomainClaims(context.Background(), fx.srv.cfg.Releases) |
| + if err != nil || len(claims) != 1 { |
| + t.Fatalf("a refused remove still released the claim: %+v (err %v)", claims, err) |
| + } |
| + if rec, _, err := mail.GetDomain(context.Background(), fx.srv.cfg.Releases, fx.sqid, domain); err != nil || rec == nil { |
| + t.Fatalf("a refused remove still deleted the domain record: %+v (err %v)", rec, err) |
| + } |
| +} |
| + |
| +// TestAPIEmailDomainRemoveIs404ForADomainTheAppDoesNotUse: naming a domain |
| +// this app does not send from is a 404, never a quiet 200. The same 404 |
| +// covers a domain claimed by ANOTHER account, deliberately in the same |
| +// words — see mail.ErrDomainNotReferenced. |
| +func TestAPIEmailDomainRemoveIs404ForADomainTheAppDoesNotUse(t *testing.T) { |
| + fx, _, _, token := emailArmed(t, "code-email-domain-rm-404") |
| + app := "titogo" |
| + declareEmail(t, fx, token, app, "hello@elsewhere.example.com") |
| + |
| + status, out := removeEmailDomain(t, fx, token, app, "never-added.example.com") |
| + if status != http.StatusNotFound { |
| + t.Fatalf("remove of an unused domain = %d %v, want 404", status, out) |
| + } |
| + if msg, _ := out["error"].(string); !strings.Contains(msg, "carlos email status") { |
| + t.Fatalf("404 does not point at the verb that lists the app's domains: %q", msg) |
| + } |
| +} |
| + |
| +// TestAPIEmailDomainRemoveReportsACustomDomainsOrphanedRecords: a custom |
| +// domain's records live in the customer's zone and are the customer's to |
| +// remove — mail.Teardown's rule, kept. The door deletes none of them and |
| +// hands the whole set back so the member can see what just went stale. |
| +func TestAPIEmailDomainRemoveReportsACustomDomainsOrphanedRecords(t *testing.T) { |
| + fx, _, writer, token := emailArmed(t, "code-email-domain-rm-custom-dns") |
| + app, domain := "titogo", "mail.example.com" |
| + declareEmail(t, fx, token, app, "hello@elsewhere.example.com") |
| + ensureEmailDomain(t, fx, token, app, domain) |
| + if len(writer.Records) != 0 { |
| + t.Fatalf("fixture broken: CARLOS published a custom domain's records: %v", writer.Records) |
| + } |
| + |
| + status, out := removeEmailDomain(t, fx, token, app, domain) |
| + if status != http.StatusOK { |
| + t.Fatalf("remove = %d %v, want 200", status, out) |
| + } |
| + orphaned, ok := out["orphaned"].([]any) |
| + if !ok || len(orphaned) != 6 { |
| + t.Fatalf("orphaned = %v, want the whole six-record set back", out["orphaned"]) |
| + } |
| +} |
| + |
| +// TestAPIEmailDomainRemoveDeletesAPlatformDomainsRecords: the other half of |
| +// the same rule — records CARLOS published, CARLOS removes, and reports |
| +// nothing orphaned because nothing is. |
| +func TestAPIEmailDomainRemoveDeletesAPlatformDomainsRecords(t *testing.T) { |
| + fx, _, writer, token := emailArmed(t, "code-email-domain-rm-platform-dns") |
| + app, domain := "titogo", "titogo.bab.oncarlos.com" |
| + declareEmail(t, fx, token, app, "hello@elsewhere.example.com") |
| + ensureEmailDomain(t, fx, token, app, domain) |
| + if len(writer.Records) == 0 { |
| + t.Fatal("fixture broken: nothing was published for a platform domain") |
| + } |
| + |
| + status, out := removeEmailDomain(t, fx, token, app, domain) |
| + if status != http.StatusOK { |
| + t.Fatalf("remove = %d %v, want 200", status, out) |
| + } |
| + if len(writer.Records) != 0 { |
| + t.Fatalf("a platform domain's published records survived: %v", writer.Records) |
| + } |
| + if orphaned, _ := out["orphaned"].([]any); len(orphaned) != 0 { |
| + t.Fatalf("orphaned = %v, want empty for a platform domain", orphaned) |
| + } |
| +} |
| + |
| // ---- enable and the F2 grant wiring ---------------------------------------- |
| // TestEnableAppliesTheTierGrantBeforeProvisioning: F2's ruling, pinned. The |
diff --git a/internal/console/server.go b/internal/console/server.go| index ed4352157..b5d5a2706 100644 |
| --- a/internal/console/server.go |
| +++ b/internal/console/server.go |
| @@ -1127,7 +1127,7 @@ func NewServer(cfg Config, st *Store) (*Server, error) { |
| // CLI identity: the device-code login `carlos auth login` drives, the |
| // approval page, and the tokens an owner can see and kill (authcli.go). |
| s.registerCLIRoutes() |
| - // The email API's nine doors (apimail.go, tenant-email task 17) — |
| + // The email API's ten doors (apimail.go, tenant-email task 17) — |
| // registered separately from registerCLIRoutes because this task's file |
| // scope is server.go and apimail.go only. |
| s.registerEmailRoutes() |
diff --git a/internal/mail/domainclaim.go b/internal/mail/domainclaim.go| index 53f2f90ca..90cc59987 100644 |
| --- a/internal/mail/domainclaim.go |
| +++ b/internal/mail/domainclaim.go |
| @@ -147,14 +147,24 @@ func claimDomain(ctx context.Context, s release.Store, domain, acct, app string, |
| // A claim already absent is a quiet no-op: release, like the claim itself, |
| // is idempotent. |
| // |
| -// Callers: ONLY Teardown, at two points (I1, review follow-up) — right |
| -// after the identity/DNS for the LAST referring app is torn down (the |
| -// shared resource is gone by then, so releasing first is safe), and again |
| -// in Teardown's orphan-claim reconciliation pass for a claim this account |
| -// holds with NO backing record at all (reachable when EnsureDomain fails |
| -// after claiming but before ever writing a record). Releasing outside |
| -// either of those two conditions would let a stranger claim a domain still |
| -// backed by a live record. |
| +// Callers: only ever the two CONDITIONS below, never a third. Releasing |
| +// outside them would let a stranger claim a domain still backed by a live |
| +// record. |
| +// |
| +// - right after the identity/DNS for the LAST referring app is torn down |
| +// (the shared resource is gone by then, so releasing first is safe) — |
| +// detachDomainApp, shared by Teardown and RemoveDomain; |
| +// - for a claim this account holds with NO backing record at all |
| +// (reachable when EnsureDomain fails after claiming but before ever |
| +// writing a record) — Teardown's orphan-claim reconciliation pass, and |
| +// RemoveDomain's removeOrphanedClaim for the same state named one |
| +// domain at a time. |
| +// |
| +// It was ONLY Teardown until `carlos email domains rm` (RemoveDomain) was |
| +// built, and that was the defect: Teardown is reached solely by |
| +// PERMANENTLY DELETING an app, so a domain claimed by the wrong app stayed |
| +// unavailable to every other account — its rightful owner included — for |
| +// the life of the deployment. |
| func releaseDomainClaim(ctx context.Context, s release.Store, domain, acct string) error { |
| cur, err := getDomainClaim(ctx, s, domain) |
| if err != nil { |
| @@ -179,9 +189,11 @@ func releaseDomainClaim(ctx context.Context, s release.Store, domain, acct strin |
| // discipline domains.ListClaims applies — one bad claim must never take |
| // the whole listing down. |
| // |
| -// The operator-facing CLI/console verb this enables is filed as owed, not |
| -// built here — this function is the read path a future door calls, not a |
| -// door itself. |
| +// The RELEASE half of the door this was written for now exists — `carlos |
| +// email domains rm` (RemoveDomain), which frees a claim without deleting |
| +// the app that holds it. A READ verb, listing claims an operator does not |
| +// hold, is still owed: this function remains the read path such a door |
| +// would call, and nothing but Teardown's own sweep calls it yet. |
| func ListDomainClaims(ctx context.Context, s release.Store) ([]DomainClaim, error) { |
| keys, err := s.List(ctx, domainClaimsPrefix) |
| if err != nil { |
diff --git a/internal/mail/walk.go b/internal/mail/walk.go| index 60be16b96..f721f61da 100644 |
| --- a/internal/mail/walk.go |
| +++ b/internal/mail/walk.go |
| @@ -38,6 +38,18 @@ var ( |
| // is not a cross-account leak, it is telling the caller which name IS |
| // theirs. |
| ErrNotOwnPlatformName = errors.New("mail: not this account's platform name") |
| + // ErrDomainNotReferenced is RemoveDomain's refusal of a domain this app |
| + // does not use — including one whose global claim belongs to somebody |
| + // else, which reads identically on purpose so the door cannot be used |
| + // as an oracle for who holds a domain the caller does not. |
| + ErrDomainNotReferenced = errors.New("mail: app does not use this domain") |
| + // ErrDomainIsFrom is RemoveDomain's refusal of the domain the app is |
| + // currently sending FROM. Enable rejects a From that is not at a |
| + // granted domain, so allowing this would break sending at the app's |
| + // next re-enable rather than at the command that caused it — the |
| + // failure moves from a refusal a person can read to silence a person |
| + // has to debug. |
| + ErrDomainIsFrom = errors.New("mail: domain is the app's From address") |
| ) |
| // Deployment is the per-deployment configuration the walks need. |
| @@ -1180,6 +1192,140 @@ func RevokeStandalone(ctx context.Context, s release.Store, p Provisioner, acct, |
| return err |
| } |
| +// domainDetach says what detachDomainApp did to one domain record. |
| +type domainDetach int |
| + |
| +const ( |
| + // detachNone decided nothing. It is only ever paired with a non-nil |
| + // error, so a caller that checks the error first never sees it. |
| + detachNone domainDetach = iota |
| + // detachNotAReferrer: this app never referred to the domain, or had |
| + // already stopped. Nothing was written and nothing was destroyed. |
| + detachNotAReferrer |
| + // detachKept: other apps still refer to the domain, so the record was |
| + // rewritten without this app and the shared estate was left alone. |
| + detachKept |
| + // detachTornDown: this app was the LAST referrer, so the published DNS |
| + // (platform domains only), the SES identity, the global claim and the |
| + // account's own domain record are all gone. |
| + detachTornDown |
| +) |
| + |
| +// detachDomainApp stops one app referring to one domain, and — only when |
| +// that leaves NO app referring to it — dismantles the domain. |
| +// |
| +// It is the body of Teardown's per-domain loop, lifted out so `carlos email |
| +// domains rm` (RemoveDomain) executes the identical sequence rather than a |
| +// second copy of it. The order below is load-bearing in three separate |
| +// places, each documented where it happens; two copies of it would be two |
| +// chances to get one of them subtly wrong. |
| +// |
| +// cur must be a FRESH read (with its etag), not a ListDomains snapshot — |
| +// see Teardown's own comment at the call site for the two reasons. |
| +// |
| +// The orphaned record set comes back rather than being deleted for a CUSTOM |
| +// domain: those records live in the customer's zone and are the customer's |
| +// to remove. Returning them is what lets a door tell an operator exactly |
| +// which names are now stale. |
| +func detachDomainApp(ctx context.Context, s release.Store, p Provisioner, w dnsrec.Writer, |
| + d Deployment, acct, app string, cur *DomainRecord, curEtag string) (domainDetach, []dnsrec.Record, error) { |
| + |
| + if !cur.RemoveApp(app) { |
| + return detachNotAReferrer, nil, nil |
| + } |
| + if len(cur.Apps) > 0 { |
| + // A sibling app still sends from this domain. Its identity, its |
| + // DKIM tokens, its DNS and the global claim ALL stay exactly as |
| + // they are — getting this backwards revokes a sibling app's |
| + // verified domain silently, with the customer's own DNS still |
| + // pointing at an identity that no longer exists. |
| + if _, err := PutDomain(ctx, s, acct, cur, curEtag); err != nil { |
| + return detachNone, nil, err |
| + } |
| + return detachKept, nil, nil |
| + } |
| + // Last referrer: remove the identity and, for a platform domain, the |
| + // records CARLOS published. A custom domain's records are the |
| + // customer's to remove, so they come back to the caller instead. |
| + var orphaned []dnsrec.Record |
| + for region, st := range cur.Regions { |
| + records := RecordSet(cur.Domain, region, st.DKIMTokens, d.DMARCRua) |
| + // Same w != nil guard as EnsureDomain's publish branch, for the |
| + // identical reason: a caller with no DNSWriter configured must not |
| + // panic on a nil interface here either. A platform domain with no |
| + // writer degrades to "orphaned" exactly like a custom one — the |
| + // records exist somewhere this process cannot reach, which is the |
| + // honest thing to report. |
| + if cur.Kind == KindPlatform && w != nil { |
| + for _, r := range records { |
| + if err := w.DeleteRecord(ctx, r); err != nil { |
| + return detachNone, nil, err |
| + } |
| + } |
| + } else { |
| + orphaned = append(orphaned, records...) |
| + } |
| + // SES refuses to delete an identity while a tenant association |
| + // exists, so the association is attempted first. The account's |
| + // configuration set is NOT disassociated: it is per account and |
| + // outlives any one app. |
| + // |
| + // A failed disassociate is logged and NOT a hard error (I2, review |
| + // follow-up, reversed from that task's first cut). The control-plane |
| + // role needs ses:DeleteTenantResourceAssociation (spec §4.2) and |
| + // that grant does not live in this repo; if it is missing, |
| + // AccessDenied is neither AlreadyExists nor NotFound, so a |
| + // `return err` here would fail EVERY release where this app is the |
| + // last referrer of a domain, and — worse — leave releaseDomainClaim |
| + // and s.Delete below unreached, which strands the domain's global |
| + // claim: nobody, including this domain's own owner, could ever claim |
| + // it again until the grant landed. That is the exact permanent |
| + // stranding `carlos email domains rm` exists to end, so this door |
| + // must not reintroduce it. DeleteIdentity is the real protection SES |
| + // itself provides (confirmed against the live API, spec §4.1: it |
| + // refuses to delete an identity that still has a tenant |
| + // association), so it stays a hard failure below and does the job |
| + // this disassociate call cannot be trusted to gate. |
| + if st.IdentityARN != "" { |
| + if err := p.DisassociateTenantResource(ctx, TenantName(acct), st.IdentityARN); err != nil { |
| + log.Printf("mail: disassociating tenant resource for %s/%s: %v", acct, cur.Domain, err) |
| + } |
| + } |
| + if err := p.DeleteIdentity(ctx, region, cur.Domain); err != nil { |
| + return detachNone, nil, err |
| + } |
| + } |
| + // The global claim (Task 17a, spec §4a) releases HERE, in the same |
| + // branch as deleting the account's own domain record — 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 the `len(cur.Apps) > 0` branch above guards. |
| + // |
| + // RELEASE BEFORE DELETE (I1, review follow-up), reversed from this |
| + // task's first cut. The shared resource the claim protects is the SES |
| + // identity, and by this point in the branch it is ALREADY destroyed |
| + // (DeleteIdentity, above) — so releasing the claim before the record is |
| + // gone hands nothing live to a winner. The reverse order |
| + // (delete-then-release, the original cut) has a real defect: if the |
| + // process dies between the two, the record is gone, so |
| + // ListDomains(acct) on any later Teardown run never lists this domain |
| + // again — the release call becomes permanently unreachable, and the |
| + // claim is stranded forever, un-reclaimable even by its own owner's |
| + // account under a different app. Releasing FIRST means a crash before |
| + // the record delete just repeats a no-op release (idempotent) and a |
| + // retriable s.Delete on the next run — self-healing again, matching |
| + // every other step here. |
| + if err := releaseDomainClaim(ctx, s, cur.Domain, acct); err != nil { |
| + return detachNone, nil, err |
| + } |
| + if err := s.Delete(ctx, DomainKey(acct, cur.Domain)); err != nil { |
| + return detachNone, nil, err |
| + } |
| + return detachTornDown, orphaned, nil |
| +} |
| + |
| // Teardown dismantles an app's email estate: credentials, principals, and |
| // its claim on every domain it referenced. |
| // |
| @@ -1254,89 +1400,13 @@ func Teardown(ctx context.Context, s release.Store, p Provisioner, w dnsrec.Writ |
| if cur == nil { |
| continue // gone since the list; nothing to do |
| } |
| - if !cur.RemoveApp(app) { |
| - continue // this app was never a referrer, or already removed |
| - } |
| - if len(cur.Apps) > 0 { |
| - if _, err := PutDomain(ctx, s, acct, cur, curEtag); err != nil { |
| - return err |
| - } |
| - continue |
| - } |
| - // Last referrer: remove the identity and, for a platform domain, |
| - // the records CARLOS published. A custom domain's records are the |
| - // customer's to remove. |
| - for region, st := range cur.Regions { |
| - // Same w != nil guard as EnsureDomain's publish branch, for |
| - // the identical reason: a caller with no DNSWriter configured |
| - // must not panic on a nil interface here either, even though |
| - // nothing in this codebase wires Teardown to such a caller |
| - // yet — the guard costs nothing and keeps the two record- |
| - // writing sites in this file from drifting onto different |
| - // nil-safety assumptions. |
| - if cur.Kind == KindPlatform && w != nil { |
| - for _, r := range RecordSet(cur.Domain, region, st.DKIMTokens, d.DMARCRua) { |
| - if err := w.DeleteRecord(ctx, r); err != nil { |
| - return err |
| - } |
| - } |
| - } |
| - // SES refuses to delete an identity while a tenant association |
| - // exists, so the association is attempted first. The account's |
| - // configuration set is NOT disassociated: it is per account and |
| - // outlives any one app. |
| - // |
| - // A failed disassociate is logged and NOT a hard error (I2, |
| - // review follow-up, reversed from this task's first cut). The |
| - // control-plane role needs ses:DeleteTenantResourceAssociation |
| - // (spec §4.2) and that grant does not live in this repo; if it |
| - // is missing, AccessDenied is neither AlreadyExists nor |
| - // NotFound, so a `return err` here would fail EVERY teardown |
| - // where this app is the last referrer of a domain, and — worse |
| - // — leave releaseDomainClaim and s.Delete below unreached, which |
| - // strands the domain's global claim: nobody, including this |
| - // domain's own owner, could ever claim it again until the grant |
| - // landed. DeleteIdentity is the real protection SES itself |
| - // provides (confirmed against the live API, spec §4.1: it |
| - // refuses to delete an identity that still has a tenant |
| - // association), so it stays a hard failure below and does the |
| - // job this disassociate call cannot be trusted to gate. |
| - if st.IdentityARN != "" { |
| - if err := p.DisassociateTenantResource(ctx, TenantName(acct), st.IdentityARN); err != nil { |
| - log.Printf("mail: teardown: disassociating tenant resource for %s/%s: %v", acct, cur.Domain, err) |
| - } |
| - } |
| - if err := p.DeleteIdentity(ctx, region, cur.Domain); err != nil { |
| - return err |
| - } |
| - } |
| - // The global claim (Task 17a, spec §4a) releases HERE, in the same |
| - // branch as deleting the account's own domain record — 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 above. |
| - // |
| - // RELEASE BEFORE DELETE (I1, review follow-up), reversed from this |
| - // task's first cut. The shared resource the claim protects is the |
| - // SES identity, and by this point in the branch it is ALREADY |
| - // destroyed (DeleteIdentity, above) — so releasing the claim before |
| - // the record is gone hands nothing live to a winner. The reverse |
| - // order (delete-then-release, the original cut) has a real defect: |
| - // if the process dies between the two, the record is gone, so |
| - // ListDomains(acct) on any later Teardown run never lists this |
| - // domain again — the release call becomes permanently unreachable, |
| - // and the claim is stranded forever, un-reclaimable even by its own |
| - // owner's account under a different app. Releasing FIRST means a |
| - // crash before the record delete just repeats a no-op release |
| - // (idempotent) and a retriable s.Delete on the next run — self- |
| - // healing again, matching every other step in this loop. |
| - if err := releaseDomainClaim(ctx, s, cur.Domain, acct); err != nil { |
| - return err |
| - } |
| - if err := s.Delete(ctx, DomainKey(acct, cur.Domain)); err != nil { |
| + // Everything from here — the sibling check, the identity and DNS, |
| + // the claim, the record — is detachDomainApp's, shared verbatim |
| + // with RemoveDomain so the two release paths cannot drift on an |
| + // ordering that is load-bearing and subtle. A domain this app never |
| + // referred to is that function's own no-op, which is what the |
| + // `continue` used to be. |
| + if _, _, err := detachDomainApp(ctx, s, p, w, d, acct, app, cur, curEtag); err != nil { |
| return err |
| } |
| } |
| @@ -1411,3 +1481,185 @@ func Teardown(ctx context.Context, s release.Store, p Provisioner, w dnsrec.Writ |
| } |
| return nil |
| } |
| + |
| +// DomainRemoval is what RemoveDomain did, so a door can describe it without |
| +// re-reading anything the walk has just deleted. |
| +type DomainRemoval struct { |
| + Domain string |
| + Kind string |
| + // TornDown is true when this app was the LAST referrer: the SES |
| + // identity, the account's domain record and the global claim are all |
| + // gone, and the domain is free for any account to claim again. |
| + TornDown bool |
| + // Remaining is the apps that still refer to the domain — non-empty |
| + // exactly when TornDown is false, and the reason nothing else was |
| + // touched. |
| + Remaining []string |
| + // Orphaned is the record set nobody deleted: a custom domain's DNS |
| + // lives in the customer's zone and is the customer's to remove |
| + // (detachDomainApp's rule, which this inherits from Teardown). Empty |
| + // for a platform domain, whose records CARLOS published and has now |
| + // deleted itself. |
| + // |
| + // Only the DKIM CNAMEs in here are token-derived and therefore dead: |
| + // the MX, SPF and DMARC records RecordSet builds are deterministic |
| + // functions of the domain and region, so they stay valid if the domain |
| + // is ever re-created. |
| + Orphaned []dnsrec.Record |
| + // ClaimOnly is the orphaned-claim repair: there was no domain record at |
| + // all — only a global claim stranded by an EnsureDomain that failed |
| + // after claimDomain and before PutDomain — and releasing it is the |
| + // whole of what happened. |
| + ClaimOnly bool |
| +} |
| + |
| +// RemoveDomain is the release door the claim always needed: it stops app |
| +// referring to domain and, when that was the last reference, dismantles the |
| +// domain and releases its global claim. |
| +// |
| +// Before this existed, Teardown — the PERMANENT deletion of a whole app — |
| +// was the only thing that could ever release a claim, so a domain claimed |
| +// by the wrong app was unreachable to every other account, including the |
| +// one that actually controls its DNS, for the life of the deployment. |
| +// |
| +// It shares detachDomainApp with Teardown rather than repeating it, so the |
| +// safety properties are the same objects, not the same intentions: |
| +// |
| +// - a domain a sibling app still refers to keeps its identity, its DNS |
| +// and its claim; only the referrer set shrinks; |
| +// - the claim is released only after DeleteIdentity, and only before the |
| +// record delete (see detachDomainApp for why each of those is the way |
| +// round it is); |
| +// - a custom domain's DNS is never deleted, it is reported back. |
| +// |
| +// Two refusals are its own, and neither is Teardown's business: |
| +// |
| +// - ErrDomainIsFrom, the domain the app currently sends FROM. Enable |
| +// refuses a From that is not at a granted domain, so removing it would |
| +// turn a working app into one that fails at its next re-enable — a |
| +// breakage discovered at runtime instead of here. Move the From first. |
| +// - ErrDomainNotReferenced, a domain this app does not use. Teardown |
| +// walks an app's whole estate and a non-referrer is simply skipped; |
| +// here the operator NAMED this domain, so silence would be a lie. |
| +// |
| +// OWNER-GATING IS UNCHANGED. Nothing here reads, probes or releases another |
| +// account's claim: releaseDomainClaim refuses a claim held by anyone else |
| +// as a quiet no-op, the orphan branch below compares the claim's account |
| +// itself, and no error message ever names a holder. |
| +// |
| +// A non-nil *DomainRemoval WITH a non-nil error means one specific thing: |
| +// the removal itself landed, and only the departing app's IAM policy shrink |
| +// (RefreshPolicies) failed. Re-running this verb would then answer |
| +// ErrDomainNotReferenced, correctly — the owed repair is `carlos email |
| +// enable`, which rewrites that policy wholesale on every call. |
| +func RemoveDomain(ctx context.Context, s release.Store, p Provisioner, w dnsrec.Writer, |
| + d Deployment, acct, app, domain string) (*DomainRemoval, error) { |
| + |
| + if !release.ValidAccount(acct) { |
| + return nil, fmt.Errorf("mail: invalid account %q", acct) |
| + } |
| + if !release.ValidApp(app) { |
| + return nil, fmt.Errorf("mail: invalid app %q", app) |
| + } |
| + if !ValidDomain(domain) { |
| + return nil, fmt.Errorf("mail: invalid domain %q", domain) |
| + } |
| + |
| + // The From check runs FIRST, before anything is read about the domain |
| + // and long before anything is written: a refusal must cost the caller |
| + // nothing and leave no half-step behind. It looks at THIS app's From |
| + // only — a sibling app keeps its own referral, so its From is still at |
| + // a domain it is granted. |
| + rec, _, err := Get(ctx, s, acct, app) |
| + if err != nil { |
| + return nil, err |
| + } |
| + if rec != nil && rec.Declared != nil { |
| + if at := strings.LastIndex(rec.Declared.From, "@"); at >= 0 && rec.Declared.From[at+1:] == domain { |
| + return nil, fmt.Errorf("mail: %s still sends as %s: %w", app, rec.Declared.From, ErrDomainIsFrom) |
| + } |
| + } |
| + |
| + cur, curEtag, err := GetDomain(ctx, s, acct, domain) |
| + if err != nil { |
| + return nil, err |
| + } |
| + if cur == nil { |
| + return removeOrphanedClaim(ctx, s, p, d, acct, domain) |
| + } |
| + |
| + res, orphaned, err := detachDomainApp(ctx, s, p, w, d, acct, app, cur, curEtag) |
| + if err != nil { |
| + return nil, err |
| + } |
| + if res == detachNotAReferrer { |
| + return nil, fmt.Errorf("mail: %s does not send from %s: %w", app, domain, ErrDomainNotReferenced) |
| + } |
| + out := &DomainRemoval{Domain: domain, Kind: cur.Kind, TornDown: res == detachTornDown, Orphaned: orphaned} |
| + if res == detachKept { |
| + out.Remaining = append([]string(nil), cur.Apps...) |
| + } |
| + // SHRINK THE DEPARTING APP'S POLICY, the mirror of EnsureDomain's own |
| + // RefreshPolicies call. EnsureUser writes a principal's policy |
| + // wholesale from the CURRENT granted domain set, and nothing else |
| + // recomputes it when a domain LEAVES that set — so without this, an app |
| + // that drops a domain a sibling still keeps alive goes on holding |
| + // ses:SendRawEmail at a live identity ARN it no longer has any claim |
| + // to, indefinitely. That is a real over-grant, not a stale string. |
| + if err := RefreshPolicies(ctx, s, p, acct, []string{app}); err != nil { |
| + return out, err |
| + } |
| + return out, nil |
| +} |
| + |
| +// removeOrphanedClaim is RemoveDomain's branch for a claim with NO backing |
| +// domain record — the state EnsureDomain leaves behind when it fails after |
| +// claimDomain and before PutDomain (an SES throttle, a tenant limit, a bad |
| +// region). Before this, only Teardown could sweep one, and only for the app |
| +// that created it; an operator who hit it on an app they did not want to |
| +// delete had no recourse at all. |
| +// |
| +// The app in the path is a MEMBERSHIP GATE here, not an ownership filter — |
| +// the same reading handleAPIEmailDomainVerify documents, and appropriate |
| +// for the same reason: the claim is account-scoped, not app-scoped, and |
| +// unlike Teardown's blind sweep the operator has NAMED this exact domain, |
| +// so there is no "a sibling app is merely about to retry" ambiguity for the |
| +// App attribution to resolve. |
| +// |
| +// DESTROY THE IDENTITY BEFORE RELEASING (I1 item 2), exactly as Teardown's |
| +// own sweep does: if the crash that orphaned this claim landed after |
| +// EnsureIdentity but before PutDomain, a live SES identity exists that no |
| +// record names, and releasing without deleting it hands a stranger that |
| +// identity's ARN and its already-minted DKIM tokens the moment they claim |
| +// the domain. EnsureDomain only ever mints in d.Region, so that is the only |
| +// region to check, and DeleteIdentity is idempotent. |
| +// |
| +// NO DISASSOCIATE HERE, unlike detachDomainApp's region loop, and that is |
| +// not an omission. EnsureDomain associates the identity with the account's |
| +// tenant only AFTER PutDomain — deliberately, so that a claim with no |
| +// record can never have an association behind it (see the ordering comment |
| +// at that call). An identity reachable from this branch is therefore always |
| +// unassociated, and DeleteIdentity takes it. |
| +func removeOrphanedClaim(ctx context.Context, s release.Store, p Provisioner, |
| + d Deployment, acct, domain string) (*DomainRemoval, error) { |
| + |
| + claim, err := getDomainClaim(ctx, s, domain) |
| + if err != nil { |
| + return nil, err |
| + } |
| + // A claim held by ANOTHER account reads exactly like no claim at all — |
| + // the same refusal, the same words. Anything else would make this door |
| + // an oracle for who holds a domain the caller does not. |
| + if claim == nil || claim.Account != acct { |
| + return nil, fmt.Errorf("mail: no sending domain %s for this account: %w", domain, ErrDomainNotReferenced) |
| + } |
| + if err := p.DeleteIdentity(ctx, d.Region, domain); err != nil { |
| + return nil, err |
| + } |
| + if err := releaseDomainClaim(ctx, s, domain, acct); err != nil { |
| + return nil, err |
| + } |
| + return &DomainRemoval{ |
| + Domain: domain, Kind: DomainKindFor(d, domain), TornDown: true, ClaimOnly: true, |
| + }, nil |
| +} |
diff --git a/internal/mail/walk_domainremove_test.go b/internal/mail/walk_domainremove_test.go| new file mode 100644 |
| index 000000000..3f88795b3 |
| --- /dev/null |
| +++ b/internal/mail/walk_domainremove_test.go |
| @@ -0,0 +1,442 @@ |
| +// RemoveDomain: `carlos email domains rm`, the release door the global |
| +// claim never had. |
| +// |
| +// The claim (domainclaim.go) is a deliberately global, create-only object |
| +// so two accounts cannot both hold one sending domain. Right — and until |
| +// this walk existed, the ONLY thing that ever released one was Teardown, |
| +// reachable only by permanently deleting the app that held it. A real |
| +// deployment hit exactly that: one app held a claim on a domain a second |
| +// app in a second account served and wanted to send from, and there was no |
| +// command in the product that could free it. |
| +// |
| +// What these tests pin is that the door does not reopen any of the holes |
| +// the claim closes: a domain a sibling app still uses keeps everything, the |
| +// release happens in an order a crash cannot strand, a custom domain's DNS |
| +// is never deleted, and no path here can read, disturb or probe another |
| +// account's claim. |
| +package mail |
| + |
| +import ( |
| + "context" |
| + "errors" |
| + "strings" |
| + "testing" |
| + "time" |
| + |
| + "github.com/carlosframework/platform/internal/dnsrec" |
| + "github.com/carlosframework/platform/internal/release" |
| +) |
| + |
| +// removeFixture is one account, one shared CUSTOM domain, two referring |
| +// apps — the shape every interesting question about this verb is asked in. |
| +// Custom rather than platform because the DNS rule (the customer's records |
| +// are the customer's to remove) only has teeth on a custom domain. |
| +func removeFixture(t *testing.T) (context.Context, release.Store, *MemProvisioner, *dnsrec.MemWriter, time.Time) { |
| + t.Helper() |
| + ctx := context.Background() |
| + s, p, w := release.NewDirStore(t.TempDir()), NewMemProvisioner("shared-aws-account"), dnsrec.NewMemWriter() |
| + now := time.Date(2026, 9, 23, 12, 0, 0, 0, time.UTC) |
| + for _, app := range []string{"app1", "app2"} { |
| + if _, _, err := EnsureDomain(ctx, s, p, w, testDeployment(), "aaa", app, "shared.example", now); err != nil { |
| + t.Fatalf("EnsureDomain %s: %v", app, err) |
| + } |
| + } |
| + return ctx, s, p, w, now |
| +} |
| + |
| +// A sibling app still referring to the domain means NOTHING is released: |
| +// not the identity, not the DNS, and above all not the claim. A stranger |
| +// must still be refused — this is the exact failure Teardown's own comment |
| +// warns about ("would let a stranger claim a domain a sibling app is still |
| +// sending from"), and a naive "just delete the claim object" reopens it. |
| +func TestRemoveDomainKeepsADomainASiblingStillUses(t *testing.T) { |
| + ctx, s, p, w, now := removeFixture(t) |
| + |
| + out, err := RemoveDomain(ctx, s, p, w, testDeployment(), "aaa", "app1", "shared.example") |
| + if err != nil { |
| + t.Fatalf("RemoveDomain: %v", err) |
| + } |
| + if out.TornDown { |
| + t.Fatalf("torn down while app2 still refers to it: %+v", out) |
| + } |
| + if len(out.Remaining) != 1 || out.Remaining[0] != "app2" { |
| + t.Fatalf("remaining = %v, want [app2]", out.Remaining) |
| + } |
| + rec, _, err := GetDomain(ctx, s, "aaa", "shared.example") |
| + if err != nil { |
| + t.Fatalf("GetDomain: %v", err) |
| + } |
| + if rec == nil || len(rec.Apps) != 1 || rec.Apps[0] != "app2" { |
| + t.Fatalf("record = %+v, want one referrer app2", rec) |
| + } |
| + if _, ok := p.Identities[identityKey("eu-west-1", "shared.example")]; !ok { |
| + t.Fatal("removing one referrer destroyed the identity the sibling still sends with") |
| + } |
| + if claim, err := getDomainClaim(ctx, s, "shared.example"); err != nil || claim == nil { |
| + t.Fatalf("claim released while a sibling still uses the domain: claim=%+v err=%v", claim, err) |
| + } |
| + stranger := NewMemProvisioner("shared-aws-account") |
| + if _, _, err := EnsureDomain(ctx, s, stranger, w, testDeployment(), "bbb", "evilapp", "shared.example", now); err == nil { |
| + t.Fatal("a stranger claimed a domain a sibling app is still sending from") |
| + } |
| +} |
| + |
| +// The last referrer leaving is what releases the claim — and the proof is |
| +// that a DIFFERENT account can then complete the whole ensure walk on the |
| +// same domain, which is the outcome the real deployment could not reach. |
| +func TestRemoveDomainLastReferrerFreesTheDomainForAnotherAccount(t *testing.T) { |
| + ctx, s, p, w, now := removeFixture(t) |
| + |
| + if _, err := RemoveDomain(ctx, s, p, w, testDeployment(), "aaa", "app1", "shared.example"); err != nil { |
| + t.Fatalf("remove app1: %v", err) |
| + } |
| + out, err := RemoveDomain(ctx, s, p, w, testDeployment(), "aaa", "app2", "shared.example") |
| + if err != nil { |
| + t.Fatalf("remove app2: %v", err) |
| + } |
| + if !out.TornDown || len(out.Remaining) != 0 { |
| + t.Fatalf("last referrer left without a teardown: %+v", out) |
| + } |
| + if _, ok := p.Identities[identityKey("eu-west-1", "shared.example")]; ok { |
| + t.Fatal("the SES identity survived the last referrer leaving") |
| + } |
| + if rec, _, err := GetDomain(ctx, s, "aaa", "shared.example"); err != nil || rec != nil { |
| + t.Fatalf("the domain record survived: %+v (err %v)", rec, err) |
| + } |
| + if claim, err := getDomainClaim(ctx, s, "shared.example"); err != nil || claim != nil { |
| + t.Fatalf("the claim survived the last referrer leaving: %+v (err %v)", claim, err) |
| + } |
| + |
| + // The whole point: another account can now take the domain. |
| + other := NewMemProvisioner("shared-aws-account") |
| + rec, _, err := EnsureDomain(ctx, s, other, w, testDeployment(), "bbb", "jam", "shared.example", now) |
| + if err != nil { |
| + t.Fatalf("a second account still cannot claim the released domain: %v", err) |
| + } |
| + if rec == nil || len(other.Identities) != 1 { |
| + t.Fatal("the second account did not actually get its own identity") |
| + } |
| +} |
| + |
| +// Removing the domain the app's own From sits at is refused. Enable rejects |
| +// a From that is not at a granted domain, so allowing it would move the |
| +// breakage from this command — where a person can read it — to the app's |
| +// next re-enable, where it presents as "email silently stopped working". |
| +func TestRemoveDomainRefusesTheDomainTheAppSendsFrom(t *testing.T) { |
| + ctx, s, p, _, _ := enableFixture(t) |
| + domain := "harbourmail.bag.oncarlos.com" |
| + |
| + _, err := RemoveDomain(ctx, s, p, dnsrec.NewMemWriter(), testDeployment(), "bag", "harbourmail", domain) |
| + if !errors.Is(err, ErrDomainIsFrom) { |
| + t.Fatalf("RemoveDomain err = %v, want ErrDomainIsFrom", err) |
| + } |
| + // A refusal must cost nothing and leave nothing half-done. |
| + if rec, _, err := GetDomain(ctx, s, "bag", domain); err != nil || rec == nil { |
| + t.Fatalf("the refusal still removed the record: %+v (err %v)", rec, err) |
| + } |
| + if claim, err := getDomainClaim(ctx, s, domain); err != nil || claim == nil { |
| + t.Fatalf("the refusal still released the claim: %+v (err %v)", claim, err) |
| + } |
| + if _, ok := p.Identities[identityKey("eu-west-1", domain)]; !ok { |
| + t.Fatal("the refusal still destroyed the identity") |
| + } |
| + // The error names the From, so a member knows what to move. |
| + if !strings.Contains(err.Error(), "noreply@"+domain) { |
| + t.Fatalf("refusal does not name the From address: %v", err) |
| + } |
| +} |
| + |
| +// A CUSTOM domain's records live in the customer's zone: CARLOS never |
| +// published them and must not delete them. They come back instead, so a |
| +// door can tell the operator exactly which names just went stale. |
| +func TestRemoveDomainLeavesACustomDomainsDNSAloneAndNamesIt(t *testing.T) { |
| + ctx := context.Background() |
| + s, p, w := release.NewDirStore(t.TempDir()), NewMemProvisioner("1"), dnsrec.NewMemWriter() |
| + now := time.Date(2026, 9, 23, 12, 0, 0, 0, time.UTC) |
| + |
| + if _, _, err := EnsureDomain(ctx, s, p, w, testDeployment(), "aaa", "app1", "custom.example", now); err != nil { |
| + t.Fatalf("EnsureDomain: %v", err) |
| + } |
| + if len(w.Records) != 0 { |
| + t.Fatalf("fixture broken: CARLOS published records for a custom domain: %v", w.Records) |
| + } |
| + out, err := RemoveDomain(ctx, s, p, w, testDeployment(), "aaa", "app1", "custom.example") |
| + if err != nil { |
| + t.Fatalf("RemoveDomain: %v", err) |
| + } |
| + if out.Kind != KindCustom { |
| + t.Fatalf("kind = %q, want %q", out.Kind, KindCustom) |
| + } |
| + // Six records: three DKIM CNAMEs, the MAIL FROM MX, its SPF TXT and |
| + // the DMARC TXT — the whole set the member was asked to publish. |
| + if len(out.Orphaned) != 6 { |
| + t.Fatalf("orphaned = %d records, want the whole set of 6: %+v", len(out.Orphaned), out.Orphaned) |
| + } |
| + var cnames int |
| + for _, r := range out.Orphaned { |
| + if r.Type == dnsrec.TypeCNAME { |
| + cnames++ |
| + } |
| + } |
| + if cnames != 3 { |
| + t.Fatalf("token-derived DKIM CNAMEs = %d, want 3", cnames) |
| + } |
| +} |
| + |
| +// A PLATFORM domain's records are CARLOS's own, so they go — the same split |
| +// Teardown makes, for the same reason. |
| +func TestRemoveDomainDeletesAPlatformDomainsPublishedRecords(t *testing.T) { |
| + ctx := context.Background() |
| + s, p, w := release.NewDirStore(t.TempDir()), NewMemProvisioner("1"), dnsrec.NewMemWriter() |
| + now := time.Date(2026, 9, 23, 12, 0, 0, 0, time.UTC) |
| + |
| + if _, _, err := EnsureDomain(ctx, s, p, w, testDeployment(), "bag", "shop", "shop.bag.oncarlos.com", now); err != nil { |
| + t.Fatalf("EnsureDomain: %v", err) |
| + } |
| + if len(w.Records) == 0 { |
| + t.Fatal("fixture broken: nothing was published for a platform domain") |
| + } |
| + out, err := RemoveDomain(ctx, s, p, w, testDeployment(), "bag", "shop", "shop.bag.oncarlos.com") |
| + if err != nil { |
| + t.Fatalf("RemoveDomain: %v", err) |
| + } |
| + if len(w.Records) != 0 { |
| + t.Fatalf("platform records survived the removal: %v", w.Records) |
| + } |
| + if len(out.Orphaned) != 0 { |
| + t.Fatalf("a platform domain reported orphaned records it does not own: %+v", out.Orphaned) |
| + } |
| +} |
| + |
| +// A domain this app does not use is a refusal, not a silent success — and |
| +// running rm twice takes the same path, so "already removed" never reads as |
| +// "removed something else". |
| +func TestRemoveDomainRefusesADomainThisAppDoesNotUse(t *testing.T) { |
| + ctx, s, p, w, _ := removeFixture(t) |
| + |
| + _, err := RemoveDomain(ctx, s, p, w, testDeployment(), "aaa", "app3", "shared.example") |
| + if !errors.Is(err, ErrDomainNotReferenced) { |
| + t.Fatalf("non-referrer err = %v, want ErrDomainNotReferenced", err) |
| + } |
| + if rec, _, err := GetDomain(ctx, s, "aaa", "shared.example"); err != nil || rec == nil || len(rec.Apps) != 2 { |
| + t.Fatalf("a non-referrer's refusal changed the record: %+v (err %v)", rec, err) |
| + } |
| + |
| + if _, err := RemoveDomain(ctx, s, p, w, testDeployment(), "aaa", "app1", "shared.example"); err != nil { |
| + t.Fatalf("first remove: %v", err) |
| + } |
| + _, err = RemoveDomain(ctx, s, p, w, testDeployment(), "aaa", "app1", "shared.example") |
| + if !errors.Is(err, ErrDomainNotReferenced) { |
| + t.Fatalf("second remove err = %v, want the same ErrDomainNotReferenced", err) |
| + } |
| +} |
| + |
| +// OWNER-GATING. A claim held by another account must be untouchable and |
| +// indistinguishable from a domain that was never claimed at all: the same |
| +// error, and nothing in it naming the holder. Anything else turns this door |
| +// into an oracle for who owns what. |
| +func TestRemoveDomainNeverTouchesAnotherAccountsClaim(t *testing.T) { |
| + ctx := context.Background() |
| + s, w := release.NewDirStore(t.TempDir()), dnsrec.NewMemWriter() |
| + p := NewMemProvisioner("shared-aws-account") |
| + now := time.Date(2026, 9, 23, 12, 0, 0, 0, time.UTC) |
| + |
| + if _, _, err := EnsureDomain(ctx, s, p, w, testDeployment(), "secretacct", "victimapp", "victim.example", now); err != nil { |
| + t.Fatalf("EnsureDomain: %v", err) |
| + } |
| + |
| + _, err := RemoveDomain(ctx, s, p, w, testDeployment(), "bbb", "evilapp", "victim.example") |
| + if !errors.Is(err, ErrDomainNotReferenced) { |
| + t.Fatalf("cross-account remove err = %v, want ErrDomainNotReferenced", err) |
| + } |
| + if strings.Contains(err.Error(), "secretacct") { |
| + t.Fatalf("the refusal named the holding account: %v", err) |
| + } |
| + if claim, err := getDomainClaim(ctx, s, "victim.example"); err != nil || claim == nil || claim.Account != "secretacct" { |
| + t.Fatalf("another account's claim was disturbed: %+v (err %v)", claim, err) |
| + } |
| + if _, ok := p.Identities[identityKey("eu-west-1", "victim.example")]; !ok { |
| + t.Fatal("another account's identity was destroyed") |
| + } |
| + // A domain nobody ever claimed answers identically. |
| + _, err2 := RemoveDomain(ctx, s, p, w, testDeployment(), "bbb", "evilapp", "unclaimed.example") |
| + if err.Error() != strings.Replace(err2.Error(), "unclaimed.example", "victim.example", 1) { |
| + t.Fatalf("claimed-by-another and never-claimed read differently:\n %v\n %v", err, err2) |
| + } |
| +} |
| + |
| +// The orphaned claim: EnsureDomain failed after claimDomain and before |
| +// PutDomain, so a global claim exists with NO record behind it. Teardown |
| +// could sweep it, but only by deleting the whole app — an operator who did |
| +// not want to do that had no recourse at all. |
| +func TestRemoveDomainSweepsAClaimWithNoDomainRecord(t *testing.T) { |
| + ctx := context.Background() |
| + s, w := release.NewDirStore(t.TempDir()), dnsrec.NewMemWriter() |
| + p := NewMemProvisioner("shared-aws-account") |
| + now := time.Date(2026, 9, 23, 12, 0, 0, 0, time.UTC) |
| + |
| + p.FailNext["EnsureIdentity"] = errors.New("SES throttled") |
| + if _, _, err := EnsureDomain(ctx, s, p, w, testDeployment(), "aaa", "app1", "stranded.example", now); err == nil { |
| + t.Fatal("expected the injected failure") |
| + } |
| + if claim, err := getDomainClaim(ctx, s, "stranded.example"); err != nil || claim == nil { |
| + t.Fatalf("fixture broken: no stranded claim: %+v (err %v)", claim, err) |
| + } |
| + |
| + out, err := RemoveDomain(ctx, s, p, w, testDeployment(), "aaa", "app1", "stranded.example") |
| + if err != nil { |
| + t.Fatalf("RemoveDomain: %v", err) |
| + } |
| + if !out.ClaimOnly || !out.TornDown { |
| + t.Fatalf("orphan sweep did not report itself as one: %+v", out) |
| + } |
| + if claim, err := getDomainClaim(ctx, s, "stranded.example"); err != nil || claim != nil { |
| + t.Fatalf("the stranded claim survived: %+v (err %v)", claim, err) |
| + } |
| + other := NewMemProvisioner("shared-aws-account") |
| + if _, _, err := EnsureDomain(ctx, s, other, w, testDeployment(), "bbb", "jam", "stranded.example", now); err != nil { |
| + t.Fatalf("another account still cannot claim the freed domain: %v", err) |
| + } |
| +} |
| + |
| +// THE ORDERING INVARIANT (walk.go's RELEASE BEFORE DELETE comment). A crash |
| +// between releasing the claim and deleting the record must be recoverable — |
| +// which it is precisely because the release came first. The reverse order |
| +// strands the claim forever: the record would be gone, so nothing would |
| +// ever list the domain again to reach the release call. |
| +func TestRemoveDomainClaimReleaseSurvivesAFailedRecordDelete(t *testing.T) { |
| + ctx := context.Background() |
| + inner, w := release.NewDirStore(t.TempDir()), dnsrec.NewMemWriter() |
| + p := NewMemProvisioner("shared-aws-account") |
| + now := time.Date(2026, 9, 23, 12, 0, 0, 0, time.UTC) |
| + |
| + if _, _, err := EnsureDomain(ctx, inner, p, w, testDeployment(), "aaa", "app1", "mine.example", now); err != nil { |
| + t.Fatalf("EnsureDomain: %v", err) |
| + } |
| + s := &failDeleteOnce{Store: inner, key: DomainKey("aaa", "mine.example")} |
| + if _, err := RemoveDomain(ctx, s, p, w, testDeployment(), "aaa", "app1", "mine.example"); err == nil { |
| + t.Fatal("expected the injected record-delete failure to surface") |
| + } |
| + if claim, err := getDomainClaim(ctx, inner, "mine.example"); err != nil || claim != nil { |
| + t.Fatalf("claim survived a failed record delete: %+v (err %v)", claim, err) |
| + } |
| + // Free immediately, with no retry required of anyone. |
| + other := NewMemProvisioner("shared-aws-account") |
| + if _, _, err := EnsureDomain(ctx, inner, other, w, testDeployment(), "bbb", "jam", "mine.example", now); err != nil { |
| + t.Fatalf("another account cannot claim the released domain: %v", err) |
| + } |
| + // And the stale record is still swept by the app's own teardown, which |
| + // must not error trying to release a claim that is now somebody else's. |
| + if err := Teardown(ctx, inner, p, w, testDeployment(), "aaa", "app1"); err != nil { |
| + t.Fatalf("teardown did not repair the leftover record: %v", err) |
| + } |
| +} |
| + |
| +// The departing app's IAM policy must SHRINK. EnsureUser writes a policy |
| +// wholesale from the current granted set and nothing else recomputes it |
| +// when a domain leaves — so without RemoveDomain's own RefreshPolicies |
| +// call, an app that drops a domain a sibling keeps alive goes on holding |
| +// ses:SendRawEmail at a live identity ARN it no longer has any claim to. |
| +func TestRemoveDomainShrinksTheDepartingAppsPolicy(t *testing.T) { |
| + ctx, s, p, pub, now := enableFixture(t) |
| + extra := "extra.bag.oncarlos.com" |
| + w := dnsrec.NewMemWriter() |
| + |
| + // A second app joins the extra domain too, so removing harbourmail's |
| + // referral leaves the identity ALIVE — the case where a stale policy is |
| + // a live over-grant rather than a dangling string. |
| + for _, app := range []string{"harbourmail", "marketing"} { |
| + if _, _, err := EnsureDomain(ctx, s, p, w, testDeployment(), "bag", app, extra, now); err != nil { |
| + t.Fatalf("EnsureDomain %s: %v", app, err) |
| + } |
| + } |
| + p.SetIdentityStatus("eu-west-1", extra, Status{DKIM: "SUCCESS", MailFrom: "SUCCESS", Sending: "SUCCESS"}) |
| + if _, err := VerifyDomain(ctx, s, p, testDeployment(), "bag", extra, "eu-west-1", now); err != nil { |
| + t.Fatalf("VerifyDomain: %v", err) |
| + } |
| + if _, err := Enable(ctx, s, p, pub, testDeployment(), "bag", "harbourmail", now); err != nil { |
| + t.Fatalf("Enable: %v", err) |
| + } |
| + user := p.Users[UserName("bag", "harbourmail")] |
| + if user == nil || !strings.Contains(user.Policy, extra) { |
| + t.Fatalf("fixture broken: policy does not name %s:\n%v", extra, user) |
| + } |
| + |
| + if _, err := RemoveDomain(ctx, s, p, w, testDeployment(), "bag", "harbourmail", extra); err != nil { |
| + t.Fatalf("RemoveDomain: %v", err) |
| + } |
| + user = p.Users[UserName("bag", "harbourmail")] |
| + if user == nil { |
| + t.Fatal("the delivered principal disappeared") |
| + } |
| + if strings.Contains(user.Policy, extra) { |
| + t.Fatalf("policy still grants sending at a domain this app no longer uses:\n%s", user.Policy) |
| + } |
| + if !strings.Contains(user.Policy, "harbourmail.bag.oncarlos.com") { |
| + t.Fatalf("policy lost the domain this app still sends from:\n%s", user.Policy) |
| + } |
| + if _, ok := p.Identities[identityKey("eu-west-1", extra)]; !ok { |
| + t.Fatal("the identity the sibling app still uses was destroyed") |
| + } |
| +} |
| + |
| +// The claim must NOT be released when the identity could not actually be |
| +// destroyed. detachDomainApp reaches releaseDomainClaim only after |
| +// DeleteIdentity succeeds, and SES refuses that delete while a tenant |
| +// association still exists (spec §4.1) — so a `rm` that trips it leaves the |
| +// domain exactly as it was. Releasing anyway is the one thing this verb |
| +// must never do: it would hand a stranger a live identity's ARN and its |
| +// already-minted DKIM tokens. |
| +func TestRemoveDomainKeepsTheClaimWhenTheIdentityCannotBeDeleted(t *testing.T) { |
| + ctx, s, p, w, _ := removeFixture(t) |
| + if _, err := RemoveDomain(ctx, s, p, w, testDeployment(), "aaa", "app1", "shared.example"); err != nil { |
| + t.Fatalf("remove app1: %v", err) |
| + } |
| + |
| + // Left associated on purpose: EnsureDomain associated it and nothing |
| + // has removed it, so this is the case DeleteIdentity's guard exists for. |
| + p.FailNext["DisassociateTenantResource"] = errors.New("AccessDenied: ses:DeleteTenantResourceAssociation") |
| + |
| + if _, err := RemoveDomain(ctx, s, p, w, testDeployment(), "aaa", "app2", "shared.example"); err == nil { |
| + t.Fatal("rm succeeded despite the identity still being associated") |
| + } |
| + if claim, err := getDomainClaim(ctx, s, "shared.example"); err != nil || claim == nil { |
| + t.Fatalf("the claim was released while the identity is still alive: %+v (err %v)", claim, err) |
| + } |
| + if _, ok := p.Identities[identityKey("eu-west-1", "shared.example")]; !ok { |
| + t.Fatal("the identity was deleted despite still being associated") |
| + } |
| + stranger := NewMemProvisioner("shared-aws-account") |
| + now := time.Date(2026, 9, 23, 12, 0, 0, 0, time.UTC) |
| + if _, _, err := EnsureDomain(ctx, s, stranger, w, testDeployment(), "bbb", "evilapp", "shared.example", now); err == nil { |
| + t.Fatal("a stranger claimed a domain whose identity is still live") |
| + } |
| +} |
| + |
| +// The mirror of Teardown's own I2 case: a failed disassociate that does not |
| +// matter (nothing was really associated) must be logged, not fatal, so `rm` |
| +// still reaches DeleteIdentity and releases the claim. A hard error here |
| +// would recreate the permanent stranding this verb exists to end. |
| +func TestRemoveDomainFinishesDespiteAnInconsequentialDisassociateFailure(t *testing.T) { |
| + ctx, s, p, w, _ := removeFixture(t) |
| + if _, err := RemoveDomain(ctx, s, p, w, testDeployment(), "aaa", "app1", "shared.example"); err != nil { |
| + t.Fatalf("remove app1: %v", err) |
| + } |
| + |
| + // Stand in for a domain that predates the tenant-association release: |
| + // verified, but never actually associated. |
| + ident := p.Identities[identityKey("eu-west-1", "shared.example")] |
| + delete(p.TenantResources[TenantName("aaa")], ident.ARN) |
| + p.FailNext["DisassociateTenantResource"] = errors.New("AccessDenied: ses:DeleteTenantResourceAssociation") |
| + |
| + out, err := RemoveDomain(ctx, s, p, w, testDeployment(), "aaa", "app2", "shared.example") |
| + if err != nil { |
| + t.Fatalf("RemoveDomain: %v, want the failed disassociate logged and the removal to finish", err) |
| + } |
| + if !out.TornDown { |
| + t.Fatalf("removal did not tear down: %+v", out) |
| + } |
| + if claim, err := getDomainClaim(ctx, s, "shared.example"); err != nil || claim != nil { |
| + t.Fatalf("the claim was stranded by an inconsequential disassociate failure: %+v (err %v)", claim, err) |
| + } |
| +} |
diff --git a/mail_cli.go b/mail_cli.go| index caf2d6517..81711c54c 100644 |
| --- a/mail_cli.go |
| +++ b/mail_cli.go |
| @@ -1,6 +1,6 @@ |
| // mail_cli.go is `carlos email`: the member surface for tenant email sending |
| -// (tenant-email spec, task 18). It calls the nine doors internal/console/ |
| -// apimail.go added in task 17 — API mode only, store.go's sibling and its |
| +// (tenant-email spec, task 18). It calls the ten doors internal/console/ |
| +// apimail.go carries (nine from task 17, plus `domains rm`) — API mode only, store.go's sibling and its |
| // pattern: a package-level flag family, run* verbs that resolve a session |
| // and print* helpers that take an io.Writer so the honesty rules below can |
| // be pinned without a subprocess. |
| @@ -75,6 +75,16 @@ const emailUsage = `usage: carlos email <enable|domains|status|test|credentials| |
| sending domain for an app already declared, print |
| the record set on a custom domain, and poll to |
| verified the same way. |
| + domains rm --app <a> <domain> |
| + stop this app sending from a domain and, when it |
| + was the LAST app using it, tear the SES identity |
| + down and RELEASE the global claim — the only way |
| + short of deleting the app to make a domain |
| + claimable by another CARLOS account again. |
| + Refused while <domain> is where this app's own |
| + From address sits: move that first. A custom |
| + domain's DNS is yours, so the now-stale records |
| + are printed rather than deleted. |
| status --app <a> domains and their per-region verification state, |
| today's counts against the cap, and the pause |
| state. |
| @@ -301,6 +311,21 @@ type apiEmailDomainVerifyReply struct { |
| Domain apiEmailDomain `json:"domain"` |
| } |
| +// apiEmailDomainRemoveReply is handleAPIEmailDomainRemove's reply. It says |
| +// what actually happened rather than just "ok", because the three outcomes |
| +// have different consequences for the member: a sibling app kept the domain |
| +// alive (nothing was released), this app was the last one (the claim is now |
| +// free for any account), or only a stranded claim existed to sweep. |
| +type apiEmailDomainRemoveReply struct { |
| + App string `json:"app"` |
| + Domain string `json:"domain"` |
| + Kind string `json:"kind"` |
| + TornDown bool `json:"torn_down"` |
| + ClaimOnly bool `json:"claim_only"` |
| + RemainingApps []string `json:"remaining_apps"` |
| + Orphaned []apiDNSRecord `json:"orphaned"` |
| +} |
| + |
| // apiEmailCredsReply is handleAPIEmailCredentialsCreate's reply — THE ONLY |
| // one anywhere in this file that carries a password. |
| type apiEmailCredsReply struct { |
| @@ -444,6 +469,21 @@ func (s *apiSession) emailDomainVerify(ctx context.Context, a apiAcct, app, doma |
| return out, nil |
| } |
| +func (s *apiSession) emailDomainRemove(ctx context.Context, a apiAcct, app, domain string) (apiEmailDomainRemoveReply, error) { |
| + status, body, err := s.deleteJSON(ctx, apiPath(a, app, "/email/domains/"+pathEscape(domain))) |
| + if err != nil { |
| + return apiEmailDomainRemoveReply{}, err |
| + } |
| + if status != http.StatusOK { |
| + return apiEmailDomainRemoveReply{}, mailAPIError(status, body) |
| + } |
| + var out apiEmailDomainRemoveReply |
| + if err := json.Unmarshal(body, &out); err != nil { |
| + return apiEmailDomainRemoveReply{}, fmt.Errorf("email domains rm: unreadable response: %w", err) |
| + } |
| + return out, nil |
| +} |
| + |
| func (s *apiSession) emailEnable(ctx context.Context, a apiAcct, app string) (apiEmailReply, error) { |
| status, body, err := s.postJSON(ctx, apiPath(a, app, "/email/enable"), struct{}{}) |
| if err != nil { |
| @@ -739,11 +779,23 @@ func runEmailEnableFlow(ctx context.Context, w io.Writer, sess *apiSession, acct |
| // ---- domains add ------------------------------------------------------------ |
| func runEmailDomains(args []string) { |
| - if len(args) == 0 || args[0] != "add" { |
| + if len(args) == 0 { |
| + fmt.Fprint(os.Stderr, emailUsage) |
| + os.Exit(1) |
| + } |
| + switch args[0] { |
| + case "add": |
| + runEmailDomainsAdd(args[1:]) |
| + // "remove" is accepted alongside "rm" because this is the one verb in |
| + // the family whose effect is irreversible for other people — a domain |
| + // released is a domain a stranger may claim — and a member who spells |
| + // it out in full should not be answered with a usage block. |
| + case "rm", "remove": |
| + runEmailDomainsRemove(args[1:]) |
| + default: |
| fmt.Fprint(os.Stderr, emailUsage) |
| os.Exit(1) |
| } |
| - runEmailDomainsAdd(args[1:]) |
| } |
| func runEmailDomainsAdd(args []string) { |
| @@ -807,6 +859,110 @@ func printPendingRecords(w io.Writer, recs []apiDNSRecord) { |
| tw.Flush() |
| } |
| +// ---- domains rm ------------------------------------------------------------- |
| + |
| +// runEmailDomainsRemove is `carlos email domains rm`: the release door for a |
| +// sending domain's GLOBAL claim. |
| +// |
| +// Why it exists, since the shape is otherwise unremarkable: the claim |
| +// (internal/mail/domainclaim.go) is a deliberately global, create-only |
| +// object, so exactly one CARLOS account can hold a domain. That is right. |
| +// What was missing was the way back out — the only thing that ever released |
| +// a claim was permanently DELETING the app that held it, so a domain |
| +// claimed by the wrong app of the wrong account was unreachable to |
| +// everybody, its rightful owner included, forever. This is that door. |
| +func runEmailDomainsRemove(args []string) { |
| + fs := flag.NewFlagSet("email domains rm", flag.ExitOnError) |
| + app := appFlag(fs, "app to remove the domain from") |
| + acctRaw, acctIDRaw, _ := accountFlags(fs) |
| + consoleFlag := fs.String("console", "", "console origin (API mode only; default: the only logged-in one)") |
| + fs.Usage = flagUsage(fs, "") |
| + fs.Parse(args) |
| + usage := "usage: carlos email domains rm --app <a> [--account <sqid-or-name>] " + |
| + "[--account-id <sqid>] [--console <origin>] <domain>" |
| + if fs.NArg() != 1 { |
| + log.Fatal(usage) |
| + } |
| + domain := strings.ToLower(strings.TrimSpace(fs.Arg(0))) |
| + name := strings.TrimSpace(*app) |
| + if name != "" { |
| + if err := validAppName(name); err != nil { |
| + log.Fatalf("email domains rm: %v", err) |
| + } |
| + } |
| + |
| + sess := requireMailAPISession(*consoleFlag) |
| + printUsing(stdout, sess) |
| + acct := resolvedAccount(sess, acctRaw, acctIDRaw) |
| + name = resolvedApp(sess, acct, name, usage) |
| + |
| + res, err := sess.emailDomainRemove(context.Background(), acct, name, domain) |
| + if err != nil { |
| + // No retry framing and no "permanent" framing either, unlike |
| + // `domains add`: this command's two refusals are both states the |
| + // caller can change (move the From; name a domain the app actually |
| + // uses), and the console's own message already says which and how. |
| + log.Fatalf("email domains rm: %v", err) |
| + } |
| + printDomainRemoval(stdout, res) |
| +} |
| + |
| +// printDomainRemoval says which of the three things happened, because they |
| +// are not interchangeable: a sibling app kept the domain alive, this app was |
| +// the last one and the claim is now free, or there was never a record at all |
| +// and a stranded claim was swept. |
| +func printDomainRemoval(w io.Writer, res apiEmailDomainRemoveReply) { |
| + c := colorFor(w) |
| + fmt.Fprintf(w, "%s %s from %s\n", c.ok("removed"), res.Domain, res.App) |
| + switch { |
| + case !res.TornDown: |
| + mutedf(w, " %s still uses it, so its identity, its DNS and its claim are untouched", |
| + strings.Join(res.RemainingApps, ", ")) |
| + return |
| + case res.ClaimOnly: |
| + mutedf(w, " there was no domain record — only a stranded claim, now released") |
| + default: |
| + mutedf(w, " last app to use it: the SES identity is gone and the domain is claimable again") |
| + } |
| + if len(res.Orphaned) == 0 { |
| + return |
| + } |
| + // A CUSTOM domain's records live in the customer's own zone and are the |
| + // customer's to remove — CARLOS never published them and will not delete |
| + // them. Printing them is the whole point: nothing else in this CLI can |
| + // tell an operator which names in their zone just went stale. |
| + dead, durable := splitDKIMRecords(res.Orphaned) |
| + if len(dead) > 0 { |
| + fmt.Fprintln(w, "these records in your DNS are now dead — the identity that signed with them is gone:") |
| + printPendingRecords(w, dead) |
| + } |
| + if len(durable) > 0 { |
| + fmt.Fprintf(w, "these are unused but still correct — re-creating %s here would want them back unchanged:\n", res.Domain) |
| + printPendingRecords(w, durable) |
| + } |
| +} |
| + |
| +// splitDKIMRecords divides an orphaned record set into the half that is now |
| +// DEAD and the half that is merely unused. |
| +// |
| +// Only the DKIM CNAMEs are derived from the identity's own tokens |
| +// (mail.RecordSet), so only they stop meaning anything the moment that |
| +// identity is deleted. The MAIL FROM MX, its SPF TXT and the DMARC TXT are |
| +// deterministic functions of the domain and the region: they come back |
| +// byte-identical if the domain is ever re-created here, so telling an |
| +// operator to rip them out would be advice that costs them a second |
| +// publish for nothing. |
| +func splitDKIMRecords(recs []apiDNSRecord) (dead, durable []apiDNSRecord) { |
| + for _, r := range recs { |
| + if r.Type == "CNAME" { |
| + dead = append(dead, r) |
| + continue |
| + } |
| + durable = append(durable, r) |
| + } |
| + return dead, durable |
| +} |
| + |
| // ---- status ----------------------------------------------------------------- |
| func runEmailStatus(args []string) { |
diff --git a/mail_cli_test.go b/mail_cli_test.go| index 4e9cb11db..ed278cbb1 100644 |
| --- a/mail_cli_test.go |
| +++ b/mail_cli_test.go |
| @@ -162,6 +162,98 @@ func TestAPISessionEmailDomainVerifyWireShape(t *testing.T) { |
| } |
| } |
| +func TestAPISessionEmailDomainRemoveWireShape(t *testing.T) { |
| + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| + if r.URL.Path != "/api/cli/apps/acct/app/email/domains/x.com" || r.Method != http.MethodDelete { |
| + t.Errorf("path/method = %s %s", r.Method, r.URL.Path) |
| + } |
| + json.NewEncoder(w).Encode(map[string]any{ |
| + "app": "app", "domain": "x.com", "kind": "custom", "torn_down": true, |
| + "orphaned": []map[string]any{ |
| + {"name": "tok._domainkey.x.com", "type": "CNAME", "values": []string{"tok.dkim.amazonses.com"}, "ttl": 300}, |
| + }, |
| + }) |
| + })) |
| + defer ts.Close() |
| + |
| + res, err := newTestSession(ts.URL).emailDomainRemove(context.Background(), apiAcct{Seg: "acct"}, "app", "x.com") |
| + if err != nil { |
| + t.Fatalf("emailDomainRemove: %v", err) |
| + } |
| + if !res.TornDown || len(res.Orphaned) != 1 { |
| + t.Fatalf("reply = %+v, want a torn-down domain with one orphaned record", res) |
| + } |
| +} |
| + |
| +// TestPrintDomainRemovalSaysWhichOfTheThreeThingsHappened: the outcomes are |
| +// not interchangeable — a sibling app kept the domain alive (nothing was |
| +// released), this app was the last one (the claim is free for any account), |
| +// or only a stranded claim existed to sweep. Printing the same line for all |
| +// three would tell a member nothing about whether the domain is now |
| +// claimable. |
| +func TestPrintDomainRemovalSaysWhichOfTheThreeThingsHappened(t *testing.T) { |
| + render := func(res apiEmailDomainRemoveReply) string { |
| + var buf bytes.Buffer |
| + printDomainRemoval(&buf, res) |
| + return buf.String() |
| + } |
| + |
| + kept := render(apiEmailDomainRemoveReply{ |
| + App: "jam", Domain: "jelly.tito.io", Kind: "custom", RemainingApps: []string{"stripe"}, |
| + }) |
| + if !strings.Contains(kept, "stripe") { |
| + t.Fatalf("a kept domain does not name the app still using it:\n%s", kept) |
| + } |
| + if strings.Contains(kept, "claimable") { |
| + t.Fatalf("a kept domain claims to have been released:\n%s", kept) |
| + } |
| + |
| + torn := render(apiEmailDomainRemoveReply{ |
| + App: "jam", Domain: "jelly.tito.io", Kind: "custom", TornDown: true, |
| + }) |
| + if !strings.Contains(torn, "claimable again") { |
| + t.Fatalf("a released domain does not say it is claimable again:\n%s", torn) |
| + } |
| + |
| + swept := render(apiEmailDomainRemoveReply{ |
| + App: "jam", Domain: "jelly.tito.io", Kind: "custom", TornDown: true, ClaimOnly: true, |
| + }) |
| + if !strings.Contains(swept, "no domain record") { |
| + t.Fatalf("an orphaned-claim sweep does not say what it swept:\n%s", swept) |
| + } |
| +} |
| + |
| +// TestPrintDomainRemovalSeparatesDeadDKIMFromDurableRecords: of the six |
| +// records in a sending domain's set, only the three DKIM CNAMEs are derived |
| +// from the identity's own tokens, so only they are dead once it is gone. |
| +// The MAIL FROM MX, its SPF TXT and the DMARC TXT are deterministic |
| +// functions of the domain and region — telling an operator to rip those out |
| +// would cost them a second publish for nothing. |
| +func TestPrintDomainRemovalSeparatesDeadDKIMFromDurableRecords(t *testing.T) { |
| + var buf bytes.Buffer |
| + printDomainRemoval(&buf, apiEmailDomainRemoveReply{ |
| + App: "jam", Domain: "jelly.tito.io", Kind: "custom", TornDown: true, |
| + Orphaned: []apiDNSRecord{ |
| + {Name: "tok1._domainkey.jelly.tito.io", Type: "CNAME", Values: []string{"tok1.dkim.amazonses.com"}}, |
| + {Name: "mail-eu-west-1.jelly.tito.io", Type: "MX", Values: []string{"10 feedback-smtp.eu-west-1.amazonses.com"}}, |
| + {Name: "_dmarc.jelly.tito.io", Type: "TXT", Values: []string{"v=DMARC1; p=none"}}, |
| + }, |
| + }) |
| + out := buf.String() |
| + |
| + deadAt := strings.Index(out, "tok1._domainkey.jelly.tito.io") |
| + durableAt := strings.Index(out, "mail-eu-west-1.jelly.tito.io") |
| + if deadAt < 0 || durableAt < 0 { |
| + t.Fatalf("the orphaned records were not printed at all:\n%s", out) |
| + } |
| + if !strings.Contains(out, "now dead") || !strings.Contains(out, "still correct") { |
| + t.Fatalf("output does not separate the dead records from the durable ones:\n%s", out) |
| + } |
| + if deadAt > durableAt { |
| + t.Fatalf("the dead records must come first — they are the ones to remove:\n%s", out) |
| + } |
| +} |
| + |
| func TestAPISessionEmailEnableWireShape(t *testing.T) { |
| ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| if r.URL.Path != "/api/cli/apps/acct/app/email/enable" || r.Method != http.MethodPost { |
| @@ -989,7 +1081,7 @@ func TestEmailVerbDispatchAndRefusals(t *testing.T) { |
| return string(out), err |
| } |
| - for _, args := range [][]string{{"email"}, {"email", "wat"}} { |
| + for _, args := range [][]string{{"email"}, {"email", "wat"}, {"email", "domains"}, {"email", "domains", "wat"}} { |
| out, err := run(nil, args...) |
| if err == nil { |
| t.Fatalf("carlos %v exited 0:\n%s", args, out) |
| @@ -1008,6 +1100,7 @@ func TestEmailVerbDispatchAndRefusals(t *testing.T) { |
| {"email", "resume", "-app", "keymail"}, |
| {"email", "credentials", "create", "-app", "keymail", "-name", "laptop"}, |
| {"email", "domains", "add", "-app", "keymail", "x.com"}, |
| + {"email", "domains", "rm", "-app", "keymail", "x.com"}, |
| } { |
| out, err := run([]string{"CARLOS_DEPLOYMENT_DIR=" + t.TempDir()}, args...) |
| if err == nil { |