Re: pg_stat_ssl additions

At Tue, 29 Jan 2019 13:50:17 +0900, Michael Paquier <michael(at)paquier(dot)xyz> wrote in <20190129045017(dot)GC3121(at)paquier(dot)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(dot)eisentraut(at)2ndquadrant(dot)com> wrote in <24783370-5acd-e0f3-8eb7-7f42ff2a026d(at)2ndquadrant(dot)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