fix: resolve Git SSH signature forwarding failures (#645)#704
Merged
fix: resolve Git SSH signature forwarding failures (#645)#704
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughMade Git SSH signature helper setup non-fatal, threaded an explicit GitSSHSigningKey through CLI and service startup flows, refactored gitconfig helper logic, converted request-body read errors to JSON 500 responses, and added tests and e2e adjustments for SSH signing flows. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(52,152,219,0.5)
participant Client
end
rect rgba(46,204,113,0.5)
participant CredentialsServer
end
rect rgba(155,89,182,0.5)
participant Tunnel
end
rect rgba(241,196,15,0.5)
participant Container
end
Client->>CredentialsServer: POST /git-ssh-signature (with pubkey)
CredentialsServer->>CredentialsServer: read request body
alt read error
CredentialsServer-->>Client: HTTP 500 JSON {"error": "..."}
else successful
CredentialsServer->>Tunnel: request helper install/configure (includes GitSSHSigningKey)
Tunnel->>Container: run credentials helper (may be async)
Container-->>Tunnel: helper install result (async)
Tunnel-->>CredentialsServer: install/config result
CredentialsServer-->>Client: HTTP 200 JSON success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
If you're new to commit signing, there are different ways to set it up: Sign commits with
|
skevetter
changed the title
The `setupGitSSHSignature` function in `cmd/up.go` opened an ephemeral SSH session to configure the git signing helper inside the container. This raced with the main tunnel's port forwarding, causing "use of closed network connection" errors that blocked the entire workspace startup. The credentials server tunnel (`credentials_server.go`) already calls `gitsshsigning.ConfigureHelper` with the base64-decoded signing key, making `setupGitSSHSignature` redundant. Removing it eliminates the race condition and the cascade of failures users reported: - Port forwarding errors on first start - "No such file" for the signing key (key never written due to setup failure) - JSON decode errors (non-JSON error responses from the signature endpoint) Also fixes the `/git-ssh-signature` HTTP handler to always return JSON error responses, even when the request body cannot be read. Closes #645
The existing E2E tests used a minimal devcontainer without forwardPorts, so they never triggered the race condition between setupGitSSHSignature's ephemeral SSH session and the main tunnel's port forwarding. This test uses a devcontainer with forwardPorts: [8300, 7080] to verify that signing works on first `devpod up` — the exact scenario that failed for the issue reporter.
The credentials server only received the SSH signing key via ExtractGitConfiguration() which reads the host's .gitconfig. When a user passes --git-ssh-signing-key on the CLI but doesn't have user.signingkey in their host git config, the credentials server never received the key — making signing silently fail. Add GitSSHSigningKey field to RunServicesOptions and thread it from the UpCmd through the IDE browser launchers into startBrowserTunnel. The addGitSSHSigningKey function now prefers the explicit key over host .gitconfig extraction, falling back only when no explicit key is provided. This was the missing piece identified from the prior issue-645-analysis branch work that was never merged to main. Part of #645
- Thread GitSSHSigningKey through cmd/ssh.go startServices into RunServicesOptions so standalone SSH reconnections also forward the explicit signing key - Make ConfigureHelper failure non-fatal in credentials_server.go so signing setup failures don't crash the entire credentials server - Fix updateGitConfig idempotency: always remove+rewrite signing config instead of skipping when program line already exists - Update helper_test.go assertions for the new idempotent behavior Part of #645
- Refactor removeSignatureHelper to buffer [gpg] and [user] sections the same way as [gpg "ssh"], dropping empty sections after filtering. Previously left behind empty [gpg] headers and stale signingkey lines, causing TestUpdateGitConfig_Idempotent and _DifferentKey to fail in CI. - Extract repeated test command string to constant (goconst lint fix) Part of #645
…sting testdata Instead of a separate duplicate test for the #645 forwardPorts race condition, add forwardPorts to the existing ssh-signing devcontainer testdata so the main signing test exercises the harder case by default. Removes ~120 lines of duplicated test code and the redundant ssh-signing-with-ports testdata directory. Part of #645
The helper script and git config are only available when the credentials server is running. Move verification checks into the same SSH session that has --start-services active instead of using bare DevPodSSH calls. Part of #645
skevetter
force-pushed
the
investigate-issue-645
branch
from
b1f6af9 to
867cad8
Compare
The signing helper is installed by the credentials server which starts asynchronously via --start-services. Add a retry loop (up to 30s) before checking for the helper binary. Part of #645
The signing helper is configured by the credentials server, which needs the key passed on each SSH invocation. Without it, the credentials server has no key to configure and the helper is never installed. Part of #645
![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
🧹 Nitpick comments (2)
pkg/gitsshsigning/helper.go (1)
178-181: Consider: removing allsigningkeyentries may overwrite user's non-workspace keys.The predicate removes any line starting with
signingkey =in[user]sections, not just devpod-managed ones. If a user has a manually configured signing key in their.gitconfigbefore devpod setup, it will be replaced.This is likely intentional given the PR's goal (ensure CLI key reaches the workspace), but worth documenting that devpod takes ownership of the signing key configuration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/gitsshsigning/helper.go` around lines 178 - 181, The code currently removes every "signingkey = " line inside the sectionUser branch by calling filterSection with a predicate matching strings.HasPrefix(trimmed, "signingkey = "), which can overwrite users' pre-existing non-devpod keys; update the logic in the sectionUser handling (the filterSection invocation) to only remove signingkey entries that are managed by devpod (e.g., detect a devpod-specific marker/comment near the key, or match a known devpod key ID pattern) or else preserve existing signingkey lines and only append/override when a devpod marker is present; reference the sectionUser case and the filterSection call to locate where to change the predicate and add marker detection or a non-destructive merge so user-managed signingkey entries are not unconditionally removed.pkg/tunnel/services_test.go (1)
22-31: Consider usingContainsinstead ofHasSuffixfor a more resilient assertion.The
HasSuffixassertion assumes the signing key flag will always be the last argument. If the implementation changes to add additional flags after the signing key, this test would fail even though the behavior is correct.Suggested change
func TestAddGitSSHSigningKey_ExplicitKeyTakesPrecedence(t *testing.T) { // When an explicit key is provided, it should be used regardless // of what ExtractGitConfiguration might return from host .gitconfig. command := testBaseCommand explicitKey := "/explicit/key.pub" result := addGitSSHSigningKey(command, explicitKey, log.Discard) encoded := base64.StdEncoding.EncodeToString([]byte(explicitKey)) - assert.True(t, strings.HasSuffix(result, "--git-user-signing-key "+encoded)) + assert.Contains(t, result, "--git-user-signing-key "+encoded) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tunnel/services_test.go` around lines 22 - 31, Replace the brittle suffix check in TestAddGitSSHSigningKey_ExplicitKeyTakesPrecedence with a containment check: instead of asserting strings.HasSuffix(result, "--git-user-signing-key "+encoded) use an assertion that result contains the signing-key flag and encoded value (e.g., assert.Contains or strings.Contains). Update the test referencing addGitSSHSigningKey to verify the presence of "--git-user-signing-key "+encoded anywhere in result so additional flags after the key won't break the test.
🤖 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 67-73: The test comment is incorrect: `-O` isn't a boolean flag
and Git doesn't pass it; update the TestParseMultipleUnknownFlags test (and its
args slice) to reflect a real git invocation or remove the test entirely.
Specifically, modify the args used in TestParseMultipleUnknownFlags (which calls
parseSSHKeygenArgs) to only include flags Git can pass (e.g., "ssh-keygen -Y
sign -n git -f /key.pub -U /tmp/buf" or use an unknown boolean like a
placeholder flag) and change the comment to state that the test ensures unknown
boolean flags (such as -U) don't prevent finding certPath and bufferFile, or
delete the test if you prefer not to be defensive. Ensure the assertions against
result.certPath and result.bufferFile remain unchanged.
---
Nitpick comments:
In `@pkg/gitsshsigning/helper.go`:
- Around line 178-181: The code currently removes every "signingkey = " line
inside the sectionUser branch by calling filterSection with a predicate matching
strings.HasPrefix(trimmed, "signingkey = "), which can overwrite users'
pre-existing non-devpod keys; update the logic in the sectionUser handling (the
filterSection invocation) to only remove signingkey entries that are managed by
devpod (e.g., detect a devpod-specific marker/comment near the key, or match a
known devpod key ID pattern) or else preserve existing signingkey lines and only
append/override when a devpod marker is present; reference the sectionUser case
and the filterSection call to locate where to change the predicate and add
marker detection or a non-destructive merge so user-managed signingkey entries
are not unconditionally removed.
In `@pkg/tunnel/services_test.go`:
- Around line 22-31: Replace the brittle suffix check in
TestAddGitSSHSigningKey_ExplicitKeyTakesPrecedence with a containment check:
instead of asserting strings.HasSuffix(result, "--git-user-signing-key
"+encoded) use an assertion that result contains the signing-key flag and
encoded value (e.g., assert.Contains or strings.Contains). Update the test
referencing addGitSSHSigningKey to verify the presence of
"--git-user-signing-key "+encoded anywhere in result so additional flags after
the key won't break the test.
🪄 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: ba39b906-a2e4-4e9e-87c8-55b5ec2e1bfb
📒 Files selected for processing (12)
cmd/agent/container/credentials_server.gocmd/agent/git_ssh_signature_test.gocmd/ssh.gocmd/up.goe2e/tests/ssh/ssh.goe2e/tests/ssh/testdata/ssh-signing/devcontainer.jsonpkg/credentials/server.gopkg/credentials/server_test.gopkg/gitsshsigning/helper.gopkg/gitsshsigning/helper_test.gopkg/tunnel/services.gopkg/tunnel/services_test.go
- Rename TestParseMultipleUnknownFlags to TestParseWithUFlag and remove -O flag (not a boolean flag, never passed by git). Test now reflects the actual git signing invocation: ssh-keygen -Y sign -n git -f <key> -U <buf> - Replace HasSuffix/HasPrefix assertions with Contains for resilience - Remove unused strings import Part of #645
removeSignatureHelper was stripping all signingkey lines from [user] sections, which would permanently delete a user's pre-existing signing key (e.g., GPG key) on workspace startup or teardown. Now only drops [user] sections that contain nothing but signingkey entries — these are the devpod-appended ones from GitConfigTemplate. User-owned sections with other entries (name, email) are preserved intact.
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)
pkg/gitsshsigning/helper.go (1)
55-55:⚠️ Potential issue | 🟡 MinorUse
%vinstead of%wfor logging errors.
%wis the error-wrapping verb forfmt.Errorf, not for logging functions. In logging contexts,%vshould be used to format the error value. The context snippet fromcredentials_server.gocorrectly uses%vin similar error logging calls.Proposed fix
- log.Errorf("Failed updating git configuration: %w", err) + log.Errorf("Failed updating git configuration: %v", err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/gitsshsigning/helper.go` at line 55, Replace the incorrect use of the fmt error-wrapping verb in the log call: update the log.Errorf invocation in pkg/gitsshsigning/helper.go (the call that currently reads log.Errorf("Failed updating git configuration: %w", err)) to use the logging-friendly verb %v instead of %w so the error is formatted correctly (i.e., change the format string to use %v and keep passing err).
🧹 Nitpick comments (1)
pkg/tunnel/services_test.go (1)
13-19: Strengthen explicit-key assertions to exact command equality.Using
assert.Containshere can miss regressions like duplicate flags or unexpected suffixes. Prefer asserting the full expected command string.Suggested change
func TestAddGitSSHSigningKey_ExplicitKey(t *testing.T) { command := testBaseCommand result := addGitSSHSigningKey(command, "/path/to/key.pub", log.Discard) encoded := base64.StdEncoding.EncodeToString([]byte("/path/to/key.pub")) - assert.Contains(t, result, "--git-user-signing-key "+encoded) + assert.Equal(t, command+" --git-user-signing-key "+encoded, result) } func TestAddGitSSHSigningKey_ExplicitKeyTakesPrecedence(t *testing.T) { @@ result := addGitSSHSigningKey(command, explicitKey, log.Discard) encoded := base64.StdEncoding.EncodeToString([]byte(explicitKey)) - assert.Contains(t, result, "--git-user-signing-key "+encoded) + assert.Equal(t, command+" --git-user-signing-key "+encoded, result) }Also applies to: 21-30
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tunnel/services_test.go` around lines 13 - 19, The test TestAddGitSSHSigningKey_ExplicitKey uses assert.Contains which can miss duplicates or extra text; update it to assert exact equality of the returned command from addGitSSHSigningKey by computing the base64-encoded key string (same as encoded := base64.StdEncoding.EncodeToString([]byte("/path/to/key.pub"))) and comparing result to the full expected command string (e.g. "--git-user-signing-key "+encoded) with assert.Equal; make the same change for the other similar test(s) around lines 21-30 so each test checks the exact command output rather than using assert.Contains.
🤖 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/tunnel/services_test.go`:
- Around line 32-43: The test currently only asserts that result contains
testBaseCommand, which passes whether the host appended a key or not; make the
behavior deterministic by ensuring no host .gitconfig is present: in
TestAddGitSSHSigningKey_EmptyExplicitKey_FallsBackToHostConfig create a
temporary HOME (or temp dir) with no .gitconfig, set os.Setenv("HOME", tmpDir)
before calling addGitSSHSigningKey(testBaseCommand, "", log.Discard), then
restore HOME and assert the result equals testBaseCommand (not just contains);
reference testBaseCommand, addGitSSHSigningKey, and
TestAddGitSSHSigningKey_EmptyExplicitKey_FallsBackToHostConfig when locating the
change.
---
Outside diff comments:
In `@pkg/gitsshsigning/helper.go`:
- Line 55: Replace the incorrect use of the fmt error-wrapping verb in the log
call: update the log.Errorf invocation in pkg/gitsshsigning/helper.go (the call
that currently reads log.Errorf("Failed updating git configuration: %w", err))
to use the logging-friendly verb %v instead of %w so the error is formatted
correctly (i.e., change the format string to use %v and keep passing err).
---
Nitpick comments:
In `@pkg/tunnel/services_test.go`:
- Around line 13-19: The test TestAddGitSSHSigningKey_ExplicitKey uses
assert.Contains which can miss duplicates or extra text; update it to assert
exact equality of the returned command from addGitSSHSigningKey by computing the
base64-encoded key string (same as encoded :=
base64.StdEncoding.EncodeToString([]byte("/path/to/key.pub"))) and comparing
result to the full expected command string (e.g. "--git-user-signing-key
"+encoded) with assert.Equal; make the same change for the other similar test(s)
around lines 21-30 so each test checks the exact command output rather than
using assert.Contains.
🪄 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: ed66af5a-1f67-42a4-89f4-35efcfef5a11
📒 Files selected for processing (5)
cmd/agent/git_ssh_signature_test.gopkg/credentials/server.gopkg/gitsshsigning/helper.gopkg/gitsshsigning/helper_test.gopkg/tunnel/services_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/credentials/server.go
- Use %v instead of %w in log.Errorf (helper.go:55) - Tighten test assertions from Contains to Equal for exact command matching - Make fallback test deterministic by isolating HOME env
This was referenced Apr 12, 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
Fixes #645 — Git SSH signature forwarding fails on workspace startup.
Root causes
Race condition:
setupGitSSHSignatureincmd/up.goopened a separate SSH session to configure the signing helper. This raced with the main tunnel's port forwarding, causing setup failures when ports closed mid-session.Missing key threading: The credentials server received the signing key from the host
.gitconfigviaExtractGitConfiguration(). When users passed--git-ssh-signing-keywithoutuser.signingkeyin their host git config, the key never reached the credentials server.Fix
setupGitSSHSignature— the credentials server tunnel configures the signing helper viagitsshsigning.ConfigureHelper, making this function redundantGitSSHSigningKeythroughRunServicesOptionsso the explicit CLI key flows fromUpCmd→ IDE launchers →startBrowserTunnel→ credentials server. Falls back to host.gitconfigwhen no explicit key is providedGitSSHSigningKeythroughcmd/ssh.gostartServicesfor standalone SSH reconnectionsConfigureHelperfailure non-fatal so signing setup errors don't crash the credentials serverupdateGitConfigto replace stale signing config on every call instead of skipping when a program line exists/git-ssh-signatureendpoint on body-read failuresSummary by CodeRabbit
New Features
Bug Fixes
Tests
Other