From 271b22fa1f857762eb1e48c0ec8e7037ff0e5232 Mon Sep 17 00:00:00 2001 From: AJ ONeal Date: Mon, 20 Apr 2026 23:20:13 -0600 Subject: [PATCH] refactor(formmailer): ergonomic cleanups after prod deploy learnings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Collapse SuccessBody/SuccessBodyFunc (and Error*) into a single required func() []byte. Callers build closures with baked-in fallbacks; one field, one code path, no priority ordering. - Split EHLO local name from peer hostname. Add LocalName field + default to os.Hostname(). smtp.NewClient gets the peer name (for AUTH + TLS binding); c.Hello(localName) overrides the EHLO greeting separately. Stdlib conflates these but they're semantically distinct. - Add TLSConfig override field (supports custom CAs, cert pinning, and test-time InsecureSkipVerify). Default is still ServerName: . - Add TrustedProxies []netip.Prefix. X-Forwarded-For is now only honored when r.RemoteAddr is in the trusted set — otherwise any client can forge their IP to bypass rate limits, blacklist, and geo gating. - Integration tests drive sendMail against a fake SMTP server that does STARTTLS + AUTH PLAIN. Regression test for StartTLS(nil) panic (Go 1.22+ requires ServerName or InsecureSkipVerify). cmd/form2mail: - Extract hotReload() helper; replaces the inline closures. - silentDropRU looks up the email FormName from Fields by Kind instead of hardcoding "input_3" (survives field renames). - Set TrustedProxies to loopback (proxy runs on same host). - scripts/deploy.sh automates the rename-first / scp / restart / verify / cleanup cycle — leaves .old in place on verification failure for rollback. --- cmd/form2mail/form2mail.go | 113 ++++++----- cmd/form2mail/scripts/deploy.sh | 42 ++++ net/formmailer/formmailer.go | 110 +++++++---- net/formmailer/sendmail_test.go | 328 ++++++++++++++++++++++++++++++++ 4 files changed, 512 insertions(+), 81 deletions(-) create mode 100755 cmd/form2mail/scripts/deploy.sh create mode 100644 net/formmailer/sendmail_test.go diff --git a/cmd/form2mail/form2mail.go b/cmd/form2mail/form2mail.go index 17fc378..f87cedf 100644 --- a/cmd/form2mail/form2mail.go +++ b/cmd/form2mail/form2mail.go @@ -8,6 +8,7 @@ import ( "io" "log" "net/http" + "net/netip" "os" "os/signal" "path/filepath" @@ -135,31 +136,18 @@ func main() { // Verify templates are readable at startup; re-read on each request so // operators can edit HTML without restarting (matches legacy behavior). - successBody, err := os.ReadFile(cfg.successFile) + successFallback, err := os.ReadFile(cfg.successFile) if err != nil { fmt.Fprintf(os.Stderr, "\nError: couldn't read success response file %q: %v\n\n", cfg.successFile, err) os.Exit(1) } - if _, err := os.ReadFile(cfg.errorFile); err != nil { + errorFallback, err := os.ReadFile(cfg.errorFile) + if err != nil { fmt.Fprintf(os.Stderr, "\nError: couldn't read error response file %q: %v\n\n", cfg.errorFile, err) os.Exit(1) } - successBodyFn := func() []byte { - b, err := os.ReadFile(cfg.successFile) - if err != nil { - log.Printf("success-file read: %v", err) - return successBody - } - return b - } - errorBodyFn := func() []byte { - b, err := os.ReadFile(cfg.errorFile) - if err != nil { - log.Printf("error-file read: %v", err) - return nil - } - return b - } + successBody := hotReload(cfg.successFile, successFallback) + errorBody := hotReload(cfg.errorFile, errorFallback) if cfg.smtpUser == "" { cfg.smtpUser = cfg.smtpFrom @@ -262,33 +250,41 @@ func main() { } fmt.Fprintf(os.Stderr, "%s\n", time.Since(tGeo).Round(time.Millisecond)) + fields := []formmailer.Field{ + {Label: "Name", FormName: "input_1", Kind: formmailer.KindText}, + {Label: "Email", FormName: "input_3", Kind: formmailer.KindEmail}, + {Label: "Phone", FormName: "input_4", Kind: formmailer.KindPhone}, + {Label: "Company", FormName: "input_5", Kind: formmailer.KindText}, + {Label: "Message", FormName: "input_7", Kind: formmailer.KindMessage}, + } fm := &formmailer.FormMailer{ - SMTPHost: cfg.smtpHost, - SMTPFrom: cfg.smtpFrom, - SMTPTo: strings.Split(cfg.smtpToList, ","), - SMTPUser: cfg.smtpUser, - SMTPPass: cfg.smtpPass, - Subject: cfg.smtpSubject, - SuccessBody: successBody, // fallback if read fails - SuccessBodyFunc: successBodyFn, - ErrorBodyFunc: errorBodyFn, - ContentType: cfg.responseType, - // Legacy behavior: bot/blacklist rejections render {.SupportEmail} - // as "[REDACTED]" rather than leaving it blank. + SMTPHost: cfg.smtpHost, + SMTPFrom: cfg.smtpFrom, + SMTPTo: strings.Split(cfg.smtpToList, ","), + SMTPUser: cfg.smtpUser, + SMTPPass: cfg.smtpPass, + Subject: cfg.smtpSubject, + SuccessBody: successBody, + ErrorBody: errorBody, + ContentType: cfg.responseType, + // Bot/blacklist rejections render {.SupportEmail} as "[REDACTED]" + // rather than leaking the real address. Validation errors (wrong + // format, missing field) still show the real support email so users + // know where to write in. HiddenSupportValue: "[REDACTED]", - Blacklist: blacklist, - Geo: geo, + // Only honor X-Forwarded-For from loopback (our reverse proxy runs + // on the same host). Prevents spoofing rate limits and geo-gating. + TrustedProxies: []netip.Prefix{ + netip.MustParsePrefix("127.0.0.0/8"), + netip.MustParsePrefix("::1/128"), + }, + Blacklist: blacklist, + Geo: geo, // North America + unknown. Unknown ("") is always allowed by formmailer. AllowedCountries: []string{"US", "CA", "MX", "CR", "VI"}, - Fields: []formmailer.Field{ - {Label: "Name", FormName: "input_1", Kind: formmailer.KindText}, - {Label: "Email", FormName: "input_3", Kind: formmailer.KindEmail}, - {Label: "Phone", FormName: "input_4", Kind: formmailer.KindPhone}, - {Label: "Company", FormName: "input_5", Kind: formmailer.KindText}, - {Label: "Message", FormName: "input_7", Kind: formmailer.KindMessage}, - }, - RPM: requestsPerMinute, - Burst: burstSize, + Fields: fields, + RPM: requestsPerMinute, + Burst: burstSize, } ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) @@ -302,7 +298,14 @@ func main() { }) mux := http.NewServeMux() - contact := silentDropRU(fm, successBody, cfg.responseType) + emailFormName := "" + for _, f := range fields { + if f.Kind == formmailer.KindEmail { + emailFormName = f.FormName + break + } + } + contact := silentDropRU(fm, emailFormName, successBody, cfg.responseType) mux.Handle("POST /contact", contact) mux.Handle("POST /contact/", contact) mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { @@ -341,19 +344,19 @@ func main() { } // silentDropRU returns a handler that silently returns the success body for -// submissions whose "input_3" email ends with ".ru" — legacy spam-trap -// behavior from the original form2mail. All other submissions fall through -// to fm. -func silentDropRU(fm http.Handler, successBody []byte, contentType string) http.Handler { +// submissions whose email (emailFormName form input) ends with ".ru" — +// legacy spam-trap behavior from the original form2mail. All other +// submissions fall through to fm. +func silentDropRU(fm http.Handler, emailFormName string, successBody func() []byte, contentType string) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { // ParseMultipartForm is safe to call twice; formmailer will see the // already-parsed form. Use a bounded reader to match formmailer's cap. r.Body = http.MaxBytesReader(w, r.Body, 10*1024) if err := r.ParseMultipartForm(10 * 1024); err == nil { - email := strings.ToLower(strings.TrimSpace(r.FormValue("input_3"))) + email := strings.ToLower(strings.TrimSpace(r.FormValue(emailFormName))) if strings.HasSuffix(email, ".ru") { w.Header().Set("Content-Type", contentType) - _, _ = w.Write(successBody) + _, _ = w.Write(successBody()) return } } @@ -361,6 +364,20 @@ func silentDropRU(fm http.Handler, successBody []byte, contentType string) http. }) } +// hotReload returns a function that re-reads path on each call, falling back +// to the provided bytes on read error (logged). Used for templates that +// operators may edit out-of-band. +func hotReload(path string, fallback []byte) func() []byte { + return func() []byte { + b, err := os.ReadFile(path) + if err != nil { + log.Printf("%s read: %v", path, err) + return fallback + } + return b + } +} + func inferContentType(path string) string { switch strings.ToLower(filepath.Ext(path)) { case ".html", ".htm": diff --git a/cmd/form2mail/scripts/deploy.sh b/cmd/form2mail/scripts/deploy.sh new file mode 100755 index 0000000..b4a3605 --- /dev/null +++ b/cmd/form2mail/scripts/deploy.sh @@ -0,0 +1,42 @@ +#!/bin/sh +# Cross-compile form2mail for linux-amd64 and deploy to @. +# Usage: scripts/deploy.sh @ [service-name] +# +# Uses the rename-old-first pattern (avoids scp ETXTBSY on running ELFs), +# restarts via systemctl, verifies --version runs from the new inode, then +# removes the .old backup. Leaves .old in place if verification fails so +# rollback is `mv ~/bin/.old ~/bin/`. +set -eu + +g_remote="${1:?usage: $0 @ [service-name]}" +g_service="${2:-form2mail}" +g_bin="form2mail" + +cd "$(dirname "$0")/.." + +echo "Building ${g_bin} for linux-amd64..." +GOOS=linux GOARCH=amd64 go build -o "${g_bin}-linux" . + +echo "Renaming old binary on ${g_remote}..." +ssh "${g_remote}" "mv ~/bin/${g_bin} ~/bin/${g_bin}.old" + +echo "Copying new binary..." +scp "${g_bin}-linux" "${g_remote}:~/bin/${g_bin}" + +echo "Restarting ${g_service}..." +ssh "${g_remote}" "sudo systemctl restart ${g_service}" +sleep 2 + +echo "Verifying..." +if ! ssh "${g_remote}" "systemctl is-active ${g_service}" > /dev/null; then + echo " FAILED: ${g_service} not active. Logs:" + ssh "${g_remote}" "sudo journalctl -u ${g_service} -n 20 --no-pager" + echo " Backup still at ~/bin/${g_bin}.old — roll back with:" + echo " ssh ${g_remote} 'mv ~/bin/${g_bin}.old ~/bin/${g_bin} && sudo systemctl restart ${g_service}'" + exit 1 +fi + +echo "Removing backup..." +ssh "${g_remote}" "rm ~/bin/${g_bin}.old" + +echo "OK: ${g_service} deployed and active." diff --git a/net/formmailer/formmailer.go b/net/formmailer/formmailer.go index fefb341..7388e81 100644 --- a/net/formmailer/formmailer.go +++ b/net/formmailer/formmailer.go @@ -45,6 +45,7 @@ import ( "net/mail" "net/netip" "net/smtp" + "os" "regexp" "slices" "strings" @@ -132,28 +133,42 @@ type FormMailer struct { SMTPUser string SMTPPass string Subject string // may contain {.Email} + // LocalName is the hostname the client announces in EHLO. Zero uses + // os.Hostname(). Matters for relay (port 25) where receivers may reject + // EHLO names that don't reverse-resolve; on submission (587) it's + // informational. NEVER the peer's name — that's TLS SNI / auth binding. + LocalName string + // TLSConfig overrides the StartTLS tls.Config. Zero uses + // &tls.Config{ServerName: }. Set to supply a custom + // root CA bundle, to disable verification (in tests), or to pin a cert. + TLSConfig *tls.Config // SMTPTimeout bounds the entire connect+auth+send cycle. Zero uses 5s. SMTPTimeout time.Duration // MXTimeout bounds the per-submission MX lookup. Zero uses 2s. MXTimeout time.Duration - // SuccessBody and ErrorBody are the response bodies sent to the client. - // ErrorBody may contain {.Error} and {.SupportEmail} placeholders. - SuccessBody []byte - ErrorBody []byte - // SuccessBodyFunc / ErrorBodyFunc, if set, are called per request and - // override SuccessBody / ErrorBody. Use for hot-reloadable templates - // (e.g. re-read an HTML file on every request). - SuccessBodyFunc func() []byte - ErrorBodyFunc func() []byte - ContentType string // inferred from SuccessBody if empty + // SuccessBody and ErrorBody are called per request and return the response + // body sent to the client. Callers typically build them as closures that + // re-read an HTML file each invocation (hot-reload) with a baked-in fallback + // to a copy read at startup. ErrorBody output may contain {.Error} and + // {.SupportEmail} placeholders. + SuccessBody func() []byte + ErrorBody func() []byte + ContentType string // inferred from a probe of SuccessBody() if empty - // HiddenSupportValue replaces {.SupportEmail} on error responses for - // requests that should not learn the operator's address (blacklist / bot - // rejections). Zero value "" hides the placeholder entirely. + // HiddenSupportValue replaces {.SupportEmail} in ErrorBody output when the + // request should not learn the operator's real address — i.e. bot and + // blacklist rejections. Legitimate validation errors see the real support + // email. Zero value "" strips the placeholder entirely. HiddenSupportValue string + // TrustedProxies — list of CIDRs whose X-Forwarded-For header we honor. + // Empty means never trust XFF (use r.RemoteAddr as-is). Without this, + // any client could forge their IP to bypass rate limiting, blacklist, + // and country gating. + TrustedProxies []netip.Prefix + // Blacklist — if set, matching IPs are rejected before any other processing. Blacklist *dataset.View[ipcohort.Cohort] @@ -204,27 +219,24 @@ func (fm *FormMailer) init() { } func (fm *FormMailer) successBody() []byte { - if fm.SuccessBodyFunc != nil { - return fm.SuccessBodyFunc() + if fm.SuccessBody == nil { + return nil } - return fm.SuccessBody + return fm.SuccessBody() } func (fm *FormMailer) errorBody() []byte { - if fm.ErrorBodyFunc != nil { - return fm.ErrorBodyFunc() + if fm.ErrorBody == nil { + return nil } - return fm.ErrorBody + return fm.ErrorBody() } func (fm *FormMailer) contentType() string { if fm.ContentType != "" { return fm.ContentType } - probe := fm.SuccessBody - if len(probe) == 0 && fm.SuccessBodyFunc != nil { - probe = fm.SuccessBodyFunc() - } + probe := fm.successBody() if bytes.Contains(probe[:min(512, len(probe))], []byte(", chain verification succeeds. + TLSConfig: &tls.Config{ + ServerName: host, // the default uses this + RootCAs: poolWith(cert.Leaf), + }, + SMTPTimeout: 5 * time.Second, + } + + if err := fm.sendMail(context.Background(), []byte("Subject: x\r\n\r\n")); err != nil { + t.Fatalf("sendMail: %v", err) + } + wg.Wait() + if serverErr != nil { + t.Fatalf("server: %v", serverErr) + } +} + +type fakeCapture struct { + ehlo *string + auth *string + from *string + rcpt *string + body *strings.Builder +} + +// runFakeSMTP speaks just enough SMTP to drive sendMail through a successful +// send: 220 banner, EHLO with STARTTLS + AUTH extensions, STARTTLS upgrade, +// AUTH PLAIN accept, MAIL, RCPT, DATA, QUIT. +func runFakeSMTP(conn net.Conn, cert tls.Certificate, cap *fakeCapture, fail func(error)) { + r := bufio.NewReader(conn) + w := bufio.NewWriter(conn) + write := func(s string) { _, _ = w.WriteString(s); _ = w.Flush() } + + write("220 fake.smtp ESMTP ready\r\n") + line, err := r.ReadString('\n') + if err != nil { + fail(err) + return + } + if !strings.HasPrefix(strings.ToUpper(line), "EHLO ") { + fail(errf("expected EHLO, got %q", line)) + return + } + if cap.ehlo != nil { + *cap.ehlo = strings.TrimSpace(line[5:]) + } + write("250-fake.smtp\r\n250-STARTTLS\r\n250 AUTH PLAIN\r\n") + + line, err = r.ReadString('\n') + if err != nil { + fail(err) + return + } + if !strings.EqualFold(strings.TrimSpace(line), "STARTTLS") { + fail(errf("expected STARTTLS, got %q", line)) + return + } + write("220 Ready to start TLS\r\n") + + tlsConn := tls.Server(conn, &tls.Config{Certificates: []tls.Certificate{cert}}) + if err := tlsConn.Handshake(); err != nil { + fail(err) + return + } + r = bufio.NewReader(tlsConn) + w = bufio.NewWriter(tlsConn) + write = func(s string) { _, _ = w.WriteString(s); _ = w.Flush() } + + // Post-STARTTLS: expect EHLO again, then AUTH, MAIL, RCPT, DATA. + for { + line, err = r.ReadString('\n') + if err != nil { + return + } + upper := strings.ToUpper(strings.TrimSpace(line)) + switch { + case strings.HasPrefix(upper, "EHLO "): + write("250-fake.smtp\r\n250 AUTH PLAIN\r\n") + case strings.HasPrefix(upper, "AUTH PLAIN"): + if cap.auth != nil { + *cap.auth = strings.TrimSpace(line[len("AUTH PLAIN"):]) + } + write("235 Authenticated\r\n") + case strings.HasPrefix(upper, "MAIL FROM:"): + if cap.from != nil { + *cap.from = extractAddr(line) + } + write("250 OK\r\n") + case strings.HasPrefix(upper, "RCPT TO:"): + if cap.rcpt != nil { + *cap.rcpt = extractAddr(line) + } + write("250 OK\r\n") + case upper == "DATA": + write("354 End data with .\r\n") + for { + bl, err := r.ReadString('\n') + if err != nil { + return + } + if bl == ".\r\n" || strings.TrimRight(bl, "\r\n") == "." { + break + } + if cap.body != nil { + cap.body.WriteString(bl) + } + } + write("250 OK\r\n") + case upper == "QUIT": + write("221 Bye\r\n") + return + case strings.HasPrefix(upper, "RSET"): + write("250 OK\r\n") + default: + write("500 unknown\r\n") + } + } +} + +func extractAddr(line string) string { + if i := strings.Index(line, "<"); i >= 0 { + if j := strings.Index(line[i:], ">"); j > 0 { + return line[i+1 : i+j] + } + } + return strings.TrimSpace(line) +} + +type errString string + +func (e errString) Error() string { return string(e) } +func errf(format string, args ...any) error { + // cheap sprintf without importing fmt (which we already import, but keep + // the test file tidy). + s := format + for _, a := range args { + idx := strings.Index(s, "%") + if idx < 0 { + break + } + s = s[:idx] + toString(a) + s[idx+2:] + } + return errString(s) +} +func toString(a any) string { + switch v := a.(type) { + case string: + return v + case error: + return v.Error() + default: + return "" + } +} + +// selfSignedCert returns a fresh ECDSA self-signed cert/key usable as both +// server cert and a root for client verification in the nil-TLSConfig test. +func selfSignedCert(t *testing.T) tls.Certificate { + t.Helper() + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatal(err) + } + tpl := &x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: "fake.smtp"}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), + IsCA: true, + IPAddresses: []net.IP{net.ParseIP("127.0.0.1")}, + DNSNames: []string{"fake.smtp", "127.0.0.1"}, + KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + } + der, err := x509.CreateCertificate(rand.Reader, tpl, tpl, &key.PublicKey, key) + if err != nil { + t.Fatal(err) + } + leaf, err := x509.ParseCertificate(der) + if err != nil { + t.Fatal(err) + } + return tls.Certificate{Certificate: [][]byte{der}, PrivateKey: key, Leaf: leaf} +} + +func poolWith(cert *x509.Certificate) *x509.CertPool { + p := x509.NewCertPool() + p.AddCert(cert) + return p +} +