diff --git a/cmd/otpmail/main.go b/cmd/otpmail/main.go index b4b97e1..93e2e3b 100644 --- a/cmd/otpmail/main.go +++ b/cmd/otpmail/main.go @@ -8,6 +8,8 @@ // OTPMAIL_MAILDIR message dir, point at tmpfs (default /var/otpmail) // OTPMAIL_SMTP_ADDR public SMTP listen addr (default :25) // OTPMAIL_CONTROL_ADDR private control-API listen addr (default 127.0.0.1:8025) +// OTPMAIL_MAX_CONNS global concurrent :25 connection cap (default 256) +// OTPMAIL_MAX_CONNS_PER_IP per-remote-IP concurrent :25 connection cap (default 8) package main import ( @@ -20,13 +22,17 @@ import ( func main() { maxBytes, _ := strconv.Atoi(os.Getenv("OTPMAIL_MAX_MSG_BYTES")) + maxConns, _ := strconv.Atoi(os.Getenv("OTPMAIL_MAX_CONNS")) + maxConnsPerIP, _ := strconv.Atoi(os.Getenv("OTPMAIL_MAX_CONNS_PER_IP")) srv, err := otpmail.New(otpmail.Config{ - Domain: os.Getenv("OTPMAIL_DOMAIN"), - Token: os.Getenv("OTPMAIL_TOKEN"), - Maildir: os.Getenv("OTPMAIL_MAILDIR"), - SMTPAddr: os.Getenv("OTPMAIL_SMTP_ADDR"), - ControlAddr: os.Getenv("OTPMAIL_CONTROL_ADDR"), - MaxMsgBytes: maxBytes, + Domain: os.Getenv("OTPMAIL_DOMAIN"), + Token: os.Getenv("OTPMAIL_TOKEN"), + Maildir: os.Getenv("OTPMAIL_MAILDIR"), + SMTPAddr: os.Getenv("OTPMAIL_SMTP_ADDR"), + ControlAddr: os.Getenv("OTPMAIL_CONTROL_ADDR"), + MaxMsgBytes: maxBytes, + MaxConns: maxConns, + MaxConnsPerIP: maxConnsPerIP, }) if err != nil { log.Fatalf("otpmail: %v", err) diff --git a/cmd/otpsignup-broker/main.go b/cmd/otpsignup-broker/main.go index e06f0a2..4b34a65 100644 --- a/cmd/otpsignup-broker/main.go +++ b/cmd/otpsignup-broker/main.go @@ -15,18 +15,29 @@ // OTPSIGNUP_DB sqlite ledger path // OTPSIGNUP_ENC_KEY 64-hex (32-byte) key sealing secrets at rest // OTPSIGNUP_MAX_IDS_PER_IP per-IP distinct-caller cap (0 = unlimited) +// OTPSIGNUP_MINT_COOLDOWN min gap between mints from one IP, a Go duration +// e.g. "30s" (0/unset = no cooldown) package main import ( "log" "os" "strconv" + "time" "github.com/pilot-protocol/app-template/internal/otpsignup" ) func main() { maxIP, _ := strconv.Atoi(os.Getenv("OTPSIGNUP_MAX_IDS_PER_IP")) + var mintCooldown time.Duration + if raw := os.Getenv("OTPSIGNUP_MINT_COOLDOWN"); raw != "" { + d, err := time.ParseDuration(raw) + if err != nil { + log.Fatalf("otpsignup-broker: OTPSIGNUP_MINT_COOLDOWN=%q: %v", raw, err) + } + mintCooldown = d + } b, err := otpsignup.New(otpsignup.Config{ Listen: os.Getenv("OTPSIGNUP_LISTEN"), MailControlURL: os.Getenv("OTPSIGNUP_MAIL_CONTROL_URL"), @@ -39,6 +50,7 @@ func main() { DBPath: os.Getenv("OTPSIGNUP_DB"), EncKeyHex: os.Getenv("OTPSIGNUP_ENC_KEY"), MaxIdentitiesPerIP: maxIP, + MintCooldown: mintCooldown, }) if err != nil { log.Fatalf("otpsignup-broker: %v", err) diff --git a/deploy/setup-broker-tls.sh b/deploy/setup-broker-tls.sh index 35344ed..1bbcaa7 100755 --- a/deploy/setup-broker-tls.sh +++ b/deploy/setup-broker-tls.sh @@ -17,9 +17,16 @@ ORIGIN="${BROKER_ORIGIN:-http://127.0.0.1:8099}" LIVE="/etc/letsencrypt/live/$HOST" # Extra nginx location blocks injected before `location /` — verbatim nginx # config for sidecar brokers that live behind the same vhost (e.g. a signup -# broker on another port). Supplied by startup.sh from the `broker-extra-locations` -# instance-metadata key so a regenerated broker.conf keeps those routes instead of -# silently dropping them. Empty ⇒ just the default `location /`. +# broker on another port). Supplied by startup.sh as the LITERAL block content +# (not a file path) from the `broker-extra-locations` instance-metadata key, so +# a regenerated broker.conf keeps those routes instead of silently dropping +# them. Empty ⇒ just the default `location /`. +# +# SECURITY: nginx does NOT inherit proxy_set_header directives into a location +# block that defines any proxy_set_header of its own — each injected location +# MUST set its own `proxy_set_header X-Real-IP $remote_addr;` (mirroring the +# default location below) or its backend will fall back to trusting a +# client-supplied X-Forwarded-For, defeating any per-IP Sybil cap. EXTRA_LOCATIONS="${BROKER_EXTRA_LOCATIONS:-}" echo "→ installing nginx + certbot" @@ -76,7 +83,9 @@ server { } NGINX # Inject any sidecar location blocks BEFORE `location /` (literal — the value - # carries nginx vars like $host/$remote_addr that bash must not expand). + # carries nginx vars like $host/$remote_addr that bash must not expand). This + # rewrites broker.conf in place (no predictable fixed-path scratch file, so + # there's nothing here for a symlink/race attack to target). if [ -n "$EXTRA_LOCATIONS" ]; then EXTRA_LOCATIONS="$EXTRA_LOCATIONS" python3 - <<'PY' import os diff --git a/deploy/startup.sh b/deploy/startup.sh index c0b6954..b3866aa 100755 --- a/deploy/startup.sh +++ b/deploy/startup.sh @@ -52,8 +52,9 @@ sudo -u pilot HOME=/opt/pilot bash -c ' cd /opt/pilot if [ -d app-template/.git ]; then (cd app-template && git pull --ff-only); else git clone --depth 1 https://github.com/pilot-protocol/app-template; fi cd app-template - go build -o /opt/pilot/publish-server ./cmd/publish-server - go build -o /opt/pilot/broker ./cmd/broker + go build -o /opt/pilot/publish-server ./cmd/publish-server + go build -o /opt/pilot/broker ./cmd/broker + go build -o /opt/pilot/otpsignup-broker ./cmd/otpsignup-broker # Sidecar signup broker (installed below only when its metadata env is set). go build -o /opt/pilot/insforge-signup-broker ./cmd/insforge-signup-broker || true ' @@ -129,6 +130,40 @@ RestartSec=3 WantedBy=multi-user.target UNIT +# ── OTP-signup broker (optional) ──────────────────────────────────────────── +# A signed broker that mints a per-user provider key with no email/code (see +# docs/BROKER-SIGNUP.md). It runs only when the operator supplies its config in +# broker-env metadata (an OTPSIGNUP_* block) — so a broker VM that doesn't offer +# signup simply doesn't run it. All app/provider/host specifics live in metadata, +# never here. Its public route is added to nginx via broker-extra-locations +# (below), so nothing app-specific is baked into this script. +if grep -q '^OTPSIGNUP_' /opt/pilot/broker.env 2>/dev/null; then + cat >/etc/systemd/system/otpsignup-broker.service < provisioned-at log *log.Logger + + // Connection admission control for the public :25 listener — an unauthenticated + // endpoint reachable by anyone, so it needs its own DoS guards independent of + // the per-message/recipient checks below: a global slot semaphore plus a + // per-remote-IP cap, so no single source can exhaust goroutines/fds and starve + // everyone else. + connSem chan struct{} + connMu sync.Mutex + connByIP map[string]int } // New validates cfg and returns a Server. @@ -77,13 +88,36 @@ func New(cfg Config) (*Server, error) { if cfg.MaxMsgBytes <= 0 { cfg.MaxMsgBytes = 256 << 10 } + if cfg.MaxConns <= 0 { + cfg.MaxConns = 256 + } + if cfg.MaxConnsPerIP <= 0 { + cfg.MaxConnsPerIP = 8 + } + // The control-plane channel between the signup broker and this mail server is + // bearer-token auth over a private interface, not mTLS (a larger deferred + // design change — see docs/BROKER-SIGNUP.md#security-notes). Until then, + // enforce a floor on token strength so a weak/guessable shared secret isn't + // the actual security boundary. + if len(cfg.Token) < minTokenLen { + return nil, fmt.Errorf("otpmail: control Token must be at least %d characters (use a long random bearer token)", minTokenLen) + } cfg.Domain = strings.ToLower(cfg.Domain) if err := os.MkdirAll(cfg.Maildir, 0o700); err != nil { return nil, fmt.Errorf("otpmail: maildir: %w", err) } - return &Server{cfg: cfg, active: map[string]time.Time{}, log: log.New(os.Stderr, "otpmail ", log.LstdFlags|log.LUTC)}, nil + return &Server{ + cfg: cfg, + active: map[string]time.Time{}, + log: log.New(os.Stderr, "otpmail ", log.LstdFlags|log.LUTC), + connSem: make(chan struct{}, cfg.MaxConns), + connByIP: map[string]int{}, + }, nil } +// minTokenLen is the minimum length required of the control-API bearer token. +const minTokenLen = 20 + // ListenAndServe starts the SMTP + control listeners and blocks until one fails. func (s *Server) ListenAndServe() error { errc := make(chan error, 2) @@ -147,8 +181,67 @@ func (s *Server) serveSMTP() error { if err != nil { continue } - go s.handleSMTP(c) + go s.acceptSMTP(c) + } +} + +// acceptSMTP applies connection admission control (a global cap plus a +// per-remote-IP cap) before handing the connection to handleSMTP, then +// releases its slot(s) once handling finishes. :25 is unauthenticated and +// internet-facing, so without this a single source (or a botnet) can open +// unbounded connections and exhaust goroutines/file descriptors — a cheap DoS +// against the one mailbox this VM exists to serve. +func (s *Server) acceptSMTP(c net.Conn) { + ip := remoteIP(c) + if !s.acquireConn(ip) { + s.log.Printf("reject smtp connection from %s: connection cap reached", ip) + c.Close() + return + } + defer s.releaseConn(ip) + s.handleSMTP(c) +} + +// acquireConn reserves one global slot and one per-IP slot, atomically enough +// that a rejected per-IP attempt gives its global slot back immediately (it +// must not leak a slot on the rejection path). Returns false if either cap is +// currently exhausted. +func (s *Server) acquireConn(ip string) bool { + select { + case s.connSem <- struct{}{}: + default: + return false // global concurrent-connection cap reached + } + s.connMu.Lock() + if s.connByIP[ip] >= s.cfg.MaxConnsPerIP { + s.connMu.Unlock() + <-s.connSem + return false // per-IP concurrent-connection cap reached + } + s.connByIP[ip]++ + s.connMu.Unlock() + return true +} + +func (s *Server) releaseConn(ip string) { + s.connMu.Lock() + if s.connByIP[ip] <= 1 { + delete(s.connByIP, ip) + } else { + s.connByIP[ip]-- + } + s.connMu.Unlock() + <-s.connSem +} + +// remoteIP strips the port from a connection's remote address (falling back to +// the raw address if it isn't host:port, e.g. some test/pipe conns). +func remoteIP(c net.Conn) string { + host, _, err := net.SplitHostPort(c.RemoteAddr().String()) + if err != nil { + return c.RemoteAddr().String() } + return host } func (s *Server) handleSMTP(c net.Conn) { diff --git a/internal/otpmail/server_test.go b/internal/otpmail/server_test.go index 5f86d50..e449188 100644 --- a/internal/otpmail/server_test.go +++ b/internal/otpmail/server_test.go @@ -12,9 +12,13 @@ import ( "time" ) +// testToken is a stand-in control-API bearer token long enough to satisfy the +// minimum-strength floor New() enforces (see minTokenLen). +const testToken = "test-control-tok-32chars-long!!!" + func newTest(t *testing.T) *Server { t.Helper() - s, err := New(Config{Domain: "mx.example.net", Token: "secret-tok", Maildir: t.TempDir()}) + s, err := New(Config{Domain: "mx.example.net", Token: testToken, Maildir: t.TempDir()}) if err != nil { t.Fatalf("New: %v", err) } @@ -157,7 +161,7 @@ func TestControlOTPRoundTrip(t *testing.T) { do := func(url, body string) *http.Response { req, _ := http.NewRequest("POST", url, strings.NewReader(body)) - req.Header.Set("Authorization", "Bearer secret-tok") + req.Header.Set("Authorization", "Bearer "+testToken) resp, err := http.DefaultClient.Do(req) if err != nil { t.Fatal(err) @@ -177,3 +181,133 @@ func TestControlOTPRoundTrip(t *testing.T) { t.Fatalf("control /otp got ready=%v code=%q", out.Ready, out.Code) } } + +// TestNewRejectsWeakControlToken guards the "enforce a strong token" mitigation +// for the (still bearer-auth-only, no mTLS) broker<->mail control plane: a short +// token must not silently produce a working server. +func TestNewRejectsWeakControlToken(t *testing.T) { + _, err := New(Config{Domain: "mx.example.net", Token: "short-tok", Maildir: t.TempDir()}) + if err == nil { + t.Fatal("expected New to reject a control token shorter than minTokenLen") + } + if _, err := New(Config{Domain: "mx.example.net", Token: "", Maildir: t.TempDir()}); err == nil { + t.Fatal("expected New to reject an empty control token") + } +} + +// TestConnAdmissionEnforcesGlobalAndPerIPCaps unit-tests the admission-control +// primitives directly (acquireConn/releaseConn), independent of real sockets: +// both the global concurrent-connection cap and the per-remote-IP cap must +// reject once exhausted, and release must free exactly the slot(s) taken. +func TestConnAdmissionEnforcesGlobalAndPerIPCaps(t *testing.T) { + s := newTest(t) + s.cfg.MaxConns = 3 + s.cfg.MaxConnsPerIP = 2 + s.connSem = make(chan struct{}, s.cfg.MaxConns) + + // Per-IP cap: 2 slots allowed for "1.1.1.1", a 3rd must be rejected even + // though the global cap (3) still has room. + if !s.acquireConn("1.1.1.1") { + t.Fatal("1st conn from 1.1.1.1 should be admitted") + } + if !s.acquireConn("1.1.1.1") { + t.Fatal("2nd conn from 1.1.1.1 should be admitted") + } + if s.acquireConn("1.1.1.1") { + t.Fatal("3rd conn from 1.1.1.1 should be rejected (per-IP cap)") + } + // A rejected per-IP attempt must not leak a global slot: a different IP + // should still be able to take the 3rd (last) global slot. + if !s.acquireConn("2.2.2.2") { + t.Fatal("conn from a different IP should still be admitted (global cap has room)") + } + // Global cap now exhausted (2 from 1.1.1.1 + 1 from 2.2.2.2 = 3): any IP, + // including a brand-new one, is rejected. + if s.acquireConn("3.3.3.3") { + t.Fatal("conn should be rejected once the global cap is exhausted") + } + // Releasing one frees exactly one global + one per-IP slot. + s.releaseConn("1.1.1.1") + if !s.acquireConn("3.3.3.3") { + t.Fatal("conn should be admitted again after a release frees a global slot") + } + if s.connByIP["1.1.1.1"] != 1 { + t.Fatalf("connByIP[1.1.1.1]=%d want 1 after one release", s.connByIP["1.1.1.1"]) + } +} + +// TestSMTPListenerRejectsExcessConnectionsFromOneIP drives real TCP connections +// through the actual :25 accept path (acceptSMTP) and confirms a source that +// opens more than MaxConnsPerIP concurrent connections gets the excess ones +// closed before any SMTP greeting — proving the DoS guard is wired into the +// live listener, not just unit-testable in isolation. +func TestSMTPListenerRejectsExcessConnectionsFromOneIP(t *testing.T) { + s := newTest(t) + s.cfg.MaxConnsPerIP = 2 + s.cfg.MaxConns = 100 + s.connSem = make(chan struct{}, s.cfg.MaxConns) + + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + defer ln.Close() + go func() { + for { + c, err := ln.Accept() + if err != nil { + return + } + go s.acceptSMTP(c) + } + }() + + dial := func() net.Conn { + c, err := net.Dial("tcp", ln.Addr().String()) + if err != nil { + t.Fatal(err) + } + _ = c.SetDeadline(time.Now().Add(5 * time.Second)) + return c + } + greets := func(c net.Conn) bool { + buf := make([]byte, 64) + n, err := c.Read(buf) + return err == nil && n > 0 && strings.HasPrefix(string(buf[:n]), "220") + } + + c1, c2 := dial(), dial() + defer c1.Close() + defer c2.Close() + if !greets(c1) { + t.Fatal("1st connection should be admitted and greeted") + } + if !greets(c2) { + t.Fatal("2nd connection should be admitted and greeted") + } + + // 3rd concurrent connection from the same (loopback) IP exceeds MaxConnsPerIP + // (2): the server must close it immediately without ever sending a greeting. + c3 := dial() + defer c3.Close() + if greets(c3) { + t.Fatal("3rd concurrent connection from the same IP should be rejected, not greeted") + } + + // Freeing a slot lets a new connection through again. + c1.Close() + // give the server goroutine a moment to notice the close and release its slot + deadline := time.Now().Add(2 * time.Second) + for { + c4 := dial() + if greets(c4) { + c4.Close() + break + } + c4.Close() + if time.Now().After(deadline) { + t.Fatal("expected a connection to be admitted again after freeing a slot") + } + time.Sleep(20 * time.Millisecond) + } +} diff --git a/internal/otpsignup/broker.go b/internal/otpsignup/broker.go index 7fbf59c..ecfdeea 100644 --- a/internal/otpsignup/broker.go +++ b/internal/otpsignup/broker.go @@ -34,6 +34,12 @@ import ( "github.com/pilot-protocol/app-template/internal/broker" ) +// minTokenLen is the minimum length required of the mail-control bearer token +// (see New) — a floor on shared-secret strength for the broker↔mail-server +// control plane, pending real mTLS between the two (deferred; see +// docs/BROKER-SIGNUP.md#security-notes). +const minTokenLen = 20 + // Config is the broker's deployment configuration (all from env; no provider or // host specifics compiled in). type Config struct { @@ -63,6 +69,12 @@ type Broker struct { mu sync.Mutex ipSeen map[string]map[string]bool // ip -> set of caller ids ipLast map[string]time.Time // ip -> last mint time + + // callerLocks serializes the check-then-act in Signup per caller id, so two + // concurrent signups for the SAME caller can't both pass the "no existing + // account" check and both mint (and pay for) a duplicate provider account. + // Keyed by caller so unrelated callers still proceed in parallel. + callerLocks sync.Map // caller (string) -> *sync.Mutex } // New validates cfg, opens the ledger, and returns a Broker. @@ -84,6 +96,14 @@ func New(cfg Config) (*Broker, error) { return nil, fmt.Errorf("otpsignup: %s is required", name) } } + // The broker↔mail-server control channel is bearer-token auth over a + // private interface, not mTLS (a larger deferred design change — see + // docs/BROKER-SIGNUP.md#security-notes). Until then, enforce a floor on + // token strength so a weak/guessable shared secret isn't the actual + // security boundary. + if len(cfg.MailToken) < minTokenLen { + return nil, fmt.Errorf("otpsignup: MailToken must be at least %d characters (use a long random bearer token)", minTokenLen) + } st, err := openStore(cfg.DBPath, cfg.EncKeyHex) if err != nil { return nil, err @@ -141,8 +161,23 @@ func (b *Broker) handleSignup(w http.ResponseWriter, r *http.Request) { var errRateLimited = errors.New("per-IP identity cap reached (anti-abuse)") +// callerLock returns (creating if needed) the mutex serializing Signup calls +// for one caller id. +func (b *Broker) callerLock(caller string) *sync.Mutex { + v, _ := b.callerLocks.LoadOrStore(caller, &sync.Mutex{}) + return v.(*sync.Mutex) +} + // Signup is the core flow, separated from HTTP so it is directly testable. func (b *Broker) Signup(ctx context.Context, caller, ip string) (*Account, error) { + // Serialize per caller: without this, two concurrent requests for the same + // caller can both observe "no existing account" below and both race through + // to mint (and register with the provider for) a duplicate account. Locking + // only on the caller id keeps unrelated callers running in parallel. + lock := b.callerLock(caller) + lock.Lock() + defer lock.Unlock() + // Idempotent: a caller that already has an account gets it back (retrievable), // no second provider account minted. if rec, ok, _ := b.store.get(caller); ok { @@ -306,15 +341,23 @@ func (b *Broker) getJSON(ctx context.Context, url string) (int, []byte) { return resp.StatusCode, body } +// clientIP returns the caller's real address for the per-IP Sybil cap. +// +// SECURITY: this reads ONLY X-Real-IP (a header a client cannot set — nginx +// overwrites it with $remote_addr on the way in) or, failing that, the raw +// socket's RemoteAddr. It deliberately does NOT consult X-Forwarded-For: +// unlike X-Real-IP, XFF is a comma-separated list a client is free to prepend +// to, so trusting the client-supplied first element would let anyone forge a +// fresh "IP" per request and mint unlimited identities past MaxIdentitiesPerIP. +// See deploy/setup-broker-tls.sh for the nginx side of this trust boundary. func clientIP(r *http.Request) string { - if xf := r.Header.Get("X-Real-IP"); xf != "" { - return xf + if ip := strings.TrimSpace(r.Header.Get("X-Real-IP")); ip != "" { + return ip } - if xff := r.Header.Get("X-Forwarded-For"); xff != "" { - return strings.TrimSpace(strings.Split(xff, ",")[0]) + if host, _, err := net.SplitHostPort(r.RemoteAddr); err == nil { + return host } - host, _, _ := net.SplitHostPort(r.RemoteAddr) - return host + return r.RemoteAddr } func digString(raw []byte, path ...string) string { diff --git a/internal/otpsignup/broker_test.go b/internal/otpsignup/broker_test.go index dc4febe..df40c04 100644 --- a/internal/otpsignup/broker_test.go +++ b/internal/otpsignup/broker_test.go @@ -8,12 +8,17 @@ import ( "net/http/httptest" "strings" "sync" + "sync/atomic" "testing" "time" "github.com/pilot-protocol/app-template/internal/broker" ) +// testMailToken is a stand-in mail-control bearer token long enough to satisfy +// the minimum-strength floor New() enforces (see minTokenLen). +const testMailToken = "test-mail-ctl-tok-32chars-long!!" + // mockProvider stands in for the identity provider: register → 201, verify → // {application:{api_key}}. mockMail stands in for the otpmail control API. func mockProvider(t *testing.T, key string) *httptest.Server { @@ -34,7 +39,7 @@ func mockMail(t *testing.T, code string) *httptest.Server { mux := http.NewServeMux() auth := func(h http.HandlerFunc) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - if r.Header.Get("Authorization") != "Bearer mail-tok" { + if r.Header.Get("Authorization") != "Bearer "+testMailToken { http.Error(w, "unauth", http.StatusUnauthorized) return } @@ -69,7 +74,7 @@ func mockMail(t *testing.T, code string) *httptest.Server { func newBroker(t *testing.T, provURL, mailURL string) *Broker { t.Helper() b, err := New(Config{ - MailControlURL: mailURL, MailToken: "mail-tok", MailDomain: "mx.example.net", + MailControlURL: mailURL, MailToken: testMailToken, MailDomain: "mx.example.net", RegisterURL: provURL + "/register", VerifyURL: provURL + "/verify", KeyPath: "application.api_key", OTPTimeout: 5 * time.Second, MaxIdentitiesPerIP: 2, }) @@ -191,3 +196,145 @@ func TestSignedHTTPFlow(t *testing.T) { t.Fatalf("unsigned status=%d want 401", r2.StatusCode) } } + +// TestClientIPIgnoresForwardedForTrustsOnlyXRealIP is a direct, table-driven +// unit test of the trust boundary clientIP() enforces: X-Real-IP (settable +// only by our own nginx, never the client) wins when present; a client-supplied +// X-Forwarded-For is NEVER consulted, even alone; the final fallback is the raw +// socket RemoteAddr. +func TestClientIPIgnoresForwardedForTrustsOnlyXRealIP(t *testing.T) { + mk := func(remoteAddr, realIP, xff string) *http.Request { + r := httptest.NewRequest(http.MethodPost, "/signup", nil) + r.RemoteAddr = remoteAddr + if realIP != "" { + r.Header.Set("X-Real-IP", realIP) + } + if xff != "" { + r.Header.Set("X-Forwarded-For", xff) + } + return r + } + cases := []struct { + name string + remoteAddr, realIP, xff string + want string + }{ + {"no headers falls back to RemoteAddr", "10.0.0.5:5555", "", "", "10.0.0.5"}, + {"X-Real-IP set by nginx wins", "127.0.0.1:9999", "203.0.113.9", "", "203.0.113.9"}, + {"spoofed XFF alone is ignored; RemoteAddr used", "127.0.0.1:9999", "", "1.2.3.4", "127.0.0.1"}, + {"X-Real-IP wins even with a forged XFF present", "127.0.0.1:9999", "203.0.113.9", "6.6.6.6, 7.7.7.7", "203.0.113.9"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := clientIP(mk(c.remoteAddr, c.realIP, c.xff)); got != c.want { + t.Errorf("clientIP()=%q want %q", got, c.want) + } + }) + } +} + +// TestForgedXForwardedForDoesNotBypassIPCap is the HTTP-layer proof for the +// same finding: an attacker who signs each request with a FRESH identity (so +// every call looks like a distinct caller) and attaches a DIFFERENT forged +// X-Forwarded-For per request (the classic Sybil-via-spoofed-IP move) must +// still hit MaxIdentitiesPerIP, because none of these requests carry an +// X-Real-IP (only our own nginx may set that), so they all correctly bucket to +// the one real connection IP. +func TestForgedXForwardedForDoesNotBypassIPCap(t *testing.T) { + prov := mockProvider(t, "K") + mail := mockMail(t, "Z9Y8X7") + defer prov.Close() + defer mail.Close() + b := newBroker(t, prov.URL, mail.URL) // MaxIdentitiesPerIP = 2 + ts := httptest.NewServer(http.HandlerFunc(b.handleSignup)) + defer ts.Close() + + signAndPost := func(forgedXFF string) *http.Response { + _, priv, _ := ed25519.GenerateKey(nil) // a fresh identity every call + hdrs := broker.Sign(priv, http.MethodPost, "/signup", []byte("{}"), time.Now()) + req, _ := http.NewRequest(http.MethodPost, ts.URL+"/signup", strings.NewReader("{}")) + for k, v := range hdrs { + req.Header.Set(k, v) + } + req.Header.Set("X-Forwarded-For", forgedXFF) // attacker-controlled; must be ignored + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatal(err) + } + return resp + } + + if r := signAndPost("9.9.9.1"); r.StatusCode != http.StatusOK { + t.Fatalf("1st distinct identity: status=%d want 200", r.StatusCode) + } + if r := signAndPost("9.9.9.2"); r.StatusCode != http.StatusOK { + t.Fatalf("2nd distinct identity: status=%d want 200", r.StatusCode) + } + // cap is 2: a 3rd distinct identity — with yet another forged XFF, exactly + // what a Sybil attacker would vary per request — must still be rejected. + if r := signAndPost("9.9.9.3"); r.StatusCode != http.StatusTooManyRequests { + t.Fatalf("3rd distinct identity with forged XFF: status=%d want 429 (a spoofed X-Forwarded-For must not bypass MaxIdentitiesPerIP)", r.StatusCode) + } +} + +// TestConcurrentSignupsForSameCallerMintOnlyOneAccount is the fix for the +// signup race: N goroutines calling Signup for the SAME caller concurrently +// must serialize on the per-caller lock, mint the provider account exactly +// once, and all return the identical (cached) account — never a duplicate. +func TestConcurrentSignupsForSameCallerMintOnlyOneAccount(t *testing.T) { + var registerCalls int32 + mux := http.NewServeMux() + mux.HandleFunc("/register", func(w http.ResponseWriter, r *http.Request) { + atomic.AddInt32(®isterCalls, 1) + time.Sleep(20 * time.Millisecond) // widen the race window + w.WriteHeader(http.StatusCreated) + w.Write([]byte(`{"message":"ok"}`)) + }) + mux.HandleFunc("/verify", func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`{"application":{"api_key":"KEY-RACE"}}`)) + }) + prov := httptest.NewServer(mux) + defer prov.Close() + mail := mockMail(t, "R4C3O1") + defer mail.Close() + b := newBroker(t, prov.URL, mail.URL) + + const n = 8 + var wg sync.WaitGroup + accts := make([]*Account, n) + errs := make([]error, n) + for i := 0; i < n; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + accts[i], errs[i] = b.Signup(context.Background(), "same-caller", "6.6.6.6") + }(i) + } + wg.Wait() + + for i, err := range errs { + if err != nil { + t.Fatalf("goroutine %d: Signup: %v", i, err) + } + } + for i := 1; i < n; i++ { + if accts[i].Email != accts[0].Email || accts[i].APIKey != accts[0].APIKey { + t.Fatalf("goroutine %d minted a different account: %+v vs %+v", i, accts[i], accts[0]) + } + } + if got := atomic.LoadInt32(®isterCalls); got != 1 { + t.Fatalf("provider /register called %d times, want exactly 1 (concurrent signups for one caller must serialize, not race)", got) + } +} + +// TestNewRejectsWeakMailToken guards the "enforce a strong token" mitigation +// for the (still bearer-auth-only, no mTLS) broker<->mail control plane. +func TestNewRejectsWeakMailToken(t *testing.T) { + _, err := New(Config{ + MailControlURL: "http://127.0.0.1:1", MailToken: "short", MailDomain: "mx.example.net", + RegisterURL: "https://example.com/register", VerifyURL: "https://example.com/verify", + }) + if err == nil { + t.Fatal("expected New to reject a MailToken shorter than minTokenLen") + } +} diff --git a/internal/otpsignup/store.go b/internal/otpsignup/store.go index 1a45fe4..6248612 100644 --- a/internal/otpsignup/store.go +++ b/internal/otpsignup/store.go @@ -34,18 +34,33 @@ func openStore(dbPath, encKeyHex string) (*store, error) { if dbPath == "" { dbPath = ":memory:" } - // A 32-byte key seals secrets at rest. Prod MUST set a stable key (else a - // restart can't decrypt prior rows); tests fall back to an ephemeral one. + // A 32-byte key seals secrets at rest. Prod MUST set a stable key: a missing + // key used to fall back SILENTLY to a fresh random one, which is worse than + // it looks — every restart would then seal under a NEW key, so store.get() + // fails to decrypt every row written under the old one. get() treats that as + // "not found" (see below), so Signup thinks the caller has no account and + // mints (and pays for) a fresh one, whose INSERT is then silently a no-op + // (INSERT OR IGNORE against the still-present old row) — every restart from + // then on re-mints again, forever, and no single key is ever reliably + // retrievable. That silently defeats the whole idempotency guarantee this + // store exists for. So: require an explicit key for any persistent store, + // and only auto-generate one for a genuinely ephemeral (:memory:, e.g. test) + // store where there is nothing on disk to survive a restart anyway. var key []byte - if encKeyHex != "" { + switch { + case encKeyHex != "": k, err := hex.DecodeString(encKeyHex) if err != nil || len(k) != 32 { return nil, fmt.Errorf("otpsignup: EncKeyHex must be 64 hex chars (32 bytes)") } key = k - } else { + case dbPath == ":memory:": key = make([]byte, 32) - _, _ = rand.Read(key) + if _, err := rand.Read(key); err != nil { + return nil, fmt.Errorf("otpsignup: generating ephemeral enc key: %w", err) + } + default: + return nil, fmt.Errorf("otpsignup: OTPSIGNUP_ENC_KEY is required for a persistent ledger (%s) — an unset key silently rotates on every restart and corrupts existing accounts (see store.go)", dbPath) } block, err := aes.NewCipher(key) if err != nil { diff --git a/internal/otpsignup/store_test.go b/internal/otpsignup/store_test.go new file mode 100644 index 0000000..d2042c5 --- /dev/null +++ b/internal/otpsignup/store_test.go @@ -0,0 +1,53 @@ +package otpsignup + +import ( + "path/filepath" + "testing" +) + +// validEncKeyHex is 64 hex chars (32 bytes) — a well-formed at-rest key for tests. +const validEncKeyHex = "abababababababababababababababababababababababababababababababab" + +// TestOpenStoreRequiresEncKeyForPersistentDB is the fix for the silent +// ephemeral-key fallback: a persistent (non-:memory:) ledger with no configured +// key must fail loudly at startup, not quietly generate a throwaway key that +// makes every previously-sealed row undecryptable after the next restart (see +// the comment on openStore for why that silently breaks idempotency). +func TestOpenStoreRequiresEncKeyForPersistentDB(t *testing.T) { + dbPath := filepath.Join(t.TempDir(), "accounts.db") + if _, err := openStore(dbPath, ""); err == nil { + t.Fatal("expected openStore to fail without an enc key for a persistent db path") + } + // A valid 32-byte hex key must still work for a persistent path. + st, err := openStore(dbPath, validEncKeyHex) + if err != nil { + t.Fatalf("openStore with a valid key: %v", err) + } + if st == nil || st.db == nil { + t.Fatal("expected a usable store") + } +} + +// TestOpenStoreAllowsEphemeralKeyForInMemoryDB confirms the exception is +// scoped precisely: a genuinely ephemeral (:memory:, or the default when +// DBPath is unset) store may still auto-generate a key, since there is nothing +// on disk that a lost key could ever fail to decrypt after a restart. +func TestOpenStoreAllowsEphemeralKeyForInMemoryDB(t *testing.T) { + if _, err := openStore(":memory:", ""); err != nil { + t.Fatalf("in-memory store should work without an explicit enc key: %v", err) + } + if _, err := openStore("", ""); err != nil { + t.Fatalf("empty dbPath (defaults to :memory:) should work without an explicit enc key: %v", err) + } +} + +// TestOpenStoreRejectsMalformedEncKey keeps the existing validation intact +// alongside the new persistent-path requirement. +func TestOpenStoreRejectsMalformedEncKey(t *testing.T) { + if _, err := openStore(":memory:", "not-hex"); err == nil { + t.Fatal("expected openStore to reject a non-hex enc key") + } + if _, err := openStore(":memory:", "abcd"); err == nil { + t.Fatal("expected openStore to reject a too-short (non-32-byte) enc key") + } +}