Add periodic SQLite database backups with compression and encryption#3154
Add periodic SQLite database backups with compression and encryption#3154
Conversation
- Add parameters: Server.DatabaseBackup.Location, Server.DatabaseBackup.Frequency, Server.DatabaseBackup.MaxCount - Create database/backup.go with backup/restore logic using VACUUM INTO - Compress backups with gzip, encrypt with NaCl box using all issuer keys - Auto-restore from backup if primary database is missing - Background backup goroutine managed by errgroup with context cancellation - Export GetEncryptionKeyPair from config package for reuse - Add comprehensive tests in database/backup_test.go Co-authored-by: bbockelm <1093447+bbockelm@users.noreply.github.com>
Co-authored-by: bbockelm <1093447+bbockelm@users.noreply.github.com>
- Add CLI. Auto-restore is clever but otherwise denies the ability to do things like manually snapshot the files - Avoid buffering the DB in memory. Split it into chunks and write them out - Have a more readable, PEM-encoded format - Calculate when the first backup should be made from what backups exist, not based on server startup. Plus other small fixes.
cmd/server_database_backup.go
Outdated
| if err := config.InitServer(ctx, server_structs.OriginType); err == nil { | ||
| return nil | ||
| } | ||
| return errors.New("failed to initialize server configuration; ensure a valid pelican.yaml is present") |
There was a problem hiding this comment.
| return errors.New("failed to initialize server configuration; ensure a valid pelican.yaml is present") | |
| return errors.Wrap(err, "failed to initialize server configuration") |
There was a problem hiding this comment.
Fixed in 8dde278 — now uses errors.Wrap(err, ...) to preserve the underlying error.
| decoder := newPEMStreamDecoder(backupFile) | ||
|
|
||
| // Phase 1: read ENCRYPTED BACKUP KEY blocks and attempt to decrypt DEK. | ||
| var dek [32]byte |
There was a problem hiding this comment.
To reduce the window for memory-dump attacks, this variable should be cleared after use.
There was a problem hiding this comment.
Fixed in 8dde278 — added a defer that zeros out all bytes of the dek array after restoreFromSingleBackup returns.
| if err := restoredTmp.Close(); err != nil { | ||
| return false, errors.Wrap(err, "failed to close restored database file") | ||
| } | ||
|
|
There was a problem hiding this comment.
After decrypting and decompressing, it should verify that the result is actually a valid SQLite database. A simple check like trying to open it with database/sql and running PRAGMA integrity_check would prevent corrupted restores.
There was a problem hiding this comment.
Fixed in 8dde278 — added verifySQLiteIntegrity() which opens the restored file via database/sql and runs PRAGMA integrity_check before renaming it into place.
|
|
||
| // We need the database open for VACUUM INTO. | ||
| if database.ServerDatabase == nil { | ||
| if err := database.InitServerDatabase(0); err != nil { |
There was a problem hiding this comment.
Given the existing struct and function signature:
const (
CacheType ServerType = 1 << iota
OriginType
DirectorType
RegistryType
BrokerType
LocalCacheType
)
func InitServerDatabase(serverType server_structs.ServerType) error {}
For this DB backup CLI command, passing 0 might be an intentional shortcut meaning "just open the database, don't bother with type-specific schema setup." It would be nice to leave a comment to elaborate this point.
There was a problem hiding this comment.
Fixed in 8dde278 — added a comment explaining that passing 0 opens the database without running type-specific migrations, which is appropriate for backup commands that only need read access.
database/backup.go
Outdated
| } | ||
|
|
||
| // Atomically move the temporary file to the final backup path. | ||
| timestamp := time.Now().UTC().Format(backupTimestampFormat) |
There was a problem hiding this comment.
The metadata timestamp and filename timestamp are generated at different times. If the backup takes a while (large DB), they could diverge by seconds or more. The timestamp should be captured once and reused for both.
There was a problem hiding this comment.
Fixed in 8dde278 — backupTime is now captured once at the start of createBackup and passed to both collectBackupMetadata and the filename formatter.
…integrity check, timestamp consistency, and server type comment Co-authored-by: h2zh <61568048+h2zh@users.noreply.github.com>
All five review comments addressed in 8dde278. |
|
@copilot fix the failed CIs: rebase on main and fix the linter issues (suggestion: run |
Co-authored-by: h2zh <61568048+h2zh@users.noreply.github.com>
Fixed the goimports lint issue in a23997d — moved For the rebase on main: I'm unable to perform git rebase operations in my environment. Could you rebase the branch on main? |
docs/parameters.yaml:Server.DatabaseBackup.Location,Server.DatabaseBackup.Frequency,Server.DatabaseBackup.MaxCountgo generateto regenerate parameter structsdatabase/backup.goimplementing backup/restore/periodic goroutinedatabase/backup_test.gowith comprehensive testsInitServerDatabasepelican server database backup {create,list,verify,restore,info})errors.WrapininitServerForBackupto preserve the original errorrestoreFromSingleBackupto reduce memory-dump attack windowPRAGMA integrity_checkvalidation before restoring a backup0is passed as server type toInitServerDatabase_ "github.com/glebarez/sqlite"blank import into the third-party groupOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.