Skip to content

refactor(server): narrow the SSH session seam and type its connection state - #6938

Open
otavio wants to merge 1 commit into
masterfrom
refactor/ssh-session-seam
Open

refactor(server): narrow the SSH session seam and type its connection state#6938
otavio wants to merge 1 commit into
masterfrom
refactor/ssh-session-seam

Conversation

@otavio

@otavio otavio commented Aug 21, 2026

Copy link
Copy Markdown
Member

What

Narrows what the channels package needs from an SSH session down to one interface declared at the
consumer, and moves per-connection state behind typed context keys and named predicates. Both exist
to make the SSH path testable: DefaultSessionHandler, DefaultDirectTCPIPHandler and the whole
server/ssh/server/auth package had no tests before this.

Why

Session exposed 17 fields and 14 methods, and the channel handlers used 19 distinct paths into it.
Two of those reached through sess.Agent.Client — two levels into another module's struct — and the
data-copy loop read sess.Device.Info.Version to choose between Close and CloseWrite, putting a
device-compatibility rule inside a byte pump. Reaching either handler required a real SSH handshake
plus a live agent, so neither could be tested.

Per-connection state hung off gliderssh.Context under the string keys "snap" and "conn", with
progress tracked as an untyped int compared using < in four files. Both auth handlers separately
reimplemented "read the connection out of the context and close it", and password.go did not reuse
publickey.go's helper — which is why that package had no test seam at all.

Comes from the C1 and C2 candidates of an architecture review of the SSH path.

Changes

  • channels: Session interface declared in the consuming package, listing the 14 operations the
    handlers actually use. Each handler splits into a thin adapter that obtains the session and a core
    that takes the interface, following the newBannerHandlerWithDeps pattern already in server.go.
    A fake is the second adapter, which is what makes the seam real rather than hypothetical.
  • session: agent, client and the seat collection are unexported, so the reach-through is now
    a compile error rather than a convention. OpenAgentForwards, DialAgent and CloseAgentWrite
    replace the paths that went through them; CloseAgentWrite owns the agent-version rule.
  • session: State becomes a real type with Evaluated(), Established() and String(). Typed
    context keys replace "snap" and "conn". advance() replaces four inline
    getSnapshot(ctx).save(...) calls, and stays unexported so only the step doing the work can
    declare it done.
  • auth: both handlers share AuthenticableSessionOrDrop. The name states the side effect,
    because the function closes the socket on the false branch.
  • tcpip: seven copies of the same log block collapse into one field set.
  • tests: 16 new tests across four files, covering both channel handlers' rejection paths, the
    auth drop-guard, the agent-close policy and the connection-state module. pipe_test.go now drives
    the fake, which let its edition and namespace setup go away.

Testing

The interesting risk here is log output, not behaviour. Four changes a reviewer should agree with:

  • direct-tcpip logged origin_port and origin_addr transposed, in all seven copies. Merging
    them corrected it. This was a live bug, not a refactor.
  • The channel path now logs session where the session handler logged uid. Both handler guards
    already logged "session": ctx.SessionID(), so one session now reads under one key. The tree is
    split 23/24 on this pre-existing inconsistency; the rest is untouched.
  • direct-tcpip logs sshid as user@namespace.device (SSHID) rather than namespace.device
    (Target.Data).
  • Forward dials now abort on SSH-context cancellation, since Dial became DialContext per the Go
    conventions. A cancelled dial's error text reaches the client in the channel rejection.

Also worth a look: CloseAgentWrite on a seat with no agent channel returns ErrSeatNotFound
instead of closing. The only caller discards the error, so there is no behavioural difference today,
but it is a deliberate change from the old silent CloseWrite.

Full server suite passes (28 packages), golangci-lint run ./... reports no issues, and go mod tidy leaves the tree clean. cloud/ does not import either package, so nothing there needs to move.

… state

The channel handlers reached through sess.Agent.Client into another module's struct and read the
device version inside the byte-copy loop, so neither handler could run without a real handshake
and a live agent. They now take an interface declared in the channels package, and the session's
agent, client and seat collection are unexported, which makes that compile-enforced rather than a
convention.

Per-connection state moves behind typed context keys and named predicates, and the two auth
handlers share one reject-and-close path. They had none before, which is why the auth package
carried no tests at all.

Changes that are not pure refactors:

- direct-tcpip logged origin_port and origin_addr transposed, in all seven copies of the block;
  collapsing them into one field set corrects it
- the channel path logs session where the session handler logged uid, agreeing with the guard log
  both handlers already emitted
- direct-tcpip logs sshid as user@namespace.device rather than namespace.device
- forward dials abort when the SSH context is cancelled (Dial -> DialContext)
- CloseAgentWrite on a seat with no agent channel reports ErrSeatNotFound instead of closing; the
  only caller discards it
@otavio
otavio requested a review from a team as a code owner August 21, 2026 20:49
@github-actions

github-actions Bot commented Aug 21, 2026

Copy link
Copy Markdown

Code Review Complete

The automated review ran but did not post an updated summary — this usually means no new issues were found since the previous review. If you've pushed changes and want a fresh pass, comment /review.

View job

@github-actions

github-actions Bot commented Aug 21, 2026

Copy link
Copy Markdown

Claude finished @otavio's task in 1m 16s —— View job


Code Review

  • Gathered PR context
  • Reviewing with 5 specialized agents (in progress)
  • Posting feedback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant