[Openvpn-devel,v6,2/2] Allow setting control channel packet size with max-packet-size

Message ID 20221104125655.656150-2-arne@rfc2549.org
State Accepted
Headers show
Series [Openvpn-devel,v6,1/2] Refactor/optimise code sending TLS control channel messages | expand

Commit Message

Arne Schwabe Nov. 4, 2022, 1:56 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 max-packet-size. Control packet size
defaults to 1250 if max-packet-size is not set.

Patch v2: rebase on latest patch set
Patch v3: Introduce TLS_CHANNEL_MTU_MIN define and give explaination
          of its value.
Patch v4: introduce max-packet-size instead of tls-mtu
Patch v5: improve documentation
Patch v6: Rebase, lower lower limit, add warning message for
          when wrapped tls-crypt-v2 keys will ignore max-packet-size

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 Changes.rst                       | 11 +++++++++-
 doc/man-sections/link-options.rst | 34 ++++++++++++++++++++++++++++---
 src/openvpn/common.h              | 13 ++++++++++++
 src/openvpn/init.c                | 23 +++++++++++++++++++--
 src/openvpn/mtu.h                 |  5 +++++
 src/openvpn/options.c             | 21 +++++++++++++++++++
 src/openvpn/options.h             |  1 +
 src/openvpn/ssl.c                 | 26 ++++++++++++-----------
 src/openvpn/ssl.h                 |  8 +++-----
 9 files changed, 119 insertions(+), 23 deletions(-)

Comments

Gert Doering Nov. 5, 2022, 9:58 p.m. UTC | #1
Stared-at-code, discussed with Arne on IRC.  There's a few things one
could have done differently (like, using constrain_int() inside
tls_init_control_channel_frame_parameters()), but this is all minor.

Tested regular t_client and server side tests (which are not excercising
the new option) - all good.

