Refactoring SSL tests
As part of the NSS patchset (and Secure Transport before that), I had to refactor the SSL tests to handle different SSL libraries. The current tests and test module is quite tied to how OpenSSL works wrt setting up the server, the attached refactors this and abstracts the OpenSSL specifics more like how the rest of the codebase is set up. The tight coupling is of course not a problem right now, but I think this patch has more benefits making it a candidate for going in regardless of the fate of the NSS patchset. This is essentially the 0002 patch from that patchset with additional cleanup and documentation: * switch_server_cert takes a set of named parameters rather than a fixed set with defaults depending on each other, which made adding ssl_passphrase to it cumbersome. It also adds readability IMO. * SSLServer is renamed SSL::Server, which in turn use SSL::Backend::X where X is the backend pointed to by with_ssl. Each backend will implement its own module which is responsible for setting up keys/certs and to resolve sslkey values to their full paths. The idea is that the namespace will also allow for an SSL::Client in the future when we implment running client tests against different servers etc. * The modules are POD documented. * While not related to the refactor per se, the hardcoded number of planned tests is removed in favor of calling done_testing(). With this, adding a new SSL library is quite straightforward, I've done the legwork to test that =) I opted for avoiding too invasive changes leaving the tests somewhat easy to compare to back branches. 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. -- Daniel Gustafsson https://vmware.com/ 0001-Refactor-SSL-tests.patch Description: Binary data
Re: Refactoring SSL tests
> 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
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
> 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
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
> 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
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
> 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
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
