Skip to content

Conversation

@mheon
Copy link
Member

@mheon mheon commented Aug 30, 2024

This is specifically for the IMA xattrs, which cannot be set except as root; as rootless, they fail with EPERM. We never noticed this before (likely because IMA xattrs seem uncommon in the wild) but if there is a file in an image with an IMA xattr rootless Podman becomes completely unable to use the image. This is particularly relevant because the catatonit binary Podman uses for building its pause image has started to include an IMA xattr on Fedora Rawhide, which is breaking rootless Podman there rather badly.

Since this cannot work as rootless, it seems simplest to try to set the xattr, but tolerate failure iff the error is EPERM and we are not run as root.

Fixes: containers/podman#18543

This is specifically for the IMA xattrs, which cannot be set
except as root; as rootless, they fail with EPERM. We never
noticed this before (likely because IMA xattrs seem uncommon in
the wild) but if there is a file in an image with an IMA xattr
rootless Podman becomes completely unable to use the image. This
is particularly relevant because the catatonit binary Podman uses
for building its pause image has started to include an IMA xattr
on Fedora Rawhide, which is breaking rootless Podman there rather
badly.

Since this cannot work as rootless, it seems simplest to try to
set the xattr, but tolerate failure iff the error is EPERM and
we are not run as root.

Fixes: containers/podman#18543

Signed-off-by: Matt Heon <[email protected]>
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 30, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mheon
Once this PR has been reviewed and has the lgtm label, please assign nalind for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@mheon
Copy link
Member Author

mheon commented Aug 30, 2024

I don't know if we can reasonably test this in CI as any test run without root will be unable to create a file with the right xattr. I suppose upgrading the CI VMs to F41 once that comes out (or ensuring rpm-plugin-ima is installed in them now?) would allow testing.

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

Does this work end-to-end?

I didn’t deeply investigate the failure code path, and this does match with the claim that #657 introduced the problem, still:

  • The PR description clearly doesn’t match the actual operation: this ignores EPERM reading, not setting attributes
  • Reading containers/podman#18543 , in there it seems that we did succeed reading the attribute, at least enough to want to set it on a file, but failed setting it (setting value of extended attribute )

for _, xattr := range []string{"security.capability", "security.ima"} {
capability, err := system.Lgetxattr(path, xattr)
if err != nil && !errors.Is(err, system.EOPNOTSUPP) && err != system.ErrNotSupportedPlatform {
if err != nil && !errors.Is(err, system.EOPNOTSUPP) && err != system.ErrNotSupportedPlatform && !(unshare.IsRootless() && errors.Is(err, syscall.EPERM)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we should have the exception here, I think, it would be better to scope it to security.ima only; if we are asked to represent a set-capability executable and we can’t do that, I think the operation should fail.

@rhatdan
Copy link
Member

rhatdan commented Sep 5, 2024

@mheon Still working on this one?

@mheon
Copy link
Member Author

mheon commented Sep 5, 2024

Yes, this is the wrong implementation, rewriting.

@mheon mheon closed this Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error setting extended attributes on "/catatonit"

3 participants