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

Allow users to remap FDs before container start #3013

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

aidanhs
Copy link

@aidanhs aidanhs commented Dec 7, 2024

Note: this is not ready for merge (no tests, no exposure via cli, not signed off) but I've been carrying/using the branch for a while and thought I'd make a PR in case it's useful to anyone/see if there's any desire to get it into a mergeable state. It's built on top of #2892 and #2893 - to see the actual changes you'll want to look at aidanhs/youki@aphs-remap-fds~2...aidanhs:youki:aphs-remap-fds. The first commit improves FD discipline by passing fewer down to the intermediate process, the second implements remap fds.

The primary purpose of this PR is to allow programs that are not disciplined with their FDs to invoke containers and pass down FDs in a controlled way (i.e. without leaking any).

For example, if I have let f = fs::File::open(...).unwrap() - let's say the FD is 25 (and may vary, depending on what other file operations I'm performing in the program/other threads). How do I pass this to my program? nsjail has an argument for this (--pass-fd) but it's a lot harder if you want to use youki as a library:

  • You cannot use preserve_fds = 23 as this will pass all file descriptors up to 25
  • You cannot relocate fd 25 to 3 via dup and use perserve_fds = 1 because you don't know what's using fd 3 - you'd need to refactor the whole program to be disciplined about FD usage (which may not even be possible if threads are involved).
  • The correct way today (once Fix --preserve-fds, eliminate stray FD being passed into container #2893 is merged) is to fork, relocate fd 25 to 3 and use preserve_fds - but now you have an extra process you need to communicate with and manage.

remap_fds solves this problem by acting as a more general --pass-fd:

  • if you pass (25, 25) the fd will be unmarked as cloexec and passed down to the container
  • if you pass (25, 3) the fd will be remapped by youki at the last moment, when it is safe to do so, and then passed down

Unfortunately this will probably not work well in combination with seccomp - because remapping has to happen so late, seccomp enforcement will already be in place. There is a way to fix this that I have noted in the code, but I think it's quite difficult.

aidanhs added 13 commits August 30, 2024 19:45
Signed-off-by: Aidan Hobson Sayers <[email protected]>
Signed-off-by: Aidan Hobson Sayers <[email protected]>
Signed-off-by: Aidan Hobson Sayers <[email protected]>
Signed-off-by: Aidan Hobson Sayers <[email protected]>
Signed-off-by: Aidan Hobson Sayers <[email protected]>
Test harness additionally needed to support

1. tests that cannot run in parallel
2. tests that need to customise create arguments

Signed-off-by: Aidan Hobson Sayers <[email protected]>
@YJDoc2 YJDoc2 marked this pull request as draft December 12, 2024 13:04
@YJDoc2 YJDoc2 added the wontfix This will not be worked on label Dec 16, 2024
@YJDoc2
Copy link
Collaborator

YJDoc2 commented Dec 16, 2024

Hey, I have made this draft + added wontfix label just to signify that there is nothing immediate too be done on this (from reviewer side). This is not really a "wontfix" , if you do get this ready for a merge, you can mark this ready to review, after which we can do the normal review process + remove the label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants