Skip to content

Harden SSH interoperability, lifecycle, and regression coverage #30

Description

@skevetter

Summary

devsy-org/ssh does not require a rewrite. With v1.2.7 as the corrected functional baseline, the remaining work is to harden protocol compatibility, channel/connection lifecycle invariants, race coverage, and CI so future changes do not regress OpenSSH/VS Code Remote-SSH behavior.

This issue tracks the remaining work as one implementation PR with multiple logical commits.

Goals

  1. Add real OpenSSH compatibility coverage for keepalive and dynamic forwarding.
  2. Validate the same integration suite in GitLab Runner CI in addition to GitHub Actions.
  3. Clarify keepalive/peer-activity semantics so comments match what the package can actually observe.
  4. Add race/stress tests around channel tracking, keepalive selection, and connection teardown.
  5. Add deterministic tests for connection callbacks and Server.Close / Server.Shutdown behavior.
  6. Keep the implementation based on golang.org/x/crypto/ssh; do not rewrite the SSH protocol layer.

Work plan

A. Keepalive semantic cleanup

  • Correct notePeerActivity documentation so it no longer claims packet-level visibility.
  • Separate internal concepts for keepalive reply vs inbound request activity where useful without breaking public API compatibility.
  • Preserve the v1.2.7 invariant that both positive and negative SSH replies prove liveness.

B. Real OpenSSH interoperability tests

  • Add test helpers for starting/stopping a real OpenSSH client process.
  • Add idle keepalive compatibility test using a short ClientAliveInterval / ClientAliveCountMax.
  • Add ssh -T -N -D <port> dynamic-forwarding regression test.
  • Exercise at least 10 sequential forwarded connections over one parent SSH connection.
  • Add concurrent forwarding/channel churn coverage.
  • Add a dead/unresponsive peer test using a TCP proxy that stops forwarding bytes while leaving sockets open.
  • Ensure child processes/listeners are always cleaned up after test failure.

C. Channel lifecycle and race hardening

  • Directly test openChannelSet add/remove/any semantics and concurrent access.
  • Stress trackingNewChannel cancellation/backpressure behavior.
  • Test channel close racing with server keepalive selection.
  • Verify channel transport errors remove stale channels and safely fall back to global keepalive.
  • Verify closing a child channel never closes a healthy parent SSH transport.
  • Run lifecycle tests under the Go race detector.

D. Connection lifecycle callbacks

  • Successful handshake: ConnectionFailedCallback == 0, ConnectionCompleteCallback == 1.
  • Failed handshake: ConnectionFailedCallback == 1, ConnectionCompleteCallback == 0.
  • Document/test ConnectionClosingCallback ordering and semantics.
  • Verify completion callbacks retain the underlying mux/transport termination cause.

E. Server shutdown hardening

  • Test Server.Close() with active connections/channels.
  • Test Server.Shutdown() preserves existing connections while rejecting new ones.
  • Test shutdown context expiration.
  • Race keepalive requests against Close() / Shutdown() and verify goroutines exit cleanly.

F. CI and runner validation

GitHub Actions

  • Keep normal go test ./... job.
  • Add go test -race ./... gate.
  • Add OpenSSH integration job, preferably with an openssh_integration build tag.
  • Run portable unit tests across Linux/macOS/Windows where practical.

GitLab Runner

  • Add/extend .gitlab-ci.yml so a GitLab runner executes the OpenSSH integration suite.
  • Ensure the runner image has openssh-client available or installs it explicitly.
  • Run the same tagged integration command used by GitHub Actions.
  • Add race validation where runner capabilities permit.
  • Treat GitLab integration failures as regressions, not optional smoke tests.

Suggested integration command:

go test -tags=openssh_integration ./...

Suggested race command:

go test -race ./...

Required invariants

  • Negative SSH keepalive reply means peer is alive.
  • No reply before the configured deadline closes the connection.
  • Channel close never closes a healthy parent transport.
  • Closed channels are eventually removed from keepalive selection.
  • Keepalive/channel-close races safely fall back when appropriate.
  • Server.Close() terminates active transports.
  • Server.Shutdown() preserves active transports until they close or the caller context expires.
  • Successful handshake produces exactly one completion callback.
  • Failed handshake produces exactly one failure callback.
  • Request-forwarding goroutines terminate on connection cancellation.
  • Real OpenSSH remains connected beyond the keepalive deadline while replying.
  • Real dynamic forwarding remains usable after repeated sequential/concurrent channel churn.

Non-goals

  • rewriting the SSH library;
  • replacing golang.org/x/crypto/ssh;
  • implementing transparent reconnect;
  • moving Devsy workspace/provider concerns into this library;
  • packet-level inspection solely to imitate OpenSSH internals.

Delivery

Implement this issue as one pull request with multiple logical commits. Suggested commit sequence:

  1. refactor(keepalive): clarify peer activity and liveness semantics
  2. test: add OpenSSH interoperability coverage
  3. test(server): harden channel lifecycle and keepalive races
  4. test(server): harden connection and shutdown lifecycle
  5. ci: run OpenSSH integration and race tests in GitHub and GitLab
  6. docs: document keepalive and lifecycle callback contracts

Commit-signing CI can be ignored during implementation; signing will be handled separately before merge.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions