systemd: Intermediate --chroot fix with the new sd_notify() implementation

Commit c5931897ae8d663e7e introduced support for talking directly to the systemd service manager about the situation for the OpenVPN tunnel. This approach makes a lot of sense and is mostly the proper way to do it. But it was discovered that it breaks OpenVPN configurations using --chroot.

The reason sd_notify() calls fails when using chroot() is that sd_notify() expects to have access to a file as declared in the $NOTIFY_SOCKET environment variable. It is the main systemd instance which is responsible to provide both the environment variable as well as the socket file sd_nodify() should use. When --chroot comes into play, the $NOTIFY_SOCKET file will not be available for OpenVPN any more.

As things are getting close to the 2.4_rc2 release we will not dare to bring a too invasive fix. As well we need some time to discuss an approrpriate solution. So this intermediate fix will only provide a "successful start" message to the systemd service manager right before chroot() happens. This will at least resolve the issue in a safe and non-intrusive way.

Signed-off-by David Sommerseth <[email protected]...>

---
 src/openvpn/init.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

src/openvpn/init.c +3 -1

index 74f1139..e47f0d4 100644
@@ -967,7 +967,27 @@ do_uid_gid_chroot (struct context *c, bool no_delay)
       if (c->options.chroot_dir)
 	{
 	  if (no_delay)
-	    platform_chroot (c->options.chroot_dir);
+            {
+#ifdef ENABLE_SYSTEMD
+              /* If OpenVPN is started by systemd, the OpenVPN process

needs
+ * to provide a preliminary status report to systemd. This
is

src/openvpn/init.c +8 -0

+               * needed as $NOTIFY_SOCKET will not be available inside the
+               * chroot, which sd_notify()/sd_notifyf() depends on.
+               *
+               * This approach is the simplest and the most non-intrusive
+               * solution right before the 2.4_rc2 release.
+               *
+               * TODO: Consider altnernative solutions - bind mount?
+               * systemd does not grok OpenVPN configuration files, thus

cannot
+ * have a sane way to know if OpenVPN will chroot or not and
to

src/openvpn/init.c +4 -0

+               * which subdirectory it will chroot into.
+               */
+              sd_notifyf(0, "READY=1\n"
+                "STATUS=Entering chroot, most of the init completed

successfully\n"

David Sommerseth <[email protected]...> on Wed, 2016/12/07 03:51:

Quoted 58 lines

Commit c5931897ae8d663e7e introduced support for talking directly to the systemd service manager about the situation for the OpenVPN tunnel. This approach makes a lot of sense and is mostly the proper way to do it. But it was discovered that it breaks OpenVPN configurations using --chroot.

The reason sd_notify() calls fails when using chroot() is that sd_notify() expects to have access to a file as declared in the $NOTIFY_SOCKET environment variable. It is the main systemd instance which is responsible to provide both the environment variable as well as the socket file sd_nodify() should use. When --chroot comes into play, the $NOTIFY_SOCKET file will not be available for OpenVPN any more.

As things are getting close to the 2.4_rc2 release we will not dare to bring a too invasive fix. As well we need some time to discuss an approrpriate solution. So this intermediate fix will only provide a "successful start" message to the systemd service manager right before chroot() happens. This will at least resolve the issue in a safe and non-intrusive way.

Signed-off-by David Sommerseth <[email protected]...>

---
 src/openvpn/init.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

src/openvpn/init.c +3 -1

index 74f1139..e47f0d4 100644
@@ -967,7 +967,27 @@ do_uid_gid_chroot (struct context *c, bool no_delay)
       if (c->options.chroot_dir)
 	{
 	  if (no_delay)
-	    platform_chroot (c->options.chroot_dir);
+            {
+#ifdef ENABLE_SYSTEMD
+              /* If OpenVPN is started by systemd, the OpenVPN process

needs
+ * to provide a preliminary status report to systemd. This
is
+ * needed as $NOTIFY_SOCKET will not be available inside
the

src/openvpn/init.c +7 -0

+               * chroot, which sd_notify()/sd_notifyf() depends on.
+               *
+               * This approach is the simplest and the most non-intrusive
+               * solution right before the 2.4_rc2 release.
+               *
+               * TODO: Consider altnernative solutions - bind mount?
+               * systemd does not grok OpenVPN configuration files, thus

cannot
+ * have a sane way to know if OpenVPN will chroot or not
and to

src/openvpn/init.c +4 -0

+               * which subdirectory it will chroot into.
+               */
+              sd_notifyf(0, "READY=1\n"
+                "STATUS=Entering chroot, most of the init completed

successfully\n"

src/openvpn/init.c do_uid_gid_chroot (struct context *c, bool no_delay) · line 993

Quoted 7 lines

src/openvpn/init.c +4 -0

+                "MAINPID=%lu", (unsigned long) getpid());
+#endif
+              platform_chroot (c->options.chroot_dir);
+            }
 	  else if (c->first_time)
 	    msg (M_INFO, "NOTE: chroot %s", why_not);
 	}  

Looks good to me, so: ACK

In long term we should think about a proper solution. Notification socket is /run/systemd/notify, so we would have to make that available from within the chroot.

Christian Hesse <[email protected]...> on Fri, 2016/12/09 18:37:

Quoted 68 lines

David Sommerseth <[email protected]...> on Wed, 2016/12/07 03:51:

Quoted 58 lines

Commit c5931897ae8d663e7e introduced support for talking directly to the systemd service manager about the situation for the OpenVPN tunnel. This approach makes a lot of sense and is mostly the proper way to do it. But it was discovered that it breaks OpenVPN configurations using --chroot.

The reason sd_notify() calls fails when using chroot() is that sd_notify() expects to have access to a file as declared in the $NOTIFY_SOCKET environment variable. It is the main systemd instance which is responsible to provide both the environment variable as well as the socket file sd_nodify() should use. When --chroot comes into play, the $NOTIFY_SOCKET file will not be available for OpenVPN any more.

As things are getting close to the 2.4_rc2 release we will not dare to bring a too invasive fix. As well we need some time to discuss an approrpriate solution. So this intermediate fix will only provide a "successful start" message to the systemd service manager right before chroot() happens. This will at least resolve the issue in a safe and non-intrusive way.

Signed-off-by David Sommerseth <[email protected]...>

---
 src/openvpn/init.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

src/openvpn/init.c +3 -1

index 74f1139..e47f0d4 100644
@@ -967,7 +967,27 @@ do_uid_gid_chroot (struct context *c, bool no_delay)
       if (c->options.chroot_dir)
 	{
 	  if (no_delay)
-	    platform_chroot (c->options.chroot_dir);
+            {
+#ifdef ENABLE_SYSTEMD
+              /* If OpenVPN is started by systemd, the OpenVPN process

needs
+ * to provide a preliminary status report to systemd. This
is
+ * needed as $NOTIFY_SOCKET will not be available inside
the

src/openvpn/init.c +7 -0

+               * chroot, which sd_notify()/sd_notifyf() depends on.
+               *
+               * This approach is the simplest and the most non-intrusive
+               * solution right before the 2.4_rc2 release.
+               *
+               * TODO: Consider altnernative solutions - bind mount?
+               * systemd does not grok OpenVPN configuration files, thus

cannot
+ * have a sane way to know if OpenVPN will chroot or not
and to

src/openvpn/init.c +4 -0

+               * which subdirectory it will chroot into.
+               */
+              sd_notifyf(0, "READY=1\n"
+                "STATUS=Entering chroot, most of the init completed

successfully\n"

src/openvpn/init.c do_uid_gid_chroot (struct context *c, bool no_delay) · line 993

Quoted 7 lines

src/openvpn/init.c +4 -0

+                "MAINPID=%lu", (unsigned long) getpid());
+#endif
+              platform_chroot (c->options.chroot_dir);
+            }
 	  else if (c->first_time)
 	    msg (M_INFO, "NOTE: chroot %s", why_not);
 	}  

Looks good to me, so: ACK

Quoted 3 lines

In long term we should think about a proper solution. Notification socket is /run/systemd/notify, so we would have to make that available from within the chroot.

BTW, systemd used to use abstract sockets for notifications, which worked with processes that chroot themselves. This was changed [0] in favor to PrivateNetwork setting. Private network (in terms of systemd) is nothing we can make use of, though. Obviously... :-p

Does it make sense to deny to chroot when running from systemd and instead add some more security features to the systemd unit? Things like ProtectSystem=strict, ProtectHome=true and friends. See systemd.exec(5) [1] for details.

From: Christian Hesse <[email protected]...>

sd_notify() uses a socket to communicate with systemd. Communication fails if the socket is not available within the chroot. So bind mount the socket into the chroot when startet from systemd.

Unsharing namespace and mounting requires extra capability CAP_SYS_ADMIN.

Quoted 6 lines

From: Christian Hesse <[email protected]...>

sd_notify() uses a socket to communicate with systemd. Communication fails if the socket is not available within the chroot. So bind mount the socket into the chroot when startet from systemd.

Quoted 2 lines

Unsharing namespace and mounting requires extra capability CAP_SYS_ADMIN.

I will pick up this one after 2.4.0 has been released. This is a very promising approach. However, I'm not too happy about CAP_SYS_ADMIN though, that grants quite some privileges. Can we look at dropping this capability once we know we won't need it any more? Perhaps when we send READY=1?

Quoted 6 lines

src/openvpn/init.c +5 -0

+              char * chroot_notify = NULL;
+
+              if (sd_notify(0, "READY=0") > 0)
+                {
+                  asprintf(&chroot_notify, "%s/notify",

c->options.chroot_dir);

Here we should use the buffer/string functions, based on the gc_arena implementation. Unfortunately we do not have a direct equivalent to asprintf(). A starting point would be to for example look at the string handling in print_sockaddr_ex() [socket.c:2386] or x_msg_va() [error.c:251] ... there might be better examples too, I'm just not able to remember them now :) .... buffer.[ch] keeps most of these functions.

The reason for this is basically to use the same well tested infrastructure. And with gc_arena, only a single gc_free() is required, regardless of how many buffers you allocate to that arena.

David Sommerseth <[email protected]...> on Sat, 2016/12/10 01:03:

Quoted 7 lines · Christian Hesse

Quoted 6 lines

From: Christian Hesse <[email protected]...>

sd_notify() uses a socket to communicate with systemd. Communication fails if the socket is not available within the chroot. So bind mount the socket into the chroot when startet from systemd.

Quoted 8 lines

Quoted 2 lines

Unsharing namespace and mounting requires extra capability CAP_SYS_ADMIN.

I will pick up this one after 2.4.0 has been released. This is a very promising approach. However, I'm not too happy about CAP_SYS_ADMIN though, that grants quite some privileges. Can we look at dropping this capability once we know we won't need it any more? Perhaps when we send READY=1?

Never tried to drop capabilities... Have to look into that. We do no longer need CAP_SYS_ADMIN after the bind mount. (Or not at all without chrooting.)

Quoted 14 lines

Quoted 6 lines

src/openvpn/init.c +5 -0

+              char * chroot_notify = NULL;
+
+              if (sd_notify(0, "READY=0") > 0)
+                {
+                  asprintf(&chroot_notify, "%s/notify",

c->options.chroot_dir);

Here we should use the buffer/string functions, based on the gc_arena implementation. Unfortunately we do not have a direct equivalent to asprintf(). A starting point would be to for example look at the string handling in print_sockaddr_ex() [socket.c:2386] or x_msg_va() [error.c:251] ... there might be better examples too, I'm just not able to remember them now :) .... buffer.[ch] keeps most of these functions.

Quoted 3 lines

The reason for this is basically to use the same well tested infrastructure. And with gc_arena, only a single gc_free() is required, regardless of how many buffers you allocate to that arena.

I do not like this myself. The patch is just a proof of concept... So this should be polished before committing. ;)

Thanks for the hints, I will have a look.

src/openvpn/init.c +5 -0

+              int fd;
+              char * chroot_notify = NULL;
+
+              if (sd_notify(0, "READY=0") > 0)
+                {

Hi,

Quoted 5 lines · Christian Hesse

src/openvpn/init.c +5 -0

+              int fd;
+              char * chroot_notify = NULL;
+
+              if (sd_notify(0, "READY=0") > 0)
+                {

src/openvpn/init.c do_uid_gid_chroot (struct context *c, bool no_delay) · line 989

Quoted 10 lines

src/openvpn/init.c +10 -0

+                  asprintf(&chroot_notify, "%s/notify", c->options.chroot_dir);
+
+                  if (unshare(CLONE_NEWNS) != 0)
+                    msg (M_ERR, "unshare failed");
+                  if ((fd = open(chroot_notify, O_WRONLY | O_CREAT | O_TRUNC, 0644)) < 0)
+                    msg (M_ERR, "touch failed");
+                  close(fd);
+                  if (mount(getenv("NOTIFY_SOCKET"), chroot_notify, NULL, MS_BIND, NULL) != 0)
+                    msg (M_ERR, "bind mounting notification socket failed");
+

This is WAY over the top of what should go into OpenVPN code. Really.

NAK on approach, NAK on code.

Is there a way make sd_notify() behave like syslog()? That is, you call something like "openlog()" which will acquire the necessary file descriptor and then you can afterwards chroot() to your heart's content and do not need access to the actual socket file anymore (because openlog() will keep it around for syslog() to use).

If sd_notify() cannot be taught to do that, either do what David proposed (disable chroot if running under systemd), or at least move that code out of init.c into something like platform.c.