add container_ro_file_t to the podman artifact store #360

Luap99 · 2025-02-24T13:56:05Z

The artifact store contains many blobs which we like to get mounted into the container. However by default selinux prevents access to these files. Now I could patch podman to change the label directly but it seems like much better to just label the entire artifact store correctly like we do with the image store.

Summary by Sourcery

Add container_ro_file_t to the podman artifact store to allow containers to access the blobs, fixing selinux denial errors.

sourcery-ai · 2025-02-24T13:56:11Z

Reviewer's Guide by Sourcery

This pull request adds SELinux labeling for the podman artifact store, allowing containers to access files stored within. It modifies container.fc and container.if to include the container_ro_file_t type for the artifact store.

No diagrams generated as the changes look simple and do not need a visual representation.

File-Level Changes

Change	Details	Files
Added SELinux labeling for the podman artifact store.	Added `container_ro_file_t` to the artifact store in `container.fc`. Modified `container.if` to include the new type.	`container.fc` `container.if`

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with @sourcery-ai issue to create an issue from it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time. You can also comment
@sourcery-ai title on the pull request to (re-)generate the title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment @sourcery-ai summary on the pull request to
(re-)generate the summary at any time.
Generate reviewer's guide: Comment @sourcery-ai guide on the pull
request to (re-)generate the reviewer's guide at any time.
Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore.
Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment
@sourcery-ai review to trigger a new review!
Generate a plan of action for an issue: Comment @sourcery-ai plan on
an issue to generate a plan of action for it.

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

Luap99 · 2025-02-24T13:58:29Z

@rhatdan @lsm5 PTAL
Note I have no idea what I am doing here basically, I just noticed while working on my artifact mount PR that it does not work with selinux enabled, I think the labels of the artifacts should match the ones from the normal image store.

But I have no idea about the selinux policy syntax so I have no idea if this is right here or something else is needed.

rhatdan · 2025-02-24T14:07:08Z

LGTM

lsm5 · 2025-02-24T14:33:14Z

@rhatdan @lsm5 PTAL Note I have no idea what I am doing here basically, I just noticed while working on my artifact mount PR that it does not work with selinux enabled, I think the labels of the artifacts should match the ones from the normal image store.

But I have no idea about the selinux policy syntax so I have no idea if this is right here or something else is needed.

LGTM. @Luap99 have you also verified your mount PR works with the rpms installed from the copr jobs here?

Luap99 · 2025-02-24T14:36:37Z

@Luap99 have you also verified your mount PR works with the rpms installed from the copr jobs here?

Not yet which is why I opened it as draft, I will try it on my test VMs later.

lsm5 · 2025-02-24T14:44:05Z

@Luap99 have you also verified your mount PR works with the rpms installed from the copr jobs here?

Not yet which is why I opened it as draft, I will try it on my test VMs later.

Ack. Once TMT tests are merged into podman, this should become easier with https://packit.dev/docs/configuration/upstream/tests#running-tests-with-builds-from-another-pull-request

Luap99 · 2025-02-25T15:48:34Z

Ok one problem is that I used the wrong root path, but even then this doesn't work, with container-selinux-2.235.0-1.20250224135753546780.pr360.2.ga341b1f.fc43.noarch from the packit task it doesn't label the dirs by default

# ls -lZ /var/lib/containers/storage/artifacts/
total 8
drwxr-xr-x. 3 root root unconfined_u:object_r:container_var_lib_t:s0  20 Feb 25 16:32 blobs
-rw-r--r--. 1 root root unconfined_u:object_r:container_var_lib_t:s0 338 Feb 25 16:32 index.json
-rw-r--r--. 1 root root unconfined_u:object_r:container_var_lib_t:s0  30 Feb 25 16:32 oci-layout

$ ls -lZ ~/.local/share/containers/storage/artifacts/
total 8
drwxr-xr-x. 3 paul paul unconfined_u:object_r:data_home_t:s0  20 Feb 25 16:35 blobs
-rw-r--r--. 1 paul paul unconfined_u:object_r:data_home_t:s0 338 Feb 25 16:35 index.json
-rw-r--r--. 1 paul paul unconfined_u:object_r:data_home_t:s0  30 Feb 25 16:35 oci-layout

How is selinux labelling supposed to work? My assumption was that this definition would cause all files to be created in mentioned paths to be labelled container_ro_file_t? I was able to force the label with restorecon but that of course should not be required. Is podman required to manually relabel all files all the time?

lsm5 · 2025-02-26T11:55:07Z

Needs a change to container.if as well. Please retry with this diff:

