Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Add TAP tests for LDAP connection parameter lookup

  2. libpq: Set LDAP protocol version 3

  1. Update LDAP Protocol in fe-connect.c to v3

    Andrew Jackson <andrewjackson947@gmail.com> — 2025-03-22T21:10:28Z

    Currently the LDAP usage in fe-connect.c does not explicitly set the
protocol version to v3. This causes issues with many LDAP servers as they
will often require clients to use the v3 protocol and disallow any use of
the v2 protocol. Further the other usage of LDAP in postgres (in
`backend/libpq/auth.c`) uses the v3 protocol.

This patch changes fe-connect.c so that it uses the v3 protocol similar to
`backend/libpq/auth.c`.

One further note is that I do not currently see any test coverage over the
LDAP functionality in `fe-connect.c`. I am happy to add that to this patch
if needed.

  2. Re: Update LDAP Protocol in fe-connect.c to v3

    Andrew Jackson <andrewjackson947@gmail.com> — 2025-03-22T21:22:47Z

    Apologies, forgot to attach the patch in the prior email.

On Sat, Mar 22, 2025 at 4:10 PM Andrew Jackson <andrewjackson947@gmail.com>
wrote:

> Currently the LDAP usage in fe-connect.c does not explicitly set the
> protocol version to v3. This causes issues with many LDAP servers as they
> will often require clients to use the v3 protocol and disallow any use of
> the v2 protocol. Further the other usage of LDAP in postgres (in
> `backend/libpq/auth.c`) uses the v3 protocol.
>
> This patch changes fe-connect.c so that it uses the v3 protocol similar to
> `backend/libpq/auth.c`.
>
> One further note is that I do not currently see any test coverage over the
> LDAP functionality in `fe-connect.c`. I am happy to add that to this patch
> if needed.
>

  3. Re: Update LDAP Protocol in fe-connect.c to v3

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-03-22T23:10:51Z

    Andrew Jackson <andrewjackson947@gmail.com> writes:
> Currently the LDAP usage in fe-connect.c does not explicitly set the
> protocol version to v3. This causes issues with many LDAP servers as they
> will often require clients to use the v3 protocol and disallow any use of
> the v2 protocol.

This is the first complaint I can recall hearing about that, so
exactly which ones are "many"?  Also, are we really sufficiently
compliant with v3 that just adding this bit is enough?

> One further note is that I do not currently see any test coverage over the
> LDAP functionality in `fe-connect.c`. I am happy to add that to this patch
> if needed.

src/test/ldap/ doesn't do it for you?

			regards, tom lane

  4. Re: Update LDAP Protocol in fe-connect.c to v3

    Andrew Jackson <andrewjackson947@gmail.com> — 2025-03-23T03:05:59Z

    > This is the first complaint I can recall hearing about that, so
exactly which ones are "many"?

I've tested a 2 before figuring out about the v3 issue. lldap[0] and the
docker image osixia/docker-openldap[1].
- lldap  gives the following error message when I attempt to connect
without the patch "Service Error: while handling incoming messages: while
receiving LDAP op: Bind request version is not equal to 3. This is a
serious client bug.". With the attached patch this error message does not
appear
-  osixia/docker-openlap gives the following error message without the
patch "67df745e conn=1001 op=0 RESULT tag=97 err=2 text=historical protocol
version requested, use LDAPv3 instead".
"

> Also, are we really sufficiently compliant with v3 that just adding this
bit is enough?

I believe that this bit is all that is needed. Per the man page for
ldap_set_option [2]: "The protocol version used by the library defaults to
LDAPv2 (now historic), which corresponds to the LDAP_VERSION2 macro.
Application developers are encouraged to explicitly set
LDAP_OPT_PROTOCOL_VERSION to LDAPv3, using the LDAP_VERSION3 macro, or to
allow users to select the protocol version."

> src/test/ldap/ doesn't do it for you?

Looking through the tests here it seems like they are all tests for the
serverside auth functionality that is configurable in pg_hba.conf. I don't
see any tests that test the client side "LDAP Lookup of Connection
Parameters" described in [3]

[0] https://github.com/lldap/lldap
[1] https://github.com/osixia/docker-openldap
[2] https://linux.die.net/man/3/ldap
[3] https://www.postgresql.org/docs/current/libpq-ldap.html


On Sat, Mar 22, 2025 at 6:10 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Andrew Jackson <andrewjackson947@gmail.com> writes:
> > Currently the LDAP usage in fe-connect.c does not explicitly set the
> > protocol version to v3. This causes issues with many LDAP servers as they
> > will often require clients to use the v3 protocol and disallow any use of
> > the v2 protocol.
>
> This is the first complaint I can recall hearing about that, so
> exactly which ones are "many"?  Also, are we really sufficiently
> compliant with v3 that just adding this bit is enough?
>
> > One further note is that I do not currently see any test coverage over
> the
> > LDAP functionality in `fe-connect.c`. I am happy to add that to this
> patch
> > if needed.
>
> src/test/ldap/ doesn't do it for you?
>
>                         regards, tom lane
>

  5. Re: Update LDAP Protocol in fe-connect.c to v3

    Peter Eisentraut <peter@eisentraut.org> — 2025-03-26T06:41:48Z

    On 23.03.25 04:05, Andrew Jackson wrote:
>  > This is the first complaint I can recall hearing about that, so
> exactly which ones are "many"?
> 
> I've tested a 2 before figuring out about the v3 issue. lldap[0] and the 
> docker image osixia/docker-openldap[1].
> - lldap  gives the following error message when I attempt to connect 
> without the patch "Service Error: while handling incoming messages: 
> while receiving LDAP op: Bind request version is not equal to 3. This is 
> a serious client bug.". With the attached patch this error message does 
> not appear
> -  osixia/docker-openlap gives the following error message without the 
> patch "67df745e conn=1001 op=0 RESULT tag=97 err=2 text=historical 
> protocol version requested, use LDAPv3 instead".
> "
> 
>  > Also, are we really sufficiently compliant with v3 that just adding 
> this bit is enough?
> 
> I believe that this bit is all that is needed. Per the man page for 
> ldap_set_option [2]: "The protocol version used by the library defaults 
> to LDAPv2 (now historic), which corresponds to the LDAP_VERSION2 macro. 
> Application developers are encouraged to explicitly set 
> LDAP_OPT_PROTOCOL_VERSION to LDAPv3, using the LDAP_VERSION3 macro, or 
> to allow users to select the protocol version."
> 
>  > src/test/ldap/ doesn't do it for you?
> 
> Looking through the tests here it seems like they are all tests for the 
> serverside auth functionality that is configurable in pg_hba.conf. I 
> don't see any tests that test the client side "LDAP Lookup of Connection 
> Parameters" described in [3]

Ah yes.  There are two independent pieces of LDAP functionality.  One is 
the client authentication support in the backend, the other is the 
connection parameter lookup in libpq.  The former does set the LDAP 
protocol version, the latter does not.  This was clearly just forgotten. 
  Your patch makes sense.

  6. Re: Update LDAP Protocol in fe-connect.c to v3

    Andrew Jackson <andrewjackson947@gmail.com> — 2025-04-01T14:19:31Z

    Hi,

Added some tests for the LDAP connection parameters lookup functionality
with the attached patch. It is based off of the tests that were added
recently that cover the connection service file libpq functionality as well
as the existing ldap test framework.

Thanks,
Andrew Jackson

On Wed, Mar 26, 2025, 1:41 AM Peter Eisentraut <peter@eisentraut.org> wrote:

> On 23.03.25 04:05, Andrew Jackson wrote:
> >  > This is the first complaint I can recall hearing about that, so
> > exactly which ones are "many"?
> >
> > I've tested a 2 before figuring out about the v3 issue. lldap[0] and the
> > docker image osixia/docker-openldap[1].
> > - lldap  gives the following error message when I attempt to connect
> > without the patch "Service Error: while handling incoming messages:
> > while receiving LDAP op: Bind request version is not equal to 3. This is
> > a serious client bug.". With the attached patch this error message does
> > not appear
> > -  osixia/docker-openlap gives the following error message without the
> > patch "67df745e conn=1001 op=0 RESULT tag=97 err=2 text=historical
> > protocol version requested, use LDAPv3 instead".
> > "
> >
> >  > Also, are we really sufficiently compliant with v3 that just adding
> > this bit is enough?
> >
> > I believe that this bit is all that is needed. Per the man page for
> > ldap_set_option [2]: "The protocol version used by the library defaults
> > to LDAPv2 (now historic), which corresponds to the LDAP_VERSION2 macro.
> > Application developers are encouraged to explicitly set
> > LDAP_OPT_PROTOCOL_VERSION to LDAPv3, using the LDAP_VERSION3 macro, or
> > to allow users to select the protocol version."
> >
> >  > src/test/ldap/ doesn't do it for you?
> >
> > Looking through the tests here it seems like they are all tests for the
> > serverside auth functionality that is configurable in pg_hba.conf. I
> > don't see any tests that test the client side "LDAP Lookup of Connection
> > Parameters" described in [3]
>
> Ah yes.  There are two independent pieces of LDAP functionality.  One is
> the client authentication support in the backend, the other is the
> connection parameter lookup in libpq.  The former does set the LDAP
> protocol version, the latter does not.  This was clearly just forgotten.
>   Your patch makes sense.
>
>

  7. Re: Update LDAP Protocol in fe-connect.c to v3

    Peter Eisentraut <peter@eisentraut.org> — 2025-04-03T13:32:49Z

    On 22.03.25 22:22, Andrew Jackson wrote:
