Re: [systemd-devel] [PATCH] Add AppArmor profile switching
Le vendredi 21 février 2014 à 03:48 +0100, Lennart Poettering a écrit : On Thu, 20.02.14 16:19, m...@zarb.org (m...@zarb.org) wrote: From: Michael Scherer m...@zarb.org This permit to switch to a specific apparmor profile when starting a daemon. This will result in a non operation if apparmor is disabled. It also add a new build requirement on libapparmor for using this feature. Applied! I made some changes though, there were some missing bits to make sure the config hookup works correctly. I don't have any apparmor available though. Could you check if everything works correctly? I will, I do have a opensuse VM for that, and I think intrigeri in CC, likely does too. I figure the only missing bit to get apparmor up to the same level of support in systemd as SELinux, SMACK and IMA have would be policy uploading during early boot. Yeah, but this requires call to a external binary, I was wondering is using some unit wouldn't be enough. Upstart also do provides a way to load a policy specificied in a job, which is maye something we could support, like on demand module loading for selinux . What do people think about it ? ( for on demand loading of profile/module ) -- Michael Scherer ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] Add AppArmor profile switching
Hi, Michael Scherer wrote (21 Feb 2014 08:39:12 GMT) : Le vendredi 21 février 2014 à 03:48 +0100, Lennart Poettering a écrit : I don't have any apparmor available though. Could you check if everything works correctly? I will, I do have a opensuse VM for that, and I think intrigeri in CC, likely does too. I'll happily test this (in our upcoming Tor unit file) as soon as we have a version of systemd in Debian, that this patchset cleanly applies to. Cheers, -- intrigeri | GnuPG key @ https://gaffer.ptitcanardnoir.org/intrigeri/intrigeri.asc | OTR fingerprint @ https://gaffer.ptitcanardnoir.org/intrigeri/otr.asc ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] Add AppArmor profile switching
On Fri, 21.02.14 09:39, Michael Scherer (m...@zarb.org) wrote: Applied! I made some changes though, there were some missing bits to make sure the config hookup works correctly. I don't have any apparmor available though. Could you check if everything works correctly? I will, I do have a opensuse VM for that, and I think intrigeri in CC, likely does too. I figure the only missing bit to get apparmor up to the same level of support in systemd as SELinux, SMACK and IMA have would be policy uploading during early boot. Yeah, but this requires call to a external binary, I was wondering is using some unit wouldn't be enough. Upstart also do provides a way to Well, MAC policies sound like something one really should upload at a time where no process but PID 1 is around, so that it is guaranteed to apply to every process on the system. Uploading it in a normal unit loads it releatively late and in parallel to other servics. I am happy to add code that uploads the AppArmor policy the same way we upload SELinux, IMA, SMACK, but either this uploading must be so simple that we can easily implement this in our own code (which is the way we went for IMA or SMACK), or they must provide us with some library, but doing this via invoking a binary is something that I don't want to see in systemd upstream. load a policy specificied in a job, which is maye something we could What is different from what we have now with AppArmorProfile=? support, like on demand module loading for selinux . Hmm, on-demand module loading for selinux? What do you mean by that? Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] Add AppArmor profile switching
On Thu, 20.02.14 16:19, m...@zarb.org (m...@zarb.org) wrote: From: Michael Scherer m...@zarb.org This permit to switch to a specific apparmor profile when starting a daemon. This will result in a non operation if apparmor is disabled. It also add a new build requirement on libapparmor for using this feature. Applied! I made some changes though, there were some missing bits to make sure the config hookup works correctly. I don't have any apparmor available though. Could you check if everything works correctly? I figure the only missing bit to get apparmor up to the same level of support in systemd as SELinux, SMACK and IMA have would be policy uploading during early boot. Thanks! Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] Add AppArmor profile switching
On Fri, 14.02.14 12:21, Michael Scherer (m...@zarb.org) wrote:
This permit to switch to a specific apparmor profile when starting a daemon.
This
will result in a non operation if apparmor is disabled.
It also add a new build requirement on libapparmor for using this feature.
---
Makefile.am | 7 +++
configure.ac | 13 +
man/systemd.exec.xml | 13 +
src/core/build.h | 8 +++-
src/core/dbus-execute.c | 1 +
src/core/execute.c| 30 ++
src/core/execute.h| 2 ++
src/core/load-fragment-gperf.gperf.m4 | 3 ++-
src/shared/exit-status.c | 3 +++
src/shared/exit-status.h | 3 ++-
10 files changed, 80 insertions(+), 3 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index 79c49e6..79d355c 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -776,6 +776,13 @@ libsystemd_shared_la_SOURCES += \
src/shared/seccomp-util.c
endif
+libsystemd_shared_la_CFLAGS = \
+ $(AM_CFLAGS) \
+ $(APPARMOR_CFLAGS)
+
+libsystemd_shared_la_LIBADD = \
+ $(APPARMOR_LIBS)
+
Why is this in libsystemd-shared? This really should be added to the
core la, not shared... Or am I missing something?
SD_BUS_PROPERTY(SELinuxContext, s, NULL, offsetof(ExecContext,
selinux_context), SD_BUS_VTABLE_PROPERTY_CONST),
+SD_BUS_PROPERTY(AppArmorProfile, s, NULL,
offsetof(ExecContext, apparmor_profile),
SD_BUS_VTABLE_PROPERTY_CONST),
Hmm, so thinking about this, we should normalize both these options and
turn the s signature into (bs), i.e. a structure made of a bool and
the label, where the bool inidcates whether a non-existing label shall
be ignored or not. We have the same split up when serializing exec
commands, and we should do that here too...
+if (context-apparmor_profile use_apparmor()) {
+char* c = context-apparmor_profile;
+bool ignore = false;
+if (c[0] == '-') {
+c++;
+ignore = true;
Indentation 8 chars please...
+}
+
+err =
aa_change_onexec(context-apparmor_profile);
+if (err 0 !ignore) {
+r = EXIT_APPARMOR;
+goto fail_child;
+}
+}
+#endif
}
@@ -140,6 +140,8 @@ struct ExecContext {
char *selinux_context;
+char *apparmor_profile;
+
Similar as above, I'd like this to be stored normalized, i.e.:
bool selinux_context_ignore;
char *selinux_context;
bool apparmor_profile_ignore;
char *apparmor_profile;
Or similar...
Lennart
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] Add AppArmor profile switching
Le vendredi 14 février 2014 à 12:31 +0100, Lennart Poettering a écrit :
On Fri, 14.02.14 12:21, Michael Scherer (m...@zarb.org) wrote:
This permit to switch to a specific apparmor profile when starting a
daemon. This
will result in a non operation if apparmor is disabled.
It also add a new build requirement on libapparmor for using this feature.
---
Makefile.am | 7 +++
configure.ac | 13 +
man/systemd.exec.xml | 13 +
src/core/build.h | 8 +++-
src/core/dbus-execute.c | 1 +
src/core/execute.c| 30 ++
src/core/execute.h| 2 ++
src/core/load-fragment-gperf.gperf.m4 | 3 ++-
src/shared/exit-status.c | 3 +++
src/shared/exit-status.h | 3 ++-
10 files changed, 80 insertions(+), 3 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index 79c49e6..79d355c 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -776,6 +776,13 @@ libsystemd_shared_la_SOURCES += \
src/shared/seccomp-util.c
endif
+libsystemd_shared_la_CFLAGS = \
+ $(AM_CFLAGS) \
+ $(APPARMOR_CFLAGS)
+
+libsystemd_shared_la_LIBADD = \
+ $(APPARMOR_LIBS)
+
Why is this in libsystemd-shared? This really should be added to the
core la, not shared... Or am I missing something?
It did also look weird this morning when I reviewed the patch, but I
trusted more my past-self than my current-self, I will take a look and
send another patch if needed.
SD_BUS_PROPERTY(SELinuxContext, s, NULL, offsetof(ExecContext,
selinux_context), SD_BUS_VTABLE_PROPERTY_CONST),
+SD_BUS_PROPERTY(AppArmorProfile, s, NULL,
offsetof(ExecContext, apparmor_profile),
SD_BUS_VTABLE_PROPERTY_CONST),
Hmm, so thinking about this, we should normalize both these options and
turn the s signature into (bs), i.e. a structure made of a bool and
the label, where the bool inidcates whether a non-existing label shall
be ignored or not. We have the same split up when serializing exec
commands, and we should do that here too...
So, you want a 2nd property SELinuxcontextIgnore/AppArmorProfileIgnore
that would be True when SELinuxContext/AppArmorProfile is prefixed by
'-', or also when SELinux/AppArmor is disabled ?
Also, SELinuxContext would be the context without the leading '-',
correct ?
+if (context-apparmor_profile use_apparmor()) {
+char* c = context-apparmor_profile;
+bool ignore = false;
+if (c[0] == '-') {
+c++;
+ignore = true;
Indentation 8 chars please...
Will do
+}
+
+err =
aa_change_onexec(context-apparmor_profile);
+if (err 0 !ignore) {
+r = EXIT_APPARMOR;
+goto fail_child;
+}
+}
+#endif
}
@@ -140,6 +140,8 @@ struct ExecContext {
char *selinux_context;
+char *apparmor_profile;
+
Similar as above, I'd like this to be stored normalized, i.e.:
bool selinux_context_ignore;
char *selinux_context;
bool apparmor_profile_ignore;
char *apparmor_profile;
Or similar...
So that would requires a custom change to load-fragment.c, so I guess
that's a separate task as we need to apply that to selinuxcontext.
So I will provides the change for SELinuxContext, then once accepted,
send a v3 of the apparmor patch with the code following the style of
SELinuxContext, is this ok ?
--
Michael Scherer
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] Add AppArmor profile switching
Le vendredi 14 février 2014 à 14:05 +0100, Michael Scherer a écrit : Le vendredi 14 février 2014 à 12:31 +0100, Lennart Poettering a écrit : On Fri, 14.02.14 12:21, Michael Scherer (m...@zarb.org) wrote: SD_BUS_PROPERTY(SELinuxContext, s, NULL, offsetof(ExecContext, selinux_context), SD_BUS_VTABLE_PROPERTY_CONST), +SD_BUS_PROPERTY(AppArmorProfile, s, NULL, offsetof(ExecContext, apparmor_profile), SD_BUS_VTABLE_PROPERTY_CONST), Hmm, so thinking about this, we should normalize both these options and turn the s signature into (bs), i.e. a structure made of a bool and the label, where the bool inidcates whether a non-existing label shall be ignored or not. We have the same split up when serializing exec commands, and we should do that here too... So, you want a 2nd property SELinuxcontextIgnore/AppArmorProfileIgnore that would be True when SELinuxContext/AppArmorProfile is prefixed by '-', or also when SELinux/AppArmor is disabled ? Mhh no, you want 1 single property, but with a struct rather than 1 string, forget about that, I misread. -- Michael Scherer ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] Add AppArmor profile switching
On Fri, 14.02.14 14:05, Michael Scherer (m...@zarb.org) wrote:
SD_BUS_PROPERTY(SELinuxContext, s, NULL,
offsetof(ExecContext, selinux_context), SD_BUS_VTABLE_PROPERTY_CONST),
+SD_BUS_PROPERTY(AppArmorProfile, s, NULL,
offsetof(ExecContext, apparmor_profile),
SD_BUS_VTABLE_PROPERTY_CONST),
Hmm, so thinking about this, we should normalize both these options and
turn the s signature into (bs), i.e. a structure made of a bool and
the label, where the bool inidcates whether a non-existing label shall
be ignored or not. We have the same split up when serializing exec
commands, and we should do that here too...
So, you want a 2nd property SELinuxcontextIgnore/AppArmorProfileIgnore
that would be True when SELinuxContext/AppArmorProfile is prefixed by
'-', or also when SELinux/AppArmor is disabled ?
Nope. Just one property as struct.
Also, SELinuxContext would be the context without the leading '-',
correct ?
Correct. The - would basically just be a way to denote this in the
unit file, but never stored like that internally nor exported over the bus.
@@ -140,6 +140,8 @@ struct ExecContext {
char *selinux_context;
+char *apparmor_profile;
+
Similar as above, I'd like this to be stored normalized, i.e.:
bool selinux_context_ignore;
char *selinux_context;
bool apparmor_profile_ignore;
char *apparmor_profile;
Or similar...
So that would requires a custom change to load-fragment.c, so I guess
that's a separate task as we need to apply that to selinuxcontext.
Yupp, there would be two parse functions that can handle
this. (internally, they can probably just invoke a single function that
takes pointers to both the char* and the bool variable)
So I will provides the change for SELinuxContext, then once accepted,
send a v3 of the apparmor patch with the code following the style of
SELinuxContext, is this ok ?
Feel free to just send them in a series.
Lennart
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