$ git diff
diff --git a/container.if b/container.if
index 3914e0f..46466ae 100644
--- a/container.if
+++ b/container.if
@@ -537,6 +537,7 @@ interface(`container_filetrans_named_content',`
     #  workdir=/var/lib/containerd/io.containerd.snapshotter.v1.overlayfs/snapshots/4/work)
     filetrans_pattern($1, container_var_lib_t, container_file_t, dir, "snapshots")
     filetrans_pattern($1, container_var_lib_t, container_ro_file_t, dir, "init")
+    filetrans_pattern($1, container_var_lib_t, container_ro_file_t, dir, "artifacts")
     filetrans_pattern($1, container_var_lib_t, container_ro_file_t, dir, "overlay")
     filetrans_pattern($1, container_var_lib_t, container_ro_file_t, dir, "overlay-images")
     filetrans_pattern($1, container_var_lib_t, container_ro_file_t, dir, "overlay-layers")
@@ -563,6 +564,7 @@ interface(`container_filetrans_named_content',`
     filetrans_pattern($1, container_var_lib_t, container_ro_file_t, dir, "containerd-soci")

     filetrans_pattern($1, data_home_t, container_ro_file_t, dir, "ramalama")
+    filetrans_pattern($1, data_home_t, container_ro_file_t, dir, "artifacts")
     filetrans_pattern($1, data_home_t, container_ro_file_t, dir, "overlay")
     filetrans_pattern($1, data_home_t, container_ro_file_t, dir, "overlay-images")
     filetrans_pattern($1, data_home_t, container_ro_file_t, dir, "overlay-layers")

@rhatdan PTAL too.

Luap99 · 2025-02-26T12:23:23Z

@lsm5 Thanks, I will test again with the new rpm builds.

For reference here is the podman PR to mount artifacts: containers/podman#25397

container.if

Luap99 · 2025-02-26T13:07:37Z

Ok this works both rootful and rootless now, thanks @lsm5

rhatdan · 2025-02-26T15:42:08Z

LGTM

sourcery-ai

Hey @Luap99 - I've reviewed your changes - here's some feedback:

Overall Comments:

Could you provide more context on why the default SELinux policy prevents access to these files?
It would be helpful to understand the specific security implications of labeling the entire artifact store.

Here's what I looked at during the review

🟢 General issues: all looks good
🟢 Security: all looks good
🟢 Testing: all looks good
🟢 Complexity: all looks good
🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

openshift-ci · 2025-02-26T15:42:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, 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:

~~OWNERS~~ [rhatdan]

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

rhatdan · 2025-02-26T15:45:12Z

@Luap99 For some reason github will not allow me to merge without a rebase.

The artifact store contains many blobs which we like to get mounted into the container. However by default selinux prevents access to these files. Now I could patch podman to change the label directly but it seems like much better to just label the entire artifact store correctly like we do with the image store. Signed-off-by: Paul Holzinger <[email protected]>

lsm5 · 2025-02-27T15:45:13Z

/cherrypick rhaos-maint

openshift-cherrypick-robot · 2025-02-27T15:45:51Z

@lsm5: new pull request created: #363

In response to this:

/cherrypick rhaos-maint

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-sigs/prow repository.

openshift-ci bot added the do-not-merge/work-in-progress label Feb 24, 2025

Luap99 force-pushed the artifacts branch from a341b1f to dce05a5 Compare February 25, 2025 15:51

Luap99 mentioned this pull request Feb 25, 2025

add artifact mount support containers/podman#25397

Merged

Luap99 force-pushed the artifacts branch from dce05a5 to 02222fc Compare February 26, 2025 12:22

lsm5 reviewed Feb 26, 2025

View reviewed changes

container.if Outdated Show resolved Hide resolved

Luap99 force-pushed the artifacts branch from 02222fc to f0bfae3 Compare February 26, 2025 13:03

rhatdan marked this pull request as ready for review February 26, 2025 15:41

openshift-ci bot removed the do-not-merge/work-in-progress label Feb 26, 2025

sourcery-ai bot reviewed Feb 26, 2025

View reviewed changes

rhatdan approved these changes Feb 26, 2025

View reviewed changes

openshift-ci bot added the approved label Feb 26, 2025

Luap99 force-pushed the artifacts branch from f0bfae3 to c318a63 Compare February 26, 2025 16:03

rhatdan merged commit 28a89f7 into containers:main Feb 26, 2025
28 of 29 checks passed

Luap99 deleted the artifacts branch February 26, 2025 16:42

openshift-cherrypick-robot mentioned this pull request Feb 27, 2025

[rhaos-maint] add container_ro_file_t to the podman artifact store #363

Merged

add container_ro_file_t to the podman artifact store #360

add container_ro_file_t to the podman artifact store #360

Uh oh!

Conversation

Luap99 commented Feb 24, 2025 • edited by sourcery-ai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by Sourcery

Uh oh!

sourcery-ai bot commented Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer's Guide by Sourcery

File-Level Changes

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

Luap99 commented Feb 24, 2025

Uh oh!

rhatdan commented Feb 24, 2025

Uh oh!

lsm5 commented Feb 24, 2025

Uh oh!

Luap99 commented Feb 24, 2025

Uh oh!

lsm5 commented Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Luap99 commented Feb 25, 2025

Uh oh!

lsm5 commented Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Luap99 commented Feb 26, 2025

Uh oh!

Uh oh!

Luap99 commented Feb 26, 2025

Uh oh!

rhatdan commented Feb 26, 2025

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Feb 26, 2025

Uh oh!

rhatdan commented Feb 26, 2025

Uh oh!

Uh oh!

lsm5 commented Feb 27, 2025

Uh oh!

openshift-cherrypick-robot commented Feb 27, 2025

Uh oh!

Uh oh!

Luap99 commented Feb 24, 2025 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Feb 24, 2025 •

edited

Loading

lsm5 commented Feb 24, 2025 •

edited

Loading

lsm5 commented Feb 26, 2025 •

edited

Loading