Remove tun/tap from the default device rules

[kolyshkin]

Looking through git blame, this was added by commit 9fac183
aka "Initial commit of runc binary", most probably by mistake.

Obviously, a container should not have access to tun/tap device, unless
it is explicitly specified in configuration.

Now, removing this might create a compatibility issue, but I see no
other choice.

Aside from the obvious misconfiguration, this should also fix the
annoying

Apr 26 03:46:56 foo.bar systemd[1]: Couldn't stat device /dev/char/10:200: No such file or directory

messages from systemd on every container start, when runc uses systemd
cgroup driver, and the system runs an old (< v240) version of systemd
(the message was presumably eliminated by [1]).

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1945929

[1] systemd/systemd@d5aecba

[rhatdan]

LGTM

[thaJeztah]

/cc @djs55 @justincormack @crosbymichael ptal

[thaJeztah]

SGTM

(I'm asking around a bit if others can see potential issues with the change)

[thaJeztah]

Got some feedback from some people; one person mentioned that this could affect users such as Tailscale, who may be using this. We have a joined Slack channel with them, so I put out a question to ask for feedback.

Also asking internally at Docker (Docker Desktop team didn't foresee issues they wouldn't be able to resolve), and asked the containerd maintainers.

[DentonGentry]

Got some feedback from some people; one person mentioned that this could affect users such as Tailscale, who may be using this. We have a joined Slack channel with them, so I put out a question to ask for feedback.

Tailscale's instructions and sample docker-compose.yml files say to explicitly include /dev/net/tun, anyone who referenced those shouldn't be impacted.

  devices:
    - /dev/net/tun:/dev/net/tun
  cap_add:
    - NET_ADMIN
    - NET_RAW

support@tailscale.com can watch for symptoms that look like a container which had been relying on getting a /dev/net/tun device via the prior default setting, and advise adding it in the docker-compose.yml or a command line argument.

[AkihiroSuda]

Cc @giuseppe Is this safe for rootless Podman?

[thaJeztah]

@kolyshkin would it be possible for you to post some test-builds from this PR somewhere for people to download?

[giuseppe]

Cc @giuseppe Is this safe for rootless Podman?

from a quick test, it seems the slirp4netns network works fine

[mrunalp]

LGTM

[kolyshkin]

@kolyshkin would it be possible for you to post some test-builds from this PR somewhere for people to download?

Those are actually available (for every PR), although not in a very consumable form. Click on "Checks" tab, when click "validate" from the left-side list, and on the bottom of that screen you'll see a big release-xxxxxx tarball under "Artifacts". It contains all the static binaries.

[thaJeztah]

@kolyshkin would it be possible for you to post some test-builds from this PR somewhere for people to download?

Those are actually available (for every PR), although not in a very consumable form. Click on "Checks" tab, when click "validate" from the left-side list, and on the bottom of that screen you'll see a big release-xxxxxx tarball under "Artifacts". It contains all the static binaries.

oh! I didn't know we were storing artefacts; let me pass that on!

[thaJeztah]

And.. there I went looking, but it's not in the Checks tab, it's in the top-level Actions tab; this is the last run of this PR; https://github.com/opencontainers/runc/actions/runs/2272679304

And download link is https://github.com/opencontainers/runc/suites/6379868023/artifacts/231538496

(super confusing!)

[fuweid]

LGTM(nb)

[cyphar]

LGTM, but I don't think this was a mistake -- the initial set of devices added was based on what was needed to get Docker containers to work with most programs people were using.

When I was reworking the devices handling code a year or two ago, I also wondered whether we should drop this (and I dropped some other far more dangerous rules, but left this one because @crosbymichael couldn't remember what this was needed for and I didn't want to break anything in a security update if I could help it).

That being said, Docker has their own default devices rules that are set externally to runc (so keeping this default is not necessary) and I don't see why a standard container would need tun/tap devices (though since the container is in a network namespace, I suspect that it wouldn't be an issue that they can create such devices). However I believe Docker generates their rules using our library functions, so maybe we should do a test build of Docker with this change and test that before we do a release?

[thaJeztah]

That being said, Docker has their own default devices rules that are set externally to runc (so keeping this default is not necessary)

Yes, I was thinking about that later; perhaps this change wouldn't affect the defaults on Docker (or Containerd for that matter).

However I believe Docker generates their rules using our library functions, so maybe we should do a test build of Docker with this change and test that before we do a release?

Good point; perhaps won't hurt to do a draft PR in the docker repo to verify (at least CI).

[thaJeztah]

opened moby/moby#43569 to get a run of CI with these changes

[kolyshkin]

opened moby/moby#43569 to get a run of CI with these changes

Seems passing. Seems that we have a consensus this can be merged. @opencontainers/runc-maintainers this is your last chance to say no :)

[thaJeztah]

Looks like we should be ready to go.

FWIW; at a first glance, Docker's defaults don't include this (I was trying to find if it was removed at some point; still digging) https://github.com/moby/moby/blob/2bbc786e4c59761d722d2d1518cd0a32829bc07f/oci/defaults.go#L121-L177

But looks like containerd's defaults do; https://github.com/containerd/containerd/blob/65627f568febf00394c0d5406d23464902d4aee8/oci/spec_opts.go#L1201-L1208

[thaJeztah]

Okay; found it; it looks like it changed in the containerd integration; moby/moby#20662

Looks like this commit; https://github.com/moby/moby//commit/9c4570a958df42d1ad19364b1a8da55b891d850a#diff-6e3774954611ecbf441ac0aa5841f11926c1e618b391ff64c330cd64e8875756L126 removed the use of configs.DefaultAllowedDevices (from runc/libcontainer/configs)

allowedDevices := mergeDevices(configs.DefaultAllowedDevices, userSpecifiedDevices)

and replaced it with a local list of defaults; https://github.com/moby/moby//commit/9c4570a958df42d1ad19364b1a8da55b891d850a#diff-0a4d29b610dc02821e36ed4b8ce4a2dffc64d09b1b554bfb277d735acac459d4R89-R208

[thaJeztah]

Let me bring this one in 👍

[zhsj]

Hi,

Is there any security concerns for using tun/tap in container? I think not.?

If not, I think it makes people more difficult to use tun/tap on Kubernetes. Now they need to use some tun/tap device plugins... (There's no convenient --device option like docker cli).

[thaJeztah]

@zhsj what runtime are you using for kubernetes? As at least docker (moby) and containerd already seem to be using the same defaults;

Okay; found it; it looks like it changed in the containerd integration; moby/moby#20662

Looks like this commit; https://github.com/moby/moby//commit/9c4570a958df42d1ad19364b1a8da55b891d850a#diff-6e3774954611ecbf441ac0aa5841f11926c1e618b391ff64c330cd64e8875756L126 removed the use of configs.DefaultAllowedDevices (from runc/libcontainer/configs)

[zhsj]

@zhsj what runtime are you using for kubernetes? As at least docker (moby) and containerd already seem to be using the same defaults;

Hmm, I'm using docker. After I re-read your comments on containerd's PR, I realized you said docker has already removed tun/tap long before. Probably I added CAP_SYS_ADMIN so that I can still use tun/tap without --device option, while cgroup won't deny it on this scenario .