Commit Graph

18453 Commits

Author SHA1 Message Date
Amaury Denoyelle
160edadb83 BUG/MINOR: quic: fix buffer overflow on retry token generation
When generating a Retry token, client CID is used as encryption input.
The client must reuse the same CID when emitting the token in a new
Initial packet.

A memory overflow can occur on quic_generate_retry_token() depending on
the size of client CID. This is because space reserved for <aad> only
accounted for QUIC_HAP_CID_LEN (size of haproxy owned generated CID).
However, the client CID size only depends on client parameter and is
instead limited to QUIC_CID_MAXLEN as specified in RFC9000.

This was reproduced with ngtcp2 and haproxy built with ASAN. Here is the error
log :
  ==14964==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fffee228cee at pc 0x7ffff785f427 bp 0x7fffee2289e0 sp 0x7fffee228188
  WRITE of size 17 at 0x7fffee228cee thread T5
      #0 0x7ffff785f426 in __interceptor_memcpy /usr/src/debug/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:827
      #1 0x555555906ea7 in quic_generate_retry_token_aad src/quic_conn.c:5452
      #2 0x555555907e72 in quic_retry_token_check src/quic_conn.c:5577
      #3 0x55555590d01e in qc_lstnr_pkt_rcv src/quic_conn.c:6103
      #4 0x5555559190fa in quic_lstnr_dghdlr src/quic_conn.c:7179
      #5 0x555555eb0abf in run_tasks_from_lists src/task.c:590
      #6 0x555555eb285f in process_runnable_tasks src/task.c:855
      #7 0x555555d9118f in run_poll_loop src/haproxy.c:2853
      #8 0x555555d91f88 in run_thread_poll_loop src/haproxy.c:3042
      #9 0x7ffff709f8fc  (/usr/lib/libc.so.6+0x868fc)
      #10 0x7ffff7121a5f  (/usr/lib/libc.so.6+0x108a5f)

This must be backported up to 2.6.

(cherry picked from commit 6c940569f6)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:49:18 +02:00
Frédéric Lécaille
c387bf149b BUILD: quic: Fix build for m68k cross-compilation
Fix several warinings as this one:

