From: Walter Alejandro Iglesias Subject: Re: Adding Message-ID to mail(1) (diff UPDATED) To: Christian Schulte Cc: tech@openbsd.org Date: Sat, 24 Aug 2024 14:20:09 +0200 On Sat, Aug 24, 2024 at 08:41:30AM +0200, Christian Schulte wrote: > 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? OpenBSD mail(1) has no "adding headers" option as other versions of mailx. That can't happen in this case. > > > 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. This will fail only if the code is bad written. In this case there's not even a variable input lenght. What about those checks used with malloc()? In previos versions of my diffs I didn't do these checks, I included them trying to follow the "secure" way things are preferred in this project. > > > > + } > > + > > + if (gethostname(hostname, sizeof(hostname))) > > + errx(1, "genmsgid: gethostname"); > > Same here. Idem above. > > > + > > + 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. I read this but I still don't fully understand it. I'll take a look. Anyways, this is the adaptation I wrote to Linux, where gethostname() prints only the hostname and POSIX HOST_NAME_MAX limited to only 64 characters. Personally I'd prefer my non-portable version. > > > + > > + 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. Same as the other. > > > + > > + 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, Did you try do that with other command line MUAs, Mutt for example? > 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, You say that OpenSMTPD is also doing it wrong? > 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. Is there some command line MUA that behave in the way you're asking? > Frankly, Message-ID alone, without References and In-Reply-To > does not add any value. You need to read the whole history (I don't advise you to do it :-)). They asked me to split my diff and poste my proposal in chunks. You can download my patches and take a look to my whole proposal: https://en.roquesor.com/Downloads/mail_patches.tar.gz > 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. I'd love to be as sure as you seem to be about what they want. :-) > > -- > Christian > > Thank Christian you for your suggestions! -- Walter