Re: [systemd-devel] [PATCH] Move apparmor code before the namespace setup

2014-10-29 Thread Michael Scherer 
On Mon, Oct 27, 2014 at 11:20:53PM +0100, Lennart Poettering wrote:
 On Mon, 27.10.14 20:16, Michael Scherer (m...@zarb.org) wrote:
 
  On Mon, Oct 27, 2014 at 03:38:37PM +0100, Lennart Poettering wrote:
   On Sat, 11.10.14 21:57, m...@zarb.org (m...@zarb.org) wrote:
   
From: Michael Scherer m...@zarb.org

Since apparmor need to access /proc to communicate with the kernel,
any unit setting / as readonly will be unable to also use the
AppArmorProfile setting, as found on debian bug 760526.
   
   A unit that sets /proc to read-only is broken anyway, I don't think we
   should work around that. or am I missing something here?
  
  When a unit set / as readonly, /proc seems to become readonly too.
 
 Yes, it ReadOnlyDirectories= is recursive. People doing that should
 use ReadWriteDirectories=/proc to open up /proc again.
 
 Note that ReadOnlyDirectories= and ReadWriteDirectories= are low-level
 functionality. If you use it you really should know what you do. This
 is different from ProtectSystem= which is a lot more high-level and
 doesn't require you to think about all the details.

Of course, but that do not seems a reason to be forced to have a workaround in 
every 
unit doing that. 

  And I would count setting /proc as readonly ( or unreadable ) as a 
  hardening 
  measure to reduce the attack surface. 
 
 Well, people can do whatever they want, but write access to /proc is
 part of the Linux API, there's ton of functionality that processes
 need access to that is only available via writes to /proc. You cannot
 really take this away, except for trivial programs. systemd is really
 not the place to push for read-only /proc/self/... 
 
 The APIs in /proc are generally useful APIs, you cannot just declare
 them unnecessary, take them away and assume things to still work.

They are useful, but in the context of the original bug report on Debian, the 
goal is
to secure tor and reduce potential information leaks on a explictely hardened 
distribution ( tails ) whose aim is to increase privacy.

So that would be a explicit decision of the downstream to restrict it using 
systemd. 
If that's not done with systemd, that would be with selinux/apparmor anyway, 
but it 
is better to have a defense in depth, in case of a apparmor policy oversight or 
anything
similar.

So in order to make it maintainable and secure, the easiest way is to start by 
restricting 
everything, and then whitelisting, like we do for firewalling and selinux 
policy. 

No one want to assume things will just work, but on the other hand, if we can 
make it just work
at the systemd level, that's IMHO better.

So I do not really understand your concern. If the concern is that fixing the 
bug do not change
anything because this is broken anyway, this is something that will be fixed 
with finer grained
whitelisting and/or fixed in the daemon if possible. While not all daemons will 
work, far from it, 
I am quite sure some will without any trouble.

On the patch itself, I do not really see a problem :
- it doesn't change anything besides the location of the code coming from a 
patch 
I submitted 9 months ago. It would surely have been accepted if I did it right 
away. 
So I do not see any increased maintainance nor migration headaches. 
- it solve a corner case, which is not documented, nor really expected, 
and hard to debug to a less expert developper. 

So if the problem is that the reason of the patch to be merged aren't sound, I 
see:
- there is a demand for it ( cf bug )
- if the patch is not merged, that mean that we will :
  - have to had 1 work around in the unit ( as said in the initial bug already )
  - still restrict it dpwnstream with apparmor
  - have apparmor policy to do the restriction anyway.

I think we both prefer to favor having the right fix at the right place rather 
than a work
around everywhere, and I think that patch is that.

-- 
Michael Scherer
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] Move apparmor code before the namespace setup

2014-10-27 Thread Michael Scherer 
On Mon, Oct 27, 2014 at 03:38:37PM +0100, Lennart Poettering wrote:
 On Sat, 11.10.14 21:57, m...@zarb.org (m...@zarb.org) wrote:
 
  From: Michael Scherer m...@zarb.org
  
  Since apparmor need to access /proc to communicate with the kernel,
  any unit setting / as readonly will be unable to also use the
  AppArmorProfile setting, as found on debian bug 760526.
 
 A unit that sets /proc to read-only is broken anyway, I don't think we
 should work around that. or am I missing something here?

When a unit set / as readonly, /proc seems to become readonly too.

And I would count setting /proc as readonly ( or unreadable ) as a hardening 
measure to reduce the attack surface. 
For example :

CVE-2012-0056 local root exploit, requires to open /proc/$PARENT/mem to work 
 http://git.zx2c4.com/CVE-2012-0056/tree/mempodipper.c

CVE-2011-2495 /proc/ information leak
 https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2011-2495

CVE-2011-1593 local root exploit, requires to read /proc to be exploited
 
http://xorl.wordpress.com/2011/04/25/cve-2011-1593-linux-kernel-proc-next_pidmap-invalid-memory-access/
 )

CVE-2011-1020 race condition on /proc permitting DOS
 https://access.redhat.com/security/cve/CVE-2011-1020

I also wonder how it would be broken, since /proc access is very restricted 
inside openshift due to selinux and most if not all softwares work fine here.
 
 If you apply the apparmor profile before setting up the namespace
 stuff you need to whitelist all the namespace operations in the
 apparmor profile. That might be quite a lot, hence: is this really
 desirable?

Apparmor profile is applied on next exec with aa_change_onexec, so we are
not restricted until we switch to the daemon, no need to whitelist anything.
( unless we start to use system/fork/exec in the exec_child function but I think
we want to avoid that ).

-- 
Michael Scherer
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] Revisiting the ExecRestart issue

2014-03-29 Thread Michael Scherer 
Le vendredi 28 mars 2014 à 12:12 -0500, Brandon Black a écrit :
 
 Hi all,
I've brought this up before, but I became busy/discouraged and
 dropped the ball.  As systemd becomes increasingly widely deployed, I
 can no longer afford to do so, so I'd like to explore this area a bit
 further on the list again and see if we can't come up with a workable
 solution, or if perhaps I've missed some systemd/cgroups change in the
 past year or so that already allows a workaround.

 [.. snip .. ]

 4) Socket Activation! I know this is what some will scream when they
 skim the above, but it's not a realistic solution in this case for a
 few reasons:
 a) The startup delay, in some cases, can be many whole wallclock
 seconds.  This is necessary and acceptable in the general sense (this
 is network service that people use with large server-side
 installations, not a desktop thing).

It only occurs on the first start, no ?


 b) The primary socket traffic we care about is UDP, and further we
 *really* care about request-response latency for this traffic.  Even
 if you could set a large enough receive buffer to handle several
 seconds of heavy UDP requests (and you can't, for at least some
 installations), the multi-second-delay in the responses isn't
 reasonable.

Again, that's a multiple second delay only for the first start, after,
this will be the regular way since the socket is directly used by the
daemon.


 c) Another side-point that might be better addressed in another
 thread: even if both of the above weren't true, this daemon uses
 several sockets for multiple roles internally, some of which share
 all low-level details (e.g. two distinct use-cases for multiple TCP
 sockets that serve different high-level protocols, where the user
 might choose arbitrary ports for both).  I'm not seeing any trivial
 way to distinguish these via socket activation - perhaps some kind of
 socket label that could be accessed by the daemon via sd_* APIs to
 distinguish would be useful here?

You can use getsockopt to get some information, and match the port/type
to the appropriate structure.
See https://trac.torproject.org/projects/tor/ticket/8908 for a patch
doing that kind of thing for tor. 


-- 
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

2014-02-21 Thread Michael Scherer 
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] Starting CUPS very late on a desktop and non-server system

2014-02-20 Thread Michael Scherer 
Le jeudi 20 février 2014 à 23:18 +0100, Paul Menzel a écrit :
 Dear systemd folks,
 
 
 after Debian’s CTTE chose systemd as the default init system for the
 next Debian release, I installed it on one of the systems.
 
 Looking at the output `systemd-analyze plot`, I noticed that CUPS takes
 700 ms to start and as this is a desktop system where not a lot is
 printed and when, then only after the user has logged in, I wonder how
 that can be dealt with systemd. Like starting it only after user login?
 Or is that something which is not nicely doable because CUPS runs as a
 system daemon?

You can start it on demand, using the activation socket system.
See http://0pointer.de/blog/projects/socket-activation2.html
( since that date back to 2011, there is likely everything already
patched upstream in a stable release )
-- 
Michael Scherer

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 2/2] Add AppArmor profile switching

2014-02-14 Thread Michael Scherer 
Le vendredi 14 février 2014 à 01:24 +0100, Lennart Poettering a écrit :
 On Fri, 03.01.14 17:22, m...@zarb.org (m...@zarb.org) wrote:
 
 Heya!
 
 This patch appears to be unmerged still. Unfortunately it doesn't apply
 anymore, but looks good otherwiese! Could you please rebase? I'll merge
 it then!

Yep, but I will first have to make at least 1 test run. 

I also wanted to see if this couldn't be refactored a bit with the
SElinux one (since that's look if some function return true, load some
config, skip if start with '-', then apply another function taking a
parameter ), and this would permit to at least test part this part of
the logic without issue.

 One minor fix though:
 
  --- a/src/core/execute.c
  +++ b/src/core/execute.c
  @@ -68,6 +68,7 @@
   #include fileio.h
   #include unit.h
   #include async.h
  +#include apparmor-util.h
   
   #define IDLE_TIMEOUT_USEC (5*USEC_PER_SEC)
   #define IDLE_TIMEOUT2_USEC (1*USEC_PER_SEC)
  @@ -1570,6 +1571,16 @@ int exec_spawn(ExecCommand *command,
   goto fail_child;
   }
   }
  +
  +if (context-apparmor_profile) {
  +if (use_apparmor()) {
 
 Can you merge these two checks into one line, i.e. 
 
if (context-apparmor_profile  use_apparmor())
 
 or so? The nesting is already too deep...

Will do.
-- 
Michael Scherer

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] Apparmor profile switching support, v2

2014-02-14 Thread Michael Scherer 
This patch implement a option AppArmorProfile to load a specific
profile for a service, following the previous SELinux
patch for SELinuxProfile configuration. It also follow the same 
convention of being non-fatal if prefixed by -. I tested it on Opensuse
only for now, and the profile still need to be loaded another way.

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH] Add AppArmor profile switching

2014-02-14 Thread Michael Scherer 
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)
+
 # 
--
 noinst_LTLIBRARIES += \
libsystemd-units.la
diff --git a/configure.ac b/configure.ac
index 48d63e8..38dfa91 100644
--- a/configure.ac
+++ b/configure.ac
@@ -382,6 +382,18 @@ if test x$enable_selinux != xno; then
 fi
 AM_CONDITIONAL(HAVE_SELINUX, [test $have_selinux = yes])
 
