fix: raise an error if MX lookup times out during sending

This avoids potentially sending mail to the A record when an MX query times out. closes #2833
2025-12-01 05:43:04 +00:00 · 2024-03-01 21:36:07 +00:00
--- a/app/lib/dns_resolver.rb
+++ b/app/lib/dns_resolver.rb
@@ -4,20 +4,22 @@ require "resolv"

 class DNSResolver

+  class LocalResolversUnavailableError < StandardError
+  end
+
  attr_reader :nameservers
  attr_reader :timeout

-  def initialize(nameservers: nil, timeout: 5)
+  def initialize(nameservers)
    @nameservers = nameservers
-    @timeout = timeout
  end

  # Return all A records for the given name
  #
  # @param [String] name
  # @return [Array<String>]
-  def a(name)
-    get_resources(name, Resolv::DNS::Resource::IN::A).map do |s|
+  def a(name, **options)
+    get_resources(name, Resolv::DNS::Resource::IN::A, **options).map do |s|
      s.address.to_s
    end
  end
@@ -26,8 +28,8 @@ class DNSResolver
  #
  # @param [String] name
  # @return [Array<String>]
-  def aaaa(name)
-    get_resources(name, Resolv::DNS::Resource::IN::AAAA).map do |s|
+  def aaaa(name, **options)
+    get_resources(name, Resolv::DNS::Resource::IN::AAAA, **options).map do |s|
      s.address.to_s
    end
  end
@@ -36,8 +38,8 @@ class DNSResolver
  #
  # @param [String] name
  # @return [Array<String>]
-  def txt(name)
-    get_resources(name, Resolv::DNS::Resource::IN::TXT).map do |s|
+  def txt(name, **options)
+    get_resources(name, Resolv::DNS::Resource::IN::TXT, **options).map do |s|
      s.data.to_s.strip
    end
  end
@@ -46,8 +48,8 @@ class DNSResolver
  #
  # @param [String] name
  # @return [Array<String>]
-  def cname(name)
-    get_resources(name, Resolv::DNS::Resource::IN::CNAME).map do |s|
+  def cname(name, **options)
+    get_resources(name, Resolv::DNS::Resource::IN::CNAME, **options).map do |s|
      s.name.to_s.downcase
    end
  end
@@ -56,8 +58,8 @@ class DNSResolver
  #
  # @param [String] name
  # @return [Array<Array<Integer, String>>]
-  def mx(name)
-    records = get_resources(name, Resolv::DNS::Resource::IN::MX).map do |m|
+  def mx(name, **options)
+    records = get_resources(name, Resolv::DNS::Resource::IN::MX, **options).map do |m|
      [m.preference.to_i, m.exchange.to_s]
    end
    records.sort do |a, b|
@@ -73,13 +75,13 @@ class DNSResolver
  #
  # @param [String] name
  # @return [Array<String>]
-  def effective_ns(name)
+  def effective_ns(name, **options)
    records = []
    parts = name.split(".")
    (parts.size - 1).times do |n|
      d = parts[n, parts.size - n + 1].join(".")

-      records = get_resources(d, Resolv::DNS::Resource::IN::NS).map do |s|
+      records = get_resources(d, Resolv::DNS::Resource::IN::NS, **options).map do |s|
        s.name.to_s
      end

@@ -94,27 +96,31 @@ class DNSResolver
  #
  # @param [String] ip_address
  # @return [String]
-  def ip_to_hostname(ip_address)
-    dns do |dns|
+  def ip_to_hostname(ip_address, **options)
+    dns(**options) do |dns|
      dns.getname(ip_address)&.to_s
    end
-  rescue Resolv::ResolvError
+  rescue Resolv::ResolvError => e
+    raise if e.message =~ /timeout/ && options[:raise_timeout_errors]
+
    ip_address
  end

  private

-  def dns
-    kwargs = @nameservers ? { nameserver: @nameservers } : {}
-    Resolv::DNS.open(**kwargs) do |dns|
-      dns.timeouts = [@timeout, @timeout / 2]
+  def dns(raise_timeout_errors: false)
+    Resolv::DNS.open(nameserver: @nameservers,
+                     raise_timeout_errors: raise_timeout_errors) do |dns|
+      dns.timeouts = [Postal::Config.dns.timeout,
+                      Postal::Config.dns.timeout / 2,
+                      Postal::Config.dns.timeout / 2]
      yield dns
    end
  end

-  def get_resources(name, type)
+  def get_resources(name, type, **options)
    encoded_name = DomainName::Punycode.encode_hostname(name)
-    dns do |dns|
+    dns(**options) do |dns|
      dns.getresources(encoded_name, type)
    end
  end
