streamlocal: bring in upstream unix forwarding implementation#7
Draft
streamlocal: bring in upstream unix forwarding implementation#7
Conversation
Adds optional (disabled by default) implementations of local->remote and
remote->local Unix forwarding through OpenSSH's protocol extensions:
- streamlocal-forward@openssh.com
- cancel-streamlocal-forward@openssh.com
- forwarded-streamlocal@openssh.com
- direct-streamlocal@openssh.com
Adds tests for Unix forwarding, reverse Unix forwarding and reverse TCP
forwarding.
Co-authored-by: Samuel Corsi-House <chouse.samuel@gmail.com>
The Reserved field in remoteUnixForwardChannelData is uint32 but the
OpenSSH PROTOCOL spec (Section 2.4) and x/crypto/ssh forwardedStreamLocalPayload
both define it as string. The current code works by coincidence because
uint32(0) and string("") produce identical wire bytes, but this is a
latent protocol bug.
Also fix server_test.go rebase artifact: newLocalListener was renamed to
newLocalTCPListener by the unix forwarding commit.
Spec: https://github.com/openssh/openssh-portable/blob/master/PROTOCOL
Ref: https://cs.opensource.google/go/x/crypto/+/master:ssh/streamlocal.go
Updates gliderlabs#196
Address review feedback from gliderlabs#196: - Return true for duplicate forward requests instead of false. Returning false causes clients with ExitOnForwardFailure=yes to disconnect, which diverges from OpenSSH behavior. (mafredri, ge9) Ref: gliderlabs#196 (comment) Ref: https://github.com/coder/coder/blob/b828412edd913bef6665cf8a0b2ca7ac93334012/agent/agentssh/forward.go#L76-L91 - Use context-aware net.ListenConfig in SimpleUnixReverseForwardingCallback instead of bare net.Listen, matching the dialer pattern used in SimpleUnixLocalForwardingCallback. (mafredri) Ref: gliderlabs#196 (comment) Updates gliderlabs#196
Several concurrency issues exist in the shared ForwardedUnixHandler: - Key the forwards map by (sessionID, addr) instead of addr alone. A single handler instance is shared across SSH connections, so addr-only keys allow cross-session collisions and let any session cancel another session's forwards. This matches the approach used in coder/coder's production fork of this code. - Pass the parent context (connection-scoped) to bicopy instead of the derived context. The derived context is cancelled when the accept loop exits, which prematurely tears down active forwarded connections that are still transferring data. - Delete the map entry atomically in the cancel handler instead of relying on the accept-loop goroutine to clean up asynchronously. This prevents a timing window where the stale entry would reject legitimate re-forward requests. Updates gliderlabs#196
- Fix doc comment for ErrRejected: the comment said "ErrReject" but the variable is "ErrRejected". Also enumerate which callbacks honor it. - Use %v on error instead of %+v on err.Error() string in reject message. The %+v verb on a string is identical to %s; the stack trace behavior of %+v only works on the error value itself. - Fix incorrect os.Stat check in TestReverseUnixForwardingWorks. The condition "err == nil && !os.IsNotExist(err)" has a redundant second clause since os.IsNotExist(nil) is always false. Simplify to "err == nil". Updates gliderlabs#196
SimpleUnixLocalForwardingCallback and SimpleUnixReverseForwardingCallback perform no validation on client-supplied socket paths. A malicious client can request forwarding to arbitrary paths, which in the reverse case causes directory creation (MkdirAll), file deletion (unlink), and socket binding at the requested path. Add prominent documentation warnings that these helpers are intended for trusted environments and that production deployments should implement custom callbacks with path validation and access control. Updates gliderlabs#196
Silence golangci-lint errcheck warnings for unchecked return values in the new streamlocal and tcpip forwarding tests. Updates gliderlabs#196
Use maps.Copy instead of manual for-range copy loops in ensureHandlers, as suggested by golang.org/x/tools/go/analysis/passes/modernize.
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
2 participants
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.
This PR brings in gliderlabs#196 to add Unix socket forwarding support.
In addition it aims to address the open comments from that PR.
I had Claude review the code to propose other improvements and bring them in.
Modernised the code to use Go 1.26 features.