Add version validation

Fix recursive version check for darwin platform
Fix cleanup for multi-package support

integration/autoupdate/tools/updater/main.go

@@ -48,7 +48,10 @@ func main() {
 		version,
 		tools.WithBaseURL(baseURL),
 	)
-	toolsVersion, reExec := updater.CheckLocal()
+	toolsVersion, reExec, err := updater.CheckLocal()
+	if err != nil {
+		log.Fatal(err)
+	}
 	if reExec {
 		// Download and update the version of client tools required by the cluster.
 		// This is required if the user passed in the TELEPORT_TOOLS_VERSION explicitly.

lib/autoupdate/tools/updater.go

@@ -36,6 +36,7 @@ import (
 	"syscall"
 	"time"
 
+	"github.com/coreos/go-semver/semver"
 	"github.com/google/uuid"
 	"github.com/gravitational/trace"
 
@@ -114,30 +115,33 @@ func NewUpdater(tools []string, toolsDir string, localVersion string, options ..
 // CheckLocal is run at client tool startup and will only perform local checks.
 // Returns the version needs to be updated and re-executed, by re-execution flag we
 // understand that update and re-execute is required.
-func (u *Updater) CheckLocal() (version string, reExec bool) {
+func (u *Updater) CheckLocal() (version string, reExec bool, err error) {
 	// Check if the user has requested a specific version of client tools.
 	requestedVersion := os.Getenv(teleportToolsVersionEnv)
 	switch requestedVersion {
 	// The user has turned off any form of automatic updates.
 	case "off":
-		return "", false
+		return "", false, nil
 	// Requested version already the same as client version.
 	case u.localVersion:
-		return u.localVersion, false
+		return u.localVersion, false, nil
+	}
+
+	if requestedVersion != "" {
+		if _, err := semver.NewVersion(requestedVersion); err != nil {
+			return "", false, trace.WrapWithMessage(err, "not valid semantic version")
+		}
+		return requestedVersion, true, nil
 	}
 
 	// If a version of client tools has already been downloaded to
 	// tools directory, return that.
-	toolsVersion, err := checkToolVersion(u.toolsDir)
+	toolsVersion, err := CheckToolVersion(u.toolsDir)
 	if err != nil {
-		return "", false
-	}
-	// The user has requested a specific version of client tools.
-	if requestedVersion != "" && requestedVersion != toolsVersion {
-		return requestedVersion, true
+		return "", false, trace.Wrap(err)
 	}
 
-	return toolsVersion, false
+	return toolsVersion, true, nil
 }
 
 // CheckRemote first checks the version set by the environment variable. If not set or disabled,
@@ -157,6 +161,13 @@ func (u *Updater) CheckRemote(ctx context.Context, proxyAddr string) (version st
 		return u.localVersion, false, nil
 	}
 
+	if requestedVersion != "" {
+		if _, err := semver.NewVersion(requestedVersion); err != nil {
+			return "", false, trace.WrapWithMessage(err, "not valid semantic version")
+		}
+		return requestedVersion, true, nil
+	}
+
 	certPool, err := x509.SystemCertPool()
 	if err != nil {
 		return "", false, trace.Wrap(err)
@@ -173,23 +184,21 @@ func (u *Updater) CheckRemote(ctx context.Context, proxyAddr string) (version st
 
 	// If a version of client tools has already been downloaded to
 	// tools directory, return that.
-	toolsVersion, err := checkToolVersion(u.toolsDir)
+	toolsVersion, err := CheckToolVersion(u.toolsDir)
 	if err != nil {
 		return "", false, trace.Wrap(err)
 	}
 
 	switch {
-	case requestedVersion != "" && requestedVersion != toolsVersion:
-		return requestedVersion, true, nil
 	case resp.AutoUpdate.ToolsMode != autoupdate.ToolsUpdateModeEnabled || resp.AutoUpdate.ToolsVersion == "":
-		return "", false, nil
+		return toolsVersion, true, nil
 	case u.localVersion == resp.AutoUpdate.ToolsVersion:
 		return resp.AutoUpdate.ToolsVersion, false, nil
 	case resp.AutoUpdate.ToolsVersion != toolsVersion:
 		return resp.AutoUpdate.ToolsVersion, true, nil
 	}
 
-	return toolsVersion, false, nil
+	return toolsVersion, true, nil
 }
 
 // UpdateWithLock acquires filesystem lock, downloads requested version package,
@@ -211,12 +220,12 @@ func (u *Updater) UpdateWithLock(ctx context.Context, toolsVersion string) (err
 	// If the version of the running binary or the version downloaded to
 	// tools directory is the same as the requested version of client tools,
 	// nothing to be done, exit early.
-	teleportVersion, err := checkToolVersion(u.toolsDir)
+	teleportVersion, err := CheckToolVersion(u.toolsDir)
 	if err != nil && !trace.IsNotFound(err) {
 		return trace.Wrap(err)
 
 	}
-	if toolsVersion == u.localVersion || toolsVersion == teleportVersion {
+	if toolsVersion == teleportVersion {
 		return nil
 	}
 
@@ -237,18 +246,26 @@ func (u *Updater) Update(ctx context.Context, toolsVersion string) error {
 		return trace.Wrap(err)
 	}
 
+	var pkgNames []string
 	for _, pkg := range packages {
-		if err := u.update(ctx, pkg); err != nil {
+		pkgName := fmt.Sprint(uuid.New().String(), updatePackageSuffix)
+		if err := u.update(ctx, pkg, pkgName); err != nil {
 			return trace.Wrap(err)
 		}
+		pkgNames = append(pkgNames, pkgName)
+	}
+
+	// Cleanup the tools directory with previously downloaded and un-archived versions.
+	if err := packaging.RemoveWithSuffix(u.toolsDir, updatePackageSuffix, pkgNames); err != nil {
+		slog.WarnContext(ctx, "failed to clean up tools directory", "error", err)
 	}
 
 	return nil
 }
 
 // update downloads the archive and validate against the hash. Download to a
 // temporary path within tools directory.
-func (u *Updater) update(ctx context.Context, pkg packageURL) error {
+func (u *Updater) update(ctx context.Context, pkg packageURL, pkgName string) error {
 	hash, err := u.downloadHash(ctx, pkg.Hash)
 	if pkg.Optional && trace.IsNotFound(err) {
 		return nil
@@ -279,7 +296,6 @@ func (u *Updater) update(ctx context.Context, pkg packageURL) error {
 		return trace.BadParameter("hash of archive does not match downloaded archive")
 	}
 
-	pkgName := fmt.Sprint(uuid.New().String(), updatePackageSuffix)
 	extractDir := filepath.Join(u.toolsDir, pkgName)
 	if runtime.GOOS != constants.DarwinOS {
 		if err := os.Mkdir(extractDir, 0o755); err != nil {
@@ -291,10 +307,6 @@ func (u *Updater) update(ctx context.Context, pkg packageURL) error {
 	if err := packaging.ReplaceToolsBinaries(u.toolsDir, f.Name(), extractDir, u.tools); err != nil {
 		return trace.Wrap(err)
 	}
-	// Cleanup the tools directory with previously downloaded and un-archived versions.
-	if err := packaging.RemoveWithSuffix(u.toolsDir, updatePackageSuffix, pkgName); err != nil {
-		slog.WarnContext(ctx, "failed to clean up tools directory", "error", err)
-	}
 
 	return nil
 }
@@ -340,7 +352,7 @@ func (u *Updater) downloadHash(ctx context.Context, url string) ([]byte, error)
 	}
 	defer resp.Body.Close()
 	if resp.StatusCode == http.StatusNotFound {
-		return nil, trace.NotFound("hash file is not found: %v", resp.StatusCode)
+		return nil, trace.NotFound("hash file is not found: %q", url)
 	}
 	if resp.StatusCode != http.StatusOK {
 		return nil, trace.BadParameter("bad status when downloading archive hash: %v", resp.StatusCode)

lib/autoupdate/tools/utils.go

@@ -63,7 +63,8 @@ func DefaultClientTools() []string {
 	}
 }
 
-func checkToolVersion(toolsDir string) (string, error) {
+// CheckToolVersion returns current installed client tools version
+func CheckToolVersion(toolsDir string) (string, error) {
 	// Find the path to the current executable.
 	path, err := toolName(toolsDir)
 	if err != nil {
@@ -87,7 +88,7 @@ func checkToolVersion(toolsDir string) (string, error) {
 	command.Env = []string{teleportToolsVersionEnv + "=off"}
 	output, err := command.Output()
 	if err != nil {
-		return "", trace.Wrap(err)
+		return "", trace.WrapWithMessage(err, "failed to determine version of %q tool", path)
 	}
 
 	// The output for "{tsh, tctl} version" can be multiple lines. Find the

lib/utils/packaging/unarchive.go

@@ -38,13 +38,13 @@ const (
 )
 
 // RemoveWithSuffix removes all that matches the provided suffix, except for file or directory with `skipName`.
-func RemoveWithSuffix(dir, suffix, skipName string) error {
+func RemoveWithSuffix(dir, suffix string, skipNames []string) error {
 	var removePaths []string
 	err := filepath.Walk(dir, func(path string, info os.FileInfo, err error) error {
 		if err != nil {
 			return trace.Wrap(err)
 		}
-		if skipName == info.Name() {
+		if slices.Contains(skipNames, info.Name()) {
 			return nil
 		}
 		if !strings.HasSuffix(info.Name(), suffix) {

lib/utils/packaging/unarchive_test.go

@@ -132,7 +132,7 @@ func TestRemoveWithSuffix(t *testing.T) {
 	dirInSkipPath := filepath.Join(skipPath, dirForRemove)
 	require.NoError(t, os.MkdirAll(skipPath, 0o755))
 
-	err := RemoveWithSuffix(testDir, dirForRemove, skipName)
+	err := RemoveWithSuffix(testDir, dirForRemove, []string{skipName})
 	require.NoError(t, err)
 
 	_, err = os.Stat(filepath.Join(testDir, dirForRemove))

lib/utils/packaging/unarchive_unix.go

@@ -186,9 +186,10 @@ func replacePkg(toolsDir string, archivePath string, extractDir string, execName
 		// swap operations. This ensures that the "com.apple.macl" extended
 		// attribute is set and macOS will not send a SIGKILL to the process
 		// if multiple processes are trying to operate on it.
-		command := exec.Command(path, "version", "--client")
+		command := exec.Command(path, "version")
+		command.Env = []string{"TELEPORT_TOOLS_VERSION=off"}
 		if err := command.Run(); err != nil {
-			return trace.Wrap(err)
+			return trace.WrapWithMessage(err, "failed to validate binary")
 		}
 
 		// Due to macOS applications not being a single binary (they are a

tool/tctl/common/tctl.go

@@ -116,7 +116,10 @@ func Run(ctx context.Context, commands []CLICommand) {
 		utils.FatalError(err)
 	}
 	updater := tools.NewUpdater(tools.DefaultClientTools(), toolsDir, teleport.Version)
-	toolsVersion, reExec := updater.CheckLocal()
+	toolsVersion, reExec, err := updater.CheckLocal()
+	if err != nil {
+		utils.FatalError(err)
+	}
 	if reExec {
 		// Download the version of client tools required by the cluster. This
 		// is required if the user passed in the TELEPORT_TOOLS_VERSION

tool/tsh/common/tsh.go

@@ -704,7 +704,10 @@ func Run(ctx context.Context, args []string, opts ...CliOption) error {
 		return trace.Wrap(err)
 	}
 	updater := tools.NewUpdater(tools.DefaultClientTools(), toolsDir, teleport.Version)
-	toolsVersion, reExec := updater.CheckLocal()
+	toolsVersion, reExec, err := updater.CheckLocal()
+	if err != nil {
+		return trace.Wrap(err)
+	}
 	if reExec {
 		// Download the version of client tools required by the cluster. This
 		// is required if the user passed in the TELEPORT_TOOLS_VERSION