BUG/MEDIUM: h3: Properly report a C-L header was found to the HTX start-line
When H3 HEADERS frames are converted to HTX, if a Content-Length header was
found, the HTX start-line must be notified by setting HTX_SL_F_CLEN flag.
Some components may rely on this flag to know there is a content-length
without looping on headers to get the info.
Among other this, it is mandatory for the FCGI multiplexer because it must
announce the message body length.
BUG/MINOR: ssl: OCSP callback only registered for first SSL_CTX
If multiple SSL_CTXs use the same certificate that has an OCSP response
file on the filesystem, only the first one will have the OCSP callback
set. This bug was introduced by "cc346678d MEDIUM: ssl: Add ocsp_certid
in ckch structure and discard ocsp buffer early" which cleared the
ocsp_response from the ckch_data after it was inserted in the tree,
which prevented subsequent contexts from having the callback registered.
Released version 2.9-dev2 with the following main changes :
- BUG/MINOR: quic: Possible leak when allocating an encryption level
- BUG/MINOR: quic: Missing QUIC connection path member initialization
- BUILD: quic: Compilation fixes for some gcc warnings with -O1
- DOC: ssl: Fix typo in 'ocsp-update' option
- DOC: ssl: Add ocsp-update troubleshooting clues and emphasize on crt-list only aspect
- BUG/MINOR: tcp_sample: bc_{dst,src} return IP not INT
- MEDIUM: acl/sample: unify sample conv parsing in a single function
- MINOR: sample: introduce c_pseudo() conv function
- MEDIUM: sample: add missing ADDR=>? compatibility matrix entries
- MINOR: sample: fix ipmask sample definition
- MEDIUM: tree-wide: fetches that may return IPV4+IPV6 now return ADDR
- MEDIUM: sample: introduce 'same' output type
- BUG/MINOR: quic: Possible crash in "show quic" dumping packet number spaces
- BUG/MINOR: cache: A 'max-age=0' cache-control directive can be overriden by a s-maxage
- BUG/MEDIUM: sink: invalid server list in sink_new_from_logsrv()
- BUG/MINOR: http_ext: unhandled ERR_ABORT in proxy_http_parse_7239()
- BUG/MINOR: sink: missing sft free in sink_deinit()
- BUG/MINOR: ring: size warning incorrectly reported as fatal error
- BUG/MINOR: ring: maxlen warning reported as alert
- BUG/MINOR: log: LF upsets maxlen for UDP targets
- MINOR: sink/api: pass explicit maxlen parameter to sink_write()
- BUG/MEDIUM: log: improper use of logsrv->maxlen for buffer targets
- BUG/MINOR: log: fix missing name error message in cfg_parse_log_forward()
- BUG/MINOR: log: fix multiple error paths in cfg_parse_log_forward()
- BUG/MINOR: log: free errmsg on error in cfg_parse_log_forward()
- BUG/MINOR: sink: invalid sft free in sink_deinit()
- BUG/MINOR: sink: fix errors handling in cfg_post_parse_ring()
- BUG/MINOR: server: set rid default value in new_server()
- MINOR: hlua_fcn/mailers: handle timeout mail from mailers section
- BUG/MINOR: sink/log: properly deinit srv in sink_new_from_logsrv()
- EXAMPLES: maintain haproxy 2.8 retrocompatibility for lua mailers script
- BUG/MINOR: hlua_fcn/queue: use atomic load to fetch queue size
- BUG/MINOR: config: Remove final '\n' in error messages
- BUG/MINOR: config: Lenient port configuration parsing
- BUG/MEDIUM: quic: token IV was not computed using a strong secret
- BUG/MINOR: quic: retry token remove one useless intermediate expand
- BUG/MEDIUM: quic: missing check of dcid for init pkt including a token
- BUG/MEDIUM: quic: timestamp shared in token was using internal time clock
- CLEANUP: quic: remove useless parameter 'key' from quic_packet_encrypt
- BUG/MINOR: hlua: hlua_yieldk ctx argument should support pointers
- BUG/MEDIUM: hlua_fcn/queue: bad pop_wait sequencing
- DOC: config: Fix fc_src description to state the source address is returned
- BUG/MINOR: sample: Fix wrong overflow detection in add/sub conveters
- BUG/MINOR: http: Return the right reason for 302
- MEDIUM: ssl: new sample fetch method to get curve name
- CI: add naming convention documentation
- CI: explicitely highlight VTest result section if there's something
- BUG/MINOR: quic: Unckecked encryption levels availability
- BUILD: quic: fix warning during compilation using gcc-6.5
- BUG/MINOR: hlua: add check for lua_newstate
- BUG/MINOR: h1-htx: Return the right reason for 302 FCGI responses
- MINOR: lua: Allow reading "proc." scoped vars from LUA core.
- MINOR: cpuset: add cpu_map_configured() to know if a cpu-map was found
- BUG/MINOR: config: do not detect NUMA topology when cpu-map is configured
- BUG/MINOR: cpuset: remove the bogus "proc" from the cpu_map struct
- BUG/MINOR: init: set process' affinity even in foreground
- CLEANUP: cpuset: remove the unused proc_t1 field in cpu_map
- CLEANUP: config: make parse_cpu_set() return documented values
- BUG/MINOR: server: Don't warn on server resolution failure with init-addr none
- MINOR: peers: add peers keyword registration
- MINOR: quic: Stop storing the TX encoded transport parameters
- MINOR: quic: Dynamic allocation for negotiated Initial TLS cipher context.
- MINOR: quic: Release asap the negotiated Initial TLS context.
- MINOR: quic: Add traces to qc_may_build_pkt()
- MEDIUM: quic: Packet building rework.
- CLEANUP: quic: Remove a useless TLS related variable from quic_conn_io_cb().
- MEDIUM: quic: Handshake I/O handler rework.
- MINOR: quic: Add traces for qc_frm_free()
- MINOR: quic: add trace about pktns packet/frames releasing
- BUG/MINOR: quic: Missing parentheses around PTO probe variable.
- MINOR: quic: Ping from Initial pktns before reaching anti-amplification limit
- BUG/MINOR: server-state: Ignore empty files
- BUG/MINOR: server-state: Avoid warning on 'file not found'
- BUG/MEDIUM: listener: Acquire proxy's lock in relax_listener() if necessary
- MINOR: quic: QUIC openssl wrapper implementation
- MINOR: quic: Include QUIC opensssl wrapper header from TLS stacks compatibility header
- MINOR: quic: Do not enable O-RTT with USE_QUIC_OPENSSL_COMPAT
- MINOR: quic: Set the QUIC connection as extra data before calling SSL_set_quic_method()
- MINOR: quic: Do not enable 0RTT with SSL_set_quic_early_data_enabled()
- MINOR: quic: Add a compilation option for the QUIC OpenSSL wrapper
- MINOR: quic: Export some KDF functions (QUIC-TLS)
- MINOR: quic: Make ->set_encryption_secrets() be callable two times
- MINOR: quic: Initialize TLS contexts for QUIC openssl wrapper
- MINOR: quic: Call the keylog callback for QUIC openssl wrapper from SSL_CTX_keylog()
- MINOR: quic: Add a quic_openssl_compat struct to quic_conn struct
- MINOR: quic: Useless call to SSL_CTX_set_quic_method()
- MINOR: quic: SSL context initialization with QUIC OpenSSL wrapper.
- MINOR: quic: Missing encoded transport parameters for QUIC OpenSSL wrapper
- MINOR: quic: Add "limited-quic" new tuning setting
- DOC: quic: Add "limited-quic" new tuning setting
- DOC: install: Document how to build a limited support for QUIC
Document "limited-quic" new tuning setting which must be used to
enable the QUIC listener bindings when haproxy is compiled against
a TLS/SSL stack without QUIC support.
MINOR: quic: Add "limited-quic" new tuning setting
This setting which may be used into a "global" section, enables the QUIC listener
bindings when haproxy is compiled with the OpenSSL wrapper. It has no effect
when haproxy is compiled against a TLS stack with QUIC support, typically quictls.
MINOR: quic: Missing encoded transport parameters for QUIC OpenSSL wrapper
This wrapper needs to have an access to an encoded version of the local transport
parameter (to be sent to the peer). They are provided to the TLS stack thanks to
qc_ssl_compat_add_tps_cb() callback.
These encoded transport parameters were attached to the QUIC connection but
removed by this commit to save memory:
MINOR: quic: Stop storing the TX encoded transport parameters
This patch restores these transport parameters and attaches them again
to the QUIC connection (quic_conn struct), but only when the QUIC OpenSSL wrapper
is compiled.
Implement qc_set_quic_transport_params() to encode the transport parameters
for a connection and to set them into the stack and make this function work
for both the OpenSSL wrapper or any other TLS stack with QUIC support. Its uses
the encoded version of the transport parameters attached to the connection
when compiled for the OpenSSL wrapper, or local parameters when compiled
with TLS stack with QUIC support. These parameters are passed to
quic_transport_params_encode() and SSL_set_quic_transport_params() as before
this patch.
MINOR: quic: SSL context initialization with QUIC OpenSSL wrapper.
When the QUIC OpenSSL wrapper is used, the keylog has to be set and a QUIC
specific TLS 1.3 extension must be added to the EncryptedExtensions message.
This is done by quic_tls_compat_init().
MINOR: quic: Useless call to SSL_CTX_set_quic_method()
SSL_set_quic_method() is already called at SSL session level. This call
is useless. Furthermore, SSL_CTX_set_quic_method() is not implemented by
the QUIC OpenSSL wrapper to come.
Should be backported as far as 2.6 to ease further backports to come.
MINOR: quic: Call the keylog callback for QUIC openssl wrapper from SSL_CTX_keylog()
SSL_CTX_keylog() is the callback used when the TLS keylog feature is enabled with
tune.ssl.keylog configuration setting. But the QUIC openssl wrapper also needs
to use such a callback to receive the QUIC TLS secrets from the TLS stack.
Add a call to the keylog callback for the QUIC openssl wrapper to SSL_CTX_keylog()
to ensure that it will be called when the TLS keylog feature is enabled.
MINOR: quic: Initialize TLS contexts for QUIC openssl wrapper
When the QUIC OpenSSL wrapper use is enabled, all the TLS contexts (SSL_CTX) must
be configured to support it. This is done calling quic_tls_compat_init() from
ssl_sock_prepare_ctx(). Note that quic_tls_compat_init() ignore the TLS context
which are not linked to non-QUIC TLS sessions/connections.
MINOR: quic: Make ->set_encryption_secrets() be callable two times
With this patch, ha_set_encryption_secrets() may be callable two times,
one time to derive the RX secrets and a second time to derive the TX secrets.
There was a missing step to do so when the RX secret was received from the stack.
In this case the secret was not stored for the keyupdate, leading the keyupdate
RX part to be uninitialized.
Add a label to initialize the keyupdate RX part and a "goto" statement to run
the concerned code after having derived the RX secrets.
This patch is required to make the keupdate feature work with the OpenSSL wrapper.
MINOR: quic: Do not enable 0RTT with SSL_set_quic_early_data_enabled()
SSL_set_quic_early_data_enabled is not implemented by the QUIC OpenSSL wrapper.
Furthermore O-RTT is not supported by this wrapper. Do not know why at
this time.
MINOR: quic: Set the QUIC connection as extra data before calling SSL_set_quic_method()
This patch is required for the QUIC OpenSSL wrapper, and does not break anything
for the other TLS stacks with their own QUIC support (quictls for instance).
The implementation of SSL_set_quic_method() needs to access the quic_conn object
to store data within. But SSL_set_quic_method() is only aware of the SSL session
object. This is the reason why it is required to set the quic_conn object
as extra data to the SSL session object before calling SSL_set_quic_method()
so that it can be retrieve by SSL_set_quic_method().
MINOR: quic: Include QUIC opensssl wrapper header from TLS stacks compatibility header
Include haproxy/quic_openssl_compat.h from haproxy/openssl-compat.h when the
compilation of the QUIC openssl wrapper for TLS stacks is enabled with
USE_QUIC_OPENSSLCOMPAT.
and SSL_QUIC_METHOD QUIC specific bio method which are also implemented by quictls
to support QUIC from OpenSSL. So, its aims is to support QUIC from a standard OpenSSL
stack without QUIC support. It relies on the OpenSSL keylog feature to retreive
the secrets derived by the OpenSSL stack during a handshake and to pass them to
the ->set_encryption_secrets() callback as this is done by quictls. It makes
usage of a callback (quic_tls_compat_msg_callback()) to handle some TLS messages
only on the receipt path. Some of them must be passed to the ->add_handshake_data()
callback as this is done with quictls to be sent to the peer as CRYPTO data.
quic_tls_compat_msg_callback() callback also sends the received TLS alert with
->send_alert() callback.
AES 128-bits with CCM mode is not supported at this time. It is often disabled by
the OpenSSL stack, but as it can be enabled by "ssl-default-bind-ciphersuites",
the wrapper will send a TLS alerts (Handhshake failure) if this algorithm is
negotiated between the client and the server.
BUG/MEDIUM: listener: Acquire proxy's lock in relax_listener() if necessary
Listener functions must follow a common locking pattern:
1. Get the proxy's lock if necessary
2. Get the protocol's lock if necessary
3. Get the listener's lock if necessary
We must take care to respect this order to avoid any ABBA issue. However, an
issue was introduced in the commit bcad7e631 ("MINOR: listener: add
relax_listener() function"). relax_listener() gets the lisener's lock and if
resume_listener() is called, the proxy's lock is then acquired.
So to fix the issue, the proxy's lock is first acquired in relax_listener(),
if necessary.
This patch should fix the issue #2222. It must be backported as far as 2.4
because the above commit is marked to be backported there.
BUG/MINOR: server-state: Avoid warning on 'file not found'
On a clean installation, users might want to use server-state-file and
the recommended zero-warning option. This caused a problem if
server-state-file was not found, as a warning was emited, causing
startup to fail.
This will allow users to specify nonexistent server-state-file at first,
and dump states to the file later.
Fixes #2190
CF: Technically speaking, this patch can be backported to all stable
versions. But it is better to do so to 2.8 only for now.
Users might want to pre-create an empty file for later dumping
server-states. This commit allows for that by emiting a notice in case
file is empty and a warning if file is not empty, but version is unknown
Fix partially: #2190
CF: Technically speaking, this patch can be backported to all stable
versions. But it is better to do so to 2.8 only for now.
MINOR: quic: Ping from Initial pktns before reaching anti-amplification limit
There are cases where there are enough room on the network to send 1200 bytes
into a PING only Initial packets. This may be considered as the last chance
for the connection to complete the handshake. Indeed, the client should
reply with at least a 1200 bytes datagram with an Initial packet inside.
This would give the haproxy endpoint a credit of 3600 bytes to complete
the handshake before reaching the anti-amplification limit again, and so on.
BUG/MINOR: quic: Missing parentheses around PTO probe variable.
It is hard to analyze the impact of this bug. I guess it could lead a connection
to probe infinitively (with an exponential backoff probe timeout) during an handshake,
but one has never seen such a case.
Add missing parentheses around ->flags of the TX packet built by qc_do_build_pkt()
to detect that this packet embeds ack-eliciting frames. In this case if a probing
packet was needed the ->pto_probe value of the packet number space must be
decremented.
quic_conn_io_cb() is the I/O handler used during the handshakes. It called
qc_prep_pkts() after having called qc_treat_rx_pkts() to prepare datagrams
after having parsed incoming ones with branches to "next_level" label depending
on the connection state and if the current TLS session was a 0-RTT session
or not. The code doing that was ugly and not easy to maintain.
As qc_prep_pkts() is able to handle all the encryption levels available for
a connection, there is no need to keep this code.
After simplification, for now on, to be short, quic_conn_io_cb() called only one
time qc_prep_pkts() after having called qc_treat_rx_pkts().
Furthermore, there are more chances that this I/O handler could be reused for
the haproxy server side connections.
The aim of this patch is to allow the building of QUIC datagrams with
as much as packets with different encryption levels inside during handshake.
At this time, this is possible only for at most two encryption levels.
That said, most of the time, a server only needs to use two encryption levels
by datagram, except during retransmissions.
Modify qc_prep_pkts(), the function responsible of building datagrams, to pass
a list of encryption levels as parameter in place of two encryption levels. This
function is also used when retransmitting datagrams. In this case this is a
customized/flexible list of encryption level which is passed to this function.
Add ->retrans new member to quic_enc_level struct, to be used as attach point
to list of encryption level used only during retransmission, and ->retrans_frms
new member which is a pointer to a list of frames to be retransmitted.
MINOR: quic: Release asap the negotiated Initial TLS context.
This context may be released at the same time as the Initial TLS context.
This is done calling quic_tls_ctx_secs_free() and pool_free() in two code locations.
Implement quic_nictx_free() to do that.
MINOR: quic: Dynamic allocation for negotiated Initial TLS cipher context.
Shorten ->negotiated_ictx quic_conn struct member (->nictx).
This variable is used during version negotiation. Indeed, a connection
may have to support support several QUIC versions of paquets during
the handshake. ->nictx is the QUIC TLS cipher context used for the negotiated
QUIC version.
This patch allows a connection to dynamically allocate this TLS cipher context.
Add a new pool (pool_head_quic_tls_ctx) for such QUIC TLS cipher context object.
Modify qc_new_conn() to initialize ->nictx to NULL value.
quic_tls_ctx_secs_free() frees all the secrets attached to a QUIC TLS cipher context.
Modify it to do nothing if it is called with a NULL TLS cipher context.
Modify to allocate ->nictx from qc_conn_finalize() just before initializing
its secrets. qc_conn_finalize() allocates -nictx only if needed (if a new QUIC
version was negotiated).
Modify qc_conn_release() which release a QUIC connection (quic_conn struct) to
release ->nictx TLS cipher context.
MINOR: quic: Stop storing the TX encoded transport parameters
There is no need to keep an encoded version of the QUIC listener transport
parameters attache to the connection.
Remove ->enc_params and ->enc_params_len member of quic_conn struct.
Use variables to build the encoded transport parameter local to
ha_quic_set_encryption_secrets() before they are passed to
SSL_set_quic_transport_params().
Modify qc_ssl_sess_init() prototype. It was expected to be used with
the encoded transport parameters as passed parameter, but they were not
used. Cleanup this function.
BUG/MINOR: server: Don't warn on server resolution failure with init-addr none
During startup, when the "none" method for "init-addr" is evaluated, a
warning is emitted if a resolution failure was previously encountered. The
documentation of the "none" method states it should be used to ignore server
resolution failures and let the server starts in DOWN state. However,
because a warning may be emitted, it is not possible to start HAProxy with
"zero-warning" option.
The same is true when "-dr" command line option is used. It is counter
intuitive and, in a way, this contradict what is specified in the
documentation.
So instead, a notice message is now emitted. At the end, if "-dr" command
line option is used or if "none" method is explicitly used, it means the
admin is agree with server resolution failures. There is no reason to emit a
warning.
This patch should fix the issue #2176. It could be backported to all stable
versions but backporting to 2.8 is probably enough for now.
CLEANUP: config: make parse_cpu_set() return documented values
parse_cpu_set() stopped returning the undocumented -1 which was a
leftover from an earlier attempt, changed from ulong to int since
it only returns a success/failure and no more a mask. Thus it must
not return -1 and its callers must only test for != 0, as is
documented.
CLEANUP: cpuset: remove the unused proc_t1 field in cpu_map
This field used to store the cpumap of the first thread in a group, and
was used till 2.4 to hold some default settings, after which it was no
longer used. Let's just drop it.
BUG/MINOR: init: set process' affinity even in foreground
The per-process CPU affinity settings are only applied during forking,
which means that cpu-map are ignored when running in foreground (e.g.
haproxy started with -db). This is historic due to the original semantics
of a process array, but isn't documented and causes surprises when trying
to debug affinity settings.
Let's make sure the setting is applied to the workers themselves even
in foreground. This may be backported to 2.6 though it is really not
important. If backported, it also depends on previous commit:
BUG/MINOR: cpuset: remove the bogus "proc" from the cpu_map struct
BUG/MINOR: cpuset: remove the bogus "proc" from the cpu_map struct
We're currently having a problem with the porting from cpu_map from
processes to thread-groups as it happened in 2.7 with commit 5b09341c0
("MEDIUM: cpu-map: replace the process number with the thread group
number"), though it seems that it has deeper roots even in 2.0 and
that it was progressively made worng over time.
The issue stems in the way the per-process and per-thread cpu-sets were
employed over time. Originally only processes were supported. Then
threads were added after an optional "/" and it was documented that
"cpu-map 1" is exactly equivalent to "cpu-map 1/all" (this was clarified
in 2.5 by commit 317804d28 ("DOC: update references to process numbers
in cpu-map and bind-process").
The reality is different: when processes were still supported, setting
"cpu-map 1" would apply the mask to the process itself (and only when
run in the background, which is not documented either and is also a
bug for another fix), and would be combined with any possible per-thread
mask when calculating the threads' affinity, possibly resulting in empty
sets. However, "cpu-map 1/all" would only set the mask for the threads
and not the process. As such the following:
cpu-map 1 odd
cpu-map 1/1-8 even
would leave no CPU while doing:
cpu-map 1/all odd
cpu-map 1/1-8 even
would allow all CPUs.
While such configs are very unlikely to ever be met (which is why this
bug is tagged minor), this is becoming quite more visible while testing
automatic CPU binding during 2.9 development because due to this bug
it's much more common to end up with incorrect bindings.
This patch fixes it by simply removing the .proc entry from cpu_map and
always setting all threads' maps. The process is no longer arbitrarily
bound to the group 1's mask, but in case threads are disabled, we'll
use thread 1's mask since it contains the configured CPUs.
This fix should be backported at least to 2.6, but no need to insist if
it resists as it's easier to break cpu-map than to fix an unlikely issue.
BUG/MINOR: config: do not detect NUMA topology when cpu-map is configured
As documented, the NUMA auto-detection is not supposed to be used when
the CPU affinity was set either by taskset (already checked) or by a
cpu-map directive. However this check was missing, so that configs
having cpu-map entries would still first bind to a single node. In
practice it has no impact on correct configs since bindings will be
replaced. However for those where the cpu-map directive are not
exhaustive it will have the impact of binding those threads to one node,
which disagrees with the doc (and makes future evolutions significantly
more complicated).
This could be backported to 2.4 where numa-cpu-mapping was added, though
if nobody encountered this by then maybe we should only focus on recent
versions that are more NUMA-friendly (e.g. 2.8 only). This patch depends
on this previous commit that brings the function we rely on:
MINOR: cpuset: add cpu_map_configured() to know if a cpu-map was found
MINOR: cpuset: add cpu_map_configured() to know if a cpu-map was found
Since we'll soon want to adjust the "thread-groups" degree of freedom
based on the presence of cpu-map, we first need to be able to detect
if cpu-map was used. This function scans all cpu-map sets to detect if
any is present, and returns true accordingly.
BUG/MINOR: h1-htx: Return the right reason for 302 FCGI responses
A FCGI response may contain a "Location" header with no status code. In this
case a 302-Found HTTP response must be returned to the client. However,
while the status code is indeed 302, the reason is wrong. "Found" must be
set instead of "Moved Temporarily".
This patch must be backported as far as 2.2. With the commit e3e4e0006
("BUG/MINOR: http: Return the right reason for 302"), this should fix the
issue #2208.
Calling lual_newstate(Init main lua stack) in the hlua_init_state()
function, the return value of lua_newstate() may be NULL (for example
in case of OOM). In this case, L will be NULL, and then crash happens
in lua_getextraspace(). So, we add a check for lua_newstate.
This should be backported at least to 2.4, maybe further.
MEDIUM: quic: Dynamic allocations of QUIC TLS encryption levels
It is possible that haproxy receives a late Initial packet after it has
released its Initial or Handshake encryption levels. In this case
it must not try to retransmit packets from such encryption levels to
speed up the handshake completion.
Mariam John [Mon, 17 Jul 2023 13:22:59 +0000 (08:22 -0500)]
MEDIUM: ssl: new sample fetch method to get curve name
Adds a new sample fetch method to get the curve name used in the
key agreement to enable better observability. In OpenSSLv3, the function
`SSL_get_negotiated_group` returns the NID of the curve and from the NID,
we get the curve name by passing the NID to OBJ_nid2sn. This was not
available in v1.1.1. SSL_get_curve_name(), which returns the curve name
directly was merged into OpenSSL master branch last week but will be available
only in its next release.
Because of a cut/paste error, the wrong reason was returned for 302
code. The 301 reason was returned instead. Thus now, "Found" is returned for
302, instead of "Moved Permanently".
This pathc should fix the issue 2208. It must be backported to all stable
versions.
BUG/MINOR: sample: Fix wrong overflow detection in add/sub conveters
When "add" or "sub" conveters are used, an overflow detection is performed.
When 2 negative integers are added (or a positive integer is substracted to
a positive one), we take care to not exceed the low limit (LLONG_MIN) and
when 2 positive integers are added, we take care to not exceed the high
limit (LLONG_MAX).
However, because of a missing 'else' statement, if there is no overflow in
the first case, we fall back on the second check (the one for positive adds)
and LLONG_MAX is returned. It means that most of time, when 2 negative
integers are added (or a positive integer is substracted to a negative one),
LLONG_MAX is returned.
This patch should solve the issue #2216. It must be backported to all stable
versions.
BUG/MEDIUM: hlua_fcn/queue: bad pop_wait sequencing
I assumed that the hlua_yieldk() function used in queue:pop_wait()
function would eventually return when the continuation function would
return.
But this is wrong, the continuation function is simply called back by the
resume after the hlua_yieldk() which does not return in this case. The
caller is no longer the initial calling function, but Lua, so when the
continuation function eventually returns, it does not give the hand back
to the C calling function (queue:pop_wait()), like we're used to, but
directly to Lua which will continue the normal execution of the (Lua)
function that triggered the C-function, effectively bypassing the end
of the C calling function.
Because of this, the queue waiting list cleanup never occurs!
This causes some undesirable effects:
- pop_wait() will slowly leak over the time, because the allocated queue
waiting entry never gets deallocated when the function is finished
- queue:push() will become slower and slower because the wait list will
keep growing indefinitely as a result of the previous leak
- the task that performed at least 1 pop_wait() could suffer from
useless wakeups because it will stay indefinitely in the queue waiting
list, so every queue:push() will try to wake the task, even if the
task is not waiting for new queue items.
- last but not least, if the task that performed at least 1 pop_wait ends
or crashes, the next queue:push() will lead to invalid reads and
process crash because it will try to wakeup a ghost task that doesn't
exist anymore.
To fix this, the pop_wait function was reworked with the assumption that
the hlua_yieldk() with continuation function never returns. Indeed, it is
now the continuation function that will take care of the cleanup, instead
of the parent function.
This must be backported in 2.8 with 86fb22c5 ("MINOR: hlua_fcn: add Queue class")
BUG/MEDIUM: quic: timestamp shared in token was using internal time clock
The internal tick clock was used to export the timestamp int the token
on retry packets. Doing this in cluster mode the nodes don't
understand the timestamp from tokens generated by others.
This patch re-work this using the the real current date (wall-clock time).
Timestamp are also now considered in secondes instead of milleseconds.
BUG/MEDIUM: quic: missing check of dcid for init pkt including a token
RFC 9000, 17.2.5.1:
"The client MUST use the value from the Source Connection ID
field of the Retry packet in the Destination Connection ID
field of subsequent packets that it sends."
There was no control of this and we could accept a different
dcid on init packets containing a valid token.
The randomized value used as new scid on retry packets is now
added in the aad used to encode the token. This way the token
will appear as invalid if the dcid missmatch the scid of
the previous retry packet.
BUG/MINOR: quic: retry token remove one useless intermediate expand
According to rfc 5869 about hkdf, extract function returns a
pseudo random key usable to perform expand using labels to derive keys.
So the intermediate expand on a label is useless, the key should be strong
enought using only one expand.
BUG/MEDIUM: quic: token IV was not computed using a strong secret
Computing the token key and IV, a stronger derived key was used
to compute the key but the weak secret was still used to compute
the IV. This could be used to found the secret.
This patch fix this using the same derived key than the one used
to compute the token key.
Thierry Fournier [Tue, 23 May 2023 16:00:46 +0000 (18:00 +0200)]
BUG/MINOR: config: Lenient port configuration parsing
Configuration parsing allow port like 8000/websocket/. This is
a nonsense and allowing this syntax may hide to the user something
not corresponding to its intent.
This patch should not be backported because it could break existing
configurations
BUG/MINOR: hlua_fcn/queue: use atomic load to fetch queue size
In hlua_queue_size(), queue size is loaded as a regular int, but the
queue might be shared by multiple threads that could perform some
atomic pushing or popping attempts in parallel, so we better use an
atomic load operation to guarantee consistent readings.
EXAMPLES: maintain haproxy 2.8 retrocompatibility for lua mailers script
Because of b58bd97 ("MINOR: hlua_fcn/mailers: handle timeout mail from
mailers section"), latest examples/lua/mailers.lua file wouldn't work
if loaded with haproxy < 2.8.2 (which is not yet released), unless it
is manually recompiled from the latest code sources.
But this can be an issue given that direct URL may be referenced in
guides/tutorials as download link for the the script, independently
from the related haproxy source code revision.
ie: http://git.haproxy.org/?p=haproxy-2.8.git;a=blob_plain;f=examples/lua/mailers.lua;hb=HEAD
Thus, in order to make examples/lua/mailers.lua work with previous 2.8
versions, we ensure that the script stays compatible with pre-b58bd97
code by adding extra checks in the script.
BUG/MINOR: sink/log: properly deinit srv in sink_new_from_logsrv()
When errors are encountered in sink_new_from_logsrv() function,
incompetely allocated ressources are freed to prevent memory leaks.
For instance: logsrv implicit server is manually cleaned up on error prior
to returning from the function.
However, since 198e92a8e5 ("MINOR: server: add a global list of all known
servers") every server created using new_server() is registered to the
global list, but unfortunately the manual srv cleanup in
sink_new_from_logsrv() doesn't remove the srv from the global list, so the
freed server will still be referenced there, which can result in invalid
reads later.
Moreover, server API has evolved since, and now the srv_drop() function is
available for that purpose, so let's use it, but make sure that srv is
freed before the proxy because on older versions srv_drop() expects the
srv to be linked to a valid proxy pointer.
This must be backported up to 2.4.
[For 2.4 version, free_server() must be used instead of srv_drop()]
BUG/MINOR: server: set rid default value in new_server()
srv->rid default value is set in _srv_parse_init() after the server is
succesfully allocated using new_server().
This is wrong because new_server() can be used independently so rid value
assignment would be skipped in this case.
Hopefully new_server() allocates server data using calloc() so srv->rid
is already set to 0 in practise. But if calloc() is replaced by malloc()
or other memory allocating function that doesn't zero-initialize srv
members, this could lead to rid being uninitialized in some cases.
This should be backported in 2.8 with 61e3894dfe ("MINOR: server: add
srv->rid (revision id) value")
BUG/MINOR: sink: invalid sft free in sink_deinit()
sft freeing attempt made in a575421 ("BUG/MINOR: sink: missing sft free in
sink_deinit()") is incomplete, because sink->sft is meant to be used as a
list and not a single sft entry.
Because of that, the previous fix only frees the first sft entry, which
fixes memory leaks for single-server forwarders (this is the case for
implicit rings), but could still result in memory leaks when multiple
servers are configured in a explicit ring sections.
What this patch does: instead of directly freeing sink->sft, it iterates
over every list members to free them.
BUG/MINOR: log: free errmsg on error in cfg_parse_log_forward()
When leaving cfg_parse_log_forward() on error paths, errmsg which is local
to the function could still point to valid data, and it's our
responsibility to free it.
Instead of freeing it everywhere it is invoved, we free it prior to
leaving the function.
BUG/MINOR: log: fix multiple error paths in cfg_parse_log_forward()
Multiple error paths were badly handled in cfg_parse_log_forward():
some errors were raised without interrupting the function execution,
resulting in undefined behavior.
Instead of fixing issues separately, let's fix the whole function at once.
This should be backported as far as 2.4.
BUG/MEDIUM: log: improper use of logsrv->maxlen for buffer targets
In e709e1e ("MEDIUM: logs: buffer targets now rely on new sink_write")
we started using the sink API instead of using the ring_write function
directly.
But as indicated in the commit message, the maxlen parameter of the log
directive now only applies to the message part and not the complete
payload. I don't know what the original intent was (maybe minimizing code
changes) but it seems wrong, because the doc doesn't mention this special
case, and the result is that the ring->buffer output can differ from all
other log output types, making it very confusing.
One last issue with this is that log messages can end up being dropped at
runtime, only for the buffer target, and even if logsrv->maxlen is
correctly set (including default: 1024) because depending on the generated
header size the payload can grow bigger than the accepted sink size (sink
maxlen is not mandatory) and we have no simple way to detect this at
configuration time.
TARGET_BUFFER still leverages the proper sink API, but thanks to
"MINOR: sink: pass explicit maxlen parameter to sink_write()" we now
explicitly pass the logsrv->maxlen to the sink_write function in order
to stop writing as soon as either sink->maxlen or logsrv->maxlen is
reached.
This restores pre-e709e1e behavior with the added benefit from using the
high-level API, which includes automatically announcing dropped message
events.
Then, we also need to take the ending '\n' into account: it is not
explicitly set when generating the logline for TARGET_BUFFER, but it will
be forcefully added by the sink_forward_io_handler function from the tcp
handler applet when log messages from the buffer are forwarded to tcp
endpoints.
In current form, because the '\n' is added later in the chain, maxlen is
not being considered anymore, so the final log message could exceed maxlen
by 1 byte, which could make receiving servers unhappy in logging context.
To prevent this, we sacrifice 1 byte from the logsrv->maxlen to ensure
that the final message will never exceed log->maxlen, even if the '\n'
char is automatically appended later by the forwarding applet.
Thanks to this change TCP (over RING/BUFFER) target now behaves like
FD and UDP targets.
This commit depends on:
- "MINOR: sink: pass explicit maxlen parameter to sink_write()"
It may be backported as far as 2.2
[For 2.2 and 2.4 the patch does not apply automatically, the sink_write()
call must be updated by hand]
MINOR: sink/api: pass explicit maxlen parameter to sink_write()
sink_write() currently relies on sink->maxlen to know when to stop
writing a given payload.
But it could be useful to pass a smaller, explicit value to sink_write()
to stop before the ring maxlen, for instance if the ring is shared between
multiple feeders.
sink_write() now takes an optional maxlen parameter:
if maxlen is > 0, then sink_write will stop writing at maxlen if maxlen
is smaller than ring->maxlen, else only ring->maxlen will be considered.
[for haproxy <= 2.7, patch must be applied by hand: that is:
__sink_write() and sink_write() should be patched to take maxlen into
account and function calls to sink_write() should use 0 as second argument
to keep original behavior]
A regression was introduced with 5464885 ("MEDIUM: log/sink: re-work
and merge of build message API.").
For UDP targets, a final '\n' is systematically inserted, but with the
rework of the build message API, it is inserted after the maxlen
limitation has been enforced, so this can lead to the final message
becoming maxlen+1. For strict syslog servers that only accept up to
maxlen characters, this could be a problem.
To fix the regression, we take the final '\n' into account prior to
building the message, like it was done before the rework of the API.
BUG/MINOR: ring: size warning incorrectly reported as fatal error
When a ring section defines its size using the "size" directive with a
smaller size than the default one or smaller than the previous one,
a warning is generated to inform the user that the new size will be
ignored.
However the err_code is returned as FATAL, so this cause haproxy to
incorrectly abort the init sequence.
Changing the err_code to ERR_WARN so that this warning doesn't refrain
from successfully starting the process.
BUG/MINOR: sink: missing sft free in sink_deinit()
Adding missing free for sft (string_forward_target) in sink_deinit(),
which resulted in minor leak for each declared ring target at deinit().
(either explicit and implicit rings are affected)
BUG/MINOR: http_ext: unhandled ERR_ABORT in proxy_http_parse_7239()
_proxy_http_parse_7239_expr() helper used in proxy_http_parse_7239()
function may return ERR_ABORT in case of memory error. But the error check
used below is insufficient to catch ERR_ABORT so the function could keep
executing prior to returning ERR_ABORT, which may cause undefined
behavior. Hopefully no sensitive handling is performed in this case so
this bug has very limited impact, but let's fix it anyway.
We now use ERR_CODE mask instead of ERR_FATAL to check if err_code is set
to any kind of error combination that should prevent the function from
further executing.
This may be backported in 2.8 with b2bb9257d2 ("MINOR: proxy/http_ext:
introduce proxy forwarded option")
BUG/MEDIUM: sink: invalid server list in sink_new_from_logsrv()
forward proxy server list created from sink_new_from_logsrv() is invalid
Indeed, srv->next is literally assigned to itself. This did not cause
issues during syslog handling because the sft was properly set, but it
will cause the free_proxy(sink->forward_px) at deinit to go wild since
free_proxy() will try to iterate through the proxy srv list to free
ressources, but because of the improper list initialization, double-free
and infinite-loop will occur.
This bug was revealed by 9b1d15f53a ("BUG/MINOR: sink: free forward_px on deinit()")
BUG/MINOR: cache: A 'max-age=0' cache-control directive can be overriden by a s-maxage
When a s-maxage cache-control directive is present, it overrides any
other max-age or expires value (see section 5.2.2.9 of RFC7234). So if
we have a max-age=0 alongside a strictly positive s-maxage, the response
should be cached.
This bug was raised in GitHub issue #2203.
The fix can be backported to all stable branches.
config : parsing [test.conf:XX] : error detected in frontend 'fe' while parsing 'http-request set-var(txn.test)' rule : converter 'ipmask' cannot be applied.
With [2], we get:
config : parsing [test.conf:XX] : error detected in frontend 'fe' while parsing 'http-request redirect' rule : error in condition: converter 'ipmask' cannot be applied in ACL expression 'bool(true),ipmask(32)'.
Now consider the following examples which are based on [1] and [2]
but with the debug() sample conv inserted in-between those incompatible
sample types:
According to the documentation, "it is safe to insert the debug converter
anywhere in a chain, even with non-printable sample types".
Thus we don't expect any side-effect from using it within a chain.
However in current implementation, because of debug() returning SMP_T_ANY
type which is a pseudo type (only resolved at runtime), the sample
compatibility checks performed at parsing time are completely uneffective.
(haproxy will start and no warning will be emitted)
The indesirable effect of this is that debug() prevents haproxy from
warning you about impossible type conversions, hiding potential errors
in the configuration that could result to unexpected evaluation or logic
while serving live traffic. We better let haproxy warn you about this kind
of errors when it has the chance.
With our previous examples, this could cause some inconveniences. Let's
say for example that you are testing a configuration prior to deploying
it. When testing the config you might want to use debug() converter from
time to time to check how the conversion chain behaves.
Now after deploying the exact same conf, you might want to remove those
testing debug() statements since they are no longer relevant.. but
removing them could "break" the config and suddenly prevent haproxy from
starting upon reload/restart. (this is the expected behavior, but it
comes a bit too late because of debug() hiding errors during tests..)
To fix this, we introduce a new output type for sample expressions:
SMP_T_SAME - may only be used as "expected" out_type (parsing time)
for sample converters.
As it name implies, it is a way for the developpers to indicate that the
resulting converter's output type is guaranteed to match the type of the
sample that is presented on the converter's input side.
(converter may alter data content, but data type must not be changed)
What it does is that it tells haproxy that if switching to the converter
(by looking at the converter's input only, since outype is SAME) is
conversion-free, then the converter type can safely be ignored for type
compatibility checks within the chain.
debug()'s out_type is thus set to SMP_T_SAME instead of ANY, which allows
it to fully comply with the doc in the sense that it does not impact the
conversion chain when inserted between sample items.
MEDIUM: tree-wide: fetches that may return IPV4+IPV6 now return ADDR
Historically, the ADDR pseudo-type did not exist. So when IPV6 support
was added to existing IPV4 sample fetches (e.g.: src,dst,hdr_ip...) the
expected out_type in related sample definitions was left on IPV4 because
it was required to declare the out_type as the lowest common denominator
(the type that can be casted into all other ones) to make compatibility
checks at parse time work properly.
However, now that ADDR pseudo-type may safely be used as out_type since
("MEDIUM: sample: add missing ADDR=>? compatibility matrix entries"), we
can use ADDR for fetches that may output both IPV4 and IPV6 at runtime.
One added benefit on top of making the code less confusing is that
'haproxy -dKsmp' output will now show "addr" instead of "ipv4" for such
fetches, so the 'haproxy -dKsmp' output better complies with the fetches
signatures from the documentation.
out_ip fetch, which returns an ip according to the doc, was purposely
left as is (returning IPV4) since the smp_fetch_url_ip() implementation
forces output type to IPV4 anyway, and since this is an historical fetch
I prefer not to touch it to prevent any regression. However if
smp_fetch_url_ip() were to be fixed to also return IPV6 in the future,
then its expected out_type may be changed to ADDR as well.
Multiple notes in the code were updated to mention that the appropriate
pseudo-type may be used instead of the lowest common denominator for
out_type when available.
Since 1478aa7 ("MEDIUM: sample: Add IPv6 support to the ipmask converter")
ipmask converter may return IPV6 as well, so the sample definition must
be updated. (the output type is currently explicited to IPV4)
This has no functional impact: one visible effect will be that
converter's visible output type will change from "ipv4" to "addr" when
using 'haproxy -dKcnv'.
SMP_T_ADDR support was added in b805f71 ("MEDIUM: sample: let the cast
functions set their output type").
According to the above commit, it is made clear that the ADDR type is
a pseudo/generic type that may be used for compatibility checks but
that cannot be emitted from a fetch or converter.
With that in mind, all conversions from ADDR to other types were
explicitly disabled in the compatibility matrix.
But later, when map_*_ip functions were updated in b2f8f08 ("MINOR: map:
The map can return IPv4 and IPv6"), we started using ADDR as "expected"
output type for converters. This still complies with the original
description from b805f71, because it is used as the theoric output
type, and is never emitted from the converters themselves (only "real"
types such as IPV4 or IPV6 are actually being emitted at runtime).
But this introduced an ambiguity as well as a few bugs, because some
compatibility checks are being performed at config parse time, and thus
rely on the expected output type to check if the conversion from current
element to the next element in the chain is theorically supported.
However, because the compatibility matrix doesn't support ADDR to other
types it is currently impossible to use map_*_ip converters in the middle
of a chain (the only supported usage is when map_*_ip converters are
at the very end of the chain).
To illustrate this, consider the following examples:
acl test str(ok),map_str_ip(test.map) -m found # this will work
acl test str(ok),map_str_ip(test.map),ipmask(24) -m found # this will raise an error
Likewise, stktable_compatible_sample() check for stick tables also relies
on out_type[table_type] compatibility check, so map_*_ip cannot be used
with sticktables at the moment:
backend st_test
stick-table type string size 1m expire 10m store http_req_rate(10m)
frontend fe
bind localhost:8080
mode http
http-request track-sc0 str(test),map_str_ip(test.map) table st_test # raises an error
To fix this, and prevent future usage of ADDR as expected output type
(for either fetches or converters) from introducing new bugs, the
ADDR=>? part of the matrix should follow the ANY type logic.
That is, ADDR, which is a pseudo-type, must be compatible with itself,
and where IPV4 and IPV6 both support a backward conversion to a given
type, ADDR must support it as well. It is done by setting the relevant
ADDR entries to c_pseudo() in the compatibility matrix to indicate that
the operation is theorically supported (c_pseudo() will never be executed
because ADDR should not be emitted: this only serves as a hint for
compatibility checks at parse time).
This is what's being done in this commit, thanks to this the broken
examples documented above should work as expected now, and future
usage of ADDR as out_type should not cause any issue.
This function is used for ANY=>!ANY conversions in the compatibility
matrix to help differentiate between real NOOP (c_none) and pseudo
conversions that are theorically supported at config parse time but can
never occur at runtime,. That is, to explicit the fact that actual related
runtime operations (e.g.: ANY->IPV4) are not NOOP since they might require
some conversion to be performed depending on the input type.
When checking the conf we don't know the effective out types so
cast[pseudo type][pseudo type] is allowed in the compatibility matrix,
but at runtime we only expect cast[real type][(real type || pseudo type)]
because fetches and converters may not emit pseudo types, thus using
c_none() everywhere was too ambiguous.
The process will crash if c_pseudo() is invoked to help catch bugs:
crashing here means that a pseudo type has been encountered on a
converter's input at runtime (because it was emitted earlier in the
chain), which is not supported and results from a broken sample fetch
or converter implementation. (pseudo types may only be used as out_type
in sample definitions for compatibility checks at parsing time)
MEDIUM: acl/sample: unify sample conv parsing in a single function
Both sample_parse_expr() and parse_acl_expr() implement some code
logic to parse sample conv list after respective fetch or acl keyword.
(Seems like the acl one was inspired by the sample one historically)
But there is clearly code duplication between the two functions, making
them hard to maintain.
Hopefully, the parsing logic between them has stayed pretty much the
same, thus the sample conv parsing part may be moved in a dedicated
helper parsing function.
This is what's being done in this commit, we're adding the new function
sample_parse_expr_cnv() which does a single thing: parse the converters
that are listed right after a sample fetch keyword and inject them into
an already existing sample expression.
Both sample_parse_expr() and parse_acl_expr() were adapted to now make
use of this specific parsing function and duplicated code parts were
cleaned up.
Although sample_parse_expr() remains quite complicated (numerous function
arguments due to contextual parsing data) the first goal was to get rid of
code duplication without impacting the current behavior, with the added
benefit that it may allow further code cleanups / simplification in the
future.
DOC: ssl: Add ocsp-update troubleshooting clues and emphasize on crt-list only aspect
The current limitation of the 'ocsp-update' option and the fact that it
can only be used in crt-lists was puzzling for some people so the doc
was amended to emphasize this specificity. A configuration extract was
added as well.
A few troubleshooting clues were added as well.
BUILD: quic: Compilation fixes for some gcc warnings with -O1
This is to prevent the build from these gcc warning when compiling with -O1 option:
willy@wtap:haproxy$ sh make-native-quic-memstat src/quic_conn.o CPU_CFLAGS="-O1"
CC src/quic_conn.o
src/quic_conn.c: In function 'qc_prep_pkts':
src/quic_conn.c:3700:44: warning: potential null pointer dereference [-Wnull-dereference]
3700 | frms = &qel->pktns->tx.frms;
| ~~~^~~~~~~
src/quic_conn.c:3626:33: warning: 'end' may be used uninitialized in this function [-Wmaybe-uninitialized]
3626 | if (end - pos < QUIC_INITIAL_PACKET_MINLEN) {
| ~~~~^~~~~
projects / thirdparty / haproxy.git / log
