[Openvpn-devel] [PATCH 1/6] Remove leftover frame_set_mtu_dynamic definitions in mtu.h

[Openvpn-devel] [PATCH 1/6] Remove leftover frame_set_mtu_dynamic definitions in mtu.h

[Arne Schwabe]

The frame_set_mtu_dynamic function and it defines were removed during
the buffer rework but this definitions were overlooked.

Signed-off-by: Arne Schwabe <arne@...1227...>
---
 src/openvpn/mtu.h | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/src/openvpn/mtu.h b/src/openvpn/mtu.h
index 7f967e066..4b8feca7c 100644
--- a/src/openvpn/mtu.h
+++ b/src/openvpn/mtu.h
@@ -253,16 +253,6 @@ unsigned int
 calc_packet_id_size_dc(const struct options *options,
                        const struct key_type *kt);
 
-
-/*
- * frame_set_mtu_dynamic and flags
- */
-
-#define SET_MTU_TUN         (1<<0) /* use tun/tap rather than link sizing */
-#define SET_MTU_UPPER_BOUND (1<<1) /* only decrease dynamic MTU */
-
-void frame_set_mtu_dynamic(struct frame *frame, int mtu, unsigned int flags);
-
 /*
  * allocate a buffer for socket or tun layer
  */
-- 
2.32.1 (Apple Git-133)

[Openvpn-devel] [PATCH 2/6] Inline frame_add_to_extra_tun function and remove frame_defined

[Arne Schwabe]

This function has only one usage and is so trivial that an extra
function makes little sense anymore.

frame_defined is no longer used, so remove the function.

Signed-off-by: Arne Schwabe <arne@...1227...>
---
 src/openvpn/init.c |  2 +-
 src/openvpn/mtu.h  | 16 ----------------
 2 files changed, 1 insertion(+), 17 deletions(-)

diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index f06afc298..6cdcef628 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -3047,7 +3047,7 @@ do_init_frame(struct context *c)
      */
     if (c->options.ce.tun_mtu_extra_defined)
     {
-        frame_add_to_extra_tun(&c->c2.frame, c->options.ce.tun_mtu_extra);
+        c->c2.frame.extra_tun += c->options.ce.tun_mtu_extra;
     }
 
     /*
diff --git a/src/openvpn/mtu.h b/src/openvpn/mtu.h
index 4b8feca7c..9db6cf26a 100644
--- a/src/openvpn/mtu.h
+++ b/src/openvpn/mtu.h
@@ -274,20 +274,4 @@ const char *format_extended_socket_error(int fd, int *mtu, struct gc_arena *gc);
 
 #endif
 
-/*
- * frame member adjustment functions
- */
-
-static inline void
-frame_add_to_extra_tun(struct frame *frame, const int increment)
-{
-    frame->extra_tun += increment;
-}
-
-static inline bool
-frame_defined(const struct frame *frame)
-{
-    return frame->buf.payload_size > 0;
-}
-
 #endif /* ifndef MTU_H */
-- 
2.32.1 (Apple Git-133)

[Openvpn-devel] [PATCH 3/6] Extract update_session_cipher into standalone function

[Arne Schwabe]

This allow the code later to check if the cipher is okay to use and
update it for the calculation for the max MTU size.

Signed-off-by: Arne Schwabe <arne@...1227...>
---
 src/openvpn/ssl.c     | 11 +----------
 src/openvpn/ssl_ncp.c | 22 ++++++++++++++++++++++
 src/openvpn/ssl_ncp.h |  8 ++++++++
 3 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 61dea996d..ddd90080b 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1678,17 +1678,8 @@ tls_session_update_crypto_params(struct tls_session *session,
                                  struct frame *frame_fragment,
                                  struct link_socket_info *lsi)
 {
-
-    bool cipher_allowed_as_fallback = options->enable_ncp_fallback
-                                      && streq(options->ciphername, session->opt->config_ciphername);
-
-    if (!session->opt->server && !cipher_allowed_as_fallback
-        && !tls_item_in_cipher_list(options->ciphername, options->ncp_ciphers))
+    if (!update_session_cipher(session, options))
     {
-        msg(D_TLS_ERRORS, "Error: negotiated cipher not allowed - %s not in %s",
-            options->ciphername, options->ncp_ciphers);
-        /* undo cipher push, abort connection setup */
-        options->ciphername = session->opt->config_ciphername;
         return false;
     }
 
diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
index 564942503..c800f718f 100644
--- a/src/openvpn/ssl_ncp.c
+++ b/src/openvpn/ssl_ncp.c
@@ -490,3 +490,25 @@ p2p_mode_ncp(struct tls_multi *multi, struct tls_session *session)
 
     gc_free(&gc);
 }
+
+
+bool
+update_session_cipher(struct tls_session *session, struct options *options)
+{
+    bool cipher_allowed_as_fallback = options->enable_ncp_fallback
+                                      && streq(options->ciphername, session->opt->config_ciphername);
+
+    if (!session->opt->server && !cipher_allowed_as_fallback
+        && !tls_item_in_cipher_list(options->ciphername, options->ncp_ciphers))
+    {
+        msg(D_TLS_ERRORS, "Error: negotiated cipher not allowed - %s not in %s",
+            options->ciphername, options->ncp_ciphers);
+        /* undo cipher push, abort connection setup */
+        options->ciphername = session->opt->config_ciphername;
+        return false;
+    }
+    else
+    {
+        return true;
+    }
+}
diff --git a/src/openvpn/ssl_ncp.h b/src/openvpn/ssl_ncp.h
index 853017f5f..5ba2f7ae7 100644
--- a/src/openvpn/ssl_ncp.h
+++ b/src/openvpn/ssl_ncp.h
@@ -148,4 +148,12 @@ const char *
 get_p2p_ncp_cipher(struct tls_session *session, const char *peer_info,
                    struct gc_arena *gc);
 
+
+/**
+ * Checks if the cipher is allowed and updates the TLS session cipher with it,
+ * otherwise returns false
+ */
+bool
+update_session_cipher(struct tls_session *session, struct options *options);
+
 #endif /* ifndef OPENVPN_SSL_NCP_H */
-- 
2.32.1 (Apple Git-133)

[Openvpn-devel] [PATCH 4/6] Implement a function to calculate the default MTU

[Arne Schwabe]

We could also just hardcode this value to 1420 but this approach does
not add much (complicated) code and it is a bit better than to have
a magic number to just be there.

Signed-off-by: Arne Schwabe <arne@...1227...>
---
 src/openvpn/mtu.c                      | 22 ++++++++++++++++++++++
 src/openvpn/mtu.h                      | 14 ++++++++++++++
 tests/unit_tests/openvpn/test_crypto.c | 19 ++++++++++++++++++-
 3 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/src/openvpn/mtu.c b/src/openvpn/mtu.c
index 59b917985..8afc16394 100644
--- a/src/openvpn/mtu.c
+++ b/src/openvpn/mtu.c
@@ -205,6 +205,28 @@ calc_options_string_link_mtu(const struct options *o, const struct frame *frame)
     return payload + overhead;
 }
 
