From 7269e33759a8899686e4d486742a6a6919cad17b Mon Sep 17 00:00:00 2001 From: rosstimothy <39066650+rosstimothy@users.noreply.github.com> Date: Fri, 2 Feb 2024 12:08:48 -0500 Subject: [PATCH] Reuse tunnel resolvers instead of creating one per connection attempt (#37566) The `CachingResolver` is backed by a `FnCache` but does not expose a way to close the underlying cache. This leads to memory leaks as captured in #37025. Instead of modifying the resolver to allow explicit cleanup to occur, the resolvers were refactored to be created once per process instead of per connenction attempt to the cluster. Since the cluster address is read from the config file, it won't be changed for the duration of the process which allows us to safely use a single resolver. The one potential downside to this approach is the cache may return possibly stale errors during an outage until the entry is TTLed. Fixes #37025 --- lib/auth/authclient/authclient.go | 22 ++++++---------------- lib/service/connect.go | 21 ++++----------------- lib/service/service.go | 26 ++++++++++++++++++++++++++ lib/tbot/renew.go | 1 + lib/tbot/tbot.go | 13 +++++++++++++ tool/tctl/common/tctl.go | 19 ++++++++++++++++++- 6 files changed, 68 insertions(+), 34 deletions(-) diff --git a/lib/auth/authclient/authclient.go b/lib/auth/authclient/authclient.go index 4086da3ef1d8d..6c9b3c65ec442 100644 --- a/lib/auth/authclient/authclient.go +++ b/lib/auth/authclient/authclient.go @@ -29,7 +29,6 @@ import ( "github.com/gravitational/teleport/api/breaker" apiclient "github.com/gravitational/teleport/api/client" - "github.com/gravitational/teleport/api/client/webclient" "github.com/gravitational/teleport/lib/auth" "github.com/gravitational/teleport/lib/reversetunnelclient" "github.com/gravitational/teleport/lib/utils" @@ -49,6 +48,11 @@ type Config struct { CircuitBreakerConfig breaker.Config // DialTimeout determines how long to wait for dialing to succeed before aborting. DialTimeout time.Duration + // Insecure turns off TLS certificate verification when enabled. + Insecure bool + // Resolver is used to identify the reverse tunnel address when connecting via + // the proxy. + Resolver reversetunnelclient.Resolver } // Connect creates a valid client connection to the auth service. It may @@ -108,25 +112,11 @@ func connectViaProxyTunnel(ctx context.Context, cfg *Config) (auth.ClientI, erro // If direct dial failed, we may have a proxy address in // cfg.AuthServers. Try connecting to the reverse tunnel // endpoint and make a client over that. - // - // TODO(nic): this logic should be implemented once and reused in IoT - // nodes. - resolver := reversetunnelclient.WebClientResolver(&webclient.Config{ - Context: ctx, - ProxyAddr: cfg.AuthServers[0].String(), - Insecure: cfg.TLS.InsecureSkipVerify, - Timeout: cfg.DialTimeout, - }) - - resolver, err := reversetunnelclient.CachingResolver(ctx, resolver, nil /* clock */) - if err != nil { - return nil, trace.Wrap(err) - } // reversetunnelclient.TunnelAuthDialer will take care of creating a net.Conn // within an SSH tunnel. dialer, err := reversetunnelclient.NewTunnelAuthDialer(reversetunnelclient.TunnelAuthDialerConfig{ - Resolver: resolver, + Resolver: cfg.Resolver, ClientConfig: cfg.SSH, Log: cfg.Log, InsecureSkipTLSVerify: cfg.TLS.InsecureSkipVerify, diff --git a/lib/service/connect.go b/lib/service/connect.go index 33ce01b2ac7ca..ef32ede08f16b 100644 --- a/lib/service/connect.go +++ b/lib/service/connect.go @@ -36,7 +36,6 @@ import ( "github.com/gravitational/teleport" apiclient "github.com/gravitational/teleport/api/client" "github.com/gravitational/teleport/api/client/proto" - "github.com/gravitational/teleport/api/client/webclient" "github.com/gravitational/teleport/api/types" "github.com/gravitational/teleport/api/utils/retryutils" "github.com/gravitational/teleport/lib" @@ -1222,7 +1221,7 @@ func (process *TeleportProcess) newClient(identity *auth.Identity) (*auth.Client logger.Debug("Attempting to discover reverse tunnel address.") logger.Debug("Attempting to connect to Auth Server through tunnel.") - tunnelClient, err := process.newClientThroughTunnel(authServers[0].String(), tlsConfig, sshClientConfig) + tunnelClient, err := process.newClientThroughTunnel(tlsConfig, sshClientConfig) if err != nil { process.log.Errorf("Node failed to establish connection to Teleport Proxy. We have tried the following endpoints:") process.log.Errorf("- connecting to auth server directly: %v", directErr) @@ -1248,7 +1247,7 @@ func (process *TeleportProcess) newClient(identity *auth.Identity) (*auth.Client logger := process.log.WithField("proxy-server", proxyServer.String()) logger.Debug("Attempting to connect to Auth Server through tunnel.") - tunnelClient, err := process.newClientThroughTunnel(proxyServer.String(), tlsConfig, sshClientConfig) + tunnelClient, err := process.newClientThroughTunnel(tlsConfig, sshClientConfig) if err != nil { return nil, trace.Errorf("Failed to connect to Proxy Server through tunnel: %v", err) } @@ -1267,21 +1266,9 @@ func (process *TeleportProcess) newClient(identity *auth.Identity) (*auth.Client return nil, trace.NotImplemented("could not find connection strategy for config version %s", process.Config.Version) } -func (process *TeleportProcess) newClientThroughTunnel(addr string, tlsConfig *tls.Config, sshConfig *ssh.ClientConfig) (*auth.Client, error) { - resolver := reversetunnelclient.WebClientResolver(&webclient.Config{ - Context: process.ExitContext(), - ProxyAddr: addr, - Insecure: lib.IsInsecureDevMode(), - Timeout: process.Config.Testing.ClientTimeout, - }) - - resolver, err := reversetunnelclient.CachingResolver(process.ExitContext(), resolver, process.Clock) - if err != nil { - return nil, trace.Wrap(err) - } - +func (process *TeleportProcess) newClientThroughTunnel(tlsConfig *tls.Config, sshConfig *ssh.ClientConfig) (*auth.Client, error) { dialer, err := reversetunnelclient.NewTunnelAuthDialer(reversetunnelclient.TunnelAuthDialerConfig{ - Resolver: resolver, + Resolver: process.resolver, ClientConfig: sshConfig, Log: process.log, InsecureSkipTLSVerify: lib.IsInsecureDevMode(), diff --git a/lib/service/service.go b/lib/service/service.go index 0f39066f9aab2..1686247984f17 100644 --- a/lib/service/service.go +++ b/lib/service/service.go @@ -60,6 +60,7 @@ import ( "github.com/gravitational/teleport" "github.com/gravitational/teleport/api/client" "github.com/gravitational/teleport/api/client/proto" + "github.com/gravitational/teleport/api/client/webclient" "github.com/gravitational/teleport/api/constants" apidefaults "github.com/gravitational/teleport/api/defaults" kubeproto "github.com/gravitational/teleport/api/gen/proto/go/teleport/kube/v1" @@ -411,6 +412,10 @@ type TeleportProcess struct { // SSHD is used to execute commands to update or validate OpenSSH config. SSHD openssh.SSHD + + // resolver is used to identify the reverse tunnel address when connecting via + // the proxy. + resolver reversetunnelclient.Resolver } type keyPairKey struct { @@ -968,6 +973,27 @@ func NewTeleport(cfg *servicecfg.Config) (*TeleportProcess, error) { } } + var resolverAddr utils.NetAddr + if cfg.Version == defaults.TeleportConfigVersionV3 && !cfg.ProxyServer.IsEmpty() { + resolverAddr = cfg.ProxyServer + } else { + resolverAddr = cfg.AuthServerAddresses()[0] + } + + process.resolver, err = reversetunnelclient.CachingResolver( + process.ExitContext(), + reversetunnelclient.WebClientResolver(&webclient.Config{ + Context: process.ExitContext(), + ProxyAddr: resolverAddr.String(), + Insecure: lib.IsInsecureDevMode(), + Timeout: process.Config.Testing.ClientTimeout, + }), + process.Clock, + ) + if err != nil { + return nil, trace.Wrap(err) + } + upgraderKind := os.Getenv("TELEPORT_EXT_UPGRADER") upgraderVersion := automaticupgrades.GetUpgraderVersion(process.GracefulExitContext()) diff --git a/lib/tbot/renew.go b/lib/tbot/renew.go index 28c7c5ddb1311..6496ad2ceb6be 100644 --- a/lib/tbot/renew.go +++ b/lib/tbot/renew.go @@ -97,6 +97,7 @@ func (b *Bot) AuthenticatedUserClientFromIdentity(ctx context.Context, id *ident SSH: sshConfig, AuthServers: []utils.NetAddr{*authAddr}, Log: b.log, + Resolver: b.resolver, } c, err := authclient.Connect(ctx, authClientConfig) diff --git a/lib/tbot/tbot.go b/lib/tbot/tbot.go index 3a147650b3515..47671d95312a4 100644 --- a/lib/tbot/tbot.go +++ b/lib/tbot/tbot.go @@ -37,6 +37,7 @@ import ( "github.com/gravitational/teleport/api/types" "github.com/gravitational/teleport/lib/auth" "github.com/gravitational/teleport/lib/modules" + "github.com/gravitational/teleport/lib/reversetunnelclient" "github.com/gravitational/teleport/lib/tbot/config" "github.com/gravitational/teleport/lib/tbot/identity" "github.com/gravitational/teleport/lib/utils" @@ -47,6 +48,7 @@ type Bot struct { log logrus.FieldLogger reloadChan chan struct{} modules modules.Modules + resolver reversetunnelclient.Resolver // These are protected by getter/setters with mutex locks mu sync.Mutex @@ -342,6 +344,17 @@ func (b *Bot) initialize(ctx context.Context) (func() error, error) { var authClient auth.ClientI + b.resolver, err = reversetunnelclient.CachingResolver( + ctx, + reversetunnelclient.WebClientResolver(&webclient.Config{ + Context: ctx, + ProxyAddr: b.cfg.AuthServer, + }), + nil /* clock */) + if err != nil { + return unlock, trace.Wrap(err) + } + fetchNewIdentity := true // First, attempt to load an identity from storage. ident, err := identity.LoadIdentity(dest, identity.BotKinds()...) diff --git a/tool/tctl/common/tctl.go b/tool/tctl/common/tctl.go index e97d7f518d95d..0fc09d5f0a64d 100644 --- a/tool/tctl/common/tctl.go +++ b/tool/tctl/common/tctl.go @@ -31,6 +31,7 @@ import ( "github.com/gravitational/teleport" "github.com/gravitational/teleport/api/breaker" + "github.com/gravitational/teleport/api/client/webclient" "github.com/gravitational/teleport/api/constants" "github.com/gravitational/teleport/api/types" "github.com/gravitational/teleport/lib/auth" @@ -40,6 +41,7 @@ import ( "github.com/gravitational/teleport/lib/config" "github.com/gravitational/teleport/lib/defaults" "github.com/gravitational/teleport/lib/modules" + "github.com/gravitational/teleport/lib/reversetunnelclient" "github.com/gravitational/teleport/lib/service/servicecfg" "github.com/gravitational/teleport/lib/utils" "github.com/gravitational/teleport/tool/common" @@ -194,6 +196,19 @@ func TryRun(commands []CLICommand, args []string) error { ctx := context.Background() + clientConfig.Resolver, err = reversetunnelclient.CachingResolver( + ctx, + reversetunnelclient.WebClientResolver(&webclient.Config{ + Context: ctx, + ProxyAddr: clientConfig.AuthServers[0].String(), + Insecure: clientConfig.Insecure, + Timeout: clientConfig.DialTimeout, + }), + nil /* clock */) + if err != nil { + return trace.Wrap(err) + } + client, err := authclient.Connect(ctx, clientConfig) if err != nil { if utils.IsUntrustedCertErr(err) { @@ -388,7 +403,9 @@ func LoadConfigFromProfile(ccf *GlobalCLIFlags, cfg *servicecfg.Config) (*authcl return nil, trace.BadParameter("your credentials are for cluster %q, please run `tsh login %q` to log in to the root cluster", profile.Cluster, rootCluster) } - authConfig := &authclient.Config{} + authConfig := &authclient.Config{ + Insecure: ccf.Insecure, + } authConfig.TLS, err = key.TeleportClientTLSConfig(cfg.CipherSuites, []string{rootCluster}) if err != nil { return nil, trace.Wrap(err)