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 <pe...@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. > >
From 53210d9ba583a2159e4847b4adda20b9a4f652f7 Mon Sep 17 00:00:00 2001 From: CommanderKeynes <andrewjackson...@gmail.coma> 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 <andrewjackson...@gmail.com> 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 00000000000..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('authdata.ldif'); +$ldap->ldapsetpw('uid=test1,dc=example,dc=net', 'secret1'); +$ldap->ldapsetpw('uid=test2,dc=example,dc=net', 'secret2'); +# +# Windows vs non-Windows: CRLF vs LF for the file's newline, relying on +# the fact that libpq uses fgets() when reading the lines of a service file. +my $newline = $windows_os ? "\r\n" : "\n"; + +my $td = PostgreSQL::Test::Utils::tempdir; + +# TODO create ldap file based on postgres connection info +my $ldif_valid = "$td/connection_params.ldif"; +append_to_file($ldif_valid, "version:1"); +append_to_file($ldif_valid, $newline); +append_to_file($ldif_valid, "dn:cn=mydatabase,dc=example,dc=net"); +append_to_file($ldif_valid, $newline); +append_to_file($ldif_valid, "changetype:add"); +append_to_file($ldif_valid, $newline); +append_to_file($ldif_valid, "objectclass:top"); +append_to_file($ldif_valid, $newline); +append_to_file($ldif_valid, "objectclass:device"); +append_to_file($ldif_valid, $newline); +append_to_file($ldif_valid, "cn:mydatabase"); +append_to_file($ldif_valid, $newline); +append_to_file($ldif_valid, "description:host="); +append_to_file($ldif_valid, $node->host); +append_to_file($ldif_valid, $newline); +append_to_file($ldif_valid, "description:port="); +append_to_file($ldif_valid, $node->port); + +$ldap->ldapadd_file($ldif_valid); + +my ($ldap_server, $ldap_port, $ldaps_port, $ldap_url, + $ldaps_url, $ldap_basedn, $ldap_rootdn +) = $ldap->prop(qw(server port s_port url s_url basedn rootdn)); + +# don't bother to check the server's cert (though perhaps we should) +$ENV{'LDAPTLS_REQCERT'} = "never"; + +note "setting up PostgreSQL instance"; + +# Create the set of service files used in the tests. +# File that includes a valid service name, that uses a decomposed connection +# string for its contents, split on spaces. +my $srvfile_valid = "$td/pg_service_valid.conf"; +append_to_file($srvfile_valid, "[my_srv]"); +append_to_file($srvfile_valid, $newline); +append_to_file($srvfile_valid, "ldap://localhost:";); +append_to_file($srvfile_valid, $ldap_port); +append_to_file($srvfile_valid, "/dc=example,dc=net?description?one?(cn=mydatabase)"); + +# File defined with no contents, used as default value for PGSERVICEFILE, +# so as no lookup is attempted in the user's home directory. +my $srvfile_empty = "$td/pg_service_empty.conf"; +append_to_file($srvfile_empty, ''); + +# Default service file in PGSYSCONFDIR. +my $srvfile_default = "$td/pg_service.conf"; + +# Missing service file. +my $srvfile_missing = "$td/pg_service_missing.conf"; + +# Set the fallback directory lookup of the service file to the temporary +# directory of this test. PGSYSCONFDIR is used if the service file +# defined in PGSERVICEFILE cannot be found, or when a service file is +# found but not the service name. +local $ENV{PGSYSCONFDIR} = $td; +# Force PGSERVICEFILE to a default location, so as this test never +# tries to look at a home directory. This value needs to remain +# at the top of this script before running any tests, and should never +# be changed. +local $ENV{PGSERVICEFILE} = "$srvfile_empty"; + +# Checks combinations of service name and a valid service file. +{ + local $ENV{PGSERVICEFILE} = $srvfile_valid; + $dummy_node->connect_ok( + 'service=my_srv', + 'connection with correct "service" string and PGSERVICEFILE', + sql => "SELECT 'connect1_1'", + expected_stdout => qr/connect1_1/); + + $dummy_node->connect_ok( + 'postgres://?service=my_srv', + 'connection with correct "service" URI and PGSERVICEFILE', + sql => "SELECT 'connect1_2'", + expected_stdout => qr/connect1_2/); + + $dummy_node->connect_fails( + 'service=undefined-service', + 'connection with incorrect "service" string and PGSERVICEFILE', + expected_stderr => + qr/definition of service "undefined-service" not found/); + + local $ENV{PGSERVICE} = 'my_srv'; + $dummy_node->connect_ok( + '', + 'connection with correct PGSERVICE and PGSERVICEFILE', + sql => "SELECT 'connect1_3'", + expected_stdout => qr/connect1_3/); + + local $ENV{PGSERVICE} = 'undefined-service'; + $dummy_node->connect_fails( + '', + 'connection with incorrect PGSERVICE and PGSERVICEFILE', + expected_stdout => + qr/definition of service "undefined-service" not found/); +} + +# Checks case of incorrect service file. +{ + local $ENV{PGSERVICEFILE} = $srvfile_missing; + $dummy_node->connect_fails( + 'service=my_srv', + 'connection with correct "service" string and incorrect PGSERVICEFILE', + expected_stderr => + qr/service file ".*pg_service_missing.conf" not found/); +} + +# Checks case of service file named "pg_service.conf" in PGSYSCONFDIR. +{ + # Create copy of valid file + my $srvfile_default = "$td/pg_service.conf"; + copy($srvfile_valid, $srvfile_default); + + $dummy_node->connect_ok( + 'service=my_srv', + 'connection with correct "service" string and pg_service.conf', + sql => "SELECT 'connect2_1'", + expected_stdout => qr/connect2_1/); + + $dummy_node->connect_ok( + 'postgres://?service=my_srv', + 'connection with correct "service" URI and default pg_service.conf', + sql => "SELECT 'connect2_2'", + expected_stdout => qr/connect2_2/); + + $dummy_node->connect_fails( + 'service=undefined-service', + 'connection with incorrect "service" string and default pg_service.conf', + expected_stderr => + qr/definition of service "undefined-service" not found/); + + local $ENV{PGSERVICE} = 'my_srv'; + $dummy_node->connect_ok( + '', + 'connection with correct PGSERVICE and default pg_service.conf', + sql => "SELECT 'connect2_3'", + expected_stdout => qr/connect2_3/); + + local $ENV{PGSERVICE} = 'undefined-service'; + $dummy_node->connect_fails( + '', + 'connection with incorrect PGSERVICE and default pg_service.conf', + expected_stdout => + qr/definition of service "undefined-service" not found/); + + # Remove default pg_service.conf. + unlink($srvfile_default); +} + +$node->teardown_node; + +done_testing(); -- 2.43.5
