[Openvpn-devel,v4] Implement AUTH_FAIL, TEMP message support

Message ID 20220914170134.2659433-1-arne@rfc2549.org
State Accepted
Headers show
Series [Openvpn-devel,v4] Implement AUTH_FAIL, TEMP message support | expand

Commit Message

Arne Schwabe Sept. 14, 2022, 7:01 a.m. UTC
This allows a server to indicate a temporary problem on the server and
allows the server to indicate how to proceed (i.e. move to the next server,
retry the same server, wait a certain time,...)

This adds options_utils.c/h to be able to unit test the new function.

Patch v2: Improve documentation, format man page better, comment that
          protocol-flags is not a user usable option.

Patch v3: cleanup parse_auth_failed_temp to use a simple const string
          instead of a buffer

Patch v4: move message + strlen(TEMP) to caller

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 doc/man-sections/script-options.rst  |  36 ++++++++++
 src/openvpn/Makefile.am              |   1 +
 src/openvpn/init.c                   |   9 ++-
 src/openvpn/openvpn.vcxproj          |   2 +
 src/openvpn/options.h                |   9 ++-
 src/openvpn/options_util.c           | 102 +++++++++++++++++++++++++++
 src/openvpn/options_util.h           |  33 +++++++++
 src/openvpn/push.c                   |  33 +++++----
 src/openvpn/ssl.c                    |  13 ++--
 src/openvpn/ssl.h                    |   3 +
 tests/unit_tests/openvpn/Makefile.am |   1 +
 tests/unit_tests/openvpn/test_misc.c |  40 +++++++++++
 12 files changed, 264 insertions(+), 18 deletions(-)
 create mode 100644 src/openvpn/options_util.c
 create mode 100644 src/openvpn/options_util.h

Comments

Gert Doering Sept. 18, 2022, 6:25 a.m. UTC | #1
Hi,

On Wed, Sep 14, 2022 at 07:01:34PM +0200, Arne Schwabe wrote:
>  src/openvpn/openvpn.vcxproj          |   2 +

This file is still gone, so the hunk also needs to go.  But I can 
ignore this.

> diff --git a/doc/man-sections/script-options.rst b/doc/man-sections/script-options.rst
> index 61c89b0ad..2a2e70fca 100644
> --- a/doc/man-sections/script-options.rst
> +++ b/doc/man-sections/script-options.rst
> @@ -102,6 +102,42 @@ SCRIPT HOOKS
>    the authentication, a :code:`1` or :code:`0` must be written to the
>    file specified by the :code:`auth_control_file`.
>  
> +  If the file specified by :code:`auth_failed_reason` exists and has non-empty

If this is referring to the environment variable, it needs to be
"auth_failed_reason_file".  If you agree, I'll change that on merge.

[..]

> diff --git a/src/openvpn/Makefile.am b/src/openvpn/Makefile.am
> index 71648ef18..79a3f9574 100644
> --- a/src/openvpn/Makefile.am
> +++ b/src/openvpn/Makefile.am
> @@ -101,6 +101,7 @@ openvpn_SOURCES = \
>  	pkcs11_mbedtls.c \
>  	openvpn.c openvpn.h \
>  	options.c options.h \
> +    options_util.c options_util.h \

Makefile wants a real TAB here, which I'll fix on commit.

More detailed stare-at-code and test to follow.

gert
Arne Schwabe Sept. 18, 2022, 10:39 a.m. UTC | #2
Am 18.09.2022 um 18:25 schrieb Gert Doering:
> Hi,
>
> On Wed, Sep 14, 2022 at 07:01:34PM +0200, Arne Schwabe wrote:
>>   src/openvpn/openvpn.vcxproj          |   2 +
> This file is still gone, so the hunk also needs to go.  But I can
> ignore this.
>
>> diff --git a/doc/man-sections/script-options.rst b/doc/man-sections/script-options.rst
>> index 61c89b0ad..2a2e70fca 100644
>> --- a/doc/man-sections/script-options.rst
>> +++ b/doc/man-sections/script-options.rst
>> @@ -102,6 +102,42 @@ SCRIPT HOOKS
>>     the authentication, a :code:`1` or :code:`0` must be written to the
>>     file specified by the :code:`auth_control_file`.
>>   
>> +  If the file specified by :code:`auth_failed_reason` exists and has non-empty
> If this is referring to the environment variable, it needs to be
> "auth_failed_reason_file".  If you agree, I'll change that on merge.

