In 8ba10fea6 ("BUG/MINOR: peers: Incomplete peers sections should be
validated."), some checks were relaxed in parse_server(), and extra logic
was added in the peers section parser in an attempt to properly ignore
incomplete "server" or "peer" statement under peers section.
This was done in response to GH #565, the main intent was that haproxy
should already complain about incomplete peers section (ie: missing
localpeer).
However, 8ba10fea69 explicitly skipped the peer cleanup upon missing
srv association for local peers. This is wrong because later haproxy
code always assumes that peer->srv is valid. Indeed, we got reports
that the (invalid) config below would cause segmentation fault on
all stable versions:
global
localpeer 01JM0TEPAREK01FQQ439DDZXD8
peers my-table
peer 01JM0TEPAREK01FQQ439DDZXD8
listen dummy
bind localhost:8080
To fix the issue, instead of by-passing some cleanup for the local
peer, handle this case specifically by doing the regular peer cleanup
and reset some fields set on the curpeers and curpeers proxy because
of the invalid local peer (do as if the peer was not declared).
It should still comply with requirements from #565.
This patch should be backported to all stable versions.
In the "peers" section parser, right after parse_server() is called, we
used to check whether the curpeers->peers_fe->srv pointer was set or not
to know if parse_server() successfuly added a server to the peers proxy,
server that we can then associate to the new peer.
However the check is wrong, as curpeers->peers_fe->srv points to the
last added server, if a server was successfully added before the
failing one, we cannot detect that the last parse_server() didn't
add a server. This is known to cause bug with bad "peer"/"server"
statements.
To fix the issue, we save a pointer on the last known
curpeers->peers_fe->srv before parse_server() is called, and we then
compare the save with the pointer after parse_server(), if the value
didn't change, then parse_server() didn't add a server. This makes
the check consistent in all situations.
It should be backported to all stable versions.
In check_config_validity() we evaluate some sample fetch expressions
(log-format, server rules, etc). These expressions may use external files like
maps.
If some particular 'default-path' was set in the global section before, it's no
longer applied to resolve file pathes in check_config_validity(). parse_cfg()
at the end of config parsing switches back to the initial cwd.
This fixes the issue #2886.
This patch should be backported in all stable versions since 2.4.0, including
2.4.0.
This commit introduces the dont-parse-log option to disable log message
parsing, allowing raw log data to be forwarded without modification.
Also, it adds the assume-rfc6587-ntf option to frame log messages
using only non-transparent framing as per RFC 6587. This avoids
missparsing in certain cases (mainly with non RFC compliant messages).
The documentation is updated to include details on the new options and
their intended use cases.
This feature was discussed in GH #2856
This commit adds a new function `prepare_log_message` to initialize log
message buffers and metadata. This function sets default values for log
level and facility, ensuring a consistent starting state for log
processing. It also prepares the buffer and metadata fields, simplifying
subsequent log parsing and construction.
This commit adds parsing of options in log-forward config sections and
prepares the scenario to implement actual changes of behaviuor. So far
we only take in account proxy->options2, which is the bit container with
more available positions.
cfg_parse_listen_match_option() takes cfg_opt array as parameter, as well
current args, expected mode and cap bitfields.
It is expected to be used under cfg_parse_listen() function or similar.
Its goal is to remove code duplication around proxy->options and
proxy->options2 handling, since the same checks are performed for the
two. Also, this function could help to evaluate proxy options for
mode-specific proxies such as log-forward section for instance:
by giving the expected mode and capatiblity as input, the function
would only match compatible options.
Current pr_mode enum is a regular enum because a proxy only supports one
mode at a time. However it can be handy for a function to be given a
list of compatible modes for a proxy, and we can't do that using a
bitfield because pr_mode is not bitfield compatible (values share
the same bits).
In this patch we manually define pr_mode values so that they are all
using separate bits and allows a function to take a bitfield of
compatible modes as parameter.
As reported by Nick Ramirez in GH #2891, it is currently not possible to
use log-profile without a log-format set on the proxy.
This is due to historical reason, because all log sending functions avoid
trying to send a log with empty message. But now with log-profile which
can override log-format, it is possible that some loggers may actually
end up generating a valid log message that should be sent! Yet from the
upper logging functions we don't know about that because loggers are
evaluated in lower API functions.
Thus, to avoid skipping potentially valid messages (thanks to log-profile
overrides), in this patch we postpone the decision to send or not empty
log messages in lower log API layer, ie: _process_send_log_final(), once
the log-profile settings were evaluated for a given logger.
A known side-effect of this change is that fe->log_count statistic may
be increased even if no log message is sent because the message was empty
and even the log-profile didn't help to produce a non empty log message.
But since configurations lacking proxy log-format are not supposed to be
used without log-profile (+ log steps combination) anyway it shouldn't be
an issue.
Historically, __send_log() was called with terminating NULL byte after
the message payload. But now that __send_log() supports being called
without terminating NULL byte (thanks to size hint), and that __sendlog()
actually stips any \n or NULL byte, we don't need to bother with that
anymore. So let's remove extra logic around __send_log() users where we
added 1 extra byte for the terminating NULL byte.
No change of behavior should be expected.
result.data.u.str.size was set to size+1 to take into account terminating
NULL byte as per the comment. But this is wrong because the caller is free
to set size to just the right amount of bytes (without terminating NULL
byte). In fact all smp API functions will not read past str.data so there
is not risk about uninitialized reads, but this leaves an ambiguity for
converters that may use all the smp size to perform transformations, and
since we don't know about the "message" memory origin, we cannot assume
that its size may be greater than size. So we max it out to size just to
be safe.
This bug was not known to cause any issue, it was spotted during code
review. It should be backported in 2.9 with b30bd7a ("MEDIUM: log/balance:
support for the "hash" lb algorithm")
This is a complementary patch to 0e1f389fe9 ("DOC: config: removing
"log-balance" references"): we properly removed all log-balance
references in the doc but there remained some in the code, let's fix
that.
It could be backported in 2.9 with 0e1f389fe9
Sample fetches %[accept_date] and %[request_date] with converters can be used
in error-log-format string. But in the most error cases they fetches nothing,
as error logs are produced on SSL handshake issues or when invalid PROXY
protocol header is used. Stream object is never allocated in such cases and
smp_fetch_accept_date() just simply returns 0.
There is a need to have a custom date format (ISO8601) also in the error logs,
along with normal logs. When sess_build_logline_orig() builds log line it
always copies the accept date to strm_logs structure. When stream is absent,
accept date is copied from the session object.
So, if the steam object wasn't allocated, let's use the session date info in
smp_fetch_accept_date(). This allows then, in sample_process(), to apply to the
fetched date different converters and formats.
This fixes the issue #2884.
Add a new macro, REGISTER_UNITTEST(), that will automatically make sure
we call hap_register_unittest(), instead of having to create a function
that will do so.
OpenSSL version greater than 3.0 does not use the same API when
manipulating EVP_PKEY structures, the EC_KEY API is deprecated and it's
not possible anymore to get an EC_GROUP and simply call
EC_GROUP_get_curve_name().
Instead, one must call EVP_PKEY_get_utf8_string_param with the
OSSL_PKEY_PARAM_GROUP_NAME parameter, but this would result in a SECG
curves name, instead of a NIST curves name in previous version.
(ex: secp384r1 vs P-384)
This patch adds 2 functions:
- the first one look for a curves name and converts it to an openssl
NID.
- the second one converts a NID to a NIST curves name
The list only contains: P-256, P-384 and P-521 for now, it could be
extended in the fure with more curves.
`make unit-tests` would run shell scripts from tests/unit/
The run-unittests.sh script will look for any .sh in tests/unit/ and
will call it twice:
- first with the 'check' argument in order to decide if we should skip
the test or not
- second to run the check
A simple test could be written this way:
#!/bin/sh
check() {
${HAPROXY_PROGRAM} -cc 'feature(OPENSSL)'
command -v socat
}
run() {
${HAPROXY_PROGRAM} -dI -f ${ROOTDIR}/examples/quick-test.cfg -c
}
case "$1" in
"check")
check
;;
"run")
run
;;
esac
The tests *MUST* be written in POSIX shell in order to be portable, and
any special commands should be tested with `command -v` before using it.
Tests are run with `sh -e` so everything must be tested.
Doing unit tests with haproxy was always a bit difficult, some of the
function you want to test would depend on the buffer or trash buffer
initialisation of HAProxy, so building a separate main() for them is
quite hard.
This patch adds a way to register a function that can be called with the
"-U" parameter on the command line, will be executed just after
step_init_1() and will exit the process with its return value as an exit
code.
When using the -U option, every keywords after this option is passed to
the callback and could be used as a parameter, letting the capability to
handle complex arguments if required by the test.
HAProxy need to be built with DEBUG_UNIT to activate this feature.
Implement a converter which takes an EVP_PKEY and converts it to a
public JWK key. This is the first step of the JWS implementation.
It supports both EC and RSA keys.
Know to work with:
- LibreSSL
- AWS-LC
- OpenSSL > 1.1.1
As reported in issue #2882, using "no-send-proxy-v2" on a server line does
not properly disable the use of proxy-protocol if it was enabled in a
default-server directive in combination with other PP options. The reason
for this is that the sending of a proxy header is determined by a test on
srv->pp_opts without any distinction, so disabling PPv2 while leaving other
options results in a PPv1 header to be sent.
Let's fix this by explicitly testing for the presence of either send-proxy
or send-proxy-v2 when deciding to send a proxy header.
This can be backported to all versions. Thanks to Andre Sencioles (@asenci)
for reporting the issue and testing the fix.
HTTP/0.9 parser was recently updated to support truncated requests in
rcv_buf operation. However, this caused a leak as input buffer is
allocated early.
In fact, the leak was already present in case of fatal errors. Fix this
by first delaying buffer allocation, so that initial checks are
performed before. Then, ensure that buffer is released in case of a
latter error.
This is considered as minor, as HTTP/0.9 is reserved for experiment and
QUIC interop usages.
This should be backported up to 2.6.
At least one user would like to allow a standards-violating client setup
WebSocket connections through haproxy to a standards-violating server that
accepts them. While this should of course never be done over the internet,
it can make sense in the datacenter between application components which do
not need to mask the data, so this typically falls into the situation of
what the "accept-unsafe-violations-in-http-request" option and the
"accept-unsafe-violations-in-http-response" option are made for.
See GH #2876 for more context.
This patch relaxes the test on the "Sec-Websocket-Key" header field in
the request, and of the "Sec-Websocket-Accept" header in the response
when these respective options are set.
The doc was updated to reference this addition. This may be backported
to 3.1 but preferably not further.
Don't reserve space for the HTX overhead on receive if the demux buffer is
not empty. Otherwise, the demux buffer may be erroneously reported as full
and this may block records processing. Because of this bug, a ping-pong loop
till timeout between data reception and demux process can be observed.
This bug was introduced by the commit 5f927f603 ("BUG/MEDIUM: mux-fcgi:
Properly handle read0 on partial records"). To fix the issue, if the demux
buffer is not empty when we try to receive more data, all free space in the
buffer can now be used. However, if the demux buffer is empty, we still try
to keep it aligned with the HTX.
This patch must be backported to 3.1.
Extends HTTP/0.9 layer to be able to deal with incomplete requests.
Instead of an error, 0 is returned. Thus, instead of a stream closure.
QUIC-MUX may retry rcv_buf operation later if more data is received,
similarly to HTTP/3 layer.
Note that HTTP/0.9 is only used for testing and interop purpose. As
such, this limitation is not considered as a bug. It is probably not
worth to backport it.
Return value of h3_rcv_buf() is incorrectly documented. Indeed, it may
return a positive value to indicate that input bytes were converted into
HTX. This is especially important, as caller uses this value to consume
the reported data amount in QCS Rx buffer.
This should be backported up to 2.6. Note that on 2.8, h3_rcv_buf() was
named h3_decode_qcs().
HTTP/3 specification allows a server to emit the entire response even if
only a partial request was received. In particular, this happens when
request STREAM FIN is delayed and transmitted in an empty payload frame.
In this case, qcc_abort_stream_read() was used by HTTP/3 layer to emit a
STOP_SENDING. Remaining received data were not transmitted to the stream
layer as they were simply discared. However, this prevents FIN
transmission to the stream layer. This causes the transfer to be
considered as prematurely closed, resulting in a cL-- log line status.
This is misleading to users which could interpret it as if the response
was not sent.
To fix this, disable STOP_SENDING emission on full preemptive reponse
emission. Rx channel is kept opened until the client closes it with
either a FIN or a RESET_STREAM. This ensures that the FIN signal can be
relayed to the stream layer, which allows the transfer to be reported as
completed.
This should be backported up to 2.9.
The PROXY v2 TLVs were not properly initialized when defined with
"set-proxy-v2-tlv-fmt" keyword, which could have caused a crash when
validating the configuration or malfunction (e.g. when used in
combination with "server-template" and/or "default-server").
The issue was introduced with commit 6f4bfed3a ("MINOR: server: Add
parser support for set-proxy-v2-tlv-fmt").
This should be backported up to 2.9.
Maxconn is a bit of a misnomer when it comes to servers, as it doesn't
control the maximum number of connections we establish to a server, but
the maximum number of simultaneous requests. So add "strict-maxconn",
that will make it so we will never establish more connections than
maxconn.
It extends the meaning of the "restricted" setting of
tune.takeover-other-tg-connections, as it will also attempt to get idle
connections from other thread groups if strict-maxconn is set.
Allow haproxy to take over idle connections from other thread groups
than our own. To control that, add a new tunable,
tune.takeover-other-tg-connections. It can have 3 values, "none", where
we won't attempt to get connections from the other thread group (the
default), "restricted", where we only will try to get idle connections
from other thread groups when we're using reverse HTTP, and "full",
where we always try to get connections from other thread groups.
Unless there is a special need, it is advised to use "none" (or
restricted if we're using reverse HTTP) as using connections from other
thread groups may have a performance impact.
In pollers that support it, provide the generation number in addition to
the fd, and, when an event happened, if the generation number is the
same, but the tgid changed, then assumed the fd was taken over by a
thread from another thread group, and just delete the event from the
current thread's poller, as we no longer want to hear about it.
Add a fixup_tgid_takeover() method to pollers for which it makes sense
(epoll, kqueue and evport). That method can be called after a takeover
of a fd from a different thread group, to make sure the poller's
internal structure reflects the new state.
Check that the call to epoll_ctl() succeeds, and if it does not, if
we're adding a new event and it fails with EEXIST, then delete and
re-add the event. There are a few cases where we may already have events
for a fd. If epoll_ctl() fails for any reason, use BUG_ON to make sure
we immediately crash, as this should not happen.
Users may have good reasons for using "tune.idle-pool.shared off", one of
them being the cost of moving cache lines between cores, or the kernel-
side locking associated with moving FDs. For this reason, when getting
close to the file descriptors limits, we must not try to kill adjacent
threads' FDs when the sharing of pools is disabled. This is extremely
expensive and kills the performance. We must limit ourselves to our local
FDs only. In such cases, it's up to the users to configure a large enough
maxconn for their usages.
Before this patch, perf top reported 9% CPU usage in connect_server()
onthe trylock used to kill connections when running at 4800 conns for
a global maxconn of 6400 on a 128-thread server. Now it doesn't spend
its time there anymore, and performance has increased by 12%. Note,
it was verified that disabling the locks in such a case has no effect
at all, so better keep them and stay safe.
In issue #2861, Jarosaw Rzeszótko reported another issue with
"show threads", this time in relation with the conversion of a stream's
accept date to local time. Indeed, if the libc was interrupted in this
same function, it could have been interrupted with a lock held, then
it's no longer possible to dump the date, and we face a deadlock.
This is easy to reproduce with logging enabled.
Let's detect we come from a signal handler and do not try to resolve
the time to localtime in this case.
While signals are not recursive, one signal (e.g. wdt) may interrupt
another one (e.g. debug). The problem this causes is that when leaving
the inner handler, it removes the outer's flag, hence the protection
that comes with it. Let's just have 3 distinct flags for regular signals,
debug signal and watchdog signal. We add a 4th definition which is an
aggregate of the 3 to ease testing.
Annika Wickert reported some occasional disconnections between haproxy
and varnish when communicating over HTTP/2, with varnish complaining
about protocol errors while captures looked apparently normal. Nils
Goroll managed to reproduce this on varnish by injecting the capture of
the outgoing haproxy traffic and noticed that haproxy was forwarding a
header value containing a trailing space, which is now explicitly
forbidden since RFC9113.
It turns out that the only way for such a header to pass through haproxy
is to arrive in h2 and not be edited, in which case it will arrive in
HTX with its undesired spaces. Since the code dealing with HTX headers
always trims spaces around them, these are not observable in dumps, but
only when started in debug mode (-d). Conversions to/from h1 also drop
the spaces.
With this patch we trim LWS both on input and on output. This way we
always present clean headers in the whole stack, and even if some are
manually crafted by the configuration or Lua, they will be trimmed on
the output.
This must be backported to all stable versions.
Thanks to Annika for the helpful capture and Nils for the help with
the analysis on the varnish side!
