carlos / platform Public

Merge branch 'tenant-iso-mail' into main

Mail: members can't adopt another party's SES identity; unverified sending-domain claims lapse (#216)
claude agent, operated by paul@keymail.dev be3338d4fbe477dc1f34ab46956cb1b5401ef53a parent f36ad36parent 19f61dd
15 files changed, +893 −47
  • cmd/console/main.go +3 −0
  • docs/site/cli.md +12 −6
  • internal/console/apimail.go +33 −11
  • internal/console/apimail_test.go +30 −0
  • internal/mail/aws.go +10 −1
  • internal/mail/aws_test.go +40 −4
  • internal/mail/domain.go +5 −0
  • internal/mail/domainclaim.go +233 −0
  • internal/mail/memprovisioner.go +7 −3
  • internal/mail/memprovisioner_test.go +27 −7
  • internal/mail/provisioner.go +20 −2
  • internal/mail/walk.go +134 −9
  • internal/mail/walk_identityowner_test.go +332 −0
  • mail_cli.go +5 −2
  • mail_cli_test.go +2 −2

CI failed — run details

diff --git a/cmd/console/main.go b/cmd/console/main.go
index b4fe8ff01..e2ee59ee9 100644
--- a/cmd/console/main.go
+++ b/cmd/console/main.go
@@ -777,6 +777,9 @@ func mailDeployment() mail.Deployment {
PlatformSuffix: strings.ToLower(strings.TrimSpace(os.Getenv("CARLOS_MAIL_PLATFORM_SUFFIX"))),
DMARCRua: strings.TrimSpace(os.Getenv("CARLOS_MAIL_DMARC_RUA")),
SNSTopicARN: strings.TrimSpace(os.Getenv("CARLOS_MAIL_SNS_TOPIC_ARN")),
+ // The proof-of-control lookup for a domain whose SES identity this
+ // account did not create (#216). Read-only DNS, no AWS.
+ Resolver: net.DefaultResolver,
}
}
diff --git a/docs/site/cli.md b/docs/site/cli.md
index a1e2ffbbe..115129a74 100644
--- a/docs/site/cli.md
+++ b/docs/site/cli.md
@@ -1200,13 +1200,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.
+A sending domain can be claimed by exactly one CARLOS account at a time. A
+claim that is never verified lapses after seven days, and then another
+account can add the domain. If the domain already has an SES identity that
+CARLOS did not create for your account, `domains add` refuses it and tells
+you which TXT record to publish (at `_carlos-mail.<domain>`) to prove you
+control the domain. Once that record is there, add the domain again. CARLOS
+sends with an identity adopted this way but never deletes it.
+
`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
+domain one app holds is unavailable to everybody else 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, releases the claim so another account can add and verify
+the domain, and deletes the SES identity if CARLOS created it for you. 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
diff --git a/internal/console/apimail.go b/internal/console/apimail.go
index baf362816..08831f908 100644
--- a/internal/console/apimail.go
+++ b/internal/console/apimail.go
@@ -1015,18 +1015,29 @@ func (s *Server) handleAPIEmailDomainEnsure(w http.ResponseWriter, r *http.Reque
d, writer := s.mailDeploymentFor()
rec, pending, err := mail.EnsureDomain(ctx, s.cfg.Releases, s.cfg.MailProvisioner, writer, d, acct.Sqid, app, domain, time.Now())
if err != nil {
- // I3 (Task 17a review follow-up): ErrDomainClaimed is a PERMANENT
- // refusal, not a transient failure — re-running buys the member
- // nothing, because the outcome does not depend on anything that
- // changes between attempts. Rendering it as the generic 502 below
- // told a member to retry something that can never succeed, with a
- // server-error status for a client-side condition. 409 names the
- // real shape (a conflict with existing state) and the message stays
- // neutral — mail.ErrDomainClaimed's own text never names the
- // holding account, and this door must not add it back.
+ // I3 (Task 17a review follow-up): ErrDomainClaimed is a refusal a
+ // retry cannot change, not a transient failure. Rendering it as the
+ // generic 502 below told a member to retry, with a server-error
+ // status for a client-side condition. 409 names the real shape (a
+ // conflict with existing state).
+ //
+ // The message is the neutral one (#216), in the shape of the
+ // hostname door's hostUnavailableMsg: it used to say "already
+ // claimed by another CARLOS account", which confirmed to anybody
+ // that some tenant holds the name.
if errors.Is(err, mail.ErrDomainClaimed) {
- jsonError(w, http.StatusConflict,
- "this domain is already claimed by another CARLOS account — choose a different domain")
+ jsonError(w, http.StatusConflict, mailDomainUnavailableMsg)
+ return
+ }
+ // ErrIdentityNotOwned (#216): SES already has an identity for this
+ // domain that this account's claim did not create. The member is
+ // told exactly how to prove control, because the proof is the only
+ // way in and nobody can guess the record.
+ if errors.Is(err, mail.ErrIdentityNotOwned) {
+ jsonError(w, http.StatusConflict, fmt.Sprintf(
+ "%s already has a sending identity this account did not create. To use it, prove you control the domain: "+
+ "publish a TXT record at %s with the value %s, then add the domain again.",
+ domain, mail.ProofRecordName(domain), mail.ProofRecordValue(acct.Sqid)))
return
}
// Same reasoning for ErrNotOwnPlatformName: refused because of what
@@ -1056,6 +1067,11 @@ func (s *Server) handleAPIEmailDomainEnsure(w http.ResponseWriter, r *http.Reque
})
}
+// mailDomainUnavailableMsg is the one refusal for a sending domain another
+// account holds, in the hostname door's neutral shape (hostUnavailableMsg):
+// it says no, and not who or whether anyone does.
+const mailDomainUnavailableMsg = "sending domain unavailable"
+
// handleAPIEmailDomainVerify answers `carlos email domains verify`:
// re-check SES's own verdict for this deployment's region and record it.
// Account-scoped, like the domain record itself (domain.go's package
@@ -1095,6 +1111,12 @@ func (s *Server) handleAPIEmailDomainVerify(w http.ResponseWriter, r *http.Reque
// record last recorded.
rec, err := mail.VerifyDomain(ctx, s.cfg.Releases, s.cfg.MailProvisioner,
s.cfg.MailDeployment, acct.Sqid, domain, region, time.Now())
+ if errors.Is(err, mail.ErrDomainClaimed) {
+ // The claim lapsed unverified and another account took the domain
+ // (#216). The same neutral words as the add door.
+ jsonError(w, http.StatusConflict, mailDomainUnavailableMsg)
+ return
+ }
if err != nil {
log.Printf("console: api email domain verify %s/%s: %v", acct.Sqid, domain, err)
jsonError(w, http.StatusBadGateway, "could not check verification — try again")
diff --git a/internal/console/apimail_test.go b/internal/console/apimail_test.go
index 62af15b14..12b52d2e5 100644
--- a/internal/console/apimail_test.go
+++ b/internal/console/apimail_test.go
@@ -1450,6 +1450,36 @@ func TestAPIEmailDomainEnsureRefusesADomainClaimedByAnotherAccount(t *testing.T)
if strings.Contains(strings.ToLower(msg), "try again") || strings.Contains(strings.ToLower(msg), "re-run") {
t.Fatalf("refusal promised a retry would help, but this refusal is permanent: %q", msg)
}
+ // Neutral (#216): the refusal must not confirm that another account
+ // holds the domain, only that this one cannot have it.
+ if msg != mailDomainUnavailableMsg {
+ t.Fatalf("refusal = %q, want the neutral %q", msg, mailDomainUnavailableMsg)
+ }
+}
+
+// TestAPIEmailDomainEnsureRefusesAForeignIdentityWithProofSteps (#216): a
+// domain already verified in SES outside CARLOS is refused with 409 and the
+// exact TXT record that would prove control, and the identity is untouched.
+func TestAPIEmailDomainEnsureRefusesAForeignIdentityWithProofSteps(t *testing.T) {
+ fx, fake, _, token := emailArmed(t, "code-email-domain-foreign")
+ app := "titogo"
+ declareEmail(t, fx, token, app, "hello@tito.io")
+ fake.Identities["eu-west-1|tito.io"] = &mail.MemIdentity{
+ ARN: "arn:aws:ses:eu-west-1:1:identity/tito.io", DKIMTokens: []string{"c1", "c2", "c3"},
+ MailFrom: "bounce.tito.io", ConfigSet: "classic",
+ Status: mail.Status{DKIM: "SUCCESS", MailFrom: "SUCCESS", Sending: "SUCCESS"},
+ }
+ status, out := postJSON(t, emailURL(fx, app)+"/domains", map[string]any{"domain": "tito.io"}, token)
+ if status != http.StatusConflict {
+ t.Fatalf("ensure a foreign identity = %d, want 409: %v", status, out)
+ }
+ msg, _ := out["error"].(string)
+ if !strings.Contains(msg, mail.ProofRecordName("tito.io")) || !strings.Contains(msg, mail.ProofRecordValue(fx.sqid)) {
+ t.Fatalf("refusal does not say how to prove control: %q", msg)
+ }
+ if got := fake.Identities["eu-west-1|tito.io"]; got.MailFrom != "bounce.tito.io" || got.ConfigSet != "classic" {
+ t.Fatalf("the foreign identity was mutated: %+v", got)
+ }
}
// TestAPIEmailEventsReadsTheLog: the door `carlos email events` calls hands
diff --git a/internal/mail/aws.go b/internal/mail/aws.go
index 0c68bd433..682b21e1b 100644
--- a/internal/mail/aws.go
+++ b/internal/mail/aws.go
@@ -291,19 +291,27 @@ func (p *awsProvisioner) SetIdentityConfigSet(ctx context.Context, region, domai
// domain and p.account (resolved once at construction — controller ruling
// F3), in the exact shape SendPolicyJSON validates:
// arn:aws:ses:<region>:<account-id>:identity/<domain>.
-func (p *awsProvisioner) EnsureIdentity(ctx context.Context, region, domain, mailFrom string) (Identity, error) {
+//
+// With adopt false, "already exists" is ErrIdentityExists, returned before
+// the identity is read or its MAIL FROM touched: the identity may belong to
+// another product in this AWS account (#216).
+func (p *awsProvisioner) EnsureIdentity(ctx context.Context, region, domain, mailFrom string, adopt bool) (Identity, error) {
if err := p.checkRegion(region); err != nil {
return Identity{}, err
}
var tokens []string
+ created := false
out, err := p.ses.CreateEmailIdentity(ctx, &sesv2.CreateEmailIdentityInput{
EmailIdentity: aws.String(domain),
})
switch {
case err == nil:
+ created = true
if out.DkimAttributes != nil {
tokens = out.DkimAttributes.Tokens
}
+ case isAlreadyExists(err) && !adopt:
+ return Identity{}, fmt.Errorf("mail: identity %q in %s: %w", domain, region, ErrIdentityExists)
case isAlreadyExists(err):
got, gerr := p.ses.GetEmailIdentity(ctx, &sesv2.GetEmailIdentityInput{
EmailIdentity: aws.String(domain),
@@ -338,6 +346,7 @@ func (p *awsProvisioner) EnsureIdentity(ctx context.Context, region, domain, mai
return Identity{
ARN: fmt.Sprintf("arn:aws:ses:%s:%s:identity/%s", region, p.account, domain),
DKIMTokens: tokens,
+ Created: created,
}, nil
}
diff --git a/internal/mail/aws_test.go b/internal/mail/aws_test.go
index ae10580bb..b57c1f893 100644
--- a/internal/mail/aws_test.go
+++ b/internal/mail/aws_test.go
@@ -145,7 +145,7 @@ func TestEnsureIdentityAlreadyExistsReadsBackTokensAndComposesTheARN(t *testing.
}
p := &awsProvisioner{ses: stub, account: "123456789012", region: "eu-west-1"}
- got, err := p.EnsureIdentity(context.Background(), "eu-west-1", "example.com", "mail.example.com")
+ got, err := p.EnsureIdentity(context.Background(), "eu-west-1", "example.com", "mail.example.com", true)
if err != nil {
t.Fatalf("EnsureIdentity: %v", err)
}
@@ -168,6 +168,42 @@ func TestEnsureIdentityAlreadyExistsReadsBackTokensAndComposesTheARN(t *testing.
}
}
+// TestEnsureIdentityWithoutAdoptLeavesAnExistingIdentityAlone (#216): with
+// adopt false, an identity that already exists may belong to another product
+// in this AWS account, so it is refused before it is read or its MAIL FROM
+// rewritten.
+func TestEnsureIdentityWithoutAdoptLeavesAnExistingIdentityAlone(t *testing.T) {
+ touched := false
+ stub := &stubSES{
+ createEmailIdentity: func(*sesv2.CreateEmailIdentityInput) (*sesv2.CreateEmailIdentityOutput, error) {
+ return nil, &sesv2types.AlreadyExistsException{Message: aws.String("identity already exists")}
+ },
+ getEmailIdentity: func(*sesv2.GetEmailIdentityInput) (*sesv2.GetEmailIdentityOutput, error) {
+ touched = true
+ return &sesv2.GetEmailIdentityOutput{}, nil
+ },
+ putEmailIdentityMailFromAttributes: func(*sesv2.PutEmailIdentityMailFromAttributesInput) (*sesv2.PutEmailIdentityMailFromAttributesOutput, error) {
+ touched = true
+ return &sesv2.PutEmailIdentityMailFromAttributesOutput{}, nil
+ },
+ }
+ p := &awsProvisioner{ses: stub, account: "123456789012", region: "eu-west-1"}
+ if _, err := p.EnsureIdentity(context.Background(), "eu-west-1", "example.com", "mail.example.com", false); !errors.Is(err, ErrIdentityExists) {
+ t.Fatalf("EnsureIdentity(adopt=false) on an existing identity = %v, want ErrIdentityExists", err)
+ }
+ if touched {
+ t.Fatal("an existing identity was read or rewritten without adopt")
+ }
+
+ stub.createEmailIdentity = func(*sesv2.CreateEmailIdentityInput) (*sesv2.CreateEmailIdentityOutput, error) {
+ return &sesv2.CreateEmailIdentityOutput{DkimAttributes: &sesv2types.DkimAttributes{Tokens: []string{"a", "b", "c"}}}, nil
+ }
+ got, err := p.EnsureIdentity(context.Background(), "eu-west-1", "example.com", "mail.example.com", false)
+ if err != nil || !got.Created {
+ t.Fatalf("a fresh create = %+v, %v; want Created", got, err)
+ }
+}
+
// TestEnsureIdentityRefusesNoTokens is the other half of the same path: an
// identity that reads back with no DKIM tokens (BYODKIM, an email-address
// identity, or a nil DkimAttributes) must not be reported as success — the
@@ -181,7 +217,7 @@ func TestEnsureIdentityRefusesNoTokens(t *testing.T) {
}
p := &awsProvisioner{ses: stub, account: "123456789012", region: "eu-west-1"}
- if _, err := p.EnsureIdentity(context.Background(), "eu-west-1", "example.com", "mail.example.com"); err == nil {
+ if _, err := p.EnsureIdentity(context.Background(), "eu-west-1", "example.com", "mail.example.com", true); err == nil {
t.Fatal("EnsureIdentity with zero DKIM tokens returned success, want an error")
}
}
@@ -273,7 +309,7 @@ func TestRegionGuardRefusesAMismatch(t *testing.T) {
return p.EnsureConfigSet(context.Background(), otherRegion, "cfg", "arn:aws:sns:eu-west-1:123456789012:topic")
}},
{"EnsureIdentity", func(p *awsProvisioner) error {
- _, err := p.EnsureIdentity(context.Background(), otherRegion, "example.com", "mail.example.com")
+ _, err := p.EnsureIdentity(context.Background(), otherRegion, "example.com", "mail.example.com", true)
return err
}},
{"IdentityStatus", func(p *awsProvisioner) error {
@@ -315,7 +351,7 @@ func TestRegionGuardPassesAnEmptyRegionThrough(t *testing.T) {
return p.EnsureConfigSet(context.Background(), "", "cfg", "arn:aws:sns:eu-west-1:123456789012:topic")
}},
{"EnsureIdentity", func(p *awsProvisioner) error {
- _, err := p.EnsureIdentity(context.Background(), "", "example.com", "mail.example.com")
+ _, err := p.EnsureIdentity(context.Background(), "", "example.com", "mail.example.com", true)
return err
}},
{"IdentityStatus", func(p *awsProvisioner) error {
diff --git a/internal/mail/domain.go b/internal/mail/domain.go
index b41fe8d8f..67fb72787 100644
--- a/internal/mail/domain.go
+++ b/internal/mail/domain.go
@@ -47,6 +47,11 @@ type RegionState struct {
ConfigSet string `json:"config_set"`
Status string `json:"status"`
CheckedAt time.Time `json:"checked_at"`
+ // Adopted marks an identity this account did not create but proved it
+ // controls the domain of (#216). CARLOS sends on it and never deletes
+ // it. Absent on every record written before it existed, which keeps
+ // those identities torn down as they always were.
+ Adopted bool `json:"adopted,omitempty"`
}
// DomainRecord is one sending domain.
diff --git a/internal/mail/domainclaim.go b/internal/mail/domainclaim.go
index 90cc59987..afeb47928 100644
--- a/internal/mail/domainclaim.go
+++ b/internal/mail/domainclaim.go
@@ -31,6 +31,7 @@ import (
"fmt"
"log"
"os"
+ "strings"
"time"
"github.com/carlosframework/platform/internal/release"
@@ -65,6 +66,28 @@ type DomainClaim struct {
// wrong caller.
App string `json:"app,omitempty"`
CreatedAt time.Time `json:"created_at"`
+ // IdentityRegions are the regions where this claim's own walk CREATED
+ // the SES identity, stamped right after the create (#216). It is the
+ // only record of that for a claim whose domain record was never
+ // written, so it is what lets a re-run keep the identity and lets the
+ // orphan sweeps delete it — and what stops either touching an identity
+ // somebody else made. Omitted on a claim written before it existed;
+ // such a claim's identity is then treated as not CARLOS's to delete.
+ IdentityRegions []string `json:"identity_regions,omitempty"`
+ // VerifiedAt is when VerifyDomain first saw SES verify the domain for
+ // this claim's account. A claim with it set never lapses (ClaimLapse).
+ VerifiedAt *time.Time `json:"verified_at,omitempty"`
+}
+
+// createdIdentityIn reports whether this claim's walk created the domain's
+// identity in region.
+func (c *DomainClaim) createdIdentityIn(region string) bool {
+ for _, r := range c.IdentityRegions {
+ if r == region {
+ return true
+ }
+ }
+ return false
}
// ErrDomainClaimed is returned by EnsureDomain when the domain's global
@@ -74,6 +97,23 @@ type DomainClaim struct {
// holding the claim is never this error; see claimDomain.
var ErrDomainClaimed = errors.New("mail: domain is claimed")
+// ErrIdentityNotOwned is EnsureDomain's refusal of a domain whose SES
+// identity this account's claim did not create — one verified outside
+// CARLOS, or left by a claim that predates the record of who created what —
+// when the account has not proved it controls the domain (#216). Adopting
+// it would hand the account send rights it never proved, rewrite the
+// identity's MAIL FROM and configuration set under its real owner, and let
+// a later teardown delete it.
+var ErrIdentityNotOwned = errors.New("mail: domain has an SES identity this account did not create")
+
+// ClaimLapse is how long a claim may hold a domain without ever being
+// verified (#216). Before it, a claim was first-come and permanent, so any
+// account could hold any domain it would never prove, against every other
+// account, for the life of the deployment. SES itself gives DKIM 72 hours
+// before it marks an identity failed; a week leaves room for a slow DNS
+// change and a weekend. A claim that reaches verified never lapses.
+const ClaimLapse = 7 * 24 * time.Hour
+
// getDomainClaim reads domain's claim. A not-yet-claimed domain returns
// (nil, nil), matching GetDomain's convention rather than propagating
// os.ErrNotExist to every caller.
@@ -116,6 +156,10 @@ func getDomainClaimETag(ctx context.Context, s release.Store, domain string) (*D
// round 2), not who most recently retried it. Any other account gets
// ErrDomainClaimed, with no identity ever minted and no domain record ever
// written — the refusal happens before either exists.
+//
+// First-come is no longer forever: EnsureDomain hands this refusal to
+// takeLapsedClaim, which lets a claim that was never verified go after
+// ClaimLapse (#216).
func claimDomain(ctx context.Context, s release.Store, domain, acct, app string, now time.Time) error {
body, err := json.Marshal(DomainClaim{Schema: 1, Domain: domain, Account: acct, App: app, CreatedAt: now})
if err != nil {
@@ -141,6 +185,195 @@ func claimDomain(ctx context.Context, s release.Store, domain, acct, app string,
}
}
+// updateOwnClaim applies change to domain's claim when acct holds it, under
+// CAS. change reports whether it changed anything; a claim held by anyone
+// else, or none at all, is left alone and is not an error.
+func updateOwnClaim(ctx context.Context, s release.Store, domain, acct string, change func(*DomainClaim) bool) error {
+ for attempt := 0; ; attempt++ {
+ cur, etag, err := getDomainClaimETag(ctx, s, domain)
+ if errors.Is(err, os.ErrNotExist) {
+ return nil
+ }
+ if err != nil {
+ return err
+ }
+ if cur.Account != acct || !change(cur) {
+ return nil
+ }
+ body, err := json.Marshal(cur)
+ if err != nil {
+ return err
+ }
+ if _, err := s.Put(ctx, domainClaimKey(domain), body, etag); err == nil {
+ return nil
+ } else if !errors.Is(err, release.ErrCASConflict) || attempt >= 2 {
+ return err
+ }
+ }
+}
+
+// stampClaimIdentity records on acct's claim that its walk created the
+// identity in region.
+func stampClaimIdentity(ctx context.Context, s release.Store, domain, acct, region string) error {
+ return updateOwnClaim(ctx, s, domain, acct, func(c *DomainClaim) bool {
+ if c.createdIdentityIn(region) {
+ return false
+ }
+ c.IdentityRegions = append(c.IdentityRegions, region)
+ return true
+ })
+}
+
+// stampClaimVerified records the first time acct's claim was verified.
+func stampClaimVerified(ctx context.Context, s release.Store, domain, acct string, now time.Time) error {
+ return updateOwnClaim(ctx, s, domain, acct, func(c *DomainClaim) bool {
+ if c.VerifiedAt != nil {
+ return false
+ }
+ at := now.UTC()
+ c.VerifiedAt = &at
+ return true
+ })
+}
+
+// claimLapsed reports whether c, held by another account, has lapsed: older
+// than ClaimLapse and never verified by any of three witnesses — the claim's
+// own stamp, the holder's domain record, and SES itself. SES is asked
+// because a holder may have published its DNS and never run verify; such a
+// domain is in use, and handing it on would also hand on a verified
+// identity.
+func claimLapsed(ctx context.Context, s release.Store, p Provisioner, c *DomainClaim, region string, now time.Time) (bool, error) {
+ if c.VerifiedAt != nil || now.Sub(c.CreatedAt) < ClaimLapse {
+ return false, nil
+ }
+ rec, _, err := GetDomain(ctx, s, c.Account, c.Domain)
+ if err != nil {
+ return false, err
+ }
+ if rec != nil {
+ for _, st := range rec.Regions {
+ if st != nil && st.Status == StatusVerified {
+ return false, nil
+ }
+ }
+ }
+ got, err := p.IdentityStatus(ctx, region, c.Domain)
+ if err != nil {
+ return false, err
+ }
+ if got.DKIM == "SUCCESS" || got.Sending == "SUCCESS" {
+ return false, nil
+ }
+ return true, nil
+}
+
+// takeLapsedClaim is EnsureDomain's answer to ErrDomainClaimed: when the
+// other account's claim has lapsed, acct takes it over; otherwise the
+// refusal stands.
+//
+// The claim moves FIRST, under CAS against the etag the lapse was judged
+// on. A holder that verifies in the meantime stamps verified_at, which moves
+// the etag, so the CAS fails and the holder keeps everything. Once the claim
+// is acct's, the holder's VerifyDomain and teardown both refuse to act on
+// the domain, so its record can no longer verify into a grant.
+//
+// Only then does the lapsed holder's own half-made identity go — and only
+// when its claim says its walk created it, and SES, asked again right
+// before the delete, still has not verified it. Left in place, the new
+// holder would meet an identity it did not create and have to prove DNS
+// control for a domain whose only obstacle was an abandoned attempt. An
+// identity the lapsed claim did not create, or one SES has since verified,
+// is not touched: the new holder meets it as a foreign identity and needs
+// proof. A crash between the CAS and the delete leaves the same state.
+func takeLapsedClaim(ctx context.Context, s release.Store, p Provisioner, region, domain, acct, app string, now time.Time) error {
+ cur, etag, err := getDomainClaimETag(ctx, s, domain)
+ if errors.Is(err, os.ErrNotExist) {
+ return claimDomain(ctx, s, domain, acct, app, now) // released meanwhile
+ }
+ if err != nil {
+ return err
+ }
+ if cur.Account == acct {
+ return nil
+ }
+ lapsed, err := claimLapsed(ctx, s, p, cur, region, now)
+ if err != nil {
+ return err
+ }
+ if !lapsed {
+ return ErrDomainClaimed
+ }
+ body, err := json.Marshal(DomainClaim{Schema: 1, Domain: domain, Account: acct, App: app, CreatedAt: now})
+ if err != nil {
+ return err
+ }
+ if _, err := s.Put(ctx, domainClaimKey(domain), body, etag); err != nil {
+ if errors.Is(err, release.ErrCASConflict) {
+ return ErrDomainClaimed // the holder or another claimant moved first
+ }
+ return err
+ }
+ log.Printf("mail: claim on %s lapsed unverified (held since %s); taken by another account", domain, cur.CreatedAt.Format(time.RFC3339))
+
+ if !cur.createdIdentityIn(region) {
+ return nil
+ }
+ if st, err := p.IdentityStatus(ctx, region, domain); err != nil {
+ log.Printf("mail: lapsed claim %s: identity status: %v; leaving the old identity", domain, err)
+ return nil
+ } else if st.DKIM == "SUCCESS" || st.Sending == "SUCCESS" {
+ return nil
+ }
+ if old, _, err := GetDomain(ctx, s, cur.Account, domain); err == nil && old != nil &&
+ old.Regions[region] != nil && old.Regions[region].IdentityARN != "" {
+ // SES refuses to delete an associated identity.
+ if err := p.DisassociateTenantResource(ctx, TenantName(cur.Account), old.Regions[region].IdentityARN); err != nil {
+ log.Printf("mail: lapsed claim %s: disassociating: %v", domain, err)
+ }
+ }
+ // A failed delete is logged, not returned: the claim is already acct's,
+ // and the walk goes on to meet the identity as a foreign one.
+ if err := p.DeleteIdentity(ctx, region, domain); err != nil {
+ log.Printf("mail: lapsed claim %s: delete old identity: %v", domain, err)
+ }
+ return nil
+}
+
+// TXTResolver is the one DNS question the walk asks: the proof record for a
+// domain whose SES identity this account did not create. net.Resolver
+// satisfies it; tests inject a fake.
+type TXTResolver interface {
+ LookupTXT(ctx context.Context, name string) ([]string, error)
+}
+
+// ProofRecordName and ProofRecordValue are the TXT record a claimant
+// publishes to prove it controls a domain that already has an SES identity
+// (#216). The value names the account, so one account's proof is no use to
+// another; only somebody who can write the domain's DNS can publish it.
+func ProofRecordName(domain string) string { return "_carlos-mail." + domain }
+
+func ProofRecordValue(acct string) string { return "carlos-mail-account=" + acct }
+
+// provesControl reports whether domain publishes acct's proof record. A nil
+// resolver proves nothing, so a deployment that never wired one refuses
+// rather than adopts.
+func provesControl(ctx context.Context, r TXTResolver, domain, acct string) bool {
+ if r == nil {
+ return false
+ }
+ vals, err := r.LookupTXT(ctx, ProofRecordName(domain))
+ if err != nil {
+ return false
+ }
+ want := ProofRecordValue(acct)
+ for _, v := range vals {
+ if strings.TrimSpace(v) == want {
+ return true
+ }
+ }
+ return false
+}
+
// releaseDomainClaim removes domain's claim, but only when it is held by
// acct — the same owner-gated neutrality domains.DetachClaim observes, so
// this can never be used to discover or disturb another account's claim.
diff --git a/internal/mail/memprovisioner.go b/internal/mail/memprovisioner.go
index 5d1f2480f..b144a8dd1 100644
--- a/internal/mail/memprovisioner.go
+++ b/internal/mail/memprovisioner.go
@@ -111,8 +111,9 @@ func (p *MemProvisioner) EnsureConfigSet(_ context.Context, region, name, topic
// EnsureIdentity is idempotent AND token-stable: a second call returns the
// identity already there rather than minting new tokens, because new tokens
-// would orphan every DKIM CNAME already published.
-func (p *MemProvisioner) EnsureIdentity(_ context.Context, region, domain, mailFrom string) (Identity, error) {
+// would orphan every DKIM CNAME already published. With adopt false an
+// existing identity is refused untouched, as the real backend does.
+func (p *MemProvisioner) EnsureIdentity(_ context.Context, region, domain, mailFrom string, adopt bool) (Identity, error) {
p.mu.Lock()
defer p.mu.Unlock()
if err := p.takeFailure("EnsureIdentity"); err != nil {
@@ -120,6 +121,9 @@ func (p *MemProvisioner) EnsureIdentity(_ context.Context, region, domain, mailF
}
k := identityKey(region, domain)
if got, ok := p.Identities[k]; ok {
+ if !adopt {
+ return Identity{}, fmt.Errorf("mail: identity %q in %s: %w", domain, region, ErrIdentityExists)
+ }
got.MailFrom = mailFrom
return Identity{ARN: got.ARN, DKIMTokens: got.DKIMTokens}, nil
}
@@ -135,7 +139,7 @@ func (p *MemProvisioner) EnsureIdentity(_ context.Context, region, domain, mailF
Status: p.StatusOnEnsure,
}
p.Identities[k] = id
- return Identity{ARN: id.ARN, DKIMTokens: id.DKIMTokens}, nil
+ return Identity{ARN: id.ARN, DKIMTokens: id.DKIMTokens, Created: true}, nil
}
// SetIdentityConfigSet models PutEmailIdentityConfigurationSetAttributes:
diff --git a/internal/mail/memprovisioner_test.go b/internal/mail/memprovisioner_test.go
index db81baf13..de3b47156 100644
--- a/internal/mail/memprovisioner_test.go
+++ b/internal/mail/memprovisioner_test.go
@@ -11,14 +11,14 @@ import (
// the SAME tokens — fresh tokens would orphan the DNS already published.
func TestMemEnsureIdentityIsIdempotentAndTokenStable(t *testing.T) {
ctx, p := context.Background(), NewMemProvisioner("1")
- first, err := p.EnsureIdentity(ctx, "eu-west-1", "x.com", "mail-eu-west-1.x.com")
+ first, err := p.EnsureIdentity(ctx, "eu-west-1", "x.com", "mail-eu-west-1.x.com", true)
if err != nil {
t.Fatalf("EnsureIdentity: %v", err)
}
if len(first.DKIMTokens) != 3 {
t.Fatalf("want 3 DKIM tokens, got %d", len(first.DKIMTokens))
}
- second, err := p.EnsureIdentity(ctx, "eu-west-1", "x.com", "mail-eu-west-1.x.com")
+ second, err := p.EnsureIdentity(ctx, "eu-west-1", "x.com", "mail-eu-west-1.x.com", true)
if err != nil {
t.Fatalf("re-EnsureIdentity: %v", err)
}
@@ -27,10 +27,30 @@ func TestMemEnsureIdentityIsIdempotentAndTokenStable(t *testing.T) {
}
}
+// Without adopt, the fake refuses an existing identity untouched, as SES
+// does through awsProvisioner (#216).
+func TestMemEnsureIdentityWithoutAdoptRefusesUntouched(t *testing.T) {
+ ctx, p := context.Background(), NewMemProvisioner("1")
+ first, err := p.EnsureIdentity(ctx, "eu-west-1", "x.com", "m1.x.com", false)
+ if err != nil || !first.Created {
+ t.Fatalf("create = %+v, %v", first, err)
+ }
+ if _, err := p.EnsureIdentity(ctx, "eu-west-1", "x.com", "m2.x.com", false); !errors.Is(err, ErrIdentityExists) {
+ t.Fatalf("re-ensure without adopt = %v, want ErrIdentityExists", err)
+ }
+ if got := p.Identities[identityKey("eu-west-1", "x.com")].MailFrom; got != "m1.x.com" {
+ t.Fatalf("MAIL FROM rewritten to %q without adopt", got)
+ }
+ again, err := p.EnsureIdentity(ctx, "eu-west-1", "x.com", "m1.x.com", true)
+ if err != nil || again.Created {
+ t.Fatalf("adopt = %+v, %v; want the existing identity, not Created", again, err)
+ }
+}
+
func TestMemIdentitiesAreRegionScoped(t *testing.T) {
ctx, p := context.Background(), NewMemProvisioner("1")
- eu, _ := p.EnsureIdentity(ctx, "eu-west-1", "x.com", "mail-eu-west-1.x.com")
- us, _ := p.EnsureIdentity(ctx, "us-east-1", "x.com", "mail-us-east-1.x.com")
+ eu, _ := p.EnsureIdentity(ctx, "eu-west-1", "x.com", "mail-eu-west-1.x.com", true)
+ us, _ := p.EnsureIdentity(ctx, "us-east-1", "x.com", "mail-us-east-1.x.com", true)
if eu.ARN == us.ARN {
t.Fatal("one ARN served two regions")
}
@@ -124,10 +144,10 @@ func TestMemFailNextIsOneShot(t *testing.T) {
ctx, p := context.Background(), NewMemProvisioner("1")
boom := errors.New("boom")
p.FailNext["EnsureIdentity"] = boom
- if _, err := p.EnsureIdentity(ctx, "eu-west-1", "x.com", "m.x.com"); !errors.Is(err, boom) {
+ if _, err := p.EnsureIdentity(ctx, "eu-west-1", "x.com", "m.x.com", true); !errors.Is(err, boom) {
t.Fatalf("first call = %v, want boom", err)
}
- if _, err := p.EnsureIdentity(ctx, "eu-west-1", "x.com", "m.x.com"); err != nil {
+ if _, err := p.EnsureIdentity(ctx, "eu-west-1", "x.com", "m.x.com", true); err != nil {
t.Fatalf("second call = %v, want nil", err)
}
}
@@ -138,7 +158,7 @@ func TestMemFailNextIsOneShot(t *testing.T) {
func TestMemProvisionerRefusesToDeleteAnAssociatedIdentity(t *testing.T) {
ctx := context.Background()
p := NewMemProvisioner("1")
- id, err := p.EnsureIdentity(ctx, "eu-west-1", "x.com", "mail-eu-west-1.x.com")
+ id, err := p.EnsureIdentity(ctx, "eu-west-1", "x.com", "mail-eu-west-1.x.com", true)
if err != nil {
t.Fatal(err)
}
diff --git a/internal/mail/provisioner.go b/internal/mail/provisioner.go
index 1b2d0bf00..3df1f2440 100644
--- a/internal/mail/provisioner.go
+++ b/internal/mail/provisioner.go
@@ -8,7 +8,10 @@
// by hand.
package mail
-import "context"
+import (
+ "context"
+ "errors"
+)
// Identity is what EnsureIdentity answers: the ARN a policy names and the
// DKIM tokens whose CNAMEs must be published. The tokens are STABLE for the
@@ -18,8 +21,17 @@ import "context"
type Identity struct {
ARN string
DKIMTokens []string
+ // Created is true when this call made the identity, false when it was
+ // already there. The walk records it, because an identity CARLOS did not
+ // create is not CARLOS's to delete (discussion #216).
+ Created bool
}
+// ErrIdentityExists is EnsureIdentity's refusal, with adopt false, of a
+// domain that already has an identity. Nothing about the identity has been
+// read or changed when it is returned.
+var ErrIdentityExists = errors.New("mail: identity already exists")
+
// Status is SES's own verdict on an identity. All three must read
// "SUCCESS" before the identity is usable; a domain that is verified for
// sending but whose MAIL FROM has not converged still sends, from the
@@ -79,7 +91,13 @@ type Provisioner interface {
// EnsureIdentity creates the domain identity, enables Easy DKIM and
// sets the custom MAIL FROM subdomain, all idempotently.
- EnsureIdentity(ctx context.Context, region, domain, mailFrom string) (Identity, error)
+ //
+ // adopt says whether an identity that already exists may be taken over:
+ // its DKIM tokens read back and its MAIL FROM rewritten. With adopt
+ // false, an existing identity is refused with ErrIdentityExists and left
+ // exactly as it was — the walk passes false unless the identity is one
+ // this account already holds or has proved it controls (#216).
+ EnsureIdentity(ctx context.Context, region, domain, mailFrom string, adopt bool) (Identity, error)
IdentityStatus(ctx context.Context, region, domain string) (Status, error)
DeleteIdentity(ctx context.Context, region, domain string) error
diff --git a/internal/mail/walk.go b/internal/mail/walk.go
index a139ea999..13a19511b 100644
--- a/internal/mail/walk.go
+++ b/internal/mail/walk.go
@@ -63,6 +63,10 @@ type Deployment struct {
PlatformSuffix string // e.g. "oncarlos.com" — the zone CARLOS owns
DMARCRua string
SNSTopicARN string
+ // Resolver answers the proof-of-control lookup for a domain whose SES
+ // identity this account did not create (#216). nil means no proof can
+ // be checked, so such a domain is always refused.
+ Resolver TXTResolver
}
// DomainKindFor decides who publishes DNS for a domain.
@@ -181,7 +185,11 @@ func EnsureDomain(ctx context.Context, s release.Store, p Provisioner, w dnsrec.
if DomainKindFor(d, domain) == KindPlatform && !platformNameOwnedBy(d, domain, acct) {
return nil, nil, fmt.Errorf("mail: %q is not %s's platform name: %w", domain, acct, ErrNotOwnPlatformName)
}
- if err := claimDomain(ctx, s, domain, acct, app, now); err != nil {
+ err := claimDomain(ctx, s, domain, acct, app, now)
+ if errors.Is(err, ErrDomainClaimed) {
+ err = takeLapsedClaim(ctx, s, p, d.Region, domain, acct, app, now)
+ }
+ if err != nil {
// Wrapping preserves errors.Is(err, ErrDomainClaimed) for callers
// (the console door translates it into its own neutral refusal) —
// %w keeps the chain — and the added context is just the domain
@@ -200,6 +208,7 @@ func EnsureDomain(ctx context.Context, s release.Store, p Provisioner, w dnsrec.
if err != nil {
return nil, nil, err
}
+ hadRecord := rec != nil
if rec == nil {
rec = &DomainRecord{
Domain: domain, Kind: DomainKindFor(d, domain),
@@ -211,11 +220,59 @@ func EnsureDomain(ctx context.Context, s release.Store, p Provisioner, w dnsrec.
}
rec.AddApp(app)
+ // WHOSE IDENTITY IS IT (#216). SES identities are per AWS account, and
+ // this one is shared with every other product in it, so "already
+ // exists" used to mean "adopt": read back its DKIM tokens, rewrite its
+ // MAIL FROM, bind it to this account's configuration set, and delete it
+ // at teardown. A member could do that to tito.io, or to the operator's
+ // own sign-in domain, and get send rights for it the moment verify read
+ // SES's existing verdict.
+ //
+ // So an existing identity is this account's to keep only when CARLOS
+ // already knows it is: the account's record names it (created by an
+ // earlier run, by a sibling app, or adopted with proof before), or the
+ // claim says this account's walk created it and the record was never
+ // written. Anything else needs proof of DNS control, and is recorded as
+ // Adopted so nothing CARLOS does later deletes it.
mailFrom := MailFromDomain(domain, d.Region)
- ident, err := p.EnsureIdentity(ctx, d.Region, domain, mailFrom)
+ prev := rec.Regions[d.Region]
+ owned := prev != nil && prev.IdentityARN != ""
+ adopted := owned && prev.Adopted
+ if !owned {
+ claim, err := getDomainClaim(ctx, s, domain)
+ if err != nil {
+ return nil, nil, err
+ }
+ owned = claim != nil && claim.Account == acct && claim.createdIdentityIn(d.Region)
+ }
+ ident, err := p.EnsureIdentity(ctx, d.Region, domain, mailFrom, owned)
+ if errors.Is(err, ErrIdentityExists) {
+ if !provesControl(ctx, d.Resolver, domain, acct) {
+ // Nothing of this account's stands behind a claim with no
+ // record: let it go, so a refusal never blocks the domain's
+ // real owner, and no orphan sweep is ever pointed at it.
+ if !hadRecord {
+ if rerr := releaseDomainClaim(ctx, s, domain, acct); rerr != nil {
+ log.Printf("mail: releasing claim on refused %s: %v", domain, rerr)
+ }
+ }
+ return nil, nil, fmt.Errorf("mail: %s in %s: %w", domain, d.Region, ErrIdentityNotOwned)
+ }
+ adopted = true
+ ident, err = p.EnsureIdentity(ctx, d.Region, domain, mailFrom, true)
+ }
if err != nil {
return nil, nil, fmt.Errorf("mail: ensure identity %s in %s: %w", domain, d.Region, err)
}
+ if ident.Created {
+ // Stamped before anything else can fail, so a crash from here on
+ // leaves a claim that says this identity is CARLOS's: the re-run
+ // keeps it, and the orphan sweeps may delete it.
+ adopted = false
+ if err := stampClaimIdentity(ctx, s, domain, acct, d.Region); err != nil {
+ return nil, nil, fmt.Errorf("mail: record identity for %s: %w", domain, err)
+ }
+ }
// configSet stays empty when no SNS topic is configured — recording a
// name for a set that was never created would leave later sends
@@ -288,7 +345,7 @@ func EnsureDomain(ctx context.Context, s release.Store, p Provisioner, w dnsrec.
rec.Regions[d.Region] = &RegionState{
IdentityARN: ident.ARN, DKIMTokens: ident.DKIMTokens,
MailFrom: mailFrom, ConfigSet: configSet,
- Status: status, CheckedAt: now,
+ Status: status, CheckedAt: now, Adopted: adopted,
}
if _, err := PutDomain(ctx, s, acct, rec, etag); err != nil {
return nil, nil, err
@@ -439,6 +496,29 @@ func VerifyDomain(ctx context.Context, s release.Store, p Provisioner, d Deploym
return nil, fmt.Errorf("mail: domain %s has no identity in %s", domain, region)
}
+ // A record whose claim another account now holds — it lapsed unverified
+ // and was taken (#216) — must not verify into a grant on what is now
+ // somebody else's identity. It reads failed, and SES is not asked. No
+ // claim at all is a record from before the claim gate, and verifies as
+ // it always did.
+ claim, err := getDomainClaim(ctx, s, domain)
+ if err != nil {
+ return nil, err
+ }
+ if claim != nil && claim.Account != acct {
+ wasVerified := st.Status == StatusVerified
+ st.Status, st.CheckedAt = StatusFailed, now
+ if _, err := PutDomain(ctx, s, acct, rec, etag); err != nil {
+ return nil, err
+ }
+ if wasVerified {
+ if err := RefreshPolicies(ctx, s, p, acct, rec.Apps); err != nil {
+ return nil, err
+ }
+ }
+ return nil, fmt.Errorf("mail: verify %s: %w", domain, ErrDomainClaimed)
+ }
+
got, err := p.IdentityStatus(ctx, region, domain)
if err != nil {
return nil, fmt.Errorf("mail: identity status for %s in %s: %w", domain, region, err)
@@ -488,6 +568,14 @@ func VerifyDomain(ctx context.Context, s release.Store, p Provisioner, d Deploym
if _, err := PutDomain(ctx, s, acct, rec, etag); err != nil {
return nil, err
}
+ // Once verified, the claim never lapses. A failed stamp is only logged:
+ // the record's own verified status protects the claim just the same
+ // until the next verify retries it.
+ if st.Status == StatusVerified && claim != nil && claim.VerifiedAt == nil {
+ if err := stampClaimVerified(ctx, s, domain, acct, now); err != nil {
+ log.Printf("mail: stamping verified on claim %s: %v", domain, err)
+ }
+ }
// Grant after rebind, for #279's reason seen from the other side: a
// repair that fixes SES and the record but leaves the IAM policy naming
@@ -1247,8 +1335,34 @@ func detachDomainApp(ctx context.Context, s release.Store, p Provisioner, w dnsr
// 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.
+ //
+ // Except where the identity is not this account's to remove (#216): one
+ // it adopted with proof, or one whose claim lapsed and now belongs to
+ // another account. Those keep their identity and their DNS; only this
+ // account's tenant association goes, so the owner can still delete the
+ // identity later.
+ //
+ // The identity itself is deleted only when the claim says this
+ // account's walk CREATED it, the same rule the orphan sweeps use. A
+ // record written before that stamp existed may have adopted somebody
+ // else's identity (the #216 attack itself, or an operator domain
+ // shared with another product), and nothing on it says which; such an
+ // identity is left in SES. Leaking it is the safe direction.
+ claim, err := getDomainClaim(ctx, s, cur.Domain)
+ if err != nil {
+ return detachNone, nil, err
+ }
+ ours := claim == nil || claim.Account == acct
var orphaned []dnsrec.Record
for region, st := range cur.Regions {
+ if !ours || st.Adopted {
+ 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)
+ }
+ }
+ continue
+ }
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
@@ -1291,8 +1405,10 @@ func detachDomainApp(ctx context.Context, s release.Store, p Provisioner, w dnsr
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
+ if claim != nil && claim.createdIdentityIn(region) {
+ 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
@@ -1459,8 +1575,14 @@ func Teardown(ctx context.Context, s release.Store, p Provisioner, w dnsrec.Writ
// fleet-wide), so that is the only region this needs to check.
// DeleteIdentity is idempotent — "already gone" is success — so
// this is free when no identity was ever actually created.
- if err := p.DeleteIdentity(ctx, d.Region, c.Domain); err != nil {
- return err
+ //
+ // Only an identity the claim says this walk CREATED (#216). One it
+ // found already there was never adopted (EnsureDomain refuses and
+ // releases such a claim), so it is somebody else's.
+ if c.createdIdentityIn(d.Region) {
+ if err := p.DeleteIdentity(ctx, d.Region, c.Domain); err != nil {
+ return err
+ }
}
if err := releaseDomainClaim(ctx, s, c.Domain, acct); err != nil {
return err
@@ -1653,8 +1775,11 @@ func removeOrphanedClaim(ctx context.Context, s release.Store, p Provisioner,
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
+ // Only an identity this claim's walk created, as in Teardown's sweep.
+ if claim.createdIdentityIn(d.Region) {
+ 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
diff --git a/internal/mail/walk_identityowner_test.go b/internal/mail/walk_identityowner_test.go
new file mode 100644
index 000000000..353272360
--- /dev/null
+++ b/internal/mail/walk_identityowner_test.go
@@ -0,0 +1,332 @@
+// Discussion #216: a member must not be able to take over an SES identity
+// this account's claim did not create, and a sending-domain claim that was
+// never verified must not hold a domain forever.
+package mail
+
+import (
+ "context"
+ "errors"
+ "testing"
+ "time"
+
+ "github.com/carlosframework/platform/internal/dnsrec"
+ "github.com/carlosframework/platform/internal/release"
+)
+
+// seedForeignIdentity stands in an identity verified in SES outside CARLOS:
+// tito.io in spec 2026-09-15 §1, an operator's own sign-in domain, or one
+// created before the claim gate existed.
+func seedForeignIdentity(p *MemProvisioner, region, domain string) *MemIdentity {
+ id := &MemIdentity{
+ ARN: "arn:aws:ses:" + region + ":shared:identity/" + domain,
+ DKIMTokens: []string{"classic1", "classic2", "classic3"},
+ MailFrom: "bounce." + domain,
+ ConfigSet: "classic-events",
+ Status: Status{DKIM: "SUCCESS", MailFrom: "SUCCESS", Sending: "SUCCESS"},
+ }
+ p.Identities[identityKey(region, domain)] = id
+ return id
+}
+
+// The HIGH finding as it stood: a member's `domains add` on a domain someone
+// else verified in SES adopted the identity, rewrote its MAIL FROM, and
+// would have bound and later deleted it.
+func TestEnsureDomainRefusesAnIdentityItDidNotCreate(t *testing.T) {
+ ctx := context.Background()
+ s, w := release.NewDirStore(t.TempDir()), dnsrec.NewMemWriter()
+ p := NewMemProvisioner("shared")
+ now := time.Date(2026, 10, 3, 12, 0, 0, 0, time.UTC)
+ d := testDeployment()
+ foreign := seedForeignIdentity(p, d.Region, "tito.io")
+
+ _, _, err := EnsureDomain(ctx, s, p, w, d, "aaa", "jelly", "tito.io", now)
+ if !errors.Is(err, ErrIdentityNotOwned) {
+ t.Fatalf("EnsureDomain on a foreign identity = %v, want ErrIdentityNotOwned", err)
+ }
+ if foreign.MailFrom != "bounce.tito.io" || foreign.ConfigSet != "classic-events" {
+ t.Fatalf("the foreign identity was mutated: MailFrom=%q ConfigSet=%q", foreign.MailFrom, foreign.ConfigSet)
+ }
+ if rec, _, err := GetDomain(ctx, s, "aaa", "tito.io"); err != nil || rec != nil {
+ t.Fatalf("a domain record was written for a refused domain: %+v %v", rec, err)
+ }
+ // The refusal leaves nothing holding the domain: no claim to block the
+ // real owner, and nothing a later Teardown could delete.
+ if c, err := getDomainClaim(ctx, s, "tito.io"); err != nil || c != nil {
+ t.Fatalf("a refused domain kept its claim: %+v %v", c, err)
+ }
+ if err := Teardown(ctx, s, p, w, d, "aaa", "jelly"); err != nil {
+ t.Fatalf("teardown: %v", err)
+ }
+ if _, ok := p.Identities[identityKey(d.Region, "tito.io")]; !ok {
+ t.Fatal("teardown deleted an identity this account never created")
+ }
+}
+
+// fakeTXT is the injectable resolver: name -> TXT values.
+type fakeTXT map[string][]string
+
+func (f fakeTXT) LookupTXT(_ context.Context, name string) ([]string, error) {
+ if v, ok := f[name]; ok {
+ return v, nil
+ }
+ return nil, errors.New("no such host")
+}
+
+// Proof of DNS control is the one way in: the claimant publishes a TXT
+// record naming its own account, and only then is the existing identity
+// adopted. Teardown still never deletes it, because CARLOS did not create
+// it.
+func TestEnsureDomainAdoptsAForeignIdentityOnlyWithDNSProof(t *testing.T) {
+ ctx := context.Background()
+ s, w := release.NewDirStore(t.TempDir()), dnsrec.NewMemWriter()
+ p := NewMemProvisioner("shared")
+ now := time.Date(2026, 10, 3, 12, 0, 0, 0, time.UTC)
+ d := testDeployment()
+ seedForeignIdentity(p, d.Region, "tito.io")
+
+ // A proof naming a DIFFERENT account proves nothing for this one.
+ d.Resolver = fakeTXT{ProofRecordName("tito.io"): {ProofRecordValue("bbb")}}
+ if _, _, err := EnsureDomain(ctx, s, p, w, d, "aaa", "jelly", "tito.io", now); !errors.Is(err, ErrIdentityNotOwned) {
+ t.Fatalf("another account's proof was accepted: %v", err)
+ }
+
+ d.Resolver = fakeTXT{ProofRecordName("tito.io"): {"v=spf1 -all", ProofRecordValue("aaa")}}
+ rec, _, err := EnsureDomain(ctx, s, p, w, d, "aaa", "jelly", "tito.io", now)
+ if err != nil {
+ t.Fatalf("EnsureDomain with a valid proof: %v", err)
+ }
+ st := rec.Regions[d.Region]
+ if !st.Adopted {
+ t.Fatal("an adopted identity was not recorded as adopted")
+ }
+ if st.DKIMTokens[0] != "classic1" {
+ t.Fatalf("adoption did not keep the identity's published DKIM tokens: %v", st.DKIMTokens)
+ }
+
+ // A re-run needs no proof any more: the record is this account's.
+ d.Resolver = nil
+ again, _, err := EnsureDomain(ctx, s, p, w, d, "aaa", "jelly", "tito.io", now)
+ if err != nil {
+ t.Fatalf("re-run after adoption: %v", err)
+ }
+ if got := again.Regions[d.Region]; !got.Adopted {
+ t.Fatal("re-run lost the adopted flag")
+ }
+
+ if err := Teardown(ctx, s, p, w, d, "aaa", "jelly"); err != nil {
+ t.Fatalf("teardown: %v", err)
+ }
+ if _, ok := p.Identities[identityKey(d.Region, "tito.io")]; !ok {
+ t.Fatal("teardown deleted an adopted identity")
+ }
+ if c, _ := getDomainClaim(ctx, s, "tito.io"); c != nil {
+ t.Fatalf("teardown left the claim behind: %+v", c)
+ }
+}
+
+// Identities CARLOS itself created for this account keep working with no
+// proof: a re-run, a sibling app, and the crash between EnsureIdentity and
+// the record (the claim remembers the create).
+func TestEnsureDomainKeepsItsOwnIdentitiesWithoutProof(t *testing.T) {
+ ctx := context.Background()
+ s, w := release.NewDirStore(t.TempDir()), dnsrec.NewMemWriter()
+ p := NewMemProvisioner("shared")
+ now := time.Date(2026, 10, 3, 12, 0, 0, 0, time.UTC)
+ d := testDeployment()
+
+ p.FailNext["EnsureConfigSet"] = errors.New("throttle")
+ if _, _, err := EnsureDomain(ctx, s, p, w, d, "aaa", "app1", "mine.com", now); err == nil {
+ t.Fatal("expected the injected failure")
+ }
+ rec, _, err := EnsureDomain(ctx, s, p, w, d, "aaa", "app1", "mine.com", now)
+ if err != nil {
+ t.Fatalf("re-run after a crash past EnsureIdentity: %v", err)
+ }
+ if rec.Regions[d.Region].Adopted {
+ t.Fatal("an identity CARLOS created was recorded as adopted")
+ }
+ if _, _, err := EnsureDomain(ctx, s, p, w, d, "aaa", "app2", "mine.com", now); err != nil {
+ t.Fatalf("sibling app: %v", err)
+ }
+}
+
+// A record written before this change (no claim stamp, no Adopted field)
+// still re-ensures and verifies. Teardown no longer deletes its identity:
+// nothing says CARLOS created it, so it is left in SES.
+func TestEnsureDomainHonoursALegacyRecord(t *testing.T) {
+ ctx := context.Background()
+ s, w := release.NewDirStore(t.TempDir()), dnsrec.NewMemWriter()
+ p := NewMemProvisioner("shared")
+ now := time.Date(2026, 10, 3, 12, 0, 0, 0, time.UTC)
+ d := testDeployment()
+ id := seedForeignIdentity(p, d.Region, "old.com") // created by a pre-gate walk
+ legacy := &DomainRecord{Domain: "old.com", Kind: KindCustom, Apps: []string{"app1"}, CreatedAt: now,
+ Regions: map[string]*RegionState{d.Region: {IdentityARN: id.ARN, DKIMTokens: id.DKIMTokens, Status: StatusVerified}}}
+ if _, err := PutDomain(ctx, s, "aaa", legacy, ""); err != nil {
+ t.Fatal(err)
+ }
+ if _, _, err := EnsureDomain(ctx, s, p, w, d, "aaa", "app1", "old.com", now); err != nil {
+ t.Fatalf("legacy re-ensure: %v", err)
+ }
+ if _, err := VerifyDomain(ctx, s, p, d, "aaa", "old.com", d.Region, now); err != nil {
+ t.Fatalf("legacy verify: %v", err)
+ }
+ if err := Teardown(ctx, s, p, w, d, "aaa", "app1"); err != nil {
+ t.Fatalf("teardown: %v", err)
+ }
+ if _, ok := p.Identities[identityKey(d.Region, "old.com")]; !ok {
+ t.Fatal("teardown deleted a legacy record's identity that nothing says CARLOS created")
+ }
+ if c, _ := getDomainClaim(ctx, s, "old.com"); c != nil {
+ t.Fatalf("teardown left the claim behind: %+v", c)
+ }
+}
+
+// The reviewer's case: on main, the #216 attack itself left a record that
+// adopted a foreign identity, with a claim and no stamp. `domains rm` after
+// this change must not delete the real owner's identity.
+func TestRemoveDomainSparesALegacyAdoptedIdentity(t *testing.T) {
+ ctx := context.Background()
+ s, w := release.NewDirStore(t.TempDir()), dnsrec.NewMemWriter()
+ p := NewMemProvisioner("shared")
+ now := time.Date(2026, 10, 3, 12, 0, 0, 0, time.UTC)
+ d := testDeployment()
+ id := seedForeignIdentity(p, d.Region, "tito.io")
+ if err := claimDomain(ctx, s, "tito.io", "aaa", "jelly", now); err != nil {
+ t.Fatal(err)
+ }
+ legacy := &DomainRecord{Domain: "tito.io", Kind: KindCustom, Apps: []string{"jelly"}, CreatedAt: now,
+ Regions: map[string]*RegionState{d.Region: {IdentityARN: id.ARN, DKIMTokens: id.DKIMTokens, Status: StatusVerified}}}
+ if _, err := PutDomain(ctx, s, "aaa", legacy, ""); err != nil {
+ t.Fatal(err)
+ }
+ if _, err := RemoveDomain(ctx, s, p, w, d, "aaa", "jelly", "tito.io"); err != nil {
+ t.Fatalf("RemoveDomain: %v", err)
+ }
+ if _, ok := p.Identities[identityKey(d.Region, "tito.io")]; !ok {
+ t.Fatal("domains rm deleted an identity a pre-fix record had adopted")
+ }
+ if c, _ := getDomainClaim(ctx, s, "tito.io"); c != nil {
+ t.Fatalf("the claim was not released: %+v", c)
+ }
+}
+
+// A holder that verifies between the lapse check and the takeover keeps
+// its claim and its identity: the verified_at stamp moves the etag.
+func TestLapsedTakeoverLosesToAHolderThatVerifies(t *testing.T) {
+ ctx := context.Background()
+ s, w := release.NewDirStore(t.TempDir()), dnsrec.NewMemWriter()
+ p := NewMemProvisioner("shared")
+ now := time.Date(2026, 10, 3, 12, 0, 0, 0, time.UTC)
+ d := testDeployment()
+ if _, _, err := EnsureDomain(ctx, s, p, w, d, "aaa", "app", "race.com", now); err != nil {
+ t.Fatal(err)
+ }
+ later := now.Add(ClaimLapse + time.Hour)
+ // Judge the lapse on the current etag, then let the holder verify.
+ cur, etag, err := getDomainClaimETag(ctx, s, "race.com")
+ if err != nil {
+ t.Fatal(err)
+ }
+ if lapsed, err := claimLapsed(ctx, s, p, cur, d.Region, later); err != nil || !lapsed {
+ t.Fatalf("setup: claim should read lapsed: %v %v", lapsed, err)
+ }
+ p.SetIdentityStatus(d.Region, "race.com", Status{DKIM: "SUCCESS", MailFrom: "SUCCESS", Sending: "SUCCESS"})
+ if _, err := VerifyDomain(ctx, s, p, d, "aaa", "race.com", d.Region, later); err != nil {
+ t.Fatal(err)
+ }
+ body := []byte(`{"schema":1,"domain":"race.com","account":"bbb","created_at":"2026-10-11T13:00:00Z"}`)
+ if _, err := s.Put(ctx, domainClaimKey("race.com"), body, etag); !errors.Is(err, release.ErrCASConflict) {
+ t.Fatalf("a takeover on the pre-verify etag = %v, want a CAS conflict", err)
+ }
+ if _, _, err := EnsureDomain(ctx, s, p, w, d, "bbb", "app", "race.com", later); !errors.Is(err, ErrDomainClaimed) {
+ t.Fatalf("takeover after the holder verified = %v, want ErrDomainClaimed", err)
+ }
+ if _, ok := p.Identities[identityKey(d.Region, "race.com")]; !ok {
+ t.Fatal("the verified holder lost its identity")
+ }
+}
+
+// Finding 2: an unverified claim lapses after ClaimLapse and another
+// account may take the domain. The squatter's half-made identity goes with
+// it, so the new claimant starts fresh, with DKIM tokens of its own.
+func TestUnverifiedClaimLapses(t *testing.T) {
+ ctx := context.Background()
+ s, w := release.NewDirStore(t.TempDir()), dnsrec.NewMemWriter()
+ p := NewMemProvisioner("shared")
+ now := time.Date(2026, 10, 3, 12, 0, 0, 0, time.UTC)
+ d := testDeployment()
+
+ first, _, err := EnsureDomain(ctx, s, p, w, d, "aaa", "squat", "victim.com", now)
+ if err != nil {
+ t.Fatal(err)
+ }
+ squatTokens := first.Regions[d.Region].DKIMTokens
+
+ if _, _, err := EnsureDomain(ctx, s, p, w, d, "bbb", "real", "victim.com", now.Add(ClaimLapse-time.Hour)); !errors.Is(err, ErrDomainClaimed) {
+ t.Fatalf("a claim inside its lapse window was taken: %v", err)
+ }
+ rec, _, err := EnsureDomain(ctx, s, p, w, d, "bbb", "real", "victim.com", now.Add(ClaimLapse+time.Hour))
+ if err != nil {
+ t.Fatalf("a lapsed claim was not released to another account: %v", err)
+ }
+ if got := rec.Regions[d.Region].DKIMTokens; got[0] == squatTokens[0] {
+ t.Fatalf("the new holder inherited the lapsed holder's identity: %v", got)
+ }
+ if c, _ := getDomainClaim(ctx, s, "victim.com"); c == nil || c.Account != "bbb" {
+ t.Fatalf("claim after lapse = %+v, want bbb's", c)
+ }
+
+ // The lapsed holder can neither verify its stale record into a grant
+ // nor tear down the identity that is now bbb's.
+ p.SetIdentityStatus(d.Region, "victim.com", Status{DKIM: "SUCCESS", MailFrom: "SUCCESS", Sending: "SUCCESS"})
+ if _, err := VerifyDomain(ctx, s, p, d, "aaa", "victim.com", d.Region, now.Add(ClaimLapse+2*time.Hour)); !errors.Is(err, ErrDomainClaimed) {
+ t.Fatalf("lapsed holder's verify = %v, want ErrDomainClaimed", err)
+ }
+ if old, _, _ := GetDomain(ctx, s, "aaa", "victim.com"); old == nil || old.Regions[d.Region].Status == StatusVerified {
+ t.Fatalf("lapsed holder's record reads verified: %+v", old)
+ }
+ if err := Teardown(ctx, s, p, w, d, "aaa", "squat"); err != nil {
+ t.Fatalf("lapsed holder's teardown: %v", err)
+ }
+ if _, ok := p.Identities[identityKey(d.Region, "victim.com")]; !ok {
+ t.Fatal("the lapsed holder's teardown deleted the new holder's identity")
+ }
+ if c, _ := getDomainClaim(ctx, s, "victim.com"); c == nil || c.Account != "bbb" {
+ t.Fatalf("the lapsed holder's teardown disturbed bbb's claim: %+v", c)
+ }
+}
+
+// A verified claim never lapses, even if its record later reads failed —
+// and neither does one SES has verified while nobody ran verify.
+func TestVerifiedClaimDoesNotLapse(t *testing.T) {
+ ctx := context.Background()
+ s, w := release.NewDirStore(t.TempDir()), dnsrec.NewMemWriter()
+ p := NewMemProvisioner("shared")
+ now := time.Date(2026, 10, 3, 12, 0, 0, 0, time.UTC)
+ d := testDeployment()
+
+ if _, _, err := EnsureDomain(ctx, s, p, w, d, "aaa", "app", "kept.com", now); err != nil {
+ t.Fatal(err)
+ }
+ p.SetIdentityStatus(d.Region, "kept.com", Status{DKIM: "SUCCESS", MailFrom: "SUCCESS", Sending: "SUCCESS"})
+ if _, err := VerifyDomain(ctx, s, p, d, "aaa", "kept.com", d.Region, now); err != nil {
+ t.Fatal(err)
+ }
+ p.SetIdentityStatus(d.Region, "kept.com", Status{DKIM: "FAILED", MailFrom: "SUCCESS", Sending: "SUCCESS"})
+ if _, err := VerifyDomain(ctx, s, p, d, "aaa", "kept.com", d.Region, now); err != nil {
+ t.Fatal(err)
+ }
+ if _, _, err := EnsureDomain(ctx, s, p, w, d, "bbb", "app", "kept.com", now.Add(10*ClaimLapse)); !errors.Is(err, ErrDomainClaimed) {
+ t.Fatalf("a once-verified claim lapsed: %v", err)
+ }
+
+ // Verified at SES, never verified through CARLOS.
+ if _, _, err := EnsureDomain(ctx, s, p, w, d, "aaa", "app", "quiet.com", now); err != nil {
+ t.Fatal(err)
+ }
+ p.SetIdentityStatus(d.Region, "quiet.com", Status{DKIM: "SUCCESS", MailFrom: "SUCCESS", Sending: "SUCCESS"})
+ if _, _, err := EnsureDomain(ctx, s, p, w, d, "bbb", "app", "quiet.com", now.Add(10*ClaimLapse)); !errors.Is(err, ErrDomainClaimed) {
+ t.Fatalf("a claim SES had verified lapsed: %v", err)
+ }
+}
diff --git a/mail_cli.go b/mail_cli.go
index 81711c54c..3d39b9008 100644
--- a/mail_cli.go
+++ b/mail_cli.go
@@ -381,11 +381,14 @@ func isEmailNotDeclared(err error) bool {
// naming the PERMANENT ones (409 claimed, 400 not-owned) without a shred of
// retry language — the exact mistake the brief calls out. Every other
// error passes through apiErrorFromBody's own text unchanged.
+//
+// The console's own words carry the advice. This used to add "choose a
+// different domain" to every 409, which is wrong for the refusal that says
+// how to prove control of the domain (#216).
func describeDomainEnsureErr(domain string, err error) error {
var me *apiMailError
if errors.As(err, &me) && me.permanent() {
- return fmt.Errorf("%s: %s — permanent: nothing about running this command again would change the "+
- "outcome; choose a different domain", domain, me.Error())
+ return fmt.Errorf("%s: %s", domain, me.Error())
}
return fmt.Errorf("ensuring domain %s: %w", domain, err)
}
diff --git a/mail_cli_test.go b/mail_cli_test.go
index ed278cbb1..74b0f88aa 100644
--- a/mail_cli_test.go
+++ b/mail_cli_test.go
@@ -112,7 +112,7 @@ func TestAPISessionEmailDomainEnsureWireShape(t *testing.T) {
func TestAPISessionEmailDomainEnsureConflictIsPermanent(t *testing.T) {
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusConflict)
- json.NewEncoder(w).Encode(map[string]any{"error": "this domain is already claimed by another CARLOS account — choose a different domain"})
+ json.NewEncoder(w).Encode(map[string]any{"error": "sending domain unavailable"})
}))
defer ts.Close()
@@ -704,7 +704,7 @@ func newFakeEmailConsole(t *testing.T, verifiedAfter int) *fakeEmailConsole {
case r.Method == http.MethodPost && strings.HasSuffix(r.URL.Path, "/email/domains"):
if fc.domainConflict {
w.WriteHeader(http.StatusConflict)
- json.NewEncoder(w).Encode(map[string]any{"error": "this domain is already claimed by another CARLOS account — choose a different domain"})
+ json.NewEncoder(w).Encode(map[string]any{"error": "sending domain unavailable"})
return
}
pending := []map[string]any{}