[Openvpn-devel] Add --up-pre with the same functionality as --down-pre

Message ID 84a29b8afab02733064c2cf715ae0b8c.squirrel@webmail.bi.invoca.ch
State Changes Requested
Headers show
Series
  • [Openvpn-devel] Add --up-pre with the same functionality as --down-pre
Related show

Commit Message

Simon Matter Nov. 22, 2017, 4:58 p.m.
Hi,

In our situation we have the requirement to run scripts before tun/tap is
opened, not after. While this could be hacked into the init script, the
proper way seems to add it to openvpn as --up-pre option. That's
independent from any init scripts / systemd service file and works the
same way as --down-pre, only for the up status.

My initial feature wish, posted 5 years ago, was turned down as won't fix:
https://community.openvpn.net/openvpn/ticket/284

But there are people who wish it and they have good reasons to wish it.
Just yesterday someone asked again:
https://community.openvpn.net/openvpn/ticket/284#comment:10

Without going into much details just one thing why --up + --up-pre is
better than hacking around outside of openvpn: the command called with
--up also gets additional run time information from openvpn by parameters
and environmental variables. You don't get all those information when
calling anything from outside of openvpn before openvpn actually starts.

If you feel there are good reasons to still refuse this patch, please let
me know.

Regards,
Simon
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Comments

Arne Schwabe Oct. 1, 2020, 12:10 p.m. | #1
Am 22.11.17 um 17:58 schrieb Simon Matter:
> Hi,
> 
> In our situation we have the requirement to run scripts before tun/tap is
> opened, not after. While this could be hacked into the init script, the
> proper way seems to add it to openvpn as --up-pre option. That's
> independent from any init scripts / systemd service file and works the
> same way as --down-pre, only for the up status.
> 
> My initial feature wish, posted 5 years ago, was turned down as won't fix:
> https://community.openvpn.net/openvpn/ticket/284
> 
> But there are people who wish it and they have good reasons to wish it.
> Just yesterday someone asked again:
> https://community.openvpn.net/openvpn/ticket/284#comment:10
> 
> Without going into much details

This patch currently misses a commit message anyway but a good commit
should explain why this change is a good one.

> just one thing why --up + --up-pre is
> better than hacking around outside of openvpn: the command called with
> --up also gets additional run time information from openvpn by parameters
> and environmental variables. You don't get all those information when
> calling anything from outside of openvpn before openvpn actually starts.
> 
> If you feel there are good reasons to still refuse this patch, please let
> me know.

I am just looking at this patch since it is still in the review queue.

- Missing documentation.
- pre-up flag is wrong in terms of scripts. If we add this, it needs to
be a different script because otherwise you will break use cases that
also need the --up script.

Also having down and down-pre but then only not also up/up-pre but a up
with flag breaks the symmetry and is confusing.

Arne
Simon Matter Oct. 1, 2020, 3:03 p.m. | #2
Hi Arne,

> Am 22.11.17 um 17:58 schrieb Simon Matter:
>> Hi,
>>
>> In our situation we have the requirement to run scripts before tun/tap
>> is
>> opened, not after. While this could be hacked into the init script, the
>> proper way seems to add it to openvpn as --up-pre option. That's
>> independent from any init scripts / systemd service file and works the
>> same way as --down-pre, only for the up status.
>>
>> My initial feature wish, posted 5 years ago, was turned down as won't
>> fix:
>> https://community.openvpn.net/openvpn/ticket/284
>>
>> But there are people who wish it and they have good reasons to wish it.
>> Just yesterday someone asked again:
>> https://community.openvpn.net/openvpn/ticket/284#comment:10
>>
>> Without going into much details
>
> This patch currently misses a commit message anyway but a good commit
> should explain why this change is a good one.
>
>> just one thing why --up + --up-pre is
>> better than hacking around outside of openvpn: the command called with
>> --up also gets additional run time information from openvpn by
>> parameters
>> and environmental variables. You don't get all those information when
>> calling anything from outside of openvpn before openvpn actually starts.
>>
>> If you feel there are good reasons to still refuse this patch, please
>> let
>> me know.
>
> I am just looking at this patch since it is still in the review queue.
>
> - Missing documentation.
> - pre-up flag is wrong in terms of scripts. If we add this, it needs to
> be a different script because otherwise you will break use cases that
> also need the --up script.
>
> Also having down and down-pre but then only not also up/up-pre but a up
> with flag breaks the symmetry and is confusing.

One of us is confused here.

What you say is missing in my patch is not missing at all. It simply
brings both, the "up" and the "down" functionality to the same level!

I modeled the --up-pre option EXACTLY the same way as the EXISTING
--down-pre option. It works the same way now for "up" as it is working
since many years for "down".

The existing man entry for --down-pre says:

--down-pre
  Call ``--down`` cmd/script before, rather than after, TUN/TAP close.

The patched man entry for --up-pre says:

--down-pre
  Call --down cmd/script before, rather than after, TUN/TAP close.

Why should --down-pre use another script while the existing code for
--up-pre doesn't do?


I really can't understand why this small patch was refused for years and I
still feel nobody ever really looked at it.

I know we could fiddle something with systemd now on Linux to get a
similar functionality but it's still not the same. And if you have mixed
environments with other Unices then it's really a mess.

So, it's a non intrusive, small patch which fixes the symmetry between
"up" and "down" path in openvpn.

Regards,
Simon
David Sommerseth Oct. 1, 2020, 3:11 p.m. | #3
On 01/10/2020 14:10, Arne Schwabe wrote:
> Am 22.11.17 um 17:58 schrieb Simon Matter:
>> Hi,
>>
>> In our situation we have the requirement to run scripts before tun/tap is
>> opened, not after. While this could be hacked into the init script, the
>> proper way seems to add it to openvpn as --up-pre option. That's
>> independent from any init scripts / systemd service file and works the
>> same way as --down-pre, only for the up status.
>>
>> My initial feature wish, posted 5 years ago, was turned down as won't fix:
>> https://community.openvpn.net/openvpn/ticket/284
>>
>> But there are people who wish it and they have good reasons to wish it.
>> Just yesterday someone asked again:
>> https://community.openvpn.net/openvpn/ticket/284#comment:10
>>
>> Without going into much details
> 
> This patch currently misses a commit message anyway but a good commit
> should explain why this change is a good one.
> 
>> just one thing why --up + --up-pre is
>> better than hacking around outside of openvpn: the command called with
>> --up also gets additional run time information from openvpn by parameters
>> and environmental variables. You don't get all those information when
>> calling anything from outside of openvpn before openvpn actually starts.
>>
>> If you feel there are good reasons to still refuse this patch, please let
>> me know.
> 
> I am just looking at this patch since it is still in the review queue.
> 
> - Missing documentation.
> - pre-up flag is wrong in terms of scripts. If we add this, it needs to
> be a different script because otherwise you will break use cases that
> also need the --up script.
> 
> Also having down and down-pre but then only not also up/up-pre but a up
> with flag breaks the symmetry and is confusing.
This patch has been lingering here on the list since 2017.  And I still
haven't changed my opinion about it, as I lack to see a proper use-case (or
"user story", which is the appropriate term for the 2020s).

My stance is pretty well covered in the ticket [1], and the only potential use
case which was provided does have, in my opinion, a better alternative by
using --management and --management-hold.

<https://community.openvpn.net/openvpn/ticket/284#comment:15>

So consider this a NAK from my side.
David Sommerseth Oct. 1, 2020, 3:45 p.m. | #4
On 01/10/2020 17:03, Simon Matter wrote:
> I really can't understand why this small patch was refused for years and I
> still feel nobody ever really looked at it.

Perhaps this also an indication of the corner case this patch is covering?

This patch started 7 years ago.  There has been 2 other users being supportive
in the Trac ticket, where at least one of them do have another functional
alternative (--management with --management-hold).

From what I recall from the last review years ago, the behavior was also not
well defined in restart scenarios (--up-restart) - where the script might be
run with different privileges, the --chroot might also change things.  Which
makes this patch very fragile for users.

All of these issues are avoided with the --management and --management-hold.

And if you still require more flexibility when starting client configurations,
you should rather consider OpenVPN 3 Linux - which can be much more fine
grained controlled via an API.  OpenVPN 3 Linux can also be used by
unprivileged users out-of-the-box, resulting in better security for what is
being executed and when it is being executed.

