From 1d196f562bc7c4a2228a68f7c32830bcd77f3503 Mon Sep 17 00:00:00 2001 From: adityamaru Date: Tue, 15 Nov 2022 14:51:05 +0000 Subject: [PATCH] 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 | 26 ++- pkg/ccl/backupccl/backup_planning.go | 2 +- .../backupdest/backup_destination.go | 40 ++-- .../backupccl/backupencryption/encryption.go | 6 +- .../backupccl/backupinfo/manifest_handling.go | 45 +---- .../testdata/backup-restore/encrypted-backups | 190 ++++++++++++++++++ pkg/jobs/jobspb/jobs.proto | 2 +- 7 files changed, 239 insertions(+), 72 deletions(-) create mode 100644 pkg/ccl/backupccl/testdata/backup-restore/encrypted-backups diff --git a/pkg/ccl/backupccl/backup_job.go b/pkg/ccl/backupccl/backup_job.go index c452ffc8ef31..bf234302d5c1 100644 --- a/pkg/ccl/backupccl/backup_job.go +++ b/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 diff --git a/pkg/ccl/backupccl/backup_planning.go b/pkg/ccl/backupccl/backup_planning.go index 6ce5e6a929a2..084d6987f816 100644 --- a/pkg/ccl/backupccl/backup_planning.go +++ b/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 { diff --git a/pkg/ccl/backupccl/backupdest/backup_destination.go b/pkg/ccl/backupccl/backupdest/backup_destination.go index 7e4b43c70d4a..2dc6538e8813 100644 --- a/pkg/ccl/backupccl/backupdest/backup_destination.go +++ b/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,14 +573,14 @@ 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 // - 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() @@ -586,28 +588,22 @@ func ResolveBackupManifests( // 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 } diff --git a/pkg/ccl/backupccl/backupencryption/encryption.go b/pkg/ccl/backupccl/backupencryption/encryption.go index a63484eb8398..dc0851aeec62 100644 --- a/pkg/ccl/backupccl/backupencryption/encryption.go +++ b/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 diff --git a/pkg/ccl/backupccl/backupinfo/manifest_handling.go b/pkg/ccl/backupccl/backupinfo/manifest_handling.go index 948cef31d355..0529fa138550 100644 --- a/pkg/ccl/backupccl/backupinfo/manifest_handling.go +++ b/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 diff --git a/pkg/ccl/backupccl/testdata/backup-restore/encrypted-backups b/pkg/ccl/backupccl/testdata/backup-restore/encrypted-backups new file mode 100644 index 000000000000..edc6889c55bf --- /dev/null +++ b/pkg/ccl/backupccl/testdata/backup-restore/encrypted-backups @@ -0,0 +1,190 @@ +new-server name=s1 +---- + +exec-sql +CREATE DATABASE d; +USE d; +CREATE TABLE foo (i INT PRIMARY KEY, s STRING); +CREATE TABLE baz (i INT PRIMARY KEY, s STRING); +INSERT INTO baz VALUES (1, 'x'),(2,'y'),(3,'z'); +---- + +exec-sql +BACKUP INTO 'nodelocal://1/full' WITH encryption_passphrase='123'; +---- + +exec-sql +BACKUP INTO 'nodelocal://1/full2' WITH encryption_passphrase='456', incremental_location='nodelocal://1/inc'; +---- + +exec-sql +BACKUP INTO 'nodelocal://1/full3' WITH kms='testkms:///cmk?AUTH=implicit'; +---- + +# No passphrase results in an error. +exec-sql expect-error-regex=(file appears encrypted) +SELECT object_name, object_type, backup_type FROM [SHOW BACKUP FROM LATEST IN 'nodelocal://1/full'] +---- +regex matches error + +query-sql +SELECT object_name, object_type, backup_type FROM [SHOW BACKUP FROM LATEST IN 'nodelocal://1/full' +WITH encryption_passphrase='123'] WHERE database_name <> 'system' ORDER BY (backup_type, object_name) +---- +bank table full +baz table full +foo table full +public schema full +public schema full +public schema full +public schema full + +exec-sql expect-error-regex=(file appears encrypted) +SELECT object_name, object_type, backup_type FROM [SHOW BACKUP FROM LATEST IN 'nodelocal://1/full2' +WITH incremental_location='nodelocal://1/inc'] WHERE database_name <> 'system' ORDER BY (backup_type, object_name); +---- +regex matches error + +query-sql +SELECT object_name, object_type, backup_type FROM [SHOW BACKUP FROM LATEST IN 'nodelocal://1/full2' +WITH encryption_passphrase='456', incremental_location='nodelocal://1/inc'] WHERE database_name <> 'system' +ORDER BY (backup_type, object_name); +---- +bank table full +baz table full +foo table full +public schema full +public schema full +public schema full +public schema full + + +# Providing a passphrase when the backup is encrypted by KMS results in an error. +exec-sql expect-error-regex=(failed to decrypt) +SELECT object_name, object_type, backup_type FROM [SHOW BACKUP FROM LATEST IN 'nodelocal://1/full3' +WITH encryption_passphrase='123'] WHERE database_name <> 'system' ORDER BY (backup_type, object_name) +---- +regex matches error + +query-sql +SELECT object_name, object_type, backup_type FROM [SHOW BACKUP FROM LATEST IN 'nodelocal://1/full3' +WITH kms='testkms:///cmk?AUTH=implicit'] WHERE database_name <> 'system' ORDER BY (backup_type, object_name) +---- +bank table full +baz table full +foo table full +public schema full +public schema full +public schema full +public schema full + + +exec-sql +INSERT INTO baz VALUES(4, 'a'), (5, 'b'), (6, 'c'); +---- + +# Add an incremental layer to each backup. + +exec-sql +BACKUP INTO LATEST IN 'nodelocal://1/full' WITH encryption_passphrase='123'; +---- + +exec-sql +BACKUP INTO LATEST IN 'nodelocal://1/full2' WITH encryption_passphrase='456', incremental_location='nodelocal://1/inc'; +---- + +exec-sql +BACKUP INTO LATEST IN 'nodelocal://1/full3' WITH kms='testkms:///cmk?AUTH=implicit'; +---- + +query-sql +SELECT object_name, object_type, backup_type FROM [SHOW BACKUP FROM LATEST IN 'nodelocal://1/full' +WITH encryption_passphrase='123'] WHERE database_name <> 'system' ORDER BY (backup_type, object_name) +---- +bank table full +baz table full +foo table full +public schema full +public schema full +public schema full +public schema full +bank table incremental +baz table incremental +foo table incremental +public schema incremental +public schema incremental +public schema incremental +public schema incremental + +query-sql +SELECT object_name, object_type, backup_type FROM [SHOW BACKUP FROM LATEST IN 'nodelocal://1/full2' +WITH encryption_passphrase='456', incremental_location='nodelocal://1/inc'] WHERE database_name <> 'system' +ORDER BY (backup_type, object_name) +---- +bank table full +baz table full +foo table full +public schema full +public schema full +public schema full +public schema full +bank table incremental +baz table incremental +foo table incremental +public schema incremental +public schema incremental +public schema incremental +public schema incremental + +query-sql +SELECT object_name, object_type, backup_type FROM [SHOW BACKUP FROM LATEST IN 'nodelocal://1/full3' +WITH kms='testkms:///cmk?AUTH=implicit'] WHERE database_name <> 'system' +ORDER BY (backup_type, object_name) +---- +bank table full +baz table full +foo table full +public schema full +public schema full +public schema full +public schema full +bank table incremental +baz table incremental +foo table incremental +public schema incremental +public schema incremental +public schema incremental +public schema incremental + +exec-sql +RESTORE DATABASE d FROM LATEST IN 'nodelocal://1/full' WITH new_db_name='d2', encryption_passphrase='123'; +---- + +exec-sql +RESTORE DATABASE d FROM LATEST IN 'nodelocal://1/full2' WITH new_db_name='d3', encryption_passphrase='456', incremental_location='nodelocal://1/inc'; +---- + +exec-sql +RESTORE DATABASE d FROM LATEST IN 'nodelocal://1/full3' WITH new_db_name='d4', kms='testkms:///cmk?AUTH=implicit'; +---- + +query-sql +USE d2; +SELECT * FROM [SHOW TABLES] ORDER BY table_name; +---- +public baz table root +public foo table root + +query-sql +USE d3; +SELECT * FROM [SHOW TABLES] ORDER BY table_name; +---- +public baz table root +public foo table root + +query-sql +USE d4; +SELECT * FROM [SHOW TABLES] ORDER BY table_name; +---- +public baz table root +public foo table root diff --git a/pkg/jobs/jobspb/jobs.proto b/pkg/jobs/jobspb/jobs.proto index e0d412af9aba..0f67997b9bd4 100644 --- a/pkg/jobs/jobspb/jobs.proto +++ b/pkg/jobs/jobspb/jobs.proto @@ -51,7 +51,7 @@ message BackupEncryptionOptions { // encryption or decryption when mode == KMS. KMSInfo kms_info = 3 [(gogoproto.customname) = "KMSInfo"]; - string raw_passphrae = 4; + string raw_passphrase = 4; repeated string raw_kms_uris = 5; }