qmail-remote: add infrastructure for EHLO parsing #173

DerDakon · 2020-08-01T10:01:52Z

Implementing extensions for qmail-remote often requires the usage of EHLO and checking if the server supports the extension. Every patch therefore usually comes with it's own version of EHLO parsing. Add yet another one that the patches can easily hook into. For the moment it will just send mails using ESMTP instead of SMTP for basically all cases, but not use any of the new information.

DerDakon · 2020-08-03T20:58:17Z

Would be nice if someone finds out why it doesn't use the %.o rule in the subdirectory.

josuah

This opens the discussion (along with the EHLO generator branch) for how to extend qmail:

through tables that patches fill to add hooks
through inline functions() calls in the source to indicate the extension writer where to place its hooks.

(1.) has the advantage of being explicit.
(2.) looks more like DJB very-static programming structures with few indirections

I prefer 1.

Maybe it can be simplified (for us, but more to write on extension writer part) by letting each callback parse the entire 250-CAPABILITY STRING?

[EDIT: changes requested are for checking the overflow]

qmail-remote.c

tests/unittest_ehlo_parse.c

josuah · 2020-11-08T13:54:32Z

ehlo_parse.h

+ * @brief parse the EHLO replies
+ * @param smtptext the reply to parse
+ * @param callbacks the list of callbacks
+ * @param count number of entries in callbacks
+ * @return mask of the matched callbacks


I do not remember these style of specifications, is the plan to add more as we touch to the source?

It would then help to have a hint about the syntax on the Mailing list or elsewhere.

josuah · 2020-11-08T13:56:27Z

ehlo_parse.h

+   * callback in case a space character follows name in the EHLO response
+   * The function is given the current extension line without the leading 250-
+   * and the length of the remainder, not including the trailing newline. This
+   * includes the name part of the line so one could reuse the same callback
+   * for multiple extensions.
+   *
+   * The callback shall return 0 if the line was ignored, or 1 if it was
+   * accepted.
+   *
+   * In case the callback is NULL the extension is automatically accepted if
+   * the name is matched and either followed by a space or newline.


Theses documentations will become very useful for writing patch extensions. There could be a EXTENDING file that point at these various locations that help extending it.

josuah · 2020-11-08T13:59:22Z

tests/unittest_ehlo_parse.c

+  tcase_add_test(tc, test_ehlo_noparams);
+  tcase_add_test(tc, test_ehlo_nodupes);
+  tcase_add_test(tc, test_ehlo_params);
+


Maybe adding a test for a 3-byte thingy.s to check whether there is overflow? (see above)

josuah · 2020-11-08T14:09:55Z

How does the extension writer knows what to do with the bitmask returned by ehlo_parse()?

It works for static arrays where extension writers can #define EXTENSION_SMTPAUTH 1 << 1, but after multiple patches are applied, the list could be mixed up in a hard to predict order (depending on the order of the patches and patch(1) algorythm).

[EDIT: For checking which capability the remote has from an extension: maybe a function(unsigned int the_bitmask, struct smtpext callback[], char * string_to_match_against_smtpext_name) to call, but then it not as easy as just bitmask & EXTENSION_SMTPAUTH. Another take is to have one global number per extension in an enum { }, and then use this syntax:

struct smtp callbacks[] = {
    [EXT_SMTPAUTH] = ext_smtpauth_ehlo
    [EXT_STARTTLS] = ext_starttls_ehlo,
    [...]
};

Then matching the extension and the callback entry is done, and this does not require cooperation between the various patches thanks to declarations of EXT_... happening in an enum }]

josuah · 2020-11-08T14:18:14Z

tests/unittest_ehlo_parse.c

