Re: check error messages in SSL tests

[Michael Paquier]

On Tue, Mar 06, 2018 at 02:54:16PM -0500, Peter Eisentraut wrote:
> This contains a slight problem:  The tests contain two different
> branches, depending on whether tls-server-end-point is supported.  But
> these branches run a different number of tests, so the test count of the
> top of the test file might be wrong.  Here is a patch that restructures
> this to count the tests more dynamically.

Not sure how I missed that.  The error is obvious as command_ok triggers
one test and command_fails_like does two of them.  Test::More also
documents what you are using here, so the patch looks fine to me.
--
Michael


signature.asc
Description: PGP signature

Re: check error messages in SSL tests

[Peter Eisentraut]

On 2/24/18 10:12, Peter Eisentraut wrote:
> On 2/24/18 07:37, Michael Paquier wrote:
>> On Fri, Feb 23, 2018 at 01:57:44PM -0500, Peter Eisentraut wrote:
>>> Oh.  I actually had that file as 0600 in my checked-out tree, probably
>>> from earlier experiments.  Fixed that.  And I also changed it to make
>>> another temp file that is explicitly 0644, because we can't rely on that
>>> being the default either.
>>
>> Thanks for fixing the first one.  I also like the second change.  This
>> patch looks fine to me.
> 
> committed

This contains a slight problem:  The tests contain two different
branches, depending on whether tls-server-end-point is supported.  But
these branches run a different number of tests, so the test count of the
top of the test file might be wrong.  Here is a patch that restructures
this to count the tests more dynamically.

Thoughts?

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From c96f5ce5cabae3bedb8d07164f2a7d3bba82f4bc Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Tue, 6 Mar 2018 14:49:07 -0500
Subject: [PATCH] Fix test counting in SSL tests

The branch that does not support tls-server-end-point runs more tests,
so we need to structure the test counting dynamically.
---
 src/test/ssl/t/002_scram.pl | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/test/ssl/t/002_scram.pl b/src/test/ssl/t/002_scram.pl
index 3d7f9abfbe..a805a3196b 100644
--- a/src/test/ssl/t/002_scram.pl
+++ b/src/test/ssl/t/002_scram.pl
@@ -8,15 +8,13 @@
 use ServerSetup;
 use File::Copy;
 
-if ($ENV{with_openssl} eq 'yes')
-{
-   plan tests => 6;
-}
-else
+if ($ENV{with_openssl} ne 'yes')
 {
plan skip_all => 'SSL not supported by this build';
 }
 
+my $number_of_tests = 6;
+
 # This is the hostname used to connect to the server.
 my $SERVERHOSTADDR = '127.0.0.1';
 
@@ -70,8 +68,11 @@

"scram_channel_binding=tls-server-end-point",
qr/channel binding type 
"tls-server-end-point" is not supported by this build/,
"SCRAM authentication with 
tls-server-end-point as channel binding");
+   $number_of_tests++;
 }
 test_connect_fails($common_connstr,
"scram_channel_binding=not-exists",
qr/unsupported SCRAM channel-binding type/,
"SCRAM authentication with invalid channel binding");
+
+done_testing($number_of_tests);

base-commit: 286c0ab257f8dde8e5494426b86c38f3877ae5c2
-- 
2.16.2

Re: check error messages in SSL tests

[Peter Eisentraut]

On 2/24/18 07:37, Michael Paquier wrote:
> On Fri, Feb 23, 2018 at 01:57:44PM -0500, Peter Eisentraut wrote:
>> Oh.  I actually had that file as 0600 in my checked-out tree, probably
>> from earlier experiments.  Fixed that.  And I also changed it to make
>> another temp file that is explicitly 0644, because we can't rely on that
>> being the default either.
> 
> Thanks for fixing the first one.  I also like the second change.  This
> patch looks fine to me.

committed

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

Re: check error messages in SSL tests

[Michael Paquier]

On Fri, Feb 23, 2018 at 01:57:44PM -0500, Peter Eisentraut wrote:
> Oh.  I actually had that file as 0600 in my checked-out tree, probably
> from earlier experiments.  Fixed that.  And I also changed it to make
> another temp file that is explicitly 0644, because we can't rely on that
> being the default either.

Thanks for fixing the first one.  I also like the second change.  This
patch looks fine to me.
--
Michael


signature.asc
Description: PGP signature

Re: check error messages in SSL tests

[Peter Eisentraut]

On 2/22/18 23:58, Michael Paquier wrote:
> One of the tests is failing:
> t/001_ssltests.pl .. 1/62
> #   Failed test 'certificate authorization fails with revoked client cert: 
> matches'
> #   at /home//git/postgres/src/test/ssl/../../../src/test/perl/TestLib.pm 
> line 354.
> #   'psql: private key file "ssl/client-revoked.key" has
> group or world access; permissions should be u=rw (0600) or less
> # '
> # doesn't match '(?^:SSL error)'
> # Looks like you failed 1 test of 62.
> t/001_ssltests.pl .. Dubious, test returned 1 (wstat 256, 0x100)
> Failed 1/62 subtests
> 
> This comes from libpq itself, and the tree uses 0644 on this file.  You
> just need to update this test so as ssl/client-revoked_tmp.key is used
> instead of ssl/client-revoked.key, and then the tests pass.

