Add GeoIP region filtering and address blocklist#1337
Add GeoIP region filtering and address blocklist#1337spacebear21 merged 8 commits intopayjoin:masterfrom
Conversation
Pull Request Test Coverage Report for Build 22214985999Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
1c3d998 to
b2cc26a
Compare
b2cc26a to
f0befa7
Compare
Add access-control feature to payjoin-mailroom providing IP-based region filtering via axum middleware. Requests from blocked ISO country codes are rejected at the network layer. A free DB-IP Lite database is fetched automatically when no explicit path is configured.
Screen V1 payjoin payloads against a configurable address blocklist. Addresses are parsed into script pubkeys at load time for canonical comparison, avoiding encoding round-trips and bech32 case issues. Supports loading from a local file, a remote URL with periodic background refresh, and local cache fallback.
Since address screening is only relevant when V1 is enabled, it doesn't make much sense to expose the blocked_* config otherwise. This replaces the `enable_v1` bool with a new `v1` config section. The presence / absence of that config section indicates whether to enable v1 or not, and blocked address settings can be configured within that section if the access-control feature is enabled.
3157858 to
9f366c2
Compare
Since the receiver's proposal contains new outputs and inputs contributed by the receiver, that PSBT should also be screened.
| } | ||
| ScreenResult::Clean => {} | ||
| ScreenResult::ParseError(e) => { | ||
| warn!("Could not parse V1 PSBT: {e}"); |
There was a problem hiding this comment.
I'm not sure whether this should just warn and pass a bad PSBT through for the counterparty to handle, or should it respond with an error on their behalf? I guess passing through has the benefit that the other party can immediately know the PSBT is bad and abort the payjoin without waiting for expiration.
There was a problem hiding this comment.
I think the UX of immediate abort is the best.
| return Err(HandlerError::Forbidden(anyhow::anyhow!( | ||
| "blocked address in V1 PSBT" | ||
| ))); |
There was a problem hiding this comment.
Is there a risk of leaking information to a malicious actor with a Forbidden response here? Maybe a 400 response with original-psbt-rejected well-known error code is appropriate?
There was a problem hiding this comment.
A bit, I used your error code suggestion here in the most recent commit.
Allow operators to block specific IP addresses or CIDR ranges
independently of geographic region.
Bare IPs ("192.0.2.1") and CIDR notation ("192.0.2.0/24") are
both accepted in the config.
| let url = | ||
| format!("https://download.db-ip.com/free/dbip-country-lite-{}-{}.mmdb.gz", now.0, now.1); |
There was a problem hiding this comment.
I did a bit of research on this and found two things:
- https://github.com/wp-statistics/GeoLite2-Country?tab=readme-ov-file provides a community-maintained CDN download link that is updated automatically without needing to specify the year/month.
- Both db-ip and the wp-statistics alternative are licensed under CC BY-SA 4.0 which requires attribution. I'm not sure if just including a download link in the code applies, but to be thorough I think just a mention in the README would do?
There was a problem hiding this comment.
Applied these changes in the last two commits.
Use the automatically-updated CDN download link from https://github.com/wp-statistics/GeoLite2-Country?tab=readme-ov-file to avoid manual date shenanigans.
Use the well-known original-psbt-rejected error code instead of 403 Forbidden so V1 senders get a standard BIP78 response that does not reveal screening details.
|
I thoroughly considered failing fast for v2 receiver for whom a bad, filtered v1. I think we shouldn't do it because it's more complicated for very little upside of a very small slice of v1 usage and our time is better spent elsewhere. Weee'd need a new |
|
|
||
| let app = build_app(services); | ||
| #[cfg(feature = "access-control")] | ||
| let app = app.into_make_service_with_connect_info::<middleware::MaybePeerIp>(); |
There was a problem hiding this comment.
First time seeing this method. I guess this middleware pattern is because this check is at the TCP/stream level and not HTTP.
Summary
access-controlfeature to payjoin-mailroom with GeoIP-based region filtering via maxminddb