Import Bruce Guenter's qmail-qfilter #227

schmonz · 2021-10-22T13:39:31Z

One of the blockers for releasing notqmail 1.09 is that in 1.08 we introduced a regression in how recipient addresses are qualified. We probably could have avoided the mistake if there'd been tests around that code, and before we try to fix it we'll want those tests in place.

With the in-tree tools currently available, there's no way to test the recipient-address qualification code without also creating and manipulating an on-disk mail queue. We'd like to avoid that integration point, and we can accomplish this by importing qmail-qfilter.

Besides its utility in tests, qmail-qfilter has been a staple of many qmail administrators' production environments for well over a decade, and its design (along with the QMAILQUEUE patch we've had since 1.07) feels like something qmail should have always included. In conjunction with the custom-error patch already merged, qmail-qfilter will finally provide our users with a no-patching-required Unix-filter API for programmatically modifying or rejecting messages during the SMTP conversation.

Besides its utility in production, having qmail-qfilter in-tree provides a needed building block for future functionality.

josuah

Should there also be a change to COPYRIGHT?
This would get added at the time of a new release maybe.

It is really good that there are well-established filtering mechanism, that saves us to add our own, and will permit many changes without patches, maintained as "plugins" or "filters".

Result of make brokemaster BRANCH=schmonz-qmail-qfilter-osf on https://github.com/josuah/notqmail-patch:

schmonz-qmail-qfilter-osf broke ext-todo
schmonz-qmail-qfilter-osf broke smtp-tls

smtp-tls: failed hunks

Makefile

@@ -781,7 +781,7 @@
 forward preline condredirect bouncesaying except maildirmake \
 maildir2mbox install instpackage instqueue instchown \
 instcheck home home+df proc proc+df binm1 binm1+df binm2 binm2+df \
-binm3 binm3+df
+binm3 binm3+df update_tmprsadh

 load: \
 make-load warn-auto.sh

hier.c.rej:

@@ -144,6 +144,7 @@
   c(auto_qmail,"bin","except",auto_uido,auto_gidq,0755);
   c(auto_qmail,"bin","maildirmake",auto_uido,auto_gidq,0755);
   c(auto_qmail,"bin","maildir2mbox",auto_uido,auto_gidq,0755);
+  c(auto_qmail,"bin","update_tmprsadh",auto_uido,auto_gidq,0755);

   c(auto_qmail,"man/man5","addresses.5",auto_uido,auto_gidq,0644);
   c(auto_qmail,"man/cat5","addresses.0",auto_uido,auto_gidq,0644);

big-todo: failed hunks

qmail-send.c:

@@ -1255,8 +1256,7 @@
  char ch;
  int match;
  unsigned long id;
- unsigned int len;
- direntry *d;
+ int z;
  int c;
  unsigned long uid;
  unsigned long pid;
@@ -1267,32 +1267,26 @@

  if (flagexitasap) return;

- if (!tododir)
+ if (!flagtododir)
   {
    if (!trigger_pulled(rfds))
      if (recent < nexttodorun)
        return;
    trigger_set();
-   tododir = opendir("todo");
-   if (!tododir)
-    {
-     pausedir("todo");
-     return;
-    }
+   readsubdir_init(&todosubdir, "todo", pausedir);
+   flagtododir = 1;
    nexttodorun = recent + SLEEP_TODO;
   }

- d = readdir(tododir);
- if (!d)
+ switch(readsubdir_next(&todosubdir, &id))
   {
-   closedir(tododir);
-   tododir = 0;
-   return;
+    case 1:
+      break;
+    case 0:
+      flagtododir = 0;
+    default:
+      return;
   }
- if (str_equal(d->d_name,".")) return;
- if (str_equal(d->d_name,"..")) return;
- len = scan_ulong(d->d_name,&id);
- if (!len || d->d_name[len]) return;

  fnmake_todo(id);

Makefile

qmail-qfilter.c

schmonz · 2021-10-22T15:23:59Z

I'm extremely happy (@josuah, I don't have the words, but at least "thank you") to find out immediately that this would break ext-todo and smtp-tls. Anyone have suggestions for how to tweak the PR to avoid these conflicts?

DerDakon

First sweep of things, not meant to be exhaustive.

hier.c

Makefile

qmail-qfilter.c

schmonz · 2021-10-22T19:05:50Z

I agree that qmail-qfilter.c needs more attention to fit smoothly into our codebase, and that one thing we needn’t worry about is breaking people’s patches (since from our codebase’s point of view this is simply new code).

I do still worry about

Inadvertently breaking qmail-qfilter (since we don’t have tests for it yet)
Unnecessarily slowing down landing a fix for Regression in 1.08: recipient-address qualification #147 (which is one of the blockers for 1.09)

