Add regular expression testing for user name mapping in the peer authentication TAP test

Started by Drouvot, Bertrandabout 3 years ago5 messages
#1Drouvot, Bertrand
bertranddrouvot.pg@gmail.com
1 attachment(s)

Hi hackers,

while working on [1]/messages/by-id/4f55303e-62c1-1072-61db-fbfb30bd66c8@gmail.com, I thought it could also be useful to add regular
expression testing for user name mapping in the peer authentication TAP
test.

This kind of test already exists in kerberos/t/001_auth.pl but the
proposed one in the peer authentication testing would probably be more
widely tested.

Please find attached a patch proposal to do so.

[1]: /messages/by-id/4f55303e-62c1-1072-61db-fbfb30bd66c8@gmail.com
/messages/by-id/4f55303e-62c1-1072-61db-fbfb30bd66c8@gmail.com

Looking forward to your feedback,

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Attachments:

v1-0001-tap-peer-test-with-regexp.patchtext/plain; charset=UTF-8; name=v1-0001-tap-peer-test-with-regexp.patchDownload
diff --git a/src/test/authentication/t/003_peer.pl b/src/test/authentication/t/003_peer.pl
index fc951dea06..4e2efbe5e3 100644
--- a/src/test/authentication/t/003_peer.pl
+++ b/src/test/authentication/t/003_peer.pl
@@ -23,18 +23,34 @@ sub reset_pg_hba
 	return;
 }
 
