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:
+ 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 + d5cbe88feeaf9e91eca119b31dda957aca68c031
--- usr.sbin/smtpd/smtpd.h
+++ usr.sbin/smtpd/smtpd.h
@@
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 {
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).
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
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
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.
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
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
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 - 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 - 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:30 -
@@ -1157,7 +1157,7 @@ mta_response(struct mta_session *s, char
s->rcptcount = 0;
if (s->relay->limits->sessdelay_transaction) {
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/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 06:59:29 -
@@ -171,7 +171,7 @@ maildir_engine(const char *dirname, int
(void)strlcpy(hostname, "localhost", sizeof
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 - 1.16
+++ usr.sbin/smtpd/mail.maildir.c 15 May 2023 06:59:29 -
@@ -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(),
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
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
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)));
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)));
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_V_ERR_CERT_NOT_YET_VALID:
-case X509_V_ERR_ERROR_IN_CERT_NOT_BEFORE_FIELD:
- break;
-case X509_V_ERR_CERT_HAS_EXPIRED:
-case
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.
>
>
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 :)
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
- ret = table_lookup(table, K_RELAYHOST, domain,
);
+ ret = table_lookup(table, K_RELAYHOST, domain,
NULL, );
if (ret == -1)
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
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
Re: smtpd: includes cleanup
Hi. Slightly updated diff, including sys/tree.h in smtpd.h. Eric. Index: aliases.c === RCS file: /cvs/src/usr.sbin/smtpd/aliases.c,v retrieving revision 1.78 diff -u -p -r1.78 aliases.c --- aliases.c 28 Apr 2020 21:46:43 - 1.78 +++ aliases.c 26 May 2021 20:15:02 - @@ -16,19 +16,8 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include - -#include -#include -#include -#include -#include #include #include -#include #include #include "smtpd.h" Index: bounce.c === RCS file: /cvs/src/usr.sbin/smtpd/bounce.c,v retrieving revision 1.84 diff -u -p -r1.84 bounce.c --- bounce.c26 May 2021 18:08:55 - 1.84 +++ bounce.c26 May 2021 20:14:41 - @@ -18,23 +18,11 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include - #include -#include -#include #include -#include -#include -#include #include #include -#include #include -#include #include "smtpd.h" #include "log.h" Index: ca.c === RCS file: /cvs/src/usr.sbin/smtpd/ca.c,v retrieving revision 1.39 diff -u -p -r1.39 ca.c --- ca.c26 May 2021 18:08:55 - 1.39 +++ ca.c27 May 2021 10:57:24 - @@ -17,26 +17,12 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include - -#include -#include +#include +#include #include #include -#include #include #include - -#include -#include -#include -#include -#include -#include -#include #include "smtpd.h" #include "log.h" Index: compress_backend.c === RCS file: /cvs/src/usr.sbin/smtpd/compress_backend.c,v retrieving revision 1.9 diff -u -p -r1.9 compress_backend.c --- compress_backend.c 20 Jan 2015 17:37:54 - 1.9 +++ compress_backend.c 26 May 2021 20:13:39 - @@ -17,18 +17,7 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include -#include - -#include -#include -#include #include -#include -#include #include "smtpd.h" Index: compress_gzip.c === RCS file: /cvs/src/usr.sbin/smtpd/compress_gzip.c,v retrieving revision 1.12 diff -u -p -r1.12 compress_gzip.c --- compress_gzip.c 26 May 2021 18:08:55 - 1.12 +++ compress_gzip.c 27 May 2021 11:01:41 - @@ -17,27 +17,10 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include -#include - -#include -#include -#include -#include -#include #include -#include -#include -#include - #include #include "smtpd.h" -#include "log.h" - #defineGZIP_BUFFER_SIZE16384 Index: config.c === RCS file: /cvs/src/usr.sbin/smtpd/config.c,v retrieving revision 1.56 diff -u -p -r1.56 config.c --- config.c26 May 2021 07:05:50 - 1.56 +++ config.c26 May 2021 20:13:14 - @@ -16,23 +16,11 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include #include -#include #include -#include -#include -#include #include -#include #include -#include - -#include #include "smtpd.h" #include "log.h" Index: control.c === RCS file: /cvs/src/usr.sbin/smtpd/control.c,v retrieving revision 1.127 diff -u -p -r1.127 control.c --- control.c 26 May 2021 18:08:55 - 1.127 +++ control.c 26 May 2021 20:12:43 - @@ -18,25 +18,15 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include #include -#include #include #include -#include -#include -#include #include #include -#include #include #include -#include #include -#include #include "smtpd.h" #include "log.h" Index: crypto.c === RCS file: /cvs/src/usr.sbin/smtpd/crypto.c,v retrieving revision 1.9 diff -u -p -r1.9 crypto.c --- crypto.c23 Jan 2021 16:11:11 - 1.9 +++ crypto.c27 May 2021 11:01:30 - @@ -16,14 +16,10 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include #include -#include -#include - #include - +#include #defineCRYPTO_BUFFER_SIZE 16384 Index: dict.c === RCS file: /cvs/src/usr.sbin/smtpd/dict.c,v retrieving revision 1.7 diff -u -p -r1.7 dict.c --- dict.c 26 May 2021 18:08:55 - 1.7 +++ dict.c 26 May 2021 20:10:54
Re: smtpd: includes cleanup
On Thu, May 27, 2021 at 08:13:36AM -0600, Todd C. Miller wrote: > On Thu, 27 May 2021 13:14:30 +0200, Eric Faurot wrote: > > > New diff with small tweaks. > > It looks like you are relying on sys/queue.h being included implicitly. > Since smtpd.h uses the TAILQ macros, should it include sys/queue.h > itself? That's reasonnable. I'm adding it. Eric.
Re: smtpd: includes cleanup
On Thu, 27 May 2021 13:14:30 +0200, Eric Faurot wrote: > New diff with small tweaks. It looks like you are relying on sys/queue.h being included implicitly. Since smtpd.h uses the TAILQ macros, should it include sys/queue.h itself? - todd
Re: smtpd: includes cleanup
New diff with small tweaks. Eric. Index: aliases.c === RCS file: /cvs/src/usr.sbin/smtpd/aliases.c,v retrieving revision 1.78 diff -u -p -r1.78 aliases.c --- aliases.c 28 Apr 2020 21:46:43 - 1.78 +++ aliases.c 26 May 2021 20:15:02 - @@ -16,19 +16,8 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include - -#include -#include -#include -#include -#include #include #include -#include #include #include "smtpd.h" Index: bounce.c === RCS file: /cvs/src/usr.sbin/smtpd/bounce.c,v retrieving revision 1.84 diff -u -p -r1.84 bounce.c --- bounce.c26 May 2021 18:08:55 - 1.84 +++ bounce.c26 May 2021 20:14:41 - @@ -18,23 +18,11 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include - #include -#include -#include #include -#include -#include -#include #include #include -#include #include -#include #include "smtpd.h" #include "log.h" Index: ca.c === RCS file: /cvs/src/usr.sbin/smtpd/ca.c,v retrieving revision 1.39 diff -u -p -r1.39 ca.c --- ca.c26 May 2021 18:08:55 - 1.39 +++ ca.c27 May 2021 10:57:24 - @@ -17,26 +17,12 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include - -#include -#include +#include +#include #include #include -#include #include #include - -#include -#include -#include -#include -#include -#include -#include #include "smtpd.h" #include "log.h" Index: compress_backend.c === RCS file: /cvs/src/usr.sbin/smtpd/compress_backend.c,v retrieving revision 1.9 diff -u -p -r1.9 compress_backend.c --- compress_backend.c 20 Jan 2015 17:37:54 - 1.9 +++ compress_backend.c 26 May 2021 20:13:39 - @@ -17,18 +17,7 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include -#include - -#include -#include -#include #include -#include -#include #include "smtpd.h" Index: compress_gzip.c === RCS file: /cvs/src/usr.sbin/smtpd/compress_gzip.c,v retrieving revision 1.12 diff -u -p -r1.12 compress_gzip.c --- compress_gzip.c 26 May 2021 18:08:55 - 1.12 +++ compress_gzip.c 27 May 2021 11:01:41 - @@ -17,27 +17,10 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include -#include - -#include -#include -#include -#include -#include #include -#include -#include -#include - #include #include "smtpd.h" -#include "log.h" - #defineGZIP_BUFFER_SIZE16384 Index: config.c === RCS file: /cvs/src/usr.sbin/smtpd/config.c,v retrieving revision 1.56 diff -u -p -r1.56 config.c --- config.c26 May 2021 07:05:50 - 1.56 +++ config.c26 May 2021 20:13:14 - @@ -16,23 +16,11 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include #include -#include #include -#include -#include -#include #include -#include #include -#include - -#include #include "smtpd.h" #include "log.h" Index: control.c === RCS file: /cvs/src/usr.sbin/smtpd/control.c,v retrieving revision 1.127 diff -u -p -r1.127 control.c --- control.c 26 May 2021 18:08:55 - 1.127 +++ control.c 26 May 2021 20:12:43 - @@ -18,25 +18,15 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include #include -#include #include #include -#include -#include -#include #include #include -#include #include #include -#include #include -#include #include "smtpd.h" #include "log.h" Index: crypto.c === RCS file: /cvs/src/usr.sbin/smtpd/crypto.c,v retrieving revision 1.9 diff -u -p -r1.9 crypto.c --- crypto.c23 Jan 2021 16:11:11 - 1.9 +++ crypto.c27 May 2021 11:01:30 - @@ -16,14 +16,10 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include #include -#include -#include - #include - +#include #defineCRYPTO_BUFFER_SIZE 16384 Index: dict.c === RCS file: /cvs/src/usr.sbin/smtpd/dict.c,v retrieving revision 1.7 diff -u -p -r1.7 dict.c --- dict.c 26 May 2021 18:08:55 - 1.7 +++ dict.c 26 May 2021 20:10:54 - @@ -17,12 +17,10 @@ * OR
Re: smtpd: err/errx -> fatal/fatalx
On Wed, 26 May 2021 16:24:42 +0200, Eric Faurot wrote: > This diff replaces calls to err(3)/errx(3) with fatal()/fatalx() from > log.c for code that runs in the deamon (we want errors logged to > syslog, not stderr). It's pretty mechanical, with two things to note: > > The call to default_config() has to be done after log_init() in smtpd.c, > and log_init() is called early in smtpctl.c, since it uses files (iobuf.c) > that uses the log api. It still logs to stderr though. OK millert@ - todd
Re: smtpd: unused code
On Tue, 25 May 2021 22:50:32 +0200, Eric Faurot wrote: > This diff removes more unused code. OK millert@ - todd
Re: smtpd: remove tls_accept/tls_connect callbacks
On Wed, Apr 28, 2021 at 02:27:11PM +0200, Eric Faurot wrote: > On Wed, Apr 21, 2021 at 11:21:51AM +0200, Eric Faurot wrote: > > There is actually no reason to defer calls to tls_accept_socket() and > > tls_connect_socket() in an event callback. The code can be simplified > > by a great deal. It also eliminates the issue of keeping a reference > > to the listener tls context in the io structure. > > Did anyone had a chance to look at it? ok tb
Re: smtpd: remove tls_accept/tls_connect callbacks
On Wed, Apr 21, 2021 at 11:21:51AM +0200, Eric Faurot wrote:
> There is actually no reason to defer calls to tls_accept_socket() and
> tls_connect_socket() in an event callback. The code can be simplified
> by a great deal. It also eliminates the issue of keeping a reference
> to the listener tls context in the io structure.
Did anyone had a chance to look at it?
> Eric.
>
>
> Index: ioev.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/ioev.c,v
> retrieving revision 1.45
> diff -u -p -r1.45 ioev.c
> --- ioev.c5 Apr 2021 15:50:11 - 1.45
> +++ ioev.c21 Apr 2021 08:35:29 -
> @@ -64,7 +64,6 @@ struct io {
> int state;
> struct event ev;
> struct tls *tls;
> - char*name;
>
> const char *error; /* only valid immediately on callback */
> };
> @@ -280,7 +279,6 @@ io_free(struct io *io)
> io->sock = -1;
> }
>
> - free(io->name);
> iobuf_clear(>iobuf);
> free(io);
> }
> @@ -817,14 +815,14 @@ io_connect_tls(struct io *io, struct tls
> if (io->tls)
> errx(1, "io_connect_tls: TLS already started");
>
> - if (hostname) {
> - if ((io->name = strdup(hostname)) == NULL)
> - err(1, "io_connect_tls");
> + if (tls_connect_socket(tls, io->sock, hostname) == -1) {
> + io->error = tls_error(tls);
> + return (-1);
> }
>
> io->tls = tls;
> io->state = IO_STATE_CONNECT_TLS;
> - io_reset(io, EV_WRITE, io_dispatch_connect_tls);
> + io_reset(io, EV_READ|EV_WRITE, io_dispatch_handshake_tls);
>
> return (0);
> }
> @@ -840,9 +838,14 @@ io_accept_tls(struct io *io, struct tls
>
> if (io->tls)
> errx(1, "io_accept_tls: TLS already started");
> - io->tls = tls;
> +
> + if (tls_accept_socket(tls, >tls, io->sock) == -1) {
> + io->error = tls_error(tls);
> + return (-1);
> + }
> +
> io->state = IO_STATE_ACCEPT_TLS;
> - io_reset(io, EV_READ, io_dispatch_accept_tls);
> + io_reset(io, EV_READ|EV_WRITE, io_dispatch_handshake_tls);
>
> return (0);
> }
> @@ -880,60 +883,6 @@ io_dispatch_handshake_tls(int fd, short
> }
>
> void
> -io_dispatch_accept_tls(int fd, short event, void *humppa)
> -{
> - struct io *io = humppa;
> - struct tls *tls = io->tls;
> - int ret;
> -
> - io_frame_enter("io_dispatch_accept_tls", io, event);
> -
> - /* Replaced by TLS context for accepted socket on success. */
> - io->tls = NULL;
> -
> - if (event == EV_TIMEOUT) {
> - io_callback(io, IO_TIMEOUT);
> - goto leave;
> - }
> -
> - if ((ret = tls_accept_socket(tls, >tls, io->sock)) == 0) {
> - io_reset(io, EV_READ|EV_WRITE, io_dispatch_handshake_tls);
> - goto leave;
> - }
> - io->error = tls_error(tls);
> - io_callback(io, IO_ERROR);
> -
> - leave:
> - io_frame_leave(io);
> - return;
> -}
> -
> -void
> -io_dispatch_connect_tls(int fd, short event, void *humppa)
> -{
> - struct io *io = humppa;
> - int ret;
> -
> - io_frame_enter("io_dispatch_connect_tls", io, event);
> -
> - if (event == EV_TIMEOUT) {
> - io_callback(io, IO_TIMEOUT);
> - goto leave;
> - }
> -
> - if ((ret = tls_connect_socket(io->tls, io->sock, io->name)) == 0) {
> - io_reset(io, EV_READ|EV_WRITE, io_dispatch_handshake_tls);
> - goto leave;
> - }
> -
> - io->error = tls_error(io->tls);
> - io_callback(io, IO_ERROR);
> -
> - leave:
> - io_frame_leave(io);
> -}
> -
> -void
> io_dispatch_read_tls(int fd, short event, void *humppa)
> {
> struct io *io = humppa;
> @@ -1017,37 +966,20 @@ io_dispatch_write_tls(int fd, short even
> void
> io_reload_tls(struct io *io)
> {
> - short ev = 0;
> - void(*dispatch)(int, short, void*) = NULL;
> -
> - switch (io->state) {
> - case IO_STATE_CONNECT_TLS:
> - ev = EV_WRITE;
> - dispatch = io_dispatch_connect_tls;
> - break;
> - case IO_STATE_ACCEPT_TLS:
> - ev = EV_READ;
> - dispatch = io_dispatch_accept_tls;
> - break;
> - case IO_STATE_UP:
> - ev = 0;
> - if (IO_READING(io) && !(io->flags & IO_PAUSE_IN)) {
> - ev = EV_READ;
> - dispatch = io_dispatch_read_tls;
> - }
> - else if (IO_WRITING(io) && !(io->flags & IO_PAUSE_OUT) &&
> - io_queued(io)) {
> - ev = EV_WRITE;
> - dispatch = io_dispatch_write_tls;
> - }
> - if (!ev)
> - return; /* paused */
> - break;
> - default:
> + if (io->state != IO_STATE_UP)
>
Re: smtpd: more unused code
On Tue, 20 Apr 2021 18:38:08 +0200, Eric Faurot wrote: > On Sun, Apr 11, 2021 at 01:54:32PM +0200, Eric Faurot wrote: > > Certificate verification is done by libtls. The former code is not used > > anymore and can be unplugged. > > Anyone willing to ok this? OK millert@ - todd
Re: smtpd: more unused code
On Sun, Apr 11, 2021 at 01:54:32PM +0200, Eric Faurot wrote:
> Certificate verification is done by libtls. The former code is not used
> anymore and can be unplugged.
Anyone willing to ok this?
> Eric.
>
> Index: dispatcher.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/dispatcher.c,v
> retrieving revision 1.2
> diff -u -p -r1.2 dispatcher.c
> --- dispatcher.c 5 Mar 2021 12:37:32 - 1.2
> +++ dispatcher.c 11 Apr 2021 11:46:17 -
> @@ -64,11 +64,6 @@ dispatcher_imsg(struct mproc *p, struct
> resolver_dispatch_result(p, imsg);
> return;
>
> - case IMSG_CERT_INIT:
> - case IMSG_CERT_VERIFY:
> - cert_dispatch_result(p, imsg);
> - return;
> -
> case IMSG_CONF_START:
> return;
> case IMSG_CONF_END:
> Index: lka.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/lka.c,v
> retrieving revision 1.244
> diff -u -p -r1.244 lka.c
> --- lka.c 31 Dec 2020 08:27:15 - 1.244
> +++ lka.c 11 Apr 2021 11:45:24 -
> @@ -111,12 +111,6 @@ lka_imsg(struct mproc *p, struct imsg *i
> resolver_dispatch_request(p, imsg);
> return;
>
> - case IMSG_CERT_INIT:
> - case IMSG_CERT_CERTIFICATE:
> - case IMSG_CERT_VERIFY:
> - cert_dispatch_request(p, imsg);
> - return;
> -
> case IMSG_MTA_DNS_HOST:
> case IMSG_MTA_DNS_MX:
> case IMSG_MTA_DNS_MX_PREFERENCE:
> Index: smtpd.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.c,v
> retrieving revision 1.337
> diff -u -p -r1.337 smtpd.c
> --- smtpd.c 5 Mar 2021 12:37:32 - 1.337
> +++ smtpd.c 11 Apr 2021 11:46:38 -
> @@ -2003,10 +2003,6 @@ imsg_to_str(int type)
> CASE(IMSG_GETNAMEINFO);
> CASE(IMSG_RES_QUERY);
>
> - CASE(IMSG_CERT_INIT);
> - CASE(IMSG_CERT_CERTIFICATE);
> - CASE(IMSG_CERT_VERIFY);
> -
> CASE(IMSG_SETUP_KEY);
> CASE(IMSG_SETUP_PEER);
> CASE(IMSG_SETUP_DONE);
> Index: smtpd.h
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.h,v
> retrieving revision 1.667
> diff -u -p -r1.667 smtpd.h
> --- smtpd.h 11 Apr 2021 07:18:08 - 1.667
> +++ smtpd.h 11 Apr 2021 11:45:58 -
> @@ -102,12 +102,6 @@
> #define P_NEWALIASES 1
> #define P_MAKEMAP2
>
> -#define CERT_ERROR -1
> -#define CERT_OK 0
> -#define CERT_NOCA1
> -#define CERT_NOCERT 2
> -#define CERT_INVALID 3
> -
> struct userinfo {
> char username[SMTPD_VUSERNAME_SIZE];
> char directory[PATH_MAX];
> @@ -211,10 +205,6 @@ enum imsg_type {
> IMSG_GETNAMEINFO,
> IMSG_RES_QUERY,
>
> - IMSG_CERT_INIT,
> - IMSG_CERT_CERTIFICATE,
> - IMSG_CERT_VERIFY,
> -
> IMSG_SETUP_KEY,
> IMSG_SETUP_PEER,
> IMSG_SETUP_DONE,
> @@ -1281,14 +1271,6 @@ int ca_X509_verify(void *, void *, cons
> void ca_imsg(struct mproc *, struct imsg *);
> void ca_init(void);
> void ca_engine_init(void);
> -
> -
> -/* cert.c */
> -int cert_init(const char *, int,
> -void (*)(void *, int, const char *, const void *, size_t), void *);
> -int cert_verify(const void *, const char *, int, void (*)(void *, int), void
> *);
> -void cert_dispatch_request(struct mproc *, struct imsg *);
> -void cert_dispatch_result(struct mproc *, struct imsg *);
>
>
> /* compress_backend.c */
> Index: smtpd/Makefile
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtpd/Makefile,v
> retrieving revision 1.112
> diff -u -p -r1.112 Makefile
> --- smtpd/Makefile11 Apr 2021 07:18:08 - 1.112
> +++ smtpd/Makefile11 Apr 2021 11:44:42 -
> @@ -7,7 +7,6 @@ PROG= smtpd
> SRCS=aliases.c
> SRCS+= bounce.c
> SRCS+= ca.c
> -SRCS+= cert.c
> SRCS+= compress_backend.c
> SRCS+= config.c
> SRCS+= control.c
>
>
Re: smtpd: more unused code
On Mon, Apr 12, 2021 at 07:56:57AM -0400, Dave Voutila wrote:
>
> Eric Faurot writes:
>
> > Certificate verification is done by libtls. The former code is not used
> > anymore and can be unplugged.
>
> Should cert.c be removed? I don't think it's used by smtp{d,ctl} or mail
> after your change. Or were you going to commit that seperately?
There are a few files that are not used anymore, My plan was to wait
after the release for actually removing them.
Eric.
>
> >
> > Eric.
> >
> > Index: dispatcher.c
> > ===
> > RCS file: /cvs/src/usr.sbin/smtpd/dispatcher.c,v
> > retrieving revision 1.2
> > diff -u -p -r1.2 dispatcher.c
> > --- dispatcher.c5 Mar 2021 12:37:32 - 1.2
> > +++ dispatcher.c11 Apr 2021 11:46:17 -
> > @@ -64,11 +64,6 @@ dispatcher_imsg(struct mproc *p, struct
> > resolver_dispatch_result(p, imsg);
> > return;
> >
> > - case IMSG_CERT_INIT:
> > - case IMSG_CERT_VERIFY:
> > - cert_dispatch_result(p, imsg);
> > - return;
> > -
> > case IMSG_CONF_START:
> > return;
> > case IMSG_CONF_END:
> > Index: lka.c
> > ===
> > RCS file: /cvs/src/usr.sbin/smtpd/lka.c,v
> > retrieving revision 1.244
> > diff -u -p -r1.244 lka.c
> > --- lka.c 31 Dec 2020 08:27:15 - 1.244
> > +++ lka.c 11 Apr 2021 11:45:24 -
> > @@ -111,12 +111,6 @@ lka_imsg(struct mproc *p, struct imsg *i
> > resolver_dispatch_request(p, imsg);
> > return;
> >
> > - case IMSG_CERT_INIT:
> > - case IMSG_CERT_CERTIFICATE:
> > - case IMSG_CERT_VERIFY:
> > - cert_dispatch_request(p, imsg);
> > - return;
> > -
> > case IMSG_MTA_DNS_HOST:
> > case IMSG_MTA_DNS_MX:
> > case IMSG_MTA_DNS_MX_PREFERENCE:
> > Index: smtpd.c
> > ===
> > RCS file: /cvs/src/usr.sbin/smtpd/smtpd.c,v
> > retrieving revision 1.337
> > diff -u -p -r1.337 smtpd.c
> > --- smtpd.c 5 Mar 2021 12:37:32 - 1.337
> > +++ smtpd.c 11 Apr 2021 11:46:38 -
> > @@ -2003,10 +2003,6 @@ imsg_to_str(int type)
> > CASE(IMSG_GETNAMEINFO);
> > CASE(IMSG_RES_QUERY);
> >
> > - CASE(IMSG_CERT_INIT);
> > - CASE(IMSG_CERT_CERTIFICATE);
> > - CASE(IMSG_CERT_VERIFY);
> > -
> > CASE(IMSG_SETUP_KEY);
> > CASE(IMSG_SETUP_PEER);
> > CASE(IMSG_SETUP_DONE);
> > Index: smtpd.h
> > ===
> > RCS file: /cvs/src/usr.sbin/smtpd/smtpd.h,v
> > retrieving revision 1.667
> > diff -u -p -r1.667 smtpd.h
> > --- smtpd.h 11 Apr 2021 07:18:08 - 1.667
> > +++ smtpd.h 11 Apr 2021 11:45:58 -
> > @@ -102,12 +102,6 @@
> > #define P_NEWALIASES 1
> > #define P_MAKEMAP 2
> >
> > -#defineCERT_ERROR -1
> > -#defineCERT_OK 0
> > -#defineCERT_NOCA1
> > -#defineCERT_NOCERT 2
> > -#defineCERT_INVALID 3
> > -
> > struct userinfo {
> > char username[SMTPD_VUSERNAME_SIZE];
> > char directory[PATH_MAX];
> > @@ -211,10 +205,6 @@ enum imsg_type {
> > IMSG_GETNAMEINFO,
> > IMSG_RES_QUERY,
> >
> > - IMSG_CERT_INIT,
> > - IMSG_CERT_CERTIFICATE,
> > - IMSG_CERT_VERIFY,
> > -
> > IMSG_SETUP_KEY,
> > IMSG_SETUP_PEER,
> > IMSG_SETUP_DONE,
> > @@ -1281,14 +1271,6 @@ int ca_X509_verify(void *, void *, cons
> > voidca_imsg(struct mproc *, struct imsg *);
> > voidca_init(void);
> > voidca_engine_init(void);
> > -
> > -
> > -/* cert.c */
> > -int cert_init(const char *, int,
> > -void (*)(void *, int, const char *, const void *, size_t), void *);
> > -int cert_verify(const void *, const char *, int, void (*)(void *, int),
> > void *);
> > -void cert_dispatch_request(struct mproc *, struct imsg *);
> > -void cert_dispatch_result(struct mproc *, struct imsg *);
> >
> >
> > /* compress_backend.c */
> > Index: smtpd/Makefile
> > ===
> > RCS file: /cvs/src/usr.sbin/smtpd/smtpd/Makefile,v
> > retrieving revision 1.112
> > diff -u -p -r1.112 Makefile
> > --- smtpd/Makefile 11 Apr 2021 07:18:08 - 1.112
> > +++ smtpd/Makefile 11 Apr 2021 11:44:42 -
> > @@ -7,7 +7,6 @@ PROG= smtpd
> > SRCS= aliases.c
> > SRCS+= bounce.c
> > SRCS+= ca.c
> > -SRCS+= cert.c
> > SRCS+= compress_backend.c
> > SRCS+= config.c
> > SRCS+= control.c
>
>
Re: smtpd: more unused code
Eric Faurot writes:
> Certificate verification is done by libtls. The former code is not used
> anymore and can be unplugged.
Should cert.c be removed? I don't think it's used by smtp{d,ctl} or mail
after your change. Or were you going to commit that seperately?
>
> Eric.
>
> Index: dispatcher.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/dispatcher.c,v
> retrieving revision 1.2
> diff -u -p -r1.2 dispatcher.c
> --- dispatcher.c 5 Mar 2021 12:37:32 - 1.2
> +++ dispatcher.c 11 Apr 2021 11:46:17 -
> @@ -64,11 +64,6 @@ dispatcher_imsg(struct mproc *p, struct
> resolver_dispatch_result(p, imsg);
> return;
>
> - case IMSG_CERT_INIT:
> - case IMSG_CERT_VERIFY:
> - cert_dispatch_result(p, imsg);
> - return;
> -
> case IMSG_CONF_START:
> return;
> case IMSG_CONF_END:
> Index: lka.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/lka.c,v
> retrieving revision 1.244
> diff -u -p -r1.244 lka.c
> --- lka.c 31 Dec 2020 08:27:15 - 1.244
> +++ lka.c 11 Apr 2021 11:45:24 -
> @@ -111,12 +111,6 @@ lka_imsg(struct mproc *p, struct imsg *i
> resolver_dispatch_request(p, imsg);
> return;
>
> - case IMSG_CERT_INIT:
> - case IMSG_CERT_CERTIFICATE:
> - case IMSG_CERT_VERIFY:
> - cert_dispatch_request(p, imsg);
> - return;
> -
> case IMSG_MTA_DNS_HOST:
> case IMSG_MTA_DNS_MX:
> case IMSG_MTA_DNS_MX_PREFERENCE:
> Index: smtpd.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.c,v
> retrieving revision 1.337
> diff -u -p -r1.337 smtpd.c
> --- smtpd.c 5 Mar 2021 12:37:32 - 1.337
> +++ smtpd.c 11 Apr 2021 11:46:38 -
> @@ -2003,10 +2003,6 @@ imsg_to_str(int type)
> CASE(IMSG_GETNAMEINFO);
> CASE(IMSG_RES_QUERY);
>
> - CASE(IMSG_CERT_INIT);
> - CASE(IMSG_CERT_CERTIFICATE);
> - CASE(IMSG_CERT_VERIFY);
> -
> CASE(IMSG_SETUP_KEY);
> CASE(IMSG_SETUP_PEER);
> CASE(IMSG_SETUP_DONE);
> Index: smtpd.h
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.h,v
> retrieving revision 1.667
> diff -u -p -r1.667 smtpd.h
> --- smtpd.h 11 Apr 2021 07:18:08 - 1.667
> +++ smtpd.h 11 Apr 2021 11:45:58 -
> @@ -102,12 +102,6 @@
> #define P_NEWALIASES 1
> #define P_MAKEMAP2
>
> -#define CERT_ERROR -1
> -#define CERT_OK 0
> -#define CERT_NOCA1
> -#define CERT_NOCERT 2
> -#define CERT_INVALID 3
> -
> struct userinfo {
> char username[SMTPD_VUSERNAME_SIZE];
> char directory[PATH_MAX];
> @@ -211,10 +205,6 @@ enum imsg_type {
> IMSG_GETNAMEINFO,
> IMSG_RES_QUERY,
>
> - IMSG_CERT_INIT,
> - IMSG_CERT_CERTIFICATE,
> - IMSG_CERT_VERIFY,
> -
> IMSG_SETUP_KEY,
> IMSG_SETUP_PEER,
> IMSG_SETUP_DONE,
> @@ -1281,14 +1271,6 @@ int ca_X509_verify(void *, void *, cons
> void ca_imsg(struct mproc *, struct imsg *);
> void ca_init(void);
> void ca_engine_init(void);
> -
> -
> -/* cert.c */
> -int cert_init(const char *, int,
> -void (*)(void *, int, const char *, const void *, size_t), void *);
> -int cert_verify(const void *, const char *, int, void (*)(void *, int), void
> *);
> -void cert_dispatch_request(struct mproc *, struct imsg *);
> -void cert_dispatch_result(struct mproc *, struct imsg *);
>
>
> /* compress_backend.c */
> Index: smtpd/Makefile
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtpd/Makefile,v
> retrieving revision 1.112
> diff -u -p -r1.112 Makefile
> --- smtpd/Makefile11 Apr 2021 07:18:08 - 1.112
> +++ smtpd/Makefile11 Apr 2021 11:44:42 -
> @@ -7,7 +7,6 @@ PROG= smtpd
> SRCS=aliases.c
> SRCS+= bounce.c
> SRCS+= ca.c
> -SRCS+= cert.c
> SRCS+= compress_backend.c
> SRCS+= config.c
> SRCS+= control.c
Re: smtpd: unused files and dependency
On Sat, Apr 10, 2021 at 02:59:36PM +0200, Eric Faurot wrote:
> Do not build unused files and remove related prototypes.
> Also remove bogus libm dependency.
ok tb
>
> Index: smtpd.h
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.h,v
> retrieving revision 1.666
> diff -u -p -r1.666 smtpd.h
> --- smtpd.h 10 Apr 2021 06:44:18 - 1.666
> +++ smtpd.h 10 Apr 2021 12:34:27 -
> @@ -1638,11 +1638,6 @@ const char *srs_encode(const char *, con
> const char *srs_decode(const char *);
>
>
> -/* ssl_smtpd.c */
> -void *ssl_mta_init(void *, char *, off_t, const char *);
> -void *ssl_smtp_init(void *, int);
> -
> -
> /* stat_backend.c */
> struct stat_backend *stat_backend_lookup(const char *);
> void stat_increment(const char *, size_t);
> Index: ssl.h
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/ssl.h,v
> retrieving revision 1.22
> diff -u -p -r1.22 ssl.h
> --- ssl.h 5 Mar 2021 12:37:32 - 1.22
> +++ ssl.h 10 Apr 2021 12:54:19 -
> @@ -65,9 +65,3 @@ int ssl_load_pkey(const void *, size_t,
> int ssl_ctx_fake_private_key(SSL_CTX *, const void *, size_t,
> char *, off_t, X509 **, EVP_PKEY **);
> char *ssl_pubkey_hash(const char *, off_t);
> -
> -/* ssl_privsep.c */
> -int ssl_by_mem_ctrl(X509_LOOKUP *, int, const char *, long, char
> **);
> -
> -/* ssl_verify.c */
> -int ssl_check_name(X509 *, const char *, int *);
> Index: smtpd/Makefile
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtpd/Makefile,v
> retrieving revision 1.111
> diff -u -p -r1.111 Makefile
> --- smtpd/Makefile5 Mar 2021 12:37:32 - 1.111
> +++ smtpd/Makefile10 Apr 2021 12:33:13 -
> @@ -51,8 +51,6 @@ SRCS+= smtp_session.c
> SRCS+= smtpd.c
> SRCS+= srs.c
> SRCS+= ssl.c
> -SRCS+= ssl_smtpd.c
> -SRCS+= ssl_verify.c
> SRCS+= stat_backend.c
> SRCS+= table.c
> SRCS+= to.c
> @@ -82,8 +80,8 @@ SRCS+= stat_ramstat.c
> MAN= sendmail.8 smtpd.8 smtpd.conf.5 table.5
> BINDIR= /usr/sbin
>
> -LDADD+= -levent -lutil -ltls -lssl -lcrypto -lm -lz
> -DPADD+= ${LIBEVENT} ${LIBUTIL} ${LIBTLS} ${LIBSSL} ${LIBCRYPTO}
> ${LIBM} ${LIBZ}
> +LDADD+= -levent -lutil -ltls -lssl -lcrypto -lz
> +DPADD+= ${LIBEVENT} ${LIBUTIL} ${LIBTLS} ${LIBSSL} ${LIBCRYPTO}
> ${LIBZ}
>
> CFLAGS+= -fstack-protector-all
> CFLAGS+= -I${.CURDIR}/..
>
Re: smtpd: default mta ciphers
On 4/1/21 4:34 PM, Eric Faurot wrote: > If not cipher list is specified for a relay rule, fallback to > the global cipher list if defined, rather than libtls default. > This is closer to the previous behavior. > > Eric. > makes sense. ok giovanni@ Cheers Giovanni OpenPGP_signature Description: OpenPGP digital signature
Re: smtpd: default mta ciphers
On Thu, 01 Apr 2021 16:34:33 +0200, Eric Faurot wrote: > If not cipher list is specified for a relay rule, fallback to > the global cipher list if defined, rather than libtls default. > This is closer to the previous behavior. OK millert@ - todd
Re: smtpd: trace and vfprintf %s NULL
Any objection or ok?
On Sat, Mar 27, 2021 at 12:52:11PM +0100, Eric Faurot wrote:
> Hello.
>
> I get reports from people seeing "vfprintf %s NULL" in their logs
> recently. The problem is in a function that should be fixed,
> but that function is only expected to but used for debug traces.
>
> This diff turns log_trace() into a macro, so the parameters
> are not needlessly evaluated when tracing is not set.
>
> Eric.
>
> Index: smtpd.h
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.h,v
> retrieving revision 1.662
> diff -u -p -r1.662 smtpd.h
> --- smtpd.h 5 Mar 2021 12:37:32 - 1.662
> +++ smtpd.h 23 Mar 2021 07:16:39 -
> @@ -1747,8 +1747,9 @@ int base64_encode_rfc3548(unsigned char
> char *, size_t);
>
> void log_trace_verbose(int);
> -void log_trace(int, const char *, ...)
> -__attribute__((format (printf, 2, 3)));
> +void log_trace0(const char *, ...)
> +__attribute__((format (printf, 1, 2)));
> +#define log_trace(m, ...) do { if (tracing & (m)) log_trace0(__VA_ARGS__);
> } while (0)
>
> /* waitq.c */
> int waitq_wait(void *, void (*)(void *, void *, void *), void *);
> Index: util.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/util.c,v
> retrieving revision 1.152
> diff -u -p -r1.152 util.c
> --- util.c29 Nov 2020 20:07:38 - 1.152
> +++ util.c23 Mar 2021 07:17:05 -
> @@ -823,15 +823,13 @@ base64_encode_rfc3548(unsigned char cons
> }
>
> void
> -log_trace(int mask, const char *emsg, ...)
> +log_trace0(const char *emsg, ...)
> {
> va_list ap;
>
> - if (tracing & mask) {
> - va_start(ap, emsg);
> - vlog(LOG_DEBUG, emsg, ap);
> - va_end(ap);
> - }
> + va_start(ap, emsg);
> + vlog(LOG_DEBUG, emsg, ap);
> + va_end(ap);
> }
>
> void
>
>
Re: smtpd: set protocols and ciphers
On Thu, Mar 25, 2021 at 06:52:13PM +0100, Eric Faurot wrote: > Hi. > > This diff allows to specify the protocol versions and ciphers > to use for outgoing TLS sessions on a per relay basis. Yes, I think we need this. ok tb
Re: smtpd: use mx name for sni
On Sun, 07 Mar 2021 21:47:45 +0100, Eric Faurot wrote: > As spotted by krw@, the mta should use the mx hostname for sni, not > the reverse dns for the peer address. Yes, this matches the previous behavior with ssl_check_name(). OK millert@ - todd
Re: smtpd: use mx name for sni
On Sun, Mar 07, 2021 at 09:47:45PM +0100, Eric Faurot wrote: > As spotted by krw@, the mta should use the mx hostname for sni, not > the reverse dns for the peer address. ok tb > > Eric. > > > Index: mta_session.c > === > RCS file: /cvs/src/usr.sbin/smtpd/mta_session.c,v > retrieving revision 1.139 > diff -u -p -r1.139 mta_session.c > --- mta_session.c 5 Mar 2021 12:37:32 - 1.139 > +++ mta_session.c 7 Mar 2021 20:18:42 - > @@ -1596,7 +1596,7 @@ mta_tls_init(struct mta_session *s) > return; > } > > - io_connect_tls(s->io, tls, s->route->dst->ptrname); > + io_connect_tls(s->io, tls, s->mxname); > } > > static void >
Re: smtpd: use libtls
On Sat, Feb 13, 2021 at 06:26:02PM +0100, Eric Faurot wrote:
> Hi.
>
> The diff seems to work for the few people who tested it (thanks).
> Anyone wants to ok this?
I read through the diff several times, but I'm not familiar with smtpd
so cannot claim a thorough review. Nothing really stood out as odd or
wrong. I like the simplification a lot.
ok tb
I only have these two trivial remarks:
> int
> -io_start_tls(struct io *io, void *tls)
> +io_accept_tls(struct io *io, struct tls *tls)
> {
> int mode;
>
> mode = io->flags & IO_RW;
> - if (mode == 0 || mode == IO_RW)
> - errx(1, "io_start_tls(): full-duplex or unset");
> + if (mode != IO_READ)
> + errx(1, "io_connect_tls: expect IO_READ mode");
should be io_accept_tls
>
> if (io->tls)
> errx(1, "io_start_tls(): TLS already started");
dito
> io->tls = tls;
> + io->state = IO_STATE_ACCEPT_TLS;
> + io_reset(io, EV_READ, io_dispatch_accept_tls);
> +
> + return (0);
> +}
Re: smtpd: use libtls
Hi.
The diff seems to work for the few people who tested it (thanks).
Anyone wants to ok this?
Eric.
Index: ca.c
===
RCS file: /cvs/src/usr.sbin/smtpd/ca.c,v
retrieving revision 1.37
diff -u -p -r1.37 ca.c
--- ca.c31 Dec 2020 08:27:15 - 1.37
+++ ca.c19 Jan 2021 11:09:54 -
@@ -69,6 +69,7 @@ static int ecdsae_do_verify(const unsign
EC_KEY *);
+static struct dict pkeys;
static uint64_t reqid = 0;
static void
@@ -132,26 +133,29 @@ ca_init(void)
struct pki *pki;
const char *k;
void*iter_dict;
+ char*hash;
log_debug("debug: init private ssl-tree");
+ dict_init();
iter_dict = NULL;
while (dict_iter(env->sc_pki_dict, _dict, , (void **))) {
if (pki->pki_key == NULL)
continue;
- if ((in = BIO_new_mem_buf(pki->pki_key,
- pki->pki_key_len)) == NULL)
- fatalx("ca_launch: key");
-
- if ((pkey = PEM_read_bio_PrivateKey(in,
- NULL, NULL, NULL)) == NULL)
- fatalx("ca_launch: PEM");
+ in = BIO_new_mem_buf(pki->pki_key, pki->pki_key_len);
+ if (in == NULL)
+ fatalx("ca_init: key");
+ pkey = PEM_read_bio_PrivateKey(in, NULL, NULL, NULL);
+ if (pkey == NULL)
+ fatalx("ca_init: PEM");
BIO_free(in);
- pki->pki_pkey = pkey;
-
- freezero(pki->pki_key, pki->pki_key_len);
- pki->pki_key = NULL;
+ hash = ssl_pubkey_hash(pki->pki_cert, pki->pki_cert_len);
+ if (dict_check(, hash))
+ EVP_PKEY_free(pkey);
+ else
+ dict_xset(, hash, pkey);
+ free(hash);
}
}
@@ -223,15 +227,15 @@ end:
void
ca_imsg(struct mproc *p, struct imsg *imsg)
{
+ EVP_PKEY*pkey;
RSA *rsa = NULL;
EC_KEY *ecdsa = NULL;
const void *from = NULL;
unsigned char *to = NULL;
struct msg m;
- const char *pkiname;
+ const char *hash;
size_t flen, tlen, padding;
int buf_len;
- struct pki *pki;
int ret = 0;
uint64_t id;
int v;
@@ -267,16 +271,15 @@ ca_imsg(struct mproc *p, struct imsg *im
case IMSG_CA_RSA_PRIVDEC:
m_msg(, imsg);
m_get_id(, );
- m_get_string(, );
+ m_get_string(, );
m_get_data(, , );
m_get_size(, );
m_get_size(, );
m_end();
- pki = dict_get(env->sc_pki_dict, pkiname);
- if (pki == NULL || pki->pki_pkey == NULL ||
- (rsa = EVP_PKEY_get1_RSA(pki->pki_pkey)) == NULL)
- fatalx("ca_imsg: invalid pki");
+ pkey = dict_get(, hash);
+ if (pkey == NULL || (rsa = EVP_PKEY_get1_RSA(pkey)) == NULL)
+ fatalx("ca_imsg: invalid pkey hash");
if ((to = calloc(1, tlen)) == NULL)
fatalx("ca_imsg: calloc");
@@ -306,14 +309,14 @@ ca_imsg(struct mproc *p, struct imsg *im
case IMSG_CA_ECDSA_SIGN:
m_msg(, imsg);
m_get_id(, );
- m_get_string(, );
+ m_get_string(, );
m_get_data(, , );
m_end();
- pki = dict_get(env->sc_pki_dict, pkiname);
- if (pki == NULL || pki->pki_pkey == NULL ||
- (ecdsa = EVP_PKEY_get1_EC_KEY(pki->pki_pkey)) == NULL)
- fatalx("ca_imsg: invalid pki");
+ pkey = dict_get(, hash);
+ if (pkey == NULL ||
+ (ecdsa = EVP_PKEY_get1_EC_KEY(pkey)) == NULL)
+ fatalx("ca_imsg: invalid pkey hash");
buf_len = ECDSA_size(ecdsa);
if ((to = calloc(1, buf_len)) == NULL)
@@ -350,12 +353,12 @@ rsae_send_imsg(int flen, const unsigned
struct imsg imsg;
int n, done = 0;
const void *toptr;
- char*pkiname;
+ char*hash;
size_t tlen;
struct msg m;
uint64_t id;
- if ((pkiname = RSA_get_ex_data(rsa, 0)) == NULL)
+ if ((hash = RSA_get_ex_data(rsa, 0)) == NULL)
return (0);
/*
@@ -365,7 +368,7 @@ rsae_send_imsg(int flen, const unsigned
m_create(p_ca, cmd, 0, 0, -1);
reqid++;
m_add_id(p_ca, reqid);
-
Re: smtpd: use libtls
Been running it for a few days, no regressions so far
> On 5 Feb 2021, at 09:35, Eric Faurot wrote:
>
> No much report so far.
> Anybody had a chance to test this?
> Here is the same diff again with manpage update this time.
>
> Eric.
>
> Index: ca.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/ca.c,v
> retrieving revision 1.37
> diff -u -p -r1.37 ca.c
> --- ca.c 31 Dec 2020 08:27:15 - 1.37
> +++ ca.c 19 Jan 2021 11:09:54 -
> @@ -69,6 +69,7 @@ static int ecdsae_do_verify(const unsign
> EC_KEY *);
>
>
> +static struct dict pkeys;
> static uint64_treqid = 0;
>
> static void
> @@ -132,26 +133,29 @@ ca_init(void)
> struct pki *pki;
> const char *k;
> void*iter_dict;
> + char*hash;
>
> log_debug("debug: init private ssl-tree");
> + dict_init();
> iter_dict = NULL;
> while (dict_iter(env->sc_pki_dict, _dict, , (void **))) {
> if (pki->pki_key == NULL)
> continue;
>
> - if ((in = BIO_new_mem_buf(pki->pki_key,
> - pki->pki_key_len)) == NULL)
> - fatalx("ca_launch: key");
> -
> - if ((pkey = PEM_read_bio_PrivateKey(in,
> - NULL, NULL, NULL)) == NULL)
> - fatalx("ca_launch: PEM");
> + in = BIO_new_mem_buf(pki->pki_key, pki->pki_key_len);
> + if (in == NULL)
> + fatalx("ca_init: key");
> + pkey = PEM_read_bio_PrivateKey(in, NULL, NULL, NULL);
> + if (pkey == NULL)
> + fatalx("ca_init: PEM");
> BIO_free(in);
>
> - pki->pki_pkey = pkey;
> -
> - freezero(pki->pki_key, pki->pki_key_len);
> - pki->pki_key = NULL;
> + hash = ssl_pubkey_hash(pki->pki_cert, pki->pki_cert_len);
> + if (dict_check(, hash))
> + EVP_PKEY_free(pkey);
> + else
> + dict_xset(, hash, pkey);
> + free(hash);
> }
> }
>
> @@ -223,15 +227,15 @@ end:
> void
> ca_imsg(struct mproc *p, struct imsg *imsg)
> {
> + EVP_PKEY*pkey;
> RSA *rsa = NULL;
> EC_KEY *ecdsa = NULL;
> const void *from = NULL;
> unsigned char *to = NULL;
> struct msg m;
> - const char *pkiname;
> + const char *hash;
> size_t flen, tlen, padding;
> int buf_len;
> - struct pki *pki;
> int ret = 0;
> uint64_t id;
> int v;
> @@ -267,16 +271,15 @@ ca_imsg(struct mproc *p, struct imsg *im
> case IMSG_CA_RSA_PRIVDEC:
> m_msg(, imsg);
> m_get_id(, );
> - m_get_string(, );
> + m_get_string(, );
> m_get_data(, , );
> m_get_size(, );
> m_get_size(, );
> m_end();
>
> - pki = dict_get(env->sc_pki_dict, pkiname);
> - if (pki == NULL || pki->pki_pkey == NULL ||
> - (rsa = EVP_PKEY_get1_RSA(pki->pki_pkey)) == NULL)
> - fatalx("ca_imsg: invalid pki");
> + pkey = dict_get(, hash);
> + if (pkey == NULL || (rsa = EVP_PKEY_get1_RSA(pkey)) == NULL)
> + fatalx("ca_imsg: invalid pkey hash");
>
> if ((to = calloc(1, tlen)) == NULL)
> fatalx("ca_imsg: calloc");
> @@ -306,14 +309,14 @@ ca_imsg(struct mproc *p, struct imsg *im
> case IMSG_CA_ECDSA_SIGN:
> m_msg(, imsg);
> m_get_id(, );
> - m_get_string(, );
> + m_get_string(, );
> m_get_data(, , );
> m_end();
>
> - pki = dict_get(env->sc_pki_dict, pkiname);
> - if (pki == NULL || pki->pki_pkey == NULL ||
> - (ecdsa = EVP_PKEY_get1_EC_KEY(pki->pki_pkey)) == NULL)
> - fatalx("ca_imsg: invalid pki");
> + pkey = dict_get(, hash);
> + if (pkey == NULL ||
> + (ecdsa = EVP_PKEY_get1_EC_KEY(pkey)) == NULL)
> + fatalx("ca_imsg: invalid pkey hash");
>
> buf_len = ECDSA_size(ecdsa);
> if ((to = calloc(1, buf_len)) == NULL)
> @@ -350,12 +353,12 @@ rsae_send_imsg(int flen, const unsigned
> struct imsg imsg;
> int n, done = 0;
> const void *toptr;
> - char*pkiname;
> + char*hash;
> size_t tlen;
> struct msg m;
> uint64_t id;
>
> - if ((pkiname = RSA_get_ex_data(rsa, 0)) == NULL)
> + if ((hash = RSA_get_ex_data(rsa, 0)) == NULL)
>
Re: smtpd: use libtls
No much report so far.
Anybody had a chance to test this?
Here is the same diff again with manpage update this time.
Eric.
Index: ca.c
===
RCS file: /cvs/src/usr.sbin/smtpd/ca.c,v
retrieving revision 1.37
diff -u -p -r1.37 ca.c
--- ca.c31 Dec 2020 08:27:15 - 1.37
+++ ca.c19 Jan 2021 11:09:54 -
@@ -69,6 +69,7 @@ static int ecdsae_do_verify(const unsign
EC_KEY *);
+static struct dict pkeys;
static uint64_t reqid = 0;
static void
@@ -132,26 +133,29 @@ ca_init(void)
struct pki *pki;
const char *k;
void*iter_dict;
+ char*hash;
log_debug("debug: init private ssl-tree");
+ dict_init();
iter_dict = NULL;
while (dict_iter(env->sc_pki_dict, _dict, , (void **))) {
if (pki->pki_key == NULL)
continue;
- if ((in = BIO_new_mem_buf(pki->pki_key,
- pki->pki_key_len)) == NULL)
- fatalx("ca_launch: key");
-
- if ((pkey = PEM_read_bio_PrivateKey(in,
- NULL, NULL, NULL)) == NULL)
- fatalx("ca_launch: PEM");
+ in = BIO_new_mem_buf(pki->pki_key, pki->pki_key_len);
+ if (in == NULL)
+ fatalx("ca_init: key");
+ pkey = PEM_read_bio_PrivateKey(in, NULL, NULL, NULL);
+ if (pkey == NULL)
+ fatalx("ca_init: PEM");
BIO_free(in);
- pki->pki_pkey = pkey;
-
- freezero(pki->pki_key, pki->pki_key_len);
- pki->pki_key = NULL;
+ hash = ssl_pubkey_hash(pki->pki_cert, pki->pki_cert_len);
+ if (dict_check(, hash))
+ EVP_PKEY_free(pkey);
+ else
+ dict_xset(, hash, pkey);
+ free(hash);
}
}
@@ -223,15 +227,15 @@ end:
void
ca_imsg(struct mproc *p, struct imsg *imsg)
{
+ EVP_PKEY*pkey;
RSA *rsa = NULL;
EC_KEY *ecdsa = NULL;
const void *from = NULL;
unsigned char *to = NULL;
struct msg m;
- const char *pkiname;
+ const char *hash;
size_t flen, tlen, padding;
int buf_len;
- struct pki *pki;
int ret = 0;
uint64_t id;
int v;
@@ -267,16 +271,15 @@ ca_imsg(struct mproc *p, struct imsg *im
case IMSG_CA_RSA_PRIVDEC:
m_msg(, imsg);
m_get_id(, );
- m_get_string(, );
+ m_get_string(, );
m_get_data(, , );
m_get_size(, );
m_get_size(, );
m_end();
- pki = dict_get(env->sc_pki_dict, pkiname);
- if (pki == NULL || pki->pki_pkey == NULL ||
- (rsa = EVP_PKEY_get1_RSA(pki->pki_pkey)) == NULL)
- fatalx("ca_imsg: invalid pki");
+ pkey = dict_get(, hash);
+ if (pkey == NULL || (rsa = EVP_PKEY_get1_RSA(pkey)) == NULL)
+ fatalx("ca_imsg: invalid pkey hash");
if ((to = calloc(1, tlen)) == NULL)
fatalx("ca_imsg: calloc");
@@ -306,14 +309,14 @@ ca_imsg(struct mproc *p, struct imsg *im
case IMSG_CA_ECDSA_SIGN:
m_msg(, imsg);
m_get_id(, );
- m_get_string(, );
+ m_get_string(, );
m_get_data(, , );
m_end();
- pki = dict_get(env->sc_pki_dict, pkiname);
- if (pki == NULL || pki->pki_pkey == NULL ||
- (ecdsa = EVP_PKEY_get1_EC_KEY(pki->pki_pkey)) == NULL)
- fatalx("ca_imsg: invalid pki");
+ pkey = dict_get(, hash);
+ if (pkey == NULL ||
+ (ecdsa = EVP_PKEY_get1_EC_KEY(pkey)) == NULL)
+ fatalx("ca_imsg: invalid pkey hash");
buf_len = ECDSA_size(ecdsa);
if ((to = calloc(1, buf_len)) == NULL)
@@ -350,12 +353,12 @@ rsae_send_imsg(int flen, const unsigned
struct imsg imsg;
int n, done = 0;
const void *toptr;
- char*pkiname;
+ char*hash;
size_t tlen;
struct msg m;
uint64_t id;
- if ((pkiname = RSA_get_ex_data(rsa, 0)) == NULL)
+ if ((hash = RSA_get_ex_data(rsa, 0)) == NULL)
return (0);
/*
@@ -365,7 +368,7 @@ rsae_send_imsg(int flen, const unsigned
m_create(p_ca, cmd, 0, 0, -1);
reqid++;
Re: smtpd: use libtls
On 1/27/21 7:29 AM, gil...@poolp.org wrote: > January 27, 2021 9:47 AM, "Lauri Tirkkonen" wrote: > >> On Wed, Jan 27 2021 09:36:31 +0100, Eric Faurot wrote: >> >>> There has been a plan for some time now to make smtpd use libtls >>> instead of openssl. Recent changes in libtls allow to move forward >>> with this. Here is a diff to start the switch. I've tried to keep >>> it as small as possible, sticking to the necessary changes. There is >>> still a lot of code that can be removed but that will be done in a >>> second time. >> >> I'm all for this, and sorry for screaming from the gallery, but I want to >> ask - >> is there a plan relating to libtls for portable OpenSMTPD? As it stands, >> OpenSSL-based systems are largely unable to use libtls (which in itself is a >> shame) - how would this change make it to portable? >> > > TL;DR: > In January 2020, I adapted OpenSMTPD to libtls for the first time and did it > both > for OpenBSD and portable. Since many systems didn't have LibreSSL available, > this > resulted in libtls being brought to the openbsd-compat layer and adapted to > build > with OpenSSL. The plan is to use libtls from LibreSSL if detected, otherwise > take > the openbsd-compat version if OpenSSL is detected. > > More (outdated) details here: > > https://poolp.org/posts/2020-01-22/january-2020-opensmtpd-work-libasr-and-libtls/ > > > As a side note: > > The work eric@ did on the libtls conversion was based on my diff but diverged > and > I will have to adapt my work from last year to make it work again. I'll take > care > of making it work again once his work is committed. > > As of today, there's no one but me working on the portable release so it > would be > nice if people interested in a portable release would step up to help. > Is it not possible to use libretls - https://git.causal.agency/libretls/about/ They plan to maintain such a compatibility layer of libtls with openssl with minimal changes. It might be better to use their effort rather than adding a burden of both a compat libtls and opensmtpd in the portable version. Quite a lot of distributions already have this present so this might be a good idea to use their work. Thoughts? Best, Aisha
Re: smtpd: use libtls
January 27, 2021 9:47 AM, "Lauri Tirkkonen" wrote: > On Wed, Jan 27 2021 09:36:31 +0100, Eric Faurot wrote: > >> There has been a plan for some time now to make smtpd use libtls >> instead of openssl. Recent changes in libtls allow to move forward >> with this. Here is a diff to start the switch. I've tried to keep >> it as small as possible, sticking to the necessary changes. There is >> still a lot of code that can be removed but that will be done in a >> second time. > > I'm all for this, and sorry for screaming from the gallery, but I want to ask > - > is there a plan relating to libtls for portable OpenSMTPD? As it stands, > OpenSSL-based systems are largely unable to use libtls (which in itself is a > shame) - how would this change make it to portable? > TL;DR: In January 2020, I adapted OpenSMTPD to libtls for the first time and did it both for OpenBSD and portable. Since many systems didn't have LibreSSL available, this resulted in libtls being brought to the openbsd-compat layer and adapted to build with OpenSSL. The plan is to use libtls from LibreSSL if detected, otherwise take the openbsd-compat version if OpenSSL is detected. More (outdated) details here: https://poolp.org/posts/2020-01-22/january-2020-opensmtpd-work-libasr-and-libtls/ As a side note: The work eric@ did on the libtls conversion was based on my diff but diverged and I will have to adapt my work from last year to make it work again. I'll take care of making it work again once his work is committed. As of today, there's no one but me working on the portable release so it would be nice if people interested in a portable release would step up to help.
Re: smtpd: use libtls
On Wed, Jan 27 2021 09:36:31 +0100, Eric Faurot wrote: > There has been a plan for some time now to make smtpd use libtls > instead of openssl. Recent changes in libtls allow to move forward > with this. Here is a diff to start the switch. I've tried to keep > it as small as possible, sticking to the necessary changes. There is > still a lot of code that can be removed but that will be done in a > second time. I'm all for this, and sorry for screaming from the gallery, but I want to ask - is there a plan relating to libtls for portable OpenSMTPD? As it stands, OpenSSL-based systems are largely unable to use libtls (which in itself is a shame) - how would this change make it to portable? -- Lauri Tirkkonen | lotheac @ IRCnet
Re: smtpd: trim down on filter processes
On Sun, 27 Dec 2020 18:41:22 +0100, Martijn van Duren wrote: > Because filters use system(3) after forking we get 2 processes for every > filter: one for waiting for system(3) to return and one running the actual > filter. > > Since the extra smtpd process does absolutely nothing we can just as easily > copy over what system(3) does internally for execve and call the shell > command directly. Since you are just passing environ unchanged, why not use execv(3) instead of execve(2) and avoid the ugly 'extern char **environ;'? - todd
Re: smtpd: trim down on filter processes
On Sun, 2020-12-27 at 11:18 -0700, Theo de Raadt wrote:
> fork_filter_process() does not feel like the right name for
> the function anymore.
>
Why not? Right now we do fork and call system system in the child.
With my diff we move to fork -> exec
The fork part is most definitely still there. Or do you object to some
other part of the name?
> Take note the exit value of the process (as seen by wait elsewhere) will
> be subtly differe after this conversion from system() to execve().
> Upon failure, rather than being 127, it is now 1.
parent_sig_handler() for the SIGCHLD case does:
if (WEXITSTATUS(status) != 0) {
A little further down we also do:
mda_sysexit = WEXITSTATUS(status);
But mda_sysexit is only used for CHILD_MDA.
So I don't see a reason why this subtle difference matters here.
>
> Martijn van Duren wrote:
>
> > Because filters use system(3) after forking we get 2 processes for every
> > filter: one for waiting for system(3) to return and one running the actual
> > filter.
> >
> > Since the extra smtpd process does absolutely nothing we can just as easily
> > copy over what system(3) does internally for execve and call the shell
> > command directly.
> >
> > OK?
> >
> > martijn@
> >
> > Index: smtpd.c
> > ===
> > RCS file: /cvs/src/usr.sbin/smtpd/smtpd.c,v
> > retrieving revision 1.335
> > diff -u -p -r1.335 smtpd.c
> > --- smtpd.c 23 Sep 2020 19:11:50 - 1.335
> > +++ smtpd.c 27 Dec 2020 17:39:02 -
> > @@ -1304,7 +1304,9 @@ fork_filter_process(const char *name, co
> > struct passwd *pw;
> > struct group*gr;
> > char exec[_POSIX_ARG_MAX];
> > + char*argp[] = { "sh", "-c", exec, NULL };
> > int execr;
> > + extern char **environ;
> >
> > if (user == NULL)
> > user = SMTPD_USER;
> > @@ -1387,11 +1389,8 @@ fork_filter_process(const char *name, co
> > */
> > if (read(STDERR_FILENO, , 1) != 0)
> > errx(1, "lka didn't properly close write end of error
> > socket");
> > - if (system(exec) == -1)
> > - err(1, NULL);
> > -
> > - /* there's no successful exit from a processor */
> > - _exit(1);
> > + execve(_PATH_BSHELL, argp, environ);
> > + err(1, "execve");
> > }
> >
> > static void
> >
> >
Re: smtpd: trim down on filter processes
fork_filter_process() does not feel like the right name for
the function anymore.
Take note the exit value of the process (as seen by wait elsewhere) will
be subtly differe after this conversion from system() to execve().
Upon failure, rather than being 127, it is now 1.
Martijn van Duren wrote:
> Because filters use system(3) after forking we get 2 processes for every
> filter: one for waiting for system(3) to return and one running the actual
> filter.
>
> Since the extra smtpd process does absolutely nothing we can just as easily
> copy over what system(3) does internally for execve and call the shell
> command directly.
>
> OK?
>
> martijn@
>
> Index: smtpd.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.c,v
> retrieving revision 1.335
> diff -u -p -r1.335 smtpd.c
> --- smtpd.c 23 Sep 2020 19:11:50 - 1.335
> +++ smtpd.c 27 Dec 2020 17:39:02 -
> @@ -1304,7 +1304,9 @@ fork_filter_process(const char *name, co
> struct passwd *pw;
> struct group*gr;
> char exec[_POSIX_ARG_MAX];
> + char*argp[] = { "sh", "-c", exec, NULL };
> int execr;
> + extern char **environ;
>
> if (user == NULL)
> user = SMTPD_USER;
> @@ -1387,11 +1389,8 @@ fork_filter_process(const char *name, co
>*/
> if (read(STDERR_FILENO, , 1) != 0)
> errx(1, "lka didn't properly close write end of error socket");
> - if (system(exec) == -1)
> - err(1, NULL);
> -
> - /* there's no successful exit from a processor */
> - _exit(1);
> + execve(_PATH_BSHELL, argp, environ);
> + err(1, "execve");
> }
>
> static void
>
>
Re: smtpd: relax ORCPT check again
November 18, 2020 10:17 PM, "Joerg Jung" wrote:
> On Wed, Nov 18, 2020 at 08:57:48PM +, gil...@poolp.org wrote:
>
>> November 18, 2020 9:54 PM, "Joerg Jung" wrote:
>>
>> Hi,
>>
>> in my opinion revision 1.423 of smtp_session.c went a bit too far.
>> Enforcing that ORCPT has to have domain results in breakage in real
>> world usage. For example, sending from root user cron jobs mails
>> via Postfix aliased to an address handled by OpenSMTPD will fail.
>>
>> RFC leaves some room for interpretation here, but interoperability
>> may be considered important as well. GitHub issue #1084 [1] has
>> some more elaborations and examples, but was opened for the same
>> reason.
>>
>> Therefore, I propose the following diff below to slightly relax
>> the ORCPT check again by skipping the domain part check.
>>
>> Comments, OK?
>>
>> maybe just skip the check ONLY if domain part is empty ?
>
> Like in the diff below and assuming that domain part is always
> nul terminated after text_to_mailaddr()?
>
This reads better IMO yes
> OK?
>
> Index: smtp_session.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v
> retrieving revision 1.426
> diff -u -p -r1.426 smtp_session.c
> --- smtp_session.c 24 Apr 2020 11:34:07 - 1.426
> +++ smtp_session.c 18 Nov 2020 21:10:36 -
> @@ -2583,7 +2583,8 @@ smtp_tx_rcpt_to(struct smtp_tx *tx, cons
>
> if (!text_to_mailaddr(>evp.dsn_orcpt, opt) ||
> !valid_localpart(tx->evp.dsn_orcpt.user) ||
> - !valid_domainpart(tx->evp.dsn_orcpt.domain)) {
> + (strlen(tx->evp.dsn_orcpt.domain) != 0 &&
> + !valid_domainpart(tx->evp.dsn_orcpt.domain))) {
> smtp_reply(tx->session,
> "553 ORCPT address syntax error");
> return;
Re: smtpd: relax ORCPT check again
On Wed, Nov 18, 2020 at 08:57:48PM +, gil...@poolp.org wrote:
> November 18, 2020 9:54 PM, "Joerg Jung" wrote:
>
> > Hi,
> >
> > in my opinion revision 1.423 of smtp_session.c went a bit too far.
> > Enforcing that ORCPT has to have domain results in breakage in real
> > world usage. For example, sending from root user cron jobs mails
> > via Postfix aliased to an address handled by OpenSMTPD will fail.
> >
> > RFC leaves some room for interpretation here, but interoperability
> > may be considered important as well. GitHub issue #1084 [1] has
> > some more elaborations and examples, but was opened for the same
> > reason.
> >
> > Therefore, I propose the following diff below to slightly relax
> > the ORCPT check again by skipping the domain part check.
> >
> > Comments, OK?
> >
>
> maybe just skip the check ONLY if domain part is empty ?
>
Like in the diff below and assuming that domain part is always
nul terminated after text_to_mailaddr()?
OK?
Index: smtp_session.c
===
RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v
retrieving revision 1.426
diff -u -p -r1.426 smtp_session.c
--- smtp_session.c 24 Apr 2020 11:34:07 - 1.426
+++ smtp_session.c 18 Nov 2020 21:10:36 -
@@ -2583,7 +2583,8 @@ smtp_tx_rcpt_to(struct smtp_tx *tx, cons
if (!text_to_mailaddr(>evp.dsn_orcpt, opt) ||
!valid_localpart(tx->evp.dsn_orcpt.user) ||
- !valid_domainpart(tx->evp.dsn_orcpt.domain)) {
+ (strlen(tx->evp.dsn_orcpt.domain) != 0 &&
+!valid_domainpart(tx->evp.dsn_orcpt.domain))) {
smtp_reply(tx->session,
"553 ORCPT address syntax error");
return;
Re: smtpd: document "pki" option for relay delivery in smtpd.conf(5)
On Sun, 13 Sep 2020 20:45:35 +0800, Nick Gasson wrote: > I struggled a bit to configure smtpd to relay to a remote server that > requires SSL client certificates. The solution is to just add a "pki > host.example.org" option, but "pki" is not listed as a valid option for > the relay delivery method, even though the parser accepts it. Committed. - todd
Re: smtpd: document "pki" option for relay delivery in smtpd.conf(5)
On 9/13/20 11:09 PM, Todd C. Miller wrote: > On Sun, 13 Sep 2020 20:45:35 +0800, Nick Gasson wrote: > >> I struggled a bit to configure smtpd to relay to a remote server that >> requires SSL client certificates. The solution is to just add a "pki >> host.example.org" option, but "pki" is not listed as a valid option for >> the relay delivery method, even though the parser accepts it. > > Looks good to me. Anyone else want to OK this? > > - todd > ok giovanni@ Cheers Giovanni
Re: smtpd: document "pki" option for relay delivery in smtpd.conf(5)
On Sun, 13 Sep 2020 20:45:35 +0800, Nick Gasson wrote: > I struggled a bit to configure smtpd to relay to a remote server that > requires SSL client certificates. The solution is to just add a "pki > host.example.org" option, but "pki" is not listed as a valid option for > the relay delivery method, even though the parser accepts it. Looks good to me. Anyone else want to OK this? - todd
Re: smtpd filters: accept bypass in commit stage
Martijn van Duren wrote: > Does this filter actually work for you? Not by my testing, nor my > understanding of filters. Filter-dkimsign works during the > filter-dataline phase, so you'd have to circumvent that one, which is > not supported. Finally got around to test it. Found the problem too late in the night to fetch the sources, build and see if it solved the issue for me. You're correct, the mail is getting signed everytime anyways. Got the impression that `commit` was the right phase after running `smtpd -dvvv -T filters` when I first tried with setting `bypass` in `mail-from` phase: 15eac9cd3aadddc1 filters session-begin 15eac9cd3aadddc1 filters protocol phase=connect, resume=n, action=proceed 15eac9cd3aadddc1 filters protocol phase=ehlo, resume=n, action=proceed 15eac9cd3aadddc1 filters protocol phase=mail-from, resume=n, action=bypass, filter=ext_relay, query=lucas@domain.invalid 15eac9cd3aadddc1 filters protocol phase=rcpt-to, resume=n, action=proceed 15eac9cd3aadddc1 filters protocol phase=data, resume=n, action=proceed smtp: 0x8941daca000: fd 16 from queue smtp: 0x8941daca000: message fd 16 15eac9cd3aadddc1 filters data-begin fd=11 smtp: 0x8941daca000: fd 17 from lka smtp: 0x8941daca000: filter fd 17 smtp: 0x8941daca000: message begin 15eac9cd3aadddc1 filters protocol phase=commit, resume=n, action=deferred, filter=dkimsign 15eac9cd3aadddc1 filters protocol phase=connect, resume=y, action=proceed `dkimsign` is only showing up in `commit` phase. After patching, it looks like this: 4ebd6c3015e64c8c smtp connected address=local host=mx.sexy.is 4ebd6c3015e64c8c filters session-begin 4ebd6c3015e64c8c filters protocol phase=connect, resume=n, action=proceed 4ebd6c3015e64c8c filters protocol phase=ehlo, resume=n, action=proceed 4ebd6c3015e64c8c filters protocol phase=mail-from, resume=n, action=proceed 4ebd6c3015e64c8c filters protocol phase=rcpt-to, resume=n, action=proceed 4ebd6c3015e64c8c filters protocol phase=data, resume=n, action=proceed smtp: 0x908842d5000: fd 16 from queue smtp: 0x908842d5000: message fd 16 4ebd6c3015e64c8c filters data-begin fd=11 smtp: 0x908842d5000: fd 17 from lka smtp: 0x908842d5000: filter fd 17 smtp: 0x908842d5000: message begin 4ebd6c3015e64c8c filters protocol phase=commit, resume=n, action=bypass, filter=ext_relay_dkimsign_override, query= but `filter-dkimsign` is being executed anyways. Later on I'll give hijacking `data` phase a shot, but I wonder if it'll, as `filter-dkimsign` registers for both `data-line` and `commit`. What will if I just hijack `data` and not `commit`? Will `filter-dkimsign` produce a empty email? - >8 - That being said, I still think that either the grammar is wrong, or the manpage should mention that `bypass` isn't available at `commit`. -Lucas
Re: smtpd filters: accept bypass in commit stage
Does this filter actually work for you? Not by my testing, nor my
understanding of filters. Filter-dkimsign works during the
filter-dataline phase, so you'd have to circumvent that one, which is
not supported.
Personally I'd sign the domain anyway, since it gives the receiver
some additional information where a message might have been altered if
it happens somewhere down a MTA-chain, but if you really don't want to
sign the message you could try setting the phase to data, since that's
where filter-dkimsign's magic happens. I haven't tested this though,
so it might be just the data-command and not the actual data.
If that doesn't work your best bet at this point is two different
listen statement for the two domains.
martijn@
On Tue, 2020-08-25 at 04:30 +, Lucas wrote:
> Hello tech@,
>
> I keep getting a syntax error with the following seemingly correct
> line:
>
> filter "dkimsign-override" phase commit \
> match mail-from bypass
>
> The problem (`/etc/mail/smtpd.conf:20: syntax error`) arises from
> smtpd.conf's grammar only allowing `filter_action_builtin_nojunk` for
> `commit` phase. It turns out that the current definition of
> `filter_action_builtin_nojunk` means no junk and no bypass. Address
> that moving bypass action to nojunk.
>
> My usecase for it is to avoid DKIM-signing emails that are externally
> relayed with `opensmtpd-filters-dkimsign`: if I also own
> lucas@domain.invalid, I don't want smtpd to sign lucas@domain.invalid's
> emails, just lu...@sexy.is'. This seems possible with `filter-chain`,
> and technically could be decided in `mail-from` phase, but sadly
> `bypass` doesn't short-circuit. Second best option is to `bypass`
> during `commit` phase, which is when `filter-dkimsign` does its magic.
>
> Comments?
>
> -Lucas
>
>
> Index: parse.y
> ===
> RCS file: /home/cvs/src/usr.sbin/smtpd/parse.y,v
> retrieving revision 1.278
> diff -u -p -r1.278 parse.y
> --- parse.y 1 Jun 2020 05:21:30 - 1.278
> +++ parse.y 25 Aug 2020 04:05:27 -
> @@ -1527,9 +1527,6 @@ filter_action_builtin_nojunk
> | JUNK {
> filter_config->junk = 1;
> }
> -| BYPASS {
> - filter_config->bypass = 1;
> -}
> ;
>
> filter_action_builtin_nojunk:
> @@ -1544,6 +1541,9 @@ REJECT STRING {
> }
> | REPORT STRING {
> filter_config->report = $2;
> +}
> +| BYPASS {
> + filter_config->bypass = 1;
> }
> ;
>
>
Re: smtpd: make smarthost to use SNI when relaying
looks good to me
ok beck@
On Sun, May 31, 2020 at 03:38:00PM +0200, Sebastien Marie wrote:
> Hi,
>
> updated diff after millert@ and beck@ remarks:
> - use union to collapse in_addr + in6_addr
> - doesn't allocate buffer and directly use s->relay->domain->name
>
> Thanks.
> --
> Sebastien Marie
>
>
> diff 73b535ef4537e8454483912fc3420bc304759e96 /home/semarie/repos/openbsd/src
> blob - d384692a0e43de47d645142a6b99e72b7d83b687
> 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
> @@ -1604,6 +1605,10 @@ mta_cert_init_cb(void *arg, int status, const char *na
> struct mta_session *s = arg;
> void *ssl;
> char *xname = NULL, *xcert = NULL;
> + union {
> + struct in_addr in4;
> + struct in6_addr in6;
> + } addrbuf;
>
> if (s->flags & MTA_WAIT)
> mta_tree_pop(_tls_init, s->id);
> @@ -1623,6 +1628,22 @@ mta_cert_init_cb(void *arg, int status, const char *na
> free(xcert);
> if (ssl == NULL)
> fatal("mta: ssl_mta_init");
> +
> + /*
> + * RFC4366 (SNI): Literal IPv4 and IPv6 addresses are not
> + * permitted in "HostName".
> + */
> + if (s->relay->domain->as_host == 1) {
> + if (inet_pton(AF_INET, s->relay->domain->name, ) != 1 &&
> + inet_pton(AF_INET6, s->relay->domain->name, ) != 1)
> {
> + log_debug("%016"PRIx64" mta tls setting SNI name=%s",
> + s->id, s->relay->domain->name);
> + if (SSL_set_tlsext_host_name(ssl,
> s->relay->domain->name) == 0)
> + log_warnx("%016"PRIx64" mta tls setting SNI
> failed",
> +s->id);
> + }
> + }
> +
> io_start_tls(s->io, ssl);
> }
>
>
Re: smtpd: make smarthost to use SNI when relaying
Hi,
updated diff after millert@ and beck@ remarks:
- use union to collapse in_addr + in6_addr
- doesn't allocate buffer and directly use s->relay->domain->name
Thanks.
--
Sebastien Marie
diff 73b535ef4537e8454483912fc3420bc304759e96 /home/semarie/repos/openbsd/src
blob - d384692a0e43de47d645142a6b99e72b7d83b687
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
@@ -1604,6 +1605,10 @@ mta_cert_init_cb(void *arg, int status, const char *na
struct mta_session *s = arg;
void *ssl;
char *xname = NULL, *xcert = NULL;
+ union {
+ struct in_addr in4;
+ struct in6_addr in6;
+ } addrbuf;
if (s->flags & MTA_WAIT)
mta_tree_pop(_tls_init, s->id);
@@ -1623,6 +1628,22 @@ mta_cert_init_cb(void *arg, int status, const char *na
free(xcert);
if (ssl == NULL)
fatal("mta: ssl_mta_init");
+
+ /*
+* RFC4366 (SNI): Literal IPv4 and IPv6 addresses are not
+* permitted in "HostName".
+*/
+ if (s->relay->domain->as_host == 1) {
+ if (inet_pton(AF_INET, s->relay->domain->name, ) != 1 &&
+ inet_pton(AF_INET6, s->relay->domain->name, ) != 1)
{
+ log_debug("%016"PRIx64" mta tls setting SNI name=%s",
+ s->id, s->relay->domain->name);
+ if (SSL_set_tlsext_host_name(ssl,
s->relay->domain->name) == 0)
+ log_warnx("%016"PRIx64" mta tls setting SNI
failed",
+ s->id);
+ }
+ }
+
io_start_tls(s->io, ssl);
}
Re: smtpd: make smarthost to use SNI when relaying
On Sat, May 30, 2020 at 05:40:43PM +0200, Sebastien Marie wrote:
> Hi,
>
> I am looking to make smtpd to set SNI (SSL_set_tlsext_host_name) when
> connecting
> to smarthost when relaying mail.
>
> After digging a bit in libtls (to stole the right code) and smtpd (to see
> where
> to put the stolen code), I have the following diff:
>
>
> diff 73b535ef4537e8454483912fc3420bc304759e96 /home/semarie/repos/openbsd/src
> blob - d384692a0e43de47d645142a6b99e72b7d83b687
> 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
> @@ -1604,6 +1605,8 @@ mta_cert_init_cb(void *arg, int status, const char *na
> struct mta_session *s = arg;
> void *ssl;
> char *xname = NULL, *xcert = NULL;
> + struct in_addr addrbuf4;
> + struct in6_addr addrbuf6;
>
> if (s->flags & MTA_WAIT)
> mta_tree_pop(_tls_init, s->id);
> @@ -1623,6 +1626,24 @@ mta_cert_init_cb(void *arg, int status, const char *na
> free(xcert);
> if (ssl == NULL)
> fatal("mta: ssl_mta_init");
> +
> + /*
> + * RFC4366 (SNI): Literal IPv4 and IPv6 addresses are not
> + * permitted in "HostName".
> + */
> + if (s->relay->domain->as_host == 1) {
> + xname = xstrdup(s->relay->domain->name);
This allocation appears to be unnecessary. I believe you should be
able to simply check with inet_pton, and then use
s->relay->domain->name
On a strictly smtpd front, it seems odd to have somthing called
domain->name possibly being an ip address in text form. It would seem
prudent to keep these things separate as we resolve things. (domain->ip, or
domain->mxip if we have resolved down to that might be better) but
that's a separate issue and larger change.
> + if (inet_pton(AF_INET, xname, ) != 1 &&
> + inet_pton(AF_INET6, xname, ) != 1) {
> + log_info("%016"PRIx64" mta setting SNI name=%s",
> + s->id, xname);
> + if (SSL_set_tlsext_host_name(ssl, xname) == 0)
> + log_warnx("%016"PRIx64" mta setting SNI failed",
> +s->id);
> + }
> + free(xname);
> + }
> +
> io_start_tls(s->io, ssl);
> }
>
>
>
> For what I understood:
>
> mta_cert_init_cb() function is responsable to prepare a connection. the SSL
> initialization (SSL_new() call) occured in ssl_mta_init() which was just
> called,
> so it seems it is the right place to call SSL_set_tlsext_host_name().
>
> We just need the hostname to configure it.
>
> Regarding mta_session structure, relay->domain->as_host is set to 1 when the
> domain is linked to smarthost configuration (or when the mx is ip address I
> think). And in smarthost case, the domain->name is the hostname. For SNI, we
> are
> excluding ip, so I assume it should copte with domain->name as ip.
>
> Does someone with better understanding of smtpd code source could confirm the
> approch is right and comment ?
>
> Please note I have only tested it on simple configuration.
>
> Thanks.
> --
> Sebastien Marie
>
Re: smtpd: make smarthost to use SNI when relaying
On Sat, 30 May 2020 17:40:43 +0200, Sebastien Marie wrote:
> I am looking to make smtpd to set SNI (SSL_set_tlsext_host_name)
> when connecting to smarthost when relaying mail.
>
> After digging a bit in libtls (to stole the right code) and smtpd (to
> see where to put the stolen code), I have the following diff:
Consider using a union for addrbuf, e.g.
union {
struct in_addr addr4;
struct in6_addr addr6;
} addrbuf;
There's no need to have both on the stack. Otherwise looks reasonable
to me.
- todd
Re: smtpd stricter forkmda()
> On 4. May 2020, at 11:17, Gilles Chehade wrote:
>
> forkmda() is never supposed to be called with an action dispatcher which
> is not local, this would indicate that the code path was abused somehow.
>
> idea suggested by Demi M. Obenour
ok jung@ (for post-lock)
> diff --git a/smtpd/smtpd.c b/smtpd/smtpd.c
> index ce1262fa..4c5fc3d9 100644
> --- a/smtpd/smtpd.c
> +++ b/smtpd/smtpd.c
> @@ -1409,6 +1409,8 @@ forkmda(struct mproc *p, uint64_t id, struct deliver
> *deliver)
> const char *pw_dir;
>
> dsp = dict_xget(env->sc_dispatchers, deliver->dispatcher);
> + if (dsp->type != DISPATCHER_LOCAL)
> + fatalx("non-local dispatcher called from forkmda()");
>
> log_debug("debug: smtpd: forking mda for session %016"PRIx64
> ": %s as %s", id, deliver->userinfo.username,
Re: smtpd: fix catch-all in virtual aliases
April 28, 2020 11:02 AM, "Joerg Jung" mailto:m...@umaxx.net?to=%22Joerg%20Jung%22%20)> wrote: On 28. Apr 2020, at 10:10, gil...@poolp.org (mailto:gil...@poolp.org) wrote: April 28, 2020 8:55 AM, "Joerg Jung" mailto:m...@umaxx.net)> wrote: Also this change might break existing valid setups (e.g. with mailing list servers), but people will likely know how to cope with it. Do you have an example of an existing valid setup that is broken with this ? No example, I just presumed (that’s why I wrote “might”). ok, because I can't think of a setup that could be broken with this as @ is the very last wildcard matching everything not matched earlier. this diff fixes an actual reported bug and passed various tests mixing no wildcard, user wildcard, domain wildcard. I think it should go in as it is a regression that probably got introduced a while back, the @ wildcard used to work for sure.
Re: smtpd: fix catch-all in virtual aliases
On Sun, 26 Apr 2020 18:30:25 +0200, Eric Faurot wrote: > When a catch-all entry (@) is used in a virtual alias table, it > eventually (and mistakenly) catches everything that expands to a > username. For example, with: > > f...@example.com user > @catchall > > "f...@example.com" expands to "user" as expected, but then "user" > expands to "catchall" because it is interpreted as "user@" (empty > domain). > > The catch-all fallback mechanism is really meant for full email > addresses in virtual context, and should not happen for usernames. > The following diff fixes it. That makes sense. OK millert@ I see that the catch-all behavior is documented in makemap(8) but not aliases(5). Does it make sense to document it there too? That's the first place I looked (the makemap manual was the 3rd place I looked). - todd
Re: smtpd: fix catch-all in virtual aliases
> On 28. Apr 2020, at 10:10, gil...@poolp.org wrote: > April 28, 2020 8:55 AM, "Joerg Jung" mailto:m...@umaxx.net>> > wrote: > >> Also this change might break existing valid setups (e.g. with mailing list >> servers), but people will likely know how to cope with it. > > Do you have an example of an existing valid setup that is broken with this ? No example, I just presumed (that’s why I wrote “might”).
Re: smtpd: fix catch-all in virtual aliases
April 28, 2020 8:55 AM, "Joerg Jung" wrote: >> On 26. Apr 2020, at 18:30, Eric Faurot wrote: >> >> When a catch-all entry (@) is used in a virtual alias table, it >> eventually (and mistakenly) catches everything that expands to a >> username. For example, with: >> >> f...@example.com user >> @ catchall >> >> "f...@example.com" expands to "user" as expected, but then "user" >> expands to "catchall" because it is interpreted as "user@" (empty >> domain). > > Which makes sense to me. If one doesn’t specify a domain after the ‘@‘, > I would expect to really catch-all for all domains and all users. > that's not how mailaddr work in OpenSMTPD, the semantic is this one: user=> user@* user@domain => user@domain @domain => *@domain @ => * and this is not an aliases only thing, this is how table lookups are performed for type K_MAILADDR. This allows you to do stuff like follows: root : root gil...@poolp.org : gilles e...@faurot.net : eric @poolp.org : poolpcatchall @faurot.net : faurotcatchall @: catchallusuer >> The catch-all fallback mechanism is really meant for full email >> addresses in virtual context, and should not happen for usernames. >> The following diff fixes it. > > Yes, I agree that catch-all only really meant to be used for single virtual > domain context and not with primary domains. > > But instead of allowing the syntax and ignoring the case in aliases.c > as in your diff below, I would prefer to “fail" on parsing of the table and > error logging that an empty domain after ‘@‘ is not a valid syntax, no? > > Also this change might break existing valid setups (e.g. with mailing list > servers), but people will likely know how to cope with it. > Do you have an example of an existing valid setup that is broken with this ?
Re: smtpd: fix catch-all in virtual aliases
> On 26. Apr 2020, at 18:30, Eric Faurot wrote: > > When a catch-all entry (@) is used in a virtual alias table, it > eventually (and mistakenly) catches everything that expands to a > username. For example, with: > >f...@example.com user >@catchall > > "f...@example.com" expands to "user" as expected, but then "user" > expands to "catchall" because it is interpreted as "user@" (empty > domain). Which makes sense to me. If one doesn’t specify a domain after the ‘@‘, I would expect to really catch-all for all domains and all users. > The catch-all fallback mechanism is really meant for full email > addresses in virtual context, and should not happen for usernames. > The following diff fixes it. Yes, I agree that catch-all only really meant to be used for single virtual domain context and not with primary domains. But instead of allowing the syntax and ignoring the case in aliases.c as in your diff below, I would prefer to “fail" on parsing of the table and error logging that an empty domain after ‘@‘ is not a valid syntax, no? Also this change might break existing valid setups (e.g. with mailing list servers), but people will likely know how to cope with it. Regards, Joerg > Index: aliases.c > === > RCS file: /cvs/src/usr.sbin/smtpd/aliases.c,v > retrieving revision 1.77 > diff -u -p -r1.77 aliases.c > --- aliases.c 28 Dec 2018 12:47:28 - 1.77 > +++ aliases.c 26 Apr 2020 16:04:51 - > @@ -164,6 +164,10 @@ aliases_virtual_get(struct expand *expan > if (ret) > goto expand; > > + /* Do not try catch-all entries if there is no domain */ > + if (domain[0] == '\0') > + return 0; > + > if (!bsnprintf(buf, sizeof(buf), "@%s", domain)) > return 0; > /* Failed ? We lookup for catch all for virtual domain */ >
Re: smtpd: simplify aliases_get()
On Sat, 25 Apr 2020 13:49:59 +0200, Eric Faurot wrote: > The current code for aliases_get() is a bit contorted I think. > This diff makes it clearer. OK millert@ - todd
Re: smtpd: trailing CR
observed no issue, still running w/ your diff April 23, 2020 1:10 PM, "Eric Faurot" wrote: > On Tue, Apr 21, 2020 at 07:08:48AM +, gil...@poolp.org wrote: > >> April 21, 2020 4:28 AM, "Todd C. Miller" wrote: >> >> On Mon, 20 Apr 2020 15:01:31 +0200, Eric Faurot wrote: >> >> There has been a discussion a while ago about the issue of trailing CR >> in smtp lines (https://marc.info/?l=openbsd-tech=156890805128121=2). >> >> It is agreed that stripping an optional CR at io level is a problem >> because the information is lost and there is no way to take a specific >> action at the protocol level if needed. >> >> So this diffs moves the CR stripping from io level to protocol level for >> SMTP dialogs. Other uses of io_getline() are internal and expect simple LF >> line ending. The current behavior should not change. >> >> This looks OK to me but I would feel better if we had successful >> reports from other people too. >> >> Have been running with eric's diff since yesterday afternoon, works for me > > No issue reported so far. > I think it can go in. > > Eric.
Re: smtpd: trailing CR
Hey, i let this patch run on one of our mailservers and it worked as expected. I did not notice any issues with it. But that server has a low volume of mails (around 8000 a day), so it is probobly not that helpful for you. Greetings Leo Am 21.04.2020 um 04:28 schrieb Todd C. Miller: This looks OK to me but I would feel better if we had successful reports from other people too.
Re: smtpd: trailing CR
On Tue, Apr 21, 2020 at 07:08:48AM +, gil...@poolp.org wrote: > April 21, 2020 4:28 AM, "Todd C. Miller" wrote: > > > On Mon, 20 Apr 2020 15:01:31 +0200, Eric Faurot wrote: > > > >> There has been a discussion a while ago about the issue of trailing CR > >> in smtp lines (https://marc.info/?l=openbsd-tech=156890805128121=2). > >> > >> It is agreed that stripping an optional CR at io level is a problem > >> because the information is lost and there is no way to take a specific > >> action at the protocol level if needed. > >> > >> So this diffs moves the CR stripping from io level to protocol level for > >> SMTP dialogs. Other uses of io_getline() are internal and expect simple LF > >> line ending. The current behavior should not change. > > > > This looks OK to me but I would feel better if we had successful > > reports from other people too. > > > > Have been running with eric's diff since yesterday afternoon, works for me No issue reported so far. I think it can go in. Eric.
Re: smtpd: fix smtpctl discover
April 20, 2020 9:07 PM, "Eric Faurot" wrote:
> Hi again,
>
> We had a report of a crash when running "smtpctl discover" on an
> envelope that has a invalid dispatcher (action name changed in the
> config).
>
> The issue is that the dispatcher is not checked after the envelope is
> loaded as a result of the discover command. But adding the check
> afterwards is not enough, because the envelope is already in the
> envelope cache, so this makes the whole live update/discover process
> actually useless (we would have to invalidate the cache).
>
> So the right fix, which also simplifies the code (always a good sign),
> is to make that check along with the other envelope validation checks
> that happen at envelope load time.
>
makes sense
> Index: queue.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/queue.c,v
> retrieving revision 1.189
> diff -u -p -r1.189 queue.c
> --- queue.c 30 Dec 2018 23:09:58 - 1.189
> +++ queue.c 20 Apr 2020 18:43:17 -
> @@ -686,7 +686,6 @@ static void
> queue_timeout(int fd, short event, void *p)
> {
> static uint32_t msgid = 0;
> - struct dispatcher *dsp;
> struct envelope evp;
> struct event *ev = p;
> struct timeval tv;
> @@ -705,13 +704,6 @@ queue_timeout(int fd, short event, void
> }
>
> if (r) {
> - dsp = dict_get(env->sc_dispatchers, evp.dispatcher);
> - if (dsp == NULL) {
> - log_warnx("warn: queue: missing dispatcher \"%s\""
> - " for envelope %016"PRIx64", ignoring",
> - evp.dispatcher, evp.id);
> - goto reset;
> - }
> if (msgid && evpid_to_msgid(evp.id) != msgid) {
> m_create(p_scheduler, IMSG_QUEUE_MESSAGE_COMMIT,
> 0, 0, -1);
> @@ -724,7 +716,6 @@ queue_timeout(int fd, short event, void
> m_close(p_scheduler);
> }
>
> -reset:
> tv.tv_sec = 0;
> tv.tv_usec = 10;
> evtimer_add(ev, );
> Index: queue_backend.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/queue_backend.c,v
> retrieving revision 1.65
> diff -u -p -r1.65 queue_backend.c
> --- queue_backend.c 30 Dec 2018 23:09:58 - 1.65
> +++ queue_backend.c 20 Apr 2020 18:42:38 -
> @@ -730,6 +730,9 @@ envelope_validate(struct envelope *ep)
> if (memchr(ep->errorline, '\0', sizeof(ep->errorline)) == NULL)
> return "invalid error line";
>
> + if (dict_get(env->sc_dispatchers, ep->dispatcher) == NULL)
> + return "unknown dispatcher";
> +
> return NULL;
> }
Re: smtpd: trailing CR
April 21, 2020 4:28 AM, "Todd C. Miller" wrote: > On Mon, 20 Apr 2020 15:01:31 +0200, Eric Faurot wrote: > >> There has been a discussion a while ago about the issue of trailing CR >> in smtp lines (https://marc.info/?l=openbsd-tech=156890805128121=2). >> >> It is agreed that stripping an optional CR at io level is a problem >> because the information is lost and there is no way to take a specific >> action at the protocol level if needed. >> >> So this diffs moves the CR stripping from io level to protocol level for >> SMTP dialogs. Other uses of io_getline() are internal and expect simple LF >> line ending. The current behavior should not change. > > This looks OK to me but I would feel better if we had successful > reports from other people too. > Have been running with eric's diff since yesterday afternoon, works for me