Yes sorry. My mistake.


Arne
Gert Doering Sept. 20, 2022, 2:26 a.m. UTC | #3
Stared-at-code.

Fixed file name reference in the documentation (as agreed), and TAB
in Makefile.am.

Tested with the "regular" server and client setups - nothing breaks,
but the new functionality isn't excercised, so this is not surprising.

Unit tests pass, too, which is good :-) - there is no real test for
"advance addr" vs. "advance remote" (there is "advance remote", but the
flags are not tested).  Maybe these could be added?


To actually *test* (and continuiously verify) the functionality I needed to
trick around a bit, creating a server-side script that "remembers" the
last TEMP fail, and succeeds on the 4th try...  but with that, it does
what it says on the tin - advance (to next addr) or not, stick to the
specified backoff time (remember to configure "--connect-retry 1" to *see*
the effect of short backoff timers :-) )

Your patch has been applied to the master branch.

commit c9474fa316a6f73286ed97b36c8f8b1ba62141bd
Author: Arne Schwabe
Date:   Wed Sep 14 19:01:34 2022 +0200

     Implement AUTH_FAIL, TEMP message support

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


--
kind regards,

Gert Doering

Patch

diff --git a/doc/man-sections/script-options.rst b/doc/man-sections/script-options.rst
index 61c89b0ad..2a2e70fca 100644
--- a/doc/man-sections/script-options.rst
+++ b/doc/man-sections/script-options.rst
@@ -102,6 +102,42 @@  SCRIPT HOOKS
   the authentication, a :code:`1` or :code:`0` must be written to the
   file specified by the :code:`auth_control_file`.
 
+  If the file specified by :code:`auth_failed_reason` exists and has non-empty
+  content, the content of this file will be used as AUTH_FAILED message. To
+  avoid race conditions, this file should be written before
+  :code:`auth_control_file`.
+
+  This auth fail reason can be something simple like "User has been permanently
+  disabled" but there are also some special auth failed messages.
+
+  The ``TEMP`` message indicates that the authentication
+  temporarily failed and that the client should continue to retry to connect.
+  The server can optionally give a user readable message and hint the client a
+  behavior how to proceed. The keywords of the ``AUTH_FAILED,TEMP`` message
+  are comma separated keys/values and provide a hint to the client how to
+  proceed. Currently defined keywords are:
+
+  ``backoff`` :code:`s`
+        instructs the client to wait at least :code:`s` seconds before the next
+        connection attempt. If the client already uses a higher delay for
+        reconnection attempt, the delay will not be shortened.
+
+  ``advance addr``
+        Instructs the client to reconnect to the next (IP) address of the
+        current server.
+
+  ``advance remote``
+        Instructs the client to skip the remaining IP addresses of the current
+        server and instead connect to the next server specified in the
+        configuration file.
+
+  ``advance no``
+        Instructs the client to retry connecting to the same server again.
+
+  For example, the message ``TEMP[backoff 42,advance no]: No free IP addresses``
+  indicates that the VPN connection can currently not succeed and instructs
+  the client to retry in 42 seconds again.
+
   When deferred authentication is in use, the script can also request
   pending authentication by writing to the file specified by the
   :code:`auth_pending_file`. The first line must be the timeout in
diff --git a/src/openvpn/Makefile.am b/src/openvpn/Makefile.am
index 71648ef18..79a3f9574 100644
--- a/src/openvpn/Makefile.am
+++ b/src/openvpn/Makefile.am
@@ -101,6 +101,7 @@  openvpn_SOURCES = \
 	pkcs11_mbedtls.c \
 	openvpn.c openvpn.h \
 	options.c options.h \
+    options_util.c options_util.h \
 	otime.c otime.h \
 	packet_id.c packet_id.h \
 	perf.c perf.h \
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index c40e975ed..d0ae7ac7d 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -484,13 +484,15 @@  next_connection_entry(struct context *c)
             /* Check if there is another resolved address to try for
              * the current connection */
             if (c->c1.link_socket_addr.current_remote
-                && c->c1.link_socket_addr.current_remote->ai_next)
+                && c->c1.link_socket_addr.current_remote->ai_next
+                && !c->options.advance_next_remote)
             {
                 c->c1.link_socket_addr.current_remote =
                     c->c1.link_socket_addr.current_remote->ai_next;
             }
             else
             {
+                c->options.advance_next_remote = false;
                 /* FIXME (schwabe) fix the persist-remote-ip option for real,
                  * this is broken probably ever since connection lists and multiple
                  * remote existed
@@ -2513,6 +2515,11 @@  socket_restart_pause(struct context *c)
             /* sec is less than 2^16; we can left shift it by up to 15 bits without overflow */
             sec = max_int(sec, 1) << min_int(backoff, 15);
         }
