Re: Question on deleting cookies from an HTTP request

2024-04-26 Thread Aleksandar Lazic

Hi Lokesh.

On 2024-04-27 (Sa.) 01:41, Lokesh Jindal wrote:

Hey folks

I have found that there is no operator "del-cookie" in HAProxy to delete cookies 
from the request. (HAProxy does support the operator "del-header").


Can you explain why such an operator is not supported? Is it due to complexity? 
Due to performance? It will be great if you can share details behind this design 
choice.


Well I'm pretty sure because nobody have added this feature into HAProxy. You 
are welcome to send a patch which add this feature.


Maybe you could add "delete" into the 
https://docs.haproxy.org/2.9/configuration.html#4.2-cookie function.


Please take a look into 
https://github.com/haproxy/haproxy/blob/master/CONTRIBUTING file if you plan to 
contribute.


We have use cases where we want to delete cookies from the request. Not having 
this support in HAProxy also makes me question if one should be deleting request 
cookies in the reverse proxy layer.


Maybe you can use some of the "*-header" functions to remove the cookie as shown 
in the example in https://docs.haproxy.org/2.9/configuration.html#4.4-replace-header



Thanks
Lokesh


Regards
Alex



Question on deleting cookies from an HTTP request

2024-04-26 Thread Lokesh Jindal
Hey folks

I have found that there is no operator "del-cookie" in HAProxy to delete
cookies from the request. (HAProxy does support the operator "del-header").

Can you explain why such an operator is not supported? Is it due to
complexity? Due to performance? It will be great if you can share details
behind this design choice.

We have use cases where we want to delete cookies from the request. Not
having this support in HAProxy also makes me question if one should be
deleting request cookies in the reverse proxy layer.

Thanks
Lokesh


Re: Odd warnings when using "option forwarded"

2024-04-26 Thread Aurelien DARRAGON


> That's interesting, because I already had `option  forwardfor except 
> 127.0.0.1` in the frontend which works perfectly.  Should that be in the 
> backend too? 

No, it's OK to use forwardfor in frontend sections since it is
historical behavior. But forwarded doesn't allow that and requires
explicit backend configuration.

> I was trying to find 'option forwarded' in the documentation, but I couldn't 
> click on it after finding it with the search box and I couldn't search all 
> the documentation with the browser, as it wasn't all on one page.

Here is the relevant config section:
https://docs.haproxy.org/dev/configuration.html#4.2-option%20forwarded

Aurelien



Re: Odd warnings when using "option forwarded"

2024-04-26 Thread Shawn Heisey

On 4/26/24 10:51, Aurelien DARRAGON wrote:

This is expected because forwarded cannot be used on frontend unlike
forwardfor:


That's interesting, because I already had `option  forwardfor except 
127.0.0.1` in the frontend which works perfectly.  Should that be in the 
backend too?


I was trying to find 'option forwarded' in the documentation, but I 
couldn't click on it after finding it with the search box and I couldn't 
search all the documentation with the browser, as it wasn't all on one page.


Thanks,
Shawn




Re: Odd warnings when using "option forwarded"

2024-04-26 Thread Aurelien DARRAGON
Hi Shawn

On 26/04/2024 18:34, Shawn Heisey wrote:
> I was just reading about the new "option forwarded" capability in 2.9,
> so I added it to my config and now I get warnings when checking the config.
> 
> [WARNING]  (253408) : config : parsing [/etc/haproxy/haproxy.cfg:45] :
> 'option forwarded' ignored because frontend 'web80' has no backend
> capability.
> [WARNING]  (253408) : config : parsing [/etc/haproxy/haproxy.cfg:60] :
> 'option forwarded' ignored because frontend 'web' has no backend
> capability.
> 
> This is the line I added to the frontends:
> 
>     option  forwarded except 127.0.0.1
> 
> I realized I don't need it on 'web80', so I removed that one.
> 
> The 'web' frontend does use backends.  Its default backend has no
> servers and denies all requests -- the request must match one of the
> ACLs to choose a backend that actually does something.
> 
> I suspect that I can ignore this warning, but I want to check here and
> make sure.  Can I set something so it doesn't emit the warning?
> 
> Thanks,
> Shawn
> 
> 

This is expected because forwarded cannot be used on frontends unlike
the good old forwardfor:

This is expected because forwarded cannot be used on frontend unlike
forwardfor:

> -- keyword -- defaults - frontend - listen -- backend 
> -
> option forwardfor X  X X X
> option forwarded (*)  X  - X X

(excerpt from the documentation)

This was done on purpose because in practice the option is only relevant
at the backend side, and supporting forwardfor in both frontend and
backend config sections has caused multiple issues in the past because
of the added complexity (requires extra care to check for the option on
the proper section, depending if it's set in the frontend or the backend
or both...), and we wanted to get rid of that extra complexity from the
start when implementing the new forwarded option.

Thus for the option to be considered, you should add it to your default
or backend sections :)

Does that help?

Regards,
Aurelien



Re: [PATCH] FEATURE: Adding MPTCP with option to disable it and fall-back to TCP

2024-04-26 Thread Matthieu Baerts
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 

Odd warnings when using "option forwarded"

2024-04-26 Thread Shawn Heisey
I was just reading about the new "option forwarded" capability in 2.9, 
so I added it to my config and now I get warnings when checking the config.


[WARNING]  (253408) : config : parsing [/etc/haproxy/haproxy.cfg:45] : 
'option forwarded' ignored because frontend 'web80' has no backend 
capability.
[WARNING]  (253408) : config : parsing [/etc/haproxy/haproxy.cfg:60] : 
'option forwarded' ignored because frontend 'web' has no backend capability.


This is the line I added to the frontends:

option  forwarded except 127.0.0.1

I realized I don't need it on 'web80', so I removed that one.

The 'web' frontend does use backends.  Its default backend has no 
servers and denies all requests -- the request must match one of the 
ACLs to choose a backend that actually does something.


I suspect that I can ignore this warning, but I want to check here and 
make sure.  Can I set something so it doesn't emit the warning?


Thanks,
Shawn




Re: [PATCH] FEATURE: Adding MPTCP with option to disable it and fall-back to TCP

2024-04-26 Thread Matthieu Baerts
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.