There are several examples in Python, but any language with D-Bus support will
work:
<https://github.com/OpenVPN/openvpn3-linux/tree/master/src/tests/python>
<https://github.com/OpenVPN/openvpn3-linux/tree/master/src/python>
Simon Matter Oct. 1, 2020, 4:36 p.m. | #5
> On 01/10/2020 17:03, Simon Matter wrote:
>> I really can't understand why this small patch was refused for years and
>> I
>> still feel nobody ever really looked at it.
>
> Perhaps this also an indication of the corner case this patch is covering?
>
> This patch started 7 years ago.  There has been 2 other users being
> supportive
> in the Trac ticket, where at least one of them do have another functional
> alternative (--management with --management-hold).
>
> From what I recall from the last review years ago, the behavior was also
> not
> well defined in restart scenarios (--up-restart) - where the script might
> be
> run with different privileges, the --chroot might also change things.
> Which
> makes this patch very fragile for users.
>
> All of these issues are avoided with the --management and
> --management-hold.

How do all these issues affect --up-pre but not the existing --down-pre?
Why was --down-pre never removed over all the years if it makes things so
fragile for users?

>
> And if you still require more flexibility when starting client
> configurations,
> you should rather consider OpenVPN 3 Linux - which can be much more fine
> grained controlled via an API.  OpenVPN 3 Linux can also be used by
> unprivileged users out-of-the-box, resulting in better security for what
> is
> being executed and when it is being executed.

OpenVPN 3 Linux is not an option here as it is limited to Linux.

Regards,
Simon
David Sommerseth Oct. 1, 2020, 5:13 p.m. | #6
On 01/10/2020 18:36, Simon Matter wrote:
>> From what I recall from the last review years ago, the behavior was also
>> not
>> well defined in restart scenarios (--up-restart) - where the script might
>> be
>> run with different privileges, the --chroot might also change things.
>> Which
>> makes this patch very fragile for users.
>>
>> All of these issues are avoided with the --management and
>> --management-hold.
>
> How do all these issues affect --up-pre but not the existing --down-pre?
> Why was --down-pre never removed over all the years if it makes things so
> fragile for users?

Several reasons.  We don't want to break backwards compatibility.  So removing
an option which has existed since 2004 is not acceptable unless it is really
security sensitive (see the discussion on this list regarding another patch
where I suggested, to remove --udp-mtu - which was also rejected).

Secondly, --down-pre has a deterministic behavior.  If you use --chroot and/or
--user/--group, the --down script is run under the same conditions whether you
use --up-restart or not.

And, since we still have not heard any convincing arguments why --management
with --management-hold cannot work for your use case, nothing has changed
since the Trac discussions years ago.  We don't want to easily add a feature
only 3 people have requested during a time span of 7 years, where one of them
already confirmed the management approach worked [1].

<https://community.openvpn.net/openvpn/ticket/284#comment:17>

Adding new options and features means we need to commit to support them for
quite a long time; regardless of the patch complexity.  So a new feature or
option really needs to have a reasonable amount of users and solves a problem
which cannot be solved through other reasonable ways.

Patch

diff -Naur openvpn-2.4.0.orig/doc/openvpn.8 openvpn-2.4.0/doc/openvpn.8
--- openvpn-2.4.0.orig/doc/openvpn.8	2016-12-26 14:01:34.000000000 +0100
+++ openvpn-2.4.0/doc/openvpn.8	2016-12-30 11:45:16.000000000 +0100
@@ -1845,6 +1845,12 @@ 
 .B route add \-net 10.0.0.0 netmask 255.255.255.0 gw $5
 .\"*********************************************************
 .TP
+.B \-\-up\-pre
+Call
+.B \-\-up
+cmd/script before, rather than after, TUN/TAP open.
+.\"*********************************************************
+.TP
 .B \-\-up\-delay
 Delay TUN/TAP open and possible
 .B \-\-up
diff -Naur openvpn-2.4.0.orig/src/openvpn/init.c openvpn-2.4.0/src/openvpn/init.c
--- openvpn-2.4.0.orig/src/openvpn/init.c	2016-12-26 12:51:00.000000000 +0100
+++ openvpn-2.4.0/src/openvpn/init.c	2016-12-30 12:05:15.000000000 +0100
@@ -1573,6 +1573,27 @@ 
     }
 #endif
 
+    /* actually run the up script based on --up-pre flag */
+    if (c->options.up_pre)
+    {
+        run_up_down (c->options.up_script,
+                     c->plugins,
+                     OPENVPN_PLUGIN_UP,
+                     "[unknown-dev]",
+#ifdef _WIN32
+                     TUN_ADAPTER_INDEX_INVALID,
+#endif
+                     dev_type_string (c->options.dev, c->options.dev_type),
+                     TUN_MTU_SIZE (&c->c2.frame),
+                     EXPANDED_SIZE (&c->c2.frame),
+                     NULL,
+                     NULL,
+                     "init",
+                     NULL,
+                     "up",
+                     c->c2.es);
+    }
+
     /* initialize (but do not open) tun/tap object */
     do_init_tun(c);
 
