fix(ssh): treat negative keepalive replies as liveness

cmd/workspace/ssh.go

@@ -2,7 +2,6 @@ package workspace
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -707,12 +706,8 @@ func startSSHKeepAlive(
 	}
 }
 
-func checkKeepAliveResponse(ok bool, err error) error {
-	if err != nil {
-		return err
-	}
-	if !ok {
-		return errors.New("keepalive request rejected")
-	}
-	return nil
+// A positive or negative SSH reply proves that the peer is alive. Only a
+// transport error means that the keepalive request failed.
+func checkKeepAliveResponse(_ bool, err error) error {
+	return err
 }

cmd/workspace/ssh_keepalive_test.go

@@ -1,8 +1,19 @@
 package workspace
 
 import (
+	"context"
+	"crypto/ed25519"
+	"crypto/rand"
+	"encoding/pem"
 	"errors"
+	"net"
+	"os/user"
 	"testing"
+	"time"
+
+	sshserver "github.com/devsy-org/devsy/pkg/ssh/server"
+	"github.com/stretchr/testify/require"
+	gossh "golang.org/x/crypto/ssh"
 )
 
 func TestCheckKeepAliveResponse(t *testing.T) {
@@ -16,8 +27,19 @@ func TestCheckKeepAliveResponse(t *testing.T) {
 		want error
 	}{
 		{name: "positive reply", ok: true},
-		{name: "negative reply", ok: false, want: errors.New("keepalive request rejected")},
-		{name: "transport error", ok: true, err: transportErr, want: transportErr},
+		{name: "negative reply proves peer liveness", ok: false},
+		{
+			name: "positive reply with transport error",
+			ok:   true,
+			err:  transportErr,
+			want: transportErr,
+		},
+		{
+			name: "negative reply with transport error",
+			ok:   false,
+			err:  transportErr,
+			want: transportErr,
+		},
 	}
 
 	for _, tt := range tests {
@@ -29,9 +51,65 @@ func TestCheckKeepAliveResponse(t *testing.T) {
 				}
 				return
 			}
-			if got == nil || got.Error() != tt.want.Error() {
+			if got == nil || !errors.Is(got, tt.want) {
 				t.Fatalf("checkKeepAliveResponse() = %v, want %v", got, tt.want)
 			}
 		})
 	}
 }
+
+func TestCheckKeepAliveResponseWithDevsySSHServer(t *testing.T) {
+	t.Parallel()
+
+	_, hostKey, err := ed25519.GenerateKey(rand.Reader)
+	require.NoError(t, err)
+	hostKeyBlock, err := gossh.MarshalPrivateKey(hostKey, "test host key")
+	require.NoError(t, err)
+	hostKeyPEM := pem.EncodeToMemory(hostKeyBlock)
+	hostSigner, err := gossh.NewSignerFromKey(hostKey)
+	require.NoError(t, err)
+
+	listener, err := net.Listen("tcp", "127.0.0.1:0")
+	require.NoError(t, err)
+	defer func() { _ = listener.Close() }()
+
+	server, err := sshserver.NewServer(
+		listener.Addr().String(),
+		hostKeyPEM,
+		nil,
+		t.TempDir(),
+		"",
+	)
+	require.NoError(t, err)
+
+	serveDone := make(chan error, 1)
+	go func() { serveDone <- server.Serve(listener) }()
+
+	currentUser, err := user.Current()
+	require.NoError(t, err)
+	client, err := gossh.Dial("tcp", listener.Addr().String(), &gossh.ClientConfig{
+		User:            currentUser.Username,
+		HostKeyCallback: gossh.FixedHostKey(hostSigner.PublicKey()),
+		Timeout:         2 * time.Second,
+	})
+	require.NoError(t, err)
+	defer func() {
+		_ = client.Close()
+		_ = server.Shutdown(context.Background())
+		select {
+		case <-serveDone:
+		case <-time.After(2 * time.Second):
+			t.Error("SSH server did not shut down")
+		}
+	}()
+
+	ok, _, err := client.SendRequest("keepalive@openssh.com", true, nil)
+	require.NoError(t, err)
+	require.False(t, ok, "the production server should reject its unsupported keepalive request")
+	require.NoError(t, checkKeepAliveResponse(ok, err))
+
+	session, err := client.NewSession()
+	require.NoError(t, err)
+	require.NoError(t, session.Run("true"))
+	_ = session.Close()
+}