[Mimedefang] [PATCH] filter_data implementation

[-]

I have a few issues with the patch presented below:

1)  The routine appears to want response codes of 2xx, 4xx, or 5xx.  However, the appropriate response code for continuing a message in SMTP is 354.  2xx codes are not valid here (RFC 5321, Section 4.3.2):

      DATA
         I: 354 -> data -> S: 250
                           E: 552, 554, 451, 452
                           E: 450, 550 (rejections for policy reasons)
         E: 503, 554

So, technically the only valid results to "DATA" BEFORE receipt of the headers and body are:  354, 421, 500, 501, 503, and 554.  2xx codes are not permitted.  This is easy to fix.

2)  What information is there at this stage that wasn't available at the filter_recipient() call, other than knowing that all (original) recipients have been collected?  Are we trying to check for a "missing recipient?"  Zero accepted or too many recipients are issues handled by the MTA itself.

If our author chooses to implement this, why not implement the per-header and end-of-headers calls too?  However, just because we can do something doesn't mean we should do it if there's no reason to.  I don't see what the call is trying to accomplish.

If there's a specific issue this is being added to address, then please let us (or at least me) know.

3)  In MXDataOK(), why are we mangling 5 of the variables?  Some we set to "UNKNOWN", but I don't see that elsewhere.  For $name, we're setting it to $ip, but why not set a PROPER literal, "[$ip]", and if it's IPv6, then "[IPv6:$ip]"?  Hasn't sendmail (or another MTA package) already done that for us?

4)  Some minor comments are inline with the patch.

That is all.

--- On Wed, 5/27/09, Martin Blapp <mbr at freebsd.org> wrote:
> And here it is, a filter_data implementation. David, please
...
> --- mimedefang.c    2009-05-24 07:40:40.000000000 +0200
> +++ mimedefang.c    2009-05-24 06:27:35.000000000 +0200
> @@ -978,12 +981,67 @@
> ...
>  static sfsistat mf_data(SMFICTX *ctx)
>  ...
> +    syslog(LOG_WARNING, "postdata: Unable to obtain private data from milter context");

Eliminate the above syslog() line in the final version.  The other milter interface routines don't have it when they perform this check.

> +    DEBUG_EXIT("mf_data", "SMFIS_TEMPFAIL");
> ...

> +    if (n == MD_ACCEPT_AND_NO_MORE_FILTERING) {
> +        /* Called in case we don't need content filtering */
> +        set_dsn(ctx, ans, 2);
                             ^  - Accept -> "3", not "2"
> +        cleanup(ctx);
> ...
> +    if (n == MD_DISCARD) {
> +        set_dsn(ctx, ans, 2);
                             ^  - Accept -> "3", not "2"
> +
> +        cleanup(ctx);
> ...
> +    if (n == MD_CONTINUE) {
> +        /* Called only in case we need to delay */
> +        set_dsn(ctx, ans, 2);
                             ^  - Accept -> "3", not "2"
> +            return SMFIS_CONTINUE;
> ...
> @@ -2092,6 +2150,7 @@
>      fprintf(stderr, "  -r                -- Do relay check before processing body\n");
>      fprintf(stderr, "  -s                -- Do sender check before processing body\n");
>      fprintf(stderr, "  -t                -- Do recipient checks before processing body\n");
> +    fprintf(stderr, "  -A                -- Process body. If not set, content filtering will be skipped\n");

   Process DATA command BEFORE receipt of message body?
   It's NOT processing the body.

>      fprintf(stderr, "  -q                -- Allow new connections to be queued by multiplexor\n");
> ...
> --- utils.c    2009-05-24 07:40:40.000000000 +0200
> +++ utils.c    2009-05-24 06:29:06.000000000 +0200
> @@ -801,6 +801,69 @@
>  ...
> +int
> +MXDataOK(char const *sockname,
> ...
> +    *msg = 0;
> +

  WHY? (following)

> +    if (!sender || !*sender) {
> +    sender = "UNKNOWN";
> +    }
> +
> +    if (!ip || !*ip) {
> +    ip = "UNKNOWN";
> +    }
> +    if (!name || !*name) {
> +    name = ip;
> +    }
> +
> +    if (!firstRecip || !*firstRecip) {
> +    firstRecip = "UNKNOWN";
> +    }
> +    if (!helo) {
> +    helo = "UNKNOWN";
> +    }

  WHY? (above)

> ...