daemon.WithCommonOptions() fix detection of user-namespaces#42736
Merged
AkihiroSuda merged 1 commit intomoby:masterfrom Sep 24, 2021
Merged
daemon.WithCommonOptions() fix detection of user-namespaces#42736AkihiroSuda merged 1 commit intomoby:masterfrom
AkihiroSuda merged 1 commit intomoby:masterfrom
Conversation
thaJeztah
added
status/2-code-review
impact/changelog
kind/bugfix
Member
Author
|
ping @justincormack @AkihiroSuda @tianon ptal |
Member
Author
|
Looks like it's missing detection for rootless now |
justincormack
approved these changes
Aug 12, 2021
thaJeztah
force-pushed
the
cap_net_raw_usens_detection
branch
from
021f848 to
ba60f2a
Compare
Member
Author
|
Rootless looks ok now, but hitting a flaky test. Opened a ticket for that: #42739 |
Member
Author
|
Another flaky: |
Member
Author
|
CI is green; I was trying to test some things, but ... Interesting; Busybox: docker run --rm -u 1000:1000 busybox ping -c1 example.com
ping: permission denied (are you root?)
PING example.com (93.184.216.34): 56 data bytes
docker run --rm busybox ls -la /bin/ping
-rwxr-xr-x 400 root root 1149184 Jun 7 17:34 /bin/pingAlpine: docker run --rm -u 1000:1000 alpine ping -c1 example.com
PING example.com (93.184.216.34): 56 data bytes
64 bytes from 93.184.216.34: seq=0 ttl=42 time=93.436 ms
--- example.com ping statistics ---
1 packets transmitted, 1 packets received, 0% packet loss
round-trip min/avg/max = 93.436/93.436/93.436 ms
docker run --rm alpine ls -la /bin/ping
lrwxrwxrwx 1 root root 12 Aug 5 12:25 /bin/ping -> /bin/busyboxSo the alpine image uses busybox for |
Member
Author
|
Note that on docker 20.10.8, I get the same for both; docker run --rm -u 1000:1000 busybox ping -c1 example.com
PING example.com (93.184.216.34): 56 data bytes
ping: permission denied (are you root?)
docker run --rm -u 1000:1000 alpine ping -c1 example.com
ping: permission denied (are you root?)
PING example.com (93.184.216.34): 56 data bytesVersions of busybox are the same in both: docker run --rm -u 1000:1000 alpine sh -c 'busybox | head -n1'
BusyBox v1.33.1 () multi-call binary.
docker run --rm -u 1000:1000 busybox sh -c 'busybox | head -n1'
BusyBox v1.33.1 (2021-06-07 17:33:50 UTC) multi-call binary. |
Member
Author
|
Tianon found this; apparently by design; https://git.alpinelinux.org/aports/tree/main/busybox/0006-ping-make-ping-work-without-root-privileges.patch?h=3.14-stable And that patch was rejected by the busybox maintainers; |
Member
Author
|
With this patch, and |
Member
Author
|
@AkihiroSuda PTAL |
1 similar comment
Member
Author
|
@AkihiroSuda PTAL |
AkihiroSuda
approved these changes
Aug 26, 2021
Member
AkihiroSuda
left a comment
AkihiroSuda
left a comment
There was a problem hiding this comment.
LGTM, but would like to see an integration test too if possible
thaJeztah
force-pushed
the
cap_net_raw_usens_detection
branch
2 times, most recently
from
076d48d to
2b826b7
Compare
Commit dae652e added support for non-privileged containers to use ICMP_PROTO (used for `ping`). This option cannot be set for containers that have user-namespaces enabled. However, the detection looks to be incorrect; HostConfig.UsernsMode was added in 6993e89 / ee21838, and the property only has meaning if the daemon is running with user namespaces enabled. In other situations, the property has no meaning. As a result of the above, the sysctl would only be set for containers running with UsernsMode=host on a daemon running with user-namespaces enabled. This patch adds a check if the daemon has user-namespaces enabled (RemappedRoot having a non-empty value), or if the daemon is running inside a user namespace (e.g. rootless mode) to fix the detection. Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
cap_net_raw_usens_detection
branch
from
2b826b7 to
a826ca3
Compare
Member
Author
|
@AkihiroSuda updated; added an integration test, PTAL |
Member
Author
|
@AkihiroSuda PTAL; this ready to merge? |
AkihiroSuda
approved these changes
Sep 24, 2021
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Commit dae652e (#41030) added support for non-privileged containers to use ICMP_PROTO (used for
ping). This option cannot be set for containers that have user-namespaces enabled.However, the detection looks to be incorrect; HostConfig.UsernsMode was added in 6993e89 (#20111) / ee21838 (#20913), and the property only has meaning if the daemon is running with user namespaces enabled. In other situations, the property has no meaning.
As a result of the above, the sysctl would only be set for containers running with UsernsMode=host on a daemon running with user-namespaces enabled.
This patch adds a check if the daemon has user-namespaces enabled (RemappedRoot having a non-empty value) to fix the detection.
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)