Check Assemble Users Against Allowed UIDs

Adding check to ensure the s2i assemble user is allowed if the --allowed-uids flag is set.
The assemble user can come from one of two sources:

1. --assemble-user flag
2. builder image io.openshift.s2i.assemble-user label

The assemble user overrides the default image user for an s2i build.
However, if the base image has ONBUILD instructions with USER directives,
all USER directives will be checked to ensure compliance.

Bug 1582976

hack/build-test-images.sh

@@ -22,6 +22,8 @@ s2i::build_test_image() {
   cd "${S2I_ROOT}"
 
   s2i::build_test_image sti-fake
+  s2i::build_test_image sti-fake-assemble-root
+  s2i::build_test_image sti-fake-assemble-user
   s2i::build_test_image sti-fake-env
   s2i::build_test_image sti-fake-user
   s2i::build_test_image sti-fake-scripts

hack/test-integration.sh

@@ -13,7 +13,7 @@ s2i::cleanup() {
 
 readonly img_count="$(docker images | grep -c sti_test/sti-fake || :)"
 
-if [ "${img_count}" != "10" ]; then
+if [ "${img_count}" != "12" ]; then
     echo "Missing test images, run 'hack/build-test-images.sh' and try again."
     exit 1
 fi

pkg/api/describe/describer.go

@@ -103,13 +103,14 @@ func describeBuilderImage(client docker.Client, config *api.Config, out io.Write
 		Tag:                config.Tag,
 		IncrementalAuthentication: config.IncrementalAuthentication,
 	}
-	pr, err := docker.GetBuilderImage(client, c)
+	dkr := docker.New(client, c.PullAuthentication)
+	builderImage, err := docker.GetBuilderImage(dkr, c)
 	if err == nil {
-		build.GenerateConfigFromLabels(c, pr)
+		build.GenerateConfigFromLabels(c, builderImage)
 		if len(c.DisplayName) > 0 {
 			fmt.Fprintf(out, "Builder Name:\t%s\n", c.DisplayName)
 		}
-		fmt.Fprintf(out, "Builder Image:\t%s\n", config.BuilderImage)
+		fmt.Fprintf(out, "Builder Image:\t%s\n", c.BuilderImage)
 		if len(c.BuilderImageVersion) > 0 {
 			fmt.Fprintf(out, "Builder Image Version:\t%s\n", c.BuilderImageVersion)
 		}

pkg/build/strategies/sti/sti.go

@@ -270,7 +270,7 @@ func (builder *STI) Prepare(config *api.Config) error {
 
 	if len(config.RuntimeImage) > 0 {
 		startTime := time.Now()
-		dockerpkg.GetRuntimeImage(config, builder.runtimeDocker)
+		dockerpkg.GetRuntimeImage(builder.runtimeDocker, config)
 		builder.result.BuildInfo.Stages = api.RecordStageAndStepInfo(builder.result.BuildInfo.Stages, api.StagePullImages, api.StepPullRuntimeImage, startTime, time.Now())
 
 		if err != nil {

pkg/build/strategies/strategies.go

@@ -42,7 +42,8 @@ func Strategy(client docker.Client, config *api.Config, overrides build.Override
 		return builder, buildInfo, nil
 	}
 
-	image, err := docker.GetBuilderImage(client, config)
+	dkr := docker.New(client, config.PullAuthentication)
+	image, err := docker.GetBuilderImage(dkr, config)
 	buildInfo.Stages = api.RecordStageAndStepInfo(buildInfo.Stages, api.StagePullImages, api.StepPullBuilderImage, startTime, time.Now())
 	if err != nil {
 		buildInfo.FailureReason = utilstatus.NewFailureReason(
@@ -53,7 +54,7 @@ func Strategy(client docker.Client, config *api.Config, overrides build.Override
 	}
 	config.HasOnBuild = image.OnBuild
 
-	if config.AssembleUser, err = docker.GetAssembleUser(client, config); err != nil {
+	if config.AssembleUser, err = docker.GetAssembleUser(dkr, config); err != nil {
 		buildInfo.FailureReason = utilstatus.NewFailureReason(
 			utilstatus.ReasonPullBuilderImageFailed,
 			utilstatus.ReasonMessagePullBuilderImageFailed,

pkg/cmd/cli/cmd/rebuild.go

@@ -1,9 +1,10 @@
 package cmd
 
 import (
-	"github.com/spf13/cobra"
 	"os"
 
+	"github.com/spf13/cobra"
+
 	"github.com/openshift/source-to-image/pkg/api"
 	"github.com/openshift/source-to-image/pkg/api/describe"
 	"github.com/openshift/source-to-image/pkg/build"
@@ -46,8 +47,8 @@ func NewCmdRebuild(cfg *api.Config) *cobra.Command {
 
 			client, err := docker.NewEngineAPIClient(cfg.DockerConfig)
 			s2ierr.CheckError(err)
-
-			pr, err := docker.GetRebuildImage(client, cfg)
+			dkr := docker.New(client, cfg.PullAuthentication)
+			pr, err := docker.GetRebuildImage(dkr, cfg)
 			s2ierr.CheckError(err)
 			err = build.GenerateConfigFromLabels(cfg, pr)
 			s2ierr.CheckError(err)

pkg/docker/util.go

@@ -272,58 +272,70 @@ func PullImage(name string, d Docker, policy api.PullPolicy) (*PullResult, error
 	return &PullResult{Image: image, OnBuild: d.IsImageOnBuild(name)}, err
 }
 
-// CheckAllowedUser retrieves the user for a Docker image and checks that user
-// against an allowed range of uids.
+// CheckAllowedUser retrieves the execution users for a Docker image and
+// checks that user against an allowed range of uids.
 // - If the range of users is not empty, then the user on the Docker image
 // needs to be a numeric user
 // - The user's uid must be contained by the range(s) specified by the uids
 // Rangelist
+// - If build image uses an assemble user (via a command override or an
+// image label), that user must be within the allowed range of uids.
 // - If the image contains ONBUILD instructions and those instructions also
-// contain a USER directive, then the user specified by that USER directive
+// contain any USER directives, then all users specified by those USER directives
 // must meet the uid range criteria as well.
-func CheckAllowedUser(d Docker, imageName string, uids user.RangeList, isOnbuild bool) error {
+func CheckAllowedUser(d Docker, imageName string, uids user.RangeList, isOnbuild bool, assembleUserConfig string) error {
 	if uids == nil || uids.Empty() {
 		return nil
 	}
-	imageUserSpec, err := d.GetImageUser(imageName)
-	if err != nil {
-		return err
-	}
-	imageUser := extractUser(imageUserSpec)
-	if !user.IsUserAllowed(imageUser, &uids) {
-		return s2ierr.NewUserNotAllowedError(imageName, false)
-	}
+
+	// OnBuild users always need to be checked for layered builds
+	// Only return error if a user is not allowed, otherwise continue
 	if isOnbuild {
-		cmds, err := d.GetOnBuild(imageName)
+		onBuildUsers, err := extractOnBuildUsers(d, imageName)
 		if err != nil {
 			return err
 		}
-		if !isOnbuildAllowed(cmds, &uids) {
-			return s2ierr.NewUserNotAllowedError(imageName, true)
+		for _, usr := range onBuildUsers {
+			if !user.IsUserAllowed(usr, &uids) {
+				return s2ierr.NewUserNotAllowedError(imageName, true)
+			}
 		}
 	}
+
+	// P1: Assemble user configuration
+	if len(assembleUserConfig) > 0 {
+		if !user.IsUserAllowed(assembleUserConfig, &uids) {
+			// Pass in the override, since assembleUser can come from the image label
+			return s2ierr.NewAssembleUserNotAllowedError(imageName, true)
+		}
+		return nil
+	}
+
+	// P2: Assemble user label in image
+	assembleUser, err := extractAssembleUser(d, imageName)
+	if err != nil {
+		return err
+	}
+	if len(assembleUser) > 0 {
+		if !user.IsUserAllowed(assembleUser, &uids) {
+			// Pass in the override, since assembleUser can come from the image label
+			return s2ierr.NewAssembleUserNotAllowedError(imageName, false)
+		}
+		return nil
+	}
+
+	// Default - image user
+	imageUser, err := extractImageUser(d, imageName)
+	if err != nil {
+		return err
+	}
+	if !user.IsUserAllowed(imageUser, &uids) {
+		return s2ierr.NewUserNotAllowedError(imageName, false)
+	}
+
 	return nil
 }
 
-var dockerLineDelim = regexp.MustCompile(`[\t\v\f\r ]+`)
-
-// isOnbuildAllowed checks a list of Docker ONBUILD instructions for user
-// directives. It ensures that any users specified by the directives falls
-// within the specified range list of users.
-func isOnbuildAllowed(directives []string, allowed *user.RangeList) bool {
-	for _, line := range directives {
-		parts := dockerLineDelim.Split(line, 2)
-		if strings.ToLower(parts[0]) != "user" {
-			continue
-		}
-		uname := extractUser(parts[1])
-		if !user.IsUserAllowed(uname, allowed) {
-			return false
-		}
-	}
-	return true
-}
-
 func extractUser(userSpec string) string {
 	if strings.Contains(userSpec, ":") {
 		parts := strings.SplitN(userSpec, ":", 2)
@@ -332,6 +344,35 @@ func extractUser(userSpec string) string {
 	return strings.TrimSpace(userSpec)
 }
 
+func extractImageUser(d Docker, imageName string) (string, error) {
+	imageUserSpec, err := d.GetImageUser(imageName)
+	if err != nil {
+		return "", err
+	}
+	imageUser := extractUser(imageUserSpec)
+	return imageUser, nil
+}
+
+var dockerLineDelim = regexp.MustCompile(`[\t\v\f\r ]+`)
+
+// extractOnBuildUsers checks a list of Docker ONBUILD instructions for user
+// directives. It returns a list of users specified in the ONBUILD directives.
+func extractOnBuildUsers(d Docker, imageName string) ([]string, error) {
+	cmds, err := d.GetOnBuild(imageName)
+	var users []string
+	if err != nil {
+		return users, err
+	}
+	for _, line := range cmds {
+		parts := dockerLineDelim.Split(line, 2)
+		if strings.ToLower(parts[0]) != "user" {
+			continue
+		}
+		users = append(users, extractUser(parts[1]))
+	}
+	return users, nil
+}
+
 // CheckReachable returns if the Docker daemon is reachable from s2i
 func (d *stiDocker) CheckReachable() error {
 	_, err := d.Version()
@@ -344,7 +385,7 @@ func pullAndCheck(image string, docker Docker, pullPolicy api.PullPolicy, config
 		return nil, err
 	}
 
-	err = CheckAllowedUser(docker, image, config.AllowedUIDs, r.OnBuild)
+	err = CheckAllowedUser(docker, image, config.AllowedUIDs, r.OnBuild, config.AssembleUser)
 	if err != nil {
 		return nil, err
 	}
@@ -356,22 +397,20 @@ func pullAndCheck(image string, docker Docker, pullPolicy api.PullPolicy, config
 // make the Docker image specified as BuilderImage available locally. It
 // returns information about the base image, containing metadata necessary for
 // choosing the right STI build strategy.
-func GetBuilderImage(client Client, config *api.Config) (*PullResult, error) {
-	d := New(client, config.PullAuthentication)
-	return pullAndCheck(config.BuilderImage, d, config.BuilderPullPolicy, config)
+func GetBuilderImage(docker Docker, config *api.Config) (*PullResult, error) {
+	return pullAndCheck(config.BuilderImage, docker, config.BuilderPullPolicy, config)
 }
 
 // GetRebuildImage obtains the metadata information for the image specified in
 // a s2i rebuild operation. Assumptions are made that the build is available
 // locally since it should have been previously built.
-func GetRebuildImage(client Client, config *api.Config) (*PullResult, error) {
-	d := New(client, config.PullAuthentication)
-	return pullAndCheck(config.Tag, d, config.BuilderPullPolicy, config)
+func GetRebuildImage(docker Docker, config *api.Config) (*PullResult, error) {
+	return pullAndCheck(config.Tag, docker, config.BuilderPullPolicy, config)
 }
 
 // GetRuntimeImage processes the config and performs operations necessary to
 // make the Docker image specified as RuntimeImage available locally.
-func GetRuntimeImage(config *api.Config, docker Docker) error {
+func GetRuntimeImage(docker Docker, config *api.Config) error {
 	_, err := pullAndCheck(config.RuntimeImage, docker, config.RuntimeImagePullPolicy, config)
 	return err
 }
@@ -405,12 +444,15 @@ func GetDefaultDockerConfig() *api.DockerConfig {
 // This functions receives the config to check if the AssembleUser was defined in command line
 // If the cmd is blank, it tries to fetch the value from the Builder Image defined Label (assemble-user)
 // Otherwise it follows the common flow, using the USER defined in Dockerfile
-func GetAssembleUser(client Client, config *api.Config) (string, error) {
+func GetAssembleUser(docker Docker, config *api.Config) (string, error) {
 	if len(config.AssembleUser) > 0 {
 		return config.AssembleUser, nil
 	}
-	d := New(client, config.PullAuthentication)
-	imageData, err := d.GetLabels(config.BuilderImage)
+	return extractAssembleUser(docker, config.BuilderImage)
+}
+
+func extractAssembleUser(docker Docker, imageName string) (string, error) {
+	imageData, err := docker.GetLabels(imageName)
 	if err != nil {
 		return "", err
 	}

pkg/docker/util_test.go

@@ -16,11 +16,13 @@ func rangeList(str string) *user.RangeList {
 
 func TestCheckAllowedUser(t *testing.T) {
 	tests := []struct {
-		name        string
-		allowedUIDs *user.RangeList
-		user        string
-		onbuild     []string
-		expectErr   bool
+		name         string
+		allowedUIDs  *user.RangeList
+		user         string
+		onbuild      []string
+		expectErr    bool
+		assembleUser string
+		labels       map[string]string
 	}{
 		{
 			name:        "AllowedUIDs is not set",
@@ -117,14 +119,73 @@ func TestCheckAllowedUser(t *testing.T) {
 			onbuild:     []string{"RUN echo \"hello world\"", "USER 10:wheel"},
 			expectErr:   false,
 		},
+		{
+			name:         "AllowedUIDs is set, numeric user, assemble user override ok",
+			allowedUIDs:  rangeList("1-"),
+			user:         "200",
+			assembleUser: "10",
+			expectErr:    false,
+		},
+		{
+			name:         "AllowedUIDs is set, numeric user, root assemble user",
+			allowedUIDs:  rangeList("1-"),
+			user:         "200",
+			assembleUser: "0",
+			expectErr:    true,
+		},
+		{
+			name:        "AllowedUIDs is set, numeric user, assemble user label ok",
+			allowedUIDs: rangeList("1-"),
+			user:        "200",
+			labels:      map[string]string{AssembleUserLabel: "10"},
+			expectErr:   false,
+		},
+		{
+			name:        "AllowedUIDs is set, numeric user, assemble user label root",
+			allowedUIDs: rangeList("1-"),
+			user:        "200",
+			labels:      map[string]string{AssembleUserLabel: "0"},
+			expectErr:   true,
+		},
+		{
+			name:        "AllowedUIDs is set, root image user, assemble user label ok",
+			allowedUIDs: rangeList("1-"),
+			user:        "0",
+			labels:      map[string]string{AssembleUserLabel: "10"},
+			expectErr:   false,
+		},
+		{
+			name:         "AllowedUIDs is set, root image user, assemble user override ok",
+			allowedUIDs:  rangeList("1-"),
+			user:         "0",
+			assembleUser: "10",
+			expectErr:    false,
+		},
+		{
+			name:        "AllowedUIDs is set, root image user, onbuild root named user with group, assemble user label ok",
+			allowedUIDs: rangeList("1-"),
+			user:        "0",
+			labels:      map[string]string{AssembleUserLabel: "10"},
+			onbuild:     []string{"RUN echo \"hello world\"", "USER root:wheel", "RUN echo \"i am gROOT\"", "USER 10"},
+			expectErr:   true,
+		},
+		{
+			name:         "AllowedUIDs is set, root image user, onbuild root named user with group, assemble user override ok",
+			allowedUIDs:  rangeList("1-"),
+			user:         "0",
+			assembleUser: "10",
+			onbuild:      []string{"RUN echo \"hello world\"", "USER root:wheel", "RUN echo \"i am gROOT\"", "USER 10"},
+			expectErr:    true,
+		},
 	}
 
 	for _, tc := range tests {
 		docker := &FakeDocker{
 			GetImageUserResult: tc.user,
 			OnBuildResult:      tc.onbuild,
+			Labels:             tc.labels,
 		}
-		err := CheckAllowedUser(docker, "", *tc.allowedUIDs, len(tc.onbuild) > 0)
+		err := CheckAllowedUser(docker, "", *tc.allowedUIDs, len(tc.onbuild) > 0, tc.assembleUser)
 		if err != nil && !tc.expectErr {
 			t.Errorf("%s: unexpected error: %v", tc.name, err)
 		}

pkg/errors/errors.go

@@ -233,7 +233,27 @@ func NewUserNotAllowedError(image string, onbuild bool) error {
 	return Error{
 		Message:    msg,
 		ErrorCode:  UserNotAllowedError,
-		Suggestion: fmt.Sprintf("modify image %q to use a numeric user within the allowed range or build without the --allowed-uids flag", image),
+		Suggestion: fmt.Sprintf("modify image %q to use a numeric user within the allowed range, or build without the --allowed-uids flag", image),
+	}
+}
+
+// NewAssembleUserNotAllowedError returns a new error that indicates that the build
+// could not run because the build or image uses an assemble user outside of the range
+// of allowed users.
+func NewAssembleUserNotAllowedError(image string, usesConfig bool) error {
+	var msg, suggestion string
+	if usesConfig {
+		msg = "assemble user must be numeric and within the range of allowed users"
+		suggestion = "build without the allowed-uids or assemble-user configurations set"
+	} else {
+		assembleLabel := "io.openshift.s2i.assemble-user"
+		msg = fmt.Sprintf("image %q includes the %q label whose value is not within the allowed range", image, assembleLabel)
+		suggestion = fmt.Sprintf("modify the %q label in image %q to use a numeric user within the allowed range, or build without the allowed-uids configuration set", assembleLabel, image)
+	}
+	return Error{
+		Message:    msg,
+		ErrorCode:  UserNotAllowedError,
+		Suggestion: suggestion,
 	}
 }
 

test/integration/images/sti-fake-assemble-root/Dockerfile

@@ -0,0 +1,12 @@
+#
+# This is fake image used for testing STI. It tests running build with the root assemble user
+#
+FROM sti_test/sti-fake
+
+LABEL io.openshift.s2i.assemble-user="0"
+
+RUN mkdir -p /sti-fake && \
+    adduser -u 431 -h /sti-fake -s /sbin/nologin -D fakeuser && \
+    chown -R fakeuser /sti-fake
+
+USER 431

test/integration/images/sti-fake-assemble-user/Dockerfile

@@ -0,0 +1,12 @@
+#
+# This is fake image used for testing STI. It tests running build with an assemble user
+#
+FROM sti_test/sti-fake
+
+LABEL io.openshift.s2i.assemble-user="431"
+
+RUN mkdir -p /sti-fake && \
+    adduser -u 431 -h /sti-fake -s /sbin/nologin -D fakeuser && \
+    chown -R fakeuser /sti-fake
+
+USER 431

test/integration/integration_test.go

@@ -47,6 +47,8 @@ const (
 	FakeNumericUserImage            = "sti_test/sti-fake-numericuser"
 	FakeImageOnBuildRootUser        = "sti_test/sti-fake-onbuild-rootuser"
 	FakeImageOnBuildNumericUser     = "sti_test/sti-fake-onbuild-numericuser"
+	FakeImageAssembleRoot           = "sti_test/sti-fake-assemble-root"
+	FakeImageAssembleUser           = "sti_test/sti-fake-assemble-user"
 
 	TagCleanBuild                              = "test/sti-fake-app"
 	TagCleanBuildUser                          = "test/sti-fake-app-user"
@@ -65,6 +67,8 @@ const (
 	TagCleanBuildAllowedUIDsNumericUser        = "test/sti-fake-alloweduids-numericuser"
 	TagCleanBuildAllowedUIDsOnBuildRoot        = "test/sti-fake-alloweduids-onbuildroot"
 	TagCleanBuildAllowedUIDsOnBuildNumericUser = "test/sti-fake-alloweduids-onbuildnumeric"
+	TagCleanBuildAllowedUIDsAssembleRoot       = "test/sti-fake-alloweduids-assembleroot"
+	TagCleanBuildAllowedUIDsAssembleUser       = "test/sti-fake-alloweduids-assembleuser"
 
 	// Need to serve the scripts from local host so any potential changes to the
 	// scripts are made available for integration testing.
@@ -260,6 +264,14 @@ func TestAllowedUIDsOnBuildNumericUser(t *testing.T) {
 	integration(t).exerciseCleanAllowedUIDsBuild(TagCleanBuildAllowedUIDsNumericUser, FakeImageOnBuildNumericUser, false)
 }
 
+func TestAllowedUIDsAssembleRoot(t *testing.T) {
+	integration(t).exerciseCleanAllowedUIDsBuild(TagCleanBuildAllowedUIDsAssembleRoot, FakeImageAssembleRoot, true)
+}
+
+func TestAllowedUIDsAssembleUser(t *testing.T) {
+	integration(t).exerciseCleanAllowedUIDsBuild(TagCleanBuildAllowedUIDsAssembleUser, FakeImageAssembleUser, false)
+}
+
 func (i *integrationTest) exerciseCleanAllowedUIDsBuild(tag, imageName string, expectError bool) {
 	t := i.t
 	config := &api.Config{