Re: Log LDAP "diagnostic messages"?

On Wed, Jul 26, 2017 at 10:40 PM, Ashutosh Bapat
<ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
> On Wed, Jul 26, 2017 at 6:51 AM, Thomas Munro
> <thomas(dot)munro(at)enterprisedb(dot)com> wrote:
>> Something like the attached.
>
> The patch prints errdetail() as "No LDAP diagnostic message
> available." when LDAP doesn't provide diagnostics. May be some error
> messages do not have any diagnostic information. In that case above
> error detail may be confusing. May be we should just omit error
> details when diagnostic message is not available.

Agreed. Here's a version that skips those useless detail messages
using a coding pattern I found elsewhere. For example, on my system,
trying to log in with the wrong password causes an "Invalid
credentials" error with no extra "DETAIL:" line logged, but trying to
use TLS when it hasn't been configured properly logs a helpful
diagnostic message.

Thanks for the review!

I also noticed a couple of other things in passing and fixed them in
this new version:

1. In one place we call ldap_get_option(LDAP_OPT_ERROR_NUMBER) after
ldap_unbind_s(). My man page says "Once [ldap_unbind()] is called,
the connection to the LDAP server is closed, and the ld structure is
invalid." So I don't think we should do that, even if it didn't
return LDAP_SUCCESS. I have no idea if any implementation would
actually fail to unbind and what state the LDAP object would be in if
it did: this is essentially the destructor function for LDAP
connections, so what are you supposed to do after that? But using the
LDAP object again seems wrong to me.

2. In several code paths we don't call ldap_unbind() on the way out,
which is technically leaking a connection. Failure to authenticate is
FATAL to the backend anyway so it probably doesn't matter much, but
hanging up without saying goodbye is considered discourteous by
some[1].

Thoughts anyone?

[1] https://www.ldap.com/the-ldap-unbind-operation

--
Thomas Munro
http://www.enterprisedb.com