[Openvpn-devel] [S] Change in openvpn[master]: Use USER_PASS_LEN instead of TLS_USERNAME_LEN for override-username

[Openvpn-devel] [S] Change in openvpn[master]: Use USER_PASS_LEN instead of TLS_USERNAME_LEN for override-username

[plaisthos (Code Review)]

[-- Attachment #1: Type: text/plain, Size: 5154 bytes --]

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/+/913?usp=email

to review the following change.


Change subject: Use USER_PASS_LEN instead of TLS_USERNAME_LEN for override-username
......................................................................

Use USER_PASS_LEN instead of TLS_USERNAME_LEN for override-username

Change-Id: I1c2c050dd160746a0f8d9c234abe1e258bc8e48d
---
M src/openvpn/multi.c
M src/openvpn/options.c
M src/openvpn/ssl_verify.c
M src/openvpn/ssl_verify.h
4 files changed, 39 insertions(+), 7 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/13/913/1

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index a673ec1..a2d3fd1 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -2705,6 +2705,12 @@
     if (!multi->locked_original_username
         && strcmp(multi->locked_username, options->override_username) != 0)
     {
+        /* Check if the username length is acceptable */
+        if (!ssl_verify_username_length(session, options->override_username))
+        {
+            return false;
+        }
+
         multi->locked_original_username = multi->locked_username;
         multi->locked_username = strdup(options->override_username);

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index ab56609..f89fc7d 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -7875,10 +7875,10 @@
     else if (streq(p[0], "override-username") && p[1] && !p[2])
     {
         VERIFY_PERMISSION(OPT_P_INSTANCE);
-        if (strlen(p[1]) > TLS_USERNAME_LEN)
+        if (strlen(p[1]) > USER_PASS_LEN)
         {
             msg(msglevel, "override-username exceeds the maximum length of %d "
-                "characters", TLS_USERNAME_LEN);
+                "characters", USER_PASS_LEN);

             /* disable the connection since ignoring the request to
              * set another username might cause serious problems */
diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index 5f8f1d3..dbd2226 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -1568,6 +1568,21 @@
     }
 }

+bool
+ssl_verify_username_length(struct tls_session *session, const char *username)
+{
+    if ((session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME)
+        && strlen(username) > TLS_USERNAME_LEN)
+    {
+        msg(D_TLS_ERRORS,
+            "TLS Auth Error: --username-as-common name specified and "
+            "username is longer than the maximum permitted Common Name "
+            "length of %d characters", TLS_USERNAME_LEN);
+        return false;
+    }
+    return true;
+}
+
 /**
  * Main username/password verification entry point
  *
@@ -1689,15 +1704,12 @@
     }

     /* check sizing of username if it will become our common name */
-    if ((session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME)
-        && strlen(up->username)>TLS_USERNAME_LEN)
+    if (ssl_verify_username_length(session, up->username))
     {
-        msg(D_TLS_ERRORS,
-            "TLS Auth Error: --username-as-common name specified and username is longer than the maximum permitted Common Name length of %d characters",
-            TLS_USERNAME_LEN);
         plugin_status = OPENVPN_PLUGIN_FUNC_ERROR;
         script_status = OPENVPN_PLUGIN_FUNC_ERROR;
     }
+
     /* auth succeeded? */
     bool plugin_ok = plugin_status == OPENVPN_PLUGIN_FUNC_SUCCESS
                      || plugin_status == OPENVPN_PLUGIN_FUNC_DEFERRED;
diff --git a/src/openvpn/ssl_verify.h b/src/openvpn/ssl_verify.h
index eba3832..7a4d44a 100644
--- a/src/openvpn/ssl_verify.h
+++ b/src/openvpn/ssl_verify.h
@@ -192,6 +192,20 @@
                       struct tls_session *session);


