Re: [PATCH] FEATURE: Adding MPTCP with option to disable it and fall-back to TCP
Hi Matthieu, On Fri, Aug 09, 2024 at 12:52:04PM +0200, Matthieu Baerts wrote: > On 09/08/2024 11:32, Willy Tarreau wrote: > > On Mon, May 06, 2024 at 02:10:02PM +0200, Björn Jacke wrote: > >> Hi, > >> > >> I came up a while ago with a patchset for MPTCP support for HAProxy also, > >> see https://github.com/haproxy/haproxy/issues/1028 > >> > >> Back then I also discussed some ideas with Willy how to implement it more > >> elegantly in HAProxy. It's still somewhere on my todo list but > >> unfortunately > >> I didn't catch that up since then. As I had a good real-world usecase > >> recently for MPTCP I though of looking into this again also. > > > > I had a look at this series, and am seeing that the largest part of the > > changes is to use ->sock_prot (either IPPROTO_TCP or IPPROTO_MPTCP) in > > getsockopt() and setsockopt() calls, which is not in Dorian's patchset. > > > > Thus I'm just wondering if this is mandatory or if there's a compatibility > > mode in the kernel so that IPPROTO_TCP also works for MPTCP sockets. Maybe > > Matthieu knows better and could enlighten us on this ? > > There is no need to change anything in the getsockopt() and setsockopt() > calls: it is still possible to use SOL_TCP (== IPPROTO_TCP). The kernel > will try to adapt the socket option to the MPTCP case, e.g. applying > something on all the subflows, or just the first one, or the MPTCP > socket, depending on the option. > > There are some socket options under SOL_MPTCP (284), but there are > "new", and specific to MPTCP, e.g. to get info about the different paths > being used, etc. > > If you replace IPPROTO_TCP in the getsockopt() and setsockopt() calls by > IPPROTO_MPTCP (262), it means you try to look at socket options with the > SOL_X25 level, that's not what we want here ;) OK, thank you for the very detailed explanation! > > Because if it's required, we definitely need to apply similar changes > > everywhere (the code has probably evolved a little bit since 2021), and > > if not, it could mean that only the part that performs the protocol > > registration (that Matthieu+Dorian also handled) and the doc should > > suffice. I think we have everything in shape now to decide what's left > > to do in order to get the feature merged. I hope this time we won't > > miss the deadline! > > In theory, the last patch I sent should be enough. > > BTW, thank you for your other reply, no issues for the delay. I will try > to look at the modifications you requested when I can find some time :) That works for me. We'll release by end of november, with a hard freeze around a month earlier, so there's no rush, though it always helps to get feedback from users, of course! If you'd figure that you can't find the time to complete it in a reasonable delay, do not hesitate to ping me again about it to let me know, we'll find how to arrange this together. Also, I think it would be kind to leave a mention about Björn's work in your commit message, who attempted the very first implementation 3-4 years ago when haproxy's internals were probably less ready to deal with this, causing his work to be left pending for a while. Thanks! Willy
Re: [PATCH] FEATURE: Adding MPTCP with option to disable it and fall-back to TCP
Hi Willy, On 09/08/2024 11:32, Willy Tarreau wrote: > Hi Björn, > > I'm coming back to this: > > On Mon, May 06, 2024 at 02:10:02PM +0200, Björn Jacke wrote: >> Hi, >> >> I came up a while ago with a patchset for MPTCP support for HAProxy also, >> see https://github.com/haproxy/haproxy/issues/1028 >> >> Back then I also discussed some ideas with Willy how to implement it more >> elegantly in HAProxy. It's still somewhere on my todo list but unfortunately >> I didn't catch that up since then. As I had a good real-world usecase >> recently for MPTCP I though of looking into this again also. > > I had a look at this series, and am seeing that the largest part of the > changes is to use ->sock_prot (either IPPROTO_TCP or IPPROTO_MPTCP) in > getsockopt() and setsockopt() calls, which is not in Dorian's patchset. > > Thus I'm just wondering if this is mandatory or if there's a compatibility > mode in the kernel so that IPPROTO_TCP also works for MPTCP sockets. Maybe > Matthieu knows better and could enlighten us on this ? There is no need to change anything in the getsockopt() and setsockopt() calls: it is still possible to use SOL_TCP (== IPPROTO_TCP). The kernel will try to adapt the socket option to the MPTCP case, e.g. applying something on all the subflows, or just the first one, or the MPTCP socket, depending on the option. There are some socket options under SOL_MPTCP (284), but there are "new", and specific to MPTCP, e.g. to get info about the different paths being used, etc. If you replace IPPROTO_TCP in the getsockopt() and setsockopt() calls by IPPROTO_MPTCP (262), it means you try to look at socket options with the SOL_X25 level, that's not what we want here ;) > Because if it's required, we definitely need to apply similar changes > everywhere (the code has probably evolved a little bit since 2021), and > if not, it could mean that only the part that performs the protocol > registration (that Matthieu+Dorian also handled) and the doc should > suffice. I think we have everything in shape now to decide what's left > to do in order to get the feature merged. I hope this time we won't > miss the deadline! In theory, the last patch I sent should be enough. BTW, thank you for your other reply, no issues for the delay. I will try to look at the modifications you requested when I can find some time :) Cheers, Matt -- Sponsored by the NGI0 Core fund.
Re: [PATCH] FEATURE: Adding MPTCP with option to disable it and fall-back to TCP
Hi Björn, I'm coming back to this: On Mon, May 06, 2024 at 02:10:02PM +0200, Björn Jacke wrote: > Hi, > > I came up a while ago with a patchset for MPTCP support for HAProxy also, > see https://github.com/haproxy/haproxy/issues/1028 > > Back then I also discussed some ideas with Willy how to implement it more > elegantly in HAProxy. It's still somewhere on my todo list but unfortunately > I didn't catch that up since then. As I had a good real-world usecase > recently for MPTCP I though of looking into this again also. I had a look at this series, and am seeing that the largest part of the changes is to use ->sock_prot (either IPPROTO_TCP or IPPROTO_MPTCP) in getsockopt() and setsockopt() calls, which is not in Dorian's patchset. Thus I'm just wondering if this is mandatory or if there's a compatibility mode in the kernel so that IPPROTO_TCP also works for MPTCP sockets. Maybe Matthieu knows better and could enlighten us on this ? Because if it's required, we definitely need to apply similar changes everywhere (the code has probably evolved a little bit since 2021), and if not, it could mean that only the part that performs the protocol registration (that Matthieu+Dorian also handled) and the doc should suffice. I think we have everything in shape now to decide what's left to do in order to get the feature merged. I hope this time we won't miss the deadline! Thanks, Willy
Re: [PATCH] FEATURE: Adding MPTCP with option to disable it and fall-back to TCP
Hi Matthieu, first, sorry for the long delay, but each time it's the same, the list of pending urgent things drags me away. I'm back on this. On Mon, Jun 03, 2024 at 05:33:31PM +0200, Matthieu Baerts wrote: > >>> 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(). > > > > I *tend* to think that a new struct protocol is easier to add and > > maintain. It could just share most (if not all) of the functions > > with tcp, and probably be declared in the same file for ease of > > sharing code. > > > > I'm seeing that in the comment you linked above I also proposed a > > keyword for "bind" and "server" lines, like we have "tfo" to enable > > TCP fast-open. It tends to be slightly easier to implement but then > > requires more care everywhere because such options do no apply to > > all protocols, so if you have "mptcp on" on a "bind quic4@:443" line, > > that's quite confusing so it deserves extra checks to make sure it > > is not silently ignored. Also I do have some doubts about how to > > retrieve the source address, maybe we'll find that in fact it should > > be seen as a new address family and not a new transport layer. But I > > think not at this point. And BTW Björn had apparently implemented a > > solution based on mptcp@ as well so it's likely workable. > > Yes, it looks better with a new 'struct protocol'. > > I worked on a first draft a few weeks ago: > > https://github.com/matttbe/haproxy/commit/f48f36191 > > As I mentioned in a previous email, I can wait for you to be back to > send a new version on the mailing list. That looks pretty interesting and clean like this! I find your approach of duplicating the tcp definition interesting, but I'd still prefer to have it pre-defined rather than duplicated from tcp though. It's the only one being done like this and I'm concerned that sooner or later we'll break it the dirty way by checking that a list element is empty, that a pointer is NULL or whatever. It's no big deal to have duplicated definitions like this, and it eases "git grep" to find all affected protocols when we want to touch a function. I'm just wondering if the "alt" argument (that you renamed from ctrl_dgram) is always exclusive with the mptcp one. It looks so at first glance. In the worst case this would just end up with a bit field instead of just a boolean, like many other functions. But yes, overall I think that the approach looks like the correct one, and it offers quite a lot of flexibility that way. Out of curiosity, did you test it on the backend side ? I don't know what it implies to do mptcp on the backend (if anything at all), it's more to make sure we're not overlooking anything. > >> 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. > > > > Yeah, understood. I will have limited availability for the next two > > weeks but if he needs some guidance to finish something almost done, > > I'll do my best. It's important to encourage first-time contributors, > > especially since it's easy to want to give up on a first patch feedback, > > we've all known that :-) > > Please take your time. As I mentioned in my previous email on the ML, > Dorian is no longer available to work on that, so I will do my best to > continue. No reason to rush, more to relax ;-) Sure, but I'd like to get this merged before 3.1 is out ;-) Feel free to send your patch(es) with a possibly updated commit messages here on the list. I don't know if we can make a portable enough regtest for this, especially if we consider that a silent fallback might lead to TCP being used. Thanks! Willy
Re: [PATCH] FEATURE: Adding MPTCP with option to disable it and fall-back to TCP
Hi Willy, On 30/05/2024 16:08, Willy Tarreau wrote: > Hi Matthieu, > > finally a bit more available again... > > On Fri, Apr 26, 2024 at 06:34:02PM +0200, Matthieu Baerts wrote: >>> 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 > > Yeah I understand, it would essentially depend on how all of this evolves > over time and how adoption grows for mptcp. Maybe one day most users will > expect it to work by default. I understand your position. >>> 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. > > What you say is true for the listeners, but the socket() call is also > used when connecting to a backend server (though maybe I missed something > during the review). Oh sorry, I understood from Dorian that the modification he did was only impacting the listener sockets, it was not supposed to affect the others. Otherwise, we have the same behaviour as when we launch HAProxy with: mptcpize run haproxy > >>> 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(). > > I *tend* to think that a new struct protocol is easier to add and > maintain. It could just share most (if not all) of the functions > with tcp, and probably be declared in the same file for ease of > sharing code. > > I'm seeing that in the comment you linked above I also proposed a > keyword for "bind" and "server" lines, like we have "tfo" to enable > TCP fast-open. It tends to be slightly easier to implement but then > requires more care everywhere because such options do no apply to > all protocols, so if you have "mptcp on" on a "bind quic4@:443" line, > that's quite confusing so it deserves extra checks to make sure it > is not silently ignored. Also I do have some doubts about how to > retrieve the source address, maybe we'll find that in fact it should > be seen as a new address family and not a new transport layer. But I > think not at this point. And BTW Björn had apparently implemented a > solution based on mptcp@ as well so it's likely workable. Yes, it looks better with a new 'struct protocol'. I worked on a first draft a few weeks ago: https://github.com/matttbe/haproxy/commit/f48f36191 As I mentioned in a previous email, I can wait for you to be back to send a new version on the mailing list. >> Can MPTCP be used as a variant of "rhttp" as well? > > I don't see how it should be related. The rhttp part is always tricky, > but the concept is that we accept a regular connection on a regular > frontend, then based on an explicit rule, return it and assign it as > an idle conn to a server. And for the server setting up pre-connected > idle conns to the gateway, it's just a regular address as well and the > protocol should not be involved there. Thank you, so "rhttp" is not a priority here for MPTCP. >>> Regarding how >>> we'd deal with the fallback, we'll see, but overall, thinking that >>> 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
Re: [PATCH] FEATURE: Adding MPTCP with option to disable it and fall-back to TCP
Hi Matthieu, finally a bit more available again... On Fri, Apr 26, 2024 at 06:34:02PM +0200, Matthieu Baerts wrote: > > 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 Yeah I understand, it would essentially depend on how all of this evolves over time and how adoption grows for mptcp. Maybe one day most users will expect it to work by default. > > 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. What you say is true for the listeners, but the socket() call is also used when connecting to a backend server (though maybe I missed something during the review). > > 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(). I *tend* to think that a new struct protocol is easier to add and maintain. It could just share most (if not all) of the functions with tcp, and probably be declared in the same file for ease of sharing code. I'm seeing that in the comment you linked above I also proposed a keyword for "bind" and "server" lines, like we have "tfo" to enable TCP fast-open. It tends to be slightly easier to implement but then requires more care everywhere because such options do no apply to all protocols, so if you have "mptcp on" on a "bind quic4@:443" line, that's quite confusing so it deserves extra checks to make sure it is not silently ignored. Also I do have some doubts about how to retrieve the source address, maybe we'll find that in fact it should be seen as a new address family and not a new transport layer. But I think not at this point. And BTW Björn had apparently implemented a solution based on mptcp@ as well so it's likely workable. > Can MPTCP be used as a variant of "rhttp" as well? I don't see how it should be related. The rhttp part is always tricky, but the concept is that we accept a regular connection on a regular frontend, then based on an explicit rule, return it and assign it as an idle conn to a server. And for the server setting up pre-connected idle conns to the gateway, it's just a regular address as well and the protocol should not be involved there. > > Regarding how > > we'd deal with the fallback, we'll see, but overall, thinking that > > 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? Yes I agree. I anticipate that if the solution eventually becomes successful, some users will then want a 3-state setting: always-on, always-off, best-effort. But let's not needlessly complexify the design for now. > > 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 fallb
Re: [PATCH] FEATURE: Adding MPTCP with option to disable it and fall-back to TCP
On Wed, May 08, 2024 at 01:19:22PM +, Dorian Craps wrote: > first of all, thank you for your interest. > > I already made a version with an option to enable MPTCP > -https://github.com/CrapsDorian/haproxy/pull/1 > > I'm working on a new version with "mptcp@address" as Willy requested. OK, thank you Dorian. I'm sorry not to have more time to assign to review your work at the moment, it's really a matter of bad timing :-/ Willy
RE: [PATCH] FEATURE: Adding MPTCP with option to disable it and fall-back to TCP
first of all, thank you for your interest. I already made a version with an option to enable MPTCP -https://github.com/CrapsDorian/haproxy/pull/1 I'm working on a new version with "mptcp@address" as Willy requested. Dorian
Re: [PATCH] FEATURE: Adding MPTCP with option to disable it and fall-back to TCP
Hi, I came up a while ago with a patchset for MPTCP support for HAProxy also, see https://github.com/haproxy/haproxy/issues/1028 Back then I also discussed some ideas with Willy how to implement it more elegantly in HAProxy. It's still somewhere on my todo list but unfortunately I didn't catch that up since then. As I had a good real-world usecase recently for MPTCP I though of looking into this again also. Björn On 25.04.24 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. 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). 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, 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. 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. 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). Thanks! Willy
Re: [PATCH] FEATURE: Adding MPTCP with option to disable it and fall-back to TCP
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
Re: [PATCH] FEATURE: Adding MPTCP with option to disable it and fall-back to TCP
Hello Nicolas, Thank you for the feedback! On 24/04/2024 17:45, 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. Indeed, any new features (or code in general) can bring security issues. Same for the protocol. Here it looks like MPTCPv1 addressed all reported issues [1]. > 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. :-) There were no CVEs linked to MPTCP before Linux being a CNA earlier this year [2]. Now any fixes could automatically get a CVE even if there is no proven security issues [3]. For example, I don't think CVE-2024-26826 can cause security issues, and TCP also got some CVEs recently [4] :) But anyway, here the CVEs are linked to the implementation in the Linux kernel, not the protocol (MPTCPv1). > 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. I understand that it is safer, first, to have an option to enable MPTCP support, then enabling it by default later. But I admit that "In a few years" sounds like "a very long time"! MPTCP has been introduced in 2020, 4 years ago, after more than two year of development. So it had time to mature a bit in 6+ years :) Still under, TCP is being used, it is not a full network implementation. When MPTCP is enabled on the server side, it will only be used when clients request it, without impacting "plain" TCP connections. So enabling it will only impact connections where the client asked to use MPTCP. I agree that it increases the attack surface, but like any new features, no? :) > 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) IPPROTO_MPTCP is a constant, it is 262. It is just that at build time, some old libC might not have IPPROTO_MPTCP. To avoid compilation errors, IPPROTO_MPTCP is defined if it was not before. MPTCP support is checked at run time. [1] https://datatracker.ietf.org/doc/html/rfc8684#name-security-considerations [2] http://www.kroah.com/log/blog/2024/02/13/linux-is-a-cna/ [3] https://docs.kernel.org/process/cve.html [4] https://lore.kernel.org/linux-cve-announce/?q=s%3Atcp Cheers, Matt -- Sponsored by the NGI0 Core fund.
Re: [PATCH] FEATURE: Adding MPTCP with option to disable it and fall-back to TCP
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. 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). 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, 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. 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. 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). Thanks! Willy
Re: [PATCH] FEATURE: Adding MPTCP with option to disable it and fall-back to TCP
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) Best regards, ~Nicolas
[PATCH] FEATURE: Adding MPTCP with option to disable it and fall-back to TCP
From: Dorian Craps Multipath TCP (MPTCP), standardized in RFC8684 [1], is a TCP extension that enables a TCP connection to use different paths. Multipath TCP has been used for several use cases. On smartphones, MPTCP enables seamless handovers between cellular and Wi-Fi networks while preserving established connections. This use-case is what pushed Apple to use MPTCP since 2013 in multiple applications [2]. On dual-stack hosts, Multipath TCP enables the TCP connection to automatically use the best performing path, either IPv4 or IPv6. If one path fails, MPTCP automatically uses the other path. To benefit from MPTCP, both the client and the server have to support it. Multipath TCP is a backward-compatible TCP extension that is enabled by default on recent Linux distributions (Debian, Ubuntu, Redhat, ...). Multipath TCP is included in the Linux kernel since version 5.6 [3]. To use it on Linux, an application must explicitly enable it when creating the socket. No need to change anything else in the application. This attached patch uses MPTCP by default instead of TCP on Linux. There is a fallback if the creation of the MPTCP socket fails. A new option has been added in the config to be able to disable MPTCP support. It sounds good to have MPTCP enabled by default, so the client can decide when to use it or not. If the client didn't ask to use MPTCP, the kernel will return a "plain" TCP socket to the server application after an "accept()". [4] IPPROTO_MPTCP is defined just in case old libC are being used and don't have the ref. The running kernel is the only one who can tell if MPTCP is supported or not, it is probably best not to check that at build time. TCP_MAXSEG is currently not supported by MPTCP: is it an issue? MPTCP devs didn't add a support for it because it has not been requested with a use-case. If you think it is important, I can report that to them. Due to the limited impact within a data center environment, MPTCP support has only been added on the listening sockets, then not between the proxy and the servers. The high-speed, low-latency nature of data center networks reduces the benefits of MPTCP, making the complexity of its implementation unnecessary in this context. Developed with the help of Matthieu Baerts (matt...@kernel.org) and Olivier Bonaventure (olivier.bonavent...@uclouvain.be) Link: https://www.rfc-editor.org/rfc/rfc8684.html [1] Link: https://www.tessares.net/apples-mptcp-story-so-far/ [2] Link: https://www.mptcp.dev/ [3] Link: https://www.mptcp.dev/faq.html#why--when-should-mptcp-be-enabled-by-default [4] --- doc/configuration.txt| 4 include/haproxy/global-t.h | 1 + include/haproxy/protocol-t.h | 7 +++ src/cfgparse-global.c| 7 ++- src/cfgparse.c | 2 +- src/proto_rhttp.c| 5 + src/proto_tcp.c | 10 ++ src/protocol.c | 12 +++- src/sock_inet.c | 21 +++-- 9 files changed, 64 insertions(+), 5 deletions(-) diff --git a/doc/configuration.txt b/doc/configuration.txt index d2d654c19..85b75de33 100644 --- a/doc/configuration.txt +++ b/doc/configuration.txt @@ -1342,6 +1342,7 @@ The following keywords are supported in the "global" section : - maxsslrate - maxzlibmem - no-memory-trimming + - no-mptcp - noepoll - noevports - nogetaddrinfo @@ -2974,6 +2975,9 @@ no-memory-trimming nice with the new process. Note that advanced memory allocators usually do not suffer from such a problem. +no-mptcp + Disables Multipath TCP (MPTCP) support when the TCP protocol is requested. + noepoll Disables the use of the "epoll" event polling system on Linux. It is equivalent to the command-line argument "-de". The next polling system diff --git a/include/haproxy/global-t.h b/include/haproxy/global-t.h index 92d2c6bc1..c2b81fb50 100644 --- a/include/haproxy/global-t.h +++ b/include/haproxy/global-t.h @@ -85,6 +85,7 @@ #define GTUNE_LISTENER_MQ_OPT(1<<28) #define GTUNE_LISTENER_MQ_ANY(GTUNE_LISTENER_MQ_FAIR | GTUNE_LISTENER_MQ_OPT) #define GTUNE_QUIC_CC_HYSTART(1<<29) +#define GTUNE_NO_MPTCP (1<<30) #define NO_ZERO_COPY_FWD 0x0001 /* Globally disable zero-copy FF */ #define NO_ZERO_COPY_FWD_PT 0x0002 /* disable zero-copy FF for PT (recv & send are disabled automatically) */ diff --git a/include/haproxy/protocol-t.h b/include/haproxy/protocol-t.h index b85f29cc0..6a7d45c52 100644 --- a/include/haproxy/protocol-t.h +++ b/include/haproxy/protocol-t.h @@ -28,6 +28,12 @@ #include #include +#ifdef __linux__ +#ifndef IPPROTO_MPTCP +#define IPPROTO_MPTCP +#endif +#endif + /* some pointer types referenced below */ struct listener; struct receiver; @@ -99,6 +105,7 @@ struct protocol { enum proto_type proto_type; /* protocol type at the socket layer (PROTO_TYPE_*) */ int sock_type; /* socket type