From: Christian Schulte Subject: Re: Adding Message-ID to mail(1) (diff UPDATED) To: tech@openbsd.org Date: Sat, 24 Aug 2024 08:41:30 +0200 On 23.08.24 10:34, Walter Alejandro Iglesias wrote: ... [snip] ... > > Index: def.h > =================================================================== > RCS file: /cvs/src/usr.bin/mail/def.h,v > diff -u -p -r1.17 def.h > --- def.h 28 Jan 2022 06:18:41 -0000 1.17 > +++ def.h 22 Aug 2024 11:02:15 -0000 > @@ -55,6 +55,9 @@ > #include > #include > #include "pathnames.h" > +#include > +#include > +#include > > #define APPEND /* New mail goes to end of mailbox */ > > Index: extern.h > =================================================================== > RCS file: /cvs/src/usr.bin/mail/extern.h,v > diff -u -p -r1.30 extern.h > --- extern.h 21 May 2024 05:00:48 -0000 1.30 > +++ extern.h 22 Aug 2024 11:02:15 -0000 > @@ -129,6 +129,7 @@ int forward(char *, FILE *, char *, int > void free_child(pid_t); > int from(void *); > off_t fsize(FILE *); > +char* genmsgid(void); > int getfold(char *, int); > int gethfield(FILE *, char *, int, char **); > int gethfromtty(struct header *, int); > Index: send.c > =================================================================== > RCS file: /cvs/src/usr.bin/mail/send.c,v > diff -u -p -r1.26 send.c > --- send.c 8 Mar 2023 04:43:11 -0000 1.26 > +++ send.c 22 Aug 2024 11:02:15 -0000 > @@ -525,6 +525,8 @@ puthead(struct header *hp, FILE *fo, int > fmt("To:", hp->h_to, fo, w&GCOMMA), gotcha++; > if (hp->h_subject != NULL && w & GSUBJECT) > fprintf(fo, "Subject: %s\n", hp->h_subject), gotcha++; > + if (fo != stdout) > + fprintf(fo, "Message-ID: %s\n", genmsgid()), gotcha++; Maybe this needs to check no message id header is already present? > if (hp->h_cc != NULL && w & GCC) > fmt("Cc:", hp->h_cc, fo, w&GCOMMA), gotcha++; > if (hp->h_bcc != NULL && w & GBCC) > Index: util.c > =================================================================== > RCS file: /cvs/src/usr.bin/mail/util.c,v > diff -u -p -r1.2 util.c > --- util.c 26 Dec 2022 19:16:01 -0000 1.2 > +++ util.c 22 Aug 2024 11:02:15 -0000 > @@ -641,3 +641,44 @@ clearnew(void) > } > } > } > + > +/* Generate Message-ID */ > +char* > +genmsgid(void) > +{ > + size_t n = 0; > + char c; > + static char r[24]; > + struct addrinfo hints, *info, *p; > + char *fqdn = NULL; > + static char hostname[NI_MAXHOST]; > + static char buf[sizeof(r) + sizeof(hostname) + 4]; > + int error = 0; > + > + while (n < sizeof(r) - 1) { > + c = arc4random_uniform(173); > + if (isalnum(c) && snprintf(&r[n++], 2, "%c", c) != 1) > + errx(1, "snprintf"); Message-ID is an optional header. No need to terminate the whole application, just because it cannot generate that optional header. Maybe that should have read "genmsgid: snprintf" for consistency, as well. > + } > + > + if (gethostname(hostname, sizeof(hostname))) > + errx(1, "genmsgid: gethostname"); Same here. > + > + memset(&hints, 0, sizeof hints); > + hints.ai_family = AF_UNSPEC; > + hints.ai_socktype = SOCK_STREAM; > + hints.ai_flags = AI_CANONNAME; > + > + if (getaddrinfo(hostname, NULL, &hints, &info) != 0) > + errx(1, "genmsgid: getaddrinfo"); And here. I think getaddrinfo is not needed and just the hostname returned by gethostname should do the job. It's setup from /etc/myname during boot. I am not sure but maybe this tries to perform DNS lookups so mail could no longer be used when the system isn't connected to a network, just to generate an optional message id header? Quite a common usecase. If this takes away that usecase, I am all against it. But not just because of that. See below. > + > + for (p = info; p != NULL; p = p->ai_next) > + fqdn = p->ai_canonname; I am not sure this is correct. The manpage of getaddrinfo has this: If the AI_CANONNAME bit is set, a successful call to getaddrinfo() will return a NUL-terminated string containing the canonical name of the specified host name in the ai_canonname element of the *first* addrinfo structure returned. I would take that "first" seriously. Maybe someone familiar with the asr code implementing getaddrinfo can comment on that. Also this is lacking NULL checks as well. > + > + error = snprintf(buf, sizeof(buf), "<%s@%s>", r, fqdn); > + if (error < 0 || error >= sizeof(buf)) > + errx(1, "genmsgid: snprintf"); Optional header, do not terminate the whole application due to this. > + > + freeaddrinfo(info); > + return(buf); > +} > Re-reading I think there is no value in just blindly adding a message id header. You cannot tell if the user intentionally did not add that header, so this could be rated a regression for some users. For example: for i in `cat /etc/lots_of_recipients`;do echo "Hell of a message." | mail $i;done If mail would silently add a different message-id header to all of those messages, it would certainly behave incorrectly, as that is the same instance of a message sent to multiple recipients from e.g. a script. In my opinion, I would not want mail to start adding that header after an upgrade, the same way I would not want OpenSMTPD to blindly start adding message ids, although it knows nothing about the message or what the author intended to do. If someone replies to one of those messages above to all recipients from /etc/lots_of_recipients, the MUA could not sort out about it to be the exact same message instance replied to, for example. Frankly, Message-ID alone, without References and In-Reply-To does not add any value. Sorry to say. Not an OpenBSD developer. Just reading some of theire mailing lists. I am pretty sure, none of your patches will ever get committed that way. -- Christian