Hi,
Nice project, thanks for releasing this. I've been troubleshooting an issue and I think I've found some potential improvements.
I was trying to use this in a docker container, with Caddy as the frontend, and some private IPs in the AllowedIP table. So I looked through the code, mainly get_ip_address_from_request() in middleware.py.
- It ignores private IPs without an option to change. In my case, the source IP is private and one I want to allow.
...
PRIVATE_IPS_PREFIX = ('10.', '172.', '192.', '127.')
...
for ip in ips:
if ip.startswith(PRIVATE_IPS_PREFIX):
continue
...
# same in the checks for REAL_IP and REMOTE_ADDR
This should be optional or changed.
- It's susceptible to x-forwarded-for spoofing without a reverse proxy, or with one in a default configuration.
X-Forwarded-For can be set by the client. In the case of a reverse proxy like nginx or caddy, their default behaviour is to append their IP to it, not rewrite it. Good security is telling the reverse proxy to rewrite it, so this isn't the responsibility of this project, but it would help security to disable reading the IP from x-forwarded-for by default, and documenting the risks and best-practises along with how to enable it. Alternatively you could read the right-most x-forwarded-for IP which would be the last reverse proxy in the default configuration, or the rewritten client IP if somebody's secured their reverse proxy. In the case of no reverse proxy it could still be spoofed though.
I think I'm going to restrict access to /admin/ via caddy but just wanted to mention these for others in case they run into the same things.
Hi,
Nice project, thanks for releasing this. I've been troubleshooting an issue and I think I've found some potential improvements.
I was trying to use this in a docker container, with Caddy as the frontend, and some private IPs in the AllowedIP table. So I looked through the code, mainly
get_ip_address_from_request()inmiddleware.py.This should be optional or changed.
X-Forwarded-For can be set by the client. In the case of a reverse proxy like nginx or caddy, their default behaviour is to append their IP to it, not rewrite it. Good security is telling the reverse proxy to rewrite it, so this isn't the responsibility of this project, but it would help security to disable reading the IP from x-forwarded-for by default, and documenting the risks and best-practises along with how to enable it. Alternatively you could read the right-most x-forwarded-for IP which would be the last reverse proxy in the default configuration, or the rewritten client IP if somebody's secured their reverse proxy. In the case of no reverse proxy it could still be spoofed though.
I think I'm going to restrict access to
/admin/via caddy but just wanted to mention these for others in case they run into the same things.