> Apologies, forgot to attach the patch in the prior email.
> 
> On Sat, Mar 22, 2025 at 4:10 PM Andrew Jackson 
> <andrewjackson947@gmail.com <mailto:andrewjackson947@gmail.com>> wrote:
> 
>     Currently the LDAP usage in fe-connect.c does not explicitly set the
>     protocol version to v3. This causes issues with many LDAP servers as
>     they will often require clients to use the v3 protocol and disallow
>     any use of the v2 protocol. Further the other usage of LDAP in
>     postgres (in `backend/libpq/auth.c`) uses the v3 protocol.
> 
>     This patch changes fe-connect.c so that it uses the v3 protocol
>     similar to `backend/libpq/auth.c`.
> 
>     One further note is that I do not currently see any test coverage
>     over the LDAP functionality in `fe-connect.c`. I am happy to add
>     that to this patch if needed.

Here is a slightly polished version of this patch.  I added an error 
message, and changed the return code, but it's a bit confusing which one 
might be the right one.

I also looked over the test file that you sent in a separate message. 
That also looks generally ok, but I'm not so deep into LDAP right now 
that I can give a detailed review.

My hunch right now is that we should probably take the patch that sets 
the version option and consider it for backpatching.  The patch with the 
tests can be held for detailed review later.

  8. Re: Update LDAP Protocol in fe-connect.c to v3

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-03T13:42:02Z

    Peter Eisentraut <peter@eisentraut.org> writes:
> Here is a slightly polished version of this patch.  I added an error 
> message, and changed the return code, but it's a bit confusing which one 
> might be the right one.

I'm kind of -0.5 on declaring the variable as "const", because none of
our existing calls of ldap_set_option do that.  I do see that the
Linux man page for ldap_set_option claims that that argument can be
const, but I think you're risking a portability gotcha for no large
gain.  LGTM otherwise.

> My hunch right now is that we should probably take the patch that sets 
> the version option and consider it for backpatching.  The patch with the 
> tests can be held for detailed review later.

+1 for that plan.

			regards, tom lane

  9. Re: Update LDAP Protocol in fe-connect.c to v3

    Andrew Jackson <andrewjackson947@gmail.com> — 2025-04-04T16:55:42Z

    Here is the same patch as v2 but with "const" removed in case you want to
move forward with that change. Tested locally against the tests I wrote in
the other patch to sanity check the change.

On Thu, Apr 3, 2025 at 8:42 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Peter Eisentraut <peter@eisentraut.org> writes:
> > Here is a slightly polished version of this patch.  I added an error
> > message, and changed the return code, but it's a bit confusing which one
> > might be the right one.
>
> I'm kind of -0.5 on declaring the variable as "const", because none of
> our existing calls of ldap_set_option do that.  I do see that the
> Linux man page for ldap_set_option claims that that argument can be
> const, but I think you're risking a portability gotcha for no large
> gain.  LGTM otherwise.
>
> > My hunch right now is that we should probably take the patch that sets
> > the version option and consider it for backpatching.  The patch with the
> > tests can be held for detailed review later.
>
> +1 for that plan.
>
>                         regards, tom lane
>

  10. Re: Update LDAP Protocol in fe-connect.c to v3

    pavel seleznev <pavel.seleznev@gmail.com> — 2025-05-15T12:54:32Z

    I applied your patch. I ran extended application tests relative to vanilla ones, which include various scenarios of working with LDAP and I think that we can safely apply the patch in the PG18.

