SSL test names

Started by Peter Eisentrautalmost 8 years ago4 messages
#1Peter Eisentraut
peter.eisentraut@2ndquadrant.com
1 attachment(s)

Here is a patch that gives the tests in the SSL test suite proper names
instead of just writing out the connection strings. So instead of

# running client tests
# test that the server doesn't accept non-SSL connections
ok 1 - sslmode=disable (should fail)
# connect without server root cert
ok 2 - sslrootcert=invalid sslmode=require
ok 3 - sslrootcert=invalid sslmode=verify-ca (should fail)
ok 4 - sslrootcert=invalid sslmode=verify-full (should fail)

you get something like

# running client tests
ok 1 - server doesn't accept non-SSL connections
ok 2 - connect without server root cert sslmode=require
ok 3 - connect without server root cert sslmode=verify-ca
ok 4 - connect without server root cert sslmode=verify-full
ok 5 - connect with wrong server root cert sslmode=require
ok 6 - connect with wrong server root cert sslmode=verify-ca
ok 7 - connect with wrong server root cert sslmode=verify-full

I have found the old way very confusing while working with several
SSL-related patches recently.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachments:

0001-Refine-SSL-tests-test-name-reporting.patchtext/plain; charset=UTF-8; name=0001-Refine-SSL-tests-test-name-reporting.patch; x-mac-creator=0; x-mac-type=0Download
From 30014a0669f9a357a7013e2ee235075196acd497 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Fri, 26 Jan 2018 16:14:34 -0500
Subject: [PATCH] Refine SSL tests test name reporting

Instead of using the psql/libpq connection string as the displayed test
name and relying on "notes" and source code comments to explain the
tests, give the tests self-explanatory names, like we do elsewhere.
---
 src/test/ssl/ServerSetup.pm    |  11 +---
 src/test/ssl/t/001_ssltests.pl | 142 +++++++++++++++++++++++++----------------
 2 files changed, 88 insertions(+), 65 deletions(-)

