Skip to content
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

storage.conf: Don't specify nodev by default #208

Closed
wants to merge 1 commit into from

Conversation

cgwalters
Copy link
Contributor

This breaks previously valid configurations, such as using
mock inside a privileged container. While it's easy for
such containers to learn to mount -o remount,dev /, it's a needless
compatibility break.

Rather, e.g. podman should learn to inject it only if it's known
safe to do so (e.g. the target container doesn't have CAP_MKNOD anyways).

This reverts the default value of
8b1a0f8
but not the ability to specify options.

Doing `nodev` on `/` breaks previously valid configurations, such as using
`mock` inside a privileged container.  While it's easy for
such containers to learn to `mount -o remount,dev /`, it's a needless
compatibility break.

Rather, e.g. podman should learn to inject it only if it's known
safe to do so (e.g. the target container doesn't have CAP_MKNOD anyways).
@rhatdan
Copy link
Member

rhatdan commented Aug 20, 2018

I actually want this for CRI-O more then podman, since running mock and having it create devices is not something we would do in mock. Why doesn't mock create a tmpfs for /dev?
Currently we give out MKNOD by default and only protection is the Device CGROUP which I believe is going away.

@cgwalters
Copy link
Contributor Author

only protection is the Device CGROUP which I believe is going away.

Reference?

since running mock and having it create devices is not something we would do in mock.

You mean do in cri-o/Kubernetes? I think scheduling traditional build systems like mock in Kubernetes is in fact a totally sane thing to do; again today it requires privileges. That said of course there's a really powerful tension to just drop mock and have RPM builds just be yum install from the base container.

Anyways, the point here is this broke compatibility, and today doesn't add any security.

@rhatdan
Copy link
Member

rhatdan commented Aug 20, 2018

Running mock in Kubernetes should be done in a volume in the container not on the container image.
The NODEV is just going to block any devices that come in the image. Not going to block devices being created in writable directories.

@cgwalters
Copy link
Contributor Author

Running mock in Kubernetes should be done in a volume in the container not on the container image.

I agree. But this change still broke my container, and doesn't really help anything today.

@rhatdan
Copy link
Member

rhatdan commented Aug 20, 2018

It is my understanding that the Device Cgroup was not implemented in V2 cgroups. You are supposed to use some Seccomp/eBPF Filtering, I believe.

cgwalters added a commit to cgwalters/rpm-ostree that referenced this pull request Oct 9, 2018
If `nodev` is set, we should fail fast.  See also
containers/storage#208
cgwalters added a commit to cgwalters/rpm-ostree that referenced this pull request Oct 9, 2018
If `nodev` is set, we should fail fast.  See also
containers/storage#208
cgwalters added a commit to cgwalters/rpm-ostree that referenced this pull request Oct 10, 2018
If `nodev` is set, we should fail fast.  See also
containers/storage#208
rh-atomic-bot pushed a commit to coreos/rpm-ostree that referenced this pull request Oct 11, 2018
If `nodev` is set, we should fail fast.  See also
containers/storage#208

Closes: #1604
Approved by: jlebon
@giuseppe
Copy link
Member

tests are failing because the commit is missing Signed-off-by:

@rhatdan
Copy link
Member

rhatdan commented Oct 31, 2018

I am closing this PR since @cgwalters can get what he wants from #226

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants