allow propagating custom exec-root (e.g. "/run/docker") to libnetwork-setkey by AkihiroSuda · Pull Request #2248 · moby/libnetwork

AkihiroSuda · 2018-08-11T12:31:39Z

part of moby/moby#37375 (in rootless mode, exec-root should be like /run/user/4242/docker rather than /run/docker)

The docker daemon needs to be modified as follows:

diff --git a/daemon/oci_linux.go b/daemon/oci_linux.go
index 00ace320df..ea7daa72df 100644
--- a/daemon/oci_linux.go
+++ b/daemon/oci_linux.go
@@ -809,7 +809,7 @@ func (daemon *Daemon) createSpec(c *container.Container) (retSpec *specs.Spec, e
                    s.Hooks = &specs.Hooks{
                            Prestart: []specs.Hook{{
                                    Path: target,
-                                   Args: []string{"libnetwork-setkey", c.ID, daemon.netController.ID()},
+                                   Args: []string{"libnetwork-setkey", c.ID, daemon.netController.ID(), "-exec-root="+daemon.configStore.GetExecRoot()},
                            }},
                    }
            }

Signed-off-by: Akihiro Suda suda.akihiro@lab.ntt.co.jp

AkihiroSuda · 2018-08-15T02:06:42Z

@fcrisciani @ctelfer PTAL?

selansen · 2018-08-15T03:08:03Z

sandbox_externalkey_unix.go

 	"io/ioutil"
 	"net"
 	"os"
+	"path/filepath"


Can you run goimport ? that will keep import lines in alphabetical order.

Isnt this already alphabetical?

Agree.. it's fine. goimports wouldn't group it differently and alphabetizes based on the whole string, not just the last part of the path.

got carried away with last name filepath :)

ctelfer

Sorry for the delay. Actually looked at it last week, but wanted to give it a second look.

Overall: LGTM

I have some style suggestions (see comments) that could tighten it up a bit. But I'm not stuck on them. @fcrisciani is unavailable for a few weeks. Maybe @abhi or @mavenugo could give a look? I'd like to hear if they are good with the way that the execRoot is getting propagated. Again, I'm ok with it, but someone with more history may know something I'm not considering.

ctelfer · 2018-08-15T17:13:53Z

sandbox_externalkey_unix.go

+		execRoot = v
+	}
+	udsBase := filepath.Join(execRoot, "libnetwork")
+	c, err := net.Dial("unix", filepath.Join(udsBase, controllerID+".sock"))


maybe skip udsBase entirely and just do:

uds := filepath.Join(execRoot, "libnetwork", controllerID+".sock") c, err := net.Dial("unix", uds)

?

I guess if we want to be even cleaner we could do it as:

const execSubdir = "libnetwork"

above and then

uds := filepath.Join(execRoot, execSubdir, controllerID+".sock") c, err := net.Dial("unix", uds)

thanks, updated

ctelfer · 2018-08-15T17:13:59Z

sandbox_externalkey_unix.go

 	}
-	uds := udsBase + c.id + ".sock"
+	uds := filepath.Join(udsBase, c.id+".sock")
 	l, err := net.Listen("unix", uds)


Similar: maybe just

uds := filepath.Join(execRoot, execSubdir, c.id+".sock")

?

MkdirAll(udsBase) is needed

Ah, right indeed.

AkihiroSuda · 2018-08-16T11:17:11Z

sandbox_externalkey_unix.go