+/**
+ * Checks if the username length is valid to use.  This checks when
+ * username-as-common-name is active if the username is shorter than
+ * the maximum TLS common name length (64).
+ *
+ * It will also display an error message if the name is too long
+ *
+ * @param session       current TLS session
+ * @param username      username to check
+ * @return              true if name is under limit or username-as-common-name
+ *                      is not active
+ */
+bool ssl_verify_username_length(struct tls_session *session,
+                                const char *username);

 /**
  * Runs the --client-crresponse script if one is defined.

--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/913?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I1c2c050dd160746a0f8d9c234abe1e258bc8e48d
Gerrit-Change-Number: 913
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos <arne-openvpn@...1227...>
Gerrit-Reviewer: flichtenheld <frank@...2641...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: flichtenheld <frank@...2641...>
Gerrit-MessageType: newchange

[-- Attachment #2: Type: text/html, Size: 9334 bytes --]

[Openvpn-devel] [S] Change in openvpn[master]: Use USER_PASS_LEN instead of TLS_USERNAME_LEN for override-username

[plaisthos (Code Review)]

[-- Attachment #1: Type: text/plain, Size: 5547 bytes --]

Attention is currently required from: flichtenheld.

Hello flichtenheld, 

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#2).


Change subject: Use USER_PASS_LEN instead of TLS_USERNAME_LEN for override-username
......................................................................

Use USER_PASS_LEN instead of TLS_USERNAME_LEN for override-username

Currently override-username is artificially restricted to the length of
TLS common-name for the corner case of using username-as-common-name, which
we explicitly do not recommend.

Do away with that limitation and only error out on longer usernames when
username-as-common-name is actually in effect.

Change-Id: I1c2c050dd160746a0f8d9c234abe1e258bc8e48d
Signed-off-by: Arne Schwabe <arne@...1227...>
---
M src/openvpn/multi.c
M src/openvpn/options.c
M src/openvpn/ssl_verify.c
M src/openvpn/ssl_verify.h
4 files changed, 42 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/13/913/2

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index a673ec1..a2d3fd1 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -2705,6 +2705,12 @@
     if (!multi->locked_original_username
         && strcmp(multi->locked_username, options->override_username) != 0)
     {
+        /* Check if the username length is acceptable */
+        if (!ssl_verify_username_length(session, options->override_username))
+        {
+            return false;
+        }
+
         multi->locked_original_username = multi->locked_username;
         multi->locked_username = strdup(options->override_username);
 
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index ab56609..f89fc7d 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -7875,10 +7875,10 @@
     else if (streq(p[0], "override-username") && p[1] && !p[2])
     {
         VERIFY_PERMISSION(OPT_P_INSTANCE);
-        if (strlen(p[1]) > TLS_USERNAME_LEN)
+        if (strlen(p[1]) > USER_PASS_LEN)
         {
             msg(msglevel, "override-username exceeds the maximum length of %d "
-                "characters", TLS_USERNAME_LEN);
+                "characters", USER_PASS_LEN);

             /* disable the connection since ignoring the request to
              * set another username might cause serious problems */
diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index 5f8f1d3..0defcbf 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -1568,6 +1568,24 @@
     }
 }

+bool
+ssl_verify_username_length(struct tls_session *session, const char *username)
+{
+    if ((session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME)
+        && strlen(username) > TLS_USERNAME_LEN)
+    {
+        msg(D_TLS_ERRORS,
+            "TLS Auth Error: --username-as-common name specified and "
+            "username is longer than the maximum permitted Common Name "
+            "length of %d characters", TLS_USERNAME_LEN);
+        return false;
+    }
+    else
+    {
+        return true;
+    }
+}
+
 /**
  * Main username/password verification entry point
  *
@@ -1689,15 +1707,12 @@
     }

     /* check sizing of username if it will become our common name */
-    if ((session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME)
-        && strlen(up->username)>TLS_USERNAME_LEN)
+    if (ssl_verify_username_length(session, up->username))
     {
-        msg(D_TLS_ERRORS,
-            "TLS Auth Error: --username-as-common name specified and username is longer than the maximum permitted Common Name length of %d characters",
-            TLS_USERNAME_LEN);
         plugin_status = OPENVPN_PLUGIN_FUNC_ERROR;
         script_status = OPENVPN_PLUGIN_FUNC_ERROR;
     }
+
     /* auth succeeded? */
     bool plugin_ok = plugin_status == OPENVPN_PLUGIN_FUNC_SUCCESS
                      || plugin_status == OPENVPN_PLUGIN_FUNC_DEFERRED;
diff --git a/src/openvpn/ssl_verify.h b/src/openvpn/ssl_verify.h
index eba3832..7a4d44a 100644
--- a/src/openvpn/ssl_verify.h
+++ b/src/openvpn/ssl_verify.h
@@ -192,6 +192,20 @@
                       struct tls_session *session);


