[fix] Make read-only mount bind actually read-only (#343)

* [fix] Mount bind readonly not really readonly
* Attempt to clarify and fix some issues with the readonly mount
* Fix some missing messages and exception handling
* Get rid of horrible bash command
* Use subproces.check_call to avoid security hazard
* Revert comment about hard link
* Add test that mount binds are readonly

locales/en.json

@@ -69,11 +69,12 @@
     "backup_archive_open_failed": "Unable to open the backup archive",
     "backup_archive_system_part_not_available": "System part '{part:s}' not available in this backup",
     "backup_archive_writing_error": "Unable to add files to backup into the compressed archive",
-    "backup_ask_for_copying_if_needed": "Your system don't support completely the quick method to organize files in the archive, do you want to organize its by copying {size:s}MB?",
+    "backup_ask_for_copying_if_needed": "Some files couldn't be prepared to be backuped using the method that avoid to temporarily waste space on the system. To perform the backup, {size:s}MB should be used temporarily. Do you agree?",
     "backup_borg_not_implemented": "Borg backup method is not yet implemented",
     "backup_cant_mount_uncompress_archive": "Unable to mount in readonly mode the uncompress archive directory",
     "backup_cleaning_failed": "Unable to clean-up the temporary backup directory",
     "backup_copying_to_organize_the_archive": "Copying {size:s}MB to organize the archive",
+    "backup_couldnt_bind": "Couldn't bind {src:s} to {dest:s}.",
     "backup_created": "Backup created",
     "backup_creating_archive": "Creating the backup archive...",
     "backup_creation_failed": "Backup creation failed",

src/yunohost/backup.py

@@ -40,6 +40,7 @@ from moulinette import msignals, m18n
 from moulinette.core import MoulinetteError
 from moulinette.utils import filesystem
 from moulinette.utils.log import getActionLogger
+from moulinette.utils.filesystem import read_file
 
 from yunohost.app import (
     app_info, _is_installed, _parse_app_instance_name
@@ -1543,23 +1544,37 @@ class BackupMethod(object):
             if not os.path.isdir(dest_dir):
                 filesystem.mkdir(dest_dir, parents=True)
 
-            # Try to bind files
+            # For directory, attempt to mount bind
             if os.path.isdir(src):
                 filesystem.mkdir(dest, parents=True, force=True)
-                ret = subprocess.call(["mount", "-r", "--rbind", src, dest])
+
-                if ret == 0:
+                try:
-                    continue
+                    subprocess.check_call(["mount", "--rbind", src, dest])
+                    subprocess.check_call(["mount", "-o", "remount,ro,bind", dest])
+                except Exception as e:
+                    logger.warning(m18n.n("backup_couldnt_bind", src=src, dest=dest))
+                    # To check if dest is mounted, use /proc/mounts that
+                    # escape spaces as \040
+                    raw_mounts = read_file("/proc/mounts").strip().split('\n')
+                    mounts = [ m.split()[1] for m in raw_mounts ]
+                    mounts = [ m.replace("\\040", " ") for m in mounts ]
+                    if dest in mounts:
+                        subprocess.check_call(["umount", "-R", dest])
                 else:
-                    logger.warning(m18n.n("bind_mouting_disable"))
+                    # Success, go to next file to organize
-                    subprocess.call(["mountpoint", "-q", dest,
-                                    "&&", "umount", "-R", dest])
-            elif os.path.isfile(src) or os.path.islink(src):
-                # Create a hardlink if src and dest are on the filesystem
-                if os.stat(src).st_dev == os.stat(dest_dir).st_dev:
-                    os.link(src, dest)
                     continue
 
-            # Add to the list to copy
+            # For files, create a hardlink
+            elif os.path.isfile(src) or os.path.islink(src):
+                # Can create a hard link only if files are on the same fs
+                # (i.e. we can't if it's on a different fs)
+                if os.stat(src).st_dev == os.stat(dest_dir).st_dev:
+                    os.link(src, dest)
+                    # Success, go to next file to organize
+                    continue
+
+            # If mountbind or hardlink couldnt be created,
+            # prepare a list of files that need to be copied
             paths_needed_to_be_copied.append(path)
 
         if len(paths_needed_to_be_copied) == 0:
@@ -1576,15 +1591,18 @@ class BackupMethod(object):
         if size > MB_ALLOWED_TO_ORGANIZE:
             try:
                 i = msignals.prompt(m18n.n('backup_ask_for_copying_if_needed',
-                                        answers='y/N', size=size))
+                                        answers='y/N', size=str(size)))
             except NotImplemented:
-                logger.error(m18n.n('backup_unable_to_organize_files'))
+                raise MoulinetteError(errno.EIO,
+                                     m18n.n('backup_unable_to_organize_files'))
             else:
                 if i != 'y' and i != 'Y':
-                    logger.error(m18n.n('backup_unable_to_organize_files'))
+                    raise MoulinetteError(errno.EIO,
+                                     m18n.n('backup_unable_to_organize_files'))
 
         # Copy unbinded path
-        logger.info(m18n.n('backup_copying_to_organize_the_archive', size=size))
+        logger.info(m18n.n('backup_copying_to_organize_the_archive',
+            size=str(size)))
         for path in paths_needed_to_be_copied:
             dest = os.path.join(self.work_dir, path['dest'])
             if os.path.isdir(path['source']):

src/yunohost/tests/test_backuprestore.py

@@ -643,7 +643,7 @@ def test_restore_archive_with_no_json(mocker):
     # Create a backup with no info.json associated
     os.system("touch /tmp/afile")
     os.system("tar -czvf /home/yunohost.backup/archives/badbackup.tar.gz /tmp/afile")
-
+    
     assert "badbackup" in backup_list()["archives"]
 
     mocker.spy(m18n, "n")
@@ -652,3 +652,26 @@ def test_restore_archive_with_no_json(mocker):
                        ignore_system=False, ignore_apps=False)
     m18n.n.assert_any_call('backup_invalid_archive')
 
+
+def test_backup_binds_are_readonly(monkeypatch):
+
+    def custom_mount_and_backup(self, backup_manager):
+        self.manager = backup_manager
+        self._organize_files()
+
+        confssh = os.path.join(self.work_dir, "conf/ssh")
+        output = subprocess.check_output("touch %s/test 2>&1 || true" % confssh,
+                                         shell=True)
+
+        assert "Read-only file system" in output
+
+        if self._recursive_umount(self.work_dir) > 0:
+            raise Exception("Backup cleaning failed !")
+
+        self.clean()
+
+    monkeypatch.setattr("yunohost.backup.BackupMethod.mount_and_backup",
+            custom_mount_and_backup)
+
+    # Create the backup
+    backup_create(ignore_system=False, ignore_apps=True)