Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Docker run with --sysctl accepts with --net=host and option values other than 0 and 1 #27484

Open
dattatrayakumbhar opened this issue Oct 18, 2016 · 22 comments

Comments

@dattatrayakumbhar
Copy link
Contributor

@dattatrayakumbhar dattatrayakumbhar commented Oct 18, 2016

Description
Docker run with --sysctl option works with --net=host. In Document, it is said that it should not work with --net=host.
net.ipv4.ip_forward value should be either 1 or 0. It accepts anything other than 1 and 0
Document link:
https://docs.docker.com/engine/reference/commandline/run/#/full-container-capabilities---privileged

Steps to reproduce the issue:

  1. Create container with --net=host an --sysctl
    docker run -it --sysctl net.ipv4.ip_forward=1 --net=host ubuntu
    which should not be allowed
  2. docker run -it --sysctl net.ipv4.ip_forward=a --net=host ubuntu
    Should give proper error saying 'option value should be 0 or 1'

Describe the results you received:

  1. Create container with --net=host an --sysctl
    docker run -it --sysctl net.ipv4.ip_forward=1 --net=host ubuntu
    :- Accepted --net=host with --sysctl
  2. docker run -it --sysctl net.ipv4.ip_forward=a --net=host ubuntu
    :- Failed with error "docker: Error response from daemon: oci runtime error: write /proc/sys/net/ipv4/ip_forward: invalid argument.
    "
    Describe the results you expected:
  3. Create container with --net=host an --sysctl
    docker run -it --sysctl net.ipv4.ip_forward=1 --net=host ubuntu
    :- Should not accept --net=host with --sysctl
  4. docker run -it --sysctl net.ipv4.ip_forward=a --net=host ubuntu
    :- Should give proper error saying 'option value should be 0 or 1'

Additional information you deem important (e.g. issue happens only occasionally):
Issue happens every time

Output of docker version:
[root@nfs-server infra_manager]# docker version
Client:
Version: 1.12.1
API version: 1.24
Go version: go1.6.3
Git commit: 23cf638
Built:
OS/Arch: linux/amd64

Server:
Version: 1.12.1
API version: 1.24
Go version: go1.6.3
Git commit: 23cf638
Built:
OS/Arch: linux/amd64

Output of docker info:
Containers: 6
Running: 0
Paused: 0
Stopped: 6
Images: 1
Server Version: 1.12.1
Storage Driver: devicemapper
Pool Name: docker-253:1-747269-pool
Pool Blocksize: 65.54 kB
Base Device Size: 10.74 GB
Backing Filesystem: xfs
Data file: /dev/loop0
Metadata file: /dev/loop1
Data Space Used: 209.2 MB
Data Space Total: 107.4 GB
Data Space Available: 1.883 GB
Metadata Space Used: 1.065 MB
Metadata Space Total: 2.147 GB
Metadata Space Available: 1.883 GB
Thin Pool Minimum Free Space: 10.74 GB
Udev Sync Supported: true
Deferred Removal Enabled: false
Deferred Deletion Enabled: false
Deferred Deleted Device Count: 0
Data loop file: /var/lib/docker/devicemapper/devicemapper/data
WARNING: Usage of loopback devices is strongly discouraged for production use. Use --storage-opt dm.thinpooldev to specify a custom block storage device.
Metadata loop file: /var/lib/docker/devicemapper/devicemapper/metadata
Library Version: 1.02.107-RHEL7 (2016-06-09)
Logging Driver: json-file
Cgroup Driver: cgroupfs
Plugins:
Volume: local
Network: bridge host null overlay
Swarm: inactive
Runtimes: runc
Default Runtime: runc
Security Options: seccomp
Kernel Version: 3.10.0-327.36.1.el7.x86_64
Operating System: CentOS Linux 7 (Core)
OSType: linux
Architecture: x86_64
CPUs: 2
Total Memory: 7.797 GiB
Name: nfs-server.cisco.com
ID: YCHJ:GOSU:POEO:MGF3:C3QV:ZWIR:T5ZK:POHY:XBQS:CH7O:RLVB:75AZ
Docker Root Dir: /var/lib/docker
Debug Mode (client): false
Debug Mode (server): false
Username: dattatrayakumbhar04
Registry: https://index.docker.io/v1/
Insecure Registries:
127.0.0.0/8

Additional environment details (AWS, VirtualBox, physical, etc.):
Instance is on aws ..
OS: ubuntu/centos

@justincormack
Copy link
Contributor

@justincormack justincormack commented Oct 18, 2016

I don't know how realistic validating the sysctl options is, there are a large number of them and in many cases the allowed values depend on the kernel version.

It should not work with host namespaces, I agree.

Loading

@justincormack
Copy link
Contributor

@justincormack justincormack commented Oct 18, 2016

