Skip to content

backup/atomic: deferred hardening from the #45 review #61

Description

@LevMuchnik

Deferred from the /review of PR #60 (issue #45). All informational; the six critical findings were fixed in that PR.

backend/atomic.py

  • os.chmod(tmp, mode) and shutil.copystat(src, tmp) act on the temp PATH after the fd is closed, rather than on the descriptor. Not reachable in this threat model (mkstemp is O_CREAT|O_EXCL with an unpredictable name, and the destination dirs are root-owned) but a free class to remove: os.fchmod(f.fileno(), mode) inside the with.
  • _UMASK = os.umask(0); os.umask(_UMASK) at module scope is a process-global get-and-set. Correct as the code stands (the module is imported during app import, while the process is single-threaded) but the safety of a utility module now depends on an import-ordering invariant nothing enforces. Any future first-import from an executor thread would leave umask at 0 for the duration. Make it lazy behind a lock, or import backend.atomic explicitly in main.py with a comment naming the invariant.
  • _copy_tolerating_rename lives in backup/runner.py but is the consumer half of atomic.py: the shfs measurement (118 ENOENT in 46,337 reads) is now repeated verbatim in three places. Move the wrapper into atomic.py and leave one-line pointers.

backend/backup/cloud_auth.py

  • A SIGKILL between mkstemp and os.replace leaves a .rcpt-tmp-* at the data_dir root holding the COMPLETE rclone.conf (OAuth refresh tokens plus any hand-added sftp/S3 secret). It is 0600 and the data_dir root is not a backup tree, so it cannot reach the cloud archive — but nothing reaps it, and an operator-driven rsync/tarball/support bundle would pick up credential litter the visible inventory does not account for. Sweep TMP_PREFIX entries older than a few minutes at startup, alongside restore_rclone_config.

scripts/restore_backup.py

  • _place branches on os.path.isdir(src) with a copy2 fallback, and the loop gates on os.path.exists rather than the type the policy implies. A corrupt backup where storage arrived as a regular file is copied into staging as a file named storage; the restore then swaps, and the next save_original raises NotADirectoryError. The pre-backup: storage/ is copied live and never verified, and includes regenerable data #45 code's isdir check happened to reject this. Record the expected type per entry and raise RestoreRefused on a mismatch.
  • The free-space precheck sizes only the backup, but assembly now also copies target-sourced entries into staging (scanner_test_set on the fallback, plus rclone.conf).
  • The RESTORE_SOURCES loop is unwrapped: an ENOSPC or EPERM inside _place escapes restore() as neither RestoreRefused nor TargetWritten, so main() prints a traceback and leaves a half-assembled <target>.restoring-<stamp> with nothing naming it. The target is untouched, so this is a usability failure rather than data loss.
  • Forwards compatibility: a backup made after backup: storage/ is copied live and never verified, and includes regenerable data #45 carries scanner_test_set, but an older checkout of this script has it in CARRIED_FROM_TARGET behind an if occupied gate and ignores the backup's copy. Nothing in a backup records which trees it contains, so no restore script can detect a tree it does not know about. A manifest.json ({"trees": [...], "excluded": [...], "format": N}) would let restore report any tree it has no policy for.

Shape (advisory, from the simplification pass)

  • RESTORE_SOURCES is built by a comprehension over BACKUP_TREES and then one key is reassigned seven lines later. Reading the dict literal alone gives the wrong answer for scanner_test_set, and if that name were ever dropped from BACKUP_TREES the override would silently re-add the key. Consider an ordered source tuple per entry ({"storage": ("backup",), "scanner_test_set": ("backup","target"), "rclone.conf": ("target",)}) taking the first source that exists, which removes the three policy constants and the override.
  • atomic_write_text is a pass-through to atomic_write_bytes with one caller.

Related: #45, #51, #58.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions