On Mon, Oct 4, 2021 at 9:14 PM Bruce Momjian <br...@momjian.us> wrote:
> On Tue, Sep 28, 2021 at 02:54:39AM -0700, tho...@habets.se wrote:
> > And you say for complex setups. Fair enough. But currently I'd say the
> > default is wrong, and what should be default is not configurable.
>
> Agreed, I think this needs much more discussion and documentation.
I'd like to try to get this conversation started again. To pique
interest I've attached a new version of 0001, which implements
`sslrootcert=system` instead as suggested upthread. In 0002 I went
further and switched the default sslmode to `verify-full` when using
the system CA roots, because I feel pretty strongly that anyone
interested in using public CA systems is also interested in verifying
hostnames. (Otherwise, why make the switch?)
Notes:
- 0001, like Thomas' original patch, uses
SSL_CTX_set_default_verify_paths(). This will load both a default file
and a default directory. This is probably what most people want if
they're using the system roots -- just give me whatever the local
system wants me to use! -- but sslrootcert currently deals with files
only, I think. Is that a problem?
- The implementation in 0002 goes all the way down to
conninfo_add_defaults(). Maybe this is overly complex. Should I just
make sslmode a derived option, via connectOptions2()?
Thanks,
--Jacob
From 14311929a443f25f5064cdb01b57fae8d575e66d Mon Sep 17 00:00:00 2001
From: Jacob Champion <jchamp...@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
From 9bab777bbafab60d7e83c7f6da859c320a09e86e Mon Sep 17 00:00:00 2001
From: Jacob Champion <jchamp...@timescale.com>
Date: Mon, 24 Oct 2022 15:30:25 -0700
Subject: [PATCH v2 1/2] libpq: add sslrootcert=system to use default CAs
Based on a patch by Thomas Habets.
Discussion: https://www.postgresql.org/message-id/flat/CA%2BkHd%2BcJwCUxVb-Gj_0ptr3_KZPwi3%2B67vK6HnLFBK9MzuYrLA%40mail.gmail.com
---
doc/src/sgml/libpq.sgml | 17 +++++++++
doc/src/sgml/runtime.sgml | 6 ++-
src/interfaces/libpq/fe-secure-openssl.c | 26 ++++++++++++-
src/test/ssl/ssl/server-cn-only+server_ca.crt | 38 +++++++++++++++++++
src/test/ssl/sslfiles.mk | 6 ++-
src/test/ssl/t/001_ssltests.pl | 23 +++++++++++
6 files changed, 112 insertions(+), 4 deletions(-)
create mode 100644 src/test/ssl/ssl/server-cn-only+server_ca.crt
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 3c9bd3d673..1401b93e72 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1725,6 +1725,23 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
to be signed by one of these authorities. The default is
<filename>~/.postgresql/root.crt</filename>.
</para>
+ <para>
+ The special value <literal>system</literal> may be specified instead, in
+ which case the system's trusted CA roots will be loaded. The exact
+ locations of these root certificates differ by SSL implementation and
+ platform. For <productname>OpenSSL</productname> in particular, the
+ locations may be further modified by the <envar>SSL_CERT_DIR</envar>
+ and <envar>SSL_CERT_FILE</envar> environment variables.
+ </para>
+ <warning>
+ <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.
+ </para>
+ </warning>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/runtime.sgml b/doc/src/sgml/runtime.sgml
index 66367587b2..8184a3d54e 100644
--- a/doc/src/sgml/runtime.sgml
+++ b/doc/src/sgml/runtime.sgml
@@ -2006,7 +2006,11 @@ pg_dumpall -p 5432 | psql -d postgres -p 5433
(<xref linkend="ssl-tcp"/>). The TCP client must connect using
<literal>sslmode=verify-ca</literal> or
<literal>verify-full</literal> and have the appropriate root certificate
- file installed (<xref linkend="libq-ssl-certificates"/>).
+ 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.
</para>
<para>
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index b42a908733..4371cc7b91 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -1050,8 +1050,30 @@ initialize_SSL(PGconn *conn)
else
fnbuf[0] = '\0';
- if (fnbuf[0] != '\0' &&
- stat(fnbuf, &buf) == 0)
+ if (strcmp(fnbuf, "system") == 0)
+ {
+ /*
+ * The "system" sentinel value indicates that we should load whatever
+ * root certificates are installed for use by OpenSSL; these locations
+ * differ by platform. Note that the default system locations may be
+ * further overridden by the SSL_CERT_DIR and SSL_CERT_FILE environment
+ * variables.
+ */
+ if (SSL_CTX_set_default_verify_paths(SSL_context) != 1)
+ {
+ char *err = SSLerrmessage(ERR_get_error());
+
+ appendPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("could not load system root certificate paths: %s\n"),
+ err);
+ SSLerrfree(err);
+ SSL_CTX_free(SSL_context);
+ return -1;
+ }
+ have_rootcert = true;
+ }
+ else if (fnbuf[0] != '\0' &&
+ stat(fnbuf, &buf) == 0)
{
X509_STORE *cvstore;
diff --git a/src/test/ssl/ssl/server-cn-only+server_ca.crt b/src/test/ssl/ssl/server-cn-only+server_ca.crt
new file mode 100644
index 0000000000..9870e8c17a
--- /dev/null
+++ b/src/test/ssl/ssl/server-cn-only+server_ca.crt
@@ -0,0 +1,38 @@
+-----BEGIN CERTIFICATE-----
+MIIDAzCCAesCCCAhAwMUEgcBMA0GCSqGSIb3DQEBCwUAMEIxQDA+BgNVBAMMN1Rl
+c3QgQ0EgZm9yIFBvc3RncmVTUUwgU1NMIHJlZ3Jlc3Npb24gdGVzdCBzZXJ2ZXIg
+Y2VydHMwHhcNMjEwMzAzMjIxMjA3WhcNNDgwNzE5MjIxMjA3WjBGMR4wHAYDVQQL
+DBVQb3N0Z3JlU1FMIHRlc3Qgc3VpdGUxJDAiBgNVBAMMG2NvbW1vbi1uYW1lLnBn
+LXNzbHRlc3QudGVzdDCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBANWz
+VPMk7i5f+W0eEadRE+TTAtsIK08CkLMUnjs7zJkxnnm6RGBXPx6vK3AkAIi+wG4Y
+mXjYP3GuMiXaLjnWh2kzBSfIRQyNbTThnhSu3nDjAVkPexsSrPyiKimFuNgDfkGe
+5dQKa9Ag2SuVU4vd9SYxOMAiIFIC4ts4MLWWJf5D/PehdSuc0e5Me+91Nnbz90nl
+ds4lHvuDR+aKnZlTHmch3wfhXv7lNQImIBzfwl36Kd/bWB0fAEVFse3iZWmigaI/
+9FKh//WIq43TNLxn68OCQoyMe/HGjZDR/Xwo3rE6jg6/iAwSWib9yabfYPKbqq2G
+oFy6aYmmEquaDgLuX7kCAwEAATANBgkqhkiG9w0BAQsFAAOCAQEA2AZrD9cTQXTW
+4j2tT8N/TTc6WK2ncN4h22NTte6vK7MVwsZJCtw5ndYkmxcWkXAqiclzWyMdayds
+WOa12CEH7jKAhivF4Hcw3oO3JHM5BA6KzLWBVz9uZksOM6mPqn29DTKvA/Y1V8tj
+mxK/KUA68h/u6inu3mo4ywBpb/tqHxxg2cjyR0faCmM0pwRM0HBr/16fUMfO83nj
+QG8g9J/bybu5sYso/aSoC5nUNp4XjmDMdVLdqg/nTe/ejS8IfFr0WQxBlqooqFgx
+MSE+kX2e2fHsuOWSU/9eClt6FpQrwoC2C8F+/4g1Uz7Liqc4yMHPwjgeP9ewrrLO
+iIhlNNPqpQ==
+-----END CERTIFICATE-----
+-----BEGIN CERTIFICATE-----
+MIIDFDCCAfygAwIBAgIIICEDAxQSBwAwDQYJKoZIhvcNAQELBQAwQDE+MDwGA1UE
+Aww1VGVzdCByb290IENBIGZvciBQb3N0Z3JlU1FMIFNTTCByZWdyZXNzaW9uIHRl
+c3Qgc3VpdGUwHhcNMjEwMzAzMjIxMjA3WhcNNDgwNzE5MjIxMjA3WjBCMUAwPgYD
+VQQDDDdUZXN0IENBIGZvciBQb3N0Z3JlU1FMIFNTTCByZWdyZXNzaW9uIHRlc3Qg
+c2VydmVyIGNlcnRzMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA4kp2
+GW5nPb6QrSrtbClfZeutyQnHrm4TMPBoNepFdIVxBX/04BguM5ImDRze/huOWA+z
+atJAQqt3R7dsDwnOnPKUKCOuHX6a1aj5L86HtVgaWTXrZFE5NtL9PIzXkWu13UW0
+UesHtbPVRv6a6fB7Npph6hHy7iPZb009A8/lTJnxSPC39u/K/sPqjrVZaAJF+wDs
+qCxCZTUtAUFvWFnR/TeXLWlFzBupS1djgI7PltbJqSn6SKTAgNZTxpRJbu9Icp6J
+/50ELwT++0n0KWVXNHrDNfI5Gaa+SxClAsPsei2jLKpgR6QFC3wn38Z9q9LjAVuC
++FWhoN1uhYeoricEXwIDAQABoxAwDjAMBgNVHRMEBTADAQH/MA0GCSqGSIb3DQEB
+CwUAA4IBAQCdCA/EoXrustoV4jJGbkdXDuOUkBurwggSNBAqUBSDvCohRoD77Ecb
+QVuzPNxWKG+E4PwfUq2ha+2yPONEJ28ZgsbHq5qlJDMJ43wlcjn6wmmAJNeSpO8F
+0V9d2X/4wNZty9/zbwTnw26KChgDHumQ0WIbCoBtdqy8KDswYOvpgws6dqc021I7
+UrFo6vZek7VoApbJgkDL6qYADa6ApfW43ThH4sViFITeYt/kSHgmy2Udhs34jMM8
+xsFP/uYpRi1b1glenwSIKiHjD4/C9vnWQt5K3gRBvYukEj2Bw9VkNRpBVCi0cOoA
+OuwX3bwzNYNbZQv4K66oRpvuoEjCNeHg
+-----END CERTIFICATE-----
diff --git a/src/test/ssl/sslfiles.mk b/src/test/ssl/sslfiles.mk
index a843a21d42..9dbd9a2d2b 100644
--- a/src/test/ssl/sslfiles.mk
+++ b/src/test/ssl/sslfiles.mk
@@ -57,7 +57,8 @@ COMBINATIONS := \
ssl/root+server.crl \
ssl/root+client_ca.crt \
ssl/root+client.crl \
- ssl/client+client_ca.crt
+ ssl/client+client_ca.crt \
+ ssl/server-cn-only+server_ca.crt
CERTIFICATES := root_ca server_ca client_ca $(SERVERS) $(CLIENTS)
STANDARD_CERTS := $(CERTIFICATES:%=ssl/%.crt)
@@ -132,6 +133,9 @@ ssl/root+client_ca.crt: ssl/root_ca.crt ssl/client_ca.crt
# and for the client, to present to the server
ssl/client+client_ca.crt: ssl/client.crt ssl/client_ca.crt
+# for the server, to present to a client that only knows the root
+ssl/server-cn-only+server_ca.crt: ssl/server-cn-only.crt ssl/server_ca.crt
+
# If a CRL is used, OpenSSL requires a CRL file for *all* the CAs in the
# chain, even if some of them are empty.
ssl/root+server.crl: ssl/root.crl ssl/server.crl
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index efe5634fff..6ad51a9269 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -441,6 +441,29 @@ $node->connect_fails(
expected_stderr =>
qr/could not get server's host name from server certificate/);
+# Test system trusted roots.
+switch_server_cert($node,
+ certfile => 'server-cn-only+server_ca',
+ keyfile => 'server-cn-only',
+ cafile => 'root_ca');
+$common_connstr =
+ "$default_ssl_connstr user=ssltestuser dbname=trustdb sslrootcert=system hostaddr=$SERVERHOSTADDR";
+
+# By default our custom-CA-signed certificate should not be trusted.
+$node->connect_fails(
+ "$common_connstr sslmode=verify-full host=common-name.pg-ssltest.test",
+ "sslrootcert=system does not connect with private CA",
+ expected_stderr => qr/SSL error: certificate verify failed/);
+
+if ($ENV{with_ssl} eq 'openssl')
+{
+ # We can modify the definition of "system" to get it trusted again.
+ local $ENV{SSL_CERT_FILE} = $node->data_dir . "/root_ca.crt";
+ $node->connect_ok(
+ "$common_connstr sslmode=verify-full host=common-name.pg-ssltest.test",
+ "sslrootcert=system connects with overridden SSL_CERT_FILE");
+}
+
# Test that the CRL works
switch_server_cert($node, certfile => 'server-revoked');
--
2.25.1
