fix: getUIDandGID is able to resolve non-existing users and groups (#…

…2106)

* fix: getUIDandGID is able to resolve non-existing users and groups

A common pattern in dockerfiles is to provide a plain uid and gid number, which doesn't neccesarily exist inside the os.

Signed-off-by: Höhl, Lukas <lukas.hoehl@accso.de>

* test: add chown dockerfile

Signed-off-by: Höhl, Lukas <lukas.hoehl@accso.de>

* chore: format

Signed-off-by: Höhl, Lukas <lukas.hoehl@accso.de>

* chore: add comment

Signed-off-by: Höhl, Lukas <lukas.hoehl@accso.de>

* tests: fix chown dockerfile

Signed-off-by: Höhl, Lukas <lukas.hoehl@accso.de>

* refactor: split up getIdsFromUsernameAndGroup func

Signed-off-by: Höhl, Lukas <lukas.hoehl@accso.de>

* fix: implement raw uid logic for LookupUser

Signed-off-by: Höhl, Lukas <lukas.hoehl@accso.de>

* test: add dockerfiles for integration test

* fix: lookup user error message

* test: add dockerfiles for non-existing user testcase

* fix: forgot error check

* tests: fix syscall credentials test

* chore: add debug output for copy command

* tests: set specific gid for integration dockerfile

* tests: fix syscall credentials test

github runner had the exact uid that i was testing on, so the groups were not empty

Signed-off-by: Höhl, Lukas <lukas.hoehl@accso.de>

* tests: fix test script

Signed-off-by: Höhl, Lukas <lukas.hoehl@accso.de>

* chore: apply golangci lint checks

Signed-off-by: Höhl, Lukas <lukas.hoehl@accso.de>

* fix: reset file ownership in createFile if not root owned

Signed-off-by: Höhl, Lukas <lukas.hoehl@accso.de>

* chore: logrus.Debugf missed format variable

Signed-off-by: Höhl, Lukas <lukas.hoehl@accso.de>

* chore(test-script): remove go html coverage

Signed-off-by: Höhl, Lukas <lukas.hoehl@accso.de>

* test(k8s): increase wait timeout

Signed-off-by: Höhl, Lukas <lukas.hoehl@accso.de>

Makefile

@@ -66,6 +66,10 @@ minikube-setup:
 test: out/executor
  @ ./scripts/test.sh
 
+test-with-coverage: test
+ go tool cover -html=out/coverage.out
+
+
 .PHONY: integration-test
 integration-test:
  @ ./scripts/integration-test.sh

integration/dockerfiles/Dockerfile_test_copy_chown_nonexisting_user

@@ -0,0 +1,22 @@
+# Copyright 2018 Google, Inc. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+FROM debian:9.11
+RUN echo "hey" > /tmp/foo
+
+FROM debian:9.11
+RUN groupadd --gid 5000 testgroup
+COPY --from=0 --chown=1001:1001 /tmp/foo /tmp/baz
+# with existing group
+COPY --from=0 --chown=1001:testgroup /tmp/foo /tmp/baz

integration/dockerfiles/Dockerfile_test_user_nonexisting

@@ -0,0 +1,25 @@
+# Copyright 2018 Google, Inc. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+FROM debian:9.11
+USER 1001:1001
+RUN echo "hey2" > /tmp/foo
+USER 1001
+RUN echo "hello" > /tmp/foobar
+
+# With existing group
+USER root
+RUN groupadd testgroup
+USER 1001:testgroup
+RUN echo "hello" > /tmp/foobar

integration/k8s_test.go

