Re: pg_stat_ssl additions - Mailing list pgsql-hackers

At Tue, 29 Jan 2019 13:50:17 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20190129045017.GC3121@paquier.xyz>
> On Tue, Jan 29, 2019 at 12:18:29PM +0900, Kyotaro HORIGUCHI wrote:
> > At Mon, 28 Jan 2019 14:53:43 +0100, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote in
<24783370-5acd-e0f3-8eb7-7f42ff2a026d@2ndquadrant.com>
> >> This is strange.  The tests work for me, and also on the cfbot.  The
> > 
> > Agreed. It seemed so also to me.
> 
> The tests look sane to me for what it's worth.
> 
> > When initializing SSL context, it picks up the root certificate
> > from my home directory, not in test installation and I had one
> > there. It is not based on $HOME but pwent so it is unchangeable
> > (and it is the right design for the purpose).
> 
> Oops.  I agree that the tests ought to be as much isolated as
> possible, without optionally fetching things depending on the user
> environment.
> 
> > +# ssl-related properties may defautly set to the files in the users'
> > +# environment. Explicitly provide them a value so that they don't
> > +# point a valid file accidentially. Some other common properties are
> > +# set here together.
> > +# Attach this at the head of $common_connstr.
> > +my $def_connstr = "user=ssltestuser dbname=trustdb sslcert=invalid sslkey=invalid sslrootcert=invalid
sslcrl=invalid";
 
> > +
> 
> Maybe it is better to just put that in test_connect_ok and
> test_connect_fails for only the SSL parameters?  I don't see much
> point in enforcing dbname and user.

Hmm. It is the first thing I did for the issue. Anyway..

Of course dbname and user are not necessary to fix it. Ok, I
attached two patches. The first applies on the current master and
prevent psql from loading ssl-related files from out of testing
environment. The second applies on the 0002 patch and prevent the
patch from loading a wrong sslrootcert.

> >  # The server should not accept non-SSL connections.
> >  test_connect_fails(
> > @@ -185,7 +192,7 @@ test_connect_ok(
> >  # Check that connecting with verify-full fails, when the hostname doesn't
> >  # match the hostname in the server's certificate.
> >  $common_connstr =
> > -  "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR";
> > +  $def_connstr."sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR";
> 
> I think this is bad, inconsistent style.  Adding the variable within
> the quoted section is fine, as much as moving SERVERHOSTADDR out.  But
> mixing styles is not.

I Understood. Thanks for the suggestion.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From a3cb52ba165074db1d2910faa8e58030d84fa7c9 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Tue, 29 Jan 2019 21:06:58 +0900
Subject: [PATCH 1/4] Prevent from loading ssl files out of testing
 environment.

SSL test script can let psql load SSL-related files out of the testing
environemnt. Fix it by explicitly setting sslcert, sslrootcert and
sslcrl when they are not used. sslkey doesn't need the fix because it
is loaded only when sslcert is loadded.
---
 src/test/ssl/t/001_ssltests.pl | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index 2b875a3c95..114541590d 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -93,7 +93,7 @@ note "running client tests";
 switch_server_cert($node, 'server-cn-only');
 
 $common_connstr =
-  "user=ssltestuser dbname=trustdb sslcert=invalid hostaddr=$SERVERHOSTADDR host=common-name.pg-ssltest.test";
+  "user=ssltestuser dbname=trustdb sslcert=invalid sslcrl=invalid hostaddr=$SERVERHOSTADDR
host=common-name.pg-ssltest.test";
 
 # The server should not accept non-SSL connections.
 test_connect_fails(
@@ -185,7 +185,7 @@ test_connect_ok(
 # Check that connecting with verify-full fails, when the hostname doesn't
 # match the hostname in the server's certificate.
 $common_connstr =
-  "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR";
+  "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt sslcrl=invalid
hostaddr=$SERVERHOSTADDR";
 
 test_connect_ok(
     $common_connstr,
@@ -205,7 +205,7 @@ test_connect_fails(
 switch_server_cert($node, 'server-multiple-alt-names');
 
 $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 sslcrl=invalid
hostaddr=$SERVERHOSTADDRsslmode=verify-full";
 
 
 test_connect_ok(
     $common_connstr,
@@ -236,7 +236,7 @@ test_connect_fails(
 switch_server_cert($node, 'server-single-alt-name');
 
 $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 sslcrl=invalid
hostaddr=$SERVERHOSTADDRsslmode=verify-full";
 
 
 test_connect_ok(
     $common_connstr,
@@ -260,7 +260,7 @@ test_connect_fails(
 switch_server_cert($node, 'server-cn-and-alt-names');
 
 $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 sslcrl=invalid
hostaddr=$SERVERHOSTADDRsslmode=verify-full";
 
 
 test_connect_ok(
     $common_connstr,
@@ -280,7 +280,7 @@ test_connect_fails(
 # not a very sensible certificate, but libpq should handle it gracefully.
 switch_server_cert($node, 'server-no-names');
 $common_connstr =
-  "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt hostaddr=$SERVERHOSTADDR";
+  "user=ssltestuser dbname=trustdb sslcert=invalid sslrootcert=ssl/root+server_ca.crt sslcrl=invalid
hostaddr=$SERVERHOSTADDR";
 
 test_connect_ok(
     $common_connstr,
@@ -301,7 +301,7 @@ $common_connstr =
 # 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 sslcrl=invalid",
     "connects without client-side CRL");
 test_connect_fails(
     $common_connstr,
@@ -316,7 +316,7 @@ test_connect_fails(
 note "running server tests";
 
 $common_connstr =
-  "sslrootcert=ssl/root+server_ca.crt sslmode=require dbname=certdb hostaddr=$SERVERHOSTADDR";
+  "sslrootcert=ssl/root+server_ca.crt sslcrl=invalid sslmode=require dbname=certdb hostaddr=$SERVERHOSTADDR";
 
 # no client cert
 test_connect_fails(
@@ -356,7 +356,7 @@ test_connect_fails(
 # 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');
 $common_connstr =
-  "user=ssltestuser dbname=certdb sslkey=ssl/client_tmp.key sslrootcert=ssl/root+server_ca.crt
hostaddr=$SERVERHOSTADDR";
+  "user=ssltestuser dbname=certdb sslkey=ssl/client_tmp.key sslrootcert=ssl/root+server_ca.crt sslcrl=invalid
hostaddr=$SERVERHOSTADDR";
 
 test_connect_ok(
     $common_connstr,
-- 
2.16.3

From 47ab02f905fa43479c8e44e89b857b311e175363 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Tue, 29 Jan 2019 21:00:29 +0900
Subject: [PATCH 4/4] Don't load wrong sslrootcert

Fixes the pg_stat_ssl patch so that 0002 don't load sslrootcert out of
testing environment.
---
 src/test/ssl/t/001_ssltests.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index dea25073b5..7bf9834964 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -312,7 +312,7 @@ test_connect_fails(
 # pg_stat_ssl
 command_like([
     'psql', '-X', '-A', '-F', ',', '-P', 'null=_null_',
-    '-d', $common_connstr,
+    '-d', "$common_connstr sslrootcert=invalid",
     '-c', "SELECT * FROM pg_stat_ssl WHERE pid = pg_backend_pid()"
              ],
              qr{^pid,ssl,version,cipher,bits,compression,clientdn\n
-- 
2.16.3