haproxy-2.9.git
20 months agoBUG/MEDIUM: cli: some err/warn msg dumps add LR into CSV output on stat's CLI
Emeric Brun [Tue, 23 Jan 2024 14:44:32 +0000 (15:44 +0100)]
BUG/MEDIUM: cli: some err/warn msg dumps add LR into CSV output on stat's CLI

The initial purpose of CSV stats through CLI was to make it easely
parsable by scripts. But in some specific cases some error or warning
messages strings containing LF were dumped into cells of this CSV.

This made some parsing failure on several tools. In addition, if a
warning or message contains to successive LF, they will be dumped
directly but double LFs tag the end of the response on CLI and the
client may consider a truncated response.

This patch extends the 'csv_enc_append' and 'csv_enc' functions used
to format quoted string content according to RFC  with an additionnal
parameter to convert multi-lines strings to one line: CRs are skipped,
and LFs are replaced with spaces. In addition and optionally, it is
also possible to remove resulting trailing spaces.

The call of this function to fill strings into stat's CSV output is
updated to force this conversion.

This patch should be backported on all supported branches (issue was
already present in v2.0)

(cherry picked from commit ef02dba7bcf4cf41aaf08fa5cabc8dd2d7537f63)
Signed-off-by: Willy Tarreau <w@1wt.eu>

20 months agoMINOR: mux-h2/traces: add a missing trace on connection WU with negative inc
Willy Tarreau [Wed, 17 Jan 2024 15:56:18 +0000 (16:56 +0100)]
MINOR: mux-h2/traces: add a missing trace on connection WU with negative inc

The test was performed but no trace emitted, which can complicate certain
diagnostics, so let's just add the trace for this rare case. It may safely
be backported though this is really not important.

(cherry picked from commit 87b74697cd58f7780f3585f70a1966553192830f)
Signed-off-by: Willy Tarreau <w@1wt.eu>

20 months agoBUG/MEDIUM: mux-h2: refine connection vs stream error on headers
Willy Tarreau [Thu, 18 Jan 2024 16:01:45 +0000 (17:01 +0100)]
BUG/MEDIUM: mux-h2: refine connection vs stream error on headers

Commit 7021a8c4d8 ("BUG/MINOR: mux-h2: also count streams for refused
ones") addressed stream counting issues on some error cases but not
completely correctly regarding the conn_err vs stream_err case. Indeed,
contrary to the initial analysis, h2c_dec_hdrs() can set H2_CS_ERROR
when facing some unrecoverable protocol errors, and it's not correct
to send it to strm_err which will only send the RST_STREAM frame and
the subsequent GOAWAY frame is in fact the result of the read timeout.

The difficulty behind this lies on the sequence of output validations
because h2c_dec_hdrs() returns two results at once:
  - frame processing status (done/incomplete/failed)
  - connection error status

The original ordering requires to write 2 exemplaries of the exact
same error handling code disposed differently, which the patch above
tried to factor to one. After careful inspection of h2c_dec_hdrs()
and its comments, it's clear that it always returns -1 on failure,
*including* connection errors. This means we can rearrange the test
to get rid of the missing data first, and immediately enter the
no-return zone where both the stream and connection errors can be
checked at the same place, making sure to consistently maintain
error counters. This is way better because we don't have to update
stream counters on the error path anymore. h2spec now passes the
test much faster.

This will need to be backported to the same branches as the commit
above, which was already backported to 2.9.

(cherry picked from commit e1c8bfd0ed960d3b3dec39e78ad75bec117912d0)
Signed-off-by: Willy Tarreau <w@1wt.eu>

20 months agoDOC: configuration: fix set-dst in actions keywords matrix
Aurelien DARRAGON [Thu, 18 Jan 2024 14:13:05 +0000 (15:13 +0100)]
DOC: configuration: fix set-dst in actions keywords matrix

Since d54e8f8107 ("DOC: config: reorganize actions into their own section")
dconv-generated shortlink for "set-dst" in actions keywords matrix is
broken.

This is due to trailing "<expr>" which should not be specified in the
matrix, but only in the actual keyword prototype and description.

This should be backported in 2.9 with d54e8f8107.

(cherry picked from commit 62ef7966f0312e31e6fb8fc9e572e0e907e28558)
Signed-off-by: Willy Tarreau <w@1wt.eu>

20 months agoBUG/MINOR: h3: fix checking on NULL Tx buffer
Amaury Denoyelle [Mon, 29 Jan 2024 13:39:19 +0000 (14:39 +0100)]
BUG/MINOR: h3: fix checking on NULL Tx buffer

The following patch was backported to handle gracefully Tx buffer
allocation failure.

  BUG/MINOR: h3: close connection on sending alloc errors

However, the backport is not correct. Indeed, mux_get_buf() returns a
pointer to <qcs.tx.buf> field which is always true. Instead, an explicit
check using b_is_null() must be done instead.

This must be backported up to 2.6.

21 months ago[RELEASE] Released version 2.9.3 v2.9.3
Willy Tarreau [Thu, 18 Jan 2024 14:32:19 +0000 (15:32 +0100)]
[RELEASE] Released version 2.9.3

Released version 2.9.3 with the following main changes :
    - BUILD: quic: missing include for quic_tp
    - BUG/MINOR: mux-quic: do not prevent non-STREAM sending on flow control
    - BUG/MINOR: mux-h2: also count streams for refused ones
    - BUG/MEDIUM: quic: keylog callback not called (USE_OPENSSL_COMPAT)

21 months agoBUG/MEDIUM: quic: keylog callback not called (USE_OPENSSL_COMPAT)
Frederic Lecaille [Tue, 16 Jan 2024 09:17:27 +0000 (10:17 +0100)]
BUG/MEDIUM: quic: keylog callback not called (USE_OPENSSL_COMPAT)

This bug impacts only the QUIC OpenSSL compatibility module (USE_QUIC_OPENSSL_COMPAT)
and it was introduced by this commit:

    BUG/MINOR: quic: Wrong keylog callback setting.

quic_tls_compat_keylog_callback() callback was no more set when the SSL keylog was
enabled by tune.ssl.keylog setting. This is the callback which sets the TLS secrets
into haproxy.

Set it again when the SSL keylog is not enabled by configuration.

Thank you to @Greg57070 for having reported this issue in GH #2412.

Must be backported as far as 2.8.

(cherry picked from commit 0eaf42a2a47f2ee73045e48274ed98e00aa44dba)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

21 months agoBUG/MINOR: mux-h2: also count streams for refused ones
Willy Tarreau [Fri, 12 Jan 2024 17:36:57 +0000 (18:36 +0100)]
BUG/MINOR: mux-h2: also count streams for refused ones

There are a few places where we can reject an incoming stream based on
technical errors such as decoded headers that are too large for the
internal buffers, or memory allocation errors. In this case we send
an RST_STREAM to abort the request, but the total stream counter was
not incremented. That's not really a problem, until one starts to try
to enforce a total stream limit using tune.h2.fe.max-total-streams,
and which will not count such faulty streams. Typically a client that
learns too large cookies and tries to replay them in a way that
overflows the maximum buffer size would be rejected and depending on
how they're implemented, they might retry forever.

This patch removes the stream count increment from h2s_new() and moves
it instead to the calling functions, so that it translates the decision
to process a new stream instead of a successfully decoded stream. The
result is that such a bogus client will now be blocked after reaching
the total stream limit.

This can be validated this way:

  global
        tune.h2.fe.max-total-streams 128
        expose-experimental-directives
        trace h2 sink stdout
        trace h2 level developer
        trace h2 verbosity complete
        trace h2 start now

  frontend h
        bind :8080
        mode http
        redirect location /

Sending this will fill frames with 15972 bytes of cookie headers that
expand to 16500 for storage+index once decoded, causing "message too large"
events:

  (dev/h2/mkhdr.sh -t p;dev/h2/mkhdr.sh -t s;
   for sid in {0..1000}; do
     dev/h2/mkhdr.sh  -t h -i $((sid*2+1)) -f es,eh \
       -R "828684410f7777772e6578616d706c652e636f6d \
           $(for i in {1..66}; do
             echo -n 60 7F 73 433d $(for j in {1..24}; do
               echo -n 2e313233343536373839; done);
            done) ";
   done) | nc 0 8080

Now it properly stops after sending 128 streams.

