Re: Refactoring SSL tests

2022-03-26 Thread Daniel Gustafsson
> On 9 Feb 2022, at 14:28, Andrew Dunstan  wrote:
> On 2/9/22 08:11, Daniel Gustafsson wrote:

>> Good point, done in the attached.
> 
> LGTM

Now that the recent changes to TAP and SSL tests have settled, I took another
pass at this.  After rebasing and fixing and polishing and taking it for
multiple spins on the CI I've pushed this today to master.

--
Daniel Gustafsson   https://vmware.com/





Re: Refactoring SSL tests

2022-02-02 Thread Andrew Dunstan


On 2/2/22 08:26, Daniel Gustafsson wrote:
> Thoughts?  I'm fairly sure there are many crimes against Perl in this patch,
> I'm happy to take pointers on how to improve that.


It feels a bit odd to me from a perl POV. I think it needs to more along
the lines of standard OO patterns. I'll take a stab at that based on
this, might be a few days.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com





Re: Refactoring SSL tests

2022-02-02 Thread Daniel Gustafsson
> On 2 Feb 2022, at 17:09, Andrew Dunstan  wrote:
> On 2/2/22 08:26, Daniel Gustafsson wrote:

>> Thoughts?  I'm fairly sure there are many crimes against Perl in this patch,
>> I'm happy to take pointers on how to improve that.
> 
> It feels a bit odd to me from a perl POV. I think it needs to more along
> the lines of standard OO patterns. I'll take a stab at that based on
> this, might be a few days.

That would be great, thanks!

--
Daniel Gustafsson   https://vmware.com/





Re: Refactoring SSL tests

2022-02-07 Thread Andrew Dunstan

On 2/2/22 14:50, Daniel Gustafsson wrote:
>> On 2 Feb 2022, at 17:09, Andrew Dunstan  wrote:
>> On 2/2/22 08:26, Daniel Gustafsson wrote:
>>> Thoughts?  I'm fairly sure there are many crimes against Perl in this patch,
>>> I'm happy to take pointers on how to improve that.
>> It feels a bit odd to me from a perl POV. I think it needs to more along
>> the lines of standard OO patterns. I'll take a stab at that based on
>> this, might be a few days.
> That would be great, thanks!
>

Here's the result of that surgery.  It's a little incomplete in that it
needs some POD adjustment, but I think the code is right - it passes
testing for me.

One of the advantages of this, apart from being more idiomatic, is that
by avoiding the use of package level variables you can have two
SSL::Server objects, one for OpenSSL and (eventually) one for NSS. This
was the original motivation for the recent install_path additions to
PostgreSQL::Test::Cluster, so it complements that work nicely.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com
From fa9bd034210aae4e11eed463e1a1365231c944f5 Mon Sep 17 00:00:00 2001
From: Andrew Dunstan 
Date: Mon, 7 Feb 2022 11:04:53 -0500
Subject: [PATCH] ssl test refactoring

---
 src/test/ssl/t/001_ssltests.pl| 144 +--
 src/test/ssl/t/002_scram.pl   |  21 +-
 src/test/ssl/t/003_sslinfo.pl |  33 ++-
 src/test/ssl/t/SSL/Backend/OpenSSL.pm | 227 ++
 src/test/ssl/t/SSL/Server.pm  | 332 ++
 src/test/ssl/t/SSLServer.pm   | 219 -
 6 files changed, 647 insertions(+), 329 deletions(-)
 create mode 100644 src/test/ssl/t/SSL/Backend/OpenSSL.pm
 create mode 100644 src/test/ssl/t/SSL/Server.pm
 delete mode 100644 src/test/ssl/t/SSLServer.pm

diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index b1fb15ce80..d5383a58ce 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -8,20 +8,24 @@ use PostgreSQL::Test::Cluster;
 use PostgreSQL::Test::Utils;
 use Test::More;
 
-use File::Copy;
-
 use FindBin;
 use lib $FindBin::RealBin;
 
-use SSLServer;
+use SSL::Server;
 
 if ($ENV{with_ssl} ne 'openssl')
 {
 	plan skip_all => 'OpenSSL not supported by this build';
 }
-else
+
+my $ssl_server = SSL::Server->new();
+sub sslkey
+{
+	return $ssl_server->sslkey(@_);
+}
+sub switch_server_cert
 {
-	plan tests => 110;
+	$ssl_server->switch_server_cert(@_);
 }
 
  Some configuration
@@ -36,39 +40,6 @@ my $SERVERHOSTCIDR = '127.0.0.1/32';
 # Allocation of base connection string shared among multiple tests.
 my $common_connstr;
 
