Change file permissions on instances #3715

Sploder12 · 2024-10-04T14:03:27Z

This PR changes file permissions on Multipass cache and data directories to be read/write only by root. Previously the files could be read by all.

MULTI-1403

codecov · 2024-10-21T13:48:57Z

Codecov Report

Attention: Patch coverage is 90.90909% with 4 lines in your changes missing coverage. Please review.

Project coverage is 88.95%. Comparing base (f01004e) to head (d97135f).
Report is 20 commits behind head on main.

Files with missing lines	Patch %	Lines
src/platform/platform_unix.cpp	33.33%	2 Missing ⚠️
src/utils/permission_utils.cpp	94.44%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3715   +/-   ##
=======================================
  Coverage   88.94%   88.95%           
=======================================
  Files         256      257    +1     
  Lines       14584    14626   +42     
=======================================
+ Hits        12972    13010   +38     
- Misses       1612     1616    +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

levkropp

Looks good, it seems that codecov wants to see test coverage for some lines:

if (!MP_PLATFORM.set_root_as_owner(path)) in write_to
extract_image doesn't seem to have tests for it at all?

Sploder12 · 2024-11-26T21:47:29Z

I have decided that the vm_image_vault tests are out of the scope of this PR. This is because writing tests would require a significant refactor of unrelated code (see 1653 in Jira).

sharder996

Thanks for your work on this @Sploder12! Tackling these parts of the code can be tricky :)

Since this PR is closely related with #3782, there might be some overlap. But, here are some thoughts I had reviewing this:

Existing instance directories/files do not have their permissions changed, only new instances.
Is there a reason to apply permissions specifically to cloud_init_iso and instance directories and not to the other files stored within the data_directory and cache_directory?
Would it be better/easier to apply blanket permissions on the storage_directory/cache_directory/data_directory in daemon_config? That would solve 1 if it doesn't interfere with 2.
Instead of the name set_root_as_owner(), how about takeown()? If need be, it could default to root/admin with an option to set ownership to the current user. That would follow pretty closely the Windows definition of takeown.

Sploder12 · 2024-11-29T22:16:42Z

@sharder996

Good point! Updating the permissions when the daemon starts is what I'm thinking to fix that, WDYT?
Some files aren't as sensitive, but it might be worth covering those too, @ricab might have more thoughts on this.
That might work, I'll give it a try
I don't like takeown personally, reminds me too much of runic C string functions like wcsxfrm. But something like take_ownership or set_owner could be nice for that. WDYT?

ricab · 2024-12-02T10:37:43Z

Some files aren't as sensitive, but it might be worth covering those too, @ricab might have more thoughts on this.

I guess covering the whole directory would be safer for the future and agree with principle of least privilege better. We can always expose any thing we need on a case by case basis.

sharder996

Initial review pass and I noticed some things that I think could be improved. Will make a more detailed review afterwards.

By setting permissions on the top level data/cache directories, all sub-directories should inherit those permissions, correct? If so, some of the code could be cleaned up.

include/multipass/utils/permission_utils.h

sharder996 · 2024-12-11T18:06:02Z

src/daemon/daemon_config.cpp

@@ -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());


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

sharder996 · 2024-12-11T18:46:51Z

src/daemon/daemon_config.cpp

+    else
+    {
+        MP_PERMISSIONS.restrict_permissions(data_directory.toStdU16String());
+        MP_PERMISSIONS.restrict_permissions(cache_directory.toStdU16String());


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

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.

sharder996 · 2024-12-11T18:49:19Z

src/daemon/default_vm_image_vault.cpp

@@ -669,7 +670,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);


If the top level data_directory and cache_directory are having their permissions and ownership set at daemon start, is there a necessity to individually set permissions and ownership of every instance directory?

sharder996 · 2024-12-11T19:04:30Z

src/utils/permission_utils.cpp