As usual, I’m not quibbling about what needs doing, only about the ordering. My thinking on this, right now, is:

In this PR, qmail-qfilter should consist of the exact same code administrators have been relying on, so that we already know it works as expected with no regressions and can defer writing tests for it
Once this PR is merged, I’ll provide a branch with regression tests for Regression in 1.08: recipient-address qualification #147
Once we have those tests running, we can merge @DerDakon’s fix
One step closer to 1.09

Post-1.09, I’m all for writing tests around qmail-qfilter and then cleaning it up however we want. But if we don’t have to do that part now — and I think we don’t — then I think we should defer that part.

DerDakon · 2021-10-23T18:29:19Z

I would squash the second commit into the first, and note what you have done in the commit message. That way these files don't clutter the git history forever.

DerDakon · 2021-10-24T07:49:57Z

And one other thing that I just found: the first commit should really just be authored by Bruce. Since @schmonz did the actual commit he will show up in history as the one who introduced that code into the repo. End of story (Co-authors could be used for people helping write the original codebase).

People that helped with this import should get co-author status for the following cleanups.

schmonz · 2022-01-19T12:13:10Z

I've rewritten the PR so it consists of

Adding the license, manpage, and .c directly under gplv2/ (attributed to @bruceg)
Making it build, install, and clean (attributed to my PubMob cohort)
Rewriting the manpage, attributed to me
Converting a few tabs to spaces to fix indentation

Since the code is very nearly identical to what many of us have been running in production for many years, I'm assuming we're comfortable deferring further cleanups to the code, all of which I've noted locally for later ("later" == after 1.09 and after we have automated tests).

Does anyone have any doubts or concerns on the license front? Or could this be mergeable?

schmonz · 2022-01-21T16:00:51Z

Updates:

Bruce has expressed intent (on Twitter and in private mail) to relicense qmail-qfilter -- I believe not only for us but for all users -- as some kind of public-domain. His default preference is the Unlicense, which would seem to make it extremely easy for us to not worry about the license while importing, and which might give us a nudge toward doing the same for the rest of our codebase (see License, unlicense, etc. #178).
This branch currently proposes the 2.1 release. It so happens there are a few newer commits in qmail-qfilter git, not yet in any release. One of them is my QMAILPPID patch, which I've been running in production for many years. The others are manpage tweaks.

Assuming Bruce can relicense qmail-qfilter without too much delay, I'll wait for that, then update this PR to remove the GPLv2 bits and catch up with the latest version.

From qmail-qfilter.git at commit 795718fe819266df3a363519231f513a8e882f9e (the newest available on master).

Co-authored-by: Amitai Schleier <schmonz-web-git@schmonz.com> Co-authored-by: Astrid Sawatsky <astrid.sawatzky@gmx.de> Co-authored-by: Evan Theriault <evanontario@hotmail.com> Co-authored-by: Jacqueline Bilston <jmasonlee@gmail.com> Co-authored-by: Joel Samaroo <xsamarjoex@gmail.com> Co-authored-by: Patrick Ledzian <LiveActionCactus@users.noreply.github.com> Co-authored-by: Trung Vo <ttrung.vo@gmail.com> Co-authored-by: Yevhen Viktorov <y3vg3nk0@gmail.com>

schmonz · 2022-03-10T20:33:00Z

Assuming Bruce can relicense qmail-qfilter without too much delay, I'll wait for that, then update this PR to remove the GPLv2 bits and catch up with the latest version.

This has been done 😁

qmail-qfilter.c

DerDakon

I think we should put those things "marked for future cleanups", at least the missing range checks and such, right into this branch. Or we flood @bruceg with PRs cleaning up all these and then just merge the latest version. Whatever works best ;)

DerDakon · 2022-03-12T19:22:19Z

qmail-qfilter.c