Oh.  I actually had that file as 0600 in my checked-out tree, probably
from earlier experiments.  Fixed that.  And I also changed it to make
another temp file that is explicitly 0644, because we can't rely on that
being the default either.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From cdcd1a6d4ed835610c7577f21955268ee90cc9a9 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Fri, 23 Feb 2018 13:54:45 -0500
Subject: [PATCH v2] Check error messages in SSL tests

In tests that check whether a connection fails, also check the error
message.  That makes sure that the connection was rejected for the right
reason.

This discovered that two tests had their connection failing for the
wrong reason.  One test failed because pg_hba.conf was not set up to
allow that user, one test failed because the client key file did not
have the right permissions.  Fix those tests and add a new one that is
really supposed to check the file permission issue.
---
 src/test/ssl/ServerSetup.pm| 42 -
 src/test/ssl/ssl/.gitignore|  2 +-
 src/test/ssl/t/001_ssltests.pl | 43 ++
 src/test/ssl/t/002_scram.pl|  4 +++-
 4 files changed, 59 insertions(+), 32 deletions(-)

diff --git a/src/test/ssl/ServerSetup.pm b/src/test/ssl/ServerSetup.pm
index 45991d61a2..27a676b65c 100644
--- a/src/test/ssl/ServerSetup.pm
+++ b/src/test/ssl/ServerSetup.pm
@@ -27,7 +27,6 @@ use Test::More;
 use Exporter 'import';
 our @EXPORT = qw(
   configure_test_server_for_ssl
-  run_test_psql
   switch_server_cert
   test_connect_fails
   test_connect_ok
@@ -35,37 +34,28 @@ our @EXPORT = qw(
 
 # Define a couple of helper functions to test connecting to the server.
 
-# Attempt connection to server with given connection string.
-sub run_test_psql
-{
-   my $connstr   = $_[0];
-
-   my $cmd = [
-   'psql', '-X', '-A', '-t', '-c', "SELECT \$\$connected with 
$connstr\$\$",
-   '-d', "$connstr" ];
-
-   my $result = run_log($cmd);
-   return $result;
-}
-
 # The first argument is a base connection string to use for connection.
 # The second argument is a complementary connection string.
 sub test_connect_ok
 {
-   my $common_connstr = $_[0];
-   my $connstr = $_[1];
-   my $test_name = $_[2];
+   my ($common_connstr, $connstr, $test_name) = @_;
 
-   ok(run_test_psql("$common_connstr $connstr"), $test_name);
+   my $cmd = [
+   'psql', '-X', '-A', '-t', '-c', "SELECT \$\$connected with 
$connstr\$\$",
+   '-d', "$common_connstr $connstr" ];
+
+   command_ok($cmd, $test_name);
 }
 
 sub test_connect_fails
 {
-   my $common_connstr = $_[0];
-   my $connstr = $_[1];
-   my $test_name = $_[2];
+   my ($common_connstr, $connstr, $expected_stderr, $test_name) = @_;
+
+   my $cmd = [
+   'psql', '-X', '-A', '-t', '-c', "SELECT \$\$connected with 
$connstr\$\$",
+   '-d', "$common_connstr $connstr" ];
 
-   ok(!run_test_psql("$common_connstr $connstr"), $test_name);
+   command_fails_like($cmd, $expected_stderr, $test_name);
 }
 
 # Copy a set of files, taking into account wildcards
@@ -169,12 +159,12 @@ sub configure_hba_for_ssl
print $hba
 "# TYPE  DATABASEUSERADDRESS METHOD\n";
print $hba
-"hostssl trustdb ssltestuser $serverhost/32
$authmethod\n";
+"hostssl trustdb all $serverhost/32
$authmethod\n";
print $hba
-"hostssl trustdb ssltestuser ::1/128 
$authmethod\n";
+"hostssl trustdb all ::1/128 
$authmethod\n";
print $hba
-"hostssl certdb  ssltestuser $serverhost/32cert\n";
+"hostssl certdb  all $serverhost/32cert\n";
print $hba
-"hostssl certdb  ssltestuser ::1/128 cert\n";
+"hostssl certdb  all ::1/128 cert\n";
close 

Re: check error messages in SSL tests

[Michael Paquier]

On Thu, Feb 22, 2018 at 08:34:30AM -0500, Peter Eisentraut wrote:
> I noticed that a couple of test cases in the SSL tests fail to connect
> not for the reason that the tests think they should.  Here is a patch to
> augment the test setup so that a test for connection rejection also
> checks that we get the expected error message.

+1 for the idea.  And good catches.

One of the tests is failing:
t/001_ssltests.pl .. 1/62
#   Failed test 'certificate authorization fails with revoked client cert: 
matches'
#   at /home//git/postgres/src/test/ssl/../../../src/test/perl/TestLib.pm 
line 354.
#   'psql: private key file "ssl/client-revoked.key" has
group or world access; permissions should be u=rw (0600) or less
# '
# doesn't match '(?^:SSL error)'
# Looks like you failed 1 test of 62.
t/001_ssltests.pl .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/62 subtests

This comes from libpq itself, and the tree uses 0644 on this file.  You
just need to update this test so as ssl/client-revoked_tmp.key is used
instead of ssl/client-revoked.key, and then the tests pass.
--
Michael


signature.asc
Description: PGP signature