From 3e38995d5b6e872ed4cc30ab7a775f3ce12adb7f Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 8 Apr 2026 12:32:25 -0500 Subject: [PATCH 01/14] fix: remove redundant SSH signature setup that races with tunnel (#645) The `setupGitSSHSignature` function in `cmd/up.go` opened an ephemeral SSH session to configure the git signing helper inside the container. This raced with the main tunnel's port forwarding, causing "use of closed network connection" errors that blocked the entire workspace startup. The credentials server tunnel (`credentials_server.go`) already calls `gitsshsigning.ConfigureHelper` with the base64-decoded signing key, making `setupGitSSHSignature` redundant. Removing it eliminates the race condition and the cascade of failures users reported: - Port forwarding errors on first start - "No such file" for the signing key (key never written due to setup failure) - JSON decode errors (non-JSON error responses from the signature endpoint) Also fixes the `/git-ssh-signature` HTTP handler to always return JSON error responses, even when the request body cannot be read. Closes #645 --- cmd/agent/git_ssh_signature_test.go | 22 +++++++++++++ cmd/up.go | 49 ----------------------------- pkg/credentials/server.go | 7 ++++- pkg/credentials/server_test.go | 33 +++++++++++++++++++ pkg/gitsshsigning/helper_test.go | 44 ++++++++++++++++++++++++++ 5 files changed, 105 insertions(+), 50 deletions(-) diff --git a/cmd/agent/git_ssh_signature_test.go b/cmd/agent/git_ssh_signature_test.go index 59a06bef02..5b44b1c0b9 100644 --- a/cmd/agent/git_ssh_signature_test.go +++ b/cmd/agent/git_ssh_signature_test.go @@ -55,3 +55,25 @@ func (s *GitSSHSignatureTestSuite) TestParseDefaultsToSign() { assert.Equal(s.T(), "sign", result.command) assert.Equal(s.T(), "/tmp/buffer", result.bufferFile) } + +func (s *GitSSHSignatureTestSuite) TestParseEmptyArgs() { + result := parseSSHKeygenArgs([]string{}) + assert.Equal(s.T(), "sign", result.command) + assert.Equal(s.T(), "", result.bufferFile) + assert.Equal(s.T(), "", result.certPath) + assert.Equal(s.T(), "", result.namespace) +} + +func (s *GitSSHSignatureTestSuite) TestParseMultipleUnknownFlags() { + // ssh-keygen may pass several unknown boolean flags; buffer file must still be found. + args := []string{"-Y", "sign", "-n", "git", "-f", "/key.pub", "-U", "-O", "/tmp/buf"} + result := parseSSHKeygenArgs(args) + assert.Equal(s.T(), "/key.pub", result.certPath) + assert.Equal(s.T(), "/tmp/buf", result.bufferFile) +} + +func (s *GitSSHSignatureTestSuite) TestParseBufferFileWithSpaces() { + args := []string{"-Y", "sign", "-n", "git", "-f", "/key.pub", "/tmp/my buffer file"} + result := parseSSHKeygenArgs(args) + assert.Equal(s.T(), "/tmp/my buffer file", result.bufferFile) +} diff --git a/cmd/up.go b/cmd/up.go index b30f248fc3..e24f4a82a0 100644 --- a/cmd/up.go +++ b/cmd/up.go @@ -15,7 +15,6 @@ import ( "strings" "syscall" - "al.essio.dev/pkg/shellescape" "github.com/blang/semver/v4" "github.com/sirupsen/logrus" "github.com/skevetter/devpod/cmd/flags" @@ -395,17 +394,6 @@ func (cmd *UpCmd) configureWorkspace( return err } - // Run after dotfiles so the signing config isn't overwritten by a - // dotfiles installer that replaces .gitconfig. - gitSSHSignatureEnabled := devPodConfig.ContextOption( - config.ContextOptionGitSSHSignatureForwarding, - ) == "true" - if cmd.GitSSHSigningKey != "" && gitSSHSignatureEnabled { - if err := setupGitSSHSignature(cmd.GitSSHSigningKey, client); err != nil { - return err - } - } - return nil } @@ -1553,43 +1541,6 @@ func collectDotfilesScriptEnvKeyvaluePairs(envFiles []string) ([]string, error) return keyValues, nil } -func setupGitSSHSignature( - signingKey string, - client client2.BaseWorkspaceClient, -) error { - execPath, err := os.Executable() - if err != nil { - return err - } - - remoteUser, err := devssh.GetUser( - client.WorkspaceConfig().ID, - client.WorkspaceConfig().SSHConfigPath, - client.WorkspaceConfig().SSHConfigIncludePath, - ) - if err != nil { - remoteUser = "root" - } - - // #nosec G204 -- execPath is from os.Executable(), not user input - out, err := exec.Command( - execPath, - "ssh", - "--user", - remoteUser, - "--context", - client.Context(), - client.Workspace(), - "--command", - shellescape.QuoteCommand( - []string{config.BinaryName, "agent", "git-ssh-signature-helper", signingKey}, - ), - ).CombinedOutput() - if err != nil { - return fmt.Errorf("setup git ssh signature helper: %w, output: %s", err, string(out)) - } - return nil -} func performGpgForwarding( client client2.BaseWorkspaceClient, diff --git a/pkg/credentials/server.go b/pkg/credentials/server.go index 2dfb17d8be..0d7a719fce 100644 --- a/pkg/credentials/server.go +++ b/pkg/credentials/server.go @@ -156,7 +156,12 @@ func handleGitSSHSignatureRequest( ) error { out, err := io.ReadAll(request.Body) if err != nil { - return fmt.Errorf("read request body: %w", err) + log.WithFields(logrus.Fields{"error": err}).Error("error reading git SSH signature request body") + writer.Header().Set("Content-Type", "application/json") + writer.WriteHeader(http.StatusInternalServerError) + errJSON, _ := json.Marshal(map[string]string{"error": fmt.Sprintf("read request body: %v", err)}) + _, _ = writer.Write(errJSON) + return nil } log.WithFields(logrus.Fields{"data": string(out)}).Debug("received git SSH signature post data") diff --git a/pkg/credentials/server_test.go b/pkg/credentials/server_test.go index 70beb8cdf9..345626ffa9 100644 --- a/pkg/credentials/server_test.go +++ b/pkg/credentials/server_test.go @@ -15,6 +15,11 @@ import ( "github.com/stretchr/testify/require" ) +// errReader is an io.Reader that always returns an error. +type errReader struct{ err error } + +func (e *errReader) Read([]byte) (int, error) { return 0, e.err } + func TestHandleGitSSHSignature_GRPCError_ReturnsJSON500(t *testing.T) { mock := &mockTunnelClient{ gitSSHSignatureFunc: func(ctx context.Context, msg *tunnel.Message) (*tunnel.Message, error) { @@ -42,6 +47,34 @@ func TestHandleGitSSHSignature_GRPCError_ReturnsJSON500(t *testing.T) { assert.Contains(t, body["error"], "Permission denied") } +func TestHandleGitSSHSignature_BodyReadError_ReturnsJSON500(t *testing.T) { + mock := &mockTunnelClient{ + gitSSHSignatureFunc: func(ctx context.Context, msg *tunnel.Message) (*tunnel.Message, error) { + t.Fatal("gRPC should not be called when body read fails") + return nil, nil + }, + } + + req := httptest.NewRequest( + http.MethodPost, + "/git-ssh-signature", + &errReader{err: fmt.Errorf("connection reset")}, + ) + w := httptest.NewRecorder() + + err := handleGitSSHSignatureRequest(context.Background(), w, req, mock, log.Discard) + require.NoError(t, err, "error should be written to response, not returned") + + resp := w.Result() + assert.Equal(t, http.StatusInternalServerError, resp.StatusCode) + assert.Equal(t, "application/json", resp.Header.Get("Content-Type")) + + var body map[string]string + err = json.NewDecoder(resp.Body).Decode(&body) + require.NoError(t, err, "response body must be valid JSON") + assert.Contains(t, body["error"], "connection reset") +} + func TestHandleGitSSHSignature_GRPCSuccess_ReturnsJSON200(t *testing.T) { expectedMessage := `{"signature":"abc123"}` mock := &mockTunnelClient{ diff --git a/pkg/gitsshsigning/helper_test.go b/pkg/gitsshsigning/helper_test.go index e76e8bb1e7..3cd0ecae54 100644 --- a/pkg/gitsshsigning/helper_test.go +++ b/pkg/gitsshsigning/helper_test.go @@ -1,10 +1,13 @@ package gitsshsigning import ( + "os" + "path/filepath" "strings" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" ) @@ -75,6 +78,47 @@ func (s *HelperTestSuite) TestRemoveSignatureHelper_PreservesUserOwnedGpgSSHKeys assert.Contains(s.T(), result, "[commit]") } +func TestUpdateGitConfig_Idempotent(t *testing.T) { + dir := t.TempDir() + gitConfigPath := filepath.Join(dir, ".gitconfig") + + // First call: writes signing config + err := updateGitConfig(gitConfigPath, "", "/path/to/key.pub") + require.NoError(t, err) + + content1, err := os.ReadFile(gitConfigPath) // #nosec G304 -- test path from t.TempDir + require.NoError(t, err) + assert.Contains(t, string(content1), "program = devpod-ssh-signature") + assert.Contains(t, string(content1), "signingkey = /path/to/key.pub") + + // Second call with same config: should be a no-op + err = updateGitConfig(gitConfigPath, "", "/path/to/key.pub") + require.NoError(t, err) + + content2, err := os.ReadFile(gitConfigPath) // #nosec G304 -- test path from t.TempDir + require.NoError(t, err) + assert.Equal(t, string(content1), string(content2), "second call should not duplicate config") +} + +func TestUpdateGitConfig_DifferentKey(t *testing.T) { + dir := t.TempDir() + gitConfigPath := filepath.Join(dir, ".gitconfig") + + // First call with key A + err := updateGitConfig(gitConfigPath, "", "/path/to/keyA.pub") + require.NoError(t, err) + + // Second call with key B: since the program line already exists, it won't rewrite + err = updateGitConfig(gitConfigPath, "", "/path/to/keyB.pub") + require.NoError(t, err) + + content, err := os.ReadFile(gitConfigPath) // #nosec G304 -- test path from t.TempDir + require.NoError(t, err) + // The idempotency check looks for program = devpod-ssh-signature, so it won't + // overwrite when the program is already configured (even with a different key) + assert.Contains(t, string(content), "program = devpod-ssh-signature") +} + func (s *HelperTestSuite) TestRemoveSignatureHelper_DropsEmptyGpgSSHSection() { input := strings.Join([]string{ "[user]", "\tname = Test User", From 809d1ce10def6d938f82f6d37c64fcccafdd0304 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 8 Apr 2026 12:35:50 -0500 Subject: [PATCH 02/14] test(e2e): add SSH signing test with forwardPorts to cover #645 race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The existing E2E tests used a minimal devcontainer without forwardPorts, so they never triggered the race condition between setupGitSSHSignature's ephemeral SSH session and the main tunnel's port forwarding. This test uses a devcontainer with forwardPorts: [8300, 7080] to verify that signing works on first `devpod up` — the exact scenario that failed for the issue reporter. --- e2e/tests/ssh/ssh.go | 119 ++++++++++++++++++ .../ssh-signing-with-ports/devcontainer.json | 5 + 2 files changed, 124 insertions(+) create mode 100644 e2e/tests/ssh/testdata/ssh-signing-with-ports/devcontainer.json diff --git a/e2e/tests/ssh/ssh.go b/e2e/tests/ssh/ssh.go index 6fd728cf73..5f4169294d 100644 --- a/e2e/tests/ssh/ssh.go +++ b/e2e/tests/ssh/ssh.go @@ -255,6 +255,125 @@ var _ = ginkgo.Describe("devpod ssh test suite", ginkgo.Label("ssh"), ginkgo.Ord }, ) + ginkgo.It( + "should sign commits on first start when devcontainer has forwardPorts", + ginkgo.SpecTimeout(7*time.Minute), + func(ctx ginkgo.SpecContext) { + if runtime.GOOS == osWindows { + ginkgo.Skip("skipping on windows") + } + + // This test reproduces the race condition from #645: when a devcontainer + // has forwardPorts configured, the old setupGitSSHSignature function + // would open an ephemeral SSH session whose port forwards conflicted + // with the main tunnel, causing the signing helper setup to fail. + tempDir, err := framework.CopyToTempDir("tests/ssh/testdata/ssh-signing-with-ports") + framework.ExpectNoError(err) + + f := framework.NewDefaultFramework(initialDir + "/bin") + _ = f.DevPodProviderAdd(ctx, "docker") + err = f.DevPodProviderUse(ctx, "docker") + framework.ExpectNoError(err) + + ginkgo.DeferCleanup(func(cleanupCtx context.Context) { + _ = f.DevPodWorkspaceDelete(cleanupCtx, tempDir) + framework.CleanupTempDir(initialDir, tempDir) + }) + + // Generate a temporary SSH key for signing + sshKeyDir, err := os.MkdirTemp("", "devpod-ssh-signing-ports-test") + framework.ExpectNoError(err) + defer func() { _ = os.RemoveAll(sshKeyDir) }() + + keyPath := filepath.Join(sshKeyDir, "id_ed25519") + // #nosec G204 -- test command with controlled arguments + err = exec.Command( + "ssh-keygen", "-t", "ed25519", "-f", keyPath, "-N", "", "-q", + ).Run() + framework.ExpectNoError(err) + + // First start with signing key and forwarded ports — this must succeed + // without the port forwarding race condition. + err = f.DevPodUp(ctx, tempDir, "--git-ssh-signing-key", keyPath+".pub") + framework.ExpectNoError(err) + + // Verify the helper script was installed + out, err := f.DevPodSSH(ctx, tempDir, + "test -x /usr/local/bin/devpod-ssh-signature && echo EXISTS", + ) + framework.ExpectNoError(err) + gomega.Expect(strings.TrimSpace(out)).To( + gomega.Equal("EXISTS"), + "devpod-ssh-signature helper should be installed on first start with forwardPorts", + ) + + // Verify git config was set + out, err = f.DevPodSSH(ctx, tempDir, "git config --global gpg.ssh.program") + framework.ExpectNoError(err) + gomega.Expect(strings.TrimSpace(out)).To(gomega.Equal("devpod-ssh-signature")) + + // Attempt a signed commit through the tunnel + commitCmd := strings.Join([]string{ + "cd /tmp", + "git init test-sign-ports-repo", + "cd test-sign-ports-repo", + "git config user.name 'Test User'", + "git config user.email 'test@example.com'", + "git config commit.gpgsign true", + "echo test > testfile", + "git add testfile", + "git commit -m 'signed commit with forwarded ports' 2>&1", + }, " && ") + + stdout, stderr, err := f.ExecCommandCapture(ctx, []string{ + "ssh", + "--agent-forwarding", + "--start-services", + tempDir, + "--command", commitCmd, + }) + ginkgo.GinkgoWriter.Printf("commit stdout: %s\n", stdout) + ginkgo.GinkgoWriter.Printf("commit stderr: %s\n", stderr) + framework.ExpectNoError(err) + + gomega.Expect(stdout).To( + gomega.ContainSubstring("signed commit with forwarded ports"), + "git commit should succeed on first start with forwardPorts configured", + ) + + // Verify the signature is valid + pubKeyBytes, err := os.ReadFile( + keyPath + ".pub", + ) // #nosec G304 -- test file with controlled path + framework.ExpectNoError(err) + pubKey := strings.TrimSpace(string(pubKeyBytes)) + + verifyCmd := strings.Join([]string{ + "cd /tmp/test-sign-ports-repo", + "echo 'test@example.com " + pubKey + "' > /tmp/allowed_signers", + "git config gpg.ssh.allowedSignersFile /tmp/allowed_signers", + "git verify-commit HEAD 2>&1", + }, " && ") + + stdout, stderr, err = f.ExecCommandCapture(ctx, []string{ + "ssh", + "--agent-forwarding", + "--start-services", + tempDir, + "--command", verifyCmd, + }) + ginkgo.GinkgoWriter.Printf("verify stdout: %s\n", stdout) + ginkgo.GinkgoWriter.Printf("verify stderr: %s\n", stderr) + framework.ExpectNoError(err) + + combined := stdout + stderr + gomega.Expect(combined).To( + gomega.ContainSubstring("Good"), + "signature should be valid on first start with forwardPorts", + ) + }, + ) + ginkgo.It( "should not install git SSH signature helper when signing key is not provided", ginkgo.SpecTimeout(5*time.Minute), diff --git a/e2e/tests/ssh/testdata/ssh-signing-with-ports/devcontainer.json b/e2e/tests/ssh/testdata/ssh-signing-with-ports/devcontainer.json new file mode 100644 index 0000000000..5fc08b3fc8 --- /dev/null +++ b/e2e/tests/ssh/testdata/ssh-signing-with-ports/devcontainer.json @@ -0,0 +1,5 @@ +{ + "name": "SSH Signing With Ports Test", + "image": "mcr.microsoft.com/devcontainers/go:1", + "forwardPorts": [8300, 7080] +} From 8edf9bfb7d23fe8be5686c55d859646cd1380b3d Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 8 Apr 2026 12:42:53 -0500 Subject: [PATCH 03/14] fix(tunnel): thread explicit signing key through to credentials server MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The credentials server only received the SSH signing key via ExtractGitConfiguration() which reads the host's .gitconfig. When a user passes --git-ssh-signing-key on the CLI but doesn't have user.signingkey in their host git config, the credentials server never received the key — making signing silently fail. Add GitSSHSigningKey field to RunServicesOptions and thread it from the UpCmd through the IDE browser launchers into startBrowserTunnel. The addGitSSHSigningKey function now prefers the explicit key over host .gitconfig extraction, falling back only when no explicit key is provided. This was the missing piece identified from the prior issue-645-analysis branch work that was never merged to main. Part of #645 --- cmd/up.go | 11 ++++++ pkg/tunnel/services.go | 29 ++++++++++------ pkg/tunnel/services_test.go | 67 +++++++++++++++++++++++++++++++++++++ 3 files changed, 97 insertions(+), 10 deletions(-) create mode 100644 pkg/tunnel/services_test.go diff --git a/cmd/up.go b/cmd/up.go index e24f4a82a0..3732dfd6bb 100644 --- a/cmd/up.go +++ b/cmd/up.go @@ -469,6 +469,7 @@ func (o *ideOpener) open( user, ideOptions, o.cmd.SSHAuthSockID, + o.cmd.GitSSHSigningKey, o.log, ) @@ -487,6 +488,7 @@ func (o *ideOpener) open( user, ideOptions, o.cmd.SSHAuthSockID, + o.cmd.GitSSHSigningKey, o.log, ) @@ -499,6 +501,7 @@ func (o *ideOpener) open( user, ideOptions, o.cmd.SSHAuthSockID, + o.cmd.GitSSHSigningKey, o.log, ) @@ -842,6 +845,7 @@ func startJupyterNotebookInBrowser( user string, ideOptions map[string]config.OptionValue, authSockID string, + gitSSHSigningKey string, logger log.Logger, ) error { if forwardGpg { @@ -888,6 +892,7 @@ func startJupyterNotebookInBrowser( false, extraPorts, authSockID, + gitSSHSigningKey, logger, ) } @@ -900,6 +905,7 @@ func startRStudioInBrowser( user string, ideOptions map[string]config.OptionValue, authSockID string, + gitSSHSigningKey string, logger log.Logger, ) error { if forwardGpg { @@ -945,6 +951,7 @@ func startRStudioInBrowser( false, extraPorts, authSockID, + gitSSHSigningKey, logger, ) } @@ -993,6 +1000,7 @@ func startVSCodeInBrowser( workspaceFolder, user string, ideOptions map[string]config.OptionValue, authSockID string, + gitSSHSigningKey string, logger log.Logger, ) error { if forwardGpg { @@ -1042,6 +1050,7 @@ func startVSCodeInBrowser( forwardPorts, extraPorts, authSockID, + gitSSHSigningKey, logger, ) } @@ -1138,6 +1147,7 @@ func startBrowserTunnel( forwardPorts bool, extraPorts []string, authSockID string, + gitSSHSigningKey string, logger log.Logger, ) error { // Setup a backhaul SSH connection using the remote user so there is an AUTH SOCK to use @@ -1233,6 +1243,7 @@ func startBrowserTunnel( ConfigureDockerCredentials: configureDockerCredentials, ConfigureGitCredentials: configureGitCredentials, ConfigureGitSSHSignatureHelper: configureGitSSHSignatureHelper, + GitSSHSigningKey: gitSSHSigningKey, Log: logger, }, ) diff --git a/pkg/tunnel/services.go b/pkg/tunnel/services.go index 5f33519f4e..0f9a21da35 100644 --- a/pkg/tunnel/services.go +++ b/pkg/tunnel/services.go @@ -51,6 +51,7 @@ type RunServicesOptions struct { ConfigureDockerCredentials bool ConfigureGitCredentials bool ConfigureGitSSHSignatureHelper bool + GitSSHSigningKey string Log log.Logger } @@ -104,16 +105,24 @@ func runTunnelServer(ctx context.Context, cancel context.CancelFunc, p tunnelSer } // addGitSSHSigningKey adds SSH signing key to command if configured. -func addGitSSHSigningKey(command string, log log.Logger) string { - format, userSigningKey, err := gitsshsigning.ExtractGitConfiguration() - if err != nil { - log.Debugf("failed to extract git configuration: %v", err) - return command - } - if format == gitsshsigning.GPGFormatSSH && userSigningKey != "" { - encodedKey := base64.StdEncoding.EncodeToString([]byte(userSigningKey)) - command += fmt.Sprintf(" --git-user-signing-key %s", encodedKey) +// When explicitKey is set (from --git-ssh-signing-key flag), it takes +// precedence over the host's .gitconfig. This ensures signing works +// even when user.signingkey is not in the host git configuration. +func addGitSSHSigningKey(command string, explicitKey string, log log.Logger) string { + userSigningKey := explicitKey + if userSigningKey == "" { + format, extracted, err := gitsshsigning.ExtractGitConfiguration() + if err != nil { + log.Debugf("failed to extract git configuration: %v", err) + return command + } + if format != gitsshsigning.GPGFormatSSH || extracted == "" { + return command + } + userSigningKey = extracted } + encodedKey := base64.StdEncoding.EncodeToString([]byte(userSigningKey)) + command += fmt.Sprintf(" --git-user-signing-key %s", encodedKey) return command } @@ -128,7 +137,7 @@ func buildCredentialsCommand(opts RunServicesOptions) string { command += " --configure-git-helper" } if opts.ConfigureGitSSHSignatureHelper { - command = addGitSSHSigningKey(command, opts.Log) + command = addGitSSHSigningKey(command, opts.GitSSHSigningKey, opts.Log) } if opts.ConfigureDockerCredentials { command += " --configure-docker-helper" diff --git a/pkg/tunnel/services_test.go b/pkg/tunnel/services_test.go new file mode 100644 index 0000000000..7a7d8c87f0 --- /dev/null +++ b/pkg/tunnel/services_test.go @@ -0,0 +1,67 @@ +package tunnel + +import ( + "encoding/base64" + "strings" + "testing" + + "github.com/skevetter/log" + "github.com/stretchr/testify/assert" +) + +func TestAddGitSSHSigningKey_ExplicitKey(t *testing.T) { + command := "devpod agent container credentials-server --user root" + result := addGitSSHSigningKey(command, "/path/to/key.pub", log.Discard) + + encoded := base64.StdEncoding.EncodeToString([]byte("/path/to/key.pub")) + assert.Contains(t, result, "--git-user-signing-key "+encoded) +} + +func TestAddGitSSHSigningKey_ExplicitKeyTakesPrecedence(t *testing.T) { + // When an explicit key is provided, it should be used regardless + // of what ExtractGitConfiguration might return from host .gitconfig. + command := "devpod agent container credentials-server --user root" + explicitKey := "/explicit/key.pub" + result := addGitSSHSigningKey(command, explicitKey, log.Discard) + + encoded := base64.StdEncoding.EncodeToString([]byte(explicitKey)) + assert.True(t, strings.HasSuffix(result, "--git-user-signing-key "+encoded)) +} + +func TestAddGitSSHSigningKey_EmptyExplicitKey_FallsBackToHostConfig(t *testing.T) { + // When explicit key is empty, the function attempts to read host .gitconfig. + // In a test environment without git SSH signing configured, it should + // return the command unchanged. + command := "devpod agent container credentials-server --user root" + result := addGitSSHSigningKey(command, "", log.Discard) + + // Without host SSH signing configured, command should be unchanged + // (or have the key appended if the test host has it configured). + // We just verify no panic and the base command is preserved. + assert.True(t, strings.HasPrefix(result, command)) +} + +func TestBuildCredentialsCommand_IncludesSigningKey(t *testing.T) { + opts := RunServicesOptions{ + User: "testuser", + ConfigureGitSSHSignatureHelper: true, + GitSSHSigningKey: "/my/key.pub", + Log: log.Discard, + } + command := buildCredentialsCommand(opts) + + encoded := base64.StdEncoding.EncodeToString([]byte("/my/key.pub")) + assert.Contains(t, command, "--git-user-signing-key "+encoded) + assert.Contains(t, command, "--user testuser") +} + +func TestBuildCredentialsCommand_NoSigningKey(t *testing.T) { + opts := RunServicesOptions{ + User: "testuser", + ConfigureGitSSHSignatureHelper: false, + Log: log.Discard, + } + command := buildCredentialsCommand(opts) + + assert.NotContains(t, command, "--git-user-signing-key") +} From 8d9cae3fc1f8d8537aa65129446fa51f70dc48bf Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 8 Apr 2026 12:44:53 -0500 Subject: [PATCH 04/14] style: apply golangci-lint-fmt formatting fixes --- cmd/up.go | 1 - pkg/credentials/server.go | 7 +++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/cmd/up.go b/cmd/up.go index 3732dfd6bb..53edc923a4 100644 --- a/cmd/up.go +++ b/cmd/up.go @@ -1552,7 +1552,6 @@ func collectDotfilesScriptEnvKeyvaluePairs(envFiles []string) ([]string, error) return keyValues, nil } - func performGpgForwarding( client client2.BaseWorkspaceClient, log log.Logger, diff --git a/pkg/credentials/server.go b/pkg/credentials/server.go index 0d7a719fce..ee7ac232a4 100644 --- a/pkg/credentials/server.go +++ b/pkg/credentials/server.go @@ -156,10 +156,13 @@ func handleGitSSHSignatureRequest( ) error { out, err := io.ReadAll(request.Body) if err != nil { - log.WithFields(logrus.Fields{"error": err}).Error("error reading git SSH signature request body") + log.WithFields(logrus.Fields{"error": err}). + Error("error reading git SSH signature request body") writer.Header().Set("Content-Type", "application/json") writer.WriteHeader(http.StatusInternalServerError) - errJSON, _ := json.Marshal(map[string]string{"error": fmt.Sprintf("read request body: %v", err)}) + errJSON, _ := json.Marshal( + map[string]string{"error": fmt.Sprintf("read request body: %v", err)}, + ) _, _ = writer.Write(errJSON) return nil } From af1eda9ef5b548e21bcd00b76ebdb327515f8a02 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 8 Apr 2026 12:55:21 -0500 Subject: [PATCH 05/14] fix: thread GitSSHSigningKey through ssh.go and harden signing setup - Thread GitSSHSigningKey through cmd/ssh.go startServices into RunServicesOptions so standalone SSH reconnections also forward the explicit signing key - Make ConfigureHelper failure non-fatal in credentials_server.go so signing setup failures don't crash the entire credentials server - Fix updateGitConfig idempotency: always remove+rewrite signing config instead of skipping when program line already exists - Update helper_test.go assertions for the new idempotent behavior Part of #645 --- cmd/agent/container/credentials_server.go | 27 ++++++++++++++--------- cmd/ssh.go | 7 ++++++ pkg/gitsshsigning/helper.go | 14 +++++++----- pkg/gitsshsigning/helper_test.go | 8 ++++--- 4 files changed, 36 insertions(+), 20 deletions(-) diff --git a/cmd/agent/container/credentials_server.go b/cmd/agent/container/credentials_server.go index c7aad53efb..a2123b9efc 100644 --- a/cmd/agent/container/credentials_server.go +++ b/cmd/agent/container/credentials_server.go @@ -135,21 +135,26 @@ func (cmd *CredentialsServerCmd) Run(ctx context.Context, port int) error { }(cmd.User) } - // configure git ssh signature helper + // configure git ssh signature helper — non-fatal so that a signing + // setup failure does not take down the entire credentials server + // (git/docker credential forwarding, port forwarding, etc.) if cmd.GitUserSigningKey != "" { decodedKey, err := base64.StdEncoding.DecodeString(cmd.GitUserSigningKey) if err != nil { - return fmt.Errorf("decode git ssh signature key: %w", err) - } - err = gitsshsigning.ConfigureHelper(cmd.User, string(decodedKey), log) - if err != nil { - return fmt.Errorf("configure git ssh signature helper: %w", err) + log.Errorf("Failed to decode git SSH signing key, signing will be unavailable: %v", err) + } else { + err = gitsshsigning.ConfigureHelper(cmd.User, string(decodedKey), log) + if err != nil { + log.Errorf( + "Failed to configure git SSH signature helper, signing will be unavailable: %v", + err, + ) + } else { + defer func(userName string) { + _ = gitsshsigning.RemoveHelper(userName) + }(cmd.User) + } } - - // cleanup when we are done - defer func(userName string) { - _ = gitsshsigning.RemoveHelper(userName) - }(cmd.User) } return credentials.RunCredentialsServer(ctx, port, tunnelClient, log) diff --git a/cmd/ssh.go b/cmd/ssh.go index e6371a84db..4534738717 100644 --- a/cmd/ssh.go +++ b/cmd/ssh.go @@ -52,6 +52,7 @@ type SSHCmd struct { AgentForwarding bool GPGAgentForwarding bool GitSSHSignatureForwarding bool + GitSSHSigningKey string // ssh keepalive options SSHKeepAliveInterval time.Duration `json:"sshKeepAliveInterval,omitempty"` @@ -147,6 +148,9 @@ func NewSSHCmd(f *flags.GlobalFlags) *cobra.Command { sshCmd.Flags(). DurationVar(&cmd.SSHKeepAliveInterval, "ssh-keepalive-interval", 55*time.Second, "How often should keepalive request be made (55s)") + sshCmd.Flags(). + StringVar(&cmd.GitSSHSigningKey, "git-ssh-signing-key", "", + "The SSH signing key to use for git commit signing inside the workspace") sshCmd.Flags().StringVar( &cmd.TermMode, "term-mode", @@ -517,6 +521,7 @@ func (cmd *SSHCmd) startTunnel( configureDockerCredentials, configureGitCredentials, configureGitSSHSignatureHelper, + cmd.GitSSHSigningKey, log, ) } @@ -652,6 +657,7 @@ func (cmd *SSHCmd) startServices( containerClient *ssh.Client, workspace *provider.Workspace, configureDockerCredentials, configureGitCredentials, configureGitSSHSignatureHelper bool, + gitSSHSigningKey string, log log.Logger, ) { if cmd.User != "" { @@ -668,6 +674,7 @@ func (cmd *SSHCmd) startServices( ConfigureDockerCredentials: configureDockerCredentials, ConfigureGitCredentials: configureGitCredentials, ConfigureGitSSHSignatureHelper: configureGitSSHSignatureHelper, + GitSSHSigningKey: gitSSHSigningKey, Log: log, }, ) diff --git a/pkg/gitsshsigning/helper.go b/pkg/gitsshsigning/helper.go index 2267cb4c23..9392ce9b01 100644 --- a/pkg/gitsshsigning/helper.go +++ b/pkg/gitsshsigning/helper.go @@ -110,12 +110,14 @@ func updateGitConfig(gitConfigPath, userName, gitSigningKey string) error { return err } - if !strings.Contains(configContent, "program = "+pkgconfig.SSHSignatureHelperName) { - newConfig := fmt.Sprintf(GitConfigTemplate, gitSigningKey) - newContent := removeSignatureHelper(configContent) + newConfig - if err := writeGitConfig(gitConfigPath, newContent, userName); err != nil { - return err - } + // Always remove any existing devpod-managed signing config and rewrite + // with the current key. The previous guard (checking whether the program + // line already existed) would silently skip key updates after unclean + // shutdowns or key rotations. + newConfig := fmt.Sprintf(GitConfigTemplate, gitSigningKey) + newContent := removeSignatureHelper(configContent) + newConfig + if err := writeGitConfig(gitConfigPath, newContent, userName); err != nil { + return err } return nil diff --git a/pkg/gitsshsigning/helper_test.go b/pkg/gitsshsigning/helper_test.go index 3cd0ecae54..2fe53b8370 100644 --- a/pkg/gitsshsigning/helper_test.go +++ b/pkg/gitsshsigning/helper_test.go @@ -108,15 +108,17 @@ func TestUpdateGitConfig_DifferentKey(t *testing.T) { err := updateGitConfig(gitConfigPath, "", "/path/to/keyA.pub") require.NoError(t, err) - // Second call with key B: since the program line already exists, it won't rewrite + // Second call with key B: should update to the new key err = updateGitConfig(gitConfigPath, "", "/path/to/keyB.pub") require.NoError(t, err) content, err := os.ReadFile(gitConfigPath) // #nosec G304 -- test path from t.TempDir require.NoError(t, err) - // The idempotency check looks for program = devpod-ssh-signature, so it won't - // overwrite when the program is already configured (even with a different key) assert.Contains(t, string(content), "program = devpod-ssh-signature") + assert.Contains(t, string(content), "signingkey = /path/to/keyB.pub", + "key should be updated to the new value") + assert.NotContains(t, string(content), "keyA", + "old key should be replaced") } func (s *HelperTestSuite) TestRemoveSignatureHelper_DropsEmptyGpgSSHSection() { From 31f303f2f10859294bbcaadc59e3a28f05c24c54 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 8 Apr 2026 13:08:31 -0500 Subject: [PATCH 06/14] fix: complete removeSignatureHelper cleanup and fix CI failures - Refactor removeSignatureHelper to buffer [gpg] and [user] sections the same way as [gpg "ssh"], dropping empty sections after filtering. Previously left behind empty [gpg] headers and stale signingkey lines, causing TestUpdateGitConfig_Idempotent and _DifferentKey to fail in CI. - Extract repeated test command string to constant (goconst lint fix) Part of #645 --- pkg/gitsshsigning/helper.go | 79 ++++++++++++++++++++++++------------- pkg/tunnel/services_test.go | 8 ++-- 2 files changed, 56 insertions(+), 31 deletions(-) diff --git a/pkg/gitsshsigning/helper.go b/pkg/gitsshsigning/helper.go index 9392ce9b01..71769d965f 100644 --- a/pkg/gitsshsigning/helper.go +++ b/pkg/gitsshsigning/helper.go @@ -153,40 +153,70 @@ func removeGitConfigHelper(gitConfigPath, userName string) error { } func removeSignatureHelper(content string) string { - inGpgSSHSection := false - inGpgSection := false - var gpgSSHBuffer []string + type sectionKind int + const ( + sectionNone sectionKind = iota + sectionGpgSSH + sectionGpg + sectionUser + ) + + current := sectionNone + var buf []string var out []string + flush := func() { + switch current { + case sectionGpgSSH: + out = append(out, filterSection(buf, func(trimmed string) bool { + return strings.HasPrefix(trimmed, "program = "+pkgconfig.SSHSignatureHelperName) + })...) + case sectionGpg: + out = append(out, filterSection(buf, func(trimmed string) bool { + return strings.HasPrefix(trimmed, "format = ssh") + })...) + case sectionUser: + out = append(out, filterSection(buf, func(trimmed string) bool { + return strings.HasPrefix(trimmed, "signingkey = ") + })...) + } + buf = nil + } + for line := range strings.Lines(content) { line = strings.TrimRight(line, "\n") trimmed := strings.TrimSpace(line) if isSectionHeader(trimmed) { - if inGpgSSHSection { - out = append(out, filterGpgSSHSection(gpgSSHBuffer)...) - gpgSSHBuffer = nil + if current != sectionNone { + flush() } - inGpgSSHSection = trimmed == `[gpg "ssh"]` - inGpgSection = trimmed == "[gpg]" - if inGpgSSHSection { - gpgSSHBuffer = append(gpgSSHBuffer, line) + switch trimmed { + case `[gpg "ssh"]`: + current = sectionGpgSSH + case "[gpg]": + current = sectionGpg + case "[user]": + current = sectionUser + default: + current = sectionNone + } + if current != sectionNone { + buf = append(buf, line) continue } } - if inGpgSSHSection { - gpgSSHBuffer = append(gpgSSHBuffer, line) + if current != sectionNone { + buf = append(buf, line) continue } - if !isDevpodManagedGpgKey(inGpgSection, trimmed) { - out = append(out, line) - } + out = append(out, line) } - if inGpgSSHSection { - out = append(out, filterGpgSSHSection(gpgSSHBuffer)...) + if current != sectionNone { + flush() } return strings.Join(out, "\n") @@ -196,23 +226,16 @@ func isSectionHeader(trimmed string) bool { return len(trimmed) > 0 && trimmed[0] == '[' } -func isDevpodManagedGpgKey(inGpgSection bool, trimmed string) bool { - if !inGpgSection || len(trimmed) == 0 || trimmed[0] == '[' { - return false - } - return strings.HasPrefix(trimmed, "format = ssh") -} - -// filterGpgSSHSection removes devpod-managed keys from a buffered [gpg "ssh"] -// section. Returns the header + remaining user keys, or nil if no user keys remain. -func filterGpgSSHSection(lines []string) []string { +// filterSection removes lines matching the predicate from a buffered section. +// Returns the header + remaining lines, or nil if no lines remain after filtering. +func filterSection(lines []string, isManaged func(string) bool) []string { if len(lines) == 0 { return nil } var kept []string for _, line := range lines[1:] { trimmed := strings.TrimSpace(line) - if !strings.HasPrefix(trimmed, "program = "+pkgconfig.SSHSignatureHelperName) { + if !isManaged(trimmed) { kept = append(kept, line) } } diff --git a/pkg/tunnel/services_test.go b/pkg/tunnel/services_test.go index 7a7d8c87f0..dac752d3fe 100644 --- a/pkg/tunnel/services_test.go +++ b/pkg/tunnel/services_test.go @@ -9,8 +9,10 @@ import ( "github.com/stretchr/testify/assert" ) +const testBaseCommand = "devpod agent container credentials-server --user root" + func TestAddGitSSHSigningKey_ExplicitKey(t *testing.T) { - command := "devpod agent container credentials-server --user root" + command := testBaseCommand result := addGitSSHSigningKey(command, "/path/to/key.pub", log.Discard) encoded := base64.StdEncoding.EncodeToString([]byte("/path/to/key.pub")) @@ -20,7 +22,7 @@ func TestAddGitSSHSigningKey_ExplicitKey(t *testing.T) { func TestAddGitSSHSigningKey_ExplicitKeyTakesPrecedence(t *testing.T) { // When an explicit key is provided, it should be used regardless // of what ExtractGitConfiguration might return from host .gitconfig. - command := "devpod agent container credentials-server --user root" + command := testBaseCommand explicitKey := "/explicit/key.pub" result := addGitSSHSigningKey(command, explicitKey, log.Discard) @@ -32,7 +34,7 @@ func TestAddGitSSHSigningKey_EmptyExplicitKey_FallsBackToHostConfig(t *testing.T // When explicit key is empty, the function attempts to read host .gitconfig. // In a test environment without git SSH signing configured, it should // return the command unchanged. - command := "devpod agent container credentials-server --user root" + command := testBaseCommand result := addGitSSHSigningKey(command, "", log.Discard) // Without host SSH signing configured, command should be unchanged From b4f0e940a01e5f2d484772a3fb7dca13f5f27055 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 8 Apr 2026 13:13:24 -0500 Subject: [PATCH 07/14] refactor(e2e): consolidate SSH signing tests, add forwardPorts to existing testdata Instead of a separate duplicate test for the #645 forwardPorts race condition, add forwardPorts to the existing ssh-signing devcontainer testdata so the main signing test exercises the harder case by default. Removes ~120 lines of duplicated test code and the redundant ssh-signing-with-ports testdata directory. Part of #645 --- e2e/tests/ssh/ssh.go | 119 ------------------ .../ssh-signing-with-ports/devcontainer.json | 5 - .../testdata/ssh-signing/devcontainer.json | 3 +- 3 files changed, 2 insertions(+), 125 deletions(-) delete mode 100644 e2e/tests/ssh/testdata/ssh-signing-with-ports/devcontainer.json diff --git a/e2e/tests/ssh/ssh.go b/e2e/tests/ssh/ssh.go index 5f4169294d..6fd728cf73 100644 --- a/e2e/tests/ssh/ssh.go +++ b/e2e/tests/ssh/ssh.go @@ -255,125 +255,6 @@ var _ = ginkgo.Describe("devpod ssh test suite", ginkgo.Label("ssh"), ginkgo.Ord }, ) - ginkgo.It( - "should sign commits on first start when devcontainer has forwardPorts", - ginkgo.SpecTimeout(7*time.Minute), - func(ctx ginkgo.SpecContext) { - if runtime.GOOS == osWindows { - ginkgo.Skip("skipping on windows") - } - - // This test reproduces the race condition from #645: when a devcontainer - // has forwardPorts configured, the old setupGitSSHSignature function - // would open an ephemeral SSH session whose port forwards conflicted - // with the main tunnel, causing the signing helper setup to fail. - tempDir, err := framework.CopyToTempDir("tests/ssh/testdata/ssh-signing-with-ports") - framework.ExpectNoError(err) - - f := framework.NewDefaultFramework(initialDir + "/bin") - _ = f.DevPodProviderAdd(ctx, "docker") - err = f.DevPodProviderUse(ctx, "docker") - framework.ExpectNoError(err) - - ginkgo.DeferCleanup(func(cleanupCtx context.Context) { - _ = f.DevPodWorkspaceDelete(cleanupCtx, tempDir) - framework.CleanupTempDir(initialDir, tempDir) - }) - - // Generate a temporary SSH key for signing - sshKeyDir, err := os.MkdirTemp("", "devpod-ssh-signing-ports-test") - framework.ExpectNoError(err) - defer func() { _ = os.RemoveAll(sshKeyDir) }() - - keyPath := filepath.Join(sshKeyDir, "id_ed25519") - // #nosec G204 -- test command with controlled arguments - err = exec.Command( - "ssh-keygen", "-t", "ed25519", "-f", keyPath, "-N", "", "-q", - ).Run() - framework.ExpectNoError(err) - - // First start with signing key and forwarded ports — this must succeed - // without the port forwarding race condition. - err = f.DevPodUp(ctx, tempDir, "--git-ssh-signing-key", keyPath+".pub") - framework.ExpectNoError(err) - - // Verify the helper script was installed - out, err := f.DevPodSSH(ctx, tempDir, - "test -x /usr/local/bin/devpod-ssh-signature && echo EXISTS", - ) - framework.ExpectNoError(err) - gomega.Expect(strings.TrimSpace(out)).To( - gomega.Equal("EXISTS"), - "devpod-ssh-signature helper should be installed on first start with forwardPorts", - ) - - // Verify git config was set - out, err = f.DevPodSSH(ctx, tempDir, "git config --global gpg.ssh.program") - framework.ExpectNoError(err) - gomega.Expect(strings.TrimSpace(out)).To(gomega.Equal("devpod-ssh-signature")) - - // Attempt a signed commit through the tunnel - commitCmd := strings.Join([]string{ - "cd /tmp", - "git init test-sign-ports-repo", - "cd test-sign-ports-repo", - "git config user.name 'Test User'", - "git config user.email 'test@example.com'", - "git config commit.gpgsign true", - "echo test > testfile", - "git add testfile", - "git commit -m 'signed commit with forwarded ports' 2>&1", - }, " && ") - - stdout, stderr, err := f.ExecCommandCapture(ctx, []string{ - "ssh", - "--agent-forwarding", - "--start-services", - tempDir, - "--command", commitCmd, - }) - ginkgo.GinkgoWriter.Printf("commit stdout: %s\n", stdout) - ginkgo.GinkgoWriter.Printf("commit stderr: %s\n", stderr) - framework.ExpectNoError(err) - - gomega.Expect(stdout).To( - gomega.ContainSubstring("signed commit with forwarded ports"), - "git commit should succeed on first start with forwardPorts configured", - ) - - // Verify the signature is valid - pubKeyBytes, err := os.ReadFile( - keyPath + ".pub", - ) // #nosec G304 -- test file with controlled path - framework.ExpectNoError(err) - pubKey := strings.TrimSpace(string(pubKeyBytes)) - - verifyCmd := strings.Join([]string{ - "cd /tmp/test-sign-ports-repo", - "echo 'test@example.com " + pubKey + "' > /tmp/allowed_signers", - "git config gpg.ssh.allowedSignersFile /tmp/allowed_signers", - "git verify-commit HEAD 2>&1", - }, " && ") - - stdout, stderr, err = f.ExecCommandCapture(ctx, []string{ - "ssh", - "--agent-forwarding", - "--start-services", - tempDir, - "--command", verifyCmd, - }) - ginkgo.GinkgoWriter.Printf("verify stdout: %s\n", stdout) - ginkgo.GinkgoWriter.Printf("verify stderr: %s\n", stderr) - framework.ExpectNoError(err) - - combined := stdout + stderr - gomega.Expect(combined).To( - gomega.ContainSubstring("Good"), - "signature should be valid on first start with forwardPorts", - ) - }, - ) - ginkgo.It( "should not install git SSH signature helper when signing key is not provided", ginkgo.SpecTimeout(5*time.Minute), diff --git a/e2e/tests/ssh/testdata/ssh-signing-with-ports/devcontainer.json b/e2e/tests/ssh/testdata/ssh-signing-with-ports/devcontainer.json deleted file mode 100644 index 5fc08b3fc8..0000000000 --- a/e2e/tests/ssh/testdata/ssh-signing-with-ports/devcontainer.json +++ /dev/null @@ -1,5 +0,0 @@ -{ - "name": "SSH Signing With Ports Test", - "image": "mcr.microsoft.com/devcontainers/go:1", - "forwardPorts": [8300, 7080] -} diff --git a/e2e/tests/ssh/testdata/ssh-signing/devcontainer.json b/e2e/tests/ssh/testdata/ssh-signing/devcontainer.json index 278d342d01..0224b074c2 100644 --- a/e2e/tests/ssh/testdata/ssh-signing/devcontainer.json +++ b/e2e/tests/ssh/testdata/ssh-signing/devcontainer.json @@ -1,4 +1,5 @@ { "name": "SSH Signing Test", - "image": "mcr.microsoft.com/devcontainers/go:1" + "image": "mcr.microsoft.com/devcontainers/go:1", + "forwardPorts": [8300, 7080] } From 867cad8ddb75ec020609262246f49b32f9a9d338 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 8 Apr 2026 13:30:35 -0500 Subject: [PATCH 08/14] fix(e2e): run signing verification inside --start-services SSH session The helper script and git config are only available when the credentials server is running. Move verification checks into the same SSH session that has --start-services active instead of using bare DevPodSSH calls. Part of #645 --- e2e/tests/ssh/ssh.go | 28 ++++++---------------------- 1 file changed, 6 insertions(+), 22 deletions(-) diff --git a/e2e/tests/ssh/ssh.go b/e2e/tests/ssh/ssh.go index 6fd728cf73..eab3813ae1 100644 --- a/e2e/tests/ssh/ssh.go +++ b/e2e/tests/ssh/ssh.go @@ -144,29 +144,13 @@ var _ = ginkgo.Describe("devpod ssh test suite", ginkgo.Label("ssh"), ginkgo.Ord err = f.DevPodUp(ctx, tempDir, "--git-ssh-signing-key", keyPath+".pub") framework.ExpectNoError(err) - // Step 1: Verify the helper script was installed and executable - out, err := f.DevPodSSH(ctx, tempDir, - "test -x /usr/local/bin/devpod-ssh-signature && echo EXISTS", - ) - framework.ExpectNoError(err) - gomega.Expect(strings.TrimSpace(out)).To( - gomega.Equal("EXISTS"), - "devpod-ssh-signature helper script should be installed and executable", - ) - - // Step 2: Verify git config was written correctly - out, err = f.DevPodSSH(ctx, tempDir, "git config --global gpg.ssh.program") - framework.ExpectNoError(err) - gomega.Expect(strings.TrimSpace(out)).To(gomega.Equal("devpod-ssh-signature")) - - out, err = f.DevPodSSH(ctx, tempDir, "git config --global gpg.format") - framework.ExpectNoError(err) - gomega.Expect(strings.TrimSpace(out)).To(gomega.Equal("ssh")) - - // Step 3: Attempt a signed commit with the credentials server - // tunnel active. The signing request is forwarded over the tunnel - // to the host where ssh-keygen performs the actual signing. + // Verify helper installation, git config, and a signed commit + // in a single SSH session with --start-services so the + // credentials server tunnel is active. commitCmd := strings.Join([]string{ + "test -x /usr/local/bin/devpod-ssh-signature", + "test \"$(git config --global gpg.ssh.program)\" = devpod-ssh-signature", + "test \"$(git config --global gpg.format)\" = ssh", "cd /tmp", "git init test-sign-repo", "cd test-sign-repo", From 75a4c907f82446c514e9c6badfa8f172c85901f8 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 8 Apr 2026 13:49:28 -0500 Subject: [PATCH 09/14] fix(e2e): wait for async credentials server to install signing helper The signing helper is installed by the credentials server which starts asynchronously via --start-services. Add a retry loop (up to 30s) before checking for the helper binary. Part of #645 --- e2e/tests/ssh/ssh.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/e2e/tests/ssh/ssh.go b/e2e/tests/ssh/ssh.go index eab3813ae1..1ece7a7c09 100644 --- a/e2e/tests/ssh/ssh.go +++ b/e2e/tests/ssh/ssh.go @@ -146,8 +146,10 @@ var _ = ginkgo.Describe("devpod ssh test suite", ginkgo.Label("ssh"), ginkgo.Ord // Verify helper installation, git config, and a signed commit // in a single SSH session with --start-services so the - // credentials server tunnel is active. + // credentials server tunnel is active. The helper is installed + // asynchronously by the credentials server, so retry briefly. commitCmd := strings.Join([]string{ + "for i in $(seq 1 30); do test -x /usr/local/bin/devpod-ssh-signature && break; sleep 1; done", "test -x /usr/local/bin/devpod-ssh-signature", "test \"$(git config --global gpg.ssh.program)\" = devpod-ssh-signature", "test \"$(git config --global gpg.format)\" = ssh", From 9bab8d229b0d576ccbd142d46a8401f29cefd664 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 8 Apr 2026 14:04:19 -0500 Subject: [PATCH 10/14] fix(e2e): pass --git-ssh-signing-key on SSH commands The signing helper is configured by the credentials server, which needs the key passed on each SSH invocation. Without it, the credentials server has no key to configure and the helper is never installed. Part of #645 --- e2e/tests/ssh/ssh.go | 38 ++++++++++++++++---------------------- 1 file changed, 16 insertions(+), 22 deletions(-) diff --git a/e2e/tests/ssh/ssh.go b/e2e/tests/ssh/ssh.go index 1ece7a7c09..af03d24216 100644 --- a/e2e/tests/ssh/ssh.go +++ b/e2e/tests/ssh/ssh.go @@ -164,13 +164,19 @@ var _ = ginkgo.Describe("devpod ssh test suite", ginkgo.Label("ssh"), ginkgo.Ord "git commit -m 'signed test commit' 2>&1", }, " && ") - stdout, stderr, err := f.ExecCommandCapture(ctx, []string{ + // The signing key must be passed on each SSH invocation so the + // credentials server can configure the helper inside the container. + sshBase := []string{ "ssh", "--agent-forwarding", "--start-services", + "--git-ssh-signing-key", keyPath + ".pub", tempDir, - "--command", commitCmd, - }) + } + + stdout, stderr, err := f.ExecCommandCapture(ctx, + append(sshBase, "--command", commitCmd), + ) ginkgo.GinkgoWriter.Printf("commit stdout: %s\n", stdout) ginkgo.GinkgoWriter.Printf("commit stderr: %s\n", stderr) framework.ExpectNoError(err) @@ -180,9 +186,7 @@ var _ = ginkgo.Describe("devpod ssh test suite", ginkgo.Label("ssh"), ginkgo.Ord "git commit should succeed with the signed test commit message", ) - // Step 4: Verify the commit is actually signed with a valid SSH signature. - // Read the public key that was used for signing so we can build - // an allowed-signers file inside the workspace for verification. + // Verify the commit is signed with a valid SSH signature. pubKeyBytes, err := os.ReadFile( keyPath + ".pub", ) // #nosec G304 -- test file with controlled path @@ -191,20 +195,14 @@ var _ = ginkgo.Describe("devpod ssh test suite", ginkgo.Label("ssh"), ginkgo.Ord verifyCmd := strings.Join([]string{ "cd /tmp/test-sign-repo", - // Create allowed signers file mapping the test email to our public key "echo 'test@example.com " + pubKey + "' > /tmp/allowed_signers", "git config gpg.ssh.allowedSignersFile /tmp/allowed_signers", - // Verify the commit signature is valid "git verify-commit HEAD 2>&1", }, " && ") - stdout, stderr, err = f.ExecCommandCapture(ctx, []string{ - "ssh", - "--agent-forwarding", - "--start-services", - tempDir, - "--command", verifyCmd, - }) + stdout, stderr, err = f.ExecCommandCapture(ctx, + append(sshBase, "--command", verifyCmd), + ) ginkgo.GinkgoWriter.Printf("verify stdout: %s\n", stdout) ginkgo.GinkgoWriter.Printf("verify stderr: %s\n", stderr) framework.ExpectNoError(err) @@ -218,13 +216,9 @@ var _ = ginkgo.Describe("devpod ssh test suite", ginkgo.Label("ssh"), ginkgo.Ord // And confirm the signature log shows the correct principal logCmd := "cd /tmp/test-sign-repo && git log --show-signature -1 2>&1" - stdout, stderr, err = f.ExecCommandCapture(ctx, []string{ - "ssh", - "--agent-forwarding", - "--start-services", - tempDir, - "--command", logCmd, - }) + stdout, stderr, err = f.ExecCommandCapture(ctx, + append(sshBase, "--command", logCmd), + ) ginkgo.GinkgoWriter.Printf("log stdout: %s\n", stdout) ginkgo.GinkgoWriter.Printf("log stderr: %s\n", stderr) framework.ExpectNoError(err) From eaafabf62653ef303f191ac80f597a6b0982f1b6 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 8 Apr 2026 16:29:03 -0500 Subject: [PATCH 11/14] fix: address CodeRabbit review feedback - Rename TestParseMultipleUnknownFlags to TestParseWithUFlag and remove -O flag (not a boolean flag, never passed by git). Test now reflects the actual git signing invocation: ssh-keygen -Y sign -n git -f -U - Replace HasSuffix/HasPrefix assertions with Contains for resilience - Remove unused strings import Part of #645 --- cmd/agent/git_ssh_signature_test.go | 7 ++++--- pkg/tunnel/services_test.go | 5 ++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/cmd/agent/git_ssh_signature_test.go b/cmd/agent/git_ssh_signature_test.go index 5b44b1c0b9..6504c07e6f 100644 --- a/cmd/agent/git_ssh_signature_test.go +++ b/cmd/agent/git_ssh_signature_test.go @@ -64,9 +64,10 @@ func (s *GitSSHSignatureTestSuite) TestParseEmptyArgs() { assert.Equal(s.T(), "", result.namespace) } -func (s *GitSSHSignatureTestSuite) TestParseMultipleUnknownFlags() { - // ssh-keygen may pass several unknown boolean flags; buffer file must still be found. - args := []string{"-Y", "sign", "-n", "git", "-f", "/key.pub", "-U", "-O", "/tmp/buf"} +func (s *GitSSHSignatureTestSuite) TestParseWithUFlag() { + // Git passes -U when using a literal SSH key value. The parser must + // still identify certPath and bufferFile with -U present. + args := []string{"-Y", "sign", "-n", "git", "-f", "/key.pub", "-U", "/tmp/buf"} result := parseSSHKeygenArgs(args) assert.Equal(s.T(), "/key.pub", result.certPath) assert.Equal(s.T(), "/tmp/buf", result.bufferFile) diff --git a/pkg/tunnel/services_test.go b/pkg/tunnel/services_test.go index dac752d3fe..3b996adeb5 100644 --- a/pkg/tunnel/services_test.go +++ b/pkg/tunnel/services_test.go @@ -2,7 +2,6 @@ package tunnel import ( "encoding/base64" - "strings" "testing" "github.com/skevetter/log" @@ -27,7 +26,7 @@ func TestAddGitSSHSigningKey_ExplicitKeyTakesPrecedence(t *testing.T) { result := addGitSSHSigningKey(command, explicitKey, log.Discard) encoded := base64.StdEncoding.EncodeToString([]byte(explicitKey)) - assert.True(t, strings.HasSuffix(result, "--git-user-signing-key "+encoded)) + assert.Contains(t, result, "--git-user-signing-key "+encoded) } func TestAddGitSSHSigningKey_EmptyExplicitKey_FallsBackToHostConfig(t *testing.T) { @@ -40,7 +39,7 @@ func TestAddGitSSHSigningKey_EmptyExplicitKey_FallsBackToHostConfig(t *testing.T // Without host SSH signing configured, command should be unchanged // (or have the key appended if the test host has it configured). // We just verify no panic and the base command is preserved. - assert.True(t, strings.HasPrefix(result, command)) + assert.Contains(t, result, command) } func TestBuildCredentialsCommand_IncludesSigningKey(t *testing.T) { From 3eeed1a44f3fbf670558e7cecf44792a631a6c92 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 8 Apr 2026 17:51:59 -0500 Subject: [PATCH 12/14] refactor: replace WithFields logging with standard Errorf Part of #645 --- pkg/credentials/server.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/credentials/server.go b/pkg/credentials/server.go index ee7ac232a4..2c932d0380 100644 --- a/pkg/credentials/server.go +++ b/pkg/credentials/server.go @@ -156,8 +156,7 @@ func handleGitSSHSignatureRequest( ) error { out, err := io.ReadAll(request.Body) if err != nil { - log.WithFields(logrus.Fields{"error": err}). - Error("error reading git SSH signature request body") + log.Errorf("error reading git SSH signature request body: %v", err) writer.Header().Set("Content-Type", "application/json") writer.WriteHeader(http.StatusInternalServerError) errJSON, _ := json.Marshal( From 98fa53789b02cdb0d914a8cc1445e490d8f92fba Mon Sep 17 00:00:00 2001 From: Samuel K Date: Fri, 10 Apr 2026 01:38:41 -0500 Subject: [PATCH 13/14] fix: preserve user-owned signingkey in gitconfig cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit removeSignatureHelper was stripping all signingkey lines from [user] sections, which would permanently delete a user's pre-existing signing key (e.g., GPG key) on workspace startup or teardown. Now only drops [user] sections that contain nothing but signingkey entries — these are the devpod-appended ones from GitConfigTemplate. User-owned sections with other entries (name, email) are preserved intact. --- pkg/gitsshsigning/helper.go | 29 ++++++++++++++++++++++++++--- pkg/gitsshsigning/helper_test.go | 19 +++++++++++++++++++ 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/pkg/gitsshsigning/helper.go b/pkg/gitsshsigning/helper.go index 71769d965f..f34088aa8d 100644 --- a/pkg/gitsshsigning/helper.go +++ b/pkg/gitsshsigning/helper.go @@ -176,9 +176,15 @@ func removeSignatureHelper(content string) string { return strings.HasPrefix(trimmed, "format = ssh") })...) case sectionUser: - out = append(out, filterSection(buf, func(trimmed string) bool { - return strings.HasPrefix(trimmed, "signingkey = ") - })...) + // Only strip [user] sections that contain nothing but signingkey + // entries — these are the ones appended by GitConfigTemplate. + // Sections with other entries (name, email, etc.) are user-owned + // and must be preserved intact to avoid data loss. + if isDevpodOnlyUserSection(buf) { + // Drop the entire section — it was appended by devpod. + } else { + out = append(out, buf...) + } } buf = nil } @@ -226,6 +232,23 @@ func isSectionHeader(trimmed string) bool { return len(trimmed) > 0 && trimmed[0] == '[' } +// isDevpodOnlyUserSection returns true when a buffered [user] section contains +// nothing but signingkey entries. Such sections are appended by GitConfigTemplate +// and are safe to remove. Sections with other entries (name, email, etc.) belong +// to the user and must be preserved. +func isDevpodOnlyUserSection(lines []string) bool { + for _, line := range lines[1:] { + trimmed := strings.TrimSpace(line) + if trimmed == "" { + continue + } + if !strings.HasPrefix(trimmed, "signingkey = ") { + return false + } + } + return true +} + // filterSection removes lines matching the predicate from a buffered section. // Returns the header + remaining lines, or nil if no lines remain after filtering. func filterSection(lines []string, isManaged func(string) bool) []string { diff --git a/pkg/gitsshsigning/helper_test.go b/pkg/gitsshsigning/helper_test.go index 2fe53b8370..565890dc63 100644 --- a/pkg/gitsshsigning/helper_test.go +++ b/pkg/gitsshsigning/helper_test.go @@ -78,6 +78,25 @@ func (s *HelperTestSuite) TestRemoveSignatureHelper_PreservesUserOwnedGpgSSHKeys assert.Contains(s.T(), result, "[commit]") } +func (s *HelperTestSuite) TestRemoveSignatureHelper_PreservesUserOwnedSigningKey() { + // A [user] section with name + signingkey is user-owned; only the + // devpod-appended [user] section (signingkey-only) should be dropped. + input := strings.Join([]string{ + "[user]", "\tname = Test User", "\tsigningkey = /my/gpg-key", + `[gpg "ssh"]`, "\tprogram = devpod-ssh-signature", + "[gpg]", "\tformat = ssh", + "[user]", "\tsigningkey = /devpod/injected-key", + }, "\n") + + result := removeSignatureHelper(input) + + assert.Contains(s.T(), result, "signingkey = /my/gpg-key", + "user-owned signingkey must be preserved") + assert.NotContains(s.T(), result, "/devpod/injected-key", + "devpod-only [user] section should be removed") + assert.Contains(s.T(), result, "Test User") +} + func TestUpdateGitConfig_Idempotent(t *testing.T) { dir := t.TempDir() gitConfigPath := filepath.Join(dir, ".gitconfig") From b59b0838c2a3f474b2af1dec7016e0a4256302b1 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Fri, 10 Apr 2026 17:05:28 -0500 Subject: [PATCH 14/14] fix: address CodeRabbit review feedback - Use %v instead of %w in log.Errorf (helper.go:55) - Tighten test assertions from Contains to Equal for exact command matching - Make fallback test deterministic by isolating HOME env --- pkg/gitsshsigning/helper.go | 2 +- pkg/tunnel/services_test.go | 18 +++++++++--------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/pkg/gitsshsigning/helper.go b/pkg/gitsshsigning/helper.go index f34088aa8d..58b102e229 100644 --- a/pkg/gitsshsigning/helper.go +++ b/pkg/gitsshsigning/helper.go @@ -52,7 +52,7 @@ func ConfigureHelper(userName, gitSigningKey string, log log.Logger) error { } log.Debugf("Got config path: %v", gitConfigPath) if err := updateGitConfig(gitConfigPath, userName, gitSigningKey); err != nil { - log.Errorf("Failed updating git configuration: %w", err) + log.Errorf("Failed updating git configuration: %v", err) return err } diff --git a/pkg/tunnel/services_test.go b/pkg/tunnel/services_test.go index 3b996adeb5..ee06a5af8c 100644 --- a/pkg/tunnel/services_test.go +++ b/pkg/tunnel/services_test.go @@ -15,7 +15,7 @@ func TestAddGitSSHSigningKey_ExplicitKey(t *testing.T) { result := addGitSSHSigningKey(command, "/path/to/key.pub", log.Discard) encoded := base64.StdEncoding.EncodeToString([]byte("/path/to/key.pub")) - assert.Contains(t, result, "--git-user-signing-key "+encoded) + assert.Equal(t, command+" --git-user-signing-key "+encoded, result) } func TestAddGitSSHSigningKey_ExplicitKeyTakesPrecedence(t *testing.T) { @@ -26,20 +26,20 @@ func TestAddGitSSHSigningKey_ExplicitKeyTakesPrecedence(t *testing.T) { result := addGitSSHSigningKey(command, explicitKey, log.Discard) encoded := base64.StdEncoding.EncodeToString([]byte(explicitKey)) - assert.Contains(t, result, "--git-user-signing-key "+encoded) + assert.Equal(t, command+" --git-user-signing-key "+encoded, result) } func TestAddGitSSHSigningKey_EmptyExplicitKey_FallsBackToHostConfig(t *testing.T) { - // When explicit key is empty, the function attempts to read host .gitconfig. - // In a test environment without git SSH signing configured, it should - // return the command unchanged. + // Ensure deterministic environment with no host git signing config. command := testBaseCommand + tmpHome := t.TempDir() + t.Setenv("HOME", tmpHome) + t.Setenv("XDG_CONFIG_HOME", tmpHome) + result := addGitSSHSigningKey(command, "", log.Discard) - // Without host SSH signing configured, command should be unchanged - // (or have the key appended if the test host has it configured). - // We just verify no panic and the base command is preserved. - assert.Contains(t, result, command) + assert.Equal(t, command, result) + assert.NotContains(t, result, "--git-user-signing-key") } func TestBuildCredentialsCommand_IncludesSigningKey(t *testing.T) {