+have_apparmor=no
+AC_ARG_ENABLE(apparmor, AS_HELP_STRING([--disable-apparmor], [Disable optional 
AppArmor support]))
+if test x$enable_apparmor != xno; then
+PKG_CHECK_MODULES([APPARMOR], [libapparmor],
+[AC_DEFINE(HAVE_APPARMOR, 1, [Define if AppArmor is 
available]) have_apparmor=yes], have_apparmor=no)
+if test x$have_apparmor = xno -a x$enable_apparmor = xyes; then
+AC_MSG_ERROR([*** AppArmor support requested but libraries not 
found])
+fi
+fi
+AM_CONDITIONAL(HAVE_APPARMOR, [test $have_apparmor = yes])
+
+
 AC_ARG_WITH(debug-shell,
 AS_HELP_STRING([--with-debug-shell=PATH],
 [Path to debug shell binary]),
@@ -1104,6 +1116,7 @@ AC_MSG_RESULT([
 PAM: ${have_pam}
 AUDIT:   ${have_audit}
 IMA: ${have_ima}
+AppArmor:${have_apparmor}
 SELinux: ${have_selinux}
 SECCOMP: ${have_seccomp}
 SMACK:   ${have_smack}
diff --git a/man/systemd.exec.xml b/man/systemd.exec.xml
index 01356e4..262638d 100644
--- a/man/systemd.exec.xml
+++ b/man/systemd.exec.xml
@@ -968,6 +968,19 @@
 /varlistentry
 
 varlistentry
+
termvarnameAppArmorProfile=/varname/term
+
+listitemparaTake a profile name as 
argument.
+The process executed by the unit will switch to
+this profile when started. Profiles must 
already
+be loaded in the kernel, or the unit will fail.
+This result in a non operation if AppArmor is 
not
+enabled. If prefixed by literal-/literal, 
all errors
+will be ignored.
+/para/listitem
+/varlistentry
+
+varlistentry
 termvarnameIgnoreSIGPIPE=/varname/term
 
 listitemparaTakes a boolean
diff --git a/src/core/build.h b/src/core/build.h
index f04f03f..3d7cd3e 100644
--- a/src/core/build.h
+++ b/src/core/build.h
@@ -45,6 +45,12 @@
 #define _SELINUX_FEATURE_ -SELINUX
 #endif
 
+#ifdef HAVE_APPARMOR
+#define _APPARMOR_FEATURE_ +APPARMOR
+#else
+#define _APPARMOR_FEATURE_ -APPARMOR
+#endif
+
 #ifdef HAVE_IMA
 #define _IMA_FEATURE_ +IMA
 #else
@@ -87,4 +93,4 @@
 #define _SECCOMP_FEATURE_ -SECCOMP
 #endif
 
-#define SYSTEMD_FEATURES _PAM_FEATURE_   _LIBWRAP_FEATURE_   
_AUDIT_FEATURE_   _SELINUX_FEATURE_   _IMA_FEATURE_   _SYSVINIT_FEATURE_ 
  _LIBCRYPTSETUP_FEATURE_   _GCRYPT_FEATURE_   _ACL_FEATURE_   
_XZ_FEATURE_ _SECCOMP_FEATURE_
+#define SYSTEMD_FEATURES _PAM_FEATURE_   _LIBWRAP_FEATURE_   
_AUDIT_FEATURE_   _SELINUX_FEATURE_   _IMA_FEATURE_   _SYSVINIT_FEATURE_ 
  _LIBCRYPTSETUP_FEATURE_   _GCRYPT_FEATURE_   _ACL_FEATURE_   
_XZ_FEATURE_   _SECCOMP_FEATURE_   _APPARMOR_FEATURE_
diff --git a/src/core/dbus-execute.c b/src/core/dbus-execute.c
index a62f517..286ae7d 100644
--- a/src/core/dbus-execute.c
+++ b/src/core/dbus-execute.c
@@ -524,6 +524,7 @@ const sd_bus_vtable bus_exec_vtable[] = {
 SD_BUS_PROPERTY(SameProcessGroup, b, bus_property_get_bool, 
offsetof(ExecContext, same_pgrp), SD_BUS_VTABLE_PROPERTY_CONST),

Re: [systemd-devel] [PATCH] Add AppArmor profile switching

2014-02-14 Thread Michael Scherer 
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

2014-02-14 Thread Michael Scherer 
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

[systemd-devel] [PATCH] export SELinuxContext on the bus as a structure

2014-02-14 Thread Michael Scherer 
This permit to hide the logic of prefixing by '-' from the consumer
of the DBus API, by presenting a boolean and a string rather than just
a raw string, with specific magic value. See 
http://lists.freedesktop.org/archives/systemd-devel/2014-February/016918.html
---
 src/core/dbus-execute.c | 21 -
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/src/core/dbus-execute.c b/src/core/dbus-execute.c
index a62f517..559aa63 100644
--- a/src/core/dbus-execute.c
+++ b/src/core/dbus-execute.c
@@ -75,6 +75,25 @@ static int property_get_environment_files(
 return sd_bus_message_close_container(reply);
 }
 
+static int property_get_ignorable_property(
+sd_bus *bus,
+const char *path,
+const char *interface,
+const char *property,
+sd_bus_message *reply,
+void *userdata,
+sd_bus_error *error) {
+
+char *c = userdata;
+
+assert(bus);
+assert(reply);
+assert(c);
+
+return sd_bus_message_append(reply, (bs), c[0] == '-', c[0] == '-' ? 
c + 1 : c);
+}
+
+
 static int property_get_rlimit(
 sd_bus *bus,
 const char *path,
@@ -523,7 +542,7 @@ const sd_bus_vtable bus_exec_vtable[] = {
 SD_BUS_PROPERTY(PrivateDevices, b, bus_property_get_bool, 
offsetof(ExecContext, private_devices), SD_BUS_VTABLE_PROPERTY_CONST),
 SD_BUS_PROPERTY(SameProcessGroup, b, bus_property_get_bool, 
offsetof(ExecContext, same_pgrp), SD_BUS_VTABLE_PROPERTY_CONST),
 SD_BUS_PROPERTY(UtmpIdentifier, s, NULL, offsetof(ExecContext, 
utmp_id), SD_BUS_VTABLE_PROPERTY_CONST),
-SD_BUS_PROPERTY(SELinuxContext, s, NULL, offsetof(ExecContext, 
selinux_context), SD_BUS_VTABLE_PROPERTY_CONST),
+SD_BUS_PROPERTY(SELinuxContext, (bs), 
property_get_ignorable_property, offsetof(ExecContext, selinux_context), 
SD_BUS_VTABLE_PROPERTY_CONST),
 SD_BUS_PROPERTY(IgnoreSIGPIPE, b, bus_property_get_bool, 
offsetof(ExecContext, ignore_sigpipe), SD_BUS_VTABLE_PROPERTY_CONST),
 SD_BUS_PROPERTY(NoNewPrivileges, b, bus_property_get_bool, 
offsetof(ExecContext, no_new_privileges), SD_BUS_VTABLE_PROPERTY_CONST),
 SD_BUS_PROPERTY(SystemCallFilter, (bas), 
property_get_syscall_filter, 0, SD_BUS_VTABLE_PROPERTY_CONST),
-- 
1.8.5.3

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 1/3] Add SELinuxContext configuration item

2014-02-07 Thread Michael Scherer 
Le jeudi 06 février 2014 à 12:21 -0800, David Timothy Strauss a écrit :
 In order to maximize consistency with newly committed options in
 systemd-nspawn, would it make sense to allow independent configuration
 of the process and file labels instead?


The file label are decided by selinux policy based on the path and/or
process domain, from what I seen.

In the case of systemd-nspawn, it is done by using a specific option of
mount, and only for tmpfs/devpts. 

So I am not sure if this can be done, and i fail to see a usecase for
that ( except having container described in .service, which could be
nice but maybe too much )

-- 
Michael Scherer

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH 2/3] Ignore the setting SELinuxContext if selinux is not enabled

2014-02-06 Thread Michael Scherer 
---
 src/core/execute.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/core/execute.c b/src/core/execute.c
index c02c768..474a4af 100644
--- a/src/core/execute.c
+++ b/src/core/execute.c
@@ -1569,7 +1569,7 @@ int exec_spawn(ExecCommand *command,
 }
 }
 #ifdef HAVE_SELINUX
-if (context-selinux_context) {
+if (context-selinux_context  use_selinux()) {
 err = 
security_check_context(context-selinux_context);
 if (err  0) {
 r = EXIT_SELINUX_CONTEXT;
-- 
1.8.5.3

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 1/2] Add switch_apparmor_profile helper, to switch the profile of the next command to run. This can be used to load a custom apparmor profile for a unit.

2014-01-06 Thread Michael Scherer 
Le lundi 06 janvier 2014 à 03:20 +0100, Zbigniew Jędrzejewski-Szmek a
écrit :
 On Fri, Jan 03, 2014 at 05:22:42PM +0100, m...@zarb.org wrote:
  From: Michael Scherer m...@zarb.org
  
  ---
   src/shared/apparmor-util.c | 15 +++
   src/shared/apparmor-util.h |  1 +
   2 files changed, 16 insertions(+)
  
  diff --git a/src/shared/apparmor-util.c b/src/shared/apparmor-util.c
  index 2b85da1..a75bec4 100644
  --- a/src/shared/apparmor-util.c
  +++ b/src/shared/apparmor-util.c
  @@ -39,3 +39,18 @@ bool use_apparmor(void) {
   
   return use_apparmor_cached;
   }
  +
  +int switch_apparmor_profile(const char * profile) {
  +_cleanup_free_ char *filename = NULL;
  +_cleanup_fclose_ FILE *proc = NULL;
  +
  +if (asprintf (filename, /proc/%d/attr/exec, getpid()) 0)
  +return -ENOMEM;
  +
  +proc = fopen (filename, w);
  +if (! proc)
  +return -errno;
  +
  +fprintf (proc, exec %s\n, profile);
  +return 0;
  +}
 This should be something like
 
 int apparmor_switch_profile(const char *profile) {
 char *p, *t;
 
 p = procfs_file_alloca(0, attr/exec);
   t = strappenda(exec , profile);
 
   return write_string_file(p, t);
 }
 
 Totally untested, but there's no unnecessary malloc, and there's
 a meaningful error returned if the thing most likely to fail, i.e. the
 write, actually fails.
   
I rewrote this part using libapparmor, so the new patch is simpler
( didn't send yet, I am adding the support of ignoring with '-' and
doing a few more tests ), so please do not merge this one :).

I will also look at adding a test, but this requires kernel support to
work ( but I can test this is a no-op ).
-- 
Michael Scherer

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] Add SELinuxContext configuration item

2014-01-03 Thread Michael Scherer 
Le vendredi 03 janvier 2014 à 00:58 +, Jóhann B. Guðmundsson a
écrit :
 On 12/28/2013 01:30 PM, Lennart Poettering wrote:
  On Fri, 27.12.13 23:26,m...@zarb.org  (m...@zarb.org) wrote:
 
  From: Michael Schererm...@zarb.org
  
  This permit to let system administrators decide of the domain of a 
  service.
  This can be used with templated units to have each service in a différent
  domain ( for example, a per customer database, using MLS or anything ),
  or can be used to force a non selinux enabled system (jvm, erlang, etc)
  to start in a different domain for each service.
  Hmm, so far (as I understood it) the SELinux guys always wanted to make
  sure that label configuration stays in the the selinux database and
  nowhere else.
 
 Right and if you add this you need to add something for the other 
 security solutions as well ( apparmor etc ).

I do not see why we need to equally support all MAC frameworks for each
change we do. And while I am familiar enough with apparmor to create a
equivalent setting ( and plan to do ), I have no idea on how to
translate the concept for smack, ima and tomoyo. 

In fact, the mere fact that tomoyo is not handled at all already show
that we do treat all security framework as equal.

 This introduces yet another place for administrators to look at while 
 debugging problems so the question to ask here is.
 
 Is adding this ability to unit files the right way to solve what's 
 trying to be solved here?

As Dan said, yes. 

Usually, the type of transition from 1 domain to another is done at the
kernel level based on the label of the file executed. See
https://wiki.gentoo.org/wiki/SELinux/Tutorials/How_does_a_process_get_into_a_certain_context
 
and http://danwalsh.livejournal.com/23944.html 

However, as said, there is some case where the approach of making the
transition based on the executed filename is not sufficient. For
example, the erlang vm, the jvm, etc, because each software will run in
the same domain, in different processes, because that's always the same
jvm being used. See the bug I mentioned before. 

Now, if you have a more precise feedback and/or a better proposal, I am
ready to hear of it, but the only alternative I see is to make the JVM,
erlang, etc, to be SELinux aware. That's a much bigger task, and I am
not sure that's worth the code duplication ( not to mention that it make
sysadmin look in several different places ). And the design I was
thinking of ( ie, replicated the current system of doing transition
based on file label ) would requires reimplementing the kernel code in
userspace, in libselinux, and I would rather avoid this for various
reasons ( security, code duplication avoidance ).

Another solution would be to create shell wrapper for every java, erlang
and mono software, and then use process transition on the shell wrapper,
but that doesn't scale that well and do not offer the flexibility of the
current patch to the sysadmin. ( and would likely be Fedora specific as
well ).

-- 
Michael Scherer

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] Add SELinuxContext configuration item

2014-01-03 Thread Michael Scherer 
Le vendredi 03 janvier 2014 à 12:23 +, Jóhann B. Guðmundsson a
écrit :
 On 01/03/2014 10:56 AM, Michael Scherer wrote:
  Le vendredi 03 janvier 2014 à 00:58 +, Jóhann B. Guðmundsson a
  écrit :
  On 12/28/2013 01:30 PM, Lennart Poettering wrote:
  On Fri, 27.12.13 23:26,m...@zarb.org  (m...@zarb.org) wrote:
 
  From: Michael Schererm...@zarb.org
 
  This permit to let system administrators decide of the domain of a 
  service.
  This can be used with templated units to have each service in a 
  différent
  domain ( for example, a per customer database, using MLS or anything ),
  or can be used to force a non selinux enabled system (jvm, erlang, etc)
  to start in a different domain for each service.
  Hmm, so far (as I understood it) the SELinux guys always wanted to make
  sure that label configuration stays in the the selinux database and
  nowhere else.
  Right and if you add this you need to add something for the other
  security solutions as well ( apparmor etc ).
  I do not see why we need to equally support all MAC frameworks for each
  change we do.
 
 Because people will start to expect being able to configure that there.

So they can as well start to fill bug report and start to contribute
code for this.

We didn't added detection of all security framework for
ConditionSecurity at the first patch, it was added later as people
expressed interest ( hence the lack of tomoyo ), so I expect the same to
be done for security frameworks. Also, having done my homework, IMA do
not have the concept of domain, apparmor has profiles, and I have no
idea for smack, so I prefer to defer integration to people who know,
based on their use cases.

And while I am familiar enough with apparmor to create a
  equivalent setting ( and plan to do ), I have no idea on how to
  translate the concept for smack, ima and tomoyo.
 
  In fact, the mere fact that tomoyo is not handled at all already show
  that we do treat all security framework as equal.
 
 How do you suppose we deal with man pages if selinux is not installed 
 but still refer to this.

In the same way we do for all #ifdef feature. For example, for PAMName,
the documentation is always present. 

 Wont users also need to check if selinux is enabled or not in the unit 
 file?

I would rather do it in the C code, no need to have everybody asking for
it.

 Should systemd warn users if selinux is not installed,enabled and fail or?

It all depend. Either we are consistent with the other settings ( ie,
setting a syscall filter will fail if not supported on the kernel ), and
so fail, or we decide that selinux is special and we should silently
ignore failure if it cannot be applied.

I choose the first one for the first patch, but adding a conditional
would be trivial if we decide to silently ignore if the setting cannot
be applied. 

The main issue being of course that people who disable selinux do not
always properly disable it ( ie using permissive rather than selinux=0
).


 
 
  This introduces yet another place for administrators to look at while
  debugging problems so the question to ask here is.
 
  Is adding this ability to unit files the right way to solve what's
  trying to be solved here?
  As Dan said, yes.
 
 I would prefer if app writers do not start hard coding SELinux contexts 
 into the unit file
 
 I hardly call that solid yes and this is what will start taking place if 
 this is introduced into the unit files.

Then what about :
I like this patch, and I have seen people saying we have this
capability in RHEL7  :^(

We should separate the concern about people in distribution/upstream
using it if offered and the rest of the world. Distribution policy is a
matter of the distribution. If Fedora wish to forbid this unless there
is a good use case, that's up to Fedora to do it, and to have it
integrated into rpmlint or anything.

I must also say that I didn't really see a rush from application
developers to add selinux support or anything, and that people can
already distribute policy along their application, with all support
problem this could create for distributions. So we already have the
problem, adding the setting in systemd wouldn't change much. 

 
  However, as said, there is some case where the approach of making the
  transition based on the executed filename is not sufficient. For
  example, the erlang vm, the jvm, etc, because each software will run in
  the same domain, in different processes, because that's always the same
  jvm being used. See the bug I mentioned before.
 
  Now, if you have a more precise feedback and/or a better proposal,
 
 Add this to the daemon startup itself ( the confile or the code ) and or 
 use runcon in an exec start pre section to set this up.

Runcon cannot be used in ExecStartPre, that's like su. So you have to
run it in ExecStart, and this make things harder for sysadmins, and
doesn't look like in line with systemd philosophy since that's replacing
configuration by code. 

On the integration

Re: [systemd-devel] Apparmor profile switching support

2014-01-03 Thread Michael Scherer 
Le vendredi 03 janvier 2014 à 17:22 +0100, m...@zarb.org a écrit :
 As discussed on the SELinux thread, this patch attempt to offer the same
 level of configuration for Apparmor distributions by permitting to the
 sysadmin to set the profile used by a unit. I didn't tested it but would 
 like to get early feedback on it from openSUSE and Ubuntu users, as they
 are the 2 main set of users of AppArmor.
 
 Main inspiration come from the upstart support, on 
 https://code.launchpad.net/~mdeslaur/upstart/apparmor-support
 However, we are currently lacking the capacity of using directly a on disk 
 profile, and
 I am not sure on the best way to support that. 

I have also been told on irc that Michael Stapelberg wrote the same kind
of patch ( if not the same, given there isn't much possible variation ),
cf https://lists.debian.org/debian-security/2014/01/msg8.html

-- 
Michael Scherer

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] Add SELinuxContext configuration item

2014-01-03 Thread Michael Scherer 
Le vendredi 03 janvier 2014 à 18:21 +0100, Zbigniew Jędrzejewski-Szmek a
écrit :
 On Fri, Jan 03, 2014 at 11:48:49AM -0500, Daniel J Walsh wrote:
   Should systemd warn users if selinux is not installed,enabled and fail
   or?
   
   It all depend. Either we are consistent with the other settings ( ie, 
   setting a syscall filter will fail if not supported on the kernel ), and 
   so
   fail, or we decide that selinux is special and we should silently ignore
   failure if it cannot be applied.
   
   I choose the first one for the first patch, but adding a conditional would
   be trivial if we decide to silently ignore if the setting cannot be
   applied.
 I think the usual style of - as the first character of RHS meaning that
 the setting can be ignored should be used.
 
 In general, if selinux=0 is used, or selinux support is not compiled
 in, those options should not result in failure. So the algorithm should
 be: if disabled, ignore, if enabled, and impossible to apply, fail, unless
 - was prefixed.

Good idea, i have coded that, I will test and send it later.

-- 
Michael Scherer

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] Add SELinuxContext configuration item

2014-01-02 Thread Michael Scherer 
Le jeudi 02 janvier 2014 à 11:30 -0500, Daniel J Walsh a écrit :
 On 12/28/2013 11:47 AM, Michael Scherer wrote:
  Le samedi 28 décembre 2013 à 14:30 +0100, Lennart Poettering a écrit :
  On Fri, 27.12.13 23:26, m...@zarb.org (m...@zarb.org) wrote:
  
  From: Michael Scherer m...@zarb.org
  
  This permit to let system administrators decide of the domain of a
  service. This can be used with templated units to have each service in
  a différent domain ( for example, a per customer database, using MLS or
  anything ), or can be used to force a non selinux enabled system (jvm,
  erlang, etc) to start in a different domain for each service.
  
  Hmm, so far (as I understood it) the SELinux guys always wanted to make 
  sure that label configuration stays in the the selinux database and 
  nowhere else.
  
  Yep, I know and they are right, we shouldn't put configuration everywhere
  in the system. But there is a few cases where the selinux policy cannot
  express what we want ( or at least, I do not know how to do it ).
  
  First case is doing something like openshift, with 1 different domain per
  user. Since the transitions are mostly handled in kernel space ( except for
  specific case like sepostgresql ), it kinda restrict what we can do in term
  of smart transition. In the case of openshift, this use a specific pam
  module (pam_openshift) and specific plugins in the code, because the policy
  is a bit too static to handle that.
  
  So using templated units, we could do for example : 
  SELinuxContext=staff_u:staff_r:%s_t:s0-s0:c0.c1023
  
  or similar tricks. Or just handle things manually, with static
  SELinuxContext ( or include, or anything ).
  
  
  The second case is caused by a limitation of the current way of doing 
  transition. My main motivation is to solve 
  https://bugzilla.redhat.com/show_bug.cgi?id=969757 , because right now, we
  cannot ask to erlang to run in a different domain unless if we write 1
  shell wrapper per erlang application just to trigger the transition, or
  until we make erlang selinux-aware, like postgresql is. So rather than
  duplicate shell wrappers or adding code everywhere, I was thinking doing it
  in systemd directly would be beneficial for everybody by reducing needed
  changes to the only stuff that matter, having the context we want to switch
  to.
  
  I do not expect SELinuxContext to be used by upstream units, and I guess 
  that a distribution can decide to have them being unused by policy if 
  that's a issue.
  
  It should also be noted that upstart do support a similar configuration for
  apparmor, 
  https://code.launchpad.net/~mdeslaur/upstart/apparmor-support/+merge/164169
 
  
 And I was pondering on adding it as well to systemd, since some systemd
  consumers are using apparmor, and because feature parity is IMHO important
  if we want to let people use systemd on Ubuntu.
  
  
  I'd like Dan Walsh's opinion whether this addition fits into what the 
  SELinux guys want or not. Dan?
  
  Patch looks fine though.
  
 
 I like this patch, and I have seen people saying we have this capability in
 RHEL7  :^(
 
 Currently people in a sysvinit scripts are using runcon for similar features.
  And as someone described handling of java, mono, erlang type scripts where
 the command is not easy to differentiate.
 
 We also allow people to do something similar with sudo.  ROLE=unconfined_r
 TYPE=unconfined_t.
 
 I would prefer if app writers do not start hard coding SELinux contexts into
 the unit files, but allowing administrators or third parties like openshift to
 override I do not have a problem with it.
 
 It could allow a administrator to say run this script as unconfined_t, which
 may or may not cause other problems.
 
 We might want to allow more granularity on this then just context.Since we
 allow sudo to specify role and type and we allow runcon to specify all fields
 of SELinux.

IE, you would like to have something like SELinuxRole, SELinuxType,
SELinuxRange and SELinuxUser, each permitting to override 1 single field
of the context ?

It shouldn't be that hard to do ( a bit longer to properly test maybe ),
I am however not sure if we should keep the 2 styles of configuration
For example, in what order would the different field be applied, if I
set SELinuxuser and the context etc.

And if we use 4 configurations directives instead of 1, it also make the
request for a default SELinux context asked by David a bit harder and a
bit less elegant ( since that would mean 4 directive for the settings, 1
directive for disabling, and 4 directive for each default of the field,
unless we only offer default context ).

I will try to cook a new version of the patch with 4 splitted fields for
next week while we discuss the proposal
-- 
Michael Scherer

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] Add SELinuxContext configuration item

2013-12-31 Thread Michael Scherer 
Le lundi 30 décembre 2013 à 03:14 -0600, David Timothy Strauss a écrit :
 On Sat, Dec 28, 2013 at 10:47 AM, Michael Scherer m...@zarb.org wrote:
  So using templated units, we could do for example :
  SELinuxContext=staff_u:staff_r:%s_t:s0-s0:c0.c1023
 
 In the spirit of making isolation easy, it would be neat to have a
 built-in convention for selinux isolation in systemd where the full
 service/unit name has a default context name, constructed much like
 the quoted example, that the admin or packager can use simply by
 turning isolation on (SELinux=true).
 
 We would love to use SELinuxContext= or SELinux= for our needs at Pantheon.

Using SELinux=true is a bit weird when it come to the naming, because
SELinux=false wouldn't disable selinux, it would just let the current
policy do the transition, that's a bit misleading.

I am not sure of the value of having 2 configuration file doing the same
thing. What about 
SELinuxContext=auto , and so replace auto by some default configuration
in that case ?

-- 
Michael Scherer

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] Add SELinuxContext configuration item

