machined userns fixes by poettering · Pull Request #5333 · systemd/systemd

poettering · 2017-02-13T18:58:16Z

Mostly some infrastructure to fix #4078, and eventually the fix for it.

martinpitt

LGTM, except for some nitpicks (feel free to ignore) and a question.

martinpitt · 2017-02-14T21:56:38Z

src/machine/machine-dbus.c

        SD_BUS_METHOD("Kill", "si", NULL, bus_machine_method_kill, SD_BUS_VTABLE_UNPRIVILEGED),
        SD_BUS_METHOD("GetAddresses", NULL, "a(iay)", bus_machine_method_get_addresses, SD_BUS_VTABLE_UNPRIVILEGED),
        SD_BUS_METHOD("GetOSRelease", NULL, "a{ss}", bus_machine_method_get_os_release, SD_BUS_VTABLE_UNPRIVILEGED),
+        SD_BUS_METHOD("GetUIDGIDBase", NULL, "u", bus_machine_method_get_uid_gid_base, SD_BUS_VTABLE_UNPRIVILEGED),


Nitpick: as this doesn't have separate out parameters for uid and gid and is hard to read that way, WDYT about "UGID" instead? Also, Base → Offset or Shift?

Hmm, internally we actually do call this "shift", hence I figure you are right, we should expose that as shift too. Will change.

I figure UID is the relevant bit here, and GIDs ranges are just always kept in sync with UIDs, and generally secondary, hence I figure we should drop the explicit reference to GIDs entirely.

I'll hence change this to become GetUIDShift() now, I hope that makes sense!

martinpitt · 2017-02-14T21:59:19Z

src/machine/machine.c

+                return -EBADMSG;
+        }
+
+        /* Not a mapping statring at 0? Then it's a complex mapping we can't expose here. */


"statring" typo

martinpitt · 2017-02-14T22:02:32Z

src/machine/machine.c

+        if (fgetc(f) != '\n')
+                return -ENXIO;
+        if (fgetc(f) != EOF)
+                return -ENXIO;


The first fgetc() asserts that the first line is always \n terminated. If we can assume that, wouldn't it be easier to read it in the fscanf() above already? If we don't want to assume that, maybe the first fgetc() should check for '\n' or EOF?

true, i added the \n now to the fscanf() format string!

martinpitt · 2017-02-14T22:13:18Z

src/machine/machine-dbus.c

+
+        /* Is user namespacing is used? ifso set the resulting UIDs always to zero */
+        if (uid != 0)
+                copy_flags |= COPY_CHOWN_ROOT;


AFAICS this is run in machined, i. e. the host's user namespace, not the container's. How does this make sure that "root" (uid 0) of the container is being used here, i. e. the shifted UID?

uh, it didn't! I only tested copy from the container, not copy to the container, hence I didn't notice. I reworked this now, to fix it up in both directions!

evverx · 2017-02-15T07:07:13Z

Mostly some infrastructure to fix #4078, and eventually the fix for it

Well, I'm reading https://lists.freedesktop.org/archives/systemd-devel/2016-September/037421.html

This issue is new and have been able to cp/mv from host to container and
preserve file/folders attributes until now. Something in my recent upgrades
have done these changes.

I think the original issue unrelated to machinectl [copy-from|copy-to]. Arnaud Gaboury tried to copy/move some files manually. And this doesn't work.

I guess the docs should contain something like:

you shouldn't even try to cp/mv files from/to `-U`-container manually

Also, I'm not sure how does this PR address bind: #4078 (comment)

poettering · 2017-02-16T13:40:07Z

I have now force pushed a new, rebased version that addresses all the issues raised.

@evverx i also added code to effectively block "machinectl bind" if userns is used, returning a brief explanatory error message. It's the best we can do until the kernel gains a "shiftfs" or similar concept.

I also added the suggested man page addition saying that copying files from/to the container image directly requires patching of UIDs/GIDs.

UID/GID mapping with userns can be arbitrarily complex. Let's break this down to a single admin-friendly parameter: let's expose the UID/GID shift of a container via a new bus call for each container, and let's show this as part of "machinectl status" if it is not 0. This should work for pretty much all real-life full OS container setups (i.e. the stuff machined is suppose to be useful for). For everything else we generate a clean error, clarifying that we can't expose the mapping.

