diff --git a/cmd/meshtender/main.go b/cmd/meshtender/main.go index 0caecff..03bf7f9 100644 --- a/cmd/meshtender/main.go +++ b/cmd/meshtender/main.go @@ -127,7 +127,10 @@ func run(logger *slog.Logger) error { httpSrv := &http.Server{ Addr: cfg.Addr, Handler: rec.Handler(srv.Handler()), - ReadHeaderTimeout: 10 * time.Second, + ReadHeaderTimeout: readHeaderTimeout, + ReadTimeout: readTimeout, + WriteTimeout: writeTimeout, + IdleTimeout: idleTimeout, } tls := cfg.TLSCert != "" && cfg.TLSKey != "" @@ -190,6 +193,32 @@ func run(logger *slog.Logger) error { // indexed statement, which isn't worth a second ticker to avoid. const janitorInterval = 5 * time.Minute +// Connection timeouts. With all of these at zero (net/http's default) a slow or idle +// peer can hold a connection open indefinitely after sending headers. +// +// - readHeaderTimeout guards the classic slowloris: headers dribbled out forever. +// - readTimeout covers the body too. limitBody caps the SIZE at 1 MiB, but with no +// deadline a client could still take days to deliver it. +// - writeTimeout is deliberately generous. It has to cover a genuine download over a +// bad link — this product's users are on rural, marginal connections, and the +// largest asset is a few hundred KB — while still bounding a peer that stops +// reading mid-response. +// - idleTimeout reaps keep-alive connections between requests. +// +// The console WebSocket is unaffected. These become deadlines on the underlying +// connection, which looks like it should sever an upgraded socket — but net/http clears +// the deadline when a handler hijacks the connection (hijackLocked calls +// rwc.SetDeadline(time.Time{})), so the socket inherits nothing and is bounded instead +// by consoleIdleTimeout and the shutdown drain. That's the one thing these timeouts +// could plausibly have broken, so +// core.TestConsoleWebSocketOutlivesServerReadTimeout pins it. +const ( + readHeaderTimeout = 10 * time.Second + readTimeout = 30 * time.Second + writeTimeout = 120 * time.Second + idleTimeout = 120 * time.Second +) + // janitorSweep is one periodic cleanup job: a name for the log, and the delete to // run. Taking a closure rather than an interface keeps the janitor decoupled from // the store (a store method value satisfies it directly) and lets the loop be diff --git a/cmd/meshtender/main_test.go b/cmd/meshtender/main_test.go index 6fc4494..1ec279b 100644 --- a/cmd/meshtender/main_test.go +++ b/cmd/meshtender/main_test.go @@ -135,6 +135,38 @@ func TestJanitorRunsEverySweep(t *testing.T) { waitForCalls(t, healthy, 2, 2*time.Second) } +// TestConnectionTimeoutsAreSet pins audit O1: every phase of a connection is bounded. +// Zero means "no limit" in net/http, so a single omission silently reopens the hole +// this closed, and nothing else would notice. +func TestConnectionTimeoutsAreSet(t *testing.T) { + t.Parallel() + for _, c := range []struct { + name string + got time.Duration + }{ + {"readHeaderTimeout", readHeaderTimeout}, + {"readTimeout", readTimeout}, + {"writeTimeout", writeTimeout}, + {"idleTimeout", idleTimeout}, + } { + if c.got <= 0 { + t.Errorf("%s = %v; zero means unlimited, so a slow or idle peer can hold a "+ + "connection open forever", c.name, c.got) + } + } + + // Headers must not outlast the whole request they belong to. + if readHeaderTimeout > readTimeout { + t.Errorf("readHeaderTimeout (%v) exceeds readTimeout (%v)", readHeaderTimeout, readTimeout) + } + // The write budget has to cover a real download on a bad link. The largest asset is + // a few hundred KB, so anything under ~30s risks truncating legitimate responses for + // exactly the rural, marginal connections this product targets. + if writeTimeout < 30*time.Second { + t.Errorf("writeTimeout = %v; too tight for a large asset over a slow link", writeTimeout) + } +} + // TestJanitorIntervalOutlivesCodeTTL: the sweep only has to keep the table // small, but an interval shorter than the code TTL would mean pointless work, and a // wildly long one would defeat the purpose. Pin it to a sane band so a future edit diff --git a/internal/core/ws_deadline_test.go b/internal/core/ws_deadline_test.go new file mode 100644 index 0000000..d3e5c85 --- /dev/null +++ b/internal/core/ws_deadline_test.go @@ -0,0 +1,118 @@ +package core + +import ( + "context" + "crypto/rand" + "net/http" + "net/http/cookiejar" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/coder/websocket" + meshcore "github.com/meshcore-go/meshcore-go" + + "github.com/jleight/meshtender/internal/auth" + "github.com/jleight/meshtender/internal/identity" + "github.com/jleight/meshtender/internal/store" +) + +// TestConsoleWebSocketOutlivesServerReadTimeout pins the one thing audit O1's server +// timeouts could plausibly have broken: a long-lived console session. +// +// http.Server's ReadTimeout/WriteTimeout/IdleTimeout become deadlines on the +// underlying connection, which looks like it should sever an upgraded WebSocket the +// moment one elapses. It doesn't — net/http clears the deadline when a handler hijacks +// the connection (hijackLocked calls rwc.SetDeadline(time.Time{})), so the socket +// inherits nothing and is bounded only by consoleIdleTimeout and the shutdown drain. +// +// That behaviour is load-bearing but belongs to the standard library, not to this +// codebase, so it's worth an explicit test: it would catch a future WebSocket library +// that re-arms deadlines after hijacking, or a change in Go's behaviour, either of +// which would silently start cutting console sessions mid-command. +// +// The server here uses a deliberately tiny 250ms ReadTimeout rather than the +// production 30s so the assertion takes a second instead of half a minute. +func TestConsoleWebSocketOutlivesServerReadTimeout(t *testing.T) { + t.Parallel() + st, ctx := coreStore(t) + + var masterKey [32]byte + if _, err := rand.Read(masterKey[:]); err != nil { + t.Fatalf("master key: %v", err) + } + idSvc, err := identity.LoadOrCreate(ctx, st, masterKey) + if err != nil { + t.Fatalf("identity: %v", err) + } + authSvc, err := auth.New(st, st.Pool(), testAuthConfig()) + if err != nil { + t.Fatalf("auth: %v", err) + } + srv, err := NewServer(st, authSvc, idSvc, testConfig()) + if err != nil { + t.Fatalf("server: %v", err) + } + + // The production timeouts live in main.go; mirror their SHAPE here with a tiny + // read deadline so the hazard reproduces in milliseconds instead of 30 seconds. + const readTimeout = 250 * time.Millisecond + ts := httptest.NewUnstartedServer(srv.Handler()) + ts.Config.ReadHeaderTimeout = readTimeout + ts.Config.ReadTimeout = readTimeout + ts.Config.WriteTimeout = readTimeout + ts.Config.IdleTimeout = readTimeout + ts.Start() + defer ts.Close() + + jar, _ := cookiejar.New(nil) + user := seedSession(t, ts, st, ctx, jar, "wsdeadline") + + repeater, err := meshcore.GenerateLocalIdentity(rand.Reader) + if err != nil { + t.Fatalf("repeater identity: %v", err) + } + rep, err := st.CreateRepeater(ctx, &store.Repeater{ + OwnerID: user.ID, Name: "Deadline", PublicKeyHex: repeater.String(), + RadioFreqHz: 869525000, RadioBwHz: 250000, RadioSF: 11, RadioCR: 5, + }) + if err != nil { + t.Fatalf("create repeater: %v", err) + } + + wsURL := "ws" + strings.TrimPrefix(ts.URL, "http") + "/repeaters/" + rep.PublicID + "/console/ws" + hdr := http.Header{} + var parts []string + for _, c := range jar.Cookies(mustURL(t, ts.URL)) { + parts = append(parts, c.Name+"="+c.Value) + } + if len(parts) > 0 { + hdr.Set("Cookie", strings.Join(parts, "; ")) + } + + dctx, dcancel := context.WithTimeout(ctx, 5*time.Second) + defer dcancel() + ws, _, err := websocket.Dial(dctx, wsURL, &websocket.DialOptions{HTTPHeader: hdr}) + if err != nil { + t.Fatalf("ws dial: %v", err) + } + defer ws.Close(websocket.StatusNormalClosure, "") + + // Idle well past every deadline above, then use the socket. A surviving write and + // read prove the connection wasn't torn down by an inherited request deadline. + time.Sleep(4 * readTimeout) + + wctx, wcancel := context.WithTimeout(ctx, 10*time.Second) + defer wcancel() + if err := ws.Write(wctx, websocket.MessageText, []byte(`{"type":"ready"}`)); err != nil { + t.Fatalf("write after %v idle failed — the socket inherited the server's read/write "+ + "deadline instead of clearing it: %v", 4*readTimeout, err) + } + // The handler answers a "ready" with its tuning/status frames, so a successful read + // confirms the session is still live end to end, not just that the write buffered. + if _, _, err := ws.Read(wctx); err != nil { + t.Fatalf("read after %v idle failed — the session did not survive the server's "+ + "request deadlines: %v", 4*readTimeout, err) + } +}