Le lun. 13 janv. 2020 à 09:59, Sébastien Michel <seb2li...@gmail.com> a écrit :
>
> Le dim. 12 janv. 2020 à 21:28, Wietse Venema <wie...@porcupine.org> a écrit :
> > Viktor Dukhovni:
> > > My instinct was to suggest that the filter goes first, and then the
> > > footer gets added, but usability rather depends on how the pending
> > > multi-line response is presented to the filter. If it is easy for the
> > > filter to match and selectively retain the footer, then you're probably
> > > right and the filter should have the final say. If, on the other hand,
> > > it becomes hard to selectively retain the footer, then I'd be tempted to
> > > add it last.
> > >
> > > The "\c" trick no longer works once the footer is combined with the
> > > original text, because (at least in principle) by that point the
> > > original text could have a verbatim "\c" in it, that is not intended to
> > > become a line-break.
> >
> > \c in a reply filter? I don't see any need for any special
> > sequences other than ${number} which is already supported.
>
> My opinion is mostly the same : if we introduce a new
> smtpd_reject_filter_maps parameter it would be executed first : the
> purpose is indeed to replace hard-coded internal message by another
> one at first.
>
> Secondly, if the reply message currently processed has not been
> replaced the code would check smtpd_reject_footer_maps and apply the
> footer if the message match. My thinking is that an administrator have
> to choose between selective replacement and selective footer, there is
> no really need to execute both : you can do it all at once with
> smtpd_reject_filter_maps (and managing the sequence of the two would
> be hard to debug : does the second regexp applies before or after text
> replacement ?)
>
> Thirdly, like the current behavior of postfix, smtpd_reject_footer
> must not be executed if smtpd_reject_footer_maps has matched, however
> whatever the execution of smtpd_reject_filter_maps the
> smtpd_reject_footer is executed ! we don't want to repeat ourself by
> adding a common footer to all the replacement text.
>
> Here is an example :
> smtpd_reject_footer_maps =
> regexp:$postfix_config_dir/amend_smtpd_responses.regexp
> smtpd_reject_filter_maps =
> regexp:$postfix_config_dir/replace_smtpd_responses.regexp
> smtpd_reject_footer = \c. Pour assistance, appelez-le 800-555-0101.
>
> amend_smtpd_responses.regexp:
> # this file add a footer to the reply with the same error message
> but in french
>
> # only for the example, this regexp is ignored because
> smtpd_reject_filter_maps define the same rule
> # 550 5.1.1 <f...@domain.tld>: Recipient address rejected: User unknown
> /^550 5.1.1 (<[^>]*>):/ $1: Adresse destinataire invalide.
> # 504 5.5.2 <foo@domain>: Sender address rejected: need
> fully-qualified address
> /^504 5.5.2 (<[^>]*>):/ $1: Emetteur invalide.
>
> replace_smtpd_responses.regexp :
> # this file replace hard-coded error message by french one
> # 550 5.1.1 <f...@domain.tld>: Recipient address rejected: User unknown
> /^550 5.1.1 (<[^>]*>):/ $1: Adresse destinataire invalide.
>
> The result for unknown recipient :
> rcpt to:<fkldk...@example.com>
> 554 5.7.1 <fkldkfdl@example>: Adresse destinataire invalide. Pour
> assistance, appelez-le 800-555-0101.
>
> > So we need a canonical multiline representation before we can even
> > think about filtering SMTP server replies. It's probably going to
> > be embedded \r\n sequences, with smtpd_chat_reply() implementing
> > the filter-then-footer or footer-then-filter, and then splitting
> > the multiline text into separate lines, before doing output and
> > appending to the session history.
> I've no opinion about that, I think that the change don't break
> anything (replacing everything starting after the first SMTP error
> code and its following SMTP extended status code)
>
> Below an extract of the proposed code :
> src/global/smtp_reply_footer.[ch] are renamed into
> src/global/smtp_reply_filter.[ch] with 2 public functions
> smtp_reply_filter() and smtp_reply_footer() and 95% of the common code
> in internal function smtp_reply_alter.
>
> diff --git a/src/smtpd/smtpd_chat.c b/src/smtpd/smtpd_chat.c
> index 63795b8..af0f69f 100644
> --- a/src/smtpd/smtpd_chat.c
> +++ b/src/smtpd/smtpd_chat.c
> @@ -102,7 +102,7 @@
> #include <maps.h>
> #include <post_mail.h>
> #include <mail_error.h>
> -#include <smtp_reply_footer.h>
> +#include <smtp_reply_filter.h>
>
> /* Application-specific. */
>
> @@ -111,8 +111,9 @@
> #include "smtpd_chat.h"
>
> /*
> - * Reject footer.
> + * Reject replacement and footer.
> */
> +static MAPS *smtpd_rej_flt_maps;
> static MAPS *smtpd_rej_ftr_maps;
>
> #define STR vstring_str
> @@ -128,12 +129,16 @@ void smtpd_chat_pre_jail_init(void)
> msg_panic("smtpd_chat_pre_jail_init: multiple calls");
>
> /*
> - * SMTP server reject footer.
> + * SMTP server reject replacement and footer.
> */
> if (*var_smtpd_rej_ftr_maps)
> smtpd_rej_ftr_maps = maps_create(VAR_SMTPD_REJ_FTR_MAPS,
> var_smtpd_rej_ftr_maps,
> DICT_FLAG_LOCK);
> + if (*var_smtpd_rej_flt_maps)
> + smtpd_rej_flt_maps = maps_create(VAR_SMTPD_REJ_FLT_MAPS,
> + var_smtpd_rej_flt_maps,
> + DICT_FLAG_LOCK);
> }
>
> /* smtp_chat_reset - reset SMTP transaction log */
> @@ -205,7 +210,8 @@ void vsmtpd_chat_reply(SMTPD_STATE *state,
> const char *format, va_list ap)
> char *cp;
> char *next;
> char *end;
> - const char *footer;
> + const char *footer = 0;
> + const char *replacement;
>
> /*
> * Slow down clients that make errors. Sleep-on-anything slows down
> @@ -216,12 +222,19 @@ void vsmtpd_chat_reply(SMTPD_STATE *state,
> const char *format, va_list ap)
>
> vstring_vsprintf(state->buffer, format, ap);
>
> - if ((*(cp = STR(state->buffer)) == '4' || *cp == '5')
> - && ((smtpd_rej_ftr_maps != 0
> + if (*(cp = STR(state->buffer)) == '4' || *cp == '5') {
> + if (smtpd_rej_flt_maps != 0
> + && (replacement = maps_find(smtpd_rej_flt_maps, cp, 0)) != 0)
> + smtp_reply_filter(state->buffer, 0, replacement,
> STR(smtpd_expand_filter),
> + smtpd_expand_lookup, (void *) state);
> + else if (smtpd_rej_ftr_maps != 0
> && (footer = maps_find(smtpd_rej_ftr_maps, cp, 0)) != 0)
> - || *(footer = var_smtpd_rej_footer) != 0))
> - smtp_reply_footer(state->buffer, 0, footer, STR(smtpd_expand_filter),
> - smtpd_expand_lookup, (void *) state);
> + smtp_reply_footer(state->buffer, 0, footer,
> STR(smtpd_expand_filter),
> + smtpd_expand_lookup, (void *) state);
> + if (footer == 0 && *(footer = var_smtpd_rej_footer) != 0)
> + smtp_reply_footer(state->buffer, 0, footer,
> STR(smtpd_expand_filter),
> + smtpd_expand_lookup, (void *) state);
> + }
>
>
> diff --git a/src/global/mail_params.h b/src/global/mail_params.h
> index 1f4c207..2fe0d0d 100644
> --- a/src/global/mail_params.h
> +++ b/src/global/mail_params.h
> @@ -4009,6 +4009,10 @@ extern char *var_smtpd_rej_footer;
> #define DEF_SMTPD_REJ_FTR_MAPS ""
> extern char *var_smtpd_rej_ftr_maps;
>
> +#define VAR_SMTPD_REJ_FLT_MAPS "smtpd_reject_filter_maps"
> +#define DEF_SMTPD_REJ_FLT_MAPS ""
> +extern char *var_smtpd_rej_flt_maps;
> +
>
This patch works as expected, I can replace some hard-coded postfix
responses or add a specific footer to some others. But message
responses in logs are disturbing because it is still the original
hard-coded message. It is because I put my code in smtpd_chat_reply in
the same place than the footer feature.
What do you think of this side effect ? Do you think like me that the
current behavior that is not to log the footer is good for the footer
only ? IMHO the replaced text returned to the client should also be
replaced in the log for consistency and to easier the support of user
problems or statistics about the replied errors.
We have two options : moving the management of
smtpd_reject_filter_maps in check_milter_reply with the need to give a
context into smtpd_chat_reply to not apply smtpd_reject_footer_maps if
a replacement occured, or other option to merge the handling of smtpd
error logging and smtpd error reply in the same place.
Sébastien
