diff --git a/cmd/workspace/ssh.go b/cmd/workspace/ssh.go index 1bbddf31d..b12b7a032 100644 --- a/cmd/workspace/ssh.go +++ b/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 } diff --git a/cmd/workspace/ssh_keepalive_test.go b/cmd/workspace/ssh_keepalive_test.go index e0be00aab..b91dc92cc 100644 --- a/cmd/workspace/ssh_keepalive_test.go +++ b/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() +}