This may be backported wherever commit 983ac4397 ("MINOR: mux-h2:
support limiting the total number of H2 streams per connection") is
present, since without it, that commit is less effective.

(cherry picked from commit 7021a8c4d8bd8b280d04f481a153a1487deb08d1)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

21 months agoBUG/MINOR: mux-quic: do not prevent non-STREAM sending on flow control
Amaury Denoyelle [Tue, 9 Jan 2024 10:42:08 +0000 (11:42 +0100)]
BUG/MINOR: mux-quic: do not prevent non-STREAM sending on flow control

Data emitted by QUIC MUX is restrained by the peer flow control. This is
checked on stream and connection level inside qcc_io_send().

The connection level check was placed early in qcc_io_send() preambule.
However, this also prevents emission of other frames STOP_SENDING and
RESET_STREAM, until flow control limitation is increased by a received
MAX_DATA. Note that local flow control frame emission is done prior in
qcc_io_send() and so are not impacted.

In the worst case, if no MAX_DATA is received for some time, this could
delay significantly streams closure and resource free. However, this
should be rare as other peers should anticipate emission of MAX_DATA
before reaching flow control limit. In the end, this is also covered by
the MUX timeout so the impact should be minimal

To fix this, move the connection level check directly inside QCS sending
loop. Note that this could cause unnecessary looping when connection
flow control level is reached and no STOP_SENDING/RESET_STREAM are
needed.

This should be backported up to 2.6.

(cherry picked from commit 333f2cababa36f4289493647fed4f7dce0babc77)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

21 months agoBUILD: quic: missing include for quic_tp
Amaury Denoyelle [Wed, 10 Jan 2024 09:57:18 +0000 (10:57 +0100)]
BUILD: quic: missing include for quic_tp

Add missing netinet/in.h required for in_addr/in6_addr types.

This should be backported up to 2.9.

(cherry picked from commit c121fcef30559ee9bcf0aeb1634c48f87ef4b22c)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

21 months ago[RELEASE] Released version 2.9.2 v2.9.2
Christopher Faulet [Thu, 11 Jan 2024 15:07:02 +0000 (16:07 +0100)]
[RELEASE] Released version 2.9.2

Released version 2.9.2 with the following main changes :
    - BUG/MINOR: resolvers: default resolvers fails when network not configured
    - DOC: config: Update documentation about local haproxy response
    - BUG/MINOR: server: Use the configured address family for the initial resolution
    - BUG/MAJOR: stconn: Disable zero-copy forwarding if consumer is shut or in error
    - MINOR: stats: store the parent proxy in stats ctx (http)
    - BUG/MEDIUM: stats: unhandled switching rules with TCP frontend
    - MINOR: server/event_hdl: add server_inetaddr struct to facilitate event data usage
    - MINOR: server/event_hdl: update _srv_event_hdl_prepare_inetaddr prototype
    - BUG/MINOR: server/event_hdl: propagate map port info through inetaddr event
    - DOC: fix typo for fastfwd QUIC option
    - BUG/MINOR: mux-quic: always report error to SC on RESET_STREAM emission
    - BUG/MINOR: mux-quic: disable fast-fwd if connection on error
    - BUG/MINOR: quic: Wrong keylog callback setting.
    - BUG/MINOR: quic: Missing call to TLS message callbacks
    - MINOR: h3: check connection error during sending
    - BUG/MINOR: h3: close connection on header list too big
    - MINOR: h3: add traces for connection init stage
    - BUG/MINOR: h3: properly handle alloc failure on finalize
    - BUG/MINOR: h3: close connection on sending alloc errors
    - BUG/MINOR: h3: disable fast-forward on buffer alloc failure
    - CI: use semantic version compare for determing "latest" OpenSSL
    - MINOR: global: export a way to list build options
    - MINOR: debug: add features and build options to "show dev"
    - REGTESTS: check attach-srv out of order declaration
    - CLEANUP: quic: Remaining useless code into server part
    - BUILD: quic: Missing quic_ssl.h header protection
    - BUG/MEDIUM: h3: fix incorrect snd_buf return value
    - BUG/MEDIUM: stconn: Forward shutdown on write timeout only if it is forwardable
    - BUG/MEDIUM: stconn: Set fsb date if zero-copy forwarding is blocked during nego
    - BUG/MEDIUM: spoe: Never create new spoe applet if there is no server up
    - MINOR: mux-h2: support limiting the total number of H2 streams per connection
    - MINOR: ot: logsrv struct becomes logger
    - MINOR: ssl: Update ssl_fc_curve/ssl_bc_curve to use SSL_get0_group_name
    - CLEANUP: quic: Double quic_dgram_parse() prototype declaration.
    - BUG/MINOR: map: list-based matching potential ordering regression
    - REGTESTS: add a test to ensure map-ordering is preserved
    - DOC: configuration: corrected description of keyword tune.ssl.ocsp-update.mindelay

21 months agoDOC: configuration: corrected description of keyword tune.ssl.ocsp-update.mindelay
Miroslav Zagorac [Tue, 9 Jan 2024 19:55:47 +0000 (20:55 +0100)]
DOC: configuration: corrected description of keyword tune.ssl.ocsp-update.mindelay

Deleted the text paragraph in the description of keyword
tune.ssl.ocsp-update.mindelay, which was added in the commit 5843237
"MINOR: ssl: Add global options to modify ocsp update min/max delay",
because it was a copy of the description of tune.ssl.ssl-ctx-cache-size.

(cherry picked from commit f6ab0446fba1f87b1a703acf98ca4587609bd122)
Signed-off-by: Willy Tarreau <w@1wt.eu>

21 months agoREGTESTS: add a test to ensure map-ordering is preserved
Aurelien DARRAGON [Mon, 8 Jan 2024 09:25:18 +0000 (10:25 +0100)]
REGTESTS: add a test to ensure map-ordering is preserved

As shown in "BUG/MINOR: map: list-based matching potential ordering
regression", list-based matching types such as dom are affected by the
order in which elements are loaded from the map.

Since this is historical behavior and existing usages depend on it, we
add a test to prevent future regressions.

(cherry picked from commit 1088f0b969e18ee5a8fe829d2071507ddef0d05e)
Signed-off-by: Willy Tarreau <w@1wt.eu>

21 months agoBUG/MINOR: map: list-based matching potential ordering regression
Aurelien DARRAGON [Wed, 3 Jan 2024 10:54:03 +0000 (11:54 +0100)]
BUG/MINOR: map: list-based matching potential ordering regression

An unexpected side-effect was introduced by 5fea597 ("MEDIUM: map/acl:
Accelerate several functions using pat_ref_elt struct ->head list")

The above commit tried to use eb tree API to manipulate elements as much
as possible in the hope to accelerate some functions.

Prior to 5fea597, pattern_read_from_file() used to iterate over all
elements from the map file in the same order they were seen in the file
(using list_for_each_entry) to push them in the pattern expression.

Now, since eb api is used to iterate over elements, the ordering is lost
very early.

This is known to cause behavior changes with existing setups (same conf
and map file) when compared with previous versions for some list-based
matching methods as described in GH #2400. For instance, the map_dom()
converter may return a different matching key from the one that was
returned by older haproxy versions.

For IP or STR matching, matching is based on tree lookups for better
efficiency, so in this case the ordering is lost at the name of
performance. The order in which they are loaded doesn't matter because
tree ordering is based on the content, it is not positional.

But with some other types, matching is based on list lookups (e.g.: dom),
and the order in which elements are pushed into the list can affect the
matching element that will be returned (in case of multiple matches, since
only the first matching element in the list will be returned).

Despite the documentation not officially stating that the file ordering
should be preserved for list-based matching methods, it's probably best
to be conservative here and stick to historical behavior. Moreover, there
was no performance benefit from using the eb tree api to iterate over
elements in pattern_read_from_file() since all elements are visited
anyway.

This should be backported to 2.9.

(cherry picked from commit b546bb6d67145db0ebd14c79d72199064208dc2f)
Signed-off-by: Willy Tarreau <w@1wt.eu>

21 months agoCLEANUP: quic: Double quic_dgram_parse() prototype declaration.
Frédéric Lécaille [Wed, 10 Jan 2024 16:22:24 +0000 (17:22 +0100)]
CLEANUP: quic: Double quic_dgram_parse() prototype declaration.

This function is defined in the RX part (quic_rx.c) and declared in quic_rx.h
header. This is its correct place.

Remove the useless declaration of this function in quic_conn.h.

Should be backported in 2.9 where this double declaration was introduced when
moving quic_dgram_parse() from quic_conn.c to quic_rx.c.

(cherry picked from commit 37d5a26cc5a5ebe1abeb2836f89424e33408a186)
Signed-off-by: Willy Tarreau <w@1wt.eu>

21 months agoMINOR: ssl: Update ssl_fc_curve/ssl_bc_curve to use SSL_get0_group_name
Mariam John [Fri, 29 Dec 2023 17:14:41 +0000 (11:14 -0600)]
MINOR: ssl: Update ssl_fc_curve/ssl_bc_curve to use SSL_get0_group_name

The function `smp_fetch_ssl_fc_ec` gets the curve name used during key
exchange. It currently uses the `SSL_get_negotiated_group`, available
since OpenSSLv3.0 to get the nid and derive the short name of the curve
from the nid. In OpenSSLv3.2, a new function, `SSL_get0_group_name` was
added that directly gives the curve name.

The function `smp_fetch_ssl_fc_ec` has been updated to use
`SSL_get0_group_name` if using OpenSSL>=3.2 and for versions >=3.0 and <
3.2 use the old SSL_get_negotiated_group to get the curve name. Another
change made is to normalize the return value, so that
`smp_fetch_ssl_fc_ec` returns curve name in uppercase.
(`SSL_get0_group_name` returns the curve name in lowercase and
`SSL_get_negotiated_group` + `OBJ_nid2sn` returns curve name in
uppercase).

Can be backported to 2.8.

(cherry picked from commit 25da2174c62b536b00bc5b0c0bb64ae95fd826b9)
Signed-off-by: Willy Tarreau <w@1wt.eu>

21 months agoMINOR: ot: logsrv struct becomes logger
Miroslav Zagorac [Mon, 8 Jan 2024 11:55:11 +0000 (12:55 +0100)]
MINOR: ot: logsrv struct becomes logger

Addition to commit 18da35c "MEDIUM: tree-wide: logsrv struct becomes logger",
when the OpenTracing filter is compiled in debug mode (using OT_DEBUG=1)
then logsrv should be changed to logger here as well.

This patch should be backported to branch 2.9.

(cherry picked from commit 86298c6913fabb5dba48aeaa9befcd957e2e8ccf)
Signed-off-by: Willy Tarreau <w@1wt.eu>

21 months agoMINOR: mux-h2: support limiting the total number of H2 streams per connection
Willy Tarreau [Fri, 13 Oct 2023 16:11:59 +0000 (18:11 +0200)]
MINOR: mux-h2: support limiting the total number of H2 streams per connection

This patch introduces a new setting: tune.h2.fe.max-total-streams. It
sets the HTTP/2 maximum number of total streams processed per incoming
connection. Once this limit is reached, HAProxy will send a graceful GOAWAY
frame informing the client that it will close the connection after all
pending streams have been closed. In practice, clients tend to close as fast
as possible when receiving this, and to establish a new connection for next
requests. Doing this is sometimes useful and desired in situations where
clients stay connected for a very long time and cause some imbalance inside a
farm. For example, in some highly dynamic environments, it is possible that
new load balancers are instantiated on the fly to adapt to a load increase,
and that once the load goes down they should be stopped without breaking
established connections. By setting a limit here, the connections will have
a limited lifetime and will be frequently renewed, with some possibly being
established to other nodes, so that existing resources are quickly released.

The default value is zero, which enforces no limit beyond those implied by
the protocol (2^30 ~= 1.07 billion). Values around 1000 were found to
already cause frequent enough connection renewal without causing any
perceptible latency to most clients. One notable exception here is h2load
which reports errors for all requests that were expected to be sent over
a given connection after it receives a GOAWAY. This is an already known
limitation: https://github.com/nghttp2/nghttp2/issues/981

The patch was made in two parts inside h2_frt_handle_headers():
  - the first one, at the end of the function, which verifies if the
    configured limit was reached and if it's needed to emit a GOAWAY ;

  - the second, just before decoding the stream frame, which verifies if
    a previously configured limit was ignored by the client, and closes
    the connection if this happens. Indeed, one reason for a connection
    to stay alive for too long definitely comes from a stupid bot that
    periodically fetches the same resource, scans lots of URLs or tries
    to brute-force something. These ones are more likely to just ignore
    the last stream ID advertised in GOAWAY than a regular browser, or
    a well-behaving client such as curl which respects it. So in order
    to make sure we can close the connection we need to enforce the
    advertised limit.

Note that a regular client will not face a problem with that because in
the worst case it will have max_concurrent_streams in flight and this
limit is taken into account when calculating the advertised last
acceptable stream ID.

Just a note: it may also be possible to move the first part above to
h2s_frt_stream_new() instead so that it's not processed for trailers,
though it doesn't seem to be more interesting, first because it has
two return points.

This is something that may be backported to 2.9 and 2.8 to offer more
control to those dealing with dynamic infrastructures, especially since
for now we cannot force a connection to be cleanly closed using rules
(e.g. github issues #946, #2146).

(cherry picked from commit 983ac4397d2a6fe94c9d4960dcc8272d87801958)
[wt: also removed the debugging printfs killed in 3.0 by e19334a343]
Signed-off-by: Willy Tarreau <w@1wt.eu>

21 months agoBUG/MEDIUM: spoe: Never create new spoe applet if there is no server up
Christopher Faulet [Fri, 5 Jan 2024 16:06:48 +0000 (17:06 +0100)]
BUG/MEDIUM: spoe: Never create new spoe applet if there is no server up

This test was already performed when a new message is queued into the
sending queue. However not when the last applet is released, in
spoe_release_appctx(). It is a quite old bug. It was introduced by commit
6f1296b5c7 ("BUG/MEDIUM: spoe: Create a SPOE applet if necessary when the
last one is released").

Because of this bug, new SPOE applets may be created and quickly released
because there is no server up, in loop and while there is at least one
message in the sending queue, consuming all the CPU. It is pretty visible if
the processing timeout is high.

To fix the bug, conditions to create or not a SPOE applet are now
centralized in spoe_create_appctx(). The test about the max connections per
second and about number of active servers are moved in this function.

This patch must be backported to all stable versions.

(cherry picked from commit 72c23bd4cd3ee3a79955fbdd601206a9fa7d19eb)
Signed-off-by: Willy Tarreau <w@1wt.eu>

21 months agoBUG/MEDIUM: stconn: Set fsb date if zero-copy forwarding is blocked during nego
Christopher Faulet [Fri, 5 Jan 2024 15:59:06 +0000 (16:59 +0100)]
BUG/MEDIUM: stconn: Set fsb date if zero-copy forwarding is blocked during nego

During the zero-copy forwarding, if the consumer side reports it is blocked,
it means it is blocked on send. At the stream-connector level, the event
must be reported to be sure to set/update the fsb date. Otherwise, write
timeouts cannot be properly reported. If this happens when no other timeout
is armed, this freezes the stream.

This patch must be backported to 2.9.

(cherry picked from commit 7cc41514226a2072b538c859cd83dc01de0aeb3f)
Signed-off-by: Willy Tarreau <w@1wt.eu>

21 months agoBUG/MEDIUM: stconn: Forward shutdown on write timeout only if it is forwardable
Christopher Faulet [Fri, 5 Jan 2024 15:48:40 +0000 (16:48 +0100)]
BUG/MEDIUM: stconn: Forward shutdown on write timeout only if it is forwardable

The commit b9c87f8082 ("BUG/MEDIUM: stconn/stream: Forward shutdown on write
timeout") introduced a regression. In sc_cond_forward_shut(), the write
timeout is considered too early to forward the shutdown. In fact, it is
always considered, even if the shutdown is not forwardable yet. It is of
course unexpected. It is especially an issue when a write timeout is
encountered on server side during the connection establishment. In this
case, if shutdown is forwarded too early on the client side, the connection
is closed before the 503 error sending.

So the write timeout must indeed be considered to forward the shutdown to
the underlying layer, but only if the shutdown is forwardable. Otherwise, we
should do nothing.

This patch should fix the issue #2404. It must be backported as far as 2.2.

(cherry picked from commit 7eb7ae283557d8467403b9db35605861a0870add)
Signed-off-by: Willy Tarreau <w@1wt.eu>

21 months agoBUG/MEDIUM: h3: fix incorrect snd_buf return value
Amaury Denoyelle [Thu, 4 Jan 2024 10:33:33 +0000 (11:33 +0100)]
BUG/MEDIUM: h3: fix incorrect snd_buf return value

h3_resp_data_send() is used to transcode HTX data into H3 data frames.
If QCS Tx buffer is not aligned when first invoked, two separate frames
may be built, first until buffer end, then with remaining space in
front.

If buffer space is not enough for at least the H3 frame header, -1 is
returned with the flag QC_SF_BLK_MROOM set to await for more room. An
issue arises if this occurs for the second frame : -1 is returned even
though HTX data were properly transcoded and removed on the first step.
This causes snd_buf callback to return an incorrect value to the stream
layer, which in the end will corrupt the channel output buffer.

To fix this, stop considering that not enough remaining space is an
error case. Instead, return 0 if this is encountered for the first frame
or the HTX removed block size for the second one. As QC_SF_BLK_MROOM is
set, this will correctly interrupt H3 encoding. Label err is thus only
properly limited to fatal error which should cause a connection closure.
A new BUG_ON() has been added which should prevent similar issues in the
future.

This issue was detected using the following client :
 $ ngtcp2-client --no-quic-dump --no-http-dump --exit-on-all-streams-close \
   127.0.0.1 20443 -n2 "http://127.0.0.1:20443/?s=50k"

This triggers the following CHECK_IF statement. Note that it may be
necessary to disable fast forwarding to enforce snd_buf usage.

Thread 1 "haproxy" received signal SIGILL, Illegal instruction.
0x00005555558bc48a in co_data (c=0x5555561ed428) at include/haproxy/channel.h:130
130             CHECK_IF_HOT(c->output > c_data(c));
[ ## gdb ## ] bt
 #0  0x00005555558bc48a in co_data (c=0x5555561ed428) at include/haproxy/channel.h:130
 #1  0x00005555558c1d69 in sc_conn_send (sc=0x5555561f92d0) at src/stconn.c:1637
 #2  0x00005555558c2683 in sc_conn_io_cb (t=0x5555561f7f10, ctx=0x5555561f92d0, state=32832) at src/stconn.c:1824
 #3  0x000055555590c48f in run_tasks_from_lists (budgets=0x7fffffffdaa0) at src/task.c:596
 #4  0x000055555590cf88 in process_runnable_tasks () at src/task.c:876
 #5  0x00005555558aae3b in run_poll_loop () at src/haproxy.c:3049
 #6  0x00005555558ab57e in run_thread_poll_loop (data=0x555555d9fa00 <ha_thread_info>) at src/haproxy.c:3251
 #7  0x00005555558ad053 in main (argc=6, argv=0x7fffffffddd8) at src/haproxy.c:3948

In case CHECK_IF are not activated, it may cause crash or incorrect
transfers.

This was introduced by the following commit
  commit 2144d2418651c1f76b91cc1f6e745feecdefcb00
  BUG/MINOR: h3: close connection on sending alloc errors

This must be backported wherever the above patch is.

(cherry picked from commit 14673fe54d91af3ce28ee2d94ca77f626eb1a7ea)
[wt: adjusted context in h3_resp_data_send]
Signed-off-by: Willy Tarreau <w@1wt.eu>

21 months agoBUILD: quic: Missing quic_ssl.h header protection
Frédéric Lécaille [Thu, 4 Jan 2024 12:56:44 +0000 (13:56 +0100)]
BUILD: quic: Missing quic_ssl.h header protection

Such "#ifdef USE_QUIC" prepocessor statements are used by QUIC C header
to avoid inclusion of QUIC headers when the QUIC support is not enabled
(by USE_QUIC make variable). Furthermore, this allows inclusions of QUIC
header from C file without having to protect them with others "#ifdef USE_QUIC"
statements as follows:

   #ifdef USE_QUIC
   #include <a QUIC header>
   #include <another one QUIC header>
   #endif /* USE_QUIC */

So, here if this quic_ssl.h header was included by a C file, and compiled without
QUIC support, this will lead to build errrors as follows:

 In file included from <a C file...>:
        include/haproxy/quic_ssl.h:35:35: warning: â\80\98enum ssl_encryption_level_tâ\80\99
        declared inside parameter list will not be visible outside of this
        definition or declaration

Should be backported to 2.9 to avoid such building issues to come.

(cherry picked from commit fd178ccdb0e0c22ce784d53762bef33b5709bdaa)
Signed-off-by: Willy Tarreau <w@1wt.eu>

21 months agoCLEANUP: quic: Remaining useless code into server part
Frédéric Lécaille [Thu, 4 Jan 2024 10:16:06 +0000 (11:16 +0100)]
CLEANUP: quic: Remaining useless code into server part

Remove some QUIC definitions of members from server structure as the haproxy QUIC
stack does not support at all the server part (QUIC client) as this time.
Remove the statements in relation with their initializations.

This patch should be backported as far as 2.6 to save memory.

(cherry picked from commit 860028db47ac2ad38de56c7711ce872245c70b83)
Signed-off-by: Willy Tarreau <w@1wt.eu>

21 months agoREGTESTS: check attach-srv out of order declaration
Amaury Denoyelle [Tue, 2 Jan 2024 13:40:25 +0000 (14:40 +0100)]
REGTESTS: check attach-srv out of order declaration

Previous patch fixed a regression which caused some config with
attach-srv to be rejected if the rule was declared before the target
server itself. To better detect this kind of error, mix the declaration
order in the corresponding regtest.

(cherry picked from commit 0627f470df366e9dadf604f5f473e2dc2693200f)
Signed-off-by: Willy Tarreau <w@1wt.eu>

21 months agoMINOR: debug: add features and build options to "show dev"
Willy Tarreau [Tue, 2 Jan 2024 10:08:04 +0000 (11:08 +0100)]
MINOR: debug: add features and build options to "show dev"

The "show dev" CLI command is still missing useful elements such as the
build options, SSL version etc. Let's just add the build features and
the build options there so that it's possible to collect all of this
from a running process without having to start the executable with -vv.

This is still dumped all at once from the parsing function since the
output is small. If it were to grow, this would possibly require to be
reworked to support a context.

It might be helpful to backport this to 2.9 since it can help narrow
down certain issues.

(cherry picked from commit 9d869b10dea0f4234eb14787f658d6b64a0a4021)
Signed-off-by: Willy Tarreau <w@1wt.eu>

21 months agoMINOR: global: export a way to list build options
Willy Tarreau [Tue, 2 Jan 2024 09:56:05 +0000 (10:56 +0100)]
MINOR: global: export a way to list build options

The new function hap_get_next_build_opt() will iterate over the list of
build options. This will be used for debugging, so that the build options
can be retrieved from the CLI.

(cherry picked from commit afba58f21e7c938b793858180b3bd8d8fd6aef5d)
Signed-off-by: Willy Tarreau <w@1wt.eu>

21 months agoCI: use semantic version compare for determing "latest" OpenSSL
Ilya Shipitsin [Fri, 29 Dec 2023 22:31:39 +0000 (23:31 +0100)]
CI: use semantic version compare for determing "latest" OpenSSL

currently "openssl-3.2.0-beta1" wins over "openssl-3.2.0" due to
string comparision. let's switch to semantic version compare

(cherry picked from commit e6d0b87f7f18cd07163687f15b086eb86ed57957)
Signed-off-by: Willy Tarreau <w@1wt.eu>

21 months agoBUG/MINOR: h3: disable fast-forward on buffer alloc failure
Amaury Denoyelle [Fri, 22 Dec 2023 15:07:10 +0000 (16:07 +0100)]
BUG/MINOR: h3: disable fast-forward on buffer alloc failure

If QCS Tx buffer cannot be allocated in nego_ff callback, disable
fast-forward for this connection and return immediately. If snd_buf is
later finally used but still no buffer can being allocated, the
connection will be closed on error.

This should fix coverity reported in github issue #2390.

This should be backported up to 2.9.

(cherry picked from commit cfa6d4cdd058ea112c461349a39d095ba96825e4)
[cf: qcc_get_stream_txbuf() does not exist. Result of mux_get_buf() is tested
     instead]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

21 months agoBUG/MINOR: h3: close connection on sending alloc errors
Amaury Denoyelle [Fri, 22 Dec 2023 08:00:13 +0000 (09:00 +0100)]
BUG/MINOR: h3: close connection on sending alloc errors

When encoding new HTTP/3 frames, QCS Tx buffer must be allocated if
currently NULL. Previously, allocation failure was not properly checked,
leaving the connection in an unspecified state, or worse risking a
crash.

Fix this by setting <h3c.err> to H3_INTERNAL_ERROR each time the
allocation fails. This will stop sending and close the connection. In
the future, it may be better to put the connection on pause waiting for
allocation to succeed but this is too complicated to implement for now
in a reliable way.

Along with the current change, return of all HTX parsing functions
(h3_resp_*_send) were set to a negative value in case of error. A new
BUG_ON() in h3_snd_buf() ensures that if such a value is returned,
either a connection error is register (via <h3c.err>) or buffer is
temporarily full (flag QC_SF_BLK_MROOM).

This should fix github issue #2389.

This should be backported up to 2.6. Note that qcc_get_stream_txbuf()
does not exist in 2.9 and below. mux_get_buf() is its equivalent. An
explicit check b_is_null(&qcs.tx.buf) should be used there.

(cherry picked from commit 2144d2418651c1f76b91cc1f6e745feecdefcb00)
[cf: H3_EV_TX_FRAME removed from trace messages because it does not exist]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

21 months agoBUG/MINOR: h3: properly handle alloc failure on finalize
Amaury Denoyelle [Fri, 15 Dec 2023 16:32:06 +0000 (17:32 +0100)]
BUG/MINOR: h3: properly handle alloc failure on finalize

If H3 control stream Tx buffer cannot be allocated, return a proper
errur through h3_finalize(). This will cause the emission of a
CONNECTION_CLOSE with error H3_INTERNAL_ERROR and closure of the whole
connection.

This should be backported up to 2.6. Note that 2.9 has some difference
which will cause conflict. The main one is that qcc_get_stream_txbuf()
does not exist in this version. Instead the check in h3_control_send()
should be made after mux_get_buf(). Finally, it may be useful to first
pick previous commit (MINOR: h3: add traces for connection init stage)
to improve context similarity.

(cherry picked from commit 7a3602a1f55dacda5a669865e04474f5d503bab7)
[cf: H3_EV_TX_FRAME removed from trace messages because it does not exist]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

21 months agoMINOR: h3: add traces for connection init stage
Amaury Denoyelle [Fri, 15 Dec 2023 16:30:36 +0000 (17:30 +0100)]
MINOR: h3: add traces for connection init stage

Add traces H3_EV_H3C_NEW. These are used for h3_init() and h3_finalize()
functions.

(cherry picked from commit a2dbd6d916ed06d7c7b3db5a8dc77bc01c13d2c0)
[cf: CTX adjt]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

21 months agoBUG/MINOR: h3: close connection on header list too big
Amaury Denoyelle [Thu, 21 Dec 2023 16:42:43 +0000 (17:42 +0100)]
BUG/MINOR: h3: close connection on header list too big

When parsing a HTX response, if too many headers are present, stop
sending and close the connection with error code H3_INTERNAL_ERROR.

Previously, no error was reported despite the interruption of header
parsing. This cause an infinite loop. However, this is considered as
minor as it happens on the response path from backend side.

This should be backported up to 2.6.
It relies on previous commit
  "MINOR: h3: check connection error during sending".

(cherry picked from commit d077f7ccf48424fa40eea29c69283ec23732c96d)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

21 months agoMINOR: h3: check connection error during sending
Amaury Denoyelle [Fri, 22 Dec 2023 10:45:54 +0000 (11:45 +0100)]
MINOR: h3: check connection error during sending

If an error occurs during HTX to H3 encoding, h3_snd_buf() should be
interrupted. This commit add this possibility by checking for <h3c.err>
member value. If non null, sending loop is stopped and an error is
reported using qcc_set_error().

This commit does not change any behavior for now, as <h3c.err> is never
set during sending. However, this will change in future commits, most
notably to reject too many headers or handle buffer allocation failure.
As such, this commit should be backported along the following fixes.
Note that in 2.6 qcc_set_error() does not exist and must be replaced by
qcc_emit_cc_app().

(cherry picked from commit 642016ce033aeb5ccc09b81b98db8046eefd2e2c)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

21 months agoBUG/MINOR: quic: Missing call to TLS message callbacks
Frédéric Lécaille [Thu, 21 Dec 2023 15:11:35 +0000 (16:11 +0100)]
BUG/MINOR: quic: Missing call to TLS message callbacks

This bug impacts only the QUIC OpenSSL compatibility module (USE_QUIC_OPENSSL_COMPAT).

The TLS capture of information from client hello enabled by
tune.ssl.capture-buffer-size could not work with USE_QUIC_OPENSSL_COMPAT. This
is due to the fact the callback set for this feature was replaced by
quic_tls_compat_msg_callback(). In fact this called must be registered by
ssl_sock_register_msg_callback() as this done for the TLS client hello capture.
A call to this function appends the function passed as parameter to a list of
callbacks to be called when the TLS stack parse a TLS message.
quic_tls_compat_msg_callback() had to be modified to return if it is called
for a non-QUIC TLS session.

Must be backported to 2.8.

(cherry picked from commit 10e96fcd177e858284252d413aaf16af06474beb)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

21 months agoBUG/MINOR: quic: Wrong keylog callback setting.
Frédéric Lécaille [Thu, 21 Dec 2023 13:14:22 +0000 (14:14 +0100)]
BUG/MINOR: quic: Wrong keylog callback setting.

This bug impacts only the QUIC OpenSSL compatibility module (USE_QUIC_OPENSSL_COMPAT).

To make this module works, quic_tls_compat_keylog_callback() function must be
set as keylog callback, or at least be called by another keylog callback.
This is what SSL_CTX_keylog() was supposed to do. In addition to export the TLS
secrets via sample fetches this latter also calls quic_tls_compat_keylog_callback()
when compiled with USE_QUIC_OPENSSL_COMPAT defined.

Before this patch, SSL_CTX_keylog() was replaced by quic_tls_compat_keylog_callback()
and the TLS secret were no more exported by sample fetches.

Must be backported to 2.8.

(cherry picked from commit b26f6fb0cb358ef1df15e21151906e4c2e9f4364)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

21 months agoBUG/MINOR: mux-quic: disable fast-fwd if connection on error
Amaury Denoyelle [Thu, 21 Dec 2023 10:15:19 +0000 (11:15 +0100)]
BUG/MINOR: mux-quic: disable fast-fwd if connection on error

Add a check on nego_ff to ensure connection is not on error. If this is
the case, fast-forward is disable to prevent unnecessary sending. If
snd_buf is latter called, stconn will be notified of the error to
interrupt the stream.

This check is necessary to ensure snd_buf and nego_ff are consistent.
Note that previously, if fast-forward was conducted even on connection
error, no sending would occur as qcc_io_send() also check these flags.
However, there is a risk that stconn is never notified of the error
status, thus it is considered as a bug.

Its impact is minimal for now as fast-forward is disable by default on
QUIC. By fixing it, it should be possible to reactive it soon.

This should be backported up to 2.9.

(cherry picked from commit 19f4f4d890bc28bbe4b850e1ced9be399f59fafb)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

21 months agoBUG/MINOR: mux-quic: always report error to SC on RESET_STREAM emission
Amaury Denoyelle [Tue, 19 Dec 2023 10:22:28 +0000 (11:22 +0100)]
BUG/MINOR: mux-quic: always report error to SC on RESET_STREAM emission

On RESET_STREAM emission, the stream Tx channel is closed. This event
must be reported to stream-conn layer to interrupt future send
operations.

Previously, se_fl_set_error() was manually invocated before/after
qcc_reset_stream(). Change this by moving se_fl_set_error() invocation
into the latter. This ensures that notification won't be forget, most
notably in HTTP/3 layer.

In most cases, behavior should be identical as both functions were
called together unless not necessary. However, there is one exception
which could cause a RESET_STREAM emission without error notification :
this happens on H3 trailers parsing error. All other H3 errors happen
before the stream-layer creation and thus the error is notified on
stream creation. This regression has been caused by the following patch :

  152beeec34baed98ad4c186454ddb25e4c496b50
  MINOR: mux-quic: report error on stream-endpoint earlier

Thus it should be backported up to 2.7.

Note that the case described above did not cause any crash or protocol
error. This is because currently MUX QUIC snd_buf operation silently
reset buffer on transmission if QCS is already closed locally. This will
however be removed in a future commit so the current patch is necessary
to prevent an invalid behavior.

(cherry picked from commit 0a69750a98bbf4c8d9ab2c84e3890956116dbfa0)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

21 months agoDOC: fix typo for fastfwd QUIC option
Amaury Denoyelle [Thu, 21 Dec 2023 10:30:39 +0000 (11:30 +0100)]
DOC: fix typo for fastfwd QUIC option

Replace prefix 'tune.quit.' by 'tune.quic.'.

This should be backported up to 2.9.

(cherry picked from commit c62487fd08c14efa9dcb89a34f470c2cee8b7a66)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

21 months agoBUG/MINOR: server/event_hdl: propagate map port info through inetaddr event
Aurelien DARRAGON [Thu, 7 Dec 2023 16:08:08 +0000 (17:08 +0100)]
BUG/MINOR: server/event_hdl: propagate map port info through inetaddr event

server addr:svc_port updates during runtime might set or clear the
SRV_F_MAPPORTS flag. Unfortunately, the flag update is still directly
performed by srv_update_addr_port() function while the addr:svc_port
update is being scheduled for atomic update. Given that existing readers
don't take server's lock to read addr:svc_port, they also check the
SRV_F_MAPPORTS flag right after without the lock.

So we could cause the readers to incorrectly interpret the svc_port from
the server struct because the mapport information is not published
atomically, resulting in inconsistencies between svc_port / mapport flag.
(MAPPORTS flag causes svc_port to be used differently by the reader)

To fix this, we publish the mapport information within the INETADDR server
event and we let the task responsible for updating server's addr and port
position or clear the flag depending on the mapport hint.

This patch depends on:
 - MINOR: server/event_hdl: add server_inetaddr struct to facilitate event data usage
 - MINOR: server/event_hdl: update _srv_event_hdl_prepare_inetaddr prototype

This should be backported in 2.9 with 683b2ae01 ("MINOR: server/event_hdl:
add SERVER_INETADDR event")

(cherry picked from commit 545e72546c0d729fd3de0f753adc905dd0b13908)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

21 months agoMINOR: server/event_hdl: update _srv_event_hdl_prepare_inetaddr prototype
Aurelien DARRAGON [Thu, 7 Dec 2023 15:39:32 +0000 (16:39 +0100)]
MINOR: server/event_hdl: update _srv_event_hdl_prepare_inetaddr prototype

Slightly change _srv_event_hdl_prepare_inetaddr() function prototype to
reduce the input arguments by learning some settings directly from the
server. Also taking this opportunity to make the function static inline
since it's relatively simple and not meant to be used directly.

(cherry picked from commit 4e50c31eab9a45bf82c5225fbaf456a8da983ce9)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

21 months agoMINOR: server/event_hdl: add server_inetaddr struct to facilitate event data usage
Aurelien DARRAGON [Thu, 7 Dec 2023 15:18:39 +0000 (16:18 +0100)]
MINOR: server/event_hdl: add server_inetaddr struct to facilitate event data usage

event_hdl_cb_data_server_inetaddr struct had some anonymous structs
defined in it, making it impossible to pass as a function argument and
harder to maintain since changes must be performed at multiple places
at once. So instead we define a storage struct named server_inetaddr
that helps to save addr:port server information in INET context.

(cherry picked from commit 14893a6a00f24e14dcca5baa335a7f9d78dafc80)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

21 months agoBUG/MEDIUM: stats: unhandled switching rules with TCP frontend
Aurelien DARRAGON [Tue, 5 Dec 2023 16:54:20 +0000 (17:54 +0100)]
BUG/MEDIUM: stats: unhandled switching rules with TCP frontend

Consider the following configuration:

  backend back
    mode http

  frontend front
    mode tcp
    bind localhost:8080
    stats enable
    stats uri /stats
    tcp-request content switch-mode http if FALSE
    use_backend back

Firing a request to /stats on the haproxy process started with the above
configuration will cause a segfault in http_handle_stats().

The cause for the crash is that in this case, the upgrade doesn't simply
switches to HTTP mode, but also changes the stream backend (changing from
the frontend itself to the targeted HTTP backend).

However, there is an inconsitency in the stats logic between the check
for the stats URI and the actual handling of the stats page.

Indeed, http_stats_check_uri() checks uri parameters from the proxy
undergoing the http analyzers, whereas http_handle_stats() uses s->be
instead.

During stream analysis, from the frontend perspective: s->be defaults to
the frontend. But if the frontend is in TCP mode and the stream is
upgraded to HTTP via backend switching rules, then s->be will be assigned
to the actual HTTP-capable backend in stream_set_backend().

What this means is that when the http analyzer first checks if the current
URI matches the one from the "stats uri" directive, it will check against
the "stats uri" directive from the frontend, but later since the stats
handlers reads the uri from s->be it wil actually use the value from the
backend and the previous safety checks are thus garbage, resulting in
unexpected behavior. (In our test case since the backend didn't define
"stats uri" it is set to NULL, and http_handle_stats() dereferences it)

To fix this, we should ensure that prechecks and actual stats processing
always rely on the same proxy source for stats config directives.

This is what is done in this patch, thanks to the previous commit, since
we can make sure that the stat applet will use ->http_px as its parent
proxy. So here we simply propagate the current proxy being analyzed
through all the stats processing functions.

This patch depends on:
 - MINOR: stats: store the parent proxy in stats ctx (http)

It should be backported up to 2.4.
For 2.4: the fix is less trivial since stats ctx was directly stored
within the applet struct at that time, so this alternative patch must be
used instead (without "MINOR: stats: store the parent proxy in stats ctx
(http)" dependency):

    diff --git a/include/haproxy/applet-t.h b/include/haproxy/applet-t.h
    index 014e01ed9..1d9a63359 100644
    --- a/include/haproxy/applet-t.h
    +++ b/include/haproxy/applet-t.h
    @@ -121,6 +121,7 @@ struct appctx {
       * keep the grouped together and avoid adding new ones.
       */
      struct {
    + struct proxy *http_px;  /* parent proxy of the current applet (only relevant for HTTP applet) */
      void *obj1;             /* context pointer used in stats dump */
      void *obj2;             /* context pointer used in stats dump */
      uint32_t domain;        /* set the stats to used, for now only proxy stats are supported */
    diff --git a/src/http_ana.c b/src/http_ana.c
    index b557da89d..1025d7711 100644
    --- a/src/http_ana.c
    +++ b/src/http_ana.c
    @@ -63,8 +63,8 @@ static enum rule_result http_req_restrict_header_names(struct stream *s, struct
     static void http_manage_client_side_cookies(struct stream *s, struct channel *req);
     static void http_manage_server_side_cookies(struct stream *s, struct channel *res);

    -static int http_stats_check_uri(struct stream *s, struct http_txn *txn, struct proxy *backend);
    -static int http_handle_stats(struct stream *s, struct channel *req);
    +static int http_stats_check_uri(struct stream *s, struct http_txn *txn, struct proxy *px);
    +static int http_handle_stats(struct stream *s, struct channel *req, struct proxy *px);

     static int http_handle_expect_hdr(struct stream *s, struct htx *htx, struct http_msg *msg);
     static int http_reply_100_continue(struct stream *s);
    @@ -428,7 +428,7 @@ int http_process_req_common(struct stream *s, struct channel *req, int an_bit, s
      }

      /* parse the whole stats request and extract the relevant information */
    - http_handle_stats(s, req);
    + http_handle_stats(s, req, px);
      verdict = http_req_get_intercept_rule(px, &px->uri_auth->http_req_rules, s);
      /* not all actions implemented: deny, allow, auth */

    @@ -3959,16 +3959,16 @@ void http_check_response_for_cacheability(struct stream *s, struct channel *res)

     /*
      * In a GET, HEAD or POST request, check if the requested URI matches the stats uri
    - * for the current backend.
    + * for the current proxy.
      *
      * It is assumed that the request is either a HEAD, GET, or POST and that the
      * uri_auth field is valid.
      *
      * Returns 1 if stats should be provided, otherwise 0.
      */
    -static int http_stats_check_uri(struct stream *s, struct http_txn *txn, struct proxy *backend)
    +static int http_stats_check_uri(struct stream *s, struct http_txn *txn, struct proxy *px)
     {
    - struct uri_auth *uri_auth = backend->uri_auth;
    + struct uri_auth *uri_auth = px->uri_auth;
      struct htx *htx;
      struct htx_sl *sl;
      struct ist uri;
    @@ -4003,14 +4003,14 @@ static int http_stats_check_uri(struct stream *s, struct http_txn *txn, struct p
      * s->target which is supposed to already point to the stats applet. The caller
      * is expected to have already assigned an appctx to the stream.
      */
    -static int http_handle_stats(struct stream *s, struct channel *req)
    +static int http_handle_stats(struct stream *s, struct channel *req, struct proxy *px)
     {
      struct stats_admin_rule *stats_admin_rule;
      struct stream_interface *si = &s->si[1];
      struct session *sess = s->sess;
      struct http_txn *txn = s->txn;
      struct http_msg *msg = &txn->req;
    - struct uri_auth *uri_auth = s->be->uri_auth;
    + struct uri_auth *uri_auth = px->uri_auth;
      const char *h, *lookup, *end;
      struct appctx *appctx;
      struct htx *htx;
    @@ -4020,6 +4020,7 @@ static int http_handle_stats(struct stream *s, struct channel *req)
      memset(&appctx->ctx.stats, 0, sizeof(appctx->ctx.stats));
      appctx->st1 = appctx->st2 = 0;
      appctx->ctx.stats.st_code = STAT_STATUS_INIT;
    + appctx->ctx.stats.http_px = px;
      appctx->ctx.stats.flags |= uri_auth->flags;
      appctx->ctx.stats.flags |= STAT_FMT_HTML; /* assume HTML mode by default */
      if ((msg->flags & HTTP_MSGF_VER_11) && (txn->meth != HTTP_METH_HEAD))
    diff --git a/src/stats.c b/src/stats.c
    index d1f3daa98..1f0b2bff7 100644
    --- a/src/stats.c
    +++ b/src/stats.c
    @@ -2863,9 +2863,9 @@ static int stats_dump_be_stats(struct stream_interface *si, struct proxy *px)
      return stats_dump_one_line(stats, stats_count, appctx);
     }

    -/* Dumps the HTML table header for proxy <px> to the trash for and uses the state from
    - * stream interface <si> and per-uri parameters <uri>. The caller is responsible
    - * for clearing the trash if needed.
    +/* Dumps the HTML table header for proxy <px> to the trash and uses the state from
    + * stream interface <si>. The caller is responsible for clearing the trash if
    + * needed.
      */
     static void stats_dump_html_px_hdr(struct stream_interface *si, struct proxy *px)
     {
    @@ -3015,17 +3015,19 @@ static void stats_dump_html_px_end(struct stream_interface *si, struct proxy *px
      * input buffer. Returns 0 if it had to stop dumping data because of lack of
      * buffer space, or non-zero if everything completed. This function is used
      * both by the CLI and the HTTP entry points, and is able to dump the output
    - * in HTML or CSV formats. If the later, <uri> must be NULL.
    + * in HTML or CSV formats.
      */
     int stats_dump_proxy_to_buffer(struct stream_interface *si, struct htx *htx,
    -        struct proxy *px, struct uri_auth *uri)
    +        struct proxy *px)
     {
      struct appctx *appctx = __objt_appctx(si->end);
    - struct stream *s = si_strm(si);
      struct channel *rep = si_ic(si);
      struct server *sv, *svs; /* server and server-state, server-state=server or server->track */
      struct listener *l;
    + struct uri_auth *uri = NULL;

    + if (appctx->ctx.stats.http_px)
    + uri = appctx->ctx.stats.http_px->uri_auth;
      chunk_reset(&trash);

      switch (appctx->ctx.stats.px_st) {
    @@ -3045,7 +3047,7 @@ int stats_dump_proxy_to_buffer(struct stream_interface *si, struct htx *htx,
      break;

      /* match '.' which means 'self' proxy */
    - if (strcmp(scope->px_id, ".") == 0 && px == s->be)
    + if (strcmp(scope->px_id, ".") == 0 && px == appctx->ctx.stats.http_px)
      break;
      scope = scope->next;
      }
    @@ -3227,10 +3229,16 @@ int stats_dump_proxy_to_buffer(struct stream_interface *si, struct htx *htx,
     }

     /* Dumps the HTTP stats head block to the trash for and uses the per-uri
    - * parameters <uri>. The caller is responsible for clearing the trash if needed.
    + * parameters from the parent proxy. The caller is responsible for clearing
    + * the trash if needed.
      */
    -static void stats_dump_html_head(struct appctx *appctx, struct uri_auth *uri)
    +static void stats_dump_html_head(struct appctx *appctx)
     {
    + struct uri_auth *uri;
    +
    + BUG_ON(!appctx->ctx.stats.http_px);
    + uri = appctx->ctx.stats.http_px->uri_auth;
    +
      /* WARNING! This must fit in the first buffer !!! */
      chunk_appendf(&trash,
                    "<!DOCTYPE HTML PUBLIC \"-//W3C//DTD HTML 4.01 Transitional//EN\"\n"
    @@ -3345,17 +3353,21 @@ static void stats_dump_html_head(struct appctx *appctx, struct uri_auth *uri)
     }

     /* Dumps the HTML stats information block to the trash for and uses the state from
    - * stream interface <si> and per-uri parameters <uri>. The caller is responsible
    - * for clearing the trash if needed.
    + * stream interface <si> and per-uri parameters from the parent proxy. The caller
    + * is responsible for clearing the trash if needed.
      */
    -static void stats_dump_html_info(struct stream_interface *si, struct uri_auth *uri)
    +static void stats_dump_html_info(struct stream_interface *si)
     {
      struct appctx *appctx = __objt_appctx(si->end);
      unsigned int up = (now.tv_sec - start_date.tv_sec);
      char scope_txt[STAT_SCOPE_TXT_MAXLEN + sizeof STAT_SCOPE_PATTERN];
      const char *scope_ptr = stats_scope_ptr(appctx, si);
    + struct uri_auth *uri;
      unsigned long long bps = (unsigned long long)read_freq_ctr(&global.out_32bps) * 32;

    + BUG_ON(!appctx->ctx.stats.http_px);
    + uri = appctx->ctx.stats.http_px->uri_auth;
    +
      /* Turn the bytes per second to bits per second and take care of the
       * usual ethernet overhead in order to help figure how far we are from
       * interface saturation since it's the only case which usually matters.
    @@ -3629,8 +3641,7 @@ static void stats_dump_json_end()
      * a pointer to the current server/listener.
      */
     static int stats_dump_proxies(struct stream_interface *si,
    -                              struct htx *htx,
    -                              struct uri_auth *uri)
    +                              struct htx *htx)
     {
      struct appctx *appctx = __objt_appctx(si->end);
      struct channel *rep = si_ic(si);
    @@ -3650,7 +3661,7 @@ static int stats_dump_proxies(struct stream_interface *si,
      px = appctx->ctx.stats.obj1;
      /* skip the disabled proxies, global frontend and non-networked ones */
      if (!px->disabled && px->uuid > 0 && (px->cap & (PR_CAP_FE | PR_CAP_BE))) {
    - if (stats_dump_proxy_to_buffer(si, htx, px, uri) == 0)
    + if (stats_dump_proxy_to_buffer(si, htx, px) == 0)
      return 0;
      }

    @@ -3666,14 +3677,12 @@ static int stats_dump_proxies(struct stream_interface *si,
     }

     /* This function dumps statistics onto the stream interface's read buffer in
    - * either CSV or HTML format. <uri> contains some HTML-specific parameters that
    - * are ignored for CSV format (hence <uri> may be NULL there). It returns 0 if
    - * it had to stop writing data and an I/O is needed, 1 if the dump is finished
    - * and the stream must be closed, or -1 in case of any error. This function is
    - * used by both the CLI and the HTTP handlers.
    + * either CSV or HTML format. It returns 0 if it had to stop writing data and
    + * an I/O is needed, 1 if the dump is finished and the stream must be closed,
    + * or -1 in case of any error. This function is used by both the CLI and the
    + * HTTP handlers.
      */
    -static int stats_dump_stat_to_buffer(struct stream_interface *si, struct htx *htx,
    -      struct uri_auth *uri)
    +static int stats_dump_stat_to_buffer(struct stream_interface *si, struct htx *htx)
     {
      struct appctx *appctx = __objt_appctx(si->end);
      struct channel *rep = si_ic(si);
    @@ -3688,7 +3697,7 @@ static int stats_dump_stat_to_buffer(struct stream_interface *si, struct htx *ht

      case STAT_ST_HEAD:
      if (appctx->ctx.stats.flags & STAT_FMT_HTML)
    - stats_dump_html_head(appctx, uri);
    + stats_dump_html_head(appctx);
      else if (appctx->ctx.stats.flags & STAT_JSON_SCHM)
      stats_dump_json_schema(&trash);
      else if (appctx->ctx.stats.flags & STAT_FMT_JSON)
    @@ -3708,7 +3717,7 @@ static int stats_dump_stat_to_buffer(struct stream_interface *si, struct htx *ht

      case STAT_ST_INFO:
      if (appctx->ctx.stats.flags & STAT_FMT_HTML) {
    - stats_dump_html_info(si, uri);
    + stats_dump_html_info(si);
      if (!stats_putchk(rep, htx, &trash))
      goto full;
      }
    @@ -3733,7 +3742,7 @@ static int stats_dump_stat_to_buffer(struct stream_interface *si, struct htx *ht
      case STATS_DOMAIN_PROXY:
      default:
      /* dump proxies */
    - if (!stats_dump_proxies(si, htx, uri))
    + if (!stats_dump_proxies(si, htx))
      return 0;
      break;
      }
    @@ -4112,11 +4121,14 @@ static int stats_process_http_post(struct stream_interface *si)
     static int stats_send_http_headers(struct stream_interface *si, struct htx *htx)
     {
      struct stream *s = si_strm(si);
    - struct uri_auth *uri = s->be->uri_auth;
    + struct uri_auth *uri;
      struct appctx *appctx = __objt_appctx(si->end);
      struct htx_sl *sl;
      unsigned int flags;

    + BUG_ON(!appctx->ctx.stats.http_px);
    + uri = appctx->ctx.stats.http_px->uri_auth;
    +
      flags = (HTX_SL_F_IS_RESP|HTX_SL_F_VER_11|HTX_SL_F_XFER_ENC|HTX_SL_F_XFER_LEN|HTX_SL_F_CHNK);
      sl = htx_add_stline(htx, HTX_BLK_RES_SL, flags, ist("HTTP/1.1"), ist("200"), ist("OK"));
      if (!sl)
    @@ -4166,11 +4178,14 @@ static int stats_send_http_redirect(struct stream_interface *si, struct htx *htx
     {
      char scope_txt[STAT_SCOPE_TXT_MAXLEN + sizeof STAT_SCOPE_PATTERN];
      struct stream *s = si_strm(si);
    - struct uri_auth *uri = s->be->uri_auth;
    + struct uri_auth *uri;
      struct appctx *appctx = __objt_appctx(si->end);
      struct htx_sl *sl;
      unsigned int flags;

    + BUG_ON(!appctx->ctx.stats.http_px);
    + uri = appctx->ctx.stats.http_px->uri_auth;
    +
      /* scope_txt = search pattern + search query, appctx->ctx.stats.scope_len is always <= STAT_SCOPE_TXT_MAXLEN */
      scope_txt[0] = 0;
      if (appctx->ctx.stats.scope_len) {
    @@ -4263,7 +4278,7 @@ static void http_stats_io_handler(struct appctx *appctx)
      }

      if (appctx->st0 == STAT_HTTP_DUMP) {
    - if (stats_dump_stat_to_buffer(si, res_htx, s->be->uri_auth))
    + if (stats_dump_stat_to_buffer(si, res_htx))
      appctx->st0 = STAT_HTTP_DONE;
      }

    @@ -4888,6 +4903,7 @@ static int cli_parse_show_stat(char **args, char *payload, struct appctx *appctx

      appctx->ctx.stats.scope_str = 0;
      appctx->ctx.stats.scope_len = 0;
    + appctx->ctx.stats.http_px = NULL; // not under http context
      appctx->ctx.stats.flags = STAT_SHNODE | STAT_SHDESC;

      if ((strm_li(si_strm(appctx->owner))->bind_conf->level & ACCESS_LVL_MASK) >= ACCESS_LVL_OPER)
    @@ -4954,7 +4970,7 @@ static int cli_io_handler_dump_info(struct appctx *appctx)
      */
     static int cli_io_handler_dump_stat(struct appctx *appctx)
     {
    - return stats_dump_stat_to_buffer(appctx->owner, NULL, NULL);
    + return stats_dump_stat_to_buffer(appctx->owner, NULL);
     }

     static int cli_io_handler_dump_json_schema(struct appctx *appctx)

(cherry picked from commit 64b7d8e173d169c31aff1cfc8495dc5e14fa1454)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

21 months agoMINOR: stats: store the parent proxy in stats ctx (http)
Aurelien DARRAGON [Tue, 5 Dec 2023 18:49:13 +0000 (19:49 +0100)]
MINOR: stats: store the parent proxy in stats ctx (http)

Some HTTP related stats functions need to know the parent proxy, mainly
to get a pointer on the related uri_auth set by the proxy or to check
scope settings.

The current design (probably historical as only the http context existed
by then) took the other approach: it propagates the uri pointer from the
http context deep down the calling stack up to the relevant functions.
For non-http contexts (cli), the pointer is set to NULL.

Doing so is not very pretty and not easy to maintain. Moreover, there were
still some places in the code were the uri pointer was learned directly
from the stream proxy because the argument was not available as argument
from those functions. This is error-prone, because if one day we decide to
change the source proxy in the parent function, we might still have some
functions down the stack that ignore the top most argument and still do
on their own, and we'll probably end up with inconsistencies.

So in this patch, we take a safer approach: the caller responsible for
creating the stats applet should set the http_px pointer so that any stats
function running under the applet that needs to know if it's running in
http context or needs to access parent proxy info may do so thanks to
the dedicated ctx->http_px pointer.

(cherry picked from commit ef9d692544f0bd961bfa5e6a4569447921bf1137)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

21 months agoBUG/MAJOR: stconn: Disable zero-copy forwarding if consumer is shut or in error
Christopher Faulet [Thu, 21 Dec 2023 09:27:53 +0000 (10:27 +0100)]
BUG/MAJOR: stconn: Disable zero-copy forwarding if consumer is shut or in error

A regression was introduced by commit 2421c6fa7d ("BUG/MEDIUM: stconn: Block
zero-copy forwarding if EOS/ERROR on consumer side"). When zero-copy
forwarding is inuse and the consumer side is shut or in error, we declare it
as blocked and it is woken up. The idea is to handle this state at the
stream-connector level. However this definitly blocks receives on the
producer side. So if the mux is unable to close by itself, but instead wait
the peer to shut, this can lead to a wake up loop. And indeed, with the
passthrough multiplexer this may happen.

To fix the issue and prevent any loop, instead of blocking the zero-copy
forwarding, we now disable it. This way, the stream-connector on producer
side will fallback on classical receives and will be able to handle peer
shutdown properly. In addition, the wakeup of the consumer side was
removed. This will be handled, if necessary, by sc_notify().

This patch should fix the issue #2395. It must be backported to 2.9.

(cherry picked from commit 123a9e7d83213bcc0478840bab2270113275fb0a)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

21 months agoBUG/MINOR: server: Use the configured address family for the initial resolution
Christopher Faulet [Wed, 20 Dec 2023 11:21:57 +0000 (12:21 +0100)]
BUG/MINOR: server: Use the configured address family for the initial resolution

A regression was introduced by the commit c886fb58eb ("MINOR: server/ip:
centralize server ip updates"). The configured address family is lost when the
server address is initialized during the startup, for the resolution based on
the libc or based on the server state-file. Thus, "ipv4@" and "ipv6@" prefixed
are ignored.

To fix the bug, we take care to use the configured address family before calling
str2ip2() in srv_apply_lastaddr() and srv_apply_via_libc() functions.

This patch should fix the issue #2393. It must be backported to 2.9.

(cherry picked from commit 3811c1de25bd8b05fb4684614883a0f04f304978)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

22 months agoDOC: config: Update documentation about local haproxy response
Christopher Faulet [Tue, 19 Dec 2023 07:51:26 +0000 (08:51 +0100)]
DOC: config: Update documentation about local haproxy response

Documentation about 'L' state in the termination state was outdated. Today,
not only the request may be intercepted, but also the response.
Documentation about 'L' must be more generic.

However, documentation about possible 2-letter termination states was also
extended to add 'LC' and 'LH' in the list. And 'LR' was adapted too.

This patch should fix the issue #2384. It may be backported to every stable
versions. Note that on 2.8 and lowers, we talk about session and not stream.

(cherry picked from commit 0a203c1d3f359396e66679991f5c84460fae80e2)
Signed-off-by: Willy Tarreau <w@1wt.eu>

22 months agoBUG/MINOR: resolvers: default resolvers fails when network not configured
William Lallemand [Mon, 18 Dec 2023 11:35:35 +0000 (12:35 +0100)]
BUG/MINOR: resolvers: default resolvers fails when network not configured

Bug #1740 was opened again, this time a user is complaining about the
"can't create socket for nameserver". This can happen if the resolv.conf
file contains a class of address which was not configured on the
machine, for example IPv6.

The fix does the same as b10b1196b ("MINOR: resolvers: shut the warning
when "default" resolvers is implicit"), and uses the
"resolvers->conf.implicit" variable to emit the error.

Though it is not needed to convert the explicit behavior with a
ERR_WARN, because this is supposed to be an unrecoverable error, unlike
the connect().

Should fix issue #1740.

Must be backported were b10b1196b was backported. (as far as 2.6)

(cherry picked from commit 0d2ebb53f752502137479c9e753a96046d3254ab)
Signed-off-by: Willy Tarreau <w@1wt.eu>

22 months ago[RELEASE] Released version 2.9.1 v2.9.1
Christopher Faulet [Fri, 15 Dec 2023 13:35:36 +0000 (14:35 +0100)]
[RELEASE] Released version 2.9.1

Released version 2.9.1 with the following main changes :
    - BUG/MINOR: ssl: Double free of OCSP Certificate ID
    - MINOR: ssl/cli: Add ha_(warning|alert) msgs to CLI ckch callback
    - BUG/MINOR: ssl: Wrong OCSP CID after modifying an SSL certficate
    - BUG/MINOR: lua: Wrong OCSP CID after modifying an SSL certficate (LUA)
    - DOC: configuration: typo req.ssl_hello_type
    - BUG/MINOR: mworker/cli: fix set severity-output support
    - BUG/MEDIUM: quic: Possible buffer overflow when building TLS records
    - BUILD: ssl: update types in wolfssl cert selection callback
    - BUG/MEDIUM: map/acl: pat_ref_{set,delete}_by_id regressions
    - BUG/MINOR: ext-check: cannot use without preserve-env
    - MINOR: version: mention that it's stable now
    - BUG/MEDIUM: quic: QUIC CID removed from tree without locking
    - BUG/MEDIUM: stconn: Block zero-copy forwarding if EOS/ERROR on consumer side
    - BUG/MEDIUM: mux-h1: Cound data from input buf during zero-copy forwarding
    - BUG/MEDIUM: mux-h1: Explicitly skip request's C-L header if not set originally
    - CLEANUP: mux-h1: Fix a trace message about C-L header addition
    - BUG/MEDIUM: mux-h2: Report too large HEADERS frame only when rxbuf is empty
    - BUG/MEDIUM: mux-quic: report early error on stream
    - DOC: config: add arguments to sample fetch methods in the table
    - DOC: config: also add arguments to the converters in the table

22 months agoDOC: config: also add arguments to the converters in the table
Willy Tarreau [Fri, 15 Dec 2023 10:18:27 +0000 (11:18 +0100)]
DOC: config: also add arguments to the converters in the table

Now that dconv supports linking to keywords with arguments from tables,
let's mention the arguments in the summary table of converters.

(cherry picked from commit c5bde03a0acac4263b7ff373f819b680d9a52102)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

22 months agoDOC: config: add arguments to sample fetch methods in the table
Willy Tarreau [Fri, 15 Dec 2023 10:18:08 +0000 (11:18 +0100)]
DOC: config: add arguments to sample fetch methods in the table

Now that dconv supports linking to keywords with arguments from tables,
let's mention the arguments in the summary tables of sample fetch methods.

(cherry picked from commit 0d261dd13a9d475e3bf9d7bae07d156fa3f5c5a6)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

22 months agoBUG/MEDIUM: mux-quic: report early error on stream
Amaury Denoyelle [Wed, 13 Dec 2023 15:28:28 +0000 (16:28 +0100)]
BUG/MEDIUM: mux-quic: report early error on stream

On STOP_SENDING reception, an error is notified to the stream layer as
no more data can be responded. However, this is not done if the stream
instance is not allocated (already freed for example).

The issue occurs if STOP_SENDING is received and the stream instance is
instantiated after it. It happens if a STREAM frame is received after it
with H3 HEADERS, which is valid in QUIC protocol due to UDP packet
reordering. In this case, stream layer is never notified about the
underlying error. Instead, reponse buffers are silently purged by the
MUX in qmux_strm_snd_buf().

This is suboptimal as there is no point in exchanging data from the
server if it cannot be eventually transferred back to the client.
However, aside from this consideration, no other issue occured. However,
this is not the case with QUIC mux-to-mux implementation. Now, if
mux-to-mux is used, qmux_strm_snd_buf() is bypassed and response if
transferred via nego_ff/done_ff callbacks. However, these functions did
not checked if QCS is already locally closed. This causes a crash when
qcc_send_stream() is called via done_ff.

To fix this crash, there is several approach, one of them would be to
adjust nego_ff/done_ff QUIC callbacks. However, another method has been
chosen. Now stream layer is flagged on error just after its
instantiation if the stream is already locally closed. This ensures that
mux-to-mux won't try to emit data as se_nego_ff() check if the opposide
SD is not on error before continuing.

Note that an alternative solution could be to not instantiate at all
stream layer if QCS is already locally closed. This is the most optimal
solution as it reduce unnecessary allocations and task processing.
However, it's not easy to implement so the easier bug fix has been
chosen for the moment.

This patch is labelled as MEDIUM as it can change behavior of all QCS
instances, wheter mux-to-mux is used or not, and thus could reveal other
architecture issues.

This should fix latest crash occurence on github issue #2392.

It should be backported up to 2.6, until a necessary period of
observation.

(cherry picked from commit af297f19f6ff954b739cde3fa76f99f665b556bd)
Signed-off-by: Willy Tarreau <w@1wt.eu>

22 months agoBUG/MEDIUM: mux-h2: Report too large HEADERS frame only when rxbuf is empty
Christopher Faulet [Wed, 13 Dec 2023 14:36:52 +0000 (15:36 +0100)]
BUG/MEDIUM: mux-h2: Report too large HEADERS frame only when rxbuf is empty

During HEADERS frames decoding, if a frame is too large to fit in a buffer,
an internal error is reported and a RST_STREAM is emitted. On the other
hand, we wait to have an empty rxbuf to decode the frame because we cannot
retry a failed HPACK decompression.

When we are decoding headers, it is valid to return an error if dbuf buffer
is full because no data can be blocked in the rxbuf (which hosts the HTX
message).

However, during the trailers decoding, it is possible to have some data not
sent yet for the current stream in the rxbug and data for another stream
fully filling the dbuf buffer. In this case, we don't decode the trailers
but we must not return an error. We must wait to empty the rxbuf first.

Now, a HEADERS frame is considered as too large if the dbuf buffer is full
and if the rxbuf is empty (the HTX message to be accurate).

This patch should fix the issue #2382. It must be backported to all stable
versions.

(cherry picked from commit 682f73b4fa6d76aa0b5b743fe92777822884772d)
Signed-off-by: Willy Tarreau <w@1wt.eu>

22 months agoCLEANUP: mux-h1: Fix a trace message about C-L header addition
Christopher Faulet [Tue, 12 Dec 2023 13:04:35 +0000 (14:04 +0100)]
CLEANUP: mux-h1: Fix a trace message about C-L header addition

This fixes a cut-paste error on a trace message notifying a 'Content-Length'
header was added during the HTTP message formatting.

(cherry picked from commit 65ca4442401c527530f5b456a8ed47a35c224480)
Signed-off-by: Willy Tarreau <w@1wt.eu>

22 months agoBUG/MEDIUM: mux-h1: Explicitly skip request's C-L header if not set originally
Christopher Faulet [Tue, 12 Dec 2023 12:56:05 +0000 (13:56 +0100)]
BUG/MEDIUM: mux-h1: Explicitly skip request's C-L header if not set originally

Commit f89ba27caa ("BUG/MEDIUM: mux-h1; Ignore headers modifications about
payload representation") introduced a regression. The Content-Length is no
longer sent to the server for requests without payload but with a
'Content-Lnegth' header explicitly set to 0, like POST request with no
payload. It is of course unexpected. In some cases, depending on the server,
such requests are considered as invalid and a 411-Length-Required is returned.

The above commit is not directly responsible for the bug, it only reveals a too
lax condition to skip the 'Content-Length' header of bodyless requests. We must
only skip this header if none was originally found, during the parsing.

This patch should fix the issue #2386. It must be backported to 2.9.

(cherry picked from commit 966a18e2b485901bd986ebe65670dc4bfbfe65b5)
Signed-off-by: Willy Tarreau <w@1wt.eu>

22 months agoBUG/MEDIUM: mux-h1: Cound data from input buf during zero-copy forwarding
Christopher Faulet [Mon, 11 Dec 2023 20:55:32 +0000 (21:55 +0100)]
BUG/MEDIUM: mux-h1: Cound data from input buf during zero-copy forwarding

During zero-copy forwarding, we first try to forward data found in the input
buffer before trying to receive more data. These data must be removed from
the amount of data to forward (the cound variable).

Otherwise, on an internal retry, in h1_fastfwd(), we can be lead to read
more data than expected. It is especially a problem on the end of a
chunk. An error is erroneously reported because more data than announced are
received.

This patch should fix the issue #2382. It must be backported to 2.9.

(cherry picked from commit eed1e8733c279cda0b6a2db574ead4e0238177f6)
Signed-off-by: Willy Tarreau <w@1wt.eu>

22 months agoBUG/MEDIUM: stconn: Block zero-copy forwarding if EOS/ERROR on consumer side
Christopher Faulet [Mon, 11 Dec 2023 12:56:15 +0000 (13:56 +0100)]
BUG/MEDIUM: stconn: Block zero-copy forwarding if EOS/ERROR on consumer side

When the producer side (h1 for now) negociates with the consumer side to
perform a zero-copy forwarding, we now consider the consumer side as blocked
if it is closed and this was reported to the SE via a end-of-stream or a
(pending) error.

It is performed before calling ->nego_ff callback function, in se_nego_ff().
This way, all consumer are concerned automatically. The aim of this patch is
to fix an issue with the QUIC mux. Indeed, it is unexpected to send a frame
on an closed stream. This triggers a BUG_ON(). Other muxes are not affected
but it remains useless to try to send data if the stream is closed.

This patch should fix the issue #2372. It must be backported to 2.9.

(cherry picked from commit 2421c6fa7db5b06109a88bffa411c8ba8f948384)
Signed-off-by: Willy Tarreau <w@1wt.eu>

22 months agoBUG/MEDIUM: quic: QUIC CID removed from tree without locking
Frédéric Lécaille [Wed, 13 Dec 2023 10:45:43 +0000 (11:45 +0100)]
BUG/MEDIUM: quic: QUIC CID removed from tree without locking

This bug arrived with this commit:

   BUG/MINOR: quic: Wrong RETIRE_CONNECTION_ID sequence number chec

Every connection ID manipulations against the by thread trees used to store the
connection IDs must be done under the trees locks. These trees are accessed by
the low level connection identification code.

When receiving a RETIRE_CONNECTION_ID frame, the concerned connection ID must
be deleted from the its underlying by thread tree but not without locking!
Add a WR lock around ebmb_delete() call to do so.

Must be backported as far as 2.7.

(cherry picked from commit dd58dff1e64fa382d368958469506c296454fde6)
Signed-off-by: Willy Tarreau <w@1wt.eu>

22 months agoMINOR: version: mention that it's stable now
Christopher Faulet [Fri, 8 Dec 2023 13:34:32 +0000 (14:34 +0100)]
MINOR: version: mention that it's stable now

This version will be maintained up to around Q1 2025. The INSTALL file
also mentions it.

22 months agoBUG/MINOR: ext-check: cannot use without preserve-env
Aurelien DARRAGON [Thu, 7 Dec 2023 08:58:27 +0000 (09:58 +0100)]
BUG/MINOR: ext-check: cannot use without preserve-env

Since 1de44da ("MINOR: ext-check: add an option to preserve environment
variables"), it is now possible to provide an extra argument to
"external-check" directive. This allows to support the "preserve-env"
option which differs from the default behavior.

However a mistake was made, because the config parser doesn't allow the
default configuration anymore: using external-check without argument will
trigger an error:
  'external-check' only supports 'preserve-env' as an argument, found ''.

This is due to as small mistake in the code that make the check
systematically report an error if the first argument is not equal to
"preserve-env". The check was modified so that the error is only reported
if the argument is provided, so that the default behavior is restored.

This should fix GH #2380 and should be backported on 2.9 and potentially
further (anywhere 1de44da is, because a note about an optional backport
up to the 2.6 was left in the original commit message)

(cherry picked from commit 63282f3bfbaee50a0716823bdc8c4251a47e6e39)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

22 months agoBUG/MEDIUM: map/acl: pat_ref_{set,delete}_by_id regressions
Aurelien DARRAGON [Fri, 8 Dec 2023 10:46:15 +0000 (11:46 +0100)]
BUG/MEDIUM: map/acl: pat_ref_{set,delete}_by_id regressions

Some regressions were introduced by 5fea59754b ("MEDIUM: map/acl:
Accelerate several functions using pat_ref_elt struct ->head list")

pat_ref_delete_by_id() fails to properly unlink and free the removed
reference because it bypasses the pat_ref_delete_by_ptr() made for
that purpose. This function is normally used everywhere the target
reference is set for removal, such as the pat_ref_delete() function
that matches pattern against a string. The call was probably skipped
by accident during the rewrite of the function.

With the above commit also comes another undesirable change:
both pat_ref_delete_by_id() and pat_ref_set_by_id() directly use the
<refelt> argument as a valid pointer (they do dereference it).

This is wrong, because <refelt> is unsafe and should be handled as an
ID, not a pointer (hence the function name). Indeed, the calling function
may directly pass user input from the CLI as <refelt> argument, so we must
first ensure that it points to a valid element before using it, else it is
probably invalid and we shouldn't touch it.

What this patch essentially does, is that it reverts pat_ref_set_by_id()
and pat_ref_delete_by_id() to pre 5fea59754b behavior. This seems like
it was the only optimization from the patch that doesn't apply.

Hopefully, after reviewing the changes with Fred, it seems that the 2
functions are only being involved in commands for manipulating maps or
acls on the cli, so the "missed" opportunity to improve their performance
shouldn't matter much. Nonetheless, if we wanted to speed up the reference
lookup by ID, we could consider adding an eb64 tree for that specific
purpose that contains all pattern references IDs (ie: pointers) so that
eb lookup functions may be used instead of linear list search.

The issue was raised by Marko Juraga as he failed to perform an an acl
removal by reference on the CLI on 2.9 which was known to work properly
on other versions.

It should be backported on 2.9.

Co-Authored-by: Frédéric Lécaille <flecaille@haproxy.com>
(cherry picked from commit d7964c52ceb18a690d33d35b0c802660241d9ba9)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

22 months agoBUILD: ssl: update types in wolfssl cert selection callback
William Lallemand [Fri, 8 Dec 2023 10:55:15 +0000 (11:55 +0100)]
BUILD: ssl: update types in wolfssl cert selection callback

The types have changed in the PR for the wolfSSL_get_sigalg_info()
function, let's update them.

Must be backported in 2.9.

(cherry picked from commit dbe9cea35b7a43a4907fa726e7a8afc31a6fbee3)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

22 months agoBUG/MEDIUM: quic: Possible buffer overflow when building TLS records
Frédéric Lécaille [Thu, 7 Dec 2023 20:12:02 +0000 (21:12 +0100)]
BUG/MEDIUM: quic: Possible buffer overflow when building TLS records

This bug impacts only the OpenSSL QUIC compatibility module (USE_QUIC_OPENSSL_COMPAT).

This may happen only when the TLS stack has to be provided with more than 1024+1+5+16
bytes of CRYPTO data. In this case several TLS records have to be built in one
call to SSL_provide_quic_data(). A 5-bytes header is created at the head
of these records. This header is used as AAD to cipher the record. But
the length of this AAD was counted two times. One time here in
quic_tls_compat_create_record() (initialization):

 adlen = quic_tls_compat_create_header(qc, rec, ad, 0);

and a second time here in the same function after quic_tls_tls_seal() return:

     ret = aad_len + outlen;

This addition is useless. Note that this bug could be reproduced when haproxy has
to authenticate the client.

Thank you to @vifino for having reported this issue in GH #2381.

Must be backported to 2.8.

(cherry picked from commit c075e4f2fc9e662459f7ab0ce8e13b70c059334a)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

22 months agoBUG/MINOR: mworker/cli: fix set severity-output support
William Lallemand [Wed, 6 Dec 2023 10:15:01 +0000 (11:15 +0100)]
BUG/MINOR: mworker/cli: fix set severity-output support

"set severity-output" is one of these command that changes the appctx
state so the next commands are affected.

Unfortunately the master CLI works with pipelining and server close
mode, which means the connection between the master and the worker is
closed after each response, so for the next command this is a new appctx
state.

To fix the problem, 2 new flags are added ACCESS_MCLI_SEVERITY_STR and
ACCESS_MCLI_SEVERITY_NB which are used to prefix each command sent to
the worker with the right "set severity-output" command.

This patch fixes issue #2350.

It could be backported as far as 2.6.

(cherry picked from commit 1c1bb8ef2abeab2dc8e4857ec2fef2b19b472be7)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

22 months agoDOC: configuration: typo req.ssl_hello_type
William Lallemand [Thu, 7 Dec 2023 14:00:58 +0000 (15:00 +0100)]
DOC: configuration: typo req.ssl_hello_type

rep_ssl_hello_type was renamed in res.ssl_hello_type a long time ago.

This patch fixes a typo where an example was renamed
"rep.ssl_hello_type" instead of "res.ssl_hello_type"

fixes issue #2377 and #2379.

Must be backported in all maintained versions.

(cherry picked from commit 906525b16bf53c156f8b559f8782a81f2b960c2b)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

22 months agoBUG/MINOR: lua: Wrong OCSP CID after modifying an SSL certficate (LUA)
Frédéric Lécaille [Wed, 6 Dec 2023 10:42:42 +0000 (11:42 +0100)]
BUG/MINOR: lua: Wrong OCSP CID after modifying an SSL certficate (LUA)

This bugfix is the same as the following one:
    "BUG/MINOR: ssl_ckch: Wrong OCSP CID after modifying an SSL certficate"
where the OCSP CID had to be reset when updating a certificate.

Must be backported to 2.8.

(cherry picked from commit 917f7c74d3d30a3f7ac77fd6ea69cfcd529e53a9)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

22 months agoBUG/MINOR: ssl: Wrong OCSP CID after modifying an SSL certficate
Frédéric Lécaille [Tue, 5 Dec 2023 14:38:29 +0000 (15:38 +0100)]
BUG/MINOR: ssl: Wrong OCSP CID after modifying an SSL certficate

This bug could be reproduced with the "set ssl cert" CLI command to update
a certificate. The OCSP CID is duplicated by ckchs_dup() which calls
ssl_sock_copy_cert_key_and_chain(). It should be computed again by
ssl_sock_load_ocsp(). This may be accomplished resetting the new ckch OCSP CID
returned by ckchs_dup().

This bug may be in relation with GH #2319.

Must be backported to 2.8.

(cherry picked from commit 75f5977ff4ccf205ee386e4f4b0c384fb6a110d5)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

22 months agoMINOR: ssl/cli: Add ha_(warning|alert) msgs to CLI ckch callback
Frédéric Lécaille [Tue, 5 Dec 2023 14:11:33 +0000 (15:11 +0100)]
MINOR: ssl/cli: Add ha_(warning|alert) msgs to CLI ckch callback

This patch allows cli_io_handler_commit_cert() callback called upon
a "commit ssl cert ..." command to prefix the messages returned by the CLI
to the by the ones built by ha_warining(), ha_alert().

Should be interesting to backport this commit to 2.8.

(cherry picked from commit 456ba6e95f094c084c2513a594e59670b5eac2d5)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

22 months agoBUG/MINOR: ssl: Double free of OCSP Certificate ID
Frédéric Lécaille [Tue, 5 Dec 2023 13:50:40 +0000 (14:50 +0100)]
BUG/MINOR: ssl: Double free of OCSP Certificate ID

This bug could be reproduced loading several certificated from "bind" line:
with "server_ocsp.pem" as argument to "crt" setting and updating
the CDSA certificate with the RSA as follows:

echo -e "set ssl cert reg-tests/ssl/ocsp_update/multicert/server_ocsp.pem.ecdsa \
     <<\n$(cat reg-tests/ssl/ocsp_update/multicert/server_ocsp.pem.rsa)\n" | socat - /tmp/stats
followed by an "commit ssl cert reg-tests/ssl/ocsp_update/multicert/server_ocsp.pem.ecdsa"
command. This could be detected by libasan as follows:

=================================================================
==507223==ERROR: AddressSanitizer: attempting double-free on 0x60200007afb0 in thread T3:
    #0 0x7fabc6fb5527 in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.1+0x54527)
    #1 0x7fabc6ae8f8c in ossl_asn1_string_embed_free (/opt/quictls/lib/libcrypto.so.81.3+0xd4f8c)
    #2 0x7fabc6af54e9 in ossl_asn1_primitive_free (/opt/quictls/lib/libcrypto.so.81.3+0xe14e9)
    #3 0x7fabc6af5960 in ossl_asn1_template_free (/opt/quictls/lib/libcrypto.so.81.3+0xe1960)
    #4 0x7fabc6af569f in ossl_asn1_item_embed_free (/opt/quictls/lib/libcrypto.so.81.3+0xe169f)
    #5 0x7fabc6af58a4 in ASN1_item_free (/opt/quictls/lib/libcrypto.so.81.3+0xe18a4)
    #6 0x46a159 in ssl_sock_free_cert_key_and_chain_contents src/ssl_ckch.c:723
    #7 0x46aa92 in ckch_store_free src/ssl_ckch.c:869
    #8 0x4704ad in cli_release_commit_cert src/ssl_ckch.c:1981
    #9 0x962e83 in cli_io_handler src/cli.c:1140
    #10 0xc1edff in task_run_applet src/applet.c:454
    #11 0xaf8be9 in run_tasks_from_lists src/task.c:634
    #12 0xafa2ed in process_runnable_tasks src/task.c:876
    #13 0xa23c72 in run_poll_loop src/haproxy.c:3024
    #14 0xa24aa3 in run_thread_poll_loop src/haproxy.c:3226
    #15 0x7fabc69e7ea6 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x7ea6)
    #16 0x7fabc6907a2e in __clone (/lib/x86_64-linux-gnu/libc.so.6+0xfba2e)

0x60200007afb0 is located 0 bytes inside of 3-byte region [0x60200007afb0,0x60200007afb3)
freed by thread T3 here:
    #0 0x7fabc6fb5527 in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.1+0x54527)
    #1 0x7fabc6ae8f8c in ossl_asn1_string_embed_free (/opt/quictls/lib/libcrypto.so.81.3+0xd4f8c)

previously allocated by thread T2 here:
    #0 0x7fabc6fb573f in malloc (/usr/lib/x86_64-linux-gnu/libasan.so.1+0x5473f)
    #1 0x7fabc6ae8d77 in ASN1_STRING_set (/opt/quictls/lib/libcrypto.so.81.3+0xd4d77)

Thread T3 created by T0 here:
    #0 0x7fabc6f84bba in pthread_create (/usr/lib/x86_64-linux-gnu/libasan.so.1+0x23bba)
    #1 0xc04f36 in setup_extra_threads src/thread.c:252
    #2 0xa2761f in main src/haproxy.c:3917
    #3 0x7fabc682fd09 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x23d09)

Thread T2 created by T0 here:
    #0 0x7fabc6f84bba in pthread_create (/usr/lib/x86_64-linux-gnu/libasan.so.1+0x23bba)
    #1 0xc04f36 in setup_extra_threads src/thread.c:252
    #2 0xa2761f in main src/haproxy.c:3917
    #3 0x7fabc682fd09 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x23d09)

SUMMARY: AddressSanitizer: double-free ??:0 __interceptor_free
==507223==ABORTING
Aborted

The OCSP CID stored in the impacted ckch data were freed but not reset to NULL,
leading to a subsequent double free.

Must be backported to 2.8.

(cherry picked from commit 7dab3e8266b8de8604dbae0a90eade85c5c0e3f1)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

22 months ago[RELEASE] Released version 2.9.0 v2.9.0
Willy Tarreau [Tue, 5 Dec 2023 15:15:30 +0000 (16:15 +0100)]
[RELEASE] Released version 2.9.0

Released version 2.9.0 with the following main changes :
    - DOC: config: add missing colon to "bytes_out" sample fetch keyword (2)
    - BUG/MINOR: cfgparse-listen: fix warning being reported as an alert
    - DOC: config: add matrix entry for "max-session-srv-conns"
    - DOC: config: fix monitor-fail typo
    - DOC: config: add context hint for proxy keywords
    - DEBUG: stream: Report lra/fsb values for front end back SC in stream dump
    - REGTESTS: sample: Test the behavior of consecutive delimiters for the field converter
    - BUG/MINOR: sample: Make the `word` converter compatible with `-m found`
    - DOC: Clarify the differences between field() and word()
    - BUG/MINOR: server/event_hdl: properly handle AF_UNSPEC for INETADDR event
    - BUILD: http_htx: silence uninitialized warning on some gcc versions
    - MINOR: acme.sh: don't use '*' in the filename for wildcard domain
    - MINOR: global: Use a dedicated bitfield to customize zero-copy fast-forwarding
    - MINOR: mux-pt: Add global option to enable/disable zero-copy forwarding
    - MINOR: mux-h1: Add global option to enable/disable zero-copy forwarding
    - MINOR: mux-h2: Add global option to enable/disable zero-copy forwarding
    - MINOR: mux-quic: Add global option to enable/disable zero-copy forwarding
    - MINOR: mux-quic: Disable zero-copy forwarding for send by default
    - DOC: config: update the reminder on the HTTP model and add some terminology
    - DOC: config: add a few more differences between HTTP/1 and 2+
    - DOC: config: clarify session vs stream
    - DOC: config: fix typo abandonned -> abandoned
    - DOC: management: fix two latest typos (optionally, exception)
    - BUG/MEDIUM: peers: fix partial message decoding
    - DOC: management: update stream vs session

22 months agoDOC: management: update stream vs session
Willy Tarreau [Tue, 5 Dec 2023 08:30:44 +0000 (09:30 +0100)]
DOC: management: update stream vs session

Indicate for some commands such as "show sess" that we now dump streams
and not sessions.

22 months agoBUG/MEDIUM: peers: fix partial message decoding
Christopher Faulet [Tue, 5 Dec 2023 08:21:38 +0000 (09:21 +0100)]
BUG/MEDIUM: peers: fix partial message decoding

peer_recv_msg() may return because the message is incomplete without
checking if a shutdown is pending for the SC. The function relies on
co_getblk() to detect shutdowns. However, the message length decoding may be
interrupted if the multi-bytes integer is incomplete. In this case, the SC
is not check for shutdowns.

When this happens, this leads to an appctx spinning loop.

This patch should fix the issue #2373. It must be backported to 2.8.

22 months agoDOC: management: fix two latest typos (optionally, exception)
Willy Tarreau [Tue, 5 Dec 2023 03:04:50 +0000 (04:04 +0100)]
DOC: management: fix two latest typos (optionally, exception)

No backport needed, these were introduced by latest commits 3dd55fa13
("MINOR: mworker/cli: implement hard-reload over the master CLI") and
cef29d370 ("MINOR: trace: define simple -dt argument").

22 months agoDOC: config: fix typo abandonned -> abandoned
Willy Tarreau [Tue, 5 Dec 2023 03:02:25 +0000 (04:02 +0100)]
DOC: config: fix typo abandonned -> abandoned

No need to backport, it was introduced by recent commit fafa34e5f ("DOC:
config: update the reminder on the HTTP model and add some terminology").

22 months agoDOC: config: clarify session vs stream
Willy Tarreau [Mon, 4 Dec 2023 17:57:12 +0000 (18:57 +0100)]
DOC: config: clarify session vs stream

Rename "session" to "stream" where relevant (termination states, queue,
and so on).

22 months agoDOC: config: add a few more differences between HTTP/1 and 2+
Willy Tarreau [Mon, 4 Dec 2023 17:34:19 +0000 (18:34 +0100)]
DOC: config: add a few more differences between HTTP/1 and 2+

Mention the lack of reason phrase and the existence of pseudo-headers for
the request and the response.

22 months agoDOC: config: update the reminder on the HTTP model and add some terminology
Willy Tarreau [Mon, 4 Dec 2023 17:16:52 +0000 (18:16 +0100)]
DOC: config: update the reminder on the HTTP model and add some terminology

It was really necessary to try to clear the confusion between sessions
and streams, so let's first lift a little bit the HTTP model part to
better consider new protocols, and explain what a stream is and how this
differs from the earlier sessions.

22 months agoMINOR: mux-quic: Disable zero-copy forwarding for send by default
Christopher Faulet [Mon, 4 Dec 2023 14:36:01 +0000 (15:36 +0100)]
MINOR: mux-quic: Disable zero-copy forwarding for send by default

There is at least an bug for now in this part and it is still unstable. Thus
it is better to disable it for now by default. It can be enable by setting
tune.quic.zero-copy-fwd-send to 'on'.

22 months agoMINOR: mux-quic: Add global option to enable/disable zero-copy forwarding
Christopher Faulet [Mon, 4 Dec 2023 14:26:23 +0000 (15:26 +0100)]
MINOR: mux-quic: Add global option to enable/disable zero-copy forwarding

tune.quic.zero-copy-fwd-send can now be used to enable or disable the
zero-copy fast-forwarding for the QUIC mux only, for sends. For now, there
is no option to disable it for receives because it is not supported yet.

It is enabled ('on') by default.

22 months agoMINOR: mux-h2: Add global option to enable/disable zero-copy forwarding
Christopher Faulet [Mon, 4 Dec 2023 14:18:49 +0000 (15:18 +0100)]
MINOR: mux-h2: Add global option to enable/disable zero-copy forwarding

tune.h2.zero-copy-fwd-send can now be used to enable or disable the
zero-copy fast-forwarding for the H2 mux only, for sends. For now, there is
no option to disable it for receives because it is not supported yet.

It is enabled ('on') by default.

22 months agoMINOR: mux-h1: Add global option to enable/disable zero-copy forwarding
Christopher Faulet [Mon, 4 Dec 2023 14:06:06 +0000 (15:06 +0100)]
MINOR: mux-h1: Add global option to enable/disable zero-copy forwarding

tune.h1.zero-copy-fwd-recv and tune.h1.zero-copy-fwd-send can now be used to
enable or disable the zero-copy fast-forwarding for the H1 mux only, for
receives or sends. Unlike the PT mux, there are 2 options here because
client and server sides can use difference muxes.

Both are enabled ('on') by default.

22 months agoMINOR: mux-pt: Add global option to enable/disable zero-copy forwarding
Christopher Faulet [Mon, 4 Dec 2023 13:48:52 +0000 (14:48 +0100)]
MINOR: mux-pt: Add global option to enable/disable zero-copy forwarding

tune.pt.zero-copy-forwarding parameter can now be used to enable or disable
the zero-copy fast-forwarding for the PT mux only. It is enabled ('on') by
default. It can be disabled by setting the parameter to 'off'. In this case,
this disables receive and send side.

22 months agoMINOR: global: Use a dedicated bitfield to customize zero-copy fast-forwarding
Christopher Faulet [Mon, 4 Dec 2023 13:18:50 +0000 (14:18 +0100)]
MINOR: global: Use a dedicated bitfield to customize zero-copy fast-forwarding

Zero-copy fast-forwading feature is a quite new and is a bit sensitive.
There is an option to disable it globally. However, all protocols have not
the same maturity. For instance, for the PT multiplexer, there is nothing
really new. The zero-copy fast-forwading is only another name for the kernel
splicing. However, for the QUIC/H3, it is pretty new, not really optimized
and it will evolved. And soon, the support will be added for the cache
applet.

In this context, it is usefull to be able to enable/disable zero-copy
fast-forwading per-protocol and applet. And when it is applicable, on sends
or receives separately. So, instead of having one flag to disable it
globally, there is now a dedicated bitfield, global.tune.no_zero_copy_fwd.

22 months agoMINOR: acme.sh: don't use '*' in the filename for wildcard domain
William Lallemand [Mon, 4 Dec 2023 10:52:31 +0000 (11:52 +0100)]
MINOR: acme.sh: don't use '*' in the filename for wildcard domain

By default acme.sh uses the '*' character in the filename for wildcard.
That can be confusing within HAProxy since the * character in front of a
filename in the stat socket is used to specified an uncommitted
transaction.

This patch replace the '*' by a '_' in the filename.  This is only done
when using the default filename, the name can still be forced with an
asterisk.

22 months agoBUILD: http_htx: silence uninitialized warning on some gcc versions
Willy Tarreau [Fri, 1 Dec 2023 14:29:44 +0000 (15:29 +0100)]
BUILD: http_htx: silence uninitialized warning on some gcc versions

Building on gcc 4.4 reports "start may be used uninitialized". This is
a classical case of dependency between two variables where the compiler
lost track of their initialization and doesn't know that if one is not
set, the other is. By just moving the second test in the else clause
of the assignment both fixes it and makes the code more efficient, and
this can be simplified as a ternary operator.

It's probably not needed to backport this, unless anyone reports build
warnings with more recent compilers (intermediary optimization levels
such as -O1 can sometimes trigger such warnings).

22 months agoBUG/MINOR: server/event_hdl: properly handle AF_UNSPEC for INETADDR event
Aurelien DARRAGON [Fri, 1 Dec 2023 16:41:40 +0000 (17:41 +0100)]
BUG/MINOR: server/event_hdl: properly handle AF_UNSPEC for INETADDR event

It is possible that a server's addr family is temporarily set to AF_UNSPEC
even if we're certain to be in INET context (ipv4, ipv6).

Indeed, as soon as IP address resolving is involved, srv->addr family will
be set to AF_UNSPEC when the resolution fails (could happen at anytime).

However, _srv_event_hdl_prepare_inetaddr() wrongly assumed that it would
only be called with AF_INET or AF_INET6 families. Because of that, the
function will handle AF_UNSPEC address as an IPV6 address: not only
we could risk reading from an unititialized area, but we would then
propagate false information when publishing the event.

In this patch we make sure to properly handle the AF_UNSPEC family in
both the "prev" and the "next" part for SERVER_INETADDR event and that
every members are explicitly initialized.

This bug was introduced by 6fde37e046 ("MINOR: server/event_hdl: add
SERVER_INETADDR event"), no backport needed.

22 months agoDOC: Clarify the differences between field() and word()
Tim Duesterhus [Thu, 30 Nov 2023 15:41:18 +0000 (16:41 +0100)]
DOC: Clarify the differences between field() and word()

word() mentions that delimiters at the start and end are ignored, but
it does not mention that consecutive delimiters are merged.

May be backported as far as the patch applies.

22 months agoBUG/MINOR: sample: Make the `word` converter compatible with `-m found`
Tim Duesterhus [Thu, 30 Nov 2023 15:41:17 +0000 (16:41 +0100)]
BUG/MINOR: sample: Make the `word` converter compatible with `-m found`

Previously an expression like:

    path,word(2,/) -m found

always returned `true`.

Bug exists since the `word` converter exists. That is:
c9a0f6d0232cf44d6b08d1964b9097a45a6c65f0

The same bug was previously fixed for the `field` converter in commit
4381d26edc03faa46401eb0fe82fd7be84be14fd.

The fix should be backported to 1.6+.

22 months agoREGTESTS: sample: Test the behavior of consecutive delimiters for the field converter
Tim Duesterhus [Thu, 30 Nov 2023 15:41:16 +0000 (16:41 +0100)]
REGTESTS: sample: Test the behavior of consecutive delimiters for the field converter

This is in preparation of a follow-up patch to fix the word converter.

22 months agoDEBUG: stream: Report lra/fsb values for front end back SC in stream dump
Christopher Faulet [Fri, 1 Dec 2023 10:16:53 +0000 (11:16 +0100)]
DEBUG: stream: Report lra/fsb values for front end back SC in stream dump

REX and WEX date are already reported. But if the corresponding SC cannot
expire on read or write, "<NEVER>" is reported instead. The same is reported
if no expiration date is set. It is not really convenient because we cannot
distinguish the two cases.

So, now, for each SC, read and wirte timer (rto/wto) are also reported in
the dump, based on .lra/.fsb dates and the current I/O timeout. The SC I/O
timeout is also reported.

22 months agoDOC: config: add context hint for proxy keywords
Aurelien DARRAGON [Mon, 20 Nov 2023 16:53:17 +0000 (17:53 +0100)]
DOC: config: add context hint for proxy keywords

Add a small list of contexts where each proxy keyword is expected to be
employed. (Similar to the defaults/frontend/backend/listen compatibility
grid).

22 months agoDOC: config: fix monitor-fail typo
Aurelien DARRAGON [Thu, 30 Nov 2023 10:11:43 +0000 (11:11 +0100)]
DOC: config: fix monitor-fail typo

monitor-fail doesn't exist, but it was mentionned in the documentation.
Fixing it with "monitor fail" instead.

22 months agoDOC: config: add matrix entry for "max-session-srv-conns"
Aurelien DARRAGON [Wed, 29 Nov 2023 09:13:18 +0000 (10:13 +0100)]
DOC: config: add matrix entry for "max-session-srv-conns"

Following 4039329 ("DOC: config: specify supported sections for
"max-session-srv-conns"), "max-session-srv-conns" was also missing
from the proxy keyword matrix.

22 months agoBUG/MINOR: cfgparse-listen: fix warning being reported as an alert
Aurelien DARRAGON [Wed, 29 Nov 2023 14:03:25 +0000 (15:03 +0100)]
BUG/MINOR: cfgparse-listen: fix warning being reported as an alert

Since b40542000d ("MEDIUM: proxy: Warn about ambiguous use of named
defaults sections") we introduced a new error to prevent user from
having an ambiguous named default section in the config which is both
inherited explicitly using "from" and implicitly by another proxy due to
the default section being the last one defined.

However, despite the error message being presented as a warning the
err_code, the commit message and the documentation, it is actually
reported as a fatal error because ha_alert() was used in place of
ha_warning().

In this patch we make the code comply with the documentation and the
intended behavior by using ha_warning() to report the error message.

This should be backported up to 2.6.

22 months agoDOC: config: add missing colon to "bytes_out" sample fetch keyword (2)
Tim Duesterhus [Thu, 30 Nov 2023 19:15:32 +0000 (20:15 +0100)]
DOC: config: add missing colon to "bytes_out" sample fetch keyword (2)

This reapplies 1eb049dc677f2de950158615ed3d8306ee5102d6, as the change was
accidentally reverted in 5ef48e063ecf992646c7af374153f106050fb8ec.

22 months ago[RELEASE] Released version 2.9-dev12 v2.9-dev12
Willy Tarreau [Thu, 30 Nov 2023 17:07:06 +0000 (18:07 +0100)]
[RELEASE] Released version 2.9-dev12

Released version 2.9-dev12 with the following main changes :
    - BUG/MINOR: global: Fix tune.disable-(fast-forward/zero-copy-forwarding) options
    - DOC: config: removing "log-balance" references
    - MINOR: server/event_hdl: add SERVER_INETADDR event
    - MINOR: tools: use const for read only pointers in ip{cmp,cpy}
    - MINOR: server/ip: centralize server ip updates
    - MINOR: backend: remove invalid mode test for "hash-balance-factor"
    - Revert "MINOR: cfgparse-listen: warn when use-server rules is used in wrong mode"
    - MINOR: proxy: add free_logformat_list() helper function
    - MINOR: proxy: add free_server_rules() helper function
    - MINOR: log/backend: prevent "use-server" rules use with LOG mode
    - MINOR: log/balance: set lbprm tot_weight on server on queue/dequeue
    - DOC: config: specify supported sections for "max-session-srv-conns"
    - DOC: config: fix timeout check inheritance restrictions
    - REGTESTS: connection: disable http_reuse_be_transparent.vtc if !TPROXY
    - DOC: lua: add sticktable class reference from Proxy.stktable
    - DOC: lua: fix Proxy.get_mode() output
    - DOC: lua: add "syslog" to Proxy.get_mode() output
    - MEDIUM: ssl: implement rsa/ecdsa selection with WolfSSL
    - MINOR: ssl: replace 'trash.area' by 'servername' in ssl_sock_switchctx_cbk()
    - MINOR: ssl: move certificate selection in a dedicate function
    - MEDIUM: ssl: use ssl_sock_chose_sni_ctx() in the clienthello callback
    - MINOR: mworker/cli: implement hard-reload over the master CLI
    - BUG/MEDIUM: mux-h1: Properly ignore trailers when a content-length is announced
    - MINOR: task/profiling: do not record task_drop_running() as a caller
    - OPTIM: pattern: save memory and time using ebst instead of ebis
    - BUILD: map: fix build warning
    - MINOR: trace: define simple -dt argument
    - MINOR: trace: parse level in a function
    - MINOR: trace: parse verbosity in a function
    - MINOR: trace: support -dt optional format
    - OPTIM: mux-h2/zero-copy: don't allocate more buffers per connections than streams
    - BUG/MINOR: quic: fix CONNECTION_CLOSE_APP encoding
    - BUG/MEDIUM: stconn: Don't perform zero-copy FF if opposite SC is blocked
    - BUG/MEDIUM: mux-h2: Remove H2_SF_NOTIFIED flag for H2S blocked on fast-forward
    - CLEANUP: quic: Remove dead definitions/declarations
    - REORG: quic: Move some QUIC CLI code to its C file
    - REORG: quic: Add a new module to handle QUIC connection IDs
    - REORG: quic: QUIC connection types header cleaning
    - BUILD: quic: Missing RX header inclusions
    - REORG: quic: Move CRYPTO data buffer defintions to QUIC TLS module
    - REORG: quic: Move QUIC CRYPTO stream definitions/declarations to QUIC TLS
    - REORG: quic: Move several inlined functions from quic_conn.h
    - REORG: quic: Move QUIC SSL BIO method related functions to quic_ssl.c
    - REORG: quic: Move the QUIC DCID parser to quic_sock.c
    - REORG: quic: Rename some functions used upon ACK receipt
    - REORG: quic: Move QUIC path definitions/declarations to quic_cc module
    - REORG: quic: Move qc_handle_conn_migration() to quic_conn.c
    - REORG: quic: Move quic_build_post_handshake_frames() to quic_conn module
    - REORG: quic: Move qc_may_probe_ipktns() to quic_tls.h
    - REORG: quic: Move qc_pkt_long() to quic_rx.h
    - REORG: quic: Rename some (quic|qc)_conn* objects to quic_conn_closed
    - REORG: quic: Move NEW_CONNECTION_ID frame builder to quic_cid
    - REORG: quic: Move ncbuf related function from quic_rx to quic_conn
    - REORG: quic: Add a new module for QUIC retry
    - BUILD: quic: Several compiler warns fixes after retry module creation
    - REORG: quic: Move qc_notify_send() to quic_conn
    - REORG: quic: Add a new module for retransmissions
    - REORG: quic: Remove qc_pkt_insert() implementation
    - REORG: quic: Move quic_increment_curr_handshake() to quic_sock
    - BUG/MINOR: cache: Remove incomplete entries from the cache when stream is closed
    - MEDIUM: cli: allow custom pattern for payload
    - CLEANUP: mworker/cli: use a label to return errors
    - MINOR: mworker/cli: implements the customized payload pattern for master CLI
    - DOC: management: add documentation about customized payload pattern
    - BUG/MEDIUM: server/event_hdl: memory overrun in _srv_event_hdl_prepare_inetaddr()
    - MINOR: event_hdl: add global tunables
    - BUG/MAJOR: server/addr: fix a race during server addr:svc_port updates
    - MEDIUM: log/balance: support FQDN for UDP log servers
    - BUG/MINOR: compression: possible NULL dereferences in comp_prepare_compress_request()
    - BUG/MEDIUM: master/cli: Properly pin the master CLI on thread 1 / group 1
    - BUG/MEDIUM: mux-quic: Stop zero-copy FF during nego if input is not empty
    - CLEANUP: log: Fix %rc comment in sess_build_logline()
    - BUG/MINOR: h3: fix TRAILERS encoding
    - BUG/MINOR: h3: always reject PUSH_PROMISE
    - MINOR: h3: use correct error code for missing SETTINGS
    - MINOR: http-fetch: Add a sample to retrieve the server status code
    - DOC: config: Improve 'status' sample documentation
    - MINOR: http-fetch: Add a sample to get the transaction status code
    - MEDIUM: http-ana: Set termination state before returning haproxy response
    - MINOR: stream: Expose session terminate state via a new sample fetch
    - MINOR: stream: add a sample fetch to get the number of connection retries
    - MINOR: stream: Expose the stream's uniq_id via a new sample fetch
    - MINOR: muxes: Rename mux_ctl_type values to use MUX_CTL_ prefix
    - MINOR: muxes: Add a callback function to send commands to mux streams
    - MINOR: muxes: Implement ->sctl() callback for muxes and return the stream id
    - MINOR: Add sample fetches to get the frontend and backend stream ID
    - BUG/MEDIUM: cli: Don't look for payload pattern on empty commands
    - DOC: config: Add argument for tune.lua.maxmem
    - DOC: config: fix mention of request slot in http-response capture
    - DOC: config: fix remaining mention of @reverse for attach-srv action
    - DOC: config: fix missing characters in set-spoe-group action
    - DOC: config: reorganize actions into their own section
    - BUG/MINOR: acme.sh: update the deploy script
    - MINOR: rhttp: mark reverse HTTP as experimental
    - CLEANUP: quic_cid: remove unused listener arg
    - BUG/MINOR: quic_tp: fix preferred_address decoding
    - MINOR: quic_tp: use in_addr/in6_addr for preferred_address
    - MINOR: acme.sh: use the master CLI for hot update
    - DOC: config: move the cache-use and cache-store actions to the proper section
    - DOC: config: fix alphabetical ordering of converter keywords
    - DOC: config: add missing colon to "bytes_out" sample fetch keyword
    - DOC: config: add an index of converter keywords
    - DOC: config: add an index of sample fetch keywords
    - BUG/MINOR: config: Stopped parsing upon unmatched environment variables
    - DEBUG: unstatify a few functions that are often present in backtraces
    - BUILD: server: shut a bogus gcc warning on certain ubuntu

22 months agoBUILD: server: shut a bogus gcc warning on certain ubuntu
Willy Tarreau [Thu, 30 Nov 2023 16:48:03 +0000 (17:48 +0100)]
BUILD: server: shut a bogus gcc warning on certain ubuntu

On ubuntu 20.04 and 22.04 with gcc 9.4 and 11.4 respectively, we get
the following warning:

  src/server.c: In function 'srv_update_addr_port':
  src/server.c:4027:3: warning: 'new_port' may be used uninitialized in this function [-Wmaybe-uninitialized]
   4027 |   _srv_event_hdl_prepare_inetaddr(&cb_data.addr, &s->addr, s->svc_port,
        |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   4028 |                                   ((ip_change) ? &sa : &s->addr),
        |                                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   4029 |                                   ((port_change) ? new_port : s->svc_port),
        |                                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   4030 |                                   1);
        |                                   ~~

It's clearly wrong, port_change only changes from 0 to anything else
*after* assigning new_port. Let's just preset new_port to zero instead
of trying to play smart with the compiler.

22 months agoDEBUG: unstatify a few functions that are often present in backtraces
Willy Tarreau [Thu, 30 Nov 2023 16:04:16 +0000 (17:04 +0100)]
DEBUG: unstatify a few functions that are often present in backtraces

It's useful to be able to recognize certain functions that are often
present in backtraces as they call lower level functions, and for this
they must not be static. Let's remove "static" in front of these
functions:

  sc_notify, sc_conn_recv, sc_conn_send, sc_conn_process,
  sc_applet_process, back_establish, stream_update_both_sc,
  httpclient_applet_io_handler, httpclient_applet_init,
  httpclient_applet_release

22 months agoBUG/MINOR: config: Stopped parsing upon unmatched environment variables
Frédéric Lécaille [Thu, 30 Nov 2023 08:26:05 +0000 (09:26 +0100)]
BUG/MINOR: config: Stopped parsing upon unmatched environment variables

When an environment variable could not be matched by getenv(), the
current character to be parsed by parse_line() from <in> variable
is the trailing double quotes. If nothing is done in such a case,
this character is skipped by parse_line(), then the following spaces
are parsed as an empty argument.

To fix this, skip the double quotes character and the following spaces
to make <in> variable point to the next argument to be parsed.

Thank you to @sigint2 for having reported this issue in GH #2367.

Must be backported as far as 2.4.

22 months agoDOC: config: add an index of sample fetch keywords
Willy Tarreau [Thu, 30 Nov 2023 15:00:14 +0000 (16:00 +0100)]
DOC: config: add an index of sample fetch keywords

Now we're adding a table for each section, it allows to more easily
spot the list of available sample fetch functions and their types.
For now the arguments are not mentioned in the table because they'd
break indexing but they can be added back later.