Merge pull request #830 from slackhq/slack-22_0_4-batch-02

sbaker617 · web-flow · commit 9b620991a9e1 · 2026-04-03T12:39:27.000-04:00
v22.0.4 backport batch 2: backup &amp; security hardening
diff --git a/go/flags/endtoend/vtbackup.txt b/go/flags/endtoend/vtbackup.txt
@@ -131,6 +131,7 @@ Flags:
       --external-compressor string                                  command with arguments to use when compressing a backup.
       --external-compressor-extension string                        extension to use when using an external compressor.
       --external-decompressor string                                command with arguments to use when decompressing a backup.
+      --external-decompressor-use-manifest                          allows the decompressor command stored in the backup manifest to be used at restore time. Enabling this is a security risk: an attacker with write access to the backup storage could modify the manifest to execute arbitrary commands on the tablet as the Vitess user. NOT RECOMMENDED.
       --file_backup_storage_root string                             Root directory for the file backup storage.
       --gcs_backup_storage_bucket string                            Google Cloud Storage bucket to use for backups.
       --gcs_backup_storage_root string                              Root prefix for all backup-related object names.
diff --git a/go/flags/endtoend/vtcombo.txt b/go/flags/endtoend/vtcombo.txt
@@ -132,6 +132,7 @@ Flags:
       --external-compressor string                                       command with arguments to use when compressing a backup.
       --external-compressor-extension string                             extension to use when using an external compressor.
       --external-decompressor string                                     command with arguments to use when decompressing a backup.
+      --external-decompressor-use-manifest                               allows the decompressor command stored in the backup manifest to be used at restore time. Enabling this is a security risk: an attacker with write access to the backup storage could modify the manifest to execute arbitrary commands on the tablet as the Vitess user. NOT RECOMMENDED.
       --external_topo_server                                             Should vtcombo use an external topology server instead of starting its own in-memory topology server. If true, vtcombo will use the flags defined in topo/server.go to open topo server
       --foreign_key_mode string                                          This is to provide how to handle foreign key constraint in create/alter table. Valid values are: allow, disallow (default "allow")
       --gate_query_cache_memory int                                      gate server query cache size in bytes, maximum amount of memory to be cached. vtgate analyzes every incoming query and generate a query plan, these plans are being cached in a lru cache. This config controls the capacity of the lru cache. (default 33554432)
diff --git a/go/flags/endtoend/vttablet.txt b/go/flags/endtoend/vttablet.txt
@@ -156,6 +156,7 @@ Flags:
       --external-compressor string                                       command with arguments to use when compressing a backup.
       --external-compressor-extension string                             extension to use when using an external compressor.
       --external-decompressor string                                     command with arguments to use when decompressing a backup.
+      --external-decompressor-use-manifest                               allows the decompressor command stored in the backup manifest to be used at restore time. Enabling this is a security risk: an attacker with write access to the backup storage could modify the manifest to execute arbitrary commands on the tablet as the Vitess user. NOT RECOMMENDED.
       --file_backup_storage_root string                                  Root directory for the file backup storage.
       --filecustomrules string                                           file based custom rule path
       --filecustomrules_watch                                            set up a watch on the target file and reload query rules when it changes
diff --git a/go/flags/endtoend/vttestserver.txt b/go/flags/endtoend/vttestserver.txt
@@ -38,6 +38,7 @@ Flags:
       --external-compressor string                                       command with arguments to use when compressing a backup.
       --external-compressor-extension string                             extension to use when using an external compressor.
       --external-decompressor string                                     command with arguments to use when decompressing a backup.
+      --external-decompressor-use-manifest                               allows the decompressor command stored in the backup manifest to be used at restore time. Enabling this is a security risk: an attacker with write access to the backup storage could modify the manifest to execute arbitrary commands on the tablet as the Vitess user. NOT RECOMMENDED.
       --external_topo_global_root string                                 the path of the global topology data in the global topology server for vtcombo process
       --external_topo_global_server_address string                       the address of the global topology server for vtcombo process
       --external_topo_implementation string                              the topology implementation to use for vtcombo process
diff --git a/go/test/endtoend/backup/vtctlbackup/backup_test.go b/go/test/endtoend/backup/vtctlbackup/backup_test.go
@@ -60,6 +60,7 @@ func TestBuiltinBackupWithExternalZstdCompressionAndManifestedDecompressor(t *te
 		CompressorEngineName:            "external",
 		ExternalCompressorCmd:           "zstd",
 		ExternalCompressorExt:           ".zst",
+		ExternalDecompressorUseManifest: true,
 		ManifestExternalDecompressorCmd: "zstd -d",
 	}
 