+        if (c->options.server_backoff_time)
+        {
+            sec = max_int(sec, c->options.server_backoff_time);
+            c->options.server_backoff_time = 0;
+        }
 
         if (sec > c->options.ce.connect_retry_seconds_max)
         {
diff --git a/src/openvpn/openvpn.vcxproj b/src/openvpn/openvpn.vcxproj
index a45fb993f..9b5e416fa 100644
--- a/src/openvpn/openvpn.vcxproj
+++ b/src/openvpn/openvpn.vcxproj
@@ -308,6 +308,7 @@ 
     <ClCompile Include="occ.c" />
     <ClCompile Include="openvpn.c" />
     <ClCompile Include="options.c" />
+    <ClCompile Include="options_util.c" />
     <ClCompile Include="otime.c" />
     <ClCompile Include="packet_id.c" />
     <ClCompile Include="perf.c" />
@@ -400,6 +401,7 @@ 
     <ClInclude Include="occ.h" />
     <ClInclude Include="openvpn.h" />
     <ClInclude Include="options.h" />
+    <ClInclude Include="options_util.h" />
     <ClInclude Include="otime.h" />
     <ClInclude Include="ovpn_dco_win.h" />
     <ClInclude Include="packet_id.h" />
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index f4549edba..c0c0417ef 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -272,10 +272,17 @@  struct options
     struct connection_list *connection_list;
 
     struct remote_list *remote_list;
-    /* Do not advanced the connection or remote addr list*/
+    /* Do not advance the connection or remote addr list*/
     bool no_advance;
+    /* Advance directly to the next remote, skipping remaining addresses of the
+     * current remote */
+    bool advance_next_remote;
     /* Counts the number of unsuccessful connection attempts */
     unsigned int unsuccessful_attempts;
+    /* the server can suggest a backoff time to the client, it
+     * will still be capped by the max timeout between connections
+     * (300s by default) */
+    int server_backoff_time;
 
 #if ENABLE_MANAGEMENT
     struct http_proxy_options *http_proxy_override;
diff --git a/src/openvpn/options_util.c b/src/openvpn/options_util.c
new file mode 100644
index 000000000..72c8bda4d
--- /dev/null
+++ b/src/openvpn/options_util.c
@@ -0,0 +1,102 @@ 
+/*
+ *  OpenVPN -- An application to securely tunnel IP networks
+ *             over a single TCP/UDP port, with support for SSL/TLS-based
+ *             session authentication and key exchange,
+ *             packet encryption, packet authentication, and
+ *             packet compression.
+ *
+ *  Copyright (C) 2002-2022 OpenVPN Inc <sales@openvpn.net>
+ *  Copyright (C) 2010-2021 Fox Crypto B.V. <openvpn@foxcrypto.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2
+ *  as published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#elif defined(_MSC_VER)
+#include "config-msvc.h"
+#endif
+
+#include "syshead.h"
+
+#include "options_util.h"
+
+const char *
+parse_auth_failed_temp(struct options *o, const char *reason)
+{
+    struct gc_arena gc = gc_new();
+
+    const char *message = reason;
+    char *m = string_alloc(reason, &gc);
+
+    /* Check if the message uses the TEMP[flags]: message format*/
+    char *endofflags = strstr(m, "]");
+
+    /* Temporary failure from the server */
+    if (m[0] == '[' && endofflags)
+    {
+        message = strstr(reason, "]") + 1;
+        /* null terminate the substring to only looks for flags between [ and ] */
+        *endofflags = '\x00';
+        const char *token = strtok(m, "[,");
+        while (token)
+        {
+            if (!strncmp(token, "backoff ", strlen("backoff ")))
+            {
+                if (sscanf(token, "backoff %d", &o->server_backoff_time) != 1)
+                {
+                    msg(D_PUSH, "invalid AUTH_FAIL,TEMP flag: %s", token);
+                    o->server_backoff_time = 0;
+                }
+            }
+            else if (!strncmp(token, "advance ", strlen("advance ")))
+            {
+                token += strlen("advance ");
+                if (!strcmp(token, "no"))
+                {
+                    o->no_advance = true;
+                }
+                else if (!strcmp(token, "remote"))
+                {
+                    o->advance_next_remote = true;
+                    o->no_advance = false;
+                }
+                else if (!strcmp(token, "addr"))
+                {
+                    /* Go on to the next remote */
+                    o->no_advance = false;
+                }
+            }
+            else
+            {
+                msg(D_PUSH_ERRORS, "WARNING: unknown AUTH_FAIL,TEMP flag: %s", token);
+            }
+            token = strtok(NULL, "[,");
+        }
+    }
+
+    /* Look for the message in the original buffer to safely be
+     * able to return it */
+    if (!message || message[0] != ':')
+    {
+        message = "";
+    }
+    else
+    {
+        /* Skip the : at the beginning */
+        message += 1;
+    }
+    gc_free(&gc);
+    return message;
+}
diff --git a/src/openvpn/options_util.h b/src/openvpn/options_util.h
new file mode 100644
index 000000000..17d62047b
--- /dev/null
+++ b/src/openvpn/options_util.h
@@ -0,0 +1,33 @@ 
+/*
+ *  OpenVPN -- An application to securely tunnel IP networks
+ *             over a single TCP/UDP port, with support for SSL/TLS-based
+ *             session authentication and key exchange,
+ *             packet encryption, packet authentication, and
+ *             packet compression.
+ *
+ *  Copyright (C) 2002-2022 OpenVPN Inc <sales@openvpn.net>
+ *  Copyright (C) 2010-2021 Fox Crypto B.V. <openvpn@foxcrypto.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2
+ *  as published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#ifndef OPTIONS_UTIL_H_
+#define OPTIONS_UTIL_H_
+
+#include "options.h"
+
+const char *
+parse_auth_failed_temp(struct options *o, const char *reason);
+
+#endif
diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 47a799282..9f47b5d5a 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -38,6 +38,7 @@ 
 
 #include "memdbg.h"
 #include "ssl_util.h"
+#include "options_util.h"
 
 static char push_reply_cmd[] = "PUSH_REPLY";
 
@@ -58,15 +59,34 @@  receive_auth_failed(struct context *c, const struct buffer *buffer)
         return;
     }
 
+    struct buffer buf = *buffer;
+
+    /* If the AUTH_FAIL message ends with a , it is an extended message that
+     * contains further flags */
+    bool authfail_extended = buf_string_compare_advance(&buf, "AUTH_FAILED,");
+
+    const char *reason = NULL;
+    if (authfail_extended && BLEN(&buf))
+    {
+        reason = BSTR(&buf);
+    }
+
+    if (authfail_extended && buf_string_match_head_str(&buf, "TEMP"))
+    {
+        parse_auth_failed_temp(&c->options, reason + strlen("TEMP"));
+        c->sig->signal_received = SIGUSR1;
+        c->sig->signal_text = "auth-temp-failure (server temporary reject)";
+    }
     /* Before checking how to react on AUTH_FAILED, first check if the
      * failed auth might be the result of an expired auth-token.
      * Note that a server restart will trigger a generic AUTH_FAILED
      * instead an AUTH_FAILED,SESSION so handle all AUTH_FAILED message
      * identical for this scenario */
