[Openvpn-devel] [M] Change in openvpn[master]: Add new helpers to handle key exchange (S_SENT_KEY/S_START) with larg...

[Openvpn-devel] [M] Change in openvpn[master]: Add new helpers to handle key exchange (S_SENT_KEY/S_START) with larg...

[Bluca (Code Review)]

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

Attention is currently required from: plaisthos.

Hello plaisthos,

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

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

to review the following change.


Change subject: Add new helpers to handle key exchange (S_SENT_KEY/S_START) with large passwords
......................................................................

Add new helpers to handle key exchange (S_SENT_KEY/S_START) with large passwords

The current key exchange uses an intermediate buffer hardcoded
at 2048 bytes, that cannot handle anything longer, as it gets
silently truncated.

This breaks using JIT use-once tokens for authentications, that
are becoming common in enterprise devices to reduce attack
surfaces, as the tokens are typically long JWT encoded strings.

Add new helpers that, instead of doing a single 2048 bytes
read/write from/to the TLS layer, reads/writes all the available
data when doing the key method 2 exchange.
Other stages of the protocol are unaffected. Note that this is
an intermediary buffer, the TLS layer is already handling
fragmented/reassembled frames appropriately, so there is no
on-the-wire difference, aside from letting the client send more
data.

Older servers will simply continue to truncate passwords longer
than the existing limit, as they are doing currently, so no
incompatible changes, behaviour stays the same and current valid
passwords are still valid when the new client uses them.
When an old client talks to a new server, likewise there are no
changes, as the previous clients have the embedded limit, so all
existing passwords will continue to work.

Tested combinations:

old client <--> new server
new client <--> old server
new client <--> new server

With the last combination, a large password can be used successfully.
With either an old server or old client, existing limits apply and
a clear error is shown in case one tries a password longer than the
existing limits.

Change-Id: I055c64ca8b23066e70eea7d7deddfb14f5354c5f
Signed-off-by: Luca Boccassi <luca.boccassi@...277...>
---
M src/openvpn/common.h
M src/openvpn/ssl.c
M src/openvpn/ssl_common.h
3 files changed, 116 insertions(+), 17 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/22/1622/1

diff --git a/src/openvpn/common.h b/src/openvpn/common.h
index aa7b721..b67a6f4 100644
--- a/src/openvpn/common.h
+++ b/src/openvpn/common.h
@@ -69,6 +69,16 @@
  */
 #define TLS_CHANNEL_BUF_SIZE 2048

+/*
+ * Buffer size for key method 2 exchange data. This needs to be
+ * large enough to hold the key source material, options string,
+ * username, password and peer info. When USER_PASS_LEN is larger
+ * than the default TLS channel size (e.g. with ENABLE_PKCS11),
+ * we need a larger buffer. The key exchange write and read paths
+ * handle this by doing multiple TLS writes/reads as needed.
+ */
+#define KEY_METHOD_BUF_SIZE (TLS_CHANNEL_BUF_SIZE + 2 * USER_PASS_LEN)
+
 /* TLS control buffer minimum size
  *
  * A control frame might have IPv6 header (40 byte),
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 576157d..83bc46e 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -860,6 +860,8 @@
     ks->plaintext_read_buf = alloc_buf(TLS_CHANNEL_BUF_SIZE);
     ks->plaintext_write_buf = alloc_buf(TLS_CHANNEL_BUF_SIZE);
     ks->ack_write_buf = alloc_buf(BUF_SIZE(&session->opt->frame));
+    ks->key_method_send_buf = alloc_buf(KEY_METHOD_BUF_SIZE);
+    ks->key_method_recv_buf = alloc_buf(KEY_METHOD_BUF_SIZE);
     reliable_init(ks->send_reliable, BUF_SIZE(&session->opt->frame),
                   session->opt->frame.buf.headroom, TLS_RELIABLE_N_SEND_BUFFERS,
                   ks->key_id ? false : session->opt->xmit_hold);
@@ -915,6 +917,8 @@
     free_buf(&ks->plaintext_read_buf);
     free_buf(&ks->plaintext_write_buf);
     free_buf(&ks->ack_write_buf);
+    free_buf(&ks->key_method_send_buf);
+    free_buf(&ks->key_method_recv_buf);
     buffer_list_free(ks->paybuf);

     reliable_free(ks->send_reliable);
@@ -2624,6 +2628,54 @@
     return true;
 }

+/**
+ * Read all currently available plaintext from the TLS object into buf,
+ * doing multiple reads if necessary. This is used for key method 2
+ * exchange data which may exceed a single TLS_CHANNEL_BUF_SIZE read
+ * when long passwords/tokens are used.
+ */
+static bool
+read_incoming_tls_plaintext_multi(struct key_state *ks, struct buffer *buf,
+                                  interval_t *wakeup, bool *continue_tls_process)
+{
+    struct buffer tmp = alloc_buf(TLS_CHANNEL_BUF_SIZE);
+
+    while (buf_forward_capacity(buf) > 0)
+    {
+        buf_init(&tmp, 0);
+        int status = key_state_read_plaintext(&ks->ks_ssl, &tmp);
+
+        update_time();
+        if (status == -1)
+        {
+            msg(D_TLS_ERRORS,
+                "TLS Error: TLS object -> incoming plaintext read error");
+            free_buf(&tmp);
+            return false;
+        }
+        if (status == 1)
+        {
+            if (!buf_write(buf, BPTR(&tmp), BLEN(&tmp)))
+            {
+                msg(D_TLS_ERRORS,
+                    "TLS Error: key exchange plaintext too large");
+                free_buf(&tmp);
+                return false;
+            }
+            *continue_tls_process = true;
+            *wakeup = 0;
+            dmsg(D_TLS_DEBUG, "TLS -> Incoming Plaintext (key exchange)");
+        }
+        else
+        {
+            break;
+        }
+    }
+
+    free_buf(&tmp);
+    return true;
+}
+
 static bool
 write_outgoing_tls_ciphertext(struct tls_session *session, bool *continue_tls_process)
 {
@@ -2836,22 +2888,40 @@
     }

     /* Read incoming plaintext from TLS object */
-    struct buffer *buf = &ks->plaintext_read_buf;
-    if (!buf->len)
+    bool in_key_exchange_read =
+        (ks->state == S_SENT_KEY && !session->opt->server)
+        || (ks->state == S_START && session->opt->server);
+
+    if (in_key_exchange_read)
     {
-        if (!read_incoming_tls_plaintext(ks, buf, wakeup, &continue_tls_process))
+        /* For key exchange, read all available plaintext into a buffer
+         * that is large enough for long passwords/tokens. Multiple reads
+         * are done to handle data exceeding TLS_CHANNEL_BUF_SIZE. */
+        if (!read_incoming_tls_plaintext_multi(ks, &ks->key_method_recv_buf,
+                                               wakeup, &continue_tls_process))
         {
             goto error;
         }
     }
+    else
+    {
+        struct buffer *buf = &ks->plaintext_read_buf;
+        if (!buf->len)
+        {
+            if (!read_incoming_tls_plaintext(ks, buf, wakeup, &continue_tls_process))
+            {
+                goto error;
+            }
+        }
+    }

