Subject: [PATCH] pgarchives: parser: handle messages in which Message-ID is missing

From: Célestin Matte <[email-masked]>
Date: Wed, 3 Nov 2021 18:02:42 +0100

Hello,

As surprising as it may seem, Message-ID is actually not a mandatory email field [1]. While most MTAs do add this field, some might not, and this will cause load_message.py to crash.
As a solution to this, when this field is missing, this patch:
- attempts to find a "Sent-Message-ID" header and use it as the Message-ID (a case I encountered when trying to import an old mbox)
- generates a new Message-ID if none exists, following (a simpler version of) [2].

[1] https://www.rfc-editor.org/rfc/rfc2822#section-3.6.4
[2] https://datatracker.ietf.org/doc/html/draft-ietf-usefor-message-id-00#section-3

Cheers,
-- 
Célestin Matte

From 36e6a6b67c7f64f524770a852587f8db072604a4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?C=C3=A9lestin=20Matte?= <[email-masked]>
Date: Wed, 3 Nov 2021 17:09:01 +0100
Subject: [PATCH] parser: handle messages in which Message-ID is missing

Message-ID is not mandatory in emails. When such a message is imported,
attempt to use Resent-Message-ID instead if it exists, or generate a new
one.
---
 loader/lib/parser.py | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/loader/lib/parser.py b/loader/lib/parser.py
index 171f197..21e1e48 100644
--- a/loader/lib/parser.py
+++ b/loader/lib/parser.py
@@ -1,6 +1,7 @@
 import re
 import datetime
 import dateutil.parser
+import random
 
 from email.parser import BytesParser
 from email.header import decode_header, Header
@@ -28,13 +29,13 @@ class ArchivesParser(object):
         # Look for a specific messageid. This means we might parse it twice,
         # but so be it. Any exception means we know it's not this one...
         try:
-            if self.clean_messageid(self.decode_mime_header(self.get_mandatory('Message-ID'))) == msgid:
+            if self.clean_messageid(self.decode_mime_header(self.get_or_generate_messageid())) == msgid:
                 return True
         except Exception:
             return False
 
     def analyze(self, date_override=None):
-        self.msgid = self.clean_messageid(self.decode_mime_header(self.get_mandatory('Message-ID')))
+        self.msgid = self.clean_messageid(self.decode_mime_header(self.get_or_generate_messageid()))
         self._from = self.decode_mime_header(self.get_mandatory('From'), True)
         self.to = self.decode_mime_header(self.get_optional('To'), True)
         self.cc = self.decode_mime_header(self.get_optional('CC'), True)
@@ -547,6 +548,25 @@ class ArchivesParser(object):
         except ValueError as ve:
             raise IgnorableException("Failed to decode header value '%s': %s" % (hdr, ve))
 
+    def get_or_generate_messageid(self):
+        x = self.msg["Message-ID"]
+        if x is None:
+            # If Message-ID is message, try using Resent-Message-ID instead
+            x = self.msg["Resent-Message-ID"]
+        if x is None:
+            # If Resent-Message-ID is missing too, forge a new Message-ID
+            # following a simpler version of
+            # https://datatracker.ietf.org/doc/html/draft-ietf-usefor-message-id-00#section-3
+            date_part = re.sub('[^A-Z0-9]', '', str(self.forgiving_date_decode(self.decode_mime_header(self.get_mandatory('Date')))))
+            random_part = random.getrandbits(64)
+            from_fqdn = self.decode_mime_header(self.get_mandatory('From'), True).split('@')
+            if len(from_fqdn) > 1:
+                fqdn = from_fqdn[1]
+            else:
+                fqdn = ""
+            x = "<" + str(date_part) + "." + str(random_part) + "@" + fqdn + ">"
+        return x
+
     def get_mandatory(self, fieldname):
         try:
             x = self.msg[fieldname]
-- 
2.33.1

---

From: Célestin Matte <[email-masked]>
Date: Wed, 3 Nov 2021 18:05:44 +0100

By the way, loader/load_message.py has a double crash issue, when importing a message raising an IgnorableException:

[...]
During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/srv/pgarchives/local//loader/load_message.py", line 155, in <module>
    log_failed_message(listid, "mbox", opt.mbox, ap, e)
  File "/srv/pgarchives/local//loader/load_message.py", line 36, in log_failed_message
    'err': str(str(err), 'us-ascii', 'replace'),
TypeError: decoding str is not supported

