From 14311929a443f25f5064cdb01b57fae8d575e66d Mon Sep 17 00:00:00 2001
From: Jacob Champion <jchampion@timescale.com>
Date: Mon, 24 Oct 2022 15:24:11 -0700
Subject: [PATCH v2 2/2] libpq: default to verify-full for system CAs

Weaker verification modes don't make much sense for public CAs.
---
 src/interfaces/libpq/fe-connect.c | 25 +++++++++++++++++++++++++
 src/interfaces/libpq/t/001_uri.pl | 30 ++++++++++++++++++++++++++++--
 src/test/ssl/t/001_ssltests.pl    |  6 ++++++
 3 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 746e9b4f1e..92b87d3318 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -5876,6 +5876,7 @@ static bool
 conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage)
 {
 	PQconninfoOption *option;
+	PQconninfoOption *sslmode_default = NULL, *sslrootcert = NULL;
 	char	   *tmp;
 
 	/*
@@ -5892,6 +5893,9 @@ conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage)
 	 */
 	for (option = options; option->keyword != NULL; option++)
 	{
+		if (strcmp(option->keyword, "sslrootcert") == 0)
+			sslrootcert = option; /* save for later */
+
 		if (option->val != NULL)
 			continue;			/* Value was in conninfo or service */
 
@@ -5936,6 +5940,13 @@ conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage)
 				}
 				continue;
 			}
+
+			/*
+			 * sslmode is not specified. Let it be filled in with the compiled
+			 * default for now, but if sslrootcert=system, we'll override the
+			 * default later before returning.
+			 */
+			sslmode_default = option;
 		}
 
 		/*
@@ -5969,6 +5980,20 @@ conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage)
 		}
 	}
 
+	/*
+	 * Special handling for sslrootcert=system with no sslmode explicitly
+	 * defined. In this case we want to strengthen the default sslmode to
+	 * verify-full.
+	 */
+	if (sslmode_default && sslrootcert)
+	{
+		if (sslrootcert->val && strcmp(sslrootcert->val, "system") == 0)
+		{
+			free(sslmode_default->val);
+			sslmode_default->val = strdup("verify-full");
+		}
+	}
+
 	return true;
 }
 
diff --git a/src/interfaces/libpq/t/001_uri.pl b/src/interfaces/libpq/t/001_uri.pl
index beaf13b49c..ccfcd77680 100644
--- a/src/interfaces/libpq/t/001_uri.pl
+++ b/src/interfaces/libpq/t/001_uri.pl
@@ -8,7 +8,9 @@ use IPC::Run;
 
 
 # List of URIs tests. For each test the first element is the input string, the
-# second the expected stdout and the third the expected stderr.
+# second the expected stdout and the third the expected stderr. Optionally,
+# additional arguments may specify key/value pairs which will override
+# environment variables for the duration of the test.
 my @tests = (
 	[
 		q{postgresql://uri-user:secret@host:12345/db},
@@ -209,20 +211,44 @@ my @tests = (
 		q{postgres://%2Fvar%2Flib%2Fpostgresql/dbname},
 		q{dbname='dbname' host='/var/lib/postgresql' (local)},
 		q{},
+	],
+	# Usually the default sslmode is 'prefer' (for libraries with SSL) or
+	# 'disable' (for those without). This default changes to 'verify-full' if
+	# the system CA store is in use.
+	[
+		q{postgresql://host?sslmode=disable},
+		q{host='host' sslmode='disable' (inet)},
+		q{},
+		PGSSLROOTCERT => "system",
+	],
+	[
+		q{postgresql://host?sslmode=prefer},
+		q{host='host' sslmode='prefer' (inet)},
+		q{},
+		PGSSLROOTCERT => "system",
+	],
+	[
+		q{postgresql://host?sslmode=verify-full},
+		q{host='host' (inet)},
+		q{},
+		PGSSLROOTCERT => "system",
 	]);
 
 # test to run for each of the above test definitions
 sub test_uri
 {
 	local $Test::Builder::Level = $Test::Builder::Level + 1;
+	local %ENV = %ENV;
 
 	my $uri;
 	my %expect;
+	my %envvars;
 	my %result;
 
-	($uri, $expect{stdout}, $expect{stderr}) = @$_;
+	($uri, $expect{stdout}, $expect{stderr}, %envvars) = @$_;
 
 	$expect{'exit'} = $expect{stderr} eq '';
+	%ENV = (%ENV, %envvars);
 
 	my $cmd = [ 'libpq_uri_regress', $uri ];
 	$result{exit} = IPC::Run::run $cmd, '>', \$result{stdout}, '2>',
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index 6ad51a9269..e4be13a23e 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -462,6 +462,12 @@ if ($ENV{with_ssl} eq 'openssl')
 	$node->connect_ok(
 		"$common_connstr sslmode=verify-full host=common-name.pg-ssltest.test",
 		"sslrootcert=system connects with overridden SSL_CERT_FILE");
+
+	# verify-full mode should be the default for system CAs.
+	$node->connect_fails(
+		"$common_connstr host=common-name.pg-ssltest.test.bad",
+		"sslrootcert=system defaults to sslmode=verify-full",
+		expected_stderr => qr/server certificate for "common-name.pg-ssltest.test" does not match host name "common-name.pg-ssltest.test.bad"/);
 }
 
 # Test that the CRL works
-- 
2.25.1