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 :)
> > [...]
> > > /* 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 @@ 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);
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 */
> - 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);
nit: we're not always consistent, but I'd prefer for new code to stay
under 80 columns.
> + }
> 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);
nit: no need to nest the ifs, this is equivalent and easier to read IMHO.
if (b->fd == -1)
...
else if (b->tls_ctx)
...
else
...
> + }
> 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);
> + }
same here
> 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);
I don't think that we need to wrap tls_close() in a loop here, being it
in a error path, so my earlier comment doesn't apply here :)
> + 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;
I'm a bit unsure about this. Shouldn't we consider a failure in setting
up the TLS layer fatal?
> + }
> + }
> 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 @@
I don't think we need to introduce a new file for this. handshake and
close are used only once (where it matters for looping), read and write
three times.
I would just inline these where needed.
(and in any case, when adding a new file it should have a copyright
and license block on top like the others.)
> [...]
> +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);
as said earlier, poll() is not needed, just continue looping:
switch (tls_handshake(ctx)) {
case 0:
break;
case -1:
return;
default:
continue;
}
or even just
do {
ret = tls_handshake(ctx);
} while (ret == TLS_WANT_POLLIN || ret == TLS_WANT_POLLOUT);
> [...]
> +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;
> +}
