[PATCH] Add peer authentication TAP test
Hi hackers,
During the work in [1]/messages/by-id/7e692b8c-0b11-45db-1cad-3afc5b57409f@amazon.com 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]/messages/by-id/YwgboqQUV1+Y/k6z@paquier.xyz) that
it's better to split the work in [1]/messages/by-id/7e692b8c-0b11-45db-1cad-3afc5b57409f@amazon.com 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]: /messages/by-id/7e692b8c-0b11-45db-1cad-3afc5b57409f@amazon.com
/messages/by-id/7e692b8c-0b11-45db-1cad-3afc5b57409f@amazon.com
[2]: /messages/by-id/YwgboqQUV1+Y/k6z@paquier.xyz
Regards,
--
Bertrand Drouvot
Amazon Web Services: https://aws.amazon.com
Attachments:
v1-0001-peer-authentication-tap-test.patchtext/plain; charset=UTF-8; name=v1-0001-peer-authentication-tap-test.patchDownload
diff --git a/src/test/authentication/t/003_peer.pl b/src/test/authentication/t/003_peer.pl
new file mode 100644
index 0000000000..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( find_in_log(
+ $node,
+ qr/connection authorized: user=testmap$session_user/, $logstart),
+ "logfile: connection authorized for user testmap$session_user");
+
+done_testing();
\ No newline at end of file
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
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
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
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
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.com
Attachments:
v2-0001-peer-authentication-tap-test.patchtext/plain; charset=UTF-8; name=v2-0001-peer-authentication-tap-test.patchDownload
diff --git a/src/test/authentication/t/003_peer.pl b/src/test/authentication/t/003_peer.pl
new file mode 100644
index 0000000000..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();
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
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
Hello!
On Windows this test fails with error:
# connection error: 'psql: error: connection to server at "127.0.0.1", port xxxxx 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 Company
Attachments:
v1-0001-Skip-003_peer.pl-for-windows.patchtext/x-patch; charset=UTF-8; name=v1-0001-Skip-003_peer.pl-for-windows.patchDownload
commit e4491b91729b307d29ce0205b455936b3a538373
Author: Anton A. Melnikov <a.melnikov@postgrespro.ru>
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.
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 xxxxx 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
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 Company
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 runLogs 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
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 runLogs 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