commit c2937d739296ef48df24f8901da4aa58d867ca8c
parent 60e2a7848465684a35041df7f56cd57d54941d16
Author: Christian Grothoff <christian@grothoff.org>
Date: Thu, 6 Aug 2026 00:15:28 +0200
clarify MFA challenge idempotency is deliberate
Diffstat:
3 files changed, 161 insertions(+), 2 deletions(-)
diff --git a/src/backenddb/do_solve_mfa_challenge.sql b/src/backenddb/do_solve_mfa_challenge.sql
@@ -53,7 +53,22 @@ BEGIN
out_retry_counter = my_rec.retry_counter;
out_solved = my_rec.solved;
- -- Check if already solved before
+ -- Check if already solved before.
+ -- NOTE: this is deliberately idempotent: once the challenge is confirmed,
+ -- ANY solution (including a wrong one) yields out_solved=TRUE and the
+ -- retry counter is left alone. This is intentional and safe:
+ -- * authorization is not granted here. It is granted by
+ -- get_mfa_challenge() reporting a non-NULL confirmation_date, which this
+ -- branch does not change; re-confirming an already confirmed challenge
+ -- therefore grants nothing that the caller did not already have.
+ -- * reaching this branch already requires knowing in_challenge_id AND the
+ -- matching in_h_body (a hash over the request body with a 32-byte random
+ -- server-side salt), which together are the capability that authorizes
+ -- the operation in the first place.
+ -- * not decrementing the retry counter here keeps a client that retries a
+ -- confirmation after a lost response from burning its attempts, and
+ -- avoids turning this endpoint into a TAN-guessing oracle on a challenge
+ -- whose code has already been used.
IF my_confirmation_date IS NOT NULL
THEN
out_solved = TRUE;
@@ -82,3 +97,11 @@ BEGIN
END IF;
END;
$$;
+
+COMMENT ON FUNCTION merchant_do_solve_mfa_challenge(INT8, BYTEA, TEXT, INT8)
+ IS 'Checks in_solution against the code of the challenge identified by'
+ ' in_challenge_id and in_h_body, and confirms the challenge if it'
+ ' matches. Returns out_solved=FALSE with a NULL out_retry_counter if no'
+ ' such (unexpired) challenge exists. Confirming an already confirmed'
+ ' challenge is idempotent: out_solved=TRUE is returned regardless of'
+ ' in_solution and the retry counter is not decremented.';
diff --git a/src/backenddb/test_merchantdb.c b/src/backenddb/test_merchantdb.c
@@ -53,6 +53,8 @@
#include "merchant-database/insert_deposit_to_transfer.h"
#include "merchant-database/insert_exchange_signing_key.h"
#include "merchant-database/insert_instance.h"
+#include "merchant-database/insert_mfa_challenge.h"
+#include "merchant-database/do_solve_mfa_challenge.h"
#include "merchant-database/insert_order.h"
#include "merchant-database/insert_order_lock.h"
#include "merchant-database/insert_otp_device.h"
@@ -10510,6 +10512,128 @@ test_statistics (void)
}
+/* ********** MFA challenges ********** */
+
+
+/**
+ * Tests solving multi-factor authentication (MFA) challenges.
+ *
+ * Besides the obvious cases, this pins down the *idempotency* of
+ * confirming a challenge that has already been confirmed: any TAN is
+ * then accepted and the retry counter is left alone. That is
+ * intentional (see the comment in do_solve_mfa_challenge.sql): the
+ * authorization is carried by the confirmation already stored in the
+ * database, which this path does not change, and reaching it already
+ * requires knowing the challenge ID together with the matching body
+ * hash. The assertions are here so that the contract cannot silently
+ * drift.
+ *
+ * @return 0 on success, 1 otherwise.
+ */
+static int
+test_mfa_challenges (void)
+{
+ struct TALER_MERCHANT_MFA_BodyHash h_body;
+ struct TALER_MERCHANT_MFA_BodySalt salt;
+ struct TALER_MERCHANT_MFA_BodyHash other_h_body;
+ uint64_t challenge_id;
+ bool solved;
+ uint32_t retry_counter;
+
+ GNUNET_CRYPTO_random_block (&h_body,
+ sizeof (h_body));
+ GNUNET_CRYPTO_random_block (&other_h_body,
+ sizeof (other_h_body));
+ GNUNET_CRYPTO_random_block (&salt,
+ sizeof (salt));
+ TEST_COND_RET_ON_FAIL (
+ GNUNET_DB_STATUS_SUCCESS_ONE_RESULT ==
+ TALER_MERCHANTDB_insert_mfa_challenge (
+ pg,
+ "test_inst_mfa",
+ TALER_MERCHANT_MFA_CO_AUTH_CONFIGURATION,
+ &h_body,
+ &salt,
+ "1234-5678",
+ GNUNET_TIME_relative_to_absolute (GNUNET_TIME_UNIT_DAYS),
+ GNUNET_TIME_UNIT_ZERO_ABS,
+ TALER_MERCHANT_MFA_CHANNEL_EMAIL,
+ "test@example.com",
+ &challenge_id),
+ "Insert MFA challenge failed\n");
+ /* A challenge is only addressable with the matching body hash. */
+ TEST_COND_RET_ON_FAIL (
+ GNUNET_DB_STATUS_SUCCESS_NO_RESULTS ==
+ TALER_MERCHANTDB_do_solve_mfa_challenge (pg,
+ challenge_id,
+ &other_h_body,
+ "1234-5678",
+ &solved,
+ &retry_counter),
+ "Challenge was solvable with a different body hash\n");
+ TEST_COND_RET_ON_FAIL (! solved,
+ "Challenge solved with a different body hash\n");
+ /* A wrong TAN is rejected and costs one of the three attempts. */
+ TEST_COND_RET_ON_FAIL (
+ GNUNET_DB_STATUS_SUCCESS_ONE_RESULT ==
+ TALER_MERCHANTDB_do_solve_mfa_challenge (pg,
+ challenge_id,
+ &h_body,
+ "0000-0000",
+ &solved,
+ &retry_counter),
+ "Solving MFA challenge failed\n");
+ TEST_COND_RET_ON_FAIL (! solved,
+ "A wrong TAN solved the challenge\n");
+ TEST_COND_RET_ON_FAIL (2 == retry_counter,
+ "A wrong TAN did not cost an attempt\n");
+ /* The correct TAN confirms the challenge. */
+ TEST_COND_RET_ON_FAIL (
+ GNUNET_DB_STATUS_SUCCESS_ONE_RESULT ==
+ TALER_MERCHANTDB_do_solve_mfa_challenge (pg,
+ challenge_id,
+ &h_body,
+ "1234-5678",
+ &solved,
+ &retry_counter),
+ "Solving MFA challenge failed\n");
+ TEST_COND_RET_ON_FAIL (solved,
+ "The correct TAN did not solve the challenge\n");
+ TEST_COND_RET_ON_FAIL (2 == retry_counter,
+ "The correct TAN cost an attempt\n");
+ /* Re-confirming an already confirmed challenge is idempotent. */
+ TEST_COND_RET_ON_FAIL (
+ GNUNET_DB_STATUS_SUCCESS_ONE_RESULT ==
+ TALER_MERCHANTDB_do_solve_mfa_challenge (pg,
+ challenge_id,
+ &h_body,
+ "0000-0000",
+ &solved,
+ &retry_counter),
+ "Solving MFA challenge failed\n");
+ TEST_COND_RET_ON_FAIL (solved,
+ "Re-confirming a confirmed challenge is not"
+ " idempotent\n");
+ TEST_COND_RET_ON_FAIL (2 == retry_counter,
+ "Re-confirming a confirmed challenge cost an"
+ " attempt\n");
+ /* ... but still only for the challenge's own body hash. */
+ TEST_COND_RET_ON_FAIL (
+ GNUNET_DB_STATUS_SUCCESS_NO_RESULTS ==
+ TALER_MERCHANTDB_do_solve_mfa_challenge (pg,
+ challenge_id,
+ &other_h_body,
+ "1234-5678",
+ &solved,
+ &retry_counter),
+ "Confirmed challenge was addressable with a different body hash\n");
+ TEST_COND_RET_ON_FAIL (! solved,
+ "Confirmed challenge solved with a different body"
+ " hash\n");
+ return 0;
+}
+
+
/**
* Function that runs all tests.
*
@@ -10533,6 +10657,7 @@ run_tests (void)
TEST_RET_ON_FAIL (test_pending_webhooks ());
TEST_RET_ON_FAIL (test_inventory_deleted_webhook ());
TEST_RET_ON_FAIL (test_statistics ());
+ TEST_RET_ON_FAIL (test_mfa_challenges ());
return 0;
}
diff --git a/src/include/merchant-database/do_solve_mfa_challenge.h b/src/include/merchant-database/do_solve_mfa_challenge.h
@@ -33,12 +33,23 @@ struct TALER_MERCHANTDB_PostgresContext;
* the solution state and (on failure) retry counter depending on
* the result.
*
+ * Solving a challenge is idempotent: if the challenge was already
+ * confirmed, @a solved is set to true and @a retry_counter is left
+ * unchanged, whatever @a solution says. This does not weaken the
+ * authorization, which is granted by the confirmation already stored
+ * in the database (see TALER_MERCHANTDB_get_mfa_challenge()) and not
+ * by this function; and reaching that state already requires knowing
+ * the @a challenge_id together with the matching @a h_body, which is
+ * exactly the capability that authorizes the operation. Not touching
+ * the retry counter here keeps a client that retries after a lost
+ * response from burning its remaining attempts.
+ *
* @param pg database context
* @param challenge_id challenge ID to be solved
* @param h_body body of the operation the challenge authorizes
* @param solution proposed solution to be checked against the actual code
* @param[out] solved set to true if the challenge was solved by
- * @a solution
+ * @a solution, or was already confirmed before
* @param[out] retry_counter set to the number of attempts that remain
* for solving the challenge (after this time)
* @return database result code
