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

e2e: rethink setenforce-0 #18564

Closed
edsantiago opened this issue May 15, 2023 · 4 comments · Fixed by #18607
Closed

e2e: rethink setenforce-0 #18564

edsantiago opened this issue May 15, 2023 · 4 comments · Fixed by #18607
Labels
locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Comments

@edsantiago
Copy link
Member

Found by accident while reviewing #18544:

se := SystemExec("setenforce", []string{"0"})
se.WaitWithDefaultTimeout()
if se.ExitCode() != 0 {
Skip("Cannot disable selinux, this may cause problem for reading cert files inside container.")
}
defer SystemExec("setenforce", []string{"1"})

se := SystemExec("setenforce", []string{"0"})
Expect(se).Should(Exit(0))
defer func() {
se2 := SystemExec("setenforce", []string{"1"})
Expect(se2).Should(Exit(0))

Let's understand if/why these are needed.

  • If not needed: remove
  • If needed: explain in comments, and Serialize.
@vrothberg
Copy link
Member

vrothberg commented May 15, 2023

Commit 3a3c558 added these lines but there's no indication of why SELinux would play a role. Maybe because the registry setup mounts HOME directories into the registry container?

I wonder if we couldn't just nuke all this code and use the hack/podman-registry-go bindings instead to create/start/stop a local registry?

@Luap99
Copy link
Member

Luap99 commented May 15, 2023

Yes please nuke this, I was also very surprised when I saw it. This by definition also means it skips all these test as rootless as setenfore is only allowed as root.

@vrothberg
Copy link
Member

vrothberg commented May 15, 2023

I didn't volunteer (yet) to do the work as time is scarce. But I had a quick look and it seems like a bit of work to get the entire file migrated.

Luap99 added a commit to Luap99/libpod that referenced this issue May 17, 2023
We should not change selinux, in a parallel context this can change the
behavior of other tests and we should never disable selinux anyway.

Lets see if this passes CI or not.

Fixes containers#18564

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
@Luap99
Copy link
Member

Luap99 commented May 17, 2023

Given that this doesn't work in a parallel context anyway (TOCTOU) I am just going to remove the setenfore calls and see if CI passes (#18607). We can look into using hack/podman-registry-go another time.

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Aug 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants