Setup HOME environment when using --userns=keep-id #8013

rhatdan · 2020-10-13T21:45:58Z

Currently the HOME environment is set to /root if
the user does not override it.

Also walk the parent directories of users homedir
to see if it is volume mounted into the container,
if yes, then set it correctly.

Fixes: #8004

Signed-off-by: Daniel J Walsh [email protected]

TomSweeneyRedHat · 2020-10-13T22:40:07Z

libpod/container_internal_linux.go

-	if MountExists(c.config.Spec.Mounts, u.HomeDir) {
-		homeDir = u.HomeDir
+	hDir := u.HomeDir
+	for hDir != "/" {


u.HomeDir is a string no?

Suggested change

for hDir != "/" {

if hDir != "/" {

This code is attempting to walk up the directories to check if their is a mountpoint from the host that covers this directory.

For example if my homedir was /home/engineering/dwalsh.
I want to check if
/home/engineering/dwalsh
/home/engineering
/home
Is mounted into the container, if yes then I can use /home/engineering/dwalsh as the homedir, rather then CWD.

No, we want to look through the directories.

I don't know if this is really wanted - when I originally wrote this, I wanted it to be specific to ensure that we didn't leak information about the user on the host into the container unless they were explicitly mounting their home

TomSweeneyRedHat · 2020-10-13T22:43:40Z

libpod/container_internal_linux.go

+			homeDir = u.HomeDir
+			break
+		}
+		hDir = filepath.Dir(hDir)


me thinks

Suggested change

hDir = filepath.Dir(hDir)

homeDir = filepath.Dir(hDir)

hDir doesn't seem to be referred to after this line otherwise.

If hDir is volume mounted into the container then

+ homeDir = u.HomeDir + break

Is executed which sets the homeDir to the directory from the host as opposed to the CWD.

edsantiago · 2020-10-14T16:00:19Z

test/e2e/toolbox_test.go

+
+		currentUser, err := user.Current()
+		Expect(err).To(BeNil())
+		session = podmanTest.Podman([]string{"run", "--name", "-v", fmt.Sprintf("%s:%s:Z", currentUser.HomeDir, currentUser.HomeDir), "test", "--userns=keep-id", fedoraToolbox, "sh", "-c", "echo $HOME"})


I haven't looked at the CI failures, but I'm guessing that --name -v is not going to work well

edsantiago · 2020-10-14T16:03:18Z

There's a bug somewhere, I don't have time right now to delve into it:

$ ./bin/podman run --userns=keep-id quay.io/libpod/testimage:20200929 printenv HOME
/home/podman

(my username on this system is 'esm', not 'podman'. There is a valid /home/podman directory on this image, and that is WORKDIR, so that's the likely reason for the default)

UPDATE: I think this goes deeper than just $HOME:

$ ./bin/podman run -it -v /home/esm --userns=keep-id alpine sh
~ $ pwd
/
~$ tail -1 /etc/passwd
esm:*:14904:14904:esm:/:/bin/sh
~$ ls -l /home
total 4
drwxr-xr-x    2 esm      esm           4096 Oct 14 16:45 esm

rhatdan · 2020-10-14T19:34:59Z

@edsantiago
try
./bin/podman run -it -v /home/esm:/home/esm --userns=keep-id alpine sh

Sadly I don't think I can handle just creating the internal /home/dir.

edsantiago · 2020-10-14T19:39:12Z

I was just trying that, and it works. I was curious why plain -v /home/esm (named volume, not a bind mount) didn't work though. But it's not that big a deal. I'm also curious about the testimage case, this is where the image specifies a WORKDIR. I can see not wanting to override that for cwd, but I do have a slight inclination toward setting $HOME to user's homedir even if there is a workdir.

rhatdan · 2020-10-14T19:58:55Z

I am not sure of what the correct thing to do is. It would simplify the code quite a bit.

rhatdan · 2020-10-14T19:59:22Z

Setting $HOME to point at an non existing homedir, might be problematic.

edsantiago · 2020-10-14T20:01:57Z

Setting $HOME to point at an non existing homedir, might be problematic.

Agreed. My concern is: image has workdir. User runs container with -v homedir. Initial cwd is workdir (not controversial), but in this implementation, $HOME also points to workdir, even though /home/user exists. I am inclined to think that $HOME should be /home/user no matter whether a workdir is defined or not.

edsantiago · 2020-10-14T20:12:22Z

Yuk - failure looks horrible, but I can't see how it could be related to your PR. Am restarting.

         time="2020-10-14T14:56:24-05:00" level=error msg="error joining network namespace for container bfa5af4e01cc6886e8de0e7218e6c68acbf8d3569e7f1bb8e34d960079723913: error retrieving network namespace at /proc/83941/ns/net: failed to Statfs \"/proc/83941/ns/net\": permission denied"

Will assume that my flake-monitor will gather it up, add it to the list, and I'll eventually notice if this one is a common one.

Currently the HOME environment is set to /root if the user does not override it. Also walk the parent directories of users homedir to see if it is volume mounted into the container, if yes, then set it correctly. Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan · 2020-10-14T20:47:08Z

@edsantiago I now cover your case as well.

./bin/podman run -v /home/dwalsh:/home/dwalsh -it --rm --userns=keep-id alpine printenv HOME
/home/dwalsh
$ ./bin/podman run -v /home/dwalsh -it --rm --userns=keep-id alpine printenv HOME
/home/dwalsh
$ ./bin/podman run -it --rm --userns=keep-id alpine printenv HOME
/

edsantiago · 2020-10-14T20:49:08Z

That looks really good, thank you. I've got tests in the works, will submit PR as soon as yours merges.

rhatdan · 2020-10-14T20:49:27Z

More tests.

$ ./bin/podman run -it --rm --userns=keep-id alpine grep dwalsh /etc/passwd
dwalsh:*:3267:3267:Daniel J Walsh:/:/bin/sh
$ ./bin/podman run -it -v /home/dwalsh --rm --userns=keep-id alpine grep dwalsh /etc/passwd
dwalsh:*:3267:3267:Daniel J Walsh:/home/dwalsh:/bin/sh
$ ./bin/podman run -it -v /home/dwalsh:/home/dwalsh --rm --userns=keep-id alpine grep dwalsh /etc/passwd
dwalsh:*:3267:3267:Daniel J Walsh:/home/dwalsh:/bin/sh
$ ./bin/podman run -it -v /home:/home --rm --userns=keep-id alpine grep dwalsh /etc/passwd
dwalsh:*:3267:3267:Daniel J Walsh:/home/dwalsh:/bin/sh
$ ./bin/podman run -it -v /home --rm --userns=keep-id alpine grep dwalsh /etc/passwd
dwalsh:*:3267:3267:Daniel J Walsh:/:/bin/sh

- run --userns=keep-id: confirm that $HOME gets set (containers#8013) - inspect: confirm that JSON output is a sane number of lines (10 or more), not an unreadable one-liner (containers#8011 and containers#8021). Do so with image, pod, network, volume because the code paths might be different. - cgroups: confirm that 'run' preserves cgroup manager (containers#7970) - sdnotify: reenable tests, and hope CI doesn't hang. This test was disabled on August 18 because CI jobs were hanging and timing out. My suspicion was that it was containers#7316, which in turn seems to have hinged on conmon containers#182. The latter was merged on Sep 16, so let's cross our fingers and see what happens. Also: remove inaccurate warning from a networking test. And, wow, fix is_cgroupsv2(), it has never actually worked. Signed-off-by: Ed Santiago <[email protected]>

TomSweeneyRedHat · 2020-10-14T21:55:12Z

@rhatdan looks like you're still on the bucking bronco of a test system.

edsantiago · 2020-10-14T22:10:27Z

One is a network/Cirrus flake, I restarted. The other looks like #7139, which I'm starting to see a lot of these days. Restarted also.

vrothberg

LGTM

rhatdan · 2020-10-15T08:36:33Z

@giuseppe @saschagrunert PTAL and merge.

saschagrunert

/lgtm

openshift-ci-robot · 2020-10-15T09:04:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan, saschagrunert

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~~ [rhatdan]

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

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

rhatdan force-pushed the homedir branch from 510d40b to bf1fbc9 Compare October 13, 2020 21:57

TomSweeneyRedHat reviewed Oct 13, 2020

View reviewed changes

rhatdan force-pushed the homedir branch from bf1fbc9 to 93a2dbb Compare October 14, 2020 14:25

edsantiago reviewed Oct 14, 2020

View reviewed changes

rhatdan force-pushed the homedir branch from 93a2dbb to 8f3b913 Compare October 14, 2020 19:19

rhatdan force-pushed the homedir branch from 8f3b913 to 6ca8067 Compare October 14, 2020 20:45

edsantiago mentioned this pull request Oct 14, 2020

System test additions #8025

Merged

vrothberg reviewed Oct 15, 2020

View reviewed changes

saschagrunert approved these changes Oct 15, 2020

View reviewed changes

openshift-ci-robot assigned saschagrunert Oct 15, 2020

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

openshift-merge-robot merged commit 41eda41 into containers:master Oct 15, 2020

edsantiago mentioned this pull request Oct 19, 2020

Cirrus CI: agent stopped responding #8068

Closed

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

Setup HOME environment when using --userns=keep-id #8013

Setup HOME environment when using --userns=keep-id #8013

rhatdan commented Oct 13, 2020

TomSweeneyRedHat Oct 13, 2020

rhatdan Oct 14, 2020

rhatdan Oct 14, 2020

mheon Oct 14, 2020

TomSweeneyRedHat Oct 13, 2020

rhatdan Oct 14, 2020

edsantiago Oct 14, 2020

edsantiago commented Oct 14, 2020 •

edited

Loading

rhatdan commented Oct 14, 2020

edsantiago commented Oct 14, 2020

rhatdan commented Oct 14, 2020

rhatdan commented Oct 14, 2020

edsantiago commented Oct 14, 2020

edsantiago commented Oct 14, 2020

rhatdan commented Oct 14, 2020

edsantiago commented Oct 14, 2020

rhatdan commented Oct 14, 2020

TomSweeneyRedHat commented Oct 14, 2020

edsantiago commented Oct 14, 2020

vrothberg left a comment

rhatdan commented Oct 15, 2020

saschagrunert left a comment

openshift-ci-robot commented Oct 15, 2020

Setup HOME environment when using --userns=keep-id #8013

Setup HOME environment when using --userns=keep-id #8013

Conversation

rhatdan commented Oct 13, 2020

TomSweeneyRedHat Oct 13, 2020

Choose a reason for hiding this comment

rhatdan Oct 14, 2020

Choose a reason for hiding this comment

rhatdan Oct 14, 2020

Choose a reason for hiding this comment

mheon Oct 14, 2020

Choose a reason for hiding this comment

TomSweeneyRedHat Oct 13, 2020

Choose a reason for hiding this comment

rhatdan Oct 14, 2020

Choose a reason for hiding this comment

edsantiago Oct 14, 2020

Choose a reason for hiding this comment

edsantiago commented Oct 14, 2020 • edited Loading

rhatdan commented Oct 14, 2020

edsantiago commented Oct 14, 2020

rhatdan commented Oct 14, 2020

rhatdan commented Oct 14, 2020

edsantiago commented Oct 14, 2020

edsantiago commented Oct 14, 2020

rhatdan commented Oct 14, 2020

edsantiago commented Oct 14, 2020

rhatdan commented Oct 14, 2020

TomSweeneyRedHat commented Oct 14, 2020

edsantiago commented Oct 14, 2020

vrothberg left a comment

Choose a reason for hiding this comment

rhatdan commented Oct 15, 2020

saschagrunert left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Oct 15, 2020

edsantiago commented Oct 14, 2020 •

edited

Loading