+int
+frame_calculate_default_mtu(struct options *o)
+{
+    struct options options = *o;
+
+    /* assume we have peer_id enabled */
+    options.use_peer_id = true;
+
+    /* We use IPv6+UDP here to have a consistent size for tun MTU no matter
+     * the combination of udp/tcp and IPv4/IPv6 */
+    int encap_overhead = datagram_overhead(AF_INET6, PROTO_UDP);
+
+    struct key_type kt;
+    init_key_type(&kt, "AES-256-GCM", "none", true, false);
+
+    size_t payload_overhead = frame_calculate_payload_overhead(0, &options, &kt);
+    size_t protocol_overhead = frame_calculate_protocol_header_size(&kt, &options, false);
+
+    return MTU_ENCAP_DEFAULT - encap_overhead - payload_overhead - protocol_overhead;
+
+}
+
 void
 frame_print(const struct frame *frame,
             int level,
diff --git a/src/openvpn/mtu.h b/src/openvpn/mtu.h
index 9db6cf26a..d643027d3 100644
--- a/src/openvpn/mtu.h
+++ b/src/openvpn/mtu.h
@@ -79,6 +79,10 @@
  */
 #define MSSFIX_DEFAULT     1492
 
+/* The default size we aim to reach to with our VPN packets by setting
+ * the MTU accordingly */
+#define MTU_ENCAP_DEFAULT   1492
+
 /*
  * Alignment of payload data such as IP packet or
  * ethernet frame.
@@ -260,6 +264,16 @@ void alloc_buf_sock_tun(struct buffer *buf,
                         const struct frame *frame,
                         const bool tuntap_buffer);
 
+
+/**
+ * Function to calculate the default MTU for Layer 3 VPNs. The function
+ * assumes that UDP packets should be a maximum of \c MTU_ENCAP_DEFAULT (1492)
+ * with a AEAD cipher. This default comes out to be 1420.
+ */
+int
+frame_calculate_default_mtu(struct options *o);
+
+
 /*
  * EXTENDED_SOCKET_ERROR_CAPABILITY functions -- print extra error info
  * on socket errors, such as PMTU size.  As of 2003.05.11, only works
diff --git a/tests/unit_tests/openvpn/test_crypto.c b/tests/unit_tests/openvpn/test_crypto.c
index 83572b827..ca595b0a5 100644
--- a/tests/unit_tests/openvpn/test_crypto.c
+++ b/tests/unit_tests/openvpn/test_crypto.c
@@ -477,6 +477,22 @@ test_mssfix_mtu_calculation(void **state)
     gc_free(&gc);
 }
 
+
+static void
+test_mtu_default_calculation(void **state)
+{
+    struct options o = {0};
+
+    /* common defaults */
+    o.ce.tun_mtu = 1400;
+    o.ce.mssfix = 1000;
+    o.replay = true;
+    o.ce.proto = PROTO_UDP;
+
+    size_t mtu = frame_calculate_default_mtu(&o);
+    assert_int_equal(1420, mtu);
+}
+
 int
 main(void)
 {
@@ -487,7 +503,8 @@ main(void)
         cmocka_unit_test(crypto_test_hmac),
         cmocka_unit_test(test_des_encrypt),
         cmocka_unit_test(test_occ_mtu_calculation),
-        cmocka_unit_test(test_mssfix_mtu_calculation)
+        cmocka_unit_test(test_mssfix_mtu_calculation),
+        cmocka_unit_test(test_mtu_default_calculation)
     };
 
 #if defined(ENABLE_CRYPTO_OPENSSL)
-- 
2.32.1 (Apple Git-133)

[Openvpn-devel] [PATCH 5/6] Change default MTU in server mode to 1420 and push it to client

[Arne Schwabe]

This changes the default MTU of the tun-mtu to 1420 to avoid MTU related
issues that are even more prominent when DCO server or clients are involved.

To maximise compatibility to lie our MTU in the default OCC message and also
push the real MTU to clients that support pushing the MTU.

Signed-off-by: Arne Schwabe <arne@...1227...>
---
 Changes.rst                              |  5 ++++
 doc/man-sections/vpn-network-options.rst | 36 ++++++++++++++++++++----
 src/openvpn/options.c                    | 31 ++++++++++++++++++--
 src/openvpn/options.h                    |  1 +
 src/openvpn/push.c                       | 16 +++++++++++
 5 files changed, 81 insertions(+), 8 deletions(-)

diff --git a/Changes.rst b/Changes.rst
index 67a23c792..79b79d608 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -141,6 +141,11 @@ 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.
+- the default of ``--tun-mtu`` has been changed to ``--tun 1420 1500`` when
+  running in server mode. This will create an mtu mismatch with older client
+  (newer client allow pushable mtu) but the most common server platforms
+  (Linux and FreeBSD) allow receiving 1500 byte packets even tun-mtu is set
+  to 1420, still allowing larger packets from client with 1500 byte mtu.
 
 Overview of changes in 2.5
 ==========================
diff --git a/doc/man-sections/vpn-network-options.rst b/doc/man-sections/vpn-network-options.rst
index 5b2f84707..2e4fff5df 100644
--- a/doc/man-sections/vpn-network-options.rst
+++ b/doc/man-sections/vpn-network-options.rst
@@ -500,21 +500,45 @@ routing.
   arguments of ``--ifconfig`` to mean "address netmask", no longer "local
   remote".
 
---tun-mtu n
-  Take the TUN device MTU to be **n** and derive the link MTU from it
-  (default :code:`1500`). In most cases, you will probably want to leave
-  this parameter set to its default value.
+--tun-mtu args
+
+  Valid syntaxes:
+  ::
+
+      tun-mtu tun-mtu
+      tun-mtu tun-mtu occ-mtu
+
+  Take the TUN device MTU to be ``tun-mtu`` and derive the link MTU from it.
+  In most cases, you will probably want to leave this parameter set to
+  its default value.
+
+  Starting with OpenVPN 2.6 in when running server mode (``--mode server``,
+  ``--server`` or ``-server-ipv6`` options present in the configuration),
+  the default will be 1420 for the tun mtu size and 1500 for the ``occ-mtu``.
+
+  The OCC MTU can be used to avoid warnings about mismatched MTU from
+  clients. If :code:`occ-mtu` is not specified, it will to default to the
+  tun-mtu
 
   The MTU (Maximum Transmission Units) is the maximum datagram size in
   bytes that can be sent unfragmented over a particular network path.
   OpenVPN requires that packets on the control and data channels be sent
   unfragmented.
 
+  It is generally advisable to set the tun MTU low enough that with the
+  encapsulation overhead is lower than the MTU of the network that is used
+  to transport the VPN packets.
+
   MTU problems often manifest themselves as connections which hang during
   periods of active usage.
 
-  It's best to use the ``--fragment`` and/or ``--mssfix`` options to deal
-  with MTU sizing issues.
+  If lowering the tun MTU to avoid MTU related problems (e.g. when tap is used
+  and an MTU of 1500 is rdquired), the ``--fragment`` and/or ``--mssfix``
+  options can be also used to deal with MTU sizing issues.
+
+  Note: Depending on the platform, the operating system allows to receive
+  packets larger than ``tun-mtu`` (e.g. Linux and FreeBSD) but other platforms
+  (like macOS) limit received packets to the same size as the MTU.
 
 --tun-mtu-extra n
   Assume that the TUN/TAP device might return as many as ``n`` bytes more
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 9a0634a5e..69c839fb6 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -814,6 +814,7 @@ init_options(struct options *o, const bool init_gc)
     o->status_file_version = 1;
     o->ce.bind_local = true;
     o->ce.tun_mtu = TUN_MTU_DEFAULT;
