WIP: persist per-user mount namespace profile #6341
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
When snap-confine constructs a per-user mount namespace it delegates the task to snap-update-ns invoked with a special option. The code in snap-update-ns switches to the appropriate user ID while retaining some capabilities required for mounting. Before snap-update-ns was never saving the per-user mount profile so it didn't need anything. Now it may start saving it (when the appropriate feature is enabled) but it runs under a non-root UID. To allow the save to really happen allow the file path pattern via apparmor template of snap-update-ns and retain more capabilities to bypass filesystem checks. Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
There is a shared code path for system and user profiles. Testing should be easier, there are many unit-testable elements now. User mount profile is only saved if the corresponding feature is enabled. Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
When snap-confine works with .mnt files (preserved mount namespaces) it usually handles them being absent just fine, only creating them when really strictly necessary (when preserving a mount namespace). While working on some tests I noticed that the per-user mount namespace file would be created (a empty placeholder file) even if that feature is enirely disabled. While the file doesn't "hurt" it was not my intention before. This patch fixes this issue. Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
This way snap-update-ns can freeze the set of snap processes without freezing itself. Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
… user This will allow snap-update-ns to understand proper locking semantics when invoked from snap-confine (it will check instead of locking). Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
|
@zyga can this be split somehow? it triggers my too big PR detector |
|
Yes, certainly! I wanted to get it all out to see it pass and have the full context. There are several small patches I can propose separately easily. The whole refactoring can be introduced in stages easily as well but I will have to chop my "refactor" patch that grew too large as I was coding this. With the logic in this branch the only remaining part of updated user mounts is change to snapd where |
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
|
NOTE: Reviewers, this is a meta, control branch. It shows all of the things working together, it shows all of the changes that remain to be merged. As changes are landing via Please focus your review on the other, much smaller branches, consulting with this one for context if necessary. |
We will soon want to be able to switch to a specific user and group identifiers, not just the real ones. This use case will be used when snapd is performing interface-triggered mount namespace update for per-user mount namespaces. It will then run snap-update-ns with more arguments to indicate that per-user mount namespace is being updated and specify the user ID of user in question. Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
The system wide and per-user mount profiles share a lot of the application logic: in both cases the system needs some kind of locking done to avoid races, the current and desired profile needs to be loaded from disk, the sequence of changes needs to be computed and applied, lastly the updated current profile needs to be saved. Up until recently the per-user mount profile was never stored or updated, just applied to an ephemeral mount namespace. This property is not changed by this patch yet. Everything is shifted into place to make that possible with minimal changes later. All of the logic that performs the mount namespace update outlined above is moved to executeMountProfileUpdate which takes an interface that abstracts the various elements of the process. The system and user update logic is refactored to offer that functionality accordingly. This decomposition also allows each of the elements to be easily tested in isolation, without having to setup extensive mocking or write to disk each time. Much of the common aspects of the system and user update code is further moved to commonProfileUpdate, offering single code for locking, loading and saving profiles, computing and applying mount changes. System and user module as well as the common application glue are tested with new unit tests, focusing on the key aspects of the differentiation. The code no longer mixes tests for how to compute profile update, how to apply profile update with every little other aspect that was previously tested before. Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
…to feature/user-mount-ns-20
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
When snap-update-ns is invoked with the (currently unused by snapd)
command line option `-u` it is an indication that we are going to run as
the specified user ID. In addition the option `--from-snap-confine` is
no longer attached to `--user-fstab` and now only implies that namespace
has already been set and that lock is already established.
This allows snap-update-ns to be used from two different contexts to
achieve distinct tasks:
1) When invoked from snap-confine, it is always paired with
`--from-snap-confine`. This, as mentioned, indicates that the mount
namespace is already unshared and that locks are held by
snap-confine.
In this mode we can be called with no further options, to initialize
the system-wide snap mount namespace or with `--user-mounts` to
initialize the per-user snap mount namespace. Note that at this time
`-u` is redundant because snap-confine already runs with the
appropriate user ID.
2) When invoked from snapd it is invoked with or without `-u` and
`--user-mounts`. When those options are absent `snap-update-ns` will
acquire appropriate locks, freeze running processes of the affected
snap and perform an update to existing system-wide mount namespace
that is joined, as appropriate. When those options are used the same
flow will happen with the exception that per-user mount namespace
will be joined and updated, running as the specified user ID.
This allows using snap-update-ns to update existing, persisted per-user
mount namespaces.
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
zyga
changed the title
|
This branch needs to be rebased on top of master once 2/3 of the contents here lands in via other PRs. It contains essential features that are not proposed elsewhere. |
…to feature/user-mount-ns-20 I'm not 100% this is correct, it's been a long while and this WIP branch wasn't entirely how I remember it. Perhaps there's some more on another machine of my somewhere.
… feature/user-mount-ns-20
|
I've merged #6360 and master into this branch. The delta is now significantly smaller and I think it can be used as a base for subsequent work to carve out useful patches and propose for master. |
|
Closing to deal with master woes and refocus. Please make sure not to remove this branch though :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This branch refactors the distinction between system-wide and per-user mount profile
update logic, adds lots of new unit tests and unifies a lot of the workflow between the two.
When the per-user-mount-namespace feature is enabled the mount namespace is persisted
by snap-confine but the per-user mount profile (aka current profile) was not saved. This branch
implements this logic.
Most of the old code is intact, just reorganised for easier testing and layering. There are minimal
changes to snap-confine (to change when the freezer cgroup is joined by the spawned process
so that snap-update-ns is not itself freezing), to the snap-update-ns apparmor template to allow
writing the per-user, current profile and to osutil package to add a helper function.
Existing spread tests pass without hiccups. The per-user mount namespace test was extended
to cover both the persistence feature being disabled and enabled.
There are several TODOs in the new code, where we should insert code needed if we wish to start
supporting $SNAP_USER_DATA, $SNAP_USER_COMMON or $HOME in per-user mount profiles but
this is not required yet. The main difficulty in implementing that is reliable user ID to user name
mapping as well as reliable lookup of user $HOME directory, that doesn't depend on easy-to-attack
environment vector.