Re: smtpd: improve syntax for relay host

2018-09-02 Thread Eric Faurot
Hi.

Same diff with associated manpage update.
If there is no objection, I'd like to commit this quickly.

Eric.

Index: smtpd.conf.5
===
RCS file: /cvs/src/usr.sbin/smtpd/smtpd.conf.5,v
retrieving revision 1.199
diff -u -p -r1.199 smtpd.conf.5
--- smtpd.conf.51 Sep 2018 19:56:28 -   1.199
+++ smtpd.conf.52 Sep 2018 18:56:44 -
@@ -228,7 +228,38 @@ to advertise during the HELO phase.
 .It Cm host Ar relay-url
 Do not perform MX lookups but relay messages to the relay host described by
 .Ar relay-url .
-If the URL uses TLS, the certificate will be verified by default.
+The format for 
+.Ar relay-url
+is
+.Sm off
+.Op Ar proto No :// Op Ar label No @
+.Ar host Op : Ar port .
+.Sm on
+The following protocols are available:
+.Pp
+.Bl -tag -width "smtp+notls" -compact
+.It smtp
+Normal SMTP session with opportunistic STARTTLS.
+.It smtp+tls
+Normal SMTP session with mandatory STARTTLS.
+.It smtp+notls
+Plain text SMTP session without TLS.
+.It lmtp
+LMTP session.
+.It smtps
+SMTP session with forced TLS on connection.
+.El
+.Pp
+If not specified, the
+.Dq smtp
+protocol is used.
+.Pp
+Specifying an auth label toggles authentication.
+An auth table must also be defined for this action.
+The protocol must explicitely require TLS.
+.Pp
+If TLS is explicitely required, the server certificate
+will be verified by default.
 .It Cm tls no-verify
 Do not require a valid certificate for the specified host.
 .It Cm auth Pf < Ar table Ns >
Index: to.c
===
RCS file: /cvs/src/usr.sbin/smtpd/to.c,v
retrieving revision 1.31
diff -u -p -r1.31 to.c
--- to.c7 Jun 2018 11:31:51 -   1.31
+++ to.c2 Sep 2018 18:56:44 -
@@ -310,15 +310,11 @@ text_to_relayhost(struct relayhost *rela
 * new schemas should be *appended* otherwise the default
 * schema index needs to be updated later in this function.
 */
-   { "smtp://",0   },
+   { "smtp://",RELAY_TLS_OPTIONAL  },
+   { "smtp+tls://",RELAY_STARTTLS  },
+   { "smtp+notls://",  0   },
{ "lmtp://",RELAY_LMTP  },
-   { "smtp+tls://",RELAY_TLS_OPTIONAL  },
-   { "smtps://",   RELAY_SMTPS },
-   { "tls://", RELAY_STARTTLS  },
-   { "smtps+auth://",  RELAY_SMTPS|RELAY_AUTH  },
-   { "tls+auth://",RELAY_STARTTLS|RELAY_AUTH   },
-   { "secure://",  RELAY_SMTPS|RELAY_STARTTLS  },
-   { "secure+auth://", RELAY_SMTPS|RELAY_STARTTLS|RELAY_AUTH }
+   { "smtps://",   RELAY_SMTPS }
};
const char *errstr = NULL;
char   *p, *q;
@@ -341,8 +337,8 @@ text_to_relayhost(struct relayhost *rela
if (strstr(buffer, "://"))
return 0;
 
-   /* no schema, default to smtp+tls:// */
-   i = 2;
+   /* no schema, default to smtp:// */
+   i = 0;
p = buffer;
}
else
@@ -397,10 +393,13 @@ text_to_relayhost(struct relayhost *rela
return 0;
if ((relay->flags & RELAY_LMTP) && (relay->port == 0))
return 0;
-   if (relay->authlabel[0] == '\0' && relay->flags & RELAY_AUTH)
-   return 0;
-   if (relay->authlabel[0] != '\0' && !(relay->flags & RELAY_AUTH))
-   return 0;
+   if (relay->authlabel[0]) {
+   /* disallow auth on non-tls scheme. */
+   if (!(relay->flags & (RELAY_STARTTLS | RELAY_SMTPS)))
+   return 0;
+   relay->flags |= RELAY_AUTH;
+   }
+
return 1;
 }
 



Re: smtpd: improve syntax for relay host