Yes, there is no attempt to look at the namespaces before setting sysctl options. In theory the different types of options should depend on different namespaces, eg net.* depends on net namespace, so doing this correctly is a bit fiddly.

Loading

@gaocegege
Copy link
Contributor

@gaocegege gaocegege commented Oct 18, 2016

#dibs

Loading

@gaocegege
Copy link
Contributor

@gaocegege gaocegege commented Oct 18, 2016

Hi, I will take it 😄
Should we check the netmode and sysctl in client, just like https://github.com/docker/docker/blob/master/cli/command/container/run.go#L90:#L92 or add the check to daemon code?

Loading

@justincormack
Copy link
Contributor

@justincormack justincormack commented Oct 18, 2016

@gaocegege I think it should be tested in the daemon.

Loading

@justincormack
Copy link
Contributor

@justincormack justincormack commented Oct 19, 2016

@gaocegege there seem to be checks in runc already https://github.com/opencontainers/runc/blob/master/libcontainer/configs/validate/validator.go#L107 which should do the right thing. Not sure why they are not working...

Loading

@gaocegege
Copy link
Contributor

@gaocegege gaocegege commented Oct 19, 2016

re @justincormack
it seems that docker's vendor is not up to date.
our vendor/src/github.com/opencontainers/runc/libcontainer/configs doesn't has the config validator.

So maybe the issue could be solved by updating vendor directory.

Loading

@gaocegege
Copy link
Contributor

@gaocegege gaocegege commented Oct 19, 2016

oh, wrong, let me check it again.

Loading

@gaocegege
Copy link
Contributor

@gaocegege gaocegege commented Oct 19, 2016

when docker run --sysctl net.ipv4.ip_forward=1 --net=host <image>, the config.json is

...
"linux": {
    "sysctl": {
            "net.ipv4.ip_forward": "1"
    },
    ...
    "namespaces": [
        {
            "type": "mount"
        },
        {
            "type": "network",
            "path": "/var/run/docker/netns/default"
        },
        {
            "type": "uts"
        },
        {
            "type": "pid"
        },
        {
            "type": "ipc"
        }
    ],
}

And when docker run --sysctl net.ipv4.ip_forward=1 <image> container, the config.json is

...
"linux": {
    "sysctl": {
            "net.ipv4.ip_forward": "1"
    },
    ...
    "namespaces": [
        {
            "type": "mount"
        },
        {
-             "type": "network",
-             "path": "/var/run/docker/netns/default"
+            "type": "network"
        },
        {
            "type": "uts"
        },
        {
            "type": "pid"
        },
        {
            "type": "ipc"
        }
    ],
}

And, the validator in runc returns sysctl "net.ipv4.ip_forward" is not allowed in the hosts network namespace error when the config.json is

...
"linux": {
    "sysctl": {
            "net.ipv4.ip_forward": "1"
    },
    ...
    "namespaces": [
        {
            "type": "mount"
        },
-       {
-             "type": "network",
-             "path": "/var/run/docker/netns/default"
-       },
        {
            "type": "uts"
        },
        {
            "type": "pid"
        },
        {
            "type": "ipc"
        }
    ],
}

Because network namespace exists if the "type" field is filled

ref https://github.com/opencontainers/runc/blob/master/libcontainer/configs/validate/validator.go#L128

I think we could add code to runc config validator, or check the fields before sending requests to containerd, any other suggestions about how to fix it?🤔

Loading

@justincormack
Copy link
Contributor

@justincormack justincormack commented Oct 19, 2016

The specification is here https://github.com/opencontainers/runtime-spec/blob/master/config-linux.md

The validator should catch the existing case - no namespace specified so it will make no changes and so be in host namespace, but it should also fail if a path is specified so the container does not own the namespace (and it may be the host namespace).

This would then also fail if you do docker run --net=container:... --sysctl which I think is also correct (although some could disagree). If there is disagreement, we could also not pass the namespace in the runc config if it is specified as host, which would also seem correct.

cc @crosbymichael WDYT?

Loading

@crosbymichael
Copy link
Contributor

@crosbymichael crosbymichael commented Oct 19, 2016

@justincormack I'd rather not make too many changes on validation. I think fixing the validator to prohibit net host, both when its omitted from the array and if it has a path pointing to the calling namespace. Other than that I think it should be allowed because someone is probably depending on that today so that one container can configure the network namespace of another or something crazy like that, who knows.

Loading

@crosbymichael
Copy link
Contributor

@crosbymichael crosbymichael commented Oct 21, 2016

@gaocegege

If you are still interested in working on this I think the best approach is to change the logic here:

https://github.com/opencontainers/runc/blob/master/libcontainer/configs/validate/validator.go#L128

There are two checks we can make for seeing if the user specified the host namespace.

  1. The Namespaces list does not have the NEWNET struct added to it
  2. If it does have a NEWNET struct added and the path is not an empty string then we need to
    a. Do a readlink on the current processes network namespace os.Readlink("/proc/self/ns/net")
    b. Do a readlink on the Path provided in the struct and compare the current processes network namespace with the one provided in the Path. If they match then it should be considered as the host namespace.

