From dab886c1aaa3780a9b29e69cecbf88dbcb3e1621 Mon Sep 17 00:00:00 2001
From: Tim Buckley <tim@goteleport.com>
Date: Fri, 1 Nov 2024 17:40:28 -0600
Subject: [PATCH] Machine ID: Fix `tshwrap` destination dir handling (#48250)

Recent CLI handling changes broke destination dir resolution for
commands that use `tshwrap.GetDestinationDir()` to resolve
destinations from either the CLI or config file.

The old behavior depends on implicit creation of an identity service
if `--destination-dir` was provided on the CLI and no config file is
loaded. We now no longer make this assumption and prefer to only
resolve explicit configurations, so this tweaks
`tshwrap.GetDestinationDir()` to explicitly handle the CLI parameter
rather than hoping it appears in the generated config.

While this is legacy functionality, it's not slated for removal in
v17. Most users should still prefer to use the new app and database
tunnel services wherever possible.

Fixes #48107
---
 lib/tbot/tshwrap/wrap.go      | 26 ++++++++++++++++----------
 lib/tbot/tshwrap/wrap_test.go |  6 +++---
 tool/tbot/db.go               |  2 +-
 tool/tbot/proxy.go            |  2 +-
 4 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/lib/tbot/tshwrap/wrap.go b/lib/tbot/tshwrap/wrap.go
index f09df1039159f..38447ce57b19d 100644
--- a/lib/tbot/tshwrap/wrap.go
+++ b/lib/tbot/tshwrap/wrap.go
@@ -130,22 +130,28 @@ type destinationHolder interface {
 	GetDestination() bot.Destination
 }
 
-// GetDestinationDirectory attempts to select an unambiguous destination, either from
-// CLI or YAML config. It returns an error if the selected destination is
-// invalid.
-func GetDestinationDirectory(botConfig *config.BotConfig) (*config.DestinationDirectory, error) {
+// GetDestinationDirectory attempts to select an unambiguous destination, either
+// from CLI or YAML config. It returns an error if the selected destination is
+// invalid. Note that CLI destinations will not be validated.
+func GetDestinationDirectory(cliDestinationPath string, botConfig *config.BotConfig) (*config.DestinationDirectory, error) {
+	if cliDestinationPath != "" {
+		d := &config.DestinationDirectory{
+			Path: cliDestinationPath,
+		}
+		if err := d.CheckAndSetDefaults(); err != nil {
+			return nil, trace.Wrap(err)
+		}
+
+		return d, nil
+	}
+
 	var destinationHolders []destinationHolder
 	for _, svc := range botConfig.Services {
 		if v, ok := svc.(destinationHolder); ok {
 			destinationHolders = append(destinationHolders, v)
 		}
 	}
-	// WARNING:
-	// This code is dependent on some unexpected "behavior" in
-	// config.FromCLIConf() - when users provide --destination-dir then all
-	// outputs configured in the YAML file are overwritten by an identity
-	// output with a directory destination with a path of --destination-dir.
-	// See: https://github.com/gravitational/teleport/issues/27206
+
 	if len(destinationHolders) == 0 {
 		return nil, trace.BadParameter("either --destination-dir or a config file containing an output or service must be specified")
 	} else if len(destinationHolders) > 1 {
diff --git a/lib/tbot/tshwrap/wrap_test.go b/lib/tbot/tshwrap/wrap_test.go
index df6ca637800ee..2e4b81d7d0648 100644
--- a/lib/tbot/tshwrap/wrap_test.go
+++ b/lib/tbot/tshwrap/wrap_test.go
@@ -68,16 +68,16 @@ func TestGetDestinationDirectory(t *testing.T) {
 		return cfg
 	}
 	t.Run("one output configured", func(t *testing.T) {
-		dest, err := GetDestinationDirectory(config(1))
+		dest, err := GetDestinationDirectory("", config(1))
 		require.NoError(t, err)
 		require.Equal(t, "/from-bot-config0", dest.Path)
 	})
 	t.Run("no outputs specified", func(t *testing.T) {
-		_, err := GetDestinationDirectory(config(0))
+		_, err := GetDestinationDirectory("", config(0))
 		require.ErrorContains(t, err, "either --destination-dir or a config file containing an output or service must be specified")
 	})
 	t.Run("multiple outputs specified", func(t *testing.T) {
-		_, err := GetDestinationDirectory(config(2))
+		_, err := GetDestinationDirectory("", config(2))
 		require.ErrorContains(t, err, "the config file contains multiple outputs and services; a --destination-dir must be specified")
 	})
 }
diff --git a/tool/tbot/db.go b/tool/tbot/db.go
index e2fe32375a31a..59d67c148563d 100644
--- a/tool/tbot/db.go
+++ b/tool/tbot/db.go
@@ -40,7 +40,7 @@ func onDBCommand(globalCfg *cli.GlobalArgs, dbCmd *cli.DBCommand) error {
 		return trace.Wrap(err)
 	}
 
-	destination, err := tshwrap.GetDestinationDirectory(botConfig)
+	destination, err := tshwrap.GetDestinationDirectory(dbCmd.DestinationDir, botConfig)
 	if err != nil {
 		return trace.Wrap(err)
 	}
diff --git a/tool/tbot/proxy.go b/tool/tbot/proxy.go
index 677a0caeb93a0..6dc3804092249 100644
--- a/tool/tbot/proxy.go
+++ b/tool/tbot/proxy.go
@@ -44,7 +44,7 @@ func onProxyCommand(
 		return trace.Wrap(err)
 	}
 
-	destination, err := tshwrap.GetDestinationDirectory(botConfig)
+	destination, err := tshwrap.GetDestinationDirectory(proxyCmd.DestinationDir, botConfig)
 	if err != nil {
 		return trace.Wrap(err)
 	}