Re: ITS#8654 - Option for LDAP client to bind to a local address

[Ryan Tandy <ryan@nardis.ca>]

On Mon, Jun 12, 2017 at 10:15:56PM +0000, Daniel Le wrote:
> Please review the code change. The diff is against the master branch of 
> git://git.openldap.org/openldap.git.

I'm not able to apply the patch from this email. The whitespace in the 
context has been mangled - your mail only contains spaces, where the 
original files contain tabs. My review is therefore only based on 
reading the code, not testing it. Please attach the actual git commit 
(use git-format-patch(1)), or link to a branch in a git repository.

I don't know a lot of this code very well, so please forgive any stupid 
questions and try to answer patiently. :)

> In os-ip.c, the ldap_socket_bind_addr() function is only called to 
> effectively bind LDAP client socket to a local IP address if 
> HAVE_GETADDRINFO and HAVE_INET_NTOP are defined.

OK. Does the code still compile and behave reasonably if they aren't? 
I'm wondering if it still makes sense to compile any of the contents of 
addr.c or even ldap_socket_bind_addr itself, for example, if getaddrinfo 
is missing. And I wonder also about the expected return value of 
ldap_set_option() when it's not actually possible to respect the 
requested setting.

> Changes to be committed:
>  (use "git reset HEAD <file>..." to unstage)
>
>        new file:   addr.c
>
> Changes not staged for commit:
>  (use "git add <file>..." to update what will be committed)
>  (use "git checkout -- <file>..." to discard changes in working directory)
>
>        modified:   ../../include/ldap.h
>        modified:   Makefile.in
>        modified:   ldap-int.h
>        modified:   options.c
>        modified:   os-ip.c

Please also update the ldap_get_option.3 man page. (And hopefully I 
don't need to say this, but I expect Microsoft's documentation is 
copyrighted and simply cloning or paraphrasing it would be a bad idea.)

> include$git diff ldap.h
> diff --git a/include/ldap.h b/include/ldap.h
> index 588e906..0268f0e 100644
> --- a/include/ldap.h
> +++ b/include/ldap.h
> @@ -109,6 +109,8 @@ LDAP_BEGIN_DECL
> #define LDAP_OPT_DIAGNOSTIC_MESSAGE            0x0032
> #define LDAP_OPT_ERROR_STRING                  LDAP_OPT_DIAGNOSTIC_MESSAGE
> #define LDAP_OPT_MATCHED_DN                    0x0033
> +/* same option code as Microsoft LDAP_OPT_SOCKET_BIND_ADDRESSES */
> +#define LDAP_OPT_BIND_ADDRESSES                 0x0044

Wouldn't it be more compatible to name the option the same?

> /* 0x0034 - 0x3fff not defined */
> /* 0x0091 used by Microsoft for LDAP_OPT_AUTO_RECONNECT */
> #define LDAP_OPT_SSPI_FLAGS                    0x0092
> @@ -815,6 +817,15 @@ typedef struct ldap_url_desc {
> #define LDAP_URL_ERR_BADEXTS   0x0a    /* bad or missing extensions */
>
> /*
> + *  data type for ldap socket bind addresses
> + */
> +typedef struct ldap_bind_addr {
> +        struct ldap_bind_addr *lba_next;
> +        char   *lba_address;
> +        int    lba_family;
> +} LDAPBindAddr;

Does this definition need to be exposed to the world? Could it be in 
ldap-int.h?

<snip ...>

> libldap$git diff os-ip.c
> diff --git a/libraries/libldap/os-ip.c b/libraries/libldap/os-ip.c
> index c7cee92..fbfb8a9 100644
> --- a/libraries/libldap/os-ip.c
> +++ b/libraries/libldap/os-ip.c
> @@ -209,6 +209,50 @@ ldap_int_prepare_socket(LDAP *ld, int s, int proto )
>        return 0;
> }
>
> +static int
> +ldap_socket_bind_addr(LDAP *ld, int s, int sf, int st)
> +{
> +       LDAPBindAddr **ba;
> +       LDAPBindAddr *bap;

Hmm. The double-pointer is the one with fewer "p"s in its name? :)

