From c2207fa9cf3a15b9660ab9f5db50a9cc6d0d63e2 Mon Sep 17 00:00:00 2001 From: Andrew LeFevre Date: Mon, 18 Sep 2023 14:59:01 -0400 Subject: [PATCH] fix direct dialing to deprecated unregistered SSH nodes (#31903) * fix direct dialing to deprecated unregistered SSH nodes * revert changing reexec error message * change the dummy server's hostname so the addr shows up in session recordings * Add test * make test pass * address feedback * return an error if direct dialing with OpenSSH 'ssh' is attempted without proxy rec mode * fix import ordering * fix integration test after adding error message * make error msg when env isn't passed link to agentless docs * fix unit test --------- Co-authored-by: Anton Miniailo --- integration/integration_test.go | 93 +++++++++++++++++++++++++++++++- lib/proxy/router.go | 37 ++++++++++--- lib/reversetunnel/localsite.go | 33 ++---------- lib/reversetunnel/remotesite.go | 5 +- lib/reversetunnel/site.go | 96 +++++++++++++++++++++++++++++++++ lib/reversetunnelclient/api.go | 8 ++- lib/srv/forward/sshserver.go | 4 +- lib/web/apiserver_test.go | 6 ++- 8 files changed, 237 insertions(+), 45 deletions(-) create mode 100644 lib/reversetunnel/site.go diff --git a/integration/integration_test.go b/integration/integration_test.go index 6f8d00ba402ef..b446af9a46a01 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -1334,6 +1334,8 @@ func testIPPropagation(t *testing.T, suite *integrationTestSuite) { tr := utils.NewTracer(utils.ThisFunction()).Start() defer tr.Stop() + t.Setenv("TELEPORT_UNSTABLE_UNLISTED_AGENT_DIALING", "yes") + startNodes := func(t *testing.T, root, leaf *helpers.TeleInstance) { rootNodes := []string{"root-one", "root-two"} leafNodes := []string{"leaf-one", "leaf-two"} @@ -1510,6 +1512,69 @@ func testIPPropagation(t *testing.T, suite *integrationTestSuite) { require.Equal(t, local.get().String(), pingResp.RemoteAddr, "client IP:port that auth server sees doesn't match the real one") } + testSSHUnregisteredNodeConnection := func(t *testing.T, instance *helpers.TeleInstance, clusterName string) { + sshListener, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + t.Cleanup(func() { + require.NoError(t, sshListener.Close()) + }) + + resultChan := make(chan bool) + // Start listening, emulating unregistered node. + go func() { + conn, err := sshListener.Accept() + if err != nil { + assert.Fail(t, err.Error()) + return + } + + buf := make([]byte, 3) + _, err = conn.Read(buf) + assert.NoError(t, err) + + // On the received connection first bytes should be SSH prefix, not PROXY protocol + resultChan <- slices.Equal(buf, []byte("SSH")) + }() + + nodeAddr := sshListener.Addr().String() + ctx, cancelFunc := context.WithTimeout(context.Background(), 5*time.Second) + t.Cleanup(cancelFunc) + + nodeHost, nodePortStr, err := net.SplitHostPort(nodeAddr) + require.NoError(t, err) + + nodePort, err := strconv.Atoi(nodePortStr) + require.NoError(t, err) + + tc, err := instance.NewClient(helpers.ClientConfig{ + Login: suite.Me.Username, + Cluster: clusterName, + Host: nodeHost, + Port: nodePort, + }) + require.NoError(t, err) + + clt, err := tc.ConnectToCluster(ctx) + require.NoError(t, err) + t.Cleanup(func() { + require.NoError(t, clt.Close()) + }) + + nodeDetails := client.NodeDetails{Addr: nodeAddr, Namespace: tc.Namespace, Cluster: tc.SiteName} + nodeClient, err := tc.ConnectToNode(ctx, clt, nodeDetails, tc.Config.HostLogin) + require.NoError(t, err) + t.Cleanup(func() { + require.NoError(t, nodeClient.Close()) + }) + + select { + case res := <-resultChan: + require.True(t, res, "Didn't receive SSH prefix as first bytes on the connection") + case <-time.After(time.Second): + require.Fail(t, "Timed out waiting for connection to the node") + } + } + testSSHAuthConnection := func(t *testing.T, instance *helpers.TeleInstance, clusterName string) { ctx := context.Background() @@ -1589,6 +1654,16 @@ func testIPPropagation(t *testing.T, suite *integrationTestSuite) { }) } }) + + _, root2, _ := createTrustedClusterPair(t, suite, startNodes, withProxyRecordingMode) + t.Run("We don't propagate IP to non Teleport nodes", func(t *testing.T) { + t.Run("connecting through root cluster", func(t *testing.T) { + testSSHUnregisteredNodeConnection(t, root2, "root-test") + }) + t.Run("connecting through leaf cluster", func(t *testing.T) { + testSSHUnregisteredNodeConnection(t, root2, "leaf-test") + }) + }) } // verifySessionJoin covers SSH into shell and joining the same session from another client @@ -7304,7 +7379,15 @@ outer: t.FailNow() } -func createTrustedClusterPair(t *testing.T, suite *integrationTestSuite, extraServices func(*testing.T, *helpers.TeleInstance, *helpers.TeleInstance)) (*client.TeleportClient, *helpers.TeleInstance, *helpers.TeleInstance) { +type serviceCfgOpt func(*servicecfg.Config) + +func withProxyRecordingMode(cfg *servicecfg.Config) { + recCfg := types.DefaultSessionRecordingConfig() + recCfg.SetMode(types.RecordAtProxy) + cfg.Auth.SessionRecordingConfig = recCfg +} + +func createTrustedClusterPair(t *testing.T, suite *integrationTestSuite, extraServices func(*testing.T, *helpers.TeleInstance, *helpers.TeleInstance), cfgOpts ...serviceCfgOpt) (*client.TeleportClient, *helpers.TeleInstance, *helpers.TeleInstance) { ctx := context.Background() username := suite.Me.Username name := "test" @@ -7355,6 +7438,11 @@ func createTrustedClusterPair(t *testing.T, suite *integrationTestSuite, extraSe tconf.Proxy.DisableWebInterface = true tconf.SSH.Enabled = false tconf.CachePolicy.MaxRetryPeriod = time.Millisecond * 500 + + for _, opt := range cfgOpts { + opt(tconf) + } + return t, nil, tconf } @@ -7618,7 +7706,8 @@ func testListResourcesAcrossClusters(t *testing.T, suite *integrationTestSuite) func testJoinOverReverseTunnelOnly(t *testing.T, suite *integrationTestSuite) { for _, proxyProtocolMode := range []multiplexer.PROXYProtocolMode{ - multiplexer.PROXYProtocolOn, multiplexer.PROXYProtocolOff, multiplexer.PROXYProtocolUnspecified} { + multiplexer.PROXYProtocolOn, multiplexer.PROXYProtocolOff, multiplexer.PROXYProtocolUnspecified, + } { t.Run(fmt.Sprintf("proxy protocol mode: %v", proxyProtocolMode), func(t *testing.T) { lib.SetInsecureDevMode(true) t.Cleanup(func() { lib.SetInsecureDevMode(false) }) diff --git a/lib/proxy/router.go b/lib/proxy/router.go index 07c2ec919304e..c2243eaae03f2 100644 --- a/lib/proxy/router.go +++ b/lib/proxy/router.go @@ -45,6 +45,12 @@ import ( "github.com/gravitational/teleport/lib/utils" ) +const errDirectDialing = `Direct dialing to nodes not found in the inventory is not supported. +If you want to connect to a node without installing Teleport on it, consider registering it with +your cluster with 'teleport join openssh'. + +See https://goteleport.com/docs/ver/14.x/server-access/guides/openssh/ for more details.` + var ( // proxiedSessions counts successful connections to nodes proxiedSessions = prometheus.NewGauge( @@ -236,11 +242,12 @@ func (r *Router) DialHost(ctx context.Context, clientSrcAddr, clientDstAddr net. principals := []string{host} var ( - isAgentlessNode bool - serverID string - serverAddr string - proxyIDs []string - sshSigner ssh.Signer + isAgentlessNode bool + isNotInventoryNode bool + serverID string + serverAddr string + proxyIDs []string + sshSigner ssh.Signer ) if target != nil { @@ -282,16 +289,29 @@ func (r *Router) DialHost(ctx context.Context, clientSrcAddr, clientDstAddr net. } } } - } else { if !r.permitUnlistedDialing { - return nil, trace.ConnectionProblem(errors.New("connection problem"), "direct dialing to nodes not found in inventory is not supported") + return nil, trace.ConnectionProblem(errors.New("connection problem"), errDirectDialing) } + + // Prepare a dummy server resource so this connection will not be + // treated like a connection to a Teleport node + isNotInventoryNode = true + isAgentlessNode = true if port == "" || port == "0" { port = strconv.Itoa(defaults.SSHServerListenPort) } - serverAddr = net.JoinHostPort(host, port) + name := "unknown server " + serverAddr + target, err = types.NewServer(name, types.KindNode, types.ServerSpecV2{ + Addr: serverAddr, + Hostname: host, + }) + if err != nil { + return nil, trace.Wrap(err) + } + target.SetSubKind(types.SubKindOpenSSHNode) + r.log.Warnf("server lookup failed: using default=%v", serverAddr) } @@ -300,6 +320,7 @@ func (r *Router) DialHost(ctx context.Context, clientSrcAddr, clientDstAddr net. To: &utils.NetAddr{AddrNetwork: "tcp", Addr: serverAddr}, OriginalClientDstAddr: clientDstAddr, GetUserAgent: agentGetter, + IsNotInventoryNode: isNotInventoryNode, IsAgentlessNode: isAgentlessNode, AgentlessSigner: sshSigner, Address: host, diff --git a/lib/reversetunnel/localsite.go b/lib/reversetunnel/localsite.go index cb8c50c7c3e93..e1c5782b4a793 100644 --- a/lib/reversetunnel/localsite.go +++ b/lib/reversetunnel/localsite.go @@ -36,7 +36,6 @@ import ( "github.com/gravitational/teleport/api/utils/sshutils" "github.com/gravitational/teleport/lib/auth" "github.com/gravitational/teleport/lib/integrations/awsoidc" - "github.com/gravitational/teleport/lib/multiplexer" "github.com/gravitational/teleport/lib/observability/metrics" "github.com/gravitational/teleport/lib/proxy/peer" "github.com/gravitational/teleport/lib/reversetunnel/track" @@ -239,30 +238,14 @@ func (s *localSite) DialAuthServer(params reversetunnelclient.DialParams) (net.C return conn, nil } -// shouldDialAndForward returns whether a connection should be proxied -// and forwarded or not. -func shouldDialAndForward(params reversetunnelclient.DialParams, recConfig types.SessionRecordingConfig) bool { - // connection is already being tunneled, do not forward - if params.FromPeerProxy { - return false - } - // the node is an agentless node, the connection must be forwarded - if params.TargetServer != nil && params.TargetServer.IsOpenSSHNode() { - return true - } - // proxy session recording mode is being used and an SSH session - // is being requested, the connection must be forwarded - if params.ConnType == types.NodeTunnel && services.IsRecordAtProxy(recConfig.GetMode()) { - return true - } - return false -} - func (s *localSite) Dial(params reversetunnelclient.DialParams) (net.Conn, error) { recConfig, err := s.accessPoint.GetSessionRecordingConfig(s.srv.Context) if err != nil { return nil, trace.Wrap(err) } + if err := checkNodeAndRecConfig(params, recConfig); err != nil { + return nil, trace.Wrap(err) + } // If the proxy is in recording mode and a SSH connection is being // requested or the target server is a registered OpenSSH node, build @@ -275,14 +258,6 @@ func (s *localSite) Dial(params reversetunnelclient.DialParams) (net.Conn, error return s.DialTCP(params) } -func shouldSendSignedPROXYHeader(signer multiplexer.PROXYHeaderSigner, useTunnel, isAgentlessNode bool, srcAddr, dstAddr net.Addr) bool { - return !(signer == nil || - useTunnel || - isAgentlessNode || - srcAddr == nil || - dstAddr == nil) -} - func (s *localSite) maybeSendSignedPROXYHeader(params reversetunnelclient.DialParams, conn net.Conn, useTunnel bool) error { if !shouldSendSignedPROXYHeader(s.srv.proxySigner, useTunnel, params.IsAgentlessNode, params.From, params.OriginalClientDstAddr) { return nil @@ -399,7 +374,7 @@ func (s *localSite) dialAndForward(params reversetunnelclient.DialParams) (_ net serverConfig := forward.ServerConfig{ AuthClient: s.client, UserAgent: userAgent, - IsAgentlessNode: params.IsAgentlessNode, + IsAgentlessNode: isAgentlessNode(params), AgentlessSigner: params.AgentlessSigner, TargetConn: targetConn, SrcAddr: params.From, diff --git a/lib/reversetunnel/remotesite.go b/lib/reversetunnel/remotesite.go index b8cac93e1250f..f2b2a7123b7c2 100644 --- a/lib/reversetunnel/remotesite.go +++ b/lib/reversetunnel/remotesite.go @@ -759,6 +759,9 @@ func (s *remoteSite) Dial(params reversetunnelclient.DialParams) (net.Conn, erro if err != nil { return nil, trace.Wrap(err) } + if err := checkNodeAndRecConfig(params, recConfig); err != nil { + return nil, trace.Wrap(err) + } // If the proxy is in recording mode and a SSH connection is being // requested or the target server is a registered OpenSSH node, build @@ -837,7 +840,7 @@ func (s *remoteSite) dialAndForward(params reversetunnelclient.DialParams) (_ ne serverConfig := forward.ServerConfig{ AuthClient: s.localClient, UserAgent: userAgent, - IsAgentlessNode: params.IsAgentlessNode, + IsAgentlessNode: isAgentlessNode(params), AgentlessSigner: params.AgentlessSigner, TargetConn: targetConn, SrcAddr: params.From, diff --git a/lib/reversetunnel/site.go b/lib/reversetunnel/site.go new file mode 100644 index 0000000000000..5df8aa7c25d36 --- /dev/null +++ b/lib/reversetunnel/site.go @@ -0,0 +1,96 @@ +/* +Copyright 2023 Gravitational, Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package reversetunnel + +import ( + "errors" + "net" + + "github.com/gravitational/trace" + + "github.com/gravitational/teleport/api/types" + "github.com/gravitational/teleport/lib/multiplexer" + "github.com/gravitational/teleport/lib/reversetunnelclient" + "github.com/gravitational/teleport/lib/services" +) + +var errDirectDialNoProxyRec = errors.New("direct dialing to nodes not found in inventory requires that the session recording mode is set to record at the proxy") + +func checkNodeAndRecConfig(params reversetunnelclient.DialParams, recConfig types.SessionRecordingConfig) error { + if params.IsNotInventoryNode && !services.IsRecordAtProxy(recConfig.GetMode()) { + return trace.Wrap(errDirectDialNoProxyRec) + } + return nil +} + +// shouldDialAndForward returns whether a connection should be proxied +// and forwarded or not. +func shouldDialAndForward(params reversetunnelclient.DialParams, recConfig types.SessionRecordingConfig) bool { + // connection is already being tunneled, do not forward + if params.FromPeerProxy { + return false + } + // the node is an agentless node, the connection must be forwarded + if params.TargetServer != nil && params.TargetServer.IsOpenSSHNode() { + return true + } + // proxy session recording mode is being used and an SSH session + // is being requested, the connection must be forwarded + if params.ConnType == types.NodeTunnel && services.IsRecordAtProxy(recConfig.GetMode()) { + return true + } + // if the node was directly dialed and not in the inventory, the + // connection must be forwarded + if params.IsNotInventoryNode { + return true + } + + return false +} + +// shouldSendSignedPROXYHeader returns whether a connection should send +// a signed PROXY header at the start of the connection or not. +func shouldSendSignedPROXYHeader(signer multiplexer.PROXYHeaderSigner, useTunnel, isAgentlessNode bool, srcAddr, dstAddr net.Addr) bool { + // nothing to sign with, can't send a signed header + if signer == nil { + return false + } + // signed PROXY headers aren't sent over a tunnel + if useTunnel { + return false + } + // we are connecting to an agentless node which won't understand the + // PROXY protocol + if isAgentlessNode { + return false + } + // we have to have both the source and destination to populate the + // signed PROXY header with if we want to send it + if srcAddr == nil || dstAddr == nil { + return false + } + + return true +} + +func isAgentlessNode(params reversetunnelclient.DialParams) bool { + // If the node is not in the inventory (was directly dialed) tell + // the forwarding server it isn't an agentless node so config checks + // pass. params.TargetServer will ensure the node is not treated as + // a Teleport node in this case. + return params.IsAgentlessNode && !params.IsNotInventoryNode +} diff --git a/lib/reversetunnelclient/api.go b/lib/reversetunnelclient/api.go index a3be09b749509..175d06be705b3 100644 --- a/lib/reversetunnelclient/api.go +++ b/lib/reversetunnelclient/api.go @@ -44,14 +44,18 @@ type DialParams struct { // forwarding proxy. GetUserAgent teleagent.Getter - // IsAgentlessNode indicates whether the Node is an OpenSSH Node. - // This includes Nodes whose sub kind is OpenSSH and OpenSSHEICE. + // IsAgentlessNode indicates whether the node is an OpenSSH node. + // This includes nodes whose sub kind is OpenSSH and OpenSSHEICE. IsAgentlessNode bool // AgentlessSigner is used for authenticating to the remote host when it is an // agentless node. AgentlessSigner ssh.Signer + // isNotInventoryNode indicates whether the node is not a known registered + // node that was directly dialed. + IsNotInventoryNode bool + // Address is used by the forwarding proxy to generate a host certificate for // the target node. This is needed because while dialing occurs via IP // address, tsh thinks it's connecting via DNS name and that's how it diff --git a/lib/srv/forward/sshserver.go b/lib/srv/forward/sshserver.go index 243c475a87076..7491c35de64e9 100644 --- a/lib/srv/forward/sshserver.go +++ b/lib/srv/forward/sshserver.go @@ -243,8 +243,8 @@ type ServerConfig struct { // or an agentless server. TargetServer types.Server - // IsAgentlessNode indicates whether the targetServer is a Node with an OpenSSH server (no teleport agent). - // This includes Nodes whose sub kind is OpenSSH and OpenSSHEphemeralKey. + // IsAgentlessNode indicates whether the targetServer is a node with an OpenSSH server (no teleport agent). + // This includes nodes whose sub kind is OpenSSH and OpenSSHEphemeralKey. IsAgentlessNode bool } diff --git a/lib/web/apiserver_test.go b/lib/web/apiserver_test.go index 30caef6eaca68..d9544abca1298 100644 --- a/lib/web/apiserver_test.go +++ b/lib/web/apiserver_test.go @@ -6016,7 +6016,11 @@ func TestDiagnoseSSHConnection(t *testing.T) { Type: types.ConnectionDiagnosticTrace_CONNECTIVITY, Status: types.ConnectionDiagnosticTrace_FAILED, Details: `Failed to connect to the Node. Ensure teleport service is running using "systemctl status teleport".`, - Error: "direct dialing to nodes not found in inventory is not supported", + Error: `Direct dialing to nodes not found in the inventory is not supported. +If you want to connect to a node without installing Teleport on it, consider registering it with +your cluster with 'teleport join openssh'. + +See https://goteleport.com/docs/ver/14.x/server-access/guides/openssh/ for more details.`, }, }, },