2018-08-30 Thread Gilles Chehade
On Wed, Aug 29, 2018 at 01:56:49PM +0200, Eric Faurot wrote:
> For clarity and consistency, we'd like to change the url-like schemes
> used for specifying smarthost relays in smtpd.conf, to make them match
> what has been set for smtp(1). The proposed changes are as follow:
> 
> - the "+auth" specifier is removed: it is implied by the presence of an
> auth label in the rest of the string
> - "secure://" is removed: use "smtp+tls://" or "smtps://" explicitely
> - "tls://" is removed, and replaced by "smtp+tls://"
> - "smtp://" becomes SMTP with opportunistic STARTTLS: use "smtp+notls://"
> to disable TLS
> - "smtp+tls://" becomes SMTP with mandatory STARTTLS: use "smtp://" for
> opportunistic STARTTLS
> 
> It might look confusing (especially since the current schemes are
> apparently not documented), but in practice, the update process is
> very simple:
> 
>   1) If you have "+auth" just remove it,
>   2) then rewrite the rest as follow:
> 
>  smtp+tls:// -> smtp://
>  smtp:// -> smtp+notls://
>  tls://  -> smtp+tls://
>  smtps://-> no change
>  lmtp:// -> no change
>  secure://   -> choose between smtp+tls:// and smtps://
> 
> For example, when relaying through a smarthost with authentication,
> the change would be:
> 
>   -action "foo" relay host "tls+auth://la...@smtp.example.com" auth 
>   +action "foo" relay host "smtp+tls://la...@smtp.example.com" auth 
> 
> or, when using smtps:
> 
>   -action "foo" relay host "smtps+auth://la...@smtp.example.com" auth 
> 
>   +action "foo" relay host "smtps://la...@smtp.example.com" auth 
> 
> The default remains SMTP with opportunistic STARTTLS, so a rule like
> the following has the same behaviour as before:
> 
>   action "foo" relay host "smtp.example.com"
> 
> Note that there is no impact on incoming or queued mails.  The
> consequences for running with the new schemes without updating the
> config first are:
> 
> - an "smtp://" relay would start to do opportunistic STARTTLS, so at worst
>   mails would be sent over a secure channel instead of plain text.
> - an "smtp+tls://" relay would not fallback to plain text if STARTTLS fails,
>   and the mail will tempfail.
> - in all other cases, the mail will tempfail with a warning.
> 
> Does that look fine?
> 
> 

To me it's the sensible approach, is cleaner too.

We should do it now because people are going to have to change their
config with 6.4 anyways.

A next step, for 6.5 would be to manage and get rid off the auth
keyword within actions by embedding the table name in relay url.


don't forget updating current.html, ok gilles@


