Index | Thread | Search

From:
"Omar Polo" <op@omarpolo.com>
Subject:
smtpd: do not mutate shared dispatcher tls config
To:
tech@openbsd.org
Cc:
Renaud Allard <renaud@allard.it>
Date:
Mon, 16 Mar 2026 23:32:45 +0100

Download raw body.

Thread
  • Omar Polo:

    smtpd: do not mutate shared dispatcher tls config

Hello,

This diff is, modulo one minor stylistic change, from Renaud Allard for
the -portable repository:

	https://github.com/OpenSMTPD/OpenSMTPD/pull/1304

(Renaud: I've just tweaked the indentation, introduced a local variable
in mta_io, and added an error check in load_pki_tree to fail earlier.)

The underlying issue is that we currently have one shared tls config
structure and we might end up changing it when constructing a
tls_context for a relay that doesn't have an explicit TLS configuration.

The diff below tries to fix this by removing altogether the shared
dispatcher.  Instead, we construct every time the tls_config on-demand.
We're also loading in memory the default CA file once to appease pledge
;-) and reduce the cost of loading it every time.

if this mail will find you well, it means that it works ;-)

jokes aside, any objections on the diff?

diffstat refs/remotes/origin/master refs/heads/op/wip
 M  usr.sbin/smtpd/mta.c          |  11+  24-
 M  usr.sbin/smtpd/mta_session.c  |  13+   8-
 M  usr.sbin/smtpd/smtpd.c        |   6+   0-
 M  usr.sbin/smtpd/smtpd.h        |   3+   1-

4 files changed, 33 insertions(+), 33 deletions(-)

diff refs/remotes/origin/master refs/heads/op/wip
commit - 551f60a76f060be70022727b3c9dd77b6ba8ce11
commit + 0624487cbff0bec0da7d8bc18d7cda7e6dc9892f
blob - 8022d23ecb02e4654968912bb112c2042701e89e
blob + 2415c421089fc6475d41964b908d0aa0e67a8d6d
--- usr.sbin/smtpd/mta.c
+++ usr.sbin/smtpd/mta.c
@@ -43,7 +43,6 @@
 #define RELAY_ONHOLD		0x01
 #define RELAY_HOLDQ		0x02
 
-static void mta_setup_dispatcher(struct dispatcher *);
 static void mta_handle_envelope(struct envelope *, const char *);
 static void mta_query_smarthost(struct envelope *);
 static void mta_on_smarthost(struct envelope *, const char *);
@@ -456,21 +455,11 @@ mta_imsg(struct mproc *p, struct imsg *imsg)
 void
 mta_postfork(void)
 {
-	struct dispatcher *dispatcher;
-	const char *key;
-	void *iter;
-
-	iter = NULL;
-	while (dict_iter(env->sc_dispatchers, &iter, &key, (void **)&dispatcher)) {
-		log_debug("%s: %s", __func__, key);
-		mta_setup_dispatcher(dispatcher);
-	}
 }
 
-static void
-mta_setup_dispatcher(struct dispatcher *dispatcher)
+struct tls_config *
+mta_tls_config_create(struct dispatcher_remote *remote, int verify)
 {
-	struct dispatcher_remote *remote;
 	static const char *dheparams[] = { "none", "auto", "legacy" };
 	struct tls_config *config;
 	struct pki *pki;
@@ -478,11 +467,6 @@ mta_setup_dispatcher(struct dispatcher *dispatcher)
 	const char *ciphers;
 	uint32_t protos;
 
-	if (dispatcher->type != DISPATCHER_REMOTE)
-		return;
-
-	remote = &dispatcher->u.remote;
-
 	if ((config = tls_config_new()) == NULL)
 		fatal("smtpd: tls_config_new");
 
@@ -520,13 +504,16 @@ mta_setup_dispatcher(struct dispatcher *dispatcher)
 		    == -1)
 			fatalx("tls_config_set_ca_mem: %s",
 			    tls_config_error(config));
+	} else {
+		if (env->sc_default_ca_cert == NULL)
+			fatalx("default CA certificate not available");
+		if (tls_config_set_ca_mem(config, env->sc_default_ca_cert,
+		    env->sc_default_ca_cert_len) == -1)
+			fatalx("tls_config_set_ca_mem: %s",
+			    tls_config_error(config));
 	}
-	else if (tls_config_set_ca_file(config, tls_default_ca_cert_file())
-	    == -1)
-		fatalx("tls_config_set_ca_file: %s",
-		    tls_config_error(config));
 
-	if (remote->tls_verify) {
+	if (verify) {
 		tls_config_verify(config);
 	} else {
 		tls_config_insecure_noverifycert(config);
@@ -534,7 +521,7 @@ mta_setup_dispatcher(struct dispatcher *dispatcher)
 		tls_config_insecure_noverifytime(config);
 	}
 
-	remote->tls_config = config;
+	return (config);
 }
 
 void