@@ -126,19 +132,28 @@ class DNSResolver
    # @param [String] name
    # @return [DNSResolver]
    def for_domain(name)
-      resolver = new
-      nameservers = resolver.effective_ns(name)
+      nameservers = local.effective_ns(name)
      ips = nameservers.map do |ns|
-        resolver.a(ns)
+        local.a(ns)
      end.flatten.uniq
-      new(nameservers: ips)
+      new(ips)
    end

    # Return a local resolver to use for lookups
    #
    # @return [DNSResolver]
    def local
-      @local ||= new
+      @local ||= begin
+        resolv_conf_path = Postal::Config.dns.resolv_conf_path
+        raise LocalResolversUnavailableError, "No resolver config found at #{resolv_conf_path}" unless File.file?(resolv_conf_path)
+
+        resolv_conf = Resolv::DNS::Config.parse_resolv_conf(resolv_conf_path)
+        if resolv_conf.nil? || resolv_conf[:nameserver].nil? || resolv_conf[:nameserver].empty?
+          raise LocalResolversUnavailableError, "Could not find nameservers in #{resolv_conf_path}"
+        end
+
+        new(resolv_conf[:nameserver])
+      end
    end

  end
--- a/app/senders/smtp_sender.rb
+++ b/app/senders/smtp_sender.rb
@@ -162,7 +162,7 @@ class SMTPSender < BaseSender
  #
  # @return [Array<String>]
  def resolve_mx_records_for_domain
-    hostnames = DNSResolver.local.mx(@domain).map(&:last)
+    hostnames = DNSResolver.local.mx(@domain, raise_timeout_errors: true).map(&:last)
    return [SMTPClient::Server.new(@domain)] if hostnames.empty?

    hostnames.map { |hostname| SMTPClient::Server.new(hostname) }
--- a/doc/config/environment-variables.md
+++ b/doc/config/environment-variables.md
@@ -63,6 +63,8 @@ This document contains all the environment variables which are available for thi
 | `DNS_DKIM_IDENTIFIER` | String | The identifier to use for DKIM keys in DNS records | postal |
 | `DNS_DOMAIN_VERIFY_PREFIX` | String | The prefix to add before TXT record verification string | postal-verification |
 | `DNS_CUSTOM_RETURN_PATH_PREFIX` | String | The domain to use on external domains which points to the Postal return path domain | psrp |
+| `DNS_TIMEOUT` | Integer | The timeout to wait for DNS resolution | 5 |
+| `DNS_RESOLV_CONF_PATH` | String | The path to the resolv.conf file containing addresses for local nameservers | /etc/resolv.conf |
 | `SMTP_HOST` | String | The hostname to send application-level e-mails to | 127.0.0.1 |
 | `SMTP_PORT` | Integer | The port number to send application-level e-mails to | 25 |
 | `SMTP_USERNAME` | String | The username to use when authentication to the SMTP server |  |
--- a/doc/config/yaml.yml
+++ b/doc/config/yaml.yml
@@ -137,6 +137,10 @@ dns:
  domain_verify_prefix: postal-verification
  # The domain to use on external domains which points to the Postal return path domain
  custom_return_path_prefix: psrp
+  # The timeout to wait for DNS resolution
+  timeout: 5
+  # The path to the resolv.conf file containing addresses for local nameservers
+  resolv_conf_path: /etc/resolv.conf

 smtp:
  # The hostname to send application-level e-mails to
--- a/lib/postal/config_schema.rb
+++ b/lib/postal/config_schema.rb
@@ -329,6 +329,16 @@ module Postal
        description "The domain to use on external domains which points to the Postal return path domain"
        default "psrp"
      end
+
+      integer :timeout do
+        description "The timeout to wait for DNS resolution"
+        default 5
+      end
+
+      string :resolv_conf_path do
+        description "The path to the resolv.conf file containing addresses for local nameservers"
+        default "/etc/resolv.conf"
+      end
    end

    group :smtp do
--- a/spec/lib/dns_resolver_spec.rb
+++ b/spec/lib/dns_resolver_spec.rb
@@ -3,7 +3,7 @@
 require "rails_helper"

 RSpec.describe DNSResolver do
-  subject(:resolver) { described_class.new }
+  subject(:resolver) { described_class.local }

  # Now, we could mock everything in here which would give us some comfort
  # but I do think that we'll benefit more from having a full E2E test here
@@ -14,15 +14,52 @@ RSpec.describe DNSResolver do
    it "returns a list of IP addresses" do
      expect(resolver.a("www.test.postalserver.io").sort).to eq ["1.2.3.4", "2.3.4.5"]
    end
+
    it "resolves a domain name containing an emoji" do
      expect(resolver.a("☺.test.postalserver.io").sort).to eq ["3.4.5.6"]
    end