-# The client's private key must not be world-readable, so take a copy
-# of the key stored in the code tree and update its permissions.
-#
-# This changes to using keys stored in a temporary path for the rest of
-# the tests. To get the full path for inclusion in connection strings, the
-# %key hash can be interrogated.
-my $cert_tempdir = PostgreSQL::Test::Utils::tempdir();
-my %key;
-my @keys = (
-	"client.key",   "client-revoked.key",
-	"client-der.key",   "client-encrypted-pem.key",
-	"client-encrypted-der.key", "client-dn.key");
-foreach my $keyfile (@keys)
-{
-	copy("ssl/$keyfile", "$cert_tempdir/$keyfile")
-	  or die
-	  "couldn't copy ssl/$keyfile to $cert_tempdir/$keyfile for permissions change: $!";
-	chmod 0600, "$cert_tempdir/$keyfile"
-	  or die "failed to change permissions on $cert_tempdir/$keyfile: $!";
-	$key{$keyfile} = PostgreSQL::Test::Utils::perl2host("$cert_tempdir/$keyfile");
-	$key{$keyfile} =~ s!\\!/!g if $PostgreSQL::Test::Utils::windows_os;
-}
-
-# Also make a copy of that explicitly world-readable.  We can't
-# necessarily rely on the file in the source tree having those
-# permissions.
-copy("ssl/client.key", "$cert_tempdir/client_wrongperms.key")
-  or die
-  "couldn't copy ssl/client_key to $cert_tempdir/client_wrongperms.key for permission change: $!";
-chmod 0644, "$cert_tempdir/client_wrongperms.key"
-  or die "failed to change permissions on $cert_tempdir/client_wrongperms.key: $!";
-$key{'client_wrongperms.key'} = PostgreSQL::Test::Utils::perl2host("$cert_tempdir/client_wrongperms.key");
-$key{'client_wrongperms.key'} =~ s!\\!/!g if $PostgreSQL::Test::Utils::windows_os;
  Set up the server.
 
 note "setting up data directory";
@@ -83,31 +54,31 @@ $node->start;
 
 # Run this before we lock down access below.
 my $result = $node->safe_psql('postgres', "SHOW ssl_library");
-is($result, 'OpenSSL', 'ssl_library parameter');
+is($result, $ssl_server->ssl_library(), 'ssl_library parameter');
 
-configure_test_server_for_ssl($node, $SERVERHOSTADDR, $SERVERHOSTCIDR,
-	'trust');
+$ssl_server->configure_test_server_for_ssl($node, $SERVERHOSTADDR,
+		   $SERVERHOSTCIDR,	'trust');
 
 note "testing password-protected keys";
 
-open my $sslconf, '>', $node->data_dir . "/sslconfig.conf";
-print $sslconf "ssl=on\n";
-print $sslconf "ssl_cert_file='server-cn-only.crt'\n"

Re: Refactoring SSL tests

2022-02-08 Thread Daniel Gustafsson
> On 7 Feb 2022, at 17:29, Andrew Dunstan  wrote:
> On 2/2/22 14:50, Daniel Gustafsson wrote:

>>> On 2 Feb 2022, at 17:09, Andrew Dunstan  wrote:
>>> On 2/2/22 08:26, Daniel Gustafsson wrote:
 Thoughts?  I'm fairly sure there are many crimes against Perl in this 
 patch,
 I'm happy to take pointers on how to improve that.
>>> It feels a bit odd to me from a perl POV. I think it needs to more along
>>> the lines of standard OO patterns. I'll take a stab at that based on
>>> this, might be a few days.
>> That would be great, thanks!
> 
> Here's the result of that surgery.  It's a little incomplete in that it
> needs some POD adjustment, but I think the code is right - it passes
> testing for me.

Confirmed, it passes all tests for me as well.

> One of the advantages of this, apart from being more idiomatic, is that
> by avoiding the use of package level variables you can have two
> SSL::Server objects, one for OpenSSL and (eventually) one for NSS. This
> was the original motivation for the recent install_path additions to
> PostgreSQL::Test::Cluster, so it complements that work nicely.

Agreed, this version is a clear improvement over my attempt. Thanks!

The attached v2 takes a stab at fixing up the POD sections.

--
Daniel Gustafsson   https://vmware.com/



v2-0001-ssl-test-refactoring.patch
Description: Binary data


Re: Refactoring SSL tests

2022-02-08 Thread Andrew Dunstan


On 2/8/22 09:24, Daniel Gustafsson wrote:
>
> The attached v2 takes a stab at fixing up the POD sections.


There a capitalization typo in SSL/Backend/OpenSSL.pm - looks like
that's my fault:


+  my $backend = SSL::backend::OpenSSL->new();


Also, I think we should document that SSL::Server::new() takes an
optional flavor parameter, in the absence of which it uses
$ENV{with_ssl}, and that 'openssl' is the only currently supported value.


cheers


andrew


-- 

Andrew Dunstan
EDB: https://www.enterprisedb.com





Re: Refactoring SSL tests

2022-02-09 Thread Daniel Gustafsson
> On 8 Feb 2022, at 16:46, Andrew Dunstan  wrote:

> There a capitalization typo in SSL/Backend/OpenSSL.pm - looks like
> that's my fault:
> 
> +  my $backend = SSL::backend::OpenSSL->new();

Fixed.

> Also, I think we should document that SSL::Server::new() takes an
> optional flavor parameter, in the absence of which it uses
> $ENV{with_ssl}, and that 'openssl' is the only currently supported value.

Good point, done in the attached.

--
Daniel Gustafsson   https://vmware.com/



v3-0001-ssl-test-refactoring.patch
Description: Binary data


Re: Refactoring SSL tests

2022-02-09 Thread Andrew Dunstan


On 2/9/22 08:11, Daniel Gustafsson wrote:
>> On 8 Feb 2022, at 16:46, Andrew Dunstan  wrote:
>> There a capitalization typo in SSL/Backend/OpenSSL.pm - looks like
>> that's my fault:
>>
>> +  my $backend = SSL::backend::OpenSSL->new();
> Fixed.
>
>> Also, I think we should document that SSL::Server::new() takes an
>> optional flavor parameter, in the absence of which it uses
>> $ENV{with_ssl}, and that 'openssl' is the only currently supported value.
> Good point, done in the attached.



LGTM


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com