From b4e8337e029e666a1486bba5f1c774e8a9e6ff76 Mon Sep 17 00:00:00 2001 From: rkoster Date: Mon, 28 Sep 2026 15:53:45 +0200 Subject: [PATCH 1/6] Add configurable SSH session duration --- jobs/ssh_proxy/spec | 3 + jobs/ssh_proxy/templates/ssh_proxy.json.erb | 17 ++++- spec/ssh_proxy_template_spec.rb | 74 +++++++++++++++++++ .../diego-ssh/cmd/ssh-proxy/config/config.go | 5 ++ .../cmd/ssh-proxy/config/config_test.go | 13 ++++ .../diego-ssh/cmd/ssh-proxy/main.go | 2 +- .../diego-ssh/proxy/proxy.go | 28 +++++-- .../diego-ssh/proxy/proxy_test.go | 13 +++- 8 files changed, 144 insertions(+), 11 deletions(-) create mode 100644 spec/ssh_proxy_template_spec.rb diff --git a/jobs/ssh_proxy/spec b/jobs/ssh_proxy/spec index 4107eb8617..0cca6dd62c 100644 --- a/jobs/ssh_proxy/spec +++ b/jobs/ssh_proxy/spec @@ -87,6 +87,9 @@ properties: diego.ssh_proxy.idle_connection_timeout_in_seconds: description: Idle timeout for incoming connections default: 300 + diego.ssh_proxy.max_connection_duration_in_seconds: + description: Maximum lifetime of an SSH connection, in seconds. Leave empty for unlimited; maximum is 3600. + default: "" diego.ssh_proxy.uaa.url: description: The domain name of the UAA diff --git a/jobs/ssh_proxy/templates/ssh_proxy.json.erb b/jobs/ssh_proxy/templates/ssh_proxy.json.erb index 920b2a4264..21eb6923de 100644 --- a/jobs/ssh_proxy/templates/ssh_proxy.json.erb +++ b/jobs/ssh_proxy/templates/ssh_proxy.json.erb @@ -39,6 +39,22 @@ config[:idle_connection_timeout] = "#{value}s" end + max_connection_duration = p("diego.ssh_proxy.max_connection_duration_in_seconds") + unless max_connection_duration.to_s.empty? + unless max_connection_duration.is_a?(Integer) || max_connection_duration.to_s.match(/\A\d+\z/) + raise "diego.ssh_proxy.max_connection_duration_in_seconds must be an integer between 1 and 3600, or empty for unlimited" + end + begin + max_connection_duration = Integer(max_connection_duration) + rescue ArgumentError, TypeError + raise "diego.ssh_proxy.max_connection_duration_in_seconds must be an integer between 1 and 3600, or empty for unlimited" + end + if max_connection_duration < 1 || max_connection_duration > 3600 + raise "diego.ssh_proxy.max_connection_duration_in_seconds must be between 1 and 3600, or empty for unlimited" + end + config[:max_connection_duration] = "#{max_connection_duration}s" + end + config[:bbs_address] = "https://" + p("diego.ssh_proxy.bbs.api_location") config[:bbs_client_cert] = "/var/vcap/jobs/ssh_proxy/config/certs/bbs/client.crt" config[:bbs_client_key] = "/var/vcap/jobs/ssh_proxy/config/certs/bbs/client.key" @@ -122,4 +138,3 @@ config.to_json %> - diff --git a/spec/ssh_proxy_template_spec.rb b/spec/ssh_proxy_template_spec.rb new file mode 100644 index 0000000000..2c45d1e1a1 --- /dev/null +++ b/spec/ssh_proxy_template_spec.rb @@ -0,0 +1,74 @@ +# frozen_string_literal: true + +# rubocop: disable Metrics/BlockLength +require 'rspec' +require 'json' +require 'ipaddr' +require 'bosh/template/test' + +describe 'ssh_proxy' do + let(:release_path) { File.join(File.dirname(__FILE__), '..') } + let(:release) { Bosh::Template::Test::ReleaseDir.new(release_path) } + let(:job) { release.job('ssh_proxy') } + let(:deployment_manifest_fragment) do + { + 'diego' => { + 'ssh_proxy' => { + 'host_key' => 'HOST KEY', + 'bbs' => { + 'ca_cert' => 'BBS CA CERT', + 'client_cert' => 'BBS CLIENT CERT', + 'client_key' => 'BBS CLIENT KEY' + } + } + }, + 'loggregator' => { + 'ca_cert' => 'LOGGREGATOR CA CERT', + 'cert' => 'LOGGREGATOR CERT', + 'key' => 'LOGGREGATOR KEY' + } + } + end + + describe 'ssh_proxy.json.erb' do + let(:template) { job.template('config/ssh_proxy.json') } + let(:rendered_config) { JSON.parse(template.render(deployment_manifest_fragment)) } + + context 'when max_connection_duration_in_seconds is empty' do + it 'omits the connection duration to allow unlimited sessions' do + expect(rendered_config).not_to have_key('max_connection_duration') + end + end + + context 'when max_connection_duration_in_seconds is configured' do + before do + deployment_manifest_fragment['diego']['ssh_proxy']['max_connection_duration_in_seconds'] = 3600 + end + + it 'renders the duration in seconds' do + expect(rendered_config['max_connection_duration']).to eq('3600s') + end + end + + context 'when max_connection_duration_in_seconds exceeds one hour' do + before do + deployment_manifest_fragment['diego']['ssh_proxy']['max_connection_duration_in_seconds'] = 3601 + end + + it 'fails rendering' do + expect { rendered_config }.to raise_error(/must be between 1 and 3600/) + end + end + + context 'when max_connection_duration_in_seconds is not an integer' do + before do + deployment_manifest_fragment['diego']['ssh_proxy']['max_connection_duration_in_seconds'] = 1.5 + end + + it 'fails rendering' do + expect { rendered_config }.to raise_error(/must be an integer between 1 and 3600/) + end + end + end +end +# rubocop: enable Metrics/BlockLength diff --git a/src/code.cloudfoundry.org/diego-ssh/cmd/ssh-proxy/config/config.go b/src/code.cloudfoundry.org/diego-ssh/cmd/ssh-proxy/config/config.go index 42336256ac..9be4322f58 100644 --- a/src/code.cloudfoundry.org/diego-ssh/cmd/ssh-proxy/config/config.go +++ b/src/code.cloudfoundry.org/diego-ssh/cmd/ssh-proxy/config/config.go @@ -6,6 +6,7 @@ import ( "encoding/json" "errors" "os" + "time" "code.cloudfoundry.org/debugserver" loggingclient "code.cloudfoundry.org/diego-logging-client" @@ -44,6 +45,7 @@ type SSHProxyConfig struct { LoggregatorConfig loggingclient.Config `json:"loggregator"` CommunicationTimeout durationjson.Duration `json:"communication_timeout,omitempty"` IdleConnectionTimeout durationjson.Duration `json:"idle_connection_timeout,omitempty"` + MaxConnectionDuration durationjson.Duration `json:"max_connection_duration,omitempty"` ConnectToInstanceAddress bool `json:"connect_to_instance_address"` BackendsTLSEnabled bool `json:"backends_tls_enabled,omitempty"` @@ -68,6 +70,9 @@ func NewSSHProxyConfig(configPath string) (SSHProxyConfig, error) { if err != nil { return SSHProxyConfig{}, err } + if proxyConfig.MaxConnectionDuration < 0 || proxyConfig.MaxConnectionDuration > durationjson.Duration(time.Hour) { + return SSHProxyConfig{}, errors.New("max_connection_duration must not exceed 1h") + } return proxyConfig, nil } diff --git a/src/code.cloudfoundry.org/diego-ssh/cmd/ssh-proxy/config/config_test.go b/src/code.cloudfoundry.org/diego-ssh/cmd/ssh-proxy/config/config_test.go index f19cfd2cda..e91d883fcd 100644 --- a/src/code.cloudfoundry.org/diego-ssh/cmd/ssh-proxy/config/config_test.go +++ b/src/code.cloudfoundry.org/diego-ssh/cmd/ssh-proxy/config/config_test.go @@ -48,6 +48,7 @@ var _ = Describe("SSHProxyConfig", func() { "debug_address": "5.5.5.5:9090", "connect_to_instance_address": true, "idle_connection_timeout": "5ms", + "max_connection_duration": "1h", "backends_tls_enabled": true, "backends_tls_ca_certificates": "./some_filepath/ca.crt", @@ -106,6 +107,7 @@ var _ = Describe("SSHProxyConfig", func() { AllowedHostKeyAlgorithms: "hostkeyalg1,hostkeyalg2,hostkeyalg3", ConnectToInstanceAddress: true, IdleConnectionTimeout: durationjson.Duration(5 * time.Millisecond), + MaxConnectionDuration: durationjson.Duration(time.Hour), LagerConfig: lagerflags.LagerConfig{ LogLevel: lagerflags.DEBUG, }, @@ -127,6 +129,17 @@ var _ = Describe("SSHProxyConfig", func() { }) }) + Context("when the max connection duration exceeds one hour", func() { + BeforeEach(func() { + configData = `{"max_connection_duration": "1h1s"}` + }) + + It("returns an error", func() { + _, err := config.NewSSHProxyConfig(configFilePath) + Expect(err).To(MatchError("max_connection_duration must not exceed 1h")) + }) + }) + Context("when the file does not contain valid json", func() { BeforeEach(func() { configData = "{{" diff --git a/src/code.cloudfoundry.org/diego-ssh/cmd/ssh-proxy/main.go b/src/code.cloudfoundry.org/diego-ssh/cmd/ssh-proxy/main.go index 8cacf3f8b4..66f09f3029 100644 --- a/src/code.cloudfoundry.org/diego-ssh/cmd/ssh-proxy/main.go +++ b/src/code.cloudfoundry.org/diego-ssh/cmd/ssh-proxy/main.go @@ -62,7 +62,7 @@ func main() { logger.Error("failed-to-get-tls-config", err) os.Exit(1) } - sshProxy := proxy.New(logger, proxySSHServerConfig, metronClient, tlsConfig) + sshProxy := proxy.New(logger, proxySSHServerConfig, metronClient, tlsConfig, time.Duration(sshProxyConfig.MaxConnectionDuration)) server := server.NewServer(logger, sshProxyConfig.Address, sshProxy, time.Duration(sshProxyConfig.IdleConnectionTimeout)) healthCheckHandler := healthcheck.NewHandler(logger) diff --git a/src/code.cloudfoundry.org/diego-ssh/proxy/proxy.go b/src/code.cloudfoundry.org/diego-ssh/proxy/proxy.go index a107b26615..c9ca941b9b 100644 --- a/src/code.cloudfoundry.org/diego-ssh/proxy/proxy.go +++ b/src/code.cloudfoundry.org/diego-ssh/proxy/proxy.go @@ -8,6 +8,7 @@ import ( "net" "strings" "sync" + "time" "unicode/utf8" loggingclient "code.cloudfoundry.org/diego-logging-client" @@ -40,14 +41,15 @@ type LogMessage struct { } type Proxy struct { - logger lager.Logger - serverConfig *ssh.ServerConfig + logger lager.Logger + serverConfig *ssh.ServerConfig connectionLock *sync.Mutex connections int metronClient loggingclient.IngressClient - tlsConfig *tls.Config + tlsConfig *tls.Config + maxConnectionDuration time.Duration } func New( @@ -55,13 +57,15 @@ func New( serverConfig *ssh.ServerConfig, metronClient loggingclient.IngressClient, tlsConfig *tls.Config, + maxConnectionDuration time.Duration, ) *Proxy { return &Proxy{ - logger: logger, - serverConfig: serverConfig, - connectionLock: &sync.Mutex{}, - metronClient: metronClient, - tlsConfig: tlsConfig, + logger: logger, + serverConfig: serverConfig, + connectionLock: &sync.Mutex{}, + metronClient: metronClient, + tlsConfig: tlsConfig, + maxConnectionDuration: maxConnectionDuration, } } @@ -79,6 +83,14 @@ func (p *Proxy) HandleConnection(netConn net.Conn) { if err != nil { return } + if p.maxConnectionDuration > 0 { + timer := time.AfterFunc(p.maxConnectionDuration, func() { + logger.Info("maximum-connection-duration-reached", lager.Data{"duration": p.maxConnectionDuration}) + _ = serverConn.Close() + _ = clientConn.Close() + }) + defer timer.Stop() + } logMessage := extractLogMessage(logger, serverConn.Permissions) diff --git a/src/code.cloudfoundry.org/diego-ssh/proxy/proxy_test.go b/src/code.cloudfoundry.org/diego-ssh/proxy/proxy_test.go index 5974393041..df3edc0ec4 100644 --- a/src/code.cloudfoundry.org/diego-ssh/proxy/proxy_test.go +++ b/src/code.cloudfoundry.org/diego-ssh/proxy/proxy_test.go @@ -68,6 +68,7 @@ var _ = Describe("Proxy", func() { proxyDone chan struct{} daemonDone chan struct{} + maxConnectionDuration time.Duration ) BeforeEach(func() { @@ -129,7 +130,7 @@ var _ = Describe("Proxy", func() { }) JustBeforeEach(func() { - sshProxy = proxy.New(logger.Session("proxy"), proxySSHConfig, fakeMetronClient, nil) + sshProxy = proxy.New(logger.Session("proxy"), proxySSHConfig, fakeMetronClient, nil, maxConnectionDuration) proxyServer = server.NewServer(logger.Session("proxy-server"), "", sshProxy, 500*time.Millisecond) proxyServer.SetListener(proxyListener) go func() { @@ -210,6 +211,16 @@ var _ = Describe("Proxy", func() { Expect(string(password)).To(Equal("fake-some-password")) }) + Context("when the maximum connection duration is reached", func() { + BeforeEach(func() { + maxConnectionDuration = 100 * time.Millisecond + }) + + It("closes the SSH connection even while it is active", func() { + Eventually(client.Wait).Should(HaveOccurred()) + }) + }) + Context("metron", func() { It("emits a successful log message on behalf of the lrp", func() { Eventually(fakeMetronClient.SendAppLogCallCount).Should(Equal(1)) From d22dee00c0e7bc5ae0998ca84c283be853f128f2 Mon Sep 17 00:00:00 2001 From: rkoster Date: Wed, 30 Sep 2026 14:53:53 +0200 Subject: [PATCH 2/6] Allow longer SSH session duration --- jobs/ssh_proxy/spec | 2 +- jobs/ssh_proxy/templates/ssh_proxy.json.erb | 8 ++++---- spec/ssh_proxy_template_spec.rb | 14 +++++++------- .../diego-ssh/cmd/ssh-proxy/config/config.go | 5 ++--- .../cmd/ssh-proxy/config/config_test.go | 18 +++++++++++++++--- 5 files changed, 29 insertions(+), 18 deletions(-) diff --git a/jobs/ssh_proxy/spec b/jobs/ssh_proxy/spec index 0cca6dd62c..8e0fb6936b 100644 --- a/jobs/ssh_proxy/spec +++ b/jobs/ssh_proxy/spec @@ -88,7 +88,7 @@ properties: description: Idle timeout for incoming connections default: 300 diego.ssh_proxy.max_connection_duration_in_seconds: - description: Maximum lifetime of an SSH connection, in seconds. Leave empty for unlimited; maximum is 3600. + description: Maximum lifetime of an SSH connection, in seconds. Leave empty for unlimited. default: "" diego.ssh_proxy.uaa.url: diff --git a/jobs/ssh_proxy/templates/ssh_proxy.json.erb b/jobs/ssh_proxy/templates/ssh_proxy.json.erb index 21eb6923de..68a17e00a5 100644 --- a/jobs/ssh_proxy/templates/ssh_proxy.json.erb +++ b/jobs/ssh_proxy/templates/ssh_proxy.json.erb @@ -42,15 +42,15 @@ max_connection_duration = p("diego.ssh_proxy.max_connection_duration_in_seconds") unless max_connection_duration.to_s.empty? unless max_connection_duration.is_a?(Integer) || max_connection_duration.to_s.match(/\A\d+\z/) - raise "diego.ssh_proxy.max_connection_duration_in_seconds must be an integer between 1 and 3600, or empty for unlimited" + raise "diego.ssh_proxy.max_connection_duration_in_seconds must be a positive integer, or empty for unlimited" end begin max_connection_duration = Integer(max_connection_duration) rescue ArgumentError, TypeError - raise "diego.ssh_proxy.max_connection_duration_in_seconds must be an integer between 1 and 3600, or empty for unlimited" + raise "diego.ssh_proxy.max_connection_duration_in_seconds must be a positive integer, or empty for unlimited" end - if max_connection_duration < 1 || max_connection_duration > 3600 - raise "diego.ssh_proxy.max_connection_duration_in_seconds must be between 1 and 3600, or empty for unlimited" + if max_connection_duration < 1 + raise "diego.ssh_proxy.max_connection_duration_in_seconds must be a positive integer, or empty for unlimited" end config[:max_connection_duration] = "#{max_connection_duration}s" end diff --git a/spec/ssh_proxy_template_spec.rb b/spec/ssh_proxy_template_spec.rb index 2c45d1e1a1..8f1517b40a 100644 --- a/spec/ssh_proxy_template_spec.rb +++ b/spec/ssh_proxy_template_spec.rb @@ -42,21 +42,21 @@ context 'when max_connection_duration_in_seconds is configured' do before do - deployment_manifest_fragment['diego']['ssh_proxy']['max_connection_duration_in_seconds'] = 3600 + deployment_manifest_fragment['diego']['ssh_proxy']['max_connection_duration_in_seconds'] = 86_400 end it 'renders the duration in seconds' do - expect(rendered_config['max_connection_duration']).to eq('3600s') + expect(rendered_config['max_connection_duration']).to eq('86400s') end end - context 'when max_connection_duration_in_seconds exceeds one hour' do + context 'when max_connection_duration_in_seconds is zero' do before do - deployment_manifest_fragment['diego']['ssh_proxy']['max_connection_duration_in_seconds'] = 3601 + deployment_manifest_fragment['diego']['ssh_proxy']['max_connection_duration_in_seconds'] = 0 end - it 'fails rendering' do - expect { rendered_config }.to raise_error(/must be between 1 and 3600/) + it 'fails rendering because finite durations must be positive' do + expect { rendered_config }.to raise_error(/must be a positive integer/) end end @@ -66,7 +66,7 @@ end it 'fails rendering' do - expect { rendered_config }.to raise_error(/must be an integer between 1 and 3600/) + expect { rendered_config }.to raise_error(/must be a positive integer/) end end end diff --git a/src/code.cloudfoundry.org/diego-ssh/cmd/ssh-proxy/config/config.go b/src/code.cloudfoundry.org/diego-ssh/cmd/ssh-proxy/config/config.go index 9be4322f58..abf3f659e1 100644 --- a/src/code.cloudfoundry.org/diego-ssh/cmd/ssh-proxy/config/config.go +++ b/src/code.cloudfoundry.org/diego-ssh/cmd/ssh-proxy/config/config.go @@ -6,7 +6,6 @@ import ( "encoding/json" "errors" "os" - "time" "code.cloudfoundry.org/debugserver" loggingclient "code.cloudfoundry.org/diego-logging-client" @@ -70,8 +69,8 @@ func NewSSHProxyConfig(configPath string) (SSHProxyConfig, error) { if err != nil { return SSHProxyConfig{}, err } - if proxyConfig.MaxConnectionDuration < 0 || proxyConfig.MaxConnectionDuration > durationjson.Duration(time.Hour) { - return SSHProxyConfig{}, errors.New("max_connection_duration must not exceed 1h") + if proxyConfig.MaxConnectionDuration < 0 { + return SSHProxyConfig{}, errors.New("max_connection_duration must not be negative") } return proxyConfig, nil diff --git a/src/code.cloudfoundry.org/diego-ssh/cmd/ssh-proxy/config/config_test.go b/src/code.cloudfoundry.org/diego-ssh/cmd/ssh-proxy/config/config_test.go index e91d883fcd..31a05a7939 100644 --- a/src/code.cloudfoundry.org/diego-ssh/cmd/ssh-proxy/config/config_test.go +++ b/src/code.cloudfoundry.org/diego-ssh/cmd/ssh-proxy/config/config_test.go @@ -129,14 +129,26 @@ var _ = Describe("SSHProxyConfig", func() { }) }) - Context("when the max connection duration exceeds one hour", func() { + Context("when the max connection duration is negative", func() { BeforeEach(func() { - configData = `{"max_connection_duration": "1h1s"}` + configData = `{"max_connection_duration": "-1s"}` }) It("returns an error", func() { _, err := config.NewSSHProxyConfig(configFilePath) - Expect(err).To(MatchError("max_connection_duration must not exceed 1h")) + Expect(err).To(MatchError("max_connection_duration must not be negative")) + }) + }) + + Context("when the max connection duration is greater than one hour", func() { + BeforeEach(func() { + configData = `{"max_connection_duration": "24h"}` + }) + + It("accepts the configured duration", func() { + proxyConfig, err := config.NewSSHProxyConfig(configFilePath) + Expect(err).NotTo(HaveOccurred()) + Expect(proxyConfig.MaxConnectionDuration).To(Equal(durationjson.Duration(24 * time.Hour))) }) }) From 44bf14e1f19963a53abcc0e972bbc2c5762cf632 Mon Sep 17 00:00:00 2001 From: rkoster Date: Wed, 30 Sep 2026 16:12:00 +0200 Subject: [PATCH 3/6] Enforce SSH session duration during backend setup --- .../diego-ssh/proxy/proxy.go | 46 ++++++-- .../diego-ssh/proxy/proxy_test.go | 104 +++++++++++++++++- 2 files changed, 132 insertions(+), 18 deletions(-) diff --git a/src/code.cloudfoundry.org/diego-ssh/proxy/proxy.go b/src/code.cloudfoundry.org/diego-ssh/proxy/proxy.go index c9ca941b9b..8e5b1ce6bd 100644 --- a/src/code.cloudfoundry.org/diego-ssh/proxy/proxy.go +++ b/src/code.cloudfoundry.org/diego-ssh/proxy/proxy.go @@ -1,6 +1,7 @@ package proxy import ( + "context" "crypto/tls" "encoding/json" "errors" @@ -41,8 +42,8 @@ type LogMessage struct { } type Proxy struct { - logger lager.Logger - serverConfig *ssh.ServerConfig + logger lager.Logger + serverConfig *ssh.ServerConfig connectionLock *sync.Mutex connections int @@ -79,19 +80,25 @@ func (p *Proxy) HandleConnection(netConn net.Conn) { } defer serverConn.Close() - clientConn, clientChannels, clientRequests, err := NewClientConn(logger, serverConn.Permissions, p.tlsConfig) - if err != nil { - return - } + ctx := context.Background() if p.maxConnectionDuration > 0 { - timer := time.AfterFunc(p.maxConnectionDuration, func() { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, p.maxConnectionDuration) + defer cancel() + stop := context.AfterFunc(ctx, func() { logger.Info("maximum-connection-duration-reached", lager.Data{"duration": p.maxConnectionDuration}) _ = serverConn.Close() - _ = clientConn.Close() }) - defer timer.Stop() + defer stop() } + clientConn, clientChannels, clientRequests, err := NewClientConn(ctx, logger, serverConn.Permissions, p.tlsConfig) + if err != nil { + return + } + stop := context.AfterFunc(ctx, func() { _ = clientConn.Close() }) + defer stop() + logMessage := extractLogMessage(logger, serverConn.Permissions) defer func() { @@ -332,7 +339,7 @@ func Wait(logger lager.Logger, waiters ...Waiter) { wg.Wait() } -func NewClientConn(logger lager.Logger, permissions *ssh.Permissions, tlsConfig *tls.Config) (ssh.Conn, <-chan ssh.NewChannel, <-chan *ssh.Request, error) { +func NewClientConn(ctx context.Context, logger lager.Logger, permissions *ssh.Permissions, tlsConfig *tls.Config) (ssh.Conn, <-chan ssh.NewChannel, <-chan *ssh.Request, error) { if permissions == nil || permissions.CriticalOptions == nil { err := errors.New("Invalid permissions from authentication") logger.Error("permissions-and-critical-options-required", err) @@ -356,7 +363,8 @@ func NewClientConn(logger lager.Logger, permissions *ssh.Permissions, tlsConfig dialer := func() (net.Conn, error) { tlsConfig := tlsConfigWithServerName(tlsConfig, targetConfig.ServerCertDomainSAN) if tlsConfig != nil && targetConfig.TLSAddress != "" { - nConn, err := tls.Dial("tcp", targetConfig.TLSAddress, tlsConfig) + tlsDialer := &tls.Dialer{Config: tlsConfig} + nConn, err := tlsDialer.DialContext(ctx, "tcp", targetConfig.TLSAddress) if err == nil { return nConn, nil } @@ -365,9 +373,12 @@ func NewClientConn(logger lager.Logger, permissions *ssh.Permissions, tlsConfig "tcp_address": targetConfig.TLSAddress, "server_cert_domain_san": targetConfig.ServerCertDomainSAN, }) + if ctx.Err() != nil { + return nil, ctx.Err() + } } - nConn, err := net.Dial("tcp", targetConfig.Address) + nConn, err := (&net.Dialer{}).DialContext(ctx, "tcp", targetConfig.Address) if err != nil { logger.Error("dial-failed", err, lager.Data{ "address": targetConfig.Address, @@ -382,6 +393,16 @@ func NewClientConn(logger lager.Logger, permissions *ssh.Permissions, tlsConfig if err != nil { return nil, nil, nil, err } + // SSH handshakes do not accept a context. Closing the socket interrupts a + // stalled handshake when the same deadline used for dialing expires. + stop := context.AfterFunc(ctx, func() { _ = nConn.Close() }) + defer stop() + handshakeComplete := false + defer func() { + if !handshakeComplete { + _ = nConn.Close() + } + }() logger.Info("connected-to-backend", lager.Data{ "backend-address": nConn.RemoteAddr().String(), @@ -439,6 +460,7 @@ func NewClientConn(logger lager.Logger, permissions *ssh.Permissions, tlsConfig return nil, nil, nil, err } + handshakeComplete = true return conn, ch, req, nil } diff --git a/src/code.cloudfoundry.org/diego-ssh/proxy/proxy_test.go b/src/code.cloudfoundry.org/diego-ssh/proxy/proxy_test.go index df3edc0ec4..0f28b1d669 100644 --- a/src/code.cloudfoundry.org/diego-ssh/proxy/proxy_test.go +++ b/src/code.cloudfoundry.org/diego-ssh/proxy/proxy_test.go @@ -1,6 +1,7 @@ package proxy_test import ( + "context" "crypto/tls" "encoding/json" "errors" @@ -66,12 +67,19 @@ var _ = Describe("Proxy", func() { proxyServer *server.Server sshdServer *server.Server - proxyDone chan struct{} - daemonDone chan struct{} + proxyDone chan struct{} + daemonDone chan struct{} maxConnectionDuration time.Duration + idleConnectionTimeout time.Duration + backendTLSConfig *tls.Config + handledConnections chan struct{} ) BeforeEach(func() { + maxConnectionDuration = 0 + idleConnectionTimeout = 500 * time.Millisecond + backendTLSConfig = nil + handledConnections = make(chan struct{}, 10) proxyDone = make(chan struct{}) daemonDone = make(chan struct{}) @@ -130,8 +138,13 @@ var _ = Describe("Proxy", func() { }) JustBeforeEach(func() { - sshProxy = proxy.New(logger.Session("proxy"), proxySSHConfig, fakeMetronClient, nil, maxConnectionDuration) - proxyServer = server.NewServer(logger.Session("proxy-server"), "", sshProxy, 500*time.Millisecond) + sshProxy = proxy.New(logger.Session("proxy"), proxySSHConfig, fakeMetronClient, backendTLSConfig, maxConnectionDuration) + handler := &server_fakes.FakeConnectionHandler{} + handler.HandleConnectionStub = func(conn net.Conn) { + sshProxy.HandleConnection(conn) + handledConnections <- struct{}{} + } + proxyServer = server.NewServer(logger.Session("proxy-server"), "", handler, idleConnectionTimeout) proxyServer.SetListener(proxyListener) go func() { proxyServer.Serve() @@ -214,10 +227,72 @@ var _ = Describe("Proxy", func() { Context("when the maximum connection duration is reached", func() { BeforeEach(func() { maxConnectionDuration = 100 * time.Millisecond + idleConnectionTimeout = 5 * time.Second }) It("closes the SSH connection even while it is active", func() { - Eventually(client.Wait).Should(HaveOccurred()) + closed := make(chan error, 1) + go func() { closed <- client.Wait() }() + Eventually(closed).Should(Receive(HaveOccurred())) + Eventually(handledConnections).Should(Receive()) + }) + }) + + Context("when backend setup stalls", func() { + var backend net.Listener + var backendClosed chan struct{} + + BeforeEach(func() { + maxConnectionDuration = 200 * time.Millisecond + idleConnectionTimeout = 5 * time.Second + var err error + backend, err = net.Listen("tcp", "127.0.0.1:0") + Expect(err).NotTo(HaveOccurred()) + DeferCleanup(backend.Close) + backendClosed = make(chan struct{}) + go func() { + defer GinkgoRecover() + conn, err := backend.Accept() + if err != nil { + return + } + defer conn.Close() + // Bound cleanup even if the proxy fails to close this socket. + Expect(conn.SetDeadline(time.Now().Add(5 * time.Second))).To(Succeed()) + _, err = io.Copy(io.Discard, conn) + if err == nil { + close(backendClosed) + } + }() + + targetJSON, err := json.Marshal(proxy.TargetConfig{ + Address: backend.Addr().String(), + TLSAddress: backend.Addr().String(), + ServerCertDomainSAN: "backend.example", + }) + Expect(err).NotTo(HaveOccurred()) + proxyAuthenticator.AuthenticateReturns(&ssh.Permissions{ + CriticalOptions: map[string]string{"proxy-target-config": string(targetJSON)}, + }, nil) + }) + + assertSetupExpires := func() { + defer client.Close() + closed := make(chan error, 1) + go func() { closed <- client.Wait() }() + Eventually(closed, 2*time.Second).Should(Receive(HaveOccurred())) + Eventually(backendClosed, 2*time.Second).Should(BeClosed()) + Eventually(handledConnections, 2*time.Second).Should(Receive()) + } + + It("closes both connections and returns when the backend SSH handshake stalls", assertSetupExpires) + + Context("with backend TLS enabled", func() { + BeforeEach(func() { + backendTLSConfig = &tls.Config{} + }) + + It("closes both connections and returns when the TLS handshake stalls", assertSetupExpires) }) }) @@ -1169,9 +1244,11 @@ var _ = Describe("Proxy", func() { newClientConnErr error tlsCfg *tls.Config + ctx context.Context ) BeforeEach(func() { + ctx = context.Background() permissions = &ssh.Permissions{ CriticalOptions: map[string]string{}, } @@ -1191,13 +1268,28 @@ var _ = Describe("Proxy", func() { sshdServer.SetListener(sshdListener) go sshdServer.Serve() - _, _, _, newClientConnErr = proxy.NewClientConn(logger, permissions, tlsCfg) + _, _, _, newClientConnErr = proxy.NewClientConn(ctx, logger, permissions, tlsCfg) }) AfterEach(func() { sshdServer.Shutdown() }) + Context("when backend dialing is cancelled", func() { + BeforeEach(func() { + var cancel context.CancelFunc + ctx, cancel = context.WithCancel(context.Background()) + cancel() + targetJSON, err := json.Marshal(proxy.TargetConfig{Address: sshdListener.Addr().String()}) + Expect(err).NotTo(HaveOccurred()) + permissions.CriticalOptions["proxy-target-config"] = string(targetJSON) + }) + + It("returns the cancellation error without establishing a connection", func() { + Expect(errors.Is(newClientConnErr, context.Canceled)).To(BeTrue()) + }) + }) + Context("when permissions is nil", func() { BeforeEach(func() { permissions = nil From 7b8feb55f8300ee26c17677ba28676f2539776c7 Mon Sep 17 00:00:00 2001 From: rkoster Date: Wed, 30 Sep 2026 16:24:14 +0200 Subject: [PATCH 4/6] Use zero for unlimited SSH session duration --- jobs/ssh_proxy/spec | 4 ++-- jobs/ssh_proxy/templates/ssh_proxy.json.erb | 16 ++++------------ spec/ssh_proxy_template_spec.rb | 20 +++++++++++--------- 3 files changed, 17 insertions(+), 23 deletions(-) diff --git a/jobs/ssh_proxy/spec b/jobs/ssh_proxy/spec index 8e0fb6936b..5d5ffdf8ae 100644 --- a/jobs/ssh_proxy/spec +++ b/jobs/ssh_proxy/spec @@ -88,8 +88,8 @@ properties: description: Idle timeout for incoming connections default: 300 diego.ssh_proxy.max_connection_duration_in_seconds: - description: Maximum lifetime of an SSH connection, in seconds. Leave empty for unlimited. - default: "" + description: Maximum lifetime of an SSH connection, in seconds. Must be a non-negative integer; 0 means unlimited. + default: 0 diego.ssh_proxy.uaa.url: description: The domain name of the UAA diff --git a/jobs/ssh_proxy/templates/ssh_proxy.json.erb b/jobs/ssh_proxy/templates/ssh_proxy.json.erb index 68a17e00a5..5d08bdaacb 100644 --- a/jobs/ssh_proxy/templates/ssh_proxy.json.erb +++ b/jobs/ssh_proxy/templates/ssh_proxy.json.erb @@ -40,18 +40,10 @@ end max_connection_duration = p("diego.ssh_proxy.max_connection_duration_in_seconds") - unless max_connection_duration.to_s.empty? - unless max_connection_duration.is_a?(Integer) || max_connection_duration.to_s.match(/\A\d+\z/) - raise "diego.ssh_proxy.max_connection_duration_in_seconds must be a positive integer, or empty for unlimited" - end - begin - max_connection_duration = Integer(max_connection_duration) - rescue ArgumentError, TypeError - raise "diego.ssh_proxy.max_connection_duration_in_seconds must be a positive integer, or empty for unlimited" - end - if max_connection_duration < 1 - raise "diego.ssh_proxy.max_connection_duration_in_seconds must be a positive integer, or empty for unlimited" - end + unless max_connection_duration.is_a?(Integer) && max_connection_duration >= 0 + raise "diego.ssh_proxy.max_connection_duration_in_seconds must be a non-negative integer; 0 means unlimited" + end + if max_connection_duration > 0 config[:max_connection_duration] = "#{max_connection_duration}s" end diff --git a/spec/ssh_proxy_template_spec.rb b/spec/ssh_proxy_template_spec.rb index 8f1517b40a..0deb82d9e6 100644 --- a/spec/ssh_proxy_template_spec.rb +++ b/spec/ssh_proxy_template_spec.rb @@ -34,7 +34,7 @@ let(:template) { job.template('config/ssh_proxy.json') } let(:rendered_config) { JSON.parse(template.render(deployment_manifest_fragment)) } - context 'when max_connection_duration_in_seconds is empty' do + context 'when max_connection_duration_in_seconds is not configured' do it 'omits the connection duration to allow unlimited sessions' do expect(rendered_config).not_to have_key('max_connection_duration') end @@ -55,18 +55,20 @@ deployment_manifest_fragment['diego']['ssh_proxy']['max_connection_duration_in_seconds'] = 0 end - it 'fails rendering because finite durations must be positive' do - expect { rendered_config }.to raise_error(/must be a positive integer/) + it 'omits the connection duration to allow unlimited sessions' do + expect(rendered_config).not_to have_key('max_connection_duration') end end - context 'when max_connection_duration_in_seconds is not an integer' do - before do - deployment_manifest_fragment['diego']['ssh_proxy']['max_connection_duration_in_seconds'] = 1.5 - end + [-1, 1.5, '', '3600', false].each do |value| + context "when max_connection_duration_in_seconds is #{value.inspect}" do + before do + deployment_manifest_fragment['diego']['ssh_proxy']['max_connection_duration_in_seconds'] = value + end - it 'fails rendering' do - expect { rendered_config }.to raise_error(/must be a positive integer/) + it 'rejects values that are not non-negative integers' do + expect { rendered_config }.to raise_error(/must be a non-negative integer/) + end end end end From 11e17fb215947ca2038e92bc753af5283f67ad92 Mon Sep 17 00:00:00 2001 From: rkoster Date: Wed, 30 Sep 2026 21:38:56 +0200 Subject: [PATCH 5/6] Improve SSH duration tests and logging --- jobs/ssh_proxy/spec | 2 +- .../cmd/ssh-proxy/config/config_test.go | 24 +++++++ .../diego-ssh/proxy/proxy.go | 14 ++-- .../diego-ssh/proxy/proxy_test.go | 65 ++++++++++++++++++- 4 files changed, 98 insertions(+), 7 deletions(-) diff --git a/jobs/ssh_proxy/spec b/jobs/ssh_proxy/spec index 5d5ffdf8ae..a005ed30d2 100644 --- a/jobs/ssh_proxy/spec +++ b/jobs/ssh_proxy/spec @@ -88,7 +88,7 @@ properties: description: Idle timeout for incoming connections default: 300 diego.ssh_proxy.max_connection_duration_in_seconds: - description: Maximum lifetime of an SSH connection, in seconds. Must be a non-negative integer; 0 means unlimited. + description: Maximum lifetime of an SSH connection, in seconds, measured from successful client authentication and including backend connection setup. On expiry, the connection and all its channels are closed abruptly without a reason message to the client. Must be a non-negative integer; 0 means unlimited. default: 0 diego.ssh_proxy.uaa.url: diff --git a/src/code.cloudfoundry.org/diego-ssh/cmd/ssh-proxy/config/config_test.go b/src/code.cloudfoundry.org/diego-ssh/cmd/ssh-proxy/config/config_test.go index 31a05a7939..3949c727d5 100644 --- a/src/code.cloudfoundry.org/diego-ssh/cmd/ssh-proxy/config/config_test.go +++ b/src/code.cloudfoundry.org/diego-ssh/cmd/ssh-proxy/config/config_test.go @@ -140,6 +140,30 @@ var _ = Describe("SSHProxyConfig", func() { }) }) + Context("when the max connection duration is omitted", func() { + BeforeEach(func() { + configData = `{}` + }) + + It("defaults to zero for unlimited sessions", func() { + proxyConfig, err := config.NewSSHProxyConfig(configFilePath) + Expect(err).NotTo(HaveOccurred()) + Expect(proxyConfig.MaxConnectionDuration).To(Equal(durationjson.Duration(0))) + }) + }) + + Context("when the max connection duration is zero", func() { + BeforeEach(func() { + configData = `{"max_connection_duration": "0s"}` + }) + + It("accepts zero for unlimited sessions", func() { + proxyConfig, err := config.NewSSHProxyConfig(configFilePath) + Expect(err).NotTo(HaveOccurred()) + Expect(proxyConfig.MaxConnectionDuration).To(Equal(durationjson.Duration(0))) + }) + }) + Context("when the max connection duration is greater than one hour", func() { BeforeEach(func() { configData = `{"max_connection_duration": "24h"}` diff --git a/src/code.cloudfoundry.org/diego-ssh/proxy/proxy.go b/src/code.cloudfoundry.org/diego-ssh/proxy/proxy.go index 8e5b1ce6bd..fc24f2f579 100644 --- a/src/code.cloudfoundry.org/diego-ssh/proxy/proxy.go +++ b/src/code.cloudfoundry.org/diego-ssh/proxy/proxy.go @@ -86,7 +86,7 @@ func (p *Proxy) HandleConnection(netConn net.Conn) { ctx, cancel = context.WithTimeout(ctx, p.maxConnectionDuration) defer cancel() stop := context.AfterFunc(ctx, func() { - logger.Info("maximum-connection-duration-reached", lager.Data{"duration": p.maxConnectionDuration}) + logger.Info("maximum-connection-duration-reached", lager.Data{"duration-in-seconds": p.maxConnectionDuration.Seconds()}) _ = serverConn.Close() }) defer stop() @@ -96,8 +96,10 @@ func (p *Proxy) HandleConnection(netConn net.Conn) { if err != nil { return } - stop := context.AfterFunc(ctx, func() { _ = clientConn.Close() }) - defer stop() + if ctx.Done() != nil { + stop := context.AfterFunc(ctx, func() { _ = clientConn.Close() }) + defer stop() + } logMessage := extractLogMessage(logger, serverConn.Permissions) @@ -395,8 +397,10 @@ func NewClientConn(ctx context.Context, logger lager.Logger, permissions *ssh.Pe } // SSH handshakes do not accept a context. Closing the socket interrupts a // stalled handshake when the same deadline used for dialing expires. - stop := context.AfterFunc(ctx, func() { _ = nConn.Close() }) - defer stop() + if ctx.Done() != nil { + stop := context.AfterFunc(ctx, func() { _ = nConn.Close() }) + defer stop() + } handshakeComplete := false defer func() { if !handshakeComplete { diff --git a/src/code.cloudfoundry.org/diego-ssh/proxy/proxy_test.go b/src/code.cloudfoundry.org/diego-ssh/proxy/proxy_test.go index 0f28b1d669..08c7be78df 100644 --- a/src/code.cloudfoundry.org/diego-ssh/proxy/proxy_test.go +++ b/src/code.cloudfoundry.org/diego-ssh/proxy/proxy_test.go @@ -225,16 +225,79 @@ var _ = Describe("Proxy", func() { }) Context("when the maximum connection duration is reached", func() { + var backendChannelClosed chan struct{} + BeforeEach(func() { - maxConnectionDuration = 100 * time.Millisecond + maxConnectionDuration = time.Second idleConnectionTimeout = 5 * time.Second + backendChannelClosed = make(chan struct{}) + handler := &fake_handlers.FakeNewChannelHandler{} + handler.HandleNewChannelStub = func(logger lager.Logger, newChannel ssh.NewChannel) { + defer GinkgoRecover() + channel, requests, err := newChannel.Accept() + Expect(err).NotTo(HaveOccurred()) + defer channel.Close() + defer close(backendChannelClosed) + go ssh.DiscardRequests(requests) + _, _ = io.Copy(channel, channel) + } + daemonNewChannelHandlers["session"] = handler }) It("closes the SSH connection even while it is active", func() { + defer client.Close() + channel, requests, err := client.OpenChannel("session", nil) + Expect(err).NotTo(HaveOccurred()) + defer channel.Close() + go ssh.DiscardRequests(requests) + + traffic := make(chan struct{}, 256) + streamEnded := make(chan error, 1) + go func() { + defer GinkgoRecover() + ticker := time.NewTicker(10 * time.Millisecond) + defer ticker.Stop() + for range ticker.C { + if _, err := channel.Write([]byte("ping")); err != nil { + streamEnded <- err + return + } + response := make([]byte, 4) + if _, err := io.ReadFull(channel, response); err != nil { + streamEnded <- err + return + } + Expect(string(response)).To(Equal("ping")) + select { + case traffic <- struct{}{}: + default: + } + } + }() + + // Establish that data travels through both proxy directions, + // then keep streaming until the lifetime closes the channel. + Eventually(traffic).Should(Receive()) + Eventually(traffic).Should(Receive()) + Eventually(streamEnded, 3*time.Second).Should(Receive(HaveOccurred())) + Eventually(backendChannelClosed).Should(BeClosed()) closed := make(chan error, 1) go func() { closed <- client.Wait() }() Eventually(closed).Should(Receive(HaveOccurred())) Eventually(handledConnections).Should(Receive()) + Eventually(logger).Should(gbytes.Say(`maximum-connection-duration-reached.*"duration-in-seconds":1`)) + }) + }) + + Context("when the client closes before the maximum duration", func() { + BeforeEach(func() { + maxConnectionDuration = 200 * time.Millisecond + }) + + It("does not log a timeout on normal closure or after the cancelled deadline", func() { + Expect(client.Close()).To(Succeed()) + Eventually(handledConnections).Should(Receive()) + Consistently(logger, 300*time.Millisecond).ShouldNot(gbytes.Say("maximum-connection-duration-reached")) }) }) From c525a642f06b06e1ff1c231debfd844b25b65def Mon Sep 17 00:00:00 2001 From: rkoster Date: Wed, 30 Sep 2026 21:51:12 +0200 Subject: [PATCH 6/6] Load ipaddr in SSH proxy template --- jobs/ssh_proxy/templates/ssh_proxy.json.erb | 1 + spec/ssh_proxy_template_spec.rb | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/jobs/ssh_proxy/templates/ssh_proxy.json.erb b/jobs/ssh_proxy/templates/ssh_proxy.json.erb index 5d08bdaacb..8e3a25e1fb 100644 --- a/jobs/ssh_proxy/templates/ssh_proxy.json.erb +++ b/jobs/ssh_proxy/templates/ssh_proxy.json.erb @@ -1,3 +1,4 @@ +<% require 'ipaddr' %> <%= def parse_ip (ip, var_name) diff --git a/spec/ssh_proxy_template_spec.rb b/spec/ssh_proxy_template_spec.rb index 0deb82d9e6..9b3795b6ec 100644 --- a/spec/ssh_proxy_template_spec.rb +++ b/spec/ssh_proxy_template_spec.rb @@ -3,7 +3,6 @@ # rubocop: disable Metrics/BlockLength require 'rspec' require 'json' -require 'ipaddr' require 'bosh/template/test' describe 'ssh_proxy' do