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/docker#27484-check if sysctls are used in host network mode. #1138

Merged
merged 2 commits into from
Oct 25, 2016

Conversation

gaocegege
Copy link
Contributor

@gaocegege gaocegege commented Oct 21, 2016

ref moby/moby#27484

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
    1. Do a readlink on the current processes network namespace os.Readlink("/proc/self/ns/net")
    2. 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.

Signed-off-by: Ce Gao ce.gao@outlook.com

@gaocegege
Copy link
Contributor Author

PTAL @crosbymichael

@gaocegege
Copy link
Contributor Author

Sorry about it, I have added it.

@crosbymichael
Copy link
Member

crosbymichael commented Oct 21, 2016

LGTM

Approved with PullApprove

if err != nil {
return fmt.Errorf("read soft link %q error", path)
}
if destOfContainer == destOfCurrentProcess {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a helper method to compare namespace links?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, the logic is a little complex.

@gaocegege gaocegege force-pushed the fix-config-validator branch 3 times, most recently from c307ab2 to 3ce3ea5 Compare October 22, 2016 02:41
@gaocegege
Copy link
Contributor Author

PTAL

return fmt.Errorf("sysctl %q is not allowed in the hosts network namespace", s)
}
if config.Namespaces.PathOf(configs.NEWNET) != "" {
Copy link
Member

@cyphar cyphar Oct 22, 2016

Choose a reason for hiding this comment

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

You can do this:

if path := config.Namespaces.PathOf(configs.NEWNET); path != "" {
  if err := checkHostNs(s, path); err != nil {
    return err
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it.

Signed-off-by: Ce Gao <ce.gao@outlook.com>
Signed-off-by: Ce Gao <ce.gao@outlook.com>
@cyphar
Copy link
Member

cyphar commented Oct 22, 2016

LGTM

Approved with PullApprove

@gaocegege
Copy link
Contributor Author

gaocegege commented Oct 22, 2016

PTAL
add some test cases now.

@hqhq
Copy link
Contributor

hqhq commented Oct 25, 2016

LGTM

Approved with PullApprove

@hqhq hqhq merged commit 4ec570d into opencontainers:master Oct 25, 2016
@justincormack
Copy link
Contributor

@gaocegege I was testing an update to runc in docker and it looks like this causes a CI failure with:

14:08:40 
14:08:40 ----------------------------------------------------------------------
14:08:40 FAIL: docker_cli_run_unix_test.go:888: DockerSuite.TestRunSysctls
14:08:40 
14:08:40 docker_cli_run_unix_test.go:893:
14:08:40     out, _ := dockerCmd(c, "run", "--sysctl", "net.ipv4.ip_forward=1", "--name", "test", "busybox", "cat", "/proc/sys/net/ipv4/ip_forward")
14:08:40 docker_utils.go:470:
14:08:40     c.Assert(result, icmd.Matches, icmd.Success)
14:08:40 ... result *cmd.Result = &cmd.Result{Cmd:(*exec.Cmd)(0xc42044e840), ExitCode:125, Error:(*exec.ExitError)(0xc420bafca0), Timeout:false, outBuffer:(*cmd.lockedBuffer)(0xc4209ced80), errBuffer:(*cmd.lockedBuffer)(0xc4209cee10)} ("\nCommand: /go/src/github.com/docker/docker/bundles/1.13.0-dev/binary-client/docker run --sysctl net.ipv4.ip_forward=1 --name test busybox cat /proc/sys/net/ipv4/ip_forward\nExitCode: 125, Error: exit status 125\nStdout: \nStderr: sysctl \"net.ipv4.ip_forward\" is not in a separate kernel namespace\n/go/src/github.com/docker/docker/bundles/1.13.0-dev/binary-client/docker: Error response from daemon: oci runtime error: sysctl \"net.ipv4.ip_forward\" is not in a separate kernel namespace.\ntime=\"2016-10-25T14:08:40Z\" level=error msg=\"error getting events from daemon: net/http: request canceled\" \n\n")
14:08:40 ... expected cmd.Expected = cmd.Expected{ExitCode:0, Timeout:false, Error:"", Out:"", Err:""}
14:08:40 ... 
14:08:40 Command: /go/src/github.com/docker/docker/bundles/1.13.0-dev/binary-client/docker run --sysctl net.ipv4.ip_forward=1 --name test busybox cat /proc/sys/net/ipv4/ip_forward
14:08:40 ExitCode: 125, Error: exit status 125
14:08:40 Stdout: 
14:08:40 Stderr: sysctl "net.ipv4.ip_forward" is not in a separate kernel namespace
14:08:40 /go/src/github.com/docker/docker/bundles/1.13.0-dev/binary-client/docker: Error response from daemon: oci runtime error: sysctl "net.ipv4.ip_forward" is not in a separate kernel namespace.
14:08:40 time="2016-10-25T14:08:40Z" level=error msg="error getting events from daemon: net/http: request canceled" 
14:08:40 
14:08:40 
14:08:40 Failures:
14:08:40 ExitCode was 125 expected 0
14:08:40 Expected no error
14:08:40 
14:08:40 
14:08:40 

@@ -125,14 +125,36 @@ func (v *ConfigValidator) sysctl(config *configs.Config) error {
}
}
if strings.HasPrefix(s, "net.") {
if config.Namespaces.Contains(configs.NEWNET) {
continue
Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah, this is a bug. I'll make a PR to fix it.

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

Successfully merging this pull request may close these issues.

7 participants