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

Makefile: Mount . with --security-opt label=disable instead of using --privileged #15479

Merged

Conversation

dcermak
Copy link
Contributor

@dcermak dcermak commented Aug 25, 2022

$(CURDIR) is mounted in podman without :Z which causes issues on systems with SELinux as then the container cannot read or write anything inside /src/. This has been worked around with the --privileged flag, but that's a rather brutal solution. Instead, adding the proper SELinux sharing setting fixes the problem and allows make vendor-in-container to run unprivileged.

Does this PR introduce a user-facing change?

None

@dcermak dcermak force-pushed the don-t-use-privileged-containers branch from c285fcd to 00b58e4 Compare August 25, 2022 09:01
@rhatdan
Copy link
Member

rhatdan commented Aug 26, 2022

NO, You should not use :Z to relabel the users homedirectory every time this is done. The better fix is to just disable SELinux during the running of the container.

--security-opt label=disabled.

@dcermak dcermak force-pushed the don-t-use-privileged-containers branch from 00b58e4 to fbeb42b Compare August 26, 2022 12:42
@dcermak
Copy link
Contributor Author

dcermak commented Aug 26, 2022

NO, You should not use :Z to relabel the users homedirectory every time this is done. The better fix is to just disable SELinux during the running of the container.

--security-opt label=disabled.

Done, thanks for the suggestion!

@rhatdan rhatdan changed the title Makefile: Mount . with :Z instead of using --privileged Makefile: Mount . with --security-opt label=disable instead of using --privileged Aug 26, 2022
@rhatdan
Copy link
Member

rhatdan commented Aug 26, 2022

LGTM
But can you fix the commit message.

$(CURDIR) is mounted in podman as is which causes issues on systems with SELinux
as then the container cannot read or write anything inside /src/. This has been
worked around with the --privileged flag, but that's a rather brutal
solution. Adding :Z is also suboptimal, as that requires a full relabeling after
every run. Instead, we disable security labeling via `--security-opt
label=disable` for this development container allowing us to run `make
vendor-in-container` unprivileged.

Signed-off-by: Dan Čermák <[email protected]>
@dcermak dcermak force-pushed the don-t-use-privileged-containers branch from fbeb42b to dcb4d43 Compare August 29, 2022 07:05
@dcermak
Copy link
Contributor Author

dcermak commented Aug 29, 2022 via email

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold

Thanks, @dcermak !

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 29, 2022
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 29, 2022
@rhatdan
Copy link
Member

rhatdan commented Aug 31, 2022

/approve
/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 31, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 31, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dcermak, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 31, 2022
@openshift-merge-robot openshift-merge-robot merged commit 4cff780 into containers:main Aug 31, 2022
@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 Sep 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants