roddhjav / apparmor.d

Full set of AppArmor profiles (~ 1500 profiles)
https://apparmor.pujol.io
GNU General Public License v2.0
468 stars 47 forks source link

File conflicts with upstream AppArmor: abstractions/trash and tunables/xdg-user-dirs #131

Closed cboltz closed 1 year ago

cboltz commented 1 year ago

abstractions/trash and tunables/xdg-user-dirs also exist in the upstream default profile set, but with completely different rules.

Please find a way to avoid this file conflict, for example by moving these two files to a *.d/ directoy - or by upstreaming your additions ;-)

roddhjav commented 1 year ago

Upstreaming all the abstractions is is planned. At some point, we could even consider upstreaming most of the profiles. But for now, the project is still in early stage where I test (and break) things.

Currently, the packages displace the files to prevent conflict issues.

Regarding abstractions/trash, this is still a work in progress (see #94), and right now it is not ready at all for upstream. I ended up overwriting the trash abstraction, as some rules were conflicting.

Regarding the xdg-user-dirs, I overwrited the existing one for historic purposes that may not be valid anymore. Actually I would like to see your opinion regarding the new variables defined in tunables/xdg-user-dirs. Do you think some could fit upstream? I mean, some are really usefull, some are not. For example user_tmp_dirs ended up being useless (and probably not a good idea anyway).

cboltz commented 1 year ago

You might be interested in https://build.opensuse.org/package/show/home:cboltz/apparmor.d ;-)

I had to add some workarounds to avoid conflicts - see the spec file and the patch I added, which should answer your questions about the conflicting files. (Maybe you can apply some of my workarounds to your repo?)

In case you wonder: Moving tunables/extend to tunables/etc.d/ is sort of a workaround to load it at the right time, until we have an upstream solution to extend tunables/global.

So far, your profiles seem to work. However, I installed the apparmor.d package only some minutes ago. I'll tell you in some days if I see things that need additions to your profiles.

Regarding the new variables - if you have a good use case for them, there are good chances to get them upstream. (No promises ;-)

roddhjav commented 1 year ago

Nice, thanks a lot. I will move tunables/extend to tunables/etc.d/. And try removing all the conflicts.

Enjoy the profile ;). Please fell free to report any issue with them. They are still pretty much a work in progress.

roddhjav commented 1 year ago

Do you know if there is a specific order to load the file in tunables/xdg-user-dirs.d?. apparmor_parser, keeps loading the wrong one first. And I have to name the previous tunables/xdg-user-dirs as tunables/xdg-user-dirs.d/site.local to be loaded first.

cboltz commented 1 year ago

Easy part first: Include files get loaded in the order the include statements appear.

With directory includes (include <foo.d/>), the order in which the parser picks up the files in that directory can be "random" AFAIK (depending on filesystem internal order or something like that), so foo.d/xyz might be read/included before foo.d/abc.

That's why moving tunables/extend to tunables/etc.d/ helps - the include directive for tunables/etc (and tunables/etc.d) comes after include <tunables/xdg-user-dirs>

cboltz commented 1 year ago

Easy part first: Include files get loaded in the order the include statements appear.

With directory includes (include <foo.d/>), the order in which the parser picks up the files in that directory can be "random" AFAIK (depending on filesystem internal order or something like that), so foo.d/xyz might be read/included before foo.d/abc.

That's why moving tunables/extend to tunables/etc.d/ helps - the include directive for tunables/etc (and tunables/etc.d) comes after include <tunables/xdg-user-dirs>

cboltz commented 1 year ago

Easy part first: Include files get loaded in the order the include statements appear.

With directory includes (include <foo.d/>), the order in which the parser picks up the files in that directory can be "random" AFAIK (depending on filesystem internal order or something like that), so foo.d/xyz might be read/included before foo.d/abc.

That's why moving tunables/extend to tunables/etc.d/ helps - the include directive for tunables/etc (and tunables/etc.d) comes after include <tunables/xdg-user-dirs>

cboltz commented 1 year ago

Easy part first: Include files get loaded in the order the include statements appear.

With directory includes (include <foo.d/>), the order in which the parser picks up the files in that directory can be "random" AFAIK (depending on filesystem internal order or something like that), so foo.d/xyz might be read/included before foo.d/abc.

That's why moving tunables/extend to tunables/etc.d/ helps - the include directive for tunables/etc (and tunables/etc.d) comes after include <tunables/xdg-user-dirs>

cboltz commented 1 year ago

Easy part first: Include files get loaded in the order the include statements appear.

With directory includes (include <foo.d/>), the order in which the parser picks up the files in that directory can be "random" AFAIK (depending on filesystem internal order or something like that), so foo.d/xyz might be read/included before foo.d/abc.

That's why moving tunables/extend to tunables/etc.d/ helps - the include directive for tunables/etc (and tunables/etc.d) comes after include <tunables/xdg-user-dirs>

cboltz commented 1 year ago

