From 89ba561a6f5ee3b6488128c38f537adf65664425 Mon Sep 17 00:00:00 2001 From: Nick <126941599+coo1white@users.noreply.github.com> Date: Sat, 25 Jul 2026 19:57:51 +0700 Subject: [PATCH] fix(backup): keep the S3 secret encrypted when a save inherits it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit UpdateS3Config only encrypted the SecretAccessKey on the branch where the caller supplied a new one. The other branch inherits the stored secret through loadS3Config, which *decrypts* it — so that path wrote the plaintext straight back to the settings row and overwrote the ciphertext. It is the normal path, not an edge case. UpdateS3Config clears the secret in its own return value, so the admin form's field is empty afterwards. Any later save — changing the endpoint, saving the schedule, pressing save twice — sends an empty secret and takes the inheriting branch. One extra save is enough to leave the secret in plaintext at rest. Nothing surfaced because loadS3Config falls back to the raw value when decryption fails, for compatibility with pre-encryption rows. Backups kept working and the only trace was a line per read: [Backup] S3 SecretAccessKey 解密失败（可能是旧的未加密数据）: decrypt: cipher: message authentication failed Seen on a live install: the stored value was 64 characters, exactly the plaintext length of an R2 secret, where AES-GCM + base64 would have been 124. The compatibility fallback turns a one-time write bug into a permanent plaintext secret. It also feeds itself: pg_dump includes the settings table, so the backup uploaded to the bucket carries the credentials for that same bucket. Fix: hoist the encryption out of the if/else so it runs for a non-empty secret whichever way it arrived. The ephemeral-key guard (#4524) moves with it, and the no-secret-at-all case still skips encryption entirely, so an update with no stored secret keeps working under an auto-generated key. The existing KeepExistingSecret test passed either way, because it only asserted loadS3Config returns the plaintext — true with the fallback too. The new test asserts on the row itself: still ciphertext after a second save, not double-wrapped after a third. Checked the new test fails without the fix and passes with it. --- backend/internal/service/backup_service.go | 12 +++-- .../internal/service/backup_service_test.go | 47 +++++++++++++++++++ 2 files changed, 56 insertions(+), 3 deletions(-) diff --git a/backend/internal/service/backup_service.go b/backend/internal/service/backup_service.go index 14080ecde8..0682ba0e36 100644 --- a/backend/internal/service/backup_service.go +++ b/backend/internal/service/backup_service.go @@ -276,19 +276,25 @@ func (s *BackupService) GetS3Config(ctx context.Context) (*BackupS3Config, error } func (s *BackupService) UpdateS3Config(ctx context.Context, cfg BackupS3Config) (*BackupS3Config, error) { - // 如果没提供 secret，保留原有值 + // 如果没提供 secret，保留原有值。loadS3Config 会解密，所以这里拿到的是明文， + // 和调用方新填的 secret 一样，都必须走下面的加密再落库。 if cfg.SecretAccessKey == "" { old, _ := s.loadS3Config(ctx) if old != nil { cfg.SecretAccessKey = old.SecretAccessKey } - } else { + } + + // 加密 SecretAccessKey。这一步不能只在“调用方填了新 secret”时做：表单保存成功 + // 后本函数会清空返回值里的 secret，所以第二次保存（改端点、存定时配置）送上来的 + // secret 是空的，会走上面的分支继承旧值。若那条路径跳过加密，明文就会覆盖掉库里 + // 的密文，而读取侧“兼容未加密旧数据”的回退会把这件事永久掩盖成一条日志。 + if cfg.SecretAccessKey != "" { // 拒绝用自动生成的临时密钥加密：该密钥每次重启都会变化，落库的密文在 // 重启/升级后无法解密（#4524）。与支付、TOTP 的处理保持一致。 if !s.encryptionKeyConfigured { return nil, ErrSecretEncryptionKeyNotConfigured } - // 加密 SecretAccessKey encrypted, err := s.encryptor.Encrypt(cfg.SecretAccessKey) if err != nil { return nil, fmt.Errorf("encrypt secret: %w", err) diff --git a/backend/internal/service/backup_service_test.go b/backend/internal/service/backup_service_test.go index c39fa6b098..e395d6b947 100644 --- a/backend/internal/service/backup_service_test.go +++ b/backend/internal/service/backup_service_test.go @@ -304,6 +304,53 @@ func TestBackupService_S3ConfigKeepExistingSecret(t *testing.T) { require.Equal(t, "AKID-NEW", internal.AccessKeyID) } +// 一次不带 secret 的保存（表单第二次提交、改端点、存定时配置）继承的是 loadS3Config +// 解密后的明文。若那条路径跳过加密，明文就会覆盖库里的密文，而读取侧“兼容未加密旧 +// 数据”的回退会把它掩盖成一条日志，功能照常，密钥却是明文落库的。 +func TestBackupService_S3ConfigStaysEncryptedAfterSecondSave(t *testing.T) { + repo := newMockSettingRepo() + svc := newTestBackupService(repo, &mockDumper{}, newMockObjectStore()) + + _, err := svc.UpdateS3Config(context.Background(), BackupS3Config{ + Bucket: "my-bucket", + AccessKeyID: "AKID", + SecretAccessKey: "original-secret", + }) + require.NoError(t, err) + + storedSecret := func() string { + raw, _ := repo.GetValue(context.Background(), settingKeyBackupS3Config) + var stored BackupS3Config + require.NoError(t, json.Unmarshal([]byte(raw), &stored)) + return stored.SecretAccessKey + } + require.Equal(t, "ENC:original-secret", storedSecret()) + + // 第二次保存不带 secret，只改别的字段。 + _, err = svc.UpdateS3Config(context.Background(), BackupS3Config{ + Bucket: "my-bucket", + AccessKeyID: "AKID-NEW", + }) + require.NoError(t, err) + + require.Equal(t, "ENC:original-secret", storedSecret(), + "secret must stay encrypted at rest after a save that inherits it") + require.NotEqual(t, "original-secret", storedSecret(), "secret must never be stored as plaintext") + + // 第三次保存，确认不会反复套壳加密。 + _, err = svc.UpdateS3Config(context.Background(), BackupS3Config{ + Bucket: "my-bucket", + AccessKeyID: "AKID-THIRD", + }) + require.NoError(t, err) + require.Equal(t, "ENC:original-secret", storedSecret(), "secret must not be double-encrypted") + + internal, err := svc.loadS3Config(context.Background()) + require.NoError(t, err) + require.Equal(t, "original-secret", internal.SecretAccessKey) + require.Equal(t, "AKID-THIRD", internal.AccessKeyID) +} + func TestBackupService_UpdateS3Config_RejectsEphemeralKey(t *testing.T) { repo := newMockSettingRepo() svc := newTestBackupServiceEphemeralKey(repo)