[Openvpn-devel,v2,2/9] Implement auth-token-user

Message ID 20210520151148.2565578-2-arne@rfc2549.org
State Accepted
Delegated to: Antonio Quartulli
Headers show
Series
  • [Openvpn-devel,v2,1/9] Move auth_token_state from multi to key_state
Related show

Commit Message

Arne Schwabe May 20, 2021, 3:11 p.m.
When not using username and password (i.e. auth-user-pass) it can still make
to provide the client with an auth-token, e.g. for allowing a session to
continue after a reconnect without requiring 2FA again.

However, without --auth-user-pass openvpn does not have a username and will
ignore any pushed auth-token command.

This patch adds support for auth-token-user to set the username that should
be used for auth-token

The spec of using auth-token-user base64-encoded-user are the ones that
OpenVPN3 already implements.

Patch V2: Improve style, fix comments and commit message

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 doc/man-sections/client-options.rst |  8 +++++++
 src/openvpn/misc.c                  | 37 +++++++++++++++++++++++++----
 src/openvpn/misc.h                  | 21 +++++++++++++---
 src/openvpn/options.c               |  5 ++++
 src/openvpn/ssl.c                   | 12 +++++++---
 src/openvpn/ssl.h                   |  2 ++
 6 files changed, 74 insertions(+), 11 deletions(-)

Comments

Antonio Quartulli June 11, 2021, 12:41 a.m. | #1
Hi,

On 20/05/2021 17:11, Arne Schwabe wrote:
> When not using username and password (i.e. auth-user-pass) it can still make
> to provide the client with an auth-token, e.g. for allowing a session to
> continue after a reconnect without requiring 2FA again.
> 
> However, without --auth-user-pass openvpn does not have a username and will
> ignore any pushed auth-token command.
> 
> This patch adds support for auth-token-user to set the username that should
> be used for auth-token
> 
> The spec of using auth-token-user base64-encoded-user are the ones that
> OpenVPN3 already implements.
> 
> Patch V2: Improve style, fix comments and commit message
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>

This patch hasn't changed since the last time it was on the mailing list
(and I acked it), so the same goes for this copy.

@Arne I have a new question though: what is expected to happen is the
--auth-token-user is specified in the global config?

Is the same user supposed to be used with every client?
Or is it just ignored?

I am testing this case and I don't see the user being pushed to the client.

Cheers,
Arne Schwabe June 11, 2021, 9:48 a.m. | #2
Am 11.06.21 um 02:41 schrieb Antonio Quartulli:
> Hi,
> 
> On 20/05/2021 17:11, Arne Schwabe wrote:
>> When not using username and password (i.e. auth-user-pass) it can still make
>> to provide the client with an auth-token, e.g. for allowing a session to
>> continue after a reconnect without requiring 2FA again.
>>
>> However, without --auth-user-pass openvpn does not have a username and will
>> ignore any pushed auth-token command.
>>
>> This patch adds support for auth-token-user to set the username that should
>> be used for auth-token
>>
>> The spec of using auth-token-user base64-encoded-user are the ones that
>> OpenVPN3 already implements.
>>
>> Patch V2: Improve style, fix comments and commit message
>>
>> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> 
> This patch hasn't changed since the last time it was on the mailing list
> (and I acked it), so the same goes for this copy.
> 
> @Arne I have a new question though: what is expected to happen is the
> --auth-token-user is specified in the global config?
> 
> Is the same user supposed to be used with every client?
> Or is it just ignored?
> 
> I am testing this case and I don't see the user being pushed to the client.
>

It is probably the same as with auth-token itself. OpenVPN will pick it
up and use it but most times it is not every useful as auth-token should
be pushed from the server. It is an artefact from how we parse things.

Arne
Antonio Quartulli June 14, 2021, 12:30 a.m. | #3
Hi,

On 11/06/2021 11:48, Arne Schwabe wrote:
> Am 11.06.21 um 02:41 schrieb Antonio Quartulli:
>> Hi,
>>
>> On 20/05/2021 17:11, Arne Schwabe wrote:
>>> When not using username and password (i.e. auth-user-pass) it can still make
>>> to provide the client with an auth-token, e.g. for allowing a session to
>>> continue after a reconnect without requiring 2FA again.
>>>
>>> However, without --auth-user-pass openvpn does not have a username and will
>>> ignore any pushed auth-token command.
>>>
>>> This patch adds support for auth-token-user to set the username that should
>>> be used for auth-token
>>>
>>> The spec of using auth-token-user base64-encoded-user are the ones that
>>> OpenVPN3 already implements.
>>>
>>> Patch V2: Improve style, fix comments and commit message
>>>
>>> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
>>
>> This patch hasn't changed since the last time it was on the mailing list
>> (and I acked it), so the same goes for this copy.
>>
>> @Arne I have a new question though: what is expected to happen is the
>> --auth-token-user is specified in the global config?
>>
>> Is the same user supposed to be used with every client?
>> Or is it just ignored?
>>
>> I am testing this case and I don't see the user being pushed to the client.
>>
> 
> It is probably the same as with auth-token itself. OpenVPN will pick it
> up and use it but most times it is not every useful as auth-token should
> be pushed from the server. It is an artefact from how we parse things.
> 

Makes sense - it is jus interpreted locally, but there is not much sense.

This said, the rest looks good.

Acked-by: Antonio Quartulli <antonio@openvpn.net>
Gert Doering June 15, 2021, 6:14 p.m. | #4
Your patch has been applied to the master and release/2.5 branch 
(long-term compat).

I'm not exactly happy with this patch for a number of reasons, which is
why I was a bit reluctant to merge it.  It does what it says on the lid,
the code is safe, and it got ACKs.  So, merging.

That said, this is sort of a special case for a special case that 
seems to have come out of OpenVPN Inc product marketing (do 2FA outside 
OpenVPN and then notice that this doesn't actually work) - and it makes
the code even more complex by having this extra boolean to check in
a number of places...  (a somewhat simpler approach could be to have
a "default username" - <NOT-SET> or such - and use that if up is not
defined)


Anyway.  I have tested the "does auth-token, on reneg, and token
expiry still work?" bit fairly thoroughly - as this is code that took
us quite a while to get right - and it seems to still work.  I have
not tested actually pushing "auth-token-user bla", but I have been
told Heiko and David have tested that path in earnest.

I have also taken the liberty to clean up the comments quite a bit
(seems Richard did not like this patch and did not spell-check :-) ).

commit b398aa37ca309948b481401adf0074ea5589eb2d (master)
commit d38d61111d08558e2f52cc9bcdc928ca9c4fca61 (release/2.5)
Author: Arne Schwabe
Date:   Thu May 20 17:11:41 2021 +0200

     Implement auth-token-user

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


--
kind regards,

Gert Doering

Patch

diff --git a/doc/man-sections/client-options.rst b/doc/man-sections/client-options.rst
index af21fbcd7..c5b7ad960 100644
--- a/doc/man-sections/client-options.rst
+++ b/doc/man-sections/client-options.rst
@@ -50,6 +50,14 @@  configuration.
   after a failed auth. Older clients will keep using the token value and
   react according to ``--auth-retry``
 
+--auth-token-user base64username
+  Companion option to ``--auth-token``. This options allows to override
+  the username used by the client when reauthenticating with the ``auth-token``.
+  It also allows to use ``--auth-token`` in setups that normally do not use
+  username and password.
+
+  The username has to be base64 encoded.
+
 --auth-user-pass
   Authenticate with server using username/password.
 
diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
index 650daa0c6..29061cd6f 100644
--- a/src/openvpn/misc.c
+++ b/src/openvpn/misc.c
@@ -490,22 +490,49 @@  void
 set_auth_token(struct user_pass *up, struct user_pass *tk, const char *token)
 {
 
-    if (strlen(token) && (up->defined || tk->defined))
+    if (strlen(token))
     {
-        /* auth-token has no password, so it needs the username
-         * either already set or copied from up */
         strncpynt(tk->password, token, USER_PASS_LEN);
-        if (up->defined)
+        tk->token_defined = true;
+
+        /*
+         * --auth-token has no username, so it needs the username
+         * either already set or copied from up, or later set by
+         * --auth-token-user
+         *
+         * Do not overwrite the username if already set to avoid
+         * overwriting an auth-token
+         */
+        if (up->defined && !tk->defined)
         {
             strncpynt(tk->username, up->username, USER_PASS_LEN);
+            tk->defined = true;
         }
-        tk->defined = true;
     }
 
     /* Cleans user/pass for nocache */
     purge_user_pass(up, false);
 }
 