+
+    it "returns an empty array when timeout is exceeded" do
+      allow(Postal::Config.dns).to receive(:timeout).and_return(0.00001)
+      expect(resolver.a("www.test.postalserver.io")).to eq []
+    end
+
+    context "when raise_timeout_errors is true" do
+      it "returns a list of IP addresses" do
+        expect(resolver.a("www.test.postalserver.io", raise_timeout_errors: true).sort).to eq ["1.2.3.4", "2.3.4.5"]
+      end
+
+      it "raises an error when the timeout is exceeded" do
+        allow(Postal::Config.dns).to receive(:timeout).and_return(0.00001)
+        expect do
+          resolver.a("www.test.postalserver.io", raise_timeout_errors: true)
+        end.to raise_error(Resolv::ResolvError, /timeout/)
+      end
+    end
  end

  describe "#aaaa" do
    it "returns a list of IP addresses" do
      expect(resolver.aaaa("www.test.postalserver.io").sort).to eq ["2a00:67a0:a::1", "2a00:67a0:a::2"]
    end
+
+    it "returns an empty array when timeout is exceeded" do
+      allow(Postal::Config.dns).to receive(:timeout).and_return(0.00001)
+      expect(resolver.aaaa("www.test.postalserver.io")).to eq []
+    end
+
+    context "when raise_timeout_errors is true" do
+      it "returns a list of IP addresses" do
+        expect(resolver.aaaa("www.test.postalserver.io", raise_timeout_errors: true).sort).to eq ["2a00:67a0:a::1", "2a00:67a0:a::2"]
+      end
+
+      it "raises an error when the timeout is exceeded" do
+        allow(Postal::Config.dns).to receive(:timeout).and_return(0.00001)
+        expect do
+          resolver.aaaa("www.test.postalserver.io", raise_timeout_errors: true)
+        end.to raise_error(Resolv::ResolvError, /timeout/)
+      end
+    end
  end

  describe "#txt" do
@@ -32,12 +69,51 @@ RSpec.describe DNSResolver do
        "another example"
      ]
    end
+
+    it "returns an empty array when timeout is exceeded" do
+      allow(Postal::Config.dns).to receive(:timeout).and_return(0.00001)
+      expect(resolver.txt("test.postalserver.io")).to eq []
+    end
+
+    context "when raise_timeout_errors is true" do
+      it "returns a list of TXT records" do
+        expect(resolver.txt("test.postalserver.io", raise_timeout_errors: true).sort).to eq [
+          "an example txt record",
+          "another example"
+        ]
+      end
+
+      it "raises an error when the timeout is exceeded" do
+        allow(Postal::Config.dns).to receive(:timeout).and_return(0.00001)
+        expect do
+          resolver.txt("test.postalserver.io", raise_timeout_errors: true)
+        end.to raise_error(Resolv::ResolvError, /timeout/)
+      end
+    end
  end

  describe "#cname" do
    it "returns a list of CNAME records" do
      expect(resolver.cname("cname.test.postalserver.io")).to eq ["www.test.postalserver.io"]
    end
+
+    it "returns an empty array when timeout is exceeded" do
+      allow(Postal::Config.dns).to receive(:timeout).and_return(0.00001)
+      expect(resolver.cname("cname.test.postalserver.io")).to eq []
+    end
+
+    context "when raise_timeout_errors is true" do
+      it "returns a list of CNAME records" do
+        expect(resolver.cname("cname.test.postalserver.io", raise_timeout_errors: true)).to eq ["www.test.postalserver.io"]
+      end
+
+      it "raises an error when the timeout is exceeded" do
+        allow(Postal::Config.dns).to receive(:timeout).and_return(0.00001)
+        expect do
+          resolver.cname("cname.test.postalserver.io", raise_timeout_errors: true)
+        end.to raise_error(Resolv::ResolvError, /timeout/)
+      end
+    end
  end

  describe "#mx" do
@@ -47,6 +123,27 @@ RSpec.describe DNSResolver do
        [20, "mx2.test.postalserver.io"]
      ]
    end
+
+    it "returns an empty array when timeout is exceeded" do
+      allow(Postal::Config.dns).to receive(:timeout).and_return(0.00001)
+      expect(resolver.mx("test.postalserver.io")).to eq []
+    end
+
+    context "when raise_timeout_errors is true" do
+      it "returns a list of MX records" do
+        expect(resolver.mx("test.postalserver.io", raise_timeout_errors: true)).to eq [
+          [10, "mx1.test.postalserver.io"],
+          [20, "mx2.test.postalserver.io"]
+        ]
+      end
+
+      it "raises an error when the timeout is exceeded" do
+        allow(Postal::Config.dns).to receive(:timeout).and_return(0.00001)
+        expect do
+          resolver.mx("test.postalserver.io", raise_timeout_errors: true)
+        end.to raise_error(Resolv::ResolvError, /timeout/)
+      end
+    end
  end

  describe "#effective_ns" do
