]> git.ipfire.org Git - thirdparty/haproxy.git/log
thirdparty/haproxy.git
4 years agoBUG/MINOR: lua: warn when registering action, conv, sf, cli or applet multiple times
Thierry Fournier [Sat, 28 Nov 2020 19:41:07 +0000 (20:41 +0100)] 
BUG/MINOR: lua: warn when registering action, conv, sf, cli or applet multiple times

Lua allows registering multiple sample-fetches, converters, action, cli,
applet/services with the same name. This is absolutely useless since only
the first registration will be used. This patch sends a warning if the case
is encountered.

This pach could be backported until 1.8, with the 3 associated patches:
 - MINOR: actions: Export actions lookup functions
 - MINOR: actions: add a function returning a service pointer from its name
 - MINOR: cli: add a function to look up a CLI service description

4 years agoMINOR: cli: add a function to look up a CLI service description
Thierry Fournier [Sat, 28 Nov 2020 19:10:08 +0000 (20:10 +0100)] 
MINOR: cli: add a function to look up a CLI service description

This function will be useful to check if the keyword is already registered.
Also add a define for the max number of args.

This will be needed by a next patch to fix a bug and will have to be
backported.

4 years agoMINOR: actions: add a function returning a service pointer from its name
Thierry Fournier [Sat, 28 Nov 2020 18:32:14 +0000 (19:32 +0100)] 
MINOR: actions: add a function returning a service pointer from its name

This function simply calls action_lookup() on the private service_keywords,
to look up a service name. This will be used to detect double registration
of a same service from Lua.

This will be needed by a next patch to fix a bug and will have to be
backported.

4 years agoMINOR: actions: Export actions lookup functions
Thierry Fournier [Sat, 28 Nov 2020 16:40:24 +0000 (17:40 +0100)] 
MINOR: actions: Export actions lookup functions

These functions will be useful to check if a keyword is already registered.
This will be needed by a next patch to fix a bug, and will need to be
backported.

4 years agoBUG/MINOR: lua: Some lua init operation are processed unsafe
Thierry Fournier [Sat, 28 Nov 2020 15:08:02 +0000 (16:08 +0100)] 
BUG/MINOR: lua: Some lua init operation are processed unsafe

Operation luaL_openlibs() and lua_prepend path are processed whithout
the safe context, so in case of failure Haproxy aborts or stops without
error message.

This patch could be backported until 1.8

4 years agoBUG/MINOR: lua: Post init register function are not executed beyond the first one
Thierry Fournier [Sat, 28 Nov 2020 10:02:58 +0000 (11:02 +0100)] 
BUG/MINOR: lua: Post init register function are not executed beyond the first one

Just because if the first init is a success we return success in place
of continuing the loop.

This patch could be backported until 1.8

4 years agoBUG/MINOR: lua: lua-load doesn't check its parameters
Thierry Fournier [Sun, 29 Nov 2020 00:06:24 +0000 (01:06 +0100)] 
BUG/MINOR: lua: lua-load doesn't check its parameters

"lua-load" doesn't check if the expected parameter is present. It tries to
open() directly the argument at second position. So if the filename is
omitted, it tries to load an empty filename.

This patch could be backported until 1.8

4 years agoBUG/MINOR: lua: missing "\n" in error message
Thierry Fournier [Sat, 28 Nov 2020 23:55:53 +0000 (00:55 +0100)] 
BUG/MINOR: lua: missing "\n" in error message

Just replace ".n" by "\n"

This could be backported until 1.9, but it is not so important.

4 years agoBUG/MINOR: mux-h2/stats: not all GOAWAY frames are errors
Willy Tarreau [Tue, 1 Dec 2020 09:47:18 +0000 (10:47 +0100)] 
BUG/MINOR: mux-h2/stats: not all GOAWAY frames are errors

The stats on haproxy.org reported ~12k GOAWAY for ~34k connections, with
only 2 protocol errorss. It turns out that the GOAWAY frame counter added
in commit a8879238c ("MINOR: mux-h2: report detected error on stats")
matches a bit too many situations. First it counts those which are not
sent as well as failed retries, second it counts as errors the cases of
attempts to cleanly close, while it's titled "GOAWAY sent on detected
error". Let's address this by moving the counter up one line and excluding
the clean codes.

This can be backported to 2.3.

4 years agoMINOR: mux-h2/trace: add traces at level ERROR for protocol errors
Willy Tarreau [Tue, 1 Dec 2020 09:24:29 +0000 (10:24 +0100)] 
MINOR: mux-h2/trace: add traces at level ERROR for protocol errors

A number of traces could be added, and a few TRACE_PROTO were replaced
with TRACE_ERROR. The goal is to be able to enable error tracing only
to detect anomalies.

It looks like they're mostly correct as they don't seem to strike on
valid H2 traffic but are very verbose on h2spec.

4 years agoMINOR: traces: add a new level "error" below the "user" level
Willy Tarreau [Tue, 1 Dec 2020 08:46:46 +0000 (09:46 +0100)] 
MINOR: traces: add a new level "error" below the "user" level

Sometimes it would be nice to be able to only trace abnormal events such
as protocol errors. Let's add a new "error" level below the "user" level
for this. This will allow to add TRACE_ERROR() at various error points
and only see them.

4 years agoBUG/MINOR: mux-h2/stats: make stream/connection proto errors more accurate
Willy Tarreau [Tue, 1 Dec 2020 09:22:43 +0000 (10:22 +0100)] 
BUG/MINOR: mux-h2/stats: make stream/connection proto errors more accurate

Since commit a8879238c ("MINOR: mux-h2: report detected error on stats")
we now have some error stats on stream/connection level protocol errors,
but some were improperly marked as stream while they're connection, and
2 or 3 relevant ones were missing and have now been added.

This could be backported to 2.3.

4 years agoMINOR: log: Logging HTTP path only with %HPO
Maciej Zdeb [Mon, 30 Nov 2020 18:27:47 +0000 (18:27 +0000)] 
MINOR: log: Logging HTTP path only with %HPO

This patch adds a new logging variable '%HPO' for logging HTTP path only
(without query string) from relative or absolute URI.

For example:
log-format "hpo=%HPO hp=%HP hu=%HU hq=%HQ"

GET /r/1 HTTP/1.1
=>
hpo=/r/1 hp=/r/1 hu=/r/1 hq=

GET /r/2?q=2 HTTP/1.1
=>
hpo=/r/2 hp=/r/2 hu=/r/2?q=2 hq=?q=2

GET http://host/r/3 HTTP/1.1
=>
hpo=/r/3 hp=http://host/r/3 hu=http://host/r/3 hq=

GET http://host/r/4?q=4 HTTP/1.1
=>
hpo=/r/4 hp=http://host/r/4 hu=http://host/r/4?q=4 hq=?q=4

4 years ago[RELEASE] Released version 2.4-dev2 v2.4-dev2
Willy Tarreau [Tue, 1 Dec 2020 07:15:26 +0000 (08:15 +0100)] 
[RELEASE] Released version 2.4-dev2

Released version 2.4-dev2 with the following main changes :
    - BUILD: Make DEBUG part of .build_opts
    - BUILD: Show the value of DEBUG= in haproxy -vv
    - CI: Set DEBUG=-DDEBUG_STRICT=1 in GitHub Actions
    - MINOR: stream: Add level 7 retries on http error 401, 403
    - CLEANUP: remove unused function "ssl_sock_is_ckch_valid"
    - BUILD: SSL: add BoringSSL guarding to "RAND_keep_random_devices_open"
    - BUILD: SSL: do not "update" BoringSSL version equivalent anymore
    - BUG/MEDIUM: http_act: Restore init of log-format list
    - DOC: better describes how to configure a fallback crt
    - BUG/MAJOR: filters: Always keep all offsets up to date during data filtering
    - MINOR: cache: Prepare helper functions for Vary support
    - MEDIUM: cache: Add the Vary header support
    - MINOR: cache: Add a process-vary option that can enable/disable Vary processing
    - BUG/CRITICAL: cache: Fix trivial crash by sending accept-encoding header
    - BUG/MAJOR: peers: fix partial message decoding
    - DOC: cache: Add new caching limitation information
    - DOC: cache: Add information about Vary support
    - DOC: better document the config file format and escaping/quoting rules
    - DOC: Clarify %HP description in log-format
    - CI: github actions: update LibreSSL to 3.3.0
    - CI: github actions: enable 51degrees feature
    - MINOR: fd/threads: silence a build warning with threads disabled
    - BUG/MINOR: tcpcheck: Don't forget to reset tcp-check flags on new kind of check
    - MINOR: tcpcheck: Don't handle anymore in-progress send rules in tcpcheck_main
    - BUG/MAJOR: tcpcheck: Allocate input and output buffers from the buffer pool
    - MINOR: tcpcheck: Don't handle anymore in-progress connect rules in tcpcheck_main
    - MINOR: config: Deprecate and ignore tune.chksize global option
    - MINOR: config: Add a warning if tune.chksize is used
    - REORG: tcpcheck: Move check option parsing functions based on tcp-check
    - MINOR: check: Always increment check health counter on CONPASS
    - MINOR: tcpcheck: Add support of L7OKC on expect rules error-status argument
    - DOC: config: Make disable-on-404 option clearer on transition conditions
    - DOC: config: Move req.hdrs and req.hdrs_bin in L7 samples fetches section
    - BUG/MINOR: http-fetch: Fix smp_fetch_body() when called from a health-check
    - MINOR: plock: use an ARMv8 instruction barrier for the pause instruction
    - MINOR: debug: add "debug dev sched" to stress the scheduler.
    - MINOR: debug: add a trivial PRNG for scheduler stress-tests
    - BUG/MEDIUM: lists: Lock the element while we check if it is in a list.
    - MINOR: task: remove tasklet_insert_into_tasklet_list()
    - MINOR: task: perform atomic counter increments only once per wakeup
    - MINOR: task: remove __tasklet_remove_from_tasklet_list()
    - BUG/MEDIUM: task: close a possible data race condition on a tasklet's list link
    - BUG/MEDIUM: local log format regression.

