MINOR: ssl_sock: Early data disabled during SSL_CTX switching (aws-lc)
This patch impacts only haproxy when built against aws-lc TLS stack (OPENSSL_IS_AWSLC).
During the SSL_CTX switching from ssl_sock_switchctx_cbk() callback,
ssl_sock_switchctx_set() is called. This latter calls SSL_set_SSL_CTX()
whose aims is to change the SSL_CTX attached o an SSL object (TLS session).
But the aws-lc (or boringssl) implementation of this function copy
the "early data enabled" setting value (boolean) coming with the SSL_CTX object
into the SSL object. So, if not set in the SSL_CTX object this setting disabled
the one which has been set by configuration into the SSL object
(see qc_set_quic_early_data_enabled(), it calls SSL_set_early_data_enabled()
with an SSL object as parameter).
Fix this enabling the "early data enabled" setting into the SSL_CTX before
setting this latter into the SSL object.
This patch is required to make QUIC 0-RTT work with haproxy built against
aws-lc.
Note that, this patch should also help in early data support for TCP connections.
MINOR: quic: Enable early data at SSL session level (aws-lc)
This patch impacts only the haproxy build against aws-lc TLS stack (USE_OPENSSL_AWSLC).
Implement qc_set_quic_early_data_enabled() new function to enable
early data at session level. To make QUIC O-RTT work, a context string
must be set calling SSL_set_quic_early_data_context(). This is a
subset of the encoded transport parameters which is used for this.
Note that some application level settings should be also added (TODO).
This patch is required to make 0-RTT work for haproxy builds against aws-lc.
MINOR: quic: Transport parameters encoding without version_information
Encode the version_information parameter only if the chosen version is provided
to quic_transport_params_encode() whose aim is to encode into a buffer all the
transport parameters passed as parameter (struct quic_params *p) in addition
to the version_information parameter.
This enables the support of transport parameters encoding without
the version_information transport parameter. This is useful for build against TLS stacks
as boringssl, aws-lc where a subset of the listener transport parameters
without version_information must be set as context string for acception
early data (see https://commondatastorage.googleapis.com/chromium-boringssl-docs/ssl.h.html#SSL_set_quic_early_data_context).
This patch is required to make haproxy builds against aws-lc TLS stack
(USE_OPENSSL_AWSLC) support 0-RTT. Does not impact the others builds.
Willy Tarreau [Sun, 21 Jan 2024 07:21:35 +0000 (08:21 +0100)]
BUILD: stick-table: fix build error on 32-bit platforms
Commit 9b2717e7b ("MINOR: stktable: use {show,set,clear} table with ptr")
stores a pointer in a long long (64bit), which fails the cas to void* on
32-bit platforms:
src/stick_table.c: In function 'table_process_entry_per_ptr':
src/stick_table.c:5136:37: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
5136 | ts = stktable_lookup_ptr(t, (void *)ptr);
On all our supported platforms, longs and pointers are of the same size,
so let's just turn this to a ulong instead.
Willy Tarreau [Wed, 17 Jan 2024 17:00:21 +0000 (18:00 +0100)]
MINOR: connection: add sample fetches to report per-connection glitches
Now with fc_glitches and bc_glitches we can retrieve the number of
detected glitches on a front or back connection. On the backend it
can indicate a bug in a server that may induce frequent reconnections
hence CPU usage in TLS reconnections, and on the frontend it may
indicate an abusive client that may be trying to attack the stack
or to fingerprint it. Small non-zero values are definitely expected
and can be caused by network glitches for example, as well as rare
bugs in the other component (or maybe even in haproxy). These should
never be considered as alarming as long as they remain low (i.e.
much less than one per request). A reg-test is provided.
Willy Tarreau [Wed, 17 Jan 2024 15:57:23 +0000 (16:57 +0100)]
MINOR: mux-h2: add a counter of "glitches" on a connection
There are a lot of H2 events which are not invalid from a protocol
perspective but which are yet anomalies, especially when repeated. They
can come from bogus or really poorly implemlented clients, as well as
purposely built attacks, as we've seen in the past with various waves
of attempts at abusing H2 stacks.
In order to better deal with such situations, it would be nice to be
able to sort out what is correct and what is not. There's already the
HTTP error counter that may even be updated on a tracked connection,
but HTTP errors are something clearly defined while there's an entire
scope of gray area around it that should not fall into it.
This patch introduces the notion of "glitches", which normally correspond
to unexpected and temporary malfunction. And this is exactly what we'd
like to monitor. For example a peer is not misbehaving if a request it
sends fails to decode because due to HPACK compression it's larger than
a buffer, and for this reason such an event is reported as a stream error
and not a connection error. But this causes trouble nonetheless and should
be accounted for, especially to detect if it's repeated. Similarly, a
truncated preamble or settings frame may very well be caused by a network
hiccup but how do we know that in the logs? For such events, a glitch
counter is incremented on the connection.
For now a total of 41 locations were instrumented with this and the
counter is reported in the traces when not null, as well as in
"show sess" and "show fd". This was done using a new function,
"h2c_report_glitch()" so that it becomes easier to extend to more
advanced processing (applying thresholds, producing logs, escalating
to connection error, tracking etc).
A test with h2spec shows it reported in 8545 trace lines for 147 tests,
with some reaching value 3 in a same test (e.g. HPACK errors).
Some places were not instrumented, typically anything that can be
triggered on perfectly valid activity (received data after RST being
emitted, timeouts, etc). Some types of events were thought about,
such as INITIAL_WINDOW_SIZE after the first SETTINGS frame, too small
window update increments, etc. It just sounds too early to know if
those are currently being triggered by perfectly legit clients. Also
it's currently not incremented on timeouts so that we don't do that
repeatedly on short keep-alive timeouts, though it could make sense.
This may change in the future depending on how it's used. For now
this is not exposed outside of traces and debugging.
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.
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.
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.
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.
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.
Willy Tarreau [Fri, 12 Jan 2024 16:53:50 +0000 (17:53 +0100)]
DEV: h2: support hex-encoded data sequences in mkhdr
For HPACK-encoded headers (particularly with huffman encoding), it's
really necessary to support hex sequences as they appear in RFC7541
examples, so let's support hex digit pairs with -R.
Now it's possible to do this to send GET https://www.example.com/ :
MEDIUM: ssl: generate '*' SNI filters for default certificates
This patch follows the previous one about default certificate selection
("MEDIUM: ssl: allow multiple fallback certificate to allow ECDSA/RSA
selection").
This patch generates '*" SNI filters for the first certificate of a
bind line, it will be used to match default certificates. Instead of
setting the default_ctx pointer in the bind line.
Since the filters are in the SNI tree, it allows to have multiple
default certificate and restore the ecdsa/rsa selection with a
multi-cert bundle.
This configuration:
# foobar.pem.ecdsa and foobar.pem.rsa
bind *:8443 ssl crt foobar.pem crt next.pem
will use "foobar.pem.ecdsa" and "foobar.pem.rsa" as default
certificates.
Note: there is still cleanup needed around default_ctx.
MEDIUM: ssl: allow multiple fallback certificate to allow ECDSA/RSA selection
This patch changes the default certificate mechanism.
Since the beginning of SSL in HAProxy, the default certificate was the first
certificate of a bind line. This allowed to fallback on this certificate
when no servername extension was sent by the server, or when no SAN nor
CN was available in the certificate.
When using a multi-certificate bundle (ecdsa+rsa), it was possible to
have both certificates as the fallback one, leting openssl chose the
right one. This was possible because a multi-certificate bundle
was generating a unique SSL_CTX for both certificates.
When the haproxy and openssl architecture evolved, we decided to
use multiple SSL_CTX for a multi-cert bundle, in order to simplify the
code and allow updates over the CLI.
However only one default_ctx was allowed, so we lost the ability to
chose between ECDSA and RSA for the default certificate.
This patch allows to use a '*' filter for a certificate, which allow to
lookup between multiple '*' filter, and have one in RSA and another one
in ECDSA. It replaces the default_ctx mechanism in the ClientHello
callback and use the standard algorithm to look for a default cert and
chose between ECDSA and RSA.
/!\ This patch breaks the automatic setting of the default certificate, which
will be introduce in the next patch. So the first certificate of a bind
line won't be used as a defaullt anymore.
To use this feature, one could use crt-list with '*' filters:
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.
Ilya Shipitsin [Thu, 11 Jan 2024 19:49:09 +0000 (20:49 +0100)]
CI: codespell: ignore some words in URLs
"trafic,ressources" are found in URIs, due to
https://github.com/codespell-project/actions-codespell/issues/55 we cannot use
wildcard for exclusion, let start with fixed list
Willy Tarreau [Thu, 11 Jan 2024 11:06:49 +0000 (12:06 +0100)]
MEDIUM: http: add the ability to redefine http-err-codes and http-fail-codes
The new global keywords "http-err-codes" and "http-fail-codes" allow to
redefine which HTTP status codes indicate a client-induced error or a
server error, as tracked by stick-table counters. This is only done
globally, though everything was done so that it could easily be extended
to a per-proxy mechanism if there was a real need for this (but it would
eat quite more RAM then).
Willy Tarreau [Wed, 10 Jan 2024 17:44:30 +0000 (18:44 +0100)]
MEDIUM: http_act: check status codes against the bit fields for err/fail
This drops the hard-coded 4xx and 5xx status codes for err_cnt and
fail_cnt, in favor of the new bit fields that will soon be configurable.
There should be no difference at all since the bit fields are initialized
to the exact same sets (400-499 for err, 500-599 minus 501 and 505 for
fail).
Willy Tarreau [Wed, 10 Jan 2024 17:36:12 +0000 (18:36 +0100)]
MINOR: http: add infrastructure to choose status codes for err / fail
At the moment, http_err_cnt and http_fail_cnt are incremented on a
well-defined set of status codes, which are checked at various places.
Over time, there have been some complains about 404, 401 or 407
triggering errors, or 500 triggering failures in SOAP environments
for example. With a small bit field that fits in a cache line we
can match the presence of a status code from 100 to 599, so that
remains cheap.
This patch adds two such bit fields, one per code class, and the
accompanying functions to set/clear/test the codes. The arrays are
preset at boot time. For now they are not used and it's not possible
to adjust them.
Willy Tarreau [Wed, 10 Jan 2024 10:28:28 +0000 (11:28 +0100)]
CLEANUP: http: avoid duplicating literals in find_http_meth()
The function does the inverse of http_known_methods[], better rely on
that array with its indices, that makes the code clearer. Note that
we purposely don't use a loop because the compiler is able to build
an evaluation tree of the size checks and content checks that's very
efficient for the most common methods. Moving a few unimportant
entries even simplified the output code a little bit (they're now
groupped by size without changing anything for the first ones).
Willy Tarreau [Wed, 10 Jan 2024 15:34:48 +0000 (16:34 +0100)]
OPTIM: http: simplify http_get_status_idx() using a hash
This function uses a large switch/case, but the problem is that due to the
numerous holes in the range, the compiler implemented a large jump table.
With a bit of experimentations, some trivial perfect-hash code works, and
since the number of entries is 19, it was enlarged to match the nearest
next power of two to avoid a large modulo operation, and fills the holes
with the default return value (HTTP_ERR_500). Jumping to 32 also results
in a lot of valid keys and allows us to pick small values, resulting in
very fast and compact code. The new function, despite keeping a 32-bytes
table, saves slightly more than 800 bytes of code+data compared to the
previous code, and avoids table jumps that affect the CPU's branch history.
Note that another simple hash worked fine and produced exactly 19 codes
(hence no need to pad holes): ((status * 8675725) >> 13) % 19
But it's still about 24 bytes larger in code to save 13 bytes of data
that are aligned anyway, and it was a bit more expensive so that was
definitely not worth it.
The validity of the table was verified with this test code added just after
it:
__attribute__((constructor)) void http_hash_test(void)
{
int i;
for (i = 0; i <= 600; i++)
printf("code %d => %d\n", i, http_get_status_idx(i));
exit(0);
}
And starting haproxy |grep -vw 14 correctly shows all ordered values
(except 500 of course which is 14).
In case new codes would be added, just play again with dev/phash to
updated the table. As long as there are less than 32 effective entries
it will remain easy to update without having to modify phash.
Willy Tarreau [Wed, 10 Jan 2024 15:25:58 +0000 (16:25 +0100)]
DEV: phash: add a trivial perfect hash generator for integers
We have a few places where we're checking many status codes. The sets
are so small that just a multiply, a shift and an optional xor are
sufficient to turn that into a smaller set. The program used to produce
them is hackish as it allows to easily fiddle with various operations
manually and experiment by brute-forcing a pair of integers for the
mul and the xor. It also supports producing an incomplete table that
gives an easier modulo operation. Let's commit it as-is so that it can
be reused later (e.g. if new status codes are introduced for example).
MINOR: map: mapfile ordering also matters for tree-based match types
Willy made me realize that tree-based matching may also suffer from
out-of-order mapfile loading, as opposed to what's being said in b546bb6d ("BUG/MINOR: map: list-based matching potential ordering
regression") and the associated REGTEST.
Indeed, in case of duplicated keys, we want to be sure that only the key
that was first seen in the file will be returned (as long as it is not
removed). The above fix is still valid, and the list-based match regtest
will also prevent regressions for tree-based match since mapfile loading
logic is currently match-type agnostic.
But let's clarify that by making both the code comment and the regtest
more precise.
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.
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.
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.
Willy Tarreau [Tue, 9 Jan 2024 13:44:08 +0000 (14:44 +0100)]
DEV: patchbot: produce a verdict for too long commit messages
Some rare commit messages area really too large because they contain
code excerpts in the message body or are release commits with their
changelog. In this case, instead of leaving an empty file that will
be silently ignored, let's produce an output message indicating that
the verdict is uncertain, with an explanation stating that there was
an error.
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).
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.
Willy Tarreau [Sat, 6 Jan 2024 13:09:35 +0000 (14:09 +0100)]
[RELEASE] Released version 3.0-dev1
Released version 3.0-dev1 with the following main changes :
- MINOR: channel: Use dedicated functions to deal with STREAMER flags
- MEDIUM: applet: Handle channel's STREAMER flags on applets size
- MINOR: applets: Use channel's field to compute amount of data received
- MEDIUM: cache: Save body size of cached objects and track it on delivery
- MEDIUM: cache: Add support for endp-to-endp fast-forwarding
- MINOR: cache: Add global option to enable/disable zero-copy forwarding
- MINOR: pattern: Use reference name as filename to read patterns from a file
- MEDIUM: pattern: Add support for virtual and optional files for patterns
- DOC: config: Add section about name format for maps and ACLs
- DOC: management/lua: Update commands about map and acl
- MINOR: promex: Add support for specialized front/back/li/srv metric names
- MINOR: promex: Export active/backup metrics per-server
- 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
- MINOR: hq-interop: add fastfwd support
- CLEANUP: mux_quic: rename ffwd function with prefix qmux_strm_
- MINOR: mux-quic: add traces for 0-copy/fast-forward
- BUG/MINOR: mworker/cli: fix set severity-output support
- CLEANUP: mworker/cli: add comments about pcli_find_and_exec_kw()
- BUG/MEDIUM: quic: Possible buffer overflow when building TLS records
- BUILD: ssl: update types in wolfssl cert selection callback
- MINOR: ssl: activate the certificate selection callback for WolfSSL
- CI: github: switch to wolfssl git-c4b77ad for new PR
- BUG/MEDIUM: map/acl: pat_ref_{set,delete}_by_id regressions
- BUG/MINOR: ext-check: cannot use without preserve-env
- CLEANUP: mux-quic: remove unused prototype
- MINOR: mux-quic: clean up qcs Rx buffer allocation API
- MINOR: mux-quic: clean up qcs Tx buffer allocation API
- CLEANUP: mux-quic: clean up app ops callback definitions
- MINOR: mux-quic: factorize QC_SF_UNKNOWN_PL_LENGTH set
- MINOR: h3: complete traces for sending
- MINOR: h3: adjust zero-copy sending related code
- MINOR: hq-interop: use zero-copy to transfer single HTX data block
- 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
- BUG/MINOR: resolvers: default resolvers fails when network not configured
- SCRIPTS: mk-patch-list: produce a list of patches
- DEV: patchbot: add the AI-based bot to pre-select candidate patches to backport
- BUG/MEDIUM: mux-h2: Switch pending error to error if demux buffer is empty
- BUG/MEDIUM: mux-h2: Only Report H2C error on read error if demux buffer is empty
- BUG/MEDIUM: mux-h2: Don't report error on SE if error is only pending on H2C
- BUG/MEDIUM: mux-h2: Don't report error on SE for closed H2 streams
- DOC: config: Update documentation about local haproxy response
- DEV: patchbot: use checked buttons as reference instead of internal table
- DEV: patchbot: allow to show/hide backported patches
- MINOR: h3: remove quic_conn only reference
- BUG/MINOR: server: Use the configured address family for the initial resolution
- MINOR: mux-quic: remove qcc_shutdown() from qcc_release()
- MINOR: mux-quic: use qcc_release in case of init failure
- MINOR: mux-quic: adjust error code in init failure
- MINOR: h3: add traces for connection init stage
- BUG/MINOR: h3: properly handle alloc failure on finalize
- MINOR: h3: use INTERNAL_ERROR code for init failure
- 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
- MEDIUM: proxy: set PR_O_HTTP_UPG on implicit upgrades
- MINOR: proxy: monitor-uri works with tcp->http upgrades
- OPTIM: server: eb lookup for server_find_by_name()
- OPTIM: server: ebtree lookups for findserver_unique_* functions
- 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
- MINOR: server: ensure connection cleanup on server addr changes
- CLEANUP: server/event_hdl: remove purge_conn hint in INETADDR event
- MEDIUM: server: merge srv_update_addr() and srv_update_addr_port() logic
- CLEANUP: server: remove unused server_parse_addr_change_request() function
- CLEANUP: resolvers: remove duplicate func prototype
- MINOR: resolvers: add unique numeric id to nameservers
- MEDIUM: server: make server_set_inetaddr() updater serializable
- MINOR: server/event_hdl: expose updater info through INETADDR event
- MINOR: server: add dns hint in server_inetaddr_updater struct
- MEDIUM: server/dns: clear RMAINT when addr resolves again
- BUG/MINOR: server/dns: use server_set_inetaddr() to unset srv addr from DNS
- BUG/MEDIUM: server/dns: perform svc_port updates atomically from SRV records
- MEDIUM: peers: use server as stream target
- CLEANUP: peers: remove unused sock_init_arg struct member
- CLEANUP: peers: remove unused "proto" and "xprt" struct members
- MINOR: peers: rely on srv->addr and remove peer->addr
- DOC: config: add context hint for server keywords
- MINOR: stktable: add table_process_entry helper function
- MINOR: stktable: use {show,set,clear} table with ptr
- MINOR: map: add map_*_key converters to provide the matching key
- DOC: fix typo for fastfwd QUIC option
- BUG/MINOR: mux-quic: always report error to SC on RESET_STREAM emission
- MEDIUM: mux-quic: add BUG_ON if sending on locally closed QCS
- 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
- BUG/MINOR: h3: close connection on sending alloc errors
- BUG/MINOR: h3: disable fast-forward on buffer alloc failure
- Revert "MINOR: mux-quic: Disable zero-copy forwarding for send by default"
- MINOR: stktable: stktable_data_ptr() cannot fail in table_process_entry()
- CLEANUP: assorted typo fixes in the code and comments
- CI: use semantic version compare for determing "latest" OpenSSL
- CLEANUP: server: remove ambiguous check in srv_update_addr_port()
- CLEANUP: resolvers: remove unused RSLV_UPD_OBSOLETE_IP flag
- CLEANUP: resolvers: remove some more unused RSLV_UDP flags
- MEDIUM: server: simplify snr_set_srv_down() to prevent confusions
- MINOR: backend: export get_server_*() functions
- MINOR: tcpcheck: export proxy_parse_tcpcheck()
- MEDIUM: udp: allow to retrieve the frontend destination address
- MINOR: global: export a way to list build options
- MINOR: debug: add features and build options to "show dev"
- BUG/MINOR: server: fix server_find_by_name() usage during parsing
- 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
- MINOR: h3: do not consider missing buf room as error on trailers
- 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
- CLEANUP: mux-h2: remove the printfs from previous commit on h2 streams limit.
- DEV: h2: add the ability to emit literals in mkhdr
- DEV: h2: add the preface as well in supported output types
- DEV: h2: support passing raw data for a frame
- IMPORT: ebtree: implement and use flsnz_long() to count bits
- IMPORT: ebtree: switch the sizes and offsets to size_t and ssize_t
- IMPORT: ebtree: rework the fls macros to better deal with arch-specific ones
- IMPORT: ebtree: make string_equal_bits turn back to unsigned char
- IMPORT: ebtree: use unsigned ints for flznz()
- IMPORT: ebtree: make string_equal_bits() return an unsigned
Willy Tarreau [Sat, 30 Dec 2023 16:24:06 +0000 (17:24 +0100)]
IMPORT: ebtree: make string_equal_bits() return an unsigned
It used to return ssize_t for -1 but in fact we're using this -1 as
the largest possible value and the result is generally cast to signed
to check if the end was reached, so better make it clearly return an
unsigned value here.
Willy Tarreau [Sat, 30 Dec 2023 15:28:05 +0000 (16:28 +0100)]
IMPORT: ebtree: make string_equal_bits turn back to unsigned char
With flsnz() instead of flsnz_long() we're now getting a better
performance on both x86 and ARM. The difference is that previously
we were relying on a function that was forcing the use of register
%eax for the 8-bit version and that was preventing the compiler
from keeping the code optimized. The gain is roughly 5% on ARM and
1% on x86.
Willy Tarreau [Sat, 30 Dec 2023 14:37:17 +0000 (15:37 +0100)]
IMPORT: ebtree: rework the fls macros to better deal with arch-specific ones
The definitions were a bit of a mess and there wasn't even a fall back to
__builtin_clz() on compilers supporting it. Now we instead define a macro
for each implementation that is set on an arch-dependent case by case,
and add the fall back ones only when not defined. This also allows the
flsnz8() to automatically fall back to the 32-bit arch-specific version
if available. This shows a consistent 33% speedup on arm for strings.
Willy Tarreau [Sun, 17 Dec 2023 15:43:25 +0000 (16:43 +0100)]
IMPORT: ebtree: implement and use flsnz_long() to count bits
The asm code shows multiple conversions. Gcc has always been terribly
bad at dealing with chars, which are constantly converted to ints for
every operation and zero-extended after each operation. But here in
addition there are conversions before and after the flsnz(). Let's
just mark the variables as long and use flsnz_long() to process them
without any conversion. This shortens the code and makes it slightly
faster.
Note that the fls operations could make use of __builtin_clz() on
gcc 4.6 and above, and it would be useful to implement native support
for ARM as well.
Willy Tarreau [Tue, 10 Oct 2023 15:09:38 +0000 (17:09 +0200)]
DEV: h2: support passing raw data for a frame
With -r it's possible to pass raw data that will be interpreted by
printf so it even supports \x sequences. E.g. for a RST_STREAM, let's
just use \x00\x00\x00\x00.
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).
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.
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.
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.
MINOR: h3: do not consider missing buf room as error on trailers
Improve h3_resp_trailers_send() return value to be similar with
h3_resp_data_send(). In particular, if QCS Tx buffer has not enough
space for trailer encoding, 0 is returned instead of an error value,
with QC_SF_BLK_MROOM set.
This unify HTTP/3 headers/data/trailers encoding functions. Negative
error codes are limited to fatal error which should cause a connection
closure. Not enough output buffer space is only a transient condition
which is reflect by the QC_SF_BLK_MROOM flag.
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.
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:
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: ‘enum ssl_encryption_level_t’
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.
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.
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.
BUG/MINOR: server: fix server_find_by_name() usage during parsing
Since below commit, server_find_by_name() now search using
'used_server_id' proxy backend tree : 4bcfe30414005323a8d4ab986bce92bb736b59df
OPTIM: server: eb lookup for server_find_by_name()
This introduces a regression if server_find_by_name() is used via
check_config_validity() during post-parsing. Indeed, used_server_id tree
is populated at the same stage so it's possible to not found an existing
server. This can cause incorrect rejection of previously valid
configuration file.
To fix this, servers are now inserted in used_server_id tree during
parsing via parse_server(). This guarantees that server instances can be
retrieved during post parsing.
A known feature which uses server_find_by_name() during post parsing is
attach-srv tcp-rule used for reverse HTTP. Prior to the current fix, a
config was wrongly rejected if the rule was declared before the server
line.
This should not be backported unless the mentionned commit is.
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.
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.
MEDIUM: server: simplify snr_set_srv_down() to prevent confusions
snr_set_srv_down() (was formely known as snr_update_srv_status()), is
still too ambiguous because it's not clear whether we will be putting
the server under maintenance or not. This is mainly due to the fact that
the function behaves differently if has_no_ip is set or not.
By reviewing the function callers, it has now become clear that
snr_resolution_cb() is always calling the function with a valid resolution
so we only want to put the server under maintenance if we don't have a
valid IP address. On the other hand snr_resolution_error_cb() always
calls the function on error, with either no resolution (for SRV requests)
or with failing resolution (all cases except RSLV_STATUS_VALID), so in
this case we decide whether to put the server under maintenance case by
case (ie: expired? timeout?)
As a result, let's simplify snr_set_srv_down() so that it is only called
when the caller really thinks that the server should be put under
maintenance, which means always for snr_resolution_error_cb(), and only
if the resolution didn't yield usable ip for snr_resolution_cb().
CLEANUP: resolvers: remove some more unused RSLV_UDP flags
RSLV_UPD_CNAME and RSLV_UPD_NAME_ERROR flags have now become useless since 3cf7f987 ("MINOR: dns: proper domain name validation when receiving DNS
response") as they are never set, but we forgot to remove them.
CLEANUP: resolvers: remove unused RSLV_UPD_OBSOLETE_IP flag
RSLV_UPD_OBSOLETE_IP was introduced with commit a8c6db8d2 ("MINOR: dns:
Cache previous DNS answers.") but the commit didn't make any use of it,
and today the flag is still unused. Since we have no valid use for it,
better remove it to prevent confusions.
CLEANUP: server: remove ambiguous check in srv_update_addr_port()
A leftover check was left by recent patch series about server
addr:svc_port propagation: a check on (msg) being set was performed
in srv_update_addr_port(), but msg is always set, so the check is not
needed and confuses coverity (See GH #2399)
MINOR: stktable: stktable_data_ptr() cannot fail in table_process_entry()
In table_process_entry(), stktable_data_ptr() result is dereferenced
without checking if it's NULL first, which may happen when bad inputs
are provided to the function.
However, data_type and ts arguments were already checked prior to calling
the function, so we know for sure that stktable_data_ptr() will never
return NULL in this case.
However some static code analyzers such as Coverity are being confused
because they think that the result might possibly be NULL.
(See GH #2398)
To make it explicit that we always provide good inputs and expect valid
result, let's switch to the __stktable_data_ptr() unsafe function.
Found issues related to QUIC fast-forward were resolved (see github
issue #2372). Reenable it by default. If any issue arises, it can be
disabled using the global statement :
tune.quit.zero-copy-fwd-send off
This can be backported to 2.9, but only after a sensible period of
observation.
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.
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.
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".
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().
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.
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.
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.
Amaury Denoyelle [Tue, 19 Dec 2023 10:25:18 +0000 (11:25 +0100)]
MEDIUM: mux-quic: add BUG_ON if sending on locally closed QCS
Previously, if snd_buf operation was conducted despite QCS already
locally closed, the input buffer was silently dropped. This situation
could happen if a RESET_STREAM was emitted butemission not reported to
the stream layer. Resetting silently the buffer ensure QUIC MUX remain
compliant with RFC 9000 which forbid emission after RESET_STREAM.
Since previous commit, it is now ensured that RESET_STREAM sending will
always be reported to stream-layer. Thus, there is no need anymore to
silently reset the buffer. A BUG_ON() statement is added to ensure this
assumption will remain valid.
The new code is deemed cleaner as it does not hide a missing error
notification on the stconn-layer. Previously, if an error was missing,
sending would continue unnecessarily with a false success status
reported for the stream.
Note that the BUG_ON() statement was also added into nego_ff callback.
This is necessary to ensure both sending path remains consistent.
This patch is labelled as MEDIUM as issues were already encountered in
snd_buf/nego_ff implementation and it's not easy to cover all occurences
during test. If the BUG_ON() is triggered without any apparent
stream-layer issue, this commit should be reverted.
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 :
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.
MINOR: map: add map_*_key converters to provide the matching key
All map_*_ converters now have an additional output type: key. Such
converters will return the matched entry's key (as found in the map file)
as a string instead of the value.
Consider this example map file:
|example.com value1
|haproxy value2
With the above map file:
str(test.example.com/url),map_dom_key(file.map) will return "example.com"
str(running haproxy),map_sub_key(file.map) will return "haproxy"
MINOR: stktable: use {show,set,clear} table with ptr
This patchs adds support for optional ptr (0xffff form) instead of key
argument to match against existing sticktable entries, ie: if the key is
empty or cannot be matched on the cli due to incompatible characters.
Lookup is performed using a linear search so it will be slower than key
search which relies on eb tree lookup.
Example:
set table mytable key mykey data.gpc0 1
show table mytable
> 0x7fbd00032bd8: key=mykey use=0 exp=86373242 shard=0 gpc0=1
clear table mytable ptr 0x7fbd00032bd8
This patchs depends on:
- "MINOR: stktable: add table_process_entry helper function"
MINOR: stktable: add table_process_entry helper function
Only keep key-related logic in table_process_entry_per_key() function,
and then use table_process_entry() function that takes an entry pointer
as argument to process the entry.
MINOR: peers: rely on srv->addr and remove peer->addr
Similarly to the previous commit, we get rid of unused peer member.
peer->addr was only used to save a copy of the sever's addr at parsing
time. But instead of relying on an intermediate variable, we can actually
use server's address directly when initiating the peer session.
As with other streams created from server's settings (tcp/http, log, ring),
we should rely on srv->svc_port for the port part of the address. This
shouldn't change anything for peers since the address is fully resolved
at parsing time and runtime changes are not supported, but this should
help to make the code future-proof.
CLEANUP: peers: remove unused "proto" and "xprt" struct members
peer->proto and peer->xprt struct members are now pure legacy: they are
only set during parsing but never used afterwards.
This is due to commit 02efedac ("MINOR: peers: now remove the remote
connection setup code") which made some cleanup in the past, but the
unused proto and xprt members were probably left unused by mistake.
Since we don't have valid uses for them, we remove them.
Also, peer_xprt() helper function was removed since it was related to
peer->xprt struct member.
Historically, we used the internal peer proxy as stream target, because
then we only cared about initiating a basic tcp connection with the
endpoint, and relying on parent proxy settings was enough.
But later, we introduced the possibility to connect to an SSL peer by
taking server's SSL parameters into acount. This was done in commit 1055e687 ("MINOR: peers: Make outgoing connection to SSL/TLS peers work.")
However, the above commit introduced an ambiguity:
peer_session_target() function was introduced, and the function will
either return the peers proxy's object or the current server's object
depending if ssl is configured or not.
While this works fine to ensure proper SSL handling while being
conservative with historical behavior, this cause other server transport
related settings to only work when ssl settings are provided, which is
quite debatable.
Indeed, while we're there, why not always using the server's object as
a stream target, to ensure all transport related options are properly
handled? Moreover, the peers documentation tells this:
... "support for all "server" parameters found in 5.2 paragraph that
are related to transport settings" ...
To remove the ambiguity and fully comply with the documentation, we make
peer_session_target() always return the server's object.
BUG/MEDIUM: server/dns: perform svc_port updates atomically from SRV records
This was the last missing bit from cd994407a ("BUG/MAJOR: server/addr:
fix a race during server addr:svc_port updates")
Indeed, despite the fix, svc_port updates from resolvers were still
directly performed on the server's struct.
Now they make proper use of the server_set_inetaddr() function so the port
change (+ optional addr change with AR) will be propagated atomically.
This patch depends on:
- "MINOR: server: ensure connection cleanup on server addr changes"
- "CLEANUP: server/event_hdl: remove purge_conn hint in INETADDR event"
- "MEDIUM: server: merge srv_update_addr() and srv_update_addr_port() logic"
- "MEDIUM: server: make server_set_inetaddr() updater serializable"
- "MINOR: server/event_hdl: expose updater info through INETADDR event"
- "MINOR: server: add dns hint in server_inetaddr_updater struct"
- "MEDIUM: server/dns: clear RMAINT when addr resolves again"
While it could be backported in 2.9 with cd994407a ("BUG/MAJOR:
server/addr: fix a race during server addr:svc_port updates") to ensure
addr and svc_port updates performed by resolver's code comply with the
API taking care of pushing the update (and thus avoid any race), some
patch dependencies are quite sensitive so it's probably best to avoid
backporting for no good reason, or at least wait for it to be considered
stable to prevent any breakeages
BUG/MINOR: server/dns: use server_set_inetaddr() to unset srv addr from DNS
As seen before, server's addr and svc_port should not be updated directly
during runtime, because even if the update is performed under the lock,
some competing threads might be reading ->addr and ->svc_port without
the lock because they simply cannot afford it.
To prevent races with such competing threads, server's addr and port
should only be updated using server_set_inetaddr() function or similar.
This patch depends on:
- "MINOR: server: ensure connection cleanup on server addr changes"
- "CLEANUP: server/event_hdl: remove purge_conn hint in INETADDR event"
- "MEDIUM: server: merge srv_update_addr() and srv_update_addr_port() logic"
- "MEDIUM: server: make server_set_inetaddr() updater serializable"
- "MINOR: server/event_hdl: expose updater info through INETADDR event"
- "MINOR: server: add dns hint in server_inetaddr_updater struct"
- "MEDIUM: server/dns: clear RMAINT when addr resolves again"
While it could be backported in 2.9 with cd994407a ("BUG/MAJOR:
server/addr: fix a race during server addr:svc_port updates") to ensure
addr and svc_port reset performed by resolver's code comply with the
API taking care of pushing the update (and thus avoid any race), some
patch dependencies are quite sensitive so it's probably best to avoid
backporting for no good reason, or at least wait for it to be considered
stable to prevent any breakeages.