+/**
+ * Checks if the username length is valid to use.  This checks when
+ * username-as-common-name is active if the username is shorter than
+ * the maximum TLS common name length (64).
+ *
+ * It will also display an error message if the name is too long
+ *
+ * @param session       current TLS session
+ * @param username      username to check
+ * @return              true if name is under limit or username-as-common-name
+ *                      is not active
+ */
+bool ssl_verify_username_length(struct tls_session *session,
+                                const char *username);

 /**
  * Runs the --client-crresponse script if one is defined.

--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/913?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I1c2c050dd160746a0f8d9c234abe1e258bc8e48d
Gerrit-Change-Number: 913
Gerrit-PatchSet: 2
Gerrit-Owner: plaisthos <arne-openvpn@...1227...>
Gerrit-Reviewer: flichtenheld <frank@...2641...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: flichtenheld <frank@...2641...>
Gerrit-MessageType: newpatchset

[-- Attachment #2: Type: text/html, Size: 9887 bytes --]

[Openvpn-devel] [S] Change in openvpn[master]: Use USER_PASS_LEN instead of TLS_USERNAME_LEN for override-username

[flichtenheld (Code Review)]

[-- Attachment #1: Type: text/plain, Size: 1243 bytes --]

Attention is currently required from: plaisthos.

flichtenheld has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/913?usp=email )

Change subject: Use USER_PASS_LEN instead of TLS_USERNAME_LEN for override-username
......................................................................


Patch Set 2: Code-Review-2

(1 comment)

File src/openvpn/ssl_verify.c:

http://gerrit.openvpn.net/c/openvpn/+/913/comment/471048b9_b3cd29b2 :
PS2, Line 1710:     if (ssl_verify_username_length(session, up->username))
Missing "not"?



--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/913?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I1c2c050dd160746a0f8d9c234abe1e258bc8e48d
Gerrit-Change-Number: 913
Gerrit-PatchSet: 2
Gerrit-Owner: plaisthos <arne-openvpn@...1227...>
Gerrit-Reviewer: flichtenheld <frank@...2641...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: plaisthos <arne-openvpn@...1227...>
Gerrit-Comment-Date: Mon, 24 Mar 2025 13:38:56 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[-- Attachment #2: Type: text/html, Size: 2359 bytes --]

[Openvpn-devel] [S] Change in openvpn[master]: Use USER_PASS_LEN instead of TLS_USERNAME_LEN for override-username

[plaisthos (Code Review)]

[-- Attachment #1: Type: text/plain, Size: 1294 bytes --]

Attention is currently required from: flichtenheld.

plaisthos has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/913?usp=email )

Change subject: Use USER_PASS_LEN instead of TLS_USERNAME_LEN for override-username
......................................................................


Patch Set 2:

(1 comment)

File src/openvpn/ssl_verify.c:

http://gerrit.openvpn.net/c/openvpn/+/913/comment/973c8450_1d433ea8 :
PS2, Line 1710:     if (ssl_verify_username_length(session, up->username))
> Missing "not"?
Acknowledged



--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/913?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I1c2c050dd160746a0f8d9c234abe1e258bc8e48d
Gerrit-Change-Number: 913
Gerrit-PatchSet: 2
Gerrit-Owner: plaisthos <arne-openvpn@...1227...>
Gerrit-Reviewer: flichtenheld <frank@...2641...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: flichtenheld <frank@...2641...>
Gerrit-Comment-Date: Mon, 24 Mar 2025 13:42:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: flichtenheld <frank@...2641...>
Gerrit-MessageType: comment

[-- Attachment #2: Type: text/html, Size: 2418 bytes --]

[Openvpn-devel] [S] Change in openvpn[master]: Use USER_PASS_LEN instead of TLS_USERNAME_LEN for override-username

[plaisthos (Code Review)]

[-- Attachment #1: Type: text/plain, Size: 5559 bytes --]

Attention is currently required from: flichtenheld.

Hello flichtenheld, 

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#3).


Change subject: Use USER_PASS_LEN instead of TLS_USERNAME_LEN for override-username
......................................................................

Use USER_PASS_LEN instead of TLS_USERNAME_LEN for override-username

Currently override-username is artificially restricted to the length of
TLS common-name (64) for the corner case of using username-as-common-name,
which we explicitly do not recommend to use.

Do away with that limitation and only error out on longer usernames when
username-as-common-name is actually in effect.

Change-Id: I1c2c050dd160746a0f8d9c234abe1e258bc8e48d
Signed-off-by: Arne Schwabe <arne@...1227...>
---
M src/openvpn/multi.c
M src/openvpn/options.c
M src/openvpn/ssl_verify.c
M src/openvpn/ssl_verify.h
4 files changed, 42 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/13/913/3

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index a673ec1..a2d3fd1 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -2705,6 +2705,12 @@
     if (!multi->locked_original_username
         && strcmp(multi->locked_username, options->override_username) != 0)
     {
+        /* Check if the username length is acceptable */
+        if (!ssl_verify_username_length(session, options->override_username))
+        {
+            return false;
+        }
+
         multi->locked_original_username = multi->locked_username;
         multi->locked_username = strdup(options->override_username);

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index ab56609..f89fc7d 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -7875,10 +7875,10 @@
     else if (streq(p[0], "override-username") && p[1] && !p[2])
     {
         VERIFY_PERMISSION(OPT_P_INSTANCE);
-        if (strlen(p[1]) > TLS_USERNAME_LEN)
+        if (strlen(p[1]) > USER_PASS_LEN)
         {
             msg(msglevel, "override-username exceeds the maximum length of %d "
-                "characters", TLS_USERNAME_LEN);
+                "characters", USER_PASS_LEN);

             /* disable the connection since ignoring the request to
              * set another username might cause serious problems */
diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index 5f8f1d3..d2cc3d1 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -1568,6 +1568,24 @@
     }
 }

