Skip to content

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

Merged
merged 1 commit into from
Dec 13, 2023

Conversation

dgl
Copy link
Contributor

@dgl dgl commented Nov 21, 2023

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.

@dgl dgl force-pushed the dgl/userns-otherfs branch 2 times, most recently from de8145a to b666bad Compare November 23, 2023 03:46
@rata rata requested a review from alban November 28, 2023 11:56
@rata
Copy link
Member

rata commented Nov 28, 2023

I'll check it out later this week, but skimming through it, SGTM. @alban can you take a look?

Copy link
Member

@rata rata left a comment

Choose a reason for hiding this comment

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

@dgl thanks a lot! This mostly LGTM, left some minor questions

@alban can you have a look too?

Copy link
Member

@alban alban left a 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.

Comment on lines +15 to 16
//go:build linux && cgo
// +build linux,cgo
Copy link
Member

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?

Copy link
Contributor Author

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.

@rata
Copy link
Member

rata commented Dec 8, 2023

Friendly ping?

@dgl
Copy link
Contributor Author

dgl commented Dec 11, 2023

@rata sorry, updated now, I had to think about this more than expected ;)

@rata rata self-requested a review December 11, 2023 15:56
Copy link
Member

@rata rata left a 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 :)

@rata rata requested a review from alban December 11, 2023 15:57
Copy link
Member

@alban alban left a 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.

@dgl dgl force-pushed the dgl/userns-otherfs branch from af1ba34 to 836d609 Compare December 13, 2023 11:30
@dgl
Copy link
Contributor Author

dgl commented Dec 13, 2023

[...] if you can amend the review changes to the previous commit, it would be great :)

Done :)

@rata
Copy link
Member

rata commented Dec 13, 2023

Thanks!

@rata rata merged commit a69f91e into kinvolk:main Dec 13, 2023
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