# HG changeset patch # User Igor Drozdov <idrozdov@gitlab.com> # Date 1652951065 -14400 # Thu May 19 13:04:25 2022 +0400 # Node ID 3a4948692b5d655284883e7d173f4b2a7174f683 # Parent b5a0002e1a3887d92469d212c04d461bda32c6ef Allow specifying formatted durations in config - If an integer is specified, we assume that these are seconds - A duration of format "500ms", "10s", "1m", etc... accepted diff --git a/cmd/gitlab-sshd/main.go b/cmd/gitlab-sshd/main.go --- a/cmd/gitlab-sshd/main.go +++ b/cmd/gitlab-sshd/main.go @@ -99,11 +99,12 @@ sig := <-done signal.Reset(syscall.SIGINT, syscall.SIGTERM) - log.WithContextFields(ctx, log.Fields{"shutdown_timeout_s": cfg.Server.GracePeriodSeconds, "signal": sig.String()}).Info("Shutdown initiated") + gracePeriod := time.Duration(cfg.Server.GracePeriod) + log.WithContextFields(ctx, log.Fields{"shutdown_timeout_s": gracePeriod.Seconds(), "signal": sig.String()}).Info("Shutdown initiated") server.Shutdown() - <-time.After(cfg.Server.GracePeriod()) + <-time.After(gracePeriod) cancel() diff --git a/internal/config/config.go b/internal/config/config.go --- a/internal/config/config.go +++ b/internal/config/config.go @@ -21,20 +21,22 @@ defaultSecretFileName = ".gitlab_shell_secret" ) +type yamlDuration time.Duration + type ServerConfig struct { - Listen string `yaml:"listen,omitempty"` - ProxyProtocol bool `yaml:"proxy_protocol,omitempty"` - ProxyPolicy string `yaml:"proxy_policy,omitempty"` - WebListen string `yaml:"web_listen,omitempty"` - ConcurrentSessionsLimit int64 `yaml:"concurrent_sessions_limit,omitempty"` - ClientAliveIntervalSeconds int64 `yaml:"client_alive_interval,omitempty"` - GracePeriodSeconds uint64 `yaml:"grace_period"` - ReadinessProbe string `yaml:"readiness_probe"` - LivenessProbe string `yaml:"liveness_probe"` - HostKeyFiles []string `yaml:"host_key_files,omitempty"` - MACs []string `yaml:"macs"` - KexAlgorithms []string `yaml:"kex_algorithms"` - Ciphers []string `yaml:"ciphers"` + Listen string `yaml:"listen,omitempty"` + ProxyProtocol bool `yaml:"proxy_protocol,omitempty"` + ProxyPolicy string `yaml:"proxy_policy,omitempty"` + WebListen string `yaml:"web_listen,omitempty"` + ConcurrentSessionsLimit int64 `yaml:"concurrent_sessions_limit,omitempty"` + ClientAliveInterval yamlDuration `yaml:"client_alive_interval,omitempty"` + GracePeriod yamlDuration `yaml:"grace_period"` + ReadinessProbe string `yaml:"readiness_probe"` + LivenessProbe string `yaml:"liveness_probe"` + HostKeyFiles []string `yaml:"host_key_files,omitempty"` + MACs []string `yaml:"macs"` + KexAlgorithms []string `yaml:"kex_algorithms"` + Ciphers []string `yaml:"ciphers"` } type HttpSettingsConfig struct { @@ -79,13 +81,13 @@ } DefaultServerConfig = ServerConfig{ - Listen: "[::]:22", - WebListen: "localhost:9122", - ConcurrentSessionsLimit: 10, - GracePeriodSeconds: 10, - ClientAliveIntervalSeconds: 15, - ReadinessProbe: "/start", - LivenessProbe: "/health", + Listen: "[::]:22", + WebListen: "localhost:9122", + ConcurrentSessionsLimit: 10, + GracePeriod: yamlDuration(10 * time.Second), + ClientAliveInterval: yamlDuration(15 * time.Second), + ReadinessProbe: "/start", + LivenessProbe: "/health", HostKeyFiles: []string{ "/run/secrets/ssh-hostkeys/ssh_host_rsa_key", "/run/secrets/ssh-hostkeys/ssh_host_ecdsa_key", @@ -94,12 +96,15 @@ } ) -func (sc *ServerConfig) ClientAliveInterval() time.Duration { - return time.Duration(sc.ClientAliveIntervalSeconds) * time.Second -} +func (d *yamlDuration) UnmarshalYAML(unmarshal func(interface{}) error) error { + var intDuration int + if err := unmarshal(&intDuration); err != nil { + return unmarshal((*time.Duration)(d)) + } -func (sc *ServerConfig) GracePeriod() time.Duration { - return time.Duration(sc.GracePeriodSeconds) * time.Second + *d = yamlDuration(time.Duration(intDuration) * time.Second) + + return nil } func (c *Config) ApplyGlobalState() { diff --git a/internal/config/config_test.go b/internal/config/config_test.go --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -3,7 +3,9 @@ import ( "os" "testing" + "time" + yaml "gopkg.in/yaml.v2" "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/require" @@ -58,3 +60,39 @@ require.Equal(t, expectedMetricNames, actualNames) } + +func TestNewFromDir(t *testing.T) { + testhelper.PrepareTestRootDir(t) + + cfg, err := NewFromDir(testhelper.TestRoot) + require.NoError(t, err) + + require.Equal(t, 10 * time.Second, time.Duration(cfg.Server.GracePeriod)) + require.Equal(t, 1 * time.Minute, time.Duration(cfg.Server.ClientAliveInterval)) +} + +func TestYAMLDuration(t *testing.T) { + testCases := []struct{ + desc string + data string + duration time.Duration + }{ + {"seconds assumed by default", "duration: 10", 10 * time.Second}, + {"milliseconds are parsed", "duration: 500ms", 500 * time.Millisecond}, + {"minutes are parsed", "duration: 1m", 1 * time.Minute}, + } + + type durationCfg struct { + Duration yamlDuration `yaml:"duration"` + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + var cfg durationCfg + err := yaml.Unmarshal([]byte(tc.data), &cfg) + require.NoError(t, err) + + require.Equal(t, tc.duration, time.Duration(cfg.Duration)) + }) + } +} diff --git a/internal/sshd/connection.go b/internal/sshd/connection.go --- a/internal/sshd/connection.go +++ b/internal/sshd/connection.go @@ -46,8 +46,8 @@ func (c *connection) handle(ctx context.Context, chans <-chan ssh.NewChannel, handler channelHandler) { ctxlog := log.WithContextFields(ctx, log.Fields{"remote_addr": c.remoteAddr}) - if c.cfg.Server.ClientAliveIntervalSeconds > 0 { - ticker := time.NewTicker(c.cfg.Server.ClientAliveInterval()) + if c.cfg.Server.ClientAliveInterval > 0 { + ticker := time.NewTicker(time.Duration(c.cfg.Server.ClientAliveInterval)) defer ticker.Stop() go c.sendKeepAliveMsg(ctx, ticker) } diff --git a/internal/sshd/connection_test.go b/internal/sshd/connection_test.go --- a/internal/sshd/connection_test.go +++ b/internal/sshd/connection_test.go @@ -80,7 +80,7 @@ } func setup(sessionsNum int64, newChannel *fakeNewChannel) (*connection, chan ssh.NewChannel) { - cfg := &config.Config{Server: config.ServerConfig{ConcurrentSessionsLimit: sessionsNum, ClientAliveIntervalSeconds: 1}} + cfg := &config.Config{Server: config.ServerConfig{ConcurrentSessionsLimit: sessionsNum}} conn := newConnection(cfg, "127.0.0.1:50000", &ssh.ServerConn{&fakeConn{}, nil}) chans := make(chan ssh.NewChannel, 1) diff --git a/internal/sshd/sshd_test.go b/internal/sshd/sshd_test.go --- a/internal/sshd/sshd_test.go +++ b/internal/sshd/sshd_test.go @@ -265,7 +265,6 @@ cfg.User = user cfg.Server.Listen = serverUrl cfg.Server.ConcurrentSessionsLimit = 1 - cfg.Server.ClientAliveIntervalSeconds = 15 cfg.Server.HostKeyFiles = []string{path.Join(testhelper.TestRoot, "certs/valid/server.key")} s, err := NewServer(cfg) diff --git a/internal/testhelper/testdata/testroot/config.yml b/internal/testhelper/testdata/testroot/config.yml --- a/internal/testhelper/testdata/testroot/config.yml +++ b/internal/testhelper/testdata/testroot/config.yml @@ -0,0 +1,3 @@ +sshd: + grace_period: 10 + client_alive_interval: 1m # HG changeset patch # User Igor Drozdov <idrozdov@gitlab.com> # Date 1652951414 -14400 # Thu May 19 13:10:14 2022 +0400 # Node ID 615e6c9ba39d727a4f4941c36a00aa83011bacd6 # Parent 3a4948692b5d655284883e7d173f4b2a7174f683 Make ProxyHeaderTimeout configurable Issue: https://gitlab.com/gitlab-org/gitlab-shell/-/issues/576 ProxyHeaderTimeout must be small to avoid DoS risk Let's make the value configurable and 500ms by default diff --git a/config.yml.example b/config.yml.example --- a/config.yml.example +++ b/config.yml.example @@ -80,6 +80,8 @@ client_alive_interval: 15 # The server waits for this time (in seconds) for the ongoing connections to complete before shutting down. Defaults to 10. grace_period: 10 + # A short timeout to decide to abort the connection if the protocol header is not seen within it. Defaults to 500ms + proxy_header_timeout: 500ms # The endpoint that returns 200 OK if the server is ready to receive incoming connections; otherwise, it returns 503 Service Unavailable. Defaults to "/start". readiness_probe: "/start" # The endpoint that returns 200 OK if the server is alive. Defaults to "/health". diff --git a/internal/config/config.go b/internal/config/config.go --- a/internal/config/config.go +++ b/internal/config/config.go @@ -31,6 +31,7 @@ ConcurrentSessionsLimit int64 `yaml:"concurrent_sessions_limit,omitempty"` ClientAliveInterval yamlDuration `yaml:"client_alive_interval,omitempty"` GracePeriod yamlDuration `yaml:"grace_period"` + ProxyHeaderTimeout yamlDuration `yaml:"proxy_header_timeout"` ReadinessProbe string `yaml:"readiness_probe"` LivenessProbe string `yaml:"liveness_probe"` HostKeyFiles []string `yaml:"host_key_files,omitempty"` @@ -86,6 +87,7 @@ ConcurrentSessionsLimit: 10, GracePeriod: yamlDuration(10 * time.Second), ClientAliveInterval: yamlDuration(15 * time.Second), + ProxyHeaderTimeout: yamlDuration(500 * time.Millisecond), ReadinessProbe: "/start", LivenessProbe: "/health", HostKeyFiles: []string{ diff --git a/internal/config/config_test.go b/internal/config/config_test.go --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -5,9 +5,9 @@ "testing" "time" - yaml "gopkg.in/yaml.v2" "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/require" + yaml "gopkg.in/yaml.v2" "gitlab.com/gitlab-org/gitlab-shell/client/testserver" "gitlab.com/gitlab-org/gitlab-shell/internal/testhelper" @@ -67,14 +67,15 @@ cfg, err := NewFromDir(testhelper.TestRoot) require.NoError(t, err) - require.Equal(t, 10 * time.Second, time.Duration(cfg.Server.GracePeriod)) - require.Equal(t, 1 * time.Minute, time.Duration(cfg.Server.ClientAliveInterval)) + require.Equal(t, 10*time.Second, time.Duration(cfg.Server.GracePeriod)) + require.Equal(t, 1*time.Minute, time.Duration(cfg.Server.ClientAliveInterval)) + require.Equal(t, 500*time.Millisecond, time.Duration(cfg.Server.ProxyHeaderTimeout)) } func TestYAMLDuration(t *testing.T) { - testCases := []struct{ - desc string - data string + testCases := []struct { + desc string + data string duration time.Duration }{ {"seconds assumed by default", "duration: 10", 10 * time.Second}, diff --git a/internal/sshd/sshd.go b/internal/sshd/sshd.go --- a/internal/sshd/sshd.go +++ b/internal/sshd/sshd.go @@ -26,7 +26,6 @@ StatusReady StatusOnShutdown StatusClosed - ProxyHeaderTimeout = 90 * time.Second ) type Server struct { @@ -97,7 +96,7 @@ sshListener = &proxyproto.Listener{ Listener: sshListener, Policy: s.requirePolicy, - ReadHeaderTimeout: ProxyHeaderTimeout, + ReadHeaderTimeout: time.Duration(s.Config.Server.ProxyHeaderTimeout), } log.ContextLogger(ctx).Info("Proxy protocol is enabled") diff --git a/internal/testhelper/testdata/testroot/config.yml b/internal/testhelper/testdata/testroot/config.yml --- a/internal/testhelper/testdata/testroot/config.yml +++ b/internal/testhelper/testdata/testroot/config.yml @@ -1,3 +1,4 @@ sshd: grace_period: 10 client_alive_interval: 1m + proxy_header_timeout: 500ms # HG changeset patch # User Stan Hu <stanhu@gmail.com> # Date 1652972660 0 # Thu May 19 15:04:20 2022 +0000 # Node ID 615b574a746d29556dfca756724aa990c7a3cc50 # Parent b5a0002e1a3887d92469d212c04d461bda32c6ef # Parent 615e6c9ba39d727a4f4941c36a00aa83011bacd6 Merge branch 'id-fix-proxy-header-timeout' into 'main' Make ProxyHeaderTimeout configurable See merge request gitlab-org/gitlab-shell!635 diff --git a/cmd/gitlab-sshd/main.go b/cmd/gitlab-sshd/main.go --- a/cmd/gitlab-sshd/main.go +++ b/cmd/gitlab-sshd/main.go @@ -99,11 +99,12 @@ sig := <-done signal.Reset(syscall.SIGINT, syscall.SIGTERM) - log.WithContextFields(ctx, log.Fields{"shutdown_timeout_s": cfg.Server.GracePeriodSeconds, "signal": sig.String()}).Info("Shutdown initiated") + gracePeriod := time.Duration(cfg.Server.GracePeriod) + log.WithContextFields(ctx, log.Fields{"shutdown_timeout_s": gracePeriod.Seconds(), "signal": sig.String()}).Info("Shutdown initiated") server.Shutdown() - <-time.After(cfg.Server.GracePeriod()) + <-time.After(gracePeriod) cancel() diff --git a/config.yml.example b/config.yml.example --- a/config.yml.example +++ b/config.yml.example @@ -80,6 +80,8 @@ client_alive_interval: 15 # The server waits for this time (in seconds) for the ongoing connections to complete before shutting down. Defaults to 10. grace_period: 10 + # A short timeout to decide to abort the connection if the protocol header is not seen within it. Defaults to 500ms + proxy_header_timeout: 500ms # The endpoint that returns 200 OK if the server is ready to receive incoming connections; otherwise, it returns 503 Service Unavailable. Defaults to "/start". readiness_probe: "/start" # The endpoint that returns 200 OK if the server is alive. Defaults to "/health". diff --git a/internal/config/config.go b/internal/config/config.go --- a/internal/config/config.go +++ b/internal/config/config.go @@ -21,20 +21,23 @@ defaultSecretFileName = ".gitlab_shell_secret" ) +type yamlDuration time.Duration + type ServerConfig struct { - Listen string `yaml:"listen,omitempty"` - ProxyProtocol bool `yaml:"proxy_protocol,omitempty"` - ProxyPolicy string `yaml:"proxy_policy,omitempty"` - WebListen string `yaml:"web_listen,omitempty"` - ConcurrentSessionsLimit int64 `yaml:"concurrent_sessions_limit,omitempty"` - ClientAliveIntervalSeconds int64 `yaml:"client_alive_interval,omitempty"` - GracePeriodSeconds uint64 `yaml:"grace_period"` - ReadinessProbe string `yaml:"readiness_probe"` - LivenessProbe string `yaml:"liveness_probe"` - HostKeyFiles []string `yaml:"host_key_files,omitempty"` - MACs []string `yaml:"macs"` - KexAlgorithms []string `yaml:"kex_algorithms"` - Ciphers []string `yaml:"ciphers"` + Listen string `yaml:"listen,omitempty"` + ProxyProtocol bool `yaml:"proxy_protocol,omitempty"` + ProxyPolicy string `yaml:"proxy_policy,omitempty"` + WebListen string `yaml:"web_listen,omitempty"` + ConcurrentSessionsLimit int64 `yaml:"concurrent_sessions_limit,omitempty"` + ClientAliveInterval yamlDuration `yaml:"client_alive_interval,omitempty"` + GracePeriod yamlDuration `yaml:"grace_period"` + ProxyHeaderTimeout yamlDuration `yaml:"proxy_header_timeout"` + ReadinessProbe string `yaml:"readiness_probe"` + LivenessProbe string `yaml:"liveness_probe"` + HostKeyFiles []string `yaml:"host_key_files,omitempty"` + MACs []string `yaml:"macs"` + KexAlgorithms []string `yaml:"kex_algorithms"` + Ciphers []string `yaml:"ciphers"` } type HttpSettingsConfig struct { @@ -79,13 +82,14 @@ } DefaultServerConfig = ServerConfig{ - Listen: "[::]:22", - WebListen: "localhost:9122", - ConcurrentSessionsLimit: 10, - GracePeriodSeconds: 10, - ClientAliveIntervalSeconds: 15, - ReadinessProbe: "/start", - LivenessProbe: "/health", + Listen: "[::]:22", + WebListen: "localhost:9122", + ConcurrentSessionsLimit: 10, + GracePeriod: yamlDuration(10 * time.Second), + ClientAliveInterval: yamlDuration(15 * time.Second), + ProxyHeaderTimeout: yamlDuration(500 * time.Millisecond), + ReadinessProbe: "/start", + LivenessProbe: "/health", HostKeyFiles: []string{ "/run/secrets/ssh-hostkeys/ssh_host_rsa_key", "/run/secrets/ssh-hostkeys/ssh_host_ecdsa_key", @@ -94,12 +98,15 @@ } ) -func (sc *ServerConfig) ClientAliveInterval() time.Duration { - return time.Duration(sc.ClientAliveIntervalSeconds) * time.Second -} +func (d *yamlDuration) UnmarshalYAML(unmarshal func(interface{}) error) error { + var intDuration int + if err := unmarshal(&intDuration); err != nil { + return unmarshal((*time.Duration)(d)) + } -func (sc *ServerConfig) GracePeriod() time.Duration { - return time.Duration(sc.GracePeriodSeconds) * time.Second + *d = yamlDuration(time.Duration(intDuration) * time.Second) + + return nil } func (c *Config) ApplyGlobalState() { diff --git a/internal/config/config_test.go b/internal/config/config_test.go --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -3,9 +3,11 @@ import ( "os" "testing" + "time" "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/require" + yaml "gopkg.in/yaml.v2" "gitlab.com/gitlab-org/gitlab-shell/client/testserver" "gitlab.com/gitlab-org/gitlab-shell/internal/testhelper" @@ -58,3 +60,40 @@ require.Equal(t, expectedMetricNames, actualNames) } + +func TestNewFromDir(t *testing.T) { + testhelper.PrepareTestRootDir(t) + + cfg, err := NewFromDir(testhelper.TestRoot) + require.NoError(t, err) + + require.Equal(t, 10*time.Second, time.Duration(cfg.Server.GracePeriod)) + require.Equal(t, 1*time.Minute, time.Duration(cfg.Server.ClientAliveInterval)) + require.Equal(t, 500*time.Millisecond, time.Duration(cfg.Server.ProxyHeaderTimeout)) +} + +func TestYAMLDuration(t *testing.T) { + testCases := []struct { + desc string + data string + duration time.Duration + }{ + {"seconds assumed by default", "duration: 10", 10 * time.Second}, + {"milliseconds are parsed", "duration: 500ms", 500 * time.Millisecond}, + {"minutes are parsed", "duration: 1m", 1 * time.Minute}, + } + + type durationCfg struct { + Duration yamlDuration `yaml:"duration"` + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + var cfg durationCfg + err := yaml.Unmarshal([]byte(tc.data), &cfg) + require.NoError(t, err) + + require.Equal(t, tc.duration, time.Duration(cfg.Duration)) + }) + } +} diff --git a/internal/sshd/connection.go b/internal/sshd/connection.go --- a/internal/sshd/connection.go +++ b/internal/sshd/connection.go @@ -46,8 +46,8 @@ func (c *connection) handle(ctx context.Context, chans <-chan ssh.NewChannel, handler channelHandler) { ctxlog := log.WithContextFields(ctx, log.Fields{"remote_addr": c.remoteAddr}) - if c.cfg.Server.ClientAliveIntervalSeconds > 0 { - ticker := time.NewTicker(c.cfg.Server.ClientAliveInterval()) + if c.cfg.Server.ClientAliveInterval > 0 { + ticker := time.NewTicker(time.Duration(c.cfg.Server.ClientAliveInterval)) defer ticker.Stop() go c.sendKeepAliveMsg(ctx, ticker) } diff --git a/internal/sshd/connection_test.go b/internal/sshd/connection_test.go --- a/internal/sshd/connection_test.go +++ b/internal/sshd/connection_test.go @@ -80,7 +80,7 @@ } func setup(sessionsNum int64, newChannel *fakeNewChannel) (*connection, chan ssh.NewChannel) { - cfg := &config.Config{Server: config.ServerConfig{ConcurrentSessionsLimit: sessionsNum, ClientAliveIntervalSeconds: 1}} + cfg := &config.Config{Server: config.ServerConfig{ConcurrentSessionsLimit: sessionsNum}} conn := newConnection(cfg, "127.0.0.1:50000", &ssh.ServerConn{&fakeConn{}, nil}) chans := make(chan ssh.NewChannel, 1) diff --git a/internal/sshd/sshd.go b/internal/sshd/sshd.go --- a/internal/sshd/sshd.go +++ b/internal/sshd/sshd.go @@ -26,7 +26,6 @@ StatusReady StatusOnShutdown StatusClosed - ProxyHeaderTimeout = 90 * time.Second ) type Server struct { @@ -97,7 +96,7 @@ sshListener = &proxyproto.Listener{ Listener: sshListener, Policy: s.requirePolicy, - ReadHeaderTimeout: ProxyHeaderTimeout, + ReadHeaderTimeout: time.Duration(s.Config.Server.ProxyHeaderTimeout), } log.ContextLogger(ctx).Info("Proxy protocol is enabled") diff --git a/internal/sshd/sshd_test.go b/internal/sshd/sshd_test.go --- a/internal/sshd/sshd_test.go +++ b/internal/sshd/sshd_test.go @@ -265,7 +265,6 @@ cfg.User = user cfg.Server.Listen = serverUrl cfg.Server.ConcurrentSessionsLimit = 1 - cfg.Server.ClientAliveIntervalSeconds = 15 cfg.Server.HostKeyFiles = []string{path.Join(testhelper.TestRoot, "certs/valid/server.key")} s, err := NewServer(cfg) diff --git a/internal/testhelper/testdata/testroot/config.yml b/internal/testhelper/testdata/testroot/config.yml --- a/internal/testhelper/testdata/testroot/config.yml +++ b/internal/testhelper/testdata/testroot/config.yml @@ -0,0 +1,4 @@ +sshd: + grace_period: 10 + client_alive_interval: 1m + proxy_header_timeout: 500ms # HG changeset patch # User Stan Hu <stanhu@gmail.com> # Date 1652984464 25200 # Thu May 19 11:21:04 2022 -0700 # Node ID 49a5f84c0ebe727653990d66163f182d1b3468dc # Parent 615b574a746d29556dfca756724aa990c7a3cc50 Release 14.5.0 - Make ProxyHeaderTimeout configurable !635 diff --git a/CHANGELOG b/CHANGELOG --- a/CHANGELOG +++ b/CHANGELOG @@ -1,3 +1,7 @@ +v14.5.0 + +- Make ProxyHeaderTimeout configurable !635 + v14.4.0 - Allow configuring SSH server algorithms !633 diff --git a/VERSION b/VERSION --- a/VERSION +++ b/VERSION @@ -1,1 +1,1 @@ -14.4.0 +14.5.0 # HG changeset patch # User Stan Hu <stanhu@gmail.com> # Date 1652984905 0 # Thu May 19 18:28:25 2022 +0000 # Node ID c1e06d31b6ef53cea0f58afa656b99c22fd38743 # Parent 615b574a746d29556dfca756724aa990c7a3cc50 # Parent 49a5f84c0ebe727653990d66163f182d1b3468dc Merge branch 'sh-release-14.5.0' into 'main' Release 14.5.0 See merge request gitlab-org/gitlab-shell!636 diff --git a/CHANGELOG b/CHANGELOG --- a/CHANGELOG +++ b/CHANGELOG @@ -1,3 +1,7 @@ +v14.5.0 + +- Make ProxyHeaderTimeout configurable !635 + v14.4.0 - Allow configuring SSH server algorithms !633 diff --git a/VERSION b/VERSION --- a/VERSION +++ b/VERSION @@ -1,1 +1,1 @@ -14.4.0 +14.5.0 # HG changeset patch # User Georges Racinet <georges.racinet@octobus.net> # Date 1664446728 -7200 # Thu Sep 29 12:18:48 2022 +0200 # Branch heptapod # Node ID aaebb02398004cb60c56d98824084bf9a2c17073 # Parent e8040b2b35ce6f600ac4d9bb0ffd03e5444f3402 # Parent c1e06d31b6ef53cea0f58afa656b99c22fd38743 Merged upstream GitLab Shell v14.5.0 and updated Heptapod version files accordingly diff --git a/CHANGELOG b/CHANGELOG --- a/CHANGELOG +++ b/CHANGELOG @@ -1,3 +1,7 @@ +v14.5.0 + +- Make ProxyHeaderTimeout configurable !635 + v14.4.0 - Allow configuring SSH server algorithms !633 diff --git a/HEPTAPOD_CHANGELOG b/HEPTAPOD_CHANGELOG --- a/HEPTAPOD_CHANGELOG +++ b/HEPTAPOD_CHANGELOG @@ -14,6 +14,10 @@ - Bump to upstream GitLab Shell v14.4.0 +## 14.5.0 + +- Bump to upstream GitLab Shell v14.5.0 + ## 14.3.2 - Bump to upstream GitLab Shell v14.3.1 diff --git a/HEPTAPOD_VERSION b/HEPTAPOD_VERSION --- a/HEPTAPOD_VERSION +++ b/HEPTAPOD_VERSION @@ -1,1 +1,1 @@ -14.4.0 +14.5.0 diff --git a/VERSION b/VERSION --- a/VERSION +++ b/VERSION @@ -1,1 +1,1 @@ -14.4.0 +14.5.0 diff --git a/cmd/gitlab-sshd/main.go b/cmd/gitlab-sshd/main.go --- a/cmd/gitlab-sshd/main.go +++ b/cmd/gitlab-sshd/main.go @@ -99,11 +99,12 @@ sig := <-done signal.Reset(syscall.SIGINT, syscall.SIGTERM) - log.WithContextFields(ctx, log.Fields{"shutdown_timeout_s": cfg.Server.GracePeriodSeconds, "signal": sig.String()}).Info("Shutdown initiated") + gracePeriod := time.Duration(cfg.Server.GracePeriod) + log.WithContextFields(ctx, log.Fields{"shutdown_timeout_s": gracePeriod.Seconds(), "signal": sig.String()}).Info("Shutdown initiated") server.Shutdown() - <-time.After(cfg.Server.GracePeriod()) + <-time.After(gracePeriod) cancel() diff --git a/config.yml.example b/config.yml.example --- a/config.yml.example +++ b/config.yml.example @@ -96,6 +96,8 @@ client_alive_interval: 15 # The server waits for this time (in seconds) for the ongoing connections to complete before shutting down. Defaults to 10. grace_period: 10 + # A short timeout to decide to abort the connection if the protocol header is not seen within it. Defaults to 500ms + proxy_header_timeout: 500ms # The endpoint that returns 200 OK if the server is ready to receive incoming connections; otherwise, it returns 503 Service Unavailable. Defaults to "/start". readiness_probe: "/start" # The endpoint that returns 200 OK if the server is alive. Defaults to "/health". diff --git a/internal/config/config.go b/internal/config/config.go --- a/internal/config/config.go +++ b/internal/config/config.go @@ -27,20 +27,23 @@ defaultHgrcPath string = "/opt/gitlab/etc/docker.hgrc:/etc/gitlab/heptapod.hgrc" ) +type yamlDuration time.Duration + type ServerConfig struct { - Listen string `yaml:"listen,omitempty"` - ProxyProtocol bool `yaml:"proxy_protocol,omitempty"` - ProxyPolicy string `yaml:"proxy_policy,omitempty"` - WebListen string `yaml:"web_listen,omitempty"` - ConcurrentSessionsLimit int64 `yaml:"concurrent_sessions_limit,omitempty"` - ClientAliveIntervalSeconds int64 `yaml:"client_alive_interval,omitempty"` - GracePeriodSeconds uint64 `yaml:"grace_period"` - ReadinessProbe string `yaml:"readiness_probe"` - LivenessProbe string `yaml:"liveness_probe"` - HostKeyFiles []string `yaml:"host_key_files,omitempty"` - MACs []string `yaml:"macs"` - KexAlgorithms []string `yaml:"kex_algorithms"` - Ciphers []string `yaml:"ciphers"` + Listen string `yaml:"listen,omitempty"` + ProxyProtocol bool `yaml:"proxy_protocol,omitempty"` + ProxyPolicy string `yaml:"proxy_policy,omitempty"` + WebListen string `yaml:"web_listen,omitempty"` + ConcurrentSessionsLimit int64 `yaml:"concurrent_sessions_limit,omitempty"` + ClientAliveInterval yamlDuration `yaml:"client_alive_interval,omitempty"` + GracePeriod yamlDuration `yaml:"grace_period"` + ProxyHeaderTimeout yamlDuration `yaml:"proxy_header_timeout"` + ReadinessProbe string `yaml:"readiness_probe"` + LivenessProbe string `yaml:"liveness_probe"` + HostKeyFiles []string `yaml:"host_key_files,omitempty"` + MACs []string `yaml:"macs"` + KexAlgorithms []string `yaml:"kex_algorithms"` + Ciphers []string `yaml:"ciphers"` } type HttpSettingsConfig struct { @@ -106,13 +109,14 @@ } DefaultServerConfig = ServerConfig{ - Listen: "[::]:22", - WebListen: "localhost:9122", - ConcurrentSessionsLimit: 10, - GracePeriodSeconds: 10, - ClientAliveIntervalSeconds: 15, - ReadinessProbe: "/start", - LivenessProbe: "/health", + Listen: "[::]:22", + WebListen: "localhost:9122", + ConcurrentSessionsLimit: 10, + GracePeriod: yamlDuration(10 * time.Second), + ClientAliveInterval: yamlDuration(15 * time.Second), + ProxyHeaderTimeout: yamlDuration(500 * time.Millisecond), + ReadinessProbe: "/start", + LivenessProbe: "/health", HostKeyFiles: []string{ "/run/secrets/ssh-hostkeys/ssh_host_rsa_key", "/run/secrets/ssh-hostkeys/ssh_host_ecdsa_key", @@ -121,12 +125,15 @@ } ) -func (sc *ServerConfig) ClientAliveInterval() time.Duration { - return time.Duration(sc.ClientAliveIntervalSeconds) * time.Second -} +func (d *yamlDuration) UnmarshalYAML(unmarshal func(interface{}) error) error { + var intDuration int + if err := unmarshal(&intDuration); err != nil { + return unmarshal((*time.Duration)(d)) + } -func (sc *ServerConfig) GracePeriod() time.Duration { - return time.Duration(sc.GracePeriodSeconds) * time.Second + *d = yamlDuration(time.Duration(intDuration) * time.Second) + + return nil } func (c *Config) ApplyGlobalState() { diff --git a/internal/config/config_test.go b/internal/config/config_test.go --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -3,9 +3,11 @@ import ( "os" "testing" + "time" "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/require" + yaml "gopkg.in/yaml.v2" "gitlab.com/gitlab-org/gitlab-shell/client/testserver" "gitlab.com/gitlab-org/gitlab-shell/internal/testhelper" @@ -58,3 +60,40 @@ require.Equal(t, expectedMetricNames, actualNames) } + +func TestNewFromDir(t *testing.T) { + testhelper.PrepareTestRootDir(t) + + cfg, err := NewFromDir(testhelper.TestRoot) + require.NoError(t, err) + + require.Equal(t, 10*time.Second, time.Duration(cfg.Server.GracePeriod)) + require.Equal(t, 1*time.Minute, time.Duration(cfg.Server.ClientAliveInterval)) + require.Equal(t, 500*time.Millisecond, time.Duration(cfg.Server.ProxyHeaderTimeout)) +} + +func TestYAMLDuration(t *testing.T) { + testCases := []struct { + desc string + data string + duration time.Duration + }{ + {"seconds assumed by default", "duration: 10", 10 * time.Second}, + {"milliseconds are parsed", "duration: 500ms", 500 * time.Millisecond}, + {"minutes are parsed", "duration: 1m", 1 * time.Minute}, + } + + type durationCfg struct { + Duration yamlDuration `yaml:"duration"` + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + var cfg durationCfg + err := yaml.Unmarshal([]byte(tc.data), &cfg) + require.NoError(t, err) + + require.Equal(t, tc.duration, time.Duration(cfg.Duration)) + }) + } +} diff --git a/internal/sshd/connection.go b/internal/sshd/connection.go --- a/internal/sshd/connection.go +++ b/internal/sshd/connection.go @@ -46,8 +46,8 @@ func (c *connection) handle(ctx context.Context, chans <-chan ssh.NewChannel, handler channelHandler) { ctxlog := log.WithContextFields(ctx, log.Fields{"remote_addr": c.remoteAddr}) - if c.cfg.Server.ClientAliveIntervalSeconds > 0 { - ticker := time.NewTicker(c.cfg.Server.ClientAliveInterval()) + if c.cfg.Server.ClientAliveInterval > 0 { + ticker := time.NewTicker(time.Duration(c.cfg.Server.ClientAliveInterval)) defer ticker.Stop() go c.sendKeepAliveMsg(ctx, ticker) } diff --git a/internal/sshd/connection_test.go b/internal/sshd/connection_test.go --- a/internal/sshd/connection_test.go +++ b/internal/sshd/connection_test.go @@ -80,7 +80,7 @@ } func setup(sessionsNum int64, newChannel *fakeNewChannel) (*connection, chan ssh.NewChannel) { - cfg := &config.Config{Server: config.ServerConfig{ConcurrentSessionsLimit: sessionsNum, ClientAliveIntervalSeconds: 1}} + cfg := &config.Config{Server: config.ServerConfig{ConcurrentSessionsLimit: sessionsNum}} conn := newConnection(cfg, "127.0.0.1:50000", &ssh.ServerConn{&fakeConn{}, nil}) chans := make(chan ssh.NewChannel, 1) diff --git a/internal/sshd/sshd.go b/internal/sshd/sshd.go --- a/internal/sshd/sshd.go +++ b/internal/sshd/sshd.go @@ -26,7 +26,6 @@ StatusReady StatusOnShutdown StatusClosed - ProxyHeaderTimeout = 90 * time.Second ) type Server struct { @@ -97,7 +96,7 @@ sshListener = &proxyproto.Listener{ Listener: sshListener, Policy: s.requirePolicy, - ReadHeaderTimeout: ProxyHeaderTimeout, + ReadHeaderTimeout: time.Duration(s.Config.Server.ProxyHeaderTimeout), } log.ContextLogger(ctx).Info("Proxy protocol is enabled") diff --git a/internal/sshd/sshd_test.go b/internal/sshd/sshd_test.go --- a/internal/sshd/sshd_test.go +++ b/internal/sshd/sshd_test.go @@ -265,7 +265,6 @@ cfg.User = user cfg.Server.Listen = serverUrl cfg.Server.ConcurrentSessionsLimit = 1 - cfg.Server.ClientAliveIntervalSeconds = 15 cfg.Server.HostKeyFiles = []string{path.Join(testhelper.TestRoot, "certs/valid/server.key")} s, err := NewServer(cfg) diff --git a/internal/testhelper/testdata/testroot/config.yml b/internal/testhelper/testdata/testroot/config.yml --- a/internal/testhelper/testdata/testroot/config.yml +++ b/internal/testhelper/testdata/testroot/config.yml @@ -0,0 +1,4 @@ +sshd: + grace_period: 10 + client_alive_interval: 1m + proxy_header_timeout: 500ms