Tested with the new option, to see what I could break (no tls-auth or
tls-crypt in use for this test)

 - not specified -> maximum TLS packet size is 1250 byte (1264 eth len)

 - --max-packet-size 100
   -> tells me: "Note: max-packet-size value outside of allowed control
       channel packet size (154 to 2048), will use 154 instead." and
   maximum TLS packet size observed is 154 byte, with a single packet
   of 169 byte

 - --max-packet-size 500
   -> most packets are 488 bytes, one is 500 bytes

 - --max-packet-size 3000
   -> it complains to me, "will use 2048 instead".  I observe outside-
   fragmented packets, with an inner UDP length of 2020, and a total
   combined length of both fragments of 2056 bytes (second IP header
   adds more bytes outside OpenVPN Control).

   This test case actually fails to establish a TLS session(!) - the
   server I connect to has the same code, but no --max-packet option
   in its config, so it does not want to see these packets:

   Nov  6 09:41:39 gentoo tun-udp-p2mp-topology-subnet[15790]: 194.97.140.21:40355 Incoming control channel packet too big, dropping.

   ... this is not surprising, tbh, as we allocate "1500+100" by default
   (so --max-packet-size 1600 *does* work, two ip fragments, being
   properly reassembled and handled by the server)


 - results are consistent for IPv4 and IPv6 (= UDP payload is smaller
   on --proto udp6, total packet size is the same)

 - mssfix also seems to do what it says on the lid

   09:10:42.860856 IP 10.194.0.1.774 > 10.194.3.9.37123: Flags [S.], seq 2812517040, ack 1148395195, win 65535, options [mss 32,nop,wscale 6,sackOK,TS val 574950381 ecr 3397293635], length 0

   (for a --max-packet-size 154 session)

 - if I poke the bear with --max-packet-size 100, *TLS* MTU will be 
   ugprade to 154 (as the warning says), but --mssfix will be broken,
   as it installs the "not constrained_int()" value to ce.mssfix

   +        int maxmtu = positive_atoi(p[1]);
   +        options->ce.tls_mtu = constrain_int(maxmtu, TLS_CHANNEL_MTU_MIN, TLS_CH
   ...
   +        options->ce.mssfix = maxmtu;

   (this will result in a mssfix value <= 0, so "no mssfix activity")

 - --maxpacket-size 113 over udp6 will result in

   09:14:47.172084 IP 10.194.0.1.774 > 10.194.3.9.50937: Flags [S.], seq 3614955968, ack 1939523050, win 65535, options [mss 0,nop,wscale 6,sackOK,TS val 371801058 ecr 3794849425], length 0

   ... which the sending stack seems to treat with "no idea what the other
   end wants, I ignore this".  114 leads to "mss 1", which also is not
   handled by the other end as "only 1 byte segments", but I see 204 bytes
   - so FreeBSD seems to set that as minimum MSS.

   I think a followup patch should maybe use "ce.tls_mtu" (constrained)
   and not the raw argument here.  Less confusion.

 - for --max-packet-size 500 ("reasonable values"), implccit mssfix mtu
   works as expected.

 - --max-packet-size 145 + tls-auth --> works

 - --max-packet-size 145 + tls-crypt --> works, though only 138 bytes

 - --max-packet-size 145 + tls-crypt-v2 --> works, though only 138 bytes

   the last 3 have been added to the "server test / master t_client.rc"
   testbed.



I have added the oxford comma, as requested, and also a "--" before
"max-packet-size" in that line.  The other two wording fixes are not
needed as that comment got completely rewritten.  Found another typo
("udp , ipv6").

Also, I have updated the man page to show "154" as the correct minimum
in v6, it was still talking about 512 (why are we limiting the maximum
to 2048, btw?).

Your patch has been applied to the master branch.

commit 5f6ea5975927627680c21c10670ccb8503f18249
Author: Arne Schwabe
Date:   Fri Nov 4 13:56:55 2022 +0100

     Allow setting control channel packet size with max-packet-size

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Frank Lichtenheld <frank@lichtenheld.com>
     Message-Id: <20221104125655.656150-2-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25477.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/Changes.rst b/Changes.rst
index fc5a1a853..f25833b40 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -100,6 +100,13 @@  Inline auth username and password
     http-proxy-user-pass too.
 
 
+Improved control channel packet size control (``max-packet-size``)
+    The size of control channel is no longer tied to
+    ``--link-mtu``/``--tun-mtu`` and can be set using ``--max-packet-size``.
+    Sending large control channel frames is also optimised by allowing 6
+    outstanding packets instead of just 4. ``max-packet-size`` will also set
+    ``mssfix`` to try to limit data-channel packets as well.
+
 Deprecated features
 -------------------
 ``inetd`` has been removed
@@ -162,7 +169,9 @@  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 ``--max-packet-size`` now.
+  The default is 1250 for the control channel size.
 - In point-to-point OpenVPN setups (no ``--server``), using
   ``--explict-exit-notiy`` on one end would terminate the other side at
   session end.  This is considered a no longer useful default and has
diff --git a/doc/man-sections/link-options.rst b/doc/man-sections/link-options.rst
index eb098a0f8..6a7f48a59 100644
--- a/doc/man-sections/link-options.rst
+++ b/doc/man-sections/link-options.rst
@@ -172,9 +172,9 @@  the local and the remote host.
   the first place, and if big packets come through anyhow (from protocols
   other than TCP), ``--fragment`` will internally fragment them.
 
-  Both ``--fragment`` and ``--mssfix`` are designed to work around cases
-  where Path MTU discovery is broken on the network path between OpenVPN
-  peers.
+  ``max-packet-size``, ``--fragment`` and ``--mssfix`` are designed to work
+  around cases where Path MTU discovery is broken on the network path
+  between OpenVPN peers.
 
   The usual symptom of such a breakdown is an OpenVPN connection which
   successfully starts, but then stalls during active usage.
@@ -189,6 +189,10 @@  the local and the remote host.
 
      --tun-mtu 1500 --fragment 1300 --mssfix
 
+  If the ``max-packet-size size`` option is used in the configuration
+  it will also act as if ``mssfix size mtu`` was specified in the
+  configuration.
+
 --mtu-disc type
   Should we do Path MTU discovery on TCP/UDP channel? Only supported on
   OSes such as Linux that supports the necessary system call to set.
@@ -465,3 +469,27 @@  the local and the remote host.
      if mode server:
          socket-flags TCP_NODELAY
          push "socket-flags TCP_NODELAY"
+
+--max-packet-size size
+  This option will instruct OpenVPN to try to limit the maximum on-write packet
+  size by restricting the control channel packet size and setting ``--mssfix``.
+
+  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, the control packet maximum size defaults to 1250.
+  The control channel packet size will be restricted to values between
+  512 and 2048. The maximum packet size includes encapsulation overhead like
+  UDP and IP.
+
+  In terms of ``--mssfix`` it will expand to:
+  ::
+
+      mssfix size mtu
+
+  If you need to set ``--mssfix`` for data channel and control channel maximum
+  packet size independently, use ``--max-packet-size`` first, followed by a
+  ``--mssfix`` in the configuration.
+
+  In general the default size of 1250 should work almost universally apart
+  from specific corner cases, especially since IPv6 requires a MTU of 1280
+  or larger.
diff --git a/src/openvpn/common.h b/src/openvpn/common.h
index b94680885..d0eaf5626 100644
--- a/src/openvpn/common.h
+++ b/src/openvpn/common.h
@@ -68,6 +68,19 @@  typedef unsigned long ptr_type;
  */
 #define TLS_CHANNEL_BUF_SIZE 2048
 
+/* TLS control buffer minimum size
+ *
+ * A control frame might have IPv6 header (40 byte),
+ * UDP (8 byte), opcode (1), session id (8),
+ * ACK array with 4 ACKs in non-ACK_V1 packets (25 bytes)
+ * tls-crypt(56) or tls-auth(up to 72). To allow secure
+ * renegotiation (dynamic tls-crypt), we set this minimum
+ * to 154, which only allows 16 byte of payload and should
+ * be considered an absolute minimum and not a good value to
+ * set
+ */
+#define TLS_CHANNEL_MTU_MIN 154
+
 /*
  * 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 626239219..a8f06e260 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2640,6 +2640,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)
     {
@@ -2840,6 +2844,21 @@  do_init_tls_wrap_key(struct context *c)
                                          options->ce.tls_crypt_v2_file,
                                          options->ce.tls_crypt_v2_file_inline);
         }
+        /* We have to ensure that the loaded tls-crypt key is small enough
+         * to fit into the initial hard reset v3 packet */
+        int wkc_len = buf_len(&c->c1.ks.tls_crypt_v2_wkc);
+
+        /* empty ACK/message id, tls-crypt, Opcode, udp , ipv6 */
+        int required_size = 5 + wkc_len + tls_crypt_buf_overhead() + 1 + 8 + 40;
+
+        if (required_size > c->options.ce.tls_mtu)
+        {
+            msg(M_WARN, "ERROR: tls-crypt-v2 client key too large to work with "
+                "requested --max-packet-size %d, requires at least "
+                "--max-packet-size %d. Packets will ignore requested "
+                "maximum packet size", c->options.ce.tls_mtu,
+                required_size);
+        }
     }
 
 