2013-12-28 Thread Michael Scherer 
Le samedi 28 décembre 2013 à 14:30 +0100, Lennart Poettering a écrit :
 On Fri, 27.12.13 23:26, m...@zarb.org (m...@zarb.org) wrote:
 
  From: Michael Scherer m...@zarb.org
  
  This permit to let system administrators decide of the domain of a service.
  This can be used with templated units to have each service in a différent
  domain ( for example, a per customer database, using MLS or anything ),
  or can be used to force a non selinux enabled system (jvm, erlang, etc)
  to start in a different domain for each service.
 
 Hmm, so far (as I understood it) the SELinux guys always wanted to make
 sure that label configuration stays in the the selinux database and
 nowhere else.

Yep, I know and they are right, we shouldn't put configuration
everywhere in the system. But there is a few cases where the selinux
policy cannot express what we want ( or at least, I do not know how to
do it ).

First case is doing something like openshift, with 1 different domain
per user. Since the transitions are mostly handled in kernel space
( except for specific case like sepostgresql ), it kinda restrict what
we can do in term of smart transition. In the case of openshift, this
use a specific pam module (pam_openshift) and specific plugins in the
code, because the policy is a bit too static to handle that.

So using templated units, we could do for example :
SELinuxContext=staff_u:staff_r:%s_t:s0-s0:c0.c1023

