commit 45902366f69d365cedc0259ac390327506fc6d94
parent ec75766944169e42f22fd406dfcebc79038cbf61
Author: Christian Grothoff <christian@grothoff.org>
Date: Fri, 7 Aug 2026 16:51:28 +0200
-fix misc nits
Diffstat:
26 files changed, 1546 insertions(+), 438 deletions(-)
diff --git a/README b/README
@@ -35,20 +35,23 @@ How it works
`POST /.well-known/paivana` (provided by `paivana-httpd`)
with a reference to the unique payment identifier.
6. `paivana-httpd` verifies the payment with the merchant, sets
- an HMAC access cookie, and redirects the browser to the original URL.
+ an access cookie, and redirects the browser to the original URL.
The access ends at the `expiration` the browser named in step 3 and
repeated in step 5 — it is hashed into the payment identifier, so the
two must agree — bounded above by the contract's `max_pickup_time`,
which is what the merchant's `max_pickup_delay` on the template sets.
7. Requests with a valid cookie are forwarded to the upstream server
- via libcurl and the response is streamed back.
-
-The cookie is an HMAC over `(expiration time, website, client address)` keyed by a
-`paivana_secret` derived from the configured `SECRET`. `SECRET` is
-required whenever the paywall is on: it is the only input to that MAC
-the client does not supply, so a key invented afresh at each start would
-invalidate every access anyone had already paid for. Under `-n` no
-cookie is ever minted and none is needed.
+ via libcurl. Neither direction is streamed: the body is buffered in
+ full before it is passed on (see "Architecture notes").
+
+The cookie carries a keyed hash over `(expiration time, website, client
+address)` keyed by a `paivana_secret` derived from the configured
+`SECRET`. The construction is `GNUNET_CRYPTO_hkdf_gnunet()`, which is
+HMAC-based but is not itself an HMAC. `SECRET` is required whenever the
+paywall is on: it is the only input to that hash the client does not
+supply, so a key invented afresh at each start would invalidate every
+access anyone had already paid for. Under `-n` no cookie is ever minted
+and none is needed.
Keep the same `SECRET` on every `paivana-httpd` serving one site — a
cookie issued by one has to verify on the next — and treat it as
@@ -59,15 +62,19 @@ any visitor, URL and expiry.
Dependencies
------------
-- GNUnet (libgnunetutil, libgnunetcurl)
+- GNUnet (libgnunetutil, libgnunetjson, libgnunetcurl)
- libmicrohttpd
- libcurl >= 7.62.0
- libjansson
-- libgcrypt >= 1.6.1
-- zlib
-- GNU Taler: libtalerutil, libtalermerchant, libtalerexchange,
+- libgcrypt
+- GNU Taler: libtalerutil, libtalerjson, libtalermerchant,
libtalermhd, libtalertemplating
+That is what the binary links against; `readelf -d` on it is the
+authority. Paivana does not use libtalerexchange, and compresses
+nothing itself — that happens inside libtalermhd, which brings zlib
+with it.
+
Build
-----
@@ -104,12 +111,23 @@ Paivana reads an INI-style `.conf` file. The only section used is
BASE_URL Public base URL of Paivana. Required unless `-f`
is given, in which case it is derived from the
forwarding headers (see below).
- SECRET Key for the access-cookie MAC and the Paivana ID
- derivation. Required unless `-n`; see above for
- why there is no sensible default. The Debian
- package generates one into
+ SECRET Key for the access-cookie MAC. Required unless
+ `-n`; see above for why there is no sensible
+ default. It is *not* an input to the Paivana ID,
+ which the browser has to be able to recompute on
+ its own. The Debian package generates one into
/etc/paivana/secrets/paivana.secret.conf, which
is not world-readable.
+ SERVE `tcp`, `unix` (Unix-domain socket) or `systemd`
+ (socket activation). There is no default: an
+ instance without it exits at startup, and it does
+ so only after the templates have been fetched.
+ PORT TCP port. Required when SERVE = tcp.
+ UNIXPATH Path to bind to. Required when SERVE = unix.
+ UNIXPATH_MODE Access mode of that socket, octal. Required
+ when SERVE = unix — it is what governs who may
+ reach Paivana, since a Unix-domain peer is
+ trusted to report the client address.
### Optional keys
@@ -122,8 +140,10 @@ Paivana reads an INI-style `.conf` file. The only section used is
subtree.
TRUSTED_PROXIES
IPv4 networks whose members are reverse proxies trusted to
- report the client address in `X-Forwarded-For`. Only
- consulted with `-f`. See "Trusted proxies" below.
+ report the client address in `Forwarded` or
+ `X-Forwarded-For`. Only consulted with `-f`, and only
+ needed when more than one proxy is in front. See "Trusted
+ proxies" below.
TRUSTED_PROXIES6
IPv6 counterpart of TRUSTED_PROXIES.
CONNECTION_LIMIT
@@ -140,11 +160,16 @@ Paivana reads an INI-style `.conf` file. The only section used is
SERVE = unix or systemd every client shares one peer
address, and behind a reverse proxy or a NAT many clients
do, so a limit there throttles everyone at once.
- SERVE `tcp` (default) or `unix` (Unix-domain socket) or `systemd`
- (systemd socket activation).
- PORT TCP port, used when SERVE = tcp.
BIND_TO IP address to bind to; dual-stack wildcard if absent.
- UNIXPATH UNIX path do bind to, used when SERVE = unix.
+ DESTINATION_UNIXPATH
+ Unix-domain socket to reach the upstream on instead of
+ connecting to the authority in `DESTINATION_BASE_URL`. That
+ URL is still what the request line and `Host` are built
+ from, so it remains required.
+ MERCHANT_BACKEND_UNIX_PATH
+ The same for the merchant backend, relative to
+ `MERCHANT_BACKEND_URL`. An unusable value is a warning and
+ the setting is then ignored, not a startup failure.
Running
@@ -152,8 +177,29 @@ Running
$ paivana-httpd -c /etc/paivana/paivana.conf
-Pass `-n` / `--no-payment` to bypass the paywall entirely (pure reverse
-proxy, useful for testing upstream plumbing).
+Besides the options GNUnet gives every program (`-c` / `--config`,
+`-L` / `--log`, `-l` / `--logfile`, `-h`, `-v`), Paivana takes four:
+
+ -n, --no-payment Bypass the paywall entirely — a pure reverse proxy,
+ which is what the test suite runs. No cookie is
+ ever minted and `SECRET` is not required.
+ -g, --global-payment
+ One payment grants access to the whole site rather
+ than to the URL it was made for. The website goes
+ into the cookie's keyed hash as the empty string,
+ and the cookie is scoped to `/` rather than to the
+ page, so cookies minted under one setting do not
+ verify under the other: flipping it invalidates
+ whatever access is outstanding.
+ -f, --respect-forwarded-headers
+ Take the client address from the forwarding
+ headers. Only safe behind a proxy that overwrites
+ them; see "Deployment behind a reverse proxy".
+ -u, --max-upload BYTES
+ Request body to buffer before forwarding, default
+ 1048576. Raising it past the 40 MiB the
+ implementation caps every body at has no effect;
+ anything larger is refused either way.
The daemon does not serve requests until it has fetched paywall templates
from the merchant backend. If template loading fails, startup is aborted:
@@ -178,9 +224,9 @@ In that setup Paivana **must** be started with `-f` /
`--respect-forwarded-headers`. A Unix-domain peer has no address of
its own, so without `-f` there is no client address at all: the access
cookie cannot be bound to a client, and `POST /.well-known/paivana`
-fails. `-f` makes Paivana take the client address from
-`X-Forwarded-For` instead, and forward the chain it was given to the
-upstream rather than replacing it.
+fails. `-f` makes Paivana take the client address from the forwarding
+headers instead, and forward the chain it was given to the upstream
+rather than replacing it.
Paivana reads both the RFC 7239 `Forwarded` header and the de-facto
`X-Forwarded-*` ones, preferring `Forwarded` where both are present,
@@ -203,13 +249,16 @@ origin — an internal host and port, which Paivana relays unchanged.
That leaks the internal name, and points the client straight at the
origin wherever the client can route to it, bypassing the paywall.
-`-f` is only safe if the server in front **overwrites** the forwarding
-headers rather than appending to whatever the client sent. Otherwise a
-client can put any address it likes in the leftmost position and so
-choose the identity its access cookie is bound to. The configurations
-below do this; if you write your own, note that nginx does not set
-`X-Forwarded-For` at all unless told to, and that Apache's
-`ProxyAddHeaders` *appends* to a client-supplied value.
+`-f` is only safe if the server in front **writes** the forwarding
+headers itself, whether by overwriting them or by appending its own
+element. What it must not do is pass the client's copies through
+untouched: Paivana believes the rightmost element (see "Trusted
+proxies"), so a header no hop of yours has written is a header the
+client filled in, and the client then chooses the identity its access
+cookie is bound to. The configurations below get this right; if you
+write your own, note that nginx sets `X-Forwarded-For` only when told
+to and forwards a client-supplied `Forwarded` verbatim — and
+`Forwarded` is the one Paivana prefers.
Conversely, do not pass `-f` to a Paivana that clients can reach
directly — there it is the client, not a proxy, that is setting those
@@ -251,9 +300,12 @@ Apache (requires mod_proxy, mod_proxy_http and mod_headers):
RequestHeader unset X-Forwarded-Port
RequestHeader unset Forwarded
- # RFC 7239; Apache emits none of its own.
+ # RFC 7239; Apache emits none of its own. It has to be an
+ # expr= value: %{...}e reads the CGI environment, which is not
+ # populated when mod_headers runs, so the %{REMOTE_ADDR}e form
+ # yields the literal string "(null)".
RequestHeader set Forwarded \
- "for=%{REMOTE_ADDR}e;proto=%{REQUEST_SCHEME}e;host=%{HTTP_HOST}e"
+ "expr=for=%{REMOTE_ADDR};proto=%{REQUEST_SCHEME};host=%{HTTP_HOST}"
ProxyPass "unix:/run/paivana/httpd/paivana-http.sock|http://example.com/"
</Location>
@@ -270,23 +322,48 @@ place and enable it yourself.
Trusted proxies
---------------
-`-f` on its own believes every hop in `X-Forwarded-For`, including the
-leftmost entry — which is whatever the client wrote, unless the server
-in front overwrites the header. `TRUSTED_PROXIES` and
-`TRUSTED_PROXIES6` say which addresses may speak for a client, so that
-correctness no longer rests on the front server's configuration alone:
+`-f` on its own extends trust exactly one hop. The chain is walked
+from the right and the walk stops at once, so the client is the
+*rightmost* element — the one the peer we accepted the connection from
+wrote. Entries a client prepends to its own header sit to the left of
+that and cannot be promoted.
+
+What `-f` alone therefore rests on is that the server in front sets or
+appends those headers itself. Appending is safe here, because the real
+peer ends up rightmost; passing the client's own headers through
+unchanged is not, and that is the failure to watch for, since nginx
+forwards a client-supplied `Forwarded` verbatim and Paivana prefers
+`Forwarded`.
+
+`TRUSTED_PROXIES` and `TRUSTED_PROXIES6` are what let the walk step
+further left, through hops you have listed, when there is more than one
+proxy in front:
[paivana]
TRUSTED_PROXIES = 10.0.0.0/8;192.168.0.0/16;
TRUSTED_PROXIES6 = 2001:db8::/32;
-The chain is then walked from the right, skipping listed proxies; the
-first element that is not one of them is the client. A client that
-prepends entries of its own cannot promote them, because the walk stops
-at the address the trusted proxy actually reported. If the peer that
-connected is not itself listed, `X-Forwarded-For` is ignored entirely.
-A Unix-domain peer is always trusted: it has no address to match, is by
-construction on this host, and access is governed by `UNIXPATH_MODE`.
+The walk then steps over each listed proxy in turn; the first element
+that is not one of them is the client. Each step leftwards is
+permitted only by the node being stepped over, so an element written by
+someone you did not list is as far back as the chain can be believed.
+
+Two things this does *not* do. The address that connected is never
+matched against these lists — `-f` is what says the peer may speak for
+a client, and the lists only govern how far past it the walk may go.
+And an element that names no address (RFC 7239 `unknown`, an obfuscated
+identifier, a host name) is not skipped either: the walk stops there
+and Paivana falls back to the socket peer, which under `SERVE = unix`
+means no client address at all and a `POST /.well-known/paivana` that
+fails. A front server that emits a `Forwarded` element it cannot fill
+in is therefore not a degraded paywall but a broken one.
+
+`X-Forwarded-Proto`, `-Host` and `-Port` are outside all of this: they
+are read as the leftmost value of the first such field line and the
+walk never vets them. Under `-f` with no `BASE_URL` they are what
+Paivana rebuilds its own scheme and authority from, which is the
+residual reason to care about the front server's configuration even
+with `TRUSTED_PROXIES` set.
Syntax notes, inherited from GNUnet's network-policy parser:
@@ -320,14 +397,17 @@ Source layout
paivana-httpd.c Entry point, scheduler, global state, shutdown
paivana-httpd_reverse.c Request-proxying state machine (core)
paivana-httpd_pay.c POST /.well-known/paivana handler
- paivana-httpd_cookie.c HMAC access-cookie logic
+ paivana-httpd_cookie.c Access-cookie keyed hash, Paivana ID
paivana-httpd_templates.c Paywall template loading and rendering
paivana-httpd_helper.c Client IP / base URL helpers
paivana-httpd_daemon.c MHD daemon startup
paivana_pd.c GNUnet project-data descriptor
+ src/frontend/ The paywall page served to unpaid visitors
+ paywall.en.must.j2 Mustache template source (Jinja2)
+ paywall.js Payment identifier, QR code, long poll
+ generate-paywall.py Renders the Jinja2 source at build time
src/include/platform.h GNUnet-style platform header (include first)
src/tests/ Automated reverse-proxy and unit tests
- contrib/paywall.en.must.j2 Default Mustache paywall template source (Jinja2)
doc/prebuilt/ Git submodule: taler-docs (man pages)
@@ -361,6 +441,12 @@ within 60 seconds yields `504 Gateway Timeout`; one that cannot be
reached at all yields `502 Bad Gateway`. The distinction matters
because caches and monitoring retry the former and not the latter.
+Those 60 seconds are fixed and not configurable, and they are the
+effective ceiling on a whole request. The MHD connection timeout does
+not bound the phase spent waiting on the origin: the connection is
+suspended for its duration, and MHD drops suspended connections from
+its timeout lists.
+
`OPTIONS` carrying `Max-Forwards: 0` is answered by Paivana itself with
an `Allow` list, as RFC 9110 §7.6.2 requires of an intermediary; any
larger value is decremented before the request is passed on.
diff --git a/debian/examples/apache2-paivana.conf b/debian/examples/apache2-paivana.conf
@@ -13,15 +13,19 @@
<Location "/">
# paivana-httpd is started with -f (see paivana-httpd.service), so it
# takes the client address for the access cookie from the forwarding
-# headers. mod_proxy's ProxyAddHeaders (on by default) *appends* the
-# real client to any X-Forwarded-For the client itself sent, which
-# would leave the client in control of the leftmost entry -- and thus
-# of its own identity. Drop the client's copies first so that what
-# mod_proxy adds is the only thing paivana-httpd sees.
+# headers. It believes the RIGHTMOST element of the chain, which is
+# whatever the hop it accepted from wrote, so what matters is that
+# every one of these headers is written here rather than forwarded
+# from the client. mod_proxy's ProxyAddHeaders (on by default) does
+# write X-Forwarded-For -- appending is fine, our element still ends
+# up rightmost -- but it writes none of the others, and Apache
+# forwards the client's copies of those untouched. Dropping them all
+# first is what makes the rest of this block exhaustive.
#
-# If this Apache is itself behind another proxy, remove these and
+# If this Apache is itself behind another proxy, remove these,
# configure mod_remoteip (RemoteIPHeader / RemoteIPInternalProxy) for
-# that hop instead.
+# that hop, and list it in paivana's TRUSTED_PROXIES so the walk may
+# step past it.
RequestHeader unset X-Forwarded-For
RequestHeader unset X-Forwarded-Proto
RequestHeader unset X-Forwarded-Host
@@ -31,10 +35,19 @@ RequestHeader unset Forwarded
# RFC 7239, which paivana-httpd prefers over the X-Forwarded-* headers
# mod_proxy adds. Apache emits no Forwarded of its own, so build the
# element here. "set" rather than "append": this is the outermost hop,
-# so a client-supplied element must not survive. Note that
-# %{REMOTE_ADDR}e yields an unbracketed IPv6 address where RFC 7239 §6
-# asks for for="[...]"; paivana-httpd accepts both.
-RequestHeader set Forwarded "for=%{REMOTE_ADDR}e;proto=%{REQUEST_SCHEME}e;host=%{HTTP_HOST}e"
+# so a client-supplied element must not survive.
+#
+# It has to be an expr= value. mod_headers' %{NAME}e reads the CGI
+# environment, which ap_add_common_vars() fills in for a handler and
+# which is empty at the fixup stage where RequestHeader runs: the
+# %{REMOTE_ADDR}e spelling emits the literal string "(null)" for all
+# three, paivana-httpd then finds no address in the element, falls
+# back to a UNIX socket peer that has none either, and every payment
+# fails to redeem. ap_expr resolves the same names correctly.
+#
+# Note that %{REMOTE_ADDR} yields an unbracketed IPv6 address where
+# RFC 7239 §6 asks for for="[...]"; paivana-httpd accepts both.
+RequestHeader set Forwarded "expr=for=%{REMOTE_ADDR};proto=%{REQUEST_SCHEME};host=%{HTTP_HOST}"
# The path paivana-httpd.socket actually listens on, and that the
# tmpfiles snippet creates. /var/lib/paivana/httpd/paivana.sock was
diff --git a/debian/examples/nginx-paivana b/debian/examples/nginx-paivana
@@ -21,15 +21,18 @@ server {
# paivana-httpd is started with -f (see paivana-httpd.service), so
# it takes the client address for the access cookie from the
- # headers set here. That is only sound because this server is the
- # outermost hop and *overwrites* them: $remote_addr is the peer we
- # actually accepted, whereas $proxy_add_x_forwarded_for would
- # append it to whatever the client claimed, leaving the client in
- # control of the leftmost entry -- and thus of its own identity.
+ # headers set here. It believes the RIGHTMOST element of the
+ # chain, which is whatever the hop it accepted from wrote -- so
+ # what matters is that this server writes these headers at all.
+ # $remote_addr is the peer we actually accepted; the danger is not
+ # $proxy_add_x_forwarded_for (appending is safe, since our own
+ # element still ends up rightmost) but leaving a header unset and
+ # letting nginx forward the client's copy of it verbatim.
#
# If this nginx is itself behind another proxy, switch to
- # $proxy_add_x_forwarded_for and set real_ip_header /
- # set_real_ip_from for that hop.
+ # $proxy_add_x_forwarded_for, set real_ip_header /
+ # set_real_ip_from for that hop, and list it in paivana's
+ # TRUSTED_PROXIES so the walk may step past it.
proxy_set_header X-Forwarded-For $remote_addr;
proxy_set_header X-Forwarded-Proto $scheme;
proxy_set_header X-Forwarded-Host $host;
@@ -37,9 +40,11 @@ server {
# RFC 7239. paivana-httpd prefers this over the X-Forwarded-*
# headers above, which are kept for origins that only speak those.
- # Again a plain "set": nginx offers no $proxy_add_forwarded, and at
- # the outermost hop we would not want one -- a client-supplied
- # element must not survive. Behind another proxy, replace this
+ # Setting it is not optional: nginx forwards a client-supplied
+ # Forwarded header verbatim, and since paivana prefers this header
+ # over X-Forwarded-For, omitting this line would hand the client
+ # the element paivana believes -- however carefully the
+ # X-Forwarded-For above is set. Behind another proxy, replace this
# with the appending form from nginx.org's "Using the Forwarded
# header", which validates $http_forwarded before extending it.
proxy_set_header Forwarded "$paivana_forwarded_elem;proto=$scheme;host=$host";
diff --git a/src/backend/paivana-httpd.c b/src/backend/paivana-httpd.c
@@ -112,20 +112,29 @@ static struct GNUNET_CURL_RescheduleContext *proxy_ctx_rc;
* 0.0.0.0/0 or ::/0 *is* the terminator and silently truncates
* everything after it. "Trust everyone" is therefore inexpressible
* — and would be a strange thing to write anyway;
- * - a v6 address handed to the v4 parser (or a stray port policy)
- * yields a valid pointer to an empty list rather than an error;
- * - an entry that is not terminated by ';' -- the last one, when the
- * operator wrote the separators but not the terminator -- is dropped
- * without a word, and so is anything after the final ';'.
+ * - the v4 parser accepts a value whose last entry is not terminated
+ * by ';' -- the operator wrote the separators but not the
+ * terminator -- and drops that entry without a word, along with
+ * anything else after the final ';'. (Its v6 counterpart refuses
+ * such a value outright, so this one is the v4 side's alone.)
*
- * The first two come out as "parsed, but nothing usable", which we
- * reject: quietly trusting nobody would send every visitor to the
- * socket address, and the operator would have no hint why. The third
- * is worse, because it succeeds: a single missing ';' would leave the
- * clients behind an unlisted proxy sharing that proxy's address as
- * their identity, i.e. sharing one paid cookie. So the entries that
- * came back are counted against the ';' that went in, and the value
- * has to end in one.
+ * A genuinely malformed entry, including a v6 network handed to the
+ * v4 parser, does come back as NULL rather than as a shorter list; it
+ * is the only one of these that announces itself.
+ *
+ * The first comes out as "parsed, but nothing usable", which we reject
+ * along with NULL: quietly trusting nobody would send every visitor to
+ * the socket address, and the operator would have no hint why. The
+ * second is worse, because it succeeds: a single missing ';' would
+ * leave the clients behind an unlisted proxy sharing that proxy's
+ * address as their identity, i.e. sharing one paid cookie. So the
+ * entries that came back are counted against the ';' that went in, and
+ * the value has to end in one.
+ *
+ * Not checked for here: an entry may also carry a port policy
+ * ("10.0.0.0/8:80;"), which parses into a perfectly usable entry whose
+ * port range nothing in paivana ever consults -- it reads as a
+ * restriction on the trust and is not one.
*
* @param c configuration to read from
* @param option name of the option
@@ -276,16 +285,30 @@ do_shutdown (void *cls)
* Never strips below "scheme://h", so that a URL consisting of
* nothing but a scheme and a host keeps its host.
*
+ * Refuses a URL carrying a query or a fragment instead of mangling it:
+ * the trailing '/' would then be part of the query ("http://h/?p=a/")
+ * and stripping it changes what the query says. Neither has a sensible
+ * reading in a base URL we concatenate a request path onto anyway --
+ * "http://h/?p=a" + "/x" is not a request for /x -- so the operator is
+ * better told than quietly corrected.
+ *
* @param[in,out] url URL to canonicalize; must have passed
* TALER_is_web_url()
+ * @return false if @a url carries a query or a fragment, in which case
+ * it is left untouched
*/
-static void
+static bool
strip_trailing_slashes (char *url)
{
size_t len = strlen (url);
const char *sep;
size_t min_len;
+ if ( (NULL != strchr (url,
+ '?')) ||
+ (NULL != strchr (url,
+ '#')) )
+ return false;
sep = strstr (url,
"://");
GNUNET_assert (NULL != sep); /* was a web URL after all! */
@@ -293,6 +316,7 @@ strip_trailing_slashes (char *url)
while ( (len > min_len) &&
('/' == url[len - 1]) )
url[--len] = '\0';
+ return true;
}
@@ -321,13 +345,36 @@ run (void *cls,
PH_cfg = c;
GNUNET_SCHEDULER_add_shutdown (&do_shutdown,
NULL);
+ if ( (0 == PH_request_buffer_max) ||
+ (PH_request_buffer_max > GNUNET_MAX_MALLOC_CHECKED) )
+ {
+ /* 0 would reject every request that carries a body at all, which
+ is not what anyone writing it means; above the hard buffer cap
+ the number simply cannot take effect. A negative argument ends
+ up here too: GNUnet parses the option with sscanf("%llu"), which
+ reads "-1" as ULLONG_MAX rather than complaining. */
+ GNUNET_log (GNUNET_ERROR_TYPE_ERROR,
+ "-u/--max-upload must be between 1 and %llu:"
+ " request bodies are buffered whole before forwarding\n",
+ (unsigned long long) GNUNET_MAX_MALLOC_CHECKED);
+ PH_global_ret = EXIT_INVALIDARGUMENT;
+ GNUNET_SCHEDULER_shutdown ();
+ return;
+ }
if (! PH_no_check)
{
if (GNUNET_OK !=
TALER_TEMPLATING_init (PAIVANA_project_data ()))
{
- GNUNET_break (0);
- PH_global_ret = EXIT_FAILURE;
+ /* Almost always a missing or unreadable $PREFIX/share/paivana/
+ templates/ -- an installation problem, not a bug, so say so
+ instead of adding a second "Assertion failed" to the one
+ TALER_TEMPLATING_init() already logged. */
+ GNUNET_log (GNUNET_ERROR_TYPE_ERROR,
+ "Failed to load the paywall templates; is paivana"
+ " installed, and does PAIVANA_PREFIX point at the"
+ " installation?\n");
+ PH_global_ret = EXIT_NOTINSTALLED;
GNUNET_SCHEDULER_shutdown ();
return;
}
@@ -340,8 +387,6 @@ run (void *cls,
return;
}
- /* No need to check return value. If given, we take,
- * otherwise it stays zero. */
if (GNUNET_OK !=
GNUNET_CONFIGURATION_get_value_string (
c,
@@ -441,19 +486,48 @@ run (void *cls,
}
else if (PH_respect_forwarded_headers)
{
+ /* Not the same mistake in reverse: a single proxy in front, one
+ that writes the forwarding headers itself, needs no policy at
+ all. PAIVANA_HTTPD_resolve_forwarding() walks the chain from
+ the right and stops at the first hop it does not trust, so
+ with no policy it stops on its first step -- at the element
+ our peer wrote, which nothing the client prepends can displace.
+ What the policy adds is the ability to keep stepping leftwards
+ through hops that are named there, which is what a chain of
+ more than one proxy needs. The hazard worth warning about is
+ narrower, and it is upstream of us. */
GNUNET_log (GNUNET_ERROR_TYPE_WARNING,
"-f/--respect-forwarded-headers is set without TRUSTED_PROXIES:"
- " every hop in X-Forwarded-For is believed, including the"
- " leftmost entry, which the client itself controls unless the"
- " proxy in front overwrites the header\n");
+ " the client address is taken from the rightmost element of the"
+ " forwarding chain, which is the client's own address only if"
+ " the server in front sets or appends these headers itself."
+ " One that passes the client's `%s' through unchanged -- as"
+ " nginx does, and `%s' is the header we prefer over `%s' --"
+ " leaves the client choosing the element we believe."
+ " TRUSTED_PROXIES is needed only for a chain of more than one"
+ " proxy\n",
+ MHD_HTTP_HEADER_FORWARDED,
+ MHD_HTTP_HEADER_FORWARDED,
+ PH_HEADER_X_FORWARDED_FOR);
}
}
+ /* No need to check the return value. If given, we take it,
+ * otherwise it stays NULL. */
GNUNET_CONFIGURATION_get_value_filename (
c,
"paivana",
"DESTINATION_UNIXPATH",
&PH_target_server_unixpath);
- strip_trailing_slashes (PH_target_server_base_url);
+ if (! strip_trailing_slashes (PH_target_server_base_url))
+ {
+ GNUNET_log_config_invalid (GNUNET_ERROR_TYPE_ERROR,
+ "paivana",
+ "DESTINATION_BASE_URL",
+ "must not carry a query or a fragment");
+ PH_global_ret = EXIT_NOTCONFIGURED;
+ GNUNET_SCHEDULER_shutdown ();
+ return;
+ }
if (! PH_no_check)
{
if (GNUNET_OK !=
@@ -528,24 +602,43 @@ run (void *cls,
"WHITELIST",
&whitelist))
{
- char *anchored;
+ regex_t bare;
int rc;
- /* Anchor the expression: regexec(3) is unanchored, so a
- WHITELIST of "/free/" would otherwise waive payment for every
- URL merely *containing* it -- including one an attacker
- appends to a path they want for free. Wrapping in a group
- keeps alternations ("a|b") from binding the anchors to only
- the first and last branch. An expression that already
- anchors itself is unaffected, as ^ and $ inside still match
- at string start/end. */
- GNUNET_asprintf (&anchored,
- "^(%s)$",
- whitelist);
- rc = regcomp (&PH_whitelist_ex,
- anchored,
+ /* Compile the expression exactly as written first, and refuse
+ the configuration if that fails. The wrapping below is a
+ textual splice, and a value like "a)|(b" splices into
+ "^(a)|(b)$" -- which compiles, but means "^a" OR "b$", each
+ anchored on one side only. Getting out of the group that way
+ needs parentheses that do not balance on their own, and that
+ is precisely what a bare regcomp() rejects; an expression that
+ is valid by itself is unaffected and keeps matching what it
+ always did. (The merchant backend validates the website_regex
+ of a template the same way.) */
+ rc = regcomp (&bare,
+ whitelist,
REG_NOSUB | REG_EXTENDED);
- GNUNET_free (anchored);
+ if (0 == rc)
+ {
+ char *anchored;
+
+ regfree (&bare);
+ /* Anchor the expression: regexec(3) is unanchored, so a
+ WHITELIST of "/free/" would otherwise waive payment for every
+ URL merely *containing* it -- including one an attacker
+ appends to a path they want for free. Wrapping in a group
+ keeps alternations ("a|b") from binding the anchors to only
+ the first and last branch. An expression that already
+ anchors itself is unaffected, as ^ and $ inside still match
+ at string start/end. */
+ GNUNET_asprintf (&anchored,
+ "^(%s)$",
+ whitelist);
+ rc = regcomp (&PH_whitelist_ex,
+ anchored,
+ REG_NOSUB | REG_EXTENDED);
+ GNUNET_free (anchored);
+ }
if (0 != rc)
{
GNUNET_log_config_invalid (GNUNET_ERROR_TYPE_ERROR,
@@ -614,7 +707,16 @@ run (void *cls,
GNUNET_SCHEDULER_shutdown ();
return;
}
- strip_trailing_slashes (PH_base_url);
+ if (! strip_trailing_slashes (PH_base_url))
+ {
+ GNUNET_log_config_invalid (GNUNET_ERROR_TYPE_ERROR,
+ "paivana",
+ "BASE_URL",
+ "must not carry a query or a fragment");
+ PH_global_ret = EXIT_NOTCONFIGURED;
+ GNUNET_SCHEDULER_shutdown ();
+ return;
+ }
}
if (GNUNET_OK !=
@@ -681,6 +783,22 @@ run (void *cls,
GNUNET_SCHEDULER_shutdown ();
return;
}
+ if ('\0' == merchant_access_token[0])
+ {
+ /* An empty value builds "Authorization: Bearer " and the backend
+ answers 401 to every request we make with it, i.e. no template
+ ever loads and no payment is ever confirmed -- and nothing
+ says why. The option being present but blank is the shape a
+ half-finished install has. */
+ GNUNET_log_config_invalid (GNUNET_ERROR_TYPE_ERROR,
+ "paivana",
+ "MERCHANT_ACCESS_TOKEN",
+ "must not be empty");
+ GNUNET_free (merchant_access_token);
+ PH_global_ret = EXIT_NOTCONFIGURED;
+ GNUNET_SCHEDULER_shutdown ();
+ return;
+ }
/* A second context, because the credential below is appended to
*every* request the context makes: on a shared context we would
hand our merchant bearer token to the origin server (and to
diff --git a/src/backend/paivana-httpd.h b/src/backend/paivana-httpd.h
@@ -133,8 +133,12 @@ extern int PH_no_check;
* trust, and the forwarding headers ("Forwarded" and the "X-Forwarded-*"
* family) are read to recover what the client actually did. Unset, no
* forwarding header is consulted at all and the peer *is* the client.
- * Only enable it when something in front overwrites those headers
- * rather than appending to what the client sent.
+ * Only enable it when the server in front writes those headers itself,
+ * either setting them or appending to them. Appending is enough --
+ * what it appends is the address it accepted the request from, so the
+ * element we read is still one it vouches for. What is not safe is a
+ * front server that passes the client's own headers through unchanged,
+ * which leaves the client writing the element we believe.
*
* This flag alone extends trust exactly one hop, to the peer, so with
* no #PH_trusted_proxies4 / #PH_trusted_proxies6 the client is the
@@ -205,10 +209,16 @@ extern unsigned int PH_connection_limit;
extern unsigned int PH_per_ip_connection_limit;
/**
- * Maximum size (in bytes) of a request body that we will buffer
- * before forwarding it upstream. Requests exceeding this are
- * rejected with HTTP 413. Settable via the `-u` / `--max-upload`
- * command-line option; defaults to 1 MiB.
+ * Maximum size (in bytes) of a request body that we will buffer before
+ * forwarding it upstream. Requests exceeding this are rejected with
+ * HTTP 413. Settable via the `-u` / `--max-upload` command-line
+ * option; defaults to 1 MiB and cannot be set above the 40 MiB
+ * #GNUNET_MAX_MALLOC_CHECKED the buffering path enforces anyway.
+ *
+ * Bounds the *proxied* path only. Bodies sent to our own endpoints
+ * never reach this buffer: `POST /.well-known/paivana` is read by
+ * TALER_MHD_parse_post_json(), whose limit is
+ * #TALER_MHD_REQUEST_BUFFER_MAX.
*/
extern unsigned long long PH_request_buffer_max;
diff --git a/src/backend/paivana-httpd_cookie.c b/src/backend/paivana-httpd_cookie.c
@@ -39,6 +39,36 @@ struct GNUNET_HashCode paivana_secret;
/**
+ * Reduce @a expiration to the granularity the cookie is able to
+ * carry.
+ *
+ * The `Set-Cookie` value transmits the expiration in seconds while
+ * the MAC covers it in microseconds, so the two agree only if the MAC
+ * is taken over the microsecond value those transmitted seconds decode
+ * back to. For a `struct GNUNET_TIME_Timestamp` proper -- "a round
+ * number of seconds in microseconds", gnunet_time_lib.h -- that is the
+ * value itself, but nothing enforces the contract at this boundary:
+ * #GNUNET_TIME_UNIT_FOREVER_TS is UINT64_MAX microseconds, which is
+ * not a round number of seconds, and reaches us straight from the
+ * client as `{"expiration":{"t_s":"never"}}'. Deriving both the wire
+ * value and the MAC input from this one function makes the round trip
+ * an identity instead of an invariant every caller has to know about;
+ * a cookie minted for such an expiration used to verify against a
+ * different digest than the one it was signed with, so the client paid
+ * and received a cookie that could never grant it anything.
+ *
+ * @param expiration end of the access being granted
+ * @return @a expiration in seconds since the epoch
+ */
+static uint64_t
+cookie_expiration_s (struct GNUNET_TIME_Timestamp expiration)
+{
+ return expiration.abs_time.abs_value_us
+ / GNUNET_TIME_UNIT_SECONDS.rel_value_us;
+}
+
+
+/**
* Compute access cookie hash for the given @a expiration, the
* @a website and @a ca.
*
@@ -57,13 +87,24 @@ compute_cookie_hash (struct GNUNET_TIME_Timestamp expiration,
{
struct GNUNET_TIME_AbsoluteNBO e;
+ if (NULL == ca)
+ {
+ /* The client address is optional at the #PAIVANA_HTTPD_check_cookie()
+ call site, which falls back to a zero-length address when MHD
+ cannot tell it one. Both memcpy() and gcry_md_write() are
+ undefined on a NULL pointer even for a zero length, so hand them
+ something to point at; the digest sees the same nothing. */
+ GNUNET_assert (0 == ca_len);
+ ca = "";
+ }
GNUNET_log (GNUNET_ERROR_TYPE_DEBUG,
"Computing cookie for %s expiring at %llu and client %s\n",
website,
- (unsigned long long) expiration.abs_time.abs_value_us,
+ (unsigned long long) cookie_expiration_s (expiration),
TALER_b2s (ca,
ca_len));
- e = GNUNET_TIME_absolute_hton (expiration.abs_time);
+ e = GNUNET_TIME_absolute_hton (
+ GNUNET_TIME_absolute_from_s (cookie_expiration_s (expiration)));
if (PH_global_cookie)
website = "";
GNUNET_assert (GNUNET_YES ==
@@ -88,6 +129,7 @@ PAIVANA_HTTPD_check_cookie (const char *cookie,
const void *ca)
{
const char *dash;
+ char *endptr;
unsigned long long u;
struct GNUNET_HashCode h;
struct GNUNET_HashCode c;
@@ -100,25 +142,32 @@ PAIVANA_HTTPD_check_cookie (const char *cookie,
GNUNET_break_op (0);
return false;
}
- dash++;
- if (1 !=
- sscanf (cookie,
- "%llu-",
- &u))
+ /* The seconds are a bare decimal count. strtoull() on its own would
+ also take leading whitespace, a '+' or '-' sign and leading zeros
+ -- none of which we ever emit, and each of which is a second
+ spelling of a cookie we already handed out -- so the first
+ character is checked here rather than left to it. What verifies
+ the literal '-' is the comparison against @a dash: it is the one
+ thing a "%llu-" format string looks like it establishes and does
+ not. */
+ if ( (! isdigit ((unsigned char) cookie[0])) ||
+ ( ('0' == cookie[0]) &&
+ (cookie + 1 != dash) ) )
{
GNUNET_break_op (0);
return false;
}
- a.abs_time = GNUNET_TIME_absolute_from_s (u);
- if (GNUNET_TIME_absolute_is_past (a.abs_time))
+ errno = 0;
+ u = strtoull (cookie,
+ &endptr,
+ 10);
+ if ( (0 != errno) ||
+ (endptr != dash) )
{
- GNUNET_log (GNUNET_ERROR_TYPE_INFO,
- "Cookie expired %s ago\n",
- GNUNET_TIME_relative2s (
- GNUNET_TIME_absolute_get_duration (a.abs_time),
- true));
+ GNUNET_break_op (0);
return false;
}
+ dash++;
if (GNUNET_OK !=
GNUNET_STRINGS_string_to_data (dash,
strlen (dash),
@@ -128,18 +177,37 @@ PAIVANA_HTTPD_check_cookie (const char *cookie,
GNUNET_break_op (0);
return false;
}
+ a.abs_time = GNUNET_TIME_absolute_from_s (u);
compute_cookie_hash (a,
website,
ca_len,
ca,
&h);
- if (0 ==
+ if (0 !=
GNUNET_memcmp_priv (&c,
&h))
- return true;
- GNUNET_log (GNUNET_ERROR_TYPE_INFO,
- "Cookie hash does not match!\n");
- return false;
+ {
+ GNUNET_log (GNUNET_ERROR_TYPE_INFO,
+ "Cookie hash does not match!\n");
+ return false;
+ }
+ /* The expiration is examined only now that the value is known to be
+ one of ours. Testing it first was not a weakness -- it is the HKDF
+ salt, so a tampered expiration fails the MAC rather than being
+ believed -- but it acted on, and logged, a field no one had
+ authenticated yet, which let any client write a timestamp of its
+ choosing into the log by presenting a made-up cookie. The price is
+ one HKDF on a value that was going to be rejected anyway. */
+ if (GNUNET_TIME_absolute_is_past (a.abs_time))
+ {
+ GNUNET_log (GNUNET_ERROR_TYPE_INFO,
+ "Cookie expired %s ago\n",
+ GNUNET_TIME_relative2s (
+ GNUNET_TIME_absolute_get_duration (a.abs_time),
+ true));
+ return false;
+ }
+ return true;
}
@@ -341,9 +409,19 @@ PAIVANA_HTTPD_compute_cookie (struct GNUNET_TIME_Timestamp expiration,
/* RFC 6265 section 4.1.1:
set-cookie-string = cookie-pair *( ";" SP cookie-av )
-- every ';' is followed by SP and an attribute, so the value
- must not end on one. */
- PAIVANA_COOKIE_NAME "=%llu-%s; %sPath=%s; Max-Age=%llu; HttpOnly",
- (unsigned long long) (expiration.abs_time.abs_value_us / 1000LLU / 1000LLU),
+ must not end on one.
+
+ `SameSite=Lax' is what browsers already default to (RFC 6265bis
+ section 5.4.7), stated rather than inherited: this cookie is a
+ bearer credential for paid access, and a default is not the place
+ to leave that. Lax and not Strict, because the access it grants
+ is reached by top-level navigation -- the wallet sends the client
+ back to the article it just paid for -- and Strict would withhold
+ the cookie on exactly that hop and show the paywall a second
+ time. */
+ PAIVANA_COOKIE_NAME "=%llu-%s; %sPath=%s; Max-Age=%llu; HttpOnly;"
+ " SameSite=Lax",
+ (unsigned long long) cookie_expiration_s (expiration),
cstr,
use_https
? "Secure; "
diff --git a/src/backend/paivana-httpd_daemon.c b/src/backend/paivana-httpd_daemon.c
@@ -73,6 +73,23 @@ struct RequestContext
/**
+ * How many seconds MHD lets a client connection sit idle before it
+ * closes it.
+ *
+ * This is *not* a per-request ceiling: MHD takes a suspended
+ * connection off its timeout lists, and every request that waits for
+ * the upstream or the merchant backend is suspended for exactly that
+ * time. What it bounds is the phases MHD itself owns -- reading the
+ * request line and headers, reading an upload, writing the response --
+ * which is what keeps a client that opens a connection and says
+ * nothing from holding one of the #PH_connection_limit slots. The
+ * time a request may spend waiting is bounded by the libcurl timeouts
+ * in paivana-httpd_reverse.c instead.
+ */
+#define CLIENT_CONNECTION_TIMEOUT 16
+
+
+/**
* Set to true if we started a daemon.
*/
static bool have_daemons;
@@ -153,8 +170,8 @@ canonical_request_target (const char *raw,
* @param upload_data_size set initially to the size of the
* @a upload_data provided; the method must update this
* value to the number of bytes NOT processed;
- * @param con_cls pointer to location where we store the
- * 'struct Request'
+ * @param con_cls pointer to the `struct RequestContext` that
+ * mhd_log_callback() made for this request
* @return #MHD_YES if the connection was handled successfully,
* #MHD_NO if the socket must be closed due to a serious
* error while handling the request
@@ -302,9 +319,12 @@ create_response (void *cls,
website,
ca_len,
ca);
+ /* The cookie value itself is deliberately not logged: it is the
+ bearer credential the client paid for, it outlives its own
+ Max-Age in the log file, and anyone sharing the client's address
+ -- everyone behind one NAT -- could replay it. */
GNUNET_log (GNUNET_ERROR_TYPE_INFO,
- "Client sent cookie %s for %s: %s\n",
- cookie,
+ "Client sent an access cookie for %s: %s\n",
website,
ok ? "good" : "invalid");
GNUNET_free (ca);
@@ -318,8 +338,10 @@ create_response (void *cls,
if (GNUNET_SYSERR != ret)
{
GNUNET_free (website);
+ /* #GNUNET_OK only says a response was queued; besides the
+ redirect to the paywall that may be a 414 or a 400. */
GNUNET_log (GNUNET_ERROR_TYPE_INFO,
- "Payment required, sending paywall page %s\n",
+ "A paywall applies, response queued: %s\n",
(GNUNET_OK == ret) ? "ok" : "failed");
return (GNUNET_OK == ret) ? MHD_YES : MHD_NO;
}
@@ -388,8 +410,12 @@ mhd_completed_cb (void *cls,
* internal `struct HttpRequest` data structure (by checking
* for matching sockets).
*
- * @param cls the HTTP server handle (a `struct MhdHttpList`)
- * @param url the URL that is being requested
+ * @param cls unused
+ * @param url the request target exactly as it arrived on the request
+ * line: RAW, i.e. neither percent-decoded nor stripped of its
+ * query, unlike the @a url create_response() is given. That is
+ * the string we forward, and the difference between the two is
+ * why canonical_request_target() has to see both.
* @param connection MHD connection object for the request
* @return the `struct RequestContext` that this @a connection is for
*/
@@ -400,6 +426,7 @@ mhd_log_callback (void *cls,
{
struct RequestContext *rc;
+ (void) cls;
rc = GNUNET_new (struct RequestContext);
rc->connection = connection;
rc->url = GNUNET_strdup (url);
@@ -465,9 +492,18 @@ start_daemon (int lsock,
0,
NULL, NULL,
&create_response, NULL,
+ /* First, and MHD says so itself: it logs about the options it is
+ still parsing, and anything before this one would be printed by
+ its built-in logger. Without the option at all -- MHD_USE_DEBUG
+ above is the same bit as MHD_USE_ERROR_LOG -- MHD fprintf()s to
+ stderr for the lifetime of the daemon, outside GNUnet's level
+ and logfile handling entirely. Malformed request lines are
+ remotely triggerable, so that is client-influenced output
+ arriving on stderr regardless of -L ERROR. */
+ MHD_OPTION_EXTERNAL_LOGGER, &TALER_MHD_handle_logs, NULL,
MHD_OPTION_LISTEN_SOCKET,
lsock,
- MHD_OPTION_CONNECTION_TIMEOUT, (unsigned int) 16,
+ MHD_OPTION_CONNECTION_TIMEOUT, (unsigned int) CLIENT_CONNECTION_TIMEOUT,
MHD_OPTION_CONNECTION_LIMIT, climit,
MHD_OPTION_PER_IP_CONNECTION_LIMIT, PH_per_ip_connection_limit,
MHD_OPTION_NOTIFY_COMPLETED, &mhd_completed_cb, NULL,
@@ -476,11 +512,17 @@ start_daemon (int lsock,
if (NULL == mhd)
{
- GNUNET_break (0);
- PH_global_ret = EXIT_FAILURE;
+ /* Not fatal on its own: TALER_MHD_listen_bind() starts one daemon
+ per getaddrinfo() result, so an empty BIND_TO yields a v4 and a
+ v6 socket, and losing the whole process because MHD would not
+ take the v6 one is the same over-reaction the GNUNET_NO arm in
+ PAIVANA_HTTPD_serve_requests() already refuses to make for a
+ socket that would not bind. Serving on nothing at all is still
+ fatal; that is decided once, by the caller. */
+ GNUNET_log (GNUNET_ERROR_TYPE_WARNING,
+ "Failed to start HTTP daemon on one of the listen sockets\n");
/* MHD did not take ownership of the socket; close it ourselves. */
GNUNET_break (0 == close (lsock));
- GNUNET_SCHEDULER_shutdown ();
return;
}
have_daemons = true;
@@ -519,20 +561,26 @@ PAIVANA_HTTPD_serve_requests ()
switch (ret)
{
case GNUNET_SYSERR:
+ /* Configuration error; TALER_MHD_listen_bind() has diagnosed it. */
PH_global_ret = EXIT_NOTCONFIGURED;
GNUNET_SCHEDULER_shutdown ();
return;
case GNUNET_NO:
- if (! have_daemons)
- {
- PH_global_ret = EXIT_NOTCONFIGURED;
- GNUNET_SCHEDULER_shutdown ();
- return;
- }
GNUNET_log (GNUNET_ERROR_TYPE_WARNING,
"Could not open all configured listen sockets\n");
break;
case GNUNET_OK:
break;
}
+ if (! have_daemons)
+ {
+ /* One policy for both ways a listener can fail to come up: a
+ socket that would not bind, and a socket MHD would not serve
+ on. Some is enough; none is not. */
+ GNUNET_log (GNUNET_ERROR_TYPE_ERROR,
+ "Not a single listen socket came up, refusing to run\n");
+ PH_global_ret = EXIT_NOTCONFIGURED;
+ GNUNET_SCHEDULER_shutdown ();
+ return;
+ }
}
diff --git a/src/backend/paivana-httpd_helper.c b/src/backend/paivana-httpd_helper.c
@@ -1354,7 +1354,18 @@ socket_address (struct MHD_Connection *connection,
*addr_len = 0;
ci = MHD_get_connection_info (connection,
MHD_CONNECTION_INFO_CLIENT_ADDRESS);
- GNUNET_assert (NULL != ci);
+ if ( (NULL == ci) ||
+ (NULL == ci->client_addr) )
+ {
+ /* MHD documents this as NULL-able -- it returns NULL when the
+ connection carries no address at all -- and no reachable case
+ was found. Still: "a peer with no address" is a case every
+ caller of ours already handles (it is the AF_UNIX case), so
+ aborting the single-process daemon over it would turn a
+ hypothetical into an outage. */
+ GNUNET_break (0);
+ return;
+ }
sa = ci->client_addr;
switch (sa->sa_family)
{
diff --git a/src/backend/paivana-httpd_helper.h b/src/backend/paivana-httpd_helper.h
@@ -51,6 +51,13 @@
* by a NULL entry (NULL for "field not present at all"). RFC 9110
* §5.3 makes repeated field lines of a list-valued field one list, so
* every line has to be considered, not just the first.
+ *
+ * `Forwarded` and `X-Forwarded-For` say the same thing in two
+ * grammars, so only one of them is walked: `Forwarded` wherever it is
+ * present at all, `X-Forwarded-For` otherwise. The two are never
+ * merged -- a proxy that emits both and disagrees with itself would
+ * otherwise make the answer depend on the order we happened to read
+ * them in.
*/
struct PAIVANA_HTTPD_Forwarding
{
diff --git a/src/backend/paivana-httpd_pay.c b/src/backend/paivana-httpd_pay.c
@@ -134,7 +134,8 @@ struct PayRequest
struct GNUNET_TIME_Timestamp expiration;
/**
- * HTTP status to return in combination with @e resp to the client.
+ * HTTP status to return in combination with @e response to the
+ * client.
*/
unsigned int response_status;
@@ -399,14 +400,30 @@ order_status_cb (struct PayRequest *ph,
NULL,
MHD_RESPMEM_PERSISTENT);
GNUNET_assert (NULL != resp);
- GNUNET_break (MHD_YES ==
- MHD_add_response_header (resp,
- MHD_HTTP_HEADER_SET_COOKIE,
- cookie));
- GNUNET_break (MHD_YES ==
- MHD_add_response_header (resp,
- MHD_HTTP_HEADER_LOCATION,
- ph->website));
+ if ( (MHD_YES !=
+ MHD_add_response_header (resp,
+ MHD_HTTP_HEADER_SET_COOKIE,
+ cookie)) ||
+ (MHD_YES !=
+ MHD_add_response_header (resp,
+ MHD_HTTP_HEADER_LOCATION,
+ ph->website)) )
+ {
+ /* Neither header is optional: without the `Set-Cookie' the
+ client has paid and been sent back to a page that will
+ paywall it again, and without the `Location' the 303 has no
+ target at all. Answering 500 at least says so, and leaves
+ the order paid and the redemption repeatable; sending the
+ 303 anyway does not. */
+ GNUNET_break (0);
+ MHD_destroy_response (resp);
+ GNUNET_free (cookie);
+ ph->response = TALER_MHD_make_error (
+ TALER_EC_GENERIC_INTERNAL_INVARIANT_FAILURE,
+ ph->website);
+ ph->response_status = MHD_HTTP_INTERNAL_SERVER_ERROR;
+ break;
+ }
GNUNET_free (cookie);
TALER_MHD_add_global_headers (resp,
false);
@@ -421,7 +438,16 @@ order_status_cb (struct PayRequest *ph,
that actually fires -- taler-merchant-httpd_auth.c answers a bad
bearer token with 401 -- and without it this landed in the
default branch below, telling the operator that a protocol
- incompatibility should be reported to us. */
+ incompatibility should be reported to us.
+
+ Note that GANA has 9801 documented as a 502 and 9803 as a 500,
+ i.e. the two the other way round from what is sent here and
+ below. The statuses are right: RFC 9110 section 15.6.3 gives
+ 502 for "an invalid response from an inbound server", which is
+ the unexpected-status case (9803), while a bearer token of ours
+ that the backend will not take is our own misconfiguration and
+ not the upstream misbehaving (9801). Fixing the registry is a
+ change in gana, a different repository. */
GNUNET_log (GNUNET_ERROR_TYPE_ERROR,
"Merchant backend at `%s' rejected our credentials (HTTP"
" %u); check MERCHANT_ACCESS_TOKEN\n",
@@ -541,6 +567,26 @@ PAIVANA_HTTPD_payment_handle (struct PayRequest *ph,
spec);
if (GNUNET_YES != ret)
return (GNUNET_NO == ret) ? MHD_YES : MHD_NO;
+ /* `website' is a URL like any other paivana handles, and every
+ other one is refused past #PH_MAX_URL_LENGTH before anything
+ looks at it. This one arrives in a JSON body instead of a
+ request line, which is the only reason it escaped that: from
+ here it goes into an HKDF, into a `Location' and into the
+ `Path' of a `Set-Cookie', where three bytes of header are spent
+ per byte of path. Bound it in the same place and at the same
+ length. */
+ if (PH_MAX_URL_LENGTH < strlen (ph->website))
+ {
+ GNUNET_break_op (0);
+ GNUNET_log (GNUNET_ERROR_TYPE_WARNING,
+ "Refusing %llu byte `website' in payment redemption\n",
+ (unsigned long long) strlen (ph->website));
+ return TALER_MHD_reply_with_error (
+ ph->connection,
+ MHD_HTTP_BAD_REQUEST,
+ TALER_EC_GENERIC_PARAMETER_MALFORMED,
+ "website");
+ }
/* `expiration' is the end of the access being bought, not a
statement about the client's clock: it is what the cookie's
Max-Age is computed from, what check_cookie() enforces, and what
@@ -591,10 +637,32 @@ PAIVANA_HTTPD_payment_handle (struct PayRequest *ph,
ph_tail,
ph);
MHD_suspend_connection (ph->connection);
- GNUNET_assert (TALER_EC_NONE ==
- TALER_MERCHANT_get_private_order_start (ph->co,
- &order_status_cb,
- ph));
+ {
+ enum TALER_ErrorCode ec;
+
+ ec = TALER_MERCHANT_get_private_order_start (ph->co,
+ &order_status_cb,
+ ph);
+ if (TALER_EC_NONE != ec)
+ {
+ /* Everything the callee can fail on here is a resource failure
+ it recovers from by telling us: curl_easy_init() or
+ curl_multi_add_handle() came back empty. That is one client's
+ redemption going wrong, and asserting on it took the daemon
+ down with every other request in flight, paid ones included. */
+ GNUNET_break (0);
+ GNUNET_CONTAINER_DLL_remove (ph_head,
+ ph_tail,
+ ph);
+ MHD_resume_connection (ph->connection);
+ TALER_MERCHANT_get_private_order_cancel (ph->co);
+ ph->co = NULL;
+ return TALER_MHD_reply_with_error (ph->connection,
+ MHD_HTTP_INTERNAL_SERVER_ERROR,
+ TALER_EC_PAIVANA_GET_ORDER_FAILED,
+ ph->order_id);
+ }
+ }
return MHD_YES;
}
diff --git a/src/backend/paivana-httpd_pay.h b/src/backend/paivana-httpd_pay.h
@@ -55,14 +55,14 @@ PAIVANA_HTTPD_payment_create (struct MHD_Connection *connection);
/**
* Make progress on handling the upload of the client.
*
- * @param[in,out] pr payment request handle
+ * @param[in,out] ph payment request handle
* @param upload_data data uploaded by the client
* @param[in,out] upload_data_size number of bytes uploaded,
* set to the number of bytes of upload that were not handled
* @return MHD status code to return
*/
enum MHD_Result
-PAIVANA_HTTPD_payment_handle (struct PayRequest *pr,
+PAIVANA_HTTPD_payment_handle (struct PayRequest *ph,
const char *upload_data,
size_t *upload_data_size);
diff --git a/src/backend/paivana-httpd_reverse.c b/src/backend/paivana-httpd_reverse.c
@@ -45,11 +45,33 @@
* @param rc return code from curl
*/
#define LOG_CURL_EASY(level,fun,rc) \
- GNUNET_log (level, _ ("%s failed at %s:%d: `%s'\n"), fun, __FILE__, \
+ GNUNET_log (level, "%s failed at %s:%d: `%s'\n", fun, __FILE__, \
__LINE__, \
curl_easy_strerror (rc))
/**
+ * Set curl option @a opt on handle @a h to @a val, saying so when
+ * libcurl declines.
+ *
+ * Every option here used to be set and the result thrown away, so a
+ * CURLE_OUT_OF_MEMORY on CURLOPT_URL or a CURLOPT_UNIX_SOCKET_PATH
+ * libcurl was not built for left no trace at all: the transfer went on
+ * with the option simply not in effect, and the operator saw only the
+ * eventual 502. Has to stay a macro because `curl_easy_setopt()' is
+ * variadic -- @a val must reach libcurl with the type it was written
+ * with.
+ */
+#define PH_SETOPT(h,opt,val) \
+ do { \
+ CURLcode setopt_rc = curl_easy_setopt (h, opt, val); \
+ \
+ if (CURLE_OK != setopt_rc) \
+ LOG_CURL_EASY (GNUNET_ERROR_TYPE_WARNING, \
+ "curl_easy_setopt(" # opt ")", \
+ setopt_rc); \
+ } while (0)
+
+/**
* Value of `upstream_content_length` when the upstream declared no
* usable `Content-Length` for its response. Not a length any message
* can have: RFC 9110 §8.6 makes the field a count of octets, and we
@@ -68,6 +90,15 @@
/**
* How long the origin has to complete a response before we give up on
* it and answer 504.
+ *
+ * This is the *effective* per-request ceiling, and the only one there
+ * is. MHD_OPTION_CONNECTION_TIMEOUT (see `paivana-httpd_daemon.c')
+ * does not bound the phase a proxied request actually spends waiting:
+ * MHD removes a suspended connection from its timeout lists, and every
+ * request is suspended for exactly as long as the upstream takes. So
+ * changing the MHD timeout has no effect on how long a slow origin can
+ * hold a connection open; this constant, and #PH_CURL_TIMEOUT_S behind
+ * it, are what do.
*/
#define PH_UPSTREAM_TIMEOUT \
GNUNET_TIME_relative_multiply (GNUNET_TIME_UNIT_SECONDS, 60)
@@ -150,12 +181,6 @@ enum RequestState
REQUEST_STATE_PROXY_UPLOAD_STARTED,
/**
- * We're done uploading data to the proxied service.
- * MHD handling should remain suspended.
- */
- REQUEST_STATE_PROXY_UPLOAD_DONE,
-
- /**
* We've finished uploading data via CURL and can now download.
* MHD handling should remain suspended.
*/
@@ -224,16 +249,6 @@ struct HttpRequest
struct MHD_Connection *con;
/**
- * Client socket read task
- */
- struct GNUNET_SCHEDULER_Task *rtask;
-
- /**
- * Client socket write task
- */
- struct GNUNET_SCHEDULER_Task *wtask;
-
- /**
* MHD response object for this request.
*/
struct MHD_Response *response;
@@ -691,6 +706,39 @@ PAIVANA_HTTPD_reverse_shutdown (void)
/**
+ * Is @a name a well-formed HTTP field name, i.e. a non-empty `token'
+ * (RFC 9110 section 5.6.2)?
+ *
+ * What `curl_check_hdr()' calls a field name is whatever stood before
+ * the first colon of a line the upstream sent, which is only a field
+ * name if the line was a field line. An obs-fold continuation line
+ * that happens to contain a colon yields a "name" of leading
+ * whitespace and text; MHD refuses to attach it, but only after we
+ * have kept it around for the length of the request and asked.
+ *
+ * @param name candidate field name, 0-terminated
+ * @return true if @a name is a token
+ */
+static bool
+is_field_name (const char *name)
+{
+ if ('\0' == *name)
+ return false;
+ for (const char *p = name; '\0' != *p; p++)
+ {
+ if ( ( ('a' <= *p) && ('z' >= *p) ) ||
+ ( ('A' <= *p) && ('Z' >= *p) ) ||
+ ( ('0' <= *p) && ('9' >= *p) ) )
+ continue;
+ if (NULL == strchr ("!#$%&'*+-.^_`|~",
+ *p))
+ return false;
+ }
+ return true;
+}
+
+
+/**
* Is @a name a hop-by-hop HTTP header name that a proxy must not
* forward (RFC 9110 section 7.6.1 and RFC 7230, section 6.1).
* Either peer may name *additional* hop-by-hop headers in its
@@ -1077,6 +1125,20 @@ curl_check_hdr (void *buffer,
GNUNET_free (ndup);
return bytes;
}
+ if (! is_field_name (hdr_type))
+ {
+ /* Not a field line at all. Dropping it here is what keeps the
+ name out of `header_head' and out of MHD_add_response_header(),
+ which would reject it once per response. */
+ GNUNET_break_op (0);
+ GNUNET_log (GNUNET_ERROR_TYPE_WARNING,
+ "Upstream `%s' sent `%s' where a field name was due;"
+ " dropping the line\n",
+ hr->url,
+ hdr_type);
+ GNUNET_free (ndup);
+ return bytes;
+ }
hdr_val = strtok (NULL, "\n");
if (NULL == hdr_val)
{
@@ -1448,12 +1510,21 @@ curl_upload_cb (void *buf,
/* We have already drained io_buf and flipped the state in a
previous call. With CURLOPT_POSTFIELDSIZE set, libcurl
should not ask for more bytes — but if it does, signal a
- clean end-of-upload rather than aborting the transfer. */
- GNUNET_assert (0 == hr->io_len);
+ clean end-of-upload rather than aborting the transfer. Both
+ conditions describe libcurl's behaviour rather than one of our
+ own invariants, so neither may abort the daemon: this process
+ is serving every other client too. */
+ GNUNET_break (0 == hr->io_len);
return 0;
}
- GNUNET_assert (REQUEST_STATE_PROXY_UPLOAD_STARTED == hr->state);
+ if (REQUEST_STATE_PROXY_UPLOAD_STARTED != hr->state)
+ {
+ /* Some state that never arms the read callback. Refuse the body
+ rather than send whatever io_buf happens to hold. */
+ GNUNET_break (0);
+ return CURL_READFUNC_ABORT;
+ }
if (0 == hr->io_len)
{
/* Should not happen: start_curl_request runs only after the
@@ -1484,6 +1555,41 @@ curl_upload_cb (void *buf,
/* ************** helper functions ************* */
/**
+ * Append the field line @a line to the header list we will send
+ * upstream for @a hr.
+ *
+ * Replaces the `hr->headers = curl_slist_append (hr->headers, ...)'
+ * idiom, which is wrong in the one case it exists to handle:
+ * curl_slist_append() returns NULL on allocation failure *without*
+ * touching the list it was given, so the assignment both leaks every
+ * header accumulated so far and silently drops them -- the request
+ * would go upstream stripped of its Authorization, its Cookie and its
+ * Host, and the origin would answer something plausible. Losing the
+ * one line we could not allocate is the smaller error.
+ *
+ * @param[in,out] hr request whose upstream header list to extend
+ * @param line complete "Name: value" line to append
+ */
+static void
+append_curl_header (struct HttpRequest *hr,
+ const char *line)
+{
+ struct curl_slist *ext;
+
+ ext = curl_slist_append (hr->headers,
+ line);
+ if (NULL == ext)
+ {
+ GNUNET_log (GNUNET_ERROR_TYPE_WARNING,
+ "Could not append `%s' to the upstream request headers\n",
+ line);
+ return;
+ }
+ hr->headers = ext;
+}
+
+
+/**
* Build the `Host:` header line to send upstream, naming the
* authority of @a url.
*
@@ -1720,8 +1826,8 @@ con_val_iter (void *cls,
GNUNET_log (GNUNET_ERROR_TYPE_DEBUG,
"Adding header `%s' to HTTP request\n",
hdr);
- hr->headers = curl_slist_append (hr->headers,
- hdr);
+ append_curl_header (hr,
+ hdr);
GNUNET_free (hdr);
return MHD_YES;
}
@@ -1754,7 +1860,6 @@ PAIVANA_HTTPD_reverse_cleanup (struct HttpRequest *hr)
"Resetting cURL handle\n");
curl_easy_cleanup (hr->curl);
hr->curl = NULL;
- hr->io_len = 0;
}
if (NULL != hr->timeout_task)
{
@@ -1824,13 +1929,34 @@ content_length_ok (struct MHD_Connection *con)
MHD_HTTP_HEADER_CONTENT_LENGTH);
if (NULL == cl_str)
return true;
+ /* RFC 9110 §8.6: Content-Length = 1*DIGIT. strtoull() is more
+ generous than that -- it skips leading whitespace and accepts a
+ sign, so " +5" and even "-1" parse -- and the sign is the one that
+ matters here, since strtoull() maps a negative value onto a huge
+ unsigned one. Check the syntax ourselves before believing the
+ number. */
+ for (const char *p = cl_str; '\0' != *p; p++)
+ if ( ('0' > *p) ||
+ ('9' < *p) )
+ return true; /* unparseable — defer judgment to the drain path */
errno = 0;
cl = strtoull (cl_str,
&endptr,
10);
- if ( (0 != errno) ||
- ('\0' != *endptr) )
- return true; /* unparseable — defer judgment to the drain path */
+ if (endptr == cl_str)
+ return true; /* empty field value — likewise not our call */
+ if (ERANGE == errno)
+ {
+ /* All digits, but beyond ULLONG_MAX: larger than any limit we
+ could be configured with. Returning true here meant answering
+ `Content-Length: 99999999999999999999999' with a 100 Continue
+ and then buffering and discarding the body -- precisely the work
+ this function exists to skip. */
+ GNUNET_log (GNUNET_ERROR_TYPE_WARNING,
+ "Rejecting upload: Content-Length `%s' is out of range\n",
+ cl_str);
+ return false;
+ }
{
/* The buffering path also enforces the hard GNUNET_MAX_MALLOC_CHECKED
(40 MiB) cap, so a Content-Length above that will be rejected
@@ -1953,12 +2079,12 @@ forward_body_verbatim (struct HttpRequest *hr,
"Forwarding a %llu byte request body with %s\n",
(unsigned long long) hr->io_len,
meth);
- curl_easy_setopt (hr->curl,
- CURLOPT_POST,
- 1L);
- curl_easy_setopt (hr->curl,
- CURLOPT_CUSTOMREQUEST,
- meth);
+ PH_SETOPT (hr->curl,
+ CURLOPT_POST,
+ 1L);
+ PH_SETOPT (hr->curl,
+ CURLOPT_CUSTOMREQUEST,
+ meth);
hr->state = REQUEST_STATE_PROXY_UPLOAD_STARTED;
}
@@ -1986,9 +2112,9 @@ configure_curl_method (struct HttpRequest *hr,
if (0 == strcasecmp (meth,
MHD_HTTP_METHOD_GET))
{
- curl_easy_setopt (hr->curl,
- CURLOPT_HTTPGET,
- 1L);
+ PH_SETOPT (hr->curl,
+ CURLOPT_HTTPGET,
+ 1L);
/* A GET body is unusual and its semantics are undefined (RFC 9110
§9.3.1), but it is not forbidden and real APIs use it --
GraphQL-over-GET, several search endpoints. Dropping it while
@@ -2005,9 +2131,9 @@ configure_curl_method (struct HttpRequest *hr,
{
GNUNET_log (GNUNET_ERROR_TYPE_DEBUG,
"Crafting a CURL POST request\n");
- curl_easy_setopt (hr->curl,
- CURLOPT_POST,
- 1L);
+ PH_SETOPT (hr->curl,
+ CURLOPT_POST,
+ 1L);
hr->state = REQUEST_STATE_PROXY_UPLOAD_STARTED;
return MHD_YES;
}
@@ -2017,9 +2143,9 @@ configure_curl_method (struct HttpRequest *hr,
/* The upstream still states the length the equivalent GET would
have had; nothing follows it (RFC 9110 §9.3.2). */
hr->head_request = true;
- curl_easy_setopt (hr->curl,
- CURLOPT_NOBODY,
- 1L);
+ PH_SETOPT (hr->curl,
+ CURLOPT_NOBODY,
+ 1L);
/* HEAD must be forwarded with whatever body the equivalent GET
carried, or the origin cannot answer the question HEAD asks. */
forward_body_verbatim (hr,
@@ -2031,9 +2157,9 @@ configure_curl_method (struct HttpRequest *hr,
{
GNUNET_log (GNUNET_ERROR_TYPE_DEBUG,
"Crafting a CURL PUT request\n");
- curl_easy_setopt (hr->curl,
- CURLOPT_UPLOAD,
- 1L);
+ PH_SETOPT (hr->curl,
+ CURLOPT_UPLOAD,
+ 1L);
hr->state = REQUEST_STATE_PROXY_UPLOAD_STARTED;
return MHD_YES;
}
@@ -2042,15 +2168,15 @@ configure_curl_method (struct HttpRequest *hr,
{
GNUNET_log (GNUNET_ERROR_TYPE_DEBUG,
"Crafting a CURL DELETE request\n");
- curl_easy_setopt (hr->curl,
- CURLOPT_CUSTOMREQUEST,
- "DELETE");
+ PH_SETOPT (hr->curl,
+ CURLOPT_CUSTOMREQUEST,
+ "DELETE");
if (0 != hr->io_len)
{
/* DELETE with a request body is unusual but legal. */
- curl_easy_setopt (hr->curl,
- CURLOPT_POST,
- 1L);
+ PH_SETOPT (hr->curl,
+ CURLOPT_POST,
+ 1L);
hr->state = REQUEST_STATE_PROXY_UPLOAD_STARTED;
}
else
@@ -2066,12 +2192,12 @@ configure_curl_method (struct HttpRequest *hr,
"Crafting a CURL PATCH request\n");
/* CURLOPT_POST=1 turns on body upload via the read callback;
CURLOPT_CUSTOMREQUEST then overrides the verb on the wire. */
- curl_easy_setopt (hr->curl,
- CURLOPT_POST,
- 1L);
- curl_easy_setopt (hr->curl,
- CURLOPT_CUSTOMREQUEST,
- "PATCH");
+ PH_SETOPT (hr->curl,
+ CURLOPT_POST,
+ 1L);
+ PH_SETOPT (hr->curl,
+ CURLOPT_CUSTOMREQUEST,
+ "PATCH");
hr->state = REQUEST_STATE_PROXY_UPLOAD_STARTED;
return MHD_YES;
}
@@ -2080,9 +2206,9 @@ configure_curl_method (struct HttpRequest *hr,
{
const char *mf;
- curl_easy_setopt (hr->curl,
- CURLOPT_CUSTOMREQUEST,
- "OPTIONS");
+ PH_SETOPT (hr->curl,
+ CURLOPT_CUSTOMREQUEST,
+ "OPTIONS");
/* RFC 9110 §7.6.2: "Each intermediary that receives a TRACE or
OPTIONS request containing a Max-Forwards header field MUST
check and update its value prior to forwarding ... If the
@@ -2317,8 +2443,8 @@ append_forwarded_headers (struct HttpRequest *hr,
"%s: %s",
PH_HEADER_X_FORWARDED_FOR,
xff);
- hr->headers = curl_slist_append (hr->headers,
- hdr);
+ append_curl_header (hr,
+ hdr);
GNUNET_free (hdr);
}
else if (NULL != peer)
@@ -2327,13 +2453,13 @@ append_forwarded_headers (struct HttpRequest *hr,
"%s: %s",
PH_HEADER_X_FORWARDED_FOR,
peer);
- hr->headers = curl_slist_append (hr->headers,
- hdr);
+ append_curl_header (hr,
+ hdr);
GNUNET_free (hdr);
}
if (NULL != hr->max_forwards)
- hr->headers = curl_slist_append (hr->headers,
- hr->max_forwards);
+ append_curl_header (hr,
+ hr->max_forwards);
proto = forwarded_proto (hr,
con,
&owned_proto);
@@ -2341,8 +2467,8 @@ append_forwarded_headers (struct HttpRequest *hr,
"%s: %s",
PH_HEADER_X_FORWARDED_PROTO,
proto);
- hr->headers = curl_slist_append (hr->headers,
- hdr);
+ append_curl_header (hr,
+ hdr);
GNUNET_free (hdr);
if (PH_respect_forwarded_headers)
{
@@ -2365,8 +2491,8 @@ append_forwarded_headers (struct HttpRequest *hr,
"%s: %s",
PH_HEADER_X_FORWARDED_HOST,
fhost);
- hr->headers = curl_slist_append (hr->headers,
- hdr);
+ append_curl_header (hr,
+ hdr);
GNUNET_free (hdr);
}
{
@@ -2399,8 +2525,8 @@ append_forwarded_headers (struct HttpRequest *hr,
"%s: %s",
PH_HEADER_X_FORWARDED_PORT,
fport);
- hr->headers = curl_slist_append (hr->headers,
- hdr);
+ append_curl_header (hr,
+ hdr);
GNUNET_free (hdr);
}
}
@@ -2489,8 +2615,8 @@ append_forwarded_headers (struct HttpRequest *hr,
MHD_HTTP_HEADER_FORWARDED,
elem);
GNUNET_free (elem);
- hr->headers = curl_slist_append (hr->headers,
- hdr);
+ append_curl_header (hr,
+ hdr);
GNUNET_free (hdr);
}
/* MHD hands us e.g. "HTTP/1.1" but Via wants just "1.1". */
@@ -2510,8 +2636,8 @@ append_forwarded_headers (struct HttpRequest *hr,
"%s: %s paivana",
MHD_HTTP_HEADER_VIA,
via_ver);
- hr->headers = curl_slist_append (hr->headers,
- hdr);
+ append_curl_header (hr,
+ hdr);
GNUNET_free (hdr);
GNUNET_free (owned_proto);
GNUNET_free (owned_host);
@@ -2558,42 +2684,54 @@ start_curl_request (struct HttpRequest *hr,
/* No need to check whether we're POSTing or PUTting.
* If not needed, one of the following values will be
* ignored.*/
- curl_easy_setopt (hr->curl,
- CURLOPT_POSTFIELDSIZE_LARGE,
- hr->io_len);
- curl_easy_setopt (hr->curl,
- CURLOPT_INFILESIZE_LARGE,
- hr->io_len);
- curl_easy_setopt (hr->curl,
- CURLOPT_HEADERFUNCTION,
- &curl_check_hdr);
- curl_easy_setopt (hr->curl,
- CURLOPT_HEADERDATA,
- hr);
- curl_easy_setopt (hr->curl,
- CURLOPT_FOLLOWLOCATION,
- 0);
- curl_easy_setopt (hr->curl,
- CURLOPT_CONNECTTIMEOUT,
- PH_CURL_CONNECT_TIMEOUT_S);
- curl_easy_setopt (hr->curl,
- CURLOPT_TIMEOUT,
- PH_CURL_TIMEOUT_S);
- curl_easy_setopt (hr->curl,
- CURLOPT_NOSIGNAL,
- 1L);
- curl_easy_setopt (hr->curl,
- CURLOPT_PRIVATE,
- hr);
- curl_easy_setopt (hr->curl,
- CURLOPT_VERBOSE,
- 0);
- curl_easy_setopt (hr->curl,
- CURLOPT_READFUNCTION,
- &curl_upload_cb);
- curl_easy_setopt (hr->curl,
- CURLOPT_READDATA,
- hr);
+ /* The casts are load-bearing, not decoration. Both options are
+ CURLOPTTYPE_OFF_T (<curl/curl.h>), so libcurl reads them with
+ va_arg(curl_off_t), and curl_off_t is 64 bits on every platform
+ (<curl/system.h>: "For any given platform/compiler curl_off_t MUST
+ be typedef'ed to a 64-bit wide signed integral data type").
+ Passing a size_t is therefore only correct where size_t is 64 bits
+ too: on i386 or armv7 libcurl takes the four bytes of io_len and
+ four bytes of whatever the caller left on the stack next to them
+ as the high half, and declares a request body of some absurd
+ length that never arrives. */
+ PH_SETOPT (hr->curl,
+ CURLOPT_POSTFIELDSIZE_LARGE,
+ (curl_off_t) hr->io_len);
+ PH_SETOPT (hr->curl,
+ CURLOPT_INFILESIZE_LARGE,
+ (curl_off_t) hr->io_len);
+ PH_SETOPT (hr->curl,
+ CURLOPT_HEADERFUNCTION,
+ &curl_check_hdr);
+ PH_SETOPT (hr->curl,
+ CURLOPT_HEADERDATA,
+ hr);
+ PH_SETOPT (hr->curl,
+ CURLOPT_FOLLOWLOCATION,
+ 0L); /* CURLOPTTYPE_LONG: read with va_arg(long) */
+ PH_SETOPT (hr->curl,
+ CURLOPT_CONNECTTIMEOUT,
+ PH_CURL_CONNECT_TIMEOUT_S);
+ PH_SETOPT (hr->curl,
+ CURLOPT_TIMEOUT,
+ PH_CURL_TIMEOUT_S);
+ PH_SETOPT (hr->curl,
+ CURLOPT_NOSIGNAL,
+ 1L);
+ /* No CURLOPT_PRIVATE here: GNUNET_CURL_job_add_raw() sets it to its
+ own `struct GNUNET_CURL_Job *' and reads it back with
+ GNUNET_assert() on the result, so anything we stored would be
+ overwritten before the transfer starts -- and would abort the
+ daemon if it were not. */
+ PH_SETOPT (hr->curl,
+ CURLOPT_VERBOSE,
+ 0L); /* CURLOPTTYPE_LONG: read with va_arg(long) */
+ PH_SETOPT (hr->curl,
+ CURLOPT_READFUNCTION,
+ &curl_upload_cb);
+ PH_SETOPT (hr->curl,
+ CURLOPT_READDATA,
+ hr);
{
char *curlurl;
@@ -2601,9 +2739,9 @@ start_curl_request (struct HttpRequest *hr,
"%s%s",
PH_target_server_base_url,
hr->url);
- curl_easy_setopt (hr->curl,
- CURLOPT_URL,
- curlurl);
+ PH_SETOPT (hr->curl,
+ CURLOPT_URL,
+ curlurl);
GNUNET_log (GNUNET_ERROR_TYPE_DEBUG,
"Forwarding request to: %s\n",
curlurl);
@@ -2612,9 +2750,9 @@ start_curl_request (struct HttpRequest *hr,
if (NULL != PH_target_server_unixpath)
{
- curl_easy_setopt (hr->curl,
- CURLOPT_UNIX_SOCKET_PATH,
- PH_target_server_unixpath);
+ PH_SETOPT (hr->curl,
+ CURLOPT_UNIX_SOCKET_PATH,
+ PH_target_server_unixpath);
GNUNET_log (GNUNET_ERROR_TYPE_DEBUG,
"Forwarding using unixpath: %s\n",
@@ -2629,8 +2767,8 @@ start_curl_request (struct HttpRequest *hr,
{
PAIVANA_LOG_DEBUG ("Faking the host header, %s\n",
host_hdr);
- hr->headers = curl_slist_append (hr->headers,
- host_hdr);
+ append_curl_header (hr,
+ host_hdr);
GNUNET_free (host_hdr);
}
}
@@ -2680,20 +2818,22 @@ start_curl_request (struct HttpRequest *hr,
/**
- * Build the final MHD response from the accumulated upstream body
- * (or the pre-built failure page, on curl error) and queue it.
+ * Queue the response `curl_download_cb()' built for @a hr.
*
- * @param[in,out] request handle
- * @param con MHD client connection to send response on
+ * @param[in,out] hr request whose response to queue
+ * @param con MHD client connection to send the response on
+ * @return the #MHD_Result of queuing it
*/
static enum MHD_Result
finalize_response (struct HttpRequest *hr,
struct MHD_Connection *con)
{
- /* `hr->response` may already be set to `curl_failure_response` by
- the curl task on upstream failure; in that case, don't build a
- buffer response and don't attach per-request headers to the
- shared failure response. */
+ /* Reaching #REQUEST_STATE_PROXY_DOWNLOAD_DONE means
+ `curl_download_cb()' got far enough to build a response; every
+ path on which it did not sets #REQUEST_STATE_PROXY_DOWNLOAD_FAILED
+ instead, which is answered elsewhere. A NULL here is thus a bug
+ in the state machine, not an upstream failure -- but it is one the
+ client should not have to see as a hung connection. */
if (NULL == hr->response)
{
GNUNET_break (0);
@@ -2806,14 +2946,21 @@ PAIVANA_HTTPD_reverse (struct HttpRequest *hr,
ver);
case REQUEST_STATE_PROXY_UPLOAD_STARTED:
- case REQUEST_STATE_PROXY_UPLOAD_DONE:
case REQUEST_STATE_PROXY_DOWNLOAD_STARTED:
/* we should not have been resumed in this state,
how did we get here? */
GNUNET_break (0);
- GNUNET_assert (GNUNET_NO == hr->suspended);
- MHD_suspend_connection (con);
- hr->suspended = GNUNET_YES;
+ /* Whether MHD can call us here at all is MHD's business, not an
+ invariant of ours, so it must not abort the daemon. What we
+ do have to preserve is that @e suspended counts the one
+ suspend reference we hold: suspending a second time would
+ leave a reference nothing ever resumes, and the connection
+ would hang until the client gave up. */
+ if (GNUNET_NO == hr->suspended)
+ {
+ MHD_suspend_connection (con);
+ hr->suspended = GNUNET_YES;
+ }
return MHD_YES;
case REQUEST_STATE_PROXY_DOWNLOAD_DONE:
diff --git a/src/backend/paivana-httpd_reverse.h b/src/backend/paivana-httpd_reverse.h
@@ -48,7 +48,10 @@ PAIVANA_HTTPD_reverse_create (struct MHD_Connection *connection,
*
* @param hr the HTTP request context
* @param con MHD connection handle
- * @param url the url in the request
+ * @param url the url in the request (unused; the URL to forward was
+ * captured by #PAIVANA_HTTPD_reverse_create() and is kept in
+ * @a hr, but the parameter is retained to mirror the MHD
+ * access handler's signature)
* @param meth the HTTP method used ("GET", "PUT", etc.)
* @param ver the HTTP version string (i.e. "HTTP/1.1")
* @param upload_data the data being uploaded (excluding HEADERS,
diff --git a/src/backend/paivana-httpd_templates.c b/src/backend/paivana-httpd_templates.c
@@ -255,7 +255,7 @@ templates_ready (void)
if (NULL == t_head)
{
GNUNET_log (GNUNET_ERROR_TYPE_ERROR,
- "The merchant backend at `%s' offers no payment template;"
+ "The merchant backend at `%s' offers no paivana template;"
" refusing to start, as every request would then be"
" forwarded for free. Pass -n if serving the site without"
" a paywall is what you want.\n",
@@ -597,7 +597,7 @@ cache_key_language (struct MHD_Connection *conn)
* Try to initialize the paywall response.
*
* @param conn connection to create the response for
- * @param t template template to create the response for
+ * @param t template to create the response for
* @return MHD status code to return
*/
static enum MHD_Result
@@ -889,8 +889,34 @@ parse_template (struct Template *t,
('\0' != regex[0]) )
{
char *anchored;
+ regex_t bare;
int rc;
+ /* Compile the merchant's expression as it stands *before* wrapping
+ it, and refuse the template if that fails. Splicing into a
+ group is not a syntactic no-op: "a)|(b" becomes "^(a)|(b)$",
+ which is "^a" OR "b$" -- each anchored on one side only, so the
+ anchoring below stops being the guarantee the manual states.
+ Every such breakout needs a parenthesis that only balances
+ against the ones we add, which is exactly what a bare regcomp()
+ rejects. The merchant backend compiles the bare expression on
+ POST /private/templates for the same reason; this closes the gap
+ for a template that reached the database some other way. Note
+ that no expression that compiles on its own changes meaning
+ here. */
+ rc = regcomp (&bare,
+ regex,
+ REG_NOSUB | REG_EXTENDED);
+ if (0 != rc)
+ {
+ GNUNET_break_op (0);
+ GNUNET_log (GNUNET_ERROR_TYPE_ERROR,
+ "Invalid regex in template %s: %s\n",
+ t->template_id,
+ regex);
+ return false;
+ }
+ regfree (&bare);
/* Anchor the merchant's expression: regexec(3) is unanchored, so
an expression like "/premium/" would otherwise put a paywall on
every URL merely *containing* it. Wrapping in a group keeps
@@ -928,9 +954,57 @@ parse_template (struct Template *t,
/**
+ * Is @a contract a template that is ours to serve?
+ *
+ * The merchant's `template_type' discriminator; the literals are
+ * TALER_MERCHANT_template_type_from_string(), which we would call if
+ * it did not live in a library paivana does not otherwise need. Note
+ * that an absent `template_type' is `fixed-order' there, so it is not
+ * ours either.
+ *
+ * @param contract template contract from the backend
+ * @return true if this is a paivana template
+ */
+static bool
+is_paivana_template (const json_t *contract)
+{
+ const json_t *tt;
+
+ tt = json_object_get (contract,
+ "template_type");
+ return ( (NULL != tt) &&
+ (json_is_string (tt)) &&
+ (0 == strcmp ("paivana",
+ json_string_value (tt))) );
+}
+
+
+/**
+ * Remove @a t from the list of templates and free it.
+ *
+ * Only for a template we decided not to use before anything was parsed
+ * into it: @e gt must already be NULL and nothing but the ID
+ * allocated.
+ *
+ * @param[in] t template to drop
+ */
+static void
+drop_template (struct Template *t)
+{
+ GNUNET_assert (NULL == t->gt);
+ GNUNET_assert (NULL == t->rce_head);
+ GNUNET_CONTAINER_DLL_remove (t_head,
+ t_tail,
+ t);
+ GNUNET_free (t->template_id);
+ GNUNET_free (t);
+}
+
+
+/**
* Callback for a GET /private/templates/$TEMPLATE_ID request.
*
- * @param cls closure
+ * @param t template the request was about
* @param tgr response details
*/
static void
@@ -942,6 +1016,23 @@ setup_template (
switch (tgr->hr.http_status)
{
case MHD_HTTP_OK:
+ if (! is_paivana_template (tgr->details.ok.template_contract))
+ {
+ /* One merchant instance serves every kind of template, and
+ `template_type' is what says which of them are ours -- as the
+ manual promises. Without this check a fixed-order template is
+ handed to parse_template(), which fails it for want of
+ `choices' and reports "Invalid template X at field choices":
+ an error about a template that is simply none of our business,
+ and (since a parse failure is fatal) one that keeps paivana
+ from starting at all next to a perfectly good paivana
+ template. */
+ GNUNET_log (GNUNET_ERROR_TYPE_INFO,
+ "Ignoring template %s: not a paivana template\n",
+ t->template_id);
+ drop_template (t);
+ break;
+ }
if (! parse_template (t,
tgr->details.ok.template_contract))
{
@@ -976,7 +1067,7 @@ setup_template (
/**
* Callback for a GET /private/templates request.
*
- * @param cls closure
+ * @param cls unused
* @param tgr response details
*/
static void
@@ -984,15 +1075,12 @@ check_templates (
void *cls,
const struct TALER_MERCHANT_GetPrivateTemplatesResponse *tgr)
{
+ (void) cls;
gpt = NULL;
switch (tgr->hr.http_status)
{
case MHD_HTTP_OK:
break;
- case MHD_HTTP_NO_CONTENT:
- /* templates_ready() turns "none" into the diagnosis and the exit. */
- templates_ready ();
- return;
case MHD_HTTP_UNAUTHORIZED:
GNUNET_log (GNUNET_ERROR_TYPE_ERROR,
"Access to templates unauthorized: %s\n",
@@ -1020,6 +1108,7 @@ check_templates (
const struct TALER_MERCHANT_GetPrivateTemplatesTemplateEntry *te
= &tgr->details.ok.templates[i];
struct Template *t;
+ struct Template *before;
t = GNUNET_new (struct Template);
t->template_id = GNUNET_strdup (te->template_id);
@@ -1027,9 +1116,25 @@ check_templates (
t->gt = TALER_MERCHANT_get_private_template_create (PH_merchant_ctx,
PH_merchant_base_url,
t->template_id);
- GNUNET_CONTAINER_DLL_insert (t_head,
- t_tail,
- t);
+ /* Insert sorted by template ID. Where two expressions both match a
+ URL, PAIVANA_HTTPD_search_templates() quotes the price of
+ whichever template it reaches first, so this list's order is a
+ pricing decision. The backend's array arrives in whatever order
+ the database returned it -- the SELECT behind
+ GET /private/templates has no ORDER BY -- so taking it as given
+ (in either direction) makes that decision depend on nothing an
+ operator can see or set, and it can differ between two restarts
+ with no configuration change. Sorting by ID is an arbitrary
+ rule, but it is a rule, and it is one an operator can act on. */
+ before = t_head;
+ while ( (NULL != before) &&
+ (0 > strcmp (before->template_id,
+ t->template_id)) )
+ before = before->next;
+ GNUNET_CONTAINER_DLL_insert_before (t_head,
+ t_tail,
+ before,
+ t);
GNUNET_assert (
TALER_EC_NONE ==
TALER_MERCHANT_get_private_template_start (t->gt,
@@ -1167,10 +1272,21 @@ PAIVANA_HTTPD_search_templates (struct MHD_Connection *connection,
MHD_add_response_header (redirect,
MHD_HTTP_HEADER_LOCATION,
url));
+ /* Both the Location and the fragment are built from the base URL,
+ which -- unless BASE_URL pins it -- comes out of the forwarding
+ headers. A cache sitting between the terminating proxy and us
+ would otherwise serve one client's redirect to another virtual
+ host (RFC 9110 section 12.5.5). */
GNUNET_break (MHD_YES ==
MHD_add_response_header (redirect,
MHD_HTTP_HEADER_VARY,
- "Cookie"));
+ (NULL != PH_base_url)
+ ? "Cookie"
+ : "Cookie, "
+ MHD_HTTP_HEADER_FORWARDED ", "
+ PH_HEADER_X_FORWARDED_PROTO ", "
+ PH_HEADER_X_FORWARDED_HOST ", "
+ PH_HEADER_X_FORWARDED_PORT));
GNUNET_break (MHD_YES ==
MHD_add_response_header (redirect,
MHD_HTTP_HEADER_CACHE_CONTROL,
@@ -1190,14 +1306,14 @@ PAIVANA_HTTPD_search_templates (struct MHD_Connection *connection,
* Return the paywall page for the given @a template.
*
* @param connection request to search paywall response for
- * @param id template to return paywall template for
+ * @param template template to return paywall page for
* @return MHD status code
*/
enum MHD_Result
PAIVANA_HTTPD_return_template (struct MHD_Connection *connection,
const char *template)
{
- GNUNET_log (GNUNET_ERROR_TYPE_INFO,
+ GNUNET_log (GNUNET_ERROR_TYPE_DEBUG,
"Searching template `%s'\n",
template);
for (struct Template *t = t_head; NULL != t; t = t->next)
@@ -1207,7 +1323,13 @@ PAIVANA_HTTPD_return_template (struct MHD_Connection *connection,
return load_paywall (connection,
t);
}
- GNUNET_break_op (0);
+ /* No GNUNET_break_op() here: the ID is whatever the client put in the
+ path, this runs before any payment, and a stale bookmark or a
+ crawler would otherwise write an ERROR-level "Assertion failed" per
+ request. A 404 is the whole answer. */
+ GNUNET_log (GNUNET_ERROR_TYPE_INFO,
+ "No template `%s', returning 404\n",
+ template);
return TALER_MHD_reply_with_error (connection,
MHD_HTTP_NOT_FOUND,
TALER_EC_PAIVANA_TEMPLATE_UNKNOWN,
diff --git a/src/backend/paivana-httpd_templates.h b/src/backend/paivana-httpd_templates.h
@@ -44,8 +44,10 @@ PAIVANA_HTTPD_load_templates (void);
*
* @param connection request to search paywall response for
* @param website site to look for paywall templates for
- * @return #GNUNET_OK on paywall returned,
- * #GNUNET_NO to close the connection with error
+ * @return #GNUNET_OK if a response was queued -- the redirect to the
+ * paywall, but also the 414 for an over-long @a website and
+ * the 400 for a request we cannot build a base URL for,
+ * #GNUNET_NO to close the connection with error,
* #GNUNET_SYSERR if there is no paywall for @a website
*/
enum GNUNET_GenericReturnValue
@@ -57,7 +59,7 @@ PAIVANA_HTTPD_search_templates (struct MHD_Connection *connection,
* Return the paywall page for the given @a template.
*
* @param connection request to search paywall response for
- * @param id template to return paywall template for
+ * @param template template to return paywall page for
* @return MHD status code
*/
enum MHD_Result
diff --git a/src/backend/test.sh b/src/backend/test.sh
@@ -3,14 +3,32 @@
# that would work in combination with 'test.conf'.
# Run after starting taler-merchant-httpd and
# *before* starting paivana-httpd.
+#
+# This is a developer convenience, not part of `make check' --
+# nothing in meson.build refers to it, and it needs a merchant
+# backend already listening on localhost:9966.
-# Exit, with status code "skip" (no 'real' failure)
+# Report the failure and exit non-zero. (The comment here used to
+# claim this was a "skip", which is a different exit status and not
+# what the body does: a merchant that will not take these calls is a
+# real failure.)
function exit_fail() {
echo "$@" >&2
exit 1
}
+for tool in curl jq;
+do
+ if ! command -v "$tool" >/dev/null 2>&1;
+ then
+ exit_fail "$tool is required but not installed"
+ fi
+done
+
LAST_RESPONSE=$(mktemp /tmp/last-response-XXXXXX.json)
+# ...and is removed again however we leave, including through
+# exit_fail, which used to leave one behind on every run.
+trap 'rm -f "$LAST_RESPONSE"' EXIT
echo -n "Configuring merchant instance ..."
diff --git a/src/tests/README b/src/tests/README
@@ -57,13 +57,22 @@ They all implement the same canned endpoints (see "Endpoints" below).
The pipelining test client `pipeline_client` is a small C program
that talks directly to the paivana listen socket using BSD sockets.
+All five bind 127.0.0.1 and nothing else. They are not hardened in
+any way -- POST /echo reflects whatever body it is given and GET
+/large/10485760 hands out 10 MiB per request -- and they have no
+business being reachable from the network for the duration of `make
+check'. They also all reject an argument that is not a port in
+1..65535 rather than defaulting: a silent fallback binds a port the
+driver is not waiting for, and the failure then surfaces five seconds
+later as "did not start on port NNNNN", naming the wrong thing.
+
Layout of the driver
--------------------
`test_reverse_proxy.sh` is the single test program automake runs.
For each available upstream (mhd / go / py / rs) it:
- 1. starts the upstream on a fixed port,
+ 1. starts the upstream on its port (see "Ports used"),
2. starts paivana-httpd -n pointed at that upstream,
3. runs a battery of HTTP tests with curl, wget, and the raw-socket
pipelining client,
@@ -82,11 +91,33 @@ Per-upstream battery (`run_battery`):
GET /status/201 2xx response status is forwarded intact
GET /status/404 4xx response status is forwarded intact
GET /status/500 5xx response status is forwarded intact
- HEAD /hello HEAD method: headers only, no body
- GET /large/131072 128 KiB binary response body streams
- through paivana intact (length check)
+ HEAD /hello HEAD method: the status is forwarded
+ HEAD /large/131072 RFC 9110 section 9.3.2's one normative
+ requirement on HEAD -- "MUST NOT send
+ content in the response" -- on a path
+ that yields 128 KiB under GET, so there
+ is something to leak. Read off the
+ socket rather than through curl, which
+ discards a body a HEAD response has no
+ business carrying and would therefore
+ report the bug as a pass.
+ GET /large/131072 128 KiB response body arrives byte for
+ byte, compared against the 'A'..'Z'
+ cycle the upstreams generate rather
+ than merely counted. (Bodies are
+ buffered whole, not streamed: a length
+ that matches says nothing about a
+ buffer reassembled in the wrong order.)
POST /echo request body is forwarded unchanged;
body round-trip
+ POST /echo (128 KiB) the same in the request direction and
+ at a size that spans several reads:
+ 128 KiB of random bytes posted and
+ compared with what comes back. POST
+ /upload below checks only the count the
+ upstream reports, so without this
+ nothing here would notice a request
+ body that arrived complete but corrupt.
POST /upload (64 KiB) large random POST upload; upstream
reports the byte count it saw
PUT /put PUT method + body forwarding
@@ -111,7 +142,11 @@ Per-upstream battery (`run_battery`):
custom X-Test header arbitrary client request headers are
forwarded unchanged
X-Upstream response header upstream response headers survive the
- round-trip back to the client
+ round-trip back to the client, and the
+ value names the upstream this battery
+ was pointed at -- a restart that
+ silently kept the previous destination
+ would satisfy a presence check
Forwarding-header tests (run once):
@@ -169,16 +204,26 @@ Forwarding-header tests (run once):
must abort startup rather than leave
paivana matching against an
uninitialised regex_t; usable ones must
- start. Only the loading is reachable
- from here: the regexec sits behind the
+ start. Two of the refused cases are
+ the anchoring: "a)|(b" and "(a$|^b" do
+ not balance on their own, so wrapping
+ them in "^(%s)$" yields an alternation
+ that has climbed out of the group and a
+ whitelist matching far more than it
+ says. paivana compiles the value bare
+ first for that reason, which is what
+ these two reach.
+
+ The matching itself is still out of
+ reach here: the regexec sits behind the
paywall that `-n` switches off, and
without `-n` paivana needs a merchant
backend to serve it templates before it
- will start at all. So the anchoring
- that keeps a WHITELIST of "/free/" from
+ will start at all. So the anchoring at
+ *match* time -- a WHITELIST of "/free/"
waiving payment for every URL merely
- containing it still has no regression
- test.
+ containing it -- has no end-to-end
+ regression test.
Cross-cutting tests (run once):
@@ -357,9 +402,14 @@ interested in widening it, so each of the three things it is minted
for -- expiration, website, client address -- is checked to be inside
the MAC and re-checked on presentation, including the obvious attempt:
reading the expiration off the value and writing a later one. Each of
-the four ways check_cookie() can reject a value has its own case, so
-that a malformed value ends in a refusal rather than in a read past
-the end of a string the client chose. `-g` is covered here and
+the ways check_cookie() can reject a value has its own case, so that a
+malformed value ends in a refusal rather than in a read past the end
+of a string the client chose. A separate group covers the values that
+are not malformed at all but merely respelled -- a leading '+', a
+leading space, a leading zero, junk between the seconds and the '-' --
+each of which decodes to the same seconds and the same hash as a
+cookie we really issued, and so is a second live spelling of one
+credential unless the parser refuses it. `-g` is covered here and
nowhere else.
The `paivana_id` is pinned against a golden vector computed
diff --git a/src/tests/early_response_upstream.c b/src/tests/early_response_upstream.c
@@ -53,6 +53,7 @@
#endif
#include "platform.h"
#include <gnunet/gnunet_util_lib.h>
+#include <limits.h>
#include <poll.h>
#include <strings.h>
@@ -106,6 +107,10 @@ read_until_eoh (int fd,
size_t *eoh)
{
size_t pos = 0;
+ /* First offset not yet examined. Carried across the read loop:
+ restarting the scan at 0 after every read would be quadratic in
+ the size of the header block. */
+ size_t scan = 0;
while (pos < cap)
{
@@ -115,18 +120,17 @@ read_until_eoh (int fd,
if (n <= 0)
return -1;
pos += (size_t) n;
- if (pos < 4)
- continue;
- for (size_t i = 0; i + 3 < pos; i++)
+ while (scan + 3 < pos)
{
- if ( ('\r' == buf[i]) &&
- ('\n' == buf[i + 1]) &&
- ('\r' == buf[i + 2]) &&
- ('\n' == buf[i + 3]) )
+ if ( ('\r' == buf[scan]) &&
+ ('\n' == buf[scan + 1]) &&
+ ('\r' == buf[scan + 2]) &&
+ ('\n' == buf[scan + 3]) )
{
- *eoh = i + 4;
+ *eoh = scan + 4;
return (ssize_t) pos;
}
+ scan++;
}
}
return -1;
@@ -173,6 +177,13 @@ find_content_length (const char *hdr,
('0' <= hdr[j]) &&
('9' >= hdr[j]) )
{
+ /* Signed overflow is undefined, and the value comes off the
+ wire: refuse a length we cannot represent rather than wrap
+ into a negative (or, with optimisation, into anything at
+ all). We have no use for one either -- the caller reads at
+ most what it announced. */
+ if (v > (LLONG_MAX - (hdr[j] - '0')) / 10)
+ return -1;
v = v * 10 + (hdr[j] - '0');
digits = true;
j++;
diff --git a/src/tests/pipeline_client.c b/src/tests/pipeline_client.c
@@ -172,18 +172,21 @@ read_headers (int fd,
size_t *have,
size_t start)
{
+ /* First offset not yet examined. Carried across the read loop:
+ restarting the scan at @a start after every read would be
+ quadratic in the size of the header block. */
+ size_t scan = start;
+
while (1)
{
- if (*have >= start + 4)
+ while (scan + 3 < *have)
{
- for (size_t i = start; i + 3 < *have; i++)
- {
- if ( ('\r' == (*buf)[i]) &&
- ('\n' == (*buf)[i + 1]) &&
- ('\r' == (*buf)[i + 2]) &&
- ('\n' == (*buf)[i + 3]) )
- return i + 4;
- }
+ if ( ('\r' == (*buf)[scan]) &&
+ ('\n' == (*buf)[scan + 1]) &&
+ ('\r' == (*buf)[scan + 2]) &&
+ ('\n' == (*buf)[scan + 3]) )
+ return scan + 4;
+ scan++;
}
if (ensure_bytes (fd,
buf,
@@ -260,11 +263,13 @@ header_status (const char *buf,
int
main (int argc, char **argv)
{
- const char *host = argv[1];
- const char *port = argv[2];
- int n_paths = argc - 3;
+ const char *host;
+ const char *port;
+ int n_paths;
int fd;
+ /* Before argc is checked: with argc == 1, argv[2] is a read one
+ past the end of the argument vector. */
if (argc < 4)
{
fprintf (stderr,
@@ -272,6 +277,9 @@ main (int argc, char **argv)
argv[0]);
return 2;
}
+ host = argv[1];
+ port = argv[2];
+ n_paths = argc - 3;
fd = connect_to (host,
port);
diff --git a/src/tests/test_client_address.c b/src/tests/test_client_address.c
@@ -601,12 +601,32 @@ same_identity (const char *a,
size_t ca_len;
size_t cb_len;
- GNUNET_assert (resolve_xff (a,
- &ca,
- &ca_len));
- GNUNET_assert (resolve_xff (b,
- &cb,
- &cb_len));
+ /* Not GNUNET_assert: these are the code under test, not this
+ test's own inputs. A regression that makes a spelling
+ unresolvable would abort here and print one backtrace, where the
+ whole point of this file is to run every check and show the
+ shape of the damage. */
+ if (! resolve_xff (a,
+ &ca,
+ &ca_len))
+ {
+ fprintf (stderr,
+ "FAIL: `%s' does not resolve at all\n",
+ a);
+ failures++;
+ return;
+ }
+ if (! resolve_xff (b,
+ &cb,
+ &cb_len))
+ {
+ fprintf (stderr,
+ "FAIL: `%s' does not resolve at all\n",
+ b);
+ failures++;
+ GNUNET_free (ca);
+ return;
+ }
if ( (ca_len != cb_len) ||
(0 != memcmp (ca,
cb,
@@ -1825,18 +1845,25 @@ main (int argc,
fprintf (stderr,
"-- distinct hosts stay distinct --\n");
{
- void *a;
- void *b;
- size_t al;
- size_t bl;
-
- GNUNET_assert (resolve_xff ("203.0.113.7",
- &a,
- &al));
- GNUNET_assert (resolve_xff ("203.0.113.8",
- &b,
- &bl));
- if (cookie_survives (a,
+ void *a = NULL;
+ void *b = NULL;
+ size_t al = 0;
+ size_t bl = 0;
+
+ /* Again: a resolver that stopped understanding dotted quads is a
+ regression to report, not a reason to abort the run. */
+ if ( (! resolve_xff ("203.0.113.7",
+ &a,
+ &al)) ||
+ (! resolve_xff ("203.0.113.8",
+ &b,
+ &bl)) )
+ {
+ fprintf (stderr,
+ "FAIL: 203.0.113.7 / 203.0.113.8 do not resolve at all\n");
+ failures++;
+ }
+ else if (cookie_survives (a,
al,
b,
bl))
diff --git a/src/tests/test_cookie_access.c b/src/tests/test_cookie_access.c
@@ -34,9 +34,9 @@
* - the website it was minted for,
* - the client address it was minted for
*
- * has to be inside the MAC and has to be re-checked, and each of the
- * four ways a value can be malformed has to end in a rejection rather
- * than in a read of something that is not there. None of this is
+ * has to be inside the MAC and has to be re-checked, and every way a
+ * value can be malformed has to end in a rejection rather than in a
+ * read of something that is not there. None of this is
* reachable from the integration suite: that runs paivana with -n,
* where do_forward is set before the request is even looked at and the
* cookie path is never entered.
@@ -472,8 +472,10 @@ main (int argc,
fprintf (stderr,
"-- malformed values are refused, not parsed --\n");
/* Every one of these is a string an attacker can put in a Cookie
- header, and each takes a different one of the four exits in
- check_cookie(). */
+ header, and between them they take every exit check_cookie() has
+ before it gets as far as comparing MACs: no separator at all, a
+ seconds field that is not a bare decimal count ending at the
+ separator, and a hash that does not decode. */
refuses ("the empty value",
"");
refuses ("a value with no '-' at all",
@@ -511,6 +513,57 @@ main (int argc,
GNUNET_free (bad);
}
{
+ /* A cookie of ours, respelled. These are not malformed values in
+ the sense above -- every one of them decodes to the same seconds
+ and the same hash as a value we really issued, so a parser that
+ accepts them accepts a second, third and fourth spelling of one
+ live credential. That is what a bare `sscanf ("%llu-")' did: it
+ took a leading '+', leading whitespace and leading zeros, and its
+ trailing '-' verified nothing, so anything at all could sit
+ between the seconds and the separator. A cookie has exactly one
+ spelling or the set of strings that grant access is not the set
+ we minted. */
+ char *good = mint_default ();
+ const char *gdash = strchr (good,
+ '-');
+ char *bad;
+
+ GNUNET_assert (NULL != gdash);
+ GNUNET_asprintf (&bad,
+ "+%s",
+ good);
+ refuses ("a live cookie with a '+' before the seconds",
+ bad);
+ GNUNET_free (bad);
+ GNUNET_asprintf (&bad,
+ " %s",
+ good);
+ refuses ("a live cookie with a space before the seconds",
+ bad);
+ GNUNET_free (bad);
+ GNUNET_asprintf (&bad,
+ "0%s",
+ good);
+ refuses ("a live cookie with a leading zero on the seconds",
+ bad);
+ GNUNET_free (bad);
+ GNUNET_asprintf (&bad,
+ "%.*sx%s",
+ (int) (gdash - good),
+ good,
+ gdash);
+ refuses ("a live cookie with junk between seconds and '-'",
+ bad);
+ GNUNET_free (bad);
+ GNUNET_asprintf (&bad,
+ "99999999999999999999999%s",
+ gdash);
+ refuses ("a seconds count that does not fit",
+ bad);
+ GNUNET_free (bad);
+ GNUNET_free (good);
+ }
+ {
char *val2;
char *swapped;
const char *dash;
diff --git a/src/tests/test_reverse_proxy.sh b/src/tests/test_reverse_proxy.sh
@@ -372,14 +372,17 @@ function test_get() {
|| { fail "curl: $(cat "$SCRATCH/err")"; }
status="$out"
[ "$status" = "$want_status" ] || fail "status=$status want=$want_status"
- if [ -n "$want_sub" ] && ! grep -q -- "$want_sub" "$SCRATCH/body";
+ # -F: $want_sub is a substring, not a pattern. Without it a '.'
+ # in an expected body matches anything and the check passes on a
+ # body it should have rejected.
+ if [ -n "$want_sub" ] && ! grep -qF -- "$want_sub" "$SCRATCH/body";
then
fail "body missing substring '$want_sub' (got: $(tr -d '\n' <"$SCRATCH/body" | head -c 120))"
fi
ok
}
-# HEAD
+# HEAD. Status only; the "no content" half is test_head_no_body().
function test_head() {
local desc="$1" path="$2" want_status="$3"
msg "$desc"
@@ -390,6 +393,65 @@ function test_head() {
ok
}
+# RFC 9110 section 9.3.2's one normative requirement on HEAD -- "the
+# server MUST NOT send content in the response" -- read off the wire
+# rather than through curl, which discards a body a HEAD response has
+# no business carrying and would therefore report the bug as a pass.
+# The path is one that produces 128 KiB under GET, so a proxy that
+# forgot the method has something to leak.
+function test_head_no_body() {
+ local label="$1"
+ msg "[$label] HEAD /large/131072 carries no content (RFC 9110 9.3.2)"
+ raw_head "127.0.0.1" "$PAIVANA_PORT" "$SCRATCH/head_raw"
+ case "$(head -c 12 "$SCRATCH/head_raw")" in
+ 'HTTP/1.1 200'*) ;;
+ *) fail "expected 200 status line, got: $(head -c 80 "$SCRATCH/head_raw")";;
+ esac
+ # Bytes after the first empty line. Zero for a conforming HEAD
+ # response; anything else is content that must not be there.
+ local extra
+ extra="$(tr -d '\r' <"$SCRATCH/head_raw" \
+ | awk 'seen { n += length ($0) + 1 }
+ /^$/ && ! seen { seen = 1 }
+ END { print n + 0 }')"
+ [ "$extra" = "0" ] || \
+ fail "HEAD response carried $extra bytes of content"
+ ok
+}
+
+# Raw HEAD /large/131072 against $1:$2, whole response into $3. Raw
+# rather than curl, which discards a body a HEAD response has no
+# business carrying and would report the bug as a pass.
+function raw_head() {
+ local host="$1" port="$2" out="$3"
+
+ ( exec 3<>"/dev/tcp/$host/$port"
+ printf 'HEAD /large/131072 HTTP/1.1\r\nHost: %s:%s\r\nConnection: close\r\n\r\n' \
+ "$host" "$port" >&3
+ timeout 10 cat <&3 >"$out" ) 2>"$SCRATCH/err" \
+ || fail "raw HEAD to $host:$port failed: $(cat "$SCRATCH/err")"
+}
+
+# Write @2 with @1 bytes of the 'A'..'Z' cycle every upstream serves
+# from /large/N, so that the response can be compared byte for byte
+# rather than merely counted. The chunk stays a whole number of
+# 26-byte cycles, which is what keeps it aligned when repeated.
+function make_large_pattern() {
+ local want="$1" out="$2"
+ local chunk='ABCDEFGHIJKLMNOPQRSTUVWXYZ'
+ local n i
+
+ while [ "${#chunk}" -lt 65536 ];
+ do
+ chunk="$chunk$chunk"
+ done
+ n=$(( (want + ${#chunk} - 1) / ${#chunk} ))
+ for ((i = 0; i < n; i++));
+ do
+ printf '%s' "$chunk"
+ done | head -c "$want" >"$out"
+}
+
# Generic method with optional body; checks status and body substring.
function test_method() {
local desc="$1" method="$2" path="$3" body="$4" want_status="$5" want_sub="$6"
@@ -404,7 +466,7 @@ function test_method() {
|| fail "curl: $(cat "$SCRATCH/err")"
[ "$status" = "$want_status" ] || \
fail "status=$status want=$want_status; body=$(head -c 200 "$SCRATCH/body")"
- if [ -n "$want_sub" ] && ! grep -q -- "$want_sub" "$SCRATCH/body";
+ if [ -n "$want_sub" ] && ! grep -qF -- "$want_sub" "$SCRATCH/body";
then
fail "body missing substring '$want_sub' (got: $(head -c 200 "$SCRATCH/body"))"
fi
@@ -432,14 +494,23 @@ function run_battery() {
/status/500 500 "status 500"
test_head "[$label] HEAD /hello" /hello 200
-
- # Medium response body (128 KiB): exercises streaming download
+ test_head_no_body "$label"
+
+ # 128 KiB response body, compared byte for byte against the
+ # 'A'..'Z' cycle every upstream generates. A length check alone
+ # would pass on a body that arrived complete but scrambled --
+ # a chunk delivered twice and another dropped, say -- which is
+ # precisely the failure a buffering proxy has to be shown not to
+ # have.
test_get "[$label] GET /large/131072 (128 KiB response)" \
/large/131072 200 ""
+ msg "[$label] 128 KiB response body is byte-for-byte intact"
local sz
sz="$(wc -c <"$SCRATCH/body" | tr -d ' ')"
- msg "[$label] verify 128 KiB body length"
[ "$sz" = "131072" ] || fail "got $sz bytes, expected 131072"
+ make_large_pattern 131072 "$SCRATCH/large_want"
+ cmp -s "$SCRATCH/large_want" "$SCRATCH/body" || \
+ fail "128 KiB body differs from the expected pattern at $(cmp "$SCRATCH/large_want" "$SCRATCH/body" 2>&1 | head -1)"
ok
# POST /echo — round-trip body
@@ -447,6 +518,24 @@ function run_battery() {
test_method "[$label] POST /echo (body round-trip)" \
POST /echo "$SCRATCH/post_body" 200 "hello-payload-$label"
+ # The same in the request direction, and at a size that does not
+ # fit in one buffer: 128 KiB of random bytes posted to /echo and
+ # compared with what comes back. POST /upload below checks only
+ # the count the upstream reports, so without this nothing in the
+ # suite would notice a request body that arrived complete but
+ # corrupt.
+ msg "[$label] POST /echo 128 KiB round-trips byte for byte"
+ dd if=/dev/urandom of="$SCRATCH/echo_big" bs=1024 count=128 status=none
+ local estatus
+ estatus="$(curl -sS -X POST --data-binary "@$SCRATCH/echo_big" \
+ -o "$SCRATCH/body" -w '%{http_code}' \
+ "$(PAIVANA_URL /echo)" 2>"$SCRATCH/err")" \
+ || fail "curl: $(cat "$SCRATCH/err")"
+ [ "$estatus" = "200" ] || fail "status=$estatus want=200"
+ cmp -s "$SCRATCH/echo_big" "$SCRATCH/body" || \
+ fail "echoed 128 KiB body differs: $(cmp "$SCRATCH/echo_big" "$SCRATCH/body" 2>&1 | head -1)"
+ ok
+
# POST /upload — byte count
dd if=/dev/urandom of="$SCRATCH/rnd" bs=1024 count=64 status=none
test_method "[$label] POST /upload (64 KiB binary upload)" \
@@ -482,7 +571,7 @@ function run_battery() {
curl -sS -o "$SCRATCH/body" "$(PAIVANA_URL /echo-headers)" 2>"$SCRATCH/err" \
|| fail "curl: $(cat "$SCRATCH/err")"
grep -qi '^x-forwarded-for:' "$SCRATCH/body" || \
- fail "upstream did not see X-Forwarded-For; headers:\n$(cat "$SCRATCH/body")"
+ fail "upstream did not see X-Forwarded-For; headers:"$'\n'"$(cat "$SCRATCH/body")"
grep -qi '^x-forwarded-proto:' "$SCRATCH/body" || \
fail "upstream did not see X-Forwarded-Proto"
grep -qi '^via:' "$SCRATCH/body" || \
@@ -500,7 +589,7 @@ function run_battery() {
seen_host="$(grep -i '^host:' "$SCRATCH/body" | tr -d '\r' | \
sed -e 's/^[Hh][Oo][Ss][Tt]: *//')"
[ -n "$seen_host" ] || \
- fail "upstream saw no Host header; headers:\n$(cat "$SCRATCH/body")"
+ fail "upstream saw no Host header; headers:"$'\n'"$(cat "$SCRATCH/body")"
[ "$seen_host" = "$want_host" ] || \
fail "upstream saw 'Host: $seen_host', want 'Host: $want_host'"
ok
@@ -618,12 +707,22 @@ function run_battery() {
fail "X-Keep-Resp (not named in Connection) was incorrectly dropped"
ok
- # Response header passthrough: upstream sets X-Upstream.
+ # Response header passthrough: upstream sets X-Upstream. Its
+ # *value* is checked against the label, not merely its presence:
+ # the point of the header is to confirm which of the four servers
+ # answered, and every case in this battery is run against a
+ # paivana we have just re-pointed. A restart that silently kept
+ # the previous destination would pass a presence check.
msg "[$label] upstream response header X-Upstream is forwarded"
curl -sS -D "$SCRATCH/hdrs" -o /dev/null "$(PAIVANA_URL /hello)" 2>"$SCRATCH/err" \
|| fail "curl: $(cat "$SCRATCH/err")"
- grep -qi '^x-upstream:' "$SCRATCH/hdrs" || \
+ local seen_upstream
+ seen_upstream="$(grep -i '^x-upstream:' "$SCRATCH/hdrs" | tr -d '\r' | \
+ sed -e 's/^[^:]*: *//')"
+ [ -n "$seen_upstream" ] || \
fail "X-Upstream header not forwarded back to client"
+ [ "$seen_upstream" = "$label" ] || \
+ fail "X-Upstream='$seen_upstream', want '$label' (answered by the wrong upstream)"
ok
}
@@ -759,17 +858,28 @@ function stop_early_upstream() {
kill -TERM "$EARLY_PID" 2>/dev/null
# In --no-drain mode the upstream may be parked on a
# connection whose peer never hangs up, so back the TERM with
- # a KILL: a wedged helper must not wedge the whole suite. The
- # watchdog has to be cancelled once the wait returns -- which
- # is the normal case -- or two seconds later it delivers a
- # SIGKILL to whatever process has inherited that pid by then,
- # quite possibly one of this suite's own later helpers, failing
- # a test downstream with no visible cause.
- ( sleep 2; kill -KILL "$EARLY_PID" 2>/dev/null ) &
- local watchdog=$!
+ # a KILL after two seconds: a wedged helper must not wedge the
+ # whole suite.
+ #
+ # Polled here rather than armed as `( sleep 2; kill -KILL ) &'
+ # and cancelled afterwards. That subshell inherits this
+ # script's EXIT trap, and the `kill -TERM' that cancels it
+ # makes bash run that trap *in the subshell*: cleanup() then
+ # rm -rf's the scratch directory and TERMs every helper while
+ # the suite is still running, and the run dies several checks
+ # later reading "logs/early-<port>.log: No such file or
+ # directory". Reproduced in isolation, and seen once in
+ # roughly twenty runs of the suite -- only when the wait
+ # really does take the full two seconds, which is the case
+ # the watchdog exists for.
+ local tries=20
+ while [ "$tries" -gt 0 ] && kill -0 "$EARLY_PID" 2>/dev/null;
+ do
+ sleep 0.1
+ tries=$((tries - 1))
+ done
+ kill -KILL "$EARLY_PID" 2>/dev/null
wait "$EARLY_PID" 2>/dev/null
- kill -TERM "$watchdog" 2>/dev/null
- wait "$watchdog" 2>/dev/null
EARLY_PID=""
fi
}
@@ -991,7 +1101,7 @@ function test_keepalive_curl() {
|| fail "curl: $(cat "$SCRATCH/err")"
local count
count="$(printf '%s\n' "$out" | grep -c '^Hello from')"
- [ "$count" = "3" ] || fail "got $count Hello lines; want 3; out:\n$out"
+ [ "$count" = "3" ] || fail "got $count Hello lines; want 3; out:"$'\n'"$out"
ok
}
@@ -1022,16 +1132,16 @@ function test_pipelined() {
printf '%s\n' "$out" >"$SCRATCH/pipeline.out"
local n
n="$(grep -c '^--- response' "$SCRATCH/pipeline.out")"
- [ "$n" = "4" ] || fail "got $n responses, want 4; output:\n$out"
+ [ "$n" = "4" ] || fail "got $n responses, want 4; output:"$'\n'"$out"
# Order preserved: responses must match the request sequence.
grep -q '^--- response 0: status=200' "$SCRATCH/pipeline.out" \
- || fail "response 0: wrong status; out:\n$out"
+ || fail "response 0: wrong status; out:"$'\n'"$out"
grep -q '^--- response 1: status=201' "$SCRATCH/pipeline.out" \
- || fail "response 1: wrong status; out:\n$out"
+ || fail "response 1: wrong status; out:"$'\n'"$out"
grep -q '^--- response 2: status=200' "$SCRATCH/pipeline.out" \
- || fail "response 2: wrong status; out:\n$out"
+ || fail "response 2: wrong status; out:"$'\n'"$out"
grep -q '^--- response 3: status=404' "$SCRATCH/pipeline.out" \
- || fail "response 3: wrong status; out:\n$out"
+ || fail "response 3: wrong status; out:"$'\n'"$out"
ok
}
@@ -1153,7 +1263,7 @@ function test_forwarded_unix() {
[ "$xff" = "203.0.113.7" ] || \
fail "X-Forwarded-For='$xff', want '203.0.113.7' (unadorned)"
grep -qi '^via:.*paivana' "$SCRATCH/body" || \
- fail "Via does not record the paivana hop; headers:\n$(cat "$SCRATCH/body")"
+ fail "Via does not record the paivana hop; headers:"$'\n'"$(cat "$SCRATCH/body")"
ok
msg "unix socket, no -f: no X-Forwarded-For is invented"
@@ -1164,7 +1274,7 @@ function test_forwarded_unix() {
-o "$SCRATCH/body" http://localhost/echo-headers 2>"$SCRATCH/err" \
|| fail "curl: $(cat "$SCRATCH/err")"
grep -qi '^x-forwarded-for:' "$SCRATCH/body" && \
- fail "upstream saw an X-Forwarded-For we cannot substantiate:\n$(cat "$SCRATCH/body")"
+ fail "upstream saw an X-Forwarded-For we cannot substantiate:"$'\n'"$(cat "$SCRATCH/body")"
grep -qi '^via:.*paivana' "$SCRATCH/body" || \
fail "Via does not record the paivana hop"
ok
@@ -1343,7 +1453,7 @@ function test_trusted_proxies_config() {
msg "startup accepted: $good"
r="$(paivana_with_config_line "$good")"
[ "$r" = "started" ] || \
- fail "paivana refused a usable policy ($good); log:\n$(cat "$LOGDIR/startup.log")"
+ fail "paivana refused a usable policy ($good); log:"$'\n'"$(cat "$LOGDIR/startup.log")"
ok
done
@@ -1366,6 +1476,15 @@ function test_trusted_proxies_config() {
# templates. So these cases pin that an unusable expression is
# refused rather than carried into the process -- the alternative
# being a paivana that runs with an uninitialised regex_t.
+#
+# The last two bad values are the anchoring itself, as far as it can
+# be reached from here. "a)|(b" and "(a$|^b" do not balance on their
+# own, so wrapping them yields "^(a)|(b)$" and "^((a$|^b))$" -- an
+# alternation that has climbed out of the group, with one branch
+# anchored on one side only and the whitelist consequently matching
+# far more than it says. paivana compiles the value bare first for
+# exactly this reason, so the configuration is refused rather than
+# accepted into a regex that means something else.
######################################################################
function test_whitelist_config() {
@@ -1375,7 +1494,9 @@ function test_whitelist_config() {
for bad in \
'WHITELIST = *invalid(' \
'WHITELIST = /free/[' \
- 'WHITELIST = /free/\'
+ 'WHITELIST = /free/\' \
+ 'WHITELIST = a)|(b' \
+ 'WHITELIST = (a$|^b'
do
msg "startup refused: $bad"
r="$(paivana_with_config_line "$bad")"
@@ -1392,7 +1513,7 @@ function test_whitelist_config() {
msg "startup accepted: $good"
r="$(paivana_with_config_line "$good")"
[ "$r" = "started" ] || \
- fail "paivana refused a usable WHITELIST ($good); log:\n$(cat "$LOGDIR/startup.log")"
+ fail "paivana refused a usable WHITELIST ($good); log:"$'\n'"$(cat "$LOGDIR/startup.log")"
ok
done
diff --git a/src/tests/upstream_go.go b/src/tests/upstream_go.go
@@ -195,15 +195,26 @@ func route(w http.ResponseWriter, r *http.Request) {
}
func main() {
- port := "8402"
+ port := 8402
if len(os.Args) > 1 {
- port = os.Args[1]
+ // ":0" would bind an ephemeral port while the driver waits
+ // for the one it asked for; ":garbage" would fail five
+ // seconds into that wait, blaming the wrong thing.
+ n, err := strconv.Atoi(os.Args[1])
+ if err != nil || n < 1 || n > 65535 {
+ fmt.Fprintf(os.Stderr, "invalid port %q\n", os.Args[1])
+ os.Exit(1)
+ }
+ port = n
}
+ // Loopback only: this server echoes an arbitrary POST body back
+ // and hands out 10 MiB on request, and has no business being
+ // reachable from the network for the duration of `make check'.
srv := &http.Server{
- Addr: ":" + port,
+ Addr: fmt.Sprintf("127.0.0.1:%d", port),
Handler: http.HandlerFunc(route),
}
- fmt.Fprintf(os.Stderr, "upstream_go listening on port %s\n", port)
+ fmt.Fprintf(os.Stderr, "upstream_go listening on port %d\n", port)
if err := srv.ListenAndServe(); err != nil {
fmt.Fprintln(os.Stderr, err)
os.Exit(1)
diff --git a/src/tests/upstream_mhd.c b/src/tests/upstream_mhd.c
@@ -25,12 +25,14 @@
* set of canned endpoints (see README).
*/
#include <errno.h>
+#include <netinet/in.h>
#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>
#include <unistd.h>
+#include <sys/socket.h>
#include <sys/time.h>
#include <microhttpd.h>
@@ -449,13 +451,37 @@ completed_cb (void *cls,
}
-static volatile sig_atomic_t run_flag = 1;
-
-static void
-on_int (int s)
+/**
+ * Parse @a arg as a TCP port number.
+ *
+ * atoi(3) would map "garbage" to 0, MHD would then bind an ephemeral
+ * port, and the driver's readiness probe would time out five seconds
+ * later blaming the port it asked for -- a failure pointing at
+ * something other than the mistake.
+ *
+ * @param arg argument to parse
+ * @param[out] port set to the parsed port on success
+ * @return 0 on success, -1 if @a arg is not a port number
+ */
+static int
+parse_port (const char *arg,
+ unsigned int *port)
{
- (void) s;
- run_flag = 0;
+ char *end;
+ long long v;
+
+ errno = 0;
+ v = strtoll (arg,
+ &end,
+ 10);
+ if ( (0 != errno) ||
+ (end == arg) ||
+ ('\0' != *end) ||
+ (v < 1) ||
+ (v > 65535) )
+ return -1;
+ *port = (unsigned int) v;
+ return 0;
}
@@ -464,20 +490,55 @@ main (int argc, char **argv)
{
unsigned int port = 8401;
struct MHD_Daemon *d;
+ struct sockaddr_in addr;
+ sigset_t quit;
+ int sig;
- if (argc > 1)
- port = (unsigned int) atoi (argv[1]);
- signal (SIGINT,
- on_int);
- signal (SIGTERM,
- on_int);
+ if ( (argc > 1) &&
+ (0 != parse_port (argv[1],
+ &port)) )
+ {
+ fprintf (stderr,
+ "invalid port `%s'\n",
+ argv[1]);
+ return 1;
+ }
signal (SIGPIPE,
SIG_IGN);
- d = MHD_start_daemon (MHD_USE_INTERNAL_POLLING_THREAD
- | MHD_USE_DUAL_STACK,
+ /* Block the two shutdown signals here, while we are still
+ single-threaded, so that MHD's internal polling thread inherits
+ the block and the signal can only be taken by the sigwait below.
+ A handler plus `while (run_flag) sleep (1);' cost up to a second
+ of shutdown latency per upstream, and a handler plus pause(2)
+ would deadlock outright whenever the process-directed signal
+ happened to be delivered to the MHD thread. */
+ sigemptyset (&quit);
+ sigaddset (&quit,
+ SIGINT);
+ sigaddset (&quit,
+ SIGTERM);
+ if (0 != sigprocmask (SIG_BLOCK,
+ &quit,
+ NULL))
+ {
+ perror ("sigprocmask");
+ return 1;
+ }
+ /* Bind the loopback interface explicitly. This is a test server
+ that echoes an arbitrary POST body back and hands out 10 MiB on
+ request; it has no business being reachable from the network for
+ the duration of `make check'. */
+ memset (&addr,
+ 0,
+ sizeof (addr));
+ addr.sin_family = AF_INET;
+ addr.sin_addr.s_addr = htonl (INADDR_LOOPBACK);
+ addr.sin_port = htons ((uint16_t) port);
+ d = MHD_start_daemon (MHD_USE_INTERNAL_POLLING_THREAD,
port,
NULL, NULL,
&handler, NULL,
+ MHD_OPTION_SOCK_ADDR, &addr,
MHD_OPTION_NOTIFY_COMPLETED, &completed_cb, NULL,
MHD_OPTION_CONNECTION_TIMEOUT, (unsigned int) 30,
MHD_OPTION_END);
@@ -492,8 +553,9 @@ main (int argc, char **argv)
"upstream_mhd listening on port %u\n",
port);
fflush (stderr);
- while (run_flag)
- sleep (1);
+ while (0 != sigwait (&quit,
+ &sig))
+ /* EINTR is the only failure sigwait can report here; retry. */;
MHD_stop_daemon (d);
return 0;
}
diff --git a/src/tests/upstream_py.py b/src/tests/upstream_py.py
@@ -152,11 +152,28 @@ class Handler(BaseHTTPRequestHandler):
self._send_text(404, "not found\n")
+def parse_port(arg):
+ # int("0") would bind an ephemeral port and report itself
+ # listening on 0, and int("garbage") would end in a traceback
+ # rather than in a usable message.
+ try:
+ port = int(arg, 10)
+ except ValueError:
+ port = -1
+ if port < 1 or port > 65535:
+ sys.stderr.write(f"invalid port {arg!r}\n")
+ sys.exit(1)
+ return port
+
+
def main():
port = 8403
if len(sys.argv) > 1:
- port = int(sys.argv[1])
- server = ThreadingHTTPServer(("", port), Handler)
+ port = parse_port(sys.argv[1])
+ # Loopback, not "": this server echoes an arbitrary POST body and
+ # hands out 10 MiB on request, and has no business being on the
+ # network for the duration of `make check'.
+ server = ThreadingHTTPServer(("127.0.0.1", port), Handler)
sys.stderr.write(f"upstream_py listening on port {port}\n")
sys.stderr.flush()
try:
diff --git a/src/tests/upstream_rs.rs b/src/tests/upstream_rs.rs
@@ -220,11 +220,23 @@ fn client_loop(mut stream: TcpStream) {
}
fn main() {
- let port: u16 = env::args()
- .nth(1)
- .and_then(|s| s.parse().ok())
- .unwrap_or(8404);
- let listener = TcpListener::bind(("0.0.0.0", port)).expect("bind failed");
+ // An argument that is not a port must be an error, not a silent
+ // fallback to the default: the driver would then wait five
+ // seconds for a port nothing ever bound and blame that port.
+ let port: u16 = match env::args().nth(1) {
+ None => 8404,
+ Some(s) => match s.parse::<u16>() {
+ Ok(p) if p >= 1 => p,
+ _ => {
+ eprintln!("invalid port {:?}", s);
+ std::process::exit(1);
+ }
+ },
+ };
+ // Loopback only: this server echoes an arbitrary POST body back
+ // and hands out 10 MiB on request, and has no business being
+ // reachable from the network for the duration of `make check'.
+ let listener = TcpListener::bind(("127.0.0.1", port)).expect("bind failed");
eprintln!("upstream_rs listening on port {}", port);
for stream in listener.incoming() {
match stream {
