-
Notifications
You must be signed in to change notification settings - Fork 10
Add support for mounting other filesystems in user namespaces #36
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
Conversation
de8145a
to
b666bad
Compare
I'll check it out later this week, but skimming through it, SGTM. @alban can you take a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I added some comments, mostly small clarifications requested.
I have some security concerns but I think it's up to the user of seccomp-agent to decide if they are valid concerns depending on their architecture, as you have explained in the README.
//go:build linux && cgo | ||
// +build linux,cgo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we require go 1.18 (see go.mod), the new directive //go:build
is necessarily supported and the old one is not required. Should we remove the old ones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll send a follow up PR, seems sensible.
Friendly ping? |
@rata sorry, updated now, I had to think about this more than expected ;) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still LGTM, added a minor comment.
I'd like @alban to take another look and if you can amend the review changes to the previous commit, it would be great :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM once Rodrigo's question has been replied.
Signed-off-by: David Leadbeater <[email protected]>
af1ba34
to
836d609
Compare
Done :) |
Thanks! |
Allow mounting other filesystems
...optionally only when CAP_SYS_ADMIN in a user namespace
When using a user namespace it is desirable that it acts like "host root", that is it has the ability to mount filesystems and so on. A bunch of this just works, e.g. tmpfs is allowed by the kernel itself. However it isn't possible to mount network filesystems.
This change adds the ability to configure a list of allowed filesystems, as well as passing the options through. This makes it possible to mount any file system configured. Whether it works will depend on the user's environment, as block devices generally won't be available within the container (and
-o loop
sadly won't work because loopback and user namespaces don't mix well), however it will work for many network file systems, tested with cifs.cc @rata
How to use
See the readme.
Testing done
See the readme.