Yes I did try that and it does fix the issue.
The question remains: should LXD set up cgroups to be root owned and root writable or should snap-confine have to be setgid.
The reason why we keep the owner as the container’s owning uid rather than uid 0 in the container is that without this, the owner of the container (especially if it’s an unprivileged user) would loose the ability to control the container’s cgroups.
If that’s a possibility for you, yes. I just tested this on our side again for unprivileged container’s you’d effectively loose the ability to attach to the container as an unprivileged user because you can’t move yourself into the cgroup.
Hmm, can you expand on that please. Does that mean regular use of LXD will be broken?
Would it be any better if we didn’t chown? (Note that when we are g+s then the file will be root.root anyway, right?)
Having snap-confine be setgid is possible, though we’d need to verify we are dropping correctly. However, if snap-confine does the chown as setgid, won’t that break container’s ability to control those cgroups? (referring to @stgraber’s point)
I think at at time we can perhaps drop the chown (as all the new files will be root.root anyway and the point of the chown was to make that so (it will now effectively become a no-op).
For LXD it’s not as big a deal since LXD itself runs as root so will always be able to attach regardless of what the container does. This permission trick comes from liblxc which can also be used by entirely unprivileged users in which case, changing /sys/fs/cgroup/freezer or any of the cgroup owner to root:root in the container would indeed prevent attaching or reconfiguring the container.
Anyway, if you run with g+s for snap-confine, that should fix this problem and I don’t expect any side effects.
If we drop the chown, won’t the files be root.user in the non-sudo case?
With g+s they should be root.root, right? We won’t run as user gid;
@stgraber we are not changing /sys/fs/cgroup/freezer, just the particular cgroups inside, is that okay?
Yes, that’s perfectly fine, liblxc only ever attaches or reconfigures the root of the cgroup.
1 Like
Yes, of course. I thought you were saying don’t setgid and don’t chown.
I’m running a spread run with the setgid changes in place. I’ll polish the patch to ensure we don’t do unneeded chowns and propose it for review.
That is fine for a test case, but snap-confine will almost certainly need to be adjusted to run as setgid, so please don’t commit the packaging changes without a security review.
Good teamwork guys. High fives all around!
4 Likes
I pushed a patch on top of the regression test that mvo made earlier. https://github.com/snapcore/snapd/pull/4230/commits/7722c0404b97fa0ac119acb495caa62c3f5ab321
With this patch testing is green for me locally (in qemu running ubuntu 16.04). I pushed it to see if there’s anything surprising on other OSes.
The fix is ready in the 2.29.4 release, however the upload is blocked because the permissions of snap-confine changed: https://launchpad.net/~snappy-dev/+snap/core/+build/108281 - so this needs to be whitelisted and/or approved from the store before this can enter into beta.
It is staged for production but not available yet. In the meantime we are doing manual approvals.