diff --git a/src/test/ssl/ServerSetup.pm b/src/test/ssl/ServerSetup.pm
index b4d5746e20..c0f21290af 100644
--- a/src/test/ssl/ServerSetup.pm
+++ b/src/test/ssl/ServerSetup.pm
@@ -39,7 +39,6 @@ our @EXPORT = qw(
 sub run_test_psql
 {
 	my $connstr   = $_[0];
-	my $logstring = $_[1];
 
 	my $cmd = [
 		'psql', '-X', '-A', '-t', '-c', "SELECT \$\$connected with $connstr\$\$",
@@ -59,9 +58,7 @@ sub test_connect_ok
 	my $connstr = $_[1];
 	my $test_name = $_[2];
 
-	my $result =
-	  run_test_psql("$common_connstr $connstr", "(should succeed)");
-	ok($result, $test_name || $connstr);
+	ok(run_test_psql("$common_connstr $connstr"), $test_name);
 }
 
 sub test_connect_fails
@@ -70,8 +67,7 @@ sub test_connect_fails
 	my $connstr = $_[1];
 	my $test_name = $_[2];
 
-	my $result = run_test_psql("$common_connstr $connstr", "(should fail)");
-	ok(!$result, $test_name || "$connstr (should fail)");
+	ok(!run_test_psql("$common_connstr $connstr"), $test_name);
 }
 
 # Copy a set of files, taking into account wildcards
@@ -151,9 +147,6 @@ sub switch_server_cert
 	my $cafile   = $_[2] || "root+client_ca";
 	my $pgdata   = $node->data_dir;
 
-	note
-	  "reloading server with certfile \"$certfile\" and cafile \"$cafile\"";
-
 	open my $sslconf, '>', "$pgdata/sslconfig.conf";
 	print $sslconf "ssl=on\n";
 	print $sslconf "ssl_ca_file='$cafile.crt'\n";
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index 28837a1391..e53bd12ae9 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -47,113 +47,134 @@
 "user=ssltestuser dbname=trustdb sslcert=invalid hostaddr=$SERVERHOSTADDR host=common-name.pg-ssltest.test";
 
 # The server should not accept non-SSL connections.
-note "test that the server doesn't accept non-SSL connections";
-test_connect_fails($common_connstr, "sslmode=disable");
+test_connect_fails($common_connstr, "sslmode=disable",
+				   "server doesn't accept non-SSL connections");
 
 # Try without a root cert. In sslmode=require, this should work. In verify-ca
 # or verify-full mode it should fail.
-note "connect without server root cert";
-test_connect_ok($common_connstr, "sslrootcert=invalid sslmode=require");
-test_connect_fails($common_connstr, "sslrootcert=invalid sslmode=verify-ca");
-test_connect_fails($common_connstr, "sslrootcert=invalid sslmode=verify-full");
+test_connect_ok($common_connstr, "sslrootcert=invalid sslmode=require",
+				"connect without server root cert sslmode=require");
+test_connect_fails($common_connstr, "sslrootcert=invalid sslmode=verify-ca",
+				"connect without server root cert sslmode=verify-ca");
+test_connect_fails($common_connstr, "sslrootcert=invalid sslmode=verify-full",
+				   "connect without server root cert sslmode=verify-full");
 
 # Try with wrong root cert, should fail. (We're using the client CA as the
 # root, but the server's key is signed by the server CA.)
-note "connect with wrong server root cert";
 test_connect_fails($common_connstr,
-	"sslrootcert=ssl/client_ca.crt sslmode=require");
+				   "sslrootcert=ssl/client_ca.crt sslmode=require",
+				   "connect with wrong server root cert sslmode=require");
 test_connect_fails($common_connstr,
-	"sslrootcert=ssl/client_ca.crt sslmode=verify-ca");
+				   "sslrootcert=ssl/client_ca.crt sslmode=verify-ca",
+				   "connect with wrong server root cert sslmode=verify-ca");
 test_connect_fails($common_connstr,
-	"sslrootcert=ssl/client_ca.crt sslmode=verify-full");
+				   "sslrootcert=ssl/client_ca.crt sslmode=verify-full",
+				   "connect with wrong server root cert sslmode=verify-full");
 
 # Try with just the server CA's cert. This fails because the root file
 # must contain the whole chain up to the root CA.
-note "connect with server CA cert, without root CA";
 test_connect_fails($common_connstr,
-	"sslrootcert=ssl/server_ca.crt sslmode=verify-ca");
+				   "sslrootcert=ssl/server_ca.crt sslmode=verify-ca",
+				   "connect with server CA cert, without root CA");
 
 # And finally, with the correct root cert.
-note "connect with correct server CA cert file";
 test_connect_ok($common_connstr,
-	"sslrootcert=ssl/root+server_ca.crt sslmode=require");
+				"sslrootcert=ssl/root+server_ca.crt sslmode=require",
+				"connect with correct server CA cert file sslmode=require");
 test_connect_ok($common_connstr,
-	"sslrootcert=ssl/root+server_ca.crt sslmode=verify-ca");
+				"sslrootcert=ssl/root+server_ca.crt sslmode=verify-ca",
+				"connect with correct server CA cert file sslmode=verify-ca");
 test_connect_ok($common_connstr,
-	"sslrootcert=ssl/root+server_ca.crt sslmode=verify-full");
+				"sslrootcert=ssl/root+server_ca.crt sslmode=verify-full",
+				"connect with correct server CA cert file sslmode=verify-full");
 
 # Test with cert root file that contains two certificates. The client should
 # be able to pick the right one, regardless of the order in the file.
 test_connect_ok($common_connstr,
-	"sslrootcert=ssl/both-cas-1.crt sslmode=verify-ca");
+				"sslrootcert=ssl/both-cas-1.crt sslmode=verify-ca",
+				"cert root file that contains two certificates, order 1");
 test_connect_ok($common_connstr,
-	"sslrootcert=ssl/both-cas-2.crt sslmode=verify-ca");
+				"sslrootcert=ssl/both-cas-2.crt sslmode=verify-ca",
+				"cert root file that contains two certificates, order 2");
 
-note "testing sslcrl option with a non-revoked cert";
+# CRL tests
 
 # Invalid CRL filename is the same as no CRL, succeeds
 test_connect_ok($common_connstr,
-	"sslrootcert=ssl/root+server_ca.crt sslmode=verify-ca sslcrl=invalid");
+				"sslrootcert=ssl/root+server_ca.crt sslmode=verify-ca sslcrl=invalid",
+				"sslcrl option with invalid file name");
 
 # A CRL belonging to a different CA is not accepted, fails
 test_connect_fails($common_connstr,
-"sslrootcert=ssl/root+server_ca.crt sslmode=verify-ca sslcrl=ssl/client.crl");
+				   "sslrootcert=ssl/root+server_ca.crt sslmode=verify-ca sslcrl=ssl/client.crl",
+				   "CRL belonging to a different CA");
 
 # With the correct CRL, succeeds (this cert is not revoked)
 test_connect_ok($common_connstr,
-"sslrootcert=ssl/root+server_ca.crt sslmode=verify-ca sslcrl=ssl/root+server.crl"
-);
+				"sslrootcert=ssl/root+server_ca.crt sslmode=verify-ca sslcrl=ssl/root+server.crl",
+				"CRL with a non-revoked cert");
 
 # Check that connecting with verify-full fails, when the hostname doesn't
 # match the hostname in the server's certificate.
-note "test mismatch between hostname and server certificate";
 $common_connstr =
-"user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR sslmode=verify-full";
+"user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR";
+
+test_connect_ok($common_connstr, "sslmode=require host=wronghost.test",
+				"mismatch between host name and server certificate sslmode=require");
+test_connect_ok($common_connstr, "sslmode=verify-ca host=wronghost.test",
+				"mismatch between host name and server certificate sslmode=verify-ca");
+test_connect_fails($common_connstr, "sslmode=verify-full host=wronghost.test",
+				   "mismatch between host name and server certificate sslmode=verify-full");
 
-test_connect_ok($common_connstr, "sslmode=require host=wronghost.test");
-test_connect_ok($common_connstr, "sslmode=verify-ca host=wronghost.test");
-test_connect_fails($common_connstr, "sslmode=verify-full host=wronghost.test");
 
 # Test Subject Alternative Names.
 switch_server_cert($node, 'server-multiple-alt-names');
 
-note "test hostname matching with X.509 Subject Alternative Names";
 $common_connstr =
 "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR sslmode=verify-full";
 
-test_connect_ok($common_connstr, "host=dns1.alt-name.pg-ssltest.test");
-test_connect_ok($common_connstr, "host=dns2.alt-name.pg-ssltest.test");
-test_connect_ok($common_connstr, "host=foo.wildcard.pg-ssltest.test");
+test_connect_ok($common_connstr, "host=dns1.alt-name.pg-ssltest.test",
+				"host name matching with X.509 Subject Alternative Names 1");
+test_connect_ok($common_connstr, "host=dns2.alt-name.pg-ssltest.test",
+				"host name matching with X.509 Subject Alternative Names 2");
+test_connect_ok($common_connstr, "host=foo.wildcard.pg-ssltest.test",
+				"host name matching with X.509 Subject Alternative Names wildcard");
 
-test_connect_fails($common_connstr, "host=wronghost.alt-name.pg-ssltest.test");
+test_connect_fails($common_connstr, "host=wronghost.alt-name.pg-ssltest.test",
+				   "host name not matching with X.509 Subject Alternative Names");
 test_connect_fails($common_connstr,
-	"host=deep.subdomain.wildcard.pg-ssltest.test");
+				   "host=deep.subdomain.wildcard.pg-ssltest.test",
+				   "host name not matching with X.509 Subject Alternative Names wildcard");
 
 # 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');
 
-note "test hostname matching with a single X.509 Subject Alternative Name";
 $common_connstr =
 "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR sslmode=verify-full";
 
-test_connect_ok($common_connstr, "host=single.alt-name.pg-ssltest.test");
+test_connect_ok($common_connstr, "host=single.alt-name.pg-ssltest.test",
+				"host name matching with a single X.509 Subject Alternative Name");
 
-test_connect_fails($common_connstr, "host=wronghost.alt-name.pg-ssltest.test");
+test_connect_fails($common_connstr, "host=wronghost.alt-name.pg-ssltest.test",
+				   "host name not matching with a single X.509 Subject Alternative Name");
 test_connect_fails($common_connstr,
-	"host=deep.subdomain.wildcard.pg-ssltest.test");
+				   "host=deep.subdomain.wildcard.pg-ssltest.test",
+				   "host name not matching with a single X.509 Subject Alternative Name wildcard");
 
 # 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');
 
-note "test certificate with both a CN and SANs";
 $common_connstr =
 "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR sslmode=verify-full";
 
-test_connect_ok($common_connstr, "host=dns1.alt-name.pg-ssltest.test");
-test_connect_ok($common_connstr, "host=dns2.alt-name.pg-ssltest.test");
-test_connect_fails($common_connstr, "host=common-name.pg-ssltest.test");
+test_connect_ok($common_connstr, "host=dns1.alt-name.pg-ssltest.test",
+				"certificate with both a CN and SANs 1");
+test_connect_ok($common_connstr, "host=dns2.alt-name.pg-ssltest.test",
+				"certificate with both a CN and SANs 2");
+test_connect_fails($common_connstr, "host=common-name.pg-ssltest.test",
+				   "certificate with both a CN and SANs ignores CN");
 
 # 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.
@@ -162,12 +183,13 @@
 "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR";
 
 test_connect_ok($common_connstr,
-	"sslmode=verify-ca host=common-name.pg-ssltest.test");
+				"sslmode=verify-ca host=common-name.pg-ssltest.test",
+				"server certificate without CN or SANs sslmode=verify-ca");
 test_connect_fails($common_connstr,
-	"sslmode=verify-full host=common-name.pg-ssltest.test");
+				   "sslmode=verify-full host=common-name.pg-ssltest.test",
+				   "server certificate without CN or SANs sslmode=verify-full");
 
 # Test that the CRL works
-note "testing client-side CRL";
 switch_server_cert($node, 'server-revoked');
 
 $common_connstr =
@@ -175,34 +197,40 @@
 
 # Without the CRL, succeeds. With it, fails.
 test_connect_ok($common_connstr,
-	"sslrootcert=ssl/root+server_ca.crt sslmode=verify-ca");
+				"sslrootcert=ssl/root+server_ca.crt sslmode=verify-ca",
+				"connects without client-side CRL");
 test_connect_fails($common_connstr,
-"sslrootcert=ssl/root+server_ca.crt sslmode=verify-ca sslcrl=ssl/root+server.crl"
-);
+				   "sslrootcert=ssl/root+server_ca.crt sslmode=verify-ca sslcrl=ssl/root+server.crl",
+				   "does not connect with client-side CRL");
 
 ### Part 2. Server-side tests.
 ###
 ### Test certificate authorization.
 
-note "testing certificate authorization";
+note "running server tests";
+
 $common_connstr =
 "sslrootcert=ssl/root+server_ca.crt sslmode=require dbname=certdb hostaddr=$SERVERHOSTADDR";
 
 # no client cert
-test_connect_fails($common_connstr, "user=ssltestuser sslcert=invalid");
+test_connect_fails($common_connstr,
+				   "user=ssltestuser sslcert=invalid",
+				   "certificate authorization fails without client cert");
 
 # correct client cert
 test_connect_ok($common_connstr,
-	"user=ssltestuser sslcert=ssl/client.crt sslkey=ssl/client_tmp.key");
+				"user=ssltestuser sslcert=ssl/client.crt sslkey=ssl/client_tmp.key",
+				"certificate authorization succeeds with correct client cert");
 
 # client cert belonging to another user
 test_connect_fails($common_connstr,
-	"user=anotheruser sslcert=ssl/client.crt sslkey=ssl/client_tmp.key");
+				   "user=anotheruser sslcert=ssl/client.crt sslkey=ssl/client_tmp.key",
+				   "certificate authorization fails with client cert belonging to another user");
 
 # revoked client cert
 test_connect_fails($common_connstr,
-"user=ssltestuser sslcert=ssl/client-revoked.crt sslkey=ssl/client-revoked.key"
-);
+				   "user=ssltestuser sslcert=ssl/client-revoked.crt sslkey=ssl/client-revoked.key",
+				   "certificate authorization fails with revoked client cert");
 
 # 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');
@@ -210,8 +238,10 @@
 "user=ssltestuser dbname=certdb sslkey=ssl/client_tmp.key sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR";
 
 test_connect_ok($common_connstr,
-	"sslmode=require sslcert=ssl/client+client_ca.crt");
-test_connect_fails($common_connstr, "sslmode=require sslcert=ssl/client.crt");
+				"sslmode=require sslcert=ssl/client+client_ca.crt",
+				"intermediate client certificate is provided by client");
+test_connect_fails($common_connstr, "sslmode=require sslcert=ssl/client.crt",
+				   "intermediate client certificate is missing");
 
 # clean up
 unlink "ssl/client_tmp.key";

base-commit: 32ff2691173559e5f0ca3ea9cd5db134af6ee37d
-- 
2.16.1

#2Michael Paquier
michael.paquier@gmail.com
In reply to: Peter Eisentraut (#1)
Re: SSL test names

On Wed, Feb 07, 2018 at 11:54:52AM -0500, Peter Eisentraut wrote:

Here is a patch that gives the tests in the SSL test suite proper names
instead of just writing out the connection strings. So instead of

# running client tests
# test that the server doesn't accept non-SSL connections
ok 1 - sslmode=disable (should fail)
# connect without server root cert
ok 2 - sslrootcert=invalid sslmode=require
ok 3 - sslrootcert=invalid sslmode=verify-ca (should fail)
ok 4 - sslrootcert=invalid sslmode=verify-full (should fail)

you get something like

# running client tests
ok 1 - server doesn't accept non-SSL connections
ok 2 - connect without server root cert sslmode=require
ok 3 - connect without server root cert sslmode=verify-ca
ok 4 - connect without server root cert sslmode=verify-full
ok 5 - connect with wrong server root cert sslmode=require
ok 6 - connect with wrong server root cert sslmode=verify-ca
ok 7 - connect with wrong server root cert sslmode=verify-full

I have found the old way very confusing while working with several
SSL-related patches recently.

No objections against that.

You need to update the comment on top of test_connect_ok in
ServerSetup.pm. Wouldn't it be better to use the expected result
as an argument and merge test_connect_ok and test_connect_fails?
--
Michael

#3Daniel Gustafsson
daniel@yesql.se
In reply to: Peter Eisentraut (#1)
Re: SSL test names

On 07 Feb 2018, at 17:54, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:

I have found the old way very confusing while working with several
SSL-related patches recently.

Agreed. I had similar, but way uglier, hacks in my Secure Transport branch.
+1 on something like this.

cheers ./daniel

#4Peter Eisentraut
peter.eisentraut@2ndquadrant.com
In reply to: Michael Paquier (#2)
Re: SSL test names

On 2/7/18 23:18, Michael Paquier wrote:

You need to update the comment on top of test_connect_ok in
ServerSetup.pm.

done and committed

Wouldn't it be better to use the expected result
as an argument and merge test_connect_ok and test_connect_fails?

That doesn't seem to be the general style, and I think it's more
readable the way it is now.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services