Merge pull request #3349 from canonical/one-to-one-id-mappings

[mounts] only allow one to one id mappings

include/multipass/id_mappings.h

@@ -24,6 +24,8 @@
 #include <algorithm>
 #include <iterator>
 #include <set>
+#include <unordered_map>
+#include <unordered_set>
 #include <utility>
 #include <vector>
 
@@ -33,24 +35,46 @@ namespace multipass
 {
 using id_mappings = std::vector<std::pair<int, int>>;
 
-inline id_mappings unique_id_mappings(const id_mappings& xid_mappings)
+inline auto unique_id_mappings(id_mappings& xid_mappings)
 {
-    id_mappings ret;
-    std::set<std::pair<int, int>> id_set;
+    std::unordered_map<int, std::unordered_set<int>> dup_id_map;
+    std::unordered_map<int, std::unordered_set<int>> dup_rev_id_map;
 
-    auto is_mapping_repeated = [&id_set](const auto& m) {
-        if (id_set.insert(m).second)
-            return false;
+    for (auto it = xid_mappings.begin(); it != xid_mappings.end();)
+    {
+        bool duplicate =
+            dup_id_map.find(it->first) != dup_id_map.end() || dup_rev_id_map.find(it->second) != dup_rev_id_map.end();
 
-        mpl::log(mpl::Level::debug, "id_mappings", fmt::format("Dropping repeated mapping {}:{}", m.first, m.second));
+        dup_id_map[it->first].insert(it->second);
+        dup_rev_id_map[it->second].insert(it->first);
 
-        return true;
+        if (duplicate)
+        {
+            mpl::log(mpl::Level::debug,
+                     "id_mappings",
+                     fmt::format("Dropping repeated mapping {}:{}", it->first, it->second));
+            it = xid_mappings.erase(it);
+        }
+        else
+        {
+            ++it;
+        }
+    }
+
+    auto filter_non_repeating = [](auto& map) {
+        for (auto it = map.begin(); it != map.end();)
+        {
+            if (it->second.size() <= 1)
+                it = map.erase(it);
+            else
+                ++it;
+        }
     };
 
-    std::remove_copy_if(xid_mappings.cbegin(), xid_mappings.cend(), std::back_insert_iterator(ret),
-                        is_mapping_repeated);
+    filter_non_repeating(dup_id_map);
+    filter_non_repeating(dup_rev_id_map);
 
-    return ret;
+    return std::make_pair(dup_id_map, dup_rev_id_map);
 }
 } // namespace multipass
 

include/multipass/mount_handler.h

@@ -123,7 +123,7 @@ class MountHandler : private DisabledCopyMove
     const SSHKeyProvider* ssh_key_provider;
     const VMMount mount_spec = {};
     const std::string target;
-    const std::string& source = mount_spec.source_path;
+    const std::string& source = mount_spec.get_source_path();
     bool active;
     std::mutex active_mutex;
 };

include/multipass/snapshot.h

