[2024-01-25 10:08] Omar Polo <o...@omarpolo.com>
> On 2024/01/24 09:38:01 +0100, Philipp <phil...@bureaucracy.de> wrote:
> > Hi Omar
> >
> > Thanks for the feedback. A updated patch is attached.
> >
> > [2024-01-23 11:26] Omar Polo <o...@omarpolo.com>
> > > On 2024/01/23 01:24:57 +0100, Philipp <phil...@bureaucracy.de> 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 :)
I have tested this now and t works fine. The only thing I see is left
are the log messages.
> > > [...]
> > > > /* 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.
I have changed all wrapper to a simple loop which waits till one sucessfull
call or an error. Because the normal read/write call also only make one call.
>
> > > > [...]
> > > > +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.
Ok, I have added a CA-file config option.
> > > > + 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.
Yes I mixed in some other patches, the attached on should work.
> 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.
>
> [...]
> > @@ -123,8 +155,16 @@ ldap_connect(const char *addr)
> > continue;
> >
> > if (connect(fd, res->ai_addr, res->ai_addrlen) == 0) {
> > + struct tls *ctx = NULL;
> > + if (lu.protocol == LDAPS) {
> > + if ((ctx = ldaps_connect(fd, lu.host)) == NULL)
> > {
> > + close(fd);
> > + fd = -1;
> > + continue;
>
> I'm a bit unsure about this. Shouldn't we consider a failure in setting
> up the TLS layer fatal?
No, because when the table-process dies smtpd would die to. So the monitoring
mail
that the ldap server has an outdated cert wouldn't reach the admins ;-)
Also there could be a secound working ldap server on the same domain.
> [...]
> > +ssize_t
> > +tls_read_wrapper(struct tls *ctx, void *buf, size_t buflen)
> > +{
> > + ssize_t rlen = 0;
> > + ssize_t ret;
> > + while (rlen <= 0) {
>
> here it should be buflen, not rlen, and it should be just > 0
>
> while (buflen > 0) {
>
> > + ret = tls_read(ctx, buf, buflen);
> > + if (ret == -1)
> > + return -1;
> > + if (ret == TLS_WANT_POLLIN || ret == TLS_WANT_POLLOUT)
> > + continue;
> > + rlen += ret;
>
> here we should also do
>
> buf += ret;
> buflen -= ret;
>
> > + }
> > + return rlen;
> > +}
Do you mix this up with write? Because this solution would block till
the buffer is full, but if there is nothing more to recive it would
block endless.
Philipp
From 56c18f72c8debc67a88bc986b485cf927c5eab5b Mon Sep 17 00:00:00 2001
From: Philipp Takacs <phil...@bureaucracy.de>
Date: Tue, 23 Jan 2024 00:55:23 +0100
Subject: [PATCH] table-ldap add ldaps support
based on libtls, autohell is ugly ass hell
need also some log messages
---
configure.ac | 2 +-
extras/tables/table-ldap/aldap.c | 26 +++++++++++--
extras/tables/table-ldap/aldap.h | 20 +++++-----
extras/tables/table-ldap/ber.c | 35 ++++++++++++++++-
extras/tables/table-ldap/ber.h | 1 +
extras/tables/table-ldap/table_ldap.c | 54 ++++++++++++++++++++++++++-
6 files changed, 120 insertions(+), 18 deletions(-)
diff --git a/configure.ac b/configure.ac
index 410a61b..14608b1 100644
--- a/configure.ac
+++ b/configure.ac
@@ -577,7 +577,7 @@ AC_ARG_WITH([libssl],
]
)
## XXX chl -lssl manually added
-LIBS="-lcrypto -lssl $LIBS"
+LIBS="-lcrypto -lssl -ltls $LIBS"
AC_TRY_LINK_FUNC([RAND_add], [AC_DEFINE([HAVE_OPENSSL], [1],
[Define if your ssl headers are included
with #include <openssl/header.h>])],
diff --git a/extras/tables/table-ldap/aldap.c b/extras/tables/table-ldap/aldap.c
index 8367615..c38aae5 100644
--- a/extras/tables/table-ldap/aldap.c
+++ b/extras/tables/table-ldap/aldap.c
@@ -21,6 +21,7 @@
#include <inttypes.h>
#include <string.h>
#include <stdlib.h>
+#include <tls.h>
#include <unistd.h>
#include "aldap.h"
@@ -55,6 +56,14 @@ void ldap_debug_elements(struct ber_element *);
int
aldap_close(struct aldap *al)
{
+ if (al->ber.tls_ctx) {
+ int ret;
+ do {
+ ret = tls_close(al->ber.tls_ctx);
+ } while (ret == TLS_WANT_POLLIN || ret == TLS_WANT_POLLOUT);
+ tls_free(al->ber.tls_ctx);
+ }
+
if (close(al->ber.fd) == -1)
return (-1);
@@ -65,13 +74,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 +584,15 @@ aldap_parse_url(const char *url, struct aldap_url *lu)
p = lu->buffer;
/* protocol */
- if (strncasecmp(LDAP_URL, p, strlen(LDAP_URL)) != 0)
+ if (strncasecmp(LDAP_URL, p, strlen(LDAP_URL)) == 0) {
+ lu->protocol = LDAP;
+ p += strlen(LDAP_URL);
+ } else if (strncasecmp(LDAPS_URL, p, strlen(LDAP_URL)) == 0) {
+ lu->protocol = LDAPS;
+ p += strlen(LDAPS_URL);
+ } else {
goto fail;
- lu->protocol = LDAP;
- p += strlen(LDAP_URL);
+ }
/* host and optional port */
if ((forward = strchr(p, '/')) != NULL)
@@ -595,6 +610,9 @@ aldap_parse_url(const char *url, struct aldap_url *lu)
}
} else {
lu->port = LDAP_PORT;
+ if (lu->protocol == LDAPS) {
+ lu->port = LDAPS_PORT;
+ }
}
/* fail if no host is given */
if (strlen(p) == 0)
diff --git a/extras/tables/table-ldap/aldap.h b/extras/tables/table-ldap/aldap.h
index 34810bc..6f80e2c 100644
--- a/extras/tables/table-ldap/aldap.h
+++ b/extras/tables/table-ldap/aldap.h
@@ -20,6 +20,8 @@
#define LDAP_URL "ldap://";
#define LDAP_PORT "389"
+#define LDAPS_URL "ldaps://"
+#define LDAPS_PORT "636"
#define LDAP_PAGED_OID "1.2.840.113556.1.4.319"
struct aldap {
@@ -69,15 +71,15 @@ enum aldap_protocol {
};
struct aldap_url {
- int protocol;
- char *host;
- char *port;
- char *dn;
+ enum aldap_protocol protocol;
+ char *host;
+ char *port;
+ char *dn;
#define MAXATTR 1024
- char *attributes[MAXATTR];
- int scope;
- char *filter;
- char *buffer;
+ char *attributes[MAXATTR];
+ int scope;
+ char *filter;
+ char *buffer;
};
enum protocol_op {
@@ -186,7 +188,7 @@ enum ldap_subfilter {
LDAP_FILT_SUBS_FIN = 2,
};
-struct aldap *aldap_init(int fd);
+struct aldap *aldap_init(int fd, struct tls *ctx);
int aldap_close(struct aldap *);
struct aldap_message *aldap_parse(struct aldap *);
void aldap_freemsg(struct aldap_message *);
diff --git a/extras/tables/table-ldap/ber.c b/extras/tables/table-ldap/ber.c
index 0f92bb2..94e887a 100644
--- a/extras/tables/table-ldap/ber.c
+++ b/extras/tables/table-ldap/ber.c
@@ -24,10 +24,11 @@
#include <limits.h>
#include <stdlib.h>
#include <err.h> /* XXX for debug output */
+#include <stdarg.h>
#include <stdio.h> /* XXX for debug output */
#include <string.h>
+#include <tls.h>
#include <unistd.h>
-#include <stdarg.h>
#include "ber.h"
@@ -747,6 +748,26 @@ ber_scanf_elements(struct ber_element *ber, char *fmt, ...)
}
+static ssize_t
+tls_write_wrapper(struct tls *ctx, const void *buf, size_t buflen)
+{
+ ssize_t ret;
+ do {
+ ret = tls_write(ctx, buf, buflen);
+ } while (ret == TLS_WANT_POLLIN || ret == TLS_WANT_POLLOUT);
+ return ret;
+}
+
+static ssize_t
+tls_read_wrapper(struct tls *ctx, void *buf, size_t buflen)
+{
+ ssize_t ret;
+ do {
+ ret = tls_read(ctx, buf, buflen);
+ } while (ret == TLS_WANT_POLLIN || ret == TLS_WANT_POLLOUT);
+ return ret;
+}
+
/*
* write ber elements to the socket
*
@@ -784,8 +805,13 @@ ber_write_elements(struct ber *ber, struct ber_element *root)
return -1;
/* XXX this should be moved to a different function */
- if (ber->fd != -1)
+ if (ber->fd != -1) {
+ if (ber->tls_ctx) {
+ return tls_write_wrapper(ber->tls_ctx, ber->br_wbuf,
+ len);
+ }
return write(ber->fd, ber->br_wbuf, len);
+ }
return (len);
}
@@ -1233,6 +1259,8 @@ ber_getc(struct ber *b, unsigned char *c)
*/
if (b->fd == -1)
r = ber_readbuf(b, c, 1);
+ else if (b->tls_ctx)
+ r = tls_read_wrapper(b->tls_ctx, c, 1);
else
r = read(b->fd, c, 1);
return r;
@@ -1253,8 +1281,11 @@ ber_read(struct ber *ber, void *buf, size_t len)
while (remain > 0) {
if (ber->fd == -1)
r = ber_readbuf(ber, b, remain);
+ else if (ber->tls_ctx)
+ r = tls_read_wrapper(ber->tls_ctx, b, remain);
else
r = read(ber->fd, b, remain);
+
if (r == -1) {
if (errno == EINTR || errno == EAGAIN)
continue;
diff --git a/extras/tables/table-ldap/ber.h b/extras/tables/table-ldap/ber.h
index d656508..4987fc2 100644
--- a/extras/tables/table-ldap/ber.h
+++ b/extras/tables/table-ldap/ber.h
@@ -34,6 +34,7 @@ struct ber_element {
struct ber {
int fd;
+ struct tls *tls_ctx;
unsigned char *br_wbuf;
unsigned char *br_wptr;
unsigned char *br_wend;
diff --git a/extras/tables/table-ldap/table_ldap.c b/extras/tables/table-ldap/table_ldap.c
index 0631e6a..4309fa0 100644
--- a/extras/tables/table-ldap/table_ldap.c
+++ b/extras/tables/table-ldap/table_ldap.c
@@ -26,6 +26,7 @@
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
+#include <tls.h>
#include <unistd.h>
#include <smtpd-api.h>
@@ -63,7 +64,7 @@ struct query {
static int ldap_run_query(int type, const char *, char *, size_t);
-static char *config, *url, *username, *password, *basedn;
+static char *config, *url, *username, *password, *basedn, *ca_file;
static struct aldap *aldap;
static struct query queries[LDAP_MAX];
@@ -80,6 +81,45 @@ table_ldap_fetch(int service, struct dict *params, char *dst, size_t sz)
return -1;
}
+static int
+tls_handshake_wrapper(struct tls *ctx)
+{
+ int ret;
+ do {
+ ret = tls_handshake(ctx);
+ } while (ret == TLS_WANT_POLLIN || ret == TLS_WANT_POLLOUT);
+ return ret;
+}
+
+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 (ca_file && tls_config_set_ca_file(config, ca_file) == -1)
+ goto fail;
+ if ((ctx = tls_client()) == NULL)
+ goto fail;
+ if (tls_configure(ctx, config) == -1)
+ goto fail;
+ if (tls_connect_socket(ctx, fd, hostname) == -1)
+ goto fail;
+ if (tls_handshake_wrapper(ctx) == -1)
+ goto fail;
+
+ tls_config_free(config);
+ return ctx;
+fail:
+ if (ctx)
+ tls_close(ctx);
+ tls_free(ctx);
+ tls_config_free(config);
+ return NULL;
+}
+
static struct aldap *
ldap_connect(const char *addr)
{
@@ -111,8 +151,16 @@ ldap_connect(const char *addr)
continue;
if (connect(fd, res->ai_addr, res->ai_addrlen) == 0) {
+ struct tls *ctx = NULL;
+ if (lu.protocol == LDAPS) {
+ if ((ctx = ldaps_connect(fd, lu.host)) == NULL) {
+ close(fd);
+ fd = -1;
+ continue;
+ }
+ }
aldap_free_url(&lu);
- return aldap_init(fd);
+ return aldap_init(fd, ctx);
}
close(fd);
@@ -223,6 +271,8 @@ ldap_config(void)
read_value(&password, key, value);
else if (!strcmp(key, "basedn"))
read_value(&basedn, key, value);
+ else if (!strcmp(key, "ca_file"))
+ read_value(&ca_file, key, value);
else if (!strcmp(key, "alias_filter"))
read_value(&queries[LDAP_ALIAS].filter, key, value);
else if (!strcmp(key, "alias_attributes")) {
--
2.39.2
