Re: mlmmj, public-inbox broken after upgrade to (portable) 7.4.0p1
Well, maybe the thing I thought would be very stupid isn't so stupid after all. In doas.conf: permit nopass smtpd as mlmmj cmd /usr/bin/mlmmj-receive permit nopass smtpd as inboxen cmd /usr/bin/public-inbox-mda And then in the ~/.forward file for those two users, "| /usr/bin/doas -u USERNAME COMMAND" -- Chris
mlmmj, public-inbox broken after upgrade to (portable) 7.4.0p1
I'm running OpenSMTPD on Alpine Linux, and I recently upgraded to
7.4.0P1. Now my mlmmj and public-inbox are broken because they use
"|command" in ~/.forward and the command is running as the smtpd user
rather than the recipient.
Can anyone help? I know some amazingly stupid ways to "fix" this, but
I'd rather not resort to a blunt instrument.
Jan 25 19:11:55 [smtpd] 3d0de01c993d1671 smtp message msgid=32238bb8 size=1912
nrcpt=1 proto=ESMTP_
Jan 25 19:11:55 [smtpd] 3d0de01c993d1671 smtp envelope evpid=32238bb864c7d0a0
from= to=_
Jan 25 19:11:55 [/usr/bin/mlmmj-receive] mlmmj-receive.c:112: Have to invoke
either as root or as the user owning listdir Invoked with uid = [108]: No error
information_
Jan 25 19:11:55 [smtpd] 3d0de01d8a01a1bb mda delivery evpid=32238bb864c7d0a0
from= to=
rcpt= user=mlmmj delay=2s result=PermFail
stat=Error ("Have to invoke either as root or as the user owning listdir")_
Jan 25 19:11:55 [smtpd] 3d0de01c993d1671 smtp disconnected reason=quit_
-- Chris
Re: opensmtpd-extra: simplify getaddrinfo() usage
Hi
sorry I have mixed up my patch files, I have attached the correct ones.
Philipp
[2024-01-25 13:16] Philipp Takacs
> [2024-01-25 10:15] Omar Polo
> > On 2024/01/24 08:51:06 +0100, Philipp wrote:
> > > [2024-01-24 00:09] Omar Polo
> > > > [...]
> > > > if you're interested in this however, we can also avoid the strdup()
> > > > here since aldap_parse_url() already strdup()s the string for parsing
> > > > (but still frees the passed argument...)
> > >
> > > I have written two patches for this, one adding the free and one to
> > > avoid the unnecessary strdup.
> > >
> > > Ass you might guess from the filenames, there are a few more patches. I'll
> > > send the rest after I have tested all my patches.
> >
> > They all read fine to me. only one nitpick in the first one
> >
> > > From fa4cdb0a74c3b5d17cdc93b6285d765fda084740 Mon Sep 17 00:00:00 2001
> > > From: Philipp Takacs
> > > Date: Wed, 24 Jan 2024 01:16:56 +0100
> > > Subject: [PATCH 11/11] table-ldap aldap_parse_url now saves the port as
> > > string
> > > [...]
> > > --- a/extras/tables/table-ldap/table_ldap.c
> > > +++ b/extras/tables/table-ldap/table_ldap.c
> > > @@ -118,7 +118,6 @@ ldap_connect(const char *addr)
> > > {
> > > struct aldap_url lu;
> > > struct addrinfo hints, *res0, *res;
> > > - char port[32];
> > > int error, r, fd = -1;
> >
> > Here you could remove `r' too since it becomes unused.
>
> Thanks fixed.
>
> > If you've tested, I could commit my getaddrinfo() diff, then if you
> > rebase these one on top of it I could merge them. (but it's also fine to
> > get the ldaps one in first, depending on how you prefer.)
>
> I have tested now your getaddrinfo rewrite and my two fixups.
> I have had some changes because they had been somewere in
> my rebase branche. clean versions of them are attached.
>
> Philipp
From d4fa732b5375e0fa8eaa0727ae82362c941bf8c1 Mon Sep 17 00:00:00 2001
From: Philipp Takacs
Date: Tue, 23 Jan 2024 13:58:47 +0100
Subject: [PATCH 2/3] table-ldap free aldap_url
---
extras/tables/table-ldap/aldap.c | 1 -
extras/tables/table-ldap/table_ldap.c | 5 -
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/extras/tables/table-ldap/aldap.c b/extras/tables/table-ldap/aldap.c
index d54a90c..552ea52 100644
--- a/extras/tables/table-ldap/aldap.c
+++ b/extras/tables/table-ldap/aldap.c
@@ -560,7 +560,6 @@ void
aldap_free_url(struct aldap_url *lu)
{
free(lu->buffer);
- free(lu->filter);
}
int
diff --git a/extras/tables/table-ldap/table_ldap.c b/extras/tables/table-ldap/table_ldap.c
index 6bdce67..79a26a8 100644
--- a/extras/tables/table-ldap/table_ldap.c
+++ b/extras/tables/table-ldap/table_ldap.c
@@ -122,13 +122,16 @@ ldap_connect(const char *addr)
if (fd == -1)
continue;
- if (connect(fd, res->ai_addr, res->ai_addrlen) == 0)
+ if (connect(fd, res->ai_addr, res->ai_addrlen) == 0) {
+ aldap_free_url();
return aldap_init(fd);
+ }
close(fd);
fd = -1;
}
+ aldap_free_url();
return NULL;
}
--
2.39.2
From dafbf8547e57b0f7142c8faf6ef776933502d151 Mon Sep 17 00:00:00 2001
From: Philipp Takacs
Date: Thu, 25 Jan 2024 12:47:46 +0100
Subject: [PATCH 3/3] table-ldap aldap_parse_url now saves the port as string
---
extras/tables/table-ldap/aldap.c | 3 ++-
extras/tables/table-ldap/aldap.h | 4 ++--
extras/tables/table-ldap/table_ldap.c | 14 --
3 files changed, 8 insertions(+), 13 deletions(-)
diff --git a/extras/tables/table-ldap/aldap.c b/extras/tables/table-ldap/aldap.c
index 552ea52..011a820 100644
--- a/extras/tables/table-ldap/aldap.c
+++ b/extras/tables/table-ldap/aldap.c
@@ -588,9 +588,10 @@ aldap_parse_url(char *url, struct aldap_url *lu)
/* if a port is given */
if (*(forward2+1) != '\0') {
#define PORT_MAX UINT16_MAX
- lu->port = strtonum(++forward2, 0, PORT_MAX, );
+ strtonum(++forward2, 0, PORT_MAX, );
if (errstr)
goto fail;
+ lu->port = forward2;
}
} else {
lu->port = LDAP_PORT;
diff --git a/extras/tables/table-ldap/aldap.h b/extras/tables/table-ldap/aldap.h
index 7cfd637..7217634 100644
--- a/extras/tables/table-ldap/aldap.h
+++ b/extras/tables/table-ldap/aldap.h
@@ -19,7 +19,7 @@
#include "ber.h"
#define LDAP_URL "ldap://;
-#define LDAP_PORT 389
+#define LDAP_PORT "389"
#define LDAP_PAGED_OID "1.2.840.113556.1.4.319"
struct aldap {
@@ -71,7 +71,7 @@ enum aldap_protocol {
struct aldap_url {
int protocol;
char *host;
- in_port_t port;
+ char *port;
char *dn;
#define MAXATTR 1024
char *attributes[MAXATTR];
diff --git a/extras/tables/table-ldap/table_ldap.c b/extras/tables/table-ldap/table_ldap.c
index 79a26a8..0f25c60 100644
--- a/extras/tables/table-ldap/table_ldap.c
+++ b/extras/tables/table-ldap/table_ldap.c
@@ -85,8 +85,8 @@ ldap_connect(const char *addr)
{
struct aldap_url lu;
struct addrinfo hints, *res0, *res;
- char *buf, port[32];
- int error, r, fd = -1;
+ char *buf;
+ int error, fd = -1;
if ((buf =
Re: opensmtpd-extra: simplify getaddrinfo() usage
[2024-01-25 10:15] Omar Polo
> On 2024/01/24 08:51:06 +0100, Philipp wrote:
> > [2024-01-24 00:09] Omar Polo
> > > [...]
> > > if you're interested in this however, we can also avoid the strdup()
> > > here since aldap_parse_url() already strdup()s the string for parsing
> > > (but still frees the passed argument...)
> >
> > I have written two patches for this, one adding the free and one to
> > avoid the unnecessary strdup.
> >
> > Ass you might guess from the filenames, there are a few more patches. I'll
> > send the rest after I have tested all my patches.
>
> They all read fine to me. only one nitpick in the first one
>
> > From fa4cdb0a74c3b5d17cdc93b6285d765fda084740 Mon Sep 17 00:00:00 2001
> > From: Philipp Takacs
> > Date: Wed, 24 Jan 2024 01:16:56 +0100
> > Subject: [PATCH 11/11] table-ldap aldap_parse_url now saves the port as
> > string
> > [...]
> > --- a/extras/tables/table-ldap/table_ldap.c
> > +++ b/extras/tables/table-ldap/table_ldap.c
> > @@ -118,7 +118,6 @@ ldap_connect(const char *addr)
> > {
> > struct aldap_url lu;
> > struct addrinfo hints, *res0, *res;
> > - char port[32];
> > int error, r, fd = -1;
>
> Here you could remove `r' too since it becomes unused.
Thanks fixed.
> If you've tested, I could commit my getaddrinfo() diff, then if you
> rebase these one on top of it I could merge them. (but it's also fine to
> get the ldaps one in first, depending on how you prefer.)
I have tested now your getaddrinfo rewrite and my two fixups.
I have had some changes because they had been somewere in
my rebase branche. clean versions of them are attached.
Philipp
From d4fa732b5375e0fa8eaa0727ae82362c941bf8c1 Mon Sep 17 00:00:00 2001
From: Philipp Takacs
Date: Tue, 23 Jan 2024 13:58:47 +0100
Subject: [PATCH 2/3] table-ldap free aldap_url
---
extras/tables/table-ldap/aldap.c | 1 -
extras/tables/table-ldap/table_ldap.c | 5 -
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/extras/tables/table-ldap/aldap.c b/extras/tables/table-ldap/aldap.c
index d54a90c..552ea52 100644
--- a/extras/tables/table-ldap/aldap.c
+++ b/extras/tables/table-ldap/aldap.c
@@ -560,7 +560,6 @@ void
aldap_free_url(struct aldap_url *lu)
{
free(lu->buffer);
- free(lu->filter);
}
int
diff --git a/extras/tables/table-ldap/table_ldap.c b/extras/tables/table-ldap/table_ldap.c
index 6bdce67..79a26a8 100644
--- a/extras/tables/table-ldap/table_ldap.c
+++ b/extras/tables/table-ldap/table_ldap.c
@@ -122,13 +122,16 @@ ldap_connect(const char *addr)
if (fd == -1)
continue;
- if (connect(fd, res->ai_addr, res->ai_addrlen) == 0)
+ if (connect(fd, res->ai_addr, res->ai_addrlen) == 0) {
+ aldap_free_url();
return aldap_init(fd);
+ }
close(fd);
fd = -1;
}
+ aldap_free_url();
return NULL;
}
--
2.39.2
From a7c50eb250bfbb79957b4edf615cdec3dd96560f Mon Sep 17 00:00:00 2001
From: Philipp Takacs
Date: Thu, 25 Jan 2024 12:47:46 +0100
Subject: [PATCH 3/3] table-ldap aldap_parse_url now saves the port as string
---
extras/tables/table-ldap/aldap.c | 3 ++-
extras/tables/table-ldap/aldap.h | 4 ++--
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/extras/tables/table-ldap/aldap.c b/extras/tables/table-ldap/aldap.c
index 552ea52..011a820 100644
--- a/extras/tables/table-ldap/aldap.c
+++ b/extras/tables/table-ldap/aldap.c
@@ -588,9 +588,10 @@ aldap_parse_url(char *url, struct aldap_url *lu)
/* if a port is given */
if (*(forward2+1) != '\0') {
#define PORT_MAX UINT16_MAX
- lu->port = strtonum(++forward2, 0, PORT_MAX, );
+ strtonum(++forward2, 0, PORT_MAX, );
if (errstr)
goto fail;
+ lu->port = forward2;
}
} else {
lu->port = LDAP_PORT;
diff --git a/extras/tables/table-ldap/aldap.h b/extras/tables/table-ldap/aldap.h
index 7cfd637..7217634 100644
--- a/extras/tables/table-ldap/aldap.h
+++ b/extras/tables/table-ldap/aldap.h
@@ -19,7 +19,7 @@
#include "ber.h"
#define LDAP_URL "ldap://;
-#define LDAP_PORT 389
+#define LDAP_PORT "389"
#define LDAP_PAGED_OID "1.2.840.113556.1.4.319"
struct aldap {
@@ -71,7 +71,7 @@ enum aldap_protocol {
struct aldap_url {
int protocol;
char *host;
- in_port_t port;
+ char *port;
char *dn;
#define MAXATTR 1024
char *attributes[MAXATTR];
--
2.39.2
Re: opensmtpd-extra: simplify getaddrinfo() usage
On 2024/01/24 08:51:06 +0100, Philipp wrote:
> [2024-01-24 00:09] Omar Polo
> > [...]
> > if you're interested in this however, we can also avoid the strdup()
> > here since aldap_parse_url() already strdup()s the string for parsing
> > (but still frees the passed argument...)
>
> I have written two patches for this, one adding the free and one to
> avoid the unnecessary strdup.
>
> Ass you might guess from the filenames, there are a few more patches. I'll
> send the rest after I have tested all my patches.
They all read fine to me. only one nitpick in the first one
> From fa4cdb0a74c3b5d17cdc93b6285d765fda084740 Mon Sep 17 00:00:00 2001
> From: Philipp Takacs
> Date: Wed, 24 Jan 2024 01:16:56 +0100
> Subject: [PATCH 11/11] table-ldap aldap_parse_url now saves the port as string
> [...]
> --- a/extras/tables/table-ldap/table_ldap.c
> +++ b/extras/tables/table-ldap/table_ldap.c
> @@ -118,7 +118,6 @@ ldap_connect(const char *addr)
> {
> struct aldap_url lu;
> struct addrinfo hints, *res0, *res;
> - char port[32];
> int error, r, fd = -1;
Here you could remove `r' too since it becomes unused.
If you've tested, I could commit my getaddrinfo() diff, then if you
rebase these one on top of it I could merge them. (but it's also fine to
get the ldaps one in first, depending on how you prefer.)
Thanks,
Omar Polo
Re: ldaps support for table-ldap
On 2024/01/24 09:38:01 +0100, Philipp wrote:
> Hi Omar
>
> Thanks for the feedback. A updated patch is attached.
>
> [2024-01-23 11:26] Omar Polo
> > On 2024/01/23 01:24:57 +0100, Philipp wrote:
> > > I have had a bit of time and implemented ldaps support for table-ldap.
> > > It is currently untested and has some todos. But I would say it's
> > > complete enough to share. So other can comment on the code. A patch
> > > is attached
> >
> > I don't use ldap and completely lack the experience with it, so I can
> > only provide some feedback on the code itself, not if it makes sense to
> > have TLS in here nor provide testing.
>
> As said in the other thread I'll test this tomorrow.
thanks :)
> > [...]
> > > /* XXX this should be moved to a different function */
> > > - if (ber->fd != -1)
> > > + if (ber->fd != -1) {
> > > + if (ber->tls_ctx) {
> > > + return tls_write(ber->tls_ctx, ber->br_wbuf, len);
> >
> > This, and the other calls to tls_read are wrong. Even in the blocking
> > case, we need to wrap these in a loop to handle TLS_WANT_POLLIN and
> > TLS_WANT_POLLOUT, as per the manpage example.
> >
> > I remember being bitten by this, as libretls (libtls over openssl)
> > returns the TLS_WANT_* more often than LibreSSL' libtls.
>
> A good to know. I'm a bit unsure about my solution. Do I need to acualy
> poll() in the read/write wrapper or is it good enough to just call it in
> a loop?
since we're using blocking i/o just calling it in a loop is enough.
poll(2) here would just be wasting time for a syscall with no real
benefit.
> > > [...]
> > > +static struct tls *
> > > +ldaps_connect(int fd, char *hostname)
> > > +{
> > > + /* XXX log */
> > > + struct tls *ctx = NULL;
> > > + struct tls_config *config = tls_config_new();
> > > + if (!config)
> > > + goto fail;
> > > + if (tls_config_set_ca_path(config, "/etc/ssl/certs") == -1)
> > > + goto fail;
> >
> > Is this needed? out-of-the-box on OpenBSD this directory doesn't exist,
> > but don't know if it's the "de-facto" place to store certs for ldap. In
> > any case, I think it should be at least configurable.
>
> This is WIP, I have just added the default ca store of debian. I coudn't
> find if libtls does this by default. A config option for a ca file is
> in planing.
If it's the default store then you don't need to explicitly add it.
You'll only need this to add a non-standard ca path.
> > > [...]
> > > + tls_config_free(config);
> > > + return ctx;
> > > +fail:
> > > + tls_close(ctx);
> > > + tls_free(ctx);
> > > + tls_config_free(config);
> >
> > these close/free calls needs to be guarded since ctx and config may be
> > NULL when here and these functions will deference the argument.
>
> Only for the tls_close, tls_free() and tls_config_free() behave like free()
> and are noops with a NULL as argument.
yeah, i was thinking about close and extended the though to the others.
> > > + return NULL;
> > > +}
> > > +
> > > static struct aldap *
> > > ldap_connect(const char *addr)
> > > {
> > > @@ -121,13 +150,25 @@ ldap_connect(const char *addr)
> >
> > ouch... ldap_connect is really a bit over-complicate. I'd prefer if we
> > simplify this a bit first. I'll soon send a diff for this.
>
> Nice, I have rebased my patch on the cleanup.
A few more comments on the diff.
by the way, this partially failed to apply on top of my getaddrinfo()
diff. If you prefer, I can push it so you can rebase against it.
also, depending on how you prefer, I don't want you to rebase commits
too much, but we could commit the other bunch of diffs first, and then
get to this one.
> --- a/extras/tables/table-ldap/aldap.c
> +++ b/extras/tables/table-ldap/aldap.c
> [...]
> @@ -55,6 +56,12 @@ voidldap_debug_elements(struct
> ber_element *);
> int
> aldap_close(struct aldap *al)
> {
> + if (al->ber.tls_ctx) {
> + if (tls_close(al->ber.tls_ctx) == -1)
> + return (-1);
I forgot that tls_close() also has the same behaviour of handshake, read
and write and to properly close the connection should be called in a
loop (this because I think it needs to do I/O for the close_notify
thingy.)
btw, for now it's fine but we shouldn't leak the context nor the file
descriptor just because of a failure. The behaviour is already here,
so it's fine to leave this like it is and look at it later.
> + tls_free(al->ber.tls_ctx);
> + }
> +
> if (close(al->ber.fd) == -1)
> return (-1);
>
> @@ -65,13 +72,14 @@ aldap_close(struct aldap *al)
> }
>
> struct aldap *
> -aldap_init(int fd)
> +aldap_init(int fd, struct tls *ctx)
> {
> struct aldap *a;
>
> if ((a = calloc(1, sizeof(*a))) == NULL)
> return NULL;
> a->ber.fd = fd;
> + a->ber.tls_ctx = ctx;
>
> return a;
> }
> @@ -574,10 +582,15 @@ aldap_parse_url(char *url, struct aldap_url *lu)
> p = lu->buffer;
>
> /* protocol */
>
