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

[daemon] add local.privileged-mounts setting #2150

Merged
merged 9 commits into from Jul 1, 2021
Merged

Conversation

Saviq
Copy link
Collaborator

@Saviq Saviq commented Jun 28, 2021

@Saviq Saviq requested a review from ricab June 28, 2021 11:56
@codecov
Copy link

codecov bot commented Jun 28, 2021

Codecov Report

Merging #2150 (7e40c30) into main (c024274) will decrease coverage by 0.05%.
The diff coverage is 41.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2150      +/-   ##
==========================================
- Coverage   82.14%   82.08%   -0.06%     
==========================================
  Files         184      185       +1     
  Lines        9536     9574      +38     
==========================================
+ Hits         7833     7859      +26     
- Misses       1703     1715      +12     
Impacted Files Coverage Δ
include/multipass/platform.h 100.00% <ø> (ø)
src/daemon/daemon.cpp 54.58% <25.00%> (+0.25%) ⬆️
src/utils/settings.cpp 22.09% <50.00%> (ø)
src/client/cli/cmd/launch.cpp 76.52% <100.00%> (+0.20%) ⬆️
src/platform/platform_linux.cpp 91.87% <100.00%> (+0.10%) ⬆️
include/multipass/exceptions/settings_exceptions.h 77.77% <0.00%> (-22.23%) ⬇️
include/multipass/settings.h 80.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c024274...7e40c30. Read the comment docs.

This way instances with mounts defined prior to disabling of the feature
will report why there's no mounts on `info` and `start`.
Copy link
Contributor

@ricab ricab left a comment

Choose a reason for hiding this comment

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

Thanks I have a few small things inline, but this looks good and works well overall (I have yet to try the counterpart though).

A couple points that I think are worth signaling:

  • If primary is launched with mounts disabled, it doesn't get the Home mount, even if the setting is later enabled. I suppose this is as expected, and kind of consistent with what happens when changing the name of the primary instance to an instance that already existed (it keeps its mounts, or absence thereof).
  • The security of this approach relies on the daemon settings not being writable (just deletable on Linux/Mac). If someone was able to edit DAEMON_CONFIG_HOME on Linux, change the permissions of certain Windows folders, or even change the name of the multipass daemon, they could have a chance at changing the setting. All of those should again require elevated privileges, but something we need to bear in mind in future changes.

tests/test_cli_client.cpp Outdated Show resolved Hide resolved
tests/test_cli_client.cpp Show resolved Hide resolved
src/client/cli/cmd/launch.cpp Outdated Show resolved Hide resolved
src/daemon/daemon.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@Saviq Saviq left a comment

Choose a reason for hiding this comment

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

* If primary is launched with mounts disabled, it doesn't get the Home mount, even if the setting is later enabled. I suppose this is as expected, and kind of consistent with what happens when changing the name of the primary instance to an instance that already existed (it keeps its mounts, or absence thereof).

Yeah, that's by design.

* The security of this approach relies on the daemon settings not being writable (just deletable on Linux/Mac). If someone was able to edit `DAEMON_CONFIG_HOME` on Linux, change the permissions of certain Windows folders, or even change the name of the multipass daemon, they could have a chance at changing the setting. All of those should again require elevated privileges, but something we need to bear in mind in future changes.

Yup, this relies on multipass set local.* requiring privileged operation. But it's not special in that we don't want any local.* settings being modifiable without privileges.

src/client/cli/cmd/launch.cpp Outdated Show resolved Hide resolved
src/daemon/daemon.cpp Outdated Show resolved Hide resolved
tests/test_cli_client.cpp Show resolved Hide resolved
Copy link
Contributor

@ricab ricab left a comment

Choose a reason for hiding this comment

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

Looks good, thanks.

@ricab
Copy link
Contributor

ricab commented Jul 1, 2021

bors r+

bors bot added a commit that referenced this pull request Jul 1, 2021
2150: [daemon] add `local.privileged-mounts` setting r=ricab a=Saviq



Co-authored-by: Michał Sawicz <michal.sawicz@canonical.com>
@bors
Copy link
Contributor

bors bot commented Jul 1, 2021

Build failed:

@ricab
Copy link
Contributor

ricab commented Jul 1, 2021

bors retry

bors bot added a commit that referenced this pull request Jul 1, 2021
2150: [daemon] add `local.privileged-mounts` setting r=ricab a=Saviq



Co-authored-by: Michał Sawicz <michal.sawicz@canonical.com>
@bors
Copy link
Contributor

bors bot commented Jul 1, 2021

Build failed:

@ricab ricab merged commit 010fa9b into main Jul 1, 2021
11 of 14 checks passed
@bors bors bot deleted the add-mounts-setting branch July 1, 2021 14:14
Saviq pushed a commit that referenced this pull request Jul 6, 2021
[daemon] add `local.privileged-mounts` setting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants