Lookup docker-proxy in libexec paths #47804

cpuguy83 · 2024-05-07T22:00:38Z

This allows distros to put docker-proxy under libexec paths as is done for docker-init.

Also expands the lookup to to not require a docker/ subdir in libexec subdir.
Since it is a generic helper that may be used for something else in the future, this is only done for binaries with a docker-.

thaJeztah

OHMAN... I went down the rabbit hole on this one 🙈

We may need to have a closer look to see what potential consequences / impact is, taking into consideration that;

we have various types of helper binaries that uses docker- prefixes for selection ("init", "userland-proxy", "cli-plugins", "credential helpers")
for some of those, we have some stricter defined locations to look for (/usr/libexec/docker/cli-plugins/) to prevent potential mis-matches (and maybe for performance reasons to some extent?)
in light of some recent changes w.r.t. AppArmor; may any of these binaries require an AppArmor profile to be applied (and if so, will those be based on their path/location)?

At least, it looks like some of this is undocumented behavior; for both the init binary and the userland-proxy, the flag description defines that it must specify the path. I somewhat wonder if "looking up" the binary was intentional, or an oversight;

--init-path string                      Path to the docker-init binary
--userland-proxy-path string            Path to the userland proxy binary

The example daemon.json in the documentation also uses an absolute path for both, which somewhat indicates that it's expected to be a full-path;

  "init-path": "/usr/libexec/docker-init",
  "userland-proxy-path": "/usr/libexec/docker-proxy",

Rather ironically, it looks like the default location we use for the docker-init binary is /usr/bin/docker-init, which means that specifying docker-init (without path) would fail to resolve. If we consider that binary not intended to be executed directly by users or shell scripts, then.. should we change that location?

Looking at the history of that ``, I see it was added in 6caaa8c (#45198), and I think there's some hairy parts there;

ee3ac3a (Add init process for zombie fighting and signal handling #26061) introduced the --init option. Originally, it was not configurable, and (out of convenience?) we looked up the docker-init binary for every container
6a12685 (configure docker-init binary path #26941) made the path configurable in both the daemon config, and per container.
- It looks like the INTENT was to allow configuring the full path to the binary to use
- But the daemon config did NOT validate if it was a full path, fully depending on Daemon.populateCommonSpec() to only lookup the path if it's empty (and otherwise fail if it was a relative path)?
- Daemon.populateCommonSpec() would ONLY do a exec.LookPath() if the daemon did not have a config set, and if the container's HostConfig didn't have a path set
a18d103 (remove --init-path from client #32470) removed the per-container config for the path
2790ac6 (Add external binaries version to docker info #27955) is where things started to become blurry;
- This PR added version information for the docker-init binary to docker info
- But it only used the default (docker-init) binary, and didn't take custom init into account; it also unconditionally looked up the binary (exec.Command("docker-init", "--version"))
17b1288 (Fix missing Init Binary in docker info output #32469) is where we went off the rails;
- this PR fixed docker info not taking the configured init into account
- but unconditionally kep the exec.Command(), so now "allowing" the custom init path to be relative
- it also codified this change in behavior by adding a test (TestCommonUnixGetInitPath). The test only covered the daemon config so things would (likely) still fail when trying to use, but all test-cases use a relative path (binary name only)
6caaa8c (Prefer loading docker-init from an appropriate "libexec" directory #45198) improved looking up the binary, to not use a blanket exec.LookPath, and instead prefer "libexec" paths
- ☝️ probably we should've updated the default location of the binary (/usr/bin/docker-init) as well in our packaging for this, but we didn't
- ☝️ it introduced config.LookupInitPath(), which internally use the config.GetInitPath() 👈 this becomes relevant!
- ☝️ ⚠️ because it also changed WithCommonOptions() to now use daemon.configStore.LookupInitPath(); previously, this would ONLY lookup the binary if the daemon config was empty (so ONLY lookup the default docker-init binary)
- ☝️ but now, it would unconditionally lookup the binary, and due to the missing validation for --init-path in the daemon config ("must be absolute"), we now changed behavior, and allowed for a non-absolute path to be specified.

I realize this is for docker-init, not for docker-userland-proxy, but we may have to check that one as well 🙈

thaJeztah · 2024-05-08T07:59:14Z

daemon/config/config_linux.go

+	if filepath.IsAbs(binary) {
+		return binary, nil
+	}


This is already handled by lookupBinPath, so looks like this check is redundant

thaJeztah · 2024-05-08T08:20:16Z

daemon/config/config_linux.go

+	// According to FHS, it is not necessary to have a subdir here.
+	// If the binary has a `docker-` prefix, let's look it up without the prefix.
+	if strings.HasPrefix("docker-", binary) {


Hm... honestly, it didn't occur to me that we currently allow for a non-absolute path (e.g. just hello) to be specified, and that to be resolved to (e.g.) /usr/local/libexec/docker/hello.

Nothing wrong with this code (I think) but now I'm wondering what we document / define for this, and if that's documented behavior. Basically, curious if we could make it;

either specify full path

or specify a docker- prefixed binary

and to not allow for the more "fuzzy" approach?

One thing that could be a potential risk is for this to be matching CLI plugins or credentials helpers;

for CLI plugins we currently use (e.g.) /usr/libexec/docker/cli-plugins/docker-buildx as one of the locations, so currently probably not an issue, but it would mean we won't be able to do the same for CLI plugins (i.e., move them to /usr/libexec)

for credentials-helpers, I think we use (e.,g.) /usr/bin/docker-credential-secretservice - TBH also wondering if such helpers should be libexec 🤔

This is only for libexec, which I think is the important mitigating factor -- effectively, this check is to ensure that we're not encouraging rampant/unnamespaced pollution of /usr/libexec directly by forcing either specifying an exact path or installing the binaries with an appropriate prefix or within an appropriate subdirectory.

(This is effectively to mirror the /usr/libexec/docker path but without the subdirectory, which according to the FHS is ~reasonable.)

thaJeztah · 2024-05-08T09:49:54Z

Looks like a good example of the Swiss cheese model, and it looks like we changed behavior (at least for docker-init) without realizing, due to missing validation in some steps, and existing code also doing the wrong thing, which we started to use in other parts of the stack...

thaJeztah · 2024-05-08T09:56:27Z

Also, I should mention that I'm not "for" or "against" one or the other, but at least it looks like the current behavior (before this change already) does not match the original intent, so there's undocumented behavior. (So the "positive" is that it's undocumented, so we can still correct if we decide to).

Regardless what we decide on, we should properly investigate potential consequences. (Do we paint ourselves into a corner?). I also have "somewhat" in mind the remaining decisions to be made on renaming things (docker -> moby) and if that means we may need to have different search patterns (more flexible lookups may mean even more patterns to search for?).

Finally; I really wonder if the current approach to look up the binary when creating the container is correct; both from a performance perspective (lookup for every container, although this may not be "too" heavy), as well as (not sure?) do we want to look up the docker-init binary at runtime, or should this be done once (on daemon startup)?

cpuguy83 · 2024-05-08T15:56:34Z

@thaJeztah The only change this is making is:

The init path may be loaded from */libexec/<name> in addition to other paths
When the proxy path is not set in the config, the default value may now be looked up in */libexec/<name> and */libexec/docker/<name>

When a proxy path is specified int he config, this still requires it to be absolute path.

As far as requiring users to specify an absolute path for the init path, I'm not sure its worth potentially breaking people, but we can certainly make that change.
Consider that these paths are all going to be gated by root access on people's systems.

I don't think this changes anything wrt docker -> moby naming, we'll still have to lookup docker-<bin> unless we decide to break people's systems, in which case we are breaking things anyway.

I don't understand the concern with looking up incorrect binary names (though I know there is a docker init plugin which is confusing AF).

tianon

LGTM (sans a few minor comment nits and Seb's "duplicate IsAbs check" note)

tianon · 2024-05-09T20:38:28Z

daemon/config/config_linux.go

-	} {
+	}
+
+	// According to FHS, it is not necessary to have a subdir here.


Suggested change

// According to FHS, it is not necessary to have a subdir here.

// According to FHS 3.0, it is not necessary to have a subdir here (see note and reference above).

tianon · 2024-05-09T20:42:15Z

daemon/config/config_linux.go

+	// According to FHS, it is not necessary to have a subdir here.
+	// If the binary has a `docker-` prefix, let's look it up without the prefix.
+	if strings.HasPrefix("docker-", binary) {


This is only for libexec, which I think is the important mitigating factor -- effectively, this check is to ensure that we're not encouraging rampant/unnamespaced pollution of /usr/libexec directly by forcing either specifying an exact path or installing the binaries with an appropriate prefix or within an appropriate subdirectory.

(This is effectively to mirror the /usr/libexec/docker path but without the subdirectory, which according to the FHS is ~reasonable.)

tianon · 2024-05-09T20:45:00Z

daemon/config/config_linux.go

-// LookupInitPath returns an absolute path to the "docker-init" binary by searching relevant "libexec" directories (per FHS 3.0 & 2.3) followed by PATH
-func (conf *Config) LookupInitPath() (string, error) {
-	binary := conf.GetInitPath()
+func lookupBinPath(binary string) (string, error) {


Even though it's a private function, IMO we should copy most of the LookupInitPath godoc so the intent is still clear; maybe something like: 😇

Suggested change

func lookupBinPath(binary string) (string, error) {

// lookupBinPath returns an absolute path to the provided binary by searching relevant "libexec" locations (per FHS 3.0 & 2.3) followed by PATH

func lookupBinPath(binary string) (string, error) {

tianon · 2024-05-09T22:48:09Z

daemon/config/config_linux.go

+	binary := conf.GetInitPath()
+	if filepath.IsAbs(binary) {
+		return binary, nil
+	}
+
+	return lookupBinPath(binary)


Suggested change

binary := conf.GetInitPath()

if filepath.IsAbs(binary) {

return binary, nil

}

return lookupBinPath(binary)

return lookupBinPath(conf.GetInitPath())

👀

thaJeztah · 2024-05-10T12:58:14Z

daemon/config/config_linux.go

+
+	// According to FHS 3.0, it is not necessary to have a subdir here (see note and reference above).
+	// If the binary has a `docker-` prefix, let's look it up without the dir prefix.
+	if strings.HasPrefix("docker-", binary) {


This has the arguments flipped;

Suggested change

if strings.HasPrefix("docker-", binary) {

if strings.HasPrefix(binary, "docker-") {

thaJeztah · 2024-05-10T13:19:46Z

daemon/config/config_linux.go

@@ -136,6 +145,16 @@ func (conf *Config) LookupInitPath() (string, error) {
 	return exec.LookPath(binary)


This fallback means we will look in $PATH, so this would also include, e.g. docker-compose 🤔 😂

cp /usr/libexec/docker/cli-plugins/docker-compose /usr/local/bin dockerd --init-path=docker-compose ... WARN[2024-05-10T13:16:18.581826096Z] failed to parse /usr/local/bin/docker-compose version error="unknown output format: Docker Compose version v2.25.0\n" docker info | grep Init Init Binary: docker-hello docker run -dit --init busybox dac20108102f70578ed24d0ba5952bb874130d14882952e37dfbfb8bc19b4e5a docker ps -a CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES dac20108102f busybox "sh" 59 seconds ago Exited (16) 58 seconds ago relaxed_boyd

Sure, but they could also have done --init-path=/usr/libexec/docker/cli-plugins/docker-compose.
I'm not understanding the concern here. We aren't resolving the wrong binaries except that the user potentially put in the wrong binary.

thaJeztah · 2024-05-10T13:20:15Z

When a proxy path is specified int he config, this still requires it to be absolute path.

Correct, this PR is not changing the absolute path, but it looks like that already happened in 6caaa8c.

That changed oci_linux.go to use LookupInitPath()

moby/daemon/oci_linux.go

Line 748 in cd08d37

    
           path, err := daemonCfg.LookupInitPath() // this will fall back to DefaultInitBinary and return an absolute path

Which uses GetInitPath internally (which can be either user-configured OR the default), and regardless where that came from will lookup the binary if it's not an absolute path

moby/daemon/config/config_linux.go

Lines 111 to 116 in cd08d37

    
           // LookupInitPath returns an absolute path to the "docker-init" binary by searching relevant "libexec" directories (per FHS 3.0 & 2.3) followed by PATH 
        
           func (conf *Config) LookupInitPath() (string, error) { 
        
           	binary := conf.GetInitPath() 
        
           	if filepath.IsAbs(binary) { 
        
           		return binary, nil 
        
           	}

Before that PR, it would only lookup if the value is empty, so only for the default;

moby/daemon/oci_linux.go

Lines 751 to 753 in 1855a55

    
           path := daemon.configStore.InitPath 
        
           if path == "" { 
        
           	path, err = exec.LookPath(dconfig.DefaultInitBinary)

cp /usr/local/bin/docker-init /usr/local/libexec/docker/hello
dockerd --init-path=hello

docker info | grep Init
 Init Binary: hello

docker run -dit --init busybox
e46854e0029156cdba0ec2d0ec8c9ba2df54da30abd86897fa63335e8a7492e2

ctr -n moby c info e46854e0029156cdba0ec2d0ec8c9ba2df54da30abd86897fa63335e8a7492e2 | grep hello
                "source": "/usr/local/libexec/docker/hello",

And with this PR, the above also works with /usr/local/libexec (if the init has a docker- prefix);

mv /usr/local/libexec/docker/hello /usr/local/libexec/docker-hello

dockerd --init-path=docker-hello

docker info | grep Init
 Init Binary: docker-hello

docker run -dit --init busybox
d001ed2baeed3b0fe1f1c83f56bcd3ec4ebeaad050f9192b28d85376e6e46694

ctr -n moby c info d001ed2baeed3b0fe1f1c83f56bcd3ec4ebeaad050f9192b28d85376e6e46694 | grep hello
                "source": "/usr/local/libexec/docker-hello",

cpuguy83 · 2024-05-10T15:53:08Z

@thaJeztah I get that it was changed, I'm just not sure I understand what the issue is with it.

This allows distros to put docker-proxy under libexec paths as is done for docker-init. Also expands the lookup to to not require a `docker/` subdir in libexec subdir. Since it is a generic helper that may be used for something else in the future, this is only done for binaries with a `docker-`. Signed-off-by: Brian Goff <cpuguy83@gmail.com>

cpuguy83 requested review from tianon and thaJeztah May 7, 2024 22:00

cpuguy83 mentioned this pull request May 7, 2024

moby-engine package should not include /etc/docker/daemon.json microsoft/azurelinux#8961

Open

cpuguy83 force-pushed the more_paths_docker_proxy branch 2 times, most recently from f644c58 to 3a17c7b Compare May 7, 2024 22:35

thaJeztah requested changes May 8, 2024

View reviewed changes

tianon approved these changes May 9, 2024

View reviewed changes

cpuguy83 force-pushed the more_paths_docker_proxy branch from 3a17c7b to c1274dd Compare May 9, 2024 21:11

tianon reviewed May 9, 2024

View reviewed changes

thaJeztah reviewed May 10, 2024

View reviewed changes

cpuguy83 force-pushed the more_paths_docker_proxy branch from c1274dd to 45eb733 Compare May 10, 2024 16:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lookup docker-proxy in libexec paths #47804

Lookup docker-proxy in libexec paths #47804

cpuguy83 commented May 7, 2024

thaJeztah left a comment

thaJeztah May 8, 2024

thaJeztah May 8, 2024

tianon May 9, 2024

thaJeztah commented May 8, 2024

thaJeztah commented May 8, 2024

cpuguy83 commented May 8, 2024

tianon left a comment

tianon May 9, 2024

tianon May 9, 2024

tianon May 9, 2024

tianon May 9, 2024

thaJeztah May 10, 2024

thaJeztah May 10, 2024

cpuguy83 May 10, 2024

thaJeztah commented May 10, 2024

cpuguy83 commented May 10, 2024

	// According to FHS, it is not necessary to have a subdir here.
	// According to FHS 3.0, it is not necessary to have a subdir here (see note and reference above).

	func lookupBinPath(binary string) (string, error) {
	// lookupBinPath returns an absolute path to the provided binary by searching relevant "libexec" locations (per FHS 3.0 & 2.3) followed by PATH
	func lookupBinPath(binary string) (string, error) {

	if strings.HasPrefix("docker-", binary) {
	if strings.HasPrefix(binary, "docker-") {

		@@ -136,6 +145,16 @@ func (conf *Config) LookupInitPath() (string, error) {
		return exec.LookPath(binary)

Lookup docker-proxy in libexec paths #47804

Are you sure you want to change the base?

Lookup docker-proxy in libexec paths #47804

Conversation

cpuguy83 commented May 7, 2024

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented May 8, 2024

thaJeztah commented May 8, 2024

cpuguy83 commented May 8, 2024

tianon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented May 10, 2024

cpuguy83 commented May 10, 2024