diff --git a/doc/metadata.txt b/doc/metadata.txt index 3f07ad9e5f90..686d37e5a941 100644 --- a/doc/metadata.txt +++ b/doc/metadata.txt @@ -226,12 +226,12 @@ See the Linux Kernel [shared subtree](https://www.kernel.org/doc/Documentation/f ``` ```{config:option} shift device-disk-device-conf -:condition: "container" :defaultdesc: "`false`" :required: "no" :shortdesc: "Whether to set up a UID/GID shifting overlay" :type: "bool" -If enabled, this option sets up a shifting overlay to translate the source UID/GID to match the container instance. +For containers, if enabled, this option sets up a shifting overlay to translate the source UID/GID to match the instance. +For virtual machines, the source UID/GID is passed through unchanged, even if the instance `raw.idmap` is set. ``` ```{config:option} size device-disk-device-conf diff --git a/lxd/device/device_utils_disk.go b/lxd/device/device_utils_disk.go index fe128800d52d..09fab0c8a63c 100644 --- a/lxd/device/device_utils_disk.go +++ b/lxd/device/device_utils_disk.go @@ -241,8 +241,59 @@ func diskAddRootUserNSEntry(idmaps []idmap.IdmapEntry, hostRootID int64) []idmap return idmaps } +// diskVMVirtiofsdResolveIDMaps returns explicit idmaps if provided, or the current namespace mappings otherwise. +func diskVMVirtiofsdResolveIDMaps(idmaps []idmap.IdmapEntry, currentIdmapSetFunc func() (*idmap.IdmapSet, error)) ([]idmap.IdmapEntry, error) { + if len(idmaps) > 0 { + return idmaps, nil + } + + if currentIdmapSetFunc == nil { + return nil, errors.New("Current idmap set function is nil") + } + + currentIdmapSet, err := currentIdmapSetFunc() + if err != nil { + return nil, fmt.Errorf("Failed getting current idmap set: %w", err) + } + + if currentIdmapSet == nil || len(currentIdmapSet.Idmap) == 0 { + return nil, errors.New("Current idmap set cannot be empty") + } + + // The current idmap set maps IDs in the current namespace (Nsid) to IDs in its parent (Hostid). + // virtiofsd runs in a child namespace of the current one, so build an identity map over each + // current Nsid range (Hostid = Nsid) to pass the host's ID range through unchanged. Reusing the + // parent Hostid values directly would be incorrect when LXD itself is nested. + effectiveIDMaps := make([]idmap.IdmapEntry, 0, len(currentIdmapSet.Idmap)) + hasUIDMap := false + hasGIDMap := false + for _, idmapEntry := range currentIdmapSet.Idmap { + effectiveIDMaps = append(effectiveIDMaps, idmap.IdmapEntry{ + Hostid: idmapEntry.Nsid, + Isuid: idmapEntry.Isuid, + Isgid: idmapEntry.Isgid, + Nsid: idmapEntry.Nsid, + Maprange: idmapEntry.Maprange, + }) + + if idmapEntry.Isuid { + hasUIDMap = true + } + + if idmapEntry.Isgid { + hasGIDMap = true + } + } + + if !hasUIDMap || !hasGIDMap { + return nil, errors.New("Current idmap set must contain both UID and GID mappings") + } + + return effectiveIDMaps, nil +} + // DiskVMVirtiofsdStart starts a new virtiofsd process with a socket present at the supplied path. -// If the idmaps slice is supplied then the proxy process is run inside a user namespace using the supplied maps. +// If the idmaps slice is empty, the current namespace mappings are used. // Returns UnsupportedError error if the host system or instance does not support virtiofsd, returns normal error // type if process cannot be started for other reasons. // Returns a revert function on success. @@ -334,10 +385,23 @@ func DiskVMVirtiofsdStart(inst instance.Instance, socketPath string, pidPath str return nil, err } - if len(idmaps) > 0 { - proc.SetUserns(&idmap.IdmapSet{Idmap: idmaps}) + // This is required because virtiofsd is split into two long-running processes. + // The child calls `pivot_root(2)`, which sandboxes both processes inside `sharePath`. + // However, it only pivots the working directory of the parent process when it starts as `/`. + // Normally this would only prevent unmounting LXD's working directory, which is OK. + // But when we run virtiofsd from a non-initial user namespace, all existing mounts are + // brought into the sandbox as a single unit (see `mount_namespaces(7)`). These remain + // alive even after unmounting them on the host (MNT_LOCKED), which can prevent LXD from + // deactivating instance volumes. + proc.Dir = "/" + + effectiveIDMaps, err := diskVMVirtiofsdResolveIDMaps(idmaps, idmap.CurrentIdmapSet) + if err != nil { + return nil, err } + proc.SetUserns(&idmap.IdmapSet{Idmap: effectiveIDMaps}, true) + err = proc.StartWithFiles(context.Background(), []*os.File{unixFile}) if err != nil { return nil, fmt.Errorf("Failed starting virtiofsd: %w", err) diff --git a/lxd/device/device_utils_disk_test.go b/lxd/device/device_utils_disk_test.go index d40a87722a56..d993f94c5faf 100644 --- a/lxd/device/device_utils_disk_test.go +++ b/lxd/device/device_utils_disk_test.go @@ -1,6 +1,7 @@ package device import ( + "errors" "testing" "github.com/stretchr/testify/assert" @@ -8,6 +9,119 @@ import ( "github.com/canonical/lxd/lxd/idmap" ) +func TestDiskVMVirtiofsdResolveIDMaps(t *testing.T) { + t.Run("Use supplied idmaps", func(t *testing.T) { + expected := []idmap.IdmapEntry{ + { + Hostid: 1000, + Isuid: true, + Nsid: 0, + Maprange: 1, + }, + { + Hostid: 1000, + Isgid: true, + Nsid: 0, + Maprange: 1, + }, + } + + currentIDMapSetFunc := func() (*idmap.IdmapSet, error) { + return nil, errors.New("should not be called") + } + + actual, err := diskVMVirtiofsdResolveIDMaps(expected, currentIDMapSetFunc) + assert.NoError(t, err) + assert.Equal(t, expected, actual) + }) + + t.Run("Fallback to current idmap set", func(t *testing.T) { + // The current idmap set maps current-namespace IDs (Nsid) to parent IDs (Hostid), here a + // non-identity mapping (0 -> 100000). The resolved maps must be an identity map over the + // current Nsid range (Hostid = Nsid) so nested LXD can start virtiofsd correctly. + currentIDMapSet := &idmap.IdmapSet{Idmap: []idmap.IdmapEntry{ + { + Hostid: 100000, + Isuid: true, + Nsid: 0, + Maprange: 65536, + }, + { + Hostid: 100000, + Isgid: true, + Nsid: 0, + Maprange: 65536, + }, + }} + + expected := []idmap.IdmapEntry{ + { + Hostid: 0, + Isuid: true, + Nsid: 0, + Maprange: 65536, + }, + { + Hostid: 0, + Isgid: true, + Nsid: 0, + Maprange: 65536, + }, + } + + currentIDMapSetFunc := func() (*idmap.IdmapSet, error) { + return currentIDMapSet, nil + } + + actual, err := diskVMVirtiofsdResolveIDMaps(nil, currentIDMapSetFunc) + assert.NoError(t, err) + assert.Equal(t, expected, actual) + }) + + t.Run("Fail if getting current idmap set", func(t *testing.T) { + currentIDMapSetFunc := func() (*idmap.IdmapSet, error) { + return nil, errors.New("boom") + } + + actual, err := diskVMVirtiofsdResolveIDMaps(nil, currentIDMapSetFunc) + assert.Nil(t, actual) + assert.EqualError(t, err, "Failed getting current idmap set: boom") + }) + + t.Run("Fail if current idmap set empty", func(t *testing.T) { + currentIDMapSetFunc := func() (*idmap.IdmapSet, error) { + return &idmap.IdmapSet{}, nil + } + + actual, err := diskVMVirtiofsdResolveIDMaps(nil, currentIDMapSetFunc) + assert.Nil(t, actual) + assert.EqualError(t, err, "Current idmap set cannot be empty") + }) + + t.Run("Fail if current idmap set has no gid map", func(t *testing.T) { + currentIDMapSetFunc := func() (*idmap.IdmapSet, error) { + return &idmap.IdmapSet{Idmap: []idmap.IdmapEntry{ + { + Hostid: 100000, + Isuid: true, + Nsid: 0, + Maprange: 65536, + }, + }}, nil + } + + actual, err := diskVMVirtiofsdResolveIDMaps(nil, currentIDMapSetFunc) + assert.Nil(t, actual) + assert.EqualError(t, err, "Current idmap set must contain both UID and GID mappings") + }) + + t.Run("Fail if current idmap set function is nil", func(t *testing.T) { + actual, err := diskVMVirtiofsdResolveIDMaps(nil, nil) + assert.Nil(t, actual) + assert.EqualError(t, err, "Current idmap set function is nil") + }) +} + func TestDiskAddRootUserNSEntry(t *testing.T) { // Check adds a combined uid/gid root entry to an empty list. var idmaps []idmap.IdmapEntry diff --git a/lxd/device/disk.go b/lxd/device/disk.go index 2959b10d7c8c..5b499a1cd485 100644 --- a/lxd/device/disk.go +++ b/lxd/device/disk.go @@ -239,12 +239,12 @@ func (d *disk) validateConfig(instConf instance.ConfigReader) error { // shortdesc: Whether to recursively mount the source path "recursive": validate.Optional(validate.IsBool), // lxdmeta:generate(entities=device-disk; group=device-conf; key=shift) - // If enabled, this option sets up a shifting overlay to translate the source UID/GID to match the container instance. + // For containers, if enabled, this option sets up a shifting overlay to translate the source UID/GID to match the instance. + // For virtual machines, the source UID/GID is passed through unchanged, even if the instance `raw.idmap` is set. // --- // type: bool // defaultdesc: `false` // required: no - // condition: container // shortdesc: Whether to set up a UID/GID shifting overlay "shift": validate.Optional(validate.IsBool), // lxdmeta:generate(entities=device-disk; group=device-conf; key=source) @@ -1173,6 +1173,16 @@ func (d *disk) startVM() (*deviceConfig.RunConfig, error) { mount.FSType = "iso9660" } + if shared.IsTrue(dbVolume.Config["security.shifted"]) { + // To be consistent with containers, we use the OwnerShift + // flag here even though it means something different for + // VMs. Containers use ID-mapped mounts because it makes + // UIDs and GIDs look the same on the host and in the + // VM. For VMs, the same effect is achieved by using an + // identity mapping for virtiofsd's nested user namespace. + mount.OwnerShift = deviceConfig.MountOwnerShiftDynamic + } + revertFunc, mountedPath, _, err := d.mountPoolVolume() if err != nil { return nil, diskSourceNotFoundError{msg: "Failed mounting volume", err: err} @@ -1233,6 +1243,10 @@ func (d *disk) startVM() (*deviceConfig.RunConfig, error) { return nil, errors.New(`Missing mount "path" setting`) } + if shared.IsTrue(d.config["shift"]) { + mount.OwnerShift = deviceConfig.MountOwnerShiftDynamic + } + // Mount the source in the instance devices directory. // This will ensure that if the exported directory configured as readonly that this // takes effect event if using virtio-fs (which doesn't support read only mode) by @@ -1250,9 +1264,20 @@ func (d *disk) startVM() (*deviceConfig.RunConfig, error) { mount.TargetPath = d.config["path"] mount.FSType = "virtiofs" - rawIDMaps, err := idmap.ParseRawIdmap(d.inst.ExpandedConfig()["raw.idmap"]) - if err != nil { - return nil, fmt.Errorf(`Failed parsing instance "raw.idmap": %w`, err) + // When security.shifted=true, the volume's files are owned by real users on the + // host (e.g. UID 0 not 1000000). For containers, the mount needs to be shifted to + // counteract the effect of entering a user namespace. But VMs don't use user + // namespaces, so we actually don't want to shift the virtiofsd process. + // + // Also, we should ignore raw.idmap for consistency with containers. If I create a + // file as user 1000 inside the container, the file on disk is owned by UID 1000. + // We don't care that container user is actually 1001000 in the root namespace. + var rawIDMaps []idmap.IdmapEntry + if mount.OwnerShift != deviceConfig.MountOwnerShiftDynamic { + rawIDMaps, err = idmap.ParseRawIdmap(d.inst.ExpandedConfig()["raw.idmap"]) + if err != nil { + return nil, fmt.Errorf(`Failed parsing instance "raw.idmap": %w`, err) + } } // If we are using restricted parent source path mode, or if a non-empty set of diff --git a/lxd/metadata/configuration.json b/lxd/metadata/configuration.json index ab0921478838..98d5c7adc1f0 100644 --- a/lxd/metadata/configuration.json +++ b/lxd/metadata/configuration.json @@ -254,9 +254,8 @@ }, { "shift": { - "condition": "container", "defaultdesc": "`false`", - "longdesc": "If enabled, this option sets up a shifting overlay to translate the source UID/GID to match the container instance.", + "longdesc": "For containers, if enabled, this option sets up a shifting overlay to translate the source UID/GID to match the instance.\nFor virtual machines, the source UID/GID is passed through unchanged, even if the instance `raw.idmap` is set.", "required": "no", "shortdesc": "Whether to set up a UID/GID shifting overlay", "type": "bool" diff --git a/lxd/subprocess/proc.go b/lxd/subprocess/proc.go index 86e51b0d7b8d..28d1c6c54580 100644 --- a/lxd/subprocess/proc.go +++ b/lxd/subprocess/proc.go @@ -38,6 +38,7 @@ type Process struct { UID uint32 `yaml:"uid"` GID uint32 `yaml:"gid"` SetGroups bool `yaml:"set_groups"` + Dir string `yaml:"dir"` StartTime int64 `yaml:"start_time"` SysProcAttr *syscall.SysProcAttr @@ -129,6 +130,7 @@ func (p *Process) start(ctx context.Context, fds []*os.File) error { cmd.Stdout = p.stdout cmd.Stderr = p.stderr cmd.Stdin = p.stdin + cmd.Dir = p.Dir cmd.SysProcAttr = p.SysProcAttr if cmd.SysProcAttr == nil { cmd.SysProcAttr = &syscall.SysProcAttr{} diff --git a/lxd/subprocess/proc_linux.go b/lxd/subprocess/proc_linux.go index 3ee22b726e69..448d81658fd0 100644 --- a/lxd/subprocess/proc_linux.go +++ b/lxd/subprocess/proc_linux.go @@ -9,14 +9,16 @@ import ( ) // SetUserns allows running inside of a user namespace. -func (p *Process) SetUserns(userns *idmap.IdmapSet) { +// If enableSetgroups is true, the process is allowed to use the setgroups syscall inside the user namespace. +func (p *Process) SetUserns(userns *idmap.IdmapSet, enableSetgroups bool) { p.SysProcAttr = &syscall.SysProcAttr{ Cloneflags: syscall.CLONE_NEWUSER, Credential: &syscall.Credential{ Uid: uint32(0), Gid: uint32(0), }, - UidMappings: userns.ToUidMappings(), - GidMappings: userns.ToGidMappings(), + UidMappings: userns.ToUidMappings(), + GidMappings: userns.ToGidMappings(), + GidMappingsEnableSetgroups: enableSetgroups, } } diff --git a/lxd/subprocess/proc_others.go b/lxd/subprocess/proc_others.go index b6d79fe0622b..f0355ba58806 100644 --- a/lxd/subprocess/proc_others.go +++ b/lxd/subprocess/proc_others.go @@ -7,6 +7,7 @@ import ( ) // SetUserns allows running inside of a user namespace. -func (p *Process) SetUserns(userns *idmap.IdmapSet) { +// If enableSetgroups is true, the process is allowed to use the setgroups syscall inside the user namespace. +func (p *Process) SetUserns(userns *idmap.IdmapSet, enableSetgroups bool) { return } diff --git a/test/suites/vm.sh b/test/suites/vm.sh index 189de0046990..134221ee0285 100644 --- a/test/suites/vm.sh +++ b/test/suites/vm.sh @@ -179,6 +179,40 @@ test_vm_pcie_bus() { lxc config device remove v1 v1block lxc storage volume delete "${pool}" v1block + sub_test "Check security.shifted volumes and shift=true directory shares are not remapped by virtiofsd in VMs" + # VMs do not use user namespaces, so files on a security.shifted volume or a shift=true + # directory share keep their real on-disk ownership inside the VM (matching containers). + # virtiofsd must ignore raw.idmap for both, otherwise the files below would appear owned by + # nobody instead of 123:456. Both disk types are attached to a single boot to exercise them + # together. + lxc config set v1 raw.idmap="both 1000000 0" + + lxc storage volume create "${pool}" v1shift --type=filesystem size=1MiB security.shifted=true + lxc config device add v1 v1shift disk source=v1shift pool="${pool}" path=/mnt + + mkdir -p "${TEST_DIR}/vm-shift-source" + touch "${TEST_DIR}/vm-shift-source/shifted-file" + chown 123:456 "${TEST_DIR}/vm-shift-source/shifted-file" + lxc config device add v1 v1shiftdir disk source="${TEST_DIR}/vm-shift-source" path=/mnt-dir shift=true + + lxc start v1 + waitInstanceReady v1 + + lxc exec v1 -- findmnt /mnt -t virtiofs + lxc exec v1 -- findmnt /mnt-dir -t virtiofs + volPath="${LXD_DIR}/storage-pools/${pool}/custom/default_v1shift" + touch "${volPath}/shifted-file" + chown 123:456 "${volPath}/shifted-file" + [ "$(lxc exec v1 -- stat /mnt/shifted-file -c '%u:%g')" = "123:456" ] + [ "$(lxc exec v1 -- stat /mnt-dir/shifted-file -c '%u:%g')" = "123:456" ] + + lxc stop -f v1 + lxc config device remove v1 v1shift + lxc config device remove v1 v1shiftdir + lxc storage volume delete "${pool}" v1shift + lxc config unset v1 raw.idmap + rm -rf "${TEST_DIR}/vm-shift-source" + lxc storage volume create "${pool}" v1dir --type=filesystem size=1MiB lxc start v1 lxc config device add v1 mydir disk source=v1dir pool="${pool}" path=/mnt