Date: Tue, 29 Jan 2019 13:50:17 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
Cc: peter.eisentraut@2ndquadrant.com, LouPicciano@comcast.net,
	tgl@sss.pgh.pa.us, bruce@momjian.us, pgsql-hackers@postgresql.org
Subject: Re: pg_stat_ssl additions
Message-ID: <20190129045017.GC3121@paquier.xyz>
References: <3f8a7a73-cebe-016f-49e3-853733f8baf2@2ndquadrant.com>
 <20190128.171400.111796002.horiguchi.kyotaro@lab.ntt.co.jp>
 <24783370-5acd-e0f3-8eb7-7f42ff2a026d@2ndquadrant.com>
 <20190129.121829.117745808.horiguchi.kyotaro@lab.ntt.co.jp>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="da4uJneut+ArUgXk"
Content-Disposition: inline
In-Reply-To: <20190129.121829.117745808.horiguchi.kyotaro@lab.ntt.co.jp>
Precedence: bulk


--da4uJneut+ArUgXk
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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@2n=
dquadrant.com> wrote in <24783370-5acd-e0f3-8eb7-7f42ff2a026d@2ndquadrant.c=
om>
>> This is strange.  The tests work for me, and also on the cfbot.  The
>=20
> 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 =3D "user=3Dssltestuser dbname=3Dtrustdb sslcert=3Dinval=
id sslkey=3Dinvalid sslrootcert=3Dinvalid sslcrl=3Dinvalid ";
> +

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.

>  # 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 =3D
> -  "user=3Dssltestuser dbname=3Dtrustdb sslcert=3Dinvalid sslrootcert=3Ds=
sl/root+server_ca.crt hostaddr=3D$SERVERHOSTADDR";
> +  $def_connstr."sslrootcert=3Dssl/root+server_ca.crt hostaddr=3D$SERVERH=
OSTADDR";

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.
--
Michael

--da4uJneut+ArUgXk
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAlxP24kACgkQnvQgOdby
QH3/NA/+N/0cb5dKHAK1ujXhOavnzsObm3uPf2N4G6eZcZvQmILEQNuZOmgE3+l5
bPny3mQrNSF3C9+Q/hAiZpkpnJthcLREp2UvVFqdHDSuuKPNeviUun4BnTPtTyXm
fSG1WP3bdGosUTXnegzdvU/Ncw6wU3stza2LgWBPP2EGb+FU6Hi7uYG+XMBjT+SO
94VFij1WtMMnioE3gDXNb8v7Jmb7PDBwSUBEOpZfGSP7ZOsDH18BuR9oM9hiEhIp
nimzmczHHs9zn9AeeTqDh5F6qbhMp3wQyEo3xpcrv2CZUnLyWFa2aWIkotUAmueH
9cjw+JsCem6OoEKcozG6TFp1RjGJYe8TZXRWR2mGJvnP93JeZklDiyDjLrngtK0A
6073yRnJ0W1pw4nxFlmI+nnD+pqHhhDr1bTJuuBnyfd3r9ly8aOQCfujr2oD54LP
H7L7+4G9E1NkAbC3zycTw5V3JlT+pSYOWZxp4BdhgQJez4GRs+IVqCuTa3TRe8Wk
Knd7iPfHCRt1j19z7esEnkfJaDhM0K+idXyb3XWVie3MA0fBsZHK/4x0pbEsFOw4
0j0EljI+D2syAylwd5td7kw8l4rWuqrYq1Oh6OK0cpRqkYHZrK9nHLrSkJWR5OXo
5rnzptfV1UwKjDq1JAkCH3pRWSJuBsAWz0WdaGwaPW9kcuSS38U=
=qxD4
-----END PGP SIGNATURE-----

--da4uJneut+ArUgXk--