config: Single, unified config file

[wking]

Reverting 7232e4b (specs: introduce the concept of a runtime.json,
2015-07-30, #88) after discussion on the mailing list. The main
reason is that it's hard to draw a clear line around “inherently
runtime-specific” or “non-portable”, so we shouldn't try to do that in
the spec. Folks who want to flag settings as non-portable for their
own system are welcome to do so (e.g. “we will clobber hooks in
bundles we run”) are welcome to do so, but we don't have to have to
split the config into multiple files to do that.

There have been a number of additional changes since #88, so this
isn't a pure Git reversion. Besides copy-pasting and the associated
link-target updates, I've:

• Restored path → destination, now that the mount type contains both
  source and target paths again. I'd prefer target to destination
  to match mount(2), but the pre-7232e4b1 phrasing was destination
  (possibly due to Windows using target for the source?).
• Restored the Windows mount example to its pre-7232e4b1 content.
• Removed required mounts from the config example (requirements landed
  in 3848a23, config-linux: specify the default devices/filesystems
  available, 2015-09-09, config-linux: specify the default devices/filesystems available #164), because specifying those mounts in the
  config is now redundant.
• Used headers (vs. bold paragraphs) to set off mount examples so we
  get link anchors in the rendered Markdown.
• Replaced references to runtime.json with references to config.json.

[wking]

There aren't many semantic changes here (just the mount re-unification), but it is touching a number of files, so I expect it may cause trivial merge conflicts with a number of open PRs (e.g. #171, #267). I'm happy to rebase this PR if those land first, or to help with rebasing those PRs if this one lands first.

[philips]

Really impossible to review but I will trust that you did it right :-P

LGTM

[wking]

On Tue, Dec 29, 2015 at 02:51:14AM -0800, Brandon Philips wrote:

Really impossible to review…

Yeah :/. If anyone has thoughts on a more reviewable way of doing
this, I'm happy to reroll. Otherwise I guess we just read through it
a few times before it lands, and then fix any bugs that slipped
through in follow-up PRs.

[hqhq]

After a brief review, all looks good, since there are not many semantic changes, I agree we can land it and fix any slipped bugs in follow up PRs. So LGTM.

Since it's a big PR, I think we can wait for one more LGTM then merge.

[wking]

On Thu, Dec 31, 2015 at 12:13:04AM -0800, Qiang Huang wrote:

Since it's a big PR, I think we can wait for one more LGTM then merge.

#88 was proposed by @philips who LGTMed this one 1, but #88 was
LGTMed by @crosbymichael, @vbatts, and @mrunalp. To avoid a split
between maintainers in favor of split or unified configs, it's
probably better to leave this PR open until they've each chimed in
here. So I'd suggest waiting for more than one additional LGTM.

[vbatts]

I am not terribly opposed. My objection is the mapping of hooks and mounts
(and potentially other configurations), that are provided by bundle
authors, that a runtime would need to ignore. These are security concerns.

On Thu, Dec 31, 2015 at 11:08 AM W. Trevor King [email protected]
wrote:

On Thu, Dec 31, 2015 at 12:13:04AM -0800, Qiang Huang wrote:

Since it's a big PR, I think we can wait for one more LGTM then merge.

#88 was proposed by @philips who LGTMed this one 1, but #88 was
LGTMed by @crosbymichael, @vbatts, and @mrunalp. To avoid a split
between maintainers in favor of split or unified configs, it's
probably better to leave this PR open until they've each chimed in
here. So I'd suggest waiting for more than one additional LGTM.

—
Reply to this email directly or view it on GitHub
#284 (comment).

[wking]

On Tue, Jan 05, 2016 at 08:40:48AM -0800, Vincent Batts wrote:

… the mapping of hooks and mounts (and potentially other
configurations), that are provided by bundle authors, that a runtime
would need to ignore. These are security concerns.

Agreed, and my preferred approach to that is flagging sensitive
settings in the docs 1. Adding those flags in this PR seemed to be
biting off too much, but I will certainly start filing warnings of
that sort (and welcome help from others in doing so) if/when something
like this PR lands.

 Subject: Single, unified config file (i.e. rolling back specs#88)
 Date: Wed, 4 Nov 2015 09:53:20 -0800
 Message-ID: <[email protected]>

 “The opencontainers/specs authors can flag settings they feel have
 serious security implications on a per-setting basis, instead of
 splitting the whole config infrastructure into separate files.”

[vbatts]

I don't follow what you're saying

On Tue, Jan 5, 2016 at 3:42 PM W. Trevor King [email protected]
wrote:

On Tue, Jan 05, 2016 at 08:40:48AM -0800, Vincent Batts wrote:

… the mapping of hooks and mounts (and potentially other
configurations), that are provided by bundle authors, that a runtime
would need to ignore. These are security concerns.

Agreed, and my preferred approach to that is flagging sensitive
settings in the docs 1. Adding those flags in this PR seemed to be
biting off too much, but I will certainly start filing warnings of
that sort (and welcome help from others in doing so) if/when something
like this PR lands.

Subject: Single, unified config file (i.e. rolling back specs#88)
Date: Wed, 4 Nov 2015 09:53:20 -0800
Message-ID: [email protected]

“The opencontainers/specs authors can flag settings they feel have
serious security implications on a per-setting basis, instead of
splitting the whole config infrastructure into separate files.”

—
Reply to this email directly or view it on GitHub
#284 (comment).

[wking]

On Tue, Jan 05, 2016 at 12:45:14PM -0800, Vincent Batts wrote:

I don't follow what you're saying

For example:

Mounts

…

Security

To avoid bundle-author attacks (for example, bind-mounting sensitive
host filesystems into the container), runtime callers should inspect
or clear this setting in configurations supplied by untrusted bundle
authors.

Linux Example

…

or:

Hooks

…

Security

Hooks execute in the runtime namespace, so they have the same access
to the runtime environment as the runtime itself. To avoid
bundle-author attacks (for example, "args": ["rm", "-rf", "/"]),
runtime callers should inspect or clear these settings in
configurations supplied by untrusted bundle authors.

Prestart

…

But this depends on being able to paint a per-setting picture of an
attack that's possible via an un-audited instance of the setting in
question, so it's more discussion than I think we want to put in this
PR.

[philips]

On Tue, Jan 5, 2016 at 10:10 PM Vincent Batts [email protected]
wrote:

I am not terribly opposed. My objection is the mapping of hooks and mounts
(and potentially other configurations), that are provided by bundle
authors, that a runtime would need to ignore. These are security concerns.

Yea, it really turns into a mess. I really hope we can avoid making the
config.json part of the only identity of a bundle for this reason as nearly
every use case I can think of requires an adjustment to config.json before
running.[1]

[1] #293 (comment)

[philips]

On Wed, Jan 6, 2016 at 2:52 AM W. Trevor King [email protected]
wrote:

On Tue, Jan 05, 2016 at 12:45:14PM -0800, Vincent Batts wrote:

Security

Hooks execute in the runtime namespace, so they have the same access
to the runtime environment as the runtime itself. To avoid
bundle-author attacks (for example, "args": ["rm", "-rf", "/"]),
runtime callers should inspect or clear these settings in
configurations supplied by untrusted bundle authors.

Do we have example use cases of when a bundle will know the exact mount
location or exact hook path and args?

If we are adding an image format to OCI I think we need to be more
stringent than just suggesting "people audit". Neither appc nor docker have
image formats that allow arbitrary execution of code outside of the
container image itself.

Brandon

[wking]

On Wed, Jan 06, 2016 at 02:20:23AM -0800, Brandon Philips wrote:

Do we have example use cases of when a bundle will know the exact
mount location or exact hook path and args?

It's for a pre-release version of this spec and and old version of
runC, but 1 is mounting a bundle-local directory (‘root’ in the
bundle, ‘/root’ in the configured mount namespace) as persistent
read/write space inside the otherwise-read-only bundle root (‘rootfs’
in the bundle, ‘/’ in the configured mount namespace). You could do
something similar with a runtime-setup layered filesystem
(e.g. unpacking the layers into bundle subdirectories and using mount
entries to stack them).

You have a high probability of being able to mount standardized host
directories from outside the bundle directory (/dev, /var/log, …), so
that would work in most situations, although it's obviously not
guaranteed.

And if you do some template expansion after unpacking the bundle, you
can add mount entries for things like $HOME (expanding to the
runtime-caller's home directory).

I see no need for the current absolute-hook-path requirement 2, and
would like to relax it to a PATH lookup (using the PATH the hook
inherits from the runtime). But even without that, ‘ip’ is at /bin/ip
on all my systems

If we are adding an image format to OCI I think we need to be more
stringent than just suggesting "people audit". Neither appc nor
docker have image formats that allow arbitrary execution of code
outside of the container image itself.

More detailed discussion of the security implications should probably
go back to the list 3, since that's a higher-level issue than “does
this PR cleanly implement the outcome we all agreed on”.

 Subject: Single, unified config file (i.e. rolling back specs#88)
 Date: Wed, 4 Nov 2015 09:53:20 -0800
 Message-ID: <[email protected]>

[duglin]

Docker's Ubuntu image includes a CMD. This is portable metadata that can be overridden at container creation time - there are others, e.g. env vars. Whether the decision to override any config data is made by the user or by the infrastructure doesn't really matter, the key point to me is that any piece of data provided by the bundle's config can be modified. Is memory (when viewed as the "recommended memory that should be used") really that different? The infrastructure, or user/deployer, should be free to accept or reject those recommended values - as they can in Docker with values like the CMD & env vars. When viewed this way, I don't see much value in trying to divide the metadata into two categories as long as we make it clear that an impl is free to modify any bit of config data (e.g. memory) for any reason (perhaps its out range, or it will always default to a certain value unless specifically given a value by the user).

[vbatts]

TBH, this effectively reads to me like "Here are the rules to provide a config that serves only as hints to the operator or the container runtime". Its seems a surprisingly non-deterministic delivery, and the security of running arbitrary code being that some hints weren't meant to be used.

[duglin]

Don't disagree, but if we don't do it then aren't we in effect banning things like docker run ubuntu some-other-cmd ?

[vbatts]

Not so much. This is why I said on the call today, there is a distinction
to be made here, that the config as is provided, is to be distinct from the
config as is run by the operator and container-runtime. Even if the
structure in config.json is just the minimal pieces of a structure. But
after the operator provides flags and settings to a container-runtime. This
would then populate the structure more fully. It would seem then that this
populated structure would written somewhere. (Perhaps in the same folder as
the state.json). This would be not a portable config, but would serve as
reference and potentially used as the defaults for future defaults of a
bundle.

On Wed, Jan 6, 2016 at 10:28 PM Doug Davis [email protected] wrote:

Don't disagree, but if we don't do it then aren't we in effect banning
things like "docker run ubuntu some-other-cmd" ?

—
Reply to this email directly or view it on GitHub
#284 (comment).

[vbatts]

(which is still similar enough to the docker concept of image config and
container config structures and how they carry-over)

On Wed, Jan 6, 2016 at 10:41 PM Vincent Batts [email protected]
wrote:

Not so much. This is why I said on the call today, there is a distinction
to be made here, that the config as is provided, is to be distinct from the
config as is run by the operator and container-runtime. Even if the
structure in config.json is just the minimal pieces of a structure. But
after the operator provides flags and settings to a container-runtime. This
would then populate the structure more fully. It would seem then that this
populated structure would written somewhere. (Perhaps in the same folder as
the state.json). This would be not a portable config, but would serve as
reference and potentially used as the defaults for future defaults of a
bundle.

On Wed, Jan 6, 2016 at 10:28 PM Doug Davis [email protected]
wrote:

Don't disagree, but if we don't do it then aren't we in effect banning
things like "docker run ubuntu some-other-cmd" ?

—
Reply to this email directly or view it on GitHub
#284 (comment)
.

[duglin]

Additionally, I think its important to keep in mind the difference between "on the wire" bundle metadata from "on the disk" bundle metadata. I agree with you that "on disk" metadata MUST NOT be ignored, either adhere to it or generate an error. But, when we talk about including metadata in the "on the wire" bundle (whether its portable or non-portable), I think that data is view more like hints exactly for the reasons we're discussing - could be out of range, could be overridden by the user, etc... So the process in converting the metadata into "on the disk" metadata will make those decisions. I think for the purposes of this discussion we should focus just on what 'runc' needs to do its job, and not how that data is put on to disk or how its used later on (like in porting to another system). And if runc start doesn't need the distinction then I'm not sure we should add it.

[vbatts]

this needs a major rebase

[wking]

On Wed, Jan 13, 2016 at 03:21:05PM -0800, Vincent Batts wrote:

this needs a major rebase

Yeah, and doing that is a real pain ;). In the light of the “do we
want this” pushback in this PR, I've gone back to the list 1. Once
we are all on board with the idea, I'll reroll this branch to land on
whatever master we have at that point.

 Subject: Re: Single, unified config file (i.e. rolling back specs#88)
 Date: Thu, 7 Jan 2016 00:55:33 -0800
 Message-ID: <[email protected]>

[wking]

On Wed, Jan 13, 2016 at 03:30:29PM -0800, W. Trevor King wrote:

Wed, Jan 13, 2016 at 03:21:05PM -0800, Vincent Batts:

this needs a major rebase

Yeah, and doing that is a real pain ;). In the light of the “do we
want this” pushback in this PR, I've gone back to the list 1. Once
we are all on board with the idea, I'll reroll this branch to land on
whatever master we have at that point.

It sounds like we're at that point now 1, so I've rerolled with
92a8686 → 526362e. There were conflicts and/or updates required due
to #279, #280, #283, #291, #292, #295, and #296. I think I made the
necessary updates to this commit, but if anyone sees something I've
missed please let me know.

[wking]

On Tue, Dec 29, 2015 at 08:20:03AM -0800, W. Trevor King wrote:

Tue, Dec 29, 2015 at 02:51:14AM -0800, Brandon Philips:

Really impossible to review…

Yeah :/. If anyone has thoughts on a more reviewable way of doing
this, I'm happy to reroll…

So no change in the commit process, but this helps a bit with
reviewing the unified files:

$ diff -u <(git cat-file -p HEAD^:config.md; git cat-file -p HEAD^:runtime-config.md) config.md

which shows what I'm suggesting (config.md) vs. a pure concatenation
of the unified files (the ‘<(…)’ stuff). In the config.md case, it
shows the changes due to mount source/target unification.

[hqhq]

LGTM

[vbatts]

This is on my plate to review today. I just posted https://gist.github.com/vbatts/5ac2723c9598875ffb0e, which is the config I have been iterating on while working on #313
It'll be a bit of walking through comparing them.

[mrunalp]

I think #284 could be reviewed/merged while #313 is being worked on i.e. #313 could make changes if required after #284 is merged.

[mrunalp]

Well, #316 got merged and this needs a rebase again :/

[vbatts]

LGTM. needs a rebase

[wking]

On Wed, Jan 27, 2016 at 07:42:26AM -0800, Vincent Batts wrote:

LGTM. needs a rebase

Rebased around #316 with 2551cf1 → cb2da54. You can check the
rebase with something like:

$ diff -u <(git diff 2551cf1^..cb2da54^) <(git diff 2551cf1..cb2da54)

which shows the only differences are contextual stuff like:

-@@ -195,7 +195,7 @@ type Network struct {
+@@ -208,7 +208,7 @@ type Network struct {