+void
+set_auth_token_user(struct user_pass *tk, const char *username)
+{
+    if (strlen(username))
+    {
+        /* Clear the username before decoding to ensure no old material is left
+         * and also allow decoding to not use all space to ensure the last byte is
+         * always 0 */
+        CLEAR(tk->username);
+        int len = openvpn_base64_decode(username, tk->username, USER_PASS_LEN - 1);
+        tk->defined = len > 0;
+        if (!tk->defined)
+        {
+            msg(D_PUSH, "Error decoding auth-token-username");
+        }
+    }
+}
+
+
 /*
  * Process string received by untrusted peer before
  * printing to console or log file.
diff --git a/src/openvpn/misc.h b/src/openvpn/misc.h
index d9005353e..0d2d42489 100644
--- a/src/openvpn/misc.h
+++ b/src/openvpn/misc.h
@@ -56,6 +56,9 @@  const char *hostname_randomize(const char *hostname, struct gc_arena *gc);
 struct user_pass
 {
     bool defined;
+    /* For auth-token username and token can be set individually, so
+     * we this second bool to track if the token (password) is defined */
+    bool token_defined;
     bool nocache;
 
 /* max length of username/password */
@@ -138,19 +141,31 @@  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
+ * Sets the auth-token to token. Ff a username is available from either
+ * up or already present in tk is the auth-token that will be used as default
+ * username for the token. 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
+ * @param token     token to use as password for the auth-token
  *
  * @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);
 
+/**
+ * Sets the auth-token username by base64 decoding the passed
+ * username
+ *
+ * @param tk        auth-token userpass to set
+ * @param username  base64 encoded username to set
+ *
+ * @note    all parameters to this function must not be null.
+ */
+void set_auth_token_user(struct user_pass *tk, const char *username);
+
 /*
  * Process string received by untrusted peer before
  * printing to console or log file.
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 5a6f37d7d..fa3ee50d6 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -8308,6 +8308,11 @@  add_option(struct options *options,
         }
 #endif
     }
+    else if (streq(p[0], "auth-token-user") && p[1] && !p[2])
+    {
+        VERIFY_PERMISSION(OPT_P_ECHO);
+        ssl_set_auth_token_user(p[1]);
+    }
     else if (streq(p[0], "single-session") && !p[1])
     {
         VERIFY_PERMISSION(OPT_P_GENERAL);
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index e843b2152..7eb356ddf 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -446,6 +446,12 @@  ssl_set_auth_token(const char *token)
     set_auth_token(&auth_user_pass, &auth_token, token);
 }
 
+void
+ssl_set_auth_token_user(const char *username)
+{
+    set_auth_token_user(&auth_token, username);
+}
+
 /*
  * Cleans an auth token and checks if it was active
  */
@@ -2310,8 +2316,8 @@  key_method_2_write(struct buffer *buf, struct tls_multi *multi, struct tls_sessi
         }
     }
 
-    /* write username/password if specified */
-    if (auth_user_pass_enabled)
+    /* write username/password if specified or we are using a auth-token */
+    if (auth_user_pass_enabled || (auth_token.token_defined && auth_token.defined))
     {
 #ifdef ENABLE_MANAGEMENT
         auth_user_pass_setup(session->opt->auth_user_pass_file, session->opt->sci);
@@ -2324,7 +2330,7 @@  key_method_2_write(struct buffer *buf, struct tls_multi *multi, struct tls_sessi
          * If we have a valid auth-token, send that instead of real
          * username/password
          */
-        if (auth_token.defined)
+        if (auth_token.token_defined && auth_token.defined)
         {
             up = &auth_token;
         }
diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
index 88f89876a..81cc22131 100644
--- a/src/openvpn/ssl.h
+++ b/src/openvpn/ssl.h
@@ -450,6 +450,8 @@  void ssl_purge_auth(const bool auth_user_pass_only);
 
 void ssl_set_auth_token(const char *token);
 
+void ssl_set_auth_token_user(const char *username);
+
 bool ssl_clean_auth_token(void);
 
 #ifdef ENABLE_MANAGEMENT