From ac4856aa7cf583c18bb9489167e9ad93d7c65784 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sat, 28 Mar 2026 10:20:15 -0500 Subject: [PATCH 1/6] fix: git SSH signature forwarding with modern git versions - Accept unknown ssh-keygen flags (-U, etc.) via FParseErrWhitelist so cobra doesn't reject flags from git >= 2.34 - Capture and return errors from setupGitSSHSignature instead of swallowing them with a generic log message - Fix removeSignatureHelper to only strip devpod-managed keys from [gpg] config sections, preserving user's own gpg settings - Remove unused log parameter from setupGitSSHSignature Fixes #645 --- cmd/agent/git_ssh_signature.go | 5 +++ cmd/agent/git_ssh_signature_test.go | 37 ++++++++++++++++ cmd/up.go | 10 ++--- pkg/gitsshsigning/helper.go | 34 +++++++++----- pkg/gitsshsigning/helper_test.go | 69 +++++++++++++++++++++++++++++ 5 files changed, 140 insertions(+), 15 deletions(-) create mode 100644 cmd/agent/git_ssh_signature_test.go create mode 100644 pkg/gitsshsigning/helper_test.go diff --git a/cmd/agent/git_ssh_signature.go b/cmd/agent/git_ssh_signature.go index cbed6c1e4..79ae65360 100644 --- a/cmd/agent/git_ssh_signature.go +++ b/cmd/agent/git_ssh_signature.go @@ -34,6 +34,11 @@ func NewGitSSHSignatureCmd(flags *flags.GlobalFlags) *cobra.Command { gitSshSignatureCmd := &cobra.Command{ Use: "git-ssh-signature", + // Allow unknown flags so that git can pass any ssh-keygen flags + // (e.g. -U for stdin input) without cobra rejecting them. + FParseErrWhitelist: cobra.FParseErrWhitelist{ + UnknownFlags: true, + }, RunE: func(_ *cobra.Command, args []string) error { logger := log.GetInstance() diff --git a/cmd/agent/git_ssh_signature_test.go b/cmd/agent/git_ssh_signature_test.go new file mode 100644 index 000000000..ee1ef29ac --- /dev/null +++ b/cmd/agent/git_ssh_signature_test.go @@ -0,0 +1,37 @@ +package agent + +import ( + "testing" + + "github.com/skevetter/devpod/cmd/flags" +) + +func TestGitSSHSignatureCmd_AcceptsUnknownFlags(t *testing.T) { + cmd := NewGitSSHSignatureCmd(&flags.GlobalFlags{}) + + // Simulate what git passes: -Y sign -n git -f /path/to/key -U /tmp/buffer + // We expect flag parsing to succeed (no "unknown shorthand flag" error). + err := cmd.ParseFlags( + []string{"-Y", "sign", "-n", "git", "-f", "/path/to/key", "-U", "/tmp/buffer"}, + ) + if err != nil { + t.Fatalf("expected flag parsing to succeed with unknown flag -U, got: %v", err) + } +} + +func TestGitSSHSignatureCmd_KnownFlagsParsed(t *testing.T) { + cmd := NewGitSSHSignatureCmd(&flags.GlobalFlags{}) + + err := cmd.ParseFlags([]string{"-Y", "sign", "-n", "git", "-f", "/path/to/key", "/tmp/buffer"}) + if err != nil { + t.Fatalf("expected flag parsing to succeed, got: %v", err) + } + + val, err := cmd.Flags().GetString("command") + if err != nil { + t.Fatalf("expected to get 'command' flag, got: %v", err) + } + if val != "sign" { + t.Fatalf("expected command flag to be 'sign', got: %q", val) + } +} diff --git a/cmd/up.go b/cmd/up.go index 43dd465ed..7486e1a5f 100644 --- a/cmd/up.go +++ b/cmd/up.go @@ -383,7 +383,7 @@ func (cmd *UpCmd) configureWorkspace( } if cmd.GitSSHSigningKey != "" { - if err := setupGitSSHSignature(cmd.GitSSHSigningKey, client, log); err != nil { + if err := setupGitSSHSignature(cmd.GitSSHSigningKey, client); err != nil { return err } } @@ -1539,7 +1539,6 @@ func collectDotfilesScriptEnvKeyvaluePairs(envFiles []string) ([]string, error) func setupGitSSHSignature( signingKey string, client client2.BaseWorkspaceClient, - log log.Logger, ) error { execPath, err := os.Executable() if err != nil { @@ -1555,7 +1554,8 @@ func setupGitSSHSignature( remoteUser = "root" } - err = exec.Command( + // #nosec G204 -- execPath is from os.Executable(), not user input + out, err := exec.Command( execPath, "ssh", "--agent-forwarding=true", @@ -1566,9 +1566,9 @@ func setupGitSSHSignature( client.Context(), client.Workspace(), "--command", fmt.Sprintf("devpod agent git-ssh-signature-helper %s", signingKey), - ).Run() + ).CombinedOutput() if err != nil { - log.Error("failure in setting up git ssh signature helper") + return fmt.Errorf("setup git ssh signature helper: %w, output: %s", err, string(out)) } return nil } diff --git a/pkg/gitsshsigning/helper.go b/pkg/gitsshsigning/helper.go index 2abee4e29..2c51b8a39 100644 --- a/pkg/gitsshsigning/helper.go +++ b/pkg/gitsshsigning/helper.go @@ -151,24 +151,38 @@ func removeGitConfigHelper(gitConfigPath, userName string) error { func removeSignatureHelper(content string) string { scan := scanner.NewScanner(strings.NewReader(content)) - isGpgSetup := false + inGpgSSHSection := false + inGpgSection := false out := []string{} for scan.Scan() { line := scan.Text() - if strings.TrimSpace(line) == "[gpg \"ssh\"]" { - isGpgSetup = true + trimmed := strings.TrimSpace(line) + + // Track section transitions + if len(trimmed) > 0 && trimmed[0] == '[' { + inGpgSSHSection = trimmed == `[gpg "ssh"]` + inGpgSection = trimmed == "[gpg]" + + // Skip the entire [gpg "ssh"] section header (devpod-managed) + if inGpgSSHSection { + continue + } + } + + // Skip all lines inside [gpg "ssh"] section + if inGpgSSHSection { continue - } else if strings.TrimSpace(line) == "[gpg]" { - isGpgSetup = true - } else if isGpgSetup { - trimmed := strings.TrimSpace(line) - if len(trimmed) > 0 && trimmed[0] == '[' { - isGpgSetup = false - } else { + } + + // Inside [gpg] section, only skip devpod-managed keys + if inGpgSection && len(trimmed) > 0 && trimmed[0] != '[' { + if strings.HasPrefix(trimmed, "format = ssh") || + strings.HasPrefix(trimmed, "program = devpod-ssh-signature") { continue } } + out = append(out, line) } diff --git a/pkg/gitsshsigning/helper_test.go b/pkg/gitsshsigning/helper_test.go new file mode 100644 index 000000000..53635f5d8 --- /dev/null +++ b/pkg/gitsshsigning/helper_test.go @@ -0,0 +1,69 @@ +package gitsshsigning + +import ( + "strings" + "testing" +) + +func TestRemoveSignatureHelper_PreservesUnrelatedGpgConfig(t *testing.T) { + input := strings.Join([]string{ + "[user]", "\tname = Test User", "\temail = test@example.com", + "[gpg \"ssh\"]", "\tprogram = devpod-ssh-signature", + "[gpg]", "\tformat = ssh", "\tprogram = /usr/bin/gpg2", + "[commit]", "\tgpgsign = true", + "[user]", "\tsigningkey = /path/to/key", + }, "\n") + + result := removeSignatureHelper(input) + + if strings.Contains(result, "devpod-ssh-signature") { + t.Errorf("expected devpod-ssh-signature to be removed, got:\n%s", result) + } + if !strings.Contains(result, "[user]") { + t.Errorf("expected [user] section to be preserved, got:\n%s", result) + } + if !strings.Contains(result, "[commit]") { + t.Errorf("expected [commit] section to be preserved, got:\n%s", result) + } + if !strings.Contains(result, "program = /usr/bin/gpg2") { + t.Errorf("expected unrelated gpg program to be preserved, got:\n%s", result) + } + if strings.Contains(result, "format = ssh") { + t.Errorf("expected 'format = ssh' to be removed, got:\n%s", result) + } +} + +func TestRemoveSignatureHelper_RemovesDevpodSections(t *testing.T) { + input := strings.Join([]string{ + "[user]", "\tname = Test User", + "[gpg \"ssh\"]", "\tprogram = devpod-ssh-signature", + "[gpg]", "\tformat = ssh", + "[user]", "\tsigningkey = /path/to/key", + }, "\n") + + result := removeSignatureHelper(input) + + if strings.Contains(result, "devpod-ssh-signature") { + t.Errorf("expected devpod-ssh-signature to be removed, got:\n%s", result) + } + if strings.Contains(result, "format = ssh") { + t.Errorf("expected 'format = ssh' under [gpg] to be removed, got:\n%s", result) + } + if !strings.Contains(result, "Test User") { + t.Errorf("expected user name to be preserved, got:\n%s", result) + } +} + +func TestRemoveSignatureHelper_NoGpgSections(t *testing.T) { + input := "[user]\n\tname = Test User\n\temail = test@example.com" + + result := removeSignatureHelper(input) + + if result != input { + t.Errorf( + "expected input to be unchanged when no gpg sections exist.\nExpected:\n%s\nGot:\n%s", + input, + result, + ) + } +} From 294d23eee1f1cf4b4ab59ec0444289101911dd00 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sat, 28 Mar 2026 10:38:27 -0500 Subject: [PATCH 2/6] test: add e2e test for git SSH signature helper setup Verifies that --git-ssh-signing-key flag correctly installs the devpod-ssh-signature helper script and configures git's gpg.ssh.program and gpg.format inside the workspace. Refs #645 --- e2e/tests/ssh/ssh.go | 76 +++++++++++++++++++ .../testdata/ssh-signing/devcontainer.json | 4 + 2 files changed, 80 insertions(+) create mode 100644 e2e/tests/ssh/testdata/ssh-signing/devcontainer.json diff --git a/e2e/tests/ssh/ssh.go b/e2e/tests/ssh/ssh.go index 0c15976f8..c39995fda 100644 --- a/e2e/tests/ssh/ssh.go +++ b/e2e/tests/ssh/ssh.go @@ -9,6 +9,7 @@ import ( "path/filepath" "runtime" "strconv" + "strings" "time" "github.com/onsi/ginkgo/v2" @@ -104,6 +105,81 @@ var _ = ginkgo.Describe("devpod ssh test suite", ginkgo.Label("ssh"), ginkgo.Ord // framework.ExpectNoError(err) // }) + ginkgo.It( + "should set up git SSH signature helper in workspace", + func(ctx context.Context) { + if runtime.GOOS == "windows" { + ginkgo.Skip("skipping on windows") + } + + tempDir, err := framework.CopyToTempDir("tests/ssh/testdata/ssh-signing") + 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-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) + + // Start workspace with git-ssh-signing-key flag + devpodUpCtx, cancel := context.WithTimeout(ctx, 5*time.Minute) + defer cancel() + err = f.DevPodUp(devpodUpCtx, tempDir, + "--git-ssh-signing-key", keyPath+".pub", + ) + framework.ExpectNoError(err) + + sshCtx, cancelSSH := context.WithTimeout(ctx, 20*time.Second) + defer cancelSSH() + + // Verify the helper script was installed + out, err := f.DevPodSSH(sshCtx, 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", + ) + + // Verify git config has the SSH signing program set + out, err = f.DevPodSSH(sshCtx, tempDir, + "git config --global gpg.ssh.program", + ) + framework.ExpectNoError(err) + gomega.Expect(strings.TrimSpace(out)).To( + gomega.Equal("devpod-ssh-signature"), + "git gpg.ssh.program should be set to devpod-ssh-signature", + ) + + // Verify git config has gpg format set to ssh + out, err = f.DevPodSSH(sshCtx, tempDir, + "git config --global gpg.format", + ) + framework.ExpectNoError(err) + gomega.Expect(strings.TrimSpace(out)).To( + gomega.Equal("ssh"), + "git gpg.format should be set to ssh", + ) + }, + ) + ginkgo.It( "should start a new workspace with a docker provider (default) and forward a port into it", func(ctx context.Context) { diff --git a/e2e/tests/ssh/testdata/ssh-signing/devcontainer.json b/e2e/tests/ssh/testdata/ssh-signing/devcontainer.json new file mode 100644 index 000000000..278d342d0 --- /dev/null +++ b/e2e/tests/ssh/testdata/ssh-signing/devcontainer.json @@ -0,0 +1,4 @@ +{ + "name": "SSH Signing Test", + "image": "mcr.microsoft.com/devcontainers/go:1" +} From f3f1c0b7be27fb1e0b97de0335826e3e59f06e39 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sat, 28 Mar 2026 11:01:27 -0500 Subject: [PATCH 3/6] refactor: rename command --- cmd/agent/git_ssh_signature.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/cmd/agent/git_ssh_signature.go b/cmd/agent/git_ssh_signature.go index 79ae65360..3eb34ca36 100644 --- a/cmd/agent/git_ssh_signature.go +++ b/cmd/agent/git_ssh_signature.go @@ -32,7 +32,7 @@ func NewGitSSHSignatureCmd(flags *flags.GlobalFlags) *cobra.Command { GlobalFlags: flags, } - gitSshSignatureCmd := &cobra.Command{ + gitSSHSignatureCmd := &cobra.Command{ Use: "git-ssh-signature", // Allow unknown flags so that git can pass any ssh-keygen flags // (e.g. -U for stdin input) without cobra rejecting them. @@ -59,10 +59,10 @@ func NewGitSSHSignatureCmd(flags *flags.GlobalFlags) *cobra.Command { }, } - gitSshSignatureCmd.Flags().StringVarP(&cmd.CertPath, "file", "f", "", "Path to the private key") - gitSshSignatureCmd.Flags().StringVarP(&cmd.Namespace, "namespace", "n", "", "Namespace") - gitSshSignatureCmd.Flags(). + gitSSHSignatureCmd.Flags().StringVarP(&cmd.CertPath, "file", "f", "", "Path to the private key") + gitSSHSignatureCmd.Flags().StringVarP(&cmd.Namespace, "namespace", "n", "", "Namespace") + gitSSHSignatureCmd.Flags(). StringVarP(&cmd.Command, "command", "Y", "sign", "Command - should be 'sign'") - return gitSshSignatureCmd + return gitSSHSignatureCmd } From 2df87b4d288a9c380cde87ce9aec001f4f9c825b Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sat, 28 Mar 2026 12:38:47 -0500 Subject: [PATCH 4/6] refactor: use testify suite --- pkg/gitsshsigning/helper_test.go | 61 +++++++++++++------------------- 1 file changed, 25 insertions(+), 36 deletions(-) diff --git a/pkg/gitsshsigning/helper_test.go b/pkg/gitsshsigning/helper_test.go index 53635f5d8..1ee54d0e3 100644 --- a/pkg/gitsshsigning/helper_test.go +++ b/pkg/gitsshsigning/helper_test.go @@ -3,12 +3,23 @@ package gitsshsigning import ( "strings" "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/suite" ) -func TestRemoveSignatureHelper_PreservesUnrelatedGpgConfig(t *testing.T) { +type HelperTestSuite struct { + suite.Suite +} + +func TestHelperSuite(t *testing.T) { + suite.Run(t, new(HelperTestSuite)) +} + +func (s *HelperTestSuite) TestRemoveSignatureHelper_PreservesUnrelatedGpgConfig() { input := strings.Join([]string{ "[user]", "\tname = Test User", "\temail = test@example.com", - "[gpg \"ssh\"]", "\tprogram = devpod-ssh-signature", + `[gpg "ssh"]`, "\tprogram = devpod-ssh-signature", "[gpg]", "\tformat = ssh", "\tprogram = /usr/bin/gpg2", "[commit]", "\tgpgsign = true", "[user]", "\tsigningkey = /path/to/key", @@ -16,54 +27,32 @@ func TestRemoveSignatureHelper_PreservesUnrelatedGpgConfig(t *testing.T) { result := removeSignatureHelper(input) - if strings.Contains(result, "devpod-ssh-signature") { - t.Errorf("expected devpod-ssh-signature to be removed, got:\n%s", result) - } - if !strings.Contains(result, "[user]") { - t.Errorf("expected [user] section to be preserved, got:\n%s", result) - } - if !strings.Contains(result, "[commit]") { - t.Errorf("expected [commit] section to be preserved, got:\n%s", result) - } - if !strings.Contains(result, "program = /usr/bin/gpg2") { - t.Errorf("expected unrelated gpg program to be preserved, got:\n%s", result) - } - if strings.Contains(result, "format = ssh") { - t.Errorf("expected 'format = ssh' to be removed, got:\n%s", result) - } + assert.NotContains(s.T(), result, "devpod-ssh-signature") + assert.Contains(s.T(), result, "[user]") + assert.Contains(s.T(), result, "[commit]") + assert.Contains(s.T(), result, "program = /usr/bin/gpg2") + assert.NotContains(s.T(), result, "format = ssh") } -func TestRemoveSignatureHelper_RemovesDevpodSections(t *testing.T) { +func (s *HelperTestSuite) TestRemoveSignatureHelper_RemovesDevpodSections() { input := strings.Join([]string{ "[user]", "\tname = Test User", - "[gpg \"ssh\"]", "\tprogram = devpod-ssh-signature", + `[gpg "ssh"]`, "\tprogram = devpod-ssh-signature", "[gpg]", "\tformat = ssh", "[user]", "\tsigningkey = /path/to/key", }, "\n") result := removeSignatureHelper(input) - if strings.Contains(result, "devpod-ssh-signature") { - t.Errorf("expected devpod-ssh-signature to be removed, got:\n%s", result) - } - if strings.Contains(result, "format = ssh") { - t.Errorf("expected 'format = ssh' under [gpg] to be removed, got:\n%s", result) - } - if !strings.Contains(result, "Test User") { - t.Errorf("expected user name to be preserved, got:\n%s", result) - } + assert.NotContains(s.T(), result, "devpod-ssh-signature") + assert.NotContains(s.T(), result, "format = ssh") + assert.Contains(s.T(), result, "Test User") } -func TestRemoveSignatureHelper_NoGpgSections(t *testing.T) { +func (s *HelperTestSuite) TestRemoveSignatureHelper_NoGpgSections() { input := "[user]\n\tname = Test User\n\temail = test@example.com" result := removeSignatureHelper(input) - if result != input { - t.Errorf( - "expected input to be unchanged when no gpg sections exist.\nExpected:\n%s\nGot:\n%s", - input, - result, - ) - } + assert.Equal(s.T(), input, result) } From 6ee2cd2fc9f5da15567ae6d976bef9ba893c425e Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sat, 28 Mar 2026 18:56:17 -0500 Subject: [PATCH 5/6] fix: address PR review feedback for git SSH signature - removeSignatureHelper now preserves user-owned keys in [gpg "ssh"] sections (e.g., allowedSignersFile). Only strips devpod-managed program entry. Drops the section header only if no user keys remain. - Shell-escape signingKey in setupGitSSHSignature to handle keys with spaces or special characters. - Convert git_ssh_signature_test.go to testify suite, add positional arg assertion and dedicated buffer file test. - Add two new helper tests: PreservesUserOwnedGpgSSHKeys and DropsEmptyGpgSSHSection. --- cmd/agent/git_ssh_signature_test.go | 63 ++++++++++++++++++++-------- cmd/up.go | 6 ++- pkg/gitsshsigning/helper.go | 64 +++++++++++++++++++++-------- pkg/gitsshsigning/helper_test.go | 33 +++++++++++++++ 4 files changed, 131 insertions(+), 35 deletions(-) diff --git a/cmd/agent/git_ssh_signature_test.go b/cmd/agent/git_ssh_signature_test.go index ee1ef29ac..10cad64c2 100644 --- a/cmd/agent/git_ssh_signature_test.go +++ b/cmd/agent/git_ssh_signature_test.go @@ -4,34 +4,63 @@ import ( "testing" "github.com/skevetter/devpod/cmd/flags" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/suite" ) -func TestGitSSHSignatureCmd_AcceptsUnknownFlags(t *testing.T) { +type GitSSHSignatureTestSuite struct { + suite.Suite +} + +func TestGitSSHSignatureSuite(t *testing.T) { + suite.Run(t, new(GitSSHSignatureTestSuite)) +} + +func (s *GitSSHSignatureTestSuite) TestAcceptsUnknownFlags() { cmd := NewGitSSHSignatureCmd(&flags.GlobalFlags{}) - // Simulate what git passes: -Y sign -n git -f /path/to/key -U /tmp/buffer - // We expect flag parsing to succeed (no "unknown shorthand flag" error). + // Git may pass: -Y sign -n git -f /path/to/key -U /tmp/buffer + // With FParseErrWhitelist, -U is treated as an unknown flag consuming + // /tmp/buffer as its value. This is fine because git always puts the + // buffer file as the last argument. We test with the buffer as a + // separate positional arg (no unknown flag consuming it). err := cmd.ParseFlags( []string{"-Y", "sign", "-n", "git", "-f", "/path/to/key", "-U", "/tmp/buffer"}, ) - if err != nil { - t.Fatalf("expected flag parsing to succeed with unknown flag -U, got: %v", err) - } + assert.NoError(s.T(), err, "flag parsing should succeed with unknown flag -U") +} + +func (s *GitSSHSignatureTestSuite) TestBufferFileAsPositionalArg() { + cmd := NewGitSSHSignatureCmd(&flags.GlobalFlags{}) + + // Standard git invocation: -Y sign -n git -f /path/to/key /tmp/buffer + // The buffer file is the last positional argument. + err := cmd.ParseFlags( + []string{"-Y", "sign", "-n", "git", "-f", "/path/to/key", "/tmp/buffer"}, + ) + assert.NoError(s.T(), err) + + args := cmd.Flags().Args() + assert.NotEmpty(s.T(), args, "should have positional args") + assert.Equal(s.T(), "/tmp/buffer", args[len(args)-1], + "last positional arg should be the buffer file") } -func TestGitSSHSignatureCmd_KnownFlagsParsed(t *testing.T) { +func (s *GitSSHSignatureTestSuite) TestKnownFlagsParsed() { cmd := NewGitSSHSignatureCmd(&flags.GlobalFlags{}) - err := cmd.ParseFlags([]string{"-Y", "sign", "-n", "git", "-f", "/path/to/key", "/tmp/buffer"}) - if err != nil { - t.Fatalf("expected flag parsing to succeed, got: %v", err) - } + err := cmd.ParseFlags( + []string{"-Y", "sign", "-n", "git", "-f", "/path/to/key", "/tmp/buffer"}, + ) + assert.NoError(s.T(), err, "flag parsing should succeed") val, err := cmd.Flags().GetString("command") - if err != nil { - t.Fatalf("expected to get 'command' flag, got: %v", err) - } - if val != "sign" { - t.Fatalf("expected command flag to be 'sign', got: %q", val) - } + assert.NoError(s.T(), err) + assert.Equal(s.T(), "sign", val, "command flag should be 'sign'") + + // The buffer file should be the last positional argument + args := cmd.Flags().Args() + assert.NotEmpty(s.T(), args, "should have positional args") + assert.Equal(s.T(), "/tmp/buffer", args[len(args)-1], + "last positional arg should be the buffer file") } diff --git a/cmd/up.go b/cmd/up.go index 7486e1a5f..131642f10 100644 --- a/cmd/up.go +++ b/cmd/up.go @@ -15,6 +15,7 @@ import ( "strings" "syscall" + "al.essio.dev/pkg/shellescape" "github.com/blang/semver/v4" "github.com/sirupsen/logrus" "github.com/skevetter/devpod/cmd/flags" @@ -1565,7 +1566,10 @@ func setupGitSSHSignature( "--context", client.Context(), client.Workspace(), - "--command", fmt.Sprintf("devpod agent git-ssh-signature-helper %s", signingKey), + "--command", + shellescape.QuoteCommand( + []string{"devpod", "agent", "git-ssh-signature-helper", signingKey}, + ), ).CombinedOutput() if err != nil { return fmt.Errorf("setup git ssh signature helper: %w, output: %s", err, string(out)) diff --git a/pkg/gitsshsigning/helper.go b/pkg/gitsshsigning/helper.go index 2c51b8a39..6e0c334a1 100644 --- a/pkg/gitsshsigning/helper.go +++ b/pkg/gitsshsigning/helper.go @@ -10,7 +10,6 @@ import ( "github.com/skevetter/devpod/pkg/command" "github.com/skevetter/devpod/pkg/file" "github.com/skevetter/log" - "github.com/skevetter/log/scanner" ) const ( @@ -150,41 +149,72 @@ func removeGitConfigHelper(gitConfigPath, userName string) error { } func removeSignatureHelper(content string) string { - scan := scanner.NewScanner(strings.NewReader(content)) inGpgSSHSection := false inGpgSection := false - out := []string{} + var gpgSSHBuffer []string + var out []string - for scan.Scan() { - line := scan.Text() + for line := range strings.Lines(content) { + line = strings.TrimRight(line, "\n") trimmed := strings.TrimSpace(line) - // Track section transitions - if len(trimmed) > 0 && trimmed[0] == '[' { + if isSectionHeader(trimmed) { + if inGpgSSHSection { + out = append(out, filterGpgSSHSection(gpgSSHBuffer)...) + gpgSSHBuffer = nil + } inGpgSSHSection = trimmed == `[gpg "ssh"]` inGpgSection = trimmed == "[gpg]" - - // Skip the entire [gpg "ssh"] section header (devpod-managed) if inGpgSSHSection { + gpgSSHBuffer = append(gpgSSHBuffer, line) continue } } - // Skip all lines inside [gpg "ssh"] section if inGpgSSHSection { + gpgSSHBuffer = append(gpgSSHBuffer, line) continue } - // Inside [gpg] section, only skip devpod-managed keys - if inGpgSection && len(trimmed) > 0 && trimmed[0] != '[' { - if strings.HasPrefix(trimmed, "format = ssh") || - strings.HasPrefix(trimmed, "program = devpod-ssh-signature") { - continue - } + if !isDevpodManagedGpgKey(inGpgSection, trimmed) { + out = append(out, line) } + } - out = append(out, line) + if inGpgSSHSection { + out = append(out, filterGpgSSHSection(gpgSSHBuffer)...) } return strings.Join(out, "\n") } + +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") || + strings.HasPrefix(trimmed, "program = devpod-ssh-signature") +} + +// 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 { + if len(lines) == 0 { + return nil + } + var kept []string + for _, line := range lines[1:] { + trimmed := strings.TrimSpace(line) + if !strings.HasPrefix(trimmed, "program = devpod-ssh-signature") { + kept = append(kept, line) + } + } + if len(kept) == 0 { + return nil + } + return append([]string{lines[0]}, kept...) +} diff --git a/pkg/gitsshsigning/helper_test.go b/pkg/gitsshsigning/helper_test.go index 1ee54d0e3..e76e8bb1e 100644 --- a/pkg/gitsshsigning/helper_test.go +++ b/pkg/gitsshsigning/helper_test.go @@ -56,3 +56,36 @@ func (s *HelperTestSuite) TestRemoveSignatureHelper_NoGpgSections() { assert.Equal(s.T(), input, result) } + +func (s *HelperTestSuite) TestRemoveSignatureHelper_PreservesUserOwnedGpgSSHKeys() { + input := strings.Join([]string{ + "[user]", "\tname = Test User", + `[gpg "ssh"]`, "\tprogram = devpod-ssh-signature", + "\tallowedSignersFile = ~/.ssh/allowed_signers", + "[commit]", "\tgpgsign = true", + }, "\n") + + result := removeSignatureHelper(input) + + assert.NotContains(s.T(), result, "devpod-ssh-signature") + assert.Contains(s.T(), result, `[gpg "ssh"]`, + "section header should be preserved when user keys remain") + assert.Contains(s.T(), result, "allowedSignersFile", + "user-owned key should be preserved") + assert.Contains(s.T(), result, "[commit]") +} + +func (s *HelperTestSuite) TestRemoveSignatureHelper_DropsEmptyGpgSSHSection() { + input := strings.Join([]string{ + "[user]", "\tname = Test User", + `[gpg "ssh"]`, "\tprogram = devpod-ssh-signature", + "[commit]", "\tgpgsign = true", + }, "\n") + + result := removeSignatureHelper(input) + + assert.NotContains(s.T(), result, "devpod-ssh-signature") + assert.NotContains(s.T(), result, `[gpg "ssh"]`, + "empty section should be dropped entirely") + assert.Contains(s.T(), result, "[commit]") +} From edffef8d661fc4bac6cc5bdc3901171f9393cb56 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sat, 28 Mar 2026 21:57:36 -0500 Subject: [PATCH 6/6] fix: address review feedback --- cmd/agent/git_ssh_signature_test.go | 34 +++++++++++++++++++---------- cmd/up.go | 22 ++++++++++++------- pkg/gitsshsigning/helper.go | 3 +-- 3 files changed, 37 insertions(+), 22 deletions(-) diff --git a/cmd/agent/git_ssh_signature_test.go b/cmd/agent/git_ssh_signature_test.go index 10cad64c2..39e982b8d 100644 --- a/cmd/agent/git_ssh_signature_test.go +++ b/cmd/agent/git_ssh_signature_test.go @@ -19,29 +19,40 @@ func TestGitSSHSignatureSuite(t *testing.T) { func (s *GitSSHSignatureTestSuite) TestAcceptsUnknownFlags() { cmd := NewGitSSHSignatureCmd(&flags.GlobalFlags{}) - // Git may pass: -Y sign -n git -f /path/to/key -U /tmp/buffer - // With FParseErrWhitelist, -U is treated as an unknown flag consuming - // /tmp/buffer as its value. This is fine because git always puts the - // buffer file as the last argument. We test with the buffer as a - // separate positional arg (no unknown flag consuming it). + // Git passes: -Y sign -n git -f /path/to/key -U /dev/stdin /tmp/buffer + // -U is an unknown flag that consumes /dev/stdin as its value. + // /tmp/buffer remains as a positional argument. err := cmd.ParseFlags( - []string{"-Y", "sign", "-n", "git", "-f", "/path/to/key", "-U", "/tmp/buffer"}, + []string{ + "-Y", + "sign", + "-n", + "git", + "-f", + "/path/to/key", + "-U", + "/dev/stdin", + "/tmp/buffer", + }, ) assert.NoError(s.T(), err, "flag parsing should succeed with unknown flag -U") + + args := cmd.Flags().Args() + s.Require().NotEmpty(args, "should have positional args") + assert.Equal(s.T(), "/tmp/buffer", args[len(args)-1], + "buffer file should be preserved as last positional arg") } func (s *GitSSHSignatureTestSuite) TestBufferFileAsPositionalArg() { cmd := NewGitSSHSignatureCmd(&flags.GlobalFlags{}) - // Standard git invocation: -Y sign -n git -f /path/to/key /tmp/buffer - // The buffer file is the last positional argument. err := cmd.ParseFlags( []string{"-Y", "sign", "-n", "git", "-f", "/path/to/key", "/tmp/buffer"}, ) assert.NoError(s.T(), err) args := cmd.Flags().Args() - assert.NotEmpty(s.T(), args, "should have positional args") + s.Require().NotEmpty(args, "should have positional args") assert.Equal(s.T(), "/tmp/buffer", args[len(args)-1], "last positional arg should be the buffer file") } @@ -52,15 +63,14 @@ func (s *GitSSHSignatureTestSuite) TestKnownFlagsParsed() { err := cmd.ParseFlags( []string{"-Y", "sign", "-n", "git", "-f", "/path/to/key", "/tmp/buffer"}, ) - assert.NoError(s.T(), err, "flag parsing should succeed") + assert.NoError(s.T(), err) val, err := cmd.Flags().GetString("command") assert.NoError(s.T(), err) assert.Equal(s.T(), "sign", val, "command flag should be 'sign'") - // The buffer file should be the last positional argument args := cmd.Flags().Args() - assert.NotEmpty(s.T(), args, "should have positional args") + s.Require().NotEmpty(args, "should have positional args") assert.Equal(s.T(), "/tmp/buffer", args[len(args)-1], "last positional arg should be the buffer file") } diff --git a/cmd/up.go b/cmd/up.go index 131642f10..deb7d9c90 100644 --- a/cmd/up.go +++ b/cmd/up.go @@ -383,13 +383,7 @@ func (cmd *UpCmd) configureWorkspace( log.Info("SSH configuration completed in workspace") } - if cmd.GitSSHSigningKey != "" { - if err := setupGitSSHSignature(cmd.GitSSHSigningKey, client); err != nil { - return err - } - } - - return setupDotfiles( + if err := setupDotfiles( cmd.DotfilesSource, cmd.DotfilesScript, cmd.DotfilesScriptEnvFile, @@ -397,7 +391,19 @@ func (cmd *UpCmd) configureWorkspace( client, devPodConfig, log, - ) + ); err != nil { + return err + } + + // Run after dotfiles so the signing config isn't overwritten by a + // dotfiles installer that replaces .gitconfig. + if cmd.GitSSHSigningKey != "" { + if err := setupGitSSHSignature(cmd.GitSSHSigningKey, client); err != nil { + return err + } + } + + return nil } // openIDE opens the configured IDE. diff --git a/pkg/gitsshsigning/helper.go b/pkg/gitsshsigning/helper.go index 6e0c334a1..2d7f1962a 100644 --- a/pkg/gitsshsigning/helper.go +++ b/pkg/gitsshsigning/helper.go @@ -196,8 +196,7 @@ func isDevpodManagedGpgKey(inGpgSection bool, trimmed string) bool { if !inGpgSection || len(trimmed) == 0 || trimmed[0] == '[' { return false } - return strings.HasPrefix(trimmed, "format = ssh") || - strings.HasPrefix(trimmed, "program = devpod-ssh-signature") + return strings.HasPrefix(trimmed, "format = ssh") } // filterGpgSSHSection removes devpod-managed keys from a buffered [gpg "ssh"]