#21: fix the DKIM, onboarding and console defects found reviewing the new work

DKIM rotation broke the domain it was run on. It retired the live key and inserted a placeholder as ACTIVE - private_key_enc was the literal string "dummy-encrypted-key" - so FindActiveKeys then failed for the whole domain and outbound mail stopped being signed, with the real key already gone. Reproduced against a database before changing anything. The root cause runs deeper: the auth service derives its encryption key with a plain SHA-256 of the master key while the protocols service uses HKDF with its own info string, so nothing the auth service seals can ever be opened by the signer. Rotation therefore moves to the protocols service, which owns that vault and the real key generator. It retires the previous key with a seven-day overlap in the same transaction rather than deleting it, because mail already sent under it is still being verified. Domain onboarding had three defects in one function. It built the alias destination by interpolating the domain name into the SQL text, which was injectable; it reused one placeholder for a CITEXT and a VARCHAR column, which made the statement fail and happened to hide the injection; and it ignored its scope argument entirely, so any DOMAIN_ADMIN could create domains. It also provisioned postmaster, abuse and security, missing the dmarc and tlsrpt aliases that the published records point at - PLAN.md section 7.4b is explicit that reports bounce without them. The TLS panel returned fixed values: acme_status "VALID" and 72 days to expiry, whatever the process actually held. It now reads the certificate watcher, and a compile-time assertion keeps the wiring from silently regressing into "disabled". The audit log and the readiness scorecard were fixtures too, and their field names did not match the API they now read. Two tests failed and a third asserted a fabricated constant: one depended on test order through a shared fixture that had a second factor enrolled, one used a fixed domain name and so passed once per database, and one expected "records: 13" from a function that never touches DNS. All three are fixed, and the suite now passes twice in a row against one database. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-08-02 13:33:33 -03:00
1 parent 972189a589
commit 30014f0f41
@@ -174,6 +174,15 @@ func run(cfg config) {
 		log.Printf("JWT_SECRET is not set: the webmail mail API stays disabled")
 	}
 	router.SetMailAPI(webmailUC, cfg.JwtSecret)
+	if dkimRepo != nil {
+		router.SetAdminDkimAPI(dkimRepo, generateDkimKey, cfg.JwtSecret)
+	}
+	// The TLS panel reads the live watcher instead of the constants the admin
+	// service used to return, which reported a healthy certificate whatever
+	// the process actually held.
+	if source, ok := certProvider.(httppresentation.TlsStatusSource); ok {
+		router.SetAdminTlsAPI(source, cfg.PrimaryMailHost, cfg.TLSMode, cfg.JwtSecret, cfg.CertPollInterval)
+	}

 	// Real-time updates: the outbox relay reads the events delivery wrote in
 	// its own transaction and hands them to the hub, which pushes them to the
@@ -142,4 +142,3 @@ func ExtractAttachment(entity *message.Entity, targetName string) ([]byte, strin
 	}
 	return nil, ""
 }
