challenger

OAuth 2.0-based authentication service that validates user can receive messages at a certain address
Log | Files | Refs | Submodules | README | LICENSE

commit 06b57c086347ad986c37c0719dbacdcd31dfee9b
parent 3f993a43e537c010dfb2f178b9b0921cf6a72d3e
Author: Christian Grothoff <christian@grothoff.org>
Date:   Thu,  6 Aug 2026 18:34:46 +0200

fix ECs and upload size limit handling

Diffstat:
Msrc/challenger/challenger-httpd_authorize.c | 9++++++---
Msrc/challenger/challenger-httpd_challenge.c | 52+++++++++++++++++++++++++++++++++++++++++-----------
Msrc/challenger/challenger-httpd_common.c | 12+++++++++++-
Msrc/challenger/challenger-httpd_common.h | 9++++++++-
Msrc/challenger/challenger-httpd_config.c | 8+++++++-
Msrc/challenger/challenger-httpd_info.c | 2+-
Msrc/challenger/challenger-httpd_setup.c | 21++++++++++++++-------
Msrc/challenger/challenger-httpd_solve.c | 18+++++++++++++-----
Msrc/challenger/challenger-httpd_token.c | 10++++++++--
Msrc/challenger/meson.build | 1+
Msrc/challenger/test-challenger-exhaustion.sh | 19++++++++++---------
Asrc/challenger/test-challenger-upload-limits.sh | 192+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
12 files changed, 312 insertions(+), 41 deletions(-)

diff --git a/src/challenger/challenger-httpd_authorize.c b/src/challenger/challenger-httpd_authorize.c @@ -79,11 +79,14 @@ CH_handler_authorize (struct CH_HandlerContext *hc, sizeof (nonce))) { GNUNET_break_op (0); + /* The nonce is present but unusable; that is a malformed request, + not a missing parameter and not an unknown validation. /challenge + and /solve answer the same way. */ return reply_error ( hc, - MHD_HTTP_NOT_FOUND, - TALER_EC_GENERIC_PARAMETER_MISSING, - hc->path); + MHD_HTTP_BAD_REQUEST, + TALER_EC_GENERIC_PARAMETER_MALFORMED, + "nonce"); } response_type = MHD_lookup_connection_value (hc->connection, diff --git a/src/challenger/challenger-httpd_challenge.c b/src/challenger/challenger-httpd_challenge.c @@ -598,6 +598,8 @@ post_iter (void *cls, (void) off; if (MHD_POSTDATA_KIND != kind) return MHD_YES; + if (bc->too_big) + return MHD_YES; /* discard the rest, we already have our answer */ if ( (NULL != bc->last_key) && (0 != strcmp (key, bc->last_key)) ) @@ -615,7 +617,11 @@ post_iter (void *cls, { GNUNET_break_op (0); bc->too_big = true; - return MHD_NO; + /* Returning MHD_NO here would put the post processor into an error + state and make MHD tear the connection down without a response; + keep going (discarding the data) so that the handler can reply + with a proper 413. */ + return MHD_YES; } bc->data = GNUNET_realloc (bc->data, bc->data_len + size + 1); @@ -632,14 +638,21 @@ post_iter (void *cls, * Check if the given address satisfies our restrictions. * * @param address address data provided by the client + * @param[out] ec set to the error code to report if the check failed; + * #TALER_EC_CHALLENGER_ADDRESS_RESTRICTION_VIOLATED if the value + * supplied by the user is at fault, and + * #TALER_EC_CHALLENGER_ADDRESS_RESTRICTION_MALFORMED if our own + * configuration is at fault * @return NULL on success, otherwise the key that failed */ static const char * -check_restrictions (const json_t *address) +check_restrictions (const json_t *address, + enum TALER_ErrorCode *ec) { const char *key; const json_t *val; + *ec = TALER_EC_CHALLENGER_ADDRESS_RESTRICTION_VIOLATED; json_object_foreach ((json_t *) CH_restrictions, key, val) { const char *str = json_string_value ( @@ -659,6 +672,7 @@ check_restrictions (const json_t *address) GNUNET_log (GNUNET_ERROR_TYPE_WARNING, "Regex missing for `%s'\n", key); + *ec = TALER_EC_CHALLENGER_ADDRESS_RESTRICTION_MALFORMED; return key; } if (0 != regcomp (&re, @@ -671,6 +685,7 @@ check_restrictions (const json_t *address) key); /* Fail closed: a restriction that cannot be compiled must not be treated as "no restriction". */ + *ec = TALER_EC_CHALLENGER_ADDRESS_RESTRICTION_MALFORMED; return key; } { @@ -756,11 +771,14 @@ CH_handler_challenge (struct CH_HandlerContext *hc, sizeof (bc->nonce))) { GNUNET_break_op (0); + /* The nonce is present but unusable; that is a malformed request, + not a missing parameter and not an unknown validation. /authorize + and /solve answer the same way. */ return TALER_MHD_reply_with_error ( hc->connection, - MHD_HTTP_NOT_FOUND, - TALER_EC_GENERIC_PARAMETER_MISSING, - hc->path); + MHD_HTTP_BAD_REQUEST, + TALER_EC_GENERIC_PARAMETER_MALFORMED, + "nonce"); } TALER_MHD_check_content_length (hc->connection, 1024); @@ -905,14 +923,26 @@ CH_handler_challenge (struct CH_HandlerContext *hc, #endif { const char *bad_field; + enum TALER_ErrorCode ec; - bad_field = check_restrictions (bc->address); + bad_field = check_restrictions (bc->address, + &ec); if (NULL != bad_field) { - GNUNET_break_op (0); + bool misconfigured + = (TALER_EC_CHALLENGER_ADDRESS_RESTRICTION_MALFORMED == ec); + + /* A restriction we cannot evaluate is our fault, not the user's, and + must not be reported as a bad request. */ + if (misconfigured) + GNUNET_break (0); + else + GNUNET_break_op (0); return reply_error (bc, - MHD_HTTP_BAD_REQUEST, - TALER_EC_GENERIC_PARAMETER_MALFORMED, + misconfigured + ? MHD_HTTP_INTERNAL_SERVER_ERROR + : MHD_HTTP_BAD_REQUEST, + ec, bad_field); } } @@ -1045,7 +1075,7 @@ CH_handler_challenge (struct CH_HandlerContext *hc, "Address changes exhausted address change limit for this process\n"); return reply_error (bc, MHD_HTTP_TOO_MANY_REQUESTS, - TALER_EC_CHALLENGER_TOO_MANY_ATTEMPTS, + TALER_EC_CHALLENGER_TOO_MANY_ADDRESS_CHANGES, "client exceeded authorization attempts limit (too many addresses attempted)"); } @@ -1059,7 +1089,7 @@ CH_handler_challenge (struct CH_HandlerContext *hc, "PIN transmission limit exhausted for this validation\n"); return reply_error (bc, MHD_HTTP_TOO_MANY_REQUESTS, - TALER_EC_CHALLENGER_TOO_MANY_ATTEMPTS, + TALER_EC_CHALLENGER_TOO_MANY_PIN_TRANSMISSIONS, "client exceeded authorization attempts limit (too many PINs)"); } diff --git a/src/challenger/challenger-httpd_common.c b/src/challenger/challenger-httpd_common.c @@ -57,23 +57,31 @@ CH_get_output_type (struct MHD_Connection *connection) const char * -CH_get_client_secret (struct MHD_Connection *connection) +CH_get_client_secret (struct MHD_Connection *connection, + enum TALER_ErrorCode *ec) { const char *bearer = "Bearer "; const char *auth; const char *tok; + enum TALER_ErrorCode ignored; + if (NULL == ec) + ec = &ignored; auth = MHD_lookup_connection_value (connection, MHD_HEADER_KIND, MHD_HTTP_HEADER_AUTHORIZATION); if (NULL == auth) + { + *ec = TALER_EC_GENERIC_PARAMETER_MISSING; return NULL; + } /* RFC 7235: the auth-scheme token ("Bearer") is case-insensitive. */ if (0 != strncasecmp (auth, bearer, strlen (bearer))) { + *ec = TALER_EC_GENERIC_PARAMETER_MALFORMED; return NULL; } tok = auth + strlen (bearer); @@ -84,8 +92,10 @@ CH_get_client_secret (struct MHD_Connection *connection) RFC_8959_PREFIX, strlen (RFC_8959_PREFIX))) { + *ec = TALER_EC_GENERIC_PARAMETER_MALFORMED; return NULL; } + *ec = TALER_EC_NONE; return tok; } diff --git a/src/challenger/challenger-httpd_common.h b/src/challenger/challenger-httpd_common.h @@ -28,10 +28,17 @@ * authorization header of @a connection. * * @param connection HTTP connection to get client secret from + * @param[out] ec on failure, set to + * #TALER_EC_GENERIC_PARAMETER_MISSING if the "Authorization" header + * is absent altogether, and to + * #TALER_EC_GENERIC_PARAMETER_MALFORMED if it is present but does + * not carry a "Bearer" token with the RFC 8959 "secret-token:" + * prefix; may be NULL if the caller does not care * @return NULL if there is no well-formed secret */ const char * -CH_get_client_secret (struct MHD_Connection *connection); +CH_get_client_secret (struct MHD_Connection *connection, + enum TALER_ErrorCode *ec); /** diff --git a/src/challenger/challenger-httpd_config.c b/src/challenger/challenger-httpd_config.c @@ -34,7 +34,13 @@ * 5: added support for GET /authorize to have a link in challenge messages to the form * 6: added ``address_type`` field in ``/config`` * 7: added ``build_version`` field in ``/config`` - * 8: minor HTTP status code corrections + * 8: HTTP status code and error code corrections: /token no longer + * returns 409 and /info no longer returns 404; /solve tells a wrong + * PIN from one that could not be checked at all; /challenge and /solve + * use distinct error codes for each way of running out of attempts; + * address restrictions have their own error codes and a restriction we + * cannot evaluate is a 500 rather than a 400; a malformed nonce is a + * 400 everywhere; /setup answers missing credentials with 403. */ diff --git a/src/challenger/challenger-httpd_info.c b/src/challenger/challenger-httpd_info.c @@ -56,7 +56,7 @@ reply_invalid_token (struct MHD_Connection *connection) connection, MHD_HTTP_UNAUTHORIZED, "invalid_token", - TALER_EC_CHALLENGER_GRANT_UNKNOWN, + TALER_EC_CHALLENGER_TOKEN_UNKNOWN, NULL); } diff --git a/src/challenger/challenger-httpd_setup.c b/src/challenger/challenger-httpd_setup.c @@ -150,14 +150,21 @@ CH_handler_setup (struct CH_HandlerContext *hc, "client_id"); } } - client_secret = CH_get_client_secret (hc->connection); - if (NULL == client_secret) { - GNUNET_break_op (0); - return TALER_MHD_reply_with_error (hc->connection, - MHD_HTTP_BAD_REQUEST, - TALER_EC_GENERIC_PARAMETER_MISSING, - MHD_HTTP_HEADER_AUTHORIZATION); + enum TALER_ErrorCode ec; + + client_secret = CH_get_client_secret (hc->connection, + &ec); + if (NULL == client_secret) + { + GNUNET_break_op (0); + /* Credentials are required to use this endpoint at all, so refusing + the request is a matter of authorization, not of a bad request. */ + return TALER_MHD_reply_with_error (hc->connection, + MHD_HTTP_FORBIDDEN, + ec, + MHD_HTTP_HEADER_AUTHORIZATION); + } } { const json_t *ro; diff --git a/src/challenger/challenger-httpd_solve.c b/src/challenger/challenger-httpd_solve.c @@ -156,6 +156,8 @@ post_iter (void *cls, (void) content_type; (void) transfer_encoding; (void) off; + if (bc->too_big) + return MHD_YES; /* discard the rest, we already have our answer */ if (0 != strcmp (key, "pin")) return MHD_YES; @@ -168,7 +170,11 @@ post_iter (void *cls, { bc->too_big = true; GNUNET_break_op (0); - return MHD_NO; + /* Returning MHD_NO here would put the post processor into an error + state and make MHD tear the connection down without a response; + keep going (discarding the data) so that the handler can reply + with a proper 413. */ + return MHD_YES; } bc->pin = GNUNET_realloc (bc->pin, bc->pin_len + size + 1); @@ -345,8 +351,10 @@ CH_handler_solve (struct CH_HandlerContext *hc, /* Distinguish the three ways in which a /solve can fail. Only the last one is actually about the PIN that was submitted; reporting the other two as "the PIN code provided is incorrect" misleads - the user, and /challenge already answers the analogous conditions - with 429 #TALER_EC_CHALLENGER_TOO_MANY_ATTEMPTS. */ + the user. Each gets its own error code so that the user agent can + tell "provide your address first" from "wait or ask for a new PIN" + from "that PIN was wrong" without parsing the human-readable + hint. */ if (no_challenge) { /* No PIN was ever transmitted for this validation, so there is @@ -354,7 +362,7 @@ CH_handler_solve (struct CH_HandlerContext *hc, GNUNET_log (GNUNET_ERROR_TYPE_INFO, "PIN submitted before any challenge was transmitted\n"); http_status = MHD_HTTP_CONFLICT; - ec = TALER_EC_CHALLENGER_MISSING_ADDRESS; + ec = TALER_EC_CHALLENGER_NO_CHALLENGE_TRANSMITTED; } else if (exhausted) { @@ -364,7 +372,7 @@ CH_handler_solve (struct CH_HandlerContext *hc, GNUNET_log (GNUNET_ERROR_TYPE_INFO, "No attempts left to check the PIN\n"); http_status = MHD_HTTP_TOO_MANY_REQUESTS; - ec = TALER_EC_CHALLENGER_TOO_MANY_ATTEMPTS; + ec = TALER_EC_CHALLENGER_NO_PIN_ATTEMPTS_LEFT; } else { diff --git a/src/challenger/challenger-httpd_token.c b/src/challenger/challenger-httpd_token.c @@ -184,6 +184,8 @@ post_iter (void *cls, (void) content_type; (void) transfer_encoding; (void) off; + if (bc->too_big) + return MHD_YES; /* discard the rest, we already have our answer */ for (unsigned int i = 0; NULL != map[i].name; i++) if (0 == strcmp (key, map[i].name)) @@ -201,7 +203,11 @@ post_iter (void *cls, { GNUNET_break_op (0); bc->too_big = true; - return MHD_NO; + /* Returning MHD_NO here would put the post processor into an error + state and make MHD tear the connection down without a response; + keep going (discarding the data) so that the handler can reply + with a proper 413. */ + return MHD_YES; } if (NULL == *ptr) *ptr = GNUNET_malloc (size + 1); @@ -378,7 +384,7 @@ CH_handler_token (struct CH_HandlerContext *hc, hc->connection, MHD_HTTP_UNAUTHORIZED, "invalid_client", - TALER_EC_CHALLENGER_GENERIC_CLIENT_UNKNOWN, + TALER_EC_CHALLENGER_CLIENT_AUTHENTICATION_FAILED, NULL); case GNUNET_DB_STATUS_SUCCESS_ONE_RESULT: break; diff --git a/src/challenger/meson.build b/src/challenger/meson.build @@ -22,6 +22,7 @@ check_SCRIPTS = [ 'test-challenger-resend', 'test-challenger-exhaustion', 'test-challenger-auth-errors', + 'test-challenger-upload-limits', ] test_helper_cat = configure_file(input: 'cat.sh', output: 'cat.sh', copy: true) diff --git a/src/challenger/test-challenger-exhaustion.sh b/src/challenger/test-challenger-exhaustion.sh @@ -22,11 +22,12 @@ # # Also a regression test for finding 12: '/solve' must not answer every # failure with 403 #TALER_EC_CHALLENGER_INVALID_PIN ("the PIN code -# provided is incorrect"). Exhaustion is reported the way /challenge -# reports it (429 #TALER_EC_CHALLENGER_TOO_MANY_ATTEMPTS), submitting a -# PIN before any challenge was transmitted is reported as a conflict, and -# 'exhausted' is set on the response that consumes the last attempt -# rather than on the one after it. +# provided is incorrect"). Running out of guesses is reported as 429 +# #TALER_EC_CHALLENGER_NO_PIN_ATTEMPTS_LEFT, submitting a PIN before any +# challenge was transmitted is reported as 409 +# #TALER_EC_CHALLENGER_NO_CHALLENGE_TRANSMITTED, and 'exhausted' is set on +# the response that consumes the last attempt rather than on the one after +# it. set -eu @@ -236,9 +237,9 @@ if [ "$CODE" = "9758" ] then exit_fail "/solve claims 'the PIN code provided is incorrect' although no PIN was ever sent" fi -if [ "$STATUS" != "409" ] || [ "$CODE" != "9759" ] +if [ "$STATUS" != "409" ] || [ "$CODE" != "9761" ] then - exit_fail "/solve: expected 409 with code 9759. Got: $STATUS / $CODE" $(cat $LAST_RESPONSE) + exit_fail "/solve: expected 409 with code 9761 (NO_CHALLENGE_TRANSMITTED). Got: $STATUS / $CODE" $(cat $LAST_RESPONSE) fi echo " OK" @@ -279,9 +280,9 @@ echo " OK" echo -n "Exhaustion must be reported like /challenge reports it ..." CODE=$(jq -r .code < "$LAST_RESPONSE") -if [ "$STATUS" != "429" ] || [ "$CODE" != "9757" ] +if [ "$STATUS" != "429" ] || [ "$CODE" != "9766" ] then - exit_fail "/solve: expected 429 with code 9757 (TOO_MANY_ATTEMPTS) once the guesses are spent. Got: $STATUS / $CODE" $(cat $LAST_RESPONSE) + exit_fail "/solve: expected 429 with code 9766 (NO_PIN_ATTEMPTS_LEFT) once the guesses are spent. Got: $STATUS / $CODE" $(cat $LAST_RESPONSE) fi if [ "$(jq -r .exhausted < "$LAST_RESPONSE")" != "true" ] then diff --git a/src/challenger/test-challenger-upload-limits.sh b/src/challenger/test-challenger-upload-limits.sh @@ -0,0 +1,192 @@ +#!/usr/bin/env bash +# This file is in the public domain. +# +# Tests that an oversized upload is answered with 413 on every endpoint +# that reads a request body, both when the size is announced via +# Content-Length and when it is not. +# +# Regression test: the per-field hard cap in the post processor iterators +# used to set 'too_big' and then return MHD_NO. That puts MHD's post +# processor into an error state, so MHD_post_process() failed, the handler +# returned MHD_NO and MHD tore the connection down *without any response*. +# The 'too_big' branch that was supposed to produce the 413 was therefore +# unreachable, and a client streaming an oversized field (chunked, i.e. +# without a Content-Length that TALER_MHD_check_content_length() could +# have rejected up front) just saw the connection drop. + +set -eu + +# Exit, with status code "skip" (no 'real' failure) +function exit_skip() { + echo " SKIP: $1" + exit 77 +} + +# Exit, with error message (hard failure) +function exit_fail() { + echo " FAIL: $@" + exit 1 +} + +# Cleanup to run whenever we exit +function cleanup() +{ + for n in $(jobs -p) + do + kill $n 2> /dev/null || true + done + rm -f "$LAST_RESPONSE" "$BIG" + wait +} + +LAST_RESPONSE=$(mktemp responseXXXXXX.log) +BIG=$(mktemp bigXXXXXX.txt) + +# Install cleanup handler (except for kill -9) +trap cleanup EXIT + +export PATH="$PATH:." + +echo -n "Testing for jq" +jq -h > /dev/null || exit_skip "jq required" +echo " FOUND" +echo -n "Testing for curl" +curl -h > /dev/null || exit_skip "curl required" +echo " FOUND" +echo -n "Testing for wget" +wget -h > /dev/null || exit_skip "wget required" +echo " FOUND" +echo -n "Testing for challenger-httpd ..." +challenger-httpd -h > /dev/null || exit_skip "challenger-httpd required" +echo " FOUND" + +CONF="test-challenger.conf" +BURL="http://localhost:9967" +REDIRECT_URI="http://client.example.com/" + +echo -n "Initialize challenger database ..." +challenger-dbinit -r -c "${CONF}" &> dbinit.log +echo " OK" + +echo -n "Add challenger client ..." +CLIENT_SECRET="secret-token:secret" +challenger-admin -c "${CONF}" -a "${CLIENT_SECRET}" "${REDIRECT_URI}" &> admin.log +echo " OK" +# We just reset the DB, thus the client ID must be 1 here: +CLIENT_ID=1 + +echo -n "Start challenger-httpd ..." +challenger-httpd -L INFO -c "${CONF}" &> httpd.log & + +# Wait for challenger to be available +for n in $(seq 1 50) +do + echo -n "." + sleep 0.2 + OK=0 + wget --tries=1 --timeout=1 "${BURL}/config" -o /dev/null -O /dev/null >/dev/null || continue + OK=1 + break +done +if [ 1 != $OK ] +then + exit_skip "Failed to launch challenger service" +fi +echo " OK" + +# 5 KiB exceeds every per-field cap in the service (1 KiB for /challenge +# and /solve, 2 KiB for /token). +printf 'x%.0s' $(seq 1 5120) > "$BIG" + +# $1: human-readable description, $2: URL, $3: form field to overflow, +# $4... : extra curl arguments. +function expect_413() +{ + local WHAT="$1" + local URL="$2" + local FIELD="$3" + shift 3 + local STATUS + local CODE + + STATUS=$(curl "$URL" \ + -X POST \ + -H "Content-Type: application/x-www-form-urlencoded" \ + --data-urlencode "${FIELD}@${BIG}" \ + "$@" \ + -w "%{http_code}" -s -o $LAST_RESPONSE) \ + || exit_fail "${WHAT}: connection failed instead of returning 413" + if [ "$STATUS" != "413" ] + then + exit_fail "${WHAT}: expected 413. Got: $STATUS" $(cat $LAST_RESPONSE) + fi + CODE=$(jq -r .code < "$LAST_RESPONSE") + # 32 == TALER_EC_GENERIC_UPLOAD_EXCEEDS_LIMIT + if [ "$CODE" != "32" ] + then + exit_fail "${WHAT}: expected code 32 (UPLOAD_EXCEEDS_LIMIT). Got: $CODE" $(cat $LAST_RESPONSE) + fi +} + +# A fresh nonce for each request, so that a rejected upload can never be +# confused with a validation that ran out of attempts. +function fresh_nonce() +{ + local STATUS + STATUS=$(curl "${BURL}/setup/${CLIENT_ID}" \ + -H "Authorization: Bearer ${CLIENT_SECRET}" \ + -d '' \ + -w "%{http_code}" -s -o $LAST_RESPONSE) + if [ "$STATUS" != "200" ] + then + exit_fail "Expected 200 OK from /setup. Got: $STATUS" $(cat $LAST_RESPONSE) + fi + NONCE=$(jq -r .nonce < "$LAST_RESPONSE") +} + +# curl sends a Content-Length for --data-urlencode, so these exercise the +# TALER_MHD_check_content_length() path. +echo -n "/challenge rejects an oversized announced upload ..." +fresh_nonce +expect_413 "/challenge with Content-Length" "${BURL}/challenge/${NONCE}" "filename" +echo " OK" + +echo -n "/solve rejects an oversized announced upload ..." +fresh_nonce +expect_413 "/solve with Content-Length" "${BURL}/solve/${NONCE}" "pin" +echo " OK" + +echo -n "/token rejects an oversized announced upload ..." +expect_413 "/token with Content-Length" "${BURL}/token" "code" +echo " OK" + +# Forcing chunked transfer removes the Content-Length, so the only thing +# left to stop the upload is the per-field cap in the post iterator -- +# the code path this test was written for. +echo -n "/challenge rejects an oversized chunked upload ..." +fresh_nonce +expect_413 "/challenge chunked" "${BURL}/challenge/${NONCE}" "filename" \ + -H "Transfer-Encoding: chunked" +echo " OK" + +echo -n "/solve rejects an oversized chunked upload ..." +fresh_nonce +expect_413 "/solve chunked" "${BURL}/solve/${NONCE}" "pin" \ + -H "Transfer-Encoding: chunked" +echo " OK" + +echo -n "/token rejects an oversized chunked upload ..." +expect_413 "/token chunked" "${BURL}/token" "code" \ + -H "Transfer-Encoding: chunked" +echo " OK" + +# The daemon must still be alive and serving after all of that. +echo -n "Service survived the oversized uploads ..." +STATUS=$(curl "${BURL}/config" -w "%{http_code}" -s -o $LAST_RESPONSE) +if [ "$STATUS" != "200" ] +then + exit_fail "Expected 200 OK from /config. Got: $STATUS" $(cat $LAST_RESPONSE) +fi +echo " OK" + +exit 0