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

Ignore security.ima xattrs #1608

Closed
wants to merge 1 commit into from
Closed

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented May 19, 2023

@rhatdan
Copy link
Member Author

rhatdan commented May 19, 2023

I am not sure this is the right thing to do, but if this does not work in rootless mode, I think we should create the same images in both rootful and rootless mode.
@nalind @vrothberg @Luap99 @giuseppe @mtrmac @flouthoc WDYT?

@Luap99
Copy link
Member

Luap99 commented May 22, 2023

containers/buildah#2127 explicitly wanted them to be copied, although it is not exactly clear to me if this was only ever used as root. It may be better to only skip it as rootless if it doesn't work rootless.

@giuseppe
Copy link
Member

I don't think we should ignore them. To address the specific issue, we can skip these xattrs when we copy the catatonit executable, but not in general

@AmedeeBulle
Copy link

The thing is that you need root privileges to be able to copy security.ima attributes, which prevents building rootless containers with anything having ima xattrs...

IMHO, rootfull should fail in case of error; rootless might issue a warning, but pass.

While catatonit is the main issue here, I guess we will run into the same issue when COPYing any file with ima xattr into the container when run rootless...

@mtrmac
Copy link
Collaborator

mtrmac commented May 22, 2023

So we need to drop the attributes when pulling and running a container rootless (because we can’t se the attribute rootless), but we must not drop when pulling and building a derived image (because they are a required part of the built image)?

One immediate difficulty is that often, at times of a pull, we don’t know whether that image is going to be run or built upon.

Another is that many builds involve running containers on that image, i.e. rootless is just in an impossible situation that must fail if we store the attribute natively.


Do we need to think about “virtualizing” the extended attribute, recording it in some c/storage metadata but not on the kernel-visible backing filesystem? That would at least allow us to take an image with IMA attributes, make a small change (edit a config file), and push that build image, rootless. For full rootless builds, including things like dnf install of more package with IMA attributes, we would need a full user-space emulation of that attribute (FUSE?).

@rhatdan
Copy link
Member Author

rhatdan commented May 22, 2023

How about we add a flag indicating whether or not you care about IMA labels. Default to fail as we do now. But allow users to set list of ignores. Move the list of ignored XAttrs to storage.conf, default to selinux.

@mtrmac
Copy link
Collaborator

mtrmac commented May 22, 2023

Yes, that would work.

I suppose for the catatonit binary we would still want to try using it rootless? Or do we need to somehow completely change the approach here? (hard-link the binary to preserve the existing root-set attribute? Not have a pause container at all, and run a non-container process?)

@giuseppe
Copy link
Member

Yes, that would work.

I suppose for the catatonit binary we would still want to try using it rootless? Or do we need to somehow completely change the approach here? (hard-link the binary to preserve the existing root-set attribute? Not have a pause container at all, and run a non-container process?)

for that specific case we don't care about IMA at all. We only need a known binary that can keep the infra container alive. We are using catatonit because it is the only static binary we can be sure about, but it could be anything. Maybe once wasm is more available, we can replace it with a hardcoded wasm payload, so that it is 1) static 2) works on all platforms.

@mtrmac
Copy link
Collaborator

mtrmac commented May 23, 2023

Do we have the option of not caring about IMA if the kernel is configured to enforce it? Or is that something that just doesn’t work and isn’t worth worrying about?

@cgwalters
Copy link
Contributor

Do we need to think about “virtualizing” the extended attribute, recording it in some c/storage metadata but not on the kernel-visible backing filesystem?

This exists today in tar-split right?

@nalind
Copy link
Member

nalind commented Jun 20, 2023

Do we need to think about “virtualizing” the extended attribute, recording it in some c/storage metadata but not on the kernel-visible backing filesystem?

This exists today in tar-split right?

The tar headers, which describe everything about the layer except the contents of non-zero-length files, are preserved so that we can reconstruct the uncompressed version of the layer diff, but that's pretty much all we can do with it, since it's not stored in a seekable format.

@cgwalters
Copy link
Contributor

cgwalters commented Jun 20, 2023

Ah, right. So what ostree does related to this topic is use an user.ostreemeta xattr (which recursively then contains things like target xattrs, serialized as a gvariant).

But the equivalent would be storing the tar-split header as a user. xattr.

EDIT: Though this is tricky because tar-split is also serializing the order of files and exactly how their names appear in the tar stream, so we'd still need to save that out of band.

@cgwalters
Copy link
Contributor

cgwalters commented Jun 20, 2023

BTW related to this, and definitely related to the existing flow where security.selinux is ignored - for the ostree-container aka "bootable container", we absolutely need extended attributes to be reliably preserved in container transports. So what we did for now instead of writing them into the tar stream is to serialize these xattrs into a special regular file, see this code.

And podman run --rm -ti quay.io/fedora/fedora-coreos:stable ls -al /ostree/repo/objects/00.

On the client side we deserialize these back.

So basically every ostree feature related to xattrs (which also includes IMA) works, even when transporting container images around.

@nalind
Copy link
Member

nalind commented Jun 20, 2023

BTW related to this, and definitely related to the existing flow where security.selinux is ignored - for the ostree-container aka "bootable container", we absolutely need extended attributes to be reliably preserved in container transports. So what we did for now instead of writing them into the tar stream is to serialize these xattrs into a special regular file, see this code.

Hold up, are you saying that xattrs are not present in tar headers in layer diffs that ostree creates?

@cgwalters
Copy link
Contributor

Hold up, are you saying that xattrs are not present in tar headers in layer diffs that ostree creates?

Yes; I didn't trust that we could reliably get the security.selinux xattr through a pull-build-repush cycle across all container runtimes and versions. Maybe that was being overly conservative though.

@nalind
Copy link
Member

nalind commented Jun 20, 2023

Hold up, are you saying that xattrs are not present in tar headers in layer diffs that ostree creates?

Yes; I didn't trust that we could reliably get the security.selinux xattr through a pull-build-repush cycle across all container runtimes and versions. Maybe that was being overly conservative though.

That information needs to also be in the headers, though, so that containers/storage can decide whether or not to apply it to layer content that it's extracting. The alternative is binaries like newuidmap not working as described in a container because security.capability got discarded.

@cgwalters
Copy link
Contributor

The alternative is binaries like newuidmap not working as described in a container because security.capability got discarded.

So far though, there hasn't been a real use case to execute file-capability binaries inside an ostree-container base image. The use case for running these as a container e.g. with podman/docker is solely to use them as a "build environment"; and there's no sane use case for running any file capabilities there - it doesn't even work outside of a privileged container (or at least --cap-add) anyways right?

The other problem strongly related to this is that in a default unprivileged Containerfile/Dockerfile style build flow, we don't allow the build process to set some of these critical xattrs because the process inherently conflates build environment == target environment. Trying to set e.g. security.selinux just can't work in general because the host policy may differ from the target system, and the host system will want to deny setting invalid or unsupported xattrs. I think IMA and other things in the security. and trusted. space have similar problems.

So use of these xattrs in container image flows (outside of base images built externally or at least with a src != target style flow) is uncommon in my experience.

Ultimately what we may want is a way to "remap" xattrs in overlayfs more natively. What would actually be nice is that the default of podman run busybox ls -Z / didn't show any security.selinux xattr at all - it'd be there on the inode in memory but not presented to userspace. But if one was set by userspace it'd be persisted (just not honored natively by the kernel) and then when we go to serialize the layer tar we'd capture what was set in the build.

@nalind
Copy link
Member

nalind commented Jun 21, 2023

The alternative is binaries like newuidmap not working as described in a container because security.capability got discarded.

So far though, there hasn't been a real use case to execute file-capability binaries inside an ostree-container base image. The use case for running these as a container e.g. with podman/docker is solely to use them as a "build environment"; and there's no sane use case for running any file capabilities there - it doesn't even work outside of a privileged container (or at least --cap-add) anyways right?

I won't try to enumerate all of the ways that file capabilities could be made relevant at run-time. I can confirm that we've had a bug report about rpm -V complaining about missing capabilities (RHBZ#1867598), which was resolved by adding a workaround in the process that builds RHEL-based images. We still have a workaround in place in buildah's own official images, however, since they're based on Fedora images. And while it doesn't specify how, the OCI spec seems pretty clear that it wants that information to be present for content that's added or modified by layers.

@cgwalters
Copy link
Contributor

had a bug report about rpm -V complaining about missing capabilities (RHBZ#1867598),

Yes...but here's the thing; the file capability xattr i.e. security.capability is already namespaced in the kernel. That's not true of most (or really any?) of the other security. xattrs honored by LSMs to my knowledge. Definitely not security.selinux or security.ima (although I know they've both been discussed).

It's also interesting to note here that for rpm (and what spawned this bug report) is that rpm doesn't treat security.selinux or security.ima in a "first class" way - the're deferred to plugins. rpm -V doesn't verify security.selinux - if it did, it'd totally fail with how w do labeling with container runtimes today.

And while it doesn't specify how, the OCI spec seems pretty clear that it wants that information to be present for content that's added or modified by layers.

I think everyone roughly agrees on this...but the problem is we need to decouple 3 things in the general case:

  • What is transported on the wire
  • What is actually set on the in-kernel inodes
  • What is presented to the container userspace

(Decoupling the first two is the biggest fix)

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"
7 participants