fix(agent/git_ssh_signature): remove agent-forwarding and start-services flags (#662)

Author: skevetter

cmd/agent/git_ssh_signature.go

@@ -1,23 +1,17 @@
 package agent
 
 import (
-	"errors"
+	"fmt"
+	"os"
+	"os/exec"
+	"strings"
 
 	"github.com/skevetter/devpod/cmd/flags"
 	"github.com/skevetter/devpod/pkg/gitsshsigning"
 	"github.com/skevetter/log"
 	"github.com/spf13/cobra"
 )
 
-type GitSSHSignatureCmd struct {
-	*flags.GlobalFlags
-
-	CertPath   string
-	Namespace  string
-	BufferFile string
-	Command    string
-}
-
 // NewGitSSHSignatureCmd creates new git-ssh-signature command
 // This agent command can be used as git ssh program by setting
 //
@@ -28,41 +22,119 @@ 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.certPath == "" {
+				return fmt.Errorf("key file (-f) is required")
+			}
+			if parsed.namespace == "" {
+				return fmt.Errorf("namespace (-n) is required")
+			}
+			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
+	consumed := make(map[int]bool)
+	for i := 0; i < len(args); i++ {
+		if next := consumeFlag(&result, args, i); next > i {
+			consumed[i] = true
+			consumed[next] = true
+			i = next
+		}
+	}
+	// The buffer file is always the last argument and is never a flag.
+	lastIdx := len(args) - 1
+	if lastIdx >= 0 && !consumed[lastIdx] && !strings.HasPrefix(args[lastIdx], "-") {
+		result.bufferFile = args[lastIdx]
+	}
+	return result
+}
+
+// consumeFlag processes a single known flag from args at position i.
+// Returns the index of the consumed value if a known flag-value pair is found,
+// or i if no value was consumed.
+func consumeFlag(result *sshKeygenArgs, args []string, i int) int {
+	if i+1 >= len(args) {
+		return i
+	}
+	next := args[i+1]
+
+	switch args[i] {
+	case "-Y":
+		if strings.HasPrefix(next, "-") {
+			return i
+		}
+		result.command = next
+		return i + 1
+	case "-f":
+		if strings.HasPrefix(next, "-") {
+			return i
+		}
+		result.certPath = next
+		return i + 1
+	case "-n":
+		if strings.HasPrefix(next, "-") {
+			return i
+		}
+		result.namespace = next
+		return i + 1
+	}
+	return 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_helper.go

@@ -29,7 +29,7 @@ type GitSSHSignatureHelperCmd struct {
 // The signing key path is a required argument for this command. It should be what equal to what you
 // would have set as user.signingkey git config.
 func NewGitSSHSignatureHelperCmd(flags *flags.GlobalFlags) *cobra.Command {
-	cmd := &GitSSHSignatureCmd{
+	cmd := &GitSSHSignatureHelperCmd{
 		GlobalFlags: flags,
 	}
 

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

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