outbound: log message when ignoring local MX (#3285)

...where a log entry can be emitted telling why a MX is rejected

Changes.md

@@ -1,9 +1,15 @@
 
 ### Unreleased
 
+#### Changed
+
+- outbound/mx_lookup: make it async/await
+
 #### Fixed
 
 - fix(outbound): allow LHLO over insecure socket if TLS is forced #3278
+- outbound: emit log message when ignoring local MX
+- fix(outbound): don't send SNI servername when connecting to an IP
 - fix(outbound): chown queue dir after creation #3291
 
 ### [3.0.3] - 2024-02-07

outbound/hmail.js

@@ -227,9 +227,7 @@ class HMailItem extends events.EventEmitter {
         }
 
         // if none of the above return codes, drop through to this...
-        mx_lookup.lookup_mx(this.todo.domain, (err, mxs) => {
-            this.found_mx(err, mxs);
-        });
+        mx_lookup.lookup_mx(this.todo.domain, this.found_mx)
     }
 
     found_mx (err, mxs) {
@@ -287,7 +285,7 @@ class HMailItem extends events.EventEmitter {
         }
     }
 
-    try_deliver () {
+    async try_deliver () {
 
         // check if there are any MXs left
         if (this.mxlist.length === 0) {
@@ -300,6 +298,13 @@ class HMailItem extends events.EventEmitter {
         const mx = this.mxlist.shift();
         const host = mx.exchange;
 
+        if (!obc.cfg.local_mx_ok) {
+            if (await net_utils.is_local_host(host)) {
+                this.loginfo(`MX ${host} is local, skipping since local_mx_ok=false`)
+                return this.try_deliver(); // try next MX
+            }
+        }
+
         this.force_tls = this.todo.force_tls;
         if (!this.force_tls) {
             if (net_utils.ip_in_list(obtls.cfg.force_tls_hosts, host)) {

outbound/mx_lookup.js

@@ -1,11 +1,9 @@
 "use strict";
 
-const dns         = require('dns');
-const net_utils   = require('haraka-net-utils')
+const dns        = require('node:dns').promises;
+const net_utils  = require('haraka-net-utils')
 
-const obc         = require('./config');
-
-exports.lookup_mx = function lookup_mx (domain, cb) {
+exports.lookup_mx = async function lookup_mx (domain, cb) {
     const mxs = [];
 
     // Possible DNS errors
@@ -22,49 +20,41 @@ exports.lookup_mx = function lookup_mx (domain, cb) {
     // EREFUSED
     // SERVFAIL
 
-    // default wrap_mx just returns our object with "priority" and "exchange" keys
-    let wrap_mx = a => a;
-    async function process_dns (err, addresses) {
-        if (err) {
-            if (err.code === 'ENODATA' || err.code === 'ENOTFOUND') {
-                // Most likely this is a hostname with no MX record
-                // Drop through and we'll get the A record instead.
-                return 0;
-            }
-            cb(err);
+    try {
+        const addresses = await net_utils.get_mx(domain)
+        for (const a of addresses) {
+            mxs.push(a);
         }
-        else if (addresses?.length) {
-            for (let i=0,l=addresses.length; i < l; i++) {
-                if (
-                    obc.cfg.local_mx_ok ||
-                    await net_utils.is_local_host(addresses[i].exchange).catch(() => null) === false
-                ) {
-                    const mx = wrap_mx(addresses[i]);
-                    mxs.push(mx);
-                }
-            }
-            cb(null, mxs);
+        if (mxs.length) {
+            if (cb) return cb(null, mxs)
+            return mxs
         }
-        else {
-            // return zero if we need to keep trying next option
-            return 0;
+    }
+    catch (err) {
+        switch (err.code) {
+            case 'ENODATA':
+            case 'ENOTFOUND':
+                // likely a hostname with no MX record, drop through
+                break
+            default:
+                throw err(err)
         }
-        return 1;
     }
 
-    net_utils.get_mx(domain, async (err, addresses) => {
-        if (await process_dns(err, addresses)) return;
+    // No MX record, try resolving A record
+
+    // wrap addresses with "priority" and "exchange" keys
+    const wrap_mx = a => ({priority:0,exchange:a});
+
+    const addresses = await dns.resolve(domain)
+    for (const a of addresses) {
+        mxs.push(wrap_mx(a));
+    }
 
-        // if MX lookup failed, we lookup an A record. To do that we change
-        // wrap_mx() to return same thing as resolveMx() does.
-        wrap_mx = a => ({priority:0,exchange:a});
-        // IS: IPv6 compatible
-        dns.resolve(domain, async (err2, addresses2) => {
-            if (await process_dns(err2, addresses2)) return;
+    if (mxs.length) return mxs
 
-            err2 = new Error("Found nowhere to deliver to");
-            err2.code = 'NOMX';
-            cb(err2);
-        });
-    });
+    const err = new Error("Found nowhere to deliver to");
+    err.code = 'NOMX';
+    if (cb) return cb(err)
+    throw err
 }

outbound/tls.js

@@ -1,5 +1,7 @@
 'use strict';
 
+const net          = require('net')
+
 const logger       = require('../logger');
 const tls_socket   = require('../tls_socket');
 const config       = require('haraka-config');
@@ -69,6 +71,7 @@ class OutboundTLS {
     }
 
     get_tls_options (mx) {
+        if (net.isIP(mx.exchange)) return this.cfg
         return Object.assign(this.cfg, {servername: mx.exchange});
     }
 

package.json

@@ -17,11 +17,11 @@
   },
   "main": "haraka.js",
   "engines": {
-    "node": ">=16"
+    "node": ">=18"
   },
   "dependencies": {
-    "address-rfc2821": "^2.1.1",
-    "address-rfc2822": "^2.1.0",
+    "address-rfc2821": "^2.1.2",
+    "address-rfc2822": "^2.2.0",
     "async": "^3.2.5",
     "daemon": "~1.1.0",
     "ipaddr.js": "~2.1.0",

tests/outbound/index.js

@@ -166,10 +166,7 @@ exports.get_tls_options = {
         const testDir = path.resolve('tests');
         this.outbound.config = this.outbound.config.module_config(testDir);
         this.obtls.test_config(tls_socket.config.module_config(testDir), this.outbound.config);
-        this.obtls.init(() => {
-            done();
-        })
-
+        this.obtls.init(done)
     },
     tearDown: done => {
         delete process.env.HARAKA_TEST_DIR;

tests/outbound/mx_lookup.js

@@ -0,0 +1,45 @@
+
+const mx = require('../../outbound/mx_lookup');
+
+exports.lookup_mx = {
+    'MX records for example.com (await)': async test => {
+        test.expect(1)
+        const r = await mx.lookup_mx('example.com');
+        test.deepEqual(r, [ { exchange: '', priority: 0 }])
+        test.done()
+    },
+    'MX records for example.com (cb)': test => {
+        test.expect(1)
+        mx.lookup_mx('example.com', (err, r) => {
+            test.deepEqual(r, [ { exchange: '', priority: 0 }])
+            test.done()
+        });
+    },
+    'MX records for tnpi.net (await)': async test => {
+        test.expect(1)
+        const r = await mx.lookup_mx('tnpi.net');
+        test.deepEqual(r, [ { exchange: 'mail.theartfarm.com', priority: 10 }])
+        test.done()
+    },
+    'MX records for tnpi.net (cb)': test => {
+        test.expect(1)
+        mx.lookup_mx('tnpi.net', (err, r) => {
+            test.deepEqual(r, [ { exchange: 'mail.theartfarm.com', priority: 10 }])
+            test.done()
+        });
+    },
+    'MX records for gmail.com (await)': async test => {
+        test.expect(1)
+        const r = await mx.lookup_mx('gmail.com');
+        // console.log(r)
+        test.ok(r.length)
+        test.done()
+    },
+    'MX records for gmail.com (callback)': test => {
+        test.expect(1)
+        mx.lookup_mx('gmail.com', (err, r) => {
+            test.ok(r.length)
+            test.done()
+        });
+    },
+}