[Openvpn-devel,M] Change in openvpn[master]: Implement override-user

Message ID 232dbf2b8e18146cc35b60c0f93f1c9b4d022ef6-HTML@gerrit.openvpn.net
State New
Headers show
Series [Openvpn-devel,M] Change in openvpn[master]: Implement override-user | expand

Commit Message

plaisthos (Code Review) Jan. 20, 2025, 12:12 p.m. UTC
Attention is currently required from: flichtenheld.

Hello flichtenheld,

I'd like you to do a code review.
Please visit

    http://gerrit.openvpn.net/c/openvpn/+/872?usp=email

to review the following change.


Change subject: Implement override-user
......................................................................

Implement override-user

Change-Id: Ia4095518d5e4447992a2974e0d7a159d79ba6b6f
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
M Changes.rst
M doc/man-sections/server-options.rst
M src/openvpn/auth_token.c
M src/openvpn/multi.c
M src/openvpn/options.c
M src/openvpn/options.h
M src/openvpn/push.c
M src/openvpn/ssl.c
M src/openvpn/ssl_common.h
M src/openvpn/ssl_verify.c
M src/openvpn/ssl_verify.h
11 files changed, 126 insertions(+), 6 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/72/872/1

Patch

diff --git a/Changes.rst b/Changes.rst
index 16ae6fc..d7def7c 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -36,6 +36,10 @@ 
     add an allowed cipher without having to spell out the default ciphers.
 
 
+Allow overriding username with ``--override-username``
+    This is intended to allow using auth-gen-token in scenarios where the
+    client use certificates and multi-factor authentication.
+
 Deprecated features
 -------------------
 ``secret`` support has been removed by default.
diff --git a/doc/man-sections/server-options.rst b/doc/man-sections/server-options.rst
index 3fe9862..2b2c262 100644
--- a/doc/man-sections/server-options.rst
+++ b/doc/man-sections/server-options.rst
@@ -670,6 +670,31 @@ 
   AND the ``--auth-user-pass-verify`` script will need to succeed in order
   for a client to be authenticated and accepted onto the VPN.
 
+--override-username
+  Sets the username of a connection the specified username.  This username
+  will also be used by the by ``--auth-gen-token``. However, the overridden
+  username comes only into effect *after* the ``--client-connect``,
+  ``--client-config-dir`` and the ``--auth-user-pass-verify`` script have
+  been run.
+
+  Also ``username-as-common-name`` will use the client provided username
+  as common-name. It is recommended to avoid the use of the
+  ``--override-username`` option if  the option ``username-as-common-name``
+  is being used.
+
+  The changed username will be picked up by the status output and also by
+  the the ``--auth-gen-token`` option. It will also be pushed to the client
+  using ``--auth-token-user``.
+
+  Special care has to be taken that the initial username of the client is
+  correctly handled with these options to avoid authentication/authorisation
+  bypasses.
+
+  This option is mainly intended for use cases that use certificates and
+  multi factor authentication and therefore do not provide a username that
+  can be used for ``--auth-gen-token`` to allow providing a username in
+  these scenarios.
+
 --vlan-tagging
   Server-only option. Turns the OpenVPN server instance into a switch that
   understands VLAN-tagging, based on IEEE 802.1Q.
diff --git a/src/openvpn/auth_token.c b/src/openvpn/auth_token.c
index 3cf55e8..312474c 100644
--- a/src/openvpn/auth_token.c
+++ b/src/openvpn/auth_token.c
@@ -330,10 +330,19 @@ 
     timestamp_initial = ntohll(timestamp_initial);
 
     hmac_ctx_t *ctx = multi->opt.auth_token_key.hmac;
+
     if (check_hmac_token(ctx, b64decoded, up->username))
     {
         ret |= AUTH_TOKEN_HMAC_OK;
     }
+    else if (multi->locked_original_username && multi->locked_username
+             && check_hmac_token(ctx, b64decoded, multi->locked_username))
+    {
+        /* if the username has been overridden, check with the overridden
+         * username and ignore the sent username in case the client does not
+         * support auth-token-user */
+        ret |= AUTH_TOKEN_HMAC_OK;
+    }
     else if (check_hmac_token(ctx, b64decoded, ""))
     {
         ret |= AUTH_TOKEN_HMAC_OK;
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index f426b46..03e6b6e 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -42,7 +42,9 @@ 
 #include "ssl_verify.h"
 #include "ssl_ncp.h"
 #include "vlan.h"
+#include "auth_token.h"
 #include <inttypes.h>
+#include <string.h>
 
 #include "memdbg.h"
 
@@ -2663,6 +2665,41 @@ 
     NULL,
 };
 
