docker exec: fail early on exec create if specified user doesn't exist #49868
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thaJeztah
added
status/2-code-review
impact/changelog
kind/bugfix
Before this patch, and error would be produced when starting the exec,
but the CLI would wait for the exec to complete, timing out after 10
seconds (default). With this change, an error is returned immediately
when creating the exec.
Note that "technically" this check may have some TOCTOU issues, because
'/etc/passwd' and '/etc/groups' may be mutated by the container in between
creating the exec and starting it.
This is very likely a corner-case, but something we can consider changing
in future (either allow creating an invalid exec, and checking before
starting, or checking both before create and before start).
With this patch:
printf 'FROM alpine\nRUN rm -f /etc/group' | docker build -t nogroup -
ID=$(docker run -dit nogroup)
time docker exec -u 0:root $ID echo hello
Error response from daemon: unable to find group root: no matching entries in group file
real 0m0.014s
user 0m0.010s
sys 0m0.003s
# numericc uid/gid (should not require lookup);
time docker exec -u 0:0 $ID echo hello
hello
real 0m0.059s
user 0m0.007s
sys 0m0.008s
# no user specified (should not require lookup);
time docker exec $ID echo hello
hello
real 0m0.057s
user 0m0.013s
sys 0m0.008s
docker rm -fv $ID
# container that does have a valid /etc/groups
ID=$(docker run -dit alpine)
time docker exec -u 0:root $ID echo hello
hello
real 0m0.063s
user 0m0.010s
sys 0m0.009s
# non-existing user or group
time docker exec -u 0:blabla $ID echo hello
Error response from daemon: unable to find group blabla: no matching entries in group file
real 0m0.013s
user 0m0.004s
sys 0m0.009s
docker rm -fv $ID
Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
exec_validate_user
branch
from
a4c1169 to
b54a038
Compare
thaJeztah
commented
Apr 28, 2025
Comment on lines
+285
to
+290
| ctrOpts := []func(*container.TestContainerConfig){ | ||
| container.WithTty(true), | ||
| container.WithUser("1:1"), | ||
| } | ||
| withoutEtcGroups := container.WithImage(build.Do(ctx, t, apiClient, fakecontext.New(t, "", fakecontext.WithDockerfile("FROM busybox\nRUN rm /etc/group")))) | ||
| withoutEtcPasswd := container.WithImage(build.Do(ctx, t, apiClient, fakecontext.New(t, "", fakecontext.WithDockerfile("FROM busybox\nRUN rm /etc/passwd")))) |
There was a problem hiding this comment.
I should point out that I had some fun with this part; if I tried to make these tests run without building an image (so running a container with sh -c 'rm /etc/passwd && top' as command), the test didn't fail as expected. I tried adding a sync in between to see if it was possibly that changes weren't written to disk, but that didn't work either. So ... something odd.
Trying to run the same scenario manually didn't reproduce that issue.
cid=$(docker run -dit --name foo busybox sh -c 'rm /etc/passwd && top') \
&& docker exec -it -u root:root $cid echo hello
# Error response from daemon: unable to find user root: no matching entries in passwd fileSo either it's something in how the tests runs, or there's some actual race somewhere. I decided not to continue looking into that, because failing to detect the problem would still cause the actual exec to fail (the new check is just an extra check), but perhaps something to try to find what's happening.
|
@dmcgowan ptal ❤️ |
dmcgowan
approved these changes
May 9, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before this patch, and error would be produced when starting the exec, but the CLI would wait for the exec to complete, timing out after 10 seconds (default). With this change, an error is returned immediately when creating the exec.
Note that "technically" this check may have some TOCTOU issues, because '/etc/passwd' and '/etc/groups' may be mutated by the container in between creating the exec and starting it.
This is very likely a corner-case, but something we can consider changing in future (either allow creating an invalid exec, and checking before starting, or checking both before create and before start).
With this patch:
- What I did
- How I did it
- How to verify it
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)