ncp patch set breaks --mssfix (and possibly --fragment)

[Gert Döring]

just so it's not lost: ncp patch set breaks --fragment, because a new frame is built and frame_finalize() sets

frame->link_mtu_dynamic = frame->link_mtu

so the --mssfix and most likely --fragment init needs to be re-done in tls_session_update_crypto_params()

[Gert Döring]

adding a bit more context

init.c has

  /* initialize MTU variables */
  do_init_frame (c);  /* calling frame_finalize_options() -> frame_finalize() internally */

  /* initialize TLS MTU variables */
  do_init_frame_tls (c);
...   
  do_init_fragment (c);
...
  /* initialize dynamic MTU variable */
  do_init_mssfix (c);

and these do_init_... functions modify variables in "struct frame" concerning maximum packet size (= to what value should mssfix reduce the MSS in SYN packets).

With the new code, we have ssl.c tls_session_update_crypto_params() which also calls frame_finalize() , overwriting the MTU values in there with the defaults. So we need to reinitialize do_init_mssfix()'ish after the frame_finalize() call, preferably without calling *back* into init.c, or without code duplication.

With just copying these two init functions, this is trivially solved (and I tested that it fixes mssfix, did not yet build a --fragment test suite) - but not pretty, and not good style...

[Steffan Karger]

For --mssfix the proposed fix works, see 0001-Fix-mssfix-when-using-NCP.patch​ for a proposed patch.

For --fragment however, I think things are more complicated. do_init_fragment() does not touch c->c2.frame at all, but operates on a pre-frame-finalize copy of c->c2.frame: c->c2.frame_fragment. I didn't investigate yet, but get the feeling that if I change that more stuff will break... For completeness I did attach a patch that does what you proposed, since I wrote it anyway ;)

[Gert Döring]

So, I finally came around to setting up a reference test server with "--fragment 500" (this cannot be pushed nor enabled dynamically from a ccd/ file, unfortunately). Here's the stanza for t_client.rc:

RUN_TITLE_6="udp / p2pm / top subnet / --fragment 500"
OPENVPN_CONF_6="$OPENVPN_BASE_P2MP --dev tun --proto udp --remote $REMOTE --port 51198 --fragment 500"
EXPECT_IFCONFIG4_6="10.194.6.2"
EXPECT_IFCONFIG6_6="fd00:abcd:194:6::1000"
PING4_HOSTS_6="10.194.6.1 10.194.0.1"
PING6_HOSTS_6="fd00:abcd:194:6::1 fd00:abcd:194:0::1"

(adapt EXPECT_IFCONFIG* addresses for what gets assigned to your client).

Good news - --fragment stuff works just fine, and it turns out I have misread the code. While do_init_fragment() does have a call to frame_set_mtu_dynamic() (which I assumed would interfere with the new frame), but this isn't operating on c2.fragment but on c2.frame_fragment - which is not affected by the new code.

So the "not-needed" patch is, indeed, not needed :-)

As far as --mssfix goes, the other patch is needed - and ACK from me. Tested, works, looks nice, and respects all the source hierarchy stuff plus "no code duplication". Thanks.

Can you please send to the list so I can formally ACK&merge?

[Gert Döring]

commit cbc3c5a9831b44ec7f59e8cb21e19ea364e6c0ee (master)
Author: Steffan Karger
Date: Sat Sep 10 08:11:12 2016 +0200

Fix --mssfix when using NCP

thanks :)

[stipa]

What is the reason we re-initialize frame on NCP negotiation? Isn't it enough just to update frame_extra ?

[stipa]

(asking because of ​https://community.openvpn.net/openvpn/ticket/1140#ticket)