> +       struct addrinfo hints, *bai;
> +       int err;
> +       int matched = 0;
> +
> +       for ( ba = &ld->ld_options.ldo_bind_addr; *ba != NULL; ba = &(*ba)->lba_next ) {

What's the use of the double pointer when we're only reading the list? I 
know this idiom is helpful when we want to modify the list, but that's 
not the case here, right? The addr.c functions later on also only use a 
single pointer.

> +               bap = *ba;
> +               if ( bap->lba_family == sf ) {
> +                       matched = 1;
> +                       break;
> +               }
> +       }
> +
> +       if ( !matched ) {

This looks like it could perhaps just be if (*ba == NULL), i.e. we fell 
off the end of the list?

> +               osip_debug(ld, "ldap_socket_bind_addr: no match\n", 0, 0, 0);
> +               return -1;
> +       }
> +
> +       memset( &hints, 0, sizeof(hints) );
> +       hints.ai_flags = AI_ADDRCONFIG;

Not AI_NUMERICHOST?

(I'm not necessarily looking for one answer or the other here; just a 
rationale, and the chosen behaviour to be documented.)

> +       hints.ai_family = sf;
> +       hints.ai_socktype = st;

Did you also test this with a UDP (cldap://) socket? I can't think why 
it wouldn't work, but I'd like to be certain. :)

> +
> +       err = getaddrinfo( bap->lba_address, NULL, &hints, &bai );

Assuming we're _not_ using AI_NUMERICHOST, then does this need to be 
holding the ldap_int_resolv_mutex?

