Re: smtpd: flags cleanup in mta

[Gilles Chehade]

On Wed, Sep 05, 2018 at 03:04:08PM +0200, Eric Faurot wrote:
> With the recent changes in the smarthost syntax, and the removal of
> the "secure" keyword, it's now possible to clarify the mta code by
> changing the TLS option from a set flags to exclusive values.
> This is far less confusing.
> 
> More cleanup to come in mta_session.c after that.
> 

nice !


> Index: mta.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/mta.c,v
> retrieving revision 1.222
> diff -u -p -r1.222 mta.c
> --- mta.c 22 Aug 2018 10:11:43 -  1.222
> +++ mta.c 5 Sep 2018 12:42:19 -
> @@ -635,6 +635,7 @@ mta_handle_envelope(struct envelope *evp
>   }
>  
>   memset(, 0, sizeof(relayh));
> + relayh.tls = RELAY_TLS_OPPORTUNISTIC;
>   if (smarthost && !text_to_relayhost(, smarthost)) {
>   log_warnx("warn: Failed to parse smarthost %s", smarthost);
>   m_create(p_queue, IMSG_MTA_DELIVERY_TEMPFAIL, 0, 0, -1);
> @@ -1730,10 +1731,9 @@ mta_relay(struct envelope *e, struct rel
>   key.flags |= RELAY_MX;
>   } else {
>   key.domain = mta_domain(e->dest.domain, 0);
> - if (!(relayh->flags & RELAY_STARTTLS))
> - key.flags |= RELAY_TLS_OPTIONAL;
>   }
>  
> + key.tls = relayh->tls;
>   key.flags |= relayh->flags;
>   key.port = relayh->port;
>   key.authlabel = relayh->authlabel;
> @@ -1748,6 +1748,7 @@ mta_relay(struct envelope *e, struct rel
>   r = xcalloc(1, sizeof *r);
>   TAILQ_INIT(>tasks);
>   r->id = generate_uid();
> + r->tls = key.tls;
>   r->flags = key.flags;
>   r->domain = key.domain;
>   r->backupname = key.backupname ?
> @@ -1834,14 +1835,25 @@ mta_relay_to_text(struct mta_relay *rela
>   (void)strlcat(buf, tmp, sizeof buf);
>   }
>  
> - if (relay->flags & RELAY_STARTTLS) {
> - (void)strlcat(buf, sep, sizeof buf);
> - (void)strlcat(buf, "starttls", sizeof buf);
> - }
> -
> - if (relay->flags & RELAY_SMTPS) {
> - (void)strlcat(buf, sep, sizeof buf);
> + (void)strlcat(buf, sep, sizeof buf);
> + switch(relay->tls) {
> + case RELAY_TLS_OPPORTUNISTIC:
> + (void)strlcat(buf, "smtp", sizeof buf);
> + break;
> + case RELAY_TLS_STARTTLS:
> + (void)strlcat(buf, "smtp+tls", sizeof buf);
> + break;
> + case RELAY_TLS_SMTPS:
>   (void)strlcat(buf, "smtps", sizeof buf);
> + break;
> + case RELAY_TLS_NO:
> + if (relay->flags & RELAY_LMTP)
> + (void)strlcat(buf, "lmtp", sizeof buf);
> + else
> + (void)strlcat(buf, "smtp+notls", sizeof buf);
> + break;
> + default:
> + (void)strlcat(buf, "???", sizeof buf);
>   }
>  
>   if (relay->flags & RELAY_AUTH) {
> @@ -1993,6 +2005,11 @@ mta_relay_cmp(const struct mta_relay *a,
>   if (a->domain < b->domain)
>   return (-1);
>   if (a->domain > b->domain)
> + return (1);
> +
> + if (a->tls < b->tls)
> + return (-1);
> + if (a->tls > b->tls)
>   return (1);
>  
>   if (a->flags < b->flags)
> Index: mta_session.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/mta_session.c,v
> retrieving revision 1.109
> diff -u -p -r1.109 mta_session.c
> --- mta_session.c 5 Sep 2018 10:15:41 -   1.109
> +++ mta_session.c 5 Sep 2018 12:42:19 -
> @@ -199,24 +199,23 @@ mta_session(struct mta_relay *relay, str
>  
>   if (relay->flags & RELAY_LMTP)
>   s->flags |= MTA_LMTP;
> - switch (relay->flags & (RELAY_SSL|RELAY_TLS_OPTIONAL)) {
> - case RELAY_SSL:
> - s->flags |= MTA_FORCE_ANYSSL;
> - s->flags |= MTA_WANT_SECURE;
> - break;
> - case RELAY_SMTPS:
> + switch (relay->tls) {
> + case RELAY_TLS_SMTPS:
>   s->flags |= MTA_FORCE_SMTPS;
>   s->flags |= MTA_WANT_SECURE;
>   break;
> - case RELAY_STARTTLS:
> + case RELAY_TLS_STARTTLS:
>   s->flags |= MTA_FORCE_TLS;
>   s->flags |= MTA_WANT_SECURE;
>   break;
> - case RELAY_TLS_OPTIONAL:
> + case RELAY_TLS_OPPORTUNISTIC:
>   /* do not force anything, try tls then smtp */
>   break;
> - default:
> + case RELAY_TLS_NO:
>   s->flags |= MTA_FORCE_PLAIN;
> + break;
> + default:
> + fatalx("bad value for relay->tls: %d", relay->tls);
>   }
>  
>   if (relay->flags & RELAY_BACKUP)
> 

smtpd: flags cleanup in mta

[Eric Faurot]

With the recent changes in the smarthost syntax, and the removal of
the "secure" keyword, it's now possible to clarify the mta code by
changing the TLS option from a set flags to exclusive values.
This is far less confusing.

More cleanup to come in mta_session.c after that.

Eric.

Index: mta.c
===
RCS file: /cvs/src/usr.sbin/smtpd/mta.c,v
retrieving revision 1.222
diff -u -p -r1.222 mta.c
--- mta.c   22 Aug 2018 10:11:43 -  1.222
+++ mta.c   5 Sep 2018 12:42:19 -
@@ -635,6 +635,7 @@ mta_handle_envelope(struct envelope *evp
}
 
memset(, 0, sizeof(relayh));
+   relayh.tls = RELAY_TLS_OPPORTUNISTIC;
if (smarthost && !text_to_relayhost(, smarthost)) {
log_warnx("warn: Failed to parse smarthost %s", smarthost);
m_create(p_queue, IMSG_MTA_DELIVERY_TEMPFAIL, 0, 0, -1);
@@ -1730,10 +1731,9 @@ mta_relay(struct envelope *e, struct rel
key.flags |= RELAY_MX;
} else {
key.domain = mta_domain(e->dest.domain, 0);
-   if (!(relayh->flags & RELAY_STARTTLS))
-   key.flags |= RELAY_TLS_OPTIONAL;
}
 
+   key.tls = relayh->tls;
key.flags |= relayh->flags;
key.port = relayh->port;
key.authlabel = relayh->authlabel;
@@ -1748,6 +1748,7 @@ mta_relay(struct envelope *e, struct rel
r = xcalloc(1, sizeof *r);
TAILQ_INIT(>tasks);
r->id = generate_uid();
+   r->tls = key.tls;
r->flags = key.flags;
r->domain = key.domain;
r->backupname = key.backupname ?
@@ -1834,14 +1835,25 @@ mta_relay_to_text(struct mta_relay *rela
(void)strlcat(buf, tmp, sizeof buf);
}
 
-   if (relay->flags & RELAY_STARTTLS) {
-   (void)strlcat(buf, sep, sizeof buf);
-   (void)strlcat(buf, "starttls", sizeof buf);
-   }
-
-   if (relay->flags & RELAY_SMTPS) {
-   (void)strlcat(buf, sep, sizeof buf);
+   (void)strlcat(buf, sep, sizeof buf);
+   switch(relay->tls) {
+   case RELAY_TLS_OPPORTUNISTIC:
+   (void)strlcat(buf, "smtp", sizeof buf);
+   break;
+   case RELAY_TLS_STARTTLS:
+   (void)strlcat(buf, "smtp+tls", sizeof buf);
+   break;
+   case RELAY_TLS_SMTPS:
(void)strlcat(buf, "smtps", sizeof buf);
+   break;
+   case RELAY_TLS_NO:
+   if (relay->flags & RELAY_LMTP)
+   (void)strlcat(buf, "lmtp", sizeof buf);
+   else
+   (void)strlcat(buf, "smtp+notls", sizeof buf);
+   break;
+   default:
+   (void)strlcat(buf, "???", sizeof buf);
}
 
if (relay->flags & RELAY_AUTH) {
@@ -1993,6 +2005,11 @@ mta_relay_cmp(const struct mta_relay *a,
if (a->domain < b->domain)
return (-1);
if (a->domain > b->domain)
+   return (1);
+
+   if (a->tls < b->tls)
+   return (-1);
+   if (a->tls > b->tls)
return (1);
 
if (a->flags < b->flags)
Index: mta_session.c
===
RCS file: /cvs/src/usr.sbin/smtpd/mta_session.c,v
retrieving revision 1.109
diff -u -p -r1.109 mta_session.c
--- mta_session.c   5 Sep 2018 10:15:41 -   1.109
+++ mta_session.c   5 Sep 2018 12:42:19 -
@@ -199,24 +199,23 @@ mta_session(struct mta_relay *relay, str
 
if (relay->flags & RELAY_LMTP)
s->flags |= MTA_LMTP;
-   switch (relay->flags & (RELAY_SSL|RELAY_TLS_OPTIONAL)) {
-   case RELAY_SSL:
-   s->flags |= MTA_FORCE_ANYSSL;
-   s->flags |= MTA_WANT_SECURE;
-   break;
-   case RELAY_SMTPS:
+   switch (relay->tls) {
+   case RELAY_TLS_SMTPS:
s->flags |= MTA_FORCE_SMTPS;
s->flags |= MTA_WANT_SECURE;
break;
-   case RELAY_STARTTLS:
+   case RELAY_TLS_STARTTLS:
s->flags |= MTA_FORCE_TLS;
s->flags |= MTA_WANT_SECURE;
break;
-   case RELAY_TLS_OPTIONAL:
+   case RELAY_TLS_OPPORTUNISTIC:
/* do not force anything, try tls then smtp */
break;
-   default:
+   case RELAY_TLS_NO:
s->flags |= MTA_FORCE_PLAIN;
+   break;
+   default:
+   fatalx("bad value for relay->tls: %d", relay->tls);
}
 
if (relay->flags & RELAY_BACKUP)
Index: smtpd.h
===
RCS file: /cvs/src/usr.sbin/smtpd/smtpd.h,v