+/**
+ *
+ * @param mi
+ */
+static void
+override_locked_username(struct multi_instance *mi)
+{
+    struct tls_multi *multi = mi->context.c2.tls_multi;
+
+    struct options *options = &mi->context.options;
+
+    if (!multi->locked_original_username
+        && strcmp(multi->locked_username, options->override_username) != 0)
+    {
+        multi->locked_original_username = multi->locked_username;
+        multi->locked_username = strdup(options->override_username);
+
+        /* Regenerate the auth-token if enabled */
+        if (multi->auth_token_initial)
+        {
+            struct user_pass up;
+            CLEAR(up);
+            strncpynt(up.username, multi->locked_username,
+                      sizeof(up.username));
+
+            generate_auth_token(&up, multi);
+        }
+
+        msg(D_MULTI_LOW, "MULTI: Note, override-username changes username "
+            "from '%s' to '%s'",
+            multi->locked_original_username,
+            multi->locked_username);
+
+    }
+}
 /*
  * Called as soon as the SSL/TLS connection is authenticated.
  *
@@ -2766,6 +2803,11 @@ 
         (*cur_handler_index)++;
     }
 
+    if (mi->context.options.override_username)
+    {
+        override_locked_username(mi);
+    }
+
     /* Check if we have forbidding options in the current mode */
     if (dco_enabled(&mi->context.options)
         && !dco_check_option(D_MULTI_ERRORS, &mi->context.options))
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 3ef4d78..bf966f3 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -7847,6 +7847,23 @@ 
         VERIFY_PERMISSION(OPT_P_INSTANCE);
         options->disable = true;
     }
+    else if (streq(p[0], "override-username") && p[1] && !p[2])
+    {
+        VERIFY_PERMISSION(OPT_P_INSTANCE);
+        if (strlen(p[1]) > TLS_USERNAME_LEN)
+        {
+            msg(msglevel, "override-username exceeds the maximum length of %d "
+                "characters", TLS_USERNAME_LEN);
+
+            /* disable the connection since ignoring the request to
+             * set another username might serious problems */
+            options->disable = true;
+        }
+        else
+        {
+            options->override_username = p[1];
+        }
+    }
     else if (streq(p[0], "tcp-nodelay") && !p[1])
     {
         VERIFY_PERMISSION(OPT_P_GENERAL);
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index 55f12dd..274f282 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -492,6 +492,7 @@ 
     const char *client_config_dir;
     bool ccd_exclusive;
     bool disable;
+    const char *override_username;
     int n_bcast_buf;
     int tcp_queue_limit;
     struct iroute *iroutes;
diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index a7cd3bf..5ccd7d8 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -595,9 +595,19 @@ 
      */
     if (tls_multi->auth_token)
     {
-        push_option_fmt(gc, push_list, M_USAGE,
-                        "auth-token %s",
+        push_option_fmt(gc, push_list, M_USAGE, "auth-token %s",
                         tls_multi->auth_token);
+
+        char *base64user;
+        int ret = openvpn_base64_encode(tls_multi->locked_username,
+                                        (int)strlen(tls_multi->locked_username),
+                                        &base64user);
+        if (ret < USER_PASS_LEN && ret > 0)
+        {
+            push_option_fmt(gc, push_list, M_USAGE, "auth-token-user %s",
+                            base64user);
+        }
+        free(base64user);
     }
 }
 
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index e4a7b57..e548299 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1260,6 +1260,7 @@ 
     free(multi->peer_info);
     free(multi->locked_cn);
     free(multi->locked_username);
+    free(multi->locked_original_username);
 
     cert_hash_free(multi->locked_cert_hash_set);
 
diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
index e092472..79d0575 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -625,7 +625,15 @@ 
      * Our locked common name, username, and cert hashes (cannot change during the life of this tls_multi object)
      */
     char *locked_cn;
+
+    /** The locked username is the username that the client used for initial
+     * authentication */
     char *locked_username;
+
+    /** The username that client initial used before being overrriden by
+     * by override-user */
+    char *locked_original_username;
+
     struct cert_hash_set *locked_cert_hash_set;
 
     /** Time of last when we updated the cached state of
diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index e7d7ed6..158fe19 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -48,9 +48,6 @@ 
 #include "push.h"
 #include "ssl_util.h"
 
-/** Maximum length of common name */
-#define TLS_USERNAME_LEN 64
-
 static void
 string_mod_remap_name(char *str)
 {
@@ -153,7 +150,10 @@ 
 {
     if (multi->locked_username)
     {
-        if (strcmp(username, multi->locked_username) != 0)
+        /* If the username has been overridden, we accept both the original
+         * username and the changed username */
+        if (strcmp(username, multi->locked_username) != 0
+            &&  (!multi->locked_original_username || strcmp(username, multi->locked_original_username) != 0))
         {
             msg(D_TLS_ERRORS, "TLS Auth Error: username attempted to change from '%s' to '%s' -- tunnel disabled",
                 multi->locked_username,
diff --git a/src/openvpn/ssl_verify.h b/src/openvpn/ssl_verify.h
index cd2ec24..77e814a 100644
--- a/src/openvpn/ssl_verify.h
+++ b/src/openvpn/ssl_verify.h
@@ -51,6 +51,9 @@ 
 /** Maximum certificate depth we will allow */
 #define MAX_CERT_DEPTH 16
 
+/** Maximum length of common name */
+#define TLS_USERNAME_LEN 64
+
 /** Structure containing the hash for a single certificate */
 struct cert_hash {
     unsigned char sha256_hash[256/8];