Repository navigation
fix(ssh): report text a server sends before its version line - #486
Open
100milliongold wants to merge 1 commit into
Open
100milliongold wants to merge 1 commit into
100milliongold wants to merge 1 commit into
Conversation
OpenSSH 9.8 and later refuse a connection under PerSourcePenalties or MaxStartups by writing "Not allowed at this time" where the version string would go and hanging up. x/crypto skips pre-version lines, so the handshake error said only EOF or "connection reset by peer" and read like a network fault. Keep up to 255 bytes of what arrives before the version line and add it to the error when the handshake fails before any version line. The text is added after classification, so it cannot turn a failure into ErrAuthFailed. Signed-off-by: Jea-Eok-Kim <gadian88@gmail.com>
100milliongold
force-pushed
the
fix/ssh-pre-version-text
branch
from
September 30, 2026 01:19
a922c96 to
08ae617
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Long individual lines retain their beginning rather than the intended diagnostic tail.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Improves SSH handshake diagnostics by preserving server text sent before the SSH version line.
Changes:
- Adds a bounded, concurrency-safe pre-version recorder.
- Integrates diagnostics into direct, bastion, and ProxyCommand connections.
- Adds unit and integration coverage for diagnostics and error classification.
| File | Description |
|---|---|
protocol/ssh/preversion.go |
Implements pre-version text recording and annotation. |
protocol/ssh/preversion_test.go |
Tests recording, bounds, quoting, and version handling. |
protocol/ssh/connection.go |
Integrates recording into direct and bastion handshakes. |
protocol/ssh/connection_test.go |
Tests direct and bastion diagnostics. |
protocol/ssh/proxy.go |
Integrates recording into ProxyCommand handshakes. |
protocol/ssh/proxy_test.go |
Tests ProxyCommand failure and timeout diagnostics. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+68
to
+70
| if len(r.line) < maxPreVersionText { | ||
| r.line = append(r.line, char) | ||
| } |
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
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.
Fixes #485
When a handshake fails before the server's version line arrives, the error now ends with what the server sent in its place:
That is how OpenSSH 9.8 and later say a connection was dropped by
PerSourcePenaltiesorMaxStartups. x/crypto skips pre-version lines, so until now the error looked like a network fault.Choices a reviewer may want to know about:
classifyHandshakeError, so a server cannot make an ordinary handshake failure look likeErrAuthFailedby sendingssh: unable to authenticate.%qkeeps control characters away from the terminal.go test ./...,go test -race ./protocol/ssh/and golangci-lint v2.13.1 pass. Each new integration test fails when its call site'sannotateis removed.This PR was written by an AI agent (Claude Code) at the request of @100milliongold. I cannot be re-invoked on my own, so @100milliongold owns the follow-up on review comments and CI.