-    if (ssl_clean_auth_token())
+    else if (ssl_clean_auth_token())
     {
         c->sig->signal_received = SIGUSR1;     /* SOFT-SIGUSR1 -- Auth failure error */
         c->sig->signal_text = "auth-failure (auth-token)";
+        c->options.no_advance = true;
     }
     else
     {
@@ -89,19 +109,8 @@  receive_auth_failed(struct context *c, const struct buffer *buffer)
         c->sig->signal_text = "auth-failure";
     }
 #ifdef ENABLE_MANAGEMENT
-    struct buffer buf = *buffer;
-
-    /* If the AUTH_FAIL message ends with a , it is an extended message that
-     * contains further flags */
-    bool authfail_extended = buf_string_compare_advance(&buf, "AUTH_FAILED,");
-
     if (management)
     {
-        const char *reason = NULL;
-        if (authfail_extended && BLEN(&buf))
-        {
-            reason = BSTR(&buf);
-        }
         management_auth_failure(management, UP_TYPE_AUTH, reason);
     }
     /*
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index d3f7a0203..672cd9c84 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1972,14 +1972,19 @@  push_peer_info(struct buffer *buf, struct tls_session *session)
         /* support for exit notify via control channel */
         iv_proto |= IV_PROTO_CC_EXIT_NOTIFY;
 