+  thingy.len = strlen(thingy.s);
+
+  const struct smtpext callbacks[] = {
+    bad_call_entry,


I did not know the compiler could handle inlining a static struct into an array. I thought it was only possible with pointers. This is convenient.

schmonz · 2020-11-10T20:42:47Z

Are there so many EHLO-capability-implementing extensions for qmail-remote out there? The TLS patch, maybe the AUTH patch... what else exists that I should be aware of?

DerDakon · 2020-11-11T17:06:34Z

I immediately remember SIZE and SMTPUTF8.

schmonz · 2020-11-11T19:13:17Z

Thought of SMTPUTF8 right after posting, as one does. Managed to not have SIZE occur to me despite having re-read that RFC today. Perhaps unsurprisingly, I can’t imagine there being too many other popular qmail-remote patches. But maybe I’m not wrong this time :-p

I don’t dislike this code, I’m just not sure it’s worth its cost. Are we hoping patch maintainers for SMTPUTF8, SIZE, STARTTLS, and AUTH will (1) notice and appreciate that notqmail is alive, (2) care to update their patches, (3) want to do so in a way that limits their applicability? Or are we thinking to take advantage of our branches for popular patches rebased onto notqmail and do our own publishing (and maintenance) of these affected patches?

I suspect the changes in this PR don’t really pay off for us unless/until all four patches (and any other important ones we might be forgetting) have been adjusted to this new API. And it seems very unlikely to me that any of the upstream patch maintainers will want to, even if we help them.

I’m not eager to deal with hand-rebasing more patches than I’ve already been doing for pkgsrc. I imagine this PR is motivated in part by you doing the same for Gentoo.

Are we in agreement that all four of these bits of functionality (SMTPUTF8, SIZE, STARTTLS, AUTH) should land in the default install at some point, in some way? If you’re thinking not (for any or all), I’m open to hearing reasons why not, and then I might see more value here. But if yes, then I need to get my thoughts about refactoring qmail-remote out of my head and onto a wiki page — because my intuition here, despite my usual bias for introducing interfaces and seams, is that we can better spend our efforts making the need for an EHLO-parsing API disappear.

DerDakon · 2020-11-11T19:33:12Z

I have written a patch that adds SIZE support long time ago and can open a PR for that so we would get that for free as a starting point.

But you are right, most motivation is that patches in Gentoo collide heavily because every one of them has to create another version of EHLO parsing.

schmonz · 2020-11-14T12:46:20Z

Okay, I've tried to show what I'm thinking about for qmail-remote. Does that look like a fruitful direction to you?

Implementing extensions for qmail-remote often requires the usage of EHLO and checking if the server supports the extension. Every patch therefore usually comes with it's own version of EHLO parsing. Add yet another one that the patches can easily hook into. For the moment it will just send mails using ESMTP instead of SMTP for basically all cases, but not use any of the new information.

DerDakon · 2020-11-25T16:18:18Z

Just a datapoint: when combining TLS and AUTH patches the custom EHLO code that both introduce collides also. So we only make things collide right from the start, not only when combining patches. But hopefully things get easier afterwards.

If we could convince the upstreams to just include our EHLO patches at the start of their next version this would work with both old and new versions easily… (just dreaming).

josuah · 2020-11-25T23:54:51Z

State of the patches for this, since we talk about it...
https://notqmail.z0.is/commit/6897896cb410a34f6fd4e80741a28d58678b17d7/

[EDIT: as well as master:
https://notqmail.z0.is/commit/aedd8beb2484608eb97ee3299e7f3b8cdde061ce/ ]

schmonz · 2020-12-18T10:41:20Z

If qmail-remote had grown EHLO hooks during the DJB era, absolute no-brainer: all the popular patches would have used it and our lives as packagers would have been slightly more glorious ever since.

Since that didn't happen, various qmail-remote patches are all doing their own EHLO parsing and interfering with each other. Decidedly non-glorious, and a pain we want to ease.

If we change the status quo while those patches are still generally necessary, we're taking on some risk.

The best-case scenario, for us, would be that all the patch maintainers release updated patches targeting notqmail 1.09 on the same day we ship it. This gambles that they're all actively maintaining their patches, they're all convinced that notqmail is the future, and they're all available to coordinate with us in a timely fashion.

The worst-case scenario would be that they're inactive, unconvinced, and/or too busy. We'd be forcing ourselves to issue rebased patches and (probably, unless upstreams change their minds) maintain them for as long as they're still generally necessary. We'd be forcing our source-based users with custom patches to understand why this notqmail update is trickier than usual and why they can trust our versions of these patches.

The worst-case scenario seems much more likely, which is why my spidey sense says we should do deeper structural work on qmail-remote, off to the side, until it makes all the popular patches unnecessary (and then it'll be low-impact to merge this, making life easier for the unpopular patches).

I'm willing to be convinced merging this sooner won't cost us so much and will bring enough benefits to be worth the churn. I'd like to understand our plan for managing it.

DerDakon · 2020-12-20T15:57:56Z

The day we start merging SMTPUTF8 or SIZE extensions we will be breaking all of those patches anyway. We should be doing this in a way that avoids breaking them again.

schmonz · 2020-12-20T19:45:42Z

Hit a key by mistake, not even sure which one

schmonz · 2020-12-20T20:00:01Z

I'm guessing we agree in principle about trying to minimize the total area under the pain curve (accounting for us as developers, us as packagers, and source-based users with patches) as we move incrementally from our current qmail-remote to a more desirable one. We might be disagreeing about which sequence of changes is most likely to minimize this integral.

Let me propose a concrete sequence (you'll recognize it from Designs), and you can point at the steps or orderings you disagree with, and/or share the sequence you're thinking of.

Copy qmail-remote to qmail-newremote
Extract qmail-newremote's SMTP client to qmail-smtpc and run it under tcpclient
Negotiate TLS in qmail-smtpc (via UCSPI-TLS), and let admin configure an alternate UCSPI client
qmail-newremote is going well! Needs more capabilities added. It's time for qmail-remote: add infrastructure for EHLO parsing #173
Add AUTH, maybe matching s/qmail's behavior (see Feature request: relaying to different smarthosts #175 for some of it)
Add SIZE, PIPELINING, SMTPUTF8, and IPv6 in whatever order is sensible

We can merge to master at each step.

Pros:

qmail-remote remains predictably annoying for users and packagers to patch: our coping strategies can remain unchanged for a while longer
Adventurous users can start giving us feedback about qmail-newremote
We'll have "data" (as much as we could possibly get, anyway) telling us when's the right time to promote qmail-newremote to qmail-remote
At that point, most users probably won't have any custom qmail-remote patches left; if they do, the new features will be worth the pain of rebasing their patches onto qmail-smtpc and the standard EHLO parser
We could keep qmail-oldremote around for another release or two, in case we missed something difficult about the transition (yay QMAILREMOTE)

Cons:

Working in parallel might mean fixes like improve smtpcode() input validation #190 need to be applied in two places for a while
Admins might be confused about whether they're supposed to be using qmail-newremote
Others?

DerDakon · 2021-08-13T08:08:13Z

Extract qmail-newremote's SMTP client to qmail-smtpc and run it under tcpclient

So now we end up having 3 copys. Or we have a new source file that these 3 binaries use, which will break just every patch to qmail-remote.

DerDakon requested review from schmonz, josuah, alanpost, jlmuir, leahneukirchen, mbhangui and xenotrope August 1, 2020 10:02

DerDakon added the development label Aug 1, 2020

DerDakon force-pushed the Dakon-remote-EHLO branch 2 times, most recently from d34b10e to b4419ec Compare August 3, 2020 19:08

DerDakon force-pushed the Dakon-remote-EHLO branch from 725dfa2 to 5e1847e Compare August 4, 2020 17:22

DerDakon force-pushed the Dakon-remote-EHLO branch from 5e1847e to 192934b Compare September 12, 2020 16:14

schmonz removed the request for review from alanpost November 3, 2020 14:57

josuah requested changes Nov 8, 2020

View reviewed changes

josuah reviewed Nov 8, 2020

View reviewed changes

DerDakon force-pushed the Dakon-remote-EHLO branch 2 times, most recently from 5145e99 to 33dc39a Compare November 8, 2020 19:39

DerDakon added the enhancement New feature or request label Nov 9, 2020

tests: prevent duplication in Makefile

aa36dde

DerDakon force-pushed the Dakon-remote-EHLO branch from 33dc39a to 8674996 Compare November 21, 2020 16:06

DerDakon force-pushed the Dakon-remote-EHLO branch from 8674996 to 6897896 Compare November 21, 2020 16:13

DerDakon mentioned this pull request Nov 22, 2020

add support for SIZE extension (RfC 1870) #191

Open

schmonz closed this Dec 20, 2020

schmonz reopened this Dec 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

qmail-remote: add infrastructure for EHLO parsing #173

qmail-remote: add infrastructure for EHLO parsing #173

DerDakon commented Aug 1, 2020

DerDakon commented Aug 3, 2020

josuah left a comment •

edited

josuah Nov 8, 2020

DerDakon Nov 8, 2020

josuah Nov 8, 2020

josuah Nov 8, 2020

josuah commented Nov 8, 2020 •

edited

josuah Nov 8, 2020 •

edited

schmonz commented Nov 10, 2020

DerDakon commented Nov 11, 2020

schmonz commented Nov 11, 2020

DerDakon commented Nov 11, 2020

schmonz commented Nov 14, 2020

DerDakon commented Nov 25, 2020

josuah commented Nov 25, 2020 •

edited

schmonz commented Dec 18, 2020

DerDakon commented Dec 20, 2020

schmonz commented Dec 20, 2020

schmonz commented Dec 20, 2020

DerDakon commented Aug 13, 2021

qmail-remote: add infrastructure for EHLO parsing #173

Are you sure you want to change the base?

qmail-remote: add infrastructure for EHLO parsing #173

Conversation

DerDakon commented Aug 1, 2020

DerDakon commented Aug 3, 2020

josuah left a comment • edited

Choose a reason for hiding this comment

josuah Nov 8, 2020

Choose a reason for hiding this comment

DerDakon Nov 8, 2020

Choose a reason for hiding this comment

josuah Nov 8, 2020

Choose a reason for hiding this comment

josuah Nov 8, 2020

Choose a reason for hiding this comment

josuah commented Nov 8, 2020 • edited

josuah Nov 8, 2020 • edited

Choose a reason for hiding this comment

schmonz commented Nov 10, 2020

DerDakon commented Nov 11, 2020

schmonz commented Nov 11, 2020

DerDakon commented Nov 11, 2020

schmonz commented Nov 14, 2020

DerDakon commented Nov 25, 2020

josuah commented Nov 25, 2020 • edited

schmonz commented Dec 18, 2020

DerDakon commented Dec 20, 2020

schmonz commented Dec 20, 2020

schmonz commented Dec 20, 2020

DerDakon commented Aug 13, 2021

josuah left a comment •

edited

josuah commented Nov 8, 2020 •

edited

josuah Nov 8, 2020 •

edited

josuah commented Nov 25, 2020 •

edited