fix(localhost-loop): fix outbound loop when target mx is localhost

[DoobleD]

Fixes #2809

Changes proposed in this pull request:

• keep target mx only if its IP is not a local IP

Checklist:

• docs updated
• tests updated
• Changes updated

[baudehlo]

I would wrap this in some sort of config too, something like cfg.local_mx_ok (which will disable this check, i.e. outbound to local IPs is disabled by default, as per this patch), and add it to booleans in outbound/config.js, and don't forget the docs.

[msimerson]

I'm not sure this is the right default. There are plenty of use cases where a MTA accepts mail and then delivers it to localhost on another port.

[baudehlo]

Sure but I guess my opinion is that, that is the rare scenario for people who know what they are doing. If we add a flag to it we have to pick a default and default safe seems best.

…

On Jun 15, 2021, at 6:50 PM, Matt Simerson ***@***.***> wrote:  I'm not sure this is the right default. There are plenty of use cases where a MTA accepts mail and then delivers it to localhost on another port. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[DoobleD]

Thanks for the feedback. Here's an attempt at a config param as suggested.

[msimerson]

LGTM

[analogic]

Sorry for being late to the party but addresses[i].exchange gives me hostname not IP

[DoobleD]

Thanks for the feedback @analogic.

I just tested it and you're right, I get the hostname too, whereas net_utils.is_local_ip seems to expect an ip.

The change solves #2809 anyway because (from what I understand) the issue was addressing cases where the MX record was returning a local IP (not a hostname), such as with domain yeah.com:

$ dig MX yeah.com
...
yeah.com.		534	IN	MX	1000 0.0.0.0.
...

In that case addresses = [ { exchange: '0.0.0.0', priority: 1000 } ] and the fix does work.

The fix would fail for an MX record returning a hostname resolving to a local ip. Is this a legitimate use case? If yes, perhaps we could use net_utils.get_ips_by_host [source] to resolve addresses[i].exchange to an ip? I'm wondering if this can badly impact performance though. I'm not knowledgeable enough on Haraka to evaluate this.

[msimerson]

use net.isIP to define if .exchange is an IP or not. When its not an IP, using net_utils.get_ips_by_host to resolve should be fine.

[DoobleD]

I've made two PRs as a fix attempt:

• prevent local delivery loop when target exchange resolves to a local hostname #3002
• add is_local_host function haraka-net-utils#63