kd2org / karadav

Lightweight NextCloud compatible WebDAV server
https://fossil.kd2.org/karadav/
GNU Affero General Public License v3.0
155 stars 14 forks source link

Enable fail2ban by logging failed requests #37

Closed piaste closed 1 year ago

piaste commented 1 year ago

TL;DR: please log the IP (X-Forwarded-For) of failed accesses to error.log, so fail2ban can pick them up and ban them.

I'm trying out this project and I'm quite impressed with the speed, great work! :)

Before using it for real data, however, I would want to harden it a little. One thing that's really easy to add is fail2ban. It works by monitoring any log file, looking for a well-defined pattern representing an unauthorized access attempt, nd bans the offending IPs.

To support fail2ban, KaraDAV would need to:

1) Read the client IP, using the X-Forwarded-For header if present (otherwise it breaks reverse proxies!)

2) If password_verify() fails, print something like "{IP} - Login failed as user {user}: wrong password"

3) If a client attempts to request a non-index.php or command while not logged in (or logged in as the wrong user), print something like "{IP} - Attempted to access unauthorized page"

I can make a quick PR for this if necessary.

bohwaz commented 1 year ago

Good idea, just two details:

  1. X-Forwarded-For should not be trusted by default (or a user could forge a IP), this should be chosen by the user if required, also the header should be configured, some proxies send a different header.
  2. This should be logged to a different file, eg. access.log, error.log is for PHP errors.

For example you could add those two constants in config:

/**
* Set to NULL to disable access log
 * @var null|string
 */
const ACCESS_LOG = __DIR__ . '/access.log';

/**
 * Set to the name of the trusted header containing the client IP, if behind a reverse proxy
 * If set to NULL, then the connecting client IP will be used.
 * @var string|null
 */
const TRUSTED_IP_HEADER = 'X-Forwarded-For';

Yes please make a PR, I don't have time to implement this sorry :)

piaste commented 1 year ago

Thanks for the feedback! I was also looking at SFTPGo and noticed that it has already handled some more reverse proxy setups in its configuration:

https://github.com/drakkan/sftpgo/blob/main/docs/full-configuration.md

  • proxy_allowed, list of IP addresses and IP ranges allowed to set client IP proxy header such as X-Forwarded-For. Any client IP proxy headers, if set on requests from a connection address not in this list, will be silently ignored. Default: empty.
  • client_ip_proxy_header, string. Defines the allowed client IP proxy header such as X-Forwarded-For, X-Real-IP etc. Default: empty
  • client_ip_header_depth, integer. Some client IP headers such as X-Forwarded-For can contain multiple IP address, this setting define the position to trust starting from the right. For example if we have: 10.0.0.1,11.0.0.1,12.0.0.1,13.0.0.1 and the depth is 0, SFTPGo will use 13.0.0.1 as client IP, if depth is 1, 12.0.0.1 will be used and so on. Default: 0.

I think I will follow this format.