fix: GPG agent forwarding crash with SSH signing keys (#731)#732
fix: GPG agent forwarding crash with SSH signing keys (#731)#732
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 6 minutes and 58 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors GPG/git signing-key detection into a helper and makes git signing-key setup non-fatal during agent workspace setup; adds unit tests for signing-key detection and an e2e test for GPG agent forwarding with SSH-format signing keys. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 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 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
force-pushed
the
investigate-issue-731
branch
from
c6b235a to
1c85086
Compare
skevetter
force-pushed
the
investigate-issue-731
branch
from
1c85086 to
07eff03
Compare
When the user has SSH-based commit signing configured (gpg.format=ssh), the GPG agent forwarding code no longer passes the SSH key path as --gitkey to setup-gpg. SSH signing keys are handled by the separate SSH signature helper path.
Setting user.signingKey in the container is optional — if it fails, GPG agent forwarding and the SSH server should still start. This prevents a bad signing key configuration from tearing down the entire tunnel.
Validates that workspace starts successfully when GPG agent forwarding is enabled and the host has gpg.format=ssh with an SSH signing key. This is the exact scenario reported in issue #731.
skevetter
force-pushed
the
investigate-issue-731
branch
from
07eff03 to
52edd73
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/tests/ssh/ssh.go (1)
88-90: Consider usingginkgo.DeferCleanupfor consistent cleanup patterns.The test uses
defer os.RemoveAllforsshKeyDirbutginkgo.GinkgoT().TempDir()forgitConfigDir. For consistency with the workspace cleanup pattern (lines 81-84) and better integration with Ginkgo's lifecycle, consider usingDeferCleanup:♻️ Suggested refactor for consistent cleanup
sshKeyDir, err := os.MkdirTemp("", "devpod-gpg-ssh-test") framework.ExpectNoError(err) - defer func() { _ = os.RemoveAll(sshKeyDir) }() + ginkgo.DeferCleanup(func() { _ = os.RemoveAll(sshKeyDir) })🤖 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 88 - 90, The test creates sshKeyDir via os.MkdirTemp and uses defer os.RemoveAll for cleanup; replace that defer with a ginkgo.DeferCleanup call so cleanup participates in Ginkgo's lifecycle. After creating sshKeyDir (the variable assigned by os.MkdirTemp and validated by framework.ExpectNoError), call ginkgo.DeferCleanup with either a closure that calls os.RemoveAll(sshKeyDir) or DeferCleanup(os.RemoveAll, sshKeyDir) to mirror the gitConfigDir pattern and ensure consistent cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/tests/ssh/ssh.go`:
- Around line 88-90: The test creates sshKeyDir via os.MkdirTemp and uses defer
os.RemoveAll for cleanup; replace that defer with a ginkgo.DeferCleanup call so
cleanup participates in Ginkgo's lifecycle. After creating sshKeyDir (the
variable assigned by os.MkdirTemp and validated by framework.ExpectNoError),
call ginkgo.DeferCleanup with either a closure that calls
os.RemoveAll(sshKeyDir) or DeferCleanup(os.RemoveAll, sshKeyDir) to mirror the
gitConfigDir pattern and ensure consistent cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 64be66fb-7fed-4237-a582-961e01631a88
📒 Files selected for processing (4)
cmd/agent/workspace/setup_gpg.gocmd/ssh.gocmd/ssh_test.goe2e/tests/ssh/ssh.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/tests/ssh/ssh.go (1)
64-114: Well-structured test that correctly verifies the fix for issue#731.The test appropriately:
- Skips on Windows where GPG forwarding behavior differs.
- Sets up an isolated environment with a temporary SSH key and git config.
- Configures
gpg.format = sshto trigger the code path that previously caused the crash.- Verifies SSH connectivity works after
DevPodUpwith--gpg-agent-forwarding.One minor inconsistency: you use
os.MkdirTemp(line 86) with explicitDeferCleanupfor the SSH key directory, butGinkgoT().TempDir()(line 97) for the git config directory (which auto-cleans). Consider usingGinkgoT().TempDir()consistently for both to simplify cleanup handling.🤖 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 64 - 114, The test mixes os.MkdirTemp (sshKeyDir) with ginkgo.GinkgoT().TempDir() (gitConfigDir); switch sshKeyDir creation to ginkgo.GinkgoT().TempDir() instead of os.MkdirTemp, remove the explicit ginkgo.DeferCleanup that calls os.RemoveAll(sshKeyDir), and keep the rest of the logic (keyPath, ssh-keygen call, and git config handling) unchanged so cleanup is consistently managed by GinkgoT().TempDir(); update references to sshKeyDir, os.MkdirTemp, ginkgo.GinkgoT().TempDir(), ginkgo.DeferCleanup, and os.RemoveAll accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/tests/ssh/ssh.go`:
- Around line 64-114: The test mixes os.MkdirTemp (sshKeyDir) with
ginkgo.GinkgoT().TempDir() (gitConfigDir); switch sshKeyDir creation to
ginkgo.GinkgoT().TempDir() instead of os.MkdirTemp, remove the explicit
ginkgo.DeferCleanup that calls os.RemoveAll(sshKeyDir), and keep the rest of the
logic (keyPath, ssh-keygen call, and git config handling) unchanged so cleanup
is consistently managed by GinkgoT().TempDir(); update references to sshKeyDir,
os.MkdirTemp, ginkgo.GinkgoT().TempDir(), ginkgo.DeferCleanup, and os.RemoveAll
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8ee99ac1-ad6c-451a-a347-32fa42cf5547
📒 Files selected for processing (1)
e2e/tests/ssh/ssh.go
1 participant
--gitkeytosetup-gpgwhengpg.format = ssh, since SSH signing keys are handled by the separate SSH signature helper pathsetup-gpgso a failure doesn't tear down the entire tunnel and kill the SSH serverFixes #731
Summary by CodeRabbit
Bug Fixes
Tests