-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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
Lookup docker-proxy in libexec paths #47804
base: master
Are you sure you want to change the base?
Conversation
f644c58
to
3a17c7b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thedocker-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 aexec.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 todocker 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")
)
- This PR added version information for the
- 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 configuredinit
into account - but unconditionally kep the
exec.Command()
, so now "allowing" the custominit
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)
- this PR fixed
- 6caaa8c (Prefer loading
docker-init
from an appropriate "libexec" directory #45198) improved looking up the binary, to not use a blanketexec.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 theconfig.GetInitPath()
👈 this becomes relevant! - ☝️
⚠️ because it also changedWithCommonOptions()
to now usedaemon.configStore.LookupInitPath()
; previously, this would ONLY lookup the binary if the daemon config was empty (so ONLY lookup the defaultdocker-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.
- ☝️ probably we should've updated the default location of the binary (
I realize this is for docker-init
, not for docker-userland-proxy
, but we may have to check that one as well 🙈
daemon/config/config_linux.go
Outdated
if filepath.IsAbs(binary) { | ||
return binary, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already handled by lookupBinPath
, so looks like this check is redundant
daemon/config/config_linux.go
Outdated
// 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 belibexec
🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
Looks like a good example of the Swiss cheese model, and it looks like we changed behavior (at least for |
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 |
@thaJeztah The only change this is making is:
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. I don't think this changes anything wrt docker -> moby naming, we'll still have to lookup I don't understand the concern with looking up incorrect binary names (though I know there is a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (sans a few minor comment nits and Seb's "duplicate IsAbs
check" note)
daemon/config/config_linux.go
Outdated
} { | ||
} | ||
|
||
// According to FHS, it is not necessary to have a subdir here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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). |
daemon/config/config_linux.go
Outdated
// 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
// 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: 😇
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) { |
3a17c7b
to
c1274dd
Compare
daemon/config/config_linux.go
Outdated
binary := conf.GetInitPath() | ||
if filepath.IsAbs(binary) { | ||
return binary, nil | ||
} | ||
|
||
return lookupBinPath(binary) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
binary := conf.GetInitPath() | |
if filepath.IsAbs(binary) { | |
return binary, nil | |
} | |
return lookupBinPath(binary) | |
return lookupBinPath(conf.GetInitPath()) |
👀
|
||
// 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has the arguments flipped;
if strings.HasPrefix("docker-", binary) { | |
if strings.HasPrefix(binary, "docker-") { |
@@ -136,6 +145,16 @@ func (conf *Config) LookupInitPath() (string, error) { | |||
return exec.LookPath(binary) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Correct, this PR is not changing the absolute path, but it looks like that already happened in 6caaa8c. That changed Line 748 in cd08d37
Which uses moby/daemon/config/config_linux.go Lines 111 to 116 in cd08d37
Before that PR, it would only lookup if the value is empty, so only for the default; Lines 751 to 753 in 1855a55
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 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", |
@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>
c1274dd
to
45eb733
Compare
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-
.