-
@@ -0,0 +1,142 @@
+package e2e
+
+import (
+	"bytes"
+	"context"
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+	"time"
+
+	embeddedpostgres "github.com/fergusstrange/embedded-postgres"
+	"github.com/google/uuid"
+
+	"lambdamail/protocols/internal/infrastructure/dkim"
+	"lambdamail/protocols/internal/infrastructure/postgres"
+	"lambdamail/protocols/internal/infrastructure/vault"
+	httppresentation "lambdamail/protocols/internal/presentation/http"
+)
+
+// Rotation has to leave a key the signer can actually open. An earlier
+// implementation retired the live key and inserted a placeholder as ACTIVE,
+// which made FindActiveKeys fail for the whole domain - the console's "rotate"
+// button silently stopped the domain being able to sign anything.
+func TestDkimRotationLeavesAUsableKey(t *testing.T) {
+	if testing.Short() {
+		t.Skip("e2e test skipped in -short mode")
+	}
+	ctx := context.Background()
+
+	runtimeDir := t.TempDir()
+	pg := embeddedpostgres.NewDatabase(embeddedpostgres.DefaultConfig().
+		Port(54349).
+		RuntimePath(runtimeDir).
+		StartTimeout(120 * time.Second))
+	if err := pg.Start(); err != nil {
+		t.Fatalf("start embedded postgres: %v", err)
+	}
+	defer pg.Stop()
+
+	pool, err := postgres.NewPool(ctx, "postgres://postgres:postgres@localhost:54349/postgres?sslmode=disable")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer pool.Close()
+	applyMigrations(t, ctx, pool)
+
+	const domainName = "rotate.example"
+	domainID := uuid.New()
+	if _, err := pool.Exec(ctx,
+		`INSERT INTO domains (id, name, punycode_name) VALUES ($1, $2, $3)`,
+		domainID, domainName, domainName); err != nil {
+		t.Fatal(err)
+	}
+
+	v, err := vault.New("a-master-key-for-rotation-tests")
+	if err != nil {
+		t.Fatal(err)
+	}
+	repo := postgres.NewDkimRepository(pool, v)
+
+	generate := func(algorithm string) ([]byte, string, error) {
+		generated, err := dkim.Generate(algorithm)
+		if err != nil {
+			return nil, "", err
+		}
+		return generated.PrivateKeyPEM, generated.PublicKeyBase64, nil
+	}
+
+	// A key is in place before the rotation, as it would be in production.
+	firstPEM, firstPub, err := generate("rsa2048")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := repo.Insert(ctx, domainName, "old", "rsa2048", firstPEM, firstPub); err != nil {
+		t.Fatal(err)
+	}
+
+	router := httppresentation.NewRouter(nil, func() error { return nil })
+	router.SetAdminDkimAPI(repo, generate, webmailSecret)
+	server := httptest.NewServer(router)
+	defer server.Close()
+
+	rotate := func(token, selector string) *http.Response {
+		body, _ := json.Marshal(map[string]string{
+			"domain": domainName, "selector": selector, "algorithm": "rsa2048",
+		})
+		req, _ := http.NewRequest(http.MethodPost, server.URL+"/api/v1/admin/dkim/rotate", bytes.NewReader(body))
+		req.Header.Set("Content-Type", "application/json")
+		if token != "" {
+			req.Header.Set("Authorization", "Bearer "+token)
+		}
+		resp, err := http.DefaultClient.Do(req)
+		if err != nil {
+			t.Fatal(err)
+		}
+		return resp
+	}
+
+	// A user token must not reach an admin endpoint.
+	if resp := rotate(mintSession(t, "user@rotate.example", "user", time.Hour), "nope"); resp.StatusCode != http.StatusUnauthorized {
+		t.Fatalf("a user token rotated a key: status %d", resp.StatusCode)
+	}
+	resp := rotate("", "nope")
+	if resp.StatusCode != http.StatusUnauthorized {
+		t.Fatalf("an anonymous caller rotated a key: status %d", resp.StatusCode)
+	}
+
+	adminToken := mintSession(t, "admin@rotate.example", "admin", time.Hour)
+	if resp := rotate(adminToken, "new"); resp.StatusCode != http.StatusOK {
+		t.Fatalf("rotate: status %d", resp.StatusCode)
+	}
+
+	// The point of the whole exercise: the signer can open the new key.
+	keys, err := repo.FindActiveKeys(ctx, domainName)
+	if err != nil {
+		t.Fatalf("the rotated key cannot be opened: %v", err)
+	}
+	if len(keys) != 1 || keys[0].Selector != "new" {
+		t.Fatalf("active keys = %+v, want exactly the new selector", keys)
+	}
+	if _, err := dkim.ParsePrivateKey(keys[0].PrivateKeyPEM); err != nil {
+		t.Fatalf("the rotated key is not a usable private key: %v", err)
+	}
+
+	// The old key is retired with an overlap, not deleted: mail already sent
+	// under it is still being verified.
+	var status string
+	var retireAfter *time.Time
+	if err := pool.QueryRow(ctx,
+		`SELECT status, retire_after FROM dkim_keys WHERE selector = 'old'`).Scan(&status, &retireAfter); err != nil {
+		t.Fatal(err)
+	}
+	if status != "RETIRING" || retireAfter == nil || !retireAfter.After(time.Now()) {
+		t.Errorf("old key status=%q retire_after=%v, want RETIRING with a future overlap", status, retireAfter)
+	}
+
+	// A selector that would be invalid in DNS is refused.
+	if resp := rotate(adminToken, "bad selector!"); resp.StatusCode != http.StatusBadRequest {
+		t.Errorf("an invalid selector was accepted: status %d", resp.StatusCode)
+	}
+}
@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"time"

 	"github.com/jackc/pgx/v5"
 	"github.com/jackc/pgx/v5/pgxpool"
@@ -122,3 +123,88 @@ func (r *DkimRepository) Insert(ctx context.Context, domainName, selector, algor

 // Ensure port interface compliance
 var _ port.DkimKeyRepository = (*DkimRepository)(nil)
+
+// DkimKeyInfo describes a key for the admin console. The private half is never
+// part of this: it exists only to be used, not to be looked at.
+type DkimKeyInfo struct {
+	ID          string     `json:"id"`
+	DomainName  string     `json:"domain"`
+	Selector    string     `json:"selector"`
+	Algorithm   string     `json:"algorithm"`
+	PublicKey   string     `json:"public_key"`
+	Status      string     `json:"status"`
+	ActivatedAt *time.Time `json:"activated_at"`
+	RetireAfter *time.Time `json:"retire_after"`
+}
+
+// ListKeys returns every key for a domain, newest first.
+func (r *DkimRepository) ListKeys(ctx context.Context, domainName string) ([]DkimKeyInfo, error) {
+	rows, err := r.pool.Query(ctx, `
+		SELECT k.id::text, d.name, k.selector, k.algorithm, k.public_key, k.status,
+		       k.activated_at, k.retire_after
+		  FROM dkim_keys k JOIN domains d ON d.id = k.domain_id
+		 WHERE d.name = $1
+		 ORDER BY k.created_at DESC
+	`, domainName)
+	if err != nil {
+		return nil, fmt.Errorf("list dkim keys for %s: %w", domainName, err)
+	}
+	defer rows.Close()
+
+	keys := []DkimKeyInfo{}
+	for rows.Next() {
+		var k DkimKeyInfo
+		if err := rows.Scan(&k.ID, &k.DomainName, &k.Selector, &k.Algorithm,
+			&k.PublicKey, &k.Status, &k.ActivatedAt, &k.RetireAfter); err != nil {
+			return nil, err
+		}
+		keys = append(keys, k)
+	}
+	return keys, rows.Err()
+}
+
+// Rotate replaces the active key for one algorithm.
+//
+// The old key is retired rather than deleted, and only after the new one is in
+// place, both inside one transaction. Mail already in flight was signed with
+// the old selector and receivers may still be resolving it, so removing it
+// immediately would fail DKIM for messages that were perfectly valid when they
+// were sent (PLAN.md section 5).
+func (r *DkimRepository) Rotate(
+	ctx context.Context, domainName, selector, algorithm string,
+	privateKeyPEM []byte, publicKey string, overlap time.Duration,
+) error {
+	ciphertext, nonce, err := r.vault.Seal(privateKeyPEM)
+	if err != nil {
+		return fmt.Errorf("seal dkim private key: %w", err)
+	}
+
+	tx, err := r.pool.Begin(ctx)
+	if err != nil {
+		return err
+	}
+	defer tx.Rollback(ctx)
+
+	var domainID string
+	if err := tx.QueryRow(ctx, `SELECT id::text FROM domains WHERE name = $1`, domainName).Scan(&domainID); err != nil {
+		return fmt.Errorf("find domain %s: %w", domainName, err)
+	}
+
+	if _, err := tx.Exec(ctx, `
+		UPDATE dkim_keys SET status = 'RETIRING', retire_after = NOW() + $3::interval
+		 WHERE domain_id = $1 AND algorithm = $2::varchar AND status = 'ACTIVE'
+	`, domainID, algorithm, overlap.String()); err != nil {
+		return fmt.Errorf("retire current dkim key: %w", err)
+	}
+
+	if _, err := tx.Exec(ctx, `
+		INSERT INTO dkim_keys (
+			domain_id, selector, algorithm, private_key_enc, private_key_nonce,
+			key_version, public_key, status, activated_at
+		) VALUES ($1, $2, $3::varchar, $4, $5, $6, $7, 'ACTIVE', NOW())
+	`, domainID, selector, algorithm, ciphertext, nonce, vault.CurrentKeyVersion, publicKey); err != nil {
+		return fmt.Errorf("insert rotated dkim key: %w", err)
+	}
+
+	return tx.Commit(ctx)
+}
@@ -0,0 +1,182 @@
+package httppresentation
+
+import (
+	"encoding/json"
+	"net/http"
+	"strings"
+	"time"
+
+	"lambdamail/protocols/internal/infrastructure/postgres"
+)
+
+// dkimOverlap is how long the previous key stays resolvable after a rotation.
+// Mail already in flight was signed with it and receivers may still be
+// fetching the selector, so removing it at once would fail DKIM for messages
+// that were valid when they were sent (PLAN.md section 5).
+const dkimOverlap = 7 * 24 * time.Hour
+
+// KeyGenerator produces a new DKIM key pair. It is injected so this package
+// does not depend on the crypto implementation.
+type KeyGenerator func(algorithm string) (privateKeyPEM []byte, publicKeyBase64 string, err error)
+
+// adminDkimAPI serves DKIM key management.
+//
+// It lives in this service, not in the auth service, because the private key
+// has to be sealed with the vault this process owns: the two services derive
+// their keys differently, so a key written elsewhere could never be opened
+// here - and a DKIM key that cannot be opened means unsigned outbound mail.
+type adminDkimAPI struct {
+	repo     *postgres.DkimRepository
+	generate KeyGenerator
+	sessions *WebSessionVerifier
+}
+
+// bearerOrCookie reads the session token from either transport.
+func bearerOrCookie(r *http.Request, cookieName string) string {
+	if header := r.Header.Get("Authorization"); strings.HasPrefix(header, "Bearer ") {
+		return strings.TrimPrefix(header, "Bearer ")
+	}
+	if cookie, err := r.Cookie(cookieName); err == nil {
+		return cookie.Value
+	}
+	return ""
+}
+
+func (a *adminDkimAPI) authenticate(w http.ResponseWriter, r *http.Request) bool {
+	token := bearerOrCookie(r, "lm_admin_session")
+
+	if token == "" {
+		writeError(w, http.StatusUnauthorized, "UNAUTHORIZED", "Admin session required")
+		return false
+	}
+	if _, err := a.sessions.RequireSurface(token, "admin"); err != nil {
+		writeError(w, http.StatusUnauthorized, "UNAUTHORIZED", "Admin session required")
+		return false
+	}
+	return true
+}
+
+func (a *adminDkimAPI) handleKeys(w http.ResponseWriter, r *http.Request) {
+	if !a.authenticate(w, r) {
+		return
+	}
+
+	domain := r.URL.Query().Get("domain")
+	if domain == "" {
+		writeError(w, http.StatusBadRequest, "DOMAIN_REQUIRED", "A domain is required")
+		return
+	}
+
+	keys, err := a.repo.ListKeys(r.Context(), domain)
+	if err != nil {
+		writeError(w, http.StatusInternalServerError, "INTERNAL_ERROR", "Could not list keys")
+		return
+	}
+	writeJSON(w, http.StatusOK, keys)
+}
+
+func (a *adminDkimAPI) handleRotate(w http.ResponseWriter, r *http.Request) {
+	if !a.authenticate(w, r) {
+		return
+	}
+	if r.Method != http.MethodPost {
+		writeError(w, http.StatusMethodNotAllowed, "METHOD_NOT_ALLOWED", "POST required")
+		return
+	}
+
+	var body struct {
+		Domain    string `json:"domain"`
+		Selector  string `json:"selector"`
+		Algorithm string `json:"algorithm"`
+	}
+	if err := json.NewDecoder(http.MaxBytesReader(w, r.Body, 4096)).Decode(&body); err != nil {
+		writeError(w, http.StatusBadRequest, "BAD_REQUEST", "Invalid JSON")
+		return
+	}
+
+	if body.Algorithm != "rsa2048" && body.Algorithm != "ed25519" {
+		writeError(w, http.StatusBadRequest, "INVALID_ALGORITHM", "Algorithm must be rsa2048 or ed25519")
+		return
+	}
+	if body.Domain == "" {
+		writeError(w, http.StatusBadRequest, "DOMAIN_REQUIRED", "A domain is required")
+		return
+	}
+	// A selector becomes part of a DNS name, so it is restricted to what is
+	// valid in one rather than trusted from the request.
+	if !isValidSelector(body.Selector) {
+		writeError(w, http.StatusBadRequest, "INVALID_SELECTOR",
+			"A selector may contain letters, digits and hyphens, up to 63 characters")
+		return
+	}
+
+	privateKeyPEM, publicKey, err := a.generate(body.Algorithm)
+	if err != nil {
+		writeError(w, http.StatusInternalServerError, "KEYGEN_FAILED", "Could not generate a key")
+		return
+	}
+
+	if err := a.repo.Rotate(r.Context(), body.Domain, body.Selector, body.Algorithm,
+		privateKeyPEM, dkimPublicRecord(body.Algorithm, publicKey), dkimOverlap); err != nil {
+		writeError(w, http.StatusInternalServerError, "ROTATE_FAILED", "Could not rotate the key")
+		return
+	}
+
+	writeJSON(w, http.StatusOK, map[string]any{
+		"domain":            body.Domain,
+		"selector":          body.Selector,
+		"algorithm":         body.Algorithm,
+		"public_key_record": dkimPublicRecord(body.Algorithm, publicKey),
+		"overlap_days":      int(dkimOverlap.Hours() / 24),
+	})
+}
+
+// dkimPublicRecord renders the TXT value a resolver expects.
+func dkimPublicRecord(algorithm, publicKeyBase64 string) string {
+	keyType := "rsa"
+	if algorithm == "ed25519" {
+		keyType = "ed25519"
+	}
+	return "v=DKIM1; k=" + keyType + "; p=" + publicKeyBase64
+}
+
+func isValidSelector(selector string) bool {
+	if selector == "" || len(selector) > 63 {
+		return false
+	}
+	for _, r := range selector {
+		switch {
+		case r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z', r >= '0' && r <= '9', r == '-':
+		default:
+			return false
+		}
+	}
+	return true
+}
+
+// SetAdminDkimAPI wires DKIM management. Without a session secret the routes
+// answer 503 rather than opening unauthenticated.
+func (r *Router) SetAdminDkimAPI(repo *postgres.DkimRepository, generate KeyGenerator, sessionSecret string) {
+	if repo == nil || generate == nil || sessionSecret == "" {
+		return
+	}
+	r.dkim = &adminDkimAPI{repo: repo, generate: generate, sessions: NewWebSessionVerifier(sessionSecret)}
+}
+
+func (r *Router) handleAdminDkimKeys(w http.ResponseWriter, req *http.Request) {
+	if r.dkim == nil {
+		writeError(w, http.StatusServiceUnavailable, "DKIM_API_DISABLED",
+			"DKIM management needs JWT_SECRET and a master key")
+		return
+	}
+	r.dkim.handleKeys(w, req)
+}
+
+func (r *Router) handleAdminDkimRotate(w http.ResponseWriter, req *http.Request) {
+	if r.dkim == nil {
+		writeError(w, http.StatusServiceUnavailable, "DKIM_API_DISABLED",
+			"DKIM management needs JWT_SECRET and a master key")
+		return
+	}
+	r.dkim.handleRotate(w, req)
+}
@@ -0,0 +1,107 @@
+package httppresentation
+
+import (
+	"net/http"
+	"time"
+)
+
+// TlsStatusSource reports what the certificate watcher actually sees. The
+// admin console used to receive fixed values - "VALID", 72 days - which told
+// an operator the certificate was healthy no matter what the process held.
+type TlsStatusSource interface {
+	EarliestExpiry() (host string, expiry time.Time, ok bool)
+	LastReload() time.Time
+	LastChange() time.Time
+	UnknownSNICount() int64
+	HasCertificateFor(host string) bool
+}
+
+type adminTlsAPI struct {
+	source     TlsStatusSource
+	mailHost   string
+	tlsMode    string
+	sessions   *WebSessionVerifier
+	pollPeriod time.Duration
+}
+
+func (a *adminTlsAPI) handleStatus(w http.ResponseWriter, r *http.Request) {
+	token := bearerOrCookie(r, "lm_admin_session")
+	if token == "" {
+		writeError(w, http.StatusUnauthorized, "UNAUTHORIZED", "Admin session required")
+		return
+	}
+	if _, err := a.sessions.RequireSurface(token, "admin"); err != nil {
+		writeError(w, http.StatusUnauthorized, "UNAUTHORIZED", "Admin session required")
+		return
+	}
+
+	status := map[string]any{
+		"tls_mode":            a.tlsMode,
+		"mail_host":           a.mailHost,
+		"has_certificate":     a.source.HasCertificateFor(a.mailHost),
+		"unknown_sni_total":   a.source.UnknownSNICount(),
+		"last_reload":         nil,
+		"last_change":         nil,
+		"watcher_healthy":     false,
+		"certificate_expires": nil,
+		"expires_in_days":     nil,
+		"state":               "UNKNOWN",
+	}
+
+	if last := a.source.LastReload(); !last.IsZero() {
+		status["last_reload"] = last.UTC().Format(time.RFC3339)
+		// Two missed polls is the smallest gap that is not a scheduling
+		// hiccup, and it is the symptom of a watcher gone blind (risk R2).
+		status["watcher_healthy"] = time.Since(last) < 3*a.pollPeriod
+	}
+	if change := a.source.LastChange(); !change.IsZero() {
+		status["last_change"] = change.UTC().Format(time.RFC3339)
+	}
+
+	if host, expiry, ok := a.source.EarliestExpiry(); ok {
+		remaining := time.Until(expiry)
+		status["certificate_expires"] = expiry.UTC().Format(time.RFC3339)
+		status["expires_in_days"] = int(remaining.Hours() / 24)
+		status["soonest_expiring_host"] = host
+
+		switch {
+		case remaining <= 0:
+			status["state"] = "EXPIRED"
+		case remaining < 24*time.Hour:
+			status["state"] = "CRITICAL"
+		case remaining < 7*24*time.Hour:
+			status["state"] = "WARNING"
+		default:
+			status["state"] = "OK"
+		}
+	}
+
+	if !status["has_certificate"].(bool) {
+		status["state"] = "NO_CERTIFICATE"
+	}
+
+	writeJSON(w, http.StatusOK, status)
+}
+
+// SetAdminTlsAPI wires the TLS panel to the live certificate source.
+func (r *Router) SetAdminTlsAPI(source TlsStatusSource, mailHost, tlsMode, sessionSecret string, pollPeriod time.Duration) {
+	if source == nil || sessionSecret == "" {
+		return
+	}
+	if pollPeriod <= 0 {
+		pollPeriod = time.Minute
+	}
+	r.tls = &adminTlsAPI{
+		source: source, mailHost: mailHost, tlsMode: tlsMode,
+		sessions: NewWebSessionVerifier(sessionSecret), pollPeriod: pollPeriod,
+	}
+}
+
+func (r *Router) handleAdminTls(w http.ResponseWriter, req *http.Request) {
+	if r.tls == nil {
+		writeError(w, http.StatusServiceUnavailable, "TLS_API_DISABLED",
+			"The TLS panel needs a certificate watcher and JWT_SECRET")
+		return
+	}
+	r.tls.handleStatus(w, req)
+}
@@ -0,0 +1,15 @@
+package httppresentation_test
+
+import (
+	"testing"
+
+	tlsprovider "lambdamail/protocols/internal/infrastructure/tls"
+	httppresentation "lambdamail/protocols/internal/presentation/http"
+)
+
+// The TLS panel is wired by a type assertion in the composition root, so a
+// change to either side would silently leave the panel reporting
+// "TLS_API_DISABLED" in production rather than failing the build.
+func TestAcmeCertWatcherSatisfiesTlsStatusSource(t *testing.T) {
+	var _ httppresentation.TlsStatusSource = (*tlsprovider.AcmeCertWatcher)(nil)
+}
@@ -0,0 +1,81 @@
+package httppresentation
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+)
+
+// buildMessage returns a message carrying one attachment with the given name
+// and content type. Both are chosen by whoever sent the mail, which is the
+// whole point: neither can be trusted.
+func buildMessage(filename, contentType string) []byte {
+	return []byte("From: sender@remote.test\r\nSubject: s\r\n" +
+		"Content-Type: multipart/mixed; boundary=B\r\n\r\n" +
+		"--B\r\nContent-Type: text/plain\r\n\r\nbody\r\n" +
+		"--B\r\nContent-Type: " + contentType + "\r\n" +
+		"Content-Disposition: attachment; filename=\"" + filename + "\"\r\n\r\n" +
+		"<script>alert(1)</script>\r\n--B--\r\n")
+}
+
+func download(t *testing.T, raw []byte, requested string) *http.Response {
+	t.Helper()
+	rec := httptest.NewRecorder()
+	(&mailAPI{}).handleAttachmentDownload(rec, httptest.NewRequest(http.MethodGet, "/x", nil), raw, requested)
+	return rec.Result()
+}
+
+// Serving an attachment's own text/html back on the webmail's origin is stored
+// XSS: the sender picks the type, and anyone can send this mailbox a message.
+func TestAttachmentDownload_DowngradesRenderableContentTypes(t *testing.T) {
+	for _, declared := range []string{"text/html", "image/svg+xml", "application/xhtml+xml", "text/javascript"} {
+		res := download(t, buildMessage("payload", declared), "payload")
+		if got := res.Header.Get("Content-Type"); got != "application/octet-stream" {
+			t.Errorf("%s was served as %q, want application/octet-stream", declared, got)
+		}
+	}
+}
+
+func TestAttachmentDownload_KeepsHarmlessContentTypes(t *testing.T) {
+	res := download(t, buildMessage("photo.png", "image/png"), "photo.png")
+	if got := res.Header.Get("Content-Type"); got != "image/png" {
+		t.Errorf("Content-Type = %q, want image/png", got)
+	}
+}
+
+// A quote in the filename used to break out of the Content-Disposition
+// parameter, which is reachable by any sender.
+func TestAttachmentDownload_NeutralisesHostileFilenames(t *testing.T) {
+	for _, name := range []string{`a"; x="y`, "a\r\nX-Injected: yes", "../../etc/passwd", "a\x00b"} {
+		res := download(t, buildMessage(name, "application/pdf"), name)
+		disposition := res.Header.Get("Content-Disposition")
+
+		if strings.Contains(disposition, "X-Injected") {
+			t.Errorf("filename %q injected a header: %q", name, disposition)
+		}
+		if strings.Count(disposition, `"`) > 2 {
+			t.Errorf("filename %q broke out of the quoted parameter: %q", name, disposition)
+		}
+		if strings.Contains(disposition, "/") {
+			t.Errorf("filename %q kept a path separator: %q", name, disposition)
+		}
+	}
+}
+
+func TestAttachmentDownload_SetsSniffingAndSandboxGuards(t *testing.T) {
+	res := download(t, buildMessage("doc.pdf", "application/pdf"), "doc.pdf")
+	if res.Header.Get("X-Content-Type-Options") != "nosniff" {
+		t.Error("X-Content-Type-Options: nosniff is missing, so a browser may sniff the bytes as HTML")
+	}
+	if !strings.Contains(res.Header.Get("Content-Security-Policy"), "sandbox") {
+		t.Error("the response is not sandboxed by CSP")
+	}
+}
+
+func TestAttachmentDownload_MissingAttachmentIs404(t *testing.T) {
+	res := download(t, buildMessage("real.pdf", "application/pdf"), "nonexistent.pdf")
+	if res.StatusCode != http.StatusNotFound {
+		t.Errorf("status = %d, want 404", res.StatusCode)
+	}
+}
@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"encoding/json"
 	"errors"
