[Mailmunge] [Mimedefang] Mailmunge API discussion (was Re: MIMEDefang 3.0-rc1)

Wed May 18 01:23:25 EDT 2022

On 5/17/22 15:56, Dianne Skoll via MIMEDefang wrote:
> On Sat, 14 May 2022 00:18:48 -0500 Richard Laager wrote:
> 
>>    * Most annoyingly, there are still the two return styles for message
>>      dispositions depending on whether we are in filter_message() or
>>      something earlier.
> 
> Fixed in recent commits.  Now any of the filter_* functions can return
> a Mailmunge::Response object, which is respected.  However, you can
> still use the old-style action_whatever calls and everything will Just Work.

Typo: stray quote in: $self->action_discard($ctx"); return;

>>    * Unless it's impossible (or unreasonable) to do optional arguments
>> in Perl, I don't see why there are both action_add_header() and
>>      action_insert_header(). Just have the insert, with $pos defaulting
>>      to -1 or something to get the add behavior.
> 
> Fixed in https://git.skoll.ca/Skollsoft-Public/mailmunge/commit/bd03caa35290760d7bb2b86d26c3a3f8505c08f6

I'd probably tighten this to -1 specifically (rather than the more 
general "negative"), at least in the docs. The idea for -1 as the magic 
value is that it is consistent with negative array indexes.

Whether you actually need to support e.g. -2 to put it before the last 
existing occurrence of the header is another matter. (This would involve 
knowing how many of that header exist and then converting a negative 
index into a positive one to send back in the milter response.) But 
documenting as -1 specifically would allow for that implementation in 
the future if someone comes up with a reason to need it.

----

You can't fix this without breaking compatibility, but it seems that 
action_change_header() and action_delete_header() are 1-indexed while 
action_insert_header() is 0-indexed. Is there a good reason for that?

> Likewise for
>>      action_{accept,drop}_with_warning(); just have an optional
>> $warning parameter on action_{accept,drop}().
> 
> Fixed in https://git.skoll.ca/Skollsoft-Public/mailmunge/commit/ef8a966d3be40b3d14f8c52d7ff41dad89f7076f

Probably typo: with-warning should be with_warning here: "If C<$warning> 
is supplied, then we call C<action_accept_with-warning>."

That said, I would inline the explanation of warning, like this:

... However, if C<$warning> is supplied, a warning message is added in a 
new C<text/plain> part that is appended to the message.

Then I'd deprecate all the action_*_with_warning() functions.

For consistency in the code (though it doesn't affect the API), I might 
then move the action_add_with_warning() code into action_add() and make 
action_add_with_warning() call action_add(), as you did with the other ones.

>>    * In the Mailmunge example video, $ctx->recipients[0] is
>>      '<bob at example.org>'. IMHO, mailmunge should be stripping off the
>>      angle brackets before the filter see it. They are just an
>> annoyance. Perhaps it should be lowercasing too, i.e. using
>> canonical_email().
> 
> Added $ctx->canonical_sender and $ctx->canonical_recipients; commit
> https://git.skoll.ca/Skollsoft-Public/mailmunge/commit/a83a841837e16584af550ed7ea9af8b9d86f5a62

+1 to spelling out canonical there instead of canon_.

Should this be memoized (cached) so that multiple calls to e.g. 
canonical_recipients() don't have to redo the work every time? Granted, 
I'm sure canonical_email() is relatively cheap, but still.

How would this intersect with the idea of sender/recipients being made 
mutable?

----

One of my local changes is to what is now _spam_assassin_init, to do the 
"use Mail::SpamAssassin" lazily:
     eval 'use Mail::SpamAssassin ();';

My comment says:

We wait to load the SpamAssassin module until it is actually going to be 
used.  MIMEDefang's built-in behavior is to load the SpamAssassin module 
right away.  By waiting, we reduce the memory consumption of workers 
that never call SpamAssassin.  For example, workers that happen to only 
handle filter_sender and filter_recipient checks.

I think I added this because at some point MIMEDefang was changed to try 
to group the non-DATA filter calls into specific child processes.

Thoughts?

----

Another local change I have is for spam_assassin_mail(), which says:
This is a modified version of MIMEDefang's spam_assassin_mail() for use 
with outgoing mail.  If the user is a local relay, the "Received:" 
header will note that they authenticated so SpamAssassin doesn't 
penalize the user (via various tests) for sending "direct to MX" mail.

The actual code says:
     # This is synthesize_received_header() inlined, except that if
     # $local_relay is set, we add "(authenticated bits=0)" to the
     # "Received:" header.

$local_relay is a variable in my filter that is set based on auth_authen 
or IP*.

Is that needed these days (with Mailmunge)? I note that 
synthesize_received_header() checks for auth_authen and uses "ESMTPA" 
instead of "ESMTP". Is that latter bit sufficient to make SpamAssassin 
happy (as opposed to my "(authenticated bits=0)"?

* These days, I only allow IP-based authentication for very rare 
exceptions. If I could 100% kill that, then the stock 
synthesize_received_header() seems fine. If I can't, what would you suggest:

A) Can I set $ctx->sendmail_macro('auth_authen') if my IP-check passes?
B) Should I just override synthesize_received_header() in my subclass
    (copying all of it and changing how $auth is set)?
C) API changes to Mailmung (e.g. synthesize_received_header() takes an
    optional $auth parameter, or there is some function like
    is_authenticated() that defaults to checking auth_authen but could
    be overridden in a subclass?
D) Something else?

----

On a more general note, what sort of linters are the cool kids using 
these days with Perl? Perl::Lint? Should Mailmunge be using one?

----

I know you stopped using GitHub for $reasons. I'm not looking to debate 
those reasons. But it would be nice to have some issue tracker and pull 
request process. Then I could be submitting these things as separate 
issues, and for some of them, pull requests.

In the past, I might have suggested (hosted) GitLab, except that they're 
cracking down on their free tier now, though they do still havve an Open 
Source Program: https://about.gitlab.com/solutions/open-source/join/ 
Self-hosting a GitLab instance is quite a pain, I'm told.

Sourcehut is alpha and will eventually be charging:
https://sourcehut.org/alpha-details/

You're already self-hosting with Gitea. I'm not sure what all you can 
configure. Can you allow people to create accounts but not repositories? 
That would fix the issue tracker piece. That still wouldn't allow for 
pull requests, but maybe you could allow that on a manual opt-in basis. 
I fully understand why you probably don't want to all arbitrary git 
hosting for random people.

-- 
Richard