v8-0002-libpq-force-sslmode-verify-full-for-system-CAs.patch

From 11b69d0bc0da163fbea8d95f8bad53bc7bbbac8c Mon Sep 17 00:00:00 2001
From: Jacob Champion <jchampion@timescale.com>
Date: Mon, 24 Oct 2022 15:24:11 -0700
Subject: [PATCH v8 2/2] libpq: force sslmode=verify-full for system CAs

Weaker verification modes don't make much sense for public CAs.
---
 doc/src/sgml/libpq.sgml           | 15 +++++----
 doc/src/sgml/runtime.sgml         |  6 ++--
 src/interfaces/libpq/fe-connect.c | 56 +++++++++++++++++++++++++++++++
 src/interfaces/libpq/t/001_uri.pl | 30 +++++++++++++++--
 src/test/ssl/t/001_ssltests.pl    | 12 +++++++
 5 files changed, 107 insertions(+), 12 deletions(-)

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index ea3458dd10..5a1712a129 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1838,15 +1838,16 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
         locations may be further modified by the <envar>SSL_CERT_DIR</envar>
         and <envar>SSL_CERT_FILE</envar> environment variables.
        </para>
-       <warning>
+       <note>
         <para>
-         When using <literal>sslrootcert=system</literal>, it is critical to
-         also use the strongest certificate verification method,
-         <literal>sslmode=verify-full</literal>. In most cases it is trivial for
-         anyone to obtain a certificate trusted by the system for a hostname
-         they control, rendering the <literal>verify-ca</literal> mode useless.
+         When using <literal>sslrootcert=system</literal>, the default
+         <literal>sslmode</literal> is changed to <literal>verify-full</literal>,
+         and any weaker setting will result in an error. In most cases it is
+         trivial for anyone to obtain a certificate trusted by the system for a
+         hostname they control, rendering <literal>verify-ca</literal> and all
+         weaker modes useless.
         </para>
-       </warning>
+       </note>
       </listitem>
      </varlistentry>
 
diff --git a/doc/src/sgml/runtime.sgml b/doc/src/sgml/runtime.sgml
index b93184537a..dbe23db54f 100644
--- a/doc/src/sgml/runtime.sgml
+++ b/doc/src/sgml/runtime.sgml
@@ -2009,9 +2009,9 @@ pg_dumpall -p 5432 | psql -d postgres -p 5433
    <literal>verify-full</literal> and have the appropriate root certificate
    file installed (<xref linkend="libq-ssl-certificates"/>). Alternatively the
    system CA pool can be used using <literal>sslrootcert=system</literal>; in
-   this case, <literal>sslmode=verify-full</literal> must be used for safety,
-   since it is generally trivial to obtain certificates which are signed by a
-   public CA.
+   this case, <literal>sslmode=verify-full</literal> is forced for safety, since
+   it is generally trivial to obtain certificates which are signed by a public
+   CA.
   </para>
 
   <para>
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index dd4b98e099..a75cdab028 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -1428,6 +1428,22 @@ connectOptions2(PGconn *conn)
 			goto oom_error;
 	}
 
+#ifndef USE_SSL
+	/*
+	 * sslrootcert=system is not supported. Since setting this changes the
+	 * default sslmode, check this _before_ we validate sslmode, to avoid
+	 * confusing the user with errors for an option they may not have set.
+	 */
+	if (conn->sslrootcert
+		&& strcmp(conn->sslrootcert, "system") == 0)
+	{
+		conn->status = CONNECTION_BAD;
+		libpq_append_conn_error(conn, "sslrootcert value \"%s\" invalid when SSL support is not compiled in",
+								conn->sslrootcert);
+		return false;
+	}
+#endif
+
 	/*
 	 * validate sslmode option
 	 */
@@ -1474,6 +1490,21 @@ connectOptions2(PGconn *conn)
 			goto oom_error;
 	}
 
+#ifdef USE_SSL
+	/*
+	 * If sslrootcert=system, make sure our chosen sslmode is compatible.
+	 */
+	if (conn->sslrootcert
+		&& strcmp(conn->sslrootcert, "system") == 0
+		&& strcmp(conn->sslmode, "verify-full") != 0)
+	{
+		conn->status = CONNECTION_BAD;
+		libpq_append_conn_error(conn, "weak sslmode \"%s\" may not be used with sslrootcert=system",
+								conn->sslmode);
+		return false;
+	}
+#endif
+
 	/*
 	 * Validate TLS protocol versions for ssl_min_protocol_version and
 	 * ssl_max_protocol_version.
@@ -6008,6 +6039,7 @@ static bool
 conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage)
 {
 	PQconninfoOption *option;
+	PQconninfoOption *sslmode_default = NULL, *sslrootcert = NULL;
 	char	   *tmp;
 
 	/*
@@ -6024,6 +6056,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 */
 
@@ -6066,6 +6101,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;
 		}
 
 		/*
@@ -6098,6 +6140,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 2ab537f97f..cd659bc1b0 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 6051ee226d..571268b4a5 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -459,6 +459,12 @@ $node->connect_fails(
 	"sslrootcert=system does not connect with private CA",
 	expected_stderr => qr/SSL error: certificate verify failed/);
 
+# Modes other than verify-full cannot be mixed with sslrootcert=system.
+$node->connect_fails(
+	"$common_connstr sslmode=verify-ca host=common-name.pg-ssltest.test",
+	"sslrootcert=system only accepts sslmode=verify-full",
+	expected_stderr => qr/weak sslmode "verify-ca" may not be used with sslrootcert=system/);
+
 SKIP:
 {
 	skip "SSL_CERT_FILE is not supported with LibreSSL" if $libressl;
@@ -468,6 +474,12 @@ SKIP:
 	$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