amadan

carlos / platform Public

mail: address Task 9 review — region guard, tenant precondition, DKIM token check, ARN test, fake parity

Six fixes from review, none a redesign:

- Every SES-facing method that takes a region parameter now refuses a
  mismatch against the client's own bound region instead of silently
  ignoring it — the dead p.region field is what makes this loud rather
  than a future silent multi-region breakage.
- EnsureTenant's doc comment now states explicitly that no resource
  association is made and that the send path must not emit
  X-SES-TENANT until identities and configuration sets are associated
  with the tenant — SES validates that association on every send.
- EnsureIdentity refuses success with zero DKIM tokens rather than
  reporting an identity that can never converge with nothing to
  diagnose why.
- Added an in-package test using a stubbed sesAPI that exercises the
  F3-critical ARN composition path directly: the already-exists
  read-back returns the right tokens, the composed ARN has the exact
  shape, and — the assertion that actually matters — that ARN is
  accepted by SendPolicyJSON. Also covers the zero-token refusal, the
  region mismatch refusal, and EnsureTenant against the stub.
- ReapKeys gets a nil-Marker guard against an infinite loop.
- Fixed two fake/real divergences in memprovisioner.go (authorised by
  the reviewer): IdentityStatus on an unknown identity now returns
  NOT_STARTED rather than erroring, matching SES's real answer; MintKey
  now enforces IAM's real two-key-per-user cap, which is the one that
  matters most since Task 13's rotation walk would otherwise pass
  against the fake and fail against production the first time it ran.
Paul Campbell 3af567450c4e7d25b89ef10b7f21fc799940db3c parent a790537
3 files changed, +232 −5
  • internal/mail/aws.go +63 −4
  • internal/mail/aws_test.go +152 −0
  • internal/mail/memprovisioner.go +17 −1
