John Fawcett: > On 30/12/2018 01:19, Wietse Venema wrote: > > John Fawcett: > >>> I would not log this for EVERY command. Especially because the > >>> logged text size by far exceeds the command size (each logfile > >>> record takes ~100 bytes, while the client needs to send only four > >>> characters plus CRLF. > >>> > >>> For example, Postfix logs pipelining errors including evidence, but > >>> it logs this at most once per session, to avoid flooding the log > >>> with garbage. > >> I have added a flag similar to the pipelining one, so that the warning > >> is output once only per session no matter how many commands are issued > >> prior to running STARTTLS. > > Looks better, but I wonder if this could be done in a similar way > > as, or combined with, the disconnect statistics. Strawman: > > > > disconnect from host[addr] ehlo=1 mail=0/1 quit=1 commands=3 > > errors=pipelining,plaintext > > > > It puts all the stats in one place, and also minimizes the amount > > of additional logfile space. > > > Here's a revised patch implementing the above logging. > > I did not take out the existing pipelining logging since it provides > additional info over the new error summary in the disconnect line, > though if it's no longer judged useful (postscreen can already handle > unauthorized pipelining) it could be removed.
As a proof of concept this seems to work. As for redundancy, I think it is useful to log the pipelining violation detail once per session because it immediately answers the question 'why' Postfix did something. However, there is a difference between proof of concept and full implementation. For example, if RCPT TO was rejected, then one would expect that 'errors=' always shows some error category. You can already see where this is going. Therefore, at this point it may be better to just log that the client sent plaintext, once per session. I have real work that leaves few cycles for Postfix, and a full implementation of errors= stats may therefore have to wait until after the stable 3.4 release. JFTR, this is what a full implementation would look like. A full implementation would update a new SMTP_STATE violation_mask field for specific violation categories (syntax, pipelining, plaintext, relay, unverified-address, unknown-user, access-deny, dnsbl, tls-handshake, lost-connection, timeout, etc). This has a huge code footprint, because every piece of code that detects a violation will need to set a violation category bit. TBD is which error categories: we don't want too few (then we could just use the existing 'protocol', 'software', 'policy', 'resource', etc. break-down) or too many buckets. For output, the implementation would use str_name_mask(), probably called from a separate function smtpd_format_error_stats(). Wietse