or similar tricks.
Or just handle things manually, with static SELinuxContext ( or include,
or anything ).


The second case is caused by a limitation of the current way of doing
transition. My main motivation is to solve
https://bugzilla.redhat.com/show_bug.cgi?id=969757 , because right now,
we cannot ask to erlang to run in a different domain unless if we write
1 shell wrapper per erlang application just to trigger the transition,
or until we make erlang selinux-aware, like postgresql is. 
So rather than duplicate shell wrappers or adding code everywhere, I was
thinking doing it in systemd directly would be beneficial for everybody
by reducing needed changes to the only stuff that matter, having the
context we want to switch to.

I do not expect SELinuxContext to be used by upstream units, and I guess
that a distribution can decide to have them being unused by policy if
that's a issue.

It should also be noted that upstart do support a similar configuration
for apparmor,
https://code.launchpad.net/~mdeslaur/upstart/apparmor-support/+merge/164169
And I was pondering on adding it as well to systemd, since some systemd
consumers are using apparmor, and because feature parity is IMHO
important if we want to let people use systemd on Ubuntu.


 I'd like Dan Walsh's opinion whether this addition fits into what the
 SELinux guys want or not. Dan?
 
 Patch looks fine though.

-- 
Michael Scherer

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] runtime directories for services vs. tmpfiles