diff --git a/internal/mail/aws.go b/internal/mail/aws.go
index c98fa9f30..c6f4ed505 100644
--- a/internal/mail/aws.go
+++ b/internal/mail/aws.go
@@ -118,6 +118,24 @@ func NewAWSProvisioner(ctx context.Context, region string) (Provisioner, error)
}, nil
}
+// checkRegion refuses a call naming a region other than the one this
+// provisioner is bound to. The SES client here is bound once at
+// construction (NewAWSProvisioner), but every SES-facing Provisioner method
+// still takes a region parameter — because the spec's multi-region story is
+// "capable, not deployed" and MemProvisioner genuinely models N regions. An
+// awsProvisioner silently routing every region through its one client would
+// let a walk test built against the fake pass while production quietly acts
+// on the wrong region (or, for EnsureIdentity, composes an ARN naming the
+// CALLER's region while the identity itself was created in a different
+// one). An empty region is accepted as "caller doesn't care" and treated as
+// this provisioner's own region.
+func (p *awsProvisioner) checkRegion(region string) error {
+ if region != "" && region != p.region {
+ return fmt.Errorf("mail: provisioner is bound to region %s, cannot act on %s", p.region, region)
+ }
+ return nil
+}
+
// ---- error classification -------------------------------------------------
func isAlreadyExists(err error) bool {
@@ -138,10 +156,23 @@ func isNotFound(err error) bool {
//
// See the TENANT ATTRIBUTION note at the top of this file: CreateTenant is a
// real, present SESv2 API, verified against the live docs on 2026-08-19. This
-// only creates the named container — it does not associate identities or
-// configuration sets with it, because EnsureTenant's signature carries none
-// to associate. Isolation is unaffected either way: the tenant buys
-// reputation isolation, and authority isolation is the IAM policy (spec §4).
+// creates ONLY the named container — it does not call
+// CreateTenantResourceAssociation, because EnsureTenant's signature carries
+// no identity ARN or configuration-set name to associate. Isolation is
+// unaffected by that gap: the tenant buys reputation isolation, and
+// authority isolation is the IAM policy (spec §4), which this does not touch.
+//
+// PRECONDITION FOR SENDING, NOT YET MET HERE: per
+// docs.aws.amazon.com/ses/latest/dg/tenants.html, a tenant needs at least
+// one verified identity AND one configuration set associated via
+// CreateTenantResourceAssociation before SES will accept a send attributed
+// to it — SES validates that association on every send. Nothing in this
+// package wires that association yet. Consequently: the send path MUST NOT
+// emit the `X-SES-TENANT: <name>` header until associations are wired,
+// wherever that lands (identities and config sets are ensured by separate
+// calls, so it is not naturally EnsureTenant's job). Emitting the header
+// against a tenant with no associated resources fails every send, the
+// opposite of what tenants exist to do.
func (p *awsProvisioner) EnsureTenant(ctx context.Context, name string) error {
_, err := p.ses.CreateTenant(ctx, &sesv2.CreateTenantInput{
TenantName: aws.String(name),
@@ -153,6 +184,9 @@ func (p *awsProvisioner) EnsureTenant(ctx context.Context, name string) error {
}
func (p *awsProvisioner) EnsureConfigSet(ctx context.Context, region, name, snsTopicARN string) error {
+ if err := p.checkRegion(region); err != nil {
+ return err
+ }
_, err := p.ses.CreateConfigurationSet(ctx, &sesv2.CreateConfigurationSetInput{
ConfigurationSetName: aws.String(name),
})
@@ -196,6 +230,9 @@ func (p *awsProvisioner) EnsureConfigSet(ctx context.Context, region, name, snsT
// 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) {
+ if err := p.checkRegion(region); err != nil {
+ return Identity{}, err
+ }
var tokens []string
out, err := p.ses.CreateEmailIdentity(ctx, &sesv2.CreateEmailIdentityInput{
EmailIdentity: aws.String(domain),
@@ -219,6 +256,15 @@ func (p *awsProvisioner) EnsureIdentity(ctx context.Context, region, domain, mai
return Identity{}, fmt.Errorf("mail: create identity %q in %s: %w", domain, region, err)
}
+ // No tokens is not a partial success: with none to publish as CNAMEs,
+ // the domain can never converge to DKIM SUCCESS, and nothing else in
+ // the walk would ever surface why. This also catches an identity that
+ // exists but was created BYODKIM or as an email-address identity rather
+ // than Easy DKIM — GetEmailIdentity succeeds but has no Tokens either.
+ if len(tokens) == 0 {
+ return Identity{}, fmt.Errorf("mail: identity %q in %s has no Easy DKIM tokens available", domain, region)
+ }
+
if _, err := p.ses.PutEmailIdentityMailFromAttributes(ctx, &sesv2.PutEmailIdentityMailFromAttributesInput{
EmailIdentity: aws.String(domain),
MailFromDomain: aws.String(mailFrom),
@@ -234,6 +280,9 @@ func (p *awsProvisioner) EnsureIdentity(ctx context.Context, region, domain, mai
}
func (p *awsProvisioner) IdentityStatus(ctx context.Context, region, domain string) (Status, error) {
+ if err := p.checkRegion(region); err != nil {
+ return Status{}, err
+ }
got, err := p.ses.GetEmailIdentity(ctx, &sesv2.GetEmailIdentityInput{
EmailIdentity: aws.String(domain),
})
@@ -257,6 +306,9 @@ func (p *awsProvisioner) IdentityStatus(ctx context.Context, region, domain stri
}
func (p *awsProvisioner) DeleteIdentity(ctx context.Context, region, domain string) error {
+ if err := p.checkRegion(region); err != nil {
+ return err
+ }
_, err := p.ses.DeleteEmailIdentity(ctx, &sesv2.DeleteEmailIdentityInput{
EmailIdentity: aws.String(domain),
})
@@ -365,6 +417,13 @@ func (p *awsProvisioner) ReapKeys(ctx context.Context, user string, keep []strin
if !out.IsTruncated {
return nil
}
+ if out.Marker == nil {
+ // IsTruncated=true with no Marker to page from would otherwise
+ // spin forever re-issuing the same request. AWS is not observed
+ // to do this, but this loop runs inside a console door, and the
+ // guard is one line while the file is already open.
+ return nil
+ }
marker = out.Marker
}
}
diff --git a/internal/mail/aws_test.go b/internal/mail/aws_test.go
index 055ebc57f..424f92416 100644
--- a/internal/mail/aws_test.go
+++ b/internal/mail/aws_test.go
@@ -1,10 +1,14 @@
package mail
import (
+ "context"
"errors"
+ "reflect"
"testing"
+ "github.com/aws/aws-sdk-go-v2/aws"
iamtypes "github.com/aws/aws-sdk-go-v2/service/iam/types"
+ "github.com/aws/aws-sdk-go-v2/service/sesv2"
sesv2types "github.com/aws/aws-sdk-go-v2/service/sesv2/types"
)
@@ -49,3 +53,151 @@ func TestMailFromFailureRejectsRatherThanFallsBack(t *testing.T) {
t.Fatalf("BehaviorOnMxFailure = %v, want REJECT_MESSAGE", mailFromBehaviour)
}
}
+
+// stubSES is a minimal sesAPI double: only the methods a given test cares
+// about are wired via the function fields; everything else answers with a
+// zero value and a nil error, which is fine because no test below relies on
+// those paths.
+type stubSES struct {
+ createTenant func(*sesv2.CreateTenantInput) (*sesv2.CreateTenantOutput, error)
+ createEmailIdentity func(*sesv2.CreateEmailIdentityInput) (*sesv2.CreateEmailIdentityOutput, error)
+ getEmailIdentity func(*sesv2.GetEmailIdentityInput) (*sesv2.GetEmailIdentityOutput, error)
+ putEmailIdentityMailFromAttributes func(*sesv2.PutEmailIdentityMailFromAttributesInput) (*sesv2.PutEmailIdentityMailFromAttributesOutput, error)
+}
+
+func (s *stubSES) CreateTenant(_ context.Context, in *sesv2.CreateTenantInput, _ ...func(*sesv2.Options)) (*sesv2.CreateTenantOutput, error) {
+ if s.createTenant != nil {
+ return s.createTenant(in)
+ }
+ return &sesv2.CreateTenantOutput{}, nil
+}
+
+func (s *stubSES) CreateEmailIdentity(_ context.Context, in *sesv2.CreateEmailIdentityInput, _ ...func(*sesv2.Options)) (*sesv2.CreateEmailIdentityOutput, error) {
+ if s.createEmailIdentity != nil {
+ return s.createEmailIdentity(in)
+ }
+ return &sesv2.CreateEmailIdentityOutput{}, nil
+}
+
+func (s *stubSES) GetEmailIdentity(_ context.Context, in *sesv2.GetEmailIdentityInput, _ ...func(*sesv2.Options)) (*sesv2.GetEmailIdentityOutput, error) {
+ if s.getEmailIdentity != nil {
+ return s.getEmailIdentity(in)
+ }
+ return &sesv2.GetEmailIdentityOutput{}, nil
+}
+
+func (s *stubSES) PutEmailIdentityMailFromAttributes(_ context.Context, in *sesv2.PutEmailIdentityMailFromAttributesInput, _ ...func(*sesv2.Options)) (*sesv2.PutEmailIdentityMailFromAttributesOutput, error) {
+ if s.putEmailIdentityMailFromAttributes != nil {
+ return s.putEmailIdentityMailFromAttributes(in)
+ }
+ return &sesv2.PutEmailIdentityMailFromAttributesOutput{}, nil
+}
+
+func (s *stubSES) DeleteEmailIdentity(_ context.Context, in *sesv2.DeleteEmailIdentityInput, _ ...func(*sesv2.Options)) (*sesv2.DeleteEmailIdentityOutput, error) {
+ return &sesv2.DeleteEmailIdentityOutput{}, nil
+}
+
+func (s *stubSES) CreateConfigurationSet(_ context.Context, in *sesv2.CreateConfigurationSetInput, _ ...func(*sesv2.Options)) (*sesv2.CreateConfigurationSetOutput, error) {
+ return &sesv2.CreateConfigurationSetOutput{}, nil
+}
+
+func (s *stubSES) CreateConfigurationSetEventDestination(_ context.Context, in *sesv2.CreateConfigurationSetEventDestinationInput, _ ...func(*sesv2.Options)) (*sesv2.CreateConfigurationSetEventDestinationOutput, error) {
+ return &sesv2.CreateConfigurationSetEventDestinationOutput{}, nil
+}
+
+var _ sesAPI = (*stubSES)(nil)
+
+// TestEnsureIdentityAlreadyExistsReadsBackTokensAndComposesTheARN pins the
+// F3-critical path: the ruling's own words are that getting this wrong means
+// every IAM policy names a resource that does not exist, and this was
+// otherwise untested until Task 12. It exercises the already-exists branch
+// (CreateEmailIdentity fails, GetEmailIdentity is read back for the DKIM
+// tokens already published in DNS), checks the exact composed ARN, and —
+// the assertion that actually matters, because it pins aws.go and policy.go
+// together — checks that ARN is ACCEPTED by SendPolicyJSON.
+func TestEnsureIdentityAlreadyExistsReadsBackTokensAndComposesTheARN(t *testing.T) {
+ wantTokens := []string{"tok1", "tok2", "tok3"}
+ 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) {
+ return &sesv2.GetEmailIdentityOutput{
+ DkimAttributes: &sesv2types.DkimAttributes{Tokens: wantTokens},
+ }, nil
+ },
+ }
+ p := &awsProvisioner{ses: stub, account: "123456789012", region: "eu-west-1"}
+
+ got, err := p.EnsureIdentity(context.Background(), "eu-west-1", "example.com", "mail.example.com")
+ if err != nil {
+ t.Fatalf("EnsureIdentity: %v", err)
+ }
+ if !reflect.DeepEqual(got.DKIMTokens, wantTokens) {
+ t.Fatalf("DKIMTokens = %v, want the ones read back from GetEmailIdentity %v", got.DKIMTokens, wantTokens)
+ }
+
+ const wantARN = "arn:aws:ses:eu-west-1:123456789012:identity/example.com"
+ if got.ARN != wantARN {
+ t.Fatalf("ARN = %q, want %q", got.ARN, wantARN)
+ }
+
+ // The assertion that actually matters: the composed ARN must satisfy
+ // SendPolicyJSON's shape validator, since EnsureIdentity's return value
+ // flows straight into EnsureUser -> SendPolicyJSON in production. A
+ // composed ARN that validator rejects would make every send fail with
+ // no way to notice until Task 12.
+ if _, err := SendPolicyJSON([]string{got.ARN}, []string{"example.com"}); err != nil {
+ t.Fatalf("composed ARN %q was rejected by SendPolicyJSON: %v", got.ARN, 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
+// walk would publish no CNAMEs and the domain could never converge, with
+// nothing surfacing why.
+func TestEnsureIdentityRefusesNoTokens(t *testing.T) {
+ stub := &stubSES{
+ createEmailIdentity: func(*sesv2.CreateEmailIdentityInput) (*sesv2.CreateEmailIdentityOutput, error) {
+ return &sesv2.CreateEmailIdentityOutput{}, 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"); err == nil {
+ t.Fatal("EnsureIdentity with zero DKIM tokens returned success, want an error")
+ }
+}
+
+// TestEnsureIdentityRefusesAMismatchedRegion pins the region guard: an
+// awsProvisioner is bound to one client at construction, and a caller naming
+// a different region must be refused loudly rather than silently routed
+// through the wrong client.
+func TestEnsureIdentityRefusesAMismatchedRegion(t *testing.T) {
+ p := &awsProvisioner{ses: &stubSES{}, account: "123456789012", region: "eu-west-1"}
+ if _, err := p.EnsureIdentity(context.Background(), "us-east-1", "example.com", "mail.example.com"); err == nil {
+ t.Fatal("EnsureIdentity accepted a region other than the one the provisioner is bound to")
+ }
+}
+
+// TestEnsureTenantCallsCreateTenantAndAcceptsAlreadyExists exercises
+// EnsureTenant, which added CreateTenant to sesAPI and was otherwise never
+// stubbed anywhere.
+func TestEnsureTenantCallsCreateTenantAndAcceptsAlreadyExists(t *testing.T) {
+ var gotName string
+ stub := &stubSES{
+ createTenant: func(in *sesv2.CreateTenantInput) (*sesv2.CreateTenantOutput, error) {
+ gotName = aws.ToString(in.TenantName)
+ return nil, &sesv2types.AlreadyExistsException{Message: aws.String("tenant already exists")}
+ },
+ }
+ p := &awsProvisioner{ses: stub, account: "123456789012", region: "eu-west-1"}
+
+ if err := p.EnsureTenant(context.Background(), "acct-42"); err != nil {
+ t.Fatalf("EnsureTenant: %v", err)
+ }
+ if gotName != "acct-42" {
+ t.Fatalf("CreateTenant called with TenantName %q, want %q", gotName, "acct-42")
+ }
+}
diff --git a/internal/mail/memprovisioner.go b/internal/mail/memprovisioner.go
index f4bb83ed1..f8e693e39 100644
--- a/internal/mail/memprovisioner.go
+++ b/internal/mail/memprovisioner.go
@@ -135,7 +135,14 @@ func (p *MemProvisioner) IdentityStatus(_ context.Context, region, domain string
}
got, ok := p.Identities[identityKey(region, domain)]
if !ok {
- return Status{}, fmt.Errorf("mail: no identity %s in %s", domain, region)
+ // Matches the real backend: SES's GetEmailIdentity 404s on an
+ // unknown identity, and awsProvisioner.IdentityStatus turns that
+ // into an all-NOT_STARTED Status with a nil error rather than an
+ // error — an identity nobody has ensured yet is not a failure, it
+ // just hasn't started. The fake used to error here; that was a
+ // divergence a walk test could pass against while production, given
+ // the same input, would not error at all.
+ return Status{DKIM: "NOT_STARTED", MailFrom: "NOT_STARTED", Sending: "NOT_STARTED"}, nil
}
return got.Status, nil
}
@@ -191,6 +198,15 @@ func (p *MemProvisioner) MintKey(_ context.Context, user string) (string, string
if !ok {
return "", "", fmt.Errorf("mail: no user %q", user)
}
+ // IAM hard-caps every user at two access keys and returns
+ // LimitExceededException on a third CreateAccessKey — this is the real
+ // constraint, so the fake enforces it too. Without this, a rotation walk
+ // that tries to mint a third key (the delivered key, a fresh key mid-
+ // rotation, and a stray third from a bug) would pass against the fake
+ // and fail the first time it ran against production.
+ if len(u.Keys) >= 2 {
+ return "", "", fmt.Errorf("mail: user %q already has 2 access keys, IAM's per-user limit", user)
+ }
p.nextKey++
id := fmt.Sprintf("AKIAFAKE%06d", p.nextKey)
secret := fmt.Sprintf("secret-%06d", p.nextKey)