Add Option To Check All Addresses For Matching target_session_attr
Hi,
I was attempting to set up a high availability system using DNS and
target_session_attrs. I was using a DNS setup similar to below and was
trying to use the connection strings `psql postgresql://
u...@pg.database.com/db_name?target_session=read-write` to have clients
dynamically connect to the primary or `psql postgresql://
u...@pg.database.com/db_name?target_session=read-only` to have clients
connect to a read replica.
The problem that I found with this setup is that if libpq is unable to get
a matching target_session_attr on the first connection attempt it does not
consider any further addresses for the given host. This patch is designed
to provide an option that allows libpq to look at additional addresses for
a given host if the target_session_attr check fails for previous addresses.
Would appreciate any feedback on the applicability/relevancy of the goal
here or the implementation.
Example DNS setup
Name | Type| Record
__|__|___
pg.database.com | A| ip_address_1
pg.database.com | A| ip_address_2
pg.database.com | A| ip_address_3
pg.database.com | A| ip_address_4
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 51083dcfd8..865eb74fd7 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -365,6 +365,11 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
"Load-Balance-Hosts", "", 8, /* sizeof("disable") = 8 */
offsetof(struct pg_conn, load_balance_hosts)},
+ {"check_all_addrs", "PGCHECKALLADDRS",
+ DefaultLoadBalanceHosts, NULL,
+ "Check-All-Addrs", "", 1,
+ offsetof(struct pg_conn, check_all_addrs)},
+
/* Terminating entry --- MUST BE LAST */
{NULL, NULL, NULL, NULL,
NULL, NULL, 0}
@@ -4030,11 +4035,11 @@ keep_going: /* We will come back to here until there is
conn->status = CONNECTION_OK;
sendTerminateConn(conn);
- /*
- * Try next host if any, but we don't want to consider
- * additional addresses for this host.
- */
- conn->try_next_host = true;
+ if (conn->check_all_addrs && conn->check_all_addrs[0] == '1')
+ conn->try_next_addr = true;
+ else
+ conn->try_next_host = true;
+
goto keep_going;
}
}
@@ -4085,11 +4090,11 @@ keep_going: /* We will come back to here until there is
conn->status = CONNECTION_OK;
sendTerminateConn(conn);
- /*
- * Try next host if any, but we don't want to consider
- * additional addresses for this host.
- */
- conn->try_next_host = true;
+ if (conn->check_all_addrs && conn->check_all_addrs[0] == '1')
+ conn->try_next_addr = true;
+ else
+ conn->try_next_host = true;
+
goto keep_going;
}
}
@@ -4703,6 +4708,7 @@ freePGconn(PGconn *conn)
free(conn->rowBuf);
free(conn->target_session_attrs);
free(conn->load_balance_hosts);
+ free(conn->check_all_addrs);
termPQExpBuffer(&conn->errorMessage);
termPQExpBuffer(&conn->workBuffer);
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 08cc391cbd..c51ec28234 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -427,6 +427,7 @@ struct pg_conn
char *target_session_attrs; /* desired session properties */
char *require_auth; /* name of the expected auth method */
char *load_balance_hosts; /* load balance over hosts */
+ char *check_all_addrs; /* whether to check all ips within a host or terminate on failure */
bool cancelRequest; /* true if this connection is used to send a
* cancel request, instead of being a normal
Re: Add Option To Check All Addresses For Matching target_session_attr
The previous patch had a mistake in a reference in the documentation. This should fix it. On Mon, Feb 24, 2025 at 10:07 AM Andrew Jackson wrote: > Looks like this needed another rebase to account for the oauth commit. > Rebase attached. > > On Mon, Feb 24, 2025 at 9:38 AM Andrew Jackson > wrote: > >> Hi, >> >> Thank you for the review! >> >> Review Response >> >> - Made a first pass at a real commit message >> - Fixed the condition on the if statement to use strcmp >> - Added a test suite in the files `src/interfaces/libpq/t/ >> 006_target_session_attr_dns.pl` and `src/interfaces/libpq/t/ >> 007_load_balance_dns_check_all_addrs.pl` which checks the >> target_session_attrs as when used with and without load balancing. >> >> Regarding the name of the variable itself I am definitely open to >> opinions on this. I didn't put too much thought initially and just chose >> `check_all_addrs`. I feel like given that it modifies the behaviour of >> `target_session_attrs` ideally it should reference that in the name but >> that would make that variable name very long: something akin to >> `target_session_attrs_check_all_addrs`. >> >> Context >> >> I tested some drivers as well and found that pgx, psycopg, and >> rust-postgres all traverse every IP address when looking for a matching >> target_session_attrs. Asyncpg and psycopg2 on the other hand follow libpq >> and terminate additional attempts after the first failure. Given this it >> seems like there is a decent amount of fragmentation in the ecosystem as to >> how exactly to implement this feature. I believe some drivers choose to >> traverse all addresses because they have users target the same use case >> outlined above. >> >> Thanks again, >> Andrew Jackson >> >> On Sun, Feb 16, 2025 at 6:03 AM Andrey Borodin >> wrote: >> >>> Hi Andrew! >>> >>> cc Jelte, I suspect he might be interested. >>> >>> > On 20 Nov 2024, at 20:51, Andrew Jackson >>> wrote: >>> > >>> > Would appreciate any feedback on the applicability/relevancy of the >>> goal here or the implementation. >>> >>> Thank you for raising the issue. Following our discussion in Discord I'm >>> putting my thoughts to list. >>> >>> >>> Context >>> >>> A DNS record might return several IPs. Consider we have a connection >>> string with "host=A,B", A is resolved to 1.1.1.1,2.2.2.2, B to >>> 3.3.3.3,4.4.4.4. >>> If we connect with "target_session_attrs=read-write" IPs 1.1.1.1 and >>> 3.3.3.3 will be probed, but 2.2.2.2 and 4.4.4.4 won't (if 1.1.1.1 and >>> 3.3.3.3 responded). >>> >>> If we enable libpq load balancing some random 2 IPs will be probed. >>> >>> IMO it's a bug, at least when load balancing is enabled. Let's consider >>> if we can change default behavior here. I suspect we can't do it for >>> "load_balance_hosts=disable". And even for load balancing this might be too >>> unexpected change for someone. >>> >>> Further I only consider proposal not as a bug fix, but as a feature. >>> >>> In Discord we have surveyed some other drivers. >>> pgx treats all IPs as different servers [1]. npgsql goes through all IPs >>> one-by-one always [2]. PGJDBC are somewhat in a decision process [3] (cc >>> Dave and Vladimir, if they would like to provide some input). >>> >>> >>> Review >>> >>> The patch needs a rebase. It's trivial, so please fine attached. The >>> patch needs real commit message, it's not trivial :) >>> >>> We definitely need to adjust tests [0]. We need to change >>> 004_load_balance_dns.pl so that it tests target_session_attrs too. >>> >>> Some documentation would be nice. >>> >>> I do not like how this check is performed >>> + if >>> (conn->check_all_addrs && conn->check_all_addrs[0] == '1') >>> Let's make it like load balancing is done [4]. >>> >>> Finally, let's think about naming alternatives for "check_all_addrs". >>> >>> I think that's enough for a first round of the review. If it's not a >>> bug, but a feature - it's a very narrow window to get to 18. But we might >>> be lucky... >>> >>> Thank you! >>> >>> >
Re: Add Option To Check All Addresses For Matching target_session_attr
Hi, Thank you for the review! Review Response - Made a first pass at a real commit message - Fixed the condition on the if statement to use strcmp - Added a test suite in the files `src/interfaces/libpq/t/ 006_target_session_attr_dns.pl` and `src/interfaces/libpq/t/ 007_load_balance_dns_check_all_addrs.pl` which checks the target_session_attrs as when used with and without load balancing. Regarding the name of the variable itself I am definitely open to opinions on this. I didn't put too much thought initially and just chose `check_all_addrs`. I feel like given that it modifies the behaviour of `target_session_attrs` ideally it should reference that in the name but that would make that variable name very long: something akin to `target_session_attrs_check_all_addrs`. Context I tested some drivers as well and found that pgx, psycopg, and rust-postgres all traverse every IP address when looking for a matching target_session_attrs. Asyncpg and psycopg2 on the other hand follow libpq and terminate additional attempts after the first failure. Given this it seems like there is a decent amount of fragmentation in the ecosystem as to how exactly to implement this feature. I believe some drivers choose to traverse all addresses because they have users target the same use case outlined above. Thanks again, Andrew Jackson On Sun, Feb 16, 2025 at 6:03 AM Andrey Borodin wrote: > Hi Andrew! > > cc Jelte, I suspect he might be interested. > > > On 20 Nov 2024, at 20:51, Andrew Jackson > wrote: > > > > Would appreciate any feedback on the applicability/relevancy of the goal > here or the implementation. > > Thank you for raising the issue. Following our discussion in Discord I'm > putting my thoughts to list. > > > Context > > A DNS record might return several IPs. Consider we have a connection > string with "host=A,B", A is resolved to 1.1.1.1,2.2.2.2, B to > 3.3.3.3,4.4.4.4. > If we connect with "target_session_attrs=read-write" IPs 1.1.1.1 and > 3.3.3.3 will be probed, but 2.2.2.2 and 4.4.4.4 won't (if 1.1.1.1 and > 3.3.3.3 responded). > > If we enable libpq load balancing some random 2 IPs will be probed. > > IMO it's a bug, at least when load balancing is enabled. Let's consider if > we can change default behavior here. I suspect we can't do it for > "load_balance_hosts=disable". And even for load balancing this might be too > unexpected change for someone. > > Further I only consider proposal not as a bug fix, but as a feature. > > In Discord we have surveyed some other drivers. > pgx treats all IPs as different servers [1]. npgsql goes through all IPs > one-by-one always [2]. PGJDBC are somewhat in a decision process [3] (cc > Dave and Vladimir, if they would like to provide some input). > > > Review > > The patch needs a rebase. It's trivial, so please fine attached. The patch > needs real commit message, it's not trivial :) > > We definitely need to adjust tests [0]. We need to change > 004_load_balance_dns.pl so that it tests target_session_attrs too. > > Some documentation would be nice. > > I do not like how this check is performed > + if (conn->check_all_addrs > && conn->check_all_addrs[0] == '1') > Let's make it like load balancing is done [4]. > > Finally, let's think about naming alternatives for "check_all_addrs". > > I think that's enough for a first round of the review. If it's not a bug, > but a feature - it's a very narrow window to get to 18. But we might be > lucky... > > Thank you! > > > Best regards, Andrey Borodin. > > [0] > https://github.com/postgres/postgres/commit/7f5b19817eaf38e70ad1153db4e644ee9456853e#diff-b05b74d2a97d7f74d4311ba1702d732f0df1b101c6ac99c146b51215174cf3ffR94 > [1] https://github.com/jackc/pgx/blob/master/pgconn/pgconn.go#L177 > [2] > https://github.com/npgsql/npgsql/blob/7f1a59fa8dc1ccc34a70154f49a768e1abf826ba/src/Npgsql/Internal/NpgsqlConnector.cs#L986 > [3] https://github.com/pgjdbc/pgjdbc/pull/3012#discussion_r1408069450 > [4] > https://github.com/postgres/postgres/commit/7f5b19817eaf38e70ad1153db4e644ee9456853e#diff-8d819454e061b9d4cdae9c8922ded05753a629d70f2ac1de1d4f6d5a4aeb7f68R1660 > From 358605cff594ba08b14edbcf497cab59834f8110 Mon Sep 17 00:00:00 2001 From: Andrey Borodin Date: Fri, 14 Feb 2025 23:13:55 +0500 Subject: [PATCH v3] Add option to check all addrs for target_session. The current behaviour of libpq with regard to searching for a matching target_session_attrs in a list of addrs is that after successfully connecting to a server if the servers session_attr does not match the request target_session_attrs no futher address is consider
Re: Add Option To Check All Addresses For Matching target_session_attr
Looks like this needed another rebase to account for the oauth commit. Rebase attached. On Mon, Feb 24, 2025 at 9:38 AM Andrew Jackson wrote: > Hi, > > Thank you for the review! > > Review Response > > - Made a first pass at a real commit message > - Fixed the condition on the if statement to use strcmp > - Added a test suite in the files `src/interfaces/libpq/t/ > 006_target_session_attr_dns.pl` and `src/interfaces/libpq/t/ > 007_load_balance_dns_check_all_addrs.pl` which checks the > target_session_attrs as when used with and without load balancing. > > Regarding the name of the variable itself I am definitely open to opinions > on this. I didn't put too much thought initially and just chose > `check_all_addrs`. I feel like given that it modifies the behaviour of > `target_session_attrs` ideally it should reference that in the name but > that would make that variable name very long: something akin to > `target_session_attrs_check_all_addrs`. > > Context > > I tested some drivers as well and found that pgx, psycopg, and > rust-postgres all traverse every IP address when looking for a matching > target_session_attrs. Asyncpg and psycopg2 on the other hand follow libpq > and terminate additional attempts after the first failure. Given this it > seems like there is a decent amount of fragmentation in the ecosystem as to > how exactly to implement this feature. I believe some drivers choose to > traverse all addresses because they have users target the same use case > outlined above. > > Thanks again, > Andrew Jackson > > On Sun, Feb 16, 2025 at 6:03 AM Andrey Borodin > wrote: > >> Hi Andrew! >> >> cc Jelte, I suspect he might be interested. >> >> > On 20 Nov 2024, at 20:51, Andrew Jackson >> wrote: >> > >> > Would appreciate any feedback on the applicability/relevancy of the >> goal here or the implementation. >> >> Thank you for raising the issue. Following our discussion in Discord I'm >> putting my thoughts to list. >> >> >> Context >> >> A DNS record might return several IPs. Consider we have a connection >> string with "host=A,B", A is resolved to 1.1.1.1,2.2.2.2, B to >> 3.3.3.3,4.4.4.4. >> If we connect with "target_session_attrs=read-write" IPs 1.1.1.1 and >> 3.3.3.3 will be probed, but 2.2.2.2 and 4.4.4.4 won't (if 1.1.1.1 and >> 3.3.3.3 responded). >> >> If we enable libpq load balancing some random 2 IPs will be probed. >> >> IMO it's a bug, at least when load balancing is enabled. Let's consider >> if we can change default behavior here. I suspect we can't do it for >> "load_balance_hosts=disable". And even for load balancing this might be too >> unexpected change for someone. >> >> Further I only consider proposal not as a bug fix, but as a feature. >> >> In Discord we have surveyed some other drivers. >> pgx treats all IPs as different servers [1]. npgsql goes through all IPs >> one-by-one always [2]. PGJDBC are somewhat in a decision process [3] (cc >> Dave and Vladimir, if they would like to provide some input). >> >> >> Review >> >> The patch needs a rebase. It's trivial, so please fine attached. The >> patch needs real commit message, it's not trivial :) >> >> We definitely need to adjust tests [0]. We need to change >> 004_load_balance_dns.pl so that it tests target_session_attrs too. >> >> Some documentation would be nice. >> >> I do not like how this check is performed >> + if (conn->check_all_addrs >> && conn->check_all_addrs[0] == '1') >> Let's make it like load balancing is done [4]. >> >> Finally, let's think about naming alternatives for "check_all_addrs". >> >> I think that's enough for a first round of the review. If it's not a bug, >> but a feature - it's a very narrow window to get to 18. But we might be >> lucky... >> >> Thank you! >> >> >> Best regards, Andrey Borodin. >> >> [0] >> https://github.com/postgres/postgres/commit/7f5b19817eaf38e70ad1153db4e644ee9456853e#diff-b05b74d2a97d7f74d4311ba1702d732f0df1b101c6ac99c146b51215174cf3ffR94 >> [1] https://github.com/jackc/pgx/blob/master/pgconn/pgconn.go#L177 >> [2] >> https://github.com/npgsql/npgsql/blob/7f1a59fa8dc1ccc34a70154f49a768e1abf826ba/src/Npgsql/Internal/NpgsqlConnector.cs#L986 >> [3] https://github.com/pgjdbc/pgjdbc/pull/3012#discussion_r1408069450 >> [4] >> https://github.com/postgres/postgres/comm
Update LDAP Protocol in fe-connect.c to v3
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.
Re: Update LDAP Protocol in fe-connect.c to v3
Apologies, forgot to attach the patch in the prior email.
On Sat, Mar 22, 2025 at 4:10 PM Andrew Jackson
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.
>
From f93256a5f80bde1d5de79941bd7313a014085f2b Mon Sep 17 00:00:00 2001
From: CommanderKeynes
Date: Sat, 22 Mar 2025 15:34:42 -0500
Subject: [PATCH v1] Set version 3 protocol
---
src/interfaces/libpq/fe-connect.c | 8
1 file changed, 8 insertions(+)
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index d5051f5e820..40d7d176cf0 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -5413,6 +5413,7 @@ ldapServiceLookup(const char *purl, PQconninfoOption *options,
*entry;
struct berval **values;
LDAP_TIMEVAL time = {PGLDAP_TIMEOUT, 0};
+ int ldapversion = LDAP_VERSION3;
if ((url = strdup(purl)) == NULL)
{
@@ -5544,6 +5545,13 @@ ldapServiceLookup(const char *purl, PQconninfoOption *options,
return 3;
}
+if ((ldap_set_option(ld, LDAP_OPT_PROTOCOL_VERSION, &ldapversion)) != LDAP_SUCCESS)
+{
+ free(url);
+ldap_unbind(ld);
+return 2;
+}
+
/*
* Perform an explicit anonymous bind.
*
--
2.47.2
Re: [PATCH] PGSERVICEFILE as part of a normal connection string
Hi,
I am working on a feature adjacent to the connection service functionality
and noticed some issues with the tests introduced in this thread. Basically
they incorrectly invoke the append perl function by passing multiple
strings to append when the function only takes one string to append. This
caused the generated service files to not actually contain any connection
parameters. The tests were only passing because the connect_ok perl
function set the connection parameters as environment variables which
covered up the misformed connection service file.
The attached patch is much more strict in that it creates a dummy database
that is not started and passes all queries though that and tests that the
connection service file correctly overrides the environment variables set
by the dummy databases' query functions
Thanks,
Andrew Jackson
On Mon, Mar 31, 2025, 4:01 PM Ryo Kanbayashi
wrote:
> On Sat, Mar 22, 2025 at 4:46 PM Michael Paquier
> wrote:
> >
> > On Thu, Mar 20, 2025 at 06:16:44PM +0900, Ryo Kanbayashi wrote:
> > > Sorry, I found a miss on 006_service.pl.
> > > Fixed patch is attached...
> >
> > Please note that the commit fest app needs all the patches of a a set
> > to be posted in the same message. In this case, v2-0001 is not going
> > to get automatic test coverage.
> >
> > Your patch naming policy is also a bit confusing. I would suggest to
> > use `git format-patch -vN -2`, where N is your version number. 0001
> > would be the new tests for service files, and 0002 the new feature,
> > with its own tests.
>
> All right.
> I attached patches generated with your suggested command :)
>
> > +if ($windows_os) {
> > +
> > +# Windows: use CRLF
> > +print $fh "[my_srv]", "\r\n";
> > +print $fh join( "\r\n", split( ' ', $node->connstr ) ), "\r\n";
> > +}
> > +else {
> > +# Non-Windows: use LF
> > +print $fh "[my_srv]", "\n";
> > +print $fh join( "\n", split( ' ', $node->connstr ) ), "\n";
> > +}
> > +close $fh;
> >
> > That's duplicated. Let's perhaps use a $newline variable and print
> > into the file using the $newline?
>
> OK.
> I reflected above comment.
>
> > Question: you are doing things this way in the test because fgets() is
> > what is used by libpq to retrieve the lines of the service file, is
> > that right?
>
> No. I'm doing above way simply because line ending code of service file
> wrote by users may become CRLF in Windows platform.
>
> > Please note that the CI is failing. It seems to me that you are
> > missing a done_testing() at the end of the script. If you have a
> > github account, I'd suggest to set up a CI in your own fork of
> > Postgres, this is really helpful to double-check the correctness of a
> > patch before posting it to the lists, and saves in round trips between
> > author and reviewer. Please see src/tools/ci/README in the code tree
> > for details.
>
> Sorry.
> I'm using Cirrus CI with GitHub and I checked passing the CI.
> But there were misses when I created patch files...
>
> > +# Copyright (c) 2023-2024, PostgreSQL Global Development Group
> >
> > These dates are incorrect. Should be 2025, as it's a new file.
>
> OK.
>
> > +++ b/src/interfaces/libpq/t/007_servicefile_opt.pl
> > @@ -0,0 +1,100 @@
> > +# Copyright (c) 2023-2024, PostgreSQL Global Development Group
> >
> > Incorrect date again in the second path with the new feature. I'd
> > suggest to merge all the tests in a single script, with only one node
> > initialized and started.
>
> OK.
> Additional test scripts have been merged to a single script ^^ b
>
> ---
> Great regards,
> Ryo Kanbayashi
>
From 0d732ee8edbb16132b95f35775388528fa9003d8 Mon Sep 17 00:00:00 2001
From: AndrewJackson
Date: Mon, 31 Mar 2025 15:18:48 -0500
Subject: [PATCH] libpq: Fix TAP tests for service files and names
This commit builds on the tests that were added in a prior commit
that tests the connection service file functionality. The tests are
fixed to correctly invoke the append_to_file subroutine which only
takes one argument and not multiple. The test also runs the statements
through a dummy database to ensure that the options from the service
file are actually being picked up and just passing because they are
defaulting to the connection environment variables that are set in
the connect_ok function.
Author: Andrew Jackson
---
src/interfaces/libpq/t/006_service.pl | 38 ++-
1 f
Re: Update LDAP Protocol in fe-connect.c to v3
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 wrote:
> Peter Eisentraut 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
>
From 4684d5cd1bc5c5150f6435bf2be1be9f957a4429 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Thu, 3 Apr 2025 15:06:13 +0200
Subject: [PATCH] libpq: Set LDAP protocol version 3
Some LDAP servers reject the default version 2 protocol. So set
version 3 before starting the connection. This matches how the
backend LDAP code has worked all along.
Co-authored-by: Andrew Jackson
Discussion: https://www.postgresql.org/message-id/flat/CAKK5BkHixcivSCA9pfd_eUp7wkLRhvQ6OtGLAYrWC%3Dk7E76LDQ%40mail.gmail.com
---
src/interfaces/libpq/fe-connect.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index ddcc7b60ab0..d9d064174de 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -5069,6 +5069,7 @@ ldapServiceLookup(const char *purl, PQconninfoOption *options,
*entry;
struct berval **values;
LDAP_TIMEVAL time = {PGLDAP_TIMEOUT, 0};
+ int ldapversion = LDAP_VERSION3;
if ((url = strdup(purl)) == NULL)
{
@@ -5200,6 +5201,15 @@ ldapServiceLookup(const char *purl, PQconninfoOption *options,
return 3;
}
+ if ((rc = ldap_set_option(ld, LDAP_OPT_PROTOCOL_VERSION, &ldapversion)) != LDAP_SUCCESS)
+ {
+ libpq_append_error(errorMessage, "could not set LDAP protocol version: %s",
+ ldap_err2string(rc));
+ free(url);
+ ldap_unbind(ld);
+ return 3;
+ }
+
/*
* Perform an explicit anonymous bind.
*
--
2.47.2
Re: Update LDAP Protocol in fe-connect.c to v3
> 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 wrote: > Andrew Jackson 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 >
Re: Update LDAP Protocol in fe-connect.c to v3
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 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.
>
>
From 53210d9ba583a2159e4847b4adda20b9a4f652f7 Mon Sep 17 00:00:00 2001
From: CommanderKeynes
Date: Mon, 31 Mar 2025 22:47:19 -0500
Subject: [PATCH] Add TAP tests for LDAP connection parameter lookup
This commit adds regressions tests that tests the LDAP Lookup
of Connection Parameters functionality. Prior to this commit
LDAP test coverage only existed for the serverside auth
functionality and for connection service file with parameters
directly specified in the file. This tests included with this PR
tests a pg_service.conf that contains a link to an LDAP system which
contains all of the connection parameters.
Author: Andrew Jackson
Discussion: https://www.postgresql.org/message-id/CAKK5BkHixcivSCA9pfd_eUp7wkLRhvQ6OtGLAYrWC%3Dk7E76LDQ%40mail.gmail.com
---
.../t/003_ldap_connection_param_lookup.pl | 202 ++
1 file changed, 202 insertions(+)
create mode 100644 src/test/ldap/t/003_ldap_connection_param_lookup.pl
diff --git a/src/test/ldap/t/003_ldap_connection_param_lookup.pl b/src/test/ldap/t/003_ldap_connection_param_lookup.pl
new file mode 100644
index 000..a531cb783c8
--- /dev/null
+++ b/src/test/ldap/t/003_ldap_connection_param_lookup.pl
@@ -0,0 +1,202 @@
+# Copyright (c) 2025, PostgreSQL Global Development Group
+use strict;
+use warnings FATAL => 'all';
+use File::Copy;
+use PostgreSQL::Test::Utils;
+use PostgreSQL::Test::Cluster;
+use Test::More;
+use FindBin;
+use lib "$FindBin::RealBin/..";
+use LdapServer;
+
+if ($ENV{with_ldap} ne 'yes')
+{
+ plan skip_all => 'LDAP not supported by this build';
+}
+elsif (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bldap\b/)
+{
+ plan skip_all =>
+ 'Potentially unsafe test LDAP not enabled in PG_TEST_EXTRA';
+}
+elsif (!$LdapServer::setup)
+{
+ plan skip_all => $LdapServer::setup_error;
+}
+#
+# This tests scenarios related to the service name and the service file,
+# for the connection options and their environment variables.
+my $dummy_node = PostgreSQL::Test::Cluster->new('dummy_node');
+$dummy_node->init;
+
+my $node = PostgreSQL::Test::Cluster->new('node');
+$node->init;
+$node->start;
+
+note "setting up LDAP server";
+
+my $ldap_rootpw = 'secret';
+my $ldap = LdapServer->new($ldap_rootpw, 'anonymous');# use anonymous auth
+$ldap->ldapadd_file(&#x
