diff --git a/cmd/helper/ssh_server.go b/cmd/helper/ssh_server.go index 337a5db5c..64c583761 100644 --- a/cmd/helper/ssh_server.go +++ b/cmd/helper/ssh_server.go @@ -97,6 +97,13 @@ func (cmd *SSHServerCmd) Run(_ *cobra.Command, _ []string) error { } } + // Sweep auth-agent-conn-* directories left behind by predecessors that + // were killed by docker exec / proxy-chain teardown before any internal + // SSH cleanup could run. Liveness is decided via a per-directory flock + // the owning process holds for its lifetime; the kernel releases the + // flock on any process exit (including SIGKILL). + helperssh.SweepStaleAgentSockets() + // start the server server, err := helperssh.NewServer( cmd.Address, diff --git a/e2e/framework/ssh_agent.go b/e2e/framework/ssh_agent.go new file mode 100644 index 000000000..7010ceda0 --- /dev/null +++ b/e2e/framework/ssh_agent.go @@ -0,0 +1,236 @@ +package framework + +import ( + "bytes" + "errors" + "fmt" + "io" + "os" + "os/exec" + "path/filepath" + "strings" + "time" + + "al.essio.dev/pkg/shellescape" +) + +type testLogger interface { + Helper() + Fatalf(format string, args ...any) +} + +// StartMockSSHAgent starts an isolated ssh-agent on a temporary socket path +// and loads a freshly generated ed25519 key into it. The returned cleanup +// function kills the agent process and removes the temp directory. +func StartMockSSHAgent(t testLogger) (authSock string, pubKey string, cleanup func()) { + t.Helper() + + tmpDir, err := os.MkdirTemp("", "devsy-mock-ssh-agent-") + if err != nil { + t.Fatalf("mkdtemp: %v", err) + } + + // Use a short socket path; long /tmp paths can exceed UNIX_PATH_MAX. + sockPath := filepath.Join(tmpDir, "agent.sock") + keyPath := filepath.Join(tmpDir, "id_ed25519") + + if err := generateAgentKey(keyPath); err != nil { + _ = os.RemoveAll(tmpDir) + t.Fatalf("%v", err) + } + + agentCmd, agentStderr, err := startSSHAgentProcess(sockPath) + if err != nil { + _ = os.RemoveAll(tmpDir) + t.Fatalf("%v", err) + } + + killAgent := func() { + if agentCmd.Process != nil { + _ = agentCmd.Process.Kill() + } + _ = agentCmd.Wait() + } + + if err := waitForAgentSocket(sockPath); err != nil { + killAgent() + _ = os.RemoveAll(tmpDir) + t.Fatalf("%v; stderr: %s", err, agentStderr.String()) + } + + if err := addKeyToAgent(sockPath, keyPath); err != nil { + killAgent() + _ = os.RemoveAll(tmpDir) + t.Fatalf("%v", err) + } + + pubBytes, err := os.ReadFile(keyPath + ".pub") // #nosec G304 + if err != nil { + killAgent() + _ = os.RemoveAll(tmpDir) + t.Fatalf("read pubkey: %v", err) + } + pubKey = strings.TrimSpace(string(pubBytes)) + + cleanup = func() { + killAgent() + _ = os.RemoveAll(tmpDir) + } + return sockPath, pubKey, cleanup +} + +func generateAgentKey(keyPath string) error { + // #nosec G204 -- test helper with controlled inputs + out, err := exec.Command("ssh-keygen", "-t", "ed25519", "-f", keyPath, "-N", "", "-q"). + CombinedOutput() + if err != nil { + return fmt.Errorf("ssh-keygen: %w: %s", err, out) + } + return nil +} + +// startSSHAgentProcess runs ssh-agent in the foreground (-D) bound to an +// explicit socket path (-a). Starting it via cmd.Start() avoids the +// daemonize/parent-exit race where the parent could return before the child +// bound the socket. +func startSSHAgentProcess(sockPath string) (*exec.Cmd, *bytes.Buffer, error) { + // #nosec G204 -- test helper with controlled inputs + cmd := exec.Command("ssh-agent", "-D", "-a", sockPath) + var stderr bytes.Buffer + cmd.Stderr = &stderr + cmd.Stdout = io.Discard + if err := cmd.Start(); err != nil { + return nil, nil, fmt.Errorf("ssh-agent start: %w", err) + } + return cmd, &stderr, nil +} + +// waitForAgentSocket polls until the agent's socket file appears so subsequent +// ssh-add invocations have something to connect to. +func waitForAgentSocket(sockPath string) error { + deadline := time.Now().Add(2 * time.Second) + for time.Now().Before(deadline) { + if _, statErr := os.Stat(sockPath); statErr == nil { + return nil + } + time.Sleep(25 * time.Millisecond) + } + return fmt.Errorf("ssh-agent did not bind socket %s within 2s", sockPath) +} + +func addKeyToAgent(sockPath, keyPath string) error { + cmd := exec.Command("ssh-add", keyPath) // #nosec G204 + cmd.Env = append(os.Environ(), "SSH_AUTH_SOCK="+sockPath) + out, err := cmd.CombinedOutput() + if err != nil { + return fmt.Errorf("ssh-add: %w: %s", err, out) + } + return nil +} + +// OpenSSHControlMaster starts an OpenSSH ControlMaster connection to the given +// devsy workspace host (typically ".devsy", as written by +// `devsy up`'s SSH config integration). Multiple sessions can then be opened +// over the same connection by re-invoking ssh with `-S `. +// +// Returns the controlPath, the host the caller should pass to multiplexed +// ssh invocations, and a closer that tears the master down. +// +// `extraEnv` is merged into the child ssh process environment (e.g. to set +// SSH_AUTH_SOCK for agent forwarding). +func OpenSSHControlMaster( + t testLogger, + workspaceHost string, + extraEnv map[string]string, +) (controlPath string, closer func(), err error) { + t.Helper() + + tmpDir, err := os.MkdirTemp("", "devsy-ssh-cm-") + if err != nil { + return "", nil, fmt.Errorf("mkdtemp: %w", err) + } + controlPath = filepath.Join(tmpDir, "cm.sock") + + // Spin the master in the background. -N: no command, -f: background after + // auth, -M: master mode, ControlPersist gives a window before timeout. + args := []string{ + "-o", "StrictHostKeyChecking=no", + "-o", "UserKnownHostsFile=/dev/null", + "-o", "ControlMaster=yes", + "-o", "ControlPath=" + controlPath, + "-o", "ControlPersist=120", + "-o", "ForwardAgent=yes", + "-N", "-f", + workspaceHost, + } + // #nosec G204 -- controlled args for test + cmd := exec.Command("ssh", args...) + cmd.Env = mergedEnv(extraEnv) + var stderr bytes.Buffer + cmd.Stderr = &stderr + cmd.Stdout = io.Discard + if err := cmd.Run(); err != nil { + _ = os.RemoveAll(tmpDir) + return "", nil, fmt.Errorf("ssh ControlMaster start: %w: %s", err, stderr.String()) + } + + closer = func() { + // Best-effort: ask the master to exit. + exitCmd := exec.Command( + "ssh", + "-O", + "exit", + "-o", + "ControlPath="+controlPath, + workspaceHost, + ) // #nosec G204 + exitCmd.Env = mergedEnv(extraEnv) + _ = exitCmd.Run() + _ = os.RemoveAll(tmpDir) + } + return controlPath, closer, nil +} + +// SSHMultiplexedExec runs cmd as a new SSH session on the existing +// ControlMaster connection at controlPath. +func SSHMultiplexedExec( + controlPath, host string, + extraEnv map[string]string, + cmd ...string, +) (stdout, stderr string, err error) { + if len(cmd) == 0 { + return "", "", errors.New("SSHMultiplexedExec: empty command") + } + args := []string{ + "-o", "ControlPath=" + controlPath, + "-o", "StrictHostKeyChecking=no", + "-o", "UserKnownHostsFile=/dev/null", + host, + "--", + } + // ssh joins remote argv with spaces before handing it to the remote + // login shell, which loses any in-argument quoting. Pre-quote each + // element so shell metacharacters (spaces, $, quotes) survive intact. + for _, a := range cmd { + args = append(args, shellescape.Quote(a)) + } + + // #nosec G204 -- controlled args for test + c := exec.Command("ssh", args...) + c.Env = mergedEnv(extraEnv) + var outBuf, errBuf bytes.Buffer + c.Stdout = &outBuf + c.Stderr = &errBuf + err = c.Run() + return outBuf.String(), errBuf.String(), err +} + +// mergedEnv returns os.Environ() with extra overrides applied last so they +// win on duplicate keys. +func mergedEnv(extra map[string]string) []string { + env := os.Environ() + for k, v := range extra { + env = append(env, k+"="+v) + } + return env +} diff --git a/e2e/tests/ssh/agent_forward.go b/e2e/tests/ssh/agent_forward.go new file mode 100644 index 000000000..f1386b8dc --- /dev/null +++ b/e2e/tests/ssh/agent_forward.go @@ -0,0 +1,381 @@ +package ssh + +import ( + "context" + "fmt" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "sync" + "time" + + "github.com/devsy-org/devsy/e2e/framework" + "github.com/onsi/ginkgo/v2" + "github.com/onsi/gomega" +) + +// SSH agent-forwarding regression tests: $SSH_AUTH_SOCK must be stable +// across sessions on one connection, distinct across connections, and the +// per-connection socket directory must be cleaned up on disconnect. +var _ = ginkgo.Describe( + "devsy ssh agent forwarding", + ginkgo.Label("ssh"), + ginkgo.Label("agent-forward"), + ginkgo.Ordered, + func() { + var ( + initialDir string + tempDir string + f *framework.Framework + authSock string + pubKey string + agentClean func() + host string + ) + + ginkgo.BeforeAll(func(ctx ginkgo.SpecContext) { + if runtime.GOOS == osWindows { + ginkgo.Skip("UNIX sockets required; skipping on windows") + } + var err error + initialDir, err = os.Getwd() + framework.ExpectNoError(err) + + tempDir, err = framework.CopyToTempDir("tests/ssh/testdata/agent-forward") + framework.ExpectNoError(err) + + f = framework.NewDefaultFramework(initialDir + "/bin") + _ = f.DevsyProviderAdd(ctx, "docker") + err = f.DevsyProviderUse(ctx, "docker") + framework.ExpectNoError(err) + + authSock, pubKey, agentClean = framework.StartMockSSHAgent(ginkgo.GinkgoT()) + + devsyUpCtx, cancel := context.WithTimeout(ctx, 5*time.Minute) + defer cancel() + err = f.DevsyUp(devsyUpCtx, tempDir) + framework.ExpectNoError(err) + + // devsy up registers an OpenSSH host alias ".devsy". + host = filepath.Base(tempDir) + ".devsy" + }) + + ginkgo.AfterAll(func(ctx ginkgo.SpecContext) { + if f != nil && tempDir != "" { + _ = f.DevsyWorkspaceDelete(ctx, tempDir) + framework.CleanupTempDir(initialDir, tempDir) + } + if agentClean != nil { + agentClean() + } + }) + + ginkgo.It( + "socket is stable across sessions on the same connection", + ginkgo.SpecTimeout(framework.TimeoutModerate()), + func(_ ginkgo.SpecContext) { + controlPath, closeCM, err := framework.OpenSSHControlMaster( + ginkgo.GinkgoT(), + host, + map[string]string{envSSHAuthSock: authSock}, + ) + framework.ExpectNoError(err) + ginkgo.DeferCleanup(closeCM) + + env := map[string]string{envSSHAuthSock: authSock} + + // Session A: capture $SSH_AUTH_SOCK inside the container and + // verify the forwarded key is reachable. + outA, errA, err := framework.SSHMultiplexedExec( + controlPath, host, env, + "sh", "-c", "printf %s \"$SSH_AUTH_SOCK\"", + ) + ginkgo.GinkgoWriter.Printf("session A stderr: %s\n", errA) + framework.ExpectNoError(err) + s1 := strings.TrimSpace(outA) + gomega.Expect(s1).NotTo(gomega.BeEmpty(), "session A must see SSH_AUTH_SOCK") + + outAddA, _, err := framework.SSHMultiplexedExec( + controlPath, host, env, + "ssh-add", "-L", + ) + framework.ExpectNoError(err) + gomega.Expect(outAddA).To( + gomega.ContainSubstring(strings.Fields(pubKey)[1]), + "session A: forwarded agent must expose our test pubkey", + ) + + // Session B: own env shows a socket file, and S1 from A is + // STILL alive (proves stability across sessions). + _, _, err = framework.SSHMultiplexedExec( + controlPath, host, env, + "sh", "-c", "test -S \"$SSH_AUTH_SOCK\"", + ) + framework.ExpectNoError( + err, + "session B: its own SSH_AUTH_SOCK must be a live socket", + ) + + outAddB, errB, err := framework.SSHMultiplexedExec( + controlPath, host, env, + "sh", "-c", "SSH_AUTH_SOCK="+s1+" ssh-add -L", + ) + ginkgo.GinkgoWriter.Printf("session B explicit-S1 stderr: %s\n", errB) + framework.ExpectNoError(err, "session B: socket from session A must still work") + gomega.Expect(outAddB).To(gomega.ContainSubstring(strings.Fields(pubKey)[1])) + + // Session B's reported SSH_AUTH_SOCK should equal S1 too — the + // regression is that this used to differ session-to-session. + outBSock, _, err := framework.SSHMultiplexedExec( + controlPath, host, env, + "sh", "-c", "printf %s \"$SSH_AUTH_SOCK\"", + ) + framework.ExpectNoError(err) + gomega.Expect(strings.TrimSpace(outBSock)).To( + gomega.Equal(s1), + "SSH_AUTH_SOCK must be identical across sessions on one connection", + ) + + // Session C: open-remote-ssh style use-after-session pattern — + // reuse S1 in yet another session. + outAddC, _, err := framework.SSHMultiplexedExec( + controlPath, host, env, + "sh", "-c", "SSH_AUTH_SOCK="+s1+" ssh-add -L", + ) + framework.ExpectNoError(err, "session C: S1 must still work") + gomega.Expect(outAddC).To(gomega.ContainSubstring(strings.Fields(pubKey)[1])) + }, + ) + + ginkgo.It( + "each connection gets its own socket", + ginkgo.SpecTimeout(framework.TimeoutModerate()), + func(_ ginkgo.SpecContext) { + env := map[string]string{envSSHAuthSock: authSock} + + cp1, close1, err := framework.OpenSSHControlMaster(ginkgo.GinkgoT(), host, env) + framework.ExpectNoError(err) + closed1 := false + ginkgo.DeferCleanup(func() { + if !closed1 { + close1() + } + }) + + cp2, close2, err := framework.OpenSSHControlMaster(ginkgo.GinkgoT(), host, env) + framework.ExpectNoError(err) + ginkgo.DeferCleanup(close2) + + out1, _, err := framework.SSHMultiplexedExec( + cp1, host, env, "sh", "-c", "printf %s \"$SSH_AUTH_SOCK\"", + ) + framework.ExpectNoError(err) + sock1 := strings.TrimSpace(out1) + + out2, _, err := framework.SSHMultiplexedExec( + cp2, host, env, "sh", "-c", "printf %s \"$SSH_AUTH_SOCK\"", + ) + framework.ExpectNoError(err) + sock2 := strings.TrimSpace(out2) + + gomega.Expect(sock1).NotTo(gomega.BeEmpty()) + gomega.Expect(sock2).NotTo(gomega.BeEmpty()) + gomega.Expect(sock1).NotTo( + gomega.Equal(sock2), + "two independent connections must each get their own agent socket", + ) + + // Close connection 1 — connection 2 must still work. + close1() + closed1 = true + + outAdd, _, err := framework.SSHMultiplexedExec( + cp2, host, env, "ssh-add", "-L", + ) + framework.ExpectNoError( + err, + "connection 2 must remain functional after connection 1 closes", + ) + gomega.Expect(outAdd).To(gomega.ContainSubstring(strings.Fields(pubKey)[1])) + }, + ) + + ginkgo.It( + "socket directory is cleaned up after connection close", + ginkgo.SpecTimeout(framework.TimeoutModerate()), + func(ctx ginkgo.SpecContext) { + env := map[string]string{envSSHAuthSock: authSock} + + cp, closeCM, err := framework.OpenSSHControlMaster(ginkgo.GinkgoT(), host, env) + framework.ExpectNoError(err) + closed := false + ginkgo.DeferCleanup(func() { + if !closed { + closeCM() + } + }) + + out, _, err := framework.SSHMultiplexedExec( + cp, host, env, "sh", "-c", "printf %s \"$SSH_AUTH_SOCK\"", + ) + framework.ExpectNoError(err) + sockPath := strings.TrimSpace(out) + gomega.Expect(sockPath).NotTo(gomega.BeEmpty()) + + // Confirm the socket exists from inside the container before close. + _, _, err = framework.SSHMultiplexedExec( + cp, host, env, "sh", "-c", "test -S "+sockPath, + ) + framework.ExpectNoError(err, "socket must exist while connection is open") + + closeCM() + closed = true + + // The cleanup hop traverses the devsy proxy → in-container + // SSH server's ctx.Done(), which can take several seconds + // under CI load. Each devsy ssh observation runs on a fresh + // connection so the just-closed socket's filesystem state is + // always up-to-date. + gomega.Eventually(func() string { + pollCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + out, _ := f.DevsySSH( + pollCtx, + tempDir, + "test -S "+sockPath+" && echo PRESENT || echo GONE", + ) + return out + }).WithTimeout(30*time.Second).WithPolling(500*time.Millisecond). + Should( + gomega.ContainSubstring("GONE"), + "socket %s must be cleaned up after connection close", + sockPath, + ) + }, + ) + + ginkgo.It( + "connection without any agent request still cleans up", + ginkgo.Label("ssh"), + ginkgo.Label("agent-forward"), + ginkgo.SpecTimeout(framework.TimeoutModerate()), + func(ctx ginkgo.SpecContext) { + tmpDir, err := os.MkdirTemp("", "devsy-ssh-cm-clean-") + framework.ExpectNoError(err) + controlPath := filepath.Join(tmpDir, "cm.sock") + ginkgo.DeferCleanup(func() { + _ = os.RemoveAll(tmpDir) + }) + + startArgs := []string{ + "-o", sshOptStrictHostKeyCheckingNo, + "-o", sshOptUserKnownHostsFileNull, + "-o", "ControlMaster=yes", + "-o", "ControlPath=" + controlPath, + "-o", "ControlPersist=120", + "-o", sshOptForwardAgentNo, + "-N", "-f", + host, + } + // #nosec G204 + startCmd := exec.Command("ssh", startArgs...) + startCmd.Env = append(os.Environ(), "SSH_AUTH_SOCK="+authSock) + combined, startErr := startCmd.CombinedOutput() + framework.ExpectNoError( + startErr, + "ssh ControlMaster start (no agent): %s", + combined, + ) + + // Run a trivial command without requesting forwarding. + trivialArgs := []string{ + "-o", "ControlPath=" + controlPath, + "-o", sshOptStrictHostKeyCheckingNo, + "-o", sshOptUserKnownHostsFileNull, + "-o", sshOptForwardAgentNo, + host, "--", + "true", + } + // #nosec G204 + trivCmd := exec.Command("ssh", trivialArgs...) + trivCmd.Env = append(os.Environ(), "SSH_AUTH_SOCK="+authSock) + trivOut, trivErr := trivCmd.CombinedOutput() + framework.ExpectNoError(trivErr, "trivial session: %s", trivOut) + + // Close the master connection. + // #nosec G204 + exitCmd := exec.Command("ssh", "-O", "exit", + "-o", "ControlPath="+controlPath, host) + exitCmd.Env = append(os.Environ(), "SSH_AUTH_SOCK="+authSock) + _ = exitCmd.Run() + + // On a fresh devsy ssh connection, assert no auth-agent-conn-* + // directories remain. With lazy allocation, none are ever + // created; with the cleanup goroutine, any leftover is removed. + gomega.Eventually(func() string { + pollCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + out, _ := f.DevsySSH( + pollCtx, + tempDir, + "sh -c 'ls -d \"$XDG_RUNTIME_DIR\"/auth-agent-conn-* 2>/dev/null | wc -l'", + ) + return strings.TrimSpace(out) + }).WithTimeout(30*time.Second).WithPolling(500*time.Millisecond). + Should( + gomega.Equal("0"), + "no auth-agent-conn-* dir must remain after a no-forward connection closes", + ) + }, + ) + + ginkgo.It( + "parallel sessions on one connection observe the same socket concurrently", + ginkgo.Label("agent-forward"), + ginkgo.SpecTimeout(framework.TimeoutModerate()), + func(_ ginkgo.SpecContext) { + controlPath, closeCM, err := framework.OpenSSHControlMaster( + ginkgo.GinkgoT(), + host, + map[string]string{envSSHAuthSock: authSock}, + ) + framework.ExpectNoError(err) + ginkgo.DeferCleanup(closeCM) + + env := map[string]string{envSSHAuthSock: authSock} + + const n = 4 + var wg sync.WaitGroup + results := make([]string, n) + errs := make([]error, n) + for i := range n { + wg.Add(1) + go func(idx int) { + defer wg.Done() + out, _, runErr := framework.SSHMultiplexedExec( + controlPath, host, env, + "sh", "-c", "printf %s \"$SSH_AUTH_SOCK\"", + ) + results[idx] = strings.TrimSpace(out) + errs[idx] = runErr + }(i) + } + wg.Wait() + + for i, e := range errs { + framework.ExpectNoError(e, fmt.Sprintf("session %d failed", i)) + } + first := results[0] + gomega.Expect(first).NotTo(gomega.BeEmpty()) + for i, r := range results { + gomega.Expect(r).To( + gomega.Equal(first), + fmt.Sprintf("session %d saw %q, expected %q", i, r, first), + ) + } + }, + ) + }, +) diff --git a/e2e/tests/ssh/ssh.go b/e2e/tests/ssh/ssh.go index 0d7f5fa8e..34f1be2ef 100644 --- a/e2e/tests/ssh/ssh.go +++ b/e2e/tests/ssh/ssh.go @@ -18,7 +18,15 @@ import ( "github.com/onsi/gomega" ) -const osWindows = "windows" +const ( + osWindows = "windows" + + envSSHAuthSock = "SSH_AUTH_SOCK" + + sshOptStrictHostKeyCheckingNo = "StrictHostKeyChecking=no" + sshOptUserKnownHostsFileNull = "UserKnownHostsFile=/dev/null" + sshOptForwardAgentNo = "ForwardAgent=no" +) var _ = ginkgo.Describe("devsy ssh test suite", ginkgo.Label("ssh"), ginkgo.Ordered, func() { var initialDir string diff --git a/e2e/tests/ssh/testdata/agent-forward/devcontainer.json b/e2e/tests/ssh/testdata/agent-forward/devcontainer.json new file mode 100644 index 000000000..9182ba364 --- /dev/null +++ b/e2e/tests/ssh/testdata/agent-forward/devcontainer.json @@ -0,0 +1,5 @@ +{ + "name": "Agent Forward", + "image": "alpine:3.20", + "onCreateCommand": "apk add --no-cache openssh-client git" +} diff --git a/go.mod b/go.mod index 3e326d0d6..61f959b33 100644 --- a/go.mod +++ b/go.mod @@ -20,7 +20,7 @@ require ( github.com/devsy-org/agentapi v1.0.1 github.com/devsy-org/api v1.1.0 github.com/devsy-org/apiserver v1.5.0 - github.com/devsy-org/ssh v1.1.0 + github.com/devsy-org/ssh v1.2.0 github.com/distribution/reference v0.6.0 github.com/docker/cli v29.4.0+incompatible github.com/docker/docker v28.5.2+incompatible diff --git a/go.sum b/go.sum index 3604b6106..8b73d6ccc 100644 --- a/go.sum +++ b/go.sum @@ -233,8 +233,8 @@ github.com/devsy-org/api v1.1.0 h1:l7T9k7RVwatwN4lxeDTF3iN6EYmfGZgR3ZTJMDGha1M= github.com/devsy-org/api v1.1.0/go.mod h1:mAZklKdnywJYiXDReBLte/H+3m69z6G7RHB3n1lI53Q= github.com/devsy-org/apiserver v1.5.0 h1:tAr2tH9QwtNEu8BTbBgHkJD6ae43S5LDdHGphLsELB8= github.com/devsy-org/apiserver v1.5.0/go.mod h1:2jKuqy8XdH/g4bIjmkj3efQgshN/NjMzm17SkvvSR88= -github.com/devsy-org/ssh v1.1.0 h1:LKwUPuyijX+x8L4zzEsObBptRjr8OTKugVCl3UPMTOs= -github.com/devsy-org/ssh v1.1.0/go.mod h1:+/FJAVr49LxNS0E4IPOXQZOnwrvNqc6cNLT7unu3MD4= +github.com/devsy-org/ssh v1.2.0 h1:rwECFiTdfqnHX9Ft9ZxTbHM504If2vNs8istL8hdngo= +github.com/devsy-org/ssh v1.2.0/go.mod h1:+/FJAVr49LxNS0E4IPOXQZOnwrvNqc6cNLT7unu3MD4= github.com/devsy-org/tailscale v1.92.2 h1:8Ew/cGVUaQa9N6Acab9SgXDWJp/Kzn3T0UejLyWPTfo= github.com/devsy-org/tailscale v1.92.2/go.mod h1:dYlHLLo1QUXtOCpXiIe8fdkZVAcQZFnjfwTdXI25fDc= github.com/dgryski/go-metro v0.0.0-20180109044635-280f6062b5bc h1:8WFBn63wegobsYAX0YjD+8suexZDga5CctH4CCTx2+8= diff --git a/pkg/ssh/server/agent.go b/pkg/ssh/server/agent.go index be728704c..009d09409 100644 --- a/pkg/ssh/server/agent.go +++ b/pkg/ssh/server/agent.go @@ -5,11 +5,15 @@ import ( "net" "os" "path/filepath" + "strings" "github.com/devsy-org/devsy/pkg/config" + "github.com/devsy-org/devsy/pkg/log" "github.com/devsy-org/ssh" ) +const agentSocketDirPrefix = "auth-agent-conn-" + func setupAgentListener(reuseSock string) (net.Listener, string, error) { runtimeDir, err := config.DefaultPathManager().RuntimeDir() if err != nil { @@ -42,3 +46,96 @@ func setupAgentListener(reuseSock string) (net.Listener, string, error) { return l, tmpDir, nil } + +// setupConnectionAgentListener creates a per-connection agent forwarding +// listener whose lifetime spans the entire SSH connection (not a single +// session). The connID should be a short hex string unique to the connection. +// The returned socketDir is the directory containing the unix socket and +// should be removed via cleanupAgentSocketDir once the connection closes. +func setupConnectionAgentListener(connID string) (net.Listener, string, error) { + runtimeDir, err := config.DefaultPathManager().RuntimeDir() + if err != nil { + return nil, "", fmt.Errorf("runtime dir: %w", err) + } + + err = os.MkdirAll(runtimeDir, 0o755) // #nosec G301 + if err != nil { + return nil, "", fmt.Errorf("create runtime dir: %w", err) + } + + dir := filepath.Join(runtimeDir, fmt.Sprintf("%s%s", agentSocketDirPrefix, connID)) + // #nosec G301 + err = os.MkdirAll(dir, 0o755) + if err != nil { + return nil, "", fmt.Errorf("creating SSH_AUTH_SOCK dir: %w", err) + } + + // Hold an exclusive flock on a per-directory lockfile for the lifetime + // of this process. The kernel releases the lock on any exit (including + // SIGKILL by docker exec when the proxy chain tears down). This is the + // only cleanup signal that survives external process termination — the + // connection-level ConnectionClosingCallback never fires in that case + // because the in-container helper is killed by signal, not via SSH + // disconnect. The startup janitor (SweepStaleAgentSockets) uses this + // lock to detect and remove orphaned directories. + if err := takeAgentDirLock(dir); err != nil { + return nil, "", err + } + + l, socketDir, err := ssh.NewAgentListener(dir) + if err != nil { + releaseAgentDirLock(dir) + cleanupAgentSocketDir(dir) + return nil, "", fmt.Errorf("new agent listener: %w", err) + } + + return l, socketDir, nil +} + +// SweepStaleAgentSockets walks the runtime directory and removes any +// auth-agent-conn-* directory whose owning process is no longer alive. +// Liveness is detected via a per-directory flock: the owning process holds +// an exclusive flock on the lockfile for its lifetime, so any other process +// that can successfully take the flock knows the original owner is gone. +// +// Call this at the start of every helper ssh-server process: in stdio mode +// the in-container helper is often SIGKILLed by docker exec when the proxy +// chain tears down, which skips all deferred cleanup (including +// ConnectionClosingCallback). Subsequent helper invocations sweep what the +// dying predecessor couldn't clean up. +func SweepStaleAgentSockets() { + runtimeDir, err := config.DefaultPathManager().RuntimeDir() + if err != nil { + return + } + entries, err := os.ReadDir(runtimeDir) + if err != nil { + return + } + for _, e := range entries { + if !e.IsDir() || !strings.HasPrefix(e.Name(), agentSocketDirPrefix) { + continue + } + dirPath := filepath.Join(runtimeDir, e.Name()) + if !agentDirIsStale(dirPath) { + continue + } + if err := os.RemoveAll(dirPath); err != nil { + log.Debugf("sweep stale agent dir %s: %v", dirPath, err) + continue + } + log.Debugf("swept stale agent socket dir: %s", dirPath) + } +} + +// cleanupAgentSocketDir removes the per-connection agent socket directory. +// Errors are logged at debug level since cleanup is best-effort. +func cleanupAgentSocketDir(path string) { + if path == "" { + return + } + err := os.RemoveAll(path) + if err != nil { + log.Debugf("cleanup agent socket dir %s: %v", path, err) + } +} diff --git a/pkg/ssh/server/agent_lock_unix.go b/pkg/ssh/server/agent_lock_unix.go new file mode 100644 index 000000000..548ab835b --- /dev/null +++ b/pkg/ssh/server/agent_lock_unix.go @@ -0,0 +1,80 @@ +//go:build !windows + +package server + +import ( + "fmt" + "os" + "path/filepath" + "sync" + "syscall" +) + +const agentLockFilename = ".owner.lock" + +// agentLockFDs keeps the lockfile descriptors for each owned socket dir +// alive for the lifetime of the process. The kernel auto-releases the +// flock when the process dies for any reason (including SIGKILL), which is +// what lets a later helper invocation's startup janitor detect orphaned +// directories. +var ( + agentLockFDsMu sync.Mutex + agentLockFDs []*os.File +) + +// takeAgentDirLock opens the lockfile inside dir and acquires an exclusive +// non-blocking flock on it, retaining the descriptor in a process-wide slot +// so the lock is held until process exit. +func takeAgentDirLock(dir string) error { + lockPath := filepath.Join(dir, agentLockFilename) + // #nosec G304 -- lockPath is derived from controlled inputs (runtimeDir + connID). + f, err := os.OpenFile(lockPath, os.O_CREATE|os.O_RDWR, 0o600) + if err != nil { + return fmt.Errorf("open lockfile: %w", err) + } + fd := int(f.Fd()) //nolint:gosec // os.File.Fd() fits in int on supported platforms + if err := syscall.Flock(fd, syscall.LOCK_EX|syscall.LOCK_NB); err != nil { + _ = f.Close() + return fmt.Errorf("flock lockfile %s: %w", lockPath, err) + } + agentLockFDsMu.Lock() + agentLockFDs = append(agentLockFDs, f) + agentLockFDsMu.Unlock() + return nil +} + +// releaseAgentDirLock drops a previously-acquired lock on dir's lockfile. +// Used on setup failure paths after takeAgentDirLock succeeded; the steady- +// state lifetime is process exit, where the kernel auto-releases. +func releaseAgentDirLock(dir string) { + lockPath := filepath.Join(dir, agentLockFilename) + agentLockFDsMu.Lock() + defer agentLockFDsMu.Unlock() + for i, f := range agentLockFDs { + if f.Name() != lockPath { + continue + } + _ = f.Close() + agentLockFDs = append(agentLockFDs[:i], agentLockFDs[i+1:]...) + return + } +} + +// agentDirIsStale reports whether the agent socket directory's owner is no +// longer alive. It probes the lockfile with a non-blocking exclusive flock: +// success means the original owner has exited. +func agentDirIsStale(dir string) bool { + lockPath := filepath.Join(dir, agentLockFilename) + // #nosec G304 -- lockPath is derived from controlled directory listing. + f, err := os.OpenFile(lockPath, os.O_CREATE|os.O_RDWR, 0o600) + if err != nil { + return false + } + defer func() { _ = f.Close() }() + fd := int(f.Fd()) //nolint:gosec // os.File.Fd() fits in int on supported platforms + if err := syscall.Flock(fd, syscall.LOCK_EX|syscall.LOCK_NB); err != nil { + return false + } + _ = syscall.Flock(fd, syscall.LOCK_UN) + return true +} diff --git a/pkg/ssh/server/agent_lock_windows.go b/pkg/ssh/server/agent_lock_windows.go new file mode 100644 index 000000000..43b188591 --- /dev/null +++ b/pkg/ssh/server/agent_lock_windows.go @@ -0,0 +1,14 @@ +//go:build windows + +package server + +// takeAgentDirLock is a no-op on Windows; the agent-forwarding code path +// is only exercised by the unix-socket-based SSH server, so a stale-dir +// detection mechanism is not needed here. +func takeAgentDirLock(string) error { return nil } + +// releaseAgentDirLock is a no-op on Windows; see takeAgentDirLock. +func releaseAgentDirLock(string) {} + +// agentDirIsStale is a no-op on Windows; see takeAgentDirLock. +func agentDirIsStale(string) bool { return false } diff --git a/pkg/ssh/server/agent_test.go b/pkg/ssh/server/agent_test.go new file mode 100644 index 000000000..a5c1739bd --- /dev/null +++ b/pkg/ssh/server/agent_test.go @@ -0,0 +1,142 @@ +package server + +import ( + "crypto/rand" + "encoding/hex" + "os" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const osWindows = "windows" + +// uniqueConnID returns a short random hex connID so parallel test binaries +// (e.g. goreleaser running pre-hook tests for multiple targets at once) +// don't collide on the shared runtime-dir flock. Kept short because macOS +// UNIX_PATH_MAX is only 104 bytes and the runtime dir prefix is already +// ~50 bytes there. +func uniqueConnID(t *testing.T) string { + t.Helper() + var b [4]byte + _, err := rand.Read(b[:]) + require.NoError(t, err) + return "t" + hex.EncodeToString(b[:]) +} + +func TestSetupConnectionAgentListener_HappyPath(t *testing.T) { + if runtime.GOOS == osWindows { + t.Skip("unix socket based test") + } + + l, socketDir, err := setupConnectionAgentListener(uniqueConnID(t)) + require.NoError(t, err) + require.NotNil(t, l) + t.Cleanup(func() { + _ = l.Close() + cleanupAgentSocketDir(socketDir) + }) + + addr := l.Addr().String() + assert.NotEmpty(t, addr, "listener should have an address") + assert.True( + t, + strings.HasPrefix(addr, socketDir+string(os.PathSeparator)) || + filepath.Dir(addr) == socketDir, + "socket %q should live under socketDir %q", + addr, + socketDir, + ) + + info, statErr := os.Stat(socketDir) + require.NoError(t, statErr) + assert.True(t, info.IsDir(), "socketDir should be a directory") + + assert.NoError(t, l.Close()) +} + +func TestCleanupAgentSocketDir(t *testing.T) { + if runtime.GOOS == osWindows { + t.Skip("unix socket based test") + } + + t.Run("removes_existing_dir", func(t *testing.T) { + dir := filepath.Join(t.TempDir(), "cleanup-target") + require.NoError(t, os.MkdirAll(dir, 0o750)) + + // Drop a file inside to make sure RemoveAll actually clears it. + require.NoError(t, os.WriteFile(filepath.Join(dir, "sock"), []byte("x"), 0o600)) + + cleanupAgentSocketDir(dir) + + _, err := os.Stat(dir) + assert.True(t, os.IsNotExist(err), "directory should be gone, stat err=%v", err) + }) + + t.Run("empty_path_is_noop", func(t *testing.T) { + // Must not panic. + cleanupAgentSocketDir("") + }) + + t.Run("nonexistent_path_is_noop", func(t *testing.T) { + // Must not panic / explode; os.RemoveAll returns nil for nonexistent. + cleanupAgentSocketDir(filepath.Join(t.TempDir(), "does-not-exist")) + }) +} + +func TestSetupConnectionAgentListener_BadRuntimeDir(t *testing.T) { + if runtime.GOOS == osWindows { + t.Skip("unix socket based test") + } + + // linuxPathManager.RuntimeDir derives from os.TempDir(), which honors + // TMPDIR. Point it under a non-directory path so MkdirAll fails. On + // darwin, os.TempDir() may not honor TMPDIR in the same way; in that + // case the test still exercises a read-only parent and should fail. + // + // /dev/null is a character device, so attempting to create a directory + // underneath it will always fail with ENOTDIR. + t.Setenv("TMPDIR", "/dev/null/definitely-not-a-dir") + + l, socketDir, err := setupConnectionAgentListener(uniqueConnID(t)) + if err == nil { + // Some platforms (notably darwin) ignore TMPDIR for the + // confstr-derived temp dir. Clean up and skip. + _ = l.Close() + cleanupAgentSocketDir(socketDir) + t.Skip("platform does not honor TMPDIR for temp dir resolution") + } + assert.Nil(t, l) + assert.Empty(t, socketDir) +} + +func TestNewConnAgentState_LifecycleAndSockPath(t *testing.T) { + if runtime.GOOS == osWindows { + t.Skip("unix socket based test") + } + + state, err := newConnAgentState(uniqueConnID(t)) + require.NoError(t, err) + require.NotNil(t, state) + + assert.Equal(t, state.listener.Addr().String(), state.sockPath(), + "sockPath must mirror listener.Addr()") + assert.NotEmpty(t, state.socketDir) + + state.close() + _, statErr := os.Stat(state.socketDir) + assert.True( + t, + os.IsNotExist(statErr), + "socketDir should be gone after close, stat err=%v", + statErr, + ) + + // close on a nil receiver must be safe. + var nilState *connAgentState + nilState.close() +} diff --git a/pkg/ssh/server/ssh.go b/pkg/ssh/server/ssh.go index 227826399..518785ed0 100644 --- a/pkg/ssh/server/ssh.go +++ b/pkg/ssh/server/ssh.go @@ -1,17 +1,91 @@ package server import ( + "crypto/rand" + "crypto/sha256" + "encoding/hex" "fmt" "net" "os" "os/exec" "os/user" + "strconv" + "sync" + "time" "github.com/devsy-org/devsy/pkg/log" "github.com/devsy-org/devsy/pkg/shell" "github.com/devsy-org/ssh" + gossh "golang.org/x/crypto/ssh" ) +// ctxKey is a private type for ssh.Context keys. +type ctxKey int + +const ( + ctxKeyConnAgent ctxKey = iota +) + +// connAgentIntent is stored on the ssh.Context for every non-reuseSock +// connection. It carries the connection id plus a lazily-populated +// per-connection agent state. The actual listener + socket directory are +// only allocated on the first session that requests agent forwarding. +type connAgentIntent struct { + connID string + mu sync.Mutex + state *connAgentState + setErr error + inited bool +} + +// connAgentState holds the per-connection agent forwarding listener and +// its socket directory. ForwardAgentConnections is started lazily on the +// first session that requests agent forwarding, since it requires an +// ssh.Session to open the auth-agent channel back to the client. +type connAgentState struct { + listener net.Listener + socketDir string + socket string + once sync.Once +} + +// newConnAgentState constructs the per-connection agent state, allocating +// the unix-socket listener and its containing directory. The socket path +// always mirrors listener.Addr().String() so callers cannot drift the two. +func newConnAgentState(connID string) (*connAgentState, error) { + l, socketDir, err := setupConnectionAgentListener(connID) + if err != nil { + return nil, err + } + return &connAgentState{ + listener: l, + socketDir: socketDir, + socket: l.Addr().String(), + }, nil +} + +func (c *connAgentState) sockPath() string { + return c.socket +} + +// startForwarding launches ForwardAgentConnections at most once per +// connection, bound to the first session that requests agent forwarding. +func (c *connAgentState) startForwarding(sess ssh.Session) { + c.once.Do(func() { + go ssh.ForwardAgentConnections(c.listener, sess) + }) +} + +func (c *connAgentState) close() { + if c == nil { + return + } + if c.listener != nil { + _ = c.listener.Close() + } + cleanupAgentSocketDir(c.socketDir) +} + const ( DefaultPort int = 8022 DefaultUserPort int = 12023 @@ -56,6 +130,13 @@ func NewServer( currentUser: currentUser.Username, sshServer: ssh.Server{ Addr: addr, + // Keep-alive at the connection level: detects dead peers in + // stdio mode where EOF on stdin can be delayed indefinitely by + // the proxy chain. 5s × 2 = ~10s detection so per-connection + // agent socket dirs are cleaned up well within typical + // test/health-check polling windows. + ClientAliveInterval: 5 * time.Second, + ClientAliveCountMax: 2, LocalPortForwardingCallback: func(ctx ssh.Context, dhost string, dport uint32) bool { log.Debugf("Accepted forward: %s:%d", dhost, dport) return true @@ -116,26 +197,108 @@ func NewServer( } server.sshServer.Handler = server.handler + server.sshServer.ConnCallback = server.connCallback + server.sshServer.ConnectionClosingCallback = cleanupAgentOnConnClosing return server, nil } +// cleanupAgentOnConnClosing tears down the per-connection agent state when +// HandleConn observes the inbound channels stream close. Runs synchronously +// in HandleConn's defer chain before sshConn.Wait() — so it fires reliably +// even when the underlying transport is stuck (e.g. in stdio mode where EOF +// on stdin can be delayed by the proxy chain). +func cleanupAgentOnConnClosing(ctx ssh.Context, _ *gossh.ServerConn) { + v := ctx.Value(ctxKeyConnAgent) + if v == nil { + return + } + intent, ok := v.(*connAgentIntent) + if !ok || intent == nil { + return + } + intent.mu.Lock() + state := intent.state + intent.mu.Unlock() + if state == nil { + log.Debugf("ssh conn close: connID=%s (no listener allocated)", intent.connID) + return + } + sock := state.sockPath() + state.close() + log.Debugf("ssh conn close: connID=%s agent_sock=%s cleaned up", intent.connID, sock) +} + +// newConnID returns a short hex identifier unique to the connection. +// Uses crypto/rand; on the unlikely event of a rand.Read failure falls +// back to a sha256-of-time derivation so the connection still gets an ID. +func newConnID(remote string) string { + var b [8]byte + _, err := rand.Read(b[:]) + if err == nil { + return hex.EncodeToString(b[:]) + } + sum := sha256.Sum256([]byte(remote + strconv.FormatInt(time.Now().UnixNano(), 10))) + return fmt.Sprintf("%x", sum)[:16] +} + +// ensureConnAgentState lazily allocates the per-connection agent listener +// the first time an agent-forwarding session arrives. Subsequent calls +// return the same state (or the same error). +func (intent *connAgentIntent) ensureState() (*connAgentState, error) { + intent.mu.Lock() + defer intent.mu.Unlock() + if intent.inited { + return intent.state, intent.setErr + } + intent.inited = true + state, err := newConnAgentState(intent.connID) + intent.state = state + intent.setErr = err + return state, err +} + func (s *server) handler(sess ssh.Session) { var err error ptyReq, winCh, isPty := sess.Pty() cmd := s.getCommand(sess, isPty) if ssh.AgentRequested(sess) { - l, tmpDir, err := setupAgentListener(s.reuseSock) - if err != nil { - exitWithError(sess, err) - return - } - defer func() { _ = l.Close() }() - defer func() { _ = os.RemoveAll(tmpDir) }() + if s.reuseSock != "" { + // openvscode backhaul / explicit shared-socket mode: keep the + // existing per-session listener behavior. + l, tmpDir, err := setupAgentListener(s.reuseSock) + if err != nil { + exitWithError(sess, err) + return + } + defer func() { _ = l.Close() }() + defer func() { _ = os.RemoveAll(tmpDir) }() - go ssh.ForwardAgentConnections(l, sess) + go ssh.ForwardAgentConnections(l, sess) - cmd.Env = append(cmd.Env, fmt.Sprintf("%s=%s", "SSH_AUTH_SOCK", l.Addr().String())) + cmd.Env = append(cmd.Env, fmt.Sprintf("%s=%s", "SSH_AUTH_SOCK", l.Addr().String())) + } else if intent, ok := sess.Context().Value(ctxKeyConnAgent).(*connAgentIntent); ok && intent != nil { + // Common interactive case: lazily allocate the connection-scoped + // listener on first request, then reuse it for every subsequent + // session on the same connection. ForwardAgentConnections needs an + // ssh.Session to open the auth-agent channel, so it is bound to + // the first session that requests agent forwarding. + state, sErr := intent.ensureState() + if sErr != nil || state == nil { + log.Errorf("ssh agent forwarding setup failed (connID=%s): %v", intent.connID, sErr) + _, _ = fmt.Fprintf( + sess.Stderr(), + "warning: ssh agent forwarding unavailable: %v\n", + sErr, + ) + exitWithError(sess, sErr) + return + } + state.startForwarding(sess) + cmd.Env = append(cmd.Env, fmt.Sprintf("%s=%s", "SSH_AUTH_SOCK", state.sockPath())) + } else { + log.Debugf("agent requested but no connection-scoped agent intent available") + } } // start shell session @@ -208,3 +371,23 @@ func (s *server) ListenAndServe() error { log.Debugf("Start ssh server on %s", s.sshServer.Addr) return s.sshServer.ListenAndServe() } + +// connCallback is invoked once per inbound SSH connection. Outside the +// explicit reuseSock (openvscode backhaul) mode it stores a lightweight +// intent on the ssh.Context and schedules a teardown goroutine. The agent +// listener itself is allocated lazily on the first session that requests +// agent forwarding, so failed-auth probes never touch the filesystem. +func (s *server) connCallback(ctx ssh.Context, conn net.Conn) net.Conn { + // Preserve the openvscode backhaul path: when a reuseSock is provided, + // the per-session setupAgentListener(reuseSock) path is the intended + // behavior. Skip setting up a per-connection listener here. + if s.reuseSock != "" { + return conn + } + + intent := &connAgentIntent{connID: newConnID(conn.RemoteAddr().String())} + ctx.SetValue(ctxKeyConnAgent, intent) + + log.Debugf("ssh conn open: connID=%s remote=%s", intent.connID, conn.RemoteAddr()) + return conn +}