Unverified Commit b3989fa4 authored by Minseong Choi's avatar Minseong Choi 💬
Browse files

fix(mail): prove SMTP deliverability before saving, and stop losing the relay

A live install passed the SMTP setup screen and then failed every one-time
code with a bare `internal error`. Four separate defects had to line up for
that, and each is fixed here.

The relay was configured with `from = noreply@<domain-A>` on an account
authenticated as `<user>@<domain-B>`. Providers that validate sender identity
— Fastmail among them — answer MAIL FROM with an unconditional 250 and only
refuse at end-of-DATA. Ping stopped at NOOP, so it never saw the refusal: the
wizard reported success, wrote the config, rolled felis-api, and every OTP
afterwards died at w.Close().

Ping now runs the same transaction a real code takes — connect, (STARTTLS,)
AUTH, MAIL FROM, RCPT TO, DATA — delivering one self-test message to the From
address, and SendOTP and Ping share deliver() so the check cannot drift from
the thing it checks. The self-test recipient cannot cause a false negative:
an authenticated submission relay accepts RCPT for any destination by
definition, while the sender identity it does validate is exactly what we
want tested. The setup screen now says a message will be sent, names the
address it went to, and warns that From must be an address the account is
allowed to send as.

A relay refusal also answered 500 `internal`, which reads as a broken panel
and sends the operator hunting through handler code instead of their [smtp]
block. It is now 502 `mail_undeliverable`, mapped inside deliverOTP so all
four doors that mail a code (onboarding, email login, op-login, migrate
step-up) answer alike. The relay's own text stays out of the response — it
can name the SMTP account, and these routes are reachable by any signed-in
player — and goes to the log instead.

writeError logged nothing when it collapsed an unmapped error to 500, so an
operator holding an `internal error` had nothing to grep for and diagnosis
degraded into guessing against a live install. It now logs the method, path,
wrapped chain and the same request_id the caller is shown.

Finally, write_felis_toml regenerated the config wholesale and never emitted
[smtp], so re-running the installer — the documented way to update felis-api —
silently erased a working relay and reverted OTP delivery to the no-Mailer
path, logging codes instead of sending them. It now carries the block forward,
cached on first read because the host toml is clobbered before the pod toml is
written. Same defect family as the root_domain loss fixed in ecbeb207: a
generated file holding a hand-set value with no carry-forward.