@@ -56,12 +153,51 @@ RSpec.describe DNSResolver do
        "the-cake-is-a-lie.katapultdns.com"
      ]
    end
+
+    it "returns an empty array when timeout is exceeded" do
+      allow(Postal::Config.dns).to receive(:timeout).and_return(0.00001)
+      expect(resolver.effective_ns("postalserver.io")).to eq []
+    end
+
+    context "when raise_timeout_errors is true" do
+      it "returns a list of NS records" do
+        expect(resolver.effective_ns("postalserver.io", raise_timeout_errors: true).sort).to eq [
+          "prestigious-honeybadger.katapultdns.com",
+          "the-cake-is-a-lie.katapultdns.com"
+        ]
+      end
+
+      it "raises an error when the timeout is exceeded" do
+        allow(Postal::Config.dns).to receive(:timeout).and_return(0.00001)
+        expect do
+          resolver.effective_ns("postalserver.io", raise_timeout_errors: true)
+        end.to raise_error(Resolv::ResolvError, /timeout/)
+      end
+    end
  end

  describe "#ip_to_hostname" do
    it "returns the hostname for the given IP" do
      expect(resolver.ip_to_hostname("151.252.1.100")).to eq "ns1.katapultdns.com"
    end
+
+    it "returns the IP when the timeout is exceeded" do
+      allow(Postal::Config.dns).to receive(:timeout).and_return(0.00001)
+      expect(resolver.ip_to_hostname("151.252.1.100")).to eq "151.252.1.100"
+    end
+
+    context "when raise_timeout_errors is true" do
+      it "returns the hostname for the given IP" do
+        expect(resolver.ip_to_hostname("151.252.1.100", raise_timeout_errors: true)).to eq "ns1.katapultdns.com"
+      end
+
+      it "raises an error when the timeout is exceeded" do
+        allow(Postal::Config.dns).to receive(:timeout).and_return(0.00001)
+        expect do
+          resolver.ip_to_hostname("151.252.1.100", raise_timeout_errors: true)
+        end.to raise_error(Resolv::ResolvError, /timeout/)
+      end
+    end
  end

  describe ".for_domain" do
@@ -72,9 +208,40 @@ RSpec.describe DNSResolver do
  end

  describe ".local" do
-    it "returns a resolver with no nameservers" do
+    after do
+      # Remove all cached values for the local resolver
+      DNSResolver.instance_variable_set(:@local, nil)
+    end
+
+    it "returns a resolver with the local machine's resolvers" do
      resolver = described_class.local
-      expect(resolver.nameservers).to be nil
+      expect(resolver.nameservers).to be_a Array
+      expect(resolver.nameservers).to_not be_empty
+    end
+
+    context "when there is no resolv.conf" do
+      it "raises an error" do
+        allow(File).to receive(:file?).with("/etc/resolv.conf").and_return(false)
+        expect { described_class.local }.to raise_error(DNSResolver::LocalResolversUnavailableError,
+                                                        /no resolver config found at/i)
+      end
+    end
+
+    context "when no nameservers are found in resolv.conf" do
+      it "raises an error" do
+        allow(Resolv::DNS::Config).to receive(:parse_resolv_conf).with("/etc/resolv.conf").and_return({})
+        expect { described_class.local }.to raise_error(DNSResolver::LocalResolversUnavailableError,
+                                                        /could not find nameservers in/i)
+      end
+    end
+
+    context "when an empty array of nameserver is found in resolv.conf" do
+      it "raises an error" do
+        allow(Resolv::DNS::Config).to receive(:parse_resolv_conf).with("/etc/resolv.conf")
+                                                                 .and_return({ nameserver: [] })
+        expect { described_class.local }.to raise_error(DNSResolver::LocalResolversUnavailableError,
+                                                        /could not find nameservers in/i)
+      end
    end
  end

--- a/spec/models/domain_spec.rb
+++ b/spec/models/domain_spec.rb
@@ -301,7 +301,7 @@ describe Domain do

    context "when local nameservers should not be used" do
      it "uses the a resolver for this domain" do
-        allow(DNSResolver).to receive(:for_domain).with(domain.name).and_return(DNSResolver.new(nameservers: ["1.2.3.4"]))
+        allow(DNSResolver).to receive(:for_domain).with(domain.name).and_return(DNSResolver.new(["1.2.3.4"]))
        expect(domain.resolver).to be_a DNSResolver
        expect(domain.resolver.nameservers).to eq ["1.2.3.4"]
      end