Hi Willy, Thank you for the feedback!
On 25/04/2024 00:12, Willy Tarreau wrote: > Hi! > > On Wed, Apr 24, 2024 at 05:45:03PM +0200, Nicolas CARPi wrote: >> Hello, >> >> On 24 Apr, Dorian Craps wrote: >>> This attached patch uses MPTCP by default instead of TCP on Linux. >> The backward compatibility of MPTCP is indeed a good point toward >> enabling it by default. Nonetheless, I feel your message should include >> a discussion on the security implications of this change. >> >> As you know, v0 had security issues. v1 addresses them, and we can >> likely consider that new attacks targeting this protocol will pop up as >> it becomes widespread. >> >> In fact, that's already the case: >> >> See: CVE-2024-26708: mptcp: really cope with fastopen race >> or CVE-2024-26826: mptcp: fix data re-injection from stale subflow >> or CVE-2024-26782 kernel: mptcp: fix double-free on socket dismantle >> >> The three CVEs above are all from April 2024. >> >> Given that MPTCP v1 is relatively new (2020), and that we do not have >> real assurances that it is at least as secure as plain TCP, I would >> humbly suggest inverting the logic, and making it an opt-in option. >> >> This way, a vulnerability impacting MPTCP would only impact users that >> enabled it, instead of 100% of HAProxy users. In a few years, making it >> the default could be reconsidered. >> >> Please note that I'm simply voicing my concern as a user, and the core >> dev team might have a very different view about these aspects. >> >>> It sounds good to have MPTCP enabled by default >> Except when looking at it through the prism of the increased attack >> surface! ;) >> >>> IPPROTO_MPTCP is defined just in case old libC are being used and >>> don't have the ref. >> Shouldn't it be defined with a value, as per >> https://www.mptcp.dev/faq.html#why-is-ipproto_mptcp-not-defined ? >> (sorry if it's a dumb remark, I'm not a C dev) > > Without going into all the details and comments above, I just want to > say that I'll study this after 3.0 is released, since there's still a > massive amount of stuff to adjust for the release and we're way past > a feature freeze. It makes sense, take your time! > I *am* interested in the feature, which has been > floating around for a few years already. However I tend to agree with > Nicolas that, at least for the principle of least surprise, I'd rather > not have this turned on by default. There might even be security > implications that some are not willing to take yet, and forcing them > to guess that they need to opt out of something is not great in general > (it might change in the future though, once everyone turns it on by > default, like we did for keep-alive, threads, and h2 for example). It makes sense as well! I initially suggested to Dorian to first send a patch where MPTCP is enabled by default because of the low impact (and also after having seen an old comment on that topic [1]). But indeed, doing that in steps sounds safer. [1] https://github.com/haproxy/haproxy/issues/1028#issuecomment-754560146 > I'm also concerned by the change at the socket layer that will make all > new sockets cause two syscalls on systems where this is not enabled, I thought the listening sockets were created only once at startup and having two syscalls would have been fine. I guess it should be easy to remember the failure the first time, to avoid the extra syscalls for the next listening sockets. > and I'd really really prefer that we use the extended syntax for > addresses that offers a lot of flexibility. We can definitely have > "mptcp@address" and probably mptcp as a variant of tcp. >From what I understood, Dorian is now looking at that. It is not clear if he should create a new proto (struct protocol) or modifying it after having called protocol_lookup() in str2sa_range(). Can MPTCP be used as a variant of "rhttp" as well? > Regarding how > we'd deal with the fallback, we'll see, but overall, thinking that> someone > would explicitly configure mptcp and not even get an error if > it cannot bind is problematic to me, because among the most common > causes of trouble are things that everyone ignores (module not loaded, > missing secondary IP, firewall rules etc) that usually cause problems. > Those we can discover at boot time should definitely be reported. With the v1 Dorian suggested where MPTCP is enabled by default, a warning is reported if it is not possible to create an MPTCP socket, and a "plain" TCP one has been created instead. If MPTCP is explicitly asked, I guess it would make sense to fail if it is not possible to create an MPTCP socket, no? > But > let's discuss this in early June instead (I mean, feel free to discuss > the points together, but I'm not going to invest more time on this at > this moment). Thank you! Please note that Dorian is doing this as part of a work with his uni (useful contributions to Open-Source), and I think he would prefer sending the v2 before June, if that's OK. I can look at rebasing it later if needed. If there is no need to implement a fallback if the kernel doesn't support MPTCP, the patch should be smaller, it should be easy to rebase it. Cheers, Matt -- Sponsored by the NGI0 Core fund.
