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())

+	})

 })