Skip to content

Commit

Permalink
Allow a custom SSH dial timeout to be set cluster wide (#45507)
Browse files Browse the repository at this point in the history
A new SSHDialTimeout field is added to the ClusterNetworkingConfiguration
to allow cluster admins to override the default dial timeout of 30s.
The dial timeout is made available in /webapi/ping, stored in the tsh
profile, and cached on disk like other similar cluster settings so
that tsh ssh can consume the relevant information without requiring
an additional round trip to the cluster. At the moment, there is no
support for an unlimited dial timeout, as that would cause
complexity with backwards compatibility.
  • Loading branch information
rosstimothy committed Aug 21, 2024
1 parent 7fd4b04 commit 2ddc37a
Show file tree
Hide file tree
Showing 20 changed files with 2,790 additions and 1,897 deletions.
3 changes: 3 additions & 0 deletions api/client/webclient/webclient.go
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,9 @@ type SSHProxySettings struct {

// TunnelPublicAddr is the public address of the SSH reverse tunnel.
TunnelPublicAddr string `json:"ssh_tunnel_public_addr,omitempty"`

// DialTimeout indicates the SSH timeout clients should use.
DialTimeout time.Duration `json:"dial_timeout,omitempty"`
}

// DBProxySettings contains database access specific proxy settings.
Expand Down
11 changes: 11 additions & 0 deletions api/profile/profile.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
"golang.org/x/crypto/ssh"
"gopkg.in/yaml.v2"

"github.com/gravitational/teleport/api/defaults"
"github.com/gravitational/teleport/api/utils"
"github.com/gravitational/teleport/api/utils/keypaths"
"github.com/gravitational/teleport/api/utils/keys"
Expand Down Expand Up @@ -115,6 +116,9 @@ type Profile struct {
// SAMLSingleLogoutEnabled is whether SAML SLO (single logout) is enabled, this can only be true if this is a SAML SSO session
// using an auth connector with a SAML SLO URL configured.
SAMLSingleLogoutEnabled bool `yaml:"saml_slo_enabled,omitempty"`

// SSHDialTimeout is the timeout value that should be used for SSH connections.
SSHDialTimeout time.Duration `yaml:"ssh_dial_timeout,omitempty"`
}

// Copy returns a shallow copy of p, or nil if p is nil.
Expand Down Expand Up @@ -398,6 +402,13 @@ func profileFromFile(filePath string) (*Profile, error) {
if p.SiteName == "" {
p.SiteName = p.Name()
}

// For backwards compatibility, if the dial timeout was not set,
// then use the DefaultIOTimeout.
if p.SSHDialTimeout == 0 {
p.SSHDialTimeout = defaults.DefaultIOTimeout
}

return &p, nil
}

Expand Down
5 changes: 5 additions & 0 deletions api/profile/profile_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"github.com/gravitational/trace"
"github.com/stretchr/testify/require"

"github.com/gravitational/teleport/api/defaults"
"github.com/gravitational/teleport/api/profile"
)

