On 2024/01/23 01:24:57 +0100, Philipp <phil...@bureaucracy.de> wrote:
> Hi
>
> 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.
> In general it would be nice, if the extras repo could get reactivated.
> Most imported would be some documentation for the existing staff.
I agree with that sentiment. There's a lot of useful stuff in there.
> From 2a2671ea2a88868ec91e24803db30935d18c081d 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
>
> untested
>
> based on libtls, autohell is ugly ass hell
don't worry about the autoconf/automake part of this. I can fix it as a
follow-up eventually (still have to take a proper look at how this repo
is built.)
> need also some log messages
> ---
> configure.ac | 2 +-
> extras/tables/table-ldap/aldap.c | 32 ++++++++++++++---
> extras/tables/table-ldap/aldap.h | 20 ++++++-----
> extras/tables/table-ldap/ber.c | 23 ++++++++++---
> extras/tables/table-ldap/ber.h | 1 +
> extras/tables/table-ldap/table_ldap.c | 49 ++++++++++++++++++++++++---
> 6 files changed, 103 insertions(+), 24 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 d54a90c..9cefe1c 100644
> --- a/extras/tables/table-ldap/aldap.c
> +++ b/extras/tables/table-ldap/aldap.c
> @@ -22,6 +22,7 @@
> #include <string.h>
> #include <stdlib.h>
> #include <unistd.h>
> +#include <tls.h>
nit: I'd keep the include sorted when possible, so tls.h should go
between stdlib.h and 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;
> }
> @@ -575,10 +583,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,7 +607,16 @@ aldap_parse_url(char *url, struct aldap_url *lu)
> goto fail;
> }
> } else {
> - lu->port = LDAP_PORT;
nit: a switch here is a bit over overly-verbose. just
lu->port = LDAP_PORT;
if (lu->protocol == LDAPS)
lp->port = LDAPS_PORT;
should do it.
> + switch (lu->protocol) {
> + case LDAP:
> + lu->port = LDAP_PORT;
> + break;
> + case LDAPS:
> + lu->port = LDAPS_PORT;
> + break;
> + default:
> + goto fail;
> + }
> }
> /* 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;
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 :)
> + 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..9bb06aa 100644
> --- a/extras/tables/table-ldap/ber.c
> +++ b/extras/tables/table-ldap/ber.c
> @@ -28,6 +28,7 @@
> #include <string.h>
> #include <unistd.h>
> #include <stdarg.h>
> +#include <tls.h>
same nit about header order.
> #include "ber.h"
>
> @@ -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.
> + }
> return write(ber->fd, ber->br_wbuf, len);
> + }
>
> return (len);
> }
> @@ -1233,8 +1238,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(b->tls_ctx, c, 1);
> + else
> + r = read(b->fd, c, 1);
> + }
> return r;
> }
>
> @@ -1253,8 +1262,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(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 090cfb4..bdfb13b 100644
> --- a/extras/tables/table-ldap/table_ldap.c
> +++ b/extras/tables/table-ldap/table_ldap.c
> @@ -27,6 +27,7 @@
> #include <stdio.h>
> #include <string.h>
> #include <unistd.h>
> +#include <tls.h>
same nit :)
> #include <smtpd-api.h>
> #include "aldap.h"
> @@ -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.
> + 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.
> + 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.