+{
+void set_single_permissions(const Path& path, const QFileDevice::Permissions& permissions)
+{
+    QString qpath = QString::fromUtf8(path.u8string());


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

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.

Sploder12 marked this pull request as draft October 4, 2024 19:31

ricab added this to the 1.15.0 milestone Oct 7, 2024

Sploder12 marked this pull request as ready for review October 21, 2024 13:22

Sploder12 force-pushed the change-permissions-on-instances branch from 5a257a8 to 1e245a5 Compare November 13, 2024 17:53

levkropp requested changes Nov 26, 2024

View reviewed changes

Sploder12 marked this pull request as draft November 26, 2024 21:19

Sploder12 marked this pull request as ready for review November 26, 2024 21:42

sharder996 requested changes Nov 29, 2024

View reviewed changes

ricab modified the milestones: 1.15.0, 1.15.1 Nov 29, 2024

Sploder12 marked this pull request as draft December 2, 2024 17:57

Sploder12 force-pushed the change-permissions-on-instances branch from 7216696 to 3af663c Compare December 4, 2024 17:12

Sploder12 added 7 commits December 4, 2024 12:28

[daemon] Change permissions for instances to root only

5d9e63b

[lxd] Change instance image permissions to root only

339185b

[tests] Add permission related tests for cloud_init

375fa14

[permissions] Add permission utils

fc86223

[tests] Add permission utils tests

af61c74

[daemon] Restrict permissions on data and cache dirs

11562ee

[permissions] Remove root option from take ownership

bafbd01

Sploder12 force-pushed the change-permissions-on-instances branch from 3af663c to bafbd01 Compare December 4, 2024 17:28

Sploder12 added 2 commits December 4, 2024 14:06

[tests] Change utils to use permission utils

a6be416

[format] Fix formatting broken by rebase conflicts

377985b

Sploder12 marked this pull request as ready for review December 4, 2024 20:12

Sploder12 requested a review from sharder996 December 9, 2024 21:51

sharder996 requested changes Dec 11, 2024

View reviewed changes

Sploder12 marked this pull request as draft December 12, 2024 16:58

Address some review comments

d97135f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change file permissions on instances #3715

Change file permissions on instances #3715

Sploder12 commented Oct 4, 2024 •

edited

Loading

codecov bot commented Oct 21, 2024 •

edited

Loading

levkropp left a comment

Sploder12 commented Nov 26, 2024

sharder996 left a comment

Sploder12 commented Nov 29, 2024

ricab commented Dec 2, 2024 •

edited

Loading

sharder996 left a comment

sharder996 Dec 11, 2024

sharder996 Dec 11, 2024

Sploder12 Dec 11, 2024

sharder996 Dec 11, 2024

sharder996 Dec 11, 2024

Sploder12 Dec 11, 2024

Change file permissions on instances #3715

Are you sure you want to change the base?

Change file permissions on instances #3715

Conversation

Sploder12 commented Oct 4, 2024 • edited Loading

codecov bot commented Oct 21, 2024 • edited Loading

Codecov Report

levkropp left a comment

Choose a reason for hiding this comment

Sploder12 commented Nov 26, 2024

sharder996 left a comment

Choose a reason for hiding this comment

Sploder12 commented Nov 29, 2024

ricab commented Dec 2, 2024 • edited Loading

sharder996 left a comment

Choose a reason for hiding this comment

sharder996 Dec 11, 2024

Choose a reason for hiding this comment

sharder996 Dec 11, 2024

Choose a reason for hiding this comment

Sploder12 Dec 11, 2024

Choose a reason for hiding this comment

sharder996 Dec 11, 2024

Choose a reason for hiding this comment

sharder996 Dec 11, 2024

Choose a reason for hiding this comment

Sploder12 Dec 11, 2024

Choose a reason for hiding this comment

Sploder12 commented Oct 4, 2024 •

edited

Loading

codecov bot commented Oct 21, 2024 •

edited

Loading

ricab commented Dec 2, 2024 •

edited

Loading