+# Delete pg_ident.conf from the given node, add a new entry to it
+# and then execute a reload to refresh it.
+sub reset_pg_ident
+{
+	my $node        = shift;
+	my $map_name    = shift;
+	my $system_user = shift;
+	my $pg_user     = shift;
+
+	unlink($node->data_dir . '/pg_ident.conf');
+	$node->append_conf('pg_ident.conf', "$map_name $system_user $pg_user");
+	$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 ($node, $role, $method, $expected_res, $test_details, %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";
+	  "authentication $status_string for method $method, role $role "
+	  . $test_details;
 
 	if ($expected_res eq 0)
 	{
@@ -87,16 +103,43 @@ my $system_user =
 # Tests without the user name map.
 # Failure as connection is attempted with a database role not mapping
 # to an authorized system user.
-test_role($node, qq{testmapuser}, 'peer', 2,
+test_role(
+	$node, qq{testmapuser}, 'peer', 2,
+	'without user name map',
 	log_like => [qr/Peer authentication failed for user "testmapuser"/]);
 
 # Tests with a user name map.
-$node->append_conf('pg_ident.conf', qq{mypeermap $system_user testmapuser});
+reset_pg_ident($node, 'mypeermap', $system_user, 'testmapuser');
 reset_pg_hba($node, 'peer map=mypeermap');
 
 # Success as the database role matches with the system user in the map.
-test_role($node, qq{testmapuser}, 'peer', 0,
+test_role($node, qq{testmapuser}, 'peer', 0, 'with user name map',
 	log_like =>
 	  [qr/connection authenticated: identity="$system_user" method=peer/]);
 
+# Test with regular expression in user name map.
+my $last_system_user_char = substr($system_user, -1);
+
+# The regular expression matches.
+reset_pg_ident($node, 'mypeermap', qq{/^.*$last_system_user_char\$},
+	'testmapuser');
+test_role(
+	$node,
+	qq{testmapuser},
+	'peer',
+	0,
+	'with regular expression in user name map',
+	log_like =>
+	  [qr/connection authenticated: identity="$system_user" method=peer/]);
+
+# The regular expression does not match.
+reset_pg_ident($node, 'mypeermap', '/^$', 'testmapuser');
+test_role(
+	$node,
+	qq{testmapuser},
+	'peer',
+	2,
+	'with regular expression in user name map',
+	log_like => [qr/no match in usermap "mypeermap" for user "testmapuser"/]);
+
 done_testing();
#2Michael Paquier
michael@paquier.xyz
In reply to: Drouvot, Bertrand (#1)
Re: Add regular expression testing for user name mapping in the peer authentication TAP test

On Fri, Oct 14, 2022 at 06:31:15PM +0200, Drouvot, Bertrand wrote:

while working on [1], I thought it could also be useful to add regular
expression testing for user name mapping in the peer authentication TAP
test.

Good idea now that we have a bit more coverage in the authentication
tests.

+# Test with regular expression in user name map.
+my $last_system_user_char = substr($system_user, -1);

This would attach to the regex the last character of the system user.
I would perhaps have used more characters than that (-3?), as substr()
with a negative number larger than the string given in input would
give the entire string. That's a nit, though.

+# The regular expression does not match.
+reset_pg_ident($node, 'mypeermap', '/^$', 'testmapuser');

This matches only an empty string, my brain gets that right?
--
Michael

#3Drouvot, Bertrand
bertranddrouvot.pg@gmail.com
In reply to: Michael Paquier (#2)
1 attachment(s)
Re: Add regular expression testing for user name mapping in the peer authentication TAP test

Hi,

On 10/15/22 5:11 AM, Michael Paquier wrote:

On Fri, Oct 14, 2022 at 06:31:15PM +0200, Drouvot, Bertrand wrote:

while working on [1], I thought it could also be useful to add regular
expression testing for user name mapping in the peer authentication TAP
test.

Good idea now that we have a bit more coverage in the authentication
tests.

Thanks for looking at it!

+# Test with regular expression in user name map.
+my $last_system_user_char = substr($system_user, -1);

This would attach to the regex the last character of the system user.

Right.

I would perhaps have used more characters than that (-3?), as substr()
with a negative number larger than the string given in input would
give the entire string. That's a nit, though.

I don't have a strong opinion on this, so let's extract the last 3
characters. This is what v2 attached does.

+# The regular expression does not match.
+reset_pg_ident($node, 'mypeermap', '/^$', 'testmapuser');

This matches only an empty string, my brain gets that right?

Right. Giving a second thought to the non matching case, I think I'd
prefer to concatenate the system_user to the system_user instead. This
is what v2 does.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Attachments:

v2-0001-tap-peer-test-with-regexp.patchtext/plain; charset=UTF-8; name=v2-0001-tap-peer-test-with-regexp.patchDownload
diff --git a/src/test/authentication/t/003_peer.pl b/src/test/authentication/t/003_peer.pl
index fc951dea06..e719b05d0f 100644
--- a/src/test/authentication/t/003_peer.pl
+++ b/src/test/authentication/t/003_peer.pl
@@ -23,18 +23,34 @@ sub reset_pg_hba
 	return;
 }
 
+# Delete pg_ident.conf from the given node, add a new entry to it
+# and then execute a reload to refresh it.
+sub reset_pg_ident
+{
+	my $node        = shift;
+	my $map_name    = shift;
+	my $system_user = shift;
+	my $pg_user     = shift;
+
+	unlink($node->data_dir . '/pg_ident.conf');
+	$node->append_conf('pg_ident.conf', "$map_name $system_user $pg_user");
+	$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 ($node, $role, $method, $expected_res, $test_details, %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";
+	  "authentication $status_string for method $method, role $role "
+	  . $test_details;
 
 	if ($expected_res eq 0)
 	{
@@ -87,16 +103,50 @@ my $system_user =
 # Tests without the user name map.
 # Failure as connection is attempted with a database role not mapping
 # to an authorized system user.
-test_role($node, qq{testmapuser}, 'peer', 2,
+test_role(
+	$node, qq{testmapuser}, 'peer', 2,
+	'without user name map',
 	log_like => [qr/Peer authentication failed for user "testmapuser"/]);
 
 # Tests with a user name map.
-$node->append_conf('pg_ident.conf', qq{mypeermap $system_user testmapuser});
+reset_pg_ident($node, 'mypeermap', $system_user, 'testmapuser');
 reset_pg_hba($node, 'peer map=mypeermap');
 
 # Success as the database role matches with the system user in the map.
-test_role($node, qq{testmapuser}, 'peer', 0,
+test_role($node, qq{testmapuser}, 'peer', 0, 'with user name map',
 	log_like =>
 	  [qr/connection authenticated: identity="$system_user" method=peer/]);
 
+# Test with regular expression in user name map.
+# Extract the last 3 characters from the system_user
+# or the entire system_user (if its length is <= -3).
+my $regex_test_string = substr($system_user, -3);
+
+# The regular expression matches.
+reset_pg_ident($node, 'mypeermap', qq{/^.*$regex_test_string\$},
+	'testmapuser');
+test_role(
+	$node,
+	qq{testmapuser},
+	'peer',
+	0,
+	'with regular expression in user name map',
+	log_like =>
+	  [qr/connection authenticated: identity="$system_user" method=peer/]);
+
+
+# Concatenate system_user to system_user.
+$regex_test_string = $system_user . $system_user;
+
+# The regular expression does not match.
+reset_pg_ident($node, 'mypeermap', qq{/^.*$regex_test_string\$},
+	'testmapuser');
+test_role(
+	$node,
+	qq{testmapuser},
+	'peer',
+	2,
+	'with regular expression in user name map',
+	log_like => [qr/no match in usermap "mypeermap" for user "testmapuser"/]);
+
 done_testing();
#4Michael Paquier
michael@paquier.xyz
In reply to: Drouvot, Bertrand (#3)
Re: Add regular expression testing for user name mapping in the peer authentication TAP test

On Sat, Oct 15, 2022 at 07:54:30AM +0200, Drouvot, Bertrand wrote:

Right. Giving a second thought to the non matching case, I think I'd prefer
to concatenate the system_user to the system_user instead. This is what v2
does.

Fine by me, so applied v2. Thanks!
--
Michael

#5Drouvot, Bertrand
bertranddrouvot.pg@gmail.com
In reply to: Michael Paquier (#4)
Re: Add regular expression testing for user name mapping in the peer authentication TAP test

Hi,

On 10/17/22 4:07 AM, Michael Paquier wrote:

On Sat, Oct 15, 2022 at 07:54:30AM +0200, Drouvot, Bertrand wrote:

Right. Giving a second thought to the non matching case, I think I'd prefer
to concatenate the system_user to the system_user instead. This is what v2
does.

Fine by me, so applied v2. Thanks!

Thanks!

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com