Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change file permissions on instances #3715

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
3 changes: 2 additions & 1 deletion include/multipass/platform.h
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,8 @@ class Platform : public Singleton<Platform>
virtual bool is_backend_supported(const QString& backend) const; // temporary (?)
virtual int chown(const char* path, unsigned int uid, unsigned int gid) const;
virtual int chmod(const char* path, unsigned int mode) const;
virtual bool set_permissions(const multipass::Path path, const QFileDevice::Permissions permissions) const;
virtual bool set_permissions(const multipass::Path& path, const QFileDevice::Permissions permissions) const;
virtual bool take_ownership(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;
Expand Down
45 changes: 45 additions & 0 deletions include/multipass/utils/permission_utils.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* 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 <http://www.gnu.org/licenses/>.
*
*/

#ifndef MULTIPASS_PERMISSION_UTILS_H
#define MULTIPASS_PERMISSION_UTILS_H

#include <multipass/singleton.h>

#include <QFileDevice>
#include <filesystem>

#define MP_PERMISSIONS multipass::PermissionUtils::instance()

namespace multipass
{
namespace fs = std::filesystem;

class PermissionUtils : public Singleton<PermissionUtils>
{
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) 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
11 changes: 11 additions & 0 deletions src/daemon/daemon_config.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include <multipass/ssl_cert_provider.h>
#include <multipass/standard_paths.h>
#include <multipass/utils.h>
#include <multipass/utils/permission_utils.h>

#include <QString>
#include <QSysInfo>
Expand Down Expand Up @@ -189,6 +190,16 @@ std::unique_ptr<const mp::DaemonConfig> mp::DaemonConfigBuilder::build()
std::make_unique<DefaultVMBlueprintProvider>(url_downloader.get(), cache_directory, manifest_ttl);
}

if (!storage_path.isEmpty())
{
MP_PERMISSIONS.restrict_permissions(storage_path.toStdU16String());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is some overlap here where the permissions on the storage path is already being set.

}
else
{
MP_PERMISSIONS.restrict_permissions(data_directory.toStdU16String());
MP_PERMISSIONS.restrict_permissions(cache_directory.toStdU16String());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you use toStdU16String() and not toStdString()? Something to do with valid characters in the Windows filesystem?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, std::filesystem::path's constructor from std::string is assumed to be the "native narrow encoding", which is not guaranteed to be UTF-8 (I think). But the char16_t constructor does handle UTF-16 conversion.

}

return std::unique_ptr<const DaemonConfig>(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),
Expand Down
2 changes: 2 additions & 0 deletions src/daemon/default_vm_image_vault.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
#include <QtConcurrent/QtConcurrent>

#include <exception>
#include <multipass/utils/permission_utils.h>

namespace mp = multipass;
namespace mpl = multipass::logging;
Expand Down Expand Up @@ -670,6 +671,7 @@ QString mp::DefaultVMImageVault::extract_image_from(const VMImage& source_image,
const mp::Path& dest_dir)
{
MP_UTILS.make_dir(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);
Expand Down
2 changes: 2 additions & 0 deletions src/iso/cloud_init_iso.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
#include <multipass/cloud_init_iso.h>
#include <multipass/file_ops.h>
#include <multipass/format.h>
#include <multipass/platform.h>
#include <multipass/utils/permission_utils.h>
#include <multipass/yaml_node_utils.h>

#include <QFile>
Expand Down
1 change: 1 addition & 0 deletions src/platform/backends/lxd/lxd_vm_image_vault.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
#include <QTemporaryDir>

#include <chrono>
#include <multipass/utils/permission_utils.h>
#include <thread>

namespace mp = multipass;
Expand Down
7 changes: 6 additions & 1 deletion src/platform/platform_unix.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,16 @@
return ::chmod(path, mode);
}

bool mp::platform::Platform::set_permissions(const mp::Path path, const QFileDevice::Permissions permissions) const
bool mp::platform::Platform::set_permissions(const mp::Path& path, const QFileDevice::Permissions permissions) const
{
return QFile::setPermissions(path, permissions);
}

bool mp::platform::Platform::take_ownership(const mp::Path& path) const

Check warning on line 71 in src/platform/platform_unix.cpp

View check run for this annotation

Codecov / codecov/patch

src/platform/platform_unix.cpp#L71

Added line #L71 was not covered by tests
{
return this->chown(path.toStdString().c_str(), 0, 0) == 0;

Check warning on line 73 in src/platform/platform_unix.cpp

View check run for this annotation

Codecov / codecov/patch

src/platform/platform_unix.cpp#L73

Added line #L73 was not covered by tests
}

bool mp::platform::Platform::symlink(const char* target, const char* link, bool is_dir) const
{
return ::symlink(target, link) == 0;
Expand Down
1 change: 1 addition & 0 deletions src/utils/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
104 changes: 104 additions & 0 deletions src/utils/permission_utils.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
/*
* 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 <http://www.gnu.org/licenses/>.
*
*/

#include <multipass/utils/permission_utils.h>

#include <multipass/file_ops.h>
#include <multipass/platform.h>

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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some funny conversion going on here. QString to u16String to u8string to QString.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, QString -> u16String -> std::filesystem::path -> u8string -> QString -> std::string -> char*. I could probably get it to QString -> u16String -> std::filesystem::path -> std::string -> char* but any better than that would require some pretty massive changes to everything.


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)
{
QString qpath = QString::fromUtf8(path.u8string());

if (!MP_PLATFORM.take_ownership(qpath))
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(

Check warning on line 48 in src/utils/permission_utils.cpp

View check run for this annotation

Codecov / codecov/patch

src/utils/permission_utils.cpp#L48

Added line #L48 was not covered by tests
ec,
fmt::format("System error occurred while handling permissions for '{}'", path.string()));

Check warning on line 50 in src/utils/permission_utils.cpp

View check run for this annotation

Codecov / codecov/patch

src/utils/permission_utils.cpp#L50

Added line #L50 was not covered by tests
}

// recursively iterates over all files in folder and applies a function that takes a path
template <class Func>
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) [[unlikely]]
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 PrivatePass& pass) noexcept : 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) const
{
apply_on_files(path, [&](const fs::path& apply_path) { set_single_owner(apply_path); });
}

