Add support for MaskPaths and ReadonlyPaths #320

mlaventure · 2016-02-04T16:55:05Z

It is currently not possible for a user to specify libcontainer MaskPaths and ReadonlyPaths fields content through the bundle json configuration.

They should be added to the spec.

The text was updated successfully, but these errors were encountered:

jessfraz · 2016-02-04T17:00:12Z

+1 ;)

On Thu, Feb 4, 2016 at 8:55 AM, Kenfe-Mickaël Laventure <
[email protected]> wrote:

It is currently not possible for a user to specify libcontainer MaskPaths
https://github.com/opencontainers/runc/blob/d66c9632bfc945547f7df4390bff35681023a24b/libcontainer/configs/config.go#L157
and ReadonlyPaths
https://github.com/opencontainers/runc/blob/d66c9632bfc945547f7df4390bff35681023a24b/libcontainer/configs/config.go#L161
fields content through the bundle json configuration.

They should be added to the spec.

—
Reply to this email directly or view it on GitHub
#320.

Jessie Frazelle
4096R / D4C4 DD60 0D66 F65A 8EFC 511E 18F3 685C 0022 BFF3
pgp.mit.edu http://pgp.mit.edu/pks/lookup?op=get&search=0x18F3685C0022BFF3

jessfraz · 2016-02-04T17:01:02Z

although readonly could probably be done by using mounts

On Thu, Feb 4, 2016 at 8:59 AM, Jessica Frazelle [email protected] wrote:

+1 ;)

On Thu, Feb 4, 2016 at 8:55 AM, Kenfe-Mickaël Laventure <
[email protected]> wrote:

It is currently not possible for a user to specify libcontainer MaskPaths
https://github.com/opencontainers/runc/blob/d66c9632bfc945547f7df4390bff35681023a24b/libcontainer/configs/config.go#L157
and ReadonlyPaths
https://github.com/opencontainers/runc/blob/d66c9632bfc945547f7df4390bff35681023a24b/libcontainer/configs/config.go#L161
fields content through the bundle json configuration.

They should be added to the spec.

—
Reply to this email directly or view it on GitHub
#320.

Jessie Frazelle
4096R / D4C4 DD60 0D66 F65A 8EFC 511E 18F3 685C 0022 BFF3
pgp.mit.edu
http://pgp.mit.edu/pks/lookup?op=get&search=0x18F3685C0022BFF3

Jessie Frazelle
4096R / D4C4 DD60 0D66 F65A 8EFC 511E 18F3 685C 0022 BFF3
pgp.mit.edu http://pgp.mit.edu/pks/lookup?op=get&search=0x18F3685C0022BFF3

mlaventure · 2016-02-04T17:08:11Z

MaskPaths too can be done with mount by adding a bind mount of the paths to /dev/null, but that's a technicality (and I have no idea what mechanism would be needed on Windows).

wking · 2016-02-04T17:33:22Z

On Thu, Feb 04, 2016 at 08:55:08AM -0800, Kenfe-Mickaël Laventure wrote:

They should be added to the spec.

There's some previous discussion in #186.

tonistiigi · 2016-02-25T17:28:21Z

although readonly could probably be done by using mounts
MaskPaths too can be done with mount by adding a bind mount of the paths to /dev/null

In runc this isn't possible atm, because you are not allowed to create a mount inside /proc

wking · 2016-02-25T23:37:46Z

On Thu, Feb 25, 2016 at 09:28:24AM -0800, Tõnis Tiigi wrote:

although readonly could probably be done by using mounts MaskPaths
too can be done with mount by adding a bind mount of the paths to
/dev/null

In runc this isn't possible atm, because you are not allowed to
create a mount inside /proc

Really? It looks like bind-mounting /dev/null over /proc entries is
allowed. On a vanilla 4.3 kernel:
—
$ unshare -Umprf --mount-proc sh -c 'mount --bind /dev/null /proc/1/mounts && file /proc/1/mounts'
/proc/1/mounts: character special (1/3)

And I don't see anything about restricting mounts in a quick skim
through 1.

