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

add container arguments to specify SELinux contexts for mounts #334

Merged
merged 3 commits into from
Jun 21, 2017

Conversation

josh-cain
Copy link
Contributor

I wanted to use the :z/Z mount options with SELinux and saw where they were not exposed. Just a small patch that will expose these from the underlying library.

@rnorth
Copy link
Member

rnorth commented May 6, 2017

Ah nice, thanks for submitting this. I'll take a look today and hopefully merge.
Thanks!
Richard

@bsideup bsideup added this to the 1.3.0 milestone May 29, 2017
@bsideup
Copy link
Member

bsideup commented May 29, 2017

Hey @rnorth,

Do you want me to take care of this PR?

@rnorth
Copy link
Member

rnorth commented May 30, 2017

Sorry @bsideup - yes please

* Possible contexts for use with SELinux
*/
public enum SelinuxContext {
SHARED(SELContext.shared), SINGLE(SELContext.single), NONE(SELContext.none);
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add a new line after every entry?

public enum SelinuxContext {
SHARED(SELContext.shared), SINGLE(SELContext.single), NONE(SELContext.none);

public final SELContext selContext;
Copy link
Member

Choose a reason for hiding this comment

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

FYI since we use Lombok, you can remove "public final" and the constructor and just add a single @Value annotation to the class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh. my. word. Where has Lombok been all my life? This is amazing.

Although @Value can't be used on an enum. Went for @AllArgsConstructor instead.

@@ -204,6 +215,15 @@ public void customClasspathResourceMappingTest() throws IOException {
}

@Test
public void customClasspathResourceMappingWithSelinuxTest() throws IOException {
// Note: This functionality doesn't work if you are running your build inside a Docker container;
Copy link
Member

Choose a reason for hiding this comment

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

I think this comment is outdated and should not be used here

* Map a file on the classpath to a file in the container, and then expose the content for testing.
*/
@ClassRule
public static GenericContainer alpineClasspathResourceSelinx = new GenericContainer("alpine:3.2")
Copy link
Member

Choose a reason for hiding this comment

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

typo. selinx -> selinux

public static GenericContainer alpineClasspathResourceSelinx = new GenericContainer("alpine:3.2")
.withExposedPorts(80)
.withClasspathResourceMapping("mappable-resource/test-resource.txt", "/content.txt", READ_WRITE, SHARED)
.withCommand("/bin/sh", "-c", "while true; do cat /content.txt | nc -l -p 80; done");
Copy link
Member

Choose a reason for hiding this comment

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

Would be helpful to test SELinux mode. Right now these new tests will pass even if we drop selinuxContext passing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True story. Not sure how I'd actually go about doing that other than inspecting the assigned seLinux type in the running container or on the host machine. Any suggestions as to how I might inspect that? I'm assuming we don't want our test to call 'inspect'...

SINGLE(SELContext.single),
NONE(SELContext.none);

public final SELContext selContext;
Copy link
Member

Choose a reason for hiding this comment

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

how about private modifier and @Getter on it? We usually use getters instead of fields access :)

Copy link
Member

Choose a reason for hiding this comment

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

ping

Copy link
Member

Choose a reason for hiding this comment

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

@cainj13 ping :)

@@ -204,6 +215,12 @@ public void customClasspathResourceMappingTest() throws IOException {
}

@Test
public void customClasspathResourceMappingWithSelinuxTest() throws IOException {
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 not an SELinux expert, but maybe this might help?
https://www.cyberciti.biz/faq/howto-display-selinux-security-context-using-ls/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, we can get the labeled context on the container (provided we use a beefier image with SELinux utils installed), but I thought the :Z option used some host configs to label it as the correct type. Wouldn't this also require knowledge of some SELinux context on the host to make sure they match? Thought we'd want to avoid having to interrogate/put dependencies on the host.

@josh-cain
Copy link
Contributor Author

Hi guys, I'm being asked about this on the aforementioned PR ^. Do I need to make some enhancements to the test? I can't really say that I know of a way to do it without introducing host dependencies as that is what determines a shared SELinux context...

@bsideup
Copy link
Member

bsideup commented Jun 10, 2017

Hi @cainj13
Ok, let's merge it as is and we will think how to test it later 👍

I pinged you about a small minor change, but everything else seems fine to me =)

@bsideup bsideup modified the milestones: 1.3.1, 1.3.0 Jun 10, 2017
@rnorth rnorth merged commit 7af0dc5 into testcontainers:master Jun 21, 2017
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