2013-07-19 Thread Michael Scherer 
Le mardi 16 juillet 2013 à 17:59 +0100, Colin Guthrie a écrit :
 'Twas brillig, and Kay Sievers at 16/07/13 17:24 did gyre and gimble:
  On Tue, Jul 16, 2013 at 6:01 PM, Lennart Poettering
  lenn...@poettering.net wrote:
  
  Hmm, I'd like such an automatism, but I'd really prefer if we could come
  up with some scheme to automatically determine all tmpfiles snippets in
  the package and apply them all automatically. But I am not sure how that
  could be done with current RPM.
 
  In fact, I'd actually like to do the same for the %systemd_post macros,
  and suchlike, where people tend to be pretty bad at always listing all
  unit files correctly.
 
 Actually rereading this bit above, I can see the desire for automated
 snippets, but are there not some cases where you would want to avoid
 calling %systemd_post macros for some shipped units? e.g. they may only
 be used internally by others (Requires=, Also= and similar). I think
 such automation might not be good. (just look at the NFS units which I
 think we've finally cleaned up a bit in Mageia - enough to actually work
 at least).
 
  Hmm, an rpmlint check for stuff like this maybe is the first step? At
  least people who care and look at that would fix their stuff?
 
 Misc was looking at doing most of that. Not sure what you mean about
 naming tho'... do you just mean the folder name (etc vs usr) or just not
 picking very nice unit names?
 
 For the folder, I put rpmlint checks in Mageia a while ago to ban units,
 tmpfiles and udev rules from shipping in /etc.
 
 And another to do with non-ghost files in /run or /var/run.
 
 I had it in my head that I'd spoken to Misc about this at the time I
 wrote them but now I have a paranoid fear that maybe I didn't... They
 are simple enough anyway, so if they are not upstream and have instead
 been reimplemented then it's no great loss, but apologies if forgot to
 ping you at the time Misc (and I've seen you twice in person since then
 too!!)
 
 Patches here. Can't check if it's upstream yet as rpmlint.zarb.org is
 down for now...

