| Message ID | 20220625234150.3398864-4-arne@rfc2549.org |
|---|---|
| State | Deferred |
| Headers | show |
| Series | [Openvpn-devel,v3,1/5] Extract update_session_cipher into standalone function | expand |
NACK. See below. On Sun, Jun 26, 2022 at 01:41:49AM +0200, Arne Schwabe wrote: > 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); As mentioned this is true for the specific options configured above. But you can easily also get different values out of this function by changing the options because frame_calculate_payload overhead does not always return the same value. There are various things you could do: 1) you could add a test here to show that this is expected behavior. 2) you could add a comment here to the same effect (although I would prefer an actual test) 3) you could change the function so that it actually always returns the same value (e.g. by overriding fragment and comp settings like you do for use_peer_id) But doing nothing is not an option, IMHO. That will just confuse others in the future. Regards, Frank
On Montag, 27. Juni 2022 10:36:02 CEST Frank Lichtenheld wrote: > As mentioned this is true for the specific options configured above. > But you can easily also get different values out of this function by > changing the options because frame_calculate_payload overhead does > not always return the same value. I don't get it. It produces the same output for the same input reliably. Do you want to see a test which forces something different to 1420 by modifying the inputs accordingly? Heiko
On Tue, Oct 11, 2022 at 03:07:04PM +0200, Heiko Hund wrote: > On Montag, 27. Juni 2022 10:36:02 CEST Frank Lichtenheld wrote: > > As mentioned this is true for the specific options configured above. > > But you can easily also get different values out of this function by > > changing the options because frame_calculate_payload overhead does > > not always return the same value. > > I don't get it. It produces the same output for the same input reliably. Do > you want to see a test which forces something different to 1420 by modifying > the inputs accordingly? Yes, I would like to know (i.e. having the correct behavior being documented by a test) what should happen if options->ce.fragment is set, or options->comp.alg. Because in the code comments it sounds like this will *always* produce 1420, and I don't actually believed that that is true (although I do not remember whether I actually tested this or just read the code). Regards,
diff --git a/src/openvpn/mtu.c b/src/openvpn/mtu.c index 8fc45b341..e4d4a9cd5 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, false); + 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 db6bddb42..e80d8bd01 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. @@ -263,6 +267,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)
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@rfc2549.org> --- 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(-)