Fix P2P epoch MTU accounting

[carrerasdarren-cell]

Summary

• pass active data-channel crypto flags through frame and MTU overhead calculations
• use the negotiated TLS session flags when calculating dynamic P2P framing, without writing session state back into the main options structure
• add focused coverage for both epoch and non-epoch P2P packet-ID overhead

This addresses the point-to-point path left after #1074: p2p_ncp_set_options() records CO_EPOCH_DATA_KEY_FORMAT in the TLS session, while the existing MTU calculation previously inferred it from options.imported_protocol_flags.

Testing

• CMake Debug test_ncp and test_crypto
• CMake ASAN/UBSAN test_ncp and test_crypto
• full Debug and ASAN/UBSAN OpenVPN builds
• local P2P TLS loopback with mssfix 1000 mtu: corrected build reports mss_fix:884 on both IPv6-loopback peers; a negative-control build using the old imported-option source reports 888

Signed off under the project DCO.

[ordex]

add a focused mssfix mtu regression test

FTR this is not a regression test because it doesn't check he wiring implemented by this patch.
I.e. after deleting the hunk in ssl.c, which is what makes this patch effective, the test still passes.

[ordex]

Another note: rather than implementing an extra function and throwing it in the SSL machinery, how about adding the two important lines directly in do_deferred_p2p_ncp() which is there exactly for this purpose (moving state out of the TLS session into the options)?

It already does something similar with use_peer_id (for example).

Moreover, you would re-use the already existing guard, that is slightly different from what you are using in this patch.

As a side note, this patch now makes openvpn print the protocol options and it revealed that other flags are not translated too. Extending this patch to address this issue is out of scope (unless you want to :)), but this is just a reminder that this needs fixing too.

[schwabe]

And more to the point. We probably should not rely on things written back to options but rather ensure that frame_calculate_dynamic gets the information from the right place.

[carrerasdarren-cell]

Thanks for the review. Updated in 3473397:

• removed p2p_ncp_update_options() and the write-back into the main options structure
• threaded the active data-channel crypto flags through frame/MTU calculation and now pass session->opt->crypto_flags from the TLS update path
• adjusted the focused test to exercise both epoch and non-epoch P2P overhead

I also ran a local production-path P2P TLS loopback with mssfix 1000 mtu. The updated build reports mss_fix:884 on both IPv6-loopback peers. Rebuilding with only the TLS call changed back to options->imported_protocol_flags reports 888, so the loopback negative control covers the wiring called out above. Debug and ASAN/UBSAN focused tests and full binary builds pass.

[ordex]

Am I wrong or you are using the imported_protocol_flags without unsetting/setting them anymore? I don't fully understand this change.

My original suggestion (to be acked/unacked by @schwabe ) was to reduce the patch something like:

--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2550,6 +2550,12 @@ do_deferred_p2p_ncp(struct context *c)

     struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];

+    /* P2P NCP stores the negotiated epoch data key format in the TLS session,
+     * but the dynamic frame calculation reads it from the main options, so
+     * mirror it here. Clear it first in case a previous negotiation set it. */
+    c->options.imported_protocol_flags &= ~CO_EPOCH_DATA_KEY_FORMAT;
+    c->options.imported_protocol_flags |= session->opt->crypto_flags & CO_EPOCH_DATA_KEY_FORMAT;
+
     const char *ncp_cipher =
         get_p2p_ncp_cipher(session, c->c2.tls_multi->peer_info, &c->options.gc);

and be done with it (plus unit-tests).

Wouldn't that work? @schwabe

[carrerasdarren-cell]

The remaining imported_protocol_flags call sites are the pre-negotiation/default calculations, where there is not yet an active TLS session to read from. The effective P2P recalculation no longer uses that field: do_deferred_p2p_ncp() calls tls_session_update_crypto_params(), and that path now passes session->opt->crypto_flags directly into frame_calculate_dynamic(). Path-MTU and OCC recalculations similarly pass c->c2.crypto_options.flags from the active data-channel state. That is why the local TLS loopback changes from 888 to 884 even though options.imported_protocol_flags remains unset in P2P mode.

Your smaller do_deferred_p2p_ncp() synchronization would fix this specific flag, including clearing it on a subsequent negotiation. I moved away from that version in response to @schwabe’s point that frame calculation should receive the information from its authoritative source rather than mirror TLS-session state into options. I am happy to reduce it to the six-line synchronization if that is the maintainer consensus; for now I have left the tested direct-source version unchanged so the two approaches can be resolved without another speculative rewrite.

[carrerasdarren-cell]

Added a production-linked regression in 4faf9cc. The existing TLS P2P system test now runs both peers with mssfix 1000 mtu and requires the negotiated epoch framing to produce mss_fix:884. The full t_cltsrv.sh test passes locally.

As a negative control, changing only the ssl.c call back to options->imported_protocol_flags makes both peers report mss_fix:888, so this test fails if the production wiring hunk is removed. I left the production implementation unchanged while the direct-source versus do_deferred_p2p_ncp() synchronization direction is being resolved.