Make any auth failure tls_authentication_status return auth failed

Previously tls_authentication_status only return
TLS_AUTHENTICATION_FAILED if there is no usable key at all. This
behaviour allows continuing using the still valid keys
(see --tran-window). However, the OpenVPN protocol lacks a way of
communicating that key is not useable to client once it reached
the TLS authenticated status (eg cert checks pass but connect or
user-pass verify fail). To avoid these desynchronisation issues
during deferred auth and renegotiation OpenVPN quietly only starts
using a new key after the hand-window has passed.

With this change any failure on a renogiation will lead to a
deauthentication of a client. This also fixes a number of bugs that
expiring auth-token and failed deferred auth is leading to key desync
or unexpected continuation of the VPN session.

The behaviour of deauthentication of all keys on deferred auth failure
has been already been used for years if authentication is done via
management interface. This commit also aligns the code paths for both.

A side effect might be that we also deauth clients earlier in some
other corner cases but the behaviour of continuing using an old
authenticated session while we already a failed authentication for the
client is most times unexpected behaviour from the user (admin).

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

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 436834de9d98d7be218c62d81bab2608d7baad1f..dd713049e0fd0fa3d27ba3d0d902bb629bbec245 100644 (file)
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -3960,17 +3960,9 @@ management_client_auth(void *arg,
                     cc_config_owned = false;
                 }
             }
-            else
+            else if (reason)
             {
-                if (reason)
-                {
-                    msg(D_MULTI_LOW, "MULTI: connection rejected: %s, CLI:%s", reason, np(client_reason));
-                }
-                if (!is_cas_pending(mi->context.c2.context_auth))
-                {
-                    send_auth_failed(&mi->context, client_reason); /* mid-session reauth failed */
-                    multi_schedule_context_wakeup(m, mi);
-                }
+                msg(D_MULTI_LOW, "MULTI: connection rejected: %s, CLI:%s", reason, np(client_reason));
             }
         }
     }

diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index 43e48adb4283b72b1df493bbb45156323785c39c..e04c5c35c5a1c1b66b271437a36e832463e470b6 100644 (file)
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -937,6 +937,9 @@ tls_authentication_status(struct tls_multi *multi, const int latency)
     /* at least one key is enabled for decryption */
     int active = 0;
 
+    /* at least one key already failed authentication */
+    bool failed_auth = false;
+
     if (latency && multi->tas_last + latency >= now)
     {
         return TLS_AUTHENTICATION_UNDEFINED;
@@ -949,7 +952,11 @@ tls_authentication_status(struct tls_multi *multi, const int latency)
         if (TLS_AUTHENTICATED(multi, ks))
         {
             active++;
-            if (ks->authenticated > KS_AUTH_FALSE)
+            if (ks->authenticated == KS_AUTH_FALSE)
+            {
+                failed_auth = true;
+            }
+            else
             {
                 unsigned int s1 = ACF_DISABLED;
                 unsigned int s2 = ACF_DISABLED;
@@ -962,6 +969,7 @@ tls_authentication_status(struct tls_multi *multi, const int latency)
                 if (s1 == ACF_FAILED || s2 == ACF_FAILED)
                 {
                     ks->authenticated = KS_AUTH_FALSE;
+                    failed_auth = true;
                 }
                 else if (s1 == ACF_UNDEFINED || s2 == ACF_UNDEFINED)
                 {
@@ -981,10 +989,19 @@ tls_authentication_status(struct tls_multi *multi, const int latency)
     }
 
 #if 0
-    dmsg(D_TLS_ERRORS, "TAS: a=%d s=%d d=%d", active, success, deferred);
+    dmsg(D_TLS_ERRORS, "TAS: a=%d s=%d d=%d f=%d", active, success, deferred, failed_auth);
 #endif
-
-    if (success)
+    if (failed_auth)
+    {
+        /* We have at least one session that failed authentication. There
+         * might be still another session with valid keys.
+         * Although our protocol allows keeping the VPN session alive
+         * with the other session (and we actually did that in earlier
+         * version, this behaviour is really strange from a user (admin)
+         * experience */
+        return TLS_AUTHENTICATION_FAILED;
+    }
+    else if (success)
     {
         return TLS_AUTHENTICATION_SUCCEEDED;
     }