Thread: [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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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