From bef60cc4628f86d8f6eb28a61d5d6b8778750e00 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 3 Sep 2026 00:28:11 -0500 Subject: [PATCH] fix: treat negative keepalive replies as liveness --- server.go | 27 ++++++++++++++------------- server_test.go | 41 ++++++++++++++++++++++++++++------------- 2 files changed, 42 insertions(+), 26 deletions(-) diff --git a/server.go b/server.go index d75297d..c78fd94 100644 --- a/server.go +++ b/server.go @@ -516,7 +516,7 @@ func (srv *Server) HandleConn(newConn net.Conn) { // of sshConn. It mirrors OpenSSH's client_alive_check(): if at least one // channel is open, the keepalive is sent as a SSH2_MSG_CHANNEL_REQUEST on // that channel; otherwise it falls back to a SSH2_MSG_GLOBAL_REQUEST. After -// ClientAliveCountMax consecutive intervals with no successful reply, +// ClientAliveCountMax consecutive intervals with no reply received, // sshConn is closed so HandleConn unblocks. Stops when `done` closes // (HandleConn returning). func (srv *Server) connectionKeepAlive( @@ -563,35 +563,36 @@ func (srv *Server) connectionKeepAlive( keepAlive.ServerRequestedKeepAliveCallback() // Mirror OpenSSH client_alive_check(): prefer a channel // request on an open channel; fall back to a global - // request if no channel is open or the channel send - // fails (channel was closed mid-flight). + // request only if no channel is open or the channel send + // fails at the transport level (for example, because the + // channel closed mid-flight). A negative SSH reply still + // proves that the peer received and processed the request. // // No outer timeout is needed here: the inFlight semaphore // already prevents overlapping probes, TimeIsUp() at the // next tick enforces the deadline, and if SendRequest // hangs forever it will be unblocked when the TimeIsUp // branch closes sshConn. - var ( - ok bool - err error - ) + var err error ch := openChans.any() if ch != nil { - ok, err = ch.SendRequest(keepAliveRequestType, true, nil) + _, err = ch.SendRequest(keepAliveRequestType, true, nil) if err != nil { openChans.remove(ch) - } - if err != nil || !ok { ch = nil } } if ch == nil { - ok, _, err = sshConn.SendRequest(keepAliveRequestType, true, nil) + _, _, err = sshConn.SendRequest(keepAliveRequestType, true, nil) } - if err == nil && ok { + // keepalive@openssh.com is intentionally unsupported by + // many peers. Both success and failure replies prove that + // the peer is alive; only a transport error means no reply + // was received. + if err == nil { keepAlive.Reset() } else { - log.Printf("ssh: keepalive request failed: ok=%t err=%v", ok, err) + log.Printf("ssh: keepalive request failed: err=%v", err) } }() } diff --git a/server_test.go b/server_test.go index 00864ab..8d620c2 100644 --- a/server_test.go +++ b/server_test.go @@ -373,12 +373,14 @@ func TestConnectionKeepAliveUsesChannelRequestWhenSessionOpen(t *testing.T) { } } -// TestConnectionKeepAliveNegativeGlobalReplyDoesNotReset verifies that a -// protocol-level negative response is not counted as a successful keepalive. -func TestConnectionKeepAliveNegativeGlobalReplyDoesNotReset(t *testing.T) { +// TestConnectionKeepAliveNegativeGlobalReplyKeepsAlive verifies OpenSSH-style +// keepalive semantics: a protocol-level failure reply still proves that the +// peer received and processed the request, so it resets the liveness deadline. +func TestConnectionKeepAliveNegativeGlobalReplyKeepsAlive(t *testing.T) { t.Parallel() closingFired := make(chan struct{}) + var globalCount atomic.Int64 srv := &Server{ Handler: func(_ Session) {}, ClientAliveInterval: 100 * time.Millisecond, @@ -414,6 +416,7 @@ func TestConnectionKeepAliveNegativeGlobalReplyDoesNotReset(t *testing.T) { go func() { for req := range reqs { if req.Type == keepAliveRequestType { + globalCount.Add(1) _ = req.Reply(false, nil) } else if req.WantReply { _ = req.Reply(true, nil) @@ -421,19 +424,26 @@ func TestConnectionKeepAliveNegativeGlobalReplyDoesNotReset(t *testing.T) { } }() + // The configured timeout is 200ms. Negative replies must keep the + // transport alive well beyond that window. select { case <-closingFired: - case <-time.After(5 * time.Second): - t.Fatal("negative global keepalive replies incorrectly reset the deadline") + t.Fatal("negative global keepalive reply did not preserve liveness") + case <-time.After(700 * time.Millisecond): + } + if got := globalCount.Load(); got < 2 { + t.Fatalf("expected multiple negative global keepalive replies, got %d", got) } } -// TestConnectionKeepAliveNegativeChannelReplyDoesNotReset is the channel -// request counterpart to TestConnectionKeepAliveNegativeGlobalReplyDoesNotReset. -func TestConnectionKeepAliveNegativeChannelReplyDoesNotReset(t *testing.T) { +// TestConnectionKeepAliveNegativeChannelReplyKeepsAlive is the channel +// counterpart to TestConnectionKeepAliveNegativeGlobalReplyKeepsAlive. A +// negative channel reply proves liveness and must not trigger global fallback. +func TestConnectionKeepAliveNegativeChannelReplyKeepsAlive(t *testing.T) { t.Parallel() closingFired := make(chan struct{}) + var globalCount, channelCount atomic.Int64 srv := &Server{ Handler: func(s Session) { <-s.Context().Done() }, ClientAliveInterval: 100 * time.Millisecond, @@ -466,7 +476,6 @@ func TestConnectionKeepAliveNegativeChannelReplyDoesNotReset(t *testing.T) { for range chans { //nolint:revive // intentional drain } }() - var globalCount atomic.Int64 go func() { for req := range reqs { if req.Type == keepAliveRequestType { @@ -486,6 +495,7 @@ func TestConnectionKeepAliveNegativeChannelReplyDoesNotReset(t *testing.T) { go func() { for req := range chReqs { if req.Type == keepAliveRequestType { + channelCount.Add(1) _ = req.Reply(false, nil) } else if req.WantReply { _ = req.Reply(true, nil) @@ -493,13 +503,18 @@ func TestConnectionKeepAliveNegativeChannelReplyDoesNotReset(t *testing.T) { } }() + // The configured timeout is 200ms. Repeated negative channel replies + // must keep the connection alive and must not cause a global fallback. select { case <-closingFired: - case <-time.After(5 * time.Second): - t.Fatal("negative channel keepalive replies incorrectly reset the deadline") + t.Fatal("negative channel keepalive reply did not preserve liveness") + case <-time.After(700 * time.Millisecond): } - if globalCount.Load() == 0 { - t.Fatal("negative channel keepalive reply did not fall back to a global request") + if got := channelCount.Load(); got < 2 { + t.Fatalf("expected multiple negative channel keepalive replies, got %d", got) + } + if got := globalCount.Load(); got != 0 { + t.Fatalf("negative channel replies unexpectedly triggered %d global keepalives", got) } }