fix(senderidentity): adopt provably-owned pre-tag identities, stop stranding the ledger

[jiashuoz]

Summary

v1.7.8 (#888) introduced an ownership tag (e2a-managed=sender-identity-v1) on every SES sending identity e2a creates, and started treating any identity lacking that tag as foreign (ErrIdentityNotOwned, never mutated). Every identity created before that release is untagged, so on upgrade Provision/Status hit an untagged GetEmailIdentity response, marked the domain sending_status=failed, and the ErrIdentityNotOwned handler unconditionally DELETEd the domain's row from sender_identity_managed_domains. The periodic reaper only iterates that ledger, so the domain was never automatically revisited again. This was confirmed in production: the ledger emptied and affected domains stayed failed indefinitely until rows were manually re-inserted.

This PR fixes both halves.

Part A — adoption. An untagged identity is now provably e2a's own, and gets tagged in place before proceeding normally, when all of the following hold:

• e2a has DKIM key material on file for the domain (the caller's stored selector)
• SES reports the identity as BYODKIM (DkimAttributes.SigningAttributesOrigin == EXTERNAL) — only e2a's own Provision ever supplies signing key material
• the installed selector (DkimAttributes.Tokens) matches e2a's stored selector for that exact domain

Any other combination — no key material, an AWS-managed/Easy-DKIM identity, or a selector mismatch — still returns ErrIdentityNotOwned untouched, exactly as before adoption existed. This negative case is security-critical: a too-loose rule would let e2a silently take over a different application's SES identity in a shared AWS account — the exact scenario the tag was introduced to prevent for self-hosters. Adoption is permanent behavior (not a one-time migration flag), so it also self-heals a tag that was removed out-of-band, which the existing code comments already flagged as a concern.

Implemented in SESProvider (internal/senderidentity/ses.go):

• canAdoptIdentity — the pure criteria check
• adoptIdentity — calls TagResource against the identity's ARN, built from region + the AWS account ID (resolved once via STS GetCallerIdentity at provider construction, since neither GetEmailIdentity nor ListEmailIdentities returns an ARN)
• Provider.Status widens to Status(ctx, domain, expectedSelector string) so it can make the same adoption judgement Provision does — the worker already holds the selector (state.Selector) at both call sites

Part B — stop deleting the ledger row for a live domain. ForgetSendingIdentityManaged (internal/identity/store.go) now leaves the row alone when the domain still exists in domains with a non-null user_id, expressed as a NOT EXISTS guard inside the DELETE itself rather than a check-then-act at the call site — WithSendingIdentityMutationLock (held by both ErrIdentityNotOwned handlers in worker.go) and the domain-delete transaction's advisory locks use different lock names and don't exclude each other, so a separate existence check beforehand could race a concurrent domain delete. A genuinely deleted domain's row is still removed exactly as before, via FinalizeSendingIdentityTombstone in the disjoint teardown branch (not touched by this change) — verified by both an existing DB test (TestManagedSendingIdentityLedgerSurvivesDomainDelete) and a new worker-level one.

Correcting the task description

The background brief said ErrIdentityNotOwned handling lives "around lines 283 and 597" of worker.go — the actual call sites (on origin/main at the time of this branch) are lines 294 and 603 (store.ForgetSendingIdentityManaged(...)), a few lines off but the same two handlers. Everything else in the brief (the tag constants, ErrIdentityNotOwned semantics, the Tokens field carrying the BYODKIM selector for an EXTERNAL-origin identity, the ARN shape arn:aws:ses:<region>:<account>:identity/<domain>) checked out against the code and the AWS SDK types as described.

Client surface checklist

Not applicable — this is internal SES-provisioning/reconciliation logic with no API, SDK, CLI, or MCP surface.

Operational risk

• SESProvider now calls sts:GetCallerIdentity once at construction (NewSESProviderFromConfig) to build the ARN TagResource needs. This is an unauthenticated-by-IAM-policy STS action (no permission grant required) that every valid AWS principal can call, so this should not newly fail in any environment where SES itself already works — but it is a new failure mode on the sender-identity startup path (already log.Fatalf on any provider-construction error, unchanged behavior class).
• Adoption calls a new AWS action, ses:TagResource, against a specific identity ARN. If the runtime IAM policy is scoped tightly (per the existing design doc's IAM guidance), it must already allow TagResource for the tagged-CreateEmailIdentity dependent-authorization requirement from the original v1.7.8 rollout — this PR doesn't need a new grant beyond what that doc already recommends.
• No schema/migration changes. ForgetSendingIdentityManaged's query changes shape but stays a single indexed DELETE.
• Rollback: revert-safe. Reverting drops back to today's behavior (no adoption, unconditional ledger delete on ownership failure) — no new state is written that the old binary can't handle.

Test plan

• go build ./...
• go test ./internal/senderidentity/... ./internal/identity/... — all pass, including new adoption + ledger-retention tests
• go test ./... — full suite green (EXIT:0, no FAIL)
• go vet ./... — clean except pre-existing, unrelated findings in internal/agent/*_test.go (confirmed identical on a clean origin/main checkout, not touched by this PR)

New tests (table-driven, following internal/senderidentity/*_test.go conventions):

• TestCanAdoptIdentity — the adoption criteria truth table
• TestSESProvider_ProvisionAdoptsProvablyOwnIdentity / _StatusAdoptsProvablyOwnIdentity — matching selector + EXTERNAL origin adopts, tags, and proceeds normally
• TestSESProvider_ProvisionRefusesMismatchedSelector / _StatusRefusesMismatchedSelector — the security-critical negative: mismatched selector stays ErrIdentityNotOwned, no tag applied, no mutation
• TestSESProvider_ProvisionRefusesAWSManagedDkimOrigin — non-EXTERNAL origin stays refused even with a coincidentally matching token
• TestSESProvider_ProvisionAlreadyTaggedDoesNotRetag — no redundant TagResource call
• TestSESProvider_ProvisionAdoptionTagFailurePropagates — a transient TagResource error propagates for retry rather than falling back to a false not-owned
• TestReconcileWorker_NotOwnedRetainsLedgerForLiveDomain / TestSyncWorker_NotOwnedRetainsLedgerForLiveDomain — ledger row survives an ownership failure on a live domain
• TestReapWorker_GenuineDeleteStillFinalizesTombstone — genuinely deleted domain's tombstone still finalizes
• TestForgetSendingIdentityManagedRetainsLedgerForLiveOwnedDomain (DB-backed, internal/identity) — the store-level guard directly

🤖 Generated with Claude Code

[jiashuoz]

Batch A revision — merge-blocking findings addressed

Pushed 7 commits (258c3fc87 → 41adef71d) addressing all 9 findings from review. Each item below maps to the corresponding finding number.

1. Refuse adoption of identities carrying foreign configuration (084dbbd35)
canAdoptIdentity now refuses adoption when the identity has a non-nil ConfigurationSetName or any Policies — either means the identity is doing something e2a never asked for, and adoption would otherwise silently keep it (feedback routing to a foreign config set, or a cross-account send policy).

2+3. Require real key material and DKIM SUCCESS to adopt (af8e342c1)

• canAdoptIdentity now takes an explicit haveKeyMaterial bool and requires it — a stored selector is not proof of key material (LoadSendingIdentityState can return one with a nil private key, e.g. mid domain-reclaim). Provider.Status widened to accept this; both worker.go call sites pass len(state.PrivateKey) > 0.
• Added DkimAttributes.Status == SUCCESS as a required criterion — the existing checks (EXTERNAL origin + selector token) are both publicly-derivable OSS constants with zero entropy. Chose strict SUCCESS, no PENDING fallback (documented in canAdoptIdentity's doc comment): every domain this feature exists to unstrand was already at sending_status=verified before the regression, so the incident population is SUCCESS by construction. DNS-comparing hardening noted as a deferred follow-up, not implemented.

4. Lazy, non-fatal STS resolution (63c9ee61f)
NewSESProviderFromConfig no longer calls sts:GetCallerIdentity at construction. Account-id resolution is now sync.Once-lazy, triggered on first adoption attempt; a failure degrades to ErrIdentityNotOwned (exactly pre-adoption-PR behavior) instead of log.Fatalf-ing the whole server. Also: callerIdentity.Account == nil is now an error instead of silently producing an account-id-less ARN.

5. Classify permanent AWS errors as non-retryable (8a37206f7)
classifyAdoptionError maps AccessDeniedException (matched via smithy's APIError.ErrorCode() — it's unmodeled in the SES v2 SDK's generated types), BadRequestException, and NotFoundException into a wrapped ErrIdentityNotOwned; throttling/5xx/network errors propagate untouched for retry. Verified against the live prod IAM policy: ses:TagResource is allowed for e2a.dev's principal, so the catastrophic every-sweep-red variant isn't active in prod today, but a self-hoster on the design doc's narrower conditioned grant would hit it.

6. Stop forgetting the ledger on ownership failure (fa9eb7cc6)
Removed both ForgetSendingIdentityManaged calls from worker.go's ErrIdentityNotOwned handlers. The NOT EXISTS guard doesn't close the race the original commit claimed — it evaluates against the deleting transaction's own snapshot, so a concurrent domain-delete that commits first still passes the guard and orphans the SES identity. FinalizeSendingIdentityTombstone (drain-window guarded) is now the sole deletion path; ForgetSendingIdentityManaged's NOT EXISTS guard stays as defense-in-depth in the SQL.

7. Guard FinalizeSendingIdentityTombstone with the same predicate (0f9d1ad3b)
Added the identical NOT EXISTS (domains WHERE user_id IS NOT NULL) guard to FinalizeSendingIdentityTombstone. Concrete gap closed: a verified, live, owned domain with NULL dkim_selector/dkim_private_key hits the no-key branch, which calls this method under the reaper's finalizeDeletion=true — pre-fix, an aged row was deleted even though the domain is still live, stranding it exactly like the original incident via a different trigger.

8. Adoption observability — folded into 63c9ee61f (success log in adoptIdentity) and fa9eb7cc6 (ALERT-convention log on refusal for a live domain, matching reaper.go's style).

9. Doc corrections (41adef71d)

• Corrected the "Steps 4, 6, 7 unaffected" claim — adoption itself writes the tag the IAM condition exists to gate, so the two controls are no longer fully independent; rewrote to say precisely what IAM still does vs. no longer independently catches.
• Added "unless provably e2a's own (see canAdoptIdentity)" to the normative untagged-identity bullet.
• Documented the domain-verification gate (live DNS probe of ownership TXT + apex MX) as the real bound on attacker-controlled input, in both the doc and canAdoptIdentity's comment.
• Restated the shared-tag-value constraint inside the amendment: adoption makes violating it silent and escalates the blast radius (adopted identity becomes Deprovision-deletable).

Verification

• go build ./... — clean
• go vet ./... — clean except the known pre-existing findings in internal/agent/*_test.go (unrelated, confirmed identical on 258c3fc87)
• go test ./internal/senderidentity/... ./internal/identity/... — senderidentity fully green; identity has one pre-existing failure (TestInboundReviewApprovalAtomicallyAdvancesAuthenticatedEngagement, in review_test.go, unrelated to sender-identity) — confirmed to fail identically on 258c3fc87 before any of this branch's changes
• go test ./... — three pre-existing failures outside this PR's diff scope (internal/agent: TestMarkSentUpdatesOutreachFromToRecipients, TestMarkSentIsNotDoubleCountedOnRedrive; internal/relay: TestInboundActivityRecording_AuthenticatedMailUpdatesCounters; plus the internal/identity one above) — all four are in packages/files this PR never touches (git diff 258c3fc87 --stat confirms zero changes to internal/agent or internal/relay), so they cannot be caused by this branch

New/updated tests

TestSESProvider_ProvisionRefusesForeignConfiguration, TestSESProvider_StatusRefusesAdoptionWithoutKeyMaterial, TestSESProvider_StatusRefusesAdoptionWhileDkimPending, TestAccountIDFromCallerIdentity, TestSESProvider_AdoptionDegradesWhenAccountIDUnavailable, TestClassifyAdoptionError, TestSESProvider_ProvisionAdoptionAccessDeniedRefusesNotRetries, TestReapWorker_NoKeyMaterialLiveDomainRetainsLedger, TestFinalizeSendingIdentityTombstoneRetainsLedgerForLiveOwnedDomain (DB-backed) — plus TestCanAdoptIdentity's truth table extended with the new criteria. Removed two tests (TestReconcileWorker_NotOwnedForgetFailureStillFires, TestSyncWorker_NotOwnedForgetFailureStillFires) that pinned the now-removed Forget-on-ownership-failure behavior.

🤖 Generated with Claude Code

[jiashuoz]

Batch B — review polish (5 commits, 41adef71d → abb1faff5)

Follow-up to the batch A security/correctness pass, addressing the remaining findings from the two independent reviews.

Commit	Finding addressed
3bb785e42	FakeProvider.Status now records the arguments it receives
8a2bb7971	DB-level tests pinning the DELETE arm of both NOT EXISTS guards
8c3ab1dcd	Ownership-failure ledger tests now assert convergence, not fake bookkeeping
c4eae8b4b	Corrected the stale "sweep is READ-ONLY" invariant comment
abb1faff5	ARN partition derived from the STS Arn instead of hardcoded arn:aws:

Why the first one mattered most: Provider.Status now carries two caller-supplied values (expectedSelector, haveKeyMaterial) that nothing verified — the fake discarded both. A call site passing "" or the wrong boolean would have left every worker test green while adoption silently stopped firing for exactly the domains this PR exists to rescue. Both are now recorded and asserted.

On the DELETE-arm tests: only the retain arm was covered, so an inverted NOT EXISTS predicate would have passed the entire suite. Both guards now have paired retain/remove coverage.

The ststypes → sestypes rename from the review was already resolved in batch A; no separate commit.

Verification

• go build ./..., go vet ./internal/senderidentity/... ./internal/identity/... — clean
• go test ./internal/senderidentity/... — green
• All 15 sender-identity tests in internal/identity — green across two consecutive runs, including the 4 added here
• Known pre-existing failures elsewhere in internal/identity were reproduced on unmodified main and are unrelated to this diff

Note for reviewers: internal/identity is unreliable against a shared local Postgres — main itself varied between 1 and 13+ failures across runs, with a different failure set each time. Several tests there use fixed fixture names and discard every setup error, so an upstream failure surfaces as a confusing nil-deref or "no rows" in an unrelated test. CI against a clean database is the arbiter. Worth a separate hygiene issue.

[jiashuoz]

Batch C revision (5 new commits, abb1faff5 → d637f5fa9)

Two independent Opus re-reviews (correctness + adversarial) of the full 12-commit diff confirmed the core security property holds and every prior finding is genuinely fixed. This batch addresses 11 operability/self-healing/doc-accuracy findings plus one real test hole from that pass. All re-verified against current code before fixing (a couple of line numbers had drifted).

1. sync.Once cached a transient STS failure forever. The cached ctx belonged to the first caller — a River job — and jobs.go sets no JobTimeout, so River's 1-minute default applies. reapManagedIdentityPage's up-to-25-domain pages with several SES round trips each could land that deadline mid-STS-call on the largest post-upgrade sweep, permanently caching DeadlineExceeded and flipping every later adoption to ErrIdentityNotOwned until a restart — the same failure shape this PR exists to fix. Fixed: identityForAdoption now runs STS on context.WithoutCancel(ctx) with its own 10s timeout, and the cache is a mutex — only success is cached permanently; a failure is cached for a 30s cooldown, then retried by the next caller.

2. Refusal reasons were discarded at the log line. canAdoptIdentity now returns (ok bool, reason string) instead of a bare bool; Provision/Status wrap the reason into the returned ErrIdentityNotOwned, and both worker.go ALERT lines now log %v of the real error alongside the constant summary — a missing IAM grant, a wrong-partition ARN, DKIM-still-PENDING, a selector mismatch, and a genuinely foreign identity are now distinguishable. Customer-visible sending_error wording unchanged.

3. New migration for domains stranded by v1.7.8. 105_sender_identity_managed_domains_repair_stranded.sql re-runs migration 101's backfill scoped to the stranded population (live, owned, sending_status='failed', no ledger row) — idempotent ON CONFLICT DO NOTHING, applied_incarnation deliberately NULL so the repaired row actually reconverges instead of reading as already-applied.

4. Missing negative-arm test. Mutation-testing confirmed hardcoding true at syncProviderIdentityWithInspection's healthy-recheck provider.Status call site left the whole suite green. New TestSyncWorker_AlreadyVerifiedNoOpCarriesRealKeyMaterialSignal seeds no key material and asserts HaveKey:false — verified it actually catches the mutation before landing it.

5. AdoptionEvidence struct replaces Provider.Status(ctx, domain, expectedSelector string, haveKeyMaterial bool)'s two independent booleans, built by one shared helper (adoptionEvidence in worker.go) at both call sites.

6. TagResource NotFoundException reclassified from ErrIdentityNotOwned to ErrIdentityNotFound — it means the identity vanished between the GET and the tag call (a delete raced adoption), not that it's foreign. Callers already treat ErrIdentityNotFound as repair/converge.

7. Empty-string AWS account id now rejected alongside nil, closing the same malformed-ARN gap from the other side.

8. FinalizeSendingIdentityTombstone's live-owner guard tightened to require d.verified, not just d.user_id IS NOT NULL — the old guard made Finalize a permanent no-op for a legitimate delete-then-immediate-re-register race (fresh unverified row), stranding the old incarnation's tombstone forever. Both arms now covered by DB tests.

9. Corrected an overclaim (code comment + design doc): DKIM SUCCESS proves SES matched whatever key it holds, not provably e2a's key — for a hypothetical foreign BYODKIM identity that's that app's key. The actual ownership work is done by the exact-selector match plus the reachability bound (see #10), not SUCCESS alone.

10. Reachability-bound caveat documented and closed. The bound ("adoption only ever attempted for a DNS-verified domain") holds only under env: production — checkDomainRecords short-circuits DNS checks to always-found otherwise. Added SESProvider.refuseAdoption (from NewSESProviderFromConfig's new production parameter, cfg.IsProduction() in main.go): a non-production deployment now refuses adoption outright rather than silently trusting an unverified domain.

11. Silent unbounded retry now logs. The no-key branch (live/owned/verified domain, NULL DKIM key material) re-runs hourly forever with 2 SES calls + a status write and previously no log line at all. Added an ALERT log matching the other two branches' convention, noting it recurs hourly.

Verification

• go build ./... — clean
• go vet ./... — clean except pre-existing, unrelated findings in internal/agent/*_test.go (not touched by this PR, present on main)
• go test ./internal/senderidentity/... — all pass (69 subtests)
• go test ./internal/identity/ -run 'SendingIdentity|Tombstone|Forget|Managed' -count=1 — all pass

Did not run the full ./internal/identity/... suite per the known local-DB-contention flakiness. Nothing was found wrong in the batch — all 11 findings re-verified as accurate against current code and fixed as described.

🤖 Generated with Claude Code