-    /* Send Key */
-    buf = &ks->plaintext_write_buf;
-    if (!buf->len
+    /* Send Key -- use larger key_method_send_buf for assembly to support
+     * long passwords/tokens that exceed TLS_CHANNEL_BUF_SIZE */
+    if (!ks->key_method_send_buf.len
         && ((ks->state == S_START && !session->opt->server)
             || (ks->state == S_GOT_KEY && session->opt->server)))
     {
-        if (!key_method_2_write(buf, multi, session))
+        if (!key_method_2_write(&ks->key_method_send_buf, multi, session))
         {
             goto error;
         }
@@ -2861,13 +2931,12 @@
         ks->state = S_SENT_KEY;
     }

-    /* Receive Key */
-    buf = &ks->plaintext_read_buf;
-    if (buf->len
+    /* Receive Key -- use the larger key_method_recv_buf */
+    if (BLEN(&ks->key_method_recv_buf)
         && ((ks->state == S_SENT_KEY && !session->opt->server)
             || (ks->state == S_START && session->opt->server)))
     {
-        if (!key_method_2_read(buf, multi, session))
+        if (!key_method_2_read(&ks->key_method_recv_buf, multi, session))
         {
             goto error;
         }
@@ -2877,20 +2946,38 @@
         ks->state = S_GOT_KEY;
     }

-    /* Write outgoing plaintext to TLS object */
-    buf = &ks->plaintext_write_buf;
-    if (buf->len)
+    /* Write key exchange data to TLS object */
+    if (ks->key_method_send_buf.len)
     {
-        int status = key_state_write_plaintext(&ks->ks_ssl, buf);
+        int status = key_state_write_plaintext(&ks->ks_ssl, &ks->key_method_send_buf);
         if (status == -1)
         {
-            msg(D_TLS_ERRORS, "TLS ERROR: Outgoing Plaintext -> TLS object write error");
+            msg(D_TLS_ERRORS, "TLS ERROR: Key Method -> TLS object write error");
             goto error;
         }
         if (status == 1)
         {
             continue_tls_process = true;
-            dmsg(D_TLS_DEBUG, "Outgoing Plaintext -> TLS");
+            dmsg(D_TLS_DEBUG, "Key Method -> TLS");
+        }
+    }
+
+    /* Write outgoing plaintext to TLS object */
+    {
+        struct buffer *buf = &ks->plaintext_write_buf;
+        if (buf->len)
+        {
+            int status = key_state_write_plaintext(&ks->ks_ssl, buf);
+            if (status == -1)
+            {
+                msg(D_TLS_ERRORS, "TLS ERROR: Outgoing Plaintext -> TLS object write error");
+                goto error;
+            }
+            if (status == 1)
+            {
+                continue_tls_process = true;
+                dmsg(D_TLS_DEBUG, "Outgoing Plaintext -> TLS");
+            }
         }
     }
     if (!check_outgoing_ciphertext(ks, session, &continue_tls_process))
diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
index 6f310a5..6f3da11 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -241,6 +241,8 @@
     struct buffer plaintext_read_buf;
     struct buffer plaintext_write_buf;
     struct buffer ack_write_buf;
+    struct buffer key_method_send_buf; /* larger buffer for key method 2 write */
+    struct buffer key_method_recv_buf; /* larger buffer for key method 2 read */

     struct reliable *send_reliable; /* holds a copy of outgoing packets until ACK received */
     struct reliable *rec_reliable;  /* order incoming ciphertext packets before we pass to TLS */

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

Gerrit-MessageType: newchange
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I055c64ca8b23066e70eea7d7deddfb14f5354c5f
Gerrit-Change-Number: 1622
Gerrit-PatchSet: 1
Gerrit-Owner: Bluca <luca.boccassi@...277...>
Gerrit-Reviewer: plaisthos <arne-openvpn@...1227...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: plaisthos <arne-openvpn@...1227...>

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

[Openvpn-devel] [M] Change in openvpn[master]: Add new helpers to handle key exchange (S_SENT_KEY/S_START) with larg...

[Bluca (Code Review)]

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

Attention is currently required from: plaisthos, selvanair.

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

Change subject: Add new helpers to handle key exchange (S_SENT_KEY/S_START) with large passwords
......................................................................


Patch Set 1:

(1 comment)

Patchset:

PS1:
Here's an alternative proposal to what was discussed here:

https://sourceforge.net/p/openvpn/mailman/openvpn-devel/thread/20260315184337.1541272-1-luca.boccassi%40gmail.com/#msg59309369
https://sourceforge.net/p/openvpn/mailman/openvpn-devel/thread/20260316224531.315912-1-luca.boccassi%40gmail.com/#msg59309910

The goal is to allow clients to optionally use longer passwords, without impacting existing users. As far as I can tell, with testing too, this should not impact existing usage.

With this change and the management interface change that was merged, I am able to use the OpenVPN client to connect to AzureVPN using a JIT-generated use-once authentication token, which is typically ~3k in size.



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

Gerrit-MessageType: comment
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I055c64ca8b23066e70eea7d7deddfb14f5354c5f
Gerrit-Change-Number: 1622
Gerrit-PatchSet: 1
Gerrit-Owner: Bluca <luca.boccassi@...277...>
Gerrit-Reviewer: plaisthos <arne-openvpn@...1227...>
Gerrit-Reviewer: selvanair <selva.nair@...277...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: plaisthos <arne-openvpn@...1227...>
Gerrit-Attention: selvanair <selva.nair@...277...>
Gerrit-Comment-Date: Mon, 06 Apr 2026 11:50:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No

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

[Openvpn-devel] [M] Change in openvpn[master]: Add new helpers to handle key exchange (S_SENT_KEY/S_START) with larg...

[plaisthos (Code Review)]

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

Attention is currently required from: Bluca, selvanair.

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

Change subject: Add new helpers to handle key exchange (S_SENT_KEY/S_START) with large passwords
......................................................................


Patch Set 1: Code-Review-1

(1 comment)

Patchset:

PS1:
I think this is a very hacky approach to solve this problem. I think we can better than that and design a proper protocol extension that allows larger username and password.

We would be better to not doctor around the problem of broken framing but rather negotiate allowing record spliting and allow more dynamic frame sizes than the 2048. Then we can also have proper support for longer username and password.



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

Gerrit-MessageType: comment
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I055c64ca8b23066e70eea7d7deddfb14f5354c5f
Gerrit-Change-Number: 1622
Gerrit-PatchSet: 1
Gerrit-Owner: Bluca <luca.boccassi@...277...>
Gerrit-Reviewer: plaisthos <arne-openvpn@...1227...>
Gerrit-Reviewer: selvanair <selva.nair@...277...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: Bluca <luca.boccassi@...277...>
Gerrit-Attention: selvanair <selva.nair@...277...>
Gerrit-Comment-Date: Tue, 07 Apr 2026 00:48:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes

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

[Openvpn-devel] [M] Change in openvpn[master]: Add new helpers to handle key exchange (S_SENT_KEY/S_START) with larg...

[plaisthos (Code Review)]

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

Attention is currently required from: Bluca, selvanair.

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

Change subject: Add new helpers to handle key exchange (S_SENT_KEY/S_START) with large passwords
......................................................................


Patch Set 1:

(1 comment)

Patchset:

PS1:
> With the last combination, a large password can be used successfully.
With either an old server or old client, existing limits apply and
a clear error is shown in case one tries a password longer than the
existing limits.

The clear error message is a quite recent change https://github.com/OpenVPN/openvpn/commit/a7f80d402fb95df3c58a8fc5d12cdb8f39c37d3e

You get very weird errors and behaviour on OpenVPN versions older than this commit. So only 2.6.13 or newer will actually give a clear error message.



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

Gerrit-MessageType: comment
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I055c64ca8b23066e70eea7d7deddfb14f5354c5f
Gerrit-Change-Number: 1622
Gerrit-PatchSet: 1
Gerrit-Owner: Bluca <luca.boccassi@...277...>
Gerrit-Reviewer: plaisthos <arne-openvpn@...1227...>
Gerrit-Reviewer: selvanair <selva.nair@...277...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: Bluca <luca.boccassi@...277...>
Gerrit-Attention: selvanair <selva.nair@...277...>
Gerrit-Comment-Date: Tue, 07 Apr 2026 01:00:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No

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

[Openvpn-devel] [M] Change in openvpn[master]: Add new helpers to handle key exchange (S_SENT_KEY/S_START) with larg...

[Bluca (Code Review)]

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

Attention is currently required from: plaisthos, selvanair.

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

Change subject: Add new helpers to handle key exchange (S_SENT_KEY/S_START) with large passwords
......................................................................


Patch Set 1:

(2 comments)

Patchset:

PS1:
> > With the last combination, a large password can be used successfully. […]
Sure, but that's 2 years old and included in LTS distros, eg. Ubuntu 24.04 and Debian 13: https://repology.org/project/openvpn/versions
It seems very unlikely that older, pre-existing deployments would suddenly start changing existing configuration and attempting to use longer passwords.


PS1:
> I think this is a very hacky approach to solve this problem. […]
Sorry, I don't really follow. This is not a TLS framing issue - the TLS layer (openssl, etc) does its own framing independently of this.

This is only an issue in the intermediate, local buffer that is used between openvpn and the TLS library.

So why would this need a protocol update? The on-the-wire format doesn't change.



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

Gerrit-MessageType: comment
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I055c64ca8b23066e70eea7d7deddfb14f5354c5f
Gerrit-Change-Number: 1622
Gerrit-PatchSet: 1
Gerrit-Owner: Bluca <luca.boccassi@...277...>
Gerrit-Reviewer: plaisthos <arne-openvpn@...1227...>
Gerrit-Reviewer: selvanair <selva.nair@...277...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: plaisthos <arne-openvpn@...1227...>
Gerrit-Attention: selvanair <selva.nair@...277...>
Gerrit-Comment-Date: Tue, 07 Apr 2026 10:26:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: plaisthos <arne-openvpn@...1227...>

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

[Openvpn-devel] [M] Change in openvpn[master]: Add new helpers to handle key exchange (S_SENT_KEY/S_START) with larg...

[plaisthos (Code Review)]

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

Attention is currently required from: Bluca, selvanair.

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

Change subject: Add new helpers to handle key exchange (S_SENT_KEY/S_START) with large passwords
......................................................................


Patch Set 1:

(2 comments)

Patchset:

PS1:
> Sorry, I don't really follow. […]
The TLS layer uses TLS records for framing and OpenVPN currently relies on TLS records for its own framing. Not many protocols do this and this also causes problem, ie when you enable record splitting.

And your patch basically decides to break this assumption but only for the key2 related methods, which is in my opinion quite hacky.

And that why I am saying that we need a proper patch/negotiation to overcome this limit instead.

But I get the feeling that you are not really interested in any solution that would actually improve on the OpenVPN protocol to implement longer username/password if it is not compatible with the approach that Microsoft has decided to take.


PS1:
> Sure, but that's 2 years old and included in LTS distros, eg. Ubuntu 24. […]
You might not care about compatibility, interoperability and behaviour of modern clients with older servers and vice versa but we care and we have take that into account. And "that's 2 years old" is way shorter than we care about. We still maintain compatibility with OpenVPN 2.2 server and clients and people are still using a lot of OpenVPN 2.4/OpenVPN 2.5.

And that your patch allows triggering very erratic behaviour with these older version is not a good thing.



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

Gerrit-MessageType: comment
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I055c64ca8b23066e70eea7d7deddfb14f5354c5f
Gerrit-Change-Number: 1622
Gerrit-PatchSet: 1
Gerrit-Owner: Bluca <luca.boccassi@...277...>
Gerrit-Reviewer: plaisthos <arne-openvpn@...1227...>
Gerrit-Reviewer: selvanair <selva.nair@...277...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: Bluca <luca.boccassi@...277...>
Gerrit-Attention: selvanair <selva.nair@...277...>
Gerrit-Comment-Date: Tue, 07 Apr 2026 11:54:13 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: plaisthos <arne-openvpn@...1227...>
Comment-In-Reply-To: Bluca <luca.boccassi@...277...>

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

[Openvpn-devel] [M] Change in openvpn[master]: Add new helpers to handle key exchange (S_SENT_KEY/S_START) with larg...

[Bluca (Code Review)]

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

Attention is currently required from: Bluca, plaisthos, selvanair.

Hello plaisthos, selvanair,

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

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

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

The following approvals got outdated and were removed:
Code-Review-1 by plaisthos


Change subject: Add new helpers to handle key exchange (S_SENT_KEY/S_START) with large passwords
......................................................................

Add new helpers to handle key exchange (S_SENT_KEY/S_START) with large passwords

The current key exchange uses an intermediate buffer hardcoded
at 2048 bytes, that cannot handle anything longer, as it gets
silently truncated.

This breaks using JIT use-once tokens for authentications, that
are becoming common in enterprise devices to reduce attack
surfaces, as the tokens are typically long JWT encoded strings.

Add new helpers that, instead of doing a single 2048 bytes
read/write from/to the TLS layer, reads/writes all the available
data when doing the key method 2 exchange.
Other stages of the protocol are unaffected. Note that this is
an intermediary buffer, the TLS layer is already handling
fragmented/reassembled frames appropriately, so there is no
on-the-wire difference, aside from letting the client send more
data.

Older servers will simply continue to truncate passwords longer
than the existing limit, as they are doing currently, so no
incompatible changes, behaviour stays the same and current valid
passwords are still valid when the new client uses them.
When an old client talks to a new server, likewise there are no
changes, as the previous clients have the embedded limit, so all
existing passwords will continue to work.

Tested combinations:

old client <--> new server
new client <--> old server
new client <--> new server

With the last combination, a large password can be used successfully.
With either an old server or old client, existing limits apply and
a clear error is shown in case one tries a password longer than the
existing limits.

Servers after a7f80d402f send a useful and clear error message.
With older servers the client will instead print a helpful error
for the user, to ensure a good UX in either case.

Change-Id: I055c64ca8b23066e70eea7d7deddfb14f5354c5f
Signed-off-by: Luca Boccassi <luca.boccassi@...277...>
---
M src/openvpn/common.h
M src/openvpn/ssl.c
M src/openvpn/ssl_common.h
3 files changed, 130 insertions(+), 17 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/22/1622/2

diff --git a/src/openvpn/common.h b/src/openvpn/common.h
index aa7b721..b67a6f4 100644
--- a/src/openvpn/common.h
+++ b/src/openvpn/common.h
@@ -69,6 +69,16 @@
  */
 #define TLS_CHANNEL_BUF_SIZE 2048

+/*
+ * Buffer size for key method 2 exchange data. This needs to be
+ * large enough to hold the key source material, options string,
+ * username, password and peer info. When USER_PASS_LEN is larger
+ * than the default TLS channel size (e.g. with ENABLE_PKCS11),
+ * we need a larger buffer. The key exchange write and read paths
+ * handle this by doing multiple TLS writes/reads as needed.
+ */
+#define KEY_METHOD_BUF_SIZE (TLS_CHANNEL_BUF_SIZE + 2 * USER_PASS_LEN)
+
 /* TLS control buffer minimum size
  *
  * A control frame might have IPv6 header (40 byte),
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 576157d..bcf2464 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -860,6 +860,8 @@
     ks->plaintext_read_buf = alloc_buf(TLS_CHANNEL_BUF_SIZE);
     ks->plaintext_write_buf = alloc_buf(TLS_CHANNEL_BUF_SIZE);
     ks->ack_write_buf = alloc_buf(BUF_SIZE(&session->opt->frame));
+    ks->key_method_send_buf = alloc_buf(KEY_METHOD_BUF_SIZE);
+    ks->key_method_recv_buf = alloc_buf(KEY_METHOD_BUF_SIZE);
     reliable_init(ks->send_reliable, BUF_SIZE(&session->opt->frame),
                   session->opt->frame.buf.headroom, TLS_RELIABLE_N_SEND_BUFFERS,
                   ks->key_id ? false : session->opt->xmit_hold);
@@ -906,6 +908,16 @@
 static void
 key_state_free(struct key_state *ks, bool clear)
 {
+    if (ks->key_method_large_payload && ks->state >= S_SENT_KEY
+        && ks->state < S_ACTIVE)
+    {
+        /* Servers before a7f80d402f do not send back a useful error so print one */
+        msg(M_WARN, "Connection failed after sending a key exchange "
+            "payload larger than %d bytes (due to a long password). "
+            "The server may not support large key exchange payloads.",
+            TLS_CHANNEL_BUF_SIZE);
+    }
+
     ks->state = S_UNDEF;

     key_state_ssl_free(&ks->ks_ssl);
@@ -915,6 +927,8 @@
     free_buf(&ks->plaintext_read_buf);
     free_buf(&ks->plaintext_write_buf);
     free_buf(&ks->ack_write_buf);
+    free_buf(&ks->key_method_send_buf);
+    free_buf(&ks->key_method_recv_buf);
     buffer_list_free(ks->paybuf);

     reliable_free(ks->send_reliable);
@@ -2624,6 +2638,54 @@
     return true;
 }

