add option to resolve symlinks in WithLinuxDevice by ginglis13 · Pull Request #7523 · containerd/containerd

ginglis13 · 2022-10-13T19:34:01Z

This change adds withLinuxDevice to take an option followSymlinks. An option
WithLinuxDeviceFollowSymlinks will call this unexported option to
follow a symlink, which will resolve a symlink before calling
DeviceFromPath. WithLinuxDevice has been changed to call
withLinuxDevice without following symlinks.

Previously DeviceFromPath used Lstat to get information about a device file, but Lstat will not follow symlinks; stat will. The use case could be using a tool like udev which will create symlinks to devices and wishing to attach one of these devices to a container.

It looks like DeviceFromPath is coming from runc, which does use Lstat instead of Stat but I didn't find any discussions as to why not resolve symlinks: opencontainers/runc/libcontainer/devices/device_unix.go

Docker is resolving symlinks themselves before calling this function:
moby/moby/oci/devices_linux.go

which they did after some previous discussions in these issues/PRs:

Fixes #7006

Signed-off-by: Gavin Inglis [email protected]

k8s-ci-robot · 2022-10-13T19:34:10Z

Hi @ginglis13. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

kzys · 2022-10-13T19:42:58Z

/ok-to-test

ginglis13 · 2022-10-13T23:41:22Z

/cc @cpuguy83 @thaJeztah

saw you folks had some work on related issues in containerd and docker, PTAL

thaJeztah · 2022-10-14T00:04:55Z

As this is a change in behavior, and other project may be using the existing functions, I wonder if (for the containerd library code), it should be opt-in to follow symlinks (also see moby/moby#20684 (comment), which suggested at the time to have a user-facing option as well); this could be a separate (WithDevicesXXX) function, and an option added to getDevices() (even more because that non-exported function is shared between multiple exported functions. (thinking of, e.g., if that function would ever be used on untrusted paths, e.g. a path in the container's filesystem, that could lead to a malicious image controlling what device would be mounted).

Whatever decision we get to, at least this behavior should be included in the function documentation so that consumers of those functions are aware of it.

Happy to hear other's thoughts on that though.

ginglis13 · 2022-10-17T16:44:48Z

As this is a change in behavior, and other project may be using the existing functions, I wonder if (for the containerd library code), it should be opt-in to follow symlinks

... and an option added to getDevices() (even more because that non-exported function is shared between multiple exported functions. (thinking of, e.g., if that function would ever be used on untrusted paths, e.g. a path in the container's filesystem, that could lead to a malicious image controlling what device would be mounted).

Whatever decision we get to, at least this behavior should be included in the function documentation so that consumers of those functions are aware of it.

@thaJeztah Yea I agree that following device symlinks should be optional, I anticipated some security concerns, hence why I opened a PR for some feedback :) I'll rework this PR to provide an option WithDevicesSymlinked or similar so that following symlinks is opt-in

samuelkarp · 2022-10-23T02:28:58Z

+					if tc.expectError {
+						assert.Error(t, err)
+					} else {
+						require.NoError(t, err)


Why require here and not assert?

The thought here was if we expect an error, then assert and log that error. However, if we do not expect an error and encounter one, then fail fast with require. But I see that's mostly just assuming the happy path. Changed to have both assert for consistency.

samuelkarp · 2022-10-23T02:29:43Z

+			}
+			if len(tc.expectedLinuxDevices) != 0 {
+				require.NotNil(t, spec.Linux)
+				require.NotNil(t, spec.Linux.Devices)


You probably want require.Len here instead.

This change modifies WithLinuxDevice to take an option `followSymlink` and be unexported as `withLinuxDevice`. An option `WithLinuxDeviceFollowSymlinks` will call this unexported option to follow a symlink, which will resolve a symlink before calling `DeviceFromPath`. `WithLinuxDevice` has been changed to call `withLinuxDevice` without following symlinks. Signed-off-by: Gavin Inglis <[email protected]>

thaJeztah

LGTM

kzys · 2022-11-03T21:52:23Z

The tests are all green finally!

@samuelkarp Can you take a look again?

k8s-ci-robot added the needs-ok-to-test label Oct 13, 2022

kzys reviewed Oct 13, 2022

View reviewed changes

Comment thread oci/utils_unix_test.go Outdated

Comment thread oci/utils_unix_test.go Outdated

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Oct 13, 2022

ginglis13 force-pushed the symlink-device branch 2 times, most recently from 04056f6 to 748ac11 Compare October 13, 2022 21:20

k8s-ci-robot requested review from cpuguy83 and thaJeztah October 13, 2022 23:41

ginglis13 force-pushed the symlink-device branch from 748ac11 to 2d073a9 Compare October 20, 2022 21:26

ginglis13 changed the title ~~resolve symlinks to devices in DeviceFromPath~~ add option to resolve symlinks in WithLinuxDevice Oct 20, 2022

ginglis13 force-pushed the symlink-device branch from 2d073a9 to b2b8573 Compare October 20, 2022 21:30

kzys reviewed Oct 20, 2022

View reviewed changes

Comment thread oci/spec_opts.go Outdated

ginglis13 force-pushed the symlink-device branch from b2b8573 to f9e6126 Compare October 20, 2022 22:00

thaJeztah reviewed Oct 20, 2022

View reviewed changes

Comment thread oci/spec_opts_test.go Outdated

thaJeztah reviewed Oct 20, 2022

View reviewed changes

Comment thread oci/spec_opts.go Outdated

Comment thread oci/spec_opts.go Outdated

ginglis13 force-pushed the symlink-device branch from f9e6126 to a913017 Compare October 21, 2022 17:59

samuelkarp reviewed Oct 23, 2022

View reviewed changes

ginglis13 force-pushed the symlink-device branch 2 times, most recently from 7f6472e to 09d7507 Compare October 26, 2022 18:19

kzys approved these changes Oct 27, 2022

View reviewed changes

ginglis13 requested review from samuelkarp and thaJeztah and removed request for cpuguy83, samuelkarp and thaJeztah November 3, 2022 17:46

ginglis13 requested a review from samuelkarp November 3, 2022 17:47

ginglis13 force-pushed the symlink-device branch from 09d7507 to 81bbd9d Compare November 3, 2022 20:25

thaJeztah approved these changes Nov 3, 2022

View reviewed changes

kzys merged commit 1e200b1 into containerd:main Nov 15, 2022

Conversation

ginglis13 commented Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Oct 13, 2022

Uh oh!

Uh oh!

Uh oh!

kzys commented Oct 13, 2022

Uh oh!

ginglis13 commented Oct 13, 2022

Uh oh!

thaJeztah commented Oct 14, 2022

Uh oh!

ginglis13 commented Oct 17, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

samuelkarp Oct 23, 2022

Choose a reason for hiding this comment

Uh oh!

ginglis13 Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

samuelkarp Oct 23, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

kzys commented Nov 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ginglis13 commented Oct 13, 2022 •

edited

Loading