Easy part first: Include files get loaded in the order the include statements appear.

With directory includes (include <foo.d/>), the order in which the parser picks up the files in that directory can be "random" AFAIK (depending on filesystem internal order or something like that), so foo.d/xyz might be read/included before foo.d/abc.

That's why moving tunables/extend to tunables/etc.d/ helps - the include directive for tunables/etc (and tunables/etc.d) comes after include <tunables/xdg-user-dirs>

cboltz commented 1 year ago

Easy part first: Include files get loaded in the order the include statements appear.

With directory includes (include <foo.d/>), the order in which the parser picks up the files in that directory can be "random" AFAIK (depending on filesystem internal order or something like that), so foo.d/xyz might be read/included before foo.d/abc.

That's why moving tunables/extend to tunables/etc.d/ helps - the include directive for tunables/etc (and tunables/etc.d) comes after include <tunables/xdg-user-dirs>

roddhjav commented 1 year ago

That explain a lot (this is also why I had the initial solution the simply override xdg-user-dirs).

My xdg-user-dirs lets says tunables/xdg-user-dirs.d/apparmor.d need to be loaded before any other file in tunables/xdg-user-dirs.d. So it cannot be in xdg-user-dirs.d but elsewhere in order to be loaded before like in home.d. However; some of these additions also use the classic xdg-user-dirs variables. Therefore; I have to split it in two part: the first one in home.d/apparmor.d and the second one in xdg-user-dirs.d/apparmor.d

It works. I still had to change the default value of variable like: @{XDG_WALLPAPERS_DIR}="@{XDG_PICTURES_DIR}/Wallpapers" to @{XDG_WALLPAPERS_DIR}="Pictures/Wallpapers" to ensure I avoid using variable not previously declared in possible local user addition.

ShellCode33 commented 1 year ago

The issue with that approach is that users are unable to use @{XDG_*} variables from /etc/apparmor.d/tunables/xdg-user-dirs.d/local. May I suggest using a subfolder instead ? The include statement of Apparmor is not recursive.

Upstream /etc/apparmor.d/tunables/xdg-user-dirs does:

include <tunables/xdg-user-dirs.d>

Your /etc/apparmor.d/tunables/xdg-user-dirs.d/apparmor.d could do something like:

include <tunables/xdg-user-dirs.d/apparmor.d.extend>

Tell users to create /etc/apparmor.d/tunables/xdg-user-dirs.d/apparmor.d.extend/local instead of /etc/apparmor.d/tunables/xdg-user-dirs.d/local.

roddhjav commented 1 year ago

I am not sure to understand the issue, @{XDG_*} variables are be loaded before anything in tunables/xdg-user-dirs.d/.

Asking user to move their local directory may breaks all install... and compatibility with profile from other sources.

cboltz commented 1 year ago

I understand the idea - basically variables defined by the apparmor.d project should be extendible in a separate sub-subdirectory. That is in theory a good idea.

The main issue I see is that it will break user include files when some of the variable definitions introduced in apparmor.d go upstream AppArmor, which most likely also means the (for upstream AppArmor "unusual") sub-subdirectories will no longer be included. And exactly that is why the idea of using these directories is not really a good idea in practise.

ShellCode33 commented 1 year ago

Thanks for your insights @cboltz ! Though I'm not sure I understand why it would break user configs once apparmor.d definitions go upstream. Which variables are you referring to ? The @{XDG_*_DIR} ones or the @{user_*_dirs} ones ? Or both ? The way I understand it, in any case if some of those variables become upstream, apparmor.d will have to remove them for user configs to keep working. (which can be very complex to manage because some distributions like ArchLinux will get upstream changes very fast while others like Debian will wait for years before they catch up with upstream AppArmor)

As long as this apparmor.d project keeps an include if exists <tunables/xdg-user-dirs.d/apparmor.d.d> it should be fine for users ? It's only a matter of apparmor.d keeping up with upstream and making sure new variables introduced upstream are removed. But maybe there's something I misunderstood, I'm not an AppArmor guru :-)

cboltz commented 1 year ago

Your understanding is correct. However, the problem is that upstream will never ship something like include if exists <tunables/xdg-user-dirs.d/apparmor.d.d> which also means the apparmor.d project would need to keep it for compability forever. While that's possible, it's also quite annoying. The better way is to not even introduce something that might need compability workarounds forever ;-)

Upstreaming new variables, profiles and/or abstractions indeed makes things interesting because of different versions depending on the distribution. I don't have a perfect solution for that ;-) but I'm sure it's a solvable problem (for example check the AppArmor version in apparmor.d make install, and skip installing files that were upstreamed to that version).

ShellCode33 commented 1 year ago

I guess the best way would be for distributions to ship the AppArmor runtime and profiles separately, so that users can decide to install alternative apparmor.d like this project instead of the one provided by upstream. There would be no conflict to handle anymore. But it would be a lot of work to convince all distributions to do so, and I'm not even sure it's a good idea