-        /* support for receiving push_reply before sending
-         * push request, also signal that the client wants
-         * to get push-reply messages without without requiring a round
-         * trip for a push request message*/
         if (session->opt->pull)
         {
+            /* support for receiving push_reply before sending
+             * push request, also signal that the client wants
+             * to get push-reply messages without requiring a round
+             * trip for a push request message*/
             iv_proto |= IV_PROTO_REQUEST_PUSH;
+
+            /* Support keywords in the AUTH_PENDING control message */
             iv_proto |= IV_PROTO_AUTH_PENDING_KW;
+
+            /* support for AUTH_FAIL,TEMP control message */
+            iv_proto |= IV_PROTO_AUTH_FAIL_TEMP;
         }
 
         /* support for Negotiable Crypto Parameters */
diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
index 12ffd44d7..8ca4c4aa8 100644
--- a/src/openvpn/ssl.h
+++ b/src/openvpn/ssl.h
@@ -100,6 +100,9 @@ 
  *  This also includes support for the protocol-flags pushed option */
 #define IV_PROTO_CC_EXIT_NOTIFY  (1<<7)
 
+/** Support for AUTH_FAIL,TEMP messages */
+#define IV_PROTO_AUTH_FAIL_TEMP  (1<<8)
+
 /* Default field in X509 to be username */
 #define X509_USERNAME_FIELD_DEFAULT "CN"
 
diff --git a/tests/unit_tests/openvpn/Makefile.am b/tests/unit_tests/openvpn/Makefile.am
index 63b53a6ac..65cf9549c 100644
--- a/tests/unit_tests/openvpn/Makefile.am
+++ b/tests/unit_tests/openvpn/Makefile.am
@@ -176,5 +176,6 @@  misc_testdriver_LDFLAGS = @TEST_LDFLAGS@
 misc_testdriver_SOURCES = test_misc.c mock_msg.c \
     mock_get_random.c \
 	$(openvpn_srcdir)/buffer.c \
+	$(openvpn_srcdir)/options_util.c \
 	$(openvpn_srcdir)/ssl_util.c \
 	$(openvpn_srcdir)/platform.c
diff --git a/tests/unit_tests/openvpn/test_misc.c b/tests/unit_tests/openvpn/test_misc.c
index 636fc45d6..90c9a1214 100644
--- a/tests/unit_tests/openvpn/test_misc.c
+++ b/tests/unit_tests/openvpn/test_misc.c
@@ -37,6 +37,7 @@ 
 #include <cmocka.h>
 
 #include "ssl_util.h"
+#include "options_util.h"
 
 static void
 test_compat_lzo_string(void **state)
@@ -72,8 +73,47 @@  test_compat_lzo_string(void **state)
     gc_free(&gc);
 }
 
+static void
+test_auth_fail_temp_no_flags(void **state)
+{
+    struct options o;
+
+    const char *teststr = "TEMP:There are no flags here [really not]";
+
+    const char *msg = parse_auth_failed_temp(&o, teststr + strlen("TEMP"));
+    assert_string_equal(msg, "There are no flags here [really not]");
+}
+
+static void
+test_auth_fail_temp_flags(void **state)
+{
+    struct options o;
+
+    const char *teststr = "[backoff 42,advance no]";
+
+    const char *msg = parse_auth_failed_temp(&o, teststr);
+    assert_string_equal(msg, "");
+    assert_int_equal(o.server_backoff_time, 42);
+    assert_int_equal(o.no_advance, true);
+}
+
+static void
+test_auth_fail_temp_flags_msg(void **state)
+{
+    struct options o;
+
+    const char *teststr = "[advance remote,backoff 77]:go round and round";
+
+    const char *msg = parse_auth_failed_temp(&o, teststr);
+    assert_string_equal(msg, "go round and round");
+    assert_int_equal(o.server_backoff_time, 77);
+}
+
 const struct CMUnitTest misc_tests[] = {
     cmocka_unit_test(test_compat_lzo_string),
+    cmocka_unit_test(test_auth_fail_temp_no_flags),
+    cmocka_unit_test(test_auth_fail_temp_flags),
+    cmocka_unit_test(test_auth_fail_temp_flags_msg),
 };
 
 int