diff mbox series

[Openvpn-devel] Ignore connection attempts while server is shutting down

Message ID 20221208153129.1207228-1-arne@rfc2549.org
State Accepted
Headers
Series [Openvpn-devel] Ignore connection attempts while server is shutting down |

Commit Message

Arne Schwabe Dec. 8, 2022, 3:31 p.m. UTC 
  Currently we still allow clients to connect while the server is waiting
to shut down. This window is very small (2s) and is only used when
explicit-exit-notify is enabled on the server side.

The chance of a client connecting during this time period is very low
unless someone puts something stupid like --connect-retry 1 3 into his/her
client config and forces the client to reconnect during this time period.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/mudp.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Gert Doering Dec. 12, 2022, 1:07 p.m. UTC | #1 
Acked-by: Gert Doering <gert@greenie.muc.de>

Stare-at-code looks good, and testing confirms that it does fix over-eager
clients reconnecting too fast:

^C2022-12-12 14:03:11 us=841186 event_wait : Interrupted system call (fd=-1,code=4)
2022-12-12 14:03:11 us=841294 SENT CONTROL [cron2-freebsd-tc-amd64]: 'RESTART' (status=1)
2022-12-12 14:03:11 us=841346 SENT CONTROL [freebsd-74-amd64]: 'RESTART' (status=1)
2022-12-12 14:03:11 us=841391 SENT CONTROL [freebsd-11-amd64]: 'RESTART' (status=1)
2022-12-12 14:03:12 us=893434 MULTI: Connection attempt from 194.97.140.21:13788 ignored while server is shutting down

and on the client:

2022-12-12 14:03:11 SIGUSR1[soft,server-pushed-connection-reset] received, process restarting
2022-12-12 14:03:11 Restart pause, 1 second(s)
2022-12-12 14:03:12 TCP/UDP: Preserving recently used remote address: [AF_INET]194.97.140.11:51196
[..]
2022-12-12 14:04:12 TLS Error: TLS key negotiation failed to occur within 60 seconds (check your network connectivity)
2022-12-12 14:04:12 TLS Error: TLS handshake failed

so, no more "it reconnects right away and then the server disappears"
(which I could reproduce without the patch - the client would then
show "Initialization Sequence Completed" and wait for --ping-timeout).


For 2.7, we might actually consider reworking the server-exit behaviour
to "exit as soon as all outstanding control messages have been ACKed"
or even "fire-and-forget on the CC EENs"?  But this is a bigger thing
than "fix goes into 2.6_beta2", I'm afraid.

Your patch has been applied to the master and release/2.6 branch.

commit 7d0a90335fe79a352456f262ce42ea501796ae87 (master)
commit f8bfe1a5fb785d2f26f3a38d597604031f081018 (release/2.6)
Author: Arne Schwabe
Date:   Thu Dec 8 16:31:29 2022 +0100

     Ignore connection attempts while server is shutting down

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20221208153129.1207228-1-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25638.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/mudp.c b/src/openvpn/mudp.c
index bdf35a8ba..458152335 100644
--- a/src/openvpn/mudp.c
+++ b/src/openvpn/mudp.c
@@ -229,8 +229,13 @@  multi_get_create_instance_udp(struct multi_context *m, bool *floated)
         if (!mi)
         {
             struct tls_pre_decrypt_state state = {0};
-
-            if (do_pre_decrypt_check(m, &state, real))
+            if (m->deferred_shutdown_signal.signal_received)
+            {
+                msg(D_MULTI_ERRORS,
+                    "MULTI: Connection attempt from %s ignored while server is "
+                    "shutting down", mroute_addr_print(&real, &gc));
+            }
+            else if (do_pre_decrypt_check(m, &state, real))
             {
                 /* This is an unknown session but with valid tls-auth/tls-crypt
                  * (or no auth at all).  If this is the initial packet of a