Skip to content

Conversation

@mrunalp
Copy link
Contributor

@mrunalp mrunalp commented Sep 14, 2015

This allows one to hide files such as /proc/kcore from the container process.

Signed-off-by: Mrunal Patel mrunalp@gmail.com

Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
@mrunalp
Copy link
Contributor Author

mrunalp commented Sep 14, 2015

@crosbymichael @LK4D4 PTAL

@wking
Copy link
Contributor

wking commented Sep 14, 2015

On Mon, Sep 14, 2015 at 02:25:03PM -0700, Mrunal Patel wrote:

This allows one to hide files such as /proc/kcore from the container process.

Why not just have this in config.md:

"mounts": [
{"name": "null", "path": "/proc/kcore"},

],

And this in runtime.md:

"mounts": {
"null": {
"type": "bind",
"source": "/dev/null"
},

}

? That way there's nothing to implement, and it doesn't seem like
that much of a setup cost for the bundle author.

@mrunalp
Copy link
Contributor Author

mrunalp commented Sep 16, 2015

@wking Agree, but the idea is simplifying the config for some common cases/scenarios. If we agree it is not worth it I will close this :)

@wking
Copy link
Contributor

wking commented Sep 16, 2015

On Wed, Sep 16, 2015 at 08:06:19AM -0700, Mrunal Patel wrote:

@wking Agree, but the idea is simplifying the config for some common
cases/scenarios. If we agree it is not worth it I will close this :)

Fair. Some things to balance when deciding on whether adding
something simplifies things or not:

a. Does it make it easier to write the config for a particular use
case?
b. Does the additional complication make the spec harder to
understand?

Any pure addition is likely to do both, although refactoring things
might make use cases easier and simplify the spec.

Perhaps we can mitigate (b) here by shifting syntactic sugar like this
PR (where the same effect could be achieved by more primitive/generic
config entries) off into its own section. That way folks could read
through the core section more quickly, and only dip into these
shortcut options if/when they feel like they want something more
convenient.

And to make life easier for implementors, it might be worth writing a
generic preprocessor that unwinds the syntactic sugar into the more
primitive/generic constructs? That would also make it easier to tell
how the sugar fits in (e.g. presumably the maskedPaths mounts are made
after we've finished setting up the generic mounts, but that's not
explicitily stated in this PR as of 8d705eb).

@@ -250,6 +250,16 @@ The actions and operators are strings that match the definitions in seccomp.h fr
}
```

## Masked Paths

Masked paths allow hiding paths in the container by bind mounting `/dev/null` over them to prevent access by the container process.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps that mount approach is an implementation approach, but not the requirement?
If some runtime, cow fs, or future kernel feature allowed the filepath to just not exist, then those could be allowable as well. Not just an exclusive bitbucket.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Fri, Sep 25, 2015 at 11:17:19AM -0700, Vincent Batts wrote:

+Masked paths allow hiding paths in the container by bind mounting
/dev/null over them to prevent access by the container process.

perhaps that mount approach is an implementation approach, but not
the requirement? If some … cow fs … feature allowed the filepath to
just not exist…

What would this look like? Looking for similar tech besides “bind
over it”, there's the device cgroups 1. But there the access is
bound to major/minor, and not by path. That's not a big deal though,
I could see something like that bound by path. But the cgroup
approach gives errors like:

cgcreate -g devices:/device-group

cgset -r devices.deny='c 1:3 mrw' device-group

cgexec -g devices:/device-group cat /dev/null

cat: /dev/null: Operation not permitted

cgdelete -g devices:/device-group

and not just an empty read. So I'd be cautious about saying “don't
worry about how it's implemented, just use this to block access”.

@vbatts
Copy link
Member

vbatts commented Sep 25, 2015

I like the idea, just need to open up the verbiage.

@mrunalp
Copy link
Contributor Author

mrunalp commented Dec 9, 2015

I am closing this for now. We could use the existing facilities to achieve this.

@mrunalp mrunalp closed this Dec 9, 2015
wking added a commit to wking/nmbug-oci that referenced this pull request Dec 31, 2015
I still feel like these should be opt-in, but the consensus is that
they should be opt-out [1].  That is currently blocking on suggested
syntax around that opt-out.  My suggestion [1] was to borrow the
maskedPaths syntax from [2], but I haven't heard any direct responses
to that.

[1]: Message-ID: <20151216215513.GG25571@odin.tremily.us>
     Subject: Re: Linux: Don't mount /sys and /proc (i.e. rolling back specs#164)
     Date: Wed, 16 Dec 2015 13:55:13 -0800
[2]: opencontainers/runtime-spec#186
     Subject: Masked paths setting in the container
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants