Add validation for sysctl

[mrunalp]

/proc/sys isn't completely namespaced and only some properties are allowed
per linux namespace.

The list has been compiled from namespaces(7)

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

[mrunalp]

@crosbymichael @LK4D4 ping
cc: @rhatdan

[crosbymichael]

is it even worth having this field when only a few are valid?

[mrunalp]

@crosbymichael We have a lot of customers asking for this. net itself gives a lot of options.

[crosbymichael]

oh, right, i see the prefixes now

[mrunalp]

Also, more options will be made configurable over time from what I gathered talking to kernel engineers.

[rhatdan]

@jeremyeder PTAL

Are all of these network sysctls actually Namespace aware?

[rhatdan]

Would we want to make this available to docker as a public interface so that it could catch these early in the parse commands?

[mrunalp]

@rhatdan Yes, we could do that and docker could validate early. Also, @jeder's input will be useful to make sure this list is sane. I am going to look at kernel source code to see if I missed any settings. There is no other way unfortunately.

[rhatdan]

I think jeder should be @jeremyeder.

[jeremyeder]

@Mrunal No, we can't just pass in the whole "net" sysctl space. Even though some might actually work, there's some strange side-effects; for example you can adjust net.ipv4.tcp_mem in a container but the stack actually respects what's set on the host's kernel.

[rhatdan]

vm.nr_hugepages

Does not blow up, however not sure it worked correctly.

docker run --privileged -ti --sysctl vm.nr_hugepages=2000 fedora sh
cat /proc/sys/vm/nr_hugepages
285

docker run --privileged -ti --sysctl vm.nr_hugepages=5000 fedora sh
# cat /proc/sys/vm/nr_hugepages
310

[mrunalp]

@jeremyeder Thanks for your input. I will get input from the right folks to create a better list for net and things that may be missing here.

[jeder]

I'm not @jeremyeder, so if he is not responding, you may have an address
wrong.

-John Eder

On Tue, Sep 29, 2015 at 4:43 AM, Mrunal Patel notifications@github.com
wrote:

@jeremyeder https://github.com/jeremyeder Thanks for your input. I will
get input from the right folks to create a better list for net and things
that may be missing here.

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

J. Eder

+1 619.847.5579 | +81 80.8903.2817 | j@eder.ws

[crosbymichael]

Should we validate at all? If you asked for something we just try to do it?

[rhatdan]

I don't think we should validate. We should leave his up to the user to validate whether it works or not, as we explain in the man pages and docs. Perhaps it would be helpful to have a page where we list sysctls that we know work with a particular kernel version.

Going forward this check is going to be very kernel specific, so verification is often going to be wrong.

[mrunalp]

@crosbymichael @rhatdan Sounds good. If we find a better way we could add it in the future.

[mrunalp]

Re-opening to see if there is interest in merging this so sysctl could be enabled safely in docker

[mrunalp]

@crosbymichael @LK4D4 WDYT? There are quite a few use cases that are blocked by not enabling this feature in docker.

[vishh]

From a community sense, it makes sense to curate a list of safe procfs tunables as part of runc. Until we have a complete list, we could possibly allow a backdoor.

[crosbymichael]

Is there a way were we can move this out of the specs and into hooks/ create/ start lifecycle?

[mrunalp]

It would be possible but a very roundabout way. Any concerns with doing it like in this PR? We could go by the man page and support what is documented.

[rhatdan]

@crosbymichael Why do you think it would be better in one of those?

[crosbymichael]

@mrunalp do you just want to do the validation at the higher level so that we don't have to recompile or update libcontainer/runc when something changes?

[rhatdan]

Not sure you want to do this, if we want to prevent users from shooting themselves in the foot, when doing runc/oci type apps. If we want to protect them, we should protect them in all tools.

One thing we are doing in runc is also looking at whether they disabled the host or ipc namespace, which is pretty difficult to do in the docker cli.

[crosbymichael]

LGTM

[mrunalp]

If we do run into an issue where we have to frequently modify this list then we could add configuration for it. I think we can merge this and then see how it goes.

[hqhq]

So after this, container will never be as privileged as host, isn't this gonna break some users who use container to hack with host, like rancherOS? ping @ibuildthecloud

The validation looks reasonable to me, just think maybe we should make it optional.

[mrunalp]

We could make it optional in privileged mode as the file system is made read only in non privileged anyways.

Sent from my iPhone

On Feb 19, 2016, at 9:36 AM, Qiang Huang notifications@github.com wrote:

So after this, container will never be as privileged as host, isn't this gonna break some users who use container to hack with host, like rancherOS? ping @ibuildthecloud

The validation looks reasonable to me, just think maybe we should make it optional.

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

[rhatdan]

Of course in --privileged mode you can just execute sysctl directly.

[mrunalp]

I think we don't need to modify this PR. One can go and set /proc/sys when in privileged mode and there is no harm in the validation when using the CLI so one distinguish between what is safe in container vs being set on the host.

[rhatdan]

I agree. We are only allowing what we know of as "Namespaced sysctls"

[hqhq]

@mrunalp Do you mean we can make the sysctl validation optional now without any furture modification? Can you explain how to do that?

[mrunalp]

I mean that this patch is sufficient. No special handling of privileged is necessary as /proc/sys is writeable under privileged and allows the user to set sysctls that are not container specific.

Sent from my iPhone

On Feb 23, 2016, at 1:46 PM, Qiang Huang notifications@github.com wrote:

@mrunalp Do you mean we can make the sysctl validation optional now without any furture modification? Can you explain how to do that?

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

[hqhq]

@mrunalp Thanks for clarification, I'm worry about people might want use sysctl with CLI in --privileged mode which is forbidden by the validation (these non-namespaced sysctls). Anyway we can do that in follow PRs if it do bother some users.

LGTM.

[rhatdan]

@hqhq I think the current assumption is that --privileged does not change other behaviour in the system. We were discussing similar behaviour with UserNamespace and it was decided that --privileged does not effect userNamespace but a --userns=host is being added to turn off user namespace. I think that precedent would follow here. In that you can do priv ops in the container, but we will continue to block you doing something accidentally.