tonistiigi · 2016-02-26T02:39:22Z

@wking This is not allowed in runc https://github.com/opencontainers/runc/blob/4951f5821b0840d42b83e76d8ba1e881858db9eb/libcontainer/rootfs_linux.go#L310

wking · 2016-02-26T05:06:19Z

On Thu, Feb 25, 2016 at 06:39:23PM -0800, Tõnis Tiigi wrote:

@wking This is not allowed in runc
https://github.com/opencontainers/runc/blob/4951f5821b0840d42b83e76d8ba1e881858db9eb/libcontainer/rootfs_linux.go#L310

Ah, got it. I'm skeptical about “we need to work around restrictions
imposed by a runtime” as motivation for spec changes. If
bind-mounting /dev/null over (or remounting read-only) generic paths
(including /proc and children) is the goal, then softening that runC
restriction to allow that behavior via mounts 1 seems simpler than
adding separate fields (MaskPaths and ReadonlyPaths) to work around an
overly-strict check.

hqhq · 2016-02-26T07:00:18Z

Yes, restriction in runC can be softened, and we already do that for fuse. So it's both doable for specs and implementations, I tend to keep this out of specs for now.

crosbymichael · 2016-02-27T00:25:34Z

We will just have this list in runc for now. You cannot specify these things as mounts because of the order of mounts and how devices are created.

crosbymichael · 2016-03-30T21:04:42Z

We are hitting this issue over and over again. Without this in the spec you are splitting things across runtime and spec settings. It would be much cleaner to have these two fields in the spec or else we need some type of --no-mask-proc flag on the runtime to achieve this functionality.

mlaventure · 2016-03-30T21:40:20Z

For my 2 cents, it doesn't make sense to have so many behavior specific flags in runC especially if the goal is to be able to get the same behavior just by shipping a bundle.

If to run my container correctly I have to do more than runc start id . there's something wrong with the specs.

Or to sum up: I'm for putting this in the specs and not adding it as a flag to the runtime

wking · 2016-03-30T21:54:10Z

On Wed, Mar 30, 2016 at 02:04:45PM -0700, Michael Crosby wrote:

We are hitting this issue over and over again. Without this in the
spec you are splitting things across runtime and spec settings. It
would be much cleaner to have these two fields in the spec…

Can you unpack that a bit? As I understand it, folks who want this
can setup bind-mounts over anything they want masked or read-only 1.
runC had some code to block that for some paths 2, but those checks
could be softened 3. There was a separate issue with the order of
config-specified mounts and runtime-initiated mounts 4, but the
solution to that seems to be “setup the config-specified mounts after
setting up the runtime-initiated mounts” 5 or “let the
config-specified mounts setup whatever they want, and have the runtime
not clobber a spec-required mount that the config has already
supplied”.

wking · 2016-03-30T22:08:45Z

On Wed, Mar 30, 2016 at 02:40:22PM -0700, Kenfe-Mickaël Laventure wrote:

For my 2 cents, it doesn't make sense to have so many behavior
specific flags in runC especially if the goal is to be able to get
the same behavior just by shipping a bundle.

And I'm +1 to this. I'm in favor of putting these settings in the
spec if we can't support their use-cases via existing spec settings.
I'm just not clear on if/why the existing spec settings are
insufficient.

crosbymichael · 2016-03-31T20:42:23Z

@vbatts @mrunalp what do you think?

vbatts · 2016-03-31T22:15:54Z

Not really opinionated on it. It seems like something that could be accomplished with hooks / between create and start

crosbymichael · 2016-03-31T22:23:49Z

I don't think hooks will work for this. These type of things need to be supported by the runtime because proc is so insecure without it but it also needs to be disabled for ppl who want to not have that

mrunalp · 2016-03-31T22:30:06Z

@crosbymichael I agree that hooks aren't the right way to handle this. I am okay either way on whether we add it to the spec or modify runc.

wking · 2016-03-31T22:33:09Z

On Thu, Mar 31, 2016 at 03:23:51PM -0700, Michael Crosby wrote:

