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.

Fixes #3866

MULTI-1403
MULTI-1723

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

Codecov Report

Attention: Patch coverage is 89.58333% with 5 lines in your changes missing coverage. Please review.

Please upload report for BASE (fix-platform-specific-functions-in-abstraction@48de023). Learn more about missing BASE report.

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

Additional details and impacted files

@@                                Coverage Diff                                @@
##             fix-platform-specific-functions-in-abstraction    #3715   +/-   ##
=================================================================================
  Coverage                                                  ?   88.99%           
=================================================================================
  Files                                                     ?      256           
  Lines                                                     ?    14625           
  Branches                                                  ?        0           
=================================================================================
  Hits                                                      ?    13015           
  Misses                                                    ?     1610           
  Partials                                                  ?        0

☔ 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.

ricab · 2025-01-15T12:21:45Z

I meant to review this but I am a little bogged down with reviews right now. This has 2 reviewers already, so I'll leave it up to them.

# Conflicts: # tests/linux/test_platform_linux.cpp

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 2 times, most recently from 3af663c to bafbd01 Compare December 4, 2024 17:28

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

Sploder12 force-pushed the change-permissions-on-instances branch from d97135f to 36e630f Compare December 17, 2024 20:37

Sploder12 changed the base branch from main to fix-platform-specific-functions-in-abstraction December 17, 2024 21:23

Sploder12 force-pushed the fix-platform-specific-functions-in-abstraction branch from 5b088d8 to d50d4e4 Compare January 6, 2025 22:11

Sploder12 force-pushed the change-permissions-on-instances branch 2 times, most recently from 2d9bed1 to 477d0b9 Compare January 7, 2025 21:05

Sploder12 marked this pull request as ready for review January 9, 2025 18:24

Sploder12 force-pushed the fix-platform-specific-functions-in-abstraction branch from 024baf7 to 48de023 Compare January 17, 2025 21:10

[daemon] Change permissions for instances to root only

ed77c70

Sploder12 added 15 commits January 17, 2025 16:23

[lxd] Change instance image permissions to root only

16283f4

[tests] Add permission related tests for cloud_init

f50e4c8

[permissions] Add permission utils

4c07be2

[tests] Add permission utils tests

fedfa74

[daemon] Restrict permissions on data and cache dirs

0b8949b

[permissions] Remove root option from take ownership

5fa30e7

[tests] Change utils to use permission utils

eb4407b

[format] Fix formatting broken by rebase conflicts

b935c17

Address some review comments

686405a

Unify after rebase

be83bc0

Readd permission setting to new files/folders

c3b60ce

[permissions] Remove all but top-level permission change

4cdf1b8

[permissions] Use umask on unix

3997885

Change Qt permissions to std

b3024f5

# Conflicts: # tests/linux/test_platform_linux.cpp

Remove leftover Qt permissions

4a1b649

Sploder12 force-pushed the change-permissions-on-instances branch from 477d0b9 to 4a1b649 Compare January 17, 2025 21:42

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

ricab commented Jan 15, 2025

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

ricab commented Jan 15, 2025

Sploder12 commented Oct 4, 2024 •

edited

Loading

codecov bot commented Oct 21, 2024 •

edited

Loading

ricab commented Dec 2, 2024 •

edited

Loading