From 9b656eb5f5902e22e136a259101ad5bbc8420e7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=B6hl=2C=20Lukas?= Date: Mon, 23 May 2022 16:24:31 +0200 Subject: [PATCH 01/21] fix: getUIDandGID is able to resolve non-existing users and groups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- pkg/commands/user.go | 2 +- pkg/commands/user_test.go | 2 +- pkg/util/command_util.go | 81 ++++++++++++++++++++------------- pkg/util/command_util_test.go | 67 ++++++++++++++++++++------- pkg/util/syscall_credentials.go | 2 +- 5 files changed, 104 insertions(+), 50 deletions(-) diff --git a/pkg/commands/user.go b/pkg/commands/user.go index ed31c7b560..d58df49fdb 100644 --- a/pkg/commands/user.go +++ b/pkg/commands/user.go @@ -30,7 +30,7 @@ import ( // for testing var ( - Lookup = util.Lookup + Lookup = util.LookupUser ) type UserCommand struct { diff --git a/pkg/commands/user_test.go b/pkg/commands/user_test.go index 99775b714c..d59d748dce 100644 --- a/pkg/commands/user_test.go +++ b/pkg/commands/user_test.go @@ -115,7 +115,7 @@ func TestUpdateUser(t *testing.T) { } return nil, fmt.Errorf("error while looking up user") } - defer func() { Lookup = util.Lookup }() + defer func() { Lookup = util.LookupUser }() buildArgs := dockerfile.NewBuildArgs([]string{}) err := cmd.ExecuteCommand(cfg, buildArgs) testutil.CheckErrorAndDeepEqual(t, false, err, test.expectedUID, cfg.User) diff --git a/pkg/util/command_util.go b/pkg/util/command_util.go index 615ef99fa4..b876d17a83 100644 --- a/pkg/util/command_util.go +++ b/pkg/util/command_util.go @@ -23,7 +23,6 @@ import ( "os" "os/user" "path/filepath" - reflect "reflect" "strconv" "strings" @@ -39,7 +38,8 @@ import ( // for testing var ( - getUIDAndGID = GetUIDAndGIDFromString + getIdsFromUsernameAndGroup = getIdsFromUsernameAndGroupImpl + getGroupFromStr = getGroup ) const ( @@ -353,7 +353,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 } @@ -372,48 +372,45 @@ func GetUIDAndGIDFromString(userGroupString string, fallbackToUID bool) (uint32, groupStr = userAndGroup[1] } - if reflect.TypeOf(userStr).String() == "int" { - return 0, 0, nil - } - - uidStr, gidStr, err := GetUserFromUsername(userStr, groupStr, fallbackToUID) - if err != nil { - return 0, 0, err - } - - // uid and gid need to be fit into uint32 - uid64, err := strconv.ParseUint(uidStr, 10, 32) + // checkif userStr is a valid id + uid, err := strconv.ParseUint(userStr, 10, 32) if err != nil { - return 0, 0, err + return getIdsFromUsernameAndGroup(userStr, groupStr, fallbackToUID) } - gid64, err := strconv.ParseUint(gidStr, 10, 32) + gid, err := strconv.ParseUint(groupStr, 10, 32) if err != nil { - return 0, 0, err + // check if the group is specified as a string + group, err := getGroupFromStr(groupStr) + if err != nil && !fallbackToUID { + return 0, 0, err + } + gid, err := strconv.ParseUint(group.Gid, 10, 32) + if err != nil && !fallbackToUID { + // no fallback and group is not a valid uid (this should never be the case, because then the whole group is malformed) + return 0, 0, err + } else if err != nil && fallbackToUID { + return uint32(uid), uint32(uid), nil + } + return uint32(uid), uint32(gid), nil } - return uint32(uid64), uint32(gid64), nil + return uint32(uid), uint32(gid), nil } -func GetUserFromUsername(userStr string, groupStr string, fallbackToUID bool) (string, string, error) { +func getIdsFromUsernameAndGroupImpl(userStr string, groupStr string, fallbackToUID bool) (uint32, uint32, error) { // Lookup by username - userObj, err := Lookup(userStr) + userObj, err := LookupUser(userStr) if err != nil { - return "", "", err + return 0, 0, err } // Same dance with groups var group *user.Group if groupStr != "" { - group, err = user.LookupGroup(groupStr) + group, err = getGroupFromStr(groupStr) if err != nil { - if _, ok := err.(user.UnknownGroupError); !ok { - return "", "", err - } - group, err = user.LookupGroupId(groupStr) - if err != nil { - return "", "", err - } + return 0, 0, err } } @@ -426,10 +423,32 @@ func GetUserFromUsername(userStr string, groupStr string, fallbackToUID bool) (s gid = group.Gid } - return uid, gid, nil + uid64, err := strconv.ParseUint(uid, 10, 32) + if err != nil { + return 0, 0, err + } + gid64, err := strconv.ParseUint(gid, 10, 32) + if err != nil { + return 0, 0, err + } + return uint32(uid64), uint32(gid64), nil +} + +func getGroup(groupStr string) (*user.Group, error) { + group, err := user.LookupGroup(groupStr) + if err != nil { + if _, ok := err.(user.UnknownGroupError); !ok { + return nil, err + } + group, err = user.LookupGroupId(groupStr) + if err != nil { + return nil, err + } + } + return group, nil } -func Lookup(userStr string) (*user.User, error) { +func LookupUser(userStr string) (*user.User, error) { userObj, err := user.Lookup(userStr) if err != nil { if _, ok := err.(user.UnknownUserError); !ok { diff --git a/pkg/util/command_util_test.go b/pkg/util/command_util_test.go index 632ef2eb42..cd35637ad6 100644 --- a/pkg/util/command_util_test.go +++ b/pkg/util/command_util_test.go @@ -555,28 +555,32 @@ func Test_RemoteUrls(t *testing.T) { func TestGetUserGroup(t *testing.T) { tests := []struct { - description string - chown string - env []string - mock func(string, bool) (uint32, uint32, error) - expectedU int64 - expectedG int64 - shdErr bool + description string + chown string + env []string + mockIDGetter func(userStr string, groupStr string, fallbackToUID bool) (uint32, uint32, error) + // needed, in case uid is a valid number, but group is a name + mockGroupIDGetter func(groupStr string) (*user.Group, error) + expectedU int64 + expectedG int64 + shdErr bool }{ { description: "non empty chown", chown: "some:some", env: []string{}, - mock: func(string, bool) (uint32, uint32, error) { return 100, 1000, nil }, - expectedU: 100, - expectedG: 1000, + mockIDGetter: func(string, string, bool) (uint32, uint32, error) { + return 100, 1000, nil + }, + expectedU: 100, + expectedG: 1000, }, { description: "non empty chown with env replacement", chown: "some:$foo", env: []string{"foo=key"}, - mock: func(c string, t bool) (uint32, uint32, error) { - if c == "some:key" { + mockIDGetter: func(userStr string, groupStr string, fallbackToUID bool) (uint32, uint32, error) { + if userStr == "some" && groupStr == "key" { return 10, 100, nil } return 0, 0, fmt.Errorf("did not resolve environment variable") @@ -586,18 +590,48 @@ func TestGetUserGroup(t *testing.T) { }, { description: "empty chown string", - mock: func(c string, t bool) (uint32, uint32, error) { + mockIDGetter: func(string, string, bool) (uint32, uint32, error) { return 0, 0, fmt.Errorf("should not be called") }, expectedU: -1, expectedG: -1, }, + { + description: "with uid instead of username", + chown: "1001:1001", + env: []string{}, + mockIDGetter: func(string, string, bool) (uint32, uint32, error) { + return 0, 0, fmt.Errorf("should not be called") + }, + expectedU: 1001, + expectedG: 1001, + }, + { + description: "uid but group is a name", + chown: "1001:mygroup", + env: []string{}, + mockIDGetter: func(string, string, bool) (uint32, uint32, error) { + return 0, 0, fmt.Errorf("should not be called") + }, + mockGroupIDGetter: func(groupStr string) (*user.Group, error) { + // random uid for mygroup since it's a test + return &user.Group{Gid: "2000"}, nil + }, + expectedU: 1001, + expectedG: 2000, + shdErr: false, + }, } for _, tc := range tests { t.Run(tc.description, func(t *testing.T) { - original := getUIDAndGID - defer func() { getUIDAndGID = original }() - getUIDAndGID = tc.mock + originalIDGetter := getIdsFromUsernameAndGroup + originalGroupIDGetter := getGroupFromStr + defer func() { + getIdsFromUsernameAndGroup = originalIDGetter + getGroupFromStr = originalGroupIDGetter + }() + getIdsFromUsernameAndGroup = tc.mockIDGetter + getGroupFromStr = tc.mockGroupIDGetter uid, gid, err := GetUserGroup(tc.chown, tc.env) testutil.CheckErrorAndDeepEqual(t, tc.shdErr, err, uid, tc.expectedU) testutil.CheckErrorAndDeepEqual(t, tc.shdErr, err, gid, tc.expectedG) @@ -686,6 +720,7 @@ func Test_GetUIDAndGIDFromString(t *testing.T) { for _, tt := range testCases { uid, gid, err := GetUIDAndGIDFromString(tt, false) if uid != uint32(expectedU) || gid != uint32(expectedG) || err != nil { + t.Logf("Error: %v", err) t.Errorf("Could not correctly decode %s to uid/gid %d:%d. Result: %d:%d", tt, expectedU, expectedG, uid, gid) } diff --git a/pkg/util/syscall_credentials.go b/pkg/util/syscall_credentials.go index 8aa9b11aa6..e04cdaa283 100644 --- a/pkg/util/syscall_credentials.go +++ b/pkg/util/syscall_credentials.go @@ -30,7 +30,7 @@ func SyscallCredentials(userStr string) (*syscall.Credential, error) { return nil, errors.Wrap(err, "get uid/gid") } - u, err := Lookup(userStr) + u, err := LookupUser(userStr) if err != nil { return nil, errors.Wrap(err, "lookup") } From 248f0f61689c939d07cc9b4423463836cb4522c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=B6hl=2C=20Lukas?= Date: Mon, 23 May 2022 16:46:16 +0200 Subject: [PATCH 02/21] test: add chown dockerfile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Höhl, Lukas --- .../dockerfiles/Dockerfile_test_chown_non_existing_user | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 integration/dockerfiles/Dockerfile_test_chown_non_existing_user diff --git a/integration/dockerfiles/Dockerfile_test_chown_non_existing_user b/integration/dockerfiles/Dockerfile_test_chown_non_existing_user new file mode 100644 index 0000000000..63ea5a2696 --- /dev/null +++ b/integration/dockerfiles/Dockerfile_test_chown_non_existing_user @@ -0,0 +1,6 @@ +FROM debian:10.2 AS first + +RUN echo "hello world" >/tmp/hello + +FROM debian:10.2 as second +COPY --from second --chown 999:999 /tmp/hello /hello From 497ba3d7f720c8001deed57925c4808e6a71ba9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=B6hl=2C=20Lukas?= Date: Mon, 23 May 2022 16:46:52 +0200 Subject: [PATCH 03/21] chore: format MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Höhl, Lukas --- pkg/util/command_util_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/util/command_util_test.go b/pkg/util/command_util_test.go index cd35637ad6..dee9e12c77 100644 --- a/pkg/util/command_util_test.go +++ b/pkg/util/command_util_test.go @@ -619,7 +619,7 @@ func TestGetUserGroup(t *testing.T) { }, expectedU: 1001, expectedG: 2000, - shdErr: false, + shdErr: false, }, } for _, tc := range tests { From bb82ecfb7aef49bd41fa96dbda70856cc54d3a0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=B6hl=2C=20Lukas?= Date: Mon, 23 May 2022 16:48:36 +0200 Subject: [PATCH 04/21] chore: add comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Höhl, Lukas --- pkg/util/command_util.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/util/command_util.go b/pkg/util/command_util.go index b876d17a83..25a5c52ca2 100644 --- a/pkg/util/command_util.go +++ b/pkg/util/command_util.go @@ -364,6 +364,7 @@ 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. +// 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] From 88d52bc32fe6c172d704ed29a407716a0cf3f8b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=B6hl=2C=20Lukas?= Date: Mon, 23 May 2022 16:56:07 +0200 Subject: [PATCH 05/21] tests: fix chown dockerfile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Höhl, Lukas --- .../dockerfiles/Dockerfile_test_chown_non_existing_user | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/integration/dockerfiles/Dockerfile_test_chown_non_existing_user b/integration/dockerfiles/Dockerfile_test_chown_non_existing_user index 63ea5a2696..31011104cb 100644 --- a/integration/dockerfiles/Dockerfile_test_chown_non_existing_user +++ b/integration/dockerfiles/Dockerfile_test_chown_non_existing_user @@ -2,5 +2,5 @@ FROM debian:10.2 AS first RUN echo "hello world" >/tmp/hello -FROM debian:10.2 as second -COPY --from second --chown 999:999 /tmp/hello /hello +FROM debian:10.2 AS second +COPY --from=first --chown=999:999 /tmp/hello /hello From ebfffdd1fd4358e4f8f523b5183720c7b94d4e00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=B6hl=2C=20Lukas?= Date: Wed, 25 May 2022 20:42:23 +0200 Subject: [PATCH 06/21] refactor: split up getIdsFromUsernameAndGroup func MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Höhl, Lukas --- Makefile | 4 + pkg/util/command_util.go | 121 +++++++++++----------- pkg/util/command_util_test.go | 177 ++++++++++++++++++++++++-------- pkg/util/syscall_credentials.go | 2 +- scripts/test.sh | 13 ++- 5 files changed, 211 insertions(+), 106 deletions(-) diff --git a/Makefile b/Makefile index f161d5c855..6c6dbc854b 100644 --- a/Makefile +++ b/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 diff --git a/pkg/util/command_util.go b/pkg/util/command_util.go index 25a5c52ca2..7a966fc180 100644 --- a/pkg/util/command_util.go +++ b/pkg/util/command_util.go @@ -38,8 +38,7 @@ import ( // for testing var ( - getIdsFromUsernameAndGroup = getIdsFromUsernameAndGroupImpl - getGroupFromStr = getGroup + GetUIDAndGID = getUIDAndGID ) const ( @@ -353,7 +352,7 @@ func GetUserGroup(chownStr string, env []string) (int64, int64, error) { return -1, -1, err } - uid32, gid32, err := GetUIDAndGIDFromString(chown, true) + uid32, gid32, err := getUIDAndGIDFromString(chown, true) if err != nil { return -1, -1, err } @@ -365,94 +364,82 @@ func GetUserGroup(chownStr string, env []string) (int64, int64, error) { // If fallbackToUID is set, the gid is equal to uid if the group is not specified // otherwise gid is set to zero. // UserID and GroupID don't need to be present on the system. -func GetUIDAndGIDFromString(userGroupString string, fallbackToUID bool) (uint32, uint32, error) { +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] } - - // checkif userStr is a valid id - uid, err := strconv.ParseUint(userStr, 10, 32) - if err != nil { - return getIdsFromUsernameAndGroup(userStr, groupStr, fallbackToUID) - } - - gid, err := strconv.ParseUint(groupStr, 10, 32) - if err != nil { - // check if the group is specified as a string - group, err := getGroupFromStr(groupStr) - if err != nil && !fallbackToUID { - return 0, 0, err - } - gid, err := strconv.ParseUint(group.Gid, 10, 32) - if err != nil && !fallbackToUID { - // no fallback and group is not a valid uid (this should never be the case, because then the whole group is malformed) - return 0, 0, err - } else if err != nil && fallbackToUID { - return uint32(uid), uint32(uid), nil - } - return uint32(uid), uint32(gid), nil - } - - return uint32(uid), uint32(gid), nil + return GetUIDAndGID(userStr, groupStr, fallbackToUID) } -func getIdsFromUsernameAndGroupImpl(userStr string, groupStr string, fallbackToUID bool) (uint32, uint32, error) { - // Lookup by username - userObj, err := LookupUser(userStr) +func getUIDAndGID(userStr string, groupStr string, fallbackToUID bool) (uint32, uint32, error) { + uid32, err := LookupUID(userStr) if err != nil { return 0, 0, err } - // Same dance with groups - var group *user.Group - if groupStr != "" { - group, err = getGroupFromStr(groupStr) - if err != nil { - return 0, 0, err + gid, err := getGIDFromName(groupStr, fallbackToUID) + if err != nil { + if errors.Is(err, fallbackToUIDError) { + return uid32, uid32, nil } + return 0, 0, err } + return uid32, uint32(gid), nil +} - uid := userObj.Uid - gid := "0" - if fallbackToUID { - gid = userObj.Gid - } - if group != nil { - gid = group.Gid - } +func getUID() - uid64, err := strconv.ParseUint(uid, 10, 32) - if err != nil { - return 0, 0, err - } - gid64, err := strconv.ParseUint(gid, 10, 32) +// 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 0, 0, err + return 0, fallbackToUIDOrError(err, fallbackToUID) } - return uint32(uid64), uint32(gid64), nil + return uint32(gid), nil } -func getGroup(groupStr string) (*user.Group, error) { +// 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 { - if _, ok := err.(user.UnknownGroupError); !ok { - return nil, err + // 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 { - return nil, err + return getGID(groupStr, fallbackToUID) } } - return group, nil + return getGID(group.Gid, fallbackToUID) +} + +var fallbackToUIDError = new(fallbackToUIDErrorType) + +type fallbackToUIDErrorType struct{} + +func (e fallbackToUIDErrorType) Error() string { + return "fallback to uid" +} + +func fallbackToUIDOrError(err error, fallbackToUID bool) error { + if fallbackToUID { + return fallbackToUIDError + } else { + return err + } } func LookupUser(userStr string) (*user.User, error) { userObj, err := user.Lookup(userStr) if err != nil { if _, ok := err.(user.UnknownUserError); !ok { + // try parsing the UID return nil, err } @@ -467,3 +454,21 @@ func LookupUser(userStr string) (*user.User, error) { return userObj, nil } + +func LookupUID(userStr string) (uint32, error) { + // checkif userStr is a valid id + uid, err := strconv.ParseUint(userStr, 10, 32) + if err != nil { + // userStr is not a valid uid, so lookup the name + user, err := LookupUser(userStr) + if err != nil { + return 0, err + } + // returned uid of the user is not a valid uid. This is rarely the case + uid, err = strconv.ParseUint(user.Uid, 10, 32) + if err != nil { + return 0, err + } + } + return uint32(uid), nil +} diff --git a/pkg/util/command_util_test.go b/pkg/util/command_util_test.go index dee9e12c77..b82d5a5fd7 100644 --- a/pkg/util/command_util_test.go +++ b/pkg/util/command_util_test.go @@ -596,42 +596,14 @@ func TestGetUserGroup(t *testing.T) { expectedU: -1, expectedG: -1, }, - { - description: "with uid instead of username", - chown: "1001:1001", - env: []string{}, - mockIDGetter: func(string, string, bool) (uint32, uint32, error) { - return 0, 0, fmt.Errorf("should not be called") - }, - expectedU: 1001, - expectedG: 1001, - }, - { - description: "uid but group is a name", - chown: "1001:mygroup", - env: []string{}, - mockIDGetter: func(string, string, bool) (uint32, uint32, error) { - return 0, 0, fmt.Errorf("should not be called") - }, - mockGroupIDGetter: func(groupStr string) (*user.Group, error) { - // random uid for mygroup since it's a test - return &user.Group{Gid: "2000"}, nil - }, - expectedU: 1001, - expectedG: 2000, - shdErr: false, - }, } for _, tc := range tests { t.Run(tc.description, func(t *testing.T) { - originalIDGetter := getIdsFromUsernameAndGroup - originalGroupIDGetter := getGroupFromStr + originalIDGetter := GetUIDAndGID defer func() { - getIdsFromUsernameAndGroup = originalIDGetter - getGroupFromStr = originalGroupIDGetter + GetUIDAndGID = originalIDGetter }() - getIdsFromUsernameAndGroup = tc.mockIDGetter - getGroupFromStr = tc.mockGroupIDGetter + GetUIDAndGID = tc.mockIDGetter uid, gid, err := GetUserGroup(tc.chown, tc.env) testutil.CheckErrorAndDeepEqual(t, tc.shdErr, err, uid, tc.expectedU) testutil.CheckErrorAndDeepEqual(t, tc.shdErr, err, gid, tc.expectedG) @@ -709,19 +681,140 @@ func Test_GetUIDAndGIDFromString(t *testing.T) { } primaryGroup := primaryGroupObj.Name - testCases := []string{ - fmt.Sprintf("%s:%s", currentUser.Uid, currentUser.Gid), - fmt.Sprintf("%s:%s", currentUser.Username, currentUser.Gid), - fmt.Sprintf("%s:%s", currentUser.Uid, primaryGroup), - fmt.Sprintf("%s:%s", currentUser.Username, primaryGroup), + type args struct { + userGroupStr string + fallbackToUID bool + } + + type expected struct { + userID uint32 + groupID uint32 + } + + expectedCurrentUserID, _ := strconv.ParseUint(currentUser.Uid, 10, 32) + expectedCurrentUserGID, _ := strconv.ParseUint(currentUser.Gid, 10, 32) + + expectedCurrentUser := expected{ + userID: uint32(expectedCurrentUserID), + groupID: uint32(expectedCurrentUserGID), + } + + testCases := []struct { + testname string + args args + expected expected + wantErr bool + }{ + { + testname: "current user uid and gid", + args: args{ + userGroupStr: fmt.Sprintf("%s:%s", currentUser.Uid, currentUser.Gid), + }, + expected: expectedCurrentUser, + }, + { + testname: "current user username and gid", + args: args{ + userGroupStr: fmt.Sprintf("%s:%s", currentUser.Username, currentUser.Gid), + }, + expected: expectedCurrentUser, + }, + { + testname: "current user username and primary group", + args: args{ + userGroupStr: fmt.Sprintf("%s:%s", currentUser.Username, primaryGroup), + }, + expected: expectedCurrentUser, + }, + { + testname: "current user uid and primary group", + args: args{ + userGroupStr: fmt.Sprintf("%s:%s", currentUser.Uid, primaryGroup), + }, + expected: expectedCurrentUser, + }, + { + testname: "non-existing valid uid and gid", + args: args{ + userGroupStr: fmt.Sprintf("%d:%d", 1001, 50000), + }, + expected: expected{ + userID: 1001, + groupID: 50000, + }, + }, + { + testname: "uid and existing group", + args: args{ + userGroupStr: fmt.Sprintf("%d:%s", 1001, primaryGroup), + }, + expected: expected{ + userID: 1001, + groupID: uint32(expectedCurrentUserGID), + }, + }, + { + testname: "uid and non existing group-name with fallbackToUID", + args: args{ + userGroupStr: fmt.Sprintf("%d:%s", 1001, "hello-world-group"), + fallbackToUID: true, + }, + expected: expected{ + userID: 1001, + groupID: 1001, + }, + }, + { + testname: "uid and non existing group-name", + args: args{ + userGroupStr: fmt.Sprintf("%d:%s", 1001, "hello-world-group"), + }, + wantErr: true, + }, + { + testname: "name and non existing gid", + args: args{ + userGroupStr: fmt.Sprintf("%s:%d", currentUser.Username, 50000), + }, + expected: expected{ + userID: expectedCurrentUser.userID, + groupID: 50000, + }, + }, + { + testname: "only uid and fallback is false", + args: args{ + userGroupStr: fmt.Sprintf("%d", expectedCurrentUserID), + fallbackToUID: false, + }, + wantErr: true, + }, + { + testname: "only uid and fallback is true", + args: args{ + userGroupStr: fmt.Sprintf("%d", expectedCurrentUserID), + fallbackToUID: true, + }, + expected: expected{ + userID: expectedCurrentUser.userID, + groupID: expectedCurrentUser.userID, + }, + }, + { + testname: "non-existing user without group", + args: args{ + userGroupStr: "helloworlduser", + }, + wantErr: true, + }, } - expectedU, _ := strconv.ParseUint(currentUser.Uid, 10, 32) - expectedG, _ := strconv.ParseUint(currentUser.Gid, 10, 32) for _, tt := range testCases { - uid, gid, err := GetUIDAndGIDFromString(tt, false) - if uid != uint32(expectedU) || gid != uint32(expectedG) || err != nil { - t.Logf("Error: %v", err) - t.Errorf("Could not correctly decode %s to uid/gid %d:%d. Result: %d:%d", tt, expectedU, expectedG, + uid, gid, err := getUIDAndGIDFromString(tt.args.userGroupStr, tt.args.fallbackToUID) + testutil.CheckError(t, tt.wantErr, err) + if uid != tt.expected.userID || gid != tt.expected.groupID { + t.Errorf("Could not correctly decode %s to uid/gid %d:%d. Result: %d:%d", + tt.args.userGroupStr, + tt.expected.userID, tt.expected.groupID, uid, gid) } } diff --git a/pkg/util/syscall_credentials.go b/pkg/util/syscall_credentials.go index e04cdaa283..e09f673c30 100644 --- a/pkg/util/syscall_credentials.go +++ b/pkg/util/syscall_credentials.go @@ -25,7 +25,7 @@ import ( ) func SyscallCredentials(userStr string) (*syscall.Credential, error) { - uid, gid, err := GetUIDAndGIDFromString(userStr, true) + uid, gid, err := getUIDAndGIDFromString(userStr, true) if err != nil { return nil, errors.Wrap(err, "get uid/gid") } diff --git a/scripts/test.sh b/scripts/test.sh index d9439a7218..294fdbefba 100755 --- a/scripts/test.sh +++ b/scripts/test.sh @@ -14,14 +14,16 @@ # See the License for the specific language governing permissions and # limitations under the License. -set -e +DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" + +#set -e RED='\033[0;31m' GREEN='\033[0;32m' RESET='\033[0m' echo "Running go tests..." -go test -cover -v -timeout 60s `go list ./... | grep -v vendor | grep -v integration` | sed ''/PASS/s//$(printf "${GREEN}PASS${RESET}")/'' | sed ''/FAIL/s//$(printf "${RED}FAIL${RESET}")/'' +go test -cover -coverprofile=out/coverage.out -v -timeout 60s `go list ./... | grep -v vendor | grep -v integration` | sed ''/PASS/s//$(printf "${GREEN}PASS${RESET}")/'' | sed ''/FAIL/s//$(printf "${RED}FAIL${RESET}")/'' GO_TEST_EXIT_CODE=${PIPESTATUS[0]} if [[ $GO_TEST_EXIT_CODE -ne 0 ]]; then exit $GO_TEST_EXIT_CODE @@ -29,9 +31,9 @@ fi echo "Running validation scripts..." scripts=( - "hack/boilerplate.sh" - "hack/gofmt.sh" - "hack/linter.sh" + "$DIR/../hack/boilerplate.sh" + "$DIR/../hack/gofmt.sh" + "$DIR/../hack/linter.sh" ) fail=0 for s in "${scripts[@]}" @@ -44,4 +46,5 @@ do fail=1 fi done + go tool cover -html=$DIR/../out/coverage.out exit $fail From c1b06ffe1cb533dc8a85a3805fbdfb20a57a0a9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=B6hl=2C=20Lukas?= Date: Fri, 27 May 2022 12:46:23 +0200 Subject: [PATCH 07/21] fix: implement raw uid logic for LookupUser MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Höhl, Lukas --- pkg/commands/run.go | 10 +--- pkg/commands/run_test.go | 20 +++---- pkg/commands/user.go | 2 +- pkg/commands/user_test.go | 4 +- pkg/util/command_util.go | 48 ++++++++-------- pkg/util/command_util_test.go | 47 ++++++--------- pkg/util/groupids_cgo.go | 4 ++ pkg/util/groupids_fallback.go | 5 ++ pkg/util/syscall_credentials.go | 6 +- pkg/util/syscall_credentials_test.go | 86 ++++++++++++++++++++++++++++ testutil/util.go | 34 +++++++++++ 11 files changed, 186 insertions(+), 80 deletions(-) create mode 100644 pkg/util/syscall_credentials_test.go diff --git a/pkg/commands/run.go b/pkg/commands/run.go index 36a87dc1eb..f0b5f74774 100644 --- a/pkg/commands/run.go +++ b/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,6 @@ 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 append(envs, fmt.Sprintf("%s=%s", constants.HOME, userObj.HomeDir)), nil @@ -256,6 +249,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 diff --git a/pkg/commands/run_test.go b/pkg/commands/run_test.go index 7891329159..0d19d94b26 100644 --- a/pkg/commands/run_test.go +++ b/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) diff --git a/pkg/commands/user.go b/pkg/commands/user.go index d58df49fdb..0c8132da75 100644 --- a/pkg/commands/user.go +++ b/pkg/commands/user.go @@ -30,7 +30,7 @@ import ( // for testing var ( - Lookup = util.LookupUser + lookup = util.LookupUser ) type UserCommand struct { diff --git a/pkg/commands/user_test.go b/pkg/commands/user_test.go index d59d748dce..65edc8a5bf 100644 --- a/pkg/commands/user_test.go +++ b/pkg/commands/user_test.go @@ -109,13 +109,13 @@ func TestUpdateUser(t *testing.T) { User: test.user, }, } - Lookup = func(_ string) (*user.User, error) { + 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.LookupUser }() + defer func() { lookup = util.LookupUser }() buildArgs := dockerfile.NewBuildArgs([]string{}) err := cmd.ExecuteCommand(cfg, buildArgs) testutil.CheckErrorAndDeepEqual(t, false, err, test.expectedUID, cfg.User) diff --git a/pkg/util/command_util.go b/pkg/util/command_util.go index 7a966fc180..f67b7c9f49 100644 --- a/pkg/util/command_util.go +++ b/pkg/util/command_util.go @@ -38,7 +38,7 @@ import ( // for testing var ( - GetUIDAndGID = getUIDAndGID + getUIDAndGIDFunc = getUIDAndGID ) const ( @@ -371,11 +371,15 @@ func getUIDAndGIDFromString(userGroupString string, fallbackToUID bool) (uint32, if len(userAndGroup) > 1 { groupStr = userAndGroup[1] } - return GetUIDAndGID(userStr, groupStr, fallbackToUID) + return getUIDAndGIDFunc(userStr, groupStr, fallbackToUID) } func getUIDAndGID(userStr string, groupStr string, fallbackToUID bool) (uint32, uint32, error) { - uid32, err := LookupUID(userStr) + user, err := LookupUser(userStr) + if err != nil { + return 0, 0, err + } + uid32, err := getUID(user.Uid) if err != nil { return 0, 0, err } @@ -390,8 +394,6 @@ func getUIDAndGID(userStr string, groupStr string, fallbackToUID bool) (uint32, return uid32, uint32(gid), nil } -func getUID() - // 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) @@ -435,40 +437,38 @@ func fallbackToUIDOrError(err error, fallbackToUID bool) 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 { - // try parsing the UID + 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 { + return nil, err + } + userObj = &user.User{ + Uid: fmt.Sprint(uid), + HomeDir: "/", + } } - - userObj = u } - return userObj, nil } -func LookupUID(userStr string) (uint32, error) { +func getUID(userStr string) (uint32, error) { // checkif userStr is a valid id uid, err := strconv.ParseUint(userStr, 10, 32) if err != nil { - // userStr is not a valid uid, so lookup the name - user, err := LookupUser(userStr) - if err != nil { - return 0, err - } - // returned uid of the user is not a valid uid. This is rarely the case - uid, err = strconv.ParseUint(user.Uid, 10, 32) - if err != nil { - return 0, err - } + return 0, err } return uint32(uid), nil } diff --git a/pkg/util/command_util_test.go b/pkg/util/command_util_test.go index b82d5a5fd7..e5b7e111b0 100644 --- a/pkg/util/command_util_test.go +++ b/pkg/util/command_util_test.go @@ -21,7 +21,6 @@ import ( "os/user" "reflect" "sort" - "strconv" "testing" "github.com/GoogleContainerTools/kaniko/testutil" @@ -599,11 +598,11 @@ func TestGetUserGroup(t *testing.T) { } for _, tc := range tests { t.Run(tc.description, func(t *testing.T) { - originalIDGetter := GetUIDAndGID + originalIDGetter := getUIDAndGIDFunc defer func() { - GetUIDAndGID = originalIDGetter + getUIDAndGIDFunc = originalIDGetter }() - GetUIDAndGID = tc.mockIDGetter + getUIDAndGIDFunc = tc.mockIDGetter uid, gid, err := GetUserGroup(tc.chown, tc.env) testutil.CheckErrorAndDeepEqual(t, tc.shdErr, err, uid, tc.expectedU) testutil.CheckErrorAndDeepEqual(t, tc.shdErr, err, gid, tc.expectedG) @@ -667,19 +666,7 @@ func TestResolveEnvironmentReplacementList(t *testing.T) { } func Test_GetUIDAndGIDFromString(t *testing.T) { - currentUser, err := user.Current() - if err != nil { - t.Fatalf("Cannot get current user: %s", err) - } - groups, err := currentUser.GroupIds() - if err != nil || len(groups) == 0 { - t.Fatalf("Cannot get groups for current user: %s", err) - } - primaryGroupObj, err := user.LookupGroupId(groups[0]) - if err != nil { - t.Fatalf("Could not lookup name of group %s: %s", groups[0], err) - } - primaryGroup := primaryGroupObj.Name + currentUser := testutil.GetCurrentUser(t) type args struct { userGroupStr string @@ -691,12 +678,9 @@ func Test_GetUIDAndGIDFromString(t *testing.T) { groupID uint32 } - expectedCurrentUserID, _ := strconv.ParseUint(currentUser.Uid, 10, 32) - expectedCurrentUserGID, _ := strconv.ParseUint(currentUser.Gid, 10, 32) - expectedCurrentUser := expected{ - userID: uint32(expectedCurrentUserID), - groupID: uint32(expectedCurrentUserGID), + userID: currentUser.UID, + groupID: currentUser.GID, } testCases := []struct { @@ -708,28 +692,28 @@ func Test_GetUIDAndGIDFromString(t *testing.T) { { testname: "current user uid and gid", args: args{ - userGroupStr: fmt.Sprintf("%s:%s", currentUser.Uid, currentUser.Gid), + userGroupStr: fmt.Sprintf("%d:%d", currentUser.UID, currentUser.GID), }, expected: expectedCurrentUser, }, { testname: "current user username and gid", args: args{ - userGroupStr: fmt.Sprintf("%s:%s", currentUser.Username, currentUser.Gid), + userGroupStr: fmt.Sprintf("%s:%d", currentUser.Username, currentUser.GID), }, expected: expectedCurrentUser, }, { testname: "current user username and primary group", args: args{ - userGroupStr: fmt.Sprintf("%s:%s", currentUser.Username, primaryGroup), + userGroupStr: fmt.Sprintf("%s:%s", currentUser.Username, currentUser.PrimaryGroup), }, expected: expectedCurrentUser, }, { testname: "current user uid and primary group", args: args{ - userGroupStr: fmt.Sprintf("%s:%s", currentUser.Uid, primaryGroup), + userGroupStr: fmt.Sprintf("%d:%s", currentUser.UID, currentUser.PrimaryGroup), }, expected: expectedCurrentUser, }, @@ -746,11 +730,11 @@ func Test_GetUIDAndGIDFromString(t *testing.T) { { testname: "uid and existing group", args: args{ - userGroupStr: fmt.Sprintf("%d:%s", 1001, primaryGroup), + userGroupStr: fmt.Sprintf("%d:%s", 1001, currentUser.PrimaryGroup), }, expected: expected{ userID: 1001, - groupID: uint32(expectedCurrentUserGID), + groupID: uint32(currentUser.GID), }, }, { @@ -784,7 +768,7 @@ func Test_GetUIDAndGIDFromString(t *testing.T) { { testname: "only uid and fallback is false", args: args{ - userGroupStr: fmt.Sprintf("%d", expectedCurrentUserID), + userGroupStr: fmt.Sprintf("%d", currentUser.UID), fallbackToUID: false, }, wantErr: true, @@ -792,7 +776,7 @@ func Test_GetUIDAndGIDFromString(t *testing.T) { { testname: "only uid and fallback is true", args: args{ - userGroupStr: fmt.Sprintf("%d", expectedCurrentUserID), + userGroupStr: fmt.Sprintf("%d", currentUser.UID), fallbackToUID: true, }, expected: expected{ @@ -812,7 +796,8 @@ func Test_GetUIDAndGIDFromString(t *testing.T) { uid, gid, err := getUIDAndGIDFromString(tt.args.userGroupStr, tt.args.fallbackToUID) testutil.CheckError(t, tt.wantErr, err) if uid != tt.expected.userID || gid != tt.expected.groupID { - t.Errorf("Could not correctly decode %s to uid/gid %d:%d. Result: %d:%d", + t.Errorf("%v failed. Could not correctly decode %s to uid/gid %d:%d. Result: %d:%d", + tt.testname, tt.args.userGroupStr, tt.expected.userID, tt.expected.groupID, uid, gid) diff --git a/pkg/util/groupids_cgo.go b/pkg/util/groupids_cgo.go index 376762e079..73865a638e 100644 --- a/pkg/util/groupids_cgo.go +++ b/pkg/util/groupids_cgo.go @@ -26,5 +26,9 @@ import ( // groupIDs returns all of the group ID's a user is a member of func groupIDs(u *user.User) ([]string, error) { + // user can have no gid if it's a non existing user + if u.Gid == "" { + return []string{}, nil + } return u.GroupIds() } diff --git a/pkg/util/groupids_fallback.go b/pkg/util/groupids_fallback.go index 62d0760244..e336d72e9f 100644 --- a/pkg/util/groupids_fallback.go +++ b/pkg/util/groupids_fallback.go @@ -45,6 +45,11 @@ type group struct { func groupIDs(u *user.User) ([]string, error) { logrus.Infof("Performing slow lookup of group ids for %s", u.Username) + // user can have no gid if it's a non existing user + if u.Gid == "" { + return []string{}, nil + } + f, err := os.Open(groupFile) if err != nil { return nil, errors.Wrap(err, "open") diff --git a/pkg/util/syscall_credentials.go b/pkg/util/syscall_credentials.go index e09f673c30..a316ea004d 100644 --- a/pkg/util/syscall_credentials.go +++ b/pkg/util/syscall_credentials.go @@ -17,6 +17,7 @@ limitations under the License. package util import ( + "fmt" "strconv" "syscall" @@ -30,13 +31,14 @@ func SyscallCredentials(userStr string) (*syscall.Credential, error) { return nil, errors.Wrap(err, "get uid/gid") } - u, err := LookupUser(userStr) + u, err := LookupUser(fmt.Sprint(uid)) if err != nil { return nil, errors.Wrap(err, "lookup") } logrus.Infof("Util.Lookup returned: %+v", u) - var groups []uint32 + // initiliaze empty + groups := []uint32{} gidStr, err := groupIDs(u) if err != nil { diff --git a/pkg/util/syscall_credentials_test.go b/pkg/util/syscall_credentials_test.go new file mode 100644 index 0000000000..64f3e51355 --- /dev/null +++ b/pkg/util/syscall_credentials_test.go @@ -0,0 +1,86 @@ +/* +Copyright 2020 Google LLC + +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. +*/ + +package util + +import ( + "fmt" + "syscall" + "testing" + + "github.com/GoogleContainerTools/kaniko/testutil" +) + +func TestSyscallCredentials(t *testing.T) { + currentUser := testutil.GetCurrentUser(t) + type args struct { + userStr string + } + tests := []struct { + name string + args args + want *syscall.Credential + wantErr bool + }{ + { + name: "non-existing user without group", + args: args{ + userStr: "helloworld-user", + }, + wantErr: true, + }, + { + name: "non-existing uid without group", + args: args{ + userStr: "1001", + }, + want: &syscall.Credential{ + Uid: 1001, + // because fallback is enabled + Gid: 1001, + Groups: []uint32{}, + }, + }, + { + name: "non-existing uid with existing gid", + args: args{ + userStr: fmt.Sprintf("1001:%d", currentUser.GID), + }, + want: &syscall.Credential{ + Uid: 1001, + Gid: currentUser.GID, + Groups: []uint32{}, + }, + }, + { + name: "existing username with non-existing gid", + args: args{ + userStr: fmt.Sprintf("%s:50000", currentUser.Username), + }, + want: &syscall.Credential{ + Uid: currentUser.UID, + Gid: 50000, + Groups: []uint32{currentUser.GID}, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := SyscallCredentials(tt.args.userStr) + testutil.CheckErrorAndDeepEqual(t, tt.wantErr, err, tt.want, got) + }) + } +} diff --git a/testutil/util.go b/testutil/util.go index 9a10ea6daf..333211fa1a 100644 --- a/testutil/util.go +++ b/testutil/util.go @@ -20,8 +20,10 @@ import ( "fmt" "io/ioutil" "os" + "os/user" "path/filepath" "reflect" + "strconv" "testing" "github.com/google/go-cmp/cmp" @@ -41,6 +43,38 @@ func SetupFiles(path string, files map[string]string) error { return nil } +type CurrentUser struct { + Username string + UID uint32 + GID uint32 + PrimaryGroup string +} + +func GetCurrentUser(t *testing.T) CurrentUser { + currentUser, err := user.Current() + if err != nil { + t.Fatalf("Cannot get current user: %s", err) + } + groups, err := currentUser.GroupIds() + if err != nil || len(groups) == 0 { + t.Fatalf("Cannot get groups for current user: %s", err) + } + primaryGroupObj, err := user.LookupGroupId(groups[0]) + if err != nil { + t.Fatalf("Could not lookup name of group %s: %s", groups[0], err) + } + primaryGroup := primaryGroupObj.Name + uid, _ := strconv.ParseUint(currentUser.Uid, 10, 32) + gid, _ := strconv.ParseUint(currentUser.Gid, 10, 32) + + return CurrentUser{ + UID: uint32(uid), + GID: uint32(gid), + PrimaryGroup: primaryGroup, + Username: currentUser.Username, + } +} + func CheckDeepEqual(t *testing.T, expected, actual interface{}) { t.Helper() if diff := cmp.Diff(actual, expected); diff != "" { From e24718416a80d9768729807d844a4353c85880b8 Mon Sep 17 00:00:00 2001 From: Lukas Hoehl Date: Fri, 27 May 2022 13:24:11 +0200 Subject: [PATCH 08/21] test: add dockerfiles for integration test --- ...ockerfile_test_copy_chown_nonexisting_user | 22 +++++++++++++++++++ .../Dockerfile_test_nonexisting_user | 21 ++++++++++++++++++ 2 files changed, 43 insertions(+) create mode 100644 integration/dockerfiles/Dockerfile_test_copy_chown_nonexisting_user create mode 100644 integration/dockerfiles/Dockerfile_test_nonexisting_user diff --git a/integration/dockerfiles/Dockerfile_test_copy_chown_nonexisting_user b/integration/dockerfiles/Dockerfile_test_copy_chown_nonexisting_user new file mode 100644 index 0000000000..9937004594 --- /dev/null +++ b/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 +COPY --from=0 --chown=1001:1001 /tmp/foo /tmp/baz +COPY --from=0 --chown=testuser:testgroup /tmp/foo /tmp/baz +COPY --from=0 --chown=testuser:1001 /tmp/foo /tmp/baz +COPY --from=0 --chown=1001:testgroup /tmp/foo /tmp/baz diff --git a/integration/dockerfiles/Dockerfile_test_nonexisting_user b/integration/dockerfiles/Dockerfile_test_nonexisting_user new file mode 100644 index 0000000000..62c2f38a85 --- /dev/null +++ b/integration/dockerfiles/Dockerfile_test_nonexisting_user @@ -0,0 +1,21 @@ +# 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 testuser:testgroup +RUN echo "hey" > /tmp/foo +USER testuser:1001 +RUN echo "hey2" >> /tmp/foo +USER 1001 +RUN echo "hello" > /tmp/foobar \ No newline at end of file From ab0374ef8f9dd22b3828a21438d6789be8fa11a8 Mon Sep 17 00:00:00 2001 From: Lukas Hoehl Date: Fri, 27 May 2022 13:44:39 +0200 Subject: [PATCH 09/21] fix: lookup user error message --- pkg/util/command_util.go | 3 +- pkg/util/command_util_test.go | 67 ++++++++++++++++++++++++---- pkg/util/syscall_credentials_test.go | 14 ++++-- testutil/util.go | 12 ++--- 4 files changed, 74 insertions(+), 22 deletions(-) diff --git a/pkg/util/command_util.go b/pkg/util/command_util.go index f67b7c9f49..77f871a4b8 100644 --- a/pkg/util/command_util.go +++ b/pkg/util/command_util.go @@ -453,7 +453,8 @@ func LookupUser(userStr string) (*user.User, error) { if err != nil { uid, err := getUID(userStr) if err != nil { - return nil, err + // 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), diff --git a/pkg/util/command_util_test.go b/pkg/util/command_util_test.go index e5b7e111b0..919254ca6f 100644 --- a/pkg/util/command_util_test.go +++ b/pkg/util/command_util_test.go @@ -21,6 +21,7 @@ import ( "os/user" "reflect" "sort" + "strconv" "testing" "github.com/GoogleContainerTools/kaniko/testutil" @@ -678,9 +679,11 @@ func Test_GetUIDAndGIDFromString(t *testing.T) { groupID uint32 } + currentUserUid, _ := strconv.ParseUint(currentUser.Uid, 10, 32) + currentUserGid, _ := strconv.ParseUint(currentUser.Gid, 10, 32) expectedCurrentUser := expected{ - userID: currentUser.UID, - groupID: currentUser.GID, + userID: uint32(currentUserUid), + groupID: uint32(currentUserGid), } testCases := []struct { @@ -692,14 +695,14 @@ func Test_GetUIDAndGIDFromString(t *testing.T) { { testname: "current user uid and gid", args: args{ - userGroupStr: fmt.Sprintf("%d:%d", currentUser.UID, currentUser.GID), + userGroupStr: fmt.Sprintf("%d:%d", currentUserUid, currentUserGid), }, expected: expectedCurrentUser, }, { testname: "current user username and gid", args: args{ - userGroupStr: fmt.Sprintf("%s:%d", currentUser.Username, currentUser.GID), + userGroupStr: fmt.Sprintf("%s:%d", currentUser.Username, currentUserGid), }, expected: expectedCurrentUser, }, @@ -713,7 +716,7 @@ func Test_GetUIDAndGIDFromString(t *testing.T) { { testname: "current user uid and primary group", args: args{ - userGroupStr: fmt.Sprintf("%d:%s", currentUser.UID, currentUser.PrimaryGroup), + userGroupStr: fmt.Sprintf("%d:%s", currentUserUid, currentUser.PrimaryGroup), }, expected: expectedCurrentUser, }, @@ -734,7 +737,7 @@ func Test_GetUIDAndGIDFromString(t *testing.T) { }, expected: expected{ userID: 1001, - groupID: uint32(currentUser.GID), + groupID: uint32(currentUserGid), }, }, { @@ -768,7 +771,7 @@ func Test_GetUIDAndGIDFromString(t *testing.T) { { testname: "only uid and fallback is false", args: args{ - userGroupStr: fmt.Sprintf("%d", currentUser.UID), + userGroupStr: fmt.Sprintf("%d", currentUserUid), fallbackToUID: false, }, wantErr: true, @@ -776,7 +779,7 @@ func Test_GetUIDAndGIDFromString(t *testing.T) { { testname: "only uid and fallback is true", args: args{ - userGroupStr: fmt.Sprintf("%d", currentUser.UID), + userGroupStr: fmt.Sprintf("%d", currentUserUid), fallbackToUID: true, }, expected: expected{ @@ -804,3 +807,51 @@ func Test_GetUIDAndGIDFromString(t *testing.T) { } } } + +func TestLookupUser(t *testing.T) { + currentUser := testutil.GetCurrentUser(t) + + type args struct { + userStr string + } + tests := []struct { + testname string + args args + expected *user.User + wantErr bool + }{ + { + testname: "non-existing user", + args: args{ + userStr: "foobazbar", + }, + wantErr: true, + }, + { + testname: "uid", + args: args{ + userStr: "30000", + }, + expected: &user.User{ + Uid: "30000", + HomeDir: "/", + }, + wantErr: false, + }, + { + testname: "current user", + args: args{ + userStr: currentUser.Username, + }, + expected: currentUser.User, + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.testname, func(t *testing.T) { + got, err := LookupUser(tt.args.userStr) + testutil.CheckErrorAndDeepEqual(t, tt.wantErr, err, tt.expected, got) + }) + } + +} diff --git a/pkg/util/syscall_credentials_test.go b/pkg/util/syscall_credentials_test.go index 64f3e51355..f8efed74ce 100644 --- a/pkg/util/syscall_credentials_test.go +++ b/pkg/util/syscall_credentials_test.go @@ -18,6 +18,7 @@ package util import ( "fmt" + "strconv" "syscall" "testing" @@ -26,6 +27,11 @@ import ( func TestSyscallCredentials(t *testing.T) { currentUser := testutil.GetCurrentUser(t) + uid, _ := strconv.ParseUint(currentUser.Uid, 10, 32) + currentUserUid32 := uint32(uid) + gid, _ := strconv.ParseUint(currentUser.Gid, 10, 32) + currentUserGid32 := uint32(gid) + type args struct { userStr string } @@ -57,11 +63,11 @@ func TestSyscallCredentials(t *testing.T) { { name: "non-existing uid with existing gid", args: args{ - userStr: fmt.Sprintf("1001:%d", currentUser.GID), + userStr: fmt.Sprintf("1001:%d", currentUserGid32), }, want: &syscall.Credential{ Uid: 1001, - Gid: currentUser.GID, + Gid: currentUserGid32, Groups: []uint32{}, }, }, @@ -71,9 +77,9 @@ func TestSyscallCredentials(t *testing.T) { userStr: fmt.Sprintf("%s:50000", currentUser.Username), }, want: &syscall.Credential{ - Uid: currentUser.UID, + Uid: currentUserUid32, Gid: 50000, - Groups: []uint32{currentUser.GID}, + Groups: []uint32{currentUserGid32}, }, }, } diff --git a/testutil/util.go b/testutil/util.go index 333211fa1a..e3d0b04c19 100644 --- a/testutil/util.go +++ b/testutil/util.go @@ -23,7 +23,6 @@ import ( "os/user" "path/filepath" "reflect" - "strconv" "testing" "github.com/google/go-cmp/cmp" @@ -44,9 +43,8 @@ func SetupFiles(path string, files map[string]string) error { } type CurrentUser struct { - Username string - UID uint32 - GID uint32 + *user.User + PrimaryGroup string } @@ -64,14 +62,10 @@ func GetCurrentUser(t *testing.T) CurrentUser { t.Fatalf("Could not lookup name of group %s: %s", groups[0], err) } primaryGroup := primaryGroupObj.Name - uid, _ := strconv.ParseUint(currentUser.Uid, 10, 32) - gid, _ := strconv.ParseUint(currentUser.Gid, 10, 32) return CurrentUser{ - UID: uint32(uid), - GID: uint32(gid), + User: currentUser, PrimaryGroup: primaryGroup, - Username: currentUser.Username, } } From 4df82feaa6749e4e2f7f70f7b2e3f8500af8f541 Mon Sep 17 00:00:00 2001 From: Lukas Hoehl Date: Fri, 27 May 2022 13:52:58 +0200 Subject: [PATCH 10/21] test: add dockerfiles for non-existing user testcase --- .../Dockerfile_test_chown_non_existing_user | 6 ------ .../Dockerfile_test_copy_chown_nonexisting_user | 4 ++-- ...sting_user => Dockerfile_test_user_nonexisting} | 14 +++++++++----- 3 files changed, 11 insertions(+), 13 deletions(-) delete mode 100644 integration/dockerfiles/Dockerfile_test_chown_non_existing_user rename integration/dockerfiles/{Dockerfile_test_nonexisting_user => Dockerfile_test_user_nonexisting} (77%) diff --git a/integration/dockerfiles/Dockerfile_test_chown_non_existing_user b/integration/dockerfiles/Dockerfile_test_chown_non_existing_user deleted file mode 100644 index 31011104cb..0000000000 --- a/integration/dockerfiles/Dockerfile_test_chown_non_existing_user +++ /dev/null @@ -1,6 +0,0 @@ -FROM debian:10.2 AS first - -RUN echo "hello world" >/tmp/hello - -FROM debian:10.2 AS second -COPY --from=first --chown=999:999 /tmp/hello /hello diff --git a/integration/dockerfiles/Dockerfile_test_copy_chown_nonexisting_user b/integration/dockerfiles/Dockerfile_test_copy_chown_nonexisting_user index 9937004594..0b8d887e9b 100644 --- a/integration/dockerfiles/Dockerfile_test_copy_chown_nonexisting_user +++ b/integration/dockerfiles/Dockerfile_test_copy_chown_nonexisting_user @@ -16,7 +16,7 @@ FROM debian:9.11 RUN echo "hey" > /tmp/foo FROM debian:9.11 +RUN groupadd testgroup COPY --from=0 --chown=1001:1001 /tmp/foo /tmp/baz -COPY --from=0 --chown=testuser:testgroup /tmp/foo /tmp/baz -COPY --from=0 --chown=testuser:1001 /tmp/foo /tmp/baz +# with existing group COPY --from=0 --chown=1001:testgroup /tmp/foo /tmp/baz diff --git a/integration/dockerfiles/Dockerfile_test_nonexisting_user b/integration/dockerfiles/Dockerfile_test_user_nonexisting similarity index 77% rename from integration/dockerfiles/Dockerfile_test_nonexisting_user rename to integration/dockerfiles/Dockerfile_test_user_nonexisting index 62c2f38a85..8b65af54ac 100644 --- a/integration/dockerfiles/Dockerfile_test_nonexisting_user +++ b/integration/dockerfiles/Dockerfile_test_user_nonexisting @@ -13,9 +13,13 @@ # limitations under the License. FROM debian:9.11 -USER testuser:testgroup -RUN echo "hey" > /tmp/foo -USER testuser:1001 -RUN echo "hey2" >> /tmp/foo +USER 1001:1001 +RUN echo "hey2" > /tmp/foo USER 1001 -RUN echo "hello" > /tmp/foobar \ No newline at end of file +RUN echo "hello" > /tmp/foobar + +# With existing group +USER root +RUN groupadd testgroup +USER 1001:testgroup +RUN echo "hello" > /tmp/foobar From 61f9f4968583334df161cd5546583d323a4b2618 Mon Sep 17 00:00:00 2001 From: Lukas Hoehl Date: Fri, 27 May 2022 13:56:16 +0200 Subject: [PATCH 11/21] fix: forgot error check --- pkg/commands/run.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/commands/run.go b/pkg/commands/run.go index f0b5f74774..3290ca0192 100644 --- a/pkg/commands/run.go +++ b/pkg/commands/run.go @@ -40,7 +40,7 @@ type RunCommand struct { // for testing var ( - userLookup = util.LookupUser + userLookup = util.LookupUser ) func (r *RunCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.BuildArgs) error { @@ -149,6 +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 { + return nil, fmt.Errorf("lookup user %v: %w", u, err) } return append(envs, fmt.Sprintf("%s=%s", constants.HOME, userObj.HomeDir)), nil From 74b294d31571fbad9a036fc578e7ccedea24daf7 Mon Sep 17 00:00:00 2001 From: Lukas Hoehl Date: Fri, 27 May 2022 17:19:18 +0200 Subject: [PATCH 12/21] tests: fix syscall credentials test --- pkg/util/syscall_credentials_test.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/pkg/util/syscall_credentials_test.go b/pkg/util/syscall_credentials_test.go index f8efed74ce..8a1cb3e20f 100644 --- a/pkg/util/syscall_credentials_test.go +++ b/pkg/util/syscall_credentials_test.go @@ -32,6 +32,13 @@ func TestSyscallCredentials(t *testing.T) { gid, _ := strconv.ParseUint(currentUser.Gid, 10, 32) currentUserGid32 := uint32(gid) + currentUserGroupIDsU32 := []uint32{} + currentUserGroupIDs, _ := currentUser.GroupIds() + for _, id := range currentUserGroupIDs { + id32, _ := strconv.ParseUint(id, 10, 32) + currentUserGroupIDsU32 = append(currentUserGroupIDsU32, uint32(id32)) + } + type args struct { userStr string } @@ -79,7 +86,7 @@ func TestSyscallCredentials(t *testing.T) { want: &syscall.Credential{ Uid: currentUserUid32, Gid: 50000, - Groups: []uint32{currentUserGid32}, + Groups: currentUserGroupIDsU32, }, }, } From 1de32bc6571d6f11630491067698fe47bfcb1ecd Mon Sep 17 00:00:00 2001 From: Lukas Hoehl Date: Fri, 27 May 2022 17:25:05 +0200 Subject: [PATCH 13/21] chore: add debug output for copy command --- pkg/commands/copy.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/commands/copy.go b/pkg/commands/copy.go index 2e572794a0..c85b0f89e8 100644 --- a/pkg/commands/copy.go +++ b/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") } From 6e639b64705448124586d0a6b4e61460d52ce592 Mon Sep 17 00:00:00 2001 From: Lukas Hoehl Date: Sat, 28 May 2022 23:18:10 +0200 Subject: [PATCH 14/21] tests: set specific gid for integration dockerfile --- .../dockerfiles/Dockerfile_test_copy_chown_nonexisting_user | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration/dockerfiles/Dockerfile_test_copy_chown_nonexisting_user b/integration/dockerfiles/Dockerfile_test_copy_chown_nonexisting_user index 0b8d887e9b..eea97ae289 100644 --- a/integration/dockerfiles/Dockerfile_test_copy_chown_nonexisting_user +++ b/integration/dockerfiles/Dockerfile_test_copy_chown_nonexisting_user @@ -16,7 +16,7 @@ FROM debian:9.11 RUN echo "hey" > /tmp/foo FROM debian:9.11 -RUN groupadd testgroup +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 From d75808c3c0c459adbd7b73234c4ef5bb09e0b1f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=B6hl=2C=20Lukas?= Date: Thu, 2 Jun 2022 16:03:10 +0200 Subject: [PATCH 15/21] tests: fix syscall credentials test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit github runner had the exact uid that i was testing on, so the groups were not empty Signed-off-by: Höhl, Lukas --- pkg/util/syscall_credentials_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/util/syscall_credentials_test.go b/pkg/util/syscall_credentials_test.go index 8a1cb3e20f..7008ff32d4 100644 --- a/pkg/util/syscall_credentials_test.go +++ b/pkg/util/syscall_credentials_test.go @@ -58,22 +58,22 @@ func TestSyscallCredentials(t *testing.T) { { name: "non-existing uid without group", args: args{ - userStr: "1001", + userStr: "50000", }, want: &syscall.Credential{ - Uid: 1001, + Uid: 50000, // because fallback is enabled - Gid: 1001, + Gid: 50000, Groups: []uint32{}, }, }, { name: "non-existing uid with existing gid", args: args{ - userStr: fmt.Sprintf("1001:%d", currentUserGid32), + userStr: fmt.Sprintf("50000:%d", currentUserGid32), }, want: &syscall.Credential{ - Uid: 1001, + Uid: 50000, Gid: currentUserGid32, Groups: []uint32{}, }, From b7d354d5f52d216f509202a342dc021c899c101f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=B6hl=2C=20Lukas?= Date: Tue, 7 Jun 2022 22:21:57 +0200 Subject: [PATCH 16/21] tests: fix test script MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Höhl, Lukas --- scripts/test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/test.sh b/scripts/test.sh index 294fdbefba..72684a38ab 100755 --- a/scripts/test.sh +++ b/scripts/test.sh @@ -39,7 +39,7 @@ fail=0 for s in "${scripts[@]}" do echo "RUN ${s}" - if "./${s}"; then + if "${s}"; then echo -e "${GREEN}PASSED${RESET} ${s}" else echo -e "${RED}FAILED${RESET} ${s}" From 830a6534540e472123c7c0951657452dd9f93a7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=B6hl=2C=20Lukas?= Date: Tue, 7 Jun 2022 22:55:38 +0200 Subject: [PATCH 17/21] chore: apply golangci lint checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Höhl, Lukas --- pkg/commands/user.go | 5 ----- pkg/commands/user_test.go | 9 --------- pkg/util/command_util.go | 5 ++--- pkg/util/command_util_test.go | 20 ++++++++++---------- pkg/util/syscall_credentials_test.go | 10 +++++----- 5 files changed, 17 insertions(+), 32 deletions(-) diff --git a/pkg/commands/user.go b/pkg/commands/user.go index 0c8132da75..937a16a691 100644 --- a/pkg/commands/user.go +++ b/pkg/commands/user.go @@ -28,11 +28,6 @@ import ( "github.com/sirupsen/logrus" ) -// for testing -var ( - lookup = util.LookupUser -) - type UserCommand struct { BaseCommand cmd *instructions.UserCommand diff --git a/pkg/commands/user_test.go b/pkg/commands/user_test.go index 65edc8a5bf..d88d11b856 100644 --- a/pkg/commands/user_test.go +++ b/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.LookupUser }() buildArgs := dockerfile.NewBuildArgs([]string{}) err := cmd.ExecuteCommand(cfg, buildArgs) testutil.CheckErrorAndDeepEqual(t, false, err, test.expectedUID, cfg.User) diff --git a/pkg/util/command_util.go b/pkg/util/command_util.go index 77f871a4b8..728f89dc4f 100644 --- a/pkg/util/command_util.go +++ b/pkg/util/command_util.go @@ -391,7 +391,7 @@ func getUIDAndGID(userStr string, groupStr string, fallbackToUID bool) (uint32, } return 0, 0, err } - return uid32, uint32(gid), nil + return uid32, gid, nil } // getGID tries to parse the gid or falls back to getGroupFromName if it's not an id @@ -432,9 +432,8 @@ func (e fallbackToUIDErrorType) Error() string { func fallbackToUIDOrError(err error, fallbackToUID bool) error { if fallbackToUID { return fallbackToUIDError - } else { - return err } + return err } // LookupUser will try to lookup the userStr inside the passwd file. diff --git a/pkg/util/command_util_test.go b/pkg/util/command_util_test.go index 919254ca6f..483fbfff84 100644 --- a/pkg/util/command_util_test.go +++ b/pkg/util/command_util_test.go @@ -679,11 +679,11 @@ func Test_GetUIDAndGIDFromString(t *testing.T) { groupID uint32 } - currentUserUid, _ := strconv.ParseUint(currentUser.Uid, 10, 32) - currentUserGid, _ := strconv.ParseUint(currentUser.Gid, 10, 32) + currentUserUID, _ := strconv.ParseUint(currentUser.Uid, 10, 32) + currentUserGID, _ := strconv.ParseUint(currentUser.Gid, 10, 32) expectedCurrentUser := expected{ - userID: uint32(currentUserUid), - groupID: uint32(currentUserGid), + userID: uint32(currentUserUID), + groupID: uint32(currentUserGID), } testCases := []struct { @@ -695,14 +695,14 @@ func Test_GetUIDAndGIDFromString(t *testing.T) { { testname: "current user uid and gid", args: args{ - userGroupStr: fmt.Sprintf("%d:%d", currentUserUid, currentUserGid), + userGroupStr: fmt.Sprintf("%d:%d", currentUserUID, currentUserGID), }, expected: expectedCurrentUser, }, { testname: "current user username and gid", args: args{ - userGroupStr: fmt.Sprintf("%s:%d", currentUser.Username, currentUserGid), + userGroupStr: fmt.Sprintf("%s:%d", currentUser.Username, currentUserGID), }, expected: expectedCurrentUser, }, @@ -716,7 +716,7 @@ func Test_GetUIDAndGIDFromString(t *testing.T) { { testname: "current user uid and primary group", args: args{ - userGroupStr: fmt.Sprintf("%d:%s", currentUserUid, currentUser.PrimaryGroup), + userGroupStr: fmt.Sprintf("%d:%s", currentUserUID, currentUser.PrimaryGroup), }, expected: expectedCurrentUser, }, @@ -737,7 +737,7 @@ func Test_GetUIDAndGIDFromString(t *testing.T) { }, expected: expected{ userID: 1001, - groupID: uint32(currentUserGid), + groupID: uint32(currentUserGID), }, }, { @@ -771,7 +771,7 @@ func Test_GetUIDAndGIDFromString(t *testing.T) { { testname: "only uid and fallback is false", args: args{ - userGroupStr: fmt.Sprintf("%d", currentUserUid), + userGroupStr: fmt.Sprintf("%d", currentUserUID), fallbackToUID: false, }, wantErr: true, @@ -779,7 +779,7 @@ func Test_GetUIDAndGIDFromString(t *testing.T) { { testname: "only uid and fallback is true", args: args{ - userGroupStr: fmt.Sprintf("%d", currentUserUid), + userGroupStr: fmt.Sprintf("%d", currentUserUID), fallbackToUID: true, }, expected: expected{ diff --git a/pkg/util/syscall_credentials_test.go b/pkg/util/syscall_credentials_test.go index 7008ff32d4..e987dcd773 100644 --- a/pkg/util/syscall_credentials_test.go +++ b/pkg/util/syscall_credentials_test.go @@ -28,9 +28,9 @@ import ( func TestSyscallCredentials(t *testing.T) { currentUser := testutil.GetCurrentUser(t) uid, _ := strconv.ParseUint(currentUser.Uid, 10, 32) - currentUserUid32 := uint32(uid) + currentUserUID32 := uint32(uid) gid, _ := strconv.ParseUint(currentUser.Gid, 10, 32) - currentUserGid32 := uint32(gid) + currentUserGID32 := uint32(gid) currentUserGroupIDsU32 := []uint32{} currentUserGroupIDs, _ := currentUser.GroupIds() @@ -70,11 +70,11 @@ func TestSyscallCredentials(t *testing.T) { { name: "non-existing uid with existing gid", args: args{ - userStr: fmt.Sprintf("50000:%d", currentUserGid32), + userStr: fmt.Sprintf("50000:%d", currentUserGID32), }, want: &syscall.Credential{ Uid: 50000, - Gid: currentUserGid32, + Gid: currentUserGID32, Groups: []uint32{}, }, }, @@ -84,7 +84,7 @@ func TestSyscallCredentials(t *testing.T) { userStr: fmt.Sprintf("%s:50000", currentUser.Username), }, want: &syscall.Credential{ - Uid: currentUserUid32, + Uid: currentUserUID32, Gid: 50000, Groups: currentUserGroupIDsU32, }, From f5e211188cab10bbcd30f7e5090445c1c00d0b5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=B6hl=2C=20Lukas?= Date: Fri, 24 Jun 2022 18:41:21 +0200 Subject: [PATCH 18/21] fix: reset file ownership in createFile if not root owned MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Höhl, Lukas --- pkg/util/fs_util.go | 48 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 41 insertions(+), 7 deletions(-) diff --git a/pkg/util/fs_util.go b/pkg/util/fs_util.go index b21653da2a..14a1b25fdf 100644 --- a/pkg/util/fs_util.go +++ b/pkg/util/fs_util.go @@ -43,8 +43,10 @@ import ( "github.com/sirupsen/logrus" ) -const DoNotChangeUID = -1 -const DoNotChangeGID = -1 +const ( + DoNotChangeUID = -1 + DoNotChangeGID = -1 +) const ( snapshotTimeout = "SNAPSHOT_TIMEOUT_DURATION" @@ -539,6 +541,26 @@ func FilepathExists(path string) bool { return !os.IsNotExist(err) } +// resetFileOwnershipIfNotMatching function changes ownership of the file at path to newUID and newGID. +// If the ownership already matches, chown is not executed. +func resetFileOwnershipIfNotMatching(path string, newUID, newGID uint32) error { + fsInfo, err := os.Lstat(path) + if err != nil { + return errors.Wrap(err, "getting stat of present file") + } + stat, ok := fsInfo.Sys().(*syscall.Stat_t) + if !ok { + return fmt.Errorf("can't convert fs.FileInfo of %v to linux syscall.Stat_t", path) + } + if stat.Uid != newUID && stat.Gid != newGID { + err = os.Chown(path, int(newUID), int(newGID)) + if err != nil { + return errors.Wrap(err, "reseting file ownership to root") + } + } + return nil +} + // CreateFile creates a file at path and copies over contents from the reader func CreateFile(path string, reader io.Reader, perm os.FileMode, uid uint32, gid uint32) error { // Create directory path if it doesn't exist @@ -546,6 +568,15 @@ func CreateFile(path string, reader io.Reader, perm os.FileMode, uid uint32, gid return errors.Wrap(err, "creating parent dir") } + // if the file is already created with ownership other than root, reset the ownership + if FilepathExists(path) { + logrus.Debugf("file at %v already exists, resetting file ownership to root") + err := resetFileOwnershipIfNotMatching(path, 0, 0) + if err != nil { + return errors.Wrap(err, "reseting file ownership") + } + + } dest, err := os.Create(path) if err != nil { return errors.Wrap(err, "creating file") @@ -793,7 +824,12 @@ func mkdirAllWithPermissions(path string, mode os.FileMode, uid, gid int64) erro } if uid > math.MaxUint32 || gid > math.MaxUint32 { // due to https://github.com/golang/go/issues/8537 - return errors.New(fmt.Sprintf("Numeric User-ID or Group-ID greater than %v are not properly supported.", uint64(math.MaxUint32))) + return errors.New( + fmt.Sprintf( + "Numeric User-ID or Group-ID greater than %v are not properly supported.", + uint64(math.MaxUint32), + ), + ) } if err := os.Chown(path, int(uid), int(gid)); err != nil { return err @@ -851,7 +887,6 @@ func CreateTargetTarfile(tarpath string) (*os.File, error) { } } return os.Create(tarpath) - } // Returns true if a file is a symlink @@ -1009,8 +1044,8 @@ type walkFSResult struct { func WalkFS( dir string, existingPaths map[string]struct{}, - changeFunc func(string) (bool, error)) ([]string, map[string]struct{}) { - + changeFunc func(string) (bool, error), +) ([]string, map[string]struct{}) { timeOutStr := os.Getenv(snapshotTimeout) if timeOutStr == "" { logrus.Tracef("Environment '%s' not set. Using default snapshot timeout '%s'", snapshotTimeout, defaultTimeout) @@ -1102,7 +1137,6 @@ func GetFSInfoMap(dir string, existing map[string]os.FileInfo) (map[string]os.Fi fileMap[path] = fi foundPaths = append(foundPaths, path) } - } return nil }, From 8a68ae8541be799edd655478b0a7990e56acf5f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=B6hl=2C=20Lukas?= Date: Mon, 27 Jun 2022 17:48:27 +0200 Subject: [PATCH 19/21] chore: logrus.Debugf missed format variable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Höhl, Lukas --- pkg/util/fs_util.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/util/fs_util.go b/pkg/util/fs_util.go index 14a1b25fdf..d29fb43b46 100644 --- a/pkg/util/fs_util.go +++ b/pkg/util/fs_util.go @@ -570,13 +570,13 @@ func CreateFile(path string, reader io.Reader, perm os.FileMode, uid uint32, gid // if the file is already created with ownership other than root, reset the ownership if FilepathExists(path) { - logrus.Debugf("file at %v already exists, resetting file ownership to root") + logrus.Debugf("file at %v already exists, resetting file ownership to root", path) err := resetFileOwnershipIfNotMatching(path, 0, 0) if err != nil { return errors.Wrap(err, "reseting file ownership") } - } + dest, err := os.Create(path) if err != nil { return errors.Wrap(err, "creating file") From 59f973d97776105f0c419744a068e8d07b03832c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=B6hl=2C=20Lukas?= Date: Mon, 27 Jun 2022 18:22:53 +0200 Subject: [PATCH 20/21] chore(test-script): remove go html coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Höhl, Lukas --- scripts/test.sh | 1 - 1 file changed, 1 deletion(-) diff --git a/scripts/test.sh b/scripts/test.sh index 72684a38ab..c097453c34 100755 --- a/scripts/test.sh +++ b/scripts/test.sh @@ -46,5 +46,4 @@ do fail=1 fi done - go tool cover -html=$DIR/../out/coverage.out exit $fail From 8813a6677139cc935a4a27b6025c38d0fae1a120 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=B6hl=2C=20Lukas?= Date: Tue, 12 Jul 2022 14:40:36 +0200 Subject: [PATCH 21/21] test(k8s): increase wait timeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Höhl, Lukas --- integration/k8s_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration/k8s_test.go b/integration/k8s_test.go index 935d5f9ef8..0765931620 100644 --- a/integration/k8s_test.go +++ b/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)