Re: [PATCH] Add peer authentication TAP test
On 25.11.2022 10:34, Michael Paquier wrote: On Fri, Nov 25, 2022 at 10:13:29AM +0300, Anton A. Melnikov wrote: The test fails almost at the beginning in reset_pg_hba call after modification pg_hba.conf and node reloading: #t/003_peer.pl .. Dubious, test returned 2 (wstat 512, 0x200) #No subtests run Logs regress_log_003_peer and 003_peer_node.log are attached. Yeah, that's failing exactly at the position I am pointing to. I'll go apply what you have.. -- Michael Thanks! With the best wishes, -- Anton A. Melnikov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: [PATCH] Add peer authentication TAP test
On Fri, Nov 25, 2022 at 10:13:29AM +0300, Anton A. Melnikov wrote: > The test fails almost at the beginning in reset_pg_hba call after > modification pg_hba.conf and node reloading: > #t/003_peer.pl .. Dubious, test returned 2 (wstat 512, 0x200) > #No subtests run > > Logs regress_log_003_peer and 003_peer_node.log are attached. Yeah, that's failing exactly at the position I am pointing to. I'll go apply what you have.. -- Michael signature.asc Description: PGP signature
Re: [PATCH] Add peer authentication TAP test
Hello, thanks for rapid answer! On 25.11.2022 08:18, Michael Paquier wrote: You are not using MSVC but MinGW, are you? The buildfarm members with TAP tests enabled are drongo, fairywren, bowerbord and jacana. Even though none of them are running the tests from src/test/authentication/, this is running on a periodic basis in the CI, where we are able to skip the test in MSVC already: postgresql:authentication / authentication/003_peer SKIP 9.73s There is MSVC on my PC. The project was build with Microsoft (R) C/C++ Optimizing Compiler Version 19.29.30136 for x64 So yes, it is plausible that we are missing more safeguards here. Your suggestion to skip under !$use_unix_sockets makes sense, as not having unix sockets is not going to work for peer and WIN32 needs SSPI to be secure with pg_regress. Where is your test failing? On the first $node->psql('postgres') at the beginning of the test? Just wondering.. The test fails almost at the beginning in reset_pg_hba call after modification pg_hba.conf and node reloading: #t/003_peer.pl .. Dubious, test returned 2 (wstat 512, 0x200) #No subtests run Logs regress_log_003_peer and 003_peer_node.log are attached. With best regards, -- Anton A. Melnikov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company2022-11-25 09:55:49.731 MSK [7648] LOG: starting PostgreSQL 16devel, compiled by Visual C++ build 1929, 64-bit 2022-11-25 09:55:49.735 MSK [7648] LOG: listening on IPv4 address "127.0.0.1", port 60161 2022-11-25 09:55:49.773 MSK [2892] LOG: database system was shut down at 2022-11-25 09:55:49 MSK 2022-11-25 09:55:49.800 MSK [7648] LOG: database system is ready to accept connections 2022-11-25 09:55:49.890 MSK [7648] LOG: received SIGHUP, reloading configuration files 2022-11-25 09:55:50.003 MSK [84] [unknown] LOG: connection received: host=127.0.0.1 port=49822 2022-11-25 09:55:50.007 MSK [84] [unknown] FATAL: no pg_hba.conf entry for host "127.0.0.1", user "postgres", database "postgres", no encryption 2022-11-25 09:55:50.114 MSK [3356] [unknown] LOG: connection received: host=127.0.0.1 port=49823 2022-11-25 09:55:50.117 MSK [3356] [unknown] FATAL: no pg_hba.conf entry for host "127.0.0.1", user "postgres", database "postgres", no encryption 2022-11-25 09:55:50.201 MSK [7648] LOG: received immediate shutdown request 2022-11-25 09:55:50.212 MSK [7648] LOG: database system is shut down # Checking port 60161 # Found port 60161 Name: node Data directory: c:/projects/1c-master-7397/postgrespro/src\test\authentication/tmp_check/t_003_peer_node_data/pgdata Backup directory: c:/projects/1c-master-7397/postgrespro/src\test\authentication/tmp_check/t_003_peer_node_data/backup Archive directory: c:/projects/1c-master-7397/postgrespro/src\test\authentication/tmp_check/t_003_peer_node_data/archives Connection string: port=60161 host=127.0.0.1 Log file: c:/projects/1c-master-7397/postgrespro/src\test\authentication/tmp_check/log/003_peer_node.log # Running: initdb -D c:/projects/1c-master-7397/postgrespro/src\test\authentication/tmp_check/t_003_peer_node_data/pgdata -A trust -N The files belonging to this database system will be owned by user "postgres". This user must also own the server process. The database cluster will be initialized with locale "English_United States.1252". The default database encoding has accordingly been set to "WIN1252". The default text search configuration will be set to "english". Data page checksums are disabled. creating directory c:/projects/1c-master-7397/postgrespro/src/test/authentication/tmp_check/t_003_peer_node_data/pgdata ... ok creating subdirectories ... ok selecting dynamic shared memory implementation ... windows selecting default max_connections ... 100 selecting default shared_buffers ... 128MB selecting default time zone ... Europe/Moscow creating configuration files ... ok running bootstrap script ... ok performing post-bootstrap initialization ... ok Sync to disk skipped. The data directory might become corrupt if the operating system crashes. Success. You can now start the database server using: pg_ctl -D ^"c^:^/projects^/1c^-master^-7397^/postgrespro^/src^\test^\authentication^/tmp^_check^/t^_003^_peer^_node^_data^/pgdata^" -l logfile start # Running: c:/projects/1c-master-7397/postgrespro/Release/pg_regress/pg_regress --config-auth c:/projects/1c-master-7397/postgrespro/src\test\authentication/tmp_check/t_003_peer_node_data/pgdata ### Starting node "node" # Running: pg_ctl -w -D c:/projects/1c-master-7397/postgrespro/src\test\authentication/tmp_check/t_003_peer_node_data/pgdata -l c:/projects/1c-master-7397/postgrespro/src\test\authentication/tmp_check/log/003_peer_node.log -o --cluster-name=node start waiting for server to start done server started # Postmaster PID for node "node" is 7648 ### Reloading node "node" # Running: pg_ctl -D c:/projects/1c-master-7397/postgrespro/src\test\authentication/tmp_check/t_003_peer_node_data/pgdata reload
Re: [PATCH] Add peer authentication TAP test
On Fri, Nov 25, 2022 at 07:56:08AM +0300, Anton A. Melnikov wrote: > On Windows this test fails with error: > # connection error: 'psql: error: connection to server at "127.0.0.1", port > x failed: > # FATAL: no pg_hba.conf entry for host "127.0.0.1", user "buildfarm", > database "postgres", no encryption' > > May be disable this test for windows like in 001_password.pl and > 002_saslprep.pl? You are not using MSVC but MinGW, are you? The buildfarm members with TAP tests enabled are drongo, fairywren, bowerbord and jacana. Even though none of them are running the tests from src/test/authentication/, this is running on a periodic basis in the CI, where we are able to skip the test in MSVC already: postgresql:authentication / authentication/003_peer SKIP 9.73s So yes, it is plausible that we are missing more safeguards here. Your suggestion to skip under !$use_unix_sockets makes sense, as not having unix sockets is not going to work for peer and WIN32 needs SSPI to be secure with pg_regress. Where is your test failing? On the first $node->psql('postgres') at the beginning of the test? Just wondering.. -- Michael signature.asc Description: PGP signature
Re: [PATCH] Add peer authentication TAP test
Hello! On Windows this test fails with error: # connection error: 'psql: error: connection to server at "127.0.0.1", port x failed: # FATAL: no pg_hba.conf entry for host "127.0.0.1", user "buildfarm", database "postgres", no encryption' May be disable this test for windows like in 001_password.pl and 002_saslprep.pl? Best wishes, -- Anton A. Melnikov Postgres Professional: http://www.postgrespro.com The Russian Postgres Companycommit e4491b91729b307d29ce0205b455936b3a538373 Author: Anton A. Melnikov Date: Fri Nov 25 07:40:11 2022 +0300 Skip test 003_peer.pl for windows like 001_password.pl and 002_saslprep.pl diff --git a/src/test/authentication/t/003_peer.pl b/src/test/authentication/t/003_peer.pl index ce8408a4f8c..7454bf4a598 100644 --- a/src/test/authentication/t/003_peer.pl +++ b/src/test/authentication/t/003_peer.pl @@ -9,6 +9,11 @@ use warnings; use PostgreSQL::Test::Cluster; use PostgreSQL::Test::Utils; use Test::More; +if (!$use_unix_sockets) +{ + plan skip_all => + "authentication tests cannot run without Unix-domain sockets"; +} # Delete pg_hba.conf from the given node, add a new entry to it # and then execute a reload to refresh it.
Re: [PATCH] Add peer authentication TAP test
Hi, On 10/3/22 9:46 AM, Michael Paquier wrote: On Fri, Sep 30, 2022 at 07:51:29PM +0200, Drouvot, Bertrand wrote: Agree that it could be simplified, thanks for the hints! Attached a simplified version. While looking at that, I have noticed that it is possible to reduce the number of connection attempts (for example no need to re-test that the connection works when the map is not set, and the authn log would be the same with the map in place). Yeah that's right, thanks for simplifying further. Note that a path's meson.build needs a refresh for any new file added into the tree, with 003_peer.pl missing so this new test was not running in the recent CI runs. The indentation was also a bit wrong and I have tweaked a few comments, before finally applying it. Thanks! -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: [PATCH] Add peer authentication TAP test
On Fri, Sep 30, 2022 at 07:51:29PM +0200, Drouvot, Bertrand wrote: > Agree that it could be simplified, thanks for the hints! > > Attached a simplified version. While looking at that, I have noticed that it is possible to reduce the number of connection attempts (for example no need to re-test that the connection works when the map is not set, and the authn log would be the same with the map in place). Note that a path's meson.build needs a refresh for any new file added into the tree, with 003_peer.pl missing so this new test was not running in the recent CI runs. The indentation was also a bit wrong and I have tweaked a few comments, before finally applying it. -- Michael signature.asc Description: PGP signature
Re: [PATCH] Add peer authentication TAP test
Hi, On 9/30/22 2:00 AM, Michael Paquier wrote: On Wed, Sep 28, 2022 at 04:24:44PM +0900, Michael Paquier wrote: Hmm, indeed. It would be more reliable to rely on the contents returned by getpeereid()/getpwuid() after one successful peer connection, then use it in the map. I was wondering whether using stuff like getpwuid() in the perl script itself would be better, but it sounds less of a headache in terms of portability to just rely on authn_id via SYSTEM_USER to generate the contents of the correct map. By the way, on an extra read I have found a few things that can be simplified - I think that test_role() should be reworked so as the log patterns expected are passed down to connect_ok() and connect_fails() rather than involving find_in_log(). You still need find_in_log() to skip properly the case where peer is not supported by the platform, of course. - get_log_size() is not necessary. You should be able to get the same information with "-s $self->logfile". - Nit: a newline should be added at the end of 003_peer.pl. -- Agree that it could be simplified, thanks for the hints! Attached a simplified version. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.comdiff --git a/src/test/authentication/t/003_peer.pl b/src/test/authentication/t/003_peer.pl new file mode 100644 index 00..57bf2343fc --- /dev/null +++ b/src/test/authentication/t/003_peer.pl @@ -0,0 +1,108 @@ + +# Copyright (c) 2022, PostgreSQL Global Development Group + +# Tests for peer authentication and user name map. +# The test is skipped if the platform does not support peer authentication. + +use strict; +use warnings; +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +# Delete pg_hba.conf from the given node, add a new entry to it +# and then execute a reload to refresh it. +sub reset_pg_hba +{ + my $node = shift; + my $hba_method = shift; + + unlink($node->data_dir . '/pg_hba.conf'); + # just for testing purposes, use a continuation line. + $node->append_conf('pg_hba.conf', "local all all\\\n $hba_method"); + $node->reload; + return; +} + +# Test access for a single role, useful to wrap all tests into one. +sub test_role +{ + local $Test::Builder::Level = $Test::Builder::Level + 1; + + my ($node, $role, $method, $expected_res, %params) = @_; + my $status_string = 'failed'; + $status_string = 'success' if ($expected_res eq 0); + + my $connstr = "user=$role"; + my $testname = +"authentication $status_string for method $method, role $role"; + + if ($expected_res eq 0) + { +$node->connect_ok($connstr, $testname, %params); + } + else + { + # No checks of the error message, only the status code. + $node->connect_fails($connstr, $testname, %params); + } +} + +# find $pat in logfile of $node. +sub find_in_log +{ + my ($node, $pat) = @_; + + my $log = PostgreSQL::Test::Utils::slurp_file($node->logfile); + return 0 if (length($log) <= 0); + + return $log =~ m/$pat/; +} + +# Initialize primary node. +my $node = PostgreSQL::Test::Cluster->new('primary'); +$node->init; +$node->append_conf('postgresql.conf', "log_connections = on\n"); +$node->start; + +# Set pg_hba.conf with the peer authentication. +reset_pg_hba($node, 'peer'); + +# Check that peer authentication is supported on this platform. +$node->psql('postgres'); + +# If not supported, then skip the rest of the test. +if (find_in_log($node, qr/peer authentication is not supported on this platform/)) +{ +plan skip_all => 'peer authentication is not supported on this platform'; +} + +# Let's create a new user for the user name map test. +$node->safe_psql('postgres', + qq{CREATE USER testmapuser}); + +# Get the system_user to define the user name map test. +my $system_user = + $node->safe_psql('postgres', q(select (string_to_array(SYSTEM_USER, ':'))[2])); + +# This connection should succeed. +test_role($node, $system_user, 'peer', 0, +log_like => + [qr/connection authenticated: identity="$system_user" method=peer/]); + +# This connection should failed. +test_role($node, qq{testmapuser}, 'peer', 2, +log_like => [qr/Peer authentication failed for user "testmapuser"/]); + +# Define a user name map. +$node->append_conf('pg_ident.conf', qq{mypeermap $system_user testmapuser}); + +# Set pg_hba.conf with the peer authentication and the user name map. +reset_pg_hba($node, 'peer map=mypeermap'); + +# This connection should now succeed. +test_role($node, qq{testmapuser}, 'peer', 0, +log_like => + [qr/connection authenticated: identity="$system_user" method=peer/]); + +done_testing();
Re: [PATCH] Add peer authentication TAP test
On Wed, Sep 28, 2022 at 04:24:44PM +0900, Michael Paquier wrote: > Hmm, indeed. It would be more reliable to rely on the contents > returned by getpeereid()/getpwuid() after one successful peer > connection, then use it in the map. I was wondering whether using > stuff like getpwuid() in the perl script itself would be better, but > it sounds less of a headache in terms of portability to just rely on > authn_id via SYSTEM_USER to generate the contents of the correct map. By the way, on an extra read I have found a few things that can be simplified - I think that test_role() should be reworked so as the log patterns expected are passed down to connect_ok() and connect_fails() rather than involving find_in_log(). You still need find_in_log() to skip properly the case where peer is not supported by the platform, of course. - get_log_size() is not necessary. You should be able to get the same information with "-s $self->logfile". - Nit: a newline should be added at the end of 003_peer.pl. -- Michael signature.asc Description: PGP signature
Re: [PATCH] Add peer authentication TAP test
On Wed, Sep 28, 2022 at 09:12:57AM +0200, Drouvot, Bertrand wrote: > Maybe the variable name should be system_user instead or should we use > another way to get the "SYSTEM_USER" to be used in the map? Hmm, indeed. It would be more reliable to rely on the contents returned by getpeereid()/getpwuid() after one successful peer connection, then use it in the map. I was wondering whether using stuff like getpwuid() in the perl script itself would be better, but it sounds less of a headache in terms of portability to just rely on authn_id via SYSTEM_USER to generate the contents of the correct map. -- Michael signature.asc Description: PGP signature
Re: [PATCH] Add peer authentication TAP test
Hi, On 9/28/22 7:52 AM, Michael Paquier wrote: On Fri, Aug 26, 2022 at 10:43:43AM +0200, Drouvot, Bertrand wrote: During the work in [1] we created a new TAP test to test the SYSTEM_USER behavior with peer authentication. It turns out that there is currently no TAP test for the peer authentication, so we think (thanks Michael for the suggestion [2]) that it's better to split the work in [1] between "pure" SYSTEM_USER related work and the "pure" peer authentication TAP test work. That's the reason of this new thread, please find attached a patch to add a new TAP test for the peer authentication. +# Get the session_user to define the user name map test. +my $session_user = + $node->safe_psql('postgres', 'select session_user'); [...] +# Define a user name map. +$node->append_conf('pg_ident.conf', qq{mypeermap $session_user testmap$session_user}); + +# Set pg_hba.conf with the peer authentication and the user name map. +reset_pg_hba($node, 'peer map=mypeermap'); A map consists of a "MAPNAME SYSTEM_USER PG_USER". Why does this test use a Postgres role (from session_user) as the system user for the peer map? Thanks for looking at it! Initially selecting the session_user with a "local" connection and no user provided during the connection is a way I came up to retrieve the "SYSTEM_USER" to be used later on in the map. Maybe the variable name should be system_user instead or should we use another way to get the "SYSTEM_USER" to be used in the map? Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: [PATCH] Add peer authentication TAP test
On Fri, Aug 26, 2022 at 10:43:43AM +0200, Drouvot, Bertrand wrote: > During the work in [1] we created a new TAP test to test the SYSTEM_USER > behavior with peer authentication. > > It turns out that there is currently no TAP test for the peer > authentication, so we think (thanks Michael for the suggestion [2]) that > it's better to split the work in [1] between "pure" SYSTEM_USER related work > and the "pure" peer authentication TAP test work. > > That's the reason of this new thread, please find attached a patch to add a > new TAP test for the peer authentication. +# Get the session_user to define the user name map test. +my $session_user = + $node->safe_psql('postgres', 'select session_user'); [...] +# Define a user name map. +$node->append_conf('pg_ident.conf', qq{mypeermap $session_user testmap$session_user}); + +# Set pg_hba.conf with the peer authentication and the user name map. +reset_pg_hba($node, 'peer map=mypeermap'); A map consists of a "MAPNAME SYSTEM_USER PG_USER". Why does this test use a Postgres role (from session_user) as the system user for the peer map? -- Michael signature.asc Description: PGP signature
[PATCH] Add peer authentication TAP test
Hi hackers, During the work in [1] we created a new TAP test to test the SYSTEM_USER behavior with peer authentication. It turns out that there is currently no TAP test for the peer authentication, so we think (thanks Michael for the suggestion [2]) that it's better to split the work in [1] between "pure" SYSTEM_USER related work and the "pure" peer authentication TAP test work. That's the reason of this new thread, please find attached a patch to add a new TAP test for the peer authentication. [1]: https://www.postgresql.org/message-id/flat/7e692b8c-0b11-45db-1cad-3afc5b57409f%40amazon.com [2]: https://www.postgresql.org/message-id/YwgboqQUV1%2BY/k6z%40paquier.xyz Regards, -- Bertrand Drouvot Amazon Web Services: https://aws.amazon.com diff --git a/src/test/authentication/t/003_peer.pl b/src/test/authentication/t/003_peer.pl new file mode 100644 index 00..006c4405a5 --- /dev/null +++ b/src/test/authentication/t/003_peer.pl @@ -0,0 +1,140 @@ + +# Copyright (c) 2022, PostgreSQL Global Development Group + +# Tests for peer authentication and user name map. +# The test is skipped if the platform does not support peer authentication. + +use strict; +use warnings; +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +# Delete pg_hba.conf from the given node, add a new entry to it +# and then execute a reload to refresh it. +sub reset_pg_hba +{ + my $node = shift; + my $hba_method = shift; + + unlink($node->data_dir . '/pg_hba.conf'); + # just for testing purposes, use a continuation line. + $node->append_conf('pg_hba.conf', "local all all\\\n $hba_method"); + $node->reload; + return; +} + +# Test access for a single role, useful to wrap all tests into one. +sub test_role +{ + local $Test::Builder::Level = $Test::Builder::Level + 1; + + my ($node, $role, $method, $expected_res) = @_; + my $status_string = 'failed'; + $status_string = 'success' if ($expected_res eq 0); + + my $connstr = "user=$role"; + my $testname = +"authentication $status_string for method $method, role $role"; + + if ($expected_res eq 0) + { +$node->connect_ok($connstr, $testname); + } + else + { + # No checks of the error message, only the status code. + $node->connect_fails($connstr, $testname); + } +} + +# return the size of logfile of $node in bytes. +sub get_log_size +{ + my ($node) = @_; + + return (stat $node->logfile)[7]; +} + +# find $pat in logfile of $node after $off-th byte. +sub find_in_log +{ + my ($node, $pat, $off) = @_; + + $off = 0 unless defined $off; + my $log = PostgreSQL::Test::Utils::slurp_file($node->logfile); + return 0 if (length($log) <= $off); + + $log = substr($log, $off); + + return $log =~ m/$pat/; +} + +# Initialize primary node. +my $node = PostgreSQL::Test::Cluster->new('primary'); +$node->init; +$node->append_conf('postgresql.conf', "log_connections = on\n"); +$node->start; + +# Get the session_user to define the user name map test. +my $session_user = + $node->safe_psql('postgres', 'select session_user'); + +# Create a new user for the user name map test. +$node->safe_psql('postgres', + qq{CREATE USER testmap$session_user}); + +# Set pg_hba.conf with the peer authentication. +reset_pg_hba($node, 'peer'); + +# Check that peer authentication is supported on this platform. +my $logstart = get_log_size($node); + +$node->psql('postgres', undef, connstr => "user=$session_user"); + +# If not supported, then skip the rest of the test. +if (find_in_log($node, qr/peer authentication is not supported on this platform/, $logstart)) +{ +plan skip_all => 'peer authentication is not supported on this platform'; +} + +# It's supported so let's test peer authentication. +# This connection should succeed. +$logstart = get_log_size($node); +test_role($node, $session_user, 'peer', 0); + +ok( find_in_log( + $node, + qr/connection authenticated: identity="$session_user" method=peer/, $logstart), + "logfile: connection authenticated for method peer and identity $session_user"); + +# This connection should failed. +$logstart = get_log_size($node); +test_role($node, qq{testmap$session_user}, 'peer', 2); + +ok( find_in_log( + $node, + qr/Peer authentication failed for user "testmap$session_user"/, $logstart), + "logfile: Peer authentication failed for user testmap$session_user"); + +# Define a user name map. +$node->append_conf('pg_ident.conf', qq{mypeermap $session_user testmap$session_user}); + +# Set pg_hba.conf with the peer authentication and the user name map. +reset_pg_hba($node, 'peer map=mypeermap'); + +# This connection should now succeed. +$logstart = get_log_size($node); +test_role($node, qq{testmap$session_user}, 'peer', 0); + +ok( find_in_log( + $node, + qr/connection authenticated: identity="$session_user" method=peer/, $logstart), + "logfile: connection authenticated for method peer and identity $session_user"); + +ok(