I don't think hooks will work for this.

Why not?

These type of things need to be supported by the runtime because
proc is so insecure without it but it also needs to be disabled for
ppl who want to not have that

Making it possible to masking or make read-only parts of /proc makes
sense, but I'm still not clear on why these spec fields are the best
way to do that. Both mount entries in the config.json and hooks
(either in config.json or in higher-level tooling after a create/start
split) seem sufficient. The mount entries aren't going to be
particularly verbose 1, regardless of where you set them up.

crosbymichael · 2016-03-31T22:34:40Z

@wking mounts wont work because it would be bind mounting the hosts dev/null over the files and that will cause fds to leak into the container breaking other things like c/r.

this is a big security feature that should be a first class option and not hidden to users

mrunalp · 2016-03-31T22:41:01Z

@crosbymichael Yeah, that's a good point. We can't just bind mount from outside. Makes the case for having a field stronger.

wking · 2016-03-31T22:41:02Z

On Thu, Mar 31, 2016 at 03:34:42PM -0700, Michael Crosby wrote:

@wking mounts wont work because it would be bind mounting the hosts
dev/null over the files and that will cause fds to leak into the
container breaking other things like c/r.

So bind-mount the container's /dev/null (e.g. in a hook, or by
exposing the pivot explicitly in the mount array 1).

Fixes opencontainers#320 This adds the maskedPaths and readonlyPaths fields to the spec so that proper masking and setting of files in /proc can be configured. Signed-off-by: Michael Crosby <[email protected]>

crosbymichael closed this as completed Feb 27, 2016

crosbymichael reopened this Mar 30, 2016

thaJeztah mentioned this issue Mar 31, 2016

[1.11-rc2] Read-only /proc/sys breaks compatibility moby/moby#21649

Closed

crosbymichael mentioned this issue Apr 1, 2016

Add masked and readonly paths #364

Merged

mrunalp closed this as completed in #364 Apr 1, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for MaskPaths and ReadonlyPaths #320

Add support for MaskPaths and ReadonlyPaths #320

mlaventure commented Feb 4, 2016

jessfraz commented Feb 4, 2016

jessfraz commented Feb 4, 2016

mlaventure commented Feb 4, 2016

wking commented Feb 4, 2016

tonistiigi commented Feb 25, 2016

wking commented Feb 25, 2016

tonistiigi commented Feb 26, 2016

wking commented Feb 26, 2016

hqhq commented Feb 26, 2016

crosbymichael commented Feb 27, 2016

crosbymichael commented Mar 30, 2016

mlaventure commented Mar 30, 2016

wking commented Mar 30, 2016

wking commented Mar 30, 2016

crosbymichael commented Mar 31, 2016

vbatts commented Mar 31, 2016

crosbymichael commented Mar 31, 2016

mrunalp commented Mar 31, 2016

wking commented Mar 31, 2016

crosbymichael commented Mar 31, 2016

mrunalp commented Mar 31, 2016

wking commented Mar 31, 2016

Add support for MaskPaths and ReadonlyPaths #320

Add support for MaskPaths and ReadonlyPaths #320

Comments

mlaventure commented Feb 4, 2016

jessfraz commented Feb 4, 2016

jessfraz commented Feb 4, 2016

mlaventure commented Feb 4, 2016

wking commented Feb 4, 2016

tonistiigi commented Feb 25, 2016

wking commented Feb 25, 2016

tonistiigi commented Feb 26, 2016

wking commented Feb 26, 2016

hqhq commented Feb 26, 2016

crosbymichael commented Feb 27, 2016

crosbymichael commented Mar 30, 2016

mlaventure commented Mar 30, 2016

wking commented Mar 30, 2016

wking commented Mar 30, 2016

crosbymichael commented Mar 31, 2016

vbatts commented Mar 31, 2016

crosbymichael commented Mar 31, 2016

mrunalp commented Mar 31, 2016

wking commented Mar 31, 2016

crosbymichael commented Mar 31, 2016

mrunalp commented Mar 31, 2016

wking commented Mar 31, 2016