Merge the auth and wire sweep (#247)

Two bugs a stranger can reach, both on the seam V-515 is about to put on the
network.

The netaddr token handshake ran inline in Listener.Accept, so a peer that
connected and never spoke was owed the full 5s handshake timeout, and no other
connection could be accepted during it. One unauthenticated stranger holding a
socket froze the seam. Accept now reads authorized conns off a channel fed by a
loop that greets each one in its own goroutine, and Close releases what is still
queued. A unix seam delegates straight through and grows nothing.

webauthn kept regs and asserts as bare maps, driven from four HTTP handlers. A
concurrent map write is a fatal runtime error rather than a recovered panic, so
two browsers beginning a challenge at once take the web daemon down, from an
endpoint that answers before any credential is proven. A mutex covers every
access, and lookup and delete fold into takeReg and takeAssert.

That fold is a security fix in its own right. Two replays of one response both
found the challenge before either deleted it, so a challenge was not single-use.
The clientDataJSON comparison is constant time now.

Checked and already right: every gating value comes from crypto/rand, expiry is
checked on use rather than on issue, readFrame caps at 4 MiB before allocating,
and internal/auth fails closed on every arm including AuthStepUp with a nil
session. stepUpOK's fail-open and fail-closed story rests on package behaviour,
since a nil PasskeySession returns false from IsStepUp.

(V-581)

internal/ipc/wire.go

@@ -131,9 +131,15 @@ const (
 	codeInternal        = "internal"
 )
 
-// codeOf maps a server-side sentinel to its wire code. Anything not matched
-// is codeInternal — we never leak internal Go error text to a module; it
-// gets a generic "internal" and the daemon logs the real error server-side.
+// codeOf maps a server-side sentinel to its wire code. Anything not matched is
+// codeInternal.
+//
+// This used to claim the text of an unmatched error stays server-side. It does
+// not: rpcErr below ships err.Error() for codeInternal and codeBadParams,
+// deliberately, because on those two codes the text is the whole diagnostic and
+// a module has no other way to see it. Worth knowing before putting a secret in
+// an error string, and worth knowing twice on a tcp seam, where that string
+// leaves the box.
 func codeOf(err error) string {
 	switch {
 	case err == nil:

internal/netaddr/netaddr.go

@@ -31,6 +31,7 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
+	"sync"
 	"time"
 
 	"golang.org/x/sys/unix"
@@ -154,31 +155,78 @@ func clientHandshake(c net.Conn, token string) error {
 // Listener wraps a net.Listener so Accept performs the token check for a tcp
 // seam. A connection that fails the check is closed and never surfaces, so
 // the protocol above this layer only ever sees authorized peers.
+//
+// A unix seam takes none of that machinery: Accept delegates straight to the
+// wrapped listener, which is what it did before the token existed.
 type Listener struct {
 	net.Listener
 	addr Addr
+
+	start     sync.Once
+	closeOnce sync.Once
+	conns     chan net.Conn
+	errc      chan error // buffered 1, re-armed so every Accept sees the error
+	done      chan struct{}
 }
 
 // Accept returns the next authorized connection. Unauthorized peers are
 // dropped and Accept keeps waiting: a bad token is a rejected stranger, not a
 // reason to stop serving.
+//
+// Each tcp handshake runs in its own goroutine rather than inline here. A peer
+// that connects and then says nothing holds its greeting open for
+// handshakeTimeout, and inline that peer stalls every other connection for
+// five seconds — one silent stranger was enough to freeze the seam.
 func (l *Listener) Accept() (net.Conn, error) {
+	if l.addr.IsUnix() {
+		return l.Listener.Accept()
+	}
+	l.start.Do(func() { go l.acceptLoop() })
+	select {
+	case c := <-l.conns:
+		return c, nil
+	case err := <-l.errc:
+		l.errc <- err
+		return nil, err
+	}
+}
+
+// acceptLoop takes connections off the wrapped listener and greets each one
+// concurrently. It ends on the first listener error, which every later Accept
+// then reports.
+func (l *Listener) acceptLoop() {
 	for {
 		c, err := l.Listener.Accept()
 		if err != nil {
-			return nil, err
+			select {
+			case l.errc <- err:
+			case <-l.done:
+			}
+			return
 		}
-		if l.addr.IsUnix() {
-			return c, nil
-		}
-		if err := serverHandshake(c, l.addr.Token); err != nil {
-			_ = c.Close()
-			continue
-		}
-		return c, nil
+		go l.greet(c)
 	}
 }
 
+func (l *Listener) greet(c net.Conn) {
+	if err := serverHandshake(c, l.addr.Token); err != nil {
+		_ = c.Close()
+		return
+	}
+	select {
+	case l.conns <- c:
+	case <-l.done:
+		_ = c.Close()
+	}
+}
+
+// Close stops the listener and releases any connection still waiting to be
+// handed to Accept.
+func (l *Listener) Close() error {
+	l.closeOnce.Do(func() { close(l.done) })
+	return l.Listener.Close()
+}
+
 // Addr reports the parsed seam address this listener was built from.
 func (l *Listener) SeamAddr() Addr { return l.addr }
 
@@ -236,7 +284,7 @@ func Listen(a Addr) (*Listener, error) {
 		if err != nil {
 			return nil, err
 		}
-		return &Listener{Listener: ln, addr: a}, nil
+		return wrap(ln, a), nil
 	}
 	if a.Token == "" {
 		return nil, fmt.Errorf("netaddr: listen %s: tcp seam requires a token", a)
@@ -245,7 +293,17 @@ func Listen(a Addr) (*Listener, error) {
 	if err != nil {
 		return nil, fmt.Errorf("netaddr: listen %s: %w", a, err)
 	}
-	return &Listener{Listener: ln, addr: a}, nil
+	return wrap(ln, a), nil
+}
+
+func wrap(ln net.Listener, a Addr) *Listener {
+	return &Listener{
+		Listener: ln,
+		addr:     a,
+		conns:    make(chan net.Conn),
+		errc:     make(chan error, 1),
+		done:     make(chan struct{}),
+	}
 }
 
 func listenUnix(path string) (net.Listener, error) {

internal/netaddr/netaddr_test.go

@@ -5,6 +5,7 @@ import (
 	"net"
 	"path/filepath"
 	"testing"
+	"time"
 )
 
 // A scheme-less address must stay unix. Every deploy in the tree writes a bare
@@ -140,6 +141,41 @@ func TestTCPUngreetedPeerDoesNotKillTheListener(t *testing.T) {
 	}
 }
 
+// A peer that connects and never speaks must not hold the seam. The greeting
+// it owes is bounded by handshakeTimeout, so serving it on the accept path
+// costs every later connection those five seconds.
+func TestTCPSilentPeerDoesNotStallTheSeam(t *testing.T) {
+	ln, addr := listenLoopback(t, "s3cret")
+	defer ln.Close()
+	go echoOnce(ln)
+
+	mute, err := net.Dial("tcp", addr.Address)
+	if err != nil {
+		t.Fatalf("mute dial: %v", err)
+	}
+	defer mute.Close()
+
+	done := make(chan string, 1)
+	go func() {
+		c, err := Dial(addr)
+		if err != nil {
+			done <- "dial: " + err.Error()
+			return
+		}
+		defer c.Close()
+		done <- roundTrip(t, c, "still here")
+	}()
+
+	select {
+	case got := <-done:
+		if got != "still here" {
+			t.Fatalf("got %q", got)
+		}
+	case <-time.After(handshakeTimeout / 2):
+		t.Fatal("a silent peer stalled the listener")
+	}
+}
+
 // A tcp seam with no token is a misconfiguration, and it must fail at bind
 // rather than serve the owner's turns to anyone who connects.
 func TestTCPListenRequiresToken(t *testing.T) {

internal/webauthn/webauthn.go

@@ -14,11 +14,13 @@ import (
 	"crypto/elliptic"
 	"crypto/rand"
 	"crypto/sha256"
+	"crypto/subtle"
 	"encoding/base64"
 	"encoding/binary"
 	"encoding/json"
 	"fmt"
 	"math/big"
+	"sync"
 	"time"
 )
 
@@ -56,8 +58,16 @@ type credentialAssertion struct {
 
 // RP — the relying party instance. Holds config and transient challenge state.
 // A single-user daemon has one RP.
+//
+// mavweb serves the four passkey endpoints from its HTTP handlers, so the two
+// challenge maps are reached concurrently even on a single-user box: a browser
+// retrying an assertion while another tab begins one is enough. A concurrent
+// map write is a fatal runtime error, not a recovered panic, so it would take
+// the whole daemon down from an endpoint that answers before any credential is
+// proven. Every read and write of regs and asserts is under mu.
 type RP struct {
 	cfg          Config
+	mu           sync.Mutex
 	regs         map[string]*credentialRegistration
 	asserts      map[string]*credentialAssertion
 	challengeTTL time.Duration
@@ -75,6 +85,13 @@ func NewRP(cfg Config) *RP {
 
 // CleanExpired removes challenges older than the TTL.
 func (rp *RP) CleanExpired() {
+	rp.mu.Lock()
+	defer rp.mu.Unlock()
+	rp.cleanExpired()
+}
+
+// cleanExpired is CleanExpired for a caller that already holds mu.
+func (rp *RP) cleanExpired() {
 	now := time.Now()
 	for k, r := range rp.regs {
 		if now.Sub(r.CreatedAt) > rp.challengeTTL {
@@ -97,12 +114,14 @@ func (rp *RP) CreationOptions(userID []byte, userName string) (map[string]any, s
 	}
 	challengeB64 := base64.RawURLEncoding.EncodeToString(challenge)
 
-	rp.CleanExpired()
+	rp.mu.Lock()
+	rp.cleanExpired()
 	rp.regs[challengeB64] = &credentialRegistration{
 		Challenge: challengeB64,
 		UserID:    userID,
 		CreatedAt: time.Now(),
 	}
+	rp.mu.Unlock()
 
 	return map[string]any{
 		"rp": map[string]string{
@@ -136,12 +155,10 @@ func (rp *RP) CreationOptions(userID []byte, userName string) (map[string]any, s
 
 // FinishRegistration parses the browser's response and stores the credential.
 func (rp *RP) FinishRegistration(save CredentialSaver, challengeB64 string, resp map[string]any) (string, error) {
-	rp.CleanExpired()
-	reg, ok := rp.regs[challengeB64]
+	reg, ok := rp.takeReg(challengeB64)
 	if !ok {
 		return "", fmt.Errorf("webauthn: unknown or expired challenge")
 	}
-	delete(rp.regs, challengeB64)
 
 	credID := rawString(resp, "id")
 	if credID == "" {
@@ -188,11 +205,13 @@ func (rp *RP) AssertionOptions() (map[string]any, string, error) {
 	}
 	challengeB64 := base64.RawURLEncoding.EncodeToString(challenge)
 
-	rp.CleanExpired()
+	rp.mu.Lock()
+	rp.cleanExpired()
 	rp.asserts[challengeB64] = &credentialAssertion{
 		Challenge: challengeB64,
 		CreatedAt: time.Now(),
 	}
+	rp.mu.Unlock()
 
 	return map[string]any{
 		"challenge":        challengeB64,
@@ -215,11 +234,9 @@ func (rp *RP) AssertionOptions() (map[string]any, string, error) {
 // FinishAssertion verifies the browser's assertion response and returns the
 // verified credential ID.
 func (rp *RP) FinishAssertion(lookup CredentialLookup, updateSignCount SignCountUpdater, challengeB64 string, resp map[string]any) (string, error) {
-	rp.CleanExpired()
-	if _, ok := rp.asserts[challengeB64]; !ok {
+	if !rp.takeAssert(challengeB64) {
 		return "", fmt.Errorf("webauthn: unknown or expired challenge")
 	}
-	delete(rp.asserts, challengeB64)
 
 	credID := rawString(resp, "id")
 	if credID == "" {
@@ -298,6 +315,34 @@ func (rp *RP) FinishAssertion(lookup CredentialLookup, updateSignCount SignCount
 	return credID, nil
 }
 
+// takeReg removes and returns the in-flight registration for challengeB64.
+// Taking under one lock is what makes a challenge single-use: looking it up
+// and deleting it separately lets two replays of the same response both find
+// it before either deletes.
+func (rp *RP) takeReg(challengeB64 string) (*credentialRegistration, bool) {
+	rp.mu.Lock()
+	defer rp.mu.Unlock()
+	rp.cleanExpired()
+	reg, ok := rp.regs[challengeB64]
+	if ok {
+		delete(rp.regs, challengeB64)
+	}
+	return reg, ok
+}
+
+// takeAssert removes the in-flight assertion for challengeB64 and reports
+// whether it was there. Single-use for the same reason takeReg is.
+func (rp *RP) takeAssert(challengeB64 string) bool {
+	rp.mu.Lock()
+	defer rp.mu.Unlock()
+	rp.cleanExpired()
+	if _, ok := rp.asserts[challengeB64]; !ok {
+		return false
+	}
+	delete(rp.asserts, challengeB64)
+	return true
+}
+
 func verifyClientDataBytes(clientDataJSON []byte, expectedType, expectedChallenge, expectedOrigin string) error {
 	var cdj struct {
 		Type      string `json:"type"`
@@ -310,7 +355,10 @@ func verifyClientDataBytes(clientDataJSON []byte, expectedType, expectedChalleng
 	if cdj.Type != expectedType {
 		return fmt.Errorf("webauthn: unexpected type %q", cdj.Type)
 	}
-	if cdj.Challenge != expectedChallenge {
+	// Constant time, because the challenge is the one secret in clientDataJSON:
+	// it is 32 bytes of crypto/rand the browser has to echo back, and a
+	// byte-at-a-time compare is the shape that leaks a guessed prefix.
+	if subtle.ConstantTimeCompare([]byte(cdj.Challenge), []byte(expectedChallenge)) != 1 {
 		return fmt.Errorf("webauthn: challenge mismatch")
 	}
 	if cdj.Origin != expectedOrigin {

internal/webauthn/webauthn_test.go

@@ -8,6 +8,7 @@ import (
 	"encoding/base64"
 	"encoding/binary"
 	"encoding/json"
+	"sync"
 	"testing"
 )
 
@@ -179,6 +180,53 @@ func TestRegisterAssertRoundTrip(t *testing.T) {
 	}
 }
 
+// The passkey endpoints are HTTP handlers, so two browsers beginning a
+// challenge at once reach the same RP. Under -race this fails on the bare maps
+// it used to keep, and in production a concurrent map write is fatal.
+func TestChallengeMapsAreConcurrencySafe(t *testing.T) {
+	rp := NewRP(Config{Origin: testOrigin, RPID: testRPID, RPName: "maven"})
+	lookup := func(string) ([]byte, int64, error) { return nil, 0, nil }
+	upd := func(string, int64) error { return nil }
+
+	var wg sync.WaitGroup
+	for i := 0; i < 16; i++ {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			for j := 0; j < 32; j++ {
+				_, chal, err := rp.AssertionOptions()
+				if err != nil {
+					t.Error(err)
+					return
+				}
+				_, _ = rp.FinishAssertion(lookup, upd, chal, map[string]any{})
+				if _, _, err := rp.CreationOptions([]byte("u"), "user"); err != nil {
+					t.Error(err)
+					return
+				}
+				rp.CleanExpired()
+			}
+		}()
+	}
+	wg.Wait()
+}
+
+// A challenge is single-use: the second presentation of one already spent is
+// unknown, whichever goroutine gets there first.
+func TestAssertionChallengeIsSingleUse(t *testing.T) {
+	rp := NewRP(Config{Origin: testOrigin, RPID: testRPID, RPName: "maven"})
+	_, chal, err := rp.AssertionOptions()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !rp.takeAssert(chal) {
+		t.Fatal("first take of a fresh challenge failed")
+	}
+	if rp.takeAssert(chal) {
+		t.Fatal("a spent challenge was accepted twice")
+	}
+}
+
 // TestAssertRejectsWrongOrigin — a phished assertion from another origin fails.
 func TestAssertRejectsWrongOrigin(t *testing.T) {
 	err := verifyClientDataBytes(clientData("webauthn.get", "abc", "https://evil.test"), "webauthn.get", "abc", testOrigin)