search

nm-l2tp / NetworkManager-l2tp

L2TP and L2TP/IPsec support for NetworkManager
GNU General Public License v2.0
486 stars 83 forks source link

Add option to enable PPP Multilink #158

Closed akrpic77 closed 3 years ago

akrpic77 commented 3 years ago

Useful when dealing with big UDP packets. Supported by Windows and Mikrotik. And pppd.

Short explanation of the option can be found here.

I combined Echo and Misc sections before adding checkbox for PPP multilink and spinbutton for MRRU.

akrpic77 commented 3 years ago

l2tp-ppp-multilink-2

dkosovic commented 3 years ago

Looks okay to me, but wondering how did you come up with the default MRRU value of 1600? Is 1600 a recommended default value from some where.

Not an issue with this PR, but thought I'll mention it, the PPP Options dialog box is/was too tall. I'll get around to to reorganizing the PPP Options dialog box into a 3 tabbed GtkNotebook before i release the next version of NetworkManager-l2tp.

akrpic77 commented 3 years ago

Thank you for accepting pull request.

I got the default value from old Multilink PPP RFC. The new specifications removes it and says it must be negotiated with some value. Windows (from Mikrotik link above) has it set at 1614. More concerning, or, incomplete is the upper limit, which I set to 4500, but protocol allows much higher value 2^16-1 (65535).

Regarding gui, there should only be MRRU set to some value or disabled, so the text "Multilink PPP" could be removed. On the tabbed interfaces - yes, I've thought about that too, but this is, as you said, for some other PR. Maybe move IPSec settings there as well.

dkosovic commented 3 years ago

Extract from pppd source code, /* mrru not specified, default to mru */ : https://github.com/ppp-project/ppp/blob/master/pppd/multilink.c#L87

pppd's default MRU is 1500 (with min of 128 and max of 16384).

The only pppd mention of min/max MRRU is the following and that is only a comment that they probably should be insisting on min/max MRRU, so looks like they aren't yet: https://github.com/ppp-project/ppp/blob/master/pppd/lcp.c#L1752

Cisco has a default MRRU of 1524 bytes with range of 128 to 16384. Juniper has a default of 1500 with range of 1500 to 4500.

Your MRRU default of 1600 and 1500 to 4500 range seems as good as any.

Similar to the GUI redundancy, NM_L2TP_KEY_MULTILINK is implied if NM_L2TP_KEY_MRRU is set. Happy to keep the Allow PPP Multilink for now in the GUI. No one has complained about the dialog height, so very, very low priority, I only notice that it is taller than the default screen size with VMware Workstation (which I can resize). It has been something I've been thinking about for a few years, but haven't gotten around to it.

Keeping the IPsec settings in a separate dialog box has some advantages, like matching other NetworkManager-L2TP GUI front ends not maintained in this repository and "IPsec Settings..." button gets greyed out if neither libreswan nor strongswan are detected which is a bit more obvious than a grayed out tab.