On 29/12/2018 23:20, Wietse Venema wrote:
> Sorry, I did not recognize the diff because all whitespace was using
> UTF8 code points, and I read mail with a text editor that is optimized
> for programing, not for text processing.
>
> After fixing the whitespace:
Thanks for reviewing it further. I've made another attempt below based
on your comments.
>> --- smtpd/smtpd.c.orig 2018-12-28 13:18:55.427162049 +0100
>> +++ smtpd/smtpd.c 2018-12-29 11:25:49.294927986 +0100
>> @@ -5677,6 +5677,8 @@
>> if (var_smtpd_enforce_tls &&
>> !state->tls_context &&
>> (cmdp->flags & SMTPD_CMD_FLAG_PRE_TLS) == 0) {
>> + msg_warn("Missing required STARTTLS command from %s",
>> + state->namaddr);
> 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.
>> - if (cmdp->action(state, argc, argv) != 0)
>> + if (cmdp->action(state, argc, argv) != 0) {
>> state->error_count++;
>> + if (state->error_mask & MAIL_ERROR_PROTOCOL)
>> + msg_info("smtp protocol error in %s from %s",
>> + cmdp->name, state->namaddr);
>> + }
> Unfortunately, once the protocol error bit is set, this will flag
> all subsequent errors as protocol errors, writing O(100) byte records
> and introducing the same logfile flooding opportunity as mentioned
> above.
>
> Even if you log this only once per session, it would blame the wrong
> command (cmdp->name), because the protocol error bit may have been
> set when the client sent an unknown command, and that code path never
> reaches the code fragment shown above.
>
> Wietse
I updated the patch so it logs a generic message just once per session.
I admit it doesn't add much to the info already available in the
disconnect line, but it is an explicit indication of an error in the
session, even when notify_classes are set to not send notifications to
the postmaster.
I am attaching a new diff as a text file, hopefully it will get through
the mail system better.
John
--- smtpd/smtpd.h.orig 2018-12-29 23:49:10.721258930 +0100
+++ smtpd/smtpd.h 2018-12-30 00:26:33.924684042 +0100
@@ -206,6 +206,7 @@
#define SMTPD_FLAG_ILL_PIPELINING (1<<1) /* inappropriate pipelining */
#define SMTPD_FLAG_AUTH_USED (1<<2) /* don't reuse SASL state */
#define SMTPD_FLAG_SMTPUTF8 (1<<3) /* RFC 6531/2 transaction */
+#define SMTPD_FLAG_CMD_BEFORE_STARTTLS (1<<4) /* inappropriate command prior
to STARTTLS */
/* Security: don't reset SMTPD_FLAG_AUTH_USED. */
#define SMTPD_MASK_MAIL_KEEP \
--- smtpd/smtpd.c.orig 2018-12-28 13:18:55.427162049 +0100
+++ smtpd/smtpd.c 2018-12-30 00:33:41.985387307 +0100
@@ -4910,9 +4910,12 @@
* require availability of the cleanup service.
*/
if (state->history != 0 && state->history->argc > threshold) {
- if (SMTPD_STAND_ALONE(state) == 0
- && (state->error_mask & state->notify_mask))
- smtpd_chat_notify(state);
+ if (SMTPD_STAND_ALONE(state) == 0) {
+ if (state->error_mask)
+ msg_info("errors from %s",state->namaddr);
+ if (state->error_mask & state->notify_mask)
+ smtpd_chat_notify(state);
+ }
state->error_mask = 0;
smtpd_chat_reset(state);
}
@@ -5677,6 +5680,12 @@
if (var_smtpd_enforce_tls &&
!state->tls_context &&
(cmdp->flags & SMTPD_CMD_FLAG_PRE_TLS) == 0) {
+ if ((state->flags & SMTPD_FLAG_CMD_BEFORE_STARTTLS) == 0) {
+ msg_warn("Missing required STARTTLS command from %s",
+ state->namaddr);
+ state->flags |= SMTPD_FLAG_CMD_BEFORE_STARTTLS;
+ }
smtpd_chat_reply(state,
"530 5.7.0 Must issue a STARTTLS command first");
state->error_count++;
