Re: smtpd: implement nullmx RFC 7505
[2023-10-18 11:42] Omar Polo
> On 2023/10/18 08:40:14 +0100, Stuart Henderson wrote:
> > On 2023/10/17 22:27, Philipp wrote:
> > > [2023-10-17 17:32] Omar Polo
> > > > [...]
>
> > > But I don't think your proposed patch is a good solution, because the
> > > result depend on the order of the RR in the repsonse. The problem is
> > > when the first entry in the response is a Null MX your patch truncate
> > > the other results and a bounce is generated. But when the Null MX is
> > > later in the response the other entries are used to deliver the mail.
>
> that's true; I fell for a shorter diff. I've attached below a version
> that doesn't depend on the order.
Your patch looks mostly good. I only have a small nitpick style comment:
> +
> + if (rr.rr.mx.preference == 0 && !strcmp(buf, "")) {
strcmp doesn't return a bool like value. Something like:
if (rr.rr.mx.preference == 0 && strcmp(buf, "") == 0)
is imho better to read.
Re: smtpd: implement nullmx RFC 7505
On 2023/10/18 08:40:14 +0100, Stuart Henderson wrote:
> On 2023/10/17 22:27, Philipp wrote:
> > [2023-10-17 17:32] Omar Polo
> > > There is one part of the RFC7505 that I'd like to quote and discuss
> > > with you however. The last paragraph of the section 3 says:
> > >
> > > : A domain that advertises a null MX MUST NOT advertise any other MX
> > > : RR.
> > >
> > > Your implementation handles the case where there are more than one MX
> > > by setting the `nullmx' field in the shared struct when we encounter
> > > one, and then don't process the other MXs when that field is set.
> > > This is fine.
> > >
> > > However, I think we can simplify here by not honouring the Null MX
> > > when there are other MX records. We're not violating the RFC. See
> > > diff below to see what I mean. The only difference is in dns.c, the
> > > rest is the same with yours.
> >
> > My reasaning for that was: When you set Null MX you explicitly opt out
> > from reciving mail even when you violate the spec. But if you want
> > it diffrent I can change it.
>
> I think in that situation, the recipient's DNS admin has *not* set a
> valid nullmx, and delivery to other MXes should still be attempted.
That's what I though too.
> > But I don't think your proposed patch is a good solution, because the
> > result depend on the order of the RR in the repsonse. The problem is
> > when the first entry in the response is a Null MX your patch truncate
> > the other results and a bounce is generated. But when the Null MX is
> > later in the response the other entries are used to deliver the mail.
that's true; I fell for a shorter diff. I've attached below a version
that doesn't depend on the order.
> > > So far I've only compile-tested the code. I don't have a spare domain
> > > to test and don't know of anyone using a Null MX.
> >
> > To test Null MX you can use example.com. To test the localhost patch a
> > friend of me has set up mxtest.yannikenss.de.
thanks!
> [...]
> > [1] I still belive OpenSMTPD should only connect to public routable
> > addresses for relay. So don't connect to something like 127.0.0.1/8,
> > ::1/128, RFC1918 addresses, ULA, ...
> > But this is independent of this patch.
>
> Support for nullmx makes sense to me. Perhaps also the localhost
> handling. But some intranet setups may require connections to
> RFC1918/ULA addresses - I don't think an MTA should prevent these.
Completely agree.
diff 2d025d839f99dc09ee525c11a4ed09a0f3bbe7d0
02bb94351d3865e61483023cab9fa02bcac2970d
commit - 2d025d839f99dc09ee525c11a4ed09a0f3bbe7d0
commit + 02bb94351d3865e61483023cab9fa02bcac2970d
blob - 4cf5d23d1d14b5400c6f4429dae0a4f6490073d4
blob + 552a5cf9115401b4fac3d131d6e5c022ebb8b7fd
--- usr.sbin/smtpd/dns.c
+++ usr.sbin/smtpd/dns.c
@@ -232,10 +232,10 @@ dns_dispatch_mx(struct asr_result *ar, void *arg)
struct dns_rrrr;
char buf[512];
size_t found;
+ int nullmx = 0;
if (ar->ar_h_errno && ar->ar_h_errno != NO_DATA &&
ar->ar_h_errno != NOTIMP) {
-
m_create(s->p, IMSG_MTA_DNS_HOST_END, 0, 0, -1);
m_add_id(s->p, s->reqid);
if (ar->ar_rcode == NXDOMAIN)
@@ -259,13 +259,29 @@ dns_dispatch_mx(struct asr_result *ar, void *arg)
unpack_rr(, );
if (rr.rr_type != T_MX)
continue;
+
print_dname(rr.rr.mx.exchange, buf, sizeof(buf));
buf[strlen(buf) - 1] = '\0';
+
+ if (rr.rr.mx.preference == 0 && !strcmp(buf, "")) {
+ nullmx = 1;
+ continue;
+ }
+
dns_lookup_host(s, buf, rr.rr.mx.preference);
found++;
}
free(ar->ar_data);
+ if (nullmx && found == 0) {
+ m_create(s->p, IMSG_MTA_DNS_HOST_END, 0, 0, -1);
+ m_add_id(s->p, s->reqid);
+ m_add_int(s->p, DNS_NULLMX);
+ m_close(s->p);
+ free(s);
+ return;
+ }
+
/* fallback to host if no MX is found. */
if (found == 0)
dns_lookup_host(s, s->name, 0);
blob - c0d0fc02931b90409441035d2744923af9e42df1
blob + 25e158b68a88c8485428a2476c1c557f8c60404d
--- usr.sbin/smtpd/mta.c
+++ usr.sbin/smtpd/mta.c
@@ -1088,6 +1088,10 @@ mta_on_mx(void *tag, void *arg, void *data)
else
relay->failstr = "No MX found for domain";
break;
+ case DNS_NULLMX:
+
Re: smtpd: implement nullmx RFC 7505
On 2023/10/17 22:27, Philipp wrote: > [2023-10-17 17:32] Omar Polo > > > > There is one part of the RFC7505 that I'd like to quote and discuss > > with you however. The last paragraph of the section 3 says: > > > > : A domain that advertises a null MX MUST NOT advertise any other MX > > : RR. > > > > Your implementation handles the case where there are more than one MX > > by setting the `nullmx' field in the shared struct when we encounter > > one, and then don't process the other MXs when that field is set. > > This is fine. > > > > However, I think we can simplify here by not honouring the Null MX > > when there are other MX records. We're not violating the RFC. See > > diff below to see what I mean. The only difference is in dns.c, the > > rest is the same with yours. > > My reasaning for that was: When you set Null MX you explicitly opt out > from reciving mail even when you violate the spec. But if you want > it diffrent I can change it. I think in that situation, the recipient's DNS admin has *not* set a valid nullmx, and delivery to other MXes should still be attempted. > But I don't think your proposed patch is a good solution, because the > result depend on the order of the RR in the repsonse. The problem is > when the first entry in the response is a Null MX your patch truncate > the other results and a bounce is generated. But when the Null MX is > later in the response the other entries are used to deliver the mail. > > > So far I've only compile-tested the code. I don't have a spare domain > > to test and don't know of anyone using a Null MX. > > To test Null MX you can use example.com. To test the localhost patch a > friend of me has set up mxtest.yannikenss.de. > > > > Because some domains set the MX record to "localhost." to get a similar > > > efect the secound patch ignores "localhost." MX entries and handles a MX > > > containing only "localhost." records like a Null MX. > > > > This could be simplified too. On top of diff below, it would need > > just something among the lines of: > > When localhost and Null MX should be handled the same this could be > simplified in one check, yes. But that "localhost." and Null MX are > handled different was on purpose. Because I would say setting a Null MX > is a explicit opt out from reciving mail and setting MX to localhost > is implicit. As said before, I have no problem which changing this. > > > I'm still not convinced about this part however. It feels wrong to me > > to hardcode such a check, it seems too much paranoic. The follow-up > > would be to check for localhost in dns_dispatch_host too? ;) > > The idea[1] about this part was because setting "localhost." as MX is used > in a similiar way as Null MX[0]. So handle this in a simmilar way as > Null MX make sense. This way the sender will get a better bounce message. > > Philipp > > [0] https://www.netmeister.org/blog/mx-diversity.html > > [1] I still belive OpenSMTPD should only connect to public routable > addresses for relay. So don't connect to something like 127.0.0.1/8, > ::1/128, RFC1918 addresses, ULA, ... > But this is independent of this patch. Support for nullmx makes sense to me. Perhaps also the localhost handling. But some intranet setups may require connections to RFC1918/ULA addresses - I don't think an MTA should prevent these. BTW, you can already block those at the DNS resolver level in a configurable way - see unbound's "private-address" feature.
Re: smtpd: implement nullmx RFC 7505
[2023-10-17 17:32] Omar Polo > sorry for the terrifc delay. > > On 2023/10/01 14:59:15 +0200, Philipp wrote: > > Hi > > > > Setting Null MX is a way for domainowners to indicate that the domain > > does not accept mail. Currently a Null MX causes a tempfail and the > > mail will be queued and tried to resubmitted till a timeout. With the > > attached patch a Null MX causes a permfail. This way the sender will > > directly get a bounce with the message "Domain does not accept mail". > > I'm not sure how much widespread the usage is, but it doesn't seem a > bad feature. It's simple to implement and it provides some (very > small) value. There is a blogpost[0] which says about 1% of the domains use Null MX and about 0.7% set the MX to "localhost." (probably for the same propose). > In general I'm OK with the idea, but would need some OKs from other > developers too. > > There is one part of the RFC7505 that I'd like to quote and discuss > with you however. The last paragraph of the section 3 says: > > : A domain that advertises a null MX MUST NOT advertise any other MX > : RR. > > Your implementation handles the case where there are more than one MX > by setting the `nullmx' field in the shared struct when we encounter > one, and then don't process the other MXs when that field is set. > This is fine. > > However, I think we can simplify here by not honouring the Null MX > when there are other MX records. We're not violating the RFC. See > diff below to see what I mean. The only difference is in dns.c, the > rest is the same with yours. My reasaning for that was: When you set Null MX you explicitly opt out from reciving mail even when you violate the spec. But if you want it diffrent I can change it. But I don't think your proposed patch is a good solution, because the result depend on the order of the RR in the repsonse. The problem is when the first entry in the response is a Null MX your patch truncate the other results and a bounce is generated. But when the Null MX is later in the response the other entries are used to deliver the mail. > So far I've only compile-tested the code. I don't have a spare domain > to test and don't know of anyone using a Null MX. To test Null MX you can use example.com. To test the localhost patch a friend of me has set up mxtest.yannikenss.de. > > Because some domains set the MX record to "localhost." to get a similar > > efect the secound patch ignores "localhost." MX entries and handles a MX > > containing only "localhost." records like a Null MX. > > This could be simplified too. On top of diff below, it would need > just something among the lines of: When localhost and Null MX should be handled the same this could be simplified in one check, yes. But that "localhost." and Null MX are handled different was on purpose. Because I would say setting a Null MX is a explicit opt out from reciving mail and setting MX to localhost is implicit. As said before, I have no problem which changing this. > I'm still not convinced about this part however. It feels wrong to me > to hardcode such a check, it seems too much paranoic. The follow-up > would be to check for localhost in dns_dispatch_host too? ;) The idea[1] about this part was because setting "localhost." as MX is used in a similiar way as Null MX[0]. So handle this in a simmilar way as Null MX make sense. This way the sender will get a better bounce message. Philipp [0] https://www.netmeister.org/blog/mx-diversity.html [1] I still belive OpenSMTPD should only connect to public routable addresses for relay. So don't connect to something like 127.0.0.1/8, ::1/128, RFC1918 addresses, ULA, ... But this is independent of this patch.
Re: smtpd: implement nullmx RFC 7505
Hello,
sorry for the terrifc delay.
On 2023/10/01 14:59:15 +0200, Philipp wrote:
> Hi
>
> Setting Null MX is a way for domainowners to indicate that the domain
> does not accept mail. Currently a Null MX causes a tempfail and the
> mail will be queued and tried to resubmitted till a timeout. With the
> attached patch a Null MX causes a permfail. This way the sender will
> directly get a bounce with the message "Domain does not accept mail".
I'm not sure how much widespread the usage is, but it doesn't seem a
bad feature. It's simple to implement and it provides some (very
small) value.
In general I'm OK with the idea, but would need some OKs from other
developers too.
There is one part of the RFC7505 that I'd like to quote and discuss
with you however. The last paragraph of the section 3 says:
: A domain that advertises a null MX MUST NOT advertise any other MX
: RR.
Your implementation handles the case where there are more than one MX
by setting the `nullmx' field in the shared struct when we encounter
one, and then don't process the other MXs when that field is set.
This is fine.
However, I think we can simplify here by not honouring the Null MX
when there are other MX records. We're not violating the RFC. See
diff below to see what I mean. The only difference is in dns.c, the
rest is the same with yours.
So far I've only compile-tested the code. I don't have a spare domain
to test and don't know of anyone using a Null MX.
> Because some domains set the MX record to "localhost." to get a similar
> efect the secound patch ignores "localhost." MX entries and handles a MX
> containing only "localhost." records like a Null MX.
This could be simplified too. On top of diff below, it would need
just something among the lines of:
- if (rr.rr.mx.preference == 0 && !strcmp(buf, "")) {
+ if ((rr.rr.mx.preference == 0 && !strcmp(buf, "")) ||
+ !strcasecmp(buf, "localhost")) {
if (found)
continue;
m_create(s->p, IMSG_MTA_DNS_HOST_END, 0, 0, -1);
I'm still not convinced about this part however. It feels wrong to me
to hardcode such a check, it seems too much paranoic. The follow-up
would be to check for localhost in dns_dispatch_host too? ;)
Thanks,
Omar Polo
diff 2d025d839f99dc09ee525c11a4ed09a0f3bbe7d0
8d6138e5b1e0bc112ff2584d8528e6bc95a39b6f
commit - 2d025d839f99dc09ee525c11a4ed09a0f3bbe7d0
commit + 8d6138e5b1e0bc112ff2584d8528e6bc95a39b6f
blob - 4cf5d23d1d14b5400c6f4429dae0a4f6490073d4
blob + effc07d142895fb2b622242efcd5c69da7f3b67c
--- usr.sbin/smtpd/dns.c
+++ usr.sbin/smtpd/dns.c
@@ -259,8 +259,22 @@ dns_dispatch_mx(struct asr_result *ar, void *arg)
unpack_rr(, );
if (rr.rr_type != T_MX)
continue;
+
print_dname(rr.rr.mx.exchange, buf, sizeof(buf));
buf[strlen(buf) - 1] = '\0';
+
+ if (rr.rr.mx.preference == 0 && !strcmp(buf, "")) {
+ if (found)
+ continue;
+ m_create(s->p, IMSG_MTA_DNS_HOST_END, 0, 0, -1);
+ m_add_id(s->p, s->reqid);
+ m_add_int(s->p, DNS_NULLMX);
+ m_close(s->p);
+ free(s);
+ found++;
+ break;
+ }
+
dns_lookup_host(s, buf, rr.rr.mx.preference);
found++;
}
blob - c0d0fc02931b90409441035d2744923af9e42df1
blob + 25e158b68a88c8485428a2476c1c557f8c60404d
--- usr.sbin/smtpd/mta.c
+++ usr.sbin/smtpd/mta.c
@@ -1088,6 +1088,10 @@ mta_on_mx(void *tag, void *arg, void *data)
else
relay->failstr = "No MX found for domain";
break;
+ case DNS_NULLMX:
+ relay->fail = IMSG_MTA_DELIVERY_PERMFAIL;
+ relay->failstr = "Domain does not accept mail";
+ break;
default:
fatalx("bad DNS lookup error code");
break;
blob - 6781286928da45e6159bce81ff2437011ebdef72
blob + 9f4732d1c27f6ef9f62951f0207f209a83038dcf
--- usr.sbin/smtpd/smtpd.h
+++ usr.sbin/smtpd/smtpd.h
@@ -1026,6 +1026,8 @@ enum dns_error {
DNS_EINVAL,
DNS_ENONAME,
DNS_ENOTFOUND,
+ /* rfc 7505 */
+ DNS_NULLMX,
};
enum lka_resp_status {
smtpd: implement nullmx RFC 7505
Hi
Setting Null MX is a way for domainowners to indicate that the domain
does not accept mail. Currently a Null MX causes a tempfail and the
mail will be queued and tried to resubmitted till a timeout. With the
attached patch a Null MX causes a permfail. This way the sender will
directly get a bounce with the message "Domain does not accept mail".
Because some domains set the MX record to "localhost." to get a similar
efect the secound patch ignores "localhost." MX entries and handles a MX
containing only "localhost." records like a Null MX.
Philipp
From 2970019967e967d98ec30f86549f38788bff6081 Mon Sep 17 00:00:00 2001
From: Philipp
Date: Sun, 2 Jul 2023 01:27:35 +0200
Subject: [PATCH 1/2] implement rfc 7505 (Null MX)
Null MX is to indicate that a domain does not accept mail.
---
usr.sbin/smtpd/dns.c | 28 +++-
usr.sbin/smtpd/mta.c | 4
usr.sbin/smtpd/smtpd.h | 2 ++
3 files changed, 29 insertions(+), 5 deletions(-)
diff --git a/usr.sbin/smtpd/dns.c b/usr.sbin/smtpd/dns.c
index 4cf5d23d1d1..d510fa2b5aa 100644
--- a/usr.sbin/smtpd/dns.c
+++ b/usr.sbin/smtpd/dns.c
@@ -44,6 +44,7 @@ struct dns_session {
size_t mxfound;
int error;
int refcount;
+ int nullmx;
};
static void dns_lookup_host(struct dns_session *, const char *, int);
@@ -195,7 +196,7 @@ dns_dispatch_host(struct asr_result *ar, void *arg)
s = lookup->session;
- for (ai = ar->ar_addrinfo; ai; ai = ai->ai_next) {
+ for (ai = ar->ar_addrinfo; s->nullmx == 0 && ai; ai = ai->ai_next) {
s->mxfound++;
m_create(s->p, IMSG_MTA_DNS_HOST, 0, 0, -1);
m_add_id(s->p, s->reqid);
@@ -215,10 +216,12 @@ dns_dispatch_host(struct asr_result *ar, void *arg)
if (--s->refcount)
return;
- m_create(s->p, IMSG_MTA_DNS_HOST_END, 0, 0, -1);
- m_add_id(s->p, s->reqid);
- m_add_int(s->p, s->mxfound ? DNS_OK : DNS_ENOTFOUND);
- m_close(s->p);
+ if (s->nullmx == 0) {
+ m_create(s->p, IMSG_MTA_DNS_HOST_END, 0, 0, -1);
+ m_add_id(s->p, s->reqid);
+ m_add_int(s->p, s->mxfound ? DNS_OK : DNS_ENOTFOUND);
+ m_close(s->p);
+ }
free(s);
}
@@ -259,6 +262,21 @@ dns_dispatch_mx(struct asr_result *ar, void *arg)
unpack_rr(, );
if (rr.rr_type != T_MX)
continue;
+
+ /* Null MX */
+ if (rr.rr.mx.preference == 0 && rr.rr.mx.exchange[0] == 0) {
+ m_create(s->p, IMSG_MTA_DNS_HOST_END, 0, 0, -1);
+ m_add_id(s->p, s->reqid);
+ m_add_int(s->p, DNS_NULLMX);
+ m_close(s->p);
+ if (found == 0)
+free(s);
+ else
+s->nullmx = 1;
+ found++;
+ break;
+ }
+
print_dname(rr.rr.mx.exchange, buf, sizeof(buf));
buf[strlen(buf) - 1] = '\0';
dns_lookup_host(s, buf, rr.rr.mx.preference);
diff --git a/usr.sbin/smtpd/mta.c b/usr.sbin/smtpd/mta.c
index c0d0fc02931..25e158b68a8 100644
--- a/usr.sbin/smtpd/mta.c
+++ b/usr.sbin/smtpd/mta.c
@@ -1088,6 +1088,10 @@ mta_on_mx(void *tag, void *arg, void *data)
else
relay->failstr = "No MX found for domain";
break;
+ case DNS_NULLMX:
+ relay->fail = IMSG_MTA_DELIVERY_PERMFAIL;
+ relay->failstr = "Domain does not accept mail";
+ break;
default:
fatalx("bad DNS lookup error code");
break;
diff --git a/usr.sbin/smtpd/smtpd.h b/usr.sbin/smtpd/smtpd.h
index 6781286928d..9f4732d1c27 100644
--- a/usr.sbin/smtpd/smtpd.h
+++ b/usr.sbin/smtpd/smtpd.h
@@ -1026,6 +1026,8 @@ enum dns_error {
DNS_EINVAL,
DNS_ENONAME,
DNS_ENOTFOUND,
+ /* rfc 7505 */
+ DNS_NULLMX,
};
enum lka_resp_status {
--
2.39.2
From ace283bbedc1e7594c850e0ae6f3b6d9d456ba77 Mon Sep 17 00:00:00 2001
From: Philipp
Date: Sun, 2 Jul 2023 01:50:20 +0200
Subject: [PATCH 2/2] filter localhost MX
A localhost MX only cause a loop. Also handle only a localhost
like a Null MX.
---
usr.sbin/smtpd/dns.c | 18 ++++++
1 file changed, 18 insertions(+)
diff --git a/usr.sbin/smtpd/dns.c b/usr.sbin/smtpd/dns.c
index d510fa2b5aa..0df221f3755 100644
--- a/usr.sbin/smtpd/dns.c
+++ b/usr.sbin/smtpd/dns.c
@@ -235,6 +235,7 @@ dns_dispatch_mx(struct asr_result *ar, void *arg)
struct dns_rr rr;
char buf[512];
size_t found;
+ int localhost;
if (ar->ar_h_errno && ar->ar_h_errno != NO_DATA &&
ar->ar_h_errno != NOTIMP) {
@@ -258,6 +259,7 @@ dns_dispatch_mx(struct asr_result *ar, void *arg)
unpack_query(, );
found = 0;
+ localhost = 0;
for (; h.ancount; h.ancount--) {
unpack_rr(, );
if (rr.rr_type != T_MX)
@@ -279,11 +281,27 @@ dns_dispatch_mx(struct asr_result *ar, void *arg)
print_dname(rr.rr.mx.exchange, buf, sizeof(buf));
buf[strlen(buf) - 1] = '\0';
+ if (strcasecmp("localhost", buf) == 0) {
+ log_warnx("ignore localhost MX-entry for domain <%s>",
+ s->name);
+ localhost++;
+ continue;
+ }
dns_lookup_host(s, buf, rr.rr.mx.preference);
found++;
}
f
Re: [diff] selectable curves in smtpd ?
On 2023/08/12 19:07, Marc Espie wrote: > On Sat, Aug 12, 2023 at 03:21:00PM +, gil...@poolp.org wrote: > > August 12, 2023 4:34 PM, "Theo Buehler" wrote: > > > > > On Sat, Aug 12, 2023 at 02:29:45PM +, gil...@poolp.org wrote: > > > > > >> Hello, > > >> > > >> Someone asked about selectable curves in the OpenSMTPD portable tracker, > > >> and it turns out I had a diff for that among a few others. > > > > > > Why do they need this? > > > > I suspect for the same reason people have needed ciphers selection in the > > past, > > being able to comply with the requirements of some certification (iirc, > > medical > > mail systems, for example, have strict requirements regarding their setup). > > > > Anyways, I've written this a long time ago and I'm providing it in case > > it's of > > any interest, feel free to discard. > > > > This is moving *backwards* from best practices. As if certification cares about that ;)
Re: [diff] selectable curves in smtpd ?
On Sat, Aug 12, 2023 at 03:21:00PM +, gil...@poolp.org wrote: > August 12, 2023 4:34 PM, "Theo Buehler" wrote: > > > On Sat, Aug 12, 2023 at 02:29:45PM +, gil...@poolp.org wrote: > > > >> Hello, > >> > >> Someone asked about selectable curves in the OpenSMTPD portable tracker, > >> and it turns out I had a diff for that among a few others. > > > > Why do they need this? > > I suspect for the same reason people have needed ciphers selection in the > past, > being able to comply with the requirements of some certification (iirc, > medical > mail systems, for example, have strict requirements regarding their setup). > > Anyways, I've written this a long time ago and I'm providing it in case it's > of > any interest, feel free to discard. > This is moving *backwards* from best practices. Notice that TLS 1.3 did remove EC parameters choice, because this could lead to downgrade MIT attacks.
Re: [diff] selectable curves in smtpd ?
August 12, 2023 4:34 PM, "Theo Buehler" wrote: > On Sat, Aug 12, 2023 at 02:29:45PM +, gil...@poolp.org wrote: > >> Hello, >> >> Someone asked about selectable curves in the OpenSMTPD portable tracker, >> and it turns out I had a diff for that among a few others. > > Why do they need this? I suspect for the same reason people have needed ciphers selection in the past, being able to comply with the requirements of some certification (iirc, medical mail systems, for example, have strict requirements regarding their setup). Anyways, I've written this a long time ago and I'm providing it in case it's of any interest, feel free to discard.
Re: [diff] selectable curves in smtpd ?
On Sat, Aug 12, 2023 at 02:29:45PM +, gil...@poolp.org wrote: > Hello, > > Someone asked about selectable curves in the OpenSMTPD portable tracker, > and it turns out I had a diff for that among a few others. Why do they need this?
[diff] selectable curves in smtpd ?
Hello,
Someone asked about selectable curves in the OpenSMTPD portable tracker,
and it turns out I had a diff for that among a few others.
The diff below adds support for the curves keyword in listener and relay
directives,
allowing to specify a curve string suitable for tls_config_set_ecdhecurves(3)
in the
same way ciphers were made selectable.
I also have a couple other diffs which I'll clean and send.
Index: mta.c
===
RCS file: /cvs/src/usr.sbin/smtpd/mta.c,v
retrieving revision 1.245
diff -u -p -u -p -r1.245 mta.c
--- mta.c 31 May 2023 16:51:46 - 1.245
+++ mta.c 12 Aug 2023 14:20:21 -
@@ -476,6 +476,7 @@ mta_setup_dispatcher(struct dispatcher *
struct pki *pki;
struct ca *ca;
const char *ciphers;
+ const char *curves;
uint32_t protos;
if (dispatcher->type != DISPATCHER_REMOTE)
@@ -490,6 +491,12 @@ mta_setup_dispatcher(struct dispatcher *
if (remote->tls_ciphers)
ciphers = remote->tls_ciphers;
if (ciphers && tls_config_set_ciphers(config, ciphers) == -1)
+ fatalx("%s", tls_config_error(config));
+
+ curves = env->sc_tls_curves;
+ if (remote->tls_curves)
+ curves = remote->tls_curves;
+ if (curves && tls_config_set_ecdhecurves(config, curves) == -1)
fatalx("%s", tls_config_error(config));
if (remote->tls_protocols) {
Index: parse.y
=======
RCS file: /cvs/src/usr.sbin/smtpd/parse.y,v
retrieving revision 1.292
diff -u -p -u -p -r1.292 parse.y
--- parse.y 10 May 2023 07:19:49 - 1.292
+++ parse.y 12 Aug 2023 14:20:21 -
@@ -125,6 +125,7 @@ static struct listen_opts {
char *pki[PKI_MAX];
int pkicount;
char *tls_ciphers;
+ char *tls_curves;
char *tls_protocols;
char *ca;
uint16_tauth;
@@ -166,7 +167,7 @@ typedef struct {
%token ACTION ADMD ALIAS ANY ARROW AUTH AUTH_OPTIONAL
%token BACKUP BOUNCE BYPASS
-%token CA CERT CHAIN CHROOT CIPHERS COMMIT COMPRESSION CONNECT
+%token CA CERT CHAIN CHROOT CIPHERS COMMIT COMPRESSION CONNECT CURVES
%token DATA DATA_LINE DHE DISCONNECT DOMAIN
%token EHLO ENABLE ENCRYPTION ERROR EXPAND_ONLY
%token FCRDNS FILTER FOR FORWARD_ONLY FROM
@@ -527,6 +528,9 @@ SMTP LIMIT limits_smtp
| SMTP CIPHERS STRING {
conf->sc_tls_ciphers = $3;
}
+| SMTP CURVES STRING {
+ conf->sc_tls_curves = $3;
+}
| SMTP MAX_MESSAGE_SIZE size {
conf->sc_maxsize = $3;
}
@@ -765,6 +769,14 @@ HELO STRING {
dsp->u.remote.tls_ciphers = $2;
}
+| CURVES STRING {
+ if (dsp->u.remote.tls_curves) {
+ yyerror("curves already specified for this dispatcher");
+ YYERROR;
+ }
+
+ dsp->u.remote.tls_curves = $2;
+}
| PROTOCOLS STRING {
if (dsp->u.remote.tls_protocols) {
yyerror("protocols already specified for this dispatcher");
@@ -2329,6 +2341,13 @@ opt_if_listen : INET4 {
}
listen_opts.tls_ciphers = $2;
}
+ | CURVES STRING {
+ if (listen_opts.tls_curves) {
+ yyerror("curves already specified");
+ YYERROR;
+ }
+ listen_opts.tls_curves = $2;
+ }
| PROTOCOLS STRING {
if (listen_opts.tls_protocols) {
yyerror("protocols already specified");
@@ -2657,6 +2676,7 @@ lookup(char *s)
{ "commit", COMMIT },
{ "compression",COMPRESSION },
{ "connect",CONNECT },
+ { "curves", CURVES },
{ "data", DATA },
{ "data-line", DATA_LINE },
{ "dhe",DHE },
@@ -3251,6 +3271,11 @@ create_if_listener(struct listen_opts *l
if (lo->pkicount == 0 && lo->ssl)
fatalx("invalid listen option: pki required for tls/smtps");
+ if (lo->tls_ciphers && !lo->ssl)
+ fatalx("invalid listen option: ciphers requires tls/smtps");
+ if (lo->tls_curves && !lo->ssl)
+ fatalx("invalid listen option: curves requires tls/smtps");
+
flags = lo->flags;
if (lo->port) {
@@ -3324,6 +3349,11 @@ config_listener(struct listener *h, str
fatal("strdup");
}
+ if
Re: smtpd, relayd, iked: drop ssl_init
On Sat, Jun 24, 2023 at 08:40:01PM +0200, Theo Buehler wrote: > On Sat, Jun 24, 2023 at 08:15:40PM +0200, Omar Polo wrote: > > while talking about a related matter with tb and jsing, jsing noted > > that ssl_init() in smtpd is completely useless. All its loading is > > already done automatically by libcrypto at runtime, and judging by the > > implementation of the called functions there's no need to actually > > force the initialization. > > > > There is similar code in relayd and iked, so apply the same treatment. > > > > I've tested smtpd and it works just as fine as before, don't use > > relayd but the regression suite is happy. I don't use iked, so some > > testing with it is welcomed. Not that I expect any sort of breakage, > > this is almost a no-op. > > It is a noop. Before doing anything, both LibreSSL and OpenSSL libcrypto > will call OPENSSL_init_crypto(0, NULL) and libssl will do similar. This > will call all these initialization functions (it is a bit convoluted, > but it is the case). > > ok tb > > (I only meant iked diff should go to tech because I don't want to ok > things behind tobhe's back). > Thanks! ok tobhe@
Re: smtpd, relayd, iked: drop ssl_init
On Sat, Jun 24, 2023 at 08:15:40PM +0200, Omar Polo wrote: > while talking about a related matter with tb and jsing, jsing noted > that ssl_init() in smtpd is completely useless. All its loading is > already done automatically by libcrypto at runtime, and judging by the > implementation of the called functions there's no need to actually > force the initialization. > > There is similar code in relayd and iked, so apply the same treatment. > > I've tested smtpd and it works just as fine as before, don't use > relayd but the regression suite is happy. I don't use iked, so some > testing with it is welcomed. Not that I expect any sort of breakage, > this is almost a no-op. It is a noop. Before doing anything, both LibreSSL and OpenSSL libcrypto will call OPENSSL_init_crypto(0, NULL) and libssl will do similar. This will call all these initialization functions (it is a bit convoluted, but it is the case). ok tb (I only meant iked diff should go to tech because I don't want to ok things behind tobhe's back).
smtpd, relayd, iked: drop ssl_init
while talking about a related matter with tb and jsing, jsing noted
that ssl_init() in smtpd is completely useless. All its loading is
already done automatically by libcrypto at runtime, and judging by the
implementation of the called functions there's no need to actually
force the initialization.
There is similar code in relayd and iked, so apply the same treatment.
I've tested smtpd and it works just as fine as before, don't use
relayd but the regression suite is happy. I don't use iked, so some
testing with it is welcomed. Not that I expect any sort of breakage,
this is almost a no-op.
ok?
diff 143f55f5d199bde9c93e92667cd4bfda0a272dd2
d9f7ac73d694ec29760e87d4b21a06e9aa8ef711
commit - 143f55f5d199bde9c93e92667cd4bfda0a272dd2
commit + d9f7ac73d694ec29760e87d4b21a06e9aa8ef711
blob - 7f7c8bee0d371e0ac4537330662bdcc7f20f0cd1
blob + c0ff87dafd5b27ba25fbaac73921cab7488f20ac
--- sbin/iked/ca.c
+++ sbin/iked/ca.c
@@ -33,7 +33,6 @@
#include
#include
-#include
#include
#include
#include
@@ -1959,17 +1958,6 @@ ca_sslinit(void)
}
void
-ca_sslinit(void)
-{
- OpenSSL_add_all_algorithms();
- ERR_load_crypto_strings();
-
- /* Init hardware crypto engines. */
- ENGINE_load_builtin_engines();
- ENGINE_register_all_complete();
-}
-
-void
ca_sslerror(const char *caller)
{
unsigned longerror;
blob - aa824d6f1966034acb591fca3d0710e00796b49c
blob + 360127f73edc1b88f85796e892e8be84829c3477
--- sbin/iked/iked.c
+++ sbin/iked/iked.c
@@ -175,7 +175,6 @@ main(int argc, char *argv[])
if (strlcpy(env->sc_conffile, conffile, PATH_MAX) >= PATH_MAX)
errx(1, "config file exceeds PATH_MAX");
- ca_sslinit();
group_init();
policy_init(env);
blob - 85958e1c2370b0780095e343a58438187a88c3dd
blob + bf83d4799ee8a498cdf9f10ba9d4f57cdfade249
--- sbin/iked/iked.h
+++ sbin/iked/iked.h
@@ -1178,7 +1178,6 @@ void ca_sslinit(void);
voidca_getkey(struct privsep *, struct iked_id *, enum imsg_type);
int ca_privkey_serialize(EVP_PKEY *, struct iked_id *);
int ca_pubkey_serialize(EVP_PKEY *, struct iked_id *);
-voidca_sslinit(void);
voidca_sslerror(const char *);
char *ca_asn1_name(uint8_t *, size_t);
void *ca_x509_name_parse(char *);
blob - a2f1c130d6b45e3082048218c11537dca485998a
blob + a1272319a945a8dc1c859151a9c06c29d10484ab
--- usr.sbin/relayd/config.c
+++ usr.sbin/relayd/config.c
@@ -293,7 +293,6 @@ config_getcfg(struct relayd *env, struct imsg *imsg)
}
if (env->sc_conf.flags & (F_TLS|F_TLSCLIENT)) {
- ssl_init(env);
if (what & CONFIG_CA_ENGINE)
ca_engine_init(env);
}
blob - edc86218960df02cb917606bdf402c776e07206d
blob + 7dd2f856e20f07ea0f9ec6e599da54c3f35ef54e
--- usr.sbin/relayd/relayd.c
+++ usr.sbin/relayd/relayd.c
@@ -255,9 +255,6 @@ main(int argc, char *argv[])
exit(0);
}
- if (env->sc_conf.flags & (F_TLS|F_TLSCLIENT))
- ssl_init(env);
-
/* rekey the TLS tickets before pushing the config */
parent_tls_ticket_rekey(0, 0, env);
if (parent_configure(env) == -1)
blob - 990cec3505fc6bc22b837cc4efb0d58af3614984
blob + 5c4618b9fc4de4867c35f2e09ad0aa8de7ed0290
--- usr.sbin/relayd/relayd.h
+++ usr.sbin/relayd/relayd.h
@@ -1293,7 +1293,6 @@ void ssl_init(struct relayd *);
int script_exec(struct relayd *, struct ctl_script *);
/* ssl.c */
-voidssl_init(struct relayd *);
char *ssl_load_key(struct relayd *, const char *, off_t *, char *);
uint8_t *ssl_update_certificate(const uint8_t *, size_t, EVP_PKEY *,
EVP_PKEY *, X509 *, size_t *);
blob - 0d76f8ba5eba40827760bba8f38a91b4247b0090
blob + 4cb7d81c1e383ec5222a77a74d215d9b13e3ee0d
--- usr.sbin/relayd/ssl.c
+++ usr.sbin/relayd/ssl.c
@@ -27,30 +27,11 @@
#include
#include
-#include
#include "relayd.h"
intssl_password_cb(char *, int, int, void *);
-void
-ssl_init(struct relayd *env)
-{
- static int initialized = 0;
-
- if (initialized)
- return;
-
- SSL_library_init();
- SSL_load_error_strings();
-
- /* Init hardware crypto engines. */
- ENGINE_load_builtin_engines();
- ENGINE_register_all_complete();
-
- initialized = 1;
-}
-
int
ssl_password_cb(char *buf, int size, int rwflag, void *u)
{
@@ -73,9 +54,6 @@ ssl_load_key(struct relayd *env, const char *name, off
long size;
char*data, *buf = NULL;
- /* Initialize SSL library once */
- ssl_init(env);
-
/*
* Read (possibly) encrypted key from file
*/
blob - 9802ee144e84c38ae747c6f25ce9d4957a84e332
blob + 86b3d032501898656da1bec3e757ff6429201b3b
--- usr.sbin/smtpd/ssl.c
+++ usr.sbin/smtpd/ssl.c
@@ -22,7 +22,6 @@
#include
#include
-#include
#include
#include
#include
@@ -31,25 +30,6 @@ void
#include &quo
Re: smtpd: allow arguments on NOOP
On Fri, 23 Jun 2023 11:58:47 +0200, Omar Polo wrote: > another diff from the -portable repo: > > https://github.com/OpenSMTPD/OpenSMTPD/pull/1150 > > per rfc-5321 § 4.1.1.9 the NOOP command allows optionally one argument > that we SHOULD ignore. > > The original diff set the check function in the commands table to NULL > because NOOP is not a phase that can have filters. However, I prefer > to stay on the safe side and add a smtp_check_noop() function. > Alternatively, we could allow a NULL check function in > smtp_session_imsg(). > > the rfc specifies only one optional string, while here for semplicity > it's relaxed to allow anything. This is a case where being liberal in what you accept seems fine. OK millert@ - todd
Re: smtpd: allow arguments on NOOP
June 23, 2023 11:58 AM, "Omar Polo" wrote:
> another diff from the -portable repo:
>
> https://github.com/OpenSMTPD/OpenSMTPD/pull/1150
>
> per rfc-5321 § 4.1.1.9 the NOOP command allows optionally one argument
> that we SHOULD ignore.
>
> The original diff set the check function in the commands table to NULL
> because NOOP is not a phase that can have filters. However, I prefer
> to stay on the safe side and add a smtp_check_noop() function.
> Alternatively, we could allow a NULL check function in
> smtp_session_imsg().
>
> the rfc specifies only one optional string, while here for semplicity
> it's relaxed to allow anything.
>
> diff /usr/src
> commit - 8def1c1c2777f0b5175283f8116e1eaab1f1962a
> path + /usr/src
> blob - 1686f03e96deeb5e6ea8b065456e04c27c752c8c
> file + usr.sbin/smtpd/smtp_session.c
> --- usr.sbin/smtpd/smtp_session.c
> +++ usr.sbin/smtpd/smtp_session.c
> @@ -212,6 +212,7 @@ static int smtp_check_noparam(struct smtp_session *,
> static int smtp_check_mail_from(struct smtp_session *, const char *);
> static int smtp_check_rcpt_to(struct smtp_session *, const char *);
> static int smtp_check_data(struct smtp_session *, const char *);
> +static int smtp_check_noop(struct smtp_session *, const char *);
> static int smtp_check_noparam(struct smtp_session *, const char *);
>
> static void smtp_filter_phase(enum filter_phase, struct smtp_session *, const
> char *);
> @@ -276,7 +277,7 @@ static struct {
> { CMD_DATA, FILTER_DATA, "DATA", smtp_check_data, smtp_proceed_data },
> { CMD_RSET, FILTER_RSET, "RSET", smtp_check_rset, smtp_proceed_rset },
> { CMD_QUIT, FILTER_QUIT, "QUIT", smtp_check_noparam, smtp_proceed_quit },
> - { CMD_NOOP, FILTER_NOOP, "NOOP", smtp_check_noparam, smtp_proceed_noop },
> + { CMD_NOOP, FILTER_NOOP, "NOOP", smtp_check_noop, smtp_proceed_noop },
> { CMD_HELP, FILTER_HELP, "HELP", smtp_check_noparam, smtp_proceed_help },
> { CMD_WIZ, FILTER_WIZ, "WIZ", smtp_check_noparam, smtp_proceed_wiz },
> { CMD_COMMIT, FILTER_COMMIT, ".", smtp_check_noparam, smtp_proceed_commit },
> @@ -1343,8 +1344,8 @@ smtp_command(struct smtp_session *s, char *line)
> break;
>
> case CMD_NOOP:
> - if (!smtp_check_noparam(s, args))
> - break;
> + if (!smtp_check_noop(s, args))
> + break;
> smtp_filter_phase(FILTER_NOOP, s, NULL);
> break;
>
> @@ -1631,6 +1632,12 @@ smtp_check_noparam(struct smtp_session *s, const char
> }
>
> static int
> +smtp_check_noop(struct smtp_session *s, const char *args)
> +{
> + return 1;
> +}
> +
> +static int
> smtp_check_noparam(struct smtp_session *s, const char *args)
> {
> if (args != NULL) {
This reads fine and you did well adding an smtp_check_noop() because it leaves
room
for hooking this command in filters which is something OpenSMTPD could benefit
from
as it would allow a filter to detect people doing NOOP loops and kick them.
Just my 2cents
smtpd: allow arguments on NOOP
another diff from the -portable repo:
https://github.com/OpenSMTPD/OpenSMTPD/pull/1150
per rfc-5321 § 4.1.1.9 the NOOP command allows optionally one argument
that we SHOULD ignore.
The original diff set the check function in the commands table to NULL
because NOOP is not a phase that can have filters. However, I prefer
to stay on the safe side and add a smtp_check_noop() function.
Alternatively, we could allow a NULL check function in
smtp_session_imsg().
the rfc specifies only one optional string, while here for semplicity
it's relaxed to allow anything.
diff /usr/src
commit - 8def1c1c2777f0b5175283f8116e1eaab1f1962a
path + /usr/src
blob - 1686f03e96deeb5e6ea8b065456e04c27c752c8c
file + usr.sbin/smtpd/smtp_session.c
--- usr.sbin/smtpd/smtp_session.c
+++ usr.sbin/smtpd/smtp_session.c
@@ -212,6 +212,7 @@ static int smtp_check_noparam(struct smtp_session *,
static int smtp_check_mail_from(struct smtp_session *, const char *);
static int smtp_check_rcpt_to(struct smtp_session *, const char *);
static int smtp_check_data(struct smtp_session *, const char *);
+static int smtp_check_noop(struct smtp_session *, const char *);
static int smtp_check_noparam(struct smtp_session *, const char *);
static void smtp_filter_phase(enum filter_phase, struct smtp_session *, const
char *);
@@ -276,7 +277,7 @@ static struct {
{ CMD_DATA, FILTER_DATA,"DATA",
smtp_check_data,smtp_proceed_data },
{ CMD_RSET, FILTER_RSET,"RSET",
smtp_check_rset,smtp_proceed_rset },
{ CMD_QUIT, FILTER_QUIT,"QUIT",
smtp_check_noparam, smtp_proceed_quit },
- { CMD_NOOP, FILTER_NOOP,"NOOP",
smtp_check_noparam, smtp_proceed_noop },
+ { CMD_NOOP, FILTER_NOOP,"NOOP",
smtp_check_noop,smtp_proceed_noop },
{ CMD_HELP, FILTER_HELP,"HELP",
smtp_check_noparam, smtp_proceed_help },
{ CMD_WIZ, FILTER_WIZ, "WIZ",
smtp_check_noparam, smtp_proceed_wiz },
{ CMD_COMMIT, FILTER_COMMIT, ".",
smtp_check_noparam, smtp_proceed_commit },
@@ -1343,8 +1344,8 @@ smtp_command(struct smtp_session *s, char *line)
break;
case CMD_NOOP:
- if (!smtp_check_noparam(s, args))
- break;
+ if (!smtp_check_noop(s, args))
+ break;
smtp_filter_phase(FILTER_NOOP, s, NULL);
break;
@@ -1631,6 +1632,12 @@ smtp_check_noparam(struct smtp_session *s, const char
}
static int
+smtp_check_noop(struct smtp_session *s, const char *args)
+{
+ return 1;
+}
+
+static int
smtp_check_noparam(struct smtp_session *s, const char *args)
{
if (args != NULL) {
Re: avoid truncation of filtered smtpd data lines
On Wed, 21 Jun 2023 19:11:09 +0200, Omar Polo wrote:
> On 2023/06/20 14:38:37 -0600, Todd C. Miller wrote:
> > > qid = ep+1;
> > > - if ((ep = strchr(qid, '|')) == NULL)
> > > - fatalx("Missing reqid: %s", line);
> > > - ep[0] = '\0';
> > > -
> >
> > This is not a new problem but we really should set errno=0 before
> > calling strtoull() for the first time. It is possible for errno
> > to already be set to ERANGE which causes problems if strtoull()
> > returns ULLONG_MAX and there is not an error. It's fine if you
> > don't want to fix that in this commit but I do think it should get
> > fixed.
>
> if you don't mind i'll fix it in a separate commit. I've also checked
> if there were other places to adjust but this appears to be the only
> one instance.
That's perfectly fine.
> > It took me a minute to realize that this is OK, but it seems fine.
> >
> > > if (strcmp(response, "proceed") == 0) {
> > > - if (parameter != NULL)
> > > - fatalx("Unexpected parameter after proceed: %s", line);
> > > filter_protocol_next(token, reqid, 0);
> > > return;
>
> yeah it's subtle, i should have pointed it out, sorry. if "response"
> contain a parameter, strcmp() return nonzero, so the parameter check
> is not needed.
>
> The drawback is that the error messages on protocol violation are a
> bit worst. Before something like "...|proceed|foobar" would fail with
> "unexpected parameter after proceed: ", now "Invalid directive:
> ", but I don't think it's a big deal.
I noticed this and I agree that it is not a big deal. If you are
writing a filter you should know enough to debug the problem with
the amount of detail provided.
OK millert@ for the diff as-is if it was not obvious from my previous
reply.
- todd
Re: avoid truncation of filtered smtpd data lines
On 2023/06/20 14:38:37 -0600, Todd C. Miller wrote:
> > qid = ep+1;
> > - if ((ep = strchr(qid, '|')) == NULL)
> > - fatalx("Missing reqid: %s", line);
> > - ep[0] = '\0';
> > -
>
> This is not a new problem but we really should set errno=0 before
> calling strtoull() for the first time. It is possible for errno
> to already be set to ERANGE which causes problems if strtoull()
> returns ULLONG_MAX and there is not an error. It's fine if you
> don't want to fix that in this commit but I do think it should get
> fixed.
if you don't mind i'll fix it in a separate commit. I've also checked
if there were other places to adjust but this appears to be the only
one instance.
> [...]
>
> It took me a minute to realize that this is OK, but it seems fine.
>
> > if (strcmp(response, "proceed") == 0) {
> > - if (parameter != NULL)
> > - fatalx("Unexpected parameter after proceed: %s", line);
> > filter_protocol_next(token, reqid, 0);
> > return;
yeah it's subtle, i should have pointed it out, sorry. if "response"
contain a parameter, strcmp() return nonzero, so the parameter check
is not needed.
The drawback is that the error messages on protocol violation are a
bit worst. Before something like "...|proceed|foobar" would fail with
"unexpected parameter after proceed: ", now "Invalid directive:
", but I don't think it's a big deal.
> > } else if (strcmp(response, "junk") == 0) {
> > - if (parameter != NULL)
> > - fatalx("Unexpected parameter after junk: %s", line);
> > if (fs->phase == FILTER_COMMIT)
> > fatalx("filter-reponse junk after DATA");
> > filter_result_junk(reqid);
> > @@ -667,11 +648,11 @@ lka_filter_process_response(const char *name, const ch
> > if (parameter == NULL)
> > fatalx("Missing parameter: %s", line);
> >
> > - if (strcmp(response, "rewrite") == 0)
> > + if (strncmp(response, "rewrite|", 8) == 0)
> > filter_result_rewrite(reqid, parameter);
> > - else if (strcmp(response, "reject") == 0)
> > + else if (strncmp(response, "reject|", 7) == 0)
> > filter_result_reject(reqid, parameter);
> > - else if (strcmp(response, "disconnect") == 0)
> > + else if (strncmp(response, "disconnect|", 11) == 0)
> > filter_result_disconnect(reqid, parameter);
> > else
> > fatalx("Invalid directive: %s", line);
> >
diff /usr/src
commit - f47faee0d8945111b03f2db500f23a23f37892bd
path + /usr/src
blob - f0429274168cddb3532c591c6fbc352e0ff7edda
file + usr.sbin/smtpd/lka_filter.c
--- usr.sbin/smtpd/lka_filter.c
+++ usr.sbin/smtpd/lka_filter.c
@@ -605,6 +605,8 @@ lka_filter_process_response(const char *name, const ch
if ((ep = strchr(kind, '|')) == NULL)
fatalx("Missing token: %s", line);
qid = ep+1;
+
+ errno = 0;
reqid = strtoull(qid, , 16);
if (qid[0] == '\0' || *ep != '|')
fatalx("Invalid reqid: %s", line);
Re: [patch] usr.sbin/smtpd filter localhost relays
Hello,
sorry for the delay and thanks for the patch.
On 2023/02/28 12:16:17 +0100, Philipp wrote:
> Hi
>
> On github someone reported an issue[0] regarding localhost MX entries.
> Currently smtpd will just use the localhost relay. This leads to a
> loop. Here a patch filtering localhost and localhost addresses for MX
> requests.
so the report is interesting. due to a typo, a mail was sent to a
wrong host which happen to have "localhost" as MX.
this makes smtpd loop for a while since it connects to localhost to
forward the mail, and connections from localhost are per-config
allowed to send mails to anyone. Rinse and repeat.
after ~2 minutes (not tested myself, the actual number was provided in
the github issue but seems fair) the loop detection (i.e. more than
100 Received headers) kicks in and the mail rejected.
Now, to be honest I don't like the proposed patch. I'm not sure
special-casing "localhost" (and its equivalent spellings) is good.
We're adding code to handle a very fringe case which doesn't really
have bad consequences (one envelope in the queue for a few minutes is
not that of a big deal) and it's not even comprehensive. I don't see
the gain.
Nothing stops me now to change my MX after sending this email to match
the MX of your server and cause you a similar loop when you'll reply.
Or picking an ip in a similarly private blocks like in 192.168.0.0/16
as it's not that uncommon I believe to have a mx that relays mails
from a LAN.
However, I won't object if some other developer think this is good and
wants to commit this or a variation.
Since we're here, some style nits on the diff inlined below.
> As next step you could implement Null-MX (rfc 7505).
This could be worthwile, but wouldn't have helped at all in this case.
It's something the owners of that domain should have used instead of
putting "localhost" as MX.
Patches to support Null-MX are welcome though.
> Philipp
>
> [0] https://github.com/OpenSMTPD/OpenSMTPD/issues/1190
>
> diff --git a/usr.sbin/smtpd/dns.c b/usr.sbin/smtpd/dns.c
> index f7c6b3df..7389efec 100644
> --- a/usr.sbin/smtpd/dns.c
> +++ b/usr.sbin/smtpd/dns.c
> @@ -53,6 +53,7 @@ struct dns_lookup {
> struct dns_session *session;
> char*host;
> int preference;
> + int filter_localhost;
> };
>
> struct dns_session {
> @@ -65,7 +66,7 @@ struct dns_session {
> int refcount;
> };
>
> -static void dns_lookup_host(struct dns_session *, const char *, int);
> +static void dns_lookup_host(struct dns_session *, const char *, int, int);
> static void dns_dispatch_host(struct asr_result *, void *);
> static void dns_dispatch_mx(struct asr_result *, void *);
> static void dns_dispatch_mx_preference(struct asr_result *, void *);
> @@ -139,7 +140,7 @@ dns_imsg(struct mproc *p, struct imsg *imsg)
> case IMSG_MTA_DNS_HOST:
> m_get_string(, );
> m_end();
> - dns_lookup_host(s, host, -1);
> + dns_lookup_host(s, host, -1, 0);
> return;
>
> case IMSG_MTA_DNS_MX:
> @@ -205,6 +206,28 @@ dns_imsg(struct mproc *p, struct imsg *imsg)
> }
> }
>
> +static int
> +is_localhost(struct sockaddr *sa)
> +{
> + struct sockaddr_in6 *ipv6;
> + struct sockaddr_in *ipv4;
> + uint32_t addr;
> +
> + switch (sa->sa_family) {
> + case AF_INET6:
> + // May check also for v6 mapped v4 addresses
please don't use C++-style comments.
> + ipv6 = (struct sockaddr_in6 *)sa;
> + return IN6_IS_ADDR_LOOPBACK(>sin6_addr);
> + case AF_INET:
> + ipv4 = (struct sockaddr_in *)sa;
> + addr = ntohl(ipv4->sin_addr.s_addr);
> + return ((addr >> 24) & 0xff) == 127;
> + default:
> + log_warnx("warn: unknown family in sockaddr");
> + }
> + return 0;
> +}
> +
> static void
> dns_dispatch_host(struct asr_result *ar, void *arg)
> {
> @@ -215,6 +238,10 @@ dns_dispatch_host(struct asr_result *ar, void *arg)
> s = lookup->session;
>
> for (ai = ar->ar_addrinfo; ai; ai = ai->ai_next) {
> + if (lookup->filter_localhost && is_localhost(ai->ai_addr)) {
> + log_warnx("warn: ignore localhost address for host %s",
> lookup->host);
even though is not always respected, please try to keep the lines
under 80 columns.k
> + continue;
> + }
> s->mxfound++;
> m_create(s->p, IMSG_MTA_DNS_HOST, 0, 0, -1);
> m_add_id(s->p, s->reqid);
> @@ -2
Re: avoid truncation of filtered smtpd data lines
On Tue, 20 Jun 2023 21:38:49 +0200, Omar Polo wrote:
> Then I realized that we don't need to copy the line at all. We're
> already using strtoull to parse the number and the payload is the last
> field of the received line, so we can just advance the pointer. The
> drawback is that we now need to use a few strncmp, but I think it's
> worth it.
This seems like a good approach, minor comments inline.
- todd
> diff /usr/src
> commit - 5c586f5f5360442b12bbc4ea18ce006ea0c3d126
> path + /usr/src
> blob - a714446c26fee299f4450ff1ad40289b5b327824
> file + usr.sbin/smtpd/lka_filter.c
> --- usr.sbin/smtpd/lka_filter.c
> +++ usr.sbin/smtpd/lka_filter.c
> @@ -593,40 +593,27 @@ lka_filter_process_response(const char *name, const ch
> {
> uint64_t reqid;
> uint64_t token;
> - char buffer[LINE_MAX];
> char *ep = NULL;
> - char *kind = NULL;
> - char *qid = NULL;
> - /*char *phase = NULL;*/
> - char *response = NULL;
> - char *parameter = NULL;
> + const char *kind = NULL;
> + const char *qid = NULL;
> + const char *response = NULL;
> + const char *parameter = NULL;
> struct filter_session *fs;
>
> - (void)strlcpy(buffer, line, sizeof buffer);
> - if ((ep = strchr(buffer, '|')) == NULL)
> + kind = line;
> +
> + if ((ep = strchr(kind, '|')) == NULL)
> fatalx("Missing token: %s", line);
> - ep[0] = '\0';
> -
> - kind = buffer;
> -
> qid = ep+1;
> - if ((ep = strchr(qid, '|')) == NULL)
> - fatalx("Missing reqid: %s", line);
> - ep[0] = '\0';
> -
This is not a new problem but we really should set errno=0 before
calling strtoull() for the first time. It is possible for errno
to already be set to ERANGE which causes problems if strtoull()
returns ULLONG_MAX and there is not an error. It's fine if you
don't want to fix that in this commit but I do think it should get
fixed.
> reqid = strtoull(qid, , 16);
> - if (qid[0] == '\0' || *ep != '\0')
> + if (qid[0] == '\0' || *ep != '|')
> fatalx("Invalid reqid: %s", line);
> if (errno == ERANGE && reqid == ULLONG_MAX)
> fatal("Invalid reqid: %s", line);
>
> - qid = ep+1;
> - if ((ep = strchr(qid, '|')) == NULL)
> - fatal("Missing directive: %s", line);
> - ep[0] = '\0';
> -
> + qid = ep + 1;
> token = strtoull(qid, , 16);
> - if (qid[0] == '\0' || *ep != '\0')
> + if (qid[0] == '\0' || *ep != '|')
> fatalx("Invalid token: %s", line);
> if (errno == ERANGE && token == ULLONG_MAX)
> fatal("Invalid token: %s", line);
> @@ -637,7 +624,7 @@ lka_filter_process_response(const char *name, const ch
> if ((fs = tree_get(, reqid)) == NULL)
> return;
>
> - if (strcmp(kind, "filter-dataline") == 0) {
> + if (strncmp(kind, "filter-dataline|", 16) == 0) {
> if (fs->phase != FILTER_DATA_LINE)
> fatalx("filter-dataline out of dataline phase");
> filter_data_next(token, reqid, response);
> @@ -646,19 +633,13 @@ lka_filter_process_response(const char *name, const ch
> if (fs->phase == FILTER_DATA_LINE)
> fatalx("filter-result in dataline phase");
>
> - if ((ep = strchr(response, '|'))) {
> + if ((ep = strchr(response, '|')) != NULL)
> parameter = ep + 1;
> - ep[0] = '\0';
> - }
>
It took me a minute to realize that this is OK, but it seems fine.
> if (strcmp(response, "proceed") == 0) {
> - if (parameter != NULL)
> - fatalx("Unexpected parameter after proceed: %s", line);
> filter_protocol_next(token, reqid, 0);
> return;
> } else if (strcmp(response, "junk") == 0) {
> - if (parameter != NULL)
> - fatalx("Unexpected parameter after junk: %s", line);
> if (fs->phase == FILTER_COMMIT)
> fatalx("filter-reponse junk after DATA");
> filter_result_junk(reqid);
> @@ -667,11 +648,11 @@ lka_filter_process_response(const char *name, const ch
> if (parameter == NULL)
> fatalx("Missing parameter: %s", line);
>
> - if (strcmp(response, "rewrite") == 0)
> + if (strncmp(response, "rewrite|", 8) == 0)
> filter_result_rewrite(reqid, parameter);
> - else if (strcmp(response, "reject") == 0)
> + else if (strncmp(response, "reject|", 7) == 0)
> filter_result_reject(reqid, parameter);
> - else if (strcmp(response, "disconnect") == 0)
> + else if (strncmp(response, "disconnect|", 11) == 0)
> filter_result_disconnect(reqid, parameter);
> else
> fatalx("Invalid directive: %s", line);
>
avoid truncation of filtered smtpd data lines
hello tech@,
this was reported some time ago on the OpenSMTPD-portable repository[0]
[0]: https://github.com/OpenSMTPD/OpenSMTPD/pull/1192
Filters can register to the data-line event to alter the mail content.
smtpd, when parsing the filter' output it first copies the received
line in a temporary buffer. Here truncation may happen if a line of
the mail is longer than LINE_MAX (minus 50 to be exact).
The proposed diff on github bumps the local buffer to SMTP_LINE_MAX +
256, but I think it's quite wasteful to have a buffer that long on the
stack (SMTP_LINE_MAX is 65535 bytes).
A very simple fix could be:
: if (strcmp(kind, "filter-dataline") == 0) {
: if (fs->phase != FILTER_DATA_LINE)
: fatalx("filter-dataline out of dataline phase");
: - filter_data_next(token, reqid, response);
: + filter_data_next(token, reqid, line + (response - buffer));
: return;
: }
: if (fs->phase == FILTER_DATA_LINE)
since `line' contains the original line and it's guaranteed that the
fields of the reply excluding the last (variable width) fits the
buffer, it works. maybe it's too clever.
Then I realized that we don't need to copy the line at all. We're
already using strtoull to parse the number and the payload is the last
field of the received line, so we can just advance the pointer. The
drawback is that we now need to use a few strncmp, but I think it's
worth it.
disclaimer: i've run this only on localhost so far.
To reproduce, you can use an awk script like the following
#!/usr/bin/awk -f
# filter-dummy: copies the data lines as-is.
BEGIN { FS = "|" }
/^config\|ready$/ {
print "register|filter|smtp-in|data-line"
print "register|ready"
fflush()
}
"filter|smtp-in|data-line" == $1"|"$4"|"$5 {
line = substr($0, length($1$2$3$4$5$6$7) + 8)
printf "filter-dataline|%s|%s|%s\n", $6, $7, line
fflush()
}
with a configuration like:
table aliases file:/etc/mail/aliases
filter "dummy" proc-exec "/path/to/filter-dummy"
listen on lo0 filter "dummy"
action "local" mbox alias
match from local for local action "local"
and then sending a mail with a very long line:
% tr -cd '[:print:]' phase != FILTER_DATA_LINE)
fatalx("filter-dataline out of dataline phase");
filter_data_next(token, reqid, response);
@@ -646,19 +633,13 @@ lka_filter_process_response(const char *name, const ch
if (fs->phase == FILTER_DATA_LINE)
fatalx("filter-result in dataline phase");
- if ((ep = strchr(response, '|'))) {
+ if ((ep = strchr(response, '|')) != NULL)
parameter = ep + 1;
- ep[0] = '\0';
- }
if (strcmp(response, "proceed") == 0) {
- if (parameter != NULL)
- fatalx("Unexpected parameter after proceed: %s", line);
filter_protocol_next(token, reqid, 0);
return;
} else if (strcmp(response, "junk") == 0) {
- if (parameter != NULL)
- fatalx("Unexpected parameter after junk: %s", line);
if (fs->phase == FILTER_COMMIT)
fatalx("filter-reponse junk after DATA");
filter_result_junk(reqid);
@@ -667,11 +648,11 @@ lka_filter_process_response(const char *name, const ch
if (parameter == NULL)
fatalx("Missing parameter: %s", line);
- if (strcmp(response, "rewrite") == 0)
+ if (strncmp(response, "rewrite|", 8) == 0)
filter_result_rewrite(reqid, parameter);
- else if (strcmp(response, "reject") == 0)
+ else if (strncmp(response, "reject|", 7) == 0)
filter_result_reject(reqid, parameter);
- else if (strcmp(response, "disconnect") == 0)
+ else if (strncmp(response, "disconnect|", 11) == 0)
filter_result_disconnect(reqid, parameter);
else
fatalx("Invalid directive: %s", line);
Re: smtpd: sync imsg_to_str()
On Sun, 18 Jun 2023 16:49:30 +0200, Omar Polo wrote: > some imsg types are missing from the big switch in imsg_to_str(), > noticed after a report in m...@opensmtpd.org. Tracing shows: > > : imsg: lka <- dispatcher: IMSG_??? (139) (len=42) > > (imsg #139 should be IMSG_REPORT_SMTP_FILTER_RESPONSE if I'm counting > right.) > > Instead of checking one by one (they're a lot!) I just copied over the > list from smtpd.h and ran an emacs macro. Some entries changed place, > but since the list is long I figured this was the best way to keep > everything in sync. Using the same order as smtpd.h makes sense. OK millert@ - todd
smtpd: sync imsg_to_str()
some imsg types are missing from the big switch in imsg_to_str(), noticed after a report in m...@opensmtpd.org. Tracing shows: : imsg: lka <- dispatcher: IMSG_??? (139) (len=42) (imsg #139 should be IMSG_REPORT_SMTP_FILTER_RESPONSE if I'm counting right.) Instead of checking one by one (they're a lot!) I just copied over the list from smtpd.h and ran an emacs macro. Some entries changed place, but since the list is long I figured this was the best way to keep everything in sync. ok? diff /usr/src commit - af580bd60cce9d8599fddb1cffa69d16b70ae3a7 path + /usr/src blob - 0bd24de8a65d0655a9866c5d3e66ad82a152959a file + usr.sbin/smtpd/smtpd.c --- usr.sbin/smtpd/smtpd.c +++ usr.sbin/smtpd/smtpd.c @@ -2081,19 +2081,22 @@ imsg_to_str(int type) CASE(IMSG_REPORT_SMTP_LINK_CONNECT); CASE(IMSG_REPORT_SMTP_LINK_DISCONNECT); - CASE(IMSG_REPORT_SMTP_LINK_TLS); CASE(IMSG_REPORT_SMTP_LINK_GREETING); CASE(IMSG_REPORT_SMTP_LINK_IDENTIFY); + CASE(IMSG_REPORT_SMTP_LINK_TLS); CASE(IMSG_REPORT_SMTP_LINK_AUTH); - CASE(IMSG_REPORT_SMTP_TX_RESET); CASE(IMSG_REPORT_SMTP_TX_BEGIN); + CASE(IMSG_REPORT_SMTP_TX_MAIL); + CASE(IMSG_REPORT_SMTP_TX_RCPT); CASE(IMSG_REPORT_SMTP_TX_ENVELOPE); + CASE(IMSG_REPORT_SMTP_TX_DATA); CASE(IMSG_REPORT_SMTP_TX_COMMIT); CASE(IMSG_REPORT_SMTP_TX_ROLLBACK); - CASE(IMSG_REPORT_SMTP_PROTOCOL_CLIENT); CASE(IMSG_REPORT_SMTP_PROTOCOL_SERVER); + CASE(IMSG_REPORT_SMTP_FILTER_RESPONSE); + CASE(IMSG_REPORT_SMTP_TIMEOUT); CASE(IMSG_FILTER_SMTP_BEGIN); CASE(IMSG_FILTER_SMTP_END); @@ -2104,6 +2107,7 @@ imsg_to_str(int type) CASE(IMSG_CA_RSA_PRIVENC); CASE(IMSG_CA_RSA_PRIVDEC); CASE(IMSG_CA_ECDSA_SIGN); + default: (void)snprintf(buf, sizeof(buf), "IMSG_??? (%d)", type);
Re: smtpd-filters: swap link-auth fields
On Wed, 14 Jun 2023 16:34:39 +0200, Omar Polo wrote: > the `link-auth' event hash the user first and the result of the > operation after; this breaks when a username has a '|' character in > it. Since this is triggered by the `auth login' command, anyone could > send a user with a '|' and, depending on the filter used, make smtpd > exit. (if the filter dies, smtpd does too) > > This was reported on the OpenSMTPD-portable github repository with > Gilles' opensmtpd-filter-rspamd: > > https://github.com/OpenSMTPD/OpenSMTPD/issues/1213 > > Diff below is straightforward and includes the documentation changes. > I believe link-auth was forgotten in revision 1.61 of lka_filter.c > when the mail-from/rcpt-to events got their fields swapped. OK millert@ - todd
Re: smtpd-filters: swap link-auth fields
Just released a new filter-rspamd with your diff, thanks > On 14 Jun 2023, at 19:23, Omar Polo wrote: > > Hello, > > the `link-auth' event hash the user first and the result of the > operation after; this breaks when a username has a '|' character in > it. Since this is triggered by the `auth login' command, anyone could > send a user with a '|' and, depending on the filter used, make smtpd > exit. (if the filter dies, smtpd does too) > > This was reported on the OpenSMTPD-portable github repository with > Gilles' opensmtpd-filter-rspamd: > >https://github.com/OpenSMTPD/OpenSMTPD/issues/1213 > > Diff below is straightforward and includes the documentation changes. > I believe link-auth was forgotten in revision 1.61 of lka_filter.c > when the mail-from/rcpt-to events got their fields swapped. > > For opensmtpd-filter-rspamd I have a corresponding diff that I'll send > to Gilles as it is off-topic for tech@, but here it is too if you want > to play with it: > >https://paste.omarpolo.com/9jtli2w > > To reproduce: (there may be quicker ways, this is just the first i > found) > ># pkg_add rspamd opensmtpd-filter-rspamd ># rcctl enable rspamd ># rcctl start rspamd > > add the rspamd filter to /etc/mail/smtpd.conf > >filter "rspamd" proc-exec "filter-rspamd" >listen on lo0 smtps pki localhost auth filter "rspamd" > > and try to do a login: > >$ nc -c -Tnoverify localhost 465 >helo localhost >auth login >b3xw >MTMyNA== > > > Thanks, > > Omar Polo > > > diff /usr/src > commit - 66c6b79616659a94b04092c9f103e3aa29809704 > path + /usr/src > blob - 0c63657be21352fb1f060505250f7a9ef4fc8d8c > file + usr.sbin/smtpd/lka_filter.c > --- usr.sbin/smtpd/lka_filter.c > +++ usr.sbin/smtpd/lka_filter.c > @@ -24,7 +24,7 @@ > #include "smtpd.h" > #include "log.h" > > -#definePROTOCOL_VERSION"0.6" > +#definePROTOCOL_VERSION"0.7" > > struct filter; > struct filter_session; > @@ -1461,7 +1461,7 @@ lka_report_smtp_link_auth(const char *direction, struc >fs->username = xstrdup(username); >} > report_smtp_broadcast(reqid, direction, tv, "link-auth", "%s|%s\n", > -username, result); > +result, username); > } > > void > blob - 313404c111c77b099b3855f43252c26877874b17 > file + usr.sbin/smtpd/smtpd-filters.7 > --- usr.sbin/smtpd/smtpd-filters.7 > +++ usr.sbin/smtpd/smtpd-filters.7 > @@ -271,12 +271,9 @@ This event is generated upon disconnection of the clie > the cipher suite used by the session and the cipher strength in bits. > .It Ic link-disconnect > This event is generated upon disconnection of the client. > -.It Ic link-auth : Ar username result > +.It Ic link-auth : Ar result username > This event is generated upon an authentication attempt by the client. > .Pp > -.Ar username > -contains the username used for the authentication attempt. > -.Pp > .Ar result > contains the string > .Dq pass , > @@ -284,6 +281,9 @@ depending on the result of the authentication attempt. > or > .Dq error > depending on the result of the authentication attempt. > +.Pp > +.Ar username > +contains the username used for the authentication attempt. > .It Ic tx-reset : Op message-id > This event is generated when a transaction is reset. > .Pp
Re: smtpd-filters: swap link-auth fields
On 2023/06/14 16:34:39 +0200, Omar Polo wrote: > For opensmtpd-filter-rspamd I have a corresponding diff that I'll send > to Gilles as it is off-topic for tech@, but here it is too if you want > to play with it: > > https://paste.omarpolo.com/9jtli2w apologize, this one has a stupid typo. I've opend a PR on github with an updated diff. https://github.com/poolpOrg/filter-rspamd/pull/46 sorry for the noise.
smtpd-filters: swap link-auth fields
Hello, the `link-auth' event hash the user first and the result of the operation after; this breaks when a username has a '|' character in it. Since this is triggered by the `auth login' command, anyone could send a user with a '|' and, depending on the filter used, make smtpd exit. (if the filter dies, smtpd does too) This was reported on the OpenSMTPD-portable github repository with Gilles' opensmtpd-filter-rspamd: https://github.com/OpenSMTPD/OpenSMTPD/issues/1213 Diff below is straightforward and includes the documentation changes. I believe link-auth was forgotten in revision 1.61 of lka_filter.c when the mail-from/rcpt-to events got their fields swapped. For opensmtpd-filter-rspamd I have a corresponding diff that I'll send to Gilles as it is off-topic for tech@, but here it is too if you want to play with it: https://paste.omarpolo.com/9jtli2w To reproduce: (there may be quicker ways, this is just the first i found) # pkg_add rspamd opensmtpd-filter-rspamd # rcctl enable rspamd # rcctl start rspamd add the rspamd filter to /etc/mail/smtpd.conf filter "rspamd" proc-exec "filter-rspamd" listen on lo0 smtps pki localhost auth filter "rspamd" and try to do a login: $ nc -c -Tnoverify localhost 465 helo localhost auth login b3xw MTMyNA== Thanks, Omar Polo diff /usr/src commit - 66c6b79616659a94b04092c9f103e3aa29809704 path + /usr/src blob - 0c63657be21352fb1f060505250f7a9ef4fc8d8c file + usr.sbin/smtpd/lka_filter.c --- usr.sbin/smtpd/lka_filter.c +++ usr.sbin/smtpd/lka_filter.c @@ -24,7 +24,7 @@ #include "smtpd.h" #include "log.h" -#definePROTOCOL_VERSION"0.6" +#definePROTOCOL_VERSION"0.7" struct filter; struct filter_session; @@ -1461,7 +1461,7 @@ lka_report_smtp_link_auth(const char *direction, struc fs->username = xstrdup(username); } report_smtp_broadcast(reqid, direction, tv, "link-auth", "%s|%s\n", - username, result); + result, username); } void blob - 313404c111c77b099b3855f43252c26877874b17 file + usr.sbin/smtpd/smtpd-filters.7 --- usr.sbin/smtpd/smtpd-filters.7 +++ usr.sbin/smtpd/smtpd-filters.7 @@ -271,12 +271,9 @@ This event is generated upon disconnection of the clie the cipher suite used by the session and the cipher strength in bits. .It Ic link-disconnect This event is generated upon disconnection of the client. -.It Ic link-auth : Ar username result +.It Ic link-auth : Ar result username This event is generated upon an authentication attempt by the client. .Pp -.Ar username -contains the username used for the authentication attempt. -.Pp .Ar result contains the string .Dq pass , @@ -284,6 +281,9 @@ depending on the result of the authentication attempt. or .Dq error depending on the result of the authentication attempt. +.Pp +.Ar username +contains the username used for the authentication attempt. .It Ic tx-reset : Op message-id This event is generated when a transaction is reset. .Pp
Re: libtls, smtpd: switch to EC_KEY_METHOD
On 2023/05/25 19:23:48 +0200, Omar Polo wrote:
> As far as I (and grep) can see, smtpd and the part it needs in libtls
> are the only user of ECDSA_METHOD in tree.
>
> What I've understood talking with tb (and apologizes if I'm making
> mistakes) is that ECDSA_METHOD was replaced with EC_KEY_METHOD. "We"
> inherited the former, it got used in smtpd, and then added the latter
> for openssh. smtpd and libtls were never updated to these new shiny
> APIs.
>
> Diff below is 99% gilles' work on OpenSMTPD-portable. I only had to
> tweak EC_KEY_METHOD_get_compute_key() since the compute key function
> has a different signature in LibreSSL than OpenSSL, and some minor
> style nits.
friendly ping
> While I've tested it (on localhost and between vms), and I'm also
> running it on linux and freebsd with OpenSSL 3.1 and 1.1 respectively
> via OpenSMTPD-portable, additional testing on busier mx is greatly
> appreciated. I don't expect regressions however.
I'm also running with this on my backup mx. I've stopped the main mx
a few times to have all the traffic going through the backup one.
Nothing to report :)
more testing is always apreciated.
> To test:
>
> - apply the diff
> - rebuild and reinstall libtls
> - rebuild, reinstall and restart smtpd
forgot to point out one thing, apologies. You need to have a
certificate with an ec key. For acme-client it means having something
like the following:
domain m.example.com {
# note the `ecdsa' on the following line.
domain key "/etc/ssl/private/m.example.com.key" ecdsa
domain full chain certificate "..."
sign with foobar
}
note also that acme-client won't re-generate the private key, so you
may have to delete it prior requesting a new certificate since it
defaults to rsa.
> It doesn't change the libtls ABI (tls_signer_ecdsa_method is internal)
> and the parts it touches are only used by smtpd AFAIK, so no need to
> rebuild anything else.
>
>
> Thanks!
diff 2b0e5a68588bd522338c80975b42b1e5c355be29
3ea882bd0e5cf2ef7f577629af6affb53aef7775
commit - 2b0e5a68588bd522338c80975b42b1e5c355be29
commit + 3ea882bd0e5cf2ef7f577629af6affb53aef7775
blob - 989339dc033f3c231659a9a37946c453d03509da
blob + f6ba5760737d40ec5250a21c0bdcc7446073111f
--- lib/libtls/tls.c
+++ lib/libtls/tls.c
@@ -389,7 +389,7 @@ tls_keypair_setup_pkey(struct tls *ctx, struct tls_key
tls_keypair_setup_pkey(struct tls *ctx, struct tls_keypair *keypair, EVP_PKEY
*pkey)
{
RSA_METHOD *rsa_method;
- ECDSA_METHOD *ecdsa_method;
+ EC_KEY_METHOD *ecdsa_method;
RSA *rsa = NULL;
EC_KEY *eckey = NULL;
int ret = -1;
@@ -427,15 +427,15 @@ tls_keypair_setup_pkey(struct tls *ctx, struct tls_key
break;
case EVP_PKEY_EC:
if ((eckey = EVP_PKEY_get1_EC_KEY(pkey)) == NULL ||
- ECDSA_set_ex_data(eckey, 0, keypair->pubkey_hash) == 0) {
+ EC_KEY_set_ex_data(eckey, 0, keypair->pubkey_hash) == 0) {
tls_set_errorx(ctx, "EC key setup failure");
goto err;
}
if (ctx->config->sign_cb != NULL) {
ecdsa_method = tls_signer_ecdsa_method();
if (ecdsa_method == NULL ||
- ECDSA_set_ex_data(eckey, 1, ctx->config) == 0 ||
- ECDSA_set_method(eckey, ecdsa_method) == 0) {
+ EC_KEY_set_ex_data(eckey, 1, ctx->config) == 0 ||
+ EC_KEY_set_method(eckey, ecdsa_method) == 0) {
tls_set_errorx(ctx, "failed to setup EC key");
goto err;
}
blob - f4c23f64e67f2f45056467dbdffe84960cdc4e2c
blob + f53b6c800941a746425ba01ffe26daf4c236bc37
--- lib/libtls/tls_internal.h
+++ lib/libtls/tls_internal.h
@@ -298,7 +298,7 @@ ECDSA_METHOD *tls_signer_ecdsa_method(void);
int tls_password_cb(char *_buf, int _size, int _rwflag, void *_u);
RSA_METHOD *tls_signer_rsa_method(void);
-ECDSA_METHOD *tls_signer_ecdsa_method(void);
+EC_KEY_METHOD *tls_signer_ecdsa_method(void);
#define TLS_PADDING_NONE 0
#define TLS_PADDING_RSA_PKCS1 1
blob - f6005d3e07ac6423098c9d35f1f03993cd4dfd88
blob + 93777aa3253fdacbc28abc852f8e660fbd882b01
--- lib/libtls/tls_signer.c
+++ lib/libtls/tls_signer.c
@@ -419,26 +419,21 @@ ECDSA_METHOD *
return (NULL);
}
-ECDSA_METHOD *
+EC_KEY_METHOD *
tls_signer_ecdsa_method(void)
{
- static ECDSA_METHOD *ecdsa_method = NULL;
+ static EC_KEY_METHOD *ecdsa_method = NULL;
pthread_mutex_lock(_method_lock);
if (ecdsa_method != NULL)
goto out;
- ecdsa_method = calloc(1, si
Re: smtpd: add missing time.h include
On Wed, 31 May 2023 11:00:37 +0200, Omar Polo wrote: > After a report of a build fail with some old gcc on RHEL7 / Centos, I > noticed that we're lacking the include time.h for time(3), > clock_gettime(3) and localtime(3). Diff below adds it in all the > missing files. I'm also including sys/time.h in smtpd.h, as noted in > event_init(3), since we're including event.h. OK millert@ - todd
smtpd: add missing time.h include
Another boring diff from opensmtpd-portable. After a report of a build fail with some old gcc on RHEL7 / Centos, I noticed that we're lacking the include time.h for time(3), clock_gettime(3) and localtime(3). Diff below adds it in all the missing files. I'm also including sys/time.h in smtpd.h, as noted in event_init(3), since we're including event.h. It wouldn't be an issue to keep this in -portable, but since the header is genuinely missing I'd prefer to have it fixed in base too instead of relying on some other header to include it. diff /usr/src commit - 79631e141468cced94e502d777a484fa0eb1f60f path + /usr/src blob - 61e7b037bd90d2397e98e52cbb68e2436478b9b2 file + usr.sbin/smtpd/bounce.c --- usr.sbin/smtpd/bounce.c +++ usr.sbin/smtpd/bounce.c @@ -22,6 +22,7 @@ #include #include #include +#include #include #include "smtpd.h" blob - 835ab5520eed8d8bbfcce21e9571f07ae89db97c file + usr.sbin/smtpd/control.c --- usr.sbin/smtpd/control.c +++ usr.sbin/smtpd/control.c @@ -26,6 +26,7 @@ #include #include #include +#include #include #include "smtpd.h" blob - c90b60d2bb3ae7046621a4f576a900fe5557ebfd file + usr.sbin/smtpd/enqueue.c --- usr.sbin/smtpd/enqueue.c +++ usr.sbin/smtpd/enqueue.c @@ -24,6 +24,7 @@ #include #include #include +#include #include #include "smtpd.h" blob - 894bf865a7662ce51138168aa0436fde6c9e7b44 file + usr.sbin/smtpd/mda.c --- usr.sbin/smtpd/mda.c +++ usr.sbin/smtpd/mda.c @@ -24,6 +24,7 @@ #include #include #include +#include #include #include blob - 05506da1dbef6fb33f23386727977c8e9118f2a8 file + usr.sbin/smtpd/mta.c --- usr.sbin/smtpd/mta.c +++ usr.sbin/smtpd/mta.c @@ -22,6 +22,7 @@ #include #include #include +#include #include #include "smtpd.h" blob - 92f1ec7705d066698a7e24455b86774b86ccbb9c file + usr.sbin/smtpd/mta_session.c --- usr.sbin/smtpd/mta_session.c +++ usr.sbin/smtpd/mta_session.c @@ -26,6 +26,7 @@ #include #include #include +#include #include #include blob - e79e3f06be4fdf53e87596a6e10aa79fbe0ffde8 file + usr.sbin/smtpd/queue.c --- usr.sbin/smtpd/queue.c +++ usr.sbin/smtpd/queue.c @@ -23,6 +23,7 @@ #include #include #include +#include #include #include "smtpd.h" blob - 646cd629879ba9c28d6ecaff8be2adef0cea0b7f file + usr.sbin/smtpd/queue_backend.c --- usr.sbin/smtpd/queue_backend.c +++ usr.sbin/smtpd/queue_backend.c @@ -23,6 +23,7 @@ #include #include #include +#include #include #include "smtpd.h" blob - ec539eb9e1123fef50027467d430b94d688232b4 file + usr.sbin/smtpd/queue_fs.c --- usr.sbin/smtpd/queue_fs.c +++ usr.sbin/smtpd/queue_fs.c @@ -28,6 +28,7 @@ #include #include #include +#include #include #include "smtpd.h" blob - 24ca71ca9ee765bcd6f1c05a24749ec6abce2ca8 file + usr.sbin/smtpd/runq.c --- usr.sbin/smtpd/runq.c +++ usr.sbin/smtpd/runq.c @@ -17,6 +17,7 @@ */ #include +#include #include "smtpd.h" blob - fa3b951bc77242dc73dd56d484e576b0ac6ffe8d file + usr.sbin/smtpd/scheduler_ramqueue.c --- usr.sbin/smtpd/scheduler_ramqueue.c +++ usr.sbin/smtpd/scheduler_ramqueue.c @@ -20,6 +20,7 @@ #include #include #include +#include #include "smtpd.h" #include "log.h" blob - 72e13e8fd8d32d748cb64567953d52612a8140ff file + usr.sbin/smtpd/smtp_session.c --- usr.sbin/smtpd/smtp_session.c +++ usr.sbin/smtpd/smtp_session.c @@ -24,6 +24,7 @@ #include #include #include +#include #include #include #include blob - 84663025648861b28f691f99940938000c17872b file + usr.sbin/smtpd/smtpctl.c --- usr.sbin/smtpd/smtpctl.c +++ usr.sbin/smtpd/smtpctl.c @@ -32,6 +32,7 @@ #include #include #include +#include #include #include blob - 5949ce05522f4675aabc72042052ec0ef2025881 file + usr.sbin/smtpd/smtpd.c --- usr.sbin/smtpd/smtpd.c +++ usr.sbin/smtpd/smtpd.c @@ -36,6 +36,7 @@ #include #include #include +#include #include #include blob - 65757e517fdbe27cedea42656835d0ac7ef20c6b file + usr.sbin/smtpd/smtpd.h --- usr.sbin/smtpd/smtpd.h +++ usr.sbin/smtpd/smtpd.h @@ -25,6 +25,7 @@ #include #include #include +#include #include #include blob - 4a5c692e9508f763e6a2e83839afd6c9994821e6 file + usr.sbin/smtpd/to.c --- usr.sbin/smtpd/to.c +++ usr.sbin/smtpd/to.c @@ -23,6 +23,7 @@ #include #include #include +#include #if IO_TLS #include #endif
Re: libtls, smtpd: switch to EC_KEY_METHOD
On Thu, May 25, 2023 at 07:23:48PM +0200, Omar Polo wrote: > As far as I (and grep) can see, smtpd and the part it needs in libtls > are the only user of ECDSA_METHOD in tree. Yes, nothing else should be using this anymore, including ports. ECDSA_METHOD and ECDH_METHOD were merged into EC_KEY_METHOD and removed as part of the OpenSSL 1.1 API transition. We still have the former two because removal is blocked by smtpd, so this diff is definitely a most welcome step in the right direction. >From the libcrypto perspective this paves the way for removal of a still exposed public struct and unneded API and will subsequently allow more internal simplifications. In particular, it should allow us to clean up and get rid of the messy split between ec/ecdsa/ecdh. > What I've understood talking with tb (and apologizes if I'm making > mistakes) is that ECDSA_METHOD was replaced with EC_KEY_METHOD. "We" > inherited the former, it got used in smtpd, and then added the latter > for openssh. smtpd and libtls were never updated to these new shiny > APIs. This is also correct. We (markus) added EC_KEY_METHOD to libcrypto to allow OpenSSH to switch to OpenSSL 1.1 API which simplified their portable efforts. (Now EC_KEY_METHOD is deprecated in OpenSSL 3 thanks to the new provider things, but one step at a time...) > Diff below is 99% gilles' work on OpenSMTPD-portable. I only had to > tweak EC_KEY_METHOD_get_compute_key() since the compute key function > has a different signature in LibreSSL than OpenSSL, and some minor > style nits. Addressing this signature difference will something to look into later. > While I've tested it (on localhost and between vms), and I'm also > running it on linux and freebsd with OpenSSL 3.1 and 1.1 respectively > via OpenSMTPD-portable, additional testing on busier mx is greatly > appreciated. I don't expect regressions however. It should also be noted that libretls carries a similar diff. > > To test: > > - apply the diff > - rebuild and reinstall libtls > - rebuild, reinstall and restart smtpd > > It doesn't change the libtls ABI (tls_signer_ecdsa_method is internal) > and the parts it touches are only used by smtpd AFAIK, so no need to > rebuild anything else. I am ok with the diff, but some more testing in the real world would be nice.
libtls, smtpd: switch to EC_KEY_METHOD
As far as I (and grep) can see, smtpd and the part it needs in libtls
are the only user of ECDSA_METHOD in tree.
What I've understood talking with tb (and apologizes if I'm making
mistakes) is that ECDSA_METHOD was replaced with EC_KEY_METHOD. "We"
inherited the former, it got used in smtpd, and then added the latter
for openssh. smtpd and libtls were never updated to these new shiny
APIs.
Diff below is 99% gilles' work on OpenSMTPD-portable. I only had to
tweak EC_KEY_METHOD_get_compute_key() since the compute key function
has a different signature in LibreSSL than OpenSSL, and some minor
style nits.
While I've tested it (on localhost and between vms), and I'm also
running it on linux and freebsd with OpenSSL 3.1 and 1.1 respectively
via OpenSMTPD-portable, additional testing on busier mx is greatly
appreciated. I don't expect regressions however.
To test:
- apply the diff
- rebuild and reinstall libtls
- rebuild, reinstall and restart smtpd
It doesn't change the libtls ABI (tls_signer_ecdsa_method is internal)
and the parts it touches are only used by smtpd AFAIK, so no need to
rebuild anything else.
Thanks!
diff a6995f7d4e4b475f514b46014b476ba2fb99e6ca
15eb8637ab039139400e655284e2e2d8ca898a03
commit - a6995f7d4e4b475f514b46014b476ba2fb99e6ca
commit + 15eb8637ab039139400e655284e2e2d8ca898a03
blob - 989339dc033f3c231659a9a37946c453d03509da
blob + f6ba5760737d40ec5250a21c0bdcc7446073111f
--- lib/libtls/tls.c
+++ lib/libtls/tls.c
@@ -389,7 +389,7 @@ tls_keypair_setup_pkey(struct tls *ctx, struct tls_key
tls_keypair_setup_pkey(struct tls *ctx, struct tls_keypair *keypair, EVP_PKEY
*pkey)
{
RSA_METHOD *rsa_method;
- ECDSA_METHOD *ecdsa_method;
+ EC_KEY_METHOD *ecdsa_method;
RSA *rsa = NULL;
EC_KEY *eckey = NULL;
int ret = -1;
@@ -427,15 +427,15 @@ tls_keypair_setup_pkey(struct tls *ctx, struct tls_key
break;
case EVP_PKEY_EC:
if ((eckey = EVP_PKEY_get1_EC_KEY(pkey)) == NULL ||
- ECDSA_set_ex_data(eckey, 0, keypair->pubkey_hash) == 0) {
+ EC_KEY_set_ex_data(eckey, 0, keypair->pubkey_hash) == 0) {
tls_set_errorx(ctx, "EC key setup failure");
goto err;
}
if (ctx->config->sign_cb != NULL) {
ecdsa_method = tls_signer_ecdsa_method();
if (ecdsa_method == NULL ||
- ECDSA_set_ex_data(eckey, 1, ctx->config) == 0 ||
- ECDSA_set_method(eckey, ecdsa_method) == 0) {
+ EC_KEY_set_ex_data(eckey, 1, ctx->config) == 0 ||
+ EC_KEY_set_method(eckey, ecdsa_method) == 0) {
tls_set_errorx(ctx, "failed to setup EC key");
goto err;
}
blob - f4c23f64e67f2f45056467dbdffe84960cdc4e2c
blob + f53b6c800941a746425ba01ffe26daf4c236bc37
--- lib/libtls/tls_internal.h
+++ lib/libtls/tls_internal.h
@@ -298,7 +298,7 @@ ECDSA_METHOD *tls_signer_ecdsa_method(void);
int tls_password_cb(char *_buf, int _size, int _rwflag, void *_u);
RSA_METHOD *tls_signer_rsa_method(void);
-ECDSA_METHOD *tls_signer_ecdsa_method(void);
+EC_KEY_METHOD *tls_signer_ecdsa_method(void);
#define TLS_PADDING_NONE 0
#define TLS_PADDING_RSA_PKCS1 1
blob - f6005d3e07ac6423098c9d35f1f03993cd4dfd88
blob + 93777aa3253fdacbc28abc852f8e660fbd882b01
--- lib/libtls/tls_signer.c
+++ lib/libtls/tls_signer.c
@@ -419,26 +419,21 @@ ECDSA_METHOD *
return (NULL);
}
-ECDSA_METHOD *
+EC_KEY_METHOD *
tls_signer_ecdsa_method(void)
{
- static ECDSA_METHOD *ecdsa_method = NULL;
+ static EC_KEY_METHOD *ecdsa_method = NULL;
pthread_mutex_lock(_method_lock);
if (ecdsa_method != NULL)
goto out;
- ecdsa_method = calloc(1, sizeof(*ecdsa_method));
+ ecdsa_method = EC_KEY_METHOD_new(NULL);
if (ecdsa_method == NULL)
goto out;
- ecdsa_method->ecdsa_do_sign = tls_ecdsa_do_sign;
- ecdsa_method->name = strdup("libtls ECDSA method");
- if (ecdsa_method->name == NULL) {
- free(ecdsa_method);
- ecdsa_method = NULL;
- }
+ EC_KEY_METHOD_set_sign(ecdsa_method, NULL, NULL, tls_ecdsa_do_sign);
out:
pthread_mutex_unlock(_method_lock);
blob - c0c918601e741019515da6c83a6874fb9f552a1a
blob + f5d81174b597f8368b45719833d92772e49e78db
--- usr.sbin/smtpd/ca.c
+++ usr.sbin/smtpd/ca.c
@@ -47,10 +47,17 @@ static int rsae_keygen(RSA *, int, BIGNUM *, BN_GENCB
static int rsae_init(RSA *);
static int rsae_finish(RSA *);
static int rsae_keygen(RSA *, int, BIGNUM *, BN_GENCB *);
+static int ecdsae_keygen(EC_KEY *);
+static int ecdsae_compute_key(void *, size_t, c
Re: smtpd: some fatal -> fatalx
On Tue, May 16, 2023 at 02:51:44PM +0200, Omar Polo wrote: > while debugging a pebkac in -portable, I noticed that in various > places we use fatal() for libtls failures. errno doesn't generally > contains anything useful after libtls functions, and in most it's > explicitly cleared to avoid misuse. > > just to provide a quick example, with `listen on ... ciphers foobar': > > % doas smtpd -d > info: OpenSMTPD 7.0.0 starting > dispatcher: no ciphers for 'foobar': No such file or directory > smtpd: process dispatcher socket closed > > So change most of them to fatalx which doesn't append errno. While > here I'm also logging the actual error, via tls_config_error() or > tls_error(), that before was missing. > > tls_config_new(), tls_server() and tls_client() failures are still > logged with fatal(), which I believe it's correct. > > ok? > make sense, ok giovanni@ Cheers Giovanni
Re: smtpd: some fatal -> fatalx
On Tue, 16 May 2023 14:51:44 +0200, Omar Polo wrote: > while debugging a pebkac in -portable, I noticed that in various > places we use fatal() for libtls failures. errno doesn't generally > contains anything useful after libtls functions, and in most it's > explicitly cleared to avoid misuse. > > just to provide a quick example, with `listen on ... ciphers foobar': > > % doas smtpd -d > info: OpenSMTPD 7.0.0 starting > dispatcher: no ciphers for 'foobar': No such file or directory > smtpd: process dispatcher socket closed > > So change most of them to fatalx which doesn't append errno. While > here I'm also logging the actual error, via tls_config_error() or > tls_error(), that before was missing. > > tls_config_new(), tls_server() and tls_client() failures are still > logged with fatal(), which I believe it's correct. OK millert@ - todd
smtpd: some fatal -> fatalx
while debugging a pebkac in -portable, I noticed that in various
places we use fatal() for libtls failures. errno doesn't generally
contains anything useful after libtls functions, and in most it's
explicitly cleared to avoid misuse.
just to provide a quick example, with `listen on ... ciphers foobar':
% doas smtpd -d
info: OpenSMTPD 7.0.0 starting
dispatcher: no ciphers for 'foobar': No such file or directory
smtpd: process dispatcher socket closed
So change most of them to fatalx which doesn't append errno. While
here I'm also logging the actual error, via tls_config_error() or
tls_error(), that before was missing.
tls_config_new(), tls_server() and tls_client() failures are still
logged with fatal(), which I believe it's correct.
ok?
diff /usr/src
commit - 27d3d13aceb86ba91128d3f12ca9fc893d83fa86
path + /usr/src
blob - dbcf2c0158186f1a3077c25d746c9a8d7a8ad9d0
file + usr.sbin/smtpd/mta.c
--- usr.sbin/smtpd/mta.c
+++ usr.sbin/smtpd/mta.c
@@ -489,38 +489,41 @@ mta_setup_dispatcher(struct dispatcher *dispatcher)
if (remote->tls_ciphers)
ciphers = remote->tls_ciphers;
if (ciphers && tls_config_set_ciphers(config, ciphers) == -1)
- fatal("%s", tls_config_error(config));
+ fatalx("%s", tls_config_error(config));
if (remote->tls_protocols) {
if (tls_config_parse_protocols(,
remote->tls_protocols) == -1)
- fatal("failed to parse protocols \"%s\"",
+ fatalx("failed to parse protocols \"%s\"",
remote->tls_protocols);
if (tls_config_set_protocols(config, protos) == -1)
- fatal("%s", tls_config_error(config));
+ fatalx("%s", tls_config_error(config));
}
if (remote->pki) {
pki = dict_get(env->sc_pki_dict, remote->pki);
if (pki == NULL)
- fatal("client pki \"%s\" not found ", remote->pki);
+ fatalx("client pki \"%s\" not found", remote->pki);
tls_config_set_dheparams(config, dheparams[pki->pki_dhe]);
tls_config_use_fake_private_key(config);
if (tls_config_set_keypair_mem(config, pki->pki_cert,
pki->pki_cert_len, NULL, 0) == -1)
- fatal("tls_config_set_keypair_mem");
+ fatalx("tls_config_set_keypair_mem: %s",
+ tls_config_error(config));
}
if (remote->ca) {
ca = dict_get(env->sc_ca_dict, remote->ca);
if (tls_config_set_ca_mem(config, ca->ca_cert, ca->ca_cert_len)
== -1)
- fatal("tls_config_set_ca_mem");
+ 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)
- fatal("tls_config_set_ca_file");
+ fatalx("tls_config_set_ca_file: %s",
+ tls_config_error(config));
if (remote->tls_verify) {
tls_config_verify(config);
blob - a9b7d48c8a5fe3e1af6d32429556f09b7579583b
file + usr.sbin/smtpd/smtp.c
--- usr.sbin/smtpd/smtp.c
+++ usr.sbin/smtpd/smtp.c
@@ -166,14 +166,14 @@ smtp_setup_listener_tls(struct listener *l)
if (l->tls_ciphers)
ciphers = l->tls_ciphers;
if (ciphers && tls_config_set_ciphers(config, ciphers) == -1)
- fatal("%s", tls_config_error(config));
+ fatalx("%s", tls_config_error(config));
if (l->tls_protocols) {
if (tls_config_parse_protocols(, l->tls_protocols) == -1)
- fatal("failed to parse protocols \"%s\"",
+ fatalx("failed to parse protocols \"%s\"",
l->tls_protocols);
if (tls_config_set_protocols(config, protos) == -1)
- fatal("%s", tls_config_error(config));
+ fatalx("%s", tls_config_error(config));
}
pki = l->pki[0];
@@ -181,7 +181,8 @@ smtp_setup_listener_tls(struct listener *l)
fatal("no pki defined");
if (tls_config_set_dheparams(config, dheparams[pki->pki_dhe]) == -1)
- fatal("tls_config_set_dheparams");
+ fatalx("tls_config_set_dheparams: %s",
+ tls_config_error(config));
tls
Re: smtpd: nits to reduce the diff with -portable
On 2023/05/15 07:34:03 -0600, "Todd C. Miller" wrote: > On Mon, 15 May 2023 13:54:35 +0200, Omar Polo wrote: > > > almost always (cast)var. I've adjusted the spacing in the line I was > > touching, grepping for common types I could only find one instance of > > a '(long long) src' in envelope.c which I'm not addressing here. > > OK millert@. Sorry, as soon as I got an ok offlist from tb@ I went ahead since it was just cosmetic. I noticed I'm too in a hurry recently when committing diffs, so i'll make sure to wait more in the future. > It would be nice to get these changes in portable > as well to avoid gratuitous differences. I'll take care of sync'ing with -portable. Thanks,
Re: smtpd: nits to reduce the diff with -portable
May 15, 2023 3:34 PM, "Todd C. Miller" wrote: > On Mon, 15 May 2023 13:54:35 +0200, Omar Polo wrote: > >> almost always (cast)var. I've adjusted the spacing in the line I was >> touching, grepping for common types I could only find one instance of >> a '(long long) src' in envelope.c which I'm not addressing here. > > OK millert@. It would be nice to get these changes in portable > as well to avoid gratuitous differences. > FYI, I've granted Omar commit access to the portable repo so he can make syncs on his own and limit differences with OpenBSD.
Re: smtpd: nits to reduce the diff with -portable
On Mon, 15 May 2023 13:54:35 +0200, Omar Polo wrote: > almost always (cast)var. I've adjusted the spacing in the line I was > touching, grepping for common types I could only find one instance of > a '(long long) src' in envelope.c which I'm not addressing here. OK millert@. It would be nice to get these changes in portable as well to avoid gratuitous differences. - todd
Re: smtpd: nits to reduce the diff with -portable
On 2023/05/15 09:40:50 +0200, theo Buehler wrote:
> Do we care that sometimes we (cast)var and sometimes we (cast) var?
> Is this at least consistent per-file?
almost always (cast)var. I've adjusted the spacing in the line I was
touching, grepping for common types I could only find one instance of
a '(long long) src' in envelope.c which I'm not addressing here.
Index: libexec/mail.local/mail.local.c
===
RCS file: /cvs/src/libexec/mail.local/mail.local.c,v
retrieving revision 1.40
diff -u -p -r1.40 mail.local.c
--- libexec/mail.local/mail.local.c 10 May 2023 08:03:49 - 1.40
+++ libexec/mail.local/mail.local.c 15 May 2023 06:59:09 -
@@ -244,7 +244,7 @@ retry:
curoff = lseek(mbfd, 0, SEEK_END);
(void)snprintf(biffmsg, sizeof biffmsg, "%s@%lld\n", name,
- (long long int)curoff);
+ (long long)curoff);
if (lseek(fd, 0, SEEK_SET) == (off_t)-1) {
mwarn("temporary file: %s", strerror(errno));
goto bad;
Index: usr.sbin/smtpd/bounce.c
===
RCS file: /cvs/src/usr.sbin/smtpd/bounce.c,v
retrieving revision 1.88
diff -u -p -r1.88 bounce.c
--- usr.sbin/smtpd/bounce.c 4 May 2023 12:43:44 -0000 1.88
+++ usr.sbin/smtpd/bounce.c 15 May 2023 06:59:29 -
@@ -305,7 +305,7 @@ bounce_send(struct bounce_session *s, co
}
static const char *
-bounce_duration(long long int d)
+bounce_duration(long long d)
{
static char buf[32];
Index: usr.sbin/smtpd/lka_filter.c
===
RCS file: /cvs/src/usr.sbin/smtpd/lka_filter.c,v
retrieving revision 1.69
diff -u -p -r1.69 lka_filter.c
--- usr.sbin/smtpd/lka_filter.c 10 May 2023 07:20:20 -0000 1.69
+++ usr.sbin/smtpd/lka_filter.c 15 May 2023 06:59:29 -
@@ -933,13 +933,13 @@ filter_protocol_query(struct filter *fil
n = io_printf(lka_proc_get_io(filter->proc),
"filter|%s|%lld.%06ld|smtp-in|%s|%016"PRIx64"|%016"PRIx64"|%s|%s\n",
PROTOCOL_VERSION,
- (long long int)tv.tv_sec, tv.tv_usec,
+ (long long)tv.tv_sec, tv.tv_usec,
phase, reqid, token, fs->rdns, param);
else
n = io_printf(lka_proc_get_io(filter->proc),
"filter|%s|%lld.%06ld|smtp-in|%s|%016"PRIx64"|%016"PRIx64"|%s\n",
PROTOCOL_VERSION,
- (long long int)tv.tv_sec, tv.tv_usec,
+ (long long)tv.tv_sec, tv.tv_usec,
phase, reqid, token, param);
if (n == -1)
fatalx("failed to write to processor");
@@ -957,7 +957,7 @@ filter_data_query(struct filter *filter,
"filter|%s|%lld.%06ld|smtp-in|data-line|"
"%016"PRIx64"|%016"PRIx64"|%s\n",
PROTOCOL_VERSION,
- (long long int)tv.tv_sec, tv.tv_usec,
+ (long long)tv.tv_sec, tv.tv_usec,
reqid, token, line);
if (n == -1)
fatalx("failed to write to processor");
@@ -1374,7 +1374,7 @@ report_smtp_broadcast(uint64_t reqid, co
va_start(ap, format);
if (io_printf(lka_proc_get_io(rp->name),
"report|%s|%lld.%06ld|%s|%s|%016"PRIx64"%s",
- PROTOCOL_VERSION, (long long int)tv->tv_sec, tv->tv_usec,
+ PROTOCOL_VERSION, (long long)tv->tv_sec, tv->tv_usec,
direction, event, reqid,
format[0] != '\n' ? "|" : "") == -1 ||
io_vprintf(lka_proc_get_io(rp->name), format, ap) == -1)
Index: usr.sbin/smtpd/mail.maildir.c
===
RCS file: /cvs/src/usr.sbin/smtpd/mail.maildir.c,v
retrieving revision 1.16
diff -u -p -r1.16 mail.maildir.c
--- usr.sbin/smtpd/mail.maildir.c 10 May 2023 07:19:49 - 1.16
+++ usr.sbin/smtpd/mail.maildir.c 15 May 2023 11:46:03 -
@@ -171,7 +171,7 @@ maildir_engine(const char *dirname, int
(void)strlcpy(hostname, "localhost", sizeof hostname);
(void)snprintf(filename, sizeof filename, "%lld.%08x.%s",
- (long long int) time(NULL),
+ (long long)time(NULL),
arc4random(),
hostname);
Index: usr.sbin/smtpd/mta_session.c
=======
RCS file: /cvs/src/usr.sbin/smtpd/mta_session.c,v
retrieving revision 1.147
diff -u -p -r1.147 mta_session.c
--- usr.sbin/smtpd/mta_session.c26 Sep 2022 08:48:52 - 1.147
+++ usr.sbin/smtpd/mta_session.c15 May 2023 06:59:
Re: smtpd: nits to reduce the diff with -portable
On Mon, May 15, 2023 at 09:22:47AM +0200, Omar Polo wrote: > On 2023/05/15 09:03:47 +0200, Omar Polo wrote: > > On 2023/05/14 18:03:17 -0600, "Theo de Raadt" wrote: > > > Omar Polo wrote: > > > > On 2023/05/10 09:30:12 +0200, Theo Buehler wrote: > > > > > > I forgot to include one off_t cast since it was in a different > > > > > > directory and -even if off topic because it's not in portable- > > > > > > instead > > > > > > of "const"-ify only tz why don't mark as const also the two arrays > > > > > > day > > > > > > and month? > > > > > > > > > > ok. > > > > > > > > > > The previous diff used (long long int) and this one now uses (long > > > > > long). > > > > > Would be nice to be consistent. > > > > > > > > Yes, indeed. smtpd uses `long long int', while for mail.local doesn't > > > > have any. I'll go with `long long int' for consistency, typed `long > > > > long' out of muscular memory. > > > > > > I think it is wrong for smtpd to use "long long int". It is pointless > > > silliness, and there is more value in being idiomatically identical with > > > the greater body of code. > > > > ack (fwiw I prefer long long too). Here's s/long long int/long long/g, > > ok? > > let's fix the indentation in smtpctl.c since I have to touch that line > anyway... I am ok with this. Do we care that sometimes we (cast)var and sometimes we (cast) var? Is this at least consistent per-file?
Re: smtpd: nits to reduce the diff with -portable
On 2023/05/15 09:03:47 +0200, Omar Polo wrote:
> On 2023/05/14 18:03:17 -0600, "Theo de Raadt" wrote:
> > Omar Polo wrote:
> > > On 2023/05/10 09:30:12 +0200, Theo Buehler wrote:
> > > > > I forgot to include one off_t cast since it was in a different
> > > > > directory and -even if off topic because it's not in portable- instead
> > > > > of "const"-ify only tz why don't mark as const also the two arrays day
> > > > > and month?
> > > >
> > > > ok.
> > > >
> > > > The previous diff used (long long int) and this one now uses (long
> > > > long).
> > > > Would be nice to be consistent.
> > >
> > > Yes, indeed. smtpd uses `long long int', while for mail.local doesn't
> > > have any. I'll go with `long long int' for consistency, typed `long
> > > long' out of muscular memory.
> >
> > I think it is wrong for smtpd to use "long long int". It is pointless
> > silliness, and there is more value in being idiomatically identical with
> > the greater body of code.
>
> ack (fwiw I prefer long long too). Here's s/long long int/long long/g,
> ok?
let's fix the indentation in smtpctl.c since I have to touch that line
anyway...
Index: libexec/mail.local/mail.local.c
===
RCS file: /cvs/src/libexec/mail.local/mail.local.c,v
retrieving revision 1.40
diff -u -p -r1.40 mail.local.c
--- libexec/mail.local/mail.local.c 10 May 2023 08:03:49 - 1.40
+++ libexec/mail.local/mail.local.c 15 May 2023 06:59:09 -
@@ -244,7 +244,7 @@ retry:
curoff = lseek(mbfd, 0, SEEK_END);
(void)snprintf(biffmsg, sizeof biffmsg, "%s@%lld\n", name,
- (long long int)curoff);
+ (long long)curoff);
if (lseek(fd, 0, SEEK_SET) == (off_t)-1) {
mwarn("temporary file: %s", strerror(errno));
goto bad;
Index: usr.sbin/smtpd/bounce.c
===
RCS file: /cvs/src/usr.sbin/smtpd/bounce.c,v
retrieving revision 1.88
diff -u -p -r1.88 bounce.c
--- usr.sbin/smtpd/bounce.c 4 May 2023 12:43:44 - 1.88
+++ usr.sbin/smtpd/bounce.c 15 May 2023 06:59:29 -
@@ -305,7 +305,7 @@ bounce_send(struct bounce_session *s, co
}
static const char *
-bounce_duration(long long int d)
+bounce_duration(long long d)
{
static char buf[32];
Index: usr.sbin/smtpd/lka_filter.c
===
RCS file: /cvs/src/usr.sbin/smtpd/lka_filter.c,v
retrieving revision 1.69
diff -u -p -r1.69 lka_filter.c
--- usr.sbin/smtpd/lka_filter.c 10 May 2023 07:20:20 - 1.69
+++ usr.sbin/smtpd/lka_filter.c 15 May 2023 06:59:29 -
@@ -933,13 +933,13 @@ filter_protocol_query(struct filter *fil
n = io_printf(lka_proc_get_io(filter->proc),
"filter|%s|%lld.%06ld|smtp-in|%s|%016"PRIx64"|%016"PRIx64"|%s|%s\n",
PROTOCOL_VERSION,
- (long long int)tv.tv_sec, tv.tv_usec,
+ (long long)tv.tv_sec, tv.tv_usec,
phase, reqid, token, fs->rdns, param);
else
n = io_printf(lka_proc_get_io(filter->proc),
"filter|%s|%lld.%06ld|smtp-in|%s|%016"PRIx64"|%016"PRIx64"|%s\n",
PROTOCOL_VERSION,
- (long long int)tv.tv_sec, tv.tv_usec,
+ (long long)tv.tv_sec, tv.tv_usec,
phase, reqid, token, param);
if (n == -1)
fatalx("failed to write to processor");
@@ -957,7 +957,7 @@ filter_data_query(struct filter *filter,
"filter|%s|%lld.%06ld|smtp-in|data-line|"
"%016"PRIx64"|%016"PRIx64"|%s\n",
PROTOCOL_VERSION,
- (long long int)tv.tv_sec, tv.tv_usec,
+ (long long)tv.tv_sec, tv.tv_usec,
reqid, token, line);
if (n == -1)
fatalx("failed to write to processor");
@@ -1374,7 +1374,7 @@ report_smtp_broadcast(uint64_t reqid, co
va_start(ap, format);
if (io_printf(lka_proc_get_io(rp->name),
"report|%s|%lld.%06ld|%s|%s|%016"PRIx64"%s",
- PROTOCOL_VERSION, (long long int)tv->tv_sec, tv->tv_usec,
+ PROTOCOL_VERSION, (long long)tv->tv_sec, tv->tv_usec,
direction, event, reqid,
format[0] != '\n' ? "|" : "") == -1 ||
io_vprintf(lka_proc_get_io(rp->name), format, ap) == -1)
Index: usr.sbin/smtpd/
Re: smtpd: nits to reduce the diff with -portable
On 2023/05/14 18:03:17 -0600, "Theo de Raadt" wrote:
> Omar Polo wrote:
> > On 2023/05/10 09:30:12 +0200, Theo Buehler wrote:
> > > > I forgot to include one off_t cast since it was in a different
> > > > directory and -even if off topic because it's not in portable- instead
> > > > of "const"-ify only tz why don't mark as const also the two arrays day
> > > > and month?
> > >
> > > ok.
> > >
> > > The previous diff used (long long int) and this one now uses (long long).
> > > Would be nice to be consistent.
> >
> > Yes, indeed. smtpd uses `long long int', while for mail.local doesn't
> > have any. I'll go with `long long int' for consistency, typed `long
> > long' out of muscular memory.
>
> I think it is wrong for smtpd to use "long long int". It is pointless
> silliness, and there is more value in being idiomatically identical with
> the greater body of code.
ack (fwiw I prefer long long too). Here's s/long long int/long long/g,
ok?
Index: libexec/mail.local/mail.local.c
===
RCS file: /cvs/src/libexec/mail.local/mail.local.c,v
retrieving revision 1.40
diff -u -p -r1.40 mail.local.c
--- libexec/mail.local/mail.local.c 10 May 2023 08:03:49 - 1.40
+++ libexec/mail.local/mail.local.c 15 May 2023 06:59:09 -
@@ -244,7 +244,7 @@ retry:
curoff = lseek(mbfd, 0, SEEK_END);
(void)snprintf(biffmsg, sizeof biffmsg, "%s@%lld\n", name,
- (long long int)curoff);
+ (long long)curoff);
if (lseek(fd, 0, SEEK_SET) == (off_t)-1) {
mwarn("temporary file: %s", strerror(errno));
goto bad;
Index: usr.sbin/smtpd/bounce.c
=======
RCS file: /cvs/src/usr.sbin/smtpd/bounce.c,v
retrieving revision 1.88
diff -u -p -r1.88 bounce.c
--- usr.sbin/smtpd/bounce.c 4 May 2023 12:43:44 - 1.88
+++ usr.sbin/smtpd/bounce.c 15 May 2023 06:59:29 -
@@ -305,7 +305,7 @@ bounce_send(struct bounce_session *s, co
}
static const char *
-bounce_duration(long long int d)
+bounce_duration(long long d)
{
static char buf[32];
Index: usr.sbin/smtpd/lka_filter.c
===
RCS file: /cvs/src/usr.sbin/smtpd/lka_filter.c,v
retrieving revision 1.69
diff -u -p -r1.69 lka_filter.c
--- usr.sbin/smtpd/lka_filter.c 10 May 2023 07:20:20 - 1.69
+++ usr.sbin/smtpd/lka_filter.c 15 May 2023 06:59:29 -
@@ -933,13 +933,13 @@ filter_protocol_query(struct filter *fil
n = io_printf(lka_proc_get_io(filter->proc),
"filter|%s|%lld.%06ld|smtp-in|%s|%016"PRIx64"|%016"PRIx64"|%s|%s\n",
PROTOCOL_VERSION,
- (long long int)tv.tv_sec, tv.tv_usec,
+ (long long)tv.tv_sec, tv.tv_usec,
phase, reqid, token, fs->rdns, param);
else
n = io_printf(lka_proc_get_io(filter->proc),
"filter|%s|%lld.%06ld|smtp-in|%s|%016"PRIx64"|%016"PRIx64"|%s\n",
PROTOCOL_VERSION,
- (long long int)tv.tv_sec, tv.tv_usec,
+ (long long)tv.tv_sec, tv.tv_usec,
phase, reqid, token, param);
if (n == -1)
fatalx("failed to write to processor");
@@ -957,7 +957,7 @@ filter_data_query(struct filter *filter,
"filter|%s|%lld.%06ld|smtp-in|data-line|"
"%016"PRIx64"|%016"PRIx64"|%s\n",
PROTOCOL_VERSION,
- (long long int)tv.tv_sec, tv.tv_usec,
+ (long long)tv.tv_sec, tv.tv_usec,
reqid, token, line);
if (n == -1)
fatalx("failed to write to processor");
@@ -1374,7 +1374,7 @@ report_smtp_broadcast(uint64_t reqid, co
va_start(ap, format);
if (io_printf(lka_proc_get_io(rp->name),
"report|%s|%lld.%06ld|%s|%s|%016"PRIx64"%s",
- PROTOCOL_VERSION, (long long int)tv->tv_sec, tv->tv_usec,
+ PROTOCOL_VERSION, (long long)tv->tv_sec, tv->tv_usec,
direction, event, reqid,
format[0] != '\n' ? "|" : "") == -1 ||
io_vprintf(lka_proc_get_io(rp->name), format, ap) == -1)
Index: usr.sbin/smtpd/mail.maildir.c
===
RCS file: /cvs/src/usr.sbin/smtpd/mail.maildir.c,v
retrieving revision 1.16
diff -u -p -r1.16 mail.maildir.c
--- usr.sbin/smtpd/mail.maildir.c 10 May 2023 07:19:49 -
Re: smtpd: nits to reduce the diff with -portable
Omar Polo wrote: > On 2023/05/10 09:30:12 +0200, Theo Buehler wrote: > > > I forgot to include one off_t cast since it was in a different > > > directory and -even if off topic because it's not in portable- instead > > > of "const"-ify only tz why don't mark as const also the two arrays day > > > and month? > > > > ok. > > > > The previous diff used (long long int) and this one now uses (long long). > > Would be nice to be consistent. > > Yes, indeed. smtpd uses `long long int', while for mail.local doesn't > have any. I'll go with `long long int' for consistency, typed `long > long' out of muscular memory. I think it is wrong for smtpd to use "long long int". It is pointless silliness, and there is more value in being idiomatically identical with the greater body of code.
Re: smtpd: nits to reduce the diff with -portable
> + (long long int)tv.tv_sec, tv.tv_usec, Please do not use that form. (long long) is enough.
Re: smtpd: nits to reduce the diff with -portable
On Wed, 10 May 2023 09:25:43 +0200, Omar Polo wrote: > I forgot to include one off_t cast since it was in a different > directory and -even if off topic because it's not in portable- instead > of "const"-ify only tz why don't mark as const also the two arrays day > and month? Sure. OK millert@ for this one too. - todd
Re: smtpd: nits to reduce the diff with -portable
On 2023/05/10 09:30:12 +0200, Theo Buehler wrote: > > I forgot to include one off_t cast since it was in a different > > directory and -even if off topic because it's not in portable- instead > > of "const"-ify only tz why don't mark as const also the two arrays day > > and month? > > ok. > > The previous diff used (long long int) and this one now uses (long long). > Would be nice to be consistent. Yes, indeed. smtpd uses `long long int', while for mail.local doesn't have any. I'll go with `long long int' for consistency, typed `long long' out of muscular memory. thanks!
Re: smtpd: nits to reduce the diff with -portable
On Wed, May 10, 2023 at 09:25:43AM +0200, Omar Polo wrote: > On 2023/05/09 19:41:51 -0600, "Todd C. Miller" wrote: > > On Wed, 10 May 2023 00:55:54 +0200, Omar Polo wrote: > > > > > As per subject, here's a few misc nits that would reduce the > > > difference with -portable. There's some printing of time_t via > > > casting to long long, some missing includes (even if in tree it builds > > > nevertheless) and a const for a variable (no idea how it went there in > > > -portable but it's not wrong so including that too.) > > > > OK millert@ > > thanks! > > I forgot to include one off_t cast since it was in a different > directory and -even if off topic because it's not in portable- instead > of "const"-ify only tz why don't mark as const also the two arrays day > and month? ok. The previous diff used (long long int) and this one now uses (long long). Would be nice to be consistent.
Re: smtpd: nits to reduce the diff with -portable
On 2023/05/09 19:41:51 -0600, "Todd C. Miller" wrote:
> On Wed, 10 May 2023 00:55:54 +0200, Omar Polo wrote:
>
> > As per subject, here's a few misc nits that would reduce the
> > difference with -portable. There's some printing of time_t via
> > casting to long long, some missing includes (even if in tree it builds
> > nevertheless) and a const for a variable (no idea how it went there in
> > -portable but it's not wrong so including that too.)
>
> OK millert@
thanks!
I forgot to include one off_t cast since it was in a different
directory and -even if off topic because it's not in portable- instead
of "const"-ify only tz why don't mark as const also the two arrays day
and month?
sorry for forgetting these in the previous mail,
diff /usr/src
commit - a2d3cb1e480c37eb6fb14cee9f2946606a0346bc
path + /usr/src
blob - a0d38b1a7c0c7f7016b7d3f69e1c5dad77227e2a
file + libexec/mail.local/mail.local.c
--- libexec/mail.local/mail.local.c
+++ libexec/mail.local/mail.local.c
@@ -243,7 +243,8 @@ retry:
}
curoff = lseek(mbfd, 0, SEEK_END);
- (void)snprintf(biffmsg, sizeof biffmsg, "%s@%lld\n", name, curoff);
+ (void)snprintf(biffmsg, sizeof biffmsg, "%s@%lld\n", name,
+ (long long)curoff);
if (lseek(fd, 0, SEEK_SET) == (off_t)-1) {
mwarn("temporary file: %s", strerror(errno));
goto bad;
blob - 6e340ccde1a521ff13805c6f31d38ee77eb5ad50
file + usr.sbin/smtpd/to.c
--- usr.sbin/smtpd/to.c
+++ usr.sbin/smtpd/to.c
@@ -140,10 +140,10 @@ time_to_text(time_t when)
{
struct tm *lt;
static char buf[40];
- char *day[] = {"Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat"};
- char *month[] = {"Jan","Feb","Mar","Apr","May","Jun",
+ const char *day[] = {"Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat"};
+ const char *month[] = {"Jan","Feb","Mar","Apr","May","Jun",
"Jul","Aug","Sep","Oct","Nov","Dec"};
- char *tz;
+ const char *tz;
long offset;
lt = localtime();
Re: smtpd: nits to reduce the diff with -portable
On Wed, 10 May 2023 00:55:54 +0200, Omar Polo wrote: > As per subject, here's a few misc nits that would reduce the > difference with -portable. There's some printing of time_t via > casting to long long, some missing includes (even if in tree it builds > nevertheless) and a const for a variable (no idea how it went there in > -portable but it's not wrong so including that too.) OK millert@ - todd
smtpd: nits to reduce the diff with -portable
As per subject, here's a few misc nits that would reduce the
difference with -portable. There's some printing of time_t via
casting to long long, some missing includes (even if in tree it builds
nevertheless) and a const for a variable (no idea how it went there in
-portable but it's not wrong so including that too.)
ok?
diff /usr/src
commit - a2d3cb1e480c37eb6fb14cee9f2946606a0346bc
path + /usr/src
blob - 52924139091915e80409892fbd92dad375ee602c
file + usr.sbin/smtpd/lka_filter.c
--- usr.sbin/smtpd/lka_filter.c
+++ usr.sbin/smtpd/lka_filter.c
@@ -933,13 +933,13 @@ filter_protocol_query(struct filter *filter, uint64_t
n = io_printf(lka_proc_get_io(filter->proc),
"filter|%s|%lld.%06ld|smtp-in|%s|%016"PRIx64"|%016"PRIx64"|%s|%s\n",
PROTOCOL_VERSION,
- tv.tv_sec, tv.tv_usec,
+ (long long int)tv.tv_sec, tv.tv_usec,
phase, reqid, token, fs->rdns, param);
else
n = io_printf(lka_proc_get_io(filter->proc),
"filter|%s|%lld.%06ld|smtp-in|%s|%016"PRIx64"|%016"PRIx64"|%s\n",
PROTOCOL_VERSION,
- tv.tv_sec, tv.tv_usec,
+ (long long int)tv.tv_sec, tv.tv_usec,
phase, reqid, token, param);
if (n == -1)
fatalx("failed to write to processor");
@@ -957,7 +957,7 @@ filter_data_query(struct filter *filter, uint64_t toke
"filter|%s|%lld.%06ld|smtp-in|data-line|"
"%016"PRIx64"|%016"PRIx64"|%s\n",
PROTOCOL_VERSION,
- tv.tv_sec, tv.tv_usec,
+ (long long int)tv.tv_sec, tv.tv_usec,
reqid, token, line);
if (n == -1)
fatalx("failed to write to processor");
@@ -1374,8 +1374,9 @@ report_smtp_broadcast(uint64_t reqid, const char *dire
va_start(ap, format);
if (io_printf(lka_proc_get_io(rp->name),
"report|%s|%lld.%06ld|%s|%s|%016"PRIx64"%s",
- PROTOCOL_VERSION, tv->tv_sec, tv->tv_usec, direction,
- event, reqid, format[0] != '\n' ? "|" : "") == -1 ||
+ PROTOCOL_VERSION, (long long int)tv->tv_sec, tv->tv_usec,
+ direction, event, reqid,
+ format[0] != '\n' ? "|" : "") == -1 ||
io_vprintf(lka_proc_get_io(rp->name), format, ap) == -1)
fatalx("failed to write to processor");
va_end(ap);
blob - c204caaf222db2e13204b09d59c15d7451d69763
file + usr.sbin/smtpd/mail.maildir.c
--- usr.sbin/smtpd/mail.maildir.c
+++ usr.sbin/smtpd/mail.maildir.c
@@ -29,6 +29,7 @@
#include
#include
#include
+#include
#include
#defineMAILADDR_ESCAPE "!#$%&'*/?^`{|}~"
blob - edfd988605240338ded5f270ce291739401fa3fb
file + usr.sbin/smtpd/mda.c
--- usr.sbin/smtpd/mda.c
+++ usr.sbin/smtpd/mda.c
@@ -507,7 +507,7 @@ mda_getlastline(int fd, char *dst, size_t dstsz)
size_t sz = 0;
ssize_t len;
int out = 0;
-
+
if (lseek(fd, 0, SEEK_SET) == -1) {
log_warn("warn: mda: lseek");
close(fd);
blob - 4915bf6002c3e68dcf0a090818b521fe45de0a28
file + usr.sbin/smtpd/parse.y
--- usr.sbin/smtpd/parse.y
+++ usr.sbin/smtpd/parse.y
@@ -34,6 +34,8 @@
#include
#include
#include
+#include
+#include
#include
#include
#include
blob - 6e340ccde1a521ff13805c6f31d38ee77eb5ad50
file + usr.sbin/smtpd/to.c
--- usr.sbin/smtpd/to.c
+++ usr.sbin/smtpd/to.c
@@ -143,7 +143,7 @@ time_to_text(time_t when)
char *day[] = {"Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat"};
char *month[] = {"Jan","Feb","Mar","Apr","May","Jun",
"Jul","Aug","Sep","Oct","Nov","Dec"};
- char *tz;
+ const char *tz;
long offset;
lt = localtime();
Re: smtpd: simplify token name extraction for %{name}
On 2023/03/19 08:11:27 -0600, Todd C. Miller wrote:
> The current code for extracting the token name from %{name} can be
> simplified by computing the token name length. The existing code
> copies "name}" to token[] using memcpy(), then strchr() to find the
> '}' and replace it with a NUL. Using strchr() here is fragile since
> token[] is not yet NUL-terminated. This is currently not a problem
> since there is an earlier check for '}' in the source string but
> it could be dangerous is the code changes further.
>
> I find it much simpler to compute the token name length, verify the
> length, copy the bytes and then explicitly NUL-terminate token.
> This results in less code and is more easily audited.
Agreed, I find it simpler too, and less fragile.
> I've also removed the duplicate check for *(pbuf+1) != '{'.
>
> OK?
(while I still have the details fresh in my mind) ok for me
smtpd: simplify token name extraction for %{name}
The current code for extracting the token name from %{name} can be
simplified by computing the token name length. The existing code
copies "name}" to token[] using memcpy(), then strchr() to find the
'}' and replace it with a NUL. Using strchr() here is fragile since
token[] is not yet NUL-terminated. This is currently not a problem
since there is an earlier check for '}' in the source string but
it could be dangerous is the code changes further.
I find it much simpler to compute the token name length, verify the
length, copy the bytes and then explicitly NUL-terminate token.
This results in less code and is more easily audited.
I've also removed the duplicate check for *(pbuf+1) != '{'.
OK?
- todd
Index: usr.sbin/smtpd/mda_variables.c
===
RCS file: /cvs/src/usr.sbin/smtpd/mda_variables.c,v
retrieving revision 1.8
diff -u -p -U10 -u -r1.8 mda_variables.c
--- usr.sbin/smtpd/mda_variables.c 19 Mar 2023 01:43:11 - 1.8
+++ usr.sbin/smtpd/mda_variables.c 19 Mar 2023 13:59:01 -
@@ -236,21 +236,21 @@ mda_expand_token(char *dest, size_t len,
ssize_t
mda_expand_format(char *buf, size_t len, const struct deliver *dlv,
const struct userinfo *ui, const char *mda_command)
{
chartmpbuf[EXPAND_BUFFER], *ptmp, *pbuf, *ebuf;
charexptok[EXPAND_BUFFER];
ssize_t exptoklen;
chartoken[MAXTOKENLEN];
- size_t ret, tmpret;
+ size_t ret, tmpret, toklen;
if (len < sizeof tmpbuf) {
log_warnx("mda_expand_format: tmp buffer < rule buffer");
return -1;
}
memset(tmpbuf, 0, sizeof tmpbuf);
pbuf = buf;
ptmp = tmpbuf;
ret = tmpret = 0;
@@ -261,48 +261,46 @@ mda_expand_format(char *buf, size_t len,
if (tmpret >= sizeof tmpbuf) {
log_warnx("warn: user directory for %s too large",
ui->directory);
return 0;
}
ret += tmpret;
ptmp += tmpret;
pbuf += 2;
}
-
/* expansion loop */
for (; *pbuf && ret < sizeof tmpbuf; ret += tmpret) {
if (*pbuf == '%' && *(pbuf + 1) == '%') {
*ptmp++ = *pbuf++;
pbuf += 1;
tmpret = 1;
continue;
}
if (*pbuf != '%' || *(pbuf + 1) != '{') {
*ptmp++ = *pbuf++;
tmpret = 1;
continue;
}
/* %{...} otherwise fail */
- if (*(pbuf+1) != '{' || (ebuf = strchr(pbuf+1, '}')) == NULL)
+ if ((ebuf = strchr(pbuf+2, '}')) == NULL)
return 0;
/* extract token from %{token} */
- if ((size_t)(ebuf - pbuf) - 1 >= sizeof token)
+ toklen = ebuf - (pbuf+2);
+ if (toklen >= sizeof token)
return 0;
- memcpy(token, pbuf+2, ebuf-pbuf-1);
- if (strchr(token, '}') == NULL)
- return 0;
- *strchr(token, '}') = '\0';
+ memcpy(token, pbuf+2, toklen);
+ token[toklen] = '\0';
exptoklen = mda_expand_token(exptok, sizeof exptok, token, dlv,
ui, mda_command);
if (exptoklen == -1)
return -1;
/* writing expanded token at ptmp will overflow tmpbuf */
if (sizeof (tmpbuf) - (ptmp - tmpbuf) <= (size_t)exptoklen)
return -1;
[patch] usr.sbin/smtpd filter localhost relays
Hi
On github someone reported an issue[0] regarding localhost MX entries.
Currently smtpd will just use the localhost relay. This leads to a
loop. Here a patch filtering localhost and localhost addresses for MX
requests.
As next step you could implement Null-MX (rfc 7505).
Philipp
[0] https://github.com/OpenSMTPD/OpenSMTPD/issues/1190
diff --git a/usr.sbin/smtpd/dns.c b/usr.sbin/smtpd/dns.c
index f7c6b3df..7389efec 100644
--- a/usr.sbin/smtpd/dns.c
+++ b/usr.sbin/smtpd/dns.c
@@ -53,6 +53,7 @@ struct dns_lookup {
struct dns_session *session;
char*host;
int preference;
+ int filter_localhost;
};
struct dns_session {
@@ -65,7 +66,7 @@ struct dns_session {
int refcount;
};
-static void dns_lookup_host(struct dns_session *, const char *, int);
+static void dns_lookup_host(struct dns_session *, const char *, int, int);
static void dns_dispatch_host(struct asr_result *, void *);
static void dns_dispatch_mx(struct asr_result *, void *);
static void dns_dispatch_mx_preference(struct asr_result *, void *);
@@ -139,7 +140,7 @@ dns_imsg(struct mproc *p, struct imsg *imsg)
case IMSG_MTA_DNS_HOST:
m_get_string(, );
m_end();
- dns_lookup_host(s, host, -1);
+ dns_lookup_host(s, host, -1, 0);
return;
case IMSG_MTA_DNS_MX:
@@ -205,6 +206,28 @@ dns_imsg(struct mproc *p, struct imsg *imsg)
}
}
+static int
+is_localhost(struct sockaddr *sa)
+{
+ struct sockaddr_in6 *ipv6;
+ struct sockaddr_in *ipv4;
+ uint32_t addr;
+
+ switch (sa->sa_family) {
+ case AF_INET6:
+ // May check also for v6 mapped v4 addresses
+ ipv6 = (struct sockaddr_in6 *)sa;
+ return IN6_IS_ADDR_LOOPBACK(>sin6_addr);
+ case AF_INET:
+ ipv4 = (struct sockaddr_in *)sa;
+ addr = ntohl(ipv4->sin_addr.s_addr);
+ return ((addr >> 24) & 0xff) == 127;
+ default:
+ log_warnx("warn: unknown family in sockaddr");
+ }
+ return 0;
+}
+
static void
dns_dispatch_host(struct asr_result *ar, void *arg)
{
@@ -215,6 +238,10 @@ dns_dispatch_host(struct asr_result *ar, void *arg)
s = lookup->session;
for (ai = ar->ar_addrinfo; ai; ai = ai->ai_next) {
+ if (lookup->filter_localhost && is_localhost(ai->ai_addr)) {
+ log_warnx("warn: ignore localhost address for host %s",
lookup->host);
+ continue;
+ }
s->mxfound++;
m_create(s->p, IMSG_MTA_DNS_HOST, 0, 0, -1);
m_add_id(s->p, s->reqid);
@@ -280,14 +307,18 @@ dns_dispatch_mx(struct asr_result *ar, void *arg)
continue;
print_dname(rr.rr.mx.exchange, buf, sizeof(buf));
buf[strlen(buf) - 1] = '\0';
- dns_lookup_host(s, buf, rr.rr.mx.preference);
+ if (strcasecmp("localhost", buf)==0) {
+ log_warnx("ignore localhost MX-entry for domain <%s>",
lookup->host);
+ continue;
+ }
+ dns_lookup_host(s, buf, rr.rr.mx.preference, 1);
found++;
}
free(ar->ar_data);
/* fallback to host if no MX is found. */
if (found == 0)
- dns_lookup_host(s, s->name, 0);
+ dns_lookup_host(s, s->name, 0, 1);
}
static void
@@ -340,7 +371,7 @@ dns_dispatch_mx_preference(struct asr_result *ar, void *arg)
}
static void
-dns_lookup_host(struct dns_session *s, const char *host, int preference)
+dns_lookup_host(struct dns_session *s, const char *host, int preference, int
filter_localhost)
{
struct dns_lookup *lookup;
struct addrinfo hints;
@@ -350,6 +381,7 @@ dns_lookup_host(struct dns_session *s, const char *host,
int preference)
lookup = xcalloc(1, sizeof *lookup);
lookup->preference = preference;
+ lookup->filter_localhost = filter_localhost;
lookup->host = xstrdup(host);
lookup->session = s;
s->refcount++;
add table-procexec to smtpd
Hi,
This is another try to add table-procexec to smtpd. This allows for table
backends to communicate with smtpd with a very simple line protocol, similar to
filter proc-exec.
The code is simple enough and after a bit of time can be used as a replace for
table-proc (which uses imsg). Currently it is not replacing anything and is
just available as an extra. I have a WIP perl-ldap table which can talk this
line protocol and its on github right now (quite old) -
https://github.com/bsd-ac/table-ldap_perl
OK to import?
Cheers,
Aisha
diff --git a/usr.sbin/smtpd/smtpctl/Makefile b/usr.sbin/smtpd/smtpctl/Makefile
index ef8148be8c9..2e8beff1ad1 100644
--- a/usr.sbin/smtpd/smtpctl/Makefile
+++ b/usr.sbin/smtpd/smtpctl/Makefile
@@ -48,6 +48,7 @@ SRCS+=table_static.c
SRCS+= table_db.c
SRCS+= table_getpwnam.c
SRCS+= table_proc.c
+SRCS+= table_procexec.c
SRCS+= unpack_dns.c
SRCS+= spfwalk.c
diff --git a/usr.sbin/smtpd/smtpd.h b/usr.sbin/smtpd/smtpd.h
index 125a6a5dfbe..ca54d54ea66 100644
--- a/usr.sbin/smtpd/smtpd.h
+++ b/usr.sbin/smtpd/smtpd.h
@@ -1662,6 +1662,7 @@ int table_regex_match(const char *, const char *);
void table_open_all(struct smtpd *);
void table_dump_all(struct smtpd *);
void table_close_all(struct smtpd *);
+const char *table_service_name(enum table_service );
/* to.c */
diff --git a/usr.sbin/smtpd/smtpd/Makefile b/usr.sbin/smtpd/smtpd/Makefile
index d914b43f705..3fcfcd1c19d 100644
--- a/usr.sbin/smtpd/smtpd/Makefile
+++ b/usr.sbin/smtpd/smtpd/Makefile
@@ -63,6 +63,7 @@ SRCS+=compress_gzip.c
SRCS+= table_db.c
SRCS+= table_getpwnam.c
SRCS+= table_proc.c
+SRCS+= table_procexec.c
SRCS+= table_static.c
SRCS+= queue_fs.c
diff --git a/usr.sbin/smtpd/table.c b/usr.sbin/smtpd/table.c
index 7328cf5df6e..4f9adfe4c57 100644
--- a/usr.sbin/smtpd/table.c
+++ b/usr.sbin/smtpd/table.c
@@ -36,8 +36,8 @@ extern struct table_backend table_backend_static;
extern struct table_backend table_backend_db;
extern struct table_backend table_backend_getpwnam;
extern struct table_backend table_backend_proc;
+extern struct table_backend table_backend_procexec;
-static const char * table_service_name(enum table_service);
static int table_parse_lookup(enum table_service, const char *, const char *,
union lookup *);
static int parse_sockaddr(struct sockaddr *, int, const char *);
@@ -49,6 +49,7 @@ static struct table_backend *backends[] = {
_backend_db,
_backend_getpwnam,
_backend_proc,
+ _backend_procexec,
NULL
};
@@ -67,7 +68,7 @@ table_backend_lookup(const char *backend)
return NULL;
}
-static const char *
+const char *
table_service_name(enum table_service s)
{
switch (s) {
diff --git a/usr.sbin/smtpd/table_procexec.c b/usr.sbin/smtpd/table_procexec.c
new file mode 100644
index 000..9375da5c0ad
--- /dev/null
+++ b/usr.sbin/smtpd/table_procexec.c
@@ -0,0 +1,326 @@
+/* $OpenBSD$ */
+
+/*
+ * Copyright (c) 2023 Aisha Tammy
+ * Copyright (c) 2020 Gilles Chehade
+ *
+ * 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
+#include
+#include
+#include
+#include
+
+#include "log.h"
+#include "smtpd.h"
+
+#define PROCEXEC_VERSION "1"
+#define PROCEXEC_TIMEOUT 500
+
+static int table_procexec_open(struct table *);
+static int table_procexec_update(struct table *);
+static void table_procexec_close(struct table *);
+static int table_procexec_lookup(struct table *, enum table_service,
+ const char *, char **);
+static int table_procexec_fetch(struct table *, enum table_service, char **);
+
+enum procexec_query;
+static int table_procexec_helper(struct table *, enum procexec_query,
+ enum table_service, const char *, char **);
+
+struct table_backend table_backend_procexec = {
+"proc-exec",
+K_ANY,
+NULL,
+NULL,
+NULL,
+table_procexec_open,
+table_procexec_update,
+table_procexec_close,
+table_procexec_lookup,
+table_procexec_f
OpenBSD Errata: February 7, 2023 (x509 xserver smtpd)
Errata patches for LibreSSL libcrypto, X11 server, and smtpd have been released for OpenBSD 7.1 and 7.2. Binary updates for the amd64, i386 and arm64 platform are available via the syspatch utility. Source code patches can be found on the respective errata page: https://www.openbsd.org/errata71.html https://www.openbsd.org/errata72.html
Re: smtpd bug in Received: header with one recipient
I took another look at this and it seems correct to me. We should not really be using tx->evp.rcpt after the it has been added to the tx->rcpts list. I plan to commit it unless there are objections. - todd
Re: smtpd bug in Received: header with one recipient
Ping.
On Sun, Oct 09, 2022 at 08:01:01AM +0200, Martijn van Duren wrote:
> Bit focused on other things atm and not familiar with this part of the
> code, but some comments.
>
> On Sat, 2022-10-08 at 12:18 -0600, Chris Waddey wrote:
>> A message with a single successful recipient but with a failed
>> rcpt to: command afterward generates an incorrect Received: header.
...
>> The following patch fixes the problem:
>> Index: smtp_session.c
>> ===
>> RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v
>> retrieving revision 1.432
>> diff -u -p -r1.432 smtp_session.c
>> --- smtp_session.c 1 Jul 2021 07:42:16 - 1.432
>> +++ smtp_session.c 8 Oct 2022 18:04:51 -
>> @@ -2732,6 +2732,7 @@ static void
>> smtp_message_begin(struct smtp_tx *tx)
>> {
>>struct smtp_session *s;
>> + struct smtp_rcpt *srfp;
>>int (*m_printf)(struct smtp_tx *, const char *, ...);
>>m_printf = smtp_message_printf;
>> @@ -2780,10 +2781,13 @@ smtp_message_begin(struct smtp_tx *tx)
>>}
>>}
>> + /* If we get failed "RCPT TO" commands that modify tx->evp, then
>> +* make sure we use the real recipient for the "for" clause */
> See style(9) how comments should be formatted:
> /*
>* Multi-line comments look like this. Make them real sentences.
>* Fill them so they look like real paragraphs.
>*/
Got it.
>>if (tx->rcptcount == 1) {
>> + srfp = TAILQ_FIRST(>rcpts);
>>m_printf(tx, "\n\tfor <%s@%s>",
>> - tx->evp.rcpt.user,
>> - tx->evp.rcpt.domain);
>> + srfp->maddr.user,
>> + srfp->maddr.domain);
> I don't see anything wrong with this, but does the code still do the correct
> thing with multiple recipients?
> RCPT TO:
> RCPT TO:
> RCPT TO:
> or
> RCPT TO:
> RCPT TO:
> RCPT TO:
For reference, testing the two above scenarios with the original gives
the following Received: header (except date and message id
differences):
Received: by thief.private (OpenSMTPD) with ESMTP id 4f5144f3; Sun, 9 Oct 2022
12:09:22 -0600 (MDT)
With the patch it still gives the following for both scenarios:
Received: by thief.private (OpenSMTPD) with ESMTP id 50bf7075; Sun, 9 Oct 2022
12:14:32 -0600 (MDT)
Modified patch with comment fix (feel free to remove the comment
entirely if you think it's not needed):
Index: smtp_session.c
===
RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v
retrieving revision 1.432
diff -u -p -r1.432 smtp_session.c
--- smtp_session.c1 Jul 2021 07:42:16 -1.432
+++ smtp_session.c9 Oct 2022 18:15:53 -
@@ -2732,6 +2732,7 @@ static void
smtp_message_begin(struct smtp_tx *tx)
{
struct smtp_session *s;
+struct smtp_rcpt *srfp;
int(*m_printf)(struct smtp_tx *, const char *, ...);
m_printf = smtp_message_printf;
@@ -2780,10 +2781,15 @@ smtp_message_begin(struct smtp_tx *tx)
}
}
+/*
+ * If we get failed "RCPT TO" commands that modify tx->evp, then
+ * make sure we use the real recipient for the "for" clause
+ */
if (tx->rcptcount == 1) {
+srfp = TAILQ_FIRST(>rcpts);
m_printf(tx, "\n\tfor <%s@%s>",
- tx->evp.rcpt.user,
-tx->evp.rcpt.domain);
+srfp->maddr.user,
+srfp->maddr.domain);
}
m_printf(tx, ";\n\t%s\n", time_to_text(time(>time)));
Patch with no comment:
Index: smtp_session.c
===
RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v
retrieving revision 1.432
diff -u -p -r1.432 smtp_session.c
--- smtp_session.c1 Jul 2021 07:42:16 -1.432
+++ smtp_session.c9 Oct 2022 18:18:44 -
@@ -2732,6 +2732,7 @@ static void
smtp_message_begin(struct smtp_tx *tx)
{
struct smtp_session *s;
+struct smtp_rcpt *srfp;
int(*m_printf)(struct smtp_tx *, const char *, ...);
m_printf = smtp_message_printf;
@@ -2781,9 +2782,10 @@ smtp_message_begin(struct smtp_tx *tx)
}
if (tx->rcptcount == 1) {
+srfp = TAILQ_FIRST(>rcpts);
m_printf(tx, "\n\tfor <%s@%s>",
-tx->evp.rcpt.user,
-tx->evp.rcpt.domain);
+srfp->maddr.user,
+srfp->maddr.domain);
}
m_printf(tx, ";\n\t%s\n", time_to_text(time(>time)));
rc.d: smtpd, unwind: add configtest
Two more, then all daemons in my accumulated `rcctl ls on' output should
be covered.
OK?
Index: smtpd
===
RCS file: /cvs/src/etc/rc.d/smtpd,v
retrieving revision 1.7
diff -u -p -r1.7 smtpd
--- smtpd 11 Jan 2018 19:52:12 - 1.7
+++ smtpd 14 Oct 2022 10:47:19 -
@@ -6,6 +6,11 @@ daemon="/usr/sbin/smtpd"
. /etc/rc.d/rc.subr
+rc_configtest() {
+ # use rc_exec here since daemon_flags may contain arguments with spaces
+ rc_exec "${daemon} -n ${daemon_flags}"
+}
+
rc_reload=NO
rc_cmd $1
Index: unwind
===
RCS file: /cvs/src/etc/rc.d/unwind,v
retrieving revision 1.2
diff -u -p -r1.2 unwind
--- unwind 7 Feb 2019 17:54:01 - 1.2
+++ unwind 14 Oct 2022 10:48:03 -
@@ -6,4 +6,9 @@ daemon="/sbin/unwind"
. /etc/rc.d/rc.subr
+rc_configtest() {
+ # use rc_exec here since daemon_flags may contain arguments with spaces
+ rc_exec "${daemon} -n ${daemon_flags}"
+}
+
rc_cmd $1
Re: smtpd bug in Received: header with one recipient
On Sun, Oct 09, 2022 at 08:01:01AM +0200, Martijn van Duren wrote:
> Bit focused on other things atm and not familiar with this part of the
> code, but some comments.
>
> On Sat, 2022-10-08 at 12:18 -0600, Chris Waddey wrote:
>> A message with a single successful recipient but with a failed
>> rcpt to: command afterward generates an incorrect Received: header.
...
>> The following patch fixes the problem:
>> Index: smtp_session.c
>> ===
>> RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v
>> retrieving revision 1.432
>> diff -u -p -r1.432 smtp_session.c
>> --- smtp_session.c 1 Jul 2021 07:42:16 - 1.432
>> +++ smtp_session.c 8 Oct 2022 18:04:51 -
>> @@ -2732,6 +2732,7 @@ static void
>> smtp_message_begin(struct smtp_tx *tx)
>> {
>>struct smtp_session *s;
>> + struct smtp_rcpt *srfp;
>>int (*m_printf)(struct smtp_tx *, const char *, ...);
>>m_printf = smtp_message_printf;
>> @@ -2780,10 +2781,13 @@ smtp_message_begin(struct smtp_tx *tx)
>>}
>>}
>> + /* If we get failed "RCPT TO" commands that modify tx->evp, then
>> +* make sure we use the real recipient for the "for" clause */
> See style(9) how comments should be formatted:
> /*
>* Multi-line comments look like this. Make them real sentences.
>* Fill them so they look like real paragraphs.
>*/
Got it.
>>if (tx->rcptcount == 1) {
>> + srfp = TAILQ_FIRST(>rcpts);
>>m_printf(tx, "\n\tfor <%s@%s>",
>> - tx->evp.rcpt.user,
>> - tx->evp.rcpt.domain);
>> + srfp->maddr.user,
>> + srfp->maddr.domain);
> I don't see anything wrong with this, but does the code still do the correct
> thing with multiple recipients?
> RCPT TO:
> RCPT TO:
> RCPT TO:
> or
> RCPT TO:
> RCPT TO:
> RCPT TO:
For reference, testing the two above scenarios with the original gives
the following Received: header (except date and message id
differences):
Received: by thief.private (OpenSMTPD) with ESMTP id 4f5144f3; Sun, 9 Oct 2022
12:09:22 -0600 (MDT)
With the patch it still gives the following for both scenarios:
Received: by thief.private (OpenSMTPD) with ESMTP id 50bf7075; Sun, 9 Oct 2022
12:14:32 -0600 (MDT)
Modified patch with comment fix (feel free to remove the comment
entirely if you think it's not needed):
Index: smtp_session.c
===
RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v
retrieving revision 1.432
diff -u -p -r1.432 smtp_session.c
--- smtp_session.c1 Jul 2021 07:42:16 -1.432
+++ smtp_session.c9 Oct 2022 18:15:53 -
@@ -2732,6 +2732,7 @@ static void
smtp_message_begin(struct smtp_tx *tx)
{
struct smtp_session *s;
+struct smtp_rcpt *srfp;
int(*m_printf)(struct smtp_tx *, const char *, ...);
m_printf = smtp_message_printf;
@@ -2780,10 +2781,15 @@ smtp_message_begin(struct smtp_tx *tx)
}
}
+/*
+ * If we get failed "RCPT TO" commands that modify tx->evp, then
+ * make sure we use the real recipient for the "for" clause
+ */
if (tx->rcptcount == 1) {
+srfp = TAILQ_FIRST(>rcpts);
m_printf(tx, "\n\tfor <%s@%s>",
- tx->evp.rcpt.user,
-tx->evp.rcpt.domain);
+srfp->maddr.user,
+srfp->maddr.domain);
}
m_printf(tx, ";\n\t%s\n", time_to_text(time(>time)));
Patch with no comment:
Index: smtp_session.c
===
RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v
retrieving revision 1.432
diff -u -p -r1.432 smtp_session.c
--- smtp_session.c1 Jul 2021 07:42:16 -1.432
+++ smtp_session.c9 Oct 2022 18:18:44 -
@@ -2732,6 +2732,7 @@ static void
smtp_message_begin(struct smtp_tx *tx)
{
struct smtp_session *s;
+struct smtp_rcpt *srfp;
int(*m_printf)(struct smtp_tx *, const char *, ...);
m_printf = smtp_message_printf;
@@ -2781,9 +2782,10 @@ smtp_message_begin(struct smtp_tx *tx)
}
if (tx->rcptcount == 1) {
+srfp = TAILQ_FIRST(>rcpts);
m_printf(tx, "\n\tfor <%s@%s>",
-tx->evp.rcpt.user,
-tx->evp.rcpt.domain);
+srfp->maddr.user,
+srfp->maddr.domain);
}
m_printf(tx, ";\n\t%s\n", time_to_text(time(>time)));
smtpd bug in Received: header with one recipient
A message with a single successful recipient but with a failed
rcpt to: command afterward generates an incorrect Received: header.
The following produces the bug:
thief$ nc localhost 25
220 thief.private ESMTP OpenSMTPD
ehlo thief.private^M
250-thief.private Hello thief.private [127.0.0.1], pleased to meet you
250-8BITMIME
250-ENHANCEDSTATUSCODES
250-SIZE 36700160
250-DSN
250 HELP
mail from:^M
250 2.0.0 Ok
rcpt to:^M
250 2.1.5 Destination address valid: Recipient ok
rcpt to:^M
550 Invalid recipient:
data^M
354 Enter mail, end with "." on a line by itself
From:^M
^M
test^M
.^M
250 2.0.0 8f9363cc Message accepted for delivery
quit^M
221 2.0.0 Bye
thief$
This gives the following message (I have the mask-src option set here):
Date: Sat, 8 Oct 2022 12:08:48 -0600 (MDT)
From: me@thief.private
Return-Path:
Delivered-To: me@thief.private
Received: by thief.private (OpenSMTPD) with ESMTP id 8f9363cc for
; Sat, 8 Oct 2022 12:08:48 -0600 (MDT)
Message-ID:
test
The following patch fixes the problem:
Index: smtp_session.c
===
RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v
retrieving revision 1.432
diff -u -p -r1.432 smtp_session.c
--- smtp_session.c 1 Jul 2021 07:42:16 - 1.432
+++ smtp_session.c 8 Oct 2022 18:04:51 -
@@ -2732,6 +2732,7 @@ static void
smtp_message_begin(struct smtp_tx *tx)
{
struct smtp_session *s;
+ struct smtp_rcpt *srfp;
int (*m_printf)(struct smtp_tx *, const char *, ...);
m_printf = smtp_message_printf;
@@ -2780,10 +2781,13 @@ smtp_message_begin(struct smtp_tx *tx)
}
}
+ /* If we get failed "RCPT TO" commands that modify tx->evp, then
+* make sure we use the real recipient for the "for" clause */
if (tx->rcptcount == 1) {
+ srfp = TAILQ_FIRST(>rcpts);
m_printf(tx, "\n\tfor <%s@%s>",
- tx->evp.rcpt.user,
- tx->evp.rcpt.domain);
+ srfp->maddr.user,
+ srfp->maddr.domain);
}
m_printf(tx, ";\n\t%s\n", time_to_text(time(>time)));
I use OpenSMTPD on OpenBSD for my personal mail, and it's great. Happy
to contribute in any way I can. Thank you for all you do.
OpenBSD Errata: September 26, 2022 (smtpd)
Errata patches for smtpd have been released for OpenBSD 7.0 and 7.1. Binary updates for the amd64, i386 and arm64 platform are available via the syspatch utility. Source code patches can be found on the respective errata page: https://www.openbsd.org/errata70.html https://www.openbsd.org/errata71.html
[PATCH] smtpd: always use an enhanced status code
A server that supports enhanced status codes must use them for all
replies.
---
usr.sbin/smtpd/bounce.c | 2 +-
usr.sbin/smtpd/lka_session.c | 4 ++--
usr.sbin/smtpd/smtp_session.c | 3 ++-
3 files changed, 5 insertions(+), 4 deletions(-)
diff --git usr.sbin/smtpd/bounce.c usr.sbin/smtpd/bounce.c
index 5d0dad68698..0982be7e756 100644
--- usr.sbin/smtpd/bounce.c
+++ usr.sbin/smtpd/bounce.c
@@ -762,7 +762,7 @@ bounce_io(struct io *io, int evt, void *arg)
break;
default:
- bounce_status(s, "442 i/o error %d", evt);
+ bounce_status(s, "442 4.3.0 i/o error %d", evt);
bounce_free(s);
break;
}
diff --git usr.sbin/smtpd/lka_session.c usr.sbin/smtpd/lka_session.c
index cac9108349e..b390417440d 100644
--- usr.sbin/smtpd/lka_session.c
+++ usr.sbin/smtpd/lka_session.c
@@ -206,9 +206,9 @@ lka_resume(struct lka_session *lks)
m_add_string(p_dispatcher, lks->errormsg);
else {
if (lks->error == LKA_PERMFAIL)
- m_add_string(p_dispatcher, "550 Invalid
recipient");
+ m_add_string(p_dispatcher, "550 5.1.0 Invalid
recipient");
else if (lks->error == LKA_TEMPFAIL)
- m_add_string(p_dispatcher, "451 Temporary
failure");
+ m_add_string(p_dispatcher, "451 4.3.0 Temporary
failure");
}
m_close(p_dispatcher);
diff --git usr.sbin/smtpd/smtp_session.c usr.sbin/smtpd/smtp_session.c
index 13756932208..167834b9536 100644
--- usr.sbin/smtpd/smtp_session.c
+++ usr.sbin/smtpd/smtp_session.c
@@ -2834,7 +2834,8 @@ smtp_message_end(struct smtp_tx *tx)
default:
/* fatal? */
- smtp_reply(s, "421 Internal server error");
+ smtp_reply(s, "421 %s Internal server error",
+ esc_code(ESC_STATUS_TEMPFAIL,
ESC_OTHER_MAIL_SYSTEM_STATUS));
}
smtp_tx_rollback(tx);
--
Sincerely,
Demi Marie Obenour (she/her/hers)
OpenPGP_0xB288B55FFF9C22C1.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: smtpd: use libtls signer
On Sat, Feb 12, 2022 at 02:49:46PM +0100, Eric Faurot wrote: > 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@ ok tb
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
Re: smtpd: LINE_MAX might not be enough for a response
On Nov 25 01:38:35, h...@stare.cz wrote:
> On Nov 24 14:47:48, j...@maudlin.dev wrote:
> >
> > Jan Stary writes:
> > > smtpd just failed to parse a SMTP response (below),
> > > saying 'line too long'.
> > >
> > > Looking at the source, this seems to be parse_smtp_response() in util.c,
> > > which errors out right away with
> > >
> > > if (len >= LINE_MAX)
> > > return "line too long";
> > >
> > > Apparently,
> > >
> > > /usr/include/sys/syslimits.h:#define LINE_MAX 2048
> > >
> > > is not enough, as shown by this line from
> > > (you guessed it) outlook.office365.com:
> >
> > > There is also
> > >
> > > smtp_session.c:#define SMTP_LINE_MAX 65535
> >
> > > Index: util.c
> > > ===
> > > RCS file: /cvs/src/usr.sbin/smtpd/util.c,v
> > > retrieving revision 1.154
> > > diff -u -p -r1.154 util.c
> > > --- util.c14 Jun 2021 17:58:16 - 1.154
> > > +++ util.c24 Nov 2021 17:59:14 -
> > > @@ -685,9 +685,6 @@ session_socket_error(int fd)
> > > const char *
> > > parse_smtp_response(char *line, size_t len, char **msg, int *cont)
> > > {
> > > - if (len >= LINE_MAX)
> > > - return "line too long";
> > > -
> > > if (len > 3) {
> > > if (msg)
> > > *msg = line + 4;
> >
> > [...]
> > At this point it seems to me that changing LINE_MAX to SMTP_LINE_MAX
> > would require a bit more digging, and I didn't look into any RFCs for
> > whether this is a standard value that outlook is ignoring.
https://datatracker.ietf.org/doc/html/rfc5321#section-4.5.3.1
https://datatracker.ietf.org/doc/html/rfc5321#section-4.5.3.1.5
I have no idea what the server is trying to say
with the long reply.
Jan
Re: smtpd: LINE_MAX might not be enough for a response
On Nov 24 14:47:48, j...@maudlin.dev wrote:
>
> Jan Stary writes:
> > smtpd just failed to parse a SMTP response (below),
> > saying 'line too long'.
> >
> > Looking at the source, this seems to be parse_smtp_response() in util.c,
> > which errors out right away with
> >
> > if (len >= LINE_MAX)
> > return "line too long";
> >
> > Apparently,
> >
> > /usr/include/sys/syslimits.h:#define LINE_MAX 2048
> >
> > is not enough, as shown by this line from
> > (you guessed it) outlook.office365.com:
>
> > There is also
> >
> > smtp_session.c:#define SMTP_LINE_MAX 65535
>
> > Index: util.c
> > ===
> > RCS file: /cvs/src/usr.sbin/smtpd/util.c,v
> > retrieving revision 1.154
> > diff -u -p -r1.154 util.c
> > --- util.c 14 Jun 2021 17:58:16 - 1.154
> > +++ util.c 24 Nov 2021 17:59:14 -
> > @@ -685,9 +685,6 @@ session_socket_error(int fd)
> > const char *
> > parse_smtp_response(char *line, size_t len, char **msg, int *cont)
> > {
> > - if (len >= LINE_MAX)
> > - return "line too long";
> > -
> > if (len > 3) {
> > if (msg)
> > *msg = line + 4;
>
> We really shouldn't simply remove this check.
Absolutely, that was just to see what the long line was.
Thanks for digging in.
Jan
> In this case parse_smtp_response gets called in two places:
>
> *
> https://github.com/openbsd/src/blob/ceeebefe3564938ff6ba87e7f465f6c35aeb8b03/usr.sbin/smtpd/bounce.c#L726
> *
> https://github.com/openbsd/src/blob/ceeebefe3564938ff6ba87e7f465f6c35aeb8b03/usr.sbin/smtpd/mta_session.c#L1252
>
> I think the correct thing to do in this case is examine how 'char *line'
> is allocated, which should help to determine if SMTP_LINE_MAX would in
> fact be a suitable replacement for LINE_MAX.
>
> Digging a little further, it appears that in usr.sbin/smtpd/bounce.c:711
> we see that LINE_MAX is used for comparison before later calling
> parse_smtp_response with char *line and size_t len:
>
> case IO_DATAIN:
> nextline:
> line = io_getline(s->io, );
> if (line == NULL && io_datalen(s->io) >= LINE_MAX) {
> bounce_status(s, "Input too long");
> bounce_free(s);
> return;
> }
>
> At this point it seems to me that changing LINE_MAX to SMTP_LINE_MAX
> would require a bit more digging, and I didn't look into any RFCs for
> whether this is a standard value that outlook is ignoring.
>
>
smtpd: LINE_MAX might not be enough for a response
This is current/amd64 on a PC.
smtpd just failed to parse a SMTP response (below),
saying 'line too long'.
Looking at the source, this seems to be parse_smtp_response() in util.c,
which errors out right away with
if (len >= LINE_MAX)
return "line too long";
Apparently,
/usr/include/sys/syslimits.h:#define LINE_MAX 2048
is not enough, as shown by this line from
(you guessed it) outlook.office365.com:
h...@stare.cz: 554 5.2.252 SendAsDenied; stary...@cvut.cz not allowed to send
as h...@biblio.stare.cz;
STOREDRV.Submission.Exception:SendAsDeniedException.MapiExceptionSendAsDenied;
Failed to process message due to a permanent exception with message Cannot
submit message. 0.35250:CF3A, 1.36674:0100, 1.61250:,
1.45378:0200, 1.44866:, 1.36674:7A00, 1.61250:,
1.45378:0500, 1.44866:0014, 1.36674:0A00, 1.61250:,
1.45378:1600, 1.44866:AE1A, 1.36674:0E00, 1.61250:,
1.45378:C71A, 1.44866:E000, 16.55847:EE0C,
17.43559:30020100,
20.52176:140F69921A001F001432, 20.50032:140F69928A1770200201E265,
0.35180:40001E32, 255.23226:2100, 255.27962:7A00, 255.27962:0A00,
255.27962:0E00, 255.31418:03000136, 0.35250:0300A531, 1.36674:0A00,
1.61250:, 1.45378:0200, 1.44866:1800, 1.36674:3200,
1.61250:, 1.45378:1D00, 1.44866:0100, 16.55847:8400,
17.43559:8803,
20.52176:140F69921A0070200A001380, 20.50032:140F69928A1710106F00,
0.35180:03000B67, 255.23226:0A00CD30, 255.27962:0A00, 255.27962:3200,
255.17082:DC04, 0.27745:7900, 4.21921:DC04, 255.27962:FA00,
255.1494:7E00, 0.38698:05000780, 1.41134:4600, 0.37692:8600,
0.37948:8600, 5.33852:534D5450,
7.36354:010001098600, 4.56248:DC04,
7.40748:0100010B8600, 7.57132:8600,
1.63016:3200, 4.39640:DC04,
8.45434:9A6929EA0096114FBBA64268B1990A848600, 1.46798:0400,
5.10786:31352E32302E343731332E3032363A414D39505230364D42383138303A65613539613834342D653163302D343234332D613934632D3263343563353536646533383A34353639323400DA10100300,
7.51330:8C34EA6973AFD9080700, 0.39570:0300, 1.55954:0A00,
0.49266:0200, 1.33010:0A00, 2.54258:, 0.40002:7A00,
1.56562:, 1.64146:3200, 1.33010:3200, 2.54258:DC04,
255.1750:0300
Yes, this is exactly 2048 bytes of an unfinished line.
(The actual error is my misconfiguration,
but that's beside the point here.)
There is also
smtp_session.c:#define SMTP_LINE_MAX 65535
which I guess would be the right replacement, but as this is
is only defined in one C file and I don't know the code base,
I'll leave that to someone else.
Naively, I am just removing the len >= condition,
which is probably not correct; I don't see where the
parsing function needs that, but perhaps this limitation
is relied upon elsewhere.
Jan
Index: util.c
===
RCS file: /cvs/src/usr.sbin/smtpd/util.c,v
retrieving revision 1.154
diff -u -p -r1.154 util.c
--- util.c 14 Jun 2021 17:58:16 - 1.154
+++ util.c 24 Nov 2021 17:59:14 -
@@ -685,9 +685,6 @@ session_socket_error(int fd)
const char *
parse_smtp_response(char *line, size_t len, char **msg, int *cont)
{
- if (len >= LINE_MAX)
- return "line too long";
-
if (len > 3) {
if (msg)
*msg = line + 4;
Re: smtpd smtp_proceed_wiz function
Crystal Kolipe [kolip...@exoticsilicon.com] wrote: > On Mon, Nov 08, 2021 at 06:13:14PM +, Stuart Henderson wrote: > > On 2021/11/08 14:52, Crystal Kolipe wrote: > > > I'm not aware of a 'wiz' command in any SMTP related RFC. > > This will become clear if you look into sendmail history :) > > Got it :). > > I assume that this won't be implemented in OpenBSD any time soon > then. We could emulate it and pretend that we are an ancient vulnerable verison of Sendmail, or pretty much any version since they all contain a plethora of vulnerabilities. While we're at it, maybe emulate Microsoft Exchange and EXIM :) Chris
Re: smtpd smtp_proceed_wiz function
On Mon, Nov 08, 2021 at 06:13:14PM +, Stuart Henderson wrote: > On 2021/11/08 14:52, Crystal Kolipe wrote: > > I'm not aware of a 'wiz' command in any SMTP related RFC. > This will become clear if you look into sendmail history :) Got it :). I assume that this won't be implemented in OpenBSD any time soon then.
Re: smtpd smtp_proceed_wiz function
On 2021/11/08 14:52, Crystal Kolipe wrote:
> src/usr.sbin/smtpd/smtp_session.c contains the following code:
>
> 1892static void
> 1893smtp_proceed_wiz(struct smtp_session *s, const char *args)
> 1894{
> 1895smtp_reply(s, "500 %s %s: this feature is not supported
> yet ;-)",
> 1896esc_code(ESC_STATUS_PERMFAIL, ESC_INVALID_COMMAND),
> 1897esc_description(ESC_INVALID_COMMAND));
> 1898}
>
> This was added between revisions 1.194 and 1.195, with no mention in the
> changelog.
>
> I'm not aware of a 'wiz' command in any SMTP related RFC.
>
> Is this spurious debugging code related to the addition of DSN and enhanced
> status code support? Or is it there as a way to identify servers that are
> running smtpd in the wild? Or is there some other reason?
>
This will become clear if you look into sendmail history :)
smtpd smtp_proceed_wiz function
src/usr.sbin/smtpd/smtp_session.c contains the following code:
1892 static void
1893 smtp_proceed_wiz(struct smtp_session *s, const char *args)
1894 {
1895 smtp_reply(s, "500 %s %s: this feature is not supported yet
;-)",
1896 esc_code(ESC_STATUS_PERMFAIL, ESC_INVALID_COMMAND),
1897 esc_description(ESC_INVALID_COMMAND));
1898 }
This was added between revisions 1.194 and 1.195, with no mention in the
changelog.
I'm not aware of a 'wiz' command in any SMTP related RFC.
Is this spurious debugging code related to the addition of DSN and enhanced
status code support? Or is it there as a way to identify servers that are
running smtpd in the wild? Or is there some other reason?
Re: Add missing manpage for smtpd
Hi Crystal Kolipe wrote: > I sent this to bugs@ a while back, but it seems to have been missed. > > smtpd-filters.7 is not installed by default. > > --- usr.sbin/smtpd/smtpd/Makefile.distWed Apr 21 04:54:10 2021 > +++ usr.sbin/smtpd/smtpd/Makefile Mon Oct 25 11:54:39 2021 > @@ -76,7 +76,7 @@ > > SRCS+= stat_ramstat.c > > -MAN= sendmail.8 smtpd.8 smtpd.conf.5 table.5 > +MAN= sendmail.8 smtpd-filters.7 smtpd.8 smtpd.conf.5 table.5 > BINDIR= /usr/sbin > > LDADD+= -levent -lutil -ltls -lssl -lcrypto -lz Following discussions with jmc@ and gilles@ I'm doing some work on the English language on smtpd-filters.7 with a view to getting it installed.
Re: Add missing manpage for smtpd
On Mon, Oct 25, 2021 at 04:12:17PM +0100, Larry Hynes wrote: > Hi > > Crystal Kolipe wrote: > > I sent this to bugs@ a while back, but it seems to have been missed. > > > > smtpd-filters.7 is not installed by default. > > > > --- usr.sbin/smtpd/smtpd/Makefile.dist Wed Apr 21 04:54:10 2021 > > +++ usr.sbin/smtpd/smtpd/Makefile Mon Oct 25 11:54:39 2021 > > @@ -76,7 +76,7 @@ > > > > SRCS+= stat_ramstat.c > > > > -MAN= sendmail.8 smtpd.8 smtpd.conf.5 table.5 > > +MAN= sendmail.8 smtpd-filters.7 smtpd.8 smtpd.conf.5 table.5 > > BINDIR=/usr/sbin > > > > LDADD+=-levent -lutil -ltls -lssl -lcrypto -lz > > Following discussions with jmc@ and gilles@ I'm doing some work on the > English language on smtpd-filters.7 with a view to getting it installed. > yep, we'll hold off until you get a chance to go over it. jmc
Add missing manpage for smtpd
I sent this to bugs@ a while back, but it seems to have been missed. smtpd-filters.7 is not installed by default. --- usr.sbin/smtpd/smtpd/Makefile.dist Wed Apr 21 04:54:10 2021 +++ usr.sbin/smtpd/smtpd/Makefile Mon Oct 25 11:54:39 2021 @@ -76,7 +76,7 @@ SRCS+= stat_ramstat.c -MAN= sendmail.8 smtpd.8 smtpd.conf.5 table.5 +MAN= sendmail.8 smtpd-filters.7 smtpd.8 smtpd.conf.5 table.5 BINDIR=/usr/sbin LDADD+=-levent -lutil -ltls -lssl -lcrypto -lz
Re: [diff] usr.sbin/smtpd add missing includes
[2021-10-18 11:09] Jonathan Gray
> On Sun, Oct 17, 2021 at 04:23:50PM +0200, Philipp wrote:
> > Hello
> >
> > I'm currently working on getting OpenSMTPD-portable build. During this
> > I found some missing includes.
>
> It would help if you could describe the platform you are building on and
> show the compile errors.
Oh sorry, I currently work on Debian and FreeBSD.
Error on Debian 11.1 with clang-11:
==
clang-11 -DHAVE_CONFIG_H -I. -I../.. -I../../usr.sbin/smtpd
-I../../openbsd-compat -I../../openbsd-compat/err_h
-I../../openbsd-compat/libasr -I. -I/usr/include
-DSMTPD_CONFDIR=\"/usr/local/etc\" -DPATH_CHROOT=\"/var/empty\"
-DPATH_SMTPCTL=\"/usr/local/sbin/smtpctl\"
-DPATH_MAILLOCAL=\"/usr/local/libexec/opensmtpd/mail.local\"
-DPATH_MAKEMAP=\"/usr/local/sbin/makemap\"
-DPATH_LIBEXEC=\"/usr/local/libexec/opensmtpd\" -DHAVE_CONFIG_H -DNO_IO
-DCONFIG_MINIMUM -DPATH_GZCAT=\"/bin/zcat\"
-DPATH_ENCRYPT=\"/usr/local/libexec/opensmtpd/encrypt\" -g -O2 -fPIC -DPIC
-Qunused-arguments -Wunknown-warning-option -Wall -Wpointer-arith
-Wuninitialized -Wsign-compare -Wformat-security -Wsizeof-pointer-memaccess
-Wno-pointer-sign -Wno-unused-result -fno-strict-aliasing -fno-builtin-memset
-D_BSD_SOURCE -D_DEFAULT_SOURCE -D_GNU_SOURCE -c -o
../../usr.sbin/smtpd/smtpctl-smtpctl.o `test -f
'../../usr.sbin/smtpd/smtpctl.c' || echo './'`../../usr.sbin/smtpd/smtpctl.c
../../usr.sbin/smtpd/smtpctl.c:519:7: warning: implicit declaration of function
'setgroups' is invalid in C99 [-Wimplicit-function-declaration]
if ((setgroups(1, >pw_gid) ||
^
../../usr.sbin/smtpd/smtpctl.c:1067:12: warning: implicit declaration of
function 'getgrnam' is invalid in C99 [-Wimplicit-function-declaration]
if ((gr = getgrnam(SMTPD_QUEUE_GROUP)) == NULL)
^
../../usr.sbin/smtpd/smtpctl.c:1067:10: warning: incompatible integer to
pointer conversion assigning to 'struct group *' from 'int' [-Wint-conversion]
if ((gr = getgrnam(SMTPD_QUEUE_GROUP)) == NULL)
^ ~~~
../../usr.sbin/smtpd/smtpctl.c:1069:13: error: incomplete definition of type
'struct group'
else if (gr->gr_gid != getegid())
~~^
../../usr.sbin/smtpd/smtpctl.c:1058:9: note: forward declaration of 'struct
group'
struct group*gr;
^
3 warnings and 1 error generated.
==
The include grp.h fixes the build, with some other portable specific
changes:
https://github.com/OpenSMTPD/OpenSMTPD/pull/1153
https://github.com/OpenSMTPD/OpenSMTPD/pull/1154
First error on FreeBSD 13.0-RELEASE-p4 with clang12:
==
clang12 -DHAVE_CONFIG_H -I. -I../.. -I../../usr.sbin/smtpd
-I../../openbsd-compat -I../../openbsd-compat/err_h
-I../../openbsd-compat/paths_h -I../../openbsd-compat/libtls -I.
-DSMTPD_CONFDIR=\"/usr/local/etc\"
-DPATH_CHROOT=\"/var/empty\"
-DPATH_SMTPCTL=\"/usr/local/sbin/smtpctl\"
-DPATH_MAILLOCAL=\"/usr/local/libexec/opensmtpd/mail.local\"
-DPATH_MAKEMAP=\"/usr/local/sbin/makemap\"
-DPATH_LIBEXEC=\"/usr/local/libexec/opensmtpd\" -DHAVE_CONFIG_H
-I/usr/local/include -DIO_TLS -DCA_FILE=\"/etc/ssl/cert.pem\"
-I/usr/local/include -fPIC -DPIC -Qunused-arguments -Wunknown-warning-option
-Wall -Wpointer-arith -Wuninitialized -Wsign-compare -Wformat-security
-Wsizeof-pointer-memaccess -Wno-pointer-sign -Wno-unused-result
-fno-strict-aliasing -fno-builtin-memset -c -o
../../usr.sbin/smtpd/smtpd-parse.o `test -f '../../usr.sbin/smtpd/parse.c' ||
echo './'`../../usr.sbin/smtpd/parse.c
../../usr.sbin/smtpd/parse.y:2639:2: warning: implicitly declaring library
function 'free' with type 'void (void *)' [-Wimplicit-function-declaration]
free(msg);
^
../../usr.sbin/smtpd/parse.y:2639:2: note: include the header or
explicitly provide a declaration for 'free'
../../usr.sbin/smtpd/parse.y:2646:10: warning: implicitly declaring library
function 'strcmp' with type 'int (const char *, const char *)'
[-Wimplicit-function-declaration]
return (strcmp(k, ((const struct keywords *)e)->k_name));
^
../../usr.sbin/smtpd/parse.y:2646:10: note: include the header or
explicitly provide a declaration for 'strcmp'
../../usr.sbin/smtpd/parse.y:2758:6: warning: implicit declaration of function
'bsearch' is invalid in C99 [-Wimplicit-function-declaration]
p = bsearch(s, keywords, sizeof(keywords)/sizeof(keywords[0]),
^
../../usr.sbin/smtpd/parse.y:2758:4: warning: incompatible integer to pointer
conversion assigning to 'const struct keywords *' from 'int'
Re: [diff] usr.sbin/smtpd add missing includes
On Sun, Oct 17, 2021 at 04:23:50PM +0200, Philipp wrote: > Hello > > I'm currently working on getting OpenSMTPD-portable build. During this > I found some missing includes. It would help if you could describe the platform you are building on and show the compile errors. > > diff --git a/usr.sbin/smtpd/parse.y b/usr.sbin/smtpd/parse.y > index 7de52a1c568..b1307c4daa6 100644 > --- a/usr.sbin/smtpd/parse.y > +++ b/usr.sbin/smtpd/parse.y > @@ -28,6 +28,8 @@ > #include > #include > > +#include > +#include > #include > #include > #include > diff --git a/usr.sbin/smtpd/smtpctl.c b/usr.sbin/smtpd/smtpctl.c > index 3d5926efdad..9e88f150ae5 100644 > --- a/usr.sbin/smtpd/smtpctl.c > +++ b/usr.sbin/smtpd/smtpctl.c > @@ -27,6 +27,7 @@ > #include > #include > #include > +#include > #include > #include > #include > >
smtpd workarounds for KAME sin6_scope_id
Hello As said in the other mail I'm currently working on building OpenSMTPD on other platforms. A problem I found is the workaround for sin6_scope_id. The problem with the workaround is that FreeBSD don't expose IN6_IS_ADDR_MC_INTFACELOCAL(). After a bit digging in the code I found this workaround isn't needed for FreeBSD. I don't know what about other OS with the KAME stack. Currently I see two ways to fix this: 1. Implement sin6_scope_id correct in OpenBSD and remove the workaround. 2. Add an extra check in the portable build process if this workaround is needed. Whats your opinion for this problem? Philipp
[diff] usr.sbin/smtpd add missing includes
Hello I'm currently working on getting OpenSMTPD-portable build. During this I found some missing includes. diff --git a/usr.sbin/smtpd/parse.y b/usr.sbin/smtpd/parse.y index 7de52a1c568..b1307c4daa6 100644 --- a/usr.sbin/smtpd/parse.y +++ b/usr.sbin/smtpd/parse.y @@ -28,6 +28,8 @@ #include #include +#include +#include #include #include #include diff --git a/usr.sbin/smtpd/smtpctl.c b/usr.sbin/smtpd/smtpctl.c index 3d5926efdad..9e88f150ae5 100644 --- a/usr.sbin/smtpd/smtpctl.c +++ b/usr.sbin/smtpd/smtpctl.c @@ -27,6 +27,7 @@ #include #include #include +#include #include #include #include
Re: smtpd: move authentication to table backends
October 11, 2021 4:44 PM, gil...@poolp.org wrote: > October 8, 2021 11:34 PM, "aisha" wrote: > >> Hi all, >> I am still working on the table-procexec for opensmtpd >> and while there, I was thinking of how to do authentication >> using LDAP, which the current table-ldap from ports does not >> support. >> The primary reason for that, I believe, is that LDAP >> authentication should be done by bind and not by returning >> the userPassword and us doing the authentication with >> crypt_checkpass. That kind of defeats one of the uses of LDAP. >> >> Here I've added a patch which pushes the authentication step >> to the table backend and it only returns the final AUTH/NOAUTH >> kind of values. >> >> While here, I also made another small change with mailaddrmap, >> where instead of returning ALL possible aliases that a user >> may use, we now pass the current mailaddr to the table, so >> it can now return a smaller set of addresses. >> >> It should not affect any workflow, so testing from others >> would be appreciated. >> >> Cheers, >> Aisha > > Hello, > > I understand what you're trying to achieve but I don't think this is > the right way to achieve that. > > First, the lookup operation is a key-value mapping returning a value > that the daemon gets to decide what to do with. You're trying to fit > K_CREDENTIALS in it but it doesn't work that way: it takes a key and > an additional parameter (the password) so the table can do something > with it and return a decision. This is why your diff has lookup take > a parameter that's used by none of the lookups, except K_CREDENTIALS > which is handled as particular case. > > I think the proper way to implement is to have a low level operation > that is specifically meant to take a key, a parameter then let table > do whatever it wants and return a boolean. > Today, there's: int table_match(struct table *table, enum table_service kind, const char *key) that's used to attempt matching a key in a table assumed to hold list: match from src [...] Maybe this should be extended similarly to this: int table_match(struct table *table, enum table_service kind, const char *key, const char *value) K_SOURCE match on a list, key is not set: table_match(table, K_SOURCE, NULL, "192.168.0.1"); K_CREDENTIALS match on a mapping, key is used to resolve: table_match(table, K_CREDENTIALS, "gilles", "ilovecandies"); This is just an idea, but I still think this should be done after proc-exec :-)
Re: smtpd: move authentication to table backends
October 8, 2021 11:34 PM, "aisha" wrote: > Hi all, > I am still working on the table-procexec for opensmtpd > and while there, I was thinking of how to do authentication > using LDAP, which the current table-ldap from ports does not > support. > The primary reason for that, I believe, is that LDAP > authentication should be done by bind and not by returning > the userPassword and us doing the authentication with > crypt_checkpass. That kind of defeats one of the uses of LDAP. > > Here I've added a patch which pushes the authentication step > to the table backend and it only returns the final AUTH/NOAUTH > kind of values. > > While here, I also made another small change with mailaddrmap, > where instead of returning ALL possible aliases that a user > may use, we now pass the current mailaddr to the table, so > it can now return a smaller set of addresses. > > It should not affect any workflow, so testing from others > would be appreciated. > > Cheers, > Aisha > Hello, I understand what you're trying to achieve but I don't think this is the right way to achieve that. First, the lookup operation is a key-value mapping returning a value that the daemon gets to decide what to do with. You're trying to fit K_CREDENTIALS in it but it doesn't work that way: it takes a key and an additional parameter (the password) so the table can do something with it and return a decision. This is why your diff has lookup take a parameter that's used by none of the lookups, except K_CREDENTIALS which is handled as particular case. I think the proper way to implement is to have a low level operation that is specifically meant to take a key, a parameter then let table do whatever it wants and return a boolean. Then I don't think this should be done before proc-exec because it's much easier than breaking the current API.
Re: smtpd: move authentication to table backends
On 21/10/08 05:34PM, aisha wrote:
> Hi all,
> I am still working on the table-procexec for opensmtpd
> and while there, I was thinking of how to do authentication
> using LDAP, which the current table-ldap from ports does not
> support.
> The primary reason for that, I believe, is that LDAP
> authentication should be done by bind and not by returning
> the userPassword and us doing the authentication with
> crypt_checkpass. That kind of defeats one of the uses of LDAP.
>
> Here I've added a patch which pushes the authentication step
> to the table backend and it only returns the final AUTH/NOAUTH
> kind of values.
>
> While here, I also made another small change with mailaddrmap,
> where instead of returning ALL possible aliases that a user
> may use, we now pass the current mailaddr to the table, so
> it can now return a smaller set of addresses.
>
> It should not affect any workflow, so testing from others
> would be appreciated.
>
> Cheers,
> Aisha
>
Same patch but change my horrible enums representation to bitshifts
diff --git a/usr.sbin/smtpd/aliases.c b/usr.sbin/smtpd/aliases.c
index a473aeca189..8e3835f78a6 100644
--- a/usr.sbin/smtpd/aliases.c
+++ b/usr.sbin/smtpd/aliases.c
@@ -45,7 +45,7 @@ aliases_get(struct expand *expand, const char *username)
/* 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, );
+ ret = table_lookup(mapping, K_ALIAS, buf, NULL, );
if (ret < 0)
return (-1);
if (ret)
@@ -54,7 +54,7 @@ aliases_get(struct expand *expand, const char *username)
}
/* no user-part tag, try looking up user */
- ret = table_lookup(mapping, K_ALIAS, buf, );
+ ret = table_lookup(mapping, K_ALIAS, buf, NULL, );
if (ret <= 0)
return ret;
@@ -116,7 +116,7 @@ aliases_virtual_get(struct expand *expand, const struct
mailaddr *maddr)
if (!bsnprintf(buf, sizeof(buf), "%s%c%s@%s",
user, *env->sc_subaddressing_delim, tag, domain))
return 0;
- ret = table_lookup(mapping, K_ALIAS, buf, );
+ ret = table_lookup(mapping, K_ALIAS, buf, NULL, );
if (ret < 0)
return (-1);
if (ret)
@@ -126,7 +126,7 @@ aliases_virtual_get(struct expand *expand, const struct
mailaddr *maddr)
/* then, check if entry exists without user-part tag */
if (!bsnprintf(buf, sizeof(buf), "%s@%s", user, domain))
return 0;
- ret = table_lookup(mapping, K_ALIAS, buf, );
+ ret = table_lookup(mapping, K_ALIAS, buf, NULL, );
if (ret < 0)
return (-1);
if (ret)
@@ -137,7 +137,7 @@ aliases_virtual_get(struct expand *expand, const struct
mailaddr *maddr)
if (!bsnprintf(buf, sizeof(buf), "%s%c%s",
user, *env->sc_subaddressing_delim, tag))
return 0;
- ret = table_lookup(mapping, K_ALIAS, buf, );
+ ret = table_lookup(mapping, K_ALIAS, buf, NULL, );
if (ret < 0)
return (-1);
if (ret)
@@ -147,7 +147,7 @@ aliases_virtual_get(struct expand *expand, const struct
mailaddr *maddr)
/* Failed ? We lookup for username only */
if (!bsnprintf(buf, sizeof(buf), "%s", user))
return 0;
- ret = table_lookup(mapping, K_ALIAS, buf, );
+ ret = table_lookup(mapping, K_ALIAS, buf, NULL, );
if (ret < 0)
return (-1);
if (ret)
@@ -160,14 +160,14 @@ aliases_virtual_get(struct expand *expand, const struct
mailaddr *maddr)
if (!bsnprintf(buf, sizeof(buf), "@%s", domain))
return 0;
/* Failed ? We lookup for catch all for virtual domain */
- ret = table_lookup(mapping, K_ALIAS, buf, );
+ ret = table_lookup(mapping, K_ALIAS, buf, NULL, );
if (ret < 0)
return (-1);
if (ret)
goto expand;
/* Failed ? We lookup for a *global* catch all */
- ret = table_lookup(mapping, K_ALIAS, "@", );
+ ret = table_lookup(mapping, K_ALIAS, "@", NULL, );
if (ret <= 0)
return (ret);
diff --git a/usr.sbin/smtpd/lka.c b/usr.sbin/smtpd/lka.c
index 764130d6078..3354ccde7d7 100644
--- a/usr.sbin/smtpd/lka.c
+++ b/usr.sbin/smtpd/lka.c
@@ -268,7 +268,7 @@ lka_imsg(struct mproc *p, struct imsg *imsg)
if (domain == NULL)
ret = table_fetch(table, K_RELAYHOST, );
else
- r
smtpd: move authentication to table backends
Hi all,
I am still working on the table-procexec for opensmtpd
and while there, I was thinking of how to do authentication
using LDAP, which the current table-ldap from ports does not
support.
The primary reason for that, I believe, is that LDAP
authentication should be done by bind and not by returning
the userPassword and us doing the authentication with
crypt_checkpass. That kind of defeats one of the uses of LDAP.
Here I've added a patch which pushes the authentication step
to the table backend and it only returns the final AUTH/NOAUTH
kind of values.
While here, I also made another small change with mailaddrmap,
where instead of returning ALL possible aliases that a user
may use, we now pass the current mailaddr to the table, so
it can now return a smaller set of addresses.
It should not affect any workflow, so testing from others
would be appreciated.
Cheers,
Aisha
diff --git a/usr.sbin/smtpd/aliases.c b/usr.sbin/smtpd/aliases.c
index a473aeca189..8e3835f78a6 100644
--- a/usr.sbin/smtpd/aliases.c
+++ b/usr.sbin/smtpd/aliases.c
@@ -45,7 +45,7 @@ aliases_get(struct expand *expand, const char *username)
/* 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, );
+ ret = table_lookup(mapping, K_ALIAS, buf, NULL, );
if (ret < 0)
return (-1);
if (ret)
@@ -54,7 +54,7 @@ aliases_get(struct expand *expand, const char *username)
}
/* no user-part tag, try looking up user */
- ret = table_lookup(mapping, K_ALIAS, buf, );
+ ret = table_lookup(mapping, K_ALIAS, buf, NULL, );
if (ret <= 0)
return ret;
@@ -116,7 +116,7 @@ aliases_virtual_get(struct expand *expand, const struct
mailaddr *maddr)
if (!bsnprintf(buf, sizeof(buf), "%s%c%s@%s",
user, *env->sc_subaddressing_delim, tag, domain))
return 0;
- ret = table_lookup(mapping, K_ALIAS, buf, );
+ ret = table_lookup(mapping, K_ALIAS, buf, NULL, );
if (ret < 0)
return (-1);
if (ret)
@@ -126,7 +126,7 @@ aliases_virtual_get(struct expand *expand, const struct
mailaddr *maddr)
/* then, check if entry exists without user-part tag */
if (!bsnprintf(buf, sizeof(buf), "%s@%s", user, domain))
return 0;
- ret = table_lookup(mapping, K_ALIAS, buf, );
+ ret = table_lookup(mapping, K_ALIAS, buf, NULL, );
if (ret < 0)
return (-1);
if (ret)
@@ -137,7 +137,7 @@ aliases_virtual_get(struct expand *expand, const struct
mailaddr *maddr)
if (!bsnprintf(buf, sizeof(buf), "%s%c%s",
user, *env->sc_subaddressing_delim, tag))
return 0;
- ret = table_lookup(mapping, K_ALIAS, buf, );
+ ret = table_lookup(mapping, K_ALIAS, buf, NULL, );
if (ret < 0)
return (-1);
if (ret)
@@ -147,7 +147,7 @@ aliases_virtual_get(struct expand *expand, const struct
mailaddr *maddr)
/* Failed ? We lookup for username only */
if (!bsnprintf(buf, sizeof(buf), "%s", user))
return 0;
- ret = table_lookup(mapping, K_ALIAS, buf, );
+ ret = table_lookup(mapping, K_ALIAS, buf, NULL, );
if (ret < 0)
return (-1);
if (ret)
@@ -160,14 +160,14 @@ aliases_virtual_get(struct expand *expand, const struct
mailaddr *maddr)
if (!bsnprintf(buf, sizeof(buf), "@%s", domain))
return 0;
/* Failed ? We lookup for catch all for virtual domain */
- ret = table_lookup(mapping, K_ALIAS, buf, );
+ ret = table_lookup(mapping, K_ALIAS, buf, NULL, );
if (ret < 0)
return (-1);
if (ret)
goto expand;
/* Failed ? We lookup for a *global* catch all */
- ret = table_lookup(mapping, K_ALIAS, "@", );
+ ret = table_lookup(mapping, K_ALIAS, "@", NULL, );
if (ret <= 0)
return (ret);
diff --git a/usr.sbin/smtpd/lka.c b/usr.sbin/smtpd/lka.c
index 764130d6078..3354ccde7d7 100644
--- a/usr.sbin/smtpd/lka.c
+++ b/usr.sbin/smtpd/lka.c
@@ -268,7 +268,7 @@ lka_imsg(struct mproc *p, struct imsg *imsg)
if (domain == NULL)
ret = table_fetch(table, K_RELAYHOST, );
else
- ret = table_lookup(table, K_RELAYHOST, domain,
);
+ ret = table_lookup(table, K_RELAYHOST, domain,
NULL, );
if (ret == -1)
m_add_int(p, LKA_TEMPFAI
Re: smtpd: srs and ruleset evaluation
Hello,
As discussed, this looks correct to me
> On 22 Sep 2021, at 15:46, Eric Faurot wrote:
>
> Hi.
>
> A user reported that decoded SRS addresses are not correctly evaluated
> against the ruleset. That's because the ruleset always matches against
> the expanded address ("dest") and not the original address ("rcpt").
> This diff should fix it.
>
> Eric.
>
>
> Index: lka_session.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/lka_session.c,v
> retrieving revision 1.95
> diff -u -p -r1.95 lka_session.c
> --- lka_session.c 14 Jun 2021 17:58:15 - 1.95
> +++ lka_session.c 21 Sep 2021 19:21:18 -
> @@ -280,19 +280,19 @@ lka_expand(struct lka_session *lks, stru
> /* handle SRS */
> if (env->sc_srs_key != NULL &&
> ep.sender.user[0] == '\0' &&
> - (strncasecmp(ep.rcpt.user, "SRS0=", 5) == 0 ||
> - strncasecmp(ep.rcpt.user, "SRS1=", 5) == 0)) {
> - srs_decoded = srs_decode(mailaddr_to_text());
> + (strncasecmp(ep.dest.user, "SRS0=", 5) == 0 ||
> + strncasecmp(ep.dest.user, "SRS1=", 5) == 0)) {
> + srs_decoded = srs_decode(mailaddr_to_text());
> if (srs_decoded &&
> - text_to_mailaddr(, srs_decoded)) {
> - /* flag envelope internal and override rcpt */
> + text_to_mailaddr(, srs_decoded)) {
> + /* flag envelope internal and override dest */
> ep.flags |= EF_INTERNAL;
> - xn->u.mailaddr = ep.rcpt;
> + xn->u.mailaddr = ep.dest;
> lks->envelope = ep;
> }
> else {
> log_warn("SRS failed to decode: %s",
> - mailaddr_to_text());
> + mailaddr_to_text());
> }
> }
>
>
Re: smtpd: srs and ruleset evaluation
On Wed, 22 Sep 2021 15:46:13 +0200, Eric Faurot wrote:
> 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.
Thanks for looking into this. OK millert@
- todd
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());
}
}
Re: [diff] src/usr.sbin/smtpd: table_diff lacks some lookup kinds
August 29, 2021 10:16 PM, gil...@poolp.org wrote:
> Hellow,
>
> The K_STRING and K_REGEX lookup kinds are missing from table_db even though
> nothing prevents
> them from working technically. The following diff is enough to allow db
> tables to be used on
> regex or string contexts.
>
> Index: table_db.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/table_db.c,v
> retrieving revision 1.22
> diff -u -p -r1.22 table_db.c
> --- table_db.c 23 Jan 2021 16:11:11 - 1.22
> +++ table_db.c 29 Aug 2021 20:08:30 -
> @@ -55,7 +55,9 @@ static char *table_db_get_entry_match(vo
>
> struct table_backend table_backend_db = {
> "db",
> -
> K_ALIAS|K_CREDENTIALS|K_DOMAIN|K_NETADDR|K_USERINFO|K_SOURCE|K_MAILADDR|K_ADDRNAME|K_MAILADDRMAP,
> + K_ALIAS|K_CREDENTIALS|K_DOMAIN|K_NETADDR|K_USERINFO|
> + K_SOURCE|K_MAILADDR|K_ADDRNAME|K_MAILADDRMAP|K_RELAYHOST|
> + K_STRING|K_REGEX,
> table_db_config,
> NULL,
> NULL,
the complete diff would be better:
Index: table_db.c
===
RCS file: /cvs/src/usr.sbin/smtpd/table_db.c,v
retrieving revision 1.22
diff -u -p -r1.22 table_db.c
--- table_db.c 23 Jan 2021 16:11:11 - 1.22
+++ table_db.c 1 Sep 2021 11:19:02 -
@@ -55,7 +55,9 @@ static char *table_db_get_entry_match(vo
struct table_backend table_backend_db = {
"db",
-
K_ALIAS|K_CREDENTIALS|K_DOMAIN|K_NETADDR|K_USERINFO|K_SOURCE|K_MAILADDR|K_ADDRNAME|K_MAILADDRMAP,
+ K_ALIAS|K_CREDENTIALS|K_DOMAIN|K_NETADDR|K_USERINFO|
+ K_SOURCE|K_MAILADDR|K_ADDRNAME|K_MAILADDRMAP|K_RELAYHOST|
+ K_STRING|K_REGEX,
table_db_config,
NULL,
NULL,
@@ -72,7 +74,8 @@ static struct keycmp {
} keycmp[] = {
{ K_DOMAIN, table_domain_match },
{ K_NETADDR, table_netaddr_match },
- { K_MAILADDR, table_mailaddr_match }
+ { K_MAILADDR, table_mailaddr_match },
+ { K_REGEX, table_regex_match },
};
struct dbhandle {
[diff] src/usr.sbin/smtpd: table_diff lacks some lookup kinds
Hellow,
The K_STRING and K_REGEX lookup kinds are missing from table_db even though
nothing prevents
them from working technically. The following diff is enough to allow db tables
to be used on
regex or string contexts.
Index: table_db.c
===
RCS file: /cvs/src/usr.sbin/smtpd/table_db.c,v
retrieving revision 1.22
diff -u -p -r1.22 table_db.c
--- table_db.c 23 Jan 2021 16:11:11 - 1.22
+++ table_db.c 29 Aug 2021 20:08:30 -
@@ -55,7 +55,9 @@ static char *table_db_get_entry_match(vo
struct table_backend table_backend_db = {
"db",
-
K_ALIAS|K_CREDENTIALS|K_DOMAIN|K_NETADDR|K_USERINFO|K_SOURCE|K_MAILADDR|K_ADDRNAME|K_MAILADDRMAP,
+ K_ALIAS|K_CREDENTIALS|K_DOMAIN|K_NETADDR|K_USERINFO|
+ K_SOURCE|K_MAILADDR|K_ADDRNAME|K_MAILADDRMAP|K_RELAYHOST|
+ K_STRING|K_REGEX,
table_db_config,
NULL,
NULL,
smtpd-filters.7 missing from makefile
Hi, Currently smtpd-filters.7 is not installed by default, which looks like an oversight. The patch below adds smtpd-filters.7 to usr.sbin/smtpd/smtpd/Makefile untrusted comment: verify with signify key for exoticsilicon.com RWRn5d3Yx35u06SleiMhZhW6FXYvG0NkGlXPEX94Q7SMqURZ/unZoXcHP0S6eJpfy4xbRhZj/lWUJUA0YOEsHkXHvyTcOet/mw8= --- usr.sbin/smtpd/smtpd/Makefile.dist Sun Apr 11 04:18:08 2021 +++ usr.sbin/smtpd/smtpd/Makefile Sun Aug 22 10:14:33 2021 @@ -77,7 +77,7 @@ SRCS+= stat_ramstat.c -MAN= sendmail.8 smtpd.8 smtpd.conf.5 table.5 +MAN= sendmail.8 smtpd.8 smtpd.conf.5 table.5 smtpd-filters.7 BINDIR=/usr/sbin LDADD+=-levent -lutil -ltls -lssl -lcrypto -lz
Re: smtpd: unnecessary "no certificate presented" log message
Hey,
this works fine on my systems. Nothing breaks so far. This ist just as
feedback to you.
Thanks and greetings
Leo
Am 30.06.2021 um 14:37 schrieb Eric Faurot:
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: smtpd: unnecessary "no certificate presented" log message
On Wed, 30 Jun 2021 14:37:44 +0200, Eric Faurot wrote: > 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. OK millert@ - todd
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
June 22, 2021 11:14 PM, "Aisha Tammy" wrote: >> [...] >> >> WRT to handshake, it has multiple uses ranging from ensuring the addons get >> their configuration >> from the daemon to synchronising the daemon start with addons readiness. >> Remember, we didn’t have this with filters and realised we couldn’t do >> without, it is the same for >> tables, they need to get their “table name” at start so we need the daemon >> to pass them, and we >> also need the daemon to not start using them before they are ready. > > I am unsure what you mean by a handshake. > sure, so let's look at procexec for filters: - when the server starts, it forks the filters and begins a handshake with each of them, emitting the following (for example): config|smtpd-version|6.6.1 config|smtp-session-timeout|300 config|subsystem|smtp-in config|ready - when the filter receives the last line, it knows the server is done and it is its turn, it emits the following: register|report|smtp-in|link-connect register|ready - at this point the handshake is over, the server is ready and the filter is ready too, they are both in a known state and synchronised before data flows to the filter. The procexec tables should have a similar handshake to allow the server to pass them information and make sure they are synchronised. The only difference with filters would be that table would have a different "register"" line in the handshake but the config part should be similar. This would an addon to implement both a filter and a table by registering for filtering, and providing a lookup backend for example. > Would something like the following be good - on exec the table process has to > print out a string > "TABLE-READY" which ensures us that the process is ready. > Almost, on exec the daemon prints the config bits (exactly like it does for filter), then it reads from the table backend for a register|ready, but yes you have the idea. > I am not exactly sure how I would implement this, my guess would be to use > event(3) with EV_READ on > backend_r (or something like that). > I haven't looked at this code in over a year now so I'm not sure what the right way of doing it is on top of my head, but looking at how filters are handled is a good startint point.
Re: add table_procexec in smtpd
> >> First, if the two implementations are not going to coexists,
> >> we can just replace table_proc.c.
> >
> > True, though proc-exec was the name used for filters so it may be a good to
> > unify and drop the legacy “proc” name.
> > This will be hidden to users so quite frankly it’s a matter of dev
> > preference.
I've kept procexec for now.
> >
> >> Second, since the goal is to expose the protocol directly, instead of
> >> relying on wrappers (through opensmtpd-extras api), we should take
> >> time to refine it properly before, and avoid having to bump it every
> >> other day. For example, I don't see the point of adding timestamps in
> >> the request. Do we want to be case-sensitive on the commands? Do we
> >> need some kind of handshake? Also, there can be long-term plan to use
> >> the same process for different tables, or even for other backends.
> >
> > I’m unsure I understand your point:
> >
> > The protocol is based on the filter protocol, follows the same logic and
> > line header to solve the same issues, this is precisely so that there’s no
> > need to bump every other day as we already figured what was needed for
> > third party adding to interoperate with smtpd.
> > This also has the advantage that you can have a single parser handle these
> > different API instead of having a filter protocol parser, a table protocol
> > parser (and maybe in the future a queue protocol parser… etc).
> >
> > WRT timestamps there were many uses for them ranging from timeout detection
> > both in daemon / add-ons, profiling, logging the time an event was
> > generated vs processes overhead, etc…
> > It allowed ensuring that all addons handling the same event have the exact
> > same timestamp associated to the event.
> >
> > WRT to case-sensitivity, I don’t recall using upper-case myself, to me the
> > protocol is case-sensitive and as far as I can remember I always used
> > lowercase :-/
> > I’m all for lowering case here.
> >
> > WRT to handshake, it has multiple uses ranging from ensuring the addons get
> > their configuration from the daemon to synchronising the daemon start with
> > addons readiness.
> > Remember, we didn’t have this with filters and realised we couldn’t do
> > without, it is the same for tables, they need to get their “table name” at
> > start so we need the daemon to pass them, and we also need the daemon to
> > not start using them before they are ready.
> >
I am unsure what you mean by a handshake.
Would something like the following be good - on exec the table process has to
print out a string "TABLE-READY" which ensures us that the process is ready.
I am not exactly sure how I would implement this, my guess would be to use
event(3) with EV_READ on backend_r (or something like that).
> >
> >> Finally the implementation could be factorized a bit, but that's a
> >> detail at this time. I think the close operation (is it really useful
> >> anyway?)
> >> should use fclose() instead of kill(), and maybe wait() too?
I've moved it to fclose(), I've not used wait() for now.
> >
> > The implementation can probably be improved, this was a PoC that allowed me
> > to write various table backends but it was never meant to be committed in
> > the first place.
>
If you can clarify what the handshake means, that would be nice.
Cheers,
Aisha
diff --git a/usr.sbin/smtpd/parse.y b/usr.sbin/smtpd/parse.y
index 011e306ac61..1b0ee5ad38f 100644
--- a/usr.sbin/smtpd/parse.y
+++ b/usr.sbin/smtpd/parse.y
@@ -2557,13 +2557,6 @@ table: TABLE STRING STRING {
config = p+1;
}
}
- if (config != NULL && *config != '/') {
- yyerror("invalid backend parameter for table:
%s",
- $2);
- free($2);
- free($3);
- YYERROR;
- }
table = table_create(conf, backend, $2, config);
if (!table_config(table)) {
yyerror("invalid configuration file %s for
table %s",
diff --git a/usr.sbin/smtpd/smtpctl/Makefile b/usr.sbin/smtpd/smtpctl/Makefile
index ef8148be8c9..46831d647dc 100644
--- a/usr.sbin/smtpd/smtpctl/Makefile
+++ b/usr.sbin/smtpd/smtpctl/Makefile
@@ -47,7 +47,7 @@ SRCS+=table.c
SRCS+= table_static.c
SRCS+= table_db.c
SRCS+= table_getpwnam.c
-SRCS+= table_proc.c
+SRCS+= table_procexec.c
Re: add table_procexec in smtpd
Re-sending, I forgot to cc: aisha & tech: > On 12 Jun 2021, at 22:47, Gilles CHEHADE wrote: > >> >> On 12 Jun 2021, at 15:15, Eric Faurot wrote: >> >> On Wed, Jun 09, 2021 at 05:41:36PM -0400, Aisha Tammy wrote: >>> Hi, >>> Here is the updated diff, which removes table_proc and adds table_procexec >>> as the default backend when no backend name matches. >>> >> >> Hi. >> >> I'm not opposed to the idea, but I have a couple of comments: >> >> First, if the two implementations are not going to coexists, >> we can just replace table_proc.c. > > True, though proc-exec was the name used for filters so it may be a good to > unify and drop the legacy “proc” name. > This will be hidden to users so quite frankly it’s a matter of dev preference. > > >> Second, since the goal is to expose the protocol directly, instead of >> relying on wrappers (through opensmtpd-extras api), we should take >> time to refine it properly before, and avoid having to bump it every >> other day. For example, I don't see the point of adding timestamps in >> the request. Do we want to be case-sensitive on the commands? Do we >> need some kind of handshake? Also, there can be long-term plan to use >> the same process for different tables, or even for other backends. > > I’m unsure I understand your point: > > The protocol is based on the filter protocol, follows the same logic and line > header to solve the same issues, this is precisely so that there’s no need to > bump every other day as we already figured what was needed for third party > adding to interoperate with smtpd. > This also has the advantage that you can have a single parser handle these > different API instead of having a filter protocol parser, a table protocol > parser (and maybe in the future a queue protocol parser… etc). > > WRT timestamps there were many uses for them ranging from timeout detection > both in daemon / add-ons, profiling, logging the time an event was generated > vs processes overhead, etc… > It allowed ensuring that all addons handling the same event have the exact > same timestamp associated to the event. > > WRT to case-sensitivity, I don’t recall using upper-case myself, to me the > protocol is case-sensitive and as far as I can remember I always used > lowercase :-/ > I’m all for lowering case here. > > WRT to handshake, it has multiple uses ranging from ensuring the addons get > their configuration from the daemon to synchronising the daemon start with > addons readiness. > Remember, we didn’t have this with filters and realised we couldn’t do > without, it is the same for tables, they need to get their “table name” at > start so we need the daemon to pass them, and we also need the daemon to not > start using them before they are ready. > > >> Finally the implementation could be factorized a bit, but that's a >> detail at this time. I think the close operation (is it really useful >> anyway?) >> should use fclose() instead of kill(), and maybe wait() too? > > The implementation can probably be improved, this was a PoC that allowed me > to write various table backends but it was never meant to be committed in the > first place.
Re: add table_procexec in smtpd
> On 12 Jun 2021, at 18:57, Aisha Tammy wrote: > > On 6/12/21 9:15 AM, Eric Faurot wrote: >> On Wed, Jun 09, 2021 at 05:41:36PM -0400, Aisha Tammy wrote: >>> Hi, >>> Here is the updated diff, which removes table_proc and adds >>> table_procexec as the default backend when no backend name matches. >>> >> Hi. >> >> I'm not opposed to the idea, but I have a couple of comments: >> >> First, if the two implementations are not going to coexists, >> we can just replace table_proc.c. > Sounds good with me :D >> >> Second, since the goal is to expose the protocol directly, instead of >> relying on wrappers (through opensmtpd-extras api), we should take >> time to refine it properly before, and avoid having to bump it every >> other day. For example, I don't see the point of adding timestamps in >> the request. > My WIP LDAP wrapper does not use the timestamps. I have not been privy to the > historical context of the development of this protocol, so I do not know why > it exists. > I am fine with removing it. I’m unsure removing them would be a good idea, not only it wouldn’t bring any benefit but it would prevent tracking the time an event was generated in the daemon from a table backend and would make the protocol diverge from the filters protocol while they are the same protocol as of today and could use the same parser with different handler functions based on the event field. >> Do we want to be case-sensitive on the commands? > I reused the current table_service_name function present in table.c and hence > set it to all-caps. I think keeping it such is not an issue but I don't care > if we move to lowercase. > I would prefer being case sensitive, so as not to overly-complicate our > checks. I agree. >> Do we need some kind of handshake? > I do not see a need for this between the table-open and the table-process. > The table-process may do handshakes (or whatever) in its backend, which we > should not be worrying about. Maybe I misunderstood but the handshake is very much needed between the daemon and non-builtin tables, similarly to filters. This is what guarantees that the daemon doesn’t start using a table that’s not ready but also what allows passing daemon informations to table backends so they build their initial state. >> Also, there can be long-term plan to use >> the same process for different tables, or even for other backends. >> >> Finally the implementation could be factorized a bit, but that's a >> detail at this time. I think the close operation (is it really useful >> anyway?) >> should use fclose() instead of kill(), and maybe wait() too? > I agree, I am not sure if the table-close is very useful. I am also fine with > moving it to fclose, though I don't know how to manually close a table >.< > > BTW, can I remove the table_check function? I haven't seen a use for it even > after scrounging around a bit. > > Unless any comments, I'll send the next patch (soonish) with fclose and > timestamps, table_check removed. >
Re: add table_procexec in smtpd
On 6/12/21 9:15 AM, Eric Faurot wrote: On Wed, Jun 09, 2021 at 05:41:36PM -0400, Aisha Tammy wrote: Hi, Here is the updated diff, which removes table_proc and adds table_procexec as the default backend when no backend name matches. Hi. I'm not opposed to the idea, but I have a couple of comments: First, if the two implementations are not going to coexists, we can just replace table_proc.c. Sounds good with me :D Second, since the goal is to expose the protocol directly, instead of relying on wrappers (through opensmtpd-extras api), we should take time to refine it properly before, and avoid having to bump it every other day. For example, I don't see the point of adding timestamps in the request. My WIP LDAP wrapper does not use the timestamps. I have not been privy to the historical context of the development of this protocol, so I do not know why it exists. I am fine with removing it. Do we want to be case-sensitive on the commands? I reused the current table_service_name function present in table.c and hence set it to all-caps. I think keeping it such is not an issue but I don't care if we move to lowercase. I would prefer being case sensitive, so as not to overly-complicate our checks. Do we need some kind of handshake? I do not see a need for this between the table-open and the table-process. The table-process may do handshakes (or whatever) in its backend, which we should not be worrying about. Also, there can be long-term plan to use the same process for different tables, or even for other backends. Finally the implementation could be factorized a bit, but that's a detail at this time. I think the close operation (is it really useful anyway?) should use fclose() instead of kill(), and maybe wait() too? I agree, I am not sure if the table-close is very useful. I am also fine with moving it to fclose, though I don't know how to manually close a table >.< BTW, can I remove the table_check function? I haven't seen a use for it even after scrounging around a bit. Unless any comments, I'll send the next patch (soonish) with fclose and timestamps, table_check removed. Cheers, Aisha Eric.
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: add table_procexec in smtpd
Hi,
Here is the updated diff, which removes table_proc and adds table_procexec as
the default backend when no backend name matches.
With this diff, I have the following configuration for smtpd:
# $OpenBSD: smtpd.conf,v 1.14 2019/11/26 20:14:38 gilles Exp $
# This is the smtpd server system-wide configuration file.
# See smtpd.conf(5) for more information.
table aliases aliases:root.t...@bsd.ac
listen on socket
# To accept external mail, replace with: listen on all
#
listen on lo0
action "local_mail" mbox alias
action "outbound" relay
action "bsd.ac" relay host smtp://10.7.0.1
# Uncomment the following to accept external mail for domain "example.org"
#
# match from any for domain "example.org" action "local_mail"
match from local for local action "local_mail"
match from local for domain "bsd.ac" action "bsd.ac"
match from local for any action "outbound"
where my /usr/local/libexec/smtpd/table-aliases contains:
#!/bin/ksh
user="${1:-r...@bsd.ac}"
while read line
do
reqid="$(echo $line | awk -F'|' '{ print $5; }')"
reply="TABLE-RESULT|$reqid|FOUND|$user"
echo $reply
done < /dev/stdin
exit 0
This should hopefully satisfy the requirements for transparency and sanity.
I will work on the opensmtpd-extras and make a PR in the github separately, if
that sounds fine.
Cheers,
Aisha
diff --git a/usr.sbin/smtpd/parse.y b/usr.sbin/smtpd/parse.y
index 011e306ac61..1b0ee5ad38f 100644
--- a/usr.sbin/smtpd/parse.y
+++ b/usr.sbin/smtpd/parse.y
@@ -2557,13 +2557,6 @@ table: TABLE STRING STRING {
config = p+1;
}
}
- if (config != NULL && *config != '/') {
- yyerror("invalid backend parameter for table:
%s",
- $2);
- free($2);
- free($3);
- YYERROR;
- }
table = table_create(conf, backend, $2, config);
if (!table_config(table)) {
yyerror("invalid configuration file %s for
table %s",
diff --git a/usr.sbin/smtpd/smtpctl/Makefile b/usr.sbin/smtpd/smtpctl/Makefile
index ef8148be8c9..46831d647dc 100644
--- a/usr.sbin/smtpd/smtpctl/Makefile
+++ b/usr.sbin/smtpd/smtpctl/Makefile
@@ -47,7 +47,7 @@ SRCS+=table.c
SRCS+= table_static.c
SRCS+= table_db.c
SRCS+= table_getpwnam.c
-SRCS+= table_proc.c
+SRCS+= table_procexec.c
SRCS+= unpack_dns.c
SRCS+= spfwalk.c
diff --git a/usr.sbin/smtpd/smtpd.h b/usr.sbin/smtpd/smtpd.h
index be934112103..221f24fbdc4 100644
--- a/usr.sbin/smtpd/smtpd.h
+++ b/usr.sbin/smtpd/smtpd.h
@@ -1656,6 +1656,7 @@ int table_regex_match(const char *, const char *);
void table_open_all(struct smtpd *);
void table_dump_all(struct smtpd *);
void table_close_all(struct smtpd *);
+const char *table_service_name(enum table_service );
/* to.c */
diff --git a/usr.sbin/smtpd/smtpd/Makefile b/usr.sbin/smtpd/smtpd/Makefile
index b31d4e42224..64e73c3bb70 100644
--- a/usr.sbin/smtpd/smtpd/Makefile
+++ b/usr.sbin/smtpd/smtpd/Makefile
@@ -62,7 +62,7 @@ SRCS+=compress_gzip.c
SRCS+= table_db.c
SRCS+= table_getpwnam.c
-SRCS+= table_proc.c
+SRCS+= table_procexec.c
SRCS+= table_static.c
SRCS+= queue_fs.c
diff --git a/usr.sbin/smtpd/table.c b/usr.sbin/smtpd/table.c
index 1d82d88b81a..a09229ca174 100644
--- a/usr.sbin/smtpd/table.c
+++ b/usr.sbin/smtpd/table.c
@@ -45,9 +45,8 @@ struct table_backend *table_backend_lookup(const char *);
extern struct table_backend table_backend_static;
extern struct table_backend table_backend_db;
extern struct table_backend table_backend_getpwnam;
-extern struct table_backend table_backend_proc;
+extern struct table_backend table_backend_procexec;
-static const char * table_service_name(enum table_service);
static int table_parse_lookup(enum table_service, const char *, const char *,
union lookup *);
static int parse_sockaddr(struct sockaddr *, int, const char *);
@@ -58,7 +57,7 @@ static struct table_backend *backends[] = {
_backend_static,
_backend_db,
_backend_getpwnam,
- _backend_proc,
+ _backend_procexec,
NULL
};
@@ -77,7 +76,7 @@ table_backend_lookup(const char *backend)
return NULL;
}
-static const char *
+const char *
table_service_name(enum table_service s)
{
switch (s) {
@@ -208,10 +207,9 @@ table_create(struct smtpd *conf, const char *backend,
const char *name,
PATH_LIBEXEC"/table-%s\"", backend);
}
if (stat(path, ) == 0) {
- tb = table_backend_lookup
