diff --git a/cmd/agent/git_ssh_signature.go b/cmd/agent/git_ssh_signature.go index 3eb34ca36..48fa82f7d 100644 --- a/cmd/agent/git_ssh_signature.go +++ b/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" @@ -9,15 +12,6 @@ import ( "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 -n -f [flags...] +// +// 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() } diff --git a/cmd/agent/git_ssh_signature_helper.go b/cmd/agent/git_ssh_signature_helper.go index 30eba0d9b..9d8627ea2 100644 --- a/cmd/agent/git_ssh_signature_helper.go +++ b/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, } diff --git a/cmd/agent/git_ssh_signature_test.go b/cmd/agent/git_ssh_signature_test.go index 39e982b8d..59a06bef0 100644 --- a/cmd/agent/git_ssh_signature_test.go +++ b/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) } diff --git a/cmd/up.go b/cmd/up.go index deb7d9c90..512dbf171 100644 --- a/cmd/up.go +++ b/cmd/up.go @@ -1565,8 +1565,6 @@ func setupGitSSHSignature( out, err := exec.Command( execPath, "ssh", - "--agent-forwarding=true", - "--start-services=true", "--user", remoteUser, "--context", diff --git a/e2e/tests/ssh/ssh.go b/e2e/tests/ssh/ssh.go index c39995fda..11cbee40b 100644 --- a/e2e/tests/ssh/ssh.go +++ b/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", ) }, )