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

Use shared selinux context for mounts #6294

Closed
wants to merge 8 commits into from

Conversation

monosoul
Copy link
Contributor

@monosoul monosoul commented Dec 4, 2022

On systems with SElinux enforcing trying to bind a path without a context will result in an error. Using shared context by default will make withFileSystemBind(String hostPath, String containerPath) method work for all systems. On Windows, Mac OS and Linux systems without SElinux it will have no effect. On systems with SElinux enforcing it will make the path accessible to the container.

@monosoul monosoul requested a review from a team as a code owner December 4, 2022 19:11
@monosoul monosoul changed the title Use shared selinux context for filesystem binds Use shared selinux context for mounts Dec 5, 2022
On systems with SElinux enforcing trying to bind a filesystem path without a context will result in an error. Using shared context by default will make `withFileSystemBind(String hostPath, String containerPath)` method work for all systems. On Windows, Mac OS and Linux systems without SElinux it will have no effect. On systems with SElinux enforcing it will make the path accessible to the container.

Signed-off-by: monosoul <Kloz.Klaud@gmail.com>
@Flurb
Copy link

Flurb commented Feb 7, 2023

As an SELinux user, I definitely upvote! Thanks for creating this PR!

@eddumelendez
Copy link
Member

Hi, @monosoul, thanks for the PR! Unfortunately, this is a breaking change due to introducing the change will, by default, bind the filesystem instead of copying the file. This will break for those using a remote Docker. Below, you can see the logic described

@Override
public SELF withClasspathResourceMapping(
final String resourcePath,
final String containerPath,
final BindMode mode,
final SelinuxContext selinuxContext
) {
final MountableFile mountableFile = MountableFile.forClasspathResource(resourcePath);
if (mode == BindMode.READ_ONLY && selinuxContext == SelinuxContext.NONE) {
withCopyFileToContainer(mountableFile, containerPath);
} else {
addFileSystemBind(mountableFile.getResolvedPath(), containerPath, mode, selinuxContext);
}
return self();
}

However, you still can use the method directly. I understand the PR aim to make this easier. Although, I think we can document this if it is not clear.

@monosoul
Copy link
Contributor Author

monosoul commented Mar 2, 2023

@eddumelendez if shared context will be the default one, I could've just changed the condition there to selinuxContext == SelinuxContext.SHARED. Or am I missing something?

@eddumelendez
Copy link
Member

Yes, that's right! 🤦🏽‍♂️ Sorry about that. I just tested locally and with my remote environment and all good.

@eddumelendez eddumelendez reopened this Mar 2, 2023
@eddumelendez eddumelendez added this to the next milestone Mar 2, 2023
@@ -1274,7 +1274,7 @@ public SELF withClasspathResourceMapping(
) {
final MountableFile mountableFile = MountableFile.forClasspathResource(resourcePath);

if (mode == BindMode.READ_ONLY && selinuxContext == SelinuxContext.NONE) {
if (mode == BindMode.READ_ONLY && selinuxContext == SelinuxContext.SHARED) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm fine updating addFileSystemBind but with withClasspathResourceMapping, I'm still thinking about it... This will promote copying resources instead of binding filesystem and current source code with those condition will do so. The only scenario to bind filesystem would be with BindMode.READ_WRITE. Ping to @kiview and @bsideup in case I'm missing something.

@eddumelendez eddumelendez removed this from the next milestone Mar 8, 2023
@eddumelendez
Copy link
Member

Superseded by #7187

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

Successfully merging this pull request may close these issues.

None yet

3 participants