Date: Fri, 18 Mar 2022 10:02:52 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Daniel Gustafsson <daniel@yesql.se>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: Out-of-tree certificate interferes ssltest
Message-ID: <YjPaPPdliVMAz3hC@paquier.xyz>
References: <99384E03-E59D-4CA3-8D5E-E3F90B11D335@yesql.se>
 <YjLOPibZ/ooGlZw6@paquier.xyz>
 <YjLhpog7Q0kRQq1K@paquier.xyz>
 <20220317.170510.1335689533199004810.horikyota.ntt@gmail.com>
 <C93D260A-AE9D-46C9-93AD-A31F221A2D4E@yesql.se>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="YMb4bSFe/eqRqJNM"
Content-Disposition: inline
In-Reply-To: <C93D260A-AE9D-46C9-93AD-A31F221A2D4E@yesql.se>
Archived-At: <https://www.postgresql.org/message-id/YjPaPPdliVMAz3hC%40paquier.xyz>
Precedence: bulk


--YMb4bSFe/eqRqJNM
Content-Type: multipart/mixed; boundary="2G7AuSfZ2crK4YqV"
Content-Disposition: inline


--2G7AuSfZ2crK4YqV
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Thu, Mar 17, 2022 at 02:28:49PM +0100, Daniel Gustafsson wrote:
> One small concern though. This hunk:
>=20
> +my $default_ssl_connstr =3D "sslkey=3Dinvalid sslcert=3Dinvalid sslrootc=
ert=3Dinvalid sslcrl=3Dinvalid sslcrldir=3Dinvalid";
> +
>  $common_connstr =3D
> -  "user=3Dssltestuser dbname=3Dtrustdb sslcert=3Dinvalid hostaddr=3D$SER=
VERHOSTADDR host=3Dcommon-name.pg-ssltest.test";
> +  "$default_ssl_connstr user=3Dssltestuser dbname=3Dtrustdb hostaddr=3D$=
SERVERHOSTADDR host=3Dcommon-name.pg-ssltest.test";
> =20
> ..together with the following changes along the lines of:
>=20
> -	"$common_connstr sslrootcert=3Dinvalid sslmode=3Drequire",
> +	"$common_connstr sslmode=3Drequire",
>=20
> ..is making it fairly hard to read the test and visualize what the connec=
tion
> string is and how the test should behave.  I don't have a better idea off=
 the
> top of my head right now, but I think this is an area to revisit and impr=
ove
> on.

I agree that this makes this set of three tests harder to follow, as
we expect a root cert to *not* be set locally.  Keeping the behavior
documented in each individual string would be better, even if that
duplicates more the keys in those final strings.

Another thing that Horiguchi-san has pointed out upthread (?) is 003,
where it is also possible to trigger failures once the environment is
hijacked.  The attached allows the full test suite to pass without
issues on my side.
--
Michael

--2G7AuSfZ2crK4YqV
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="ssltest-tap-3.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index 5c5b16fbe7..605e405de3 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -138,8 +138,13 @@ note "running client tests";
=20
 switch_server_cert($node, 'server-cn-only');
=20
+# Set of default settings for SSL parameters in connection string.  This
+# makes the tests protected against any defaults the environment may have
+# in ~/.postgresql/.
+my $default_ssl_connstr =3D "sslkey=3Dinvalid sslcert=3Dinvalid sslrootcer=
t=3Dinvalid sslcrl=3Dinvalid sslcrldir=3Dinvalid";
+
 $common_connstr =3D