src/qmux_trace.c:80:45: error: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 4 has type ‘uint64_t’ {aka ‘const long long unsigned int’} [-Werror=format=]
   80 |    chunk_appendf(&trace_buf, " qcs=%p .id=%lu .st=%s",
      |                                           ~~^
      |                                             |
      |                                             long unsigned int
      |                                           %llu
   81 |                  qcs, qcs->id,
      |                       ~~~~~~~
      |                          |
      |                          uint64_t {aka const long long unsigned int}
compilation terminated due to -Wfatal-errors.

Cast remaining uint64_t variables as ullong with %llu as printf format and size_t
others as ulong with %lu as printf format.

Thank you to Ilya for having reported this issue in GH #1899.

Must be backported to 2.6

(cherry picked from commit ea492e3e47)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:49:14 +02:00
Amaury Denoyelle
8c26e397c2 BUILD: ssl_sock: fix null dereference for QUIC build
A previous commit tries to fix uninitialized GCC warning on ssl code for
QUIC build. See the fix here :
  48e46f98cc
  BUILD: ssl_sock: bind_conf uninitialized in ssl_sock_bind_verifycbk()

However, this is incomplete as it still reports possible NULL
dereference on ctx variable (GCC v12.2.0). Here is the compilation
result :

  src/ssl_sock.c: In function ‘ssl_sock_bind_verifycbk’:
  src/ssl_sock.c:1739:12: error: potential null pointer dereference [-Werror=null-dereference]
   1739 |         ctx->xprt_st |= SSL_SOCK_ST_FL_VERIFY_DONE;
        |

To fix this, remove check on qc which can also never happens and replace
it with a BUG_ON. This seems to satisfy GCC on my machine.

This must be backported up to 2.6.

(cherry picked from commit ba303deadc)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:49:07 +02:00
Thierry Fournier
c4706a113e BUG/MEDIUM: httpclient: segfault when the httpclient parser fails
If the uri is unexpected ("/" in place of "http://xxx/"), some parsing
function fails. The failure is not handled.

This patch handle these errors. Note: the return code is boolean, maybe
we can return more precise error for Lua reporting ?

Must be backported in 2.6.

(cherry picked from commit 74a9eb5216)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:49:03 +02:00
Frédéric Lécaille
c9aeed34a5 BUILD: quic: QUIC mux build fix for 32-bit build
Thank you to Ilya for having reported this issue in GH #1897

Must be backported to 2.6.

(cherry picked from commit 5a5d05c71b)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:47:53 +02:00
Tim Duesterhus
0855bad311 CI: Replace the deprecated ::set-output command by writing to $GITHUB_OUTPUT in workflow definition
See "CI: Replace the deprecated `::set-output` command by writing to
$GITHUB_OUTPUT in matrix.py" for the reasoning behind this commit.

(cherry picked from commit b87ecbb179)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:47:49 +02:00
Tim Duesterhus
7258191d11 CI: Replace the deprecated ::set-output command by writing to $GITHUB_OUTPUT in matrix.py
As announced in

https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/

the `::set-output` command is deprecated, because processes during the workflow
execution might output untrusted information that might include the
`::set-output` command, thus allowing these untrusted information to hijack the
build.

The replacement is writing to the file indicated by the `$GITHUB_OUTPUT`
environment variable.

(cherry picked from commit 8a03bf4052)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:47:43 +02:00
Christopher Faulet
252ac81019 MINOR: httpclient/lua: Don't set req_payload callback if body is empty
The HTTPclient callback req_payload callback is set when a request payload
must be streamed. In the lua, this callback is set when a body is passed as
argument in one of httpclient functions (head/get/post/put/delete). However,
there is no reason to set it if body string is empty.

This patch is related to the issue #1898. It may be backported as far as
2.5.

(cherry picked from commit 380ae9c3ff)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:47:11 +02:00
Christopher Faulet
bb170f4dff BUG/MEDIUM: httpclient: Don't set EOM flag on an empty HTX message
In the HTTP client, when the request body is streamed, at the end of the
payload, we must be sure to not set the EOM flag on an empty message.
Otherwise, because there is no data, the buffer is reset to be released and
the flag is lost. Thus, the HTTP client is never notified of the end of
payload for the request and the applet is blocked. If the HTTP client is
instanciated from a Lua script, it is even worse because we fall into a
wakeup loop between the lua script and the HTTP client applet. At the end,
HAProxy is killed because of the watchdog.

This patch should fix the issue #1898. It must be backported to 2.6.

(cherry picked from commit 48005de17c)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:47:05 +02:00
Frédéric Lécaille
80bb8f8ab5 BUILD: ssl_sock: bind_conf uninitialized in ssl_sock_bind_verifycbk()
Even if this cannot happen, ensure <bind_conf> is initialized in this
function to please some compilers.

Takes the opportunity of this patch to replace an ABORT_NOW() by
a BUG_ON() because if the variable values they test are not initialized,
this is really because there is a bug.

Must be backported to 2.6.

(cherry picked from commit 48e46f98cc)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:46:59 +02:00
Amaury Denoyelle
85c059a307 MINOR: quic: implement datagram cleanup for quic_receiver_buf
Each time data is read on QUIC receiver socket, we try to reuse the
first datagram of the currently used quic_receiver_buf instead of
allocating a new one. This algorithm is suboptimal if there is several
unused datagrams as only the first one is tested and its buffer removed
from quic_receiver_buf.

If QUIC traffic is quite substential, this can lead to an important
number of quic_dgram occurences allocated from pool_head_quic_dgram and
a lack of free space in allocated quic_receiver_buf buffers.

To improve this, each time we want to reuse a datagram, we pop elements
until a non-yet released datagram is found or the list is empty. All
intermediary elements are freed and the last found datagram can be
reused. This operation has been extracted in a dedicated function named
quic_rxbuf_purge_dgrams().

This should improve memory consumption incured by quic_dgram instances under heavy
QUIC traffic. Note that there is still room for improvement as if the
first datagram is still in use, it may block several unused datagram
after him. However this requires to support removal of datagrams out of
order which is currently not possible.

This should be backported up to 2.6.

(cherry picked from commit 91b2305ad7)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:46:15 +02:00
Amaury Denoyelle
6f2acccd90 CLEANUP: quic: improve naming for rxbuf/datagrams handling
QUIC datagrams are read from a random thread. They are then redispatch
to the connection thread according to the first packet DCID. These
operations are implemented through a special buffer designed to avoid
locking.

Refactor this code with the following changes :
* <rxbuf> type is renamed <quic_receiver_buf>. Its list element is also
  renamed to highligh its attach point to a receiver.
* <quic_dgram> and <quic_receiver_buf> definition are moved to
  quic_sock-t.h. This helps to reduce the size of quic_conn-t.h.
* <quic_dgram> list elements are renamed to highlight their attach point
  into a <quic_receiver_buf> and a <quic_dghdlr>.

This should be backported up to 2.6.

(cherry picked from commit 1cba8d60f3)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:46:11 +02:00
Amaury Denoyelle
94cf0e576d CLEANUP: quic: remove unused rxbufs member in receiver
rxbuf is the structure used to store QUIC datagrams and redispatch them
to the connection thread.

Each receiver manages a list of rxbuf. This was stored both as an array
and a mt_list. Currently, only mt_list is needed so removed <rxbufs>
member from receiver structure.

This should be backported up to 2.6.

(cherry picked from commit 8c4d062d25)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:46:08 +02:00
Frédéric Lécaille
118e94d0c0 MINOR: quic: Split the secrets key allocation in two parts
Implement quic_tls_secrets_keys_alloc()/quic_tls_secrets_keys_free() to allocate
the memory for only one direction (RX or TX).
Modify ha_quic_set_encryption_secrets() to call these functions for one of this
direction (or both). So, for now on we can rely on the value of the secret keys
to know if it was derived.
Remove QUIC_FL_TLS_SECRETS_SET flag which is no more useful.
Consequently, the secrets are dumped by the traces only if derived.

Must be backported to 2.6.

(cherry picked from commit e1a49cfd4d)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:46:04 +02:00
Frédéric Lécaille
e0212bcb47 BUG/MINOR: quic: Stalled 0RTT connections with big ClientHello TLS message
This issue was reproduced with -Q picoquic client option to split a big ClientHello
message into two Initial packets and haproxy as server without any knowledged of
any previous ORTT session (restarted after a firt 0RTT session). The ORTT received
packets were removed from their queue when the second Initial packet was parsed,
and the QUIC handshake state never progressed and remained at Initial state.

To avoid such situations, after having treated some Initial packets we always
check if there are ORTT packets to parse and we never remove them from their
queue. This will be done after the hanshake is completed or upon idle timeout
expiration.

Also add more traces to be able to analize the handshake progression.

Tested with ngtcp2 and picoquic

Must be backported to 2.6.

(cherry picked from commit 4aa7d8197a)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:46:01 +02:00
Frédéric Lécaille
c094639d65 MINOR: quic: Use a non-contiguous buffer for RX CRYPTO data
Implement quic_get_ncbuf() to dynamically allocate a new ncbuf to be attached to
any quic_cstream struct which needs such a buffer. Note that there is no quic_cstream
for 0RTT encryption level. quic_free_ncbuf() is added to release the memory
allocated for a non-contiguous buffer.

Modify qc_handle_crypto_frm() to call this function and allocate an ncbuf for
crypto data which are not received in order. The crypto data which are received in
order are not buffered but provide to the TLS stack (calling qc_provide_cdata()).

Modify qc_treat_rx_crypto_frms() which is called after having provided the
in order received crypto data to the TLS stack to provide again the remaining
crypto data which has been buffered, if possible (if they are in order). Each time
buffered CRYPTO data were consumed, we try to release the memory allocated for
the non-contiguous buffer (ncbuf).
Also move rx.crypto.offset quic_enc_level struct member to rx.offset quic_cstream
struct member.

Must be backported to 2.6.

(cherry picked from commit 9f9263ed13)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:45:58 +02:00
Frédéric Lécaille
f05bd4e5bd MINOR: quic: Extract CRYPTO frame parsing from qc_parse_pkt_frms()
Implement qc_handle_crypto_frm() to parse a CRYPTO frame.

Must be backported to 2.6.

(cherry picked from commit a20c93e6e2)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:45:54 +02:00
Frédéric Lécaille
6b84c17d99 MINOR: quic: New quic_cstream object implementation
Add new quic_cstream struct definition to implement the CRYPTO data stream.
This is a simplication of the qcs object (QUIC streams) for the CRYPTO data
without any information about the flow control. They are not attached to any
tree, but to a QUIC encryption level, one by encryption level except for
the early data encryption level (for 0RTT). A stream descriptor is also allocated
for each CRYPTO data stream.

Must be backported to 2.6

(cherry picked from commit 7e3f7c47e9)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:45:49 +02:00
Willy Tarreau
2f824716d3 BUG/MINOR: server: make sure "show servers state" hides private bits
In the past we've seen "show servers state" dump some internal bits for
the check states, that were causing regtests to fail. The relevant bits
have been added to the doc to fix the public API and make sure they do
not change by accident, but the output doesn't take care of masking the
undesired ones, causing regtests (and possibly user programs) to fail
when new bits are added. Let's add the mask for the only documented ones
(0x0F for check and 0x1F for agent respectively).

This could be backported wherever the server state is present, though
there's a tiny risk that some undocumented bits might have already
leaked to some user scripts, so it might be wise to wait a bit before
doing that or even not to backport too far.

(cherry picked from commit 99521abd59)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:45:28 +02:00
Willy Tarreau
603c54d9fa BUG/MAJOR: stick-tables: do not try to index a server name for applets
Since commit 03cdf55e6 ("MINOR: stream: Stickiness server lookup by name.")
in 2.0-dev6, server names may be used instead of their IDs, in order to
perform stickiness. However the commit above may end up trying to insert
an empty server name in the dictionary when the server is an applet
instead, resulting in an immediate segfault. This is typically what
happens when a "stick-store" rule is present in a backend featuring a
"stats" directive. As there doesn't seem to be an easy way around it,
it seems to imply that "stick-store" is not much used anymore.

The solution here is to only try to insert non-null keys into the
dictionary. The patch moves the check of the key type before the
first lock so that the test on the key can be performed under the lock
instead of locking twice (the patch is more readable with diff -b).

Note that before 2.4, there's no <key> variable there as it was
introduced by commit 92149f9a8 ("MEDIUM: stick-tables: Add srvkey
option to stick-table"), but the __objt_server(s->target)->id still
needs to be tested.

This needs to be backported as far as 2.0.

(cherry picked from commit bc7c207f74)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:43:15 +02:00
Aurelien DARRAGON
6e7ac7cf7d DOC: configuration: missing 'if' in tcp-request content example
An example given for tcp-request content rule with lua
was missing 'if' keyword. Using it "as is" makes haproxy unhappy.

The example was introduced with 579d83b05.
So it may be backported as far as 1.6, but it is a really minor typo.

(cherry picked from commit d49b559a15)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:43:07 +02:00
Amaury Denoyelle
a28fac9d8e BUG/MINOR: quic: set IP_PKTINFO socket option for QUIC receivers only
Move code which activates IP_PKTINFO socket option (or affiliated
options) from sock_inet_bind_receiver() to quic_bind_listener()
function. This change is useful for two reasons :

* first, and the most important one : this activates IP_PKTINFO only for
  QUIC receivers. The previous version impacted all datagram receivers,
  used for example by log-forwarder. This should reduce memory usage for
  these datagram sockets which do not need this option.

* second, USE_QUIC preprocessor statements are removed from
  src/sock_inet.c which clean up the code.

IP_PKTINFO was introduced recently by the following patch :
  97ecc7a8ea (quic-dev/qns)
  MEDIUM: quic: retrieve frontend destination address

For the moment, this does not impact any stable release. However, as
previous patch is scheduled for 2.6 backporting, the current change must
also be backported to the same versions.

(cherry picked from commit 487d04f6d7)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:42:54 +02:00
Willy Tarreau
b088056f3d CLEANUP: quic/receiver: remove the now unused tx_qring list
The tx_qrings[] and tx_qring_list in the receiver are not used
anymore since commit f2476053f ("MINOR: quic: replace custom buf on Tx
by default struct buffer"), the only place where they're referenced
was in quic_alloc_tx_rings_listener(), which by the way implies that
these were not even freed on exit.

Let's just remove them. This should be backported to 2.6 since the
commit above also was.

(cherry picked from commit cab054bbf9)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:42:38 +02:00
Amaury Denoyelle
3701ab7823 MEDIUM: quic: retrieve frontend destination address
Retrieve the frontend destination address for a QUIC connection. This
address is retrieve from the first received datagram and then stored in
the associated quic-conn.

This feature relies on IP_PKTINFO or affiliated flags support on the
socket. This flag is set for each QUIC listeners in
sock_inet_bind_receiver(). To retrieve the destination address,
recvfrom() has been replaced by recvmsg() syscall. This operation and
parsing of msghdr structure has been extracted in a wrapper quic_recv().

This change is useful to finalize the implementation of 'dst' sample
fetch. As such, quic_sock_get_dst() has been edited to return local
address from the quic-conn. As a best effort, if local address is not
available due to kernel non-support of IP_PKTINFO, address of the
listener is returned instead.

This should be backported up to 2.6.

(cherry picked from commit 97ecc7a8ea)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:42:27 +02:00
Willy Tarreau
d513bcc5d1 BUG/MEDIUM: config: count line arguments without dereferencing the output
Previous commit 8a6767d26 ("BUG/MINOR: config: don't count trailing spaces
as empty arg (v2)") was still not enough. As reported by ClusterFuzz in
issue 52049 (https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=52049),
there remains a case where for the sake of reporting the correct argument
count, the function may produce virtual args that span beyond the end of
the output buffer if that one is too short. That's what's happening with
a config file of one empty line followed by a large number of args.

This means that what args[] points to cannot be relied on and that a
different approach is needed. Since no output is produced for spaces and
comments, we know that args[arg] continues to point to out+outpos as long
as only comments or spaces are found, which is what we're interested in.

As such it's safe to check the last arg's pointer against the one before
the trailing zero was emitted, in order to decide to count one final arg.

No backport is needed, unless the commit above is backported.

(cherry picked from commit 94ab139266)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:41:58 +02:00
Erwan Le Goas
46fd5a187d BUG/MINOR: config: don't count trailing spaces as empty arg (v2)
In parse_line(), spaces increment the arg count and it is incremented
again on '#' or end of line, resulting in an extra empty arg at the
end of arg's list. The visible effect is that the reported arg count
is in excess of 1. It doesn't seem to affect regular function but
specialized ones like anonymisation depends on this count.

This is the second attempt for this problem, here the explanation :

When called for the first line, no <out> was allocated yet so it's NULL,
letting the caller realloc a larger line if needed. However the words are
parsed and their respective args[arg] are filled with out+position, which
means that while the first arg is NULL, the other ones are no and fail the
test that was meant to avoid dereferencing a NULL. Let's simply check <out>
instead of <args> since the latter is always derived from the former and
cannot be NULL without the former also being.

This may need to be backported to stable versions.

(cherry picked from commit 8a6767d266)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:41:54 +02:00
wrightlaw
3593db1a39 BUG/MINOR: smtpchk: SMTP Service check should gracefully close SMTP transaction
At present option smtpchk closes the TCP connection abruptly on completion of service checking,
even if successful. This can result in a very high volume of errors in backend SMTP server logs.
This patch ensures an SMTP QUIT is sent and a positive 2xx response is received from the SMTP
server prior to disconnection.

This patch depends on the following one:

 * MINOR: smtpchk: Update expect rule to fully match replies to EHLO commands

This patch should fix the issue #1812. It may be backported as far as 2.2
with the commit above On the 2.2, proxy_parse_smtpchk_opt() function is
located in src/check.c

[cf: I updated reg-tests script accordingly]

(cherry picked from commit 9a8d8a3fd0)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-10 08:45:06 +02:00
Christopher Faulet
9d21c53f63 MINOR: smtpchk: Update expect rule to fully match replies to EHLO commands
The response to EHLO command is a multiline reply. However the corresponding
expect rule only match on the first line. For now, it is not an issue. But
to be able to send the QUIT command and gracefully close the connection, we
must be sure to consume the full EHLO reply first.

To do so, the regex has been updated to match all 2xx lines at a time.

(cherry picked from commit 2ec1ffaed0)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-10 08:45:06 +02:00
Christopher Faulet
88316b6b99 BUG/MINOR: mux-h1: Account consumed output data on synchronous connection error
The commit 372b38f935 ("BUG/MEDIUM: mux-h1: Handle connection error after a
synchronous send") introduced a bug. In h1_snd_buf(), consumed data are not
properly accounted if a connection error is detected. Indeed, data are
consumed when the output buffer is filled. But, on connection error, we exit
from the loop without incremented total variable accordingly.

When this happens, this leaves the channel buffer in an inconsistent
state. The buffer may be empty with some output at the channel level.
Because an error is reported, it is harmless. But it is safer to fix this
bug now to avoid any regression in future.

This patch must be backported as far as 2.2.

(cherry picked from commit b0b8e9bbd2)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-10 08:45:06 +02:00
Amaury Denoyelle
3fb33cd62e CLEANUP: quic: fix indentation
Fix some indentation in qc_lstnr_pkt_rcv().

This should be backported up to 2.6.

(cherry picked from commit 90121b3321)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-10 08:43:48 +02:00
Amaury Denoyelle
17d919a749 MINOR: mux-quic: check quic-conn return code on Tx
Inspect return code of qc_send_mux(). If quic-conn layer reports an
error, this will interrupt the current emission process.

This should be backported up to 2.6.

(cherry picked from commit 036cc5d880)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-10 08:43:48 +02:00
Amaury Denoyelle
68f165013c MINOR: quic: limit usage of ssl_sock_ctx in favor of quic_conn
Continue on the cleanup of QUIC stack and components.

quic_conn uses internally a ssl_sock_ctx to handle mandatory TLS QUIC
integration. However, this is merely as a convenience, and it is not
equivalent to stackable ssl xprt layer in the context of HTTP1 or 2.

To better emphasize this, ssl_sock_ctx usage in quic_conn has been
removed wherever it is not necessary : namely in functions not related
to TLS. quic_conn struct now contains its own wait_event for tasklet
quic_conn_io_cb().

This should be backported up to 2.6.

(cherry picked from commit 2ed840015f)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-10 08:43:48 +02:00
Aurelien DARRAGON
692efcc20a BUG/MINOR: hlua: hlua_channel_insert_data() behavior conflicts with documentation
Channel.insert(channel, string, [,offset]):

When no offset is provided, hlua_channel_insert_data() inserts
string at the end of incoming data.

This behavior conflicts with the documentation that explicitly says
that the default behavior is to insert the string in front of incoming data.

This patch fixes hlua_channel_insert_data() behavior so that it fully
complies with the documentation.

Thanks to Smackd0wn for noticing it.

This could be backported to 2.6 and 2.5

(cherry picked from commit afb7dafb44)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-10 08:43:48 +02:00
Willy Tarreau
fb36fd0f01 BUILD: http_fetch: silence an uninitiialized warning with gcc-4/5/6 at -Os
Gcc 4.x, 5.x and 6.x report this when compiling http_fetch.c:

  src/http_fetch.c: In function 'smp_fetch_meth':
  src/http_fetch.c:357:6: warning: 'htx' may be used uninitialized in this function [-Wmaybe-uninitialized]
     sl = http_get_stline(htx);

That's quite weird since there's no such code path, but presetting the
htx variable to NULL during declaration is enough to shut it up.

This may be backported to any version that has dbbdb25f1 ("BUG/MINOR:
http-fetch: Use integer value when possible in "method" sample fetch")
as it's the one that triggered this warning (hence at least 2.0).

(cherry picked from commit 2e2b79d157)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-10 08:43:48 +02:00
Christopher Faulet
7e22d25354 BUG/MINOR: http-fetch: Update method after a prefetch in smp_fetch_meth()
In smp_fetch_meth(), smp_prefetch_htx() function may be called to parse the
HTX message and update the HTTP transaction accordingly. In this case, in
smp_fetch_metch() and on success, we must update "meth" variable. Otherwise,
the variable is still equal to HTTP_METH_OTHER and the string version is
always used instead of the enum for known methods.

This patch must be backported as far as 2.0.

(cherry picked from commit eefcd8a97d)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-10 08:43:48 +02:00
Willy Tarreau
a3b16c2907 MINOR: init: do not try to shrink existing RLIMIT_NOFIlE
As seen in issue #1866, some environments will not allow to change the
current FD limit, and actually we don't need to do it, we only do it as
a byproduct of adjusting the limit to the one that fits. Here we're
replacing calls to setrlimit() with calls to raise_rlim_nofile(), which
will avoid making the setrlimit() syscall in case the desired value is
lower than the current process' one.

This depends on previous commit "MINOR: fd: add a new function to only
raise RLIMIT_NOFILE" and may need to be backported to 2.6, possibly
earlier, depending on users' experience in such environments.

(cherry picked from commit c06557c23b)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-10 08:43:48 +02:00
Willy Tarreau
d22ff05c3e MINOR: fd: add a new function to only raise RLIMIT_NOFILE
In issue #1866 an issue was reported under docker, by which a user cannot
lower the number of FD needed. It looks like a restriction imposed in this
environment, but it results in an error while it ought not have to in the
case of shrinking.

This patch adds a new function raise_rlim_nofile() that takes the desired
new setting, compares it to the current one, and only calls setrlimit() if
one of the values in the new setting is larger than the older one. As such
it will continue to emit warnings and errors in case of failure to raise
the limit but will never shrink it.

This patch is only preliminary to another one, but will have to be
backported where relevant (likely only 2.6).

(cherry picked from commit 922a907926)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-10 08:43:48 +02:00
Willy Tarreau
c83e5448d3 BUILD: h1: silence an initiialized warning with gcc-4.7 and -Os
Building h1.c with gcc-4.7 -Os produces the following warning:

  src/h1.c: In function 'h1_headers_to_hdr_list':
  src/h1.c:1101:36: warning: 'ptr' may be used uninitialized in this function [-Wmaybe-uninitialized]

In fact ptr may be taken from sl.rq.u.ptr which is only initialized after
passing through the relevant states, but gcc doesn't know which states
are visited. Adding an ALREADY_CHECKED() statement there is sufficient to
shut it up and doesn't affect the emitted code.

This may be backported to stable versions to make sure that builds on older
distros and systems is clean.

(cherry picked from commit 55d2e8577e)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-10 08:43:48 +02:00
Olivier Houchard
2075777c35 BUG/MEDIUM: lua: handle stick table implicit arguments right.
In hlua_lua2arg_check(), we allow for the first argument to not be
provided, if it has a type we know, this is true for frontend, backend,
and stick table. However, the stick table code was changed. It used
to be deduced from the proxy, but it is now directly provided in struct
args. So setting the proxy there no longer work, and we have to
explicitely set the stick table.
Not doing so will lead the code do use the proxy pointer as a stick
table pointer, which will likely cause crashes.

This should be backported up to 2.0.

(cherry picked from commit 14f6268883)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-10 08:43:48 +02:00
Olivier Houchard
bb0cb68fe6 BUG/MEDIUM: lua: Don't crash in hlua_lua2arg_check on failure
In hlua_lua2arg_check(), on failure, before calling free_argp(), make
sure to always mark the failed argument as ARGT_STOP. We only want to
free argument prior to that point, because we did not allocate the
strings after this one, and so we don't want to free them.

This should be backported up to 2.2.

(cherry picked from commit ca43161a8d)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-10 08:43:48 +02:00
Amaury Denoyelle
114974dbc4 BUG/MINOR: mux-quic: ignore STOP_SENDING for locally closed stream
It is possible to receive a STOP_SENDING frame for a locally closed
stream. This was not properly managed as this would result in a BUG_ON()
crash from qcs_idle_open() call under qcc_recv_stop_sending().

Now, STOP_SENDING frames are ignored when received on streams already
locally closed. This has two consequences depending on the reason of
closure :

* if a RESET_STREAM was already emitted and closed the stream, this
  patch prevents to emit a new RESET_STREAM. This behavior is thus
  better.

* if stream was closed due to all data transmitted, no RESET_STREAM will
  be built. This is contrary to the RFC 9000 which advice to transmit
  it, even on "Data Sent" state. However, this is not mandatory so the
  new behavior is acceptable, even if it could be improved.

This crash has been detected on haproxy.org. This can be artifically
reproduced by adding the following snippet at the end of qc_send_mux()
when doing a request with a small payload response :
  qcc_recv_stop_sending(qc->qcc, 0, 0);

This must be backported up to 2.6.

(cherry picked from commit d7755375a5)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-10 08:43:48 +02:00
Amaury Denoyelle
4a6be622b2 CLEANUP: quic: create a dedicated quic_conn module
xprt_quic module was too large and did not reflect the true architecture
by contrast to the other protocols in haproxy.

Extract code related to XPRT layer and keep it under xprt_quic module.
This code should only contains a simple API to communicate between QUIC
lower layer and connection/MUX.

The vast majority of the code has been moved into a new module named
quic_conn. This module is responsible to the implementation of QUIC
lower layer. Conceptually, it overlaps with TCP kernel implementation
when comparing QUIC and HTTP1/2 stacks of haproxy.

This should be backported up to 2.6.

(cherry picked from commit 92fa63f735)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-10 08:43:48 +02:00
Amaury Denoyelle
228883ca32 CLEANUP: quic: remove duplicated varint code from xprt_quic.h
There was some identical code between xprt_quic and quic_enc modules.
This concerns helper on QUIC varint type. Keep only the version in
quic_enc file : this should help to reduce dependency on xprt_quic
module.

Note that quic_max_int_by_size() has been removed and is replaced by the
identical quic_max_int().

This should be backported up to 2.6.

(cherry picked from commit a2639383ec)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-10 08:43:48 +02:00
Amaury Denoyelle
6bff24ec61 CLEANUP: quic: remove unused function prototype
Removed hexdump unusued prototype from quic_tls.c.

This should be backported up to 2.6.

(cherry picked from commit ac9bf016bf)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-10 08:43:48 +02:00
Amaury Denoyelle
69baa24752 CLEANUP: quic: fix headers
Clean up quic sources by adjusting headers list included depending
on the actual dependency of each source file.

On some occasion, xprt_quic.h was removed from included list. This is
useful to help reducing the dependency on this single file and cleaning
up QUIC haproxy architecture.

This should be backported up to 2.6.

(cherry picked from commit 5c25dc5bfd)
[cf: Include <haproxy/global.h> from cfgparse-quic.c instead of only
     <haproxy/global-t.h">. On 2.7, it is shipped with "tools.h" (tools.h >
     cli.h > global.h). But not on the 2.6]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-10 08:41:20 +02:00
Amaury Denoyelle
90a008239e BUG/MINOR: quic: adjust quic_tls prototypes
Two prototypes in quic_tls module were not identical to the actual
function definition.

* quic_tls_decrypt2() : the second argument const attribute is not
  present, to be able to use it with EVP_CIPHER_CTX_ctlr(). As a
  consequence of this change, token field of quic_rx_packet is now
  declared as non-const.

* quic_tls_generate_retry_integrity_tag() : the second argument type
  differ between the two. Adjust this by fixing it to as unsigned char
  to match EVP_EncryptUpdate() SSL function.

This situation did not seem to have any visible effect. However, this is
clearly an undefined behavior and should be treated as a bug.

This should be backported up to 2.6.

(cherry picked from commit f3c40f83fb)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-10 07:43:42 +02:00
Amaury Denoyelle
adf910e519 CLEANUP: quic: remove global var definition in quic_tls header
Some variables related to QUIC TLS were defined in a header file : their
definitions are now moved properly in the implementation file, with only
declarations in the header.

This should be backported up to 2.6.

(cherry picked from commit a19bb6f0b2)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-10 07:43:39 +02:00
Amaury Denoyelle
e53c5615da CLEANUP: mux-quic: remove usage of non-standard ull type
ull is a typedef to unsigned long long. It is only defined in
xprt_quic-t.h. Its usage should be limited over time to reduce xprt_quic
dependency over the whole code. It can be replaced by ullong typedef
from compat.h.

For the moment, ull references have been replaced in qmux_trace module.
They were only used for printf format and has been replaced by the true
variable type.

This change is useful to reduce dependencies on xprt_quic in other
files.

This should be backported up to 2.6.

(cherry picked from commit d6922d5471)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-10 07:43:35 +02:00
Christopher Faulet
e8da4211a9 DOC: config: Fix pgsql-check documentation to make user param mandatory
The username is required in the Start-up message. Thus, since the 2.2, when
this health-check was refactored, the user parameter is mandatory. On prior
versions, when no username is provided, no pgsql check is performed but only
a basic tcpcheck.

This patch should be backported as far as 2.2.

(cherry picked from commit 59307b3e4e)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-10 07:43:24 +02:00
Fatih Acar
3b26843205 BUG/MINOR: checks: update pgsql regex on auth packet
This patch adds support to the following authentication methods:

- AUTH_REQ_GSS (7)
- AUTH_REQ_SSPI (9)
- AUTH_REQ_SASL (10)

Note that since AUTH_REQ_SASL allows multiple authentication mechanisms
such as SCRAM-SHA-256 or SCRAM-SHA-256-PLUS, the auth payload length may
vary since the method is sent in plaintext. In order to allow this, the
regex now matches any payload length.

This partially fixes Github issue #1508 since user authentication is
still broken but should restore pre-2.2 behavior.

This should be backported up to 2.2.

Signed-off-by: Fatih Acar <facar@scaleway.com>
(cherry picked from commit 0d6fb7a3eb)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-10 07:43:19 +02:00