I did not see the need for additional LDAP tests, since compatibility is provided by the LDAP library itself,  and not by the Postgres code. Also  protocol version 3 has been published for quite a long time (more than 20 years) and real use by clients is through version 3

  11. Re: Update LDAP Protocol in fe-connect.c to v3

    Peter Eisentraut <peter@eisentraut.org> — 2025-05-23T11:51:05Z

    On 15.05.25 14:54, Pavel Seleznev wrote:
> I applied your patch. I ran extended application tests relative to vanilla ones, which include various scenarios of working with LDAP and I think that we can safely apply the patch in the PG18.
> 
> I did not see the need for additional LDAP tests, since compatibility is provided by the LDAP library itself,  and not by the Postgres code. Also  protocol version 3 has been published for quite a long time (more than 20 years) and real use by clients is through version 3

FWIW, I'm prepared to commit this, but not for PG 18 at this point.

  12. Re: Update LDAP Protocol in fe-connect.c to v3

    Peter Eisentraut <peter@eisentraut.org> — 2025-08-12T15:36:52Z

    On 23.05.25 13:51, Peter Eisentraut wrote:
> On 15.05.25 14:54, Pavel Seleznev wrote:
>> I applied your patch. I ran extended application tests relative to 
>> vanilla ones, which include various scenarios of working with LDAP and 
>> I think that we can safely apply the patch in the PG18.
>>
>> I did not see the need for additional LDAP tests, since compatibility 
>> is provided by the LDAP library itself,  and not by the Postgres code. 
>> Also  protocol version 3 has been published for quite a long time 
>> (more than 20 years) and real use by clients is through version 3
> 
> FWIW, I'm prepared to commit this, but not for PG 18 at this point.

I have looked at this patch again.  There is a CI failure on Windows, 
and I was also able to reproduce this in my own CI runs, so it's not 
just a fluke.

https://cirrus-ci.com/task/6576976439279616?logs=build#L1271

The whole thing is puzzling, since we use the same code in the backend, 
without any Windows-specific guards.  No clue.

  13. Re: Update LDAP Protocol in fe-connect.c to v3

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-08-12T15:46:00Z

    On Tue, Aug 12, 2025 at 8:37 AM Peter Eisentraut <peter@eisentraut.org> wrote:
> The whole thing is puzzling, since we use the same code in the backend,
> without any Windows-specific guards.  No clue.

I think it's a CI-wide failure, or at least the cfbot is showing a red
column at the moment.

--Jacob

  14. Re: Update LDAP Protocol in fe-connect.c to v3

    Andres Freund <andres@anarazel.de> — 2025-08-12T15:55:09Z

    Hi,

On 2025-08-12 08:46:00 -0700, Jacob Champion wrote:
> On Tue, Aug 12, 2025 at 8:37 AM Peter Eisentraut <peter@eisentraut.org> wrote:
> > The whole thing is puzzling, since we use the same code in the backend,
> > without any Windows-specific guards.  No clue.
> 
> I think it's a CI-wide failure, or at least the cfbot is showing a red
> column at the moment.

See https://postgr.es/m/CAN55FZ1RuBhJmPWs3Oi%3D9UoezDfrtO-VaU67db5%2B0_uy19uF%2BA%40mail.gmail.com

Greetings,

Andres Freund

  15. Re: Update LDAP Protocol in fe-connect.c to v3

    Peter Eisentraut <peter@eisentraut.org> — 2025-08-12T19:22:35Z

    On 12.08.25 17:46, Jacob Champion wrote:
> On Tue, Aug 12, 2025 at 8:37 AM Peter Eisentraut <peter@eisentraut.org> wrote:
>> The whole thing is puzzling, since we use the same code in the backend,
>> without any Windows-specific guards.  No clue.
> 
> I think it's a CI-wide failure, or at least the cfbot is showing a red
> column at the moment.

Ok, then I'm going to be wild and commit it anyway.  Done.

  16. Re: Update LDAP Protocol in fe-connect.c to v3

    Peter Eisentraut <peter@eisentraut.org> — 2025-08-15T09:02:35Z

    On 01.04.25 16:19, Andrew Jackson wrote:
> Added some tests for the LDAP connection parameters lookup functionality 
> with the attached patch. It is based off of the tests that were added 
> recently that cover the connection service file libpq functionality as 
> well as the existing ldap test framework.

I have committed this additional test file.  Thanks.