diff --git a/go/test/endtoend/backup/vtctlbackup/backup_utils.go b/go/test/endtoend/backup/vtctlbackup/backup_utils.go
@@ -92,6 +92,7 @@ type CompressionDetails struct {
 	ExternalCompressorCmd           string
 	ExternalCompressorExt           string
 	ExternalDecompressorCmd         string
+	ExternalDecompressorUseManifest bool
 	ManifestExternalDecompressorCmd string
 }
 
@@ -305,6 +306,9 @@ func getCompressorArgs(cDetails *CompressionDetails) []string {
 	if cDetails.ExternalDecompressorCmd != "" {
 		args = append(args, fmt.Sprintf("--external-decompressor=%s", cDetails.ExternalDecompressorCmd))
 	}
+	if cDetails.ExternalDecompressorUseManifest {
+		args = append(args, "--external-decompressor-use-manifest")
+	}
 	if cDetails.ManifestExternalDecompressorCmd != "" {
 		args = append(args, fmt.Sprintf("--manifest-external-decompressor=%s", cDetails.ManifestExternalDecompressorCmd))
 	}
diff --git a/go/test/endtoend/backup/xtrabackup/xtrabackup_test.go b/go/test/endtoend/backup/xtrabackup/xtrabackup_test.go
@@ -59,6 +59,7 @@ func TestXtrabackupWithExternalZstdCompressionAndManifestedDecompressor(t *testi
 		CompressorEngineName:            "external",
 		ExternalCompressorCmd:           "zstd",
 		ExternalCompressorExt:           ".zst",
+		ExternalDecompressorUseManifest: true,
 		ManifestExternalDecompressorCmd: "zstd -d",
 	}
 
diff --git a/go/vt/hook/hook.go b/go/vt/hook/hook.go
@@ -23,11 +23,12 @@ import (
 	"io"
 	"os"
 	"os/exec"
-	"path"
+	"path/filepath"
 	"strings"
 	"syscall"
 	"time"
 
+	"vitess.io/vitess/go/fileutil"
 	vtenv "vitess.io/vitess/go/vt/env"
 	"vitess.io/vitess/go/vt/log"
 )
@@ -98,19 +99,17 @@ func NewHookWithEnv(name string, params []string, env map[string]string) *Hook {
 
 // findHook tries to locate the hook, and returns the exec.Cmd for it.
 func (hook *Hook) findHook(ctx context.Context) (*exec.Cmd, int, error) {
-	// Check the hook path.
-	if strings.Contains(hook.Name, "/") {
-		return nil, HOOK_INVALID_NAME, fmt.Errorf("hook cannot contain '/'")
-	}
-
 	// Find our root.
 	root, err := vtenv.VtRoot()
 	if err != nil {
 		return nil, HOOK_VTROOT_ERROR, fmt.Errorf("cannot get VTROOT: %v", err)
 	}
 
 	// See if the hook exists.
-	vthook := path.Join(root, "vthook", hook.Name)
+	vthook, err := fileutil.SafePathJoin(filepath.Join(root, "vthook"), hook.Name)
+	if err != nil {
+		return nil, HOOK_INVALID_NAME, fmt.Errorf("invalid hook name %q: %v", hook.Name, err)
+	}
 	_, err = os.Stat(vthook)
 	if err != nil {
 		if os.IsNotExist(err) {
diff --git a/go/vt/mysqlctl/blackbox/backup_test.go b/go/vt/mysqlctl/blackbox/backup_test.go
@@ -20,6 +20,7 @@ package blackbox
 import (
 	"bytes"
 	"context"
+	"encoding/json"
 	"errors"
 	"fmt"
 	"io"
@@ -926,3 +927,60 @@ func TestExecuteRestoreFailToReadEachFileTwice(t *testing.T) {
 	require.Equal(t, 5, ss.SourceOpenStats)
 	require.Equal(t, 5, ss.SourceReadStats)
 }
+
+func TestBackupRetryPropagatesHashToManifest(t *testing.T) {
+	ctx := utils.LeakCheckContext(t)
+	backupRoot, keyspace, shard, ts := SetupCluster(ctx, t, 2, 2)
+
+	bufferPerFiles := make(map[string]*rwCloseFailFirstCall)
+	be := &mysqlctl.BuiltinBackupEngine{}
+	bh := &mysqlctl.FakeBackupHandle{}
+	bh.AddFileReturnF = func(filename string) mysqlctl.FakeBackupHandleAddFileReturn {
+		_, isRetry := bufferPerFiles[filename]
+		newBuffer := newWriteCloseFailFirstWrite(isRetry)
+		bufferPerFiles[filename] = newBuffer
+		return mysqlctl.FakeBackupHandleAddFileReturn{WriteCloser: newBuffer}
+	}
+
+	fakedb := fakesqldb.New(t)
+	defer fakedb.Close()
+	mysqld := mysqlctl.NewFakeMysqlDaemon(fakedb)
+	defer mysqld.Close()
+	mysqld.ExpectedExecuteSuperQueryList = []string{"STOP REPLICA", "START REPLICA"}
+
+	ctx, cancel := context.WithCancel(ctx)
+	backupResult, err := be.ExecuteBackup(ctx, mysqlctl.BackupParams{
+		Logger: logutil.NewMemoryLogger(),
+		Mysqld: mysqld,
+		Cnf: &mysqlctl.Mycnf{
+			InnodbDataHomeDir:     path.Join(backupRoot, "innodb"),
+			InnodbLogGroupHomeDir: path.Join(backupRoot, "log"),
+			DataDir:               path.Join(backupRoot, "datadir"),
+		},
+		Stats:                backupstats.NewFakeStats(),
+		Concurrency:          1,
+		HookExtraEnv:         map[string]string{},
+		TopoServer:           ts,
+		Keyspace:             keyspace,
+		Shard:                shard,
+		MysqlShutdownTimeout: MysqlShutdownTimeout,
+	}, bh)
+	cancel()
+
+	require.NoError(t, err)
+	require.Equal(t, mysqlctl.BackupUsable, backupResult)
+
+	// Parse the MANIFEST and verify all file entries have non-empty hashes.
+	manifestBuf, ok := bufferPerFiles["MANIFEST"]
+	require.True(t, ok, "MANIFEST should have been written")
+
+	var manifest struct {
+		FileEntries []mysqlctl.FileEntry
+	}
+	require.NoError(t, json.Unmarshal(manifestBuf.Bytes(), &manifest))
+
+	require.NotEmpty(t, manifest.FileEntries, "manifest should contain file entries")
+	for _, fe := range manifest.FileEntries {
+		assert.NotEmptyf(t, fe.Hash, "file %s should have a non-empty hash in the manifest", fe.Name)
+	}
+}
diff --git a/go/vt/mysqlctl/builtinbackupengine.go b/go/vt/mysqlctl/builtinbackupengine.go
@@ -657,6 +657,13 @@ func (be *BuiltinBackupEngine) backupFiles(
 		if err != nil {
 			return err
 		}
+		// Propagate retry results back to the original entries so the
+		// manifest records correct hashes and metadata.
+		for i, fe := range newFEs {
+			if fe.Name != "" {
+				fes[i] = fe
+			}
+		}
 	}
 
 	// Backup the MANIFEST file and apply retry logic.
@@ -1327,10 +1334,7 @@ func (be *BuiltinBackupEngine) restoreFile(ctx context.Context, params RestorePa
 			// for backward compatibility
 			deCompressionEngine = PgzipCompressor
 		}
-		externalDecompressorCmd := ExternalDecompressorCmd
-		if externalDecompressorCmd == "" && bm.ExternalDecompressor != "" {
-			externalDecompressorCmd = bm.ExternalDecompressor
-		}
+		externalDecompressorCmd := resolveExternalDecompressor(bm.ExternalDecompressor)
 		if externalDecompressorCmd != "" {
 			if deCompressionEngine == ExternalCompressor {
 				deCompressionEngine = externalDecompressorCmd
diff --git a/go/vt/mysqlctl/compression.go b/go/vt/mysqlctl/compression.go
@@ -52,9 +52,10 @@ var (
 	ExternalCompressorCmd           string
 	ExternalCompressorExt           string
 	ExternalDecompressorCmd         string
+	ExternalDecompressorUseManifest bool
 	ManifestExternalDecompressorCmd string
 
-	errUnsupportedDeCompressionEngine = errors.New("unsupported engine in MANIFEST. You need to provide --external-decompressor if using 'external' compression engine")
+	errUnsupportedDeCompressionEngine = errors.New("unsupported engine in MANIFEST. You need to provide --external-decompressor if using 'external' compression engine. Alternatively, set --external-decompressor-use-manifest to use the decompressor command from the backup manifest, but this is NOT RECOMMENDED as it is a security risk")
 	errUnsupportedCompressionEngine   = errors.New("unsupported engine value for --compression-engine-name. supported values are 'external', 'pgzip', 'pargzip', 'zstd', 'lz4'")
 
 	// this is used by getEngineFromExtension() to figure out which engine to use in case the user didn't specify
@@ -77,6 +78,7 @@ func registerBackupCompressionFlags(fs *pflag.FlagSet) {
 	fs.StringVar(&ExternalCompressorCmd, "external-compressor", ExternalCompressorCmd, "command with arguments to use when compressing a backup.")
 	fs.StringVar(&ExternalCompressorExt, "external-compressor-extension", ExternalCompressorExt, "extension to use when using an external compressor.")
 	fs.StringVar(&ExternalDecompressorCmd, "external-decompressor", ExternalDecompressorCmd, "command with arguments to use when decompressing a backup.")
+	fs.BoolVar(&ExternalDecompressorUseManifest, "external-decompressor-use-manifest", ExternalDecompressorUseManifest, "allows the decompressor command stored in the backup manifest to be used at restore time. Enabling this is a security risk: an attacker with write access to the backup storage could modify the manifest to execute arbitrary commands on the tablet as the Vitess user. NOT RECOMMENDED.")
 	fs.StringVar(&ManifestExternalDecompressorCmd, "manifest-external-decompressor", ManifestExternalDecompressorCmd, "command with arguments to store in the backup manifest when compressing a backup with an external compression engine.")
 }
 
@@ -91,6 +93,20 @@ func getExtensionFromEngine(engine string) (string, error) {
 	return "", fmt.Errorf("%w %q", errUnsupportedCompressionEngine, engine)
 }
 
+// resolveExternalDecompressor returns the external decompressor command to use
+// at restore time. The CLI flag (--external-decompressor) takes precedence. The
+// backup manifest value is only used when --external-decompressor-use-manifest
+// is explicitly set to true.
+func resolveExternalDecompressor(manifestDecompressor string) string {
+	if ExternalDecompressorCmd != "" {
+		return ExternalDecompressorCmd
+	}
+	if ExternalDecompressorUseManifest && manifestDecompressor != "" {
+		return manifestDecompressor
+	}
+	return ""
+}
+
 // Validates if the external decompressor exists and return its path.
 func validateExternalCmd(cmd string) (string, error) {
 	if cmd == "" {
diff --git a/go/vt/mysqlctl/compression_external_decompressor_test.go b/go/vt/mysqlctl/compression_external_decompressor_test.go
@@ -0,0 +1,93 @@
+/*
+Copyright 2026 The Vitess Authors.
+
+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 mysqlctl
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestResolveExternalDecompressor(t *testing.T) {
+	tests := []struct {
+		name                 string
+		cliDecompressorCmd   string
+		useManifest          bool
+		manifestDecompressor string
+		expected             string
+	}{
+		{
+			name:                 "CLI flag takes precedence over manifest",
+			cliDecompressorCmd:   "zstd -d",
+			useManifest:          true,
+			manifestDecompressor: "gzip -d",
+			expected:             "zstd -d",
+		},
+		{
+			name:                 "CLI flag takes precedence even when use-manifest is false",
+			cliDecompressorCmd:   "zstd -d",
+			useManifest:          false,
+			manifestDecompressor: "gzip -d",
+			expected:             "zstd -d",
+		},
+		{
+			name:                 "manifest used when use-manifest is true and no CLI flag",
+			cliDecompressorCmd:   "",
+			useManifest:          true,
+			manifestDecompressor: "gzip -d",
+			expected:             "gzip -d",
+		},
+		{
+			name:                 "manifest ignored when use-manifest is false",
+			cliDecompressorCmd:   "",
+			useManifest:          false,
+			manifestDecompressor: "gzip -d",
+			expected:             "",
+		},
+		{
+			name:                 "empty when nothing is set",
+			cliDecompressorCmd:   "",
+			useManifest:          false,
+			manifestDecompressor: "",
+			expected:             "",
+		},
+		{
+			name:                 "empty when use-manifest is true but manifest is empty",
+			cliDecompressorCmd:   "",
+			useManifest:          true,
+			manifestDecompressor: "",
+			expected:             "",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			origCmd := ExternalDecompressorCmd
+			origAllow := ExternalDecompressorUseManifest
+			t.Cleanup(func() {
+				ExternalDecompressorCmd = origCmd
+				ExternalDecompressorUseManifest = origAllow
+			})
+
+			ExternalDecompressorCmd = tt.cliDecompressorCmd
+			ExternalDecompressorUseManifest = tt.useManifest
+
+			result := resolveExternalDecompressor(tt.manifestDecompressor)
+			assert.Equal(t, tt.expected, result)
+		})
+	}
+}
diff --git a/go/vt/mysqlctl/filebackupstorage/file.go b/go/vt/mysqlctl/filebackupstorage/file.go
@@ -23,7 +23,6 @@ import (
 	"fmt"
 	"io"
 	"os"
-	"path"
 
 	"github.com/spf13/pflag"
 
@@ -96,7 +95,10 @@ func (fbh *FileBackupHandle) AddFile(ctx context.Context, filename string, files
 	if fbh.readOnly {
 		return nil, fmt.Errorf("AddFile cannot be called on read-only backup")
 	}
-	p := path.Join(FileBackupStorageRoot, fbh.dir, fbh.name, filename)
+	p, err := fileutil.SafePathJoin(FileBackupStorageRoot, fbh.dir, fbh.name, filename)
+	if err != nil {
+		return nil, err
+	}
 	f, err := os2.Create(p)
 	if err != nil {
 		return nil, err
@@ -188,7 +190,10 @@ func (fbs *FileBackupStorage) StartBackup(ctx context.Context, dir, name string)
 	}
 
 	// Create the subdirectory for this named backup.
-	p = path.Join(p, name)
+	p, err = fileutil.SafePathJoin(p, name)
+	if err != nil {
+		return nil, err
+	}
 	if err = os2.Mkdir(p); err != nil {
 		return nil, err
 	}
diff --git a/go/vt/mysqlctl/mysqlshellbackupengine.go b/go/vt/mysqlctl/mysqlshellbackupengine.go
diff --git a/go/vt/mysqlctl/xtrabackupengine.go b/go/vt/mysqlctl/xtrabackupengine.go
diff --git a/go/vt/vttablet/grpctmserver/server.go b/go/vt/vttablet/grpctmserver/server.go
diff --git a/go/vt/vttablet/tmrpctest/test_tm_rpc.go b/go/vt/vttablet/tmrpctest/test_tm_rpc.go

Original file line number	Diff line number	Diff line change
`@@ -60,6 +60,7 @@ func TestBuiltinBackupWithExternalZstdCompressionAndManifestedDecompressor(t *te`
`60`	`60`	`CompressorEngineName: "external",`
`61`	`61`	`ExternalCompressorCmd: "zstd",`
`62`	`62`	`ExternalCompressorExt: ".zst",`
	`63`	`+ ExternalDecompressorUseManifest: true,`
`63`	`64`	`ManifestExternalDecompressorCmd: "zstd -d",`
`64`	`65`	`}`
`65`	`66`
Original file line number	Diff line number	Diff line change
`@@ -92,6 +92,7 @@ type CompressionDetails struct {`
`92`	`92`	`ExternalCompressorCmd string`
`93`	`93`	`ExternalCompressorExt string`
`94`	`94`	`ExternalDecompressorCmd string`
	`95`	`+ ExternalDecompressorUseManifest bool`
`95`	`96`	`ManifestExternalDecompressorCmd string`
`96`	`97`	`}`
`97`	`98`
`@@ -305,6 +306,9 @@ func getCompressorArgs(cDetails *CompressionDetails) []string {`
`305`	`306`	`if cDetails.ExternalDecompressorCmd != "" {`
`306`	`307`	`args = append(args, fmt.Sprintf("--external-decompressor=%s", cDetails.ExternalDecompressorCmd))`
`307`	`308`	`}`
	`309`	`+ if cDetails.ExternalDecompressorUseManifest {`
	`310`	`+ args = append(args, "--external-decompressor-use-manifest")`
	`311`	`+ }`
`308`	`312`	`if cDetails.ManifestExternalDecompressorCmd != "" {`
`309`	`313`	`args = append(args, fmt.Sprintf("--manifest-external-decompressor=%s", cDetails.ManifestExternalDecompressorCmd))`
`310`	`314`	`}`
Original file line number	Diff line number	Diff line change
`@@ -59,6 +59,7 @@ func TestXtrabackupWithExternalZstdCompressionAndManifestedDecompressor(t *testi`
`59`	`59`	`CompressorEngineName: "external",`
`60`	`60`	`ExternalCompressorCmd: "zstd",`
`61`	`61`	`ExternalCompressorExt: ".zst",`
	`62`	`+ ExternalDecompressorUseManifest: true,`
`62`	`63`	`ManifestExternalDecompressorCmd: "zstd -d",`
`63`	`64`	`}`
`64`	`65`