fix: resolve SSH signature key path for container-to-host forwarding (#645)#714
Merged
fix: resolve SSH signature key path for container-to-host forwarding (#645)#714
Conversation
When the signing request includes the public key content, Sign() writes it to a host-local temp file instead of using the container-local KeyPath which does not exist on the host.
The container-side client now reads the cert file and includes its content in the request body, so the host can create a temp file rather than relying on the container-local path.
Adds a second commit in the e2e test that configures user.signingkey as a file path (not inline key content), which exercises the code path that was broken for real users.
|
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 (1)
📝 WalkthroughWalkthroughClient-side signing requests now include the public key contents; the server can write that content to a temp key file for ssh-keygen. SSH agent forwarding failures are logged and treated non-fatal. Tests and e2e flows updated to exercise public-key-in-request and agent behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer / Git Client
participant Helper as git-ssh-signing (client)
participant Tunnel as Tunnel Server
participant Signer as Local Signer / ssh-keygen
participant Agent as ssh-agent
Dev->>Helper: invokes git-ssh program with buffer & cert path
Helper->>Helper: read cert file -> include PublicKey in request
Helper->>Tunnel: POST GitSSHSignatureRequest (Content, KeyPath, PublicKey)
Tunnel->>Tunnel: deserialize request
Tunnel->>Signer: resolveKeyFile(): if PublicKey -> write temp key file
Tunnel->>Agent: (optional) use SSH agent if available
Tunnel->>Signer: run `ssh-keygen -Y sign -f <resolvedKey> -n git` with Content
Signer-->>Tunnel: signed payload or stderr error
Tunnel-->>Helper: response (signed blob or error)
Helper-->>Dev: pass signed result to Git
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
🚥 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 |
ssh-keygen -Y sign -f <path> reads the public key directly from the given path. The previous .pub suffix stripping was incorrect — ssh-keygen does not auto-append .pub for agent-based signing.
user.signingkey inside the container is a host path, not key content. Use ssh-add -L to get the actual public key from the forwarded agent.
The e2e test generates a key with ssh-keygen but never adds it to the SSH agent. This means ssh-add -L inside the container returns nothing, the public key file is empty, and the host falls back to the container path which doesn't exist.
CI runners don't have an SSH agent running, so ssh-add fails. Start a dedicated agent, parse its output for SSH_AUTH_SOCK and SSH_AGENT_PID, and clean up on test completion.
The SSH agent cleanup kills the agent process but leaves SSH_AUTH_SOCK set in the process env. Subsequent ordered tests inherit the stale socket path, causing devpod up to fatally fail when it tries to forward the dead agent.
Replace manual os.Setenv + os.Unsetenv with GinkgoT().Setenv which automatically restores the original value when the spec completes, preventing stale SSH_AUTH_SOCK from leaking into subsequent tests.
SSH_AUTH_SOCK commonly points to stale sockets (tmux, screen, reconnected terminals). Following OpenSSH's behavior, log a warning and continue rather than fatally aborting devpod up. Agent forwarding is optional — nothing in the tunnel lifecycle depends on it.
Reference the specific OpenSSH source (clientloop.c, ssh_config(5)) that establishes agent forwarding failures as non-fatal, and explain why stale SSH_AUTH_SOCK is common in practice.
Remove redundant assertion messages, obvious comments, and over-explained test descriptions. Keep only comments that add context not evident from the code itself.
Owner
Author
|
@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 (2)
pkg/credentials/integration_test.go (1)
106-107: Don’t discard marshal errors in the test helper path.Even in tests, checking this error gives clearer failures if response shape changes later.
Proposed test-hygiene fix
- jsonBytes, _ := json.Marshal(sig) + jsonBytes, err := json.Marshal(sig) + require.NoError(t, err) return &tunnel.Message{Message: string(jsonBytes)}, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/credentials/integration_test.go` around lines 106 - 107, The test helper currently discards the error from json.Marshal(sig); change it to capture the error (jsonBytes, err := json.Marshal(sig)) and if err != nil return an error instead of returning a malformed Message (e.g., return nil, fmt.Errorf("marshal sig: %w", err)); update the surrounding function (the helper that returns (*tunnel.Message, error)) to propagate that error and add the necessary import (fmt) if missing so test failures show marshal problems instead of silently producing bad JSON in tunnel.Message.e2e/tests/ssh/ssh.go (1)
158-163: Use the parsed agent PID directly in cleanup.Reading
SSH_AGENT_PIDfrom env during cleanup is less robust than capturing PID once and reusing it.Proposed cleanup hardening
t := ginkgo.GinkgoT() + var agentPID string for line := range strings.SplitSeq(string(agentOut), "\n") { for _, prefix := range []string{"SSH_AUTH_SOCK=", "SSH_AGENT_PID="} { if _, after, ok := strings.Cut(line, prefix); ok { val := after if semi := strings.Index(val, ";"); semi >= 0 { val = val[:semi] } key := prefix[:len(prefix)-1] + if key == "SSH_AGENT_PID" { + agentPID = val + } t.Setenv(key, val) } } } ginkgo.DeferCleanup(func(_ context.Context) { - if pid := os.Getenv("SSH_AGENT_PID"); pid != "" { + if agentPID != "" { // `#nosec` G204 -- controlled pid from ssh-agent we started - _ = exec.Command("kill", pid).Run() + _ = exec.Command("kill", agentPID).Run() } })🤖 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 158 - 163, Capture the ssh-agent PID once when you start the agent (e.g., store it in a local variable like sshAgentPID or parsedAgentPID) and use that stored value inside the ginkgo.DeferCleanup closure instead of calling os.Getenv("SSH_AGENT_PID") at cleanup time; parse to an int (strconv.Atoi) when you capture it to validate it and then call exec.Command("kill", strconv.Itoa(parsedAgentPID)).Run() (or syscall.Kill) in the existing ginkgo.DeferCleanup anonymous function to ensure you always kill the same agent you started and avoid reading the environment at teardown.
🤖 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 158-163: Capture the ssh-agent PID once when you start the agent
(e.g., store it in a local variable like sshAgentPID or parsedAgentPID) and use
that stored value inside the ginkgo.DeferCleanup closure instead of calling
os.Getenv("SSH_AGENT_PID") at cleanup time; parse to an int (strconv.Atoi) when
you capture it to validate it and then call exec.Command("kill",
strconv.Itoa(parsedAgentPID)).Run() (or syscall.Kill) in the existing
ginkgo.DeferCleanup anonymous function to ensure you always kill the same agent
you started and avoid reading the environment at teardown.
In `@pkg/credentials/integration_test.go`:
- Around line 106-107: The test helper currently discards the error from
json.Marshal(sig); change it to capture the error (jsonBytes, err :=
json.Marshal(sig)) and if err != nil return an error instead of returning a
malformed Message (e.g., return nil, fmt.Errorf("marshal sig: %w", err)); update
the surrounding function (the helper that returns (*tunnel.Message, error)) to
propagate that error and add the necessary import (fmt) if missing so test
failures show marshal problems instead of silently producing bad JSON in
tunnel.Message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d206d36-9a94-4233-92f6-c53500c9b099
📒 Files selected for processing (8)
e2e/tests/ssh/ssh.gopkg/agent/tunnelserver/tunnelserver_gitssh_test.gopkg/credentials/integration_test.gopkg/devcontainer/sshtunnel/sshtunnel.gopkg/gitsshsigning/client.gopkg/gitsshsigning/client_test.gopkg/gitsshsigning/server.gopkg/gitsshsigning/server_test.go
Capture SSH agent PID at parse time instead of reading env at cleanup. Check json.Marshal error in test helper instead of discarding it.
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 because the container-local temp key path (
/tmp/.git_signing_key_tmpXXXXXX) is forwarded verbatim to the host'sSign()function, wheressh-keygencan't find it.Core fix: public key content forwarding
client.go): reads the public key file content and includes it asPublicKeyin the signature requestserver.go):Sign()callsresolveKeyFile()which writes thePublicKeycontent to a host-local temp file before invokingssh-keygenPublicKeyis empty, falls back toKeyPathNon-fatal SSH agent forwarding
sshtunnel.go): agent forwarding failure is now logged as a warning instead of fatally abortingdevpod upclientloop.creturns NULL on agent failure without terminating the session;ExitOnForwardFailureinssh_config(5)explicitly excludes agent forwardingSSH_AUTH_SOCKis common in practice (tmux, screen, reconnected terminals)Test coverage
Sign()withPublicKeycontent and non-existent key pathsPublicKeyflows through the tunnelPublicKeydeserializationSummary by CodeRabbit
New Features
Bug Fixes
Tests