I don't understand what this line is supposed to do (removing non-ascii characters?), but a simple str(err) fixes the issue.

Cheers,
-- 
Célestin Matte

---

From: Magnus Hagander <[email-masked]>
Date: Thu, 4 Nov 2021 11:31:33 +0100

On Wed, Nov 3, 2021 at 6:02 PM Célestin Matte <[email-masked]>
wrote:

> Hello,
>
> As surprising as it may seem, Message-ID is actually not a mandatory email
> field [1]. While most MTAs do add this field, some might not, and this will
> cause load_message.py to crash.
> - attempts to find a "Sent-Message-ID" header and use it as the Message-ID
> (a case I encountered when trying to import an old mbox)

I don't think this should be the responsibility of pglister. As you say,
"most MTAs do add this field" -- and the solution is to configure the MTA
to do this. We already rely on the MTA to get a lot of other important
things right.

It may be something that should get documented somewhere as a requirement.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ <http://www.hagander.net/>
 Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>

---

From: Magnus Hagander <[email-masked]>
Date: Thu, 4 Nov 2021 11:32:37 +0100

On Thu, Nov 4, 2021 at 11:31 AM Magnus Hagander <[email-masked]> wrote:

>> As surprising as it may seem, Message-ID is actually not a mandatory
>> email field [1]. While most MTAs do add this field, some might not, and
>> this will cause load_message.py to crash.
>> - attempts to find a "Sent-Message-ID" header and use it as the
>> Message-ID (a case I encountered when trying to import an old mbox)

Sorry, I mean pgarchives here of course, not pglister :)

---

From: Magnus Hagander <[email-masked]>
Date: Thu, 4 Nov 2021 11:35:41 +0100

On Wed, Nov 3, 2021 at 6:05 PM Célestin Matte <[email-masked]>
wrote:

> By the way, loader/load_message.py has a double crash issue, when
> importing a message raising an IgnorableException:
>
> [...]
> I don't understand what this line is supposed to do (removing non-ascii
> characters?), but a simple str(err) fixes the issue.
>

It's supposed to remove non-ascii characters.

I think this is a leftover from the py2->py3 conversion. It looks like an
overenthusiastic regexp replacement in the 2to3 tool. See bb5775ef where it
came from. I'll go change it to jut str(err).

---

From: Célestin Matte <[email-masked]>
Date: Thu, 4 Nov 2021 16:34:35 +0100

> I don't think this should be the responsibility of pglister. As you say, "most MTAs do add this field" -- and the solution is to configure the MTA to do this. We already rely on the MTA to get a lot of other important things right.

But then these messages will get delivered by pglister but pgarchives will fail to archive them, although they do not actually break requirements. Shouldn't we follow the RFC here?

> It may be something that should get documented somewhere as a requirement. 

I'll write a small readme for pgarchives

-- 
Célestin Matte

---

From: Célestin Matte <[email-masked]>
Date: Thu, 4 Nov 2021 16:46:29 +0100

> I think this is a leftover from the py2->py3 conversion. It looks like an overenthusiastic regexp replacement in the 2to3 tool. See bb5775ef where it came from. I'll go change it to jut str(err).

Right. Beware that there are other similar failing conversions in the same commit on lines in loader/lib/parser.py that should be addressed as well

-- 
Célestin Matte

---

From: Alvaro Herrera <[email-masked]>
Date: Thu, 4 Nov 2021 16:47:28 -0300

On 2021-Nov-04, Célestin Matte wrote:

> > I don't think this should be the responsibility of pglister. As you
> > say, "most MTAs do add this field" -- and the solution is to
> > configure the MTA to do this. We already rely on the MTA to get a
> > lot of other important things right.
> 
> But then these messages will get delivered by pglister but pgarchives
> will fail to archive them, although they do not actually break
> requirements. Shouldn't we follow the RFC here?

Maybe pglister should refuse to deliver messages that don't contain
a Message-Id.

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/
"Ninguna manada de bestias tiene una voz tan horrible como la humana" (Orual)

---

From: Christophe Pettus <[email-masked]>
Date: Thu, 4 Nov 2021 12:56:06 -0700

> On Nov 4, 2021, at 12:47, Alvaro Herrera <[email-masked]> wrote:

That seems reasonable.  If there's no message ID by the time pglister gets it, something is pretty broken along the path from the sender to us.

---

From: Tom Lane <[email-masked]>
Date: Thu, 04 Nov 2021 16:28:34 -0400

> That seems reasonable.  If there's no message ID by the time pglister gets it, something is pretty broken along the path from the sender to us.

FWIW, I've long used a spam filtering rule that sends anything without
a message ID, or with a forged local message ID, straight to /dev/null.
It's quite effective, and I've not seen it eat any valid traffic.

			regards, tom lane

---

From: Magnus Hagander <[email-masked]>
Date: Thu, 4 Nov 2021 22:07:14 +0100

> > requirements. Shouldn't we follow the RFC here?

I agree that the scenario is a problem, per below.  I don't agree that
making up an id is a solution to that problem.

> a Message-Id.

It should. I actually thought it did already, but apparently it does not. I
guess we've only ever used it under properly configured MTAs :)

Have you actually come across any case where a *proper* non-spam message is
sent without a message-id and passes through actual mailservers on the way?

Looking through the approximately 1.4 million mails in the postgres list
archives, not a single one has a message-id generated by the archives
server MTA (which is configured to generate it). Not a single one by our
inbound relay servers. And exactly one by the pglister server -- which
turns out to be a bounce that ended up in the archives because of a
misconfiguration back in 2018 that's not visible in the public archives.

---

From: Célestin Matte <[email-masked]>
Date: Fri, 5 Nov 2021 12:31:23 +0100

> Have you actually come across any case where a *proper* non-spam message is sent without a message-id and passes through actual mailservers on the way? 
> 
> Looking through the approximately 1.4 million mails in the postgres list archives, not a single one has a message-id generated by the archives server MTA (which is configured to generate it). Not a single one by our inbound relay servers. And exactly one by the pglister server -- which turns out to be a bounce that ended up in the archives because of a misconfiguration back in 2018 that's not visible in the public archives.

After some tests, I do have a very few number of non-spam examples (3 emails from 2 different people in a postfix+mailman mbox of 5k emails), but they date from 2003-2007.

Exim already handles empty Message-IDs by default by generating them [1], although it will let a message with Resent-Message-ID pass through as-if. I tested such a case, and pglister seems to actually drop the message (or fail silently).

[1]: https://www.exim.org/exim-html-current/doc/html/spec_html/ch-message_processing.html#SECID226
-- 
Célestin Matte

---

From: Justin Clift <[email-masked]>
Date: Sat, 06 Nov 2021 00:00:20 +1100

On 2021-11-05 22:31, Célestin Matte wrote:
>> Have you actually come across any case where a *proper* non-spam 
>> message is sent without a message-id and passes through actual 
>> mailservers on the way? 

As a data point, I'm pretty sure it was the message-id field
being missing from emails sent by scan.co.uk (a UK computer
retailer) a while back, which caused their non-delivery to
the mail servers here (@postgresql.org).

So, being that we bounce them, it wouldn't at all surprise me
that there's not much presence in our archives. :)

+ Justin

---

From: Stefan Kaltenbrunner <[email-masked]>
Date: Fri, 5 Nov 2021 21:44:33 +0100

On 11/4/21 10:07 PM, Magnus Hagander wrote:
> 
> 
> On Thu, Nov 4, 2021 at 8:47 PM Alvaro Herrera <[email-masked] 
> <mailto:[email-masked]>> wrote:
> 
> It should. I actually thought it did already, but apparently it does 
> not. I guess we've only ever used it under properly configured MTAs :)
> 
> Have you actually come across any case where a *proper* non-spam message 
> is sent without a message-id and passes through actual mailservers on 
> the way?
> 

as mentioned down-thread by Justin Clift we have been plain rejecting 
mails without a message-id on the postgresql.org inbound relays since 
March 27th 2012(!) according to our repo and the number of rejects due 
to that rule is actually not-insignificant (approximately 200-400/day 
with the majority being for a very small number of bounce generating 
senders) but the number of complaints is also approaching (almost) zero.

So the reason why pglister is not seeing them a lot is because we dont 
accept them upstream, not that they dont exist in the wild...

Though the ones in the wild seem to be "not very useful"...

Stefan

Stefan

---

From: Magnus Hagander <[email-masked]>
Date: Sat, 6 Nov 2021 18:02:09 +0100

> senders) but the number of complaints is also approaching (almost) zero.

Oh I forgot about that one. That clearly explains why I didn't find
anything *after* 2012 in the archives -- but in my defence, we don't have
any from before that either :)

I thought we actually manufactured a message-id on them rather than reject
them, but I guess I was confusing it with how we handle email injected
internally where I think we do that.