Re: [diff] selectable curves in smtpd ?
August 12, 2023 4:34 PM, "Theo Buehler" wrote: > On Sat, Aug 12, 2023 at 02:29:45PM +, gil...@poolp.org wrote: > >> Hello, >> >> Someone asked about selectable curves in the OpenSMTPD portable tracker, >> and it turns out I had a diff for that among a few others. > > Why do they need this? I suspect for the same reason people have needed ciphers selection in the past, being able to comply with the requirements of some certification (iirc, medical mail systems, for example, have strict requirements regarding their setup). Anyways, I've written this a long time ago and I'm providing it in case it's of any interest, feel free to discard.
[diff] selectable curves in smtpd ?
Hello,
Someone asked about selectable curves in the OpenSMTPD portable tracker,
and it turns out I had a diff for that among a few others.
The diff below adds support for the curves keyword in listener and relay
directives,
allowing to specify a curve string suitable for tls_config_set_ecdhecurves(3)
in the
same way ciphers were made selectable.
I also have a couple other diffs which I'll clean and send.
Index: mta.c
===
RCS file: /cvs/src/usr.sbin/smtpd/mta.c,v
retrieving revision 1.245
diff -u -p -u -p -r1.245 mta.c
--- mta.c 31 May 2023 16:51:46 - 1.245
+++ mta.c 12 Aug 2023 14:20:21 -
@@ -476,6 +476,7 @@ mta_setup_dispatcher(struct dispatcher *
struct pki *pki;
struct ca *ca;
const char *ciphers;
+ const char *curves;
uint32_t protos;
if (dispatcher->type != DISPATCHER_REMOTE)
@@ -490,6 +491,12 @@ mta_setup_dispatcher(struct dispatcher *
if (remote->tls_ciphers)
ciphers = remote->tls_ciphers;
if (ciphers && tls_config_set_ciphers(config, ciphers) == -1)
+ fatalx("%s", tls_config_error(config));
+
+ curves = env->sc_tls_curves;
+ if (remote->tls_curves)
+ curves = remote->tls_curves;
+ if (curves && tls_config_set_ecdhecurves(config, curves) == -1)
fatalx("%s", tls_config_error(config));
if (remote->tls_protocols) {
Index: parse.y
===
RCS file: /cvs/src/usr.sbin/smtpd/parse.y,v
retrieving revision 1.292
diff -u -p -u -p -r1.292 parse.y
--- parse.y 10 May 2023 07:19:49 - 1.292
+++ parse.y 12 Aug 2023 14:20:21 -
@@ -125,6 +125,7 @@ static struct listen_opts {
char *pki[PKI_MAX];
int pkicount;
char *tls_ciphers;
+ char *tls_curves;
char *tls_protocols;
char *ca;
uint16_tauth;
@@ -166,7 +167,7 @@ typedef struct {
%token ACTION ADMD ALIAS ANY ARROW AUTH AUTH_OPTIONAL
%token BACKUP BOUNCE BYPASS
-%token CA CERT CHAIN CHROOT CIPHERS COMMIT COMPRESSION CONNECT
+%token CA CERT CHAIN CHROOT CIPHERS COMMIT COMPRESSION CONNECT CURVES
%token DATA DATA_LINE DHE DISCONNECT DOMAIN
%token EHLO ENABLE ENCRYPTION ERROR EXPAND_ONLY
%token FCRDNS FILTER FOR FORWARD_ONLY FROM
@@ -527,6 +528,9 @@ SMTP LIMIT limits_smtp
| SMTP CIPHERS STRING {
conf->sc_tls_ciphers = $3;
}
+| SMTP CURVES STRING {
+ conf->sc_tls_curves = $3;
+}
| SMTP MAX_MESSAGE_SIZE size {
conf->sc_maxsize = $3;
}
@@ -765,6 +769,14 @@ HELO STRING {
dsp->u.remote.tls_ciphers = $2;
}
+| CURVES STRING {
+ if (dsp->u.remote.tls_curves) {
+ yyerror("curves already specified for this dispatcher");
+ YYERROR;
+ }
+
+ dsp->u.remote.tls_curves = $2;
+}
| PROTOCOLS STRING {
if (dsp->u.remote.tls_protocols) {
yyerror("protocols already specified for this dispatcher");
@@ -2329,6 +2341,13 @@ opt_if_listen : INET4 {
}
listen_opts.tls_ciphers = $2;
}
+ | CURVES STRING {
+ if (listen_opts.tls_curves) {
+ yyerror("curves already specified");
+ YYERROR;
+ }
+ listen_opts.tls_curves = $2;
+ }
| PROTOCOLS STRING {
if (listen_opts.tls_protocols) {
yyerror("protocols already specified");
@@ -2657,6 +2676,7 @@ lookup(char *s)
{ "commit", COMMIT },
{ "compression",COMPRESSION },
{ "connect",CONNECT },
+ { "curves", CURVES },
{ "data", DATA },
{ "data-line", DATA_LINE },
{ "dhe",DHE },
@@ -3251,6 +3271,11 @@ create_if_listener(struct listen_opts *l
if (lo->pkicount == 0 && lo->ssl)
fatalx("invalid listen option: pki required for tls/smtps");
+ if (lo->tls_ciphers && !lo->ssl)
+ fatalx("invalid listen option: ciphers requires tls/smtps");
+ if (lo->tls_curves && !lo->ssl)
+ fatalx("invalid listen option: curves requires tls/smtps");
+
flags = lo->flags;
if (lo->port) {
@@ -3324,6 +3349,11 @@ config_listener(struct listener *h, str
fatal("strdup");
}
+ if (lo->tls_curves != NULL &&
+ (h->tls_curves = strdup(lo->tls_curves)) == NULL) {
+ fatal("strdup");
+ }
+
if (lo->tls_protocols != NULL &&
(h->tls_protocols = strdup(lo->tls_protocols)) == NULL) {
fatal("strdup");
@@
Re: smtpd: allow arguments on NOOP
June 23, 2023 11:58 AM, "Omar Polo" wrote:
> another diff from the -portable repo:
>
> https://github.com/OpenSMTPD/OpenSMTPD/pull/1150
>
> per rfc-5321 § 4.1.1.9 the NOOP command allows optionally one argument
> that we SHOULD ignore.
>
> The original diff set the check function in the commands table to NULL
> because NOOP is not a phase that can have filters. However, I prefer
> to stay on the safe side and add a smtp_check_noop() function.
> Alternatively, we could allow a NULL check function in
> smtp_session_imsg().
>
> the rfc specifies only one optional string, while here for semplicity
> it's relaxed to allow anything.
>
> diff /usr/src
> commit - 8def1c1c2777f0b5175283f8116e1eaab1f1962a
> path + /usr/src
> blob - 1686f03e96deeb5e6ea8b065456e04c27c752c8c
> file + usr.sbin/smtpd/smtp_session.c
> --- usr.sbin/smtpd/smtp_session.c
> +++ usr.sbin/smtpd/smtp_session.c
> @@ -212,6 +212,7 @@ static int smtp_check_noparam(struct smtp_session *,
> static int smtp_check_mail_from(struct smtp_session *, const char *);
> static int smtp_check_rcpt_to(struct smtp_session *, const char *);
> static int smtp_check_data(struct smtp_session *, const char *);
> +static int smtp_check_noop(struct smtp_session *, const char *);
> static int smtp_check_noparam(struct smtp_session *, const char *);
>
> static void smtp_filter_phase(enum filter_phase, struct smtp_session *, const
> char *);
> @@ -276,7 +277,7 @@ static struct {
> { CMD_DATA, FILTER_DATA, "DATA", smtp_check_data, smtp_proceed_data },
> { CMD_RSET, FILTER_RSET, "RSET", smtp_check_rset, smtp_proceed_rset },
> { CMD_QUIT, FILTER_QUIT, "QUIT", smtp_check_noparam, smtp_proceed_quit },
> - { CMD_NOOP, FILTER_NOOP, "NOOP", smtp_check_noparam, smtp_proceed_noop },
> + { CMD_NOOP, FILTER_NOOP, "NOOP", smtp_check_noop, smtp_proceed_noop },
> { CMD_HELP, FILTER_HELP, "HELP", smtp_check_noparam, smtp_proceed_help },
> { CMD_WIZ, FILTER_WIZ, "WIZ", smtp_check_noparam, smtp_proceed_wiz },
> { CMD_COMMIT, FILTER_COMMIT, ".", smtp_check_noparam, smtp_proceed_commit },
> @@ -1343,8 +1344,8 @@ smtp_command(struct smtp_session *s, char *line)
> break;
>
> case CMD_NOOP:
> - if (!smtp_check_noparam(s, args))
> - break;
> + if (!smtp_check_noop(s, args))
> + break;
> smtp_filter_phase(FILTER_NOOP, s, NULL);
> break;
>
> @@ -1631,6 +1632,12 @@ smtp_check_noparam(struct smtp_session *s, const char
> }
>
> static int
> +smtp_check_noop(struct smtp_session *s, const char *args)
> +{
> + return 1;
> +}
> +
> +static int
> smtp_check_noparam(struct smtp_session *s, const char *args)
> {
> if (args != NULL) {
This reads fine and you did well adding an smtp_check_noop() because it leaves
room
for hooking this command in filters which is something OpenSMTPD could benefit
from
as it would allow a filter to detect people doing NOOP loops and kick them.
Just my 2cents
Re: smtpd-filters: swap link-auth fields
Just released a new filter-rspamd with your diff, thanks > On 14 Jun 2023, at 19:23, Omar Polo wrote: > > Hello, > > the `link-auth' event hash the user first and the result of the > operation after; this breaks when a username has a '|' character in > it. Since this is triggered by the `auth login' command, anyone could > send a user with a '|' and, depending on the filter used, make smtpd > exit. (if the filter dies, smtpd does too) > > This was reported on the OpenSMTPD-portable github repository with > Gilles' opensmtpd-filter-rspamd: > >https://github.com/OpenSMTPD/OpenSMTPD/issues/1213 > > Diff below is straightforward and includes the documentation changes. > I believe link-auth was forgotten in revision 1.61 of lka_filter.c > when the mail-from/rcpt-to events got their fields swapped. > > For opensmtpd-filter-rspamd I have a corresponding diff that I'll send > to Gilles as it is off-topic for tech@, but here it is too if you want > to play with it: > >https://paste.omarpolo.com/9jtli2w > > To reproduce: (there may be quicker ways, this is just the first i > found) > ># pkg_add rspamd opensmtpd-filter-rspamd ># rcctl enable rspamd ># rcctl start rspamd > > add the rspamd filter to /etc/mail/smtpd.conf > >filter "rspamd" proc-exec "filter-rspamd" >listen on lo0 smtps pki localhost auth filter "rspamd" > > and try to do a login: > >$ nc -c -Tnoverify localhost 465 >helo localhost >auth login >b3xw >MTMyNA== > > > Thanks, > > Omar Polo > > > diff /usr/src > commit - 66c6b79616659a94b04092c9f103e3aa29809704 > path + /usr/src > blob - 0c63657be21352fb1f060505250f7a9ef4fc8d8c > file + usr.sbin/smtpd/lka_filter.c > --- usr.sbin/smtpd/lka_filter.c > +++ usr.sbin/smtpd/lka_filter.c > @@ -24,7 +24,7 @@ > #include "smtpd.h" > #include "log.h" > > -#definePROTOCOL_VERSION"0.6" > +#definePROTOCOL_VERSION"0.7" > > struct filter; > struct filter_session; > @@ -1461,7 +1461,7 @@ lka_report_smtp_link_auth(const char *direction, struc >fs->username = xstrdup(username); >} >report_smtp_broadcast(reqid, direction, tv, "link-auth", "%s|%s\n", > -username, result); > +result, username); > } > > void > blob - 313404c111c77b099b3855f43252c26877874b17 > file + usr.sbin/smtpd/smtpd-filters.7 > --- usr.sbin/smtpd/smtpd-filters.7 > +++ usr.sbin/smtpd/smtpd-filters.7 > @@ -271,12 +271,9 @@ This event is generated upon disconnection of the clie > the cipher suite used by the session and the cipher strength in bits. > .It Ic link-disconnect > This event is generated upon disconnection of the client. > -.It Ic link-auth : Ar username result > +.It Ic link-auth : Ar result username > This event is generated upon an authentication attempt by the client. > .Pp > -.Ar username > -contains the username used for the authentication attempt. > -.Pp > .Ar result > contains the string > .Dq pass , > @@ -284,6 +281,9 @@ depending on the result of the authentication attempt. > or > .Dq error > depending on the result of the authentication attempt. > +.Pp > +.Ar username > +contains the username used for the authentication attempt. > .It Ic tx-reset : Op message-id > This event is generated when a transaction is reset. > .Pp
Re: smtpd: nits to reduce the diff with -portable
May 15, 2023 3:34 PM, "Todd C. Miller" wrote: > On Mon, 15 May 2023 13:54:35 +0200, Omar Polo wrote: > >> almost always (cast)var. I've adjusted the spacing in the line I was >> touching, grepping for common types I could only find one instance of >> a '(long long) src' in envelope.c which I'm not addressing here. > > OK millert@. It would be nice to get these changes in portable > as well to avoid gratuitous differences. > FYI, I've granted Omar commit access to the portable repo so he can make syncs on his own and limit differences with OpenBSD.
Re: smtpd: move authentication to table backends
October 11, 2021 4:44 PM, gil...@poolp.org wrote: > October 8, 2021 11:34 PM, "aisha" wrote: > >> Hi all, >> I am still working on the table-procexec for opensmtpd >> and while there, I was thinking of how to do authentication >> using LDAP, which the current table-ldap from ports does not >> support. >> The primary reason for that, I believe, is that LDAP >> authentication should be done by bind and not by returning >> the userPassword and us doing the authentication with >> crypt_checkpass. That kind of defeats one of the uses of LDAP. >> >> Here I've added a patch which pushes the authentication step >> to the table backend and it only returns the final AUTH/NOAUTH >> kind of values. >> >> While here, I also made another small change with mailaddrmap, >> where instead of returning ALL possible aliases that a user >> may use, we now pass the current mailaddr to the table, so >> it can now return a smaller set of addresses. >> >> It should not affect any workflow, so testing from others >> would be appreciated. >> >> Cheers, >> Aisha > > Hello, > > I understand what you're trying to achieve but I don't think this is > the right way to achieve that. > > First, the lookup operation is a key-value mapping returning a value > that the daemon gets to decide what to do with. You're trying to fit > K_CREDENTIALS in it but it doesn't work that way: it takes a key and > an additional parameter (the password) so the table can do something > with it and return a decision. This is why your diff has lookup take > a parameter that's used by none of the lookups, except K_CREDENTIALS > which is handled as particular case. > > I think the proper way to implement is to have a low level operation > that is specifically meant to take a key, a parameter then let table > do whatever it wants and return a boolean. > Today, there's: int table_match(struct table *table, enum table_service kind, const char *key) that's used to attempt matching a key in a table assumed to hold list: match from src [...] Maybe this should be extended similarly to this: int table_match(struct table *table, enum table_service kind, const char *key, const char *value) K_SOURCE match on a list, key is not set: table_match(table, K_SOURCE, NULL, "192.168.0.1"); K_CREDENTIALS match on a mapping, key is used to resolve: table_match(table, K_CREDENTIALS, "gilles", "ilovecandies"); This is just an idea, but I still think this should be done after proc-exec :-)
Re: smtpd: move authentication to table backends
October 8, 2021 11:34 PM, "aisha" wrote: > Hi all, > I am still working on the table-procexec for opensmtpd > and while there, I was thinking of how to do authentication > using LDAP, which the current table-ldap from ports does not > support. > The primary reason for that, I believe, is that LDAP > authentication should be done by bind and not by returning > the userPassword and us doing the authentication with > crypt_checkpass. That kind of defeats one of the uses of LDAP. > > Here I've added a patch which pushes the authentication step > to the table backend and it only returns the final AUTH/NOAUTH > kind of values. > > While here, I also made another small change with mailaddrmap, > where instead of returning ALL possible aliases that a user > may use, we now pass the current mailaddr to the table, so > it can now return a smaller set of addresses. > > It should not affect any workflow, so testing from others > would be appreciated. > > Cheers, > Aisha > Hello, I understand what you're trying to achieve but I don't think this is the right way to achieve that. First, the lookup operation is a key-value mapping returning a value that the daemon gets to decide what to do with. You're trying to fit K_CREDENTIALS in it but it doesn't work that way: it takes a key and an additional parameter (the password) so the table can do something with it and return a decision. This is why your diff has lookup take a parameter that's used by none of the lookups, except K_CREDENTIALS which is handled as particular case. I think the proper way to implement is to have a low level operation that is specifically meant to take a key, a parameter then let table do whatever it wants and return a boolean. Then I don't think this should be done before proc-exec because it's much easier than breaking the current API.
Re: smtpd: srs and ruleset evaluation
Hello,
As discussed, this looks correct to me
> On 22 Sep 2021, at 15:46, Eric Faurot wrote:
>
> Hi.
>
> A user reported that decoded SRS addresses are not correctly evaluated
> against the ruleset. That's because the ruleset always matches against
> the expanded address ("dest") and not the original address ("rcpt").
> This diff should fix it.
>
> Eric.
>
>
> Index: lka_session.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/lka_session.c,v
> retrieving revision 1.95
> diff -u -p -r1.95 lka_session.c
> --- lka_session.c 14 Jun 2021 17:58:15 - 1.95
> +++ lka_session.c 21 Sep 2021 19:21:18 -
> @@ -280,19 +280,19 @@ lka_expand(struct lka_session *lks, stru
> /* handle SRS */
> if (env->sc_srs_key != NULL &&
> ep.sender.user[0] == '\0' &&
> - (strncasecmp(ep.rcpt.user, "SRS0=", 5) == 0 ||
> - strncasecmp(ep.rcpt.user, "SRS1=", 5) == 0)) {
> - srs_decoded = srs_decode(mailaddr_to_text());
> + (strncasecmp(ep.dest.user, "SRS0=", 5) == 0 ||
> + strncasecmp(ep.dest.user, "SRS1=", 5) == 0)) {
> + srs_decoded = srs_decode(mailaddr_to_text());
> if (srs_decoded &&
> - text_to_mailaddr(, srs_decoded)) {
> - /* flag envelope internal and override rcpt */
> + text_to_mailaddr(, srs_decoded)) {
> + /* flag envelope internal and override dest */
> ep.flags |= EF_INTERNAL;
> - xn->u.mailaddr = ep.rcpt;
> + xn->u.mailaddr = ep.dest;
> lks->envelope = ep;
> }
> else {
> log_warn("SRS failed to decode: %s",
> - mailaddr_to_text());
> + mailaddr_to_text());
> }
> }
>
>
Re: [diff] src/usr.sbin/smtpd: table_diff lacks some lookup kinds
August 29, 2021 10:16 PM, gil...@poolp.org wrote:
> Hellow,
>
> The K_STRING and K_REGEX lookup kinds are missing from table_db even though
> nothing prevents
> them from working technically. The following diff is enough to allow db
> tables to be used on
> regex or string contexts.
>
> Index: table_db.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/table_db.c,v
> retrieving revision 1.22
> diff -u -p -r1.22 table_db.c
> --- table_db.c 23 Jan 2021 16:11:11 - 1.22
> +++ table_db.c 29 Aug 2021 20:08:30 -
> @@ -55,7 +55,9 @@ static char *table_db_get_entry_match(vo
>
> struct table_backend table_backend_db = {
> "db",
> -
> K_ALIAS|K_CREDENTIALS|K_DOMAIN|K_NETADDR|K_USERINFO|K_SOURCE|K_MAILADDR|K_ADDRNAME|K_MAILADDRMAP,
> + K_ALIAS|K_CREDENTIALS|K_DOMAIN|K_NETADDR|K_USERINFO|
> + K_SOURCE|K_MAILADDR|K_ADDRNAME|K_MAILADDRMAP|K_RELAYHOST|
> + K_STRING|K_REGEX,
> table_db_config,
> NULL,
> NULL,
the complete diff would be better:
Index: table_db.c
===
RCS file: /cvs/src/usr.sbin/smtpd/table_db.c,v
retrieving revision 1.22
diff -u -p -r1.22 table_db.c
--- table_db.c 23 Jan 2021 16:11:11 - 1.22
+++ table_db.c 1 Sep 2021 11:19:02 -
@@ -55,7 +55,9 @@ static char *table_db_get_entry_match(vo
struct table_backend table_backend_db = {
"db",
-
K_ALIAS|K_CREDENTIALS|K_DOMAIN|K_NETADDR|K_USERINFO|K_SOURCE|K_MAILADDR|K_ADDRNAME|K_MAILADDRMAP,
+ K_ALIAS|K_CREDENTIALS|K_DOMAIN|K_NETADDR|K_USERINFO|
+ K_SOURCE|K_MAILADDR|K_ADDRNAME|K_MAILADDRMAP|K_RELAYHOST|
+ K_STRING|K_REGEX,
table_db_config,
NULL,
NULL,
@@ -72,7 +74,8 @@ static struct keycmp {
} keycmp[] = {
{ K_DOMAIN, table_domain_match },
{ K_NETADDR, table_netaddr_match },
- { K_MAILADDR, table_mailaddr_match }
+ { K_MAILADDR, table_mailaddr_match },
+ { K_REGEX, table_regex_match },
};
struct dbhandle {
[diff] src/usr.sbin/smtpd: table_diff lacks some lookup kinds
Hellow,
The K_STRING and K_REGEX lookup kinds are missing from table_db even though
nothing prevents
them from working technically. The following diff is enough to allow db tables
to be used on
regex or string contexts.
Index: table_db.c
===
RCS file: /cvs/src/usr.sbin/smtpd/table_db.c,v
retrieving revision 1.22
diff -u -p -r1.22 table_db.c
--- table_db.c 23 Jan 2021 16:11:11 - 1.22
+++ table_db.c 29 Aug 2021 20:08:30 -
@@ -55,7 +55,9 @@ static char *table_db_get_entry_match(vo
struct table_backend table_backend_db = {
"db",
-
K_ALIAS|K_CREDENTIALS|K_DOMAIN|K_NETADDR|K_USERINFO|K_SOURCE|K_MAILADDR|K_ADDRNAME|K_MAILADDRMAP,
+ K_ALIAS|K_CREDENTIALS|K_DOMAIN|K_NETADDR|K_USERINFO|
+ K_SOURCE|K_MAILADDR|K_ADDRNAME|K_MAILADDRMAP|K_RELAYHOST|
+ K_STRING|K_REGEX,
table_db_config,
NULL,
NULL,
Re: DANE in libressl?
> On 29 Aug 2021, at 16:14, Peter J. Philipp wrote: > > On Sun, Aug 29, 2021 at 07:16:20AM -0600, Theo de Raadt wrote: >> Is there a strong reason why this has to be in that specific library? > > Not really. I did see gnutls has dane functions and openssl has them too. > I can stick to just rolling the needed functionality in the syslogd. > > Noone out there is doing this already right? > Hello, I had started working on a standalone dane resolver based upon asr but I decided not to move it forward: OpenSSL has an interface for DANE and !OpenBSD projects are more likely to implement that interface, so I thought my plan of a standalone implementation would be inferior to a LibreSSL implementation that could be picked by ports and a libtls interface that could be picked by base daemons. I don’t have much code but I can share if you’re still interested.
Re: add table_procexec in smtpd
June 22, 2021 11:14 PM, "Aisha Tammy" wrote: >> [...] >> >> WRT to handshake, it has multiple uses ranging from ensuring the addons get >> their configuration >> from the daemon to synchronising the daemon start with addons readiness. >> Remember, we didn’t have this with filters and realised we couldn’t do >> without, it is the same for >> tables, they need to get their “table name” at start so we need the daemon >> to pass them, and we >> also need the daemon to not start using them before they are ready. > > I am unsure what you mean by a handshake. > sure, so let's look at procexec for filters: - when the server starts, it forks the filters and begins a handshake with each of them, emitting the following (for example): config|smtpd-version|6.6.1 config|smtp-session-timeout|300 config|subsystem|smtp-in config|ready - when the filter receives the last line, it knows the server is done and it is its turn, it emits the following: register|report|smtp-in|link-connect register|ready - at this point the handshake is over, the server is ready and the filter is ready too, they are both in a known state and synchronised before data flows to the filter. The procexec tables should have a similar handshake to allow the server to pass them information and make sure they are synchronised. The only difference with filters would be that table would have a different "register"" line in the handshake but the config part should be similar. This would an addon to implement both a filter and a table by registering for filtering, and providing a lookup backend for example. > Would something like the following be good - on exec the table process has to > print out a string > "TABLE-READY" which ensures us that the process is ready. > Almost, on exec the daemon prints the config bits (exactly like it does for filter), then it reads from the table backend for a register|ready, but yes you have the idea. > I am not exactly sure how I would implement this, my guess would be to use > event(3) with EV_READ on > backend_r (or something like that). > I haven't looked at this code in over a year now so I'm not sure what the right way of doing it is on top of my head, but looking at how filters are handled is a good startint point.
Re: add table_procexec in smtpd
Re-sending, I forgot to cc: aisha & tech: > On 12 Jun 2021, at 22:47, Gilles CHEHADE wrote: > >> >> On 12 Jun 2021, at 15:15, Eric Faurot wrote: >> >> On Wed, Jun 09, 2021 at 05:41:36PM -0400, Aisha Tammy wrote: >>> Hi, >>> Here is the updated diff, which removes table_proc and adds table_procexec >>> as the default backend when no backend name matches. >>> >> >> Hi. >> >> I'm not opposed to the idea, but I have a couple of comments: >> >> First, if the two implementations are not going to coexists, >> we can just replace table_proc.c. > > True, though proc-exec was the name used for filters so it may be a good to > unify and drop the legacy “proc” name. > This will be hidden to users so quite frankly it’s a matter of dev preference. > > >> Second, since the goal is to expose the protocol directly, instead of >> relying on wrappers (through opensmtpd-extras api), we should take >> time to refine it properly before, and avoid having to bump it every >> other day. For example, I don't see the point of adding timestamps in >> the request. Do we want to be case-sensitive on the commands? Do we >> need some kind of handshake? Also, there can be long-term plan to use >> the same process for different tables, or even for other backends. > > I’m unsure I understand your point: > > The protocol is based on the filter protocol, follows the same logic and line > header to solve the same issues, this is precisely so that there’s no need to > bump every other day as we already figured what was needed for third party > adding to interoperate with smtpd. > This also has the advantage that you can have a single parser handle these > different API instead of having a filter protocol parser, a table protocol > parser (and maybe in the future a queue protocol parser… etc). > > WRT timestamps there were many uses for them ranging from timeout detection > both in daemon / add-ons, profiling, logging the time an event was generated > vs processes overhead, etc… > It allowed ensuring that all addons handling the same event have the exact > same timestamp associated to the event. > > WRT to case-sensitivity, I don’t recall using upper-case myself, to me the > protocol is case-sensitive and as far as I can remember I always used > lowercase :-/ > I’m all for lowering case here. > > WRT to handshake, it has multiple uses ranging from ensuring the addons get > their configuration from the daemon to synchronising the daemon start with > addons readiness. > Remember, we didn’t have this with filters and realised we couldn’t do > without, it is the same for tables, they need to get their “table name” at > start so we need the daemon to pass them, and we also need the daemon to not > start using them before they are ready. > > >> Finally the implementation could be factorized a bit, but that's a >> detail at this time. I think the close operation (is it really useful >> anyway?) >> should use fclose() instead of kill(), and maybe wait() too? > > The implementation can probably be improved, this was a PoC that allowed me > to write various table backends but it was never meant to be committed in the > first place.
Re: add table_procexec in smtpd
> On 12 Jun 2021, at 18:57, Aisha Tammy wrote: > > On 6/12/21 9:15 AM, Eric Faurot wrote: >> On Wed, Jun 09, 2021 at 05:41:36PM -0400, Aisha Tammy wrote: >>> Hi, >>> Here is the updated diff, which removes table_proc and adds >>> table_procexec as the default backend when no backend name matches. >>> >> Hi. >> >> I'm not opposed to the idea, but I have a couple of comments: >> >> First, if the two implementations are not going to coexists, >> we can just replace table_proc.c. > Sounds good with me :D >> >> Second, since the goal is to expose the protocol directly, instead of >> relying on wrappers (through opensmtpd-extras api), we should take >> time to refine it properly before, and avoid having to bump it every >> other day. For example, I don't see the point of adding timestamps in >> the request. > My WIP LDAP wrapper does not use the timestamps. I have not been privy to the > historical context of the development of this protocol, so I do not know why > it exists. > I am fine with removing it. I’m unsure removing them would be a good idea, not only it wouldn’t bring any benefit but it would prevent tracking the time an event was generated in the daemon from a table backend and would make the protocol diverge from the filters protocol while they are the same protocol as of today and could use the same parser with different handler functions based on the event field. >> Do we want to be case-sensitive on the commands? > I reused the current table_service_name function present in table.c and hence > set it to all-caps. I think keeping it such is not an issue but I don't care > if we move to lowercase. > I would prefer being case sensitive, so as not to overly-complicate our > checks. I agree. >> Do we need some kind of handshake? > I do not see a need for this between the table-open and the table-process. > The table-process may do handshakes (or whatever) in its backend, which we > should not be worrying about. Maybe I misunderstood but the handshake is very much needed between the daemon and non-builtin tables, similarly to filters. This is what guarantees that the daemon doesn’t start using a table that’s not ready but also what allows passing daemon informations to table backends so they build their initial state. >> Also, there can be long-term plan to use >> the same process for different tables, or even for other backends. >> >> Finally the implementation could be factorized a bit, but that's a >> detail at this time. I think the close operation (is it really useful >> anyway?) >> should use fclose() instead of kill(), and maybe wait() too? > I agree, I am not sure if the table-close is very useful. I am also fine with > moving it to fclose, though I don't know how to manually close a table >.< > > BTW, can I remove the table_check function? I haven't seen a use for it even > after scrounging around a bit. > > Unless any comments, I'll send the next patch (soonish) with fclose and > timestamps, table_check removed. >
Re: add table_procexec in smtpd
> On 9 Jun 2021, at 17:13, Aisha Tammy wrote: > > > > On 6/9/21 10:34 AM, Gilles CHEHADE wrote: >> >>> On 9 Jun 2021, at 15:47, Aisha Tammy wrote: >>> >>> On 6/9/21 5:19 AM, Gilles CHEHADE wrote: >>>> Hi, >>>> >>>> I wrote table_procexec (despite the copyright which I copy-pasted and >>>> forgot to replace author) so just providing a bit of insight: >>> Ah, I did not know that. I will fix the header in the next patch >> No problem, here’s a write-up for reference: >> >> https://poolp.org/posts/2020-05-28/may-2020-opensmtpd-6.7.1p1-release-table-procexec-and-many-pocs/ > Yes, this was very helpful and was also where I started from :D >> >>>> table_procexec was written as a proof of concept for a new table protocol >>>> inspired by the filter protocol to make it easier to write privsep table >>>> backends using any language or library available without requiring >>>> dependencies in the daemon. >>>> >>>> I implemented it as a table backend because it was the easiest way to show >>>> a working implementation of the protocol without making changes in the >>>> daemon, >>>> the table_procexec backend sits in between the daemon and “new” tables to >>>> translate old protocol queries into new protocol queries and new protocol >>>> results into old protocol results, >>>> but the intent was to move the underlying implementation of the table API >>>> to the protocol and not retain this table proxy. >>> Do you mean that ALL table backends should be replaced by the new model? >>> Yes, it should be possible to do that but I don't know if that should be >>> done simultaneously as introducing procexec. >> Builtin tables within the daemon do not need an update because they are >> built in the lookup process and do not rely on IPC. >> >> Table backends from opensmtpd-extras would all need an update, yes, but the >> change only affects the protocol and not the interface so the change doesn’t >> happen in the backends but in the api library that abstracts communication >> with the daemon, backends just need to be relinked to the new library, since >> the opensmtpd-extras are expected to match specific opensmtpd releases (no >> backwards compatibility) and they are rebuilt whenever a package is created, >> this isn’t as big and hurtful as it looks. >> >> I agree that maybe this should not be done simultaneously to introducing >> procexec but I still don’t think the way it is introduced here is the proper >> way: >> >> Ultimately you want to be able to do: >> >> table foobar mytable:/etc/smtpd/mytable.conf >> >> and not be aware that there’s a table-procexec or a procexec protocol >> ensuring communication with the daemon. >> >> The fact that you introduce “procexec” in a way that requires it to appear >> in the config will make this very hard to hide/remove/change in the future >> as people will start relying on it. > Ah, this makes it a lot more clearer on what should be done. Now I can work > towards this in a better fashion. >> >> Maybe the simplest way is to temporarily introduce procexec as a builtin >> table backend, like you did, but do the necessary work in parse.y and >> table.c to detect that a table backend in smtpd.conf is meant to be executed >> by table-procexec so the daemon can do it transparently. > This might be hard to concurrently with having table_proc present, as it does > exactly this - use table_proc backend if the backend name is not present in > the table names. > So what I could do is to remove table_proc and introduce table_procexec and > simultaneously update the opensmtpd-extras port to use table_procexec API. Yes, table_proc becomes 100% useless once a switch occurs to table_procexec, I see no downside in killing it in favour of table_procexec as long as opensmtpd-extras is updated too so the backends remain available. I have _never_ had any question regarding table_proc, I doubt anyone besides eric@ and I ever wrote custom backends that didn’t end up in -extras so … -extras is probably the only user of table_proc :-) > Does that sound like a feasible approach? > I'll send an updated diff soonish. To me this sounds like the most sensible approach to bring procexec support to smtpd in a way that’s future-proof and that doesn’t break current features. Someone else should comment but this would be how I’d do it Gilles
Re: add table_procexec in smtpd
> On 9 Jun 2021, at 15:47, Aisha Tammy wrote: > > On 6/9/21 5:19 AM, Gilles CHEHADE wrote: >> Hi, >> >> I wrote table_procexec (despite the copyright which I copy-pasted and forgot >> to replace author) so just providing a bit of insight: > Ah, I did not know that. I will fix the header in the next patch No problem, here’s a write-up for reference: https://poolp.org/posts/2020-05-28/may-2020-opensmtpd-6.7.1p1-release-table-procexec-and-many-pocs/ >> table_procexec was written as a proof of concept for a new table protocol >> inspired by the filter protocol to make it easier to write privsep table >> backends using any language or library available without requiring >> dependencies in the daemon. >> >> I implemented it as a table backend because it was the easiest way to show a >> working implementation of the protocol without making changes in the daemon, >> the table_procexec backend sits in between the daemon and “new” tables to >> translate old protocol queries into new protocol queries and new protocol >> results into old protocol results, >> but the intent was to move the underlying implementation of the table API to >> the protocol and not retain this table proxy. > Do you mean that ALL table backends should be replaced by the new model? Yes, > it should be possible to do that but I don't know if that should be done > simultaneously as introducing procexec. Builtin tables within the daemon do not need an update because they are built in the lookup process and do not rely on IPC. Table backends from opensmtpd-extras would all need an update, yes, but the change only affects the protocol and not the interface so the change doesn’t happen in the backends but in the api library that abstracts communication with the daemon, backends just need to be relinked to the new library, since the opensmtpd-extras are expected to match specific opensmtpd releases (no backwards compatibility) and they are rebuilt whenever a package is created, this isn’t as big and hurtful as it looks. I agree that maybe this should not be done simultaneously to introducing procexec but I still don’t think the way it is introduced here is the proper way: Ultimately you want to be able to do: table foobar mytable:/etc/smtpd/mytable.conf and not be aware that there’s a table-procexec or a procexec protocol ensuring communication with the daemon. The fact that you introduce “procexec” in a way that requires it to appear in the config will make this very hard to hide/remove/change in the future as people will start relying on it. Maybe the simplest way is to temporarily introduce procexec as a builtin table backend, like you did, but do the necessary work in parse.y and table.c to detect that a table backend in smtpd.conf is meant to be executed by table-procexec so the daemon can do it transparently. This way, if or when the table API relies on the procexec protocol instead of the old protocol, the table-procexec layer can be removed and people will not see a difference. >> Not my call but I don’t think what you’re proposing is the proper way to >> integrate it. >> >> There are a few downsides in plugging it this way but the most annoying one >> is that instead of starting a table with a configuration parameter, this is >> tricking it into executing the table_procexec and using the configuration >> path as the path to the “new” table, which means that you have no way to >> provide the new table with a configuration for itself. > I don't know if this is true. I was able to provide parameters to the > procexec executable via > Yes, it works (and is how I did my testing), but this is kind of abusing the mechanism it is built upon. In theory you should provide the backend + a configuration file path: table aliases aliases_procexec:/path/to/config not: table aliases "proc-exec:/usr/local/bin/aliases_procexec root.t...@bsd.ac" What you did works but is not how it is intended to work and will be at odds with other backends that expect a config file or a table mapping. I think your diff is very close to something nice but it lacks some adaptations in parse.y / table.c to hide the plumbing of table_procexec which should remain invisible to the user IMO. This is just my 2cts, maybe others have a different opinion on this of course Gilles
Re: add table_procexec in smtpd
Hi,
I wrote table_procexec (despite the copyright which I copy-pasted and forgot to
replace author) so just providing a bit of insight:
table_procexec was written as a proof of concept for a new table protocol
inspired by the filter protocol to make it easier to write privsep table
backends using any language or library available without requiring dependencies
in the daemon.
I implemented it as a table backend because it was the easiest way to show a
working implementation of the protocol without making changes in the daemon,
the table_procexec backend sits in between the daemon and “new” tables to
translate old protocol queries into new protocol queries and new protocol
results into old protocol results,
but the intent was to move the underlying implementation of the table API to
the protocol and not retain this table proxy.
Not my call but I don’t think what you’re proposing is the proper way to
integrate it.
There are a few downsides in plugging it this way but the most annoying one is
that instead of starting a table with a configuration parameter, this is
tricking it into executing the table_procexec and using the configuration path
as the path to the “new” table, which means that you have no way to provide the
new table with a configuration for itself.
Gilles
> On 8 Jun 2021, at 23:04, Aisha Tammy wrote:
>
> Hi,
> I've attached a slightly updated patch for the procexec.
> Ping for someone to take a look :)
>
> Cheers,
> Aisha
>
> diff --git a/usr.sbin/smtpd/smtpctl/Makefile b/usr.sbin/smtpd/smtpctl/Makefile
> index ef8148be8c9..2e8beff1ad1 100644
> --- a/usr.sbin/smtpd/smtpctl/Makefile
> +++ b/usr.sbin/smtpd/smtpctl/Makefile
> @@ -48,6 +48,7 @@ SRCS+= table_static.c
> SRCS+=table_db.c
> SRCS+=table_getpwnam.c
> SRCS+=table_proc.c
> +SRCS+= table_procexec.c
> SRCS+=unpack_dns.c
> SRCS+=spfwalk.c
>
> diff --git a/usr.sbin/smtpd/smtpd.h b/usr.sbin/smtpd/smtpd.h
> index be934112103..221f24fbdc4 100644
> --- a/usr.sbin/smtpd/smtpd.h
> +++ b/usr.sbin/smtpd/smtpd.h
> @@ -1656,6 +1656,7 @@ int table_regex_match(const char *, const char *);
> void table_open_all(struct smtpd *);
> void table_dump_all(struct smtpd *);
> void table_close_all(struct smtpd *);
> +const char *table_service_name(enum table_service );
>
>
> /* to.c */
> diff --git a/usr.sbin/smtpd/smtpd/Makefile b/usr.sbin/smtpd/smtpd/Makefile
> index b31d4e42224..debfc8d8ab7 100644
> --- a/usr.sbin/smtpd/smtpd/Makefile
> +++ b/usr.sbin/smtpd/smtpd/Makefile
> @@ -63,6 +63,7 @@ SRCS+= compress_gzip.c
> SRCS+=table_db.c
> SRCS+=table_getpwnam.c
> SRCS+=table_proc.c
> +SRCS+= table_procexec.c
> SRCS+=table_static.c
>
> SRCS+=queue_fs.c
> diff --git a/usr.sbin/smtpd/table.c b/usr.sbin/smtpd/table.c
> index 1d82d88b81a..0c67d205065 100644
> --- a/usr.sbin/smtpd/table.c
> +++ b/usr.sbin/smtpd/table.c
> @@ -46,8 +46,8 @@ extern struct table_backend table_backend_static;
> extern struct table_backend table_backend_db;
> extern struct table_backend table_backend_getpwnam;
> extern struct table_backend table_backend_proc;
> +extern struct table_backend table_backend_procexec;
>
> -static const char * table_service_name(enum table_service);
> static int table_parse_lookup(enum table_service, const char *, const char *,
> union lookup *);
> static int parse_sockaddr(struct sockaddr *, int, const char *);
> @@ -59,6 +59,7 @@ static struct table_backend *backends[] = {
> _backend_db,
> _backend_getpwnam,
> _backend_proc,
> + _backend_procexec,
> NULL
> };
>
> @@ -77,7 +78,7 @@ table_backend_lookup(const char *backend)
> return NULL;
> }
>
> -static const char *
> +const char *
> table_service_name(enum table_service s)
> {
> switch (s) {
> diff --git a/usr.sbin/smtpd/table_procexec.c b/usr.sbin/smtpd/table_procexec.c
> new file mode 100644
> index 000..88bfc435fb3
> --- /dev/null
> +++ b/usr.sbin/smtpd/table_procexec.c
> @@ -0,0 +1,346 @@
> +/*
> + * Copyright (c) 2013 Eric Faurot
> + *
> + * Permission to use, copy, modify, and distribute this software for any
> + * purpose with or without fee is hereby granted, provided that the above
> + * copyright notice and this permission notice appear in all copies.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> + * WHA
Re: smtpd: use libtls
Been running it for a few days, no regressions so far
> On 5 Feb 2021, at 09:35, Eric Faurot wrote:
>
> No much report so far.
> Anybody had a chance to test this?
> Here is the same diff again with manpage update this time.
>
> Eric.
>
> Index: ca.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/ca.c,v
> retrieving revision 1.37
> diff -u -p -r1.37 ca.c
> --- ca.c 31 Dec 2020 08:27:15 - 1.37
> +++ ca.c 19 Jan 2021 11:09:54 -
> @@ -69,6 +69,7 @@ static int ecdsae_do_verify(const unsign
> EC_KEY *);
>
>
> +static struct dict pkeys;
> static uint64_treqid = 0;
>
> static void
> @@ -132,26 +133,29 @@ ca_init(void)
> struct pki *pki;
> const char *k;
> void*iter_dict;
> + char*hash;
>
> log_debug("debug: init private ssl-tree");
> + dict_init();
> iter_dict = NULL;
> while (dict_iter(env->sc_pki_dict, _dict, , (void **))) {
> if (pki->pki_key == NULL)
> continue;
>
> - if ((in = BIO_new_mem_buf(pki->pki_key,
> - pki->pki_key_len)) == NULL)
> - fatalx("ca_launch: key");
> -
> - if ((pkey = PEM_read_bio_PrivateKey(in,
> - NULL, NULL, NULL)) == NULL)
> - fatalx("ca_launch: PEM");
> + in = BIO_new_mem_buf(pki->pki_key, pki->pki_key_len);
> + if (in == NULL)
> + fatalx("ca_init: key");
> + pkey = PEM_read_bio_PrivateKey(in, NULL, NULL, NULL);
> + if (pkey == NULL)
> + fatalx("ca_init: PEM");
> BIO_free(in);
>
> - pki->pki_pkey = pkey;
> -
> - freezero(pki->pki_key, pki->pki_key_len);
> - pki->pki_key = NULL;
> + hash = ssl_pubkey_hash(pki->pki_cert, pki->pki_cert_len);
> + if (dict_check(, hash))
> + EVP_PKEY_free(pkey);
> + else
> + dict_xset(, hash, pkey);
> + free(hash);
> }
> }
>
> @@ -223,15 +227,15 @@ end:
> void
> ca_imsg(struct mproc *p, struct imsg *imsg)
> {
> + EVP_PKEY*pkey;
> RSA *rsa = NULL;
> EC_KEY *ecdsa = NULL;
> const void *from = NULL;
> unsigned char *to = NULL;
> struct msg m;
> - const char *pkiname;
> + const char *hash;
> size_t flen, tlen, padding;
> int buf_len;
> - struct pki *pki;
> int ret = 0;
> uint64_t id;
> int v;
> @@ -267,16 +271,15 @@ ca_imsg(struct mproc *p, struct imsg *im
> case IMSG_CA_RSA_PRIVDEC:
> m_msg(, imsg);
> m_get_id(, );
> - m_get_string(, );
> + m_get_string(, );
> m_get_data(, , );
> m_get_size(, );
> m_get_size(, );
> m_end();
>
> - pki = dict_get(env->sc_pki_dict, pkiname);
> - if (pki == NULL || pki->pki_pkey == NULL ||
> - (rsa = EVP_PKEY_get1_RSA(pki->pki_pkey)) == NULL)
> - fatalx("ca_imsg: invalid pki");
> + pkey = dict_get(, hash);
> + if (pkey == NULL || (rsa = EVP_PKEY_get1_RSA(pkey)) == NULL)
> + fatalx("ca_imsg: invalid pkey hash");
>
> if ((to = calloc(1, tlen)) == NULL)
> fatalx("ca_imsg: calloc");
> @@ -306,14 +309,14 @@ ca_imsg(struct mproc *p, struct imsg *im
> case IMSG_CA_ECDSA_SIGN:
> m_msg(, imsg);
> m_get_id(, );
> - m_get_string(, );
> + m_get_string(, );
> m_get_data(, , );
> m_end();
>
> - pki = dict_get(env->sc_pki_dict, pkiname);
> - if (pki == NULL || pki->pki_pkey == NULL ||
> - (ecdsa = EVP_PKEY_get1_EC_KEY(pki->pki_pkey)) == NULL)
> - fatalx("ca_imsg: invalid pki");
> + pkey = dict_get(, hash);
> + if (pkey == NULL ||
> + (ecdsa = EVP_PKEY_get1_EC_KEY(pkey)) == NULL)
> + fatalx("ca_imsg: invalid pkey hash");
>
> buf_len = ECDSA_size(ecdsa);
> if ((to = calloc(1, buf_len)) == NULL)
> @@ -350,12 +353,12 @@ rsae_send_imsg(int flen, const unsigned
> struct imsg imsg;
> int n, done = 0;
> const void *toptr;
> - char*pkiname;
> + char*hash;
> size_t tlen;
> struct msg m;
> uint64_t id;
>
> - if ((pkiname = RSA_get_ex_data(rsa, 0)) == NULL)
> + if ((hash = RSA_get_ex_data(rsa, 0)) == NULL)
>
Re: smtpd: use libtls
January 27, 2021 9:47 AM, "Lauri Tirkkonen" wrote: > On Wed, Jan 27 2021 09:36:31 +0100, Eric Faurot wrote: > >> There has been a plan for some time now to make smtpd use libtls >> instead of openssl. Recent changes in libtls allow to move forward >> with this. Here is a diff to start the switch. I've tried to keep >> it as small as possible, sticking to the necessary changes. There is >> still a lot of code that can be removed but that will be done in a >> second time. > > I'm all for this, and sorry for screaming from the gallery, but I want to ask > - > is there a plan relating to libtls for portable OpenSMTPD? As it stands, > OpenSSL-based systems are largely unable to use libtls (which in itself is a > shame) - how would this change make it to portable? > TL;DR: In January 2020, I adapted OpenSMTPD to libtls for the first time and did it both for OpenBSD and portable. Since many systems didn't have LibreSSL available, this resulted in libtls being brought to the openbsd-compat layer and adapted to build with OpenSSL. The plan is to use libtls from LibreSSL if detected, otherwise take the openbsd-compat version if OpenSSL is detected. More (outdated) details here: https://poolp.org/posts/2020-01-22/january-2020-opensmtpd-work-libasr-and-libtls/ As a side note: The work eric@ did on the libtls conversion was based on my diff but diverged and I will have to adapt my work from last year to make it work again. I'll take care of making it work again once his work is committed. As of today, there's no one but me working on the portable release so it would be nice if people interested in a portable release would step up to help.
Re: vacation.1: correct .forward file example
January 21, 2021 11:45 AM, gil...@poolp.org wrote: > January 21, 2021 11:25 AM, "Claus Assmann" wrote: > >> On Thu, Jan 21, 2021, Martin Vahlensieck wrote: >> >>> I think the backslash at the beginning of the line is an error. >> >> Why? Does it fail when used as described? >> >>> -\eeric, "|/usr/bin/vacation -a allman eric" >> >> Originally this was to avoid recursion, i.e., >> \eric >> will not be expanded again. >> >> Maybe that has been changed (with smtpd instead of sendmail) so it >> is not necessary anymore? > > It is unnecessary with smtpd. > > If you have an alias which points to itself, you end up in two cases: > > a- the alias key is not a user, in which case this is a cycle and the > expansion fails > b- the alias key is a user, in which case this ends the expansion b- ... in which case this ends _this branch of_ the expansion
Re: vacation.1: correct .forward file example
January 21, 2021 11:25 AM, "Claus Assmann" wrote: > On Thu, Jan 21, 2021, Martin Vahlensieck wrote: > >> I think the backslash at the beginning of the line is an error. > > Why? Does it fail when used as described? > >> -\eeric, "|/usr/bin/vacation -a allman eric" > > Originally this was to avoid recursion, i.e., > \eric > will not be expanded again. > > Maybe that has been changed (with smtpd instead of sendmail) so it > is not necessary anymore? > It is unnecessary with smtpd. If you have an alias which points to itself, you end up in two cases: a- the alias key is not a user, in which case this is a cycle and the expansion fails b- the alias key is a user, in which case this ends the expansion
Re: [diff] src/usr.sbin/smtpd: change process names
it's slightly different:
ca is for "crypto agent" unsure if there's really an interest in renaming
internally,
furthermore there's a separate crypto API for encrypted queue so not
distinguishing
between crypto api and crypto agent might be confusing.
Gilles
December 27, 2020 5:43 PM, "Martijn van Duren"
wrote:
> This one reads OK to me, with one minor bikeshed:
> You rename klondike to crypto, but the internals still refer to CA
> everywhere. Wouldn't it be cleaner to leave klondike in step one and do
> a s/CA/CRYPTO/ in a second step so everything is in concent?
> Personally I prefer the name crypto over ca.
>
> martijn@
>
> On Sat, 2020-12-19 at 23:21 +, gil...@poolp.org wrote:
>
>> December 19, 2020 11:26 PM, "Martijn van Duren"
>> wrote:
>>
>> Personally I'd rather wait to keep the names in sync, especially since
>> it's an easy 2 line diff that can easily be incorperated in the bigger
>> thing. But it's not something I'm going to loose sleep over if others
>> thing it can go in right now.
>>
>> Fair enough :-)
>>
>> Below is the diff that changes all references to pony into dispatcher.
>>
>> I didn't rename pony.c to dispatcher.c as this would break the diff, but if
>> this gets
>> committed I'll submit a diff for the rename + Makefile bit
>>
>> diff --git a/usr.sbin/smtpd/bounce.c b/usr.sbin/smtpd/bounce.c
>> index e6fc55780a1..455da6ff8b1 100644
>> --- a/usr.sbin/smtpd/bounce.c
>> +++ b/usr.sbin/smtpd/bounce.c
>> @@ -290,7 +290,7 @@ bounce_drain()
>> }
>>
>> log_debug("debug: bounce: requesting new enqueue socket...");
>> - m_compose(p_pony, IMSG_QUEUE_SMTP_SESSION, 0, 0, -1, NULL,
>> 0);
>> + m_compose(p_dispatcher, IMSG_QUEUE_SMTP_SESSION, 0, 0, -1,
>> NULL, 0);
>>
>> running += 1;
>> }
>> diff --git a/usr.sbin/smtpd/ca.c b/usr.sbin/smtpd/ca.c
>> index fdc177e28b3..0299ee6cecc 100644
>> --- a/usr.sbin/smtpd/ca.c
>> +++ b/usr.sbin/smtpd/ca.c
>> @@ -110,10 +110,10 @@ ca(void)
>>
>> config_peer(PROC_CONTROL);
>> config_peer(PROC_PARENT);
>> - config_peer(PROC_PONY);
>> + config_peer(PROC_DISPATCHER);
>>
>> /* Ignore them until we get our config */
>> - mproc_disable(p_pony);
>> + mproc_disable(p_dispatcher);
>>
>> if (pledge("stdio", NULL) == -1)
>> err(1, "pledge");
>> @@ -246,7 +246,7 @@ ca_imsg(struct mproc *p, struct imsg *imsg)
>> ca_init();
>>
>> /* Start fulfilling requests */
>> - mproc_enable(p_pony);
>> + mproc_enable(p_dispatcher);
>> return;
>>
>> case IMSG_CTL_VERBOSE:
>> @@ -385,7 +385,7 @@ rsae_send_imsg(int flen, const unsigned char *from,
>> unsigned char *to,
>> if (n == 0)
>> break;
>>
>> - log_imsg(PROC_PONY, PROC_CA, );
>> + log_imsg(PROC_DISPATCHER, PROC_CA, );
>>
>> switch (imsg.hdr.type) {
>> case IMSG_CA_RSA_PRIVENC:
>> @@ -393,7 +393,7 @@ rsae_send_imsg(int flen, const unsigned char *from,
>> unsigned char *to,
>> break;
>> default:
>> /* Another imsg is queued up in the buffer */
>> - pony_imsg(p_ca, );
>> + dispatcher_imsg(p_ca, );
>> imsg_free();
>> continue;
>> }
>> @@ -569,14 +569,14 @@ ecdsae_send_enc_imsg(const unsigned char *dgst, int
>> dgst_len,
>> if (n == 0)
>> break;
>>
>> - log_imsg(PROC_PONY, PROC_CA, );
>> + log_imsg(PROC_DISPATCHER, PROC_CA, );
>>
>> switch (imsg.hdr.type) {
>> case IMSG_CA_ECDSA_SIGN:
>> break;
>> default:
>> /* Another imsg is queued up in the buffer */
>> - pony_imsg(p_ca, );
>> + dispatcher_imsg(p_ca, );
>> imsg_free();
>> continue;
>> }
>> diff --git a/usr.sbin/smtpd/config.c b/usr.sbin/smtpd/config.c
>> index 529420ac0f2..2882349ceba 100644
>> --- a/usr.sbin/smtpd/config.c
>> +++ b/usr.sbin/smtpd/config.c
>> @@ -325,8 +325,8 @@ config_peer(enum smtp_proc_type proc)
>> p = p_queue;
>> else if (proc == PROC_SCHEDULER)
>> p = p_scheduler;
>> - else if (proc == PROC_PONY)
>> - p = p_pony;
>> + else if (proc == PROC_DISPATCHER)
>> + p = p_dispatcher;
>> else if (proc == PROC_CA)
>> p = p_ca;
>> else
>> diff --git a/usr.sbin/smtpd
[diff] src/usr.sbin/smtpd: plug a memory leak in regex lookups
Hello,
The following diff plugs a memory leak in regex lookups.
Cheers,
diff --git a/usr.sbin/smtpd/table.c b/usr.sbin/smtpd/table.c
index 4691..d1578403 100644
--- a/usr.sbin/smtpd/table.c
+++ b/usr.sbin/smtpd/table.c
@@ -470,6 +470,7 @@ table_regex_match(const char *string, const char *pattern)
{
regex_t preg;
int cflags = REG_EXTENDED|REG_NOSUB;
+ int ret;
if (strncmp(pattern, "(?i)", 4) == 0) {
cflags |= REG_ICASE;
@@ -479,7 +480,11 @@ table_regex_match(const char *string, const char *pattern)
if (regcomp(, pattern, cflags) != 0)
return (0);
- if (regexec(, string, 0, NULL, 0) != 0)
+ ret = regexec(, string, 0, NULL, 0);
+
+ regfree();
+
+ if (ret != 0)
return (0);
return (1);
Re: [diff] src/usr.sbin/smtpd: add a forward-file option
> On 20 Dec 2020, at 18:15, Chris Bennett > wrote: > > On Sun, Dec 20, 2020 at 09:51:35AM +0100, Gilles CHEHADE wrote: >> >> >>> On 20 Dec 2020, at 07:13, Sebastien Marie wrote: >>> >>> On Sat, Dec 19, 2020 at 10:36:32PM +, gil...@poolp.org wrote: >>>> Hello, >>>> >>>> Whenever a rule with a local action (mbox, maildir, lmtp or mda) is >>>> matched, smtpd will >>>> attempt to search for a ~/.forward file in the recipient directory and >>>> process it. This >>>> may be convenient for some setups but it is an implicit behavior that's >>>> not overridable >>>> and not always wanted. >>>> >>>> This diff changes this behavior by requiring the admins to explicitly >>>> allow the forward >>>> files processing in the actions when desired: >>>> >>>> action "local_users" maildir forward-file >>>> >>>> >>>> With this diff, if forward-file is not specified, code to request parent >>>> process for an >>>> fd is bypassed and the expansion layer just pretends parent couldn't find >>>> one. This let >>>> the code fallback in an already existing code path with the proper >>>> behavior and is very >>>> uninvasive. >>>> >>> >>> if I could understood the direction (which is fine as it makes the >>> daemon less behaviour dependant on a user settings), the default seems >>> wrong to me (at least for now, and for OpenBSD base specifically). >>> >>> Currently, root@ mail delivery is based on /root/.forward file: >>> install is writing this file to redirect root@ mail to user (if user >>> was created at install-time). It is done this way since 2011 (see >>> distrib/miniroot/install.sh rev 1.218). So I assume that all installs >>> which were done with a user configured, since 2011, could use it. >> >> Yes, the default would need to be changed as follows: >> >> mini$ diff -uNp smtpd.conf smtpd.conf.new >> >> >> --- smtpd.conf Mon Dec 14 22:13:04 2020 >> +++ smtpd.conf.new Sun Dec 20 09:43:22 2020 >> @@ -11,7 +11,7 @@ listen on socket >> # >> listen on all hostname debug.poolp.org >> >> -action "local_mail" maildir alias >> +action "local_mail" maildir alias forward-file >> action "outbound" relay >> > > My src tree still has mbox as the default. There was talk of changing > from mbox to maildir as default. Is this now going forward also? Nope, sorry, this was just an example from my machine which uses maildir so that you get the idea, I don’t propose we name machines “debug.poolp.org” by default either ;-) > While mbox is simple, once I moved to Dovecot for IMAP, changing mbox to > maildir was easy, but needed, amongst some other non-mbox choices. > > I think new users will be much happier learning maildir and skipping the > whole mbox thing. > > My 2 cents. :^) I don’t need to be convinced as I was the one who launched the maildir debate, however base is not ready and mail doesn’t support maildir to begin with. If there was a switch to maildir, the first effect would be that you could not read the mail sent to root at install and the daily, weekly and monthly mails, including the insecurity report.
Re: [diff] usr.sbin/smtpd: fix event handling upon exit
Ping ?
> On 14 Dec 2020, at 11:34, Gilles Chehade wrote:
>
> Hello,
>
> Upon termination, the parent process will call parent_shutdown() which will
> in turn call mproc_clear() to properly terminate IPC with child processes.
>
> In mproc_clear(), event_del() is called but a check is lacking to ensure
> event_add() was called prior to this.
>
> On OpenBSD, this doesn’t seem to cause any issue but on other systems with a
> different libevent, calling event_del() without a matching event_add() either
> causes a runtime warning or a crash upon exit.
>
> Gilles
>
>
> diff --git a/usr.sbin/smtpd/mproc.c b/usr.sbin/smtpd/mproc.c
> index bde229e1..dac38af2 100644
> --- a/usr.sbin/smtpd/mproc.c
> +++ b/usr.sbin/smtpd/mproc.c
> @@ -90,7 +90,8 @@ mproc_clear(struct mproc *p)
> {
> log_debug("debug: clearing p=%s, fd=%d, pid=%d", p->name,
> p->imsgbuf.fd, p->pid);
>
> - event_del(>ev);
> + if (p->events)
> + event_del(>ev);
> close(p->imsgbuf.fd);
> imsg_clear(>imsgbuf);
> }
>
Re: [diff] src/usr.sbin/smtpd: add a forward-file option
> On 20 Dec 2020, at 10:14, Sebastien Marie wrote: > > On Sat, Dec 19, 2020 at 11:19:10PM -0700, Theo de Raadt wrote: >> There are thousands of people with smtpd configurations, and sysmerge >> is not going to handle this. >> >> We cannot expect them all to change their files. This is madness. > > Well, it wouldn't be the first time. But I agree that such changes > should be rare and have really good reason for. > > So yes, even if the option is desirable and being off-by-default would > be a good default, the flag-day way for handling it is complex. I really want to emphasise that I don’t suggest off-by-default on OpenBSD but just making the feature explicit. The default smtpd.conf could still have the option to retain the default behaviour. > Regarding the option itself, if I recall correctly some descriptions > made by Gilles about smtpd, opening ~/.forward is one of the few tasks > done by the priviligied process of smtpd. So it could make sense to > avoid it if not need. > > Gilles, could you confirm that having an option to remove .forward > capability (whatever the default value of the option is) could > effectively help to reduce the attack surface of smtpd ? Yes, this is one of the benefits. Setups that don’t ask for forward-file don’t go through the parent process at every recipient submitted in a session, that’s one imsg less handled by the privileged process. > For example, as immediate consequence, I see no reason for smtpd > priviliegied process to keep a full filesystem view: it might be > possible to restricted it to few directories with unveil(2) (I assume > priviliegied process is still need for bsd_auth, and bsd_auth will > have some requirements). Yes !
Re: [diff] src/usr.sbin/smtpd: add a forward-file option
> On 20 Dec 2020, at 10:03, Gilles CHEHADE wrote: > > >> On 20 Dec 2020, at 07:19, Theo de Raadt wrote: >> >> There are thousands of people with smtpd configurations, and sysmerge >> is not going to handle this. >> >> We cannot expect them all to change their files. This is madness. >> >> Gilles, I think you should be adding an option that blocks it optionally, >> and then some operators can use that. If they wish. I am surprised you >> think this can be a default, when as Sebastien points out the base system >> uses it today... >> > > I know that this isn’t convenient and my first version of the diff was a > “disalllow-forward-file” option but: > > The diff was to discuss what I think is the right way of doing it, not the > one I find the most convenient. > If this is not desired, I can submit a diff for the convenient way but I > would have hated not showing what I think is right first. > > In addition, my diff is a turn on a feature explicitly whereas the > “disallow-forward-file” option is a turn off an implicit behaviour, > and when I see that some people don’t even know that .forward files are a > thing, I feel it’s the wrong way around. People who > want forward files know they exist and can ask for it, whereas people who > don’t know they exist or who don’t request it will > get it behind their backs. > > As I said to semarie@ and millert@, the default configuration could be > adapted to add forward-file to the mbox action, > and this diff could be adapted to not ignore .forward files but warn that > they are used on a rule without the keyword to > give people two releases to adapt since we can’t expect everyone to change > their files but we can expect them to upgrade > at least every two releases. > > Also, what doesn’t show on this diff is that if we rely on the implicit > behaviour and a “disallow-forward-file” it kind of makes > other features backwards too in terms of configuration. > > Assuming disallow-forward-file, then do we add an option to disallow > execution of an mda or do we add an option to allow it ? > Does the default behaviour of forward files is to execute custom commands or > not ? > If not, then how do we express it if there’s no option visible in the conf ? > > It makes the grammar very weird :-/ I’d like to add something I forgot to mention, there are two bonus benefits to do this: 1- the forward file handling requires an indirection through the parent process to obtain an fd to the .forward file, an indirection through a privileged process which would be completely bypassed for all users who do not explicitly require it. 2- if we make the feature explicit, then it becomes easier to add some security safe-guards in the parent process right before execution of an MDA: if it is asked to execute a custom command but the configuration states there Is no .forward file allowed, then we can detect something is fishy and refuse to fork a child process for delivery.
Re: [diff] src/usr.sbin/smtpd: add a forward-file option
> On 20 Dec 2020, at 03:21, Theo de Raadt wrote: > > Todd C. Miller wrote: > >> I like this direction but I worry about breaking existing configs. >> How are we going to alert existing users that they need to update >> their configs if the behavior silently changes? > > I think the configuration is backwards. > > Every endpoint box will need these new stanza. > > Very strange. I’m really unsure about that. It does make your endpoints and mine need the option because we receive mail on machines where users have shells, but from what I observe this is not the common case. From what I observe, most people expose mail through IMAP, the forward files are not accessible to recipients. I could be wrong but I think we are biased on this.
Re: [diff] src/usr.sbin/smtpd: add a forward-file option
> On 20 Dec 2020, at 07:19, Theo de Raadt wrote: > > There are thousands of people with smtpd configurations, and sysmerge > is not going to handle this. > > We cannot expect them all to change their files. This is madness. > > Gilles, I think you should be adding an option that blocks it optionally, > and then some operators can use that. If they wish. I am surprised you > think this can be a default, when as Sebastien points out the base system > uses it today... > I know that this isn’t convenient and my first version of the diff was a “disalllow-forward-file” option but: The diff was to discuss what I think is the right way of doing it, not the one I find the most convenient. If this is not desired, I can submit a diff for the convenient way but I would have hated not showing what I think is right first. In addition, my diff is a turn on a feature explicitly whereas the “disallow-forward-file” option is a turn off an implicit behaviour, and when I see that some people don’t even know that .forward files are a thing, I feel it’s the wrong way around. People who want forward files know they exist and can ask for it, whereas people who don’t know they exist or who don’t request it will get it behind their backs. As I said to semarie@ and millert@, the default configuration could be adapted to add forward-file to the mbox action, and this diff could be adapted to not ignore .forward files but warn that they are used on a rule without the keyword to give people two releases to adapt since we can’t expect everyone to change their files but we can expect them to upgrade at least every two releases. Also, what doesn’t show on this diff is that if we rely on the implicit behaviour and a “disallow-forward-file” it kind of makes other features backwards too in terms of configuration. Assuming disallow-forward-file, then do we add an option to disallow execution of an mda or do we add an option to allow it ? Does the default behaviour of forward files is to execute custom commands or not ? If not, then how do we express it if there’s no option visible in the conf ? It makes the grammar very weird :-/
Re: [diff] src/usr.sbin/smtpd: add a forward-file option
> On 20 Dec 2020, at 07:13, Sebastien Marie wrote: > > On Sat, Dec 19, 2020 at 10:36:32PM +, gil...@poolp.org wrote: >> Hello, >> >> Whenever a rule with a local action (mbox, maildir, lmtp or mda) is matched, >> smtpd will >> attempt to search for a ~/.forward file in the recipient directory and >> process it. This >> may be convenient for some setups but it is an implicit behavior that's not >> overridable >> and not always wanted. >> >> This diff changes this behavior by requiring the admins to explicitly allow >> the forward >> files processing in the actions when desired: >> >>action "local_users" maildir forward-file >> >> >> With this diff, if forward-file is not specified, code to request parent >> process for an >> fd is bypassed and the expansion layer just pretends parent couldn't find >> one. This let >> the code fallback in an already existing code path with the proper behavior >> and is very >> uninvasive. >> > > if I could understood the direction (which is fine as it makes the > daemon less behaviour dependant on a user settings), the default seems > wrong to me (at least for now, and for OpenBSD base specifically). > > Currently, root@ mail delivery is based on /root/.forward file: > install is writing this file to redirect root@ mail to user (if user > was created at install-time). It is done this way since 2011 (see > distrib/miniroot/install.sh rev 1.218). So I assume that all installs > which were done with a user configured, since 2011, could use it. Yes, the default would need to be changed as follows: mini$ diff -uNp smtpd.conf smtpd.conf.new --- smtpd.conf Mon Dec 14 22:13:04 2020 +++ smtpd.conf.new Sun Dec 20 09:43:22 2020 @@ -11,7 +11,7 @@ listen on socket # listen on all hostname debug.poolp.org -action "local_mail" maildir alias +action "local_mail" maildir alias forward-file action "outbound" relay # Uncomment the following to accept external mail for domain "example.org" mini$ > At first step, I would keep the default smtpd.conf with "forward-file" > option set. It would make smtpd(1) to default to no "forward-file" if > not set (what your diff do), but set the default to with > "forward-file" for OpenBSD base. > > Admin could remove the option if he/she doesn't use it. Yes, I agree and I was showing the idea more than suggesting a default configuration for OpenBSD base. If the default config had the diff I showed above, which is what you suggest, then there would be no behaviour change on a default install. As for existing setups, as I answered to millert@, there could even be a two release plan so that not having the keyword would not break forward file but just warn the admin in logs that the keyword is now needed whenever a .forward file is used. This would push them into adapting their configuration file before it breaks a year from now. Since there’s currently no way of not having forward files, then this does not break setups as not doing anything keeps the old behaviour + warnings.
Re: [diff] src/usr.sbin/smtpd: add a forward-file option
> On 20 Dec 2020, at 02:09, Todd C. Miller wrote: > > I like this direction but I worry about breaking existing configs. > How are we going to alert existing users that they need to update > their configs if the behavior silently changes? > > - todd I agree and this diff was more to suggest a direction and spark discussion than a request to get this in. Today there’s no way to disable forward files and OpenBSD supports two releases. If we agreed this is the right direction then we could have a two-release plan: 1- introduce the keyword but not require it yet 2- add the keyword to the default configuration file 3- throw in a warning in logs whenever a .forward file is used with an action that doesn’t have the keyword set With this, existing setups would not break but start warning that a configuration file change is required. If the configuration change is made, the warnings stop right away. People get two releases to fix their configuration before the keyword becomes mandatory. I’ll address other concerns raised by semarie@ and deraadt@ as a reply to their mail.
Re: [diff] src/usr.sbin/smtpd: change process names
December 19, 2020 11:26 PM, "Martijn van Duren"
wrote:
> Personally I'd rather wait to keep the names in sync, especially since
> it's an easy 2 line diff that can easily be incorperated in the bigger
> thing. But it's not something I'm going to loose sleep over if others
> thing it can go in right now.
>
Fair enough :-)
Below is the diff that changes all references to pony into dispatcher.
I didn't rename pony.c to dispatcher.c as this would break the diff, but if
this gets
committed I'll submit a diff for the rename + Makefile bit
diff --git a/usr.sbin/smtpd/bounce.c b/usr.sbin/smtpd/bounce.c
index e6fc55780a1..455da6ff8b1 100644
--- a/usr.sbin/smtpd/bounce.c
+++ b/usr.sbin/smtpd/bounce.c
@@ -290,7 +290,7 @@ bounce_drain()
}
log_debug("debug: bounce: requesting new enqueue socket...");
- m_compose(p_pony, IMSG_QUEUE_SMTP_SESSION, 0, 0, -1, NULL, 0);
+ m_compose(p_dispatcher, IMSG_QUEUE_SMTP_SESSION, 0, 0, -1,
NULL, 0);
running += 1;
}
diff --git a/usr.sbin/smtpd/ca.c b/usr.sbin/smtpd/ca.c
index fdc177e28b3..0299ee6cecc 100644
--- a/usr.sbin/smtpd/ca.c
+++ b/usr.sbin/smtpd/ca.c
@@ -110,10 +110,10 @@ ca(void)
config_peer(PROC_CONTROL);
config_peer(PROC_PARENT);
- config_peer(PROC_PONY);
+ config_peer(PROC_DISPATCHER);
/* Ignore them until we get our config */
- mproc_disable(p_pony);
+ mproc_disable(p_dispatcher);
if (pledge("stdio", NULL) == -1)
err(1, "pledge");
@@ -246,7 +246,7 @@ ca_imsg(struct mproc *p, struct imsg *imsg)
ca_init();
/* Start fulfilling requests */
- mproc_enable(p_pony);
+ mproc_enable(p_dispatcher);
return;
case IMSG_CTL_VERBOSE:
@@ -385,7 +385,7 @@ rsae_send_imsg(int flen, const unsigned char *from,
unsigned char *to,
if (n == 0)
break;
- log_imsg(PROC_PONY, PROC_CA, );
+ log_imsg(PROC_DISPATCHER, PROC_CA, );
switch (imsg.hdr.type) {
case IMSG_CA_RSA_PRIVENC:
@@ -393,7 +393,7 @@ rsae_send_imsg(int flen, const unsigned char *from,
unsigned char *to,
break;
default:
/* Another imsg is queued up in the buffer */
- pony_imsg(p_ca, );
+ dispatcher_imsg(p_ca, );
imsg_free();
continue;
}
@@ -569,14 +569,14 @@ ecdsae_send_enc_imsg(const unsigned char *dgst, int
dgst_len,
if (n == 0)
break;
- log_imsg(PROC_PONY, PROC_CA, );
+ log_imsg(PROC_DISPATCHER, PROC_CA, );
switch (imsg.hdr.type) {
case IMSG_CA_ECDSA_SIGN:
break;
default:
/* Another imsg is queued up in the buffer */
- pony_imsg(p_ca, );
+ dispatcher_imsg(p_ca, );
imsg_free();
continue;
}
diff --git a/usr.sbin/smtpd/config.c b/usr.sbin/smtpd/config.c
index 529420ac0f2..2882349ceba 100644
--- a/usr.sbin/smtpd/config.c
+++ b/usr.sbin/smtpd/config.c
@@ -325,8 +325,8 @@ config_peer(enum smtp_proc_type proc)
p = p_queue;
else if (proc == PROC_SCHEDULER)
p = p_scheduler;
- else if (proc == PROC_PONY)
- p = p_pony;
+ else if (proc == PROC_DISPATCHER)
+ p = p_dispatcher;
else if (proc == PROC_CA)
p = p_ca;
else
diff --git a/usr.sbin/smtpd/control.c b/usr.sbin/smtpd/control.c
index 6ea52b62cfb..e9e19cd1870 100644
--- a/usr.sbin/smtpd/control.c
+++ b/usr.sbin/smtpd/control.c
@@ -248,7 +248,7 @@ control(void)
config_peer(PROC_QUEUE);
config_peer(PROC_PARENT);
config_peer(PROC_LKA);
- config_peer(PROC_PONY);
+ config_peer(PROC_DISPATCHER);
config_peer(PROC_CA);
control_listen();
@@ -450,7 +450,7 @@ control_dispatch_ext(struct mproc *p, struct imsg *imsg)
m_compose(p, IMSG_CTL_FAIL, 0, 0, -1, NULL, 0);
return;
}
- m_compose(p_pony, IMSG_CTL_SMTP_SESSION, c->id, 0, -1,
+ m_compose(p_dispatcher, IMSG_CTL_SMTP_SESSION, c->id, 0, -1,
>euid, sizeof(c->euid));
return;
@@ -597,7 +597,7 @@ control_dispatch_ext(struct mproc *p, struct imsg *imsg)
}
log_info("info: smtp paused");
env->sc_flags
[diff] src/usr.sbin/smtpd: add allow-exec to explicitly allow commands from aliases
Last diff of the series:
This introduces the same logic as forward-file for executing commands.
Executing commands from aliases should be discouraged as you can always achieve
the same
through a forward file and benefit from the privilege separation of running a
command as
a separate user rather than as the smtpd user... but historically commands have
been ran
from aliases so the aliases expansion supports running custom commands.
With this diff, an admin must explicitly allow commands to be ran from aliases:
action "local_users" maildir alias allow-exec
otherwise sessions resolving to an alias that's a command temporarily fail.
Because aliases and virtual uses the same expansion loop, this applies to both:
action "local_users" maildir virtual allow-exec
diff --git a/usr.sbin/smtpd/lka_session.c b/usr.sbin/smtpd/lka_session.c
index aea0780017e..7a817d868ee 100644
--- a/usr.sbin/smtpd/lka_session.c
+++ b/usr.sbin/smtpd/lka_session.c
@@ -489,6 +489,12 @@ lka_expand(struct lka_session *lks, struct rule *rule,
struct expandnode *xn)
lks->error = LKA_TEMPFAIL;
break;
}
+ } else {
+ if (! dsp->u.local.allow_expand_exec) {
+ log_trace(TRACE_EXPAND, "expand: matched expand
with no allow-exec");
+ lks->error = LKA_TEMPFAIL;
+ break;
+ }
}
log_trace(TRACE_EXPAND, "expand: lka_expand: filter: %s "
diff --git a/usr.sbin/smtpd/parse.y b/usr.sbin/smtpd/parse.y
index 908c189c93d..3a42487acc7 100644
--- a/usr.sbin/smtpd/parse.y
+++ b/usr.sbin/smtpd/parse.y
@@ -608,7 +608,7 @@ USER STRING {
dispatcher->u.local.user = $2;
}
-| ALIAS tables {
+| ALIAS tables allow_exec {
struct table *t = $2;
if (dispatcher->u.local.table_alias) {
@@ -628,8 +628,9 @@ USER STRING {
}
dispatcher->u.local.table_alias = strdup(t->t_name);
+ dispatcher->u.local.allow_expand_exec = $3;
}
-| VIRTUAL tables {
+| VIRTUAL tables allow_exec {
struct table *t = $2;
if (dispatcher->u.local.table_virtual) {
@@ -649,6 +650,7 @@ USER STRING {
}
dispatcher->u.local.table_virtual = strdup(t->t_name);
+ dispatcher->u.local.allow_expand_exec = $3;
}
| USERBASE tables {
struct table *t = $2;
diff --git a/usr.sbin/smtpd/smtpd.conf.5 b/usr.sbin/smtpd/smtpd.conf.5
index c2ef5f568ca..15623b58d86 100644
--- a/usr.sbin/smtpd/smtpd.conf.5
+++ b/usr.sbin/smtpd/smtpd.conf.5
@@ -167,12 +167,16 @@ Relay the message to another SMTP server.
.Pp
The local delivery methods support additional options:
.Bl -tag -width Ds
-.It Cm alias Pf < Ar table Ns >
+.It Cm alias Pf < Ar table Ns > Op Cm allow-exec
Use the mapping
.Ar table
for
.Xr aliases 5
expansion.
+.Pp
+If
+.Cm allow-exec
+is specified, aliases are allowed to execute a custom command.
.It Cm forward-file Op Cm allow-exec
Allow the use of a .forward file in user home directory .
.Pp
@@ -211,12 +215,16 @@ The
does not apply for the
.Cm user
option.
-.It Cm virtual Pf < Ar table Ns >
+.It Cm virtual Pf < Ar table Ns > Op Cm allow-exec
Use the mapping
.Ar table
for virtual expansion.
The aliasing table format is described in
.Xr table 5 .
+.Pp
+If
+.Cm allow-exec
+is specified, virtuals are allowed to execute a custom command.
.It Cm wrapper Ar name
Use the wrapper specified in
.Cm mda wrapper .
diff --git a/usr.sbin/smtpd/smtpd.h b/usr.sbin/smtpd/smtpd.h
index 57a8bebec79..7a0695ac5da 100644
--- a/usr.sbin/smtpd/smtpd.h
+++ b/usr.sbin/smtpd/smtpd.h
@@ -1161,6 +1161,7 @@ struct dispatcher_local {
uint8_t forward_only;
uint8_t forward_file;
+ uint8_t allow_expand_exec;
uint8_t allow_forward_exec;
char*mda_wrapper;
[diff] src/usr.sbin/smtpd: add allow-exec to explicitly allow custom mda
Hello,
As is done in other MTA, smtpd allows execution of a custom command in forward
files so
users can plug their procmail, fdm and other. It is currently not possible to
allow the
users to forward their mail through a .forward file without also allowing them
to run a
custom mda.
This diff builds on top of the previous one, it removes the ability to execute
a custom
command from a ~/.forward file by default unless admin explicitly allows it in
config:
action "local_users" maildir forward-file allow-exec
If a user adds a command, the session will be rejected with a temporary failure
until
the .forward file is fixed.
diff --git a/usr.sbin/smtpd/lka_session.c b/usr.sbin/smtpd/lka_session.c
index ff328441957..aea0780017e 100644
--- a/usr.sbin/smtpd/lka_session.c
+++ b/usr.sbin/smtpd/lka_session.c
@@ -482,6 +482,15 @@ lka_expand(struct lka_session *lks, struct rule *le,
struct expandnode *xn)
lks->error = LKA_TEMPFAIL;
break;
}
+
+ if (xn->parent->forwarded) {
+ if (! dsp->u.local.allow_forward_exec) {
+ log_trace(TRACE_EXPAND, "expand: matched
forward with no allow-exec");
+ lks->error = LKA_TEMPFAIL;
+ break;
+ }
+ }
+
log_trace(TRACE_EXPAND, "expand: lka_expand: filter: %s "
"[depth=%d]", xn->u.buffer, xn->depth);
lka_submit(lks, rule, xn);
diff --git a/usr.sbin/smtpd/parse.y b/usr.sbin/smtpd/parse.y
index 752c3376b77..908c189c93d 100644
--- a/usr.sbin/smtpd/parse.y
+++ b/usr.sbin/smtpd/parse.y
@@ -173,7 +173,7 @@ typedef struct {
%}
-%token ACTION ADMD ALIAS ANY ARROW AUTH AUTH_OPTIONAL
+%token ACTION ADMD ALIAS ALLOW_EXEC ANY ARROW AUTH AUTH_OPTIONAL
%token BACKUP BOUNCE BYPASS
%token CA CERT CHAIN CHROOT CIPHERS COMMIT COMPRESSION CONNECT
%token DATA DATA_LINE DHE DISCONNECT DOMAIN
@@ -200,7 +200,7 @@ typedef struct {
%token STRING
%token NUMBER
%type table
-%typesize negation
+%typesize negation allow_exec
%type tables tablenew tableref
%%
@@ -580,6 +580,10 @@ SRS KEY STRING {
;
+allow_exec : ALLOW_EXEC{ $$ = 1; }
+ | /* empty */ { $$ = 0; }
+ ;
+
dispatcher_local_option:
USER STRING {
if (dispatcher->u.local.is_mbox) {
@@ -669,12 +673,13 @@ USER STRING {
}
dispatcher->u.local.mda_wrapper = $2;
}
-| FORWARD_FILE {
+| FORWARD_FILE allow_exec {
if (dispatcher->u.local.forward_file) {
yyerror("forward-file already specified for this dispatcher");
YYERROR;
}
dispatcher->u.local.forward_file = 1;
+ dispatcher->u.local.allow_forward_exec = $2;
}
;
@@ -2628,6 +2633,7 @@ lookup(char *s)
{ "action", ACTION },
{ "admd", ADMD },
{ "alias", ALIAS },
+ { "allow-exec", ALLOW_EXEC },
{ "any",ANY },
{ "auth", AUTH },
{ "auth-optional", AUTH_OPTIONAL },
diff --git a/usr.sbin/smtpd/smtpd.conf.5 b/usr.sbin/smtpd/smtpd.conf.5
index fa98e13e158..c2ef5f568ca 100644
--- a/usr.sbin/smtpd/smtpd.conf.5
+++ b/usr.sbin/smtpd/smtpd.conf.5
@@ -173,8 +173,12 @@ Use the mapping
for
.Xr aliases 5
expansion.
-.It Cm forward-file
+.It Cm forward-file Op Cm allow-exec
Allow the use of a .forward file in user home directory .
+.Pp
+If
+.Cm allow-exec
+is specified, the .forward file is allowed to execute a custom command.
.It Xo
.Cm ttl
.Sm off
diff --git a/usr.sbin/smtpd/smtpd.h b/usr.sbin/smtpd/smtpd.h
index 8225f3ff157..57a8bebec79 100644
--- a/usr.sbin/smtpd/smtpd.h
+++ b/usr.sbin/smtpd/smtpd.h
@@ -1161,6 +1161,8 @@ struct dispatcher_local {
uint8_t forward_only;
uint8_t forward_file;
+ uint8_t allow_forward_exec;
+
char*mda_wrapper;
char*command;
[diff] src/usr.sbin/smtpd: add a forward-file option
Hello,
Whenever a rule with a local action (mbox, maildir, lmtp or mda) is matched,
smtpd will
attempt to search for a ~/.forward file in the recipient directory and process
it. This
may be convenient for some setups but it is an implicit behavior that's not
overridable
and not always wanted.
This diff changes this behavior by requiring the admins to explicitly allow the
forward
files processing in the actions when desired:
action "local_users" maildir forward-file
With this diff, if forward-file is not specified, code to request parent
process for an
fd is bypassed and the expansion layer just pretends parent couldn't find one.
This let
the code fallback in an already existing code path with the proper behavior and
is very
uninvasive.
diff --git a/usr.sbin/smtpd/lka_session.c b/usr.sbin/smtpd/lka_session.c
index ed1fd36fafd..ff328441957 100644
--- a/usr.sbin/smtpd/lka_session.c
+++ b/usr.sbin/smtpd/lka_session.c
@@ -434,9 +434,17 @@ lka_expand(struct lka_session *lks, struct rule *rule,
struct expandnode *xn)
fwreq.uid = lk.userinfo.uid;
fwreq.gid = lk.userinfo.gid;
- m_compose(p_parent, IMSG_LKA_OPEN_FORWARD, 0, 0, -1,
- , sizeof(fwreq));
- lks->flags |= F_WAITING;
+ if (dsp->u.local.forward_file) {
+ log_debug("OPENING FORWARD FILE");
+ m_compose(p_parent, IMSG_LKA_OPEN_FORWARD, 0, 0, -1,
+ , sizeof(fwreq));
+ lks->flags |= F_WAITING;
+ } else {
+ log_debug("BYPASSING FORWARD FILE");
+ fwreq.status = 1;
+ lks->flags |= F_WAITING;
+ lka_session_forward_reply(, -1);
+ }
break;
case EXPAND_FILENAME:
diff --git a/usr.sbin/smtpd/parse.y b/usr.sbin/smtpd/parse.y
index 9f1cb52ec98..752c3376b77 100644
--- a/usr.sbin/smtpd/parse.y
+++ b/usr.sbin/smtpd/parse.y
@@ -178,7 +178,7 @@ typedef struct {
%token CA CERT CHAIN CHROOT CIPHERS COMMIT COMPRESSION CONNECT
%token DATA DATA_LINE DHE DISCONNECT DOMAIN
%token EHLO ENABLE ENCRYPTION ERROR EXPAND_ONLY
-%token FCRDNS FILTER FOR FORWARD_ONLY FROM
+%token FCRDNS FILTER FOR FORWARD_FILE FORWARD_ONLY FROM
%token GROUP
%token HELO HELO_SRC HOST HOSTNAME HOSTNAMES
%token INCLUDE INET4 INET6
@@ -669,6 +669,13 @@ USER STRING {
}
dispatcher->u.local.mda_wrapper = $2;
}
+| FORWARD_FILE {
+ if (dispatcher->u.local.forward_file) {
+ yyerror("forward-file already specified for this dispatcher");
+ YYERROR;
+ }
+ dispatcher->u.local.forward_file = 1;
+}
;
dispatcher_local_options:
@@ -2646,6 +2653,7 @@ lookup(char *s)
{ "fcrdns", FCRDNS },
{ "filter", FILTER },
{ "for",FOR },
+ { "forward-file", FORWARD_FILE },
{ "forward-only", FORWARD_ONLY },
{ "from", FROM },
{ "group", GROUP },
diff --git a/usr.sbin/smtpd/smtpd.conf.5 b/usr.sbin/smtpd/smtpd.conf.5
index 36207c39a1e..fa98e13e158 100644
--- a/usr.sbin/smtpd/smtpd.conf.5
+++ b/usr.sbin/smtpd/smtpd.conf.5
@@ -173,6 +173,8 @@ Use the mapping
for
.Xr aliases 5
expansion.
+.It Cm forward-file
+Allow the use of a .forward file in user home directory .
.It Xo
.Cm ttl
.Sm off
diff --git a/usr.sbin/smtpd/smtpd.h b/usr.sbin/smtpd/smtpd.h
index 529ff683f76..8225f3ff157 100644
--- a/usr.sbin/smtpd/smtpd.h
+++ b/usr.sbin/smtpd/smtpd.h
@@ -1159,6 +1159,7 @@ struct dispatcher_local {
uint8_t expand_only;
uint8_t forward_only;
+ uint8_t forward_file;
char*mda_wrapper;
char*command;
Re: [diff] src/usr.sbin/smtpd: change process names
I agree but I thought this should be done in a second time as it is quite invasive and not required for the change to be visible outside the daemon December 19, 2020 11:13 PM, "Martijn van Duren" wrote: > I'm in favour of this change, since I like proper nomenclature. > But I think you should push this one to its logical conclusion and also > rename the enum and potential other pony/klondike references, because > with your diff the naming is inconsistent, which is even more confusing. > > martijn@ > > On Sat, 2020-12-19 at 22:06 +, gil...@poolp.org wrote: > >> Hello, >> >> A very long time ago, smtpd had several more processes which then got >> factored >> into a single one. We couldn't find a decent name back then but since a >> hacker >> had requested a pony from me I temporarily named the process "pony express" >> as >> it was in charge of delivering mail. Later, reyk improved the joke as he >> named >> the privsep crypto process klondike. >> >> A few years ago when the config was reworked, we used the term "dispatcher" >> to >> identify if a mail was dispatched to the mda or mta layer. >> >> Unless someone is very emotionally attached to the initial joke, I suggest >> the >> pony express process be renamed to dispatcher and klondike to crypto. The >> goal >> is not just to end the joke but also to avoid a process name with a space, >> and >> also because it makes it less obvious what these processes do. >> >> diff --git a/usr.sbin/smtpd/smtpd.c b/usr.sbin/smtpd/smtpd.c >> index 854c2ab0cb6..b364b22e472 100644 >> --- a/usr.sbin/smtpd/smtpd.c >> +++ b/usr.sbin/smtpd/smtpd.c >> @@ -1913,9 +1913,9 @@ proc_title(enum smtp_proc_type proc) >> case PROC_SCHEDULER: >> return "scheduler"; >> case PROC_PONY: >> - return "pony express"; >> + return "dispatcher"; >> case PROC_CA: >> - return "klondike"; >> + return "crypto"; >> case PROC_CLIENT: >> return "client"; >> case PROC_PROCESSOR:
[diff] src/usr.sbin/smtpd: change process names
Hello, A very long time ago, smtpd had several more processes which then got factored into a single one. We couldn't find a decent name back then but since a hacker had requested a pony from me I temporarily named the process "pony express" as it was in charge of delivering mail. Later, reyk improved the joke as he named the privsep crypto process klondike. A few years ago when the config was reworked, we used the term "dispatcher" to identify if a mail was dispatched to the mda or mta layer. Unless someone is very emotionally attached to the initial joke, I suggest the pony express process be renamed to dispatcher and klondike to crypto. The goal is not just to end the joke but also to avoid a process name with a space, and also because it makes it less obvious what these processes do. diff --git a/usr.sbin/smtpd/smtpd.c b/usr.sbin/smtpd/smtpd.c index 854c2ab0cb6..b364b22e472 100644 --- a/usr.sbin/smtpd/smtpd.c +++ b/usr.sbin/smtpd/smtpd.c @@ -1913,9 +1913,9 @@ proc_title(enum smtp_proc_type proc) case PROC_SCHEDULER: return "scheduler"; case PROC_PONY: - return "pony express"; + return "dispatcher"; case PROC_CA: - return "klondike"; + return "crypto"; case PROC_CLIENT: return "client"; case PROC_PROCESSOR:
Re: [diff] src/usr.sbin/smtpd: plug two memory leaks
December 17, 2020 4:02 PM, gil...@poolp.org wrote: > Hello, > > The following diffs plug two memory leaks in smtpd: > > a- in lka_filter.c, the name of the filter chain for a session is strdup()-ed > upon session allocation but not released upon session release. free() it > in lka_filter_end(). > > b- in smtp_session.c, filter io channel should be released when a tx is over, > but it isn't. call io_free() on the channel in smtp_tx_free() if we do > have a channel ready. > please disregard b-, there's no leak here as martijn@ pointed out in private
[diff] src/usr.sbin/smtpd: plug two memory leaks
Hello,
The following diffs plug two memory leaks in smtpd:
a- in lka_filter.c, the name of the filter chain for a session is strdup()-ed
upon session allocation but not released upon session release. free() it
in lka_filter_end().
b- in smtp_session.c, filter io channel should be released when a tx is over,
but it isn't. call io_free() on the channel in smtp_tx_free() if we do
have a channel ready.
Gilles
diff --git a/usr.sbin/smtpd/lka_filter.c b/usr.sbin/smtpd/lka_filter.c
index 9891e6140a3..6eb0829efca 100644
--- a/usr.sbin/smtpd/lka_filter.c
+++ b/usr.sbin/smtpd/lka_filter.c
@@ -535,6 +535,7 @@ lka_filter_end(uint64_t reqid)
free(fs->mail_from);
free(fs->username);
free(fs->lastparam);
+ free(fs->filter_name);
free(fs);
log_trace(TRACE_FILTERS, "%016"PRIx64" filters session-end", reqid);
}
diff --git a/usr.sbin/smtpd/smtp_session.c b/usr.sbin/smtpd/smtp_session.c
index 60123ad9a80..4530f44fb69 100644
--- a/usr.sbin/smtpd/smtp_session.c
+++ b/usr.sbin/smtpd/smtp_session.c
@@ -2438,6 +2438,11 @@ smtp_tx_free(struct smtp_tx *tx)
if (tx->ofile)
fclose(tx->ofile);
+ if (tx->filter) {
+ io_free(tx->filter);
+ tx->filter = NULL;
+ }
+
tx->session->tx = NULL;
free(tx);
[diff] usr.sbin/smtpd: fix event handling upon exit
Hello,
Upon termination, the parent process will call parent_shutdown() which will in
turn call mproc_clear() to properly terminate IPC with child processes.
In mproc_clear(), event_del() is called but a check is lacking to ensure
event_add() was called prior to this.
On OpenBSD, this doesn’t seem to cause any issue but on other systems with a
different libevent, calling event_del() without a matching event_add() either
causes a runtime warning or a crash upon exit.
Gilles
diff --git a/usr.sbin/smtpd/mproc.c b/usr.sbin/smtpd/mproc.c
index bde229e1..dac38af2 100644
--- a/usr.sbin/smtpd/mproc.c
+++ b/usr.sbin/smtpd/mproc.c
@@ -90,7 +90,8 @@ mproc_clear(struct mproc *p)
{
log_debug("debug: clearing p=%s, fd=%d, pid=%d", p->name,
p->imsgbuf.fd, p->pid);
- event_del(>ev);
+ if (p->events)
+ event_del(>ev);
close(p->imsgbuf.fd);
imsg_clear(>imsgbuf);
}
Re: smtpd: relax ORCPT check again
November 18, 2020 10:17 PM, "Joerg Jung" wrote:
> On Wed, Nov 18, 2020 at 08:57:48PM +, gil...@poolp.org wrote:
>
>> November 18, 2020 9:54 PM, "Joerg Jung" wrote:
>>
>> Hi,
>>
>> in my opinion revision 1.423 of smtp_session.c went a bit too far.
>> Enforcing that ORCPT has to have domain results in breakage in real
>> world usage. For example, sending from root user cron jobs mails
>> via Postfix aliased to an address handled by OpenSMTPD will fail.
>>
>> RFC leaves some room for interpretation here, but interoperability
>> may be considered important as well. GitHub issue #1084 [1] has
>> some more elaborations and examples, but was opened for the same
>> reason.
>>
>> Therefore, I propose the following diff below to slightly relax
>> the ORCPT check again by skipping the domain part check.
>>
>> Comments, OK?
>>
>> maybe just skip the check ONLY if domain part is empty ?
>
> Like in the diff below and assuming that domain part is always
> nul terminated after text_to_mailaddr()?
>
This reads better IMO yes
> OK?
>
> Index: smtp_session.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v
> retrieving revision 1.426
> diff -u -p -r1.426 smtp_session.c
> --- smtp_session.c 24 Apr 2020 11:34:07 - 1.426
> +++ smtp_session.c 18 Nov 2020 21:10:36 -
> @@ -2583,7 +2583,8 @@ smtp_tx_rcpt_to(struct smtp_tx *tx, cons
>
> if (!text_to_mailaddr(>evp.dsn_orcpt, opt) ||
> !valid_localpart(tx->evp.dsn_orcpt.user) ||
> - !valid_domainpart(tx->evp.dsn_orcpt.domain)) {
> + (strlen(tx->evp.dsn_orcpt.domain) != 0 &&
> + !valid_domainpart(tx->evp.dsn_orcpt.domain))) {
> smtp_reply(tx->session,
> "553 ORCPT address syntax error");
> return;
smtpd stricter forkmda()
hello,
forkmda() is never supposed to be called with an action dispatcher which
is not local, this would indicate that the code path was abused somehow.
idea suggested by Demi M. Obenour
diff --git a/smtpd/smtpd.c b/smtpd/smtpd.c
index ce1262fa..4c5fc3d9 100644
--- a/smtpd/smtpd.c
+++ b/smtpd/smtpd.c
@@ -1409,6 +1409,8 @@ forkmda(struct mproc *p, uint64_t id, struct deliver
*deliver)
const char *pw_dir;
dsp = dict_xget(env->sc_dispatchers, deliver->dispatcher);
+ if (dsp->type != DISPATCHER_LOCAL)
+ fatalx("non-local dispatcher called from forkmda()");
log_debug("debug: smtpd: forking mda for session %016"PRIx64
": %s as %s", id, deliver->userinfo.username,
--
Gilles Chehade @poolpOrg
https://www.poolp.org patreon: https://www.patreon.com/gilles
Re: smtpd: fix catch-all in virtual aliases
April 28, 2020 11:02 AM, "Joerg Jung" mailto:m...@umaxx.net?to=%22Joerg%20Jung%22%20)> wrote: On 28. Apr 2020, at 10:10, gil...@poolp.org (mailto:gil...@poolp.org) wrote: April 28, 2020 8:55 AM, "Joerg Jung" mailto:m...@umaxx.net)> wrote: Also this change might break existing valid setups (e.g. with mailing list servers), but people will likely know how to cope with it. Do you have an example of an existing valid setup that is broken with this ? No example, I just presumed (that’s why I wrote “might”). ok, because I can't think of a setup that could be broken with this as @ is the very last wildcard matching everything not matched earlier. this diff fixes an actual reported bug and passed various tests mixing no wildcard, user wildcard, domain wildcard. I think it should go in as it is a regression that probably got introduced a while back, the @ wildcard used to work for sure.
Re: smtpd: fix catch-all in virtual aliases
April 28, 2020 8:55 AM, "Joerg Jung" wrote: >> On 26. Apr 2020, at 18:30, Eric Faurot wrote: >> >> When a catch-all entry (@) is used in a virtual alias table, it >> eventually (and mistakenly) catches everything that expands to a >> username. For example, with: >> >> f...@example.com user >> @ catchall >> >> "f...@example.com" expands to "user" as expected, but then "user" >> expands to "catchall" because it is interpreted as "user@" (empty >> domain). > > Which makes sense to me. If one doesn’t specify a domain after the ‘@‘, > I would expect to really catch-all for all domains and all users. > that's not how mailaddr work in OpenSMTPD, the semantic is this one: user=> user@* user@domain => user@domain @domain => *@domain @ => * and this is not an aliases only thing, this is how table lookups are performed for type K_MAILADDR. This allows you to do stuff like follows: root : root gil...@poolp.org : gilles e...@faurot.net : eric @poolp.org : poolpcatchall @faurot.net : faurotcatchall @: catchallusuer >> The catch-all fallback mechanism is really meant for full email >> addresses in virtual context, and should not happen for usernames. >> The following diff fixes it. > > Yes, I agree that catch-all only really meant to be used for single virtual > domain context and not with primary domains. > > But instead of allowing the syntax and ignoring the case in aliases.c > as in your diff below, I would prefer to “fail" on parsing of the table and > error logging that an empty domain after ‘@‘ is not a valid syntax, no? > > Also this change might break existing valid setups (e.g. with mailing list > servers), but people will likely know how to cope with it. > Do you have an example of an existing valid setup that is broken with this ?
Re: OpenSMTPD: unprivileged mode - now with diff
April 26, 2020 10:34 AM, "Christopher Zimmermann" wrote:
> Hi,
>
> I further developed my approach to allow running smtpd with fewer privileges.
> This diff does two
> things:
>
> - always run lmtp deliveries as SMTPD_USER. The change to mda_unpriv.c is
> needed, because otherwise
> all mails would be delivered to SMTPD_USER.
>
> - add two internal flags NOPRIV and NEEDPRIV. NOPRIV can be configured by the
> simple directive
> "no-priv". NEEDPRIV gets set on all delivery methods / options requiring
> setuid() to run as the
> receipient user.
> A configuration error is produced on any conflict betweed NEEDPRIV and NOPRIV.
> In case of a NOPRIV run smtpd will drop root privileges.
> This will break .forward and alias filters.
>
> The change to the lmtp delivery has benefits even without the second change.
> With the second change
> my smtpd now runs without root privileges.
> The NEEDPRIV/NOPRIV options are meant to allow restricting of the privileges
> of other delivery
> methods.
>
> I am now looking for OKs on the first change to do unprivileged lmtp
> deliveries and feedback on the
> general approach of the second change.
>
The LMTP change seems interesting to me, it means that a broken LMTP delivery
will fail with _smtpd privileges instead of the (unprivileged) recipient user
so I think it's a good move.
I'm less convinced by the other change and it doesn't only break .forward and
alias, it also breaks authentication, tables reloading, and probably stuff my
mind is not yet awake enough to think of.
> Index: mda_unpriv.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/mda_unpriv.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 mda_unpriv.c
> --- mda_unpriv.c 2 Feb 2020 22:13:48 - 1.6
> +++ mda_unpriv.c 26 Apr 2020 05:27:34 -
> @@ -69,8 +69,8 @@ mda_unpriv(struct dispatcher *dsp, struc
> xasprintf(_environ[idx++], "RECIPIENT=%s@%s", deliver->dest.user,
> deliver->dest.domain);
> xasprintf(_environ[idx++], "SHELL=/bin/sh");
> xasprintf(_environ[idx++], "LOCAL=%s", deliver->rcpt.user);
> - xasprintf(_environ[idx++], "LOGNAME=%s", pw_name);
> - xasprintf(_environ[idx++], "USER=%s", pw_name);
> + xasprintf(_environ[idx++], "LOGNAME=%s", deliver->userinfo.username);
> + xasprintf(_environ[idx++], "USER=%s", deliver->userinfo.username);
> if (deliver->sender.user[0])
> xasprintf(_environ[idx++], "SENDER=%s@%s",
> Index: parse.y
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/parse.y,v
> retrieving revision 1.277
> diff -u -p -r1.277 parse.y
> --- parse.y 24 Feb 2020 23:54:27 - 1.277
> +++ parse.y 26 Apr 2020 05:27:35 -
> @@ -154,6 +154,7 @@ static int host_v4(struct listen_opts *)
> static int host_v6(struct listen_opts *);
> static int host_dns(struct listen_opts *);
> static int interface(struct listen_opts *);
> +static void need_priv(const char *);
> int delaytonum(char *);
> int is_if_in_group(const char *, const char *);
> @@ -186,7 +187,7 @@ typedef struct {
> %token KEY
> %token LIMIT LISTEN LMTP LOCAL
> %token MAIL_FROM MAILDIR MASK_SRC MASQUERADE MATCH MAX_MESSAGE_SIZE
> MAX_DEFERRED MBOX MDA MTA MX
> -%token NO_DSN NO_VERIFY NOOP
> +%token NO_DSN NO_PRIV NO_VERIFY NOOP
> %token ON
> %token PHASE PKI PORT PROC PROC_EXEC PROXY_V2
> %token QUEUE QUIT
> @@ -212,6 +213,7 @@ grammar : /* empty */
> | grammar ca '\n'
> | grammar mda '\n'
> | grammar mta '\n'
> + | grammar privs '\n'
> | grammar pki '\n'
> | grammar proc '\n'
> | grammar queue '\n'
> @@ -379,6 +381,20 @@ MTA MAX_DEFERRED NUMBER {
> ;
> +privs:
> +NO_PRIV {
> + if (conf->sc_opts & SMTPD_OPT_NEEDPRIV) {
> + yyerror("Unprivileged operation is not possible.");
> + YYERROR;
> + }
> + else {
> + log_warnx("Unprivileged operation requested.");
> + conf->sc_opts |= SMTPD_OPT_NOPRIV;
> + }
> +}
> +;
> +
> +
> pki:
> PKI STRING {
> char buf[HOST_NAME_MAX+1];
> @@ -566,6 +582,8 @@ SRS KEY STRING {
> dispatcher_local_option:
> USER STRING {
> + need_priv("with user");
> +
> if (dispatcher->u.local.is_mbox) {
> yyerror("user may not be specified for this dispatcher");
> YYERROR;
> @@ -662,16 +680,20 @@ dispatcher_local_option dispatcher_local
> dispatcher_local:
> MBOX {
> + need_priv("mbox");
> dispatcher->u.local.is_mbox = 1;
> asprintf(>u.local.command, "/usr/libexec/mail.local -f
> %%{mbox.from} --
> %%{user.username}");
> } dispatcher_local_options
> | MAILDIR {
> + need_priv("maildir");
> asprintf(>u.local.command, "/usr/libexec/mail.maildir");
> } dispatcher_local_options
> | MAILDIR JUNK {
> + need_priv("maildir");
> asprintf(>u.local.command, "/usr/libexec/mail.maildir -j");
> } dispatcher_local_options
> | MAILDIR STRING {
> + need_priv("maildir");
> if (strncmp($2, "~/", 2) == 0)
> asprintf(>u.local.command,
> "/usr/libexec/mail.maildir \"%%{user.directory}/%s\"", $2+2);
> @@ -680,6 +702,7 @@ MBOX {
> "/usr/libexec/mail.maildir \"%s\"", $2);
> } dispatcher_local_options
> | MAILDIR STRING
Re: smtpd: trailing CR
observed no issue, still running w/ your diff April 23, 2020 1:10 PM, "Eric Faurot" wrote: > On Tue, Apr 21, 2020 at 07:08:48AM +, gil...@poolp.org wrote: > >> April 21, 2020 4:28 AM, "Todd C. Miller" wrote: >> >> On Mon, 20 Apr 2020 15:01:31 +0200, Eric Faurot wrote: >> >> There has been a discussion a while ago about the issue of trailing CR >> in smtp lines (https://marc.info/?l=openbsd-tech=156890805128121=2). >> >> It is agreed that stripping an optional CR at io level is a problem >> because the information is lost and there is no way to take a specific >> action at the protocol level if needed. >> >> So this diffs moves the CR stripping from io level to protocol level for >> SMTP dialogs. Other uses of io_getline() are internal and expect simple LF >> line ending. The current behavior should not change. >> >> This looks OK to me but I would feel better if we had successful >> reports from other people too. >> >> Have been running with eric's diff since yesterday afternoon, works for me > > No issue reported so far. > I think it can go in. > > Eric.
Re: smtpd: fix smtpctl discover
April 20, 2020 9:07 PM, "Eric Faurot" wrote:
> Hi again,
>
> We had a report of a crash when running "smtpctl discover" on an
> envelope that has a invalid dispatcher (action name changed in the
> config).
>
> The issue is that the dispatcher is not checked after the envelope is
> loaded as a result of the discover command. But adding the check
> afterwards is not enough, because the envelope is already in the
> envelope cache, so this makes the whole live update/discover process
> actually useless (we would have to invalidate the cache).
>
> So the right fix, which also simplifies the code (always a good sign),
> is to make that check along with the other envelope validation checks
> that happen at envelope load time.
>
makes sense
> Index: queue.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/queue.c,v
> retrieving revision 1.189
> diff -u -p -r1.189 queue.c
> --- queue.c 30 Dec 2018 23:09:58 - 1.189
> +++ queue.c 20 Apr 2020 18:43:17 -
> @@ -686,7 +686,6 @@ static void
> queue_timeout(int fd, short event, void *p)
> {
> static uint32_t msgid = 0;
> - struct dispatcher *dsp;
> struct envelope evp;
> struct event *ev = p;
> struct timeval tv;
> @@ -705,13 +704,6 @@ queue_timeout(int fd, short event, void
> }
>
> if (r) {
> - dsp = dict_get(env->sc_dispatchers, evp.dispatcher);
> - if (dsp == NULL) {
> - log_warnx("warn: queue: missing dispatcher \"%s\""
> - " for envelope %016"PRIx64", ignoring",
> - evp.dispatcher, evp.id);
> - goto reset;
> - }
> if (msgid && evpid_to_msgid(evp.id) != msgid) {
> m_create(p_scheduler, IMSG_QUEUE_MESSAGE_COMMIT,
> 0, 0, -1);
> @@ -724,7 +716,6 @@ queue_timeout(int fd, short event, void
> m_close(p_scheduler);
> }
>
> -reset:
> tv.tv_sec = 0;
> tv.tv_usec = 10;
> evtimer_add(ev, );
> Index: queue_backend.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/queue_backend.c,v
> retrieving revision 1.65
> diff -u -p -r1.65 queue_backend.c
> --- queue_backend.c 30 Dec 2018 23:09:58 - 1.65
> +++ queue_backend.c 20 Apr 2020 18:42:38 -
> @@ -730,6 +730,9 @@ envelope_validate(struct envelope *ep)
> if (memchr(ep->errorline, '\0', sizeof(ep->errorline)) == NULL)
> return "invalid error line";
>
> + if (dict_get(env->sc_dispatchers, ep->dispatcher) == NULL)
> + return "unknown dispatcher";
> +
> return NULL;
> }
Re: smtpd: trailing CR
April 21, 2020 4:28 AM, "Todd C. Miller" wrote: > On Mon, 20 Apr 2020 15:01:31 +0200, Eric Faurot wrote: > >> There has been a discussion a while ago about the issue of trailing CR >> in smtp lines (https://marc.info/?l=openbsd-tech=156890805128121=2). >> >> It is agreed that stripping an optional CR at io level is a problem >> because the information is lost and there is no way to take a specific >> action at the protocol level if needed. >> >> So this diffs moves the CR stripping from io level to protocol level for >> SMTP dialogs. Other uses of io_getline() are internal and expect simple LF >> line ending. The current behavior should not change. > > This looks OK to me but I would feel better if we had successful > reports from other people too. > Have been running with eric's diff since yesterday afternoon, works for me
Re: smtpd: fix report event format
April 8, 2020 11:51 PM, gil...@poolp.org wrote: > April 8, 2020 6:15 PM, "Joerg Jung" wrote: > >>> On 8. Apr 2020, at 17:19, Eric Faurot wrote: >>> >>> Some users had issues with report events for MAIL FROM and RCPT TO >>> when "|" appear in the mail address (yes, it seems to happen), because >>> that's also the field separator. To make parsing the report lines a >>> bit more straightforward, it's better to put the address as the last >>> field. >> >> While this obviously would fix things, I wonder if there is a better choice >> for >> the separator available, to avoid similar issues in future. >> Something that does not show up in hostnames, mail from, rcpt to, or >> anything else SMTP (filter) protocol related. >> Maybe better use ‘$' or even a control character like RS/US record/unit >> separator (ASCII no 29/30)? > > I don't know, having a printable character eases reading event log with > std tools, the problem is really that user-supplied parameters to verbs > are always last on an event except for tx-mail and tx-rcpt so we cannot > use the same logic as with data-line. > Sent too early, missed the following: I'll fix filter-rspamd to handle both pre and post change this week so there is a port update available before eric@ commits his fix.
Re: smtpd: fix report event format
April 8, 2020 6:15 PM, "Joerg Jung" wrote: >> On 8. Apr 2020, at 17:19, Eric Faurot wrote: >> >> Some users had issues with report events for MAIL FROM and RCPT TO >> when "|" appear in the mail address (yes, it seems to happen), because >> that's also the field separator. To make parsing the report lines a >> bit more straightforward, it's better to put the address as the last >> field. > > While this obviously would fix things, I wonder if there is a better choice > for > the separator available, to avoid similar issues in future. > Something that does not show up in hostnames, mail from, rcpt to, or > anything else SMTP (filter) protocol related. > Maybe better use ‘$' or even a control character like RS/US record/unit > separator (ASCII no 29/30)? > I don't know, having a printable character eases reading event log with std tools, the problem is really that user-supplied parameters to verbs are always last on an event except for tx-mail and tx-rcpt so we cannot use the same logic as with data-line.
Re: bump smtpd version?
yes ok January 30, 2020 2:02 PM, "Solene Rapenne" wrote: > on https://opensmtpd.org the OpenBSD version file says 6.6.2 while we > currently have 6.6.1 in CVS. > > Should we bump to 6.6.2? > > Index: smtpd.h > === > RCS file: /data/cvs/src/usr.sbin/smtpd/smtpd.h,v > retrieving revision 1.650 > diff -u -p -r1.650 smtpd.h > --- smtpd.h 8 Jan 2020 01:41:11 - 1.650 > +++ smtpd.h 30 Jan 2020 13:00:21 - > @@ -51,7 +51,7 @@ > #define SMTPD_QUEUE_EXPIRY (4 * 24 * 60 * 60) > #define SMTPD_SOCKET "/var/run/smtpd.sock" > #define SMTPD_NAME "OpenSMTPD" > -#define SMTPD_VERSION "6.6.1" > +#define SMTPD_VERSION "6.6.2" > #define SMTPD_SESSION_TIMEOUT 300 > #define SMTPD_BACKLOG 5
Errata patches for OpenSMTPD have been released for OpenBSD 6.5 and 6.6.
Hello tech@ Erratas have been published for a security vulnerability discovered in smtpd by Qualys: 6.5/030_smtpd_exec.patch.sig 6.6/019_smtpd_exec.patch.sig It is VERY important that you syspatch as soon as possible. I'll write about this bug later when things have settled, particularly about what made it possible and what plans we have so similar bugs don't lead to similar consequences, two ideas have been discussed to lock things down. But for now, it's syspatch time. Gilles
Re: .Aq in smtpd.conf(5)
December 5, 2019 4:34 PM, "Ingo Schwarze" wrote: > Hi Jason, > > Jason McIntyre wrote on Thu, Dec 05, 2019 at 02:54:20PM +: > >> i have reverted it, with a heavy heart. > > Do you think i should try and convince the groff folks to always > render .Aq/.Ao/.Ac as ASCII '<' and '>', even in UTF-8, HTML, PDF > output and the like? > > Benefits: > > + easier rules for authors, smaller risk of mistakes, > and ".Aq Mt" would no longer be an exception > + allows nicer and simpler mdoc(7) source code > for configuration file documentation like smtpd.conf and pf.conf > and nicer markup in pages documenting HTML code > + in programming documentation (as opposed to, say, quantum physics > calculations) angle brackets are exceedingly rare, > making .Aq mostly useless under the current rules > > Downside: > > - In cases where authors really want angle brackets - e.g., does > a free software package to help with quantum mechanics calculations > exist? - the output will look slightly worse, but not much worse, > and given the rarity, i think that would be acceptable. Besides, > people who want real angle brackets in some unusual situation > can still use .Eo \(la foobar Ec \(ra . > To me this already looks like quantum mechanics calculations :-p
smtpd: remove implicit listen on socket
hello, smtpd has an implicit listener which is "listen on socket". I propose that we write it explicitely in the default config and give up with this last bit of implicit configuration. The goal behind that is to stop having implicit behaviors but it is also to improve security in the daemon: OpenSMTPD uses /var/run/smtpd.sock both as a control socket AND enqueuer socket, which means that socket is rw-rw-rw- and the control process has the charge of checking uid of caller and if permission is allowed to run a specific command. I think we should really have a control socket and one/many SMTP sockets so the control socket could be given tigher filesystem permissions while we could also allow multiple enqueue sockets with different permissions, and control them through the smtpd.conf ruleset like we do for any other connection. The first step towards that is this diff. ok ? Index: smtpd.conf === RCS file: /cvs/src/etc/mail/smtpd.conf,v retrieving revision 1.13 diff -u -p -r1.13 smtpd.conf --- smtpd.conf 25 Nov 2019 13:30:04 - 1.13 +++ smtpd.conf 26 Nov 2019 06:27:11 - @@ -5,6 +5,8 @@ table aliases file:/etc/mail/aliases +listen on socket + # To accept external mail, replace with: listen on all # listen on lo0 -- Gilles Chehade @poolpOrg https://www.poolp.orgpatreon: https://www.patreon.com/gilles
Re: smtpd remove implicit ruleset behavior
On Mon, Nov 25, 2019 at 08:30:21AM +0100, Gilles Chehade wrote:
> On Mon, Nov 25, 2019 at 01:09:20AM +0100, Joerg Jung wrote:
> > On Sun, Nov 24, 2019 at 10:54:14AM +0100, Gilles Chehade wrote:
> > >
> > > Ten years ago, it seemed a very neat idea that OpenSMTPD would have some
> > > implicit defaults to avoid people creating open relays.
> > >
> > > Back then I was trying to make the smtpd.conf as compact as possible and
> > > came up with the very nice idea of "implicit local" so that we would get
> > > a very compact:
> > >
> > > accept for any relay
> > >
> > > which would not be an open relay as it translated to:
> > >
> > > accept from local for any relay
> > >
> > >
> > > This idea was carried when we moved the syntax to match/action.
> > >
> > > I think this was an error from the beginning and we should only have the
> > > explicit notation as I see a trend in people coming up with:
> > >
> > > match for domain foobar.org action "deliver"
> > >
> > > which, read loud, seems to imply that mail for domain foobar.org will be
> > > delivered but which actually fails because it translates as:
> > >
> > > match from local for domain foobar.org action "deliver"
> > >
> > > and actually limits the scope to local users...
> > >
> > > People keep making this mistake over and over which as safe as it is, is
> > > a serious hint that the mistake is on smtpd's side.
> > >
> > >
> > > Is there strong objection to move to a mode where implicit notation will
> > > no longer be allowed ?
> >
> > No objections. Yes, please make the notation explicit and remove the
> > syntactic sugar which often seems to be the reason for confusions.
> >
> > > This could start with us adding the explicit notation to default config,
> > > then put a startup warning in the next release so configurations are not
> > > broken but people spot that this is no longer encouraged and we can then
> > > later kill it.
> >
> > Sounds like a good plan to me.
> >
>
> This diff makes default smtpd.conf use the explicit notation.
>
> ok ?
>
and this diff makes smtpd warn at startup that implicit rules were used:
laptop$ doas smtpd
smtpd: ruleset relies on implicit 'from' at line 10
smtpd: ruleset relies on implicit 'for' at line 11
laptop$
alternatively these warnx() can be turned into errx() if we want to
go right away into explicit mode without warning for a release.
Index: parse.y
===
RCS file: /cvs/src/usr.sbin/smtpd/parse.y,v
retrieving revision 1.264
diff -u -p -r1.264 parse.y
--- parse.y 12 Nov 2019 21:02:42 - 1.264
+++ parse.y 25 Nov 2019 07:39:28 -
@@ -1313,10 +1313,12 @@ MATCH {
rule = xcalloc(1, sizeof *rule);
} match_options action {
if (!rule->flag_from) {
+ warnx("ruleset relies on implicit 'from' at line %d",
file->lineno);
rule->table_from = strdup("");
rule->flag_from = 1;
}
if (!rule->flag_for) {
+ warnx("ruleset relies on implicit 'for' at line %d",
file->lineno);
rule->table_for = strdup("");
rule->flag_for = 1;
}
--
Gilles Chehade @poolpOrg
https://www.poolp.orgpatreon: https://www.patreon.com/gilles
Re: smtpd remove implicit ruleset behavior
On Mon, Nov 25, 2019 at 01:09:20AM +0100, Joerg Jung wrote: > On Sun, Nov 24, 2019 at 10:54:14AM +0100, Gilles Chehade wrote: > > > > Ten years ago, it seemed a very neat idea that OpenSMTPD would have some > > implicit defaults to avoid people creating open relays. > > > > Back then I was trying to make the smtpd.conf as compact as possible and > > came up with the very nice idea of "implicit local" so that we would get > > a very compact: > > > > accept for any relay > > > > which would not be an open relay as it translated to: > > > > accept from local for any relay > > > > > > This idea was carried when we moved the syntax to match/action. > > > > I think this was an error from the beginning and we should only have the > > explicit notation as I see a trend in people coming up with: > > > > match for domain foobar.org action "deliver" > > > > which, read loud, seems to imply that mail for domain foobar.org will be > > delivered but which actually fails because it translates as: > > > > match from local for domain foobar.org action "deliver" > > > > and actually limits the scope to local users... > > > > People keep making this mistake over and over which as safe as it is, is > > a serious hint that the mistake is on smtpd's side. > > > > > > Is there strong objection to move to a mode where implicit notation will > > no longer be allowed ? > > No objections. Yes, please make the notation explicit and remove the > syntactic sugar which often seems to be the reason for confusions. > > > This could start with us adding the explicit notation to default config, > > then put a startup warning in the next release so configurations are not > > broken but people spot that this is no longer encouraged and we can then > > later kill it. > > Sounds like a good plan to me. > This diff makes default smtpd.conf use the explicit notation. ok ? Index: smtpd.conf === RCS file: /cvs/src/etc/mail/smtpd.conf,v retrieving revision 1.11 diff -u -p -r1.11 smtpd.conf --- smtpd.conf 4 Jun 2018 21:10:58 - 1.11 +++ smtpd.conf 25 Nov 2019 07:28:48 - @@ -15,5 +15,5 @@ action "relay" relay # Uncomment the following to accept external mail for domain "example.org" # # match from any for domain "example.org" action "local" -match for local action "local" -match for any action "relay" +match from local for local action "local" +match from local for any action "relay" -- Gilles Chehade @poolpOrg https://www.poolp.orgpatreon: https://www.patreon.com/gilles
smtpd remove implicit ruleset behavior
Hello, Ten years ago, it seemed a very neat idea that OpenSMTPD would have some implicit defaults to avoid people creating open relays. Back then I was trying to make the smtpd.conf as compact as possible and came up with the very nice idea of "implicit local" so that we would get a very compact: accept for any relay which would not be an open relay as it translated to: accept from local for any relay This idea was carried when we moved the syntax to match/action. I think this was an error from the beginning and we should only have the explicit notation as I see a trend in people coming up with: match for domain foobar.org action "deliver" which, read loud, seems to imply that mail for domain foobar.org will be delivered but which actually fails because it translates as: match from local for domain foobar.org action "deliver" and actually limits the scope to local users... People keep making this mistake over and over which as safe as it is, is a serious hint that the mistake is on smtpd's side. Is there strong objection to move to a mode where implicit notation will no longer be allowed ? This could start with us adding the explicit notation to default config, then put a startup warning in the next release so configurations are not broken but people spot that this is no longer encouraged and we can then later kill it. -- Gilles Chehade @poolpOrg https://www.poolp.orgpatreon: https://www.patreon.com/gilles
Re: smtpd handling of \r in DATA part
> RCS file: /cvs/src/usr.sbin/smtpd/ioev.h,v
> retrieving revision 1.16
> diff -u -p -r1.16 ioev.h
> --- ioev.h30 Nov 2016 17:43:32 - 1.16
> +++ ioev.h24 Apr 2019 09:33:35 -
> @@ -65,5 +65,6 @@ size_t io_queued(struct io *);
> /* Buffered input functions */
> void* io_data(struct io *);
> size_t io_datalen(struct io *);
> -char* io_getline(struct io *, size_t *);
> +char* io_getline_n(struct io *, size_t *);
> +char* io_getline_rn(struct io *, size_t *);
> void io_drop(struct io *, size_t);
> Index: lka_filter.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/lka_filter.c,v
> retrieving revision 1.35
> diff -u -p -r1.35 lka_filter.c
> --- lka_filter.c 8 Apr 2019 07:44:45 - 1.35
> +++ lka_filter.c 24 Apr 2019 09:33:35 -
> @@ -393,7 +393,7 @@ filter_session_io(struct io *io, int evt
> switch (evt) {
> case IO_DATAIN:
> nextline:
> - line = io_getline(fs->io, );
> + line = io_getline_rn(fs->io, );
> /* No complete line received */
> if (line == NULL)
> return;
> Index: lka_proc.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/lka_proc.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 lka_proc.c
> --- lka_proc.c21 Dec 2018 19:07:47 - 1.6
> +++ lka_proc.c24 Apr 2019 09:33:35 -
> @@ -124,7 +124,7 @@ processor_io(struct io *io, int evt, voi
> switch (evt) {
> case IO_DATAIN:
> nextline:
> - line = io_getline(io, );
> + line = io_getline_n(io, );
> /* No complete line received */
> if (line == NULL)
> return;
> Index: mta_session.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/mta_session.c,v
> retrieving revision 1.115
> diff -u -p -r1.115 mta_session.c
> --- mta_session.c 23 Dec 2018 16:37:53 - 1.115
> +++ mta_session.c 24 Apr 2019 09:33:35 -
> @@ -,7 +,7 @@ mta_io(struct io *io, int evt, void *arg
>
> case IO_DATAIN:
> nextline:
> - line = io_getline(s->io, );
> + line = io_getline_rn(s->io, );
> if (line == NULL) {
> if (io_datalen(s->io) >= LINE_MAX) {
> mta_error(s, "Input too long");
> Index: smtp_client.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtp_client.c,v
> retrieving revision 1.8
> diff -u -p -r1.8 smtp_client.c
> --- smtp_client.c 20 Sep 2018 11:42:28 - 1.8
> +++ smtp_client.c 24 Apr 2019 09:33:35 -
> @@ -667,7 +667,7 @@ smtp_client_readline(struct smtp_client
> char *line, *msg, *p;
> int cont;
>
> - line = io_getline(proto->io, );
> + line = io_getline_rn(proto->io, );
> if (line == NULL) {
> if (io_datalen(proto->io) >= proto->params.linemax)
> smtp_client_abort(proto, FAIL_PROTO, "Line too long");
> Index: smtp_session.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v
> retrieving revision 1.389
> diff -u -p -r1.389 smtp_session.c
> --- smtp_session.c20 Feb 2019 11:56:27 - 1.389
> +++ smtp_session.c24 Apr 2019 09:33:35 -
> @@ -1078,7 +1078,7 @@ smtp_io(struct io *io, int evt, void *ar
>
> case IO_DATAIN:
> nextline:
> - line = io_getline(s->io, );
> + line = io_getline_rn(s->io, );
> if ((line == NULL && io_datalen(s->io) >= SMTP_LINE_MAX) ||
> (line && len >= SMTP_LINE_MAX)) {
> s->flags |= SF_BADINPUT;
> @@ -2727,7 +2727,7 @@ filter_session_io(struct io *io, int evt
> switch (evt) {
> case IO_DATAIN:
> nextline:
> - line = io_getline(tx->filter, );
> + line = io_getline_rn(tx->filter, );
> /* No complete line received */
> if (line == NULL)
> return;
--
Gilles Chehade @poolpOrg
https://www.poolp.orgpatreon: https://www.patreon.com/gilles
Re: smtpd handling of \r in DATA part
October 4, 2019 9:55 AM, "Martijn van Duren" wrote: > > This is similar to the diff I send a few months ago, but still doesn't > fix the case when someone sends a standalone '\n' as line-ending. > Unsure I understand that, can you elaborate ? > I'd prefer if we go with reverting (=my previous diff) until we have > something that definitively fixes things. If people send broken mails > they get broken signatures until that time. Fine by me
Re: smtpd handling of \r in DATA part
= i;
+ return (buf);
+ }
+
+ return (NULL);
+}
+
void
iobuf_normalize(struct iobuf *io)
{
diff --git a/smtpd/iobuf.h b/smtpd/iobuf.h
index c454d0a1..e6e8023c 100644
--- a/smtpd/iobuf.h
+++ b/smtpd/iobuf.h
@@ -52,6 +52,7 @@ size_tiobuf_len(struct iobuf *);
size_t iobuf_left(struct iobuf *);
char *iobuf_data(struct iobuf *);
char *iobuf_getline(struct iobuf *, size_t *);
+char *iobuf_getline_nostrip(struct iobuf *, size_t *);
ssize_tiobuf_read(struct iobuf *, int);
ssize_tiobuf_read_tls(struct iobuf *, void *);
diff --git a/smtpd/ioev.c b/smtpd/ioev.c
index d36210fd..4b3733de 100644
--- a/smtpd/ioev.c
+++ b/smtpd/ioev.c
@@ -506,6 +506,12 @@ io_getline(struct io *io, size_t *sz)
return iobuf_getline(>iobuf, sz);
}
+char *
+io_getline_nostrip(struct io *io, size_t *sz)
+{
+ return iobuf_getline_nostrip(>iobuf, sz);
+}
+
void
io_drop(struct io *io, size_t sz)
{
diff --git a/smtpd/ioev.h b/smtpd/ioev.h
index f155a7fc..857f7a20 100644
--- a/smtpd/ioev.h
+++ b/smtpd/ioev.h
@@ -67,4 +67,5 @@ size_t io_queued(struct io *);
void* io_data(struct io *);
size_t io_datalen(struct io *);
char* io_getline(struct io *, size_t *);
+char* io_getline_nostrip(struct io *, size_t *);
void io_drop(struct io *, size_t);
diff --git a/smtpd/lka_filter.c b/smtpd/lka_filter.c
index 695ce4c3..1a7c6c39 100644
--- a/smtpd/lka_filter.c
+++ b/smtpd/lka_filter.c
@@ -399,7 +399,7 @@ filter_session_io(struct io *io, int evt, void *arg)
switch (evt) {
case IO_DATAIN:
nextline:
- line = io_getline(fs->io, );
+ line = io_getline_nostrip(fs->io, );
/* No complete line received */
if (line == NULL)
return;
@@ -653,7 +653,7 @@ filter_data_internal(struct filter_session *fs, uint64_t
token, uint64_t reqid,
/* no filter_entry, we either had none or reached end of chain */
if (filter_entry == NULL) {
- io_printf(fs->io, "%s\r\n", line);
+ io_printf(fs->io, "%s\n", line);
return;
}
diff --git a/smtpd/lka_proc.c b/smtpd/lka_proc.c
index f90d006f..c44a081f 100644
--- a/smtpd/lka_proc.c
+++ b/smtpd/lka_proc.c
@@ -152,7 +152,7 @@ processor_io(struct io *io, int evt, void *arg)
switch (evt) {
case IO_DATAIN:
- while ((line = io_getline(io, )) != NULL) {
+ while ((line = io_getline_nostrip(io, )) != NULL) {
if (strncmp("register|", line, 9) == 0) {
processor_register(name, line);
continue;
@@ -182,7 +182,7 @@ processor_errfd(struct io *io, int evt, void *arg)
switch (evt) {
case IO_DATAIN:
- while ((line = io_getline(io, )) != NULL)
+ while ((line = io_getline_nostrip(io, )) != NULL)
log_warnx("%s: %s", name, line);
}
}
diff --git a/smtpd/smtp_session.c b/smtpd/smtp_session.c
index 95db9099..2e38f5f7 100644
--- a/smtpd/smtp_session.c
+++ b/smtpd/smtp_session.c
@@ -1109,15 +1109,6 @@ smtp_io(struct io *io, int evt, void *arg)
if (line == NULL)
return;
- if (strchr(line, '\r')) {
- s->flags |= SF_BADINPUT;
- smtp_reply(s, "500 %s is only allowed before ",
- esc_code(ESC_STATUS_PERMFAIL, ESC_OTHER_STATUS));
- smtp_enter_state(s, STATE_QUIT);
- io_set_write(io);
- return;
- }
-
/* Message body */
eom = 0;
if (s->state == STATE_BODY) {
@@ -2792,7 +2783,7 @@ filter_session_io(struct io *io, int evt, void *arg)
switch (evt) {
case IO_DATAIN:
nextline:
- line = io_getline(tx->filter, );
+ line = io_getline_nostrip(tx->filter, );
/* No complete line received */
if (line == NULL)
return;
--
Gilles Chehade @poolpOrg
https://www.poolp.orgpatreon: https://www.patreon.com/gilles
Re: smtpd handling of \r in DATA part
On Fri, Oct 04, 2019 at 07:35:39AM +0200, Martijn van Duren wrote:
> On 10/3/19 9:05 PM, Eric Faurot wrote:
> > On Thu, Sep 19, 2019 at 05:48:17PM +, gil...@poolp.org wrote:
> >
> >>> To me, the only real problem with '\r' is at the end of lines. It's
> >>> confusing
> >>> since you never really know whether it's part of the content or the
> >>> protocol.
> >>>
> >>> So I suggest that we strip all '\r' found at the end of a line,
> >>> and retain the others.
> >>>
> >>
> >> I'm not sure the only problem is at the end of lines, but I don't think
> >> any solution that's graceful to bad clients will be correct so I'm okay
> >> with your suggestion.
> >>
> >
> > Here is a diff for that.
> > Note that it strips the '\r' on all input, not just DATA.
> >
> > Eric.
> >
> > Index: smtp_session.c
> > ===
> > RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v
> > retrieving revision 1.414
> > diff -u -p -r1.414 smtp_session.c
> > --- smtp_session.c 3 Oct 2019 05:08:21 - 1.414
> > +++ smtp_session.c 3 Oct 2019 18:55:52 -
> > @@ -1109,14 +1109,9 @@ smtp_io(struct io *io, int evt, void *ar
> > if (line == NULL)
> > return;
> >
> > - if (strchr(line, '\r')) {
> > - s->flags |= SF_BADINPUT;
> > - smtp_reply(s, "500 %s is only allowed before ",
> > - esc_code(ESC_STATUS_PERMFAIL, ESC_OTHER_STATUS));
> > - smtp_enter_state(s, STATE_QUIT);
> > - io_set_write(io);
> > - return;
> > - }
> > + /* Strip trailing '\r' */
> > + while (len && line[len - 1] == '\r')
> > + line[--len] = '\0';
> >
> > /* Message body */
> > eom = 0;
> >
> See my previous mail.
> This will break existing dkim signatures on mails which have trailing
> '\r'. Even though clients must not send those, they apparently do.
>
I agree with you for the most part but let's also put perspective here.
This week, on my machines, out of 15k incoming and 14k outgoing e-mails
only 2 mails had '\r' in the incoming path, 0 on the outgoing path, the
2 mails were spam.
Not saying the '\r' issue doesn't need to be adressed but just pointing
that it is a margin case and that the "quick fix" we want at this point
in the release cycle must not take risks of regressions for everyone to
cope with that margin case.
> Do we
> want to invalidate them as well? If that's the case, just remove this
> check, because if the old signatures don't matter than the new signature
> from filter-dkimsign (and others) won't matter as well.
>
I agree, both solutions break DKIM for trailing '\r' e-mails, but if we
go that path I prefer that we handle this upfront rather than on filter
returns.
> Seriously, these solutions are like trying to fit a too large carpet,
> annoying bumps will just be moved around. If we *really* want to fix
> this we need to make it fit within the specifications:
>
> [...]
>
> This means stop opportunistic scanning for '\r' in iobuf!
>
Sure but fixing iobuf is not a two liner and it affects virtually all of
the daemon and at this point we're looking for stability in the code, so
unless eric@ or you can come up with a diff that's trivial and that will
not affect any code paths beyond smtp client and filter getlines(), I'll
prefer a degraded margin case than taking the risk of a regression.
I don't know iobuf enough to dive into this in a rush but if you managed
to pull a iobuf_getline_lf() that's fully isolated from rest of iobuf so
we could use that one in smtp_session.c and lka_proc.c, I'd be fine too,
and it would solve your concern.
--
Gilles Chehade @poolpOrg
https://www.poolp.orgpatreon: https://www.patreon.com/gilles
Re: smtpd: mail.* tempfail vs permfail
September 25, 2019 3:23 PM, "Martijn van Duren"
wrote:
> EHLO,
EHLO,
> Recently I moved my mailserver to a new HD resulting in temporary
> inaccessibility of my maildir (temporarily root owned because of
> recursive copy), leaving smtpd running thinking nothing of it.
> This resulted in a few mails being PERMFAIL.
>
> I talked to gilles@ about this and he's somewhat inclined to agree with
> me that this is a TEMPFAIL situation. Even if the directory where the
> mail is supposed to be delivered is permanently not accessible I'd
> argue that we should keep this a TEMPFAIL, because an admin is inclined
> to spot this misconfiguration mistake earlier this way (e.g. via mailq
> monitoring) and mail won't get lost.
>
> The diff below fixes this for mail.maildir and mail.mboxfile.
> For mail.maildir I left the cases that appeared to be obvious
> misconfiguration as PERMFAIL, but might be worth to convert these to
> EX_TEMPFAIL as well.
>
> Note that mail.lmtp already uses EX_TEMPFAIL in all cases.
>
> While here I also removed the mkdirs from utils.c, which got me
> confused at first (it's also part of mail.maildir.c and not used
> anywhere else).
>
> thoughts? OK?
I'm generally ok with making mda's less strict when it comes to errors
that can genuinely happen from a misconfiguration. I don't think we'll
convert all errors because by buffering on our side in hope that we're
going to recover somehow, we're also delaying the permanent failure on
the sending host which is not always desirable. A lot of errors can be
made less strict for sure, we need to address them case by case.
The mkdirs from utils.c were left-overs from when we converted maildir
to be a real mda rather than an smtpd builtin, good riddance.
diff reads ok to me
> martijn@
>
> Index: mail.maildir.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/mail.maildir.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 mail.maildir.c
> --- mail.maildir.c 3 Jul 2019 03:24:03 - 1.12
> +++ mail.maildir.c 25 Sep 2019 13:21:36 -
> @@ -31,6 +31,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #define MAILADDR_ESCAPE "!#$%&'*/?^`{|}~"
> @@ -93,8 +94,11 @@ maildir_mkdirs(const char *dirname)
> char pathname[PATH_MAX];
> char *subdirs[] = { "cur", "tmp", "new" };
>
> - if (mkdirs(dirname, 0700) == -1 && errno != EEXIST)
> - err(1, NULL);
> + if (mkdirs(dirname, 0700) == -1 && errno != EEXIST) {
> + if (errno == EINVAL || errno == ENAMETOOLONG)
> + err(1, NULL);
> + err(EX_TEMPFAIL, NULL);
> + }
>
> for (i = 0; i < nitems(subdirs); ++i) {
> ret = snprintf(pathname, sizeof pathname, "%s/%s", dirname,
> @@ -102,7 +106,7 @@ maildir_mkdirs(const char *dirname)
> if (ret < 0 || (size_t)ret >= sizeof pathname)
> errc(1, ENAMETOOLONG, "%s/%s", dirname, subdirs[i]);
> if (mkdir(pathname, 0700) == -1 && errno != EEXIST)
> - err(1, NULL);
> + err(EX_TEMPFAIL, NULL);
> }
> }
>
> @@ -178,9 +182,9 @@ maildir_engine(const char *dirname, int
>
> fd = open(tmp, O_CREAT | O_EXCL | O_WRONLY, 0600);
> if (fd == -1)
> - err(1, NULL);
> + err(EX_TEMPFAIL, NULL);
> if ((fp = fdopen(fd, "w")) == NULL)
> - err(1, NULL);
> + err(EX_TEMPFAIL, NULL);
>
> while ((linelen = getline(, , stdin)) != -1) {
> line[strcspn(line, "\n")] = '\0';
> @@ -194,19 +198,19 @@ maildir_engine(const char *dirname, int
> }
> free(line);
> if (ferror(stdin))
> - err(1, NULL);
> + err(EX_TEMPFAIL, NULL);
>
> if (fflush(fp) == EOF ||
> ferror(fp) ||
> fsync(fd) == -1 ||
> fclose(fp) == EOF)
> - err(1, NULL);
> + err(EX_TEMPFAIL, NULL);
>
> (void)snprintf(new, sizeof new, "%s/new/%s",
> is_junk ? junkpath : dirname, filename);
>
> if (rename(tmp, new) == -1)
> - err(1, NULL);
> + err(EX_TEMPFAIL, NULL);
>
> exit(0);
> }
> @@ -223,8 +227,10 @@ mkdirs_component(const char *path, mode_
> if (mkdir(path, mode | S_IWUSR | S_IXUSR) == -1)
> return 0;
> }
> - else if (!S_ISDIR(sb.st_mode))
> + else if (!S_ISDIR(sb.st_mode)) {
> + errno = ENOTDIR;
> return 0;
> + }
>
> return 1;
> }
> @@ -238,12 +244,16 @@ mkdirs(const char *path, mode_t mode)
> const char *p;
>
> /* absolute path required */
> - if (*path != '/')
> + if (*path != '/') {
> + errno = EINVAL;
> return 0;
> + }
>
> /* make sure we don't exceed PATH_MAX */
> - if (strlen(path) >= sizeof buf)
> + if (strlen(path) >= sizeof buf) {
> + errno = ENAMETOOLONG;
> return 0;
> + }
>
> memset(buf, 0, sizeof buf);
> for (p = path;
Re: smtpd: smtpc: ssl_check_name() dead assignment
September 21, 2019 10:03 AM, "Sebastien Marie" wrote:
> Hi,
>
> The current code in smtp_verify_server_cert() has a dead assignment for return
> code of ssl_check_name().
>
> At first stance, I was a bit unsure if return code should be checked or not to
> detect certificate error. but as soon ssl_check_name() is entered, `match' is
> set to 0, and only set to 1 when the name is know to be good.
>
> so I assume it is fine to ignore the return code.
>
fine by me, i had the same diff
> please note that ssl_check_name() is only called at twice places:
> - smtpc.c, here
> - mta_session.c, where return code isn't checked too
>
> so maybe the function API should be changed ? or to return void, or to return
> the match value ?
>
both are fine for me
I don't care much about which way is the best for this because it is a temporary
solution. I converted OpenSMTPD to libtls which handles certificate verification
in a simpler way, so this code is meant to die after 6.6 is out hopefully :-)
> diff ea5e035f4d57ede9f18c82c5c9decc5f46c1925a /home/semarie/repos/openbsd/src
> blob - fb6d711d95f3a8203e44d6662002a32c92a89629
> file + usr.sbin/smtpd/smtpc.c
> --- usr.sbin/smtpd/smtpc.c
> +++ usr.sbin/smtpd/smtpc.c
> @@ -351,10 +351,10 @@ smtp_verify_server_cert(void *tag, struct smtp_client
> SSL *ssl = ctx;
> X509 *cert;
> long res;
> - int r, match;
> + int match;
>
> if ((cert = SSL_get_peer_certificate(ssl))) {
> - r = ssl_check_name(cert, servname, );
> + (void)ssl_check_name(cert, servname, );
> X509_free(cert);
> res = SSL_get_verify_result(ssl);
> if (res == X509_V_OK) {
Re: smtpd handling of \r in DATA part
September 19, 2019 7:26 PM, "Eric Faurot" wrote: > On Thu, Sep 19, 2019 at 05:46:47PM +0200, Gilles Chehade wrote: > >> Hello, >> >> The RFC for SMTP states the following (section 2.3.8): >> >> In addition, the appearance of "bare" "CR" or "LF" characters in text >> (i.e., either without the other) has a long history of causing >> problems in mail implementations and applications that use the mail >> system as a tool. SMTP client implementations MUST NOT transmit >> these characters except when they are intended as line terminators >> and then MUST, as indicated above, transmit them only as a >> sequence. >> >> As a result, OpenSMTPD rejects DATA containing with the following: >> >> 500 5.0.0 is only allowed before >> >> requiring that clients encode DATA if is part of it. >> >> My question is: are we too strict ? >> >> Not two MTA do the same thing. Some will leave '\r' in the body and then >> write it to the user mailbox or relay it. Other change it into a '\n' or >> skip it. The first ones take the risk of a MUA not handling '\r' well or >> an MTA rejecting later, the second ones break DKIM-signatures. >> >> The only good way to deal with this is to stick to the RFC ... BUT users >> then experience message rejections when using broken clients (semarie@'s >> printer is an example of one). >> >> So: >> >> a- do we leave '\r' in the body ? >> b- do we turn '\r' into '\n' > > I don't think it's a good solution. > What happens if there is a dot right after the '\r'? > It gets escaped, just like if '\n' had been sent in place of '\r'. foo\r.\rbar => foo\n..\nbar I don't think it's a good solution either but it's one used in real world, this is the approach Gmail (among others) took. >> c- do we keep strict behavior ? >> d- do we keep strict behavior + provide a knob for '\r' to work ? > > I'm not sure the RFC actually requires the server to reject mails with > "bare" '\r'. It just says the client must not transmit them. So maybe > we should just discard them at receive time... > The RFC doesn't require the server to reject mails with "bare" '\r' BUT since it states that we shouldn't transmit them, not rejecting them has the side-effect that we MUST alter DATA (and break DKIM for these). > To me, the only real problem with '\r' is at the end of lines. It's confusing > since you never really know whether it's part of the content or the protocol. > > So I suggest that we strip all '\r' found at the end of a line, > and retain the others. > I'm not sure the only problem is at the end of lines, but I don't think any solution that's graceful to bad clients will be correct so I'm okay with your suggestion.
smtpd handling of \r in DATA part
Hello, The RFC for SMTP states the following (section 2.3.8): In addition, the appearance of "bare" "CR" or "LF" characters in text (i.e., either without the other) has a long history of causing problems in mail implementations and applications that use the mail system as a tool. SMTP client implementations MUST NOT transmit these characters except when they are intended as line terminators and then MUST, as indicated above, transmit them only as a sequence. As a result, OpenSMTPD rejects DATA containing with the following: 500 5.0.0 is only allowed before requiring that clients encode DATA if is part of it. My question is: are we too strict ? Not two MTA do the same thing. Some will leave '\r' in the body and then write it to the user mailbox or relay it. Other change it into a '\n' or skip it. The first ones take the risk of a MUA not handling '\r' well or an MTA rejecting later, the second ones break DKIM-signatures. The only good way to deal with this is to stick to the RFC ... BUT users then experience message rejections when using broken clients (semarie@'s printer is an example of one). So: a- do we leave '\r' in the body ? b- do we turn '\r' into '\n' c- do we keep strict behavior ? d- do we keep strict behavior + provide a knob for '\r' to work ? -- Gilles Chehade @poolpOrg https://www.poolp.orgpatreon: https://www.patreon.com/gilles
Re: PATCH: smtpd: don't strcmp() NULL in mta_relay_cmp()
On Fri, Sep 13, 2019 at 09:34:33PM +0200, Caspar Schutijser wrote: > Hi, > Hi, > smtpd crashed on one of my machines. I attempted to start it again but > it crashed again after a few seconds. > > [...] > > So I think the problem is that b->backupname is passed to strcmp() > without checking whether b->backupname != NULL. The diff below adds > such a check, in line with other string comparisons that are performed > in mta_relay_cmp(). In the same function, I think the same problem > exists while comparing a->authlabel and b->authlabel so I added a > similar check there. > > I'd like you to double-check whether the fix is indeed correct. > Yes, your understanding of the problem is correct and your diff also, so I committed it a minute ago, thanks ! I'm curious about what configuration allowed you to hit this though, can you share your smtpd.conf with me ? -- Gilles Chehade @poolpOrg https://www.poolp.orgpatreon: https://www.patreon.com/gilles
smtpd junk filtering action
The following diff introduces the junk action which allows builtin
filters to junk a session or transaction.
The following would junk a session if it doesn't have rdns when it
reaches the helo filtering hook:
match "check-rdns" phase helo match !rdns junk
Currently this is not doable so builtin filters may only accept or
throw away a session or transaction, whereas with this diff you'll
be able to automatically classify in Junk folder with the junk mda
option:
action "local_users" maildir junk
This also allows simplifying proc filters that want to junk as the
current method requires registering for data-line and injecting an
X-Spam: Yes header, while this can now be handled by returning the
junk filter response at any phase and letting smtpd prefill header
Index: lka_filter.c
===
RCS file: /cvs/src/usr.sbin/smtpd/lka_filter.c,v
retrieving revision 1.44
diff -u -p -r1.44 lka_filter.c
--- lka_filter.c29 Aug 2019 08:49:55 - 1.44
+++ lka_filter.c2 Sep 2019 20:04:37 -
@@ -56,6 +56,7 @@ static intfilter_builtins_mail_from(str
static int filter_builtins_rcpt_to(struct filter_session *, struct filter
*, uint64_t, const char *);
static voidfilter_result_proceed(uint64_t);
+static voidfilter_result_junk(uint64_t);
static voidfilter_result_rewrite(uint64_t, const char *);
static voidfilter_result_reject(uint64_t, const char *);
static voidfilter_result_disconnect(uint64_t, const char *);
@@ -479,6 +480,11 @@ lka_filter_process_response(const char *
fatalx("Unexpected parameter after proceed: %s", line);
filter_protocol_next(token, reqid, 0);
return;
+ } else if (strcmp(response, "junk") == 0) {
+ if (parameter != NULL)
+ fatalx("Unexpected parameter after junk: %s", line);
+ filter_result_junk(reqid);
+ return;
} else {
if (parameter == NULL)
fatalx("Missing parameter: %s", line);
@@ -574,6 +580,15 @@ filter_protocol_internal(struct filter_s
filter_result_disconnect(reqid,
filter->config->disconnect);
return;
}
+ else if (filter->config->junk) {
+ log_trace(TRACE_FILTERS, "%016"PRIx64" filters protocol
phase=%s, "
+ "resume=%s, action=junk, filter=%s, query=%s",
+ fs->id, phase_name, resume ? "y" : "n",
+ filter->name,
+ param);
+ filter_result_junk(reqid);
+ return;
+ }
else {
log_trace(TRACE_FILTERS, "%016"PRIx64" filters protocol
phase=%s, "
"resume=%s, action=reject, filter=%s, query=%s,
response=%s",
@@ -759,6 +774,15 @@ filter_result_proceed(uint64_t reqid)
m_create(p_pony, IMSG_FILTER_SMTP_PROTOCOL, 0, 0, -1);
m_add_id(p_pony, reqid);
m_add_int(p_pony, FILTER_PROCEED);
+ m_close(p_pony);
+}
+
+static void
+filter_result_junk(uint64_t reqid)
+{
+ m_create(p_pony, IMSG_FILTER_SMTP_PROTOCOL, 0, 0, -1);
+ m_add_id(p_pony, reqid);
+ m_add_int(p_pony, FILTER_JUNK);
m_close(p_pony);
}
Index: lka_report.c
===
RCS file: /cvs/src/usr.sbin/smtpd/lka_report.c,v
retrieving revision 1.27
diff -u -p -r1.27 lka_report.c
--- lka_report.c29 Aug 2019 07:23:18 - 1.27
+++ lka_report.c2 Sep 2019 20:04:38 -
@@ -428,6 +428,9 @@ lka_report_smtp_filter_response(const ch
case FILTER_PROCEED:
response_name = "proceed";
break;
+ case FILTER_JUNK:
+ response_name = "junk";
+ break;
case FILTER_REWRITE:
response_name = "rewrite";
break;
Index: parse.y
===
RCS file: /cvs/src/usr.sbin/smtpd/parse.y,v
retrieving revision 1.259
diff -u -p -r1.259 parse.y
--- parse.y 25 Aug 2019 03:40:45 - 1.259
+++ parse.y 2 Sep 2019 20:04:38 -
@@ -1297,6 +1297,9 @@ REJECT STRING {
| REWRITE STRING {
filter_config->rewrite = $2;
}
+| JUNK {
+ filter_config->junk = 1;
+}
;
filter_phase_check_fcrdns:
Index: smtp_session.c
===
RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v
retrieving revision 1.408
diff -u -p -r1.408 smtp_session.c
--- smtp_session.c 28 Aug 2019 15:50:36 - 1.408
+++ smtp_session.c 2 Sep 2019 20:04:39 -
@@ -126,6 +126,8 @@ struct smtp_tx {
int rcvcount;
int has_date;
int
Re: [Patch] smtp(1) with proto "smtps" should default to port smtps/465
committed thanks !
August 31, 2019 1:57 PM, "Ross L Richardson" wrote:
> ...unless I'm very mistaken!
>
> Ross
>
>
> Index: smtpc.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtpc.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 smtpc.c
> --- smtpc.c 2 Jul 2019 09:36:20 - 1.6
> +++ smtpc.c 31 Aug 2019 11:48:17 -
> @@ -229,7 +229,7 @@ parse_server(char *server)
> else if (!strcmp(scheme, "smtps")) {
> params.tls_req = TLS_SMTPS;
> if (port == NULL)
> - port = "submission";
> + port = "smtps";
> }
> else if (!strcmp(scheme, "smtp")) {
> }
Re: smtpd: change PATH for filters
September 2, 2019 9:15 PM, "Martijn van Duren"
wrote:
> On 9/2/19 9:10 PM, gil...@poolp.org wrote:
>
>> September 2, 2019 7:29 PM, "Martijn van Duren"
>> wrote:
>>
>>> On 9/2/19 6:00 PM, gil...@poolp.org wrote:
>>
>> September 2, 2019 5:55 PM, gil...@poolp.org wrote:
>>
>> September 2, 2019 5:23 PM, "Martijn van Duren"
>> wrote:
>>
>> Gilles should probably elaborate, but the way things are now is that
>> system(3) is used to start the filters, allowing us to run any arbitrary
>> (set of) command(s) as a filter.
>>
>> Since the filters now in ports are non-interactive commands I proposed
>> to move them to /usr/local/libexec/smtpd, which gilles@ is a proponent
>> of. This however means that all filters need to be specified by a full
>> path, which is not something I would promote.
>>
>> Hence the proposition of this diff.
>> I don't feel comfortable adding that path to PATH, even if we're going
>> to call system() right behind.
>>
>> Why not detect if the command starts with '/' and prepend libexec directory
>> if that's not the case ?
>>
>> I also want to rework the command line before it's passed to system() so we
>> exec and save some unnecessary processes which are only waiting for a child
>> to exit its infinite loop.
>>
>> To do that, we are going to copy the command anyways so checking if command
>> starts with a / and prepending the absolute path is going to be easy.
>>> Something like this?
>>
>> can't test right now but comments inlined:
>>
>>> Index: smtpd.c
>>> ===
>>> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.c,v
>>> retrieving revision 1.324
>>> diff -u -p -r1.324 smtpd.c
>>> --- smtpd.c 26 Jul 2019 07:08:34 - 1.324
>>> +++ smtpd.c 2 Sep 2019 17:27:31 -
>>> @@ -1277,6 +1277,8 @@ fork_processor(const char *name, const c
>>> int sp[2], errfd[2];
>>> struct passwd *pw;
>>> struct group *gr;
>>> + char exec[_POSIX_ARG_MAX];
>>
>> I don't know if _POSIX_ARG_MAX is the proper define to use,
>> I genuinely don't know so someone more knowledgeable should jump in.
>
> https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/limits.h.html
> {_POSIX_ARG_MAX}
> Maximum length of argument to the exec functions including environment data.
> Value: 4 096
>
looks good to me unless someone objects
>>> + int execr;
>>>
>>> if (user == NULL)
>>> user = SMTPD_USER;
>>> @@ -1340,6 +1342,14 @@ fork_processor(const char *name, const c
>>> signal(SIGHUP, SIG_DFL) == SIG_ERR)
>>> err(1, "signal");
>>>
>>> + if (command[0] == '/')
>>> + execr = snprintf(exec, sizeof(exec), "exec %s", command);
>>> + else
>>> + execr = snprintf(exec, sizeof(exec), "exec %s/filter-%s",
>>> + PATH_LIBEXEC, command);
>>
>> I don't really like that 'filter-' is implicit.
>>
>> So if I specify full path it's filter-rspamd, when i don't it's rspamd,
>> which is confusing because that's also the name of a software on my
>> system.
>>
>> I think people can live with typing 'filter-' and we keep it explicit.
>
> Sure, no problem. I choose this approach to be more in line with
> queue-*, scheduler-*, and table-*.
> I'll change it before commit.
>
okie dokie
I dislike that we did this for queue-*, scheduler-* and table-* too,
I'd rather see if we can change that in the future.
I have a plan for 2020 to switch queue / scheduler / table to an API
like filters' so we might want to revisit then :-)
>>> + if (execr >= (int) sizeof(exec))
>>> + errx(1, "%s: exec path too long", name);
>>> +
>>> /*
>>> * Wait for lka to acknowledge that it received the fd.
>>> * This prevents a race condition between the filter sending an error
>>> @@ -1350,7 +1360,7 @@ fork_processor(const char *name, const c
>>> */
>>> if (read(STDERR_FILENO, , 1) != 0)
>>> errx(1, "lka didn't properly close write end of error socket");
>>> - if (system(command) == -1)
>>> + if (system(exec) == -1)
>>> err(1, NULL);
>>>
>>> /* there's no successful exit from a processor */
>>> Index: smtpd.conf.5
>>> ===
>>> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.conf.5,v
>>> retrieving revision 1.221
>>> diff -u -p -r1.221
Re: smtpd: change PATH for filters
September 2, 2019 7:29 PM, "Martijn van Duren" wrote: > On 9/2/19 6:00 PM, gil...@poolp.org wrote: > >> September 2, 2019 5:55 PM, gil...@poolp.org wrote: >> >>> September 2, 2019 5:23 PM, "Martijn van Duren" >>> wrote: >> >> Gilles should probably elaborate, but the way things are now is that >> system(3) is used to start the filters, allowing us to run any arbitrary >> (set of) command(s) as a filter. >> >> Since the filters now in ports are non-interactive commands I proposed >> to move them to /usr/local/libexec/smtpd, which gilles@ is a proponent >> of. This however means that all filters need to be specified by a full >> path, which is not something I would promote. >> >> Hence the proposition of this diff. >>> I don't feel comfortable adding that path to PATH, even if we're going >>> to call system() right behind. >>> >>> Why not detect if the command starts with '/' and prepend libexec directory >>> if that's not the case ? >> >> I also want to rework the command line before it's passed to system() so we >> exec and save some unnecessary processes which are only waiting for a child >> to exit its infinite loop. >> >> To do that, we are going to copy the command anyways so checking if command >> starts with a / and prepending the absolute path is going to be easy. > > Something like this? > can't test right now but comments inlined: > Index: smtpd.c > === > RCS file: /cvs/src/usr.sbin/smtpd/smtpd.c,v > retrieving revision 1.324 > diff -u -p -r1.324 smtpd.c > --- smtpd.c 26 Jul 2019 07:08:34 - 1.324 > +++ smtpd.c 2 Sep 2019 17:27:31 - > @@ -1277,6 +1277,8 @@ fork_processor(const char *name, const c > int sp[2], errfd[2]; > struct passwd *pw; > struct group *gr; > + char exec[_POSIX_ARG_MAX]; I don't know if _POSIX_ARG_MAX is the proper define to use, I genuinely don't know so someone more knowledgeable should jump in. > + int execr; > > if (user == NULL) > user = SMTPD_USER; > @@ -1340,6 +1342,14 @@ fork_processor(const char *name, const c > signal(SIGHUP, SIG_DFL) == SIG_ERR) > err(1, "signal"); > > + if (command[0] == '/') > + execr = snprintf(exec, sizeof(exec), "exec %s", command); > + else > + execr = snprintf(exec, sizeof(exec), "exec %s/filter-%s", > + PATH_LIBEXEC, command); I don't really like that 'filter-' is implicit. So if I specify full path it's filter-rspamd, when i don't it's rspamd, which is confusing because that's also the name of a software on my system. I think people can live with typing 'filter-' and we keep it explicit. > + if (execr >= (int) sizeof(exec)) > + errx(1, "%s: exec path too long", name); > + > /* > * Wait for lka to acknowledge that it received the fd. > * This prevents a race condition between the filter sending an error > @@ -1350,7 +1360,7 @@ fork_processor(const char *name, const c > */ > if (read(STDERR_FILENO, , 1) != 0) > errx(1, "lka didn't properly close write end of error socket"); > - if (system(command) == -1) > + if (system(exec) == -1) > err(1, NULL); > > /* there's no successful exit from a processor */ > Index: smtpd.conf.5 > === > RCS file: /cvs/src/usr.sbin/smtpd/smtpd.conf.5,v > retrieving revision 1.221 > diff -u -p -r1.221 smtpd.conf.5 > --- smtpd.conf.5 17 Aug 2019 14:43:21 - 1.221 > +++ smtpd.conf.5 2 Sep 2019 17:27:31 - > @@ -383,6 +383,11 @@ filter > .Ar filter-name > from > .Ar command . > +If > +.Ar command > +starts with a slash it is executed with an absolute path, > +else it will be prepended with > +.Dq /usr/local/libexec/smtpd/filter- . > .It Ic include Qq Ar pathname > Replace this directive with the content of the additional configuration > file at the absolute > @@ -757,6 +762,11 @@ Register an external process named > from > .Ar command . > Such processes may be used to share the same instance between multiple > filters. > +If > +.Ar command > +starts with a slash it is executed with an absolute path, > +else it will be prepended with > +.Dq /usr/local/libexec/smtpd/filter- . > .It Ic queue Cm compression > Store queue files in a compressed format. > This may be useful to save disk space.
Re: smtpd: change PATH for filters
September 2, 2019 5:55 PM, gil...@poolp.org wrote: > September 2, 2019 5:23 PM, "Martijn van Duren" > wrote: > >> Gilles should probably elaborate, but the way things are now is that >> system(3) is used to start the filters, allowing us to run any arbitrary >> (set of) command(s) as a filter. >> >> Since the filters now in ports are non-interactive commands I proposed >> to move them to /usr/local/libexec/smtpd, which gilles@ is a proponent >> of. This however means that all filters need to be specified by a full >> path, which is not something I would promote. >> >> Hence the proposition of this diff. > > I don't feel comfortable adding that path to PATH, even if we're going > to call system() right behind. > > Why not detect if the command starts with '/' and prepend libexec directory > if that's not the case ? > I also want to rework the command line before it's passed to system() so we exec and save some unnecessary processes which are only waiting for a child to exit its infinite loop. To do that, we are going to copy the command anyways so checking if command starts with a / and prepending the absolute path is going to be easy.
Re: smtpd: change PATH for filters
September 2, 2019 5:23 PM, "Martijn van Duren"
wrote:
> Gilles should probably elaborate, but the way things are now is that
> system(3) is used to start the filters, allowing us to run any arbitrary
> (set of) command(s) as a filter.
>
> Since the filters now in ports are non-interactive commands I proposed
> to move them to /usr/local/libexec/smtpd, which gilles@ is a proponent
> of. This however means that all filters need to be specified by a full
> path, which is not something I would promote.
>
> Hence the proposition of this diff.
>
I don't feel comfortable adding that path to PATH, even if we're going
to call system() right behind.
Why not detect if the command starts with '/' and prepend libexec directory
if that's not the case ?
> On 9/2/19 5:13 PM, Theo de Raadt wrote:
>
>> This seems really unconvenitional.
>>
>> PATH is normally used by interactive commands, or scripts which want
>> easily accessible programs. libexec has traditionally been excluded.
>> The idea is that programs which need things in libexec, must hardcode
>> the path. Intentionally. This is a subdirectory of libexec, probably
>> trying to follow the same pattern. So why is it not excluded in the same
>> way?
>>
>> Second questoin: are filter programs going to be in /bin or /usr/sbin?
>> If not, why is /bin on this PATH you are defining?
>>
>> It smell execvp abuse.
>>
>> Martijn van Duren wrote:
>>
>>> With filters most likely defaulting to /usr/local/libexec/smtpd in the
>>> near future I would like to add this as the default PATH, followed by
>>> the usual suspects. I left the usual suspects in place because people
>>> have shown they already implement filters in awk and probably will do
>>> so in /bin/sh in the future, which will need all the other paths.
>>>
>>> I put it in PATH_FILTER, so it can easily be altered for portable
>>> where sysadmins may decide to change the path for filters or for
>>> us if we ever want anything in base under /usr/libexec/smtpd.
>>>
>>> OK?
>>>
>>> martijn@
>>>
>>> Index: smtpd.c
>>> ===
>>> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.c,v
>>> retrieving revision 1.324
>>> diff -u -p -r1.324 smtpd.c
>>> --- smtpd.c 26 Jul 2019 07:08:34 - 1.324
>>> +++ smtpd.c 2 Sep 2019 15:00:47 -
>>> @@ -1350,6 +1350,7 @@ fork_processor(const char *name, const c
>>> */
>>> if (read(STDERR_FILENO, , 1) != 0)
>>> errx(1, "lka didn't properly close write end of error socket");
>>> + setenv("PATH", PATH_FILTER, 1);
>>> if (system(command) == -1)
>>> err(1, NULL);
>>>
>>> Index: smtpd.h
>>> ===
>>> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.h,v
>>> retrieving revision 1.633
>>> diff -u -p -r1.633 smtpd.h
>>> --- smtpd.h 28 Aug 2019 15:50:36 - 1.633
>>> +++ smtpd.h 2 Sep 2019 15:00:47 -
>>> @@ -56,6 +56,7 @@
>>> #define SMTPD_BACKLOG 5
>>>
>>> #define PATH_SMTPCTL "/usr/sbin/smtpctl"
>>> +#define PATH_FILTER "/usr/local/libexec/smtpd:" _PATH_DEFPATH
>>>
>>> #define PATH_OFFLINE "/offline"
>>> #define PATH_PURGE "/purge"
Re: [Patch] smtp(1) with proto "smtps" should default to port smtps/465
no you're not, i'll commit this tomorrow unless eric@ beats me to it
31 août 2019 13:57 "Ross L Richardson" a écrit:
> ...unless I'm very mistaken!
>
> Ross
>
>
> Index: smtpc.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtpc.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 smtpc.c
> --- smtpc.c 2 Jul 2019 09:36:20 - 1.6
> +++ smtpc.c 31 Aug 2019 11:48:17 -
> @@ -229,7 +229,7 @@ parse_server(char *server)
> else if (!strcmp(scheme, "smtps")) {
> params.tls_req = TLS_SMTPS;
> if (port == NULL)
> - port = "submission";
> + port = "smtps";
> }
> else if (!strcmp(scheme, "smtp")) {
> }
Re: smtpd reports - expectation management
29 août 2019 07:02 "Martijn van Duren" a
écrit:
> Since we don't support any smtp-out events at time of writing, so we
> might as well throw an error if people try to register one. This way
> people won't be surprised if the registration succeeds, but no event
> ever arrives.
>
> OK?
>
no warning for unused smtp_out ?
ok with ifdef-ing out smtp-out for now, i can bring it back.
for the record, smtp-out has been implemented and I have it running on my
machines but it requires a rework of the filter session registration (not
visible to users) which I'm not comfortable to do before 6.6.
> Index: lka_report.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/lka_report.c,v
> retrieving revision 1.26
> diff -u -p -r1.26 lka_report.c
> --- lka_report.c 28 Aug 2019 15:50:36 - 1.26
> +++ lka_report.c 29 Aug 2019 05:01:21 -
> @@ -107,10 +107,13 @@ lka_report_register_hook(const char *nam
> subsystem = _in;
> hook += 8;
> }
> +#if 0
> + /* No smtp-out event has been implemented yet */
> else if (strncmp(hook, "smtp-out|", 9) == 0) {
> subsystem = _out;
> hook += 9;
> }
> +#endif
> else
> fatalx("Invalid message direction: %s", hook);
Re: smtpd filters: banner hostname
28 août 2019 15:10 "Martijn van Duren" a écrit: > On 8/28/19 2:52 PM, gil...@poolp.org wrote: > >> Yes sorry my bad, but this still covers the textstring argument: >> >> We need to be able to reject at banner and this is going to be implemented >> as a filter phase, so basically if we expect people to need it for filters >> the proper place to pass your textstring to filters is in that filter phase. > > I'll commit it without the textstring. > ok ! > I just don't understand what you mean by reject at banner. > We only allow filtering on smtp-in and since we are the once that > produce the banner, how will you filter at that point? I still haven't fully gone through this but the bottom line is: - today we have report link-connect and we have filter phase connect - link-connect makes sense in getting info from the connection to build session, this is good - filter phase connect is kinda of weird as it gets the same info as the previous report event which is not right and it answers results in the banner displayed, it's kind of a strange filter phase in my opinion I think the filter phase connect should really be a filter phase "greeting" and this would get the session id to retrieve session state accumulated from report link-connect, as well as the banner as input, resulting in a proceed, rewrite, reject or disconnect action. It would make more sense but i haven't gone completely through this to make sure its correct, I'll make a proposal soon.
Re: smtpd filters: banner hostname
28 août 2019 14:07 "Martijn van Duren" a
écrit:
>>
>> So my understanding is that you'd like a filter to be able to use the
>> result from an SPF lookup to reject a message in your filter and have
>> the hostname that was used in the banner part of the message.
>
> Yes, because that's the only point in the transaction where I can be
> 100% certain on which hostname the transaction took place. This is
> because of listen on * hostnames , where table can change at
> runtime based on its backend.
>
Ok, so far so good :-)
>> If that's so, I think the diff below is both not enough and a bit too
>> specific, preventing filters with different use-cases than yours from
>> using the information as easily:
>>
>> 1- first, i think your report event is too specific, keep in mind the
>> report events are used to build the internal state of the session,
>> so that you can basically duplicate struct smtp_session from smtpd
>> in a filter. the textstring part here is not really related to the
>> state of the session more than it is related to your use-case.
>
> It's not related to my use-case. I included it for completeness.
> E.g. if someone wants to monitor what mailservers are being connected to
> in case of when we implement the smtp-out case.
> Since there's no uniform way to distil this information (it's freetext
> after all) I thought I'd return the textstring as specified by RFC5321.
> If you see harm in having the textstring part in the filters I don't
> mind dropping them, but I expect at some point people will ask for it.
>
There's no harm just as there's no harm adding more information to pretty
much any reporting event: "no harm" shouldn't be the argument.
The hostname makes a lot of sense because it is part of the session, this
means that currently you can't fully rebuild a session state in a filter,
we need a report event to allow filters to learn that hostname for sure.
The textstring part doesn't because:
a- it isn't a session state, the SMTP engine doesn't track it, it serves no
purpose in a session
b- ALL report events will have the same "ESMTP OpenSMTPD" value, it carries no
information, it is hardcoded
c- if there is a use-case that really required it, it can still be obtained
with protocol-server events
For b-, smtp-out will not be hardcoded but c- is still valid and if it so
happens that displaying the banner part is a very common use-case, we are
still allowed to create a specific report event.
For the time being, I think your diff without textstring is ok
>> 2- then, your use case is to allow rejecting with a custom message so
>> this can't be done with report events, which are informative, this
>> requires a filter event which allow taking decisions (like reject,
>> in this case). So I think that what's missing here is a phase that
>> allows filters to take a decision before printing the banner and I
>> think THIS is where your textstring goes, the filter receives that
>> report event with the hostname, it receives the filter request for
>> the banner and it can decide to write a custom banner.
>
> No, SPF filters at HELO/EHLO and MAIL FROM. If a host is rejected
> (because of a -all match) and the spf-record contains an "exp" (explain)
> entry it resolves fetches the txt record of where "exp" points to.
> Said record is then being evaluated by domain-spec and allows for the
> expansion of "%{r}", which evaluates to the domainname of the host
> doing the spf-checking.
>
Ok, so you can use actual filter hooks
> Say I would mail from openbsd+t...@imperialat.at to gil...@poolp.org and
> changed the txt imperialat.at to "v=spf1 mx -all exp:exp.imperialat.at"
> and txt exp.imperialat.at would be: "Message rejected by %{r}"
> If mx2.pool.org would reject the message because the sending ip is not
> listed in the spf imperialat.at, the resulting rejecting status would
> be:
> "550 5.7.1 Message rejected by mx2.poolp.org".
>
> See RFC7208 section 6.2, 7.2, 7,3 and 8.4
>
> So for the above I would need to know what the hostname of the
> parsing agent is. According to RFC5321 this information is available in
> the greeting directly after the status code. So if I were to register
> this on report|smtp-in|link-greeting I would have the domain name and I
> can cache this information on my session variable until I (potentially)
> need it during filter|helo, filter|ehlo and filter|mailfrom.
>
I understand and do you agree that link-greeting _without_ textstring is
not preventing you from doing that ?
Because nowhere is the textstring used in this pattern.
>> I may want to write filters that keep track of the hostname for which
>> the MX is operating and that do not necessarily do anything at banner
>> so to me tying banner stuff outside of a banner phase is not correct.
>>
>> Does it make sense ?
>
> I don't want to do anything at banner, I just want to know the banner,
> or more specifically, the hostname in the banner.
>
Yes sorry my bad, but this still covers the
Re: smtpd filters: banner hostname
28 août 2019 12:26 "Gilles Chehade" a écrit:
> On Wed, Aug 28, 2019 at 10:33:29AM +0200, Martijn van Duren wrote:
>
>> On 8/28/19 9:23 AM, gil...@poolp.org wrote:
>> 28 ao??t 2019 09:04 "Martijn van Duren" a
>> ??crit:
>>
>> Currently looking into writing an spf filter based on libopensmtpd.
>> While working through the spec I found in RFC7208 section 7.3 that:
>> The "r" macro expands to the name of the receiving MTA.
>> In other words the hostname presented in the banner. Unfortunately we
>> also support the hostnames directive, which supports ip-hostname
>> mappings via dynamic tables, which makes it impossible to transfer via
>> "config|".
>>
>> This is a major change that can break (and in the case of libopensmtpd
>> will break) parsers. We're currently at 0.1, so I don't know if we want
>> push it to 1 just yet, or if we want to call 1 release-stable and just
>> bump it to 0.2 for now since we don't have a release yet with filters.
>>
>> thoughts?
>>
>> I'm sorry but I'm unsure I understand what you're trying to do with the
>> banner,
>> can you explain ?
>>
>> If there's need for the hostname presented in the banner to be passed to
>> filters,
>> which makes sense, it needs its own reporting event which is basically the
>> server
>> side of the link-identify event.
>>
>> One thing for sure, you can't put that info in the link-connect event
>> because the
>> banner is displayed _after_ link-connect and while in smtp-in we already
>> know the
>> hostname we'll use in the banner, this isn't the case for smtp-out which
>> will not
>> be able to infer that information before actually seeing the banner.
>>
>> So the diff below implements report|link-greeting
>>
>> I haven't implemented the smtp-out case, since none of the smtp-out
>> cases appear to be currently implemented.
>>
>> Does this read better?
>
> No sorry, I'm very confused :-)
>
> Let me explain my understanding just to make sure we're talking about
> the same thing.
>
> You said:
>
>> spf has an "exp" modifier, which allows the reject message to be
>> specified by the spf administrator. To do so it can utilize several
>> macros. One option being "%{r}", which expands to:
>> domain name of host performing the check.
>
> So my understanding is that you'd like a filter to be able to use the
> result from an SPF lookup to reject a message in your filter and have
> the hostname that was used in the banner part of the message.
>
> If that's so, I think the diff below is both not enough and a bit too
> specific, preventing filters with different use-cases than yours from
> using the information as easily:
>
> 1- first, i think your report event is too specific, keep in mind the
> report events are used to build the internal state of the session,
> so that you can basically duplicate struct smtp_session from smtpd
> in a filter. the textstring part here is not really related to the
> state of the session more than it is related to your use-case.
>
> 2- then, your use case is to allow rejecting with a custom message so
> this can't be done with report events, which are informative, this
> requires a filter event which allow taking decisions (like reject,
> in this case). So I think that what's missing here is a phase that
> allows filters to take a decision before printing the banner and I
> think THIS is where your textstring goes, the filter receives that
> report event with the hostname, it receives the filter request for
> the banner and it can decide to write a custom banner.
>
> I may want to write filters that keep track of the hostname for which
> the MX is operating and that do not necessarily do anything at banner
> so to me tying banner stuff outside of a banner phase is not correct.
>
> Does it make sense ?
>
By the way, as a side note, I already had a use-case which required a
banner filtering phase to allow rejecting at banner and which doesn't
need the server hostname so this tends to go in favour of what I said
above:
my filter would not register to receive the server hostname because I
don't care about that, but I'd still need to get the banner so I will
be able to take a decision to disconnect with a different message.
Re: smtpd filters: banner hostname
+void
> +lka_report_smtp_link_greeting(const char *direction, uint64_t reqid,
> +struct timeval *tv, const char *domain, const char *textstring)
> +{
> + report_smtp_broadcast(reqid, direction, tv, "link-greeting", "%s|%s\n",
> + domain, textstring);
> }
>
> void
> Index: report_smtp.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/report_smtp.c,v
> retrieving revision 1.8
> diff -u -p -r1.8 report_smtp.c
> --- report_smtp.c 26 Jul 2019 06:30:13 - 1.8
> +++ report_smtp.c 28 Aug 2019 08:32:33 -
> @@ -64,6 +64,23 @@ report_smtp_link_connect(const char *dir
> }
>
> void
> +report_smtp_link_greeting(const char *direction, uint64_t qid, const char
> *domain,
> +const char *textstring)
> +{
> + struct timeval tv;
> +
> + gettimeofday(, NULL);
> +
> + m_create(p_lka, IMSG_REPORT_SMTP_LINK_GREETING, 0, 0, -1);
> + m_add_string(p_lka, direction);
> + m_add_timeval(p_lka, );
> + m_add_id(p_lka, qid);
> + m_add_string(p_lka, domain);
> + m_add_string(p_lka, textstring);
> + m_close(p_lka);
> +}
> +
> +void
> report_smtp_link_identify(const char *direction, uint64_t qid, const char
> *method, const char *identity)
> {
> struct timeval tv;
> Index: smtp_session.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v
> retrieving revision 1.407
> diff -u -p -r1.407 smtp_session.c
> --- smtp_session.c14 Aug 2019 21:11:25 - 1.407
> +++ smtp_session.c28 Aug 2019 08:32:33 -
> @@ -2046,8 +2046,11 @@ smtp_proceed_connected(struct smtp_sessi
> static void
> smtp_send_banner(struct smtp_session *s)
> {
> + char textstring[256];
> smtp_reply(s, "220 %s ESMTP %s", s->smtpname, SMTPD_NAME);
> s->banner_sent = 1;
> + snprintf(textstring, sizeof(textstring), "ESMTP %s", SMTPD_NAME);
> + report_smtp_link_greeting("smtp-in", s->id, s->smtpname, textstring);
> }
>
> void
> Index: smtpd.h
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.h,v
> retrieving revision 1.632
> diff -u -p -r1.632 smtpd.h
> --- smtpd.h 23 Aug 2019 07:09:52 - 1.632
> +++ smtpd.h 28 Aug 2019 08:32:33 -
> @@ -310,6 +310,7 @@ enum imsg_type {
>
> IMSG_REPORT_SMTP_LINK_CONNECT,
> IMSG_REPORT_SMTP_LINK_DISCONNECT,
> + IMSG_REPORT_SMTP_LINK_GREETING,
> IMSG_REPORT_SMTP_LINK_IDENTIFY,
> IMSG_REPORT_SMTP_LINK_TLS,
> IMSG_REPORT_SMTP_LINK_AUTH,
> @@ -1332,6 +1333,8 @@ void lka_report_register_hook(const char
> void lka_report_smtp_link_connect(const char *, struct timeval *, uint64_t,
> const char *, int,
> const struct sockaddr_storage *, const struct sockaddr_storage *);
> void lka_report_smtp_link_disconnect(const char *, struct timeval *,
> uint64_t);
> +void lka_report_smtp_link_greeting(const char *, uint64_t, struct timeval *,
> const char *,
> +const char *);
> void lka_report_smtp_link_identify(const char *, struct timeval *, uint64_t,
> const char *, const char *);
> void lka_report_smtp_link_tls(const char *, struct timeval *, uint64_t,
> const char *);
> void lka_report_smtp_link_auth(const char *, struct timeval *, uint64_t,
> const char *, const char *);
> @@ -1501,6 +1504,8 @@ int queue_message_walk(struct envelope *
> void report_smtp_link_connect(const char *, uint64_t, const char *, int,
> const struct sockaddr_storage *, const struct sockaddr_storage *);
> void report_smtp_link_disconnect(const char *, uint64_t);
> +void report_smtp_link_greeting(const char *, uint64_t, const char *,
> +const char *);
> void report_smtp_link_identify(const char *, uint64_t, const char *, const
> char *);
> void report_smtp_link_tls(const char *, uint64_t, const char *);
> void report_smtp_link_auth(const char *, uint64_t, const char *, const char
> *);
>
--
Gilles Chehade @poolpOrg
https://www.poolp.orgpatreon: https://www.patreon.com/gilles
Re: Make filter line handling more developer friendly
On Mon, Aug 26, 2019 at 12:04:06PM +0200, Martijn van Duren wrote:
> On 8/26/19 11:42 AM, Sebastien Marie wrote:
> > On Sun, Aug 25, 2019 at 07:01:01AM +0200, Martijn van Duren wrote:
> >> Right now all we get is "Misbehaving filter", which doesn't tell the
> >> developer a lot.
> >>
> >> Diff below does the following:
> >> - Make the register_hooks actually fail on misbehaviour.
> >> - Change some *ep = 0 to a more semantically correct ep[0] = '\0'
> >> - Hoist some checks from lka_filter_process_response to processor_io
> >> - Make lka_filter_process_response void (it fatals now)
> >> - strtoull returns ULLONG_MAX on error, not ULONG_MAX
> >> - change str{,n}casecmp to str{,n}cmp for consistency.
> >> - change an fugly "nextline:" "goto nextline" loop to an actual while.
> >> - restructure lka_filter_process_response to be shorter.
> >> and most importantly
> >> - Add a descriptive fatal() minefield.
> >>
> >> -12 LoC.
> >>
> >> Tested with filter-dnsbl with both a successful and rejected connection.
> >>
> >> OK?
> >
> > just one remark.
> >
> >> Index: lka_filter.c
> >> ===
> >> RCS file: /cvs/src/usr.sbin/smtpd/lka_filter.c,v
> >> retrieving revision 1.41
> >> diff -u -p -r1.41 lka_filter.c
> >> --- lka_filter.c 18 Aug 2019 16:52:02 - 1.41
> >> +++ lka_filter.c 25 Aug 2019 04:47:47 -
> >> @@ -429,88 +429,68 @@ lka_filter_process_response(const char *
> >>struct filter_session *fs;
> >>
> >>(void)strlcpy(buffer, line, sizeof buffer);
> >> - if ((ep = strchr(buffer, '|')) == NULL)
> >> - return 0;
> >> - *ep = 0;
> >> + ep = strchr(buffer, '|');
> >> + ep[0] = '\0';
> >
> > is it possible to buffer to not have '|' ? if yes, you could deference NULL.
> >
> >
> You're absolutely right.
> It's not a risk, since both would crash smtpd, but doing the check would
> result in a clearer error message, which is the purpose of this
> endeavour.
>
ok gilles@
> Index: lka_filter.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/lka_filter.c,v
> retrieving revision 1.41
> diff -u -p -r1.41 lka_filter.c
> --- lka_filter.c 18 Aug 2019 16:52:02 - 1.41
> +++ lka_filter.c 26 Aug 2019 10:03:43 -
> @@ -61,7 +61,7 @@ static void filter_result_reject(uint64_
> static void filter_result_disconnect(uint64_t, const char *);
>
> static void filter_session_io(struct io *, int, void *);
> -int lka_filter_process_response(const char *, const char *);
> +void lka_filter_process_response(const char *, const char *);
>
>
> struct filter_session {
> @@ -218,13 +218,13 @@ lka_filter_register_hook(const char *nam
> hook += 8;
> }
> else
> - return;
> + fatalx("Invalid message direction: %s", hook);
>
> for (i = 0; i < nitems(filter_execs); i++)
> if (strcmp(hook, filter_execs[i].phase_name) == 0)
> break;
> if (i == nitems(filter_execs))
> - return;
> + fatalx("Unrecognized report name: %s", hook);
>
> iter = NULL;
> while (dict_iter(, , _name, (void **)))
> @@ -414,7 +414,7 @@ filter_session_io(struct io *io, int evt
> }
> }
>
> -int
> +void
> lka_filter_process_response(const char *name, const char *line)
> {
> uint64_t reqid;
> @@ -430,87 +430,68 @@ lka_filter_process_response(const char *
>
> (void)strlcpy(buffer, line, sizeof buffer);
> if ((ep = strchr(buffer, '|')) == NULL)
> - return 0;
> - *ep = 0;
> + fatalx("Missing token: %s", line);
> + ep[0] = '\0';
>
> kind = buffer;
> - if (strcmp(kind, "register") == 0)
> - return 1;
> -
> - if (strcmp(kind, "filter-result") != 0 &&
> - strcmp(kind, "filter-dataline") != 0)
> - return 0;
>
> qid = ep+1;
> if ((ep = strchr(qid, '|')) == NULL)
> - return 0;
> - *ep = 0;
> + fatalx("Missing reqid: %s", line);
> + ep[0] = '\0';
>
> token = strtoull(qid, , 16);
> if (qid[0] == '\0' || *ep != '\0')
> - return 0;
> - if (errno == ERANGE && token ==
Re: smtpd filters: banner hostname
28 août 2019 09:04 "Martijn van Duren" a
écrit:
> Currently looking into writing an spf filter based on libopensmtpd.
> While working through the spec I found in RFC7208 section 7.3 that:
> The "r" macro expands to the name of the receiving MTA.
> In other words the hostname presented in the banner. Unfortunately we
> also support the hostnames directive, which supports ip-hostname
> mappings via dynamic tables, which makes it impossible to transfer via
> "config|".
>
> This is a major change that can break (and in the case of libopensmtpd
> will break) parsers. We're currently at 0.1, so I don't know if we want
> push it to 1 just yet, or if we want to call 1 release-stable and just
> bump it to 0.2 for now since we don't have a release yet with filters.
>
> thoughts?
>
I'm sorry but I'm unsure I understand what you're trying to do with the banner,
can you explain ?
If there's need for the hostname presented in the banner to be passed to
filters,
which makes sense, it needs its own reporting event which is basically the
server
side of the link-identify event.
One thing for sure, you can't put that info in the link-connect event because
the
banner is displayed _after_ link-connect and while in smtp-in we already know
the
hostname we'll use in the banner, this isn't the case for smtp-out which will
not
be able to infer that information before actually seeing the banner.
> Index: lka.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/lka.c,v
> retrieving revision 1.239
> diff -u -p -r1.239 lka.c
> --- lka.c 26 Jul 2019 06:30:13 - 1.239
> +++ lka.c 28 Aug 2019 06:28:33 -
> @@ -88,6 +88,7 @@ lka_imsg(struct mproc *p, struct imsg *i
> const char *heloname;
> const char *filter_name;
> const char *result;
> + const char *banner;
> struct sockaddr_storage ss_src, ss_dest;
> int filter_response;
> int filter_phase;
> @@ -388,9 +389,11 @@ lka_imsg(struct mproc *p, struct imsg *i
> m_get_int(, );
> m_get_sockaddr(, (struct sockaddr *)_src);
> m_get_sockaddr(, (struct sockaddr *)_dest);
> + m_get_string(, );
> m_end();
>
> - lka_report_smtp_link_connect(direction, , reqid, rdns, fcrdns, _src,
> _dest);
> + lka_report_smtp_link_connect(direction, , reqid, rdns,
> + fcrdns, _src, _dest, banner);
> return;
>
> case IMSG_REPORT_SMTP_LINK_DISCONNECT:
> Index: lka_report.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/lka_report.c,v
> retrieving revision 1.24
> diff -u -p -r1.24 lka_report.c
> --- lka_report.c 18 Aug 2019 16:52:02 - 1.24
> +++ lka_report.c 28 Aug 2019 06:28:33 -
> @@ -165,10 +165,10 @@ report_smtp_broadcast(uint64_t reqid, co
> }
>
> void
> -lka_report_smtp_link_connect(const char *direction, struct timeval *tv,
> uint64_t reqid, const char
> *rdns,
> - int fcrdns,
> +lka_report_smtp_link_connect(const char *direction, struct timeval *tv,
> + uint64_t reqid, const char *rdns, int fcrdns,
> const struct sockaddr_storage *ss_src,
> - const struct sockaddr_storage *ss_dest)
> + const struct sockaddr_storage *ss_dest, const char *banner)
> {
> char src[NI_MAXHOST + 5];
> char dest[NI_MAXHOST + 5];
> @@ -207,8 +207,8 @@ lka_report_smtp_link_connect(const char
> }
>
> report_smtp_broadcast(reqid, direction, tv, "link-connect",
> - "%016"PRIx64"|%s|%s|%s|%s\n",
> - reqid, rdns, fcrdns_str, src, dest);
> + "%016"PRIx64"|%s|%s|%s|%s|%s\n",
> + reqid, rdns, fcrdns_str, src, dest, banner);
> }
>
> void
> Index: report_smtp.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/report_smtp.c,v
> retrieving revision 1.8
> diff -u -p -r1.8 report_smtp.c
> --- report_smtp.c 26 Jul 2019 06:30:13 - 1.8
> +++ report_smtp.c 28 Aug 2019 06:28:33 -
> @@ -46,7 +46,7 @@
> void
> report_smtp_link_connect(const char *direction, uint64_t qid, const char
> *rdns, int fcrdns,
> const struct sockaddr_storage *ss_src,
> - const struct sockaddr_storage *ss_dest)
> + const struct sockaddr_storage *ss_dest, const char *banner)
> {
> struct timeval tv;
>
> @@ -60,6 +60,7 @@ report_smtp_link_connect(const char *dir
> m_add_int(p_lka, fcrdns);
> m_add_sockaddr(p_lka, (const struct sockaddr *)ss_src);
> m_add_sockaddr(p_lka, (const struct sockaddr *)ss_dest);
> + m_add_string(p_lka, banner);
> m_close(p_lka);
> }
>
> Index: smtp_session.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v
> retrieving revision 1.407
> diff -u -p -r1.407 smtp_session.c
> --- smtp_session.c 14 Aug 2019 21:11:25 - 1.407
> +++ smtp_session.c 28 Aug 2019 06:28:33 -
> @@ -2029,7 +2029,7 @@ smtp_connected(struct smtp_session *s)
> smtp_filter_begin(s);
>
> report_smtp_link_connect("smtp-in", s->id, s->rdns, s->fcrdns, >ss,
> - >listener->ss);
> + >listener->ss, s->smtpname);
>
> smtp_filter_phase(FILTER_CONNECT, s, ss_to_text(>ss));
> }
> Index: smtpd.h
>
Re: Make filter line handling more developer friendly
25 août 2019 07:01 "Martijn van Duren" a
écrit:
> Right now all we get is "Misbehaving filter", which doesn't tell the
> developer a lot.
>
Indeed
> Diff below does the following:
> - Make the register_hooks actually fail on misbehaviour.
> - Change some *ep = 0 to a more semantically correct ep[0] = '\0'
> - Hoist some checks from lka_filter_process_response to processor_io
> - Make lka_filter_process_response void (it fatals now)
> - strtoull returns ULLONG_MAX on error, not ULONG_MAX
> - change str{,n}casecmp to str{,n}cmp for consistency.
> - change an fugly "nextline:" "goto nextline" loop to an actual while.
> - restructure lka_filter_process_response to be shorter.
> and most importantly
> - Add a descriptive fatal() minefield.
>
> -12 LoC.
>
> Tested with filter-dnsbl with both a successful and rejected connection.
>
> OK?
>
> martijn@
>
Reads fine but I'll give it a better read today and run with it on my
machines for a day see if I observe any issues with filter-rspamd and
filter-senderscore as well as some builtin filters.
> Index: lka_filter.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/lka_filter.c,v
> retrieving revision 1.41
> diff -u -p -r1.41 lka_filter.c
> --- lka_filter.c 18 Aug 2019 16:52:02 - 1.41
> +++ lka_filter.c 25 Aug 2019 04:47:47 -
> @@ -61,7 +61,7 @@ static void filter_result_reject(uint64_
> static void filter_result_disconnect(uint64_t, const char *);
>
> static void filter_session_io(struct io *, int, void *);
> -int lka_filter_process_response(const char *, const char *);
> +void lka_filter_process_response(const char *, const char *);
>
> struct filter_session {
> @@ -218,13 +218,13 @@ lka_filter_register_hook(const char *nam
> hook += 8;
> }
> else
> - return;
> + fatalx("Invalid message direction: %s", hook);
>
> for (i = 0; i < nitems(filter_execs); i++)
> if (strcmp(hook, filter_execs[i].phase_name) == 0)
> break;
> if (i == nitems(filter_execs))
> - return;
> + fatalx("Unrecognized report name: %s", hook);
>
> iter = NULL;
> while (dict_iter(, , _name, (void **)))
> @@ -414,7 +414,7 @@ filter_session_io(struct io *io, int evt
> }
> }
>
> -int
> +void
> lka_filter_process_response(const char *name, const char *line)
> {
> uint64_t reqid;
> @@ -429,88 +429,68 @@ lka_filter_process_response(const char *
> struct filter_session *fs;
>
> (void)strlcpy(buffer, line, sizeof buffer);
> - if ((ep = strchr(buffer, '|')) == NULL)
> - return 0;
> - *ep = 0;
> + ep = strchr(buffer, '|');
> + ep[0] = '\0';
>
> kind = buffer;
> - if (strcmp(kind, "register") == 0)
> - return 1;
> -
> - if (strcmp(kind, "filter-result") != 0 &&
> - strcmp(kind, "filter-dataline") != 0)
> - return 0;
>
> qid = ep+1;
> if ((ep = strchr(qid, '|')) == NULL)
> - return 0;
> - *ep = 0;
> + fatalx("Missing reqid: %s", line);
> + ep[0] = '\0';
>
> token = strtoull(qid, , 16);
> if (qid[0] == '\0' || *ep != '\0')
> - return 0;
> - if (errno == ERANGE && token == ULONG_MAX)
> - return 0;
> + fatalx("Invalid token: %s", line);
> + if (errno == ERANGE && token == ULLONG_MAX)
> + fatal("Invalid token: %s", line);
>
> qid = ep+1;
> if ((ep = strchr(qid, '|')) == NULL)
> - return 0;
> - *ep = 0;
> + fatal("Missing directive: %s", line);
> + ep[0] = '\0';
>
> reqid = strtoull(qid, , 16);
> if (qid[0] == '\0' || *ep != '\0')
> - return 0;
> - if (errno == ERANGE && reqid == ULONG_MAX)
> - return 0;
> + fatalx("Invalid reqid: %s", line);
> + if (errno == ERANGE && reqid == ULLONG_MAX)
> + fatal("Invalid reqid: %s", line);
>
> response = ep+1;
>
> fs = tree_xget(, reqid);
> if (strcmp(kind, "filter-dataline") == 0) {
> if (fs->phase != FILTER_DATA_LINE)
> - fatalx("misbehaving filter");
> + fatalx("filter-dataline out of dataline phase");
> filter_data_next(token, reqid, response);
> - return 1;
> + return;
> }
> if (fs->phase == FILTER_DATA_LINE)
> - fatalx("misbehaving filter");
> + fatalx("filter-result in dataline phase");
>
> if ((ep = strchr(response, '|'))) {
> parameter = ep + 1;
> - *ep = 0;
> + ep[0] = '\0';
> }
>
> - if (strcmp(response, "proceed") != 0 &&
> - strcmp(response, "reject") != 0 &&
> - strcmp(response, "disconnect") != 0 &&
> - strcmp(response, "rewrite") != 0)
> - return 0;
> -
> - if (strcmp(response, "proceed") == 0 &&
> - parameter)
> - return 0;
> -
> - if (strcmp(response, "proceed") != 0 &&
> - parameter == NULL)
> - return 0;
> -
> - if (strcmp(response, "rewrite") == 0) {
> - filter_result_rewrite(reqid, parameter);
> - return 1;
> - }
> -
> - if (strcmp(response, "reject") == 0) {
> - filter_result_reject(reqid, parameter);
> - return 1;
> - }
> -
> - if (strcmp(response, "disconnect") == 0) {
> - filter_result_disconnect(reqid, parameter);
> - return 1;
> + if (strcmp(response, "proceed") == 0) {
> + if (parameter != NULL)
> + fatalx("Unexpected parameter after proceed: %s", line);
> + filter_protocol_next(token, reqid, 0);
> + return;
> + } else {
> + if (parameter == NULL)
> +
Re: smtpd filters: prettify proc-exec
On Fri, Aug 23, 2019 at 07:33:29PM +0200, Martijn van Duren wrote:
> On 8/23/19 7:06 PM, Gilles Chehade wrote:
> > On Fri, Aug 23, 2019 at 09:03:51AM +0200, Martijn van Duren wrote:
> >> Hello,
> >>
> >
> > Hello,
> >
> >
> >> When running processes with proc-exec any logging send over stderr is
> >> not very intelligible since the process name is automatically generated
> >> and results in , which make them hard to read.
> >>
> >
> > Agreed
> >
> >
> >> To clean this up I reckon we can do three things:
> >> 1) Add an extra optional proc parameter that sets the name. Diff below
> >>does this.
> >> 2) Change the process name to match the filter name.
> >> 3) Do both
> >>
> >> I reckon we're still early enough in the development process for us to
> >> change the default processor name in proc-exec.
> >>
> >> Thoughts?
> >>
> >
> > I'm unsure about 1- because it really reads cumbersome in my opinion:
> >
> >filter "rspamd" proc-exec proc "rspamd" "/usr/local/bin/filter-rspamd"
> >
> > Option 2- has my preference because it is really what you'd expect out
> > of the box, it was on my todo but if you're looking into it I'd rather
> > let you do it :-)
> >
> >
> I agree.
>
> That would be this diff.
>
> OK?
>
last_dynproc_id is no longer needed ?
nice
> Index: parse.y
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/parse.y,v
> retrieving revision 1.257
> diff -u -p -r1.257 parse.y
> --- parse.y 11 Aug 2019 17:23:12 - 1.257
> +++ parse.y 23 Aug 2019 17:33:17 -
> @@ -108,7 +108,6 @@ struct rule *rule;
> struct processor *processor;
> struct filter_config *filter_config;
> static uint32_t last_dynchain_id = 1;
> -static uint32_t last_dynproc_id = 1;
>
> enum listen_options {
> LO_FAMILY = 0x01,
> @@ -1598,12 +1597,6 @@ FILTER STRING PROC STRING {
> }
> |
> FILTER STRING PROC_EXEC STRING {
> - charbuffer[128];
> -
> - do {
> - (void)snprintf(buffer, sizeof buffer, "",
> last_dynproc_id++);
> - } while (dict_check(conf->sc_processors_dict, buffer));
> -
> if (dict_get(conf->sc_filters_dict, $2)) {
> yyerror("filter already exists with that name: %s", $2);
> free($2);
> @@ -1617,7 +1610,7 @@ FILTER STRING PROC_EXEC STRING {
> filter_config = xcalloc(1, sizeof *filter_config);
> filter_config->filter_type = FILTER_TYPE_PROC;
> filter_config->name = $2;
> - filter_config->proc = xstrdup(buffer);
> + filter_config->proc = xstrdup($2);
> dict_set(conf->sc_filters_dict, $2, filter_config);
> } proc_params {
> dict_set(conf->sc_processors_dict, filter_config->proc, processor);
>
--
Gilles Chehade @poolpOrg
https://www.poolp.orgpatreon: https://www.patreon.com/gilles
Re: smtpd filters: prettify proc-exec
On Fri, Aug 23, 2019 at 09:03:51AM +0200, Martijn van Duren wrote:
> Hello,
>
Hello,
> When running processes with proc-exec any logging send over stderr is
> not very intelligible since the process name is automatically generated
> and results in , which make them hard to read.
>
Agreed
> To clean this up I reckon we can do three things:
> 1) Add an extra optional proc parameter that sets the name. Diff below
>does this.
> 2) Change the process name to match the filter name.
> 3) Do both
>
> I reckon we're still early enough in the development process for us to
> change the default processor name in proc-exec.
>
> Thoughts?
>
I'm unsure about 1- because it really reads cumbersome in my opinion:
filter "rspamd" proc-exec proc "rspamd" "/usr/local/bin/filter-rspamd"
Option 2- has my preference because it is really what you'd expect out
of the box, it was on my todo but if you're looking into it I'd rather
let you do it :-)
> Index: parse.y
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/parse.y,v
> retrieving revision 1.257
> diff -u -p -r1.257 parse.y
> --- parse.y 11 Aug 2019 17:23:12 - 1.257
> +++ parse.y 23 Aug 2019 07:00:08 -
> @@ -1625,6 +1625,37 @@ FILTER STRING PROC_EXEC STRING {
> filter_config = NULL;
> }
> |
> +FILTER STRING PROC_EXEC PROC STRING STRING {
> + if (dict_get(conf->sc_processors_dict, $5)) {
> + yyerror("processor already exists with that name: %s", $5);
> + free($2);
> + free($5);
> + free($6);
> + YYERROR;
> + }
> +
> + if (dict_get(conf->sc_filters_dict, $2)) {
> + yyerror("filter already exists with that name: %s", $2);
> + free($2);
> + free($5);
> + free($6);
> + YYERROR;
> + }
> +
> + processor = xcalloc(1, sizeof *processor);
> + processor->command = $6;
> +
> + filter_config = xcalloc(1, sizeof *filter_config);
> + filter_config->filter_type = FILTER_TYPE_PROC;
> + filter_config->name = $2;
> + filter_config->proc = $5;
> + dict_set(conf->sc_filters_dict, $2, filter_config);
> +} proc_params {
> + dict_set(conf->sc_processors_dict, filter_config->proc, processor);
> + processor = NULL;
> + filter_config = NULL;
> +}
> +|
> FILTER STRING PHASE {
> if (dict_get(conf->sc_filters_dict, $2)) {
> yyerror("filter already exists with that name: %s", $2);
>
--
Gilles Chehade @poolpOrg
https://www.poolp.orgpatreon: https://www.patreon.com/gilles
fix inet6 displaying in smtpd
hello,
smtpd improperly displays inet6 addresses as reported by semarie@:
address=smtps://IPv6:2a01:e0c:1::25:465
fact is, helo response and Received headers display inet6 addresses
as IPv6:2a01:e0c:1::25 which initially led me to use this format in
smtpd instead of the proper one which is [2a01:e0c:1::25]
this diff ensures the proper format is used internally while having
a specialized ss_to_helo_text() function in smtp_session to use the
alternate format in the two places that actually need it.
special care taken in envelope.c to temporarily accept both formats
in disk envelopes for the transition phase.
diff tested and ok semarie@ who suggested I show it to a broader
audience.
Index: envelope.c
===
RCS file: /cvs/src/usr.sbin/smtpd/envelope.c,v
retrieving revision 1.43
diff -u -p -r1.43 envelope.c
--- envelope.c 3 Jul 2019 03:24:03 - 1.43
+++ envelope.c 11 Aug 2019 12:57:33 -
@@ -297,7 +297,16 @@ ascii_load_sockaddr(struct sockaddr_stor
ss->ss_family = AF_LOCAL;
}
else if (strncasecmp("IPv6:", buf, 5) == 0) {
+ /* XXX - remove this after 6.6 release */
if (inet_pton(AF_INET6, buf + 5, _addr) != 1)
+ return 0;
+ ssin6.sin6_family = AF_INET6;
+ memcpy(ss, , sizeof(ssin6));
+ ss->ss_len = sizeof(struct sockaddr_in6);
+ }
+ else if (buf[0] == '[' && buf[strlen(buf)-1] == ']') {
+ buf[strlen(buf)-1] = '\0';
+ if (inet_pton(AF_INET6, buf+1, _addr) != 1)
return 0;
ssin6.sin6_family = AF_INET6;
memcpy(ss, , sizeof(ssin6));
Index: smtp_session.c
===
RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v
retrieving revision 1.404
diff -u -p -r1.404 smtp_session.c
--- smtp_session.c 10 Aug 2019 16:07:01 - 1.404
+++ smtp_session.c 11 Aug 2019 12:57:34 -
@@ -283,6 +283,22 @@ static struct tree wait_ssl_verify;
static struct tree wait_filters;
static struct tree wait_filter_fd;
+static const char *
+ss_to_helo_text(const struct sockaddr_storage *ss)
+{
+ static char buf[NI_MAXHOST + 5];
+ static char helobuf[NI_MAXHOST + 5];
+
+ (void)strlcpy(buf, ss_to_text(ss), sizeof buf);
+
+ if (buf[0] != '[')
+ return buf;
+ buf[strlen(buf)-1] = '\0';
+
+ (void)snprintf(helobuf, sizeof helobuf, "IPv6:%s", buf+1);
+ return helobuf;
+}
+
static void
header_append_domain_buffer(char *buffer, char *domain, size_t len)
{
@@ -1739,7 +1755,7 @@ smtp_proceed_helo(struct smtp_session *s
smtp_reply(s, "250 %s Hello %s [%s], pleased to meet you",
s->smtpname,
s->helo,
- ss_to_text(>ss));
+ ss_to_helo_text(>ss));
}
static void
@@ -1756,7 +1772,7 @@ smtp_proceed_ehlo(struct smtp_session *s
smtp_reply(s, "250-%s Hello %s [%s], pleased to meet you",
s->smtpname,
s->helo,
- ss_to_text(>ss));
+ ss_to_helo_text(>ss));
smtp_reply(s, "250-8BITMIME");
smtp_reply(s, "250-ENHANCEDSTATUSCODES");
@@ -2805,7 +2821,7 @@ smtp_message_begin(struct smtp_tx *tx)
m_printf(tx, "from %s (%s [%s])",
s->helo,
s->rdns,
- ss_to_text(>ss));
+ ss_to_helo_text(>ss));
}
m_printf(tx, "\n\tby %s (%s) with %sSMTP%s%s id %08x",
s->smtpname,
Index: to.c
===
RCS file: /cvs/src/usr.sbin/smtpd/to.c,v
retrieving revision 1.38
diff -u -p -r1.38 to.c
--- to.c11 Aug 2019 10:54:44 - 1.38
+++ to.c11 Aug 2019 12:57:34 -
@@ -169,10 +169,9 @@ sa_to_text(const struct sockaddr *sa)
const struct in6_addr *in6_addr;
in6 = (const struct sockaddr_in6 *)sa;
- (void)strlcpy(buf, "IPv6:", sizeof(buf));
- p = buf + 5;
+ p = buf;
in6_addr = >sin6_addr;
- (void)bsnprintf(p, NI_MAXHOST, "%s", in6addr_to_text(in6_addr));
+ (void)bsnprintf(p, NI_MAXHOST, "[%s]",
in6addr_to_text(in6_addr));
}
return (buf);
--
Gilles Chehade @poolpOrg
https://www.poolp.orgpatreon: https://www.patreon.com/gilles
Re: smtpd: default to standard ports in relay-host
On Tue, Jul 23, 2019 at 12:15:26PM +0200, Klemens Nanni wrote:
> On Tue, Jul 23, 2019 at 11:00:04AM +0200, Gilles Chehade wrote:
> > there is no schema today that should default to 587, unless
> > submission:// is introduced as an alias to smtp+tls AND port 587.
> Alright; this way we guarantee not to break any setup as the default of
> 25 for everything is kept, except `smtps' which now picks 465.
>
> I also tweaked the wording, which seems well enough to me to ask for OKs.
>
One last change and the diff is ok gilles@:
/* need to specify an explicit port for LMTP */
if (relay->flags & RELAY_LMTP)
relay->port = 0;
This becomes unnecessary with your diff since you assign port to 0
for lmtp:// already.
> Index: smtpd.conf.5
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.conf.5,v
> retrieving revision 1.210
> diff -u -p -r1.210 smtpd.conf.5
> --- smtpd.conf.5 22 Dec 2018 08:54:02 - 1.210
> +++ smtpd.conf.5 23 Jul 2019 10:07:47 -
> @@ -250,9 +250,14 @@ Normal SMTP session with mandatory START
> Plain text SMTP session without TLS.
> .It lmtp
> LMTP session.
> +.Ar port
> +is required.
> .It smtps
> -SMTP session with forced TLS on connection.
> +SMTP session with forced TLS on connection, default port is 465.
> .El
> +Unless noted,
> +.Ar port
> +defaults to 25.
> .Pp
> The
> .Ar label
> Index: to.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/to.c,v
> retrieving revision 1.36
> diff -u -p -r1.36 to.c
> --- to.c 22 Jul 2019 23:01:48 - 1.36
> +++ to.c 23 Jul 2019 10:07:47 -
> @@ -305,16 +305,17 @@ text_to_relayhost(struct relayhost *rela
> const char *name;
> int tls;
> uint16_t flags;
> + uint16_t port;
> } schemas [] = {
> /*
>* new schemas should be *appended* otherwise the default
>* schema index needs to be updated later in this function.
>*/
> - { "smtp://",RELAY_TLS_OPPORTUNISTIC, 0
> },
> - { "smtp+tls://",RELAY_TLS_STARTTLS, 0
> },
> - { "smtp+notls://", RELAY_TLS_NO,0
> },
> - { "lmtp://",RELAY_TLS_NO,RELAY_LMTP
> },
> - { "smtps://", RELAY_TLS_SMTPS, 0
> }
> + { "smtp://",RELAY_TLS_OPPORTUNISTIC, 0,
> 25 },
> + { "smtp+tls://",RELAY_TLS_STARTTLS, 0,
> 25 },
> + { "smtp+notls://", RELAY_TLS_NO,0,
> 25 },
> + { "lmtp://",RELAY_TLS_NO,RELAY_LMTP,
> 0 },
> + { "smtps://", RELAY_TLS_SMTPS, 0,
> 465 }
> };
> const char *errstr = NULL;
> char *p, *q;
> @@ -346,6 +347,7 @@ text_to_relayhost(struct relayhost *rela
>
> relay->tls = schemas[i].tls;
> relay->flags = schemas[i].flags;
> + relay->port = schemas[i].port;
>
> /* need to specify an explicit port for LMTP */
> if (relay->flags & RELAY_LMTP)
>
--
Gilles Chehade @poolpOrg
https://www.poolp.orgpatreon: https://www.patreon.com/gilles
Re: Diff to stop using reserved words for smtpd.conf(5) examples
On Tue, Jul 23, 2019 at 08:51:38PM +0200, Ingo Schwarze wrote: > Hi Gilles, > > Gilles Chehade wrote on Tue, Jul 23, 2019 at 08:27:06AM +0200: > > On Mon, Jul 22, 2019 at 05:05:01PM -0400, Kurt Mosiejczuk wrote: > > >> This is a diff for that changes the example smtpd.conf and smtpd.conf.5 > >> so that it no longer uses words that are parts of the configuration > >> syntax as labels for actions. A large chunk of my delay to a release > >> with the new smtpd.conf configuration syntax was my confusion with the > >> examples. Even writing this diff I realized that the quotes were only > >> necessary in the examples because configuration grammar was being > >> reused as labels. > > > I'll let ingo and jmc comment on this, I think the rationale for > > using the quotes and the reserved words was precisely to show we > > should use the quotes at these places > > I do agree with showing quotes around action names and other > user-selected strings and identifiers. I think it helps understanding > by making keywords easier to distinguish from other, replaceable > strings. Even if in some cases, the syntax does not require the > quotes. > > > and that reserved words in quotes were acceptable. > > The smtpd.conf(5) manual explains that explicitly, and i think it > may make sense to show an example for it, even though i'm unsure > whether the practice of re-using reserved words as user-defined > identifiers should be recommended. > > But Theo's argument that it can be confusing to explain too many > different aspects in the same example makes sense to me, in particular > when the explanation of the example doesn't even mention all the > aspects being demonstrated. Also, Kurt's experience confirms that > this kind of confusion can indeed arise. > > > To be honest, ever since we made these examples, no one has ever > > mailed me with a broken config due to a reserved keyword whereas > > it used to be the case every now and then before. > > So, don't remove the quotes. > > Avoiding clashes that *require* quoting in most of the examples > as Kurt suggests might make sense. I don't feel strongly about > it either way. > > Also, giving one example (maybe at the end?) that explicitly shows > and mentions the possibility of using a quoted reserved word as > a user-selected string might make sense, too. Or maybe not, if > we do not want to encourage it? Do we? Not sure, no strong > feelings about that point either. > > I think there is a lot of room here for you to decide what you > consider good and robust style for writing this particular configuration > file. > Well I think we should remove the reserved keywords as suggested by Kurt but keep the quotes in all examples to make it very explicit that we are expecting a string literal at this point. I would be fine with his diff, where the new table names are enclosed in quotes. I don't think showing or hiding that we can use reserved words is really important, most people don't use reserved words, most people will either change the name or quote appropriately upon parse error, only a very few people need help with this, this is really a marginal case (people share a lot of smtpd.conf with me and I did some calls for sharing config when we were assessing the new configuration file format). -- Gilles Chehade @poolpOrg https://www.poolp.orgpatreon: https://www.patreon.com/gilles
Re: smtpd: Allow labels containing "@"
On Wed, Jul 24, 2019 at 12:18:05AM +0200, Klemens Nanni wrote:
> On Tue, Jul 23, 2019 at 09:06:33AM +0200, Gilles Chehade wrote:
> > On Tue, Jul 23, 2019 at 08:51:54AM +0200, Sebastien Marie wrote:
> > > it seems to me this url is wrong. the '@' in username should be
> > > urlencoded.
> > >
> > > smtps://klemens%40posteo...@posteo.de:465.
> OK, according to this it is indeed "wrong", but as gilles already noted
> we do not comply with any specification in this regard anyway.
>
No, I didn't say we don't comply with any specification, I said I didn't
have the RFC in mind because I tend to think of the relay url as if it's
an internal format for smtpd and nothing more. It doesn't mean that this
format wasn't chosen because it is standardized. Sorry, I wasn't clear.
If you take any tool or library that parses an url into components, that
will parse our relay url correctly. Similarly, if you use one that build
an url based on components, it will create a valid relay url if you skip
the (optional) password and the username is a valid label in smtpd.
>>> o = urlparse('smtp+tls://la...@mail.poolp.org:587')
>>> o.scheme, o.username, o.hostname, o.port
('smtp+tls', 'label', 'mail.poolp.org', 587)
>>> b = urlunparse(o)
>>> b
'smtp+tls://la...@mail.poolp.org:587'
>>>
At the very least we should make sure that changes to the relay url will
not break this and I'm not so sure after reading semarie@'s mail.
> > Just to clarify one thing, the "username" is not really a username, it is
> > the key used to lookup the username/password pair in a table.
> The current documentation does not impose any limitation on labels.
>
> > - maybe using @ in a label is not too practical
> Why not? It is indeed quite practical as my use case shows.
>
Yes, but you are only seeing YOUR very specific use case here.
The more generic use-case is that you are using an e-mail address as the
username and by allowing @ in labels you are allowing not only yours but
other users too, and @ is far from being the only annoying character you
can find in an e-mail address.
I said using @ in a label might not be practical because we're currently
able to hide behind the fact that we make rules about what a valid label
is, whereas when we add @ any e-mail address should be usable so we need
to make sure they can be expressed without breaking relay url.
As an example, if we allow @ and consider an e-mail acceptable for label
then we need to also allow _at least_ these ones:
#define MAILADDR_ALLOWED "!#$%&'*/?^`{|}~+-=_"
Maybe they aren't an issue, but it needs to be assessed.
> > - maybe it should be urlencoded indeed
> I agree with benno that this is the job of the user.
>
And it isn't today because we have the indirection.
> > I agree that since we have a format that looks somehow standard, we
> > should at least adhere to it.
> Adhere to what exactly?
>
Well very simple:
Today the urls smtpd deal with are RFC-compliant and we can parse, build
and validate them easily with various tools.
Does the change you propose affect that ?
Does it allow smtpd to load url that can't parse, build or validate ?
> As far as I can tell, my diff does not break anything. Existing
> `relay-url' values containing more than one "@" are invalid, hence
> cannot be in use.
>
Yes, and this is a side-effect of what you're proposing, which is adding
a new use-case. Your change does not break existing setups which is good
but doesn't mean that the change in itself is correct.
> Also, to.c:text_to_mailaddr() already uses
>
> 106 username = buffer;
> 107 hostname = strrchr(username, '@');
>
In this case only one @ can be found because a mailaddr is built from
localpart where @ is disallowed and domainpart where @ is disallowed,
the two joined by a @.
See below:
> where the username may contain "@". Consider the following example to
> demonstrate why my use case should be no different:
>
> # cat smtpd.conf
> table secrets file:/etc/mail/secrets
> action "relay" relay host smtps://klem...@posteo.de@posteo.de:465 auth
>
> ...
> # cat secrets
> klem...@posteo.de mysecret
> ...
>
No it may not contain @, if it does it means you have found a bug and we
are missing a call to valid_localpart() somehwere because a mailaddr, as
the name implies, is an e-mail address and two @ are not valid.
> From table(5) "Credentials tables"
>
> In a relay context, the credentials are a mapping of labels and
> username:password pairs, where the username may be omitted if identical
> to the label:
>
> label1 user:pas
Re: smtpd: Allow labels containing "@"
On Tue, Jul 23, 2019 at 10:20:10PM +0200, Sebastian Benoit wrote:
> Gilles Chehade(gil...@poolp.org) on 2019.07.23 09:06:33 +0200:
> > On Tue, Jul 23, 2019 at 08:51:54AM +0200, Sebastien Marie wrote:
> > > On Mon, Jul 22, 2019 at 11:26:28PM +0200, Klemens Nanni wrote:
> > > > My mail is klem...@posteo.de and the provider expects this full address
> > > > as username, so that makes for the following perfectly
> > > > valid SMTP URL smtps://klem...@posteo.de@posteo.de:465.
> > >
> > > it seems to me this url is wrong. the '@' in username should be
> > > urlencoded.
> > >
> > > smtps://klemens%40posteo...@posteo.de:465.
> > >
> > > RFC3986 Uniform Resource Identifier (URI): Generic Syntax
> > >
> > > 3.2.1. User Information
> > >
> > > The userinfo subcomponent may consist of a user name and, optionally,
> > > scheme-specific information about how to gain authorization to access
> > > the resource. The user information, if present, is followed by a
> > > commercial at-sign ("@") that delimits it from the host.
> > >
> > > userinfo= *( unreserved / pct-encoded / sub-delims / ":" )
> > >
> >
> > just to clarify one thing, the "username" is not really a username, it is
> > the key used to lookup the username/password pair in a table.
> >
> > that being said:
> >
> > - maybe using @ in a label is not too practical
> > - maybe it should be urlencoded indeed
> >
> > I didn't think about RFC3986 because we're really using the relay url as
> > an internal format between smtpd processes, not as a standardized format
> > so it could as well be schema=smtp,label=klem...@posteo.de,host=posteo.de
> >
> > I agree that since we have a format that looks somehow standard, we
> > should at least adhere to it.
>
> Why should i, when typing something in the config file have to care about
> url-encoding? Should this not just be done in the parser?
>
There are two things that are being mixed here.
1- you don't need to care about url-encoding, no one has ever had to use
url-encoding in credentials and as of today you can use whatever user
and password without relying on url-encoding ... because they are not
part of the relay url but rely on an indirection to a table. keep not
caring about url-encoding as much as I don't.
2- kn@ is bringing a NEW use-case where he wants to use the username for
the label, this raises questions because unlike the indirection, more
characters than just @ need to be escaped.
I see kn@ has sent another mail so I will explain further in my reply to
him.
--
Gilles Chehade @poolpOrg
https://www.poolp.orgpatreon: https://www.patreon.com/gilles
Re: smtpd: default to standard ports in relay-host
On Tue, Jul 23, 2019 at 10:54:57AM +0200, Klemens Nanni wrote:
> On Tue, Jul 23, 2019 at 08:18:18AM +0200, Gilles Chehade wrote:
> > it should definitely default to 25 in my opinion, disregarding if people
> > use "relay host" for submission or not.
> Fine with me; I just tried to be consistent with the rest of smtpd's
> default values.
>
> > relay host may be used for an MX hub (it's actually the case in my setup
> > which has public and internal MX), and it'd be very strange to be forced
> > to specify port 25 when you're doing SMTP relaying from MX to MX.
> Good example.
>
> > maybe we could default to 587 if we detect smtp+tls and an auth label ?
> That sounds like having one schema default to two different ports
> depending on the detected protocol; I considered for a moment, but others
> mentioned the same critics I had: smtpd should not be too clever.
>
> > or maybe we should have submission:// which is smtp+tls AND port 587 ?
> This seems sensible, although I'd like to handle the addition of schemas
> separately so we can fix the existing behaviour first.
>
> > well yes, objections on that one, by defaulting smtp+tls to 25 you broke
> > three of my setups :-)
> Updated diff changing it to 587. Would that be OK code-wise?
>
sorry sorry sorry, I didn't have my coffee, I meant _exactly_ the opposite:
by defaulting smtp+tls to 587, you broke three of my setups.
there is no schema today that should default to 587, unless
submission:// is introduced as an alias to smtp+tls AND port 587.
> Index: smtpd.conf.5
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.conf.5,v
> retrieving revision 1.210
> diff -u -p -r1.210 smtpd.conf.5
> --- smtpd.conf.5 22 Dec 2018 08:54:02 - 1.210
> +++ smtpd.conf.5 23 Jul 2019 08:54:33 -
> @@ -242,16 +242,15 @@ The following protocols are available:
> .Pp
> .Bl -tag -width "smtp+notls" -compact
> .It smtp
> -Normal SMTP session with opportunistic STARTTLS
> -(the default).
> +Normal SMTP session with opportunistic STARTTLS on port 587 (the default).
> .It smtp+tls
> -Normal SMTP session with mandatory STARTTLS.
> +Normal SMTP session with mandatory STARTTLS on port 587.
> .It smtp+notls
> -Plain text SMTP session without TLS.
> +Plain text SMTP session without TLS on port 25.
> .It lmtp
> LMTP session.
> .It smtps
> -SMTP session with forced TLS on connection.
> +SMTP session with forced TLS on connection on port 465.
> .El
> .Pp
> The
> Index: to.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/to.c,v
> retrieving revision 1.36
> diff -u -p -r1.36 to.c
> --- to.c 22 Jul 2019 23:01:48 - 1.36
> +++ to.c 23 Jul 2019 08:54:33 -
> @@ -305,16 +305,17 @@ text_to_relayhost(struct relayhost *rela
> const char *name;
> int tls;
> uint16_t flags;
> + uint16_t port;
> } schemas [] = {
> /*
>* new schemas should be *appended* otherwise the default
>* schema index needs to be updated later in this function.
>*/
> - { "smtp://",RELAY_TLS_OPPORTUNISTIC, 0
> },
> - { "smtp+tls://",RELAY_TLS_STARTTLS, 0
> },
> - { "smtp+notls://", RELAY_TLS_NO,0
> },
> - { "lmtp://",RELAY_TLS_NO,RELAY_LMTP
> },
> - { "smtps://", RELAY_TLS_SMTPS, 0
> }
> + { "smtp://",RELAY_TLS_OPPORTUNISTIC, 0,
> 587 },
> + { "smtp+tls://",RELAY_TLS_STARTTLS, 0,
> 587 },
> + { "smtp+notls://", RELAY_TLS_NO,0,
> 25 },
> + { "lmtp://",RELAY_TLS_NO,RELAY_LMTP,
> 0 },
> + { "smtps://", RELAY_TLS_SMTPS, 0,
> 465 }
> };
> const char *errstr = NULL;
> char *p, *q;
> @@ -346,6 +347,7 @@ text_to_relayhost(struct relayhost *rela
>
> relay->tls = schemas[i].tls;
> relay->flags = schemas[i].flags;
> + relay->port = schemas[i].port;
>
> /* need to specify an explicit port for LMTP */
> if (relay->flags & RELAY_LMTP)
>
--
Gilles Chehade @poolpOrg
https://www.poolp.orgpatreon: https://www.patreon.com/gilles
Re: smtpd: Allow labels containing "@"
On Tue, Jul 23, 2019 at 08:51:54AM +0200, Sebastien Marie wrote:
> On Mon, Jul 22, 2019 at 11:26:28PM +0200, Klemens Nanni wrote:
> > My mail is klem...@posteo.de and the provider expects this full address
> > as username, so that makes for the following perfectly
> > valid SMTP URL smtps://klem...@posteo.de@posteo.de:465.
>
> it seems to me this url is wrong. the '@' in username should be urlencoded.
>
> smtps://klemens%40posteo...@posteo.de:465.
>
> RFC3986 Uniform Resource Identifier (URI): Generic Syntax
>
> 3.2.1. User Information
>
> The userinfo subcomponent may consist of a user name and, optionally,
> scheme-specific information about how to gain authorization to access
> the resource. The user information, if present, is followed by a
> commercial at-sign ("@") that delimits it from the host.
>
> userinfo= *( unreserved / pct-encoded / sub-delims / ":" )
>
just to clarify one thing, the "username" is not really a username, it is
the key used to lookup the username/password pair in a table.
that being said:
- maybe using @ in a label is not too practical
- maybe it should be urlencoded indeed
I didn't think about RFC3986 because we're really using the relay url as
an internal format between smtpd processes, not as a standardized format
so it could as well be schema=smtp,label=klem...@posteo.de,host=posteo.de
I agree that since we have a format that looks somehow standard, we
should at least adhere to it.
my previous ok is withdrawned, sorry kn@
> > I've been doing that with mutt(1) for ages.
> >
> > smtpd.conf(5) has the following syntax:
> >
> > host relay-url
> > Do not perform MX lookups but relay messages to the relay
> > host described by relay-url. The format for relay-url is
> > [proto://[label@]]host[:port]. The following protocols
> > are available:
> >
> > yielding the following config in my case:
> >
> > action "relay" relay host smtps://klem...@posteo.de@posteo.de:465 auth
> >
> >
> > However, when parsing the label in the `relay-url', smtpd(8) stops at
> > the first "@" sign, not expecting labels to contain it. The following
> > diff fixes this spanning the label to the last occurence of "@" as is
> > already done in other code places.
> >
> > Feedback? Objections?
> > OK?
> >
> > Index: to.c
> > ===
> > RCS file: /cvs/src/usr.sbin/smtpd/to.c,v
> > retrieving revision 1.35
> > diff -u -p -r1.35 to.c
> > --- to.c30 Dec 2018 23:09:58 - 1.35
> > +++ to.c22 Jul 2019 21:02:48 -
> > @@ -352,7 +352,7 @@ text_to_relayhost(struct relayhost *rela
> > relay->port = 0;
> >
> > /* first, we extract the label if any */
> > - if ((q = strchr(p, '@')) != NULL) {
> > + if ((q = strrchr(p, '@')) != NULL) {
> > *q = 0;
> > if (strlcpy(relay->authlabel, p, sizeof (relay->authlabel))
> > >= sizeof (relay->authlabel))
> >
>
> --
> Sebastien Marie
>
--
Gilles Chehade @poolpOrg
https://www.poolp.orgpatreon: https://www.patreon.com/gilles
Re: Diff to stop using reserved words for smtpd.conf(5) examples
On Mon, Jul 22, 2019 at 05:05:01PM -0400, Kurt Mosiejczuk wrote: > This is a diff for that changes the example smtpd.conf and smtpd.conf.5 > so that it no longer uses words that are parts of the configuration > syntax as labels for actions. A large chunk of my delay to a release > with the new smtpd.conf configuration syntax was my confusion with the > examples. Even writing this diff I realized that the quotes were only > necessary in the examples because configuration grammar was being > reused as labels. > I'll let ingo and jmc comment on this, I think the rationale for using the quotes and the reserved words was precisely to show we should use the quotes at these places and that reserved words in quotes were acceptable. To be honest, ever since we made these examples, no one has ever mailed me with a broken config due to a reserved keyword whereas it used to be the case every now and then before. > Index: etc/mail/smtpd.conf > === > RCS file: /cvs/src/etc/mail/smtpd.conf,v > retrieving revision 1.11 > diff -u -p -r1.11 smtpd.conf > --- etc/mail/smtpd.conf 4 Jun 2018 21:10:58 - 1.11 > +++ etc/mail/smtpd.conf 22 Jul 2019 20:58:24 - > @@ -9,11 +9,11 @@ table aliases file:/etc/mail/aliases > # > listen on lo0 > > -action "local" mbox alias > -action "relay" relay > +action local-mail mbox alias > +action inet-mail relay > > # Uncomment the following to accept external mail for domain "example.org" > # > -# match from any for domain "example.org" action "local" > -match for local action "local" > -match for any action "relay" > +# match from any for domain "example.org" action local-mail > +match for local action local-mail > +match for any action inet-mail > Index: usr.sbin/smtpd/smtpd.conf.5 > === > RCS file: /cvs/src/usr.sbin/smtpd/smtpd.conf.5,v > retrieving revision 1.210 > diff -u -p -r1.210 smtpd.conf.5 > --- usr.sbin/smtpd/smtpd.conf.5 22 Dec 2018 08:54:02 - 1.210 > +++ usr.sbin/smtpd/smtpd.conf.5 22 Jul 2019 20:58:24 - > @@ -871,12 +871,12 @@ table secrets file:/etc/mail/secrets > > listen on lo0 > > -action "local" mbox alias > -action "relay" relay host smtp+tls://b...@smtp.example.com \e > +action local-mail mbox alias > +action inet-mail relay host smtp+tls://b...@smtp.example.com \e > auth > > -match for local action "local" > -match for any action "relay" > +match for local action local-mail > +match for any action inet-mail > .Ed > .Pp > In this second example, > @@ -908,12 +908,12 @@ listen on egress tls pki mail.example.co > > action mda_with_aliases mda "/path/to/mda \-f \-" alias > action mda_without_aliases mda "/path/to/mda \-f \-" > -action "relay" relay > +action inet-mail relay > > match for local action mda_with_aliases > match from any for domain example.com action mda_without_aliases > -match for any action "relay" > -match auth from any for any action "relay" > +match for any action inet-mail > +match auth from any for any action inet-mail > .Ed > .Pp > For sites that wish to sign messages using DKIM, the > @@ -929,12 +929,12 @@ table aliases file:/etc/mail/aliases > listen on lo0 > listen on lo0 port 10028 tag DKIM > > -action "mbox" mbox alias > -action "relay" relay > +action local-mbox mbox alias > +action inet-mail relay > action relay_dkim relay host smtp://127.0.0.1:10027 > > -match for local action "mbox" > -match tag DKIM for any action "relay" > +match for local action local-mbox > +match tag DKIM for any action inet-mail > match for any action relay_dkim > .Ed > .Pp > @@ -952,14 +952,14 @@ table other-relays file:/etc/mail/other- > listen on lo0 > listen on egress > > -action "mbox" mbox alias > -action "relay" relay > +action local-mbox mbox alias > +action inet-mail relay > > -match for local action "mbox" > -match for any action "relay" > +match for local action local-mbox > +match for any action inet-mail > match !from src mail\-from "@example.com" for any \e >reject > -match from any for domain example.com action "mbox" > +match from any for domain example.com action local-mbox > .Ed > .Sh SEE ALSO > .Xr mailer.conf 5 , > -- Gilles Chehade @poolpOrg https://www.poolp.orgpatreon: https://www.patreon.com/gilles
Re: smtpd: default to standard ports in relay-host
s function.
>*/
> - { "smtp://",RELAY_TLS_OPPORTUNISTIC, 0
> },
> - { "smtp+tls://",RELAY_TLS_STARTTLS, 0
> },
> - { "smtp+notls://", RELAY_TLS_NO,0
> },
> - { "lmtp://",RELAY_TLS_NO,RELAY_LMTP
> },
> - { "smtps://", RELAY_TLS_SMTPS, 0
> }
> + { "smtp://",RELAY_TLS_OPPORTUNISTIC, 0,
> 25 },
> + { "smtp+tls://",RELAY_TLS_STARTTLS, 0,
> 587 },
> + { "smtp+notls://", RELAY_TLS_NO,0,
> 25 },
> + { "lmtp://",RELAY_TLS_NO,RELAY_LMTP,
> 0 },
> + { "smtps://", RELAY_TLS_SMTPS, 0,
> 465 }
> };
> const char *errstr = NULL;
> char *p, *q;
> @@ -346,6 +347,7 @@ text_to_relayhost(struct relayhost *rela
>
> relay->tls = schemas[i].tls;
> relay->flags = schemas[i].flags;
> + relay->port = schemas[i].port;
>
> /* need to specify an explicit port for LMTP */
> if (relay->flags & RELAY_LMTP)
>
--
Gilles Chehade @poolpOrg
https://www.poolp.orgpatreon: https://www.patreon.com/gilles
Re: smtpd: Use IPPORT_HILASTAUTO not 0xffff
On Tue, Jul 23, 2019 at 12:20:04AM +0200, Klemens Nanni wrote:
> More mnemonic and readable.
>
indeed, I didn't know about IPPORT_HILASTAUTO :-)
> OK?
yes, ok
> Index: to.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/to.c,v
> retrieving revision 1.35
> diff -u -p -r1.35 to.c
> --- to.c 30 Dec 2018 23:09:58 - 1.35
> +++ to.c 22 Jul 2019 22:16:29 -
> @@ -385,7 +385,7 @@ text_to_relayhost(struct relayhost *rela
> /* finally, we extract the port */
> p = beg + len;
> if (*p == ':') {
> - relay->port = strtonum(p+1, 1, 0x, );
> + relay->port = strtonum(p+1, 1, IPPORT_HILASTAUTO, );
> if (errstr)
> return 0;
> }
>
--
Gilles Chehade @poolpOrg
https://www.poolp.orgpatreon: https://www.patreon.com/gilles
Re: smtpd: Allow labels containing "@"
On Mon, Jul 22, 2019 at 11:26:28PM +0200, Klemens Nanni wrote:
> My mail is klem...@posteo.de and the provider expects this full address
> as username, so that makes for the following perfectly
> valid SMTP URL smtps://klem...@posteo.de@posteo.de:465.
>
> I've been doing that with mutt(1) for ages.
>
> smtpd.conf(5) has the following syntax:
>
> host relay-url
> Do not perform MX lookups but relay messages to the relay
> host described by relay-url. The format for relay-url is
> [proto://[label@]]host[:port]. The following protocols
> are available:
>
> yielding the following config in my case:
>
> action "relay" relay host smtps://klem...@posteo.de@posteo.de:465 auth
>
>
> However, when parsing the label in the `relay-url', smtpd(8) stops at
> the first "@" sign, not expecting labels to contain it. The following
> diff fixes this spanning the label to the last occurence of "@" as is
> already done in other code places.
>
> Feedback? Objections?
> OK?
>
no objection, ok
> Index: to.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/to.c,v
> retrieving revision 1.35
> diff -u -p -r1.35 to.c
> --- to.c 30 Dec 2018 23:09:58 - 1.35
> +++ to.c 22 Jul 2019 21:02:48 -
> @@ -352,7 +352,7 @@ text_to_relayhost(struct relayhost *rela
> relay->port = 0;
>
> /* first, we extract the label if any */
> - if ((q = strchr(p, '@')) != NULL) {
> + if ((q = strrchr(p, '@')) != NULL) {
> *q = 0;
> if (strlcpy(relay->authlabel, p, sizeof (relay->authlabel))
> >= sizeof (relay->authlabel))
>
--
Gilles Chehade @poolpOrg
https://www.poolp.orgpatreon: https://www.patreon.com/gilles
Re: smtpd replace mkstemp+fdopen with tmpfile
On Mon, Jul 01, 2019 at 08:25:02PM +0200, Martijn van Duren wrote:
> No functional change intended, just simplify the code.
>
> OK?
>
yes, ok gilles@
> Index: enqueue.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/enqueue.c,v
> retrieving revision 1.115
> diff -u -p -r1.115 enqueue.c
> --- enqueue.c 31 May 2018 21:06:12 - 1.115
> +++ enqueue.c 1 Jul 2019 18:23:02 -
> @@ -171,8 +171,6 @@ enqueue(int argc, char *argv[], FILE *of
> FILE*fp = NULL, *fout;
> size_t sz = 0, envid_sz = 0;
> ssize_t len;
> - int fd;
> - char sfn[] = "/tmp/smtpd.XX";
> char*line;
> int dotted;
> int inheaders = 1;
> @@ -269,16 +267,9 @@ enqueue(int argc, char *argv[], FILE *of
> argc--;
> }
>
> - if ((fd = mkstemp(sfn)) == -1 ||
> - (fp = fdopen(fd, "w+")) == NULL) {
> - int saved_errno = errno;
> - if (fd != -1) {
> - unlink(sfn);
> - close(fd);
> - }
> - errc(EX_UNAVAILABLE, saved_errno, "mkstemp");
> - }
> - unlink(sfn);
> + if ((fp = tmpfile()) == NULL)
> + err(EX_UNAVAILABLE, "tmpfile");
> +
> msg.noheader = parse_message(stdin, fake_from == NULL, tflag, fp);
>
> if (msg.rcpt_cnt == 0)
> Index: smtpc.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtpc.c,v
> retrieving revision 1.5
> diff -u -p -r1.5 smtpc.c
> --- smtpc.c 15 May 2019 05:02:43 - 1.5
> +++ smtpc.c 1 Jul 2019 18:23:02 -
> @@ -263,19 +263,12 @@ parse_server(char *server)
> void
> parse_message(FILE *ifp)
> {
> - char sfn[] = "/tmp/smtp.XX";
> char *line = NULL;
> size_t linesz = 0;
> ssize_t len;
> - int fd;
>
> - if ((fd = mkstemp(sfn)) == -1)
> - fatal("mkstemp");
> - unlink(sfn);
> -
> - mail.fp = fdopen(fd, "w+");
> - if ((mail.fp) == NULL)
> - fatal("fdopen");
> + if ((mail.fp = tmpfile()) == NULL)
> + fatal("tmpfile");
>
> for (;;) {
> if ((len = getline(, , ifp)) == -1) {
> Index: smtpctl.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtpctl.c,v
> retrieving revision 1.163
> diff -u -p -r1.163 smtpctl.c
> --- smtpctl.c 14 Jan 2019 09:37:40 - 1.163
> +++ smtpctl.c 1 Jul 2019 18:23:02 -
> @@ -1297,20 +1297,10 @@ display(const char *s)
>
> if (is_encrypted_fp(fp)) {
> int i;
> - int fd;
> FILE *ofp = NULL;
> - charsfn[] = "/tmp/smtpd.XX";
>
> - if ((fd = mkstemp(sfn)) == -1 ||
> - (ofp = fdopen(fd, "w+")) == NULL) {
> - int saved_errno = errno;
> - if (fd != -1) {
> - unlink(sfn);
> - close(fd);
> - }
> - errc(1, saved_errno, "mkstemp");
> - }
> - unlink(sfn);
> + if ((ofp = tmpfile()) == NULL)
> + err(1, "tmpfile");
>
> for (i = 0; i < 3; i++) {
> key = getpass("key> ");
>
--
Gilles Chehade @poolpOrg
https://www.poolp.orgpatreon: https://www.patreon.com/gilles
Re: smtpd fix proc filter chaining with proceed
On Sun, Jun 30, 2019 at 04:54:28PM +0200, Martijn van Duren wrote:
> Found by Mischa Peters by running some experimental code of mine.
>
> When chaining two proc-based filters together the second proc will not
> get its parameter correctly, since lka_filter_process_response won't get
> a parameter from the proceed keyword, resulting in a "...|(null)"
>
> The solution seems to be to save the parameter in the filter_session
> struct.
>
> While here remove some useless if(NULL) checks and fix two memory leaks
> (fs->helo and fs->mail_from).
>
> Mischa has been running with this diff for several days now without
> issue.
>
> OK?
>
yes ok gilles@
> Index: lka_filter.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/lka_filter.c,v
> retrieving revision 1.36
> diff -u -p -r1.36 lka_filter.c
> --- lka_filter.c 2 May 2019 11:39:45 - 1.36
> +++ lka_filter.c 30 Jun 2019 14:50:58 -
> @@ -41,7 +41,7 @@ struct filter;
> struct filter_session;
> static void filter_protocol_internal(struct filter_session *, uint64_t *,
> uint64_t, enum filter_phase, const char *);
> static void filter_protocol(uint64_t, enum filter_phase, const char *);
> -static void filter_protocol_next(uint64_t, uint64_t, enum filter_phase,
> const char *);
> +static void filter_protocol_next(uint64_t, uint64_t, enum filter_phase);
> static void filter_protocol_query(struct filter *, uint64_t, uint64_t,
> const char *, const char *);
>
> static void filter_data_internal(struct filter_session *, uint64_t,
> uint64_t, const char *);
> @@ -68,6 +68,8 @@ struct filter_session {
> uint64_tid;
> struct io *io;
>
> + char *lastparam;
> +
> char *filter_name;
> struct sockaddr_storage ss_src;
> struct sockaddr_storage ss_dest;
> @@ -337,6 +339,9 @@ lka_filter_end(uint64_t reqid)
>
> fs = tree_xpop(, reqid);
> free(fs->rdns);
> + free(fs->helo);
> + free(fs->mail_from);
> + free(fs->lastparam);
> free(fs);
> log_trace(TRACE_FILTERS, "%016"PRIx64" filters session-end", reqid);
> }
> @@ -504,7 +509,7 @@ lka_filter_process_response(const char *
> return 1;
> }
>
> - filter_protocol_next(token, reqid, 0, parameter);
> + filter_protocol_next(token, reqid, 0);
> return 1;
> }
>
> @@ -658,14 +663,11 @@ filter_protocol(uint64_t reqid, enum fil
> switch (phase) {
> case FILTER_HELO:
> case FILTER_EHLO:
> - if (fs->helo)
> - free(fs->helo);
> + free(fs->helo);
> fs->helo = xstrdup(param);
> break;
> case FILTER_MAIL_FROM:
> - if (fs->mail_from)
> - free(fs->mail_from);
> -
> + free(fs->mail_from);
> fs->mail_from = xstrdup(param + 1);
> *strchr(fs->mail_from, '>') = '\0';
> param = fs->mail_from;
> @@ -683,13 +685,17 @@ filter_protocol(uint64_t reqid, enum fil
> default:
> break;
> }
> +
> + free(fs->lastparam);
> + fs->lastparam = xstrdup(param);
> +
> filter_protocol_internal(fs, , reqid, phase, param);
> if (nparam)
> free(nparam);
> }
>
> static void
> -filter_protocol_next(uint64_t token, uint64_t reqid, enum filter_phase
> phase, const char *param)
> +filter_protocol_next(uint64_t token, uint64_t reqid, enum filter_phase phase)
> {
> struct filter_session *fs;
>
> @@ -697,7 +703,7 @@ filter_protocol_next(uint64_t token, uin
> if ((fs = tree_get(, reqid)) == NULL)
> return;
>
> - filter_protocol_internal(fs, , reqid, phase, param);
> + filter_protocol_internal(fs, , reqid, phase, fs->lastparam);
> }
>
> static void
--
Gilles Chehade @poolpOrg
https://www.poolp.orgpatreon: https://www.patreon.com/gilles
Re: tls_load_file.3
On Mon, Jun 17, 2019 at 10:12:52AM +0200, alf wrote: > Hello, > > while adding a missing "the" for tls_config_set_cert_file > it appeared to me that "file" and "filename" are used > inconsistent. I went with "file" since "filename" to my > eyes/ears implies the filename without path, however that > maybe wrong. > > Alf > > Index: lib/libtls/man/tls_load_file.3 > === > RCS file: /cvs/src/lib/libtls/man/tls_load_file.3,v > retrieving revision 1.11 > diff -u -p -r1.11 tls_load_file.3 > --- lib/libtls/man/tls_load_file.329 Nov 2018 14:24:23 - 1.11 > +++ lib/libtls/man/tls_load_file.317 Jun 2019 08:07:53 - > @@ -217,7 +217,7 @@ call, ensuring that the memory contents > returns the path of the file that contains the default root certificates. > .Pp > .Fn tls_config_set_ca_file > -sets the filename used to load a file > +sets the file used to load a file IMO "set the file used to load a file" reads very weird > containing the root certificates. > .Pp > .Fn tls_config_set_ca_path > @@ -228,13 +228,13 @@ certificates. > sets the root certificates directly from memory. > .Pp > .Fn tls_config_set_cert_file > -sets file from which the public certificate will be read. > +sets the file from which the public certificate will be read. I don't see what was wrong here :-/ > .Pp > .Fn tls_config_set_cert_mem > sets the public certificate directly from memory. > .Pp > .Fn tls_config_set_crl_file > -sets the filename used to load a file containing the > +sets the file used to load a file containing the same > Certificate Revocation List (CRL). > .Pp > .Fn tls_config_set_crl_mem > -- Gilles Chehade @poolpOrg https://www.poolp.orgpatreon: https://www.patreon.com/gilles
Re: [patch] use acme-client to sign certificated with ecdsa keys
On Fri, Jun 14, 2019 at 03:54:38PM +0200, Florian Obser wrote:
> On Fri, Jun 14, 2019 at 02:04:00PM +0200, Renaud Allard wrote:
> >
> >
> > On 6/14/19 1:58 PM, Florian Obser wrote:
> > > On Fri, Jun 14, 2019 at 09:50:35AM +0200, Renaud Allard wrote:
> > > >
> > > >
> > > > On 6/12/19 2:30 PM, Renaud Allard wrote:
> > > > >
> > > > >
> > > > > On 6/11/19 2:36 PM, Sebastian Benoit wrote:
> > > > > > Hi,
> > > > > >
> > > > > > some feedback below.
> > > > > >
> > > > > > Renaud: maybe wait for feedback from florian or gilles until
> > > > > > acting on my comments, sometimes sending diffs to fast creates more
> > > > > > work ;)
> > > > > >
> > > > > > /Benno
> > > > > >
> > > > >
> > > > > As suggested by benno@
> > > > > removal of the global variable
> > > > > removal of KEYTYPE which was not used and was a leftover of a former
> > > > > patch
> > > > > define ECDSA_KEY to be more readable
> > > > >
> > > >
> > > > Any comment or OK on my latest patch?
> > > >
> > >
> > > I'd prefer to use enums like the rest of the code.
> > >
> >
> > Indeed, that seems even more explicit. I can't say an official OK, but
> > that's OK to me :)
> >
> >
>
>
> Bit more tweaking to the parse.y. This makes keytype reusable for the
> account key.
>
yes makes sense
> Still looking for an official OK :)
>
> I'll probably just put it in soon...
>
ok gilles@ still :-)
> diff --git extern.h extern.h
> index 17c6aa54f18..f6293a371ad 100644
> --- extern.h
> +++ extern.h
> @@ -207,7 +207,8 @@ intrevokeproc(int, const char *, const
> char *,
> int, int, const char *const *, size_t);
> int fileproc(int, const char *, const char *, const char *,
> const char *);
> -int keyproc(int, const char *, const char **, size_t);
> +int keyproc(int, const char *, const char **, size_t,
> + enum keytype);
> int netproc(int, int, int, int, int, int, int,
> struct authority_c *, const char *const *,
> size_t);
> @@ -275,11 +276,6 @@ char *json_fmt_signed(const char *, const
> char *, const char *);
> */
> int verbose;
>
> -/*
> - * Should we switch to ecdsa?
> - */
> -int ecdsa;
> -
> /*
> * What component is the process within (COMP__MAX for none)?
> */
> diff --git keyproc.c keyproc.c
> index 9c392a0f3f6..f9ce081457a 100644
> --- keyproc.c
> +++ keyproc.c
> @@ -74,8 +74,8 @@ add_ext(STACK_OF(X509_EXTENSION) *sk, int nid, const char
> *value)
> * jail and, on success, ship it to "netsock" as an X509 request.
> */
> int
> -keyproc(int netsock, const char *keyfile,
> -const char **alts, size_t altsz)
> +keyproc(int netsock, const char *keyfile, const char **alts, size_t altsz,
> +enum keytype keytype)
> {
> char*der64 = NULL, *der = NULL, *dercp;
> char*sans = NULL, *san = NULL;
> @@ -117,14 +117,17 @@ keyproc(int netsock, const char *keyfile,
> }
>
> if (newkey) {
> - if (ecdsa) {
> + switch (keytype) {
> + case KT_ECDSA:
> if ((pkey = ec_key_create(f, keyfile)) == NULL)
> goto out;
> dodbg("%s: generated ECDSA domain key", keyfile);
> - } else {
> + break;
> + case KT_RSA:
> if ((pkey = rsa_key_create(f, keyfile)) == NULL)
> goto out;
> dodbg("%s: generated RSA domain key", keyfile);
> + break;
> }
> } else {
> if ((pkey = key_load(f, keyfile)) == NULL)
> diff --git main.c main.c
> index ea8f7c5d348..d70a7048f47 100644
> --- main.c
> +++ main.c
> @@ -49,7 +49,6 @@ main(int argc, char *argv[])
> int popts = 0;
> pid_t pids[COMP__MAX];
> extern intverbose;
> - extern intecdsa;
> extern enum comp proccomp;
> size_ti, altsz, ne;
>
> @@ -148,10 +147,6 @@ main(int argc, char *argv[])
> errx(EXIT_FAILURE
Re: [patch] use acme-client to sign certificated with ecdsa keys
On Fri, Jun 14, 2019 at 01:58:58PM +0200, Florian Obser wrote:
> On Fri, Jun 14, 2019 at 09:50:35AM +0200, Renaud Allard wrote:
> >
> >
> > On 6/12/19 2:30 PM, Renaud Allard wrote:
> > >
> > >
> > > On 6/11/19 2:36 PM, Sebastian Benoit wrote:
> > > > Hi,
> > > >
> > > > some feedback below.
> > > >
> > > > Renaud: maybe wait for feedback from florian or gilles until
> > > > acting on my comments, sometimes sending diffs to fast creates more
> > > > work ;)
> > > >
> > > > /Benno
> > > >
> > >
> > > As suggested by benno@
> > > removal of the global variable
> > > removal of KEYTYPE which was not used and was a leftover of a former patch
> > > define ECDSA_KEY to be more readable
> > >
> >
> > Any comment or OK on my latest patch?
> >
>
> I'd prefer to use enums like the rest of the code.
>
yes, ok gilles@
> diff --git extern.h extern.h
> index 17c6aa54f18..f6293a371ad 100644
> --- extern.h
> +++ extern.h
> @@ -207,7 +207,8 @@ intrevokeproc(int, const char *, const
> char *,
> int, int, const char *const *, size_t);
> int fileproc(int, const char *, const char *, const char *,
> const char *);
> -int keyproc(int, const char *, const char **, size_t);
> +int keyproc(int, const char *, const char **, size_t,
> + enum keytype);
> int netproc(int, int, int, int, int, int, int,
> struct authority_c *, const char *const *,
> size_t);
> @@ -275,11 +276,6 @@ char *json_fmt_signed(const char *, const
> char *, const char *);
> */
> int verbose;
>
> -/*
> - * Should we switch to ecdsa?
> - */
> -int ecdsa;
> -
> /*
> * What component is the process within (COMP__MAX for none)?
> */
> diff --git keyproc.c keyproc.c
> index 9c392a0f3f6..f9ce081457a 100644
> --- keyproc.c
> +++ keyproc.c
> @@ -74,8 +74,8 @@ add_ext(STACK_OF(X509_EXTENSION) *sk, int nid, const char
> *value)
> * jail and, on success, ship it to "netsock" as an X509 request.
> */
> int
> -keyproc(int netsock, const char *keyfile,
> -const char **alts, size_t altsz)
> +keyproc(int netsock, const char *keyfile, const char **alts, size_t altsz,
> +enum keytype keytype)
> {
> char*der64 = NULL, *der = NULL, *dercp;
> char*sans = NULL, *san = NULL;
> @@ -117,14 +117,17 @@ keyproc(int netsock, const char *keyfile,
> }
>
> if (newkey) {
> - if (ecdsa) {
> + switch (keytype) {
> + case KT_ECDSA:
> if ((pkey = ec_key_create(f, keyfile)) == NULL)
> goto out;
> dodbg("%s: generated ECDSA domain key", keyfile);
> - } else {
> + break;
> + case KT_RSA:
> if ((pkey = rsa_key_create(f, keyfile)) == NULL)
> goto out;
> dodbg("%s: generated RSA domain key", keyfile);
> + break;
> }
> } else {
> if ((pkey = key_load(f, keyfile)) == NULL)
> diff --git main.c main.c
> index ea8f7c5d348..d70a7048f47 100644
> --- main.c
> +++ main.c
> @@ -49,7 +49,6 @@ main(int argc, char *argv[])
> int popts = 0;
> pid_t pids[COMP__MAX];
> extern intverbose;
> - extern intecdsa;
> extern enum comp proccomp;
> size_ti, altsz, ne;
>
> @@ -148,10 +147,6 @@ main(int argc, char *argv[])
> errx(EXIT_FAILURE, "authority %s not found", auth);
> }
>
> - if (domain->keytype == 1) {
> - ecdsa = 1;
> - }
> -
> acctkey = authority->account;
>
> if ((chngdir = domain->challengedir) == NULL)
> @@ -258,7 +253,8 @@ main(int argc, char *argv[])
> close(file_fds[0]);
> close(file_fds[1]);
> c = keyproc(key_fds[0], domain->key,
> - (const char **)alts, altsz);
> + (const char **)alts, altsz,
> + domain->keytype);
> exit(c ? EXIT_SUCCESS : EXIT_FAILURE);
> }
>
> diff --git parse.h parse.h
> index 78405590568..7f2d3ca546c 100644
> --- parse.h
> +++ parse.h
> @@ -27,6 +27,11 @@
Re: [patch] use acme-client to sign certificated with ecdsa keys
; > - * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> > - * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> > - */
> > -
> > -#include
> > -#include
> > -#include
> > -
> > -#include
> > -#include
> > -#include
> > -
> > -#include "rsa.h"
> > -
> > -/*
> > - * Default number of bits when creating a new key.
> > - */
> > -#defineKBITS 4096
> > -
> > -/*
> > - * Create an RSA key with the default KBITS number of bits.
> > - */
> > -EVP_PKEY *
> > -rsa_key_create(FILE *f, const char *fname)
> > -{
> > - EVP_PKEY_CTX*ctx = NULL;
> > - EVP_PKEY*pkey = NULL;
> > -
> > - /* First, create the context and the key. */
> > -
> > - if ((ctx = EVP_PKEY_CTX_new_id(EVP_PKEY_RSA, NULL)) == NULL) {
> > - warnx("EVP_PKEY_CTX_new_id");
> > - goto err;
> > - } else if (EVP_PKEY_keygen_init(ctx) <= 0) {
> > - warnx("EVP_PKEY_keygen_init");
> > - goto err;
> > - } else if (EVP_PKEY_CTX_set_rsa_keygen_bits(ctx, KBITS) <= 0) {
> > - warnx("EVP_PKEY_set_rsa_keygen_bits");
> > - goto err;
> > - } else if (EVP_PKEY_keygen(ctx, ) <= 0) {
> > - warnx("EVP_PKEY_keygen");
> > - goto err;
> > - }
> > -
> > - /* Serialise the key to the disc. */
> > -
> > - if (PEM_write_PrivateKey(f, pkey, NULL, NULL, 0, NULL, NULL))
> > - goto out;
> > -
> > - warnx("%s: PEM_write_PrivateKey", fname);
> > -err:
> > - EVP_PKEY_free(pkey);
> > - pkey = NULL;
> > -out:
> > - EVP_PKEY_CTX_free(ctx);
> > - return pkey;
> > -}
> > -
> > -
> > -EVP_PKEY *
> > -rsa_key_load(FILE *f, const char *fname)
> > -{
> > - EVP_PKEY*pkey;
> > -
> > - pkey = PEM_read_PrivateKey(f, NULL, NULL, NULL);
> > - if (pkey == NULL) {
> > - warnx("%s: PEM_read_PrivateKey", fname);
> > - return NULL;
> > - } else if (EVP_PKEY_type(pkey->type) == EVP_PKEY_RSA)
> > - return pkey;
> > -
> > - warnx("%s: unsupported key type", fname);
> > - EVP_PKEY_free(pkey);
> > - return NULL;
> > -}
> > Index: rsa.h
> > ===
> > RCS file: rsa.h
> > diff -N rsa.h
> > --- rsa.h 31 Aug 2016 22:01:42 - 1.1
> > +++ /dev/null 1 Jan 1970 00:00:00 -
> > @@ -1,23 +0,0 @@
> > -/* $Id: rsa.h,v 1.1 2016/08/31 22:01:42 florian Exp $ */
> > -/*
> > - * Copyright (c) 2016 Kristaps Dzonsons
> > - *
> > - * Permission to use, copy, modify, and distribute this software for any
> > - * purpose with or without fee is hereby granted, provided that the above
> > - * copyright notice and this permission notice appear in all copies.
> > - *
> > - * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHORS DISCLAIM ALL WARRANTIES
> > - * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> > - * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHORS BE LIABLE FOR
> > - * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> > - * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> > - * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> > - * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> > - */
> > -#ifndef RSA_H
> > -#define RSA_H
> > -
> > -EVP_PKEY *rsa_key_create(FILE *, const char *);
> > -EVP_PKEY *rsa_key_load(FILE *, const char *);
> > -
> > -#endif /* ! RSA_H */
>
>
>
>
> --
> I'm not entirely sure you are real.
>
--
Gilles Chehade @poolpOrg
https://www.poolp.org tip me: https://paypal.me/poolpOrg
