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

Make SELinux labels opt-in (--oci-worker-selinux=<BOOL>) #3203

Merged
merged 1 commit into from
Nov 2, 2022

Conversation

AkihiroSuda
Copy link
Member

Fix #3202

@thaJeztah
Copy link
Member

So, the only concern I have is (my dislike of) booleans, especially if their default may change at some point (if so, we probably need to have a new option SELinuxDisabled)

I'm not very good at the SELinux terminology, and not sure which options should be "boolean only", or (may) need further customization down the line, so don't have good suggestions right now.

With the above out of the way, some quick looks at what we have in Moby, which (perhaps) may help verify if the current design "looks right";

  1. For docker run, we use --security-opt label=<option> where label=disable means "don't apply labels", but custom labels can be passed.
    • ^^ don't know if that applies to build as well (custom labels), but mostly refering to that to indicate "more than just boolean"
  2. For volumes we have the z and Z options to apply SELinux labels; would something similar be needed for mounts used during build? (RUN --mount=....)?
  3. For the daemon on the other hand, we also have the --selinux-enabled option (config.EnableSelinuxSupport), which is disabled by default, and that one is a boolean.
    • ^^ This PR seems to (roughly) match that, but (as mentioned) whether that design was "correct" in moby or not should be looked at.

@AkihiroSuda
Copy link
Member Author

Yes, this is analogous to 3.

1. For docker run, we use --security-opt label=<option> where label=disable means "don't apply labels", but custom labels can be passed.

AFAICS this rather means calling s.Process.SelinuxLabel, s.Linux.MountLabel, err = label.InitLabels([]string{"disable"}), while 3 does not touch the labels at all

2. For volumes we have the z and Z options to apply SELinux labels; would something similar be needed for mounts used during build? (RUN --mount=....)?

Maybe yes, but that is out of the scope of this PR.

@@ -9,6 +9,7 @@ const (
Hostname = prefix + "hostname"
Network = prefix + "network" // "cni" or "host"
ApparmorProfile = prefix + "apparmor.profile"
SELinux = prefix + "selinux" // "true" or "false"
Copy link
Member

Choose a reason for hiding this comment

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

Could be "enabled" and "disabled"

Suggested change
SELinux = prefix + "selinux" // "true" or "false"
SELinux = prefix + "selinux" // SELinux support ("enabled", "disabled")

Copy link
Member Author

Choose a reason for hiding this comment

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

@tonistiigi WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

No strict opinion. Personally would prefer values that work with ParseBool. Or selinux.enabled maybe in that case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to selinux.enabled

@tonistiigi
Copy link
Member

@AkihiroSuda Check the gofmt

Fix issue 3202

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
@AkihiroSuda AkihiroSuda force-pushed the make-selinux-labels-opt-in branch from cda5e94 to bd57e5f Compare November 2, 2022 05:33
@AkihiroSuda
Copy link
Member Author

fixed

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@AkihiroSuda AkihiroSuda requested a review from cpuguy83 November 2, 2022 12:33
Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

I think the issue likely arose when we got rid of the selinux build tag.

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.

Make SELinux labels opt-in
4 participants