Make changes to /etc/passwd on disk for non-read only #6991

mheon · 2020-07-15T20:01:51Z

Bind-mounting /etc/passwd into the container is problematic becuase of how system utilities like useradd work. They want to make a copy and then rename to try to prevent breakage; this is, unfortunately, impossible when the file they want to rename is a bind mount. The current behavior is fine for read-only containers, though, because we expect useradd to fail in those cases.

Instead of bind-mounting, we can edit /etc/passwd in the container's rootfs. This is kind of gross, because the change will show up in podman diff and similar tools, and will be included in images made by podman commit. However, it's a lot better than breaking important system tools.

Fixes #6953

openshift-ci-robot · 2020-07-15T20:01:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mheon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

edsantiago

Nice work. A few questions for you to ponder.

edsantiago · 2020-07-15T20:15:36Z

libpod/container_internal_linux.go

+	}
+	if err == nil {
+		return "", nil
+	}


Could these two ifs be refactored into one?

if err != User.ErrNoPasswdEntries { return "", err }

(I find it easier to read with the err == nil case handled explicitly, but either is fine.)

edsantiago · 2020-07-15T20:18:42Z

libpod/container_internal_linux.go

+		}
+	}
+	if hasHomedir {
+		homeDir = u.HomeDir


Would it make sense to set WORKINGDIR here too (if unset)?

$ ./bin/podman run -it --rm --userns=keep-id --security-opt label=disable -v /home/esm:/home/esm alpine_labels sh / $ pwd / <--- I kind of expected /home/esm / $ tail -1 /etc/passwd esm:x:14904:14904:esm:/home/esm:/bin/sh <--- it was detected / $ ls -la /home/esm [as expected] <---- and it exists

I'm reluctant to reset workdir given it could have been deliberately set by the user - I don't have a way of telling that this deep in the code.

If workingdir == "" then we know it was never set.

By this point, in the spec, it's already been defaulted to / if it was not set by the user.

edsantiago · 2020-07-15T20:22:33Z

libpod/container_internal_linux.go

-	return fmt.Sprintf("%s:x:%s:%s:%s:%s:/bin/sh\n", u.Username, u.Uid, u.Gid, u.Username, c.WorkingDir()), nil
+
+	// Lookup the user to see if it exists in the container image.
+	_, err = lookup.GetUser(c.state.Mountpoint, u.Username)


Corner case: would it make sense to do lookup by UID instead of name? This is super-unlikely, but if the podman user has a UID already defined in the image's /etc/passwd, appending a new entry is going to be mostly a NOP: ls and ps and anything that does lookups is going to show the preexisting username.

I considered that, and decided it's probably not worth the potential performance hit of a duplicate lookup.

rhatdan · 2020-07-15T20:46:32Z

Nice work @mheon, I was thinking of doing this myself. Glad you did it.

rhatdan · 2020-07-15T20:48:20Z

Does this work for both use cases.

if I specify a --user 1234, we add an entry for 1234 to /etc/passwd.

If you use --userns=keepid we add your entry to /etc/passwd.

mheon · 2020-07-15T20:52:13Z

Should work for both - this happens after we've already generated both entries for /etc/passwd and the only difference is where we write them to (a new file or the existing passwd)

rhatdan · 2020-07-15T21:57:29Z

@owtaylor FYI

TomSweeneyRedHat

Tests don't look too hip.
The changes LGTM for what you're trying to do, however, mucking around with /etc/passwd gives me the heebie-jeebies. I'll nod to @rhatdan and @mtrmac on the safeness of that.

mheon · 2020-07-16T15:32:34Z

Added a further test and un-broke the existing passwd tests (used read-only mode to force us back to the old bind mount behavior).

mtrmac · 2020-07-16T17:41:01Z

libpod/container_internal_linux.go

-	return fmt.Sprintf("%s:x:%s:%s:%s:%s:/bin/sh\n", u.Username, u.Uid, u.Gid, u.Username, c.WorkingDir()), nil
+
+	// Lookup the user to see if it exists in the container image.
+	_, err = lookup.GetUser(c.state.Mountpoint, u.Username)


(A pedantic note no-one should care about: POSIX does allow all-numeric usernames, with the username different from the UID value. In user input like the chown CLI, where the input is ambiguous, the input is defined to be interpreted as a username if it exists, in preference to UID.

Either way lookup.GetUser parsing the string and guessing whether the caller meant a name or an UID does the wrong thing, when we know the input is a name.)

mtrmac · 2020-07-16T17:41:51Z

libpod/container_internal_linux.go

-	return fmt.Sprintf("%s:x:%s:%s:%s:%s:/bin/sh\n", u.Username, u.Uid, u.Gid, u.Username, c.WorkingDir()), nil
+
+	// Lookup the user to see if it exists in the container image.
+	_, err = lookup.GetUser(c.state.Mountpoint, u.Username)


What if the user does exist inside the container, but with a different UID?

I think we end up with duplicate entries in that case, with this one being the second of the two. I don't think that's completely disastrous, but it's definitely not ideal.

mtrmac · 2020-07-16T17:47:01Z

libpod/container_internal_linux.go

+	}
+
+	// If the user's actual home directory exists, or was mounted in - use
+	// that.


This is interesting, but somewhat out of scope.

Is it safe? I can easily imagine containers that assume that ~ == WORKDIR.

