backupccl: fix bug in resolving encrypted backup manifests

In #87311 we refactored `ResolveBackupManifests` to concurrently
load manifests from base and incremental layers by calling
`FetchPreviousBackups`. The mistake in that diff was to
pass in only the incremental layers of the backup to `FetchPreviousBackup`
instead of passing in the base backup + incremental layers.
This was silently passing all tests because in `ResolveBackupManifests`
we explicitly setup the base backup layer in all the relevant slices before
processing each incremental layer. The one case this was not okay in
was encrypted backups. `FetchPreviousBackups` apart from doing the obvious would
also read the encryption options from the base backups before reading the
manifests from each layer. Now, because we stopped sending the base backup
in the input slice, this step would fail since the method would go to the
first incremental backup (instead of the base backup) and attempt to
read the ENCRYPTION_INFO file. This file is only ever written to the base
backup and so a `SHOW BACKUP` or a `RESTORE` of an encrypted backup would
fail with a file not found error.

In this diff we:

1) Fix `ResolveBackupManifests` by passing in the base backup + incremental
backups to `FetchPreviousBackups`.

2) Make it the callers responsibility to pass in the fully hydrated encryption
options before calling `FetchPreviousBackups` so that the method is *only* fetching
backup manifests.

Fixes: #91886

Release note (bug fix): fixes a bug that would cause `SHOW BACKUP` and
`RESTORE` of encrypted incremental backups to fail

pkg/ccl/backupccl/backup_job.go

