Re: Log LDAP "diagnostic messages"?

From: Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>
To: Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>
Cc: Pg Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Log LDAP "diagnostic messages"?
Date: 2017-07-27 05:20:45
Message-ID: CAEepm=2HVRZJsUAg+Njgkn+23_Fo=P-YeeMn9uQO+G8C0kzJ2Q@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
ldap-diagnostic-message-v2.patch application/octet-stream 4.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Masahiko Sawada 2017-07-27 05:38:29 Re: Missing comment for max_logical_replication_workers in postgresql.conf.sample
Previous Message Thomas Munro 2017-07-27 04:27:09 A couple of postgresql.conf.sample discrepancies