From b41c864d569357a102ee2335a4947e59e5e2b08a Mon Sep 17 00:00:00 2001
From: Matthew Heon <matthew.heon@pm.me>
Date: Thu, 27 Feb 2020 16:08:29 -0500
Subject: [PATCH] Ensure that exec sessions inherit supplemental groups

This corrects a regression from Podman 1.4.x where container exec
sessions inherited supplemental groups from the container, iff
the exec session did not specify a user.

Signed-off-by: Matthew Heon <matthew.heon@pm.me>
---
 libpod/container_api.go            |  5 -----
 libpod/container_internal_linux.go |  5 ++++-
 libpod/oci_conmon_linux.go         | 25 +++++++++++++++++++++----
 test/e2e/exec_test.go              | 24 ++++++++++++++++++++++++
 4 files changed, 49 insertions(+), 10 deletions(-)

diff --git a/libpod/container_api.go b/libpod/container_api.go
index d612341bce..dabbe27dcd 100644
--- a/libpod/container_api.go
+++ b/libpod/container_api.go
@@ -270,11 +270,6 @@ func (c *Container) Exec(tty, privileged bool, env map[string]string, cmd []stri
 		}
 	}()
 
-	// if the user is empty, we should inherit the user that the container is currently running with
-	if user == "" {
-		user = c.config.User
-	}
-
 	opts := new(ExecOptions)
 	opts.Cmd = cmd
 	opts.CapAdd = capList
diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go
index 7390262647..63968918cb 100644
--- a/libpod/container_internal_linux.go
+++ b/libpod/container_internal_linux.go
@@ -330,7 +330,10 @@ func (c *Container) generateSpec(ctx context.Context) (*spec.Spec, error) {
 
 	// Add addition groups if c.config.GroupAdd is not empty
 	if len(c.config.Groups) > 0 {
-		gids, _ := lookup.GetContainerGroups(c.config.Groups, c.state.Mountpoint, nil)
+		gids, err := lookup.GetContainerGroups(c.config.Groups, c.state.Mountpoint, overrides)
+		if err != nil {
+			return nil, errors.Wrapf(err, "error looking up supplemental groups for container %s", c.ID())
+		}
 		for _, gid := range gids {
 			g.AddProcessAdditionalGid(gid)
 		}
diff --git a/libpod/oci_conmon_linux.go b/libpod/oci_conmon_linux.go
index 07d38693f0..800f896036 100644
--- a/libpod/oci_conmon_linux.go
+++ b/libpod/oci_conmon_linux.go
@@ -1252,18 +1252,35 @@ func prepareProcessExec(c *Container, cmd, env []string, tty bool, cwd, user, se
 
 	}
 
+	var addGroups []string
+	var sgids []uint32
+
+	// if the user is empty, we should inherit the user that the container is currently running with
+	if user == "" {
+		user = c.config.User
+		addGroups = c.config.Groups
+	}
+
 	overrides := c.getUserOverrides()
 	execUser, err := lookup.GetUserGroupInfo(c.state.Mountpoint, user, overrides)
 	if err != nil {
 		return nil, err
 	}
 
+	if len(addGroups) > 0 {
+		sgids, err = lookup.GetContainerGroups(addGroups, c.state.Mountpoint, overrides)
+		if err != nil {
+			return nil, errors.Wrapf(err, "error looking up supplemental groups for container %s exec session %s", c.ID(), sessionID)
+		}
+	}
+
 	// If user was set, look it up in the container to get a UID to use on
 	// the host
-	if user != "" {
-		sgids := make([]uint32, 0, len(execUser.Sgids))
-		for _, sgid := range execUser.Sgids {
-			sgids = append(sgids, uint32(sgid))
+	if user != "" || len(sgids) > 0 {
+		if user != "" {
+			for _, sgid := range execUser.Sgids {
+				sgids = append(sgids, uint32(sgid))
+			}
 		}
 		processUser := spec.User{
 			UID:            uint32(execUser.Uid),
diff --git a/test/e2e/exec_test.go b/test/e2e/exec_test.go
index ed4eb3335f..ab806f6831 100644
--- a/test/e2e/exec_test.go
+++ b/test/e2e/exec_test.go
@@ -1,6 +1,7 @@
 package integration
 
 import (
+	"fmt"
 	"os"
 	"strings"
 
@@ -244,4 +245,27 @@ var _ = Describe("Podman exec", func() {
 		Expect(session.ExitCode()).To(Equal(0))
 	})
 
+	It("podman exec preserves --group-add groups", func() {
+		groupName := "group1"
+		gid := "4444"
+		ctrName1 := "ctr1"
+		ctr1 := podmanTest.Podman([]string{"run", "-ti", "--name", ctrName1, fedoraMinimal, "groupadd", "-g", gid, groupName})
+		ctr1.WaitWithDefaultTimeout()
+		Expect(ctr1.ExitCode()).To(Equal(0))
+
+		imgName := "img1"
+		commit := podmanTest.Podman([]string{"commit", ctrName1, imgName})
+		commit.WaitWithDefaultTimeout()
+		Expect(commit.ExitCode()).To(Equal(0))
+
+		ctrName2 := "ctr2"
+		ctr2 := podmanTest.Podman([]string{"run", "-d", "--name", ctrName2, "--group-add", groupName, imgName, "sleep", "300"})
+		ctr2.WaitWithDefaultTimeout()
+		Expect(ctr2.ExitCode()).To(Equal(0))
+
+		exec := podmanTest.Podman([]string{"exec", "-ti", ctrName2, "id"})
+		exec.WaitWithDefaultTimeout()
+		Expect(exec.ExitCode()).To(Equal(0))
+		Expect(strings.Contains(exec.OutputToString(), fmt.Sprintf("%s(%s)", gid, groupName))).To(BeTrue())
+	})
 })