bug: don't allow invalid entries in the relay_acl_allow file

[bnjbvr]

I was using an inline comment with the following format, in relay_acl_allow (not this exact IP, but shown here as an example):

8.8.8.8/32 # My safe machine IP

Unfortunately, the code later reads this line as a single entry, and is fine parsing the entire line, including what I thought was a comment after #, as an IP. The ipaddr npm module then seems to understand this as 8.8.8.8/0, allowing any IP to pass the relay ACL test.

As a result, my machine was sending spam all over the world, because I've inappropriately assumed that the ACL worked fine, thus didn't impose any restriction in the firewall for that port. I fixed those two mistakes (misconfiguration of the comment + added some firewall rules), but I think it'd be super great to:

1. make it super explicit what is fine or not for a plugin configuration to accept
2. optionally, for the relay plugin, not take into account any line that doesn't look like an IP address, based on testing it with a regular expression or something like this

[bnjbvr]

Renamed the issue, because I think the (2) item on the above list should really be implemented; it's too easy otherwise to shoot oneself in the feet by writing an incorrect configuration line, and thus allowing spammers to use the software.

[bnjbvr]

Minimal repro showing the issue (arguably it's in ipaddr.js):

const ipaddr = require('ipaddr.js');

let cidr = "8.8.8.8/32 # This machine";
cidr = cidr.split('/');
let c_net = cidr[0];
let c_mask = cidr[1] || 32;

let cnetip = ipaddr.parse(c_net);
console.log('c net ip =', cnetip);

let ip = ipaddr.parse("13.37.42.42");
console.log('ip=', ip);

console.log('accepted?', ip.match(cnetip, c_mask));

This will show accepted? true for any value of ip.

[msimerson]

1. Unless you find some examples in the documentation of using inline comments, then the bug here is in your expectations, not in the software.
2. Your expectation is not unreasonable, but you are not reporting a bug, you are asking for a feature.

[bnjbvr]

Thanks for your answer, but I disagree with your assessment of 1: the input must be mistrusted, or errors must be signaled explicitly, or invalid lines must be explicitly ruled out. Anything else will lead an administrator of such an instance to shoot themselves in the feet, as I did in a particularly catastrophic way.

[msimerson]

We look forward to you contributing a Pull Request to brings the file parsing up to your expectations.