+bool
+ssl_verify_username_length(struct tls_session *session, const char *username)
+{
+    if ((session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME)
+        && strlen(username) > TLS_USERNAME_LEN)
+    {
+        msg(D_TLS_ERRORS,
+            "TLS Auth Error: --username-as-common name specified and "
+            "username is longer than the maximum permitted Common Name "
+            "length of %d characters", TLS_USERNAME_LEN);
+        return false;
+    }
+    else
+    {
+        return true;
+    }
+}
+
 /**
  * Main username/password verification entry point
  *
@@ -1689,15 +1707,12 @@
     }

     /* check sizing of username if it will become our common name */
-    if ((session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME)
-        && strlen(up->username)>TLS_USERNAME_LEN)
+    if (!ssl_verify_username_length(session, up->username))
     {
-        msg(D_TLS_ERRORS,
-            "TLS Auth Error: --username-as-common name specified and username is longer than the maximum permitted Common Name length of %d characters",
-            TLS_USERNAME_LEN);
         plugin_status = OPENVPN_PLUGIN_FUNC_ERROR;
         script_status = OPENVPN_PLUGIN_FUNC_ERROR;
     }
+
     /* auth succeeded? */
     bool plugin_ok = plugin_status == OPENVPN_PLUGIN_FUNC_SUCCESS
                      || plugin_status == OPENVPN_PLUGIN_FUNC_DEFERRED;
diff --git a/src/openvpn/ssl_verify.h b/src/openvpn/ssl_verify.h
index eba3832..7a4d44a 100644
--- a/src/openvpn/ssl_verify.h
+++ b/src/openvpn/ssl_verify.h
@@ -192,6 +192,20 @@
                       struct tls_session *session);


