feat(akonadiserver): Add AppArmor profile for akonadiserver
ClosedPublic

Authored by knauss on Sep 12 2019, 2:10 PM.

Details

Summary

Debian and Neon already use Apparmor to secure AkonadiServer, we should
join forces and move the AppArmor profile upstream so other Linux
distributions can take advantages of it.

Closes: #411792

Test Plan

run akonadi with AppArmor enabled.

Diff Detail

Repository
R165 Akonadi
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
knauss created this revision.Sep 12 2019, 2:10 PM
Restricted Application added a project: KDE PIM. · View Herald TranscriptSep 12 2019, 2:10 PM
Restricted Application added a subscriber: kde-pim. · View Herald Transcript
knauss requested review of this revision.Sep 12 2019, 2:10 PM
dvratil added inline comments.Sep 12 2019, 2:43 PM
CMakeLists.txt
356

Should this be installed on on-apparmor distros? Could it be hidden behind a cmake option?

knauss updated this revision to Diff 65927.Sep 12 2019, 2:52 PM

cleanup and split AppArmor profile.

Intrigeri pointed out some guidelines for Apparmor profiles.
(thanks intrigeri)

knauss retitled this revision from feat(akonadiserver): Add AppArmor profile for akonadiserver to [WIP] feat(akonadiserver): Add AppArmor profile for akonadiserver.Sep 12 2019, 2:53 PM
knauss edited the summary of this revision. (Show Details)
knauss added inline comments.
apparmor/usr.bin.akonadiserver
5

REMOVE flags=(complain), when Postgres and SQLite profiles are added!

knauss added inline comments.Sep 12 2019, 2:55 PM
CMakeLists.txt
356

Yes I think we need a cmake option. Also this is only to run with root permissions.
Do you know how I check for root permissions?

knauss updated this revision to Diff 65928.Sep 12 2019, 2:58 PM

fixing typos.

dvratil added inline comments.Sep 13 2019, 8:32 AM
CMakeLists.txt
356

Well as a developer you do not run make install as root, but then you don't need to set the cmake option either, so no problem.

If you are a packager, you are using make DESTDIR=.... install to install into a prefix that will be packaged, so that will not need root privileges either.

cgiboudeaux added inline comments.
CMakeLists.txt
356

${KDE_INSTALL_SYSCONFDIR}/apparmor.d

knauss updated this revision to Diff 66613.Sep 22 2019, 11:58 AM

Add Apparmor profile for SQLite and PostgreSQL.
Make it installable as normal user

knauss retitled this revision from [WIP] feat(akonadiserver): Add AppArmor profile for akonadiserver to feat(akonadiserver): Add AppArmor profile for akonadiserver.Sep 22 2019, 12:00 PM
knauss marked 5 inline comments as done.

I've tested now mysql, postgresql and sqlite backend made sure everything starts/shutdown.

Did you test on both Neon and plain Debian?

CMakeLists.txt
356

The CMake option is still missing. I think I'd prefer a conditional add_subdirectory() instead of the install() here.

dvratil requested changes to this revision.Sep 23 2019, 8:43 AM
This revision now requires changes to proceed.Sep 23 2019, 8:43 AM
knauss updated this revision to Diff 66697.Sep 23 2019, 7:10 PM
  • Add commindline option to disable AppArmor.
  • Move Apparmor stuff into apparmor/CMakeLists.txt.
knauss marked an inline comment as done.Sep 23 2019, 7:16 PM

Did you test on both Neon and plain Debian?

tested only on plain Debian with a new installation. I added @jriddell to this Differential to test on Neon, but if @jriddell do not answer we will get the info if the AppArmor profiles are fine some days later, as Neon build the master master branch. Also feedback from other distributions will come in later.

dvratil requested changes to this revision.Oct 1 2019, 11:41 AM
dvratil added inline comments.
CMakeLists.txt
140

I would prefer this to be opt-in, rather than opt-out (and generally, options should be named positively (INSTALL_APPARMOR)

This revision now requires changes to proceed.Oct 1 2019, 11:41 AM
knauss updated this revision to Diff 67398.Oct 6 2019, 11:15 PM
  • Improve Apparmor profiles
  • Make config possitive.
knauss updated this revision to Diff 67399.Oct 6 2019, 11:20 PM
knauss marked an inline comment as done.
  • replace /home/* with \${HOME}.
dvratil requested changes to this revision.Oct 7 2019, 10:58 AM
dvratil added inline comments.
CMakeLists.txt
140

The default needs to be toggled to FALSE now to make it opt-in for packagers... :-)

This revision now requires changes to proceed.Oct 7 2019, 10:58 AM
knauss added inline comments.Oct 7 2019, 3:05 PM
CMakeLists.txt
140

The reason to opt-out instead of opt-in is, that it enhance security, so everyone who is using AppArmor should use it and having these files lying around don't harm anyone.
I call this "Security by Default".

But yes we can call it INSTALL_APPARMOR and make it as TRUE by default, that would do the same

140

As I already said, I think it should be opt-out, because it is a security feature, so every one who supports should enable it! And for those that don't support AppArmor, those file don't hurt, they are just annoying. With the default TRUE, we treat packagers to install Akonadi as secure as possible.

cgiboudeaux added inline comments.Oct 7 2019, 3:13 PM
apparmor/mysqld_akonadi
31

This supposes XDG_DATA_HOME is not set or set to $HOME/.local/share

apparmor/postgresql_akonadi
20–21

Same here

apparmor/usr.bin.akonadiserver
30

same thing with XDG_CONFIG_HOME

knauss edited the summary of this revision. (Show Details)Oct 7 2019, 4:20 PM
knauss added inline comments.Oct 7 2019, 4:22 PM
apparmor/mysqld_akonadi
31

AppArmor has currently no support for XDG_DATA_HOME, so we can only use the default location for linux installation. This is properly a feature request on AppArmor.

dvratil accepted this revision.Oct 8 2019, 8:56 AM

Ok, let's roll with it, then.

We have enough time to fixup stuff before 19.12, if needed.

apparmor/mysqld_akonadi
31

The discussion about XDG dirs support in apparmor happened first 6 years ago...doesn't shine the best light on apparmor, then... :-(

This revision is now accepted and ready to land.Oct 8 2019, 8:56 AM
This revision was automatically updated to reflect the committed changes.

Hello.
This should fix the bug 411093, am I right?

Hello.
This should fix the bug 411093, am I right?

well this patch is published with 19.08.0. The bug in 411093 seems more like a downstream bug.