+{
+  const char* env;
+  size_t offset;
+  if ((env = mmap(0, env_len, PROT_READ, MAP_PRIVATE, ENVIN, 0)) == MAP_FAILED)


The first 0 must become NULL.

DerDakon · 2022-03-12T19:30:38Z

qmail-qfilter.c

+static size_t msg_len = 0;
+
+/* Parse the sender address into user and host portions */
+size_t parse_sender(const char* env)


This whole functions has a fundamental issue if the input is not "sane", i.e. if the data that is mmap'ed is not 0-terminated. In this case the strlen() below will run out of bounds and likely crash. The length passed to mmap will need to be passed to this function, and to parse_rcpts() as well so one can replace the strlen(env) by memchr(env, 0, envlen).

I tried to address this upstream in bruceg/qmail-qfilter#4

DerDakon · 2022-03-12T19:35:47Z

qmail-qfilter.c

+/* Create a temporary invisible file opened for read/write */
+int mktmpfile()
+{
+  char filename[sizeof(TMPDIR)+19] = TMPDIR "/fixheaders.XXXXXX";


The size of the buffer is not needed, when it is initialized it will automatically get the correct size. This will as well remove the magic 19.

DerDakon · 2022-03-12T19:38:05Z

qmail-qfilter.c

+  command* tail = 0;
+  command* head = 0;


These should be NULL.

DerDakon · 2022-03-12T19:42:55Z

qmail-qfilter.c

+
+  filters = parse_args(argc-1, argv+1);
+
+  if ((qqargv[0] = getenv("QQF_QMAILQUEUE")) == 0)


schmonz requested review from DerDakon, josuah, leahneukirchen and mbhangui as code owners October 22, 2021 13:39

josuah approved these changes Oct 22, 2021

View reviewed changes

Makefile Show resolved Hide resolved

qmail-qfilter.c Show resolved Hide resolved

qmail-qfilter.c Show resolved Hide resolved

schmonz added this to the 1.09 milestone Oct 22, 2021

schmonz added bug Something isn't working enhancement New feature or request labels Oct 22, 2021

DerDakon requested changes Oct 22, 2021

View reviewed changes

mbhangui approved these changes Oct 23, 2021

View reviewed changes

schmonz force-pushed the schmonz-qmail-qfilter-osf branch 2 times, most recently from f06c05c to b98f098 Compare January 19, 2022 11:39

schmonz requested a review from DerDakon January 19, 2022 14:18

schmonz force-pushed the schmonz-qmail-qfilter-osf branch from 202c30d to 06d121d Compare January 19, 2022 14:24

schmonz mentioned this pull request Jan 20, 2022

Test qmail-inject recipient address qualification. #232

Open

schmonz force-pushed the schmonz-qmail-qfilter-osf branch 3 times, most recently from f791a11 to 03c3cfd Compare January 25, 2022 21:27

schmonz mentioned this pull request Jan 25, 2022

qmail-inject: fix header parse regression #229

Merged

bruceg and others added 3 commits March 1, 2022 10:38

Import qmail-qfilter code and manpage.

7f203d2

From qmail-qfilter.git at commit 795718fe819266df3a363519231f513a8e882f9e (the newest available on master).

Rewrite qmail-qfilter manual page.

0603868

schmonz force-pushed the schmonz-qmail-qfilter-osf branch from 7015b72 to 0603868 Compare March 10, 2022 20:29

DerDakon reviewed Mar 12, 2022

View reviewed changes

qmail-qfilter.c Outdated Show resolved Hide resolved

qmail-qfilter: honor conf-qmail.

b704fb6

DerDakon requested changes Mar 12, 2022

View reviewed changes

schmonz modified the milestones: 1.09, 1.90 Sep 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import Bruce Guenter's qmail-qfilter #227

Import Bruce Guenter's qmail-qfilter #227

schmonz commented Oct 22, 2021

josuah left a comment •

edited

schmonz commented Oct 22, 2021

DerDakon left a comment

schmonz commented Oct 22, 2021

DerDakon commented Oct 23, 2021

DerDakon commented Oct 24, 2021

schmonz commented Jan 19, 2022

schmonz commented Jan 21, 2022

schmonz commented Mar 10, 2022

DerDakon left a comment

DerDakon Mar 12, 2022

DerDakon Mar 12, 2022

DerDakon Oct 22, 2022

DerDakon Mar 12, 2022

DerDakon Mar 12, 2022

DerDakon Mar 12, 2022


		filters = parse_args(argc-1, argv+1);

		if ((qqargv[0] = getenv("QQF_QMAILQUEUE")) == 0)

Import Bruce Guenter's qmail-qfilter #227

Are you sure you want to change the base?

Import Bruce Guenter's qmail-qfilter #227

Conversation

schmonz commented Oct 22, 2021

josuah left a comment • edited

Choose a reason for hiding this comment

schmonz commented Oct 22, 2021

DerDakon left a comment

Choose a reason for hiding this comment

schmonz commented Oct 22, 2021

DerDakon commented Oct 23, 2021

DerDakon commented Oct 24, 2021

schmonz commented Jan 19, 2022

schmonz commented Jan 21, 2022

schmonz commented Mar 10, 2022

DerDakon left a comment

Choose a reason for hiding this comment

DerDakon Mar 12, 2022

Choose a reason for hiding this comment

DerDakon Mar 12, 2022

Choose a reason for hiding this comment

DerDakon Oct 22, 2022

Choose a reason for hiding this comment

DerDakon Mar 12, 2022

Choose a reason for hiding this comment

DerDakon Mar 12, 2022

Choose a reason for hiding this comment

DerDakon Mar 12, 2022

Choose a reason for hiding this comment

josuah left a comment •

edited