fix(agent/git_ssh_signature): ssh signature forwarding fails when signing#648
Merged
fix(agent/git_ssh_signature): ssh signature forwarding fails when signing#648
Conversation
- 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
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
📝 WalkthroughWalkthroughWhitelist unknown ssh-keygen flags for the Git SSH signature CLI command, rename the command variable, capture helper command output on failure, refactor removal of devpod-managed Git SSH signing config, and add unit and e2e tests validating flag parsing, config filtering, and workspace installation. Changes
Sequence Diagram(s)(omitted — changes do not introduce a new multi-component control flow that requires a sequence diagram) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/up.go (1)
1557-1569:⚠️ Potential issue | 🟠 MajorShell-escape
signingKeybefore building--command.This injects raw CLI input into the remote command string. A key path with spaces breaks the helper call, and shell metacharacters can change what runs in the workspace. Quote/escape the value, or pass it as a distinct argv element if the SSH layer supports that.
Possible fix
+ escapedSigningKey := strings.ReplaceAll(signingKey, "'", `'"'"'`) + // `#nosec` G204 -- execPath is from os.Executable(), not user input out, err := exec.Command( execPath, "ssh", @@ "--user", remoteUser, "--context", client.Context(), client.Workspace(), - "--command", fmt.Sprintf("devpod agent git-ssh-signature-helper %s", signingKey), + "--command", fmt.Sprintf( + "devpod agent git-ssh-signature-helper '%s'", + escapedSigningKey, + ), ).CombinedOutput()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/up.go` around lines 1557 - 1569, The remote command currently injects signingKey directly into the SSH --command string (see execPath and exec.Command call), which allows spaces or shell metacharacters to break or alter the command; fix this by shell-escaping or quoting signingKey before building the "--command" argument (e.g., replace the raw fmt.Sprintf("devpod agent git-ssh-signature-helper %s", signingKey) usage with a properly quoted/escaped value such as using strconv.Quote(signingKey) or a dedicated shell-escape utility), and add the needed import; ensure the changed value is used in the exec.Command call so the remote helper receives a single safe argument.
🧹 Nitpick comments (2)
e2e/tests/ssh/ssh.go (1)
140-179: Exercise a real signed Git command in this e2e.This only proves the helper was installed and two config keys were written. It won't fail if the actual signing path still breaks when Git invokes
gpg.ssh.programwith newerssh-keygenflags. Running a smallgit init && git add && git commit -Sinside the workspace would cover the regression this PR is targeting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/ssh/ssh.go` around lines 140 - 179, The test currently only verifies the helper script and git configs but never actually exercises signing; update the e2e in ssh.go to perform a real signed commit using the existing helpers (invoke f.DevPodSSH after DevPodUp) by running commands like "git init", creating a test file, "git add", and "git commit -S -m 'test'" (or "GIT_COMMITTER_NAME/EMAIL" as needed) and then verify the commit is signed (e.g., check "git log --show-signature -1" or inspect commit exit code/output). Use the same sshCtx/context pattern and framework.ExpectNoError/gomega.Expect assertions around the commit and the signature verification to fail the test if SSH-based signing actually breaks; reference f.DevPodUp, f.DevPodSSH, and the existing sshCtx/DevPodUp setup when adding this step.cmd/agent/git_ssh_signature_test.go (1)
14-18: Assert the remaining buffer-file arg as well.These tests only check that flag parsing doesn't error.
RunEactually depends on the remaining positional arg being/tmp/buffer, so the suite should assert that after parsing; otherwise it can stay green while signing still fails.Minimal assertion to add after each
ParseFlagscallif err != nil { t.Fatalf("expected flag parsing to succeed, got: %v", err) } + if args := cmd.Flags().Args(); len(args) != 1 || args[0] != "/tmp/buffer" { + t.Fatalf("expected remaining args to contain only the buffer file, got: %v", args) + }Also applies to: 25-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/agent/git_ssh_signature_test.go` around lines 14 - 18, After each call to cmd.ParseFlags in git_ssh_signature_test.go (the ones at lines ~14 and 25-35), assert that the remaining positional argument is the expected buffer file ("/tmp/buffer") before proceeding to RunE; e.g., call cmd.Flags().Args() (or Flags().Arg(0)) and require that the first positional arg equals "/tmp/buffer" so the test fails if the buffer-file is not preserved by ParseFlags.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/gitsshsigning/helper.go`:
- Around line 162-176: The current logic uses inGpgSSHSection to unconditionally
skip every line in the `[gpg "ssh"]` subsection, which erases user-owned keys;
change this to collect lines inside the `[gpg "ssh"]` section (using the
existing inGpgSSHSection and trimmed checks) and only remove the specific
DevPod-managed entry(ies) while preserving other keys (e.g., allowedSignersFile,
revocationFile, custom program). Concretely: when you detect the section header
(trimmed == `[gpg "ssh"]`) set inGpgSSHSection but do not continue immediately;
instead buffer lines for that section, filter out the DevPod-managed config
lines from that buffer, and then if the buffer still contains other key lines
emit the header plus the remaining lines to the output, otherwise drop the whole
subsection; update any code that previously continued on inGpgSSHSection to use
this buffered/filter approach.
---
Outside diff comments:
In `@cmd/up.go`:
- Around line 1557-1569: The remote command currently injects signingKey
directly into the SSH --command string (see execPath and exec.Command call),
which allows spaces or shell metacharacters to break or alter the command; fix
this by shell-escaping or quoting signingKey before building the "--command"
argument (e.g., replace the raw fmt.Sprintf("devpod agent
git-ssh-signature-helper %s", signingKey) usage with a properly quoted/escaped
value such as using strconv.Quote(signingKey) or a dedicated shell-escape
utility), and add the needed import; ensure the changed value is used in the
exec.Command call so the remote helper receives a single safe argument.
---
Nitpick comments:
In `@cmd/agent/git_ssh_signature_test.go`:
- Around line 14-18: After each call to cmd.ParseFlags in
git_ssh_signature_test.go (the ones at lines ~14 and 25-35), assert that the
remaining positional argument is the expected buffer file ("/tmp/buffer") before
proceeding to RunE; e.g., call cmd.Flags().Args() (or Flags().Arg(0)) and
require that the first positional arg equals "/tmp/buffer" so the test fails if
the buffer-file is not preserved by ParseFlags.
In `@e2e/tests/ssh/ssh.go`:
- Around line 140-179: The test currently only verifies the helper script and
git configs but never actually exercises signing; update the e2e in ssh.go to
perform a real signed commit using the existing helpers (invoke f.DevPodSSH
after DevPodUp) by running commands like "git init", creating a test file, "git
add", and "git commit -S -m 'test'" (or "GIT_COMMITTER_NAME/EMAIL" as needed)
and then verify the commit is signed (e.g., check "git log --show-signature -1"
or inspect commit exit code/output). Use the same sshCtx/context pattern and
framework.ExpectNoError/gomega.Expect assertions around the commit and the
signature verification to fail the test if SSH-based signing actually breaks;
reference f.DevPodUp, f.DevPodSSH, and the existing sshCtx/DevPodUp setup when
adding this step.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 10f9d27e-f34f-4fce-9fbb-66acafec64af
📒 Files selected for processing (7)
cmd/agent/git_ssh_signature.gocmd/agent/git_ssh_signature_test.gocmd/up.goe2e/tests/ssh/ssh.goe2e/tests/ssh/testdata/ssh-signing/devcontainer.jsonpkg/gitsshsigning/helper.gopkg/gitsshsigning/helper_test.go
|
If you're new to commit signing, there are different ways to set it up: Sign commits with
|
- 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.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/gitsshsigning/helper.go (1)
195-201: Dead code:programcheck in[gpg]section.According to
GitConfigTemplate,program = devpod-ssh-signatureis under[gpg "ssh"], not[gpg]. This check (line 200) will never match since[gpg "ssh"]lines are handled byfilterGpgSSHSectioninstead.Consider removing the unreachable condition for clarity:
♻️ Suggested simplification
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") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/gitsshsigning/helper.go` around lines 195 - 201, The function isDevpodManagedGpgKey contains an unreachable check for 'program = devpod-ssh-signature' because that key lives under the [gpg "ssh"] subsection (handled elsewhere by filterGpgSSHSection); remove the redundant "program" prefix check and simplify isDevpodManagedGpgKey to only detect "format = ssh" (retain the existing inGpgSection and trimmed bounds checks) and update any affected callers/tests to reflect the simplified behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/agent/git_ssh_signature_test.go`:
- Around line 43-46: Replace the non-fatal assert with a fatal requirement
before indexing positional args: when calling cmd.Flags().Args() into the args
variable, use s.Require().NotEmpty(args, "should have positional args") (instead
of assert.NotEmpty(...)) before accessing args[len(args)-1]; do the same for the
other test case around lines 63-65 so we never index into an empty slice.
Reference: variable args and the call site cmd.Flags().Args() in the test
functions.
- Around line 22-30: The test TestAcceptsUnknownFlags currently passes "-U" and
"/tmp/buffer" such that the unknown flag consumes the buffer instead of testing
the positional-buffer forwarding; update the cmd.ParseFlags invocation in
git_ssh_signature_test.go so the buffer is provided as a separate positional
argument (e.g., include "-U" as one element and "/tmp/buffer" as the following
separate element, or append an extra "/tmp/buffer" positional arg after the
flags) to exercise the described positional-buffer scenario.
---
Nitpick comments:
In `@pkg/gitsshsigning/helper.go`:
- Around line 195-201: The function isDevpodManagedGpgKey contains an
unreachable check for 'program = devpod-ssh-signature' because that key lives
under the [gpg "ssh"] subsection (handled elsewhere by filterGpgSSHSection);
remove the redundant "program" prefix check and simplify isDevpodManagedGpgKey
to only detect "format = ssh" (retain the existing inGpgSection and trimmed
bounds checks) and update any affected callers/tests to reflect the simplified
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d051a24d-c7f1-4289-9d1d-cba7e0cf4f1d
📒 Files selected for processing (4)
cmd/agent/git_ssh_signature_test.gocmd/up.gopkg/gitsshsigning/helper.gopkg/gitsshsigning/helper_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/gitsshsigning/helper_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/up.go
skevetter
force-pushed
the
issue-645-git-ssh-signature
branch
from
2a1b8ab to
e9d10ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/up.go (1)
386-400:⚠️ Potential issue | 🟠 MajorRun
setupGitSSHSignatureaftersetupDotfiles.Dotfiles installers often rewrite
~/.gitconfig. With the current order, an explicit--git-ssh-signing-keysetup can be silently undone by the later dotfiles step, so the workspace no longer ends up with the DevPod-managed SSH signing config the flag asked for.Suggested change
- if cmd.GitSSHSigningKey != "" { - if err := setupGitSSHSignature(cmd.GitSSHSigningKey, client); err != nil { - return err - } - } - - return setupDotfiles( + if err := setupDotfiles( cmd.DotfilesSource, cmd.DotfilesScript, cmd.DotfilesScriptEnvFile, cmd.DotfilesScriptEnv, client, devPodConfig, log, - ) + ); err != nil { + return err + } + + if cmd.GitSSHSigningKey != "" { + if err := setupGitSSHSignature(cmd.GitSSHSigningKey, client); err != nil { + return err + } + } + + return nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/up.go` around lines 386 - 400, The current code calls setupGitSSHSignature (triggered when cmd.GitSSHSigningKey != "") before setupDotfiles, which allows dotfiles installers to overwrite the Git config and remove the SSH signing setup; move the conditional invocation of setupGitSSHSignature to run after setupDotfiles completes (i.e., call setupDotfiles first and then, if cmd.GitSSHSigningKey != "", call setupGitSSHSignature using the same client and propagate any error), preserving the existing error handling and return behavior so any failure in setupGitSSHSignature still returns an error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/gitsshsigning/helper_test.go`:
- Around line 60-76: Update the test
TestRemoveSignatureHelper_PreservesUserOwnedGpgSSHKeys to also assert that the
gpg format setting used for SSH signing is preserved: after calling
removeSignatureHelper(input) add an assertion that result contains `gpg.format =
ssh` (or the exact `format = ssh` line as emitted by your parser) so the test
fails if removeSignatureHelper strips the `format = ssh` entry; reference the
existing test name and the removeSignatureHelper function to locate where to add
this new assertion.
---
Outside diff comments:
In `@cmd/up.go`:
- Around line 386-400: The current code calls setupGitSSHSignature (triggered
when cmd.GitSSHSigningKey != "") before setupDotfiles, which allows dotfiles
installers to overwrite the Git config and remove the SSH signing setup; move
the conditional invocation of setupGitSSHSignature to run after setupDotfiles
completes (i.e., call setupDotfiles first and then, if cmd.GitSSHSigningKey !=
"", call setupGitSSHSignature using the same client and propagate any error),
preserving the existing error handling and return behavior so any failure in
setupGitSSHSignature still returns an error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 76bdf024-a9db-4435-b442-613156c916aa
📒 Files selected for processing (4)
cmd/agent/git_ssh_signature_test.gocmd/up.gopkg/gitsshsigning/helper.gopkg/gitsshsigning/helper_test.go
✅ Files skipped from review due to trivial changes (1)
- cmd/agent/git_ssh_signature_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/gitsshsigning/helper.go
Comment on lines
+60
to
+76
| 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]") | ||
| } |
There was a problem hiding this comment.
Add the gpg.format = ssh preservation regression to this test.
This fixture is the right place to prove user-owned SSH-signing config survives cleanup. Right now pkg/gitsshsigning/helper.go:195-200 drops every [gpg] format = ssh entry, so a config that keeps allowedSignersFile but relies on SSH signing still gets broken while this test passes.
Suggested test extension
func (s *HelperTestSuite) TestRemoveSignatureHelper_PreservesUserOwnedGpgSSHKeys() {
input := strings.Join([]string{
"[user]", "\tname = Test User",
+ "[gpg]", "\tformat = ssh",
`[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, "format = ssh",
+ "user-owned ssh signing format should be preserved")
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]")
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/gitsshsigning/helper_test.go` around lines 60 - 76, Update the test
TestRemoveSignatureHelper_PreservesUserOwnedGpgSSHKeys to also assert that the
gpg format setting used for SSH signing is preserved: after calling
removeSignatureHelper(input) add an assertion that result contains `gpg.format =
ssh` (or the exact `format = ssh` line as emitted by your parser) so the test
fails if removeSignatureHelper strips the `format = ssh` entry; reference the
existing test name and the removeSignatureHelper function to locate where to add
this new assertion.
e9d10ea to
edffef8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/up.go`:
- Around line 1575-1578: The helper is being invoked with the signingKey as the
last positional argv which git-ssh-signature.go expects to be the buffer file;
remove the appended signingKey from the shellescape.QuoteCommand call that
builds the "devpod agent git-ssh-signature-helper" invocation in cmd/up.go, and
instead persist the key via the installer path/flag that sets user.signingkey
(or add an explicit setup flag to the helper). Concretely: stop passing
signingKey as the final positional argument in the argv construction in
cmd/up.go, and either (A) call the installer code/path that writes
user.signingkey into config, or (B) extend the helper command
(cmd/agent/git_ssh_signature.go) to accept and parse an explicit
--setup-signing-key flag and use that for setup; update the caller accordingly
so the helper never treats the key as the Git buffer file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 84f7683a-fb99-4da8-a81b-7712385643ed
📒 Files selected for processing (3)
cmd/agent/git_ssh_signature_test.gocmd/up.gopkg/gitsshsigning/helper.go
✅ Files skipped from review due to trivial changes (2)
- pkg/gitsshsigning/helper.go
- cmd/agent/git_ssh_signature_test.go
Comment on lines
+1575
to
+1578
| "--command", | ||
| shellescape.QuoteCommand( | ||
| []string{"devpod", "agent", "git-ssh-signature-helper", signingKey}, | ||
| ), |
There was a problem hiding this comment.
This calls the runtime helper with the wrong argv shape.
cmd/agent/git_ssh_signature.go treats the last positional argument as the buffer file for a Git-invoked sign operation; it never reads a setup-time signing key from args. With signingKey appended here, devpod up --git-ssh-signing-key=... will try to sign signingKey as if it were the buffer file, so the helper setup fails before any Git config is installed. Please route the key through the installer path/flag that persists user.signingkey, or extend the helper command to accept an explicit signing-key argument for setup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/up.go` around lines 1575 - 1578, The helper is being invoked with the
signingKey as the last positional argv which git-ssh-signature.go expects to be
the buffer file; remove the appended signingKey from the
shellescape.QuoteCommand call that builds the "devpod agent
git-ssh-signature-helper" invocation in cmd/up.go, and instead persist the key
via the installer path/flag that sets user.signingkey (or add an explicit setup
flag to the helper). Concretely: stop passing signingKey as the final positional
argument in the argv construction in cmd/up.go, and either (A) call the
installer code/path that writes user.signingkey into config, or (B) extend the
helper command (cmd/agent/git_ssh_signature.go) to accept and parse an explicit
--setup-signing-key flag and use that for setup; update the caller accordingly
so the helper never treats the key as the Git buffer file.
This was referenced Mar 29, 2026
This was referenced Apr 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Tests