L4 module "remote_ip_list" to support fail2ban

[trefzaxSICKAG]

This PR provides a module to integrate fail2ban into L4 and works similar to https://github.com/Javex/caddy-fail2ban.
The module can be used to protect protocols on layer 4 against brute force attacks by allowing to block an IP address after too many failed login attempts. I have used it to protect a MQTT server, but it could also be used for other protocols with login mechanisms like FTP, SSH, OPC UA, SNMP, IMAP, POP3, SMTP, LDAP...

The module introduces a matcher remote_ip_list which can be used to check if the IP address of the sender is contained in a remote IP list. This can be used to e.g. ban the matched IP.

Example Caddyfile:

layer4 {
    :1883 {
        @notbanned {
            not {
                remote_ip_list banned-ips
            }
        }
        route @notbanned {
            proxy {
                proxy_protocol v1
                upstream message-broker:1883
            }
        }
    }
}

[vnxme]

Hi! I actually like the idea of this matcher. As far as I understand, it requires https://github.com/Javex/caddy-fail2ban (a realtively new plugin I haven't conducted a deep analysis of, but it will be inside a go binary, so it's not a big deal then) and https://github.com/fail2ban/fail2ban (a completely separate non-go project). In other words, even if Caddy is built with caddy-l4 and this matcher included, the desired fail2ban functionality won't work unless one installs and configures fail2ban separately. Thus, it makes me doubt whether this matcher should be part of caddy-l4, https://github.com/Javex/caddy-fail2ban, or even be a separate solution.

[mohammed90]

One thing stood out to me. I couldn't capture all the references as I'm trying to do it on the phone.

[trefzaxSICKAG]

Hi! I actually like the idea of this matcher. As far as I understand, it requires https://github.com/Javex/caddy-fail2ban (a realtively new plugin I haven't conducted a deep analysis of, but it will be inside a go binary, so it's not a big deal then) and https://github.com/fail2ban/fail2ban (a completely separate non-go project). In other words, even if Caddy is built with caddy-l4 and this matcher included, the desired fail2ban functionality won't work unless one installs and configures fail2ban separately. Thus, it makes me doubt whether this matcher should be part of caddy-l4, https://github.com/Javex/caddy-fail2ban, or even be a separate solution.

Hi, thank you for your review!
The name of the matcher is probably a bit misleading as it basically just monitors a specific file in the file system for banned IPs.
So you don't necessarily need to use fail2ban along with it, although that was the intention behind this module. https://github.com/Javex/caddy-fail2ban does the same for Caddy, this module provides the functionality for L4.

[vnxme]

@trefzaxSICKAG Thanks for keeping your PR updated.

I have another topic for discussion with regards to this matcher: shall we actually name it fail2ban? What this matcher does is basically checking if a remote IP is present in the list stored in a predefined file. But once checked and matched, it's up to the user whether to ban it or serve anything. In my view, fail2ban and anything ban-related wouldn't suit this matcher. So I would propose remote_ip_list or remote_ip_file as simple and self-explanatory alternatives.

If the renaming is approved, I think the code has to be cleared to exclude any words derived from ban to avoid confusion.

[trefzaxSICKAG]

Thank you for your suggestion @vnxme
I have renamed the module to remote_ip_list and removed all variables derived from ban. I hope this makes the module easier to understand.

[mohammed90]

At this point, this is just a remote_ip matcher, which already exists. Maybe the only difference is that it watches for the file update.

[mholt]

Thanks again for the contribution/working on this. I don't really have much/any experience with fail2ban, so I will continue to defer other reviews to the others who have already commented, but I just had a couple stylistic nits. Conventionally, initialisms in Go are capitalized, so all instances of Ip in identifiers should become IP. I commented on a few of them but there are more.

I'm good with merging this, however, it does bring in fsnotify, a moderately complex dependency (not in terms of dependencies, thankfully, just code), and it would be a new dependency for Caddy if, someday, the caddy-l4 module is merged into the standard distribution. There's a good chance this would be an external plugin at that point (as would possibly some other L4 modules but I haven't looked closely yet). That's okay -- just an FYI for the future.

Thanks!

[vnxme]

It's more about code styling, but is there much sense in a one-line function that is called from a single place? I believe, it's just an additional execution hop we could eliminate.

[trefzaxSICKAG]

I left it like this for the time being to simplify the code at the provision. The idea was to avoid starting goroutines there.

[trefzaxSICKAG]

Thank you for your review @mholt
I have changed the identifiers containing Ip to IP and renamed the JSON struct tag. Also the tests should now be successful for Windows, there was a problem with joining file paths.