> Index: to.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/to.c,v
> retrieving revision 1.31
> diff -u -p -r1.31 to.c
> --- to.c  7 Jun 2018 11:31:51 -   1.31
> +++ to.c  29 Aug 2018 07:32:52 -
> @@ -310,15 +310,11 @@ text_to_relayhost(struct relayhost *rela
>* new schemas should be *appended* otherwise the default
>* schema index needs to be updated later in this function.
>*/
> - { "smtp://",0   },
> + { "smtp://",RELAY_TLS_OPTIONAL  },
> + { "smtp+tls://",RELAY_STARTTLS  },
> + { "smtp+notls://",  0   },
>   { "lmtp://",RELAY_LMTP  },
> - { "smtp+tls://",RELAY_TLS_OPTIONAL  },
> - { "smtps://",   RELAY_SMTPS },
> - { "tls://", RELAY_STARTTLS  },
> - { "smtps+auth://",  RELAY_SMTPS|RELAY_AUTH  },
> - { "tls+auth://",RELAY_STARTTLS|RELAY_AUTH   },
> - { "secure://",  RELAY_SMTPS|RELAY_STARTTLS  },
> - { "secure+auth://", RELAY_SMTPS|RELAY_STARTTLS|RELAY_AUTH }
> + { "smtps://",   RELAY_SMTPS }
>   };
>   const char *errstr = NULL;
>   char   *p, *q;
> @@ -341,8 +337,8 @@ text_to_relayhost(struct relayhost *rela
>   if (strstr(buffer, "://"))
>   return 0;
>  
> - /* no schema, default to smtp+tls:// */
> - i = 2;
> + /* no schema, default to smtp:// */
> + i = 0;
>   p = buffer;
>   }
>   else
> @@ -397,10 +393,13 @@ text_to_relayhost(struct relayhost *rela
>   return 0;
>   if ((relay->flags & RELAY_LMTP) && (relay->port == 0))
>   return 0;
> - if (relay->authlabel[0] == '\0' && relay->flags & RELAY_AUTH)
> - return 0;
> - if (relay->authlabel[0] != '\0' && !(relay->flags & RELAY_AUTH))
> - return 0;
> + if (relay->authlabel[0]) {
> + /* disallow auth on non-tls 

smtpd: improve syntax for relay host

2018-08-29 Thread Eric Faurot
For clarity and consistency, we'd like to change the url-like schemes
used for specifying smarthost relays in smtpd.conf, to make them match
what has been set for smtp(1). The proposed changes are as follow:

- the "+auth" specifier is removed: it is implied by the presence of an
auth label in the rest of the string
- "secure://" is removed: use "smtp+tls://" or "smtps://" explicitely
- "tls://" is removed, and replaced by "smtp+tls://"
- "smtp://" becomes SMTP with opportunistic STARTTLS: use "smtp+notls://"
to disable TLS
- "smtp+tls://" becomes SMTP with mandatory STARTTLS: use "smtp://" for
opportunistic STARTTLS

It might look confusing (especially since the current schemes are
apparently not documented), but in practice, the update process is
very simple:

  1) If you have "+auth" just remove it,
  2) then rewrite the rest as follow:

 smtp+tls:// -> smtp://
 smtp:// -> smtp+notls://
 tls://  -> smtp+tls://
 smtps://-> no change
 lmtp:// -> no change
 secure://   -> choose between smtp+tls:// and smtps://

For example, when relaying through a smarthost with authentication,
the change would be:

  -action "foo" relay host "tls+auth://la...@smtp.example.com" auth 
  +action "foo" relay host "smtp+tls://la...@smtp.example.com" auth 

or, when using smtps:

  -action "foo" relay host "smtps+auth://la...@smtp.example.com" auth 
  +action "foo" relay host "smtps://la...@smtp.example.com" auth 

The default remains SMTP with opportunistic STARTTLS, so a rule like
the following has the same behaviour as before:

  action "foo" relay host "smtp.example.com"

Note that there is no impact on incoming or queued mails.  The
consequences for running with the new schemes without updating the
config first are:

- an "smtp://" relay would start to do opportunistic STARTTLS, so at worst
  mails would be sent over a secure channel instead of plain text.
- an "smtp+tls://" relay would not fallback to plain text if STARTTLS fails,
  and the mail will tempfail.
- in all other cases, the mail will tempfail with a warning.

Does that look fine?


Eric.

Index: to.c
===
RCS file: /cvs/src/usr.sbin/smtpd/to.c,v
retrieving revision 1.31
diff -u -p -r1.31 to.c
--- to.c7 Jun 2018 11:31:51 -   1.31
+++ to.c29 Aug 2018 07:32:52 -
@@ -310,15 +310,11 @@ text_to_relayhost(struct relayhost *rela
 * new schemas should be *appended* otherwise the default
 * schema index needs to be updated later in this function.
 */
-   { "smtp://",0   },
+   { "smtp://",RELAY_TLS_OPTIONAL  },
+   { "smtp+tls://",RELAY_STARTTLS  },
+   { "smtp+notls://",  0   },
{ "lmtp://",RELAY_LMTP  },
-   { "smtp+tls://",RELAY_TLS_OPTIONAL  },
-   { "smtps://",   RELAY_SMTPS },
-   { "tls://", RELAY_STARTTLS  },
-   { "smtps+auth://",  RELAY_SMTPS|RELAY_AUTH  },
-   { "tls+auth://",RELAY_STARTTLS|RELAY_AUTH   },
-   { "secure://",  RELAY_SMTPS|RELAY_STARTTLS  },
-   { "secure+auth://", RELAY_SMTPS|RELAY_STARTTLS|RELAY_AUTH }
+   { "smtps://",   RELAY_SMTPS }
};
const char *errstr = NULL;
char   *p, *q;
@@ -341,8 +337,8 @@ text_to_relayhost(struct relayhost *rela
if (strstr(buffer, "://"))
return 0;
 
-   /* no schema, default to smtp+tls:// */
-   i = 2;
+   /* no schema, default to smtp:// */
+   i = 0;
p = buffer;
}
else
@@ -397,10 +393,13 @@ text_to_relayhost(struct relayhost *rela
return 0;
if ((relay->flags & RELAY_LMTP) && (relay->port == 0))
return 0;
-   if (relay->authlabel[0] == '\0' && relay->flags & RELAY_AUTH)
-   return 0;
-   if (relay->authlabel[0] != '\0' && !(relay->flags & RELAY_AUTH))
-   return 0;
+   if (relay->authlabel[0]) {
+   /* disallow auth on non-tls scheme. */
+   if (!(relay->flags & (RELAY_STARTTLS | RELAY_SMTPS)))
+   return 0;
+   relay->flags |= RELAY_AUTH;
+   }
+
return 1;
 }