What do you think?

Loading

@gaocegege
Copy link
Contributor

@gaocegege gaocegege commented Oct 21, 2016

re @crosbymichael

Yeah, so we change the logic in runc and update https://github.com/docker/docker/blob/master/hack/dockerfile/install-binaries.sh#L6, right?

I will open a PR, if I have some problems, could I ask you for help?

😄

Loading

gaocegege added a commit to gaocegege/runc that referenced this issue Oct 21, 2016
gaocegege added a commit to gaocegege/runc that referenced this issue Oct 21, 2016
Signed-off-by: Ce Gao <ce.gao@outlook.com>
gaocegege added a commit to gaocegege/runc that referenced this issue Oct 21, 2016
Signed-off-by: Ce Gao <ce.gao@outlook.com>
gaocegege added a commit to gaocegege/runc that referenced this issue Oct 22, 2016
Signed-off-by: Ce Gao <ce.gao@outlook.com>
gaocegege added a commit to gaocegege/runc that referenced this issue Oct 22, 2016
Signed-off-by: Ce Gao <ce.gao@outlook.com>
gaocegege added a commit to gaocegege/runc that referenced this issue Oct 22, 2016
Signed-off-by: Ce Gao <ce.gao@outlook.com>
hqhq added a commit to opencontainers/runc that referenced this issue Oct 25, 2016
moby/moby#27484-check if sysctls are used in host network mode.
@gaocegege
Copy link
Contributor

@gaocegege gaocegege commented Oct 25, 2016

The logic has been merged into runc: opencontainers/runc@4ec570d

Should we update runc to this commit? 🤔

Loading

@justincormack
Copy link
Contributor

@justincormack justincormack commented Oct 26, 2016

Should not have been closed, ugh, github.

@gaocegege can you test this in docker/docker see my comment here opencontainers/runc#1138 (comment)

Loading

@justincormack
Copy link
Contributor

@justincormack justincormack commented Oct 26, 2016

ugh, github stop closing it! ok, need to update here.

Loading

@thaJeztah thaJeztah reopened this Nov 3, 2016
@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Nov 3, 2016

opening again 😄

Loading

@mlaventure mlaventure closed this in 2e28d80 Nov 4, 2016
@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Nov 4, 2016

aaaaand reopening....again (I guess) @mlaventure ^^

Loading

@thaJeztah thaJeztah reopened this Nov 4, 2016
@anandrm
Copy link

@anandrm anandrm commented Nov 23, 2016

Just tried this on 1.13-dev version .

Is this the expected response ?

#docker run -it --sysctl net.ipv4.ip_forward=a --net=host ubuntu
docker: Error response from daemon: oci runtime error: container_linux.go:247: starting container process caused "process_linux.go:359: container init caused "write /proc/sys/net/ipv4/ip_forward: invalid argument"".

docker info

Containers: 3
Running: 0
Paused: 0
Stopped: 3
Images: 6
Server Version: 1.13.0-dev
Storage Driver: aufs
Root Dir: /var/lib/docker/aufs
Backing Filesystem: extfs
Dirs: 14
Dirperm1 Supported: true
Logging Driver: json-file
Cgroup Driver: cgroupfs
Plugins:
Volume: local
Network: bridge host null overlay
Swarm: active
NodeID: 0cl24z968g5h1kezsckpkrxn1
Is Manager: false
Node Address: 10.11.23.145
Runtimes: runc
Default Runtime: runc
Security Options: apparmor seccomp
Kernel Version: 4.4.0-42-generic
Operating System: Ubuntu 16.04.1 LTS
OSType: linux
Architecture: x86_64
CPUs: 1
Total Memory: 992.5 MiB
Name: ubuntu16
ID: PTKW:IWFD:Q7WH:SUDK:6LNC:2WRR:DDZ3:FVEB:YUZZ:HM2B:ISOH:YDAU
Docker Root Dir: /var/lib/docker
Debug Mode (client): false
Debug Mode (server): false
Registry: https://index.docker.io/v1/
WARNING: No swap limit support
Insecure Registries:
127.0.0.0/8
Live Restore Enabled: false

Loading

@thaJeztah thaJeztah added this to the 1.13.0 milestone Nov 23, 2016
@boaz0
Copy link
Member

@boaz0 boaz0 commented Jul 16, 2017

@thaJeztah it seems like we can close this one.

Loading

@ntjn
Copy link

@ntjn ntjn commented Oct 30, 2017

Same error message with Docker version 17.10.0-ce, build f4ffd2511c
Seems to be expected message for me.

Loading

@boaz0
Copy link
Member

@boaz0 boaz0 commented Oct 30, 2017

@ntjn Can you copy-paste docker info and docker version?

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

10 participants