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 😄). 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 322543c. 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 😆 )

[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 :)