Do not error on trying to write IMA xattr as rootless #5741

mheon · 2024-09-17T12:52:40Z

Rootless users cannot set the security.ima xattr on files (presumably for security reasons, they get an EPERM on trying to do so). We will normally try and preserve that xattr, so when trying to add a file with an IMA xattr to a build on a Buildah without this patch, you get an error. With this patch, the error is downgraded to a warning, as it's better to successfully build with a missing xattr than blocking all builds which want to include the offending file.

The urgency on this has become somewhat higher as it seems like F41/Rawhide are installing rpm-plugin-ima by default, which is setting IMA xattrs on some files that Podman relies on - for example, the catatonit binary we use for pid pause images. Without this patch, building the pause image as rootless will always fail on a system with rpm-plugin-ima installed.

Fixes: containers/podman#18543

What type of PR is this?

/kind bug

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Including a file with the `security.ima` xattr set as a non-root user will no longer fail as the xattr could not be set.

mheon · 2024-09-17T12:55:12Z

I don't know if testing this is particularly possible in CI, as it would require the file to be set up as root, and then an integration test run as a non-root user to verify the functionality. Could be done by including the test file in the VM images, maybe?

rhatdan · 2024-09-17T13:18:06Z

LGTM

edsantiago · 2024-09-17T13:26:26Z

For testing purposes only, feel free to use c20240826t190000z-f40f39d13 (built in containers/automation_images#384). Those images have the default catatonit, i.e., do not include the Cirrus F41 workaround. See that PR diff for more.

Please do not merge with those VM images, they are old. I will be happy to build new ones if such is desired.

mheon · 2024-09-18T17:46:37Z

@edsantiago The Podman package is installed in the test VMs, right? That should have dragged in catatonit, which we know has IMA xattrs we can test on.

@nalind Are any of the existing integration tests run rootless?

edsantiago · 2024-09-18T17:52:48Z

@mheon please see my comment above. TL;DR we have a kludge workaround in test VMs, where we strip IMA. Without that, we would have no CI. The test images I mention in my comment above are built without that workaround, so you can use them in your testing.

nalind · 2024-09-18T17:58:35Z

@nalind Are any of the existing integration tests run rootless?

Yes, there are rootless test jobs.

mheon · 2024-09-18T18:24:12Z

@edsantiago Mind taking a look at the test I added?

edsantiago

LGTM with a few questions and nits. Suggestion:

@test "bud with ADD with file with IMA xattr" {
  _prefetch alpine

  local catatonit=/usr/libexec/catatonit/catatonit
  if [[ ! -e $catatonit ]]; then
      skip "Catatonit path $catatonit not found"
  fi

  run getfattr -d -n security.ima $catatonit
  echo "getfattr $catatonit -> '$output'"
  if [[ $status -ne 0 ]]; then
      skip "pointless: $catatonit does not have the desired xattr"
  fi

  local contextdir=${TEST_SCRATCH_DIR}/add-ima
  mkdir -p $contextdir
  cat > $contextdir/Dockerfile <<EOF
FROM alpine
ADD $catatonit /bin/catatonit
EOF

  # We do not care if the attribute was actually added, as rootless is allowed to discard it.
  # Only that the file builds successfully.
  run_buildah build $contextdir
}

Do we know if the attr package is installed in CI VMs?

edsantiago · 2024-09-18T18:44:59Z

tests/bud.bats

+
+  # Verify that /usr/libexec/catatonit/catatonit both exists and has an IMA xattr
+  if getfattr -d -m 'security.ima' /usr/libexec/catatonit/catatonit | grep -q ima; then
+    skip "catatonit does not exist or does not have an appropriate xattr"


I'm not a big fan of ambiguous skips or test failures. If I were looking at this skip, I'd want to know more.

edsantiago · 2024-09-18T18:45:59Z

tests/bud.bats

+EOF
+
+  # Verify that /usr/libexec/catatonit/catatonit both exists and has an IMA xattr
+  if getfattr -d -m 'security.ima' /usr/libexec/catatonit/catatonit | grep -q ima; then


Did you mean to use -m (match)? Not the tighter -n (name)?

edsantiago · 2024-09-18T18:46:33Z

tests/bud.bats

+
+  # We do not care if the attribute was actually added, as rootless is allowed to discard it.
+  # Only that the file builds successfully.
+  run_buildah build -f $contextdir/Dockerfile $contextdir


Can probably ditch the -f Dockerfile and save some duplication

edsantiago · 2024-09-18T20:03:52Z

Welp. (1) Buildah CI doesn't test on rawhide; (2) Debian doesn't have attr; (3) f40 is failing in a way that makes no sense to me

nalind · 2024-09-18T20:19:08Z

The current iteration of the test doesn't ensure that there's a /usr/libexec/catatonit/catatonit under the build context directory. I'd suggest either copying it first, then checking for the extended attribute, or reworking the test to use buildah add, which should hit the same code paths.

mheon · 2024-09-19T13:48:56Z

@nalind Can't copy it, that strips the xattr. I'll rework to use buildah add.

nalind · 2024-09-19T15:00:59Z

/lgtm
This doesn't address cases where the a file with this xattr is part of a base image, but we don't control that in this repository.

mheon · 2024-09-19T15:09:07Z

I'm inclined to just let those error for now - we've never heard of a case where that has actually happened.

mheon · 2024-09-19T15:17:05Z

/cherry-pick v1.37

openshift-cherrypick-robot · 2024-09-19T15:17:08Z

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

In response to this:

/cherry-pick v1.37

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.

nalind · 2024-09-19T15:23:11Z

/approve
/cherry-pick release-1.37

openshift-cherrypick-robot · 2024-09-19T15:23:14Z

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

In response to this:

/approve
/cherry-pick release-1.37

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 · 2024-09-19T15:23:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, mheon, nalind

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~~ [nalind]

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

nalind · 2024-09-19T15:24:52Z

tests/add.bats

@@ -321,3 +321,16 @@ stuff/mystuff"
  run_buildah 125 add --retry-delay=0.142857s --retry=14 --cert-dir ${TEST_SCRATCH_DIR} $cid https://localhost:${HTTP_SERVER_PORT}/randomfile
  assert "$output" =~ "retrying in 142.*ms .*14/14.*"
 }