Tests cover the case a MAIL FROM probe cannot see: a fake relay that answers
250 to MAIL FROM and 550 at end-of-DATA must fail both Ping and SendOTP, and
the 502 must carry a distinct machine code without leaking the relay's text.
parent 32be3e17
Loading
Loading
Loading
Loading
+18 −6
Changes for cmd/felis/tui_smtp.go: 18 added lines, 6 removed lines.
Original line number Diff line number Diff line
@@ -89,7 +89,7 @@ func (m *smtpModel) build() *huh.Form {
	return m.sized(newFelisForm(huh.NewGroup(
		huh.NewNote().
			Title("Email (SMTP)").
			Description("The relay Felis mails one-time codes through — email verification, email login and operator sign-in all need it. The password goes into a Kubernetes Secret; only the other fields are written to felis.toml."),
			Description("The relay Felis mails one-time codes through — email verification, email login and operator sign-in all need it. The password goes into a Kubernetes Secret; only the other fields are written to felis.toml. Saving sends one self-test message to the From address: nothing is written unless it is delivered."),
		huh.NewInput().
			Title("SMTP host").
			Description("Your provider's relay, e.g. smtp.gmail.com or smtp.mailgun.org.").
@@ -102,7 +102,7 @@ func (m *smtpModel) build() *huh.Form {
			Validate(validateSMTPPort),
		huh.NewInput().
			Title("From address").
			Description("The sender codes are mailed as, e.g. felis@your-domain.").
			Description("The sender codes are mailed as, e.g. felis@your-domain. It must be an address this account is allowed to send as — providers reject a From on a domain you have not verified with them, and they usually do it only after the message body, not when you connect.").
			Value(&m.in.from).
			Validate(validateSMTPFrom),
		huh.NewInput().
@@ -225,11 +225,14 @@ func (m *smtpModel) normalizeInputs() {
func (m *smtpModel) View() string {
	switch m.step {
	case esWorking:
		return "  " + m.sp.View() + " " + tuiHint.Render("Verifying the relay, saving email settings and rolling the API…") + "\n"
		return "  " + m.sp.View() + " " + tuiHint.Render("Delivering a self-test message, saving email settings and rolling the API…") + "\n"
	case esDone:
		var b strings.Builder
		b.WriteString(tuiSuccessBanner("Email configured — codes are now mailed.") + "\n\n")
		b.WriteString(tuiInfo("Relay → "+smtpDetail(m.in)) + "\n")
		// Named because it is checkable: the operator can open that inbox and see the
		// proof, rather than taking "configured" on faith.
		b.WriteString(tuiHint.Render("A self-test message was delivered to "+m.in.from+".") + "\n")
		b.WriteString("\n" + tuiAction("enter", "continue"))
		return b.String()
	case esError:
@@ -290,9 +293,18 @@ func currentSMTPInputs() smtpInputs {
}

// applySMTPConfig proves the relay works, then persists it and rolls felis-api:
// Ping (connect/STARTTLS/AUTH, no mail sent) → [smtp] into both config files →
// the felis-smtp Secret → the config Secret → rollout. A failed Ping leaves the
// install untouched, so a typo dies at the keyboard, not at a player's OTP.
// Ping (a full transaction — connect/STARTTLS/AUTH/MAIL FROM/RCPT/DATA, which
// delivers one self-test message to the From address) → [smtp] into both config
// files → the felis-smtp Secret → the config Secret → rollout. A failed Ping
// leaves the install untouched, so a bad relay dies at the keyboard, not at a
// player's OTP.
//
// Ping really sends, because a cheaper probe cannot answer the question this
// screen exists to answer. Relays that validate sender identity — Fastmail, and
// it is not alone — return an unconditional 250 to MAIL FROM and only refuse at
// end-of-DATA. The earlier connect/AUTH/NOOP check therefore accepted a From on
// a domain the account could not send as, wrote the config, and left every OTP
// failing afterwards with this screen reporting success.
func applySMTPConfig(ctx context.Context, in smtpInputs) error {
	port, err := strconv.Atoi(in.port)
	if err != nil {
+37 −1
Changes for deploy/bootstrap.sh: 37 added lines, 1 removed line.
Original line number Diff line number Diff line
@@ -1668,8 +1668,43 @@ EOF
  ok "panel TLS certificate ready (${PANEL_TLS_CERT})"
}

# persisted_smtp_block echoes the [smtp] section an earlier run left behind, or
# nothing. Unlike every other value in the generated toml, [smtp] is not derived
# from this script's inputs -- `felis setup`'s SMTP screen writes it, after
# proving the relay works. A wholesale `cat >` therefore erases it on every
# re-run, and since re-running the installer is the documented way to update
# felis-api, an operator who updates loses mail: OTP delivery silently reverts
# to the no-Mailer path and every code is logged instead of sent. Same defect
# family as the root_domain loss fixed in ecbeb20 -- generated file, hand-set
# value, no carry-forward.
#
# Cached on first call because write_felis_toml clobbers felis.host.toml before
# it is called again for felis.pod.toml: by then the file this would read from
# no longer has the block. The pod toml is the fallback for exactly that window.
persisted_smtp_block() {
  if [ -z "${SMTP_BLOCK_CACHED:-}" ]; then
    SMTP_BLOCK_CACHED=1
    SMTP_BLOCK=""
    local f
    for f in "${STATE_DIR}/felis.host.toml" "${STATE_DIR}/felis.pod.toml"; do
      [ -r "$f" ] || continue
      # Print from [smtp] up to (not including) the next section header.
      SMTP_BLOCK="$(awk '/^[[:space:]]*\[smtp\]/ { f=1 }
                         f && /^[[:space:]]*\[/ && !/^[[:space:]]*\[smtp\]/ { exit }
                         f { print }' "$f")"
      [ -n "$SMTP_BLOCK" ] && break
    done
  fi
  printf '%s' "$SMTP_BLOCK"
}

write_felis_toml() {
  local target="$1" db_host="$2"
  local target="$1" db_host="$2" smtp_block
  smtp_block="$(persisted_smtp_block)"
  if [ -n "$smtp_block" ]; then
    log "carrying forward the configured [smtp] relay"
    smtp_block="${smtp_block}"$'\n' # keep a blank line before the next section
  fi
  cat > "$target" <<EOF
# Generated by deploy/bootstrap.sh — do not edit by hand; rerun the installer.
[server]
@@ -1701,6 +1736,7 @@ local_path = "/var/lib/felis/archives"
admin_hostname = "op.console.${FELIS_ROOT_DOMAIN}"
panel_hostname = "console.${FELIS_ROOT_DOMAIN}"

${smtp_block}
# Third-party Yggdrasil sources federated by the hasJoined multiplexer. Mojang is
# always the code-owned identity anchor (premium-first), prepended in Go; sources here
# append as namespace-rewritten guests. Shipping LittleSkin by default lets Mojang and
+20 −0
Changes for docs/openapi.yaml: 20 added lines, 0 removed lines.
Original line number Diff line number Diff line
@@ -135,6 +135,18 @@ components:
      content:
        application/json:
          schema: { $ref: '#/components/schemas/Error' }
    MailUndeliverable:
      description: >
        The configured SMTP relay refused the message (code mail_undeliverable), so no
        code was delivered. Distinct from 500 because the fault is in the install's
        [smtp] settings, not in the request or the platform — most often a From address
        the relay will not let this account send as. The relay's own text is deliberately
        withheld (it names the SMTP account) and written to the felis-api log instead,
        keyed by the same request_id this response carries. Retrying the same address
        changes nothing until an operator fixes the relay.
      content:
        application/json:
          schema: { $ref: '#/components/schemas/Error' }
    AccessResult:
      description: The structured access mutation succeeded; the raw RCON reply is in output.
      content:
@@ -2105,6 +2117,8 @@ paths:
          content:
            application/json:
              schema: { $ref: '#/components/schemas/Error' }
        '502':
          $ref: '#/components/responses/MailUndeliverable'

  /api/v1/auth/email/verify:
    post:
@@ -2219,6 +2233,8 @@ paths:
          content:
            application/json:
              schema: { $ref: '#/components/schemas/Error' }
        '502':
          $ref: '#/components/responses/MailUndeliverable'

  /api/v1/auth/op-login/status/{id}:
    get:
@@ -3531,6 +3547,8 @@ paths:
              schema: { $ref: '#/components/schemas/Error' }
        '401':
          $ref: '#/components/responses/Unauthorized'
        '502':
          $ref: '#/components/responses/MailUndeliverable'

  /api/v1/account/email/verify:
    post:
@@ -3835,6 +3853,8 @@ paths:
          content:
            application/json:
              schema: { $ref: '#/components/schemas/Error' }
        '502':
          $ref: '#/components/responses/MailUndeliverable'

  /api/v1/account/migrate/confirm/otp/verify:
    post:
+10 −0
Changes for internal/api/errors.go: 10 added lines, 0 removed lines.
Original line number Diff line number Diff line
@@ -4,6 +4,7 @@ import (
	"encoding/json"
	"errors"
	"fmt"
	"log"
	"net/http"
)

@@ -103,9 +104,18 @@ func writeJSON(w http.ResponseWriter, status int, v any) {

// writeError renders err as the standard error envelope. Non-apiError values
// collapse to a 500 so driver/internal details never reach the client.
//
// That collapse is deliberately lossy on the wire and deliberately NOT lossy in
// the log. Everything the client is denied — the driver message, the wrapped
// chain, the handler that produced it — is written to stderr first, keyed by the
// same request_id the caller is shown. Without that line an operator holding a
// "internal error" has nothing to grep for, and diagnosis degrades into guessing
// against a live install; it cost a full debugging session to learn that once.
func writeError(w http.ResponseWriter, r *http.Request, err error) {
	var ae *apiError
	if !errors.As(err, &ae) {
		log.Printf("api: %s %s: unmapped error (request_id=%s): %v",
			r.Method, r.URL.Path, requestIDFromContext(r.Context()), err)
		ae = newError(http.StatusInternalServerError, "internal", "internal error")
	}
	body := map[string]any{
+15 −1
Changes for internal/api/handlers_email_otp.go: 15 added lines, 1 removed line.
Original line number Diff line number Diff line
@@ -227,7 +227,21 @@ func (a *API) deliverOTP(ctx context.Context, email, code string) error {
		log.Printf("email-otp: no Mailer configured; code for %s is %s (KNOWN-LIMITATION: demo has no SMTP)", email, code)
		return nil
	}
	return a.Mailer.SendOTP(ctx, email, code)
	if err := a.Mailer.SendOTP(ctx, email, code); err != nil {
		// Mapped here rather than at each of the four call sites, so every door that
		// mails a code answers the same way. A relay refusal is neither the caller's
		// fault nor a bug in Felis, and a bare 500 says neither — it reads as "the
		// panel is broken" and sends the operator hunting through handler code
		// instead of their [smtp] block.
		//
		// The relay's own text stays in the log: it can name the SMTP account and the
		// sending identity ("smtp: auth as [email protected]: 535 …"), and these routes
		// are reachable by any signed-in player.
		log.Printf("api: OTP delivery failed (request_id=%s): %v", requestIDFromContext(ctx), err)
		return newError(http.StatusBadGateway, "mail_undeliverable",
			"the mail relay refused this message; ask the server operator to check the SMTP settings")
	}
	return nil
}

// setEmailRequest is the record-email body: the address to bind to the caller's
Loading