Merge pull request kata-containers#2418 from devimc/topic/virtcontain…

…ers/improveRootless rootless: use libcontainer API to detect rootless
amshinde · Jan 29, 2020 · db679fb · db679fb
2 parents 61d826e + 4a77b0f
commit db679fb
Show file tree

Hide file tree

Showing 5 changed files with 31 additions and 230 deletions.
diff --git a/cli/main_test.go b/cli/main_test.go
@@ -24,6 +24,7 @@ import (
 	"github.com/dlespiau/covertool/pkg/cover"
 	ktu "github.com/kata-containers/runtime/pkg/katatestutils"
 	"github.com/kata-containers/runtime/pkg/katautils"
+	"github.com/kata-containers/runtime/pkg/rootless"
 	vc "github.com/kata-containers/runtime/virtcontainers"
 	"github.com/kata-containers/runtime/virtcontainers/pkg/compatoci"
 	"github.com/kata-containers/runtime/virtcontainers/pkg/oci"
@@ -1068,6 +1069,9 @@ func TestMainResetCLIGlobals(t *testing.T) {
 }
 
 func createTempContainerIDMapping(containerID, sandboxID string) (string, error) {
+	// Mocking rootless
+	rootless.IsRootless = func() bool { return false }
+
 	tmpDir, err := ioutil.TempDir("", "containers-mapping")
 	if err != nil {
 		return "", err

diff --git a/pkg/katautils/oci_test.go b/pkg/katautils/oci_test.go
@@ -13,9 +13,14 @@ import (
 	"path/filepath"
 	"testing"
 
+	"github.com/kata-containers/runtime/pkg/rootless"
 	"github.com/stretchr/testify/assert"
 )
 
+func init() {
+	rootless.IsRootless = func() bool { return false }
+}
+
 func createTempContainerIDMapping(containerID, sandboxID string) (string, error) {
 	tmpDir, err := ioutil.TempDir("", "containers-mapping")
 	if err != nil {

diff --git a/pkg/rootless/rootless.go b/pkg/rootless/rootless.go
@@ -20,20 +20,17 @@
 package rootless
 
 import (
-	"bufio"
 	"context"
 	"crypto/rand"
 	"fmt"
-	"io"
 	"os"
 	"path/filepath"
 	"runtime"
-	"strconv"
 	"strings"
 	"sync"
 
 	"github.com/containernetworking/plugins/pkg/ns"
-	"github.com/pkg/errors"
+	"github.com/opencontainers/runc/libcontainer/system"
 	"github.com/sirupsen/logrus"
 	"golang.org/x/sys/unix"
 )
@@ -53,13 +50,12 @@ var (
 	// which user-specific non-essential runtime files are stored.
 	rootlessDir = os.Getenv("XDG_RUNTIME_DIR")
 
-	// uidMapPath defines the location of the uid_map file to
-	// determine whether a user is root or not
-	uidMapPath = "/proc/self/uid_map"
-
 	rootlessLog = logrus.WithFields(logrus.Fields{
 		"source": "rootless",
 	})
+
+	//The function is declared this way for mocking in unit tests
+	IsRootless = isRootlessFunc
 )
 
 // SetLogger sets up a logger for the rootless pkg
@@ -68,71 +64,24 @@ func SetLogger(ctx context.Context, logger *logrus.Entry) {
 	rootlessLog = logger.WithFields(fields)
 }
 
-// setRootless reads a uid_map file, compares the UID of the
-// user inside the container vs on the host. If the host UID
-// is not root, but the container ID is, it can be determined
-// the user is running rootlessly.
-func setRootless() error {
-	initRootless = true
-	file, err := os.Open(uidMapPath)
-	if err != nil {
-		return err
-	}
-	defer file.Close()
-
-	buf := bufio.NewReader(file)
-	for {
-		line, _, err := buf.ReadLine()
-		if err != nil {
-			if err == io.EOF {
-				return nil
-			}
-			return err
-		}
-		if line == nil {
-			return nil
-		}
-
-		var parseError = errors.Errorf("Failed to parse uid map file %s", uidMapPath)
-		// if the container id (id[0]) is 0 (root inside the container)
-		// has a mapping to the host id (id[1]) that is not root, then
-		// it can be determined that the host user is running rootless
-		ids := strings.Fields(string(line))
-		// do some sanity checks
-		if len(ids) != 3 {
-			return parseError
-		}
-		userNSUid, err := strconv.ParseUint(ids[0], 10, 0)
-		if err != nil {
-			return parseError
-		}
-		hostUID, err := strconv.ParseUint(ids[1], 10, 0)
-		if err != nil {
-			return parseError
-		}
-		rangeUID, err := strconv.ParseUint(ids[2], 10, 0)
-		if err != nil || rangeUID == 0 {
-			return parseError
-		}
-
-		if userNSUid == 0 && hostUID != 0 {
-			rootlessLog.Info("Running as rootless")
-			isRootless = true
-			return nil
-		}
-	}
-}
-
 // IsRootless states whether kata is being ran with root or not
-func IsRootless() bool {
+func isRootlessFunc() bool {
 	rLock.Lock()
+	defer rLock.Unlock()
 	if !initRootless {
-		err := setRootless()
-		if err != nil {
-			rootlessLog.WithError(err).Error("Unable to determine if running rootless")
+		initRootless = true
+		isRootless = true
+		// --rootless and --systemd-cgroup options must honoured
+		// but with the current implementation this is not possible
+		// https://github.com/kata-containers/runtime/issues/2412
+		if os.Geteuid() != 0 {
+			return true
+		}
+		if system.RunningInUserNS() {
+			return true
 		}
+		isRootless = false
 	}
-	rLock.Unlock()
 	return isRootless
 }
 

diff --git a/pkg/rootless/rootless_test.go b/pkg/rootless/rootless_test.go
@@ -4,165 +4,3 @@
 //
 
 package rootless
-
-import (
-	"fmt"
-	"io/ioutil"
-	"os"
-	"path/filepath"
-	"testing"
-
-	"github.com/stretchr/testify/assert"
-)
-
-type uidMapping struct {
-	userNSUID int
-	hostUID   int
-	rangeUID  int
-}
-
-type testScenario struct {
-	isRootless bool
-	uidMap     []uidMapping
-}
-
-var uidMapPathStore = uidMapPath
-
-func createTestUIDMapFile(input string) error {
-	f, err := os.Create(uidMapPath)
-	if err != nil {
-		return err
-	}
-	defer f.Close()
-
-	_, err = f.WriteString(input)
-	if err != nil {
-		return err
-	}
-
-	return nil
-}
-
-func uidMapString(userNSUID, hostUID, rangeUID int) string {
-	return fmt.Sprintf("\t%d\t%d\t%d", userNSUID, hostUID, rangeUID)
-}
-
-func testWithUIDMapContent(content string, expectedRootless bool, t *testing.T) {
-	assert := assert.New(t)
-	// Create a test-specific message that is added to each assert
-	// call. It will be displayed if any assert test fails.
-	msg := fmt.Sprintf("isRootless[%t]: %s", expectedRootless, content)
-
-	tmpDir, err := ioutil.TempDir("", "")
-	assert.NoError(err)
-
-	uidMapPath = filepath.Join(tmpDir, "testUIDMapFile")
-	defer func() {
-		uidMapPath = uidMapPathStore
-		os.RemoveAll(uidMapPath)
-		os.RemoveAll(tmpDir)
-		isRootless = false
-		initRootless = false
-	}()
-
-	err = createTestUIDMapFile(content)
-	assert.NoError(err, msg)
-
-	// make call to IsRootless, this should also call
-	// SetRootless
-	assert.Equal(expectedRootless, IsRootless(), msg)
-}
-
-func TestIsRootless(t *testing.T) {
-	assert := assert.New(t)
-
-	// by default isRootless should be set to false initially
-	assert.False(isRootless)
-
-	allScenarios := []testScenario{
-		//"User NS UID is not root UID"
-		{
-			isRootless: false,
-			uidMap: []uidMapping{
-				{1, 0, 1},
-				{1, 0, 1000},
-
-				{1, 1000, 1},
-				{1, 1000, 1000},
-
-				{1000, 1000, 1},
-				{1000, 1000, 1000},
-
-				{1000, 1000, 5555},
-			},
-		},
-
-		//"Host NS UID is root UID"
-		{
-			isRootless: false,
-			uidMap: []uidMapping{
-				{0, 0, 1},
-				{0, 0, 1000},
-
-				{1, 0, 1},
-				{1, 0, 1000},
-
-				{1000, 0, 0},
-				{1000, 0, 1},
-				{1000, 0, 1000},
-			},
-		},
-
-		//"UID range is zero"
-		{
-			isRootless: false,
-			uidMap: []uidMapping{
-				{0, 0, 0},
-				{1, 0, 0},
-				{0, 1, 0},
-				{1, 1000, 0},
-				{1000, 1000, 0},
-			},
-		},
-
-		//"Negative UIDs"
-		{
-			isRootless: false,
-			uidMap: []uidMapping{
-				{-1, 0, 0},
-				{-1, 0, 1},
-				{-1, 0, 1000},
-
-				{0, -1, 0},
-				{0, -1, 1},
-				{0, -1, 1000},
-
-				{1000, 1000, -1},
-				{1000, 1000, -1},
-				{1000, 1000, -1000},
-			},
-		},
-
-		//"User NS UID is root UID, host UID is not root UID"
-		{
-			isRootless: true,
-			uidMap: []uidMapping{
-				{0, 1, 1},
-				{0, 1000, 1},
-				{0, 1000, 5555},
-			},
-		},
-	}
-
-	// Run the tests
-	for _, scenario := range allScenarios {
-		for _, uidMap := range scenario.uidMap {
-			mapping := uidMapString(uidMap.userNSUID, uidMap.hostUID, uidMap.rangeUID)
-			testWithUIDMapContent(mapping, scenario.isRootless, t)
-		}
-	}
-
-	testWithUIDMapContent("", false, t)
-
-	testWithUIDMapContent("This is not a mapping", false, t)
-}
diff --git a/virtcontainers/api_test.go b/virtcontainers/api_test.go
@@ -16,6 +16,7 @@ import (
 	"testing"
 
 	ktu "github.com/kata-containers/runtime/pkg/katatestutils"
+	"github.com/kata-containers/runtime/pkg/rootless"
 	"github.com/kata-containers/runtime/virtcontainers/persist"
 	"github.com/kata-containers/runtime/virtcontainers/persist/fs"
 	"github.com/kata-containers/runtime/virtcontainers/pkg/annotations"
@@ -40,6 +41,10 @@ var containerAnnotations = map[string]string{
 	"container.hello": "container.world",
 }
 
+func init() {
+	rootless.IsRootless = func() bool { return false }
+}
+
 func newEmptySpec() *specs.Spec {
 	return &specs.Spec{
 		Linux: &specs.Linux{