+
+@test "add file with IMA xattr" {
+    if getfattr -d -n 'security.ima' /usr/libexec/catatonit/catatonit | grep -q ima; then


Suggested change

if getfattr -d -n 'security.ima' /usr/libexec/catatonit/catatonit | grep -q ima; then

if ! getfattr -d -n 'security.ima' /usr/libexec/catatonit/catatonit | grep -q ima; then

Also note that this will not be tested in podman CI, because that only runs bud.bats. When this is vendored into podman, a new test will be required.

Rootless users cannot set the `security.ima` xattr on files (presumably for security reasons, they get an EPERM on trying to do so). We will normally try and preserve that xattr, so when trying to add a file with an IMA xattr to a build on a Buildah without this patch, you get an error. With this patch, the error is downgraded to a warning, as it's better to successfully build with a missing xattr than blocking all builds which want to include the offending file. The urgency on this has become somewhat higher as it seems like F41/Rawhide are installing rpm-plugin-ima by default, which is setting IMA xattrs on some files that Podman relies on - for example, the catatonit binary we use for pid pause images. Without this patch, building the pause image as rootless will always fail on a system with rpm-plugin-ima installed. Fixes: containers/podman#18543 Signed-off-by: Matt Heon <[email protected]>

nalind · 2024-09-19T17:41:32Z

/lgtm

openshift-cherrypick-robot · 2024-09-19T17:44:40Z

@mheon: cannot checkout v1.37: error checking out "v1.37": exit status 1 error: pathspec 'v1.37' did not match any file(s) known to git

In response to this:

/cherry-pick v1.37

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-cherrypick-robot · 2024-09-19T17:45:19Z

@nalind: #5741 failed to apply on top of branch "release-1.37":

Applying: Do not error on trying to write IMA xattr as rootless
Using index info to reconstruct a base tree...
M	.cirrus.yml
M	copier/xattrs.go
M	tests/add.bats
Falling back to patching base and 3-way merge...
Auto-merging tests/add.bats
CONFLICT (content): Merge conflict in tests/add.bats
Auto-merging copier/xattrs.go
Auto-merging .cirrus.yml
CONFLICT (content): Merge conflict in .cirrus.yml
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 error on trying to write IMA xattr as rootless
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:

/approve
/cherry-pick release-1.37

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.

mheon · 2024-09-19T18:47:10Z

@edsantiago We should consider new VM builds once this lands in Podman. Not super urgent though.

mheon force-pushed the fix_podman_18543 branch from 9a4b3b8 to 412b48f Compare September 17, 2024 16:58

mheon force-pushed the fix_podman_18543 branch from 412b48f to 5d73b93 Compare September 18, 2024 18:24

edsantiago approved these changes Sep 18, 2024

View reviewed changes

mheon force-pushed the fix_podman_18543 branch from 5d73b93 to 9b2cfce Compare September 19, 2024 13:55

openshift-ci bot assigned nalind Sep 19, 2024

openshift-ci bot added the lgtm label Sep 19, 2024

openshift-ci bot added the approved label Sep 19, 2024

nalind reviewed Sep 19, 2024

View reviewed changes

mheon force-pushed the fix_podman_18543 branch from 9b2cfce to 5e82f27 Compare September 19, 2024 15:45

openshift-ci bot removed the lgtm label Sep 19, 2024

openshift-ci bot added the lgtm label Sep 19, 2024

openshift-merge-bot bot merged commit d8a9a87 into containers:main Sep 19, 2024
33 checks passed

mheon mentioned this pull request Sep 19, 2024

[release-1.37] Do not error on trying to write IMA xattr as rootless #5746

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not error on trying to write IMA xattr as rootless #5741

Do not error on trying to write IMA xattr as rootless #5741

mheon commented Sep 17, 2024 •

edited

Loading

mheon commented Sep 17, 2024

rhatdan commented Sep 17, 2024

edsantiago commented Sep 17, 2024

mheon commented Sep 18, 2024

edsantiago commented Sep 18, 2024

nalind commented Sep 18, 2024

mheon commented Sep 18, 2024

edsantiago left a comment

edsantiago Sep 18, 2024

edsantiago Sep 18, 2024

edsantiago Sep 18, 2024

edsantiago commented Sep 18, 2024

nalind commented Sep 18, 2024

mheon commented Sep 19, 2024

nalind commented Sep 19, 2024

mheon commented Sep 19, 2024

mheon commented Sep 19, 2024

openshift-cherrypick-robot commented Sep 19, 2024

nalind commented Sep 19, 2024

openshift-cherrypick-robot commented Sep 19, 2024

openshift-ci bot commented Sep 19, 2024

nalind Sep 19, 2024

edsantiago Sep 19, 2024

nalind commented Sep 19, 2024

openshift-cherrypick-robot commented Sep 19, 2024

openshift-cherrypick-robot commented Sep 19, 2024

mheon commented Sep 19, 2024

	if getfattr -d -n 'security.ima' /usr/libexec/catatonit/catatonit \| grep -q ima; then
	if ! getfattr -d -n 'security.ima' /usr/libexec/catatonit/catatonit \| grep -q ima; then

Do not error on trying to write IMA xattr as rootless #5741

Do not error on trying to write IMA xattr as rootless #5741

Conversation

mheon commented Sep 17, 2024 • edited Loading

What type of PR is this?

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

mheon commented Sep 17, 2024

rhatdan commented Sep 17, 2024

edsantiago commented Sep 17, 2024

mheon commented Sep 18, 2024

edsantiago commented Sep 18, 2024

nalind commented Sep 18, 2024

mheon commented Sep 18, 2024

edsantiago left a comment

Choose a reason for hiding this comment

edsantiago Sep 18, 2024

Choose a reason for hiding this comment

edsantiago Sep 18, 2024

Choose a reason for hiding this comment

edsantiago Sep 18, 2024

Choose a reason for hiding this comment

edsantiago commented Sep 18, 2024

nalind commented Sep 18, 2024

mheon commented Sep 19, 2024

nalind commented Sep 19, 2024

mheon commented Sep 19, 2024

mheon commented Sep 19, 2024

openshift-cherrypick-robot commented Sep 19, 2024

nalind commented Sep 19, 2024

openshift-cherrypick-robot commented Sep 19, 2024

openshift-ci bot commented Sep 19, 2024

nalind Sep 19, 2024

Choose a reason for hiding this comment

edsantiago Sep 19, 2024

Choose a reason for hiding this comment

nalind commented Sep 19, 2024

openshift-cherrypick-robot commented Sep 19, 2024

openshift-cherrypick-robot commented Sep 19, 2024

mheon commented Sep 19, 2024

mheon commented Sep 17, 2024 •

edited

Loading