diff --git a/internal/hub/systems/ssh_timeout_test.go b/internal/hub/systems/ssh_timeout_test.go index d0b3c6711..217ac12dd 100644 --- a/internal/hub/systems/ssh_timeout_test.go +++ b/internal/hub/systems/ssh_timeout_test.go @@ -3,13 +3,17 @@ package systems import ( + "crypto/ed25519" + "crypto/rand" "errors" + "net" "sync" "testing" "testing/synctest" "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "golang.org/x/crypto/ssh" ) @@ -91,3 +95,120 @@ func TestCreateSessionDuringClose(t *testing.T) { wg.Wait() } } + +// TestDialSSHHandshakeTimeout covers a peer that accepts the TCP connection but +// never sends an SSH banner. Without a handshake deadline the dial blocks the +// updater forever (GHSA-h9jh-29rh-w464). +func TestDialSSHHandshakeTimeout(t *testing.T) { + prev := sshHandshakeTimeout + sshHandshakeTimeout = 200 * time.Millisecond + t.Cleanup(func() { sshHandshakeTimeout = prev }) + + ln, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + defer ln.Close() + + accepted := make(chan net.Conn, 1) + go func() { + conn, err := ln.Accept() + if err == nil { + accepted <- conn + } + }() + + config := &ssh.ClientConfig{ + User: "u", + HostKeyCallback: ssh.InsecureIgnoreHostKey(), + Timeout: 4 * time.Second, + } + + done := make(chan error, 1) + go func() { + client, err := dialSSHWithKeepAlive("tcp", ln.Addr().String(), config) + if client != nil { + client.Close() + } + done <- err + }() + + select { + case err := <-done: + assert.Error(t, err, "a silent peer must fail the handshake") + case <-time.After(5 * time.Second): + t.Fatal("dial blocked on a peer that never sends an SSH banner") + } + + // the hub must close its side of the connection + conn := <-accepted + defer conn.Close() + _ = conn.SetReadDeadline(time.Now().Add(2 * time.Second)) + _, err = conn.Read(make([]byte, 256)) + for err == nil { + _, err = conn.Read(make([]byte, 256)) + } + var netErr net.Error + assert.False(t, errors.As(err, &netErr) && netErr.Timeout(), "hub should close the connection, got %v", err) +} + +// TestDialSSHClearsHandshakeDeadline ensures the handshake deadline does not +// carry over to the established connection, which is reused for many updates. +func TestDialSSHClearsHandshakeDeadline(t *testing.T) { + prev := sshHandshakeTimeout + sshHandshakeTimeout = 200 * time.Millisecond + t.Cleanup(func() { sshHandshakeTimeout = prev }) + + _, hostPriv, err := ed25519.GenerateKey(rand.Reader) + require.NoError(t, err) + hostSigner, err := ssh.NewSignerFromKey(hostPriv) + require.NoError(t, err) + _, clientPriv, err := ed25519.GenerateKey(rand.Reader) + require.NoError(t, err) + clientSigner, err := ssh.NewSignerFromKey(clientPriv) + require.NoError(t, err) + + serverConfig := &ssh.ServerConfig{ + PublicKeyCallback: func(ssh.ConnMetadata, ssh.PublicKey) (*ssh.Permissions, error) { return nil, nil }, + } + serverConfig.AddHostKey(hostSigner) + + ln, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + defer ln.Close() + + go func() { + conn, err := ln.Accept() + if err != nil { + return + } + defer conn.Close() + _, chans, reqs, err := ssh.NewServerConn(conn, serverConfig) + if err != nil { + return + } + go ssh.DiscardRequests(reqs) + for newChan := range chans { + ch, chReqs, err := newChan.Accept() + if err != nil { + continue + } + go ssh.DiscardRequests(chReqs) + ch.Close() + } + }() + + config := &ssh.ClientConfig{ + User: "u", + Auth: []ssh.AuthMethod{ssh.PublicKeys(clientSigner)}, + HostKeyCallback: ssh.InsecureIgnoreHostKey(), + Timeout: 4 * time.Second, + } + client, err := dialSSHWithKeepAlive("tcp", ln.Addr().String(), config) + require.NoError(t, err) + defer client.Close() + + // wait past the handshake deadline; the connection must still be usable + time.Sleep(3 * sshHandshakeTimeout) + session, err := client.NewSession() + require.NoError(t, err, "connection should outlive the handshake deadline") + session.Close() +} diff --git a/internal/hub/systems/system.go b/internal/hub/systems/system.go index 91e4b945f..1bd1662a7 100644 --- a/internal/hub/systems/system.go +++ b/internal/hub/systems/system.go @@ -992,6 +992,12 @@ func (s *System) createSSHClient() error { // per-operation timeout in runSSHOperation instead (see issue #2041). const sshKeepAliveInterval = 30 * time.Second +// sshHandshakeTimeout bounds the SSH handshake after the TCP connection is +// established. ssh.ClientConfig.Timeout only covers the TCP connect, so a peer +// that accepts the connection but never sends an SSH banner would otherwise +// block the updater forever. +var sshHandshakeTimeout = 10 * time.Second + // dialSSHWithKeepAlive dials an SSH connection like ssh.Dial, but enables TCP // keep-alive on the underlying connection so half-open connections are // eventually detected by the operating system. @@ -1004,11 +1010,14 @@ func dialSSHWithKeepAlive(network, addr string, config *ssh.ClientConfig) (*ssh. if err != nil { return nil, err } + _ = conn.SetDeadline(time.Now().Add(sshHandshakeTimeout)) sshConn, chans, reqs, err := ssh.NewClientConn(conn, addr, config) if err != nil { _ = conn.Close() return nil, err } + // clear the handshake deadline so it doesn't apply to the long-lived connection + _ = conn.SetDeadline(time.Time{}) return ssh.NewClient(sshConn, chans, reqs), nil }