void mp::PermissionUtils::restrict_permissions(const fs::path& path) const
{
apply_on_files(path, [&](const fs::path& apply_path) {
set_single_owner(apply_path);
set_single_permissions(apply_path, QFile::ReadOwner | QFile::WriteOwner | QFile::ExeOwner);
});
}
3 changes: 2 additions & 1 deletion src/utils/utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include <multipass/ssh/ssh_session.h>
#include <multipass/standard_paths.h>
#include <multipass/utils.h>
#include <multipass/utils/permission_utils.h>

#include <QDir>
#include <QFileInfo>
Expand Down Expand Up @@ -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;
Expand Down
3 changes: 3 additions & 0 deletions src/utils/vm_image_vault_utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
*/

#include <multipass/format.h>
#include <multipass/platform.h>
#include <multipass/utils/permission_utils.h>
#include <multipass/vm_image_host.h>
#include <multipass/vm_image_vault.h>
#include <multipass/xz_image_decoder.h>
Expand Down Expand Up @@ -45,6 +47,7 @@ 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);

return new_path;
}

Expand Down
1 change: 1 addition & 0 deletions tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
47 changes: 47 additions & 0 deletions tests/mock_permission_utils.h
Original file line number Diff line number Diff line change
@@ -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 <http://www.gnu.org/licenses/>.
*
*/

#ifndef MULTIPASS_MOCK_PERMISSION_UTILS_H
#define MULTIPASS_MOCK_PERMISSION_UTILS_H

#include "common.h"
#include "mock_singleton_helpers.h"

#include <multipass/utils/permission_utils.h>

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), (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
5 changes: 5 additions & 0 deletions tests/mock_platform.h
Original file line number Diff line number Diff line change
Expand Up @@ -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(int, chmod, (const char*, unsigned int), (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, 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));
Expand Down
5 changes: 5 additions & 0 deletions tests/test_alias_dict.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -607,6 +608,10 @@ struct DaemonAliasTestsuite

mpt::MockSettings::GuardedMock mock_settings_injection = mpt::MockSettings::inject<StrictMock>();
mpt::MockSettings& mock_settings = *mock_settings_injection.first;

const mpt::MockPermissionUtils::GuardedMock mock_permission_utils_injection =
mpt::MockPermissionUtils::inject<NiceMock>();
mpt::MockPermissionUtils& mock_permission_utils = *mock_permission_utils_injection.first;
};

TEST_P(DaemonAliasTestsuite, purge_removes_purged_instance_aliases_and_scripts)
Expand Down
5 changes: 5 additions & 0 deletions tests/test_client_common.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -60,6 +61,10 @@ struct TestClientCommon : public mpt::DaemonTestFixture
std::make_unique<NiceMock<mpt::MockCertProvider>>()};
std::unique_ptr<mpt::MockCertStore> mock_cert_store{std::make_unique<mpt::MockCertStore>()};

const mpt::MockPermissionUtils::GuardedMock mock_permission_utils_injection =
mpt::MockPermissionUtils::inject<NiceMock>();
mpt::MockPermissionUtils& mock_permission_utils = *mock_permission_utils_injection.first;

const std::string server_address{"localhost:50052"};
mpt::TempDir temp_dir;
};
Expand Down
Loading
Loading