[Openvpn-devel,8/8] Make any auth failure tls_authentication_status return auth failed

Message ID 20201023120259.29783-7-arne@rfc2549.org
State Accepted
Headers show
Series
  • [Openvpn-devel] Remove --disable-def-auth configure argument
Related show

Commit Message

Arne Schwabe Oct. 23, 2020, 12:02 p.m.
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>
---
 src/openvpn/multi.c      | 12 ++----------
 src/openvpn/ssl_verify.c | 25 +++++++++++++++++++++----
 2 files changed, 23 insertions(+), 14 deletions(-)

Comments

Gert Doering Nov. 29, 2020, 11:27 a.m. | #1
Acked-by: Gert Doering <gert@greenie.muc.de>

Stared at the code for a bit, and it looks simple and straightforward
enough.  With 1/8...7/8 reviewed, I think I even understand what it
does :-)

The bit about "ks->authenticated == KS_AUTH_FALSE" becomes clear when
you look ks_auth_state - FALSE is 0, everything else is > FALSE, so
the "else" is basically the same condition as before.


Testing (only script backend + pam auth + token):

This improves the "token expired on renegotiation" even further,
though it still shows some interesting timing aspects.

Case 1: auth-gen-token 600, sync plugin auth-pam

2020-11-29 10:30:16 us=100840 --auth-token-gen: auth-token from client expired
2020-11-29 10:30:16 us=100868 TLS: Username/auth-token authentication failed for username 'fbsd-tc-master'
2020-11-29 10:30:16 us=101534 Control Channel: TLSv1.3, cipher TLSv1.3 TLS_AES_256_GCM_SHA384, 2048 bit RSA
2020-11-29 10:30:32 us=478267 Delayed exit in 5 seconds
2020-11-29 10:30:32 us=478374 SENT CONTROL [cron2-freebsd-tc-amd64]: 'AUTH_FAILED,SESSION: token expired' (status=1)

I wonder what it is doing here, between 10:30:16 and 10:30:32?  Expiring
the active key?

On the client side, we see renegotiation, TLS handshake, then a pause
(in which the connection still works fine(!)), then AUTH FAIL, and then
reconnect with user/password credentials:

2020-11-29 10:30:16 TLS: soft reset sec=151/151 bytes=89537/-1 pkts=705/0
2020-11-29 10:30:16 Control Channel: TLSv1.3, cipher TLSv1.3 TLS_AES_256_GCM_SHA384, 2048 bit RSA
2020-11-29 10:30:32 AUTH: Received control message: AUTH_FAILED,SESSION: token expired

2020-11-29 10:30:32 Restart pause, 5 second(s)
..
2020-11-29 10:30:37 TLS: Initial packet from ...
..
2020-11-29 10:30:37 PUSH: Received control message: 'PUSH_REPLY,...,xauth-tokenSESS_ID,...
..
2020-11-29 10:30:38 Initialization Sequence Completed


The server confirms:

2020-11-29 10:30:37 us=551426 TLS: Username/Password authentication succeeded for username 'fbsd-tc-master' 


So - this is definitely very nice.



Test case 2: auth-gen-token 600, deferred plugin auth-pam with 16s delay

- initial connect takes 18s (which is expected)
- initial TLS-reconnect with token takes 0.002s (which is very nice :) )
- TLS reconnect after 10 minute, with expired token...

2020-11-29 10:50:05 us=911127 --auth-token-gen: auth-token from client expired
2020-11-29 10:50:05 us=911147 TLS: Username/auth-token authentication failed for username 'fbsd-tc-master'
2020-11-29 10:50:05 us=912088 Control Channel: TLSv1.3, cipher TLSv1.3 TLS_AES_256_GCM_SHA384, 2048 bit RSA
2020-11-29 10:50:21 us=533468 Delayed exit in 5 seconds
2020-11-29 10:50:21 us=533597 SENT CONTROL [cron2-freebsd-tc-amd64]: 'AUTH_FAILED,SESSION: token expired' (status=1)

.. does the right thing now!

The client understands, and reconnects with password:

2020-11-29 10:50:26 us=601574 Re-using SSL/TLS context
2020-11-29 10:50:26 us=614341 PLUGIN AUTH-PAM: do deferred auth '/tmp/openvpn_acf_3131079bbfa3c5c756b7e374b8328dfc.tmp'
2020-11-29 10:50:26 us=627822 TLS: Username/Password authentication deferred for username 'fbsd-tc-master' 
2020-11-29 10:50:41 us=641229 PLUGIN AUTH-PAM: BACKGROUND: fbsd-tc-master: deferred auth: PAM succeeded
2020-11-29 10:50:42 us=814877 SENT CONTROL [cron2-freebsd-tc-amd64]: 'PUSH_REPLY,...,auth-tokenSESS_ID,...,key-derivation tls-ekm' (status=1)


And things are back to "it pings".  Of course the "no-ping" time is longer in 
this case, due to client reconnect (5s) *plus* 16s AUTH-PAM backgrounding, 
but this is exactly what is to be expected.

(This is the case where OpenVPN would previously get totally confused and 
never tell the client "hey, you're dead, reconnect!" - so, bug fixed, great 
stuff)


Your patch has been applied to the master branch (because it needs all
the prerequisite refactoring patches).  Now, how to get the bugfix part
into 2.5.1?

commit 88dc4276485bf2a4cecae3cff55d2e146dcd31ca
Author: Arne Schwabe
Date:   Fri Oct 23 14:02:59 2020 +0200

     Make any auth failure tls_authentication_status return auth failed

     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>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 9becb2b2..401dfa8e 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -3963,17 +3963,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 98985c51..a4538d38 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -939,6 +939,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;
@@ -951,7 +954,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;
@@ -964,6 +971,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)
                 {
@@ -983,10 +991,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;
     }