@@ -31,7 +31,7 @@ class QDateTime;
 namespace multipass
 {
 class MemorySize;
-struct VMMount;
+class VMMount;
 
 class Snapshot : private DisabledCopyMove
 {

include/multipass/virtual_machine.h

@@ -37,7 +37,7 @@
 namespace multipass
 {
 class MemorySize;
-struct VMMount;
+class VMMount;
 struct VMSpecs;
 class MountHandler;
 class Snapshot;

include/multipass/vm_mount.h

@@ -26,33 +26,63 @@
 
 namespace multipass
 {
-struct VMMount
+class VMMount
 {
+public:
     enum class MountType : int
     {
         Classic = 0,
         Native = 1
     };
 
     VMMount() = default;
-    VMMount(const QJsonObject& json);
+    explicit VMMount(const QJsonObject& json);
     VMMount(const std::string& sourcePath, id_mappings gidMappings, id_mappings uidMappings, MountType mountType);
 
     QJsonObject serialize() const;
 
+    const std::string& get_source_path() const noexcept;
+    const id_mappings& get_gid_mappings() const noexcept;
+    const id_mappings& get_uid_mappings() const noexcept;
+    MountType get_mount_type() const noexcept;
+
+    friend bool operator==(const VMMount& a, const VMMount& b) noexcept;
+    friend bool operator!=(const VMMount& a, const VMMount& b) noexcept;
+
+private:
     std::string source_path;
     id_mappings gid_mappings;
     id_mappings uid_mappings;
     MountType mount_type;
 };
 
-inline bool operator==(const VMMount& a, const VMMount& b)
+inline const std::string& VMMount::get_source_path() const noexcept
+{
+    return source_path;
+}
+
+inline const multipass::id_mappings& VMMount::get_gid_mappings() const noexcept
+{
+    return gid_mappings;
+}
+
+inline const multipass::id_mappings& VMMount::get_uid_mappings() const noexcept
+{
+    return uid_mappings;
+}
+
+inline VMMount::MountType VMMount::get_mount_type() const noexcept
+{
+    return mount_type;
+}
+
+inline bool operator==(const VMMount& a, const VMMount& b) noexcept
 {
     return std::tie(a.source_path, a.gid_mappings, a.uid_mappings, a.mount_type) ==
            std::tie(b.source_path, b.gid_mappings, b.uid_mappings, b.mount_type);
 }
 
-inline bool operator!=(const VMMount& a, const VMMount& b) // TODO drop in C++20
+inline bool operator!=(const VMMount& a, const VMMount& b) noexcept // TODO drop in C++20
 {
     return !(a == b);
 }

src/client/cli/cmd/mount.cpp

@@ -118,16 +118,14 @@ mp::ParseCode cmd::Mount::parse_args(mp::ArgParser* parser)
                                   "<target> [<target> ...]");
 
     QCommandLineOption gid_mappings({"g", "gid-map"},
-                                    "A mapping of group IDs for use in the mount. "
-                                    "File and folder ownership will be mapped from "
-                                    "<host> to <instance> inside the instance. Can be "
-                                    "used multiple times.",
+                                    "A mapping of group IDs for use in the mount. File and folder ownership will be "
+                                    "mapped from <host> to <instance> inside the instance. Can be used multiple times. "
+                                    "Mappings can only be specified as a one-to-one relationship.",
                                     "host>:<instance");
     QCommandLineOption uid_mappings({"u", "uid-map"},
-                                    "A mapping of user IDs for use in the mount. "
-                                    "File and folder ownership will be mapped from "
-                                    "<host> to <instance> inside the instance. Can be "
-                                    "used multiple times.",
+                                    "A mapping of user IDs for use in the mount. File and folder ownership will be "
+                                    "mapped from <host> to <instance> inside the instance. Can be used multiple times. "
+                                    "Mappings can only be specified as a one-to-one relationship.",
                                     "host>:<instance");
     QCommandLineOption mount_type_option({"t", "type"},
                                          "Specify the type of mount to use.\n"

src/daemon/daemon.cpp

@@ -1213,20 +1213,20 @@ void populate_mount_info(const std::unordered_map<std::string, mp::VMMount>& mou
     {
         for (const auto& mount : mounts)
         {
-            if (mount.second.source_path.size() > mount_info->longest_path_len())
-                mount_info->set_longest_path_len(mount.second.source_path.size());
+            if (mount.second.get_source_path().size() > mount_info->longest_path_len())
+                mount_info->set_longest_path_len(mount.second.get_source_path().size());
 
             auto entry = mount_info->add_mount_paths();
-            entry->set_source_path(mount.second.source_path);
+            entry->set_source_path(mount.second.get_source_path());
             entry->set_target_path(mount.first);
 
-            for (const auto& uid_mapping : mount.second.uid_mappings)
+            for (const auto& uid_mapping : mount.second.get_uid_mappings())
             {
                 auto uid_pair = entry->mutable_mount_maps()->add_uid_mappings();
                 uid_pair->set_host_id(uid_mapping.first);
                 uid_pair->set_instance_id(uid_mapping.second);
             }
-            for (const auto& gid_mapping : mount.second.gid_mappings)
+            for (const auto& gid_mapping : mount.second.get_gid_mappings())
             {
                 auto gid_pair = entry->mutable_mount_maps()->add_gid_mappings();
                 gid_pair->set_host_id(gid_mapping.first);
@@ -3215,7 +3215,7 @@ bool mp::Daemon::create_missing_mounts(std::unordered_map<std::string, VMMount>&
                 mpl::log(mpl::Level::warning,
                          category,
                          fmt::format(R"(Removing mount "{}" => "{}" from '{}': {})",
-                                     mount_spec.source_path,
+                                     mount_spec.get_source_path(),
                                      target,
                                      vm->vm_name,
                                      e.what()));
@@ -3233,7 +3233,7 @@ bool mp::Daemon::create_missing_mounts(std::unordered_map<std::string, VMMount>&
 
 mp::MountHandler::UPtr mp::Daemon::make_mount(VirtualMachine* vm, const std::string& target, const VMMount& mount)
 {
-    return mount.mount_type == VMMount::MountType::Classic
+    return mount.get_mount_type() == VMMount::MountType::Classic
                ? std::make_unique<SSHFSMountHandler>(vm, config->ssh_key_provider.get(), target, mount)
                : vm->make_native_mount_handler(target, mount);
 }

src/platform/backends/lxd/lxd_virtual_machine.cpp

@@ -159,8 +159,8 @@ QJsonObject generate_devices_config(const multipass::VirtualMachineDescription&
 
 bool uses_default_id_mappings(const multipass::VMMount& mount)
 {
-    const auto& gid_mappings = mount.gid_mappings;
-    const auto& uid_mappings = mount.uid_mappings;
+    const auto& gid_mappings = mount.get_gid_mappings();
+    const auto& uid_mappings = mount.get_uid_mappings();
 
     // -1 is the default value for gid and uid
     return gid_mappings.size() == 1 && gid_mappings.front().second == -1 && uid_mappings.size() == 1 &&

src/platform/backends/qemu/qemu_mount_handler.cpp

@@ -56,16 +56,16 @@ QemuMountHandler::QemuMountHandler(QemuVirtualMachine* vm,
     }
 
     // Need to ensure no more than one uid/gid map is passed in here.
-    if (this->mount_spec.uid_mappings.size() > 1 || this->mount_spec.gid_mappings.size() > 1)
+    if (this->mount_spec.get_uid_mappings().size() > 1 || this->mount_spec.get_gid_mappings().size() > 1)
         throw std::runtime_error("Only one mapping per native mount allowed.");
 
     mpl::log(mpl::Level::info, category,
              fmt::format("initializing native mount {} => {} in '{}'", source, target, vm->vm_name));
 
-    const auto uid_map =
-        this->mount_spec.uid_mappings.empty() ? std::make_pair(1000, 1000) : this->mount_spec.uid_mappings[0];
-    const auto gid_map =
-        this->mount_spec.gid_mappings.empty() ? std::make_pair(1000, 1000) : this->mount_spec.gid_mappings[0];
+    const auto uid_map = this->mount_spec.get_uid_mappings().empty() ? std::make_pair(1000, 1000)
+                                                                     : this->mount_spec.get_uid_mappings()[0];
+    const auto gid_map = this->mount_spec.get_gid_mappings().empty() ? std::make_pair(1000, 1000)
+                                                                     : this->mount_spec.get_gid_mappings()[0];
     const auto uid_arg = QString("uid_map=%1:%2,").arg(uid_map.first).arg(uid_map.second == -1 ? 1000 : uid_map.second);
     const auto gid_arg = QString{"gid_map=%1:%2,"}.arg(gid_map.first).arg(gid_map.second == -1 ? 1000 : gid_map.second);
     vm_mount_args[tag] = {

src/sshfs_mount/sshfs_mount_handler.cpp

@@ -127,12 +127,13 @@ SSHFSMountHandler::SSHFSMountHandler(VirtualMachine* vm,
              ssh_key_provider->private_key_as_base64(),
              source,
              target,
-             this->mount_spec.gid_mappings,
-             this->mount_spec.uid_mappings}
+             this->mount_spec.get_gid_mappings(),
+             this->mount_spec.get_uid_mappings()}
 {
-    mpl::log(mpl::Level::info,
-             category,
-             fmt::format("initializing mount {} => {} in '{}'", this->mount_spec.source_path, target, vm->vm_name));
+    mpl::log(
+        mpl::Level::info,
+        category,
+        fmt::format("initializing mount {} => {} in '{}'", this->mount_spec.get_source_path(), target, vm->vm_name));
 }
 
 bool SSHFSMountHandler::is_active()

src/utils/vm_mount.cpp

@@ -41,12 +41,40 @@ mp::VMMount parse_json(const QJsonObject& json)
             {gid_entry.toObject()["host_gid"].toInt(), gid_entry.toObject()["instance_gid"].toInt()});
     }
 
-    uid_mappings = mp::unique_id_mappings(uid_mappings);
-    gid_mappings = mp::unique_id_mappings(gid_mappings);
+    mp::unique_id_mappings(uid_mappings);
+    mp::unique_id_mappings(gid_mappings);
     auto mount_type = mp::VMMount::MountType(json["mount_type"].toInt());
 
     return mp::VMMount{std::move(source_path), std::move(gid_mappings), std::move(uid_mappings), mount_type};
 }
+
+auto print_mappings(const std::unordered_map<int, std::unordered_set<int>>& dup_id_map,
+                    const std::unordered_map<int, std::unordered_set<int>>& dup_rev_id_map)
+{
+    std::string retval;
+
+    for (const auto& pair : dup_id_map)
+    {
+        retval += fmt::format("{}: [", pair.first);
+        for (const auto& mapping : pair.second)
+            retval += fmt::format("{}:{}, ", pair.first, mapping);
+
+        retval = retval.substr(0, retval.size() - 2);
+        retval += "]; ";
+    }
+
+    for (const auto& pair : dup_rev_id_map)
+    {
+        retval += fmt::format("{}: [", pair.first);
+        for (const auto& mapping : pair.second)
+            retval += fmt::format("{}:{}, ", mapping, pair.first);
+
+        retval = retval.substr(0, retval.size() - 2);
+        retval += "]; ";
+    }
+
+    return retval.substr(0, retval.size() - 2);
+}
 } // namespace
 
 mp::VMMount::VMMount(const std::string& sourcePath,
@@ -58,6 +86,25 @@ mp::VMMount::VMMount(const std::string& sourcePath,
       uid_mappings(std::move(uidMappings)),
       mount_type(mountType)
 {
+    fmt::memory_buffer errors;
+
+    if (const auto& [dup_uid_map, dup_rev_uid_map] = mp::unique_id_mappings(uid_mappings);
+        !dup_uid_map.empty() || !dup_rev_uid_map.empty())
+    {
+        fmt::format_to(std::back_inserter(errors),
+                       fmt::format("\nuids: {}", print_mappings(dup_uid_map, dup_rev_uid_map)));
+    }
+
+    if (const auto& [dup_gid_map, dup_rev_gid_map] = mp::unique_id_mappings(gid_mappings);
+        !dup_gid_map.empty() || !dup_rev_gid_map.empty())
+    {
+        fmt::format_to(std::back_inserter(errors),
+                       fmt::format("\ngids: {}", print_mappings(dup_gid_map, dup_rev_gid_map)));
+    }
+
+    if (errors.size())
+        throw std::runtime_error(
+            fmt::format("Mount cannot apply mapping with duplicate ids:{}", fmt::to_string(errors)));
 }
 
 mp::VMMount::VMMount(const QJsonObject& json) : VMMount{parse_json(json)} // delegate on copy ctor

tests/daemon_test_fixture.cpp

@@ -460,7 +460,7 @@ std::string mpt::DaemonTestFixture::fake_json_contents(const std::string& defaul
                                                             "                \"gid_mappings\": ["));
 
         QStringList gid_array_elements;
-        for (const auto& gid_pair : mount.gid_mappings)
+        for (const auto& gid_pair : mount.get_gid_mappings())
         {
             gid_array_elements += QString::fromStdString(fmt::format("\n                    {{\n"
                                                                      "                        \"host_gid\": {},\n"
@@ -473,13 +473,14 @@ std::string mpt::DaemonTestFixture::fake_json_contents(const std::string& defaul
         mount_element += QString::fromStdString(fmt::format("\n                ],\n"
                                                             "                \"source_path\": \"{}\",\n"
                                                             "                \"target_path\": \"{}\",\n",
-                                                            mount.source_path, mountpoint));
+                                                            mount.get_source_path(),
+                                                            mountpoint));
         mount_element += QString::fromStdString(fmt::format("                \"mount_type\": {},\n"
                                                             "                \"uid_mappings\": [",
-                                                            mount.mount_type));
+                                                            mount.get_mount_type()));
 
         QStringList uid_array_elements;
-        for (const auto& uid_pair : mount.uid_mappings)
+        for (const auto& uid_pair : mount.get_uid_mappings())
         {
             uid_array_elements += QString::fromStdString(fmt::format("\n                    {{\n"
                                                                      "                        \"host_uid\": {},\n"