With this change we'll not show an "Addresses" field for machines that we don't know any addresses for. This changes print_addresses() to never suffix its output with a newline, leaving that to the caller. That's a good idea since depending on who the caller is, different rules apply: if no addresses are found, then the list view still wants a newline, but the status view does not. This also changes the function to return the number of found addresses, which can be used to decide when to add a newline or not.

…ameter This adds a unified "copy_flags" parameter to all copy_xyz() function calls, replacing the various boolean flags so far used. This should make many invocations more readable as it is clear what behaviour is precisely requested. This also prepares ground for adding support for more modes later on.

This changes the file copy logic of machined to set the UID/GID of all copied files to 0 if the host and container do not share the same user namespace. Fixes: systemd#4078

Actually initialize the "error" structure with the error we got

…applied As the kernel won't map the UIDs this is simply not safe, and hence we should generate a clean error and refuse it. We can restore this feature later should a "shiftfs" become available in the kernel.

martinpitt

Thanks, the uid/gid adjustment now makes sense, and the other issues from initial review got addressed too. I'm not sure if the "disable bind mount" is really necessary or a good idea, but I just pose the comment and won't block the landing on it. I'd appreciate a rewording of the last commit though :-)

martinpitt · 2017-02-17T10:16:13Z

src/machine/machine-dbus.c

+        if (r < 0)
+                return r;
+        if (uid != 0)
+                return sd_bus_error_setf(error, SD_BUS_ERROR_NOT_SUPPORTED, "Can't bind mount on container with user namespacing applied.");


Hmm, the most likely result of bind-mounting with shifted UIDs is that the container can only read the files, but not write anything into it; this is certainly a common use case, and not particularly unsafe?

Well, information leakage is also a problem. It's not sufficient to say that we prohibit write access, it's also not safe giving read access to files that a process should not have read access to. Worse even: if the bind mount contains suid files, then the mismatch might possibly even permit somebody who shouldn't have privs get privs. I'd rather not get into that and I think it's simpler to simply step away from it... In the long run I hope that shiftfs in the kernel will fix this properly for us.

OK, good point. I have no strong opinion about it, so let's land this and revisit if someone actually complains?

martinpitt · 2017-02-17T10:17:58Z

man/systemd-nspawn.xml

+        <para>Note that when user namespacing is used file ownership on disk reflects this, and all of the container's
+        files and directories are owned by the container's effective user and group IDs. This means that copying files
+        from and to the container image requires correction of the numeric UID/GID values, according to the UID/GID
+        shift applied.</para></listitem>


Could you please change the commit message to use less strong words?

poettering added the machine label Feb 13, 2017

poettering mentioned this pull request Feb 13, 2017

machined: "machinectl copy-from" and "machinectl copy-to" need to adjust uid/gid accordingl to userns if containers uses them #4078

Closed

martinpitt reviewed Feb 14, 2017

View reviewed changes

poettering force-pushed the machined-copy-files-userns branch 2 times, most recently from f3f1226 to e41c6a5 Compare February 16, 2017 13:33

poettering added 6 commits February 17, 2017 10:22

machined: when copying files from/to userns containers chown to root

d01cd40

This changes the file copy logic of machined to set the UID/GID of all copied files to 0 if the host and container do not share the same user namespace. Fixes: systemd#4078

machined: properly propagate long-running operation errors

3aca832

Actually initialize the "error" structure with the error we got

machined: refuse bind mounts on containers that have user namespaces …

7f43928

…applied As the kernel won't map the UIDs this is simply not safe, and hence we should generate a clean error and refuse it. We can restore this feature later should a "shiftfs" become available in the kernel.

poettering force-pushed the machined-copy-files-userns branch from aae614c to d62f6cb Compare February 17, 2017 09:24

martinpitt approved these changes Feb 17, 2017

View reviewed changes

man: document that user namespacing complicates file copies

aa10469

poettering force-pushed the machined-copy-files-userns branch from d62f6cb to aa10469 Compare February 17, 2017 10:47

martinpitt added 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 Feb 17, 2017

poettering merged commit e4363cd into systemd:master Feb 17, 2017

Uh oh!

Conversation

poettering commented Feb 13, 2017

Uh oh!

martinpitt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evverx commented Feb 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poettering commented Feb 16, 2017

Uh oh!

martinpitt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

evverx commented Feb 15, 2017 •

edited

Loading