From d46ce1a9a5a0884d05dfa736974abe2e4f83c031 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Thu, 3 Oct 2024 17:34:58 -0400 Subject: [PATCH 01/12] [daemon] Change permissions for instances to root only --- include/multipass/platform.h | 1 + src/daemon/default_vm_image_vault.cpp | 7 +++++-- src/iso/cloud_init_iso.cpp | 7 +++++++ src/platform/platform_unix.cpp | 5 +++++ src/utils/vm_image_vault_utils.cpp | 8 ++++++++ tests/mock_platform.h | 5 +++++ tests/test_base_virtual_machine_factory.cpp | 5 +++++ tests/test_cloud_init_iso.cpp | 7 +++++++ 8 files changed, 43 insertions(+), 2 deletions(-) diff --git a/include/multipass/platform.h b/include/multipass/platform.h index c5e42e4191..036ad04c2b 100644 --- a/include/multipass/platform.h +++ b/include/multipass/platform.h @@ -59,6 +59,7 @@ class Platform : public Singleton virtual bool is_backend_supported(const QString& backend) const; // temporary (?) virtual int chown(const char* path, unsigned int uid, unsigned int gid) const; virtual bool set_permissions(const Path& path, const Perms permissions) const; + virtual bool set_root_as_owner(const multipass::Path& path) const; virtual bool link(const char* target, const char* link) const; virtual bool symlink(const char* target, const char* link, bool is_dir) const; virtual int utime(const char* path, int atime, int mtime) const; diff --git a/src/daemon/default_vm_image_vault.cpp b/src/daemon/default_vm_image_vault.cpp index 42f1d00915..d2f66c9790 100644 --- a/src/daemon/default_vm_image_vault.cpp +++ b/src/daemon/default_vm_image_vault.cpp @@ -669,7 +669,9 @@ QString mp::DefaultVMImageVault::extract_image_from(const VMImage& source_image, const ProgressMonitor& monitor, const mp::Path& dest_dir) { - MP_UTILS.make_dir(dest_dir); + MP_UTILS.make_dir(dest_dir, QFile::ReadOwner | QFile::WriteOwner | QFile::ExeOwner); + MP_PLATFORM.set_root_as_owner(dest_dir); + QFileInfo file_info{source_image.image_path}; const auto image_name = file_info.fileName().remove(".xz"); const auto image_path = QDir(dest_dir).filePath(image_name); @@ -679,7 +681,8 @@ QString mp::DefaultVMImageVault::extract_image_from(const VMImage& source_image, mp::VMImage mp::DefaultVMImageVault::image_instance_from(const VMImage& prepared_image, const mp::Path& dest_dir) { - MP_UTILS.make_dir(dest_dir); + MP_UTILS.make_dir(dest_dir, QFile::ReadOwner | QFile::WriteOwner | QFile::ExeOwner); + MP_PLATFORM.set_root_as_owner(dest_dir); return {mp::vault::copy(prepared_image.image_path, dest_dir), prepared_image.id, diff --git a/src/iso/cloud_init_iso.cpp b/src/iso/cloud_init_iso.cpp index 69eb7aaf62..11e51b8839 100644 --- a/src/iso/cloud_init_iso.cpp +++ b/src/iso/cloud_init_iso.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include @@ -536,6 +537,12 @@ void mp::CloudInitIso::write_to(const Path& path) throw std::runtime_error{fmt::format( "Failed to open file for writing during cloud-init generation: {}; path: {}", f.errorString(), path)}; + if (!MP_PLATFORM.set_permissions(path, QFile::ReadOwner | QFile::WriteOwner)) + throw std::runtime_error{fmt::format("Failed to set permissions during cloud-init generation: path: {}", path)}; + + if (!MP_PLATFORM.set_root_as_owner(path)) + throw std::runtime_error{fmt::format("Failed to set owner during cloud-init generation: path: {}", path)}; + const uint32_t num_reserved_bytes = 32768u; const uint32_t num_reserved_blocks = num_blocks(num_reserved_bytes); f.seek(num_reserved_bytes); diff --git a/src/platform/platform_unix.cpp b/src/platform/platform_unix.cpp index c0c87e74ba..e265dc94e0 100644 --- a/src/platform/platform_unix.cpp +++ b/src/platform/platform_unix.cpp @@ -63,6 +63,11 @@ bool mp::platform::Platform::set_permissions(const Path& path, const Perms permi return QFile::setPermissions(path, permissions); } +bool mp::platform::Platform::set_root_as_owner(const mp::Path& path) const +{ + return this->chown(path.toStdString().c_str(), 0, 0) == 0; +} + bool mp::platform::Platform::symlink(const char* target, const char* link, bool is_dir) const { return ::symlink(target, link) == 0; diff --git a/src/utils/vm_image_vault_utils.cpp b/src/utils/vm_image_vault_utils.cpp index ce5f09deec..860394d96d 100644 --- a/src/utils/vm_image_vault_utils.cpp +++ b/src/utils/vm_image_vault_utils.cpp @@ -16,6 +16,7 @@ */ #include +#include #include #include #include @@ -45,6 +46,10 @@ QString mp::vault::copy(const QString& file_name, const QDir& output_dir) const auto source_name = info.fileName(); auto new_path = output_dir.filePath(source_name); QFile::copy(file_name, new_path); + + MP_PLATFORM.set_permissions(new_path, QFile::ReadOwner | QFile::WriteOwner | QFile::ExeOwner); + MP_PLATFORM.set_root_as_owner(new_path); + return new_path; } @@ -90,6 +95,9 @@ QString mp::vault::extract_image(const mp::Path& image_path, const mp::ProgressM xz_decoder.decode_to(new_image_path, monitor); + MP_PLATFORM.set_permissions(new_image_path, QFile::ReadOwner | QFile::WriteOwner | QFile::ExeOwner); + MP_PLATFORM.set_root_as_owner(new_image_path); + mp::vault::delete_file(image_path); return new_image_path; diff --git a/tests/mock_platform.h b/tests/mock_platform.h index 30baa9fe92..54594dcaf1 100644 --- a/tests/mock_platform.h +++ b/tests/mock_platform.h @@ -42,6 +42,11 @@ class MockPlatform : public platform::Platform MOCK_METHOD(bool, is_alias_supported, (const std::string&, const std::string&), (const, override)); MOCK_METHOD(bool, set_permissions, (const multipass::Path&, const QFileDevice::Permissions), (const, override)); MOCK_METHOD(int, chown, (const char*, unsigned int, unsigned int), (const, override)); + MOCK_METHOD(bool, + set_permissions, + (const multipass::Path& path, const QFileDevice::Permissions), + (const, override)); + MOCK_METHOD(bool, set_root_as_owner, (const multipass::Path& path), (const, override)); MOCK_METHOD(bool, link, (const char*, const char*), (const, override)); MOCK_METHOD(bool, symlink, (const char*, const char*, bool), (const, override)); MOCK_METHOD(int, utime, (const char*, int, int), (const, override)); diff --git a/tests/test_base_virtual_machine_factory.cpp b/tests/test_base_virtual_machine_factory.cpp index 02e773887b..324afbf8df 100644 --- a/tests/test_base_virtual_machine_factory.cpp +++ b/tests/test_base_virtual_machine_factory.cpp @@ -123,6 +123,11 @@ TEST_F(BaseFactory, networks_throws) // at this time. Instead, just make sure an ISO image is created and has the expected path. TEST_F(BaseFactory, creates_cloud_init_iso_image) { + auto [mock_platform, platform_guard] = mpt::MockPlatform::inject(); + + ON_CALL(*mock_platform, set_permissions).WillByDefault(Return(true)); + ON_CALL(*mock_platform, set_root_as_owner).WillByDefault(Return(true)); + MockBaseFactory factory; const std::string name{"foo"}; const YAML::Node metadata{YAML::Load({fmt::format("name: {}", name)})}, vendor_data{metadata}, user_data{metadata}, diff --git a/tests/test_cloud_init_iso.cpp b/tests/test_cloud_init_iso.cpp index 5b110892b6..3b207741d8 100644 --- a/tests/test_cloud_init_iso.cpp +++ b/tests/test_cloud_init_iso.cpp @@ -17,6 +17,7 @@ #include "common.h" #include "mock_file_ops.h" +#include "mock_platform.h" #include "temp_dir.h" #include @@ -70,9 +71,15 @@ struct CloudInitIso : public Test CloudInitIso() { iso_path = QDir{temp_dir.path()}.filePath("test.iso"); + + ON_CALL(mock_platform, set_permissions).WillByDefault(Return(true)); + ON_CALL(mock_platform, set_root_as_owner).WillByDefault(Return(true)); } mpt::TempDir temp_dir; QString iso_path; + + const mpt::MockPlatform::GuardedMock attr{mpt::MockPlatform::inject()}; + mpt::MockPlatform& mock_platform = *attr.first; }; TEST_F(CloudInitIso, check_contains_false) From fa516354efe13e9e063ee83856eb6a69770eb2e4 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Fri, 4 Oct 2024 09:45:53 -0400 Subject: [PATCH 02/12] [lxd] Change instance image permissions to root only --- src/daemon/default_vm_image_vault.cpp | 2 +- .../backends/lxd/lxd_vm_image_vault.cpp | 3 +++ src/utils/vm_image_vault_utils.cpp | 4 ++-- tests/test_image_vault.cpp | 18 ++++++++++++++++++ 4 files changed, 24 insertions(+), 3 deletions(-) diff --git a/src/daemon/default_vm_image_vault.cpp b/src/daemon/default_vm_image_vault.cpp index d2f66c9790..6c93afa8c7 100644 --- a/src/daemon/default_vm_image_vault.cpp +++ b/src/daemon/default_vm_image_vault.cpp @@ -669,7 +669,7 @@ QString mp::DefaultVMImageVault::extract_image_from(const VMImage& source_image, const ProgressMonitor& monitor, const mp::Path& dest_dir) { - MP_UTILS.make_dir(dest_dir, QFile::ReadOwner | QFile::WriteOwner | QFile::ExeOwner); + MP_UTILS.make_dir(dest_dir, QFile::ReadOwner | QFile::WriteOwner); MP_PLATFORM.set_root_as_owner(dest_dir); QFileInfo file_info{source_image.image_path}; diff --git a/src/platform/backends/lxd/lxd_vm_image_vault.cpp b/src/platform/backends/lxd/lxd_vm_image_vault.cpp index b2b7cd05af..e53cefdcc1 100644 --- a/src/platform/backends/lxd/lxd_vm_image_vault.cpp +++ b/src/platform/backends/lxd/lxd_vm_image_vault.cpp @@ -93,6 +93,9 @@ QString post_process_downloaded_image(const QString& image_path, const mp::Progr mp::vault::delete_file(original_image_path); } + MP_PLATFORM.set_permissions(new_image_path, QFile::ReadOwner | QFile::WriteOwner); + MP_PLATFORM.set_root_as_owner(new_image_path); + return new_image_path; } diff --git a/src/utils/vm_image_vault_utils.cpp b/src/utils/vm_image_vault_utils.cpp index 860394d96d..a2735d7da1 100644 --- a/src/utils/vm_image_vault_utils.cpp +++ b/src/utils/vm_image_vault_utils.cpp @@ -47,7 +47,7 @@ QString mp::vault::copy(const QString& file_name, const QDir& output_dir) auto new_path = output_dir.filePath(source_name); QFile::copy(file_name, new_path); - MP_PLATFORM.set_permissions(new_path, QFile::ReadOwner | QFile::WriteOwner | QFile::ExeOwner); + MP_PLATFORM.set_permissions(new_path, QFile::ReadOwner | QFile::WriteOwner); MP_PLATFORM.set_root_as_owner(new_path); return new_path; @@ -95,7 +95,7 @@ QString mp::vault::extract_image(const mp::Path& image_path, const mp::ProgressM xz_decoder.decode_to(new_image_path, monitor); - MP_PLATFORM.set_permissions(new_image_path, QFile::ReadOwner | QFile::WriteOwner | QFile::ExeOwner); + MP_PLATFORM.set_permissions(new_image_path, QFile::ReadOwner | QFile::WriteOwner); MP_PLATFORM.set_root_as_owner(new_image_path); mp::vault::delete_file(image_path); diff --git a/tests/test_image_vault.cpp b/tests/test_image_vault.cpp index 29f3a31861..aae5398d22 100644 --- a/tests/test_image_vault.cpp +++ b/tests/test_image_vault.cpp @@ -21,6 +21,7 @@ #include "mock_image_host.h" #include "mock_json_utils.h" #include "mock_logger.h" +#include "mock_platform.h" #include "mock_process_factory.h" #include "path.h" #include "stub_url_downloader.h" @@ -413,6 +414,11 @@ TEST_F(ImageVault, remembers_prepared_images) TEST_F(ImageVault, uses_image_from_prepare) { + auto [mock_platform, platform_guard] = mpt::MockPlatform::inject(); + + ON_CALL(*mock_platform, set_permissions).WillByDefault(Return(true)); + ON_CALL(*mock_platform, set_root_as_owner).WillByDefault(Return(true)); + constexpr auto expected_data = "12345-pied-piper-rats"; QDir dir{cache_dir.path()}; @@ -508,6 +514,12 @@ TEST_F(ImageVault, invalid_image_dir_is_removed) TEST_F(ImageVault, DISABLE_ON_WINDOWS_AND_MACOS(file_based_fetch_copies_image_and_returns_expected_info)) { + auto [mock_platform, platform_guard] = mpt::MockPlatform::inject(); + + ON_CALL(*mock_platform, is_image_url_supported).WillByDefault(Return(true)); + ON_CALL(*mock_platform, set_permissions).WillByDefault(Return(true)); + ON_CALL(*mock_platform, set_root_as_owner).WillByDefault(Return(true)); + mpt::TempFile file; mp::DefaultVMImageVault vault{hosts, &url_downloader, cache_dir.path(), data_dir.path(), mp::days{0}}; auto query = default_query; @@ -739,6 +751,12 @@ TEST_F(ImageVault, minimum_image_size_returns_expected_size) TEST_F(ImageVault, DISABLE_ON_WINDOWS_AND_MACOS(file_based_minimum_size_returns_expected_size)) { + auto [mock_platform, platform_guard] = mpt::MockPlatform::inject(); + + ON_CALL(*mock_platform, is_image_url_supported).WillByDefault(Return(true)); + ON_CALL(*mock_platform, set_permissions).WillByDefault(Return(true)); + ON_CALL(*mock_platform, set_root_as_owner).WillByDefault(Return(true)); + const mp::MemorySize image_size{"2097152"}; const mp::ProcessState qemuimg_exit_status{0, std::nullopt}; const QByteArray qemuimg_output(fake_img_info(image_size)); From 18ace88d59797ad7b164ade6e6f74daddad4f552 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Tue, 26 Nov 2024 13:23:56 -0500 Subject: [PATCH 03/12] [tests] Add permission related tests for cloud_init --- tests/test_cloud_init_iso.cpp | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/test_cloud_init_iso.cpp b/tests/test_cloud_init_iso.cpp index 3b207741d8..feaf20c55c 100644 --- a/tests/test_cloud_init_iso.cpp +++ b/tests/test_cloud_init_iso.cpp @@ -481,3 +481,27 @@ TEST_F(CloudInitIso, getInstanceIdFromCloudInit) EXPECT_EQ(MP_CLOUD_INIT_FILE_OPS.get_instance_id_from_cloud_init(iso_path.toStdString()), "vm1"); } + +TEST_F(CloudInitIso, write_throws_on_failure_to_set_permissions) +{ + mp::CloudInitIso original_iso; + original_iso.add_file("test-name", "test-data"); + + EXPECT_CALL(mock_platform, set_permissions).WillOnce(Return(false)); + + MP_EXPECT_THROW_THAT(original_iso.write_to(iso_path), + std::runtime_error, + mpt::match_what(HasSubstr("Failed to set permissions"))); +} + +TEST_F(CloudInitIso, write_throws_on_failure_to_set_owner) +{ + mp::CloudInitIso original_iso; + original_iso.add_file("test-name", "test-data"); + + EXPECT_CALL(mock_platform, set_root_as_owner).WillOnce(Return(false)); + + MP_EXPECT_THROW_THAT(original_iso.write_to(iso_path), + std::runtime_error, + mpt::match_what(HasSubstr("Failed to set owner"))); +} From 3cb65f2ced708651bf324c706354967f51f8bfe3 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Mon, 2 Dec 2024 15:02:52 -0500 Subject: [PATCH 04/12] [permissions] Add permission utils --- include/multipass/platform.h | 2 +- include/multipass/utils/permission_utils.h | 43 ++++++++ src/daemon/default_vm_image_vault.cpp | 5 +- src/iso/cloud_init_iso.cpp | 7 +- .../backends/lxd/lxd_vm_image_vault.cpp | 4 +- src/platform/platform_unix.cpp | 9 +- src/utils/CMakeLists.txt | 1 + src/utils/permission_utils.cpp | 102 ++++++++++++++++++ src/utils/vm_image_vault_utils.cpp | 7 +- tests/lxd/test_lxd_image_vault.cpp | 4 + tests/mock_permission_utils.h | 47 ++++++++ tests/mock_platform.h | 2 +- tests/test_base_virtual_machine_factory.cpp | 6 +- tests/test_cloud_init_iso.cpp | 68 ++++++------ tests/test_image_vault.cpp | 21 ++-- tests/test_utils.cpp | 5 + 16 files changed, 272 insertions(+), 61 deletions(-) create mode 100644 include/multipass/utils/permission_utils.h create mode 100644 src/utils/permission_utils.cpp create mode 100644 tests/mock_permission_utils.h diff --git a/include/multipass/platform.h b/include/multipass/platform.h index 036ad04c2b..461a2bac5a 100644 --- a/include/multipass/platform.h +++ b/include/multipass/platform.h @@ -59,7 +59,7 @@ class Platform : public Singleton virtual bool is_backend_supported(const QString& backend) const; // temporary (?) virtual int chown(const char* path, unsigned int uid, unsigned int gid) const; virtual bool set_permissions(const Path& path, const Perms permissions) const; - virtual bool set_root_as_owner(const multipass::Path& path) const; + virtual bool take_ownership(const Path& path, bool root = true) const; virtual bool link(const char* target, const char* link) const; virtual bool symlink(const char* target, const char* link, bool is_dir) const; virtual int utime(const char* path, int atime, int mtime) const; diff --git a/include/multipass/utils/permission_utils.h b/include/multipass/utils/permission_utils.h new file mode 100644 index 0000000000..3f72be9867 --- /dev/null +++ b/include/multipass/utils/permission_utils.h @@ -0,0 +1,43 @@ +/* + * Copyright (C) Canonical, Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 3. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * + */ + +#ifndef MULTIPASS_PERMISSION_UTILS_H +#define MULTIPASS_PERMISSION_UTILS_H + +#include +#include +#include + +#define MP_PERMISSIONS multipass::PermissionUtils::instance() + +namespace multipass +{ + +class PermissionUtils : public Singleton +{ +public: + PermissionUtils(const PrivatePass&) noexcept; + + virtual void set_permissions(const fs::path& path, const QFileDevice::Permissions& permissions) const; + virtual void take_ownership(const fs::path& path, bool root = true) const; + + // sets owner to root and sets permissions such that only owner has access. + virtual void restrict_permissions(const fs::path& path) const; +}; +} // namespace multipass + +#endif // MULTIPASS_PERMISSION_UTILS_H diff --git a/src/daemon/default_vm_image_vault.cpp b/src/daemon/default_vm_image_vault.cpp index 6c93afa8c7..1dd6c8b729 100644 --- a/src/daemon/default_vm_image_vault.cpp +++ b/src/daemon/default_vm_image_vault.cpp @@ -41,6 +41,7 @@ #include #include +#include namespace mp = multipass; namespace mpl = multipass::logging; @@ -670,7 +671,7 @@ QString mp::DefaultVMImageVault::extract_image_from(const VMImage& source_image, const mp::Path& dest_dir) { MP_UTILS.make_dir(dest_dir, QFile::ReadOwner | QFile::WriteOwner); - MP_PLATFORM.set_root_as_owner(dest_dir); + MP_PERMISSIONS.take_ownership(dest_dir.toStdString()); QFileInfo file_info{source_image.image_path}; const auto image_name = file_info.fileName().remove(".xz"); @@ -682,7 +683,7 @@ QString mp::DefaultVMImageVault::extract_image_from(const VMImage& source_image, mp::VMImage mp::DefaultVMImageVault::image_instance_from(const VMImage& prepared_image, const mp::Path& dest_dir) { MP_UTILS.make_dir(dest_dir, QFile::ReadOwner | QFile::WriteOwner | QFile::ExeOwner); - MP_PLATFORM.set_root_as_owner(dest_dir); + MP_PERMISSIONS.take_ownership(dest_dir.toStdString()); return {mp::vault::copy(prepared_image.image_path, dest_dir), prepared_image.id, diff --git a/src/iso/cloud_init_iso.cpp b/src/iso/cloud_init_iso.cpp index 11e51b8839..161b064736 100644 --- a/src/iso/cloud_init_iso.cpp +++ b/src/iso/cloud_init_iso.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include @@ -537,11 +538,7 @@ void mp::CloudInitIso::write_to(const Path& path) throw std::runtime_error{fmt::format( "Failed to open file for writing during cloud-init generation: {}; path: {}", f.errorString(), path)}; - if (!MP_PLATFORM.set_permissions(path, QFile::ReadOwner | QFile::WriteOwner)) - throw std::runtime_error{fmt::format("Failed to set permissions during cloud-init generation: path: {}", path)}; - - if (!MP_PLATFORM.set_root_as_owner(path)) - throw std::runtime_error{fmt::format("Failed to set owner during cloud-init generation: path: {}", path)}; + MP_PERMISSIONS.restrict_permissions(path.toStdU16String()); const uint32_t num_reserved_bytes = 32768u; const uint32_t num_reserved_blocks = num_blocks(num_reserved_bytes); diff --git a/src/platform/backends/lxd/lxd_vm_image_vault.cpp b/src/platform/backends/lxd/lxd_vm_image_vault.cpp index e53cefdcc1..63e94e4fa3 100644 --- a/src/platform/backends/lxd/lxd_vm_image_vault.cpp +++ b/src/platform/backends/lxd/lxd_vm_image_vault.cpp @@ -47,6 +47,7 @@ #include #include +#include #include namespace mp = multipass; @@ -93,8 +94,7 @@ QString post_process_downloaded_image(const QString& image_path, const mp::Progr mp::vault::delete_file(original_image_path); } - MP_PLATFORM.set_permissions(new_image_path, QFile::ReadOwner | QFile::WriteOwner); - MP_PLATFORM.set_root_as_owner(new_image_path); + MP_PERMISSIONS.restrict_permissions(new_image_path.toStdU16String()); return new_image_path; } diff --git a/src/platform/platform_unix.cpp b/src/platform/platform_unix.cpp index e265dc94e0..eb68a41a11 100644 --- a/src/platform/platform_unix.cpp +++ b/src/platform/platform_unix.cpp @@ -63,9 +63,14 @@ bool mp::platform::Platform::set_permissions(const Path& path, const Perms permi return QFile::setPermissions(path, permissions); } -bool mp::platform::Platform::set_root_as_owner(const mp::Path& path) const +bool mp::platform::Platform::take_ownership(const mp::Path& path, bool root) const { - return this->chown(path.toStdString().c_str(), 0, 0) == 0; + if (root) + { + return this->chown(path.toStdString().c_str(), 0, 0) == 0; + } + + throw std::logic_error("NYI"); } bool mp::platform::Platform::symlink(const char* target, const char* link, bool is_dir) const diff --git a/src/utils/CMakeLists.txt b/src/utils/CMakeLists.txt index f0b501c22b..006c198678 100644 --- a/src/utils/CMakeLists.txt +++ b/src/utils/CMakeLists.txt @@ -16,6 +16,7 @@ function(add_target TARGET_NAME) add_library(${TARGET_NAME} STATIC file_ops.cpp memory_size.cpp + permission_utils.cpp json_utils.cpp snap_utils.cpp standard_paths.cpp diff --git a/src/utils/permission_utils.cpp b/src/utils/permission_utils.cpp new file mode 100644 index 0000000000..14a37d6cb4 --- /dev/null +++ b/src/utils/permission_utils.cpp @@ -0,0 +1,102 @@ +/* + * Copyright (C) Canonical, Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 3. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * + */ + +#include + +namespace mp = multipass; +namespace fs = mp::fs; + +namespace +{ +void set_single_permissions(const fs::path& path, const QFileDevice::Permissions& permissions) +{ + QString qpath = QString::fromUtf8(path.u8string()); + + if (!MP_PLATFORM.set_permissions(qpath, permissions)) + throw std::runtime_error(fmt::format("Cannot set permissions for '{}'", path.string())); +} + +void set_single_owner(const fs::path& path, bool root) +{ + QString qpath = QString::fromUtf8(path.u8string()); + + if (!MP_PLATFORM.take_ownership(qpath, root)) + throw std::runtime_error(fmt::format("Cannot set owner for '{}'", path.string())); +} + +// only exists because MP_FILEOPS doesn't overload the throwing variaions of std::filesystem functions +void throw_if_error(const fs::path& path, const std::error_code& ec) +{ + if (ec) + throw std::system_error( + ec, + fmt::format("System error occurred while handling permissions for '{}'", path.string())); +} + +// recursively iterates over all files in folder and applies a function that takes a path +template +void apply_on_files(const fs::path& path, Func&& func) +{ + std::error_code ec{}; + if (!MP_FILEOPS.exists(path, ec) || ec) + throw std::runtime_error(fmt::format("Cannot handle permissions for nonexistent file '{}'", path.string())); + + func(path); + + // iterate over children if directory + if (MP_FILEOPS.is_directory(path, ec)) + { + auto dir_iterator = MP_FILEOPS.recursive_dir_iterator(path, ec); + throw_if_error(path, ec); + + if (!dir_iterator) + throw std::runtime_error(fmt::format("Cannot iterate over directory '{}'", path.string())); + + while (dir_iterator->hasNext()) + { + const auto& entry = dir_iterator->next(); + + func(entry.path()); + } + } + + throw_if_error(path, ec); +} +} // namespace + +mp::PermissionUtils::PermissionUtils(const Singleton::PrivatePass& pass) noexcept + : Singleton::Singleton{pass} +{ +} + +void mp::PermissionUtils::set_permissions(const fs::path& path, const QFileDevice::Permissions& permissions) const +{ + apply_on_files(path, [&](const fs::path& apply_path) { set_single_permissions(apply_path, permissions); }); +} + +void mp::PermissionUtils::take_ownership(const fs::path& path, bool root) const +{ + apply_on_files(path, [&](const fs::path& apply_path) { set_single_owner(apply_path, root); }); +} + +void mp::PermissionUtils::restrict_permissions(const fs::path& path) const +{ + apply_on_files(path, [&](const fs::path& apply_path) { + set_single_owner(apply_path, true); + set_single_permissions(apply_path, QFile::ReadOwner | QFile::WriteOwner); + }); +} diff --git a/src/utils/vm_image_vault_utils.cpp b/src/utils/vm_image_vault_utils.cpp index a2735d7da1..5a62d6c952 100644 --- a/src/utils/vm_image_vault_utils.cpp +++ b/src/utils/vm_image_vault_utils.cpp @@ -17,6 +17,7 @@ #include #include +#include #include #include #include @@ -47,8 +48,7 @@ QString mp::vault::copy(const QString& file_name, const QDir& output_dir) auto new_path = output_dir.filePath(source_name); QFile::copy(file_name, new_path); - MP_PLATFORM.set_permissions(new_path, QFile::ReadOwner | QFile::WriteOwner); - MP_PLATFORM.set_root_as_owner(new_path); + MP_PERMISSIONS.restrict_permissions(new_path.toStdU16String()); return new_path; } @@ -95,8 +95,7 @@ QString mp::vault::extract_image(const mp::Path& image_path, const mp::ProgressM xz_decoder.decode_to(new_image_path, monitor); - MP_PLATFORM.set_permissions(new_image_path, QFile::ReadOwner | QFile::WriteOwner); - MP_PLATFORM.set_root_as_owner(new_image_path); + MP_PERMISSIONS.restrict_permissions(new_image_path.toStdU16String()); mp::vault::delete_file(image_path); diff --git a/tests/lxd/test_lxd_image_vault.cpp b/tests/lxd/test_lxd_image_vault.cpp index 16c5d0b983..2fcb6f8906 100644 --- a/tests/lxd/test_lxd_image_vault.cpp +++ b/tests/lxd/test_lxd_image_vault.cpp @@ -22,6 +22,7 @@ #include "tests/common.h" #include "tests/mock_image_host.h" #include "tests/mock_logger.h" +#include "tests/mock_permission_utils.h" #include "tests/mock_process_factory.h" #include "tests/stub_url_downloader.h" #include "tests/temp_dir.h" @@ -60,6 +61,9 @@ struct LXDImageVault : public Test mpt::MockLogger::Scope logger_scope = mpt::MockLogger::inject(); std::unique_ptr> mock_network_access_manager; + mpt::MockPermissionUtils::GuardedMock mock_permission_utils_injection = + mpt::MockPermissionUtils::inject(); + mpt::MockPermissionUtils& mock_permission_utils = *mock_permission_utils_injection.first; std::vector hosts; NiceMock host; QUrl base_url{"unix:///foo@1.0"}; diff --git a/tests/mock_permission_utils.h b/tests/mock_permission_utils.h new file mode 100644 index 0000000000..69a8b07aba --- /dev/null +++ b/tests/mock_permission_utils.h @@ -0,0 +1,47 @@ +/* + * Copyright (C) Canonical, Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 3. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * + */ + +#ifndef MULTIPASS_MOCK_PERMISSION_UTILS_H +#define MULTIPASS_MOCK_PERMISSION_UTILS_H + +#include "common.h" +#include "mock_singleton_helpers.h" + +#include + +namespace multipass::test +{ + +class MockPermissionUtils : public PermissionUtils +{ +public: + MockPermissionUtils(const PrivatePass& pass) : PermissionUtils(pass) + { + } + + MOCK_METHOD(void, + set_permissions, + (const fs::path& path, const QFileDevice::Permissions& permissions), + (const, override)); + MOCK_METHOD(void, take_ownership, (const fs::path& path, bool root), (const, override)); + MOCK_METHOD(void, restrict_permissions, (const fs::path& path), (const, override)); + + MP_MOCK_SINGLETON_BOILERPLATE(MockPermissionUtils, PermissionUtils); +}; +} // namespace multipass::test + +#endif // MULTIPASS_MOCK_PERMISSION_UTILS_H diff --git a/tests/mock_platform.h b/tests/mock_platform.h index 54594dcaf1..fe6d0a6f23 100644 --- a/tests/mock_platform.h +++ b/tests/mock_platform.h @@ -46,7 +46,7 @@ class MockPlatform : public platform::Platform set_permissions, (const multipass::Path& path, const QFileDevice::Permissions), (const, override)); - MOCK_METHOD(bool, set_root_as_owner, (const multipass::Path& path), (const, override)); + MOCK_METHOD(bool, take_ownership, (const multipass::Path& path, bool root), (const, override)); MOCK_METHOD(bool, link, (const char*, const char*), (const, override)); MOCK_METHOD(bool, symlink, (const char*, const char*, bool), (const, override)); MOCK_METHOD(int, utime, (const char*, int, int), (const, override)); diff --git a/tests/test_base_virtual_machine_factory.cpp b/tests/test_base_virtual_machine_factory.cpp index 324afbf8df..0494484ba5 100644 --- a/tests/test_base_virtual_machine_factory.cpp +++ b/tests/test_base_virtual_machine_factory.cpp @@ -17,6 +17,7 @@ #include "common.h" #include "mock_logger.h" +#include "mock_permission_utils.h" #include "mock_platform.h" #include "stub_ssh_key_provider.h" #include "stub_url_downloader.h" @@ -123,10 +124,9 @@ TEST_F(BaseFactory, networks_throws) // at this time. Instead, just make sure an ISO image is created and has the expected path. TEST_F(BaseFactory, creates_cloud_init_iso_image) { - auto [mock_platform, platform_guard] = mpt::MockPlatform::inject(); + auto [mock_permission_utils, permission_utils_guard] = mpt::MockPermissionUtils::inject(); - ON_CALL(*mock_platform, set_permissions).WillByDefault(Return(true)); - ON_CALL(*mock_platform, set_root_as_owner).WillByDefault(Return(true)); + EXPECT_CALL(*mock_permission_utils, restrict_permissions(_)); MockBaseFactory factory; const std::string name{"foo"}; diff --git a/tests/test_cloud_init_iso.cpp b/tests/test_cloud_init_iso.cpp index feaf20c55c..194fe3506c 100644 --- a/tests/test_cloud_init_iso.cpp +++ b/tests/test_cloud_init_iso.cpp @@ -17,7 +17,7 @@ #include "common.h" #include "mock_file_ops.h" -#include "mock_platform.h" +#include "mock_permission_utils.h" #include "temp_dir.h" #include @@ -71,15 +71,14 @@ struct CloudInitIso : public Test CloudInitIso() { iso_path = QDir{temp_dir.path()}.filePath("test.iso"); - - ON_CALL(mock_platform, set_permissions).WillByDefault(Return(true)); - ON_CALL(mock_platform, set_root_as_owner).WillByDefault(Return(true)); + std_iso_path = std::filesystem::path(iso_path.toStdU16String()); } mpt::TempDir temp_dir; QString iso_path; + std::filesystem::path std_iso_path; - const mpt::MockPlatform::GuardedMock attr{mpt::MockPlatform::inject()}; - mpt::MockPlatform& mock_platform = *attr.first; + const mpt::MockPermissionUtils::GuardedMock attr{mpt::MockPermissionUtils::inject()}; + mpt::MockPermissionUtils& mock_permission_utils = *attr.first; }; TEST_F(CloudInitIso, check_contains_false) @@ -141,6 +140,8 @@ TEST_F(CloudInitIso, check_index_operator_found_key) TEST_F(CloudInitIso, creates_iso_file) { + EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); + mp::CloudInitIso iso; iso.add_file("test", "test data"); iso.write_to(iso_path); @@ -152,6 +153,8 @@ TEST_F(CloudInitIso, creates_iso_file) TEST_F(CloudInitIso, reads_iso_file_failed_to_open_file) { + EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); + mp::CloudInitIso original_iso; original_iso.write_to(iso_path); @@ -165,6 +168,8 @@ TEST_F(CloudInitIso, reads_iso_file_failed_to_open_file) TEST_F(CloudInitIso, reads_iso_file_failed_to_read_single_bytes) { + EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); + mp::CloudInitIso original_iso; original_iso.write_to(iso_path); @@ -183,6 +188,8 @@ TEST_F(CloudInitIso, reads_iso_file_failed_to_read_single_bytes) TEST_F(CloudInitIso, reads_iso_file_failed_to_check_it_has_Joliet_volume_descriptor) { + EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); + mp::CloudInitIso original_iso; original_iso.write_to(iso_path); @@ -206,6 +213,8 @@ TEST_F(CloudInitIso, reads_iso_file_failed_to_check_it_has_Joliet_volume_descrip TEST_F(CloudInitIso, reads_iso_file_Joliet_volume_descriptor_malformed) { + EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); + mp::CloudInitIso original_iso; original_iso.write_to(iso_path); @@ -231,6 +240,8 @@ TEST_F(CloudInitIso, reads_iso_file_Joliet_volume_descriptor_malformed) TEST_F(CloudInitIso, reads_iso_file_failed_to_read_array) { + EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); + mp::CloudInitIso original_iso; original_iso.write_to(iso_path); @@ -252,6 +263,8 @@ TEST_F(CloudInitIso, reads_iso_file_failed_to_read_array) TEST_F(CloudInitIso, reads_iso_file_failed_to_check_root_dir_record_data) { + EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); + mp::CloudInitIso original_iso; original_iso.write_to(iso_path); @@ -279,6 +292,8 @@ TEST_F(CloudInitIso, reads_iso_file_failed_to_check_root_dir_record_data) TEST_F(CloudInitIso, reads_iso_file_failed_to_read_vec) { + EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); + mp::CloudInitIso original_iso; // At least one actual file entry is need to reach the read_bytes_to_vec call original_iso.add_file("test1", "test data1"); @@ -304,6 +319,8 @@ TEST_F(CloudInitIso, reads_iso_file_failed_to_read_vec) TEST_F(CloudInitIso, reads_iso_file_encoded_file_name_is_not_even_length) { + EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); + mp::CloudInitIso original_iso; // At least one actual file entry is need to reach the convert_u16_name_back call original_iso.add_file("test1", "test data1"); @@ -339,6 +356,8 @@ TEST_F(CloudInitIso, reads_iso_file_encoded_file_name_is_not_even_length) TEST_F(CloudInitIso, reads_iso_file_with_random_string_data) { + EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); + mp::CloudInitIso original_iso; original_iso.add_file("test1", "test data1"); @@ -375,6 +394,9 @@ timezone: Europe/Amsterdam - path: /etc/pollinate/add-user-agent content: "multipass/version/1.14.0-dev.1209+g5b2c7f7d # written by Multipass\nmultipass/driver/qemu-8.0.4 # written by Multipass\nmultipass/host/ubuntu-23.10 # written by Multipass\nmultipass/alias/default # written by Multipass\n" )"; + + EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); + mp::CloudInitIso original_iso; original_iso.add_file("meta-data", default_meta_data_content); @@ -390,6 +412,8 @@ timezone: Europe/Amsterdam TEST_F(CloudInitIso, updateCloudInitWithNewNonEmptyExtraInterfaces) { + EXPECT_CALL(mock_permission_utils, restrict_permissions(std_iso_path)).Times(2); + mp::CloudInitIso original_iso; original_iso.add_file("meta-data", default_meta_data_content); @@ -416,6 +440,8 @@ TEST_F(CloudInitIso, updateCloudInitWithNewNonEmptyExtraInterfaces) TEST_F(CloudInitIso, updateCloudInitWithNewEmptyExtraInterfaces) { + EXPECT_CALL(mock_permission_utils, restrict_permissions(std_iso_path)).Times(2); + mp::CloudInitIso original_iso; original_iso.add_file("meta-data", default_meta_data_content); original_iso.add_file("network-config", "dummy_data"); @@ -439,6 +465,8 @@ TEST_F(CloudInitIso, updateCloneCloudInitSrcFileWithExtraInterfaces) const std::string src_network_config_data_content = fmt::format(network_config_data_content_template, "00:00:00:00:00:00", "00:00:00:00:00:01"); + EXPECT_CALL(mock_permission_utils, restrict_permissions(std_iso_path)).Times(2); + mp::CloudInitIso original_iso; original_iso.add_file("meta-data", src_meta_data_content); original_iso.add_file("network-config", src_network_config_data_content); @@ -464,6 +492,8 @@ TEST_F(CloudInitIso, updateCloneCloudInitSrcFileWithExtraInterfaces) TEST_F(CloudInitIso, addExtraInterfaceToCloudInit) { + EXPECT_CALL(mock_permission_utils, restrict_permissions(std_iso_path)).Times(2); + mp::CloudInitIso original_iso; original_iso.add_file("meta-data", default_meta_data_content); original_iso.write_to(iso_path); @@ -475,33 +505,11 @@ TEST_F(CloudInitIso, addExtraInterfaceToCloudInit) TEST_F(CloudInitIso, getInstanceIdFromCloudInit) { + EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); + mp::CloudInitIso original_iso; original_iso.add_file("meta-data", default_meta_data_content); original_iso.write_to(iso_path); EXPECT_EQ(MP_CLOUD_INIT_FILE_OPS.get_instance_id_from_cloud_init(iso_path.toStdString()), "vm1"); } - -TEST_F(CloudInitIso, write_throws_on_failure_to_set_permissions) -{ - mp::CloudInitIso original_iso; - original_iso.add_file("test-name", "test-data"); - - EXPECT_CALL(mock_platform, set_permissions).WillOnce(Return(false)); - - MP_EXPECT_THROW_THAT(original_iso.write_to(iso_path), - std::runtime_error, - mpt::match_what(HasSubstr("Failed to set permissions"))); -} - -TEST_F(CloudInitIso, write_throws_on_failure_to_set_owner) -{ - mp::CloudInitIso original_iso; - original_iso.add_file("test-name", "test-data"); - - EXPECT_CALL(mock_platform, set_root_as_owner).WillOnce(Return(false)); - - MP_EXPECT_THROW_THAT(original_iso.write_to(iso_path), - std::runtime_error, - mpt::match_what(HasSubstr("Failed to set owner"))); -} diff --git a/tests/test_image_vault.cpp b/tests/test_image_vault.cpp index aae5398d22..77d5ab5dcd 100644 --- a/tests/test_image_vault.cpp +++ b/tests/test_image_vault.cpp @@ -21,6 +21,7 @@ #include "mock_image_host.h" #include "mock_json_utils.h" #include "mock_logger.h" +#include "mock_permission_utils.h" #include "mock_platform.h" #include "mock_process_factory.h" #include "path.h" @@ -167,6 +168,9 @@ struct ImageVault : public testing::Test NiceMock host; mpt::MockJsonUtils::GuardedMock mock_json_utils_injection = mpt::MockJsonUtils::inject(); mpt::MockJsonUtils& mock_json_utils = *mock_json_utils_injection.first; + mpt::MockPermissionUtils::GuardedMock mock_permission_utils_injection = + mpt::MockPermissionUtils::inject(); + mpt::MockPermissionUtils& mock_permission_utils = *mock_permission_utils_injection.first; mp::ProgressMonitor stub_monitor{[](int, int) { return true; }}; mp::VMImageVault::PrepareAction stub_prepare{ [](const mp::VMImage& source_image) -> mp::VMImage { return source_image; }}; @@ -414,10 +418,9 @@ TEST_F(ImageVault, remembers_prepared_images) TEST_F(ImageVault, uses_image_from_prepare) { - auto [mock_platform, platform_guard] = mpt::MockPlatform::inject(); + auto [mock_permission_utils, permission_utils_guard] = mpt::MockPermissionUtils::inject(); - ON_CALL(*mock_platform, set_permissions).WillByDefault(Return(true)); - ON_CALL(*mock_platform, set_root_as_owner).WillByDefault(Return(true)); + EXPECT_CALL(*mock_permission_utils, restrict_permissions(_)); constexpr auto expected_data = "12345-pied-piper-rats"; @@ -514,11 +517,9 @@ TEST_F(ImageVault, invalid_image_dir_is_removed) TEST_F(ImageVault, DISABLE_ON_WINDOWS_AND_MACOS(file_based_fetch_copies_image_and_returns_expected_info)) { - auto [mock_platform, platform_guard] = mpt::MockPlatform::inject(); + auto [mock_permission_utils, permission_utils_guard] = mpt::MockPermissionUtils::inject(); - ON_CALL(*mock_platform, is_image_url_supported).WillByDefault(Return(true)); - ON_CALL(*mock_platform, set_permissions).WillByDefault(Return(true)); - ON_CALL(*mock_platform, set_root_as_owner).WillByDefault(Return(true)); + EXPECT_CALL(*mock_permission_utils, restrict_permissions(_)); mpt::TempFile file; mp::DefaultVMImageVault vault{hosts, &url_downloader, cache_dir.path(), data_dir.path(), mp::days{0}}; @@ -751,11 +752,9 @@ TEST_F(ImageVault, minimum_image_size_returns_expected_size) TEST_F(ImageVault, DISABLE_ON_WINDOWS_AND_MACOS(file_based_minimum_size_returns_expected_size)) { - auto [mock_platform, platform_guard] = mpt::MockPlatform::inject(); + auto [mock_permission_utils, permission_utils_guard] = mpt::MockPermissionUtils::inject(); - ON_CALL(*mock_platform, is_image_url_supported).WillByDefault(Return(true)); - ON_CALL(*mock_platform, set_permissions).WillByDefault(Return(true)); - ON_CALL(*mock_platform, set_root_as_owner).WillByDefault(Return(true)); + EXPECT_CALL(*mock_permission_utils, restrict_permissions(_)); const mp::MemorySize image_size{"2097152"}; const mp::ProcessState qemuimg_exit_status{0, std::nullopt}; diff --git a/tests/test_utils.cpp b/tests/test_utils.cpp index 4732add8a6..42236255c8 100644 --- a/tests/test_utils.cpp +++ b/tests/test_utils.cpp @@ -20,6 +20,7 @@ #include "mock_file_ops.h" #include "mock_logger.h" #include "mock_openssl_syscalls.h" +#include "mock_permission_utils.h" #include "mock_ssh.h" #include "mock_ssh_process_exit_status.h" #include "mock_ssh_test_fixture.h" @@ -749,6 +750,10 @@ TEST(Utils, check_filesystem_bytes_available_returns_non_negative) TEST(VaultUtils, copy_creates_new_file_and_returned_path_exists) { + auto [mock_permission_utils, permission_utils_guard] = mpt::MockPermissionUtils::inject(); + + EXPECT_CALL(*mock_permission_utils, restrict_permissions(_)); + mpt::TempDir temp_dir1, temp_dir2; auto orig_file_path = QDir(temp_dir1.path()).filePath("test_file"); From 04bb354e7fdf66e822e0f01f996584872c39175a Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Mon, 2 Dec 2024 17:36:27 -0500 Subject: [PATCH 05/12] [tests] Add permission utils tests --- include/multipass/utils/permission_utils.h | 13 +- src/utils/permission_utils.cpp | 21 +- tests/CMakeLists.txt | 1 + tests/mock_permission_utils.h | 6 +- tests/test_permission_utils.cpp | 227 +++++++++++++++++++++ 5 files changed, 252 insertions(+), 16 deletions(-) create mode 100644 tests/test_permission_utils.cpp diff --git a/include/multipass/utils/permission_utils.h b/include/multipass/utils/permission_utils.h index 3f72be9867..6bb35a56bd 100644 --- a/include/multipass/utils/permission_utils.h +++ b/include/multipass/utils/permission_utils.h @@ -18,10 +18,11 @@ #ifndef MULTIPASS_PERMISSION_UTILS_H #define MULTIPASS_PERMISSION_UTILS_H -#include -#include #include +#include +#include + #define MP_PERMISSIONS multipass::PermissionUtils::instance() namespace multipass @@ -30,13 +31,15 @@ namespace multipass class PermissionUtils : public Singleton { public: + using Path = std::filesystem::path; + PermissionUtils(const PrivatePass&) noexcept; - virtual void set_permissions(const fs::path& path, const QFileDevice::Permissions& permissions) const; - virtual void take_ownership(const fs::path& path, bool root = true) const; + virtual void set_permissions(const Path& path, const QFileDevice::Permissions& permissions) const; + virtual void take_ownership(const Path& path, bool root = true) const; // sets owner to root and sets permissions such that only owner has access. - virtual void restrict_permissions(const fs::path& path) const; + virtual void restrict_permissions(const Path& path) const; }; } // namespace multipass diff --git a/src/utils/permission_utils.cpp b/src/utils/permission_utils.cpp index 14a37d6cb4..31004e9ff2 100644 --- a/src/utils/permission_utils.cpp +++ b/src/utils/permission_utils.cpp @@ -17,12 +17,17 @@ #include +#include +#include + namespace mp = multipass; namespace fs = mp::fs; +using Path = mp::PermissionUtils::Path; + namespace { -void set_single_permissions(const fs::path& path, const QFileDevice::Permissions& permissions) +void set_single_permissions(const Path& path, const QFileDevice::Permissions& permissions) { QString qpath = QString::fromUtf8(path.u8string()); @@ -30,7 +35,7 @@ void set_single_permissions(const fs::path& path, const QFileDevice::Permissions throw std::runtime_error(fmt::format("Cannot set permissions for '{}'", path.string())); } -void set_single_owner(const fs::path& path, bool root) +void set_single_owner(const Path& path, bool root) { QString qpath = QString::fromUtf8(path.u8string()); @@ -39,7 +44,7 @@ void set_single_owner(const fs::path& path, bool root) } // only exists because MP_FILEOPS doesn't overload the throwing variaions of std::filesystem functions -void throw_if_error(const fs::path& path, const std::error_code& ec) +void throw_if_error(const Path& path, const std::error_code& ec) { if (ec) throw std::system_error( @@ -49,7 +54,7 @@ void throw_if_error(const fs::path& path, const std::error_code& ec) // recursively iterates over all files in folder and applies a function that takes a path template -void apply_on_files(const fs::path& path, Func&& func) +void apply_on_files(const Path& path, Func&& func) { std::error_code ec{}; if (!MP_FILEOPS.exists(path, ec) || ec) @@ -63,7 +68,7 @@ void apply_on_files(const fs::path& path, Func&& func) auto dir_iterator = MP_FILEOPS.recursive_dir_iterator(path, ec); throw_if_error(path, ec); - if (!dir_iterator) + if (!dir_iterator) [[unlikely]] throw std::runtime_error(fmt::format("Cannot iterate over directory '{}'", path.string())); while (dir_iterator->hasNext()) @@ -83,17 +88,17 @@ mp::PermissionUtils::PermissionUtils(const Singleton::PrivatePa { } -void mp::PermissionUtils::set_permissions(const fs::path& path, const QFileDevice::Permissions& permissions) const +void mp::PermissionUtils::set_permissions(const Path& path, const QFileDevice::Permissions& permissions) const { apply_on_files(path, [&](const fs::path& apply_path) { set_single_permissions(apply_path, permissions); }); } -void mp::PermissionUtils::take_ownership(const fs::path& path, bool root) const +void mp::PermissionUtils::take_ownership(const Path& path, bool root) const { apply_on_files(path, [&](const fs::path& apply_path) { set_single_owner(apply_path, root); }); } -void mp::PermissionUtils::restrict_permissions(const fs::path& path) const +void mp::PermissionUtils::restrict_permissions(const Path& path) const { apply_on_files(path, [&](const fs::path& apply_path) { set_single_owner(apply_path, true); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index b201a931ae..a1e24a761d 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -124,6 +124,7 @@ add_executable(multipass_tests test_sftp_utils.cpp test_file_ops.cpp test_recursive_dir_iter.cpp + test_permission_utils.cpp ) target_include_directories(multipass_tests diff --git a/tests/mock_permission_utils.h b/tests/mock_permission_utils.h index 69a8b07aba..0d630a19d5 100644 --- a/tests/mock_permission_utils.h +++ b/tests/mock_permission_utils.h @@ -35,10 +35,10 @@ class MockPermissionUtils : public PermissionUtils MOCK_METHOD(void, set_permissions, - (const fs::path& path, const QFileDevice::Permissions& permissions), + (const Path& path, const QFileDevice::Permissions& permissions), (const, override)); - MOCK_METHOD(void, take_ownership, (const fs::path& path, bool root), (const, override)); - MOCK_METHOD(void, restrict_permissions, (const fs::path& path), (const, override)); + MOCK_METHOD(void, take_ownership, (const Path& path, bool root), (const, override)); + MOCK_METHOD(void, restrict_permissions, (const Path& path), (const, override)); MP_MOCK_SINGLETON_BOILERPLATE(MockPermissionUtils, PermissionUtils); }; diff --git a/tests/test_permission_utils.cpp b/tests/test_permission_utils.cpp new file mode 100644 index 0000000000..ae132b93ee --- /dev/null +++ b/tests/test_permission_utils.cpp @@ -0,0 +1,227 @@ +/* + * Copyright (C) Canonical, Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 3. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * + */ + +#include + +#include "mock_file_ops.h" +#include "mock_permission_utils.h" +#include "mock_platform.h" +#include "mock_recursive_dir_iterator.h" + +namespace mp = multipass; +namespace mpt = mp::test; +using namespace testing; + +namespace +{ +struct TestPermissionUtils : public Test +{ + const mpt::MockFileOps::GuardedMock guarded_mock_file_ops = mpt::MockFileOps::inject(); + mpt::MockFileOps& mock_file_ops = *guarded_mock_file_ops.first; + const mpt::MockPlatform::GuardedMock guarded_mock_platform = mpt::MockPlatform::inject(); + mpt::MockPlatform& mock_platform = *guarded_mock_platform.first; + + const mp::PermissionUtils::Path test_path{"test_path"}; + const QString test_qpath{QString::fromUtf8(test_path.u8string())}; + static constexpr QFileDevice::Permissions test_permissions{QFileDevice::Permission::ReadOwner}; + static constexpr QFileDevice::Permissions restricted_permissions{QFileDevice::Permission::ReadOwner | + QFileDevice::Permission::WriteOwner}; +}; + +struct TestPermissionUtilsNoFile : public TestPermissionUtils +{ + TestPermissionUtilsNoFile() + { + EXPECT_CALL(mock_file_ops, exists(test_path, _)).WillOnce(Return(false)); + } +}; + +TEST_F(TestPermissionUtilsNoFile, set_permissions_throws_when_file_non_existant) +{ + MP_EXPECT_THROW_THAT(MP_PERMISSIONS.set_permissions(test_path, test_permissions), + std::runtime_error, + mpt::match_what(HasSubstr(test_path.string()))); +} + +TEST_F(TestPermissionUtilsNoFile, take_ownership_throws_when_file_non_existant) +{ + MP_EXPECT_THROW_THAT(MP_PERMISSIONS.take_ownership(test_path), + std::runtime_error, + mpt::match_what(HasSubstr(test_path.string()))); +} + +TEST_F(TestPermissionUtilsNoFile, restrict_permissions_throws_when_file_non_existant) +{ + MP_EXPECT_THROW_THAT(MP_PERMISSIONS.restrict_permissions(test_path), + std::runtime_error, + mpt::match_what(AllOf(HasSubstr("nonexistent file"), HasSubstr(test_path.string())))); +} + +struct TestPermissionUtilsFile : public TestPermissionUtils +{ + TestPermissionUtilsFile() + { + EXPECT_CALL(mock_file_ops, exists(test_path, _)).WillOnce(Return(true)); + EXPECT_CALL(mock_file_ops, is_directory(test_path, _)).WillRepeatedly(Return(false)); + } +}; + +TEST_F(TestPermissionUtilsFile, set_permissions_sets_permissions) +{ + EXPECT_CALL(mock_platform, set_permissions(test_qpath, test_permissions)).WillOnce(Return(true)); + + MP_PERMISSIONS.set_permissions(test_path, test_permissions); +} + +TEST_F(TestPermissionUtilsFile, set_permissions_throws_on_failure) +{ + EXPECT_CALL(mock_platform, set_permissions(test_qpath, test_permissions)).WillOnce(Return(false)); + + MP_EXPECT_THROW_THAT(MP_PERMISSIONS.set_permissions(test_path, test_permissions), + std::runtime_error, + mpt::match_what(AllOf(HasSubstr("Cannot set permissions"), HasSubstr(test_path.string())))); +} + +TEST_F(TestPermissionUtilsFile, take_ownership_takes_ownership) +{ + EXPECT_CALL(mock_platform, take_ownership(test_qpath, true)).WillOnce(Return(true)); + + MP_PERMISSIONS.take_ownership(test_path, true); +} + +TEST_F(TestPermissionUtilsFile, take_ownership_takes_user_ownership) +{ + EXPECT_CALL(mock_platform, take_ownership(test_qpath, false)).WillOnce(Return(true)); + + MP_PERMISSIONS.take_ownership(test_path, false); +} + +TEST_F(TestPermissionUtilsFile, take_ownership_throws_on_failure) +{ + EXPECT_CALL(mock_platform, take_ownership(test_qpath, _)).WillOnce(Return(false)); + + MP_EXPECT_THROW_THAT(MP_PERMISSIONS.take_ownership(test_path), + std::runtime_error, + mpt::match_what(AllOf(HasSubstr("Cannot set owner"), HasSubstr(test_path.string())))); +} + +TEST_F(TestPermissionUtilsFile, restrict_permissions_restricts_permissions) +{ + EXPECT_CALL(mock_platform, take_ownership(test_qpath, true)).WillOnce(Return(true)); + EXPECT_CALL(mock_platform, set_permissions(test_qpath, restricted_permissions)).WillOnce(Return(true)); + + MP_PERMISSIONS.restrict_permissions(test_path); +} + +struct TestPermissionUtilsDir : public TestPermissionUtils +{ + TestPermissionUtilsDir() + { + EXPECT_CALL(mock_file_ops, exists(test_path, _)).WillRepeatedly(Return(true)); + EXPECT_CALL(mock_file_ops, is_directory(test_path, _)).WillOnce(Return(true)); + + EXPECT_CALL(entry1, path).WillRepeatedly(ReturnRef(path1)); + EXPECT_CALL(entry2, path).WillRepeatedly(ReturnRef(path2)); + + auto iter = std::make_unique(); + auto iter_p = iter.get(); + + EXPECT_CALL(*iter_p, hasNext).WillOnce(Return(true)).WillOnce(Return(true)).WillRepeatedly(Return(false)); + EXPECT_CALL(*iter_p, next).WillOnce(ReturnRef(entry1)).WillOnce(ReturnRef(entry2)); + EXPECT_CALL(mock_file_ops, recursive_dir_iterator(test_path, _)).WillOnce(Return(std::move(iter))); + } + + const mp::PermissionUtils::Path path1{"Hello.txt"}; + const QString qpath1{QString::fromUtf8(path1.u8string())}; + mpt::MockDirectoryEntry entry1; + + const mp::PermissionUtils::Path path2{"World.txt"}; + const QString qpath2{QString::fromUtf8(path2.u8string())}; + mpt::MockDirectoryEntry entry2; +}; + +TEST_F(TestPermissionUtilsDir, set_permissions_iterates_dir) +{ + EXPECT_CALL(mock_platform, set_permissions(test_qpath, test_permissions)).WillOnce(Return(true)); + EXPECT_CALL(mock_platform, set_permissions(qpath1, test_permissions)).WillOnce(Return(true)); + EXPECT_CALL(mock_platform, set_permissions(qpath2, test_permissions)).WillOnce(Return(true)); + + MP_PERMISSIONS.set_permissions(test_path, test_permissions); +} + +TEST_F(TestPermissionUtilsDir, take_ownership_iterates_dir) +{ + EXPECT_CALL(mock_platform, take_ownership(test_qpath, false)).WillOnce(Return(true)); + EXPECT_CALL(mock_platform, take_ownership(qpath1, false)).WillOnce(Return(true)); + EXPECT_CALL(mock_platform, take_ownership(qpath2, false)).WillOnce(Return(true)); + + MP_PERMISSIONS.take_ownership(test_path, false); +} + +TEST_F(TestPermissionUtilsDir, restrict_permissions_iterates_dir) +{ + EXPECT_CALL(mock_platform, take_ownership(test_qpath, true)).WillOnce(Return(true)); + EXPECT_CALL(mock_platform, take_ownership(qpath1, true)).WillOnce(Return(true)); + EXPECT_CALL(mock_platform, take_ownership(qpath2, true)).WillOnce(Return(true)); + + EXPECT_CALL(mock_platform, set_permissions(test_qpath, restricted_permissions)).WillOnce(Return(true)); + EXPECT_CALL(mock_platform, set_permissions(qpath1, restricted_permissions)).WillOnce(Return(true)); + EXPECT_CALL(mock_platform, set_permissions(qpath2, restricted_permissions)).WillOnce(Return(true)); + + MP_PERMISSIONS.restrict_permissions(test_path); +} + +struct TestPermissionUtilsBadDir : public TestPermissionUtils +{ + TestPermissionUtilsBadDir() + { + EXPECT_CALL(mock_file_ops, exists(test_path, _)).WillRepeatedly(Return(true)); + EXPECT_CALL(mock_file_ops, is_directory(test_path, _)).WillOnce(Return(true)); + + EXPECT_CALL(mock_file_ops, recursive_dir_iterator(test_path, _)).WillOnce(Return(nullptr)); + } +}; + +TEST_F(TestPermissionUtilsBadDir, set_permissions_throws_on_broken_iterator) +{ + EXPECT_CALL(mock_platform, set_permissions(test_qpath, test_permissions)).WillOnce(Return(true)); + + MP_EXPECT_THROW_THAT(MP_PERMISSIONS.set_permissions(test_path, test_permissions), + std::runtime_error, + mpt::match_what(AllOf(HasSubstr("Cannot iterate"), HasSubstr(test_path.string())))); +} + +TEST_F(TestPermissionUtilsBadDir, take_ownership_throws_on_broken_iterator) +{ + EXPECT_CALL(mock_platform, take_ownership(test_qpath, false)).WillOnce(Return(true)); + + MP_EXPECT_THROW_THAT(MP_PERMISSIONS.take_ownership(test_path, false), + std::runtime_error, + mpt::match_what(AllOf(HasSubstr("Cannot iterate"), HasSubstr(test_path.string())))); +} + +TEST_F(TestPermissionUtilsBadDir, restrict_permissions_throws_on_broken_iterator) +{ + EXPECT_CALL(mock_platform, set_permissions(test_qpath, restricted_permissions)).WillOnce(Return(true)); + EXPECT_CALL(mock_platform, take_ownership(test_qpath, true)).WillOnce(Return(true)); + + MP_EXPECT_THROW_THAT(MP_PERMISSIONS.restrict_permissions(test_path), + std::runtime_error, + mpt::match_what(AllOf(HasSubstr("Cannot iterate"), HasSubstr(test_path.string())))); +} + +} // namespace From 6ee71ea33b65622d240153d65982a8649916d0c4 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Wed, 4 Dec 2024 09:22:30 -0500 Subject: [PATCH 06/12] [daemon] Restrict permissions on data and cache dirs --- src/daemon/daemon_config.cpp | 11 +++++++++ src/daemon/default_vm_image_vault.cpp | 4 ++-- tests/lxd/test_lxd_image_vault.cpp | 2 +- tests/test_alias_dict.cpp | 5 ++++ tests/test_client_common.cpp | 5 ++++ tests/test_daemon.cpp | 32 ++++++++++++++++++++++++++ tests/test_daemon_authenticate.cpp | 5 ++++ tests/test_daemon_clone.cpp | 5 ++++ tests/test_daemon_find.cpp | 5 ++++ tests/test_daemon_launch.cpp | 5 ++++ tests/test_daemon_mount.cpp | 5 ++++ tests/test_daemon_snapshot_restore.cpp | 5 ++++ tests/test_daemon_start.cpp | 5 ++++ tests/test_daemon_suspend.cpp | 5 ++++ tests/test_daemon_umount.cpp | 5 ++++ tests/unix/test_daemon_rpc.cpp | 5 ++++ 16 files changed, 106 insertions(+), 3 deletions(-) diff --git a/src/daemon/daemon_config.cpp b/src/daemon/daemon_config.cpp index 1df1e5bcba..fdaa088516 100644 --- a/src/daemon/daemon_config.cpp +++ b/src/daemon/daemon_config.cpp @@ -31,6 +31,7 @@ #include #include #include +#include #include #include @@ -189,6 +190,16 @@ std::unique_ptr mp::DaemonConfigBuilder::build() std::make_unique(url_downloader.get(), cache_directory, manifest_ttl); } + if (!storage_path.isEmpty()) + { + MP_PERMISSIONS.restrict_permissions(storage_path.toStdU16String()); + } + else + { + MP_PERMISSIONS.restrict_permissions(data_directory.toStdU16String()); + MP_PERMISSIONS.restrict_permissions(cache_directory.toStdU16String()); + } + return std::unique_ptr(new DaemonConfig{ std::move(url_downloader), std::move(factory), std::move(image_hosts), std::move(vault), std::move(name_generator), std::move(ssh_key_provider), std::move(cert_provider), std::move(client_cert_store), diff --git a/src/daemon/default_vm_image_vault.cpp b/src/daemon/default_vm_image_vault.cpp index 1dd6c8b729..a57778b767 100644 --- a/src/daemon/default_vm_image_vault.cpp +++ b/src/daemon/default_vm_image_vault.cpp @@ -671,7 +671,7 @@ QString mp::DefaultVMImageVault::extract_image_from(const VMImage& source_image, const mp::Path& dest_dir) { MP_UTILS.make_dir(dest_dir, QFile::ReadOwner | QFile::WriteOwner); - MP_PERMISSIONS.take_ownership(dest_dir.toStdString()); + MP_PERMISSIONS.take_ownership(dest_dir.toStdU16String()); QFileInfo file_info{source_image.image_path}; const auto image_name = file_info.fileName().remove(".xz"); @@ -683,7 +683,7 @@ QString mp::DefaultVMImageVault::extract_image_from(const VMImage& source_image, mp::VMImage mp::DefaultVMImageVault::image_instance_from(const VMImage& prepared_image, const mp::Path& dest_dir) { MP_UTILS.make_dir(dest_dir, QFile::ReadOwner | QFile::WriteOwner | QFile::ExeOwner); - MP_PERMISSIONS.take_ownership(dest_dir.toStdString()); + MP_PERMISSIONS.take_ownership(dest_dir.toStdU16String()); return {mp::vault::copy(prepared_image.image_path, dest_dir), prepared_image.id, diff --git a/tests/lxd/test_lxd_image_vault.cpp b/tests/lxd/test_lxd_image_vault.cpp index 2fcb6f8906..2bf545c12d 100644 --- a/tests/lxd/test_lxd_image_vault.cpp +++ b/tests/lxd/test_lxd_image_vault.cpp @@ -61,7 +61,7 @@ struct LXDImageVault : public Test mpt::MockLogger::Scope logger_scope = mpt::MockLogger::inject(); std::unique_ptr> mock_network_access_manager; - mpt::MockPermissionUtils::GuardedMock mock_permission_utils_injection = + const mpt::MockPermissionUtils::GuardedMock mock_permission_utils_injection = mpt::MockPermissionUtils::inject(); mpt::MockPermissionUtils& mock_permission_utils = *mock_permission_utils_injection.first; std::vector hosts; diff --git a/tests/test_alias_dict.cpp b/tests/test_alias_dict.cpp index b8cb38559d..4f1cb88109 100644 --- a/tests/test_alias_dict.cpp +++ b/tests/test_alias_dict.cpp @@ -28,6 +28,7 @@ #include "file_operations.h" #include "json_test_utils.h" #include "mock_file_ops.h" +#include "mock_permission_utils.h" #include "mock_platform.h" #include "mock_settings.h" #include "mock_vm_image_vault.h" @@ -607,6 +608,10 @@ struct DaemonAliasTestsuite mpt::MockSettings::GuardedMock mock_settings_injection = mpt::MockSettings::inject(); mpt::MockSettings& mock_settings = *mock_settings_injection.first; + + const mpt::MockPermissionUtils::GuardedMock mock_permission_utils_injection = + mpt::MockPermissionUtils::inject(); + mpt::MockPermissionUtils& mock_permission_utils = *mock_permission_utils_injection.first; }; TEST_P(DaemonAliasTestsuite, purge_removes_purged_instance_aliases_and_scripts) diff --git a/tests/test_client_common.cpp b/tests/test_client_common.cpp index c02ff554bb..2165eb9a03 100644 --- a/tests/test_client_common.cpp +++ b/tests/test_client_common.cpp @@ -22,6 +22,7 @@ #include "mock_cert_store.h" #include "mock_client_rpc.h" #include "mock_daemon.h" +#include "mock_permission_utils.h" #include "mock_standard_paths.h" #include "mock_utils.h" #include "stub_terminal.h" @@ -60,6 +61,10 @@ struct TestClientCommon : public mpt::DaemonTestFixture std::make_unique>()}; std::unique_ptr mock_cert_store{std::make_unique()}; + const mpt::MockPermissionUtils::GuardedMock mock_permission_utils_injection = + mpt::MockPermissionUtils::inject(); + mpt::MockPermissionUtils& mock_permission_utils = *mock_permission_utils_injection.first; + const std::string server_address{"localhost:50052"}; mpt::TempDir temp_dir; }; diff --git a/tests/test_daemon.cpp b/tests/test_daemon.cpp index 8f5479ff95..1ee3ccaadc 100644 --- a/tests/test_daemon.cpp +++ b/tests/test_daemon.cpp @@ -27,6 +27,7 @@ #include "mock_image_host.h" #include "mock_json_utils.h" #include "mock_logger.h" +#include "mock_permission_utils.h" #include "mock_platform.h" #include "mock_server_reader_writer.h" #include "mock_settings.h" @@ -139,6 +140,10 @@ struct Daemon : public mpt::DaemonTestFixture mpt::MockSettings::GuardedMock mock_settings_injection = mpt::MockSettings::inject(); mpt::MockSettings& mock_settings = *mock_settings_injection.first; + const mpt::MockPermissionUtils::GuardedMock mock_permission_utils_injection = + mpt::MockPermissionUtils::inject(); + mpt::MockPermissionUtils& mock_permission_utils = *mock_permission_utils_injection.first; + const mpt::MockJsonUtils::GuardedMock mock_json_utils_injection = mpt::MockJsonUtils::inject(); mpt::MockJsonUtils& mock_json_utils = *mock_json_utils_injection.first; }; @@ -2519,4 +2524,31 @@ TEST_F(Daemon, info_all_returns_all_instances) mp::Daemon daemon{config_builder.build()}; call_daemon_slot(daemon, &mp::Daemon::info, mp::InfoRequest{}, mock_server); } + +TEST_F(Daemon, sets_permissions_on_provided_storage_path) +{ + const QString path{"Where all the secrets go"}; + const std::filesystem::path std_path{path.toStdU16String()}; + + EXPECT_CALL(mock_platform, multipass_storage_location()).WillOnce(Return(path)); + EXPECT_CALL(mock_permission_utils, restrict_permissions(std_path)); + + config_builder.build(); +} + +TEST_F(Daemon, sets_permissions_on_storage_dirs) +{ + + config_builder.data_directory = "Sensitive data location"; + const std::filesystem::path std_data_path{config_builder.data_directory.toStdU16String()}; + + config_builder.cache_directory = "Pirate's secret cache"; + const std::filesystem::path std_cache_path{config_builder.cache_directory.toStdU16String()}; + + EXPECT_CALL(mock_permission_utils, restrict_permissions(std_data_path)); + EXPECT_CALL(mock_permission_utils, restrict_permissions(std_cache_path)); + + config_builder.build(); +} + } // namespace diff --git a/tests/test_daemon_authenticate.cpp b/tests/test_daemon_authenticate.cpp index 60fd37e6ed..fbbc172fe7 100644 --- a/tests/test_daemon_authenticate.cpp +++ b/tests/test_daemon_authenticate.cpp @@ -17,6 +17,7 @@ #include "common.h" #include "daemon_test_fixture.h" +#include "mock_permission_utils.h" #include "mock_platform.h" #include "mock_server_reader_writer.h" #include "mock_settings.h" @@ -50,6 +51,10 @@ struct TestDaemonAuthenticate : public mpt::DaemonTestFixture mpt::MockSettings::GuardedMock mock_settings_injection = mpt::MockSettings::inject(); mpt::MockSettings& mock_settings = *mock_settings_injection.first; + + const mpt::MockPermissionUtils::GuardedMock mock_permission_utils_injection = + mpt::MockPermissionUtils::inject(); + mpt::MockPermissionUtils& mock_permission_utils = *mock_permission_utils_injection.first; }; } // namespace diff --git a/tests/test_daemon_clone.cpp b/tests/test_daemon_clone.cpp index 55026ece62..f77f0fd615 100644 --- a/tests/test_daemon_clone.cpp +++ b/tests/test_daemon_clone.cpp @@ -17,6 +17,7 @@ #include "common.h" #include "daemon_test_fixture.h" +#include "mock_permission_utils.h" #include "mock_platform.h" #include "mock_server_reader_writer.h" #include "mock_virtual_machine.h" @@ -57,6 +58,10 @@ struct TestDaemonClone : public mpt::DaemonTestFixture const mpt::MockPlatform::GuardedMock attr{mpt::MockPlatform::inject()}; const mpt::MockPlatform& mock_platform = *attr.first; + + const mpt::MockPermissionUtils::GuardedMock mock_permission_utils_injection = + mpt::MockPermissionUtils::inject(); + mpt::MockPermissionUtils& mock_permission_utils = *mock_permission_utils_injection.first; }; TEST_F(TestDaemonClone, missingOnSrcInstance) diff --git a/tests/test_daemon_find.cpp b/tests/test_daemon_find.cpp index b8077f8829..f6f54262fd 100644 --- a/tests/test_daemon_find.cpp +++ b/tests/test_daemon_find.cpp @@ -18,6 +18,7 @@ #include "common.h" #include "daemon_test_fixture.h" #include "mock_image_host.h" +#include "mock_permission_utils.h" #include "mock_platform.h" #include "mock_settings.h" #include "mock_vm_blueprint_provider.h" @@ -55,6 +56,10 @@ struct DaemonFind : public mpt::DaemonTestFixture mpt::MockSettings::GuardedMock mock_settings_injection = mpt::MockSettings::inject(); mpt::MockSettings& mock_settings = *mock_settings_injection.first; + + const mpt::MockPermissionUtils::GuardedMock mock_permission_utils_injection = + mpt::MockPermissionUtils::inject(); + mpt::MockPermissionUtils& mock_permission_utils = *mock_permission_utils_injection.first; }; TEST_F(DaemonFind, blankQueryReturnsAllData) diff --git a/tests/test_daemon_launch.cpp b/tests/test_daemon_launch.cpp index 566229ed4d..ca3d14861d 100644 --- a/tests/test_daemon_launch.cpp +++ b/tests/test_daemon_launch.cpp @@ -19,6 +19,7 @@ #include "common.h" #include "daemon_test_fixture.h" #include "mock_image_host.h" +#include "mock_permission_utils.h" #include "mock_json_utils.h" #include "mock_platform.h" #include "mock_server_reader_writer.h" @@ -54,6 +55,10 @@ struct TestDaemonLaunch : public mpt::DaemonTestFixture mpt::MockSettings& mock_settings = *mock_settings_injection.first; const mpt::MockJsonUtils::GuardedMock mock_json_utils_injection = mpt::MockJsonUtils::inject(); + + const mpt::MockPermissionUtils::GuardedMock mock_permission_utils_injection = + mpt::MockPermissionUtils::inject(); + mpt::MockPermissionUtils& mock_permission_utils = *mock_permission_utils_injection.first; }; TEST_F(TestDaemonLaunch, blueprintFoundMountsWorkspaceWithNameOverride) diff --git a/tests/test_daemon_mount.cpp b/tests/test_daemon_mount.cpp index 2972fc3149..cc392d644c 100644 --- a/tests/test_daemon_mount.cpp +++ b/tests/test_daemon_mount.cpp @@ -20,6 +20,7 @@ #include "mock_file_ops.h" #include "mock_logger.h" #include "mock_mount_handler.h" +#include "mock_permission_utils.h" #include "mock_platform.h" #include "mock_server_reader_writer.h" #include "mock_settings.h" @@ -68,6 +69,10 @@ struct TestDaemonMount : public mpt::DaemonTestFixture mpt::MockSettings::GuardedMock mock_settings_injection = mpt::MockSettings::inject(); mpt::MockSettings& mock_settings = *mock_settings_injection.first; + + const mpt::MockPermissionUtils::GuardedMock mock_permission_utils_injection = + mpt::MockPermissionUtils::inject(); + mpt::MockPermissionUtils& mock_permission_utils = *mock_permission_utils_injection.first; }; } // namespace diff --git a/tests/test_daemon_snapshot_restore.cpp b/tests/test_daemon_snapshot_restore.cpp index 7474b65148..6b83119e08 100644 --- a/tests/test_daemon_snapshot_restore.cpp +++ b/tests/test_daemon_snapshot_restore.cpp @@ -17,6 +17,7 @@ #include "common.h" #include "daemon_test_fixture.h" +#include "mock_permission_utils.h" #include "mock_platform.h" #include "mock_server_reader_writer.h" #include "mock_settings.h" @@ -65,6 +66,10 @@ struct TestDaemonSnapshotRestoreBase : public mpt::DaemonTestFixture mpt::MockSettings::GuardedMock mock_settings_injection = mpt::MockSettings::inject(); mpt::MockSettings& mock_settings = *mock_settings_injection.first; + const mpt::MockPermissionUtils::GuardedMock mock_permission_utils_injection = + mpt::MockPermissionUtils::inject(); + mpt::MockPermissionUtils& mock_permission_utils = *mock_permission_utils_injection.first; + mpt::MockVirtualMachineFactory& mock_factory = *use_a_mock_vm_factory(); std::vector extra_interfaces; diff --git a/tests/test_daemon_start.cpp b/tests/test_daemon_start.cpp index dc2fced146..fff214a31d 100644 --- a/tests/test_daemon_start.cpp +++ b/tests/test_daemon_start.cpp @@ -21,6 +21,7 @@ #include "common.h" #include "mock_image_host.h" #include "mock_mount_handler.h" +#include "mock_permission_utils.h" #include "mock_platform.h" #include "mock_server_reader_writer.h" #include "mock_settings.h" @@ -54,6 +55,10 @@ struct TestDaemonStart : public mpt::DaemonTestFixture mpt::MockSettings::GuardedMock mock_settings_injection = mpt::MockSettings::inject(); mpt::MockSettings& mock_settings = *mock_settings_injection.first; + + const mpt::MockPermissionUtils::GuardedMock mock_permission_utils_injection = + mpt::MockPermissionUtils::inject(); + mpt::MockPermissionUtils& mock_permission_utils = *mock_permission_utils_injection.first; }; TEST_F(TestDaemonStart, successfulStartOkStatus) diff --git a/tests/test_daemon_suspend.cpp b/tests/test_daemon_suspend.cpp index 98e8a0fe71..b0dc27672a 100644 --- a/tests/test_daemon_suspend.cpp +++ b/tests/test_daemon_suspend.cpp @@ -18,6 +18,7 @@ #include "common.h" #include "daemon_test_fixture.h" #include "mock_mount_handler.h" +#include "mock_permission_utils.h" #include "mock_platform.h" #include "mock_server_reader_writer.h" #include "mock_settings.h" @@ -54,6 +55,10 @@ struct TestDaemonSuspend : public mpt::DaemonTestFixture mpt::MockSettings::GuardedMock mock_settings_injection = mpt::MockSettings::inject(); mpt::MockSettings& mock_settings = *mock_settings_injection.first; + + const mpt::MockPermissionUtils::GuardedMock mock_permission_utils_injection = + mpt::MockPermissionUtils::inject(); + mpt::MockPermissionUtils& mock_permission_utils = *mock_permission_utils_injection.first; }; } // namespace diff --git a/tests/test_daemon_umount.cpp b/tests/test_daemon_umount.cpp index 69705432be..dbd7cfdfeb 100644 --- a/tests/test_daemon_umount.cpp +++ b/tests/test_daemon_umount.cpp @@ -19,6 +19,7 @@ #include "daemon_test_fixture.h" #include "mock_logger.h" #include "mock_mount_handler.h" +#include "mock_permission_utils.h" #include "mock_platform.h" #include "mock_server_reader_writer.h" #include "mock_settings.h" @@ -57,6 +58,10 @@ struct TestDaemonUmount : public mpt::DaemonTestFixture mpt::MockSettings::GuardedMock mock_settings_injection = mpt::MockSettings::inject(); mpt::MockSettings& mock_settings = *mock_settings_injection.first; + + const mpt::MockPermissionUtils::GuardedMock mock_permission_utils_injection = + mpt::MockPermissionUtils::inject(); + mpt::MockPermissionUtils& mock_permission_utils = *mock_permission_utils_injection.first; }; } // namespace diff --git a/tests/unix/test_daemon_rpc.cpp b/tests/unix/test_daemon_rpc.cpp index 77e66a7420..7ec3b86af5 100644 --- a/tests/unix/test_daemon_rpc.cpp +++ b/tests/unix/test_daemon_rpc.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include @@ -98,6 +99,10 @@ struct TestDaemonRpc : public mpt::DaemonTestFixture mpt::MockUtils::GuardedMock attr{mpt::MockUtils::inject()}; mpt::MockUtils* mock_utils = attr.first; + + const mpt::MockPermissionUtils::GuardedMock mock_permission_utils_injection = + mpt::MockPermissionUtils::inject(); + mpt::MockPermissionUtils& mock_permission_utils = *mock_permission_utils_injection.first; }; } // namespace From 81f0a32fc6f727852b000a0f1fd8ed51c83cad0c Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Wed, 4 Dec 2024 11:30:04 -0500 Subject: [PATCH 07/12] [permissions] Remove root option from take ownership --- include/multipass/platform.h | 2 +- include/multipass/utils/permission_utils.h | 2 +- src/platform/platform_unix.cpp | 9 ++--- src/utils/permission_utils.cpp | 12 +++---- tests/mock_permission_utils.h | 2 +- tests/mock_platform.h | 2 +- tests/test_permission_utils.cpp | 41 +++++++++------------- 7 files changed, 28 insertions(+), 42 deletions(-) diff --git a/include/multipass/platform.h b/include/multipass/platform.h index 461a2bac5a..041c923388 100644 --- a/include/multipass/platform.h +++ b/include/multipass/platform.h @@ -59,7 +59,7 @@ class Platform : public Singleton virtual bool is_backend_supported(const QString& backend) const; // temporary (?) virtual int chown(const char* path, unsigned int uid, unsigned int gid) const; virtual bool set_permissions(const Path& path, const Perms permissions) const; - virtual bool take_ownership(const Path& path, bool root = true) const; + virtual bool take_ownership(const Path& path) const; virtual bool link(const char* target, const char* link) const; virtual bool symlink(const char* target, const char* link, bool is_dir) const; virtual int utime(const char* path, int atime, int mtime) const; diff --git a/include/multipass/utils/permission_utils.h b/include/multipass/utils/permission_utils.h index 6bb35a56bd..5241fec24c 100644 --- a/include/multipass/utils/permission_utils.h +++ b/include/multipass/utils/permission_utils.h @@ -36,7 +36,7 @@ class PermissionUtils : public Singleton PermissionUtils(const PrivatePass&) noexcept; virtual void set_permissions(const Path& path, const QFileDevice::Permissions& permissions) const; - virtual void take_ownership(const Path& path, bool root = true) const; + virtual void take_ownership(const Path& path) const; // sets owner to root and sets permissions such that only owner has access. virtual void restrict_permissions(const Path& path) const; diff --git a/src/platform/platform_unix.cpp b/src/platform/platform_unix.cpp index eb68a41a11..1d69368e73 100644 --- a/src/platform/platform_unix.cpp +++ b/src/platform/platform_unix.cpp @@ -63,14 +63,9 @@ bool mp::platform::Platform::set_permissions(const Path& path, const Perms permi return QFile::setPermissions(path, permissions); } -bool mp::platform::Platform::take_ownership(const mp::Path& path, bool root) const +bool mp::platform::Platform::take_ownership(const mp::Path& path) const { - if (root) - { - return this->chown(path.toStdString().c_str(), 0, 0) == 0; - } - - throw std::logic_error("NYI"); + return this->chown(path.toStdString().c_str(), 0, 0) == 0; } bool mp::platform::Platform::symlink(const char* target, const char* link, bool is_dir) const diff --git a/src/utils/permission_utils.cpp b/src/utils/permission_utils.cpp index 31004e9ff2..9ee79cb457 100644 --- a/src/utils/permission_utils.cpp +++ b/src/utils/permission_utils.cpp @@ -35,11 +35,11 @@ void set_single_permissions(const Path& path, const QFileDevice::Permissions& pe throw std::runtime_error(fmt::format("Cannot set permissions for '{}'", path.string())); } -void set_single_owner(const Path& path, bool root) +void set_single_owner(const Path& path) { QString qpath = QString::fromUtf8(path.u8string()); - if (!MP_PLATFORM.take_ownership(qpath, root)) + if (!MP_PLATFORM.take_ownership(qpath)) throw std::runtime_error(fmt::format("Cannot set owner for '{}'", path.string())); } @@ -93,15 +93,15 @@ void mp::PermissionUtils::set_permissions(const Path& path, const QFileDevice::P apply_on_files(path, [&](const fs::path& apply_path) { set_single_permissions(apply_path, permissions); }); } -void mp::PermissionUtils::take_ownership(const Path& path, bool root) const +void mp::PermissionUtils::take_ownership(const Path& path) const { - apply_on_files(path, [&](const fs::path& apply_path) { set_single_owner(apply_path, root); }); + apply_on_files(path, [&](const fs::path& apply_path) { set_single_owner(apply_path); }); } void mp::PermissionUtils::restrict_permissions(const Path& path) const { apply_on_files(path, [&](const fs::path& apply_path) { - set_single_owner(apply_path, true); - set_single_permissions(apply_path, QFile::ReadOwner | QFile::WriteOwner); + set_single_owner(apply_path); + set_single_permissions(apply_path, QFile::ReadOwner | QFile::WriteOwner | QFile::ExeOwner); }); } diff --git a/tests/mock_permission_utils.h b/tests/mock_permission_utils.h index 0d630a19d5..e0973c9a27 100644 --- a/tests/mock_permission_utils.h +++ b/tests/mock_permission_utils.h @@ -37,7 +37,7 @@ class MockPermissionUtils : public PermissionUtils set_permissions, (const Path& path, const QFileDevice::Permissions& permissions), (const, override)); - MOCK_METHOD(void, take_ownership, (const Path& path, bool root), (const, override)); + MOCK_METHOD(void, take_ownership, (const Path& path), (const, override)); MOCK_METHOD(void, restrict_permissions, (const Path& path), (const, override)); MP_MOCK_SINGLETON_BOILERPLATE(MockPermissionUtils, PermissionUtils); diff --git a/tests/mock_platform.h b/tests/mock_platform.h index fe6d0a6f23..3a3e0385b4 100644 --- a/tests/mock_platform.h +++ b/tests/mock_platform.h @@ -46,7 +46,7 @@ class MockPlatform : public platform::Platform set_permissions, (const multipass::Path& path, const QFileDevice::Permissions), (const, override)); - MOCK_METHOD(bool, take_ownership, (const multipass::Path& path, bool root), (const, override)); + MOCK_METHOD(bool, take_ownership, (const multipass::Path& path), (const, override)); MOCK_METHOD(bool, link, (const char*, const char*), (const, override)); MOCK_METHOD(bool, symlink, (const char*, const char*, bool), (const, override)); MOCK_METHOD(int, utime, (const char*, int, int), (const, override)); diff --git a/tests/test_permission_utils.cpp b/tests/test_permission_utils.cpp index ae132b93ee..cd8374f1d6 100644 --- a/tests/test_permission_utils.cpp +++ b/tests/test_permission_utils.cpp @@ -15,8 +15,6 @@ * */ -#include - #include "mock_file_ops.h" #include "mock_permission_utils.h" #include "mock_platform.h" @@ -38,8 +36,8 @@ struct TestPermissionUtils : public Test const mp::PermissionUtils::Path test_path{"test_path"}; const QString test_qpath{QString::fromUtf8(test_path.u8string())}; static constexpr QFileDevice::Permissions test_permissions{QFileDevice::Permission::ReadOwner}; - static constexpr QFileDevice::Permissions restricted_permissions{QFileDevice::Permission::ReadOwner | - QFileDevice::Permission::WriteOwner}; + static constexpr QFileDevice::Permissions restricted_permissions{ + QFileDevice::Permission::ReadOwner | QFileDevice::Permission::WriteOwner | QFile::ExeOwner}; }; struct TestPermissionUtilsNoFile : public TestPermissionUtils @@ -98,21 +96,14 @@ TEST_F(TestPermissionUtilsFile, set_permissions_throws_on_failure) TEST_F(TestPermissionUtilsFile, take_ownership_takes_ownership) { - EXPECT_CALL(mock_platform, take_ownership(test_qpath, true)).WillOnce(Return(true)); - - MP_PERMISSIONS.take_ownership(test_path, true); -} - -TEST_F(TestPermissionUtilsFile, take_ownership_takes_user_ownership) -{ - EXPECT_CALL(mock_platform, take_ownership(test_qpath, false)).WillOnce(Return(true)); + EXPECT_CALL(mock_platform, take_ownership(test_qpath)).WillOnce(Return(true)); - MP_PERMISSIONS.take_ownership(test_path, false); + MP_PERMISSIONS.take_ownership(test_path); } TEST_F(TestPermissionUtilsFile, take_ownership_throws_on_failure) { - EXPECT_CALL(mock_platform, take_ownership(test_qpath, _)).WillOnce(Return(false)); + EXPECT_CALL(mock_platform, take_ownership(test_qpath)).WillOnce(Return(false)); MP_EXPECT_THROW_THAT(MP_PERMISSIONS.take_ownership(test_path), std::runtime_error, @@ -121,7 +112,7 @@ TEST_F(TestPermissionUtilsFile, take_ownership_throws_on_failure) TEST_F(TestPermissionUtilsFile, restrict_permissions_restricts_permissions) { - EXPECT_CALL(mock_platform, take_ownership(test_qpath, true)).WillOnce(Return(true)); + EXPECT_CALL(mock_platform, take_ownership(test_qpath)).WillOnce(Return(true)); EXPECT_CALL(mock_platform, set_permissions(test_qpath, restricted_permissions)).WillOnce(Return(true)); MP_PERMISSIONS.restrict_permissions(test_path); @@ -165,18 +156,18 @@ TEST_F(TestPermissionUtilsDir, set_permissions_iterates_dir) TEST_F(TestPermissionUtilsDir, take_ownership_iterates_dir) { - EXPECT_CALL(mock_platform, take_ownership(test_qpath, false)).WillOnce(Return(true)); - EXPECT_CALL(mock_platform, take_ownership(qpath1, false)).WillOnce(Return(true)); - EXPECT_CALL(mock_platform, take_ownership(qpath2, false)).WillOnce(Return(true)); + EXPECT_CALL(mock_platform, take_ownership(test_qpath)).WillOnce(Return(true)); + EXPECT_CALL(mock_platform, take_ownership(qpath1)).WillOnce(Return(true)); + EXPECT_CALL(mock_platform, take_ownership(qpath2)).WillOnce(Return(true)); - MP_PERMISSIONS.take_ownership(test_path, false); + MP_PERMISSIONS.take_ownership(test_path); } TEST_F(TestPermissionUtilsDir, restrict_permissions_iterates_dir) { - EXPECT_CALL(mock_platform, take_ownership(test_qpath, true)).WillOnce(Return(true)); - EXPECT_CALL(mock_platform, take_ownership(qpath1, true)).WillOnce(Return(true)); - EXPECT_CALL(mock_platform, take_ownership(qpath2, true)).WillOnce(Return(true)); + EXPECT_CALL(mock_platform, take_ownership(test_qpath)).WillOnce(Return(true)); + EXPECT_CALL(mock_platform, take_ownership(qpath1)).WillOnce(Return(true)); + EXPECT_CALL(mock_platform, take_ownership(qpath2)).WillOnce(Return(true)); EXPECT_CALL(mock_platform, set_permissions(test_qpath, restricted_permissions)).WillOnce(Return(true)); EXPECT_CALL(mock_platform, set_permissions(qpath1, restricted_permissions)).WillOnce(Return(true)); @@ -207,9 +198,9 @@ TEST_F(TestPermissionUtilsBadDir, set_permissions_throws_on_broken_iterator) TEST_F(TestPermissionUtilsBadDir, take_ownership_throws_on_broken_iterator) { - EXPECT_CALL(mock_platform, take_ownership(test_qpath, false)).WillOnce(Return(true)); + EXPECT_CALL(mock_platform, take_ownership(test_qpath)).WillOnce(Return(true)); - MP_EXPECT_THROW_THAT(MP_PERMISSIONS.take_ownership(test_path, false), + MP_EXPECT_THROW_THAT(MP_PERMISSIONS.take_ownership(test_path), std::runtime_error, mpt::match_what(AllOf(HasSubstr("Cannot iterate"), HasSubstr(test_path.string())))); } @@ -217,7 +208,7 @@ TEST_F(TestPermissionUtilsBadDir, take_ownership_throws_on_broken_iterator) TEST_F(TestPermissionUtilsBadDir, restrict_permissions_throws_on_broken_iterator) { EXPECT_CALL(mock_platform, set_permissions(test_qpath, restricted_permissions)).WillOnce(Return(true)); - EXPECT_CALL(mock_platform, take_ownership(test_qpath, true)).WillOnce(Return(true)); + EXPECT_CALL(mock_platform, take_ownership(test_qpath)).WillOnce(Return(true)); MP_EXPECT_THROW_THAT(MP_PERMISSIONS.restrict_permissions(test_path), std::runtime_error, From 345b38e819eb04bddfb170998cce54be6d5e1d55 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Wed, 4 Dec 2024 14:06:15 -0500 Subject: [PATCH 08/12] [tests] Change utils to use permission utils --- src/utils/utils.cpp | 3 ++- tests/test_image_vault.cpp | 14 ++++---------- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/src/utils/utils.cpp b/src/utils/utils.cpp index 5dacfd8dc7..2b73926517 100644 --- a/src/utils/utils.cpp +++ b/src/utils/utils.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include @@ -303,7 +304,7 @@ mp::Path mp::Utils::make_dir(const QDir& a_dir, const QString& name, QFileDevice if (permissions) { - MP_PLATFORM.set_permissions(dir_path, permissions); + MP_PERMISSIONS.set_permissions(dir_path.toStdU16String(), permissions); } return dir_path; diff --git a/tests/test_image_vault.cpp b/tests/test_image_vault.cpp index 77d5ab5dcd..e0744b69f6 100644 --- a/tests/test_image_vault.cpp +++ b/tests/test_image_vault.cpp @@ -168,7 +168,7 @@ struct ImageVault : public testing::Test NiceMock host; mpt::MockJsonUtils::GuardedMock mock_json_utils_injection = mpt::MockJsonUtils::inject(); mpt::MockJsonUtils& mock_json_utils = *mock_json_utils_injection.first; - mpt::MockPermissionUtils::GuardedMock mock_permission_utils_injection = + const mpt::MockPermissionUtils::GuardedMock mock_permission_utils_injection = mpt::MockPermissionUtils::inject(); mpt::MockPermissionUtils& mock_permission_utils = *mock_permission_utils_injection.first; mp::ProgressMonitor stub_monitor{[](int, int) { return true; }}; @@ -418,9 +418,7 @@ TEST_F(ImageVault, remembers_prepared_images) TEST_F(ImageVault, uses_image_from_prepare) { - auto [mock_permission_utils, permission_utils_guard] = mpt::MockPermissionUtils::inject(); - - EXPECT_CALL(*mock_permission_utils, restrict_permissions(_)); + EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); constexpr auto expected_data = "12345-pied-piper-rats"; @@ -517,9 +515,7 @@ TEST_F(ImageVault, invalid_image_dir_is_removed) TEST_F(ImageVault, DISABLE_ON_WINDOWS_AND_MACOS(file_based_fetch_copies_image_and_returns_expected_info)) { - auto [mock_permission_utils, permission_utils_guard] = mpt::MockPermissionUtils::inject(); - - EXPECT_CALL(*mock_permission_utils, restrict_permissions(_)); + EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); mpt::TempFile file; mp::DefaultVMImageVault vault{hosts, &url_downloader, cache_dir.path(), data_dir.path(), mp::days{0}}; @@ -752,9 +748,7 @@ TEST_F(ImageVault, minimum_image_size_returns_expected_size) TEST_F(ImageVault, DISABLE_ON_WINDOWS_AND_MACOS(file_based_minimum_size_returns_expected_size)) { - auto [mock_permission_utils, permission_utils_guard] = mpt::MockPermissionUtils::inject(); - - EXPECT_CALL(*mock_permission_utils, restrict_permissions(_)); + EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); const mp::MemorySize image_size{"2097152"}; const mp::ProcessState qemuimg_exit_status{0, std::nullopt}; From d88b055b3c0e01924fee54cf9802df5a19f6f2a7 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Wed, 4 Dec 2024 14:24:09 -0500 Subject: [PATCH 09/12] [format] Fix formatting broken by rebase conflicts --- tests/test_daemon_launch.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_daemon_launch.cpp b/tests/test_daemon_launch.cpp index ca3d14861d..9a0c66a799 100644 --- a/tests/test_daemon_launch.cpp +++ b/tests/test_daemon_launch.cpp @@ -19,8 +19,8 @@ #include "common.h" #include "daemon_test_fixture.h" #include "mock_image_host.h" -#include "mock_permission_utils.h" #include "mock_json_utils.h" +#include "mock_permission_utils.h" #include "mock_platform.h" #include "mock_server_reader_writer.h" #include "mock_settings.h" From bc08d4bab4e0b5318c93b815074e167be4c66c1f Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Thu, 12 Dec 2024 12:05:27 -0500 Subject: [PATCH 10/12] Address some review comments --- include/multipass/utils/permission_utils.h | 9 +++-- src/daemon/default_vm_image_vault.cpp | 6 ++-- src/iso/cloud_init_iso.cpp | 2 -- .../backends/lxd/lxd_vm_image_vault.cpp | 2 -- src/utils/permission_utils.cpp | 19 +++++----- src/utils/vm_image_vault_utils.cpp | 4 --- tests/lxd/test_lxd_image_vault.cpp | 4 --- tests/mock_permission_utils.h | 6 ++-- tests/test_base_virtual_machine_factory.cpp | 5 --- tests/test_cloud_init_iso.cpp | 36 ------------------- tests/test_image_vault.cpp | 10 ------ tests/test_permission_utils.cpp | 6 ++-- tests/test_utils.cpp | 4 --- 13 files changed, 20 insertions(+), 93 deletions(-) diff --git a/include/multipass/utils/permission_utils.h b/include/multipass/utils/permission_utils.h index 5241fec24c..95b0d47a4c 100644 --- a/include/multipass/utils/permission_utils.h +++ b/include/multipass/utils/permission_utils.h @@ -27,19 +27,18 @@ namespace multipass { +namespace fs = std::filesystem; class PermissionUtils : public Singleton { public: - using Path = std::filesystem::path; - PermissionUtils(const PrivatePass&) noexcept; - virtual void set_permissions(const Path& path, const QFileDevice::Permissions& permissions) const; - virtual void take_ownership(const Path& path) const; + virtual void set_permissions(const fs::path& path, const QFileDevice::Permissions& permissions) const; + virtual void take_ownership(const fs::path& path) const; // sets owner to root and sets permissions such that only owner has access. - virtual void restrict_permissions(const Path& path) const; + virtual void restrict_permissions(const fs::path& path) const; }; } // namespace multipass diff --git a/src/daemon/default_vm_image_vault.cpp b/src/daemon/default_vm_image_vault.cpp index a57778b767..23c5e49593 100644 --- a/src/daemon/default_vm_image_vault.cpp +++ b/src/daemon/default_vm_image_vault.cpp @@ -670,8 +670,7 @@ QString mp::DefaultVMImageVault::extract_image_from(const VMImage& source_image, const ProgressMonitor& monitor, const mp::Path& dest_dir) { - MP_UTILS.make_dir(dest_dir, QFile::ReadOwner | QFile::WriteOwner); - MP_PERMISSIONS.take_ownership(dest_dir.toStdU16String()); + MP_UTILS.make_dir(dest_dir); QFileInfo file_info{source_image.image_path}; const auto image_name = file_info.fileName().remove(".xz"); @@ -682,8 +681,7 @@ QString mp::DefaultVMImageVault::extract_image_from(const VMImage& source_image, mp::VMImage mp::DefaultVMImageVault::image_instance_from(const VMImage& prepared_image, const mp::Path& dest_dir) { - MP_UTILS.make_dir(dest_dir, QFile::ReadOwner | QFile::WriteOwner | QFile::ExeOwner); - MP_PERMISSIONS.take_ownership(dest_dir.toStdU16String()); + MP_UTILS.make_dir(dest_dir); return {mp::vault::copy(prepared_image.image_path, dest_dir), prepared_image.id, diff --git a/src/iso/cloud_init_iso.cpp b/src/iso/cloud_init_iso.cpp index 161b064736..3fbb2f675d 100644 --- a/src/iso/cloud_init_iso.cpp +++ b/src/iso/cloud_init_iso.cpp @@ -538,8 +538,6 @@ void mp::CloudInitIso::write_to(const Path& path) throw std::runtime_error{fmt::format( "Failed to open file for writing during cloud-init generation: {}; path: {}", f.errorString(), path)}; - MP_PERMISSIONS.restrict_permissions(path.toStdU16String()); - const uint32_t num_reserved_bytes = 32768u; const uint32_t num_reserved_blocks = num_blocks(num_reserved_bytes); f.seek(num_reserved_bytes); diff --git a/src/platform/backends/lxd/lxd_vm_image_vault.cpp b/src/platform/backends/lxd/lxd_vm_image_vault.cpp index 63e94e4fa3..7e6860e0b5 100644 --- a/src/platform/backends/lxd/lxd_vm_image_vault.cpp +++ b/src/platform/backends/lxd/lxd_vm_image_vault.cpp @@ -94,8 +94,6 @@ QString post_process_downloaded_image(const QString& image_path, const mp::Progr mp::vault::delete_file(original_image_path); } - MP_PERMISSIONS.restrict_permissions(new_image_path.toStdU16String()); - return new_image_path; } diff --git a/src/utils/permission_utils.cpp b/src/utils/permission_utils.cpp index 9ee79cb457..f2f326b24e 100644 --- a/src/utils/permission_utils.cpp +++ b/src/utils/permission_utils.cpp @@ -23,11 +23,9 @@ namespace mp = multipass; namespace fs = mp::fs; -using Path = mp::PermissionUtils::Path; - namespace { -void set_single_permissions(const Path& path, const QFileDevice::Permissions& permissions) +void set_single_permissions(const fs::path& path, const QFileDevice::Permissions& permissions) { QString qpath = QString::fromUtf8(path.u8string()); @@ -35,7 +33,7 @@ void set_single_permissions(const Path& path, const QFileDevice::Permissions& pe throw std::runtime_error(fmt::format("Cannot set permissions for '{}'", path.string())); } -void set_single_owner(const Path& path) +void set_single_owner(const fs::path& path) { QString qpath = QString::fromUtf8(path.u8string()); @@ -44,7 +42,7 @@ void set_single_owner(const Path& path) } // only exists because MP_FILEOPS doesn't overload the throwing variaions of std::filesystem functions -void throw_if_error(const Path& path, const std::error_code& ec) +void throw_if_error(const fs::path& path, const std::error_code& ec) { if (ec) throw std::system_error( @@ -54,7 +52,7 @@ void throw_if_error(const Path& path, const std::error_code& ec) // recursively iterates over all files in folder and applies a function that takes a path template -void apply_on_files(const Path& path, Func&& func) +void apply_on_files(const fs::path& path, Func&& func) { std::error_code ec{}; if (!MP_FILEOPS.exists(path, ec) || ec) @@ -83,22 +81,21 @@ void apply_on_files(const Path& path, Func&& func) } } // namespace -mp::PermissionUtils::PermissionUtils(const Singleton::PrivatePass& pass) noexcept - : Singleton::Singleton{pass} +mp::PermissionUtils::PermissionUtils(const PrivatePass& pass) noexcept : Singleton{pass} { } -void mp::PermissionUtils::set_permissions(const Path& path, const QFileDevice::Permissions& permissions) const +void mp::PermissionUtils::set_permissions(const fs::path& path, const QFileDevice::Permissions& permissions) const { apply_on_files(path, [&](const fs::path& apply_path) { set_single_permissions(apply_path, permissions); }); } -void mp::PermissionUtils::take_ownership(const Path& path) const +void mp::PermissionUtils::take_ownership(const fs::path& path) const { apply_on_files(path, [&](const fs::path& apply_path) { set_single_owner(apply_path); }); } -void mp::PermissionUtils::restrict_permissions(const Path& path) const +void mp::PermissionUtils::restrict_permissions(const fs::path& path) const { apply_on_files(path, [&](const fs::path& apply_path) { set_single_owner(apply_path); diff --git a/src/utils/vm_image_vault_utils.cpp b/src/utils/vm_image_vault_utils.cpp index 5a62d6c952..22812f77b8 100644 --- a/src/utils/vm_image_vault_utils.cpp +++ b/src/utils/vm_image_vault_utils.cpp @@ -48,8 +48,6 @@ QString mp::vault::copy(const QString& file_name, const QDir& output_dir) auto new_path = output_dir.filePath(source_name); QFile::copy(file_name, new_path); - MP_PERMISSIONS.restrict_permissions(new_path.toStdU16String()); - return new_path; } @@ -95,8 +93,6 @@ QString mp::vault::extract_image(const mp::Path& image_path, const mp::ProgressM xz_decoder.decode_to(new_image_path, monitor); - MP_PERMISSIONS.restrict_permissions(new_image_path.toStdU16String()); - mp::vault::delete_file(image_path); return new_image_path; diff --git a/tests/lxd/test_lxd_image_vault.cpp b/tests/lxd/test_lxd_image_vault.cpp index 2bf545c12d..16c5d0b983 100644 --- a/tests/lxd/test_lxd_image_vault.cpp +++ b/tests/lxd/test_lxd_image_vault.cpp @@ -22,7 +22,6 @@ #include "tests/common.h" #include "tests/mock_image_host.h" #include "tests/mock_logger.h" -#include "tests/mock_permission_utils.h" #include "tests/mock_process_factory.h" #include "tests/stub_url_downloader.h" #include "tests/temp_dir.h" @@ -61,9 +60,6 @@ struct LXDImageVault : public Test mpt::MockLogger::Scope logger_scope = mpt::MockLogger::inject(); std::unique_ptr> mock_network_access_manager; - const mpt::MockPermissionUtils::GuardedMock mock_permission_utils_injection = - mpt::MockPermissionUtils::inject(); - mpt::MockPermissionUtils& mock_permission_utils = *mock_permission_utils_injection.first; std::vector hosts; NiceMock host; QUrl base_url{"unix:///foo@1.0"}; diff --git a/tests/mock_permission_utils.h b/tests/mock_permission_utils.h index e0973c9a27..ec95c568d3 100644 --- a/tests/mock_permission_utils.h +++ b/tests/mock_permission_utils.h @@ -35,10 +35,10 @@ class MockPermissionUtils : public PermissionUtils MOCK_METHOD(void, set_permissions, - (const Path& path, const QFileDevice::Permissions& permissions), + (const fs::path& path, const QFileDevice::Permissions& permissions), (const, override)); - MOCK_METHOD(void, take_ownership, (const Path& path), (const, override)); - MOCK_METHOD(void, restrict_permissions, (const Path& path), (const, override)); + MOCK_METHOD(void, take_ownership, (const fs::path& path), (const, override)); + MOCK_METHOD(void, restrict_permissions, (const fs::path& path), (const, override)); MP_MOCK_SINGLETON_BOILERPLATE(MockPermissionUtils, PermissionUtils); }; diff --git a/tests/test_base_virtual_machine_factory.cpp b/tests/test_base_virtual_machine_factory.cpp index 0494484ba5..02e773887b 100644 --- a/tests/test_base_virtual_machine_factory.cpp +++ b/tests/test_base_virtual_machine_factory.cpp @@ -17,7 +17,6 @@ #include "common.h" #include "mock_logger.h" -#include "mock_permission_utils.h" #include "mock_platform.h" #include "stub_ssh_key_provider.h" #include "stub_url_downloader.h" @@ -124,10 +123,6 @@ TEST_F(BaseFactory, networks_throws) // at this time. Instead, just make sure an ISO image is created and has the expected path. TEST_F(BaseFactory, creates_cloud_init_iso_image) { - auto [mock_permission_utils, permission_utils_guard] = mpt::MockPermissionUtils::inject(); - - EXPECT_CALL(*mock_permission_utils, restrict_permissions(_)); - MockBaseFactory factory; const std::string name{"foo"}; const YAML::Node metadata{YAML::Load({fmt::format("name: {}", name)})}, vendor_data{metadata}, user_data{metadata}, diff --git a/tests/test_cloud_init_iso.cpp b/tests/test_cloud_init_iso.cpp index 194fe3506c..41eedacbe9 100644 --- a/tests/test_cloud_init_iso.cpp +++ b/tests/test_cloud_init_iso.cpp @@ -17,7 +17,6 @@ #include "common.h" #include "mock_file_ops.h" -#include "mock_permission_utils.h" #include "temp_dir.h" #include @@ -76,9 +75,6 @@ struct CloudInitIso : public Test mpt::TempDir temp_dir; QString iso_path; std::filesystem::path std_iso_path; - - const mpt::MockPermissionUtils::GuardedMock attr{mpt::MockPermissionUtils::inject()}; - mpt::MockPermissionUtils& mock_permission_utils = *attr.first; }; TEST_F(CloudInitIso, check_contains_false) @@ -140,8 +136,6 @@ TEST_F(CloudInitIso, check_index_operator_found_key) TEST_F(CloudInitIso, creates_iso_file) { - EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); - mp::CloudInitIso iso; iso.add_file("test", "test data"); iso.write_to(iso_path); @@ -153,8 +147,6 @@ TEST_F(CloudInitIso, creates_iso_file) TEST_F(CloudInitIso, reads_iso_file_failed_to_open_file) { - EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); - mp::CloudInitIso original_iso; original_iso.write_to(iso_path); @@ -168,8 +160,6 @@ TEST_F(CloudInitIso, reads_iso_file_failed_to_open_file) TEST_F(CloudInitIso, reads_iso_file_failed_to_read_single_bytes) { - EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); - mp::CloudInitIso original_iso; original_iso.write_to(iso_path); @@ -188,8 +178,6 @@ TEST_F(CloudInitIso, reads_iso_file_failed_to_read_single_bytes) TEST_F(CloudInitIso, reads_iso_file_failed_to_check_it_has_Joliet_volume_descriptor) { - EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); - mp::CloudInitIso original_iso; original_iso.write_to(iso_path); @@ -213,8 +201,6 @@ TEST_F(CloudInitIso, reads_iso_file_failed_to_check_it_has_Joliet_volume_descrip TEST_F(CloudInitIso, reads_iso_file_Joliet_volume_descriptor_malformed) { - EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); - mp::CloudInitIso original_iso; original_iso.write_to(iso_path); @@ -240,8 +226,6 @@ TEST_F(CloudInitIso, reads_iso_file_Joliet_volume_descriptor_malformed) TEST_F(CloudInitIso, reads_iso_file_failed_to_read_array) { - EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); - mp::CloudInitIso original_iso; original_iso.write_to(iso_path); @@ -263,8 +247,6 @@ TEST_F(CloudInitIso, reads_iso_file_failed_to_read_array) TEST_F(CloudInitIso, reads_iso_file_failed_to_check_root_dir_record_data) { - EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); - mp::CloudInitIso original_iso; original_iso.write_to(iso_path); @@ -292,8 +274,6 @@ TEST_F(CloudInitIso, reads_iso_file_failed_to_check_root_dir_record_data) TEST_F(CloudInitIso, reads_iso_file_failed_to_read_vec) { - EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); - mp::CloudInitIso original_iso; // At least one actual file entry is need to reach the read_bytes_to_vec call original_iso.add_file("test1", "test data1"); @@ -319,8 +299,6 @@ TEST_F(CloudInitIso, reads_iso_file_failed_to_read_vec) TEST_F(CloudInitIso, reads_iso_file_encoded_file_name_is_not_even_length) { - EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); - mp::CloudInitIso original_iso; // At least one actual file entry is need to reach the convert_u16_name_back call original_iso.add_file("test1", "test data1"); @@ -356,8 +334,6 @@ TEST_F(CloudInitIso, reads_iso_file_encoded_file_name_is_not_even_length) TEST_F(CloudInitIso, reads_iso_file_with_random_string_data) { - EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); - mp::CloudInitIso original_iso; original_iso.add_file("test1", "test data1"); @@ -395,8 +371,6 @@ timezone: Europe/Amsterdam content: "multipass/version/1.14.0-dev.1209+g5b2c7f7d # written by Multipass\nmultipass/driver/qemu-8.0.4 # written by Multipass\nmultipass/host/ubuntu-23.10 # written by Multipass\nmultipass/alias/default # written by Multipass\n" )"; - EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); - mp::CloudInitIso original_iso; original_iso.add_file("meta-data", default_meta_data_content); @@ -412,8 +386,6 @@ timezone: Europe/Amsterdam TEST_F(CloudInitIso, updateCloudInitWithNewNonEmptyExtraInterfaces) { - EXPECT_CALL(mock_permission_utils, restrict_permissions(std_iso_path)).Times(2); - mp::CloudInitIso original_iso; original_iso.add_file("meta-data", default_meta_data_content); @@ -440,8 +412,6 @@ TEST_F(CloudInitIso, updateCloudInitWithNewNonEmptyExtraInterfaces) TEST_F(CloudInitIso, updateCloudInitWithNewEmptyExtraInterfaces) { - EXPECT_CALL(mock_permission_utils, restrict_permissions(std_iso_path)).Times(2); - mp::CloudInitIso original_iso; original_iso.add_file("meta-data", default_meta_data_content); original_iso.add_file("network-config", "dummy_data"); @@ -465,8 +435,6 @@ TEST_F(CloudInitIso, updateCloneCloudInitSrcFileWithExtraInterfaces) const std::string src_network_config_data_content = fmt::format(network_config_data_content_template, "00:00:00:00:00:00", "00:00:00:00:00:01"); - EXPECT_CALL(mock_permission_utils, restrict_permissions(std_iso_path)).Times(2); - mp::CloudInitIso original_iso; original_iso.add_file("meta-data", src_meta_data_content); original_iso.add_file("network-config", src_network_config_data_content); @@ -492,8 +460,6 @@ TEST_F(CloudInitIso, updateCloneCloudInitSrcFileWithExtraInterfaces) TEST_F(CloudInitIso, addExtraInterfaceToCloudInit) { - EXPECT_CALL(mock_permission_utils, restrict_permissions(std_iso_path)).Times(2); - mp::CloudInitIso original_iso; original_iso.add_file("meta-data", default_meta_data_content); original_iso.write_to(iso_path); @@ -505,8 +471,6 @@ TEST_F(CloudInitIso, addExtraInterfaceToCloudInit) TEST_F(CloudInitIso, getInstanceIdFromCloudInit) { - EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); - mp::CloudInitIso original_iso; original_iso.add_file("meta-data", default_meta_data_content); original_iso.write_to(iso_path); diff --git a/tests/test_image_vault.cpp b/tests/test_image_vault.cpp index e0744b69f6..3fc1b5d24b 100644 --- a/tests/test_image_vault.cpp +++ b/tests/test_image_vault.cpp @@ -21,7 +21,6 @@ #include "mock_image_host.h" #include "mock_json_utils.h" #include "mock_logger.h" -#include "mock_permission_utils.h" #include "mock_platform.h" #include "mock_process_factory.h" #include "path.h" @@ -168,9 +167,6 @@ struct ImageVault : public testing::Test NiceMock host; mpt::MockJsonUtils::GuardedMock mock_json_utils_injection = mpt::MockJsonUtils::inject(); mpt::MockJsonUtils& mock_json_utils = *mock_json_utils_injection.first; - const mpt::MockPermissionUtils::GuardedMock mock_permission_utils_injection = - mpt::MockPermissionUtils::inject(); - mpt::MockPermissionUtils& mock_permission_utils = *mock_permission_utils_injection.first; mp::ProgressMonitor stub_monitor{[](int, int) { return true; }}; mp::VMImageVault::PrepareAction stub_prepare{ [](const mp::VMImage& source_image) -> mp::VMImage { return source_image; }}; @@ -418,8 +414,6 @@ TEST_F(ImageVault, remembers_prepared_images) TEST_F(ImageVault, uses_image_from_prepare) { - EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); - constexpr auto expected_data = "12345-pied-piper-rats"; QDir dir{cache_dir.path()}; @@ -515,8 +509,6 @@ TEST_F(ImageVault, invalid_image_dir_is_removed) TEST_F(ImageVault, DISABLE_ON_WINDOWS_AND_MACOS(file_based_fetch_copies_image_and_returns_expected_info)) { - EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); - mpt::TempFile file; mp::DefaultVMImageVault vault{hosts, &url_downloader, cache_dir.path(), data_dir.path(), mp::days{0}}; auto query = default_query; @@ -748,8 +740,6 @@ TEST_F(ImageVault, minimum_image_size_returns_expected_size) TEST_F(ImageVault, DISABLE_ON_WINDOWS_AND_MACOS(file_based_minimum_size_returns_expected_size)) { - EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); - const mp::MemorySize image_size{"2097152"}; const mp::ProcessState qemuimg_exit_status{0, std::nullopt}; const QByteArray qemuimg_output(fake_img_info(image_size)); diff --git a/tests/test_permission_utils.cpp b/tests/test_permission_utils.cpp index cd8374f1d6..724bd9ca01 100644 --- a/tests/test_permission_utils.cpp +++ b/tests/test_permission_utils.cpp @@ -33,7 +33,7 @@ struct TestPermissionUtils : public Test const mpt::MockPlatform::GuardedMock guarded_mock_platform = mpt::MockPlatform::inject(); mpt::MockPlatform& mock_platform = *guarded_mock_platform.first; - const mp::PermissionUtils::Path test_path{"test_path"}; + const mp::fs::path test_path{"test_path"}; const QString test_qpath{QString::fromUtf8(test_path.u8string())}; static constexpr QFileDevice::Permissions test_permissions{QFileDevice::Permission::ReadOwner}; static constexpr QFileDevice::Permissions restricted_permissions{ @@ -136,11 +136,11 @@ struct TestPermissionUtilsDir : public TestPermissionUtils EXPECT_CALL(mock_file_ops, recursive_dir_iterator(test_path, _)).WillOnce(Return(std::move(iter))); } - const mp::PermissionUtils::Path path1{"Hello.txt"}; + const mp::fs::path path1{"Hello.txt"}; const QString qpath1{QString::fromUtf8(path1.u8string())}; mpt::MockDirectoryEntry entry1; - const mp::PermissionUtils::Path path2{"World.txt"}; + const mp::fs::path path2{"World.txt"}; const QString qpath2{QString::fromUtf8(path2.u8string())}; mpt::MockDirectoryEntry entry2; }; diff --git a/tests/test_utils.cpp b/tests/test_utils.cpp index 42236255c8..5fd509494a 100644 --- a/tests/test_utils.cpp +++ b/tests/test_utils.cpp @@ -750,10 +750,6 @@ TEST(Utils, check_filesystem_bytes_available_returns_non_negative) TEST(VaultUtils, copy_creates_new_file_and_returned_path_exists) { - auto [mock_permission_utils, permission_utils_guard] = mpt::MockPermissionUtils::inject(); - - EXPECT_CALL(*mock_permission_utils, restrict_permissions(_)); - mpt::TempDir temp_dir1, temp_dir2; auto orig_file_path = QDir(temp_dir1.path()).filePath("test_file"); From 36e630f021fad5a065030058f8f374bf5e48ccbd Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Tue, 17 Dec 2024 15:37:09 -0500 Subject: [PATCH 11/12] Unify after rebase --- src/platform/platform_unix.cpp | 2 +- tests/mock_platform.h | 8 ++------ 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/src/platform/platform_unix.cpp b/src/platform/platform_unix.cpp index 1d69368e73..89fb3abfe4 100644 --- a/src/platform/platform_unix.cpp +++ b/src/platform/platform_unix.cpp @@ -63,7 +63,7 @@ bool mp::platform::Platform::set_permissions(const Path& path, const Perms permi return QFile::setPermissions(path, permissions); } -bool mp::platform::Platform::take_ownership(const mp::Path& path) const +bool mp::platform::Platform::take_ownership(const Path& path) const { return this->chown(path.toStdString().c_str(), 0, 0) == 0; } diff --git a/tests/mock_platform.h b/tests/mock_platform.h index 3a3e0385b4..8e01fc85a0 100644 --- a/tests/mock_platform.h +++ b/tests/mock_platform.h @@ -40,13 +40,9 @@ class MockPlatform : public platform::Platform MOCK_METHOD(bool, is_remote_supported, (const std::string&), (const, override)); MOCK_METHOD(bool, is_backend_supported, (const QString&), (const, override)); MOCK_METHOD(bool, is_alias_supported, (const std::string&, const std::string&), (const, override)); - MOCK_METHOD(bool, set_permissions, (const multipass::Path&, const QFileDevice::Permissions), (const, override)); MOCK_METHOD(int, chown, (const char*, unsigned int, unsigned int), (const, override)); - MOCK_METHOD(bool, - set_permissions, - (const multipass::Path& path, const QFileDevice::Permissions), - (const, override)); - MOCK_METHOD(bool, take_ownership, (const multipass::Path& path), (const, override)); + MOCK_METHOD(bool, set_permissions, (const Path&, const Perms), (const, override)); + MOCK_METHOD(bool, take_ownership, (const Path&), (const, override)); MOCK_METHOD(bool, link, (const char*, const char*), (const, override)); MOCK_METHOD(bool, symlink, (const char*, const char*, bool), (const, override)); MOCK_METHOD(int, utime, (const char*, int, int), (const, override)); From 73f969e9c0a42c159e55a88ecba1b86394a0360c Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Wed, 18 Dec 2024 13:54:51 -0500 Subject: [PATCH 12/12] Readd permission setting to new files/folders --- include/multipass/utils.h | 10 ++++-- src/cert/client_cert_store.cpp | 3 +- src/iso/cloud_init_iso.cpp | 2 ++ .../backends/lxd/lxd_vm_image_vault.cpp | 2 ++ src/utils/vm_image_vault_utils.cpp | 4 +++ tests/lxd/test_lxd_image_vault.cpp | 4 +++ tests/test_base_virtual_machine_factory.cpp | 4 +++ tests/test_client_cert_store.cpp | 13 +++++++ tests/test_cloud_init_iso.cpp | 36 +++++++++++++++++++ tests/test_image_vault.cpp | 10 ++++++ tests/test_utils.cpp | 3 ++ 11 files changed, 87 insertions(+), 4 deletions(-) diff --git a/include/multipass/utils.h b/include/multipass/utils.h index df65dc31d9..0b6520ddf8 100644 --- a/include/multipass/utils.h +++ b/include/multipass/utils.h @@ -202,14 +202,18 @@ class Utils : public Singleton public: Utils(const Singleton::PrivatePass&) noexcept; + static constexpr QFileDevice::Permissions default_permissions = + QFile::ReadOwner | QFile::WriteOwner | QFile::ExeOwner; + virtual qint64 filesystem_bytes_available(const QString& data_directory) const; virtual void exit(int code); virtual std::string contents_of(const multipass::Path& file_path) const; virtual void make_file_with_content(const std::string& file_name, const std::string& content, const bool& overwrite = false); - virtual Path make_dir(const QDir& a_dir, const QString& name, - QFileDevice::Permissions permissions = QFileDevice::Permissions(0)); - virtual Path make_dir(const QDir& dir, QFileDevice::Permissions permissions = QFileDevice::Permissions(0)); + virtual Path make_dir(const QDir& a_dir, + const QString& name, + QFileDevice::Permissions permissions = default_permissions); + virtual Path make_dir(const QDir& dir, QFileDevice::Permissions permissions = default_permissions); // command and process helpers virtual std::string run_cmd_for_output(const QString& cmd, const QStringList& args, diff --git a/src/cert/client_cert_store.cpp b/src/cert/client_cert_store.cpp index 52dade2a2b..fed9c52b89 100644 --- a/src/cert/client_cert_store.cpp +++ b/src/cert/client_cert_store.cpp @@ -26,6 +26,7 @@ #include #include +#include #include namespace mp = multipass; @@ -76,7 +77,7 @@ void mp::ClientCertStore::add_cert(const std::string& pem_cert) if (!MP_FILEOPS.open(file, QIODevice::WriteOnly)) throw std::runtime_error("failed to create file to store certificate"); - file.setPermissions(QFile::ReadOwner | QFile::WriteOwner); + MP_PERMISSIONS.restrict_permissions(file.fileName().toStdU16String()); // QIODevice::Append is not supported in QSaveFile, so must write out all of the // existing clients certs each time. diff --git a/src/iso/cloud_init_iso.cpp b/src/iso/cloud_init_iso.cpp index 3fbb2f675d..161b064736 100644 --- a/src/iso/cloud_init_iso.cpp +++ b/src/iso/cloud_init_iso.cpp @@ -538,6 +538,8 @@ void mp::CloudInitIso::write_to(const Path& path) throw std::runtime_error{fmt::format( "Failed to open file for writing during cloud-init generation: {}; path: {}", f.errorString(), path)}; + MP_PERMISSIONS.restrict_permissions(path.toStdU16String()); + const uint32_t num_reserved_bytes = 32768u; const uint32_t num_reserved_blocks = num_blocks(num_reserved_bytes); f.seek(num_reserved_bytes); diff --git a/src/platform/backends/lxd/lxd_vm_image_vault.cpp b/src/platform/backends/lxd/lxd_vm_image_vault.cpp index 7e6860e0b5..63e94e4fa3 100644 --- a/src/platform/backends/lxd/lxd_vm_image_vault.cpp +++ b/src/platform/backends/lxd/lxd_vm_image_vault.cpp @@ -94,6 +94,8 @@ QString post_process_downloaded_image(const QString& image_path, const mp::Progr mp::vault::delete_file(original_image_path); } + MP_PERMISSIONS.restrict_permissions(new_image_path.toStdU16String()); + return new_image_path; } diff --git a/src/utils/vm_image_vault_utils.cpp b/src/utils/vm_image_vault_utils.cpp index 22812f77b8..de78caffa3 100644 --- a/src/utils/vm_image_vault_utils.cpp +++ b/src/utils/vm_image_vault_utils.cpp @@ -48,6 +48,8 @@ QString mp::vault::copy(const QString& file_name, const QDir& output_dir) auto new_path = output_dir.filePath(source_name); QFile::copy(file_name, new_path); + MP_PERMISSIONS.restrict_permissions(new_path.toStdU16String()); + return new_path; } @@ -95,6 +97,8 @@ QString mp::vault::extract_image(const mp::Path& image_path, const mp::ProgressM mp::vault::delete_file(image_path); + MP_PERMISSIONS.restrict_permissions(new_image_path.toStdU16String()); + return new_image_path; } diff --git a/tests/lxd/test_lxd_image_vault.cpp b/tests/lxd/test_lxd_image_vault.cpp index 16c5d0b983..2bf545c12d 100644 --- a/tests/lxd/test_lxd_image_vault.cpp +++ b/tests/lxd/test_lxd_image_vault.cpp @@ -22,6 +22,7 @@ #include "tests/common.h" #include "tests/mock_image_host.h" #include "tests/mock_logger.h" +#include "tests/mock_permission_utils.h" #include "tests/mock_process_factory.h" #include "tests/stub_url_downloader.h" #include "tests/temp_dir.h" @@ -60,6 +61,9 @@ struct LXDImageVault : public Test mpt::MockLogger::Scope logger_scope = mpt::MockLogger::inject(); std::unique_ptr> mock_network_access_manager; + const mpt::MockPermissionUtils::GuardedMock mock_permission_utils_injection = + mpt::MockPermissionUtils::inject(); + mpt::MockPermissionUtils& mock_permission_utils = *mock_permission_utils_injection.first; std::vector hosts; NiceMock host; QUrl base_url{"unix:///foo@1.0"}; diff --git a/tests/test_base_virtual_machine_factory.cpp b/tests/test_base_virtual_machine_factory.cpp index 02e773887b..5abb8c1685 100644 --- a/tests/test_base_virtual_machine_factory.cpp +++ b/tests/test_base_virtual_machine_factory.cpp @@ -17,6 +17,7 @@ #include "common.h" #include "mock_logger.h" +#include "mock_permission_utils.h" #include "mock_platform.h" #include "stub_ssh_key_provider.h" #include "stub_url_downloader.h" @@ -123,6 +124,9 @@ TEST_F(BaseFactory, networks_throws) // at this time. Instead, just make sure an ISO image is created and has the expected path. TEST_F(BaseFactory, creates_cloud_init_iso_image) { + auto [mock_permission_utils, permission_utils_guard] = mpt::MockPermissionUtils::inject(); + EXPECT_CALL(*mock_permission_utils, restrict_permissions(_)); + MockBaseFactory factory; const std::string name{"foo"}; const YAML::Node metadata{YAML::Load({fmt::format("name: {}", name)})}, vendor_data{metadata}, user_data{metadata}, diff --git a/tests/test_client_cert_store.cpp b/tests/test_client_cert_store.cpp index f2c58b18ee..316ee3c353 100644 --- a/tests/test_client_cert_store.cpp +++ b/tests/test_client_cert_store.cpp @@ -19,6 +19,7 @@ #include "file_operations.h" #include "mock_file_ops.h" #include "mock_logger.h" +#include "mock_permission_utils.h" #include "mock_utils.h" #include "temp_dir.h" @@ -99,6 +100,9 @@ TEST_F(ClientCertStore, add_cert_throws_on_invalid_data) TEST_F(ClientCertStore, add_cert_stores_certificate) { + auto [mock_permission_utils, permission_utils_guard] = mpt::MockPermissionUtils::inject(); + EXPECT_CALL(*mock_permission_utils, restrict_permissions(_)); + mp::ClientCertStore cert_store{temp_dir.path()}; EXPECT_NO_THROW(cert_store.add_cert(cert_data)); @@ -151,6 +155,9 @@ TEST_F(ClientCertStore, addCertAlreadyExistingDoesNotAddAgain) TEST_F(ClientCertStore, addCertWithExistingCertPersistsCerts) { + auto [mock_permission_utils, permission_utils_guard] = mpt::MockPermissionUtils::inject(); + EXPECT_CALL(*mock_permission_utils, restrict_permissions(_)); + const QDir dir{cert_dir}; const auto cert_path = dir.filePath("multipass_client_certs.pem"); mpt::make_file_with_content(cert_path, cert_data); @@ -191,6 +198,9 @@ TEST_F(ClientCertStore, openingFileForWritingFailsAndThrows) auto [mock_file_ops, guard] = mpt::MockFileOps::inject(); EXPECT_CALL(*mock_file_ops, open(_, _)).WillOnce(Return(false)); + auto [mock_permission_utils, permission_utils_guard] = mpt::MockPermissionUtils::inject(); + EXPECT_CALL(*mock_permission_utils, set_permissions(_, _)); + mp::ClientCertStore cert_store{temp_dir.path()}; MP_EXPECT_THROW_THAT(cert_store.add_cert(cert_data), std::runtime_error, @@ -206,6 +216,9 @@ TEST_F(ClientCertStore, writingFileFailsAndThrows) EXPECT_CALL(*mock_file_ops, write(_, _)).WillOnce(Return(-1)); EXPECT_CALL(*mock_file_ops, commit).WillOnce(Return(false)); + auto [mock_permission_utils, permission_utils_guard] = mpt::MockPermissionUtils::inject(); + EXPECT_CALL(*mock_permission_utils, restrict_permissions(_)); + mp::ClientCertStore cert_store{temp_dir.path()}; MP_EXPECT_THROW_THAT(cert_store.add_cert(cert_data), std::runtime_error, diff --git a/tests/test_cloud_init_iso.cpp b/tests/test_cloud_init_iso.cpp index 41eedacbe9..194fe3506c 100644 --- a/tests/test_cloud_init_iso.cpp +++ b/tests/test_cloud_init_iso.cpp @@ -17,6 +17,7 @@ #include "common.h" #include "mock_file_ops.h" +#include "mock_permission_utils.h" #include "temp_dir.h" #include @@ -75,6 +76,9 @@ struct CloudInitIso : public Test mpt::TempDir temp_dir; QString iso_path; std::filesystem::path std_iso_path; + + const mpt::MockPermissionUtils::GuardedMock attr{mpt::MockPermissionUtils::inject()}; + mpt::MockPermissionUtils& mock_permission_utils = *attr.first; }; TEST_F(CloudInitIso, check_contains_false) @@ -136,6 +140,8 @@ TEST_F(CloudInitIso, check_index_operator_found_key) TEST_F(CloudInitIso, creates_iso_file) { + EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); + mp::CloudInitIso iso; iso.add_file("test", "test data"); iso.write_to(iso_path); @@ -147,6 +153,8 @@ TEST_F(CloudInitIso, creates_iso_file) TEST_F(CloudInitIso, reads_iso_file_failed_to_open_file) { + EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); + mp::CloudInitIso original_iso; original_iso.write_to(iso_path); @@ -160,6 +168,8 @@ TEST_F(CloudInitIso, reads_iso_file_failed_to_open_file) TEST_F(CloudInitIso, reads_iso_file_failed_to_read_single_bytes) { + EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); + mp::CloudInitIso original_iso; original_iso.write_to(iso_path); @@ -178,6 +188,8 @@ TEST_F(CloudInitIso, reads_iso_file_failed_to_read_single_bytes) TEST_F(CloudInitIso, reads_iso_file_failed_to_check_it_has_Joliet_volume_descriptor) { + EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); + mp::CloudInitIso original_iso; original_iso.write_to(iso_path); @@ -201,6 +213,8 @@ TEST_F(CloudInitIso, reads_iso_file_failed_to_check_it_has_Joliet_volume_descrip TEST_F(CloudInitIso, reads_iso_file_Joliet_volume_descriptor_malformed) { + EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); + mp::CloudInitIso original_iso; original_iso.write_to(iso_path); @@ -226,6 +240,8 @@ TEST_F(CloudInitIso, reads_iso_file_Joliet_volume_descriptor_malformed) TEST_F(CloudInitIso, reads_iso_file_failed_to_read_array) { + EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); + mp::CloudInitIso original_iso; original_iso.write_to(iso_path); @@ -247,6 +263,8 @@ TEST_F(CloudInitIso, reads_iso_file_failed_to_read_array) TEST_F(CloudInitIso, reads_iso_file_failed_to_check_root_dir_record_data) { + EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); + mp::CloudInitIso original_iso; original_iso.write_to(iso_path); @@ -274,6 +292,8 @@ TEST_F(CloudInitIso, reads_iso_file_failed_to_check_root_dir_record_data) TEST_F(CloudInitIso, reads_iso_file_failed_to_read_vec) { + EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); + mp::CloudInitIso original_iso; // At least one actual file entry is need to reach the read_bytes_to_vec call original_iso.add_file("test1", "test data1"); @@ -299,6 +319,8 @@ TEST_F(CloudInitIso, reads_iso_file_failed_to_read_vec) TEST_F(CloudInitIso, reads_iso_file_encoded_file_name_is_not_even_length) { + EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); + mp::CloudInitIso original_iso; // At least one actual file entry is need to reach the convert_u16_name_back call original_iso.add_file("test1", "test data1"); @@ -334,6 +356,8 @@ TEST_F(CloudInitIso, reads_iso_file_encoded_file_name_is_not_even_length) TEST_F(CloudInitIso, reads_iso_file_with_random_string_data) { + EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); + mp::CloudInitIso original_iso; original_iso.add_file("test1", "test data1"); @@ -371,6 +395,8 @@ timezone: Europe/Amsterdam content: "multipass/version/1.14.0-dev.1209+g5b2c7f7d # written by Multipass\nmultipass/driver/qemu-8.0.4 # written by Multipass\nmultipass/host/ubuntu-23.10 # written by Multipass\nmultipass/alias/default # written by Multipass\n" )"; + EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); + mp::CloudInitIso original_iso; original_iso.add_file("meta-data", default_meta_data_content); @@ -386,6 +412,8 @@ timezone: Europe/Amsterdam TEST_F(CloudInitIso, updateCloudInitWithNewNonEmptyExtraInterfaces) { + EXPECT_CALL(mock_permission_utils, restrict_permissions(std_iso_path)).Times(2); + mp::CloudInitIso original_iso; original_iso.add_file("meta-data", default_meta_data_content); @@ -412,6 +440,8 @@ TEST_F(CloudInitIso, updateCloudInitWithNewNonEmptyExtraInterfaces) TEST_F(CloudInitIso, updateCloudInitWithNewEmptyExtraInterfaces) { + EXPECT_CALL(mock_permission_utils, restrict_permissions(std_iso_path)).Times(2); + mp::CloudInitIso original_iso; original_iso.add_file("meta-data", default_meta_data_content); original_iso.add_file("network-config", "dummy_data"); @@ -435,6 +465,8 @@ TEST_F(CloudInitIso, updateCloneCloudInitSrcFileWithExtraInterfaces) const std::string src_network_config_data_content = fmt::format(network_config_data_content_template, "00:00:00:00:00:00", "00:00:00:00:00:01"); + EXPECT_CALL(mock_permission_utils, restrict_permissions(std_iso_path)).Times(2); + mp::CloudInitIso original_iso; original_iso.add_file("meta-data", src_meta_data_content); original_iso.add_file("network-config", src_network_config_data_content); @@ -460,6 +492,8 @@ TEST_F(CloudInitIso, updateCloneCloudInitSrcFileWithExtraInterfaces) TEST_F(CloudInitIso, addExtraInterfaceToCloudInit) { + EXPECT_CALL(mock_permission_utils, restrict_permissions(std_iso_path)).Times(2); + mp::CloudInitIso original_iso; original_iso.add_file("meta-data", default_meta_data_content); original_iso.write_to(iso_path); @@ -471,6 +505,8 @@ TEST_F(CloudInitIso, addExtraInterfaceToCloudInit) TEST_F(CloudInitIso, getInstanceIdFromCloudInit) { + EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); + mp::CloudInitIso original_iso; original_iso.add_file("meta-data", default_meta_data_content); original_iso.write_to(iso_path); diff --git a/tests/test_image_vault.cpp b/tests/test_image_vault.cpp index 3fc1b5d24b..e0744b69f6 100644 --- a/tests/test_image_vault.cpp +++ b/tests/test_image_vault.cpp @@ -21,6 +21,7 @@ #include "mock_image_host.h" #include "mock_json_utils.h" #include "mock_logger.h" +#include "mock_permission_utils.h" #include "mock_platform.h" #include "mock_process_factory.h" #include "path.h" @@ -167,6 +168,9 @@ struct ImageVault : public testing::Test NiceMock host; mpt::MockJsonUtils::GuardedMock mock_json_utils_injection = mpt::MockJsonUtils::inject(); mpt::MockJsonUtils& mock_json_utils = *mock_json_utils_injection.first; + const mpt::MockPermissionUtils::GuardedMock mock_permission_utils_injection = + mpt::MockPermissionUtils::inject(); + mpt::MockPermissionUtils& mock_permission_utils = *mock_permission_utils_injection.first; mp::ProgressMonitor stub_monitor{[](int, int) { return true; }}; mp::VMImageVault::PrepareAction stub_prepare{ [](const mp::VMImage& source_image) -> mp::VMImage { return source_image; }}; @@ -414,6 +418,8 @@ TEST_F(ImageVault, remembers_prepared_images) TEST_F(ImageVault, uses_image_from_prepare) { + EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); + constexpr auto expected_data = "12345-pied-piper-rats"; QDir dir{cache_dir.path()}; @@ -509,6 +515,8 @@ TEST_F(ImageVault, invalid_image_dir_is_removed) TEST_F(ImageVault, DISABLE_ON_WINDOWS_AND_MACOS(file_based_fetch_copies_image_and_returns_expected_info)) { + EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); + mpt::TempFile file; mp::DefaultVMImageVault vault{hosts, &url_downloader, cache_dir.path(), data_dir.path(), mp::days{0}}; auto query = default_query; @@ -740,6 +748,8 @@ TEST_F(ImageVault, minimum_image_size_returns_expected_size) TEST_F(ImageVault, DISABLE_ON_WINDOWS_AND_MACOS(file_based_minimum_size_returns_expected_size)) { + EXPECT_CALL(mock_permission_utils, restrict_permissions(_)); + const mp::MemorySize image_size{"2097152"}; const mp::ProcessState qemuimg_exit_status{0, std::nullopt}; const QByteArray qemuimg_output(fake_img_info(image_size)); diff --git a/tests/test_utils.cpp b/tests/test_utils.cpp index 5fd509494a..1bea453925 100644 --- a/tests/test_utils.cpp +++ b/tests/test_utils.cpp @@ -750,6 +750,9 @@ TEST(Utils, check_filesystem_bytes_available_returns_non_negative) TEST(VaultUtils, copy_creates_new_file_and_returned_path_exists) { + auto [mock_permission_utils, permission_utils_guard] = mpt::MockPermissionUtils::inject(); + EXPECT_CALL(*mock_permission_utils, restrict_permissions(_)); + mpt::TempDir temp_dir1, temp_dir2; auto orig_file_path = QDir(temp_dir1.path()).filePath("test_file");