Minor cleanups in the SSL tests
When writing a new SSL test for another patch it struck me that the SSL tests
are doing configuration management without using the test framework API's. The
attached patches cleans this up, no testcases are altered as part of this.
0001 makes the test for PG_TEST_EXTRA a top-level if statement not attached to
any other conditional. There is no change in functionality, it's mainly for
readability (PG_TEST_EXTRA is it's own concept, not tied to library presence).
0002 ports over editing configfiles to using append_conf() instead of opening
and writing to them directly.
--
Daniel Gustafsson
Attachments:
0002-Use-library-functions-to-edit-config-in-SSL-tests.patchapplication/octet-stream; name=0002-Use-library-functions-to-edit-config-in-SSL-tests.patch; x-unix-mode=0644Download
From f8ff35fbc5816add4775d1aabae5700b29876439 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Mon, 13 May 2024 13:17:26 +0200
Subject: [PATCH 2/2] Use library functions to edit config in SSL tests
The SSL tests were editing the postgres configuration by directly
reading and writing the files rather than using append_conf() from
the testcode library.
---
src/test/ssl/t/SSL/Server.pm | 67 ++++++++++++++----------------------
1 file changed, 26 insertions(+), 41 deletions(-)
diff --git a/src/test/ssl/t/SSL/Server.pm b/src/test/ssl/t/SSL/Server.pm
index ca4c7b567b..e690bce4ca 100644
--- a/src/test/ssl/t/SSL/Server.pm
+++ b/src/test/ssl/t/SSL/Server.pm
@@ -191,17 +191,14 @@ sub configure_test_server_for_ssl
}
# enable logging etc.
- open my $conf, '>>', "$pgdata/postgresql.conf" or die $!;
- print $conf "fsync=off\n";
- print $conf "log_connections=on\n";
- print $conf "log_hostname=on\n";
- print $conf "listen_addresses='$serverhost'\n";
- print $conf "log_statement=all\n";
+ $node->append_conf('postgresql.conf', "fsync=off");
+ $node->append_conf('postgresql.conf', "log_connections=on");
+ $node->append_conf('postgresql.conf', "log_hostname=on");
+ $node->append_conf('postgresql.conf', "listen_addresses='$serverhost'");
+ $node->append_conf('postgresql.conf', "log_statement=all");
# enable SSL and set up server key
- print $conf "include 'sslconfig.conf'\n";
-
- close $conf;
+ $node->append_conf('postgresql.conf', "include 'sslconfig.conf'");
# SSL configuration will be placed here
open my $sslconf, '>', "$pgdata/sslconfig.conf" or die $!;
@@ -290,13 +287,12 @@ sub switch_server_cert
my %params = @_;
my $pgdata = $node->data_dir;
- open my $sslconf, '>', "$pgdata/sslconfig.conf" or die $!;
- print $sslconf "ssl=on\n";
- print $sslconf $backend->set_server_cert(\%params);
- print $sslconf "ssl_passphrase_command='"
- . $params{passphrase_cmd} . "'\n"
+ ok(unlink($node->data_dir . '/sslconfig.conf'));
+ $node->append_conf('sslconfig.conf', "ssl=on");
+ $node->append_conf('sslconfig.conf', $backend->set_server_cert(\%params));
+ $node->append_conf('sslconfig.conf', "ssl_passphrase_command='" . $params{passphrase_cmd} . "'")
if defined $params{passphrase_cmd};
- close $sslconf;
+ #$node->append_conf('sslconfig.conf', "ssl_snimode=off");
return if (defined($params{restart}) && $params{restart} eq 'no');
@@ -315,34 +311,23 @@ sub _configure_hba_for_ssl
# but seems best to keep it as narrow as possible for security reasons.
#
# When connecting to certdb, also check the client certificate.
- open my $hba, '>', "$pgdata/pg_hba.conf" or die $!;
- print $hba
- "# TYPE DATABASE USER ADDRESS METHOD OPTIONS\n";
- print $hba
- "hostssl trustdb md5testuser $servercidr md5\n";
- print $hba
- "hostssl trustdb all $servercidr $authmethod\n";
- print $hba
- "hostssl verifydb ssltestuser $servercidr $authmethod clientcert=verify-full\n";
- print $hba
- "hostssl verifydb anotheruser $servercidr $authmethod clientcert=verify-full\n";
- print $hba
- "hostssl verifydb yetanotheruser $servercidr $authmethod clientcert=verify-ca\n";
- print $hba
- "hostssl certdb all $servercidr cert\n";
- print $hba
- "hostssl certdb_dn all $servercidr cert clientname=DN map=dn\n",
- "hostssl certdb_dn_re all $servercidr cert clientname=DN map=dnre\n",
- "hostssl certdb_cn all $servercidr cert clientname=CN map=cn\n";
- close $hba;
+ ok(unlink($node->data_dir . '/pg_hba.conf'));
+ $node->append_conf('pg_hba.conf', "# TYPE DATABASE USER ADDRESS METHOD OPTIONS");
+ $node->append_conf('pg_hba.conf', "hostssl trustdb md5testuser $servercidr md5");
+ $node->append_conf('pg_hba.conf', "hostssl trustdb all $servercidr $authmethod");
+ $node->append_conf('pg_hba.conf', "hostssl verifydb ssltestuser $servercidr $authmethod clientcert=verify-full");
+ $node->append_conf('pg_hba.conf', "hostssl verifydb anotheruser $servercidr $authmethod clientcert=verify-full");
+ $node->append_conf('pg_hba.conf', "hostssl verifydb yetanotheruser $servercidr $authmethod clientcert=verify-ca");
+ $node->append_conf('pg_hba.conf', "hostssl certdb all $servercidr cert");
+ $node->append_conf('pg_hba.conf', "hostssl certdb_dn all $servercidr cert clientname=DN map=dn");
+ $node->append_conf('pg_hba.conf', "hostssl certdb_dn_re all $servercidr cert clientname=DN map=dnre");
+ $node->append_conf('pg_hba.conf', "hostssl certdb_cn all $servercidr cert clientname=CN map=cn");
# Also set the ident maps. Note: fields with commas must be quoted
- open my $map, ">", "$pgdata/pg_ident.conf" or die $!;
- print $map
- "# MAPNAME SYSTEM-USERNAME PG-USERNAME\n",
- "dn \"CN=ssltestuser-dn,OU=Testing,OU=Engineering,O=PGDG\" ssltestuser\n",
- "dnre \"/^.*OU=Testing,.*\$\" ssltestuser\n",
- "cn ssltestuser-dn ssltestuser\n";
+ ok(unlink($node->data_dir . '/pg_ident.conf'));
+ $node->append_conf('pg_ident.conf', "dn \"CN=ssltestuser-dn,OU=Testing,OU=Engineering,O=PGDG\" ssltestuser");
+ $node->append_conf('pg_ident.conf', "dnre \"/^.*OU=Testing,.*\$\" ssltestuser");
+ $node->append_conf('pg_ident.conf', "cn ssltestuser-dn ssltestuser");
return;
}
--
2.39.3 (Apple Git-146)
0001-Test-for-PG_TEST_EXTRA-separately.patchapplication/octet-stream; name=0001-Test-for-PG_TEST_EXTRA-separately.patch; x-unix-mode=0644Download
From 8049606d216a28b408df079c4a8c84f6dca8da0b Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Mon, 13 May 2024 13:11:34 +0200
Subject: [PATCH 1/2] Test for PG_TEST_EXTRA separately
PG_TEST_EXTRA is an override and should be tested for separately from
any other test.
---
src/test/ssl/t/001_ssltests.pl | 2 +-
src/test/ssl/t/002_scram.pl | 2 +-
src/test/ssl/t/003_sslinfo.pl | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index 94ff043c8e..8114fee59c 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -17,7 +17,7 @@ if ($ENV{with_ssl} ne 'openssl')
{
plan skip_all => 'OpenSSL not supported by this build';
}
-elsif (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bssl\b/)
+if (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bssl\b/)
{
plan skip_all =>
'Potentially unsafe test SSL not enabled in PG_TEST_EXTRA';
diff --git a/src/test/ssl/t/002_scram.pl b/src/test/ssl/t/002_scram.pl
index dd93224124..c5d8617e44 100644
--- a/src/test/ssl/t/002_scram.pl
+++ b/src/test/ssl/t/002_scram.pl
@@ -20,7 +20,7 @@ if ($ENV{with_ssl} ne 'openssl')
{
plan skip_all => 'OpenSSL not supported by this build';
}
-elsif (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bssl\b/)
+if (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bssl\b/)
{
plan skip_all =>
'Potentially unsafe test SSL not enabled in PG_TEST_EXTRA';
diff --git a/src/test/ssl/t/003_sslinfo.pl b/src/test/ssl/t/003_sslinfo.pl
index 2ae5724846..b6cfa5d0b0 100644
--- a/src/test/ssl/t/003_sslinfo.pl
+++ b/src/test/ssl/t/003_sslinfo.pl
@@ -18,7 +18,7 @@ if ($ENV{with_ssl} ne 'openssl')
{
plan skip_all => 'OpenSSL not supported by this build';
}
-elsif (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bssl\b/)
+if (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bssl\b/)
{
plan skip_all =>
'Potentially unsafe test SSL not enabled in PG_TEST_EXTRA';
--
2.39.3 (Apple Git-146)
On 16.05.24 09:24, Daniel Gustafsson wrote:
When writing a new SSL test for another patch it struck me that the SSL tests
are doing configuration management without using the test framework API's. The
attached patches cleans this up, no testcases are altered as part of this.0001 makes the test for PG_TEST_EXTRA a top-level if statement not attached to
any other conditional. There is no change in functionality, it's mainly for
readability (PG_TEST_EXTRA is it's own concept, not tied to library presence).
Makes sense to me.
0002 ports over editing configfiles to using append_conf() instead of opening
and writing to them directly.
Yes, it's probably preferable to use append_conf() here. You might want
to run your patch through pgperltidy. The result doesn't look bad, but
a bit different than what you had crafted.
append_conf() opens and closes the file for each call. It might be nice
if it could accept a list. Or you can just pass the whole block as one
string, like it was done for pg_ident.conf before.
On 16 May 2024, at 11:43, Peter Eisentraut <peter@eisentraut.org> wrote:
You might want to run your patch through pgperltidy. The result doesn't look bad, but a bit different than what you had crafted.
Ugh, I thought I had but clearly had forgotten. Fixed now.
append_conf() opens and closes the file for each call. It might be nice if it could accept a list. Or you can just pass the whole block as one string, like it was done for pg_ident.conf before.
The attached v2 pass the whole block as a here-doc which seemed like the best
option to retain readability of the config.
--
Daniel Gustafsson
Attachments:
v2-0002-Use-library-functions-to-edit-config-in-SSL-tests.patchapplication/octet-stream; name=v2-0002-Use-library-functions-to-edit-config-in-SSL-tests.patch; x-unix-mode=0644Download
From 449d5317078962aa02d23f9dc389b936d187a855 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Mon, 13 May 2024 13:17:26 +0200
Subject: [PATCH v2 2/2] Use library functions to edit config in SSL tests
The SSL tests were editing the postgres configuration by directly
reading and writing the files rather than using append_conf() from
the testcode library.
---
src/test/ssl/t/SSL/Server.pm | 79 +++++++++++++++++-------------------
1 file changed, 37 insertions(+), 42 deletions(-)
diff --git a/src/test/ssl/t/SSL/Server.pm b/src/test/ssl/t/SSL/Server.pm
index ca4c7b567b..6bf4af8fc8 100644
--- a/src/test/ssl/t/SSL/Server.pm
+++ b/src/test/ssl/t/SSL/Server.pm
@@ -191,17 +191,17 @@ sub configure_test_server_for_ssl
}
# enable logging etc.
- open my $conf, '>>', "$pgdata/postgresql.conf" or die $!;
- print $conf "fsync=off\n";
- print $conf "log_connections=on\n";
- print $conf "log_hostname=on\n";
- print $conf "listen_addresses='$serverhost'\n";
- print $conf "log_statement=all\n";
+ $node->append_conf('postgresql.conf', <<EOF
+fsync=off
+log_connections=on
+log_hostname=on
+listen_addresses='$serverhost'
+log_statement=all
+EOF
+ );
# enable SSL and set up server key
- print $conf "include 'sslconfig.conf'\n";
-
- close $conf;
+ $node->append_conf('postgresql.conf', "include 'sslconfig.conf'");
# SSL configuration will be placed here
open my $sslconf, '>', "$pgdata/sslconfig.conf" or die $!;
@@ -290,13 +290,12 @@ sub switch_server_cert
my %params = @_;
my $pgdata = $node->data_dir;
- open my $sslconf, '>', "$pgdata/sslconfig.conf" or die $!;
- print $sslconf "ssl=on\n";
- print $sslconf $backend->set_server_cert(\%params);
- print $sslconf "ssl_passphrase_command='"
- . $params{passphrase_cmd} . "'\n"
+ ok(unlink($node->data_dir . '/sslconfig.conf'));
+ $node->append_conf('sslconfig.conf', "ssl=on");
+ $node->append_conf('sslconfig.conf', $backend->set_server_cert(\%params));
+ $node->append_conf('sslconfig.conf',
+ "ssl_passphrase_command='" . $params{passphrase_cmd} . "'")
if defined $params{passphrase_cmd};
- close $sslconf;
return if (defined($params{restart}) && $params{restart} eq 'no');
@@ -315,35 +314,31 @@ sub _configure_hba_for_ssl
# but seems best to keep it as narrow as possible for security reasons.
#
# When connecting to certdb, also check the client certificate.
- open my $hba, '>', "$pgdata/pg_hba.conf" or die $!;
- print $hba
- "# TYPE DATABASE USER ADDRESS METHOD OPTIONS\n";
- print $hba
- "hostssl trustdb md5testuser $servercidr md5\n";
- print $hba
- "hostssl trustdb all $servercidr $authmethod\n";
- print $hba
- "hostssl verifydb ssltestuser $servercidr $authmethod clientcert=verify-full\n";
- print $hba
- "hostssl verifydb anotheruser $servercidr $authmethod clientcert=verify-full\n";
- print $hba
- "hostssl verifydb yetanotheruser $servercidr $authmethod clientcert=verify-ca\n";
- print $hba
- "hostssl certdb all $servercidr cert\n";
- print $hba
- "hostssl certdb_dn all $servercidr cert clientname=DN map=dn\n",
- "hostssl certdb_dn_re all $servercidr cert clientname=DN map=dnre\n",
- "hostssl certdb_cn all $servercidr cert clientname=CN map=cn\n";
- close $hba;
+ ok(unlink($node->data_dir . '/pg_hba.conf'));
+ $node->append_conf(
+ 'pg_hba.conf', <<EOF
+# TYPE DATABASE USER ADDRESS METHOD OPTIONS
+hostssl trustdb md5testuser $servercidr md5
+hostssl trustdb all $servercidr $authmethod
+hostssl verifydb ssltestuser $servercidr $authmethod clientcert=verify-full
+hostssl verifydb anotheruser $servercidr $authmethod clientcert=verify-full
+hostssl verifydb yetanotheruser $servercidr $authmethod clientcert=verify-ca
+hostssl certdb all $servercidr cert
+hostssl certdb_dn all $servercidr cert clientname=DN map=dn
+hostssl certdb_dn_re all $servercidr cert clientname=DN map=dnre
+hostssl certdb_cn all $servercidr cert clientname=CN map=cn
+EOF
+ );
# Also set the ident maps. Note: fields with commas must be quoted
- open my $map, ">", "$pgdata/pg_ident.conf" or die $!;
- print $map
- "# MAPNAME SYSTEM-USERNAME PG-USERNAME\n",
- "dn \"CN=ssltestuser-dn,OU=Testing,OU=Engineering,O=PGDG\" ssltestuser\n",
- "dnre \"/^.*OU=Testing,.*\$\" ssltestuser\n",
- "cn ssltestuser-dn ssltestuser\n";
-
+ ok(unlink($node->data_dir . '/pg_ident.conf'));
+ $node->append_conf(
+ 'pg_ident.conf', <<EOF
+dn "CN=ssltestuser-dn,OU=Testing,OU=Engineering,O=PGDG" ssltestuser
+dnre "/^.*OU=Testing,.*\$" ssltestuser
+cn ssltestuser-dn ssltestuser
+EOF
+ );
return;
}
--
2.39.3 (Apple Git-146)
v2-0001-Test-for-PG_TEST_EXTRA-separately.patchapplication/octet-stream; name=v2-0001-Test-for-PG_TEST_EXTRA-separately.patch; x-unix-mode=0644Download
From 9628ea09b1d87393d56427068abcdfca4bbd51d1 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Mon, 13 May 2024 13:11:34 +0200
Subject: [PATCH v2 1/2] Test for PG_TEST_EXTRA separately
PG_TEST_EXTRA is an override and should be tested for separately from
any other test.
---
src/test/ssl/t/001_ssltests.pl | 2 +-
src/test/ssl/t/002_scram.pl | 2 +-
src/test/ssl/t/003_sslinfo.pl | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index 68c1b6b41f..e0953fb2ff 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -17,7 +17,7 @@ if ($ENV{with_ssl} ne 'openssl')
{
plan skip_all => 'OpenSSL not supported by this build';
}
-elsif (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bssl\b/)
+if (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bssl\b/)
{
plan skip_all =>
'Potentially unsafe test SSL not enabled in PG_TEST_EXTRA';
diff --git a/src/test/ssl/t/002_scram.pl b/src/test/ssl/t/002_scram.pl
index dd93224124..c5d8617e44 100644
--- a/src/test/ssl/t/002_scram.pl
+++ b/src/test/ssl/t/002_scram.pl
@@ -20,7 +20,7 @@ if ($ENV{with_ssl} ne 'openssl')
{
plan skip_all => 'OpenSSL not supported by this build';
}
-elsif (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bssl\b/)
+if (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bssl\b/)
{
plan skip_all =>
'Potentially unsafe test SSL not enabled in PG_TEST_EXTRA';
diff --git a/src/test/ssl/t/003_sslinfo.pl b/src/test/ssl/t/003_sslinfo.pl
index 2ae5724846..b6cfa5d0b0 100644
--- a/src/test/ssl/t/003_sslinfo.pl
+++ b/src/test/ssl/t/003_sslinfo.pl
@@ -18,7 +18,7 @@ if ($ENV{with_ssl} ne 'openssl')
{
plan skip_all => 'OpenSSL not supported by this build';
}
-elsif (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bssl\b/)
+if (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bssl\b/)
{
plan skip_all =>
'Potentially unsafe test SSL not enabled in PG_TEST_EXTRA';
--
2.39.3 (Apple Git-146)
On 16.05.24 23:27, Daniel Gustafsson wrote:
On 16 May 2024, at 11:43, Peter Eisentraut <peter@eisentraut.org> wrote:
You might want to run your patch through pgperltidy. The result doesn't look bad, but a bit different than what you had crafted.
Ugh, I thought I had but clearly had forgotten. Fixed now.
append_conf() opens and closes the file for each call. It might be nice if it could accept a list. Or you can just pass the whole block as one string, like it was done for pg_ident.conf before.
The attached v2 pass the whole block as a here-doc which seemed like the best
option to retain readability of the config.
Works for me.
On 17 May 2024, at 07:57, Peter Eisentraut <peter@eisentraut.org> wrote:
On 16.05.24 23:27, Daniel Gustafsson wrote:
On 16 May 2024, at 11:43, Peter Eisentraut <peter@eisentraut.org> wrote:
You might want to run your patch through pgperltidy. The result doesn't look bad, but a bit different than what you had crafted.Ugh, I thought I had but clearly had forgotten. Fixed now.
append_conf() opens and closes the file for each call. It might be nice if it could accept a list. Or you can just pass the whole block as one string, like it was done for pg_ident.conf before.
The attached v2 pass the whole block as a here-doc which seemed like the best
option to retain readability of the config.Works for me.
Thanks for review. Once the tree opens up for v18 I'll go ahead with this.
--
Daniel Gustafsson
On 17 May 2024, at 09:58, Daniel Gustafsson <daniel@yesql.se> wrote:
On 17 May 2024, at 07:57, Peter Eisentraut <peter@eisentraut.org> wrote:
On 16.05.24 23:27, Daniel Gustafsson wrote:
On 16 May 2024, at 11:43, Peter Eisentraut <peter@eisentraut.org> wrote:
You might want to run your patch through pgperltidy. The result doesn't look bad, but a bit different than what you had crafted.Ugh, I thought I had but clearly had forgotten. Fixed now.
append_conf() opens and closes the file for each call. It might be nice if it could accept a list. Or you can just pass the whole block as one string, like it was done for pg_ident.conf before.
The attached v2 pass the whole block as a here-doc which seemed like the best
option to retain readability of the config.Works for me.
Thanks for review. Once the tree opens up for v18 I'll go ahead with this.
This has now been pushed after a little bit of editorializing and another
pgperltidy run.
--
Daniel Gustafsson