+/**
+ * Read all currently available plaintext from the TLS object into buf,
+ * doing multiple reads if necessary. This is used for key method 2
+ * exchange data which may exceed a single TLS_CHANNEL_BUF_SIZE read
+ * when long passwords/tokens are used.
+ */
+static bool
+read_incoming_tls_plaintext_multi(struct key_state *ks, struct buffer *buf,
+                                  interval_t *wakeup, bool *continue_tls_process)
+{
+    struct buffer tmp = alloc_buf(TLS_CHANNEL_BUF_SIZE);
+
+    while (buf_forward_capacity(buf) > 0)
+    {
+        buf_init(&tmp, 0);
+        int status = key_state_read_plaintext(&ks->ks_ssl, &tmp);
+
+        update_time();
+        if (status == -1)
+        {
+            msg(D_TLS_ERRORS,
+                "TLS Error: TLS object -> incoming plaintext read error");
+            free_buf(&tmp);
+            return false;
+        }
+        if (status == 1)
+        {
+            if (!buf_write(buf, BPTR(&tmp), BLEN(&tmp)))
+            {
+                msg(D_TLS_ERRORS,
+                    "TLS Error: key exchange plaintext too large");
+                free_buf(&tmp);
+                return false;
+            }
+            *continue_tls_process = true;
+            *wakeup = 0;
+            dmsg(D_TLS_DEBUG, "TLS -> Incoming Plaintext (key exchange)");
+        }
+        else
+        {
+            break;
+        }
+    }
+
+    free_buf(&tmp);
+    return true;
+}
+
 static bool
 write_outgoing_tls_ciphertext(struct tls_session *session, bool *continue_tls_process)
 {
@@ -2836,38 +2898,58 @@
     }

     /* Read incoming plaintext from TLS object */
