Phocuswright Europe 2022 - Attendees list
Hi there, I hope you are doing well. We are glad to inform you that PHOCUSWRIGHT EUROPE 2022 Email list is available for purchase. Please let me know if you would like to acquire the Email list. Awaiting your response! Thanks in advance, Lexie Turner Demand Lead Generation
Re: [RFC] [PATCH] BUG: http-ana: fix crash or wrong header deletion by http-restrict-req-hdr-names
Hi Mateusz,
On Tue, Aug 16, 2022 at 11:58:02PM +, hapr...@sl.damisa.net wrote:
> Hi,
>
> as suggested by Willy on GitHub, I'm submitting my patch for
> https://github.com/haproxy/haproxy/issues/1822.
Thank you!
> This is my first contribution, so I'm tagging it as RFC for now ;)
>
> I'm not entirely happy with using goto (suggested by Tim) to avoid
> hitting htx_get_next_blk call at the end of the loop, but I'm not
> familiar with HAproxy coding standards.
We're generally fine with gotos when they're the most efficient
solution (anyway, 1/5 of instructions executed by a CPU are jumps,
so it's pointless to hate gotos, it's just that using too many of
them will make the code less readable).
However here the goto at the beginning potentially maintains a
problem: I'm wondering how we can be certain that htx_remove_blk()
doesn't return a NULL if we remove the last block, in which case
going back to the beginning of the loop without testing it can
be a problem.
> I think it would be nicer to:
>
> 1. Introduce flag variable preserve_blk
> 2. Reset it to 1 at the beginning of the while (blk) loop
> 3. Replace htx_remove_blk + continue with preserve_blk = 0 + break
> 4. At the end of the loop, call htx_get_next_blk if preserve_blk is set
>or call htx_remove_blk otherwise
Yep I agree with this. I was thinking that an even better option
would be to remove the branches from the inner loop and use it only
to spot unusual chars, something like:
end = istlen(n);
for (i = 0; i < end; i++) {
if (!isalnum() ...)
break;
}
if (i < end) {
/* unwanted char found */
if (px->options...)
goto block;
blk = htx_remove_blk();
continue;
}
The for() loop could even be replaced with strcspn(). I just suspect
that for large sets it's slower if it needs to rebuild an internal map,
so we'd probably rather keep the for() loop as-is.
> I have not included severity of the patch, because on GitHub issue is
> still marked as `status: needs-triage`. I think MEDIUM would be
> appropriate.
Yes I think so as well.
> By the way, while writing VTest to cover this bug, I spotted something
> "suspicious" about reg tests for FCGI backends - my-fcgi-app FCGI app is
> defined, but it is not used anywhere? be-fcgi* backends look exactly
> like be-http* to me.
Indeed, I don't know either. I'm wondering if it's not needed just to
enable something by just being present, but I don't see what (and no
comment in the file indicates this). Maybe it's a leftover of an
initial attempt at testing fcgi, I don't know. Let's keep it for now,
we'll check with Christopher when he's back.
But if you can provide an updated patch for the loop, I'm fine with
merging your patch as-is.
Thanks!
Willy
[RFC] [PATCH] BUG: http-ana: fix crash or wrong header deletion by http-restrict-req-hdr-names
Hi,
as suggested by Willy on GitHub, I'm submitting my patch for
https://github.com/haproxy/haproxy/issues/1822.
This is my first contribution, so I'm tagging it as RFC for now ;)
I'm not entirely happy with using goto (suggested by Tim) to avoid
hitting htx_get_next_blk call at the end of the loop, but I'm not
familiar with HAproxy coding standards. I think it would be nicer to:
1. Introduce flag variable preserve_blk
2. Reset it to 1 at the beginning of the while (blk) loop
3. Replace htx_remove_blk + continue with preserve_blk = 0 + break
4. At the end of the loop, call htx_get_next_blk if preserve_blk is set
or call htx_remove_blk otherwise
I have not included severity of the patch, because on GitHub issue is
still marked as `status: needs-triage`. I think MEDIUM would be
appropriate.
By the way, while writing VTest to cover this bug, I spotted something
"suspicious" about reg tests for FCGI backends - my-fcgi-app FCGI app is
defined, but it is not used anywhere? be-fcgi* backends look exactly
like be-http* to me.
Best regards
Mateusz Małek
>From 17dcd8147fb7f48b3fcce5a7a51ff921f4f69848 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mateusz=20Ma=C5=82ek?=
Date: Wed, 17 Aug 2022 00:57:10 +0200
Subject: [PATCH] BUG: http-ana: fix crash or wrong header deletion by
http-restrict-req-hdr-names
When using `option http-restrict-req-hdr-names delete`, HAproxy may
crash or delete wrong header after receiving request containing multiple
forbidden characters in single header name; exact behavior depends on
number of request headers, number of forbidden characters and position
of header containing them.
This patch fixes GitHub issue #1822.
Must be backported as far as 2.2 (buggy feature got included in 2.2.25,
2.4.18 and 2.5.8).
---
.../http-rules/restrict_req_hdr_names.vtc | 47 +++
src/http_ana.c| 7 ++-
2 files changed, 53 insertions(+), 1 deletion(-)
diff --git a/reg-tests/http-rules/restrict_req_hdr_names.vtc
b/reg-tests/http-rules/restrict_req_hdr_names.vtc
index 071b9bded..eed90eab2 100644
--- a/reg-tests/http-rules/restrict_req_hdr_names.vtc
+++ b/reg-tests/http-rules/restrict_req_hdr_names.vtc
@@ -35,6 +35,26 @@ server s5 {
txresp
} -start
+server s6 {
+rxreq
+expect req.http.x_my_hdr_with_lots_of_underscores ==
+txresp
+} -start
+
+server s7 {
+rxreq
+expect req.http.x_my_hdr-1 ==
+expect req.http.x-my-hdr-2 == on
+txresp
+} -start
+
+server s8 {
+rxreq
+expect req.http.x-my_hdr-1 ==
+expect req.http.x-my_hdr-2 ==
+txresp
+} -start
+
haproxy h1 -conf {
defaults
mode http
@@ -50,6 +70,9 @@ haproxy h1 -conf {
use_backend be-fcgi1 if { path /req4 }
use_backend be-fcgi2 if { path /req5 }
use_backend be-fcgi3 if { path /req6 }
+use_backend be-http4 if { path /req7 }
+use_backend be-http5 if { path /req8 }
+use_backend be-http6 if { path /req9 }
backend be-http1
server s1 ${s1_addr}:${s1_port}
@@ -72,6 +95,18 @@ haproxy h1 -conf {
backend be-fcgi3
option http-restrict-req-hdr-names reject
+backend be-http4
+option http-restrict-req-hdr-names delete
+server s6 ${s6_addr}:${s6_port}
+
+backend be-http5
+option http-restrict-req-hdr-names delete
+server s7 ${s7_addr}:${s7_port}
+
+backend be-http6
+option http-restrict-req-hdr-names delete
+server s8 ${s8_addr}:${s8_port}
+
defaults
mode http
timeout connect "${HAPROXY_TEST_TIMEOUT-5s}"
@@ -114,6 +149,18 @@ client c1 -connect ${h1_fe1_sock} {
txreq -req GET -url /req6 -hdr "X-my_hdr: on"
rxresp
expect resp.status == 403
+
+txreq -req GET -url /req7 -hdr "X_my_hdr_with_lots_of_underscores: on"
+rxresp
+expect resp.status == 200
+
+txreq -req GET -url /req8 -hdr "X_my_hdr-1: on" -hdr "X-my-hdr-2: on"
+rxresp
+expect resp.status == 200
+
+txreq -req GET -url /req9 -hdr "X-my_hdr-1: on" -hdr "X-my_hdr-2: on"
+rxresp
+expect resp.status == 200
} -run
client c2 -connect ${h1_fe2_sock} {
diff --git a/src/http_ana.c b/src/http_ana.c
index 4b74dd60d..a2929cef5 100644
--- a/src/http_ana.c
+++ b/src/http_ana.c
@@ -2641,6 +2641,7 @@ static enum rule_result
http_req_restrict_header_names(struct stream *s, struct
blk = htx_get_first_blk(htx);
while (blk) {
+ next_iteration:
enum htx_blk_type type = htx_get_blk_type(blk);
if (type == HTX_BLK_HDR) {
@@ -2653,7 +2654,11 @@ static enum rule_result
http_req_restrict_header_names(struct stream *s, struct
if (px->options2 &
PR_O2_RSTRICT_REQ_HDR_NAMES_BLK)
goto block;
blk = htx_remove_blk(htx, blk);
- continue;
+ /* We must
Re: Buffer limits when adding a large number of CA certs into one ca-file via socket
On Tue, Aug 16, 2022 at 11:16:43AM +, Lais, Alexander wrote: > Hi William, > > Thank you! I figured you were on holidays. A lot of our team are as well. > > Do you see this being back ported to 2.5 / 2.6 (LTS) as well? Unfortunately we usually don't backport this kind of features as this is an API change and could break things. The stable branches are meant to be maintenance only. Also it will probably need some adjustments and new keywords to remove a specific index in the file and that kind of things. -- William Lallemand
Re: Buffer limits when adding a large number of CA certs into one ca-file via socket
Hi William, Thank you! I figured you were on holidays. A lot of our team are as well. Do you see this being back ported to 2.5 / 2.6 (LTS) as well? Thanks and kind regards, Alex > On 16. Aug 2022, at 11:07, William Lallemand wrote: > > On Thu, Aug 04, 2022 at 11:57:16AM +, Lais, Alexander wrote: >> Hi William, >> >> thanks again for the PoC you referenced in the GitHub issue. >> This would solve the use case for us and would fix the ca-cert editing / >> updating feature introduced in HAProxy 2.5. >> >> Can we support further with the development, be it with code or testing, to >> get from this PoC to a full fix in one of next release streams? >> >> Thanks and kind regards, >> Alex >> > Hello Alex, > > Sorry for the late reply, I was in vacation for a few days. > > I'm going to finish the development and tests for the feature so this > could be integrated for the next 2.7 major version. > > Regards, > > -- > William Lallemand
Re: Buffer limits when adding a large number of CA certs into one ca-file via socket
On Thu, Aug 04, 2022 at 11:57:16AM +, Lais, Alexander wrote: > Hi William, > > thanks again for the PoC you referenced in the GitHub issue. > This would solve the use case for us and would fix the ca-cert editing / > updating feature introduced in HAProxy 2.5. > > Can we support further with the development, be it with code or testing, to > get from this PoC to a full fix in one of next release streams? > > Thanks and kind regards, > Alex > Hello Alex, Sorry for the late reply, I was in vacation for a few days. I'm going to finish the development and tests for the feature so this could be integrated for the next 2.7 major version. Regards, -- William Lallemand