+	"mime"
 	"net/http"
 	"strconv"
 	"strings"
@@ -151,17 +152,67 @@ func (m *mailAPI) handleAttachmentDownload(w http.ResponseWriter, _ *http.Reques
 		return
 	}

-	if contentType == "" {
-		contentType = "application/octet-stream"
-	}
-
-	w.Header().Set("Content-Type", contentType)
-	w.Header().Set("Content-Disposition", "attachment; filename=\""+filename+"\"")
+	// The sender chose this filename and this content type, so neither is
+	// trusted. Serving an attachment's own text/html back on the webmail's
+	// origin is stored XSS, and a quote in the name breaks out of the
+	// Content-Disposition parameter - both reachable by anyone who can send
+	// this mailbox a message.
+	w.Header().Set("Content-Type", safeAttachmentContentType(contentType))
+	w.Header().Set("Content-Disposition",
+		mime.FormatMediaType("attachment", map[string]string{"filename": sanitiseFilename(filename)}))
+	// Without nosniff a browser may still sniff the bytes and render them as
+	// HTML regardless of the declared type.
+	w.Header().Set("X-Content-Type-Options", "nosniff")
+	w.Header().Set("Content-Security-Policy", "default-src 'none'; sandbox")
 	w.Header().Set("Content-Length", strconv.Itoa(len(data)))
