From 042d0a85019961c0513df31fd8b63b4e47879673 Mon Sep 17 00:00:00 2001 From: Andrew Fyfe Date: Mon, 1 Jan 2018 21:07:24 +0000 Subject: [PATCH] add IPv6 support to mx check - add support for looking up IPv6 addresses (socket.getaddrinfo) - add support for International Domain Names in case the DNS reponse contains one, to_unicode() decodes them to a utf8 string - add logging for potential configuration errors: - DNS configuration errors (i.e. no MX records) - server name resolution errors (i.e. resolv.conf doesn't contain a working name server) - invalid values in MX records - when looking up an ip address, don't stop on the first failure check all addresses - update mock results for tests, return a real MX record for mock dns queries --- modoboa/admin/lib.py | 59 ++++++++---- .../management/commands/subcommands/_mx.py | 3 +- modoboa/admin/tests/test_api.py | 16 ++-- modoboa/admin/tests/test_domain.py | 15 +-- modoboa/admin/tests/test_import_.py | 19 ++-- modoboa/admin/tests/test_mx.py | 96 ++++++++++++++++--- modoboa/admin/tests/utils.py | 62 +++++++++++- test_project/test_project/settings.py | 1 + 8 files changed, 210 insertions(+), 61 deletions(-) diff --git a/modoboa/admin/lib.py b/modoboa/admin/lib.py index 3955ec25b..96beb2608 100644 --- a/modoboa/admin/lib.py +++ b/modoboa/admin/lib.py @@ -7,14 +7,17 @@ from __future__ import unicode_literals from functools import wraps import ipaddress from itertools import chain +import logging import random import socket import string +from dns.name import IDNA_2008_UTS_46 import dns.resolver from django.contrib.contenttypes.models import ContentType from django.db.models import Q +from django.utils.encoding import smart_text from django.utils.translation import ugettext as _ from modoboa.core.models import User @@ -170,39 +173,55 @@ def import_dlist(user, row, formopts): _import_alias(user, row) -def get_domain_mx_list(name): +def get_domain_mx_list(domain): """Return a list of MX IP address for domain.""" result = [] + logger = logging.getLogger("modoboa.admin") try: - answers = dns.resolver.query(name, "MX") - except (dns.resolver.NoAnswer, dns.resolver.NXDOMAIN, - dns.resolver.NoNameservers): - return result - for answer in answers: - try: - # work if .exchange is a name or IP - address = socket.gethostbyname(str(answer.exchange)) - except socket.gaierror: - pass - else: + dns_answers = dns.resolver.query(domain, "MX") + except dns.resolver.NXDOMAIN as e: + logger.error(_("No DNS records found for %s") % domain, exc_info=e) + except dns.resolver.NoAnswer as e: + logger.error(_("No MX record for %s") % domain, exc_info=e) + except dns.resolver.NoNameservers as e: + logger.error(_("No working name servers found"), exc_info=e) + except dns.resolver.Timeout as e: + logger.warning( + _("DNS resolution timeout, unable to query %s at the moment") % domain, + exc_info=e) + else: + for dns_answer in dns_answers: try: - # we must have a valid IP - address = ipaddress.ip_address("{}".format(address)) - except ValueError: - pass + mx_domain = dns_answer.exchange.to_unicode( + omit_final_dot=True, idna_codec=IDNA_2008_UTS_46) + ip_answers = socket.getaddrinfo( + mx_domain, 25, 0, socket.SOCK_STREAM) + except socket.gaierror as e: + logger.warning( + _("Unable to lookup ip addresses for %s; %s") % ( + mx_domain, str(e)), + exc_info=e) else: - result.append((str(answer.exchange), address)) + for ip_answer in ip_answers: + try: + mx_ip = ipaddress.ip_address( + smart_text(ip_answer[4][0])) + except ValueError as e: + logger.warning( + _("Invalid IP address format for %s; %s") % ( + mx_domain, smart_text(ip_answer[4][0])), + exc_info=e) + else: + result.append((mx_domain, mx_ip)) return result def domain_has_authorized_mx(name): """Check if domain has authorized mx record at least.""" valid_mxs = param_tools.get_global_parameter("valid_mxs") - valid_mxs = [ipaddress.ip_network(u"{}".format(v.strip())) + valid_mxs = [ipaddress.ip_network(smart_text(v.strip())) for v in valid_mxs.split() if v.strip()] domain_mxs = get_domain_mx_list(name) - if len(domain_mxs) == 0: - return True for mx_addr, mx_ip_addr in domain_mxs: for subnet in valid_mxs: if mx_ip_addr in subnet: diff --git a/modoboa/admin/management/commands/subcommands/_mx.py b/modoboa/admin/management/commands/subcommands/_mx.py index 22dce6177..b86955eae 100644 --- a/modoboa/admin/management/commands/subcommands/_mx.py +++ b/modoboa/admin/management/commands/subcommands/_mx.py @@ -12,6 +12,7 @@ from django.core import mail from django.core.mail import EmailMessage from django.core.management.base import BaseCommand from django.template.loader import render_to_string +from django.utils.encoding import smart_text from django.utils.functional import cached_property from django.utils.translation import ugettext as _ @@ -41,7 +42,7 @@ class CheckMXRecords(BaseCommand): def valid_mxs(self): """Return valid MXs set in admin.""" valid_mxs = param_tools.get_global_parameter("valid_mxs") - return [ipaddress.ip_network(u"{}".format(v.strip())) + return [ipaddress.ip_network(smart_text(v.strip())) for v in valid_mxs.split() if v.strip()] def add_arguments(self, parser): diff --git a/modoboa/admin/tests/test_api.py b/modoboa/admin/tests/test_api.py index 0a4cfbf59..7078d616b 100644 --- a/modoboa/admin/tests/test_api.py +++ b/modoboa/admin/tests/test_api.py @@ -73,11 +73,11 @@ class DomainAPITestCase(ModoAPITestCase): self.assertEqual(response.status_code, 403) @patch.object(dns.resolver.Resolver, "query") - @patch("socket.gethostbyname") - def test_create_domain_with_mx_check(self, mock_gethostbyname, mock_query): + @patch("socket.getaddrinfo") + def test_create_domain_with_mx_check(self, mock_getaddrinfo, mock_query): """Check domain creation when MX check is activated.""" self.set_global_parameter("enable_admin_limits", False, app="limits") - self.set_global_parameter("valid_mxs", "1.2.3.4") + self.set_global_parameter("valid_mxs", "192.0.2.1 2001:db8::1") self.set_global_parameter("domains_must_have_authorized_mx", True) reseller = core_factories.UserFactory( username="reseller", groups=("Resellers", )) @@ -85,19 +85,19 @@ class DomainAPITestCase(ModoAPITestCase): self.client.credentials(HTTP_AUTHORIZATION='Token ' + token.key) url = reverse("api:domain-list") - mock_query.return_value = [utils.FakeDNSAnswer("mail.ok.com")] - mock_gethostbyname.return_value = "1.2.3.5" + mock_query.side_effect = utils.mock_dns_query_result + mock_getaddrinfo.side_effect = utils.mock_ip_query_result response = self.client.post( - url, {"name": "test3.com", "quota": 0, "default_mailbox_quota": 10} + url, {"name": "no-mx.example.com", "quota": 0, "default_mailbox_quota": 10} ) self.assertEqual(response.status_code, 400) self.assertEqual( response.json()["name"][0], "No authorized MX record found for this domain") - mock_gethostbyname.return_value = "1.2.3.4" + mock_getaddrinfo.side_effect = utils.mock_ip_query_result response = self.client.post( - url, {"name": "test3.com", "quota": 0, "default_mailbox_quota": 10} + url, {"name": "test4.com", "quota": 0, "default_mailbox_quota": 10} ) self.assertEqual(response.status_code, 201) diff --git a/modoboa/admin/tests/test_domain.py b/modoboa/admin/tests/test_domain.py index 553a861a8..c497e44c8 100644 --- a/modoboa/admin/tests/test_domain.py +++ b/modoboa/admin/tests/test_domain.py @@ -199,21 +199,21 @@ class DomainTestCase(ModoTestCase): self.assertContains(response, "value=\"50\"") @patch.object(dns.resolver.Resolver, "query") - @patch("socket.gethostbyname") - def test_create_and_check_mx(self, mock_gethostbyname, mock_query): + @patch("socket.getaddrinfo") + def test_create_and_check_mx(self, mock_getaddrinfo, mock_query): """Check for authorized MX record.""" reseller = core_factories.UserFactory( username="reseller", groups=("Resellers", )) self.client.force_login(reseller) self.set_global_parameter("enable_admin_limits", False, app="limits") - self.set_global_parameter("valid_mxs", "1.2.3.4") + self.set_global_parameter("valid_mxs", "192.0.2.1 2001:db8::1") self.set_global_parameter("domains_must_have_authorized_mx", True) - mock_query.return_value = [utils.FakeDNSAnswer("mail.ok.com")] - mock_gethostbyname.return_value = "1.2.3.5" + mock_query.side_effect = utils.mock_dns_query_result + mock_getaddrinfo.side_effect = utils.mock_ip_query_result values = { - "name": "pouet.com", "quota": 0, "default_mailbox_quota": 0, + "name": "no-mx.example.com", "quota": 0, "default_mailbox_quota": 0, "create_dom_admin": True, "dom_admin_username": "toto", "create_aliases": True, "type": "domain", "stepid": "step3", "with_mailbox": True @@ -222,7 +222,8 @@ class DomainTestCase(ModoTestCase): self.assertFalse( Domain.objects.filter(name=values["name"]).exists()) - mock_gethostbyname.return_value = "1.2.3.4" + values["name"] = "pouet.com" + mock_getaddrinfo.side_effect = utils.mock_ip_query_result self.ajax_post(reverse("admin:domain_add"), values) def test_modify(self): diff --git a/modoboa/admin/tests/test_import_.py b/modoboa/admin/tests/test_import_.py index 370c2087c..4444797b7 100644 --- a/modoboa/admin/tests/test_import_.py +++ b/modoboa/admin/tests/test_import_.py @@ -2,8 +2,8 @@ from __future__ import unicode_literals -from mock import patch import dns.resolver +from mock import patch from django.core.files.base import ContentFile from django.urls import reverse @@ -80,19 +80,19 @@ domainalias; domalias1.com; domain1.com; True "Default mailbox quota cannot be greater than domain quota") @patch.object(dns.resolver.Resolver, "query") - @patch("socket.gethostbyname") - def test_domain_import_with_mx_check(self, mock_gethostbyname, mock_query): + @patch("socket.getaddrinfo") + def test_domain_import_with_mx_check(self, mock_getaddrinfo, mock_query): """Check domain import when MX check is enabled.""" reseller = core_factories.UserFactory( username="reseller", groups=("Resellers", )) self.client.force_login(reseller) - self.set_global_parameter("valid_mxs", "1.2.3.4") + self.set_global_parameter("valid_mxs", "192.0.2.1 2001:db8::1") self.set_global_parameter("domains_must_have_authorized_mx", True) - mock_query.return_value = [utils.FakeDNSAnswer("mail.ok.com")] - mock_gethostbyname.return_value = "1.2.3.5" + mock_query.side_effect = utils.mock_dns_query_result + mock_getaddrinfo.side_effect = utils.mock_ip_query_result f = ContentFile( - b"domain; domain1.com; 100; 1; True", name="domains.csv") + b"domain; test3.com; 100; 1; True", name="domains.csv") resp = self.client.post( reverse("admin:domain_import"), { "sourcefile": f @@ -100,8 +100,9 @@ domainalias; domalias1.com; domain1.com; True ) self.assertContains(resp, "No authorized MX record found for domain") - mock_gethostbyname.return_value = "1.2.3.4" - f.seek(0) + mock_getaddrinfo.side_effect = utils.mock_ip_query_result + f = ContentFile( + b"domain; domain1.com; 100; 1; True", name="domains.csv") resp = self.client.post( reverse("admin:domain_import"), { "sourcefile": f diff --git a/modoboa/admin/tests/test_mx.py b/modoboa/admin/tests/test_mx.py index 53d2a94c6..38361b406 100644 --- a/modoboa/admin/tests/test_mx.py +++ b/modoboa/admin/tests/test_mx.py @@ -2,17 +2,22 @@ from __future__ import unicode_literals +import dns.resolver from mock import patch +from testfixtures import LogCapture from django.core import mail from django.core import management -from django.urls import reverse from django.test import override_settings +from django.urls import reverse +from django.utils.translation import ugettext as _ from modoboa.core import factories as core_factories from modoboa.lib.tests import ModoTestCase +from . import utils from .. import factories +from ..lib import get_domain_mx_list from .. import models @@ -25,25 +30,33 @@ class MXTestCase(ModoTestCase): super(MXTestCase, cls).setUpTestData() cls.domain = factories.DomainFactory(name="modoboa.org") # should not exist - cls.bad_domain = factories.DomainFactory(name="pouet.com") + cls.bad_domain = factories.DomainFactory(name="does-not-exist.example.com") # Add domain admin with mailbox mb = factories.MailboxFactory( address="admin", domain=cls.bad_domain, - user__username="admin@pouet.com", + user__username="admin@does-not-exist.example.com", user__groups=("DomainAdmins", ) ) cls.bad_domain.add_admin(mb.user) # Add domain admin with no mailbox admin = core_factories.UserFactory( - username="admin2@pouet.com", groups=("DomainAdmins", )) + username="admin2@does-not-exist.example.com", groups=("DomainAdmins", )) cls.bad_domain.add_admin(admin) - cls.localconfig.parameters.set_value("valid_mxs", "127.0.0.1") + cls.localconfig.parameters.set_value( + "valid_mxs", "192.0.2.1 2001:db8::1") cls.localconfig.save() models.MXRecord.objects.all().delete() - def test_management_command(self): + @patch("gevent.socket.gethostbyname") + @patch("socket.getaddrinfo") + @patch.object(dns.resolver.Resolver, "query") + def test_management_command( + self, mock_query, mock_getaddrinfo, mock_g_gethostbyname): """Check that command works fine.""" + mock_query.side_effect = utils.mock_dns_query_result + mock_getaddrinfo.side_effect = utils.mock_ip_query_result + mock_g_gethostbyname.return_value = "1.2.3.4" self.assertEqual(models.MXRecord.objects.count(), 0) management.call_command("modo", "check_mx", "--no-dnsbl", "--ttl=0") self.assertTrue( @@ -63,8 +76,15 @@ class MXTestCase(ModoTestCase): qs = models.MXRecord.objects.filter(domain=self.domain) self.assertEqual(id, qs[0].id) - def test_single_domain_update(self): + @patch("gevent.socket.gethostbyname") + @patch("socket.getaddrinfo") + @patch.object(dns.resolver.Resolver, "query") + def test_single_domain_update( + self, mock_query, mock_getaddrinfo, mock_g_gethostbyname): """Update only one domain.""" + mock_query.side_effect = utils.mock_dns_query_result + mock_getaddrinfo.side_effect = utils.mock_ip_query_result + mock_g_gethostbyname.return_value = "1.2.3.4" management.call_command( "modo", "check_mx", "--domain", self.domain.name) self.assertTrue( @@ -81,6 +101,36 @@ class MXTestCase(ModoTestCase): management.call_command( "modo", "check_mx", "--domain", "toto.com") + @patch("socket.getaddrinfo") + @patch.object(dns.resolver.Resolver, "query") + def test_get_domain_mx_list_logging(self, mock_query, mock_getaddrinfo): + mock_query.side_effect = utils.mock_dns_query_result + mock_getaddrinfo.side_effect = utils.mock_ip_query_result + with LogCapture("modoboa.admin") as log: + get_domain_mx_list("does-not-exist.example.com") + get_domain_mx_list("no-mx.example.com") + get_domain_mx_list("no-ns-servers.example.com") + get_domain_mx_list("timeout.example.com") + get_domain_mx_list("no-lookup.example.com") + get_domain_mx_list("bad-response.example.com") + log.check( + ("modoboa.admin", "ERROR", + _("No DNS records found for %s") + % "does-not-exist.example.com"), + ("modoboa.admin", "ERROR", + _("No MX record for %s") % "no-mx.example.com"), + ("modoboa.admin", "ERROR", _("No working name servers found")), + ("modoboa.admin", "WARNING", + _("DNS resolution timeout, unable to query %s at the moment") + % "timeout.example.com"), + ("modoboa.admin", "WARNING", + _("Unable to lookup ip addresses for %s; %s") + % ("does-not-exist.example.com", "")), + ("modoboa.admin", "WARNING", + _("Invalid IP address format for %s; %s") + % ("bad-response.example.com", "BAD")), + ) + @override_settings(DNSBL_PROVIDERS=["zen.spamhaus.org"]) class DNSBLTestCase(ModoTestCase): @@ -91,15 +141,22 @@ class DNSBLTestCase(ModoTestCase): """Create some data.""" super(DNSBLTestCase, cls).setUpTestData() cls.domain = factories.DomainFactory(name="modoboa.org") - factories.DomainFactory(name="pouet.com") # should not exist + factories.DomainFactory(name="does-not-exist.example.com") cls.domain2 = factories.DomainFactory( name="test.localhost") # Should not be checked cls.domain3 = factories.DomainFactory( name="modoboa.com", enable_dns_checks=False) models.DNSBLResult.objects.all().delete() - def test_management_command(self): + @patch("gevent.socket.gethostbyname") + @patch("socket.getaddrinfo") + @patch.object(dns.resolver.Resolver, "query") + def test_management_command( + self, mock_query, mock_getaddrinfo, mock_g_gethostbyname): """Check that command works fine.""" + mock_query.side_effect = utils.mock_dns_query_result + mock_getaddrinfo.side_effect = utils.mock_ip_query_result + mock_g_gethostbyname.return_value = "1.2.3.4" self.assertEqual(models.DNSBLResult.objects.count(), 0) management.call_command("modo", "check_mx") self.assertTrue( @@ -113,15 +170,30 @@ class DNSBLTestCase(ModoTestCase): self.assertTrue(self.domain2.uses_a_reserved_tld) @patch("gevent.socket.gethostbyname") - def test_notifications(self, mock_gethostbyname): + @patch("socket.getaddrinfo") + @patch.object(dns.resolver.Resolver, "query") + def test_notifications( + self, mock_query, mock_getaddrinfo, mock_g_gethostbyname): """Check notifications.""" - mock_gethostbyname.return_value = "1.2.3.4" + mock_query.side_effect = utils.mock_dns_query_result + mock_getaddrinfo.side_effect = utils.mock_ip_query_result + mock_g_gethostbyname.return_value = "1.2.3.4" management.call_command( "modo", "check_mx", "--email", "user@example.test") + if len(mail.outbox) != 2: + for message in mail.outbox: + print(message.subject) self.assertEqual(len(mail.outbox), 2) - def test_management_command_no_dnsbl(self): + @patch("gevent.socket.gethostbyname") + @patch("socket.getaddrinfo") + @patch.object(dns.resolver.Resolver, "query") + def test_management_command_no_dnsbl( + self, mock_query, mock_getaddrinfo, mock_g_gethostbyname): """Check that command works fine without dnsbl.""" + mock_query.side_effect = utils.mock_dns_query_result + mock_getaddrinfo.side_effect = utils.mock_ip_query_result + mock_g_gethostbyname.return_value = "1.2.3.4" self.assertEqual(models.DNSBLResult.objects.count(), 0) management.call_command("modo", "check_mx", "--no-dnsbl") self.assertFalse( diff --git a/modoboa/admin/tests/utils.py b/modoboa/admin/tests/utils.py index bd981323a..8d5d64c26 100644 --- a/modoboa/admin/tests/utils.py +++ b/modoboa/admin/tests/utils.py @@ -1,8 +1,62 @@ +# -*- coding: utf-8 -*- + """Test utilities.""" +from __future__ import unicode_literals -class FakeDNSAnswer(object): - """Fake answer.""" +import socket - def __init__(self, exchange): - self.exchange = exchange +from dns.name import Name +from dns.rdtypes.ANY.MX import MX +from dns.resolver import NoAnswer, NoNameservers, NXDOMAIN, Timeout + +_MX_RECORD_1 = MX("IN", "MX", 10, Name("mx.example.com".split("."))) +_MX_RECORD_2 = MX("IN", "MX", 10, Name("mx2.example.com".split("."))) +_BAD_MX_RECORD = MX("IN", "MX", 10, Name("bad-response.example.com".split("."))) +_DNE_MX_RECORD = MX("IN", "MX", 10, Name("does-not-exist.example.com".split("."))) +_MX_RECORDS = [_MX_RECORD_1] + +_IPV4_RECORD_1 = (socket.AF_INET, socket.SOCK_STREAM, 6, "", ("192.0.2.1", 25)) +_IPV4_RECORD_2 = (socket.AF_INET, socket.SOCK_STREAM, 6, "", ("192.0.2.2", 25)) +_IPV6_RECORD = (socket.AF_INET, socket.SOCK_STREAM, 6, "", ("2001:db8::1", 25)) +_BAD_IP_RECORD = (socket.AF_INET, socket.SOCK_STREAM, 6, "", ("BAD", 25)) +_IP_RECORDS = [_IPV4_RECORD_1, _IPV6_RECORD] + +_POSSIBLE_DNS_RESULTS = { + "test3.com": [_MX_RECORD_2], + "no-mx.example.com": NoAnswer(), + "does-not-exist.example.com": NXDOMAIN(), + "timeout.example.com": Timeout(), + "no-ns-servers.example.com": NoNameservers(), + "bad-response.example.com": [_BAD_MX_RECORD], + "no-lookup.example.com": [_DNE_MX_RECORD], +} + +_POSSIBLE_IP_RESULTS = { + "test3.com": [_IPV4_RECORD_2], + "mx2.example.com": [_IPV4_RECORD_2], + "bad-response.example.com": [_BAD_IP_RECORD], + "does-not-exist.example.com": socket.gaierror(), + "192.0.2.254": socket.gaierror(), + "2001:db8:254": socket.gaierror(), +} + + +def mock_dns_query_result(qname, *args, **kwargs): + if qname in _POSSIBLE_DNS_RESULTS: + if isinstance(_POSSIBLE_DNS_RESULTS[qname], Exception): + raise _POSSIBLE_DNS_RESULTS[qname] + else: + return _POSSIBLE_DNS_RESULTS[qname] + + return _MX_RECORDS + + +def mock_ip_query_result(host, port, *args, **kwargs): + if host in _POSSIBLE_IP_RESULTS: + if isinstance(_POSSIBLE_IP_RESULTS[host], Exception): + raise _POSSIBLE_IP_RESULTS[host] + else: + return _POSSIBLE_IP_RESULTS[host] + + return _IP_RECORDS diff --git a/test_project/test_project/settings.py b/test_project/test_project/settings.py index 3f9d0dcda..ca0df91f2 100644 --- a/test_project/test_project/settings.py +++ b/test_project/test_project/settings.py @@ -237,6 +237,7 @@ CKEDITOR_CONFIGS = { LOGGING = { 'version': 1, + 'disable_existing_loggers': False, 'formatters': { 'syslog': { 'format': '%(name)s: %(levelname)s %(message)s'