Expand Down Expand Up @@ -75,6 +76,10 @@ func TestProfileBasics(t *testing.T) {
require.NoError(t, err)
require.Equal(t, p.Name(), name)

// Update the dial timeout because when the profile is loaded, an
// empty timeout is implicitly set to match the default value.
p.SSHDialTimeout = defaults.DefaultIOTimeout

// load and verify current profile
clone, err := profile.FromDir(dir, "")
require.NoError(t, err)
Expand Down
7 changes: 7 additions & 0 deletions api/proto/teleport/legacy/types/types.proto
Original file line number Diff line number Diff line change
Expand Up @@ -1787,6 +1787,13 @@ message ClusterNetworkingConfigSpecV2 {

// CaseInsensitiveRouting causes proxies to use case-insensitive hostname matching.
bool CaseInsensitiveRouting = 12 [(gogoproto.jsontag) = "case_insensitive_routing,omitempty"];

// SSHDialTimeout is a custom dial timeout used when establishing
// SSH connections. If not set, the default timeout of 30s will be used.
int64 SSHDialTimeout = 13 [
(gogoproto.jsontag) = "ssh_dial_timeout,omitempty",
(gogoproto.casttype) = "Duration"
];
}

// TunnelStrategyV1 defines possible tunnel strategy types.
Expand Down
26 changes: 26 additions & 0 deletions api/types/networking.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,12 @@ type ClusterNetworkingConfig interface {

// SetCaseInsensitiveRouting sets the case-insenstivie routing option.
SetCaseInsensitiveRouting(cir bool)

// GetSSHDialTimeout gets timeout value that should be used for SSH connections.
GetSSHDialTimeout() time.Duration

// SetSSHDialTimeout sets the timeout value that should be used for SSH connections.
SetSSHDialTimeout(t time.Duration)
}

// NewClusterNetworkingConfigFromConfigFile is a convenience method to create
Expand Down Expand Up @@ -417,6 +423,26 @@ func (c *ClusterNetworkingConfigV2) SetCaseInsensitiveRouting(cir bool) {
c.Spec.CaseInsensitiveRouting = cir
}

// GetSSHDialTimeout returns the timeout to be used for SSH connections.
// If the value is not set, or was intentionally set to zero or a negative value,
// [defaults.DefaultIOTimeout] is returned instead. This is because
// a zero value cannot be distinguished to mean no timeout, or
// that a value had never been set.
func (c *ClusterNetworkingConfigV2) GetSSHDialTimeout() time.Duration {
if c.Spec.SSHDialTimeout <= 0 {
return defaults.DefaultIOTimeout
}

return c.Spec.SSHDialTimeout.Duration()
}

// SetSSHDialTimeout updates the SSH connection timeout. The value is
// not validated, but will not be respected if zero or negative. See
// the docs on [ClusterNetworkingConfigV2.GetSSHDialTimeout] for more details.
func (c *ClusterNetworkingConfigV2) SetSSHDialTimeout(t time.Duration) {
c.Spec.SSHDialTimeout = Duration(t)
}

// MarshalYAML defines how a proxy listener mode should be marshaled to a string
func (p ProxyListenerMode) MarshalYAML() (interface{}, error) {
return strings.ToLower(p.String()), nil
Expand Down
39 changes: 39 additions & 0 deletions api/types/networking_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,13 @@ package types
import (
"strings"
"testing"
"time"

"github.com/gravitational/trace"
"github.com/stretchr/testify/require"
"gopkg.in/yaml.v2"

"github.com/gravitational/teleport/api/defaults"
)

func TestProxyListenerModeMarshalYAML(t *testing.T) {
Expand Down Expand Up @@ -89,3 +92,39 @@ func TestProxyListenerModeUnmarshalYAML(t *testing.T) {
})
}
}

func TestSSHDialTimeout(t *testing.T) {
cfg := DefaultClusterNetworkingConfig()

// Validate that the default config, which has no dial timeout set
// returns the default value.
require.Equal(t, defaults.DefaultIOTimeout, cfg.GetSSHDialTimeout())

// A zero value can be set, but retrieving a zero value will result
// in the default value since we cannot distinguish between an
// old config prior to the addition of the ssh dial timeout being added
// and an explicitly set value of zero.
cfg.SetSSHDialTimeout(0)
require.Equal(t, defaults.DefaultIOTimeout, cfg.GetSSHDialTimeout())

// Validate that a non-zero value is honored.
cfg.SetSSHDialTimeout(time.Minute)
require.Equal(t, time.Minute, cfg.GetSSHDialTimeout())

// Validate that unmarshaling a config without a timeout set
// returns the default value.
raw, err := yaml.Marshal(DefaultClusterNetworkingConfig())
require.NoError(t, err)

var cnc ClusterNetworkingConfigV2
require.NoError(t, yaml.Unmarshal(raw, &cnc))
require.Equal(t, defaults.DefaultIOTimeout, cnc.GetSSHDialTimeout())

// Validate that unmarshaling a config with a valid timeout
// set returns the correct duration.
raw, err = yaml.Marshal(cfg)
require.NoError(t, err)

require.NoError(t, yaml.Unmarshal(raw, &cnc))
require.Equal(t, time.Minute, cnc.GetSSHDialTimeout())
}
Loading

0 comments on commit 2ddc37a

Please sign in to comment.