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

user units: implicitly enable PrivateUsers= when sandboxing options are set #25233

Merged
merged 1 commit into from Apr 13, 2023

Conversation

bluca
Copy link
Member

@bluca bluca commented Nov 1, 2022

Enabling these options when not running as root requires a user namespace, so implicitly enable PrivateUsers=.

@bluca bluca changed the title user units: implicitly enable PrivateUsers= when sandboxing options a… user units: implicitly enable PrivateUsers= when sandboxing options are set Nov 1, 2022
src/core/service.c Outdated Show resolved Hide resolved
src/core/service.c Outdated Show resolved Hide resolved
src/core/service.c Outdated Show resolved Hide resolved
@yuwata yuwata added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR labels Nov 2, 2022
src/core/service.c Outdated Show resolved Hide resolved
src/core/service.c Outdated Show resolved Hide resolved
src/core/service.c Outdated Show resolved Hide resolved
src/core/service.c Outdated Show resolved Hide resolved
@bluca bluca removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR labels Nov 2, 2022
@poettering
Copy link
Member

Would a new PrivateUser=identity concept work for this too? i.e. a userns that is private to the service, but just a full identity mapping?

@bluca
Copy link
Member Author

bluca commented Nov 30, 2022

Would a new PrivateUser=identity concept work for this too? i.e. a userns that is private to the service, but just a full identity mapping?

It turns out it cannot be done from the user manager, as it requires privileges to set up an id map of more than the current uid

@keszybz keszybz added the please-review PR is ready for (re-)review by a maintainer label Dec 8, 2022
@bluca bluca added ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR and removed please-review PR is ready for (re-)review by a maintainer labels Dec 14, 2022
@github-actions github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR labels Dec 26, 2022
@DaanDeMeyer
Copy link
Contributor

Would a new PrivateUser=identity concept work for this too? i.e. a userns that is private to the service, but just a full identity mapping?

It turns out it cannot be done from the user manager, as it requires privileges to set up an id map of more than the current uid

@brauner and @poettering had the idea to add a service to manage uid/gid ranges that would hand out user namespaces with a uid/gid range and idmapping configured to tools and services. Rootless nspawn would make use of this. I wonder if we could use the same service here to get an identity mapping without needing privileges? Not sure about the security implications though.

@bluca
Copy link
Member Author

bluca commented Dec 28, 2022

Would a new PrivateUser=identity concept work for this too? i.e. a userns that is private to the service, but just a full identity mapping?

It turns out it cannot be done from the user manager, as it requires privileges to set up an id map of more than the current uid

@brauner and @poettering had the idea to add a service to manage uid/gid ranges that would hand out user namespaces with a uid/gid range and idmapping configured to tools and services. Rootless nspawn would make use of this. I wonder if we could use the same service here to get an identity mapping without needing privileges? Not sure about the security implications though.

If it worked it could be retrofitted to use it yes - but it can be done later

@bluca
Copy link
Member Author

bluca commented Dec 29, 2022

Would a new PrivateUser=identity concept work for this too? i.e. a userns that is private to the service, but just a full identity mapping?

It turns out it cannot be done from the user manager, as it requires privileges to set up an id map of more than the current uid

@brauner and @poettering had the idea to add a service to manage uid/gid ranges that would hand out user namespaces with a uid/gid range and idmapping configured to tools and services. Rootless nspawn would make use of this. I wonder if we could use the same service here to get an identity mapping without needing privileges? Not sure about the security implications though.

mmh I tried this, but when the receiving end of the namespace FD is not the same uid, I get -EPERM back from setns(fd, CLONE_NEWUSER), any idea what else needs to be done?

@brauner
Copy link
Contributor

brauner commented Dec 29, 2022

Would a new PrivateUser=identity concept work for this too? i.e. a userns that is private to the service, but just a full identity mapping?

It turns out it cannot be done from the user manager, as it requires privileges to set up an id map of more than the current uid

@brauner and @poettering had the idea to add a service to manage uid/gid ranges that would hand out user namespaces with a uid/gid range and idmapping configured to tools and services. Rootless nspawn would make use of this. I wonder if we could use the same service here to get an identity mapping without needing privileges? Not sure about the security implications though.

mmh I tried this, but when the receiving end of the namespace FD is not the same uid, I get -EPERM back from setns(fd, CLONE_NEWUSER), any idea what else needs to be done?

Just summarizing what I explained elsewhere just now. To make this work you should have the acquire_userns() function setuid to the client's uid that requested the userns creation, then create the userns, and then have the parent write the mapping that you want. This ensures that the client owns the userns. It's also important that the client and the creator of the userns are located in the same userns - the actual restriction is a bit more detailed but it doesn't matter here - so if you do nested userns creation then you should take the client's userns into account so the parent userns relationship is correctly set up. Also, you should be ON HOLIDAY. STEP AWAY FROM THE COMPUTER.

@bluca
Copy link
Member Author

bluca commented Dec 29, 2022

Also, you should be ON HOLIDAY. STEP AWAY FROM THE COMPUTER.

That's why I have time to do the cool stuff! Beer-fueled hacking go brrrr

Copy link
Member

@keszybz keszybz left a comment

Choose a reason for hiding this comment

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

I think we should merge this.

The only open issue is how to handle ProtectKernelModules= and other settings that are partially implemented via seccomp. Lennart's approach gives slightly better backwards-compat, and Luca's approach (current patch) gives slightly more reasonable behaviour for users. I think we should be fine with the latter because of the preparatory work that was done: queries in distros and on the mailing list. The expectation is that that units that would be affected by this are very rare or nonexistent. Of course we will need to announce this in NEWS, etc, but I think we can get away with this. Let's merge this now, while there is still some time before the next release, to give it some time to "bake".

Needs a rebase and typo fixes.

man/system-or-user-ns.xml Outdated Show resolved Hide resolved
man/system-or-user-ns.xml Outdated Show resolved Hide resolved
@bluca bluca added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed needs-rebase good-to-merge/with-minor-suggestions labels Apr 12, 2023
@keszybz
Copy link
Member

keszybz commented Apr 12, 2023

CentOS CI (Arch Linux) fails deep in TEST-75-RESOLVED. But I don't think this is related, the unit is up and running when the failure happens.

…re set

Enabling these options when not running as root requires a user
namespace, so implicitly enable PrivateUsers=.
This has a side effect as it changes which users are visible to the unit.
However until now these options did not work at all for user units, and
in practice just a handful of user units in Fedora, Debian and Ubuntu
mistakenly used them (and they have been all fixed since).

This fixes the long-standing confusing issue that the user and system
units take the same options but the behaviour is wildly (and sometimes
silently) different depending on which is which, with user units
requiring manually specifiying PrivateUsers= in order for sandboxing
options to actually work and not be silently ignored.
@keszybz
Copy link
Member

keszybz commented Apr 13, 2023

jammy-ppc64el: TEST-29-PORTABLE: FAIL (1809 s)

@bluca bluca merged commit 6ef721c into systemd:main Apr 13, 2023
44 of 47 checks passed
@bluca bluca deleted the user_units_private_users branch April 13, 2023 20:33
@github-actions github-actions bot removed the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Apr 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

7 participants