Also, is it OK to give the container this information about the outside? There is the precedent of telling the container the user name already, sure.

The primary case I was trying to capture here was pet containers (Toolbox) which explicitly bind-mount in the user's /home. I think we're safe to set in that case, given the user's /home was mounted in explicitly at container creation.

Dropping the case where we set /home if it exists in the container rootfs would simplify things and prevent the leakage when the home directory was not explicitly added, so I think I'll do that.

Update: Dropped down to just an explicit mount.

mheon · 2020-07-17T20:00:47Z

Finally green

mheon · 2020-07-23T17:42:24Z

@rhatdan @vrothberg @TomSweeneyRedHat @jwhonce PTAL and merge

Bind-mounting /etc/passwd into the container is problematic becuase of how system utilities like `useradd` work. They want to make a copy and then rename to try to prevent breakage; this is, unfortunately, impossible when the file they want to rename is a bind mount. The current behavior is fine for read-only containers, though, because we expect useradd to fail in those cases. Instead of bind-mounting, we can edit /etc/passwd in the container's rootfs. This is kind of gross, because the change will show up in `podman diff` and similar tools, and will be included in images made by `podman commit`. However, it's a lot better than breaking important system tools. Fixes containers#6953 Signed-off-by: Matthew Heon <[email protected]>

rhatdan · 2020-07-23T19:35:24Z

LGTM
I think we should think of some corner cases like @mtrmac Talks about, but we have those already.

rhatdan · 2020-07-29T08:58:53Z

@vrothberg @giuseppe @baude @jwhonce @TomSweeneyRedHat @ashley-cui @QiWang19 PTAL

edsantiago · 2020-07-29T16:22:53Z

LGTM

rhatdan · 2020-07-29T18:24:08Z

/lgtm

- New test for containers#6991 - passwd file is writable even when run with --userns=keep-id - Enable another keep-id test, commented out due to containers#6593 - New test for podman system df Also, independently, removed this line: apt-get -y upgrade conmon ...because it's causing CI failures, probably because of the boothole CVE, probably because the Ubuntu grub update was rushed out. I believe it is safe to remove this, because both Ubuntu 19 and 20 report: conmon is already the newest version (2.0.18~1). Signed-off-by: Ed Santiago <[email protected]>

openshift-ci-robot requested review from umohnani8 and vrothberg July 15, 2020 20:01

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 15, 2020

edsantiago reviewed Jul 15, 2020

View reviewed changes

mheon force-pushed the change_passwd_ondisk branch from b0beef7 to 131b6a7 Compare July 15, 2020 20:28

TomSweeneyRedHat reviewed Jul 16, 2020

View reviewed changes

mheon force-pushed the change_passwd_ondisk branch from 131b6a7 to a804ef4 Compare July 16, 2020 15:31

mheon force-pushed the change_passwd_ondisk branch from a804ef4 to 325acd2 Compare July 16, 2020 17:12

mtrmac reviewed Jul 16, 2020

View reviewed changes

mheon force-pushed the change_passwd_ondisk branch 3 times, most recently from 6632218 to beab905 Compare July 17, 2020 18:23

mheon force-pushed the change_passwd_ondisk branch from beab905 to bae6853 Compare July 23, 2020 18:27

openshift-ci-robot assigned rhatdan Jul 29, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2020

openshift-merge-robot merged commit 044a7cb into containers:master Jul 29, 2020

edsantiago mentioned this pull request Jul 29, 2020

System tests: new system-df and passwd tests #7151

Merged

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 24, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make changes to /etc/passwd on disk for non-read only #6991

Make changes to /etc/passwd on disk for non-read only #6991

mheon commented Jul 15, 2020

openshift-ci-robot commented Jul 15, 2020

edsantiago left a comment

edsantiago Jul 15, 2020

mtrmac Jul 16, 2020

edsantiago Jul 15, 2020

mheon Jul 15, 2020

rhatdan Jul 15, 2020

mheon Jul 15, 2020

rhatdan Jul 24, 2020

edsantiago Jul 15, 2020

mheon Jul 15, 2020

rhatdan commented Jul 15, 2020

rhatdan commented Jul 15, 2020

mheon commented Jul 15, 2020

rhatdan commented Jul 15, 2020

TomSweeneyRedHat left a comment

mheon commented Jul 16, 2020

mtrmac Jul 16, 2020

mtrmac Jul 16, 2020

mheon Jul 23, 2020

mtrmac Jul 16, 2020

mheon Jul 17, 2020

mheon Jul 23, 2020

mheon commented Jul 17, 2020

mheon commented Jul 23, 2020

rhatdan commented Jul 23, 2020

rhatdan commented Jul 29, 2020

edsantiago commented Jul 29, 2020

rhatdan commented Jul 29, 2020

Make changes to /etc/passwd on disk for non-read only #6991

Make changes to /etc/passwd on disk for non-read only #6991

Conversation

mheon commented Jul 15, 2020

openshift-ci-robot commented Jul 15, 2020

edsantiago left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Jul 15, 2020

rhatdan commented Jul 15, 2020

mheon commented Jul 15, 2020

rhatdan commented Jul 15, 2020

TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

mheon commented Jul 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mheon commented Jul 17, 2020

mheon commented Jul 23, 2020

rhatdan commented Jul 23, 2020

rhatdan commented Jul 29, 2020

edsantiago commented Jul 29, 2020

rhatdan commented Jul 29, 2020