-    struct buffer *buf = &ks->plaintext_read_buf;
-    if (!buf->len)
+    bool in_key_exchange_read =
+        (ks->state == S_SENT_KEY && !session->opt->server)
+        || (ks->state == S_START && session->opt->server);
+
+    if (in_key_exchange_read)
     {
-        if (!read_incoming_tls_plaintext(ks, buf, wakeup, &continue_tls_process))
+        /* For key exchange, read all available plaintext into a buffer
+         * that is large enough for long passwords/tokens. Multiple reads
+         * are done to handle data exceeding TLS_CHANNEL_BUF_SIZE. */
+        if (!read_incoming_tls_plaintext_multi(ks, &ks->key_method_recv_buf,
+                                               wakeup, &continue_tls_process))
         {
             goto error;
         }
     }
+    else
+    {
+        struct buffer *buf = &ks->plaintext_read_buf;
+        if (!buf->len)
+        {
+            if (!read_incoming_tls_plaintext(ks, buf, wakeup, &continue_tls_process))
+            {
+                goto error;
+            }
+        }
+    }

-    /* Send Key */
-    buf = &ks->plaintext_write_buf;
-    if (!buf->len
+    /* Send Key -- use larger key_method_send_buf for assembly to support
+     * long passwords/tokens that exceed TLS_CHANNEL_BUF_SIZE */
+    if (!ks->key_method_send_buf.len
         && ((ks->state == S_START && !session->opt->server)
             || (ks->state == S_GOT_KEY && session->opt->server)))
     {
-        if (!key_method_2_write(buf, multi, session))
+        if (!key_method_2_write(&ks->key_method_send_buf, multi, session))
         {
             goto error;
         }

+        ks->key_method_large_payload =
+            BLEN(&ks->key_method_send_buf) > TLS_CHANNEL_BUF_SIZE;
+
         continue_tls_process = true;
         dmsg(D_TLS_DEBUG_MED, "STATE S_SENT_KEY");
         ks->state = S_SENT_KEY;
     }

-    /* Receive Key */
-    buf = &ks->plaintext_read_buf;
-    if (buf->len
+    /* Receive Key -- use the larger key_method_recv_buf */
+    if (BLEN(&ks->key_method_recv_buf)
         && ((ks->state == S_SENT_KEY && !session->opt->server)
             || (ks->state == S_START && session->opt->server)))
     {
-        if (!key_method_2_read(buf, multi, session))
+        if (!key_method_2_read(&ks->key_method_recv_buf, multi, session))
         {
             goto error;
         }
@@ -2877,20 +2959,38 @@
         ks->state = S_GOT_KEY;
     }

-    /* Write outgoing plaintext to TLS object */
-    buf = &ks->plaintext_write_buf;
-    if (buf->len)
+    /* Write key exchange data to TLS object */
+    if (ks->key_method_send_buf.len)
     {
-        int status = key_state_write_plaintext(&ks->ks_ssl, buf);
+        int status = key_state_write_plaintext(&ks->ks_ssl, &ks->key_method_send_buf);
         if (status == -1)
         {
-            msg(D_TLS_ERRORS, "TLS ERROR: Outgoing Plaintext -> TLS object write error");
+            msg(D_TLS_ERRORS, "TLS ERROR: Key Method -> TLS object write error");
             goto error;
         }
         if (status == 1)
         {
             continue_tls_process = true;
-            dmsg(D_TLS_DEBUG, "Outgoing Plaintext -> TLS");
+            dmsg(D_TLS_DEBUG, "Key Method -> TLS");
+        }
+    }
+
+    /* Write outgoing plaintext to TLS object */
+    {
+        struct buffer *buf = &ks->plaintext_write_buf;
+        if (buf->len)
+        {
+            int status = key_state_write_plaintext(&ks->ks_ssl, buf);
+            if (status == -1)
+            {
+                msg(D_TLS_ERRORS, "TLS ERROR: Outgoing Plaintext -> TLS object write error");
+                goto error;
+            }
+            if (status == 1)
+            {
+                continue_tls_process = true;
+                dmsg(D_TLS_DEBUG, "Outgoing Plaintext -> TLS");
+            }
         }
     }
     if (!check_outgoing_ciphertext(ks, session, &continue_tls_process))
diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
index 6f310a5..913a0c2 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -241,6 +241,9 @@
     struct buffer plaintext_read_buf;
     struct buffer plaintext_write_buf;
     struct buffer ack_write_buf;
+    struct buffer key_method_send_buf; /* larger buffer for key method 2 write */
+    struct buffer key_method_recv_buf; /* larger buffer for key method 2 read */
+    bool key_method_large_payload;     /* key exchange exceeded TLS_CHANNEL_BUF_SIZE */

     struct reliable *send_reliable; /* holds a copy of outgoing packets until ACK received */
     struct reliable *rec_reliable;  /* order incoming ciphertext packets before we pass to TLS */

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

Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I055c64ca8b23066e70eea7d7deddfb14f5354c5f
Gerrit-Change-Number: 1622
Gerrit-PatchSet: 2
Gerrit-Owner: Bluca <luca.boccassi@...277...>
Gerrit-Reviewer: plaisthos <arne-openvpn@...1227...>
Gerrit-Reviewer: selvanair <selva.nair@...277...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: plaisthos <arne-openvpn@...1227...>
Gerrit-Attention: Bluca <luca.boccassi@...277...>
Gerrit-Attention: selvanair <selva.nair@...277...>

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

[Openvpn-devel] [M] Change in openvpn[master]: Add new helpers to handle key exchange (S_SENT_KEY/S_START) with larg...

[Bluca (Code Review)]

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

Attention is currently required from: plaisthos, selvanair.

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

Change subject: Add new helpers to handle key exchange (S_SENT_KEY/S_START) with large passwords
......................................................................


Patch Set 2:

(2 comments)

Patchset:

PS1:
> But I get the feeling that you are not really interested in any solution that would actually improve on the OpenVPN protocol to implement longer username/password if it is not compatible with the approach that Microsoft has decided to take.

Of course what I am after is making my use case work, otherwise what would be the point? I don't think there's anything wrong with wanting to fix things that don't work, that's how open source contributions happen after all, everyone has their own itches to scratch, and mine is being able to ditch all proprietary software from my machine and still be able to do my job. It doesn't feel like it's a bad goal to have, no?

So if you have specific suggestions that can improve the situation in _both_ cases, I'd be happy to work on implementing them.


PS1:
> You might not care about compatibility, interoperability and behaviour of modern clients with older servers and vice versa but we care and we have take that into account.

This is not true, and I must say being on the receiving end of such jibes is really not motivating, as I've been testing these changes in various combinations as explained in the commit message exactly because I don't want to break anything.

But one thing is breaking backward compatibility, and something entirely different is when something _never worked_ in the first place.

This is what happens when trying to use a long password with various combinations of existing client/server pairings.

Pre-a7f80d402f client to pre-a7f80d402f server:

2026-04-08 00:53:44 us=768916 Attempting to establish TCP connection with [AF_INET]127.0.0.1:11940
2026-04-08 00:53:44 us=769011 TCP connection established with [AF_INET]127.0.0.1:11940
2026-04-08 00:53:44 us=769020 TCPv4_CLIENT link local: (not bound)
2026-04-08 00:53:44 us=769026 TCPv4_CLIENT link remote: [AF_INET]127.0.0.1:11940
2026-04-08 00:53:44 us=769450 TLS: Initial packet from [AF_INET]127.0.0.1:11940, sid=34dfa3d7 a78d4716
2026-04-08 00:53:44 us=769480 TLS Error: Key Method #2 write failed
2026-04-08 00:53:44 us=769486 TLS Error: TLS handshake failed
2026-04-08 00:53:44 us=769527 Fatal TLS error (check_tls_errors_co), restarting
2026-04-08 00:53:44 us=769541 TCP/UDP: Closing socket
2026-04-08 00:53:44 us=769569 SIGUSR1[soft,tls-error] received, process restarting
2026-04-08 00:53:44 us=769582 Restart pause, 1 second(s)

2.7 client (without this patch) to pre-a7f80d402f server:

2026-04-08 01:08:17 us=72026 Attempting to establish TCP connection with [AF_INET]127.0.0.1:11940
2026-04-08 01:08:17 us=72132 TCP connection established with [AF_INET]127.0.0.1:11940
2026-04-08 01:08:17 us=72141 TCPv4_CLIENT link local: (not bound)
2026-04-08 01:08:17 us=72147 TCPv4_CLIENT link remote: [AF_INET]127.0.0.1:11940
2026-04-08 01:08:17 us=72596 TLS: Initial packet from [AF_INET]127.0.0.1:11940, sid=dd20ab34 e3af8d3c
2026-04-08 01:08:17 us=72627 TLS Error: Key Method #2 write failed
2026-04-08 01:08:17 us=72636 TLS Error: TLS handshake failed
2026-04-08 01:08:17 us=72702 Fatal TLS error (check_tls_errors_co), restarting
2026-04-08 01:08:17 us=72721 TCP/UDP: Closing socket
2026-04-08 01:08:17 us=72753 SIGUSR1[soft,tls-error] received, process restarting
2026-04-08 01:08:17 us=72766 Restart pause, 2 second(s)

2.7 client (without this patch) to 2.7 server (without this patch):

2026-04-08 01:10:10 us=299468 Attempting to establish TCP connection with [AF_INET]127.0.0.1:11940
2026-04-08 01:10:10 us=299563 TCP connection established with [AF_INET]127.0.0.1:11940
2026-04-08 01:10:10 us=299574 TCPv4_CLIENT link local: (not bound)
2026-04-08 01:10:10 us=299581 TCPv4_CLIENT link remote: [AF_INET]127.0.0.1:11940
2026-04-08 01:10:10 us=299899 TLS: Initial packet from [AF_INET]127.0.0.1:11940, sid=ef841a67 6d757318
2026-04-08 01:10:10 us=299912 TLS Error: Key Method #2 write failed
2026-04-08 01:10:10 us=299917 TLS Error: TLS handshake failed
2026-04-08 01:10:10 us=299967 Fatal TLS error (check_tls_errors_co), restarting
2026-04-08 01:10:10 us=299989 TCP/UDP: Closing socket
2026-04-08 01:10:10 us=300016 SIGUSR1[soft,tls-error] received, process restarting
2026-04-08 01:10:10 us=300026 Restart pause, 2 second(s)

pre-a7f80d402f client to 2.7 server (without this patch):

2026-04-08 01:11:56 us=44683 Attempting to establish TCP connection with [AF_INET]127.0.0.1:11940
2026-04-08 01:11:56 us=44820 TCP connection established with [AF_INET]127.0.0.1:11940
2026-04-08 01:11:56 us=44827 TCPv4_CLIENT link local: (not bound)
2026-04-08 01:11:56 us=44831 TCPv4_CLIENT link remote: [AF_INET]127.0.0.1:11940
2026-04-08 01:11:56 us=45442 TLS: Initial packet from [AF_INET]127.0.0.1:11940, sid=9f2bc992 ef68a461
2026-04-08 01:11:56 us=45469 TLS Error: Key Method #2 write failed
2026-04-08 01:11:56 us=45475 TLS Error: TLS handshake failed
2026-04-08 01:11:56 us=45522 Fatal TLS error (check_tls_errors_co), restarting
2026-04-08 01:11:56 us=45537 TCP/UDP: Closing socket
2026-04-08 01:11:56 us=45572 SIGUSR1[soft,tls-error] received, process restarting
2026-04-08 01:11:56 us=45582 Restart pause, 1 second(s)


Basically the only difference is that the new version waits 2s instead of 1s. The message is exactly the same, an unspecified "key method" failure, that doesn't really say the password is too long, and is entirely unhelpful to a casual user. The error reporting from a7f80d402f doesn't trigger in any combination.

Now here's how it looks with this patch, when connecting to a 2.7 server before this patch:

2026-04-08 01:34:55 us=526614 Attempting to establish TCP connection with [AF_INET]127.0.0.1:11940
2026-04-08 01:34:55 us=526683 TCP connection established with [AF_INET]127.0.0.1:11940
2026-04-08 01:34:55 us=526690 TCPv4_CLIENT link local: (not bound)
2026-04-08 01:34:55 us=526694 TCPv4_CLIENT link remote: [AF_INET]127.0.0.1:11940
2026-04-08 01:34:55 us=527014 TLS: Initial packet from [AF_INET]127.0.0.1:11940, sid=b3756543 a0f9575c
2026-04-08 01:34:55 us=527030 WARNING: this configuration may cache passwords in memory -- use the auth-nocache option to prevent this
2026-04-08 01:34:55 us=527737 VERIFY OK: depth=1, CN=Test CA
2026-04-08 01:34:55 us=527836 VERIFY OK: depth=0, CN=server
2026-04-08 01:34:55 us=568575 Control Channel: TLSv1.3, cipher TLSv1.3 TLS_AES_256_GCM_SHA384, peer certificate: 256 bits ECprime256v1, signature: ecdsa-with-SHA256, peer signing digest/type: ecdsa_secp256r1_sha256 ECDSA, key agreement: X25519MLKEM768
2026-04-08 01:34:55 us=568624 [server] Peer Connection Initiated with [AF_INET]127.0.0.1:11940
2026-04-08 01:34:55 us=568639 TLS: move_session: dest=TM_ACTIVE src=TM_INITIAL reinit_src=1
2026-04-08 01:34:55 us=568717 TLS: tls_multi_process: initial untrusted session promoted to trusted
2026-04-08 01:34:55 us=569034 AUTH: Received control message: AUTH_FAILED,Username or password is too long. Maximum length is 128 bytes
2026-04-08 01:34:55 us=569150 TCP/UDP: Closing socket
2026-04-08 01:34:55 us=569194 SIGTERM[soft,auth-failure] received, process exiting

IE: the new error path reporting from a7f80d402f actually fires up, and reports a useful and actionable message to the user.

With a pre-a7f80d402f server there was no helpful message with a patched client, so I've now added it.

So not only this patch doesn't break compatibility or make the UX worse, it actually improves it in the error case.



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

Gerrit-MessageType: comment
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I055c64ca8b23066e70eea7d7deddfb14f5354c5f
Gerrit-Change-Number: 1622
Gerrit-PatchSet: 2
Gerrit-Owner: Bluca <luca.boccassi@...277...>
Gerrit-Reviewer: plaisthos <arne-openvpn@...1227...>
Gerrit-Reviewer: selvanair <selva.nair@...277...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: plaisthos <arne-openvpn@...1227...>
Gerrit-Attention: selvanair <selva.nair@...277...>
Gerrit-Comment-Date: Wed, 08 Apr 2026 00:54:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: plaisthos <arne-openvpn@...1227...>
Comment-In-Reply-To: Bluca <luca.boccassi@...277...>

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

[Openvpn-devel] [M] Change in openvpn[master]: Add new helpers to handle key exchange (S_SENT_KEY/S_START) with larg...

[plaisthos (Code Review)]

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

Attention is currently required from: Bluca, selvanair.

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

Change subject: Add new helpers to handle key exchange (S_SENT_KEY/S_START) with large passwords
......................................................................


Patch Set 2:

(2 comments)

Patchset:

PS1:
> > But I get the feeling that you are not really interested in any solution that would actually impro […]
There are multiple way that I can see that are better suited to solve the problem of supporting longer username/passwords in OpenVPN. They would have many advantages over the approach that you are trying to force in here. They would have only the one downside of not working with the Azure OpenVPN implementation.

This is not the first patch/protocol change that has been rejected by the OpenVPN maintainers that is in use for the protocol. See the xor patch for another example.

I understand that this is frustrating for you but maybe you should complain to Microsoft instead.


PS1:
> With a pre-a7f80d402f server there was no helpful message with a patched client, so I've now added it.

And while you consider that behaviour completely acceptable and the problem that your patch introduces, I really don't like adding another obscure way an OpenVPN connection can fail. While it might be a not be a big deal for you, you also do not need to debug and support OpenVPN.

> The error reporting from a7f80d402f doesn't trigger in any combination.

To trigger the error reporting of a7f80d402f in earlier version you need a client compiled with --enable-pkcs11 against a server without --enable-pkcs1, ie only accepting passwords of 128 bytes or shorter. As these are not common, as you noticed, it took quite a long time to actually this situation being properly recognised and fixed.



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

Gerrit-MessageType: comment
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I055c64ca8b23066e70eea7d7deddfb14f5354c5f
Gerrit-Change-Number: 1622
Gerrit-PatchSet: 2
Gerrit-Owner: Bluca <luca.boccassi@...277...>
Gerrit-Reviewer: plaisthos <arne-openvpn@...1227...>
Gerrit-Reviewer: selvanair <selva.nair@...277...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: Bluca <luca.boccassi@...277...>
Gerrit-Attention: selvanair <selva.nair@...277...>
Gerrit-Comment-Date: Wed, 08 Apr 2026 01:42:01 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: plaisthos <arne-openvpn@...1227...>
Comment-In-Reply-To: Bluca <luca.boccassi@...277...>

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

[Openvpn-devel] [M] Change in openvpn[master]: Add new helpers to handle key exchange (S_SENT_KEY/S_START) with larg...

[plaisthos (Code Review)]

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

Attention is currently required from: Bluca, selvanair.

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

Change subject: Add new helpers to handle key exchange (S_SENT_KEY/S_START) with large passwords
......................................................................


Patch Set 2: Code-Review-1

(5 comments)

Patchset:

PS2:
I still do not agree with the approach as such but here are additional some comments on the patch itself.


File src/openvpn/common.h:

http://gerrit.openvpn.net/c/openvpn/+/1622/comment/a14781fe_894ca37b?usp=email :
PS2, Line 76:  * than the default TLS channel size (e.g. with ENABLE_PKCS11),
So an OpenVPN compiled without --enable-pkcs11 gets a buffer of 2048 + 2 * 128? That feels a bit odd.


File src/openvpn/ssl.c:

http://gerrit.openvpn.net/c/openvpn/+/1622/comment/a7bf99f1_be645d0f?usp=email :
PS2, Line 919:     }
there are other reasons why a handshake can fail at this point. Printing a message that suggest that probably password is at fault can be misleading.


http://gerrit.openvpn.net/c/openvpn/+/1622/comment/ffbce5ce_262ab2e5?usp=email :
PS2, Line 2645:  * when long passwords/tokens are used.
So this method will basically greedily read as many TLS records as there are currently on the wire. This will make future improvements a lot harder where there are server/clients out there that basically ignore the framing and consider anything that is sent in the same batch to be part of the key method.

Even the original Microsoft patch does not do that. It still uses only one TLS record (max 16kB) for key2 method.

The correct approach here would be to try to read a whole TLS record (16 kB) and take whatever the SSL_read gives you as the key2 method and not doing multiple smaller reads that break the assumption of TLS record framing that OpenVPN (currently) uses.

I am surprised that this did not already cause problems for you when the server sends push reply immediately after its key2 write and this is being put into the key packet. But maybe you never got such unlucky timing.


http://gerrit.openvpn.net/c/openvpn/+/1622/comment/9a4c89e5_f09f9549?usp=email :
PS2, Line 2940:             BLEN(&ks->key_method_send_buf) > TLS_CHANNEL_BUF_SIZE;
This can also be triggered by other methods like setting multiple setenv UV_xx with push-peerinfo in the configuration and then the user will get the error message iwth the long password above.



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

Gerrit-MessageType: comment
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I055c64ca8b23066e70eea7d7deddfb14f5354c5f
Gerrit-Change-Number: 1622
Gerrit-PatchSet: 2
Gerrit-Owner: Bluca <luca.boccassi@...277...>
Gerrit-Reviewer: plaisthos <arne-openvpn@...1227...>
Gerrit-Reviewer: selvanair <selva.nair@...277...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: Bluca <luca.boccassi@...277...>
Gerrit-Attention: selvanair <selva.nair@...277...>
Gerrit-Comment-Date: Wed, 08 Apr 2026 02:07:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes

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

[Openvpn-devel] [M] Change in openvpn[master]: ssl: use TLS record-sized buffers for key method 2 exchange

[Bluca (Code Review)]

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

Attention is currently required from: Bluca, plaisthos, selvanair.

Hello plaisthos, selvanair,

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

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

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

The following approvals got outdated and were removed:
Code-Review-1 by plaisthos


Change subject: ssl: use TLS record-sized buffers for key method 2 exchange
......................................................................

ssl: use TLS record-sized buffers for key method 2 exchange

The current key exchange uses plaintext_read_buf/plaintext_write_buf
(sized at TLS_CHANNEL_BUF_SIZE, 2048 bytes) as intermediary buffers
for the key method 2 exchange. Passwords or tokens longer than
~1900 bytes (after accounting for key material, options and peer
info overhead) get silently truncated.

This breaks using JIT use-once tokens for authentication, which are
becoming common in enterprise setups. These tokens are typically
long JWT-encoded strings that exceed the 2048-byte buffer.

Instead of increasing TLS_CHANNEL_BUF_SIZE (which would change the
control channel framing and require both endpoints to be updated),
introduce separate key_method_send_buf and key_method_recv_buf
buffers in key_state, sized at TLS_RECORD_MAX_SIZE (16384 bytes,
the maximum plaintext payload of a single TLS record).

On the write side, key_method_2_write() assembles the full payload
into the 16 KB buffer, which is then passed to the TLS library via
a single key_state_write_plaintext() call. The TLS library creates
one TLS record, and the existing write_outgoing_tls_ciphertext()
already splits the resulting ciphertext into properly-sized reliable
transport packets.

On the read side, a single key_state_read_plaintext() call reads
into the 16 KB buffer. Since SSL_read/BIO_read returns one TLS
record worth of data per call, this preserves the one-read-per-
message framing assumption used throughout OpenVPN.

All other control channel messages (push, CR_RESPONSE, etc.)
continue to use the original 2048-byte plaintext_read/write_buf.

When the key exchange payload exceeds TLS_CHANNEL_BUF_SIZE and the
connection subsequently fails (state never reaches S_ACTIVE), a
diagnostic message is printed at key_state teardown to help the
user identify that the server may not support large payloads.

Protocol compatibility:
- old client <-> new server: no change, old limits apply
- new client <-> old server: large passwords will cause the old
  server to truncate/fail; a clear error message is shown
- new client <-> new server: large passwords work

Change-Id: I055c64ca8b23066e70eea7d7deddfb14f5354c5f
Signed-off-by: Luca Boccassi <luca.boccassi@...277...>
---
M src/openvpn/common.h
M src/openvpn/ssl.c
M src/openvpn/ssl_common.h
3 files changed, 88 insertions(+), 18 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/22/1622/3

diff --git a/src/openvpn/common.h b/src/openvpn/common.h
index aa7b721..b1a8516 100644
--- a/src/openvpn/common.h
+++ b/src/openvpn/common.h
@@ -69,6 +69,17 @@
  */
 #define TLS_CHANNEL_BUF_SIZE 2048

+/*
+ * Buffer size for key method 2 exchange data (send and receive).
+ * A single TLS record can carry up to 2^14 (16384) bytes of
+ * plaintext. We size the key exchange buffers to a full TLS
+ * record so that payloads larger than TLS_CHANNEL_BUF_SIZE
+ * (e.g. with long PKCS#11 passwords/tokens) can be sent and
+ * received in a single TLS read/write without breaking the
+ * one-read-per-message assumption used elsewhere.
+ */
+#define TLS_RECORD_MAX_SIZE (1 << 14)
+
 /* TLS control buffer minimum size
  *
  * A control frame might have IPv6 header (40 byte),
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 576157d..c8bf35c 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -860,6 +860,8 @@
     ks->plaintext_read_buf = alloc_buf(TLS_CHANNEL_BUF_SIZE);
     ks->plaintext_write_buf = alloc_buf(TLS_CHANNEL_BUF_SIZE);
     ks->ack_write_buf = alloc_buf(BUF_SIZE(&session->opt->frame));
+    ks->key_method_send_buf = alloc_buf(TLS_RECORD_MAX_SIZE);
+    ks->key_method_recv_buf = alloc_buf(TLS_RECORD_MAX_SIZE);
     reliable_init(ks->send_reliable, BUF_SIZE(&session->opt->frame),
                   session->opt->frame.buf.headroom, TLS_RELIABLE_N_SEND_BUFFERS,
                   ks->key_id ? false : session->opt->xmit_hold);
@@ -906,6 +908,17 @@
 static void
 key_state_free(struct key_state *ks, bool clear)
 {
+    if (ks->key_method_large_payload && ks->state >= S_SENT_KEY
+        && ks->state < S_ACTIVE)
+    {
+        /* Servers before a7f80d402f do not send back a useful error so print one */
+        msg(M_WARN, "Connection failed after sending a key exchange "
+            "payload larger than %d bytes (due to a long password). "
+            "This may happen when the server does not support large "
+            "key exchange payloads, try with a smaller password.",
+            TLS_CHANNEL_BUF_SIZE);
+    }
+
     ks->state = S_UNDEF;

     key_state_ssl_free(&ks->ks_ssl);
@@ -915,6 +928,8 @@
     free_buf(&ks->plaintext_read_buf);
     free_buf(&ks->plaintext_write_buf);
     free_buf(&ks->ack_write_buf);
+    free_buf(&ks->key_method_send_buf);
+    free_buf(&ks->key_method_recv_buf);
     buffer_list_free(ks->paybuf);

     reliable_free(ks->send_reliable);
@@ -2836,38 +2851,61 @@
     }

     /* Read incoming plaintext from TLS object */
-    struct buffer *buf = &ks->plaintext_read_buf;
-    if (!buf->len)
+    bool in_key_exchange_read =
+        (ks->state == S_SENT_KEY && !session->opt->server)
+        || (ks->state == S_START && session->opt->server);
+
+    if (in_key_exchange_read)
     {
-        if (!read_incoming_tls_plaintext(ks, buf, wakeup, &continue_tls_process))
+        /* For key exchange, read into a buffer large enough for a full
+         * TLS record (16 KB) so that long passwords/tokens that exceed
+         * TLS_CHANNEL_BUF_SIZE are received in a single read. */
+        if (!BLEN(&ks->key_method_recv_buf))
         {
-            goto error;
+            if (!read_incoming_tls_plaintext(ks, &ks->key_method_recv_buf,
+                                             wakeup, &continue_tls_process))
+            {
+                goto error;
+            }
+        }
+    }
+    else
+    {
+        struct buffer *buf = &ks->plaintext_read_buf;
+        if (!buf->len)
+        {
+            if (!read_incoming_tls_plaintext(ks, buf, wakeup, &continue_tls_process))
+            {
+                goto error;
+            }
         }
     }

-    /* Send Key */
-    buf = &ks->plaintext_write_buf;
-    if (!buf->len
+    /* Send Key -- use larger key_method_send_buf for assembly to support
+     * long passwords/tokens that exceed TLS_CHANNEL_BUF_SIZE */
+    if (!ks->key_method_send_buf.len
         && ((ks->state == S_START && !session->opt->server)
             || (ks->state == S_GOT_KEY && session->opt->server)))
     {
-        if (!key_method_2_write(buf, multi, session))
+        if (!key_method_2_write(&ks->key_method_send_buf, multi, session))
         {
             goto error;
         }

+        ks->key_method_large_payload =
+            BLEN(&ks->key_method_send_buf) > TLS_CHANNEL_BUF_SIZE;
+
         continue_tls_process = true;
         dmsg(D_TLS_DEBUG_MED, "STATE S_SENT_KEY");
         ks->state = S_SENT_KEY;
     }

-    /* Receive Key */
-    buf = &ks->plaintext_read_buf;
-    if (buf->len
+    /* Receive Key -- use the larger key_method_recv_buf */
+    if (BLEN(&ks->key_method_recv_buf)
         && ((ks->state == S_SENT_KEY && !session->opt->server)
             || (ks->state == S_START && session->opt->server)))
     {
-        if (!key_method_2_read(buf, multi, session))
+        if (!key_method_2_read(&ks->key_method_recv_buf, multi, session))
         {
             goto error;
         }
@@ -2877,20 +2915,38 @@
         ks->state = S_GOT_KEY;
     }

-    /* Write outgoing plaintext to TLS object */
-    buf = &ks->plaintext_write_buf;
-    if (buf->len)
+    /* Write key exchange data to TLS object */
+    if (ks->key_method_send_buf.len)
     {
-        int status = key_state_write_plaintext(&ks->ks_ssl, buf);
+        int status = key_state_write_plaintext(&ks->ks_ssl, &ks->key_method_send_buf);
         if (status == -1)
         {
-            msg(D_TLS_ERRORS, "TLS ERROR: Outgoing Plaintext -> TLS object write error");
+            msg(D_TLS_ERRORS, "TLS ERROR: Key Method -> TLS object write error");
             goto error;
         }
         if (status == 1)
         {
             continue_tls_process = true;
-            dmsg(D_TLS_DEBUG, "Outgoing Plaintext -> TLS");
+            dmsg(D_TLS_DEBUG, "Key Method -> TLS");
+        }
+    }
+
+    /* Write outgoing plaintext to TLS object */
+    {
+        struct buffer *buf = &ks->plaintext_write_buf;
+        if (buf->len)
+        {
+            int status = key_state_write_plaintext(&ks->ks_ssl, buf);
+            if (status == -1)
+            {
+                msg(D_TLS_ERRORS, "TLS ERROR: Outgoing Plaintext -> TLS object write error");
+                goto error;
+            }
+            if (status == 1)
+            {
+                continue_tls_process = true;
+                dmsg(D_TLS_DEBUG, "Outgoing Plaintext -> TLS");
+            }
         }
     }
     if (!check_outgoing_ciphertext(ks, session, &continue_tls_process))
diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
index 6f310a5..913a0c2 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -241,6 +241,9 @@
     struct buffer plaintext_read_buf;
     struct buffer plaintext_write_buf;
     struct buffer ack_write_buf;
+    struct buffer key_method_send_buf; /* larger buffer for key method 2 write */
+    struct buffer key_method_recv_buf; /* larger buffer for key method 2 read */
+    bool key_method_large_payload;     /* key exchange exceeded TLS_CHANNEL_BUF_SIZE */

     struct reliable *send_reliable; /* holds a copy of outgoing packets until ACK received */
     struct reliable *rec_reliable;  /* order incoming ciphertext packets before we pass to TLS */

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

Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I055c64ca8b23066e70eea7d7deddfb14f5354c5f
Gerrit-Change-Number: 1622
Gerrit-PatchSet: 3
Gerrit-Owner: Bluca <luca.boccassi@...277...>
Gerrit-Reviewer: plaisthos <arne-openvpn@...1227...>
Gerrit-Reviewer: selvanair <selva.nair@...277...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: plaisthos <arne-openvpn@...1227...>
Gerrit-Attention: Bluca <luca.boccassi@...277...>
Gerrit-Attention: selvanair <selva.nair@...277...>

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

[Openvpn-devel] [M] Change in openvpn[master]: ssl: use TLS record-sized buffers for key method 2 exchange

[Bluca (Code Review)]

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

Attention is currently required from: plaisthos, selvanair.

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

Change subject: ssl: use TLS record-sized buffers for key method 2 exchange
......................................................................


Patch Set 2:

(5 comments)

Patchset:

PS1:
> > With a pre-a7f80d402f server there was no helpful message with a patched client, so I've now added it.
>
> And while you consider that behaviour completely acceptable and the problem that your patch introduces, I really don't like adding another obscure way an OpenVPN connection can fail. While it might be a not be a big deal for you, you also do not need to debug and support OpenVPN.

But the problem is not introduced by this patch, it's always been there, and the logs above intended to show exactly this.
The intent of this change is that it doesn't make it any better or any worse - it simply stays the same: if the server doesn't support it, you get a clear error message from the client saying so.


PS1:
> There are multiple way that I can see that are better suited to solve the problem of supporting longer username/passwords in OpenVPN. They would have many advantages over the approach that you are trying to force in here. They would have only the one downside of not working with the Azure OpenVPN implementation.

Well, that one downside is the one thing I am motivated by...

> I understand that this is frustrating for you but maybe you should complain to Microsoft instead.

Even if there was anybody to complain to (might as well try to complain to a brick wall), using tokens for auth is not going to go away. There are way too many real world security disasters because passwords get exfiltrated or phished and then used to gain permanent access to intranets. JIT single-use tokens are not going anywhere, regardless of any complaint one might make, and they don't fit in the current hardcoded buffer.


File src/openvpn/ssl.c:

http://gerrit.openvpn.net/c/openvpn/+/1622/comment/7ddd6405_e59c67df?usp=email :
PS2, Line 919:     }
> there are other reasons why a handshake can fail at this point. […]
Reworded a bit to make it into a suggestion rather than a statement, can further reword it if needed


http://gerrit.openvpn.net/c/openvpn/+/1622/comment/cc5a8238_93462793?usp=email :
PS2, Line 2645:  * when long passwords/tokens are used.
> So this method will basically greedily read as many TLS records as there are currently on the wire. […]
Done


http://gerrit.openvpn.net/c/openvpn/+/1622/comment/0a3fd2a0_be58e8fd?usp=email :
PS2, Line 2940:             BLEN(&ks->key_method_send_buf) > TLS_CHANNEL_BUF_SIZE;
> This can also be triggered by other methods like setting multiple setenv UV_xx with push-peerinfo in […]
Doesn't push_peer_info() hardcode a limit of 512 * 3, which is lower than 2048 anyway? Could you please share a config that would reproduce this issue?



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

Gerrit-MessageType: comment
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I055c64ca8b23066e70eea7d7deddfb14f5354c5f
Gerrit-Change-Number: 1622
Gerrit-PatchSet: 2
Gerrit-Owner: Bluca <luca.boccassi@...277...>
Gerrit-Reviewer: plaisthos <arne-openvpn@...1227...>
Gerrit-Reviewer: selvanair <selva.nair@...277...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: plaisthos <arne-openvpn@...1227...>
Gerrit-Attention: selvanair <selva.nair@...277...>
Gerrit-Comment-Date: Thu, 09 Apr 2026 01:10:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: plaisthos <arne-openvpn@...1227...>
Comment-In-Reply-To: Bluca <luca.boccassi@...277...>

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

[Openvpn-devel] [M] Change in openvpn[master]: ssl: use TLS record-sized buffers for key method 2 exchange

[Bluca (Code Review)]

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

Attention is currently required from: plaisthos, selvanair.

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

Change subject: ssl: use TLS record-sized buffers for key method 2 exchange
......................................................................


Patch Set 3:

(1 comment)

File src/openvpn/common.h:

http://gerrit.openvpn.net/c/openvpn/+/1622/comment/aa56e2af_15c93d1a?usp=email :
PS2, Line 76:  * than the default TLS channel size (e.g. with ENABLE_PKCS11),
> So an OpenVPN compiled without --enable-pkcs11 gets a buffer of 2048 + 2 * 128? That feels a bit odd […]
Done



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

Gerrit-MessageType: comment
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I055c64ca8b23066e70eea7d7deddfb14f5354c5f
Gerrit-Change-Number: 1622
Gerrit-PatchSet: 3
Gerrit-Owner: Bluca <luca.boccassi@...277...>
Gerrit-Reviewer: plaisthos <arne-openvpn@...1227...>
Gerrit-Reviewer: selvanair <selva.nair@...277...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: plaisthos <arne-openvpn@...1227...>
Gerrit-Attention: selvanair <selva.nair@...277...>
Gerrit-Comment-Date: Thu, 09 Apr 2026 01:10:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: plaisthos <arne-openvpn@...1227...>

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