Horton

Thread

Commits

  1. Skip TAP test for peer authentication if there are no unix-domain sockets

  2. Add authentication TAP test for peer authentication

  1. [PATCH] Add peer authentication TAP test

    Drouvot, Bertrand <bdrouvot@amazon.com> — 2022-08-26T08:43:43Z

    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

  2. Re: [PATCH] Add peer authentication TAP test

    Michael Paquier <michael@paquier.xyz> — 2022-09-28T05:52:26Z

    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

  3. Re: [PATCH] Add peer authentication TAP test

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2022-09-28T07:12:57Z

    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

  4. Re: [PATCH] Add peer authentication TAP test

    Michael Paquier <michael@paquier.xyz> — 2022-09-28T07:24:44Z

    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

  5. Re: [PATCH] Add peer authentication TAP test

    Michael Paquier <michael@paquier.xyz> — 2022-09-30T00:00:54Z

    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

  6. Re: [PATCH] Add peer authentication TAP test

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2022-09-30T17:51:29Z

    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

  7. Re: [PATCH] Add peer authentication TAP test

    Michael Paquier <michael@paquier.xyz> — 2022-10-03T07:46:38Z

    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

  8. Re: [PATCH] Add peer authentication TAP test

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2022-10-03T08:59:21Z

    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

  9. Re: [PATCH] Add peer authentication TAP test

    Anton A. Melnikov <aamelnikov@inbox.ru> — 2022-11-25T04:56:08Z

    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

  10. Re: [PATCH] Add peer authentication TAP test

    Michael Paquier <michael@paquier.xyz> — 2022-11-25T05:18:51Z

    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

  11. Re: [PATCH] Add peer authentication TAP test

    Anton A. Melnikov <aamelnikov@inbox.ru> — 2022-11-25T07:13:29Z

    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

  12. Re: [PATCH] Add peer authentication TAP test

    Michael Paquier <michael@paquier.xyz> — 2022-11-25T07:34:12Z

    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

  13. Re: [PATCH] Add peer authentication TAP test

    Anton A. Melnikov <aamelnikov@inbox.ru> — 2022-11-25T07:40:43Z

    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