Support xattr translation

[stefanberger]

This adds support for a treefile "xattr-translation" dictionary, and
will then translate extended attributes when they are committed into the
ostree commit.

This can be used for example to translate "user.ima" (which is a common
attribute used by for example ima-evm-utils) to "security.ima", which
would require root permissions to write to during compose. Add the
following to a treefile to achieve this:

xattr-translation:
  user.ima: security.ima

Signed-off-by: Patrick Uiterwijk [email protected]
Signed-off-by: Stefan Berger [email protected]

[openshift-ci-robot]

Hi @stefanberger. Thanks for your PR.

I'm waiting for a coreos member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[lucab]

/ok-to-test

[lucab]

Is there a strong need to have this xattr-translation field available to users since the beginning? To me it feels like a pretty low level knob, which can be used in many other messy ways. If we really want this logic to be configurable I'd rather attach it to some other IMA tunable, and eventually fold it down to a specific boolean gate.

[cgwalters]

We're giving a bit of conflicting review here, this came from
#2563 (comment)
ostreedev/ostree#2272 (comment)

So...I think we're all agreed that we want a higher level IMA bit, but supporting this allows easily iterating on it outside of the core for now.

[cgwalters]

Suggested change

	xattr_translation: Option<BTreeMap<String, serde_json::Value>>,
	xattr_translation: Option<BTreeMap<String, String>>,

Since what we're mapping here is xattr names it seems reasonable to constrain them both to UTF-8 strings. (ostree core supports both as bytestrings, but there's really no sane reason to use non-UTF8 xattr names IMO)

[jlebon]

Yeah, agreed. Unlike add_commit_metadata just above which has to use Value because it can be any type.

[cgwalters]

One sidechannel discussion we just had is whether since rofiles-fuse now supports directly setting xattrs, and we extend the allowlist to include the ima attribute, if any translation support is necessary.

I think even though rpm-ostree composes run privileged today, it will generally be a problem for "cross builds" if we try to directly set the privileged attribute on the build host. For example, today it works to run rpm-ostree in unprivileged podman - there we still can't set true security.* attributes I don't think. While for rpm-ostree I think we can assume we're always building from a Fedora-derived OS, that's definitely not true for ostree where it should work to build an ostree commit targeting Fedora from an Ubuntu host/container for example.

So I think this "map user. attribute" helps us avoid any dependencies on the host which is a good thing.

[jlebon]

So now that we have xattr support in rofiles-fuse (thanks for that!), I think nothing stops us from directly setting the privileged xattr, right? rpm-ostree composes today require privileged root anyway. So then, all we need to do for now to unblock experimenting is

diff --git a/src/libpriv/rpmostree-postprocess.cxx b/src/libpriv/rpmostree-postprocess.cxx
index fd56993e..6c4fe4c0 100644
--- a/src/libpriv/rpmostree-postprocess.cxx
+++ b/src/libpriv/rpmostree-postprocess.cxx
@@ -1262,6 +1262,7 @@ filter_xattrs_impl (OstreeRepo     *repo,
   /* If you have a use case for something else, file an issue */
   static const char *accepted_xattrs[] =
     { "security.capability", /* https://lwn.net/Articles/211883/ */
+      "security.ima",
       "user.pax.flags" /* https://github.com/projectatomic/rpm-ostree/issues/412 */
     };
   g_autoptr(GVariant) existing_xattrs = NULL;

That avoids having to cement this API in the treefile, which feels like we should discuss a bit more after fleshing out the client-side story (I'm not sure if for your use case or for IoT whether package layering is supported, but at the rpm-ostree level we should figure out how we want to support that in an IMA-enabled system, which likely will affect the server-side of this).

[cgwalters]

By analogy today, the way rpm-ostree handles SELinux is that we synthesize the label when committing the objects, we don't actually try to set the physical xattr on the files. For bare-user mode repos, we end up storing the xattrs as a user.ostreemeta xattr, so we don't have any dependency on trying to set security.selinux on physical files. And I think this is actually required for the reasons outlined in openshift/os#523

[jlebon]

Yeah, agreed. Unlike add_commit_metadata just above which has to use Value because it can be any type.

[jlebon]

Should this come before handling of accepted_xattrs? That way, you can translate towards something in the hardcoded list.

[stefanberger]

Thanks for getting back to me so quickly. So here are a couple of things :

• How do you test these things with rpm-ostree quickly? I am currently patching the cosa container and then build my fedora-coreos-config. The round trip times for making changes to the code are too long.
• I am writing the xattr user.ima since security.ima cannot be written to from within the cosa build container (when trying to set it with sudo setfattr)
• I have a working derivative of fedora-coreos-config that creates a QEMU image with IMA Appraisal enforcing file signatures. The first target was to sign files only in /usr/bin and use SELinux labels as selectors in the IMA policy rule, though due to the SELinux selector also files in other dirs have to be signed. My repo is here: https://github.com/stefanberger/fedora-coreos-config/commits/testing-devel
• I am using a postprocess script to sign the files. The good thing is it allows for lots of flexibility to choose the files to sign and to experiment with it. The bad thing is that one has to pass in a PEM-formatted private key to the postprocess script to be able to sign the files, though that problem would probably still exist if the signing was done in some other way. Evmctl requires a local key file.
• I don't mind hard-coding the translation of user.ima to security.ima and getting rid of the configurability via the treefile, if this helps.
• When RPMs starts carrying file signatures they will probably not apply (https://github.com/rpm-software-management/rpm/blob/master/plugins/ima.c#L71) and we'll have to adjust this plugin for it, assuming that it works in the rpm-ostree environment. Should it use an environment variable or just write user.ima upon failure to write security.ima ?

[cgwalters]

How do you test these things with rpm-ostree quickly? I am currently patching the cosa container and then build my fedora-coreos-config. The round trip times for making changes to the code are too long.

I run makesudoinstall from rpm-ostree git into a toolbox container in which I also did the same from coreos-assembler git. But you can also use coreos-assembler as a toolbox/dev container. Basically, set things up so the same container has access to both rpm-ostree git and the config git/cosa builddir.

A bit more here https://coreos.github.io/coreos-assembler/devel/

[cgwalters]

I don't mind hard-coding the translation of user.ima to security.ima and getting rid of the configurability via the treefile, if this helps.

Mmmm....I don't have a really strong opinion on this. A compromise position might be automatically translating user.ostree.security. to security.`?

That said also, an entirely separate path is to support this outside of generating a commit; something like a high level tool ostree-ima-sign --repo=/path/to/repo --commit=fedora/x86_64/coreos/testing-devel --key=/path/to/key.

That would maximize flexibility. And a further benefit is it would help for the CI versus delivery problem (CI won't have a key, or at least not the production key) etc.

[stefanberger]

Mmmm....I don't have a really strong opinion on this. A compromise position might be automatically translating user.ostree.security. to security.`?

It depends on what is writing the extended attributes. evmctl can either write security.ima or user.ima.

That said also, an entirely separate path is to support this outside of generating a commit; something like a high level tool ostree-ima-sign --repo=/path/to/repo --commit=fedora/x86_64/coreos/testing-devel --key=/path/to/key.

That would maximize flexibility. And a further benefit is it would help for the CI versus delivery problem (CI won't have a key, or at least not the production key) etc.

I am not sure what it would take to build ostre-ima-sign...

What should be the difference between the production build server and a CI server in terms of the config? Would they both use identical config files (tree file etc.) but only the key on the system would be different? Is there an existing example of how this has been done in the past?

Can environment variables (containing base64 encoded key for example) be passed to the cosa build container and from there into a postprocess script? That could be a possibility to pass in a key with the same mechanism but the key could be different on CI and production build servers.

[cgwalters]

OK, this is analogous in need to how we handle GPG signing of the commit object today. See this bit in the FCOS pipeline:
https://github.com/coreos/fedora-coreos-pipeline/blob/5c5b189a84a0aeed23bc927f3653888a9f3c32f0/Jenkinsfile#L288
Basically in between generating the ostree commit and generating the qemu image (from which all the other images are derived), we call out to the Fedora signing server as a distinct step.

That GPG signature in this case is just an additional commitmeta file that we append to the ostree tarball too.

Now, IMA is much more complex because we're signing lots of files. (There's also a further complication you touched on that in some cases we may want to reuse already extant signatures inside an RPM we downloaded (that will need work to do). But we can't rely solely on RPM signatures because not all of our content comes from RPM, and further at some point I'd like to support locally-generated signatures too. But we can ignore that for now)

I guess the tradeoff here is: If we do signatures as a secondary step, it's much cleaner because we don't need to hack in our signing script and key into postprocess. An external tool can set up the environment however it wants. (You could even imagine a pipeline just passes the whole ostree commit tarball to a separate system/container and receives a signed one back)
But the downside is that because in the ostree model, extended attributes are part of the object checksum, we will end up rewriting the ostree commit effectively. (Note that GPG signature will need to be done after this hence)

It's more efficient hence to generate the signatures as part of the single build process, but that requires having the key be present on the same container doing the build. I suspect the need for "signing services" are going to drive the architecture here towards us providing a distinct tool - or at least a clearly distinct phase.

[cgwalters]

(for the gpg implementation specific to Fedora/FCOS, see https://github.com/coreos/coreos-assembler/blob/master/src/cmd-sign )

[cgwalters]

I think my short term proposal is: let's just hardcode translating user.ima to security.ima - that will make it easier to experiment in this space and avoid blocking.

Now, using the ostree API it's completely possible to walk an ostree commit and generate a new one derived from it in a completely unprivileged fashion even (the same way one can generate gpg signatures as non-root). I can try to carve off a bit of time later today to demo code to do that and inject an arbitrary xattr on arbitrary files, then we can look at plugging it into evmctl sign.

[openshift-ci]

@stefanberger: PR needs rebase.

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.

[stefanberger]

I think my short term proposal is: let's just hardcode translating user.ima to security.ima - that will make it easier to experiment in this space and avoid blocking.

I can look into this but it will take a while even though it will only be a few lines. Most of the existing code will go..

[stefanberger]

Hardcoded translation now...

[stefanberger]

@cgwalters Patrick also has this patch here for ostree level xattr translation: puiterwijk/ostree@f89bbaf . I don't think he has sent a PR for it. Is it needed?

[cgwalters]

OK #2747 is some stub code that demonstrates rewriting an input ostree commit, injecting an arbitrary new xattr. We can now extend it to actually fork off evmctl - or even better, talk to the underlying C library directly and avoid re-parsing the certificate for each file, etc.

[cgwalters]

@stefanberger I believe that commit is no longer necessary.

[stefanberger]

OK #2747 is some stub code that demonstrates rewriting an input ostree commit, injecting an arbitrary new xattr. We can now stub out extending it to actually fork off evmctl - or even better, talk to the underlying C library directly and avoid re-parsing the certificate for each file, etc.

Ok. Unfortunately I am not much of a Rust expert.

The Rust code will need to calculate the sha256 hash over each file. It should probably then be calling this function here, if we ever wanted to pass passwords for keys:

int sign_hash(const char *hashalgo, const unsigned char *hash, int size, const char *keyfile, const char *keypass, unsigned char *sig)

https://sourceforge.net/p/linux-ima/ima-evm-utils/ci/master/tree/src/libimaevm.c#l973

The function we have to end up in is this one. It will parse the key every time, though.

static int sign_hash_v2(const char *algo, const unsigned char *hash,
			int size, const char *keyfile, unsigned char *sig)

(https://sourceforge.net/p/linux-ima/ima-evm-utils/ci/master/tree/src/libimaevm.c#l886)

There are some funny things going on with a global variable imaevm_params:

	log_info("hash(%s): ", imaevm_params.hash_algo);
	log_dump(hash, size);

	pkey = read_priv_pkey(keyfile, imaevm_params.keypass);
	if (!pkey)
		return -1;