Re: [Openvpn-devel] [PATCH 1/1] bind mount systemd notification socket into chroot

2016-12-10 Thread David Sommerseth 
On 10/12/16 13:08, Christian Hesse wrote:
> David Sommerseth  on Sat, 2016/12/10 01:03:
>> On 10/12/16 00:19, Christian Hesse wrote:
>>> From: Christian Hesse 
>>>
>>> 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.  
>>
>> 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.)

Sounds reasonable.  Since we need to take CAP_SYS_ADMIN out of the unit
file regardless, we need to drop the CAP_SYS_ADMIN capability inside
OpenVPN anyhow.  Just need to figure out the best place for it.

As a pointer for managing capabilities, have a look at the capsetp(3)
man page.

>>> +  char * chroot_notify = NULL;
>>> +
>>> +  if (sd_notify(0, "READY=0") > 0)
>>> +{
>>> +  asprintf(_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.
> 
> 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.

It's a great PoC!  And good to see that we might find a solution for
this.  Will look forward to dig deeper into this in the coming weeks too.


-- 
kind regards,

David Sommerseth
OpenVPN Technologies, Inc




signature.asc
Description: OpenPGP digital signature
--
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today.http://sdm.link/xeonphi___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH 1/1] bind mount systemd notification socket into chroot

2016-12-10 Thread David Sommerseth 
On 10/12/16 13:29, Gert Doering wrote:
> Hi,
> 
> On Sat, Dec 10, 2016 at 12:19:07AM +0100, Christian Hesse wrote:
>> +  int fd;
>> +  char * chroot_notify = NULL;
>> +
>> +  if (sd_notify(0, "READY=0") > 0)
>> +{
>> +  asprintf(_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).

First of all, bind mounts are fairly common in Linux these days -
especially when you start to try to chroot/containerize (including
docker)/"jail" and otherwise isolate applications.  Chroots in general
are considered a bit too easy to escape from, unless privileges are
dropped at the right point.  Namespaces and other container techniques
are often somewhat better, but still does not yet provide the full
isolation many expects.  In this context bind mounts are often
considered simpler and to some degree safer than a full mount of /dev,
/proc, /sys, /run and so on.

Unfortunately, there exists no "openlog()" equivalent to my knowledge
for sd_notify() and other functions depending on the /run/systemd
directory.  But it's a good idea, and in fact such a "prepare for
chroot" in libsystemd could probably do such a bind mount for the
caller.  I have a good contact with one of the upstream systemd
developers and will discuss this with him ... so with time, we might
have something better.

Well, there is one feature systemd guys might throw at us instantly when
we open this topic ... PrivateNetwork=true in the unit file.  But that
is useless for OpenVPN as then we will only have access to the lo
network device, all other network devices are unavailable.

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

That's a fair argument.  As long as thelibsystemd shipped in RHEL7 does
not, to our knowledge, carry such a feature which we need; implementing
this functionality inside platform.c is reasonable.

What I am wondering then is if we should implement platform_chroot()
which is called from init.c.  Where it will just call chroot() on
non-systemd and non-Windows systems.  When systemd is enabled and
detected detected it will prepare the bind mount, drop capabilities and
then call chroot().

Is that a reasonable approach?  Or would you prefer to have just the
bind mount stuff isolated into platform.c?


-- 
kind regards,

David Sommerseth
OpenVPN Technologies, Inc




signature.asc
Description: OpenPGP digital signature
--
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today.http://sdm.link/xeonphi___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH 1/1] bind mount systemd notification socket into chroot

2016-12-10 Thread Gert Doering 
Hi,

On Sat, Dec 10, 2016 at 12:19:07AM +0100, Christian Hesse wrote:
> +  int fd;
> +  char * chroot_notify = NULL;
> +
> +  if (sd_notify(0, "READY=0") > 0)
> +{
> +  asprintf(_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.

gert
-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de


signature.asc
Description: PGP signature
--
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today.http://sdm.link/xeonphi___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH 1/1] bind mount systemd notification socket into chroot

2016-12-10 Thread Christian Hesse 
David Sommerseth  on Sat, 2016/12/10 01:03:
> On 10/12/16 00:19, Christian Hesse wrote:
> > From: Christian Hesse 
> > 
> > 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.  
> 
> 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.)

> > +  char * chroot_notify = NULL;
> > +
> > +  if (sd_notify(0, "READY=0") > 0)
> > +{
> > +  asprintf(_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.

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.
-- 
main(a){char*c=/*Schoene Gruesse */"B?IJj;MEH"
"CX:;",b;for(a/*Best regards my address:*/=0;b=c[a++];)
putchar(b-1/(/*Chriscc -ox -xc - && ./x*/b/42*2-3)*42);}


pgpdFAvSbXJm8.pgp
Description: OpenPGP digital signature
--
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today.http://sdm.link/xeonphi___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH 1/1] bind mount systemd notification socket into chroot

2016-12-09 Thread David Sommerseth 
On 10/12/16 00:19, Christian Hesse wrote:
> From: Christian Hesse 
> 
> 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.

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?

> +  char * chroot_notify = NULL;
> +
> +  if (sd_notify(0, "READY=0") > 0)
> +{
> +  asprintf(_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.


-- 
kind regards,

David Sommerseth
OpenVPN Technologies, Inc




signature.asc
Description: OpenPGP digital signature
--
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today.http://sdm.link/xeonphi___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH 1/1] bind mount systemd notification socket into chroot

2016-12-09 Thread Christian Hesse 
From: Christian Hesse 

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.

Signed-off-by: Christian Hesse 
---
 distro/systemd/openvpn-client@.service |  2 +-
 distro/systemd/openvpn-server@.service |  2 +-
 src/openvpn/init.c | 37 +++---
 3 files changed, 23 insertions(+), 18 deletions(-)

diff --git a/distro/systemd/openvpn-client@.service 
b/distro/systemd/openvpn-client@.service
index 5618af3..3ebd1da 100644
--- a/distro/systemd/openvpn-client@.service
+++ b/distro/systemd/openvpn-client@.service
@@ -13,7 +13,7 @@ RuntimeDirectory=openvpn-client
 RuntimeDirectoryMode=0710
 WorkingDirectory=/etc/openvpn/client
 ExecStart=/usr/sbin/openvpn --suppress-timestamps --nobind --config %i.conf
-CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_RAW CAP_SETGID 
CAP_SETUID CAP_SYS_CHROOT CAP_DAC_OVERRIDE
+CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_RAW CAP_SETGID 
CAP_SETUID CAP_SYS_ADMIN CAP_SYS_CHROOT CAP_DAC_OVERRIDE
 LimitNPROC=10
 DeviceAllow=/dev/null rw
 DeviceAllow=/dev/net/tun rw
diff --git a/distro/systemd/openvpn-server@.service 
b/distro/systemd/openvpn-server@.service
index b9b4dba..bcf1bcf 100644
--- a/distro/systemd/openvpn-server@.service
+++ b/distro/systemd/openvpn-server@.service
@@ -13,7 +13,7 @@ RuntimeDirectory=openvpn-server
 RuntimeDirectoryMode=0710
 WorkingDirectory=/etc/openvpn/server
 ExecStart=/usr/sbin/openvpn --status %t/openvpn-server/status-%i.log 
--status-version 2 --suppress-timestamps --config %i.conf
-CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_BIND_SERVICE 
CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SYS_CHROOT CAP_DAC_OVERRIDE
+CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_BIND_SERVICE 
CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SYS_ADMIN CAP_SYS_CHROOT CAP_DAC_OVERRIDE
 LimitNPROC=10
 DeviceAllow=/dev/null rw
 DeviceAllow=/dev/net/tun rw
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 26b236d..81bf9ab 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -31,6 +31,9 @@
 #include "syshead.h"
 
 #ifdef ENABLE_SYSTEMD
+#include 
+#include 
+#include 
 #include 
 #endif
 
@@ -969,22 +972,24 @@ do_uid_gid_chroot (struct context *c, bool no_delay)
  if (no_delay)
 {
 #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
-   * 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
-   * which subdirectory it will chroot into.
-   */
-  sd_notifyf(0, "READY=1\n"
-"STATUS=Entering chroot, most of the init completed 
successfully\n"
-"MAINPID=%lu", (unsigned long) getpid());
+  int fd;
+  char * chroot_notify = NULL;
+
+  if (sd_notify(0, "READY=0") > 0)
+{
+  asprintf(_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");
+
+  setenv("NOTIFY_SOCKET", "/notify", 1);
+  free(chroot_notify);
+}
 #endif
   platform_chroot (c->options.chroot_dir);
 }
-- 
2.10.2


--
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today.http://sdm.link/xeonphi
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel