Hi,
Find attached the first stab at a final patch making generated bounce
mails follow more closely the RFCs 3461, 3464 and 6522. The attached
file includes the patch from my previous mail in this thread.
The patch is based on opensmtp 7.5.0rc1 (w/ the additional patch in the
rc1 announcement thread on this ml).
It does, in short:
- uses the correct content type(s) for DSNs, as specified in RFC3464 and
RFC6522
- adds xtext_decode() utility func (following the style/interface of the
base64_decode() utility func)
- extends the data structures of the bounce message and envelope, as
necesary, to write the required fields outlined by RFC3461 and RFC3464
into the machine readable sub mime part, namely, per-message field:
* Original-Envelope-Id: (required if ENVID= was used, see RFC3461)
and per recipiend fields:
* Original-Recipient: (required if ORCPT= was used, see RFC3461)
* Arrival-Date: (optional, but added it while at it...)
* Diagnostic-Code: (optional, but added it while at it...)
- adds a check whether the SMTP ENVID= param is valid xtext (RFC3461)
- minor: removes a space in already set Final-Recipient: field
(following more closely the example layout from RFC3464)
One thing I wasn't sure about what to do is *where* to handle the xtext
decoding of the ENVID= and ORCPT= SMTP params. Right now the SMTP
handling part just considers them "opaque text", as they are just handed
down the relays, unmodified. When copying those into a DSN, they must be
decoded, though, and the decoded result is considered being printable
US-ASCII. From RFC3461 4.2 about ORCPT:
`Due to limitations in the Delivery Status Notification format, the
value of the original recipient address prior to encoding as "xtext"
MUST consist entirely of printable (graphic and white space)
characters from the US-ASCII repertoire.`
A functional identical wording is used in 4.4 for the ENVID= param.
The attached patch keeps the SMTP behaviour to just treat those params
as opaque strings, but checks if the decoded part is US-ASCII when
generating a bounce - and if it's not, doesn't write it to the
respective DSN field(s).
I think the SMTP handling might be the better place to check for that,
and refuse such SMTP params outright, maybe?
Thanks
On Tue, Mar 19, 2024 at 04:22:13PM +0100, Tassilo Philipp wrote:
Alright, find attached a first patch, fixing up some content-type
headers, as outlined by RFC3464 and RFC6522 - in detail:
The patch's first hunk follows RFC3464, which specifies that a DSN
should have a top-level type of "multipart/report" with a parameter
"report-type=delivery-status"; the actual format is described in
RFC6522, and is specified having two or three sub mime parts, a human
readable message, a machine parsable part with content-type
"message/delivery-status" and an optional 3rd part of either
"text/rfc822-headers" or "message/rfc822".
The latter is part of the second hunk of the patch, b/c
"text/rfc822-headers" was used in any case, even if the full message
was returned in the DSN.
Also, the end of the second hunk removes a check, which I think is
unintuitive to begin with, but unsure. Basically, it only returned the
headers for NOTIFY=SUCCESS DSNs when RET=HDRS was also set (explicitly
or implicitly).
Here's the section from RFC3461:
When the value of the RET parameter is FULL, the full
message SHOULD be returned for any DSN which conveys notification of
delivery failure. (However, if the length of the message is greater
than some implementation-specified length, the MTA MAY return only
the headers even if the RET parameter specified FULL.) If a DSN
contains no notifications of delivery failure, the MTA SHOULD return
only the headers.
The original code does what the last line implies, but only when
RET=HDRS is set, meaning it ignores it for anything but
NOTIFY=SUCCESS, and returns always the full message.
I'm not sure what would be best to do here, all in all the RFC says
'SHOULD'... either way, the check there should reflect the same logic
used in the if...else... block of the second hunk, which is what the
patch shoots for. I chose to keep it simple and simply do what RET=
asks for. So, if the original logic is kept, the second hunk's
if...else... block would need an additional "s->msg->bounce.type ==
B_DELIVERED" check.
I tested the patch with different combinations of the RET= and NOTIFY=
parameters, and so far it seems to work fine.
PS: I'm working on related other patches, at the moment, which I'll
mail when they are done, namely adding a "Original-Envelope-ID" header
in the DSN when the ENVID= param was present, as well as
"Original-Recipient" for any ORCPT= param. Those are specified in
RFC3461 section 6.3., but they are a bit more involved as the params
need to be decoded and recoded, and I just haven't found the time,
yet.
Thanks!
On Wed, Mar 13, 2024 at 12:04:24PM +0100, Tassilo Philipp wrote:
Ok, will get busy... I also noticed two other issues, one related to
ENVID= (should be generated for DSNs), and some data in the DSN's
message/delivery-status parts, that must be generated according to
eh RFC, when ENVID= or ORCPT= are present, which are not, currently.
Will look at those, as well. Might take a few days, though... will
keep you posted.
Cheers
On Wed, Mar 13, 2024 at 11:00:51AM +0000, gil...@poolp.org wrote:
March 13, 2024 10:31 AM, "Tassilo Philipp" <tphil...@potion-studios.com> wrote:
Hello,
I noticed that DSNs generated by OpenSMTPd use "Content-Type:
multipart/mixed", instead of "Content-Type: multipart/report",
as defined by RFC3461 (and described in RFC3464 and RFC3462). I
wonder if there's a reason for that?
I haven only done a cursory review of the actual multiparts
themselves, but they seem to (mostly) follow the mentioned RFCs
with for example "Content-Type: text/rfc822-headers". However,
if RET=FULL, the content type should be IMHO only
"message/rfc822" and not "text/rfc822-headers", the latter seems
to be currently used for both, RET=HDRS and RET=FULL. Need to
read up on it more to get a clearer picture, though, I just
started digging into it.
Either way... I think the RFCs should be followed, want me to prepare a patch?
Yes please, patch and RFC reference so this can be checked too
Thanks,
--- ./usr.sbin/smtpd/bounce.c
+++ ./usr.sbin/smtpd/bounce.c
@@ -452,7 +452,8 @@
"To: %s\r\n"
"Date: %s\r\n"
"MIME-Version: 1.0\r\n"
- "Content-Type: multipart/mixed;"
+ "Content-Type: multipart/report;"
+ "report-type=delivery-status;"
"boundary=\"%16" PRIu64 "/%s\"\r\n"
"\r\n"
"This is a MIME-encapsulated message.\r\n"
@@ -534,19 +535,27 @@
break;
case BOUNCE_DATA_MESSAGE:
- io_xprintf(s->io,
- "--%16" PRIu64 "/%s\r\n"
- "Content-Description: Message headers\r\n"
- "Content-Type: text/rfc822-headers\r\n"
- "\r\n",
- s->boundary, s->smtpname);
+ if(s->msg->bounce.dsn_ret == DSN_RETHDRS) {
+ io_xprintf(s->io,
+ "--%16" PRIu64 "/%s\r\n"
+ "Content-Description: Message headers\r\n"
+ "Content-Type: text/rfc822-headers\r\n"
+ "\r\n",
+ s->boundary, s->smtpname);
+ } else {
+ io_xprintf(s->io,
+ "--%16" PRIu64 "/%s\r\n"
+ "Content-Description: Full message\r\n"
+ "Content-Type: message/rfc822\r\n"
+ "\r\n",
+ s->boundary, s->smtpname);
+ }
n = io_queued(s->io);
while (io_queued(s->io) < BOUNCE_HIWAT) {
if ((len = getline(&line, &sz, s->msgfp)) == -1)
break;
if (len == 1 && line[0] == '\n' && /* end of headers */
- s->msg->bounce.type == B_DELIVERED &&
s->msg->bounce.dsn_ret == DSN_RETHDRS) {
free(line);
fclose(s->msgfp);
--- ./usr.sbin/smtpd/bounce.c
+++ ./usr.sbin/smtpd/bounce.c
@@ -57,6 +57,9 @@
char *report;
uint8_t esc_class;
uint8_t esc_code;
+ char *errorline;
+ /* decoded xtext SMTP ORCPT param, len always <= encoded */
+ char orcpt[DSN_ORCPT_LEN+1];
};
struct bounce_message {
@@ -68,6 +71,9 @@
char *to;
time_t timeout;
TAILQ_HEAD(, bounce_envelope) envelopes;
+ time_t creation;
+ /* decoded xtext SMTP ENVID param, len always <= encoded */
+ char envid[DSN_ENVID_LEN+1];
};
struct bounce_session {
@@ -120,6 +126,18 @@
}
}
+static int
+count_printable_ascii_chars(const char *s)
+{
+ const char *p = s;
+ for (; *p; ++p) {
+ if (!isprint((unsigned char)*p))
+ break;
+ }
+ return p - s;
+}
+
+
void
bounce_add(uint64_t evpid)
{
@@ -127,6 +145,7 @@
struct envelope evp;
struct bounce_message key, *msg;
struct bounce_envelope *be;
+ int n;
bounce_init();
@@ -164,6 +183,14 @@
msg->msgid = key.msgid;
msg->bounce = key.bounce;
+ msg->creation = evp.creation;
+ n = xtext_decode(evp.dsn_envid, msg->envid, sizeof(msg->envid));
+ if (n == -1 || n != count_printable_ascii_chars(msg->envid)) {
+ msg->envid[0] = '\0';
+ log_debug("debug: bounce: ill-formed ENVID param
(decoded xtext not "
+ "printable ascii, not setting DSN
Original-Envelope-Id: header");
+ }
+
TAILQ_INIT(&msg->envelopes);
msg->smtpname = xstrdup(evp.smtpname);
@@ -187,11 +214,18 @@
be = xmalloc(sizeof *be);
be->id = evpid;
be->report = xstrdup(buf);
+ be->errorline = xstrdup(evp.errorline);
(void)strlcpy(be->dest.user, evp.dest.user, sizeof(be->dest.user));
(void)strlcpy(be->dest.domain, evp.dest.domain,
sizeof(be->dest.domain));
be->esc_class = evp.esc_class;
be->esc_code = evp.esc_code;
+ n = xtext_decode(evp.dsn_orcpt, be->orcpt, sizeof(be->orcpt));
+ if (n == -1 || n != count_printable_ascii_chars(be->orcpt)) {
+ be->orcpt[0] = '\0';
+ log_debug("debug: bounce: ill-formed ORCPT param (decoded xtext
not "
+ "printable ascii, not setting DSN Original-Recipient:
header");
+ }
TAILQ_INSERT_TAIL(&msg->envelopes, be, entry);
log_debug("debug: bounce: adding report %16"PRIx64": %s", be->id,
be->report);
@@ -404,6 +438,7 @@
return (1);
}
+
static int
bounce_next(struct bounce_session *s)
{
@@ -452,7 +487,8 @@
"To: %s\r\n"
"Date: %s\r\n"
"MIME-Version: 1.0\r\n"
- "Content-Type: multipart/mixed;"
+ "Content-Type: multipart/report;"
+ "report-type=delivery-status;"
"boundary=\"%16" PRIu64 "/%s\"\r\n"
"\r\n"
"This is a MIME-encapsulated message.\r\n"
@@ -510,21 +546,35 @@
"\r\n",
s->boundary, s->smtpname);
+ if(strlen(s->msg->envid) > 0)
+ io_xprintf(s->io,
+ "Original-Envelope-Id: %s\r\n",
+ s->msg->envid);
+
io_xprintf(s->io,
"Reporting-MTA: dns; %s\r\n"
+ "Arrival-Date: %s\r\n"
"\r\n",
- s->smtpname);
+ s->smtpname,
+ time_to_text(s->msg->creation));
TAILQ_FOREACH(evp, &s->msg->envelopes, entry) {
+ if(strlen(evp->orcpt) > 0)
+ io_xprintf(s->io,
+ "Original-Recipient: %s\r\n",
+ evp->orcpt);
+
io_xprintf(s->io,
- "Final-Recipient: rfc822; %s@%s\r\n"
+ "Final-Recipient: rfc822;%s@%s\r\n"
"Action: %s\r\n"
"Status: %s\r\n"
+ "Diagnostic-Code: %s\r\n"
"\r\n",
evp->dest.user,
evp->dest.domain,
action_str(&s->msg->bounce),
- esc_code(evp->esc_class, evp->esc_code));
+ esc_code(evp->esc_class, evp->esc_code),
+ evp->errorline);
}
log_trace(TRACE_BOUNCE, "bounce: %p: >>> [... %zu bytes ...]",
@@ -534,12 +584,21 @@
break;
case BOUNCE_DATA_MESSAGE:
- io_xprintf(s->io,
- "--%16" PRIu64 "/%s\r\n"
- "Content-Description: Message headers\r\n"
- "Content-Type: text/rfc822-headers\r\n"
- "\r\n",
- s->boundary, s->smtpname);
+ if(s->msg->bounce.type != B_FAILED || s->msg->bounce.dsn_ret !=
DSN_RETFULL) {
+ io_xprintf(s->io,
+ "--%16" PRIu64 "/%s\r\n"
+ "Content-Description: Message headers\r\n"
+ "Content-Type: text/rfc822-headers\r\n"
+ "\r\n",
+ s->boundary, s->smtpname);
+ } else {
+ io_xprintf(s->io,
+ "--%16" PRIu64 "/%s\r\n"
+ "Content-Description: Full message\r\n"
+ "Content-Type: message/rfc822\r\n"
+ "\r\n",
+ s->boundary, s->smtpname);
+ }
n = io_queued(s->io);
while (io_queued(s->io) < BOUNCE_HIWAT) {
@@ -629,6 +688,7 @@
}
TAILQ_REMOVE(&msg->envelopes, be, entry);
free(be->report);
+ free(be->errorline);
free(be);
n += 1;
}
--- ./usr.sbin/smtpd/smtpd.h
+++ ./usr.sbin/smtpd/smtpd.h
@@ -1757,6 +1757,7 @@
int base64_decode(char const *, unsigned char *, size_t);
int base64_encode_rfc3548(unsigned char const *, size_t,
char *, size_t);
+int xtext_decode(char const *, unsigned char *, size_t);
void xclosefrom(int);
void log_trace_verbose(int);
--- ./usr.sbin/smtpd/util.c
+++ ./usr.sbin/smtpd/util.c
@@ -838,6 +838,41 @@
return ret;
}
+/* guarantees output is null terminated, returns num data bytes (without
+ * trailing null) stored at dest, or -1 on error */
+int
+xtext_decode(char const *src, unsigned char *dest, size_t destsize)
+{
+ char s[3];
+ size_t i = 0;
+
+ s[2] = '\0';
+
+ if(!valid_xtext(src))
+ return 0;
+
+ while (*src != '\0') {
+ if (i >= destsize)
+ return (-1);
+
+ if (*src == '+') {
+ s[0] = (unsigned char)*++src;
+ s[1] = (unsigned char)*++src;
+ dest[i++] = (unsigned char)strtoul(s, NULL, 16);
+ }
+ else
+ dest[i++] = *src;
+
+ ++src;
+ }
+
+ if (i >= destsize)
+ return (-1);
+
+ dest[i] = '\0';
+ return (i);
+}
+
void
log_trace0(const char *emsg, ...)
{
--- ./usr.sbin/smtpd/smtp_session.c
+++ ./usr.sbin/smtpd/smtp_session.c
@@ -2395,6 +2395,12 @@
tx->evp.dsn_ret = DSN_RETFULL;
} else if (ADVERTISE_EXT_DSN(tx->session) && strncasecmp(opt,
"ENVID=", 6) == 0) {
opt += 6;
+ if (!valid_xtext(opt)) {
+ smtp_reply(tx->session,
+ "501 ENVID parameter syntax error");
+ smtp_tx_free(tx);
+ return;
+ }
if (strlcpy(tx->evp.dsn_envid, opt,
sizeof(tx->evp.dsn_envid))
>= sizeof(tx->evp.dsn_envid)) {
smtp_reply(tx->session,
