MAJOR: mux-quic: implement a simplified mux version
Re-implement the QUIC mux. It will reuse the mechanics from the previous
mux without all untested/unsupported features. This should ease the
maintenance.
Note that a lot of features are broken for the moment. They will be
re-implemented on the following commits to have a clean commit history.
Add BUG_ON statement when handling a non implemented frames on the
control stream. This is required because frames must be removed from the
RX buffer or else it will stall the buffer.
MINOR: quic: fix segfault on CONNECTION_CLOSE parsing
At the moment the reason_phrase member of a
quic_connection_close/quic_connection_close_app structure is not
allocated. Comment the memcpy to it to avoid segfault.
Willy Tarreau [Fri, 3 Dec 2021 16:38:42 +0000 (17:38 +0100)]
IMPORT: slz: use the correct CRC32 instruction when running in 32-bit mode
Many ARMv8 processors also support Aarch32 and can run armv7 and even
thumb2 code. While armv8 compilers will not emit these instructions,
armv7 compilers that are aware of these processors will do. For
example, using gcc built for an armv7 target and passing it
"-mcpu=cortex-a72" or "-march=armv8-a+crc" will result in the CRC32
instruction to be used.
In this case the current assembly code fails because with the ARM and
Thumb2 instruction sets there is no such "%wX" half-registers. We need
to use "%X" instead as the native 32-bit register when running with a
32-bit instruction set, and use "%wX" when using the 64-bit instruction
set (A64).
Willy Tarreau [Mon, 6 Dec 2021 07:01:02 +0000 (07:01 +0000)]
BUILD: tree-wide: avoid warnings caused by redundant checks of obj_types
At many places we use construct such as:
if (objt_server(blah))
do_something(objt_server(blah));
At -O2 the compiler manages to simplify the operation and see that the
second one returns the same result as the first one. But at -O1 that's
not always the case, and the compiler is able to emit a second
expression and sees the potential null that results from it, and may
warn about a potential null deref (e.g. with gcc-6.5). There are two
solutions to this:
- either the result of the first test has to be passed to a local
variable
- or the second reference ought to be unchecked using the __objt_*
variant.
This patch fixes all occurrences at once by taking the second approach
(the least intrusive). For constructs like:
objt_server(blah) ? objt_server(blah)->name : "no name"
a macro could be useful. It would for example take the object type
(server), the field name (name) and the default value. But there
are probably not enough occurrences across the whole code for this
to really matter.
DOC: spoe: Clarify use of the event directive in spoe-message section
Only one event is possible for a spoe-message section. If defined several
time, only the last one is considered. The documentation is now explicit on
this point.
Tim Duesterhus [Wed, 1 Dec 2021 22:04:15 +0000 (23:04 +0100)]
BUG/MEDIUM: sample: Fix memory leak in sample_conv_jwt_member_query
The function leaked one full buffer per invocation. Fix this by simply removing
the call to alloc_trash_chunk(), the static chunk from get_trash_chunk() is
sufficient.
BUILD: bug: Fix error when compiling with -DDEBUG_STRICT_NOCRASH
ha_backtrace_to_stderr() must be declared in CRASH_NOW() macro whe HAProxy
is compiled with DEBUG_STRICT_NOCRASH. Otherwise an error is reported during
compilation:
include/haproxy/bug.h:58:26: error: implicit declaration of function ‘ha_backtrace_to_stderr’ [-Werror=implicit-function-declaration]
58 | #define CRASH_NOW() do { ha_backtrace_to_stderr(); } while (0)
BUG/MINOR: resolvers: Don't overwrite the error for invalid query domain name
When a response is validated, the query domain name is checked to be sure it
is the same than the one requested. When an error is reported, the wrong
goto label was used. Thus, the error was lost. Instead of
RSLV_RESP_WRONG_NAME, RSLV_RESP_INVALID was reported.
This bug was introduced by the commit c1699f8c1 ("MEDIUM: resolvers: No
longer store query items in a list into the response").
This patch should fix the issue #1473. No backport is needed.
BUG/MEDIUM: h1: Properly reset h1m flags when headers parsing is restarted
If H1 headers are not fully received at once, the parsing is restarted a
last time when all headers are finally received. When this happens, the h1m
flags are sanitized to remove all value set during parsing.
But some flags where erroneously preserved. Among others, H1_MF_TE_CHUNKED
flag was not removed, what could lead to parsing error.
To fix the bug and make things easy, a mask has been added with all flags
that must be preserved. It will be more stable. This mask is used to
sanitize h1m flags.
This patch should fix the issue #1469. It must be backported to 2.5.
Emeric Brun [Wed, 1 Dec 2021 11:08:42 +0000 (12:08 +0100)]
BUG/MAJOR: segfault using multiple log forward sections.
For each new log forward section, the proxy was added to the log forward
proxy list but the ref on the previous log forward section's proxy was
scratched using "init_new_proxy" which performs a memset. After configuration
parsing this list contains only the last section's proxy.
The post processing walk through this list to resolve "ring" names.
Since some section's proxies are missing in this list, the resolving
is not done for those ones and the pointer on the ring is kept to null
causing a segfault at runtime trying to write a log message
into the ring.
This patch shift the "init_new_proxy" before adding the ref on the
previous log forward section's proxy on currently parsed one.
MEDIUM: resolvers: No longer store query items in a list into the response
When the response is parsed, query items are stored in a list, attached to
the parsed response (resolve_response).
First, there is one and only one query sent at a time. Thus, there is no
reason to use a list. There is a test to be sure there is only one query
item in the response. Then, the reference on this query item is only used to
validate the domain name is the one requested. So the query list can be
removed. We only expect one query item, no reason to loop on query records.
In addition, the query domain name is now immediately checked against the
resolution domain name. This way, the query item is only manipulated during
the response parsing.
BUG/MEDIUM: resolvers: Detach query item on response error
When a new response is parsed, it is unexpected to have an old query item
still attached to the resolution. And indeed, when the response is parsed
and validated, the query item is detached and used for a last check on its
dname. However, this is only true for a valid response. If an error is
detected, the query is not detached. This leads to undefined behavior (most
probably a crash) on the next response because the first element in the
query list is referencing an old response.
BUG/MINOR: server: Don't rely on last default-server to init server SSL context
During post-parsing stage, the SSL context of a server is initialized if SSL
is configured on the server or its default-server. It is required to be able
to enable SSL at runtime. However a regression was introduced, because the
last parsed default-server is used. But it is not necessarily the
default-server line used to configure the server. This may lead to
erroneously initialize the SSL context for a server without SSL parameter or
the skip it while it should be done.
The problem is the default-server used to configure a server is not saved
during configuration parsing. So, the information is lost during the
post-parsing. To fix the bug, the SRV_F_DEFSRV_USE_SSL flag is
introduced. It is used to know when a server was initialized with a
default-server using SSL.
For the record, the commit f63704488e ("MEDIUM: cli/ssl: configure ssl on
server at runtime") has introduced the bug.
Add pointer to counters as a member for h1c structure. This pointer is
initialized on h1_init function. This is useful to quickly access and
manipulate the counters inside every h1 functions.
BUG/MINOR: mux-h1: Fix splicing for messages with unknown length
Splicing was disabled fo Messages with an unknown length (no C-L or T-E
header) with no valid reason. So now, it is possible to use the kernel
splicing for such messages.
BUG/MEDIUM: mux-h1: Fix splicing by properly detecting end of message
Since the 2.4.4, the splicing support in the H1 multiplexer is buggy because
end of the message is not properly detected.
On the 2.4, when the requests is spliced, there is no issue. But when the
response is spliced, the client connection is always closed at the end of the
message. Note the response is still fully sent.
On the 2.5 and higher, when the last requests on a connection is spliced, a
client abort is reported. For other requests there is no issue. In all cases,
the requests are fully sent. When the response is spliced, the server connection
hangs till the server timeout and a server abort is reported. The response is
fully sent with no delay.
The root cause is the EOM block suppression. There is no longer extra block to
be sure to call a last time rcv_buf()/snd_buf() callback functions. At the end,
to fix the issue, we must now detect end of the message in rcv_pipe() and
snd_pipe() callback functions. To do so, we rely on the announced message length
to know when the payload is finished. This works because the chunk-encoded
messages are not spliced.
This patch must be backported as far as 2.4 after an observation period.
David CARLIER [Fri, 26 Nov 2021 20:44:44 +0000 (20:44 +0000)]
MEDIUM: pool: Following up on previous pool trimming update.
Apple libmalloc has its own notion of memory arenas as malloc_zone with
rich API having various callbacks for various allocations strategies but
here we just use the defaults.
In trim_all_pools, we advise to purge each zone as much as possible, called "greedy" mode.
BUG/MINOR: vars: Fix the set-var and unset-var converters
In commit 3a4bedccc6 the variable logic was changed. Instead of
accessing variables by their name during runtime, the variable tables
are now indexed by a hash of the name. But the set-var and unset-var
converters try to access the correct variable by calculating a hash on
the sample instead of the already calculated variable hash.
As soon as the connection ID (the one choosen by the QUIC server) has been used
by the client, we can delete its original destination connection ID from its tree.
This patch modifies ha_quic_set_encryption_secrets() to store the
secrets received by the TLS stack and prepare the information for the
next key update thanks to quic_tls_key_update().
qc_pkt_decrypt() is modified to check if we must used the next or the
previous key phase information to decrypt a short packet.
The information are rotated if the packet could be decrypted with the
next key phase information. Then new secrets, keys and IVs are updated
calling quic_tls_key_update() to prepare the next key phase.
quic_build_packet_short_header() is also modified to handle the key phase
bit from the current key phase information.
MINOR: quic: Add quic_tls_key_update() function for Key Update
This function derives the next RX and TX keys and IVs from secrets
for the next key update key phase. We also implement quic_tls_rotate_keys()
which rotate the key update key phase information to be able to continue
to decrypt old key phase packets. Most of these information are pointers
to unsigned char.
MINOR: quic: Optional header protection key for quic_tls_derive_keys()
quic_tls_derive_keys() is responsible to derive the AEAD keys, IVs and$
header protection key from a secret provided by the TLS stack. We want
to make the derivation of the header protection key be optional. This
is required for the Key Update process where there is no update for
the header protection key.
MINOR: quic: Add structures to maintain key phase information
When running Key Update process, we must maintain much information
especially when the key phase bit has been toggled by the peer as
it is possible that it is due to late packets. This patch adds
quic_tls_kp new structure to do so. They are used to store
previous and next secrets, keys and IVs associated to the previous
and next RX key phase. We also need the next TX key phase information
to be able to encrypt packets for the next key phase.
MINOR: quic: Possible crash when inspecting the xprt context
haproxy may crash when running this statement in qc_lstnr_pkt_rcv():
conn_ctx = qc->conn->xprt_ctx;
because qc->conn may not be initialized. With this patch we ensure
qc->conn is correctly initialized before accessing its ->xprt_ctx
members. We zero the xrpt_ctx structure (ssl_conn_ctx struct), then
initialize its ->conn member with HA_ATOMIC_STORE. Then, ->conn and
->conn->xptr_ctx members of quic_conn struct can be accessed with HA_ATOMIC_LOAD()
MINOR: quic: Immediately close if no transport parameters extension found
If the ClientHello callback does not manage to find a correct QUIC transport
parameters extension, we immediately close the connection with
missing_extension(109) as TLS alert which is turned into 0x16d QUIC connection
error.
When sending a CONNECTION_CLOSE frame to immediately close the connection,
do not provide CRYPTO data to the TLS stack. Do not built anything else than a
CONNECTION_CLOSE and do not derive any secret when in immediately close state.
Seize the opportunity of this patch to rename ->err quic_conn struct member
to ->error_code.
We set this TLS error when no application protocol could be negotiated
via the TLS callback concerned. It is converted as a QUIC CRYPTO_ERROR
error (0x178).
Willy Tarreau [Tue, 30 Nov 2021 08:32:21 +0000 (09:32 +0100)]
BUILD: evports: remove a leftover from the dead_fd cleanup
Commit b1f29bc62 ("MINOR: activity/fd: remove the dead_fd counter") got
rid of FD_UPDT_DEAD, but evports managed to slip through the cracks and
wasn't cleaned up, thus it doesn't build anymore, as reported in github
issue #1467. We just need to remove the related lines since the situation
is already handled by the remaining conditions.
Thanks to Dominik Hassler for reporting the issue and confirming the fix.
BUG/MEDIUM: cli: Properly set stream analyzers to process one command at a time
The proxy used by the master CLI is an internal proxy and no filter are
registered on it. Thus, there is no reason to take care to set or unset
filter analyzers in the master CLI analyzers. AN_REQ_FLT_END was set on the
request channel to prevent the infinite forward and be sure to be able to
process one commande at a time. However, the only work because
CF_FLT_ANALYZE flag was used by error as a channel analyzer instead of a
channel flag. This erroneously set AN_RES_FLT_END on the request channel,
that really prevent the infinite forward, be side effet.
In fact, We must avoid this kind of trick because this only work by chance
and may be source of bugs in future. Instead, we must always keep the CLI
request analyzer and add an early return if the response is not fully
processed. It happens when the CLI response analyzer is set.
Willy Tarreau [Fri, 26 Nov 2021 14:45:41 +0000 (15:45 +0100)]
CI: github actions: add the output of $CC -dM -E-
Sometimes figuring what differs between platforms is useful to fix
build issues, to decide what ifdef to add for example. Let's always
call $CC -dM -E- before starting make.
Willy Tarreau [Fri, 26 Nov 2021 14:55:55 +0000 (15:55 +0100)]
BUILD: pools: only detect link-time jemalloc on ELF platforms
The build broke on Windows and MacOS after commit ed232148a ("MEDIUM:
pool: refactor malloc_trim/glibc and jemalloc api addition detections."),
because the extern+attribute(weak) combination doesn't result in a really
weak symbol and it causes an undefined symbol at link time.
Let's reserve this detection to ELF platforms. The runtime detection using
dladdr() remains used if defined.
BUG/MINOR: mworker: deinit of thread poller was called when not initialized
Commit 67e371e ("BUG/MEDIUM: mworker: FD leak of the eventpoll in wait
mode") introduced a regression. Upon a reload it tries to deinit the
poller per thread, but no poll loop was initialized after loading the
configuration.
This patch fixes the issue by moving this part of the code in
mworker_reload(), since this function will be called only when the
poller is fully initialized.
Amaury Denoyelle [Thu, 25 Nov 2021 17:06:56 +0000 (18:06 +0100)]
MINOR: quic: use more verbose QUIC traces set at compile-time
Remove the verbosity set to 0 on quic_init_stdout_traces. This will
generate even more verbose traces on stdout with the default verbosity
of 1 when compiling with -DENABLE_QUIC_STDOUT_TRACES.
Amaury Denoyelle [Thu, 25 Nov 2021 15:05:16 +0000 (16:05 +0100)]
MINOR: quic: activate QUIC traces at compilation
Implement a function quic_init_stdout_traces called at STG_INIT. If
ENABLE_QUIC_STDOUT_TRACES preprocessor define is set, the QUIC trace
module will be automatically activated to emit traces on stdout on the
developer level.
The main purpose for now is to be able to generate traces on the haproxy
docker image used for QUIC interop testing suite. This should facilitate
test failure analysis.
Amaury Denoyelle [Wed, 24 Nov 2021 14:32:46 +0000 (15:32 +0100)]
MEDIUM: quic: handle CIDs to rattach received packets to connection
Change the way the CIDs are organized to rattach received packets DCID
to QUIC connection. This is necessary to be able to handle multiple DCID
to one connection.
For this, the quic_connection_id structure has been extended. When
allocated, they are inserted in the receiver CID tree instead of the
quic_conn directly. When receiving a packet, the receiver tree is
inspected to retrieve the quic_connection_id. The quic_connection_id
contains now contains a reference to the QUIC connection.
Amaury Denoyelle [Wed, 24 Nov 2021 14:16:08 +0000 (15:16 +0100)]
REORG: quic: add comment on rare thread concurrence during CID alloc
The comment is here to warn about a possible thread concurrence issue
when treating INITIAL packets from the same client. The macro unlikely
is added to further highlight this scarce occurence.
BUG/MEDIUM: mworker: FD leak of the eventpoll in wait mode
Since 2.5, before re-executing in wait mode, the master can have a
working configuration loaded, with a eventpoll fd. This case was not
handled correctly and a new eventpoll FD is leaking in the master at
each reload, which is inherited by the new worker.
BUG/MINOR: mworker: does not add the -sf in wait mode
Since the wait mode is automatically executed after charging the
configuration, -sf was shown in argv[] with the previous PID, which is
normal, but also the current one. This is only a visual problem when
listing the processes, because -sf does not do anything in wait mode.
Fix the issue by removing the whole "-sf" part in wait mode, but the
executed command can be seen in the argv[] of the latest worker forked.
Bertrand Jacquin [Wed, 24 Nov 2021 21:16:06 +0000 (21:16 +0000)]
BUG/MINOR: lua: remove loop initial declarations
HAProxy is documented to support gcc >= 3.4 as per INSTALL file, however
hlua.c makes use of c11 only loop initial declarations leading to build
failure when using gcc-4.9.4:
x86_64-unknown-linux-gnu-gcc -Iinclude -Wchar-subscripts -Wcomment -Wformat -Winit-self -Wmain -Wmissing-braces -Wno-pragmas -Wparentheses -Wreturn-type -Wsequence-point -Wstrict-aliasing -Wswitch -Wtrigraphs -Wuninitialized -Wunknown-pragmas -Wunused-label -Wunused-variable -Wunused-value -Wpointer-sign -Wimplicit -pthread -fdiagnostics-color=auto -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -O3 -msse -mfpmath=sse -march=core2 -g -fPIC -g -Wall -Wextra -Wundef -Wdeclaration-after-statement -fwrapv -Wno-unused-label -Wno-sign-compare -Wno-unused-parameter -Wno-clobbered -Wno-missing-field-initializers -Wtype-limits -DUSE_EPOLL -DUSE_NETFILTER -DUSE_PCRE2 -DUSE_PCRE2_JIT -DUSE_POLL -DUSE_THREAD -DUSE_BACKTRACE -DUSE_TPROXY -DUSE_LINUX_TPROXY -DUSE_LINUX_SPLICE -DUSE_LIBCRYPT -DUSE_CRYPT_H -DUSE_GETADDRINFO -DUSE_OPENSSL -DUSE_LUA -DUSE_ACCEPT4 -DUSE_SLZ -DUSE_CPU_AFFINITY -DUSE_TFO -DUSE_NS -DUSE_DL -DUSE_RT -DUSE_PRCTL -DUSE_THREAD_DUMP -DUSE_PCRE2 -DPCRE2_CODE_UNIT_WIDTH=8 -I/usr/local/include -DCONFIG_HAPROXY_VERSION=\"2.5.0\" -DCONFIG_HAPROXY_DATE=\"2021/11/23\" -c -o src/connection.o src/connection.c
src/hlua.c: In function 'hlua_config_prepend_path':
src/hlua.c:11292:2: error: 'for' loop initial declarations are only allowed in C99 or C11 mode
for (size_t i = 0; i < 2; i++) {
^
src/hlua.c:11292:2: note: use option -std=c99, -std=gnu99, -std=c11 or -std=gnu11 to compile your code
This commit moves loop iterator to an explicit declaration.
Must be backported to 2.5 because this issue was introduced in
v2.5-dev10~69 with commit 9e5e586e35c5 ("BUG/MINOR: lua: Fix lua error
handling in `hlua_config_prepend_path()`")
MEDIUM: mworker: seamless reload use the internal sockpairs
With the master worker, the seamless reload was still requiring an
external stats socket to the previous process, which is a pain to
configure.
This patch implements a way to use the internal socketpair between the
master and the workers to transfer the sockets during the reload.
This way, the master will always try to transfer the socket, even
without any configuration.
The master will still reload with the -x argument, followed by the
sockpair@ syntax. ( ex -x sockpair@4 ). Which use the FD of internal CLI
to the worker.
BUG/MINOR: cache: Fix loop on cache entries in "show cache"
A regression was introduced in the commit da91842b6 ("BUG/MEDIUM: cache/cli:
make "show cache" thread-safe"). When cli_io_handler_show_cache() is called,
only one node is retrieved and is used to fill the output buffer in loop.
Once set, the "node" variable is never renewed. At the end, all nodes are
dumped but each one is duplicated several time into the output buffer.
This patch must be backported everywhere the above commit is. It means only
to 2.5 and 2.4.
Willy Tarreau [Tue, 23 Nov 2021 14:40:21 +0000 (15:40 +0100)]
[RELEASE] Released version 2.5.0
Released version 2.5.0 with the following main changes :
- BUILD: SSL: add quictls build to scripts/build-ssl.sh
- BUILD: SSL: add QUICTLS to build matrix
- CLEANUP: sock: Wrap `accept4_broken = 1` into additional parenthesis
- BUILD: cli: clear a maybe-unused warning on some older compilers
- BUG/MEDIUM: cli: make sure we can report a warning from a bind keyword
- BUG/MINOR: ssl: make SSL counters atomic
- CLEANUP: assorted typo fixes in the code and comments
- BUG/MINOR: ssl: free correctly the sni in the backend SSL cache
- MINOR: version: mention that it's stable now
Willy Tarreau [Mon, 22 Nov 2021 16:46:13 +0000 (17:46 +0100)]
BUG/MINOR: ssl: make SSL counters atomic
SSL counters were added with commit d0447a7c3 ("MINOR: ssl: add counters
for ssl sessions") in 2.4, but their updates were not atomic, so it's
likely that under significant loads they are not correct.
Willy Tarreau [Sat, 20 Nov 2021 19:10:41 +0000 (20:10 +0100)]
BUG/MEDIUM: cli: make sure we can report a warning from a bind keyword
Since recent 2.5 commit c8cac04bd ("MEDIUM: listener: deprecate "process"
in favor of "thread" on bind lines"), the "process" bind keyword may
report a warning. However some parts like the "stats socket" parser
will call such bind keywords and do not expect to face warnings, so
this will instantly cause a fatal error to be reported. A concrete
effect is that "stats socket ... process 1" will hard-fail indicating
the keyword is deprecated and will be removed in 2.7.
We must relax this test, but the code isn't designed to report warnings,
it uses a single string and only supports reporting an error code (-1).
This patch makes a special case of the ERR_WARN code and uses ha_warning()
to report it, and keeps the rest of the existing error code for other
non-warning codes. Now "process" on the "stats socket" is properly
reported as a warning.
Willy Tarreau [Sat, 20 Nov 2021 18:17:38 +0000 (19:17 +0100)]
BUILD: cli: clear a maybe-unused warning on some older compilers
The SHOW_TOT() and SHOW_AVG() macros used in cli_io_handler_show_activity()
produce a warning on gcc 4.7 on MIPS with threads disabled because the
compiler doesn't know that global.nbthread is necessarily non-null, hence
that at least one iteration is performed. Let's just change the loop for
a do {} while () that lets the compiler know it's always initialized. It
also has the tiny benefit of making the code shorter.
Willy Tarreau [Fri, 19 Nov 2021 18:30:04 +0000 (19:30 +0100)]
[RELEASE] Released version 2.5-dev15
Released version 2.5-dev15 with the following main changes :
- BUG/MINOR: stick-table/cli: Check for invalid ipv6 key
- CLEANUP: peers: Remove useless test on peer variable in peer_trace()
- DOC: log: Add comments to specify when session's listener is defined or not
- BUG/MEDIUM: mux-h1: Handle delayed silent shut in h1_process() to release H1C
- REGTESTS: ssl_crt-list_filters: feature cmd incorrectly set
- DOC: internals: document the list API
- BUG/MINOR: h3: ignore unknown frame types
- MINOR: quic: redirect app_ops snd_buf through mux
- MEDIUM: quic: inspect ALPN to install app_ops
- MINOR: quic: support hq-interop
- MEDIUM: quic: send version negotiation packet on unknown version
- BUG/MEDIUM: mworker: cleanup the listeners when reexecuting
- DOC: internals: document the scheduler API
- BUG/MINOR: quic: fix version negotiation packet generation
- CLEANUP: ssl: fix wrong #else commentary
- MINOR: config: support default values for environment variables
- SCRIPTS: run-regtests: reduce the number of processes needed to check options
- SCRIPT: run-regtests: avoid several calls to grep to test for features
- SCRIPT: run-regtests: avoid calling awk to compute the version
- REGTEST: set retries count to zero for all tests that expect at 503
- REGTESTS: make tcp-check_min-recv fail fast
- REGTESTS: extend the default I/O timeouts and make them overridable
- BUG/MEDIUM: ssl: backend TLS resumption with sni and TLSv1.3
- BUG/MEDIUM: ssl: abort with the correct SSL error when SNI not found
- REGTESTS: ssl: test the TLS resumption
- BUILD: makefile: stop opening sub-shells for each and every command
- BUILD: makefile: reorder objects by build time
- BUG/MEDIUM: mux-h2: always process a pending shut read
- MINOR: quic_sock: missing CO_FL_ADDR_TO_SET flag
- MINOR: quic: Possible wrong connection identification
- MINOR: quic: Correctly pad UDP datagrams
- MINOR: quic: Support transport parameters draft TLS extension
- MINOR: quic: Anti-amplification implementation
- MINOR: quic: Wrong Initial packet connection initialization
- MINOR: quic: Wrong ACK range building
- MINOR: quic: Update some QUIC protocol errors
- MINOR: quic: Send CONNECTION_CLOSE frame upon TLS alert
- MINOR: quic: Wrong largest acked packet number parsing
- MINOR: quic: Add minimalistic support for stream flow control frames
- MINOR: quic: Wrong value for version negotiation packet 'Unused' field
- MINOR: quic: Support draft-29 QUIC version
- BUG/MINOR: quic: fix segfault on trace for version negotiation
- BUG/MINOR: hq-interop: fix potential NULL dereference
- BUILD: quic: fix potential NULL dereference on xprt_quic
- DOC: lua: documentation about the httpclient API
- BUG/MEDIUM: cache/cli: make "show cache" thread-safe
- BUG/MEDIUM: shctx: leave the block allocator when enough blocks are found
- BUG/MINOR: shctx: do not look for available blocks when the first one is enough
- MINOR: shctx: add a few BUG_ON() for consistency checks
Willy Tarreau [Fri, 19 Nov 2021 16:47:18 +0000 (17:47 +0100)]
MINOR: shctx: add a few BUG_ON() for consistency checks
The shctx code relies on sensitive conditions that are hard to infer
from the code itself, let's add some BUG_ON() to verify them. They
helped spot the previous bugs.
Willy Tarreau [Fri, 19 Nov 2021 16:42:49 +0000 (17:42 +0100)]
BUG/MINOR: shctx: do not look for available blocks when the first one is enough
In shctx_row_reserve_hot() we only leave if we've found the exact
requested size instead of at least as large, as is documented. This
results in extra lookups and free calls in the avail loop while it is
not needed, and participates to seeing a negative data_len early as
spotted in previous bugs.
It doesn't seem to have any other impact however, but it's better to
backport it to stable branches.
Willy Tarreau [Fri, 19 Nov 2021 16:29:23 +0000 (17:29 +0100)]
BUG/MEDIUM: shctx: leave the block allocator when enough blocks are found
In shctx_row_reserve_hot(), a missing break allows the avail loop to
loop for a while after having allocated the required blocks, possibly
leading to the point where it could trigger the watchdog after checking
up to 2 million blocks. In addition, the extra iteration may leave one
block assigned with size zero at the head of the avail list, and mark
it as being an isolated chain of 1 block. It's unclear whether this
could have had other consequences.
There is a non-negligible chance that it addreses bugs #1451 and #1284,
as the pattern observed in the loop looks exactly the same as the one
reported there in the crashes.
It's only marked medium because it is extremely hard to trigger. Here
the conditions were reproduced when starting 4k connections at once
requesting objects of random sizes between 0 and 20k to store them into
a small 1MB cache. However the watchdog will never trigger in such a case
so one needs to instrument the functions.
Thanks to Sohaib Ahmad and @g0uZ for providing useful traces.
This will need to be backported to all stable branches.
Willy Tarreau [Fri, 19 Nov 2021 16:25:41 +0000 (17:25 +0100)]
BUG/MEDIUM: cache/cli: make "show cache" thread-safe
The "show cache" command restarts from the previous node to look for a
duplicate key, but does this after having released the lock, so under
high write load, the node has many chances of having been reassigned
and the dereference of the node crashes after a few iterations. Since
the keys are unique anyway, there's no point looking for a dup, so
let's just continue from the next value.
This is only marked as medium as it seems to have been there for a
while, and discovering it that late simply means that nobody uses that
command, thus in practice it has a very limited impact on real users.
Amaury Denoyelle [Thu, 18 Nov 2021 13:38:00 +0000 (14:38 +0100)]
BUG/MINOR: quic: fix segfault on trace for version negotiation
When receiving Initial packets for Version Negotiation, no quic_conn is
instantiated. Thus, on the final trace, the quic_conn dereferencement
must be tested before using it.
MINOR: quic: Add minimalistic support for stream flow control frames
This simple patch add the parsing support for theses frames. But nothing is
done at this time about the streams or flow control concerned. This is only to
prevent some QUIC tracker or interop runner tests from failing for a reason
independant of their tested features.
MINOR: quic: Wrong largest acked packet number parsing
When we have already received ACK frames with the same largest packet
number, this is not an error at all. In this case, we must continue
to parse the ACK current frame.
MINOR: quic: Send CONNECTION_CLOSE frame upon TLS alert
Add ->err member to quic_conn struct to store the connection errors.
This is the responsability of ->send_alert callback of SSL_QUIC_METHOD
struct to handle the TLS alert and consequently update ->err value.
At this time, when entering qc_build_pkt() we build a CONNECTION_CLOSE
frame close the connection when ->err value is not null.
When adding a range, if no "lower" range was present in the ack range root for
the packet number space concerned, we did not check if the new added range could
overlap the next one. This leaded haproxy to crash when encoding negative integer
when building ACK frames.
This bug was revealed thanks to "multi_packet_client_hello" QUIC tracker
test which makes a client send two first Initial packets out of order.
->qc (QUIC connection) member of packet structure were badly initialized
when received as second Initial packet (from picoquic -Q for instance).
This leaded to corrupt the quic_conn structure with random behaviors
as size effects. This bug came with this commit:
"MINOR: quic: Possible wrong connection identification"
MINOR: quic: Support transport parameters draft TLS extension
If we want to run quic-tracker against haproxy, we must at least
support the draft version of the TLS extension for the QUIC transport
parameters (0xffa5). quic-tracker QUIC version is draft-29 at this time.
We select this depending on the QUIC version. If draft, we select the
draft TLS extension.
UDP datagrams with Initial packet were padded only for the clients (haproxy
servers). But such packets MUST also be padded for the servers (haproxy
listeners). Furthere, for servers, only UDP datagrams containing ack-eliciting
Initial packet must be padded.
MINOR: quic: Possible wrong connection identification
A client may send several Initial packets. This is the case for picoquic
with -Q option. In this case we must identify the connection of incoming
Initial packets thanks to the original destination connection ID.
When allocating destination addresses for QUIC connections we did not set
this flag which denotes these addresses have been set. This had as side
effect to prevent the H3 request results from being returned to the QUIC clients.
Note that this bug was revealed by this commit:
"MEDIUM: backend: Rely on addresses at stream level to init server connection"
Thanks to Christopher for having found the real cause of this issue.
Willy Tarreau [Fri, 19 Nov 2021 10:41:10 +0000 (11:41 +0100)]
BUG/MEDIUM: mux-h2: always process a pending shut read
During 2.4-dev, an issue with partial frames was fixed with commit 3d4631fec ("BUG/MEDIUM: mux-h2: fix read0 handling on partial frames").
However this patch is not completely correct. It makes h2_recv() return
0 if the connection was shut for reads, but this not make h2_io_cb()
call h2_process(), so if there are any pending data left in the demux
buffer, they will never be processed, and the I/O callback will be
called in loops forever from the poller.
The correct return value there is 1, as is done at the end of the
function to report a pending read0.
This should definitely fix issue #1328. However even after a lot of
tests I couldn't manage to reproduce it, the conditions to enter that
situation are quite racy.
This must be backported to 2.0 since the fix above was merged into
2.0.21 and 2.2.9.
Willy Tarreau [Fri, 19 Nov 2021 09:23:36 +0000 (10:23 +0100)]
BUILD: makefile: stop opening sub-shells for each and every command
We're spending ~8% of the total build time calling a shell to display
"CC" using the "echo" command! We don't really need this, as make also
knows a "$(info ...)" command to print a message. However there's a catch,
this command trims leading spaces, so we need to use an invisible space
using "$ ". Furthermore, in GNU make 3.80 and older, $(info) doesn't show
anything, so we only do that for 3.81 and above, older versions continue
to use echo.
This measurably speeds up build time especially at -O0 that developers
use most of the time for quick checks.
BUG/MEDIUM: ssl: abort with the correct SSL error when SNI not found
Since commit c2aae74 ("MEDIUM: ssl: Handle early data with OpenSSL
1.1.1"), the codepath of the clientHello callback changed, letting an
unknown SNI escape with a 'return 1' instead of passing through the
abort label.
An error was still emitted because the frontend continued the handshake
with the initial_ctx, which can't be used to achieve an handshake.
However, it had the ugly side effect of letting the request pass in the
case of a TLS resume. Which could be surprising when combining strict-sni
with the removing of a crt-list entry over the CLI for example. (like
its done in the ssl/new_del_ssl_crlfile.vtc reg-test).
This patch switches the code path of the allow_early and abort label, so
the default code path is the abort one, letting the clientHello returns
the correct SSL_AD_UNRECOGNIZED_NAME in case of errors.
Which means the client will now receive:
OpenSSL error[0x14094458] ssl3_read_bytes: tlsv1 unrecognized name
projects / thirdparty / haproxy.git / log