-  "user=3Dssltestuser dbname=3Dtrustdb sslcert=3Dinvalid hostaddr=3D$SERVE=
RHOSTADDR host=3Dcommon-name.pg-ssltest.test";
+  "$default_ssl_connstr user=3Dssltestuser dbname=3Dtrustdb hostaddr=3D$SE=
RVERHOSTADDR host=3Dcommon-name.pg-ssltest.test";
=20
 # The server should not accept non-SSL connections.
 $node->connect_fails(
@@ -216,9 +221,10 @@ $node->connect_fails(
 	"CRL belonging to a different CA",
 	expected_stderr =3D> qr/SSL error: certificate verify failed/);
=20
-# The same for CRL directory
+# The same for CRL directory.  sslcrl=3D'' is added here to override the
+# invalid default, so as this does not interfere with this case.
 $node->connect_fails(
-	"$common_connstr sslrootcert=3Dssl/root+server_ca.crt sslmode=3Dverify-ca=
 sslcrldir=3Dssl/client-crldir",
+	"$common_connstr sslcrl=3D'' sslrootcert=3Dssl/root+server_ca.crt sslmode=
=3Dverify-ca sslcrldir=3Dssl/client-crldir",
 	"directory CRL belonging to a different CA",
 	expected_stderr =3D> qr/SSL error: certificate verify failed/);
=20
@@ -235,7 +241,7 @@ $node->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=3Dssl=
/root+server_ca.crt hostaddr=3D$SERVERHOSTADDR";
+  "$default_ssl_connstr user=3Dssltestuser dbname=3Dtrustdb sslrootcert=3D=
ssl/root+server_ca.crt hostaddr=3D$SERVERHOSTADDR";
=20
 $node->connect_ok("$common_connstr sslmode=3Drequire host=3Dwronghost.test=
",
 	"mismatch between host name and server certificate sslmode=3Drequire");
@@ -253,7 +259,7 @@ $node->connect_fails(
 switch_server_cert($node, 'server-multiple-alt-names');
=20
 $common_connstr =3D
-  "user=3Dssltestuser dbname=3Dtrustdb sslcert=3Dinvalid sslrootcert=3Dssl=
/root+server_ca.crt hostaddr=3D$SERVERHOSTADDR sslmode=3Dverify-full";
+  "$default_ssl_connstr user=3Dssltestuser dbname=3Dtrustdb sslrootcert=3D=
ssl/root+server_ca.crt hostaddr=3D$SERVERHOSTADDR sslmode=3Dverify-full";
=20
 $node->connect_ok(
 	"$common_connstr host=3Ddns1.alt-name.pg-ssltest.test",
@@ -282,7 +288,7 @@ $node->connect_fails(
 switch_server_cert($node, 'server-single-alt-name');
=20
 $common_connstr =3D
-  "user=3Dssltestuser dbname=3Dtrustdb sslcert=3Dinvalid sslrootcert=3Dssl=
/root+server_ca.crt hostaddr=3D$SERVERHOSTADDR sslmode=3Dverify-full";
+  "$default_ssl_connstr user=3Dssltestuser dbname=3Dtrustdb sslrootcert=3D=
ssl/root+server_ca.crt hostaddr=3D$SERVERHOSTADDR sslmode=3Dverify-full";
=20
 $node->connect_ok(
 	"$common_connstr host=3Dsingle.alt-name.pg-ssltest.test",
@@ -306,7 +312,7 @@ $node->connect_fails(
 switch_server_cert($node, 'server-cn-and-alt-names');
=20
 $common_connstr =3D
-  "user=3Dssltestuser dbname=3Dtrustdb sslcert=3Dinvalid sslrootcert=3Dssl=
/root+server_ca.crt hostaddr=3D$SERVERHOSTADDR sslmode=3Dverify-full";
+  "$default_ssl_connstr user=3Dssltestuser dbname=3Dtrustdb sslrootcert=3D=
ssl/root+server_ca.crt hostaddr=3D$SERVERHOSTADDR sslmode=3Dverify-full";
=20
 $node->connect_ok("$common_connstr host=3Ddns1.alt-name.pg-ssltest.test",
 	"certificate with both a CN and SANs 1");
@@ -323,7 +329,7 @@ $node->connect_fails(
 # not a very sensible certificate, but libpq should handle it gracefully.
 switch_server_cert($node, 'server-no-names');
 $common_connstr =3D
-  "user=3Dssltestuser dbname=3Dtrustdb sslcert=3Dinvalid sslrootcert=3Dssl=
/root+server_ca.crt hostaddr=3D$SERVERHOSTADDR";
+  "$default_ssl_connstr user=3Dssltestuser dbname=3Dtrustdb sslrootcert=3D=
ssl/root+server_ca.crt hostaddr=3D$SERVERHOSTADDR";
=20
 $node->connect_ok(
 	"$common_connstr sslmode=3Dverify-ca host=3Dcommon-name.pg-ssltest.test",
@@ -339,7 +345,7 @@ $node->connect_fails(
 switch_server_cert($node, 'server-revoked');
=20
 $common_connstr =3D
-  "user=3Dssltestuser dbname=3Dtrustdb sslcert=3Dinvalid hostaddr=3D$SERVE=
RHOSTADDR host=3Dcommon-name.pg-ssltest.test";
+  "$default_ssl_connstr user=3Dssltestuser dbname=3Dtrustdb hostaddr=3D$SE=
RVERHOSTADDR host=3Dcommon-name.pg-ssltest.test";
=20
 # Without the CRL, succeeds. With it, fails.
 $node->connect_ok(
@@ -349,8 +355,10 @@ $node->connect_fails(
 	"$common_connstr sslrootcert=3Dssl/root+server_ca.crt sslmode=3Dverify-ca=
 sslcrl=3Dssl/root+server.crl",
 	"does not connect with client-side CRL file",
 	expected_stderr =3D> qr/SSL error: certificate verify failed/);
+# sslcrl=3D'' is added here to override the invalid default, so as this
+# does not interfere with this case.
 $node->connect_fails(
-	"$common_connstr sslrootcert=3Dssl/root+server_ca.crt sslmode=3Dverify-ca=
 sslcrldir=3Dssl/root+server-crldir",
+	"$common_connstr sslcrl=3D'' sslrootcert=3Dssl/root+server_ca.crt sslmode=
=3Dverify-ca sslcrldir=3Dssl/root+server-crldir",
 	"does not connect with client-side CRL directory",
 	expected_stderr =3D> qr/SSL error: certificate verify failed/);
=20
@@ -392,7 +400,7 @@ $node->connect_fails(
 note "running server tests";
=20
 $common_connstr =3D
-  "sslrootcert=3Dssl/root+server_ca.crt sslmode=3Drequire dbname=3Dcertdb =
hostaddr=3D$SERVERHOSTADDR host=3Dlocalhost";
+  "$default_ssl_connstr sslrootcert=3Dssl/root+server_ca.crt sslmode=3Dreq=
uire dbname=3Dcertdb hostaddr=3D$SERVERHOSTADDR host=3Dlocalhost";
=20
 # no client cert
 $node->connect_fails(
@@ -569,7 +577,7 @@ $node->connect_fails(
 # works, iff username matches Common Name
 # fails, iff username doesn't match Common Name.
 $common_connstr =3D
-  "sslrootcert=3Dssl/root+server_ca.crt sslmode=3Drequire dbname=3Dverifyd=
b hostaddr=3D$SERVERHOSTADDR host=3Dlocalhost";
+  "$default_ssl_connstr sslrootcert=3Dssl/root+server_ca.crt sslmode=3Dreq=
uire dbname=3Dverifydb hostaddr=3D$SERVERHOSTADDR host=3Dlocalhost";
=20
 $node->connect_ok(
 	"$common_connstr user=3Dssltestuser sslcert=3Dssl/client.crt sslkey=3D$ke=
y{'client.key'}",
@@ -596,7 +604,7 @@ $node->connect_ok(
 # 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 =3D
-  "user=3Dssltestuser dbname=3Dcertdb sslkey=3D$key{'client.key'} sslrootc=
ert=3Dssl/root+server_ca.crt hostaddr=3D$SERVERHOSTADDR host=3Dlocalhost";
+  "$default_ssl_connstr user=3Dssltestuser dbname=3Dcertdb sslkey=3D$key{'=
client.key'} sslrootcert=3Dssl/root+server_ca.crt hostaddr=3D$SERVERHOSTADD=
R host=3Dlocalhost";
=20
 $node->connect_ok(
 	"$common_connstr sslmode=3Drequire sslcert=3Dssl/client+client_ca.crt",
diff --git a/src/test/ssl/t/003_sslinfo.pl b/src/test/ssl/t/003_sslinfo.pl
index 95742081f3..72eb6b860c 100644
--- a/src/test/ssl/t/003_sslinfo.pl
+++ b/src/test/ssl/t/003_sslinfo.pl
@@ -62,8 +62,13 @@ configure_test_server_for_ssl($node, $SERVERHOSTADDR, $S=
ERVERHOSTCIDR,
 # 001 test does.
 switch_server_cert($node, 'server-revoked');
=20
+# Set of default settings for SSL parameters in connection string.  This
+# makes the tests protected against any defaults the environment may have
+# in ~/.postgresql/.
+my $default_ssl_connstr =3D "sslkey=3Dinvalid sslcert=3Dinvalid sslrootcer=
t=3Dinvalid sslcrl=3Dinvalid sslcrldir=3Dinvalid";
+
 $common_connstr =3D
-  "sslrootcert=3Dssl/root+server_ca.crt sslmode=3Drequire dbname=3Dcertdb =
hostaddr=3D$SERVERHOSTADDR host=3Dlocalhost " .
+  "$default_ssl_connstr sslrootcert=3Dssl/root+server_ca.crt sslmode=3Dreq=
uire dbname=3Dcertdb hostaddr=3D$SERVERHOSTADDR host=3Dlocalhost " .
   "user=3Dssltestuser sslcert=3Dssl/client_ext.crt sslkey=3D$client_tmp_ke=
y";
=20
 # Make sure we can connect even though previous test suites have establish=
ed this
@@ -93,7 +98,7 @@ $result =3D $node->safe_psql("certdb", "SELECT ssl_client=
_cert_present();",
 is($result, 't', "ssl_client_cert_present() for connection with cert");
=20
 $result =3D $node->safe_psql("trustdb", "SELECT ssl_client_cert_present();=
",
-  connstr =3D> "sslrootcert=3Dssl/root+server_ca.crt sslmode=3Drequire " .
+  connstr =3D> "$default_ssl_connstr sslrootcert=3Dssl/root+server_ca.crt =
sslmode=3Drequire " .
   "dbname=3Dtrustdb hostaddr=3D$SERVERHOSTADDR user=3Dssltestuser host=3Dl=
ocalhost");
 is($result, 'f', "ssl_client_cert_present() for connection without cert");
=20
@@ -108,7 +113,7 @@ $result =3D $node->psql("certdb", "SELECT ssl_client_dn=
_field('invalid');",
 is($result, '3', "ssl_client_dn_field() for an invalid field");
=20
 $result =3D $node->safe_psql("trustdb", "SELECT ssl_client_dn_field('commo=
nName');",
-  connstr =3D> "sslrootcert=3Dssl/root+server_ca.crt sslmode=3Drequire " .
+  connstr =3D> "$default_ssl_connstr sslrootcert=3Dssl/root+server_ca.crt =
sslmode=3Drequire " .
   "dbname=3Dtrustdb hostaddr=3D$SERVERHOSTADDR user=3Dssltestuser host=3Dl=
ocalhost");
 is($result, '', "ssl_client_dn_field() for connection without cert");
=20

--2G7AuSfZ2crK4YqV--

--YMb4bSFe/eqRqJNM
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmIz2jwACgkQnvQgOdby
QH1fXg/9GXVin7wV86sfqvJDCLmkHoSPDKtZIXlTMltO5meX7YSEnJAgTlbMqsAZ
Ll9RbgqZNFQR/9lERnlWXQ6RwuWU5ugUl516TBMBg43pXxSkLav1fBTXKXQRdl7v
FaxerStjJaEzXRPqTIXhpU0cGR1upY8oigEd9WZHg/+dDYL7eD7h4iMJTFzfkHsF
fN6w7WZdQxp0Atx/sGfsnsCEWa9hUsV7C4hnaLa4TOPaBgGjyv41O/jurAp16193
zIFwEx2IX7pl6nZeva/MA2lSLnAPh1m2tnqfNFmShbtNM18qJ6YsCRd+fRO4a0BU
U5zLWjfDtz0r7H6WS+0MkfyM8uPrmz3XgbbRckcMrITjLNVgjMezT9NpGUMvnwY0
61dbFOrvqYDImwUEMerISXaPXZ/awSJy8Z/1Sl4blwdVDjW0hdXSJqrtEFQn+Qjv
AmJDtr9TSuoyf4qUpQh+CAfVpVeFK/PhLMfOODZhciWoBLrTXnS6FX0nbFNP7gmK
g7HsquZuGbmPh6IRZcOJku68XTAefcH62ocMMMxuxltYw+CV4VYOAYw06QeZheuv
R1CkZXvyindXicbSj6smIvQAMI7wPQzjxND93q6Tolz82WLNPQW77bK/Vo/1ioza
IkjnvSfVfG2FrMygbPckjfALJ1tNOO99C9i+PGXQK9xaPhtwzw8=
=uFKM
-----END PGP SIGNATURE-----

--YMb4bSFe/eqRqJNM--