libcontainer: relax validation for absolute paths by thaJeztah · Pull Request #3004 · opencontainers/runc

thaJeztah · 2021-06-07T08:32:40Z

Commits 1f1e91b and 2192670 (#2917) added validation for mountpoints to be an absolute path, to match the OCI specs.

Unfortunately, the old behavior (accepting the path to be a relative path) has been around for a long time, and although "not according to the spec", various higher level runtimes rely on this behavior.

While higher level runtime have been updated to address this requirement, there will be a transition period before all runtimes are updated to carry these fixes.

This patch relaxes the validation, to generate a WARNING instead of failing, allowing runtimes to update (but allowing them to update runc to the current version, which includes security fixes).

We can remove this exception in a future patch release.

relates to #2928, containerd/containerd#5547

thaJeztah · 2021-06-07T08:33:56Z

@kolyshkin @AkihiroSuda PTAL. Let me know if this is acceptable for v1.0.0 (we can remove it in a patch release after that)

AkihiroSuda · 2021-06-07T08:40:01Z

AFAIK, only Docker Swarm-mode was known to have depended on the old behavior, and that was fixed in Docker 20.10.7: moby/moby@afbb127

Is there any other project that depends on the old behavior?

thaJeztah · 2021-06-07T08:46:18Z

BuildKit also needed changes, and (I think) for the swarmkit case at least, it's possible that existing services will break after updating (as they would have to be recreated for the new paths to be set); I'm wondering if there would be other situations where this could happen.

AkihiroSuda · 2021-06-07T08:51:25Z

BuildKit also needed changes,

Do we have an issue ticket?
BuildKit CI does not seem hitting the issue (for the master branch, at least) moby/buildkit#2143

thaJeztah · 2021-06-07T08:54:28Z

A fix was merged on master moby/buildkit#2123, and back ported to the v0.8 branch (but no tagged release exists with that fix)

thaJeztah · 2021-06-07T08:55:56Z

libcontainer/specconv/spec_linux.go

+		// Relax validation for backward compatibility
+		oldDest := m.Destination
+		var err error
+		m.Destination, err = filepath.Abs(m.Destination)


can probably simplify this to filepath.Join("/", ...) (like on BuildKit); let me do so

libcontainer/specconv/spec_linux.go

dims · 2021-06-08T01:49:50Z

I like this approach. thanks @thaJeztah

AkihiroSuda · 2021-06-08T09:08:22Z

libcontainer/specconv/spec_linux.go

+		// Relax validation for backward compatibility
+		// TODO (runc 1.2.0): replace a warning below with
+		// return nil, fmt.Errorf("mount destination %s not absolute", m.Destination)
+		logrus.Warnf("mount destination %s not absolute (this won't be supported past runc 1.2.0)", m.Destination)


We are still not sure how our post-v1 version scheme will look like.
So "late 2021" might be better than "1.2.0"?

Think we should make it more "generic"?

Suggested change

logrus.Warnf("mount destination %s not absolute (this won't be supported past runc 1.2.0)", m.Destination)

logrus.Warnf("mount destination %s not absolute (this will no longer be supported in a future release)", m.Destination)

Updated the description and TODO

libcontainer/configs/validate/validator.go

cyphar

You need to disable (t.Skip is fine, with a link to this PR or the linked issue) the TestValidateMounts test.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Commits 1f1e91b and 2192670 added validation for mountpoints to be an absolute path, to match the OCI specs. Unfortunately, the old behavior (accepting the path to be a relative path) has been around for a long time, and although "not according to the spec", various higher level runtimes rely on this behavior. While higher level runtime have been updated to address this requirement, there will be a transition period before all runtimes are updated to carry these fixes. This patch relaxes the validation, to generate a WARNING instead of failing, allowing runtimes to update (but allowing them to update runc to the current version, which includes security fixes). We can remove this exception in a future patch release. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2021-06-09T11:35:20Z

You need to disable (t.Skip is fine, with a link to this PR or the linked issue) the TestValidateMounts test.

Arf. My mistake; fixed!

kolyshkin

LGTM

cyphar

LGTM.

thaJeztah · 2021-06-10T15:30:07Z

Thanks! I opened #3020 to track removal of this.

This PR contains the following updates: | Package | Type | Update | Change | OpenSSF | |---|---|---|---|---| | [github.com/opencontainers/runc](https://github.com/opencontainers/runc) | require | patch | `v1.4.0` → `v1.4.1` | [![OpenSSF Scorecard](https://api.securityscorecards.dev/projects/github.com/opencontainers/runc/badge)](https://securityscorecards.dev/viewer/?uri=github.com/opencontainers/runc) | --- > ⚠️ **Warning** > > Some dependencies could not be looked up. Check the [Dependency Dashboard](issues/23) for more information. --- ### Release Notes <details> <summary>opencontainers/runc (github.com/opencontainers/runc)</summary> ### [`v1.4.1`](https://github.com/opencontainers/runc/blob/HEAD/CHANGELOG.md#100---2021-06-22) [Compare Source](opencontainers/runc@v1.4.0...v1.4.1) > A wizard is never late, nor is he early, he arrives precisely when he means > to. As runc follows Semantic Versioning, we will endeavour to not make any breaking changes without bumping the major version number of runc. However, it should be noted that Go API usage of runc's internal implementation (libcontainer) is *not* covered by this policy. ##### Removed - Removed libcontainer/configs.Device\* identifiers (deprecated since rc94, use libcontainer/devices). ([#2999](opencontainers/runc#2999)) - Removed libcontainer/system.RunningInUserNS function (deprecated since rc94, use libcontainer/userns). ([#2999](opencontainers/runc#2999)) ##### Deprecated - The usage of relative paths for mountpoints will now produce a warning (such configurations are outside of the spec, and in future runc will produce an error when given such configurations). ([#2917](opencontainers/runc#2917), [#3004](opencontainers/runc#3004)) ##### Fixed - cgroupv2: devices: rework the filter generation to produce consistent results with cgroupv1, and always clobber any existing eBPF program(s) to fix `runc update` and avoid leaking eBPF programs (resulting in errors when managing containers). ([#2951](opencontainers/runc#2951)) - cgroupv2: correctly convert "number of IOs" statistics in a cgroupv1-compatible way. ([#2965](opencontainers/runc#2965), [#2967](opencontainers/runc#2967), [#2968](opencontainers/runc#2968), [#2964](opencontainers/runc#2964)) - cgroupv2: support larger than 32-bit IO statistics on 32-bit architectures. - cgroupv2: wait for freeze to finish before returning from the freezing code, optimize the method for checking whether a cgroup is frozen. ([#2955](opencontainers/runc#2955)) - cgroups/systemd: fixed "retry on dbus disconnect" logic introduced in rc94 - cgroups/systemd: fixed returning "unit already exists" error from a systemd cgroup manager (regression in rc94). ([#2997](opencontainers/runc#2997), [#2996](opencontainers/runc#2996)) ##### Added - cgroupv2: support SkipDevices with systemd driver. ([#2958](opencontainers/runc#2958), [#3019](opencontainers/runc#3019)) - cgroup1: blkio: support BFQ weights. ([#3010](opencontainers/runc#3010)) - cgroupv2: set per-device io weights if BFQ IO scheduler is available. ([#3022](opencontainers/runc#3022)) ##### Changed - cgroup/systemd: return, not ignore, stop unit error from Destroy. ([#2946](opencontainers/runc#2946)) - Fix all golangci-lint failures. ([#2781](opencontainers/runc#2781), [#2962](opencontainers/runc#2962)) - Make `runc --version` output sane even when built with `go get` or otherwise outside of our build scripts. ([#2962](opencontainers/runc#2962)) - cgroups: set SkipDevices during runc update (so we don't modify cgroups at all during `runc update`). ([#2994](opencontainers/runc#2994))  [Unreleased]: opencontainers/runc@v1.3.0-rc.1...HEAD [1.3.0]: opencontainers/runc@v1.3.0-rc.2...v1.3.0 [1.2.0]: opencontainers/runc@v1.2.0-rc.1...v1.2.0 [1.1.0]: opencontainers/runc@v1.1.0-rc.1...v1.1.0 [1.0.0]: https://github.com/opencontainers/runc/releases/tag/v1.0.0  [Unreleased 1.0.z]: opencontainers/runc@v1.0.3...release-1.0 [1.0.3]: opencontainers/runc@v1.0.2...v1.0.3 [1.0.2]: opencontainers/runc@v1.0.1...v1.0.2 [1.0.1]: opencontainers/runc@v1.0.0...v1.0.1  [Unreleased 1.1.z]: opencontainers/runc@v1.1.15...release-1.1 [1.1.15]: opencontainers/runc@v1.1.14...v1.1.15 [1.1.14]: opencontainers/runc@v1.1.13...v1.1.14 [1.1.13]: opencontainers/runc@v1.1.12...v1.1.13 [1.1.12]: opencontainers/runc@v1.1.11...v1.1.12 [1.1.11]: opencontainers/runc@v1.1.10...v1.1.11 [1.1.10]: opencontainers/runc@v1.1.9...v1.1.10 [1.1.9]: opencontainers/runc@v1.1.8...v1.1.9 [1.1.8]: opencontainers/runc@v1.1.7...v1.1.8 [1.1.7]: opencontainers/runc@v1.1.6...v1.1.7 [1.1.6]: opencontainers/runc@v1.1.5...v1.1.6 [1.1.5]: opencontainers/runc@v1.1.4...v1.1.5 [1.1.4]: opencontainers/runc@v1.1.3...v1.1.4 [1.1.3]: opencontainers/runc@v1.1.2...v1.1.3 [1.1.2]: opencontainers/runc@v1.1.1...v1.1.2 [1.1.1]: opencontainers/runc@v1.1.0...v1.1.1 [1.1.0-rc.1]: opencontainers/runc@v1.0.0...v1.1.0-rc.1  [Unreleased 1.2.z]: opencontainers/runc@v1.2.9...release-1.2 [1.2.9]: opencontainers/runc@v1.2.8...v1.2.9 [1.2.8]: opencontainers/runc@v1.2.7...v1.2.8 [1.2.7]: opencontainers/runc@v1.2.6...v1.2.7 [1.2.6]: opencontainers/runc@v1.2.5...v1.2.6 [1.2.5]: opencontainers/runc@v1.2.4...v1.2.5 [1.2.4]: opencontainers/runc@v1.2.3...v1.2.4 [1.2.3]: opencontainers/runc@v1.2.2...v1.2.3 [1.2.2]: opencontainers/runc@v1.2.1...v1.2.2 [1.2.1]: opencontainers/runc@v1.2.0...v1.2.1 [1.2.0-rc.3]: opencontainers/runc@v1.2.0-rc.2...v1.2.0-rc.3 [1.2.0-rc.2]: opencontainers/runc@v1.2.0-rc.1...v1.2.0-rc.2 [1.2.0-rc.1]: opencontainers/runc@v1.1.0...v1.2.0-rc.1  [Unreleased 1.3.z]: opencontainers/runc@v1.3.4...release-1.3 [1.3.4]: opencontainers/runc@v1.3.3...v1.3.4 [1.3.3]: opencontainers/runc@v1.3.2...v1.3.3 [1.3.2]: opencontainers/runc@v1.3.1...v1.3.2 [1.3.1]: opencontainers/runc@v1.3.0...v1.3.1 [1.3.0]: opencontainers/runc@v1.3.0-rc.2...v1.3.0 [1.3.0-rc.2]: opencontainers/runc@v1.3.0-rc.1...v1.3.0-rc.2 [1.3.0-rc.1]: opencontainers/runc@v1.2.0...v1.3.0-rc.1  [Unreleased 1.4.z]: opencontainers/runc@v1.4.1...release-1.4 [1.4.1]: opencontainers/runc@v1.4.0...v1.4.1 [1.4.0]: opencontainers/runc@v1.4.0-rc.3...v1.4.0 [1.4.0-rc.3]: opencontainers/runc@v1.4.0-rc.2...v1.4.0-rc.3 [1.4.0-rc.2]: opencontainers/runc@v1.4.0-rc.1...v1.4.0-rc.2 [1.4.0-rc.1]: opencontainers/runc@v1.3.0...v1.4.0-rc.1  [Unreleased 1.5.z]: opencontainers/runc@v1.5.0-rc.1...release-1.5 [1.5.0-rc.1]: opencontainers/runc@v1.4.0...v1.5.0-rc.1 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At 12:00 AM through 04:59 AM and 10:00 PM through 11:59 PM, Monday through Friday ( * 0-4,22-23 * * 1-5 ), Only on Sunday and Saturday ( * * * * 0,6 ) (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Reviewed-on: https://altlinux.space/stapler/stplr/pulls/361 Co-authored-by: Renovate Bot <stapler-helper-bot@noreply.altlinux.space> Co-committed-by: Renovate Bot <stapler-helper-bot@noreply.altlinux.space>

thaJeztah force-pushed the relax_validation branch from dd349d9 to 82dc2ff Compare June 7, 2021 08:37

thaJeztah commented Jun 7, 2021

View reviewed changes

thaJeztah force-pushed the relax_validation branch from 82dc2ff to c5a5189 Compare June 7, 2021 08:57

thaJeztah commented Jun 7, 2021

View reviewed changes

libcontainer/specconv/spec_linux.go Outdated Show resolved Hide resolved

This was referenced Jun 7, 2021

"mount destination not absolute: unknown." Seems to be tied to Upgrade from Ubuntu 18.04 to Ubuntu 20.04 moby/moby#42480

Closed

VERSION: release runc 1.0.0 #2971

Merged

thaJeztah force-pushed the relax_validation branch 3 times, most recently from dbcf967 to 21aaece Compare June 7, 2021 22:34

AkihiroSuda reviewed Jun 8, 2021

View reviewed changes

libcontainer/configs/validate/validator.go Show resolved Hide resolved

AkihiroSuda added this to the 1.0.0 milestone Jun 8, 2021

AkihiroSuda added the impact/changelog label Jun 8, 2021

thaJeztah force-pushed the relax_validation branch from 21aaece to 5e163ae Compare June 8, 2021 14:57

cyphar requested changes Jun 9, 2021

View reviewed changes

thaJeztah added 2 commits June 9, 2021 13:15

configs/validator: move cgroup validation to the list of checks

dbb3541

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah force-pushed the relax_validation branch from 5e163ae to b31a934 Compare June 9, 2021 11:20

kolyshkin approved these changes Jun 9, 2021

View reviewed changes

cyphar approved these changes Jun 10, 2021

View reviewed changes

cyphar closed this in f454bb1 Jun 10, 2021

cyphar merged commit f454bb1 into opencontainers:master Jun 10, 2021

thaJeztah deleted the relax_validation branch June 10, 2021 15:27

thaJeztah mentioned this pull request Jun 10, 2021

v1.x.x: enforce absolute paths for mounts again #3020

Closed

sbesson mentioned this pull request Jun 16, 2021

Add .cache folder to .dockerignore for server, slave and web containers ome/devspace#181

Merged

cortiz mentioned this pull request Jun 22, 2021

[Docker] Docker images don't work due > The usage of relative paths for mountpoints will now produce a warning (librunc) craftercms/craftercms#4799

Closed

This was referenced Jul 21, 2021

container error after upgrade to containerd.io 1.4.6-1 containerd/containerd#5547

Closed

Swarm stack using short-syntax for config doesn't work moby/moby#42436

Closed

mmguero mentioned this pull request Feb 9, 2022

mount destination [/opt/zeek/share/zeek/site/intel] not absolute: unknown cisagov/Malcolm#189

Closed

cpuguy83 mentioned this pull request Jul 27, 2023

Handle relative paths in VOLUME directives moby/moby#46093

Open

	logrus.Warnf("mount destination %s not absolute (this won't be supported past runc 1.2.0)", m.Destination)
	logrus.Warnf("mount destination %s not absolute (this will no longer be supported in a future release)", m.Destination)

Conversation

thaJeztah commented Jun 7, 2021

Uh oh!

thaJeztah commented Jun 7, 2021

Uh oh!

AkihiroSuda commented Jun 7, 2021

Uh oh!

thaJeztah commented Jun 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkihiroSuda commented Jun 7, 2021

Uh oh!

thaJeztah commented Jun 7, 2021

Uh oh!

thaJeztah Jun 7, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dims commented Jun 8, 2021

Uh oh!

AkihiroSuda Jun 8, 2021

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jun 8, 2021

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jun 8, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cyphar left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jun 9, 2021

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

cyphar left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jun 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

thaJeztah commented Jun 7, 2021 •

edited

Loading