This patch allows to replace the host header generated by the
httpclient instead of adding a new one, resulting in the server replying
an error 400.
The host header is now generated from the uri only if it wasn't found in
the list of headers.
Also add a new request in the VTC file to test this.
This patch must be backported in 2.5.
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.
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
__ssl_sock_load_new_ckch_instance() does not free correctly the SNI in
the session cache, it only frees the one in the current tid.
This bug was introduced with e18d4e8 ("BUG/MEDIUM: ssl: backend TLS
resumption with sni and TLSv1.3").
This fix must be backported where the mentionned commit was backported.
(all maintained versions).
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.
This needs to be backported to 2.4.
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.
No backport is needed.
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.
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
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.
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.
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.
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.
This should be backported to all stable versions.
A warning is triggered by gcc9 on this code path, which is the compiler
version used by ubuntu20.04 on the github CI.
This is linked to github issue #1445.
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.
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.
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.
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"
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.
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.
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.
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.
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
Instead of:
OpenSSL error[0x14094410] ssl3_read_bytes: sslv3 alert handshake failure
Which was the error emitted before HAProxy 1.8.
This patch must be carrefuly backported as far as 1.8 once we validated
its impact.
When establishing an outboud connection, haproxy checks if the cached
TLS session has the same SNI as the connection we are trying to
resume.
This test was done by calling SSL_get_servername() which in TLSv1.2
returned the SNI. With TLSv1.3 this is not the case anymore and this
function returns NULL, which invalidates any outboud connection we are
trying to resume if it uses the sni keyword on its server line.
This patch fixes the problem by storing the SNI in the "reused_sess"
structure beside the session itself.
The ssl_sock_set_servername() now has a RWLOCK because this session
cache entry could be accessed by the CLI when trying to update a
certificate on the backend.
This fix must be backported in every maintained version, however the
RWLOCK only exists since version 2.4.
With the CI occasionally slowing down, we're starting to see again some
spurious failures despite the long 1-second timeouts. This reports false
positives that are disturbing and doesn't provide as much value as this
could. However at this delay it already becomes a pain for developers
to wait for the tests to complete.
This commit adds support for the new environment variable
HAPROXY_TEST_TIMEOUT that will allow anyone to modify the connect,
client and server timeouts. It was set to 5 seconds by default, which
should be plenty for quite some time in the CI. All relevant values
that were 200ms or above were replaced by this one. A few larger
values were left as they are special. One test for the set-timeout
action that used to rely on a fixed 1-sec value was extended to a
fixed 5-sec, as the timeout is normally not reached, but it needs
to be known to compare the old and new values.
This test was dependent on the check/server timeout to detect a failure,
which is not logical since we also use that one as an upper bound for
success in the second test, and that needlessly extends the test duration.
Let's make sur the timeout strikes immediately with only 1 ms timeout. Now
the total tests time is around 5.3-5.4s down from 8.7s in dev14. There is
still quite some room for improvement in the peers tests which all wait 2s
before starting but this will be more complicated.
Some tests expect a 503, typically those that check that wrong CA/CRL
will not be accepted between a server and a frontend. But such tests
tend to last very long simply because of the 1-second turn-around on
connection retries that happens during the failure. Let's properly set
the retries count to zero for these ones. One test purposely wants to
exhaust the retries so the retries was set to 1 instead.
For each test, the version number is evaluated using a call to awk,
which can be slow to start depending on the versions and OS. This is
only needed for a printf() call to keep only leading digits of each
component, multiply them by 1000 and pad them to 3 digits, something
that's clearly doable in plain shell in a portable way. This is what
this patch does, and it saves yet another 400 ms here on the full
test sequence.
grep is used in the arguments loops to check for features such as OPENSSL
or LUA or services like prometheus-exporter. Let's just look for the words
inside the list, which requires to prepend a delimitor at the beginning of
the list and add one at the end.
run-tegtests is starting to take a lot of time to spot which tests are
eligible, because for each test file a lot of "sed" sub-processes are
launched. This commit eliminates calls to sed by using the shell's
internal processing and parsing the VTC file only once. Instead of
extracting each option one by one from the file, all entries that look
like a valid option are passed to a single case/esac statement and their
value is extracted. Splitting into lists is simply done by adjusting the
IFS depending on the list's delimiter, which, contrary to the // pattern
modifier, is supported on every shell.
This was tested on both bash and dash, and the tests' execution time
dropped by 31% from 8.7 seconds to 6.0 seconds.
Sometimes it is really useful to be able to specify a default value for
an optional environment variable, like the ${name-value} construct in
shell. In fact we're really missing this for a number of settings in
reg tests, starting with timeouts.
This commit simply adds support for the common syntax above. Other
common forms like '+' to replace existing variables, or ':-' and ':+'
to act on empty variables, were not implemented at this stage, as they
are less commonly needed.
The else is not for boringSSL but for the lack of Client Hello callback.
Should have been changed in 1fc44d4 ("BUILD: ssl: guard Client Hello
callbacks with HAVE_SSL_CLIENT_HELLO_CB macro instead of openssl
version").
Could be backported in 2.4.