@@ -89,7 +89,7 @@ func TestK8s(t *testing.T) {
  t.Logf("Waiting for K8s kaniko build job to finish: %s\n",
  "job/kaniko-test-"+job.Name)
 
- kubeWaitCmd := exec.Command("kubectl", "wait", "--for=condition=complete", "--timeout=1m",
+ kubeWaitCmd := exec.Command("kubectl", "wait", "--for=condition=complete", "--timeout=2m",
  "job/kaniko-test-"+job.Name)
  if out, errR := RunCommandWithoutTest(kubeWaitCmd); errR != nil {
  t.Log(kubeWaitCmd.Args)

pkg/commands/copy.go

@@ -53,6 +53,7 @@ func (c *CopyCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.Bu
 
  replacementEnvs := buildArgs.ReplacementEnvs(config.Env)
  uid, gid, err := getUserGroup(c.cmd.Chown, replacementEnvs)
+ logrus.Debugf("found uid %v and gid %v for chown string %v", uid, gid, c.cmd.Chown)
  if err != nil {
  return errors.Wrap(err, "getting user group from chown")
  }

pkg/commands/run.go

@@ -20,7 +20,6 @@ import (
  "fmt"
  "os"
  "os/exec"
- "os/user"
  "strings"
  "syscall"
 
@@ -41,8 +40,7 @@ type RunCommand struct {
 
 // for testing
 var (
- userLookup = user.Lookup
- userLookupID = user.LookupId
+ userLookup = util.LookupUser
 )
 
 func (r *RunCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.BuildArgs) error {
@@ -151,11 +149,7 @@ func addDefaultHOME(u string, envs []string) ([]string, error) {
  // Otherwise the user is set to uid and HOME is /
  userObj, err := userLookup(u)
  if err != nil {
- if uo, e := userLookupID(u); e == nil {
- userObj = uo
- } else {
- return nil, err
- }
+ return nil, fmt.Errorf("lookup user %v: %w", u, err)
  }
 
  return append(envs, fmt.Sprintf("%s=%s", constants.HOME, userObj.HomeDir)), nil
@@ -256,6 +250,7 @@ func (cr *CachingRunCommand) MetadataOnly() bool {
  return false
 }
 
+// todo: this should create the workdir if it doesn't exist, atleast this is what docker does
 func setWorkDirIfExists(workdir string) string {
  if _, err := os.Lstat(workdir); err == nil {
  return workdir

pkg/commands/run_test.go

@@ -18,7 +18,6 @@ package commands
 import (
  "archive/tar"
  "bytes"
- "errors"
  "io"
  "io/ioutil"
  "log"
@@ -38,7 +37,6 @@ func Test_addDefaultHOME(t *testing.T) {
  user string
  mockUser *user.User
  lookupError error
- mockUserID *user.User
  initial []string
  expected []string
  }{
@@ -81,19 +79,18 @@ func Test_addDefaultHOME(t *testing.T) {
  },
  },
  {
- name: "USER is set using the UID",
- user: "newuser",
- lookupError: errors.New("User not found"),
- mockUserID: &user.User{
- Username: "user",
- HomeDir: "/home/user",
+ name: "USER is set using the UID",
+ user: "1000",
+ mockUser: &user.User{
+ Username: "1000",
+ HomeDir: "/",
  },
  initial: []string{
  "PATH=/something/else",
  },
  expected: []string{
  "PATH=/something/else",
- "HOME=/home/user",
+ "HOME=/",
  },
  },
  {
@@ -113,11 +110,10 @@ func Test_addDefaultHOME(t *testing.T) {
  }
  for _, test := range tests {
  t.Run(test.name, func(t *testing.T) {
+ original := userLookup
  userLookup = func(username string) (*user.User, error) { return test.mockUser, test.lookupError }
- userLookupID = func(username string) (*user.User, error) { return test.mockUserID, nil }
  defer func() {
- userLookup = user.Lookup
- userLookupID = user.LookupId
+ userLookup = original
  }()
  actual, err := addDefaultHOME(test.user, test.initial)
  testutil.CheckErrorAndDeepEqual(t, false, err, test.expected, actual)

pkg/commands/user.go

@@ -28,11 +28,6 @@ import (
  "github.com/sirupsen/logrus"
 )
 
-// for testing
-var (
- Lookup = util.Lookup
-)
-
 type UserCommand struct {
  BaseCommand
  cmd *instructions.UserCommand

pkg/commands/user_test.go

@@ -16,12 +16,10 @@ limitations under the License.
 package commands
 
 import (
- "fmt"
  "os/user"
  "testing"
 
  "github.com/GoogleContainerTools/kaniko/pkg/dockerfile"
- "github.com/GoogleContainerTools/kaniko/pkg/util"
 
  "github.com/GoogleContainerTools/kaniko/testutil"
  v1 "github.com/google/go-containerregistry/pkg/v1"
@@ -109,13 +107,6 @@ func TestUpdateUser(t *testing.T) {
  User: test.user,
  },
  }
- Lookup = func(_ string) (*user.User, error) {
- if test.userObj != nil {
- return test.userObj, nil
- }
- return nil, fmt.Errorf("error while looking up user")
- }
- defer func() { Lookup = util.Lookup }()
  buildArgs := dockerfile.NewBuildArgs([]string{})
  err := cmd.ExecuteCommand(cfg, buildArgs)
  testutil.CheckErrorAndDeepEqual(t, false, err, test.expectedUID, cfg.User)

pkg/util/command_util.go

@@ -23,7 +23,6 @@ import (
  "os"
  "os/user"
  "path/filepath"
- reflect "reflect"
  "strconv"
  "strings"
 
@@ -39,7 +38,7 @@ import (
 
 // for testing
 var (
- getUIDAndGID = GetUIDAndGIDFromString
+ getUIDAndGIDFunc = getUIDAndGID
 )
 
 const (
@@ -353,7 +352,7 @@ func GetUserGroup(chownStr string, env []string) (int64, int64, error) {
  return -1, -1, err
  }
 
- uid32, gid32, err := getUIDAndGID(chown, true)
+ uid32, gid32, err := getUIDAndGIDFromString(chown, true)
  if err != nil {
  return -1, -1, err
  }
@@ -364,86 +363,112 @@ func GetUserGroup(chownStr string, env []string) (int64, int64, error) {
 // Extract user and group id from a string formatted 'user:group'.
 // If fallbackToUID is set, the gid is equal to uid if the group is not specified
 // otherwise gid is set to zero.
-func GetUIDAndGIDFromString(userGroupString string, fallbackToUID bool) (uint32, uint32, error) {
+// UserID and GroupID don't need to be present on the system.
+func getUIDAndGIDFromString(userGroupString string, fallbackToUID bool) (uint32, uint32, error) {
  userAndGroup := strings.Split(userGroupString, ":")
  userStr := userAndGroup[0]
  var groupStr string
  if len(userAndGroup) > 1 {
  groupStr = userAndGroup[1]
  }
+ return getUIDAndGIDFunc(userStr, groupStr, fallbackToUID)
+}
 
- if reflect.TypeOf(userStr).String() == "int" {
- return 0, 0, nil
- }
-
- uidStr, gidStr, err := GetUserFromUsername(userStr, groupStr, fallbackToUID)
+func getUIDAndGID(userStr string, groupStr string, fallbackToUID bool) (uint32, uint32, error) {
+ user, err := LookupUser(userStr)
  if err != nil {
  return 0, 0, err
  }
-
- // uid and gid need to be fit into uint32
- uid64, err := strconv.ParseUint(uidStr, 10, 32)
+ uid32, err := getUID(user.Uid)
  if err != nil {
  return 0, 0, err
  }
 
- gid64, err := strconv.ParseUint(gidStr, 10, 32)
+ gid, err := getGIDFromName(groupStr, fallbackToUID)
  if err != nil {
+ if errors.Is(err, fallbackToUIDError) {
+ return uid32, uid32, nil
+ }
  return 0, 0, err
  }
-
- return uint32(uid64), uint32(gid64), nil
+ return uid32, gid, nil
 }
 
-func GetUserFromUsername(userStr string, groupStr string, fallbackToUID bool) (string, string, error) {
- // Lookup by username
- userObj, err := Lookup(userStr)
+// getGID tries to parse the gid or falls back to getGroupFromName if it's not an id
+func getGID(groupStr string, fallbackToUID bool) (uint32, error) {
+ gid, err := strconv.ParseUint(groupStr, 10, 32)
  if err != nil {
- return "", "", err
+ return 0, fallbackToUIDOrError(err, fallbackToUID)
  }
+ return uint32(gid), nil
+}
 
- // Same dance with groups
- var group *user.Group
- if groupStr != "" {
- group, err = user.LookupGroup(groupStr)
+// getGIDFromName tries to parse the groupStr into an existing group.
+// if the group doesn't exist, fallback to getGID to parse non-existing valid GIDs.
+func getGIDFromName(groupStr string, fallbackToUID bool) (uint32, error) {
+ group, err := user.LookupGroup(groupStr)
+ if err != nil {
+ // unknown group error could relate to a non existing group
+ var groupErr *user.UnknownGroupError
+ if errors.Is(err, groupErr) {
+ return getGID(groupStr, fallbackToUID)
+ }
+ group, err = user.LookupGroupId(groupStr)
  if err != nil {
- if _, ok := err.(user.UnknownGroupError); !ok {
- return "", "", err
- }
- group, err = user.LookupGroupId(groupStr)
- if err != nil {
- return "", "", err
- }
+ return getGID(groupStr, fallbackToUID)
  }
  }
+ return getGID(group.Gid, fallbackToUID)
+}
+
+var fallbackToUIDError = new(fallbackToUIDErrorType)
+
+type fallbackToUIDErrorType struct{}
 
- uid := userObj.Uid
- gid := "0"
+func (e fallbackToUIDErrorType) Error() string {
+ return "fallback to uid"
+}
+
+func fallbackToUIDOrError(err error, fallbackToUID bool) error {
  if fallbackToUID {
- gid = userObj.Gid
+ return fallbackToUIDError
  }
- if group != nil {
- gid = group.Gid
- }
-
- return uid, gid, nil
+ return err
 }
 
-func Lookup(userStr string) (*user.User, error) {
+// LookupUser will try to lookup the userStr inside the passwd file.
+// If the user does not exists, the function will fallback to parsing the userStr as an uid.
+func LookupUser(userStr string) (*user.User, error) {
  userObj, err := user.Lookup(userStr)
  if err != nil {
- if _, ok := err.(user.UnknownUserError); !ok {
+ unknownUserErr := new(user.UnknownUserError)
+ // only return if it's not an unknown user error
+ if !errors.As(err, unknownUserErr) {
  return nil, err
  }
 
  // Lookup by id
- u, e := user.LookupId(userStr)
- if e != nil {
- return nil, err
+ userObj, err = user.LookupId(userStr)
+ if err != nil {
+ uid, err := getUID(userStr)
+ if err != nil {
+ // at this point, the user does not exist and the userStr is not a valid number.
+ return nil, fmt.Errorf("user %v is not a uid and does not exist on the system", userStr)
+ }
+ userObj = &user.User{
+ Uid: fmt.Sprint(uid),
+ HomeDir: "/",
+ }
  }
-
- userObj = u
  }
-
  return userObj, nil
 }
+
+func getUID(userStr string) (uint32, error) {
+ // checkif userStr is a valid id
+ uid, err := strconv.ParseUint(userStr, 10, 32)
+ if err != nil {
+ return 0, err
+ }
+ return uint32(uid), nil
+}