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