Do not add powercap mask if no paths are masked #20510

mheon · 2023-10-27T13:00:12Z

This solves --security-opt unmask=ALL still masking the path.

[NO NEW TESTS NEEDED] Can't easily test this as we do not have access to it in CI.

Does this PR introduce a user-facing change?

NONE

mheon · 2023-10-27T13:00:24Z

We'll also need this in 4.7.2 @lsm5

mheon · 2023-10-27T13:00:31Z

@giuseppe @Luap99 PTAL

Luap99 · 2023-10-27T13:05:49Z

docs/source/markdown/options/security-opt.md

@@ -31,6 +31,6 @@ Note: Labeling can be disabled for all <<|pods/>>containers by setting label=fal
  for the possible mount options are specified in the **proc(5)** man page.

 - **unmask**=_ALL_ or _/path/1:/path/2_, or shell expanded paths (/proc/*): Paths to unmask separated by a colon. If set to **ALL**, it unmasks all the paths that are masked or made read-only by default.
-  The default masked paths are **/proc/acpi, /proc/kcore, /proc/keys, /proc/latency_stats, /proc/sched_debug, /proc/scsi, /proc/timer_list, /proc/timer_stats, /sys/firmware, and /sys/fs/selinux**.  The default paths that are read-only are **/proc/asound**, **/proc/bus**, **/proc/fs**, **/proc/irq**, **/proc/sys**, **/proc/sysrq-trigger**, **/sys/fs/cgroup**.
+  The default masked paths are **/proc/acpi, /proc/kcore, /proc/keys, /proc/latency_stats, /proc/sched_debug, /proc/scsi, /proc/timer_list, /proc/timer_stats, /sys/firmware, and /sys/fs/selinux**.  The default paths that are read-only are **/proc/asound**, **/proc/bus**, **/proc/fs**, **/proc/irq**, **/proc/sys**, **/proc/sysrq-trigger**, **/sys/fs/cgroup**, **/sys/devices/virtual/powercap**.


this is not a readonly path, it should go after /sys/fs/selinux in the sentence before.

giuseppe

LGTM

This solves `--security-opt unmask=ALL` still masking the path. [NO NEW TESTS NEEDED] Can't easily test this as we do not have access to it in CI. Signed-off-by: Matthew Heon <[email protected]>

Luap99

/lgtm
/hold

openshift-ci · 2023-10-27T14:04:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, Luap99, mheon

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:

~~OWNERS~~ [Luap99,giuseppe,mheon]

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

mheon · 2023-10-27T14:18:00Z

/cherry-pick v4.7

openshift-cherrypick-robot · 2023-10-27T14:18:03Z

@mheon: once the present PR merges, I will cherry-pick it on top of v4.7 in a new PR and assign it to you.

In response to this:

/cherry-pick v4.7

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

mheon · 2023-10-27T15:44:03Z

/hold cancel

openshift-cherrypick-robot · 2023-10-27T15:48:31Z

@mheon: #20510 failed to apply on top of branch "v4.7":

Applying: Do not add powercap mask if no paths are masked
Using index info to reconstruct a base tree...
M	libpod/container_internal_linux.go
Falling back to patching base and 3-way merge...
Auto-merging libpod/container_internal_linux.go
CONFLICT (content): Merge conflict in libpod/container_internal_linux.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Do not add powercap mask if no paths are masked
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick v4.7

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

rhatdan · 2023-10-27T15:59:08Z

Should this be masked in podman build?

In buildah, we have

func setupMaskedPaths(g *generate.Generator) {
	for _, mp := range []string{
		"/proc/acpi",
		"/proc/kcore",
		"/proc/keys",
		"/proc/latency_stats",
		"/proc/timer_list",
		"/proc/timer_stats",
		"/proc/sched_debug",
		"/proc/scsi",
		"/sys/firmware",
		"/sys/fs/selinux",
		"/sys/dev",
	} {
		g.AddLinuxMaskedPaths(mp)
	}
}

Seems like these should be centralized somewhere.

mheon · 2023-10-27T15:59:51Z

Yeah, it should also be masked in Buildah, Good point.

mheon · 2023-10-27T16:00:34Z

/cherry-pick v4.7

openshift-cherrypick-robot · 2023-10-27T16:01:15Z

@mheon: new pull request created: #20514

In response to this:

/cherry-pick v4.7

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

mheon · 2023-10-27T16:08:06Z

containers/buildah#5110 for buildah

rhatdan · 2023-10-27T16:10:43Z

I am not seeing this masked by default?

$ podman run alpine ls /sys/devices/virtual/powercap/
intel-rapl
intel-rapl-mmio

mheon · 2023-10-27T16:14:40Z

Works over here.

Upstream Podman (4.7.1):

podman run -t -i --rm fedora ls -al /sys/devices/virtual/powercap
total 0
drwxr-xr-x.  3 nobody nobody 0 Oct 27 16:13 .
drwxr-xr-x. 25 nobody nobody 0 Oct 27 16:13 ..
drwxr-xr-x.  4 nobody nobody 0 Oct 27 16:13 intel-rapl

My branch, with both Powercap patches:

./bin/podman run -t -i --rm fedora ls -al /sys/devices/virtual/powercap
total 0
drwxrwxrwt.  2 root   root   40 Oct 27 16:13 .
drwxr-xr-x. 25 nobody nobody  0 Oct 27 16:13 ..

TomSweeneyRedHat · 2023-11-02T21:38:24Z

Partially addresses https://issues.redhat.com/browse/RHEL-12098 and https://issues.redhat.com/browse/RHEL-12097. A vendor of Buildah with the fix is needed to complete the fix.

openshift-ci bot added release-note-none approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 27, 2023

mheon force-pushed the powercap_2 branch 2 times, most recently from aa0d0f9 to 0568cdc Compare October 27, 2023 13:04

Luap99 reviewed Oct 27, 2023

View reviewed changes

giuseppe approved these changes Oct 27, 2023

View reviewed changes

Do not add powercap mask if no paths are masked

c6d410c

This solves `--security-opt unmask=ALL` still masking the path. [NO NEW TESTS NEEDED] Can't easily test this as we do not have access to it in CI. Signed-off-by: Matthew Heon <[email protected]>

mheon force-pushed the powercap_2 branch from 0568cdc to c6d410c Compare October 27, 2023 13:55

Luap99 approved these changes Oct 27, 2023

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 27, 2023

openshift-ci bot assigned Luap99 Oct 27, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 27, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 27, 2023

openshift-ci bot merged commit 6ec458d into containers:main Oct 27, 2023
99 checks passed

mheon mentioned this pull request Oct 27, 2023

[v4.7] Mask /sys/devices/virtual/powercap #20509

Merged

openshift-cherrypick-robot mentioned this pull request Oct 27, 2023

[v4.7] Do not add powercap mask if no paths are masked #20514

Merged

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 Feb 1, 2024

github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not add powercap mask if no paths are masked #20510

Do not add powercap mask if no paths are masked #20510

mheon commented Oct 27, 2023

mheon commented Oct 27, 2023

mheon commented Oct 27, 2023

Luap99 Oct 27, 2023

giuseppe left a comment

Luap99 left a comment

openshift-ci bot commented Oct 27, 2023

mheon commented Oct 27, 2023

openshift-cherrypick-robot commented Oct 27, 2023

mheon commented Oct 27, 2023

openshift-cherrypick-robot commented Oct 27, 2023

rhatdan commented Oct 27, 2023

mheon commented Oct 27, 2023

mheon commented Oct 27, 2023

openshift-cherrypick-robot commented Oct 27, 2023

mheon commented Oct 27, 2023

rhatdan commented Oct 27, 2023

mheon commented Oct 27, 2023

TomSweeneyRedHat commented Nov 2, 2023

Do not add powercap mask if no paths are masked #20510

Do not add powercap mask if no paths are masked #20510

Conversation

mheon commented Oct 27, 2023

Does this PR introduce a user-facing change?

mheon commented Oct 27, 2023

mheon commented Oct 27, 2023

Luap99 Oct 27, 2023

Choose a reason for hiding this comment

giuseppe left a comment

Choose a reason for hiding this comment

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 27, 2023

mheon commented Oct 27, 2023

openshift-cherrypick-robot commented Oct 27, 2023

mheon commented Oct 27, 2023

openshift-cherrypick-robot commented Oct 27, 2023

rhatdan commented Oct 27, 2023

mheon commented Oct 27, 2023

mheon commented Oct 27, 2023

openshift-cherrypick-robot commented Oct 27, 2023

mheon commented Oct 27, 2023

rhatdan commented Oct 27, 2023

mheon commented Oct 27, 2023

TomSweeneyRedHat commented Nov 2, 2023