@@ -808,15 +808,25 @@ func getBackupDetailAndManifest(
 	mem := execCfg.RootMemoryMonitor.MakeBoundAccount()
 	defer mem.Close(ctx)
 
-	prevBackups, encryptionOptions, memSize, err := backupinfo.FetchPreviousBackups(ctx, &mem, user,
-		makeCloudStorage, backupDestination.PrevBackupURIs, *initialDetails.EncryptionOptions, &kmsEnv)
+	var prevBackups []backuppb.BackupManifest
+	var baseEncryptionOptions *jobspb.BackupEncryptionOptions
+	if len(backupDestination.PrevBackupURIs) != 0 {
+		var err error
+		baseEncryptionOptions, err = backupencryption.GetEncryptionFromBase(ctx, user, makeCloudStorage,
+			backupDestination.PrevBackupURIs[0], *initialDetails.EncryptionOptions, &kmsEnv)
+		if err != nil {
+			return jobspb.BackupDetails{}, backuppb.BackupManifest{}, err
+		}
 
-	if err != nil {
-		return jobspb.BackupDetails{}, backuppb.BackupManifest{}, err
+		var memSize int64
+		prevBackups, memSize, err = backupinfo.GetBackupManifests(ctx, &mem, user,
+			makeCloudStorage, backupDestination.PrevBackupURIs, baseEncryptionOptions, &kmsEnv)
+
+		if err != nil {
+			return jobspb.BackupDetails{}, backuppb.BackupManifest{}, err
+		}
+		defer mem.Shrink(ctx, memSize)
 	}
-	defer func() {
-		mem.Shrink(ctx, memSize)
-	}()
 
 	if len(prevBackups) > 0 {
 		baseManifest := prevBackups[0]
@@ -889,7 +899,7 @@ func getBackupDetailAndManifest(
 		backupDestination.ChosenSubdir,
 		backupDestination.URIsByLocalityKV,
 		prevBackups,
-		encryptionOptions,
+		baseEncryptionOptions,
 		&kmsEnv)
 	if err != nil {
 		return jobspb.BackupDetails{}, backuppb.BackupManifest{}, err

pkg/ccl/backupccl/backup_planning.go

@@ -659,7 +659,7 @@ func backupPlanHook(
 			if err := requireEnterprise(p.ExecCfg(), "encryption"); err != nil {
 				return err
 			}
-			encryptionParams.RawPassphrae = pw
+			encryptionParams.RawPassphrase = pw
 		case jobspb.EncryptionMode_KMS:
 			encryptionParams.RawKmsUris = kms
 			if err := requireEnterprise(p.ExecCfg(), "encryption"); err != nil {

pkg/ccl/backupccl/backupdest/backup_destination.go

@@ -97,6 +97,8 @@ type ResolvedDestination struct {
 	URIsByLocalityKV map[string]string
 
 	// PrevBackupURIs is the list of full paths for previous backups in the chain.
+	// This includes the base backup at index 0, and any subsequent incremental
+	// backups. This field will not be populated when running a full backup.
 	PrevBackupURIs []string
 }
 
@@ -535,14 +537,14 @@ func ResolveBackupManifests(
 	}
 	ownedMemSize += memSize
 
-	var prev []string
+	var incrementalBackups []string
 	if len(incStores) > 0 {
-		prev, err = FindPriorBackups(ctx, incStores[0], includeManifest)
+		incrementalBackups, err = FindPriorBackups(ctx, incStores[0], includeManifest)
 		if err != nil {
 			return nil, nil, nil, 0, err
 		}
 	}
-	numLayers := len(prev) + 1
+	numLayers := len(incrementalBackups) + 1
 
 	defaultURIs = make([]string, numLayers)
 	mainBackupManifests = make([]backuppb.BackupManifest, numLayers)
@@ -571,43 +573,37 @@ func ResolveBackupManifests(
 			}
 		}
 
-		// For each backup layer we construct the default URI. We don't load the
-		// manifests in this loop since we want to do that concurrently.
-		for i := range prev {
-			// prev[i] is the path to the manifest file itself for layer i -- the
+		// For each incremental backup layer we construct the default URI. We don't
+		// load the manifests in this loop since we want to do that concurrently.
+		for i := range incrementalBackups {
+			// incrementalBackups[i] is the path to the manifest file itself for layer i -- the
 			// dirname piece of that path is the subdirectory in each of the
 			// partitions in which we'll also expect to find a partition manifest.
 			// Recall full inc URI is <prefix>/<subdir>/<incSubDir>
-			incSubDir := path.Dir(prev[i])
+			incSubDir := path.Dir(incrementalBackups[i])
 			u := *baseURIs[0] // NB: makes a copy to avoid mutating the baseURI.
 			u.Path = backuputils.JoinURLPath(u.Path, incSubDir)
 			defaultURIs[i+1] = u.String()
 		}
 
 		// Load the default backup manifests for each backup layer, this is done
 		// concurrently.
-		enc := jobspb.BackupEncryptionOptions{
-			Mode: jobspb.EncryptionMode_None,
-		}
-		if encryption != nil {
-			enc = *encryption
-		}
-		defaultManifestsForEachLayer, _, memSize, err := backupinfo.FetchPreviousBackups(ctx, mem, user,
-			mkStore, defaultURIs[1:], enc, kmsEnv)
+		defaultManifestsForEachLayer, memSize, err := backupinfo.GetBackupManifests(ctx, mem, user,
+			mkStore, defaultURIs, encryption, kmsEnv)
 		if err != nil {
 			return nil, nil, nil, 0, err
 		}
 		ownedMemSize += memSize
 
-		// Iterate over the layers one last time to memoize the loaded manifests and
-		// read the locality info.
+		// Iterate over the incremental backups one last time to memoize the loaded
+		// manifests and read the locality info.
 		//
 		// TODO(adityamaru): Parallelize the loading of the locality descriptors.
-		for i := range prev {
+		for i := range incrementalBackups {
 			// The manifest for incremental layer i slots in at i+1 since the full
 			// backup manifest occupies index 0 in `mainBackupManifests`.
-			mainBackupManifests[i+1] = defaultManifestsForEachLayer[i]
-			incSubDir := path.Dir(prev[i])
+			mainBackupManifests[i+1] = defaultManifestsForEachLayer[i+1]
+			incSubDir := path.Dir(incrementalBackups[i])
 			partitionURIs := make([]string, numPartitions)
 			for j := range baseURIs {
 				u := *baseURIs[j] // NB: makes a copy to avoid mutating the baseURI.
@@ -616,7 +612,7 @@ func ResolveBackupManifests(
 			}
 
 			localityInfo[i+1], err = backupinfo.GetLocalityInfo(ctx, incStores, partitionURIs,
-				defaultManifestsForEachLayer[i], encryption, kmsEnv, incSubDir)
+				defaultManifestsForEachLayer[i+1], encryption, kmsEnv, incSubDir)
 			if err != nil {
 				return nil, nil, nil, 0, err
 			}

pkg/ccl/backupccl/backupencryption/encryption.go

@@ -237,7 +237,7 @@ func MakeNewEncryptionOptions(
 		encryptionInfo = &jobspb.EncryptionInfo{Salt: salt}
 		encryptionOptions = &jobspb.BackupEncryptionOptions{
 			Mode: jobspb.EncryptionMode_Passphrase,
-			Key:  storageccl.GenerateKey([]byte(encryptionParams.RawPassphrae), salt),
+			Key:  storageccl.GenerateKey([]byte(encryptionParams.RawPassphrase), salt),
 		}
 	case jobspb.EncryptionMode_KMS:
 		// Generate a 32 byte/256-bit crypto-random number which will serve as
@@ -354,7 +354,7 @@ func WriteNewEncryptionInfoToBackup(
 	return cloud.WriteFile(ctx, dest, newEncryptionInfoFile, bytes.NewReader(buf))
 }
 
-// GetEncryptionFromBase retrieves the encryption options of a base backup. It
+// GetEncryptionFromBase retrieves the encryption options of the base backup. It
 // is expected that incremental backups use the same encryption options as the
 // base backups.
 func GetEncryptionFromBase(
@@ -381,7 +381,7 @@ func GetEncryptionFromBase(
 		case jobspb.EncryptionMode_Passphrase:
 			encryptionOptions = &jobspb.BackupEncryptionOptions{
 				Mode: jobspb.EncryptionMode_Passphrase,
-				Key:  storageccl.GenerateKey([]byte(encryptionParams.RawPassphrae), opts[0].Salt),
+				Key:  storageccl.GenerateKey([]byte(encryptionParams.RawPassphrase), opts[0].Salt),
 			}
 		case jobspb.EncryptionMode_KMS:
 			var defaultKMSInfo *jobspb.BackupEncryptionOptions_KMSInfo

pkg/ccl/backupccl/backupinfo/manifest_handling.go

@@ -1291,43 +1291,11 @@ func NewTimestampedCheckpointFileName() string {
 	return fmt.Sprintf("%s-%s", BackupManifestCheckpointName, hex.EncodeToString(buffer))
 }
 
-// FetchPreviousBackups takes a list of URIs of previous backups and returns
-// their manifest as well as the encryption options of the first backup in the
-// chain.
-func FetchPreviousBackups(
-	ctx context.Context,
-	mem *mon.BoundAccount,
-	user username.SQLUsername,
-	makeCloudStorage cloud.ExternalStorageFromURIFactory,
-	prevBackupURIs []string,
-	encryptionParams jobspb.BackupEncryptionOptions,
-	kmsEnv cloud.KMSEnv,
-) ([]backuppb.BackupManifest, *jobspb.BackupEncryptionOptions, int64, error) {
-	ctx, sp := tracing.ChildSpan(ctx, "backupinfo.FetchPreviousBackups")
-	defer sp.Finish()
-
-	if len(prevBackupURIs) == 0 {
-		return nil, nil, 0, nil
-	}
-
-	baseBackup := prevBackupURIs[0]
-	encryptionOptions, err := backupencryption.GetEncryptionFromBase(ctx, user, makeCloudStorage, baseBackup,
-		encryptionParams, kmsEnv)
-	if err != nil {
-		return nil, nil, 0, err
-	}
-	prevBackups, size, err := getBackupManifests(ctx, mem, user, makeCloudStorage, prevBackupURIs,
-		encryptionOptions, kmsEnv)
-	if err != nil {
-		return nil, nil, 0, err
-	}
-
-	return prevBackups, encryptionOptions, size, nil
-}
-
-// getBackupManifests fetches the backup manifest from a list of backup URIs.
-// The manifests are loaded from External Storage in parallel.
-func getBackupManifests(
+// GetBackupManifests fetches the backup manifest from a list of backup URIs.
+// The caller is expected to pass in the fully hydrated encryptionParams
+// required to read the manifests. The manifests are loaded from External
+// Storage in parallel.
+func GetBackupManifests(
 	ctx context.Context,
 	mem *mon.BoundAccount,
 	user username.SQLUsername,
@@ -1336,6 +1304,9 @@ func getBackupManifests(
 	encryption *jobspb.BackupEncryptionOptions,
 	kmsEnv cloud.KMSEnv,
 ) ([]backuppb.BackupManifest, int64, error) {
+	ctx, sp := tracing.ChildSpan(ctx, "backupinfo.GetBackupManifests")
+	defer sp.Finish()
+
 	manifests := make([]backuppb.BackupManifest, len(backupURIs))
 	if len(backupURIs) == 0 {
 		return manifests, 0, nil