+/**
+ * Checks if the username length is valid to use.  This checks when
+ * username-as-common-name is active if the username is shorter than
+ * the maximum TLS common name length (64).
+ *
+ * It will also display an error message if the name is too long
+ *
+ * @param session       current TLS session
+ * @param username      username to check
+ * @return              true if name is under limit or username-as-common-name
+ *                      is not active
+ */
+bool ssl_verify_username_length(struct tls_session *session,
+                                const char *username);

 /**
  * Runs the --client-crresponse script if one is defined.

--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/913?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I1c2c050dd160746a0f8d9c234abe1e258bc8e48d
Gerrit-Change-Number: 913
Gerrit-PatchSet: 3
Gerrit-Owner: plaisthos <arne-openvpn@...1227...>
Gerrit-Reviewer: flichtenheld <frank@...2641...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: flichtenheld <frank@...2641...>
Gerrit-MessageType: newpatchset

[-- Attachment #2: Type: text/html, Size: 9900 bytes --]

[Openvpn-devel] [S] Change in openvpn[master]: Use USER_PASS_LEN instead of TLS_USERNAME_LEN for override-username

[flichtenheld (Code Review)]

[-- Attachment #1: Type: text/plain, Size: 918 bytes --]

flichtenheld has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/913?usp=email )

Change subject: Use USER_PASS_LEN instead of TLS_USERNAME_LEN for override-username
......................................................................


Patch Set 3: -Code-Review


--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/913?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I1c2c050dd160746a0f8d9c234abe1e258bc8e48d
Gerrit-Change-Number: 913
Gerrit-PatchSet: 3
Gerrit-Owner: plaisthos <arne-openvpn@...1227...>
Gerrit-Reviewer: flichtenheld <frank@...2641...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Comment-Date: Mon, 24 Mar 2025 14:03:49 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[-- Attachment #2: Type: text/html, Size: 1660 bytes --]

[Openvpn-devel] [S] Change in openvpn[master]: Use USER_PASS_LEN instead of TLS_USERNAME_LEN for override-username

[cron2 (Code Review)]

[-- Attachment #1: Type: text/plain, Size: 1061 bytes --]

Attention is currently required from: plaisthos.

cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/913?usp=email )

Change subject: Use USER_PASS_LEN instead of TLS_USERNAME_LEN for override-username
......................................................................


Patch Set 3: Code-Review+2


--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/913?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I1c2c050dd160746a0f8d9c234abe1e258bc8e48d
Gerrit-Change-Number: 913
Gerrit-PatchSet: 3
Gerrit-Owner: plaisthos <arne-openvpn@...1227...>
Gerrit-Reviewer: cron2 <gert@...1296...>
Gerrit-Reviewer: flichtenheld <frank@...2641...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: plaisthos <arne-openvpn@...1227...>
Gerrit-Comment-Date: Wed, 02 Apr 2025 13:45:28 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[-- Attachment #2: Type: text/html, Size: 1909 bytes --]

[Openvpn-devel] [PATCH v3] Use USER_PASS_LEN instead of TLS_USERNAME_LEN for override-username

[Gert Doering]

From: Arne Schwabe <arne@...1227...>

Currently override-username is artificially restricted to the length of
TLS common-name (64) for the corner case of using username-as-common-name,
which we explicitly do not recommend to use.

Do away with that limitation and only error out on longer usernames when
username-as-common-name is actually in effect.

Change-Id: I1c2c050dd160746a0f8d9c234abe1e258bc8e48d
Signed-off-by: Arne Schwabe <arne@...1227...>
Acked-by: Gert Doering <gert@...1296...>
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.

Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/913
This mail reflects revision 3 of this Change.

Acked-by according to Gerrit (reflected above):
Gert Doering <gert@...1296...>

        
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index a673ec1..a2d3fd1 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -2705,6 +2705,12 @@
     if (!multi->locked_original_username
         && strcmp(multi->locked_username, options->override_username) != 0)
     {
+        /* Check if the username length is acceptable */
+        if (!ssl_verify_username_length(session, options->override_username))
+        {
+            return false;
+        }
+
         multi->locked_original_username = multi->locked_username;
         multi->locked_username = strdup(options->override_username);
 
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index ab56609..f89fc7d 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -7875,10 +7875,10 @@
     else if (streq(p[0], "override-username") && p[1] && !p[2])
     {
         VERIFY_PERMISSION(OPT_P_INSTANCE);
-        if (strlen(p[1]) > TLS_USERNAME_LEN)
+        if (strlen(p[1]) > USER_PASS_LEN)
         {
             msg(msglevel, "override-username exceeds the maximum length of %d "
-                "characters", TLS_USERNAME_LEN);
+                "characters", USER_PASS_LEN);
 
             /* disable the connection since ignoring the request to
              * set another username might cause serious problems */
diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index 5f8f1d3..d2cc3d1 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -1568,6 +1568,24 @@
     }
 }
 
+bool
+ssl_verify_username_length(struct tls_session *session, const char *username)
+{
+    if ((session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME)
+        && strlen(username) > TLS_USERNAME_LEN)
+    {
+        msg(D_TLS_ERRORS,
+            "TLS Auth Error: --username-as-common name specified and "
+            "username is longer than the maximum permitted Common Name "
+            "length of %d characters", TLS_USERNAME_LEN);
+        return false;
+    }
+    else
+    {
+        return true;
+    }
+}
+
 /**
  * Main username/password verification entry point
  *
@@ -1689,15 +1707,12 @@
     }
 
     /* check sizing of username if it will become our common name */
