From 7f70ff96727761092c2b07a53b65d1cc5c137c6c Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 8 Dec 2021 14:48:04 +0100 Subject: [PATCH 1/2] oci.getDevices(): move "non-dir, non '/dev/console'" case into switch This makes it slightly clearer that these are all part of the same logic. Signed-off-by: Sebastiaan van Stijn --- oci/utils_unix.go | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/oci/utils_unix.go b/oci/utils_unix.go index 7d42901e3..7063184ea 100644 --- a/oci/utils_unix.go +++ b/oci/utils_unix.go @@ -81,24 +81,25 @@ func getDevices(path, containerPath string) ([]specs.LinuxDevice, error) { } case f.Name() == "console": continue - } - device, err := DeviceFromPath(filepath.Join(path, f.Name())) - if err != nil { - if err == ErrNotADevice { + default: + device, err := DeviceFromPath(filepath.Join(path, f.Name())) + if err != nil { + if err == ErrNotADevice { + continue + } + if os.IsNotExist(err) { + continue + } + return nil, err + } + if device.Type == fifoDevice { continue } - if os.IsNotExist(err) { - continue + if containerPath != "" { + device.Path = filepath.Join(containerPath, filepath.Base(f.Name())) } - return nil, err + out = append(out, *device) } - if device.Type == fifoDevice { - continue - } - if containerPath != "" { - device.Path = filepath.Join(containerPath, filepath.Base(f.Name())) - } - out = append(out, *device) } return out, nil } From b7f673790f63168960a438abf4f223f1f7b8e54c Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 1 Dec 2021 12:27:55 +0100 Subject: [PATCH 2/2] OCI: Mount (accessible) host devices in privileged rootless containers Allow rootless containers with privileged to mount devices that are accessible (ignore permission errors in rootless mode). This patch updates oci.getDevices() to ignore access denied errors on sub- directories and files within the given path if the container is running with userns enabled. Note that these errors are _only_ ignored on paths _under_ the specified path, and not the path itself, so if `HostDevices()` is used, and `/dev` itself is not accessible, or `WithDevices()` is used to specify a device that is not accessible, an error is still produced. Tests were added, which includes a temporary workaround for compatibility with Go 1.16 (we could decide to skip these tests on Go 1.16 instead). To verify the patch in a container: docker run --rm -v $(pwd):/go/src/github.com/containerd/containerd -w /go/src/github.com/containerd/containerd golang:1.17 sh -c 'go test -v -run TestHostDevices ./oci' === RUN TestHostDevicesOSReadDirFailure --- PASS: TestHostDevicesOSReadDirFailure (0.00s) === RUN TestHostDevicesOSReadDirFailureInUserNS --- PASS: TestHostDevicesOSReadDirFailureInUserNS (0.00s) === RUN TestHostDevicesDeviceFromPathFailure --- PASS: TestHostDevicesDeviceFromPathFailure (0.00s) === RUN TestHostDevicesDeviceFromPathFailureInUserNS --- PASS: TestHostDevicesDeviceFromPathFailureInUserNS (0.00s) === RUN TestHostDevicesAllValid --- PASS: TestHostDevicesAllValid (0.00s) PASS ok github.com/containerd/containerd/oci 0.006s Signed-off-by: Sebastiaan van Stijn --- oci/utils_unix.go | 28 +++++++- oci/utils_unix_go116_test.go | 55 ++++++++++++++++ oci/utils_unix_go117_test.go | 24 +++++++ oci/utils_unix_test.go | 123 ++++++++++++++++++++++++++++++++++- 4 files changed, 228 insertions(+), 2 deletions(-) create mode 100644 oci/utils_unix_go116_test.go create mode 100644 oci/utils_unix_go117_test.go diff --git a/oci/utils_unix.go b/oci/utils_unix.go index 7063184ea..f43d894c3 100644 --- a/oci/utils_unix.go +++ b/oci/utils_unix.go @@ -23,6 +23,7 @@ import ( "os" "path/filepath" + "github.com/containerd/containerd/pkg/userns" specs "github.com/opencontainers/runtime-spec/specs-go" "github.com/pkg/errors" "golang.org/x/sys/unix" @@ -31,6 +32,13 @@ import ( // ErrNotADevice denotes that a file is not a valid linux device. var ErrNotADevice = errors.New("not a device node") +// Testing dependencies +var ( + osReadDir = os.ReadDir + usernsRunningInUserNS = userns.RunningInUserNS + overrideDeviceFromPath func(path string) error +) + // HostDevices returns all devices that can be found under /dev directory. func HostDevices() ([]specs.LinuxDevice, error) { return getDevices("/dev", "") @@ -53,7 +61,7 @@ func getDevices(path, containerPath string) ([]specs.LinuxDevice, error) { return []specs.LinuxDevice{*dev}, nil } - files, err := os.ReadDir(path) + files, err := osReadDir(path) if err != nil { return nil, err } @@ -73,6 +81,12 @@ func getDevices(path, containerPath string) ([]specs.LinuxDevice, error) { } sub, err := getDevices(filepath.Join(path, f.Name()), cp) if err != nil { + if errors.Is(err, os.ErrPermission) && usernsRunningInUserNS() { + // ignore the "permission denied" error if running in userns. + // This allows rootless containers to use devices that are + // accessible, ignoring devices / subdirectories that are not. + continue + } return nil, err } @@ -90,6 +104,12 @@ func getDevices(path, containerPath string) ([]specs.LinuxDevice, error) { if os.IsNotExist(err) { continue } + if errors.Is(err, os.ErrPermission) && usernsRunningInUserNS() { + // ignore the "permission denied" error if running in userns. + // This allows rootless containers to use devices that are + // accessible, ignoring devices that are not. + continue + } return nil, err } if device.Type == fifoDevice { @@ -115,6 +135,12 @@ const ( // DeviceFromPath takes the path to a device to look up the information about a // linux device and returns that information as a LinuxDevice struct. func DeviceFromPath(path string) (*specs.LinuxDevice, error) { + if overrideDeviceFromPath != nil { + if err := overrideDeviceFromPath(path); err != nil { + return nil, err + } + } + var stat unix.Stat_t if err := unix.Lstat(path, &stat); err != nil { return nil, err diff --git a/oci/utils_unix_go116_test.go b/oci/utils_unix_go116_test.go new file mode 100644 index 000000000..b1fa01b19 --- /dev/null +++ b/oci/utils_unix_go116_test.go @@ -0,0 +1,55 @@ +//go:build !go1.17 && !windows && !darwin +// +build !go1.17,!windows,!darwin + +/* + Copyright The containerd Authors. + + 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 oci + +import "io/fs" + +// The following code is adapted from go1.17.1/src/io/fs/readdir.go +// to compensate for the lack of fs.FileInfoToDirEntry in Go 1.16. + +// dirInfo is a DirEntry based on a FileInfo. +type dirInfo struct { + fileInfo fs.FileInfo +} + +func (di dirInfo) IsDir() bool { + return di.fileInfo.IsDir() +} + +func (di dirInfo) Type() fs.FileMode { + return di.fileInfo.Mode().Type() +} + +func (di dirInfo) Info() (fs.FileInfo, error) { + return di.fileInfo, nil +} + +func (di dirInfo) Name() string { + return di.fileInfo.Name() +} + +// fileInfoToDirEntry returns a DirEntry that returns information from info. +// If info is nil, FileInfoToDirEntry returns nil. +func fileInfoToDirEntry(info fs.FileInfo) fs.DirEntry { + if info == nil { + return nil + } + return dirInfo{fileInfo: info} +} diff --git a/oci/utils_unix_go117_test.go b/oci/utils_unix_go117_test.go new file mode 100644 index 000000000..20ef980ce --- /dev/null +++ b/oci/utils_unix_go117_test.go @@ -0,0 +1,24 @@ +//go:build go1.17 && !windows && !darwin +// +build go1.17,!windows,!darwin + +/* + Copyright The containerd Authors. + + 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 oci + +import "io/fs" + +var fileInfoToDirEntry = fs.FileInfoToDirEntry diff --git a/oci/utils_unix_test.go b/oci/utils_unix_test.go index 7ec324562..9f2c8d05e 100644 --- a/oci/utils_unix_test.go +++ b/oci/utils_unix_test.go @@ -19,7 +19,128 @@ package oci -import "testing" +import ( + "errors" + "fmt" + "os" + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/containerd/containerd/pkg/userns" +) + +func cleanupTest() { + overrideDeviceFromPath = nil + osReadDir = os.ReadDir + usernsRunningInUserNS = userns.RunningInUserNS +} + +// Based on test from runc: +// https://github.com/opencontainers/runc/blob/v1.0.0/libcontainer/devices/device_unix_test.go#L34-L47 +func TestHostDevicesOSReadDirFailure(t *testing.T) { + testError := fmt.Errorf("test error: %w", os.ErrPermission) + + // Override os.ReadDir to inject error. + osReadDir = func(dirname string) ([]os.DirEntry, error) { + return nil, testError + } + + // Override userns.RunningInUserNS to ensure not running in user namespace. + usernsRunningInUserNS = func() bool { + return false + } + defer cleanupTest() + + _, err := HostDevices() + if !errors.Is(err, testError) { + t.Fatalf("Unexpected error %v, expected %v", err, testError) + } +} + +// Based on test from runc: +// https://github.com/opencontainers/runc/blob/v1.0.0/libcontainer/devices/device_unix_test.go#L34-L47 +func TestHostDevicesOSReadDirFailureInUserNS(t *testing.T) { + testError := fmt.Errorf("test error: %w", os.ErrPermission) + + // Override os.ReadDir to inject error. + osReadDir = func(dirname string) ([]os.DirEntry, error) { + if dirname == "/dev" { + fi, err := os.Lstat("/dev/null") + if err != nil { + t.Fatalf("Unexpected error %v", err) + } + + return []os.DirEntry{fileInfoToDirEntry(fi)}, nil + } + return nil, testError + } + // Override userns.RunningInUserNS to ensure running in user namespace. + usernsRunningInUserNS = func() bool { + return true + } + defer cleanupTest() + + _, err := HostDevices() + if !errors.Is(err, nil) { + t.Fatalf("Unexpected error %v, expected %v", err, nil) + } +} + +// Based on test from runc: +// https://github.com/opencontainers/runc/blob/v1.0.0/libcontainer/devices/device_unix_test.go#L49-L74 +func TestHostDevicesDeviceFromPathFailure(t *testing.T) { + testError := fmt.Errorf("test error: %w", os.ErrPermission) + + // Override DeviceFromPath to produce an os.ErrPermission on /dev/null. + overrideDeviceFromPath = func(path string) error { + if path == "/dev/null" { + return testError + } + return nil + } + + // Override userns.RunningInUserNS to ensure not running in user namespace. + usernsRunningInUserNS = func() bool { + return false + } + defer cleanupTest() + + d, err := HostDevices() + if !errors.Is(err, testError) { + t.Fatalf("Unexpected error %v, expected %v", err, testError) + } + + assert.Equal(t, 0, len(d)) +} + +// Based on test from runc: +// https://github.com/opencontainers/runc/blob/v1.0.0/libcontainer/devices/device_unix_test.go#L49-L74 +func TestHostDevicesDeviceFromPathFailureInUserNS(t *testing.T) { + testError := fmt.Errorf("test error: %w", os.ErrPermission) + + // Override DeviceFromPath to produce an os.ErrPermission on all devices, + // except for /dev/null. + overrideDeviceFromPath = func(path string) error { + if path == "/dev/null" { + return nil + } + return testError + } + + // Override userns.RunningInUserNS to ensure running in user namespace. + usernsRunningInUserNS = func() bool { + return true + } + defer cleanupTest() + + d, err := HostDevices() + if !errors.Is(err, nil) { + t.Fatalf("Unexpected error %v, expected %v", err, nil) + } + assert.Equal(t, 1, len(d)) + assert.Equal(t, d[0].Path, "/dev/null") +} func TestHostDevicesAllValid(t *testing.T) { devices, err := HostDevices()