diff --git a/e2e/tests/ssh/ssh.go b/e2e/tests/ssh/ssh.go index af03d2421..1356d2f09 100644 --- a/e2e/tests/ssh/ssh.go +++ b/e2e/tests/ssh/ssh.go @@ -140,6 +140,36 @@ var _ = ginkgo.Describe("devpod ssh test suite", ginkgo.Label("ssh"), ginkgo.Ord ).Run() framework.ExpectNoError(err) + agentOut, err := exec.Command("ssh-agent", "-s").Output() + framework.ExpectNoError(err) + t := ginkgo.GinkgoT() + var agentPID string + for line := range strings.SplitSeq(string(agentOut), "\n") { + for _, prefix := range []string{"SSH_AUTH_SOCK=", "SSH_AGENT_PID="} { + if _, after, ok := strings.Cut(line, prefix); ok { + val := after + if semi := strings.Index(val, ";"); semi >= 0 { + val = val[:semi] + } + key := prefix[:len(prefix)-1] + if key == "SSH_AGENT_PID" { + agentPID = val + } + t.Setenv(key, val) + } + } + } + ginkgo.DeferCleanup(func(_ context.Context) { + if agentPID != "" { + // #nosec G204 -- controlled pid from ssh-agent we started + _ = exec.Command("kill", agentPID).Run() + } + }) + + // #nosec G204 -- test command with controlled arguments + err = exec.Command("ssh-add", keyPath).Run() + framework.ExpectNoError(err) + // Start workspace with git-ssh-signing-key flag err = f.DevPodUp(ctx, tempDir, "--git-ssh-signing-key", keyPath+".pub") framework.ExpectNoError(err) @@ -162,6 +192,11 @@ var _ = ginkgo.Describe("devpod ssh test suite", ginkgo.Label("ssh"), ginkgo.Ord "echo test > testfile", "git add testfile", "git commit -m 'signed test commit' 2>&1", + "ssh-add -L | head -1 > /tmp/test_signing_key.pub", + "git config --global user.signingkey /tmp/test_signing_key.pub", + "echo world >> file.txt", + "git add file.txt", + "git commit -m 'signed commit with file path key' 2>&1", }, " && ") // The signing key must be passed on each SSH invocation so the diff --git a/pkg/agent/tunnelserver/tunnelserver_gitssh_test.go b/pkg/agent/tunnelserver/tunnelserver_gitssh_test.go new file mode 100644 index 000000000..53cbbe0b5 --- /dev/null +++ b/pkg/agent/tunnelserver/tunnelserver_gitssh_test.go @@ -0,0 +1,33 @@ +package tunnelserver + +import ( + "context" + "encoding/json" + "testing" + + "github.com/skevetter/devpod/pkg/agent/tunnel" + "github.com/skevetter/devpod/pkg/gitsshsigning" + "github.com/skevetter/log" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestGitSSHSignature_DeserializesPublicKey(t *testing.T) { + req := gitsshsigning.GitSSHSignatureRequest{ + Content: "tree abc\nauthor Test\n\ncommit", + KeyPath: "/container/tmp/.git_signing_key_tmpXXX", + PublicKey: "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIFake test@test.com", + } + reqJSON, err := json.Marshal(req) + require.NoError(t, err) + + ts := New(log.Discard) + + _, err = ts.GitSSHSignature(context.Background(), &tunnel.Message{ + Message: string(reqJSON), + }) + require.Error(t, err) + assert.NotContains(t, err.Error(), "git ssh sign request") + assert.NotContains(t, err.Error(), "resolve signing key") + assert.Contains(t, err.Error(), "failed to sign commit") +} diff --git a/pkg/credentials/integration_test.go b/pkg/credentials/integration_test.go index 0c37a05cf..141b45778 100644 --- a/pkg/credentials/integration_test.go +++ b/pkg/credentials/integration_test.go @@ -92,3 +92,49 @@ func TestIntegration_SigningSuccess_WritesSigFile(t *testing.T) { require.NoError(t, readErr) assert.Equal(t, expectedSig, sigContent) } + +func TestIntegration_SignatureRequest_IncludesPublicKeyContent(t *testing.T) { + var receivedMessage string + mock := &mockTunnelClient{ + gitSSHSignatureFunc: func(ctx context.Context, msg *tunnel.Message) (*tunnel.Message, error) { + receivedMessage = msg.Message + sig := gitsshsigning.GitSSHSignatureResponse{ + Signature: []byte( + "-----BEGIN SSH SIGNATURE-----\ntest\n-----END SSH SIGNATURE-----\n", + ), + } + jsonBytes, err := json.Marshal(sig) + if err != nil { + return nil, fmt.Errorf("marshal sig: %w", err) + } + return &tunnel.Message{Message: string(jsonBytes)}, nil + }, + } + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + err := handleGitSSHSignatureRequest(context.Background(), w, r, mock, log.Discard) + if err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + } + })) + defer server.Close() + + gitsshsigning.SetSignatureServerURL(server.URL + "/git-ssh-signature") + t.Cleanup(func() { gitsshsigning.SetSignatureServerURL("") }) + + tmpDir := t.TempDir() + certFile := filepath.Join(tmpDir, "test_key.pub") + pubKeyContent := "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAITest test@example.com" + require.NoError(t, os.WriteFile(certFile, []byte(pubKeyContent), 0o600)) + + bufferFile := filepath.Join(tmpDir, "buffer") + require.NoError(t, os.WriteFile(bufferFile, []byte("commit content"), 0o600)) + + err := gitsshsigning.HandleGitSSHProgramCall(certFile, "git", bufferFile, log.Discard) + require.NoError(t, err) + + var req gitsshsigning.GitSSHSignatureRequest + require.NoError(t, json.Unmarshal([]byte(receivedMessage), &req)) + assert.Equal(t, pubKeyContent, req.PublicKey) + assert.Equal(t, "commit content", req.Content) +} diff --git a/pkg/devcontainer/sshtunnel/sshtunnel.go b/pkg/devcontainer/sshtunnel/sshtunnel.go index 5d3a2bdfc..dc148c29b 100644 --- a/pkg/devcontainer/sshtunnel/sshtunnel.go +++ b/pkg/devcontainer/sshtunnel/sshtunnel.go @@ -349,7 +349,15 @@ func establishSSHSession( } // setupSSHAgentForwarding configures SSH agent forwarding on the session. -// Errors are returned to the caller rather than sent to a channel directly. +// +// Failures are logged but never fatal. This matches OpenSSH's behavior: +// - clientloop.c: client_request_agent() returns NULL on failure, +// sending SSH2_MSG_CHANNEL_OPEN_FAILURE without terminating the session. +// - ssh_config(5) ExitOnForwardFailure only covers "dynamic, tunnel, +// local, and remote port forwardings" — agent forwarding is excluded. +// +// Stale SSH_AUTH_SOCK is common in practice (tmux, screen, reconnected +// terminals), so a fatal error here would break devpod up for many users. func setupSSHAgentForwarding( ts *sshSessionTunnel, sshClient *ssh.Client, @@ -368,9 +376,9 @@ func setupSSHAgentForwarding( } if err != nil { - ts.opts.Log.Warnf("SSH agent forwarding failed: %v", err) + ts.opts.Log.Warnf("SSH agent forwarding failed (continuing without agent): %v", err) } - return err + return nil } // runCommandInSSHTunnel runs the agent command over the SSH tunnel and returns diff --git a/pkg/gitsshsigning/client.go b/pkg/gitsshsigning/client.go index 98b1e73a6..7bd813389 100644 --- a/pkg/gitsshsigning/client.go +++ b/pkg/gitsshsigning/client.go @@ -80,9 +80,18 @@ func writeSignatureToFile(signature []byte, bufferFile string, log log.Logger) e } func createSignatureRequestBody(content []byte, certPath string) ([]byte, error) { + var publicKey string + pubKeyData, readErr := os.ReadFile( + certPath, + ) // #nosec G304 -- certPath comes from git config, not user input + if readErr == nil { + publicKey = strings.TrimSpace(string(pubKeyData)) + } + request := &GitSSHSignatureRequest{ - Content: string(content), - KeyPath: certPath, + Content: string(content), + KeyPath: certPath, + PublicKey: publicKey, } return json.Marshal(request) } diff --git a/pkg/gitsshsigning/client_test.go b/pkg/gitsshsigning/client_test.go index c12404338..55cdbbe84 100644 --- a/pkg/gitsshsigning/client_test.go +++ b/pkg/gitsshsigning/client_test.go @@ -4,6 +4,8 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "os" + "path/filepath" "testing" "github.com/skevetter/log" @@ -96,3 +98,27 @@ func TestRequestContentSignature_InvalidJSON(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "not json at all") } + +func TestCreateSignatureRequestBody_IncludesPublicKeyContent(t *testing.T) { + certPath := filepath.Join(t.TempDir(), "test_key.pub") + pubKeyContent := "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAITest test@example.com" + require.NoError(t, os.WriteFile(certPath, []byte(pubKeyContent), 0o600)) + + body, err := createSignatureRequestBody([]byte("commit content"), certPath) + require.NoError(t, err) + + var req GitSSHSignatureRequest + require.NoError(t, json.Unmarshal(body, &req)) + assert.Equal(t, "commit content", req.Content) + assert.Equal(t, certPath, req.KeyPath) + assert.Equal(t, pubKeyContent, req.PublicKey) +} + +func TestCreateSignatureRequestBody_MissingCertFile_OmitsPublicKey(t *testing.T) { + body, err := createSignatureRequestBody([]byte("commit content"), "/nonexistent/key.pub") + require.NoError(t, err) + + var req GitSSHSignatureRequest + require.NoError(t, json.Unmarshal(body, &req)) + assert.Empty(t, req.PublicKey) +} diff --git a/pkg/gitsshsigning/server.go b/pkg/gitsshsigning/server.go index 9c4fc6982..8d2ea01d5 100644 --- a/pkg/gitsshsigning/server.go +++ b/pkg/gitsshsigning/server.go @@ -3,12 +3,14 @@ package gitsshsigning import ( "bytes" "fmt" + "os" "os/exec" ) type GitSSHSignatureRequest struct { - Content string - KeyPath string + Content string + KeyPath string + PublicKey string // Public key content; when set, written to a temp file for ssh-keygen } type GitSSHSignatureResponse struct { @@ -18,22 +20,38 @@ type GitSSHSignatureResponse struct { // Sign signs the content using the private key and returns the signature. // This is intended to be a drop-in replacement for gpg.ssh.program for git, // so we simply execute ssh-keygen in the same way as git would do locally. +// +// When PublicKey is set, it is written to a temporary file that ssh-keygen +// can read. This is necessary because the original KeyPath comes from +// inside the container and does not exist on the host where Sign() runs. func (req *GitSSHSignatureRequest) Sign() (*GitSSHSignatureResponse, error) { - // Create a buffer to store the commit content + keyFile, cleanup, err := req.resolveKeyFile() + if err != nil { + return nil, fmt.Errorf("resolve signing key: %w", err) + } + defer cleanup() + var commitBuffer bytes.Buffer commitBuffer.WriteString(req.Content) - // Create the command to run ssh-keygen - cmd := exec.Command("ssh-keygen", "-Y", "sign", "-f", req.KeyPath, "-n", "git") + //nolint:gosec // keyFile is a controlled temp path or validated KeyPath + cmd := exec.Command( + "ssh-keygen", + "-Y", + "sign", + "-f", + keyFile, + "-n", + "git", + ) cmd.Stdin = &commitBuffer - // Capture the output of the command var out bytes.Buffer var stderr bytes.Buffer cmd.Stdout = &out cmd.Stderr = &stderr - err := cmd.Run() + err = cmd.Run() if err != nil { return nil, fmt.Errorf("failed to sign commit: %w, stderr: %s", err, stderr.String()) } @@ -42,3 +60,35 @@ func (req *GitSSHSignatureRequest) Sign() (*GitSSHSignatureResponse, error) { Signature: out.Bytes(), }, nil } + +// resolveKeyFile returns the path to use for ssh-keygen -f and a cleanup function. +// When PublicKey content is available, it writes a temp file. Otherwise falls back to KeyPath. +func (req *GitSSHSignatureRequest) resolveKeyFile() (string, func(), error) { + noop := func() {} + + if req.PublicKey == "" { + return req.KeyPath, noop, nil + } + + // ssh-keygen -Y sign -f reads the public key directly from + // to identify which SSH agent key to use for signing. We write the public + // key content to a temp file and pass that path to -f. + tmpFile, err := os.CreateTemp("", ".git_signing_key_*") + if err != nil { + return "", noop, fmt.Errorf("create temp key file: %w", err) + } + keyPath := tmpFile.Name() + + if _, err := tmpFile.WriteString(req.PublicKey); err != nil { + _ = tmpFile.Close() + _ = os.Remove(keyPath) + return "", noop, fmt.Errorf("write public key: %w", err) + } + _ = tmpFile.Close() + + cleanup := func() { + _ = os.Remove(keyPath) + } + + return keyPath, cleanup, nil +} diff --git a/pkg/gitsshsigning/server_test.go b/pkg/gitsshsigning/server_test.go new file mode 100644 index 000000000..39993db32 --- /dev/null +++ b/pkg/gitsshsigning/server_test.go @@ -0,0 +1,31 @@ +package gitsshsigning + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSign_WithPublicKeyContent_WritesToTempFile(t *testing.T) { + req := &GitSSHSignatureRequest{ + Content: "tree abc123\nauthor Test \n\ntest commit", + KeyPath: "/tmp/.git_signing_key_does_not_exist", + PublicKey: "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIFakeKeyForTesting test@example.com", + } + + _, err := req.Sign() + require.Error(t, err) + assert.NotContains(t, err.Error(), "/tmp/.git_signing_key_does_not_exist") +} + +func TestSign_NonExistentKeyPath_ReturnsError(t *testing.T) { + req := &GitSSHSignatureRequest{ + Content: "tree abc123\nauthor Test \n\ntest commit", + KeyPath: "/tmp/.git_signing_key_does_not_exist", + } + + _, err := req.Sign() + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to sign commit") +}