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: 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: 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
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
> + * 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.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +
> +#include
>
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)
>
[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.
[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);
}
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.orgpatreon: https://www.patreon.com/gilles
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
= 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
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
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
|", 16) == 0) {
> + if (strncmp(line, "register|report|", 16) == 0) {
> lka_report_register_hook(name, line+16);
> return;
> }
>
> - if (strncasecmp(line, "register|filter|", 16) == 0) {
> + if (strncmp(line, "register|filter|", 16) == 0) {
> lka_filter_register_hook(name, line+16);
> return;
> }
> +
> + fatalx("Invalid register line received: %s", line);
> }
>
> static void
> processor_io(struct io *io, int evt, void *arg)
> {
> + struct processor_instance *processor;
> const char *name = arg;
> char*line = NULL;
> ssize_t len;
>
> switch (evt) {
> case IO_DATAIN:
> - nextline:
> - line = io_getline(io, );
> - /* No complete line received */
> - if (line == NULL)
> - return;
> -
> - if (strncasecmp("register|", line, 9) == 0)
> - processor_register(name, line);
> - else if (! lka_filter_process_response(name, line))
> - fatalx("misbehaving filter");
> -
> - goto nextline;
> + while ((line = io_getline(io, )) != NULL) {
> + if (strncmp("register|", line, 9) == 0) {
> + processor_register(name, line);
> + continue;
> + }
> +
> + processor = dict_xget(, name);
> + if (!processor->ready)
> + fatalx("Non-register message before register|"
> + "ready: %s", line);
> + else if (strncmp(line, "filter-result|", 14) == 0 ||
> + strncmp(line, "filter-dataline|", 16) || 0)
> + lka_filter_process_response(name, line);
> + else
> + fatalx("Invalid filter message type: %s", line);
> + }
> }
> }
>
> 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 26 Aug 2019 10:03:43 -
> @@ -102,16 +102,16 @@ lka_report_register_hook(const char *nam
> void *iter;
> size_t i;
>
> - if (strncasecmp(hook, "smtp-in|", 8) == 0) {
> + if (strncmp(hook, "smtp-in|", 8) == 0) {
> subsystem = _in;
> hook += 8;
> }
> - else if (strncasecmp(hook, "smtp-out|", 9) == 0) {
> + else if (strncmp(hook, "smtp-out|", 9) == 0) {
> subsystem = _out;
> hook += 9;
> }
> else
> - return;
> + fatalx("Invalid message direction: %s", hook);
>
> if (strcmp(hook, "*") == 0) {
> iter = NULL;
> @@ -127,7 +127,7 @@ lka_report_register_hook(const char *nam
> if (strcmp(hook, smtp_events[i].event) == 0)
> break;
> if (i == nitems(smtp_events))
> - return;
> + fatalx("Unrecognized report name: %s", hook);
>
> tailq = dict_get(subsystem, hook);
> rp = xcalloc(1, sizeof *rp);
>
--
Gilles Chehade @poolpOrg
https://www.poolp.orgpatreon: https://www.patreon.com/gilles
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
, "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 @@
> * limit all paths to PATH_MAX
> */
>
> +enum keytype {
> + KT_RSA = 0,
> + KT_ECDSA
> +};
> +
> struct authority_c {
> TAILQ_ENTRY(authority_c) entry;
> char*name;
> @@ -36,9 +41,9 @@ struct authority_c {
>
> struct domain_c {
> TAILQ_ENTRY(domain_c)entry;
> - TAILQ_HEAD(, altname_c) altname_list;
> - int altname_count;
> - int keytype;
> + TAILQ_HEAD(, altname_c) altname_list;
> + int altname_count;
> + enum keytype keytype;
> char*domain;
> char*key;
> char*cert;
> diff --git parse.y parse.y
> index 994492706bb..9d10be26dbd 100644
> --- parse.y
> +++ parse.y
> @@ -100,7 +100,7 @@ typedef struct {
> %}
>
> %token AUTHORITY URL API ACCOUNT
> -%token DOMAIN ALTERNATIVE NAMES CERT FULL CHAIN KEY SIGN WITH
> CHALLENGEDIR KEYTYPE
> +%token DOMAIN ALTERNATIVE NAMES CERT FULL CHAIN KEY SIGN WITH
> CHALLENGEDIR
> %token YES NO
> %token INCLUDE
> %token ERROR
> @@ -108,6 +108,7 @@ typedef struct {
> %token STRING
> %token NUMBER
> %type string
> +%type keytype
>
> %%
>
> @@ -260,13 +261,9 @@ domain : DOMAIN STRING {
> }
> ;
>
> -keytype : RSA {
> - domain->keytype = 0;
> - }
> - | ECDSA {
> - domain->keytype = 1;
> - }
> - | /* nothing */
> +keytype : RSA { $$ = KT_RSA; }
> + | ECDSA { $$ = KT_ECDSA; }
> + | { $$ = KT_RSA; }
> ;
>
> domainopts_l : domainopts_l domainoptsl nl
> @@ -292,6 +289,7 @@ domainoptsl : ALTERNATIVE NAMES '{' altname_l '}'
> YYERROR;
> }
> domain->key = s;
> + domain->keytype = $4;
> }
> | DOMAIN CERT STRING {
> char *s;
>
>
> --
> I'm not entirely sure you are real.
>
--
Gilles Chehade @poolpOrg
https://www.poolp.orgpatreon: https://www.patreon.com/gilles
Re: [patch] use acme-client to sign certificated with ecdsa keys
> * limit all paths to PATH_MAX
> */
>
> +enum keytype {
> + KT_RSA = 0,
> + KT_ECDSA
> +};
> +
> struct authority_c {
> TAILQ_ENTRY(authority_c) entry;
> char*name;
> @@ -36,9 +41,9 @@ struct authority_c {
>
> struct domain_c {
> TAILQ_ENTRY(domain_c)entry;
> - TAILQ_HEAD(, altname_c) altname_list;
> - int altname_count;
> - int keytype;
> + TAILQ_HEAD(, altname_c) altname_list;
> + int altname_count;
> + enum keytype keytype;
> char*domain;
> char*key;
> char*cert;
> diff --git parse.y parse.y
> index 994492706bb..0b68a35fb73 100644
> --- parse.y
> +++ parse.y
> @@ -100,7 +100,7 @@ typedef struct {
> %}
>
> %token AUTHORITY URL API ACCOUNT
> -%token DOMAIN ALTERNATIVE NAMES CERT FULL CHAIN KEY SIGN WITH
> CHALLENGEDIR KEYTYPE
> +%token DOMAIN ALTERNATIVE NAMES CERT FULL CHAIN KEY SIGN WITH
> CHALLENGEDIR
> %token YES NO
> %token INCLUDE
> %token ERROR
> @@ -260,13 +260,15 @@ domain : DOMAIN STRING {
> }
> ;
>
> -keytype : RSA {
> - domain->keytype = 0;
> +keytype : RSA {
> + domain->keytype = KT_RSA;
> }
> | ECDSA {
> - domain->keytype = 1;
> + domain->keytype = KT_ECDSA;
> + }
> + | { /* nothing */
> + domain->keytype = KT_RSA;
> }
> - | /* nothing */
> ;
>
> domainopts_l : domainopts_l domainoptsl nl
>
>
> --
> I'm not entirely sure you are real.
>
--
Gilles Chehade @poolpOrg
https://www.poolp.orgpatreon: https://www.patreon.com/gilles
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
Re: [patch] use acme-client to sign certificated with ecdsa keys
LLENGEDIR KEYTYPE
> %token YES NO
> %token INCLUDE
> %token ERROR
> +%token RSA ECDSA
> %token STRING
> %token NUMBER
> %type string
> @@ -258,12 +260,21 @@ domain : DOMAIN STRING {
> }
> ;
>
> +keytype : RSA {
> + domain->keytype = 0;
> + }
> + | ECDSA {
> + domain->keytype = 1;
> + }
> + | /* nothing */
> + ;
> +
> domainopts_l : domainopts_l domainoptsl nl
> | domainoptsl optnl
> ;
>
> domainoptsl : ALTERNATIVE NAMES '{' altname_l '}'
> - | DOMAIN KEY STRING {
> + | DOMAIN KEY STRING keytype {
> char *s;
> if (domain->key != NULL) {
> yyerror("duplicate key");
> @@ -427,10 +438,12 @@ lookup(char *s)
> {"chain", CHAIN},
> {"challengedir",CHALLENGEDIR},
> {"domain", DOMAIN},
> + {"ecdsa", ECDSA},
> {"full",FULL},
> {"include", INCLUDE},
> {"key", KEY},
> {"names", NAMES},
> + {"rsa", RSA},
> {"sign",SIGN},
> {"url", URL},
> {"with",WITH},
> Index: rsa.c
> ===
> RCS file: rsa.c
> diff -N rsa.c
> --- rsa.c 28 Jul 2018 15:25:23 - 1.7
> +++ /dev/null 1 Jan 1970 00:00:00 -
> @@ -1,88 +0,0 @@
> -/* $Id: rsa.c,v 1.7 2018/07/28 15:25:23 tb 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.
> - */
> -
> -#include
> -#include
> -#include
> -
> -#include
> -#include
> -#include
> -
> -#include "rsa.h"
> -
> -/*
> - * Default number of bits when creating a new key.
> - */
> -#define KBITS 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 */
--
Gilles Chehade @poolpOrg
https://www.poolp.org tip me: https://paypal.me/poolpOrg
Re: [patch] use acme-client to sign certificated with ecdsa keys
On Wed, Jun 05, 2019 at 08:39:51AM +0200, Renaud Allard wrote:
>
>
> On 6/5/19 8:20 AM, Gilles Chehade wrote:
> > On Tue, Jun 04, 2019 at 03:54:11PM +0200, Renaud Allard wrote:
> > >
> > >
> > > On 6/3/19 11:53 AM, Renaud Allard wrote:
> > > > > >
> > > > > > On 5/29/19 9:58 AM, Florian Obser wrote:
> > > > > > > why not let acme-client generate the key?
> > > > > >
> > > > >
> > > > > Here is a more complete diff where you can use the -E switch to
> > > > > generate a ECDSA key instead of the RSA one.
> > >
> > > I refined a little bit the patch to not put ecdsa functions into rsa.c.
> > > So I
> > > renamed rsa.c to key.c and removed the rsa references to functions which
> > > apply to both rsa and ecdsa.
> > >
> >
> > reads, builds and works fine for me
> >
> > a couple comments inlined
> >
>
> I removed the parenthesis and used another wording, removed the RSA from a
> "Load RSA key" as it might not be RSA and added E to the SYNOPSYS.
>
ok gilles@
> Index: Makefile
> ===
> RCS file: /cvs/src/usr.sbin/acme-client/Makefile,v
> retrieving revision 1.8
> diff -u -p -r1.8 Makefile
> --- Makefile 3 Jul 2017 22:21:47 - 1.8
> +++ Makefile 5 Jun 2019 06:37:00 -
> @@ -2,7 +2,7 @@
> PROG=acme-client
> SRCS=acctproc.c base64.c certproc.c chngproc.c dbg.c
> dnsproc.c
> SRCS+= fileproc.c http.c jsmn.c json.c keyproc.c main.c
> netproc.c
> -SRCS+= parse.y revokeproc.c rsa.c util.c
> +SRCS+= parse.y revokeproc.c key.c util.c
>
> MAN= acme-client.1 acme-client.conf.5
>
> Index: acctproc.c
> ===
> RCS file: /cvs/src/usr.sbin/acme-client/acctproc.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 acctproc.c
> --- acctproc.c28 Jul 2018 15:25:23 - 1.12
> +++ acctproc.c5 Jun 2019 06:37:00 -
> @@ -82,8 +82,8 @@ op_thumb_rsa(EVP_PKEY *pkey)
> warnx("bn2string");
> else if ((exp = bn2string(r->e)) == NULL)
> warnx("bn2string");
> - else if ((json = json_fmt_thumb_rsa(exp, mod)) == NULL)
> - warnx("json_fmt_thumb_rsa");
> + else if ((json = json_fmt_thumb_key(exp, mod)) == NULL)
> + warnx("json_fmt_thumb_key");
>
> free(exp);
> free(mod);
> @@ -175,10 +175,10 @@ op_sign_rsa(char **head, char **prot, EV
> warnx("bn2string");
> else if ((exp = bn2string(r->e)) == NULL)
> warnx("bn2string");
> - else if ((*head = json_fmt_header_rsa(exp, mod)) == NULL)
> - warnx("json_fmt_header_rsa");
> - else if ((*prot = json_fmt_protected_rsa(exp, mod, nonce)) == NULL)
> - warnx("json_fmt_protected_rsa");
> + else if ((*head = json_fmt_header_key(exp, mod)) == NULL)
> + warnx("json_fmt_header_key");
> + else if ((*prot = json_fmt_protected_key(exp, mod, nonce)) == NULL)
> + warnx("json_fmt_protected_key");
> else
> rc = 1;
>
> @@ -338,7 +338,7 @@ acctproc(int netsock, const char *acctke
> goto out;
> dodbg("%s: generated RSA account key", acctkey);
> } else {
> - if ((pkey = rsa_key_load(f, acctkey)) == NULL)
> + if ((pkey = key_load(f, acctkey)) == NULL)
> goto out;
> doddbg("%s: loaded RSA account key", acctkey);
> }
> Index: acme-client.1
> ===
> RCS file: /cvs/src/usr.sbin/acme-client/acme-client.1,v
> retrieving revision 1.29
> diff -u -p -r1.29 acme-client.1
> --- acme-client.1 3 Feb 2019 20:39:35 - 1.29
> +++ acme-client.1 5 Jun 2019 06:37:00 -
> @@ -22,7 +22,7 @@
> .Nd ACME client
> .Sh SYNOPSIS
> .Nm acme-client
> -.Op Fl ADFnrv
> +.Op Fl ADEFnrv
> .Op Fl f Ar configfile
> .Ar domain
> .Sh DESCRIPTION
> @@ -79,7 +79,9 @@ The options are as follows:
> .It Fl A
> Create a new RSA account key if one does not already exist.
> .It Fl D
> -Create a new RSA domain key if one does not already exist.
> +Create a new domain key if one does not already exist. Defaults to RSA.
> +.It Fl E
> +Switch the new domain key algorithm to ECDS
Re: [patch] use acme-client to sign certificated with ecdsa keys
) == NULL)
> - goto out;
> - dodbg("%s: generated RSA domain key", keyfile);
> + if (ecdsa) {
> + if ((pkey = ec_key_create(f, keyfile)) == NULL)
> + goto out;
> + dodbg("%s: generated ECDSA domain key", keyfile);
> + } else {
> + if ((pkey = rsa_key_create(f, keyfile)) == NULL)
> + goto out;
> + dodbg("%s: generated RSA domain key", keyfile);
> + }
> } else {
> - if ((pkey = rsa_key_load(f, keyfile)) == NULL)
> + if ((pkey = key_load(f, keyfile)) == NULL)
> goto out;
> doddbg("%s: loaded RSA domain key", keyfile);
> }
> Index: main.c
> ===
> RCS file: /cvs/src/usr.sbin/acme-client/main.c,v
> retrieving revision 1.45
> diff -u -p -r1.45 main.c
> --- main.c9 Mar 2019 18:07:40 - 1.45
> +++ main.c4 Jun 2019 13:50:28 -
> @@ -49,6 +49,7 @@ main(int argc, char *argv[])
> int popts = 0;
> pid_t pids[COMP__MAX];
> extern intverbose;
> + extern bool ecdsa;
> extern enum comp proccomp;
> size_ti, altsz, ne;
>
> @@ -57,7 +58,7 @@ main(int argc, char *argv[])
> struct domain_c *domain = NULL;
> struct altname_c*ac;
>
> - while ((c = getopt(argc, argv, "ADFnrvf:")) != -1)
> + while ((c = getopt(argc, argv, "ADEFnrvf:")) != -1)
> switch (c) {
> case 'A':
> popts |= ACME_OPT_NEWACCT;
> @@ -65,6 +66,10 @@ main(int argc, char *argv[])
> case 'D':
> popts |= ACME_OPT_NEWDKEY;
> break;
> + case 'E':
> + ecdsa = true;
> + popts |= ACME_OPT_DKEYEC;
> + break;
> case 'F':
> force = 1;
> break;
> @@ -180,6 +185,10 @@ main(int argc, char *argv[])
> != -1) {
> dodbg("%s: domain key exists (not creating)", domain->key);
> popts &= ~ACME_OPT_NEWDKEY;
> + }
> +
> + if (popts & ACME_OPT_DKEYEC) {
> + ecdsa = true;
> }
>
> if (access(chngdir, R_OK) == -1) {
> Index: parse.h
> ===
> RCS file: /cvs/src/usr.sbin/acme-client/parse.h,v
> retrieving revision 1.9
> diff -u -p -r1.9 parse.h
> --- parse.h 27 Nov 2017 16:53:04 - 1.9
> +++ parse.h 4 Jun 2019 13:50:28 -
> @@ -61,6 +61,7 @@ struct keyfile {
> #define ACME_OPT_NEWACCT 0x0002
> #define ACME_OPT_NEWDKEY 0x0004
> #define ACME_OPT_CHECK 0x0008
> +#define ACME_OPT_DKEYEC 0x0016
>
> struct acme_conf {
> int opts;
> Index: rsa.c
> ===
> RCS file: rsa.c
> diff -N rsa.c
> --- rsa.c 28 Jul 2018 15:25:23 - 1.7
> +++ /dev/null 1 Jan 1970 00:00:00 -
> @@ -1,88 +0,0 @@
> -/* $Id: rsa.c,v 1.7 2018/07/28 15:25:23 tb 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.
> - */
> -
> -#include
> -#include
> -#include
> -
> -#include
> -#include
> -#include
> -
> -#include "rsa.h"
> -
> -/*
> - * Default number of bits when creating a new key.
> - */
> -#define KBITS 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
Re: mention opensmtpd mail archive on a web page
sorry, im slowly catching up on my mail backlog diff committed, thanks On Mon, May 06, 2019 at 08:27:56PM +0300, Sergey Bronnikov wrote: > diff --git a/opensmtpd/list.html b/opensmtpd/list.html > index cdb66803b..97e55d3df 100644 > --- a/opensmtpd/list.html > +++ b/opensmtpd/list.html > @@ -38,7 +38,8 @@ > >General OpenSMTPD user discussion list >General purpose discussions, issues and ideas may be discussed on > - our mailto:m...@opensmtpd.org;>m...@opensmtpd.org list. The > + our mailto:m...@opensmtpd.org;>m...@opensmtpd.org list > + (https://www.mail-archive.com/misc@opensmtpd.org/;>Archive). > The >list is not moderated, however registration is required. > >To register, simply send a mail to > > Sergey > -- Gilles Chehade @poolpOrg https://www.poolp.org tip me: https://paypal.me/poolpOrg
Re: smtpd - Filter-dataline response is the parameter
On Mon, Apr 08, 2019 at 08:20:56AM +0200, Martijn van Duren wrote:
> Found this the hard way by playing with the new filter API.
>
> When sending a plaintext message, which has an '|' in the text it would
> cut of the data at that point. The diff below pushes the parameter split
> to after handling the filter-dataline.
>
> Fixes my usecase.
>
> OK?
>
> martijn@
>
OK but oops, I stole your commit ...
I owe you one
> Index: lka_filter.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/lka_filter.c,v
> retrieving revision 1.34
> diff -u -p -r1.34 lka_filter.c
> --- lka_filter.c 15 Jan 2019 04:49:50 - 1.34
> +++ lka_filter.c 8 Apr 2019 06:20:26 -
> @@ -458,14 +458,14 @@ lka_filter_process_response(const char *
> return 0;
>
> response = ep+1;
> - if ((ep = strchr(response, '|'))) {
> - parameter = ep + 1;
> - *ep = 0;
> - }
> -
> if (strcmp(kind, "filter-dataline") == 0) {
> filter_data_next(token, reqid, response);
> return 1;
> + }
> +
> + if ((ep = strchr(response, '|'))) {
> + parameter = ep + 1;
> + *ep = 0;
> }
>
> if (strcmp(response, "proceed") != 0 &&
>
--
Gilles Chehade @poolpOrg
https://www.poolp.org tip me: https://paypal.me/poolpOrg
Re: mail(1): use "sendmail" as argv[0] for sendmail
On Fri, Mar 01, 2019 at 03:16:26PM +0200, Lauri Tirkkonen wrote:
> For some reason mail(1) is using "send-mail" as argv[0] for sendmail.
> /etc/mailer.conf and smtpctl handle this identically to "sendmail", so
> it seems a bit redundant. This diff makes mail(1) use "sendmail" as
> argv[0], possibly allowing that duplication to be removed later from
> mailer.conf and smtpctl.
>
> Noticed while porting mail(1) to Unleashed (where we also have
> mailwrapper, but no default configuration for "send-mail").
>
I wish we had an historian who could enlighten us as to why both exist.
smtpctl handles send-mail because mail uses send-mail and I suspect that
some hackers use mua from the seventies that also use send-mail.
if we could confirm that there's no harm in the following diff, then the
send-mail case in mailer.conf and smtpctl could bite the dust afaic
> diff --git a/usr.bin/mail/send.c b/usr.bin/mail/send.c
> index 8f127ac837f..723f9da0a53 100644
> --- a/usr.bin/mail/send.c
> +++ b/usr.bin/mail/send.c
> @@ -371,7 +371,7 @@ mail1(struct header *hp, int printheaders)
> (void)savemail(expand(cp), mtf);
>
> /* Setup sendmail arguments. */
> -*ap++ = "send-mail";
> +*ap++ = "sendmail";
> *ap++ = "-i";
> *ap++ = "-t";
> cp = hp->h_from ? hp->h_from : value("from");
> --
> Lauri Tirkkonen | lotheac @ IRCnet
>
--
Gilles Chehade @poolpOrg
https://www.poolp.org tip me: https://paypal.me/poolpOrg
Re: grep: convert fgetln to getline
On Thu, Jan 31, 2019 at 03:10:53PM +0200, Lauri Tirkkonen wrote: > On Wed, Jan 30 2019 20:32:50 -0500, Ted Unangst wrote: > > Thanks for digging into this. I went ahead and committed your diff. > > Thanks for committing it. > > You know, having seen fgetln's allocation strategy and its usage of the > stdio internals, I couldn't help but wonder if it's something that could > be eventually removed entirely. It's used in a bunch of places in-tree > for sure, but not too many to convert, I think; and I've already done a > few... > > It's not exactly a serious suggestion at this point; I realize there > probably is third-party software too that uses this function (Linuxes > provide compat for it in libbsd; there must be a reason for that). I'm > speaking as someone who's been removing a bunch of crap from that other > OS I mentioned, so that's my reason for this line of thinking slash > pipe-dreaming ;) > quite amazing to watch tedu talk to another tedu :-| -- Gilles Chehade @poolpOrg https://www.poolp.org tip me: https://paypal.me/poolpOrg
Re: strlcpy() or strscpy()?
On Sun, Jan 27, 2019 at 01:07:05AM -0500, 0sjfoij...@firemail.cc wrote:
> Recently on LCA2019, Joel Sing made a presentation about "Security
> Vulnerability Mitigations"[1]
> (very good, btw). He suggests function strlcpy(3) as a secure API.
> In the same conference, though, Kees Cook ("Making C Less Dangerous in the
> Linux kernel"[2]),
> recommends strscpy() as more secure. So, my question is: what's the best to
> use?
>
if i understand correctly, strscpy() works just like strlcpy():
size_t
strlcpy(char *dst, const char *src, size_t dstsize);
ssize_t
strscpy(char *dest, const char *src, size_t count);
with the only difference being that strlcpy() returns an unsigned size_t
which would be the size it would have written, then strscpy() returns an
ssize_t which returns the size or -E2BIG in case of an overflow.
quite frankly, i think the claim that strscpy() is safer, easier or that
it makes overflow easier to detect is bullshit for lack of a better word
given that they both work very similar:
if (strlcpy(dest, src, sizeof dest) >= sizeof dest) {
// overflow
}
if (strscpy(dest, src, sizeof dest) == -E2BIG) {
// overflow
}
and that strscpy() is essentially strlcpy() in NIH disguise:
ssize_t
strscpy(char *dest, const char *src, size_t count)
{
ssize_t ret;
if ((ret = strlcpy(dest, src, count)) >= count)
return -E2BIG;
return ret;
}
just my opinion
--
Gilles Chehade @poolpOrg
https://www.poolp.org tip me: https://paypal.me/poolpOrg
Re: makemap.8 patch
On Tue, Nov 20, 2018 at 04:12:13PM -0600, Edgar Pettijohn wrote: > > > > > why db ? > > Do you need makemap for file backend? > oh gosh am I dumb. your diff is ok, ignore me, will commit later today. -- Gilles Chehade @poolpOrg https://www.poolp.org tip me: https://paypal.me/poolpOrg
Re: makemap.8 patch
On Sun, Nov 18, 2018 at 08:32:47AM -0600, Edgar Pettijohn III wrote: > Use new syntax. > Sorry was on the road. Comment inlined: > Index: makemap.8 > > === > RCS file: /cvs/src/usr.sbin/smtpd/makemap.8,v > retrieving revision 1.29 > diff -u -p -u -r1.29 makemap.8 > --- makemap.8 ??13 Feb 2016 08:53:18 - ??1.29 > +++ makemap.8 ??18 Nov 2018 14:29:33 - > @@ -105,8 +105,11 @@ In addition to adding an entry to the pr > ??one must add a filter rule that accepts mail for the domain > ??map, for example: > ??.Bd -literal -offset indent > -table domains "/etc/mail/domains" > -accept for domain deliver to mbox > +table domains db:/etc/mail/domains.db > + why db ? > +action "local" mbox > + > +match for domain action "local" > ??.Ed > ??.Sh VIRTUAL DOMAINS > ??Virtual domains may also be kept in tables. > @@ -140,11 +143,13 @@ In addition to adding an entry to the vi > ??one must add a filter rule that accepts mail for virtual domains, > ??for example: > ??.Bd -literal -offset indent > -table vdomains "/etc/mail/vdomains" > -table vusers "/etc/mail/users" > +table vdomains db:/etc/mail/vdomains.db > +table vusers db:/etc/mail/users.db > + why db ? > +action "local" mbox virtual > > -accept for domain virtual deliver to mbox > -accept for domain example.org virtual deliver to mbox > +match for domain action "local" > +match for domain "example.org" action "local" > ??.Ed > ??.Sh FILES > ??.Bl -tag -width "/etc/mail/aliasesXXX" -compact > Documentation should stick to the file backend which is the best one for the general case. The db backend is an extension of the file backend and unless you have a very specific use case, it brings no benefit whatsoever. It ISN'T faster than the file backend and unless you have a good rationale for using it, there's actually no good reason to. The only reason we still support it is because some corner cases do make sense, and even in those cases I'd argue there are better backends. -- Gilles Chehade @poolpOrg https://www.poolp.org tip me: https://paypal.me/poolpOrg
Re: [PATCH] parse ! on hostname.if for autoinstall
On Fri, Nov 09, 2018 at 05:10:00PM +0100, Julien Dhaille wrote:
> Hi.
>
> During auto upgrade via the auto_upgrade.conf file (no DHCP server),
> shell commands are skipped.
> This small diff is coming from parse_hn_line() in /etc/netstart, thus,
> it keeps the behavior similar.
> I think it's handy, especially if you need a static route in order to
> reach a mirror for example.
>
diff is barely readable :-p
> diff --git install.sub install.sub
> index bce1fa50358..31dbafdc95d 100644
> --- install.sub
> +++ install.sub
> @@ -2319,8 +2319,11 @@ parse_hn_line() {
> ?? _cmds[${#_cmds[*]}]="ifconfig $_if ${_c[@]}
> up;dhclient
> $_if"
> ?? V4_DHCPCONF=true
> ?? ;;
> - '!'*|bridge)
> - # Skip shell commands and bridge in the
> installer.
> + '!'*) _cmd=$(print -- "${_c[@]}" | sed 's/\$if/'$_if'/g')
> + _cmds[${#_cmds[*]}]="${_cmd#!}"
> + ;;
> + bridge)
> + # Skip bridge in the installer.
> ?? return
> ?????????? ;;
> ?? *)?? _cmds[${#_cmds[*]}]="ifconfig $_if ${_c[@]}"
>
>
> Cheers
>
--
Gilles Chehade @poolpOrg
https://www.poolp.org tip me: https://paypal.me/poolpOrg
Re: smtpd(8) match mail-from entire domain
Sorry for the delay, was catching up.
On Thu, Oct 25, 2018 at 10:32:32PM +0200, Martijn van Duren wrote:
> Back in the old days of the ancient syntax smtpd.conf(5) contained
> the following section:
> sender [!]
> If specified, the rule will only be matched if the sender
> email address is found in the table senders. The table
> may contain complete email addresses or apply to an
> entire domain if prefixed with ???@???.
>
> This almost worked for me, except when adding @. in my
> sqlite backend (haven't tested with different backends). I reported
> this way back in 2016 and left it at that, but today I had a machine
> at my $DAYJOB that got an annoying amount of spam from a single
> domain that varied in user component and source ip. So filtering on
> domain would've helped a lot.
>
There was a bug in the mailaddr matching which got fixed a while ago
so this should not be a problem with the smtpd shipped with 6.4
> The following diff implements what the old sender said it would do
> for mail-from and rcpt-to.
>
> So far only lightly tested on a private server.
>
> thoughts?
>
Have you checked that it still doesn't work ??
I've been using the following for many many many months:
match from any mail-from "@qq.com" for any reject
So as far as I know there's no need for your diff...
$ nc localhost 25
220 poolp.org ESMTP OpenSMTPD
helo localhost
250 poolp.org Hello localhost [127.0.0.1], pleased to meet you
mail from:
250 2.0.0: Ok
rcpt to:
550 Invalid recipient
^C
The diff would be wrong anyway but that's another story
> Index: ruleset.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/ruleset.c,v
> retrieving revision 1.36
> diff -u -p -r1.36 ruleset.c
> --- ruleset.c 16 Jun 2018 19:41:26 - 1.36
> +++ ruleset.c 25 Oct 2018 20:18:53 -
> @@ -179,6 +179,13 @@ ruleset_match_smtp_mail_from(struct rule
> table = table_find(env, r->table_smtp_mail_from, NULL);
> if ((ret = ruleset_match_table_lookup(table, key, K_MAILADDR)) < 0)
> return -1;
> + if (ret == 0) {
> + if ((key = strchr(key, '@')) == NULL)
> + return 0;
> + ret = ruleset_match_table_lookup(table, key, K_MAILADDR);
> + if (ret < 0)
> + return -1;
> + }
>
> return r->flag_smtp_mail_from < 0 ? !ret : ret;
> }
> @@ -199,6 +206,13 @@ ruleset_match_smtp_rcpt_to(struct rule *
> table = table_find(env, r->table_smtp_rcpt_to, NULL);
> if ((ret = ruleset_match_table_lookup(table, key, K_MAILADDR)) < 0)
> return -1;
> + if (ret == 0) {
> + if ((key = strchr(key, '@')) == NULL)
> + return 0;
> + ret = ruleset_match_table_lookup(table, key, K_MAILADDR);
> + if (ret < 0)
> + return -1;
> + }
>
> return r->flag_smtp_rcpt_to < 0 ? !ret : ret;
> }
> Index: smtpd.conf.5
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.conf.5,v
> retrieving revision 1.206
> diff -u -p -r1.206 smtpd.conf.5
> --- smtpd.conf.5 8 Oct 2018 06:10:17 - 1.206
> +++ smtpd.conf.5 25 Oct 2018 20:18:53 -
> @@ -531,6 +531,11 @@ Specify that session's HELO / EHLO shoul
> .Xc
> Specify that transactions's MAIL FROM should match the string or list table
> .Ar sender .
> +The
> +.Ar sender
> +may contain complete email addresses or apply to an entire domain if prefixed
> +with
> +.Sq @ .
> .It Xo
> .Op Ic \&!
> .Cm rcpt\-to
> @@ -538,6 +543,11 @@ Specify that transactions's MAIL FROM sh
> .Xc
> Specify that transaction's RCPT TO should match the string or list table
> .Ar recipient .
> +The
> +.Ar recipient
> +may contain complete email addresses or apply to an entire domain if prefixed
> +with
> +.Sq @ .
> .It Xo
> .Op Ic \&!
> .Cm tag Ar tag
>
--
Gilles Chehade
https://www.poolp.org @poolpOrg
Re: Fix descriptions of smtps vs smtp+tls in smtpd.conf.5
On Thu, Oct 25, 2018 at 07:24:33AM +0100, Raf Czlonka wrote: > On Thu, Oct 25, 2018 at 07:11:47AM BST, Gilles Chehade wrote: > > > > smtpd will _always_ display a 'starttls' log line when the TLS channel > > starts, > > disregarding if TLS was started at connect time (smtps) or within the > > protocol > > (smtp+tls, or even smtp since it does opportunistic tls). > > > > I guess this is the confusing bit - seeing 'starttls' in the log > file and thinking 'STARTTLS', i.e. the "TLS upgrade". > yes, maybe it should just display 'tls' instead of 'starttls' -- Gilles Chehade https://www.poolp.org @poolpOrg
Re: Fix descriptions of smtps vs smtp+tls in smtpd.conf.5
On Mon, Oct 22, 2018 at 08:37:25PM -0400, trondd wrote: > Unless I'm confused, it seems the description of the smarthosts smtps and > smtp+tls are revered in the smtpd.conf man page. > You are confused ;-) > My log seemed to back this up. When using smtp+tls, which the man page said > uses STARTTLS but seems to actually use TLS which my ISP does not: > > Oct 21 21:42:58 ember smtpd[41596]: ca9dba5e7f80e6ca mta connecting > address=smtp+tls://68.87.20.6:465 host=omta-ch2.sys.comcast.net > Oct 21 21:42:58 ember smtpd[41596]: ca9dba5e7f80e6ca mta connected > Oct 21 21:43:59 ember smtpd[41596]: ca9dba5e7f80e6ca mta error > reason=Connection closed unexpectedly > You are mistaking smtps and smtp+tls: In an smtps session, the TLS negotation takes place during the connection so client and server are already in a secure channel when the SMTP session gets started. In a smtp+tls session, the TLS negotiation takes place after the session has started in plaintext through the use of the STARTTLS SMTP extension. In your example here, you are using smtp+tls on a host that expects smtps so the TLS negotation can't play out and you're kicked out. > And with smtps, which the man page said uses TLS, logs show STARTTLS: > > Oct 21 22:02:06 ember smtpd[66745]: a9193b70dbc40df0 mta connecting > address=smtps://68.87.20.6:465 host=omta-ch2.sys.comcast.net > Oct 21 22:02:06 ember smtpd[66745]: a9193b70dbc40df0 mta connected > Oct 21 22:02:06 ember smtpd[66745]: a9193b70dbc40df0 mta starttls > ciphers=version=TLSv1.2, cipher=ECDHE-RSA-AES256-GCM-SHA384, bits=256 > Oct 21 22:02:06 ember smtpd[66745]: smtp-out: Server certificate verification > succeeded on session a9193b70dbc40df0 > TLS and STARTTLS are essentially the same as far as you're concerned. smtpd will _always_ display a 'starttls' log line when the TLS channel starts, disregarding if TLS was started at connect time (smtps) or within the protocol (smtp+tls, or even smtp since it does opportunistic tls). The only issue here is that you attempted to connect in plaintext then upgrade a session on a host that didn't speak plaintext and expected sessions to speak TLS from the start. -- Gilles Chehade https://www.poolp.org @poolpOrg
Re: spf walk: lookup aaaa records with "a" mechanism
On Mon, Oct 15, 2018 at 01:08:06AM +0200, Tim Kuijsten wrote:
> Hi,
>
Hi,
> When the "a" designated sender mechanism is used in an spf txt record, both
> v4 and v6 addresses are matched according to [1], so let `smtpctl spf walk`
> resolve both A and records.
>
> [...]
>
> -Tim
>
> [1] https://tools.ietf.org/html/rfc7208#section-5.3
Correct, unfortunately this comes slightly too late for 6.4
Thanks for your diff
> diff --git a/usr.sbin/smtpd/spfwalk.c b/usr.sbin/smtpd/spfwalk.c
> index c4ce2e3d891..22b057963f9 100644
> --- a/usr.sbin/smtpd/spfwalk.c
> +++ b/usr.sbin/smtpd/spfwalk.c
> @@ -192,6 +192,7 @@ dispatch_txt(struct dns_rr *rr)
> }
> if (strncasecmp("a:", *ap, 2) == 0) {
> lookup_record(T_A, *(ap) + 2, dispatch_a);
> + lookup_record(T_, *(ap) + 2, dispatch_);
> continue;
> }
> if (strncasecmp("exists:", *ap, 7) == 0) {
--
Gilles Chehade
https://www.poolp.org @poolpOrg
Re: smtpd: flags cleanup in mta
TA_FORCE_TLS;
> s->flags |= MTA_WANT_SECURE;
> break;
> - case RELAY_TLS_OPTIONAL:
> + case RELAY_TLS_OPPORTUNISTIC:
> /* do not force anything, try tls then smtp */
> break;
> - default:
> + case RELAY_TLS_NO:
> s->flags |= MTA_FORCE_PLAIN;
> + break;
> + default:
> + fatalx("bad value for relay->tls: %d", relay->tls);
> }
>
> if (relay->flags & RELAY_BACKUP)
> Index: smtpd.h
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.h,v
> retrieving revision 1.558
> diff -u -p -r1.558 smtpd.h
> --- smtpd.h 4 Sep 2018 13:04:42 - 1.558
> +++ smtpd.h 5 Sep 2018 12:42:19 -
> @@ -84,11 +84,11 @@
> #define F_RECEIVEDAUTH 0x800
> #define F_MASQUERADE0x1000
>
> +#define RELAY_TLS_OPPORTUNISTIC 0
> +#define RELAY_TLS_STARTTLS 1
> +#define RELAY_TLS_SMTPS 2
> +#define RELAY_TLS_NO 3
>
> -#define RELAY_STARTTLS 0x01
> -#define RELAY_SMTPS 0x02
> -#define RELAY_TLS_OPTIONAL 0x04
> -#define RELAY_SSL(RELAY_STARTTLS | RELAY_SMTPS)
> #define RELAY_AUTH 0x08
> #define RELAY_BACKUP 0x10
> #define RELAY_MX 0x20
> @@ -115,6 +115,7 @@ struct netaddr {
>
> struct relayhost {
> uint16_t flags;
> + int tls;
> char hostname[HOST_NAME_MAX+1];
> uint16_t port;
> char authlabel[PATH_MAX];
> @@ -732,6 +733,7 @@ struct mta_relay {
> struct dispatcher *dispatcher;
> struct mta_domain *domain;
> struct mta_limits *limits;
> + int tls;
> int flags;
> char*backupname;
> int backuppref;
> Index: to.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/to.c,v
> retrieving revision 1.32
> diff -u -p -r1.32 to.c
> --- to.c 3 Sep 2018 11:30:14 - 1.32
> +++ to.c 5 Sep 2018 12:42:19 -
> @@ -304,17 +304,18 @@ text_to_relayhost(struct relayhost *rela
> {
> static const struct schema {
> const char *name;
> - uint16_t flags;
> + int tls;
> + uint16_t flags;
> } schemas [] = {
> /*
>* new schemas should be *appended* otherwise the default
>* schema index needs to be updated later in this function.
>*/
> - { "smtp://",RELAY_TLS_OPTIONAL },
> - { "smtp+tls://",RELAY_STARTTLS },
> - { "smtp+notls://", 0 },
> - { "lmtp://",RELAY_LMTP },
> - { "smtps://", RELAY_SMTPS }
> + { "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
> }
> };
> const char *errstr = NULL;
> char *p, *q;
> @@ -344,6 +345,7 @@ text_to_relayhost(struct relayhost *rela
> else
> p = buffer + strlen(schemas[i].name);
>
> + relay->tls = schemas[i].tls;
> relay->flags = schemas[i].flags;
>
> /* need to specify an explicit port for LMTP */
> @@ -395,7 +397,8 @@ text_to_relayhost(struct relayhost *rela
> return 0;
> if (relay->authlabel[0]) {
> /* disallow auth on non-tls scheme. */
> - if (!(relay->flags & (RELAY_STARTTLS | RELAY_SMTPS)))
> + if (relay->tls != RELAY_TLS_STARTTLS &&
> + relay->tls != RELAY_TLS_SMTPS)
> return 0;
> relay->flags |= RELAY_AUTH;
> }
>
--
Gilles Chehade
https://www.poolp.org @poolpOrg
Re: smtpd: malloc+strlcpy -> strndup
On Mon, Sep 03, 2018 at 11:43:02PM +0800, Michael Mikonos wrote:
> On Mon, Sep 03, 2018 at 02:24:49PM +0800, Michael Mikonos wrote:
> > On Sat, Sep 01, 2018 at 11:31:49PM +0200, Gilles Chehade wrote:
> > > On Sat, Sep 01, 2018 at 09:20:59PM +0800, Michael Mikonos wrote:
> > > > Hello,
> > > >
> > > > Replace a malloc+strlcpy with strndup in cmdline_symset().
> > > > Parameter s is a "keyname=value" string and sym is the
> > > > "keyname" part.
> > > >
> > > > If s is "=value", sym will be an empty string.
> > > > The patch doesn't change this behaviour although
> > > > it might be undesirable to call symset() with
> > > > an empty string. Possibly it could also return -1
> > > > if len is zero. Thoughts?
> > > >
> > >
> > > Not opposed to the diff but at this late hour I find it easier to read
> > > the malloc+strlcpy and be sure there's not an off-by-one than with the
> > > strndup version, I'll read again tomorrow.
> >
> > In my understanding the length argument of strndup(3) doesn't include
> > the terminating NUL character. I think the linux manual for strndup(3)
> > is slightly clearer on this because it has the text:
> >
> > ... only n bytes are copied, and a terminating null byte ('\0') is
> > added.
> >
> > > Just wanted to remind you that this function is shared between daemons
> > > so this can't be an smtpd-only change :-)
>
> Thanks for the reminder. Here is a new version of the patch to include
> other daemons. I also followed a suggestion from halex@ to remove the
> strlen() calls and determine length using val-s. Did I miss anything?
>
this looks good to me
> Index: acme-client/parse.y
> ===
> RCS file: /cvs/src/usr.sbin/acme-client/parse.y,v
> retrieving revision 1.29
> diff -u -p -u -r1.29 parse.y
> --- acme-client/parse.y 3 Aug 2018 17:57:21 - 1.29
> +++ acme-client/parse.y 3 Sep 2018 15:18:23 -
> @@ -839,17 +839,12 @@ cmdline_symset(char *s)
> {
> char*sym, *val;
> int ret;
> - size_t len;
>
> if ((val = strrchr(s, '=')) == NULL)
> return -1;
> -
> - len = strlen(s) - strlen(val) + 1;
> - if ((sym = malloc(len)) == NULL)
> - errx(EXIT_FAILURE, "cmdline_symset: malloc");
> -
> - strlcpy(sym, s, len);
> -
> + sym = strndup(s, val - s);
> + if (sym == NULL)
> + errx(EXIT_FAILURE, "%s: strndup", __func__);
> ret = symset(sym, val + 1, 1);
> free(sym);
>
> Index: bgpd/parse.y
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/parse.y,v
> retrieving revision 1.331
> diff -u -p -u -r1.331 parse.y
> --- bgpd/parse.y 27 Aug 2018 19:32:37 - 1.331
> +++ bgpd/parse.y 3 Sep 2018 15:18:24 -
> @@ -3145,17 +3145,12 @@ cmdline_symset(char *s)
> {
> char*sym, *val;
> int ret;
> - size_t len;
>
> if ((val = strrchr(s, '=')) == NULL)
> return (-1);
> -
> - len = strlen(s) - strlen(val) + 1;
> - if ((sym = malloc(len)) == NULL)
> - fatal("cmdline_symset: malloc");
> -
> - strlcpy(sym, s, len);
> -
> + sym = strndup(s, val - s);
> + if (sym == NULL)
> + fatal("%s: strndup", __func__);
> ret = symset(sym, val + 1, 1);
> free(sym);
>
> Index: dvmrpd/parse.y
> ===
> RCS file: /cvs/src/usr.sbin/dvmrpd/parse.y,v
> retrieving revision 1.36
> diff -u -p -u -r1.36 parse.y
> --- dvmrpd/parse.y11 Jul 2018 07:39:22 - 1.36
> +++ dvmrpd/parse.y3 Sep 2018 15:18:24 -
> @@ -834,17 +834,12 @@ cmdline_symset(char *s)
> {
> char*sym, *val;
> int ret;
> - size_t len;
>
> if ((val = strrchr(s, '=')) == NULL)
> return (-1);
> -
> - len = strlen(s) - strlen(val) + 1;
> - if ((sym = malloc(len)) == NULL)
> - errx(1, "cmdline_symset: malloc");
> -
> - strlcpy(sym, s, len);
> -
> + sym = strndup(s, val - s);
> + if (sym == NULL)
> + errx(1, "%s: strndup", __func__);
> ret = symset(sym, val + 1, 1);
> free(sym);
>
> Index: eigrpd/parse.y
> ===
> RCS file: /cvs/src/usr
Re: Update to table(5) man page
On Wed, Sep 05, 2018 at 07:08:39AM +0100, Jason McIntyre wrote: > On Tue, Sep 04, 2018 at 08:54:37AM -0400, Matt Schwartz wrote: > > Below is a diff to clear up the description of the Userinfo table in > > table(5). I also added an example of how it can be used with an Alias > > table. > > > > Thanks, > > Matt > > > > [...] > > > i think your diff reads better than what's there now. gilles, eric? > agreed -- Gilles Chehade https://www.poolp.org @poolpOrg
Re: Corrected patch for smtpd.conf(5) man page
On Mon, Sep 03, 2018 at 12:30:22PM +0100, Jason McIntyre wrote: > On Sun, Sep 02, 2018 at 11:59:25AM -0400, Matt Schwartz wrote: > > The earlier patch I created was obviously no good. Sorry for the > > noise. Included is the fixed patch that just adds some text for > > properly using an mda wrapper in the actions: > > > > Index: smtpd.conf.5 > > === > > RCS file: /cvs/src/usr.sbin/smtpd/smtpd.conf.5,v > > retrieving revision 1.199 > > diff -u -p -u -r1.199 smtpd.conf.5 > > --- smtpd.conf.51 Sep 2018 19:56:28 - 1.199 > > +++ smtpd.conf.52 Sep 2018 15:53:57 - > > @@ -205,6 +205,9 @@ Use the mapping > > for virtual expansion. > > The aliasing table format is described in > > .Xr table 5 . > > +.It Cm wrapper Ar name > > +Use the wrapper specified in > > +.Cm mda wrapper. > > that looks correct (i think). but with one adjustment - there should be > a space between "wrapper" and the full stop. > > gilles? eric? > yes this reads ok > > .El > > .Pp > > The relay delivery methods also support additional options: > > > -- Gilles Chehade https://www.poolp.org @poolpOrg
Re: smtpd: malloc+strlcpy -> strndup
On Sat, Sep 01, 2018 at 09:20:59PM +0800, Michael Mikonos wrote: > Hello, > > Replace a malloc+strlcpy with strndup in cmdline_symset(). > Parameter s is a "keyname=value" string and sym is the > "keyname" part. > > If s is "=value", sym will be an empty string. > The patch doesn't change this behaviour although > it might be undesirable to call symset() with > an empty string. Possibly it could also return -1 > if len is zero. Thoughts? > Not opposed to the diff but at this late hour I find it easier to read the malloc+strlcpy and be sure there's not an off-by-one than with the strndup version, I'll read again tomorrow. Just wanted to remind you that this function is shared between daemons so this can't be an smtpd-only change :-) > Index: parse.y > === > RCS file: /cvs/src/usr.sbin/smtpd/parse.y,v > retrieving revision 1.218 > diff -u -p -u -r1.218 parse.y > --- parse.y 25 Aug 2018 19:05:23 - 1.218 > +++ parse.y 1 Sep 2018 12:42:45 - > @@ -2129,11 +2129,10 @@ cmdline_symset(char *s) > if ((val = strrchr(s, '=')) == NULL) > return (-1); > > - len = strlen(s) - strlen(val) + 1; > - if ((sym = malloc(len)) == NULL) > - errx(1, "cmdline_symset: malloc"); > - > - (void)strlcpy(sym, s, len); > + len = strlen(s) - strlen(val); > + sym = strndup(s, len); > + if (sym == NULL) > + errx(1, "%s: strndup", __func__); > > ret = symset(sym, val + 1, 1); > free(sym); > -- Gilles Chehade https://www.poolp.org @poolpOrg
Re: smtpd: smtp_client_state() error message
On Sat, Sep 01, 2018 at 06:41:43PM +0800, Michael Mikonos wrote:
> Hello,
>
> smtp_client_state() and smtp_client_response() both do
>
> switch (proto->state) ...
>
> but smtp_client_state() doesn't print the id of the bad state
> in the default case. This patch makes the fatalx() message the
> same for both. Does this look OK?
>
yes, ok for me
> Index: smtp_client.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtp_client.c,v
> retrieving revision 1.6
> diff -u -p -u -r1.6 smtp_client.c
> --- smtp_client.c 30 Aug 2018 11:58:01 - 1.6
> +++ smtp_client.c 1 Sep 2018 10:32:43 -
> @@ -420,7 +420,7 @@ smtp_client_state(struct smtp_client *pr
> break;
>
> default:
> - fatalx("smtp_client_state: unknown state");
> + fatalx("%s: bad state %d", __func__, proto->state);
> }
> #undef smtp_client_state
> }
>
--
Gilles Chehade
https://www.poolp.org @poolpOrg
Re: smtpd: improve syntax for relay host
/* no schema, default to smtp+tls:// */
> - i = 2;
> + /* no schema, default to smtp:// */
> + i = 0;
> p = buffer;
> }
> else
> @@ -397,10 +393,13 @@ text_to_relayhost(struct relayhost *rela
> return 0;
> if ((relay->flags & RELAY_LMTP) && (relay->port == 0))
> return 0;
> - if (relay->authlabel[0] == '\0' && relay->flags & RELAY_AUTH)
> - return 0;
> - if (relay->authlabel[0] != '\0' && !(relay->flags & RELAY_AUTH))
> - return 0;
> + if (relay->authlabel[0]) {
> + /* disallow auth on non-tls scheme. */
> + if (!(relay->flags & (RELAY_STARTTLS | RELAY_SMTPS)))
> + return 0;
> + relay->flags |= RELAY_AUTH;
> + }
> +
> return 1;
> }
>
--
Gilles Chehade
https://www.poolp.org @poolpOrg
Re: smtpd: improve message parser
ng with a '.' */
> + if (line[0] == '.')
> + line += 1;
> +
> + /* account for newline */
> + tx->datain += strlen(line) + 1;
> + if (tx->datain > env->sc_maxsize) {
> + tx->error = TX_ERROR_SIZE;
> + return 0;
> + }
> + }
>
> - /* escape lines starting with a '.' */
> - if (line[0] == '.')
> - line += 1;
> -
> - /* account for newline */
> - tx->datain += strlen(line) + 1;
> - if (tx->datain > env->sc_maxsize) {
> - tx->error = TX_ERROR_SIZE;
> + if (rfc5322_push(tx->parser, line) == -1) {
> + log_warnx("failed to push dataline");
> + tx->error = TX_ERROR_INTERNAL;
> return 0;
> }
>
> - if (!tx->hdrdone) {
> -
> - /* folded header that must be skipped */
> - if (isspace((unsigned char)line[0]) && tx->skiphdr)
> + for(;;) {
> + r = rfc5322_next(tx->parser, );
> + switch (r) {
> + case -1:
> + if (errno == ENOMEM)
> + tx->error = TX_ERROR_INTERNAL;
> + else
> + tx->error = TX_ERROR_MALFORMED;
> return 0;
> - tx->skiphdr = 0;
>
> - /* BCC should be stripped from headers */
> - if (strncasecmp("bcc:", line, 4) == 0) {
> - tx->skiphdr = 1;
> + case RFC5322_NONE:
> + /* Need more data */
> return 0;
> - }
>
> - /* check for loop */
> - if (strncasecmp("Received: ", line, 10) == 0)
> - tx->rcvcount++;
> - if (tx->rcvcount == MAX_HOPS_COUNT) {
> - tx->error = TX_ERROR_LOOP;
> - log_warnx("warn: loop detected");
> - return 0;
> - }
> + case RFC5322_HEADER_START:
> + /* ignore bcc */
> + if (!strcasecmp("Bcc", res.hdr))
> + continue;
> +
> + if (!strcasecmp("To", res.hdr) ||
> + !strcasecmp("Cc", res.hdr) ||
> + !strcasecmp("From", res.hdr)) {
> + rfc5322_unfold_header(tx->parser);
> + continue;
> + }
>
> - if (line[0] == '\0')
> - tx->hdrdone = 1;
> - }
> + if (!strcasecmp("Received", res.hdr)) {
> + if (++tx->rcvcount >= MAX_HOPS_COUNT) {
> + log_warnx("warn: loop detected");
> + tx->error = TX_ERROR_LOOP;
> + return 0;
> + }
> + }
> + else if (!tx->has_date && !strcasecmp("Date", res.hdr))
> + tx->has_date = 1;
> + else if (!tx->has_message_id &&
> + !strcasecmp("Message-Id", res.hdr))
> + tx->has_message_id = 1;
> +
> + smtp_message_printf(tx, "%s:%s\n", res.hdr, res.value);
> + break;
> +
> + case RFC5322_HEADER_CONT:
>
> - ret = rfc2822_parser_feed(>rfc2822_parser, line);
> - if (ret == -1)
> - tx->error = TX_ERROR_RESOURCES;
> + if (!strcasecmp("Bcc", res.hdr) ||
> + !strcasecmp("To", res.hdr) ||
> + !strcasecmp("Cc", res.hdr) ||
> + !strcasecmp("From", res.hdr))
> + continue;
>
> - if (ret == 0)
> - tx->error = TX_ERROR_MALFORMED;
> + smtp_message_printf(tx, "%s\n", res.value);
> + break;
> +
> + case RFC5322_HEADER_END:
> + if (!strcasecmp("To", res.hdr) ||
> + !strcasecmp("Cc", res.hdr) ||
> + !strcasecmp("From", res.hdr))
> + header_domain_append_callback(tx, res.hdr,
> + res.value);
> + break;
> +
> + case RFC5322_END_OF_HEADERS:
> + if (tx->session->listener->local ||
> + tx->session->listener->port == 587) {
>
> - return 0;
> + if (!tx->has_date) {
> + log_debug("debug: %p: adding Date", tx);
> + smtp_message_printf(tx, "Date: %s\n",
> + time_to_text(tx->time));
> + }
> +
> + if (!tx->has_message_id) {
> + log_debug("debug: %p: adding
> Message-ID", tx);
> + smtp_message_printf(tx,
> + "Message-ID: <%016"PRIx64"@%s>\n",
> + generate_uid(),
> + tx->session->listener->hostname);
> + }
> + }
> + break;
> +
> + case RFC5322_BODY_START:
> + case RFC5322_BODY:
> + smtp_message_printf(tx, "%s\n", res.value);
> + break;
> +
> + case RFC5322_END_OF_MESSAGE:
> + return 1;
> +
> + default:
> + fatalx("%s", __func__);
> + }
> + }
> }
>
> static void
> Index: smtpd.h
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.h,v
> retrieving revision 1.556
> diff -u -p -r1.556 smtpd.h
> --- smtpd.h 25 Jul 2018 16:00:48 - 1.556
> +++ smtpd.h 26 Jul 2018 14:40:57 -
> @@ -30,8 +30,6 @@
> #include "smtpd-api.h"
> #include "ioev.h"
>
> -#include "rfc2822.h"
> -
> #define CHECK_IMSG_DATA_SIZE(imsg, expected_sz) do { \
> if ((imsg)->hdr.len - IMSG_HEADER_SIZE != (expected_sz))\
> fatalx("smtpd: imsg %d: data size expected %zd got %zd",\
> Index: smtpd/Makefile
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtpd/Makefile,v
> retrieving revision 1.92
> diff -u -p -r1.92 Makefile
> --- smtpd/Makefile25 Jul 2018 16:00:48 - 1.92
> +++ smtpd/Makefile26 Jul 2018 14:40:57 -
> @@ -36,6 +36,7 @@ SRCS+= pony.c
> SRCS+= queue.c
> SRCS+= queue_backend.c
> SRCS+= resolver.c
> +SRCS+= rfc5322.c
> SRCS+= ruleset.c
> SRCS+= runq.c
> SRCS+= scheduler.c
> @@ -52,9 +53,6 @@ SRCS+= to.c
> SRCS+= tree.c
> SRCS+= util.c
> SRCS+= waitq.c
> -
> -# RFC parsers
> -SRCS+= rfc2822.c
>
> # backends
> SRCS+= compress_gzip.c
>
--
Gilles Chehade
https://www.poolp.org @poolpOrg
Re: smtpd/parse.y : fix line count
index >= 0)
> - return (c);
> + return;
> +
> + if (file->ungetpos >= file->ungetsize) {
> + void *p = reallocarray(file->ungetbuf, file->ungetsize, 2);
> + if (p == NULL)
> + err(1, "lungetc");
> + file->ungetbuf = p;
> + file->ungetsize *= 2;
> }
> - if (pushback_index < MAXPUSHBACK-1)
> - return (pushback_buffer[pushback_index++] = c);
> - else
> - return (EOF);
> + file->ungetbuf[file->ungetpos++] = c;
> }
>
> int
> @@ -1739,9 +1760,6 @@ findeol(void)
> {
> int c;
>
> - parsebuf = NULL;
> - pushback_index = 0;
> -
> /* skip to either EOF or the first real EOL */
> while (1) {
> c = lgetc(0);
> @@ -1772,7 +1790,7 @@ top:
> if (c == '#')
> while ((c = lgetc(0)) != '\n' && c != EOF)
> ; /* nothing */
> - if (c == '$' && parsebuf == NULL) {
> + if (c == '$' && !expanding) {
> while (1) {
> if ((c = lgetc(0)) == EOF)
> return (0);
> @@ -1794,8 +1812,13 @@ top:
> yyerror("macro '%s' not defined", buf);
> return (findeol());
> }
> - parsebuf = val;
> - parseindex = 0;
> + p = val + strlen(val) - 1;
> + lungetc(DONE_EXPAND);
> + while (p >= val) {
> + lungetc(*p);
> + p--;
> + }
> + lungetc(START_EXPAND);
> goto top;
> }
>
> @@ -1957,7 +1980,16 @@ pushfile(const char *name, int secret)
> free(nfile);
> return (NULL);
> }
> - nfile->lineno = 1;
> + nfile->lineno = TAILQ_EMPTY() ? 1 : 0;
> + nfile->ungetsize = 16;
> + nfile->ungetbuf = malloc(nfile->ungetsize);
> + if (nfile->ungetbuf == NULL) {
> + log_warn("warn: malloc");
> + fclose(nfile->stream);
> + free(nfile->name);
> + free(nfile);
> + return (NULL);
> + }
> TAILQ_INSERT_TAIL(, nfile, entry);
> return (nfile);
> }
> @@ -1973,6 +2005,7 @@ popfile(void)
> TAILQ_REMOVE(, file, entry);
> fclose(file->stream);
> free(file->name);
> + free(file->ungetbuf);
> free(file);
> file = prev;
> return (file ? 0 : EOF);
>
--
Gilles Chehade
https://www.poolp.org @poolpOrg
Re: smtpd: make relay to smarthost to verify TLS by default
1.183
> diff -u -p -r1.183 smtpd.conf.5
> --- smtpd.conf.5 31 May 2018 13:36:35 - 1.183
> +++ smtpd.conf.5 31 May 2018 19:56:04 -
> @@ -205,6 +205,9 @@ to advertise during the HELO phase.
> .It Cm host Ar relay-url
> Do not perform MX lookups but relay messages to the relay host described by
> .Ar relay-url .
> +If the url uses tls, the certificate will be verified by default.
> +.It Cm tls Ar no-verify
> +Do not require valid certificate for the specified host.
> .It Cm auth Pf < Ar table Ns >
> Use the mapping
> .Ar table
> Index: smtpd.h
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.h,v
> retrieving revision 1.545
> diff -u -p -r1.545 smtpd.h
> --- smtpd.h 29 May 2018 21:05:52 - 1.545
> +++ smtpd.h 31 May 2018 19:56:05 -
> @@ -1068,6 +1068,7 @@ struct dispatcher_remote {
>
> char*smarthost;
> char*auth;
> + int tls_noverify;
>
> int backup;
> char*backupmx;
>
--
Gilles Chehade
https://www.poolp.org @poolpOrg
Re: smtpd: make relay to smarthost to verify TLS by default
atcher");
> + YYERROR;
> + }
> +
> + dispatcher->u.remote.tls_noverify = 1;
> +}
> | AUTH tables {
> struct table *t = $2;
>
> @@ -1571,6 +1584,7 @@ lookup(char *s)
> { "mta",MTA },
> { "mx", MX },
> { "no-dsn", NODSN },
> + { "no-verify", NOVERIFY },
> { "on", ON },
> { "pki",PKI },
> { "port", PORT },
> Index: mta.c
> =======
> RCS file: /cvs/src/usr.sbin/smtpd/mta.c,v
> retrieving revision 1.212
> diff -u -p -r1.212 mta.c
> --- mta.c 31 May 2018 11:56:10 - 1.212
> +++ mta.c 31 May 2018 13:51:00 -
> @@ -616,6 +616,9 @@ mta_route_next_task(struct mta_relay *re
> m_add_int(p_queue, 0);
> m_close(p_queue);
> }
> +
> + if (dispatcher->u.remote.tls_noverify == 1)
> + evp->agent.mta.relay.flags &= ~F_TLS_VERIFY;
> }
>
> return (task);
>
--
Gilles Chehade
https://www.poolp.org @poolpOrg
Re: corrections in smtpd.conf(5)
On Thu, May 31, 2018 at 03:22:02PM +0200, Sebastien Marie wrote: > Hi, > > The following diff corrects smtpd.conf man page in two ways: > > - Replace virtual(5) reference by table(5) as virtual table format is > documentation in table(5) man page under "Aliasing tables" section. > > - Add "auth " documentation. Example at end of the man page uses > it, so it should be documented. > This reads ok, thanks ! > Index: smtpd.conf.5 > === > RCS file: /cvs/src/usr.sbin/smtpd/smtpd.conf.5,v > retrieving revision 1.182 > diff -u -p -r1.182 smtpd.conf.5 > --- smtpd.conf.5 30 May 2018 13:44:03 - 1.182 > +++ smtpd.conf.5 31 May 2018 13:14:30 - > @@ -179,9 +179,9 @@ option. > .It Cm virtual Pf < Ar table Ns > > Use the mapping > .Ar table > -for > -.Xr virtual 5 > -expansion. > +for virtual expansion. > +The aliasing table format is described in > +.Xr table 5 . > .El > .Pp > The relay delivery methods also support additional options: > @@ -205,6 +205,17 @@ to advertise during the HELO phase. > .It Cm host Ar relay-url > Do not perform MX lookups but relay messages to the relay host described by > .Ar relay-url . > +.It Cm auth Pf < Ar table Ns > > +Use the mapping > +.Ar table > +for connecting to > +.Ar relay-url > +using credentials. > +This option is usable only with > +.Cm host > +option. > +The credential table format is described in > +.Xr table 5 . > .It Cm mail\-from Ar mailaddr > Use > .Ar mailaddr > -- Gilles Chehade https://www.poolp.org @poolpOrg
Re: smtpd.conf new grammar
On Thu, May 24, 2018 at 04:38:17PM -0400, Rupert Gallagher wrote: > On Thu, May 24, 2018 at 14:18, Gilles Chehade <gil...@poolp.org> wrote: > > > In effect, instead of having: > > accept from any for local deliver to mbox > > > > You will have: > > action "my_action" mbox > > match from any for local action "my_action" > > It may solve some obscure technical problem, but is a horrible thing to read > and write. How about keeping the best of both worlds? Leave the old beautiful > PF-like syntax to humans, and translate it into the newEgyptian(tm) on the > fly? It doesn't solve "obscure" technical problems, it solves _many_ issues a lot of users have reported ranging from syntax ambiguity to envelopes on disk not reflecting changes in configuration. One-line rules have lot of consequences which go far beyond the configuration phase: the structures are impacted, the ruleset evaluation is impacted, the aliases expansions are impacted, the queue is impacted, format of envelopes are impacted, I could go on and on since this impacts almost the entire daemon actually. The impact is not just cosmethic stuff. I removed _hundreds_ of lines of very unnecessary and complex code that was ONLY there to make the design error work. Some of these removals led to concrete security improvements like .forward files, written by untrusted users, be processed with their privileges rather than _smtpd. Not doable with one-line rules. I could write pages about the shortcomings from the previous config, and pages about why it can't be made to work and why the new config fixes it in the proper way. We tried hard to find ways to retain a one-line rules configuration but after months we exhausted the ideas and we have a much clearer understanding that it's NOT doable. Either we accept that, or we are just going to grow more complex and slowly stop writing code because every time you touch an area there's a risk you run into the complexity. You don't have to trust my word: > How about keeping the best of both worlds? Leave the old beautiful PF-like > syntax to humans, > and translate it into the newEgyptian(tm) on the fly? If this was possible, then you could easily write a translator that will convert old grammar to new one without removing all the benefits and not reintroducing the complex code. By all means, show me, I'd be impressed for real. -- Gilles Chehade https://www.poolp.org @poolpOrg
smtpd.conf new grammar
Hi, I have just committed a major change in smtpd that'll require smtpd.conf to be rewritten before your update to the new code. The new grammar is not TOO different from the former one, a lot of stuff remains exactly identical, but the ruleset is now split into two parts: - a named action - a matching pattern which is associated to a named action In effect, instead of having: accept from any for local deliver to mbox You will have: action "my_action" mbox match from any for local action "my_action" There are a few keywords that have been shortened too but all in all the switch to new grammar is easy, the smtpd.conf man page has been updated, and it continues being improved thanks to ingo and jmc. The man page by itself should be enough to do the switch. Since this is quite a major change, I also wrote a post that describes a conversion of my own complex smtpd.conf to new grammar: https://poolp.org/posts/2018-05-21/switching-to-opensmtpd-new-config/ I have also compiled a list of directives recognized by the parser which I intend to use for regress tests: https://poolp.org/~gilles/smtpd.conf As for the reasons behind the change they are numerous, I explained some at EuroBSDCon 2017, I explained some on my blog, the bottom line is that while one-line rules were apparently an awesome idea, they were actually a design error that had consequences on pretty much the entire daemon. We didn't realize it until a few months ago, we tried hard to maintain a one-line rule grammar but it became more and more obvious that this just isn't doable without creating issues and unnecessary complexity. The new grammar is cleaner, it helped remove ~700 lines of complex code, made the handling of .forward files as well much safer, removed a lot of very unpleasant side-effects most people didn't even realize existed ... until they hit that one case for which we had no way to work around. Anyways, looking forward for you to test and report how it works for you :-) -- Gilles Chehade https://www.poolp.org @poolpOrg
Re: smtpd: remove filter leftovers
break;
> }
>
> - smtp_filter_mail(s);
> + /* only check sendertable if defined and user has authenticated
> */
> + if (s->flags & SF_AUTHENTICATED && s->listener->sendertable[0])
> {
> + m_create(p_lka, IMSG_SMTP_CHECK_SENDER, 0, 0, -1);
> + m_add_id(p_lka, s->id);
> + m_add_string(p_lka, s->listener->sendertable);
> + m_add_string(p_lka, s->username);
> + m_add_mailaddr(p_lka, >tx->evp.sender);
> + m_close(p_lka);
> + tree_xset(_lka_mail, s->id, s);
> + }
> + else
> + smtp_queue_create_message(s);
> break;
> /*
>* TRANSACTION
> @@ -1710,7 +1589,11 @@ smtp_command(struct smtp_session *s, cha
> if (args && smtp_parse_rcpt_args(s, args) == -1)
> break;
>
> - smtp_filter_rcpt(s);
> + m_create(p_lka, IMSG_SMTP_EXPAND_RCPT, 0, 0, -1);
> + m_add_id(p_lka, s->id);
> + m_add_envelope(p_lka, >tx->evp);
> + m_close(p_lka);
> + tree_xset(_lka_rcpt, s->id, s);
> break;
>
> case CMD_RSET:
> @@ -1745,7 +1628,7 @@ smtp_command(struct smtp_session *s, cha
> break;
> }
>
> - smtp_filter_data(s);
> + smtp_queue_open_message(s);
> break;
> /*
>* ANY
> @@ -2023,6 +1906,7 @@ smtp_lookup_servername(struct smtp_sessi
> static void
> smtp_connected(struct smtp_session *s)
> {
> + struct ca_cert_req_msg req_ca_cert;
> struct sockaddr_storage ss;
> socklen_t sl;
>
> @@ -2037,7 +1921,25 @@ smtp_connected(struct smtp_session *s)
> return;
> }
>
> - smtp_filter_connect(s, (struct sockaddr *));
> + if (s->listener->flags & F_SMTPS) {
> + req_ca_cert.reqid = s->id;
> + if (s->listener->pki_name[0]) {
> + (void)strlcpy(req_ca_cert.name, s->listener->pki_name,
> + sizeof req_ca_cert.name);
> + req_ca_cert.fallback = 0;
> + }
> + else {
> + (void)strlcpy(req_ca_cert.name, s->smtpname,
> + sizeof req_ca_cert.name);
> + req_ca_cert.fallback = 1;
> + }
> + m_compose(p_lka, IMSG_SMTP_TLS_INIT, 0, 0, -1,
> + _ca_cert, sizeof(req_ca_cert));
> + tree_xset(_ssl_init, s->id, s);
> + return;
> + }
> +
> + smtp_send_banner(s);
> }
>
> static void
> @@ -2416,48 +2318,6 @@ smtp_queue_rollback(struct smtp_session
> m_create(p_queue, IMSG_SMTP_MESSAGE_ROLLBACK, 0, 0, -1);
> m_add_msgid(p_queue, s->tx->msgid);
> m_close(p_queue);
> -}
> -
> -static void
> -smtp_filter_connect(struct smtp_session *s, struct sockaddr *sa)
> -{
> - tree_xset(_filter, s->id, s);
> - smtp_filter_response(s->id, QUERY_CONNECT, FILTER_OK, 0, NULL);
> -}
> -
> -static void
> -smtp_filter_eom(struct smtp_session *s)
> -{
> - tree_xset(_filter, s->id, s);
> - smtp_filter_response(s->id, QUERY_EOM, FILTER_OK, 0, NULL);
> -}
> -
> -static void
> -smtp_filter_helo(struct smtp_session *s)
> -{
> - tree_xset(_filter, s->id, s);
> - smtp_filter_response(s->id, QUERY_HELO, FILTER_OK, 0, NULL);
> -}
> -
> -static void
> -smtp_filter_mail(struct smtp_session *s)
> -{
> - tree_xset(_filter, s->id, s);
> - smtp_filter_response(s->id, QUERY_MAIL, FILTER_OK, 0, NULL);
> -}
> -
> -static void
> -smtp_filter_rcpt(struct smtp_session *s)
> -{
> - tree_xset(_filter, s->id, s);
> - smtp_filter_response(s->id, QUERY_RCPT, FILTER_OK, 0, NULL);
> -}
> -
> -static void
> -smtp_filter_data(struct smtp_session *s)
> -{
> - tree_xset(_filter, s->id, s);
> - smtp_filter_response(s->id, QUERY_DATA, FILTER_OK, 0, NULL);
> }
>
> static void
>
--
Gilles Chehade
https://www.poolp.org @poolpOrg
Re: smtpd: tweak static table parser
On Mon, Aug 28, 2017 at 05:27:03PM +0200, Eric Faurot wrote:
> Hi,
>
> The current static (file) table parser is a bit clumsy. It tries to
> determine the type (mapping or list entry) of each line independently
> by splitting on allowed separators (" \t:") without using any context,
> then fails if the type is not what's expected. It's impossible to define
> a list of ipv6 addresses for instance, since ':' is a valid separator.
>
> This diff changes the parser logic. If the table is untyped, determine
> its type by examining the first entry: if it contains a separator, type
> is "mapping", otherwise type is "list". All entries are then parsed
> according to the table type. The "list" type can also be forced by using
> the "@list" directive in a comment. This allows to define list of entries
> containing a separator.
>
> Existing table files should still be working as expected.
> As a bonus, parse errors are now logged with line number.
>
as discussed, i think it's a neat idea
the diff is ok gilles@ too
> Index: table_static.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/table_static.c,v
> retrieving revision 1.16
> diff -u -p -r1.16 table_static.c
> --- table_static.c14 Aug 2017 08:01:14 - 1.16
> +++ table_static.c16 Aug 2017 15:27:22 -
> @@ -73,7 +73,8 @@ static int
> table_static_config(struct table *t)
> {
> FILE*fp;
> - char*buf = NULL;
> + char*buf = NULL, *p;
> + int lineno = 0;
> size_t sz = 0;
> ssize_t flen;
> char*keyp;
> @@ -90,14 +91,47 @@ table_static_config(struct table *t)
> }
>
> while ((flen = getline(, , fp)) != -1) {
> + lineno++;
> if (buf[flen - 1] == '\n')
> - buf[flen - 1] = '\0';
> + buf[--flen] = '\0';
>
> keyp = buf;
> - while (isspace((unsigned char)*keyp))
> + while (isspace((unsigned char)*keyp)) {
> ++keyp;
> - if (*keyp == '\0' || *keyp == '#')
> + --flen;
> + }
> + if (*keyp == '\0')
> + continue;
> + while (isspace((unsigned char)keyp[flen - 1]))
> + keyp[--flen] = '\0';
> + if (*keyp == '#') {
> + if (t->t_type == T_NONE) {
> + keyp++;
> + while (isspace((unsigned char)*keyp))
> + ++keyp;
> + if (!strcmp(keyp, "@list"))
> + t->t_type = T_LIST;
> + }
> continue;
> + }
> +
> + if (t->t_type == T_NONE) {
> + for (p = keyp; *p; p++) {
> + if (*p == ' ' || *p == '\t' || *p == ':') {
> + t->t_type = T_HASH;
> + break;
> + }
> + }
> + if (t->t_type == T_NONE)
> + t->t_type = T_LIST;
> + }
> +
> + if (t->t_type == T_LIST) {
> + table_add(t, keyp, NULL);
> + continue;
> + }
> +
> + /* T_HASH */
> valp = keyp;
> strsep(, " \t:");
> if (valp) {
> @@ -111,18 +145,20 @@ table_static_config(struct table *t)
> if (*valp == '\0')
> valp = NULL;
> }
> + if (valp == NULL) {
> + log_warnx("%s: invalid map entry line %d", t->t_config,
> + lineno);
> + goto end;
> + }
>
> - if (t->t_type == 0)
> - t->t_type = (valp == keyp || valp == NULL) ? T_LIST :
> - T_HASH;
> + table_add(t, keyp, valp);
> + }
>
> - if ((valp == keyp || valp == NULL) && t->t_type == T_LIST)
> - table_add(t, keyp, NULL);
> - else if ((valp != keyp && valp != NULL) && t->t_type == T_HASH)
> - table_add(t, keyp, valp);
> - else
> - goto end;
> + if (ferror(fp)) {
> + log_warn("%s: getline", t->t_config);
> + goto end;
> }
> +
> /* Accept empty alias files; treat them as hashes */
> if (t->t_type == T_NONE && t->t_backend->services & K_ALIAS)
> t->t_type = T_HASH;
>
--
Gilles Chehade
https://www.poolp.org @poolpOrg
Re: smtpd: simplify table parser
On Sun, Aug 13, 2017 at 01:45:48PM +0200, Eric Faurot wrote:
> Remove the table_static_parse() indirection for parsing the file content.
> The "type" parameter is useless since the "(t->t_type & type)" test is always
> true. I think this is a left-over from the old design when table parsing was
> done in context of its intended use in the global config.
>
this is a leftover from when tables were called maps and used to be
declared with a type
ok gilles@
> Index: table_static.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/table_static.c,v
> retrieving revision 1.15
> diff -u -p -r1.15 table_static.c
> --- table_static.c22 Jan 2016 13:08:44 - 1.15
> +++ table_static.c13 Aug 2017 11:28:50 -
> @@ -47,7 +47,6 @@ static int table_static_lookup(void *, s
> static int table_static_fetch(void *, struct dict *, enum table_service,
> union lookup *);
> static void table_static_close(void *);
> -static int table_static_parse(struct table *, const char *, enum table_type);
>
> struct table_backend table_backend_static = {
> K_ALIAS|K_CREDENTIALS|K_DOMAIN|K_NETADDR|K_USERINFO|
> @@ -71,17 +70,7 @@ static struct keycmp {
>
>
> static int
> -table_static_config(struct table *table)
> -{
> - /* no config ? ok */
> - if (*table->t_config == '\0')
> - return 1;
> -
> - return table_static_parse(table, table->t_config, T_LIST|T_HASH);
> -}
> -
> -static int
> -table_static_parse(struct table *t, const char *config, enum table_type type)
> +table_static_config(struct table *t)
> {
> FILE*fp;
> char*buf = NULL;
> @@ -91,10 +80,14 @@ table_static_parse(struct table *t, cons
> char*valp;
> size_t ret = 0;
>
> -if ((fp = fopen(config, "r")) == NULL) {
> -log_warn("warn: Table \"%s\"", config);
> -return 0;
> -}
> + /* no config ? ok */
> + if (*t->t_config == '\0')
> + return 1;
> +
> + if ((fp = fopen(t->t_config, "r")) == NULL) {
> + log_warn("warn: Table \"%s\"", t->t_config);
> + return 0;
> + }
>
> while ((flen = getline(, , fp)) != -1) {
> if (buf[flen - 1] == '\n')
> @@ -122,9 +115,6 @@ table_static_parse(struct table *t, cons
> if (t->t_type == 0)
> t->t_type = (valp == keyp || valp == NULL) ? T_LIST :
> T_HASH;
> -
> - if (!(t->t_type & type))
> - goto end;
>
> if ((valp == keyp || valp == NULL) && t->t_type == T_LIST)
> table_add(t, keyp, NULL);
>
--
Gilles Chehade
https://www.poolp.org @poolpOrg
Re: [patch] remove smtpd filter code
On Fri, Aug 04, 2017 at 02:56:21PM +0200, Gilles Chehade wrote: > On Fri, Aug 04, 2017 at 01:13:06PM +0200, Eric Faurot wrote: > > Hi, > > > > Experimental support for filters has been removed some time ago from > > the config parser. Now we want to get rid of the remaining code. > > It's not that trivial, so we proceed in several steps. > > > > The first (and trickiest) one is to bypass the filter code for > > incoming smtp sessions, so that filter.c can be unhooked. > > This is what the following diff does: > > > > - drop filter configuration, > > - drop filter events, > > - simulate a positive reply for all filter queries, > > - write message content directly to the file. > > > > There should be no functionnal change. > > > > this should be tested by many people right away to spot subtle regressions > Just a clarification because i've received the same question three times since this mail :-) We're not killing filters, most of the implementation is correct and the filters support is pretty much functional at the exception of some cases which are very unfortunately show stoppers... We're removing the filter code from the SMTP state machine to plug it at a different spot and fix the current limitations. Most of the code is to be reused, this is not a rewrite but a refactor. -- Gilles Chehade https://www.poolp.org @poolpOrg
Re: [patch] remove smtpd filter code
;
> @@ -2103,7 +2037,6 @@ smtp_connected(struct smtp_session *s)
> return;
> }
>
> - s->flags |= SF_FILTERCONN;
> smtp_filter_connect(s, (struct sockaddr *));
> }
>
> @@ -2131,7 +2064,6 @@ smtp_message_end(struct smtp_session *s)
> tree_xpop(_filter_data, s->id);
>
> if (s->tx->msgflags & MF_ERROR) {
> - smtp_filter_tx_rollback(s);
> smtp_queue_rollback(s);
> if (s->tx->msgflags & MF_ERROR_SIZE)
> smtp_reply(s, "554 %s %s: Transaction failed, message
> too big",
> @@ -2144,6 +2076,9 @@ smtp_message_end(struct smtp_session *s)
> return;
> }
>
> + fclose(s->tx->ofile);
> + s->tx->ofile = NULL;
> +
> smtp_queue_commit(s);
> }
>
> @@ -2157,7 +2092,7 @@ smtp_message_printf(struct smtp_session
> return -1;
>
> va_start(ap, fmt);
> - len = io_vprintf(s->tx->oev, fmt, ap);
> + len = vfprintf(s->tx->ofile, fmt, ap);
> va_end(ap);
>
> if (len < 0) {
> @@ -2236,13 +2171,9 @@ smtp_free(struct smtp_session *s, const
> if (s->tx) {
> if (s->tx->msgid)
> smtp_queue_rollback(s);
> - smtp_filter_tx_rollback(s);
> smtp_tx_free(s->tx);
> }
>
> - if (s->flags & SF_FILTERCONN)
> - smtp_filter_disconnect(s);
> -
> if (s->flags & SF_SECURE && s->listener->flags & F_SMTPS)
> stat_decrement("smtp.smtps", 1);
> if (s->flags & SF_SECURE && s->listener->flags & F_STARTTLS)
> @@ -2488,83 +2419,45 @@ smtp_queue_rollback(struct smtp_session
> }
>
> static void
> -smtp_filter_rset(struct smtp_session *s)
> -{
> - filter_event(s->id, EVENT_RESET);
> -}
> -
> -static void
> -smtp_filter_tx_begin(struct smtp_session *s)
> -{
> - s->flags |= SF_FILTERTX;
> - filter_event(s->id, EVENT_TX_BEGIN);
> -}
> -
> -static void
> -smtp_filter_tx_commit(struct smtp_session *s)
> -{
> - s->flags &= ~SF_FILTERTX;
> - filter_event(s->id, EVENT_TX_COMMIT);
> -}
> -
> -static void
> -smtp_filter_tx_rollback(struct smtp_session *s)
> -{
> - s->flags &= ~SF_FILTERTX;
> - filter_event(s->id, EVENT_TX_ROLLBACK);
> -}
> -
> -static void
> -smtp_filter_disconnect(struct smtp_session *s)
> -{
> - filter_event(s->id, EVENT_DISCONNECT);
> -}
> -
> -static void
> smtp_filter_connect(struct smtp_session *s, struct sockaddr *sa)
> {
> - char*filter;
> -
> tree_xset(_filter, s->id, s);
> -
> - filter = s->listener->filter[0] ? s->listener->filter : NULL;
> -
> - filter_connect(s->id, sa, (struct sockaddr *)>ss, s->hostname,
> filter);
> + smtp_filter_response(s->id, QUERY_CONNECT, FILTER_OK, 0, NULL);
> }
>
> static void
> smtp_filter_eom(struct smtp_session *s)
> {
> tree_xset(_filter, s->id, s);
> - filter_eom(s->id, QUERY_EOM, s->tx->odatalen);
> + smtp_filter_response(s->id, QUERY_EOM, FILTER_OK, 0, NULL);
> }
>
> static void
> smtp_filter_helo(struct smtp_session *s)
> {
> tree_xset(_filter, s->id, s);
> - filter_line(s->id, QUERY_HELO, s->helo);
> + smtp_filter_response(s->id, QUERY_HELO, FILTER_OK, 0, NULL);
> }
>
> static void
> smtp_filter_mail(struct smtp_session *s)
> {
> tree_xset(_filter, s->id, s);
> - filter_mailaddr(s->id, QUERY_MAIL, >tx->evp.sender);
> + smtp_filter_response(s->id, QUERY_MAIL, FILTER_OK, 0, NULL);
> }
>
> static void
> smtp_filter_rcpt(struct smtp_session *s)
> {
> tree_xset(_filter, s->id, s);
> - filter_mailaddr(s->id, QUERY_RCPT, >tx->evp.rcpt);
> + smtp_filter_response(s->id, QUERY_RCPT, FILTER_OK, 0, NULL);
> }
>
> static void
> smtp_filter_data(struct smtp_session *s)
> {
> tree_xset(_filter, s->id, s);
> - filter_line(s->id, QUERY_DATA, NULL);
> + smtp_filter_response(s->id, QUERY_DATA, FILTER_OK, 0, NULL);
> }
>
> static void
> @@ -2624,11 +2517,6 @@ smtp_filter_dataline(struct smtp_session
> if (ret == 0) {
> s->tx->msgflags |= MF_ERROR_MALFORMED;
> return;
> - }
> -
> - if (io_queued(s->tx->oev) > DATA_HIWAT && !io_paused(s->io, IO_IN)) {
> - log_debug("debug: smtp: %p: filter congestion: pausing
> session", s);
> - io_pause(s->io, IO_IN);
> }
> }
>
> Index: smtpd.h
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.h,v
> retrieving revision 1.533
> diff -u -p -r1.533 smtpd.h
> --- smtpd.h 27 Jul 2017 18:48:30 - 1.533
> +++ smtpd.h 4 Aug 2017 09:53:16 -
> @@ -1200,18 +1200,6 @@ int expand_to_text(struct expand *, char
> RB_PROTOTYPE(expandtree, expandnode, nodes, expand_cmp);
>
>
> -/* filter.c */
> -void filter_postfork(void);
> -void filter_configure(void);
> -void filter_connect(uint64_t, const struct sockaddr *,
> -const struct sockaddr *, const char *, const char *);
> -void filter_mailaddr(uint64_t, int, const struct mailaddr *);
> -void filter_line(uint64_t, int, const char *);
> -void filter_eom(uint64_t, int, size_t);
> -void filter_event(uint64_t, int);
> -void filter_build_fd_chain(uint64_t, int);
> -
> -
> /* forward.c */
> int forwards_get(int, struct expand *);
>
> Index: smtpd/Makefile
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtpd/Makefile,v
> retrieving revision 1.87
> diff -u -p -r1.87 Makefile
> --- smtpd/Makefile26 May 2017 21:30:00 - 1.87
> +++ smtpd/Makefile3 Aug 2017 09:55:57 -
> @@ -17,7 +17,6 @@ SRCS+= dns.c
> SRCS+= envelope.c
> SRCS+= esc.c
> SRCS+= expand.c
> -SRCS+= filter.c
> SRCS+= forward.c
> SRCS+= iobuf.c
> SRCS+= ioev.c
>
--
Gilles Chehade
https://www.poolp.org @poolpOrg
Re: smtpd session hang
On Fri, Jun 16, 2017 at 07:12:43PM +0300, Henri Kemppainen wrote: > > > Nice catch, the diff reads fine to me, I'll commit later today when I > > > have another ok from eric@ > > > Yes, this looks correct. But, I would rather move the resume test before > > the EOM test, to avoid touching the session after the transfer has been > > finalized by smtp_data_io_done(). > > It oughtn't make a difference as long as the duplex control is correct, > but I'm fine with that change. > > > > side note, smtpd should not have been able to leak more than 5 fd, it > > > should not have been able to exhaust, is this what you observed ? > > For the record, we discussed this with Gilles on irc and while we saw > more than a dozen leaked fds, it's okay as smtpd will allow as many > incoming sessions as the dtable can accommodate (with an fd reserve). > The lower limits are on outgoing connections. > > New diff with reordered code. I'll see if I can get Adam to run one more > round of testing.. > Committed thanks :) -- Gilles Chehade https://www.poolp.org @poolpOrg
Re: smtpd session hang
On Fri, Jun 16, 2017 at 01:05:25AM +0300, Henri Kemppainen wrote:
> I had a little debugging session with awolk@ over at #openbsd-daily. His
> smtpd would over time end up with hung sessions that never timeout.
>
> The problem is related to the data_io path's congestion control which
> may pause the session. In this case the io system will not wait for
> read events and as such will not have a chance to timeout until it is
> resumed.
>
> If the pause happens when a full message is just about to pass through
> the data_io path, the session is never resumed, even though there is
> obviously no more congestion and the session should be reading more
> input from the client again.
>
> A debug trace excerpt shows the course of events:
>
> mtp: 0xe54baa1e000: IO_DATAIN ob=0>
> debug: smtp: 0xe54baa1e000: filter congestion: pausing session
> smtp: 0xe54baa1e000 (data): IO_LOWAT ob=0>
> debug: smtp: 0xe54baa1e000: data io done (259146 bytes)
> debug: 0xe54baa1e000: end of message, msgflags=0x
> smtp: 0xe54baa1e000: >>> 250 2.0.0: 4f36f9e3 Message accepted for delivery
> smtp: 0xe54baa1e000: STATE_BODY -> STATE_HELO
> smtp: 0xe54baa1e000: IO_LOWAT ib=0 ob=0>
>
> From this point on, session 0xe54baa1e000 and its io 0xe551d0d5000
> (which has the pause_in flag) are never seen again in the trace, and
> fstat shows a corresponding connection to smtpd that never goes away.
>
> The proposed fix is to always resume the session if the data_io path
> hits the low water mark.
>
> Mr. Wolk tested this diff against smtpd on 6.1 as well as a against
> -current version of smtpd (compiled on the same system running 6.1).
>
Nice catch, the diff reads fine to me, I'll commit later today when I
have another ok from eric@
side note, smtpd should not have been able to leak more than 5 fd, it
should not have been able to exhaust, is this what you observed ?
> Index: usr.sbin/smtpd/smtp_session.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v
> retrieving revision 1.303
> diff -u -p -r1.303 smtp_session.c
> --- usr.sbin/smtpd/smtp_session.c 17 May 2017 14:00:06 - 1.303
> +++ usr.sbin/smtpd/smtp_session.c 15 Jun 2017 20:28:12 -
> @@ -1474,9 +1474,10 @@ smtp_data_io(struct io *io, int evt, voi
> break;
>
> case IO_LOWAT:
> - if (s->tx->dataeom && io_queued(s->tx->oev) == 0) {
> + if (s->tx->dataeom && io_queued(s->tx->oev) == 0)
> smtp_data_io_done(s);
> - } else if (io_paused(s->io, IO_IN)) {
> +
> + if (io_paused(s->io, IO_IN)) {
> log_debug("debug: smtp: %p: filter congestion over:
> resuming session", s);
> io_resume(s->io, IO_IN);
> }
>
>
--
Gilles Chehade
https://www.poolp.org @poolpOrg
Re: [PATCH] Do not mention newaliases(8) in /etc/mail/aliases
On Wed, May 31, 2017 at 09:53:38AM -0500, Jimmy Hess wrote: > On Wed, May 31, 2017 at 6:34 AM, Gilles Chehade <gil...@poolp.org> wrote: > > > It is not that simple because newaliases works when you have one aliases > > database (e.g. /etc/mail/aliases). This is the case on the default setup > > but smtpd supports per-rule aliases mappings and for example the MX that > > I run for poolp.org > > Sounds like newaliases should check the config for the existence of > multiple aliases > mappings, And either enumerate and make sure all of them get refreshed, Or > return an error listing out possible alias mappings to refresh and > ask you to pick > > newaliases (-a | ) > I'm not opposed to adding parameter to newaliases I'm very opposed to implicitely refreshing everything automatically. > Either way, requiring some wordy incantation such as"smtpctl > update table aliases" > to get what newaliases did for standard configurations is not very cool. > technically, this can be fixed with mailwrapper(8) which is why we did not bother with a shortcut directly in the code, we should probably do a mailwrapper shortcut out of the box even if it doesn't work on other setups. > Or for non-standard configurations that break newaliases list as a > documented caveat > of using the options for custom tables. > > -- > -JH -- Gilles Chehade https://www.poolp.org @poolpOrg
Re: [PATCH] Do not mention newaliases(8) in /etc/mail/aliases
On Wed, May 31, 2017 at 11:59:08AM +0100, Stuart Henderson wrote: > On 2017/05/31 11:10, Jason McIntyre wrote: > > On Wed, May 31, 2017 at 11:49:18AM +0200, Antoine Jacoutot wrote: > > > On May 31, 2017 11:35:28 AM GMT+02:00, Consus <con...@gmx.com> wrote: > > > >OpenBSD defaults to file table now so there is no need in running > > > >newaliases(8). > > > >--- > > > > etc/mail/aliases | 8 ++-- > > > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > > > > >diff --git a/etc/mail/aliases b/etc/mail/aliases > > > >index c1ac04b5a81..045b2b2a456 100644 > > > >--- a/etc/mail/aliases > > > >+++ b/etc/mail/aliases > > > >@@ -1,12 +1,8 @@ > > > > # > > > > # $OpenBSD: aliases,v 1.64 2017/03/18 21:18:01 florian Exp $ > > > > # > > > >-# Aliases in this file will NOT be expanded in the header from > > > >-# Mail, but WILL be visible over networks or from > > > >/usr/libexec/mail.local. > > > >-# > > > >-# >>>>>>>>>> The program "newaliases" must be run after > > > >-# >> NOTE >> this file is updated for any changes to > > > >-# >>>>>>>>>> show through to smtpd. > > > >+# Aliases in this file will NOT be expanded in the header from Mail, > > > >but WILL > > > >+# be visible over networks or from /usr/libexec/mail.local. > > > > # > > > > > > > > # Basic system aliases -- these MUST be present > > > >-- > > > >2.13.0 > > > > > > I proposed the same a while ago but people preferred to keep it in regard > > > to other MTA. Maybe it's time to revisit ? > > > > > > > > > > well what's there now is incorrect, so i think something needs to > > happen. even if we just prefix the text with "For databases" or > > something. > > But you need "smtpctl update table aliases" instead don't you? (At least > that is how I read the manual). > > (It would seem useful if "newaliases" did whatever is necessary for the > table type you have in use so you don't need to think about it and the > documentation can be simple..) > It is not that simple because newaliases works when you have one aliases database (e.g. /etc/mail/aliases). This is the case on the default setup but smtpd supports per-rule aliases mappings and for example the MX that I run for poolp.org and opensmtpd.org has two different sets of aliases, one for each domain, and now newaliases can't work anymore. We can hack it so it works in some cases but it will never be able to do the work correctly for non-default configurations and we will still have to provide and document the smtpctl command. -- Gilles Chehade https://www.poolp.org @poolpOrg
Re: [patch] typo smtpd/dns.c
On Tue, May 30, 2017 at 05:26:08PM -0500, Edgar Pettijohn wrote:
> fix typo
thanks, will commit
when sending diffs, please inline them instead of attaching as it is
easier for us to work with them that way
Gilles
> Index: dns.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/dns.c,v
> retrieving revision 1.83
> diff -u -p -u -r1.83 dns.c
> --- dns.c 28 Oct 2015 07:28:13 - 1.83
> +++ dns.c 30 May 2017 22:09:15 -
> @@ -246,7 +246,7 @@ dns_imsg(struct mproc *p, struct imsg *i
>
> as = res_query_async(s->name, C_IN, T_MX, NULL);
> if (as == NULL) {
> - log_warn("warn: req_query_async: %s", s->name);
> + log_warn("warn: res_query_async: %s", s->name);
> m_create(s->p, IMSG_MTA_DNS_HOST_END, 0, 0, -1);
> m_add_id(s->p, s->reqid);
> m_add_int(s->p, DNS_EINVAL);
--
Gilles Chehade
https://www.poolp.org @poolpOrg
Re: Convert explicit_bzero+free to freezero on smtpd(8)
On Thu, May 11, 2017 at 11:33:10AM +0100, Ricardo Mestre wrote:
> Hi,
>
> This converts explicit_bzero+free to freezero on smtpd(8).
>
> OK?
Sorry i was away from town
I'll have a look at freezero() tomorrow as I missed most of the
discussion about its semantics and I'll ok then
Thanks
> Index: ca.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/ca.c,v
> retrieving revision 1.26
> diff -u -p -u -r1.26 ca.c
> --- ca.c 9 Jan 2017 09:53:23 - 1.26
> +++ ca.c 11 May 2017 10:16:47 -
> @@ -142,8 +142,7 @@ ca_init(void)
>
> pki->pki_pkey = pkey;
>
> - explicit_bzero(pki->pki_key, pki->pki_key_len);
> - free(pki->pki_key);
> + freezero(pki->pki_key, pki->pki_key_len);
> pki->pki_key = NULL;
> }
> }
> Index: config.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/config.c,v
> retrieving revision 1.37
> diff -u -p -u -r1.37 config.c
> --- config.c 1 Sep 2016 10:54:25 - 1.37
> +++ config.c 11 May 2017 10:16:48 -
> @@ -70,12 +70,8 @@ purge_config(uint8_t what)
> }
> if (what & PURGE_PKI) {
> while (dict_poproot(env->sc_pki_dict, (void **))) {
> - explicit_bzero(p->pki_cert, p->pki_cert_len);
> - free(p->pki_cert);
> - if (p->pki_key) {
> - explicit_bzero(p->pki_key, p->pki_key_len);
> - free(p->pki_key);
> - }
> + freezero(p->pki_cert, p->pki_cert_len);
> + freezero(p->pki_key, p->pki_key_len);
> if (p->pki_pkey)
> EVP_PKEY_free(p->pki_pkey);
> free(p);
> @@ -86,14 +82,10 @@ purge_config(uint8_t what)
> iter_dict = NULL;
> while (dict_iter(env->sc_pki_dict, _dict, ,
> (void **))) {
> - explicit_bzero(p->pki_cert, p->pki_cert_len);
> - free(p->pki_cert);
> + freezero(p->pki_cert, p->pki_cert_len);
> p->pki_cert = NULL;
> - if (p->pki_key) {
> - explicit_bzero(p->pki_key, p->pki_key_len);
> - free(p->pki_key);
> - p->pki_key = NULL;
> - }
> + freezero(p->pki_key, p->pki_key_len);
> + p->pki_key = NULL;
> if (p->pki_pkey)
> EVP_PKEY_free(p->pki_pkey);
> p->pki_pkey = NULL;
> Index: mta_session.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/mta_session.c,v
> retrieving revision 1.96
> diff -u -p -u -r1.96 mta_session.c
> --- mta_session.c 30 Nov 2016 17:43:32 - 1.96
> +++ mta_session.c 11 May 2017 10:16:50 -
> @@ -341,8 +341,7 @@ mta_session_imsg(struct mproc *p, struct
> fatal("mta: ssl_mta_init");
> io_start_tls(s->io, ssl);
>
> - explicit_bzero(resp_ca_cert->cert, resp_ca_cert->cert_len);
> - free(resp_ca_cert->cert);
> + freezero(resp_ca_cert->cert, resp_ca_cert->cert_len);
> free(resp_ca_cert);
> return;
>
> Index: smtp_session.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v
> retrieving revision 1.302
> diff -u -p -u -r1.302 smtp_session.c
> --- smtp_session.c30 Nov 2016 17:43:32 - 1.302
> +++ smtp_session.c11 May 2017 10:16:54 -
> @@ -962,8 +962,7 @@ smtp_session_imsg(struct mproc *p, struc
> io_set_read(s->io);
> io_start_tls(s->io, ssl);
>
> - explicit_bzero(resp_ca_cert->cert, resp_ca_cert->cert_len);
> - free(resp_ca_cert->cert);
> + freezero(resp_ca_cert->cert, resp_ca_cert->cert_len);
> free(resp_ca_cert);
> return;
>
--
Gilles Chehade
https://www.poolp.org @poolpOrg
Re: regarding OpenSSL License change
On Fri, Mar 24, 2017 at 11:55:10AM -0400, Michael W. Lucas wrote: > On Fri, Mar 24, 2017 at 02:37:58PM +0100, Sebastian Benoit wrote: > > It's about "You cannot change the licence without consent of the author" and > > "We just assume that you say yes to this because we dont care about your > > rights", which is morally and legally wrong. > > > It's very simple. Four words. > > "Silence is not consent." > > Not in contracts. Not in sex. And not in licensing. > This is the clearest description of the situation. Sadly, "clear" is something the OpenSSL folks are unfamiliar with... -- Gilles Chehade https://www.poolp.org @poolpOrg
Re: wpa key vs wep key
On Tue, Mar 21, 2017 at 09:16:39AM +0100, Antoine Jacoutot wrote:
> On Tue, Mar 21, 2017 at 02:10:48PM +0900, Stefan Sperling wrote:
> > I see no reason to leave WEP enabled if a WPA key is set, and leaving
> > WPA enabled when a WEP key is set.
> >
> > Several cases of "my wifi suddenly stopped working" turned out to be due
> > to stale WEP keys interfering with WPA. I think it is better to let the
> > kernel handle this transition instead of requiring 'ifconfig -nwkey'.
>
> I fully agree. I've been annoyed by this countless times :-)
>
same here
> > Index: ieee80211_ioctl.c
> > ===
> > RCS file: /cvs/src/sys/net80211/ieee80211_ioctl.c,v
> > retrieving revision 1.50
> > diff -u -p -r1.50 ieee80211_ioctl.c
> > --- ieee80211_ioctl.c 12 Mar 2017 03:13:50 - 1.50
> > +++ ieee80211_ioctl.c 21 Mar 2017 05:03:46 -
> > @@ -55,6 +55,8 @@ void ieee80211_node2req(struct ieee8021
> > const struct ieee80211_node *, struct ieee80211_nodereq *);
> > voidieee80211_req2node(struct ieee80211com *,
> > const struct ieee80211_nodereq *, struct ieee80211_node *);
> > +voidieee80211_disable_wep(struct ieee80211com *);
> > +voidieee80211_disable_rsn(struct ieee80211com *);
> >
> > void
> > ieee80211_node2req(struct ieee80211com *ic, const struct ieee80211_node
> > *ni,
> > @@ -166,6 +168,32 @@ ieee80211_req2node(struct ieee80211com *
> > ni->ni_state = nr->nr_state;
> > }
> >
> > +void
> > +ieee80211_disable_wep(struct ieee80211com *ic)
> > +{
> > + struct ieee80211_key *k;
> > + int i;
> > +
> > + for (i = 0; i < IEEE80211_WEP_NKID; i++) {
> > + k = >ic_nw_keys[i];
> > + if (k->k_cipher != IEEE80211_CIPHER_NONE)
> > + (*ic->ic_delete_key)(ic, NULL, k);
> > + memset(k, 0, sizeof(*k));
> > + }
> > + ic->ic_flags &= ~IEEE80211_F_WEPON;
> > +}
> > +
> > +void
> > +ieee80211_disable_rsn(struct ieee80211com *ic)
> > +{
> > + ic->ic_flags &= ~(IEEE80211_F_PSK | IEEE80211_F_RSNON);
> > + memset(ic->ic_psk, 0, sizeof(ic->ic_psk));
> > + ic->ic_rsnprotos = 0;
> > + ic->ic_rsnakms = 0;
> > + ic->ic_rsngroupcipher = 0;
> > + ic->ic_rsnciphers = 0;
> > +}
> > +
> > static int
> > ieee80211_ioctl_setnwkeys(struct ieee80211com *ic,
> > const struct ieee80211_nwkey *nwkey)
> > @@ -212,6 +240,8 @@ ieee80211_ioctl_setnwkeys(struct ieee802
> >
> > ic->ic_def_txkey = nwkey->i_defkid - 1;
> > ic->ic_flags |= IEEE80211_F_WEPON;
> > + if (ic->ic_flags & IEEE80211_F_RSNON)
> > + ieee80211_disable_rsn(ic);
> >
> > return ENETRESET;
> > }
> > @@ -464,6 +494,8 @@ ieee80211_ioctl(struct ifnet *ifp, u_lon
> > if (psk->i_enabled) {
> > ic->ic_flags |= IEEE80211_F_PSK;
> > memcpy(ic->ic_psk, psk->i_psk, sizeof(ic->ic_psk));
> > + if (ic->ic_flags & IEEE80211_F_WEPON)
> > + ieee80211_disable_wep(ic);
> > } else {
> > ic->ic_flags &= ~IEEE80211_F_PSK;
> > memset(ic->ic_psk, 0, sizeof(ic->ic_psk));
> > @@ -496,6 +528,8 @@ ieee80211_ioctl(struct ifnet *ifp, u_lon
> > break;
> > kr = (struct ieee80211_keyrun *)data;
> > error = ieee80211_keyrun(ic, kr->i_macaddr);
> > + if (error == 0 && (ic->ic_flags & IEEE80211_F_WEPON))
> > + ieee80211_disable_wep(ic);
> > break;
> > case SIOCS80211POWER:
> > if ((error = suser(curproc, 0)) != 0)
> >
>
> --
> Antoine
>
--
Gilles Chehade
https://www.poolp.org @poolpOrg
Re: vmd 4/5: replace openpty(4) with a local function
t; + */
> + if ((ioctl(env->vmd_ptmfd, PTMGET, ) == -1))
> + return (-1);
> +
> + vm->vm_tty = ptm.cfd;
> + close(ptm.sfd);
> + if ((vm->vm_ttyname = strdup(ptm.sn)) == NULL)
> + goto fail;
> +
> + return (0);
> + fail:
> + vm_closetty(vm);
> + return (-1);
> +}
> +
> +void
> +vm_closetty(struct vmd_vm *vm)
> +{
> + if (vm->vm_tty != -1) {
> + close(vm->vm_tty);
> + vm->vm_tty = -1;
> + }
> + free(vm->vm_ttyname);
> + vm->vm_ttyname = NULL;
> +}
> +
> void
> switch_remove(struct vmd_switch *vsw)
> {
> diff --git usr.sbin/vmd/vmd.h usr.sbin/vmd/vmd.h
> index e371112..26d345c 100644
> --- usr.sbin/vmd/vmd.h
> +++ usr.sbin/vmd/vmd.h
> @@ -185,6 +185,7 @@ struct vmd {
> struct switchlist *vmd_switches;
>
> int vmd_fd;
> + int vmd_ptmfd;
> };
>
> /* vmd.c */
> @@ -197,6 +198,8 @@ void vm_stop(struct vmd_vm *, int);
> void vm_remove(struct vmd_vm *);
> int vm_register(struct privsep *, struct vmop_create_params *,
> struct vmd_vm **, uint32_t);
> +int vm_opentty(struct vmd_vm *);
> +void vm_closetty(struct vmd_vm *);
> void switch_remove(struct vmd_switch *);
> struct vmd_switch *switch_getbyname(const char *);
> char *get_string(uint8_t *, size_t);
>
--
Gilles Chehade
https://www.poolp.org @poolpOrg
Re: vmd 3/5: add size checks for control imsg
On Mon, Feb 27, 2017 at 10:52:14AM +0100, Reyk Floeter wrote:
> Reminder: using IMSG_SIZE_CHECK() in user-facing imsg handlers is a
> bad thing as an invalid imsg would kill the daemon (via fatal).
>
> OK?
>
> Add size checks for imsg received over the control socket.
>
> Additionally, make sure that vmd never fatal()s when receiving an
> invalid imsg from an arbitrary user over the control socket.
>
ok gilles@
> diff --git usr.sbin/vmd/control.c usr.sbin/vmd/control.c
> index 5e0141f..cda7df9 100644
> --- usr.sbin/vmd/control.c
> +++ usr.sbin/vmd/control.c
> @@ -286,11 +286,13 @@ control_close(int fd, struct control_sock *cs)
> void
> control_dispatch_imsg(int fd, short event, void *arg)
> {
> - struct control_sock *cs = arg;
> - struct privsep *ps = cs->cs_env;
> - struct ctl_conn *c;
> - struct imsg imsg;
> - int n, v, ret = 0;
> + struct control_sock *cs = arg;
> + struct privsep *ps = cs->cs_env;
> + struct ctl_conn *c;
> + struct imsg imsg;
> + struct vmop_create_paramsvmc;
> + struct vmop_id vid;
> + int n, v, ret = 0;
>
> if ((c = control_connbyfd(fd)) == NULL) {
> log_warn("%s: fd %d: not found", __func__, fd);
> @@ -347,7 +349,8 @@ control_dispatch_imsg(int fd, short event, void *arg)
> c->flags |= CTL_CONN_NOTIFY;
> break;
> case IMSG_CTL_VERBOSE:
> - IMSG_SIZE_CHECK(, );
> + if (IMSG_DATA_SIZE() < sizeof(v))
> + goto fail;
>
> memcpy(, imsg.data, sizeof(v));
> log_setverbose(v);
> @@ -355,17 +358,34 @@ control_dispatch_imsg(int fd, short event, void *arg)
> proc_forward_imsg(ps, , PROC_PARENT, -1);
> break;
> case IMSG_VMDOP_START_VM_REQUEST:
> + if (IMSG_DATA_SIZE() < sizeof(vmc))
> + goto fail;
> + if (proc_compose_imsg(ps, PROC_PARENT, -1,
> + imsg.hdr.type, fd, -1,
> + imsg.data, IMSG_DATA_SIZE()) == -1) {
> + control_close(fd, cs);
> + return;
> + }
> + break;
> case IMSG_VMDOP_TERMINATE_VM_REQUEST:
> - case IMSG_VMDOP_GET_INFO_VM_REQUEST:
> - imsg.hdr.peerid = fd;
> -
> + if (IMSG_DATA_SIZE() < sizeof(vid))
> + goto fail;
> if (proc_compose_imsg(ps, PROC_PARENT, -1,
> - imsg.hdr.type, imsg.hdr.peerid, -1,
> + imsg.hdr.type, fd, -1,
> imsg.data, IMSG_DATA_SIZE()) == -1) {
> control_close(fd, cs);
> return;
> }
> break;
> + case IMSG_VMDOP_GET_INFO_VM_REQUEST:
> + if (IMSG_DATA_SIZE() != 0)
> + goto fail;
> + if (proc_compose_imsg(ps, PROC_PARENT, -1,
> + imsg.hdr.type, fd, -1, NULL, 0) == -1) {
> + control_close(fd, cs);
> + return;
> + }
> + break;
> case IMSG_VMDOP_LOAD:
> case IMSG_VMDOP_RELOAD:
> case IMSG_CTL_RESET:
> @@ -384,6 +404,8 @@ control_dispatch_imsg(int fd, short event, void *arg)
> return;
>
> fail:
> + if (ret == 0)
> + ret = EINVAL;
> imsg_compose_event(>iev, IMSG_CTL_FAIL,
> 0, 0, -1, , sizeof(ret));
> imsg_flush(>iev.ibuf);
>
--
Gilles Chehade
https://www.poolp.org @poolpOrg
Re: asr: support for RES_USE_DNSSEC
(0x1 << 5)
> +#define CD_MASK (0x1 << 4)
> #define RCODE_MASK (0xf)
>
> #define OPCODE(v)((v) & OPCODE_MASK)
> @@ -297,7 +299,7 @@ __BEGIN_HIDDEN_DECLS
> void _asr_pack_init(struct asr_pack *, char *, size_t);
> int _asr_pack_header(struct asr_pack *, const struct asr_dns_header *);
> int _asr_pack_query(struct asr_pack *, uint16_t, uint16_t, const char *);
> -int _asr_pack_edns0(struct asr_pack *, uint16_t);
> +int _asr_pack_edns0(struct asr_pack *, uint16_t, int);
> void _asr_unpack_init(struct asr_unpack *, const char *, size_t);
> int _asr_unpack_header(struct asr_unpack *, struct asr_dns_header *);
> int _asr_unpack_query(struct asr_unpack *, struct asr_dns_query *);
> Index: asr/asr_utils.c
> ===
> RCS file: /d/cvs/src/lib/libc/asr/asr_utils.c,v
> retrieving revision 1.16
> diff -u -p -r1.16 asr_utils.c
> --- asr/asr_utils.c 19 Feb 2017 12:02:30 - 1.16
> +++ asr/asr_utils.c 27 Feb 2017 07:25:11 -
> @@ -423,12 +423,19 @@ _asr_pack_query(struct asr_pack *p, uint
> }
>
> int
> -_asr_pack_edns0(struct asr_pack *p, uint16_t pktsz)
> +_asr_pack_edns0(struct asr_pack *p, uint16_t pktsz, int dnssec_do)
> {
> + DPRINT("asr EDNS0 pktsz:%hu dnssec:%s\n", pktsz,
> + dnssec_do ? "yes" : "no");
> +
> pack_dname(p, ""); /* root */
> pack_u16(p, T_OPT); /* OPT */
> pack_u16(p, pktsz); /* UDP payload size */
> - pack_u32(p, 0); /* extended RCODE and flags */
> +
> + /* extended RCODE and flags */
> + pack_u16(p, 0);
> + pack_u16(p, dnssec_do ? DNS_MESSAGEEXTFLAG_DO : 0);
> +
> pack_u16(p, 0); /* RDATA len */
>
> return (p->err) ? (-1) : (0);
> Index: asr/res_mkquery.c
> ===
> RCS file: /d/cvs/src/lib/libc/asr/res_mkquery.c,v
> retrieving revision 1.10
> diff -u -p -r1.10 res_mkquery.c
> --- asr/res_mkquery.c 18 Feb 2017 19:23:05 - 1.10
> +++ asr/res_mkquery.c 27 Feb 2017 07:25:11 -
> @@ -61,14 +61,15 @@ res_mkquery(int op, const char *dname, i
> if (ac->ac_options & RES_RECURSE)
> h.flags |= RD_MASK;
> h.qdcount = 1;
> - if (ac->ac_options & RES_USE_EDNS0)
> + if (ac->ac_options & (RES_USE_EDNS0 | RES_USE_DNSSEC))
> h.arcount = 1;
>
> _asr_pack_init(, buf, buflen);
> _asr_pack_header(, );
> _asr_pack_query(, type, class, dn);
> - if (ac->ac_options & RES_USE_EDNS0)
> - _asr_pack_edns0(, MAXPACKETSZ);
> + if (ac->ac_options & (RES_USE_EDNS0 | RES_USE_DNSSEC))
> + _asr_pack_edns0(, MAXPACKETSZ,
> + ac->ac_options & RES_USE_DNSSEC);
>
> _asr_ctx_unref(ac);
>
> Index: asr/res_send_async.c
> ===
> RCS file: /d/cvs/src/lib/libc/asr/res_send_async.c,v
> retrieving revision 1.32
> diff -u -p -r1.32 res_send_async.c
> --- asr/res_send_async.c 18 Feb 2017 22:25:13 - 1.32
> +++ asr/res_send_async.c 27 Feb 2017 07:25:11 -
> @@ -377,14 +377,15 @@ setup_query(struct asr_query *as, const
> if (as->as_ctx->ac_options & RES_RECURSE)
> h.flags |= RD_MASK;
> h.qdcount = 1;
> - if (as->as_ctx->ac_options & RES_USE_EDNS0)
> + if (as->as_ctx->ac_options & (RES_USE_EDNS0 | RES_USE_DNSSEC))
> h.arcount = 1;
>
> _asr_pack_init(, as->as.dns.obuf, as->as.dns.obufsize);
> _asr_pack_header(, );
> _asr_pack_query(, type, class, dname);
> - if (as->as_ctx->ac_options & RES_USE_EDNS0)
> - _asr_pack_edns0(, MAXPACKETSZ);
> + if (as->as_ctx->ac_options & (RES_USE_EDNS0 | RES_USE_DNSSEC))
> + _asr_pack_edns0(, MAXPACKETSZ,
> + as->as_ctx->ac_options & RES_USE_DNSSEC);
> if (p.err) {
> DPRINT("error packing query");
> errno = EINVAL;
> Index: net/resolver.3
> ===
> RCS file: /d/cvs/src/lib/libc/net/resolver.3,v
> retrieving revision 1.36
> diff -u -p -r1.36 resolver.3
> --- net/resolver.318 Feb 2017 19:23:05 - 1.36
> +++ net/resolver.327 Feb 2017 07:25:11 -
> @@ -199,9 +199,6 @@ uses 4096 bytes as input buffer size.
> Request that the resolver uses
> Domain Name System Security Extensions (DNSSEC),
> as defined in RFCs 4033, 4034, and 4035.
> -On
> -.Ox
> -this option does nothing.
> .El
> .Pp
> The
>
>
> --
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
>
--
Gilles Chehade
https://www.poolp.org @poolpOrg
Re: asr: slightly better error reporting for getnameinfo()
On Mon, Feb 20, 2017 at 09:37:28PM +0100, Eric Faurot wrote:
> Report the errno set by getifaddrs(3) if the setup for AI_ADDRCONFIG fails,
> rather than a non-informative EAI_FAIL. Compare to -1 for error detection
> while here.
>
> Eric.
ok gilles@
> Index: asr/getaddrinfo_async.c
> ===
> RCS file: /cvs/src/lib/libc/asr/getaddrinfo_async.c,v
> retrieving revision 1.50
> diff -u -p -r1.50 getaddrinfo_async.c
> --- asr/getaddrinfo_async.c 16 Dec 2015 16:32:30 - 1.50
> +++ asr/getaddrinfo_async.c 20 Feb 2017 20:09:25 -
> @@ -191,8 +191,9 @@ getaddrinfo_async_run(struct asr_query *
>
> /* Restrict result set to configured address families */
> if (ai->ai_flags & AI_ADDRCONFIG) {
> - if (addrconfig_setup(as) != 0) {
> - ar->ar_gai_errno = EAI_FAIL;
> + if (addrconfig_setup(as) == -1) {
> + ar->ar_errno = errno;
> + ar->ar_gai_errno = EAI_SYSTEM;
> async_set_state(as, ASR_STATE_HALT);
> break;
> }
> @@ -679,7 +680,7 @@ addrconfig_setup(struct asr_query *as)
> struct sockaddr_in *sinp;
> struct sockaddr_in6 *sin6p;
>
> - if (getifaddrs() != 0)
> + if (getifaddrs() == -1)
> return (-1);
>
> as->as.ai.flags |= ASYNC_NO_INET | ASYNC_NO_INET6;
>
--
Gilles Chehade
https://www.poolp.org @poolpOrg
Re: opensmtpd faq commands
On Mon, Jan 30, 2017 at 07:00:18PM -0500, Daniel Jakots wrote:
> On Mon, 30 Jan 2017 18:38:12 -0500, Daniel Jakots <danj+o...@chown.me>
> wrote:
>
> > Hi,
> >
> > When following the example from the OpenSMTPD faq [0], first thing is
> > to create a new user with:
> >
> > useradd -g =uid -c "Virtual Mail" -d /var/vmail -s /sbin/nologin vmail
> >
> > which raises
> > warnx("Warning: home directory `%s' doesn't exist, and -m was"
> > " not specified", home);
> >
> > I don't think giving a command that produces a warning is a good thing
> > in an official documentation. Also solving this problem make other
> > commands useless so it's a bit shorter.
> >
> > [0]: https://opensmtpd.org/faq/example1.html
>
> While checking that my diff wasn't mangled, I noticed that it doesn't
> follow html syntax from OpenBSD faq, so let's be consistent:
>
ok
> Index: opensmtpd/faq/example1.html
> ===
> RCS file: /cvs/www/opensmtpd/faq/example1.html,v
> retrieving revision 1.12
> diff -u -p -r1.12 example1.html
> --- opensmtpd/faq/example1.html 31 Oct 2016 20:52:22 - 1.12
> +++ opensmtpd/faq/example1.html 30 Jan 2017 23:58:39 -
> @@ -90,9 +90,7 @@ virtual users.
> This user needs to be created:
>
>
> -# useradd -g =uid -c "Virtual Mail" -d /var/vmail -s /sbin/nologin
> vmail
> -# mkdir /var/vmail
> -# chown vmail:vmail /var/vmail
> +# useradd -m -g =uid -c "Virtual Mail" -d /var/vmail -s /sbin/nologin
> vmail
>
>
> Afterwards, the /etc/passwd file will contain an entry like
> @@ -199,9 +197,9 @@ maildir folder are mapped to the single
> In this example, Dovecot is used as an IMAP server.
>
>
> -# export
> PKG_PATH=http://your.local.mirror/pub/OpenBSD/%c/packages/%a
> -# pkg_add dovecot
> -# rcctl enable dovecot
> +# export PKG_PATH=http://your.local.mirror/pub/OpenBSD/%c/packages/%a
> +# pkg_add dovecot
> +# rcctl enable dovecot
>
>
> Virtual users access and read their mails via IMAP.
>
--
Gilles Chehade
https://www.poolp.org @poolpOrg
Re: userspace doesn't need to set sa_len, sun_len, etc
On Sun, Jan 22, 2017 at 07:07:28AM +1000, Philip Guenther wrote:
> On Sun, 22 Jan 2017, Martin Pieuchot wrote:
> > On 21/01/17(Sat) 20:40, Philip Guenther wrote:
> > >
> > > This is your periodic reminder that it's pointless for userspace to set
> > > the {sun,sin,sin6,etc}_len field in sockaddrs being passed to the POSIX
> > > APIs. An application can use it internally for its own purposes, but the
> > > kernel always overwrites the value when passed in.
> >
> > I'd say explicit is better than implicit. Is there any bug in the code
> > below?
> >
> > I believe it is simpler to always set the length, it makes our life easier
> > when review code between kernel and userland. Or should we have two
> > different practices because history screwed up?
>
> Let's something that has no effect, just for visual consistency? When the
> value there is ignored, what reason is there to believe the correct value
> is being used?
>
> Meanwhile, new code coming in from elsewhere is unlikely to set it, as
> that field doesn't exist on non-BSDs, so we either (a) add ignored
> assignments, or (b) be inconsistent inside userland. Oh, and -portable
> projects have to patch out use of it.
>
> I would rather have userland be consistent with itself and inconsistent
> with the kernel, than try for an incomplete consistency between userland
> and the kernel that spreads the inconsistency inside userland.
>
> Kernel and userland best practices are already quite different, including
> this in that list is the lesser evil.
>
I would love to remove this from smtpd to reduce diff with portable,
so i'm generally ok with the idea
--
Gilles Chehade
https://www.poolp.org @poolpOrg
Re: smtpd: hide struct io
t; + io_printf(s->tx->oev, " auth=%s", s->username[0] ?
> "yes" : "no");
> if (s->username[0])
> - io_printf(>tx->oev, " user=%s", s->username);
> + io_printf(s->tx->oev, " user=%s", s->username);
> }
> }
>
> if (s->tx->rcptcount == 1) {
> - io_printf(>tx->oev, "\n\tfor <%s@%s>",
> + io_printf(s->tx->oev, "\n\tfor <%s@%s>",
> s->tx->evp.rcpt.user,
> s->tx->evp.rcpt.domain);
> }
>
> - io_printf(>tx->oev, ";\n\t%s\n", time_to_text(time(NULL)));
> + io_printf(s->tx->oev, ";\n\t%s\n", time_to_text(time(NULL)));
>
> - s->tx->odatalen = io_queued(>tx->oev);
> + s->tx->odatalen = io_queued(s->tx->oev);
>
> - io_set_write(>tx->oev);
> + io_set_write(s->tx->oev);
>
> smtp_enter_state(s, STATE_BODY);
> smtp_reply(s, "354 Enter mail, end with \".\""
> @@ -1257,13 +1249,13 @@ smtp_io(struct io *io, int evt, void *ar
>
> case IO_TLSREADY:
> log_info("%016"PRIx64" smtp event=starttls address=%s host=%s
> ciphers=\"%s\"",
> - s->id, ss_to_text(>ss), s->hostname,
> ssl_to_text(io_ssl(>io)));
> + s->id, ss_to_text(>ss), s->hostname,
> ssl_to_text(io_ssl(s->io)));
>
> s->flags |= SF_SECURE;
> s->helo[0] = '\0';
>
> if (smtp_verify_certificate(s)) {
> - io_pause(>io, IO_PAUSE_IN);
> + io_pause(s->io, IO_PAUSE_IN);
> break;
> }
>
> @@ -1280,8 +1272,8 @@ smtp_io(struct io *io, int evt, void *ar
>
> case IO_DATAIN:
> nextline:
> - line = io_getline(>io, );
> - if ((line == NULL && io_datalen(>io) >= LINE_MAX) ||
> + line = io_getline(s->io, );
> + if ((line == NULL && io_datalen(s->io) >= LINE_MAX) ||
> (line && len >= LINE_MAX)) {
> s->flags |= SF_BADINPUT;
> smtp_reply(s, "500 %s: Line too long",
> @@ -1302,7 +1294,7 @@ smtp_io(struct io *io, int evt, void *ar
> }
>
> /* Pipelining not supported */
> - if (io_datalen(>io)) {
> + if (io_datalen(s->io)) {
> s->flags |= SF_BADINPUT;
> smtp_reply(s, "500 %s %s: Pipelining not supported",
> esc_code(ESC_STATUS_PERMFAIL, ESC_INVALID_COMMAND),
> @@ -1321,7 +1313,7 @@ smtp_io(struct io *io, int evt, void *ar
> io_set_write(io);
>
> s->tx->dataeom = 1;
> - if (io_queued(>tx->oev) == 0)
> + if (io_queued(s->tx->oev) == 0)
> smtp_data_io_done(s);
> return;
> }
> @@ -1400,7 +1392,6 @@ smtp_tx(struct smtp_session *s)
> return 0;
>
> TAILQ_INIT(>rcpts);
> - io_init(>oev, NULL);
>
> s->tx = tx;
> tx->session = s;
> @@ -1454,6 +1445,9 @@ smtp_tx_free(struct smtp_tx *tx)
> free(rcpt);
> }
>
> + if (tx->oev)
> + io_free(tx->oev);
> +
> tx->session->tx = NULL;
>
> free(tx);
> @@ -1472,21 +1466,21 @@ smtp_data_io(struct io *io, int evt, voi
> case IO_DISCONNECTED:
> case IO_ERROR:
> log_debug("debug: smtp: %p: io error on mfa", s);
> - io_clear(>tx->oev);
> - iobuf_clear(>tx->obuf);
> + io_free(s->tx->oev);
> + s->tx->oev = NULL;
> s->tx->msgflags |= MF_ERROR_IO;
> - if (io_paused(>io, IO_PAUSE_IN)) {
> + if (io_paused(s->io, IO_PAUSE_IN)) {
> log_debug("debug: smtp: %p: resuming session after mfa
> error", s);
> - io_resume(>io, IO_PAUSE_IN);
> + io_resume(s->io, IO_PAUSE_IN);
> }
> break;
>
> case IO_LOWAT:
> - if (s->tx->dataeom && io_queued(>tx->oev) == 0) {
> + if (s->tx->dataeom && io_queued(s->tx->oev) == 0) {
> smtp_data_io_done(s);
> - } else if (io_paused(>io, IO_PAUSE_IN)) {
> + } else if (io_paused(s->io, IO_PAUSE_IN)) {
> log_debug("debug: smtp: %p: filter congestion over:
> resuming session", s);
> - io_resume(>io, IO_PAUSE_IN);
> + io_resume(s->io, IO_PAUSE_IN);
> }
> break;
>
> @@ -1499,8 +1493,11 @@ static void
> smtp_data_io_done(struct smtp_session *s)
> {
> log_debug("debug: smtp: %p: data io done (%zu bytes)", s,
> s->tx->odatalen);
> - io_clear(>tx->oev);
> - iobuf_clear(>tx->obuf);
> +
> + if (s->tx->oev) {
> + io_free(s->tx->oev);
> + s->tx->oev = NULL;
> + }
>
> if (s->tx->msgflags & MF_ERROR) {
>
> @@ -2075,7 +2072,7 @@ smtp_lookup_servername(struct smtp_sessi
> if (s->listener->hostnametable[0]) {
> sa_len = sizeof(ss);
> sa = (struct sockaddr *)
> - if (getsockname(io_fileno(>io), sa, _len) == -1) {
> + if (getsockname(io_fileno(s->io), sa, _len) == -1) {
> log_warn("warn: getsockname()");
> }
> else {
> @@ -2103,7 +2100,7 @@ smtp_connected(struct smtp_session *s)
> s->id, ss_to_text(>ss), s->hostname);
>
> sl = sizeof(ss);
> - if (getsockname(io_fileno(>io), (struct sockaddr*), ) == -1) {
> + if (getsockname(io_fileno(s->io), (struct sockaddr*), ) == -1) {
> smtp_free(s, strerror(errno));
> return;
> }
> @@ -2162,7 +2159,7 @@ smtp_message_printf(struct smtp_session
> return -1;
>
> va_start(ap, fmt);
> - len = io_vprintf(>tx->oev, fmt, ap);
> + len = io_vprintf(s->tx->oev, fmt, ap);
> va_end(ap);
>
> if (len < 0) {
> @@ -2192,7 +2189,7 @@ smtp_reply(struct smtp_session *s, char
>
> log_trace(TRACE_SMTP, "smtp: %p: >>> %s", s, buf);
>
> - io_xprintf(>io, "%s\r\n", buf);
> + io_xprintf(s->io, "%s\r\n", buf);
>
> switch (buf[0]) {
> case '5':
> @@ -2239,11 +2236,8 @@ smtp_free(struct smtp_session *s, const
> tree_pop(_filter_data, s->id);
>
> if (s->tx) {
> - if (s->tx->msgid) {
> + if (s->tx->msgid)
> smtp_queue_rollback(s);
> - io_clear(>tx->oev);
> - iobuf_clear(>tx->obuf);
> - }
> smtp_filter_tx_rollback(s);
> smtp_tx_free(s->tx);
> }
> @@ -2256,8 +2250,7 @@ smtp_free(struct smtp_session *s, const
> if (s->flags & SF_SECURE && s->listener->flags & F_STARTTLS)
> stat_decrement("smtp.tls", 1);
>
> - io_clear(>io);
> - iobuf_clear(>iobuf);
> + io_free(s->io);
> free(s);
>
> smtp_collect();
> @@ -2346,10 +2339,10 @@ smtp_verify_certificate(struct smtp_sess
> >= sizeof req_ca_vrfy.name)
> return 0;
>
> - x = SSL_get_peer_certificate(io_ssl(>io));
> + x = SSL_get_peer_certificate(io_ssl(s->io));
> if (x == NULL)
> return 0;
> - xchain = SSL_get_peer_cert_chain(io_ssl(>io));
> + xchain = SSL_get_peer_cert_chain(io_ssl(s->io));
>
> /*
>* Client provided a certificate and possibly a certificate chain.
> @@ -2635,9 +2628,9 @@ smtp_filter_dataline(struct smtp_session
> return;
> }
>
> - if (io_queued(>tx->oev) > DATA_HIWAT && !io_paused(>io,
> IO_PAUSE_IN)) {
> + if (io_queued(s->tx->oev) > DATA_HIWAT && !io_paused(s->io,
> IO_PAUSE_IN)) {
> log_debug("debug: smtp: %p: filter congestion: pausing
> session", s);
> - io_pause(>io, IO_PAUSE_IN);
> + io_pause(s->io, IO_PAUSE_IN);
> }
> }
>
> Index: smtpd.h
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.h,v
> retrieving revision 1.525
> diff -u -p -r1.525 smtpd.h
> --- smtpd.h 25 Nov 2016 09:21:21 - 1.525
> +++ smtpd.h 28 Nov 2016 12:51:29 -
> @@ -22,6 +22,8 @@
> #define nitems(_a) (sizeof((_a)) / sizeof((_a)[0]))
> #endif
>
> +#include
> +
> #include "smtpd-defines.h"
> #include "smtpd-api.h"
> #include "ioev.h"
>
--
Gilles Chehade
https://www.poolp.org @poolpOrg
Re: smtpd: more internal cleanups
tp_reply(s, "%d %s", code, line);
> > smtp_enter_state(s, STATE_HELO);
> > - io_reload(>io);
> > return;
> > }
> > smtp_message_end(s);
> > @@ -1198,7 +1182,6 @@ smtp_filter_fd(uint64_t id, int fd)
> > smtp_reply(s, "421 %s: Temporary Error",
> > esc_code(ESC_STATUS_TEMPFAIL,
> > ESC_OTHER_MAIL_SYSTEM_STATUS));
> > smtp_enter_state(s, STATE_QUIT);
> > - io_reload(>io);
> > return;
> > }
> >
> > @@ -1257,7 +1240,6 @@ smtp_filter_fd(uint64_t id, int fd)
> > " on a line by itself");
> >
> > tree_xset(_filter_data, s->id, s);
> > - io_reload(>io);
> > }
> >
> > static void
> > @@ -1341,8 +1323,6 @@ smtp_io(struct io *io, int evt, void *ar
> > s->tx->dataeom = 1;
> > if (io_queued(>tx->oev) == 0)
> > smtp_data_io_done(s);
> > - else
> > - io_reload(>tx->oev);
> > return;
> > }
> >
> > @@ -1547,7 +1527,6 @@ smtp_data_io_done(struct smtp_session *s
> > smtp_reply(s, "421 Internal server error");
> > smtp_tx_free(s->tx);
> > smtp_enter_state(s, STATE_HELO);
> > - io_reload(>io);
> > }
> > else {
> > smtp_filter_eom(s);
> > @@ -2137,7 +2116,6 @@ static void
> > smtp_send_banner(struct smtp_session *s)
> > {
> > smtp_reply(s, "220 %s ESMTP %s", s->smtpname, SMTPD_NAME);
> > - io_reload(>io);
> > }
> >
> > void
> > @@ -2466,7 +2444,6 @@ smtp_auth_failure_resume(int fd, short e
> >
> > smtp_reply(s, "535 Authentication failed");
> > smtp_enter_state(s, STATE_HELO);
> > - io_reload(>io);
> > }
> >
> > static void
> > @@ -2662,7 +2639,6 @@ smtp_filter_dataline(struct smtp_session
> > log_debug("debug: smtp: %p: filter congestion over: pausing
> > session", s);
> > io_pause(>io, IO_PAUSE_IN);
> > }
> > - io_reload(>tx->oev);
> > }
> >
> > #define CASE(x) case x : return #x
>
--
Gilles Chehade
https://www.poolp.org @poolpOrg