-    if ((session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME)
-        && strlen(up->username)>TLS_USERNAME_LEN)
+    if (!ssl_verify_username_length(session, up->username))
     {
-        msg(D_TLS_ERRORS,
-            "TLS Auth Error: --username-as-common name specified and username is longer than the maximum permitted Common Name length of %d characters",
-            TLS_USERNAME_LEN);
         plugin_status = OPENVPN_PLUGIN_FUNC_ERROR;
         script_status = OPENVPN_PLUGIN_FUNC_ERROR;
     }
+
     /* auth succeeded? */
     bool plugin_ok = plugin_status == OPENVPN_PLUGIN_FUNC_SUCCESS
                      || plugin_status == OPENVPN_PLUGIN_FUNC_DEFERRED;
diff --git a/src/openvpn/ssl_verify.h b/src/openvpn/ssl_verify.h
index eba3832..7a4d44a 100644
--- a/src/openvpn/ssl_verify.h
+++ b/src/openvpn/ssl_verify.h
@@ -192,6 +192,20 @@
                       struct tls_session *session);
 
 
+/**
+ * Checks if the username length is valid to use.  This checks when
+ * username-as-common-name is active if the username is shorter than
+ * the maximum TLS common name length (64).
+ *
+ * It will also display an error message if the name is too long
+ *
+ * @param session       current TLS session
+ * @param username      username to check
+ * @return              true if name is under limit or username-as-common-name
+ *                      is not active
+ */
+bool ssl_verify_username_length(struct tls_session *session,
+                                const char *username);
 
 /**
  * Runs the --client-crresponse script if one is defined.

[Openvpn-devel] [PATCH applied] Re: Use USER_PASS_LEN instead of TLS_USERNAME_LEN for override-username

[Gert Doering]

"Because it makes sense" :-) - haven't explicitly tested all the various
corner cases, but the important bits are quite obvious from stare-at-code.

ssl_verify_username_length() looks "new" but is just refactored from
existing in-function code for reuse.

Your patch has been applied to the master branch.

commit bb8f2e373cf5d6c4d4320694ddd14bb61d5756b3
Author: Arne Schwabe
Date:   Wed Apr 2 15:45:39 2025 +0200

     Use USER_PASS_LEN instead of TLS_USERNAME_LEN for override-username

     Signed-off-by: Arne Schwabe <arne@...1227...>
     Acked-by: Gert Doering <gert@...1296...>
     Message-Id: <20250402134546.3504-1-gert@...1296...>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg31323.html
     Signed-off-by: Gert Doering <gert@...1296...>


--
kind regards,

Gert Doering

[Openvpn-devel] [S] Change in openvpn[master]: Use USER_PASS_LEN instead of TLS_USERNAME_LEN for override-username

[cron2 (Code Review)]

[-- Attachment #1: Type: text/plain, Size: 5777 bytes --]

cron2 has uploaded a new patch set (#4) to the change originally created by plaisthos. ( http://gerrit.openvpn.net/c/openvpn/+/913?usp=email )

The following approvals got outdated and were removed:
Code-Review+2 by cron2


Change subject: Use USER_PASS_LEN instead of TLS_USERNAME_LEN for override-username
......................................................................

Use USER_PASS_LEN instead of TLS_USERNAME_LEN for override-username

Currently override-username is artificially restricted to the length of
TLS common-name (64) for the corner case of using username-as-common-name,
which we explicitly do not recommend to use.

Do away with that limitation and only error out on longer usernames when
username-as-common-name is actually in effect.

Change-Id: I1c2c050dd160746a0f8d9c234abe1e258bc8e48d
Signed-off-by: Arne Schwabe <arne@...1227...>
Acked-by: Gert Doering <gert@...1296...>
Message-Id: <20250402134546.3504-1-gert@...1296...>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg31323.html
Signed-off-by: Gert Doering <gert@...1296...>
---
M src/openvpn/multi.c
M src/openvpn/options.c
M src/openvpn/ssl_verify.c
M src/openvpn/ssl_verify.h
4 files changed, 42 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/13/913/4

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index a673ec1..a2d3fd1 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -2705,6 +2705,12 @@
     if (!multi->locked_original_username
         && strcmp(multi->locked_username, options->override_username) != 0)
     {
+        /* Check if the username length is acceptable */
+        if (!ssl_verify_username_length(session, options->override_username))
+        {
+            return false;
+        }
+
         multi->locked_original_username = multi->locked_username;
         multi->locked_username = strdup(options->override_username);

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 6605a42..96119c4 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -7880,10 +7880,10 @@
     else if (streq(p[0], "override-username") && p[1] && !p[2])
     {
         VERIFY_PERMISSION(OPT_P_INSTANCE);
-        if (strlen(p[1]) > TLS_USERNAME_LEN)
+        if (strlen(p[1]) > USER_PASS_LEN)
         {
             msg(msglevel, "override-username exceeds the maximum length of %d "
-                "characters", TLS_USERNAME_LEN);
+                "characters", USER_PASS_LEN);

             /* disable the connection since ignoring the request to
              * set another username might cause serious problems */
diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index 5f8f1d3..d2cc3d1 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -1568,6 +1568,24 @@
     }
 }

