Add configurable SSH session duration

[rkoster]

Summary

• Add an optional maximum SSH session duration for the ssh_proxy job, configured as a non-negative integer number of seconds. The default is 0 for unlimited sessions.
• Measure the lifetime from successful client authentication, including backend connection setup. Close the connection and its channels when the duration expires, even while traffic is active.
• Add Go and BOSH template coverage for duration parsing, defaults, stalled backend setup, active-channel teardown, and timeout logging.

Testing

• devbox run test:go — proxy and config package tests with the race detector
• devbox run test — 8 template specs passed
• devbox run bundle exec rubocop spec/ssh_proxy_template_spec.rb

[jcvrabo]

a simple alternative to the requested enhancement on #1177

Tested it and it works as described, I only question the need to cap the max session duration to 1 hour. I think it could be more flexible to leave it to the operator with an eventual much larger cap.

[rkoster]

I only question the need to cap the max session duration to 1 hour. I think it could be more flexible to leave it to the operator with an eventual much larger cap.

Removed the hard cap. Operators can now configure any positive duration in seconds, or leave the property empty for unlimited sessions. Added coverage for a 24-hour duration in d22dee0.

[Copilot]

Copilot review overview

🟡 Changes recommended

Both validation paths permit durations above the stated one-hour cap.

Review effort: Balanced
Findings: 2

Open (2)

• Enforce 3,600-second maximum duration in template validation · New
• Enforce one-hour maximum duration in configuration validation · New

What changed in this PR

Adds configurable SSH session lifetimes, defaulting to unlimited, with connection termination upon expiry.

Changes:

• Propagates duration configuration into the SSH proxy.
• Adds timer-based connection termination.
• Adds Go and BOSH template tests and validation.

File	Description
proxy/​proxy.go	Enforces configured connection duration.
proxy/​proxy_test.go	Tests active connection expiry.
cmd/​ssh-proxy/​main.go	Passes duration into the proxy.
cmd/​ssh-proxy/​config/​config.go	Parses and validates duration.
cmd/​ssh-proxy/​config/​config_test.go	Tests configuration parsing.
spec/​ssh_proxy_template_spec.rb	Tests BOSH template rendering.
jobs/​ssh_proxy/​templates/​ssh_proxy.json.erb	Renders and validates duration.
jobs/​ssh_proxy/​spec	Defines the new BOSH property.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot review overview

🟡 Changes recommended

The limit is not enforced while backend connection setup is stalled.

Review effort: Balanced
Findings: 1

Open (1)

• Start connection lifetime enforcement before backend setup · New

Resolved since last review (2)

• Enforce one-hour maximum duration in configuration validation
• Enforce 3,600-second maximum duration in template validation

[Copilot]

Copilot review overview

🟢 Approval recommended

The configured lifetime is consistently validated, propagated, enforced, and covered by targeted tests.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Start connection lifetime enforcement before backend setup

[Copilot]

Copilot review overview

🟢 Approval recommended

Deadline handling is consistently applied and supported by focused configuration, template, and connection tests.

Review effort: Balanced
Findings: None

[ameowlia]

