feature: IP address allow/deny list

[stapelberg]

This depends on config file support.

[joonas-fi]

In case you'd get any inspiration out of it, I recently added IP filtering in my self-built HTTP loadbalancer (yes, I know, I'm not right in the head :smile:). Its test is here:

https://github.com/function61/edgerouter/blob/49503f3d41309b426246eb4fbd86a8123657b623/pkg/erserver/ipfilter_test.go#L32

And the filter implementation is here: https://github.com/function61/edgerouter/blob/49503f3d41309b426246eb4fbd86a8123657b623/pkg/erserver/ipfilter.go#L36

Observations:

• I found inet.af/netaddr helpful for IP prefix matching
• The rule model (= allow to all / allow to only specified) might have some overlap with this project if you desire a feature where some IPs can access all rsync modules, some IPs can only access some rsync modules, e.g.:

allow_all { prefix = "127.0.0.0/24" } # this exact server
allow_all { prefix = "192.168.1.0/24" } # trusted VLAN
allow_all { prefix = "100.75.44.30/32" } # joonas work

allow_specified {
    prefix = "100.56.80.66/32" # joonas phone
    modules = ["test"]
}

[stapelberg]

Thanks for the tips! I went with a similar approach in commit https://github.com/gokrazy/rsync/commit/322543c7c9ee5f9b2128b6f7ccc931d05ae21df1. Let me know if you have any feedback on that :)

[joonas-fi]

No problem!

Looks good, the only thing I was unsure about is the validation relationship between the if and there not being a default branch in the switch:

I know the code works like it's supposed to, but I'm worried if there's ever refactoring, the two sections have an unwritten relationship and they're some distance apart.

I tried to have a go at refactoring them together, but am not sure if it's better:

ipMatches,err:=func()(bool,error){  
    if who == "all" {
        // The all keyword matches any remote IP address
        return true, nil
    } else {
        _, net, err := net.ParseCIDR(who)
        if err != nil {
            return false, fmt.Errorf("invalid acl: %q (syntax: allow|deny <all|ipnet>)", acl)
        }

        return net.Contains(remoteIP),nil
    }
}()
if err!=nil{
    return err
}

switch action {
case "allow":
    if ipMatches {
        return nil
    } else {
        continue // Skip this instruction, the remote IP does not match
    }
case "deny":
    return ipMatches {
        return fmt.Errorf("access denied (acl %q)", acl)
    } else {
        continue // Skip this instruction, the remote IP does not match
    }
default:
    return fmt.Errorf("invalid acl: %q (syntax: allow|deny <all|ipnet>)", acl)
}

(excuse the non-fmt'd code, I'm used to writing sloppy code and having fmt fix it, it's therapeutic :laughing: )

[stapelberg]

Thanks for flagging this. I have added a default statement which errors out, and I think that’s a pragmatic way of catching the most severe issues with this construct :)