fix: harden docsgpt backup and docsgpt restore

From review of #2799:

- The archive is created 0600 rather than at the process umask: it holds the
  install's data, and --with-settings puts .env and its secrets in it.
- restore validates everything the manifest declares before the stack is
  stopped, so a damaged archive fails while DocsGPT is still running rather
  than after `compose down` has taken it away.
- Only the volumes a backup is made of are restored. A hand-made manifest can
  no longer point import_volume at postgres_data, whose contents it empties.
- psql runs with ON_ERROR_STOP=on, so a restore that fails halfway cannot
  start DocsGPT again and call it a success.
- import_volume unpacks into the container's own filesystem first and clears
  the live volume only once the tar has come out whole, so a corrupt one
  leaves the volume as it was.
- The backend and the worker stop while the archive is made and start again
  even if the dump fails, so the dump and the volume tars describe the same
  moment instead of drifting apart as ingestion writes.

docs/content/Deploying/Docker-Deploying.mdx

@@ -89,6 +89,12 @@ docsgpt backup                      # into <stack>/backups
 docsgpt backup --out /mnt/backups   # somewhere else, e.g. a mounted disk
 ```
 
+While the archive is made, the backend and the worker stop and start again, so
+the database dump and the files in the volumes describe the same moment; Postgres
+itself keeps running. On a small install that pause is seconds, but count on it
+if you run `docsgpt backup` from cron. The archive is written readable only by
+the user who took it.
+
 The archive does **not** include `.env`, because that file holds the install's
 secrets. `docsgpt backup --with-settings` puts it in, for when the archive
 itself is stored somewhere private. Keep `.env` safe separately otherwise: the

docs/content/changelog.mdx

@@ -18,7 +18,8 @@ request, and [Upgrading](/upgrading) covers the steps an existing deployment has
 `docsgpt backup` writes a dump of the database and a tar of each data volume into one archive, and
 `docsgpt restore <archive>` puts them back. The settings file is left out unless
 `--with-settings` asks for it, since it holds the install's secrets, and a backup from a newer
-DocsGPT is refused unless you pass `--force`. See
+DocsGPT is refused unless you pass `--force`. The archive is written readable only by its owner,
+and the backend and the worker pause while it is made so the dump and the volume tars match. See
 [Backups](/Deploying/Docker-Deploying#backups).
 
 ## 0.21.0

docsgpt/deploy/backup.py

@@ -13,6 +13,7 @@ from __future__ import annotations
 
 import io
 import json
+import os
 import tarfile
 import time
 from collections.abc import Mapping
@@ -50,7 +51,10 @@ def write_archive(
 ) -> None:
     """Write the backup archive; the manifest is added last so a truncated file has none."""
     path.parent.mkdir(parents=True, exist_ok=True)
-    with tarfile.open(path, "w:gz") as archive:
+    # 0600 from the start: the dump is the install's data, and --with-settings adds its secrets.
+    descriptor = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600)
+    os.fchmod(descriptor, 0o600)
+    with os.fdopen(descriptor, "wb") as handle, tarfile.open(fileobj=handle, mode="w:gz") as archive:
         archive.add(dump, arcname=DUMP)
         for name, tar_path in sorted(volume_tars.items()):
             archive.add(tar_path, arcname=volume_member(name))
@@ -81,6 +85,34 @@ def read_manifest(path: Path) -> dict:
     return manifest
 
 
+def validate(path: Path, manifest: Mapping[str, object]) -> list[str]:
+    """The volumes to restore, once the archive is known to hold everything it declares.
+
+    Checked before the stack is stopped: a damaged or hand-made archive must fail while DocsGPT is
+    still running, not after ``docker compose down``. Only the volumes a backup is made of are
+    accepted, so a manifest cannot name ``postgres_data`` and have it emptied on the way in.
+    """
+    volumes = manifest.get("volumes")
+    if not isinstance(volumes, list) or not all(isinstance(name, str) for name in volumes):
+        raise DeployError(f"{path} is not a DocsGPT backup: its manifest does not list the volumes it holds")
+    unsupported = sorted(set(volumes) - set(DATA_VOLUMES))
+    if unsupported:
+        raise DeployError(
+            f"{path} names volumes that are not part of a backup: {', '.join(unsupported)}. "
+            f"A DocsGPT backup holds {', '.join(DATA_VOLUMES)}."
+        )
+    required = [DUMP, *(volume_member(name) for name in volumes)]
+    try:
+        with tarfile.open(path, "r:gz") as archive:
+            present = {member.name for member in archive.getmembers() if member.isfile()}
+    except (tarfile.TarError, OSError) as exc:
+        raise DeployError(f"{path} is not a DocsGPT backup: {exc}") from exc
+    missing = [name for name in required if name not in present]
+    if missing:
+        raise DeployError(f"{path} is missing {', '.join(missing)}, so there is nothing to restore from")
+    return list(volumes)
+
+
 def extract(path: Path, destination: Path) -> None:
     """Unpack the archive into ``destination`` (data filter: no paths outside it, no devices)."""
     with tarfile.open(path, "r:gz") as archive:

docsgpt/deploy/commands.py

@@ -371,6 +371,26 @@ def backup(args, context: Optional[Context] = None) -> int:
     target = out_dir / backup_format.archive_name(taken_at)
     image = _stack_image(env)
 
+    print("Pausing the backend and the worker so the database and the files match ...")
+    context.docker.compose(directory, "stop", "backend", "worker")
+    try:
+        _write_backup(args, context, directory, env, target, image, taken_at)
+    finally:
+        print("Starting the backend and the worker again ...")
+        context.docker.compose(directory, "up", "-d", "backend", "worker")
+
+    size = target.stat().st_size / 1_000_000
+    print(f"\nBackup written to {target} ({size:.1f} MB)")
+    if args.with_settings:
+        print("It contains .env, so it holds this install's secrets: keep it somewhere private.")
+    else:
+        print("Settings are not in it; `docsgpt backup --with-settings` includes .env, secrets and all.")
+    print(f"Restore it with: docsgpt restore {target}")
+    return 0
+
+
+def _write_backup(args, context: Context, directory: Path, env, target: Path, image: str, taken_at) -> None:
+    """Dump the database and the data volumes into ``target``, with the writers stopped."""
     with tempfile.TemporaryDirectory() as workspace:
         work = Path(workspace)
         dump = work / backup_format.DUMP
@@ -398,15 +418,6 @@ def backup(args, context: Optional[Context] = None) -> int:
         settings = directory / ".env" if args.with_settings else None
         backup_format.write_archive(target, dump=dump, volume_tars=volume_tars, manifest=manifest, settings=settings)
 
-    size = target.stat().st_size / 1_000_000
-    print(f"\nBackup written to {target} ({size:.1f} MB)")
-    if args.with_settings:
-        print("It contains .env, so it holds this install's secrets: keep it somewhere private.")
-    else:
-        print("Settings are not in it; `docsgpt backup --with-settings` includes .env, secrets and all.")
-    print(f"Restore it with: docsgpt restore {target}")
-    return 0
-
 
 def restore(args, context: Optional[Context] = None) -> int:
     """Put a backup's database and data volumes back over this install."""
@@ -414,6 +425,8 @@ def restore(args, context: Optional[Context] = None) -> int:
     archive = Path(args.archive).expanduser()
     manifest = backup_format.read_manifest(archive)
     backup_format.check_version(manifest, context.version, args.force)
+    # Everything the archive declares is checked here, while DocsGPT is still up.
+    volumes = backup_format.validate(archive, manifest)
 
     directory = stack.stack_dir(args.dir)
     env = _installed(directory)
@@ -436,7 +449,7 @@ def restore(args, context: Optional[Context] = None) -> int:
     with tempfile.TemporaryDirectory() as workspace:
         work = Path(workspace)
         backup_format.extract(archive, work)
-        for name in manifest.get("volumes", []):
+        for name in volumes:
             tar_path = work / backup_format.volume_member(name)
             if not tar_path.is_file():
                 raise DeployError(f"{archive} is missing the {name} volume it says it contains")
@@ -452,7 +465,7 @@ def restore(args, context: Optional[Context] = None) -> int:
         with dump.open("r", encoding="utf-8") as handle:
             context.docker.compose(
                 directory, "exec", "-T", "postgres",
-                "psql", "--quiet", "-U", "docsgpt", "-d", "docsgpt",
+                "psql", "--quiet", "--set", "ON_ERROR_STOP=on", "-U", "docsgpt", "-d", "docsgpt",
                 stdin=handle,
             )
 

docsgpt/deploy/docker.py

@@ -119,8 +119,11 @@ class Docker:
         """Replace ``volume``'s contents with the tar at ``source``; the volume is created when missing."""
         with source.open("rb") as handle:
             self._run(
-                ["docker", "run", "--rm", "-i", "-v", f"{volume}:/data", image,
-                 "sh", "-c", "find /data -mindepth 1 -delete && tar xf - -C /data"],
+                ["docker", "run", "--rm", "-i", "-v", f"{volume}:/data", image, "sh", "-c",
+                 # Unpack into the container's own filesystem first: a truncated or corrupt tar must
+                 # fail before the live volume is touched, not halfway through emptying it.
+                 "set -e; rm -rf /stage; mkdir /stage; tar xf - -C /stage; "
+                 "find /data -mindepth 1 -delete; tar cf - -C /stage . | tar xf - -C /data"],
                 stdin=handle,
             )
 

tests/deploy/test_backup.py

@@ -2,6 +2,7 @@
 
 import io
 import json
+import stat
 import tarfile
 
 import pytest
@@ -26,6 +27,22 @@ def _manifest(path):
         return json.loads(archive.extractfile(backup_module.MANIFEST).read())
 
 
+def _rewrite(archive, dest, *, volumes=None, drop=()):
+    """The same archive with other volumes in its manifest, or with members left out."""
+    with tarfile.open(archive, "r:gz") as source, tarfile.open(dest, "w:gz") as out:
+        for member in source.getmembers():
+            if member.name in drop:
+                continue
+            body = source.extractfile(member).read() if member.isfile() else None
+            if member.name == backup_module.MANIFEST and volumes is not None:
+                manifest = json.loads(body.decode())
+                manifest["volumes"] = volumes
+                body = json.dumps(manifest).encode()
+                member.size = len(body)
+            out.addfile(member, io.BytesIO(body) if body is not None else None)
+    return dest
+
+
 def _copy_with_version(archive, version, dest):
     """The same archive, with another DocsGPT version written into its manifest."""
     with tarfile.open(archive, "r:gz") as source, tarfile.open(dest, "w:gz") as out:
@@ -80,6 +97,40 @@ class TestBackup:
         assert dumps, docker.calls
         assert dumps[0][:3] == ["exec", "-T", "postgres"]
 
+    def test_the_archive_is_private_to_whoever_took_it(self, tmp_path):
+        """It can hold .env, and the dump is the install's data either way."""
+        _installed(tmp_path)
+        out = tmp_path / "backups"
+        argv = ["backup", "--dir", str(tmp_path), "--out", str(out), "--with-settings"]
+        assert _run(argv, _context()) == 0
+        archive = next(out.glob("*.tar.gz"))
+        assert stat.S_IMODE(archive.stat().st_mode) == 0o600
+
+    def test_the_writers_are_stopped_while_the_archive_is_made(self, tmp_path):
+        """Ingestion writes files and rows; a dump taken alongside live writes would not match them."""
+        _installed(tmp_path)
+        docker = FakeDocker()
+        assert _run(["backup", "--dir", str(tmp_path), "--out", str(tmp_path / "b")], _context(docker)) == 0
+        joined = [" ".join(args) for _, args in docker.calls]
+        stopped = next(index for index, call in enumerate(joined) if call.startswith("stop backend worker"))
+        dumped = next(index for index, call in enumerate(joined) if "pg_dump" in call)
+        started = next(index for index, call in enumerate(joined) if call.startswith("up -d backend worker"))
+        assert stopped < dumped < started, joined
+
+    def test_the_writers_come_back_even_when_the_dump_fails(self, tmp_path):
+        _installed(tmp_path)
+
+        class FailingDump(FakeDocker):
+            def compose(self, directory, *args, **kwargs):
+                if "pg_dump" in args:
+                    raise DeployError("pg_dump exploded")
+                return super().compose(directory, *args, **kwargs)
+
+        docker = FailingDump()
+        with pytest.raises(DeployError, match="pg_dump exploded"):
+            _run(["backup", "--dir", str(tmp_path), "--out", str(tmp_path / "b")], _context(docker))
+        assert any(" ".join(args).startswith("up -d backend worker") for _, args in docker.calls), docker.calls
+
     def test_without_an_install(self, tmp_path, capsys):
         assert _run(["backup", "--dir", str(tmp_path)], _context()) == 1
         assert "docsgpt up" in capsys.readouterr().err
@@ -122,6 +173,31 @@ class TestRestore:
         with pytest.raises(DeployError, match="does not exist"):
             _run(["restore", str(tmp_path / "nope.tar.gz"), "--dir", str(tmp_path), "--yes"], _context())
 
+    def test_a_manifest_naming_another_volume_is_refused(self, tmp_path):
+        """import_volume empties what it is given, so a hand-made manifest must not name postgres_data."""
+        crafted = _rewrite(self._backup(tmp_path), tmp_path / "crafted.tar.gz", volumes=["postgres_data"])
+        docker = FakeDocker(volumes={"docsgpt_postgres_data"})
+        with pytest.raises(DeployError, match="not part of a backup"):
+            _run(["restore", str(crafted), "--dir", str(tmp_path), "--yes"], _context(docker))
+        assert docker.calls == [], "nothing was stopped"
+        assert docker.volume_ops == [], "and nothing was touched"
+
+    def test_a_damaged_archive_fails_before_the_stack_is_stopped(self, tmp_path):
+        """Discovering a missing member after `compose down` would leave DocsGPT down for nothing."""
+        without_dump = _rewrite(self._backup(tmp_path), tmp_path / "nodump.tar.gz", drop=(backup_module.DUMP,))
+        docker = FakeDocker(volumes={"docsgpt_postgres_data"})
+        with pytest.raises(DeployError, match="nothing to restore"):
+            _run(["restore", str(without_dump), "--dir", str(tmp_path), "--yes"], _context(docker))
+        assert docker.calls == [], "DocsGPT is still running"
+
+    def test_psql_stops_at_the_first_failing_statement(self, tmp_path):
+        """Without it psql runs on after an error and a half-restored database looks like success."""
+        archive = self._backup(tmp_path)
+        docker = FakeDocker(volumes={"docsgpt_postgres_data"})
+        assert _run(["restore", str(archive), "--dir", str(tmp_path), "--yes"], _context(docker)) == 0
+        psql = next(args for _, args in docker.calls if "psql" in args)
+        assert "ON_ERROR_STOP=on" in psql
+
     def test_a_newer_backup_is_refused_without_force(self, tmp_path):
         """Restoring a 0.22 backup into 0.21 would hand an older schema newer data."""
         newer = _copy_with_version(self._backup(tmp_path), "0.22.0", tmp_path / "newer.tar.gz")

tests/deploy/test_docker.py

@@ -129,7 +129,10 @@ class TestVolumes:
         assert "docsgpt_inputs:/data" in args
         assert args[-2] == "-c"
         assert "tar xf - -C /data" in args[-1]
-        assert "find /data -mindepth 1 -delete" in args[-1], "old contents go first"
+        command = args[-1]
+        assert command.index("tar xf - -C /stage") < command.index("find /data -mindepth 1 -delete"), (
+            "the incoming tar is unpacked outside the volume first, so a corrupt one leaves it alone"
+        )
         assert runner.streams[0][1] is not None, "the tar is fed in on stdin"