Firefox 90+ apparently sends more than 10 headers, so we need to bump
the number to some higher number. Bump it to 100 just to be on a save
side, this is for internal use only anyway.
(cherry picked from commit e4654d1a6a)
The statschannel truncated test still terminates abruptly sometimes and
it doesn't return the answer for the first query. This might happen
when the second process_request() discovers there's not enough space
before the sending is complete and the connection is terminated before
the client gets the data.
Change the isc_http, so it pauses the reading when it receives the data
and resumes it only after the sending has completed or there's
incomplete request waiting for more data.
This makes the request processing slightly less efficient, but also less
taxing for the server, because previously all requests that has been
received via single TCP read would be processed in the loop and the
sends would be queued after the read callback has processed a full
buffer.
(cherry picked from commit 13959781cb)
Rewrite the isc_httpd to be more robust.
1. Replace the hand-crafted HTTP request parser with picohttpparser for
parsing the whole HTTP/1.0 and HTTP/1.1 requests. Limit the number
of allowed headers to 10 (arbitrary number).
2. Replace the hand-crafted URL parser with isc_url_parse for parsing
the URL from the HTTP request.
3. Increase the receive buffer to match the isc_netmgr buffers, so we
can at least receive two full isc_nm_read()s. This makes the
truncation processing much simpler.
4. Process the received buffer from single isc_nm_read() in a single
loop and schedule the sends to be independent of each other.
The first two changes makes the code simpler and rely on already
existing libraries that we already had (isc_url based on nodejs) or are
used elsewhere (picohttpparser).
The second two changes remove the artificial "truncation" limit on
parsing multiple request. Now only a request that has too many
headers (currently 10) or is too big (so, the receive buffer fills up
without reaching end of the request) will end the connection.
We can be benevolent here with the limites, because the statschannel
channel is by definition private and access must be allowed only to
administrators of the server. There are no timers, no rate-limiting, no
upper limit on the number of requests that can be served, etc.
(cherry picked from commit beecde7120)
PicoHTTPParser is a tiny, primitive, fast HTTP request/response parser.
Unlike most parsers, it is stateless and does not allocate memory by
itself. All it does is accept pointer to buffer and the output
structure, and setups the pointers in the latter to point at the
necessary portions of the buffer.
(cherry picked from commit 3a8884f024)
It was possible that accept callback can be called after listener
shutdown. In such a case the callback pointer equals NULL, leading to
segmentation fault. This commit fixes that.
This commit introduces a primitive isc__nmsocket_stop() which performs
shutting down on a multilayered socket ensuring the proper order of
the operations.
The shared data within the socket object can be destroyed after the
call completed, as it is guaranteed to not be used from within the
context of other worker threads.
(cherry picked from commit 5ab2c0ebb3)
Instead of having "arbitrary" (void *)-1 to define non-linked, add a
ISC_LINK_TOMBSTONE(type) macro that replaces the "magic" value with a
define.
(cherry picked from commit 5e20c2ccfb)
Since we are using designated initializers, we were missing initializers
for ISC_LIST and ISC_LINK, add them, so you can do
*foo = (foo_t){ .list = ISC_LIST_INITIALIZER };
Instead of:
*foo = (foo_t){ 0 };
ISC_LIST_INIT(foo->list);
(cherry picked from commit cb3c36b8bf)
I.e. print the name of the function in BIND that called the system
function that returned an error. Since it was useful for pthreads
code, it seems worthwhile doing so everywhere.
(cherry picked from commit 26ed03a61e)
Mostly generated automatically with the following semantic patch,
except where coccinelle was confused by #ifdef in lib/isc/net.c
@@ expression list args; @@
- UNEXPECTED_ERROR(__FILE__, __LINE__, args)
+ UNEXPECTED_ERROR(args)
@@ expression list args; @@
- FATAL_ERROR(__FILE__, __LINE__, args)
+ FATAL_ERROR(args)
(cherry picked from commit ec50c58f52)
This commit fixes TLS DNS verification error message reporting which
we probably broke during one of the recent networking code
refactorings.
This prevent e.g. dig from producing useful error messages related to
TLS certificates verification.
Ensure that TLS error is empty before calling SSL_get_error() or doing
SSL I/O so that the result will not get affected by prior error
statuses.
In particular, the improper error handling led to intermittent unit
test failure and, thus, could be responsible for some of the system
test failures and other intermittent TLS-related issues.
See here for more details:
https://www.openssl.org/docs/man3.0/man3/SSL_get_error.html
In particular, it mentions the following:
> The current thread's error queue must be empty before the TLS/SSL
> I/O operation is attempted, or SSL_get_error() will not work
> reliably.
As we use the result of SSL_get_error() to decide on I/O operations,
we need to ensure that it works reliably by cleaning the error queue.
TLS DNS: empty error queue before attempting I/O
previously, when ISC_BUFFER_USEINLINE was defined, macros were
used to implement isc_buffer primitives (isc_buffer_init(),
isc_buffer_region(), etc). these macros were missing the DbC
assertions for those primitives, which made it possible for
coding errors to go undetected.
adding the assertions to the macros caused compiler warnings on
some platforms. therefore, this commit converts the ISC__BUFFER
macros to static inline functions instead, with assertions included,
and eliminates the non-inline implementation from buffer.c.
the --enable-buffer-useinline configure option has been removed.
(cherry picked from commit 1926ddc987)
- use isc_buffer functions when appropriate, rather than converting
to and from isc_region unnecessarily
- use the zlib total_out value instead of calculating it
- use c99 struct initialization
(cherry picked from commit 4b7248545e)
An assertion failure would be triggered when the TCP connection
is canceled during sending the data back to the client.
Don't require the state to be `RECV` on non successful read to
gracefully handle canceled TCP connection during the SEND state of the
HTTPD channel.
(cherry picked from commit 6562227cc8)
when the compression buffer was reused for multiple statistics
requests, responses could grow beyond the correct size. this was
because the buffer was not cleared before reuse; compressed data
was still written to the beginning of the buffer, but then the size
of used region was increased by the amount written, rather than set
to the amount written. this caused responses to grow larger and
larger, potentially reading past the end of the allocated buffer.
(cherry picked from commit 47e9fa981e)
This should make sure that the memory context is not destroyed
before the memory pool, which is using the context.
(cherry picked from commit e97c3eea95)
When the HTTP request has a body part after the HTTP headers, it is
not getting processed and is being prepended to the next request's data,
which results in an error when trying to parse it.
Improve the httpd.c:process_request() function with the following
additions:
1. Require that HTTP POST requests must have Content-Length header.
2. When Content-Length header is set, extract its value, and make sure
that it is valid and that the whole request's body is received before
processing the request.
3. Discard the request's body by consuming Content-Length worth of data
in the buffer.
(cherry picked from commit c2bbdc8a648c9630b2c9cea5227ad5c309c2ade5)
Add a new `const char **fvalue` parameter to the httpd.c:have_header()
function which, when set, will point to the found header's value.
(cherry picked from commit 376e698dc21f4117d6461101c4cfbaef2b724592)
In some circumstances generic TLS code could have resumed data reading
unexpectedly on the TCP layer code. Due to this, the behaviour of
isc_nm_pauseread() and isc_nm_resumeread() might have been
unexpected. This commit fixes that.
The bug does not seems to have real consequences in the existing code
due to the way the code is used. However, the bug could have lead to
unexpected behaviour and, at any rate, makes the TLS code behave
differently from the TCP code, with which it attempts to be as
compatible as possible.
(cherry picked from commit ec0647d546204a0e09aeaf0e2aabb37f1fb67dd0)
Sometimes tls_do_bio() might be called when there is no new data to
process (most notably, when resuming reads), in such a case internal
TLS session state will remain untouched and old value in 'errno' will
alter the result of SSL_get_error() call, possibly making it to return
SSL_ERROR_SYSCALL. This value will be treated as an error, and will
lead to closing the connection, which is not what expected.
The STATID_CONNECT and STATID_CONNECTFAIL statistics were used
incorrectly. The STATID_CONNECT was incremented twice (once in
the *_connect_direct() and once in the callback) and STATID_CONNECTFAIL
would not be incremented at all if the failure happened in the callback.
Closes: #3452
(cherry picked from commit 59e1703b50dc4a5c52e3d6ae13bdd873a677b03f)
On FreeBSD (and perhaps other *BSD) systems, the TCP connect() call (via
uv_tcp_connect()) can fail with transient UV_EADDRINUSE error. The UDP
code already handles this by trying three times (is a charm) before
giving up. Add a code for the TCP, TCPDNS and TLSDNS layers to also try
three times before giving up by calling uv_tcp_connect() from the
callback two more time on UV_EADDRINUSE error.
Additionally, stop the timer only if we succeed or on hard error via
isc__nm_failed_connect_cb().
(cherry picked from commit b21f507c0ac5b5d2d2c27fd2d71e27e8605dd5fc)
Before this change the TLS code would ignore the accept callback result,
and would not try to gracefully close the connection. This had not been
noticed, as it is not really required for DoH. Now the code tries to
shut down the TLS connection gracefully when accepting it is not
successful.
(cherry picked from commit ffcb54211e)
Otherwise the code path will lead to a call to SSL_get_error()
returning SSL_ERROR_SSL, which in turn might lead to closing
connection to early in an unexpected way, as it is clearly not what is
intended.
The issue was found when working on loppmgr branch and appears to
be timing related as well. Might be responsible for some unexpected
transmission failures e.g. on zone transfers.
(cherry picked from commit 8585b92f98)
In some operations - most prominently when establishing connection -
it might be beneficial to bail out earlier when the network manager
is stopping.
The issue is backported from loopmgr branch, where such a change is
not only beneficial, but required.
(cherry picked from commit fc74b15e67)
In some cases - in particular, in case of errors, NULL might be passed
to a connection callback instead of a handle that could have led to
an abort. This commit ensures that such a situation will not occur.
The issue was found when working on the loopmgr branch.
(cherry picked from commit ac4fb34f18)
This commit ensures that the underlying TCP socket of a TLS connection
gets closed earlier whenever there are no pending operations on it.
In the loop-manager branch, in some circumstances the connection
could have remained opened for far too long for no reason. This
commit ensures that will not happen.
(cherry picked from commit 88524e26ec)
This commit adds a proper implementation of
isc_nmhandle_setwritetimeout() for TLS connections. Now it passes the
value to the underlying TCP handle.
(cherry picked from commit 237ce05b89)
it's a style violation to have REQUIRE or INSIST contain code that
must run for the server to work. this was being done with some
atomic_compare_exchange calls. these have been cleaned up. uses
of atomic_compare_exchange in assertions have been replaced with
a new macro atomic_compare_exchange_enforced, which uses RUNTIME_CHECK
to ensure that the exchange was successful.
(cherry picked from commit a499794984)
This commit ensures that on reconfiguration the set of HTTP
endpoints (=paths) is being updated within HTTP listeners.
(cherry picked from commit d2e13ddf22)
This commit separates TLS context creation code from xfrin_start() as
it has become too large and hard to follow into a new
function (similarly how it is done in dighost.c)
The dead code has been removed from the cleanup section of the TLS
creation code:
* there is no way 'tlsctx' can equal 'found';
* there is no way 'sess_cache' can be non-NULL in the cleanup section.
Also, it fixes a bug in the older version of the code, where TLS
client session context fetched from the cache would not get passed to
isc_nm_tlsdnsconnect().
(cherry picked from commit 98f758ed4f)
The recently added TLS client session cache used
SSL_SESSION_is_resumable() to avoid polluting the cache with
non-resumable sessions. However, it turned out that we cannot provide
a shim for this function across the whole range of OpenSSL versions
due to the fact that OpenSSL 1.1.0 does uses opaque pointers for
SSL_SESSION objects.
The commit replaces the shim for SSL_SESSION_is_resumable() with a non
public approximation of it on systems shipped with OpenSSL 1.1.0. It
is not turned into a proper shim because it does not fully emulate the
behaviour of SSL_SESSION_is_resumable(), but in our case it is good
enough, as it still helps to protect the cache from pollution.
For systems shipped with OpenSSL 1.0.X and derivatives (e.g. older
versions of LibreSSL), the provided replacement perfectly mimics the
function it is intended to replace.
(cherry picked from commit 40be3c9263)
The commit fixes a corner case in client-side DoH code, when a write
attempt is done on a closing socket (session).
The change ensures that the write call-back will be called with a
proper error code (see failed_send_cb() call in client_httpsend()).
(cherry picked from commit 9abb00bb5f)
In such a case it will return UV_EINVAL (-EINVAL), leading to
aborting, as the code expects the function to succeed.
(cherry picked from commit 245f7cec2e)
This commit extends DoT code with TLS client session resumption
support implemented on top of the TLS client session cache.
(cherry picked from commit 86465c1dac)
This commit extends TLS stream code and DoH code with TLS client
session resumption support implemented on top of the TLS client
session cache.
(cherry picked from commit 90bc13a5d5)
This commit extends TLS context cache with TLS client session cache so
that an associated session cache can be stored alongside the TLS
context within the context cache.
(cherry picked from commit 987892d113)
This commit adds an implementation of a client TLS session cache. TLS
client session cache is an object which allows efficient storing and
retrieval of previously saved TLS sessions so that they can be
resumed. This object is supposed to be a foundation for implementing
TLS session resumption - a standard technique to reduce the cost of
re-establishing a connection to the remote server endpoint.
OpenSSL does server-side TLS session caching transparently by
default. However, on the client-side, a TLS session to resume must be
manually specified when establishing the TLS connection. The TLS
client session cache is precisely the foundation for that.
(cherry picked from commit 4ef40988f3)
Before the changes from this commit were introduced, the accept
callback function will get called twice when accepting connection
during two of these stages:
* when accepting the TCP connection;
* when handshake has completed.
That is clearly an error, as it should have been called only once. As
far as I understand it the mistake is a result of TLS DNS transport
being essentially a fork of TCP transport, where calling the accept
callback immediately after accepting TCP connection makes sense.
This commit fixes this mistake. It did not have any very serious
consequences because in BIND the accept callback only checks an ACL
and updates stats.
(cherry picked from commit e616d7f240)
Under specific rare timing circumstances the uv_read_start() could
fail with UV_EINVAL when the connection is reset between the connect (or
accept) and the uv_read_start() call on the nmworker loop. Handle such
situation gracefully by propagating the errors from uv_read_start() into
upper layers, so the socket can be internally closed().
(cherry picked from commit b432d5d3bc)