-	w.WriteHeader(http.StatusOK)
 	_, _ = w.Write(data)
 }

+// renderableTypes are the content types a browser will execute or render in a
+// document context. They are downgraded so an attachment can never become a
+// page on this origin.
+var renderableTypes = map[string]bool{
+	"text/html":                     true,
+	"application/xhtml+xml":         true,
+	"image/svg+xml":                 true,
+	"application/xml":               true,
+	"text/xml":                      true,
+	"application/javascript":        true,
+	"text/javascript":               true,
+	"application/x-javascript":      true,
+	"application/xhtml":             true,
+	"application/vnd.wap.xhtml+xml": true,
+}
+
+func safeAttachmentContentType(declared string) string {
+	declared = strings.ToLower(strings.TrimSpace(declared))
+	if declared == "" || renderableTypes[declared] {
+		return "application/octet-stream"
+	}
+	return declared
+}
+
+// sanitiseFilename keeps a name usable in a header and on a filesystem: no
+// path separators, no control characters, no quotes.
+func sanitiseFilename(name string) string {
+	name = strings.Map(func(r rune) rune {
+		switch {
+		case r < 0x20 || r == 0x7f:
+			return -1
+		case r == '"' || r == '\\' || r == '/' || r == ':':
+			return '_'
+		}
+		return r
+	}, name)
+	name = strings.TrimSpace(name)
+	if name == "" || name == "." || name == ".." {
+		return "attachment"
+	}
+	if len(name) > 200 {
+		name = name[:200]
+	}
+	return name
+}

 func (m *mailAPI) handleSeen(w http.ResponseWriter, r *http.Request) {
 	session, ok := m.authenticate(w, r)
@@ -29,6 +29,11 @@ type Router struct {
 	// events pushes new mail to the browser. Like mail, it stays nil without a
 	// session secret.
 	events *EventHub
+	// dkim manages signing keys. It lives here because the private key must be
+	// sealed with this process's vault.
+	dkim *adminDkimAPI
+	// tls reports what the certificate watcher sees, rather than a constant.
+	tls *adminTlsAPI
 	// degradedFunc reports a condition that leaves the service running but
 	// not fit for production, the clearest case being a self-signed
 	// certificate standing in for one Traefik never issued
@@ -71,6 +76,9 @@ func (r *Router) registerRoutes() {
 	r.mux.HandleFunc("/.well-known/autoconfig/mail/config-v1.1.xml", r.handleThunderbirdAutoconfig)
 	r.mux.HandleFunc("/autodiscover/autodiscover.xml", r.handleOutlookAutodiscover)
 	r.mux.HandleFunc("/api/v1/events", r.handleEvents)
+	r.mux.HandleFunc("/api/v1/admin/tls", r.handleAdminTls)
+	r.mux.HandleFunc("/api/v1/admin/dkim/keys", r.handleAdminDkimKeys)
+	r.mux.HandleFunc("/api/v1/admin/dkim/rotate", r.handleAdminDkimRotate)
 	r.mux.HandleFunc("/api/v1/mail/folders", r.handleMailFolders)
 	r.mux.HandleFunc("/api/v1/mail/messages", r.handleMailMessages)
 	r.mux.HandleFunc("/api/v1/mail/message/", r.handleMailMessage)