From 4e375d7a3a248360c10ea2c522cd3885a6fc809e Mon Sep 17 00:00:00 2001 From: Cesar Talledo Date: Sun, 21 Nov 2021 05:10:13 +0000 Subject: [PATCH] Improve uid shifting via chown. Sysbox performs uid-shifting using chown for host dirs that are mounted into certain special dirs in the container (e.g., /var/lib/docker, /var/lib/kubelet, etc.) Prior to this change, the uid shifting via chown worked well when the host dir was initially owned by root:root. In this case, when the container started, Sysbox would shift uids to match the container's root user (e.g., 165536:165536); later when the container stopped, Sysbox would revert the uids back to the original one (i.e, root:root). However, we've seen situations where the uid revert may not occur because the host dir gets detached from the host. In this case, a subsequent creation of a container where said host dir is again mounted into a special dir in the container, would result in Sysbox not performing or mishandling the uid shifting. This commit fixes this by ensuring that Sysbox always performs the uid shifting regardless of the original uid associated with the host dir being mounted into the container. Signed-off-by: Cesar Talledo --- idShiftUtils/idShiftUtils.go | 47 +++++++++--------------------------- mgr.go | 22 ++++++++--------- utils.go | 36 ++++++--------------------- 3 files changed, 30 insertions(+), 75 deletions(-) diff --git a/idShiftUtils/idShiftUtils.go b/idShiftUtils/idShiftUtils.go index 4a24ef3..ed1a07b 100644 --- a/idShiftUtils/idShiftUtils.go +++ b/idShiftUtils/idShiftUtils.go @@ -33,13 +33,6 @@ import ( mapset "github.com/deckarep/golang-set" ) -type OffsetType int - -const ( - OffsetAdd OffsetType = iota - OffsetSub -) - type aclType int const ( @@ -48,7 +41,7 @@ const ( ) // Shifts the ACL type user and group IDs by the given offset -func shiftAclType(aclT aclType, path string, uidOffset, gidOffset uint32, offsetDir OffsetType) error { +func shiftAclType(aclT aclType, path string, uidOffset, gidOffset int32) error { var facl aclLib.ACL var err error @@ -77,13 +70,8 @@ func shiftAclType(aclT aclType, path string, uidOffset, gidOffset uint32, offset continue } - if offsetDir == OffsetAdd { - uid = uid + uint64(uidOffset) - } else { - uid = uid - uint64(uidOffset) - } - - e.Qualifier = strconv.FormatUint(uid, 10) + targetUid := uint64(int32(uid) + uidOffset) + e.Qualifier = strconv.FormatUint(targetUid, 10) aclShifted = true } @@ -95,13 +83,8 @@ func shiftAclType(aclT aclType, path string, uidOffset, gidOffset uint32, offset continue } - if offsetDir == OffsetAdd { - gid = gid + uint64(gidOffset) - } else { - gid = gid - uint64(gidOffset) - } - - e.Qualifier = strconv.FormatUint(gid, 10) + targetGid := uint64(int32(gid) + gidOffset) + e.Qualifier = strconv.FormatUint(targetGid, 10) aclShifted = true } @@ -129,17 +112,17 @@ func shiftAclType(aclT aclType, path string, uidOffset, gidOffset uint32, offset } // Shifts the ACL user and group IDs by the given offset, both for access and default ACLs -func shiftAclIds(path string, isDir bool, uidOffset, gidOffset uint32, offsetDir OffsetType) error { +func shiftAclIds(path string, isDir bool, uidOffset, gidOffset int32) error { // Access list - err := shiftAclType(aclTypeAccess, path, uidOffset, gidOffset, offsetDir) + err := shiftAclType(aclTypeAccess, path, uidOffset, gidOffset) if err != nil { return err } // Default list (for directories only) if isDir { - err = shiftAclType(aclTypeDefault, path, uidOffset, gidOffset, offsetDir) + err = shiftAclType(aclTypeDefault, path, uidOffset, gidOffset) if err != nil { return err } @@ -150,13 +133,12 @@ func shiftAclIds(path string, isDir bool, uidOffset, gidOffset uint32, offsetDir // "Shifts" ownership of user and group IDs on the given directory and files and directories // below it by the given offset, using chown. -func ShiftIdsWithChown(baseDir string, uidOffset, gidOffset uint32, offsetDir OffsetType) error { +func ShiftIdsWithChown(baseDir string, uidOffset, gidOffset int32) error { hardLinks := []uint64{} err := godirwalk.Walk(baseDir, &godirwalk.Options{ Callback: func(path string, de *godirwalk.Dirent) error { - var targetUid, targetGid uint32 // When doing the chown, we don't follow symlinks as we want to change // the ownership of the symlinks themselves. We will chown the @@ -184,13 +166,8 @@ func ShiftIdsWithChown(baseDir string, uidOffset, gidOffset uint32, offsetDir Of hardLinks = append(hardLinks, st.Ino) } - if offsetDir == OffsetAdd { - targetUid = st.Uid + uidOffset - targetGid = st.Gid + gidOffset - } else { - targetUid = st.Uid - uidOffset - targetGid = st.Gid - gidOffset - } + targetUid := int32(st.Uid) + uidOffset + targetGid := int32(st.Gid) + gidOffset logrus.Debugf("chown %s from %d:%d to %d:%d", path, st.Uid, st.Gid, targetUid, targetGid) @@ -203,7 +180,7 @@ func ShiftIdsWithChown(baseDir string, uidOffset, gidOffset uint32, offsetDir Of // the Linux access control list (ACL) for the file if fi.Mode()&os.ModeSymlink == 0 { - if err := shiftAclIds(path, fi.IsDir(), uidOffset, gidOffset, offsetDir); err != nil { + if err := shiftAclIds(path, fi.IsDir(), uidOffset, gidOffset); err != nil { return fmt.Errorf("failed to shift ACL for %s: %s", path, err) } } diff --git a/mgr.go b/mgr.go index a3b0134..9110e08 100644 --- a/mgr.go +++ b/mgr.go @@ -476,15 +476,13 @@ func (mgr *SysboxMgr) unregister(id string) error { // revert mount prep actions for _, revInfo := range info.mntPrepRev { if revInfo.uidShifted { - logrus.Infof("reverting uid-shift on %s for %s", revInfo.path, formatter.ContainerID{id}) - // revInfo.targetUid is guaranteed to be higher than revInfo.origUid - // (we checked in prepMounts()) + uidOffset := int32(revInfo.origUid) - int32(revInfo.targetUid) + gidOffset := int32(revInfo.origGid) - int32(revInfo.targetGid) - uidOffset := revInfo.targetUid - revInfo.origUid - gidOffset := revInfo.targetGid - revInfo.origGid + logrus.Infof("reverting uid-shift on %s for %s (%d -> %d)", revInfo.path, formatter.ContainerID{id}, revInfo.targetUid, revInfo.origUid) - if err = idShiftUtils.ShiftIdsWithChown(revInfo.path, uidOffset, gidOffset, idShiftUtils.OffsetSub); err != nil { + if err = idShiftUtils.ShiftIdsWithChown(revInfo.path, uidOffset, gidOffset); err != nil { logrus.Warnf("failed to revert uid-shift of mount source at %s: %s", revInfo.path, err) } @@ -837,15 +835,15 @@ func (mgr *SysboxMgr) prepMounts(id string, uid, gid uint32, prepList []ipcLib.M } if needUidShift { - logrus.Infof("shifting uids at %s for %s", src, formatter.ContainerID{id}) - // uid is guaranteed to be higher than origUid (we checked in - // mntSrcUidShiftNeeded()) + // Offset may be positive or negative + uidOffset := int32(uid) - int32(origUid) + gidOffset := int32(gid) - int32(origGid) - uidOffset := uid - origUid - gidOffset := gid - origGid + logrus.Infof("shifting uids at %s for %s (%d -> %d)", src, formatter.ContainerID{id}, origUid, uid) - if err = idShiftUtils.ShiftIdsWithChown(src, uidOffset, gidOffset, idShiftUtils.OffsetAdd); err != nil { + // TODO: for kernel >= 5.12, we need to do this using the new ID-mapped mount feature (faster, more reliable). + if err = idShiftUtils.ShiftIdsWithChown(src, uidOffset, gidOffset); err != nil { return fmt.Errorf("failed to shift uids via chown for mount source at %s: %s", src, err) } diff --git a/utils.go b/utils.go index 0aff63b..5556751 100644 --- a/utils.go +++ b/utils.go @@ -723,28 +723,13 @@ func mntSrcUidShiftNeeded(mntSrc string, uid, gid uint32) (bool, uint32, uint32, mntSrcUid = st.Uid mntSrcGid = st.Gid - // If the host uid assigned to the container is higher than the uid - // of the dir being mounted into the container, then we perform uid + // If the host uid assigned to the container's root user differs from the + // uid of the dir being mounted into the container, then we perform uid // shifting. Same for gid. - if uid > mntSrcUid && gid > mntSrcGid { + if uid != mntSrcUid && gid != mntSrcGid { return true, mntSrcUid, mntSrcGid, nil } - // If the host uid assigned to the container is lower than the uid of the dir - // being mounted, we skip the uid shift. This is in fact an anomalous case - // because in general the host dir being mounted into the container should - // have host range uids (e.g., 0->65535) and these are usually lower than the - // uids assigned to the container (which come from the subuid ranges in - // /etc/subuid). - if uid < mntSrcUid || gid < mntSrcGid { - - logrus.Infof("skipping uid shift on %s because its uid:gid (%d:%d) is "+ - "> the container's assigned uid:gid (%d:%d)", - mntSrc, mntSrcUid, mntSrcGid, uid, gid) - - return false, mntSrcUid, mntSrcGid, nil - } - // If the mount dir has same ownership as the container, check the subdirs // before we make a determination on whether ownership shifting will be // required. @@ -755,21 +740,16 @@ func mntSrcUidShiftNeeded(mntSrc string, uid, gid uint32) (bool, uint32, uint32, return false, 0, 0, err } - needChown := true + numNeedChown := 0 for _, fi := range dirFis { st, _ := fi.Sys().(*syscall.Stat_t) - - if uid <= st.Uid && gid <= st.Gid { - - logrus.Infof("skipping uid shift on %s because subdir %s has uid:gid (%d:%d) which is "+ - ">= the container's assigned uid:gid (%d:%d)", - mntSrc, fi.Name(), st.Uid, st.Gid, uid, gid) - - needChown = false - break + if uid != st.Uid || gid != st.Gid { + numNeedChown += 1 } } + needChown := (numNeedChown == len(dirFis)) + return needChown, mntSrcUid, mntSrcGid, nil }