FEATURE: Allow sender domains to be blacklisted

FEATURE: Allow sender domains to be blacklisted

Under some circumstances, this feature should help prevent messages from
cluttering the Discourse mail logs for admins.

Implement this functionality in FastRejection, not Postfix itself, as
that is probably the first place someone is going to look.

From 589222a6b10779746552360f3ae51645c69de495 Mon Sep 17 00:00:00 2001
From: Saj Goonatilleke <saj@discourse.org>
Date: Tue, 27 Nov 2018 18:03:47 +1100
Subject: [PATCH] FEATURE: Allow sender domains to be blacklisted

Under some circumstances, this feature should help prevent messages from
cluttering the Discourse mail logs for admins.

Implement this functionality in `FastRejection`, not Postfix itself, as
that is probably the first place someone is going to look.

diff --git a/README.md b/README.md
index 1d51c36..ff36a57 100644
--- a/README.md
+++ b/README.md
@@ -43,6 +43,13 @@ For example, if you wanted to add a pre-delivery milter, you might use:
     -e POSTCONF_smtpd_milters=192.0.2.42:12345
 
 
+## Blacklisting sender domains
+
+The `BLACKLISTED_SENDER_DOMAINS` environment variable accepts a
+space-separated list of domain names.  Mail messages from these senders will
+be fast-failed with SMTP code 554.
+
+
 ## Syslog integration
 
 Postfix loves to log everything to syslog.  In fact, that's really all it
diff --git a/lib/mail_receiver/fast_rejection.rb b/lib/mail_receiver/fast_rejection.rb
index 7e19626..9b419d6 100644
--- a/lib/mail_receiver/fast_rejection.rb
+++ b/lib/mail_receiver/fast_rejection.rb
@@ -4,6 +4,7 @@ require 'uri'
 require 'cgi'
 require 'net/http'
 
+require_relative 'mail'
 require_relative 'mail_receiver_base'
 
 class FastRejection < MailReceiverBase
@@ -12,6 +13,10 @@ class FastRejection < MailReceiverBase
 		super(env_file)
 
 		@disabled = @env['DISCOURSE_FAST_REJECTION_DISABLED'] || !@env['DISCOURSE_BASE_URL']
+
+		@blacklisted_sender_domains = Hash[
+			@env.fetch('BLACKLISTED_SENDER_DOMAINS', "").split(" ").map { |v| [v.downcase, nil] }
+		]
 	end
 
 	def disabled?
@@ -50,6 +55,16 @@ class FastRejection < MailReceiverBase
 			return 'defer_if_permit No recipient specified'
 		end
 
+		domain = domain_from_addrspec(args['sender'])
+		if domain.empty?
+			logger.info("deferred mail with domainless sender #{args['sender']}")
+			return 'defer_if_permit Invalid sender'
+		end
+		if @blacklisted_sender_domains.key? domain
+			logger.info("rejected mail from blacklisted sender domain #{domain} (from #{args['sender']})")
+			return 'reject Invalid sender'
+		end
+
 		maybe_reject_email(args['sender'], args['recipient'])
 	end
 
diff --git a/lib/mail_receiver/mail.rb b/lib/mail_receiver/mail.rb
new file mode 100644
index 0000000..c8e9cb7
--- /dev/null
+++ b/lib/mail_receiver/mail.rb
@@ -0,0 +1,10 @@
+# Parse and return the domain from an Internet addr-spec.  Return value is
+# normalised to lowercase.
+#
+# This implementation is the simplest thing that could possibly work.  Do
+# not use this function to parse generalised mailbox or group addresses.
+#
+# See section 3.4 of RFC 2822.
+def domain_from_addrspec(addrspec)
+	(addrspec.split("@", 2)[1] or "").downcase
+end
diff --git a/spec/fixtures/standard.json b/spec/fixtures/standard.json
index 08378b1..5f7490a 100644
--- a/spec/fixtures/standard.json
+++ b/spec/fixtures/standard.json
@@ -1,5 +1,6 @@
 {
 	"DISCOURSE_API_KEY": "EXAMPLE_KEY",
 	"DISCOURSE_API_USERNAME": "eviltrout",
-	"DISCOURSE_BASE_URL": "https://localhost:8080"
+	"DISCOURSE_BASE_URL": "https://localhost:8080",
+	"BLACKLISTED_SENDER_DOMAINS": "bad.com SAD.NET"
 }
diff --git a/spec/lib/fast_reject_spec.rb b/spec/lib/fast_reject_spec.rb
index 634d9b4..2846d8b 100644
--- a/spec/lib/fast_reject_spec.rb
+++ b/spec/lib/fast_reject_spec.rb
@@ -55,6 +55,36 @@ describe FastRejection do
 			expect(response).to start_with('defer_if_permit')
 		end
 
+		it "returns defer_if_permit if sender addr-spec contains no domain" do
+			response = receiver.process_single_request(
+				'request' => 'smtpd_access_policy',
+				'protocol_state' => 'RCPT',
+				'sender' => 'miscreant',
+				'recipient' => 'discourse@example.com'
+			)
+			expect(response).to start_with('defer_if_permit')
+		end
+
+		it "returns reject if sender domain is blacklisted" do
+			response = receiver.process_single_request(
+				'request' => 'smtpd_access_policy',
+				'protocol_state' => 'RCPT',
+				'sender' => 'miscreant@bad.com',
+				'recipient' => 'discourse@example.com'
+			)
+			expect(response).to start_with('reject')
+		end
+
+		it "returns reject if sender domain is blacklisted with differing case" do
+			response = receiver.process_single_request(
+				'request' => 'smtpd_access_policy',
+				'protocol_state' => 'RCPT',
+				'sender' => 'miscreant@SaD.NeT',
+				'recipient' => 'discourse@example.com'
+			)
+			expect(response).to start_with('reject')
+		end
+
 		it "returns dunno if everything looks good" do
 			expect_any_instance_of(Net::HTTP).to receive(:request) do |http|
 				response = Net::HTTPSuccess.new(http, 200, "OK")
diff --git a/spec/lib/mail_spec.rb b/spec/lib/mail_spec.rb
new file mode 100644
index 0000000..92cdd58
--- /dev/null
+++ b/spec/lib/mail_spec.rb
@@ -0,0 +1,13 @@
+require_relative '../../lib/mail_receiver/mail'
+
+RSpec.describe 'domain_from_addrspec' do
+
+	it "normalises domains to lowercase" do
+		expect(domain_from_addrspec("local-part@DOMAIN.NET")).to eq "domain.net"
+	end
+
+	it "returns an empty string if a domain was not found" do
+		expect(domain_from_addrspec("local-part")).to be_empty
+	end
+
+end

GitHub

1 Like

Why not use a Set here instead?

@blacklisted_sender_domains = @env.fetch('BLACKLISTED_SENDER_DOMAINS', "").split(" ").map(&:downcase).to_set

# and use like this

if @blacklisted_sender_domains.include?(domain)

We try not to use and/or and instead only use &&/||.

Thanks for the feedback. I come from a Python/Go background so your comments were very helpful.

I was under the mistaken impression that Ruby did not have a standard set type. (Looking at my browser history, it’s possible that I was reading the docs for core, saw Array and Hash, but did not see a Set.)

TIL about the ampersand operator.

1 Like