From 73f969e9c0a42c159e55a88ecba1b86394a0360c Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Wed, 18 Dec 2024 13:54:51 -0500 Subject: [PATCH] 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");