| Message ID | 20201023120259.29783-7-arne@rfc2549.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel] Remove --disable-def-auth configure argument | expand |
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
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; }
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(-)