-	c, err := net.Dial("unix", udsBase+controllerID+".sock")
+	execRoot := "/run/docker"
+	if v := os.Getenv("_LIBNETWORK_EXEC_ROOT"); v != "" {


On second thought, maybe the underscore prefix is not needed?

the original intention was to make sure this variable should not be set by human users

Agree that the '_' is not necessary. Doubt people will accidentally prefix an environment variable with LIBNETWORK_.

removed the underscore prefix

AkihiroSuda · 2018-08-24T05:11:31Z

@ctelfer @selansen PTAL?

ctelfer · 2018-08-24T14:07:07Z

@fcrisciani will be back next week. Would still like someone w/ more history in how Moby things are done to comment re: using the env variables to pass this info. So, this is for "rootless" containers right? (which I'm pretty excited to see happen) If the environment variable is always set by moby, (as your proposed moby patch would indicate), then if someone set it to try to hijack the path, the injected path would always get clobbered. So, should be safe. Hrm.... Would it be better or cleaner, perhaps, to pass the root directory as an optional 3rd parameter (4th in argument list) to "libnetwork-setkey" / processSetKeyReexec(). (or maybe even make it mandetory) Honestly asking, not sure...

ctelfer · 2018-08-24T14:18:32Z

Realized we definitely don't want a mandatory extra argument because that would break libnetwork integration w/ moby until moby accepted the moby change. Optional would still work, though. Passing via env does avoid changing the _windows.go code. (not that the change would be big... just another parameter to ignore) @abhi WDYT?

AkihiroSuda · 2018-08-24T15:31:52Z

So, this is for "rootless" containers right?

Yes, but should be useful for "rootful" dockerd --exec-root as well

if someone set it to try to hijack the path,

Is there any attack flow?

ctelfer · 2018-08-24T16:39:42Z

My apologies for not being clear. I was saying that I don't think there is an attack path. Once moby is upgraded there is no attack flow for certain. Your proposed moby patch always sets (overrides) the environment variable so there's no way for some pre-set value of the environment variable to redirect it.

It just now occurred to me that there could be an attack flow if we merged the libnetwork change, and then vendored libnetwork into moby (say, because we want some other libnetwork feature/fix) before moby accepted the update to set LIBNETWORK_EXEC_ROOT. In that case, if someone could arrange to set LIBNETWORK_EXEC_ROOT in the dockerd environment (while it was running as root), then they could redirect the exec root to a directory location of their choosing.

This is probably not a real issue because injecting arbitrary environment variables into the dockerd probably allows one to do much worse things and would usually require significant privilege in the first place.

AkihiroSuda · 2018-08-24T16:42:20Z

if someone could arrange to set LIBNETWORK_EXEC_ROOT in the dockerd environment

Is specs.Hook.Env automatically propagated from the parent os.Environ?

ctelfer · 2018-08-24T17:23:59Z

That I couldn't say without looking. But what I was thinking was that when moby goes and rexecs itself, it probably propagates its environment variables. If LIBNETWORK_EXEC_ROOT were somehow set, that would propagate to the processSetKeyReexec(). Again, this is premised on some scenario where this patch is merged into libnetwork, vendored into moby and for some reason the moby patch to set LIBNETWORK_EXEC_ROOT in daemon/oci_linux.go is not yet merged. (So, this is a bit of a contrived scenario. Am not saying we should block this patch because of it. Just a thought experiment.)

selansen · 2018-09-04T21:50:58Z

sandbox_externalkey_unix.go

 		Key:         key}

-	c, err := net.Dial("unix", udsBase+controllerID+".sock")
+	execRoot := "/run/docker"


can we make it as defultExecRoot constant variable instead of using like this ?

selansen · 2018-09-04T21:51:46Z

sandbox_externalkey_unix.go

 }

 func (c *controller) startExternalKeyListener() error {
+	execRoot := "/run/docker"


same as above comment. we can use constant

selansen · 2018-09-04T21:54:06Z

LGTM. Pls see if you can address one review comment that I have added.

AkihiroSuda · 2018-09-05T14:15:39Z

done

selansen · 2018-09-05T17:00:24Z

Thanks. LGTM

AkihiroSuda · 2018-09-11T06:00:23Z

@ctelfer LGTY? ^^

ctelfer · 2018-09-13T20:46:57Z

Alright. Sorry this has taken so long. Have had trouble finding others w/ history on the integration side of this. Discussed this w/ @thaJeztah w.r.t. the moby integration and he said he definitely preferred not to pass that info around in an environment variable. So could we please consider passing that path as an argument instead?

I can think of two ways this could work:

add optional argument [3] that contains the exec-root path.
- pros: simplest, newer libnetwork works backwards w/ older moby until moby change gets merged
- cons: argument [3] becomes mandatory if we ever need to add more arguments to setkey (not that it has been touched in years)
make arguments [3], [4], ... flag lists like -e EXEC-ROOT -f FOO -b BAR
- pros: extensible, future proof
- cons: lots more code for a use case we may never have
Do option 1, but ALSO have treat some special argument like '--default-exec-root' just mean to use the default exec-root if present.
- pros: pretty simple, still works backwards compatibly, could be turned into full option-parsing in the future if/when we need to add new optional flags/arguments
- cons: a bit less pretty

Anyway, sorry to stretch this out. What seems best from your standpoint, given that passing by environment would be frowned upon? Maybe you have alternative approaches? Thanks!

AkihiroSuda · 2018-09-14T04:58:18Z

Added -exec-root flag

…-setkey The docker daemon needs to be modified as follows: diff --git a/daemon/oci_linux.go b/daemon/oci_linux.go index 00ace320df..ea7daa72df 100644 --- a/daemon/oci_linux.go +++ b/daemon/oci_linux.go @@ -809,7 +809,7 @@ func (daemon *Daemon) createSpec(c *container.Container) (retSpec *specs.Spec, e s.Hooks = &specs.Hooks{ Prestart: []specs.Hook{{ Path: target, - Args: []string{"libnetwork-setkey", c.ID, daemon.netController.ID()}, + Args: []string{"libnetwork-setkey", c.ID, daemon.netController.ID(), "-exec-root="+daemon.configStore.GetExecRoot()}, }}, } } Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

selansen · 2018-09-14T13:05:22Z

Thanks @AkihiroSuda . LGTM

ctelfer

LGTM

ctelfer · 2018-09-14T14:19:59Z

Thanks again for your patience @AkihiroSuda ! Am looking forward to seeing this go into moby!

thaJeztah · 2018-09-14T15:35:58Z

Thanks!

AkihiroSuda · 2018-09-14T15:46:21Z

thanks, opened moby/moby#37850

selansen reviewed Aug 15, 2018

View reviewed changes

ctelfer approved these changes Aug 15, 2018

View reviewed changes

AkihiroSuda force-pushed the propagte-exec-root branch 2 times, most recently from d8c55c0 to 9ac90d5 Compare August 16, 2018 11:01

AkihiroSuda commented Aug 16, 2018

View reviewed changes

AkihiroSuda force-pushed the propagte-exec-root branch from 9ac90d5 to e42a8d9 Compare August 24, 2018 13:57

selansen reviewed Sep 4, 2018

View reviewed changes

AkihiroSuda force-pushed the propagte-exec-root branch from e42a8d9 to 75565f3 Compare September 5, 2018 14:15

AkihiroSuda force-pushed the propagte-exec-root branch from 75565f3 to 3c86b73 Compare September 14, 2018 04:57

AkihiroSuda force-pushed the propagte-exec-root branch from 3c86b73 to 03bbfad Compare September 14, 2018 05:09

ctelfer approved these changes Sep 14, 2018

View reviewed changes

ctelfer merged commit 20461b8 into moby:master Sep 14, 2018

AkihiroSuda mentioned this pull request Sep 14, 2018

daemon: propagate exec-root to libnetwork-setkey moby/moby#37850

Merged

Conversation

AkihiroSuda commented Aug 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkihiroSuda commented Aug 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ctelfer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Aug 24, 2018

Uh oh!

ctelfer commented Aug 24, 2018

Uh oh!

ctelfer commented Aug 24, 2018

Uh oh!

AkihiroSuda commented Aug 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ctelfer commented Aug 24, 2018

Uh oh!

AkihiroSuda commented Aug 24, 2018

Uh oh!

ctelfer commented Aug 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

selansen commented Sep 4, 2018

Uh oh!

AkihiroSuda commented Sep 5, 2018

Uh oh!

selansen commented Sep 5, 2018

Uh oh!

AkihiroSuda commented Sep 11, 2018

Uh oh!

ctelfer commented Sep 13, 2018

Uh oh!

AkihiroSuda commented Sep 14, 2018

Uh oh!

selansen commented Sep 14, 2018

Uh oh!

ctelfer left a comment

Choose a reason for hiding this comment

Uh oh!

ctelfer commented Sep 14, 2018

Uh oh!

thaJeztah commented Sep 14, 2018

Uh oh!

AkihiroSuda commented Sep 14, 2018

Uh oh!

Reviewers

Assignees

Labels

AkihiroSuda commented Aug 11, 2018 •

edited

Loading

AkihiroSuda commented Aug 24, 2018 •

edited

Loading