I used AI to review and these are the findings that I agree with (I removed the ones I didn't)

Critical

None.

Important

1. "Active session" test never exercises an active session

• Where: src/code.cloudfoundry.org/diego-ssh/proxy/proxy_test.go:233
• Problem: It("closes the SSH connection even while it is active") dials an SSH client, then waits on client.Wait(). It never opens a channel, session, or data stream, so the connection is idle during the 100ms window. The test proves the timeout fires before idleConnectionTimeout (5s). It does not prove that proxied channels with traffic get torn down, which is the PR's main claim.
• Evidence: Test body is only go func() { closed <- client.Wait() }() plus two Eventually assertions (lines 234-237).
• Fix: Open a session or channel (for example, client.NewSession() running a long command, or stream bytes over a channel), then assert the session or channel ends after the duration.

2. Timeout log line never asserted

• Where: src/code.cloudfoundry.org/diego-ssh/proxy/proxy.go:89, proxy_test.go:233
• Problem: No test asserts maximum-connection-duration-reached. No test asserts it is absent on a normal close. This log is the only way operators can tell a forced cutoff from a client disconnect.
• Fix: Add Eventually(logger).Should(gbytes.Say("maximum-connection-duration-reached")) to the max-duration test.

Minor

3. context.AfterFunc registered even when unlimited

• Where: src/code.cloudfoundry.org/diego-ssh/proxy/proxy.go:99 (also proxy.go:398 in NewClientConn)
• Problem: The serverConn.Close() AfterFunc is guarded by if p.maxConnectionDuration > 0. The clientConn.Close() AfterFunc (line 99) and the nConn.Close() AfterFunc (line 398) are registered unconditionally, including on context.Background() in the default unlimited case. This is not a leak: Background's Done() is nil, so nothing is ever scheduled, and stop() is deferred. It is a small per-connection allocation, and it's inconsistent with the guarded block just above.
• Fix: Move lines 99-100 inside the existing if p.maxConnectionDuration > 0 block. Optionally guard line 398 with if ctx.Done() != nil.

4. Abrupt disconnect with no reason given to the client

• Where: src/code.cloudfoundry.org/diego-ssh/proxy/proxy.go:88-91
• Problem: On expiry, serverConn.Close() drops the TCP stream. cf ssh users see a bare disconnect with no hint that a session lifetime limit caused it.
• Fix: Consider writing a message to open session channels' stderr, or sending an SSH disconnect message, before closing. At minimum, document the behavior in the job spec description.

6. No config test for omitted or zero duration

• Where: src/code.cloudfoundry.org/diego-ssh/cmd/ssh-proxy/config/config_test.go:51,110
• Problem: The baseline fixture now sets "max_connection_duration": "1h". There are tests for -1s (rejected) and 24h, but none shows that an omitted field parses to 0 (unlimited). That is the default path the ERB relies on.
• Fix: Add a context with the field omitted, and assert MaxConnectionDuration == 0.

Nits

7. Unused require 'ipaddr'

• Where: spec/ssh_proxy_template_spec.rb:6 (new file)
• Problem: IPAddr is never referenced.
• Fix: Remove the require.

8. Duration logged as raw nanoseconds

• Where: src/code.cloudfoundry.org/diego-ssh/proxy/proxy.go:89
• Problem: lager.Data{"duration": p.maxConnectionDuration} serializes as an int64 in nanoseconds (for example, 60000000000), which is easy to misread as seconds.
• Fix: Convert to seconds in every log that emits this duration, and name the key to match. For example: lager.Data{"duration-in-seconds": p.maxConnectionDuration.Seconds()}. That way the logged value lines up with the operator-facing property max_connection_duration_in_seconds.

10. Timer starts after client auth, not at TCP accept

• Where: src/code.cloudfoundry.org/diego-ssh/proxy/proxy.go:77-83
• Problem: The deadline is created after ssh.NewServerConn returns, so client handshake and auth time don't count toward the limit. That's reasonable, but the spec says "Maximum lifetime of an SSH connection".
• Fix: Clarify the spec description (for example, "measured from successful authentication").

[rkoster]

Addressed in 11e17fb.

"Active session" test never exercises an active session

Added bidirectional channel traffic through the proxy, with assertions that timeout terminates the stream, both channel ends, and the connection.

Timeout log line never asserted
Duration logged as raw nanoseconds

Added expiry-log coverage and a normal-close test confirming no timeout log appears after the cancelled deadline. The log now uses duration-in-seconds.

context.AfterFunc registered even when unlimited

Guarded both backend cancellation callbacks with ctx.Done() != nil.

Abrupt disconnect with no reason given to the client
Timer starts after client auth, not at TCP accept

The job spec now documents abrupt closure without a client-facing reason and timing from successful client authentication, including backend setup.

No config test for omitted or zero duration

Added both cases, asserting zero for unlimited sessions.

Unused require 'ipaddr'

Retained it because the rendered ssh_proxy.json.erb uses IPAddr.new for address validation.

Verification: proxy/config tests pass with the race detector; all 8 template specs and RuboCop pass.