amadan
Merge pull request #266 from carlosframework/email-followups
Fixes carlos email test, which could not succeed at all, and corrects two spec statements the first live send disproved. Merged with the required check unable to run — GitHub Actions has been billing-dead since ~2026-08-16. The gate was reproduced locally against the merge result: 31 packages green, vet and gofmt clean. minio-contract remains unverified for this change (it needs a container runtime).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3 files changed,
+268
−8
docs/superpowers/specs/2026-08-19-tenant-email-design.md+44 −4mail_cli.go+57 −4mail_cli_test.go+167 −0
diff --git a/docs/superpowers/specs/2026-08-19-tenant-email-design.md b/docs/superpowers/specs/2026-08-19-tenant-email-design.md| index 575b126fc..ffd302525 100644 |
| --- a/docs/superpowers/specs/2026-08-19-tenant-email-design.md |
| +++ b/docs/superpowers/specs/2026-08-19-tenant-email-design.md |
| @@ -21,9 +21,14 @@ in the estate and cannot be bought back. |
| ## 1. Scope and v1 success criterion |
| **v1 is done when `seapointish` sends its real sign-in mail through the |
| -primitive on a custom domain**, with DKIM passing, SPF passing, DMARC |
| -aligned, and the message landing in a Gmail inbox — with **zero seapointish |
| -code changes**. |
| +primitive on a custom domain**, with DKIM passing *and aligned*, SPF |
| +passing, DMARC therefore passing, and the message landing in a Gmail inbox |
| +— with **zero seapointish code changes**. |
| + |
| +(SPF authenticates but never *aligns* under the mandatory `aspf=s`; DMARC |
| +rides on DKIM alone. §7 says why, and why that is the right trade rather |
| +than a defect to fix. The platform-domain half of this criterion was met on |
| +2026-08-23.) |
| That app is the criterion because it already fits the contract without |
| being asked to. `internal/app/signinflow.go` reads |
| @@ -397,6 +402,40 @@ unable to authenticate another tenant's From address on a shared |
| registrable domain. On a custom domain it is merely good hygiene; on |
| `oncarlos.com` it is the tenant boundary. |
| +**Which means DMARC passes on the DKIM rail ALONE — corrected 2026-08-23, |
| +from the first live send.** §1's criterion says "DKIM passing, SPF passing, |
| +DMARC aligned", and that phrasing invites the reading that both rails |
| +align. They do not, and cannot. `aspf=s` requires the envelope sender's |
| +domain to match the `From:` domain EXACTLY, but the envelope sender is the |
| +region-qualified MAIL FROM subdomain (`mail-eu-west-1.<host>`) that the |
| +paragraph below makes mandatory. Strict SPF alignment therefore fails by |
| +construction, permanently, for every sender this primitive provisions. SPF |
| +still *authenticates* — the `include:amazonses.com` record is published and |
| +passes — it just never *aligns*. |
| + |
| +DMARC passes because it needs only one aligned rail, and DKIM aligns |
| +exactly (SES signs with `d=` the identity domain, which is the `From:` |
| +domain). Verified on the first production send: `seapointish.bag.oncarlos.com` |
| +delivered with DKIM aligned and SPF authenticated-but-unaligned. |
| + |
| +Two consequences worth stating where the design is read, because both are |
| +easy to get wrong later: |
| + |
| +- **DKIM is the only rail holding the tenant boundary.** Disabling DKIM |
| + signing on an identity does not degrade to SPF — it drops DMARC to a |
| + failure. Nothing in the code should offer that as an option. |
| +- **`p=reject` is safe but unforgiving.** A tenant moved to `p=reject` |
| + survives entirely on DKIM, so a DKIM CNAME lost from a customer's zone |
| + becomes total delivery failure rather than a downgrade. Raising the |
| + policy is a decision to take with the DNS in view. |
| + |
| +Relaxing to `aspf=r` would make SPF align (the MAIL FROM subdomain shares |
| +an organizational domain with the `From:`), and is exactly the wrong |
| +trade: on `oncarlos.com`, relaxed alignment is what lets one tenant |
| +authenticate another's From address, which is the hole `adkim=s` exists to |
| +close. The strictness stays; the honest statement of what it costs is this |
| +paragraph. |
| + |
| **The MAIL FROM subdomain carries the region, from the first domain ever |
| verified.** The MX target is region-specific, and one subdomain cannot hold |
| two regions' MX records without ambiguity. `mail-eu-west-1.<host>` — the |
| @@ -716,4 +755,5 @@ before. |
| `SetKeyActive(false)` fires, audit and console surface record it, resume |
| restores. |
| - **e2e — the criterion**: seapointish sends a real sign-in link from a |
| - custom domain that lands with DKIM and SPF passing and DMARC aligned. |
| + custom domain that lands with DKIM passing and aligned, SPF passing |
| + (authenticated, not aligned — see §7), and DMARC therefore passing. |
diff --git a/mail_cli.go b/mail_cli.go| index 34707b2d4..afcaec980 100644 |
| --- a/mail_cli.go |
| +++ b/mail_cli.go |
| @@ -943,6 +943,41 @@ type emailTestResult struct { |
| // endpoint on the far end. |
| var sendTestMessageFn = sendTestMessage |
| +// sleepFn is the second seam, and it exists so the retry window below can be |
| +// asserted without a test that actually waits two minutes. |
| +var sleepFn = func(ctx context.Context, d time.Duration) error { |
| + t := time.NewTimer(d) |
| + defer t.Stop() |
| + select { |
| + case <-t.C: |
| + return nil |
| + case <-ctx.Done(): |
| + return ctx.Err() |
| + } |
| +} |
| + |
| +// smtpAuthError marks the ONE failure this command must be patient about, so |
| +// the retry below can tell it from every other way a send can fail. |
| +type smtpAuthError struct{ err error } |
| + |
| +func (e *smtpAuthError) Error() string { return "smtp auth: " + e.err.Error() } |
| +func (e *smtpAuthError) Unwrap() error { return e.err } |
| + |
| +// The auth-retry window (issue #265). SES's SMTP endpoint does not recognise |
| +// a just-minted IAM access key for up to a minute or two; until it does, |
| +// every attempt is refused with 535 "Authentication Credentials Invalid", |
| +// which reads exactly like a wrong password and is not one. |
| +// |
| +// Retrying is not a nicety here, it is the difference between a command that |
| +// works and one that never does. This flow mints a FRESH credential on every |
| +// invocation, so a human who reruns it after a failure is not giving the key |
| +// more time — they are testing a brand-new zero-second-old key. The patience |
| +// has to live inside the run that minted it. |
| +const ( |
| + emailTestAuthAttempts = 13 |
| + emailTestAuthPause = 10 * time.Second |
| +) |
| + |
| // runEmailTestFlow mints a throwaway standalone credential, sends through |
| // it, and revokes it — success or failure, always. This CLI has no other |
| // way to prove sending actually works: the delivered credential's password |
| @@ -962,9 +997,27 @@ func runEmailTestFlow(ctx context.Context, w io.Writer, sess *apiSession, acct a |
| } |
| }() |
| - detail, err := sendTestMessageFn(creds, to) |
| - if err != nil { |
| - return emailTestResult{}, fmt.Errorf("sending through %s: %w", creds.Host, err) |
| + var detail string |
| + for attempt := 1; ; attempt++ { |
| + detail, err = sendTestMessageFn(creds, to) |
| + if err == nil { |
| + break |
| + } |
| + // Only the auth refusal is retried. A refused connection or a |
| + // message SES rejects is a real answer, and spending two minutes |
| + // re-asking would turn a clear error into a hang. |
| + var authErr *smtpAuthError |
| + if !errors.As(err, &authErr) || attempt >= emailTestAuthAttempts { |
| + return emailTestResult{}, fmt.Errorf("sending through %s: %w", creds.Host, err) |
| + } |
| + if attempt == 1 { |
| + fmt.Fprintf(w, "waiting for the new credential to reach %s — SES does not see a "+ |
| + "fresh key for a minute or two (up to %s)\n", |
| + creds.Host, (emailTestAuthAttempts-1)*emailTestAuthPause) |
| + } |
| + if serr := sleepFn(ctx, emailTestAuthPause); serr != nil { |
| + return emailTestResult{}, serr |
| + } |
| } |
| return emailTestResult{host: creds.Host, from: creds.From, detail: detail}, nil |
| } |
| @@ -996,7 +1049,7 @@ func sendTestMessage(creds apiEmailCredsReply, to string) (string, error) { |
| } |
| auth := smtp.PlainAuth("", creds.Username, creds.Password, creds.Host) |
| if err := c.Auth(auth); err != nil { |
| - return "", fmt.Errorf("smtp auth: %w", err) |
| + return "", &smtpAuthError{err} |
| } |
| if err := c.Mail(creds.From); err != nil { |
| return "", fmt.Errorf("mail from: %w", err) |
diff --git a/mail_cli_test.go b/mail_cli_test.go| index d4dd2e2c6..2251b0662 100644 |
| --- a/mail_cli_test.go |
| +++ b/mail_cli_test.go |
| @@ -13,12 +13,14 @@ |
| package main |
| import ( |
| + "bufio" |
| "bytes" |
| "context" |
| "encoding/json" |
| "errors" |
| "fmt" |
| "log" |
| + "net" |
| "net/http" |
| "net/http/httptest" |
| "os/exec" |
| @@ -802,6 +804,171 @@ func TestRunEmailTestFlowRevokesOnFailureToo(t *testing.T) { |
| } |
| } |
| +// TestSendTestMessageMarksA535AsAnAuthFailure pins the link the whole #265 |
| +// fix hangs from: the real sendTestMessage must return an error the retry |
| +// loop can RECOGNISE. The retry tests above substitute sendTestMessageFn, so |
| +// they cannot see this — mutation-testing them proved it, by reverting the |
| +// sentinel to a plain fmt.Errorf and watching every one of them stay green. |
| +// |
| +// So this one drives the real function against a fake SMTP server that |
| +// refuses AUTH with 535, exactly as SES does for a key it cannot see yet. |
| +// The server deliberately does NOT advertise STARTTLS: sendTestMessage only |
| +// upgrades when the extension is offered, and Go's PlainAuth permits |
| +// unencrypted credentials to a loopback host, which is what makes an |
| +// in-process server possible at all. |
| +func TestSendTestMessageMarksA535AsAnAuthFailure(t *testing.T) { |
| + ln, err := net.Listen("tcp", "127.0.0.1:0") |
| + if err != nil { |
| + t.Fatalf("listen: %v", err) |
| + } |
| + defer ln.Close() |
| + go func() { |
| + c, err := ln.Accept() |
| + if err != nil { |
| + return |
| + } |
| + defer c.Close() |
| + br := bufio.NewReader(c) |
| + fmt.Fprint(c, "220 fake ESMTP\r\n") |
| + for { |
| + line, err := br.ReadString('\n') |
| + if err != nil { |
| + return |
| + } |
| + switch { |
| + case strings.HasPrefix(line, "EHLO"), strings.HasPrefix(line, "HELO"): |
| + fmt.Fprint(c, "250-fake\r\n250 AUTH PLAIN\r\n") |
| + case strings.HasPrefix(line, "AUTH"): |
| + fmt.Fprint(c, "535 Authentication Credentials Invalid\r\n") |
| + case strings.HasPrefix(line, "QUIT"): |
| + fmt.Fprint(c, "221 bye\r\n") |
| + return |
| + default: |
| + fmt.Fprint(c, "250 ok\r\n") |
| + } |
| + } |
| + }() |
| + |
| + host, port, err := net.SplitHostPort(ln.Addr().String()) |
| + if err != nil { |
| + t.Fatalf("split: %v", err) |
| + } |
| + creds := apiEmailCredsReply{ |
| + Host: host, Port: port, |
| + Username: "AKIAEXAMPLE", Password: "not-the-right-one", |
| + From: "noreply@example.com", |
| + } |
| + _, err = sendTestMessage(creds, "friend@example.com") |
| + if err == nil { |
| + t.Fatal("want an error from a 535") |
| + } |
| + var authErr *smtpAuthError |
| + if !errors.As(err, &authErr) { |
| + t.Fatalf("a 535 must be recognisable as an auth failure, got %T: %v — "+ |
| + "the retry loop keys on this, so an unmarked 535 makes carlos email test "+ |
| + "fail on the first attempt again (#265)", err, err) |
| + } |
| +} |
| + |
| +// TestRunEmailTestFlowRetriesTheAuthWindow is issue #265. SES's SMTP |
| +// endpoint does not see a just-minted IAM access key for a minute or two, so |
| +// the FIRST auth attempt against a fresh credential fails with 535. Because |
| +// this flow mints a new credential every run, a human retrying the command |
| +// never gets past it either — every retry is another zero-second-old key. |
| +// The flow must therefore be patient itself. |
| +func TestRunEmailTestFlowRetriesTheAuthWindow(t *testing.T) { |
| + fc := newFakeEmailConsole(t, 1) |
| + sess := newTestSession(fc.ts.URL) |
| + origSend, origSleep := sendTestMessageFn, sleepFn |
| + calls := 0 |
| + sendTestMessageFn = func(apiEmailCredsReply, string) (string, error) { |
| + calls++ |
| + if calls < 3 { |
| + return "", &smtpAuthError{fmt.Errorf("535 %q", "Authentication Credentials Invalid")} |
| + } |
| + return "accepted for delivery", nil |
| + } |
| + slept := 0 |
| + sleepFn = func(context.Context, time.Duration) error { slept++; return nil } |
| + t.Cleanup(func() { sendTestMessageFn, sleepFn = origSend, origSleep }) |
| + |
| + var buf bytes.Buffer |
| + res, err := runEmailTestFlow(context.Background(), &buf, sess, apiAcct{Seg: "aaa"}, "seapointish", "friend@gmail.com") |
| + if err != nil { |
| + t.Fatalf("runEmailTestFlow: %v", err) |
| + } |
| + if res.detail == "" { |
| + t.Fatal("result detail is empty") |
| + } |
| + if calls != 3 { |
| + t.Fatalf("send attempts = %d, want 3 (two refusals then success)", calls) |
| + } |
| + if slept != 2 { |
| + t.Fatalf("slept %d times, want 2 — one wait between each attempt", slept) |
| + } |
| + // The wait is long enough to look like a hang; saying so is part of the |
| + // fix, not decoration. |
| + if !strings.Contains(buf.String(), "waiting") { |
| + t.Fatalf("nothing told the user why it is waiting; output = %q", buf.String()) |
| + } |
| + if !fc.credRevoked { |
| + t.Fatal("the temporary standalone credential was never revoked") |
| + } |
| +} |
| + |
| +// TestRunEmailTestFlowGivesUpAndStillRevokes: patience is bounded. An auth |
| +// failure that outlasts the window is a real failure and must surface as |
| +// one — and the credential must still not be left behind. |
| +func TestRunEmailTestFlowGivesUpAndStillRevokes(t *testing.T) { |
| + fc := newFakeEmailConsole(t, 1) |
| + sess := newTestSession(fc.ts.URL) |
| + origSend, origSleep := sendTestMessageFn, sleepFn |
| + calls := 0 |
| + sendTestMessageFn = func(apiEmailCredsReply, string) (string, error) { |
| + calls++ |
| + return "", &smtpAuthError{fmt.Errorf("535 %q", "Authentication Credentials Invalid")} |
| + } |
| + sleepFn = func(context.Context, time.Duration) error { return nil } |
| + t.Cleanup(func() { sendTestMessageFn, sleepFn = origSend, origSleep }) |
| + |
| + var buf bytes.Buffer |
| + _, err := runEmailTestFlow(context.Background(), &buf, sess, apiAcct{Seg: "aaa"}, "seapointish", "friend@gmail.com") |
| + if err == nil { |
| + t.Fatal("want an error when auth never succeeds") |
| + } |
| + if calls != emailTestAuthAttempts { |
| + t.Fatalf("send attempts = %d, want emailTestAuthAttempts = %d", calls, emailTestAuthAttempts) |
| + } |
| + if !fc.credRevoked { |
| + t.Fatal("giving up must still revoke the temporary credential") |
| + } |
| +} |
| + |
| +// TestRunEmailTestFlowDoesNotRetryANonAuthFailure: the retry exists for one |
| +// specific transient. A refused connection, or a message SES rejects, must |
| +// fail on the FIRST attempt — retrying those for two minutes would turn a |
| +// clear error into a hang. |
| +func TestRunEmailTestFlowDoesNotRetryANonAuthFailure(t *testing.T) { |
| + fc := newFakeEmailConsole(t, 1) |
| + sess := newTestSession(fc.ts.URL) |
| + origSend, origSleep := sendTestMessageFn, sleepFn |
| + calls := 0 |
| + sendTestMessageFn = func(apiEmailCredsReply, string) (string, error) { |
| + calls++ |
| + return "", fmt.Errorf("connection refused") |
| + } |
| + sleepFn = func(context.Context, time.Duration) error { t.Fatal("slept on a non-auth failure"); return nil } |
| + t.Cleanup(func() { sendTestMessageFn, sleepFn = origSend, origSleep }) |
| + |
| + var buf bytes.Buffer |
| + if _, err := runEmailTestFlow(context.Background(), &buf, sess, apiAcct{Seg: "aaa"}, "seapointish", "friend@gmail.com"); err == nil { |
| + t.Fatal("want an error") |
| + } |
| + if calls != 1 { |
| + t.Fatalf("send attempts = %d, want 1 — a non-auth failure is not retried", calls) |
| + } |
| +} |
| + |
| // ---- RunEmail-level: dispatch and refusals (real binary) ------------------- |
| // TestEmailVerbDispatchAndRefusals drives the real binary through |