[Openvpn-devel] Fix auth-token not being updated if auth-nocache is set

Message ID 20201130123928.21837-1-arne@rfc2549.org
State Accepted
Headers show
Series
  • [Openvpn-devel] Fix auth-token not being updated if auth-nocache is set
Related show

Commit Message

Arne Schwabe Nov. 30, 2020, 12:39 p.m.
This fixes the auth-token not being updated if auth-nocache is set. Our
set_auth_token method ensures that the auth-token always has a username
but is a little bit too strict in the check.

Also add doxygen documentation and remove null checks. We use this function
only with non-null pointers and it makes it a bit nicer to read.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/misc.c |  9 +++++++--
 src/openvpn/misc.h | 11 +++++++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

Comments

Gert Doering Nov. 30, 2020, 4:35 p.m. | #1
Acked-by: Gert Doering <gert@greenie.muc.de>

Thanks for digging into this - this was an annoying and hard 
to diagnose "sometimes, TLS reconnects fail for users where 
it *should* succeed due to tokens being used" problem (that
openvpn considers tokens sensitive and never logs them 
didn't help pinpointing the issue :-) ).

I tested this with a client built with lots of extra
debug output, gen-auth-token and frequend tls-renegotiates - 
and indeed, "up->defined" goes to "0" after the first incoming 
token if auth-nocache is active - so, no *further* tokens are 
learned.  With the patch, we also look at "tk->defined", which 
is *then* defined, and tokens work even in that case.

Verified with a 2.4 client against a master server, including
a master restart without having to re-enter credentials on
the client.  Yay :-)

Your patch has been applied to the master, release/2.5 and
release/2.4 branch (bugfix, identical code in all branches).

commit fb789947ab1eba3e68fb8e4b3551d095a53962bd (master)
commit 95e183723fc6571c73ed070b22923df2ce666af2 (release/2.5)
commit f9b73042892c14b906772e72b3116d809457c721 (release/2.4)
Author: Arne Schwabe
Date:   Mon Nov 30 13:39:28 2020 +0100

     Fix auth-token not being updated if auth-nocache is set

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


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
index 1038b383..c0c72dd7 100644
--- a/src/openvpn/misc.c
+++ b/src/openvpn/misc.c
@@ -510,10 +510,15 @@  void
 set_auth_token(struct user_pass *up, struct user_pass *tk, const char *token)
 {
 
-    if (token && strlen(token) && up && up->defined)
+    if (strlen(token) && (up->defined || tk->defined))
     {
+        /* auth-token has no password, so it needs the username
+         * either already set or copied from up */
         strncpynt(tk->password, token, USER_PASS_LEN);
-        strncpynt(tk->username, up->username, USER_PASS_LEN);
+        if (up->defined)
+        {
+            strncpynt(tk->username, up->username, USER_PASS_LEN);
+        }
         tk->defined = true;
     }
 
diff --git a/src/openvpn/misc.h b/src/openvpn/misc.h
index a03d94e2..8c3a1227 100644
--- a/src/openvpn/misc.h
+++ b/src/openvpn/misc.h
@@ -145,6 +145,17 @@  void fail_user_pass(const char *prefix,
 
 void purge_user_pass(struct user_pass *up, const bool force);
 
+/**
+ * Sets the auth-token to token if a username is available from either
+ * up or already present in tk. The method will also purge up if
+ * the auth-nocache option is active.
+ *
+ * @param up        (non Auth-token) Username/password
+ * @param tk        auth-token userpass to set
+ * @param token     token to use as password for the
+ *
+ * @note    all parameters to this function must not be null.
+ */
 void set_auth_token(struct user_pass *up, struct user_pass *tk,
                     const char *token);