fix(gpg): passthrough gpg-tunnel STDERR - #838
Conversation
gpg -K against an uninitialized ~/.gnupg on the remote emits benign first-run notices (directory/keybox/trustdb created) to stderr. These were logged at error level, and #797 made Writer route lines through the structured encoder, so they now render as visible ERROR log lines on every ssh even though IsGpgTunnelRunning already reports success via its returned bool.
✅ Deploy Preview for devsydev canceled.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded ChangesGPG stderr forwarding
Estimated code review effort: 2 (Simple) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
✅ Deploy Preview for images-devsy-sh canceled.
|
✅ Deploy Preview for images-devsy-sh canceled.
|
Writer always routes lines through the structured encoder (levels, timestamps, JSON/logfmt). Some callers need a subprocess's own output to reach the user verbatim instead of being wrapped as a log line.
Pull request was converted to draft
Trim comments on PassthroughWriter.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/log/writer_test.go (1)
136-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the primary stderr destination.
This test checks only the extra sink. It does not verify
os.Stderr, which is the primary destination used byPassthroughWriter. A regression that removes or formats the stderr write would still pass. Capture or inject the stderr destination and assertraw output\nthere as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/log/writer_test.go` around lines 136 - 151, Update TestPassthroughWriter_WritesRawBytesUnformatted to capture or inject the os.Stderr destination used by PassthroughWriter, then assert it receives exactly “raw output\n” in addition to the existing sink assertion. Preserve the test’s verification that output remains unformatted raw bytes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/gpg/gpg_forwarding.go`:
- Line 32: Update the writer used by IsGpgTunnelRunning near PassthroughWriter
so local stderr write failures cannot propagate as the probe’s tunnel-status
error. Use an error-discarding writer for that stream while preserving remote
command or SSH failures, ensuring IsGpgTunnelRunning returns false only for
actual tunnel/query failures.
---
Nitpick comments:
In `@pkg/log/writer_test.go`:
- Around line 136-151: Update TestPassthroughWriter_WritesRawBytesUnformatted to
capture or inject the os.Stderr destination used by PassthroughWriter, then
assert it receives exactly “raw output\n” in addition to the existing sink
assertion. Preserve the test’s verification that output remains unformatted raw
bytes.
🪄 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 Plus
Run ID: 2bf45d4c-f309-48d9-aefb-b4a4fbd49179
📒 Files selected for processing (3)
pkg/gpg/gpg_forwarding.gopkg/log/writer.gopkg/log/writer_test.go
| client *ssh.Client, | ||
| ) bool { | ||
| writer := log.Writer(log.LevelError) | ||
| writer := log.PassthroughWriter() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -A 50 -B 8 \
'func Run|sess\.Stderr|sess\.Run|type RunOptions' \
--glob '*.go' .
rg -n -A 25 -B 5 \
'IsGpgTunnelRunning|PassthroughWriter' \
--glob '*.go' .Repository: devsy-org/devsy
Length of output: 50371
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== candidate files =="
fd -a 'gpg_forwarding.go|helper.go|devssh|PassthroughWriter|ssh/helper.go' . | sed 's#^\./##'
echo "== gpg_forwarding excerpt =="
if [ -f pkg/gpg/gpg_forwarding.go ]; then
nl -ba pkg/gpg/gpg_forwarding.go | sed -n '1,90p'
fi
echo "== ssh helper Run excerpt =="
if [ -f pkg/ssh/helper.go ]; then
nl -ba pkg/ssh/helper.go | sed -n '100,180p'
fi
echo "== log PassthroughWriter definitions/usages =="
rg -n -A 10 -B 5 'PassthroughWriter|type.*Writer|func .*Write' pkg log --glob '*.go'Repository: devsy-org/devsy
Length of output: 927
🌐 Web query:
github.com/gliderlabs/ssh Session Stderr Run stream copy error documentation
💡 Result:
The gliderlabs/ssh package does not have a specific "Stderr Run stream copy error" built into the library itself; rather, common issues involving Stderr, Run, and stream copying are typically related to Go's io management or how the underlying ssh.Session is configured [1][2][3]. When using gliderlabs/ssh, the Session interface provides access to standard streams (Stdin, Stdout, Stderr). If you encounter errors during stream operations (such as copying data), they often stem from these common causes: 1. Improper Stream Setup: The Session interface acts as a wrapper around an SSH channel [1]. If you call Run without properly configuring Stdout or Stderr, or if the process running on the server expects input (Stdin) that is not provided, the stream may hit an EOF or block indefinitely [1][2]. 2. Blocking IO: If you use io.Copy for streams without considering that the session might close or the underlying process might terminate, you may trigger errors or blocking behavior [2][4]. 3. Stream Handling Logic: In the server-side Handler, you must manually manage the association between the Session's streams and your command's streams [4]. For example, if you are spawning a process, you must assign its Stdout/Stderr/Stdin to the session object explicitly [2]. Best practices for debugging and implementing stream copying: - Verify that your server-side handler correctly assigns the Session's streams to the command you are running: cmd.Stdout = s; cmd.Stderr = s.Stderr [5][2]. - Use the provided test examples in the repository as a reference for correct stream redirection, specifically looking at how TestStderr and TestStdout are implemented [5]. - If you are building a PTY-enabled session, ensure you are using io.Copy correctly to pipe the PTY's output to the Session and vice versa [4]. For specific error messages (e.g., EOF, broken pipe), check if the client-side session is closed before the server-side has finished processing the stream [3]. Ensure you are not exiting the handler (e.g., via s.Exit) before the copy operations are complete [6][3].
Citations:
- 1: How to open a program with password entering on a remote host when I have connected the remote host by ssh？ gliderlabs/ssh#84
- 2: Windows - Process keeps running when stdin is set gliderlabs/ssh#219
- 3: Problem with non-interactive command handle gliderlabs/ssh#212
- 4: https://github.com/gliderlabs/ssh/blob/master/_examples/ssh-pty/pty.go
- 5: https://github.com/gliderlabs/ssh/blob/master/session_test.go
- 6: https://github.com/gliderlabs/ssh/blob/master/session.go
🌐 Web query:
github.com/gliderlabs/ssh Session client-side Run Stderr copy error "ssh.Session" "Stderr.Run()"
💡 Result:
The gliderlabs/ssh package wraps the golang.org/x/crypto/ssh package to provide a high-level API for SSH servers [1][2]. When encountering issues with ssh.Session Stderr handling, it is important to distinguish between server-side implementation and client-side consumption, as gliderlabs/ssh is primarily for building SSH servers [1][3]. On the server side (using gliderlabs/ssh), the Session interface provides access to Stderr via the s.Stderr method, which returns an io.Writer [4][5]. You can write output to this writer to send it to the client [5]. On the client side (using the standard golang.org/x/crypto/ssh library), the Session.Run method manages the execution of commands [6]. If you encounter errors during execution or stream copying, note the following: 1. Standard Stream Binding: Before calling Run, you must explicitly set the Session's Stderr field to an io.Writer (e.g., os.Stderr or a bytes.Buffer) [7][6][5]. If it remains nil, the library automatically directs output to io.Discard [6]. 2. Blocking Behavior: The client-side session shares a fixed amount of buffering between Stdout and Stderr [6]. If the reader for Stderr or Stdout is not serviced fast enough (or is not read at all), it can cause the remote command to block [6]. 3. Execution Errors: The error returned by Session.Run is nil only if the command runs, completes standard stream copying without issues, and exits with a zero status [6]. If I/O problems occur during the copying of Stdin, Stdout, or Stderr, these will be returned as errors from Run [6]. To debug issues with Stderr copying or Run failures: - Ensure the client has correctly assigned an io.Writer to the session's Stderr field before calling Run [7][6]. - If you are running complex commands or non-interactive tasks, verify that the remote shell environment is not blocking due to unconsumed output [6]. - If you suspect an issue with the server-side implementation, ensure your server handler is properly writing to the session's Stderr channel [5]. For further diagnosis, verify that your client-side code is not encountering an ExitError or an I/O error by checking the specific type of the returned error from Run [6].
Citations:
- 1: https://pkg.go.dev/github.com/gliderlabs/ssh
- 2: https://github.com/gliderlabs/ssh/blob/master/README.md
- 3: https://github.com/gliderlabs/ssh/blob/master/doc.go
- 4: https://github.com/tailscale/tailscale/blob/v1.100.0/ssh/tailssh/session.go
- 5: https://github.com/gliderlabs/ssh/blob/master/session_test.go
- 6: https://github.com/golang/crypto/blob/master/ssh/session.go
- 7: Problem with non-interactive command handle gliderlabs/ssh#212
🌐 Web query:
golang.org/x/crypto/ssh Session Stderr io.Copy session Run stream error
💡 Result:
In the golang.org/x/crypto/ssh package, Session.Run combines Start and Wait into a single operation [1][2]. When using Run, Start, or Wait, you must be aware of how the package handles I/O streams (Stdin, Stdout, Stderr) and potential blocking conditions [1][2]. If you are encountering a "stream error" or a hang when using io.Copy in conjunction with ssh.Session, it is typically due to one of the following reasons: 1. Buffering Constraints: The ssh package uses a fixed amount of buffering shared between Stdout and Stderr [1][2]. If either stream is not read (or read too slowly) by your Go application, it can block the remote command, causing the session to hang indefinitely [1][2]. Always ensure that Stdout and Stderr are actively drained, for example, by using io.Copy in separate goroutines or assigning them to buffers/files [2][3]. 2. Improper Use of Run vs. Start/Wait: Run is a high-level helper that waits for the command to finish and handles stream copying [1][2]. If you are manually managing io.Copy for streams and calling Run, you may encounter errors or deadlocks because Run also attempts to manage the session's stream lifecycle [1][2]. If you need manual control over stream copying, use Start and Wait separately instead of Run [2][3]. 3. Channel EOF/Closing: Run and Wait rely on receiving an exit status from the remote server [1][2]. If the remote process does not send an exit status, or if streams are not closed correctly, Wait may hang or return an *ExitMissingError [1][2]. Ensure that if you are using StdinPipe(), you close the pipe after writing your input so the remote process receives an EOF and can terminate [2][4][3]. 4. Multiple Commands: A Session can only accept one call to Run, Start, Shell, Output, or CombinedOutput [1][2]. You cannot reuse the same Session for multiple sequential commands by calling Run repeatedly [4][3]. If you need to run multiple commands in the same shell, use Shell() and write commands to the StdinPipe() [3][5]. If you are debugging a hang, check if you have accidentally called Wait multiple times or failed to drain Stderr even if you expect no output [6][7]. Using session.CombinedOutput() is a common alternative if you want to merge Stdout and Stderr into a single buffer to simplify management [7].
Citations:
- 1: https://go.googlesource.com/crypto/+/refs/heads/master/ssh/session.go
- 2: https://github.com/golang/crypto/blob/master/ssh/session.go
- 3: https://stackoverflow.com/questions/76166288/golang-multiple-commands-in-the-same-ssh-session
- 4: https://stackoverflow.com/questions/35513125/run-multiple-commands-on-cisco-using-golang-crypto-ssh
- 5: x/crypto/ssh: stuck in Wait on more than one command golang/go#44060
- 6: x/crypto/ssh: semantics around running session.Wait() after calling session.Run(), when EOF messages are sent golang/go#32453
- 7: https://stackoverflow.com/questions/34678633/using-gos-crypto-ssh-to-run-commands-on-a-cisco-switch
Keep local stderr write errors out of tunnel status.
devssh.Run returns I/O errors from SSH streams, but IsGpgTunnelRunning maps every err != nil to false. When PassthroughWriter.Write fails on the local stderr path, this probe reports no tunnel even if the remote command succeeds. Use a writer that ignores local stderr errors for this query, or separate stream I/O errors from remote SSH exit errors.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/gpg/gpg_forwarding.go` at line 32, Update the writer used by
IsGpgTunnelRunning near PassthroughWriter so local stderr write failures cannot
propagate as the probe’s tunnel-status error. Use an error-discarding writer for
that stream while preserving remote command or SSH failures, ensuring
IsGpgTunnelRunning returns false only for actual tunnel/query failures.
A local write failure surfaced through devssh.Run's Stderr copy as a command error, so IsGpgTunnelRunning could report a working tunnel as not running. levelWriter never propagated write errors; match that.
Summary
IsGpgTunnelRunningpiped the remotegpg -Kcheck's stderr throughlog.Writer(log.LevelError). On a fresh~/.gnupg,gpg -Kemits harmless one-time init notices (directory/keybox/trustdb created) to stderr.Writer()wrote raw bytes as passthrough, so this went unnoticed. After fix(log): route Writer through the structured encoder instead of raw passthrough #797 routedWriter()through the structured encoder, those same benign lines now render as visibleERRORlog entries on everysshto a fresh workspace.log.PassthroughWriter(): writes bytes exactly as received, with no level filtering, line buffering, or structured formatting. SwitchedIsGpgTunnelRunningto use it, restoring the pre-fix(log): route Writer through the structured encoder instead of raw passthrough #797 look for this output (verified against the exact gpg message text from the regression).Summary by CodeRabbit
Bug Fixes
Tests