We are now on sf.net.

And someone already contacted me to get the patches, I said i was ok on
the principle, but from the code point of view, it could have been
refactored ( but I didn't do it yet );

IE having a structure like :
['regexp','name-of-exception','message'], have it exposed in the
configuration and have a generic module using this, so every
distribution could add them.

IIRC, Suse do have this kind of patchs, but I didn't merged them yet.
 

-- 
Michael Scherer

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] System units packaging and rpmlint

2013-05-21 Thread Michael Scherer 
Le lundi 20 mai 2013 à 19:58 -0700, T.C. Hollingsworth a écrit :
 On Sat, May 18, 2013 at 2:44 PM, Michael Scherer m...@zarb.org wrote:
  So I planned to warn if the unit are directly in /lib, but I know there
  is some distribution that didn't choose this path yet. So when /usr is
  not merged, what is the canonical location ( ie, for Opensuse, Mandriva,
  since they are both rpm based ) ?
 
 Shouldn't rpmlint just make sure the spec uses %{_unitdir}?  Nothing
 except the systemd package should hardcode this directory.

Parsing spec is not something I will attempt
It cannot been done in a robust way ( or at least, wasn't last time I
looked ).

 (Or if the check has to be on the binary RPM side, just `rpm --eval
 '%_unitdir'` for the right location?)

I already do that, but I look in that directory to see if there is a
file for systemd ( I was using *.service, but since dbus use the same
suffix ). So by definition, i cannot detect out of that directory file
to warn about them ( unless for well know error like in /etc ).


-- 
Michael Scherer

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] System units packaging and rpmlint