+bool
+ssl_verify_username_length(struct tls_session *session, const char *username)
+{
+    if ((session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME)
+        && strlen(username) > TLS_USERNAME_LEN)
+    {
+        msg(D_TLS_ERRORS,
+            "TLS Auth Error: --username-as-common name specified and "
+            "username is longer than the maximum permitted Common Name "
+            "length of %d characters", TLS_USERNAME_LEN);
+        return false;
+    }
+    else
+    {
+        return true;
+    }
+}
+
 /**
  * Main username/password verification entry point
  *
@@ -1689,15 +1707,12 @@
     }

     /* check sizing of username if it will become our common name */
-    if ((session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME)
-        && strlen(up->username)>TLS_USERNAME_LEN)
+    if (!ssl_verify_username_length(session, up->username))
     {
-        msg(D_TLS_ERRORS,
-            "TLS Auth Error: --username-as-common name specified and username is longer than the maximum permitted Common Name length of %d characters",
-            TLS_USERNAME_LEN);
         plugin_status = OPENVPN_PLUGIN_FUNC_ERROR;
         script_status = OPENVPN_PLUGIN_FUNC_ERROR;
     }
+
     /* auth succeeded? */
     bool plugin_ok = plugin_status == OPENVPN_PLUGIN_FUNC_SUCCESS
                      || plugin_status == OPENVPN_PLUGIN_FUNC_DEFERRED;
diff --git a/src/openvpn/ssl_verify.h b/src/openvpn/ssl_verify.h
index eba3832..7a4d44a 100644
--- a/src/openvpn/ssl_verify.h
+++ b/src/openvpn/ssl_verify.h
@@ -192,6 +192,20 @@
                       struct tls_session *session);


+/**
+ * Checks if the username length is valid to use.  This checks when
+ * username-as-common-name is active if the username is shorter than
+ * the maximum TLS common name length (64).
+ *
+ * It will also display an error message if the name is too long
+ *
+ * @param session       current TLS session
+ * @param username      username to check
+ * @return              true if name is under limit or username-as-common-name
+ *                      is not active
+ */
+bool ssl_verify_username_length(struct tls_session *session,
+                                const char *username);

 /**
  * Runs the --client-crresponse script if one is defined.

--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/913?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I1c2c050dd160746a0f8d9c234abe1e258bc8e48d
Gerrit-Change-Number: 913
Gerrit-PatchSet: 4
Gerrit-Owner: plaisthos <arne-openvpn@...1227...>
Gerrit-Reviewer: cron2 <gert@...1296...>
Gerrit-Reviewer: flichtenheld <frank@...2641...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-MessageType: newpatchset

[-- Attachment #2: Type: text/html, Size: 10189 bytes --]

[Openvpn-devel] [S] Change in openvpn[master]: Use USER_PASS_LEN instead of TLS_USERNAME_LEN for override-username

[cron2 (Code Review)]

[-- Attachment #1: Type: text/plain, Size: 5563 bytes --]

cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/913?usp=email )

Change subject: Use USER_PASS_LEN instead of TLS_USERNAME_LEN for override-username
......................................................................

Use USER_PASS_LEN instead of TLS_USERNAME_LEN for override-username

Currently override-username is artificially restricted to the length of
TLS common-name (64) for the corner case of using username-as-common-name,
which we explicitly do not recommend to use.

Do away with that limitation and only error out on longer usernames when
username-as-common-name is actually in effect.

Change-Id: I1c2c050dd160746a0f8d9c234abe1e258bc8e48d
Signed-off-by: Arne Schwabe <arne@...1227...>
Acked-by: Gert Doering <gert@...1296...>
Message-Id: <20250402134546.3504-1-gert@...1296...>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg31323.html
Signed-off-by: Gert Doering <gert@...1296...>
---
M src/openvpn/multi.c
M src/openvpn/options.c
M src/openvpn/ssl_verify.c
M src/openvpn/ssl_verify.h
4 files changed, 42 insertions(+), 7 deletions(-)




diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index a673ec1..a2d3fd1 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -2705,6 +2705,12 @@
     if (!multi->locked_original_username
         && strcmp(multi->locked_username, options->override_username) != 0)
     {
+        /* Check if the username length is acceptable */
+        if (!ssl_verify_username_length(session, options->override_username))
+        {
+            return false;
+        }
+
         multi->locked_original_username = multi->locked_username;
         multi->locked_username = strdup(options->override_username);

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 6605a42..96119c4 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -7880,10 +7880,10 @@
     else if (streq(p[0], "override-username") && p[1] && !p[2])
     {
         VERIFY_PERMISSION(OPT_P_INSTANCE);
-        if (strlen(p[1]) > TLS_USERNAME_LEN)
+        if (strlen(p[1]) > USER_PASS_LEN)
         {
             msg(msglevel, "override-username exceeds the maximum length of %d "
-                "characters", TLS_USERNAME_LEN);
+                "characters", USER_PASS_LEN);

             /* disable the connection since ignoring the request to
              * set another username might cause serious problems */
diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index 5f8f1d3..d2cc3d1 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -1568,6 +1568,24 @@
     }
 }

