Fixes from the phase-1 code review
- one locking protocol per location: backup steps and restores hold a shared lock, prune and untrash an exclusive one and refuse while any run (backup, restore, export) exists; starting a backup checks and creates the run under the exclusive lock, closing the race with prune - snapshot entries are validated when read (relative path without '.', '..', NUL, backslash or control characters); raw restore also checks that the resolved directory stays below the target; the restore API and service accept canonical paths only - recovery kit confirmation requires a recent password confirmation and the confirmation code printed in the current kit file (new code per kit version), proving the kit was actually downloaded - prune decides on the authenticated index files on the location and refreshes the local index cache from them - restore job marks the run failed on any error; restore runs without progress for 6 hours are failed so they cannot block cleanup forever - tests for safe paths and a malicious snapshot path
This commit is contained in:
@@ -52,9 +52,22 @@ class BackupService {
|
||||
if ($this->config->getSystemValue('objectstore', null) !== null) {
|
||||
throw new \RuntimeException('Object storage as primary storage is not supported yet');
|
||||
}
|
||||
if ($this->locking->isLocked(PruneService::lockKey($target), ILockingProvider::LOCK_EXCLUSIVE)) {
|
||||
throw new \RuntimeException('A cleanup of ' . $target->getName() . ' is running; try again later');
|
||||
// Check and create the run under the location's exclusive lock, so a cleanup cannot start
|
||||
// in between (it takes the same lock and refuses while a run exists).
|
||||
$lock = PruneService::lockKey($target);
|
||||
try {
|
||||
$this->locking->acquireLock($lock, ILockingProvider::LOCK_EXCLUSIVE);
|
||||
} catch (LockedException) {
|
||||
throw new \RuntimeException('A cleanup or another start for ' . $target->getName() . ' is running; try again later');
|
||||
}
|
||||
try {
|
||||
return $this->createRun($target, $label);
|
||||
} finally {
|
||||
$this->locking->releaseLock($lock, ILockingProvider::LOCK_EXCLUSIVE);
|
||||
}
|
||||
}
|
||||
|
||||
private function createRun(Target $target, string $label): Run {
|
||||
$running = $this->runs->findRunning($target->getId(), self::KIND_FULL);
|
||||
if ($running !== []) {
|
||||
return $running[0];
|
||||
@@ -96,6 +109,14 @@ class BackupService {
|
||||
} catch (LockedException) {
|
||||
return $run;
|
||||
}
|
||||
// Shared lock on the location while this step writes: a cleanup (exclusive) cannot run now.
|
||||
$targetLock = 'ng_backup/target/' . $run->getTargetId();
|
||||
try {
|
||||
$this->locking->acquireLock($targetLock, ILockingProvider::LOCK_SHARED);
|
||||
} catch (LockedException) {
|
||||
$this->locking->releaseLock($lockKey, ILockingProvider::LOCK_EXCLUSIVE);
|
||||
return $run;
|
||||
}
|
||||
try {
|
||||
$run = $this->runs->find($run->getId()); // fresh state after acquiring the lock
|
||||
if ($run->getStatus() !== Run::RUNNING || $run->getKind() !== self::KIND_FULL) {
|
||||
@@ -140,6 +161,7 @@ class BackupService {
|
||||
}
|
||||
return $run;
|
||||
} finally {
|
||||
$this->locking->releaseLock($targetLock, ILockingProvider::LOCK_SHARED);
|
||||
$this->locking->releaseLock($lockKey, ILockingProvider::LOCK_EXCLUSIVE);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user