Re: smtpd: use libtls signer
On Sun, Jan 30, 2022 at 10:55:40AM +0100, Eric Faurot wrote: > Hi. > > This diff makes use of the new libtls signer api to simplify tls privsep. Updated diff after libtls signer api tweak by jsing@ Eric. Index: ca.c === RCS file: /cvs/src/usr.sbin/smtpd/ca.c,v retrieving revision 1.40 diff -u -p -r1.40 ca.c --- ca.c14 Jun 2021 17:58:15 - 1.40 +++ ca.c12 Feb 2022 12:49:04 - @@ -1,6 +1,7 @@ /* $OpenBSD: ca.c,v 1.40 2021/06/14 17:58:15 eric Exp $*/ /* + * Copyright (c) 2021 Eric Faurot * Copyright (c) 2014 Reyk Floeter * Copyright (c) 2012 Gilles Chehade * @@ -17,45 +18,23 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include +#include #include #include +#include #include +#include #include #include "smtpd.h" -#include "log.h" #include "ssl.h" +#include "log.h" -static int ca_verify_cb(int, X509_STORE_CTX *); - -static int rsae_send_imsg(int, const unsigned char *, unsigned char *, - RSA *, int, unsigned int); -static int rsae_pub_enc(int, const unsigned char *, unsigned char *, - RSA *, int); -static int rsae_pub_dec(int,const unsigned char *, unsigned char *, - RSA *, int); -static int rsae_priv_enc(int, const unsigned char *, unsigned char *, - RSA *, int); -static int rsae_priv_dec(int, const unsigned char *, unsigned char *, - RSA *, int); -static int rsae_mod_exp(BIGNUM *, const BIGNUM *, RSA *, BN_CTX *); -static int rsae_bn_mod_exp(BIGNUM *, const BIGNUM *, const BIGNUM *, - const BIGNUM *, BN_CTX *, BN_MONT_CTX *); -static int rsae_init(RSA *); -static int rsae_finish(RSA *); -static int rsae_keygen(RSA *, int, BIGNUM *, BN_GENCB *); - -static ECDSA_SIG *ecdsae_do_sign(const unsigned char *, int, const BIGNUM *, -const BIGNUM *, EC_KEY *); -static int ecdsae_sign_setup(EC_KEY *, BN_CTX *, BIGNUM **, BIGNUM **); -static int ecdsae_do_verify(const unsigned char *, int, const ECDSA_SIG *, -EC_KEY *); - +static void ca_imsg(struct mproc *, struct imsg *); +static void ca_init(void); -static struct dict pkeys; -static uint64_t reqid = 0; +static struct tls_signer *signer; +static uint64_t reqid = 0; static void ca_shutdown(void) @@ -69,7 +48,9 @@ ca(void) { struct passwd *pw; - purge_config(PURGE_LISTENERS|PURGE_TABLES|PURGE_RULES|PURGE_DISPATCHERS); + ca_init(); + + purge_config(PURGE_EVERYTHING); if ((pw = getpwnam(SMTPD_USER)) == NULL) fatalx("unknown user " SMTPD_USER); @@ -98,9 +79,6 @@ ca(void) config_peer(PROC_PARENT); config_peer(PROC_DISPATCHER); - /* Ignore them until we get our config */ - mproc_disable(p_dispatcher); - if (pledge("stdio", NULL) == -1) fatal("pledge"); @@ -110,120 +88,35 @@ ca(void) return (0); } -void +static void ca_init(void) { - BIO *in = NULL; - EVP_PKEY*pkey = NULL; - struct pki *pki; - const char *k; - void*iter_dict; - char*hash; + struct pki *pki; + void *iter_dict; + + if ((signer = tls_signer_new()) == NULL) + fatal("tls_signer_new"); - log_debug("debug: init private ssl-tree"); - dict_init(); iter_dict = NULL; - while (dict_iter(env->sc_pki_dict, _dict, , (void **))) { + while (dict_iter(env->sc_pki_dict, _dict, NULL, (void **))) { if (pki->pki_key == NULL) continue; - - 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); - - 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); + if (tls_signer_add_keypair_mem(signer, pki->pki_cert, + pki->pki_cert_len, pki->pki_key, pki->pki_key_len) == -1) + fatalx("ca_init: tls_signer_add_keypair_mem"); } } -static int -ca_verify_cb(int ok, X509_STORE_CTX *ctx) -{ - switch (X509_STORE_CTX_get_error(ctx)) { - case X509_V_OK: - break; -case X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT: - break; -case X509_
smtpd: use libtls signer
Hi. This diff makes use of the new libtls signer api to simplify tls privsep. Eric. Index: ca.c === RCS file: /cvs/src/usr.sbin/smtpd/ca.c,v retrieving revision 1.40 diff -u -p -r1.40 ca.c --- ca.c14 Jun 2021 17:58:15 - 1.40 +++ ca.c26 Jan 2022 14:01:15 - @@ -1,6 +1,7 @@ /* $OpenBSD: ca.c,v 1.40 2021/06/14 17:58:15 eric Exp $*/ /* + * Copyright (c) 2021 Eric Faurot * Copyright (c) 2014 Reyk Floeter * Copyright (c) 2012 Gilles Chehade * @@ -17,45 +18,23 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include +#include #include #include +#include #include +#include #include #include "smtpd.h" -#include "log.h" #include "ssl.h" +#include "log.h" -static int ca_verify_cb(int, X509_STORE_CTX *); - -static int rsae_send_imsg(int, const unsigned char *, unsigned char *, - RSA *, int, unsigned int); -static int rsae_pub_enc(int, const unsigned char *, unsigned char *, - RSA *, int); -static int rsae_pub_dec(int,const unsigned char *, unsigned char *, - RSA *, int); -static int rsae_priv_enc(int, const unsigned char *, unsigned char *, - RSA *, int); -static int rsae_priv_dec(int, const unsigned char *, unsigned char *, - RSA *, int); -static int rsae_mod_exp(BIGNUM *, const BIGNUM *, RSA *, BN_CTX *); -static int rsae_bn_mod_exp(BIGNUM *, const BIGNUM *, const BIGNUM *, - const BIGNUM *, BN_CTX *, BN_MONT_CTX *); -static int rsae_init(RSA *); -static int rsae_finish(RSA *); -static int rsae_keygen(RSA *, int, BIGNUM *, BN_GENCB *); - -static ECDSA_SIG *ecdsae_do_sign(const unsigned char *, int, const BIGNUM *, -const BIGNUM *, EC_KEY *); -static int ecdsae_sign_setup(EC_KEY *, BN_CTX *, BIGNUM **, BIGNUM **); -static int ecdsae_do_verify(const unsigned char *, int, const ECDSA_SIG *, -EC_KEY *); - +static void ca_imsg(struct mproc *, struct imsg *); +static void ca_init(void); -static struct dict pkeys; -static uint64_t reqid = 0; +static struct tls_signer *signer; +static uint64_t reqid = 0; static void ca_shutdown(void) @@ -69,7 +48,9 @@ ca(void) { struct passwd *pw; - purge_config(PURGE_LISTENERS|PURGE_TABLES|PURGE_RULES|PURGE_DISPATCHERS); + ca_init(); + + purge_config(PURGE_EVERYTHING); if ((pw = getpwnam(SMTPD_USER)) == NULL) fatalx("unknown user " SMTPD_USER); @@ -98,9 +79,6 @@ ca(void) config_peer(PROC_PARENT); config_peer(PROC_DISPATCHER); - /* Ignore them until we get our config */ - mproc_disable(p_dispatcher); - if (pledge("stdio", NULL) == -1) fatal("pledge"); @@ -110,120 +88,35 @@ ca(void) return (0); } -void +static void ca_init(void) { - BIO *in = NULL; - EVP_PKEY*pkey = NULL; - struct pki *pki; - const char *k; - void*iter_dict; - char*hash; + struct pki *pki; + void *iter_dict; + + if ((signer = tls_signer_new()) == NULL) + fatal("tls_signer_new"); - log_debug("debug: init private ssl-tree"); - dict_init(); iter_dict = NULL; - while (dict_iter(env->sc_pki_dict, _dict, , (void **))) { + while (dict_iter(env->sc_pki_dict, _dict, NULL, (void **))) { if (pki->pki_key == NULL) continue; - - 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); - - 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); + if (tls_signer_add_keypair_mem(signer, pki->pki_cert, + pki->pki_cert_len, pki->pki_key, pki->pki_key_len) == -1) + fatalx("ca_init: tls_signer_add_keypair_mem"); } } -static int -ca_verify_cb(int ok, X509_STORE_CTX *ctx) -{ - switch (X509_STORE_CTX_get_error(ctx)) { - case X509_V_OK: - break; -case X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT: - break; -case X509_V_ERR_CERT_NOT_YET_VALID: -case X509_V_ERR_ERROR_IN_CERT_NOT_BEFORE_FIELD: - break; -cas
smtpd: srs and ruleset evaluation
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()); } }
smtpd: unnecessary "no certificate presented" log message
Except for specific cases, SMTP servers do not expect client certificates for TLS sessions. The log message for missing certificate is not very useful in practice (handshake fails before if it was required anyway), and it is even confusing for people. I think it can go away. Eric. Index: smtp_session.c === RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v retrieving revision 1.431 diff -u -p -r1.431 smtp_session.c --- smtp_session.c 14 Jun 2021 17:58:16 - 1.431 +++ smtp_session.c 30 Jun 2021 08:09:29 - @@ -1070,11 +1070,6 @@ smtp_tls_started(struct smtp_session *s) (s->flags & SF_VERIFIED) ? "verified" : "unchecked", tls_peer_cert_hash(io_tls(s->io))); } - else { - log_info("%016"PRIx64" smtp " - "cert-check result=\"no certificate presented\"", - s->id); - } if (s->listener->flags & F_SMTPS) { stat_increment("smtp.smtps", 1);
Re: add table_procexec in smtpd
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. 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. 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? Eric.
Re: smtpd: includes cleanup
Hi. Slightly updated diff, including sys/tree.h in smtpd.h. Eric. Index: aliases.c === RCS file: /cvs/src/usr.sbin/smtpd/aliases.c,v retrieving revision 1.78 diff -u -p -r1.78 aliases.c --- aliases.c 28 Apr 2020 21:46:43 - 1.78 +++ aliases.c 26 May 2021 20:15:02 - @@ -16,19 +16,8 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include - -#include -#include -#include -#include -#include #include #include -#include #include #include "smtpd.h" Index: bounce.c === RCS file: /cvs/src/usr.sbin/smtpd/bounce.c,v retrieving revision 1.84 diff -u -p -r1.84 bounce.c --- bounce.c26 May 2021 18:08:55 - 1.84 +++ bounce.c26 May 2021 20:14:41 - @@ -18,23 +18,11 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include - #include -#include -#include #include -#include -#include -#include #include #include -#include #include -#include #include "smtpd.h" #include "log.h" Index: ca.c === RCS file: /cvs/src/usr.sbin/smtpd/ca.c,v retrieving revision 1.39 diff -u -p -r1.39 ca.c --- ca.c26 May 2021 18:08:55 - 1.39 +++ ca.c27 May 2021 10:57:24 - @@ -17,26 +17,12 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include - -#include -#include +#include +#include #include #include -#include #include #include - -#include -#include -#include -#include -#include -#include -#include #include "smtpd.h" #include "log.h" Index: compress_backend.c === RCS file: /cvs/src/usr.sbin/smtpd/compress_backend.c,v retrieving revision 1.9 diff -u -p -r1.9 compress_backend.c --- compress_backend.c 20 Jan 2015 17:37:54 - 1.9 +++ compress_backend.c 26 May 2021 20:13:39 - @@ -17,18 +17,7 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include -#include - -#include -#include -#include #include -#include -#include #include "smtpd.h" Index: compress_gzip.c === RCS file: /cvs/src/usr.sbin/smtpd/compress_gzip.c,v retrieving revision 1.12 diff -u -p -r1.12 compress_gzip.c --- compress_gzip.c 26 May 2021 18:08:55 - 1.12 +++ compress_gzip.c 27 May 2021 11:01:41 - @@ -17,27 +17,10 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include -#include - -#include -#include -#include -#include -#include #include -#include -#include -#include - #include #include "smtpd.h" -#include "log.h" - #defineGZIP_BUFFER_SIZE16384 Index: config.c === RCS file: /cvs/src/usr.sbin/smtpd/config.c,v retrieving revision 1.56 diff -u -p -r1.56 config.c --- config.c26 May 2021 07:05:50 - 1.56 +++ config.c26 May 2021 20:13:14 - @@ -16,23 +16,11 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include #include -#include #include -#include -#include -#include #include -#include #include -#include - -#include #include "smtpd.h" #include "log.h" Index: control.c === RCS file: /cvs/src/usr.sbin/smtpd/control.c,v retrieving revision 1.127 diff -u -p -r1.127 control.c --- control.c 26 May 2021 18:08:55 - 1.127 +++ control.c 26 May 2021 20:12:43 - @@ -18,25 +18,15 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include #include -#include #include #include -#include -#include -#include #include #include -#include #include #include -#include #include -#include #include "smtpd.h" #include "log.h" Index: crypto.c === RCS file: /cvs/src/usr.sbin/smtpd/crypto.c,v retrieving revision 1.9 diff -u -p -r1.9 crypto.c --- crypto.c23 Jan 2021 16:11:11 - 1.9 +++ crypto.c27 May 2021 11:01:30 - @@ -16,14 +16,10 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include #include -#include -#include - #include - +#include #defineCRYPTO_BUFFER_SIZE 16384 Index: dict.c === RCS file: /cvs/src/usr.sbin/smtpd/dict.c,v retrieving revision 1.7 diff -u -p -r1.7 dict.c --- dict.c 26 May 2021 18:08:55 - 1.7 +++ dict.c 26 May 2021 20:10:54
Re: smtpd: includes cleanup
On Thu, May 27, 2021 at 08:13:36AM -0600, Todd C. Miller wrote: > On Thu, 27 May 2021 13:14:30 +0200, Eric Faurot wrote: > > > New diff with small tweaks. > > It looks like you are relying on sys/queue.h being included implicitly. > Since smtpd.h uses the TAILQ macros, should it include sys/queue.h > itself? That's reasonnable. I'm adding it. Eric.
Re: smtpd: includes cleanup
New diff with small tweaks. Eric. Index: aliases.c === RCS file: /cvs/src/usr.sbin/smtpd/aliases.c,v retrieving revision 1.78 diff -u -p -r1.78 aliases.c --- aliases.c 28 Apr 2020 21:46:43 - 1.78 +++ aliases.c 26 May 2021 20:15:02 - @@ -16,19 +16,8 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include - -#include -#include -#include -#include -#include #include #include -#include #include #include "smtpd.h" Index: bounce.c === RCS file: /cvs/src/usr.sbin/smtpd/bounce.c,v retrieving revision 1.84 diff -u -p -r1.84 bounce.c --- bounce.c26 May 2021 18:08:55 - 1.84 +++ bounce.c26 May 2021 20:14:41 - @@ -18,23 +18,11 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include - #include -#include -#include #include -#include -#include -#include #include #include -#include #include -#include #include "smtpd.h" #include "log.h" Index: ca.c === RCS file: /cvs/src/usr.sbin/smtpd/ca.c,v retrieving revision 1.39 diff -u -p -r1.39 ca.c --- ca.c26 May 2021 18:08:55 - 1.39 +++ ca.c27 May 2021 10:57:24 - @@ -17,26 +17,12 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include - -#include -#include +#include +#include #include #include -#include #include #include - -#include -#include -#include -#include -#include -#include -#include #include "smtpd.h" #include "log.h" Index: compress_backend.c === RCS file: /cvs/src/usr.sbin/smtpd/compress_backend.c,v retrieving revision 1.9 diff -u -p -r1.9 compress_backend.c --- compress_backend.c 20 Jan 2015 17:37:54 - 1.9 +++ compress_backend.c 26 May 2021 20:13:39 - @@ -17,18 +17,7 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include -#include - -#include -#include -#include #include -#include -#include #include "smtpd.h" Index: compress_gzip.c === RCS file: /cvs/src/usr.sbin/smtpd/compress_gzip.c,v retrieving revision 1.12 diff -u -p -r1.12 compress_gzip.c --- compress_gzip.c 26 May 2021 18:08:55 - 1.12 +++ compress_gzip.c 27 May 2021 11:01:41 - @@ -17,27 +17,10 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include -#include - -#include -#include -#include -#include -#include #include -#include -#include -#include - #include #include "smtpd.h" -#include "log.h" - #defineGZIP_BUFFER_SIZE16384 Index: config.c === RCS file: /cvs/src/usr.sbin/smtpd/config.c,v retrieving revision 1.56 diff -u -p -r1.56 config.c --- config.c26 May 2021 07:05:50 - 1.56 +++ config.c26 May 2021 20:13:14 - @@ -16,23 +16,11 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include #include -#include #include -#include -#include -#include #include -#include #include -#include - -#include #include "smtpd.h" #include "log.h" Index: control.c === RCS file: /cvs/src/usr.sbin/smtpd/control.c,v retrieving revision 1.127 diff -u -p -r1.127 control.c --- control.c 26 May 2021 18:08:55 - 1.127 +++ control.c 26 May 2021 20:12:43 - @@ -18,25 +18,15 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include #include -#include #include #include -#include -#include -#include #include #include -#include #include #include -#include #include -#include #include "smtpd.h" #include "log.h" Index: crypto.c === RCS file: /cvs/src/usr.sbin/smtpd/crypto.c,v retrieving revision 1.9 diff -u -p -r1.9 crypto.c --- crypto.c23 Jan 2021 16:11:11 - 1.9 +++ crypto.c27 May 2021 11:01:30 - @@ -16,14 +16,10 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include #include -#include -#include - #include - +#include #defineCRYPTO_BUFFER_SIZE 16384 Index: dict.c === RCS file: /cvs/src/usr.sbin/smtpd/dict.c,v retrieving revision 1.7 diff -u -p -r1.7 dict.c --- dict.c 26 May 2021 18:08:55 - 1.7 +++ dict.c 26 May 2021 20:10:54 - @@ -17,12 +17,10 @@ * OR
smtpd: includes cleanup
Cruft has accumulated on that front. This diff cleans it up: the few headers that are directly required for smtpd.h are included there, and unnecessary includes are removed from the rest of the files. Eric. Index: aliases.c === RCS file: /cvs/src/usr.sbin/smtpd/aliases.c,v retrieving revision 1.78 diff -u -p -r1.78 aliases.c --- aliases.c 28 Apr 2020 21:46:43 - 1.78 +++ aliases.c 26 May 2021 20:15:02 - @@ -16,19 +16,8 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include - -#include -#include -#include -#include -#include #include #include -#include #include #include "smtpd.h" Index: bounce.c === RCS file: /cvs/src/usr.sbin/smtpd/bounce.c,v retrieving revision 1.84 diff -u -p -r1.84 bounce.c --- bounce.c26 May 2021 18:08:55 - 1.84 +++ bounce.c26 May 2021 20:14:41 - @@ -18,23 +18,11 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include - #include -#include -#include #include -#include -#include -#include #include #include -#include #include -#include #include "smtpd.h" #include "log.h" Index: ca.c === RCS file: /cvs/src/usr.sbin/smtpd/ca.c,v retrieving revision 1.39 diff -u -p -r1.39 ca.c --- ca.c26 May 2021 18:08:55 - 1.39 +++ ca.c26 May 2021 20:14:08 - @@ -17,17 +17,8 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include - -#include -#include #include #include -#include -#include #include #include Index: compress_backend.c === RCS file: /cvs/src/usr.sbin/smtpd/compress_backend.c,v retrieving revision 1.9 diff -u -p -r1.9 compress_backend.c --- compress_backend.c 20 Jan 2015 17:37:54 - 1.9 +++ compress_backend.c 26 May 2021 20:13:39 - @@ -17,18 +17,7 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include -#include - -#include -#include -#include #include -#include -#include #include "smtpd.h" Index: compress_gzip.c === RCS file: /cvs/src/usr.sbin/smtpd/compress_gzip.c,v retrieving revision 1.12 diff -u -p -r1.12 compress_gzip.c --- compress_gzip.c 26 May 2021 18:08:55 - 1.12 +++ compress_gzip.c 26 May 2021 20:13:30 - @@ -17,22 +17,7 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include -#include - -#include -#include -#include -#include -#include #include -#include -#include -#include - #include #include "smtpd.h" Index: config.c === RCS file: /cvs/src/usr.sbin/smtpd/config.c,v retrieving revision 1.56 diff -u -p -r1.56 config.c --- config.c26 May 2021 07:05:50 - 1.56 +++ config.c26 May 2021 20:13:14 - @@ -16,23 +16,11 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include #include -#include #include -#include -#include -#include #include -#include #include -#include - -#include #include "smtpd.h" #include "log.h" Index: control.c === RCS file: /cvs/src/usr.sbin/smtpd/control.c,v retrieving revision 1.127 diff -u -p -r1.127 control.c --- control.c 26 May 2021 18:08:55 - 1.127 +++ control.c 26 May 2021 20:12:43 - @@ -18,25 +18,15 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include #include -#include #include #include -#include -#include -#include #include #include -#include #include #include -#include #include -#include #include "smtpd.h" #include "log.h" Index: crypto.c === RCS file: /cvs/src/usr.sbin/smtpd/crypto.c,v retrieving revision 1.9 diff -u -p -r1.9 crypto.c --- crypto.c23 Jan 2021 16:11:11 - 1.9 +++ crypto.c26 May 2021 20:11:28 - @@ -16,10 +16,8 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include #include -#include #include #include Index: dict.c === RCS file: /cvs/src/usr.sbin/smtpd/dict.c,v retrieving revision 1.7 diff -u -p -r1.7 dict.c --- dict.c 26 May 2021 18:08:55 - 1.7 +++ dict.c 26 May 2021 20:10:54 - @@ -17,12 +17,10 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include #include
smtpd: err/errx -> fatal/fatalx
This diff replaces calls to err(3)/errx(3) with fatal()/fatalx() from log.c for code that runs in the deamon (we want errors logged to syslog, not stderr). It's pretty mechanical, with two things to note: The call to default_config() has to be done after log_init() in smtpd.c, and log_init() is called early in smtpctl.c, since it uses files (iobuf.c) that uses the log api. It still logs to stderr though. Eric. Index: bounce.c === RCS file: /cvs/src/usr.sbin/smtpd/bounce.c,v retrieving revision 1.83 diff -u -p -r1.83 bounce.c --- bounce.c31 Dec 2020 08:27:15 - 1.83 +++ bounce.c26 May 2021 11:14:04 - @@ -23,7 +23,6 @@ #include #include -#include #include #include #include @@ -141,7 +140,7 @@ bounce_add(uint64_t evpid) } if (evp.type != D_BOUNCE) - errx(1, "bounce: evp:%016" PRIx64 " is not of type D_BOUNCE!", + fatalx("bounce: evp:%016" PRIx64 " is not of type D_BOUNCE!", evp.id); key.msgid = evpid_to_msgid(evpid); Index: ca.c === RCS file: /cvs/src/usr.sbin/smtpd/ca.c,v retrieving revision 1.38 diff -u -p -r1.38 ca.c --- ca.c5 Mar 2021 12:37:32 - 1.38 +++ ca.c26 May 2021 11:17:24 - @@ -22,7 +22,6 @@ #include #include -#include #include #include #include @@ -117,7 +116,7 @@ ca(void) mproc_disable(p_dispatcher); if (pledge("stdio", NULL) == -1) - err(1, "pledge"); + fatal("pledge"); event_dispatch(); fatalx("exited event loop"); @@ -333,7 +332,7 @@ ca_imsg(struct mproc *p, struct imsg *im return; } - errx(1, "ca_imsg: unexpected %s imsg", imsg_to_str(imsg->hdr.type)); + fatalx("ca_imsg: unexpected %s imsg", imsg_to_str(imsg->hdr.type)); } /* Index: compress_gzip.c === RCS file: /cvs/src/usr.sbin/smtpd/compress_gzip.c,v retrieving revision 1.11 diff -u -p -r1.11 compress_gzip.c --- compress_gzip.c 23 Jan 2021 16:11:11 - 1.11 +++ compress_gzip.c 26 May 2021 11:14:58 - @@ -24,7 +24,6 @@ #include #include -#include #include #include #include Index: control.c === RCS file: /cvs/src/usr.sbin/smtpd/control.c,v retrieving revision 1.126 diff -u -p -r1.126 control.c --- control.c 31 Dec 2020 08:27:15 - 1.126 +++ control.c 26 May 2021 11:15:39 - @@ -25,7 +25,6 @@ #include #include -#include #include #include #include @@ -159,7 +158,7 @@ control_imsg(struct mproc *p, struct ims return; } - errx(1, "control_imsg: unexpected %s imsg", + fatalx("control_imsg: unexpected %s imsg", imsg_to_str(imsg->hdr.type)); } @@ -254,7 +253,7 @@ control(void) control_listen(); if (pledge("stdio unix recvfd sendfd", NULL) == -1) - err(1, "pledge"); + fatal("pledge"); event_dispatch(); fatalx("exited event loop"); Index: dict.c === RCS file: /cvs/src/usr.sbin/smtpd/dict.c,v retrieving revision 1.6 diff -u -p -r1.6 dict.c --- dict.c 23 Dec 2018 16:06:24 - 1.6 +++ dict.c 26 May 2021 11:16:31 - @@ -20,12 +20,12 @@ #include #include -#include #include #include #include #include "dict.h" +#include "log.h" struct dictentry { SPLAY_ENTRY(dictentry) entry; @@ -72,7 +72,7 @@ dict_set(struct dict *d, const char *k, key.key = k; if ((entry = SPLAY_FIND(_dict, >dict, )) == NULL) { if ((entry = dict_alloc(k, data)) == NULL) - err(1, "dict_set: malloc"); + fatal("dict_set: malloc"); SPLAY_INSERT(_dict, >dict, entry); old = NULL; d->count += 1; @@ -90,9 +90,9 @@ dict_xset(struct dict *d, const char * k struct dictentry*entry; if ((entry = dict_alloc(k, data)) == NULL) - err(1, "dict_xset: malloc"); + fatal("dict_xset: malloc"); if (SPLAY_INSERT(_dict, >dict, entry)) - errx(1, "dict_xset(%p, %s)", d, k); + fatalx("dict_xset(%p, %s)", d, k); d->count += 1; } @@ -115,7 +115,7 @@ dict_xget(struct dict *d, const char *k) key.key = k; if ((entry = SPLAY_FIND(_dict, >dict, )) == NULL) - errx(1, "dict_xget(%p, %s)", d, k); + fatalx("dict_xget(%p, %s)", d, k); return (entry->data); } @@ -146,7 +146,7 @@ dict_xpop(struct dict *d, const char *k) key.key = k; if ((entry = SPLAY_FIND(_dict, >dict, )) == NULL) - errx(1, "dict_xpop(%p, %s)", d, k); +
smtpd: unused code
This diff removes more unused code. Eric. Index: config.c === RCS file: /cvs/src/usr.sbin/smtpd/config.c,v retrieving revision 1.55 diff -u -p -r1.55 config.c --- config.c9 Apr 2021 16:43:43 - 1.55 +++ config.c25 May 2021 20:31:16 - @@ -285,7 +285,6 @@ purge_config(uint8_t what) while (dict_poproot(env->sc_pki_dict, (void **))) { freezero(p->pki_cert, p->pki_cert_len); freezero(p->pki_key, p->pki_key_len); - EVP_PKEY_free(p->pki_pkey); free(p); } free(env->sc_pki_dict); @@ -298,8 +297,6 @@ purge_config(uint8_t what) p->pki_cert = NULL; freezero(p->pki_key, p->pki_key_len); p->pki_key = NULL; - EVP_PKEY_free(p->pki_pkey); - p->pki_pkey = NULL; } } } Index: ssl.c === RCS file: /cvs/src/usr.sbin/smtpd/ssl.c,v retrieving revision 1.94 diff -u -p -r1.94 ssl.c --- ssl.c 5 Mar 2021 12:37:32 - 1.94 +++ ssl.c 25 May 2021 20:39:30 - @@ -65,41 +65,7 @@ ssl_init(void) inited = 1; } -int -ssl_setup(SSL_CTX **ctxp, struct pki *pki, -int (*sni_cb)(SSL *,int *,void *), const char *ciphers) -{ - SSL_CTX *ctx; - uint8_t sid[SSL_MAX_SID_CTX_LENGTH]; - - ctx = ssl_ctx_create(pki->pki_name, pki->pki_cert, pki->pki_cert_len, ciphers); - - /* -* Set session ID context to a random value. We don't support -* persistent caching of sessions so it is OK to set a temporary -* session ID context that is valid during run time. -*/ - arc4random_buf(sid, sizeof(sid)); - if (!SSL_CTX_set_session_id_context(ctx, sid, sizeof(sid))) - goto err; - - if (sni_cb) - SSL_CTX_set_tlsext_servername_callback(ctx, sni_cb); - - SSL_CTX_set_dh_auto(ctx, pki->pki_dhe); - - SSL_CTX_set_ecdh_auto(ctx, 1); - - *ctxp = ctx; - return 1; - -err: - SSL_CTX_free(ctx); - ssl_error("ssl_setup"); - return 0; -} - -char * +static char * ssl_load_file(const char *name, off_t *len, mode_t perm) { struct stat st; @@ -177,7 +143,7 @@ end: return ret; } -char * +static char * ssl_load_key(const char *name, off_t *len, char *pass, mode_t perm, const char *pkiname) { FILE*fp = NULL; @@ -253,53 +219,6 @@ fail: return (NULL); } -SSL_CTX * -ssl_ctx_create(const char *pkiname, char *cert, off_t cert_len, const char *ciphers) -{ - SSL_CTX *ctx; - size_t pkinamelen = 0; - - ctx = SSL_CTX_new(SSLv23_method()); - if (ctx == NULL) { - ssl_error("ssl_ctx_create"); - fatal("ssl_ctx_create: could not create SSL context"); - } - - SSL_CTX_set_session_cache_mode(ctx, SSL_SESS_CACHE_OFF); - SSL_CTX_set_timeout(ctx, SSL_SESSION_TIMEOUT); - SSL_CTX_set_options(ctx, - SSL_OP_ALL | SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3 | SSL_OP_NO_TICKET); - SSL_CTX_set_options(ctx, - SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION); - SSL_CTX_set_options(ctx, SSL_OP_NO_CLIENT_RENEGOTIATION); - SSL_CTX_set_options(ctx, SSL_OP_CIPHER_SERVER_PREFERENCE); - - if (ciphers == NULL) - ciphers = SSL_CIPHERS; - if (!SSL_CTX_set_cipher_list(ctx, ciphers)) { - ssl_error("ssl_ctx_create"); - fatal("ssl_ctx_create: could not set cipher list"); - } - - if (cert != NULL) { - if (pkiname != NULL) - pkinamelen = strlen(pkiname) + 1; - if (!SSL_CTX_use_certificate_chain_mem(ctx, cert, cert_len)) { - ssl_error("ssl_ctx_create"); - fatal("ssl_ctx_create: invalid certificate chain"); - } else if (!ssl_ctx_fake_private_key(ctx, - pkiname, pkinamelen, cert, cert_len, NULL, NULL)) { - ssl_error("ssl_ctx_create"); - fatal("ssl_ctx_create: could not fake private key"); - } else if (!SSL_CTX_check_private_key(ctx)) { - ssl_error("ssl_ctx_create"); - fatal("ssl_ctx_create: invalid private key"); - } - } - - return (ctx); -} - int ssl_load_certificate(struct pki *p, const char *pathname) { @@ -329,19 +248,6 @@ ssl_load_cafile(struct ca *c, const char return 1; } -const char * -ssl_to_text(const SSL *ssl) -{ - static char buf[256]; - - (void)snprintf(buf, sizeof buf, "%s:%s:%d", - SSL_get_version(ssl), - SSL_get_cipher_name(ssl), - SSL_get_cipher_bits(ssl, NULL)); - - return (buf);
smtp: more tls options
The new -T option in smtp(1) allows to plug more TLS parameters easily. For completeness and consistency, this diff adds the following: cafile=: override the default root certificates nosni: disable SNI completely noverify: synonym for -C that can be recycled servername=: synonym for -S (undocumented) that can be recycled too Eric. Index: smtp.1 === RCS file: /cvs/src/usr.sbin/smtpd/smtp.1,v retrieving revision 1.11 diff -u -p -r1.11 smtp.1 --- smtp.1 22 May 2021 12:16:06 - 1.11 +++ smtp.1 22 May 2021 12:49:17 - @@ -53,6 +53,10 @@ This option requires a TLS or STARTTLS .Ar server . .It Fl C Do not require server certificate to be valid. +This flag is deprecated. +Use +.Dq Fl T No noverify +instead. .It Fl F Ar from Set the return-path (MAIL FROM) for the SMTP transaction. Default to the current username. @@ -105,12 +109,21 @@ Refer to .Xr tls_config_set_ciphers 3 for .Ar value . +.It noverify +Do not require server certificate to be valid. +.It nosni +Disable Server Name Indication (SNI). .It protocols Ns = Ns Ar value Specify the protocols to use. Refer to .Xr tls_config_parse_protocols 3 for .Ar value . +.It servername Ns = Ns Ar value +Use +.Ar value +for Server Name Indication (SNI). +Defaults to the specified server hostname. .El .It Fl v Be more verbose. Index: smtpc.c === RCS file: /cvs/src/usr.sbin/smtpd/smtpc.c,v retrieving revision 1.16 diff -u -p -r1.16 smtpc.c --- smtpc.c 22 May 2021 09:09:07 - 1.16 +++ smtpc.c 22 May 2021 12:36:11 - @@ -48,6 +48,8 @@ static struct smtp_mail mail; static const char *servname = NULL; static struct tls_config *tls_config; +static int nosni = 0; +static const char *cafile = NULL; static const char *protocols = NULL; static const char *ciphers = NULL; @@ -65,25 +67,53 @@ static void parse_tls_options(char *opt) { static char * const tokens[] = { -#define CIPHERS 0 +#define CAFILE 0 + "cafile", +#define CIPHERS 1 "ciphers", -#define PROTOCOLS 1 +#define NOSNI 2 + "nosni", +#define NOVERIFY 3 + "noverify", +#define PROTOCOLS 4 "protocols", +#define SERVERNAME 5 + "servername", NULL }; char *value; while (*opt) { switch (getsubopt(, tokens, )) { + case CAFILE: + if (value == NULL) + fatalx("missing value for cafile"); + cafile = value; + break; case CIPHERS: if (value == NULL) fatalx("missing value for ciphers"); ciphers = value; break; + case NOSNI: + if (value != NULL) + fatalx("no value expected for nosni"); + nosni = 1; + break; + case NOVERIFY: + if (value != NULL) + fatalx("no value expected for noverify"); + params.tls_verify = 0; + break; case PROTOCOLS: if (value == NULL) fatalx("missing value for protocols"); protocols = value; break; + case SERVERNAME: + if (value == NULL) + fatalx("missing value for servername"); + servname = value; + break; case -1: if (suboptarg) fatalx("invalid TLS option \"%s\"", suboptarg); @@ -208,7 +238,10 @@ main(int argc, char **argv) if (ciphers && tls_config_set_ciphers(tls_config, ciphers) == -1) fatalx("tls_config_set_ciphers: %s", tls_config_error(tls_config)); - if (tls_config_set_ca_file(tls_config, tls_default_ca_cert_file()) == -1) + + if (cafile == NULL) + cafile = tls_default_ca_cert_file(); + if (tls_config_set_ca_file(tls_config, cafile) == -1) fatal("tls_set_ca_file"); if (!params.tls_verify) { tls_config_insecure_noverifycert(tls_config); @@ -332,7 +365,8 @@ parse_server(char *server) if (servname == NULL) servname = host; - params.tls_servname = servname; + if (nosni == 0) + params.tls_servname = servname; memset(, 0, sizeof(hints)); hints.ai_family = AF_UNSPEC;
Re: smtp(1): protocols and ciphers
Here is an updated diff integrating different suggestions I received. - use -T (for TLS) instead of -O - use getsubopt(3) which I didn't know - manpage tweaks Eric. Index: smtp.1 === RCS file: /cvs/src/usr.sbin/smtpd/smtp.1,v retrieving revision 1.9 diff -u -p -r1.9 smtp.1 --- smtp.1 13 Feb 2021 08:07:48 - 1.9 +++ smtp.1 20 May 2021 09:06:20 - @@ -27,6 +27,7 @@ .Op Fl F Ar from .Op Fl H Ar helo .Op Fl s Ar server +.Op Fl T Ar params .Op Ar recipient ... .Sh DESCRIPTION The @@ -91,6 +92,26 @@ SMTP session with forced TLS on connecti .Pp Defaults to .Dq smtp://localhost:25 . +.It Fl T Ar params +Set specific parameters for TLS sessions. +The +.Ar params +string is a comma or space separated list of options. +The available options are: +.Bl -tag -width Ds +.It protocols Ns = Ns Ar value +Specify the protocols to use. +Refer to +.Xr tls_config_parse_protocols 3 +for +.Ar value . +.It ciphers Ns = Ns Ar value +Specify the allowed ciphers. +Refer to +.Xr tls_config_set_ciphers 3 +for +.Ar value . +.El .It Fl v Be more verbose. This option can be specified multiple times. Index: smtpc.c === RCS file: /cvs/src/usr.sbin/smtpd/smtpc.c,v retrieving revision 1.15 diff -u -p -r1.15 smtpc.c --- smtpc.c 10 Apr 2021 10:19:19 - 1.15 +++ smtpc.c 20 May 2021 08:57:16 - @@ -48,22 +48,57 @@ static struct smtp_mail mail; static const char *servname = NULL; static struct tls_config *tls_config; +static const char *protocols = NULL; +static const char *ciphers = NULL; + static void usage(void) { extern char *__progname; fprintf(stderr, "usage: %s [-Chnv] [-a authfile] [-F from] [-H helo] " - "[-s server] [recipient ...]\n", __progname); + "[-s server] [-T params] [recipient ...]\n", __progname); exit(1); } +static void +parse_tls_options(char *opt) +{ + static char * const tokens[] = { +#define CIPHERS 0 + "ciphers", +#define PROTOCOLS 1 + "protocols", + NULL }; + char *value; + + while (*opt) { + switch (getsubopt(, tokens, )) { + case CIPHERS: + if (value == NULL) + fatalx("missing value for ciphers"); + ciphers = value; + break; + case PROTOCOLS: + if (value == NULL) + fatalx("missing value for protocols"); + protocols = value; + break; + case -1: + if (suboptarg) + fatalx("invalid TLS option \"%s\"", suboptarg); + fatalx("missing TLS option"); + } + } +} + int main(int argc, char **argv) { char hostname[256]; FILE *authfile; int ch, i; + uint32_t protos; char *server = "localhost"; char *authstr = NULL; size_t alloc = 0; @@ -91,7 +126,7 @@ main(int argc, char **argv) memset(, 0, sizeof(mail)); mail.from = pw->pw_name; - while ((ch = getopt(argc, argv, "CF:H:S:a:hns:v")) != -1) { + while ((ch = getopt(argc, argv, "CF:H:S:T:a:hns:v")) != -1) { switch (ch) { case 'C': params.tls_verify = 0; @@ -105,6 +140,9 @@ main(int argc, char **argv) case 'S': servname = optarg; break; + case 'T': + parse_tls_options(optarg); + break; case 'a': if ((authfile = fopen(optarg, "r")) == NULL) fatal("%s: open", optarg); @@ -159,6 +197,17 @@ main(int argc, char **argv) tls_config = tls_config_new(); if (tls_config == NULL) fatal("tls_config_new"); + + if (protocols) { + if (tls_config_parse_protocols(, protocols) == -1) + fatalx("failed to parse protocol '%s'", protocols); + if (tls_config_set_protocols(tls_config, protos) == -1) + fatalx("tls_config_set_protocols: %s", + tls_config_error(tls_config)); + } + if (ciphers && tls_config_set_ciphers(tls_config, ciphers) == -1) + fatalx("tls_config_set_ciphers: %s", + tls_config_error(tls_config)); if (tls_config_set_ca_file(tls_config, tls_default_ca_cert_file()) == -1) fatal("tls_set_ca_file"); if (!params.tls_verify) {
smtp(1): protocols and ciphers
Hello. This diff allows to specify protcols and ciphers in smtp(1). I thought it was cleaner to added a generic -O option flag for this. Eric. Index: smtp.1 === RCS file: /cvs/src/usr.sbin/smtpd/smtp.1,v retrieving revision 1.9 diff -u -p -r1.9 smtp.1 --- smtp.1 13 Feb 2021 08:07:48 - 1.9 +++ smtp.1 14 May 2021 13:25:59 - @@ -26,6 +26,7 @@ .Op Fl a Ar authfile .Op Fl F Ar from .Op Fl H Ar helo +.Op Fl O Ar option .Op Fl s Ar server .Op Ar recipient ... .Sh DESCRIPTION @@ -63,6 +64,31 @@ Display usage. Do not actually execute a transaction, just try to establish an SMTP session and quit. When this option is given, no message is read from the standard input. +.It Fl O Ar option +Set additional configuration options that don't have a specific flag. +The +.Ar option +string has the form +.Sm off +.Ar name No = Ar value +.Sm on +where +.Ar name +is one of the following: +.Bl -tag -width "protocols" +.It protocols +Specify the protocols to use for tls connections. +Refer to +.Xr tls_config_parse_protocols 3 +for +.Ar value . +.It ciphers +Specify the allowed ciphers for tls connections. +Refer to +.Xr tls_config_set_ciphers 3 +for +.Ar value . +.El .It Fl s Ar server Specify the server to connect to and connection parameters. The format is Index: smtpc.c === RCS file: /cvs/src/usr.sbin/smtpd/smtpc.c,v retrieving revision 1.15 diff -u -p -r1.15 smtpc.c --- smtpc.c 10 Apr 2021 10:19:19 - 1.15 +++ smtpc.c 14 May 2021 13:31:45 - @@ -48,22 +48,45 @@ static struct smtp_mail mail; static const char *servname = NULL; static struct tls_config *tls_config; +static const char *protocols = NULL; +static const char *ciphers = NULL; + static void usage(void) { extern char *__progname; fprintf(stderr, "usage: %s [-Chnv] [-a authfile] [-F from] [-H helo] " + "[-O option] " "[-s server] [recipient ...]\n", __progname); exit(1); } +static void +parse_option(char *opt) +{ + char *v; + + v = strchr(opt, '='); + if (v == NULL) + fatalx("invalid option string \"%s\"", opt); + *v++ = '\0'; + + if (!strcmp(opt, "ciphers")) + ciphers = v; + else if (!strcmp(opt, "protocols")) + protocols = v; + else + fatalx("unknown option \"%s\"", opt); +} + int main(int argc, char **argv) { char hostname[256]; FILE *authfile; int ch, i; + uint32_t protos; char *server = "localhost"; char *authstr = NULL; size_t alloc = 0; @@ -91,7 +114,7 @@ main(int argc, char **argv) memset(, 0, sizeof(mail)); mail.from = pw->pw_name; - while ((ch = getopt(argc, argv, "CF:H:S:a:hns:v")) != -1) { + while ((ch = getopt(argc, argv, "CF:H:O:S:a:hns:v")) != -1) { switch (ch) { case 'C': params.tls_verify = 0; @@ -102,6 +125,9 @@ main(int argc, char **argv) case 'H': params.helo = optarg; break; + case 'O': + parse_option(optarg); + break; case 'S': servname = optarg; break; @@ -159,6 +185,17 @@ main(int argc, char **argv) tls_config = tls_config_new(); if (tls_config == NULL) fatal("tls_config_new"); + + if (protocols) { + if (tls_config_parse_protocols(, protocols) == -1) + fatalx("failed to parse protocol '%s'", protocols); + if (tls_config_set_protocols(tls_config, protos) == -1) + fatalx("tls_config_set_protocols: %s", + tls_config_error(tls_config)); + } + if (ciphers && tls_config_set_ciphers(tls_config, ciphers) == -1) + fatalx("tls_config_set_ciphers: %s", + tls_config_error(tls_config)); if (tls_config_set_ca_file(tls_config, tls_default_ca_cert_file()) == -1) fatal("tls_set_ca_file"); if (!params.tls_verify) {
Re: smtpd: remove tls_accept/tls_connect callbacks
On Wed, Apr 21, 2021 at 11:21:51AM +0200, Eric Faurot wrote: > There is actually no reason to defer calls to tls_accept_socket() and > tls_connect_socket() in an event callback. The code can be simplified > by a great deal. It also eliminates the issue of keeping a reference > to the listener tls context in the io structure. Did anyone had a chance to look at it? > Eric. > > > Index: ioev.c > === > RCS file: /cvs/src/usr.sbin/smtpd/ioev.c,v > retrieving revision 1.45 > diff -u -p -r1.45 ioev.c > --- ioev.c5 Apr 2021 15:50:11 - 1.45 > +++ ioev.c21 Apr 2021 08:35:29 - > @@ -64,7 +64,6 @@ struct io { > int state; > struct event ev; > struct tls *tls; > - char*name; > > const char *error; /* only valid immediately on callback */ > }; > @@ -280,7 +279,6 @@ io_free(struct io *io) > io->sock = -1; > } > > - free(io->name); > iobuf_clear(>iobuf); > free(io); > } > @@ -817,14 +815,14 @@ io_connect_tls(struct io *io, struct tls > if (io->tls) > errx(1, "io_connect_tls: TLS already started"); > > - if (hostname) { > - if ((io->name = strdup(hostname)) == NULL) > - err(1, "io_connect_tls"); > + if (tls_connect_socket(tls, io->sock, hostname) == -1) { > + io->error = tls_error(tls); > + return (-1); > } > > io->tls = tls; > io->state = IO_STATE_CONNECT_TLS; > - io_reset(io, EV_WRITE, io_dispatch_connect_tls); > + io_reset(io, EV_READ|EV_WRITE, io_dispatch_handshake_tls); > > return (0); > } > @@ -840,9 +838,14 @@ io_accept_tls(struct io *io, struct tls > > if (io->tls) > errx(1, "io_accept_tls: TLS already started"); > - io->tls = tls; > + > + if (tls_accept_socket(tls, >tls, io->sock) == -1) { > + io->error = tls_error(tls); > + return (-1); > + } > + > io->state = IO_STATE_ACCEPT_TLS; > - io_reset(io, EV_READ, io_dispatch_accept_tls); > + io_reset(io, EV_READ|EV_WRITE, io_dispatch_handshake_tls); > > return (0); > } > @@ -880,60 +883,6 @@ io_dispatch_handshake_tls(int fd, short > } > > void > -io_dispatch_accept_tls(int fd, short event, void *humppa) > -{ > - struct io *io = humppa; > - struct tls *tls = io->tls; > - int ret; > - > - io_frame_enter("io_dispatch_accept_tls", io, event); > - > - /* Replaced by TLS context for accepted socket on success. */ > - io->tls = NULL; > - > - if (event == EV_TIMEOUT) { > - io_callback(io, IO_TIMEOUT); > - goto leave; > - } > - > - if ((ret = tls_accept_socket(tls, >tls, io->sock)) == 0) { > - io_reset(io, EV_READ|EV_WRITE, io_dispatch_handshake_tls); > - goto leave; > - } > - io->error = tls_error(tls); > - io_callback(io, IO_ERROR); > - > - leave: > - io_frame_leave(io); > - return; > -} > - > -void > -io_dispatch_connect_tls(int fd, short event, void *humppa) > -{ > - struct io *io = humppa; > - int ret; > - > - io_frame_enter("io_dispatch_connect_tls", io, event); > - > - if (event == EV_TIMEOUT) { > - io_callback(io, IO_TIMEOUT); > - goto leave; > - } > - > - if ((ret = tls_connect_socket(io->tls, io->sock, io->name)) == 0) { > - io_reset(io, EV_READ|EV_WRITE, io_dispatch_handshake_tls); > - goto leave; > - } > - > - io->error = tls_error(io->tls); > - io_callback(io, IO_ERROR); > - > - leave: > - io_frame_leave(io); > -} > - > -void > io_dispatch_read_tls(int fd, short event, void *humppa) > { > struct io *io = humppa; > @@ -1017,37 +966,20 @@ io_dispatch_write_tls(int fd, short even > void > io_reload_tls(struct io *io) > { > - short ev = 0; > - void(*dispatch)(int, short, void*) = NULL; > - > - switch (io->state) { > - case IO_STATE_CONNECT_TLS: > - ev = EV_WRITE; > - dispatch = io_dispatch_connect_tls; > - break; > - case IO_STATE_ACCEPT_TLS: > - ev = EV_READ; > - dispatch = io_dispatch_accept_tls; > - break; > - case IO_STATE_UP: > - ev = 0; > -
smtpd: remove tls_accept/tls_connect callbacks
There is actually no reason to defer calls to tls_accept_socket() and tls_connect_socket() in an event callback. The code can be simplified by a great deal. It also eliminates the issue of keeping a reference to the listener tls context in the io structure. Eric. Index: ioev.c === RCS file: /cvs/src/usr.sbin/smtpd/ioev.c,v retrieving revision 1.45 diff -u -p -r1.45 ioev.c --- ioev.c 5 Apr 2021 15:50:11 - 1.45 +++ ioev.c 21 Apr 2021 08:35:29 - @@ -64,7 +64,6 @@ struct io { int state; struct event ev; struct tls *tls; - char*name; const char *error; /* only valid immediately on callback */ }; @@ -280,7 +279,6 @@ io_free(struct io *io) io->sock = -1; } - free(io->name); iobuf_clear(>iobuf); free(io); } @@ -817,14 +815,14 @@ io_connect_tls(struct io *io, struct tls if (io->tls) errx(1, "io_connect_tls: TLS already started"); - if (hostname) { - if ((io->name = strdup(hostname)) == NULL) - err(1, "io_connect_tls"); + if (tls_connect_socket(tls, io->sock, hostname) == -1) { + io->error = tls_error(tls); + return (-1); } io->tls = tls; io->state = IO_STATE_CONNECT_TLS; - io_reset(io, EV_WRITE, io_dispatch_connect_tls); + io_reset(io, EV_READ|EV_WRITE, io_dispatch_handshake_tls); return (0); } @@ -840,9 +838,14 @@ io_accept_tls(struct io *io, struct tls if (io->tls) errx(1, "io_accept_tls: TLS already started"); - io->tls = tls; + + if (tls_accept_socket(tls, >tls, io->sock) == -1) { + io->error = tls_error(tls); + return (-1); + } + io->state = IO_STATE_ACCEPT_TLS; - io_reset(io, EV_READ, io_dispatch_accept_tls); + io_reset(io, EV_READ|EV_WRITE, io_dispatch_handshake_tls); return (0); } @@ -880,60 +883,6 @@ io_dispatch_handshake_tls(int fd, short } void -io_dispatch_accept_tls(int fd, short event, void *humppa) -{ - struct io *io = humppa; - struct tls *tls = io->tls; - int ret; - - io_frame_enter("io_dispatch_accept_tls", io, event); - - /* Replaced by TLS context for accepted socket on success. */ - io->tls = NULL; - - if (event == EV_TIMEOUT) { - io_callback(io, IO_TIMEOUT); - goto leave; - } - - if ((ret = tls_accept_socket(tls, >tls, io->sock)) == 0) { - io_reset(io, EV_READ|EV_WRITE, io_dispatch_handshake_tls); - goto leave; - } - io->error = tls_error(tls); - io_callback(io, IO_ERROR); - - leave: - io_frame_leave(io); - return; -} - -void -io_dispatch_connect_tls(int fd, short event, void *humppa) -{ - struct io *io = humppa; - int ret; - - io_frame_enter("io_dispatch_connect_tls", io, event); - - if (event == EV_TIMEOUT) { - io_callback(io, IO_TIMEOUT); - goto leave; - } - - if ((ret = tls_connect_socket(io->tls, io->sock, io->name)) == 0) { - io_reset(io, EV_READ|EV_WRITE, io_dispatch_handshake_tls); - goto leave; - } - - io->error = tls_error(io->tls); - io_callback(io, IO_ERROR); - - leave: - io_frame_leave(io); -} - -void io_dispatch_read_tls(int fd, short event, void *humppa) { struct io *io = humppa; @@ -1017,37 +966,20 @@ io_dispatch_write_tls(int fd, short even void io_reload_tls(struct io *io) { - short ev = 0; - void(*dispatch)(int, short, void*) = NULL; - - switch (io->state) { - case IO_STATE_CONNECT_TLS: - ev = EV_WRITE; - dispatch = io_dispatch_connect_tls; - break; - case IO_STATE_ACCEPT_TLS: - ev = EV_READ; - dispatch = io_dispatch_accept_tls; - break; - case IO_STATE_UP: - ev = 0; - if (IO_READING(io) && !(io->flags & IO_PAUSE_IN)) { - ev = EV_READ; - dispatch = io_dispatch_read_tls; - } - else if (IO_WRITING(io) && !(io->flags & IO_PAUSE_OUT) && - io_queued(io)) { - ev = EV_WRITE; - dispatch = io_dispatch_write_tls; - } - if (!ev) - return; /* paused */ - break; - default: + if (io->state != IO_STATE_UP) errx(1, "io_reload_tls: bad state"); + + if (IO_READING(io) && !(io->flags & IO_PAUSE_IN)) { + io_reset(io, EV_READ, io_dispatch_read_tls); + return; + } + + if (IO_WRITING(io) &&
Re: smtpd: more unused code
On Sun, Apr 11, 2021 at 01:54:32PM +0200, Eric Faurot wrote: > Certificate verification is done by libtls. The former code is not used > anymore and can be unplugged. Anyone willing to ok this? > Eric. > > Index: dispatcher.c > === > RCS file: /cvs/src/usr.sbin/smtpd/dispatcher.c,v > retrieving revision 1.2 > diff -u -p -r1.2 dispatcher.c > --- dispatcher.c 5 Mar 2021 12:37:32 - 1.2 > +++ dispatcher.c 11 Apr 2021 11:46:17 - > @@ -64,11 +64,6 @@ dispatcher_imsg(struct mproc *p, struct > resolver_dispatch_result(p, imsg); > return; > > - case IMSG_CERT_INIT: > - case IMSG_CERT_VERIFY: > - cert_dispatch_result(p, imsg); > - return; > - > case IMSG_CONF_START: > return; > case IMSG_CONF_END: > Index: lka.c > === > RCS file: /cvs/src/usr.sbin/smtpd/lka.c,v > retrieving revision 1.244 > diff -u -p -r1.244 lka.c > --- lka.c 31 Dec 2020 08:27:15 - 1.244 > +++ lka.c 11 Apr 2021 11:45:24 - > @@ -111,12 +111,6 @@ lka_imsg(struct mproc *p, struct imsg *i > resolver_dispatch_request(p, imsg); > return; > > - case IMSG_CERT_INIT: > - case IMSG_CERT_CERTIFICATE: > - case IMSG_CERT_VERIFY: > - cert_dispatch_request(p, imsg); > - return; > - > case IMSG_MTA_DNS_HOST: > case IMSG_MTA_DNS_MX: > case IMSG_MTA_DNS_MX_PREFERENCE: > Index: smtpd.c > === > RCS file: /cvs/src/usr.sbin/smtpd/smtpd.c,v > retrieving revision 1.337 > diff -u -p -r1.337 smtpd.c > --- smtpd.c 5 Mar 2021 12:37:32 - 1.337 > +++ smtpd.c 11 Apr 2021 11:46:38 - > @@ -2003,10 +2003,6 @@ imsg_to_str(int type) > CASE(IMSG_GETNAMEINFO); > CASE(IMSG_RES_QUERY); > > - CASE(IMSG_CERT_INIT); > - CASE(IMSG_CERT_CERTIFICATE); > - CASE(IMSG_CERT_VERIFY); > - > CASE(IMSG_SETUP_KEY); > CASE(IMSG_SETUP_PEER); > CASE(IMSG_SETUP_DONE); > Index: smtpd.h > === > RCS file: /cvs/src/usr.sbin/smtpd/smtpd.h,v > retrieving revision 1.667 > diff -u -p -r1.667 smtpd.h > --- smtpd.h 11 Apr 2021 07:18:08 - 1.667 > +++ smtpd.h 11 Apr 2021 11:45:58 - > @@ -102,12 +102,6 @@ > #define P_NEWALIASES 1 > #define P_MAKEMAP2 > > -#define CERT_ERROR -1 > -#define CERT_OK 0 > -#define CERT_NOCA1 > -#define CERT_NOCERT 2 > -#define CERT_INVALID 3 > - > struct userinfo { > char username[SMTPD_VUSERNAME_SIZE]; > char directory[PATH_MAX]; > @@ -211,10 +205,6 @@ enum imsg_type { > IMSG_GETNAMEINFO, > IMSG_RES_QUERY, > > - IMSG_CERT_INIT, > - IMSG_CERT_CERTIFICATE, > - IMSG_CERT_VERIFY, > - > IMSG_SETUP_KEY, > IMSG_SETUP_PEER, > IMSG_SETUP_DONE, > @@ -1281,14 +1271,6 @@ int ca_X509_verify(void *, void *, cons > void ca_imsg(struct mproc *, struct imsg *); > void ca_init(void); > void ca_engine_init(void); > - > - > -/* cert.c */ > -int cert_init(const char *, int, > -void (*)(void *, int, const char *, const void *, size_t), void *); > -int cert_verify(const void *, const char *, int, void (*)(void *, int), void > *); > -void cert_dispatch_request(struct mproc *, struct imsg *); > -void cert_dispatch_result(struct mproc *, struct imsg *); > > > /* compress_backend.c */ > Index: smtpd/Makefile > === > RCS file: /cvs/src/usr.sbin/smtpd/smtpd/Makefile,v > retrieving revision 1.112 > diff -u -p -r1.112 Makefile > --- smtpd/Makefile11 Apr 2021 07:18:08 - 1.112 > +++ smtpd/Makefile11 Apr 2021 11:44:42 - > @@ -7,7 +7,6 @@ PROG= smtpd > SRCS=aliases.c > SRCS+= bounce.c > SRCS+= ca.c > -SRCS+= cert.c > SRCS+= compress_backend.c > SRCS+= config.c > SRCS+= control.c > >
Re: smtpd: more unused code
On Mon, Apr 12, 2021 at 07:56:57AM -0400, Dave Voutila wrote: > > Eric Faurot writes: > > > Certificate verification is done by libtls. The former code is not used > > anymore and can be unplugged. > > Should cert.c be removed? I don't think it's used by smtp{d,ctl} or mail > after your change. Or were you going to commit that seperately? There are a few files that are not used anymore, My plan was to wait after the release for actually removing them. Eric. > > > > > Eric. > > > > Index: dispatcher.c > > === > > RCS file: /cvs/src/usr.sbin/smtpd/dispatcher.c,v > > retrieving revision 1.2 > > diff -u -p -r1.2 dispatcher.c > > --- dispatcher.c5 Mar 2021 12:37:32 - 1.2 > > +++ dispatcher.c11 Apr 2021 11:46:17 - > > @@ -64,11 +64,6 @@ dispatcher_imsg(struct mproc *p, struct > > resolver_dispatch_result(p, imsg); > > return; > > > > - case IMSG_CERT_INIT: > > - case IMSG_CERT_VERIFY: > > - cert_dispatch_result(p, imsg); > > - return; > > - > > case IMSG_CONF_START: > > return; > > case IMSG_CONF_END: > > Index: lka.c > > === > > RCS file: /cvs/src/usr.sbin/smtpd/lka.c,v > > retrieving revision 1.244 > > diff -u -p -r1.244 lka.c > > --- lka.c 31 Dec 2020 08:27:15 - 1.244 > > +++ lka.c 11 Apr 2021 11:45:24 - > > @@ -111,12 +111,6 @@ lka_imsg(struct mproc *p, struct imsg *i > > resolver_dispatch_request(p, imsg); > > return; > > > > - case IMSG_CERT_INIT: > > - case IMSG_CERT_CERTIFICATE: > > - case IMSG_CERT_VERIFY: > > - cert_dispatch_request(p, imsg); > > - return; > > - > > case IMSG_MTA_DNS_HOST: > > case IMSG_MTA_DNS_MX: > > case IMSG_MTA_DNS_MX_PREFERENCE: > > Index: smtpd.c > > === > > RCS file: /cvs/src/usr.sbin/smtpd/smtpd.c,v > > retrieving revision 1.337 > > diff -u -p -r1.337 smtpd.c > > --- smtpd.c 5 Mar 2021 12:37:32 - 1.337 > > +++ smtpd.c 11 Apr 2021 11:46:38 - > > @@ -2003,10 +2003,6 @@ imsg_to_str(int type) > > CASE(IMSG_GETNAMEINFO); > > CASE(IMSG_RES_QUERY); > > > > - CASE(IMSG_CERT_INIT); > > - CASE(IMSG_CERT_CERTIFICATE); > > - CASE(IMSG_CERT_VERIFY); > > - > > CASE(IMSG_SETUP_KEY); > > CASE(IMSG_SETUP_PEER); > > CASE(IMSG_SETUP_DONE); > > Index: smtpd.h > > === > > RCS file: /cvs/src/usr.sbin/smtpd/smtpd.h,v > > retrieving revision 1.667 > > diff -u -p -r1.667 smtpd.h > > --- smtpd.h 11 Apr 2021 07:18:08 - 1.667 > > +++ smtpd.h 11 Apr 2021 11:45:58 - > > @@ -102,12 +102,6 @@ > > #define P_NEWALIASES 1 > > #define P_MAKEMAP 2 > > > > -#defineCERT_ERROR -1 > > -#defineCERT_OK 0 > > -#defineCERT_NOCA1 > > -#defineCERT_NOCERT 2 > > -#defineCERT_INVALID 3 > > - > > struct userinfo { > > char username[SMTPD_VUSERNAME_SIZE]; > > char directory[PATH_MAX]; > > @@ -211,10 +205,6 @@ enum imsg_type { > > IMSG_GETNAMEINFO, > > IMSG_RES_QUERY, > > > > - IMSG_CERT_INIT, > > - IMSG_CERT_CERTIFICATE, > > - IMSG_CERT_VERIFY, > > - > > IMSG_SETUP_KEY, > > IMSG_SETUP_PEER, > > IMSG_SETUP_DONE, > > @@ -1281,14 +1271,6 @@ int ca_X509_verify(void *, void *, cons > > voidca_imsg(struct mproc *, struct imsg *); > > voidca_init(void); > > voidca_engine_init(void); > > - > > - > > -/* cert.c */ > > -int cert_init(const char *, int, > > -void (*)(void *, int, const char *, const void *, size_t), void *); > > -int cert_verify(const void *, const char *, int, void (*)(void *, int), > > void *); > > -void cert_dispatch_request(struct mproc *, struct imsg *); > > -void cert_dispatch_result(struct mproc *, struct imsg *); > > > > > > /* compress_backend.c */ > > Index: smtpd/Makefile > > === > > RCS file: /cvs/src/usr.sbin/smtpd/smtpd/Makefile,v > > retrieving revision 1.112 > > diff -u -p -r1.112 Makefile > > --- smtpd/Makefile 11 Apr 2021 07:18:08 - 1.112 > > +++ smtpd/Makefile 11 Apr 2021 11:44:42 - > > @@ -7,7 +7,6 @@ PROG= smtpd > > SRCS= aliases.c > > SRCS+= bounce.c > > SRCS+= ca.c > > -SRCS+= cert.c > > SRCS+= compress_backend.c > > SRCS+= config.c > > SRCS+= control.c > >
smtpd: more unused code
Certificate verification is done by libtls. The former code is not used anymore and can be unplugged. Eric. Index: dispatcher.c === RCS file: /cvs/src/usr.sbin/smtpd/dispatcher.c,v retrieving revision 1.2 diff -u -p -r1.2 dispatcher.c --- dispatcher.c5 Mar 2021 12:37:32 - 1.2 +++ dispatcher.c11 Apr 2021 11:46:17 - @@ -64,11 +64,6 @@ dispatcher_imsg(struct mproc *p, struct resolver_dispatch_result(p, imsg); return; - case IMSG_CERT_INIT: - case IMSG_CERT_VERIFY: - cert_dispatch_result(p, imsg); - return; - case IMSG_CONF_START: return; case IMSG_CONF_END: Index: lka.c === RCS file: /cvs/src/usr.sbin/smtpd/lka.c,v retrieving revision 1.244 diff -u -p -r1.244 lka.c --- lka.c 31 Dec 2020 08:27:15 - 1.244 +++ lka.c 11 Apr 2021 11:45:24 - @@ -111,12 +111,6 @@ lka_imsg(struct mproc *p, struct imsg *i resolver_dispatch_request(p, imsg); return; - case IMSG_CERT_INIT: - case IMSG_CERT_CERTIFICATE: - case IMSG_CERT_VERIFY: - cert_dispatch_request(p, imsg); - return; - case IMSG_MTA_DNS_HOST: case IMSG_MTA_DNS_MX: case IMSG_MTA_DNS_MX_PREFERENCE: Index: smtpd.c === RCS file: /cvs/src/usr.sbin/smtpd/smtpd.c,v retrieving revision 1.337 diff -u -p -r1.337 smtpd.c --- smtpd.c 5 Mar 2021 12:37:32 - 1.337 +++ smtpd.c 11 Apr 2021 11:46:38 - @@ -2003,10 +2003,6 @@ imsg_to_str(int type) CASE(IMSG_GETNAMEINFO); CASE(IMSG_RES_QUERY); - CASE(IMSG_CERT_INIT); - CASE(IMSG_CERT_CERTIFICATE); - CASE(IMSG_CERT_VERIFY); - CASE(IMSG_SETUP_KEY); CASE(IMSG_SETUP_PEER); CASE(IMSG_SETUP_DONE); Index: smtpd.h === RCS file: /cvs/src/usr.sbin/smtpd/smtpd.h,v retrieving revision 1.667 diff -u -p -r1.667 smtpd.h --- smtpd.h 11 Apr 2021 07:18:08 - 1.667 +++ smtpd.h 11 Apr 2021 11:45:58 - @@ -102,12 +102,6 @@ #define P_NEWALIASES 1 #define P_MAKEMAP 2 -#defineCERT_ERROR -1 -#defineCERT_OK 0 -#defineCERT_NOCA1 -#defineCERT_NOCERT 2 -#defineCERT_INVALID 3 - struct userinfo { char username[SMTPD_VUSERNAME_SIZE]; char directory[PATH_MAX]; @@ -211,10 +205,6 @@ enum imsg_type { IMSG_GETNAMEINFO, IMSG_RES_QUERY, - IMSG_CERT_INIT, - IMSG_CERT_CERTIFICATE, - IMSG_CERT_VERIFY, - IMSG_SETUP_KEY, IMSG_SETUP_PEER, IMSG_SETUP_DONE, @@ -1281,14 +1271,6 @@ int ca_X509_verify(void *, void *, cons voidca_imsg(struct mproc *, struct imsg *); voidca_init(void); voidca_engine_init(void); - - -/* cert.c */ -int cert_init(const char *, int, -void (*)(void *, int, const char *, const void *, size_t), void *); -int cert_verify(const void *, const char *, int, void (*)(void *, int), void *); -void cert_dispatch_request(struct mproc *, struct imsg *); -void cert_dispatch_result(struct mproc *, struct imsg *); /* compress_backend.c */ Index: smtpd/Makefile === RCS file: /cvs/src/usr.sbin/smtpd/smtpd/Makefile,v retrieving revision 1.112 diff -u -p -r1.112 Makefile --- smtpd/Makefile 11 Apr 2021 07:18:08 - 1.112 +++ smtpd/Makefile 11 Apr 2021 11:44:42 - @@ -7,7 +7,6 @@ PROG= smtpd SRCS= aliases.c SRCS+= bounce.c SRCS+= ca.c -SRCS+= cert.c SRCS+= compress_backend.c SRCS+= config.c SRCS+= control.c
smtpd: unused files and dependency
Do not build unused files and remove related prototypes. Also remove bogus libm dependency. Index: smtpd.h === RCS file: /cvs/src/usr.sbin/smtpd/smtpd.h,v retrieving revision 1.666 diff -u -p -r1.666 smtpd.h --- smtpd.h 10 Apr 2021 06:44:18 - 1.666 +++ smtpd.h 10 Apr 2021 12:34:27 - @@ -1638,11 +1638,6 @@ const char *srs_encode(const char *, con const char *srs_decode(const char *); -/* ssl_smtpd.c */ -void *ssl_mta_init(void *, char *, off_t, const char *); -void *ssl_smtp_init(void *, int); - - /* stat_backend.c */ struct stat_backend*stat_backend_lookup(const char *); void stat_increment(const char *, size_t); Index: ssl.h === RCS file: /cvs/src/usr.sbin/smtpd/ssl.h,v retrieving revision 1.22 diff -u -p -r1.22 ssl.h --- ssl.h 5 Mar 2021 12:37:32 - 1.22 +++ ssl.h 10 Apr 2021 12:54:19 - @@ -65,9 +65,3 @@ int ssl_load_pkey(const void *, size_t, intssl_ctx_fake_private_key(SSL_CTX *, const void *, size_t, char *, off_t, X509 **, EVP_PKEY **); char *ssl_pubkey_hash(const char *, off_t); - -/* ssl_privsep.c */ -intssl_by_mem_ctrl(X509_LOOKUP *, int, const char *, long, char **); - -/* ssl_verify.c */ -int ssl_check_name(X509 *, const char *, int *); Index: smtpd/Makefile === RCS file: /cvs/src/usr.sbin/smtpd/smtpd/Makefile,v retrieving revision 1.111 diff -u -p -r1.111 Makefile --- smtpd/Makefile 5 Mar 2021 12:37:32 - 1.111 +++ smtpd/Makefile 10 Apr 2021 12:33:13 - @@ -51,8 +51,6 @@ SRCS+=smtp_session.c SRCS+= smtpd.c SRCS+= srs.c SRCS+= ssl.c -SRCS+= ssl_smtpd.c -SRCS+= ssl_verify.c SRCS+= stat_backend.c SRCS+= table.c SRCS+= to.c @@ -82,8 +80,8 @@ SRCS+=stat_ramstat.c MAN= sendmail.8 smtpd.8 smtpd.conf.5 table.5 BINDIR=/usr/sbin -LDADD+=-levent -lutil -ltls -lssl -lcrypto -lm -lz -DPADD+=${LIBEVENT} ${LIBUTIL} ${LIBTLS} ${LIBSSL} ${LIBCRYPTO} ${LIBM} ${LIBZ} +LDADD+=-levent -lutil -ltls -lssl -lcrypto -lz +DPADD+=${LIBEVENT} ${LIBUTIL} ${LIBTLS} ${LIBSSL} ${LIBCRYPTO} ${LIBZ} CFLAGS+= -fstack-protector-all CFLAGS+= -I${.CURDIR}/..
smtp: dead code
This diff removes unused code and lib depends from smtp(1). Eric. Index: smtpc.c === RCS file: /cvs/src/usr.sbin/smtpd/smtpc.c,v retrieving revision 1.14 diff -u -p -r1.14 smtpc.c --- smtpc.c 5 Mar 2021 12:37:32 - 1.14 +++ smtpc.c 10 Apr 2021 08:25:20 - @@ -32,10 +32,7 @@ #include #include -#include - #include "smtp.h" -#include "ssl.h" #include "log.h" static void parse_server(char *); @@ -368,37 +365,6 @@ log_trace(int lvl, const char *emsg, ... vlog(LOG_DEBUG, emsg, ap); va_end(ap); } -} - -void -smtp_verify_server_cert(void *tag, struct smtp_client *proto, void *ctx) -{ - SSL *ssl = ctx; - X509 *cert; - long res; - int match; - - if ((cert = SSL_get_peer_certificate(ssl))) { - (void)ssl_check_name(cert, servname, ); - X509_free(cert); - res = SSL_get_verify_result(ssl); - if (res == X509_V_OK) { - if (match) { - log_debug("valid certificate"); - smtp_cert_verified(proto, CERT_OK); - } - else { - log_debug("certificate does not match hostname"); - smtp_cert_verified(proto, CERT_INVALID); - } - return; - } - log_debug("certificate validation error %ld", res); - } - else - log_debug("no certificate provided"); - - smtp_cert_verified(proto, CERT_INVALID); } void Index: smtp/Makefile === RCS file: /cvs/src/usr.sbin/smtpd/smtp/Makefile,v retrieving revision 1.4 diff -u -p -r1.4 Makefile --- smtp/Makefile 5 Mar 2021 12:37:32 - 1.4 +++ smtp/Makefile 10 Apr 2021 08:25:20 - @@ -12,12 +12,10 @@ SRCS+= ioev.c SRCS+= log.c SRCS+= smtp_client.c SRCS+= smtpc.c -SRCS+= ssl.c -SRCS+= ssl_verify.c CPPFLAGS+= -DIO_TLS -LDADD+=-levent -lutil -ltls -lssl -lcrypto -lm -lz -DPADD+=${LIBEVENT} ${LIBUTIL} ${LIBTLS} ${LIBSSL} ${LIBCRYPTO} ${LIBM} ${LIBZ} +LDADD+=-levent -lutil -ltls +DPADD+=${LIBEVENT} ${LIBUTIL} ${LIBTLS} .include
smtpd: tls ciphers and protocols on listeners
Hi, This diff allows to specify tls ciphers and protocols on listen rules, as it's been done already for relay actions. While there, sanitize error checking on protocols config in the mta. Eric. Index: config.c === RCS file: /cvs/src/usr.sbin/smtpd/config.c,v retrieving revision 1.54 diff -u -p -r1.54 config.c --- config.c5 Mar 2021 12:37:32 - 1.54 +++ config.c6 Apr 2021 10:24:18 - @@ -252,6 +252,8 @@ purge_config(uint8_t what) if (what & PURGE_LISTENERS) { while ((l = TAILQ_FIRST(env->sc_listeners)) != NULL) { TAILQ_REMOVE(env->sc_listeners, l, entry); + free(l->tls_ciphers); + free(l->tls_protocols); free(l->pki); free(l); } Index: mta.c === RCS file: /cvs/src/usr.sbin/smtpd/mta.c,v retrieving revision 1.237 diff -u -p -r1.237 mta.c --- mta.c 2 Apr 2021 06:30:55 - 1.237 +++ mta.c 6 Apr 2021 10:39:52 - @@ -508,10 +508,14 @@ mta_setup_dispatcher(struct dispatcher * if (ciphers && tls_config_set_ciphers(config, ciphers) == -1) err(1, "%s", tls_config_error(config)); - if (remote->tls_protocols && - (tls_config_parse_protocols(, remote->tls_protocols) == -1 - || tls_config_set_protocols(config, protos) == -1)) - err(1, "%s", tls_config_error(config)); + if (remote->tls_protocols) { + if (tls_config_parse_protocols(, + remote->tls_protocols) == -1) + err(1, "failed to parse protocols \"%s\"", + remote->tls_protocols); + if (tls_config_set_protocols(config, protos) == -1) + err(1, "%s", tls_config_error(config)); + } if (remote->pki) { pki = dict_get(env->sc_pki_dict, remote->pki); Index: parse.y === RCS file: /cvs/src/usr.sbin/smtpd/parse.y,v retrieving revision 1.286 diff -u -p -r1.286 parse.y --- parse.y 31 Mar 2021 17:47:16 - 1.286 +++ parse.y 6 Apr 2021 10:19:33 - @@ -137,6 +137,8 @@ static struct listen_opts { char *filtername; char *pki[PKI_MAX]; int pkicount; + char *tls_ciphers; + char *tls_protocols; char *ca; uint16_tauth; struct table *authtable; @@ -2333,6 +2335,20 @@ opt_if_listen : INET4 { listen_opts.options |= LO_SSL; listen_opts.ssl = F_STARTTLS|F_STARTTLS_REQUIRE|F_TLS_VERIFY; } + | CIPHERS STRING { + if (listen_opts.tls_ciphers) { + yyerror("ciphers already specified"); + YYERROR; + } + listen_opts.tls_ciphers = $2; + } + | PROTOCOLS STRING { + if (listen_opts.tls_protocols) { + yyerror("protocols already specified"); + YYERROR; + } + listen_opts.tls_protocols = $2; + } | PKI STRING{ if (listen_opts.pkicount == PKI_MAX) { yyerror("too many pki specified"); @@ -2516,7 +2532,11 @@ listen : LISTEN { memset(_opts, 0, sizeof listen_opts); listen_opts.family = AF_UNSPEC; listen_opts.flags |= F_EXT_DSN; - } ON listener_type + } ON listener_type { + free(listen_opts.tls_protocols); + free(listen_opts.tls_ciphers); + memset(_opts, 0, sizeof listen_opts); + } ; table : TABLE STRING STRING { @@ -3310,6 +3330,16 @@ config_listener(struct listener *h, str log_warnx("pki name not found: %s", lo->pki[i]); fatalx(NULL); } + } + + if (lo->tls_ciphers != NULL && + (h->tls_ciphers = strdup(lo->tls_ciphers)) == NULL) { + fatal("strdup"); + } + + if (lo->tls_protocols != NULL && + (h->tls_protocols = strdup(lo->tls_protocols)) == NULL) { + fatal("strdup"); } if (lo->ca != NULL) { Index: smtp.c === RCS file: /cvs/src/usr.sbin/smtpd/smtp.c,v retrieving revision 1.168 diff -u -p -r1.168 smtp.c --- smtp.c 10 Mar 2021 17:25:59 - 1.168 +++ smtp.c 6 Apr
smtpd: default mta ciphers
If not cipher list is specified for a relay rule, fallback to the global cipher list if defined, rather than libtls default. This is closer to the previous behavior. Eric. Index: mta.c === RCS file: /cvs/src/usr.sbin/smtpd/mta.c,v retrieving revision 1.236 diff -u -p -r1.236 mta.c --- mta.c 31 Mar 2021 17:47:16 - 1.236 +++ mta.c 31 Mar 2021 19:14:13 - @@ -491,6 +491,7 @@ mta_setup_dispatcher(struct dispatcher * struct tls_config *config; struct pki *pki; struct ca *ca; + const char *ciphers; uint32_t protos; if (dispatcher->type != DISPATCHER_REMOTE) @@ -501,8 +502,10 @@ mta_setup_dispatcher(struct dispatcher * if ((config = tls_config_new()) == NULL) fatal("smtpd: tls_config_new"); - if (remote->tls_ciphers && - tls_config_set_ciphers(config, remote->tls_ciphers) == -1) + ciphers = env->sc_tls_ciphers; + if (remote->tls_ciphers) + ciphers = remote->tls_ciphers; + if (ciphers && tls_config_set_ciphers(config, ciphers) == -1) err(1, "%s", tls_config_error(config)); if (remote->tls_protocols &&
Re: smtpd: trace and vfprintf %s NULL
Any objection or ok? On Sat, Mar 27, 2021 at 12:52:11PM +0100, Eric Faurot wrote: > Hello. > > I get reports from people seeing "vfprintf %s NULL" in their logs > recently. The problem is in a function that should be fixed, > but that function is only expected to but used for debug traces. > > This diff turns log_trace() into a macro, so the parameters > are not needlessly evaluated when tracing is not set. > > Eric. > > Index: smtpd.h > === > RCS file: /cvs/src/usr.sbin/smtpd/smtpd.h,v > retrieving revision 1.662 > diff -u -p -r1.662 smtpd.h > --- smtpd.h 5 Mar 2021 12:37:32 - 1.662 > +++ smtpd.h 23 Mar 2021 07:16:39 - > @@ -1747,8 +1747,9 @@ int base64_encode_rfc3548(unsigned char > char *, size_t); > > void log_trace_verbose(int); > -void log_trace(int, const char *, ...) > -__attribute__((format (printf, 2, 3))); > +void log_trace0(const char *, ...) > +__attribute__((format (printf, 1, 2))); > +#define log_trace(m, ...) do { if (tracing & (m)) log_trace0(__VA_ARGS__); > } while (0) > > /* waitq.c */ > int waitq_wait(void *, void (*)(void *, void *, void *), void *); > Index: util.c > === > RCS file: /cvs/src/usr.sbin/smtpd/util.c,v > retrieving revision 1.152 > diff -u -p -r1.152 util.c > --- util.c29 Nov 2020 20:07:38 - 1.152 > +++ util.c23 Mar 2021 07:17:05 - > @@ -823,15 +823,13 @@ base64_encode_rfc3548(unsigned char cons > } > > void > -log_trace(int mask, const char *emsg, ...) > +log_trace0(const char *emsg, ...) > { > va_list ap; > > - if (tracing & mask) { > - va_start(ap, emsg); > - vlog(LOG_DEBUG, emsg, ap); > - va_end(ap); > - } > + va_start(ap, emsg); > + vlog(LOG_DEBUG, emsg, ap); > + va_end(ap); > } > > void > >
smtpd: trace and vfprintf %s NULL
Hello. I get reports from people seeing "vfprintf %s NULL" in their logs recently. The problem is in a function that should be fixed, but that function is only expected to but used for debug traces. This diff turns log_trace() into a macro, so the parameters are not needlessly evaluated when tracing is not set. Eric. Index: smtpd.h === RCS file: /cvs/src/usr.sbin/smtpd/smtpd.h,v retrieving revision 1.662 diff -u -p -r1.662 smtpd.h --- smtpd.h 5 Mar 2021 12:37:32 - 1.662 +++ smtpd.h 23 Mar 2021 07:16:39 - @@ -1747,8 +1747,9 @@ int base64_encode_rfc3548(unsigned char char *, size_t); void log_trace_verbose(int); -void log_trace(int, const char *, ...) -__attribute__((format (printf, 2, 3))); +void log_trace0(const char *, ...) +__attribute__((format (printf, 1, 2))); +#define log_trace(m, ...) do { if (tracing & (m)) log_trace0(__VA_ARGS__); } while (0) /* waitq.c */ int waitq_wait(void *, void (*)(void *, void *, void *), void *); Index: util.c === RCS file: /cvs/src/usr.sbin/smtpd/util.c,v retrieving revision 1.152 diff -u -p -r1.152 util.c --- util.c 29 Nov 2020 20:07:38 - 1.152 +++ util.c 23 Mar 2021 07:17:05 - @@ -823,15 +823,13 @@ base64_encode_rfc3548(unsigned char cons } void -log_trace(int mask, const char *emsg, ...) +log_trace0(const char *emsg, ...) { va_list ap; - if (tracing & mask) { - va_start(ap, emsg); - vlog(LOG_DEBUG, emsg, ap); - va_end(ap); - } + va_start(ap, emsg); + vlog(LOG_DEBUG, emsg, ap); + va_end(ap); } void
smtpd: set protocols and ciphers
Hi. This diff allows to specify the protocol versions and ciphers to use for outgoing TLS sessions on a per relay basis. Eric. Index: mta.c === RCS file: /cvs/src/usr.sbin/smtpd/mta.c,v retrieving revision 1.235 diff -u -p -r1.235 mta.c --- mta.c 5 Mar 2021 12:37:32 - 1.235 +++ mta.c 25 Mar 2021 13:06:20 - @@ -491,6 +491,7 @@ mta_setup_dispatcher(struct dispatcher * struct tls_config *config; struct pki *pki; struct ca *ca; + uint32_t protos; if (dispatcher->type != DISPATCHER_REMOTE) return; @@ -500,10 +501,14 @@ mta_setup_dispatcher(struct dispatcher * if ((config = tls_config_new()) == NULL) fatal("smtpd: tls_config_new"); - if (env->sc_tls_ciphers) { - if (tls_config_set_ciphers(config, env->sc_tls_ciphers) == -1) - err(1, "%s", tls_config_error(config)); - } + if (remote->tls_ciphers && + tls_config_set_ciphers(config, remote->tls_ciphers) == -1) + err(1, "%s", tls_config_error(config)); + + if (remote->tls_protocols && + (tls_config_parse_protocols(, remote->tls_protocols) == -1 + || tls_config_set_protocols(config, protos) == -1)) + err(1, "%s", tls_config_error(config)); if (remote->pki) { pki = dict_get(env->sc_pki_dict, remote->pki); Index: parse.y === RCS file: /cvs/src/usr.sbin/smtpd/parse.y,v retrieving revision 1.285 diff -u -p -r1.285 parse.y --- parse.y 5 Mar 2021 12:37:32 - 1.285 +++ parse.y 25 Mar 2021 13:05:38 - @@ -190,7 +190,7 @@ typedef struct { %token MAIL_FROM MAILDIR MASK_SRC MASQUERADE MATCH MAX_MESSAGE_SIZE MAX_DEFERRED MBOX MDA MTA MX %token NO_DSN NO_VERIFY NOOP %token ON -%token PHASE PKI PORT PROC PROC_EXEC PROXY_V2 +%token PHASE PKI PORT PROC PROC_EXEC PROTOCOLS PROXY_V2 %token QUEUE QUIT %token RCPT_TO RDNS RECIPIENT RECEIVEDAUTH REGEX RELAY REJECT REPORT REWRITE RSET %token SCHEDULER SENDER SENDERS SMTP SMTP_IN SMTP_OUT SMTPS SOCKET SRC SRS SUB_ADDR_DELIM @@ -768,6 +768,22 @@ HELO STRING { dsp->u.remote.ca = $2; } +| CIPHERS STRING { + if (dsp->u.remote.tls_ciphers) { + yyerror("ciphers already specified for this dispatcher"); + YYERROR; + } + + dsp->u.remote.tls_ciphers = $2; +} +| PROTOCOLS STRING { + if (dsp->u.remote.tls_protocols) { + yyerror("protocols already specified for this dispatcher"); + YYERROR; + } + + dsp->u.remote.tls_protocols = $2; +} | SRC tables { struct table *t = $2; @@ -2682,6 +2698,7 @@ lookup(char *s) { "port", PORT }, { "proc", PROC }, { "proc-exec", PROC_EXEC }, + { "protocols", PROTOCOLS }, { "proxy-v2", PROXY_V2 }, { "queue", QUEUE }, { "quit", QUIT }, Index: smtpd.conf.5 === RCS file: /cvs/src/usr.sbin/smtpd/smtpd.conf.5,v retrieving revision 1.258 diff -u -p -r1.258 smtpd.conf.5 --- smtpd.conf.55 Mar 2021 12:37:32 - 1.258 +++ smtpd.conf.525 Mar 2021 17:47:00 - @@ -298,6 +298,18 @@ When used with a smarthost, the protocol If .Cm no-verify is specified, do not require a valid certificate. +.It Cm protocols Ar protostr +Define the protocol versions to be used for TLS sessions. +Refer to the +.Xr tls_config_parse_protocols 3 +manpage for the format of +.Ar protostr . +.It Cm ciphers Ar cipherstr +Define the list of ciphers that may be used for TLS sessions. +Refer to the +.Xr tls_config_set_ciphers 3 +manpage for the format of +.Ar cipherstr . .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.662 diff -u -p -r1.662 smtpd.h --- smtpd.h 5 Mar 2021 12:37:32 - 1.662 +++ smtpd.h 25 Mar 2021 13:06:36 - @@ -1192,6 +1192,8 @@ struct dispatcher_remote { char*auth; int tls_required; int tls_noverify; + char*tls_protocols; + char*tls_ciphers; int backup; char*backupmx;
smtpd: use mx name for sni
As spotted by krw@, the mta should use the mx hostname for sni, not the reverse dns for the peer address. Eric. Index: mta_session.c === RCS file: /cvs/src/usr.sbin/smtpd/mta_session.c,v retrieving revision 1.139 diff -u -p -r1.139 mta_session.c --- mta_session.c 5 Mar 2021 12:37:32 - 1.139 +++ mta_session.c 7 Mar 2021 20:18:42 - @@ -1596,7 +1596,7 @@ mta_tls_init(struct mta_session *s) return; } - io_connect_tls(s->io, tls, s->route->dst->ptrname); + io_connect_tls(s->io, tls, s->mxname); } static void
Re: smtpd: use libtls
Hi. The diff seems to work for the few people who tested it (thanks). Anyone wants to ok this? 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.c31 Dec 2020 08:27:15 - 1.37 +++ ca.c19 Jan 2021 11:09:54 - @@ -69,6 +69,7 @@ static int ecdsae_do_verify(const unsign EC_KEY *); +static struct dict pkeys; static uint64_t reqid = 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) return (0); /* @@ -365,7 +368,7 @@ rsae_send_imsg(int flen, const unsigned m_create(p_ca, cmd, 0, 0, -1); reqid++; m_add_id(p_ca, reqid); -
Re: smtpd: use libtls
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.c31 Dec 2020 08:27:15 - 1.37 +++ ca.c19 Jan 2021 11:09:54 - @@ -69,6 +69,7 @@ static int ecdsae_do_verify(const unsign EC_KEY *); +static struct dict pkeys; static uint64_t reqid = 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) return (0); /* @@ -365,7 +368,7 @@ rsae_send_imsg(int flen, const unsigned m_create(p_ca, cmd, 0, 0, -1); reqid++;
smtpd: use libtls
There has been a plan for some time now to make smtpd use libtls instead of openssl. Recent changes in libtls allow to move forward with this. Here is a diff to start the switch. I've tried to keep it as small as possible, sticking to the necessary changes. There is still a lot of code that can be removed but that will be done in a second time. There are two visible changes that the user must be aware of. First, certificate verification is now delegated to libtls, so peer certificate, if provided, can only be reported as either "valid" or "unverified" (if no-verify is requested). In other words, invalid certificates can no longer (at least for now) be detected but still accepted. Secondly, the way SNI is configured has changed: the "pki" listener option can now be used multiple times to add extra certificates on a listener. Each tls-enabled listener must have at least one certificate explicitly set. The extra ones are used for SNI, and only the specified certificates will be used for this listener. So there is no more fallback or global lookup on configured pki names. Tests and comments would be greatly appreciated. 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.c31 Dec 2020 08:27:15 - 1.37 +++ ca.c19 Jan 2021 11:09:54 - @@ -69,6 +69,7 @@ static int ecdsae_do_verify(const unsign EC_KEY *); +static struct dict pkeys; static uint64_t reqid = 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 || -
Re: smtp(1) add authentication
On Sun, Dec 20, 2020 at 07:58:56PM +0100, Martijn van Duren wrote: > Playing around with the filter API I want an easier way to send mail > with authentication instead of doing the transaction manually via > openssl or via bloated mailclients. Turns out we already have all the > plumbing in place and just need to hook it up. > > OK? ok eric@ > martijn@ > > Index: smtpc.c > === > RCS file: /cvs/src/usr.sbin/smtpd/smtpc.c,v > retrieving revision 1.11 > diff -u -p -r1.11 smtpc.c > --- smtpc.c 14 Sep 2020 18:32:11 - 1.11 > +++ smtpc.c 20 Dec 2020 18:57:13 - > @@ -56,9 +56,8 @@ usage(void) > { > extern char *__progname; > > - fprintf(stderr, > - "usage: %s [-Chnv] [-F from] [-H helo] [-s server] [-S name] rcpt > ...\n", > - __progname); > + fprintf(stderr, "usage: %s [-Chnv] [-F from] [-H helo] [-a authfile] " > + "[-s server] [-S name] rcpt ...\n", __progname); > exit(1); > } > > @@ -66,8 +65,12 @@ int > main(int argc, char **argv) > { > char hostname[256]; > + FILE *authfile; > int ch, i; > char *server = "localhost"; > + char *authstr = NULL; > + size_t alloc = 0; > + ssize_t len; > struct passwd *pw; > > log_init(1, 0); > @@ -91,7 +94,7 @@ main(int argc, char **argv) > memset(, 0, sizeof(mail)); > mail.from = pw->pw_name; > > - while ((ch = getopt(argc, argv, "CF:H:S:hns:v")) != -1) { > + while ((ch = getopt(argc, argv, "CF:H:S:a:hns:v")) != -1) { > switch (ch) { > case 'C': > params.tls_verify = 0; > @@ -107,6 +110,23 @@ main(int argc, char **argv) > break; > case 'h': > usage(); > + break; > + case 'a': > + if ((authfile = fopen(optarg, "r")) == NULL) > + fatal("%s: open", optarg); > + if ((len = getline(, , authfile)) == -1) > + fatal("%s: Failed to read username", optarg); > + if (authstr[len - 1] == '\n') > + authstr[len - 1] = '\0'; > + params.auth_user = authstr; > + authstr = NULL; > + len = 0; > + if ((len = getline(, , authfile)) == -1) > + fatal("%s: Failed to read password", optarg); > + if (authstr[len - 1] == '\n') > + authstr[len - 1] = '\0'; > + params.auth_pass = authstr; > + fclose(authfile); > break; > case 'n': > noaction = 1; > Index: smtp.1 > === > RCS file: /cvs/src/usr.sbin/smtpd/smtp.1,v > retrieving revision 1.7 > diff -u -p -r1.7 smtp.1 > --- smtp.14 Jul 2018 08:23:43 - 1.7 > +++ smtp.120 Dec 2020 18:57:13 - > @@ -25,6 +25,7 @@ > .Op Fl Chnv > .Op Fl F Ar from > .Op Fl H Ar helo > +.Op Fl a Ar authfile > .Op Fl s Ar server > .Op Ar recipient ... > .Sh DESCRIPTION > @@ -49,6 +50,13 @@ Set the return-path (MAIL FROM) for the > Default to the current username. > .It Fl H Ar helo > Define the hostname to advertise (HELO) when establishing the SMTP session. > +.It Fl a Ar authfile > +Perform a login before sending the message. > +The username and password are read from > +.Ar authfile > +and need to be on the first and second line respectively. > +This option requires a TLS or STARTTLS > +.Ar server . > .It Fl h > Display version and usage. > .It Fl n > > >
smtpd: simplify codepath
When creating a tls session on an incoming connection, a useless roundtrip through the lka is made (see cert.c) to retreive a certificate which is not used anyway: the necessary contexts have already been set up for all pki names (in smtp.c:smtp_setup_events()). This diff makes the codepath more straight-forward and should not change the current behaviour. Please test and report if you are using server-side tls. Eric. Index: smtp_session.c === RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v retrieving revision 1.426 diff -u -p -r1.426 smtp_session.c --- smtp_session.c 24 Apr 2020 11:34:07 - 1.426 +++ smtp_session.c 14 Oct 2020 17:12:05 - @@ -134,7 +134,6 @@ struct smtp_session { uint64_t id; struct io *io; struct listener *listener; - void*ssl_ctx; struct sockaddr_storage ss; char rdns[HOST_NAME_MAX+1]; char smtpname[HOST_NAME_MAX+1]; @@ -193,8 +192,7 @@ static void smtp_rfc4954_auth_plain(stru static void smtp_rfc4954_auth_login(struct smtp_session *, char *); static void smtp_free(struct smtp_session *, const char *); static const char *smtp_strstate(int); -static void smtp_cert_init(struct smtp_session *); -static void smtp_cert_init_cb(void *, int, const char *, const void *, size_t); +static void smtp_tls_init(struct smtp_session *); static void smtp_cert_verify(struct smtp_session *); static void smtp_cert_verify_cb(void *, int); static void smtp_auth_failure_pause(struct smtp_session *); @@ -306,7 +304,6 @@ static struct tree wait_parent_auth; static struct tree wait_queue_msg; static struct tree wait_queue_fd; static struct tree wait_queue_commit; -static struct tree wait_ssl_init; static struct tree wait_ssl_verify; static struct tree wait_filters; static struct tree wait_filter_fd; @@ -587,7 +584,6 @@ smtp_session_init(void) tree_init(_queue_msg); tree_init(_queue_fd); tree_init(_queue_commit); - tree_init(_ssl_init); tree_init(_ssl_verify); tree_init(_filters); tree_init(_filter_fd); @@ -1193,7 +1189,7 @@ smtp_io(struct io *io, int evt, void *ar /* Wait for the client to start tls */ if (s->state == STATE_TLS) { - smtp_cert_init(s); + smtp_tls_init(s); break; } @@ -2071,7 +2067,7 @@ static void smtp_proceed_connected(struct smtp_session *s) { if (s->listener->flags & F_SMTPS) - smtp_cert_init(s); + smtp_tls_init(s); else smtp_send_banner(s); } @@ -2261,34 +2257,20 @@ smtp_mailaddr(struct mailaddr *maddr, ch } static void -smtp_cert_init(struct smtp_session *s) +smtp_tls_init(struct smtp_session *s) { - const char *name; - int fallback; + void *ssl_ctx; + void *ssl; - if (s->listener->pki_name[0]) { - name = s->listener->pki_name; - fallback = 0; - } + if (s->listener->pki_name[0]) + ssl_ctx = dict_get(env->sc_ssl_dict, s->listener->pki_name); else { - name = s->smtpname; - fallback = 1; + ssl_ctx = dict_get(env->sc_ssl_dict, s->smtpname); + if (ssl_ctx == NULL) + ssl_ctx = dict_get(env->sc_ssl_dict, "*"); } - if (cert_init(name, fallback, smtp_cert_init_cb, s)) - tree_xset(_ssl_init, s->id, s); -} - -static void -smtp_cert_init_cb(void *arg, int status, const char *name, const void *cert, -size_t cert_len) -{ - struct smtp_session *s = arg; - void *ssl, *ssl_ctx; - - tree_pop(_ssl_init, s->id); - - if (status == CA_FAIL) { + if (ssl_ctx == NULL) { log_info("%016"PRIx64" smtp disconnected " "reason=ca-failure", s->id); @@ -2296,7 +2278,6 @@ smtp_cert_init_cb(void *arg, int status, return; } - ssl_ctx = dict_get(env->sc_ssl_dict, name); ssl = ssl_smtp_init(ssl_ctx, s->listener->flags & F_TLS_VERIFY); io_set_read(s->io); io_start_tls(s->io, ssl);
smtpd: fix catch-all in virtual aliases
Hi. When a catch-all entry (@) is used in a virtual alias table, it eventually (and mistakenly) catches everything that expands to a username. For example, with: f...@example.com user @catchall "f...@example.com" expands to "user" as expected, but then "user" expands to "catchall" because it is interpreted as "user@" (empty domain). The catch-all fallback mechanism is really meant for full email addresses in virtual context, and should not happen for usernames. The following diff fixes it. Eric. Index: aliases.c === RCS file: /cvs/src/usr.sbin/smtpd/aliases.c,v retrieving revision 1.77 diff -u -p -r1.77 aliases.c --- aliases.c 28 Dec 2018 12:47:28 - 1.77 +++ aliases.c 26 Apr 2020 16:04:51 - @@ -164,6 +164,10 @@ aliases_virtual_get(struct expand *expan if (ret) goto expand; + /* Do not try catch-all entries if there is no domain */ + if (domain[0] == '\0') + return 0; + if (!bsnprintf(buf, sizeof(buf), "@%s", domain)) return 0; /* Failed ? We lookup for catch all for virtual domain */
smtpd: simplify aliases_get()
Hi, The current code for aliases_get() is a bit contorted I think. This diff makes it clearer. Eric. Index: aliases.c === RCS file: /cvs/src/usr.sbin/smtpd/aliases.c,v retrieving revision 1.77 diff -u -p -r1.77 aliases.c --- aliases.c 28 Dec 2018 12:47:28 - 1.77 +++ aliases.c 25 Apr 2020 11:35:08 - @@ -53,23 +53,18 @@ aliases_get(struct expand *expand, const xlowercase(buf, username, sizeof(buf)); - /* first, check if entry has a user-part tag */ - pbuf = strchr(buf, *env->sc_subaddressing_delim); - if (pbuf) { - ret = table_lookup(mapping, K_ALIAS, buf, ); - if (ret < 0) - return (-1); - if (ret) - goto expand; + /* First, check the username as given. */ + ret = table_lookup(mapping, K_ALIAS, buf, ); + if (ret == 0 && (pbuf = strchr(buf, *env->sc_subaddressing_delim))) { + /* Not found, retry without the tag if there is one. */ *pbuf = '\0'; + ret = table_lookup(mapping, K_ALIAS, buf, ); } - /* no user-part tag, try looking up user */ - ret = table_lookup(mapping, K_ALIAS, buf, ); + /* Not found or error */ if (ret <= 0) return ret; -expand: /* foreach node in table_alias expandtree, we merge */ nbaliases = 0; RB_FOREACH(xn, expandtree, >tree) {
Re: resolv.conf(5) says options inet6 does nothing
On Thu, Apr 23, 2020 at 10:34:39AM -0600, Theo de Raadt wrote: > It says the keyword gets parsed, but then does performs no action. > > But that is different from not parsing it. > > Additionally, this explains an option which other systems support, and > by explaining it this way, it is also explaining our behaviour in case > of inet6 vs inet4 conditions. > > So... I think it should stay. Eric, do you have an opinion? The doc lies because the inet6 option does not set the RES_USE_INET6 flag as stated. I think we should leave the entry in the doc but fix the wording to say it's there for historical reasons and does nothing. If we want to resurrect that option at some point, maybe we can consider making it set the RES_USE_INET6 flag but that feels like a bad idea right now, and this flag is apparently deprecated. But we should also fix the manpage for res_init(3) as the description of the flag is wrong too. Eric.
Re: smtpd: trailing CR
On Tue, Apr 21, 2020 at 07:08:48AM +, gil...@poolp.org wrote: > April 21, 2020 4:28 AM, "Todd C. Miller" wrote: > > > On Mon, 20 Apr 2020 15:01:31 +0200, Eric Faurot wrote: > > > >> There has been a discussion a while ago about the issue of trailing CR > >> in smtp lines (https://marc.info/?l=openbsd-tech=156890805128121=2). > >> > >> It is agreed that stripping an optional CR at io level is a problem > >> because the information is lost and there is no way to take a specific > >> action at the protocol level if needed. > >> > >> So this diffs moves the CR stripping from io level to protocol level for > >> SMTP dialogs. Other uses of io_getline() are internal and expect simple LF > >> line ending. The current behavior should not change. > > > > This looks OK to me but I would feel better if we had successful > > reports from other people too. > > > > Have been running with eric's diff since yesterday afternoon, works for me No issue reported so far. I think it can go in. Eric.
smtpd: fix smtpctl discover
Hi again, We had a report of a crash when running "smtpctl discover" on an envelope that has a invalid dispatcher (action name changed in the config). The issue is that the dispatcher is not checked after the envelope is loaded as a result of the discover command. But adding the check afterwards is not enough, because the envelope is already in the envelope cache, so this makes the whole live update/discover process actually useless (we would have to invalidate the cache). So the right fix, which also simplifies the code (always a good sign), is to make that check along with the other envelope validation checks that happen at envelope load time. Eric. Index: queue.c === RCS file: /cvs/src/usr.sbin/smtpd/queue.c,v retrieving revision 1.189 diff -u -p -r1.189 queue.c --- queue.c 30 Dec 2018 23:09:58 - 1.189 +++ queue.c 20 Apr 2020 18:43:17 - @@ -686,7 +686,6 @@ static void queue_timeout(int fd, short event, void *p) { static uint32_t msgid = 0; - struct dispatcher *dsp; struct envelope evp; struct event*ev = p; struct timeval tv; @@ -705,13 +704,6 @@ queue_timeout(int fd, short event, void } if (r) { - dsp = dict_get(env->sc_dispatchers, evp.dispatcher); - if (dsp == NULL) { - log_warnx("warn: queue: missing dispatcher \"%s\"" - " for envelope %016"PRIx64", ignoring", - evp.dispatcher, evp.id); - goto reset; - } if (msgid && evpid_to_msgid(evp.id) != msgid) { m_create(p_scheduler, IMSG_QUEUE_MESSAGE_COMMIT, 0, 0, -1); @@ -724,7 +716,6 @@ queue_timeout(int fd, short event, void m_close(p_scheduler); } -reset: tv.tv_sec = 0; tv.tv_usec = 10; evtimer_add(ev, ); Index: queue_backend.c === RCS file: /cvs/src/usr.sbin/smtpd/queue_backend.c,v retrieving revision 1.65 diff -u -p -r1.65 queue_backend.c --- queue_backend.c 30 Dec 2018 23:09:58 - 1.65 +++ queue_backend.c 20 Apr 2020 18:42:38 - @@ -730,6 +730,9 @@ envelope_validate(struct envelope *ep) if (memchr(ep->errorline, '\0', sizeof(ep->errorline)) == NULL) return "invalid error line"; + if (dict_get(env->sc_dispatchers, ep->dispatcher) == NULL) + return "unknown dispatcher"; + return NULL; }
smtpd: trailing CR
Hi, There has been a discussion a while ago about the issue of trailing CR in smtp lines (https://marc.info/?l=openbsd-tech=156890805128121=2). It is agreed that stripping an optional CR at io level is a problem because the information is lost and there is no way to take a specific action at the protocol level if needed. So this diffs moves the CR stripping from io level to protocol level for SMTP dialogs. Other uses of io_getline() are internal and expect simple LF line ending. The current behavior should not change. Comments? Eric. Index: bounce.c === RCS file: /cvs/src/usr.sbin/smtpd/bounce.c,v retrieving revision 1.81 diff -u -p -r1.81 bounce.c --- bounce.c25 Nov 2019 12:11:26 - 1.81 +++ bounce.c20 Apr 2020 08:06:43 - @@ -728,6 +728,10 @@ bounce_io(struct io *io, int evt, void * if (line == NULL) break; + /* Strip trailing '\r' */ + if (len && line[len - 1] == '\r') + line[--len] = '\0'; + log_trace(TRACE_BOUNCE, "bounce: %p: <<< %s", s, line); if ((error = parse_smtp_response(line, len, , ))) { Index: iobuf.c === RCS file: /cvs/src/usr.sbin/smtpd/iobuf.c,v retrieving revision 1.12 diff -u -p -r1.12 iobuf.c --- iobuf.c 3 Oct 2019 07:03:23 - 1.12 +++ iobuf.c 18 Mar 2020 20:25:41 - @@ -174,10 +174,9 @@ iobuf_getline(struct iobuf *iobuf, size_ * the next call to iobuf_normalize() or iobuf_extend(). */ iobuf_drop(iobuf, i + 1); - len = (i && buf[i - 1] == '\r') ? i - 1 : i; - buf[len] = '\0'; + buf[i] = '\0'; if (rlen) - *rlen = len; + *rlen = i; return (buf); } Index: lka_filter.c === RCS file: /cvs/src/usr.sbin/smtpd/lka_filter.c,v retrieving revision 1.61 diff -u -p -r1.61 lka_filter.c --- lka_filter.c17 Apr 2020 14:20:13 - 1.61 +++ lka_filter.c19 Apr 2020 18:56:57 - @@ -851,7 +851,7 @@ filter_data_internal(struct filter_sessi /* 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; } Index: mta_session.c === RCS file: /cvs/src/usr.sbin/smtpd/mta_session.c,v retrieving revision 1.134 diff -u -p -r1.134 mta_session.c --- mta_session.c 10 Apr 2020 19:28:57 - 1.134 +++ mta_session.c 19 Apr 2020 10:25:55 - @@ -1257,6 +1257,10 @@ mta_io(struct io *io, int evt, void *arg return; } + /* Strip trailing '\r' */ + if (len && line[len - 1] == '\r') + line[--len] = '\0'; + log_trace(TRACE_MTA, "mta: %p: <<< %s", s, line); mta_report_protocol_server(s, line); Index: smtp_client.c === RCS file: /cvs/src/usr.sbin/smtpd/smtp_client.c,v retrieving revision 1.13 diff -u -p -r1.13 smtp_client.c --- smtp_client.c 24 Feb 2020 23:54:27 - 1.13 +++ smtp_client.c 19 Apr 2020 10:26:25 - @@ -680,6 +680,10 @@ smtp_client_readline(struct smtp_client return 0; } + /* Strip trailing '\r' */ + if (len && line[len - 1] == '\r') + line[--len] = '\0'; + log_trace(TRACE_SMTPCLT, "%p: <<< %s", proto, line); /* Validate SMTP */ Index: smtp_session.c === RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v retrieving revision 1.425 diff -u -p -r1.425 smtp_session.c --- smtp_session.c 15 Mar 2020 16:34:57 - 1.425 +++ smtp_session.c 20 Apr 2020 08:06:35 - @@ -1132,6 +1132,10 @@ smtp_io(struct io *io, int evt, void *ar if (line == NULL) return; + /* Strip trailing '\r' */ + if (len && line[len - 1] == '\r') + line[--len] = '\0'; + /* Message body */ eom = 0; if (s->state == STATE_BODY) {
smtpd: fix report event format
Some users had issues with report events for MAIL FROM and RCPT TO when "|" appear in the mail address (yes, it seems to happen), because that's also the field separator. To make parsing the report lines a bit more straightforward, it's better to put the address as the last field. Note that this is a protocol change, so external filters will have to be updated. Eric. Index: lka_filter.c === RCS file: /cvs/src/usr.sbin/smtpd/lka_filter.c,v retrieving revision 1.60 diff -u -p -r1.60 lka_filter.c --- lka_filter.c8 Jan 2020 01:41:11 - 1.60 +++ lka_filter.c4 Apr 2020 08:39:19 - @@ -35,7 +35,7 @@ #include "smtpd.h" #include "log.h" -#definePROTOCOL_VERSION"0.5" +#definePROTOCOL_VERSION"0.6" struct filter; struct filter_session; @@ -1526,7 +1526,7 @@ lka_report_smtp_tx_mail(const char *dire break; } report_smtp_broadcast(reqid, direction, tv, "tx-mail", "%08x|%s|%s\n", - msgid, address, result); + msgid, result, address); } void @@ -1546,7 +1546,7 @@ lka_report_smtp_tx_rcpt(const char *dire break; } report_smtp_broadcast(reqid, direction, tv, "tx-rcpt", "%08x|%s|%s\n", - msgid, address, result); + msgid, result, address); } void
smtpd: use CRLF in local enqueuer
This diff makes the local enqueuer use CRLF line ending during the SMTP dialog, as required by the protocol. Eric. Index: enqueue.c === RCS file: /cvs/src/usr.sbin/smtpd/enqueue.c,v retrieving revision 1.117 diff -u -p -r1.117 enqueue.c --- enqueue.c 8 Mar 2020 21:47:05 - 1.117 +++ enqueue.c 9 Mar 2020 13:15:59 - @@ -136,7 +136,7 @@ struct { #define QP_TEST_WRAP(fp, buf, linelen, size) do {\ if (((linelen) += (size)) + 1 > 76) { \ - fprintf((fp), "=\n"); \ + fprintf((fp), "=\r\n"); \ if (buf[0] == '.') \ fprintf((fp), "."); \ (linelen) = (size); \ @@ -178,7 +178,7 @@ qp_encoded_write(FILE *fp, char *buf) fprintf(fp, "%c", *buf); } } - fprintf(fp, "\n"); + fprintf(fp, "\r\n"); } int @@ -325,7 +325,7 @@ enqueue(int argc, char *argv[], FILE *of if (!get_responses(fout, 1)) goto fail; - if (!send_line(fout, verbose, "EHLO localhost\n")) + if (!send_line(fout, verbose, "EHLO localhost\r\n")) goto fail; if (!get_responses(fout, 1)) goto fail; @@ -333,7 +333,7 @@ enqueue(int argc, char *argv[], FILE *of if (msg.dsn_envid != NULL) envid_sz = strlen(msg.dsn_envid); - if (!send_line(fout, verbose, "MAIL FROM:<%s> %s%s %s%s\n", + if (!send_line(fout, verbose, "MAIL FROM:<%s> %s%s %s%s\r\n", msg.from, msg.dsn_ret ? "RET=" : "", msg.dsn_ret ? msg.dsn_ret : "", @@ -344,7 +344,7 @@ enqueue(int argc, char *argv[], FILE *of goto fail; for (i = 0; i < msg.rcpt_cnt; i++) { - if (!send_line(fout, verbose, "RCPT TO:<%s> %s%s\n", + if (!send_line(fout, verbose, "RCPT TO:<%s> %s%s\r\n", msg.rcpts[i], msg.dsn_notify ? "NOTIFY=" : "", msg.dsn_notify ? msg.dsn_notify : "")) @@ -353,41 +353,41 @@ enqueue(int argc, char *argv[], FILE *of goto fail; } - if (!send_line(fout, verbose, "DATA\n")) + if (!send_line(fout, verbose, "DATA\r\n")) goto fail; if (!get_responses(fout, 1)) goto fail; /* add From */ - if (!msg.saw_from && !send_line(fout, 0, "From: %s%s<%s>\n", + if (!msg.saw_from && !send_line(fout, 0, "From: %s%s<%s>\r\n", msg.fromname ? msg.fromname : "", msg.fromname ? " " : "", msg.from)) goto fail; /* add Date */ - if (!msg.saw_date && !send_line(fout, 0, "Date: %s\n", + if (!msg.saw_date && !send_line(fout, 0, "Date: %s\r\n", time_to_text(timestamp))) goto fail; if (msg.need_linesplit) { /* we will always need to mime encode for long lines */ if (!msg.saw_mime_version && !send_line(fout, 0, - "MIME-Version: 1.0\n")) + "MIME-Version: 1.0\r\n")) goto fail; if (!msg.saw_content_type && !send_line(fout, 0, - "Content-Type: text/plain; charset=unknown-8bit\n")) + "Content-Type: text/plain; charset=unknown-8bit\r\n")) goto fail; if (!msg.saw_content_disposition && !send_line(fout, 0, - "Content-Disposition: inline\n")) + "Content-Disposition: inline\r\n")) goto fail; if (!msg.saw_content_transfer_encoding && !send_line(fout, 0, - "Content-Transfer-Encoding: quoted-printable\n")) + "Content-Transfer-Encoding: quoted-printable\r\n")) goto fail; } /* add separating newline */ if (msg.noheader) { - if (!send_line(fout, 0, "\n")) + if (!send_line(fout, 0, "\r\n")) goto fail; inheaders = 0; } @@ -420,7 +420,7 @@ enqueue(int argc, char *argv[], FILE *of if (msg.saw_content_transfer_encoding || msg.noheader || inheaders || !msg.need_linesplit) { - if (!send_line(fout, 0, "%.*s", (int)len, line)) + if (!send_line(fout, 0, "%.*s\r\n", (int)len - 1, line)) goto fail; if (inheaders && buf[0] == '\n') inheaders = 0; @@ -431,12 +431,12 @@ enqueue(int argc, char *argv[], FILE *of qp_encoded_write(fout, line); }
Re: smtpd: add support for cidr in hostname resolution for spf walk
On Tue, Nov 12, 2019 at 02:16:25PM +0100, Quentin Rameau wrote: Hello > Hello, > > Here's a patch for smtpctl spf resolution, adding support for target > specified as a hostname + cidr. > > Yes, SPF lets you specify targets like a:example.com/24. > > Due to the async and recursive nature of DNS resolution in spfwalk.c, > it's kind of hard to pass data around without too much memory > allocation, so I decided to just pass cidr(s) as integer value instead > of keeping the original string and passing pointers around. > > Comments welcome, thanks! I find it confusing that everything is stuffed into the sender struct, including the callback. I don't really see the point. If you don't need to pass the target value over to the callback, there is no need to have it in the struct either, especially since the pointer is only valid at the time you call the lookup function. Another point, I don't understand how the parse_sender() function is supposed to work. Can you give examples with cidr4 and cidr6? Eric. > > Index: usr.sbin/smtpd/spfwalk.c > === > RCS file: /var/cvs/src/usr.sbin/smtpd/spfwalk.c,v > retrieving revision 1.15 > diff -u -r1.15 spfwalk.c > --- usr.sbin/smtpd/spfwalk.c 11 Nov 2019 17:20:25 - 1.15 > +++ usr.sbin/smtpd/spfwalk.c 12 Nov 2019 12:43:25 - > @@ -40,15 +40,24 @@ > #include "unpack_dns.h" > #include "parser.h" > > +struct sender { > + void(*cb)(struct dns_rr *, struct sender *); > + char *target; > + int cidr4; > + int cidr6; > +}; > + > +void *xmalloc(size_t); > int spfwalk(int, struct parameter *); > > -static void dispatch_txt(struct dns_rr *); > -static void dispatch_mx(struct dns_rr *); > -static void dispatch_a(struct dns_rr *); > -static void dispatch_(struct dns_rr *); > -static void lookup_record(int, const char *, void (*)(struct dns_rr *)); > +static void dispatch_txt(struct dns_rr *, struct sender *); > +static void dispatch_mx(struct dns_rr *, struct sender *); > +static void dispatch_a(struct dns_rr *, struct sender *); > +static void dispatch_(struct dns_rr *, struct sender *); > +static void lookup_record(int, struct sender *); > static void dispatch_record(struct asr_result *, void *); > static ssize_t parse_txt(const char *, size_t, char *, size_t); > +static int parse_sender(struct sender *); > > int ip_v4 = 0; > int ip_v6 = 0; > @@ -59,6 +68,7 @@ > int > spfwalk(int argc, struct parameter *argv) > { > + struct sendersdr; > const char *ip_family = NULL; > char*line = NULL; > size_t linesize = 0; > @@ -81,12 +91,17 @@ > dict_init(); > event_init(); > > + sdr.cidr4 = sdr.cidr6 = -1; > + sdr.cb = dispatch_txt; > + > while ((linelen = getline(, , stdin)) != -1) { > while (linelen-- > 0 && isspace(line[linelen])) > line[linelen] = '\0'; > > - if (linelen > 0) > - lookup_record(T_TXT, line, dispatch_txt); > + if (linelen > 0) { > + sdr.target = line; > + lookup_record(T_TXT, ); > + } > } > > free(line); > @@ -100,20 +115,23 @@ > } > > void > -lookup_record(int type, const char *record, void (*cb)(struct dns_rr *)) > +lookup_record(int type, struct sender *sdr) > { > struct asr_query *as; > + struct sender *nsdr; > > - as = res_query_async(record, C_IN, type, NULL); > + as = res_query_async(sdr->target, C_IN, type, NULL); > if (as == NULL) > err(1, "res_query_async"); > - event_asr_run(as, dispatch_record, cb); > + nsdr = xmalloc(sizeof(*nsdr)); > + *nsdr = *sdr; > + event_asr_run(as, dispatch_record, (void *)nsdr); > } > > void > dispatch_record(struct asr_result *ar, void *arg) > { > - void (*cb)(struct dns_rr *) = arg; > + struct sender *sdr = arg; > struct unpack pack; > struct dns_header h; > struct dns_query q; > @@ -121,7 +139,7 @@ > > /* best effort */ > if (ar->ar_h_errno && ar->ar_h_errno != NO_DATA) > - return; > + goto end; > > unpack_init(, ar->ar_data, ar->ar_datalen); > unpack_header(, ); > @@ -130,19 +148,22 @@ > for (; h.ancount; h.ancount--) { > unpack_rr(, ); > /**/ > - cb(); > + sdr->cb(, sdr); > } > +end: > + free(sdr); > } > > void > -dispatch_txt(struct dns_rr *rr) > +dispatch_txt(struct dns_rr *rr, struct sender *sdr) > { > + char buf[4096]; > + char *argv[512]; > + char buf2[512]; > + struct sender lsdr; > struct in6_addr ina; > -char buf[4096]; > -char buf2[512]; > -char *in = buf; > -char *argv[512]; > -char **ap = argv; > + char **ap = argv; > + char *in = buf; > char
Re: smtpd handling of \r in DATA part
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;
Re: smtpd handling of \r in DATA part
On Thu, Sep 19, 2019 at 05:46:47PM +0200, Gilles Chehade wrote: > Hello, > > The RFC for SMTP states the following (section 2.3.8): > > In addition, the appearance of "bare" "CR" or "LF" characters in text > (i.e., either without the other) has a long history of causing > problems in mail implementations and applications that use the mail > system as a tool. SMTP client implementations MUST NOT transmit > these characters except when they are intended as line terminators > and then MUST, as indicated above, transmit them only as a > sequence. > > > As a result, OpenSMTPD rejects DATA containing with the following: > > 500 5.0.0 is only allowed before > > requiring that clients encode DATA if is part of it. > > My question is: are we too strict ? > > Not two MTA do the same thing. Some will leave '\r' in the body and then > write it to the user mailbox or relay it. Other change it into a '\n' or > skip it. The first ones take the risk of a MUA not handling '\r' well or > an MTA rejecting later, the second ones break DKIM-signatures. > > The only good way to deal with this is to stick to the RFC ... BUT users > then experience message rejections when using broken clients (semarie@'s > printer is an example of one). > > So: > > a- do we leave '\r' in the body ? > b- do we turn '\r' into '\n' I don't think it's a good solution. What happens if there is a dot right after the '\r'? > c- do we keep strict behavior ? > d- do we keep strict behavior + provide a knob for '\r' to work ? I'm not sure the RFC actually requires the server to reject mails with "bare" '\r'. It just says the client must not transmit them. So maybe we should just discard them at receive time... 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. Eric.
Re: smtp(1) certificate validation
On Tue, Sep 10, 2019 at 06:12:12PM +0100, Stuart Henderson wrote: > > + if (!SSL_CTX_load_verify_locations(ssl_ctx, "/etc/ssl/cert.pem", NULL)) > > shouldn't that use X509_get_default_cert_file()? Yes, that looks better. Updated locally. Eric.
Re: smtp(1) certificate validation
On Fri, Sep 06, 2019 at 08:41:21AM +0200, Eric Faurot wrote: > Hi, > > This patch adds the missing bits for verifying the server certificate > in smtp(1). Take two: now check the name(s) of the server certificate. I borrowed code from libtls for now. This will be cleaned up when the daemon is ported to libtls. Eric. Index: smtpc.c === RCS file: /cvs/src/usr.sbin/smtpd/smtpc.c,v retrieving revision 1.8 diff -u -p -r1.8 smtpc.c --- smtpc.c 2 Sep 2019 20:05:21 - 1.8 +++ smtpc.c 10 Sep 2019 14:40:25 - @@ -20,6 +20,7 @@ #include #include +#include #include #include #include @@ -30,12 +31,12 @@ #include #include +#include + #include "smtp.h" +#include "ssl.h" #include "log.h" -void ssl_init(void); -void *ssl_mta_init(void *, char *, off_t, const char *); - static void parse_server(char *); static void parse_message(FILE *); static void resume(void); @@ -46,6 +47,9 @@ static int noaction = 0; static struct addrinfo *res0, *ai; static struct smtp_params params; static struct smtp_mail mail; +static const char *servname = NULL; + +static SSL_CTX *ssl_ctx; static void usage(void) @@ -53,7 +57,7 @@ usage(void) extern char *__progname; fprintf(stderr, - "usage: %s [-Chnv] [-F from] [-H helo] [-s server] rcpt ...\n", + "usage: %s [-Chnv] [-F from] [-H helo] [-s server] [-S name] rcpt ...\n", __progname); exit(1); } @@ -87,7 +91,7 @@ main(int argc, char **argv) memset(, 0, sizeof(mail)); mail.from = pw->pw_name; - while ((ch = getopt(argc, argv, "CF:H:hns:v")) != -1) { + while ((ch = getopt(argc, argv, "CF:H:S:hns:v")) != -1) { switch (ch) { case 'C': params.tls_verify = 0; @@ -98,6 +102,9 @@ main(int argc, char **argv) case 'H': params.helo = optarg; break; + case 'S': + servname = optarg; + break; case 'h': usage(); break; @@ -132,6 +139,13 @@ main(int argc, char **argv) ssl_init(); event_init(); + ssl_ctx = ssl_ctx_create(NULL, NULL, 0, NULL); + if (!SSL_CTX_load_verify_locations(ssl_ctx, "/etc/ssl/cert.pem", NULL)) + fatal("SSL_CTX_load_verify_locations"); + if (!SSL_CTX_set_ssl_version(ssl_ctx, SSLv23_client_method())) + fatal("SSL_CTX_set_ssl_version"); + SSL_CTX_set_verify(ssl_ctx, SSL_VERIFY_NONE , NULL); + if (pledge("stdio inet dns tmppath", NULL) == -1) fatal("pledge"); @@ -245,6 +259,9 @@ parse_server(char *server) if (port == NULL) port = "smtp"; + if (servname == NULL) + servname = host; + memset(, 0, sizeof(hints)); hints.ai_family = AF_UNSPEC; hints.ai_socktype = SOCK_STREAM; @@ -330,20 +347,42 @@ log_trace(int lvl, const char *emsg, ... void smtp_verify_server_cert(void *tag, struct smtp_client *proto, void *ctx) { - log_debug("validating server certificate..."); + SSL *ssl = ctx; + X509 *cert; + long res; + int r, match; + + if ((cert = SSL_get_peer_certificate(ssl))) { + r = ssl_check_name(cert, servname, ); + X509_free(cert); + res = SSL_get_verify_result(ssl); + if (res == X509_V_OK) { + if (match) { + log_debug("valid certificate"); + smtp_cert_verified(proto, CERT_OK); + } + else { + log_debug("certificate does not match hostname"); + smtp_cert_verified(proto, CERT_INVALID); + } + return; + } + log_debug("certificate validation error %ld", res); + } + else + log_debug("no certificate provided"); - /* Not implemented for now. */ - smtp_cert_verified(proto, CERT_UNKNOWN); + smtp_cert_verified(proto, CERT_INVALID); } void smtp_require_tls(void *tag, struct smtp_client *proto) { - void *ctx; - - ctx = ssl_mta_init(NULL, NULL, 0, NULL); + SSL *ssl = NULL; - smtp_set_tls(proto, ctx); + if ((ssl = SSL_new(ssl_ctx)) == NULL) + fatal("SSL_new"); + smtp_set_tls(proto, ssl); } void Index: ssl.h === RCS file: /cvs/src/usr.sbin/smtpd/ssl.h,v retrieving revision 1.20 diff -u -p
smtp(1) certificate validation
Hi, This patch adds the missing bits for verifying the server certificate in smtp(1). Eric. Index: smtpc.c === RCS file: /cvs/src/usr.sbin/smtpd/smtpc.c,v retrieving revision 1.8 diff -u -p -r1.8 smtpc.c --- smtpc.c 2 Sep 2019 20:05:21 - 1.8 +++ smtpc.c 6 Sep 2019 06:39:15 - @@ -20,6 +20,7 @@ #include #include +#include #include #include #include @@ -30,12 +31,12 @@ #include #include +#include + #include "smtp.h" +#include "ssl.h" #include "log.h" -void ssl_init(void); -void *ssl_mta_init(void *, char *, off_t, const char *); - static void parse_server(char *); static void parse_message(FILE *); static void resume(void); @@ -47,6 +48,8 @@ static struct addrinfo *res0, *ai; static struct smtp_params params; static struct smtp_mail mail; +static SSL_CTX *ssl_ctx; + static void usage(void) { @@ -132,6 +135,13 @@ main(int argc, char **argv) ssl_init(); event_init(); + ssl_ctx = ssl_ctx_create(NULL, NULL, 0, NULL); + if (!SSL_CTX_load_verify_locations(ssl_ctx, "/etc/ssl/cert.pem", NULL)) + fatal("SSL_CTX_load_verify_locations"); + if (!SSL_CTX_set_ssl_version(ssl_ctx, SSLv23_client_method())) + fatal("SSL_CTX_set_ssl_version"); + SSL_CTX_set_verify(ssl_ctx, SSL_VERIFY_NONE , NULL); + if (pledge("stdio inet dns tmppath", NULL) == -1) fatal("pledge"); @@ -330,20 +340,34 @@ log_trace(int lvl, const char *emsg, ... void smtp_verify_server_cert(void *tag, struct smtp_client *proto, void *ctx) { - log_debug("validating server certificate..."); + SSL *ssl = ctx; + X509 *cert; + long res; + + if ((cert = SSL_get_peer_certificate(ssl))) { + X509_free(cert); + res = SSL_get_verify_result(ssl); + if (res == X509_V_OK) { + log_debug("valid certificate"); + smtp_cert_verified(proto, CERT_OK); + return; + } + log_debug("certificate validation error %ld", res); + } + else + log_debug("no certificate provided"); - /* Not implemented for now. */ - smtp_cert_verified(proto, CERT_UNKNOWN); + smtp_cert_verified(proto, CERT_INVALID); } void smtp_require_tls(void *tag, struct smtp_client *proto) { - void *ctx; - - ctx = ssl_mta_init(NULL, NULL, 0, NULL); + SSL *ssl = NULL; - smtp_set_tls(proto, ctx); + if ((ssl = SSL_new(ssl_ctx)) == NULL) + fatal("SSL_new"); + smtp_set_tls(proto, ssl); } void Index: smtp/Makefile === RCS file: /cvs/src/usr.sbin/smtpd/smtp/Makefile,v retrieving revision 1.2 diff -u -p -r1.2 Makefile --- smtp/Makefile 12 Jun 2019 17:42:53 - 1.2 +++ smtp/Makefile 5 Sep 2019 18:42:07 - @@ -13,7 +13,6 @@ SRCS+=log.c SRCS+= smtp_client.c SRCS+= smtpc.c SRCS+= ssl.c -SRCS+= ssl_smtpd.c CPPFLAGS+= -DIO_TLS
Re: gethostbyname return TRY_AGAIN on network outage
Hi. Your diff looks correct. I can't see a reason for the "ar_datalen == -1" special case here. According to my tests, gethostbyname() with a bogus nameserver (not responding, not reachable, ...) now fails with TRY_AGAIN, but still fails with HOST_NOT_FOUND if "bind" is not set on the lookup directive, which feels consistent. I have to review the resolver logic to see if a fix is needed at deeper level. But still, this diff makes sense on its own, so ok eric@. Eric. On Wed, Jun 26, 2019 at 11:57:10AM +0200, Martijn van Duren wrote: > Found this yesterday while playing with a dnsbl filter for smtpd during > network outage and found that gethostbyname returns HOST_NOT_FOUND. > Glibc, musl, and FreeBSD return TRY_AGAIN. > > This is annoying because for a DNSBL I need to be able to differentiate > between a resolver not being available and an entry not existing in the > upstream database. > > The reason is that in gethostnamadr_async.c for case ASR_STATE_NOT_FOUND > the ar_h_errno is set to HOST_NOT_FOUND if we don't have a subq_h_errno. > > Looking at case ASR_STATE_SUBQUERY we don't copy subq_h_errno from > ar->ar_h_errno, while res_send_async.c (which is used by gethostnamaddr > as a backend) always sets ar_h_errno and ar_count in case > ASR_STATE_HALT. > > I've looked at the code but I can't find any reason why not getting a > reply gets this special treatment. > > Assuming I haven't missed anything the diff below uses the fact that > no packet also has ar_count set to 0 (res_send_async.c:268) and always > copy the ar_h_errno from the subquery. > > This both reduces the LoC and fixes the issue for me. > > OK? > > martijn@ > > Index: gethostnamadr_async.c > === > RCS file: /cvs/src/lib/libc/asr/gethostnamadr_async.c,v > retrieving revision 1.44 > diff -u -p -r1.44 gethostnamadr_async.c > --- gethostnamadr_async.c 28 Apr 2018 15:16:49 - 1.44 > +++ gethostnamadr_async.c 26 Jun 2019 09:55:02 - > @@ -289,12 +289,10 @@ gethostnamadr_async_run(struct asr_query > /* Done. */ > as->as_subq = NULL; > > - if (ar->ar_datalen == -1) { > - async_set_state(as, ASR_STATE_NEXT_DB); > - break; > - } > - > - /* If we got a packet but no anwser, use the next DB. */ > + /* > + * We either got no packet or a packet without an answer. > + * Saveguard the h_errno and use the next DB. > + */ > if (ar->ar_count == 0) { > free(ar->ar_data); > as->as.hostnamadr.subq_h_errno = ar->ar_h_errno; > >
smtpd: update table api
Hi. This diff changes the internal table interface. The backends now return results as formatted strings, parsing is delegated to the upper layer. It's been lightly tested already, but more tests would be very welcome, especially with setups involving lots of tables (including external ones). Eric. Index: smtpd.h === RCS file: /cvs/src/usr.sbin/smtpd/smtpd.h,v retrieving revision 1.594 diff -u -p -r1.594 smtpd.h --- smtpd.h 13 Dec 2018 17:08:10 - 1.594 +++ smtpd.h 17 Dec 2018 16:33:09 - @@ -375,8 +375,8 @@ struct table_backend { void *(*open)(struct table *); int (*update)(struct table *); void(*close)(void *); - int (*lookup)(void *, struct dict *, const char *, enum table_service, union lookup *); - int (*fetch)(void *, struct dict *, enum table_service, union lookup *); + int (*lookup)(void *, struct dict *, const char *, enum table_service, char **); + int (*fetch)(void *, struct dict *, enum table_service, char **); }; @@ -1601,8 +1601,6 @@ int table_regex_match(const char *, cons void table_open_all(struct smtpd *); void table_dump_all(struct smtpd *); void table_close_all(struct smtpd *); -int table_parse_lookup(enum table_service, const char *, const char *, -union lookup *); /* to.c */ Index: table.c === RCS file: /cvs/src/usr.sbin/smtpd/table.c,v retrieving revision 1.32 diff -u -p -r1.32 table.c --- table.c 2 Nov 2018 13:45:59 - 1.32 +++ table.c 17 Dec 2018 16:09:02 - @@ -53,6 +53,8 @@ extern struct table_backend table_backen static const char * table_service_name(enum table_service); static const char * table_backend_name(struct table_backend *); static const char * table_dump_lookup(enum table_service, union lookup *); +static int table_parse_lookup(enum table_service, const char *, const char *, +union lookup *); static int parse_sockaddr(struct sockaddr *, int, const char *); static unsigned int last_table_id = 0; @@ -125,7 +127,7 @@ table_lookup(struct table *table, struct union lookup *lk) { int r; - charlkey[1024]; + charlkey[1024], *buf = NULL; if (table->t_backend->lookup == NULL) return (-1); @@ -135,9 +137,9 @@ table_lookup(struct table *table, struct return -1; } - r = table->t_backend->lookup(table->t_handle, params, lkey, kind, lk); + r = table->t_backend->lookup(table->t_handle, params, lkey, kind, lk ? : NULL); - if (r == 1) + if (r == 1) { log_trace(TRACE_LOOKUP, "lookup: %s \"%s\" as %s in table %s:%s -> %s%s%s", lk ? "lookup" : "check", lkey, @@ -145,8 +147,11 @@ table_lookup(struct table *table, struct table_backend_name(table->t_backend), table->t_name, lk ? "\"" : "", - (lk) ? table_dump_lookup(kind, lk): "found", + (lk) ? buf : "found", lk ? "\"" : ""); + if (buf) + r = table_parse_lookup(kind, lkey, buf, lk); + } else log_trace(TRACE_LOOKUP, "lookup: %s \"%s\" as %s in table %s:%s -> %d", lk ? "lookup" : "check", @@ -156,6 +161,8 @@ table_lookup(struct table *table, struct table->t_name, r); + free(buf); + return (r); } @@ -163,20 +170,24 @@ int table_fetch(struct table *table, struct dict *params, enum table_service kind, union lookup *lk) { int r; + char*buf = NULL; if (table->t_backend->fetch == NULL) return (-1); - r = table->t_backend->fetch(table->t_handle, params, kind, lk); + r = table->t_backend->fetch(table->t_handle, params, kind, lk ? : NULL); - if (r == 1) + if (r == 1) { log_trace(TRACE_LOOKUP, "lookup: fetch %s from table %s:%s -> %s%s%s", table_service_name(kind), table_backend_name(table->t_backend), table->t_name, lk ? "\"" : "", - (lk) ? table_dump_lookup(kind, lk): "found", + (lk) ? buf : "found", lk ? "\"" : ""); + if (buf) + r = table_parse_lookup(kind, NULL, buf, lk); + } else log_trace(TRACE_LOOKUP, "lookup: fetch %s from table %s:%s -> %d", table_service_name(kind), @@ -184,6 +195,8 @@ table_fetch(struct table *table, struct table->t_name, r); + free(buf); + return (r); } @@ -535,7 +548,7 @@ table_close_all(struct smtpd *conf) table_close(t); }
smtpd: force TLS when relaying
There is currently no way to force TLS on a relay rule in general, and force certificate checking. Typical use case: a secondary MX needing to relay safely to lower preference MXs. This diff below allows the "tls" option to be used alone, including on non-smarthost relay rules, to specify that the relay must be using TLS. The "no-verify" keyword becomes optional. Currently, the different cases are as follows: - action relay Standard relaying, using smtp with opportunistic STARTTLS. When using TLS, certificates are not checked. - action relay host Relay through smarthost, using TLS or not, depending on the protocol. When using TLS, certificates are checked. - action relay host tls no-verify Same as above, but certificates are not checked. With the proposed change, we get: - action relay Standard relaying, using smtp with opportunistic STARTTLS. When using TLS, certificates are not checked. - action relay tls Standard relaying, using smtp with mandatory STARTTLS. Certificates are checked. - action relay tls no-verify Same as above, but certificates are not checked. - action relay host Relay through smarthost, using TLS or not, depending on the protocol. For "smtp+tls://" and "smtps://" certificates are checked. For "smtp://" (opportunistic TLS) certificates are not checked. - action relay host tls Relay through smarthost with mandatory TLS. Certificates are checked. The "smtp://" protocol is updated to "smtp+tls://" internally. The "smtp+notls://" protocol is rejected, and no relaying happens. - action relay host tls no-verify Same as above, but certificates are not checked. The differences with the currently allowed syntax are: 1) the "tls no-verify" option on smarthost relay actually forces TLS, 2) a relay with a "smtp://" smarthost and no "tls no-verify" does not require a valid certificate anymore. It is more constistent altogether, and in practice it should not be a problem because most smarthost configurations uses strict TLS. Now, for the secondary MX example, the rule would look like: action "do-backup" relay backup tls Comments? Eric. Index: mta.c === RCS file: /cvs/src/usr.sbin/smtpd/mta.c,v retrieving revision 1.225 diff -u -p -r1.225 mta.c --- mta.c 19 Sep 2018 05:31:12 - 1.225 +++ mta.c 21 Sep 2018 08:09:14 - @@ -657,6 +657,23 @@ mta_handle_envelope(struct envelope *evp return; } + if (dispatcher->u.remote.tls_required) { + /* Reject relay if smtp+notls:// is requested */ + if (relayh.tls == RELAY_TLS_NO) { + log_warnx("warn: TLS required for action \"%s\"", + evp->dispatcher); + m_create(p_queue, IMSG_MTA_DELIVERY_TEMPFAIL, 0, 0, -1); + m_add_evpid(p_queue, evp->id); + m_add_string(p_queue, "TLS required for relaying"); + m_add_int(p_queue, ESC_OTHER_STATUS); + m_close(p_queue); + return; + } + /* Update smtp:// to smtp+tls:// */ + if (relayh.tls == RELAY_TLS_OPPORTUNISTIC) + relayh.tls = RELAY_TLS_STARTTLS; + } + relay = mta_relay(evp, ); /* ignore if we don't know the limits yet */ if (relay->limits && @@ -1739,7 +1756,7 @@ mta_relay(struct envelope *e, struct rel if (!key.authlabel[0]) key.authlabel = NULL; - if (dispatcher->u.remote.smarthost && + if ((key.tls == RELAY_TLS_STARTTLS || key.tls == RELAY_TLS_SMTPS) && dispatcher->u.remote.tls_noverify == 0) key.flags |= RELAY_TLS_VERIFY; Index: parse.y === RCS file: /cvs/src/usr.sbin/smtpd/parse.y,v retrieving revision 1.221 diff -u -p -r1.221 parse.y --- parse.y 7 Sep 2018 07:35:31 - 1.221 +++ parse.y 21 Sep 2018 08:09:14 - @@ -739,17 +739,21 @@ HELO STRING { dispatcher->u.remote.smarthost = strdup(t->t_name); } -| TLS NO_VERIFY { - if (dispatcher->u.remote.smarthost == NULL) { - yyerror("tls no-verify may not be specified without host on a dispatcher"); +| TLS { + if (dispatcher->u.remote.tls_required == 1) { + yyerror("tls already specified for this dispatcher"); YYERROR; } - if (dispatcher->u.remote.tls_noverify == 1) { - yyerror("tls no-verify already specified for this dispatcher"); + dispatcher->u.remote.tls_required = 1; +} +| TLS NO_VERIFY { + if (dispatcher->u.remote.tls_required == 1) { + yyerror("tls already specified for this dispatcher"); YYERROR; } +
smtpd: flags cleanup in mta
With the recent changes in the smarthost syntax, and the removal of the "secure" keyword, it's now possible to clarify the mta code by changing the TLS option from a set flags to exclusive values. This is far less confusing. More cleanup to come in mta_session.c after that. Eric. Index: mta.c === RCS file: /cvs/src/usr.sbin/smtpd/mta.c,v retrieving revision 1.222 diff -u -p -r1.222 mta.c --- mta.c 22 Aug 2018 10:11:43 - 1.222 +++ mta.c 5 Sep 2018 12:42:19 - @@ -635,6 +635,7 @@ mta_handle_envelope(struct envelope *evp } memset(, 0, sizeof(relayh)); + relayh.tls = RELAY_TLS_OPPORTUNISTIC; if (smarthost && !text_to_relayhost(, smarthost)) { log_warnx("warn: Failed to parse smarthost %s", smarthost); m_create(p_queue, IMSG_MTA_DELIVERY_TEMPFAIL, 0, 0, -1); @@ -1730,10 +1731,9 @@ mta_relay(struct envelope *e, struct rel key.flags |= RELAY_MX; } else { key.domain = mta_domain(e->dest.domain, 0); - if (!(relayh->flags & RELAY_STARTTLS)) - key.flags |= RELAY_TLS_OPTIONAL; } + key.tls = relayh->tls; key.flags |= relayh->flags; key.port = relayh->port; key.authlabel = relayh->authlabel; @@ -1748,6 +1748,7 @@ mta_relay(struct envelope *e, struct rel r = xcalloc(1, sizeof *r); TAILQ_INIT(>tasks); r->id = generate_uid(); + r->tls = key.tls; r->flags = key.flags; r->domain = key.domain; r->backupname = key.backupname ? @@ -1834,14 +1835,25 @@ mta_relay_to_text(struct mta_relay *rela (void)strlcat(buf, tmp, sizeof buf); } - if (relay->flags & RELAY_STARTTLS) { - (void)strlcat(buf, sep, sizeof buf); - (void)strlcat(buf, "starttls", sizeof buf); - } - - if (relay->flags & RELAY_SMTPS) { - (void)strlcat(buf, sep, sizeof buf); + (void)strlcat(buf, sep, sizeof buf); + switch(relay->tls) { + case RELAY_TLS_OPPORTUNISTIC: + (void)strlcat(buf, "smtp", sizeof buf); + break; + case RELAY_TLS_STARTTLS: + (void)strlcat(buf, "smtp+tls", sizeof buf); + break; + case RELAY_TLS_SMTPS: (void)strlcat(buf, "smtps", sizeof buf); + break; + case RELAY_TLS_NO: + if (relay->flags & RELAY_LMTP) + (void)strlcat(buf, "lmtp", sizeof buf); + else + (void)strlcat(buf, "smtp+notls", sizeof buf); + break; + default: + (void)strlcat(buf, "???", sizeof buf); } if (relay->flags & RELAY_AUTH) { @@ -1993,6 +2005,11 @@ mta_relay_cmp(const struct mta_relay *a, if (a->domain < b->domain) return (-1); if (a->domain > b->domain) + return (1); + + if (a->tls < b->tls) + return (-1); + if (a->tls > b->tls) return (1); if (a->flags < b->flags) Index: mta_session.c === RCS file: /cvs/src/usr.sbin/smtpd/mta_session.c,v retrieving revision 1.109 diff -u -p -r1.109 mta_session.c --- mta_session.c 5 Sep 2018 10:15:41 - 1.109 +++ mta_session.c 5 Sep 2018 12:42:19 - @@ -199,24 +199,23 @@ mta_session(struct mta_relay *relay, str if (relay->flags & RELAY_LMTP) s->flags |= MTA_LMTP; - switch (relay->flags & (RELAY_SSL|RELAY_TLS_OPTIONAL)) { - case RELAY_SSL: - s->flags |= MTA_FORCE_ANYSSL; - s->flags |= MTA_WANT_SECURE; - break; - case RELAY_SMTPS: + switch (relay->tls) { + case RELAY_TLS_SMTPS: s->flags |= MTA_FORCE_SMTPS; s->flags |= MTA_WANT_SECURE; break; - case RELAY_STARTTLS: + case RELAY_TLS_STARTTLS: s->flags |= MTA_FORCE_TLS; s->flags |= MTA_WANT_SECURE; break; - case RELAY_TLS_OPTIONAL: + case RELAY_TLS_OPPORTUNISTIC: /* do not force anything, try tls then smtp */ break; - default: + case RELAY_TLS_NO: s->flags |= MTA_FORCE_PLAIN; + break; + default: + fatalx("bad value for relay->tls: %d", relay->tls); } if (relay->flags & RELAY_BACKUP) Index: smtpd.h === RCS file: /cvs/src/usr.sbin/smtpd/smtpd.h,v
Re: smtpd: improve syntax for relay host
Hi. Same diff with associated manpage update. If there is no objection, I'd like to commit this quickly. Eric. Index: smtpd.conf.5 === RCS file: /cvs/src/usr.sbin/smtpd/smtpd.conf.5,v retrieving revision 1.199 diff -u -p -r1.199 smtpd.conf.5 --- smtpd.conf.51 Sep 2018 19:56:28 - 1.199 +++ smtpd.conf.52 Sep 2018 18:56:44 - @@ -228,7 +228,38 @@ to advertise during the HELO phase. .It Cm host Ar relay-url Do not perform MX lookups but relay messages to the relay host described by .Ar relay-url . -If the URL uses TLS, the certificate will be verified by default. +The format for +.Ar relay-url +is +.Sm off +.Op Ar proto No :// Op Ar label No @ +.Ar host Op : Ar port . +.Sm on +The following protocols are available: +.Pp +.Bl -tag -width "smtp+notls" -compact +.It smtp +Normal SMTP session with opportunistic STARTTLS. +.It smtp+tls +Normal SMTP session with mandatory STARTTLS. +.It smtp+notls +Plain text SMTP session without TLS. +.It lmtp +LMTP session. +.It smtps +SMTP session with forced TLS on connection. +.El +.Pp +If not specified, the +.Dq smtp +protocol is used. +.Pp +Specifying an auth label toggles authentication. +An auth table must also be defined for this action. +The protocol must explicitely require TLS. +.Pp +If TLS is explicitely required, the server certificate +will be verified by default. .It Cm tls no-verify Do not require a valid certificate for the specified host. .It Cm auth Pf < Ar table Ns > Index: to.c === RCS file: /cvs/src/usr.sbin/smtpd/to.c,v retrieving revision 1.31 diff -u -p -r1.31 to.c --- to.c7 Jun 2018 11:31:51 - 1.31 +++ to.c2 Sep 2018 18:56:44 - @@ -310,15 +310,11 @@ text_to_relayhost(struct relayhost *rela * new schemas should be *appended* otherwise the default * schema index needs to be updated later in this function. */ - { "smtp://",0 }, + { "smtp://",RELAY_TLS_OPTIONAL }, + { "smtp+tls://",RELAY_STARTTLS }, + { "smtp+notls://", 0 }, { "lmtp://",RELAY_LMTP }, - { "smtp+tls://",RELAY_TLS_OPTIONAL }, - { "smtps://", RELAY_SMTPS }, - { "tls://", RELAY_STARTTLS }, - { "smtps+auth://", RELAY_SMTPS|RELAY_AUTH }, - { "tls+auth://",RELAY_STARTTLS|RELAY_AUTH }, - { "secure://", RELAY_SMTPS|RELAY_STARTTLS }, - { "secure+auth://", RELAY_SMTPS|RELAY_STARTTLS|RELAY_AUTH } + { "smtps://", RELAY_SMTPS } }; const char *errstr = NULL; char *p, *q; @@ -341,8 +337,8 @@ text_to_relayhost(struct relayhost *rela if (strstr(buffer, "://")) return 0; - /* no schema, default to smtp+tls:// */ - i = 2; + /* no schema, default to smtp:// */ + i = 0; p = buffer; } else @@ -397,10 +393,13 @@ text_to_relayhost(struct relayhost *rela return 0; if ((relay->flags & RELAY_LMTP) && (relay->port == 0)) return 0; - if (relay->authlabel[0] == '\0' && relay->flags & RELAY_AUTH) - return 0; - if (relay->authlabel[0] != '\0' && !(relay->flags & RELAY_AUTH)) - return 0; + if (relay->authlabel[0]) { + /* disallow auth on non-tls scheme. */ + if (!(relay->flags & (RELAY_STARTTLS | RELAY_SMTPS))) + return 0; + relay->flags |= RELAY_AUTH; + } + return 1; }
smtpd: improve syntax for relay host
For clarity and consistency, we'd like to change the url-like schemes used for specifying smarthost relays in smtpd.conf, to make them match what has been set for smtp(1). The proposed changes are as follow: - the "+auth" specifier is removed: it is implied by the presence of an auth label in the rest of the string - "secure://" is removed: use "smtp+tls://" or "smtps://" explicitely - "tls://" is removed, and replaced by "smtp+tls://" - "smtp://" becomes SMTP with opportunistic STARTTLS: use "smtp+notls://" to disable TLS - "smtp+tls://" becomes SMTP with mandatory STARTTLS: use "smtp://" for opportunistic STARTTLS It might look confusing (especially since the current schemes are apparently not documented), but in practice, the update process is very simple: 1) If you have "+auth" just remove it, 2) then rewrite the rest as follow: smtp+tls:// -> smtp:// smtp:// -> smtp+notls:// tls:// -> smtp+tls:// smtps://-> no change lmtp:// -> no change secure:// -> choose between smtp+tls:// and smtps:// For example, when relaying through a smarthost with authentication, the change would be: -action "foo" relay host "tls+auth://la...@smtp.example.com" auth +action "foo" relay host "smtp+tls://la...@smtp.example.com" auth or, when using smtps: -action "foo" relay host "smtps+auth://la...@smtp.example.com" auth +action "foo" relay host "smtps://la...@smtp.example.com" auth The default remains SMTP with opportunistic STARTTLS, so a rule like the following has the same behaviour as before: action "foo" relay host "smtp.example.com" Note that there is no impact on incoming or queued mails. The consequences for running with the new schemes without updating the config first are: - an "smtp://" relay would start to do opportunistic STARTTLS, so at worst mails would be sent over a secure channel instead of plain text. - an "smtp+tls://" relay would not fallback to plain text if STARTTLS fails, and the mail will tempfail. - in all other cases, the mail will tempfail with a warning. Does that look fine? Eric. Index: to.c === RCS file: /cvs/src/usr.sbin/smtpd/to.c,v retrieving revision 1.31 diff -u -p -r1.31 to.c --- to.c7 Jun 2018 11:31:51 - 1.31 +++ to.c29 Aug 2018 07:32:52 - @@ -310,15 +310,11 @@ text_to_relayhost(struct relayhost *rela * new schemas should be *appended* otherwise the default * schema index needs to be updated later in this function. */ - { "smtp://",0 }, + { "smtp://",RELAY_TLS_OPTIONAL }, + { "smtp+tls://",RELAY_STARTTLS }, + { "smtp+notls://", 0 }, { "lmtp://",RELAY_LMTP }, - { "smtp+tls://",RELAY_TLS_OPTIONAL }, - { "smtps://", RELAY_SMTPS }, - { "tls://", RELAY_STARTTLS }, - { "smtps+auth://", RELAY_SMTPS|RELAY_AUTH }, - { "tls+auth://",RELAY_STARTTLS|RELAY_AUTH }, - { "secure://", RELAY_SMTPS|RELAY_STARTTLS }, - { "secure+auth://", RELAY_SMTPS|RELAY_STARTTLS|RELAY_AUTH } + { "smtps://", RELAY_SMTPS } }; const char *errstr = NULL; char *p, *q; @@ -341,8 +337,8 @@ text_to_relayhost(struct relayhost *rela if (strstr(buffer, "://")) return 0; - /* no schema, default to smtp+tls:// */ - i = 2; + /* no schema, default to smtp:// */ + i = 0; p = buffer; } else @@ -397,10 +393,13 @@ text_to_relayhost(struct relayhost *rela return 0; if ((relay->flags & RELAY_LMTP) && (relay->port == 0)) return 0; - if (relay->authlabel[0] == '\0' && relay->flags & RELAY_AUTH) - return 0; - if (relay->authlabel[0] != '\0' && !(relay->flags & RELAY_AUTH)) - return 0; + if (relay->authlabel[0]) { + /* disallow auth on non-tls scheme. */ + if (!(relay->flags & (RELAY_STARTTLS | RELAY_SMTPS))) + return 0; + relay->flags |= RELAY_AUTH; + } + return 1; }
smtpd: improve message parser
Hi, The message parser was introduced for different reasons, when it turned out that altering the incoming message was actually needed. It is used to add required "Date" and "Message-ID" headers if missing, to append the local domain to incomplete addresses found in "To", "From" and "Cc" headers, and to provide basic sender masquerading. The current implementation was a best-effort, but it has some short-comings, especially when it comes to the handling of long lines. So this diff provides a replacement for the whole parser, with the following intended improvements: - Use a more straightforward interface rather than the callback approach. - Avoid unnecessary string copy. - Stop using fixed-size string buffers, especially on the stack. This is a step towards better handling of message line length in the daemon. Please test and report success/issues. Eric. Index: rfc5322.c === RCS file: rfc5322.c diff -N rfc5322.c --- /dev/null 1 Jan 1970 00:00:00 - +++ rfc5322.c 26 Jul 2018 14:40:56 - @@ -0,0 +1,264 @@ +/* $OpenBSD: rfc5322.c,v 1.7 2016/02/04 22:35:17 eric Exp $ */ + +/* + * Copyright (c) 2018 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 + +#include "rfc5322.h" + +struct buf { + char*buf; + size_t bufsz; + size_t buflen; + size_t bufmax; +}; + +static int buf_alloc(struct buf *, size_t); +static int buf_grow(struct buf *, size_t); +static int buf_cat(struct buf *, const char *); + +struct rfc5322_parser { + const char *line; + int state; /* last parser state */ + int next; /* parser needs data */ + int unfold; + const char *currhdr; + struct buf hdr; + struct buf val; +}; + +struct rfc5322_parser * +rfc5322_parser_new(void) +{ + struct rfc5322_parser *parser; + + parser = calloc(1, sizeof(*parser)); + if (parser == NULL) + return NULL; + + rfc5322_clear(parser); + parser->hdr.bufmax = 1024; + parser->val.bufmax = 65536; + + return parser; +} + +void +rfc5322_free(struct rfc5322_parser *parser) +{ + free(parser->hdr.buf); + free(parser->val.buf); + free(parser); +} + +void +rfc5322_clear(struct rfc5322_parser *parser) +{ + parser->line = NULL; + parser->state = RFC5322_NONE; + parser->next = 0; + parser->hdr.buflen = 0; + parser->val.buflen = 0; +} + +int +rfc5322_push(struct rfc5322_parser *parser, const char *line) +{ + if (parser->line) { + errno = EALREADY; + return -1; + } + + parser->line = line; + parser->next = 0; + + return 0; +} + +int +rfc5322_unfold_header(struct rfc5322_parser *parser) +{ + if (parser->unfold) { + errno = EALREADY; + return -1; + } + + if (parser->currhdr == NULL) { + errno = EOPNOTSUPP; + return -1; + } + + if (buf_cat(>val, parser->currhdr) == -1) + return -1; + + parser->currhdr = NULL; + parser->unfold = 1; + + return 0; +} + +static int +_rfc5322_next(struct rfc5322_parser *parser, struct rfc5322_result *res) +{ + size_t len; + const char *pos, *line; + + line = parser->line; + + switch(parser->state) { + + case RFC5322_HEADER_START: + case RFC5322_HEADER_CONT: + res->hdr = parser->hdr.buf; + + if (line && (line[0] == ' ' || line[0] == '\t')) { + parser->line = NULL; + parser->next = 1; + if (parser->unfold) { + if (buf_cat(>val, "\n") == -1 || + buf_cat(>val, line) == -1) + return -1; + } + res->value = line; + retu
smtpd: improve internal resolver interface
s->flags |= MTA_WAIT; + resolver_getnameinfo(s->route->dst->sa, 0, mta_getnameinfo_cb, s); } } @@ -257,12 +253,11 @@ mta_session_imsg(struct mproc *p, struct struct ca_vrfy_resp_msg *resp_ca_vrfy; struct ca_cert_resp_msg *resp_ca_cert; struct mta_session *s; - struct mta_host *h; struct msg m; uint64_t reqid; const char *name; void*ssl; - int dnserror, status; + int status; struct stat sb; switch (imsg->hdr.type) { @@ -313,26 +308,6 @@ mta_session_imsg(struct mproc *p, struct mta_enter_state(s, MTA_MAIL); return; - case IMSG_MTA_DNS_PTR: - m_msg(, imsg); - m_get_id(, ); - m_get_int(, ); - if (dnserror) - name = NULL; - else - m_get_string(, ); - m_end(); - s = mta_tree_pop(_ptr, reqid); - if (s == NULL) - return; - - h = s->route->dst; - h->lastptrquery = time(NULL); - if (name) - h->ptrname = xstrdup(name); - waitq_run(>ptrname, h->ptrname); - return; - case IMSG_MTA_TLS_INIT: resp_ca_cert = imsg->data; s = mta_tree_pop(_ssl_init, resp_ca_cert->reqid); @@ -462,6 +437,19 @@ mta_free(struct mta_session *s) free(s); stat_decrement("mta.session", 1); mta_route_collect(relay, route); +} + +static void +mta_getnameinfo_cb(void *arg, int gaierrno, const char *host, const char *serv) +{ + struct mta_session *s = arg; + struct mta_host *h; + + h = s->route->dst; + h->lastptrquery = time(NULL); + if (host) + h->ptrname = xstrdup(host); + waitq_run(>ptrname, h->ptrname); } static void Index: pony.c === RCS file: /cvs/src/usr.sbin/smtpd/pony.c,v retrieving revision 1.20 diff -u -p -r1.20 pony.c --- pony.c 24 May 2018 11:38:24 - 1.20 +++ pony.c 20 Jul 2018 17:25:14 - @@ -56,6 +56,13 @@ pony_imsg(struct mproc *p, struct imsg * pony_shutdown(); switch (imsg->hdr.type) { + + case IMSG_GETADDRINFO: + case IMSG_GETADDRINFO_END: + case IMSG_GETNAMEINFO: + resolver_dispatch_result(p, imsg); + return; + case IMSG_CONF_START: return; case IMSG_CONF_END: @@ -75,7 +82,6 @@ pony_imsg(struct mproc *p, struct imsg * return; /* smtp imsg */ - case IMSG_SMTP_DNS_PTR: case IMSG_SMTP_CHECK_SENDER: case IMSG_SMTP_EXPAND_RCPT: case IMSG_SMTP_LOOKUP_HELO: @@ -104,7 +110,6 @@ pony_imsg(struct mproc *p, struct imsg * case IMSG_MTA_DNS_HOST: case IMSG_MTA_DNS_HOST_END: case IMSG_MTA_DNS_MX_PREFERENCE: - case IMSG_MTA_DNS_PTR: case IMSG_MTA_TLS_INIT: case IMSG_MTA_TLS_VERIFY: case IMSG_CTL_RESUME_ROUTE: Index: resolver.c === RCS file: resolver.c diff -N resolver.c --- /dev/null 1 Jan 1970 00:00:00 - +++ resolver.c 20 Jul 2018 17:25:15 - @@ -0,0 +1,366 @@ +/* $OpenBSD$ */ + +/* + * Copyright (c) 2017-2018 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 + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "smtpd.h" +#include "log.h" + +#define p_resolver p_lka + +struct request { + SPLAY_ENTRY(request) entry; + uint32_t id; + void(*cb_ai)(void *, int, struct addrinfo *); + void(*cb_ni)(void *, int, const char *, const char *); + void*arg; + struct a
Re: smtpd: make relay to smarthost to verify TLS by default
On Thu, May 31, 2018 at 04:06:31PM +0200, Sebastien Marie wrote: > Hi, > > When using smarthost ("host" option of "relay") for outgoing mails, TLS > connection aren't verified. If it could make sens for standard MX, I > think it would be better to verify the connection by default if the user > specifies a TLS-aware url for the relay. > > The diff below changes the behaviour of: > action "foo" relay host "smtps://example.com" > > Currently, smtpd will connect to example.com without verifying TLS at > all. There is no option to force such verification (it was present in > with previous grammar). > > With the following diff, the TLS connection is verified by default (and > the connection aborted on error). Opportunistic TLS will be still possible > with a new option: tls no-verify. > > So, for having the old behaviour the syntax will be: > action "foo" relay host "smtps://example.com" tls no-verify > > It affects only smarthost connection. Outgoing using MX still use > opportunistic TLS. > > Regarding the diff, it adds F_TLS_VERIFY option by default for each call > of text_to_relayhost(), so also for "smtp://" or "lmtp://" urls. I checked > that "smtp://" isn't affected by such flag (there is no TLS connection > to verify), and I hope it will be the same for "lmtp://" (confirmation > will be welcome). Next, the grammar is extended to permit to clear the > flag if requested. smtpd(1) already have all the magic to check the > connection. > > Thanks. > -- > Sebastien Marie Hello. This makes sense, indeed. Here is a slightly updated diff for your proposal. It makes the documentatino more accurate: the server certificate is always verified, the flag is only meant to accept invalid certificates. It also fixes build (apparently the mta.c chunk was incorrect). Eric. 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 19:56:03 - @@ -677,6 +677,9 @@ mta_handle_envelope(struct envelope *evp return; } + if (dispatcher->u.remote.tls_noverify == 0) + evp->agent.mta.relay.flags |= F_TLS_VERIFY; + relay = mta_relay(evp); /* ignore if we don't know the limits yet */ if (relay->limits && Index: parse.y === RCS file: /cvs/src/usr.sbin/smtpd/parse.y,v retrieving revision 1.206 diff -u -p -r1.206 parse.y --- parse.y 30 May 2018 19:01:58 - 1.206 +++ parse.y 31 May 2018 19:56:04 - @@ -182,7 +182,7 @@ typedef struct { %token KEY %token LIMIT LISTEN LMTP LOCAL %token MAIL_FROM MAILDIR MASK_SRC MASQUERADE MATCH MAX_MESSAGE_SIZE MAX_DEFERRED MBOX MDA MTA MX -%token NODSN +%token NODSN NOVERIFY %token ON %token PKI PORT %token QUEUE @@ -541,6 +541,19 @@ HELO STRING { dispatcher->u.remote.smarthost = strdup(t->t_name); } +| TLS NOVERIFY { + if (dispatcher->u.remote.smarthost == NULL) { + yyerror("tls no-verify may not be specified without host on a dispatcher"); + YYERROR; + } + + if (dispatcher->u.remote.tls_noverify == 1) { + yyerror("tls no-verify already specified for this dispatcher"); + 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: smtpd.conf.5 === RCS file: /cvs/src/usr.sbin/smtpd/smtpd.conf.5,v retrieving revision 1.183 diff -u -p -r1.183 smtpd.conf.5 --- smtpd.conf.531 May 2018 13:36:35 - 1.183 +++ smtpd.conf.531 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;
smtpd: simplify header rewrite code-path
Hi. The smtp message parser uses the header_domain_append_callback() and header_masquerade_callback() functions to add missing domains in header fields, and rewrite the sender in the From field if required. The two functions are basically the same, except that the latter also rewrites the sender after appending the missing domain. It is also conditionnally hooked to the parser, replacing the former callback. This is a bit confusing. Instead of having two instances of the same complex code, keep only one function that handles everything. It simplifies the code-path and it makes it clear what conditions leads to rewriting the sender. As a bonus, it also fixes a bug (actually not triggerable) in the removed code ("!= 1" should be "== -1"). Please test, especially if you are using the "masquerade" option. Eric. Index: smtp_session.c === RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v retrieving revision 1.314 diff -u -p -r1.314 smtp_session.c --- smtp_session.c 20 Oct 2017 12:23:36 - 1.314 +++ smtp_session.c 10 Nov 2017 13:37:56 - @@ -325,88 +325,6 @@ header_append_domain_buffer(char *buffer } static void -header_domain_append_callback(const struct rfc2822_header *hdr, void *arg) -{ - struct smtp_session*s = arg; - struct rfc2822_line*l; - size_t i, j; - int escape, quote, comment, skip; - charbuffer[APPEND_DOMAIN_BUFFER_SIZE]; - - if (smtp_message_printf(s, "%s:", hdr->name) == -1) - return; - - i = j = 0; - escape = quote = comment = skip = 0; - memset(buffer, 0, sizeof buffer); - - TAILQ_FOREACH(l, >lines, next) { - for (i = 0; i < strlen(l->buffer); ++i) { - if (l->buffer[i] == '(' && !escape && !quote) - comment++; - if (l->buffer[i] == '"' && !escape && !comment) - quote = !quote; - if (l->buffer[i] == ')' && !escape && !quote && comment) - comment--; - if (l->buffer[i] == '\\' && !escape && !comment && !quote) - escape = 1; - else - escape = 0; - - /* found a separator, buffer contains a full address */ - if (l->buffer[i] == ',' && !escape && !quote && !comment) { - if (!skip && j + strlen(s->listener->hostname) + 1 < sizeof buffer) - header_append_domain_buffer(buffer, s->listener->hostname, sizeof buffer); - if (smtp_message_printf(s, "%s,", buffer) == -1) - return; - j = 0; - skip = 0; - memset(buffer, 0, sizeof buffer); - } - else { - if (skip) { - if (smtp_message_printf(s, "%c", - l->buffer[i]) == -1) - return; - } - else { - buffer[j++] = l->buffer[i]; - if (j == sizeof (buffer) - 1) { - if (smtp_message_printf(s, "%s", - buffer) != -1) - return; - skip = 1; - j = 0; - memset(buffer, 0, sizeof buffer); - } - } - } - } - if (skip) { - if (smtp_message_printf(s, "\n") == -1) - return; - } - else { - buffer[j++] = '\n'; - if (j == sizeof (buffer) - 1) { - if (smtp_message_printf(s, "%s", buffer) == -1) - return; - skip = 1; - j = 0; - memset(buffer, 0, sizeof buffer); - } - } - } - - /* end of header, if buffer is not empty we'll process it */ - if (buffer[0]) { - if (j + strlen(s->listener->hostname) + 1 < sizeof buffer) - header_append_domain_buffer(buffer, s->listener->hostname, sizeof buffer); -
Re: smtpd: remove filter leftovers
On Tue, Aug 29, 2017 at 10:26:19AM +0200, Eric Faurot wrote: > Now that the filter code path has been short-circuited, start removing stub > smtp_filter_*() indirections. I'm doing this one function at a time to keep > the diffs simple, starting with smtp_filter_connect(). Actually the complete diff is simple enough. Eric. Index: smtp_session.c === RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v retrieving revision 1.305 diff -u -p -r1.305 smtp_session.c --- smtp_session.c 13 Aug 2017 11:10:30 - 1.305 +++ smtp_session.c 29 Aug 2017 10:08:06 - @@ -189,12 +189,6 @@ static void smtp_queue_open_message(stru static void smtp_queue_commit(struct smtp_session *); static void smtp_queue_rollback(struct smtp_session *); -static void smtp_filter_connect(struct smtp_session *, struct sockaddr *); -static void smtp_filter_eom(struct smtp_session *); -static void smtp_filter_helo(struct smtp_session *); -static void smtp_filter_mail(struct smtp_session *); -static void smtp_filter_rcpt(struct smtp_session *); -static void smtp_filter_data(struct smtp_session *); static void smtp_filter_dataline(struct smtp_session *, const char *); static struct { int code; const char *cmd; } commands[] = { @@ -1001,150 +995,6 @@ smtp_tls_verified(struct smtp_session *s } void -smtp_filter_response(uint64_t id, int query, int status, uint32_t code, -const char *line) -{ - struct smtp_session *s; - struct ca_cert_req_msg req_ca_cert; - - s = tree_xpop(_filter, id); - - if (status == FILTER_CLOSE) { - code = code ? code : 421; - line = line ? line : "Temporary failure"; - smtp_reply(s, "%d %s", code, line); - smtp_enter_state(s, STATE_QUIT); - return; - } - - switch (query) { - - case QUERY_CONNECT: - if (status != FILTER_OK) { - log_info("%016"PRIx64" smtp " - "event=closed address=%s host=%s reason=filter-reject", - s->id, ss_to_text(>ss), s->hostname); - smtp_free(s, "rejected by filter"); - return; - } - - 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); - return; - - case QUERY_HELO: - if (status != FILTER_OK) { - code = code ? code : 530; - line = line ? line : "Hello rejected"; - smtp_reply(s, "%d %s", code, line); - return; - } - - smtp_enter_state(s, STATE_HELO); - smtp_reply(s, "250%c%s Hello %s [%s], pleased to meet you", - (s->flags & SF_EHLO) ? '-' : ' ', - s->smtpname, - s->helo, - ss_to_text(>ss)); - - if (s->flags & SF_EHLO) { - smtp_reply(s, "250-8BITMIME"); - smtp_reply(s, "250-ENHANCEDSTATUSCODES"); - smtp_reply(s, "250-SIZE %zu", env->sc_maxsize); - if (ADVERTISE_EXT_DSN(s)) - smtp_reply(s, "250-DSN"); - if (ADVERTISE_TLS(s)) - smtp_reply(s, "250-STARTTLS"); - if (ADVERTISE_AUTH(s)) - smtp_reply(s, "250-AUTH PLAIN LOGIN"); - smtp_reply(s, "250 HELP"); - } - return; - - case QUERY_MAIL: - if (status != FILTER_OK) { - smtp_tx_free(s->tx); - code = code ? code : 530; - line = line ? line : "Sender rejected"; - s
smtpd: remove filter leftovers
Now that the filter code path has been short-circuited, start removing stub smtp_filter_*() indirections. I'm doing this one function at a time to keep the diffs simple, starting with smtp_filter_connect(). Eric. Index: smtp_session.c === RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v retrieving revision 1.305 diff -u -p -r1.305 smtp_session.c --- smtp_session.c 13 Aug 2017 11:10:30 - 1.305 +++ smtp_session.c 29 Aug 2017 08:03:49 - @@ -189,7 +189,6 @@ static void smtp_queue_open_message(stru static void smtp_queue_commit(struct smtp_session *); static void smtp_queue_rollback(struct smtp_session *); -static void smtp_filter_connect(struct smtp_session *, struct sockaddr *); static void smtp_filter_eom(struct smtp_session *); static void smtp_filter_helo(struct smtp_session *); static void smtp_filter_mail(struct smtp_session *); @@ -1005,7 +1004,6 @@ smtp_filter_response(uint64_t id, int qu const char *line) { struct smtp_session *s; - struct ca_cert_req_msg req_ca_cert; s = tree_xpop(_filter, id); @@ -1019,35 +1017,6 @@ smtp_filter_response(uint64_t id, int qu switch (query) { - case QUERY_CONNECT: - if (status != FILTER_OK) { - log_info("%016"PRIx64" smtp " - "event=closed address=%s host=%s reason=filter-reject", - s->id, ss_to_text(>ss), s->hostname); - smtp_free(s, "rejected by filter"); - return; - } - - 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); - return; - case QUERY_HELO: if (status != FILTER_OK) { code = code ? code : 530; @@ -2023,6 +1992,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 +2007,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,13 +2404,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
smtpd: tweak static table parser
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. Eric. 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.c 14 Aug 2017 08:01:14 - 1.16 +++ table_static.c 16 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;
smtpd: simplify table parser
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. Eric. 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.c 22 Jan 2016 13:08:44 - 1.15 +++ table_static.c 13 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);
[patch] remove smtpd filter code
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. Eric. Index: pony.c === RCS file: /cvs/src/usr.sbin/smtpd/pony.c,v retrieving revision 1.17 diff -u -p -r1.17 pony.c --- pony.c 9 Jan 2017 09:53:23 - 1.17 +++ pony.c 3 Aug 2017 09:57:22 - @@ -60,7 +60,7 @@ pony_imsg(struct mproc *p, struct imsg * case IMSG_CONF_START: return; case IMSG_CONF_END: - filter_configure(); + smtp_configure(); return; case IMSG_CTL_VERBOSE: m_msg(, imsg); @@ -148,7 +148,6 @@ pony(void) mda_postfork(); mta_postfork(); smtp_postfork(); - filter_postfork(); /* do not purge listeners and pki, they are purged * in smtp_configure() Index: smtp_session.c === RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v retrieving revision 1.304 diff -u -p -r1.304 smtp_session.c --- smtp_session.c 19 Jun 2017 08:35:56 - 1.304 +++ smtp_session.c 4 Aug 2017 10:32:41 - @@ -69,9 +69,6 @@ enum session_flags { SF_BOUNCE = 0x0010, SF_VERIFIED = 0x0020, SF_BADINPUT = 0x0080, - SF_FILTERCONN = 0x0100, - SF_FILTERDATA = 0x0200, - SF_FILTERTX = 0x0400, }; enum message_flags { @@ -116,7 +113,7 @@ struct smtp_tx { size_t datain; size_t odatalen; - struct io *oev; + FILE*ofile; int hdrdone; int rcvcount; int dataeom; @@ -167,7 +164,6 @@ static void smtp_connected(struct smtp_s static void smtp_send_banner(struct smtp_session *); static void smtp_tls_verified(struct smtp_session *); static void smtp_io(struct io *, int, void *); -static void smtp_data_io(struct io *, int, void *); static void smtp_data_io_done(struct smtp_session *); static void smtp_enter_state(struct smtp_session *, int); static void smtp_reply(struct smtp_session *, char *, ...); @@ -194,11 +190,6 @@ static void smtp_queue_commit(struct smt static void smtp_queue_rollback(struct smtp_session *); static void smtp_filter_connect(struct smtp_session *, struct sockaddr *); -static void smtp_filter_rset(struct smtp_session *); -static void smtp_filter_disconnect(struct smtp_session *); -static void smtp_filter_tx_begin(struct smtp_session *); -static void smtp_filter_tx_commit(struct smtp_session *); -static void smtp_filter_tx_rollback(struct smtp_session *); static void smtp_filter_eom(struct smtp_session *); static void smtp_filter_helo(struct smtp_session *); static void smtp_filter_mail(struct smtp_session *); @@ -728,12 +719,10 @@ smtp_session_imsg(struct mproc *p, struc break; case LKA_PERMFAIL: - smtp_filter_tx_rollback(s); smtp_tx_free(s->tx); smtp_reply(s, "%d %s", 530, "Sender rejected"); break; case LKA_TEMPFAIL: - smtp_filter_tx_rollback(s); smtp_tx_free(s->tx); smtp_reply(s, "421 %s: Temporary Error", esc_code(ESC_STATUS_TEMPFAIL, ESC_OTHER_MAIL_SYSTEM_STATUS)); @@ -785,7 +774,6 @@ smtp_session_imsg(struct mproc *p, struc smtp_reply(s, "250 %s: Ok", esc_code(ESC_STATUS_OK, ESC_OTHER_STATUS)); } else { - smtp_filter_tx_rollback(s); smtp_tx_free(s->tx); smtp_reply(s, "421 %s: Temporary Error", esc_code(ESC_STATUS_TEMPFAIL, ESC_OTHER_MAIL_SYSTEM_STATUS)); @@ -813,7 +801,7 @@ smtp_session_imsg(struct mproc *p, struc log_debug("smtp: %p: fd %d from queue", s, imsg->fd); tree_xset(_filter, s->id, s); - filter_build_fd_chain(s->id, imsg->fd); + smtp_filter_fd(s->id, imsg->fd); return; case IMSG_QUEUE_ENVELOPE_SUBMIT: @@ -869,7 +857,6 @@ smtp_session_imsg(struct mproc *p, struc m_end(); s = tree_xpop(_queue_commit, reqid); if
Re: SSHFP with EDNS0/DNSSEC
On Wed, Jul 12, 2017 at 07:45:36AM +0200, Christian Barthel wrote: > Hi, > > earlier this year, jca@ worked on support for DNSSEC and the EDNS0 > extension [1] and committed this work at [2] (thanks!). I tried this > with SSHFP records to check authenticity of hosts with DNSSEC; but ssh > reported that the hostkey fingerprints were insecure. > > I am using this configuration file: > > # cat /etc/resolv.conf > nameserver 8.8.8.8 > options edns0 > > And ssh reports the following: > > $ ssh -o VerifyHostKeyDNS=yes - doamin_with_sshpf_dnssec > ... > debug3: verify_host_key_dns > debug1: found 8 insecure fingerprints in DNS > debug1: matching host key fingerprint found in DNS > The authenticity of host 'xxx ()' can't be established. > ECDSA key fingerprint is > Matching host key fingerprint found in DNS. > Are you sure you want to continue connecting (yes/no)? > ... > > I tried to find out why and after going through the asr code, I found > the following: > > Index: lib/libc/asr/res_send_async.c > === > RCS file: /cvs/src/lib/libc/asr/res_send_async.c,v > retrieving revision 1.36 > diff -u -p -r1.36 res_send_async.c > --- lib/libc/asr/res_send_async.c 15 Mar 2017 15:54:41 - 1.36 > +++ lib/libc/asr/res_send_async.c 11 Jul 2017 20:09:59 - > @@ -385,7 +385,7 @@ setup_query(struct asr_query *as, const > _asr_pack_query(, type, class, dname); > if (as->as_ctx->ac_options & (RES_USE_EDNS0 | RES_USE_DNSSEC)) > _asr_pack_edns0(, MAXPACKETSZ, > - as->as_ctx->ac_options & RES_USE_DNSSEC); > + as->as_ctx->ac_options & (RES_USE_EDNS0 | RES_USE_DNSSEC)); > if (p.err) { > DPRINT("error packing query"); > errno = EINVAL; The current code is correct, RES_USE_EDNS0 does not imply RES_USE_DNSSEC. The real problem is that there is no resolv.conf option for RES_USE_DNSSEC. It can only be set in user code by tweaking _res.options. Eric.
Re: smtpd session hang
On Fri, Jun 16, 2017 at 09:11:29AM +0200, Gilles Chehade wrote: > 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 > ib=16839 ob=0> > > debug: smtp: 0xe54baa1e000: filter congestion: pausing session > > smtp: 0xe54baa1e000 (data): IO_LOWAT > ib=0 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@ 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(). 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: asr: support for RES_USE_DNSSEC
On Sat, Feb 25, 2017 at 11:17:37PM +0100, Peter J. Philipp wrote: > Hi, > > I'm not the best in reading patches, so I'm going to query you. Does > your patch check for the "AD" flag from the resolver? As basically a > DNSSEC able recursive nameserver should set this meaning it has > authenticated the data. I wrote a patch for DNSSEC (possibly erroneous > by comparing it to you) and posted it to #opensmtpd in hopes that eric > would see it. Much of that functionality is superfluous now but it does > have an "AD_MASK" check. > > Here is my patch from last year, which I gave up on, feel free to cherry > pick anything needed out of it. You'll see some similarities but they > are different enough to show two different peoples work. Sorry for not getting back to you about this diff at that time. I'll have a look at it. > http://centroid.eu/private/dnssec.patch.txt > > Yours is a lot more complete of course. > > Cheers, > > -peter >
Re: asr: support for RES_USE_DNSSEC
On Sat, Feb 25, 2017 at 05:55:36PM +0100, Jeremie Courreges-Anglas wrote: > > This flag is useful for software that wants to rely on the resolver to > perform DNSSEC validation. Among the use cases there are DANE and SSHFP > records, and the obvious interfaces that I think are useful are > res_mkquery and getrrsetbyname. The latter still doesn't support > DNSSEC, another diff will follow. > > The first hunk is not supposed to be committed, it is only here for > basic testing (add "options dnssec" to resolv.conf). Other testing > could involve postfix or exim. > > Since RES_USE_DNSSEC now actually adds an EDNS0 OPT record to the > outgoing packet, one can be concerned with problems with resolvers out > there. Windows seems to have a way to disable EDNS0, I am not aware of > existing mechanisms elsewhere. > > Thoughts? ok? Looks good. ok eric@ > > Index: asr/asr.c > === > RCS file: /d/cvs/src/lib/libc/asr/asr.c,v > retrieving revision 1.56 > diff -u -p -r1.56 asr.c > --- asr/asr.c 23 Feb 2017 17:04:02 - 1.56 > +++ asr/asr.c 25 Feb 2017 16:04:30 - > @@ -597,6 +597,8 @@ pass0(char **tok, int n, struct asr_ctx > ac->ac_options |= RES_USEVC; > else if (!strcmp(tok[i], "edns0")) > ac->ac_options |= RES_USE_EDNS0; > + else if (!strcmp(tok[i], "dnssec")) > + ac->ac_options |= RES_USE_DNSSEC; > else if ((!strncmp(tok[i], "ndots:", 6))) { > e = NULL; > d = strtonum(tok[i] + 6, 1, 16, ); > Index: asr/asr_private.h > === > RCS file: /d/cvs/src/lib/libc/asr/asr_private.h,v > retrieving revision 1.43 > diff -u -p -r1.43 asr_private.h > --- asr/asr_private.h 23 Feb 2017 17:04:02 - 1.43 > +++ asr/asr_private.h 25 Feb 2017 14:45:16 - > @@ -297,7 +297,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 25 Feb 2017 16:06:01 - > @@ -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 25 Feb 2017 14:45:16 - > @@ -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 > +++
Re: asr: support for RES_USE_DNSSEC
On Sat, Feb 25, 2017 at 07:24:48PM +0100, Jeremie Courreges-Anglas wrote: > Jeremie Courreges-Anglaswrites: > > > This flag is useful for software that wants to rely on the resolver to > > perform DNSSEC validation. Among the use cases there are DANE and SSHFP > > records, and the obvious interfaces that I think are useful are > > res_mkquery and getrrsetbyname. The latter still doesn't support > > DNSSEC, another diff will follow. > > Diff on top of the previous one. I'd like to make getrrsetbyname(3) > "use" RES_USE_DNSSEC again. This would improve support for ssh -o > VerifyHostKeyDNS (SSHFP records). > > The "easy" way would be something like: > > Index: getrrsetbyname_async.c > === > RCS file: /d/cvs/src/lib/libc/asr/getrrsetbyname_async.c,v > retrieving revision 1.11 > diff -u -p -p -u -r1.11 getrrsetbyname_async.c > --- getrrsetbyname_async.c23 Feb 2017 17:04:02 - 1.11 > +++ getrrsetbyname_async.c25 Feb 2017 17:25:25 - > @@ -42,6 +42,7 @@ getrrsetbyname_async(const char *hostnam > struct asr_query *as; > > ac = _asr_use_resolver(asr); > + ac->ac_options |= RES_USE_DNSSEC; > if ((as = _asr_async_new(ac, ASR_GETRRSETBYNAME)) == NULL) > goto abort; /* errno set */ > as->as_run = getrrsetbyname_async_run; > > IIUC this means that we modify the thread-local resolver options used by > subsequent queries. Cleaning up by resetting the flag before returning > doesn't work in all cases because you could have overlap between two > active getrrsetbyname_async and eg getaddrinfo_async contexts. Right. It has to be set a the query level. > The diff below instead adds an as_flags member to struct asr_query, and > merges the flags of the various union members. struct rrset and struct > ni keep their flags member, as they are a different kind of flags. > A subset of as_flags is passed down to the child ASR_SEND subq, the only > flag that is inherited right now is ASYNC_DNSSEC, which allows > getrrsetbyname_async to communicate its intent. > > That's a bit of churn for a small improvement, maybe there is a simpler > diff? > > Comments welcome. > Well, I like the as_flags cleanup part of the diff. Internal flags should indeed be on the asr_query structure directly, so they are not confused with query-specific parameters. For the flags inheritance, I'm not so sure. I would rather keep the current internal API for now, and set the flags explicitely for this specific case. Eric. Index: getrrsetbyname_async.c === RCS file: /cvs/src/lib/libc/asr/getrrsetbyname_async.c,v retrieving revision 1.11 diff -u -p -r1.11 getrrsetbyname_async.c --- getrrsetbyname_async.c 23 Feb 2017 17:04:02 - 1.11 +++ getrrsetbyname_async.c 26 Feb 2017 09:33:47 - @@ -104,6 +104,7 @@ getrrsetbyname_async_run(struct asr_quer async_set_state(as, ASR_STATE_HALT); break; } + as->as_subq->as_flags |= ASYNC_DNSSEC; async_set_state(as, ASR_STATE_SUBQUERY); break;
asr: slightly better error reporting for getnameinfo()
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. 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;
Re: asr EDNS0 support
On Sat, Feb 18, 2017 at 03:53:54PM +0100, Jeremie Courreges-Anglas wrote: > > > Seems to work fine here. We already use a 4096 bytes input buffer, the > edns0 option makes libc advertize this to the DNS resolver. nsd, > unbound, dig(1) and ports/net/isc-bind support this feature. I'm not > suggesting that we use it by default right now, but this could be > a desirable change. > > ok? Yes, I like it a lot. One small comment tough: better use T_OPT rather than 41 in _asr_pack_edns0(). Besides that, ok eric@ > > Index: share/man/man5/resolv.conf.5 > === > RCS file: /d/cvs/src/share/man/man5/resolv.conf.5,v > retrieving revision 1.51 > diff -u -p -r1.51 resolv.conf.5 > --- share/man/man5/resolv.conf.5 24 Jan 2017 12:43:00 - 1.51 > +++ share/man/man5/resolv.conf.5 18 Feb 2017 14:33:37 - > @@ -258,21 +258,21 @@ lines are able to handle the extension. > On > .Ox > this option does nothing. > -.\" .Pp > -.\" To verify whether a server supports EDNS, > -.\" query it using the > -.\" .Xr dig 1 > -.\" query option > -.\" .Li +edns=0 : > -.\" the reply indicates compliance (EDNS version 0) > -.\" and whether a UDP packet larger than 512 bytes can be used. > -.\" Note that EDNS0 can cause the server to send packets > -.\" large enough to require fragmentation. > -.\" Other factors such as packet filters may impede these, > -.\" particularly if there is a reduced MTU, > -.\" as is often the case with > -.\" .Xr pppoe 4 > -.\" or with tunnels. > +.Pp > +To verify whether a server supports EDNS, > +query it using the > +.Xr dig 1 > +query option > +.Li +edns=0 : > +the reply indicates compliance (EDNS version 0) > +and whether a UDP packet larger than 512 bytes can be used. > +Note that EDNS0 can cause the server to send packets > +large enough to require fragmentation. > +Other factors such as packet filters may impede these, > +particularly if there is a reduced MTU, > +as is often the case with > +.Xr pppoe 4 > +or with tunnels. > .It Cm inet6 > Enables support for IPv6-only applications, by setting RES_USE_INET6 in > _res.options (see > Index: lib/libc/asr/asr.c > === > RCS file: /d/cvs/src/lib/libc/asr/asr.c,v > retrieving revision 1.54 > diff -u -p -r1.54 asr.c > --- lib/libc/asr/asr.c18 Jun 2016 15:25:28 - 1.54 > +++ lib/libc/asr/asr.c16 Feb 2017 15:36:30 - > @@ -603,6 +603,8 @@ pass0(char **tok, int n, struct asr_ctx > for (i = 1; i < n; i++) { > if (!strcmp(tok[i], "tcp")) > ac->ac_options |= RES_USEVC; > + else if (!strcmp(tok[i], "edns0")) > + ac->ac_options |= RES_USE_EDNS0; > else if ((!strncmp(tok[i], "ndots:", 6))) { > e = NULL; > d = strtonum(tok[i] + 6, 1, 16, ); > Index: lib/libc/asr/asr_private.h > === > RCS file: /d/cvs/src/lib/libc/asr/asr_private.h,v > retrieving revision 1.41 > diff -u -p -r1.41 asr_private.h > --- lib/libc/asr/asr_private.h17 Feb 2017 22:24:45 - 1.41 > +++ lib/libc/asr/asr_private.h18 Feb 2017 10:44:56 - > @@ -294,6 +294,7 @@ enum asr_state { > ASR_STATE_HALT, > }; > > +#define MAXPACKETSZ 4096 > > __BEGIN_HIDDEN_DECLS > > @@ -301,6 +302,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); > 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: lib/libc/asr/asr_run.3 > === > RCS file: /d/cvs/src/lib/libc/asr/asr_run.3,v > retrieving revision 1.2 > diff -u -p -r1.2 asr_run.3 > --- lib/libc/asr/asr_run.326 Mar 2014 18:13:15 - 1.2 > +++ lib/libc/asr/asr_run.318 Feb 2017 14:39:22 - > @@ -314,6 +314,3 @@ so sharing a resolver among threads is n > .Xr getrrsetbyname 3 , > .Xr res_send 3 , > .Xr resolv.conf 5 > -.Sh CAVEATS > -This DNS resolver implementation doesn't support > -the EDNS0 protocol extension yet. > Index: lib/libc/asr/asr_utils.c > === > RCS file: /d/cvs/src/lib/libc/asr/asr_utils.c,v > retrieving revision 1.14 > diff -u -p -r1.14 asr_utils.c > --- lib/libc/asr/asr_utils.c 17 Feb 2017 22:24:45 - 1.14 > +++ lib/libc/asr/asr_utils.c 18 Feb 2017 10:44:23 - > @@ -381,6 +381,14 @@ pack_u16(struct asr_pack *p, uint16_t v) > } >
smtpd: hide struct io
Hi, After the recent series of cleanups, it is now possible to make struct io opaque: - move struct io definition in ioev.c - replace io_init/io_clear with io_new/io_free - allocate an iobuf for each new io internally - use struct io pointer in the rest of the code - remove remaining uses of iobuf_* The diff is mostly mechanical. Eric. Index: bounce.c === RCS file: /cvs/src/usr.sbin/smtpd/bounce.c,v retrieving revision 1.76 diff -u -p -r1.76 bounce.c --- bounce.c22 Nov 2016 07:28:42 - 1.76 +++ bounce.c28 Nov 2016 12:51:26 - @@ -80,8 +80,7 @@ struct bounce_session { struct bounce_message *msg; FILE*msgfp; int state; - struct iobuf iobuf; - struct ioio; + struct io *io; uint64_t boundary; }; @@ -229,12 +228,11 @@ bounce_fd(int fd) s = xcalloc(1, sizeof(*s), "bounce_fd"); s->smtpname = xstrdup(msg->smtpname, "bounce_fd"); s->state = BOUNCE_EHLO; - iobuf_xinit(>iobuf, 0, 0, "bounce_run"); - io_init(>io, >iobuf); - io_set_callback(>io, bounce_io, s); - io_set_fd(>io, fd); - io_set_timeout(>io, 3); - io_set_read(>io); + s->io = io_new(); + io_set_callback(s->io, bounce_io, s); + io_set_fd(s->io, fd); + io_set_timeout(s->io, 3); + io_set_read(s->io); s->boundary = generate_uid(); log_debug("debug: bounce: new session %p", s); @@ -313,7 +311,7 @@ bounce_send(struct bounce_session *s, co log_trace(TRACE_BOUNCE, "bounce: %p: >>> %s", s, p); - io_xprintf(>io, "%s\n", p); + io_xprintf(s->io, "%s\n", p); free(p); } @@ -452,7 +450,7 @@ bounce_next(struct bounce_session *s) case BOUNCE_DATA_NOTICE: /* Construct an appropriate notice. */ - io_xprintf(>io, + io_xprintf(s->io, "Subject: Delivery status notification: %s\n" "From: Mailer Daemon\n" "To: %s\n" @@ -470,7 +468,7 @@ bounce_next(struct bounce_session *s) s->boundary, s->smtpname); - io_xprintf(>io, + io_xprintf(s->io, "--%16" PRIu64 "/%s\n" "Content-Description: Notification\n" "Content-Type: text/plain; charset=us-ascii\n" @@ -481,14 +479,14 @@ bounce_next(struct bounce_session *s) switch (s->msg->bounce.type) { case B_ERROR: - io_xprint(>io, notice_error); + io_xprint(s->io, notice_error); break; case B_WARNING: - io_xprintf(>io, notice_warning, + io_xprintf(s->io, notice_warning, bounce_duration(s->msg->bounce.delay)); break; case B_DSN: - io_xprint(>io, s->msg->bounce.mta_without_dsn ? + io_xprint(s->io, s->msg->bounce.mta_without_dsn ? notice_relay : notice_success); break; default: @@ -496,32 +494,32 @@ bounce_next(struct bounce_session *s) } TAILQ_FOREACH(evp, >msg->envelopes, entry) { - io_xprint(>io, evp->report); + io_xprint(s->io, evp->report); } - io_xprint(>io, "\n"); + io_xprint(s->io, "\n"); if (s->msg->bounce.type == B_WARNING) - io_xprintf(>io, notice_warning2, + io_xprintf(s->io, notice_warning2, bounce_duration(s->msg->bounce.expire)); - io_xprintf(>io, + io_xprintf(s->io, "Below is a copy of the original message:\n" "\n"); - io_xprintf(>io, + io_xprintf(s->io, "--%16" PRIu64 "/%s\n" "Content-Description: Delivery Report\n" "Content-Type: message/delivery-status\n" "\n", s->boundary, s->smtpname); - io_xprintf(>io, + io_xprintf(s->io, "Reporting-MTA: dns; %s\n" "\n", s->smtpname); TAILQ_FOREACH(evp, >msg->envelopes, entry) { - io_xprintf(>io, + io_xprintf(s->io, "Final-Recipient: rfc822; %s@%s\n" "Action: %s\n" "Status: %s\n" @@ -533,21 +531,21 @@ bounce_next(struct
smtpd: more internal cleanups
Hi. When using the internal io api, the caller had to explicitely reset an io event in some cases (basically when data is buffered outside of an io callback context) which could easily be missed. This has been the cause of some of smtpd bugs in the past (hanging sessions). After the recent changes, it's now possible to make it a lot simpler by triggering an event reload internally when data is queued. So the api user does not have to worry about it. Eric. Index: ioev.c === RCS file: /cvs/src/usr.sbin/smtpd/ioev.c,v retrieving revision 1.31 diff -u -p -r1.31 ioev.c --- ioev.c 22 Nov 2016 07:28:42 - 1.31 +++ ioev.c 22 Nov 2016 22:24:25 - @@ -370,13 +370,25 @@ io_set_write(struct io *io) int io_write(struct io *io, const void *buf, size_t len) { - return iobuf_queue(io->iobuf, buf, len); + int r; + + r = iobuf_queue(io->iobuf, buf, len); + + io_reload(io); + + return r; } int io_writev(struct io *io, const struct iovec *iov, int iovcount) { - return iobuf_queuev(io->iobuf, iov, iovcount); + int r; + + r = iobuf_queuev(io->iobuf, iov, iovcount); + + io_reload(io); + + return r; } int Index: mta_session.c === RCS file: /cvs/src/usr.sbin/smtpd/mta_session.c,v retrieving revision 1.89 diff -u -p -r1.89 mta_session.c --- mta_session.c 22 Nov 2016 07:28:42 - 1.89 +++ mta_session.c 22 Nov 2016 22:24:26 - @@ -280,7 +280,6 @@ mta_session_imsg(struct mproc *p, struct mta_flush_task(s, IMSG_MTA_DELIVERY_TEMPFAIL, "Could not get message fd", 0, 0); mta_enter_state(s, MTA_READY); - io_reload(>io); return; } @@ -289,7 +288,6 @@ mta_session_imsg(struct mproc *p, struct fatal("mta: fdopen"); mta_enter_state(s, MTA_MAIL); - io_reload(>io); return; case IMSG_MTA_DNS_PTR: @@ -366,7 +364,6 @@ mta_session_imsg(struct mproc *p, struct mta_tls_verified(s); io_resume(>io, IO_PAUSE_IN); - io_reload(>io); return; case IMSG_MTA_LOOKUP_HELO: @@ -456,7 +453,6 @@ mta_on_timeout(struct runq *runq, void * s->hangon++; mta_enter_state(s, MTA_READY); - io_reload(>io); } static void Index: smtp_session.c === RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v retrieving revision 1.295 diff -u -p -r1.295 smtp_session.c --- smtp_session.c 22 Nov 2016 07:28:42 - 1.295 +++ smtp_session.c 22 Nov 2016 22:24:26 - @@ -738,14 +738,12 @@ smtp_session_imsg(struct mproc *p, struc smtp_filter_tx_rollback(s); smtp_tx_free(s->tx); smtp_reply(s, "%d %s", 530, "Sender rejected"); - io_reload(>io); break; case LKA_TEMPFAIL: smtp_filter_tx_rollback(s); smtp_tx_free(s->tx); smtp_reply(s, "421 %s: Temporary Error", esc_code(ESC_STATUS_TEMPFAIL, ESC_OTHER_MAIL_SYSTEM_STATUS)); - io_reload(>io); break; } return; @@ -766,7 +764,6 @@ smtp_session_imsg(struct mproc *p, struc case LKA_TEMPFAIL: smtp_reply(s, "%s", line); } - io_reload(>io); return; case IMSG_SMTP_LOOKUP_HELO: @@ -802,7 +799,6 @@ smtp_session_imsg(struct mproc *p, struc smtp_enter_state(s, STATE_QUIT); } m_end(); - io_reload(>io); return; case IMSG_SMTP_MESSAGE_OPEN: @@ -818,7 +814,6 @@ smtp_session_imsg(struct mproc *p, struc 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; } @@ -872,7 +867,6 @@ smtp_session_imsg(struct mproc *p, struc esc_code(ESC_STATUS_OK, ESC_DESTINATION_ADDRESS_VALID), esc_description(ESC_DESTINATION_ADDRESS_VALID)); } - io_reload(>io); return; case IMSG_SMTP_MESSAGE_COMMIT: @@ -887,7 +881,6 @@ smtp_session_imsg(struct mproc *p, struc smtp_reply(s, "421 %s: Temporary failure", esc_code(ESC_STATUS_TEMPFAIL,
smtpd: simplify internal io api
The api user should not have to care about normalizing the io input buffer (i.e. resetting the read/write pos in the buffer). Do it internally when reloading the io event. Eric. Index: bounce.c === RCS file: /cvs/src/usr.sbin/smtpd/bounce.c,v retrieving revision 1.75 diff -u -p -r1.75 bounce.c --- bounce.c21 Nov 2016 13:00:43 - 1.75 +++ bounce.c21 Nov 2016 19:33:46 - @@ -728,10 +728,8 @@ bounce_io(struct io *io, int evt, void * return; } - if (line == NULL) { - iobuf_normalize(>iobuf); + if (line == NULL) break; - } log_trace(TRACE_BOUNCE, "bounce: %p: <<< %s", s, line); Index: filter.c === RCS file: /cvs/src/usr.sbin/smtpd/filter.c,v retrieving revision 1.22 diff -u -p -r1.22 filter.c --- filter.c21 Nov 2016 13:00:43 - 1.22 +++ filter.c21 Nov 2016 19:33:46 - @@ -706,7 +706,6 @@ filter_tx_io(struct io *io, int evt, voi } s->idatalen += n; io_drop(>iev, n); - iobuf_normalize(>ibuf); return; case IO_DISCONNECTED: Index: ioev.c === RCS file: /cvs/src/usr.sbin/smtpd/ioev.c,v retrieving revision 1.30 diff -u -p -r1.30 ioev.c --- ioev.c 20 Nov 2016 08:43:36 - 1.30 +++ ioev.c 21 Nov 2016 19:33:46 - @@ -465,6 +465,9 @@ io_reload(struct io *io) if (io->flags & IO_HELD) return; + if (io->iobuf) + iobuf_normalize(io->iobuf); + #ifdef IO_SSL if (io->ssl) { io_reload_ssl(io); Index: mta_session.c === RCS file: /cvs/src/usr.sbin/smtpd/mta_session.c,v retrieving revision 1.88 diff -u -p -r1.88 mta_session.c --- mta_session.c 21 Nov 2016 13:00:43 - 1.88 +++ mta_session.c 21 Nov 2016 19:33:47 - @@ -1182,10 +1182,8 @@ mta_io(struct io *io, int evt, void *arg if (io_datalen(>io) >= LINE_MAX) { mta_error(s, "Input too long"); mta_free(s); - return; } - iobuf_normalize(>iobuf); - break; + return; } log_trace(TRACE_MTA, "mta: %p: <<< %s", s, line); @@ -1263,8 +1261,6 @@ mta_io(struct io *io, int evt, void *arg mta_connect(s); return; } - - iobuf_normalize(>iobuf); if (io_datalen(>io)) { log_debug("debug: mta: remaining data in input buffer"); Index: smtp_session.c === RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v retrieving revision 1.294 diff -u -p -r1.294 smtp_session.c --- smtp_session.c 21 Nov 2016 13:00:43 - 1.294 +++ smtp_session.c 21 Nov 2016 19:33:48 - @@ -1310,10 +1310,8 @@ smtp_io(struct io *io, int evt, void *ar } /* No complete line received */ - if (line == NULL) { - iobuf_normalize(>iobuf); + if (line == NULL) return; - } /* Message body */ if (s->state == STATE_BODY && strcmp(line, ".")) { @@ -1338,7 +1336,6 @@ smtp_io(struct io *io, int evt, void *ar rfc2822_parser_flush(>tx->rfc2822_parser); - iobuf_normalize(>iobuf); io_set_write(io); s->tx->dataeom = 1; @@ -1353,7 +1350,6 @@ smtp_io(struct io *io, int evt, void *ar (void)strlcpy(s->cmd, line, sizeof s->cmd); io_set_write(io); smtp_command(s, line); - iobuf_normalize(>iobuf); break; case IO_LOWAT:
smtpd: internal cleanups, part 3
Hi, Next step towards hiding the struct io internals. This diff adds new io_*() api functions for dealing with buffered data. They are simple wrappers around their iobuf_*() counterpart, with better names in some cases. The point is of course to be able remove the use of iobuf_*() in the rest of the daemon. Eric. Index: ioev.c === RCS file: /cvs/src/usr.sbin/smtpd/ioev.c,v retrieving revision 1.28 diff -u -p -r1.28 ioev.c --- ioev.c 17 Nov 2016 07:33:06 - 1.28 +++ ioev.c 17 Nov 2016 14:01:28 - @@ -354,6 +354,92 @@ io_set_write(struct io *io) io_reload(io); } +/* + * Buffered output functions + */ + +int +io_write(struct io *io, const void *buf, size_t len) +{ + return iobuf_queue(io->iobuf, buf, len); +} + +int +io_writev(struct io *io, const struct iovec *iov, int iovcount) +{ + return iobuf_queuev(io->iobuf, iov, iovcount); +} + +int +io_print(struct io *io, const char *s) +{ + return io_write(io, s, strlen(s)); +} + +int +io_printf(struct io *io, const char *fmt, ...) +{ + va_list ap; + int r; + + va_start(ap, fmt); + r = io_vprintf(io, fmt, ap); + va_end(ap); + + return r; +} + +int +io_vprintf(struct io *io, const char *fmt, va_list ap) +{ + + char *buf; + int len; + + len = vasprintf(, fmt, ap); + if (len == -1) + return -1; + len = io_write(io, buf, len); + free(buf); + + return len; +} + +size_t +io_queued(struct io *io) +{ + return iobuf_queued(io->iobuf); +} + +/* + * Buffered input functions + */ + +void * +io_data(struct io *io) +{ + return iobuf_data(io->iobuf); +} + +size_t +io_datalen(struct io *io) +{ + return iobuf_len(io->iobuf); +} + +char * +io_getline(struct io *io, size_t *sz) +{ + return iobuf_getline(io->iobuf, sz); +} + +void +io_drop(struct io *io, size_t sz) +{ + return iobuf_drop(io->iobuf, sz); +} + + #define IO_READING(io) (((io)->flags & IO_RW) != IO_WRITE) #define IO_WRITING(io) (((io)->flags & IO_RW) != IO_READ) @@ -428,12 +514,6 @@ size_t io_pending(struct io *io) { return iobuf_len(io->iobuf); -} - -size_t -io_queued(struct io *io) -{ - return iobuf_queued(io->iobuf); } const char* Index: ioev.h === RCS file: /cvs/src/usr.sbin/smtpd/ioev.h,v retrieving revision 1.8 diff -u -p -r1.8 ioev.h --- ioev.h 17 Nov 2016 07:33:06 - 1.8 +++ ioev.h 17 Nov 2016 14:01:28 - @@ -68,3 +68,17 @@ int io_connect(struct io *, const struct int io_start_tls(struct io *, void *); const char* io_strio(struct io *); const char* io_strevent(int); + +/* Buffered output functions */ +int io_write(struct io *, const void *, size_t); +int io_writev(struct io *, const struct iovec *, int); +int io_print(struct io *, const char *); +int io_printf(struct io *, const char *, ...); +int io_vprintf(struct io *, const char *, va_list); +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 *); +void io_drop(struct io *, size_t); Index: smtpd.h === RCS file: /cvs/src/usr.sbin/smtpd/smtpd.h,v retrieving revision 1.523 diff -u -p -r1.523 smtpd.h --- smtpd.h 4 Sep 2016 09:33:49 - 1.523 +++ smtpd.h 17 Nov 2016 14:01:28 - @@ -1494,6 +1494,8 @@ void *xmemdup(const void *, size_t, cons char *strip(char *); void iobuf_xinit(struct iobuf *, size_t, size_t, const char *); void iobuf_xfqueue(struct iobuf *, const char *, const char *, ...); +int io_xprint(struct io *, const char *); +int io_xprintf(struct io *, const char *, ...); void log_envelope(const struct envelope *, const char *, const char *, const char *); int session_socket_error(int); Index: util.c === RCS file: /cvs/src/usr.sbin/smtpd/util.c,v retrieving revision 1.128 diff -u -p -r1.128 util.c --- util.c 31 Aug 2016 10:18:08 - 1.128 +++ util.c 17 Nov 2016 14:01:28 - @@ -133,6 +133,33 @@ iobuf_xfqueue(struct iobuf *io, const ch fatalx("exiting"); } } + +int +io_xprintf(struct io *io, const char *fmt, ...) +{ + va_list ap; + int len; + + va_start(ap, fmt); + len = io_vprintf(io, fmt, ap); + va_end(ap); + if (len == -1) + fatal("io_xprintf(%p, %s, ...)", io, fmt); + + return len; +} + +int +io_xprint(struct io *io, const char *str) +{ + int len; + + len = io_print(io, str); + if (len == -1) + fatal("io_xprint(%p, %s, ...)", io, str); + + return len; +} #endif char *
smtpd: internal cleanups, part 2
This diff removes the IO_TLSVERIFIED which is not a io event, and inlines the necessary code where the callback functions are called for this event. Eric. Index: ioev.c === RCS file: /cvs/src/usr.sbin/smtpd/ioev.c,v retrieving revision 1.27 diff -u -p -r1.27 ioev.c --- ioev.c 16 Nov 2016 21:30:37 - 1.27 +++ ioev.c 16 Nov 2016 21:56:25 - @@ -118,7 +118,6 @@ io_strevent(int evt) switch (evt) { CASE(IO_CONNECTED); CASE(IO_TLSREADY); - CASE(IO_TLSVERIFIED); CASE(IO_DATAIN); CASE(IO_LOWAT); CASE(IO_DISCONNECTED); Index: ioev.h === RCS file: /cvs/src/usr.sbin/smtpd/ioev.h,v retrieving revision 1.7 diff -u -p -r1.7 ioev.h --- ioev.h 16 Nov 2016 21:30:37 - 1.7 +++ ioev.h 16 Nov 2016 21:56:25 - @@ -20,7 +20,6 @@ enum { IO_CONNECTED = 0, /* connection successful*/ IO_TLSREADY,/* TLS started successfully */ - IO_TLSVERIFIED, /* XXX - needs more work*/ IO_TLSERROR,/* XXX - needs more work*/ IO_DATAIN, /* new data in input buffer */ IO_LOWAT, /* output queue running low */ Index: mta_session.c === RCS file: /cvs/src/usr.sbin/smtpd/mta_session.c,v retrieving revision 1.84 diff -u -p -r1.84 mta_session.c --- mta_session.c 16 Nov 2016 21:30:37 - 1.84 +++ mta_session.c 16 Nov 2016 21:56:25 - @@ -259,6 +259,7 @@ mta_session_imsg(struct mproc *p, struct const char *name; void*ssl; int dnserror, status; + X509*x; switch (imsg->hdr.type) { @@ -363,7 +364,22 @@ mta_session_imsg(struct mproc *p, struct return; } - mta_io(>io, IO_TLSVERIFIED, s->io.arg); + x = SSL_get_peer_certificate(s->io.ssl); + if (x) { + log_info("smtp-out: Server certificate verification %s " + "on session %016"PRIx64, + (s->flags & MTA_VERIFIED) ? "succeeded" : "failed", + s->id); + X509_free(x); + } + + if (s->use_smtps) { + mta_enter_state(s, MTA_BANNER); + io_set_read(>io); + } + else + mta_enter_state(s, MTA_EHLO); + io_resume(>io, IO_PAUSE_IN); io_reload(>io); return; @@ -1141,7 +1157,6 @@ mta_io(struct io *io, int evt, void *arg size_t len; const char *error; int cont; - X509*x; log_trace(TRACE_IO, "mta: %p: %s %s", s, io_strevent(evt), io_strio(io)); @@ -1170,24 +1185,6 @@ mta_io(struct io *io, int evt, void *arg io_pause(>io, IO_PAUSE_IN); break; } - - case IO_TLSVERIFIED: - x = SSL_get_peer_certificate(s->io.ssl); - if (x) { - log_info("smtp-out: Server certificate verification %s " - "on session %016"PRIx64, - (s->flags & MTA_VERIFIED) ? "succeeded" : "failed", - s->id); - X509_free(x); - } - - if (s->use_smtps) { - mta_enter_state(s, MTA_BANNER); - io_set_read(io); - } - else - mta_enter_state(s, MTA_EHLO); - break; case IO_DATAIN: nextline: Index: smtp_session.c === RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v retrieving revision 1.290 diff -u -p -r1.290 smtp_session.c --- smtp_session.c 16 Nov 2016 21:30:37 - 1.290 +++ smtp_session.c 16 Nov 2016 21:56:26 - @@ -698,6 +698,7 @@ smtp_session_imsg(struct mproc *p, struc uint32_t msgid; int status, success, dnserror; void*ssl_ctx; + X509*x; switch (imsg->hdr.type) { case IMSG_SMTP_DNS_PTR: @@ -993,7 +994,26 @@ smtp_session_imsg(struct mproc *p, struc smtp_free(s, "SSL certificate check failed"); return; } - smtp_io(>io, IO_TLSVERIFIED, s->io.arg); + + x = SSL_get_peer_certificate(s->io.ssl); + if (x) { +
smtpd: internal cleanups
Hi, I'm working on improving the async io interface in smtpd, make it simpler to use and less error-prone. The short-term goal is to make the io structure opaque. With this first diff, the user pointer is passed as parameter to the io callback instead of having the user dereference the io structure. There are places where the callback function is triggered outside of the io layer. It's not desirable, and it needs to be fixed in a separate diff. Eric. Index: bounce.c === RCS file: /cvs/src/usr.sbin/smtpd/bounce.c,v retrieving revision 1.72 diff -u -p -r1.72 bounce.c --- bounce.c3 Feb 2016 05:57:09 - 1.72 +++ bounce.c16 Nov 2016 19:35:54 - @@ -97,7 +97,7 @@ static int bounce_next_message(struct b static int bounce_next(struct bounce_session *); static void bounce_delivery(struct bounce_message *, int, const char *); static void bounce_status(struct bounce_session *, const char *, ...); -static void bounce_io(struct io *, int); +static void bounce_io(struct io *, int, void *); static void bounce_timeout(int, short, void *); static void bounce_free(struct bounce_session *); static const char *action_str(const struct delivery_bounce *); @@ -712,9 +712,9 @@ bounce_free(struct bounce_session *s) } static void -bounce_io(struct io *io, int evt) +bounce_io(struct io *io, int evt, void *arg) { - struct bounce_session *s = io->arg; + struct bounce_session *s = arg; const char *error; char*line, *msg; int cont; Index: filter.c === RCS file: /cvs/src/usr.sbin/smtpd/filter.c,v retrieving revision 1.19 diff -u -p -r1.19 filter.c --- filter.c29 Jun 2016 06:46:06 - 1.19 +++ filter.c16 Nov 2016 19:35:54 - @@ -114,7 +114,7 @@ static void filter_run_query(struct filt static void filter_end_query(struct filter_query *); static void filter_set_sink(struct filter_session *, int); static int filter_tx(struct filter_session *, int); -static void filter_tx_io(struct io *, int); +static void filter_tx_io(struct io *, int, void *); static TAILQ_HEAD(, filter_proc) procs; struct dictchains; @@ -678,9 +678,9 @@ filter_tx(struct filter_session *s, int } static void -filter_tx_io(struct io *io, int evt) +filter_tx_io(struct io *io, int evt, void *arg) { - struct filter_session *s = io->arg; + struct filter_session *s = arg; size_t len, n; char*data; Index: ioev.c === RCS file: /cvs/src/usr.sbin/smtpd/ioev.c,v retrieving revision 1.26 diff -u -p -r1.26 ioev.c --- ioev.c 16 May 2016 21:43:16 - 1.26 +++ ioev.c 16 Nov 2016 19:35:54 - @@ -226,7 +226,7 @@ _io_init() void io_init(struct io *io, int sock, void *arg, - void(*cb)(struct io*, int), struct iobuf *iobuf) + void(*cb)(struct io*, int, void *), struct iobuf *iobuf) { _io_init(); @@ -580,7 +580,7 @@ leave: void io_callback(struct io *io, int evt) { - io->cb(io, evt); + io->cb(io, evt, io->arg); } int Index: ioev.h === RCS file: /cvs/src/usr.sbin/smtpd/ioev.h,v retrieving revision 1.6 diff -u -p -r1.6 ioev.h --- ioev.h 25 Mar 2016 15:06:58 - 1.6 +++ ioev.h 16 Nov 2016 19:35:54 - @@ -41,7 +41,7 @@ struct iobuf; struct io { int sock; void*arg; - void(*cb)(struct io*, int); + void(*cb)(struct io*, int, void *); struct iobuf*iobuf; size_t lowat; int timeout; @@ -55,7 +55,8 @@ struct io { void io_set_nonblocking(int); void io_set_nolinger(int); -void io_init(struct io*, int, void*, void(*)(struct io*, int), struct iobuf*); +void io_init(struct io*, int, void*, void(*)(struct io*, int, void *), +struct iobuf*); void io_clear(struct io*); void io_set_read(struct io *); void io_set_write(struct io *); Index: mda.c === RCS file: /cvs/src/usr.sbin/smtpd/mda.c,v retrieving revision 1.120 diff -u -p -r1.120 mda.c --- mda.c 1 Sep 2016 15:12:45 - 1.120 +++ mda.c 16 Nov 2016 19:35:55 - @@ -84,7 +84,7 @@ struct mda_session { FILE*datafp; }; -static void mda_io(struct io *, int); +static void mda_io(struct io *, int, void *); static int mda_check_loop(FILE *, struct mda_envelope *); static int mda_getlastline(int, char *, size_t); static void mda_done(struct mda_session *); @@ -496,9 +496,9 @@ mda_postprivdrop() } static void -mda_io(struct io *io, int evt) +mda_io(struct io *io, int evt, void *arg) { - struct mda_session
smtpd config parsing cleanup
Because of the small ad hoc changes that were made here and there over the years, the listener config code has become a bit convoluted I think. Here is a little cleanup to: - have all listener creation functions take listen_opts as param, and call config_listener() when done, which adds the listener(s) to the current config list of listeners. - make the fallback chain between interface(), host_v4() host_v6() and host_dns() obvious when creating an if_listener. - fix a bug where the specified family was ignored if the listener is given as a hostname. Comments? Eric. Index: parse.y === RCS file: /cvs/src/usr.sbin/smtpd/parse.y,v retrieving revision 1.189 diff -u -p -r1.189 parse.y --- parse.y 31 Aug 2016 15:24:04 - 1.189 +++ parse.y 9 Sep 2016 11:11:54 - @@ -138,15 +138,14 @@ static struct listen_opts { uint32_toptions; } listen_opts; -static struct listener *create_sock_listener(struct listen_opts *); -static void create_if_listener(struct listenerlist *, struct listen_opts *); -static void config_listener(struct listener *, struct listen_opts *); - -struct listener*host_v4(const char *, in_port_t); -struct listener*host_v6(const char *, in_port_t); -int host_dns(struct listenerlist *, struct listen_opts *); -int host(struct listenerlist *, struct listen_opts *); -int interface(struct listenerlist *, struct listen_opts *); +static voidcreate_sock_listener(struct listen_opts *); +static voidcreate_if_listener(struct listen_opts *); +static voidconfig_listener(struct listener *, struct listen_opts *); +static int host_v4(struct listen_opts *); +static int host_v6(struct listen_opts *); +static int host_dns(struct listen_opts *); +static int interface(struct listen_opts *); + voidset_local(const char *); voidset_localaddrs(struct table *); int delaytonum(char *); @@ -695,13 +694,13 @@ socket_listener : SOCKET sock_listen { yyerror("socket listener already configured"); YYERROR; } - conf->sc_sock_listener = create_sock_listener(_opts); + create_sock_listener(_opts); } ; if_listener: STRING if_listen { listen_opts.ifx = $1; - create_if_listener(conf->sc_listeners, _opts); + create_if_listener(_opts); } ; @@ -2005,7 +2004,7 @@ parse_config(struct smtpd *x_conf, const /* If the socket listener was not configured, create a default one. */ if (!conf->sc_sock_listener) { memset(_opts, 0, sizeof listen_opts); - conf->sc_sock_listener = create_sock_listener(_opts); + create_sock_listener(_opts); } /* Free macros and check which have not been used. */ @@ -2109,7 +2108,7 @@ symget(const char *nam) return (NULL); } -static struct listener * +static void create_sock_listener(struct listen_opts *lo) { struct listener *l = xcalloc(1, sizeof(*l), "create_sock_listener"); @@ -2118,13 +2117,12 @@ create_sock_listener(struct listen_opts l->ss.ss_family = AF_LOCAL; l->ss.ss_len = sizeof(struct sockaddr *); l->local = 1; + conf->sc_sock_listener = l; config_listener(l, lo); - - return (l); } static void -create_if_listener(struct listenerlist *ll, struct listen_opts *lo) +create_if_listener(struct listen_opts *lo) { uint16_tflags; @@ -2142,17 +2140,11 @@ create_if_listener(struct listenerlist * if (lo->port) { lo->flags = lo->ssl|lo->auth|flags; lo->port = htons(lo->port); - if (!interface(ll, lo)) - if (host(ll, lo) <= 0) - errx(1, "invalid virtual ip or interface: %s", lo->ifx); } else { if (lo->ssl & F_SMTPS) { lo->port = htons(465); lo->flags = F_SMTPS|lo->auth|flags; - if (!interface(ll, lo)) - if (host(ll, lo) <= 0) - errx(1, "invalid virtual ip or interface: %s", lo->ifx); } if (!lo->ssl || (lo->ssl & F_STARTTLS)) { @@ -2160,11 +2152,19 @@ create_if_listener(struct listenerlist * lo->flags = lo->auth|flags; if (lo->ssl & F_STARTTLS) lo->flags |= F_STARTTLS; - if (!interface(ll, lo)) - if (host(ll, lo) <= 0) - errx(1, "invalid virtual ip or interface:
smtpd shutdown cleanup
Previously, all processes would shutdown on receiving SIGINT or SIGTERM. When going down, the parent process would kill all the other process and waitpid() them. Now, only the parent process handles SIGTERM and SIGINT, other processes ignore them. Upon receiving one of these signals, the parent process all imsg sockets and waitpid() for the children. It fatal()s if one of the imsg sockets is closed unexpectedly. Other processes exit() "normally" when one of their imsg socket is closed (except for client connection on the control socket of course). That's how they are supposed to stop now. When doing so, they log as "debug" instead of "info" because useless logs are useless. This makes the shutdown sequence much saner. Eric. Index: ca.c === RCS file: /cvs/src/usr.sbin/smtpd/ca.c,v retrieving revision 1.24 diff -u -p -r1.24 ca.c --- ca.c4 Sep 2016 16:10:31 - 1.24 +++ ca.c6 Sep 2016 19:33:45 - @@ -66,29 +66,14 @@ static uint64_t rsae_reqid = 0; static void ca_shutdown(void) { - log_info("info: ca agent exiting"); + log_debug("debug: ca agent exiting"); _exit(0); } -static void -ca_sig_handler(int sig, short event, void *p) -{ - switch (sig) { - case SIGINT: - case SIGTERM: - ca_shutdown(); - break; - default: - fatalx("ca_sig_handler: unexpected signal"); - } -} - int ca(void) { struct passwd *pw; - struct event ev_sigint; - struct event ev_sigterm; purge_config(PURGE_LISTENERS|PURGE_TABLES|PURGE_RULES); @@ -110,10 +95,8 @@ ca(void) imsg_callback = ca_imsg; event_init(); - signal_set(_sigint, SIGINT, ca_sig_handler, NULL); - signal_set(_sigterm, SIGTERM, ca_sig_handler, NULL); - signal_add(_sigint, NULL); - signal_add(_sigterm, NULL); + signal(SIGINT, SIG_IGN); + signal(SIGTERM, SIG_IGN); signal(SIGPIPE, SIG_IGN); signal(SIGHUP, SIG_IGN); @@ -242,6 +225,9 @@ ca_imsg(struct mproc *p, struct imsg *im int ret = 0; uint64_t id; int v; + + if (imsg == NULL) + ca_shutdown(); if (p->proc == PROC_PARENT) { switch (imsg->hdr.type) { Index: control.c === RCS file: /cvs/src/usr.sbin/smtpd/control.c,v retrieving revision 1.116 diff -u -p -r1.116 control.c --- control.c 4 Sep 2016 16:10:31 - 1.116 +++ control.c 6 Sep 2016 19:33:45 - @@ -63,7 +63,6 @@ static void control_shutdown(void); static void control_listen(void); static void control_accept(int, short, void *); static void control_close(struct ctl_conn *); -static void control_sig_handler(int, short, void *); static void control_dispatch_ext(struct mproc *, struct imsg *); static void control_digest_update(const char *, size_t, int); static void control_broadcast_verbose(int, int); @@ -89,6 +88,12 @@ control_imsg(struct mproc *p, struct ims const void *data; size_t sz; + if (imsg == NULL) { + if (p->proc != PROC_CLIENT) + control_shutdown(); + return; + } + if (p->proc == PROC_PONY) { switch (imsg->hdr.type) { case IMSG_CTL_SMTP_SESSION: @@ -186,19 +191,6 @@ control_imsg(struct mproc *p, struct ims imsg_to_str(imsg->hdr.type)); } -static void -control_sig_handler(int sig, short event, void *p) -{ - switch (sig) { - case SIGINT: - case SIGTERM: - control_shutdown(); - break; - default: - fatalx("control_sig_handler: unexpected signal"); - } -} - int control_create_socket(void) { @@ -245,8 +237,6 @@ int control(void) { struct passwd *pw; - struct event ev_sigint; - struct event ev_sigterm; purge_config(PURGE_EVERYTHING); @@ -271,10 +261,8 @@ control(void) imsg_callback = control_imsg; event_init(); - signal_set(_sigint, SIGINT, control_sig_handler, NULL); - signal_set(_sigterm, SIGTERM, control_sig_handler, NULL); - signal_add(_sigint, NULL); - signal_add(_sigterm, NULL); + signal(SIGINT, SIG_IGN); + signal(SIGTERM, SIG_IGN); signal(SIGPIPE, SIG_IGN); signal(SIGHUP, SIG_IGN); @@ -305,7 +293,7 @@ control(void) static void control_shutdown(void) { - log_info("info: control process exiting"); + log_debug("debug: control agent exiting"); _exit(0); } Index: lka.c === RCS file: /cvs/src/usr.sbin/smtpd/lka.c,v retrieving revision 1.196 diff -u -p -r1.196 lka.c --- lka.c 4 Sep
Another step in cleaning the smtpd exit path.
The smtpd processes are not expected to ever leave their event loop. So stop pretending that the *_shutdown() functions could ever be called in this context, and just fatal() if event_dispatch() returns. Eric. Index: ca.c === RCS file: /cvs/src/usr.sbin/smtpd/ca.c,v retrieving revision 1.23 diff -u -p -r1.23 ca.c --- ca.c1 Sep 2016 10:54:25 - 1.23 +++ ca.c4 Sep 2016 14:37:31 - @@ -127,9 +127,8 @@ ca(void) if (pledge("stdio", NULL) == -1) err(1, "pledge"); - if (event_dispatch() < 0) - fatal("event_dispatch"); - ca_shutdown(); + event_dispatch(); + fatalx("exited event loop"); return (0); } Index: control.c === RCS file: /cvs/src/usr.sbin/smtpd/control.c,v retrieving revision 1.115 diff -u -p -r1.115 control.c --- control.c 4 Sep 2016 09:33:49 - 1.115 +++ control.c 4 Sep 2016 14:37:31 - @@ -296,9 +296,8 @@ control(void) if (pledge("stdio unix recvfd sendfd", NULL) == -1) err(1, "pledge"); - if (event_dispatch() < 0) - fatal("event_dispatch"); - control_shutdown(); + event_dispatch(); + fatalx("exited event loop"); return (0); } Index: lka.c === RCS file: /cvs/src/usr.sbin/smtpd/lka.c,v retrieving revision 1.195 diff -u -p -r1.195 lka.c --- lka.c 3 Sep 2016 15:54:14 - 1.195 +++ lka.c 4 Sep 2016 14:37:31 - @@ -448,9 +448,8 @@ lka(void) if (pledge("stdio rpath inet dns getpw recvfd proc exec", NULL) == -1) err(1, "pledge"); - if (event_dispatch() < 0) - fatal("event_dispatch"); - lka_shutdown(); + event_dispatch(); + fatalx("exited event loop"); return (0); } Index: pony.c === RCS file: /cvs/src/usr.sbin/smtpd/pony.c,v retrieving revision 1.14 diff -u -p -r1.14 pony.c --- pony.c 1 Sep 2016 10:54:25 - 1.14 +++ pony.c 4 Sep 2016 14:37:31 - @@ -209,9 +209,8 @@ pony(void) if (pledge("stdio inet unix recvfd sendfd", NULL) == -1) err(1, "pledge"); - if (event_dispatch() < 0) - fatal("event_dispatch"); - pony_shutdown(); + event_dispatch(); + fatalx("exited event loop"); return (0); } Index: queue.c === RCS file: /cvs/src/usr.sbin/smtpd/queue.c,v retrieving revision 1.180 diff -u -p -r1.180 queue.c --- queue.c 1 Sep 2016 10:54:25 - 1.180 +++ queue.c 4 Sep 2016 14:37:31 - @@ -720,9 +720,8 @@ queue(void) if (pledge("stdio rpath wpath cpath flock recvfd sendfd", NULL) == -1) err(1, "pledge"); - if (event_dispatch() < 0) - fatal("event_dispatch"); - queue_shutdown(); + event_dispatch(); + fatalx("exited event loop"); return (0); } Index: scheduler.c === RCS file: /cvs/src/usr.sbin/smtpd/scheduler.c,v retrieving revision 1.53 diff -u -p -r1.53 scheduler.c --- scheduler.c 1 Sep 2016 10:54:25 - 1.53 +++ scheduler.c 4 Sep 2016 14:37:31 - @@ -489,9 +489,8 @@ scheduler(void) if (pledge("stdio", NULL) == -1) err(1, "pledge"); - if (event_dispatch() < 0) - fatal("event_dispatch"); - scheduler_shutdown(); + event_dispatch(); + fatalx("exited event loop"); return (0); } Index: smtpd.c === RCS file: /cvs/src/usr.sbin/smtpd/smtpd.c,v retrieving revision 1.283 diff -u -p -r1.283 smtpd.c --- smtpd.c 4 Sep 2016 09:33:49 - 1.283 +++ smtpd.c 4 Sep 2016 14:37:31 - @@ -1050,8 +1050,8 @@ smtpd(void) { "getpw sendfd proc exec id inet unix", NULL) == -1) err(1, "pledge"); - if (event_dispatch() < 0) - fatal("smtpd: event_dispatch"); + event_dispatch(); + fatalx("exited event loop"); return (0); }
Re: smtpctl stop
On Sat, Sep 03, 2016 at 08:12:20PM +0200, Gilles Chehade wrote: > On Sat, Sep 03, 2016 at 08:09:25PM +0200, Eric Faurot wrote: > > Hi, > > > > Here is a diff to remove the "smtpctl stop" command. > > The proper way to stop a daemon is kill(1)/pkill(1) only. > > It makes no sense to have different code path for that. > > > > much agreed, queue code was designed to be kill-safe, the mta doesn't > care because it will retry and the smtp code can catch & 421 Well, we can still implement a graceful shutdown if needed, but it should be triggered by the signal. Eric.
smtpctl stop
Hi, Here is a diff to remove the "smtpctl stop" command. The proper way to stop a daemon is kill(1)/pkill(1) only. It makes no sense to have different code path for that. Eric. Index: control.c === RCS file: /cvs/src/usr.sbin/smtpd/control.c,v retrieving revision 1.114 diff -u -p -r1.114 control.c --- control.c 1 Sep 2016 10:54:25 - 1.114 +++ control.c 3 Sep 2016 17:42:37 - @@ -485,7 +485,7 @@ control_dispatch_ext(struct mproc *p, st switch (imsg->hdr.type) { case IMSG_CTL_SMTP_SESSION: - if (env->sc_flags & (SMTPD_SMTP_PAUSED | SMTPD_EXITING)) { + if (env->sc_flags & SMTPD_SMTP_PAUSED) { m_compose(p, IMSG_CTL_FAIL, 0, 0, -1, NULL, 0); return; } @@ -511,22 +511,6 @@ control_dispatch_ext(struct mproc *p, st kvp->val = val; } m_compose(p, IMSG_CTL_GET_STATS, 0, 0, -1, kvp, sizeof *kvp); - return; - - case IMSG_CTL_SHUTDOWN: - /* NEEDS_FIX */ - log_debug("debug: received shutdown request"); - - if (c->euid) - goto badcred; - - if (env->sc_flags & SMTPD_EXITING) { - m_compose(p, IMSG_CTL_FAIL, 0, 0, -1, NULL, 0); - return; - } - env->sc_flags |= SMTPD_EXITING; - m_compose(p, IMSG_CTL_OK, 0, 0, -1, NULL, 0); - m_compose(p_parent, IMSG_CTL_SHUTDOWN, 0, 0, -1, NULL, 0); return; case IMSG_CTL_VERBOSE: Index: smtpctl.8 === RCS file: /cvs/src/usr.sbin/smtpd/smtpctl.8,v retrieving revision 1.57 diff -u -p -r1.57 smtpctl.8 --- smtpctl.8 14 Jun 2016 22:40:48 - 1.57 +++ smtpctl.8 3 Sep 2016 17:42:37 - @@ -245,8 +245,6 @@ Displays runtime statistics concerning .Xr smtpd 8 . .It Cm show status Shows if MTA, MDA and SMTP systems are currently running or paused. -.It Cm stop -Stop the server. .It Cm trace Ar subsystem Enables real-time tracing of .Ar subsystem . Index: smtpctl.c === RCS file: /cvs/src/usr.sbin/smtpd/smtpctl.c,v retrieving revision 1.150 diff -u -p -r1.150 smtpctl.c --- smtpctl.c 3 Sep 2016 16:06:26 - 1.150 +++ smtpctl.c 3 Sep 2016 17:42:37 - @@ -864,13 +864,6 @@ do_show_status(int argc, struct paramete } static int -do_stop(int argc, struct parameter *argv) -{ - srv_send(IMSG_CTL_SHUTDOWN, NULL, 0); - return srv_check_result(1); -} - -static int do_trace(int argc, struct parameter *argv) { int v; @@ -1080,7 +1073,6 @@ main(int argc, char **argv) cmd_install("show routes", do_show_routes); cmd_install("show stats", do_show_stats); cmd_install("show status", do_show_status); - cmd_install("stop", do_stop); cmd_install("trace ", do_trace); cmd_install("uncorrupt ",do_uncorrupt); cmd_install("unprofile ", do_unprofile); Index: smtpd.c === RCS file: /cvs/src/usr.sbin/smtpd/smtpd.c,v retrieving revision 1.282 diff -u -p -r1.282 smtpd.c --- smtpd.c 1 Sep 2016 10:54:25 - 1.282 +++ smtpd.c 3 Sep 2016 17:42:37 - @@ -257,10 +257,6 @@ parent_imsg(struct mproc *p, struct imsg m_end(); profiling = v; return; - - case IMSG_CTL_SHUTDOWN: - parent_shutdown(0); - return; } } @@ -1752,7 +1748,6 @@ imsg_to_str(int type) CASE(IMSG_CTL_REMOVE); CASE(IMSG_CTL_SCHEDULE); CASE(IMSG_CTL_SHOW_STATUS); - CASE(IMSG_CTL_SHUTDOWN); CASE(IMSG_CTL_TRACE_DISABLE); CASE(IMSG_CTL_TRACE_ENABLE); CASE(IMSG_CTL_UPDATE_TABLE); Index: smtpd.h === RCS file: /cvs/src/usr.sbin/smtpd/smtpd.h,v retrieving revision 1.522 diff -u -p -r1.522 smtpd.h --- smtpd.h 3 Sep 2016 16:06:26 - 1.522 +++ smtpd.h 3 Sep 2016 17:42:37 - @@ -161,7 +161,7 @@ union lookup { * Bump IMSG_VERSION whenever a change is made to enum imsg_type. * This will ensure that we can never use a wrong version of smtpctl with smtpd. */ -#defineIMSG_VERSION15 +#defineIMSG_VERSION16 enum imsg_type { IMSG_NONE, @@ -195,7 +195,6 @@ enum imsg_type { IMSG_CTL_REMOVE, IMSG_CTL_SCHEDULE, IMSG_CTL_SHOW_STATUS, - IMSG_CTL_SHUTDOWN, IMSG_CTL_TRACE_DISABLE, IMSG_CTL_TRACE_ENABLE, IMSG_CTL_UPDATE_TABLE, @@ -572,7
Re: smtpd 5.9.1 vs 6.0 & possible corner case / regression
Can you guys try this diff? Eric. Index: rfc2822.c === RCS file: /cvs/src/usr.sbin/smtpd/rfc2822.c,v retrieving revision 1.7 diff -u -p -r1.7 rfc2822.c --- rfc2822.c 4 Feb 2016 22:35:17 - 1.7 +++ rfc2822.c 20 Aug 2016 10:53:27 - @@ -138,6 +138,7 @@ rfc2822_parser_init(struct rfc2822_parse TAILQ_INIT(>header.lines); rfc2822_header_default_callback(rp, hdr_dflt_cb, NULL); rfc2822_body_callback(rp, body_dflt_cb, NULL); + rfc2822_parser_reset(rp); } void
Re: Bug in gethostbyaddr and patch to solve
On Mon, Aug 25, 2014 at 10:39:59PM -0500, Vladimir Támara Patiño wrote: Using tcpdump in a firewall with 5.5 (also happens with 5.4 and I guess with current) and certain addres of the LAN I got always a segfault. It is a bug within the function gethostbyaddr. It can be reproduced with the minimal test program available at: http://openbsd.7691.n7.nabble.com/problem-with-gethostbyaddr-on-OBSD-5-4-td242329.html and the following steps: 1. Create a entry in /etc/hosts with IP address but without name, for example: echo 192.168.1.89 /etc/hosts 2. Compile the test program of the link cc -o gethostbyaddr gethostbyaddr.c 3. Run de test program with the address added to /etc/hosts without name: ./gethostbyaddr 192.168.1.89 This bug was fixed some times ago. http://openbsd.cs.toronto.edu/cgi-bin/cvsweb/src/lib/libc/asr/gethostnamadr_async.c.diff?r1=1.28r2=1.29f=h Eric.
Re: problem with gethostbyaddr() on OBSD 5.4?
On Sun, Feb 02, 2014 at 03:12:36PM +0100, IMAP List Administration wrote: [I forgot to send this to the list] Hi Eric, On 02/01/2014 11:43 AM, Eric Faurot wrote: The following diff fixes the problems with the example IPs you gave us. - subsequent PTR records are now set as aliases in the hostent - need to accept '/' in dname labels (maybe others?) Since the code differs, I'm guessing your patch is for -current. We're running -stable. Could you possibly supply a patch for that? cheers, Rob Urban Hi, This is the diff against -stable. Eric. Index: asr_utils.c === RCS file: /cvs/src/lib/libc/asr/asr_utils.c,v retrieving revision 1.8 diff -u -p -r1.8 asr_utils.c --- asr_utils.c 12 Jul 2013 14:36:21 - 1.8 +++ asr_utils.c 3 Feb 2014 09:44:29 - @@ -55,7 +55,7 @@ dname_check_label(const char *s, size_t return (-1); for (l--; l; l--, s++) - if (!(isalnum(*s) || *s == '_' || *s == '-')) + if (!(isalnum(*s) || *s == '_' || *s == '-' || *s == '/')) return (-1); return (0); Index: gethostnamadr_async.c === RCS file: /cvs/src/lib/libc/asr/gethostnamadr_async.c,v retrieving revision 1.22 diff -u -p -r1.22 gethostnamadr_async.c --- gethostnamadr_async.c 17 Jul 2013 07:43:23 - 1.22 +++ gethostnamadr_async.c 3 Feb 2014 09:44:30 - @@ -504,8 +504,7 @@ hostent_from_packet(int reqtype, int fam if (strcasecmp(rr.rr_dname, dname) != 0) continue; if (hostent_set_cname(h, rr.rr.ptr.ptrname, 1) == -1) - goto fail; - /* XXX See if we need MULTI_PTRS_ARE_ALIASES */ + hostent_add_alias(h, rr.rr.ptr.ptrname, 1); break; case T_A:
Re: problem with gethostbyaddr() on OBSD 5.4?
On Sat, Feb 01, 2014 at 01:17:21AM +0100, IMAP List Administration wrote: Hello Folks, I run a Postfix MTA on OpenBSD. Recently I migrated the server from OBSD v5.3 to v5.4. Soon afterwards I noticed postfix was falsely rejecting mails based on a FCrDNS (forward-confirmed reverse DNS) test. FCrDNS means the DNS configuration of a connecting client is tested for forward and reverse DNS consistency. I first suspected a change in Postfix, but the developer (Wietse Venema) ruled out any changes to this Postfix functionality. Further investigation shows that gethostbyaddr() behaves differently on OBSD 5.3 and 5.4. The problem seems to manifest itself when the DNS configuration of a client is non-trivial, e.g., when there are multiple PTR records, or when there is a CNAME record which must be resolved before a PTR lookup can be performed. I tested using a slightly modified Postfix utility (gethostbyaddr.c) which I attach below. On OBSD 5.4 this program returns correct results for trivial DNS client configurations, but host address not found for non-trivial ones. On OBSD 5.3 the program returns correct results in all cases. As far as I can tell, the two OBSD systems are configured identically. For example, /etc/resolv.conf has the same lookup order (lookup file bind), and the same nameserver. DNS tools such as host, dig, or Net::DNS return correct results. Here are some examples of IP-addresses that illustrate the problem: 195.234.50.30 72.26.200.202 96.47.67.46 173.231.138.204 To summarize, gethostbyaddr() on OBSD 5.4 does not seem to be behaving properly and not as it did on 5.3. Can anyone confirm this? cheers, Rob Urban Hi, Thanks for your report. The following diff fixes the problems with the example IPs you gave us. - subsequent PTR records are now set as aliases in the hostent - need to accept '/' in dname labels (maybe others?) Please check if it works for you. Eric. Index: asr_utils.c === RCS file: /cvs/src/lib/libc/asr/asr_utils.c,v retrieving revision 1.9 diff -u -p -u -r1.9 asr_utils.c --- asr_utils.c 24 Nov 2013 23:51:29 - 1.9 +++ asr_utils.c 1 Feb 2014 10:34:23 - @@ -55,7 +55,7 @@ dname_check_label(const char *s, size_t return (-1); for (l--; l; l--, s++) - if (!(isalnum((unsigned char)*s) || *s == '_' || *s == '-')) + if (!(isalnum((unsigned char)*s) || *s == '_' || *s == '-' || *s == '/')) return (-1); return (0); Index: gethostnamadr_async.c === RCS file: /cvs/src/lib/libc/asr/gethostnamadr_async.c,v retrieving revision 1.23 diff -u -p -u -r1.23 gethostnamadr_async.c --- gethostnamadr_async.c 24 Nov 2013 23:51:29 - 1.23 +++ gethostnamadr_async.c 1 Feb 2014 10:34:24 - @@ -505,8 +505,7 @@ hostent_from_packet(int reqtype, int fam if (strcasecmp(rr.rr_dname, dname) != 0) continue; if (hostent_set_cname(h, rr.rr.ptr.ptrname, 1) == -1) - goto fail; - /* XXX See if we need MULTI_PTRS_ARE_ALIASES */ + hostent_add_alias(h, rr.rr.ptr.ptrname, 1); break; case T_A:
make async resolver API public
Hi, As suggested by theo@, it is probably time to make the async resolver API public, so that people can start using it. So, here is a diff that does two things (will be committed separately). First, move asr.h to include/ and install the manpages. It only exposes the API that has been in libc for a while now, so there is no new symbol strictly speaking, but maybe it still deserves a minor bump. The other thing is to add a simple wrapper to our libevent, so that libevent programs can immediatly and easily benefit from the async resolver. You have to run make includes in /usr/src/include before building. Comments welcome. Eric. Index: include/Makefile === RCS file: /cvs/src/include/Makefile,v retrieving revision 1.181 diff -u -p -r1.181 Makefile --- include/Makefile8 Dec 2013 17:05:09 - 1.181 +++ include/Makefile26 Dec 2013 16:50:20 - @@ -9,7 +9,7 @@ .include bsd.own.mk -FILES= a.out.h ar.h assert.h bitstring.h blf.h bm.h bsd_auth.h \ +FILES= a.out.h ar.h asr.h assert.h bitstring.h blf.h bm.h bsd_auth.h \ complex.h cpio.h ctype.h curses.h db.h dbm.h des.h dirent.h disktab.h \ dlfcn.h elf_abi.h err.h errno.h fenv.h float.h fnmatch.h fstab.h fts.h \ ftw.h getopt.h glob.h grp.h ifaddrs.h inttypes.h iso646.h kvm.h \ Index: include/asr.h === RCS file: include/asr.h diff -N include/asr.h --- /dev/null 1 Jan 1970 00:00:00 - +++ include/asr.h 26 Dec 2013 16:50:20 - @@ -0,0 +1,100 @@ +/* $OpenBSD: asr.h,v 1.7 2013/07/12 14:36:21 eric Exp $*/ +/* + * Copyright (c) 2012 Eric Faurot e...@openbsd.org + * + * 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 sys/types.h +#include sys/socket.h +#include netdb.h +#include netinet/in.h + +/* + * This part is the generic API for the async mechanism. It could be useful + * beyond the resolver. + */ + +/* Return values for async_run() */ +#define ASYNC_COND 0 /* wait for fd condition */ +#define ASYNC_YIELD1 /* partial result */ +#define ASYNC_DONE 2 /* done */ + +/* Expected fd conditions */ +#define ASYNC_READ 1 +#define ASYNC_WRITE2 + +/* This opaque structure holds an async query state. */ +struct async; + +/* + * This is the structure through which async_run() returns async + * results to the caller. + */ +struct async_res { + int ar_cond; + int ar_fd; + int ar_timeout; + + int ar_errno; + int ar_h_errno; + int ar_gai_errno; + int ar_rrset_errno; + + int ar_count; + + int ar_rcode; + void*ar_data; + int ar_datalen; + union { + struct sockaddr sa; + struct sockaddr_in sain; + struct sockaddr_in6 sain6; + }ar_sa; + + struct addrinfo *ar_addrinfo; + struct rrsetinfo *ar_rrsetinfo; + struct hostent *ar_hostent; + struct netent*ar_netent; +}; + +int asr_async_run(struct async *, struct async_res *); +int asr_async_run_sync(struct async *, struct async_res *); +void asr_async_abort(struct async *); + +/* This opaque structure holds an async resolver context. */ +struct asr; + +struct asr *asr_resolver(const char *); +void asr_resolver_done(struct asr *); + +/* Async version of the resolver API */ + +struct async *res_send_async(const unsigned char *, int, struct asr *); +struct async *res_query_async(const char *, int, int, struct asr *); +struct async *res_search_async(const char *, int, int, struct asr *); + +struct async *getrrsetbyname_async(const char *, unsigned int, unsigned int, +unsigned int, struct asr *); + +struct async *gethostbyname_async(const char *, struct asr *); +struct async *gethostbyname2_async(const char *, int, struct asr *); +struct async *gethostbyaddr_async(const void *, socklen_t, int, struct asr *); + +struct async *getnetbyname_async(const char *, struct asr *); +struct async *getnetbyaddr_async(in_addr_t, int, struct asr *); + +struct async *getaddrinfo_async(const char *, const char *, +const struct addrinfo *, struct asr *); +struct async *getnameinfo_async(const struct sockaddr
Re: [UPDATE] www/papers/index.html - Eric's OpenSMTPd presentation
On Tue, Apr 02, 2013 at 01:17:03PM -0400, Nick Holland wrote: On 04/02/2013 09:20 AM, Jiri B wrote: Index: index.html === RCS file: /cvs/www/papers/index.html,v retrieving revision 1.166 diff -u -p -r1.166 index.html --- index.html 23 Mar 2013 17:56:07 - 1.166 +++ index.html 2 Apr 2013 13:01:44 - @@ -18,6 +18,13 @@ h3Presentations: AsiaBSDCon 2013/h3 blockquote font color=#009000strong +a href=https://poolp.org/~eric/asiabsdcon2013-smtpd/;OpenSMTPD: We deliver!/a, +Eric Faurot +/strong/fontbr +/blockquote + +blockquote +font color=#009000strong a href=asiabsdcon2013/phessler-bgp-spamd-presentation.pdfUsing BGP for Realtime import and export of OpenBSD SPAMD entries/a, Peter Hessler /strong/fontbr good presentation, and worth adding here, I think, but took me quite a while (and an incompatible browser -- IE!) to figure out how to move around in it... Assuming I'm not the last person to see a presentation like this, a note to use the arrow keys to negotiate the pages would probably be good. Nick. I wanted to convert it to pdf or simple static html+imgs before linking it to the site, but it didn't work too well... Eric.
smtpd alias expansion updated, please test
Hi, I have just commited a rather deep change to the alias expansion logic in smtpd. It fixes a bug reported by halex@ in virtual maps, where an alias expanding to two different virtual users would end up being sent only to the first one. Alias expansion is tricky. We want to make sure that the change doesn't introduce a regression. So please test and report problems you notice to gil...@openbsd.org and e...@openbsd.org. Thank you. Eric.
fix/add defines for ehci registers
They are only necessary for split isochronous transactions, which are not currently supported. So nothing uses those at the moment, but it doesn't hurt to have them right. Eric. Index: ehcireg.h === RCS file: /cvs/src/sys/dev/usb/ehcireg.h,v retrieving revision 1.16 diff -u -r1.16 ehcireg.h --- ehcireg.h 29 Nov 2008 08:52:03 - 1.16 +++ ehcireg.h 24 Oct 2011 20:18:42 - @@ -241,11 +241,13 @@ #define EHCI_SITD_SET_HUBA(x) ((x) 16) #define EHCI_SITD_GET_PORT(x) (((x) 23) 0x7f) /* hub port */ #define EHCI_SITD_SET_PORT(x) ((x) 23) +#define EHCI_SITD_GET_DIR(x) (((x) 31) 0x1) /* direction */ +#define EHCI_SITD_SET_DIR(x) ((x) 31) u_int32_t sitd_sched; -#define EHCI_QH_GET_SMASK(x) (((x) 0) 0xff) /* intr sched mask */ -#define EHCI_QH_SET_SMASK(x) ((x) 0) -#define EHCI_QH_GET_CMASK(x) (((x) 8) 0xff) /* split completion mask */ -#define EHCI_QH_SET_CMASK(x) ((x) 8) +#define EHCI_SITD_GET_SMASK(x) (((x) 0) 0xff) /* intr sched mask */ +#define EHCI_SITD_SET_SMASK(x) ((x) 0) +#define EHCI_SITD_GET_CMASK(x) (((x) 8) 0xff) /* split completion mask */ +#define EHCI_SITD_SET_CMASK(x) ((x) 8) u_int32_t sitd_trans; #define EHCI_SITD_GET_STATUS(x)(((x) 0) 0xff) /* status */ #define EHCI_SITD_ACTIVE 0x80 @@ -256,7 +258,7 @@ #define EHCI_SITD_MISSEDMICRO 0x04 #define EHCI_SITD_SPLITXSTATE 0x02 #define EHCI_SITD_GET_CPROG(x) (((x) 8) 0xff) /* c-mask progress */ -#define EHCI_SITD_SET_CPROG(x) (((x) 8) 0xff) +#define EHCI_SITD_SET_CPROG(x) ((x) 8) #define EHCI_SITD_GET_BYTES(x) (((x) 16) 0x7ff) /* bytes to transfer */ #define EHCI_SITD_SET_BYTES(x) ((x) 16) #define EHCI_SITD_GET_PG(x)(((x) 30) 0x1) /* buffer page */ @@ -266,7 +268,8 @@ /* page (buffer) 0 */ #define EHCI_SITD_GET_OFFSET(x)(((x) 0) 0xfff) /* current offset */ /* page (buffer) 1 */ -#define EHCI_SITD_GET_TCOUNT(x)(((x) 0) 0x3) /* transaction count */ +#define EHCI_SITD_GET_TCOUNT(x)(((x) 0) 0x7) /* transaction count */ +#define EHCI_SITD_SET_TCOUNT(x)((x) 0) #define EHCI_SITD_GET_TP(x)(((x) 3) 0x3) /* transaction position */ #define EHCI_SITD_SET_TP(x)((x) 3) #define EHCI_SITD_TP_ALL 0x0
small fix in ehci
actually read the register from the right place, and simplify code a bit. Index: ehci.c === RCS file: /cvs/src/sys/dev/usb/ehci.c,v retrieving revision 1.117 diff -u -r1.117 ehci.c --- ehci.c 3 Jul 2011 15:47:17 - 1.117 +++ ehci.c 4 Jul 2011 17:19:35 - @@ -2148,11 +2148,10 @@ } hubd = ehci_hubd; hubd.bNbrPorts = sc-sc_noport; - v = EOREAD4(sc, EHCI_HCSPARAMS); + v = EREAD4(sc, EHCI_HCSPARAMS); USETW(hubd.wHubCharacteristics, EHCI_HCS_PPC(v) ? UHD_PWR_INDIVIDUAL : UHD_PWR_NO_SWITCH | - EHCI_HCS_P_INDICATOR(EREAD4(sc, EHCI_HCSPARAMS)) - ? UHD_PORT_IND : 0); + EHCI_HCS_P_INDICATOR(v) ? UHD_PORT_IND : 0); hubd.bPwrOn2PwrGood = 200; /* XXX can't find out? */ for (i = 0, l = sc-sc_noport; l 0; i++, l -= 8, v = 8) hubd.DeviceRemovable[i++] = 0; /* XXX can't find out? */
Re: smtpd w/ async DNS
On Sat, Oct 30, 2010 at 07:26:00PM +0200, Peter J. Philipp wrote: I had a look at the pack.c file where the DNS compression is being handled. It looks good to me. But I have one concern that needs to be confirmed. In function dname_expand() on lines: 54 ptr = 256 * (n ~0xc0) + data[offset + 1]; 55 if (ptr = offset) 56 return (-1); The pointer is checked against offset meaning that a compression loop can't occur. This is good. However what happens if you have a DNS reply packet with a name with two labels in it, one being a normal label of a name and the second being a compression pointer that points back to the first label, kinda like so.. [8]centroid[C0 back to 8] I'm worried it might go into an infinite loop or crash even. Perhaps it should check that it cannot go back to a label inside a dns name that is being parsed. You are totally right. The following patch should fix it. Thanks a lot. Eric. --- pack.c.orig Sat Oct 30 21:22:14 2010 +++ pack.c Sat Oct 30 21:22:06 2010 @@ -38,21 +38,21 @@ ssize_t dname_expand(const unsigned char *data, size_t len, size_t offset, size_t *newoffset, char *dst, size_t max) { - size_t n, count, end, ptr; + size_t n, count, end, ptr, start; ssize_t res; if (offset = len) return (-1); res = 0; - end = offset; + end = start = offset; for(; (n = data[offset]); ) { if ((n 0xc0) == 0xc0) { if (offset + 2 len) return (-1); ptr = 256 * (n ~0xc0) + data[offset + 1]; - if (ptr = offset) + if (ptr = start) return (-1); if (end offset + 2) end = offset + 2; @@ -91,8 +91,9 @@ const char * dname_nthlabel(int n, const unsigned char *data, size_t len, size_t offset) { int i; - size_t c, ptr; + size_t c, ptr, start; + start = offset; for(i = 0;;) { c = data[offset]; if (c == 0) @@ -101,7 +102,7 @@ dname_nthlabel(int n, const unsigned char *data, size_ if (len offset + 2) return (NULL); ptr = 256 * (c ~0xc0) + data[offset + 1]; - if (ptr = offset) + if (ptr = start) return (NULL); offset = ptr; continue; @@ -117,14 +118,15 @@ dname_nthlabel(int n, const unsigned char *data, size_ ssize_t dname_count_labels(const unsigned char *data, size_t len, size_t offset) { - size_t c, n, ptr; + size_t c, n, ptr, start; + start = offset; for(n = 0; (c = data[offset]); ) { if ((c 0xc0) == 0xc0) { if (len offset + 2) return (-1); ptr = 256 * (c ~0xc0) + data[offset + 1]; - if (ptr = offset) + if (ptr = start) return (-1); offset = ptr; continue;
Re: smtpd w/ async DNS
On Thu, Oct 14, 2010 at 03:20:06PM -0600, Theo de Raadt wrote: On Thu, Oct 14, 2010 at 11:57 AM, Mike Belopuhov m...@crypt.org.ru wrote: this dns code has a serious flaw. you use arc4random to allocate request IDs. this is a bad decision, as you actually want a non-repeating property. Why? Each query transmission uses a newly allocated socket with a unique (random) source port address. The same txid might be used by multiple concurrent queries, but forgeries have to match both the txid and the source port so there's no risk of colliding attacks. Here's a random sequence that arc4random can legitimately put out: 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 There is is random. It could. Saying that DNS doesn't care is irresposible. It was never specifically designed to not care, and this is much more likely than with other protocols since it's id space is only 16 bits. Protecting this only with the 16 bit socket port number is irresponsible. Protecting it with an additional 16 bits -- essentially for free -- which do not collide is the responsible thing to do. Also, that code doesn't seem to check the source IP address of the response DNS packet. An easy fix (e.g., what djbdns does) is use connect(2) on the UDP socket so the kernel discards packets from bad source IPs/ports, and then just use send(2) and recv(2). Right. These issues have been fixed. Thanks for the reports. Note that this code is still very experimental and should be used with care. Lots of things are still missing, but I wanted to get something functional enough to test and settle the API. Eric.