+bool
+ssl_verify_username_length(struct tls_session *session, const char *username)
+{
+    if ((session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME)
+        && strlen(username) > TLS_USERNAME_LEN)
+    {
+        msg(D_TLS_ERRORS,
+            "TLS Auth Error: --username-as-common name specified and "
+            "username is longer than the maximum permitted Common Name "
+            "length of %d characters", TLS_USERNAME_LEN);
+        return false;
+    }
+    else
+    {
+        return true;
+    }
+}
+
 /**
  * Main username/password verification entry point
  *
@@ -1689,15 +1707,12 @@
     }

     /* check sizing of username if it will become our common name */
-    if ((session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME)
-        && strlen(up->username)>TLS_USERNAME_LEN)
+    if (!ssl_verify_username_length(session, up->username))
     {
-        msg(D_TLS_ERRORS,
-            "TLS Auth Error: --username-as-common name specified and username is longer than the maximum permitted Common Name length of %d characters",
-            TLS_USERNAME_LEN);
         plugin_status = OPENVPN_PLUGIN_FUNC_ERROR;
         script_status = OPENVPN_PLUGIN_FUNC_ERROR;
     }
+
     /* auth succeeded? */
     bool plugin_ok = plugin_status == OPENVPN_PLUGIN_FUNC_SUCCESS
                      || plugin_status == OPENVPN_PLUGIN_FUNC_DEFERRED;
diff --git a/src/openvpn/ssl_verify.h b/src/openvpn/ssl_verify.h
index eba3832..7a4d44a 100644
--- a/src/openvpn/ssl_verify.h
+++ b/src/openvpn/ssl_verify.h
@@ -192,6 +192,20 @@
                       struct tls_session *session);


+/**
+ * Checks if the username length is valid to use.  This checks when
+ * username-as-common-name is active if the username is shorter than
+ * the maximum TLS common name length (64).
+ *
+ * It will also display an error message if the name is too long
+ *
+ * @param session       current TLS session
+ * @param username      username to check
+ * @return              true if name is under limit or username-as-common-name
+ *                      is not active
+ */
+bool ssl_verify_username_length(struct tls_session *session,
+                                const char *username);

 /**
  * Runs the --client-crresponse script if one is defined.

--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/913?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I1c2c050dd160746a0f8d9c234abe1e258bc8e48d
Gerrit-Change-Number: 913
Gerrit-PatchSet: 4
Gerrit-Owner: plaisthos <arne-openvpn@...1227...>
Gerrit-Reviewer: cron2 <gert@...1296...>
Gerrit-Reviewer: flichtenheld <frank@...2641...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-MessageType: merged

[-- Attachment #2: Type: text/html, Size: 9950 bytes --]