Skip to content

Commit 7247df2

Browse files
skevetterskevetter
committed
fix: use DisableFlagParsing to correctly handle ssh-keygen argument format
Replace the cobra flag-parsing approach with DisableFlagParsing:true and a dedicated parseSSHKeygenArgs parser. This command implements the ssh-keygen protocol used by git, not a standard CLI; cobra's flag parser cannot handle boolean flags like -U (ssh-agent mode) that git passes before the buffer file because pflag consumes the following argument as the flag's value, leaving positional args empty. With DisableFlagParsing, RunE receives the raw args and parseSSHKeygenArgs parses -Y/-f/-n by position, treating the last non-flag argument as the buffer file regardless of what flags precede it. delegateToSSHKeygen now takes args directly instead of parsing os.Args.
1 parent d5456f4 commit 7247df2

File tree

2 files changed

+91
-172
lines changed

2 files changed

+91
-172
lines changed

‎cmd/agent/git_ssh_signature.go‎

Lines changed: 60 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -31,91 +31,91 @@ type GitSSHSignatureCmd struct {
3131
//
3232
// custom-ssh-signature-handler -Y sign -n git -f /Users/johndoe/.ssh/my-key.pub /tmp/.git_signing_buffer_tmp4Euk6d
3333
func NewGitSSHSignatureCmd(flags *flags.GlobalFlags) *cobra.Command {
34-
cmd := &GitSSHSignatureCmd{
35-
GlobalFlags: flags,
36-
}
37-
38-
gitSSHSignatureCmd := &cobra.Command{
34+
return &cobra.Command{
3935
Use: "git-ssh-signature",
40-
// Allow unknown flags so that git can pass any ssh-keygen flags
41-
// (e.g. -U for stdin input) without cobra rejecting them.
42-
FParseErrWhitelist: cobra.FParseErrWhitelist{
43-
UnknownFlags: true,
44-
},
45-
RunE: func(_ *cobra.Command, _ []string) error {
36+
// This command implements the ssh-keygen protocol used by git for commit
37+
// signing. Disable cobra's flag parsing so we can handle the ssh-keygen
38+
// argument format directly, including boolean flags like -U (ssh-agent
39+
// mode) that cobra cannot distinguish from flags that take a value.
40+
DisableFlagParsing: true,
41+
RunE: func(_ *cobra.Command, args []string) error {
4642
logger := log.GetInstance()
4743

44+
parsed := parseSSHKeygenArgs(args)
45+
4846
// For non-sign operations (verify, find-principals, check-novalidate),
4947
// delegate command to system ssh-keygen since op does not require the tunnel.
50-
if cmd.Command != "sign" {
51-
return delegateToSSHKeygen(logger)
48+
if parsed.command != "sign" {
49+
return delegateToSSHKeygen(args, logger)
5250
}
5351

54-
// Extract the buffer file directly from os.Args instead of cobra's
55-
// parsed args. When git signs with an ssh-agent key, it passes
56-
// -U (a boolean flag) before the buffer file. Because cobra doesn't
57-
// know -U is boolean, it consumes the buffer file as -U's value,
58-
// leaving cobra's positional args empty.
59-
bufferFile, err := extractBufferFileFromArgs()
60-
if err != nil {
61-
return err
52+
if parsed.bufferFile == "" {
53+
return fmt.Errorf("buffer file is required")
6254
}
63-
cmd.BufferFile = bufferFile
6455

6556
return gitsshsigning.HandleGitSSHProgramCall(
66-
cmd.CertPath, cmd.Namespace, cmd.BufferFile, logger)
57+
parsed.certPath, parsed.namespace, parsed.bufferFile, logger)
6758
},
6859
}
60+
}
6961

70-
gitSSHSignatureCmd.Flags().StringVarP(&cmd.CertPath, "file", "f", "", "Path to the private key")
71-
gitSSHSignatureCmd.Flags().StringVarP(&cmd.Namespace, "namespace", "n", "", "Namespace")
72-
gitSSHSignatureCmd.Flags().
73-
StringVarP(&cmd.Command, "command", "Y", "sign", "Command - should be 'sign'")
62+
// sshKeygenArgs holds the parsed result of a git ssh-keygen invocation.
63+
type sshKeygenArgs struct {
64+
command string // -Y value (sign, verify, find-principals, etc.)
65+
certPath string // -f value (path to public key)
66+
namespace string // -n value (always "git" for commit signing)
67+
bufferFile string // last non-flag argument
68+
}
7469

75-
return gitSSHSignatureCmd
70+
// parseSSHKeygenArgs parses the ssh-keygen argument format used by git:
71+
//
72+
// -Y <command> -n <namespace> -f <key> [flags...] <buffer-file>
73+
//
74+
// The buffer file is always the last argument and is never a flag.
75+
// Unknown flags (e.g. -U for ssh-agent mode) are ignored.
76+
func parseSSHKeygenArgs(args []string) sshKeygenArgs {
77+
result := sshKeygenArgs{
78+
command: "sign",
79+
} // git only ever calls with sign, but default defensively
80+
for i := 0; i < len(args); i++ {
81+
consumeFlag(&result, args, &i)
82+
}
83+
// The buffer file is always the last argument and is never a flag.
84+
if len(args) > 0 && !strings.HasPrefix(args[len(args)-1], "-") {
85+
result.bufferFile = args[len(args)-1]
86+
}
87+
return result
7688
}
7789

78-
// extractBufferFileFromArgs extracts the buffer file directly from os.Args.
79-
// It scans backward from the end of the arguments (after "git-ssh-signature")
80-
// looking for the last non-flag argument, which is the buffer file path.
81-
func extractBufferFileFromArgs() (string, error) {
82-
for i, arg := range os.Args {
83-
if strings.HasSuffix(arg, "git-ssh-signature") {
84-
remaining := os.Args[i+1:]
85-
for j := len(remaining) - 1; j >= 0; j-- {
86-
if !strings.HasPrefix(remaining[j], "-") {
87-
return remaining[j], nil
88-
}
89-
}
90-
return "", fmt.Errorf("buffer file is required (no non-flag argument found in args)")
91-
}
90+
// consumeFlag processes a single known flag from args at position i, advancing
91+
// i past the flag's value when present.
92+
func consumeFlag(result *sshKeygenArgs, args []string, i *int) {
93+
if *i+1 >= len(args) {
94+
return
95+
}
96+
switch args[*i] {
97+
case "-Y":
98+
result.command = args[*i+1]
99+
*i++
100+
case "-f":
101+
result.certPath = args[*i+1]
102+
*i++
103+
case "-n":
104+
result.namespace = args[*i+1]
105+
*i++
92106
}
93-
return "", fmt.Errorf("buffer file is required (git-ssh-signature not found in process args)")
94107
}
95108

96-
// delegateToSSHKeygen forwards the original arguments to the system ssh-keygen binary.
97-
func delegateToSSHKeygen(logger log.Logger) error {
109+
// delegateToSSHKeygen forwards args to the system ssh-keygen binary.
110+
func delegateToSSHKeygen(args []string, logger log.Logger) error {
98111
sshKeygen, err := exec.LookPath("ssh-keygen")
99112
if err != nil {
100113
return fmt.Errorf("find ssh-keygen: %w", err)
101114
}
102115

103-
// Extract the arguments that were originally passed to this command.
104-
// Find "git-ssh-signature" in os.Args and take everything after it.
105-
var sshArgs []string
106-
for i, arg := range os.Args {
107-
if strings.HasSuffix(arg, "git-ssh-signature") {
108-
sshArgs = os.Args[i+1:]
109-
break
110-
}
111-
}
112-
if sshArgs == nil {
113-
return fmt.Errorf("git-ssh-signature not found in process arguments")
114-
}
115-
116-
logger.Debugf("delegating to ssh-keygen: %s %v", sshKeygen, sshArgs)
116+
logger.Debugf("delegating to ssh-keygen: %s %v", sshKeygen, args)
117117

118-
c := exec.Command(sshKeygen, sshArgs...) // #nosec G204,G304,G702
118+
c := exec.Command(sshKeygen, args...) // #nosec G204,G304,G702
119119
c.Stdin = os.Stdin
120120
c.Stdout = os.Stdout
121121
c.Stderr = os.Stderr

‎cmd/agent/git_ssh_signature_test.go‎

Lines changed: 31 additions & 112 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,8 @@
11
package agent
22

33
import (
4-
"os"
54
"testing"
65

7-
"github.com/skevetter/devpod/cmd/flags"
86
"github.com/stretchr/testify/assert"
97
"github.com/stretchr/testify/suite"
108
)
@@ -17,122 +15,43 @@ func TestGitSSHSignatureSuite(t *testing.T) {
1715
suite.Run(t, new(GitSSHSignatureTestSuite))
1816
}
1917

20-
func (s *GitSSHSignatureTestSuite) TestAcceptsUnknownFlags() {
21-
cmd := NewGitSSHSignatureCmd(&flags.GlobalFlags{})
22-
23-
// Git passes: -Y sign -n git -f /path/to/key -U /dev/stdin /tmp/buffer
24-
// -U is an unknown flag that consumes /dev/stdin as its value.
25-
// /tmp/buffer remains as a positional argument.
26-
err := cmd.ParseFlags(
27-
[]string{
28-
"-Y",
29-
"sign",
30-
"-n",
31-
"git",
32-
"-f",
33-
"/path/to/key",
34-
"-U",
35-
"/dev/stdin",
36-
"/tmp/buffer",
37-
},
38-
)
39-
assert.NoError(s.T(), err, "flag parsing should succeed with unknown flag -U")
40-
41-
args := cmd.Flags().Args()
42-
s.Require().NotEmpty(args, "should have positional args")
43-
assert.Equal(s.T(), "/tmp/buffer", args[len(args)-1],
44-
"buffer file should be preserved as last positional arg")
18+
func (s *GitSSHSignatureTestSuite) TestParseBasicSignArgs() {
19+
args := []string{"-Y", "sign", "-n", "git", "-f", "/path/to/key.pub", "/tmp/buffer"}
20+
result := parseSSHKeygenArgs(args)
21+
assert.Equal(s.T(), "sign", result.command)
22+
assert.Equal(s.T(), "git", result.namespace)
23+
assert.Equal(s.T(), "/path/to/key.pub", result.certPath)
24+
assert.Equal(s.T(), "/tmp/buffer", result.bufferFile)
4525
}
4626

47-
func (s *GitSSHSignatureTestSuite) TestBufferFileAsPositionalArg() {
48-
cmd := NewGitSSHSignatureCmd(&flags.GlobalFlags{})
49-
50-
err := cmd.ParseFlags(
51-
[]string{"-Y", "sign", "-n", "git", "-f", "/path/to/key", "/tmp/buffer"},
52-
)
53-
assert.NoError(s.T(), err)
54-
55-
args := cmd.Flags().Args()
56-
s.Require().NotEmpty(args, "should have positional args")
57-
assert.Equal(s.T(), "/tmp/buffer", args[len(args)-1],
58-
"last positional arg should be the buffer file")
27+
func (s *GitSSHSignatureTestSuite) TestParseWithAgentFlag() {
28+
// When the signing key is loaded in the ssh-agent, git passes -U (a boolean
29+
// "use agent" flag) immediately before the buffer file. The buffer file must
30+
// still be recognised as the last non-flag argument.
31+
args := []string{"-Y", "sign", "-n", "git", "-f", "/path/to/key.pub", "-U", "/tmp/buffer"}
32+
result := parseSSHKeygenArgs(args)
33+
assert.Equal(s.T(), "sign", result.command)
34+
assert.Equal(s.T(), "/path/to/key.pub", result.certPath)
35+
assert.Equal(s.T(), "/tmp/buffer", result.bufferFile)
5936
}
6037

61-
func (s *GitSSHSignatureTestSuite) TestUnknownBooleanFlagBeforeBuffer() {
62-
// When git signs with an ssh-agent key it passes:
63-
// -Y sign -n git -f /path/to/key.pub -U /tmp/buffer
64-
// Here -U is a boolean flag (use ssh-agent) but cobra doesn't know that,
65-
// so it consumes /tmp/buffer as -U's value, leaving no positional args.
66-
// This test verifies that extractBufferFileFromArgs handles this correctly
67-
// by reading directly from os.Args.
68-
cmd := NewGitSSHSignatureCmd(&flags.GlobalFlags{})
69-
70-
// Demonstrate that cobra's flag parsing eats the buffer file
71-
err := cmd.ParseFlags(
72-
[]string{"-Y", "sign", "-n", "git", "-f", "/path/to/key.pub", "-U", "/tmp/buffer"},
73-
)
74-
assert.NoError(s.T(), err, "flag parsing should succeed")
75-
cobraArgs := cmd.Flags().Args()
76-
assert.Empty(s.T(), cobraArgs,
77-
"cobra should have no positional args because -U consumed /tmp/buffer")
78-
79-
// Verify that extractBufferFileFromArgs finds the buffer file from os.Args
80-
origArgs := os.Args
81-
defer func() { os.Args = origArgs }()
82-
83-
os.Args = []string{
84-
"devpod", "agent", "git-ssh-signature",
85-
"-Y", "sign", "-n", "git", "-f", "/path/to/key.pub", "-U", "/tmp/buffer",
86-
}
87-
bufferFile, err := extractBufferFileFromArgs()
88-
assert.NoError(s.T(), err)
89-
assert.Equal(s.T(), "/tmp/buffer", bufferFile,
90-
"should extract buffer file even when -U precedes it")
38+
func (s *GitSSHSignatureTestSuite) TestParseNonSignCommand() {
39+
args := []string{"-Y", "verify", "-n", "git", "-f", "/path/to/key.pub", "/tmp/buffer"}
40+
result := parseSSHKeygenArgs(args)
41+
assert.Equal(s.T(), "verify", result.command)
9142
}
9243

93-
func (s *GitSSHSignatureTestSuite) TestExtractBufferFileFromArgs() {
94-
origArgs := os.Args
95-
defer func() { os.Args = origArgs }()
96-
97-
// Normal case: buffer file is last arg
98-
os.Args = []string{
99-
"devpod", "agent", "git-ssh-signature",
100-
"-Y", "sign", "-n", "git", "-f", "/path/to/key", "/tmp/buffer",
101-
}
102-
bufferFile, err := extractBufferFileFromArgs()
103-
assert.NoError(s.T(), err)
104-
assert.Equal(s.T(), "/tmp/buffer", bufferFile)
105-
106-
// Error case: no non-flag argument after git-ssh-signature (all args are flags)
107-
os.Args = []string{
108-
"devpod", "agent", "git-ssh-signature",
109-
"-Y", "-n", "-f", "-U",
110-
}
111-
_, err = extractBufferFileFromArgs()
112-
assert.Error(s.T(), err)
113-
assert.Contains(s.T(), err.Error(), "no non-flag argument found")
114-
115-
// Error case: git-ssh-signature not in args
116-
os.Args = []string{"some-other-binary", "-Y", "sign"}
117-
_, err = extractBufferFileFromArgs()
118-
assert.Error(s.T(), err)
119-
assert.Contains(s.T(), err.Error(), "git-ssh-signature not found")
44+
func (s *GitSSHSignatureTestSuite) TestParseMissingBufferFile() {
45+
// All args end in a flag — no buffer file present.
46+
args := []string{"-Y", "sign", "-n", "git", "-f", "/path/to/key.pub", "-U"}
47+
result := parseSSHKeygenArgs(args)
48+
assert.Equal(s.T(), "", result.bufferFile)
12049
}
12150

122-
func (s *GitSSHSignatureTestSuite) TestKnownFlagsParsed() {
123-
cmd := NewGitSSHSignatureCmd(&flags.GlobalFlags{})
124-
125-
err := cmd.ParseFlags(
126-
[]string{"-Y", "sign", "-n", "git", "-f", "/path/to/key", "/tmp/buffer"},
127-
)
128-
assert.NoError(s.T(), err)
129-
130-
val, err := cmd.Flags().GetString("command")
131-
assert.NoError(s.T(), err)
132-
assert.Equal(s.T(), "sign", val, "command flag should be 'sign'")
133-
134-
args := cmd.Flags().Args()
135-
s.Require().NotEmpty(args, "should have positional args")
136-
assert.Equal(s.T(), "/tmp/buffer", args[len(args)-1],
137-
"last positional arg should be the buffer file")
51+
func (s *GitSSHSignatureTestSuite) TestParseDefaultsToSign() {
52+
// If -Y is absent the command defaults to "sign".
53+
args := []string{"-n", "git", "-f", "/path/to/key.pub", "/tmp/buffer"}
54+
result := parseSSHKeygenArgs(args)
55+
assert.Equal(s.T(), "sign", result.command)
56+
assert.Equal(s.T(), "/tmp/buffer", result.bufferFile)
13857
}

0 commit comments

Comments
 (0)