blob - 8aa23779f8f46fb4ec69517a724563eae2a0515f
blob + 94c4128ab72888936ffcf61192ed29925ef4b26e
--- usr.sbin/smtpd/mta_session.c
+++ usr.sbin/smtpd/mta_session.c
@@ -1198,6 +1198,7 @@ static void
 mta_io(struct io *io, int evt, void *arg)
 {
 	struct mta_session	*s = arg;
+	struct dispatcher_remote *remote;
 	char			*line, *msg, *p;
 	size_t			 len;
 	const char		*error;
@@ -1227,7 +1228,10 @@ mta_io(struct io *io, int evt, void *arg)
 		log_info("%016"PRIx64" mta tls ciphers=%s",
 		    s->id, tls_to_text(io_tls(s->io)));
 		s->flags |= MTA_TLS;
-		if (s->relay->dispatcher->u.remote.tls_verify)
+
+		remote = &s->relay->dispatcher->u.remote;
+		if (remote->tls_verify ||
+		    ((s->flags & MTA_WANT_SECURE) && !remote->tls_required))
 			s->flags |= MTA_TLS_VERIFIED;
 
 		mta_tls_started(s);
@@ -1568,6 +1572,7 @@ static void
 mta_tls_init(struct mta_session *s)
 {
 	struct dispatcher_remote *remote;
+	struct tls_config *config;
 	struct tls *tls;
 
 	if ((tls = tls_client()) == NULL) {
@@ -1577,19 +1582,19 @@ mta_tls_init(struct mta_session *s)
 	}
 
 	remote = &s->relay->dispatcher->u.remote;
-	if ((s->flags & MTA_WANT_SECURE) && !remote->tls_required) {
-		/* If TLS not explicitly configured, use implicit config. */
-		remote->tls_required = 1;
-		remote->tls_verify = 1;
-		tls_config_verify(remote->tls_config);
-	}
-	if (tls_configure(tls, remote->tls_config) == -1) {
+	config = mta_tls_config_create(remote, remote->tls_verify ||
+	    ((s->flags & MTA_WANT_SECURE) && !remote->tls_required));
+
+	if (tls_configure(tls, config) == -1) {
 		log_info("%016"PRIx64" mta closing reason=tls-failure", s->id);
 		tls_free(tls);
+		tls_config_free(config);
 		s->flags |= MTA_FREE;
 		return;
 	}
 
+	tls_config_free(config);
+
 	if (io_connect_tls(s->io, tls, s->mxname) == -1) {
 		log_info("%016"PRIx64" mta closing reason=tls-connect-failed", s->id);
 		tls_free(tls);
blob - 086bfa42a5b7ad6b5b1f30e7589948f2364a9462
blob + 12a0782b07d6a6b01e66001f245cb2eba60c9ac9
--- usr.sbin/smtpd/smtpd.c
+++ usr.sbin/smtpd/smtpd.c
@@ -1118,6 +1118,12 @@ load_pki_tree(void)
 		if (!ssl_load_cafile(sca, sca->ca_cert_file))
 			fatalx("load_pki_tree: failed to load CA file");
 	}
+
+	env->sc_default_ca_cert = tls_load_file(tls_default_ca_cert_file(),
+	    &env->sc_default_ca_cert_len, NULL);
+	if (env->sc_default_ca_cert == NULL)
+		fatalx("load_pki_tree: failed to load default CA file: %s",
+		    tls_default_ca_cert_file());
 }
 
 void
blob - 3574db4c03d611801672016b1d0d4314746802cf
blob + 4b13cfc0e71cfc8cebb6af5f64ba380c82516a12
--- usr.sbin/smtpd/smtpd.h
+++ usr.sbin/smtpd/smtpd.h
@@ -611,6 +611,8 @@ struct smtpd {
 	struct dispatcher			*sc_dispatcher_bounce;
 
 	struct dict			       *sc_ca_dict;
+	uint8_t				       *sc_default_ca_cert;
+	size_t				        sc_default_ca_cert_len;
 	struct dict			       *sc_pki_dict;
 	struct dict			       *sc_ssl_dict;
 
@@ -1181,7 +1183,6 @@ struct dispatcher_remote {
 
 	char	*source;
 
-	struct tls_config *tls_config;
 	char	*ca;
 	char	*pki;
 
@@ -1492,6 +1493,7 @@ void m_clear_params(struct dict *);
 
 
 /* mta.c */
+struct tls_config *mta_tls_config_create(struct dispatcher_remote *, int);
 void mta_postfork(void);
 void mta_postprivdrop(void);
 void mta_imsg(struct mproc *, struct imsg *);