fix timeout in non-TLS mode with systemd
PATCH 1/1 fix timeout in non-TLS mode with systemd
From: Christian Hesse <[email protected]...>
In non-TLS configuration we wait for the remote peer to connect before issuing "Initialization Sequence Completed". So prevent to time out by telling systemd service manager we are ready for now. Status will be "Non-TLS mode, ready for now. Waiting for peer..." and changes once the remote peer connects.
This fixes #801 (static key tunnels impossible to start via systemd)
--- src/openvpn/openvpn.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
src/openvpn/openvpn.c +19 -0
@@ -30,6 +30,10 @@#include "syshead.h" +#ifdef ENABLE_SYSTEMD +#include <systemd/sd-daemon.h> +#endif +#include "init.h" #include "forward.h" #include "multi.h"
@@ -73,6 +77,21 @@ tunnel_point_to_point(struct context *c)return ; } +#ifdef ENABLE_SYSTEMD +/* In non-TLS configuration we wait for the remote peer to connect +* before issuing "Initialization Sequence Completed". So prevent to +* time out by telling systemd service manager we are ready for now. +* Status will be "Non-TLS mode, ready for now. Waiting for peer..." +* and changes once the remote peer connects. */ +if (c->options.tls_client == false + && c->options.tls_server == false) + { +sd_notifyf (0 ,"READY=1\n" +"STATUS=Non-TLS mode, ready for now. Waiting for peer...\n" +"MAINPID=%lu" , (unsigned long )getpid ()); + } +#endif +/* main event loop */ while (true) {
Signature · 1 line
2.11.0
Hi,
Quoted 8 lines · Christian Hesse
src/openvpn/openvpn.c +4
-0@@ -73,6 +77,21 @@ tunnel_point_to_point(struct context *c)return ; } +#ifdef ENABLE_SYSTEMD +/* In non-TLS configuration we wait for the remote peer to connect +* before issuing "Initialization Sequence Completed". So prevent to +* time out by telling systemd service manager we are ready for now.
src/openvpn/openvpn.c tunnel_point_to_point(struct context *c) · line 93
Quoted 10 lines
src/openvpn/openvpn.c +10
-0+* Status will be "Non-TLS mode, ready for now. Waiting for peer..." +* and changes once the remote peer connects. */ +if (c->options.tls_client == false + && c->options.tls_server == false) + { +sd_notifyf (0 ,"READY=1\n" +"STATUS=Non-TLS mode, ready for now. Waiting for peer...\n" +"MAINPID=%lu" , (unsigned long )getpid ()); + } +#endif
We definitely need a better approach than "litter ENABLE_SYSTEMD all over the code".
(Also, this is the wrong check anyway. p2p mode can go along with TLS just fine - what you need to check for is --server or --client, which is something else than --tls-server / --tls-client)
gert
Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany [email protected]...
fax: +49-89-35655025 [email protected]...Gert Doering <[email protected]...> on Wed, 2016/12/28 19:57:
Quoted 17 lines
Hi,
Quoted 14 lines · Christian Hesse
src/openvpn/openvpn.c +10
-0@@ -73,6 +77,21 @@ tunnel_point_to_point(struct context *c)return ; } +#ifdef ENABLE_SYSTEMD +/* In non-TLS configuration we wait for the remote peer to connect +* before issuing "Initialization Sequence Completed". So prevent to +* time out by telling systemd service manager we are ready for now. +* Status will be "Non-TLS mode, ready for now. Waiting for peer..." +* and changes once the remote peer connects. */ +if (c->options.tls_client == false + && c->options.tls_server == false) + { + sd_notifyf(0, "READY=1\n"
Quoted 8 lines
src/openvpn/openvpn.c tunnel_point_to_point(struct context *c) · line 93
Quoted 5 lines
(quoted diff) +1
-0+ "STATUS=Non-TLS mode, ready for now. Waiting forpeer...\n"
src/openvpn/openvpn.c +3
-0+ "MAINPID=%lu", (unsigned long) getpid()); + } +#endif We definitely need a better approach than "litter
ENABLE_SYSTEMDall over the code".
Well, openvpn supports a number of modes of operation... Some of these have other requirements than others.
In general we have three options:
1. Use Type=forking in unit files. We decided not to go this way.
2. Use Type=notify and tell systemd we are ready about when we would have forked. This works as well as Type=forking does, but has some unhandled corner cases.
3. Use Type=notify and "litter ENABLE_SYSTEMD all over the code". This provides the best error handling.
(There is Type=simple, but that is just fire-and-forget - could not be worse.)
IMHO we should go with option three. Given the number of #ifdefs all over the code - is this really an issue? My patch increases the number of sd_notify() calls from four to five.
For informational purpose we could add even more calls. That would allow to set intermediate status message, something like: "Up and running, currently serving 25 client connections."
Quoted 3 lines
(Also, this is the wrong check anyway. p2p mode can go along with TLS just fine - what you need to check for is --server or --client, which is something else than --tls-server / --tls-client)
No, the check is correct.
In server move openvpn reports ready when it finally is ready to handle incoming connections. That is fine.
In TLS client mode openvpn reports ready when it connected to a server successfully. It fails if it can not connect. That is the desired behavior. So that is fine as well.
The patch handles non-TLS P2P mode: Two nodes have a shared secret (or connect unencrypted in pure tunnel mode) and connect to each other. The culprit: There is no node that is ready all the time. As we do not want to bail out before the other is connects we have to report ready after initialization but before the remote connects. This really is about non-TLS, not client/server. No?
Signature · 3 lines
main(a){char*c=/* Schoene Gruesse */"B?IJj;MEH"
"CX:;",b;for(a/* Best regards my address: */=0;b=c[a++];)
putchar(b-1/(/* Chris cc -ox -xc - && ./x */b/42*2-3)*42);}Hi,
Quoted 5 lines · Christian Hesse
Quoted 2 lines
We definitely need a better approach than "litter
ENABLE_SYSTEMDall over the code".Well, openvpn supports a number of modes of operation... Some of these have other requirements than others.
This I understand quite well :-) (look at the discussions surrounding NCP and poor man's NCP, and why that ended up patch version 7 or so)
[..]
Quoted 3 lines
IMHO we should go with option three. Given the number of #ifdefs all over the code - is this really an issue? My patch increases the number of
sd_notify()calls from four to five.
Especially given the high amount of #ifdef we already have, every additional one is "one too many" - doubly so for platform-specific stuff in otherwise platform-independent code like init.c
While technically not "platform", nothing else but a subset of Linux distributions use systemd today, so I consider this to be in the same league as #ifdef TARGET_FREEBSD or #if _WIN32, which both should not
appear in generic high-level code. tun.c, route.c, platform.c are
unavoidably bad... but this should serve as a bad example, not as "match this!" target :-)
[..]
Quoted 3 lines
For informational purpose we could add even more calls. That would allow to set intermediate status message, something like: "Up and running, currently serving 25 client connections."
True, and I think that is a useful feature - but it needs an abstraction layer somewhere in between. Adding #ifdef ENABLE_SYSTEMD all across the code is something that is not a good approach for long-term maintainability, and to support *other* interfaces to status reporting - like dbus, or whatever interesting APIs other operating operating systems come up with one day .
We already update our status file regularily, so maybe this is a reasonable point in the code to abstract out something like "platform_notify_status()" (or whatever good name you can come up with), which could then go to, like, "platform-notify.c", and have all the #ifdef ENABLE_SYSTEMD that are needed.
[..]
Quoted 8 lines
Quoted 3 lines
(Also, this is the wrong check anyway. p2p mode can go along with TLS just fine - what you need to check for is --server or --client, which is something else than --tls-server / --tls-client)
No, the check is correct.
In server move openvpn reports ready when it finally is ready to handle incoming connections. That is fine.
"server" or "TLS server"? These are not the same :-) - you can have p2p mode with --tls-server.
But I'll accept that you're likely right there, and --tls-server reports "ready" when it's set up and ready to go, not having to wait for a packet from the tls-client - this stuff has bitten us in t_client.rc recently (p2p startup not reporting "ready!" and t_client.rc then aborting the test run).
gert
Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany [email protected]...
fax: +49-89-35655025 [email protected]...Quoted 1 line · Gert Doering
nothing else but a subset of Linux distributions use systemd today,
If including the "millions" of various Linux distributions on DistroWatch, you might very well be right.
But a far better measuring point would be which Linux distributions the majority of Linux users do use. And in my experience, the list which is gathered on wikipedia [1] covers what the vast majority of Linux users installs. At least the majority of users I have met on various conferences over the last few years.
[1] <https://en.wikipedia.org/wiki/Systemd#Adoption_and_reception>
Signature · 4 lines
kind regards, David Sommerseth OpenVPN Technologies, Inc
Hi,
Quoted 6 lines · David Sommerseth
Quoted 1 line · Gert Doering
nothing else but a subset of Linux distributions use systemd today,
If including the "millions" of various Linux distributions on DistroWatch, you might very well be right.
Quoted 7 lines
But a far better measuring point would be which Linux distributions the majority of Linux users do use. And in my experience, the list which is gathered on wikipedia [1] covers what the vast majority of Linux users installs. At least the majority of users I have met on various conferences over the last few years.
[1] <https://en.wikipedia.org/wiki/Systemd#Adoption_and_reception>
My point was not whether it's "many" or "few" Linux distributions, but it's "a (large) subset of Linux-only". Even if it reaches 99% one day, it's still "Linux", which makes it a more of a platform-dependent thing for me, and not a "feature", when regarding "where do we want to have these #ifdef".
gert
Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany [email protected]...
fax: +49-89-35655025 [email protected]...Hi,
Quoted 4 lines · Christian Hesse
For informational purpose we could add even more calls. That would allow to set intermediate status message, something like: "Up and running, currently serving 25 client connections."
If the daemon has to send specially crafted messages to systemd there is no end to it. Arguably, we should also register as an event provider and send event logs to Windows and whatever the equivalent be on OS X etc...
A cleaner approach would be to leverage on the existing management interface and use standalone modules to do a translation between the management and systemd/launchd//whatever. We already have the status and progress info sent to the management interface. Just as windows GUI uses the management interface we could somehow have an external connector from mgmt to systemd and keep such systemd specific stuff out of openvpn.
Selva
P.S.
I am not a fan of systemd so the above is partly colored by my dislike for it.. (it has done several extremely bad things to me in the past including uninstalling zfs modules on update on a system which had zfs on root and no rescue disk.. well partly my fault..)
PATCH 1/2 move systemd specific code to platform.c
In reply to Gert DoeringFrom: Christian Hesse <[email protected]...>
We have voices that do not want to "litter ENABLE_SYSTEMD all over the code". So move the systemd specific bits to platform_notify() in platform.c.
--- src/openvpn/init.c | 23 +++++------------------ src/openvpn/platform.c | 13 +++++++++++++ src/openvpn/platform.h | 2 ++ 3 files changed, 20 insertions(+), 18 deletions(-)
src/openvpn/init.c +5 -18
@@ -30,10 +30,6 @@#include "syshead.h" -#ifdef ENABLE_SYSTEMD -#include <systemd/sd-daemon.h> -#endif -#include "win32.h" #include "init.h" #include "sig.h"
@@ -983,13 +979,11 @@ possibly_become_daemon(const struct options *options) {bool ret = false; -#ifdef ENABLE_SYSTEMD /* return without forking if we are running from systemd */ -if (sd_notify (0 ,"READY=0" ) >0 ) +if (platform_notify ("READY=0" ,"STATUS=Possibly become daemon" ) >0 ) {return ret; } -#endif if (options->daemon) {
@@ -1026,7 +1020,6 @@ 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
@@ -1040,10 +1033,8 @@ do_uid_gid_chroot(struct context *c, bool no_delay)* 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 ()); -#endif +platform_notify ("READY=1" , +"STATUS=Entering chroot, most of the init completed successfully" );platform_chroot (c->options.chroot_dir); }else if (c->first_time)
@@ -1384,17 +1375,13 @@ initialization_sequence_completed(struct context *c, const unsigned int flags)show_adapters (M_INFO |M_NOPREFIX );msg (M_INFO ,"%s With Errors ( see http://openvpn.net/faq.html#dhcpclientserv )" , message);#else -#ifdef ENABLE_SYSTEMD -sd_notifyf (0 ,"STATUS=Failed to start up: %s With Errors\nERRNO=1" , message); -#endif /* HAVE_SYSTEMD_SD_DAEMON_H */ +platform_notify ("READY=0" ,"STATUS=Failed to start up" );msg (M_INFO ,"%s With Errors" , message);#endif }else { -#ifdef ENABLE_SYSTEMD -sd_notifyf (0 ,"READY=1\nSTATUS=%s\nMAINPID=%lu" , message, (unsigned long )getpid ()); -#endif +platform_notify ("READY=1" ,"STATUS=Initialization Sequence Completed" );msg (M_INFO ,"%s" , message); }
src/openvpn/platform.c +13 -0
@@ -30,6 +30,10 @@#include "syshead.h" +#ifdef ENABLE_SYSTEMD +#include <systemd/sd-daemon.h> +#endif +#include "buffer.h" #include "error.h" #include "win32.h"
@@ -336,3 +340,12 @@ platform_stat(const char *path, platform_stat_t *buf)#endif } +int +platform_notify (const char *status,const char *message) +{ +#ifdef ENABLE_SYSTEMD +return sd_notifyf (0 ,"%s\n%s" , status, message); +#endif + +return 0 ; +}
src/openvpn/platform.h +2 -0
@@ -144,4 +144,6 @@ typedef struct stat platform_stat_t;#endif int platform_stat (const char *path,platform_stat_t *buf); +int platform_notify (const char *status,const char *message); +#endif /* ifndef PLATFORM_H */
Signature · 1 line
2.11.0
PATCH v2 2/2 fix timeout in non-TLS mode with systemd
From: Christian Hesse <[email protected]...>
In non-TLS configuration we wait for the remote peer to connect before issuing "Initialization Sequence Completed". So prevent to time out by telling systemd service manager we are ready for now. Status will be "Non-TLS mode, ready for now. Waiting for peer..." and changes once the remote peer connects.
This fixes #801 (static key tunnels impossible to start via systemd)
v2: Rebase on "move systemd specific code to platform.c" (commit
46e647933030da848774656029c4c4a1f204e2f1).
--- src/openvpn/openvpn.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
src/openvpn/openvpn.c +12 -0
@@ -73,6 +73,18 @@ tunnel_point_to_point(struct context *c)return ; } +/* In non-TLS configuration we wait for the remote peer to connect +* before issuing "Initialization Sequence Completed". So prevent to +* time out by telling systemd service manager we are ready for now. +* Status will be "Non-TLS mode, ready for now. Waiting for peer..." +* and changes once the remote peer connects. */ +if (c->options.tls_client == false + && c->options.tls_server == false) + { +platform_notify ("READY=1" , +"STATUS=Non-TLS mode, ready for now. Waiting for peer..." ); + } +/* main event loop */ while (true) {
Signature · 1 line
2.11.0
Re: [Openvpn-devel] [PATCH 1/2] move systemd specific code to platform.c
In reply to Christian HesseHi,
Quoted 5 lines · Christian Hesse
From: Christian Hesse <[email protected]...>
We have voices that do not want to "litter
ENABLE_SYSTEMDall over the code". So move the systemd specific bits toplatform_notify()inplatform.c.
While this is better, it's still far from a proper abstraction that could be used for cross-platform notification - like Selva said, we might want to add windows event logging eventually (quite likely a larger user base than Linux)...
So, for a start, passing arguments to platform_notify() that do not make sense to anything that is not sd_notify() is a non-starter.
Then, please investigate using the existing status file / management interface code to add this, instead of adding new calls in init.c (I mentioned this yesterday). If our existing status file / management code is not up to more fine-grained notification, I'd rather see that one improved, and then call out to systemd / windows events / ... than new code in parallel to the existing notification code.
gert
Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany [email protected]...
fax: +49-89-35655025 [email protected]...