diff --git a/hack/build-test-images.sh b/hack/build-test-images.sh index fece5fdd7..5f71a6823 100755 --- a/hack/build-test-images.sh +++ b/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 diff --git a/hack/test-integration.sh b/hack/test-integration.sh index 2bafddda6..cbf5c5feb 100755 --- a/hack/test-integration.sh +++ b/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 diff --git a/pkg/api/describe/describer.go b/pkg/api/describe/describer.go index 313fd3d84..05bcb2434 100644 --- a/pkg/api/describe/describer.go +++ b/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) } diff --git a/pkg/build/strategies/sti/sti.go b/pkg/build/strategies/sti/sti.go index 63a2d8d3d..496fa23a3 100644 --- a/pkg/build/strategies/sti/sti.go +++ b/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 { diff --git a/pkg/build/strategies/strategies.go b/pkg/build/strategies/strategies.go index 5cacd7f72..13a919e53 100644 --- a/pkg/build/strategies/strategies.go +++ b/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, diff --git a/pkg/cmd/cli/cmd/rebuild.go b/pkg/cmd/cli/cmd/rebuild.go index dfa32db24..7f3b54ef3 100644 --- a/pkg/cmd/cli/cmd/rebuild.go +++ b/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) diff --git a/pkg/docker/util.go b/pkg/docker/util.go index c592a4468..bcaffa976 100644 --- a/pkg/docker/util.go +++ b/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 } diff --git a/pkg/docker/util_test.go b/pkg/docker/util_test.go index d6d324023..d3b94ceff 100644 --- a/pkg/docker/util_test.go +++ b/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) } diff --git a/pkg/errors/errors.go b/pkg/errors/errors.go index 2fba1bc71..06c62f58d 100644 --- a/pkg/errors/errors.go +++ b/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, } } diff --git a/test/integration/images/sti-fake-assemble-root/Dockerfile b/test/integration/images/sti-fake-assemble-root/Dockerfile new file mode 100644 index 000000000..44311297c --- /dev/null +++ b/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 diff --git a/test/integration/images/sti-fake-assemble-user/Dockerfile b/test/integration/images/sti-fake-assemble-user/Dockerfile new file mode 100644 index 000000000..219b98172 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/test/integration/integration_test.go b/test/integration/integration_test.go index b7dd3971c..2e5b12795 100644 --- a/test/integration/integration_test.go +++ b/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{