@@ -3187,7 +3206,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,
@@ -3195,7 +3214,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 d4856f166..370806fb5 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 9f5e4b35d..a86cc5cf1 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -826,6 +826,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;
@@ -1712,6 +1713,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);
 
@@ -6457,6 +6459,25 @@  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], "max-packet-size") && p[1] && !p[2])
+    {
+        VERIFY_PERMISSION(OPT_P_MTU|OPT_P_CONNECTION);
+        int maxmtu = positive_atoi(p[1]);
+        options->ce.tls_mtu = constrain_int(maxmtu, TLS_CHANNEL_MTU_MIN, TLS_CHANNEL_BUF_SIZE);
+
+        if (maxmtu < TLS_CHANNEL_MTU_MIN || maxmtu > TLS_CHANNEL_BUF_SIZE)
+        {
+            msg(M_WARN, "Note: max-packet-size value outside of allowed "
+                "control channel packet size (%d to %d), will use %d "
+                "instead.", TLS_CHANNEL_MTU_MIN, TLS_CHANNEL_BUF_SIZE,
+                options->ce.tls_mtu);
+        }
+
+        /* also set mssfix maxmtu mtu */
+        options->ce.mssfix = maxmtu;
+        options->ce.mssfix_default = false;
+        options->ce.mssfix_encap = true;
+    }
 #ifdef ENABLE_FRAGMENT
     else if (streq(p[0], "mtu-dynamic"))
     {
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index 55322baa0..b165ee5b7 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 528fd9026..8eac09048 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -297,8 +297,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,
@@ -323,18 +322,21 @@  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 frames
+     * also need to fit into data channel buffer we have the same
+     * default of 1500 + 100 as data channel buffers have. Increasing
+     * control channel 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);
 }
 
 /**
@@ -1315,9 +1317,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 02046b7fa..646ec581a 100644
--- a/src/openvpn/ssl.h
+++ b/src/openvpn/ssl.h
@@ -166,10 +166,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.
@@ -181,8 +180,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.