4 years agoBUG/MEDIUM: local log format regression.
Emeric Brun [Fri, 27 Nov 2020 15:24:34 +0000 (16:24 +0100)] 
BUG/MEDIUM: local log format regression.

Since 2.3 default local log format always adds hostame field.
This behavior change was due to log/sink re-work, because according
to rfc3164 the hostname field is mandatory.

This patch re-introduce a legacy "local" format which is analog
to rfc3164 but with hostname stripped. This is the new
default if logs are generated by haproxy.

To stay compliant with previous configurations, the option
"log-send-hostname" acts as if the default format is switched
to rfc3164.

This patch addresses the github issue #963

This patch should be backported in branches >= 2.3.

4 years agoBUG/MEDIUM: task: close a possible data race condition on a tasklet's list link
Willy Tarreau [Mon, 30 Nov 2020 13:58:53 +0000 (14:58 +0100)] 
BUG/MEDIUM: task: close a possible data race condition on a tasklet's list link

In issue #958 Ashley Penney reported intermittent crashes on AWS's ARM
nodes which would not happen on x86 nodes. After investigation it turned
out that the Neoverse N1 CPU cores used in the Graviton2 CPU are much
more aggressive than the usual Cortex A53/A72/A55 or any x86 regarding
memory ordering.

The issue that was triggered there is that if a tasklet_wakeup() call
is made on a tasklet scheduled to run on a foreign thread and that
tasklet is just being dequeued to be processed, there can be a race at
two places:
  - if MT_LIST_TRY_ADDQ() happens between MT_LIST_BEHEAD() and
    LIST_SPLICE_END_DETACHED() if the tasklet is alone in the list,
    because the emptiness tests matches ;

  - if MT_LIST_TRY_ADDQ() happens during LIST_DEL_INIT() in
    run_tasks_from_lists(), then depending on how LIST_DEL_INIT() ends
    up being implemented, it may even corrupt the adjacent nodes while
    they're being reused for the in-tree storage.

This issue was introduced in 2.2 when support for waking up remote
tasklets was added. Initially the attachment of a tasklet to a list
was enough to know its status and this used to be stable information.
Now it's not sufficient to rely on this anymore, thus we need to use
a different information.

This patch solves this by adding a new task flag, TASK_IN_LIST, which
is atomically set before attaching a tasklet to a list, and is only
removed after the tasklet is detached from a list. It is checked
by tasklet_wakeup_on() so that it may only be done while the tasklet
is out of any list, and is cleared during the state switch when calling
the tasklet. Note that the flag is not set for pure tasks as it's not
needed.

However this introduces a new special case: the function
tasklet_remove_from_tasklet_list() needs to keep both states in sync
and cannot check both the state and the attachment to a list at the
same time. This function is already limited to being used by the thread
owning the tasklet, so in this case the test remains reliable. However,
just like its predecessors, this function is wrong by design and it
should probably be replaced with a stricter one, a lazy one, or be
totally removed (it's only used in checks to avoid calling a possibly
scheduled event, and when freeing a tasklet). Regardless, for now the
function exists so the flag is removed only if the deletion could be
done, which covers all cases we're interested in regarding the insertion.
This removal is safe against a concurrent tasklet_wakeup_on() since
MT_LIST_DEL() guarantees the atomic test, and will ultimately clear
the flag only if the task could be deleted, so the flag will always
reflect the last state.

This should be carefully be backported as far as 2.2 after some
observation period. This patch depends on previous patch
"MINOR: task: remove __tasklet_remove_from_tasklet_list()".

4 years agoMINOR: task: remove __tasklet_remove_from_tasklet_list()
Willy Tarreau [Mon, 30 Nov 2020 13:52:11 +0000 (14:52 +0100)] 
MINOR: task: remove __tasklet_remove_from_tasklet_list()

This function is only used at a single place directly within the
scheduler in run_tasks_from_lists() and it really ought not be called
by anything else, regardless of what its comment says. Let's delete
it, move the two lines directly into the call place, and take this
opportunity to factor the atomic decrement on tasks_run_queue. A comment
was added on the remaining one tasklet_remove_from_tasklet_list() to
mention the risks in using it.

4 years agoMINOR: task: perform atomic counter increments only once per wakeup
Willy Tarreau [Mon, 30 Nov 2020 14:39:00 +0000 (15:39 +0100)] 
MINOR: task: perform atomic counter increments only once per wakeup

In process_runnable_tasks(), we walk the run queue and pick tasks to
insert them into the local list. And for each of these operations we
perform a few increments, some of which are atomic, and they're even
performed under the runqueue's lock. This is useless inside the loop,
better do them at the end, since we don't use these values inside the
loop and they're not used anywhere else either during this time. The
only one is task_list_size which is accessed in parallel by other
threads performing remote tasklet wakeups, but it's already
approximative and is used to decide to get out of the loop when the
limit is reached. So now we compute it first as an initial budget
instead.

4 years agoMINOR: task: remove tasklet_insert_into_tasklet_list()
Willy Tarreau [Mon, 30 Nov 2020 14:30:22 +0000 (15:30 +0100)] 
MINOR: task: remove tasklet_insert_into_tasklet_list()

This function is only called at a single place and adds more confusion
than it removes. It also makes one think it could be used outside of
the scheduler while it must absolutely not. Let's just move its two
lines to the call place, making the code more readable there. In
addition this clearly shows that the preliminary LIST_INIT() is
useless since the entry is immediately overwritten.

4 years agoBUG/MEDIUM: lists: Lock the element while we check if it is in a list.
Olivier Houchard [Wed, 25 Nov 2020 19:38:00 +0000 (20:38 +0100)] 
BUG/MEDIUM: lists: Lock the element while we check if it is in a list.

In MT_LIST_TRY_ADDQ() and MT_LIST_TRY_ADD() we can't just check if the
element is already in a list, because there's a small race condition, it
could be added  between the time we checked, and the time we actually set
its next and prev, so we have to lock it first.

This is required to address issue #958.

This should be backported to 2.3, 2.2 and 2.1.

4 years agoMINOR: debug: add a trivial PRNG for scheduler stress-tests
Willy Tarreau [Mon, 30 Nov 2020 15:17:33 +0000 (16:17 +0100)] 
MINOR: debug: add a trivial PRNG for scheduler stress-tests

Commit a5a447984 ("MINOR: debug: add "debug dev sched" to stress the
scheduler.") doesn't scale with threads because ha_random64() takes care
of being totally thread-safe for use with UUIDs. We don't need this for
the stress-testing functions, let's just implement a xorshift PRNG
instead. On 8 threads the performance jumped from 230k ctx/s with 96%
spent in ha_random64() to 14M ctx/s.

4 years agoMINOR: debug: add "debug dev sched" to stress the scheduler.
Willy Tarreau [Sun, 29 Nov 2020 16:12:15 +0000 (17:12 +0100)] 
MINOR: debug: add "debug dev sched" to stress the scheduler.

This command supports starting a bunch of tasks or tasklets, either on the
current thread (mask=0), all (default), or any set, either single-threaded
or multi-threaded, and possibly auto-scheduled.

These tasks/tasklets will randomly pick another one to wake it up. The
tasks only do it 50% of the time while tasklets always wake two tasks up,
in order to achieve roughly 50% load (since the target might already be
woken up).

4 years agoMINOR: plock: use an ARMv8 instruction barrier for the pause instruction
Your Name [Sat, 28 Nov 2020 15:37:14 +0000 (15:37 +0000)] 
MINOR: plock: use an ARMv8 instruction barrier for the pause instruction

As suggested by @AGSaidi in issue #958, on ARMv8 its convenient to use
an "isb" instruction in pl_cpu_relax() to improve fairness. Without it
I've met a few watchdog conditions on valid locks with 16 threads,
indicating that some threads couldn't manage to get it in 2 seconds. I
never happened again with it. In addition, the performance increased
by slightly more than 5% thanks to the reduced contention.

This should be backported as far as 2.2, possibly even 2.0.

4 years agoBUG/MINOR: http-fetch: Fix smp_fetch_body() when called from a health-check
Christopher Faulet [Wed, 25 Nov 2020 07:08:08 +0000 (08:08 +0100)] 
BUG/MINOR: http-fetch: Fix smp_fetch_body() when called from a health-check

res.body may be called from a health-check. It is probably never used. But it is
possibe. In such case, there is no channel. Thus we must not use it
unconditionally to set the flag SMP_F_MAY_CHANGE on the smp.

Now the condition test the channel first. In addtion, the flag is not set if the
payload is fully received.

This patch must be backported as far as 2.2.

4 years agoDOC: config: Move req.hdrs and req.hdrs_bin in L7 samples fetches section
Christopher Faulet [Tue, 24 Nov 2020 16:13:24 +0000 (17:13 +0100)] 
DOC: config: Move req.hdrs and req.hdrs_bin in L7 samples fetches section

req.hdrs and req.hdrs_bin are L7 sample fetches, not L6. They were in the wrong
section.

This patch may be backported as far as 1.8.

4 years agoDOC: config: Make disable-on-404 option clearer on transition conditions
Christopher Faulet [Fri, 20 Nov 2020 17:54:13 +0000 (18:54 +0100)] 
DOC: config: Make disable-on-404 option clearer on transition conditions

This option is only evaluated for running server. A stopped server becoming
up again but still replying 404s will stay stopped.

4 years agoMINOR: tcpcheck: Add support of L7OKC on expect rules error-status argument
Christopher Faulet [Fri, 20 Nov 2020 16:47:47 +0000 (17:47 +0100)] 
MINOR: tcpcheck: Add support of L7OKC on expect rules error-status argument

L7OKC may now be used as an error status for an HTTP/TCP expect rule. Thus
it is for instance possible to write:

    option httpchk GET /isalive
    http-check expect status 200,404
    http-check expect status 200 error-status L7OKC

It is more or less the same than the disable-on-404 option except that if a
DOWN is up again but still replying a 404 will be set to NOLB state. While
it will stay in DOWN state with the disable-on-404 option.

4 years agoMINOR: check: Always increment check health counter on CONPASS
Christopher Faulet [Fri, 20 Nov 2020 17:13:02 +0000 (18:13 +0100)] 
MINOR: check: Always increment check health counter on CONPASS

Regarding the health counter, a check finished with the CONDPASS result is
now the same than with the PASSED result: The health counter is always
incemented. Before, it was only performed is the health counter was not 0.

There is no change for the disable-on-404 option because it is only
evaluated for running or stopping servers. So with an health check counter
greater than 0. But it will make possible to handle (STOPPED -> STOPPING)
transition for servers.

4 years agoREORG: tcpcheck: Move check option parsing functions based on tcp-check
Christopher Faulet [Fri, 27 Nov 2020 08:58:02 +0000 (09:58 +0100)] 
REORG: tcpcheck: Move check option parsing functions based on tcp-check

The parsing of the check options based on tcp-check rules (redis, spop,
smtp, http...) are moved aways from check.c. Now, these functions are placed
in tcpcheck.c. These functions are only related to the tcpcheck ruleset
configured on a proxy and not to the health-check attached to a server.

4 years agoMINOR: config: Add a warning if tune.chksize is used
Christopher Faulet [Wed, 25 Nov 2020 16:33:03 +0000 (17:33 +0100)] 
MINOR: config: Add a warning if tune.chksize is used

This option is now deprecated. It is recent, but it is now marked as
deprecated as far as 2.2. Thus, there is now a warning in the 2.4 if this
option is still used. It will be removed in 2.5.

Becaue the 2.3 is quite new, this patch may be backported to 2.3.

4 years agoMINOR: config: Deprecate and ignore tune.chksize global option
Christopher Faulet [Wed, 25 Nov 2020 16:20:57 +0000 (17:20 +0100)] 
MINOR: config: Deprecate and ignore tune.chksize global option

This option is now ignored because I/O check buffers are now allocated using the
buffer pool. Thus, it is marked as deprecated in the documentation and ignored
during the configuration parsing. The field is also removed from the global
structure.

Because this option is ignored since a recent fix, backported as fare as 2.2,
this patch should be backported too. Especially because it updates the
documentation.

4 years agoMINOR: tcpcheck: Don't handle anymore in-progress connect rules in tcpcheck_main
Christopher Faulet [Wed, 25 Nov 2020 15:47:30 +0000 (16:47 +0100)] 
MINOR: tcpcheck: Don't handle anymore in-progress connect rules in tcpcheck_main

The special handling of in-progress connect rules at the begining of
tcpcheck_main() function can be removed. Instead, at the begining of the
tcpcheck_eval_connect() function, we test is there is already an existing
connection. In this case, it means we are waiting for a connection
establishment. In addition, before evaluating a new connect rule, we take
care to release any previous connection.

4 years agoBUG/MAJOR: tcpcheck: Allocate input and output buffers from the buffer pool
Christopher Faulet [Wed, 25 Nov 2020 12:47:00 +0000 (13:47 +0100)] 
BUG/MAJOR: tcpcheck: Allocate input and output buffers from the buffer pool

Historically, the input and output buffers of a check are allocated by hand
during the startup, with a specific size (not necessarily the same than
other buffers). But since the recent refactoring of the checks to rely
exclusively on the tcp-checks and to use the underlying mux layer, this part
is totally buggy. Indeed, because these buffers are now passed to a mux,
they maybe be swapped if a zero-copy is possible. In fact, for now it is
only possible in h2_rcv_buf(). Thus the bug concretely only exists if a h2
health-check is performed. But, it is a latent bug for other muxes.

Another problem is the size of these buffers. because it may differ for the
other buffer size, it might be source of bugs.

Finally, for configurations with hundreds of thousands of servers, having 2
buffers per check always allocated may be an issue.

To fix the bug, we now allocate these buffers when required using the buffer
pool. Thus not-running checks don't waste memory and muxes may swap them if
possible. The only drawback is the check buffers have now always the same
size than buffers used by the streams. This deprecates indirectly the
"tune.chksize" global option.

In addition, the http-check regtest have been update to perform some h2
health-checks.

Many thanks to @VigneshSP94 for its help on this bug.

This patch should solve the issue #936. It relies on the commit "MINOR:
tcpcheck: Don't handle anymore in-progress send rules in tcpcheck_main".
Both must be backport as far as 2.2.

bla

4 years agoMINOR: tcpcheck: Don't handle anymore in-progress send rules in tcpcheck_main
Christopher Faulet [Wed, 25 Nov 2020 12:34:51 +0000 (13:34 +0100)] 
MINOR: tcpcheck: Don't handle anymore in-progress send rules in tcpcheck_main

The special handling of in-progress send rules at the begining of
tcpcheck_main() function can be removed. Instead, at the begining of the
tcpcheck_eval_send() function, we test is there is some data in the output
buffer. In this case, it means we are evaluating an unfinished send rule and
we can jump to the sending part, skipping the formatting part.

This patch is mandatory for a major fix on the checks and must be backported
as far as 2.2.

4 years agoBUG/MINOR: tcpcheck: Don't forget to reset tcp-check flags on new kind of check
Christopher Faulet [Wed, 25 Nov 2020 15:43:12 +0000 (16:43 +0100)] 
BUG/MINOR: tcpcheck: Don't forget to reset tcp-check flags on new kind of check

When a new kind of check is found during the parsing of a proxy section (via
an option directive), we must reset tcpcheck flags for this proxy. It is
mandatory to not inherit some flags from a previously declared check (for
instance in the default section).

This patch must be backported as far as 2.2.

4 years agoMINOR: fd/threads: silence a build warning with threads disabled
Willy Tarreau [Thu, 26 Nov 2020 21:25:10 +0000 (22:25 +0100)] 
MINOR: fd/threads: silence a build warning with threads disabled

Building with gcc-9.3.0 without threads may result in this warning:

In file included from include/haproxy/api-t.h:36,
                 from include/haproxy/api.h:33,
                 from src/fd.c:90:
src/fd.c: In function 'updt_fd_polling':
include/haproxy/fd.h:507:11: warning: array subscript 63 is above array bounds of 'int[1]' [-Warray-bounds]
  507 |  DISGUISE(write(poller_wr_pipe[tid], &c, 1));
include/haproxy/compiler.h:92:41: note: in definition of macro 'DISGUISE'
   92 | #define DISGUISE(v) ({ typeof(v) __v = (v); ALREADY_CHECKED(__v); __v; })
      |                                         ^
src/fd.c:113:5: note: while referencing 'poller_wr_pipe'
  113 | int poller_wr_pipe[MAX_THREADS]; // Pipe to wake the threads
      |     ^~~~~~~~~~~~~~

gcc is wrong but this time it cannot be blamed because it doesn't know
that the FD's thread_mask always has at least one bit set. Let's add
the test for all_threads_mask there. It will also remove that test and
drop the else block.

4 years agoCI: github actions: enable 51degrees feature
Ilya Shipitsin [Thu, 26 Nov 2020 15:59:42 +0000 (20:59 +0500)] 
CI: github actions: enable 51degrees feature

4 years agoCI: github actions: update LibreSSL to 3.3.0
Ilya Shipitsin [Thu, 26 Nov 2020 15:56:51 +0000 (20:56 +0500)] 
CI: github actions: update LibreSSL to 3.3.0

4 years agoDOC: Clarify %HP description in log-format
Maciej Zdeb [Thu, 26 Nov 2020 10:45:52 +0000 (10:45 +0000)] 
DOC: Clarify %HP description in log-format

%HP is used to report HTTP request URI in logs, which might be relative
or absolute. Description in documentation should not suggest that it
behaves exactly the same as "path" sample fetch.

This is even more important after 30ee1efe676e8264af16bab833c621d60a72a4d7
because right now, when HTTP2 is a standard, %HP usually returns absolute
URI.

This might be backported as far as 2.1

4 years agoDOC: better document the config file format and escaping/quoting rules
Willy Tarreau [Wed, 25 Nov 2020 18:58:20 +0000 (19:58 +0100)] 
DOC: better document the config file format and escaping/quoting rules

It's always a pain to figure how to proceed when special characters need
to be embedded inside arguments of an expression. Let's document the
configuration file format and how unquoting/unescaping works at each
level (top level and argument level) so that everyone hopefully finds
suitable reminders or examples for complex cases.

This is related to github issue #200 and addresses issues #712 and #966.

4 years agoDOC: cache: Add information about Vary support
Remi Tricot-Le Breton [Thu, 26 Nov 2020 14:51:50 +0000 (15:51 +0100)] 
DOC: cache: Add information about Vary support

We do not skip all responses containing a Vary in the cachign mechanism
anymore. Under certain conditions such responses might be cached.

4 years agoDOC: cache: Add new caching limitation information
Remi Tricot-Le Breton [Thu, 26 Nov 2020 14:51:29 +0000 (15:51 +0100)] 
DOC: cache: Add new caching limitation information

Responses that do not have an explicit expiration time or a validator
will not be cached anymore.

Must be backported if cc9bf2e ("MEDIUM: cache: Change caching
conditions") is backported.

4 years agoBUG/MAJOR: peers: fix partial message decoding
Willy Tarreau [Thu, 26 Nov 2020 16:06:04 +0000 (17:06 +0100)] 
BUG/MAJOR: peers: fix partial message decoding

Another bug in the peers message parser was uncovered by last commit
1dfd4f106 ("BUG/MEDIUM: peers: fix decoding of multi-byte length in
stick-table messages"): the function return on incomplete message does
not check if the channel has a pending close before deciding to return
0. It did not hurt previously because the loop calling co_getblk() once
per character would have depleted the buffer and hit the end, causing
<0 to be returned and matching the condition. But now that we process
at once what is available this cannot be relied on anymore and it's
now clearly visible that the final check is missing.

What happens when this strikes is that if a peer connection breaks in
the middle of a message, the function will return 0 (missing data) but
the caller doesn't check for the closed buffer, subscribes to reads,
and the applet handler is immediately called again since some data are
still available. This is detected by the loop prevention and the process
dies complaining that an appctx is spinning.

This patch simply adds the check for closed channel. It must be
backported to the same versions as the fix above.

4 years agoBUG/CRITICAL: cache: Fix trivial crash by sending accept-encoding header
Tim Duesterhus [Tue, 24 Nov 2020 21:22:56 +0000 (22:22 +0100)] 
BUG/CRITICAL: cache: Fix trivial crash by sending accept-encoding header

Since commit 3d08236cb344607557f22e00cf1cc3e321a1fa95 HAProxy can be trivially
crashed remotely by sending an `accept-encoding` HTTP request header that
contains 16 commas.

This is because the `values` array in `accept_encoding_normalizer` accepts only
16 entries and it is not verified whether the end is reached during looping.

Fix this issue by checking the length. This patch also simplifies the ist
processing in the loop, because it manually calculated offsets and lengths,
when the ist API exposes perfectly safe functions to advance and truncate ists.

I wonder whether the accept_encoding_normalizer function is able to re-use some
existing function for parsing headers that may contain lists of values. I'll
leave this evaluation up to someone else, only patching the obvious crash.

This commit is 2.4-dev specific and was merged just a few hours ago. No
backport needed.

4 years agoMINOR: cache: Add a process-vary option that can enable/disable Vary processing
Remi Tricot-Le Breton [Mon, 16 Nov 2020 14:56:10 +0000 (15:56 +0100)] 
MINOR: cache: Add a process-vary option that can enable/disable Vary processing

The cache section's process-vary option takes a 0 or 1 value to disable
or enable the vary processing.
When disabled, a response containing such a header will never be cached.
When enabled, we will calculate a preliminary hash for a subset of request
headers on all the incoming requests (which might come with a cpu cost) which
will be used to build a secondary key for a given request (see RFC 7234#4.1).
The default value is 0 (disabled).

4 years agoMEDIUM: cache: Add the Vary header support
Remi Tricot-Le Breton [Mon, 16 Nov 2020 14:56:09 +0000 (15:56 +0100)] 
MEDIUM: cache: Add the Vary header support

Calculate a preliminary secondary key for every request we see so that
we can have a real secondary key if the response is cacheable and
contains a manageable Vary header.
The cache's ebtree is now allowed to have multiple entries with the same
primary key. Two of those entries will be distinguished thanks to
secondary keys stored in the cache_entry (based on hashes of a subset of
their headers).
When looking for an entry in the cache (cache_use), we still use the
primary key (built the same way as before), but in case of match, we
also need to check if the entry has a vary signature. If it has one, we
need to perform an extra check based on the newly built secondary key.
We will only be able to forge a response out of the cache if both the
primary and secondary keys match with one of our entries. Otherwise the
request will be forwarder to the server.

4 years agoMINOR: cache: Prepare helper functions for Vary support
Remi Tricot-Le Breton [Mon, 16 Nov 2020 14:56:08 +0000 (15:56 +0100)] 
MINOR: cache: Prepare helper functions for Vary support

The Vary functionality is based on a secondary key that needs to be
calculated for every request to which a server answers with a Vary
header. The Vary header, which can only be found in server responses,
determines which headers of the request need to be taken into account in
the secondary key. Since we do not want to have to store all the headers
of the request until we have the response, we will pre-calculate as many
sub-hashes as there are headers that we want to manage in a Vary
context. We will only focus on a subset of headers which are likely to
be mentioned in a Vary response (accept-encoding and referer for now).
Every managed header will have its own normalization function which is
in charge of transforming the header value into a core representation,
more robust to insignificant changes that could exist between multiple
clients. For instance, two accept-encoding values mentioning the same
encodings but in different orders should give the same hash.
This patch adds a function that parses a Vary header value and checks if
all the values belong to our supported subset. It also adds the
normalization functions for our two headers, as well as utility
functions that can prebuild a secondary key for a given request and
transform it into an actual secondary key after the vary signature is
determined from the response.

4 years agoBUG/MAJOR: filters: Always keep all offsets up to date during data filtering
Christopher Faulet [Tue, 24 Nov 2020 08:49:01 +0000 (09:49 +0100)] 
BUG/MAJOR: filters: Always keep all offsets up to date during data filtering

When at least one data filter is registered on a channel, the offsets of all
filters must be kept up to date. For data filters but also for others. It is
safer to do it in that way. Indirectly, this patch fixes 2 hidden bugs
revealed by the commit 22fca1f2c ("BUG/MEDIUM: filters: Forward all filtered
data at the end of http filtering").

The first one, the worst of both, happens at the end of http filtering when
at least one data filtered is registered on the channel. We call the
http_end() callback function on the filters, when defined, to finish the
http filtering. But it is performed for all filters. Before the commit
22fca1f2c, the only risk was to call the http_end() callback function
unexpectedly on a filter. Now, we may have an overflow on the offset
variable, used at the end to forward all filtered data. Of course, from the
moment we forward an arbitrary huge amount of data, all kinds of bad things
may happen. So offset computation is performed for all filters and
http_end() callback function is called only for data filters.

The other one happens when a data filter alter the data of a channel, it
must update the offsets of all previous filters. But the offset of non-data
filters must be up to date, otherwise, here too we may have an integer
overflow.

Another way to fix these bugs is to always ignore non-data filters from the
offsets computation. But this patch is safer and probably easier to
maintain.

This patch must be backported in all versions where the above commit is. So
as far as 2.0.

4 years agoDOC: better describes how to configure a fallback crt
Joao Morais [Tue, 24 Nov 2020 11:24:30 +0000 (08:24 -0300)] 
DOC: better describes how to configure a fallback crt

A default certificate is always the first one declared in the bind line,
either from `crt` or from `crt-line` option. This commit updates the
description of how to configure a fallback certificate, clarifying that
it needs to be the first one of the bind line.

Should be merged as far as the first SNI filter implementation.

4 years agoBUG/MEDIUM: http_act: Restore init of log-format list
Maciej Zdeb [Mon, 23 Nov 2020 16:03:09 +0000 (16:03 +0000)] 
BUG/MEDIUM: http_act: Restore init of log-format list

Restore init of log-format list in parse_http_del_header which was
accidently deleted by commit ebdd4c55da4360bde7878604ea528c2031a26541
(implementation of different header matching methods for
http-request/response del-header).

This is related to GitHub issue #909

4 years agoBUILD: SSL: do not "update" BoringSSL version equivalent anymore
Ilya Shipitsin [Sat, 21 Nov 2020 18:13:41 +0000 (23:13 +0500)] 
BUILD: SSL: do not "update" BoringSSL version equivalent anymore

we have added all required fine guarding, no need to reduce
BoringSSL version back to 1.1.0 anymore, we do not depend on it

4 years agoBUILD: SSL: add BoringSSL guarding to "RAND_keep_random_devices_open"
Ilya Shipitsin [Sat, 21 Nov 2020 18:10:53 +0000 (23:10 +0500)] 
BUILD: SSL: add BoringSSL guarding to "RAND_keep_random_devices_open"

"RAND_keep_random_devices_open" is OpenSSL specific, does not present
in other OpenSSL variants like LibreSSL or BoringSSL. BoringSSL recently
"updated" its internal openssl version to 1.1.1, we temporarily set it
back to 1.1.0, as we are going to remove that hack, let us add proper
guarding.

4 years agoCLEANUP: remove unused function "ssl_sock_is_ckch_valid"
Ilya Shipitsin [Sat, 21 Nov 2020 18:10:04 +0000 (23:10 +0500)] 
CLEANUP: remove unused function "ssl_sock_is_ckch_valid"

"ssl_sock_is_ckch_valid" is not used anymore, let us remove it

4 years agoMINOR: stream: Add level 7 retries on http error 401, 403
Julien Pivotto [Thu, 12 Nov 2020 10:14:05 +0000 (11:14 +0100)] 
MINOR: stream: Add level 7 retries on http error 401, 403

Level-7 retries are only possible with a restricted number of HTTP
return codes. While it is usually not safe to retry on 401 and 403, I
came up with an authentication backend which was not synchronizing
authentication of users. While not perfect, being allowed to also retry
on those return codes is really helpful and acts as a hotfix until we
can fix the backend.

Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
4 years agoCI: Set DEBUG=-DDEBUG_STRICT=1 in GitHub Actions
Tim Duesterhus [Sat, 21 Nov 2020 17:08:00 +0000 (18:08 +0100)] 
CI: Set DEBUG=-DDEBUG_STRICT=1 in GitHub Actions

This was missing when migrating from Travis.

4 years agoBUILD: Show the value of DEBUG= in haproxy -vv
Tim Duesterhus [Sat, 21 Nov 2020 17:07:59 +0000 (18:07 +0100)] 
BUILD: Show the value of DEBUG= in haproxy -vv

Previously this was not visible after building.

4 years agoBUILD: Make DEBUG part of .build_opts
Tim Duesterhus [Sat, 21 Nov 2020 17:07:58 +0000 (18:07 +0100)] 
BUILD: Make DEBUG part of .build_opts

This forces a recompilation if the value of DEBUG= changes.

4 years ago[RELEASE] Released version 2.4-dev1 v2.4-dev1
Willy Tarreau [Sat, 21 Nov 2020 15:00:40 +0000 (16:00 +0100)] 
[RELEASE] Released version 2.4-dev1

Released version 2.4-dev1 with the following main changes :
    - MINOR: ist: Add istend() function to return a pointer to the end of the string
    - MINOR: sample: Add converters to parse FIX messages
    - REGTEST: converter: Add a regtest for fix converters
    - MINOR: sample: Add converts to parses MQTT messages
    - REGTEST: converter: Add a regtest for MQTT converters
    - MINOR: compat: automatically include malloc.h on glibc
    - MEDIUM: pools: call malloc_trim() from pool_gc()
    - MEDIUM: pattern: call malloc_trim() on pat_ref_reload()
    - MINOR: pattern: move the update revision to the pat_ref, not the expression
    - CLEANUP: pattern: delete the back refs at once during pat_ref_reload()
    - MINOR: pattern: new sflag PAT_SF_REGFREE indicates regex_free() is needed
    - MINOR: pattern: make the delete and prune functions more generic
    - MEDIUM: pattern: link all final elements from the reference
    - MEDIUM: pattern: change the pat_del_* functions to delete from the references
    - MINOR: pattern: remerge the list and tree deletion functions
    - MINOR: pattern: perform a single call to pat_delete_gen() under the expression
    - CLEANUP: acl: don't reference the generic pattern deletion function anymore
    - CLEANUP: pattern: remove pat_delete_fcts[] and pattern_head->delete()
    - MINOR: pattern: introduce pat_ref_delete_by_ptr() to delete a valid reference
    - MINOR: pattern: store a generation number in the reference patterns
    - MEDIUM: pattern: only match patterns that match the current generation
    - MINOR: pattern: add pat_ref_commit() to commit a previously inserted element
    - MINOR: pattern: implement pat_ref_load() to load a pattern at a given generation
    - MINOR: pattern: add pat_ref_purge_older() to purge old entries
    - MEDIUM: pattern: make pat_ref_prune() rely on pat_ref_purge_older()
    - MINOR: pattern: during reload, delete elements frem the ref, not the expression
    - MINOR: pattern: prepare removal of a pattern from the list head
    - MEDIUM: pattern: turn the pattern chaining to single-linked list
    - CLEANUP: cfgparse: remove duplicate registration for transparent build options
    - BUG/MINOR: ssl: don't report 1024 bits DH param load error when it's higher
    - MINOR: http-htx: Add understandable errors for the errorfiles parsing
    - MINOR: ssl: instantiate stats module
    - MINOR: ssl: count client hello for stats
    - MINOR: ssl: add counters for ssl sessions
    - DOC: config: Fix a typo on ssl_c_chain_der
    - MINOR: server: remove idle lock in srv_cleanup_connections
    - BUILD: ssl: silence build warning on uninitialised counters
    - BUILD: http-htx: fix build warning regarding long type in printf
    - REGTEST: ssl: test wildcard and multi-type + exclusions
    - BUG/MEDIUM: ssl/crt-list: correctly insert crt-list line if crt already loaded
    - CI: Expand use of GitHub Actions for CI
    - REGTEST: ssl: mark reg-tests/ssl/ssl_crt-list_filters.vtc as broken
    - BUG/MINOR: pattern: a sample marked as const could be written
    - BUG/MINOR: lua: set buffer size during map lookups
    - MEDIUM: cache: Change caching conditions
    - BUG/MINOR: stats: free dynamically stats fields/lines on shutdown
    - BUG/MEDIUM: stats: prevent crash if counters not alloc with dummy one
    - MINOR: peers: Add traces to peer_treat_updatemsg().
    - BUG/MINOR: peers: Do not ignore a protocol error for dictionary entries.
    - BUG/MINOR: peers: Missing TX cache entries reset.
    - BUG/MEDIUM: peers: fix decoding of multi-byte length in stick-table messages
    - BUG/MINOR: http-fetch: Extract cookie value even when no cookie name
    - BUG/MINOR: http-fetch: Fix calls w/o parentheses of the cookie sample fetches
    - BUG/MEDIUM: check: reuse srv proto only if using same mode
    - MINOR: check: report error on incompatible proto
    - MINOR: check: report error on incompatible connect proto
    - BUG/MINOR: http-htx: Handle warnings when parsing http-error and http-errors
    - BUG/MAJOR: spoe: Be sure to remove all references on a released spoe applet
    - MINOR: spoe: Don't close connection in sync mode on processing timeout
    - BUG/MINOR: tcpcheck: Don't warn on unused rules if check option is after
    - MINOR: init: Fix the prototype for per-thread free callbacks
    - MINOR: config/mux-h2: Return ERR_ flags from init_h2() instead of a status
    - CLEANUP: config: Return ERR_NONE from config callbacks instead of 0
    - MINOR: cfgparse: tighten the scope of newnameserver variable, free it on error.
    - REGTEST: make ssl_client_samples and ssl_server_samples require to 2.2
    - REGTESTS: Add sample_fetches/cook.vtc
    - BUG/MEDIUM: filters: Forward all filtered data at the end of http filtering
    - BUG/MINOR: http-ana: Don't wait for the body of CONNECT requests
    - CLEANUP: flt-trace: Remove unused random-parsing option
    - MINOR: flt-trace: Add an option to inhibits trace messages
    - MINOR: flt-trace: Use a bitfield for the trace options
    - REGTESTS: Add a script to test the random forwarding with several filters
    - REGTESTS: mark the abns test as broken again
    - REGTESTS: converter: add url_dec test
    - CI: Stop hijacking the hosts file
    - CI: Make the h2spec workflow more consistent with the VTest workflow
    - CI: travis-ci: remove amd64, osx builds
    - CI: travis-ci: arm64 are not allowed to fail anymore
    - DOC: add missing 3.10 in the summary
    - MINOR: ssl: remove client hello counters
    - MEDIUM: stats: add counters for failed handshake
    - MINOR: ssl: create common ssl_ctx init
    - MEDIUM: cli/ssl: configure ssl on server at runtime
    - REGTEST: server/cli_set_ssl.vtc requires OpenSSL
    - DOC: coding-style: update a few rules about pointers
    - BUG/MINOR: ssl: segv on startup when AKID but no keyid
    - BUILD: ssl: use SSL_MODE_ASYNC macro instead of OPENSSL_VERSION
    - BUG/MEDIUM: http-ana: Don't eval http-after-response ruleset on empty messages
    - BUG/MEDIUM: ssl/crt-list: bundle support broken in crt-list
    - BUG/MEDIUM: ssl: error when no certificate are found
    - BUG/MINOR: ssl/crt-list: load bundle in crt-list only if activated
    - BUG/MEDIUM: ssl/crt-list: fix error when no file found
    - CI: Github Actions: enable prometheus exporter
    - CI: Github Actions: remove LibreSSL-3.0.2 builds
    - CI: Github Actions: enable BoringSSL builds
    - CI: travis-ci: remove builds migrated to GH actions
    - BUILD: makefile: enable crypt(3) for OpenBSD
    - CI: Github Action: run "apt-get update" before packages restore
    - BUILD: SSL: guard TLS13 ciphersuites with HAVE_SSL_CTX_SET_CIPHERSUITES
    - CI: Pass the github.event_name to matrix.py
    - CI: Clean up Windows CI
    - DOC: clarify how to create a fallback crt
    - CLEANUP: connection: do not use conn->owner when the session is known
    - BUG/MAJOR: connection: reset conn->owner when detaching from session list
    - REGTESTS: mark proxy_protocol_random_fail as broken
    - BUG/MINOR: http_htx: Fix searching headers by substring
    - MINOR: http_act: Add -m flag for del-header name matching method

4 years agoMINOR: http_act: Add -m flag for del-header name matching method
Maciej Zdeb [Fri, 20 Nov 2020 13:58:48 +0000 (13:58 +0000)] 
MINOR: http_act: Add -m flag for del-header name matching method

This patch adds -m flag which allows to specify header name
matching method when deleting headers from http request/response.
Currently beg, end, sub, str and reg are supported.

This is related to GitHub issue #909

4 years agoBUG/MINOR: http_htx: Fix searching headers by substring
Maciej Zdeb [Fri, 20 Nov 2020 12:12:24 +0000 (12:12 +0000)] 
BUG/MINOR: http_htx: Fix searching headers by substring

Function __http_find_header is used to search headers by name using specified
matching method. Matching by substring returned unexpected results due to wrong
length of substring supplied to strnistr function.

Fixed also the boolean condition by inverting it, as we're interested in
headers that contains the substring.

This patch should be backported as far as 2.2

4 years agoREGTESTS: mark proxy_protocol_random_fail as broken
Willy Tarreau [Sat, 21 Nov 2020 14:33:03 +0000 (15:33 +0100)] 
REGTESTS: mark proxy_protocol_random_fail as broken

As indicated in issue #907 it very frequently fails on FreeBSD, and
looking at it, it's broken in multiple ways. It relies on log ordering
between two layers, the first one being allowed to support h2. Given
that on FreeBSD it usually ends up in timeouts, it's very likely that
for some reason one frontend logs before the other one or that curl
uses h2 instead of h1 there, and that the log instance waits forever
for its lines.

Usually it works fine when run locally though, so let's not remove it
and mark it as broken instead so that it can still be used when relevant.

4 years agoBUG/MAJOR: connection: reset conn->owner when detaching from session list
Willy Tarreau [Fri, 20 Nov 2020 16:22:44 +0000 (17:22 +0100)] 
BUG/MAJOR: connection: reset conn->owner when detaching from session list

Baptiste reported a new crash affecting 2.3 which can be triggered
when using H2 on the backend, with http-reuse always and with a tens
of clients doing close only. There are a few combined cases which cause
this to happen, but each time the issue is the same, an already freed
session is dereferenced in session_unown_conn().

Two cases were identified to cause this:
  - a connection referencing a session as its owner, which is detached
    from the session's list and is destroyed after this session ends.
    The test on conn->owner before calling session_unown_conn() is not
    sufficent as the pointer is not null but is not valid anymore.

  - a connection that never goes idle and that gets killed form the
    mux, where session_free() is called first, then conn_free() calls
    session_unown_conn() which scans the just freed session for older
    connections. This one is only triggered with DEBUG_UAF

The reason for this session to be present here is that it's needed during
the connection setup, to be passed to conn_install_mux_be() to mux->init()
as the owning session, but it's never deleted aftrewards. Furthermore, even
conn_session_free() doesn't delete this pointer after freeing the session
that lies there. Both do definitely result in a use-after-free that's more
easily triggered under DEBUG_UAF.

This patch makes sure that the owner is always deleted after detaching
or killing the session. However it is currently not possible to clear
the owner right after a synchronous init because the proxy protocol
apparently needs it (a reg test checks this), and if we leave it past
the connection setup with the session not attached anywhere, it's hard
to catch the right moment to detach it. This means that the session may
remain in conn->owner as long as the connection has never been added to
nor removed from the session's idle list. Given that this patch needs to
remain simple enough to be backported, instead it adds a workaround in
session_unown_conn() to detect that the element is already not attached
anywhere.

This fix absolutely requires previous patch "CLEANUP: connection: do not
use conn->owner when the session is known" otherwise the situation will
be even worse, as some places used to rely on conn->owner instead of the
session.

The fix could theorically be backported as far as 1.8. However, the code
in this area has significantly changed along versions and there are more
risks of breaking working stuff than fixing real issues there. The issue
was really woken up in two steps during 2.3-dev when slightly reworking
the idle conns with commit 08016ab82 ("MEDIUM: connection: Add private
connections synchronously in session server list") and when adding
support for storing used H2 connections in the session and adding the
necessary call to session_unown_conn() in the muxes. But the same test
managed to crash 2.2 when built in DEBUG_UAF and patched like this,
proving that we used to already leave dangling pointers behind us:

|  diff --git a/include/haproxy/connection.h b/include/haproxy/connection.h
|  index f8f235c1a..dd30b5f80 100644
|  --- a/include/haproxy/connection.h
|  +++ b/include/haproxy/connection.h
|  @@ -458,6 +458,10 @@ static inline void conn_free(struct connection *conn)
|                          sess->idle_conns--;
|                  session_unown_conn(sess, conn);
|          }
|  +       else {
|  +               struct session *sess = conn->owner;
|  +               BUG_ON(sess && sess->origin != &conn->obj_type);
|  +       }
|
|          sockaddr_free(&conn->src);
|          sockaddr_free(&conn->dst);

It's uncertain whether an existing code path there can lead to dereferencing
conn->owner when it's bad, though certain suspicious memory corruption bugs
make one think it's a likely candidate. The patch should not be hard to
adapt there.

Backports to 2.1 and older are left to the appreciation of the person
doing the backport.

A reproducer consists in this:

  global
    nbthread 1

  listen l
    bind :9000
    mode http
    http-reuse always
    server s 127.0.0.1:8999 proto h2

  frontend f
    bind :8999 proto h2
    mode http
    http-request return status 200

Then this will make it crash within 2-3 seconds:

  $ h1load -e -r 1 -c 10 http://0:9000/

If it does not, it might be that DEBUG_UAF was not used (it's harder then)
and it might be useful to restart.

4 years agoCLEANUP: connection: do not use conn->owner when the session is known
Willy Tarreau [Fri, 20 Nov 2020 16:08:15 +0000 (17:08 +0100)] 
CLEANUP: connection: do not use conn->owner when the session is known

At a few places we used to rely on conn->owner to retrieve the session
while the session is already known. This is not correct because at some
of these points the reason the connection's owner was still the session
(instead of NULL) is a mistake. At one place a comparison is even made
between the session and conn->owner assuming it's valid without checking
if it's NULL. Let's clean this up to use the session all the time.

Note that this will be needed for a forthcoming fix and will have to be
backported.

4 years agoDOC: clarify how to create a fallback crt
Joao Morais [Sat, 21 Nov 2020 10:42:20 +0000 (07:42 -0300)] 
DOC: clarify how to create a fallback crt

HAProxy uses CN and SAN of the certificates to match incoming SNI, and
use the matching certificate in the TLS handshake. `crt-list` goes
further and allows to configure SNI filters to explicitly define the
FQDNs that should match a certificate.

The first declared certificate of the `crt-list` option follows the same
rules, and it's also used as a fallback - the certificate that should be
used if SNI isn't provided or the provided one cannot match any
certificate or SNI filter. If a provided SNI matches the CN or SAN of
the first certificate, the first certificate would be used even if a
matching SNI filter is declared later.

This change clarifies this scenario and documents a filter that can be
used to convert the first declared certificate as a proper fallback.

Should be merged as far as the first SNI filter implementation.

4 years agoCI: Clean up Windows CI
Tim Duesterhus [Thu, 19 Nov 2020 23:16:01 +0000 (00:16 +0100)] 
CI: Clean up Windows CI

This patch cleans up the Windows CI to look more similar to the refactored
Linux CI on GitHub Actions.

It switches the environment set-up from some manual cygwin setup via choco to
the msys2/setup-msys2@v2 action which just works and allows later steps to look
like any others without need to manually specify the shell.

This new setup is much faster than before where a single Windows build required
more than 10 minutes with more than 5 minutes just spent setting up the
environment and more than 6 minutes compiling HAProxy.

With this patch the setting of of the environment is done in less than a minute
and HAProxy is compiled in less than 2 minutes.

The only drawback is that Lua does not appear to be readily available. I expect
this to be acceptable and that the benefits far outweight this small drawback.

4 years agoCI: Pass the github.event_name to matrix.py
Tim Duesterhus [Thu, 19 Nov 2020 23:16:00 +0000 (00:16 +0100)] 
CI: Pass the github.event_name to matrix.py

This is a preparation to later run some matrix entries on schedule only.

Within the matrix.py script it can now be detected whether the workflow is
running on schedule by using:

    if build_type == "schedule":
        matrix.append(...)

4 years agoBUILD: SSL: guard TLS13 ciphersuites with HAVE_SSL_CTX_SET_CIPHERSUITES
Ilya Shipitsin [Sat, 21 Nov 2020 09:37:34 +0000 (14:37 +0500)] 
BUILD: SSL: guard TLS13 ciphersuites with HAVE_SSL_CTX_SET_CIPHERSUITES

HAVE_SSL_CTX_SET_CIPHERSUITES is newly defined macro set in openssl-compat.h,
which helps to identify ssl libs (currently OpenSSL-1.1.1 only) that supports
TLS13 cipersuites manipulation on TLS13 context

4 years agoCI: Github Action: run "apt-get update" before packages restore
Ilya Shipitsin [Sat, 21 Nov 2020 08:42:19 +0000 (13:42 +0500)] 
CI: Github Action: run "apt-get update" before packages restore

ubuntu somehow needs it, no idea why does not apt-get do it itself.

4 years agoBUILD: makefile: enable crypt(3) for OpenBSD
Matthieu Guegan [Fri, 20 Nov 2020 09:50:39 +0000 (10:50 +0100)] 
BUILD: makefile: enable crypt(3) for OpenBSD

Allow OpenBSD to support encrypted passwords in Userlists.

OpenBSD's crypt(3) function is provided directly by libc and does not
require -lcrypt.
Signed-off-by: Matthieu Guegan <matthieu.guegan@deindeal.ch>
4 years agoCI: travis-ci: remove builds migrated to GH actions
Ilya Shipitsin [Fri, 20 Nov 2020 09:43:57 +0000 (14:43 +0500)] 
CI: travis-ci: remove builds migrated to GH actions

4 years agoCI: Github Actions: enable BoringSSL builds
Ilya Shipitsin [Fri, 20 Nov 2020 09:43:23 +0000 (14:43 +0500)] 
CI: Github Actions: enable BoringSSL builds

4 years agoCI: Github Actions: remove LibreSSL-3.0.2 builds
Ilya Shipitsin [Fri, 20 Nov 2020 09:42:27 +0000 (14:42 +0500)] 
CI: Github Actions: remove LibreSSL-3.0.2 builds

4 years agoCI: Github Actions: enable prometheus exporter
Ilya Shipitsin [Fri, 20 Nov 2020 09:41:40 +0000 (14:41 +0500)] 
CI: Github Actions: enable prometheus exporter

4 years agoBUG/MEDIUM: ssl/crt-list: fix error when no file found
William Lallemand [Fri, 20 Nov 2020 17:26:09 +0000 (18:26 +0100)] 
BUG/MEDIUM: ssl/crt-list: fix error when no file found

When a file from a crt-list was not found, this one was ignored silently
letting HAProxy starts without it.

This bug was introduced by 47da821 ("MEDIUM: ssl: emulates the
multi-cert bundles in the crtlist").

This commit adds a found variable which is checked once we tried every
bundle combination so we can exits with an error if none were found.

Must be backported in 2.3.

4 years agoBUG/MINOR: ssl/crt-list: load bundle in crt-list only if activated
William Lallemand [Fri, 20 Nov 2020 17:23:40 +0000 (18:23 +0100)] 
BUG/MINOR: ssl/crt-list: load bundle in crt-list only if activated

Don't try to load a bundle from a crt-list if the bundle support was
disabled with ssl-load-extra-files.

Must be backported to 2.3.

4 years agoBUG/MEDIUM: ssl: error when no certificate are found
William Lallemand [Fri, 20 Nov 2020 14:36:13 +0000 (15:36 +0100)] 
BUG/MEDIUM: ssl: error when no certificate are found

When a non-existing file was specified in the configuration, haproxy
does not exits with an error which is not normal.

This bug was introduced by dfa93be ("MEDIUM: ssl: emulate multi-cert
bundles loading in standard loading") which does nothing if the stat
failed.

This patch introduce a "found" variable which is checked at the end of
the function so we exit with an error if no find were found.

Must be backported to 2.3.

4 years agoBUG/MEDIUM: ssl/crt-list: bundle support broken in crt-list
William Lallemand [Fri, 20 Nov 2020 13:23:38 +0000 (14:23 +0100)] 
BUG/MEDIUM: ssl/crt-list: bundle support broken in crt-list

In issue #970 it was reported that the bundle loading does not work
anymore with crt-list.

This bug was introduced by 47da821 ("MEDIUM: ssl: emulates the
multi-cert bundles in the crtlist") which incorrectly uses "path"
instead of "crt_path" in the name resolution.

Must be backported to 2.3.

4 years agoBUG/MEDIUM: http-ana: Don't eval http-after-response ruleset on empty messages
Christopher Faulet [Wed, 18 Nov 2020 15:44:02 +0000 (16:44 +0100)] 
BUG/MEDIUM: http-ana: Don't eval http-after-response ruleset on empty messages

It is not possible on response comming from a server, but an errorfile may be
empty. In this case, the http-after-response ruleset must not be evaluated
because it is totally unexpected to manipulate headers on an empty HTX message.

This patch must be backported everywhere the http-after-response rules are
supported, i.e as far as 2.2.

4 years agoBUILD: ssl: use SSL_MODE_ASYNC macro instead of OPENSSL_VERSION
Ilya Shipitsin [Fri, 13 Nov 2020 20:56:34 +0000 (01:56 +0500)] 
BUILD: ssl: use SSL_MODE_ASYNC macro instead of OPENSSL_VERSION

4 years agoBUG/MINOR: ssl: segv on startup when AKID but no keyid
William Lallemand [Thu, 19 Nov 2020 15:24:13 +0000 (16:24 +0100)] 
BUG/MINOR: ssl: segv on startup when AKID but no keyid

In bug #959 it was reported that haproxy segfault on startup when trying
to load a certifcate which use the X509v3 AKID extension but without the
keyid field.

This field is not mandatory and could be replaced by the serial or the
DirName.

For example:

   X509v3 extensions:
       X509v3 Basic Constraints:
           CA:FALSE
       X509v3 Subject Key Identifier:
           42:7D:5F:6C:3E:0D:B7:2C:FD:6A:8A:32:C6:C6:B9:90:05:D1:B2:9B
       X509v3 Authority Key Identifier:
           DirName:/O=HAProxy Technologies/CN=HAProxy Test Intermediate CA
           serial:F2:AB:C1:41:9F:AB:45:8E:86:23:AD:C5:54:ED:DF:FA

This bug was introduced by 70df7b ("MINOR: ssl: add "issuers-chain-path" directive").

This patch must be backported as far as 2.2.

4 years agoDOC: coding-style: update a few rules about pointers
Willy Tarreau [Wed, 18 Nov 2020 18:53:45 +0000 (19:53 +0100)] 
DOC: coding-style: update a few rules about pointers

It's really annoying to see that in 2020 we're still facing bugs caused
by dangling pointers in the code that result from poorly written rules
about how these pointers are supposed to be handled, set and reset. Let's
add a few supposedly obvious (but apparently not) rules about how pointers
have to be used through out the code in hope to make such bad practices
disappear (or at least have something to point the authors to after
reviewing their code).

4 years agoREGTEST: server/cli_set_ssl.vtc requires OpenSSL
William Lallemand [Wed, 18 Nov 2020 16:41:28 +0000 (17:41 +0100)] 
REGTEST: server/cli_set_ssl.vtc requires OpenSSL

Add OpenSSL as a requirement for this test.

4 years agoMEDIUM: cli/ssl: configure ssl on server at runtime
William Dauchy [Sat, 14 Nov 2020 18:25:33 +0000 (19:25 +0100)] 
MEDIUM: cli/ssl: configure ssl on server at runtime

in the context of a progressive backend migration, we want to be able to
activate SSL on outgoing connections to the server at runtime without
reloading.
This patch adds a `set server ssl` command; in order to allow that:

- add `srv_use_ssl` to `show servers state` command for compatibility,
  also update associated parsing
- when using default-server ssl setting, and `no-ssl` on server line,
  init SSL ctx without activating it
- when triggering ssl API, de/activate SSL connections as requested
- clean ongoing connections as it is done for addr/port changes, without
  checking prior server state

example config:

backend be_foo
  default-server ssl
  server srv0 127.0.0.1:6011 weight 1 no-ssl

show servers state:

  5 be_foo 1 srv0 127.0.0.1 2 0 1 1 15 1 0 4 0 0 0 0 - 6011 - -1

where srv0 can switch to ssl later during the runtime:

  set server be_foo/srv0 ssl on

  5 be_foo 1 srv0 127.0.0.1 2 0 1 1 15 1 0 4 0 0 0 0 - 6011 - 1

Also update existing tests and create a new one.

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoMINOR: ssl: create common ssl_ctx init
William Dauchy [Sat, 14 Nov 2020 18:25:32 +0000 (19:25 +0100)] 
MINOR: ssl: create common ssl_ctx init

a common init for ssl_ctx will be later usable in other functions in
order to support hot enable of ssl during runtime.

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoMEDIUM: stats: add counters for failed handshake
Amaury Denoyelle [Fri, 13 Nov 2020 15:05:00 +0000 (16:05 +0100)] 
MEDIUM: stats: add counters for failed handshake

Report on ssl stats the total number of handshakes terminated in a
failure.

4 years agoMINOR: ssl: remove client hello counters
Amaury Denoyelle [Fri, 13 Nov 2020 15:04:59 +0000 (16:04 +0100)] 
MINOR: ssl: remove client hello counters

Remove the ssl client hello received counter. This counter is not
meaningful and was only implemented on the fronted.

4 years agoDOC: add missing 3.10 in the summary
William Lallemand [Wed, 18 Nov 2020 09:41:24 +0000 (10:41 +0100)] 
DOC: add missing 3.10 in the summary

3.10. Log forwarding was missing in the summary.

4 years agoCI: travis-ci: arm64 are not allowed to fail anymore
Ilya Shipitsin [Wed, 11 Nov 2020 18:16:22 +0000 (23:16 +0500)] 
CI: travis-ci: arm64 are not allowed to fail anymore

4 years agoCI: travis-ci: remove amd64, osx builds
Ilya Shipitsin [Wed, 11 Nov 2020 18:15:20 +0000 (23:15 +0500)] 
CI: travis-ci: remove amd64, osx builds

they are migrated to Github Actions

4 years agoCI: Make the h2spec workflow more consistent with the VTest workflow
Tim Duesterhus [Fri, 13 Nov 2020 19:15:53 +0000 (20:15 +0100)] 
CI: Make the h2spec workflow more consistent with the VTest workflow

This PR aims to make the workflow more consistent, by reusing the same wording
for the step names and the same commands to make it look like the vtest
workflow as much as possible.

It was renamed to compliance.yml to match the human readable name better. This
also allows to extend the workflow with other compliance tools later on, nicely
grouping those jobs together in a single file.

No functional changes have been made.

4 years agoCI: Stop hijacking the hosts file
Tim Duesterhus [Wed, 11 Nov 2020 21:36:54 +0000 (22:36 +0100)] 
CI: Stop hijacking the hosts file

vtest/VTest#24 is merged now. This step is no longer required.

4 years agoREGTESTS: converter: add url_dec test
William Dauchy [Sun, 15 Nov 2020 13:04:43 +0000 (14:04 +0100)] 
REGTESTS: converter: add url_dec test

while looking at `url_dec` implementation I realised there was not yet a
simple test to avoid future regressions.
This one is testing simple case, including the "+" behaviour depending
on the argument passed to `url_dec`

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoREGTESTS: mark the abns test as broken again
Willy Tarreau [Tue, 17 Nov 2020 10:45:10 +0000 (11:45 +0100)] 
REGTESTS: mark the abns test as broken again

There is apparently a race in this test that would require relying on
haproxy's output to make it reliably work, but currently vtest doesn't
have this option. Let's mark it broken again to avoid polluting the CI.

This is discussed in github issue #950.

4 years agoREGTESTS: Add a script to test the random forwarding with several filters
Christopher Faulet [Tue, 17 Nov 2020 09:47:35 +0000 (10:47 +0100)] 
REGTESTS: Add a script to test the random forwarding with several filters

Three filters are used. The compression filter is enclose by two trace filters,
both with the random forwarding enabled. An HTTP test is performed but also a
TCP test using an HTTP tunnel.

4 years agoMINOR: flt-trace: Use a bitfield for the trace options
Christopher Faulet [Tue, 17 Nov 2020 10:33:36 +0000 (11:33 +0100)] 
MINOR: flt-trace: Use a bitfield for the trace options

Instead of using a integer for each option, we now use a bitfield. Each option
is represented as a flag now.

4 years agoMINOR: flt-trace: Add an option to inhibits trace messages
Christopher Faulet [Tue, 17 Nov 2020 09:45:05 +0000 (10:45 +0100)] 
MINOR: flt-trace: Add an option to inhibits trace messages

The 'quiet' option may be set to inibits the trace messages. The trace filter is
a bit verbose. This option may be used to not display the messages.

4 years agoCLEANUP: flt-trace: Remove unused random-parsing option
Christopher Faulet [Tue, 17 Nov 2020 09:43:26 +0000 (10:43 +0100)] 
CLEANUP: flt-trace: Remove unused random-parsing option

This option was only used by the legacy HTTP mode. In HTX, it is not used. So it
can be removed.

4 years agoBUG/MINOR: http-ana: Don't wait for the body of CONNECT requests
Christopher Faulet [Mon, 16 Nov 2020 15:03:35 +0000 (16:03 +0100)] 
BUG/MINOR: http-ana: Don't wait for the body of CONNECT requests

CONNECT requests are bodyless messages but with no EOM blocks. Thus, conditions
to stop waiting for the message payload are not suited to this kind of
messages. Indeed, the message finishes on an EOH block. But the tunnel mode at
the stream level is only set in HTTP_XFER_BODY analyser. So, the stream is
blocked, waiting for a body that does not exist till a timeout expires.

To fix this bug, we just stop waiting for a body for CONNECT requests. Another
solution is to rely on HTX_SL_F_BODYLESS/HTTP_MSGF_BODYLESS flags. But this one
is less intrusive.

This message must be backported as far as 2.0. For the 2.0, only the HTX part
must be fixed.

4 years agoBUG/MEDIUM: filters: Forward all filtered data at the end of http filtering
Christopher Faulet [Mon, 16 Nov 2020 09:10:38 +0000 (10:10 +0100)] 
BUG/MEDIUM: filters: Forward all filtered data at the end of http filtering

When http filtering ends, if there are some filtered data not forwarded yet, we
forward them, in flt_http_end(). Most of time, this doesn't happen, except when
a tunnel is established using a CONNECT. In this case, there is not EOM on the
request and there is no body. Thus the headers are never forwarded, blocking the
stream.

This patch must be backported as far as 2.0. Prior versions don't suffer of this
bug because there is no HTX support. On the 2.0, the change is only applicable
on HTX streams. A special test must be performed to make sure.

4 years agoREGTESTS: Add sample_fetches/cook.vtc
Tim Duesterhus [Fri, 13 Nov 2020 18:36:47 +0000 (19:36 +0100)] 
REGTESTS: Add sample_fetches/cook.vtc

Add a reg-test verifying the fix in dea7c209f8a77b471323dd97bdc1ac4d7a17b812.

Some parts of the configuration used in the were taken from the initial bug
report from Maciej.

Should be backported together with dea7c209f8a77b471323dd97bdc1ac4d7a17b812
(all stable versions).

Co-authored-by: Maciej Zdeb <maciej@zdeb.pl>