feat(kyc): persist webhook authentication failures for audit

[bilhokista]

Closes #1104.

Requirement 1 was already met — by something better

The issue asks for subtle::ConstantTimeEq for the signature comparison. verify_webhook_signature already compares in constant time, via Mac::verify_slice:

// `verify_slice` is constant time, so this leaks nothing about the digest.
mac.verify_slice(&expected)

That is the hmac crate's own constant-time check — internally it is the same subtle primitive, reached through the API designed for exactly this. Swapping it for a hand-rolled ConstantTimeEq on the hex string would be a step backwards: it adds a dependency, and comparing hex text rather than raw digest bytes is easier to get subtly wrong.

So I did not change it. If you specifically want the subtle dependency for audit reasons, say so and I will add it, but I did not want to weaken working code to match the letter of the description.

Requirement 2 was genuinely missing

"Log all failed webhook authentication attempts into kyc_webhook_logs" was not happening. The handler returns early on a signature failure:

if let Err(err) = verify_webhook_signature(...) {
    warn!(reason = ?err, "KYC webhook rejected");
    return (err.status(), ...);   // <- returns here
}

…and the INSERT INTO kyc_webhook_logs sits roughly eighty lines further down, after the payload parse and the database update. Rejected requests therefore left no trace in the database at all — only a warn! line on stdout. Those are precisely the attempts an audit most wants: someone probing the endpoint or guessing at the secret.

Rejections are now persisted before the early return.

Schema

A rejected request never reaches the parser, so wallet_address, event_type, kyc_status and raw_payload cannot be populated for one. The migration relaxes those four to nullable rather than filling them with sentinels, so "we never learned this" stays distinguishable from a real value — and kyc_status is an enum, so a sentinel would have meant polluting the enum itself.

It also adds auth_failure_reason TEXT (NULL for a successful request) with a partial index on (processed_at DESC) WHERE auth_failure_reason IS NOT NULL, which is the query the column exists for.

The .down.sql restores NOT NULL, which requires first deleting the rows that could not populate those columns — exactly the failure rows this migration introduced. That is called out in a comment, since it makes the down-migration lossy and a reviewer should agree to it deliberately.

Three security decisions worth review

• The audit label is precise; the client message stays coarse. client_message deliberately cannot distinguish a malformed signature from a wrong one, and that must not change — so audit_reason() is a separate method that is only ever stored, never returned. There is a test asserting the client message still hides the variant.
• The rejected body is capped at 8 KiB and must parse as JSON. It is unauthenticated, attacker-controlled input; storing it unbounded turns this audit table into a way to fill the disk. Over the cap, or not valid JSON, it is recorded as NULL — the reason and timestamp are what an audit needs, the payload is a convenience.
• The audit write never changes the response. A failure to log is logged and swallowed. The caller has already been rejected, and returning a different status because our own logging failed would leak internal state.

I did not record the client IP. Doing so means taking ConnectInfo and thinking about proxy headers (X-Forwarded-For is caller-controlled unless the proxy is trusted), which is a separate decision rather than something to slip into this change. Happy to follow up if you want it.

Verification

Honest note: I could not run cargo test for the crate — it needs the full dependency graph and sqlx's database or offline metadata, neither of which I have here.

What I did do: lifted SignatureError, its impls and the three new tests into a standalone file and compiled them with rustc --test --edition 2021. It compiles clean and all three tests pass — that the four audit reasons are distinct, that they are stable snake_case identifiers (they land in a column that dashboards will filter on, so they are an interface, not prose), and that adding a precise audit label did not make the client response precise too.

The SQL and the sqlx query in log_auth_failure need CI and a real database.

🤖 Generated with Claude Code

https://claude.ai/code/session_01CrfEY1tvXrbeMDAUzxfuk7