2013-05-19 Thread Michael Scherer 
Le samedi 18 mai 2013 à 23:50 -0700, David Strauss a écrit :
 I'm skipping to the questions I can answer.
 
 On Sat, May 18, 2013 at 2:44 PM, Michael Scherer m...@zarb.org wrote:
  - we should avoid as much as possible to use Type=forking when we can
  avoid it.
 
  This one is likely the one that will be met with resistance from
  packagers, so before adding it as warning, I would like to be sure that
  I am not totally wrong.
 
  A standard daemon will fork ( likely twice ) in the background,  do
  various stuff and then write the pidfile on /run.
 
  By not going in the background, we can :
  - avoid forking 1 or 2 times for nothing,
  - avoid taking memory for /run, and avoid taking a inode
 
  So that's should be a little bit better in most case, or do I miss
  anything ?
  ( ie, something like this is so negligible that we shouldn't care )
 
 That's not correct. Doing Type=simple services is generally
 undesirable because they lack proper support for dependent units only
 starting after startup completes (versus when startup begins, which is
 all systemd knows about a Type=simple unit). Type=notify is the most
 desirable because it gives the benefits you list for Type=simple but
 still allows proper dependency effects. However, Type=notify requires
 use of systemd APIs from the daemon's own code.

In this case, that's Type=simple vs Type=forking. IE, is Type=forking
better in term of having the startup being complete in the general
case ? ( of course, there is always exception, but  ).

Type=notify is better, but that requires patching daemon.

However, I could check if the binary is linked with libsystemd-daemon
and trigger a warning for using Type=notify.

 If a service is socket-activiated, though, dependencies on the service
 itself are generally unnecessary, as units can depend on the socket
 instead. In such situations, Type=simple is quite good, with
 Type=notify offering almost only academic advantages over Type=simple.

Socket activated is another beast, and I do not know how I can check if
a binary can be socket activated to trigger a warning.

  - some package install directly file in /usr/lib/systemd/system/*.wants
  There is some special case ( like plymouth ), but usually, that
  shouldn't be done directly in the package, but better done in %post, and
  in /etc ?
 
 No, the systemd ideal is that packages should not ever install systemd
 units or settings into /etc. Lennart's long-term goal is for daemons
 to not install anything by default to /etc, but I think our scope of
 authority in this discussion ends with systemd-related matters.
 
  IE, is this right to make a warning or a error when that occurs ?
 
 It should be an error.

For /etc, yes, that's planned.

But as said in the thread, my issue is :

A package ship /usr/lib/systemd/system/foo.wants/bar.service as a link
to /usr/lib/systemd/system/bar.service

instead of using WantedBy=bar.service 

Is there one technical reason to prefer the former to the latter ?
I would be tempted to mandate to use WantedBy because that would be more
consistant with most packages, and hardcoding that setting on the FS
forbid to override it in /etc, but maybe that's the goal. 

  I was also interested into checking the syntax of systemd file, but
  since systemd is moving quite fast, I doubt to be able to keep a up to
  date parser of unit/service/timer/snapshot files. And duplicating code
  of systemd in python do not seems like a smart move.
 
 It should be possible to invoke something in systemd to sanity-check
 the included unit files.
 
 Is it okay if the Python invokes a subprocess? 

It is ok. Bonus point for a parsable output, but just having a exit code
would be ok for integration and let people run the binary and read
errors.

 If not, I work to add
 unit file sanity checking to the C APIs and extend our Python
 bindings.

I think since lintian ( a similar tool for debian package ) is in perl,
so having it in a external binary would be better for them.

Now, from a deployment point of view, desktop-file-validate was
backported on stable version for our builders at Mageia, and since the
tool was self contained, that was ok, but pulling over a complete
systemd backport on the server would likely be refused by any sysadmins,
so if the tool could be compiled as a standalone without interfering
with existing systemd installation, that would be nice. 
( if not, I guess they can use some chroot tricks as i do for others
quality checking tools )

-- 
Michael Scherer

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] System units packaging and rpmlint

2013-05-18 Thread Michael Scherer 
Hi,

After the last discussion on fedora-devel about boot times[1], I have
been looking at the various units on my disk and if I could code
something in rpmlint ( tool to verify a rpm against a set of guidelines
) to have packagers follow some best practices.

So I have been writing various checks that emit Warning/Errors, and
would like to get some feedback on a few assumption I made when writing
the checks :

- pid file should be in /run, or in a subdirectory of /run
I have seen a few service who still use /var/run, but can I safely
assume that anything booting with systemd would have /run?
(and so warn if something is using /run for that) 

- unit file should be in %_unitdir, which is on /usr/lib for
distribution with merged /usr ( at least, on Fedora and Mageia )

So I planned to warn if the unit are directly in /lib, but I know there
is some distribution that didn't choose this path yet. So when /usr is
not merged, what is the canonical location ( ie, for Opensuse, Mandriva,
since they are both rpm based ) ?


- we should avoid as much as possible to use Type=forking when we can
avoid it. 

This one is likely the one that will be met with resistance from
packagers, so before adding it as warning, I would like to be sure that
I am not totally wrong.

A standard daemon will fork ( likely twice ) in the background,  do
various stuff and then write the pidfile on /run. 

By not going in the background, we can :
- avoid forking 1 or 2 times for nothing,
- avoid taking memory for /run, and avoid taking a inode

So that's should be a little bit better in most case, or do I miss
anything ?
( ie, something like this is so negligible that we shouldn't care )

If there is other good argument to add in the explanation of the error,
that would help to convince the users.


- if using Type=forking, it is better to use PIDFile, 
While systemd seems to support fine to guess the main pid, I think this
should be avoided when possible , according to
http://lists.freedesktop.org/archives/systemd-devel/2011-June/002690.html . 
So does it make sense to send a warning if there is a service that do
not use PIDFile and of type Forking ?

- some package install directly file in /usr/lib/systemd/system/*.wants
There is some special case ( like plymouth ), but usually, that
shouldn't be done directly in the package, but better done in %post, and
in /etc ?

IE, is this right to make a warning or a error when that occurs ?


I was also interested into checking the syntax of systemd file, but
since systemd is moving quite fast, I doubt to be able to keep a up to
date parser of unit/service/timer/snapshot files. And duplicating code
of systemd in python do not seems like a smart move. 

I didn't found any way to reuse systemd code, but I think that a tool
like desktop-file-validate would be quite useful for all distributions.


[1] http://lists.fedoraproject.org/pipermail/devel/2013-May/182697.html

-- 
Michael Scherer

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel