Lookup docker-proxy in libexec paths

[cpuguy83]

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]

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

[thaJeztah]

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 🤔

[tianon]

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]

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]

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]

@thaJeztah The only change this is making is:

1. The init path may be loaded from */libexec/<name> in addition to other paths
2. 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]

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]

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]

Suggested change

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

👀

[thaJeztah]

This has the arguments flipped;

Suggested change

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

[thaJeztah]

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

[cpuguy83]

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]

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]

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

[cpuguy83]

Ping just want to understand what the concerns are with having a custom binary name, especially when it would be prefixed with docker- anyway.

[thaJeztah]

LGTM

[tianon]

We discussed a few things related to this in today's maintainer call, but ultimately came to the conclusion that they're all orthogonal to this change. 👍