use assert_secure for all commands that use the manifest

This already excludes the debug commands that we wouldn't want this on.

src/borg/archiver.py

@@ -36,7 +36,7 @@ from . import helpers
 from .algorithms.crc32 import crc32
 from .archive import Archive, ArchiveChecker, ArchiveRecreater, Statistics, is_special
 from .archive import BackupOSError, backup_io
-from .cache import Cache
+from .cache import Cache, assert_secure
 from .constants import *  # NOQA
 from .compress import CompressionSpec
 from .crypto.key import key_creator, tam_required_file, tam_required, RepoKey, PassphraseKey
@@ -86,7 +86,8 @@ def argument(args, str_or_bool):
     return str_or_bool
 
 
-def with_repository(fake=False, invert_fake=False, create=False, lock=True, exclusive=False, manifest=True, cache=False):
+def with_repository(fake=False, invert_fake=False, create=False, lock=True, exclusive=False, manifest=True, cache=False,
+                    secure=True):
     """
     Method decorator for subcommand-handling methods: do_XYZ(self, args, repository, …)
 
@@ -97,6 +98,7 @@ def with_repository(fake=False, invert_fake=False, create=False, lock=True, excl
     :param exclusive: (str or bool) lock repository exclusively (for writing)
     :param manifest: load manifest and key, pass them as keyword arguments
     :param cache: open cache, pass it as keyword argument (implies manifest)
+    :param secure: do assert_secure after loading manifest
     """
     def decorator(method):
         @functools.wraps(method)
