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.
> [...]
> > @@ -69,15 +71,15 @@ enum aldap_protocol {
> > };
> >
> > struct aldap_url {
> > - int protocol;
> > - char *host;
> > - in_port_t port;
> > - char *dn;
> > + enum aldap_protocol protocol;
>
> tbh I would have just kept 'int protocol' here. not sure if specifying
> the enum and re-indent everything really helps. but i don't object it :)
I just have seen that the 'enum protocol' is not used in the struct and
because I had the file allready open I just changed it. This doesn't fix
any bug. It's only a bit cleaner code.
> > + char *host;
> > + in_port_t 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 {
> > [...]
> > @@ -784,8 +785,12 @@ 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(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?
> > [...]
> > @@ -80,6 +81,34 @@ table_ldap_fetch(int service, struct dict *params, char
> > *dst, size_t sz)
> > return -1;
> > }
> >
> > +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 ((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(ctx) == -1)
> > + goto fail;
>
> tls_handshake() needs almost the same loop as tls_write and tls_read to
> handle TLS_WANT_POLLIN/OUT.
>
> > + 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.
> > + 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.
From 4a49cdea6ff08ed758544022ca0842b1c4a19076 Mon Sep 17 00:00:00 2001
From: Philipp Takacs <phil...@bureaucracy.de>
Date: Tue, 23 Jan 2024 00:55:23 +0100
Subject: [PATCH 03/11] table-ldap add ldaps support
untested
based on libtls, autohell is ugly ass hell
need also some log messages
---
configure.ac | 2 +-
extras/tables/table-ldap/Makefile.am | 1 +
extras/tables/table-ldap/aldap.c | 24 +++++++++--
extras/tables/table-ldap/aldap.h | 20 +++++----
extras/tables/table-ldap/ber.c | 26 +++++++++---
extras/tables/table-ldap/ber.h | 1 +
extras/tables/table-ldap/table_ldap.c | 42 ++++++++++++++++++-
extras/tables/table-ldap/tlswrapper.c | 60 +++++++++++++++++++++++++++
extras/tables/table-ldap/tlswrapper.h | 3 ++
9 files changed, 158 insertions(+), 21 deletions(-)
create mode 100644 extras/tables/table-ldap/tlswrapper.c
create mode 100644 extras/tables/table-ldap/tlswrapper.h
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/Makefile.am b/extras/tables/table-ldap/Makefile.am
index f2ecba2..6a4f6c2 100644
--- a/extras/tables/table-ldap/Makefile.am
+++ b/extras/tables/table-ldap/Makefile.am
@@ -7,3 +7,4 @@ table_ldap_SOURCES = $(SRCS)
table_ldap_SOURCES += table_ldap.c
table_ldap_SOURCES += aldap.c
table_ldap_SOURCES += ber.c
+table_ldap_SOURCES += tlswrapper.c
diff --git a/extras/tables/table-ldap/aldap.c b/extras/tables/table-ldap/aldap.c
index 552ea52..edd3cc3 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,12 @@ void ldap_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);
+ 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 */
- 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)
@@ -594,6 +607,9 @@ aldap_parse_url(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 7cfd637..fec106b 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;
- in_port_t port;
- char *dn;
+ enum aldap_protocol protocol;
+ char *host;
+ in_port_t 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..21a1924 100644
--- a/extras/tables/table-ldap/ber.c
+++ b/extras/tables/table-ldap/ber.c
@@ -24,12 +24,14 @@
#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"
+#include "tlswrapper.h"
#define MINIMUM(a, b) (((a) < (b)) ? (a) : (b))
@@ -784,8 +786,12 @@ 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,8 +1239,12 @@ ber_getc(struct ber *b, unsigned char *c)
*/
if (b->fd == -1)
r = ber_readbuf(b, c, 1);
- else
- r = read(b->fd, 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 +1263,12 @@ ber_read(struct ber *ber, void *buf, size_t len)
while (remain > 0) {
if (ber->fd == -1)
r = ber_readbuf(ber, b, remain);
- else
- r = read(ber->fd, 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 79a26a8..3ea17a0 100644
--- a/extras/tables/table-ldap/table_ldap.c
+++ b/extras/tables/table-ldap/table_ldap.c
@@ -26,10 +26,12 @@
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
+#include <tls.h>
#include <unistd.h>
#include <smtpd-api.h>
#include "aldap.h"
+#include "tlswrapper.h"
#define MAX_LDAP_IDENTIFIER 32
#define MAX_LDAP_URL 256
@@ -80,6 +82,36 @@ table_ldap_fetch(int service, struct dict *params, char *dst, size_t sz)
return -1;
}
+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;
+ /* XXX fix needed, build time config and run time for ca_file */
+ if (tls_config_set_ca_path(config, "/etc/ssl/certs") == -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, fd) == -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)
{
@@ -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;
+ }
+ }
aldap_free_url(&lu);
- return aldap_init(fd);
+ return aldap_init(fd, ctx);
}
close(fd);
diff --git a/extras/tables/table-ldap/tlswrapper.c b/extras/tables/table-ldap/tlswrapper.c
new file mode 100644
index 0000000..03487ec
--- /dev/null
+++ b/extras/tables/table-ldap/tlswrapper.c
@@ -0,0 +1,60 @@
+#include <poll.h>
+#include <tls.h>
+#include "tlswrapper.h"
+
+int
+tls_handshake_wrapper(struct tls *ctx, int fd)
+{
+ struct pollfd pfd[1];
+ pfd[0].fd = fd;
+
+ while (1) {
+ switch (tls_handshake(ctx)) {
+ case -1:
+ return -1;
+ case TLS_WANT_POLLIN:
+ pfd[0].events = POLLIN;
+ break;
+ case TLS_WANT_POLLOUT:
+ pfd[0].events = POLLOUT;
+ break;
+ default:
+ return 0;
+ }
+ poll(pfd, 1, -1);
+ }
+}
+
+ssize_t
+tls_write_wrapper(struct tls *ctx, const void *buf, size_t buflen)
+{
+ ssize_t ret;
+ ssize_t len = buflen;
+
+ while (len > 0) {
+ ret = tls_write(ctx, buf, len);
+ if (ret == TLS_WANT_POLLIN || ret == TLS_WANT_POLLOUT)
+ continue;
+ if (ret == -1)
+ return ret;
+ buf += ret;
+ len -= ret;
+ }
+ return buflen;
+}
+
+ssize_t
+tls_read_wrapper(struct tls *ctx, void *buf, size_t buflen)
+{
+ ssize_t rlen = 0;
+ ssize_t ret;
+ while (rlen <= 0) {
+ ret = tls_read(ctx, buf, buflen);
+ if (ret == -1)
+ return -1;
+ if (ret == TLS_WANT_POLLIN || ret == TLS_WANT_POLLOUT)
+ continue;
+ rlen += ret;
+ }
+ return rlen;
+}
diff --git a/extras/tables/table-ldap/tlswrapper.h b/extras/tables/table-ldap/tlswrapper.h
new file mode 100644
index 0000000..02aa02d
--- /dev/null
+++ b/extras/tables/table-ldap/tlswrapper.h
@@ -0,0 +1,3 @@
+int tls_handshake_wrapper(struct tls *ctx, int fd);
+ssize_t tls_write_wrapper(struct tls *ctx, const void *buf, size_t buflen);
+ssize_t tls_read_wrapper(struct tls *ctx, void *buf, size_t buflen);
--
2.39.2