+    o->ce.occ_mtu = 0;
     o->ce.link_mtu = LINK_MTU_DEFAULT;
     o->ce.mtu_discover_type = -1;
     o->ce.mssfix = 0;
@@ -3031,6 +3032,16 @@ options_postprocess_mutate_ce(struct options *o, struct connection_entry *ce)
         if (!ce->tun_mtu_defined && !ce->link_mtu_defined)
         {
             ce->tun_mtu_defined = true;
+            if (o->mode == MODE_SERVER)
+            {
+                /* If we are running in P2MP mode we default to a MTU
+                 * that is low enough by default to fit into a 1492
+                 * MTU UDP IPv6 packet:
+                 *
+                 */
+                ce->tun_mtu = frame_calculate_default_mtu(o);
+                ce->occ_mtu = TUN_MTU_DEFAULT;
+            }
         }
         if ((dev == DEV_TYPE_TAP) && !ce->tun_mtu_extra_defined)
         {
@@ -4018,7 +4029,15 @@ options_string(const struct options *o,
     buf_printf(&out, ",link-mtu %u",
                (unsigned int) calc_options_string_link_mtu(o, frame));
 
-    buf_printf(&out, ",tun-mtu %d", frame->tun_mtu);
+    if (o->ce.occ_mtu != 0)
+    {
+        buf_printf(&out, ",tun-mtu %d", o->ce.occ_mtu);
+    }
+    else
+    {
+        buf_printf(&out, ",tun-mtu %d", frame->tun_mtu);
+    }
+
     buf_printf(&out, ",proto %s",  proto_remote(o->ce.proto, remote));
 
     bool p2p_nopull = o->mode == MODE_POINT_TO_POINT && !PULL_DEFINED(o);
@@ -6262,11 +6281,19 @@ add_option(struct options *options,
         options->ce.link_mtu = positive_atoi(p[1]);
         options->ce.link_mtu_defined = true;
     }
-    else if (streq(p[0], "tun-mtu") && p[1] && !p[2])
+    else if (streq(p[0], "tun-mtu") && p[1] && !p[3])
     {
         VERIFY_PERMISSION(OPT_P_MTU|OPT_P_CONNECTION);
         options->ce.tun_mtu = positive_atoi(p[1]);
         options->ce.tun_mtu_defined = true;
+        if (p[2])
+        {
+            options->ce.occ_mtu = positive_atoi(p[2]);
+        }
+        else
+        {
+            options->ce.occ_mtu = 0;
+        }
     }
     else if (streq(p[0], "tun-mtu-extra") && p[1] && !p[2])
     {
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index c2937dc37..1085a462a 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -118,6 +118,7 @@ struct connection_entry
     const char *socks_proxy_authfile;
 
     int tun_mtu;         /* MTU of tun device */
+    int occ_mtu;         /* if non-null, this is the MTU we announce to peers in OCC */
     bool tun_mtu_defined; /* true if user overriding parm with command line option */
     int tun_mtu_extra;
     bool tun_mtu_extra_defined;
diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 63257348a..463957a82 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -603,6 +603,22 @@ prepare_push_reply(struct context *c, struct gc_arena *gc,
     {
         push_option_fmt(gc, push_list, M_USAGE, "key-derivation tls-ekm");
     }
+
+    /* Push our mtu to the peer if it supports pushable MTUs */
+    int client_max_mtu = 0;
+    const char *iv_mtu = extract_var_peer_info(tls_multi->peer_info, "IV_MTU=", gc);
+
+    if (iv_mtu &&  sscanf(iv_mtu, "%d", &client_max_mtu) == 1)
+    {
+        push_option_fmt(gc, push_list, M_USAGE, "tun-mtu %d", o->ce.tun_mtu);
+        if (client_max_mtu < o->ce.tun_mtu)
+        {
+            msg(M_WARN, "Warning reported maximum MTU from client (%d) is lower "
+                "than MTU used on the server (%d). Add tun-max-mtu %d "
+                "to client configuration.", client_max_mtu,
+                o->ce.tun_mtu, o->ce.tun_mtu);
+        }
+    }
     return true;
 }
 
-- 
2.32.1 (Apple Git-133)

[Openvpn-devel] [PATCH 6/6] Allow tun-mtu to be pushed

[Arne Schwabe]

This allows tun-mtu to pushed but only up to the size of the preallocated
buffers. This is not a perfect solution but should allow most of the use
cases where the mtu is close enough to 1500.

Signed-off-by: Arne Schwabe <arne@...1227...>
---
 Changes.rst                              |  8 ++++
 doc/man-sections/client-options.rst      |  4 ++
 doc/man-sections/vpn-network-options.rst |  5 +++
 src/openvpn/init.c                       | 52 ++++++++++++++++++++----
 src/openvpn/mtu.c                        |  1 +
 src/openvpn/mtu.h                        |  3 ++
 src/openvpn/options.c                    | 15 ++++++-
 src/openvpn/options.h                    |  2 +
 src/openvpn/ssl.c                        |  3 ++
 9 files changed, 85 insertions(+), 8 deletions(-)

diff --git a/Changes.rst b/Changes.rst
index 79b79d608..e99671bcb 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -79,6 +79,14 @@ Cookie based handshake for UDP server
     shake. The tls-crypt-v2 option allows controlling if older clients are
     accepted.
 
+
+Tun MTU can be pushed
+    As part of changing the ``--tun-mtu`` default to 1420 (see below), the
+    client can now also dynamically configure its MTU and the server will
+    try to push the client MTU when the client supports it. The directive
+    ``--tun-mtu-max`` has been introduced to specify the maximum pushable
+    MTU size.
+
 Deprecated features
 -------------------
 ``inetd`` has been removed
diff --git a/doc/man-sections/client-options.rst b/doc/man-sections/client-options.rst
index 8e0e4f18a..230e51e8d 100644
--- a/doc/man-sections/client-options.rst
+++ b/doc/man-sections/client-options.rst
@@ -358,6 +358,10 @@ configuration.
         The client announces the list of supported ciphers configured with the
         ``--data-ciphers`` option to the server.
 
+  :code:`IV_MTU=<max_mtu>`
+        The client announces the support of pushable MTU and the maximum MTU
+        the client is willing to accept.
+
   :code:`IV_GUI_VER=<gui_id> <version>`
         The UI version of a UI if one is running, for example
         :code:`de.blinkt.openvpn 0.5.47` for the Android app.
diff --git a/doc/man-sections/vpn-network-options.rst b/doc/man-sections/vpn-network-options.rst
index 2e4fff5df..71aa3f4c7 100644
--- a/doc/man-sections/vpn-network-options.rst
+++ b/doc/man-sections/vpn-network-options.rst
@@ -540,6 +540,11 @@ routing.
   packets larger than ``tun-mtu`` (e.g. Linux and FreeBSD) but other platforms
   (like macOS) limit received packets to the same size as the MTU.
 
+--tun-max-mtu maxmtu
+  This configures the maximum MTU size that a server can push to ``maxmtu``.
+  The default for ``maxmtu`` is 1600. This will increase internal buffers
+  allocation for larger packet sizes.
+
 --tun-mtu-extra n
   Assume that the TUN/TAP device might return as many as ``n`` bytes more
   than the ``--tun-mtu`` size on read. This parameter defaults to 0, which
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 6cdcef628..e9f9778a3 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2126,7 +2126,8 @@ pull_permission_mask(const struct context *c)
         | OPT_P_ECHO
         | OPT_P_PULL_MODE
         | OPT_P_PEER_ID
-        | OPT_P_NCP;
+        | OPT_P_NCP
+        | OPT_P_PUSH_MTU;
 
     if (!c->options.route_nopull)
     {
@@ -2283,12 +2284,39 @@ do_deferred_options(struct context *c, const unsigned int found)
 #endif
 
         struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
+        if (!update_session_cipher(session, &c->options))
+        {
+            /* The update_session_cipher method wil already print an error */
+            return false;
+        }
+
+        /* Cipher is considered safe, so we can use it to calculate the max
+         * MTU size */
+        if (found & OPT_P_PUSH_MTU)
+        {
+            /* MTU has changed, check that the pushed MTU is small enough to
+             * be able to change it */
+            msg(D_PUSH, "OPTIONS IMPORT: tun-mtu set to %d", c->options.ce.tun_mtu);
+
+            struct frame *frame = &c->c2.frame;
+
+            if (c->options.ce.tun_mtu > frame->tun_max_mtu)
+            {
+                msg(D_PUSH_ERRORS, "Server pushed a large mtu, please add "
+                    "tun-mtu-max %d in the client configuration",
+                    c->options.ce.tun_mtu);
+            }
+            frame->tun_mtu = min_int(frame->tun_max_mtu, c->options.ce.tun_mtu);
+        }
+
         if (!tls_session_update_crypto_params(session, &c->options, &c->c2.frame,
                                               frame_fragment, get_link_socket_info(c)))
         {
             msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to import crypto options");
             return false;
         }
+
+
     }
 
     return true;
@@ -2446,10 +2474,16 @@ frame_finalize_options(struct context *c, const struct options *o)
     struct frame *frame = &c->c2.frame;
 
     frame->tun_mtu = get_frame_mtu(c, o);
+    frame->tun_max_mtu = o->ce.tun_mtu_max;
+
+    /* max mtu needs to be at least as large as the tun mtu */
+    frame->tun_max_mtu = max_int(frame->tun_mtu, frame->tun_max_mtu);
 
-    /* We always allow at least 1500 MTU packets to be received in our buffer
-     * space */
-    size_t payload_size = max_int(1500, frame->tun_mtu);
+    /* We always allow at least 1600 MTU packets to be received in our buffer
+     * space to allow server to push "baby giant MTU sizes */
+    frame->tun_max_mtu = max_int(1600, frame->tun_max_mtu);
+
+    size_t payload_size = frame->tun_max_mtu;
 
     /* The extra tun needs to be added to the payload size */
     if (o->ce.tun_mtu_defined)
@@ -2457,9 +2491,9 @@ frame_finalize_options(struct context *c, const struct options *o)
         payload_size += o->ce.tun_mtu_extra;
     }
 
-    /* Add 100 byte of extra space in the buffer to account for slightly
-     * mismatched MUTs between peers */
-    payload_size += 100;
+    /* Add 32 byte of extra space in the buffer to account for small errors
+     * in the calculation */
+    payload_size += 32;
 
 
     /* the space that is reserved before the payload to add extra headers to it
@@ -2992,6 +3026,10 @@ do_init_frame_tls(struct context *c)
                c->c2.frame.buf.payload_size);
         frame_print(&c->c2.tls_multi->opt.frame, D_MTU_INFO,
                     "Control Channel MTU parms");
+
+        /* Keep the max mtu also in the frame of tls multi so it can access
+         * it in push_peer_info */
+        c->c2.tls_multi->opt.frame.tun_max_mtu = c->c2.frame.tun_max_mtu;
     }
     if (c->c2.tls_auth_standalone)
     {
diff --git a/src/openvpn/mtu.c b/src/openvpn/mtu.c
index 8afc16394..d883569c8 100644
--- a/src/openvpn/mtu.c
+++ b/src/openvpn/mtu.c
@@ -244,6 +244,7 @@ frame_print(const struct frame *frame,
     buf_printf(&out, " max_frag:%d", frame->max_fragment_size);
 #endif
     buf_printf(&out, " tun_mtu:%d", frame->tun_mtu);
+    buf_printf(&out, " tun_max_mtu:%d", frame->tun_max_mtu);
     buf_printf(&out, " headroom:%d", frame->buf.headroom);
     buf_printf(&out, " payload:%d", frame->buf.payload_size);
     buf_printf(&out, " tailroom:%d", frame->buf.tailroom);
diff --git a/src/openvpn/mtu.h b/src/openvpn/mtu.h
index d643027d3..e80d8bd01 100644
--- a/src/openvpn/mtu.h
+++ b/src/openvpn/mtu.h
@@ -137,6 +137,9 @@ struct frame {
                                  *   control frame payload (although most of
                                  *   code ignores it)
                                  */
+    int tun_max_mtu;            /**< the maximum tun-mtu size the buffers are
+                                 *   are sized for. This is the upper bound that
+                                 *   a server can push as MTU */
 
     int extra_tun;              /**< Maximum number of bytes in excess of
                                  *   the tun/tap MTU that might be read
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 69c839fb6..7a07daa40 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -6283,7 +6283,7 @@ add_option(struct options *options,
     }
     else if (streq(p[0], "tun-mtu") && p[1] && !p[3])
     {
-        VERIFY_PERMISSION(OPT_P_MTU|OPT_P_CONNECTION);
+        VERIFY_PERMISSION(OPT_P_PUSH_MTU|OPT_P_CONNECTION);
         options->ce.tun_mtu = positive_atoi(p[1]);
         options->ce.tun_mtu_defined = true;
         if (p[2])
@@ -6295,6 +6295,19 @@ add_option(struct options *options,
             options->ce.occ_mtu = 0;
         }
     }
+    else if (streq(p[0], "tun-mtu-max") && p[1] && !p[3])
+    {
+        VERIFY_PERMISSION(OPT_P_MTU|OPT_P_CONNECTION);
+        int max_mtu = positive_atoi(p[1]);
+        if (max_mtu < 68 || max_mtu > 65536)
+        {
+            msg(msglevel, "--tun-mtu-max value '%s' is invalid", p[1]);
+        }
+        else
+        {
+            options->ce.tun_mtu_max = max_mtu;
+        }
+    }
     else if (streq(p[0], "tun-mtu-extra") && p[1] && !p[2])
     {
         VERIFY_PERMISSION(OPT_P_MTU|OPT_P_CONNECTION);
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index 1085a462a..5a1720ca9 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -119,6 +119,7 @@ struct connection_entry
 
     int tun_mtu;         /* MTU of tun device */
     int occ_mtu;         /* if non-null, this is the MTU we announce to peers in OCC */
+    int tun_mtu_max;     /* maximum MTU that can be pushed */
     bool tun_mtu_defined; /* true if user overriding parm with command line option */
     int tun_mtu_extra;
     bool tun_mtu_extra_defined;
@@ -720,6 +721,7 @@ struct options
 #define OPT_P_CONNECTION      (1<<27)
 #define OPT_P_PEER_ID         (1<<28)
 #define OPT_P_INLINE          (1<<29)
+#define OPT_P_PUSH_MTU        (1<<30)
 
 #define OPT_P_DEFAULT   (~(OPT_P_INSTANCE|OPT_P_PULL_MODE))
 
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index ddd90080b..a6071e3c1 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1939,6 +1939,9 @@ push_peer_info(struct buffer *buf, struct tls_session *session)
         {
             iv_proto |= IV_PROTO_REQUEST_PUSH;
             iv_proto |= IV_PROTO_AUTH_PENDING_KW;
+
+            /* support for tun-mtu as part of the push message */
+            buf_printf(&out, "IV_MTU=%d\n", session->opt->frame.tun_max_mtu);
         }
 
         /* support for Negotiable Crypto Parameters */
-- 
2.32.1 (Apple Git-133)

[Openvpn-devel] [PATCH applied] Re: Remove leftover frame_set_mtu_dynamic definitions in mtu.h

[Gert Doering]

Acked-by: Gert Doering <gert@...1296...>

Trivial enough.  Totally untested.  Just verified with "git grep".

Your patch has been applied to the master branch.

commit e2df31b6835eb15af6524d67f6fd93ff7a3cc4dd
Author: Arne Schwabe
Date:   Tue Jun 21 18:16:44 2022 +0200

     Remove leftover frame_set_mtu_dynamic definitions in mtu.h

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


--
kind regards,

Gert Doering

[Openvpn-devel] [PATCH applied] Re: Inline frame_add_to_extra_tun function and remove frame_defined

[Gert Doering]

Acked-by: Gert Doering <gert@...1296...>

Trivial enough.  Only compile tested (+stare at code).

Your patch has been applied to the master branch.

commit 65c6308608e5a637564db0cf241a16501190342a
Author: Arne Schwabe
Date:   Tue Jun 21 18:16:45 2022 +0200

     Inline frame_add_to_extra_tun function and remove frame_defined

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


--
kind regards,

Gert Doering

Re: [Openvpn-devel] [PATCH 3/6] Extract update_session_cipher into standalone function

[Frank Lichtenheld]

Acked-By: Frank Lichtenheld <frank@...2641...>

Trivial code move.

On Tue, Jun 21, 2022 at 06:16:46PM +0200, Arne Schwabe wrote:
> This allow the code later to check if the cipher is okay to use and
> update it for the calculation for the max MTU size.
> 
> Signed-off-by: Arne Schwabe <arne@...1227...>
> ---
>  src/openvpn/ssl.c     | 11 +----------
>  src/openvpn/ssl_ncp.c | 22 ++++++++++++++++++++++
>  src/openvpn/ssl_ncp.h |  8 ++++++++
>  3 files changed, 31 insertions(+), 10 deletions(-)
> 
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 61dea996d..ddd90080b 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -1678,17 +1678,8 @@ tls_session_update_crypto_params(struct tls_session *session,
>                                   struct frame *frame_fragment,
>                                   struct link_socket_info *lsi)
>  {
> -
> -    bool cipher_allowed_as_fallback = options->enable_ncp_fallback
> -                                      && streq(options->ciphername, session->opt->config_ciphername);
> -
> -    if (!session->opt->server && !cipher_allowed_as_fallback
> -        && !tls_item_in_cipher_list(options->ciphername, options->ncp_ciphers))
> +    if (!update_session_cipher(session, options))
>      {
> -        msg(D_TLS_ERRORS, "Error: negotiated cipher not allowed - %s not in %s",
> -            options->ciphername, options->ncp_ciphers);
> -        /* undo cipher push, abort connection setup */
> -        options->ciphername = session->opt->config_ciphername;
>          return false;
>      }
>  
> diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
> index 564942503..c800f718f 100644
> --- a/src/openvpn/ssl_ncp.c
> +++ b/src/openvpn/ssl_ncp.c
> @@ -490,3 +490,25 @@ p2p_mode_ncp(struct tls_multi *multi, struct tls_session *session)
>  
>      gc_free(&gc);
>  }
> +
> +
> +bool
> +update_session_cipher(struct tls_session *session, struct options *options)
> +{
> +    bool cipher_allowed_as_fallback = options->enable_ncp_fallback
> +                                      && streq(options->ciphername, session->opt->config_ciphername);
> +
> +    if (!session->opt->server && !cipher_allowed_as_fallback
> +        && !tls_item_in_cipher_list(options->ciphername, options->ncp_ciphers))
> +    {
> +        msg(D_TLS_ERRORS, "Error: negotiated cipher not allowed - %s not in %s",
> +            options->ciphername, options->ncp_ciphers);
> +        /* undo cipher push, abort connection setup */
> +        options->ciphername = session->opt->config_ciphername;
> +        return false;
> +    }
> +    else
> +    {
> +        return true;
> +    }
> +}
> diff --git a/src/openvpn/ssl_ncp.h b/src/openvpn/ssl_ncp.h
> index 853017f5f..5ba2f7ae7 100644
> --- a/src/openvpn/ssl_ncp.h
> +++ b/src/openvpn/ssl_ncp.h
> @@ -148,4 +148,12 @@ const char *
>  get_p2p_ncp_cipher(struct tls_session *session, const char *peer_info,
>                     struct gc_arena *gc);
>  
> +
> +/**
> + * Checks if the cipher is allowed and updates the TLS session cipher with it,
> + * otherwise returns false
> + */
> +bool
> +update_session_cipher(struct tls_session *session, struct options *options);
> +
>  #endif /* ifndef OPENVPN_SSL_NCP_H */
> -- 
> 2.32.1 (Apple Git-133)
> 
> 
> 
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH 4/6] Implement a function to calculate the default MTU

[Frank Lichtenheld]

NACK, doesn't build:

mtu.c: In function ‘frame_calculate_default_mtu’:
mtu.c:223:31: error: too few arguments to function ‘frame_calculate_payload_overhead’
  223 |     size_t payload_overhead = frame_calculate_payload_overhead(0, &options, &kt);
      |                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
mtu.c:111:1: note: declared here
  111 | frame_calculate_payload_overhead(const struct frame *frame,
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
make[3]: *** [Makefile:754: mtu.o] Error 1

On Tue, Jun 21, 2022 at 06:16:47PM +0200, Arne Schwabe wrote:
> We could also just hardcode this value to 1420 but this approach does
> not add much (complicated) code and it is a bit better than to have
> a magic number to just be there.
> 
> Signed-off-by: Arne Schwabe <arne@...1227...>
> ---
>  src/openvpn/mtu.c                      | 22 ++++++++++++++++++++++
>  src/openvpn/mtu.h                      | 14 ++++++++++++++
>  tests/unit_tests/openvpn/test_crypto.c | 19 ++++++++++++++++++-
>  3 files changed, 54 insertions(+), 1 deletion(-)

Re: [Openvpn-devel] [PATCH 5/6] Change default MTU in server mode to 1420 and push it to client

[Frank Lichtenheld]

Code changes look fine to me, but several documentation suggestions.

On Tue, Jun 21, 2022 at 06:16:48PM +0200, Arne Schwabe wrote:
> This changes the default MTU of the tun-mtu to 1420 to avoid MTU related
> issues that are even more prominent when DCO server or clients are involved.
> 
> To maximise compatibility to lie our MTU in the default OCC message and also
> push the real MTU to clients that support pushing the MTU.
> 
> Signed-off-by: Arne Schwabe <arne@...1227...>
> ---
>  Changes.rst                              |  5 ++++
>  doc/man-sections/vpn-network-options.rst | 36 ++++++++++++++++++++----
>  src/openvpn/options.c                    | 31 ++++++++++++++++++--
>  src/openvpn/options.h                    |  1 +
>  src/openvpn/push.c                       | 16 +++++++++++
>  5 files changed, 81 insertions(+), 8 deletions(-)
> 
> diff --git a/Changes.rst b/Changes.rst
> index 67a23c792..79b79d608 100644
> --- a/Changes.rst
> +++ b/Changes.rst
> @@ -141,6 +141,11 @@ 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.
> +- the default of ``--tun-mtu`` has been changed to ``--tun 1420 1500`` when

"--tun" -> "--tun-mtu"

> +  running in server mode. This will create an mtu mismatch with older client

"clients"

> +  (newer client allow pushable mtu) but the most common server platforms

"clients"

> +  (Linux and FreeBSD) allow receiving 1500 byte packets even tun-mtu is set
> +  to 1420, still allowing larger packets from client with 1500 byte mtu.

"clients"

>  
>  Overview of changes in 2.5
>  ==========================
> diff --git a/doc/man-sections/vpn-network-options.rst b/doc/man-sections/vpn-network-options.rst
> index 5b2f84707..2e4fff5df 100644
> --- a/doc/man-sections/vpn-network-options.rst
> +++ b/doc/man-sections/vpn-network-options.rst
> @@ -500,21 +500,45 @@ routing.
>    arguments of ``--ifconfig`` to mean "address netmask", no longer "local
>    remote".
>  
> ---tun-mtu n
> -  Take the TUN device MTU to be **n** and derive the link MTU from it
> -  (default :code:`1500`). In most cases, you will probably want to leave
> -  this parameter set to its default value.
> +--tun-mtu args
> +
> +  Valid syntaxes:
> +  ::
> +
> +      tun-mtu tun-mtu
> +      tun-mtu tun-mtu occ-mtu
> +
> +  Take the TUN device MTU to be ``tun-mtu`` and derive the link MTU from it.
> +  In most cases, you will probably want to leave this parameter set to
> +  its default value.
> +
> +  Starting with OpenVPN 2.6 in when running server mode (``--mode server``,

"in when running" -> "when running in"

> +  ``--server`` or ``-server-ipv6`` options present in the configuration),

", or" Oxford comma FTW!

> +  the default will be 1420 for the tun mtu size and 1500 for the ``occ-mtu``.
> +
> +  The OCC MTU can be used to avoid warnings about mismatched MTU from
> +  clients. If :code:`occ-mtu` is not specified, it will to default to the

Maybe explicitely "older clients"?

> +  tun-mtu

Missing full stop.

>  
>    The MTU (Maximum Transmission Units) is the maximum datagram size in
>    bytes that can be sent unfragmented over a particular network path.
>    OpenVPN requires that packets on the control and data channels be sent
>    unfragmented.
>  
> +  It is generally advisable to set the tun MTU low enough that with the
> +  encapsulation overhead is lower than the MTU of the network that is used
> +  to transport the VPN packets.

I know what you want to say ;) But as written it makes no sense. Maybe
trying another way:

Note that the tun MTU must be lower than the MTU of the actual network that is
used to transport the VPN packets. That is due to the additional encapsulation
applied as part of the OpenVPN protocol. The default is chosen to be safe
with a network MTU of 1500 independently of the used OpenVPN options (which
lead to slightly different encapsulation overhead).

> +
>    MTU problems often manifest themselves as connections which hang during
>    periods of active usage.
>  
> -  It's best to use the ``--fragment`` and/or ``--mssfix`` options to deal
> -  with MTU sizing issues.
> +  If lowering the tun MTU to avoid MTU related problems (e.g. when tap is used
> +  and an MTU of 1500 is rdquired), the ``--fragment`` and/or ``--mssfix``

"required"

> +  options can be also used to deal with MTU sizing issues.
> +
> +  Note: Depending on the platform, the operating system allows to receive
> +  packets larger than ``tun-mtu`` (e.g. Linux and FreeBSD) but other platforms
> +  (like macOS) limit received packets to the same size as the MTU.
>  
>  --tun-mtu-extra n
>    Assume that the TUN/TAP device might return as many as ``n`` bytes more
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 9a0634a5e..69c839fb6 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -814,6 +814,7 @@ init_options(struct options *o, const bool init_gc)
>      o->status_file_version = 1;
>      o->ce.bind_local = true;
>      o->ce.tun_mtu = TUN_MTU_DEFAULT;
> +    o->ce.occ_mtu = 0;
>      o->ce.link_mtu = LINK_MTU_DEFAULT;
>      o->ce.mtu_discover_type = -1;
>      o->ce.mssfix = 0;
> @@ -3031,6 +3032,16 @@ options_postprocess_mutate_ce(struct options *o, struct connection_entry *ce)
>          if (!ce->tun_mtu_defined && !ce->link_mtu_defined)
>          {
>              ce->tun_mtu_defined = true;
> +            if (o->mode == MODE_SERVER)
> +            {
> +                /* If we are running in P2MP mode we default to a MTU
> +                 * that is low enough by default to fit into a 1492
> +                 * MTU UDP IPv6 packet:
> +                 *
> +                 */
> +                ce->tun_mtu = frame_calculate_default_mtu(o);
> +                ce->occ_mtu = TUN_MTU_DEFAULT;
> +            }
>          }
>          if ((dev == DEV_TYPE_TAP) && !ce->tun_mtu_extra_defined)
>          {
> @@ -4018,7 +4029,15 @@ options_string(const struct options *o,
>      buf_printf(&out, ",link-mtu %u",
>                 (unsigned int) calc_options_string_link_mtu(o, frame));
>  
> -    buf_printf(&out, ",tun-mtu %d", frame->tun_mtu);
> +    if (o->ce.occ_mtu != 0)
> +    {
> +        buf_printf(&out, ",tun-mtu %d", o->ce.occ_mtu);
> +    }
> +    else
> +    {
> +        buf_printf(&out, ",tun-mtu %d", frame->tun_mtu);
> +    }
> +
>      buf_printf(&out, ",proto %s",  proto_remote(o->ce.proto, remote));
>  
>      bool p2p_nopull = o->mode == MODE_POINT_TO_POINT && !PULL_DEFINED(o);
> @@ -6262,11 +6281,19 @@ add_option(struct options *options,
>          options->ce.link_mtu = positive_atoi(p[1]);
>          options->ce.link_mtu_defined = true;
>      }
> -    else if (streq(p[0], "tun-mtu") && p[1] && !p[2])
> +    else if (streq(p[0], "tun-mtu") && p[1] && !p[3])
>      {
>          VERIFY_PERMISSION(OPT_P_MTU|OPT_P_CONNECTION);
>          options->ce.tun_mtu = positive_atoi(p[1]);
>          options->ce.tun_mtu_defined = true;
> +        if (p[2])
> +        {
> +            options->ce.occ_mtu = positive_atoi(p[2]);
> +        }
> +        else
> +        {
> +            options->ce.occ_mtu = 0;
> +        }
>      }
>      else if (streq(p[0], "tun-mtu-extra") && p[1] && !p[2])
>      {
> diff --git a/src/openvpn/options.h b/src/openvpn/options.h
> index c2937dc37..1085a462a 100644
> --- a/src/openvpn/options.h
> +++ b/src/openvpn/options.h
> @@ -118,6 +118,7 @@ struct connection_entry
>      const char *socks_proxy_authfile;
>  
>      int tun_mtu;         /* MTU of tun device */
> +    int occ_mtu;         /* if non-null, this is the MTU we announce to peers in OCC */
>      bool tun_mtu_defined; /* true if user overriding parm with command line option */
>      int tun_mtu_extra;
>      bool tun_mtu_extra_defined;
> diff --git a/src/openvpn/push.c b/src/openvpn/push.c
> index 63257348a..463957a82 100644
> --- a/src/openvpn/push.c
> +++ b/src/openvpn/push.c
> @@ -603,6 +603,22 @@ prepare_push_reply(struct context *c, struct gc_arena *gc,
>      {
>          push_option_fmt(gc, push_list, M_USAGE, "key-derivation tls-ekm");
>      }
> +
> +    /* Push our mtu to the peer if it supports pushable MTUs */
> +    int client_max_mtu = 0;
> +    const char *iv_mtu = extract_var_peer_info(tls_multi->peer_info, "IV_MTU=", gc);
> +
> +    if (iv_mtu &&  sscanf(iv_mtu, "%d", &client_max_mtu) == 1)

random space before sscanf

> +    {
> +        push_option_fmt(gc, push_list, M_USAGE, "tun-mtu %d", o->ce.tun_mtu);
> +        if (client_max_mtu < o->ce.tun_mtu)
> +        {
> +            msg(M_WARN, "Warning reported maximum MTU from client (%d) is lower "

Missing colon after "Warning".

> +                "than MTU used on the server (%d). Add tun-max-mtu %d "
> +                "to client configuration.", client_max_mtu,
> +                o->ce.tun_mtu, o->ce.tun_mtu);
> +        }
> +    }
>      return true;
>  }

Re: [Openvpn-devel] [PATCH 5/6] Change default MTU in server mode to 1420 and push it to client

[Frank Lichtenheld]

On Fri, Jun 24, 2022 at 10:35:24AM +0200, Frank Lichtenheld wrote:
> Code changes look fine to me, but several documentation suggestions.

Hmm, after reading the next patch might I suggest to move the
change to push.c actually to the next patch? Both references
to IV_MTU and tun-max-mtu only make sense then, right?
 
> On Tue, Jun 21, 2022 at 06:16:48PM +0200, Arne Schwabe wrote:
> > diff --git a/src/openvpn/push.c b/src/openvpn/push.c
> > index 63257348a..463957a82 100644
> > --- a/src/openvpn/push.c
> > +++ b/src/openvpn/push.c
> > @@ -603,6 +603,22 @@ prepare_push_reply(struct context *c, struct gc_arena *gc,
> >      {
> >          push_option_fmt(gc, push_list, M_USAGE, "key-derivation tls-ekm");
> >      }
> > +
> > +    /* Push our mtu to the peer if it supports pushable MTUs */
> > +    int client_max_mtu = 0;
> > +    const char *iv_mtu = extract_var_peer_info(tls_multi->peer_info, "IV_MTU=", gc);
> > +
> > +    if (iv_mtu &&  sscanf(iv_mtu, "%d", &client_max_mtu) == 1)
> 
> random space before sscanf
> 
> > +    {
> > +        push_option_fmt(gc, push_list, M_USAGE, "tun-mtu %d", o->ce.tun_mtu);
> > +        if (client_max_mtu < o->ce.tun_mtu)
> > +        {
> > +            msg(M_WARN, "Warning reported maximum MTU from client (%d) is lower "
> 
> Missing colon after "Warning".
> 
> > +                "than MTU used on the server (%d). Add tun-max-mtu %d "
> > +                "to client configuration.", client_max_mtu,
> > +                o->ce.tun_mtu, o->ce.tun_mtu);
> > +        }
> > +    }
> >      return true;
> >  }
> 
> 
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH 6/6] Allow tun-mtu to be pushed

[Frank Lichtenheld]

Only skimmed this. A few small typo fixes and the like.

On Tue, Jun 21, 2022 at 06:16:49PM +0200, Arne Schwabe wrote:
> This allows tun-mtu to pushed but only up to the size of the preallocated
> buffers. This is not a perfect solution but should allow most of the use
> cases where the mtu is close enough to 1500.
> 
> Signed-off-by: Arne Schwabe <arne@...1227...>
> ---
>  Changes.rst                              |  8 ++++
>  doc/man-sections/client-options.rst      |  4 ++
>  doc/man-sections/vpn-network-options.rst |  5 +++
>  src/openvpn/init.c                       | 52 ++++++++++++++++++++----
>  src/openvpn/mtu.c                        |  1 +
>  src/openvpn/mtu.h                        |  3 ++
>  src/openvpn/options.c                    | 15 ++++++-
>  src/openvpn/options.h                    |  2 +
>  src/openvpn/ssl.c                        |  3 ++
>  9 files changed, 85 insertions(+), 8 deletions(-)
> 
> diff --git a/Changes.rst b/Changes.rst
> index 79b79d608..e99671bcb 100644
> --- a/Changes.rst
> +++ b/Changes.rst
> @@ -79,6 +79,14 @@ Cookie based handshake for UDP server
>      shake. The tls-crypt-v2 option allows controlling if older clients are
>      accepted.
>  
> +
> +Tun MTU can be pushed
> +    As part of changing the ``--tun-mtu`` default to 1420 (see below), the
> +    client can now also dynamically configure its MTU and the server will
> +    try to push the client MTU when the client supports it. The directive
> +    ``--tun-mtu-max`` has been introduced to specify the maximum pushable
> +    MTU size.
> +
>  Deprecated features
>  -------------------
>  ``inetd`` has been removed
> diff --git a/doc/man-sections/client-options.rst b/doc/man-sections/client-options.rst
> index 8e0e4f18a..230e51e8d 100644
> --- a/doc/man-sections/client-options.rst
> +++ b/doc/man-sections/client-options.rst
> @@ -358,6 +358,10 @@ configuration.
>          The client announces the list of supported ciphers configured with the
>          ``--data-ciphers`` option to the server.
>  
> +  :code:`IV_MTU=<max_mtu>`
> +        The client announces the support of pushable MTU and the maximum MTU
> +        the client is willing to accept.

"it" instead of repeating "the client"?

> +
>    :code:`IV_GUI_VER=<gui_id> <version>`
>          The UI version of a UI if one is running, for example
>          :code:`de.blinkt.openvpn 0.5.47` for the Android app.
> diff --git a/doc/man-sections/vpn-network-options.rst b/doc/man-sections/vpn-network-options.rst
> index 2e4fff5df..71aa3f4c7 100644
> --- a/doc/man-sections/vpn-network-options.rst
> +++ b/doc/man-sections/vpn-network-options.rst
> @@ -540,6 +540,11 @@ routing.
>    packets larger than ``tun-mtu`` (e.g. Linux and FreeBSD) but other platforms
>    (like macOS) limit received packets to the same size as the MTU.
>  
> +--tun-max-mtu maxmtu
> +  This configures the maximum MTU size that a server can push to ``maxmtu``.
> +  The default for ``maxmtu`` is 1600. This will increase internal buffers
> +  allocation for larger packet sizes.
> +
>  --tun-mtu-extra n
>    Assume that the TUN/TAP device might return as many as ``n`` bytes more
>    than the ``--tun-mtu`` size on read. This parameter defaults to 0, which
> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> index 6cdcef628..e9f9778a3 100644
> --- a/src/openvpn/init.c
> +++ b/src/openvpn/init.c
> @@ -2126,7 +2126,8 @@ pull_permission_mask(const struct context *c)
>          | OPT_P_ECHO
>          | OPT_P_PULL_MODE
>          | OPT_P_PEER_ID
> -        | OPT_P_NCP;
> +        | OPT_P_NCP
> +        | OPT_P_PUSH_MTU;
>  
>      if (!c->options.route_nopull)
>      {
> @@ -2283,12 +2284,39 @@ do_deferred_options(struct context *c, const unsigned int found)
>  #endif
>  
>          struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
> +        if (!update_session_cipher(session, &c->options))
> +        {
> +            /* The update_session_cipher method wil already print an error */

"will"

> +            return false;
> +        }
> +
> +        /* Cipher is considered safe, so we can use it to calculate the max
> +         * MTU size */
> +        if (found & OPT_P_PUSH_MTU)
> +        {
> +            /* MTU has changed, check that the pushed MTU is small enough to
> +             * be able to change it */
> +            msg(D_PUSH, "OPTIONS IMPORT: tun-mtu set to %d", c->options.ce.tun_mtu);
> +
> +            struct frame *frame = &c->c2.frame;
> +
> +            if (c->options.ce.tun_mtu > frame->tun_max_mtu)
> +            {
> +                msg(D_PUSH_ERRORS, "Server pushed a large mtu, please add "
> +                    "tun-mtu-max %d in the client configuration",
> +                    c->options.ce.tun_mtu);
> +            }
> +            frame->tun_mtu = min_int(frame->tun_max_mtu, c->options.ce.tun_mtu);
> +        }
> +
>          if (!tls_session_update_crypto_params(session, &c->options, &c->c2.frame,
>                                                frame_fragment, get_link_socket_info(c)))
>          {
>              msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to import crypto options");
>              return false;
>          }
> +
> +

Spurious whitespace change.

>      }
>  
>      return true;
> @@ -2446,10 +2474,16 @@ frame_finalize_options(struct context *c, const struct options *o)
>      struct frame *frame = &c->c2.frame;
>  
>      frame->tun_mtu = get_frame_mtu(c, o);
> +    frame->tun_max_mtu = o->ce.tun_mtu_max;
> +
> +    /* max mtu needs to be at least as large as the tun mtu */
> +    frame->tun_max_mtu = max_int(frame->tun_mtu, frame->tun_max_mtu);
>  
> -    /* We always allow at least 1500 MTU packets to be received in our buffer
> -     * space */
> -    size_t payload_size = max_int(1500, frame->tun_mtu);
> +    /* We always allow at least 1600 MTU packets to be received in our buffer
> +     * space to allow server to push "baby giant MTU sizes */

Missing "

> +    frame->tun_max_mtu = max_int(1600, frame->tun_max_mtu);
> +
> +    size_t payload_size = frame->tun_max_mtu;
>  
>      /* The extra tun needs to be added to the payload size */
>      if (o->ce.tun_mtu_defined)
[...]

Re: [Openvpn-devel] [PATCH 5/6] Change default MTU in server mode to 1420 and push it to client

[Arne Schwabe]

Am 24.06.22 um 10:38 schrieb Frank Lichtenheld:
> On Fri, Jun 24, 2022 at 10:35:24AM +0200, Frank Lichtenheld wrote:
>> Code changes look fine to me, but several documentation suggestions.
> 
> Hmm, after reading the next patch might I suggest to move the
> change to push.c actually to the next patch? Both references
> to IV_MTU and tun-max-mtu only make sense then, right?

Well basically this implements the server side functionality while the 
next patch implements the client side. And the changes to push.c are 
server side. I could also reorder these two to first introduce the 
client side support and then make use of it.

Arne

Re: [Openvpn-devel] [PATCH 5/6] Change default MTU in server mode to 1420 and push it to client

[Gert Doering]

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

Hi,

On Tue, Jun 21, 2022 at 06:16:48PM +0200, Arne Schwabe wrote:
> This changes the default MTU of the tun-mtu to 1420 to avoid MTU related
> issues that are even more prominent when DCO server or clients are involved.

I'm not convinced that this change "by default" is a desirable change.

Being able to do that (pushable MTU) is good, but changing the default is
fairly fundamental, and it will break use cases that rely on 1500 inner
MTU (and outside fragmentation). 

Originally, it was said that "with DCO, outside fragmentation is not
possible", but if I remember right, this turned out to be incorrect,
and implementations handle outside fragmentation just fine.  So there
seems to be no hard pressing need to change the default (I am willing
to be corrected if I misremember something).

Not saying that outside fragmentation is *desirable*, so for TCP, 
"mssfix 1492 mtu" will (already) ensure we have no fragmentation
issues - whatever tun-mtu we have.

gert

-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             gert@...1296...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]

Re: [Openvpn-devel] [PATCH 5/6] Change default MTU in server mode to 1420 and push it to client

[Arne Schwabe]

Am 24.06.22 um 12:35 schrieb Gert Doering:
> Hi,
> 
> On Tue, Jun 21, 2022 at 06:16:48PM +0200, Arne Schwabe wrote:
>> This changes the default MTU of the tun-mtu to 1420 to avoid MTU related
>> issues that are even more prominent when DCO server or clients are involved.
> 
> I'm not convinced that this change "by default" is a desirable change.
> 
> Being able to do that (pushable MTU) is good, but changing the default is
> fairly fundamental, and it will break use cases that rely on 1500 inner
> MTU (and outside fragmentation).
> 
> Originally, it was said that "with DCO, outside fragmentation is not
> possible", but if I remember right, this turned out to be incorrect,
> and implementations handle outside fragmentation just fine.  So there
> seems to be no hard pressing need to change the default (I am willing
> to be corrected if I misremember something).
> 
> Not saying that outside fragmentation is *desirable*, so for TCP,
> "mssfix 1492 mtu" will (already) ensure we have no fragmentation
> issues - whatever tun-mtu we have.

I understand that this is quite fundamental change but I think 
alternative of always using a wrong MTU and hoping that mssfix and 
outside fragmentation are going to save the day what we currently do, is 
lying to ourselves.

I still think this is a reasonable change. Yes, it might break in some 
very obscure setups but for those setups, people can still set the MTU 
back to 1500. Tap still uses the 1500 default anyway.

Arne

Re: [Openvpn-devel] [PATCH 5/6] Change default MTU in server mode to 1420 and push it to client

[Gert Doering]

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

Hi,

On Fri, Jun 24, 2022 at 12:52:23PM +0200, Arne Schwabe wrote:
> I still think this is a reasonable change. Yes, it might break in some 
> very obscure setups but for those setups, people can still set the MTU 
> back to 1500. Tap still uses the 1500 default anyway.

It will break all setups that have openvpn "in the path" and something
else that throws away ICMP fragmentation required messages.

Like

  Host A ---(1500)--> OpenVPN --(1400)--> Host B

if Host A sends a 1500 byte packet with DF bit (or IPv6), the network
stack on the OpenVPN server needs to return an ICMP "packet too big"
message.  If that gets lost - and there are too many folks that do throw
away ICMP packets - we have a black hole.


We see this in commercial VPN setups as well - having a large-enough
MTU for the occasional UDP (or whatever else) packet, and ensuring TCP
packets are of the desirable packet size by means of MSS manipulation
is more robust.

So, still NAK on changing the default.  Turning your argument around: 
whoever things they need this can enable this on their own.

gert
-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             gert@...1296...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]