@@ -117,6 +119,8 @@ def with_repository(fake=False, invert_fake=False, create=False, lock=True, excl
                     kwargs['manifest'], kwargs['key'] = Manifest.load(repository)
                     if 'compression' in args:
                         kwargs['key'].compressor = args.compression.compressor
+                    if secure:
+                        assert_secure(repository, kwargs['manifest'])
                 if cache:
                     with Cache(repository, kwargs['key'], kwargs['manifest'],
                                do_files=getattr(args, 'cache_files', False),

src/borg/cache.py

@@ -32,6 +32,25 @@ FileCacheEntry = namedtuple('FileCacheEntry', 'age inode size mtime chunk_ids')
 
 
 class SecurityManager:
+    """
+    Tracks repositories. Ensures that nothing bad happens (repository swaps,
+    replay attacks, unknown repositories etc.).
+
+    This is complicated by the Cache being initially used for this, while
+    only some commands actually use the Cache, which meant that other commands
+    did not perform these checks.
+
+    Further complications were created by the Cache being a cache, so it
+    could be legitimately deleted, which is annoying because Borg didn't
+    recognize repositories after that.
+
+    Therefore a second location, the security database (see get_security_dir),
+    was introduced which stores this information. However, this means that
+    the code has to deal with a cache existing but no security DB entry,
+    or inconsistencies between the security DB and the cache which have to
+    be reconciled, and also with no cache existing but a security DB entry.
+    """
+
     def __init__(self, repository):
         self.repository = repository
         self.dir = get_security_dir(repository.id_str)
@@ -66,19 +85,19 @@ class SecurityManager:
         with open(self.manifest_ts_file, 'w') as fd:
             fd.write(manifest.timestamp)
 
-    def assert_location_matches(self, cache_config):
+    def assert_location_matches(self, cache_config=None):
         # Warn user before sending data to a relocated repository
         try:
             with open(self.location_file) as fd:
                 previous_location = fd.read()
-            logger.debug('security: read previous_location %r', previous_location)
+            logger.debug('security: read previous location %r', previous_location)
         except FileNotFoundError:
-            logger.debug('security: previous_location file %s not found', self.location_file)
+            logger.debug('security: previous location file %s not found', self.location_file)
             previous_location = None
         except OSError as exc:
             logger.warning('Could not read previous location file: %s', exc)
             previous_location = None
-        if cache_config.previous_location and previous_location != cache_config.previous_location:
+        if cache_config and cache_config.previous_location and previous_location != cache_config.previous_location:
             # Reconcile cache and security dir; we take the cache location.
             previous_location = cache_config.previous_location
             logger.debug('security: using previous_location of cache: %r', previous_location)
@@ -95,9 +114,10 @@ class SecurityManager:
             logger.debug('security: updating location stored in cache and security dir')
             with open(self.location_file, 'w') as fd:
                 fd.write(repository_location)
-            cache_config.save()
+            if cache_config:
+                cache_config.save()
 
-    def assert_no_manifest_replay(self, manifest, key, cache_config):
+    def assert_no_manifest_replay(self, manifest, key, cache_config=None):
         try:
             with open(self.manifest_ts_file) as fd:
                 timestamp = fd.read()
@@ -108,7 +128,8 @@ class SecurityManager:
         except OSError as exc:
             logger.warning('Could not read previous location file: %s', exc)
             timestamp = ''
-        timestamp = max(timestamp, cache_config.timestamp or '')
+        if cache_config:
+            timestamp = max(timestamp, cache_config.timestamp or '')
         logger.debug('security: determined newest manifest timestamp as %s', timestamp)
         # If repository is older than the cache or security dir something fishy is going on
         if timestamp and timestamp > manifest.timestamp:
@@ -117,30 +138,39 @@ class SecurityManager:
             else:
                 raise Cache.RepositoryReplay()
 
-    def assert_key_type(self, key, cache):
+    def assert_key_type(self, key, cache_config=None):
         # Make sure an encrypted repository has not been swapped for an unencrypted repository
-        if cache.key_type is not None and cache.key_type != str(key.TYPE):
+        if cache_config and cache_config.key_type is not None and cache_config.key_type != str(key.TYPE):
             raise Cache.EncryptionMethodMismatch()
         if self.known() and not self.key_matches(key):
             raise Cache.EncryptionMethodMismatch()
 
     def assert_secure(self, manifest, key, *, cache_config=None, warn_if_unencrypted=True):
+        # warn_if_unencrypted=False is only used for initializing a new repository.
+        # Thus, avoiding asking about a repository that's currently initializing.
         self.assert_access_unknown(warn_if_unencrypted, manifest, key)
         if cache_config:
             self._assert_secure(manifest, key, cache_config)
         else:
-            with CacheConfig(self.repository):
-                self._assert_secure(manifest, key, cache_config)
+            cache_config = CacheConfig(self.repository)
+            if cache_config.exists():
+                with cache_config:
+                    self._assert_secure(manifest, key, cache_config)
+            else:
+                self._assert_secure(manifest, key)
+        logger.debug('security: repository checks ok, allowing access')
 
-    def _assert_secure(self, manifest, key, cache_config):
+    def _assert_secure(self, manifest, key, cache_config=None):
         self.assert_location_matches(cache_config)
         self.assert_key_type(key, cache_config)
         self.assert_no_manifest_replay(manifest, key, cache_config)
         if not self.known():
-            logger.debug('security: saving state for previously unknown repository')
+            logger.debug('security: remembering previously unknown repository')
             self.save(manifest, key)
 
     def assert_access_unknown(self, warn_if_unencrypted, manifest, key):
+        # warn_if_unencrypted=False is only used for initializing a new repository.
+        # Thus, avoiding asking about a repository that's currently initializing.
         if not key.logically_encrypted and not self.known():
             msg = ("Warning: Attempting to access a previously unknown unencrypted repository!\n" +
                    "Do you want to continue? [yN] ")
@@ -149,9 +179,9 @@ class SecurityManager:
                 retry=False, env_var_override='BORG_UNKNOWN_UNENCRYPTED_REPO_ACCESS_IS_OK')
             if allow_access:
                 if warn_if_unencrypted:
-                    logger.debug('security: saving state for unknown unencrypted repository (explicitly granted)')
+                    logger.debug('security: remembering unknown unencrypted repository (explicitly allowed)')
                 else:
-                    logger.debug('security: saving state for unknown unencrypted repository')
+                    logger.debug('security: initializing unencrypted repository')
                 self.save(manifest, key)
             else:
                 raise Cache.CacheInitAbortedError()
@@ -197,6 +227,9 @@ class CacheConfig:
     def __exit__(self, exc_type, exc_val, exc_tb):
         self.close()
 
+    def exists(self):
+        return os.path.exists(self.config_path)
+
     def create(self):
         assert not self.exists()
         config = configparser.ConfigParser(interpolation=None)

src/borg/testsuite/archiver.py

@@ -1709,7 +1709,7 @@ class ArchiverTestCase(ArchiverTestCaseBase):
         self.create_test_files()
         self.cmd('init', '--encryption=repokey', self.repository_location)
         log = self.cmd('--debug', 'create', self.repository_location + '::test', 'input')
-        assert 'security: read previous_location' in log
+        assert 'security: read previous location' in log
 
     def _get_sizes(self, compression, compressible, size=10000):
         if compressible: