AppArmor: Support for loading a pre-compiled profile #15905

YmrDtnJu · 2020-05-25T08:58:57Z

Let systemd load a pre-compiled AppArmor profile file from either
/etc/systemd/profile.apparmor if that file exists or
/usr/lib/systemd/profile.apparmor. If neither file exists, systemd will not
load anything and will also not log this at all.

The pre-compiled profile file needs to contain a profile with name systemd.
systemd will change to that profile after loading the file. If the file does
not contain a profile with that name, systemd will log an error.

If systemd is already confined in a profile, it will not load the profile file
and not change to any profile.

If anything goes wrong, systemd will only log errors or warnings. It will not
fail to boot.

Things that may need to be discussed:

What if loading the profile file or changing to the profile fails? Currently, the patch just continues booting.
Should this be documented anywhere?
Are the file names and paths for the profile files ok?

A pre-compiled AppArmor profile file can be created using the --ofile option of apparmor_parser.

The idea behind this is, that the kernel does not provide any method of loading a profile file at startup. One has to use an initrd to load the profile file and let start systemd with the profile. If you don't have an initrd (e.g. for VMs that don't need one) you cannot confine systemd itself in an AppArmor profile. With this patch it is possible to do so.

poettering

looks OK, but a couple of comments

src/core/apparmor-setup.c

poettering · 2020-05-25T13:35:03Z

src/core/apparmor-setup.c

+
+        r = read_one_line_file("/proc/self/attr/apparmor/current", &current_profile);
+        if (r < 0) {
+                if (errno == ENOENT) {


error is returned in r, not errno, i.e. if (r == -ENOENT) …

poettering · 2020-05-25T13:35:23Z

src/core/apparmor-setup.c

+                if (errno == ENOENT) {
+                        r = read_one_line_file("/proc/self/attr/current", &current_profile);
+                        if (r < 0) {
+                                log_warning_errno(errno, "Failed to get the current AppArmor profile of systemd from /proc/self/attr/current: %m");


poettering · 2020-05-25T13:35:43Z

src/core/apparmor-setup.c

+                        r = read_one_line_file("/proc/self/attr/current", &current_profile);
+                        if (r < 0) {
+                                log_warning_errno(errno, "Failed to get the current AppArmor profile of systemd from /proc/self/attr/current: %m");
+                                return 0;


why check these two paths btw? can you add a comment, please?

poettering · 2020-05-25T13:44:18Z

src/core/apparmor-setup.c

+        r = aa_kernel_interface_load_policy_from_fd(interface, fd);
+        if (r < 0) {
+                log_warning_errno(errno, "Failed to load AppArmor profile from file %s into the kernel: %m", profile_file);
+                aa_kernel_interface_unref(interface);


instead of unreffing this explicitly in each exit path, please use DEFINE_TRIVIAL_CLEANUP_FUNC() and then use _cleanup_(…) on the variable to automatically destroy the object when the function is left.

poettering · 2020-05-25T13:45:13Z

src/core/apparmor-setup.c

+                        log_error("Failed to change to AppArmor profile " APPARMOR_PROFILE_NAME ". Please ensure that the profile file %s contains a profile with that name.", profile_file);
+                } else {
+                        log_error_errno(errno, "Failed to change to AppArmor profile " APPARMOR_PROFILE_NAME ": %m");
+                }


as per out coding style, no {} around single-line if blocks:

https://systemd.io/CODING_STYLE/#formatting

src/core/apparmor-setup.c

poettering · 2020-05-25T13:47:08Z

src/core/apparmor-setup.c

+
+#define APPARMOR_PROFILE_NAME "systemd"
+#define APPARMOR_PROFILE_ETC_PATH "/etc/systemd/profile.apparmor"
+#define APPARMOR_PROFILE_USR_PATH "/usr/lib/systemd/profile.apparmor"


quite frankly I don't see the point of these defines. The latter two are used exactly once, and it would be more readable to just specify them literally where they are used. And the former is simple enough to not result in typos if repeated, hence I don't think it warrants a macro here..

I mean, macros are great to abbreviate stuff, to lower the risk of making typos, to make things configurable. But neither of those reasons apply here, hence please don't add the extra indirection that these macros are.

poettering · 2020-05-25T13:47:25Z

src/core/apparmor-setup.c

+
+int mac_apparmor_setup(void) {
+#if HAVE_APPARMOR
+        int r = -1;


why assign -1 here? it gets overwritten right away

poettering · 2020-05-25T13:49:47Z

Are you an apparmor upstream dev btw? If not I think it would make sense to get a review from someone involved in aa upstream, to get a blessing. Can you cc a suitable person from the aa team?

Or @xnox do you know the right person to CC?

The thing is simply that I don't know AA, and given I am a Fedora guy can't test it really, so I'd like a set of eyes on this that knows that stuff.

YmrDtnJu · 2020-05-25T15:15:39Z

I did not see the comments in the commit itself. I will change the commits again.

src/core/apparmor-setup.c

xnox · 2020-05-25T22:10:00Z

@jdstrand can you please review this?

xnox · 2020-05-25T22:16:27Z

I would feel better if securityfs was mounted, apparmor_parser invoke to compile profile for systemd, and then it loaded and switched to.

The paths /etc/systemd/profile.apparmor, /usr/lib/systemd/profile.apparmor seem akward to use, as they do not encode the kernel apparmor abi available, i.e. the 26b63962.0 cache directory name in /var/cache/apparmor on my system. It would feel more apparmor-natural to load /var/cache/apparmor/$apparmor-abi/usr.lib.systemd.systemd instead, if available. Or attempt to generate it.

YmrDtnJu · 2020-05-26T00:40:16Z

We can add /var/cache/apparmor/$apparmor-abi/usr.lib.systemd.systemd to the list of paths instead of /etc/... or before or after it. I will have to find out how to read the ABI version from the kernel.

Running apparmor_parser is something I do not want to do, because there is no standardized way to do it. Distributions with AppArmor support (as Ubuntu does) may have the same layout of files for profiles but others may not. We would force those users to have the same layout which may not fit there needs. Using a pre-compiled file allows everyone to create that file in the way as they wish. They can even distribute it in /usr with their systemd package (as I do).

I am not sure what you mean by securitfyfs being mounted or not. The profil is loaded using a file in securityfs. So it has to be mounted anyway in all cases.

poettering

looks great, one more minor point

src/core/apparmor-setup.c

poettering · 2020-05-26T07:14:45Z

Running apparmor_parser is something I do not want to do, because there is no standardized way to do it. Distributions with AppArmor support (as Ubuntu does) may have the same layout of files for profiles but others may not. We would force those users to have the same layout which may not fit there needs. Using a pre-compiled file allows everyone to create that file in the way as they wish. They can even distribute it in /usr with their systemd package (as I do).

Shelling out to external binaries (that's what this apparmor_parser thing is, no?) is something I really don't like. We generally want to be more than a bunch of superglued shell scripts, but if we keep invoking all kinds of external binaries in order we just become that...

YmrDtnJu · 2020-05-26T07:56:21Z

apparmor_parser is an application to read text profile, compile them into binary profiles and load those into the kernel. so yes, it is an external binary.

poettering · 2020-05-26T08:13:01Z

looks good by me now. Would be good to get at least a superficial blessing from some upstream AA folks, but if that never materializes we can merge this as it is, too. let's give them a few days though

YmrDtnJu · 2020-05-26T08:26:51Z

I will ask for a comment on their irc channel.

xnox · 2020-05-26T09:26:26Z

looks good by me now. Would be good to get at least a superficial blessing from some upstream AA folks, but if that never materializes we can merge this as it is, too. let's give them a few days though

I did request jdstrand to review this. (AA upstream).

xnox · 2020-05-26T09:43:03Z

Running apparmor_parser is something I do not want to do, because there is no standardized way to do it. Distributions with AppArmor support (as Ubuntu does) may have the same layout of files for profiles but others may not. We would force those users to have the same layout which may not fit there needs. Using a pre-compiled file allows everyone to create that file in the way as they wish. They can even distribute it in /usr with their systemd package (as I do).

Shelling out to external binaries (that's what this apparmor_parser thing is, no?) is something I really don't like. We generally want to be more than a bunch of superglued shell scripts, but if we keep invoking all kinds of external binaries in order we just become that...

I understand that calling external binaries in core/main.c is undesirable. It's just I don't see how the current naming scheme of a single precompiled profile.apparmor can work. As two kernels, can have different precompiled representation of the profile depending on the apparmor features available. Which will fail to load on a non-matching kernel.

Thus I'd still rather see loading precompiled binary profile from /var/cache/apparmor/${hash-of-apparmor-features}/usr.lib.systemd.systemd. At the very least profile.apparmor needs an extension profile.apparmor.${hash-of-apparmor-features}, or like be a different cache directory.

Without doing this, I don't see how one can store a precompiled profile for the next boot using the new kernel, whilst preserving capability of booting using fallback/old kernel, when the two have different sets of apparmor features. See https://gitlab.com/apparmor/apparmor/-/blob/master/libraries/libapparmor/src/features.c

A precompiled profile is per-kernel, and is not universal.

YmrDtnJu · 2020-05-26T10:00:02Z

Then I am out of options here. I will not propose a patch that calls apparmor_parser and compiles the profile.
libapparmor provides aa_features_id. It returns the hash we need. I can add support for that. It should be easy. We can first check /var/cache/apparmor/${hash-of-apparmor-features}/usr.lib.systemd.systemd and load that, if it exists. If it does not, look at /etc/... and then at /usr/lib/systemd/.... Each file with the hash appended.

poettering · 2020-05-26T13:39:30Z

Thus I'd still rather see loading precompiled binary profile from /var/cache/apparmor/${hash-of-apparmor-features}/usr.lib.systemd.systemd. At the very least profile.apparmor needs an extension profile.apparmor.${hash-of-apparmor-features}, or like be a different cache directory.

Makes sense. Wouldn't including uname -r suffice though?

poettering · 2020-05-26T13:43:07Z

i am not too fond of the idea to shell out to the aa profile compiler. I think that's something the RPM/DEB can do whenever kernel/aa policy is updated. But supporting a mode where the kernel realease (or that aa feature mask thing you found) is included in the profile path makes a lot of sense. I'd first look for the version with the feature mask in the name, and then fall back to the generic ones. After all there appears to be a strong usecase for just using a plain name, for example if you have kernel+initrd+rootfs all immutable and pre-built in a verifiable fashion (secureboot/verity/…) and thus kernel+init+rootfs are always updated together anyway and each kernel has a matching rootfs anyway...

YmrDtnJu · 2020-05-26T13:57:09Z

Makes sense. Wouldn't including uname -r suffice though?

I am not sure, but I don't think so. It is common for AppArmor to distinguish the feature set of a particular kernel with the feature hash. I think, systemd should stick to that method, if we want to distinguish between different feature sets at all. And after all, it is just one call of a function of a library we have to use anyway.

After all there appears to be a strong usecase for just using a plain name, for example if you have kernel+initrd+rootfs all immutable and pre-build in a verifiable fashion (secureboot/verity/…) and thus kernel+init+rootfs are always updated together anyway and each kernel has a matching rootfs anyway...

This is also my intention. I expect this feature to be used by people who build their own images. dm_verity, fs_verity or ima/evm can be used to sign the compiled profile file. This is, why I also search for the file in /usr so that it can be installed by the package manager or an image building tool. Also searching for the profile file in /var/cache/apparmor will enable Ubuntu and other general purpose distributions to start systemd with a profile applied too.

@xnox Is that ok for you? If so, I will update the patch accordingly.

jdstrand · 2020-05-26T20:12:33Z

While @xnox tagged me, I'm going to defer to @jrjohansen (AppArmor lead) on this since he'll have the most up to date information on libapparmor (eg, to avoid calling out to apparmor_parser) as well as upstream AppArmor's thoughts on full system policy, policy from initrd, etc.

jrjohansen · 2020-05-26T22:52:13Z

src/core/apparmor-setup.c

+        FOREACH_STRING(profile_file, "/etc/systemd/profile.apparmor", "/usr/lib/systemd/profile.apparmor") {
+                fd = open(profile_file, O_RDONLY | O_CLOEXEC);
+                if (fd < 0) {
+                        if (errno == ENOENT)
+                                continue;
+                        log_warning_errno(errno, "Failed to open binary AppArmor profile file %s, ignoring: %m", profile_file);
+                } else
+                        break;
+        }


I have concerns here both of these locations are outside of apparmor regular cache management, which means they will require manual update to keep from being stale. Also neither of these take into account that policy can be compiled differently for different kernel versions. This can lead to the following cases

Everything working because the policy features matches up well with the kernel.

The kernel rejecting policy because it uses an abi, or feature set it doesn't know how to handle.

The kernel accepting the policy but it doesn't enforce everything it could. The kernel is backwards compatible to older abis, but when using a single cache file and multiple kernels the policy cache has to be compiled to the lowest common denominator.

It would be far better if we can standardize this to work within regular apparmor policy compiles and loads, which would mean we could have the cache being updated when policy is updated and also different cache files for the different kernel versions.

jrjohansen · 2020-05-26T22:53:44Z

src/core/apparmor-setup.c

+        r = aa_kernel_interface_new(&interface, NULL, NULL);
+        if (r < 0) {
+                log_warning_errno(errno, "Failed to get a new AppArmor kernel interface: %m");
+                return 0;
+        }


This is going to fail if securityfs or apparmorfs are not premounted. Basically there is a hidden dependency here.

mount_setup_early ensures that api filesystems like securitfy are mounted. It is called before in main.c before mac_apparmor_setup. The comment in mount_setup_early explicitly mentions this as a requirement for SELinux so it should be ok for AppArmor too.

Yeah, we do have "basic" filesystems mounted already directly by systemd mount_setup_early, it handles things like dev, proc, sys, securityfs, for selinux, apparmor, smack, etc.

Actually, src/core/mount-setup.c only covers securityfs + smackfs. selinuxf is mounted by selinux code. apparmorfs is currently not covered, didn't know this was a thing. Maybe this PR should carry another commit mounting it too, if that's necessary?

Its new and hasn't been merged yet. Once it lands we can work on a patch for that.

jrjohansen · 2020-05-26T23:09:05Z

I would feel better if securityfs was mounted, apparmor_parser invoke to compile profile for systemd, and then it loaded and switched to.

securityfs or apparmorfs have to be pre mounted to be able to load the policy regardless, so there is a hidden dependency that is not being taken care of here.

As for the apparmor_parser it isn't needed as long as you are grabbing the correct policy cache file. But ...

The paths /etc/systemd/profile.apparmor, /usr/lib/systemd/profile.apparmor seem akward to use, as they do not encode the kernel apparmor abi available, i.e. the 26b63962.0 cache directory name in /var/cache/apparmor on my system. It would feel more apparmor-natural to load /var/cache/apparmor/$apparmor-abi/usr.lib.systemd.systemd instead, if available. Or attempt to generate it.

as you point out the current paths don't do that, they share the precompiled cache between kernel versions and don't get updated when policy is compiled.

jrjohansen · 2020-05-26T23:15:14Z

We can add /var/cache/apparmor/$apparmor-abi/usr.lib.systemd.systemd to the list of paths instead of /etc/... or before or after it. I will have to find out how to read the ABI version from the kernel.

you can use libapparmor

Running apparmor_parser is something I do not want to do, because there is no standardized way to do it. Distributions with AppArmor support (as Ubuntu does) may have the same layout of files for profiles but others may not. We would force those users to have the same layout which may not fit there needs. Using a pre-compiled file allows everyone to create that file in the way as they wish. They can even distribute it in /usr with their systemd package (as I do).

I completely agree that the apparmor_parser (policy compiler) should not be called from here. However the current scheme is problematic. Yes different distros may specify different locations, or even multiple locations.

But then should we standardize on way to specify the locations and everybody can work with that?

I am not sure what you mean by securitfyfs being mounted or not. The profil is loaded using a file in securityfs. So it has to be mounted anyway in all cases.

Ideally the code should be able to try to resolve this dependency and not have it be hidden.

jrjohansen · 2020-05-26T23:19:57Z

Then I am out of options here. I will not propose a patch that calls apparmor_parser and compiles the profile.
libapparmor provides aa_features_id. It returns the hash we need. I can add support for that. It should be easy. We can first check /var/cache/apparmor/${hash-of-apparmor-features}/usr.lib.systemd.systemd and load that, if it exists. If it does not, look at /etc/... and then at /usr/lib/systemd/.... Each file with the hash appended.

you don't need to call out to the parser, the code already exists in libapparmor, see man aa_policy_cache_replace_all

jrjohansen · 2020-05-26T23:22:14Z

Thus I'd still rather see loading precompiled binary profile from /var/cache/apparmor/${hash-of-apparmor-features}/usr.lib.systemd.systemd. At the very least profile.apparmor needs an extension profile.apparmor.${hash-of-apparmor-features}, or like be a different cache directory.

Makes sense. Wouldn't including uname -r suffice though?

unfortunately no, but there are ways to get at which cache via library routines.

jrjohansen · 2020-05-26T23:29:17Z

Makes sense. Wouldn't including uname -r suffice though?

I am not sure, but I don't think so. It is common for AppArmor to distinguish the feature set of a particular kernel with the feature hash. I think, systemd should stick to that method, if we want to distinguish between different feature sets at all. And after all, it is just one call of a function of a library we have to use anyway.

After all there appears to be a strong usecase for just using a plain name, for example if you have kernel+initrd+rootfs all immutable and pre-build in a verifiable fashion (secureboot/verity/…) and thus kernel+init+rootfs are always updated together anyway and each kernel has a matching rootfs anyway...

This is also my intention. I expect this feature to be used by people who build their own images. dm_verity, fs_verity or ima/evm can be used to sign the compiled profile file. This is, why I also search for the file in /usr so that it can be installed by the package manager or an image building tool. Also searching for the profile file in /var/cache/apparmor will enable Ubuntu and other general purpose distributions to start systemd with a profile applied too.

@xnox Is that ok for you? If so, I will update the patch accordingly.

yes this is fine, and something we have been working towards, you are just front running us a bit.

YmrDtnJu · 2020-05-27T10:46:46Z

@poettering The version of libapparmor for the CI checks do not seem to have support for the aa_features_id function. It has been introduced to libapparmor about two years ago in commit https://gitlab.com/apparmor/apparmor/-/commit/8d9c904174ceb29b17984e61b94fcd72aaa753bc and released with AppArmor 2.13.

I have an updated version that lets meson check for aa_features_id, but this means, that my code will not compiled with the CI checks. I am not sure if that is ok with you.

xnox · 2020-05-27T11:07:17Z

src/core/apparmor-setup.c

+        r = strv_extendf(&profile_files, "/etc/systemd/profile.apparmor.%s", feature_hash);
+        if (r < 0)
+                return log_oom();
+        r = strv_extendf(&profile_files, "/usr/lib/systemd/profile.apparmor.%s", feature_hash);
+        if (r < 0)
+                return log_oom();
+        r = strv_extend(&profile_files, "/etc/systemd/profile.apparmor");
+        if (r < 0)
+                return log_oom();
+        r = strv_extend(&profile_files, "/usr/lib/systemd/profile.apparmor");
+        if (r < 0)
+                return log_oom();


I would drop all of these, in favor of only loading from /var/cache/apparmor.

It is the correct and the default location, which all prebuilt / immutable images should have, as well as any "package" (deb, rpm, etc) based systems too.

Why do we still need loading from /etc/systemd, /usr/lib/systemd, if loading from /var/cache is there? Note this is binary profile, rather than the textual representation of the profile.

Otherwise the search paths look fine, and ordering is fine. I just don't think we need them at all.

I am ok with that. I will just maintain a small patch privately that adds the other directories back.

Sorry, but /var is a no-go for early boot stuff like this, see other comment, sorry.

(The other directories make a lot more sense to me than the /var path...)

poettering · 2020-06-01T10:29:39Z

Yeah, I'd move them all out of the way.

Cause /etc should be empty and /usr should be immutable or read-only.

I very much agree with that sentiment, but only to some point: I think /etc should be shipped empty, and populated minimally on first boot (or every boot, on entirely stateless systems), but not necessarily be empty forever. Because ultimately you need configuration/state somewhere, and /etc is for that just fine. Because if it would be entirely empty all the time and you just have /cache then you basically just renamed "etc" to "cache" and what good does that do?

/etc/dev/hwdb.bin i thoguht it is located in /usr/lib/udev/hwdb.bin. Am I confused, and is ubuntu broken?

It can be in either. The idea is that people who built immutable images that are updated as a whole put it int /usr/lib. — And distros who are updated with classic packages and where additional packages may drop in new hwdb files any time rebuild it on package update time (or whenever else the user drops in a hwdb file) and that file is then placed in /etc. Or to say this differently: data generated from data in /etc must also be in /etc and cannot be in /usr.

poettering · 2020-06-01T10:32:37Z

https://salsa.debian.org/systemd-team/systemd/-/blob/debian/master/debian/udev.postinst#L48

This is expected.

That way you generate stuff into /usr that is from data in /etc potentially. that's just weird. And people who drop in additional hwdb files into /etc and then rebuild the thing will be disconnected from your distros hwdb rebuilds.

As long as Debian does updates with "apt" it should stick the file in /etc, since it incorporates stuff from all kinds of dirs, including /etc. If Debian however decides to become an immutable-/usr-only distro, then it should do the "--usr" thing, and disallow people to drop in their own hwdb files in /etc.

xnox · 2020-06-01T10:55:49Z

Yeah, I'd move them all out of the way.
Cause /etc should be empty and /usr should be immutable or read-only.

I very much agree with that sentiment, but only to some point: I think /etc should be shipped empty, and populated minimally on first boot (or every boot, on entirely stateless systems), but not necessarily be empty forever. Because ultimately you need configuration/state somewhere, and /etc is for that just fine. Because if it would be entirely empty all the time and you just have /cache then you basically just renamed "etc" to "cache" and what good does that do?

I was more thinking /cache should contain the generated combined state of /usr and /etc.

If /etc doesn't have customizations yet, stuff in /cache simply contains things built from /usr contents. Or it can even be simply symlinks to prebuilt things in /usr.

However, if and when customisation appear in /etc, state gets rebuild, and then /cache contains the combined output of things that are from /usr and /etc. I.e. local certs & stock certs, combined together into CA-bundles of all the formats. Or local apparmor rules overrides & stock apparmor profiles, combined together into binary profiles, for both distro kernels and custom kernels.

/etc/dev/hwdb.bin i thoguht it is located in /usr/lib/udev/hwdb.bin. Am I confused, and is ubuntu broken?

It can be in either.

Ack, did not know that.

xnox · 2020-06-01T16:40:52Z

bionic-i386 & bionic-ppc64el builds somehow ran against old apparmor, will re-trigger that in a bit.

evverx · 2020-06-01T23:38:41Z

Right now systemd seems to be failing to compile when libapparmor is too old with:

cc -Isrc/core/2ac6ece@@core@sta -Isrc/core -I../src/core -Isrc/basic -I../src/basic -Isrc/boot -I../src/boot -Isrc/home -I../src/home -Isrc/shared -I../src/shared -Isrc/systemd -I../src/systemd -Isrc/journal -I../src/journal -Isrc/journal-remote -I../src/journal-remote -Isrc/nspawn -I../src/nspawn -Isrc/resolve -I../src/resolve -Isrc/timesync -I../src/timesync -I../src/time-wait-sync -Isrc/login -I../src/login -Isrc/udev -I../src/udev -Isrc/libudev -I../src/libudev -Isrc/shutdown -I../src/shutdown -I../src/xdg-autostart-generator -I../src/libsystemd/sd-bus -I../src/libsystemd/sd-device -I../src/libsystemd/sd-event -I../src/libsystemd/sd-hwdb -I../src/libsystemd/sd-id128 -I../src/libsystemd/sd-netlink -I../src/libsystemd/sd-network -I../src/libsystemd/sd-resolve -Isrc/libsystemd-network -I../src/libsystemd-network -I. -I../ -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/uuid -flto -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Werror -std=gnu99 -Wno-unused-parameter -Wno-missing-field-initializers -Wno-unused-result -Wno-format-signedness -Werror=undef -Wlogical-op -Wmissing-include-dirs -Wold-style-definition -Wpointer-arith -Winit-self -Wfloat-equal -Wsuggest-attribute=noreturn -Werror=missing-prototypes -Werror=implicit-function-declaration -Werror=missing-declarations -Werror=return-type -Werror=incompatible-pointer-types -Werror=format=2 -Wstrict-prototypes -Wredundant-decls -Wmissing-noreturn -Wimplicit-fallthrough=5 -Wshadow -Wendif-labels -Wstrict-aliasing=2 -Wwrite-strings -Werror=overflow -Werror=shift-count-overflow -Werror=shift-overflow=2 -Wdate-time -Wnested-externs -Wno-maybe-uninitialized -ffast-math -fno-common -fdiagnostics-show-option -fno-strict-aliasing -fvisibility=hidden -fstack-protector -fstack-protector-strong --param=ssp-buffer-size=4 -ffunction-sections -fdata-sections -Werror=shadow -include config.h -g -O3 -fdebug-prefix-map=/tmp/autopkgtest.xXYlG9/build.OT1/systemd=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -fPIC -pthread  -MD -MQ 'src/core/2ac6ece@@core@sta/apparmor-setup.c.o' -MF 'src/core/2ac6ece@@core@sta/apparmor-setup.c.o.d' -o 'src/core/2ac6ece@@core@sta/apparmor-setup.c.o' -c ../src/core/apparmor-setup.c
../src/core/apparmor-setup.c: In function ‘mac_apparmor_setup’:
../src/core/apparmor-setup.c:65:26: error: implicit declaration of function ‘aa_policy_cache_dir_path_preview’; did you mean ‘aa_policy_cache_replace_all’? [-Werror=implicit-function-declaration]
         cache_dir_path = aa_policy_cache_dir_path_preview(features, AT_FDCWD, "/etc/systemd/apparmor-early");
                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                          aa_policy_cache_replace_all
../src/core/apparmor-setup.c:65:26: error: nested extern declaration of ‘aa_policy_cache_dir_path_preview’ [-Werror=nested-externs]
../src/core/apparmor-setup.c:65:24: error: assignment makes pointer from integer without a cast [-Werror=int-conversion]
         cache_dir_path = aa_policy_cache_dir_path_preview(features, AT_FDCWD, "/etc/systemd/apparmor-early");
                        ^
cc1: all warnings being treated as errors
ninja: build stopped: subcommand failed.

I think it would make sense to bump up the dependency in meson.build.

"BuildDepends" in https://salsa.debian.org/systemd-team/systemd/-/blob/upstream-ci/debian/control#L27 should probably be updated as well.

YmrDtnJu · 2020-06-02T08:23:47Z

@evverx i am aware of that and have a new commit waiting here until it has been decided what path should be used. i can push the commit if you want, but it will still contain the same path.

xnox · 2020-06-02T09:37:50Z

"BuildDepends" in https://salsa.debian.org/systemd-team/systemd/-/blob/upstream-ci/debian/control#L27 should probably be updated as well.

No, we should not update debian/control just yet.

As I mentioned above the bionic amd64, arm64, s390x built against the updated apparmor, whilst for unknonw reason to me i386 ppc64el did not. Although the ppa used for testing has the up to date apparmor.

I with there was a simple "redeliver" button for the checks. As hunting through the webhook history is not nice.

xnox · 2020-06-02T09:42:31Z

Yeah, i386 & ppc64el tests are now pending again, awaiting results.

evverx · 2020-06-02T16:19:39Z

have a new commit waiting here until it has been decided what path should be used.

Agreed. I think it would be better to wait until it's settled.

No, we should not update debian/control just yet.

Since it's the "upstream-ci" branch I think it should be OK to update it once the PR is merged.

xnox · 2020-06-02T16:42:58Z

No, we should not update debian/control just yet.

Since it's the "upstream-ci" branch I think it should be OK to update it once the PR is merged.

but that will not magically install a newer version of apparmor =)

can you please explain what you hope to achieve by changing the packaging there? It does not need any changes.

xnox · 2020-06-02T16:43:42Z

bionic-i386 => most tests passed, and upstream test timedout. From my point of view that's acceptable to merge, there are no regressions identified.

evverx · 2020-06-02T17:03:24Z

can you please explain what you hope to achieve by changing the packaging there?

The idea is to make it clear that the package won't build without the latest version of libapparmor.

It does not need any changes.

I'm not sure I understand what is the point of having the "upstream-ci" branch then if it isn't supposed to be used to keep track of build dependencies?

jrjohansen · 2020-06-02T17:09:15Z

On 6/2/20 1:24 AM, YmrDtnJu wrote: @evverx <https://github.com/evverx> i am aware of that and have a new commit waiting here until it has been decided what path should be used. i can push the commit if you want, but it will still contain the same path.

I talked it over with @jdstrand and given the constraints and history /etc/apparmor/earlypolicy/ seem like the best place. I am fine with hard coding the location for now, and us following up after with a patch to make it configurable via apparmor's pkg-config.

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15905 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AH4SSHVVPGUONHRNBTPYLUTRUSZKFANCNFSM4NJLEUOA>.

ddstreet · 2020-06-02T18:53:57Z

can you please explain what you hope to achieve by changing the packaging there?

The idea is to make it clear that the package won't build without the latest version of libapparmor.

It does not need any changes.

I'm not sure I understand what is the point of having the "upstream-ci" branch then if it isn't supposed to be used to keep track of build dependencies?

yes, I can update the upstream-ci minimum apparmor version. I'd like to wait until this PR is merged, in case there are any more changes.

And currently for this PR, apparmor 2.13 is the right minimum version to use, correct? Based on earlier statement:

aa_features_id function. It has been introduced to libapparmor about two years ago in commit https://gitlab.com/apparmor/apparmor/-/commit/8d9c904174ceb29b17984e61b94fcd72aaa753bc and released with AppArmor 2.13.

src/core/apparmor-setup.c

poettering · 2020-06-02T20:55:07Z

I talked it over with @jdstrand and given the constraints and history /etc/apparmor/earlypolicy/ seem like the best place. I am fine with hard coding the location for now, and us following up after with a patch to make it configurable via apparmor's pkg-config.

Sounds perfect to me!

poettering

the NULL thing matters

src/core/apparmor-setup.c

xnox

See unresolved conversations in the Files Changed tab.

…up time Let systemd load a set of pre-compiled AppArmor profile files from a policy cache at /etc/apparmor/earlypolicy. Maintenance of that policy cache must be done outside of systemd. After successfully loading the profiles systemd will attempt to change to a profile named systemd. If systemd is already confined in a profile, it will not load any profile files and will not attempt to change it's profile. If anything goes wrong, systemd will only log failures. It will not fail to start.

YmrDtnJu · 2020-06-03T13:29:53Z

New commit with all changes done. Please check.

Currently, systemd will not load the profile if it is already confined in a profile. Should we change that and just skip the change to the systemd profile but stil load the policy cache?

keszybz · 2020-06-03T16:08:42Z

New commit with all changes done. Please check.

Currently, systemd will not load the profile if it is already confined in a profile. Should we change that and just skip the change to the systemd profile but stil load the policy cache?

Hmm, maybe. @jrjohansen?

poettering · 2020-06-09T18:27:40Z

been a week. I think we are good to merge this. if there's left to fix after all we can handle that as follow-up PR of course.

poettering requested changes May 25, 2020

View reviewed changes

poettering added apparmor reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels May 25, 2020

poettering requested changes May 25, 2020

View reviewed changes

poettering requested changes May 26, 2020

View reviewed changes

src/core/apparmor-setup.c Outdated Show resolved Hide resolved

src/core/apparmor-setup.c Outdated Show resolved Hide resolved

jrjohansen reviewed May 26, 2020

View reviewed changes

xnox reviewed May 27, 2020

View reviewed changes

xnox reviewed Jun 2, 2020

View reviewed changes

src/core/apparmor-setup.c Show resolved Hide resolved

poettering requested changes Jun 2, 2020

View reviewed changes

src/core/apparmor-setup.c Outdated Show resolved Hide resolved

src/core/apparmor-setup.c Outdated Show resolved Hide resolved

keszybz reviewed Jun 3, 2020

View reviewed changes

src/core/apparmor-setup.c Outdated Show resolved Hide resolved

src/core/apparmor-setup.c Show resolved Hide resolved

keszybz changed the title ~~AppArmor: Support for loading a pre-compiled profile and change to it.~~ AppArmor: Support for loading a pre-compiled profile Jun 3, 2020

xnox requested changes Jun 3, 2020

View reviewed changes

xnox approved these changes Jun 3, 2020

View reviewed changes

poettering merged commit 2ffadd3 into systemd:master Jun 9, 2020

poettering added the pid1 label Jun 9, 2020

AppArmor: Support for loading a pre-compiled profile #15905

AppArmor: Support for loading a pre-compiled profile #15905

Conversation

YmrDtnJu commented May 25, 2020

poettering left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poettering commented May 25, 2020

YmrDtnJu commented May 25, 2020

xnox commented May 25, 2020

xnox commented May 25, 2020

YmrDtnJu commented May 26, 2020 • edited

poettering left a comment

Choose a reason for hiding this comment

poettering commented May 26, 2020

YmrDtnJu commented May 26, 2020

poettering commented May 26, 2020

YmrDtnJu commented May 26, 2020

xnox commented May 26, 2020

xnox commented May 26, 2020

YmrDtnJu commented May 26, 2020 • edited

poettering commented May 26, 2020

poettering commented May 26, 2020 • edited

YmrDtnJu commented May 26, 2020

jdstrand commented May 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrjohansen commented May 26, 2020

jrjohansen commented May 26, 2020

jrjohansen commented May 26, 2020

jrjohansen commented May 26, 2020

jrjohansen commented May 26, 2020

YmrDtnJu commented May 27, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poettering commented Jun 1, 2020 • edited

poettering commented Jun 1, 2020

xnox commented Jun 1, 2020

xnox commented Jun 1, 2020

evverx commented Jun 1, 2020

YmrDtnJu commented Jun 2, 2020

xnox commented Jun 2, 2020

xnox commented Jun 2, 2020

evverx commented Jun 2, 2020

xnox commented Jun 2, 2020

xnox commented Jun 2, 2020

evverx commented Jun 2, 2020

jrjohansen commented Jun 2, 2020 via email

ddstreet commented Jun 2, 2020

poettering commented Jun 2, 2020

poettering left a comment

Choose a reason for hiding this comment

xnox left a comment

Choose a reason for hiding this comment

YmrDtnJu commented Jun 3, 2020

keszybz commented Jun 3, 2020

poettering commented Jun 9, 2020

YmrDtnJu commented May 26, 2020 •

edited

YmrDtnJu commented May 26, 2020 •

edited

poettering commented May 26, 2020 •

edited

YmrDtnJu commented May 27, 2020 •

edited

poettering commented Jun 1, 2020 •

edited