diff mbox series

[Openvpn-devel,v3,26/28] Allow setting control channel packet size with tls-mtu

Message ID 20220511110810.1944975-1-arne@rfc2549.org
State Superseded
Headers show
Series None | expand

Commit Message

Arne Schwabe May 11, 2022, 1:08 a.m. UTC 
Currently control packet size is controlled by tun-mtu in a very
non-obvious way since the control overhead is not taken into account
and control channel packet will end up with a different size than
data channel packet.

Instead we decouple this and introduce tls-mtu which defaults to
1250.

Patch v2: rebase on latest patch set
Patch v3: Introduce TLS_CHANNEL_MTU_MIN define and give explaination
          of its value.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 Changes.rst                       |  8 ++++++++
 doc/man-sections/link-options.rst |  7 +++++++
 src/openvpn/common.h              | 10 ++++++++++
 src/openvpn/init.c                |  8 ++++++--
 src/openvpn/mtu.h                 |  5 +++++
 src/openvpn/options.c             | 14 ++++++++++++++
 src/openvpn/options.h             |  1 +
 src/openvpn/ssl.c                 | 25 +++++++++++++------------
 src/openvpn/ssl.h                 |  8 +++-----
 9 files changed, 67 insertions(+), 19 deletions(-)

Comments

Frank Lichtenheld May 11, 2022, 1:27 a.m. UTC | #1 
> Arne Schwabe <arne@rfc2549.org> hat am 11.05.2022 13:08 geschrieben:
> 
>  
> Currently control packet size is controlled by tun-mtu in a very
> non-obvious way since the control overhead is not taken into account
> and control channel packet will end up with a different size than
> data channel packet.
> 
> Instead we decouple this and introduce tls-mtu which defaults to
> 1250.
> 
> Patch v2: rebase on latest patch set
> Patch v3: Introduce TLS_CHANNEL_MTU_MIN define and give explaination
>           of its value.

"explanation"

> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>  Changes.rst                       |  8 ++++++++
>  doc/man-sections/link-options.rst |  7 +++++++
>  src/openvpn/common.h              | 10 ++++++++++
>  src/openvpn/init.c                |  8 ++++++--
>  src/openvpn/mtu.h                 |  5 +++++
>  src/openvpn/options.c             | 14 ++++++++++++++
>  src/openvpn/options.h             |  1 +
>  src/openvpn/ssl.c                 | 25 +++++++++++++------------
>  src/openvpn/ssl.h                 |  8 +++-----
>  9 files changed, 67 insertions(+), 19 deletions(-)
> 
[...]
> diff --git a/src/openvpn/common.h b/src/openvpn/common.h
> index b94680885..056c25438 100644
> --- a/src/openvpn/common.h
> +++ b/src/openvpn/common.h
> @@ -68,6 +68,16 @@ typedef unsigned long ptr_type;
>   */
>  #define TLS_CHANNEL_BUF_SIZE 2048
>  
> +/* TLS control buffer minimum size, this size is not actually inherent to
> + * the protocol but. Our current sending window is 6 and the receive window

Maybe "the protocol. However, our" ?

> + * is 8 or 12 depending on the OpenVPN version. We need to be able to send
> + * a TLS record of size TLS_CHANNEL_BUF_SIZE. Splitting this into more than
> + * 6 packets (with overhead) would complicate our sending logic a lot more, so
> + * we settle here for a "round" number that allow with overhead of ~100 bytes

remove "allow"

> + * to be larger than TLS_CHANNEL_BUF_SIZE. E.g. 6x ~400 > 2048.

"to be" -> "is"

> + * */
> +#define TLS_CHANNEL_MTU_MIN 512
> +
>  /*
>   * This parameter controls the maximum size of a bundle
>   * of pushed options.
[...]

Regards,
--
Frank Lichtenheld
Heiko Hund Sept. 15, 2022, 2:47 p.m. UTC | #2 
This patch need to be rebased again, does not apply to master anymore:
checking file src/openvpn/ssl.c
Hunk #1 succeeded at 297 (offset 1 line).
Hunk #2 FAILED at 321.
Hunk #3 succeeded at 1256 (offset -42 lines).

On Mittwoch, 11. Mai 2022 13:08:10 CEST Arne Schwabe wrote:
> @@ -141,6 +147,8 @@ User-visible Changes
> +- control channel packet maximum size is no longer influenced by ``--link-
mtu``/``--tun-mtu``
> +  and must be set by ``--tls-mtu`` now.

Must it or is there still some cc-mtu derivation in place in case it is not 
given? If not, wouldn't it make sense to at least catch obvious breakage. 
Usually link/tun-mtu is lowered for that reason. Further down in the patch 
there are min/max_int() calls in tls_init_control_channel_frame_parameters(), 
do they do what I suggested here already?

> diff --git a/doc/man-sections/link-options.rst
> +--tls-mtu size

Is --tls-mtu the best name for the option we can come up with? From my outside 
perspective (can hardly spell mtu right) the "tls" part is somewhat misnamed, 
as this is an SSL VPN after all. When you know that the framing for the data 
channel is not TLS you can make sense of it, but who does.

> +  The maximum packet size includes encapsulation overhead like UDP
> and IP. 
> \ No newline at end of file

^ this

> +    int tls_mtu;         /* Maximum MTU for the
> control channel messages */

The "M" already stands for "Maximum".
Arne Schwabe Sept. 15, 2022, 11:37 p.m. UTC | #3 
Am 16.09.2022 um 02:47 schrieb Heiko Hund:
> This patch need to be rebased again, does not apply to master anymore:
> checking file src/openvpn/ssl.c
> Hunk #1 succeeded at 297 (offset 1 line).
> Hunk #2 FAILED at 321.
> Hunk #3 succeeded at 1256 (offset -42 lines).
>
> On Mittwoch, 11. Mai 2022 13:08:10 CEST Arne Schwabe wrote:
>> @@ -141,6 +147,8 @@ User-visible Changes
>> +- control channel packet maximum size is no longer influenced by ``--link-
> mtu``/``--tun-mtu``
>> +  and must be set by ``--tls-mtu`` now.
> Must it or is there still some cc-mtu derivation in place in case it is not
> given? If not, wouldn't it make sense to at least catch obvious breakage.
> Usually link/tun-mtu is lowered for that reason. Further down in the patch
> there are min/max_int() calls in tls_init_control_channel_frame_parameters(),
> do they do what I suggested here already?

The current code is really broken and also if you lower it too much it 
just stops completely working as it for example will try to send a push 
message that is too large to fit the size. So I decided to use a clean 
sheet approach here. And the current tls-mtu default is 1200 or 
something similar anyway. So it is unlikely to break on any normal 
connection.


>> diff --git a/doc/man-sections/link-options.rst
>> +--tls-mtu size
> Is --tls-mtu the best name for the option we can come up with? From my outside
> perspective (can hardly spell mtu right) the "tls" part is somewhat misnamed,
> as this is an SSL VPN after all. When you know that the framing for the data
> channel is not TLS you can make sense of it, but who does.
It fits the naming of the other options we have. But if you can come up 
with a better naming, I am all ears.
>
>> +  The maximum packet size includes encapsulation overhead like UDP
>> and IP.
>> \ No newline at end of file
> ^ this
>
>> +    int tls_mtu;         /* Maximum MTU for the
>> control channel messages */
> The "M" already stands for "Maximum".
>
>
>
>
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
diff mbox series

Patch

diff --git a/Changes.rst b/Changes.rst
index 67a23c792..2d00859e3 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -79,6 +79,12 @@  Cookie based handshake for UDP server
     shake. The tls-crypt-v2 option allows controlling if older clients are
     accepted.
 
+Improved control channel packet size control (``--tls-mtu``)
+    The size of control channel is no longer tied to
+    ``--link-mtu``/``--tun-mtu`` and can be set using ``--tls-mtu``. Setting
+    the size to small sizes no longer breaks the OpenVPN protocol in certain
+    situations.
+
 Deprecated features
 -------------------
 ``inetd`` has been removed
@@ -141,6 +147,8 @@  User-visible Changes
 - Option ``--nobind`` is default when ``--client`` or ``--pull`` is used in the configuration
 - :code:`link_mtu` parameter is removed from environment or replaced with 0 when scripts are
   called with parameters. This parameter is unreliable and no longer internally calculated.
+- control channel packet maximum size is no longer influenced by ``--link-mtu``/``--tun-mtu``
+  and must be set by ``--tls-mtu`` now.
 
 Overview of changes in 2.5
 ==========================
diff --git a/doc/man-sections/link-options.rst b/doc/man-sections/link-options.rst
index 6473ad423..40ff2c3ee 100644
--- a/doc/man-sections/link-options.rst
+++ b/doc/man-sections/link-options.rst
@@ -454,3 +454,10 @@  the local and the remote host.
      if mode server:
          socket-flags TCP_NODELAY
          push "socket-flags TCP_NODELAY"
+
+--tls-mtu size
+  This option sets the maximum size for control channel packets. OpenVPN will
+  try to keep its control channel messages below this size but due to some
+  constraints in the protocol this is not always possible. If the option is
+  not set, it default to 1250. Valid sizes are between 512 and 2048.
+  The maximum packet size includes encapsulation overhead like UDP and IP.
\ No newline at end of file
diff --git a/src/openvpn/common.h b/src/openvpn/common.h
index b94680885..056c25438 100644
--- a/src/openvpn/common.h
+++ b/src/openvpn/common.h
@@ -68,6 +68,16 @@  typedef unsigned long ptr_type;
  */
 #define TLS_CHANNEL_BUF_SIZE 2048
 
+/* TLS control buffer minimum size, this size is not actually inherent to
+ * the protocol but. Our current sending window is 6 and the receive window
+ * is 8 or 12 depending on the OpenVPN version. We need to be able to send
+ * a TLS record of size TLS_CHANNEL_BUF_SIZE. Splitting this into more than
+ * 6 packets (with overhead) would complicate our sending logic a lot more, so
+ * we settle here for a "round" number that allow with overhead of ~100 bytes
+ * to be larger than TLS_CHANNEL_BUF_SIZE. E.g. 6x ~400 > 2048.
+ * */
+#define TLS_CHANNEL_MTU_MIN 512
+
 /*
  * This parameter controls the maximum size of a bundle
  * of pushed options.
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index b0c62a859..c86866219 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2451,6 +2451,10 @@  frame_finalize_options(struct context *c, const struct options *o)
      * space */
     size_t payload_size = max_int(1500, frame->tun_mtu);
 
+    /* we need to be also large enough to hold larger control channel packets
+     * if configured */
+    payload_size = max_int(payload_size, o->ce.tls_mtu);
+
     /* The extra tun needs to be added to the payload size */
     if (o->ce.tun_mtu_defined)
     {
@@ -2987,7 +2991,7 @@  do_init_frame_tls(struct context *c)
 {
     if (c->c2.tls_multi)
     {
-        tls_multi_init_finalize(c->c2.tls_multi, &c->c2.frame);
+        tls_multi_init_finalize(c->c2.tls_multi, c->options.ce.tls_mtu);
         ASSERT(c->c2.tls_multi->opt.frame.buf.payload_size <=
                c->c2.frame.buf.payload_size);
         frame_print(&c->c2.tls_multi->opt.frame, D_MTU_INFO,
@@ -2995,7 +2999,7 @@  do_init_frame_tls(struct context *c)
     }
     if (c->c2.tls_auth_standalone)
     {
-        tls_init_control_channel_frame_parameters(&c->c2.frame, &c->c2.tls_auth_standalone->frame);
+        tls_init_control_channel_frame_parameters(&c->c2.tls_auth_standalone->frame, c->options.ce.tls_mtu);
         frame_print(&c->c2.tls_auth_standalone->frame, D_MTU_INFO,
                     "TLS-Auth MTU parms");
         c->c2.tls_auth_standalone->tls_wrap.work = alloc_buf_gc(BUF_SIZE(&c->c2.frame), &c->c2.gc);
diff --git a/src/openvpn/mtu.h b/src/openvpn/mtu.h
index 7f967e066..86959bd53 100644
--- a/src/openvpn/mtu.h
+++ b/src/openvpn/mtu.h
@@ -79,6 +79,11 @@ 
  */
 #define MSSFIX_DEFAULT     1492
 
+/*
+ * Default maximum size of control channel packets
+ */
+#define TLS_MTU_DEFAULT    1250
+
 /*
  * Alignment of payload data such as IP packet or
  * ethernet frame.
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 9ff384d09..8241eb163 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -815,6 +815,7 @@  init_options(struct options *o, const bool init_gc)
     o->ce.bind_local = true;
     o->ce.tun_mtu = TUN_MTU_DEFAULT;
     o->ce.link_mtu = LINK_MTU_DEFAULT;
+    o->ce.tls_mtu = TLS_MTU_DEFAULT;
     o->ce.mtu_discover_type = -1;
     o->ce.mssfix = 0;
     o->ce.mssfix_default = true;
@@ -1582,6 +1583,7 @@  show_connection_entry(const struct connection_entry *o)
     SHOW_BOOL(link_mtu_defined);
     SHOW_INT(tun_mtu_extra);
     SHOW_BOOL(tun_mtu_extra_defined);
+    SHOW_INT(tls_mtu);
 
     SHOW_INT(mtu_discover_type);
 
@@ -6281,6 +6283,18 @@  add_option(struct options *options,
         options->ce.tun_mtu_extra = positive_atoi(p[1]);
         options->ce.tun_mtu_extra_defined = true;
     }
+    else if (streq(p[0], "tls-mtu") && p[1] && !p[2])
+    {
+        VERIFY_PERMISSION(OPT_P_MTU|OPT_P_CONNECTION);
+        int tls_mtu = atoi(p[1]);
+        if (tls_mtu < TLS_CHANNEL_MTU_MIN || tls_mtu > TLS_CHANNEL_BUF_SIZE)
+        {
+            msg(msglevel, "Bad tls-mtu value, must be between %d and %d",
+                TLS_CHANNEL_MTU_MIN, TLS_CHANNEL_BUF_SIZE);
+            goto err;
+        }
+        options->ce.tls_mtu = positive_atoi(p[1]);
+    }
 #ifdef ENABLE_FRAGMENT
     else if (streq(p[0], "mtu-dynamic"))
     {
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index c2937dc37..6615d1c74 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -123,6 +123,7 @@  struct connection_entry
     bool tun_mtu_extra_defined;
     int link_mtu;        /* MTU of device over which tunnel packets pass via TCP/UDP */
     bool link_mtu_defined; /* true if user overriding parm with command line option */
+    int tls_mtu;         /* Maximum MTU for the control channel messages */
 
     /* Advanced MTU negotiation and datagram fragmentation options */
     int mtu_discover_type; /* used if OS supports setting Path MTU discovery options on socket */
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 1e3c500d8..eb4898a85 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -296,8 +296,7 @@  tls_limit_reneg_bytes(const char *ciphername, int *reneg_bytes)
 }
 
 void
-tls_init_control_channel_frame_parameters(const struct frame *data_channel_frame,
-                                          struct frame *frame)
+tls_init_control_channel_frame_parameters(struct frame *frame, int tls_mtu)
 {
     /*
      * frame->extra_frame is already initialized with tls_auth buffer requirements,
@@ -322,18 +321,20 @@  tls_init_control_channel_frame_parameters(const struct frame *data_channel_frame
 
     /* Previous OpenVPN version calculated the maximum size and buffer of a
      * control frame depending on the overhead of the data channel frame
-     * overhead and limited its maximum size to 1250. We always allocate the
-     * TLS_CHANNEL_BUF_SIZE buffer size since a lot of code blindly assumes
-     * a large buffer (e.g. PUSH_BUNDLE_SIZE) and also our peer might have
-     * a higher size configured and we still want to be able to receive the
-     * packets. frame->mtu_mtu is set as suggestion for the maximum packet
-     * size */
-    frame->buf.payload_size = 1250 + overhead;
+     * overhead and limited its maximum size to 1250. Since control frame
+     * frames also need to fit into data channel buffer we have the same
+     * default of 1500 + 100 as data channel buffers have. Increasing
+     * tls-mtu beyond this limit also increases the data channel buffers */
+    frame->buf.payload_size = max_int(1500, tls_mtu) + 100;
 
     frame->buf.headroom = overhead;
     frame->buf.tailroom = overhead;
 
-    frame->tun_mtu = min_int(data_channel_frame->tun_mtu, 1250);
+    frame->tun_mtu = tls_mtu;
+
+    /* Ensure the tun-mtu stays in a valid range */
+    frame->tun_mtu = min_int(frame->tun_mtu, TLS_CHANNEL_BUF_SIZE);
+    frame->tun_mtu = max_int(frame->tun_mtu, TLS_CHANNEL_MTU_MIN);
 }
 
 /**
@@ -1299,9 +1300,9 @@  tls_multi_init(struct tls_options *tls_options)
 }
 
 void
-tls_multi_init_finalize(struct tls_multi *multi, const struct frame *frame)
+tls_multi_init_finalize(struct tls_multi *multi, int tls_mtu)
 {
-    tls_init_control_channel_frame_parameters(frame, &multi->opt.frame);
+    tls_init_control_channel_frame_parameters(&multi->opt.frame, tls_mtu);
     /* initialize the active and untrusted sessions */
 
     tls_session_init(multi, &multi->session[TM_ACTIVE]);
diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
index 0ba86d3e6..cab5f449e 100644
--- a/src/openvpn/ssl.h
+++ b/src/openvpn/ssl.h
@@ -156,10 +156,9 @@  struct tls_multi *tls_multi_init(struct tls_options *tls_options);
  *
  * @param multi        - The \c tls_multi structure of which to finalize
  *                       initialization.
- * @param frame        - The data channel's \c frame structure.
+ * @param tls_mtu      - maximum allowed size for control channel packets
  */
-void tls_multi_init_finalize(struct tls_multi *multi,
-                             const struct frame *frame);
+void tls_multi_init_finalize(struct tls_multi *multi, int tls_mtu);
 
 /*
  * Initialize a standalone tls-auth verification object.
@@ -171,8 +170,7 @@  struct tls_auth_standalone *tls_auth_standalone_init(struct tls_options *tls_opt
  * Setups the control channel frame size parameters from the data channel
  * parameters
  */
-void tls_init_control_channel_frame_parameters(const struct frame *data_channel_frame,
-                                               struct frame *frame);
+void tls_init_control_channel_frame_parameters(struct frame *frame, int tls_mtu);
 
 /*
  * Set local and remote option compatibility strings.