Preserve groups in exec sessions in ctrs with --user

Podman wants to guarantee that exec sessions retain the groups of
the container they are started in, unless explicitly overridden
by the user. This guarantee was broken for containers where the
`--user` flag was specified; this patch resolves that.

Somewhere in the Exec rewrite for APIv2, I changed the location
where the container's User is passed into the exec session
(similar to groups, we also want to preserve user unless
overridden). The lower-level Exec APIs already handled setting
user and group appropriately if not specified when the exec
session was created, but I added duplicate code to handle this
higher in the stack - and that code only handled setting user,
not supplemental groups, breaking support in that specific case.

Two things conspired to make this one hard to track down: first,
things were only broken if the container explicitly set a user;
otherwise, the container user would still appear to be unset to
the lower-level code, which would properly set supplemental
groups (this tricked our existing test into passing). Also, the
`crun` OCI runtime will add the groups without prompting, which
further masked the problem there. I debated making `runc` do the
same, but in the end it's better to fix this in Podman - it's
better to be explicit about what we want done so we will work
with all OCI runtimes.

Signed-off-by: Matthew Heon <matthew.heon@pm.me>

2 files changed, 29 insertions, 6 deletions

diff --git a/libpod/container_exec.go b/libpod/container_exec.go
index f5f54c7cc..fce26acb0 100644
--- a/libpod/container_exec.go
+++ b/libpod/container_exec.go
@@ -980,11 +980,6 @@ func prepareForExec(c *Container, session *ExecSession) (*ExecOptions, error) {
 		capList = capabilities.AllCapabilities()
 	}
 
-	user := c.config.User
-	if session.Config.User != "" {
-		user = session.Config.User
-	}
-
 	if err := c.createExecBundle(session.ID()); err != nil {
 		return nil, err
 	}
@@ -995,7 +990,7 @@ func prepareForExec(c *Container, session *ExecSession) (*ExecOptions, error) {
 	opts.Env = session.Config.Environment
 	opts.Terminal = session.Config.Terminal
 	opts.Cwd = session.Config.WorkDir
-	opts.User = user
+	opts.User = session.Config.User
 	opts.PreserveFDs = session.Config.PreserveFDs
 	opts.DetachKeys = session.Config.DetachKeys
 	opts.ExitCommand = session.Config.ExitCommand
diff --git a/test/e2e/exec_test.go b/test/e2e/exec_test.go
index 055546f88..6841aa5a2 100644
--- a/test/e2e/exec_test.go
+++ b/test/e2e/exec_test.go
@@ -283,6 +283,34 @@ var _ = Describe("Podman exec", func() {
 		Expect(strings.Contains(exec.OutputToString(), fmt.Sprintf("%s(%s)", gid, groupName))).To(BeTrue())
 	})
 
+	It("podman exec preserves container groups with --user and --group-add", func() {
+		SkipIfRemote()
+		dockerfile := `FROM fedora-minimal
+RUN groupadd -g 4000 first
+RUN groupadd -g 4001 second
+RUN useradd -u 1000 auser`
+		imgName := "testimg"
+		podmanTest.BuildImage(dockerfile, imgName, "false")
+
+		ctrName := "testctr"
+		ctr := podmanTest.Podman([]string{"run", "-t", "-i", "-d", "--name", ctrName, "--user", "auser:first", "--group-add", "second", imgName, "sleep", "300"})
+		ctr.WaitWithDefaultTimeout()
+		Expect(ctr.ExitCode()).To(Equal(0))
+
+		exec := podmanTest.Podman([]string{"exec", "-t", ctrName, "id"})
+		exec.WaitWithDefaultTimeout()
+		Expect(exec.ExitCode()).To(Equal(0))
+		output := exec.OutputToString()
+		Expect(strings.Contains(output, "4000(first)")).To(BeTrue())
+		Expect(strings.Contains(output, "4001(second)")).To(BeTrue())
+		Expect(strings.Contains(output, "1000(auser)")).To(BeTrue())
+
+		// Kill the container just so the test does not take 15 seconds to stop.
+		kill := podmanTest.Podman([]string{"kill", ctrName})
+		kill.WaitWithDefaultTimeout()
+		Expect(kill.ExitCode()).To(Equal(0))
+	})
+
 	It("podman exec --detach", func() {
 		ctrName := "testctr"
 		ctr := podmanTest.Podman([]string{"run", "-t", "-i", "-d", "--name", ctrName, ALPINE, "top"})