@@ -1639,23 +1660,26 @@ 
         do_ifconfig(c->c1.tuntap, c->c1.tuntap->actual_name, TUN_MTU_SIZE(&c->c2.frame), c->c2.es);
     }
 
-    /* run the up script */
-    run_up_down(c->options.up_script,
-                c->plugins,
-                OPENVPN_PLUGIN_UP,
-                c->c1.tuntap->actual_name,
+    /* actually run the up script based on --up-pre flag */
+    if (!c->options.up_pre)
+    {
+        run_up_down(c->options.up_script,
+                    c->plugins,
+                    OPENVPN_PLUGIN_UP,
+                    c->c1.tuntap->actual_name,
 #ifdef _WIN32
-                c->c1.tuntap->adapter_index,
+                    c->c1.tuntap->adapter_index,
 #endif
-                dev_type_string(c->options.dev, c->options.dev_type),
-                TUN_MTU_SIZE(&c->c2.frame),
-                EXPANDED_SIZE(&c->c2.frame),
-                print_in_addr_t(c->c1.tuntap->local, IA_EMPTY_IF_UNDEF, &gc),
-                print_in_addr_t(c->c1.tuntap->remote_netmask, IA_EMPTY_IF_UNDEF, &gc),
-                "init",
-                NULL,
-                "up",
-                c->c2.es);
+                    dev_type_string(c->options.dev, c->options.dev_type),
+                    TUN_MTU_SIZE(&c->c2.frame),
+                    EXPANDED_SIZE(&c->c2.frame),
+                    print_in_addr_t(c->c1.tuntap->local, IA_EMPTY_IF_UNDEF, &gc),
+                    print_in_addr_t(c->c1.tuntap->remote_netmask, IA_EMPTY_IF_UNDEF, &gc),
+                    "init",
+                    NULL,
+                    "up",
+                    c->c2.es);
+    }
 
 #if defined(_WIN32)
     if (c->options.block_outside_dns)
diff -Naur openvpn-2.4.0.orig/src/openvpn/options.c openvpn-2.4.0/src/openvpn/options.c
--- openvpn-2.4.0.orig/src/openvpn/options.c	2016-12-26 12:51:00.000000000 +0100
+++ openvpn-2.4.0/src/openvpn/options.c	2016-12-30 12:09:19.000000000 +0100
@@ -301,6 +301,7 @@ 
     "                  Execute as: cmd tun/tap-dev tun-mtu link-mtu \\\n"
     "                              ifconfig-local-ip ifconfig-remote-ip\n"
     "                  (pre --user or --group UID/GID change)\n"
+    "--up-pre        : Run --up command before TUN/TAP open.\n"
     "--up-delay      : Delay tun/tap open and possible --up script execution\n"
     "                  until after TCP/UDP connection establishment with peer.\n"
     "--down cmd      : Run command cmd after tun device close.\n"
@@ -1623,6 +1624,7 @@ 
     SHOW_STR(up_script);
     SHOW_STR(down_script);
     SHOW_BOOL(down_pre);
+    SHOW_BOOL(up_pre);
     SHOW_BOOL(up_restart);
     SHOW_BOOL(up_delay);
     SHOW_BOOL(daemon);
@@ -5530,6 +5532,11 @@ 
         VERIFY_PERMISSION(OPT_P_GENERAL);
         options->down_pre = true;
     }
+    else if (streq(p[0], "up-pre") && !p[1])
+    {
+        VERIFY_PERMISSION(OPT_P_GENERAL);
+        options->up_pre = true;
+    }
     else if (streq(p[0], "up-delay") && !p[1])
     {
         VERIFY_PERMISSION(OPT_P_GENERAL);
diff -Naur openvpn-2.4.0.orig/src/openvpn/options.h openvpn-2.4.0/src/openvpn/options.h
--- openvpn-2.4.0.orig/src/openvpn/options.h	2016-12-26 12:51:00.000000000 +0100
+++ openvpn-2.4.0/src/openvpn/options.h	2016-12-30 12:09:47.000000000 +0100
@@ -285,6 +285,7 @@ 
     const char *down_script;
     bool user_script_used;
     bool down_pre;
+    bool up_pre;
     bool up_delay;
     bool up_restart;
     bool daemon;