fix(agent/git_ssh_signature): remove agent-forwarding and start-services flags from setupGitSSHSignature

cmd/agent/git_ssh_signature.go

@@ -1,7 +1,10 @@
 package agent
 
 import (
-	"errors"
+	"fmt"
+	"os"
+	"os/exec"
+	"strings"
 
 	"github.com/skevetter/devpod/cmd/flags"
 	"github.com/skevetter/devpod/pkg/gitsshsigning"
@@ -28,41 +31,94 @@ type GitSSHSignatureCmd struct {
 //
 //	custom-ssh-signature-handler -Y sign -n git -f /Users/johndoe/.ssh/my-key.pub /tmp/.git_signing_buffer_tmp4Euk6d
 func NewGitSSHSignatureCmd(flags *flags.GlobalFlags) *cobra.Command {
-	cmd := &GitSSHSignatureCmd{
-		GlobalFlags: flags,
-	}
-
-	gitSSHSignatureCmd := &cobra.Command{
+	return &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,
-		},
+		// This command implements the ssh-keygen protocol used by git for commit
+		// signing. Disable cobra's flag parsing so we can handle the ssh-keygen
+		// argument format directly, including boolean flags like -U (ssh-agent
+		// mode) that cobra cannot distinguish from flags that take a value.
+		DisableFlagParsing: true,
 		RunE: func(_ *cobra.Command, args []string) error {
 			logger := log.GetInstance()
 
-			if len(args) < 1 {
-				logger.Fatalf("Buffer file is required")
-			}
+			parsed := parseSSHKeygenArgs(args)
 
-			// Check if the required -Y sign flags are present
-			if cmd.Command != "sign" {
-				return errors.New("must include '-Y sign' arguments")
+			// For non-sign operations (verify, find-principals, check-novalidate),
+			// delegate command to system ssh-keygen since op does not require the tunnel.
+			if parsed.command != "sign" {
+				return delegateToSSHKeygen(args, logger)
 			}
 
-			// The last argument is the buffer file
-			cmd.BufferFile = args[len(args)-1]
+			if parsed.bufferFile == "" {
+				return fmt.Errorf("buffer file is required")
+			}
 
 			return gitsshsigning.HandleGitSSHProgramCall(
-				cmd.CertPath, cmd.Namespace, cmd.BufferFile, logger)
+				parsed.certPath, parsed.namespace, parsed.bufferFile, logger)
 		},
 	}
+}
+
+// sshKeygenArgs holds the parsed result of a git ssh-keygen invocation.
+type sshKeygenArgs struct {
+	command    string // -Y value (sign, verify, find-principals, etc.)
+	certPath   string // -f value (path to public key)
+	namespace  string // -n value (always "git" for commit signing)
+	bufferFile string // last non-flag argument
+}
+
+// parseSSHKeygenArgs parses the ssh-keygen argument format used by git:
+//
+//	-Y <command> -n <namespace> -f <key> [flags...] <buffer-file>
+//
+// The buffer file is always the last argument and is never a flag.
+// Unknown flags (e.g. -U for ssh-agent mode) are ignored.
+func parseSSHKeygenArgs(args []string) sshKeygenArgs {
+	result := sshKeygenArgs{
+		command: "sign",
+	} // git only ever calls with sign, but default defensively
+	for i := 0; i < len(args); i++ {
+		consumeFlag(&result, args, &i)
+	}
+	// The buffer file is always the last argument and is never a flag.
+	if len(args) > 0 && !strings.HasPrefix(args[len(args)-1], "-") {
+		result.bufferFile = args[len(args)-1]
+	}
+	return result
+}
+
+// consumeFlag processes a single known flag from args at position i, advancing
+// i past the flag's value when present.
+func consumeFlag(result *sshKeygenArgs, args []string, i *int) {
+	if *i+1 >= len(args) {
+		return
+	}
+	switch args[*i] {
+	case "-Y":
+		result.command = args[*i+1]
+		*i++
+	case "-f":
+		result.certPath = args[*i+1]
+		*i++
+	case "-n":
+		result.namespace = args[*i+1]
+		*i++
+	}
+}
+
+// delegateToSSHKeygen forwards args to the system ssh-keygen binary.
+func delegateToSSHKeygen(args []string, logger log.Logger) error {
+	sshKeygen, err := exec.LookPath("ssh-keygen")
+	if err != nil {
+		return fmt.Errorf("find ssh-keygen: %w", err)
+	}
+
+	logger.Debugf("delegating to ssh-keygen: %s %v", sshKeygen, args)
 
-	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'")
+	c := exec.Command(sshKeygen, args...) // #nosec G204,G304,G702
+	c.Stdin = os.Stdin
+	c.Stdout = os.Stdout
+	c.Stderr = os.Stderr
 
-	return gitSSHSignatureCmd
+	return c.Run()
 }

cmd/agent/git_ssh_signature_test.go

@@ -3,7 +3,6 @@ package agent
 import (
 	"testing"
 
-	"github.com/skevetter/devpod/cmd/flags"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/suite"
 )
@@ -16,61 +15,43 @@ func TestGitSSHSignatureSuite(t *testing.T) {
 	suite.Run(t, new(GitSSHSignatureTestSuite))
 }
 
-func (s *GitSSHSignatureTestSuite) TestAcceptsUnknownFlags() {
-	cmd := NewGitSSHSignatureCmd(&flags.GlobalFlags{})
-
-	// 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",
-			"/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) TestParseBasicSignArgs() {
+	args := []string{"-Y", "sign", "-n", "git", "-f", "/path/to/key.pub", "/tmp/buffer"}
+	result := parseSSHKeygenArgs(args)
+	assert.Equal(s.T(), "sign", result.command)
+	assert.Equal(s.T(), "git", result.namespace)
+	assert.Equal(s.T(), "/path/to/key.pub", result.certPath)
+	assert.Equal(s.T(), "/tmp/buffer", result.bufferFile)
 }
 
-func (s *GitSSHSignatureTestSuite) TestBufferFileAsPositionalArg() {
-	cmd := NewGitSSHSignatureCmd(&flags.GlobalFlags{})
-
-	err := cmd.ParseFlags(
-		[]string{"-Y", "sign", "-n", "git", "-f", "/path/to/key", "/tmp/buffer"},
-	)
-	assert.NoError(s.T(), err)
-
-	args := cmd.Flags().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")
+func (s *GitSSHSignatureTestSuite) TestParseWithAgentFlag() {
+	// When the signing key is loaded in the ssh-agent, git passes -U (a boolean
+	// "use agent" flag) immediately before the buffer file. The buffer file must
+	// still be recognised as the last non-flag argument.
+	args := []string{"-Y", "sign", "-n", "git", "-f", "/path/to/key.pub", "-U", "/tmp/buffer"}
+	result := parseSSHKeygenArgs(args)
+	assert.Equal(s.T(), "sign", result.command)
+	assert.Equal(s.T(), "/path/to/key.pub", result.certPath)
+	assert.Equal(s.T(), "/tmp/buffer", result.bufferFile)
 }
 
-func (s *GitSSHSignatureTestSuite) TestKnownFlagsParsed() {
-	cmd := NewGitSSHSignatureCmd(&flags.GlobalFlags{})
-
-	err := cmd.ParseFlags(
-		[]string{"-Y", "sign", "-n", "git", "-f", "/path/to/key", "/tmp/buffer"},
-	)
-	assert.NoError(s.T(), err)
+func (s *GitSSHSignatureTestSuite) TestParseNonSignCommand() {
+	args := []string{"-Y", "verify", "-n", "git", "-f", "/path/to/key.pub", "/tmp/buffer"}
+	result := parseSSHKeygenArgs(args)
+	assert.Equal(s.T(), "verify", result.command)
+}
 
-	val, err := cmd.Flags().GetString("command")
-	assert.NoError(s.T(), err)
-	assert.Equal(s.T(), "sign", val, "command flag should be 'sign'")
+func (s *GitSSHSignatureTestSuite) TestParseMissingBufferFile() {
+	// All args end in a flag — no buffer file present.
+	args := []string{"-Y", "sign", "-n", "git", "-f", "/path/to/key.pub", "-U"}
+	result := parseSSHKeygenArgs(args)
+	assert.Equal(s.T(), "", result.bufferFile)
+}
 
-	args := cmd.Flags().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")
+func (s *GitSSHSignatureTestSuite) TestParseDefaultsToSign() {
+	// If -Y is absent the command defaults to "sign".
+	args := []string{"-n", "git", "-f", "/path/to/key.pub", "/tmp/buffer"}
+	result := parseSSHKeygenArgs(args)
+	assert.Equal(s.T(), "sign", result.command)
+	assert.Equal(s.T(), "/tmp/buffer", result.bufferFile)
 }

cmd/up.go

@@ -1565,8 +1565,6 @@ func setupGitSSHSignature(
 	out, err := exec.Command(
 		execPath,
 		"ssh",
-		"--agent-forwarding=true",
-		"--start-services=true",
 		"--user",
 		remoteUser,
 		"--context",

e2e/tests/ssh/ssh.go

@@ -106,8 +106,9 @@ var _ = ginkgo.Describe("devpod ssh test suite", ginkgo.Label("ssh"), ginkgo.Ord
 	// })
 
 	ginkgo.It(
-		"should set up git SSH signature helper in workspace",
-		func(ctx context.Context) {
+		"should set up git SSH signature helper and sign a commit",
+		ginkgo.SpecTimeout(7*time.Minute),
+		func(ctx ginkgo.SpecContext) {
 			if runtime.GOOS == "windows" {
 				ginkgo.Skip("skipping on windows")
 			}
@@ -138,18 +139,11 @@ var _ = ginkgo.Describe("devpod ssh test suite", ginkgo.Label("ssh"), ginkgo.Ord
 			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",
-			)
+			err = f.DevPodUp(ctx, 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,
+			// 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)
@@ -158,24 +152,103 @@ var _ = ginkgo.Describe("devpod ssh test suite", ginkgo.Label("ssh"), ginkgo.Ord
 				"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",
-			)
+			// 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"),
-				"git gpg.ssh.program should be set to devpod-ssh-signature",
+			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.
+			commitCmd := strings.Join([]string{
+				"cd /tmp",
+				"git init test-sign-repo",
+				"cd test-sign-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 test commit' 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 test commit"),
+				"git commit should succeed with the signed test commit message",
 			)
 
-			// Verify git config has gpg format set to ssh
-			out, err = f.DevPodSSH(sshCtx, tempDir,
-				"git config --global gpg.format",
+			// 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.
+			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-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,
+			})
+			ginkgo.GinkgoWriter.Printf("verify stdout: %s\n", stdout)
+			ginkgo.GinkgoWriter.Printf("verify stderr: %s\n", stderr)
+			framework.ExpectNoError(err)
+
+			// git verify-commit writes signature details to stderr
+			combined := stdout + stderr
+			gomega.Expect(combined).To(
+				gomega.ContainSubstring("Good"),
+				"git verify-commit should report a good SSH signature",
 			)
+
+			// 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,
+			})
+			ginkgo.GinkgoWriter.Printf("log stdout: %s\n", stdout)
+			ginkgo.GinkgoWriter.Printf("log stderr: %s\n", stderr)
 			framework.ExpectNoError(err)
-			gomega.Expect(strings.TrimSpace(out)).To(
-				gomega.Equal("ssh"),
-				"git gpg.format should be set to ssh",
+
+			combined = stdout + stderr
+			gomega.Expect(combined).To(
+				gomega.ContainSubstring("Good"),
+				"git log --show-signature should report a good signature",
+			)
+			gomega.Expect(combined).To(
+				gomega.ContainSubstring("test@example.com"),
+				"signature should be associated with the test email principal",
 			)
 		},
 	)