From e441a7c3cf1d807c998ce60c25f72a370d4f16af Mon Sep 17 00:00:00 2001 From: Daniel Gustafsson Date: Mon, 8 Feb 2021 23:52:34 +0100 Subject: [PATCH v2] Refactor SSL testharness for multiple library The SSL testharness was fully tied to OpenSSL in the way the server was set up and reconfigured. This refactors the SSLServer module into a SSL library agnostic SSL/Server module which in turn use SSL/Backend/ modules for the implementation details. No changes are done to the actual tests, this only change how setup and teardown is performed. --- src/test/ssl/t/001_ssltests.pl | 80 ++++------ src/test/ssl/t/002_scram.pl | 8 +- src/test/ssl/t/SSL/Backend/OpenSSL.pm | 139 ++++++++++++++++++ .../ssl/t/{SSLServer.pm => SSL/Server.pm} | 88 ++++++----- 4 files changed, 210 insertions(+), 105 deletions(-) create mode 100644 src/test/ssl/t/SSL/Backend/OpenSSL.pm rename src/test/ssl/t/{SSLServer.pm => SSL/Server.pm} (78%) diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl index bfada03d3e..849df5dedc 100644 --- a/src/test/ssl/t/001_ssltests.pl +++ b/src/test/ssl/t/001_ssltests.pl @@ -4,12 +4,10 @@ use PostgresNode; use TestLib; use Test::More; -use File::Copy; - use FindBin; use lib $FindBin::RealBin; -use SSLServer; +use SSL::Server; if ($ENV{with_ssl} ne 'openssl') { @@ -32,32 +30,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 ssl/client.key to ssl/client_tmp.key etc for the rest -# of the tests. -my @keys = ( - "client", "client-revoked", - "client-der", "client-encrypted-pem", - "client-encrypted-der"); -foreach my $key (@keys) -{ - copy("ssl/${key}.key", "ssl/${key}_tmp.key") - or die - "couldn't copy ssl/${key}.key to ssl/${key}_tmp.key for permissions change: $!"; - chmod 0600, "ssl/${key}_tmp.key" - or die "failed to change permissions on ssl/${key}_tmp.key: $!"; -} - -# Also make a copy of that explicitly world-readable. We can't -# necessarily rely on the file in the source tree having those -# permissions. Add it to @keys to include it in the final clean -# up phase. -copy("ssl/client.key", "ssl/client_wrongperms_tmp.key"); -chmod 0644, "ssl/client_wrongperms_tmp.key"; -push @keys, 'client_wrongperms'; - #### Set up the server. note "setting up data directory"; @@ -72,31 +44,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'); 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"; -print $sslconf "ssl_key_file='server-password.key'\n"; -print $sslconf "ssl_passphrase_command='echo wrongpassword'\n"; -close $sslconf; +switch_server_cert($node, + certfile => 'server-cn-only', + cafile => 'root+client_ca', + keyfile => 'server-password', + passphrase_cmd => 'echo wrongpassword', + restart => 'no' ); command_fails( [ 'pg_ctl', '-D', $node->data_dir, '-l', $node->logfile, 'restart' ], 'restart fails with password-protected key file with wrong password'); $node->_update_pid(0); -open $sslconf, '>', $node->data_dir . "/sslconfig.conf"; -print $sslconf "ssl=on\n"; -print $sslconf "ssl_cert_file='server-cn-only.crt'\n"; -print $sslconf "ssl_key_file='server-password.key'\n"; -print $sslconf "ssl_passphrase_command='echo secret1'\n"; -close $sslconf; +switch_server_cert($node, + certfile => 'server-cn-only', + cafile => 'root+client_ca', + keyfile => 'server-password', + passphrase_cmd => 'echo secret1', + restart => 'no'); command_ok( [ 'pg_ctl', '-D', $node->data_dir, '-l', $node->logfile, 'restart' ], @@ -129,7 +101,7 @@ command_ok( note "running client tests"; -switch_server_cert($node, 'server-cn-only'); +switch_server_cert($node, certfile => 'server-cn-only'); $common_connstr = "user=ssltestuser dbname=trustdb sslcert=invalid hostaddr=$SERVERHOSTADDR host=common-name.pg-ssltest.test"; @@ -254,7 +226,7 @@ test_connect_fails( "mismatch between host name and server certificate sslmode=verify-full"); # Test Subject Alternative Names. -switch_server_cert($node, 'server-multiple-alt-names'); +switch_server_cert($node, certfile => 'server-multiple-alt-names'); $common_connstr = "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR sslmode=verify-full"; @@ -285,7 +257,7 @@ test_connect_fails( # Test certificate with a single Subject Alternative Name. (this gives a # slightly different error message, that's all) -switch_server_cert($node, 'server-single-alt-name'); +switch_server_cert($node, certfile => 'server-single-alt-name'); $common_connstr = "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR sslmode=verify-full"; @@ -309,7 +281,7 @@ test_connect_fails( # Test server certificate with a CN and SANs. Per RFCs 2818 and 6125, the CN # should be ignored when the certificate has both. -switch_server_cert($node, 'server-cn-and-alt-names'); +switch_server_cert($node, certfile => 'server-cn-and-alt-names'); $common_connstr = "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR sslmode=verify-full"; @@ -330,7 +302,7 @@ test_connect_fails( # Finally, test a server certificate that has no CN or SANs. Of course, that's # not a very sensible certificate, but libpq should handle it gracefully. -switch_server_cert($node, 'server-no-names'); +switch_server_cert($node, certfile => 'server-no-names'); $common_connstr = "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR"; @@ -345,7 +317,7 @@ test_connect_fails( "server certificate without CN or SANs sslmode=verify-full"); # Test that the CRL works -switch_server_cert($node, 'server-revoked'); +switch_server_cert($node, certfile => 'server-revoked'); $common_connstr = "user=ssltestuser dbname=trustdb sslcert=invalid hostaddr=$SERVERHOSTADDR host=common-name.pg-ssltest.test"; @@ -405,6 +377,8 @@ test_connect_fails( ### ### Test certificate authorization. +switch_server_cert($node, certfile => 'server-revoked'); + note "running server tests"; $common_connstr = @@ -552,7 +526,7 @@ test_connect_ok( ); # intermediate client_ca.crt is provided by client, and isn't in server's ssl_ca_file -switch_server_cert($node, 'server-cn-only', 'root_ca'); +switch_server_cert($node, certfile => 'server-cn-only', cafile => 'root_ca'); $common_connstr = "user=ssltestuser dbname=certdb sslkey=ssl/client_tmp.key sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR"; @@ -564,7 +538,7 @@ test_connect_fails($common_connstr, "sslmode=require sslcert=ssl/client.crt", qr/SSL error/, "intermediate client certificate is missing"); # test server-side CRL directory -switch_server_cert($node, 'server-cn-only', undef, undef, 'root+client-crldir'); +switch_server_cert($node, certfile => 'server-cn-only', crldir => 'root+client-crldir'); # revoked client cert test_connect_fails( @@ -574,7 +548,5 @@ test_connect_fails( "certificate authorization fails with revoked client cert with server-side CRL directory"); # clean up -foreach my $key (@keys) -{ - unlink("ssl/${key}_tmp.key"); -} + +SSL::Server::cleanup(); diff --git a/src/test/ssl/t/002_scram.pl b/src/test/ssl/t/002_scram.pl index 410b9e910d..ceb2301642 100644 --- a/src/test/ssl/t/002_scram.pl +++ b/src/test/ssl/t/002_scram.pl @@ -11,11 +11,11 @@ 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'; + plan skip_all => 'SSL not supported by this build'; } # This is the hostname used to connect to the server. @@ -47,7 +47,7 @@ $node->start; # Configure server for SSL connections, with password handling. configure_test_server_for_ssl($node, $SERVERHOSTADDR, $SERVERHOSTCIDR, "scram-sha-256", "pass", "scram-sha-256"); -switch_server_cert($node, 'server-cn-only'); +switch_server_cert($node, certfile => 'server-cn-only'); $ENV{PGPASSWORD} = "pass"; $common_connstr = "dbname=trustdb sslmode=require sslcert=invalid sslrootcert=invalid hostaddr=$SERVERHOSTADDR"; @@ -103,6 +103,6 @@ test_connect_fails( "Cert authentication and channel_binding=require"); # clean up -unlink($client_tmp_key); +SSL::Server::cleanup(); done_testing($number_of_tests); diff --git a/src/test/ssl/t/SSL/Backend/OpenSSL.pm b/src/test/ssl/t/SSL/Backend/OpenSSL.pm new file mode 100644 index 0000000000..0deb867759 --- /dev/null +++ b/src/test/ssl/t/SSL/Backend/OpenSSL.pm @@ -0,0 +1,139 @@ +package SSL::Backend::OpenSSL; + +use strict; +use warnings; +use Exporter; +use File::Basename; +use File::Copy; + +our @ISA = qw(Exporter); +our @EXPORT_OK = qw(get_new_openssl_backend install_certificates); + +our (@keys); + +INIT +{ + @keys = ( + "client", "client-revoked", + "client-der", "client-encrypted-pem", + "client-encrypted-der"); +} + +sub new +{ + my ($class) = @_; + + my $self = { _library => 'OpenSSL' }; + + bless $self, $class; + + return $self; +} + +sub get_new_openssl_backend +{ + my $class = 'SSL::Backend::OpenSSL'; + + my $backend = $class->new(); + + return $backend; +} + +sub init +{ + # 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 ssl/client.key to ssl/client_tmp.key etc for the rest + # of the tests. + foreach my $key (@keys) + { + copy("ssl/${key}.key", "ssl/${key}_tmp.key") + or die + "couldn't copy ssl/${key}.key to ssl/${key}_tmp.key for permissions change: $!"; + chmod 0600, "ssl/${key}_tmp.key" + or die "failed to change permissions on ssl/${key}_tmp.key: $!"; + } + + # Also make a copy of that explicitly world-readable. We can't + # necessarily rely on the file in the source tree having those + # permissions. Add it to @keys to include it in the final clean + # up phase. + copy("ssl/client.key", "ssl/client_wrongperms_tmp.key") + or die + "couldn't copy ssl/client.key to ssl/client_wrongperms_tmp.key: $!"; + chmod 0644, "ssl/client_wrongperms_tmp.key" + or die + "failed to change permissions on ssl/client_wrongperms_tmp.key: $!"; + push @keys, 'client_wrongperms'; +} + +# Change the configuration to use given server cert file, and reload +# the server so that the configuration takes effect. +sub set_server_cert +{ + my $self = $_[0]; + my $params = $_[1]; + + $params->{cafile} = 'root+client_ca' unless defined $params->{cafile}; + $params->{crlfile} = 'root+client.crl' unless defined $params->{crlfile}; + $params->{keyfile} = $params->{certfile} unless defined $params->{keyfile}; + + my $sslconf = + "ssl_ca_file='$params->{cafile}.crt'\n" + . "ssl_cert_file='$params->{certfile}.crt'\n" + . "ssl_key_file='$params->{keyfile}.key'\n" + . "ssl_crl_file='$params->{crlfile}'\n"; + $sslconf .= "ssl_crl_dir='$params->{crldir}'\n" if defined $params->{crldir}; + + return $sslconf; +} + +sub get_library +{ + my ($self) = @_; + + return $self->{_library}; +} + +# Copy a set of files, taking into account wildcards +sub __copy_files +{ + my $orig = shift; + my $dest = shift; + + my @orig_files = glob $orig; + foreach my $orig_file (@orig_files) + { + my $base_file = basename($orig_file); + copy($orig_file, "$dest/$base_file") + or die "Could not copy $orig_file to $dest"; + } + return; +} + +sub install_certificates +{ + my $self = $_[0]; + my $pgdata = $_[1]; + + # Copy all server certificates and keys, and client root cert, to the data dir + __copy_files("ssl/server-*.crt", $pgdata); + __copy_files("ssl/server-*.key", $pgdata); + chmod(0600, glob "$pgdata/server-*.key") or die $!; + __copy_files("ssl/root+client_ca.crt", $pgdata); + __copy_files("ssl/root_ca.crt", $pgdata); + __copy_files("ssl/root+client.crl", $pgdata); + mkdir("$pgdata/root+client-crldir"); + __copy_files("ssl/root+client-crldir/*", "$pgdata/root+client-crldir/"); +} + +sub cleanup +{ + foreach my $key (@keys) + { + unlink("ssl/${key}_tmp.key"); + } +} + +1; diff --git a/src/test/ssl/t/SSLServer.pm b/src/test/ssl/t/SSL/Server.pm similarity index 78% rename from src/test/ssl/t/SSLServer.pm rename to src/test/ssl/t/SSL/Server.pm index 5ec5e0dac8..66f0eaefe3 100644 --- a/src/test/ssl/t/SSLServer.pm +++ b/src/test/ssl/t/SSL/Server.pm @@ -23,15 +23,27 @@ # explicitly because an invalid sslcert or sslrootcert, respectively, # causes those to be ignored.) -package SSLServer; +package SSL::Server; use strict; use warnings; use PostgresNode; +use RecursiveCopy; use TestLib; -use File::Basename; use File::Copy; use Test::More; +use SSL::Backend::OpenSSL qw(get_new_openssl_backend install_certificates); + +our ($openssl, $backend); + +# The TLS backend which the server is using should be mostly transparent for +# the user, apart from individual configuration settings, so keep the backend +# specific things abstracted behind SSL::Server. +if ($ENV{with_ssl} eq 'openssl') +{ + $backend = get_new_openssl_backend(); + $openssl = 1; +} use Exporter 'import'; our @EXPORT = qw( @@ -77,22 +89,6 @@ sub test_connect_fails return; } -# Copy a set of files, taking into account wildcards -sub copy_files -{ - my $orig = shift; - my $dest = shift; - - my @orig_files = glob $orig; - foreach my $orig_file (@orig_files) - { - my $base_file = basename($orig_file); - copy($orig_file, "$dest/$base_file") - or die "Could not copy $orig_file to $dest"; - } - return; -} - # serverhost: what to put in listen_addresses, e.g. '127.0.0.1' # servercidr: what to put in pg_hba.conf, e.g. '127.0.0.1/32' sub configure_test_server_for_ssl @@ -143,15 +139,8 @@ sub configure_test_server_for_ssl open my $sslconf, '>', "$pgdata/sslconfig.conf"; close $sslconf; - # Copy all server certificates and keys, and client root cert, to the data dir - copy_files("ssl/server-*.crt", $pgdata); - copy_files("ssl/server-*.key", $pgdata); - chmod(0600, glob "$pgdata/server-*.key") or die $!; - copy_files("ssl/root+client_ca.crt", $pgdata); - copy_files("ssl/root_ca.crt", $pgdata); - copy_files("ssl/root+client.crl", $pgdata); - mkdir("$pgdata/root+client-crldir"); - copy_files("ssl/root+client-crldir/*", "$pgdata/root+client-crldir/"); + # install certificates and keys + $backend->install_certificates($pgdata); # Stop and restart server to load new listen_addresses. $node->restart; @@ -159,36 +148,41 @@ sub configure_test_server_for_ssl # Change pg_hba after restart because hostssl requires ssl=on configure_hba_for_ssl($node, $servercidr, $authmethod); + # Finally, perform backend specific configuration + $backend->init(); + return; } -# Change the configuration to use given server cert file, and reload -# the server so that the configuration takes effect. +sub ssl_library +{ + return $backend->get_library(); +} + +sub cleanup +{ + $backend->cleanup(); +} + +# Change the configuration to use the given set of certificate, key, ca and +# CRL, and potentially reload the configuration by restarting the server so +# that the configuration takes effect. Restarting is the default, passing +# restart => 'no' opts out of it leaving the server running. sub switch_server_cert { - my $node = $_[0]; - my $certfile = $_[1]; - my $cafile = $_[2] || "root+client_ca"; - my $crlfile = "root+client.crl"; - my $crldir; - my $pgdata = $node->data_dir; - - # defaults to use crl file - if (defined $_[3] || defined $_[4]) - { - $crlfile = $_[3]; - $crldir = $_[4]; - } + my $node = shift; + my %params = @_; + my $pgdata = $node->data_dir; open my $sslconf, '>', "$pgdata/sslconfig.conf"; print $sslconf "ssl=on\n"; - print $sslconf "ssl_ca_file='$cafile.crt'\n"; - print $sslconf "ssl_cert_file='$certfile.crt'\n"; - print $sslconf "ssl_key_file='$certfile.key'\n"; - print $sslconf "ssl_crl_file='$crlfile'\n" if defined $crlfile; - print $sslconf "ssl_crl_dir='$crldir'\n" if defined $crldir; + print $sslconf $backend->set_server_cert(\%params); + print $sslconf "ssl_passphrase_command='" . $params{passphrase_cmd} . "'\n" + if defined $params{passphrase_cmd}; close $sslconf; + return if (defined($params{restart}) && $params{restart} eq 'no'); + $node->restart; return; } -- 2.21.1 (Apple Git-122.3)