Merge pull request moby#3848 from sipsma/close-ssh-conn

ssh: add fallback to ensure conn is closed in all cases.

Author: sipsma

Dockerfile

@@ -235,7 +235,7 @@ COPY --link --from=dnsname /usr/bin/dnsname /opt/cni/bin/
 
 FROM buildkit-base AS integration-tests-base
 ENV BUILDKIT_INTEGRATION_ROOTLESS_IDPAIR="1000:1000"
-RUN apk add --no-cache shadow shadow-uidmap sudo vim iptables ip6tables dnsmasq fuse curl git-daemon \
+RUN apk add --no-cache shadow shadow-uidmap sudo vim iptables ip6tables dnsmasq fuse curl git-daemon openssh-client \
   && useradd --create-home --home-dir /home/user --uid 1000 -s /bin/sh user \
   && echo "XDG_RUNTIME_DIR=/run/user/1000; export XDG_RUNTIME_DIR" >> /home/user/.profile \
   && mkdir -m 0700 -p /run/user/1000 \

frontend/dockerfile/dockerfile_ssh_test.go

@@ -1,13 +1,16 @@
 package dockerfile
 
 import (
+	"bytes"
 	"crypto/rand"
 	"crypto/rsa"
 	"crypto/x509"
 	"encoding/pem"
 	"os"
+	"os/exec"
 	"path/filepath"
 	"testing"
+	"time"
 
 	"github.com/containerd/continuity/fs/fstest"
 	"github.com/moby/buildkit/client"
@@ -20,6 +23,7 @@ import (
 
 var sshTests = integration.TestFuncs(
 	testSSHSocketParams,
+	testSSHFileDescriptorsClosed,
 )
 
 func init() {
@@ -73,3 +77,74 @@ RUN --mount=type=ssh,mode=741,uid=100,gid=102 [ "$(stat -c "%u %g %f" $SSH_AUTH_
 	}, nil)
 	require.NoError(t, err)
 }
+
+func testSSHFileDescriptorsClosed(t *testing.T, sb integration.Sandbox) {
+	f := getFrontend(t, sb)
+
+	dockerfile := []byte(`
+FROM alpine
+RUN --mount=type=ssh apk update \
+ && apk add openssh-client-default \
+ && mkdir -p -m 0600 ~/.ssh \
+ && ssh-keyscan github.com >> ~/.ssh/known_hosts \
+ && for i in $(seq 1 3); do \
+        ssh -T [email protected]; \
+    done; \
+    exit 0;
+`)
+
+	dir, err := integration.Tmpdir(
+		t,
+		fstest.CreateFile("Dockerfile", dockerfile, 0600),
+	)
+	require.NoError(t, err)
+
+	c, err := client.New(sb.Context(), sb.Address())
+	require.NoError(t, err)
+	defer c.Close()
+
+	// not using t.TempDir() here because the path ends up longer than the unix socket max length
+	tmpDir, err := os.MkdirTemp("", "buildkit-ssh-test-")
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		os.RemoveAll(tmpDir)
+	})
+	sockPath := filepath.Join(tmpDir, "ssh-agent.sock")
+
+	sshAgentCmd := exec.CommandContext(sb.Context(), "ssh-agent", "-s", "-d", "-a", sockPath)
+	sshAgentOutputBuf := &bytes.Buffer{}
+	sshAgentCmd.Stderr = sshAgentOutputBuf
+	require.NoError(t, sshAgentCmd.Start())
+	var found bool
+	for i := 0; i < 100; i++ {
+		_, err := os.Stat(sockPath)
+		if err == nil {
+			found = true
+			break
+		}
+		time.Sleep(100 * time.Millisecond)
+	}
+	if !found {
+		sshAgentOutput := sshAgentOutputBuf.String()
+		t.Fatalf("ssh-agent failed to start: %s", sshAgentOutput)
+	}
+
+	ssh, err := sshprovider.NewSSHAgentProvider([]sshprovider.AgentConfig{{
+		Paths: []string{sockPath},
+	}})
+	require.NoError(t, err)
+
+	_, err = f.Solve(sb.Context(), c, client.SolveOpt{
+		LocalDirs: map[string]string{
+			dockerui.DefaultLocalNameDockerfile: dir,
+			dockerui.DefaultLocalNameContext:    dir,
+		},
+		Session: []session.Attachable{ssh},
+	}, nil)
+	require.NoError(t, err)
+
+	sshAgentOutput := sshAgentOutputBuf.String()
+	require.Contains(t, sshAgentOutput, "process_message: socket 1")
+	require.NotContains(t, sshAgentOutput, "process_message: socket 2")
+	require.NotContains(t, sshAgentOutput, "process_message: socket 3")
+}

session/sshforward/copy.go

@@ -24,10 +24,12 @@ func Copy(ctx context.Context, conn io.ReadWriteCloser, stream Stream, closeStre
 				if err == io.EOF {
 					// indicates client performed CloseSend, but they may still be
 					// reading data
-					if conn, ok := conn.(interface {
+					if closeWriter, ok := conn.(interface {
 						CloseWrite() error
 					}); ok {
-						conn.CloseWrite()
+						closeWriter.CloseWrite()
+					} else {
+						conn.Close()
 					}
 					return nil
 				}