Skip to content

daemon.WithCommonOptions() fix detection of user-namespaces #42736

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.

Sign up for GitHub

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

Jump to bottom
Merged
AkihiroSuda merged 1 commit into from
Sep 24, 2021
Merged

daemon.WithCommonOptions() fix detection of user-namespaces #42736

AkihiroSuda merged 1 commit into from
Sep 24, 2021
+45 −3

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Aug 11, 2021

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)

@thaJeztah
Copy link
Member Author

thaJeztah commented Aug 11, 2021

ping @justincormack @AkihiroSuda @tianon ptal

@thaJeztah
Copy link
Member Author

thaJeztah commented Aug 11, 2021

Looks like it's missing detection for rootless now

@thaJeztah thaJeztah force-pushed the cap_net_raw_usens_detection branch from 021f848 to ba60f2a Compare August 12, 2021 11:16
@thaJeztah
Copy link
Member Author

thaJeztah commented Aug 12, 2021

Rootless looks ok now, but hitting a flaky test. Opened a ticket for that: #42739

=== FAIL: libnetwork/networkdb TestNetworkDBCRUDTableEntries (7.25s)
    networkdb_test.go:314: Entry existence verification test failed for node2(df7cec9ba2d9)
2021/08/12 11:35:37 Closing DB instances...

@thaJeztah
Copy link
Member Author

thaJeztah commented Aug 12, 2021

Another flaky: TestBuildWCOWSandboxSize - opened #42743

@thaJeztah
Copy link
Member Author

thaJeztah commented Aug 12, 2021 •
edited
Loading

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/ping

Alpine:

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/busybox

So the alpine image uses busybox for ping how is that in the busybox image (hardlinks?)

@thaJeztah
Copy link
Member Author

thaJeztah commented Aug 12, 2021

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 bytes

Versions 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.

@thaJeztah
Copy link
Member Author

thaJeztah commented Aug 12, 2021 •
edited
Loading

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;

http://lists.busybox.net/pipermail/busybox/2014-January/080224.html

@thaJeztah
Copy link
Member Author

thaJeztah commented Aug 12, 2021

With this patch, and CAP_NET_RAW removed, it works with alpine and a non-privileged user;

docker run --rm -u 1000:1000 --cap-drop CAP_NET_RAW 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=92.999 ms

--- example.com ping statistics ---
1 packets transmitted, 1 packets received, 0% packet loss
round-trip min/avg/max = 92.999/92.999/92.999 ms

@thaJeztah
Copy link
Member Author

thaJeztah commented Aug 12, 2021

@AkihiroSuda PTAL

1 similar comment
@thaJeztah
Copy link
Member Author

thaJeztah commented Aug 23, 2021

@AkihiroSuda PTAL

@AkihiroSuda AkihiroSuda self-requested a review August 26, 2021 19:15
@AkihiroSuda AkihiroSuda self-assigned this Aug 26, 2021
AkihiroSuda
AkihiroSuda approved these changes Aug 26, 2021
View reviewed changes
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but would like to see an integration test too if possible

@thaJeztah thaJeztah force-pushed the cap_net_raw_usens_detection branch 2 times, most recently from 076d48d to 2b826b7 Compare August 30, 2021 15:06
@thaJeztah
daemon.WithCommonOptions() fix detection of user-namespaces
a826ca3 
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 <github@gone.nl>
@thaJeztah thaJeztah force-pushed the cap_net_raw_usens_detection branch from 2b826b7 to a826ca3 Compare August 30, 2021 17:48
@thaJeztah
Copy link
Member Author

thaJeztah commented Sep 2, 2021

@AkihiroSuda updated; added an integration test, PTAL

@thaJeztah
Copy link
Member Author

thaJeztah commented Sep 24, 2021

@AkihiroSuda PTAL; this ready to merge?

@AkihiroSuda AkihiroSuda merged commit 30413e5 into moby:master Sep 24, 2021
@thaJeztah thaJeztah deleted the cap_net_raw_usens_detection branch September 24, 2021 13:56
@thaJeztah thaJeztah mentioned this pull request Sep 28, 2021
Closed
@thaJeztah thaJeztah added this to the 21.xx milestone Oct 15, 2021
@olljanat olljanat mentioned this pull request Oct 28, 2021
Merged
@AkihiroSuda AkihiroSuda mentioned this pull request Dec 15, 2021
Merged
@thaJeztah thaJeztah mentioned this pull request Sep 15, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

+1 more reviewer

@justincormack justincormack justincormack approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@AkihiroSuda AkihiroSuda

Labels

impact/changelog kind/bugfix PR's that fix bugs process/cherry-picked status/2-code-review

Projects

None yet

Milestone

23.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@thaJeztah @justincormack @AkihiroSuda