> +       if ( err != 0 ) {
> +               osip_debug( ld, "ldap_socket_bind_addr: %s getaddrinfo error %s\n",
> +                           bap->lba_address, AC_GAI_STRERROR(err), 0 );
> +               return -1;
> +       }
> +
> +       if ( bind( s, bai->ai_addr, bai->ai_addrlen )) {
> +               osip_debug( ld, "ldap_socket_bind_addr: %s bind error %s\n",
> +                           bap->lba_address, strerror(errno), 0 );

Should probably be sock_errstr( sock_errno() )?

Bearing in mind that osip_debug is only TRACE level:
- should we ldap_pvt_set_errno() if bind() fails?
- can we do anything sensible with the getaddrinfo() result?

> +               return -1;
> +       }
> +
> +       osip_debug( ld, "ldap_socket_bind_addr: %s bind success\n", bap->lba_address, 0, 0 );
> +       return 0;
> +}
> +
> #ifndef HAVE_WINSOCK
>
> #undef TRACE
> @@ -655,6 +699,8 @@ ldap_connect_to_host(LDAP *ld, Sockbuf *sb,
>                        } break;
>                }
>
> +               ldap_socket_bind_addr( ld, s, sai->ai_family, socktype);
> +
>                rc = ldap_pvt_connect( ld, s,
>                        sai->ai_addr, sai->ai_addrlen, async );
>                if ( rc == 0 || rc == -2 ) {
>
> libldap$cat addr.c
> /* LIBLDAP addr.c -- Utility functions for socket bind addresses */

Maybe "bindaddr.c"? "addr.c" sounds more generic - I would expect it to 
contain utilities for working with addresses in general, while this code 
is for a pretty specific purpose.

I'd drop "LIBLDAP"; seems to be a url.c'ism, not a pattern.

> /* $OpenLDAP$ */
> /* This work is part of OpenLDAP Software <http://www.openldap.org/>.
> *
> * Copyright 1998-2017 The OpenLDAP Foundation.

"1998-2017" seems slightly odd for new code, as does the "Portions 
Copyright (c) 1996" below. I admit though that I'm not really sure what 
standard practice is here for new code derived from older.

> * All rights reserved.
> *
> * Redistribution and use in source and binary forms, with or without
> * modification, are permitted only as authorized by the OpenLDAP
> * Public License.
> *
> * A copy of this license is available in the file LICENSE in the
> * top-level directory of the distribution or, alternatively, at
> * <http://www.OpenLDAP.org/license.html>.
> */
> /* Portions Copyright (c) 1996 Regents of the University of Michigan.
> * All rights reserved.
> */

<snip ...>

> int
> ldap_parse_bind_addr( LDAPBindAddr **lba_list, const char *addresses )
> {
>        int  i;
>        char **specs;
>        LDAPBindAddr *lba_ptr;
>
>        assert(lba_list != NULL);
>        assert(addresses != NULL);
>
>        *lba_list = NULL;
>
>        specs = ldap_str2charray(addresses, ", ");
>        if (specs == NULL) {
>                return LDAP_NO_MEMORY;
>        }
>
>        /* Count the number of local addresses */
>        for (i = 0; specs[i] != NULL; i++) /* NO-OP */;
>
>        /* Put the addresses in the list backward */
>        while (--i >= 0) {

Any reason for that, besides reusing the existing 'i'?

>                lba_ptr = LDAP_CALLOC(1, sizeof(LDAPBindAddr));
>                if (lba_ptr == NULL) {
>                        ldap_charray_free(specs);
>                        ldap_free_bind_addr_list(*lba_list);
>                        *lba_list = NULL;
>                        return LDAP_NO_MEMORY;
>                }
>
>                lba_ptr->lba_address = specs[i];
>                specs[i] = NULL;
>
>                if (strchr(lba_ptr->lba_address, ':') != NULL) {
>                        lba_ptr->lba_family = AF_INET6;

Does this need to be hidden behind LDAP_PF_INET6?

Do you need actually need lba_family? os-ip.c has:

#if defined( HAVE_GETADDRINFO ) && defined( HAVE_INET_NTOP )
#  ifdef LDAP_PF_INET6
int ldap_int_inet4or6 = AF_UNSPEC;
#  else
int ldap_int_inet4or6 = AF_INET;
#  endif
#endif

I think I sort of see how you arrived at this: storing the string lets 
ldap_list_bind_addr return the same one again later; and if you store 
the string and not a family, then the loop in ldap_socket_bind_addr 
suddenly starts making a lot more getaddrinfo() calls. But the strchr() 
is bugging me; I'd rather see getaddrinfo() responsible for the parsing 
if possible.

>                } else {
>                        lba_ptr->lba_family = AF_INET;
>                }
>
>                lba_ptr->lba_next = *lba_list;
>                *lba_list = lba_ptr;
>        }
>
>        /* This should be an array of NULLs now */
>        ldap_charray_free(specs);
>
>        return LDAP_SUCCESS;
> }
>
> char *
> ldap_list_bind_addr ( LDAPBindAddr *lba_list )
> {
>        LDAPBindAddr *lba_ptr;
>        int size;
>        char *s, *p;
>
>        if (lba_list == NULL) {
>                return NULL;
>        }
>
>        /* Figure out how big the string of bind addresses is */
>        size = 1; /* nul-terminated string */

If there are any items, you already have a place for the nul: 
overwriting the final separator. Therefore I wonder if this (size=1) 
could just be a special case when there are 0 elements?

>        for (lba_ptr = lba_list; lba_ptr != NULL; lba_ptr = lba_ptr->lba_next) {
>                if (lba_ptr->lba_address == NULL) {
>                        continue;
>                }
>                size += strlen(lba_ptr->lba_address) + 1;  /* address and space or comma */

Seems most likely to be a space in this implementation, but yes ;)

>        }
>
>        s = LDAP_MALLOC(size);
>        if (s == NULL) {
>                return NULL;
>        }
>
>        p = s;
>        for (lba_ptr = lba_list; lba_ptr != NULL; lba_ptr = lba_ptr->lba_next) {
>                if (lba_ptr->lba_address == NULL) {
>                        continue;
>                }
>                strcpy(p, lba_ptr->lba_address);
>                p += strlen(lba_ptr->lba_address);
>                *p++ = ' ';
>        }
>
>        if (p != s)
>                p--;  /* nuke that extra space or comma */
>
>        *p = '\0';
>        return s;
> }

The overall design makes sense to me. These comments were largely 
nit-picks.

Hope this helps,

Ryan